RE: [PATCH 1/2] drm/amdgpu: Remove separate guilty compute userq reset
"Liang, Prike" <[email protected]>
| Newsgroups | org.freedesktop.lists.amd-gfx |
|---|---|
| Message-ID | <DS7PR12MB60054E72D5D810485F15E949FBDA2@DS7PR12MB6005.namprd12.prod.outlook.com> |
AMD General
Regards,
Prike
> -----Original Message-----
> From: Alex Deucher <[email protected]>
> Sent: Wednesday, August 12, 2026 8:47 PM
> To: Liang, Prike <[email protected]>
> Cc: [email protected]; Zhang, Jesse(Jie) <[email protected]>;
> Liu, Shaoyun <[email protected]>; Deucher, Alexander
> <[email protected]>; Koenig, Christian <[email protected]>
> Subject: Re: [PATCH 1/2] drm/amdgpu: Remove separate guilty compute userq reset
>
> On Wed, Aug 12, 2026 at 5:18 AM Liang, Prike <[email protected]> wrote:
> >
> > AMD General
> >
> > Regards,
> > Prike
> >
> > > -----Original Message-----
> > > From: Alex Deucher <[email protected]>
> > > Sent: Friday, August 7, 2026 10:38 PM
> > > To: Liang, Prike <[email protected]>
> > > Cc: [email protected]; Zhang, Jesse(Jie)
> > > <[email protected]>; Liu, Shaoyun <[email protected]>; Deucher,
> > > Alexander <[email protected]>; Koenig, Christian
> > > <[email protected]>
> > > Subject: Re: [PATCH 1/2] drm/amdgpu: Remove separate guilty compute
> > > userq reset
> > >
> > > On Fri, Aug 7, 2026 at 10:11 AM Liang, Prike <[email protected]> wrote:
> > > >
> > > > AMD General
> > > >
> > > > Regards,
> > > > Prike
> > > >
> > > > > -----Original Message-----
> > > > > From: Alex Deucher <[email protected]>
> > > > > Sent: Friday, August 7, 2026 9:47 PM
> > > > > To: Liang, Prike <[email protected]>
> > > > > Cc: [email protected]; Zhang, Jesse(Jie)
> > > > > <[email protected]>; Liu, Shaoyun <[email protected]>;
> > > > > Deucher, Alexander <[email protected]>; Koenig,
> > > > > Christian <[email protected]>
> > > > > Subject: Re: [PATCH 1/2] drm/amdgpu: Remove separate guilty
> > > > > compute userq reset
> > > > >
> > > > > On Fri, Aug 7, 2026 at 9:39 AM Liang, Prike <[email protected]> wrote:
> > > > > >
> > > > > > AMD General
> > > > > >
> > > > > >
> > > > > >
> > > > > > Regards,
> > > > > > Prike
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Alex Deucher <[email protected]>
> > > > > > > Sent: Tuesday, August 4, 2026 10:18 PM
> > > > > > > To: Liang, Prike <[email protected]>
> > > > > > > Cc: [email protected]; Zhang, Jesse(Jie)
> > > > > > > <[email protected]>; Liu, Shaoyun <[email protected]>;
> > > > > > > Deucher, Alexander <[email protected]>; Koenig,
> > > > > > > Christian <[email protected]>
> > > > > > > Subject: Re: [PATCH 1/2] drm/amdgpu: Remove separate guilty
> > > > > > > compute userq reset
> > > > > > >
> > > > > > > On Tue, Aug 4, 2026 at 4:16 AM Liang, Prike
> > > > > > > <[email protected]>
> > > wrote:
> > > > > > > >
> > > > > > > > AMD General
> > > > > > > >
> > > > > > > > Regards,
> > > > > > > > Prike
> > > > > > > >
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: Alex Deucher <[email protected]>
> > > > > > > > > Sent: Monday, August 3, 2026 9:40 PM
> > > > > > > > > To: Liang, Prike <[email protected]>
> > > > > > > > > Cc: [email protected]; Zhang, Jesse(Jie)
> > > > > > > > > <[email protected]>; Liu, Shaoyun
> > > > > > > > > <[email protected]>; Deucher, Alexander
> > > > > > > > > <[email protected]>; Koenig, Christian
> > > > > > > > > <[email protected]>
> > > > > > > > > Subject: Re: [PATCH 1/2] drm/amdgpu: Remove separate
> > > > > > > > > guilty compute userq reset
> > > > > > > > >
> > > > > > > > > On Mon, Aug 3, 2026 at 9:36 AM Liang, Prike
> > > > > > > > > <[email protected]>
> > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > AMD General
> > > > > > > > > >
> > > > > > > > > > Regards,
> > > > > > > > > > Prike
> > > > > > > > > >
> > > > > > > > > > > -----Original Message-----
> > > > > > > > > > > From: Alex Deucher <[email protected]>
> > > > > > > > > > > Sent: Monday, August 3, 2026 9:23 PM
> > > > > > > > > > > To: Liang, Prike <[email protected]>
> > > > > > > > > > > Cc: [email protected]; Zhang, Jesse(Jie)
> > > > > > > > > > > <[email protected]>; Liu, Shaoyun
> > > > > > > > > > > <[email protected]>; Deucher, Alexander
> > > > > > > > > > > <[email protected]>; Koenig, Christian
> > > > > > > > > > > <[email protected]>
> > > > > > > > > > > Subject: Re: [PATCH 1/2] drm/amdgpu: Remove separate
> > > > > > > > > > > guilty compute userq reset
> > > > > > > > > > >
> > > > > > > > > > > On Mon, Aug 3, 2026 at 5:25 AM Liang, Prike
> > > > > > > > > > > <[email protected]>
> > > > > > > wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > AMD General
> > > > > > > > > > > >
> > > > > > > > > > > > As for the hung userq, it should be identified by
> > > > > > > > > > > > the MES reset API with the
> > > > > > > > > > > hang_detect_only setting. However, it is unlikely to
> > > > > > > > > > > miss detecting the invalid opcode hang case,
> > > > > > > > > > > especially given that the userq invalid opcode IGT
> > > > > > > > > > > test has not been
> > > implemented yet.
> > > > > > > > > > > >
> > > > > > > > > > > > Hi @Liu, Shaoyun, are you aware of any known userq
> > > > > > > > > > > > hang scenarios that cannot
> > > > > > > > > > > be identified by MES API hang_detect_only? If not,
> > > > > > > > > > > could you please help review the following patch,
> > > > > > > > > > > which unifies the userq reset path for hung
> > > > > > > > > queues?
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > We added it in the first place to deal with those cases.
> > > > > > > > > > > There can be queues which are not hung, but will
> > > > > > > > > > > never complete and hence never signal their fence.
> > > > > > > > > > > E.g., you can have a queue that is waiting on a
> > > > > > > > > > > memory location that MES can preempt, but due to a
> > > > > > > > > > > bug elsewhere that memory location will never change
> > > > > > > > > > > so the fence will
> > > > > > > never signal.
> > > > > > > > > >
> > > > > > > > > > Thank you for the input. However, for fake timeout
> > > > > > > > > > cases such as the long shader
> > > > > > > > > scenario, we should identify the fake hang cases by
> > > > > > > > > checking whether the guilty queue appears in the hang
> > > > > > > > > list, or whether the queue rptr is still updating? If
> > > > > > > > > so, preempt the queue rather than resetting
> > > > > it?
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > We have to assume that if we end up in the queue reset
> > > > > > > > > path that the queue is
> > > > > > > hung.
> > > > > > > > > The fences have to signal in finite time. If we preempt
> > > > > > > > > the queue that won't signal the fence so we'll just end
> > > > > > > > > up in the queue reset path again. Preemption of a queue
> > > > > > > > > that won't make progress only makes sense
> > > > > > > if fences are not involved.
> > > > > > > >
> > > > > > > > The most cases relevant to userq fence timeout and reset
> > > > > > > > worker likely to be
> > > > > > > scheduled when a userq fence polling period expires. If a
> > > > > > > long running shader is detected, the driver may need to try
> > > > > > > preempting the queue
> > > > > within a few retry cycles.
> > > > > > > If the userq fence remains unsignaled after the retries are
> > > > > > > exhausted, the driver can either return -ETIME to userspace
> > > > > > > for further handling or fall back to resetting the queue directly.
> > > > > > > Meanwhile, If the preemption succeeds and the queue
> > > > > > > completes its work during the subsequent restore process, no
> > > > > > > further reset is necessary for the
> > > > > guilty queue?
> > > > > > > >
> > > > > > >
> > > > > > > If an application wants to run long running jobs they
> > > > > > > shouldn't use protected fences in the first place. If they
> > > > > > > don't use protected fences, then it should behave like KFD
> > > > > > > queues. If there is some operation that needs to happen the
> > > > > > > queues will get preempted and then will continue later. DMA
> > > > > > > fences need to signal in finite time so we can't just keep pushing them
> off.
> > > > > > > Other kernel paging operations may
> > > > > depend on them signalling.
> > > > > >
> > > > > > Regarding the protected fences, do you mean the userq fence
> > > > > > that triggers the
> > > > > hang detection work queue?
> > > > > >
> > > > >
> > > > > Yes.
> > > > >
> > > > > > If so, the current implementation calls
> > > > > > amdgpu_userq_fence_init() at fence emit
> > > > > time (i.e., at each queue submission), which both initializes
> > > > > the dma_fence structure and starts the hang detection timer for
> > > > > that submission. Once the timeout expires,
> > > > > amdgpu_userq_hang_detect_work() fires and triggers a queue reset
> > > > > regardless of whether the queue is actually hung or simply
> > > > > long-running. But there's a core problem for the kernel to
> > > > > distinguish a hung queue from a slow one at fence emit time,
> > > > > since both look
> > > identical an unsignaled fence with an active queue.
> > > > > >
> > > > >
> > > > > That is the intended behavior. If you opt into using a dma
> > > > > fence, you need to abide by its semantics.
> > > > >
> > > > > > However, we can defer that determination to the first timeout
> > > > > > expiry in
> > > > > amdgpu_userq_hang_detect_work(). At that point we have more
> > > > > information to work
> > > > > with:
> > > > > >
> > > > > >
> > > > > > Check for real hang: query rptr and rptr has not updated since
> > > > > > the fence was
> > > > > emitted, the queue is genuinely stuck (frozen waves, infinite loop).
> > > > > This is a real hang, reset immediately and forcing completing the fences.
> > > > > > Check for forward progress: if rptr has updated but the fence
> > > > > > is still unsignaled, the
> > > > > queue is making progress, so it is long running, not hung. In this case:
> > > > > >
> > > > > > Extend the timeout (reschedule hang_detect_work with a longer
> > > > > > interval) Track retry count if the queue still has not
> > > > > > completed after N extended
> > > > > cycles, escalate: Preempt the queue, force completing the fence
> > > > > and return -ETIME to userspace then let the application decide:
> > > > > resubmit, split the
> > > workload, or drop.
> > > > > >
> > > > > > Avoid unnecessary resets: if preemption succeeds and the queue
> > > > > > resumes and
> > > > > completes within the extended window, cancel the reset entirely
> > > > > since the queue was slow, not broken.
> > > > > >
> > > > > >
> > > > > > Do you think this is the right way for handling long-running
> > > > > > tasks to resolve the
> > > > > userq timeout issue, or does it align with your idea?
> > > > >
> > > > > If you want to run a super long running job, then you shouldn't
> > > > > use a dma fence in the first place. There might be a job that
> > > > > takes 2 minutes or even an hour to run, but you can't wait that
> > > > > long for a dma fence. If you do, the kernel might run out of
> > > > > memory and the OOM killer will start killing processes or you
> > > > > could deadlock. If you want to run a long job, don't use a dma
> > > > > fence. If you don't use a dma fence, then the kernel can
> > > > > preempt the queues, do whatever memory
> > > management it needs to and then it can resume the queues. dma
> > > fences have to signal in finite time.
> > > >
> > > > Got it. So to skip attaching a dma_fence to a long-running
> > > > submission, does this
> > > require an explicit flag from userspace at queue creation, or is
> > > there another mechanism to identify it on the kernel side?
> > > >
> > >
> > > Userspace would simply not call the userq signal IOCTL if they want
> > > to run long running jobs.
> >
> > Yes, this approach can also handle long-running jobs submitted from userspace.
> However, after revisiting the Mesa code, it appears that a fence is always emitted for
> each userq submission, and I could not find any code for identifying long-running
> submissions. How should long-running submissions be identified in userspace?
>
> Userspace should not use protected fences in that case. In theory compositors that
> use explicit sync do not need to use the protected fence. Someone just needs to
> handle this case in mesa.
Understanding, I will create a new thread for tracking the long running submission work item.
> >
> > If a long-running submission does not use a protected fence, hang_detect_work
> should not be scheduled for that submission, and this cleanup patch should not
> affect the corresponding long-running userq. This should therefore be treated as a
> separate case requiring further investigation, including how to identify long-running
> userq submissions and avoid attaching protected fences to them.
> >
>
> All we need to do is check for the presence of a fence on the queue (the application
> would have called the fence IOCTL). If the fence is present, then we'd reset the
> queue. If no fence, and MES hang_detect doesn't detect the queue as hung, then
> there is no need to reset it.
>
> > Regarding the cleanup, if all hung user queues can be identified through
> hang_detect_only, do we still need to handle the guilty userq separately? Could we
> simply reset all guilty queues by traversing the list of hung doorbells, as proposed in
> this cleanup patch?
>
> If there are no protected fences, then yes, all we need is MES hang detect.
Per this cleanup for the guilty reset handling, may I have a review or ack for those clean up series?
> Alex
>
> >
> > Regards,
> > Prike
> >
> > > Alex
> > >
> > > > Thanks,
> > > > Prike
> > > > > Alex
> > > > >
> > > > > >
> > > > > > Thanks,
> > > > > > Prike
> > > > > >
> > > > > > Avoid unnecessary resets: if preemption succeeds and the queue
> > > > > > resumes and completes within the extended window, cancel the
> > > > > > reset entirely — the queue was slow, not broken.>
> > > > > > > Alex
> > > > > > >
> > > > > > > > In the longer term, we may need to introduce a more robust
> > > > > > > > mechanism to
> > > > > > > distinguish slow queue cases from genuine hangs, handling
> > > > > > > them appropriately via extending time slice for completing
> > > > > > > the queue submission, userspace driven resubmission, or
> > > > > > > dropping the work through a queue reset. If it makes sense
> > > > > > > and right way to do, then I will work on implementing the
> > > > > > > solution for such slow/long queue
> > > cases.
> > > > > > > >
> > > > > > > > Thanks,
> > > > > > > > Prike
> > > > > > > >
> > > > > > > > > Alex
> > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Alex
> > > > > > > > > > >
> > > > > > > > > > > > Regards,
> > > > > > > > > > > > Prike
> > > > > > > > > > > >
> > > > > > > > > > > > > -----Original Message-----
> > > > > > > > > > > > > From: Liang, Prike
> > > > > > > > > > > > > Sent: Thursday, July 23, 2026 2:38 PM
> > > > > > > > > > > > > To: [email protected]; Zhang,
> > > > > > > > > > > > > Jesse(Jie) <[email protected]>
> > > > > > > > > > > > > Cc: Deucher, Alexander
> > > > > > > > > > > > > <[email protected]>; Koenig, Christian
> > > > > > > > > > > > > <[email protected]>
> > > > > > > > > > > > > Subject: RE: [PATCH 1/2] drm/amdgpu: Remove
> > > > > > > > > > > > > separate guilty compute userq reset
> > > > > > > > > > > > >
> > > > > > > > > > > > > I checked each different userq hang cases, and
> > > > > > > > > > > > > the guilty userq can be identified by the MES
> > > > > > > > > > > > > firmware and report
> > > > > correctly.
> > > > > > > > > > > > > @Zhang,
> > > > > > > > > > > > > Jesse(Jie) could you further check as well at you side?
> > > > > > > > > > > > >
> > > > > > > > > > > > > If there're some hang queues miss identified by
> > > > > > > > > > > > > MES firmware, then the correct thing is to
> > > > > > > > > > > > > further debug from MES firmware side rather than
> > > > > > > > > > > > > have such strange reset sequence and this reset
> > > > > > > > > > > > > workaround should be
> > > > > > > > > > > cleaned sooner or later.
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > Regards,
> > > > > > > > > > > > > Prike
> > > > > > > > > > > > >
> > > > > > > > > > > > > > -----Original Message-----
> > > > > > > > > > > > > > From: Liang, Prike <[email protected]>
> > > > > > > > > > > > > > Sent: Wednesday, July 15, 2026 2:31 PM
> > > > > > > > > > > > > > To: [email protected]
> > > > > > > > > > > > > > Cc: Deucher, Alexander
> > > > > > > > > > > > > > <[email protected]>; Koenig, Christian
> > > > > > > > > > > > > > <[email protected]>; Liang, Prike
> > > > > > > > > > > > > > <[email protected]>
> > > > > > > > > > > > > > Subject: [PATCH 1/2] drm/amdgpu: Remove
> > > > > > > > > > > > > > separate guilty compute userq reset
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > amdgpu_mes_detect_and_reset_hung_queues()
> > > > > > > > > > > > > > already detects the guilty compute user queue
> > > > > > > > > > > > > > and resets it through mes_userq_reset_queue().
> > > > > > > > > > > > > > The additional reset via
> > > > > > > > > > > > > > mes_userq_reset() is unnecessary, so remove it
> > > > > > > > > > > > > > to unify the compute userq
> > > > > > > > > > > reset.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Signed-off-by: Prike Liang
> > > > > > > > > > > > > > <[email protected]>
> > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 5 -----
> > > > > > > > > > > > > > drivers/gpu/drm/amd/amdgpu/mes_userqueue.c |
> > > > > > > > > > > > > > 2 --
> > > > > > > > > > > > > > 2 files changed, 7 deletions(-)
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > diff --git
> > > > > > > > > > > > > > a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> > > > > > > > > > > > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> > > > > > > > > > > > > > index 1e275c2e7dd3..4f2d5ff2f7be 100644
> > > > > > > > > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> > > > > > > > > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> > > > > > > > > > > > > > @@ -2315,11 +2315,6 @@ int
> > > > > > > > > > > > > > amdgpu_gfx_reset_mes_compute(struct
> > > > > > > > > > > > > > amdgpu_device *adev,
> > > > > > > > > > > > > > deferred_end[n_deferred].fence = guilty_fence;
> > > > > > > > > > > > > > n_deferred++;
> > > > > > > > > > > > > > }
> > > > > > > > > > > > > > - if (uq) {
> > > > > > > > > > > > > > - r = mes_userq_reset(uq);
> > > > > > > > > > > > > > - if (r)
> > > > > > > > > > > > > > - goto out;
> > > > > > > > > > > > > > - }
> > > > > > > > > > > > > > for (i = 0; i < num_hung; i++) {
> > > > > > > > > > > > > > struct amdgpu_ring *hr = NULL;
> > > > > > > > > > > > > > struct amdgpu_fence *hf = NULL;
> > > > > > > > > > > > > > diff --git
> > > > > > > > > > > > > > a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> > > > > > > > > > > > > > b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> > > > > > > > > > > > > > index b6bfa3974839..fab21d4275f3 100644
> > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> > > > > > > > > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/mes_userqueue
> > > > > > > > > > > > > > +++ .c
> > > > > > > > > > > > > > @@ -226,8 +226,6 @@ int
> > > > > > > > > > > > > > mes_userq_reset_queue(struct amdgpu_device
> > > > > > > > > > > > > > *adev,
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > xa_for_each(&adev->userq_doorbell_xa, uq_id, uq) {
> > > > > > > > > > > > > > if (uq->queue_type == queue_type) {
> > > > > > > > > > > > > > - if (uq == guilty_uq)
> > > > > > > > > > > > > > - continue;
> > > > > > > > > > > > > > if (uq->doorbell_index == db) {
> > > > > > > > > > > > > > uq->state =
> > > AMDGPU_USERQ_STATE_HUNG;
> > > > > > > > > > > > > > if (use_mmio)
> > > > > > > > > > > > > > --
> > > > > > > > > > > > > > 2.34.1
> > > > > > > > > > > >