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 pclk rate calculation for bonded dsi Date: Mon, 09 Mar 2026 08:39:06 +1000 Message-ID: In-Reply-To: <20260306163255.215456-1-mitltlatltl@gmail.com> References: <20260306163255.215456-1-mitltlatltl@gmail.com> <20260306163255.215456-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 **Correctness:** The fix is correct. With the example values in the commit = message: - Full width: `DIV_ROUND_UP(1904 * 8, 24) =3D DIV_ROUND_UP(15232, 24) =3D 6= 35` - Per-link: `DIV_ROUND_UP(952 * 8, 24) =3D DIV_ROUND_UP(7616, 24) =3D 318`,= then `318 * 2 =3D 636` The difference of 1 pixel matters because `dsi_timing_setup()` at line 1008= halves `hdisplay` before computing the compressed width per link, so the p= clk rate calculation should match that behavior. **Code review:** The implementation is clean. The split-round-recombine pattern is easy to f= ollow: ```c hdisplay =3D mode->hdisplay; if (is_bonded_dsi) hdisplay /=3D 2; new_hdisplay =3D DIV_ROUND_UP(hdisplay * drm_dsc_get_bpp_int(dsc), dsc->bits_per_component * 3); if (is_bonded_dsi) new_hdisplay *=3D 2; ``` **Minor observations:** 1. The commit message has a small typo: `new_full_display` should be `new_f= ull_hdisplay` on the last line of the example to be consistent with the nam= ing used just above it. 2. **Subtle divergence with `dsi_timing_setup`:** The commit message claims= this "aligns with the hdisplay we program later in `dsi_timing_setup()`", = but the two calculations are not strictly identical. `dsi_timing_setup()` u= ses `msm_dsc_get_bytes_per_line(dsc)` (i.e., `slice_count * slice_chunk_siz= e`) divided by `bytes_per_pclk`, while `dsi_adjust_pclk_for_compression()` = uses `DIV_ROUND_UP(hdisplay * bpp_int, bpc * 3)`. These formulas converge o= nly when the DSC slice parameters produce the same effective ratio. The *ro= unding behavior* per half-link does align, which is what matters for this f= ix, but the comment could be slightly more precise about what "aligns" mean= s =E2=80=94 it's the halving of hdisplay before compression calculation, no= t the exact formula. 3. The function only handles VIDEO mode pclk calculation (the `FIXME` comme= nt above notes CMD mode is different). This is fine since the `mult_frac` l= ine at the end is only meaningful for video timing. **Overall:** The patch correctly fixes a real bug where the pclk rate could= be off by one compressed pixel's worth of clock cycles in bonded DSI + DSC= configurations. Recommend applying with the typo fix in the commit message. --- Generated by Claude Code Patch Reviewer