public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH] drm/msm/dsi: fix pclk rate calculation for bonded dsi
@ 2026-03-06 16:32 Pengyu Luo
  2026-03-06 16:50 ` Dmitry Baryshkov
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: Pengyu Luo @ 2026-03-06 16:32 UTC (permalink / raw)
  To: Rob Clark, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang,
	Sean Paul, Marijn Suijten, David Airlie, Simona Vetter
  Cc: Pengyu Luo, linux-arm-msm, dri-devel, freedreno, linux-kernel

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>
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index e8e83ee61..db6da9937 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -584,13 +584,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,7 +620,7 @@ 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
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] drm/msm/dsi: fix pclk rate calculation for bonded dsi
  2026-03-06 16:32 [PATCH] drm/msm/dsi: fix pclk rate calculation for bonded dsi Pengyu Luo
@ 2026-03-06 16:50 ` Dmitry Baryshkov
  2026-03-07 19:57 ` kernel test robot
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 5+ messages in thread
From: Dmitry Baryshkov @ 2026-03-06 16:50 UTC (permalink / raw)
  To: Pengyu Luo
  Cc: Rob Clark, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang,
	Sean Paul, Marijn Suijten, David Airlie, Simona Vetter,
	linux-arm-msm, dri-devel, freedreno, linux-kernel

On Sat, Mar 07, 2026 at 12:32:38AM +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>
> ---
>  drivers/gpu/drm/msm/dsi/dsi_host.c | 29 +++++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>


-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] drm/msm/dsi: fix pclk rate calculation for bonded dsi
  2026-03-06 16:32 [PATCH] drm/msm/dsi: fix pclk rate calculation for bonded dsi Pengyu Luo
  2026-03-06 16:50 ` Dmitry Baryshkov
@ 2026-03-07 19:57 ` kernel test robot
  2026-03-08 22:39 ` Claude review: " Claude Code Review Bot
  2026-03-08 22:39 ` Claude Code Review Bot
  3 siblings, 0 replies; 5+ messages in thread
From: kernel test robot @ 2026-03-07 19:57 UTC (permalink / raw)
  To: Pengyu Luo, Rob Clark, Dmitry Baryshkov, Abhinav Kumar,
	Jessica Zhang, Sean Paul, Marijn Suijten, David Airlie,
	Simona Vetter
  Cc: oe-kbuild-all, Pengyu Luo, linux-arm-msm, dri-devel, freedreno,
	linux-kernel

Hi Pengyu,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on linus/master v7.0-rc2 next-20260306]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Pengyu-Luo/drm-msm-dsi-fix-pclk-rate-calculation-for-bonded-dsi/20260307-010943
base:   https://gitlab.freedesktop.org/drm/misc/kernel.git drm-misc-next
patch link:    https://lore.kernel.org/r/20260306163255.215456-1-mitltlatltl%40gmail.com
patch subject: [PATCH] drm/msm/dsi: fix pclk rate calculation for bonded dsi
config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20260308/202603080314.XeqyRZ7A-lkp@intel.com/config)
compiler: arc-linux-gcc (GCC) 15.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260308/202603080314.XeqyRZ7A-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202603080314.XeqyRZ7A-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> Warning: drivers/gpu/drm/msm/dsi/dsi_host.c:590 function parameter 'is_bonded_dsi' not described in 'dsi_adjust_pclk_for_compression'
>> Warning: drivers/gpu/drm/msm/dsi/dsi_host.c:590 function parameter 'is_bonded_dsi' not described in 'dsi_adjust_pclk_for_compression'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Claude review: drm/msm/dsi: fix pclk rate calculation for bonded dsi
  2026-03-06 16:32 [PATCH] drm/msm/dsi: fix pclk rate calculation for bonded dsi Pengyu Luo
  2026-03-06 16:50 ` Dmitry Baryshkov
  2026-03-07 19:57 ` kernel test robot
@ 2026-03-08 22:39 ` Claude Code Review Bot
  2026-03-08 22:39 ` Claude Code Review Bot
  3 siblings, 0 replies; 5+ messages in thread
From: Claude Code Review Bot @ 2026-03-08 22:39 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: drm/msm/dsi: fix pclk rate calculation for bonded dsi
Author: Pengyu Luo <mitltlatltl@gmail.com>
Patches: 3
Reviewed: 2026-03-09T08:39:05.779362

---

This is a single-patch bug fix for the DSI pixel clock rate calculation when using bonded (dual-link) DSI with Display Stream Compression (DSC). The fix is logically sound: in bonded DSI mode, each link handles half the display width independently, so the DSC-compressed width should be calculated per-link (with per-link rounding) rather than for the full width. The commit message provides a clear numerical example demonstrating the off-by-one difference.

The fix is small, well-scoped, and correctly addresses the rounding discrepancy. It has appropriate `Fixes:` tag.

**Verdict: Good patch, minor nits below.**

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Claude review: drm/msm/dsi: fix pclk rate calculation for bonded dsi
  2026-03-06 16:32 [PATCH] drm/msm/dsi: fix pclk rate calculation for bonded dsi Pengyu Luo
                   ` (2 preceding siblings ...)
  2026-03-08 22:39 ` Claude review: " Claude Code Review Bot
@ 2026-03-08 22:39 ` Claude Code Review Bot
  3 siblings, 0 replies; 5+ messages in thread
