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

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

[Claude Code Review Bot]

Patch Review

**dma_buf leak fix is correct and important:**

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

The original code returns `-ENOSYS` without calling `dma_buf_put(dma_buf)`, leaking the dma_buf reference acquired by `dma_buf_get(fd)`. The fix is straightforward and correct.

**Dumb buffer TTM ref preservation:**

```c
-	ttm_ref_object_base_unref(tfile, arg.rep.handle);
+	if (ret)
+		ttm_ref_object_base_unref(tfile, arg.rep.handle);
```

Previously, the TTM base object reference was unconditionally dropped after dumb buffer creation. This patch preserves it on the success path. The commit message says this keeps the dumb buffer valid for framebuffer usage. This makes sense if the TTM object reference was the only thing preventing the surface from being destroyed while still in use. However, this changes the cleanup responsibility — whoever destroys the dumb buffer must now handle this reference. The `refcount_release = NULL` set just above (`usurf->prime.base.refcount_release = NULL`) disables the normal TTM release callback, so the GEM object destruction path must handle this. This interaction should be documented or verified to avoid future leaks.

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

Patch Review

**The dma_buf leak fix in ttm_prime_fd_to_handle is clearly correct.** The original code returns `-ENOSYS` without calling `dma_buf_put(dma_buf)`, leaking the reference obtained from `dma_buf_get(fd)`:

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

This is a straightforward and correct fix.

**The vmw_dumb_create change needs careful consideration.** The patch changes the error path to conditionally unref:

```c
-	ttm_ref_object_base_unref(tfile, arg.rep.handle);
+	if (ret)
+		ttm_ref_object_base_unref(tfile, arg.rep.handle);
```

Combined with `usurf->prime.base.refcount_release = NULL`, this preserves the TTM reference for dumb buffers on the success path. The idea is that the GEM object holds the surface alive, and the TTM refcount_release callback is nulled out so that when the TTM ref is eventually dropped, it doesn't try to destroy the surface (since the GEM path handles that). This is a somewhat subtle lifetime management change — the commit message should explain the ownership model more clearly.

**Setting `refcount_release = NULL` is outside the error check.** The line `usurf->prime.base.refcount_release = NULL;` executes before the `err:` label, meaning it only runs on the success path. This is correct, but the code flow would be clearer if there were a comment explaining why.

---
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

