Re: [PATCH v2 2/3] drm/msm/dsi: Set PHY usescase (and mode) before registering DSI host
On Sun, Feb 09, 2025 at 10:42:53PM +0100, Marijn Suijten wrote:
> Ordering issues here cause an uninitialized (default STANDALONE)
> usecase to be programmed (which appears to be a MUX) in some cases
> when msm_dsi_host_register() is called, leading to the slave PLL in
> bonded-DSI mode to source from a clock parent (dsi1vco) that is off.
>
> This should seemingly not be a problem as the actual dispcc clocks from
> DSI1 that are muxed in the clock tree of DSI0 are way further down, this
> bit still seems to have an effect on them somehow and causes the right
> side of the panel controlled by DSI1 to not function.
>
> In an ideal world this code is refactored to no longer have such
> error-prone calls "across subsystems", and instead model the "PLL src"
> register field as a regular mux so that changing the clock parents
> programmatically or in DTS via `assigned-clock-parents` has the
> desired effect.
> But for the avid reader, the clocks that we *are* muxing into DSI0's
> tree are way further down, so if this bit turns out to be a simple mux
> between dsiXvco and out_div, that shouldn't have any effect as this
> whole tree is off anyway.
>
> Signed-off-by: Marijn Suijten
> ---
> drivers/gpu/drm/msm/dsi/dsi_manager.c | 30 +++---
> 1 file changed, 19 insertions(+), 11 deletions(-)
Fixes: 57bf43389337 ("drm/msm/dsi: Pass down use case to PHY")
Reviewed-by: Dmitry Baryshkov
>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> index
> a210b7c9e5ca281a46fbdb226e25832719a684ea..b93205c034e4acc73d536deeddce6ebd694b4a80
> 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> @@ -74,17 +74,33 @@ static int dsi_mgr_setup_components(int id)
> int ret;
>
> if (!IS_BONDED_DSI()) {
> + /* Set the usecase before calling msm_dsi_host_register(),
> which would
> + * already program the PLL source mux based on a default
> usecase.
> + */
> + msm_dsi_phy_set_usecase(msm_dsi->phy, MSM_DSI_PHY_STANDALONE);
> + msm_dsi_host_set_phy_mode(msm_dsi->host, msm_dsi->phy);
> +
> ret = msm_dsi_host_register(msm_dsi->host);
> if (ret)
> return ret;
> -
> - msm_dsi_phy_set_usecase(msm_dsi->phy, MSM_DSI_PHY_STANDALONE);
> - msm_dsi_host_set_phy_mode(msm_dsi->host, msm_dsi->phy);
> } else if (other_dsi) {
> struct msm_dsi *master_link_dsi = IS_MASTER_DSI_LINK(id) ?
> msm_dsi : other_dsi;
> struct msm_dsi *slave_link_dsi = IS_MASTER_DSI_LINK(id) ?
> other_dsi : msm_dsi;
> +
> + /* PLL0 is to drive both DSI link clocks in bonded DSI mode.
> + *
> + /* Set the usecase before calling msm_dsi_host_register(),
> which would
> + * already program the PLL source mux based on a default
> usecase.
> + */
> + msm_dsi_phy_set_usecase(clk_master_dsi->phy,
> + MSM_DSI_PHY_MASTER);
> + msm_dsi_phy_set_usecase(clk_slave_dsi->phy,
> + MSM_DSI_PHY_SLAVE);
> + msm_dsi_host_set_phy_mode(msm_dsi->host, msm_dsi->phy);
> + msm_dsi_host_set_phy_mode(other_dsi->host, other_dsi->phy);
> +
> /* Register slave host first, so that slave DSI device
>* has a chance to probe, and do not block the master
>* DSI device's probe.
> @@ -98,14 +114,6 @@ static int dsi_mgr_setup_components(int id)
> ret = msm_dsi_host_register(master_link_dsi->host);
> if (ret)
> return ret;
> -
> - /* PLL0 is to drive both 2 DSI link clocks in bonded DSI mode.
> */
> - msm_dsi_phy_set_usecase(clk_master_dsi->phy,
> - MSM_DSI_PHY_MASTER);
> - msm_dsi_phy_set_usecase(clk_slave_dsi->phy,
> - MSM_DSI_PHY_SLAVE);
> - msm_dsi_host_set_phy_mode(msm_dsi->host, msm_dsi->phy);
> - msm_dsi_host_set_phy_mode(other_dsi->host, other_dsi->phy);
> }
>
> return 0;
>
> --
> 2.48.1
>
--
With best wishes
Dmitry
Re: [PATCH v2 3/3] drm/msm/dpu: Remove arbitrary limit of 1 interface in DSC topology
On Sun, Feb 09, 2025 at 10:42:54PM +0100, Marijn Suijten wrote:
> When DSC is enabled the number of interfaces is forced to be 1, and
> documented that it is a "power-optimal" layout to use two DSC encoders
> together with two Layer Mixers. However, the same layout (two DSC
> hard-slice encoders with two LMs) is also used when the display is
> fed with data over two instead of one interface (common on 4k@120Hz
> smartphone panels with Dual-DSI). Solve this by simply removing the
> num_intf = 1 assignment as the count is already calculated by computing
> the number of physical encoders within the virtual encoder.
>
> Fixes: 7e9cc175b159 ("drm/msm/disp/dpu1: Add support for DSC in topology")
> Signed-off-by: Marijn Suijten
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
Reviewed-by: Dmitry Baryshkov
--
With best wishes
Dmitry
[PATCH v2 1/3] drm/msm/dsi: Use existing per-interface slice count in DSC timing
When configuring the timing of DSI hosts (interfaces) in
dsi_timing_setup() all values written to registers are taking
bonded-mode into account by dividing the original mode width by 2
(half the data is sent over each of the two DSI hosts), but the full
width instead of the interface width is passed as hdisplay parameter to
dsi_update_dsc_timing().
Currently only msm_dsc_get_slices_per_intf() is called within
dsi_update_dsc_timing() with the `hdisplay` argument which clearly
documents that it wants the width of a single interface (which, again,
in bonded DSI mode is half the total width of the mode) resulting in all
subsequent values to be completely off.
However, as soon as we start to pass the halved hdisplay
into dsi_update_dsc_timing() we might as well discard
msm_dsc_get_slices_per_intf() since the value it calculates is already
available in dsc->slice_count which is per-interface by the current
design of MSM DPU/DSI implementations and their use of the DRM DSC
helpers.
Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
Signed-off-by: Marijn Suijten
---
drivers/gpu/drm/msm/dsi/dsi_host.c | 8
drivers/gpu/drm/msm/msm_dsc_helper.h | 11 ---
2 files changed, 4 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index
007311c21fdaa0462b05d53cd8a2aad0269b1727..42e100a8adca09d7b55afce0e2553e76d898744f
100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -846,7 +846,7 @@ static void dsi_ctrl_enable(struct msm_dsi_host *msm_host,
dsi_write(msm_host, REG_DSI_CPHY_MODE_CTRL, BIT(0));
}
-static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool
is_cmd_mode, u32 hdisplay)
+static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool
is_cmd_mode)
{
struct drm_dsc_config *dsc = msm_host->dsc;
u32 reg, reg_ctrl, reg_ctrl2;
@@ -858,7 +858,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host
*msm_host, bool is_cmd_mod
/* first calculate dsc parameters and then program
* compress mode registers
*/
- slice_per_intf = msm_dsc_get_slices_per_intf(dsc, hdisplay);
+ slice_per_intf = dsc->slice_count;
total_bytes_per_intf = dsc->slice_chunk_size * slice_per_intf;
bytes_per_pkt = dsc->slice_chunk_size; /* * slice_per_pkt; */
@@ -991,7 +991,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host,
bool is_bonded_dsi)
if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO) {
if (msm_host->dsc)
- dsi_update_dsc_timing(msm_host, false, mode->hdisplay);
+ dsi_update_dsc_timing(msm_host, false);
dsi_write(msm_host, REG_DSI_ACTIVE_H,
DSI_ACTIVE_H_START(ha_start) |
@@ -1012,7 +1012,7 @@ static void dsi_timing_setup(struct msm_dsi_host
*msm_host, bool is_bonded_dsi)
DSI_ACTIVE_VSYNC_VPOS_END(vs_end));
} else {/* command mode */
if (msm_host->dsc)
- dsi_update_dsc_timing(msm_host, true, mode->hdisplay);
+ dsi_update_dsc_timing(msm_host, true);
/* image data and 1 byte write_memory_start cmd */
if (!msm_host->dsc)
diff --git a/drivers/gpu/drm/msm/msm_dsc_helper.h
b/drivers/gpu/drm/msm/msm_dsc_helper.h
index
b9049fe1e2790703a6f42dd7e6cd3fa5eea29389..63f95523b2cbb48f822210ac47cdd3526f231a89
100644
--- a/drivers/gpu/drm/msm/msm_dsc_helper.h
+++ b/drivers/gpu/drm/msm/msm_dsc_helper.h
@@ -12,17 +12,6 @@
#include
#include
-/**
- * msm_dsc_get_slices_per_intf() - calculate number of slices per interface
- * @dsc: Pointer to drm dsc config struct
- * @intf_width: interface width in pixels
- * Returns: Integer representing the number of slices for the given interface
- */
-static inline u32 msm_dsc_get_slices_per_intf(const struct drm_dsc_config
*dsc, u32 intf_width)
-{
- return DIV_ROUND_UP(intf_width, dsc->slice_width);
-}
-
/**
* msm_dsc_get_bytes_per_line() - calculate bytes per line
* @dsc: Pointer to drm dsc config struct
--
2.48.1
[PATCH v2 2/3] drm/msm/dsi: Set PHY usescase (and mode) before registering DSI host
Ordering issues here cause an uninitialized (default STANDALONE)
usecase to be programmed (which appears to be a MUX) in some cases
when msm_dsi_host_register() is called, leading to the slave PLL in
bonded-DSI mode to source from a clock parent (dsi1vco) that is off.
This should seemingly not be a problem as the actual dispcc clocks from
DSI1 that are muxed in the clock tree of DSI0 are way further down, this
bit still seems to have an effect on them somehow and causes the right
side of the panel controlled by DSI1 to not function.
In an ideal world this code is refactored to no longer have such
error-prone calls "across subsystems", and instead model the "PLL src"
register field as a regular mux so that changing the clock parents
programmatically or in DTS via `assigned-clock-parents` has the
desired effect.
But for the avid reader, the clocks that we *are* muxing into DSI0's
tree are way further down, so if this bit turns out to be a simple mux
between dsiXvco and out_div, that shouldn't have any effect as this
whole tree is off anyway.
Signed-off-by: Marijn Suijten
---
drivers/gpu/drm/msm/dsi/dsi_manager.c | 30 +++---
1 file changed, 19 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c
b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index
a210b7c9e5ca281a46fbdb226e25832719a684ea..b93205c034e4acc73d536deeddce6ebd694b4a80
100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -74,17 +74,33 @@ static int dsi_mgr_setup_components(int id)
int ret;
if (!IS_BONDED_DSI()) {
+ /* Set the usecase before calling msm_dsi_host_register(),
which would
+* already program the PLL source mux based on a default
usecase.
+*/
+ msm_dsi_phy_set_usecase(msm_dsi->phy, MSM_DSI_PHY_STANDALONE);
+ msm_dsi_host_set_phy_mode(msm_dsi->host, msm_dsi->phy);
+
ret = msm_dsi_host_register(msm_dsi->host);
if (ret)
return ret;
-
- msm_dsi_phy_set_usecase(msm_dsi->phy, MSM_DSI_PHY_STANDALONE);
- msm_dsi_host_set_phy_mode(msm_dsi->host, msm_dsi->phy);
} else if (other_dsi) {
struct msm_dsi *master_link_dsi = IS_MASTER_DSI_LINK(id) ?
msm_dsi : other_dsi;
struct msm_dsi *slave_link_dsi = IS_MASTER_DSI_LINK(id) ?
other_dsi : msm_dsi;
+
+ /* PLL0 is to drive both DSI link clocks in bonded DSI mode.
+*
+ /* Set the usecase before calling msm_dsi_host_register(),
which would
+* already program the PLL source mux based on a default
usecase.
+*/
+ msm_dsi_phy_set_usecase(clk_master_dsi->phy,
+ MSM_DSI_PHY_MASTER);
+ msm_dsi_phy_set_usecase(clk_slave_dsi->phy,
+ MSM_DSI_PHY_SLAVE);
+ msm_dsi_host_set_phy_mode(msm_dsi->host, msm_dsi->phy);
+ msm_dsi_host_set_phy_mode(other_dsi->host, other_dsi->phy);
+
/* Register slave host first, so that slave DSI device
* has a chance to probe, and do not block the master
* DSI device's probe.
@@ -98,14 +114,6 @@ static int dsi_mgr_setup_components(int id)
ret = msm_dsi_host_register(master_link_dsi->host);
if (ret)
return ret;
-
- /* PLL0 is to drive both 2 DSI link clocks in bonded DSI mode.
*/
- msm_dsi_phy_set_usecase(clk_master_dsi->phy,
- MSM_DSI_PHY_MASTER);
- msm_dsi_phy_set_usecase(clk_slave_dsi->phy,
- MSM_DSI_PHY_SLAVE);
- msm_dsi_host_set_phy_mode(msm_dsi->host, msm_dsi->phy);
- msm_dsi_host_set_phy_mode(other_dsi->host, other_dsi->phy);
}
return 0;
--
2.48.1
[PATCH v2 3/3] drm/msm/dpu: Remove arbitrary limit of 1 interface in DSC topology
When DSC is enabled the number of interfaces is forced to be 1, and
documented that it is a "power-optimal" layout to use two DSC encoders
together with two Layer Mixers. However, the same layout (two DSC
hard-slice encoders with two LMs) is also used when the display is
fed with data over two instead of one interface (common on 4k@120Hz
smartphone panels with Dual-DSI). Solve this by simply removing the
num_intf = 1 assignment as the count is already calculated by computing
the number of physical encoders within the virtual encoder.
Fixes: 7e9cc175b159 ("drm/msm/disp/dpu1: Add support for DSC in topology")
Signed-off-by: Marijn Suijten
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index
b585cd17462345f94bcc2ddd57902cc7c312ae63..b0870318471bd7cceda70fd15ea7bcc8658af604
100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -686,20 +686,21 @@ static struct msm_display_topology
dpu_encoder_get_topology(
if (dsc) {
/*
-* Use 2 DSC encoders and 2 layer mixers per single interface
+* Use 2 DSC encoders, 2 layer mixers and 1 or 2 interfaces
* when Display Stream Compression (DSC) is enabled,
* and when enough DSC blocks are available.
* This is power-optimal and can drive up to (including) 4k
* screens.
*/
- if (dpu_kms->catalog->dsc_count >= 2) {
+ WARN(topology.num_intf > 2,
+"DSC topology cannot support more than 2 interfaces\n");
+ if (intf_count >= 2 || dpu_kms->catalog->dsc_count >= 2) {
topology.num_dsc = 2;
topology.num_lm = 2;
} else {
topology.num_dsc = 1;
topology.num_lm = 1;
}
- topology.num_intf = 1;
}
return topology;
--
2.48.1
[PATCH v2 0/3] drm/msm: Initial fixes for DUALPIPE (+DSC) topology
This series covers a step-up towards supporting the DUALPIPE DSC topology, also known as 2:2:2 topology (on active-CTL hardware). It involves 2 layer mixers, 2 DSC compression encoders, and 2 interfaces (on DSI, this is called bonded-DSI) where bandwidth constraints (e.g. 4k panels at 120Hz) require two interfaces to transmit pixel data. Enabling this topology will be hard(er) than downstream as hacking a layout type in DTS won't be describing the hardware, but "dynamically" determining it at runtime may pose some of a challenge that is left to a future series. Such changes will also involve the 1:1:1 topology needed for constrained hardware like the Fairphone 5 on SC7280 with access to only one DSC encoder and thus ruled out of the current 2:2:1 topology. Likewise, the patches and discussions around improving active-CTL configuration to support bonded interfaces (that share a single CTL block) are still in full swing and hence elided from this series, apart from one patch to fix the ACTIVE_DSC register coding to support updates, so that it is not forgotten about. This issue and successful resolution of all the problems is discussed and demonstrated in https://gitlab.freedesktop.org/drm/msm/-/issues/41. Signed-off-by: Marijn Suijten --- Changes in v2: - Dropped patches that were applied; - dsi_mgr_setup_components() now sets both the usecase and phy_mode prior to calling msm_dsi_host_register(), and for non-bonded too; - Added patch to remove a forced num_intf = 1 when DSC is enabled; - Reworked hdisplay/2 "fix" when calculating "DSC timing" to instead use dsc->slice_count, allowing us to remove msm_dsc_get_slices_per_intf() entirely; - Link to v1: https://lore.kernel.org/r/20240417-drm-msm-initial-dualpipe-dsc-fixes-v1-0-78ae3ee9a...@somainline.org Depends on: - https://lore.kernel.org/linux-arm-msm/20250122-dpu-111-topology-v2-1-505e95964...@somainline.org/ (only to prevent conflicts with the patch that removes a hardcoded num_intf = 1;). --- Marijn Suijten (3): drm/msm/dsi: Use existing per-interface slice count in DSC timing drm/msm/dsi: Set PHY usescase (and mode) before registering DSI host drm/msm/dpu: Remove arbitrary limit of 1 interface in DSC topology drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 7 --- drivers/gpu/drm/msm/dsi/dsi_host.c | 8 drivers/gpu/drm/msm/dsi/dsi_manager.c | 30 ++--- drivers/gpu/drm/msm/msm_dsc_helper.h| 11 --- 4 files changed, 27 insertions(+), 29 deletions(-) --- base-commit: ed58d103e6da15a442ff87567898768dc3a66987 change-id: 20240416-drm-msm-initial-dualpipe-dsc-fixes-3f0715b03bf4 prerequisite-message-id: <20250122-dpu-111-topology-v2-1-505e95964...@somainline.org> prerequisite-patch-id: 9ed44ae089b173f452a6603e6739b0b3bf2d9274 Best regards, -- Marijn Suijten
Re: [PATCH] drm/msm/dpu: Fix uninitialized variable
On Sun, Feb 09, 2025 at 10:32:33PM -0500, Ethan Carter Edwards wrote:
> There is a possibility for an uninitialized *ret* variable to be
> returned in some code paths.
>
> Fix this by initializing *ret* to 0.
>
> Addresses-Coverity-ID: 1642546 ("Uninitialized scalar variable")
> Fixes: 774bcfb731765d ("drm/msm/dpu: add support for virtual planes")
> Signed-off-by: Ethan Carter Edwards
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index
> 098abc2c0003cde90ce6219c97ee18fa055a92a5..74edaa9ecee72111b70f32b832486aeebe545a28
> 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -1164,7 +1164,7 @@ int dpu_assign_plane_resources(struct dpu_global_state
> *global_state,
> unsigned int num_planes)
> {
> unsigned int i;
> - int ret;
> + int ret = 0;
Thanks, but I think it better to make the function return ret from
within the loop and return explicit 0 if there was no error.
>
> for (i = 0; i < num_planes; i++) {
> struct drm_plane_state *plane_state = states[i];
>
> ---
> base-commit: a64dcfb451e254085a7daee5fe51bf22959d52d3
> change-id: 20250209-dpu-c3fac78fc617
>
> Best regards,
> --
> Ethan Carter Edwards
>
--
With best wishes
Dmitry
Re: [PATCH v2] drm/msm/dpu: Fix uninitialized variable
On Sun, Feb 09, 2025 at 10:51:54PM -0500, Ethan Carter Edwards wrote:
> There is a possibility for an uninitialized *ret* variable to be
> returned in some code paths.
>
> Fix this by initializing *ret* to 0.
>
> Addresses-Coverity-ID: 1642546 ("Uninitialized scalar variable")
> Fixes: 774bcfb731765d ("drm/msm/dpu: add support for virtual planes")
> Signed-off-by: Ethan Carter Edwards
> ---
> Changes in v2:
> - Return explicit 0 when no error occurs
> - Add hardening mailing lists
> - Link to v1:
> https://lore.kernel.org/r/20250209-dpu-v1-1-0db666884...@ethancedwards.com
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
Reviewed-by: Dmitry Baryshkov
--
With best wishes
Dmitry
Re: [PATCH v2 2/3] drm/msm/dsi: Set PHY usescase (and mode) before registering DSI host
On 2/9/2025 1:42 PM, Marijn Suijten wrote: Ordering issues here cause an uninitialized (default STANDALONE) usecase to be programmed (which appears to be a MUX) in some cases when msm_dsi_host_register() is called, leading to the slave PLL in bonded-DSI mode to source from a clock parent (dsi1vco) that is off. This should seemingly not be a problem as the actual dispcc clocks from DSI1 that are muxed in the clock tree of DSI0 are way further down, this bit still seems to have an effect on them somehow and causes the right side of the panel controlled by DSI1 to not function. In an ideal world this code is refactored to no longer have such error-prone calls "across subsystems", and instead model the "PLL src" register field as a regular mux so that changing the clock parents programmatically or in DTS via `assigned-clock-parents` has the desired effect. But for the avid reader, the clocks that we *are* muxing into DSI0's tree are way further down, so if this bit turns out to be a simple mux between dsiXvco and out_div, that shouldn't have any effect as this whole tree is off anyway. Signed-off-by: Marijn Suijten --- drivers/gpu/drm/msm/dsi/dsi_manager.c | 30 +++--- 1 file changed, 19 insertions(+), 11 deletions(-) Reviewed-by: Abhinav Kumar
[PATCH v2 1/3] drm/display: bridge-connector: add DisplayPort bridges
DRM HDMI Codec framework is useful not only for the HDMI bridges, but
also for the DisplayPort bridges. Add new DRM_BRIDGE_OP_DisplayPort
define in order to distinguish DP bridges. Create HDMI codec device
automatically for DP bridges which have declared audio support.
Note, unlike HDMI devices, which already have a framework to handle HPD
notifications in a standard way, DP drivers don't (yet?) have such a
framework. As such it is necessary to manually call
drm_connector_hdmi_audio_plugged_notify(). This requirement hopefully
can be lifted later on, if/when DRM framework gets better DisplayPort
ports support in the core layer.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/display/drm_bridge_connector.c | 66 --
include/drm/drm_bridge.h | 14 +-
2 files changed, 65 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c
b/drivers/gpu/drm/display/drm_bridge_connector.c
index
30c736fc0067e31a97db242e5b16ea8a5b4cf359..5e031395b801f9a1371dcb4ac09f3da23e4615dd
100644
--- a/drivers/gpu/drm/display/drm_bridge_connector.c
+++ b/drivers/gpu/drm/display/drm_bridge_connector.c
@@ -98,6 +98,13 @@ struct drm_bridge_connector {
* HDMI connector infrastructure, if any (see &DRM_BRIDGE_OP_HDMI).
*/
struct drm_bridge *bridge_hdmi;
+ /**
+* @bridge_dp:
+*
+* The bridge in the chain that implements necessary support for the
+* DisplayPort connector infrastructure, if any (see
&DRM_BRIDGE_OP_DisplayPort).
+*/
+ struct drm_bridge *bridge_dp;
};
#define to_drm_bridge_connector(x) \
@@ -496,6 +503,25 @@ static const struct drm_connector_hdmi_audio_funcs
drm_bridge_connector_hdmi_aud
.mute_stream = drm_bridge_connector_audio_mute_stream,
};
+static int drm_bridge_connector_hdmi_audio_init(struct drm_connector
*connector,
+ struct drm_bridge *bridge)
+{
+ if (!bridge->hdmi_audio_max_i2s_playback_channels &&
+ !bridge->hdmi_audio_spdif_playback)
+ return 0;
+
+ if (!bridge->funcs->hdmi_audio_prepare ||
+ !bridge->funcs->hdmi_audio_shutdown)
+ return -EINVAL;
+
+ return drm_connector_hdmi_audio_init(connector,
+bridge->hdmi_audio_dev,
+
&drm_bridge_connector_hdmi_audio_funcs,
+
bridge->hdmi_audio_max_i2s_playback_channels,
+bridge->hdmi_audio_spdif_playback,
+bridge->hdmi_audio_dai_port);
+}
+
/*
-
* Bridge Connector Initialisation
*/
@@ -564,6 +590,8 @@ struct drm_connector *drm_bridge_connector_init(struct
drm_device *drm,
if (bridge->ops & DRM_BRIDGE_OP_HDMI) {
if (bridge_connector->bridge_hdmi)
return ERR_PTR(-EBUSY);
+ if (bridge_connector->bridge_dp)
+ return ERR_PTR(-EINVAL);
if (!bridge->funcs->hdmi_write_infoframe ||
!bridge->funcs->hdmi_clear_infoframe)
return ERR_PTR(-EINVAL);
@@ -576,6 +604,16 @@ struct drm_connector *drm_bridge_connector_init(struct
drm_device *drm,
max_bpc = bridge->max_bpc;
}
+ if (bridge->ops & DRM_BRIDGE_OP_DisplayPort) {
+ if (bridge_connector->bridge_dp)
+ return ERR_PTR(-EBUSY);
+ if (bridge_connector->bridge_hdmi)
+ return ERR_PTR(-EINVAL);
+
+ bridge_connector->bridge_dp = bridge;
+
+ }
+
if (!drm_bridge_get_next_bridge(bridge))
connector_type = bridge->type;
@@ -612,21 +650,21 @@ struct drm_connector *drm_bridge_connector_init(struct
drm_device *drm,
if (ret)
return ERR_PTR(ret);
- if (bridge->hdmi_audio_max_i2s_playback_channels ||
- bridge->hdmi_audio_spdif_playback) {
- if (!bridge->funcs->hdmi_audio_prepare ||
- !bridge->funcs->hdmi_audio_shutdown)
- return ERR_PTR(-EINVAL);
+ ret = drm_bridge_connector_hdmi_audio_init(connector, bridge);
+ if (ret)
+ return ERR_PTR(ret);
+ } else if (bridge_connector->bridge_dp) {
+ bridge = bridge_connector->bridge_dp;
- ret = drm_connector_hdmi_audio_init(connector,
-
bridge->hdmi_audio_dev,
-
[PATCH v2 3/3] drm/msm/dp: reuse generic HDMI codec implementation
The MSM DisplayPort driver implements several HDMI codec functions
in the driver, e.g. it manually manages HDMI codec device registration,
returning ELD and plugged_cb support. In order to reduce code
duplication reuse drm_hdmi_audio_* helpers and drm_bridge_connector
integration.
As a part of this change, also drop the call to
drm_connector_attach_dp_subconnector_property(), it is now being handled
by the drm_bridge_connector.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/Kconfig | 1 +
drivers/gpu/drm/msm/dp/dp_audio.c | 131
drivers/gpu/drm/msm/dp/dp_audio.h | 27 ++--
drivers/gpu/drm/msm/dp/dp_display.c | 31 ++---
drivers/gpu/drm/msm/dp/dp_display.h | 6 --
drivers/gpu/drm/msm/dp/dp_drm.c | 11 ++-
6 files changed, 34 insertions(+), 173 deletions(-)
diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
index
7ec833b6d8292f8cb26cfe5582812f2754cd4d35..fe36a3bcfe03994952d1b5e1b423e923e3e3b014
100644
--- a/drivers/gpu/drm/msm/Kconfig
+++ b/drivers/gpu/drm/msm/Kconfig
@@ -104,6 +104,7 @@ config DRM_MSM_DPU
config DRM_MSM_DP
bool "Enable DisplayPort support in MSM DRM driver"
depends on DRM_MSM
+ select DRM_DISPLAY_HDMI_AUDIO_HELPER
select RATIONAL
default y
help
diff --git a/drivers/gpu/drm/msm/dp/dp_audio.c
b/drivers/gpu/drm/msm/dp/dp_audio.c
index
70fdc9fe228a7149546accd8479a9e4397f3d5dd..f8bfb908f9b4bf93ad5480f0785e3aed23dde160
100644
--- a/drivers/gpu/drm/msm/dp/dp_audio.c
+++ b/drivers/gpu/drm/msm/dp/dp_audio.c
@@ -13,13 +13,13 @@
#include "dp_catalog.h"
#include "dp_audio.h"
+#include "dp_drm.h"
#include "dp_panel.h"
#include "dp_reg.h"
#include "dp_display.h"
#include "dp_utils.h"
struct msm_dp_audio_private {
- struct platform_device *audio_pdev;
struct platform_device *pdev;
struct drm_device *drm_dev;
struct msm_dp_catalog *catalog;
@@ -160,24 +160,11 @@ static void msm_dp_audio_enable(struct
msm_dp_audio_private *audio, bool enable)
msm_dp_catalog_audio_enable(catalog, enable);
}
-static struct msm_dp_audio_private *msm_dp_audio_get_data(struct
platform_device *pdev)
+static struct msm_dp_audio_private *msm_dp_audio_get_data(struct msm_dp
*msm_dp_display)
{
struct msm_dp_audio *msm_dp_audio;
- struct msm_dp *msm_dp_display;
-
- if (!pdev) {
- DRM_ERROR("invalid input\n");
- return ERR_PTR(-ENODEV);
- }
-
- msm_dp_display = platform_get_drvdata(pdev);
- if (!msm_dp_display) {
- DRM_ERROR("invalid input\n");
- return ERR_PTR(-ENODEV);
- }
msm_dp_audio = msm_dp_display->msm_dp_audio;
-
if (!msm_dp_audio) {
DRM_ERROR("invalid msm_dp_audio data\n");
return ERR_PTR(-EINVAL);
@@ -186,68 +173,16 @@ static struct msm_dp_audio_private
*msm_dp_audio_get_data(struct platform_device
return container_of(msm_dp_audio, struct msm_dp_audio_private,
msm_dp_audio);
}
-static int msm_dp_audio_hook_plugged_cb(struct device *dev, void *data,
- hdmi_codec_plugged_cb fn,
- struct device *codec_dev)
-{
-
- struct platform_device *pdev;
- struct msm_dp *msm_dp_display;
-
- pdev = to_platform_device(dev);
- if (!pdev) {
- pr_err("invalid input\n");
- return -ENODEV;
- }
-
- msm_dp_display = platform_get_drvdata(pdev);
- if (!msm_dp_display) {
- pr_err("invalid input\n");
- return -ENODEV;
- }
-
- return msm_dp_display_set_plugged_cb(msm_dp_display, fn, codec_dev);
-}
-
-static int msm_dp_audio_get_eld(struct device *dev,
- void *data, uint8_t *buf, size_t len)
-{
- struct platform_device *pdev;
- struct msm_dp *msm_dp_display;
-
- pdev = to_platform_device(dev);
-
- if (!pdev) {
- DRM_ERROR("invalid input\n");
- return -ENODEV;
- }
-
- msm_dp_display = platform_get_drvdata(pdev);
- if (!msm_dp_display) {
- DRM_ERROR("invalid input\n");
- return -ENODEV;
- }
-
- mutex_lock(&msm_dp_display->connector->eld_mutex);
- memcpy(buf, msm_dp_display->connector->eld,
- min(sizeof(msm_dp_display->connector->eld), len));
- mutex_unlock(&msm_dp_display->connector->eld_mutex);
-
- return 0;
-}
-
-int msm_dp_audio_hw_params(struct device *dev,
- void *data,
- struct hdmi_codec_daifmt *daifmt,
- struct hdmi_codec_params *params)
+int msm_dp_audio_prepare(struct drm_connector *connector,
+struct drm_bridge *bridge,
+struct hdmi_codec_daifmt *daifmt,
+struct hdmi_codec_params *params)
{
int rc = 0;
struct msm_dp_audio_private *audio;
- struct platform_device *pdev;
struct msm_d
[PATCH v2 0/3] drm/bridge: reuse DRM HDMI Audio helpers for DisplayPort bridges
A lot of DisplayPort bridges use HDMI Codec in order to provide audio support. Present DRM HDMI Audio support has been written with the HDMI and in particular DRM HDMI Connector framework support, however those audio helpers can be easily reused for DisplayPort drivers too. Patches by Hermes Wu that targeted implementing HDMI Audio support in the iTE IT6506 driver pointed out the necessity of allowing one to use generic audio helpers for DisplayPort drivers, as otherwise each driver has to manually (and correctly) implement the get_eld() and plugged_cb support. Implement necessary integration in drm_bridge_connector and provide an example implementation in the msm/dp driver. The plan is to land core parts via the drm-misc-next tree and msm patch via the msm-next tree. Signed-off-by: Dmitry Baryshkov --- Changes in v2: - Added drm_connector_attach_dp_subconnector_property() patches - Link to v1: https://lore.kernel.org/r/20250206-dp-hdmi-audio-v1-0-8aa14a8c0...@linaro.org --- Dmitry Baryshkov (3): drm/display: bridge-connector: add DisplayPort bridges drm/display: bridge_connector: add DisplayPort subconnector property drm/msm/dp: reuse generic HDMI codec implementation drivers/gpu/drm/display/drm_bridge_connector.c | 68 ++--- drivers/gpu/drm/msm/Kconfig| 1 + drivers/gpu/drm/msm/dp/dp_audio.c | 131 +++-- drivers/gpu/drm/msm/dp/dp_audio.h | 27 ++--- drivers/gpu/drm/msm/dp/dp_display.c| 31 ++ drivers/gpu/drm/msm/dp/dp_display.h| 6 -- drivers/gpu/drm/msm/dp/dp_drm.c| 11 ++- include/drm/drm_bridge.h | 14 ++- 8 files changed, 101 insertions(+), 188 deletions(-) --- base-commit: ed58d103e6da15a442ff87567898768dc3a66987 change-id: 20250206-dp-hdmi-audio-15d9fdbebb9f Best regards, -- Dmitry Baryshkov
[PATCH v2 2/3] drm/display: bridge_connector: add DisplayPort subconnector property
Create the DisplayPort subconnector property for DP connectors managed through drm_bridge_connector, removing the need to create it manually by the drivers. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/display/drm_bridge_connector.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c index 5e031395b801f9a1371dcb4ac09f3da23e4615dd..df9e6b46b40454385f7023310327c5c99d5c6a5a 100644 --- a/drivers/gpu/drm/display/drm_bridge_connector.c +++ b/drivers/gpu/drm/display/drm_bridge_connector.c @@ -662,6 +662,8 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, if (ret) return ERR_PTR(ret); + drm_connector_attach_dp_subconnector_property(connector); + ret = drm_bridge_connector_hdmi_audio_init(connector, bridge); if (ret) return ERR_PTR(ret); -- 2.39.5
Re: [PATCH v2] drm/msm/dp: account for widebus and yuv420 during mode validation
On 06/02/2025 19:46, Abhinav Kumar wrote:
Widebus allows the DP controller to operate in 2 pixel per clock mode.
The mode validation logic validates the mode->clock against the max
DP pixel clock. However the max DP pixel clock limit assumes widebus
is already enabled. Adjust the mode validation logic to only compare
the adjusted pixel clock which accounts for widebus against the max DP
pixel clock. Also fix the mode validation logic for YUV420 modes as in
that case as well, only half the pixel clock is needed.
Fixes: 757a2f36ab09 ("drm/msm/dp: enable widebus feature for display port")
Fixes: 6db6e5606576 ("drm/msm/dp: change clock related programming for YUV420 over
DP")
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Abhinav Kumar
Reviewed-by: Dmitry Baryshkov
---
Changes in v2:
- move msm_dp_wide_bus_available() to the next line
- Link to v1:
https://lore.kernel.org/r/20250128-dp-widebus-fix-v1-1-b66d22655...@quicinc.com
---
drivers/gpu/drm/msm/dp/dp_display.c | 11 ++-
drivers/gpu/drm/msm/dp/dp_drm.c | 5 -
2 files changed, 10 insertions(+), 6 deletions(-)
This patch fixes a CLOCK_HIGH mode rejection error on the Microsoft
Surface Pro 11 (OLED panel) which I had previously been working-around
by raising DP_MAX_PIXEL_CLK_KHZ.
Tested-by: Dale Whinham
--
Best regards,
Dale
Re: [PATCH v3] drm/bridge-connector: handle subconnector types
On Fri, Jan 17, 2025 at 11:50:50AM +0200, Dmitry Baryshkov wrote: > If the created connector type supports subconnector type property, > create and attach corresponding it. The default subtype value is 0, > which maps to the DRM_MODE_SUBCONNECTOR_Unknown type. Also remove > subconnector creation from the msm_dp driver to prevent having duplicate > properties on the DP connectors. > > Signed-off-by: Dmitry Baryshkov > --- > This is a leftover of my previous attempt to implement USB-C DisplayPort > uABI. The idea was dropped, but I consider this part still to be useful, > as it allows one to register corresponding subconnector properties and > also to export the subconnector type. > --- > Changes in v3: > - Rebased on top of linux-next > - Drop subconnector property from msm_dp driver > - Link to v2: > https://lore.kernel.org/r/20230903214934.2877259-1-dmitry.barysh...@linaro.org > > Changes in v2: > - Dropped all DP and USB-related patches > - Dropped the patch adding default subtype to > drm_connector_attach_dp_subconnector_property() > - Replaced creation of TV subconnector property with the check that it > was created beforehand (Neil, Laurent) > - Link to v1: > https://lore.kernel.org/r/20230729004913.215872-1-dmitry.barysh...@linaro.org/ > --- > drivers/gpu/drm/display/drm_bridge_connector.c | 28 > +- > drivers/gpu/drm/msm/dp/dp_drm.c| 3 --- > include/drm/drm_bridge.h | 4 > 3 files changed, 31 insertions(+), 4 deletions(-) It seems this isn't getting any response. Also we don't have (and don't expect) DVI-I and TV bridges. Let me merge DP part to [1] and drop other parts. https://lore.kernel.org/linux-arm-msm/20250206-dp-hdmi-audio-v1-0-8aa14a8c0...@linaro.org -- With best wishes Dmitry
Re: [PATCH v3 2/2] arm64: dts: qcom: sa8775p-ride: Enable Adreno 663 GPU
On Wed, Nov 13, 2024 at 02:18:43AM +0530, Akhil P Oommen wrote:
> On 10/30/2024 12:32 PM, Akhil P Oommen wrote:
> > From: Puranam V G Tejaswi
> >
> > Enable GPU for sa8775p-ride platform and provide path for zap
> > shader.
> >
> > Signed-off-by: Puranam V G Tejaswi
> > Signed-off-by: Akhil P Oommen
> > Reviewed-by: Dmitry Baryshkov
> > ---
> > arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi | 8
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi
> > b/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi
> > index 0c1b21def4b6..4901163df8f3 100644
> > --- a/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi
> > @@ -407,6 +407,14 @@ queue3 {
> > };
> > };
> >
> > +&gpu {
> > + status = "okay";
> > +};
> > +
> > +&gpu_zap_shader {
> > + firmware-name = "qcom/sa8775p/a663_zap.mbn";
> > +};
> > +
> > &i2c11 {
> > clock-frequency = <40>;
> > pinctrl-0 = <&qup_i2c11_default>;
> >
>
> Bjorn,
>
> Please ignore this patch for now. This is probably not the right
> platform dtsi file where gpu should be enabled. I am discussing about
> this internally. Will send a revision or a new patch based on the
> conclusion.
Akhil, any updates on this?
--
With best wishes
Dmitry
