* [PATCH] drm/amdgpu/userq: add mutex lock/unlocked version of amdgpu_userq_put
@ 2026-03-20 9:41 Sunil Khatri
2026-03-20 9:46 ` Christian König
0 siblings, 1 reply; 9+ messages in thread
From: Sunil Khatri @ 2026-03-20 9:41 UTC (permalink / raw)
To: Alex Deucher, Christian König; +Cc: amd-gfx, Sunil Khatri, Liang, Prike
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. 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--)
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] drm/amdgpu/userq: add mutex lock/unlocked version of amdgpu_userq_put
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
0 siblings, 1 reply; 9+ messages in thread
From: Christian König @ 2026-03-20 9:46 UTC (permalink / raw)
To: Sunil Khatri, Alex Deucher; +Cc: amd-gfx, Liang, Prike
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.
As far as I can see that is illegal to begin with. Why are we doing that?
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--)
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] drm/amdgpu/userq: add mutex lock/unlocked version of amdgpu_userq_put
2026-03-20 9:46 ` Christian König
@ 2026-03-20 10:02 ` Khatri, Sunil
2026-03-20 10:13 ` Christian König
0 siblings, 1 reply; 9+ messages in thread
From: Khatri, Sunil @ 2026-03-20 10:02 UTC (permalink / raw)
To: Christian König, Sunil Khatri, Alex Deucher; +Cc: amd-gfx, Liang, Prike
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.
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--)
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] drm/amdgpu/userq: add mutex lock/unlocked version of amdgpu_userq_put
2026-03-20 10:02 ` Khatri, Sunil
@ 2026-03-20 10:13 ` Christian König
2026-03-20 10:53 ` Khatri, Sunil
0 siblings, 1 reply; 9+ messages in thread
From: Christian König @ 2026-03-20 10:13 UTC (permalink / raw)
To: Khatri, Sunil, Sunil Khatri, Alex Deucher; +Cc: amd-gfx, Liang, Prike
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?
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--)
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] drm/amdgpu/userq: add mutex lock/unlocked version of amdgpu_userq_put
2026-03-20 10:13 ` Christian König
@ 2026-03-20 10:53 ` Khatri, Sunil
2026-03-20 10:57 ` Christian König
0 siblings, 1 reply; 9+ messages in thread
From: Khatri, Sunil @ 2026-03-20 10:53 UTC (permalink / raw)
To: Christian König, Sunil Khatri, Alex Deucher; +Cc: amd-gfx, Liang, Prike
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 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--)
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] drm/amdgpu/userq: add mutex lock/unlocked version of amdgpu_userq_put
2026-03-20 10:53 ` Khatri, Sunil
@ 2026-03-20 10:57 ` Christian König
2026-03-20 11:14 ` Khatri, Sunil
0 siblings, 1 reply; 9+ messages in thread
From: Christian König @ 2026-03-20 10:57 UTC (permalink / raw)
To: Khatri, Sunil, Sunil Khatri, Alex Deucher; +Cc: amd-gfx, Liang, Prike
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.
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--)
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] drm/amdgpu/userq: add mutex lock/unlocked version of amdgpu_userq_put
2026-03-20 10:57 ` Christian König
@ 2026-03-20 11:14 ` Khatri, Sunil
2026-03-20 12:10 ` Khatri, Sunil
0 siblings, 1 reply; 9+ messages in thread
From: Khatri, Sunil @ 2026-03-20 11:14 UTC (permalink / raw)
To: Christian König, Sunil Khatri, Alex Deucher; +Cc: amd-gfx, Liang, Prike
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--)
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] drm/amdgpu/userq: add mutex lock/unlocked version of amdgpu_userq_put
2026-03-20 11:14 ` Khatri, Sunil
@ 2026-03-20 12:10 ` Khatri, Sunil
0 siblings, 0 replies; 9+ messages in thread
From: Khatri, Sunil @ 2026-03-20 12:10 UTC (permalink / raw)
To: Christian König, Sunil Khatri, Alex Deucher; +Cc: amd-gfx, Liang, Prike
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--)
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] drm/amdgpu/userq: add mutex lock/unlocked version of amdgpu_userq_put
@ 2026-03-20 9:43 Sunil Khatri
0 siblings, 0 replies; 9+ messages in thread
From: Sunil Khatri @ 2026-03-20 9:43 UTC (permalink / raw)
To: Alex Deucher, Christian König; +Cc: amd-gfx, Sunil Khatri, Prike Liang
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. 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: Prike Liang <Prike.Liang@amd.com>
Suggested-by: Prike Liang <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--)
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2026-03-20 12:11 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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
-- strict thread matches above, loose matches on Subject: below --
2026-03-20 9:43 Sunil Khatri
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox