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 <[email protected]>
> ---
> .../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
