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: Defer SSPP allocation until CRTC check Date: Fri, 13 Mar 2026 14:21:22 +1000 Message-ID: In-Reply-To: <20260312-msm-next-quad-pipe-split-v19-2-4ffa2b06c996@linaro.org> References: <20260312-msm-next-quad-pipe-split-v19-0-4ffa2b06c996@linaro.org> <20260312-msm-next-quad-pipe-split-v19-2-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: Significant refactor, generally correct but has notable observati= ons.** This patch unifies the virtual and non-virtual plane paths. Key changes: - Merges `dpu_plane_virtual_atomic_check()` into `dpu_plane_atomic_check()`= (the vtable entry is unified) - Creates `dpu_plane_assign_resources()` for the non-virtual case (mirrorin= g `dpu_plane_virtual_assign_resources()`) - Both are called from `dpu_assign_plane_resources()` in CRTC check - `dpu_crtc_assign_planes()` is factored out and called for non-virtual pla= nes too Observations: 1. **Blank line removal at line 803:** The patch removes a blank line betwe= en `crtc_state->planes_changed =3D true;` and `return 0;` in the virtual pa= th. This is a trivial cosmetic change mixed into a functional patch. Minor = nit. 2. **The `dpu_plane_atomic_check` now always calls `drm_atomic_get_plane_st= ate()`** which can add the plane to the atomic state even in non-virtual mo= de. Previously the non-virtual path used `drm_atomic_get_new_plane_state()`= (which is read-only). This is a behavioral change =E2=80=94 `drm_atomic_ge= t_plane_state()` can trigger plane state duplication. This may be intention= al to ensure the plane state is writable since `dpu_plane_split` now runs f= rom the CRTC check phase, but it's worth confirming there are no side effec= ts. 3. **The clearing of `pstate->pipe[i].sspp =3D NULL` for invisible planes w= as removed.** The comment said "resources are freed by `dpu_crtc_assign_pla= ne_resources()`" =E2=80=94 since the non-virtual path now also goes through= `dpu_crtc_assign_planes()`, this should still be handled. But verify that = the non-virtual path properly handles invisible planes (the `!plane_state->= visible` continue at line 1307 skips assignment, but doesn't clear stale SS= PP pointers). 4. **`crtc_state` variable is declared outside the loop in `dpu_assign_plan= e_resources()`** but assigned inside. If `plane_state->crtc` is NULL, `crtc= _state` retains its previous value (NULL or from a prior iteration). This s= hould be fine since the continue guards check `!plane_state->fb || !plane_s= tate->visible`, but it would be cleaner to initialize inside the loop or NU= LL it explicitly. **No Reviewed-by tag.** This is the most complex patch and warrants careful= review. --- Generated by Claude Code Patch Reviewer