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 virtual planes by default Date: Fri, 27 Feb 2026 12:05:40 +1000 Message-ID: In-Reply-To: <20260226-dpu-enable-virt-planes-v2-1-87971236fe86@oss.qualcomm.com> References: <20260226-dpu-enable-virt-planes-v2-1-87971236fe86@oss.qualcomm.com> <20260226-dpu-enable-virt-planes-v2-1-87971236fe86@oss.qualcomm.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 **Code change:** ```c -bool dpu_use_virtual_planes; +bool dpu_use_virtual_planes =3D true; ``` **Observations:** 1. **Correct and minimal change.** The diff does exactly what the commit me= ssage describes. The explicit `=3D true` initializer is the right approach = (rather than, say, inverting the logic and the parameter name). 2. **Boot parameter documentation.** The commit message correctly documents= the fallback: `msm.dpu_use_virtual_planes=3Dfalse`. This matches the `modu= le_param(dpu_use_virtual_planes, bool, 0)` declaration at `dpu_kms.c:56`. 3. **Minor nit =E2=80=94 no MODULE_PARM_DESC.** The parameter has no `MODUL= E_PARM_DESC()`, so `modinfo` won't show any description for it. This pre-da= tes this patch, but given that the parameter is now the escape hatch for a = default-on feature, it might be worth adding one in a follow-up: ```c MODULE_PARM_DESC(dpu_use_virtual_planes, "Use virtual planes for improved pipeline utilization (= default: true)"); ``` 4. **Parameter permissions are 0.** The `module_param(..., 0)` means the pa= rameter is not exposed in sysfs at all =E2=80=94 it's boot-time only. This = is fine for this kind of fundamental mode switch, since changing it at runt= ime would likely be unsafe without a full modeset teardown/rebuild. 5. **Usage sites are clean.** The variable is checked in two places: - `dpu_kms.c:852` =E2=80=94 choosing between `dpu_plane_init_virtual()` = and `dpu_plane_init()` during hardware init. - `dpu_crtc.c:1537` =E2=80=94 conditionally calling `dpu_crtc_reassign_p= lanes()` during atomic check. =20 Both are one-shot init or atomic-check paths, so the default flip doesn'= t introduce any new races or lifecycle concerns. **Verdict:** The patch is correct and ready. No blocking issues. The only s= uggestion is the optional `MODULE_PARM_DESC` addition for better self-docum= entation, but that can be a separate cleanup. --- Generated by Claude Code Patch Reviewer