[PATCH v3 0/5] Fix some issues from igt runs.

[PATCH v3 0/5] Fix some issues from igt runs.

[Maaz Mombasawala]

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

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 (5):
  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.
  drm/vmwgfx: Remove duplicates in vmwgfx_bo.h

 drivers/gpu/drm/vmwgfx/ttm_object.c          |  7 +++--
 drivers/gpu/drm/vmwgfx/vmwgfx_bo.c           |  6 ++--
 drivers/gpu/drm/vmwgfx/vmwgfx_bo.h           | 15 ---------
 drivers/gpu/drm/vmwgfx/vmwgfx_cotable.c      |  4 +--
 drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c | 16 +++++++---
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.h          |  1 -
 drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c      |  4 +--
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c          | 32 ++++++++++++++++++--
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.h          |  9 ++++++
 drivers/gpu/drm/vmwgfx/vmwgfx_overlay.c      |  2 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_prime.c        |  2 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_resource.c     | 12 ++++----
 drivers/gpu/drm/vmwgfx/vmwgfx_shader.c       |  4 +--
 drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c         | 12 ++------
 drivers/gpu/drm/vmwgfx/vmwgfx_surface.c      | 11 ++++---
 drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c         | 12 ++++++++
 16 files changed, 94 insertions(+), 55 deletions(-)

-- 
2.54.0

[PATCH v3 1/5] 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.

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

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

[Claude Code Review Bot]

Patch Review

**Coding style issue — declaration after statement:**

```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 `struct drm_crtc *crtc` and `struct vmw_private *dev_priv` declarations are interspersed with code. While C99/GNU extensions allow this, Linux kernel style strongly prefers all declarations at the top of the block. Move `crtc` and `dev_priv` declarations to the top (alongside `new_state`, `old_state`) and assign them after the NULL checks. Also there's a double space (`=  new_state`) on the crtc assignment line.

**Questionable NULL check on new_state:** In the atomic commit path, `drm_atomic_get_new_plane_state()` should always return a valid state for a plane that is part of the commit — this function is called from `atomic_update`, which only runs for planes included in the commit. If `new_state` is NULL here, something has gone seriously wrong at a higher layer. A `WARN_ON(!new_state)` plus return might be more appropriate to flag the unexpected condition rather than silently returning.

**Logic correctness:** The ternary for `crtc` is correct — it safely handles `old_state` being NULL. The original code `new_state->crtc ?: old_state->crtc` would crash if `new_state->crtc` was NULL and `old_state` was also NULL.

---
Generated by Claude Code Patch Reviewer

[PATCH v3 2/5] 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

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

[Claude Code Review Bot]

Patch Review

**Good fix for missing mode_valid callback.** The `drm_mode_config_funcs.mode_valid` callback is invoked from `drm_mode_setcrtc` but was not previously set for vmwgfx, meaning modes that only go through the legacy setcrtc path bypassed validation.

**Duplicated validation logic:** The new `vmw_kms_config_mode_valid()` duplicates a significant portion of the checks from `vmw_stdu_connector_mode_valid()` (size validation, memory checks, vrefresh). The stdu connector path checks additional constraints (stdu_max_width/height, max_mob_pages, max_mob_size) that the new config-level callback does not. This is reasonable since the config-level check applies to all display units (sou, ldu, stdu), but it would be worth a comment noting the relationship between the two validation levels.

**GPU_TILE_SIZE move is correct.** Moving the `#define GPU_TILE_SIZE 64` from `vmwgfx_stdu.c` to `vmwgfx_kms.h` makes sense since it's now used in `vmwgfx_kms.c` as well.

**vrefresh check added to stdu connector callback too** — good for consistency, covering both the atomic (connector mode_valid) and legacy (config mode_valid) paths.

---
Generated by Claude Code Patch Reviewer

[PATCH v3 3/5] 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

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

[Claude Code Review Bot]

Patch Review

