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.
