Re: [PATCH v5] drm/amdgpu: bind BOs when the eviction fence is attached

On 8/25/26 09:53, Yifan Zhang wrote:
> From: Prerona Ghosh <[email protected]>
> 
> DMA-buf imports are created in TTM_PL_SYSTEM and land on the VM idle
> list, so nothing binds them for a VM using user queues: amdgpu_vm_validate()
> and amdgpu_userq_bo_validate() skip idle BOs, and amdgpu_cs never runs.
> AMDGPU_GEM_VA then programs PTEs without AMDGPU_PTE_VALID and the first
> GPU access faults.
> 
> amdgpu_evf_mgr_attach_fence() is the right place to fix this, but it only
> validated while the eviction fence was unsignaled. At amdgpu_gem_object_open()
> time the fence is still the permanently signaled stub installed by
> amdgpu_evf_mgr_init(); a real fence only appears after amdgpu_evf_mgr_rearm().
> Natively created BOs hide the bug because amdgpu_bo_create() already
> validated them.
> 
> Validate imported BOs even when the fence is signaled, and unwind the
> bo_va if that fails.
> 
> v3: validate inside amdgpu_gem_object_open (Christian)
> v4: fix amdgpu_evf_mgr_attach_fence itself instead (Christian)
> v5: restrict the extra validation to imports so that resident and pinned
>     BOs are not migrated or failed on every GEM open
> 
> Signed-off-by: Prerona Ghosh <[email protected]>
> Signed-off-by: Yifan Zhang <[email protected]>
> Assisted-by: Claude:claude-opus-5.0
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c | 11 ++++++++---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c            |  9 ++++++++-
>  2 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> index 4c5e38dea4c2..c2f9380511a3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> @@ -101,11 +101,16 @@ int amdgpu_evf_mgr_attach_fence(struct amdgpu_eviction_fence_mgr *evf_mgr,
>  	struct dma_resv *resv = bo->tbo.base.resv;
>  	int ret;
>  
> -	if (!dma_fence_is_signaled(ev_fence)) {
> -
> +	/*
> +	 * Imports are created in TTM_PL_SYSTEM and this is the only place a VM
> +	 * using user queues binds them, so validate those even when the fence
> +	 * is already signaled.
> +	 */
> +	if (!dma_fence_is_signaled(ev_fence) ||
> +	    drm_gem_is_imported(&bo->tbo.base)) {

That still doesn't make much sense. When the fence is signaled there is no reason whatsoever to validate imported BOs.

We are still missing something here which explains why that doesn't work as expected.

>  		amdgpu_bo_placement_from_domain(bo, bo->allowed_domains);
>  		ret = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
> -		if (!ret)
> +		if (!ret && !dma_fence_is_signaled(ev_fence))

That check is superfluous.

>  			dma_resv_add_fence(resv, ev_fence,
>  					   DMA_RESV_USAGE_BOOKKEEP);
>  	} else {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index f754a4a3a1c2..3118b3036564 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -264,9 +264,16 @@ static int amdgpu_gem_object_open(struct drm_gem_object *obj,
>  	bo_va = amdgpu_vm_bo_find(vm, abo);
>  	if (!bo_va) {
>  		bo_va = amdgpu_vm_bo_add(adev, vm, abo);
> +		if (!bo_va) {
> +			r = -ENOMEM;
> +			goto out_unlock;
> +		}
> +

That should probably be a separate patch.

Regards,
Christian.

>  		r = amdgpu_evf_mgr_attach_fence(&fpriv->evf_mgr, abo);
> -		if (r)
> +		if (r) {
> +			amdgpu_vm_bo_del(adev, bo_va);
>  			goto out_unlock;
> +		}
>  	} else {
>  		++bo_va->ref_count;
>  	}