[PATCH] drm/msm/dsi: Restore widebus calculation for CMDMode panels

[PATCH] drm/msm/dsi: Restore widebus calculation for CMDMode panels

[Marijn Suijten]

Commit ac47870fd795 ("drm/msm/dsi: fix hdisplay calculation when
programming dsi registers") makes a false and ungrounded statement that
"Since CMD mode does not use this, we can remove !(msm_host->mode_flags
& MIPI_DSI_MODE_VIDEO) safely." which isn't the case at all.
dsi_timing_setup() affects both command mode and video mode panels, and
by no longer having any path that sets bits_per_pclk = 48 (contrary to
the updated code-comment) all DSI DSC panels on SM8350 and above (i.e.
with widebus support) regress thanks to this patch.

The entire reason that video mode was originally omitted from this code
path is because it was never tested before; any change that enables
widebus for video mode panels should not regress the command mode path.

Thus add back the path allows 6 bytes or 48 bits to be sent per pclk
on command mode DSI panels with widebus, restoring the panel on devices
like the Sony Xperia 1 III and upwards.

Fixes: ac47870fd795 ("drm/msm/dsi: fix hdisplay calculation when programming dsi registers")
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
---
In addition I can't say I understand the original commit message
at all; it mentions a BPC=10 mode however the highest format that
mipi_dsi_pixel_format_to_bpp supports is RGB888 thus it won't
ever return anything above 24, which is the original amount the
non-command-mode path defaulted to (regardless of widebus)...  Was that
patch doing anything for video mode at all?

It feels like the conditional introduced here is only making things more
confusing, but I don't have enough input to confirm what the video-mode
path should be doing in widebus mode (multiply BPC by the number of
components and by 2 in case of widebus?).
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index e8e83ee61eb0..168e37e38fc7 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -1029,10 +1029,14 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 		 * unused anyway.
 		 */
 		h_total -= hdisplay;
-		if (wide_bus_enabled)
-			bits_per_pclk = mipi_dsi_pixel_format_to_bpp(msm_host->format);
-		else
+		if (wide_bus_enabled) {
+			if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
+				bits_per_pclk = mipi_dsi_pixel_format_to_bpp(msm_host->format);
+			else
+				bits_per_pclk = 48;
+		} else {
 			bits_per_pclk = 24;
+		}
 
 		hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc) * 8, bits_per_pclk);
 

---
base-commit: 3fa5e5702a82d259897bd7e209469bc06368bf31
change-id: 20260311-dsi-dsc-regresses-again-4be27dfc4001

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

Re: [PATCH] drm/msm/dsi: Restore widebus calculation for CMDMode panels

[Pengyu Luo]

On Thu, Mar 12, 2026 at 6:31 AM Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> Commit ac47870fd795 ("drm/msm/dsi: fix hdisplay calculation when
> programming dsi registers") makes a false and ungrounded statement that
> "Since CMD mode does not use this, we can remove !(msm_host->mode_flags
> & MIPI_DSI_MODE_VIDEO) safely." which isn't the case at all.
> dsi_timing_setup() affects both command mode and video mode panels, and
> by no longer having any path that sets bits_per_pclk = 48 (contrary to
> the updated code-comment) all DSI DSC panels on SM8350 and above (i.e.
> with widebus support) regress thanks to this patch.
>
> The entire reason that video mode was originally omitted from this code
> path is because it was never tested before; any change that enables
> widebus for video mode panels should not regress the command mode path.
>
> Thus add back the path allows 6 bytes or 48 bits to be sent per pclk
> on command mode DSI panels with widebus, restoring the panel on devices
> like the Sony Xperia 1 III and upwards.
>
> Fixes: ac47870fd795 ("drm/msm/dsi: fix hdisplay calculation when programming dsi registers")
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> ---

Apologies, I messed up, I had sent the same fixes days ago.
https://lore.kernel.org/linux-arm-msm/20260307111250.105772-2-mitltlatltl@gmail.com/

> In addition I can't say I understand the original commit message
> at all; it mentions a BPC=10 mode however the highest format that
> mipi_dsi_pixel_format_to_bpp supports is RGB888 thus it won't
> ever return anything above 24, which is the original amount the
> non-command-mode path defaulted to (regardless of widebus)...  Was that
> patch doing anything for video mode at all?
>

RGB888 is the dst bpc, which is tied to qcom,mdss-dsi-bpp in the downstream.

Actually, we should use src bpc here, another fixe
https://lore.kernel.org/linux-arm-msm/20260307111250.105772-1-mitltlatltl@gmail.com/

> It feels like the conditional introduced here is only making things more
> confusing, but I don't have enough input to confirm what the video-mode
> path should be doing in widebus mode (multiply BPC by the number of
> components and by 2 in case of widebus?).

I left a comment. For CMD mode, it consumes 6 bytes, for Video mode,

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



