[PATCH v2] drm/msm/dsi: fix pclk calculation for bonded dsi

[PATCH v2] drm/msm/dsi: fix pclk calculation for bonded dsi

[Pengyu Luo]

Recently, we round up new_hdisplay once at most, for bonded dsi, we
may need twice, since they are independent links, we should round up
each half separately. This also aligns with the hdisplay we program
later in dsi_timing_setup()

Example:
	full_hdisplay = 1904, dsc_bpp = 8, bpc = 8
	new_full_hdisplay = DIV_ROUND_UP(1904 * 8, 8 * 3) = 635

if we use half display
	new_half_hdisplay = DIV_ROUND_UP(952 * 8, 8 * 3) = 318
	new_full_display = 636

Fixes: 7c9e4a554d4a ("drm/msm/dsi: Reduce pclk rate for compression")
Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
v2: add a parameter desciption to the function doc (kernel test robot)
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 32 +++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index e8e83ee61e..06f094fc32 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -569,6 +569,7 @@ void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host)
  * dsi_adjust_pclk_for_compression() - Adjust the pclk rate for compression case
  * @mode: The selected mode for the DSI output
  * @dsc: DRM DSC configuration for this DSI output
+ * @is_bonded_dsi: True if two DSI controllers are bonded
  *
  * Adjust the pclk rate by calculating a new hdisplay proportional to
  * the compression ratio such that:
@@ -584,13 +585,30 @@ void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host)
  *  FIXME: Reconsider this if/when CMD mode handling is rewritten to use
  *  transfer time and data overhead as a starting point of the calculations.
  */
-static unsigned long dsi_adjust_pclk_for_compression(const struct drm_display_mode *mode,
-		const struct drm_dsc_config *dsc)
+static unsigned long
+dsi_adjust_pclk_for_compression(const struct drm_display_mode *mode,
+				const struct drm_dsc_config *dsc,
+				bool is_bonded_dsi)
 {
-	int new_hdisplay = DIV_ROUND_UP(mode->hdisplay * drm_dsc_get_bpp_int(dsc),
-			dsc->bits_per_component * 3);
+	int hdisplay, new_hdisplay, new_htotal;
 
-	int new_htotal = mode->htotal - mode->hdisplay + new_hdisplay;
+	/*
+	 * For bonded DSI, split hdisplay across two links and round up each
+	 * half separately, passing the full hdisplay would only round up once.
+	 * This also aligns with the hdisplay we program later in
+	 * dsi_timing_setup()
+	 */
+	hdisplay = mode->hdisplay;
+	if (is_bonded_dsi)
+		hdisplay /= 2;
+
+	new_hdisplay = DIV_ROUND_UP(hdisplay * drm_dsc_get_bpp_int(dsc),
+				    dsc->bits_per_component * 3);
+
+	if (is_bonded_dsi)
+		new_hdisplay *= 2;
+
+	new_htotal = mode->htotal - mode->hdisplay + new_hdisplay;
 
 	return mult_frac(mode->clock * 1000u, new_htotal, mode->htotal);
 }
@@ -603,12 +621,12 @@ static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode,
 	pclk_rate = mode->clock * 1000u;
 
 	if (dsc)
-		pclk_rate = dsi_adjust_pclk_for_compression(mode, dsc);
+		pclk_rate = dsi_adjust_pclk_for_compression(mode, dsc, is_bonded_dsi);
 
 	/*
 	 * For bonded DSI mode, the current DRM mode has the complete width of the
 	 * panel. Since, the complete panel is driven by two DSI controllers,
-	 * the clock rates have to be split between the two dsi controllers.
+	 * the clock rates have to be split between the two DSI controllers.
 	 * Adjust the byte and pixel clock rates for each dsi host accordingly.
 	 */
 	if (is_bonded_dsi)
-- 
2.53.0

Re: [PATCH v2] drm/msm/dsi: fix pclk calculation for bonded dsi

[Dmitry Baryshkov]

