[PATCH] drm/msm/dsi: fix hdisplay calculation when programming dsi registers

[PATCH] drm/msm/dsi: fix hdisplay calculation when programming dsi registers

[Pengyu Luo]

Recently, the hdisplay calculation is working for 3:1 compressed ratio
only. If we have a video panel with DSC BPP = 8, and BPC = 10, we still
use the default bits_per_pclk = 24, then we get the wrong hdisplay. We
can draw the conclusion by cross-comparing the calculation with the
calculation in dsi_adjust_pclk_for_compression().

Since CMD mode does not use this, we can remove
!(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO) safely.

Fixes: efcbd6f9cdeb ("drm/msm/dsi: Enable widebus for DSI")
Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index e0de545d4..e8e83ee61 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -993,7 +993,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 
 	if (msm_host->dsc) {
 		struct drm_dsc_config *dsc = msm_host->dsc;
-		u32 bytes_per_pclk;
+		u32 bits_per_pclk;
 
 		/* update dsc params with timing params */
 		if (!dsc || !mode->hdisplay || !mode->vdisplay) {
@@ -1015,7 +1015,9 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 
 		/*
 		 * DPU sends 3 bytes per pclk cycle to DSI. If widebus is
-		 * enabled, bus width is extended to 6 bytes.
+		 * enabled, MDP always sends out 48-bit compressed data per
+		 * pclk and on average, DSI consumes an amount of compressed
+		 * data equivalent to the uncompressed pixel depth per pclk.
 		 *
 		 * Calculate the number of pclks needed to transmit one line of
 		 * the compressed data.
@@ -1027,12 +1029,12 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 		 * unused anyway.
 		 */
 		h_total -= hdisplay;
-		if (wide_bus_enabled && !(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO))
-			bytes_per_pclk = 6;
+		if (wide_bus_enabled)
+			bits_per_pclk = mipi_dsi_pixel_format_to_bpp(msm_host->format);
 		else
-			bytes_per_pclk = 3;
+			bits_per_pclk = 24;
 
-		hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), bytes_per_pclk);
+		hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc) * 8, bits_per_pclk);
 
 		h_total += hdisplay;
 		ha_end = ha_start + hdisplay;
-- 
2.53.0

Claude review: drm/msm/dsi: fix hdisplay calculation when programming dsi registers

[Claude Code Review Bot]

Patch Review

The commit message states the fix is derived by "cross-comparing the calculation with the calculation in `dsi_adjust_pclk_for_compression()`." However, `dsi_adjust_pclk_for_compression` uses `dsc->bits_per_component * 3` as the uncompressed bits-per-pixel:

```c
int new_hdisplay = DIV_ROUND_UP(mode->hdisplay * drm_dsc_get_bpp_int(dsc),
		dsc->bits_per_component * 3);
```

The patch instead uses `mipi_dsi_pixel_format_to_bpp(msm_host->format)`:

> +		if (wide_bus_enabled)
> +			bits_per_pclk = mipi_dsi_pixel_format_to_bpp(msm_host->format);

For a 10bpc panel (the scenario the commit message highlights as broken), the DSI pixel format is still `MIPI_DSI_FMT_RGB888`, so `mipi_dsi_pixel_format_to_bpp` returns 24. But `bits_per_component * 3` would be 30. These two functions would compute different `hdisplay` values for the same panel. If the intent is to match `dsi_adjust_pclk_for_compression`, shouldn't this use `dsc->bits_per_component * 3` instead? Could you clarify which divisor is actually correct for the timing register programming, and why it differs from the pclk adjustment function?

Regarding the removal of the video mode guard:

> -		if (wide_bus_enabled && !(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO))
> -			bytes_per_pclk = 6;
> +		if (wide_bus_enabled)
> +			bits_per_pclk = mipi_dsi_pixel_format_to_bpp(msm_host->format);

