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

[sashiko-bot@kernel.org]

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