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

[PATCH 0/7] 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.

Note that some patches are applicable to DSC-less DUALPIPE bonded mode
as well, such as the patch that allows the slave interface to always be
flushed as that is only supposed to be excluded in the yet-unsupported
PPSPLIT topology.

This series also contains some patches that I'm not too sure about:

  drm/msm/dpu: Correct dual-ctl -> dual-intf typo in comment

    Downstream doesn't skip the slave INTF flush on active-CTL [1]
    (again, just like cmdmode, only when PPSPLIT is enabled [2]), and
    even added an extra comment [1] explaining this case.  Hence a
    dual-intf but single-flush case doesn't seem to exist as there's
    only one CTL according to the remainder of the comment.
    Maybe the whole comment is wrong?

  drm/msm/dsi: Set PHY usescase before registering DSI host

    It seems intentional to only set the usecase after
    msm_dsi_host_register() in case it fails, so maybe a non-zero `ret`
    here should reset the usecase?  Likewise should the function call be
    moved in !IS_BONDED_DSI() above?

    Ideally we also understand what I am doing differently (maybe
    wrongly) in my panel driver that makes the PLL turn on and configure
    before the usecase has been set, even though these calls are messy
    and error-prone nevertheless.

[1]: https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/display-kernel.lnx.5.4.r1-rel/msm/sde/sde_encoder_phys_vid.c?ref_type=heads#L794-804
[2]: https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/display-kernel.lnx.5.4.r1-rel/msm/sde/sde_encoder_phys_cmd.c?ref_type=heads#L1131-1139

Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
---
Marijn Suijten (7):
      drm/msm/dsi: Print dual-DSI-adjusted pclk instead of original mode pclk
      drm/msm/dsi: Pass bonded-DSI hdisplay/2 to DSC timing configuration
      drm/msm/dpu: Always flush the slave INTF on the CTL
      drm/msm/dpu: Allow configuring multiple active DSC blocks
      drm/msm/dpu: Correct dual-ctl -> dual-intf typo in comment
      drm/msm/dsi: Set PHY usescase before registering DSI host
      drm/msm/dpu: Rename `ctx` parameter to `intf` to match other functions

 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c |  3 ---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c |  2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c           |  9 ++++++---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c          | 14 +++++++-------
 drivers/gpu/drm/msm/dsi/dsi_host.c                   | 14 +++++++-------
 drivers/gpu/drm/msm/dsi/dsi_manager.c                | 15 +++++++++++----
 6 files changed, 32 insertions(+), 25 deletions(-)
---
base-commit: 6bd343537461b57f3efe5dfc5fc193a232dfef1e
change-id: 20240416-drm-msm-initial-dualpipe-dsc-fixes-3f0715b03bf4

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

[PATCH 1/7] drm/msm/dsi: Print dual-DSI-adjusted pclk instead of original mode pclk

[Marijn Suijten]

When dual-DSI (bonded DSI) was added in commit ed9976a09b48
("drm/msm/dsi: adjust dsi timing for dual dsi mode") some DBG() prints
were not updated, leading to print the original mode->clock rather
than the adjusted (typically the mode clock divided by two, though more
recently also adjusted for DSC compression) msm_host->pixel_clk_rate
which is passed to clk_set_rate() just below.  Fix that by printing the
actual pixel_clk_rate that is being set.

