From: "Khatri, Sunil" <sukhatri@amd.com>
To: "Christian König" <christian.koenig@amd.com>,
"Sunil Khatri" <sunil.khatri@amd.com>,
"Alex Deucher" <alexander.deucher@amd.com>
Cc: amd-gfx@lists.freedesktop.org, Liang@rtg-sunil-navi33.amd.com,
Prike <Prike.Liang@amd.com>
Subject: Re: [PATCH] drm/amdgpu/userq: add mutex lock/unlocked version of amdgpu_userq_put
Date: Fri, 20 Mar 2026 17:40:55 +0530 [thread overview]
Message-ID: <8251843c-db9b-4841-a6d8-807bbd4e5fe6@amd.com> (raw)
In-Reply-To: <df13f705-78c5-4e3a-86bf-8fc969b30d5c@amd.com>
Sent a different patch to fix the irrelevant get/put.
drm/amdgpu/userq: cleanup amdgpu_userq_get/put where not needed
regards
Sunil khatri
On 20-03-2026 04:44 pm, Khatri, Sunil wrote:
>
> On 20-03-2026 04:27 pm, Christian König wrote:
>> On 3/20/26 11:53, Khatri, Sunil wrote:
>>> On 20-03-2026 03:43 pm, Christian König wrote:
>>>> On 3/20/26 11:02, Khatri, Sunil wrote:
>>>>> On 20-03-2026 03:16 pm, Christian König wrote:
>>>>>> On 3/20/26 10:41, Sunil Khatri wrote:
>>>>>>> There is a possibility of deadlock when last reference to a
>>>>>>> queue is
>>>>>>> put in certain situations where mutex is already help when calling
>>>>>>> the amdgpu_userq_destroy.
>>>>>> From functions
>>>>>> As far as I can see that is illegal to begin with. Why are we
>>>>>> doing that?
>>>>> This is to fix the deadlock that prike shared and many other
>>>>> places where deadlock could still be caused.
>>>>> There is a possibility of amdgpu_userq_put being called for last
>>>>> reference from amdgpu_userq_restore_worker or
>>>>> amdgpu_eviction_fence_suspend_worker via amdgpu_evf_mgr_shutdown
>>>>> and all these functions already hold the userq_mutex and on last
>>>>> reference when destroy is called it again takes userq_mutex and
>>>>> causing deadlock.
>>>>>
>>>>> Thats why when we are dropping the reference we pass the
>>>>> information of the handled could be called with lock already taken
>>>>> and hence the handling.
>>>> Well that sounds like completely broken handling.
>>>>
>>>> Why are dropping an userqueu reference while holding the lock in
>>>> the first place?
>>> we are doing at withing the locked state in most of the place in the
>>> code.
>>> few examples:
>>> static void amdgpu_userq_restore_worker(struct work_struct *work)
>>> {
>>> struct amdgpu_userq_mgr *uq_mgr = work_to_uq_mgr(work,
>>> resume_work.work);
>>> struct amdgpu_fpriv *fpriv = uq_mgr_to_fpriv(uq_mgr);
>>> struct dma_fence *ev_fence;
>>> int ret;
>>>
>>> mutex_lock(&uq_mgr->userq_mutex);
>>> ev_fence = amdgpu_evf_mgr_get_fence(&fpriv->evf_mgr);
>>> if (!dma_fence_is_signaled(ev_fence))
>>> goto unlock;
>>>
>>> ret = amdgpu_userq_vm_validate(uq_mgr);
>>> if (ret) {
>>> drm_file_err(uq_mgr->file, "Failed to validate BOs to
>>> restore\n");
>>> goto unlock;
>>> }
>>>
>>> // Here the restore all is going through all the queues one bye
>>> one by doing get and put
>> That is ok, the question is why the heck is doing that get/put? See
>> we need get/put because we are *not* holding the lock.
>>
>> When we are holding the lock no get/put is needed at all.
> Got your point. In that case there is a bug in code and this could be
> resolved with clean up or doing get/put where queue mutex is not locked.
> Will send the update here.
>
> Regards
> Sunil Khatri
>>
>> Regards,
>> Christian.
>>
>>> and doing the restore of each queue. Now during put if its last
>>> reference due to race with another thread in putting we will
>>> call the destroy with locks taken which causes deadlock.
>>> ret = amdgpu_userq_restore_all(uq_mgr);
>>> if (ret) {
>>> drm_file_err(uq_mgr->file, "Failed to restore all queues\n");
>>> goto unlock;
>>> }
>>>
>>> unlock:
>>> mutex_unlock(&uq_mgr->userq_mutex);
>>> dma_fence_put(ev_fence);
>>> }
>>>
>>> Another example:
>>> static void
>>> amdgpu_eviction_fence_suspend_worker(struct work_struct *work)
>>> {
>>> struct amdgpu_eviction_fence_mgr *evf_mgr =
>>> container_of(work, struct amdgpu_eviction_fence_mgr,
>>> suspend_work);
>>> struct amdgpu_fpriv *fpriv =
>>> container_of(evf_mgr, struct amdgpu_fpriv, evf_mgr);
>>> struct amdgpu_userq_mgr *uq_mgr = &fpriv->userq_mgr;
>>> struct dma_fence *ev_fence;
>>> bool cookie;
>>>
>>> mutex_lock(&uq_mgr->userq_mutex);
>>>
>>> /*
>>> * This is intentionally after taking the userq_mutex since we do
>>> * allocate memory while holding this lock, but only after
>>> ensuring that
>>> * the eviction fence is signaled.
>>> */
>>> cookie = dma_fence_begin_signalling();
>>>
>>> ev_fence = amdgpu_evf_mgr_get_fence(evf_mgr);
>>>
>>>
>>> /* Here in userq evict we do a ref get and put a various places
>>> while the mutex is already taken */
>>>
>>> amdgpu_userq_evict(uq_mgr, !evf_mgr->shutdown);
>>>
>>> /*
>>> * Signaling the eviction fence must be done while holding the
>>> * userq_mutex. Otherwise we won't resume the queues before
>>> issuing the
>>> * next fence.
>>> */
>>> dma_fence_signal(ev_fence);
>>> dma_fence_end_signalling(cookie);
>>> dma_fence_put(ev_fence);
>>> mutex_unlock(&uq_mgr->userq_mutex);
>>>
>>> }
>>>
>>> Regards
>>> Sunil.
>>>
>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> Regards
>>>>> Sunil khatri
>>>>>
>>>>>> Regards,
>>>>>> Christian.
>>>>>>
>>>>>>> So based on the thread where it could be
>>>>>>> locked we pass the locked information in the destroy functionality
>>>>>>> to avoid taking the lock again.
>>>>>>>
>>>>>>> Cc: Liang, Prike <Prike.Liang@amd.com>
>>>>>>> Suggested-by: Liang, Prike <Prike.Liang@amd.com>
>>>>>>> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
>>>>>>> ---
>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 52
>>>>>>> +++++++++++++------
>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h | 2 +-
>>>>>>> .../gpu/drm/amd/amdgpu/amdgpu_userq_fence.c | 4 +-
>>>>>>> 3 files changed, 40 insertions(+), 18 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>>>>>> index ced9ade44be4..9482664e9c2c 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>>>>>> @@ -617,13 +617,17 @@ amdgpu_userq_get_doorbell_index(struct
>>>>>>> amdgpu_userq_mgr *uq_mgr,
>>>>>>> }
>>>>>>> static int
>>>>>>> -amdgpu_userq_destroy(struct amdgpu_userq_mgr *uq_mgr, struct
>>>>>>> amdgpu_usermode_queue *queue)
>>>>>>> +amdgpu_userq_destroy(struct amdgpu_userq_mgr *uq_mgr, struct
>>>>>>> amdgpu_usermode_queue *queue,
>>>>>>> + bool locked)
>>>>>>> {
>>>>>>> struct amdgpu_device *adev = uq_mgr->adev;
>>>>>>> int r = 0;
>>>>>>> - cancel_delayed_work_sync(&uq_mgr->resume_work);
>>>>>>> + /* It safe to unlock since we are in destroy and the queue
>>>>>>> ref is only this */
>>>>>>> + if (locked)
>>>>>>> + mutex_unlock(&uq_mgr->userq_mutex);
>>>>>>> + cancel_delayed_work_sync(&uq_mgr->resume_work);
>>>>>>> /* Cancel any pending hang detection work and cleanup */
>>>>>>> cancel_delayed_work_sync(&queue->hang_detect_work);
>>>>>>> @@ -657,13 +661,27 @@ amdgpu_userq_destroy(struct
>>>>>>> amdgpu_userq_mgr *uq_mgr, struct amdgpu_usermode_que
>>>>>>> queue->state = AMDGPU_USERQ_STATE_HUNG;
>>>>>>> }
>>>>>>> amdgpu_userq_cleanup(queue);
>>>>>>> - mutex_unlock(&uq_mgr->userq_mutex);
>>>>>>> +
>>>>>>> + if (!locked)
>>>>>>> + mutex_unlock(&uq_mgr->userq_mutex);
>>>>>>> pm_runtime_put_autosuspend(adev_to_drm(adev)->dev);
>>>>>>> return r;
>>>>>>> }
>>>>>>> +static void amdgpu_userq_kref_destroy_locked(struct kref
>>>>>>> *kref)
>>>>>>> +{
>>>>>>> + int r;
>>>>>>> + struct amdgpu_usermode_queue *queue =
>>>>>>> + container_of(kref, struct amdgpu_usermode_queue,
>>>>>>> refcount);
>>>>>>> + struct amdgpu_userq_mgr *uq_mgr = queue->userq_mgr;
>>>>>>> +
>>>>>>> + r = amdgpu_userq_destroy(uq_mgr, queue, true);
>>>>>>> + if (r)
>>>>>>> + drm_file_err(uq_mgr->file, "Failed to destroy usermode
>>>>>>> queue %d\n", r);
>>>>>>> +}
>>>>>>> +
>>>>>>> static void amdgpu_userq_kref_destroy(struct kref *kref)
>>>>>>> {
>>>>>>> int r;
>>>>>>> @@ -671,7 +689,7 @@ static void amdgpu_userq_kref_destroy(struct
>>>>>>> kref *kref)
>>>>>>> container_of(kref, struct amdgpu_usermode_queue,
>>>>>>> refcount);
>>>>>>> struct amdgpu_userq_mgr *uq_mgr = queue->userq_mgr;
>>>>>>> - r = amdgpu_userq_destroy(uq_mgr, queue);
>>>>>>> + r = amdgpu_userq_destroy(uq_mgr, queue, false);
>>>>>>> if (r)
>>>>>>> drm_file_err(uq_mgr->file, "Failed to destroy
>>>>>>> usermode queue %d\n", r);
>>>>>>> }
>>>>>>> @@ -689,10 +707,14 @@ struct amdgpu_usermode_queue
>>>>>>> *amdgpu_userq_get(struct amdgpu_userq_mgr *uq_mgr,
>>>>>>> return queue;
>>>>>>> }
>>>>>>> -void amdgpu_userq_put(struct amdgpu_usermode_queue *queue)
>>>>>>> +void amdgpu_userq_put(struct amdgpu_usermode_queue *queue, bool
>>>>>>> locked)
>>>>>>> {
>>>>>>> - if (queue)
>>>>>>> - kref_put(&queue->refcount, amdgpu_userq_kref_destroy);
>>>>>>> + if (queue) {
>>>>>>> + if (locked)
>>>>>>> + kref_put(&queue->refcount,
>>>>>>> amdgpu_userq_kref_destroy_locked);
>>>>>>> + else
>>>>>>> + kref_put(&queue->refcount, amdgpu_userq_kref_destroy);
>>>>>>> + }
>>>>>>> }
>>>>>>> static int amdgpu_userq_priority_permit(struct drm_file
>>>>>>> *filp,
>>>>>>> @@ -978,7 +1000,7 @@ int amdgpu_userq_ioctl(struct drm_device
>>>>>>> *dev, void *data,
>>>>>>> if (!queue)
>>>>>>> return -ENOENT;
>>>>>>> - amdgpu_userq_put(queue);
>>>>>>> + amdgpu_userq_put(queue, false);
>>>>>>> break;
>>>>>>> }
>>>>>>> @@ -1007,7 +1029,7 @@ amdgpu_userq_restore_all(struct
>>>>>>> amdgpu_userq_mgr *uq_mgr)
>>>>>>> drm_file_err(uq_mgr->file,
>>>>>>> "trying restore queue without va
>>>>>>> mapping\n");
>>>>>>> queue->state = AMDGPU_USERQ_STATE_INVALID_VA;
>>>>>>> - amdgpu_userq_put(queue);
>>>>>>> + amdgpu_userq_put(queue, true);
>>>>>>> continue;
>>>>>>> }
>>>>>>> @@ -1015,7 +1037,7 @@ amdgpu_userq_restore_all(struct
>>>>>>> amdgpu_userq_mgr *uq_mgr)
>>>>>>> if (r)
>>>>>>> ret = r;
>>>>>>> - amdgpu_userq_put(queue);
>>>>>>> + amdgpu_userq_put(queue, true);
>>>>>>> }
>>>>>>> if (ret)
>>>>>>> @@ -1258,7 +1280,7 @@ amdgpu_userq_evict_all(struct
>>>>>>> amdgpu_userq_mgr *uq_mgr)
>>>>>>> r = amdgpu_userq_preempt_helper(queue);
>>>>>>> if (r)
>>>>>>> ret = r;
>>>>>>> - amdgpu_userq_put(queue);
>>>>>>> + amdgpu_userq_put(queue, true);
>>>>>>> }
>>>>>>> if (ret)
>>>>>>> @@ -1298,17 +1320,17 @@ amdgpu_userq_wait_for_signal(struct
>>>>>>> amdgpu_userq_mgr *uq_mgr)
>>>>>>> struct dma_fence *f = queue->last_fence;
>>>>>>> if (!f || dma_fence_is_signaled(f)) {
>>>>>>> - amdgpu_userq_put(queue);
>>>>>>> + amdgpu_userq_put(queue, true);
>>>>>>> continue;
>>>>>>> }
>>>>>>> ret = dma_fence_wait_timeout(f, true,
>>>>>>> msecs_to_jiffies(100));
>>>>>>> if (ret <= 0) {
>>>>>>> drm_file_err(uq_mgr->file, "Timed out waiting
>>>>>>> for fence=%llu:%llu\n",
>>>>>>> f->context, f->seqno);
>>>>>>> - amdgpu_userq_put(queue);
>>>>>>> + amdgpu_userq_put(queue, true);
>>>>>>> return -ETIMEDOUT;
>>>>>>> }
>>>>>>> - amdgpu_userq_put(queue);
>>>>>>> + amdgpu_userq_put(queue, true);
>>>>>>> }
>>>>>>> return 0;
>>>>>>> @@ -1366,7 +1388,7 @@ void amdgpu_userq_mgr_fini(struct
>>>>>>> amdgpu_userq_mgr *userq_mgr)
>>>>>>> if (!queue)
>>>>>>> break;
>>>>>>> - amdgpu_userq_put(queue);
>>>>>>> + amdgpu_userq_put(queue, false);
>>>>>>> }
>>>>>>> xa_destroy(&userq_mgr->userq_xa);
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
>>>>>>> index f0abc16d02cc..2a496e74ec6a 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
>>>>>>> @@ -116,7 +116,7 @@ struct amdgpu_db_info {
>>>>>>> };
>>>>>>> struct amdgpu_usermode_queue *amdgpu_userq_get(struct
>>>>>>> amdgpu_userq_mgr *uq_mgr, u32 qid);
>>>>>>> -void amdgpu_userq_put(struct amdgpu_usermode_queue *queue);
>>>>>>> +void amdgpu_userq_put(struct amdgpu_usermode_queue *queue, bool
>>>>>>> locked);
>>>>>>> int amdgpu_userq_ioctl(struct drm_device *dev, void *data,
>>>>>>> struct drm_file *filp);
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
>>>>>>> index 18390d37a7e0..10e08cb6bd13 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
>>>>>>> @@ -612,7 +612,7 @@ int amdgpu_userq_signal_ioctl(struct
>>>>>>> drm_device *dev, void *data,
>>>>>>> kfree(syncobj_handles);
>>>>>>> if (queue)
>>>>>>> - amdgpu_userq_put(queue);
>>>>>>> + amdgpu_userq_put(queue, false);
>>>>>>> return r;
>>>>>>> }
>>>>>>> @@ -914,7 +914,7 @@ amdgpu_userq_wait_return_fence_info(struct
>>>>>>> drm_file *filp,
>>>>>>> r = 0;
>>>>>>> put_waitq:
>>>>>>> - amdgpu_userq_put(waitq);
>>>>>>> + amdgpu_userq_put(waitq, false);
>>>>>>> free_fences:
>>>>>>> while (num_fences--)
next prev parent reply other threads:[~2026-03-20 12:11 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-03-20 9:41 [PATCH] drm/amdgpu/userq: add mutex lock/unlocked version of amdgpu_userq_put Sunil Khatri
2026-03-20 9:46 ` Christian König
2026-03-20 10:02 ` Khatri, Sunil
2026-03-20 10:13 ` Christian König
2026-03-20 10:53 ` Khatri, Sunil
2026-03-20 10:57 ` Christian König
2026-03-20 11:14 ` Khatri, Sunil
2026-03-20 12:10 ` Khatri, Sunil [this message]
-- strict thread matches above, loose matches on Subject: below --
2026-03-20 9:43 Sunil Khatri
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8251843c-db9b-4841-a6d8-807bbd4e5fe6@amd.com \
--to=sukhatri@amd.com \
--cc=Liang@rtg-sunil-navi33.amd.com \
--cc=Prike.Liang@amd.com \
--cc=alexander.deucher@amd.com \
--cc=amd-gfx@lists.freedesktop.org \
--cc=christian.koenig@amd.com \
--cc=sunil.khatri@amd.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox