[PATCH] drm/vmwgfx: Check whether CRTC is active when updating planes

[PATCH] drm/vmwgfx: Check whether CRTC is active when updating planes

[Ian Forbes]

The IGT test kms_universal_plane triggers the error condition in this
branch because the screen target is not defined.

Also access the STDU via the plane since it's guaranteed to be non-null.

Fixes: e05162c017e2 ("drm: Store new plane state in a variable for atomic_update and disable")
Signed-off-by: Ian Forbes <ian.forbes@broadcom.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
index 5ad7e942ceb8..9deeee9b92da 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
@@ -44,6 +44,8 @@
 	container_of(x, struct vmw_screen_target_display_unit, base.encoder)
 #define vmw_connector_to_stdu(x) \
 	container_of(x, struct vmw_screen_target_display_unit, base.connector)
+#define vmw_primary_to_stdu(x) \
+	container_of(x, struct vmw_screen_target_display_unit, base.primary)
 
 /*
  * Some renderers such as llvmpipe will align the width and height of their
@@ -1418,7 +1420,8 @@ vmw_stdu_primary_plane_atomic_update(struct drm_plane *plane,
 	int ret;
 
 	/* If case of device error, maintain consistent atomic state */
-	if (crtc && new_state->fb) {
+	if (crtc && new_state->fb &&
+	    drm_atomic_get_new_crtc_state(state, crtc)->active) {
 		struct vmw_framebuffer *vfb =
 			vmw_framebuffer_to_vfb(new_state->fb);
 		stdu = vmw_crtc_to_stdu(crtc);
@@ -1442,9 +1445,8 @@ vmw_stdu_primary_plane_atomic_update(struct drm_plane *plane,
 		if (ret)
 			DRM_ERROR("Failed to update STDU.\n");
 	} else {
-		crtc = old_state->crtc;
-		stdu = vmw_crtc_to_stdu(crtc);
-		dev_priv = vmw_priv(crtc->dev);
+		stdu = vmw_primary_to_stdu(plane);
+		dev_priv = vmw_priv(plane->dev);
 
 		/* Blank STDU when fb and crtc are NULL */
 		if (!stdu->defined)
-- 
2.54.0

Claude review: drm/vmwgfx: Check whether CRTC is active when updating planes

[Claude Code Review Bot]

Overall Series Review

Subject: drm/vmwgfx: Check whether CRTC is active when updating planes
Author: Ian Forbes <ian.forbes@broadcom.com>
Patches: 1
Reviewed: 2026-05-25T18:10:44.733742

---

This is a single-patch fix for the vmwgfx STDU (Screen Target Display Unit) primary plane atomic update path. The patch addresses two issues:

1. **Missing CRTC active check**: The `atomic_update` callback was executing the full surface bind/update path even when the CRTC was not active, which the IGT `kms_universal_plane` test exposed — the screen target is not defined in that state, triggering an error.

2. **NULL dereference in else branch**: The old code accessed `old_state->crtc` in the else branch without a NULL check, which is unsafe since the else branch is entered when `new_state->crtc` is NULL, and `old_state->crtc` is not guaranteed to be non-NULL either.

The fix is correct, minimal, and well-targeted. Both issues are real bugs.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/vmwgfx: Check whether CRTC is active when updating planes

[Claude Code Review Bot]

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., DPMS off), the plane update should go through the blank/unbind path rather than 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 atomic framework guarantees the CRTC state is part of the commit (ensured by `drm_atomic_helper_check_modeset()` which runs before `atomic_update`). This is consistent with how the same function is used elsewhere in this file (line 433, 900).

**The else-branch rework (lines 1441–1442)**:
```c
stdu = vmw_primary_to_stdu(plane);
dev_priv = vmw_priv(plane->dev);
```
This replaces the old unsafe code:
```c
crtc = old_state->crtc;
stdu = vmw_crtc_to_stdu(crtc);
dev_priv = 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 guarantee `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()` is safe because `base.primary` is an embedded `struct drm_plane` (not a pointer) in `vmw_display_unit`, as confirmed in `vmwgfx_kms.h`. The plane pointer itself is always valid in this callback.

**The new macro (line 47–48)**:
```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 also handles the case where `crtc` is non-NULL, `fb` is non-NULL, but the CRTC is inactive. In that case, the code falls through to the blank path, which calls `vmw_stdu_bind_st(dev_priv, stdu, NULL)` and `vmw_stdu_update_st()` — but only if `stdu->defined` is true. This is correct behavior: when a CRTC is being deactivated, the STDU should be blanked. The `!stdu->defined` early return guards against double-unbinding.

**Fixes tag**: References `e05162c017e2 ("drm: Store new plane state in a variable for atomic_update and disable")`, which is the commit that restructured 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