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 <[email protected]>
---
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,
