[PATCH v2] drm/vmwgfx: unpin BOs without revalidating placement

[PATCH v2] drm/vmwgfx: unpin BOs without revalidating placement

[潘煜杭]

Hello,

I apologize for the repeated mistakes in my previous messages and patch
submissions. Thank you for your patience. This version includes the corrected
Fixes tag and an unwrapped patch diff.

Unpinning a buffer object does not change its placement. Avoid calling
ttm_bo_validate() on the unpin path, where a placement validation failure
currently triggers BUG_ON() and crashes the kernel during PRIME cleanup.

Keep validation on the pin path, where it is needed to establish the
requested placement before pinning.

During PRIME/GEM cleanup, vmw_gem_object_unpin() calls
vmw_bo_pin_reserved(vbo, false). The latter currently calls
ttm_bo_validate() for both pin and unpin operations. In the observed Linux
7.3 QEMU run, ttm_bo_validate() returned -EINVAL on the unpin path; the
following BUG_ON() caused a kernel BUG during process teardown.

The path is:

  exit_group()
    -> GEM/PRIME object cleanup
    -> vmw_gem_object_unpin()
    -> vmw_bo_pin_reserved(vbo, false)
    -> ttm_bo_validate() returns -EINVAL
    -> BUG_ON(ret != 0 || bo->resource->mem_type != old_mem_type)

Observed report excerpt:

  [BO_PIN_TRACE] before-validate pin=0 old_mem_type=0 target_mem_type=0 flags=0x10
  [BO_PIN_TRACE] after-validate ret=-22 old_mem_type=0 new_mem_type=0 ...
  kernel BUG at drivers/gpu/drm/vmwgfx/vmwgfx_bo.c:342!
  RIP: vmw_bo_pin_reserved+0x36d/0x380

The change skips placement validation when unpinning and retains it on the
pin path.

Fixes: fbe86ca56791 ("drm/vmwgfx: switch over to the new pin interface v2")
Cc: stable@vger.kernel.org
Signed-off-by: Yuhang Pan <242270054@hdu.edu.cn>
---

Sorry, the previous submission was damaged by Thunderbird's line wrapping.
This v2 keeps the diff header on one physical line. I verified patch
applicability to the clean Linux 7.3 index after normalizing email CRLF line
endings; runtime verification of this exact patch is still pending.

 drivers/gpu/drm/vmwgfx/vmwgfx_bo.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
index 9c7a73c..14d28e2 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
@@ -308,6 +308,11 @@ void vmw_bo_pin_reserved(struct vmw_bo *vbo, bool pin)
 	if (pin == !!bo->pin_count)
 		return;
 
+	if (!pin) {
+		ttm_bo_unpin(bo);
+		return;
+	}
+
 	pl.fpfn = 0;
 	pl.lpfn = 0;
 	pl.mem_type = bo->resource->mem_type;
@@ -321,10 +326,7 @@ void vmw_bo_pin_reserved(struct vmw_bo *vbo, bool pin)
 
 	BUG_ON(ret != 0 || bo->resource->mem_type != old_mem_type);
 