Best wishes,
Pengyu Luo

Re: [PATCH] drm/msm/dsi: Restore widebus calculation for CMDMode panels

[AngeloGioacchino Del Regno]

Il 11/03/26 23:31, Marijn Suijten ha scritto:
> Commit ac47870fd795 ("drm/msm/dsi: fix hdisplay calculation when
> programming dsi registers") makes a false and ungrounded statement that
> "Since CMD mode does not use this, we can remove !(msm_host->mode_flags
> & MIPI_DSI_MODE_VIDEO) safely." which isn't the case at all.
> dsi_timing_setup() affects both command mode and video mode panels, and
> by no longer having any path that sets bits_per_pclk = 48 (contrary to
> the updated code-comment) all DSI DSC panels on SM8350 and above (i.e.
> with widebus support) regress thanks to this patch.
> 
> The entire reason that video mode was originally omitted from this code
> path is because it was never tested before; any change that enables
> widebus for video mode panels should not regress the command mode path.
> 
> Thus add back the path allows 6 bytes or 48 bits to be sent per pclk
> on command mode DSI panels with widebus, restoring the panel on devices
> like the Sony Xperia 1 III and upwards.
> 
> Fixes: ac47870fd795 ("drm/msm/dsi: fix hdisplay calculation when programming dsi registers")
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> ---
> In addition I can't say I understand the original commit message
> at all; it mentions a BPC=10 mode however the highest format that
> mipi_dsi_pixel_format_to_bpp supports is RGB888 thus it won't
> ever return anything above 24, which is the original amount the
> non-command-mode path defaulted to (regardless of widebus)...  Was that
> patch doing anything for video mode at all?
> 
> It feels like the conditional introduced here is only making things more
> confusing, but I don't have enough input to confirm what the video-mode
> path should be doing in widebus mode (multiply BPC by the number of
> components and by 2 in case of widebus?).
> ---
>   drivers/gpu/drm/msm/dsi/dsi_host.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index e8e83ee61eb0..168e37e38fc7 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -1029,10 +1029,14 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>   		 * unused anyway.
>   		 */
>   		h_total -= hdisplay;
> -		if (wide_bus_enabled)
> -			bits_per_pclk = mipi_dsi_pixel_format_to_bpp(msm_host->format);
> -		else
> +		if (wide_bus_enabled) {
> +			if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
> +				bits_per_pclk = mipi_dsi_pixel_format_to_bpp(msm_host->format);
> +			else
> +				bits_per_pclk = 48;
> +		} else {
>   			bits_per_pclk = 24;
> +		}

Marijn et al,

I don't really know this hardware specifically, but I had a very fast look
at this patch, and I believe that however you put it, this looks like being
either plain wrong or very weird.

I think this should be, instead....

	/* (don't add this comment) assuming RGB888/666, this will be 24 for both Command 
and Video modes */
	bits_per_pclk = mipi_dsi_pixel_format_to_bpp(msm_host->format);

	/* Can send twice the bits per clock if WideBus with Video Mode */
	if (wide_bus_enabled && msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
		bits_per_pclk *= 2;

...because, unless there's a hardware quirk (and that should be really clarified
with a big comment stating that), I don't see why command mode should always be
24 bits per clock, and I also don't see why a widebus case would do 48 bits per
clock even in the RGB666_PACKED and RGB565 cases (which may not even be possible
but *meh*).

Just my two cents.

Reminding you all again that I don't know this HW, so I may have said something
wrong here.

Cheers!
Angelo

>   
>   		hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc) * 8, bits_per_pclk);
>   
> 
> ---
> base-commit: 3fa5e5702a82d259897bd7e209469bc06368bf31
> change-id: 20260311-dsi-dsc-regresses-again-4be27dfc4001
> 
> Best regards,

Re: [PATCH] drm/msm/dsi: Restore widebus calculation for CMDMode panels

[Marijn Suijten]

On 2026-03-12 14:18:43, AngeloGioacchino Del Regno wrote:
...
> Marijn et al,
> 
> I don't really know this hardware specifically, but I had a very fast look
> at this patch, and I believe that however you put it, this looks like being
> either plain wrong or very weird.

I would say I also don't know the hardware here very well, and which parameters
apply when and were.  Most importantly, just be aware that the code we're
looking at is for **DSC**, i.e. when we're no longer transfering single pixels
per pclk, but compressed slices of a given number of bytes that can loosely be
converted back to a "number of pixels".

> I think this should be, instead....
> 
> 	/* (don't add this comment) assuming RGB888/666, this will be 24 for both Command 
> and Video modes */
> 	bits_per_pclk = mipi_dsi_pixel_format_to_bpp(msm_host->format);

That would make sense, if it weren't for DSC.  The older downstream kernel I'm
looking at -for at least CMD mode- disregards the format entirely, and just
divides the "width" (computed like msm_dsc_get_bytes_per_line()) by 3 or 6
depending on widebus.