Fixes: ed9976a09b48 ("drm/msm/dsi: adjust dsi timing for dual dsi mode")
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 9d86a6aca6f2..c80be74cf10b 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -356,8 +356,8 @@ int dsi_link_clk_set_rate_6g(struct msm_dsi_host *msm_host)
 {
 	int ret;
 
-	DBG("Set clk rates: pclk=%d, byteclk=%lu",
-		msm_host->mode->clock, msm_host->byte_clk_rate);
+	DBG("Set clk rates: pclk=%lu, byteclk=%lu",
+	    msm_host->pixel_clk_rate, msm_host->byte_clk_rate);
 
 	ret = dev_pm_opp_set_rate(&msm_host->pdev->dev,
 				  msm_host->byte_clk_rate);
@@ -430,9 +430,9 @@ int dsi_link_clk_set_rate_v2(struct msm_dsi_host *msm_host)
 {
 	int ret;
 
-	DBG("Set clk rates: pclk=%d, byteclk=%lu, esc_clk=%lu, dsi_src_clk=%lu",
-		msm_host->mode->clock, msm_host->byte_clk_rate,
-		msm_host->esc_clk_rate, msm_host->src_clk_rate);
+	DBG("Set clk rates: pclk=%lu, byteclk=%lu, esc_clk=%lu, dsi_src_clk=%lu",
+	    msm_host->pixel_clk_rate, msm_host->byte_clk_rate,
+	    msm_host->esc_clk_rate, msm_host->src_clk_rate);
 
 	ret = clk_set_rate(msm_host->byte_clk, msm_host->byte_clk_rate);
 	if (ret) {

-- 
2.44.0

[PATCH 2/7] drm/msm/dsi: Pass bonded-DSI hdisplay/2 to DSC timing configuration

[Marijn Suijten]

When configuring the timing of DSI hosts (interfaces) in
dsi_timing_setup() all values written to registers are taking bonded
DSI 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).  Thus pass the
bonded-mode-adjusted hdisplay parameter into dsi_update_dsc_timing()
otherwise all values written to registers by this function (i.e. the
number of slices per interface or packet, and derived from this the EOL
byte number) are twice too large.

Inversely the panel driver is expected to only set the slice width and
number of slices for half the panel, i.e. what will be sent by each
host individually, rather than fixing that up like hdisplay here.

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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index c80be74cf10b..9d0c940dcb28 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -987,7 +987,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, hdisplay);
 
 		dsi_write(msm_host, REG_DSI_ACTIVE_H,
 			DSI_ACTIVE_H_START(ha_start) |
@@ -1008,7 +1008,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, hdisplay);
 
 		/* image data and 1 byte write_memory_start cmd */
 		if (!msm_host->dsc)

-- 
2.44.0

[PATCH 3/7] drm/msm/dpu: Always flush the slave INTF on the CTL

[Marijn Suijten]

As we can clearly see in a downstream kernel [1], flushing the slave INTF
is skipped /only if/ the PPSPLIT topology is active.

However, when DPU was originally submitted to mainline PPSPLIT was no
longer part of it (seems to have been ripped out before submission), but
this clause was incorrectly ported from the original SDE driver.  Given
that there is no support for PPSPLIT (currently), flushing the slave
INTF should /never/ be skipped (as the `if (ppsplit && !master) goto
skip;` clause downstream never becomes true).

[1]: https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/display-kernel.lnx.5.4.r1-rel/msm/sde/sde_encoder_phys_cmd.c?ref_type=heads#L1131-1139

Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index fc1d5736d7fc..489be1c0c704 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -448,9 +448,6 @@ static void dpu_encoder_phys_cmd_enable_helper(
 
 	_dpu_encoder_phys_cmd_pingpong_config(phys_enc);
 
-	if (!dpu_encoder_phys_cmd_is_master(phys_enc))
-		return;
-
 	ctl = phys_enc->hw_ctl;
 	ctl->ops.update_pending_flush_intf(ctl, phys_enc->hw_intf->idx);
 }

-- 
2.44.0

[PATCH 4/7] drm/msm/dpu: Allow configuring multiple active DSC blocks

[Marijn Suijten]

Just like the active interface and writeback block in ctl_intf_cfg_v1(),
and later the rest of the blocks in followup active-CTL fixes or
reworks, multiple calls to this function should enable additional DSC
blocks instead of overwriting the blocks that are enabled.

This pattern is observed in an active-CTL scenario since DPU 5.0.0 where
for example bonded-DSI uses a single CTL to drive multiple INTFs, and
each encoder calls this function individually with the INTF (hence the
pre-existing update instead of overwrite of this bitmask) and DSC blocks
it wishes to be enabled, and expects them to be OR'd into the bitmask.

The reverse already exists in reset_intf_cfg_v1() where only specified
DSC blocks are removed out of the CTL_DSC_ACTIVE bitmask (same for all
other blocks and ACTIVE bitmasks), leaving the rest enabled.

Fixes: 77f6da90487c ("drm/msm/disp/dpu1: Add DSC support in hw_ctl")
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
index a06f69d0b257..2e50049f2f85 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
@@ -545,6 +545,7 @@ static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx,
 {
 	struct dpu_hw_blk_reg_map *c = &ctx->hw;
 	u32 intf_active = 0;
+	u32 dsc_active = 0;
 	u32 wb_active = 0;
 	u32 mode_sel = 0;
 
@@ -560,6 +561,7 @@ static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx,
 
 	intf_active = DPU_REG_READ(c, CTL_INTF_ACTIVE);
 	wb_active = DPU_REG_READ(c, CTL_WB_ACTIVE);
+	dsc_active = DPU_REG_READ(c, CTL_DSC_ACTIVE);
 
 	if (cfg->intf)
 		intf_active |= BIT(cfg->intf - INTF_0);
@@ -567,17 +569,18 @@ static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx,
 	if (cfg->wb)
 		wb_active |= BIT(cfg->wb - WB_0);
 
+	if (cfg->dsc)
+		dsc_active |= cfg->dsc;
+
 	DPU_REG_WRITE(c, CTL_TOP, mode_sel);
 	DPU_REG_WRITE(c, CTL_INTF_ACTIVE, intf_active);
 	DPU_REG_WRITE(c, CTL_WB_ACTIVE, wb_active);
+	DPU_REG_WRITE(c, CTL_DSC_ACTIVE, dsc_active);
 
 	if (cfg->merge_3d)
 		DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE,
 			      BIT(cfg->merge_3d - MERGE_3D_0));
 
-	if (cfg->dsc)
-		DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc);
-
 	if (cfg->cdm)
 		DPU_REG_WRITE(c, CTL_CDM_ACTIVE, cfg->cdm);
 }