-	if (pin)
-		ttm_bo_pin(bo);
-	else
-		ttm_bo_unpin(bo);
+	ttm_bo_pin(bo);
 }
 
 /**
-- 
2.43.0

Regards,
Yuhang Pan
242270054@hdu.edu.cn

Re: [PATCH v2] drm/vmwgfx: unpin BOs without revalidating placement

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm/vmwgfx: vestigial ttm_bo_validate() and BUG_ON() in vmw_bo_pin_reserved()
- [Medium] drm/vmwgfx: inaccurate commit description and email notes above tear line
--

commit 25af1e918639e493179b2c83013bfbfaba035064
Author: 潘煜杭 <242270054@hdu.edu.cn>

drm/vmwgfx: unpin BOs without revalidating placement

This commit skips calling ttm_bo_validate() when unpinning a buffer object in
vmw_bo_pin_reserved() to avoid hitting BUG_ON() during PRIME/GEM teardown,
while retaining the ttm_bo_validate() call on the pin path.

>     drm/vmwgfx: unpin BOs without revalidating placement
>     
>     Hello,
>     
>     I apologize for the repeated mistakes in my previous messages and patch
>     submissions. Thank you for your patience. This version includes the corrected
>     Fixes tag and an unwrapped patch diff.
>     
>     Unpinning a buffer object does not change its placement. Avoid calling
>     ttm_bo_validate() on the unpin path, where a placement validation failure
>     currently triggers BUG_ON() and crashes the kernel during PRIME cleanup.
>     
>     Keep validation on the pin path, where it is needed to establish the
>     requested placement before pinning.
>     
>     During PRIME/GEM cleanup, vmw_gem_object_unpin() calls
>     vmw_bo_pin_reserved(vbo, false). The latter currently calls
>     ttm_bo_validate() for both pin and unpin operations. In the observed Linux
>     7.3 QEMU run, ttm_bo_validate() returned -EINVAL on the unpin path; the
>     following BUG_ON() caused a kernel BUG during process teardown.
>     
>     The path is:
>     
>       exit_group()
>         -> GEM/PRIME object cleanup
>         -> vmw_gem_object_unpin()
>         -> vmw_bo_pin_reserved(vbo, false)
>         -> ttm_bo_validate() returns -EINVAL
>         -> BUG_ON(ret != 0 || bo->resource->mem_type != old_mem_type)
>     
>     Observed report excerpt:
>     
>       [BO_PIN_TRACE] before-validate pin=0 old_mem_type=0 target_mem_type=0 flags=0x10
>       [BO_PIN_TRACE] after-validate ret=-22 old_mem_type=0 new_mem_type=0 ...
>       kernel BUG at drivers/gpu/drm/vmwgfx/vmwgfx_bo.c:342!
>       RIP: vmw_bo_pin_reserved+0x36d/0x380
>     
>     The change skips placement validation when unpinning and retains it on the
>     pin path.
>     
>     Fixes: fbe86ca56791 ("drm/vmwgfx: switch over to the new pin interface v2")
>     Cc: stable@vger.kernel.org
>     Signed-off-by: Yuhang Pan <242270054@hdu.edu.cn>

[Severity: Medium]
Should the opening greeting and submission notes be moved below the ---
separator so they are not recorded in the permanent git commit log?

Also, is the statement "Keep validation on the pin path, where it is needed
to establish the requested placement before pinning" accurate?

Looking at the kernel-doc above vmw_bo_pin_reserved() in
drivers/gpu/drm/vmwgfx/vmwgfx_bo.c:

    /**
     * vmw_bo_pin_reserved - Pin or unpin a buffer object without moving it.
     *
     * @vbo: The buffer object. Must be reserved.
     * @pin: Whether to pin or unpin.
     *
     */

vmw_bo_pin_reserved() does not take or establish a requested placement; it
constructs a temporary struct ttm_place from the BO's existing
bo->resource->mem_type and bo->resource->placement and asserts via BUG_ON()
that mem_type does not change, which is a leftover from how
TTM_PL_FLAG_NO_EVICT was toggled prior to commit fbe86ca56791 ("drm/vmwgfx:
switch over to the new pin interface v2").

In addition, would it be helpful for the commit message to explain why
ttm_bo_validate() returned -EINVAL when flags=0x10 (TTM_PL_FLAG_FALLBACK)?
Commits a78a8da51b36 ("drm/ttm: replace busy placement with flags v6") and
cc941c70df39 ("drm/ttm: improve idle/busy handling v5") caused
vmw_bo_placement_set() to mark fallback placements with TTM_PL_FLAG_FALLBACK
and made ttm_resource_compatible() reject TTM_PL_FLAG_FALLBACK on the
non-evicting first pass of ttm_bo_validate(), causing ttm_bo_validate() to
fall through to:

    if (bo->pin_count)
        return -EINVAL;

when called on an already-pinned BO.

> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> index 9c7a73c0b0dc3..14d28e2ff0481 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> @@ -308,6 +308,11 @@ void vmw_bo_pin_reserved(struct vmw_bo *vbo, bool pin)
>  	if (pin == !!bo->pin_count)
>  		return;
>  
> +	if (!pin) {
> +		ttm_bo_unpin(bo);
> +		return;
> +	}
> +
>  	pl.fpfn = 0;
>  	pl.lpfn = 0;
>  	pl.mem_type = bo->resource->mem_type;
> @@ -321,10 +326,7 @@ void vmw_bo_pin_reserved(struct vmw_bo *vbo, bool pin)
>  
>  	BUG_ON(ret != 0 || bo->resource->mem_type != old_mem_type);
>  
> -	if (pin)
> -		ttm_bo_pin(bo);
> -	else
> -		ttm_bo_unpin(bo);
> +	ttm_bo_pin(bo);
>  }