**vmw_bo_free reservation concern:** The added `ttm_bo_reserve`/`ttm_bo_unreserve` pair wraps `vmw_resource_mob_detach` in the buffer object destructor:

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

This is inside `vmw_bo_free`, which is the TTM buffer object destruction callback. At this point the buffer object refcount is already 0 (confirmed by the `WARN_ON(kref_read(&vbo->tbo.base.refcount) != 0)` at the top of `vmw_bo_free`). Calling `ttm_bo_reserve` on an object with refcount 0 is potentially problematic — `ttm_bo_reserve` calls `__ttm_bo_reserve` which does `ww_mutex_lock` on the reservation object. If another thread is concurrently trying to lock this (e.g., during eviction), the object could be in an inconsistent state. The return value of `ttm_bo_reserve` is also not checked here (the `(void)` cast on `vmw_resource_reserve` above suggests ignoring errors is intentional for this path, but the reserve call itself could fail).

Compare with `vmw_resource_release` (vmwgfx_resource.c:129) which does `ttm_bo_reserve` but has `BUG_ON(ret)` — at least there the BO has a valid refcount.

Consider whether `dma_resv_assert_held` is the right thing to assert in `vmw_resource_mob_detach`, or whether the assertion should be conditionally relaxed for the destructor path.

**vkms CRC worker reservation — reasonable fix:**

```c
+		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);
```

This is a valid fix since `compute_crc` calls `vmw_bo_map_and_cache`, which likely needs the reservation held. However, looking at the `goto done` path:

```c
+done:
+
 		vmw_surface_unreference(&surf);
```

There's a blank line after the `done:` label — minor style nit. More importantly, when the reservation fails the CRC is silently skipped with only a warning. This is acceptable for a CRC worker (best-effort), but the CRC subsystem will get stale/missing CRC entries for the affected frames.

---
Generated by Claude Code Patch Reviewer

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

[Maaz Mombasawala]

Preserve a ttm reference 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.

Signed-off-by: Maaz Mombasawala <maaz.mombasawala@broadcom.com>
---
 drivers/gpu/drm/vmwgfx/ttm_object.c     | 7 +++++--
 drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 5 +++--
 2 files changed, 8 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_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
index b2d3927b5567..256e7b89daea 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
@@ -2328,11 +2328,12 @@ 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;
+
 err:
 	if (res)
 		vmw_resource_unreference(&res);
-
-	ttm_ref_object_base_unref(tfile, arg.rep.handle);
+	if (ret)
+		ttm_ref_object_base_unref(tfile, arg.rep.handle);
 
 	return ret;
 }
-- 
2.54.0

Re: [PATCH v3 4/5] drm/vmwgfx: Change ttm refs for dumb buffers.

[Zack Rusin]

[-- Attachment #1: Type: text/plain, Size: 2999 bytes --]

On Thu, May 14, 2026 at 6:46 PM Maaz Mombasawala
<maaz.mombasawala@broadcom.com> wrote:
>
> Preserve a ttm reference 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.
>
> Signed-off-by: Maaz Mombasawala <maaz.mombasawala@broadcom.com>
> ---
>  drivers/gpu/drm/vmwgfx/ttm_object.c     | 7 +++++--
>  drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 5 +++--
>  2 files changed, 8 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_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> index b2d3927b5567..256e7b89daea 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> @@ -2328,11 +2328,12 @@ 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;
> +
>  err:
>         if (res)
>                 vmw_resource_unreference(&res);
> -
> -       ttm_ref_object_base_unref(tfile, arg.rep.handle);
> +       if (ret)
> +               ttm_ref_object_base_unref(tfile, arg.rep.handle);

That still doesn't look correct. dumb buffers own the resource and
should drop it themselves. What you did is made arg.rep.handle stay in
tdev->idr and now both the tfile and dumb_buffer own the surface.
Whichever one is destroyed first will make the other UAF on that
surface.

For all the commits in that series that are fixes please find a proper
Fixes commit. And if you want to land this through drm-misc-fixes then
you'll need to remove the last one from the series because I don't
think we should land general cleanups through fixes.

z

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5414 bytes --]

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

