[PATCH v2 0/3] drm/msm: Initial fixes for DUALPIPE (+DSC) topology

[PATCH v2 0/3] drm/msm: Initial fixes for DUALPIPE (+DSC) topology

[Marijn Suijten]

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 <marijn.suijten@somainline.org>
---
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-78ae3ee9a697@somainline.org

Depends on:
- https://lore.kernel.org/linux-arm-msm/20250122-dpu-111-topology-v2-1-505e95964af9@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-505e95964af9@somainline.org>
prerequisite-patch-id: 9ed44ae089b173f452a6603e6739b0b3bf2d9274

Best regards,
-- 
Marijn Suijten <marijn.suijten@somainline.org>

[PATCH v2 1/3] drm/msm/dsi: Use existing per-interface slice count in DSC timing

[Marijn Suijten]

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 <marijn.suijten@somainline.org>
---
 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 <linux/math.h>
 #include <drm/display/drm_dsc_helper.h>
 
-/**
- * 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

[Marijn Suijten]

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 <marijn.suijten@somainline.org>
---
 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

[Marijn Suijten]

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 <marijn.suijten@somainline.org>
---
 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

Re: [PATCH v2 2/3] drm/msm/dsi: Set PHY usescase (and mode) before registering DSI host

[Dmitry Baryshkov]

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 <marijn.suijten@somainline.org>
> ---
>  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 <dmitry.baryshkov@linaro.org>

> 
> 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

[Dmitry Baryshkov]

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 <marijn.suijten@somainline.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH v2 2/3] drm/msm/dsi: Set PHY usescase (and mode) before registering DSI host

[Abhinav Kumar]

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 <marijn.suijten@somainline.org>
> ---
>   drivers/gpu/drm/msm/dsi/dsi_manager.c | 30 +++++++++++++++++++-----------
>   1 file changed, 19 insertions(+), 11 deletions(-)
> 

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

Re: [PATCH v2 2/3] drm/msm/dsi: Set PHY usescase (and mode) before registering DSI host

[Marijn Suijten]

On 2025-02-10 01:11:59, Dmitry Baryshkov wrote:
> 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 <marijn.suijten@somainline.org>
> > ---
> >  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")

I'm not exactly confident about that.  Abhinav pointed out in
https://gitlab.freedesktop.org/drm/msm/-/issues/41#note_2375646 that
msm_dsi_host_register() was not supposed to be enabling the PHY, which I
provided a counter-stacktrace for to show that is indeed the case.

Either this was always a problem that's only become visible now (and it's an
issue with that patch), or a different change causes msm_dsi_host_register()
to enable the PHY and program the usecase too early?

What do you think?

- Marijn

> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> > 
> > 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 2/3] drm/msm/dsi: Set PHY usescase (and mode) before registering DSI host

[Dmitry Baryshkov]

On Mon, Feb 10, 2025 at 01:54:29PM +0100, Marijn Suijten wrote:
> On 2025-02-10 01:11:59, Dmitry Baryshkov wrote:
> > 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 <marijn.suijten@somainline.org>
> > > ---
> > >  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")
> 
> I'm not exactly confident about that.  Abhinav pointed out in
> https://gitlab.freedesktop.org/drm/msm/-/issues/41#note_2375646 that
> msm_dsi_host_register() was not supposed to be enabling the PHY, which I
> provided a counter-stacktrace for to show that is indeed the case.
> 
> Either this was always a problem that's only become visible now (and it's an
> issue with that patch), or a different change causes msm_dsi_host_register()
> to enable the PHY and program the usecase too early?

As currently usecase is being programmed after the DSI host being
registered, there might be a race condition between panel driver probe
_and_ usecase programming.

> 
> What do you think?
> 
> - Marijn
> 
> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > 
> > > 
> > > 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

-- 
With best wishes
Dmitry

Re: [PATCH v2 2/3] drm/msm/dsi: Set PHY usescase (and mode) before registering DSI host

[Abhinav Kumar]

On 2/10/2025 6:24 AM, Dmitry Baryshkov wrote:
> On Mon, Feb 10, 2025 at 01:54:29PM +0100, Marijn Suijten wrote:
>> On 2025-02-10 01:11:59, Dmitry Baryshkov wrote:
>>> 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 <marijn.suijten@somainline.org>
>>>> ---
>>>>   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")
>>
>> I'm not exactly confident about that.  Abhinav pointed out in
>> https://gitlab.freedesktop.org/drm/msm/-/issues/41#note_2375646 that
>> msm_dsi_host_register() was not supposed to be enabling the PHY, which I
>> provided a counter-stacktrace for to show that is indeed the case.
>>
>> Either this was always a problem that's only become visible now (and it's an
>> issue with that patch), or a different change causes msm_dsi_host_register()
>> to enable the PHY and program the usecase too early?
> 
> As currently usecase is being programmed after the DSI host being
> registered, there might be a race condition between panel driver probe
> _and_ usecase programming.
> 
>>
>> What do you think?
>>
>> - Marijn
>>

Yes I agree with Dmitry's explanation. The race condition between the 
two can cause this. Hence I am also fine with this change.

>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>
>>>>
>>>> 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 1/3] drm/msm/dsi: Use existing per-interface slice count in DSC timing

[Jessica Zhang]

On 2/9/2025 1:42 PM, Marijn Suijten wrote:
> 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 <marijn.suijten@somainline.org>

Hi Marijn,

Reviewed-by: Jessica Zhang <quic_jesszhan@quicinc.com>

Thanks,

Jessica Zhang

> ---
>   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 <linux/math.h>
>   #include <drm/display/drm_dsc_helper.h>
>   
> -/**
> - * 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
> 
> 

Re: [PATCH v2 3/3] drm/msm/dpu: Remove arbitrary limit of 1 interface in DSC topology

[Jessica Zhang]

On 2/9/2025 1:42 PM, 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 <marijn.suijten@somainline.org>

Reviewed-by: Jessica Zhang <quic_jesszhan@quicinc.com>

> ---
>   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
> 

Re: [PATCH v2 1/3] drm/msm/dsi: Use existing per-interface slice count in DSC timing

[Dmitry Baryshkov]

On Sun, Feb 09, 2025 at 10:42:52PM +0100, Marijn Suijten wrote:
> 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 <marijn.suijten@somainline.org>
> ---
>  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(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH v2 2/3] drm/msm/dsi: Set PHY usescase (and mode) before registering DSI host

[Abhinav Kumar]

Hi Marijn

On 2/10/2025 2:17 PM, Abhinav Kumar wrote:
> 
> 
> On 2/10/2025 6:24 AM, Dmitry Baryshkov wrote:
>> On Mon, Feb 10, 2025 at 01:54:29PM +0100, Marijn Suijten wrote:
>>> On 2025-02-10 01:11:59, Dmitry Baryshkov wrote:
>>>> 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 <marijn.suijten@somainline.org>
>>>>> ---
>>>>>   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")
>>>
>>> I'm not exactly confident about that.  Abhinav pointed out in
>>> https://gitlab.freedesktop.org/drm/msm/-/issues/41#note_2375646 that
>>> msm_dsi_host_register() was not supposed to be enabling the PHY, which I
>>> provided a counter-stacktrace for to show that is indeed the case.
>>>
>>> Either this was always a problem that's only become visible now (and 
>>> it's an
>>> issue with that patch), or a different change causes 
>>> msm_dsi_host_register()
>>> to enable the PHY and program the usecase too early?
>>
>> As currently usecase is being programmed after the DSI host being
>> registered, there might be a race condition between panel driver probe
>> _and_ usecase programming.
>>
>>>
>>> What do you think?
>>>
>>> - Marijn
>>>
> 
> Yes I agree with Dmitry's explanation. The race condition between the 
> two can cause this. Hence I am also fine with this change.
> 
>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>
>>>>>
>>>>> 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.
>>>>> +         */