From: Claude Code Review Bot @ 2026-03-08 22:39 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Correctness:** The fix is correct. With the example values in the commit message:
- Full width: `DIV_ROUND_UP(1904 * 8, 24) = DIV_ROUND_UP(15232, 24) = 635`
- Per-link: `DIV_ROUND_UP(952 * 8, 24) = DIV_ROUND_UP(7616, 24) = 318`, then `318 * 2 = 636`

The difference of 1 pixel matters because `dsi_timing_setup()` at line 1008 halves `hdisplay` before computing the compressed width per link, so the pclk rate calculation should match that behavior.

**Code review:**

The implementation is clean. The split-round-recombine pattern is easy to follow:

```c
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;
```

**Minor observations:**

1. The commit message has a small typo: `new_full_display` should be `new_full_hdisplay` on the last line of the example to be consistent with the naming used just above it.

2. **Subtle divergence with `dsi_timing_setup`:** The commit message claims this "aligns with the hdisplay we program later in `dsi_timing_setup()`", but the two calculations are not strictly identical. `dsi_timing_setup()` uses `msm_dsc_get_bytes_per_line(dsc)` (i.e., `slice_count * slice_chunk_size`) divided by `bytes_per_pclk`, while `dsi_adjust_pclk_for_compression()` uses `DIV_ROUND_UP(hdisplay * bpp_int, bpc * 3)`. These formulas converge only when the DSC slice parameters produce the same effective ratio. The *rounding behavior* per half-link does align, which is what matters for this fix, but the comment could be slightly more precise about what "aligns" means — it's the halving of hdisplay before compression calculation, not the exact formula.

3. The function only handles VIDEO mode pclk calculation (the `FIXME` comment above notes CMD mode is different). This is fine since the `mult_frac` line at the end is only meaningful for video timing.

**Overall:** The patch correctly fixes a real bug where the pclk rate could be off by one compressed pixel's worth of clock cycles in bonded DSI + DSC configurations. Recommend applying with the typo fix in the commit message.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2026-03-08 22:39 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-03-06 16:32 [PATCH] drm/msm/dsi: fix pclk rate calculation for bonded dsi Pengyu Luo
2026-03-06 16:50 ` Dmitry Baryshkov
2026-03-07 19:57 ` kernel test robot
2026-03-08 22:39 ` Claude review: " Claude Code Review Bot
2026-03-08 22:39 ` Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox