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()
- [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