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: support DSC configurations with slice_per_pkt > 1 Date: Mon, 25 May 2026 20:00:01 +1000 Message-ID: In-Reply-To: <20260521-sm8650-7-1-bonded-dsi-v4-1-a4dd5e0850f1@linaro.org> References: <20260521-sm8650-7-1-bonded-dsi-v4-0-a4dd5e0850f1@linaro.org> <20260521-sm8650-7-1-bonded-dsi-v4-1-a4dd5e0850f1@linaro.org> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Concept**: Replaces the hardcoded `slice_per_pkt =3D 1` assumption in the= MSM DSI host with support for configurable values. A `dsc_slice_per_pkt` f= ield is added to `struct drm_dsc_config`. **Issues:** 1. **Division by zero risk**: In `dsi_update_dsc_timing()`, the new code do= es: ```c pkt_per_line =3D slice_per_intf / msm_host->dsc_slice_per_pkt; ``` If `dsc_slice_per_pkt` is 0 (e.g. a panel driver that sets `dsc` but doe= sn't set this new field, and the `?: 1` fallback in `dsi_host_attach` is so= mehow bypassed or a different code path sets the DSC config), this is an OO= PS. The fallback at attach time (`dsi->dsc->dsc_slice_per_pkt ?: 1`) is goo= d but fragile =E2=80=94 consider also clamping or validating in `dsi_update= _dsc_timing` itself. 2. **Questionable placement in `drm_dsc_config`**: The comment says "This i= s not part of DSC standard, and only used in some DSI panels so far." Addin= g transport-layer packetization config to a struct representing the DSC sta= ndard parameter set conflates concerns. The existing `msm_dsi_host` already= has a local `dsc_slice_per_pkt` field =E2=80=94 you could keep it purely t= here, and pass it from `mipi_dsi_device` via a separate field rather than p= olluting `drm_dsc_config`. This needs buy-in from DRM core maintainers (Max= ime, Thomas, etc.). 3. **Naming redundancy**: The field is called `dsc_slice_per_pkt` inside `s= truct drm_dsc_config`. Since it's already in a `dsc` struct, just `slice_pe= r_pkt` would be sufficient and consistent with how other fields like `slice= _count`, `slice_width` etc. are named. 4. **The `pkt_per_line` log2 encoding check**: The existing code at line 96= 4 warns if `pkt_per_line > 4` because the register field is log2-encoded an= d only supports 1, 2, 4. With `slice_per_pkt > 1`, `pkt_per_line` can now b= e smaller (e.g., 4 slices / 4 slice_per_pkt =3D 1), but there's no validati= on that `slice_per_intf` is evenly divisible by `dsc_slice_per_pkt`, which = could give non-power-of-2 results. --- Generated by Claude Code Patch Reviewer