From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/vmwgfx: Check whether CRTC is active when updating planes
Date: Mon, 25 May 2026 18:10:44 +1000
Message-ID: <review-patch1-20260522195303.797091-1-ian.forbes@broadcom.com>
In-Reply-To: <20260522195303.797091-1-ian.forbes@broadcom.com>
References: <20260522195303.797091-1-ian.forbes@broadcom.com> <20260522195303.797091-1-ian.forbes@broadcom.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

**Correctness: Good**

**The `active` check (line 1417)**:
```c
if (crtc && new_state->fb &&
    drm_atomic_get_new_crtc_state(state, crtc)->active) {
```
This is the right approach. When a CRTC exists but is not active (e.g., DPM=
S off), the plane update should go through the blank/unbind path rather tha=
n trying to bind and update the surface. The `drm_atomic_get_new_crtc_state=
()` call is safe here because when `new_state->crtc` is non-NULL, the DRM a=
tomic framework guarantees the CRTC state is part of the commit (ensured by=
 `drm_atomic_helper_check_modeset()` which runs before `atomic_update`). Th=
is is consistent with how the same function is used elsewhere in this file =
(line 433, 900).

**The else-branch rework (lines 1441=E2=80=931442)**:
```c
stdu =3D vmw_primary_to_stdu(plane);
dev_priv =3D vmw_priv(plane->dev);
```
This replaces the old unsafe code:
```c
crtc =3D old_state->crtc;
stdu =3D vmw_crtc_to_stdu(crtc);
dev_priv =3D vmw_priv(crtc->dev);
```
The old code dereferenced `old_state->crtc` without a NULL check. Since the=
 else branch is entered when `new_state->crtc` is NULL, there's no guarante=
e `old_state->crtc` is also non-NULL (e.g., if the plane was never attached=
 to a CRTC). Using `container_of(plane, ...)` via `vmw_primary_to_stdu()` i=
s safe because `base.primary` is an embedded `struct drm_plane` (not a poin=
ter) in `vmw_display_unit`, as confirmed in `vmwgfx_kms.h`. The plane point=
er itself is always valid in this callback.

**The new macro (line 47=E2=80=9348)**:
```c
#define vmw_primary_to_stdu(x) \
	container_of(x, struct vmw_screen_target_display_unit, base.primary)
```
Follows the existing pattern of `vmw_crtc_to_stdu`, `vmw_encoder_to_stdu`, =
and `vmw_connector_to_stdu`. Consistent style.

**One minor concern**: With the added `active` check, the else branch now a=
lso handles the case where `crtc` is non-NULL, `fb` is non-NULL, but the CR=
TC is inactive. In that case, the code falls through to the blank path, whi=
ch calls `vmw_stdu_bind_st(dev_priv, stdu, NULL)` and `vmw_stdu_update_st()=
` =E2=80=94 but only if `stdu->defined` is true. This is correct behavior: =
when a CRTC is being deactivated, the STDU should be blanked. The `!stdu->d=
efined` early return guards against double-unbinding.

**Fixes tag**: References `e05162c017e2 ("drm: Store new plane state in a v=
ariable for atomic_update and disable")`, which is the commit that restruct=
ured how the plane states are accessed. This is the correct provenance for =
the bug.

**Verdict**: The patch is correct and ready to merge. No issues found.

---
Generated by Claude Code Patch Reviewer