Re: [PATCH 02/11] drm/amdgpu: remove deadlocks from amdgpu_userq_pre_reset

["Khatri, Sunil" <sukhatri@amd.com>]

On 22-04-2026 12:54 pm, Christian König wrote:
> On 4/22/26 09:19, Khatri, Sunil wrote:
>> On 22-04-2026 12:43 pm, Christian König wrote:
>>> On 4/22/26 06:53, Khatri, Sunil wrote:
>>>> On 21-04-2026 06:25 pm, Christian König wrote:
>>>>> The purpose of a GPU reset is to make sure that fence can be signaled
>>>>> again and the signal and resume workers can make progress again.
>>>>>
>>>>> So waiting for the resume worker or any fence in the GPU reset path is
>>>>> just utterly nonsense.
>>>>>
>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>> ---
>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 26 +++++++++++------------
>>>>>   1 file changed, 12 insertions(+), 14 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>>>> index 8f48520cb822..b632bc3c952b 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>>>> @@ -1496,23 +1496,21 @@ void amdgpu_userq_pre_reset(struct amdgpu_device *adev)
>>>>>   {
>>>>>   	const struct amdgpu_userq_funcs *userq_funcs;
>>>>>   	struct amdgpu_usermode_queue *queue;
>>>>> -	struct amdgpu_userq_mgr *uqm;
>>>>>   	unsigned long queue_id;
>>>>>   
>>>>> +	/* TODO: We probably need a new lock for the queue state */
>>>>>   	xa_for_each(&adev->userq_doorbell_xa, queue_id, queue) {
>>>>> -		uqm = queue->userq_mgr;
>>>>> -		cancel_delayed_work_sync(&uqm->resume_work);
>>>>> -		if (queue->state == AMDGPU_USERQ_STATE_MAPPED) {
>>>>> -			amdgpu_userq_wait_for_last_fence(queue);
>>>>> -			userq_funcs = adev->userq_funcs[queue->queue_type];
>>>>> -			userq_funcs->unmap(queue);
>>>>> -			/* just mark all queues as hung at this point.
>>>>> -			 * if unmap succeeds, we could map again
>>>>> -			 * in amdgpu_userq_post_reset() if vram is not lost
>>>>> -			 */
>>>>> -			queue->state = AMDGPU_USERQ_STATE_HUNG;
>>>>> -			amdgpu_userq_fence_driver_force_completion(queue);
>>>>> -		}
>>>>> +		if (queue->state != AMDGPU_USERQ_STATE_MAPPED)
>>>>> +			continue;
>>>> If the queue is in prempt state and if at that time we are in this function we should still be doing force completion for work in those queue else the waiters will keep waiting.
>>>>
>>>>> +
>>>>> +		userq_funcs = adev->userq_funcs[queue->queue_type];
>>>>> +		userq_funcs->unmap(queue);
>>>> GPU is already hung if we are here and observation is we are unable to unmap as we have tried to reset via the fw and that failed to atleast thats what i have seen. Could we skip unmap ???
>>>>> +		/* just mark all queues as hung at this point.
>>>>> +		 * if unmap succeeds, we could map again
>>>>> +		 * in amdgpu_userq_post_reset() if vram is not lost
>>>>> +		 */
>>>>> +		queue->state = AMDGPU_USERQ_STATE_HUNG;
>>>>> +		amdgpu_userq_fence_driver_force_completion(queue);
>>>> we should be calling completion irrespective of queue state here. The GPU atleast the queue is hung and fw has failed to reset. We have to release the fences by foce completion.
>>> Yeah, I agree completely with those require comments I has similar thoughts while going over this.
>>>
>>> I'm only driven by bugs here and my goal with the patch was to remove the obvious deadlock in the function.
>>>
>>> Somebody needs to sit down and go over the whole handling for GPU resets and make sure it plays nicely with user queues.
>>>
>>> Regards,
>>> Christian.
>> For now this is what i think we could do.
>> In the loop: If a queue is mapped -> try unmap
>>
>> Irrespective of the queue state:
>> Set queue to HUNG and call amdgpu_userq_fence_driver_force_completion
> How do we protect the queue state? We don't have a lock for that and in a GPU reset you can't grab things like the userq_lock.

It would be best and i thought of this too earlier to have a per queue 
or per queue manager lock and which is for setting up the state of 
queue...That will help a lot too in the all the hw function like prempt 
resume etc that we call as many times lock is only needed for state 
change than anything else... If you want i can look into that later once 
these patches land up or you decide if you want to fix it now.

Regards
Sunil Khatri

>
> Could we call unmap while holding the doorbell XA lock or would that clash with something?
>
> Regards,
> Christian.
>
>> Regards
>> Sunil Khatri
>>>> Regards
>>>> Sunil Khatri
>>>>
>>>>>   	}
>>>>>   }
>>>>>