[PATCH v3 5/5] drm/vmwgfx: Remove duplicates in vmwgfx_bo.h

[Maaz Mombasawala]

The vmw_user_bo_ref and vmw_user_bo_unref functions are identical to
vmw_bo_reference and vmw_bo_unreference respectively, remove them.

Signed-off-by: Maaz Mombasawala <maaz.mombasawala@broadcom.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_bo.c       |  4 ++--
 drivers/gpu/drm/vmwgfx/vmwgfx_bo.h       | 15 ---------------
 drivers/gpu/drm/vmwgfx/vmwgfx_cotable.c  |  4 ++--
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.h      |  1 -
 drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c  |  4 ++--
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c      |  4 ++--
 drivers/gpu/drm/vmwgfx/vmwgfx_overlay.c  |  2 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_prime.c    |  2 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 12 ++++++------
 drivers/gpu/drm/vmwgfx/vmwgfx_shader.c   |  4 ++--
 drivers/gpu/drm/vmwgfx/vmwgfx_surface.c  |  6 +++---
 11 files changed, 21 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
index 6817e0b8ce99..4071190408b5 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
@@ -539,7 +539,7 @@ static int vmw_user_bo_synccpu_release(struct drm_file *filp,
 		if (!(flags & drm_vmw_synccpu_allow_cs)) {
 			atomic_dec(&vmw_bo->cpu_writers);
 		}
-		vmw_user_bo_unref(&vmw_bo);
+		vmw_bo_unreference(&vmw_bo);
 	}
 
 	return ret;