[Severity: Medium]
Should ttm_bo_validate(), BUG_ON(), and the now-unused local variables (ctx,
pl, placement, old_mem_type, ret) be removed from the pin path of
vmw_bo_pin_reserved() as well, so that it simply calls ttm_bo_pin(bo) or
ttm_bo_unpin(bo)?

Prior to commit fbe86ca56791 ("drm/vmwgfx: switch over to the new pin
interface v2"), TTM represented pinned BOs via TTM_PL_FLAG_NO_EVICT in
bo->mem.placement, so vmw_bo_pin_reserved() copied the current placement into
pl, toggled TTM_PL_FLAG_NO_EVICT, and called ttm_bo_validate() solely to
update the flag and LRU state in-place without moving the BO. Once commit
fbe86ca56791 replaced TTM_PL_FLAG_NO_EVICT with ttm_bo_pin() and
ttm_bo_unpin(), the ttm_bo_validate() call and BUG_ON() became a vestige on
both pin and unpin paths. All callers that require a specific placement
(vmw_bo_pin_in_placement(), vmw_bo_pin_in_vram_or_gmr(),
vmw_bo_pin_in_start_of_vram(), vmw_resource_pin(), and
vmw_query_bo_switch_commit()) already call ttm_bo_validate() prior to calling
vmw_bo_pin_reserved(vbo, true).

Leaving ttm_bo_validate() and BUG_ON() on the pin path still causes a problem
when pinning a BO whose resource was allocated in a fallback busy_domain so
that bo->resource->placement has TTM_PL_FLAG_FALLBACK (0x10) set, reachable
via:

    drm_gem_map_attach()
      -> vmw_gem_object_pin()
        -> vmw_bo_pin_reserved(vbo, true)

or:

    vmw_resource_pin()
      -> vmw_bo_pin_reserved(vbo, true)

When vmw_bo_pin_reserved(vbo, true) copies bo->resource->placement into
pl.flags, the first pass of ttm_bo_validate() (force_space == false) calls
ttm_resource_compatible() in drivers/gpu/drm/ttm/ttm_resource.c:

    if (place->flags & (evicting ? TTM_PL_FLAG_DESIRED :
                        TTM_PL_FLAG_FALLBACK))
        continue;

which rejects TTM_PL_FLAG_FALLBACK when evicting is false and returns false.
Because bo->pin_count is 0, ttm_bo_validate() then enters
ttm_bo_alloc_resource() in drivers/gpu/drm/ttm/ttm_bo.c:

    ticket = dma_resv_locking_ctx(bo->base.resv);
    ret = dma_resv_reserve_fences(bo->base.resv, TTM_NUM_MOVE_FENCES);
    if (unlikely(ret))
        return ret;

before skipping the TTM_PL_FLAG_FALLBACK placement, returning -ENOSPC, and
only succeeding on the second pass (force_space == true). If
dma_resv_reserve_fences() fails with -ENOMEM, ttm_bo_validate() returns
-ENOMEM and triggers BUG_ON(ret != 0 || bo->resource->mem_type !=
old_mem_type) in vmw_bo_pin_reserved().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/93888c18-7ce4-4341-b387-a80a29847d3e@hdu.edu.cn?part=1

[PATCH v2] drm/vmwgfx: unpin BOs without revalidating placement

[潘煜杭]

Hello,

I apologize for the repeated mistakes in my previous messages and patch
submissions. Thank you for your patience. This version includes the corrected
Fixes tag and an unwrapped patch diff.

Unpinning a buffer object does not change its placement. Avoid calling
ttm_bo_validate() on the unpin path, where a placement validation failure
currently triggers BUG_ON() and crashes the kernel during PRIME cleanup.

Keep validation on the pin path, where it is needed to establish the
requested placement before pinning.

During PRIME/GEM cleanup, vmw_gem_object_unpin() calls
vmw_bo_pin_reserved(vbo, false). The latter currently calls
ttm_bo_validate() for both pin and unpin operations. In the observed Linux
7.3 QEMU run, ttm_bo_validate() returned -EINVAL on the unpin path; the
following BUG_ON() caused a kernel BUG during process teardown.

The path is:

  exit_group()
    -> GEM/PRIME object cleanup
    -> vmw_gem_object_unpin()
    -> vmw_bo_pin_reserved(vbo, false)
    -> ttm_bo_validate() returns -EINVAL
    -> BUG_ON(ret != 0 || bo->resource->mem_type != old_mem_type)

Observed report excerpt:

  [BO_PIN_TRACE] before-validate pin=0 old_mem_type=0 target_mem_type=0 flags=0x10
  [BO_PIN_TRACE] after-validate ret=-22 old_mem_type=0 new_mem_type=0 ...
  kernel BUG at drivers/gpu/drm/vmwgfx/vmwgfx_bo.c:342!
  RIP: vmw_bo_pin_reserved+0x36d/0x380

The change skips placement validation when unpinning and retains it on the
pin path.

Fixes: fbe86ca56791 ("drm/vmwgfx: switch over to the new pin interface v2")
Cc: stable@vger.kernel.org
Signed-off-by: Yuhang Pan <242270054@hdu.edu.cn>
---

Sorry, the previous submission was damaged by Thunderbird's line wrapping.
This v2 keeps the diff header on one physical line. I verified patch
applicability to the clean Linux 7.3 index after normalizing email CRLF line
endings; runtime verification of this exact patch is still pending.

 drivers/gpu/drm/vmwgfx/vmwgfx_bo.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
index 9c7a73c..14d28e2 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
@@ -308,6 +308,11 @@ void vmw_bo_pin_reserved(struct vmw_bo *vbo, bool pin)
 	if (pin == !!bo->pin_count)
 		return;
 
+	if (!pin) {
+		ttm_bo_unpin(bo);
+		return;
+	}
+
 	pl.fpfn = 0;
 	pl.lpfn = 0;
 	pl.mem_type = bo->resource->mem_type;
@@ -321,10 +326,7 @@ void vmw_bo_pin_reserved(struct vmw_bo *vbo, bool pin)
 
 	BUG_ON(ret != 0 || bo->resource->mem_type != old_mem_type);
 
-	if (pin)
-		ttm_bo_pin(bo);
-	else
-		ttm_bo_unpin(bo);
+	ttm_bo_pin(bo);
 }
 
 /**
-- 
2.43.0

Regards,
Yuhang Pan
242270054@hdu.edu.cn

Re: [PATCH v2] drm/vmwgfx: unpin BOs without revalidating placement

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm/vmwgfx: vestigial ttm_bo_validate() and BUG_ON() in vmw_bo_pin_reserved()
- [Low] drm/vmwgfx: email text and inaccurate claim in vmw_bo_pin_reserved() commit log
--

commit da725c3c2e0e2c05d0e0cf86657330ca0fde440a
Author: 潘煜杭 <242270054@hdu.edu.cn>

drm/vmwgfx: unpin BOs without revalidating placement

This commit skips calling ttm_bo_validate() when unpinning a buffer object in
vmw_bo_pin_reserved() to avoid triggering BUG_ON() when validation fails on a
pinned buffer object during PRIME cleanup.

>     drm/vmwgfx: unpin BOs without revalidating placement
>     
>     Hello,
>     
>     I apologize for the repeated mistakes in my previous messages and patch
>     submissions. Thank you for your patience. This version includes the corrected
>     Fixes tag and an unwrapped patch diff.
>     
>     Unpinning a buffer object does not change its placement. Avoid calling
>     ttm_bo_validate() on the unpin path, where a placement validation failure
>     currently triggers BUG_ON() and crashes the kernel during PRIME cleanup.
>     
>     Keep validation on the pin path, where it is needed to establish the
>     requested placement before pinning.
>     
>     During PRIME/GEM cleanup, vmw_gem_object_unpin() calls
>     vmw_bo_pin_reserved(vbo, false). The latter currently calls
>     ttm_bo_validate() for both pin and unpin operations. In the observed Linux
>     7.3 QEMU run, ttm_bo_validate() returned -EINVAL on the unpin path; the
>     following BUG_ON() caused a kernel BUG during process teardown.
>     
>     The path is:
>     
>       exit_group()
>         -> GEM/PRIME object cleanup
>         -> vmw_gem_object_unpin()
>         -> vmw_bo_pin_reserved(vbo, false)
>         -> ttm_bo_validate() returns -EINVAL
>         -> BUG_ON(ret != 0 || bo->resource->mem_type != old_mem_type)
>     
>     Observed report excerpt:
>     
>       [BO_PIN_TRACE] before-validate pin=0 old_mem_type=0 target_mem_type=0 flags=0x10
>       [BO_PIN_TRACE] after-validate ret=-22 old_mem_type=0 new_mem_type=0 ...
>       kernel BUG at drivers/gpu/drm/vmwgfx/vmwgfx_bo.c:342!
>       RIP: vmw_bo_pin_reserved+0x36d/0x380
>     
>     The change skips placement validation when unpinning and retains it on the
>     pin path.
>     
>     Fixes: fbe86ca56791 ("drm/vmwgfx: switch over to the new pin interface v2")
>     Cc: stable@vger.kernel.org
>     Signed-off-by: Yuhang Pan <242270054@hdu.edu.cn>

[Severity: Low]
This isn't a bug in the code, but should the introductory greeting ("Hello,
I apologize...") be moved below the "---" separator line so that it is not
recorded in the permanent git commit history?

Also, is the statement in the commit message that validation on the pin path
"is needed to establish the requested placement before pinning" accurate?
Looking at vmw_bo_pin_reserved() in drivers/gpu/drm/vmwgfx/vmwgfx_bo.c, it is
documented to pin or unpin a buffer object without moving it, takes no
placement parameter, and only copies bo->resource->mem_type and
bo->resource->placement from the already-allocated resource (a leftover from
when TTM pinned and unpinned BOs by toggling TTM_PL_FLAG_NO_EVICT via
ttm_bo_validate() prior to commit fbe86ca56791).

In the trace shown in the commit message, flags=0x10 is TTM_PL_FLAG_FALLBACK:
ttm_resource_compatible() rejects TTM_PL_FLAG_FALLBACK on the first validation
pass when force_space is false, which causes ttm_bo_validate() to reach the
if (bo->pin_count) return -EINVAL check.

> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> index 9c7a73c0b0dc3..14d28e2ff0481 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> @@ -308,6 +308,11 @@ void vmw_bo_pin_reserved(struct vmw_bo *vbo, bool pin)
>  	if (pin == !!bo->pin_count)
>  		return;
>  
> +	if (!pin) {
> +		ttm_bo_unpin(bo);
> +		return;
> +	}
> +
>  	pl.fpfn = 0;
>  	pl.lpfn = 0;
>  	pl.mem_type = bo->resource->mem_type;
> @@ -321,10 +326,7 @@ void vmw_bo_pin_reserved(struct vmw_bo *vbo, bool pin)
>  
>  	BUG_ON(ret != 0 || bo->resource->mem_type != old_mem_type);
>  
> -	if (pin)
> -		ttm_bo_pin(bo);
> -	else
> -		ttm_bo_unpin(bo);
> +	ttm_bo_pin(bo);

[Severity: Medium]
Should the ttm_bo_validate() call and BUG_ON() in vmw_bo_pin_reserved() be
removed from the pin path as well?

When a buffer object resides in a fallback placement, bo->resource->placement
has TTM_PL_FLAG_FALLBACK (0x10) set via placement_flags() in
drivers/gpu/drm/vmwgfx/vmwgfx_bo.c. When vmw_bo_pin_reserved(vbo, true) is
called (for example via vmw_gem_object_pin(), vmw_resource_pin(), or
vmw_execbuf_process()), it copies bo->resource->placement into pl.flags and
calls ttm_bo_validate(bo, &placement, &ctx):

drivers/gpu/drm/vmwgfx/vmwgfx_bo.c:vmw_bo_pin_reserved() {
	pl.fpfn = 0;
	pl.lpfn = 0;
	pl.mem_type = bo->resource->mem_type;
	pl.flags = bo->resource->placement;
	...
	ret = ttm_bo_validate(bo, &placement, &ctx);

	BUG_ON(ret != 0 || bo->resource->mem_type != old_mem_type);
}

On the first pass of ttm_bo_validate() (where force_space is false),
ttm_resource_compatible() rejects TTM_PL_FLAG_FALLBACK and returns false:

drivers/gpu/drm/ttm/ttm_resource.c:ttm_resource_compatible() {
		if (place->flags & (evicting ? TTM_PL_FLAG_DESIRED :
				    TTM_PL_FLAG_FALLBACK))
			continue;
}

Because ttm_resource_compatible() returns false, ttm_bo_validate() calls
ttm_bo_alloc_resource(), which calls dma_resv_reserve_fences() before skipping
the fallback placement and retrying with force_space set to true:

drivers/gpu/drm/ttm/ttm_bo.c:ttm_bo_alloc_resource() {
	ticket = dma_resv_locking_ctx(bo->base.resv);
	ret = dma_resv_reserve_fences(bo->base.resv, TTM_NUM_MOVE_FENCES);
	if (unlikely(ret))
		return ret;
}

If dma_resv_reserve_fences() fails with -ENOMEM under memory pressure,
ttm_bo_validate() returns -ENOMEM and triggers BUG_ON(ret != 0 ||
bo->resource->mem_type != old_mem_type) on the pin path.

Prior to commit fbe86ca56791 ("drm/vmwgfx: switch over to the new pin
interface v2"), TTM pinned and unpinned BOs by toggling TTM_PL_FLAG_NO_EVICT
in pl.flags and calling ttm_bo_validate() to update the placement flag
in-place. Once commit fbe86ca56791 switched to ttm_bo_pin() and
ttm_bo_unpin(), the ttm_bo_validate() call in vmw_bo_pin_reserved() became
vestigial on both paths: whenever ttm_bo_validate() succeeds here, it returns
0 early at the ttm_resource_compatible() check without moving the buffer or
creating a TTM, and callers needing a specific placement already call
ttm_bo_validate() beforehand.

Could vmw_bo_pin_reserved() drop the ttm_bo_validate() block and unused local
variables (ctx, pl, placement, old_mem_type, ret) and simply call
ttm_bo_pin(bo) or ttm_bo_unpin(bo) when pin != !!bo->pin_count?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/064da7ba-58ca-4027-88a9-12fb41e8b7d4@hdu.edu.cn?part=1