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: 潘煜杭 <[email protected]> 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: [email protected] > Signed-off-by: Yuhang Pan <[email protected]> [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/[email protected]?part=1