The old code intentionally applied widebus only for CMD mode (not video). The commit message says "Since CMD mode does not use this, we can remove `!(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)` safely." But the guard was about preventing widebus from affecting video mode, not about CMD mode not reaching this code. Looking at the code, CMD mode does reach this path -- the `if (msm_host->dsc)` block runs before the `if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)` / `else` split, so both video and CMD mode execute the hdisplay calculation. For CMD mode with DSC, the `hdisplay` value computed here feeds into `DSI_CMD_MDP_STREAM0_TOTAL_H_TOTAL`. Could you explain why it is now safe to apply widebus bits-per-pclk to video mode when it was previously deliberately excluded?

One minor concern with the error return from `mipi_dsi_pixel_format_to_bpp`:

> +			bits_per_pclk = mipi_dsi_pixel_format_to_bpp(msm_host->format);

`mipi_dsi_pixel_format_to_bpp` returns `-EINVAL` for unrecognized formats. Since `bits_per_pclk` is `u32`, this would wrap to `~0U`, causing the `DIV_ROUND_UP` to yield 1 for `hdisplay`. The format should always be valid by this point (it was accepted during `dsi_dev_attach`), so this is not a practical bug, but it is worth noting that no existing caller in this function checks the return value either.

The non-widebus path is algebraically equivalent to the old code:

> +		else
> +			bits_per_pclk = 24;
>
> -		hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), bytes_per_pclk);
> +		hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc) * 8, bits_per_pclk);

`DIV_ROUND_UP(bytes * 8, 24)` simplifies to `DIV_ROUND_UP(bytes, 3)`, which matches the old `DIV_ROUND_UP(bytes, 3)`. No issue there.

The updated comment is better at describing the actual hardware behavior:

> +		 * enabled, MDP always sends out 48-bit compressed data per
> +		 * pclk and on average, DSI consumes an amount of compressed
> +		 * data equivalent to the uncompressed pixel depth per pclk.

This describes MDP sending 48 bits and DSI consuming at the uncompressed pixel depth rate. If the uncompressed pixel depth for 10bpc is indeed meant to be 24 (the DSI bus format bpp) rather than 30 (`bpc * 3`), this comment makes sense but then `dsi_adjust_pclk_for_compression` is arguably the one with the inconsistency. The commit message's claim about cross-comparing with that function then needs qualification.

In summary: the main question is whether `mipi_dsi_pixel_format_to_bpp` or `bits_per_component * 3` is the correct divisor, especially for 10bpc panels. The patch should either use the same formula as `dsi_adjust_pclk_for_compression` (for consistency) or explain why they should differ. Also, the commit message's explanation for removing the video mode guard doesn't quite track with the code structure and could use clarification.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/msm/dsi: fix hdisplay calculation when programming dsi registers

[Claude Code Review Bot]

Overall Series Review

Subject: drm/msm/dsi: fix hdisplay calculation when programming dsi registers
Author: Pengyu Luo <mitltlatltl@gmail.com>
Patches: 1
Reviewed: 2026-02-15T19:06:21.715308

---

This is a single patch fixing the `hdisplay` calculation in `dsi_timing_setup()` for DSC-compressed video when widebus is enabled. The commit message claims the current code only works for a 3:1 compressed ratio and that a panel with DSC BPP=8 and BPC=10 would get wrong values because the default `bits_per_pclk` of 24 doesn't account for pixel formats with different bit depths.

The patch renames `bytes_per_pclk` to `bits_per_pclk`, replaces the hardcoded 3/6 values with `mipi_dsi_pixel_format_to_bpp()` (when widebus is enabled) or 24 (when not), and adjusts the division to work in bits rather than bytes. It also removes the `MIPI_DSI_MODE_VIDEO` guard, so widebus now takes effect for video mode too (previously it was CMD-only).

The core mathematical concern is whether `mipi_dsi_pixel_format_to_bpp(msm_host->format)` is the correct divisor. The commit message says this should cross-compare with `dsi_adjust_pclk_for_compression()`, which uses `dsc->bits_per_component * 3` as its uncompressed-bpp divisor. For 8bpc panels with RGB888 these agree (both yield 24), but they diverge for 10bpc or 12bpc panels where `bpc * 3` would be 30 or 36 while `mipi_dsi_pixel_format_to_bpp(MIPI_DSI_FMT_RGB888)` still returns 24. This discrepancy warrants clarification.

---
Generated by Claude Code Patch Reviewer