@@ -581,7 +581,7 @@ int vmw_user_bo_synccpu_ioctl(struct drm_device *dev, void *data,
 			return ret;
 
 		ret = vmw_user_bo_synccpu_grab(vbo, arg->flags);
-		vmw_user_bo_unref(&vbo);
+		vmw_bo_unreference(&vbo);
 		if (unlikely(ret != 0)) {
 			if (ret == -ERESTARTSYS || ret == -EBUSY)
 				return -EBUSY;
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.h b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.h
index cf84a163bfcb..f3c2cfb42588 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.h
@@ -213,21 +213,6 @@ static inline struct vmw_bo *vmw_bo_reference(struct vmw_bo *buf)
 	return buf;
 }
 
-static inline struct vmw_bo *vmw_user_bo_ref(struct vmw_bo *vbo)
-{
-	drm_gem_object_get(&vbo->tbo.base);
-	return vbo;
-}
-
-static inline void vmw_user_bo_unref(struct vmw_bo **buf)
-{
-	struct vmw_bo *tmp_buf = *buf;
-
-	*buf = NULL;
-	if (tmp_buf)
-		drm_gem_object_put(&tmp_buf->tbo.base);
-}
-
 static inline struct vmw_bo *to_vmw_bo(struct drm_gem_object *gobj)
 {
 	return container_of((gobj), struct vmw_bo, tbo.base);
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_cotable.c b/drivers/gpu/drm/vmwgfx/vmwgfx_cotable.c
index 091f1039a052..9c882e746e3c 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_cotable.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_cotable.c
@@ -502,7 +502,7 @@ static int vmw_cotable_resize(struct vmw_resource *res, size_t new_size)
 
 	vmw_resource_mob_attach(res);
 	/* Let go of the old mob. */
-	vmw_user_bo_unref(&old_buf);
+	vmw_bo_unreference(&old_buf);
 	res->id = vcotbl->type;
 
 	ret = dma_resv_reserve_fences(bo->base.resv, 1);
@@ -521,7 +521,7 @@ static int vmw_cotable_resize(struct vmw_resource *res, size_t new_size)
 out_wait:
 	ttm_bo_unpin(bo);
 	ttm_bo_unreserve(bo);
-	vmw_user_bo_unref(&buf);
+	vmw_bo_unreference(&buf);
 
 out_done:
 	MKS_STAT_TIME_POP(MKSSTAT_KERN_COTABLE_RESIZE);
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
index 38bea8abab84..812f1224f83e 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
@@ -96,7 +96,6 @@ struct vmwgfx_hash_item {
 
 struct vmw_res_func;
 
-struct vmw_bo;
 struct vmw_bo;
 struct vmw_resource_dirty;
 
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
index e1f18020170a..f3fd45f28140 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
@@ -1148,7 +1148,7 @@ static int vmw_translate_mob_ptr(struct vmw_private *dev_priv,
 	vmw_bo_placement_set(vmw_bo, VMW_BO_DOMAIN_MOB, VMW_BO_DOMAIN_MOB);
 	ret = vmw_validation_add_bo(sw_context->ctx, vmw_bo);
 	tmp_bo = vmw_bo;
-	vmw_user_bo_unref(&tmp_bo);
+	vmw_bo_unreference(&tmp_bo);
 	if (unlikely(ret != 0))
 		return ret;
 
@@ -1205,7 +1205,7 @@ static int vmw_translate_guest_ptr(struct vmw_private *dev_priv,
 			     VMW_BO_DOMAIN_GMR | VMW_BO_DOMAIN_VRAM);
 	ret = vmw_validation_add_bo(sw_context->ctx, vmw_bo);
 	tmp_bo = vmw_bo;
-	vmw_user_bo_unref(&tmp_bo);
+	vmw_bo_unreference(&tmp_bo);
 	if (unlikely(ret != 0))
 		return ret;
 
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 4db29b645a38..6bd83e49885b 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -1968,7 +1968,7 @@ int vmw_connector_get_modes(struct drm_connector *connector)
 struct vmw_user_object *vmw_user_object_ref(struct vmw_user_object *uo)
 {
 	if (uo->buffer)
-		vmw_user_bo_ref(uo->buffer);
+		vmw_bo_reference(uo->buffer);
 	else if (uo->surface)
 		vmw_surface_reference(uo->surface);
 	return uo;
@@ -1977,7 +1977,7 @@ struct vmw_user_object *vmw_user_object_ref(struct vmw_user_object *uo)
 void vmw_user_object_unref(struct vmw_user_object *uo)
 {
 	if (uo->buffer)
-		vmw_user_bo_unref(&uo->buffer);
+		vmw_bo_unreference(&uo->buffer);
 	else if (uo->surface)
 		vmw_surface_unreference(&uo->surface);
 }
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_overlay.c b/drivers/gpu/drm/vmwgfx/vmwgfx_overlay.c
index 679adf7c7183..d4a1cd50c325 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_overlay.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_overlay.c
@@ -451,7 +451,7 @@ int vmw_overlay_ioctl(struct drm_device *dev, void *data,
 
 	ret = vmw_overlay_update_stream(dev_priv, buf, arg, true);
 
-	vmw_user_bo_unref(&buf);
+	vmw_bo_unreference(&buf);
 
 out_unlock:
 	mutex_unlock(&overlay->mutex);
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c b/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c
index 598b90ac7590..6abeb1ee1f8f 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c
@@ -117,7 +117,7 @@ int vmw_prime_handle_to_fd(struct drm_device *dev,
 								 handle, flags,
 								 prime_fd);
 		}
-		vmw_user_bo_unref(&vbo);
+		vmw_bo_unreference(&vbo);
 	}
 
 	return ret;
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
index 388011696941..1391bed79f7b 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
@@ -143,7 +143,7 @@ static void vmw_resource_release(struct kref *kref)
 		if (res->coherent)
 			vmw_bo_dirty_release(res->guest_memory_bo);
 		ttm_bo_unreserve(bo);
-		vmw_user_bo_unref(&res->guest_memory_bo);
+		vmw_bo_unreference(&res->guest_memory_bo);
 	}
 
 	if (likely(res->hw_destroy != NULL)) {
@@ -315,7 +315,7 @@ int vmw_user_object_lookup(struct vmw_private *dev_priv,
 							    uo->buffer,
 							    handle);
 		if (uo->surface)
-			vmw_user_bo_unref(&uo->buffer);
+			vmw_bo_unreference(&uo->buffer);
 	}
 
 	return ret;
@@ -466,11 +466,11 @@ void vmw_resource_unreserve(struct vmw_resource *res,
 			vmw_resource_mob_detach(res);
 			if (res->coherent)
 				vmw_bo_dirty_release(res->guest_memory_bo);
-			vmw_user_bo_unref(&res->guest_memory_bo);
+			vmw_bo_unreference(&res->guest_memory_bo);
 		}
 
 		if (new_guest_memory_bo) {
-			res->guest_memory_bo = vmw_user_bo_ref(new_guest_memory_bo);
+			res->guest_memory_bo = vmw_bo_reference(new_guest_memory_bo);
 
 			/*
 			 * The validation code should already have added a
@@ -560,7 +560,7 @@ vmw_resource_check_buffer(struct ww_acquire_ctx *ticket,
 	drm_gem_object_put(&val_buf->bo->base);
 	val_buf->bo = NULL;
 	if (guest_memory_dirty)
-		vmw_user_bo_unref(&res->guest_memory_bo);
+		vmw_bo_unreference(&res->guest_memory_bo);
 
 	return ret;
 }
@@ -736,7 +736,7 @@ int vmw_resource_validate(struct vmw_resource *res, bool intr,
 		goto out_no_validate;
 	else if (!res->func->needs_guest_memory && res->guest_memory_bo) {
 		WARN_ON_ONCE(vmw_resource_mob_attached(res));
-		vmw_user_bo_unref(&res->guest_memory_bo);
+		vmw_bo_unreference(&res->guest_memory_bo);
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_shader.c b/drivers/gpu/drm/vmwgfx/vmwgfx_shader.c
index eca4e3e97eb4..ede4de9dc8b0 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_shader.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_shader.c
@@ -180,7 +180,7 @@ static int vmw_gb_shader_init(struct vmw_private *dev_priv,
 
 	res->guest_memory_size = size;
 	if (byte_code) {
-		res->guest_memory_bo = vmw_user_bo_ref(byte_code);
+		res->guest_memory_bo = vmw_bo_reference(byte_code);
 		res->guest_memory_offset = offset;
 	}
 	shader->size = size;
@@ -809,7 +809,7 @@ static int vmw_shader_define(struct drm_device *dev, struct drm_file *file_priv,
 				    shader_type, num_input_sig,
 				    num_output_sig, tfile, shader_handle);
 out_bad_arg:
-	vmw_user_bo_unref(&buffer);
+	vmw_bo_unreference(&buffer);
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
index 256e7b89daea..3525d1388f33 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
@@ -972,7 +972,7 @@ static int vmw_buffer_prime_to_surface_base(struct vmw_private *dev_priv,
 
 	*base_p = base;
 out:
-	vmw_user_bo_unref(&bo);
+	vmw_bo_unreference(&bo);
 
 	return ret;
 }
@@ -1593,12 +1593,12 @@ vmw_gb_surface_define_internal(struct drm_device *dev,
 		if (ret == 0) {
 			if (res->guest_memory_bo->is_dumb) {
 				VMW_DEBUG_USER("Can't backup surface with a dumb buffer.\n");
-				vmw_user_bo_unref(&res->guest_memory_bo);
+				vmw_bo_unreference(&res->guest_memory_bo);
 				ret = -EINVAL;
 				goto out_unlock;
 			} else if (res->guest_memory_bo->tbo.base.size < res->guest_memory_size) {
 				VMW_DEBUG_USER("Surface backup buffer too small.\n");
-				vmw_user_bo_unref(&res->guest_memory_bo);
+				vmw_bo_unreference(&res->guest_memory_bo);
 				ret = -EINVAL;
 				goto out_unlock;
 			} else {
-- 
2.54.0

Claude review: drm/vmwgfx: Remove duplicates in vmwgfx_bo.h

[Claude Code Review Bot]

Patch Review

**Straightforward and correct cleanup.** The removed functions:

```c
-static inline struct vmw_bo *vmw_user_bo_ref(struct vmw_bo *vbo)
-{
-	drm_gem_object_get(&vbo->tbo.base);
-	return vbo;
-}
-
-static inline void vmw_user_bo_unref(struct vmw_bo **buf)
-{
-	struct vmw_bo *tmp_buf = *buf;
-
-	*buf = NULL;
-	if (tmp_buf)
-		drm_gem_object_put(&tmp_buf->tbo.base);
-}
```

These are indeed identical to `vmw_bo_reference` and `vmw_bo_unreference`. All callers are mechanically updated. The duplicate `struct vmw_bo;` forward declaration removal in `vmwgfx_drv.h` is a nice bonus cleanup.

One concern: this patch touches files also touched by patch 4 (`vmwgfx_surface.c`). The series should apply cleanly in order, but it would be worth confirming that the `vmwgfx_surface.c` changes in patch 5 are based on the state after patch 4 (they appear to be, since line numbers differ).

**Summary:** The series fixes real bugs. The main concerns are the coding style in patch 1 (declarations after statements, double space), the safety of `ttm_bo_reserve` on a zero-refcount BO in patch 3, and ensuring the TTM reference lifetime change in patch 4 doesn't introduce a new leak path. Patch 2 and 5 look good.

---
Generated by Claude Code Patch Reviewer

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: 7
Reviewed: 2026-05-16T10:17:48.595176

---

This is a v3 series of 5 patches fixing issues discovered by running igt-gpu-tools against vmwgfx. The fixes address: a NULL dereference in cursor plane update, missing vrefresh validation, missing TTM reservation assertions, a dma_buf reference leak, dumb buffer TTM reference lifetime issues, and cleanup of duplicate helper functions.

The series is reasonable in scope and addresses real bugs. However, there are some coding style issues and one concern about the safety of the reservation approach in the bo destructor path.

---
Generated by Claude Code Patch Reviewer

[PATCH v2 0/5] Fix some issues from igt runs.

[Maaz Mombasawala]

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

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 (5):
  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.
  drm/vmwgfx: Remove duplicates in vmwgfx_bo.h

 drivers/gpu/drm/vmwgfx/ttm_object.c          |  7 +++--
 drivers/gpu/drm/vmwgfx/vmwgfx_bo.c           |  6 ++--
 drivers/gpu/drm/vmwgfx/vmwgfx_bo.h           | 15 ---------
 drivers/gpu/drm/vmwgfx/vmwgfx_cotable.c      |  4 +--
 drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c | 16 +++++++---
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.h          |  1 -
 drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c      |  4 +--
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c          | 32 ++++++++++++++++++--
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.h          |  9 ++++++
 drivers/gpu/drm/vmwgfx/vmwgfx_overlay.c      |  2 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_prime.c        |  2 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_resource.c     | 12 ++++----
 drivers/gpu/drm/vmwgfx/vmwgfx_shader.c       |  4 +--
 drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c         | 12 ++------
 drivers/gpu/drm/vmwgfx/vmwgfx_surface.c      | 11 ++++---
 drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c         | 12 ++++++++
 16 files changed, 94 insertions(+), 55 deletions(-)

-- 
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: 6
Reviewed: 2026-05-16T14:26:30.462768

---

This is a 5-patch series fixing issues found via igt-gpu-tools testing on vmwgfx. The fixes address: a null dereference in cursor plane updates, missing vrefresh validation, missing TTM BO reservation lock acquisition, dumb buffer reference count leaks, and removal of duplicate helper functions. The fixes are generally reasonable and address real bugs, but there are some coding style issues (patch 1) and a locking concern (patch 3) worth discussing.

---

---
Generated by Claude Code Patch Reviewer