From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/msm/dsi: fix hdisplay calculation when programming dsi registers Date: Sun, 15 Feb 2026 19:06:21 +1000 Message-ID: In-Reply-To: <20260214105145.105308-1-mitltlatltl@gmail.com> References: <20260214105145.105308-1-mitltlatltl@gmail.com> <20260214105145.105308-1-mitltlatltl@gmail.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review The commit message states the fix is derived by "cross-comparing the calculat= ion with the calculation in `dsi_adjust_pclk_for_compression()`." However, `d= si_adjust_pclk_for_compression` uses `dsc->bits_per_component * 3` as the unc= ompressed bits-per-pixel: ```c int new_hdisplay =3D 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 =3D 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_t= o_bpp` returns 24. But `bits_per_component * 3` would be 30. These two functi= ons would compute different `hdisplay` values for the same panel. If the inte= nt is to match `dsi_adjust_pclk_for_compression`, shouldn't this use `dsc->bi= ts_per_component * 3` instead? Could you clarify which divisor is actually co= rrect for the timing register programming, and why it differs from the pclk a= djustment function? Regarding the removal of the video mode guard: > - if (wide_bus_enabled && !(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)) > - bytes_per_pclk =3D 6; > + if (wide_bus_enabled) > + bits_per_pclk =3D 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 pre= venting widebus from affecting video mode, not about CMD mode not reaching th= is code. Looking at the code, CMD mode does reach this path -- the `if (msm_h= ost->dsc)` block runs before the `if (msm_host->mode_flags & MIPI_DSI_MODE_VI= DEO)` / `else` split, so both video and CMD mode execute the hdisplay calcula= tion. For CMD mode with DSC, the `hdisplay` value computed here feeds into `D= SI_CMD_MDP_STREAM0_TOTAL_H_TOTAL`. Could you explain why it is now safe to ap= ply widebus bits-per-pclk to video mode when it was previously deliberately e= xcluded? One minor concern with the error return from `mipi_dsi_pixel_format_to_bpp`: > + bits_per_pclk =3D mipi_dsi_pixel_format_to_bpp(msm_host->format); `mipi_dsi_pixel_format_to_bpp` returns `-EINVAL` for unrecognized formats. Si= nce `bits_per_pclk` is `u32`, this would wrap to `~0U`, causing the `DIV_ROUN= D_UP` to yield 1 for `hdisplay`. The format should always be valid by this po= int (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 =3D 24; > > - hdisplay =3D DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), byt= es_per_pclk); > + hdisplay =3D 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 m= atches 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 pixe= l 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 s= ense but then `dsi_adjust_pclk_for_compression` is arguably the one with the = inconsistency. The commit message's claim about cross-comparing with that fun= ction then needs qualification. In summary: the main question is whether `mipi_dsi_pixel_format_to_bpp` or `b= its_per_component * 3` is the correct divisor, especially for 10bpc panels. T= he patch should either use the same formula as `dsi_adjust_pclk_for_compressi= on` (for consistency) or explain why they should differ. Also, the commit mes= sage'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