[PATCH v3 4/5] drm/amdgpu: validate the eviction fence before attaching/detaching

[Christian König]

On 4/30/25 04:40, Prike Liang wrote:
> Before the user queue BOs resume workqueue is scheduled;
> there's no valid eviction fence to attach the gem obj.
> For this case, it doesn't need to attach/detach the eviction
> fence. Also, it needs to unlock the bo first before returning
> from the eviction fence attached error.
> 
> Signed-off-by: Prike Liang <Prike.Liang at amd.com>
> ---
>  .../drm/amd/amdgpu/amdgpu_eviction_fence.c    | 22 +++++++++++++++----
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c       |  3 ++-
>  2 files changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> index 7a5f02ef45a7..242bfb91c4f7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> @@ -196,16 +196,22 @@ int amdgpu_eviction_fence_attach(struct amdgpu_eviction_fence_mgr *evf_mgr,
>  	if (!resv)
>  		return 0;
>  
> +	/* Validate the eviction fence first */
> +	spin_lock(&evf_mgr->ev_fence_lock);
> +	ev_fence = evf_mgr->ev_fence;
> +	if (!ev_fence ||
> +			dma_fence_is_signaled(&evf_mgr->ev_fence->base)) {
> +		spin_unlock(&evf_mgr->ev_fence_lock);
> +		return 0;
> +	}
> +
>  	ret = dma_resv_reserve_fences(resv, 1);
>  	if (ret) {
>  		DRM_DEBUG_DRIVER("Failed to resv fence space\n");
>  		return ret;
>  	}
>  
> -	spin_lock(&evf_mgr->ev_fence_lock);
> -	ev_fence = evf_mgr->ev_fence;
> -	if (ev_fence)
> -		dma_resv_add_fence(resv, &ev_fence->base, DMA_RESV_USAGE_BOOKKEEP);
> +	dma_resv_add_fence(resv, &ev_fence->base, DMA_RESV_USAGE_BOOKKEEP);

Once more: Absolutely clear NAK to that! You are breaking the logic here.


>  	spin_unlock(&evf_mgr->ev_fence_lock);
>  
>  	return 0;
> @@ -216,6 +222,14 @@ void amdgpu_eviction_fence_detach(struct amdgpu_eviction_fence_mgr *evf_mgr,
>  {
>  	struct dma_fence *stub = dma_fence_get_stub();
>  
> +	spin_lock(&evf_mgr->ev_fence_lock);
> +	if (!evf_mgr->ev_fence ||
> +			dma_fence_is_signaled(&evf_mgr->ev_fence->base)) {
> +		spin_unlock(&evf_mgr->ev_fence_lock);
> +		return;
> +	}
> +	spin_unlock(&evf_mgr->ev_fence_lock);
> +
That is unnecessary as far as I can see.

>  	dma_resv_replace_fences(bo->tbo.base.resv, evf_mgr->ev_fence_ctx,
>  				stub, DMA_RESV_USAGE_BOOKKEEP);
>  	dma_fence_put(stub);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index f03fc3cf4d50..86779dc817b9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -294,10 +294,11 @@ static int amdgpu_gem_object_open(struct drm_gem_object *obj,
>  	else
>  		++bo_va->ref_count;
>  
> -	/* attach gfx eviction fence */
> +	/* attach gfx the validated eviction fence */
>  	r = amdgpu_eviction_fence_attach(&fpriv->evf_mgr, abo);
>  	if (r) {
>  		DRM_DEBUG_DRIVER("Failed to attach eviction fence to BO\n");
> +		amdgpu_bo_unreserve(abo);
Adding this here looks like the only valid fix in the patch.

Regards,
Christian.

>  		return r;
>  	}
>