[PATCH v4 0/4] Fix some issues from igt runs.

[PATCH v4 0/4] Fix some issues from igt runs.

[Maaz Mombasawala]

I ran igt-gpu-tools with vmwgfx and fixed some issues I found.

Changes in v4:
- For ttm ref change, changed to incrementing the refcount on
the ttm base object of the dumb surface instead of holding a
ttm ref object so that it is not exposed to tfile.
- Added Fixes messages.
- Removed BO cleanup patch.

Changes in v3:
- Minor changes so series can apply cleanly on drm-misc-next.

Changes in v2:
- Changes based on Zack's comments.
- For vrefresh changes, removed stdu specific checks so that test
kms_invalid_mode@overflow-vrefresh passes with sou and ldu displays.
For ttm ref changes, fixed a ref leak in ttm_prime_fd_to_handle()
which fixes a memleak in test vmw_prime@tri-map-dmabuf and removed
release callback for dumb buffer surface since the gem buffer will
handle release of the surface.
Added commit to remove duplicate functions in vmwgfx_bo.h.


Maaz Mombasawala (4):
  drm/vmwgfx: Add some checks to vmw_cursor_plane_atomic_update
  drm/vmwgfx: Check vrefresh in drm_mode_setcrtc.
  drm/vmwgfx: Reserve ttm object before resv usage
  drm/vmwgfx: Change ttm refs for dumb buffers.

 drivers/gpu/drm/vmwgfx/ttm_object.c          |  7 +++--
 drivers/gpu/drm/vmwgfx/vmwgfx_bo.c           |  4 ++-
 drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c | 16 +++++++----
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.h          |  1 +
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c          | 28 ++++++++++++++++++++
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.h          |  9 +++++++
 drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c         | 12 +++------
 drivers/gpu/drm/vmwgfx/vmwgfx_surface.c      | 14 +++++++++-
 drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c         | 12 +++++++++
 9 files changed, 85 insertions(+), 18 deletions(-)

-- 
2.54.0

[PATCH v4 1/4] drm/vmwgfx: Add some checks to vmw_cursor_plane_atomic_update

[Maaz Mombasawala]

In cases where old_state may not be present we hit a null dereference.
Add a check to fix this and also do an early return if there is no
new_state or crtc since there is nothing to do in that case.
This fixes igt test kms_cursor_legacy@torture-bo.