[PATCH v5 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 v5:
- Get display unit from cursor plane, so there is no need for
old_crtc.
- Cast ALIGN to u64.
- Check return value of ttm_bo_reserve().

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           |  8 +++++-
 drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c | 16 +++++++----
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.h          |  1 +
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c          | 30 ++++++++++++++++++++
 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         |  8 ++++++
 9 files changed, 86 insertions(+), 19 deletions(-)

-- 
2.54.0

[PATCH v5 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.
Get display unit from cursor plane instead to fix this and also do an
early return if there is no new_state 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, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c b/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c
index b010fc7ca68e..35e3fd2bbdad 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c
@@ -22,6 +22,9 @@
 #define VMW_CURSOR_SNOOP_WIDTH 64
 #define VMW_CURSOR_SNOOP_HEIGHT 64
 
+#define vmw_cursor_to_du(cursor_plane) \
+	container_of(cursor_plane, struct vmw_display_unit, cursor.base)
+
 struct vmw_svga_fifo_cmd_define_cursor {
 	u32 cmd;
 	SVGAFifoCmdDefineAlphaCursor cursor;
@@ -742,13 +745,14 @@ 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;
+
+	new_state = drm_atomic_get_new_plane_state(state, plane);
+	if (!new_state)
+		return;
+
 	struct vmw_private *dev_priv = vmw_priv(plane->dev);
-	struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
+	struct vmw_display_unit *du = vmw_cursor_to_du(plane);
 	struct vmw_plane_state *vps = vmw_plane_state_to_vps(new_state);
 	s32 hotspot_x, hotspot_y, cursor_x, cursor_y;
 
-- 
2.54.0

[PATCH v5 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  | 30 ++++++++++++++++++++++++++++
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.h  |  9 +++++++++
 drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 12 +++--------
 3 files changed, 42 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index a170e38c5223..11edb422d2ba 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -1063,8 +1063,38 @@ 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 = 0;
+
+	ret = drm_mode_validate_size(mode, dev_priv->texture_max_width,
+				     dev_priv->texture_max_height);
+	if (ret != MODE_OK)
+		return ret;
+
+	required_mem = (u64)ALIGN(mode->hdisplay, GPU_TILE_SIZE) *
+		       ALIGN(mode->vdisplay, GPU_TILE_SIZE) *
+		       assumed_cpp;
+	required_mem = ALIGN(required_mem, PAGE_SIZE);
+
+	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 v5 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   | 6 ++++++
 drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c | 8 ++++++++
 2 files changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
index 1264a9ac4241..a69a6764ead2 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
@@ -52,10 +52,16 @@ static void vmw_bo_free(struct ttm_buffer_object *bo)
 		WARN_ON(vbo != res->guest_memory_bo);
 		WARN_ON(!res->guest_memory_bo);
 		if (res->guest_memory_bo) {
+			int ret = 0;
 			/* Reserve and switch the backing mob. */
 			mutex_lock(&res->dev_priv->cmdbuf_mutex);
 			(void)vmw_resource_reserve(res, false, true);
+			ret = ttm_bo_reserve(bo, false, false, NULL);
+			if (ret != 0)
+				drm_warn(&res->dev_priv->drm,
+					 "%s: failed reserve\n", __func__);
 			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..d0c371df7e4c 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,14 @@ 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 v5 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 a69a6764ead2..3de255a619a8 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
@@ -72,7 +72,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 812f1224f83e..cd46d3995ade 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
@@ -1185,6 +1185,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 ab611943d774..5a9c953eb73c 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-06-04T13:30:06.941577

---

This is a 4-patch series from Maaz Mombasawala (Broadcom/VMware) fixing issues found by running igt-gpu-tools against the vmwgfx driver, now at v5. The series addresses a null deref in cursor updates, missing vrefresh validation, missing TTM reservation locks, and dumb buffer lifetime management.

**Patches 1 and 2** are clean and correct. **Patch 3** has a problematic error handling path in `vmw_bo_free` where a failed `ttm_bo_reserve` will still proceed to call `vmw_resource_mob_detach` (which asserts the lock is held) and `ttm_bo_unreserve` (on an unreserved BO). **Patch 4** has the most significant issue: the return value of `ttm_base_object_lookup_for_ref()` is not checked, which can lead to a refcount underflow in `vmw_dumb_surface_unref()` at cleanup time if the lookup fails. Additionally, Patch 3 is missing a Fixes tag even though it addresses specific dma_resv_assert_held warnings.

---

---
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

**Status: Looks good, minor style nit**

The fix correctly addresses a null dereference when `old_state` is not present. The root cause is that the old code did:
```c
struct drm_crtc *crtc = new_state->crtc ?: old_state->crtc;
struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
```
...which crashes if both are NULL or `old_state` itself is NULL.

The new approach of getting `du` via `container_of(plane, struct vmw_display_unit, cursor.base)` is correct — the cursor plane *is* `du->cursor.base`, so `container_of` is valid and avoids the dependency on any plane state. The early return on `!new_state` is also appropriate since there's nothing to update.

**Minor style nit**: The declarations of `dev_priv` and `du` appear after the `if (!new_state) return;` statement:
```c
new_state = drm_atomic_get_new_plane_state(state, plane);
if (!new_state)
    return;

struct vmw_private *dev_priv = vmw_priv(plane->dev);
struct vmw_display_unit *du = vmw_cursor_to_du(plane);
```
While this is valid C11 (which the kernel uses), it's a coding style preference issue. Some reviewers prefer all declarations before statements within a block. Not a correctness issue.

---

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

Patch Review

**Status: Looks good, but missing Fixes/Cc stable tags**

The patch adds `drm_mode_config_funcs.mode_valid` to validate modes set via the legacy `drm_mode_setcrtc` path, which the existing per-connector `mode_valid` doesn't cover. The vrefresh check (`if (!drm_mode_vrefresh(mode)) return MODE_BAD`) is also added to the STDU connector's `vmw_stdu_connector_mode_valid`.

Good decisions in this patch:
- Moving `GPU_TILE_SIZE` from `vmwgfx_stdu.c` to `vmwgfx_kms.h` for reuse.
- Casting the `ALIGN` result to `u64` to avoid 32-bit overflow in `required_mem` calculation:
  ```c
  required_mem = (u64)ALIGN(mode->hdisplay, GPU_TILE_SIZE) *
                 ALIGN(mode->vdisplay, GPU_TILE_SIZE) *
                 assumed_cpp;
  ```

**Missing tags**: This patch fixes `kms_invalid_mode@overflow-vrefresh` but has no `Fixes:` tag or `Cc: stable@` annotation. If this is a regression from a specific commit, a Fixes tag would help with backporting. Even without a clear single culprit, a Cc: stable would be appropriate if this matters for older kernels.

---

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

Patch Review

**Status: Has issues in error handling**

The intent is correct — `vmw_resource_mob_detach()` calls `dma_resv_assert_held()`, so the buffer object's reservation must be held.

**Issue in `vmw_bo_free` (vmwgfx_bo.c)**: If `ttm_bo_reserve()` fails, the code warns but continues to call both `vmw_resource_mob_detach(res)` and `ttm_bo_unreserve(bo)`:
```c
ret = ttm_bo_reserve(bo, false, false, NULL);
if (ret != 0)
    drm_warn(&res->dev_priv->drm,
             "%s: failed reserve\n", __func__);
vmw_resource_mob_detach(res);   /* asserts lock held — will fire */
ttm_bo_unreserve(bo);           /* unreserves without holding reservation */
```

If the reserve fails:
1. `vmw_resource_mob_detach` will hit the `dma_resv_assert_held` assertion — the very thing this patch is meant to fix.
2. `ttm_bo_unreserve` on a non-reserved BO is undefined behavior (likely corrupts the ww_mutex state).

In practice, `ttm_bo_reserve(bo, false, false, NULL)` (non-interruptible, blocking) should essentially never fail, but since v5 explicitly added this check per review feedback, the error path should be correct. A simple approach would be to gate the unreserve:
```c
ret = ttm_bo_reserve(bo, false, false, NULL);
if (ret != 0)
    drm_warn(...);
vmw_resource_mob_detach(res);
if (ret == 0)
    ttm_bo_unreserve(bo);
```
Or skip the detach on failure if that's safe for the destroy path.

**The `crc_generate_worker` change (vmwgfx_vkms.c)** looks correct — the `goto done` properly skips `compute_crc` and falls through to `vmw_surface_unreference(&surf)`.

**Missing tags**: This patch is also missing `Fixes:` and `Cc: stable` tags despite fixing lockdep/dma_resv assertion warnings.

---

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

Patch Review

**Status: Has a significant unchecked return value**

**Critical issue — unchecked return value (vmwgfx_surface.c:2332)**:
```c
ttm_base_object_lookup_for_ref(dev_priv->tdev, arg.rep.handle);
```
`ttm_base_object_lookup_for_ref()` returns `struct ttm_base_object *` (or NULL on failure), but the return value is completely discarded. This function is called to acquire an extra reference on the base object that will later be dropped by `vmw_dumb_surface_unref → ttm_base_object_unref`. If the lookup fails and no reference is acquired, the subsequent `ttm_base_object_unref` in `vmw_dumb_surface_unref` will decrement a refcount that was never incremented, causing a refcount underflow and potential use-after-free.

The fix should check the return value:
```c
if (!ttm_base_object_lookup_for_ref(dev_priv->tdev, arg.rep.handle)) {
    ret = -EINVAL;
    goto err;
}
```

**The dma_buf leak fix in `ttm_prime_fd_to_handle` (ttm_object.c)** is correct and straightforward — the early return was missing a `dma_buf_put()`:
```c
// Before: leaked dma_buf ref on !tdev->ops
if (dma_buf->ops != &tdev->ops)
    return -ENOSYS;

// After: properly releases via goto
if (dma_buf->ops != &tdev->ops) {
    ret = -ENOSYS;
    goto out;
}
```

**The `vmw_dumb_surface_unref` function**: Setting `refcount_release = NULL` is safe because `ttm_release_base()` (ttm_object.c:241) checks for NULL before calling it. The function correctly pairs `ttm_base_object_unref` (drops the base object ref from `lookup_for_ref`) with `vmw_surface_unreference` (drops the resource ref). The ordering is safe since the base object destruction (with NULL release callback) only does IDR cleanup and tfile unref, not memory deallocation.

---
Generated by Claude Code Patch Reviewer