On Sun, Mar 08, 2026 at 03:06:27PM +0800, Pengyu Luo wrote:
> Recently, we round up new_hdisplay once at most, for bonded dsi, we
> may need twice, since they are independent links, we should round up
> each half separately. This also aligns with the hdisplay we program
> later in dsi_timing_setup()
> 
> Example:
> 	full_hdisplay = 1904, dsc_bpp = 8, bpc = 8
> 	new_full_hdisplay = DIV_ROUND_UP(1904 * 8, 8 * 3) = 635
> 
> if we use half display
> 	new_half_hdisplay = DIV_ROUND_UP(952 * 8, 8 * 3) = 318
> 	new_full_display = 636
> 
> Fixes: 7c9e4a554d4a ("drm/msm/dsi: Reduce pclk rate for compression")
> Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> ---
> v2: add a parameter desciption to the function doc (kernel test robot)
> ---
>  drivers/gpu/drm/msm/dsi/dsi_host.c | 32 +++++++++++++++++++++++-------
>  1 file changed, 25 insertions(+), 7 deletions(-)

The previous version has been picked up alrady (before the report came).
Would you mind sending a fix for it, describing the parameter?


-- 
With best wishes
Dmitry

Claude review: drm/msm/dsi: fix pclk calculation for bonded dsi

[Claude Code Review Bot]

Overall Series Review

Subject: drm/msm/dsi: fix pclk calculation for bonded dsi
Author: Pengyu Luo <mitltlatltl@gmail.com>
Patches: 2
Reviewed: 2026-03-09T07:54:17.628899

---

This is a single-patch fix for a pixel clock (pclk) calculation issue in bonded DSI mode with DSC compression. The fix is correct and well-motivated. In bonded DSI, each link carries half the display width independently, so the DSC compression rounding (DIV_ROUND_UP) should be applied to each half separately rather than to the full width. The current code rounds once on the full width, which can produce a result that differs by one pixel from what the hardware is actually programmed with in `dsi_timing_setup()`.

The patch has a Reviewed-by from Dmitry Baryshkov and a Fixes tag. The logic is sound and the change is minimal.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/msm/dsi: fix pclk calculation for bonded dsi

[Claude Code Review Bot]

Patch Review

**Correctness: Good**

The fix correctly addresses a real rounding discrepancy. Looking at `dsi_timing_setup()` (line 986-992 in the current tree), when `is_bonded_dsi` is true, `hdisplay /= 2` occurs *before* the DSC compression calculation at line 1035:

```c
hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), bytes_per_pclk);
```

So the timing path works on half the display width, meaning rounding is applied per-link. The pclk calculation should do the same, and this patch achieves that. The example in the commit message clearly demonstrates the off-by-one:

```
full_hdisplay = 1904, dsc_bpp = 8, bpc = 8
new_full_hdisplay = DIV_ROUND_UP(1904 * 8, 8 * 3) = 635

new_half_hdisplay = DIV_ROUND_UP(952 * 8, 8 * 3) = 318
new_full_display = 318 * 2 = 636
```

**Minor observations:**

1. The unrelated comment change from "dsi" to "DSI" on line 179 is a trivial style fix. It's fine to include but it's cosmetic noise in what is otherwise a Fixes-tagged patch:
```
-	 * the clock rates have to be split between the two dsi controllers.
+	 * the clock rates have to be split between the two DSI controllers.
```

2. The commit message says "v2: add a parameter desciption" — typo "desciption" should be "description", though this is below the `---` marker so it won't appear in the final git log.

3. There is a subtle assumption that `mode->hdisplay` is always even for bonded DSI (since `hdisplay /= 2` is integer division). This is the same assumption made in `dsi_timing_setup()` so it's consistent, but worth noting that odd hdisplay values in bonded mode would silently truncate a pixel.

**Overall: The patch looks correct and ready to merge.** The fix aligns the pclk calculation with what the hardware is actually programmed to do, and the logic is clean and well-commented.

---
Generated by Claude Code Patch Reviewer