Re: [PATCH] drm/amdgpu: dont pin wptr bo instead use eviction fence

On 15-07-2026 09:31 pm, Khatri, Sunil wrote:
>
>
> On 15-07-2026 08:30 pm, Christian König wrote:
>> On 7/15/26 16:24, Sunil Khatri wrote:
>>> Instead of pinning the wptr bo attach the eviction fence to
>>> the bo to make sure it remains valid all the time.
>>>
>>> Signed-off-by: Sunil Khatri<[email protected]>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c  | 34 ++++++++++++++++++++++
>>>   drivers/gpu/drm/amd/amdgpu/mes_userqueue.c | 17 ++++-------
>>>   2 files changed, 39 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> index 911fb325c70c..e1eb689710c8 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> @@ -936,6 +936,29 @@ static int amdgpu_userq_validate_vm(void *param, struct amdgpu_bo *bo)
>>>   	return ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
>>>   }
>>>   
>>> +static int amdgpu_userq_validate_wptr_bo(struct drm_exec *exec,
>>> +					 struct amdgpu_usermode_queue *queue)
>>> +{
>>> +	struct amdgpu_bo *bo = queue->wptr_obj.obj;
>>> +	int ret;
>>> +
>>> +	if (!bo)
>>> +		return -EINVAL;
>>> +
>>> +	ret = drm_exec_prepare_obj(exec, &bo->tbo.base,
>>> +				   TTM_NUM_MOVE_FENCES + 1);
>>> +	if (unlikely(ret))
>>> +		return ret;
>> The WPTR BO should already be locked and prepared with fence slots by the caller, doing that here again can cause problems.
>>
>>> +
>>> +	ret = amdgpu_ttm_alloc_gart(&bo->tbo);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	queue->wptr_obj.gpu_addr = amdgpu_bo_gpu_offset(bo);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>   /* Handle all BOs on the invalidated list, validate them and update the PTs */
>>>   static int
>>>   amdgpu_userq_bo_validate(struct amdgpu_device *adev, struct drm_exec *exec,
>>> @@ -988,6 +1011,7 @@ amdgpu_userq_vm_validate_and_restore_queue(struct amdgpu_userq_mgr *uq_mgr)
>>>   	struct amdgpu_vm *vm = &fpriv->vm;
>>>   	unsigned long key, tmp_key;
>>>   	struct amdgpu_bo_va *bo_va;
>>> +	struct amdgpu_usermode_queue *queue;
>>>   	struct amdgpu_bo *bo;
>>>   	struct drm_exec exec;
>>>   	struct xarray xa;
>>> @@ -1094,6 +1118,16 @@ amdgpu_userq_vm_validate_and_restore_queue(struct amdgpu_userq_mgr *uq_mgr)
>>>   	if (ret)
>>>   		goto unlock_all;
>>>   
>>> +	xa_for_each(&uq_mgr->userq_xa, tmp_key, queue) {
>>> +		ret = amdgpu_userq_validate_wptr_bo(&exec, queue);
>>> +		if (unlikely(ret)) {
>>> +			drm_file_err(uq_mgr->file,
>>> +				     "failed to validate wptr bo on resume, qid=%lu ret=%d\n",
>>> +				     tmp_key, ret);
>>> +			goto unlock_all;
>>> +		}
>>> +	}
>>> +
>>>   	/*
>>>   	 * We need to wait for all VM updates to finish before restarting the
>>>   	 * queues. Using the idle list like that is now ok since everything is
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
>>> index 19c3cb61929c..ab8baaa83e60 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
>>> @@ -71,27 +71,23 @@ mes_userq_create_wptr_mapping(struct amdgpu_device *adev,
>>>   		ret = -EINVAL;
>>>   		goto fail_map;
>>>   	}
>>> -
>>> -	/* TODO use eviction fence instead of pinning. */
>>> -	ret = amdgpu_bo_pin(wptr_obj->obj, AMDGPU_GEM_DOMAIN_GTT);
>>> +	/* Keep WPTR BO under eviction-fence control instead of pinning. */
>>> +	ret = amdgpu_evf_mgr_attach_fence(&uq_mgr_to_fpriv(uq_mgr)->evf_mgr, wptr_obj->obj);
>> The eviction fence should already be attached to the WPTR BO when it is part of the VM.
> Sorry i dint understand. From amdgpu_userq_create, sequence is like 
> this uq_funcs->mqd_create(queue, &args->in) -> 
> mes_userq_create_wptr_mapping (This is where eviction fence is attached)
>
> after mqd_create we are checking if the ev fence is signalled or not 
> and based on that we are again going ahead to attach new ev fence if 
> signaled to all bos.
>
>
>> The problem is more that the function is called at the completely wrong time. See amdgpu_userq_create():
> You mean to say mqd create is called at wrong place ?
>> And BTW why do we have the abstraction of uq_funcs? That doesn't seem to make sense.
>
> I also need to check that.
>
Yes, we are having the abstraction layer for uq_functions but not really 
needed. I feel the reason here is to not contaminate userqueues code 
with mes layer code that's all. Since this is written by @alex, i would 
like him to comment on this.

Regards
Sunil Khatri
>
> Regards
> Sunil Khatri
>
>> Regards,
>> Christian.
>>
>>>   	if (ret) {
>>> -		DRM_ERROR("Failed to pin wptr bo. ret %d\n", ret);
>>> +		DRM_ERROR("Failed to attach eviction fence to wptr bo. ret %d\n", ret);
>>>   		goto fail_map;
>>>   	}
>>>   
>>>   	ret = amdgpu_ttm_alloc_gart(&wptr_obj->obj->tbo);
>>>   	if (ret) {
>>> -		DRM_ERROR("Failed to bind bo to GART. ret %d\n", ret);
>>> -		goto fail_alloc_gart;
>>> +		DRM_ERROR("Failed to bind wptr bo to GART. ret %d\n", ret);
>>> +		goto fail_map;
>>>   	}
>>>   
>>>   	queue->wptr_obj.gpu_addr = amdgpu_bo_gpu_offset(wptr_obj->obj);
>>>   
>>>   	drm_exec_fini(&exec);
>>>   	return 0;
>>> -
>>> -fail_alloc_gart:
>>> -	amdgpu_bo_unpin(wptr_obj->obj);
>>>   fail_map:
>>>   	amdgpu_bo_unref(&wptr_obj->obj);
>>>   fail_lock:
>>> @@ -520,9 +516,6 @@ static void mes_userq_mqd_destroy(struct amdgpu_usermode_queue *queue)
>>>   	amdgpu_bo_free_kernel(&queue->mqd.obj, &queue->mqd.gpu_addr,
>>>   			      &queue->mqd.cpu_ptr);
>>>   
>>> -	amdgpu_bo_reserve(queue->wptr_obj.obj, true);
>>> -	amdgpu_bo_unpin(queue->wptr_obj.obj);
>>> -	amdgpu_bo_unreserve(queue->wptr_obj.obj);
>>>   	amdgpu_bo_unref(&queue->wptr_obj.obj);
>>>   }
>>>