[PATCH] drm/amdgpu/userq: use drm_exec in amdgpu_userq_fence_read_wptr

[PATCH] drm/amdgpu/userq: use drm_exec in amdgpu_userq_fence_read_wptr

[Sunil Khatri]

To access the bo from vm mapping first lock the root bo and
then the object bo of the mapping to make sure both locks
are taken safely.

Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
---
 .../gpu/drm/amd/amdgpu/amdgpu_userq_fence.c   | 50 +++++++++----------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
index 35ced3c26c8c..e5522e953edc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
@@ -370,34 +370,35 @@ static int amdgpu_userq_fence_read_wptr(struct amdgpu_device *adev,
 {
 	struct amdgpu_bo_va_mapping *mapping;
 	struct amdgpu_bo *bo;
+	struct drm_exec exec;
 	u64 addr, *ptr;
-	int r;
-
-	r = amdgpu_bo_reserve(queue->vm->root.bo, false);
-	if (r)
-		return r;
+	int ret;
 
 	addr = queue->userq_prop->wptr_gpu_addr;
 	addr &= AMDGPU_GMC_HOLE_MASK;
 
-	mapping = amdgpu_vm_bo_lookup_mapping(queue->vm, addr >> PAGE_SHIFT);
-	if (!mapping) {
-		amdgpu_bo_unreserve(queue->vm->root.bo);
-		DRM_ERROR("Failed to lookup amdgpu_bo_va_mapping\n");
-		return -EINVAL;
-	}
+	drm_exec_init(&exec, DRM_EXEC_IGNORE_DUPLICATES, 2);
+	drm_exec_until_all_locked(&exec) {
+		ret = amdgpu_vm_lock_pd(queue->vm, &exec, 1);
+		drm_exec_retry_on_contention(&exec);
+		if (unlikely(ret))
+			goto lock_error;
 
-	bo = amdgpu_bo_ref(mapping->bo_va->base.bo);
-	amdgpu_bo_unreserve(queue->vm->root.bo);
-	r = amdgpu_bo_reserve(bo, true);
-	if (r) {
-		amdgpu_bo_unref(&bo);
-		DRM_ERROR("Failed to reserve userqueue wptr bo");
-		return r;
+		mapping = amdgpu_vm_bo_lookup_mapping(queue->vm, addr >> PAGE_SHIFT);
+		if (!mapping) {
+			ret = -EINVAL;
+			goto lock_error;
+		}
+
+		ret = drm_exec_lock_obj(&exec, &mapping->bo_va->base.bo->tbo.base);
+		drm_exec_retry_on_contention(&exec);
+		if (unlikely(ret))
+			goto lock_error;
 	}
 
-	r = amdgpu_bo_kmap(bo, (void **)&ptr);
-	if (r) {
+	bo = amdgpu_bo_ref(mapping->bo_va->base.bo);
+	ret = amdgpu_bo_kmap(bo, (void **)&ptr);
+	if (ret) {
 		DRM_ERROR("Failed mapping the userqueue wptr bo");
 		goto map_error;
 	}
@@ -405,16 +406,15 @@ static int amdgpu_userq_fence_read_wptr(struct amdgpu_device *adev,
 	*wptr = le64_to_cpu(*ptr);
 
 	amdgpu_bo_kunmap(bo);
-	amdgpu_bo_unreserve(bo);
 	amdgpu_bo_unref(&bo);
-
+	drm_exec_fini(&exec);
 	return 0;
 
 map_error:
-	amdgpu_bo_unreserve(bo);
 	amdgpu_bo_unref(&bo);
-
-	return r;
+lock_error:
+	drm_exec_fini(&exec);
+	return ret;
 }
 
 static void
-- 
2.34.1

Re: [PATCH] drm/amdgpu/userq: use drm_exec in amdgpu_userq_fence_read_wptr

[Christian König]

On 5/8/26 09:01, Sunil Khatri wrote:
> To access the bo from vm mapping first lock the root bo and
> then the object bo of the mapping to make sure both locks
> are taken safely.
> 
> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
> ---
>  .../gpu/drm/amd/amdgpu/amdgpu_userq_fence.c   | 50 +++++++++----------
>  1 file changed, 25 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> index 35ced3c26c8c..e5522e953edc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> @@ -370,34 +370,35 @@ static int amdgpu_userq_fence_read_wptr(struct amdgpu_device *adev,
>  {
>  	struct amdgpu_bo_va_mapping *mapping;
>  	struct amdgpu_bo *bo;
> +	struct drm_exec exec;
>  	u64 addr, *ptr;
> -	int r;
> -
> -	r = amdgpu_bo_reserve(queue->vm->root.bo, false);
> -	if (r)
> -		return r;
> +	int ret;
>  
>  	addr = queue->userq_prop->wptr_gpu_addr;
>  	addr &= AMDGPU_GMC_HOLE_MASK;
>  
> -	mapping = amdgpu_vm_bo_lookup_mapping(queue->vm, addr >> PAGE_SHIFT);
> -	if (!mapping) {
> -		amdgpu_bo_unreserve(queue->vm->root.bo);
> -		DRM_ERROR("Failed to lookup amdgpu_bo_va_mapping\n");
> -		return -EINVAL;
> -	}
> +	drm_exec_init(&exec, DRM_EXEC_IGNORE_DUPLICATES, 2);
> +	drm_exec_until_all_locked(&exec) {
> +		ret = amdgpu_vm_lock_pd(queue->vm, &exec, 1);
> +		drm_exec_retry_on_contention(&exec);
> +		if (unlikely(ret))
> +			goto lock_error;
>  
> -	bo = amdgpu_bo_ref(mapping->bo_va->base.bo);
> -	amdgpu_bo_unreserve(queue->vm->root.bo);
> -	r = amdgpu_bo_reserve(bo, true);
> -	if (r) {
> -		amdgpu_bo_unref(&bo);
> -		DRM_ERROR("Failed to reserve userqueue wptr bo");
> -		return r;
> +		mapping = amdgpu_vm_bo_lookup_mapping(queue->vm, addr >> PAGE_SHIFT);
> +		if (!mapping) {
> +			ret = -EINVAL;
> +			goto lock_error;
> +		}
> +
> +		ret = drm_exec_lock_obj(&exec, &mapping->bo_va->base.bo->tbo.base);
> +		drm_exec_retry_on_contention(&exec);
> +		if (unlikely(ret))
> +			goto lock_error;
>  	}
>  
> -	r = amdgpu_bo_kmap(bo, (void **)&ptr);
> -	if (r) {
> +	bo = amdgpu_bo_ref(mapping->bo_va->base.bo);

As far as I can see we don't need that reference any more since drm_exec always keeps a reference to all the buffers it has locked.

Apart from that looks good to me,
Christian.

> +	ret = amdgpu_bo_kmap(bo, (void **)&ptr);
> +	if (ret) {
>  		DRM_ERROR("Failed mapping the userqueue wptr bo");
>  		goto map_error;
>  	}
> @@ -405,16 +406,15 @@ static int amdgpu_userq_fence_read_wptr(struct amdgpu_device *adev,
>  	*wptr = le64_to_cpu(*ptr);
>  
>  	amdgpu_bo_kunmap(bo);
> -	amdgpu_bo_unreserve(bo);
>  	amdgpu_bo_unref(&bo);
> -
> +	drm_exec_fini(&exec);
>  	return 0;
>  
>  map_error:
> -	amdgpu_bo_unreserve(bo);
>  	amdgpu_bo_unref(&bo);
> -
> -	return r;
> +lock_error:
> +	drm_exec_fini(&exec);
> +	return ret;
>  }
>  
>  static void