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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

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,