Re: [PATCH v9 5/5] drm/amdgpu: Record QUEUE_RESET WAIT_EVENT notifications

On 7/16/26 14:53, SHANMUGAM, SRINIVASAN wrote:
> AMD General
> 
>> -----Original Message-----
>> From: SHANMUGAM, SRINIVASAN
>> Sent: Tuesday, July 14, 2026 7:43 PM
>> To: Koenig, Christian <[email protected]>; Deucher, Alexander
>> <[email protected]>
>> Cc: [email protected]
>> Subject: RE: [PATCH v9 5/5] drm/amdgpu: Record QUEUE_RESET WAIT_EVENT
>> notifications
>>
>>
>>
>>> -----Original Message-----
>>> From: Koenig, Christian <[email protected]>
>>> Sent: Tuesday, July 14, 2026 6:36 PM
>>> To: SHANMUGAM, SRINIVASAN <[email protected]>;
>> Deucher,
>>> Alexander <[email protected]>
>>> Cc: [email protected]
>>> Subject: Re: [PATCH v9 5/5] drm/amdgpu: Record QUEUE_RESET
>> WAIT_EVENT
>>> notifications
>>>
>>> On 7/14/26 10:10, Srinivasan Shanmugam wrote:
>>>> The queue reset path already resolves the affected queue directly
>>>> from the doorbell and funnels successful reset handling through
>>>> amdgpu_userq_handle_hung_queue(). Reuse that common handling point
>>>> to create the corresponding WAIT_EVENT record.
>>>>
>>>> Keeping the notification in the common helper ensures that queue
>>>> state is updated before userspace is notified and that both EVENTFD
>>>> and WAIT_EVENT observe the same reset. It also avoids duplicating
>>>> the doorbell lookup or adding a queue scan in the MES reset path.
>>>>
>>>> No MES or GFX interrupt changes are needed.
>>>>
>>>> Signed-off-by: Srinivasan Shanmugam <[email protected]>
>>>> Reviewed-by: Alex Deucher <[email protected]>
>>>> ---
>>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 6 ++++++
>>>>  1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>>> index 28f9d3fe0e80..9e1d03eb31d7 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>>> @@ -166,6 +166,7 @@ void amdgpu_userq_handle_hung_queue(struct
>>> amdgpu_device *adev,
>>>>                               struct amdgpu_usermode_queue *queue)  {
>>>>   struct amdgpu_eventfd_mgr *eventfd_mgr;
>>>> + struct amdgpu_wait_event_mgr *wait_event_mgr;
>>>>
>>>>   queue->state = AMDGPU_USERQ_STATE_HUNG;
>>>>   atomic_inc(&adev->gpu_reset_counter);
>>>> @@ -176,6 +177,11 @@ void amdgpu_userq_handle_hung_queue(struct
>>> amdgpu_device *adev,
>>>>   amdgpu_eventfd_signal(eventfd_mgr,
>>>>                         DRM_AMDGPU_EVENT_TYPE_QUEUE_RESET,
>>>>                         queue);
>>>> +
>>>> + wait_event_mgr = amdgpu_userq_wait_event_mgr(queue->userq_mgr);
>>>> + amdgpu_wait_event_add(wait_event_mgr,
>>>> +                       DRM_AMDGPU_EVENT_TYPE_QUEUE_RESET,
>>>> +                       queue);
>>>
>>> How are we making sure here that this wait_event_mgr doesn't go out of scope?
>>
>> Good point. wait_event_mgr is part of the owning amdgpu_fpriv, not the queue itself.
>> In the reset path, we currently take a reference to the queue, which guarantees that
>> the queue remains valid while we handle the reset.
>> However, that reference does not explicitly guarantee that the owning amdgpu_fpriv
>> (and therefore wait_event_mgr) is still alive.
>> I'll review the lifetime rules and teardown sequence to make sure the manager
>> cannot be freed while a reset notification is still in progress.
>> If additional synchronization or a reference is needed, I'll include that in the next
>> revision.
> 
> Hi Christian,
> 
> Thanks for pointing this out.
> 
> I reviewed the lifetime and teardown sequence in more detail. wait_event_mgr is part of the owning amdgpu_fpriv, so it is not allocated or freed independently. Its lifetime is tied to the corresponding drm_file.
> 
> During file teardown, the USERQ manager and queues are torn down before amdgpu_wait_event_mgr_fini() is called. Queue teardown removes the producer lookup paths, and amdgpu_wait_event_mgr_fini() marks the manager dead, removes pending records, releases their queue references, and wakes blocked waiters before amdgpu_fpriv is freed.
> 
> Based on this teardown ordering, the producer should no longer be able to access wait_event_mgr after it is finalized, so an additional manager reference does not appear to be necessary.
> 
> I've also documented this ownership and teardown ordering in Patch 2 ("Add wait-event manager and per-file lifetime plumbing") in https://patchwork.freedesktop.org/patch/740465/?series=164813&rev=11

Sounds great, feel free to add Reviewed-by: Christian König <[email protected]> to the patch set as well.

Thanks,
Christian.

> 
> Thanks,
> Srini
>>
>> Thanks,
>> Srini
>>
>>>
>>> Regards,
>>> Christian.