-- 
2.44.0

[PATCH 5/7] drm/msm/dpu: Correct dual-ctl -> dual-intf typo in comment

[Marijn Suijten]

This comment one line down references a single, "same CTL" that controls
two interfaces, so the comment should clearly describe two interfaces
used with a single active CTL and not "two CTLs".

Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index d9e7dbf0499c..7e849fe74801 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -428,7 +428,7 @@ static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys *phys_enc)
 	dpu_encoder_phys_vid_setup_timing_engine(phys_enc);
 
 	/*
-	 * For single flush cases (dual-ctl or pp-split), skip setting the
+	 * For single flush cases (dual-intf or pp-split), skip setting the
 	 * flush bit for the slave intf, since both intfs use same ctl
 	 * and HW will only flush the master.
 	 */

-- 
2.44.0

[PATCH 6/7] drm/msm/dsi: Set PHY usescase before registering DSI host

[Marijn Suijten]

Ordering issues here cause an uninitalized (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 | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index af2a287cb3bd..17f43b3c0494 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -85,6 +85,17 @@ static int dsi_mgr_setup_components(int 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 2 DSI link clocks in bonded DSI mode.
+		 *
+		 * Set the usecase before calling msm_dsi_host_register() to prevent it from
+		 * enabling and configuring the usecase (which is just a mux bit) first.
+		 */
+		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);
+
 		/* Register slave host first, so that slave DSI device
 		 * has a chance to probe, and do not block the master
 		 * DSI device's probe.
@@ -100,10 +111,6 @@ static int dsi_mgr_setup_components(int id)
 			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);
 	}

-- 
2.44.0

[PATCH 7/7] drm/msm/dpu: Rename `ctx` parameter to `intf` to match other functions

[Marijn Suijten]

All other functions in dpu_hw_intf name the "self" parameter `intf`,
except dpu_hw_intf_setup_timing_engine() and the recently added
dpu_hw_intf_program_intf_cmd_cfg().  Clean that up for consistency.

Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
index 965692ef7892..34d0c4e04d27 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -96,11 +96,11 @@
 #define INTF_CFG2_DCE_DATA_COMPRESS     BIT(12)
 
 
-static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
+static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *intf,
 		const struct dpu_hw_intf_timing_params *p,
 		const struct dpu_format *fmt)
 {
-	struct dpu_hw_blk_reg_map *c = &ctx->hw;
+	struct dpu_hw_blk_reg_map *c = &intf->hw;
 	u32 hsync_period, vsync_period;
 	u32 display_v_start, display_v_end;
 	u32 hsync_start_x, hsync_end_x;
@@ -118,7 +118,7 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
 	/* read interface_cfg */
 	intf_cfg = DPU_REG_READ(c, INTF_CONFIG);
 
-	if (ctx->cap->type == INTF_DP)
+	if (intf->cap->type == INTF_DP)
 		dp_intf = true;
 
 	hsync_period = p->hsync_pulse_width + p->h_back_porch + p->width +
@@ -223,7 +223,7 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
 	DPU_REG_WRITE(c, INTF_FRAME_LINE_COUNT_EN, 0x3);
 	DPU_REG_WRITE(c, INTF_CONFIG, intf_cfg);
 	DPU_REG_WRITE(c, INTF_PANEL_FORMAT, panel_format);
-	if (ctx->cap->features & BIT(DPU_DATA_HCTL_EN)) {
+	if (intf->cap->features & BIT(DPU_DATA_HCTL_EN)) {
 		/*
 		 * DATA_HCTL_EN controls data timing which can be different from
 		 * video timing. It is recommended to enable it for all cases, except
@@ -518,10 +518,10 @@ static void dpu_hw_intf_disable_autorefresh(struct dpu_hw_intf *intf,
 
 }
 
-static void dpu_hw_intf_program_intf_cmd_cfg(struct dpu_hw_intf *ctx,
+static void dpu_hw_intf_program_intf_cmd_cfg(struct dpu_hw_intf *intf,
 					     struct dpu_hw_intf_cmd_mode_cfg *cmd_mode_cfg)
 {
-	u32 intf_cfg2 = DPU_REG_READ(&ctx->hw, INTF_CONFIG2);
+	u32 intf_cfg2 = DPU_REG_READ(&intf->hw, INTF_CONFIG2);
 
 	if (cmd_mode_cfg->data_compress)
 		intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
@@ -529,7 +529,7 @@ static void dpu_hw_intf_program_intf_cmd_cfg(struct dpu_hw_intf *ctx,
 	if (cmd_mode_cfg->wide_bus_en)
 		intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
 
-	DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
+	DPU_REG_WRITE(&intf->hw, INTF_CONFIG2, intf_cfg2);
 }
 
 struct dpu_hw_intf *dpu_hw_intf_init(struct drm_device *dev,

-- 
2.44.0

Re: [PATCH 6/7] drm/msm/dsi: Set PHY usescase before registering DSI host

[Dmitry Baryshkov]

On Wed, 17 Apr 2024 at 02:57, Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> Ordering issues here cause an uninitalized (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 | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> index af2a287cb3bd..17f43b3c0494 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> @@ -85,6 +85,17 @@ static int dsi_mgr_setup_components(int 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 2 DSI link clocks in bonded DSI mode.
> +                *
> +                * Set the usecase before calling msm_dsi_host_register() to prevent it from
> +                * enabling and configuring the usecase (which is just a mux bit) first.
> +                */
> +               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);
> +
>                 /* Register slave host first, so that slave DSI device
>                  * has a chance to probe, and do not block the master
>                  * DSI device's probe.
> @@ -100,10 +111,6 @@ static int dsi_mgr_setup_components(int id)
>                         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);

Please move msm_dsi_host_set_phy_mode() calls too. Also please update
the non-bonded case.

>         }
>
> --
> 2.44.0
>


-- 
With best wishes
Dmitry

Re: [PATCH 1/7] drm/msm/dsi: Print dual-DSI-adjusted pclk instead of original mode pclk

[Dmitry Baryshkov]

On Wed, 17 Apr 2024 at 02:57, Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> When dual-DSI (bonded DSI) was added in commit ed9976a09b48
> ("drm/msm/dsi: adjust dsi timing for dual dsi mode") some DBG() prints
> were not updated, leading to print the original mode->clock rather
> than the adjusted (typically the mode clock divided by two, though more
> recently also adjusted for DSC compression) msm_host->pixel_clk_rate
> which is passed to clk_set_rate() just below.  Fix that by printing the
> actual pixel_clk_rate that is being set.
>
> Fixes: ed9976a09b48 ("drm/msm/dsi: adjust dsi timing for dual dsi mode")
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> ---
>  drivers/gpu/drm/msm/dsi/dsi_host.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

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

-- 
With best wishes
Dmitry

Re: [PATCH 2/7] drm/msm/dsi: Pass bonded-DSI hdisplay/2 to DSC timing configuration

[Dmitry Baryshkov]

On Wed, 17 Apr 2024 at 02:57, Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> When configuring the timing of DSI hosts (interfaces) in
> dsi_timing_setup() all values written to registers are taking bonded
> DSI 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).  Thus pass the
> bonded-mode-adjusted hdisplay parameter into dsi_update_dsc_timing()
> otherwise all values written to registers by this function (i.e. the
> number of slices per interface or packet, and derived from this the EOL
> byte number) are twice too large.
>
> Inversely the panel driver is expected to only set the slice width and
> number of slices for half the panel, i.e. what will be sent by each
> host individually, rather than fixing that up like hdisplay here.
>
> 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 | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

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


-- 
With best wishes
Dmitry

Re: [PATCH 3/7] drm/msm/dpu: Always flush the slave INTF on the CTL

[Dmitry Baryshkov]

On Wed, 17 Apr 2024 at 02:57, Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> As we can clearly see in a downstream kernel [1], flushing the slave INTF
> is skipped /only if/ the PPSPLIT topology is active.
>
> However, when DPU was originally submitted to mainline PPSPLIT was no
> longer part of it (seems to have been ripped out before submission), but
> this clause was incorrectly ported from the original SDE driver.  Given
> that there is no support for PPSPLIT (currently), flushing the slave
> INTF should /never/ be skipped (as the `if (ppsplit && !master) goto
> skip;` clause downstream never becomes true).
>
> [1]: https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/display-kernel.lnx.5.4.r1-rel/msm/sde/sde_encoder_phys_cmd.c?ref_type=heads#L1131-1139
>
> Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 3 ---
>  1 file changed, 3 deletions(-)


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

-- 
With best wishes
Dmitry

Re: [PATCH 4/7] drm/msm/dpu: Allow configuring multiple active DSC blocks

[Dmitry Baryshkov]

On Wed, 17 Apr 2024 at 02:57, Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> Just like the active interface and writeback block in ctl_intf_cfg_v1(),
> and later the rest of the blocks in followup active-CTL fixes or
> reworks, multiple calls to this function should enable additional DSC
> blocks instead of overwriting the blocks that are enabled.
>
> This pattern is observed in an active-CTL scenario since DPU 5.0.0 where
> for example bonded-DSI uses a single CTL to drive multiple INTFs, and
> each encoder calls this function individually with the INTF (hence the
> pre-existing update instead of overwrite of this bitmask) and DSC blocks
> it wishes to be enabled, and expects them to be OR'd into the bitmask.
>
> The reverse already exists in reset_intf_cfg_v1() where only specified
> DSC blocks are removed out of the CTL_DSC_ACTIVE bitmask (same for all
> other blocks and ACTIVE bitmasks), leaving the rest enabled.
>
> Fixes: 77f6da90487c ("drm/msm/disp/dpu1: Add DSC support in hw_ctl")
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)


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

-- 
With best wishes
Dmitry

Re: [PATCH 6/7] drm/msm/dsi: Set PHY usescase before registering DSI host

[Marijn Suijten]

On 2024-04-17 11:18:58, Dmitry Baryshkov wrote:
> On Wed, 17 Apr 2024 at 02:57, Marijn Suijten
> <marijn.suijten@somainline.org> wrote:
> >
> > Ordering issues here cause an uninitalized (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 | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > index af2a287cb3bd..17f43b3c0494 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > @@ -85,6 +85,17 @@ static int dsi_mgr_setup_components(int 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 2 DSI link clocks in bonded DSI mode.
> > +                *
> > +                * Set the usecase before calling msm_dsi_host_register() to prevent it from
> > +                * enabling and configuring the usecase (which is just a mux bit) first.
> > +                */
> > +               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);
> > +
> >                 /* Register slave host first, so that slave DSI device
> >                  * has a chance to probe, and do not block the master
> >                  * DSI device's probe.
> > @@ -100,10 +111,6 @@ static int dsi_mgr_setup_components(int id)
> >                         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);
> 
> Please move msm_dsi_host_set_phy_mode() calls too.

Ack.  Yeah, given that msm_dsi_host_register() causes a modeset and finally the
PLL turning on, these should be set up as well.

For anyone else following along, I have pasted the stacktrace that showcases
the execution flow in the drm/msm tracker:

https://gitlab.freedesktop.org/drm/msm/-/issues/41#note_2376115

Abhinav also pointed out that this PLL source was correctly set in earlier
devcoredump reports, so it might have been a recent development/regression?
This seems to be the only issue originating from it, but folks were adamant that
dsi_mgr_setup_components() (ultimately) would never turn the PLL on, which is
"debunked" by said stacktrace.  Maybe other assumptions are affected by this
change?

> Also please update the non-bonded case.

Definitely, as suggested in the cover letter.  A similar stacktrace to the above
is acquired on a non-bonded setup, which is also relying on the variable to be
initialized to 0 to select the "local PLL source", rather than being correctly
set via this msm_dsi_phy_set_usecase() configuration.

- Marijn

> >         }
> >
> > --
> > 2.44.0
> >
> 
> 
> -- 
> With best wishes
> Dmitry

Re: [PATCH 5/7] drm/msm/dpu: Correct dual-ctl -> dual-intf typo in comment

[Dmitry Baryshkov]

On Wed, Apr 17, 2024 at 01:57:45AM +0200, Marijn Suijten wrote:
> This comment one line down references a single, "same CTL" that controls
> two interfaces, so the comment should clearly describe two interfaces
> used with a single active CTL and not "two CTLs".
> 
> Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> index d9e7dbf0499c..7e849fe74801 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> @@ -428,7 +428,7 @@ static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys *phys_enc)
>  	dpu_encoder_phys_vid_setup_timing_engine(phys_enc);
>  
>  	/*
> -	 * For single flush cases (dual-ctl or pp-split), skip setting the
> +	 * For single flush cases (dual-intf or pp-split), skip setting the

It should be fixed, but in the other way: it's 'single-ctl'. See
sde_encoder_phys_needs_single_flush().

>  	 * flush bit for the slave intf, since both intfs use same ctl
>  	 * and HW will only flush the master.
>  	 */
> 
> -- 
> 2.44.0
> 

-- 
With best wishes
Dmitry

Re: [PATCH 7/7] drm/msm/dpu: Rename `ctx` parameter to `intf` to match other functions

[Dmitry Baryshkov]

On Wed, Apr 17, 2024 at 01:57:47AM +0200, Marijn Suijten wrote:
> All other functions in dpu_hw_intf name the "self" parameter `intf`,
> except dpu_hw_intf_setup_timing_engine() and the recently added
> dpu_hw_intf_program_intf_cmd_cfg().  Clean that up for consistency.

I really have mixed feelings towards such patches. On one hand it
improves readability, on the other hand, it's just a name, it has no
specific value.

Still:

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


> 
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> index 965692ef7892..34d0c4e04d27 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> @@ -96,11 +96,11 @@
>  #define INTF_CFG2_DCE_DATA_COMPRESS     BIT(12)
>  
>  
> -static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
> +static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *intf,
>  		const struct dpu_hw_intf_timing_params *p,
>  		const struct dpu_format *fmt)
>  {
> -	struct dpu_hw_blk_reg_map *c = &ctx->hw;
> +	struct dpu_hw_blk_reg_map *c = &intf->hw;
>  	u32 hsync_period, vsync_period;
>  	u32 display_v_start, display_v_end;
>  	u32 hsync_start_x, hsync_end_x;
> @@ -118,7 +118,7 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
>  	/* read interface_cfg */
>  	intf_cfg = DPU_REG_READ(c, INTF_CONFIG);
>  
> -	if (ctx->cap->type == INTF_DP)
> +	if (intf->cap->type == INTF_DP)
>  		dp_intf = true;
>  
>  	hsync_period = p->hsync_pulse_width + p->h_back_porch + p->width +
> @@ -223,7 +223,7 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
>  	DPU_REG_WRITE(c, INTF_FRAME_LINE_COUNT_EN, 0x3);
>  	DPU_REG_WRITE(c, INTF_CONFIG, intf_cfg);
>  	DPU_REG_WRITE(c, INTF_PANEL_FORMAT, panel_format);
> -	if (ctx->cap->features & BIT(DPU_DATA_HCTL_EN)) {
> +	if (intf->cap->features & BIT(DPU_DATA_HCTL_EN)) {
>  		/*
>  		 * DATA_HCTL_EN controls data timing which can be different from
>  		 * video timing. It is recommended to enable it for all cases, except
> @@ -518,10 +518,10 @@ static void dpu_hw_intf_disable_autorefresh(struct dpu_hw_intf *intf,
>  
>  }
>  
> -static void dpu_hw_intf_program_intf_cmd_cfg(struct dpu_hw_intf *ctx,
> +static void dpu_hw_intf_program_intf_cmd_cfg(struct dpu_hw_intf *intf,
>  					     struct dpu_hw_intf_cmd_mode_cfg *cmd_mode_cfg)
>  {
> -	u32 intf_cfg2 = DPU_REG_READ(&ctx->hw, INTF_CONFIG2);
> +	u32 intf_cfg2 = DPU_REG_READ(&intf->hw, INTF_CONFIG2);
>  
>  	if (cmd_mode_cfg->data_compress)
>  		intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
> @@ -529,7 +529,7 @@ static void dpu_hw_intf_program_intf_cmd_cfg(struct dpu_hw_intf *ctx,
>  	if (cmd_mode_cfg->wide_bus_en)
>  		intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
>  
> -	DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
> +	DPU_REG_WRITE(&intf->hw, INTF_CONFIG2, intf_cfg2);
>  }
>  
>  struct dpu_hw_intf *dpu_hw_intf_init(struct drm_device *dev,
> 
> -- 
> 2.44.0
> 

-- 
With best wishes
Dmitry

Re: [PATCH 2/7] drm/msm/dsi: Pass bonded-DSI hdisplay/2 to DSC timing configuration

[Marijn Suijten]

On 2024-04-17 14:58:25, Dmitry Baryshkov wrote:
> On Wed, 17 Apr 2024 at 02:57, Marijn Suijten
> <marijn.suijten@somainline.org> wrote:
> >
> > When configuring the timing of DSI hosts (interfaces) in
> > dsi_timing_setup() all values written to registers are taking bonded
> > DSI 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).  Thus pass the
> > bonded-mode-adjusted hdisplay parameter into dsi_update_dsc_timing()
> > otherwise all values written to registers by this function (i.e. the
> > number of slices per interface or packet, and derived from this the EOL
> > byte number) are twice too large.
> >
> > Inversely the panel driver is expected to only set the slice width and
> > number of slices for half the panel, i.e. what will be sent by each
> > host individually, rather than fixing that up like hdisplay here.
> >
> > 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 | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Thanks, it seems this patch has already been picked up for 6.10 [1] to test at
least, but I'd advise you to drop it until I resend it in v2, as it no longer
performs as written in the title.

When I wrote this patch in in June 2023, commit efcbd6f9cdeb ("drm/msm/
dsi: Enable widebus for DSI") from August 2023 wasn't there yet.  That patch
updates hdisplay (because it is unused after that point) with the number
of compressed bytes to be sent over each interface, which is effectively
hdisplay (based on slice_count * slice_width, so as explained in the commit
message that corresponds to half the panel width), divided by a compression
ratio of 3 or 6 depending on widebus, thus passing a way too low value into
dsi_update_dsc_timing().

As a result this patch regresses the DSC panel on my SM8150 Sony Xperia 1, and
likely also explains why it was quite hard to get the porches "just right" on
the Xperia 1 III with its dual-DSI dual-DSC 4k@120Hz panel (that these patches
are specifically for).

I'm still thinking of how to best fix that: probably introducing a new separate
local variable, though dsi_update_dsc_timing() only uses it to calculate
the number of slices per interface, which again as written in the commit
description, is currently required to already be for one interface (in other
words, the Xperia 1 with only a single intf sets slice_count=2, but the Xperia 1
III with 2 bonded DSI interfaces sets slice_count=1).  Which means that this is
always equivalent to slice_per_intf = dsc->slice_count.

Let me know which approach is preferred.

- Marijn

[1]: https://gitlab.freedesktop.org/drm/msm/-/merge_requests/110

Re: [PATCH 2/7] drm/msm/dsi: Pass bonded-DSI hdisplay/2 to DSC timing configuration

[Dmitry Baryshkov]

On Sat, Apr 20, 2024 at 12:18:39AM +0200, Marijn Suijten wrote:
> On 2024-04-17 14:58:25, Dmitry Baryshkov wrote:
> > On Wed, 17 Apr 2024 at 02:57, Marijn Suijten
> > <marijn.suijten@somainline.org> wrote:
> > >
> > > When configuring the timing of DSI hosts (interfaces) in
> > > dsi_timing_setup() all values written to registers are taking bonded
> > > DSI 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).  Thus pass the
> > > bonded-mode-adjusted hdisplay parameter into dsi_update_dsc_timing()
> > > otherwise all values written to registers by this function (i.e. the
> > > number of slices per interface or packet, and derived from this the EOL
> > > byte number) are twice too large.
> > >
> > > Inversely the panel driver is expected to only set the slice width and
> > > number of slices for half the panel, i.e. what will be sent by each
> > > host individually, rather than fixing that up like hdisplay here.
> > >
> > > 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 | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> Thanks, it seems this patch has already been picked up for 6.10 [1] to test at
> least, but I'd advise you to drop it until I resend it in v2, as it no longer
> performs as written in the title.

Ok, dropping.

> 
> When I wrote this patch in in June 2023, commit efcbd6f9cdeb ("drm/msm/
> dsi: Enable widebus for DSI") from August 2023 wasn't there yet.  That patch
> updates hdisplay (because it is unused after that point) with the number
> of compressed bytes to be sent over each interface, which is effectively
> hdisplay (based on slice_count * slice_width, so as explained in the commit
> message that corresponds to half the panel width), divided by a compression
> ratio of 3 or 6 depending on widebus, thus passing a way too low value into
> dsi_update_dsc_timing().
> 
> As a result this patch regresses the DSC panel on my SM8150 Sony Xperia 1, and
> likely also explains why it was quite hard to get the porches "just right" on
> the Xperia 1 III with its dual-DSI dual-DSC 4k@120Hz panel (that these patches
> are specifically for).
> 
> I'm still thinking of how to best fix that: probably introducing a new separate
> local variable, though dsi_update_dsc_timing() only uses it to calculate
> the number of slices per interface, which again as written in the commit
> description, is currently required to already be for one interface (in other
> words, the Xperia 1 with only a single intf sets slice_count=2, but the Xperia 1
> III with 2 bonded DSI interfaces sets slice_count=1).  Which means that this is
> always equivalent to slice_per_intf = dsc->slice_count.
> 
> Let me know which approach is preferred.
> 
> - Marijn
> 
> [1]: https://gitlab.freedesktop.org/drm/msm/-/merge_requests/110

-- 
With best wishes
Dmitry

Re: [PATCH 3/7] drm/msm/dpu: Always flush the slave INTF on the CTL

[Abhinav Kumar]

On 4/16/2024 4:57 PM, Marijn Suijten wrote:
> As we can clearly see in a downstream kernel [1], flushing the slave INTF
> is skipped /only if/ the PPSPLIT topology is active.
> 
> However, when DPU was originally submitted to mainline PPSPLIT was no
> longer part of it (seems to have been ripped out before submission), but
> this clause was incorrectly ported from the original SDE driver.  Given
> that there is no support for PPSPLIT (currently), flushing the slave
> INTF should /never/ be skipped (as the `if (ppsplit && !master) goto
> skip;` clause downstream never becomes true).
> 
> [1]: https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/display-kernel.lnx.5.4.r1-rel/msm/sde/sde_encoder_phys_cmd.c?ref_type=heads#L1131-1139
> 
> Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 3 ---
>   1 file changed, 3 deletions(-)
> 

Yes, I agree with this, even though I did think earlier that intf master 
flush was sufficient , I cross-checked the docs and this is the right way.


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

Re: [PATCH 5/7] drm/msm/dpu: Correct dual-ctl -> dual-intf typo in comment

[Marijn Suijten]

On 2024-04-18 02:30:59, Dmitry Baryshkov wrote:
> On Wed, Apr 17, 2024 at 01:57:45AM +0200, Marijn Suijten wrote:
> > This comment one line down references a single, "same CTL" that controls
> > two interfaces, so the comment should clearly describe two interfaces
> > used with a single active CTL and not "two CTLs".
> > 
> > Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
> > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> > index d9e7dbf0499c..7e849fe74801 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> > @@ -428,7 +428,7 @@ static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys *phys_enc)
> >  	dpu_encoder_phys_vid_setup_timing_engine(phys_enc);
> >  
> >  	/*
> > -	 * For single flush cases (dual-ctl or pp-split), skip setting the
> > +	 * For single flush cases (dual-intf or pp-split), skip setting the
> 
> It should be fixed, but in the other way: it's 'single-ctl'. See
> sde_encoder_phys_needs_single_flush().

As written in the cover letter I was unsure about this comment.

You are right that sde_encoder_phys_needs_single_flush() is supposed to return
true in pp-split or single-ctl.  However, the second part of the comment (right
below) is in conflict with another patch that I've sent as part of these series:
on a single-ctl setup with dual interfaces, the second INTF needs to be marked
for flushing.

While that observation and fix is for CMD-mode, the exact same comment is found
downstream for video mode:

https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/display-kernel.lnx.5.4.r1-rel/msm/sde/sde_encoder_phys_vid.c?ref_type=heads#L794-804

You were fixing exactly that in one of your preliminary Active-CTL patches by
making dpu_encoder_phys_vid_needs_single_flush() return for Active-CTL, so we
should probably update this comment in the same patch when you send it?

(that is: the flush bit needs to be set for the slave intf in Active-CTL. Before
Active-CTL, a slave encoder would actually have two CTLs and two INTFs where the
flush bit was probably skipped on both slaves?)

On a side-note, since the needs_single_flush callback is used elsehwere, I'm
unsure if that patch affects things in the wrong way since the downstream file
linked above applies the check for CTL_ACTIVE_CFG directly in-line without
affecting the callback.

- Marijn

> >  	 * flush bit for the slave intf, since both intfs use same ctl
> >  	 * and HW will only flush the master.
> >  	 */
> > 
> > -- 
> > 2.44.0
> > 
> 
> -- 
> With best wishes
> Dmitry