My compiler is throwing a warning here

drivers/gpu/drm/msm/dsi/dsi_manager.c: In function 
'dsi_mgr_setup_components':
drivers/gpu/drm/msm/dsi/dsi_manager.c:94:3: warning: "/*" within comment 
[-Wcomment]
    /* Set the usecase before calling msm_dsi_host_register(), which would
    ^
drivers/gpu/drm/msm/dsi/dsi_manager.c: At top level:

Can you pls address this one so that I can pick this one up?

Thanks

Abhinav

Re: [PATCH v2 2/3] drm/msm/dsi: Set PHY usescase (and mode) before registering DSI host

[Dmitry Baryshkov]

On Wed, Feb 12, 2025 at 05:13:08PM -0800, Abhinav Kumar wrote:
> Hi Marijn
> 
> On 2/10/2025 2:17 PM, Abhinav Kumar wrote:
> > 
> > 
> > On 2/10/2025 6:24 AM, Dmitry Baryshkov wrote:
> > > On Mon, Feb 10, 2025 at 01:54:29PM +0100, Marijn Suijten wrote:
> > > > On 2025-02-10 01:11:59, Dmitry Baryshkov wrote:
> > > > > 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 <marijn.suijten@somainline.org>
> > > > > > ---
> > > > > >   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")
> > > > 
> > > > I'm not exactly confident about that.  Abhinav pointed out in
> > > > https://gitlab.freedesktop.org/drm/msm/-/issues/41#note_2375646 that
> > > > msm_dsi_host_register() was not supposed to be enabling the PHY, which I
> > > > provided a counter-stacktrace for to show that is indeed the case.
> > > > 
> > > > Either this was always a problem that's only become visible now
> > > > (and it's an
> > > > issue with that patch), or a different change causes
> > > > msm_dsi_host_register()
> > > > to enable the PHY and program the usecase too early?
> > > 
> > > As currently usecase is being programmed after the DSI host being
> > > registered, there might be a race condition between panel driver probe
> > > _and_ usecase programming.
> > > 
> > > > 
> > > > What do you think?
> > > > 
> > > > - Marijn
> > > > 
> > 
> > Yes I agree with Dmitry's explanation. The race condition between the
> > two can cause this. Hence I am also fine with this change.
> > 
> > > > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > 
> > > > > > 
> > > > > > 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.
> > > > > > +         */
> 
> My compiler is throwing a warning here
> 
> drivers/gpu/drm/msm/dsi/dsi_manager.c: In function
> 'dsi_mgr_setup_components':
> drivers/gpu/drm/msm/dsi/dsi_manager.c:94:3: warning: "/*" within comment
> [-Wcomment]
>    /* Set the usecase before calling msm_dsi_host_register(), which would
>    ^
> drivers/gpu/drm/msm/dsi/dsi_manager.c: At top level:
> 
> Can you pls address this one so that I can pick this one up?

While you are at it, Marijn, could you please also fix the first comment
to be properly formatted?

-- 
With best wishes
Dmitry

Re: [PATCH v2 2/3] drm/msm/dsi: Set PHY usescase (and mode) before registering DSI host

[Marijn Suijten]

On 2025-02-13 04:13:06, Dmitry Baryshkov wrote:
> On Wed, Feb 12, 2025 at 05:13:08PM -0800, Abhinav Kumar wrote:
> > Hi Marijn
> > 
> > On 2/10/2025 2:17 PM, Abhinav Kumar wrote:
> > > 
> > > 
> > > On 2/10/2025 6:24 AM, Dmitry Baryshkov wrote:
> > > > On Mon, Feb 10, 2025 at 01:54:29PM +0100, Marijn Suijten wrote:
> > > > > On 2025-02-10 01:11:59, Dmitry Baryshkov wrote:
> > > > > > 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 <marijn.suijten@somainline.org>
> > > > > > > ---
> > > > > > >   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")
> > > > > 
> > > > > I'm not exactly confident about that.  Abhinav pointed out in
> > > > > https://gitlab.freedesktop.org/drm/msm/-/issues/41#note_2375646 that
> > > > > msm_dsi_host_register() was not supposed to be enabling the PHY, which I
> > > > > provided a counter-stacktrace for to show that is indeed the case.
> > > > > 
> > > > > Either this was always a problem that's only become visible now
> > > > > (and it's an
> > > > > issue with that patch), or a different change causes
> > > > > msm_dsi_host_register()
> > > > > to enable the PHY and program the usecase too early?
> > > > 
> > > > As currently usecase is being programmed after the DSI host being
> > > > registered, there might be a race condition between panel driver probe
> > > > _and_ usecase programming.
> > > > 
> > > > > 
> > > > > What do you think?
> > > > > 
> > > > > - Marijn
> > > > > 
> > > 
> > > Yes I agree with Dmitry's explanation. The race condition between the
> > > two can cause this. Hence I am also fine with this change.
> > > 
> > > > > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > > 
> > > > > > > 
> > > > > > > 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.
> > > > > > > +         */
> > 
> > My compiler is throwing a warning here
> > 
> > drivers/gpu/drm/msm/dsi/dsi_manager.c: In function
> > 'dsi_mgr_setup_components':
> > drivers/gpu/drm/msm/dsi/dsi_manager.c:94:3: warning: "/*" within comment
> > [-Wcomment]
> >    /* Set the usecase before calling msm_dsi_host_register(), which would
> >    ^
> > drivers/gpu/drm/msm/dsi/dsi_manager.c: At top level:
> > 
> > Can you pls address this one so that I can pick this one up?

Whoops, that looks like copy-paste mistake when copying the comment from
!IS_BONDED_DSI() and merging it with the original comment about PLL0.
Will resend when I understand Dmitry's question below;

> 
> While you are at it, Marijn, could you please also fix the first comment
> to be properly formatted?

What exactly do you need to be reformatted about the first comment?  It's
exactly the same in both branches.  No multiline comments in dsi_manager.c
start with a newline after the first /*.  Anything I'm missing?

- Marijn

> -- 
> With best wishes
> Dmitry

Re: [PATCH v2 2/3] drm/msm/dsi: Set PHY usescase (and mode) before registering DSI host

[Dmitry Baryshkov]

On Mon, 17 Feb 2025 at 01:06, Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> On 2025-02-13 04:13:06, Dmitry Baryshkov wrote:
> > On Wed, Feb 12, 2025 at 05:13:08PM -0800, Abhinav Kumar wrote:
> > > Hi Marijn
> > >
> > > On 2/10/2025 2:17 PM, Abhinav Kumar wrote:
> > > >
> > > >
> > > > On 2/10/2025 6:24 AM, Dmitry Baryshkov wrote:
> > > > > On Mon, Feb 10, 2025 at 01:54:29PM +0100, Marijn Suijten wrote:
> > > > > > On 2025-02-10 01:11:59, Dmitry Baryshkov wrote:
> > > > > > > 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 <marijn.suijten@somainline.org>
> > > > > > > > ---
> > > > > > > >   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")
> > > > > >
> > > > > > I'm not exactly confident about that.  Abhinav pointed out in
> > > > > > https://gitlab.freedesktop.org/drm/msm/-/issues/41#note_2375646 that
> > > > > > msm_dsi_host_register() was not supposed to be enabling the PHY, which I
> > > > > > provided a counter-stacktrace for to show that is indeed the case.
> > > > > >
> > > > > > Either this was always a problem that's only become visible now
> > > > > > (and it's an
> > > > > > issue with that patch), or a different change causes
> > > > > > msm_dsi_host_register()
> > > > > > to enable the PHY and program the usecase too early?
> > > > >
> > > > > As currently usecase is being programmed after the DSI host being
> > > > > registered, there might be a race condition between panel driver probe
> > > > > _and_ usecase programming.
> > > > >
> > > > > >
> > > > > > What do you think?
> > > > > >
> > > > > > - Marijn
> > > > > >
> > > >
> > > > Yes I agree with Dmitry's explanation. The race condition between the
> > > > two can cause this. Hence I am also fine with this change.
> > > >
> > > > > > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > > >
> > > > > > > >
> > > > > > > > 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.
> > > > > > > > +         */
> > >
> > > My compiler is throwing a warning here
> > >
> > > drivers/gpu/drm/msm/dsi/dsi_manager.c: In function
> > > 'dsi_mgr_setup_components':
> > > drivers/gpu/drm/msm/dsi/dsi_manager.c:94:3: warning: "/*" within comment
> > > [-Wcomment]
> > >    /* Set the usecase before calling msm_dsi_host_register(), which would
> > >    ^
> > > drivers/gpu/drm/msm/dsi/dsi_manager.c: At top level:
> > >
> > > Can you pls address this one so that I can pick this one up?
>
> Whoops, that looks like copy-paste mistake when copying the comment from
> !IS_BONDED_DSI() and merging it with the original comment about PLL0.
> Will resend when I understand Dmitry's question below;
>
> >
> > While you are at it, Marijn, could you please also fix the first comment
> > to be properly formatted?
>
> What exactly do you need to be reformatted about the first comment?  It's
> exactly the same in both branches.  No multiline comments in dsi_manager.c
> start with a newline after the first /*.  Anything I'm missing?

Yes. But those, I think, are older comments. Please use proper
formatting for new comments.

>
> - Marijn
>
> > --
> > With best wishes
> > Dmitry



-- 
With best wishes
Dmitry