Re: [PATCH V1 1/2] drm/amdgpu/userq: avoid uneccessary locking in amdgpu_userq_create

[Khatri, Sunil]

On 08-04-2026 01:53 pm, Christian König wrote:
On 4/8/26 07:36, Sunil Khatri wrote:
Reorganise code to avoid holding mutex userq_mutex while
also trying to grab exec lock ww_mutex where its not needed
for function amdgpu_userq_input_va_validate
Signed-off-by: Sunil Khatri<sunil.kha...@amd.com>
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 33 +++++++++++------------
  1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
index 3a6e7a569c78..3f502c18879a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
@@ -737,28 +737,17 @@ amdgpu_userq_create(struct drm_file *filp, union 
drm_amdgpu_userq *args)
                return r;
        }
  
-	/*
-        * There could be a situation that we are creating a new queue while
-        * the other queues under this UQ_mgr are suspended. So if there is any
-        * resume work pending, wait for it to get done.
-        *
-        * This will also make sure we have a valid eviction fence ready to be 
used.
-        */
-       amdgpu_userq_ensure_ev_fence(&fpriv->userq_mgr, &fpriv->evf_mgr);
-
        uq_funcs = adev->userq_funcs[args->in.ip_type];
        if (!uq_funcs) {
                drm_file_err(uq_mgr->file, "Usermode queue is not supported for this 
IP (%u)\n",
                             args->in.ip_type);
-               r = -EINVAL;
-               goto unlock;
+               return -EINVAL;
        }
  
  	queue = kzalloc(sizeof(struct amdgpu_usermode_queue), GFP_KERNEL);
        if (!queue) {
                drm_file_err(uq_mgr->file, "Failed to allocate memory for 
queue\n");
-               r = -ENOMEM;
-               goto unlock;
+               return -ENOMEM;
        }
  
  	INIT_LIST_HEAD(&queue->userq_va_list);
@@ -781,12 +770,21 @@ amdgpu_userq_create(struct drm_file *filp, union 
drm_amdgpu_userq *args)
                goto free_queue;
        }
  
+	/*
+        * There could be a situation that we are creating a new queue while
+        * the other queues under this UQ_mgr are suspended. So if there is any
+        * resume work pending, wait for it to get done.
+        *
+        * This will also make sure we have a valid eviction fence ready to be 
used.
+        */
+       amdgpu_userq_ensure_ev_fence(&fpriv->userq_mgr, &fpriv->evf_mgr);
Even that position is not correct. After grabbing the userq_mutex we can't 
allocate memory any more.

+
        /* Convert relative doorbell offset into absolute doorbell index */
        index = amdgpu_userq_get_doorbell_index(uq_mgr, &db_info, filp);
        if (index == (uint64_t)-EINVAL) {
                drm_file_err(uq_mgr->file, "Failed to get doorbell for 
queue\n");
                r = -EINVAL;
-               goto free_queue;
+               goto unlock;
        }
  
  	queue->doorbell_index = index;
@@ -794,7 +792,7 @@ amdgpu_userq_create(struct drm_file *filp, union 
drm_amdgpu_userq *args)
        r = amdgpu_userq_fence_driver_alloc(adev, &queue->fence_drv);
So doing that here is also forbidden.
If i am not wrong we could move this whole code including 
amdgpu_userq_fence_driver_alloc and above out of mutex. Does that sounds 
correct ? Regards Sunil khatri

Regards,
Christian.

        if (r) {
                drm_file_err(uq_mgr->file, "Failed to alloc fence driver\n");
-               goto free_queue;
+               goto unlock;
        }
  
  	r = uq_funcs->mqd_create(queue, &args->in);
@@ -858,11 +856,10 @@ amdgpu_userq_create(struct drm_file *filp, union 
drm_amdgpu_userq *args)
        up_read(&adev->reset_domain->sem);
  clean_fence_driver:
        amdgpu_userq_fence_driver_free(queue);
-free_queue:
-       kfree(queue);
  unlock:
        mutex_unlock(&uq_mgr->userq_mutex);
-
+free_queue:
+       kfree(queue);
        return r;
  }