In other words that's clearly demonstrating that input/output "bits per
component" (in uncompressed space) are irrelevant when we're transmitting
compressed pixels.

> 	/* Can send twice the bits per clock if WideBus with Video Mode */
> 	if (wide_bus_enabled && msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
> 		bits_per_pclk *= 2;

And this is where the original patch got things wrong too; widebus isn't limited
to VIDEO mode, in fact we were specifically only allowing CMD mode to use it
thus far.  That comment and condition should be inverted.

In this old downstream, widebus also doesn't seem to affect the pclk rate for
VIDEO mode, only the ratio between bits per component and the chroma format
(making up the total size of the pixel) and the configured "bits per pixel"
value are taken into account; that ratio is generally 3.

> ...because, unless there's a hardware quirk (and that should be really clarified
> with a big comment stating that), I don't see why command mode should always be
> 24 bits per clock, and I also don't see why a widebus case would do 48 bits per
> clock even in the RGB666_PACKED and RGB565 cases (which may not even be possible
> but *meh*).
> 
> Just my two cents.
> 
> Reminding you all again that I don't know this HW, so I may have said something
> wrong here.
>
> Cheers!
> Angelo

Kind regards,
Marijn

Claude review: drm/msm/dsi: Restore widebus calculation for CMDMode panels

[Claude Code Review Bot]

Overall Series Review

Subject: drm/msm/dsi: Restore widebus calculation for CMDMode panels
Author: Marijn Suijten <marijn.suijten@somainline.org>
Patches: 4
Reviewed: 2026-03-13T14:40:21.566414

---

This is a single-patch fix for a regression introduced by commit ac47870fd795 ("drm/msm/dsi: fix hdisplay calculation when programming dsi registers"). That commit removed the command-mode widebus path that set `bits_per_pclk = 48`, incorrectly claiming CMD mode doesn't use this code path. The result is that all DSI DSC command-mode panels on SM8350+ (with widebus support) broke — the `hdisplay` calculation became wrong because `mipi_dsi_pixel_format_to_bpp()` returns 24 for RGB888, identical to the non-widebus case, so widebus effectively did nothing.

The fix is logically correct and restores the original behavior for CMD mode while preserving the video-mode change from ac47870fd795.

**Note:** The current drm-next tree (line 1030 of `dsi_host.c`) already has a different refactoring of this area that uses `bytes_per_pclk` (3/6) instead of `bits_per_pclk` (24/48), and already includes the `!(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)` guard. This means the regression may have already been fixed differently on drm-next, and this patch targets a tree where ac47870fd795 exists but that subsequent refactoring hasn't landed. The maintainers should verify which tree this needs to go into (likely a stable backport target).

---
Generated by Claude Code Patch Reviewer

Claude review: drm/msm/dsi: Restore widebus calculation for CMDMode panels

[Claude Code Review Bot]

Patch Review

**Correctness: The fix is sound.** The commit message clearly explains the regression and why the removed check was necessary. The code change correctly restores the `bits_per_pclk = 48` path for command-mode DSC panels with widebus:

```c
-		if (wide_bus_enabled)
-			bits_per_pclk = mipi_dsi_pixel_format_to_bpp(msm_host->format);
-		else
+		if (wide_bus_enabled) {
+			if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
+				bits_per_pclk = mipi_dsi_pixel_format_to_bpp(msm_host->format);
+			else
+				bits_per_pclk = 48;
+		} else {
 			bits_per_pclk = 24;
+		}
```

**Minor observations:**

1. **Video-mode widebus path questionable (noted by author):** The author rightly questions in the cover notes whether `mipi_dsi_pixel_format_to_bpp()` makes sense for the video-mode widebus path — for RGB888 it returns 24, which is the same as the non-widebus case. This means widebus is still effectively a no-op for video mode DSC panels. However, this patch is correct in not trying to fix that — it's a minimal regression fix that restores the working CMD mode path without changing the (possibly still broken) video mode behavior.

2. **Commit message typo:** Line 99 says "add back the path allows" — should read "add back the path **that** allows".

3. **drm-next divergence:** As noted above, the drm-next tree at `dsi_host.c:1030-1033` already has:
   ```c
   if (wide_bus_enabled && !(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO))
       bytes_per_pclk = 6;
   else
       bytes_per_pclk = 3;
   ```
   This is functionally equivalent to the fix (6 bytes = 48 bits, 3 bytes = 24 bits for CMD mode), and also doesn't apply `mipi_dsi_pixel_format_to_bpp()` for video mode at all. The maintainers should clarify whether this patch is intended for a stable branch where the intermediate refactoring hasn't been applied.

**Verdict:** The fix is correct and well-motivated. The only action item is the minor typo in the commit message and clarifying the target tree/branch.

---
Generated by Claude Code Patch Reviewer