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

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

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