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/dpu: Enable quad-pipe for DSC and dual-DSI case Date: Fri, 13 Mar 2026 14:21:22 +1000 Message-ID: In-Reply-To: <20260312-msm-next-quad-pipe-split-v19-4-4ffa2b06c996@linaro.org> References: <20260312-msm-next-quad-pipe-split-v19-0-4ffa2b06c996@linaro.org> <20260312-msm-next-quad-pipe-split-v19-4-4ffa2b06c996@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 **Review: Enables the feature, several issues.** 1. **Shadowed `kms` variable in `dpu_crtc_get_topology()`:** The function t= akes `struct dpu_kms *dpu_kms` as a parameter but also declares a local: ```c struct msm_drm_private *priv =3D crtc->dev->dev_private; struct dpu_kms *kms =3D to_dpu_kms(priv->kms); ``` The local `kms` is used at line 1428 (`kms->perf.max_core_clk_rate`) whi= le `dpu_kms` is used at line 1436 (`dpu_kms->catalog->caps->has_3d_merge`).= Both should be the same object, but having two variables referring to the = same thing is confusing and error-prone. The `dpu_kms` parameter should be = used consistently, and the local `kms`/`priv` declarations should be remove= d. 2. **`dpu_encoder_helper_get_3d_blend_mode()` condition change:** ```c // Before: dpu_cstate->num_mixers =3D=3D CRTC_DUAL_MIXERS // After: (dpu_cstate->num_mixers !=3D 1) ``` This changes the semantics. Previously, 3D merge was only used when exac= tly 2 mixers were present. Now it's used for any number of mixers > 1, incl= uding 3 or 4. For quad-pipe with 4 mixers, 3D merge should indeed be used f= or each pair, but this inline helper returns a single `BLEND_3D_H_ROW_INT` = regardless of which mixer pair is being configured. Is this correct for the= quad-pipe case where you'd want merge on pairs (0,1) and (2,3) but not acr= oss all 4? The callers need to handle this per-pair =E2=80=94 verify they d= o. 3. **DSC topology logic:** ```c if (topology.num_dsc) { if (num_rt_intf =3D=3D 1) topology.num_lm =3D 2; topology.num_dsc =3D topology.num_lm; } ``` For single-interface DSC, this forces `num_lm =3D 2` regardless of the r= esolution check above. This seems intentional for backward compatibility wi= th existing 2:2:1 DSC topologies. But for dual-interface with DSC, `topolog= y.num_dsc` is set to `topology.num_lm`, which could be 2 or 4. When `num_lm= =3D 2` and `num_rt_intf =3D 2`, you get `num_dsc =3D 2` (2:2:2). When `num= _lm =3D 4`, you get `num_dsc =3D 4` (4:4:2). This seems correct. However, the original code had a guard: `if (topology.num_intf >=3D 2 ||= dpu_kms->catalog->dsc_count >=3D 2)` checking DSC block availability. The = new code removes this check =E2=80=94 if the hardware has fewer than `num_l= m` DSC blocks, this could fail later during resource allocation. The resour= ce manager should catch this, but the early check was a useful safety net. 4. **`STAGES_PER_PLANE` changed from 1 to 2 unconditionally** =E2=80=94 thi= s doubles the size of `pipe[]` and `pipe_cfg[]` arrays in `dpu_plane_state`= for all platforms, even those that never use quad-pipe. This increases mem= ory usage per plane state. For embedded/mobile this is a minor concern but = worth noting. 5. **`MAX_CHANNELS_PER_ENC` changed from 2 to 4** =E2=80=94 the `hw_lm` and= `hw_mixer` arrays in `dpu_encoder_helper_reset_mixers()` are now sized 4. = The `dpu_rm_get_assigned_resources()` call that fills these arrays must als= o be capable of returning up to 4 entries. Verify this is the case. 6. **`min(2, topology.num_lm)` type mismatch**: `topology.num_lm` is `u32` = while `2` is `int`. The `min()` macro in the kernel requires matching types= . This should use `min_t(u32, 2, topology.num_lm)` or similar, otherwise it= may trigger a compile warning/error depending on kernel version and config. Has Reviewed-by from Dmitry Baryshkov and Jessica Zhang. **The shadowed var= iable and `min()` type issues should be addressed.** --- Generated by Claude Code Patch Reviewer