Fixes: 965544150d1c ("drm/vmwgfx: Refactor cursor handling")
Cc: Zack Rusin <zack.rusin@broadcom.com>
Cc: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Maaz Mombasawala <maaz.mombasawala@broadcom.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c b/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c
index b010fc7ca68e..583115c94800 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c
@@ -742,11 +742,17 @@ vmw_cursor_plane_atomic_update(struct drm_plane *plane,
 			       struct drm_atomic_commit *state)
 {
 	struct vmw_bo *bo;
-	struct drm_plane_state *new_state =
-		drm_atomic_get_new_plane_state(state, plane);
-	struct drm_plane_state *old_state =
-		drm_atomic_get_old_plane_state(state, plane);
-	struct drm_crtc *crtc = new_state->crtc ?: old_state->crtc;
+	struct drm_plane_state *new_state;
+	struct drm_plane_state *old_state;
+
+	new_state = drm_atomic_get_new_plane_state(state, plane);
+	if (!new_state)
+		return;
+	old_state = drm_atomic_get_old_plane_state(state, plane);
+	struct drm_crtc *crtc =  new_state->crtc ? new_state->crtc :
+		(old_state ? old_state->crtc : NULL);
+	if (!crtc)
+		return;
 	struct vmw_private *dev_priv = vmw_priv(plane->dev);
 	struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
 	struct vmw_plane_state *vps = vmw_plane_state_to_vps(new_state);
-- 
2.54.0

[PATCH v4 2/4] drm/vmwgfx: Check vrefresh in drm_mode_setcrtc.

[Maaz Mombasawala]

Add the mode_valid() callback in drm_mode_config_funcs to check for valid
mode during drm_mode_setcrtc.
Both the mode_valid() callbacks also check if the vrefresh value is
correct.
This fixes igt test kms_invalid_mode@overflow-vrefresh.

Signed-off-by: Maaz Mombasawala <maaz.mombasawala@broadcom.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c  | 28 ++++++++++++++++++++++++++++
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.h  |  9 +++++++++
 drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 12 +++---------
 3 files changed, 40 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 1b407b61f683..4db29b645a38 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -1063,8 +1063,36 @@ vmw_kms_atomic_check_modeset(struct drm_device *dev,
 	return ret;
 }
 
+static enum drm_mode_status
+vmw_kms_config_mode_valid(struct drm_device *dev,
+			  const struct drm_display_mode *mode)
+{
+	enum drm_mode_status ret;
+	struct vmw_private *dev_priv = vmw_priv(dev);
+	u64 assumed_cpp = dev_priv->assume_16bpp ? 2 : 4;
+	/* Align width and height to account for GPU tile over-alignment */
+	u64 required_mem = ALIGN(mode->hdisplay, GPU_TILE_SIZE) *
+			   ALIGN(mode->vdisplay, GPU_TILE_SIZE) *
+			   assumed_cpp;
+	required_mem = ALIGN(required_mem, PAGE_SIZE);
+
+	ret = drm_mode_validate_size(mode, dev_priv->texture_max_width,
+				     dev_priv->texture_max_height);
+	if (ret != MODE_OK)
+		return ret;
+
+	if (required_mem > dev_priv->max_primary_mem)
+		return MODE_MEM;
+
+	if (!drm_mode_vrefresh(mode))
+		return MODE_BAD;
+
+	return MODE_OK;
+}
+
 static const struct drm_mode_config_funcs vmw_kms_funcs = {
 	.fb_create = vmw_kms_fb_create,
+	.mode_valid = vmw_kms_config_mode_valid,
 	.atomic_check = vmw_kms_atomic_check_modeset,
 	.atomic_commit = drm_atomic_helper_commit,
 };
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
index 2224d7d91d1b..1b717caecc78 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
@@ -16,6 +16,15 @@
 #include <drm/drm_framebuffer.h>
 #include <drm/drm_probe_helper.h>
 
+/*
+ * Some renderers such as llvmpipe will align the width and height of their
+ * buffers to match their tile size. We need to keep this in mind when exposing
+ * modes to userspace so that this possible over-allocation will not exceed
+ * graphics memory. 64x64 pixels seems to be a reasonable upper bound for the
+ * tile size of current renderers.
+ */
+#define GPU_TILE_SIZE 64
+
 /**
  * struct vmw_du_update_plane - Closure structure for vmw_du_helper_plane_update
  * @plane: Plane which is being updated.
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
index 4139837f4caf..04c5c20588d8 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
@@ -45,15 +45,6 @@
 #define vmw_connector_to_stdu(x) \
 	container_of(x, struct vmw_screen_target_display_unit, base.connector)
 
-/*
- * Some renderers such as llvmpipe will align the width and height of their
- * buffers to match their tile size. We need to keep this in mind when exposing
- * modes to userspace so that this possible over-allocation will not exceed
- * graphics memory. 64x64 pixels seems to be a reasonable upper bound for the
- * tile size of current renderers.
- */
-#define GPU_TILE_SIZE 64
-
 enum stdu_content_type {
 	SAME_AS_DISPLAY = 0,
 	SEPARATE_SURFACE,
@@ -870,6 +861,9 @@ vmw_stdu_connector_mode_valid(struct drm_connector *connector,
 	if (required_mem > dev_priv->max_mob_size)
 		return MODE_MEM;
 
+	if (!drm_mode_vrefresh(mode))
+		return MODE_BAD;
+
 	return MODE_OK;
 }
 
-- 
2.54.0

[PATCH v4 3/4] drm/vmwgfx: Reserve ttm object before resv usage

[Maaz Mombasawala]

We hit dma_resv_assert_held warnings in several places, reserve the ttm
base object when it's being used to avoid them.

Signed-off-by: Maaz Mombasawala <maaz.mombasawala@broadcom.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_bo.c   |  2 ++
 drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c | 12 ++++++++++++
 2 files changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
index 9c7a73c0b0dc..6817e0b8ce99 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
@@ -55,7 +55,9 @@ static void vmw_bo_free(struct ttm_buffer_object *bo)
 			/* Reserve and switch the backing mob. */
 			mutex_lock(&res->dev_priv->cmdbuf_mutex);
 			(void)vmw_resource_reserve(res, false, true);
+			ttm_bo_reserve(bo, false, false, NULL);
 			vmw_resource_mob_detach(res);
+			ttm_bo_unreserve(bo);
 			if (res->dirty)
 				res->func->dirty_free(res);
 			if (res->coherent)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c
index 7b8163b5e501..da6ccd5e318e 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c
@@ -131,6 +131,7 @@ crc_generate_worker(struct work_struct *work)
 	spin_unlock_irq(&du->vkms.crc_state_lock);
 
 	if (surf) {
+		int ret;
 		if (vmw_surface_sync(vmw, surf)) {
 			drm_warn(
 				crtc->dev,
@@ -138,7 +139,18 @@ crc_generate_worker(struct work_struct *work)
 			return;
 		}
 
+		ret = ttm_bo_reserve(&surf->res.guest_memory_bo->tbo, false, false, NULL);
+		if (ret != 0) {
+			drm_warn(&vmw->drm, "%s: failed reserve\n", __func__);
+			goto done;
+		}
+
 		compute_crc(crtc, surf, &crc32);
+
+		ttm_bo_unreserve(&surf->res.guest_memory_bo->tbo);
+
+done:
+
 		vmw_surface_unreference(&surf);
 	}
 
-- 
2.54.0

[PATCH v4 4/4] drm/vmwgfx: Change ttm refs for dumb buffers.

[Maaz Mombasawala]

Preserve a ref to surface during dumb buffer creation. This keeps the dumb
buffer valid for framebuffer usage and fixes all igt tests that use dumb
buffers.
Also fix ttm_prime_fd_to_handle(), which in the error case was leaking a
dma_buf reference. During vmw_prime_fd_to_handle() this function
is expected to fail for dumb buffers since the fd is for a gem object,
the dma_buf would in turn hold a reference to the dumb buffer gem object
and cause a memory leak.

Fixes: f42c09e614f1 ("drm/vmwgfx: Fix dumb buffer leak")
Cc: Ian Forbes <ian.forbes@broadcom.com>
Cc: Zack Rusin <zack.rusin@broadcom.com>
Cc: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Maaz Mombasawala <maaz.mombasawala@broadcom.com>
---
 drivers/gpu/drm/vmwgfx/ttm_object.c     |  7 +++++--
 drivers/gpu/drm/vmwgfx/vmwgfx_bo.c      |  2 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.h     |  1 +
 drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 14 +++++++++++++-
 4 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/ttm_object.c b/drivers/gpu/drm/vmwgfx/ttm_object.c
index 2421b0dd057c..f9042bafdc93 100644
--- a/drivers/gpu/drm/vmwgfx/ttm_object.c
+++ b/drivers/gpu/drm/vmwgfx/ttm_object.c
@@ -547,14 +547,17 @@ int ttm_prime_fd_to_handle(struct ttm_object_file *tfile,
 	if (IS_ERR(dma_buf))
 		return PTR_ERR(dma_buf);
 
-	if (dma_buf->ops != &tdev->ops)
-		return -ENOSYS;
+	if (dma_buf->ops != &tdev->ops) {
+		ret = -ENOSYS;
+		goto out;
+	}
 
 	prime = (struct ttm_prime_object *) dma_buf->priv;
 	base = &prime->base;
 	*handle = base->handle;
 	ret = ttm_ref_object_add(tfile, base, NULL, false);
 
+out:
 	dma_buf_put(dma_buf);
 
 	return ret;
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
index 6817e0b8ce99..d1ce1a58e134 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
@@ -68,7 +68,7 @@ static void vmw_bo_free(struct ttm_buffer_object *bo)
 					       0);
 			mutex_unlock(&res->dev_priv->cmdbuf_mutex);
 		}
-		vmw_surface_unreference(&vbo->dumb_surface);
+		vmw_dumb_surface_unref(&vbo->dumb_surface);
 	}
 	WARN_ON(!RB_EMPTY_ROOT(&vbo->res_tree));
 	drm_gem_object_release(&vbo->tbo.base);
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
index 38bea8abab84..cda55ee009d2 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
@@ -1186,6 +1186,7 @@ u32 vmw_lookup_surface_handle_for_buffer(struct vmw_private *vmw,
 int vmw_dumb_create(struct drm_file *file_priv,
 		    struct drm_device *dev,
 		    struct drm_mode_create_dumb *args);
+void vmw_dumb_surface_unref(struct vmw_surface **dumb_surface);
 
 /*
  * Shader management - vmwgfx_shader.c
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
index b2d3927b5567..34e55ad07ca9 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
@@ -2328,11 +2328,23 @@ int vmw_dumb_create(struct drm_file *file_priv,
 	struct vmw_user_surface *usurf = container_of(vbo->dumb_surface,
 						struct vmw_user_surface, srf);
 	usurf->prime.base.refcount_release = NULL;
+
+	ttm_base_object_lookup_for_ref(dev_priv->tdev, arg.rep.handle);
+
 err:
 	if (res)
 		vmw_resource_unreference(&res);
-
 	ttm_ref_object_base_unref(tfile, arg.rep.handle);
 
 	return ret;
 }
+
+void vmw_dumb_surface_unref(struct vmw_surface **dumb_surface)
+{
+	struct vmw_user_surface *usurf = container_of(*dumb_surface,
+						struct vmw_user_surface, srf);
+	struct ttm_base_object *base = &usurf->prime.base;
+
+	ttm_base_object_unref(&base);
+	vmw_surface_unreference(dumb_surface);
+}
-- 
2.54.0

Claude review: Fix some issues from igt runs.

[Claude Code Review Bot]

Overall Series Review

Subject: Fix some issues from igt runs.
Author: Maaz Mombasawala <maaz.mombasawala@broadcom.com>
Patches: 5
Reviewed: 2026-05-25T19:32:32.822170

---

This is a 4-patch series from Maaz Mombasawala (Broadcom/VMware) fixing issues discovered while running IGT GPU test suite against vmwgfx. The patches address: a null-pointer dereference in cursor plane updates, missing vrefresh validation, missing TTM reservation assertions, and dumb buffer refcounting fixes.

The series is reasonable in scope and each patch addresses a real bug. However, there are notable code quality issues: **patch 1** has a C declaration-after-statement style violation, **patch 3** adds a TTM reservation in `vmw_bo_free` that may be called from a context where the reservation could deadlock, and **patch 4** has a critical issue with the return value of `ttm_base_object_lookup_for_ref()` being discarded (silently ignoring errors on the success path). The patches are generally on the right track but need refinement in several areas.

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm/vmwgfx: Add some checks to vmw_cursor_plane_atomic_update

[Claude Code Review Bot]

Patch Review

**Purpose:** Fix a null dereference when `old_state` is NULL in `vmw_cursor_plane_atomic_update`, found by IGT test `kms_cursor_legacy@torture-bo`.

**Issues:**

1. **Declaration-after-statement / mixed declarations and code (style):** The patch introduces variable declarations after statements, which violates the kernel coding style (and strict C89 rules still enforced by some kernel configs):

```c
new_state = drm_atomic_get_new_plane_state(state, plane);
if (!new_state)
    return;
old_state = drm_atomic_get_old_plane_state(state, plane);
struct drm_crtc *crtc =  new_state->crtc ? new_state->crtc :
    (old_state ? old_state->crtc : NULL);
if (!crtc)
    return;
struct vmw_private *dev_priv = vmw_priv(plane->dev);
```

The declarations of `crtc` and `dev_priv` appear after executable statements (the `if` guards). While C99/C11 allows this and the kernel has moved to C11, placing declarations like `struct drm_crtc *crtc` and `struct vmw_private *dev_priv` after `if` statements with `return` is unusual in kernel code. It would be cleaner to declare `crtc` at the top with `new_state`/`old_state` and assign it after the null checks.

2. **Double space:** Minor nit — there's a double space before `new_state->crtc`:
```c
struct drm_crtc *crtc =  new_state->crtc ? new_state->crtc :
```

3. **Can `new_state` actually be NULL here?** In the atomic commit path, `drm_atomic_get_new_plane_state` should always return non-NULL for a plane that is in the commit. If the plane is not in the atomic state, it shouldn't be called at all. The commit message says this fixes `kms_cursor_legacy@torture-bo`, but it would be helpful to understand the exact call path that leads to `new_state` being NULL — this might indicate the real bug is elsewhere (the plane shouldn't be in an update callback if it's not part of the state).

4. **Missing Fixes tag specificity:** The Fixes tag references the refactor commit but no explanation is given as to why the prior code (which used the same `drm_atomic_get_old/new_plane_state` calls) didn't trigger this. A sentence about the calling context would help reviewers.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/vmwgfx: Check vrefresh in drm_mode_setcrtc.

[Claude Code Review Bot]

Patch Review

**Purpose:** Add `mode_valid` callback to `drm_mode_config_funcs` to reject modes with zero vrefresh during `drm_mode_setcrtc`, and add vrefresh checks to STDU connector mode validation. Fixes IGT test `kms_invalid_mode@overflow-vrefresh`.

**Issues:**

1. **Config `mode_valid` checks `max_primary_mem` but connector checks different limits:** The new `vmw_kms_config_mode_valid()` validates against `dev_priv->max_primary_mem`, while the STDU connector's `vmw_stdu_connector_mode_valid()` also checks `max_mob_pages` and `max_mob_size` plus STDU-specific width/height limits. This is correct — the config-level check is necessarily more permissive since it's display-unit agnostic — but it's worth noting that the config-level check could pass modes that the connector level rejects.

2. **`GPU_TILE_SIZE` moved to header — good refactoring:** Moving the `GPU_TILE_SIZE` define from `vmwgfx_stdu.c` to `vmwgfx_kms.h` is the right thing since it's now used in both files.

3. **Missing Fixes tag:** Unlike patches 1 and 4, this patch has no `Fixes:` tag. The cover letter mentions adding Fixes messages, but this patch doesn't have one. If this is a regression fix, it should have one.

4. **Overall approach looks correct:** Using `drm_mode_config_funcs.mode_valid` is the correct hook for validating modes during `drm_mode_setcrtc` for all display types (SOU, LDU, STDU). The vrefresh check with `drm_mode_vrefresh()` returning 0 is a sensible rejection criterion.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/vmwgfx: Reserve ttm object before resv usage

[Claude Code Review Bot]

Patch Review

**Purpose:** Fix `dma_resv_assert_held` warnings by reserving the TTM buffer object before operations that require the reservation to be held.

**Issues:**

1. **`ttm_bo_reserve` in `vmw_bo_free` — potential deadlock concern:** In `vmw_bo_free`, the buffer object is being freed (refcount is 0, per the `WARN_ON` at line 45). Calling `ttm_bo_reserve(bo, false, false, NULL)` on a BO whose refcount is already 0 is unusual. The reservation is typically held by something with a live reference. Since this is a teardown path and no other thread should hold a reference, the reservation should succeed immediately, but this pattern is fragile. If anything else tries to acquire the reservation concurrently (which shouldn't happen given refcount==0), it could deadlock with `interruptible=false, no_wait=false`.

```c
ttm_bo_reserve(bo, false, false, NULL);
vmw_resource_mob_detach(res);
ttm_bo_unreserve(bo);
```

Consider whether `dma_resv_lock` directly (without going through `ttm_bo_reserve`) would be more appropriate here, or whether the callers already guarantee the reservation is held and the real fix should be elsewhere.

2. **Missing Fixes tag:** No `Fixes:` tag on this patch, despite addressing concrete warnings.

3. **VKMS CRC worker — `goto done` skips `compute_crc` but still unreferences:** The error handling in `crc_generate_worker` looks correct — on reserve failure, it jumps to `done:` which calls `vmw_surface_unreference`, which is the right cleanup. However, the blank line after `done:` is unnecessary:

```c
done:

	vmw_surface_unreference(&surf);
```

4. **VKMS reserve — is `guest_memory_bo` guaranteed non-NULL?** The code does:
```c
ret = ttm_bo_reserve(&surf->res.guest_memory_bo->tbo, false, false, NULL);
```
If `surf->res.guest_memory_bo` could be NULL, this would oops. Looking at the flow, `vmw_surface_sync` succeeded so the surface should have a valid backing BO, but a NULL check or assertion might be prudent.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/vmwgfx: Change ttm refs for dumb buffers.

[Claude Code Review Bot]

Patch Review

**Purpose:** Fix dumb buffer lifetime by taking an extra ttm_base_object reference during creation and releasing it properly on cleanup. Also fixes a dma_buf reference leak in `ttm_prime_fd_to_handle`.

**Issues:**

1. **Critical: Return value of `ttm_base_object_lookup_for_ref` is discarded on success path:**

```c
ttm_base_object_lookup_for_ref(dev_priv->tdev, arg.rep.handle);

err:
```

`ttm_base_object_lookup_for_ref()` returns a `struct ttm_base_object *` which is the refcount-incremented pointer, and this return value is completely discarded. The intent is clearly to increment the base object's refcount, but ignoring the return value means:
- If the lookup fails (returns NULL), the code silently proceeds without error handling.
- The code is relying on the side-effect of `kref_get_unless_zero` inside the function, but by convention the caller is expected to use the returned pointer and eventually call `ttm_base_object_unref` — ignoring the return value makes the intent unclear.

This should at minimum check the return value and handle the error case. Something like:
```c
if (!ttm_base_object_lookup_for_ref(dev_priv->tdev, arg.rep.handle)) {
    ret = -EINVAL;
    goto err;
}
```

2. **`ttm_prime_fd_to_handle` dma_buf leak fix — correct and straightforward:** The fix to add `dma_buf_put()` on the non-matching ops path is clearly correct. The old code returned `-ENOSYS` without calling `dma_buf_put(dma_buf)`, leaking the reference acquired by `dma_buf_get(fd)`.

```c
if (dma_buf->ops != &tdev->ops) {
    ret = -ENOSYS;
    goto out;
}
```

This is a clean and correct fix.

3. **`vmw_dumb_surface_unref` — double-free risk?** The function does:
```c
void vmw_dumb_surface_unref(struct vmw_surface **dumb_surface)
{
    struct ttm_base_object *base = &usurf->prime.base;
    ttm_base_object_unref(&base);
    vmw_surface_unreference(dumb_surface);
}
```

This relies on both the base object ref and the surface ref being in sync. If `refcount_release` is NULL (set earlier in `vmw_dumb_create`), then `ttm_base_object_unref` dropping to zero won't trigger any release callback. The `vmw_surface_unreference` then handles the surface cleanup. The ordering matters — `ttm_base_object_unref` must go first since `vmw_surface_unreference` may free the containing structure. This looks correct as written, since `vmw_surface_unreference` will only free when its own refcount drops to zero.

4. **Commit message says "fixes all igt tests that use dumb buffers" — that's a broad claim.** It would be better to list specific test names that were verified.

5. **The error-path `ttm_ref_object_base_unref` call ordering:** After the patch, on the success path the code does:
```c
ttm_base_object_lookup_for_ref(...)  // take base ref
err:
    vmw_resource_unreference(&res);   // drop resource ref
    ttm_ref_object_base_unref(tfile, arg.rep.handle);  // drop tfile ref
```

The `ttm_ref_object_base_unref` still runs on the success path. This means after `vmw_dumb_create` returns successfully, the only thing keeping the surface alive is the extra base object reference taken by `ttm_base_object_lookup_for_ref`. This is the design intent but should be documented with a comment.

---
Generated by Claude Code Patch Reviewer