Claude review: drm/msm/dpu: Defer SSPP allocation until CRTC check

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**Review: Significant refactor, generally correct but has notable observations.**

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 (mirroring `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 planes too

Observations:

1. **Blank line removal at line 803:** The patch removes a blank line between `crtc_state->planes_changed = true;` and `return 0;` in the virtual path. 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_state()`** which can add the plane to the atomic state even in non-virtual mode. Previously the non-virtual path used `drm_atomic_get_new_plane_state()` (which is read-only). This is a behavioral change — `drm_atomic_get_plane_state()` can trigger plane state duplication. This may be intentional to ensure the plane state is writable since `dpu_plane_split` now runs from the CRTC check phase, but it's worth confirming there are no side effects.

3. **The clearing of `pstate->pipe[i].sspp = NULL` for invisible planes was removed.** The comment said "resources are freed by `dpu_crtc_assign_plane_resources()`" — 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 SSPP pointers).

4. **`crtc_state` variable is declared outside the loop in `dpu_assign_plane_resources()`** but assigned inside. If `plane_state->crtc` is NULL, `crtc_state` retains its previous value (NULL or from a prior iteration). This should be fine since the continue guards check `!plane_state->fb || !plane_state->visible`, but it would be cleaner to initialize inside the loop or NULL it explicitly.

**No Reviewed-by tag.** This is the most complex patch and warrants careful review.

---
Generated by Claude Code Patch Reviewer