Re: [PATCH 6/7] drm/xe: Clean up GuC software state after a wedge

[Matthew Brost <matthew.brost@intel.com>]

On Thu, Oct 23, 2025 at 11:43:34AM -0600, Summers, Stuart wrote:
> On Wed, 2025-10-22 at 14:15 -0700, Matthew Brost wrote:
> > On Mon, Oct 20, 2025 at 09:45:28PM +0000, Stuart Summers wrote:
> > > When the driver is wedged during a hardware failure, there
> > > is a chance the queue kill coming from those events can
> > > race with either the scheduler teardown or the queue
> > > deregistration with GuC. Basically the following two
> > > scenarios can occur (from event trace):
> > > 
> > > Scheduler start missing:
> > >   xe_exec_queue_create
> > 
> > The queues should be initialized in a started state unless a GT reset
> > or
> > VF migration is in progress. In both cases, upon successful
> > completion,
> > all queues will be restarted.
> > 
> > I did spot a bug in GT resets — if those fail, we don’t properly
> > restart
> > the queues. That should be fixed.
> > 
> > Also, I think xe_guc_declare_wedged is incorrect now that I’m looking
> > at
> > it.
> > 
> > It probably should be:
> > 
> > void xe_guc_declare_wedged(struct xe_guc *guc)
> > {
> >         xe_gt_assert(guc_to_gt(guc), guc_to_xe(guc)->wedged.mode);
> > 
> >         xe_guc_ct_stop(&guc->ct);
> >         xe_guc_submit_wedge(guc);
> >         xe_guc_sanitize(guc);
> > }
> 
> It's a good point, but to be clear, I'm not doing a GT reset here.
> Actually in the case I'm testing, there is an explicit PCIe FLR and
> then I'm explicitly wedging after and making sure the unbind completes
> error-free. Not something I'm necessarily planning on driving into the
> tree here, but doing this for some internal testing.
> 
> But yeah I'll give this a try with a GT reset in the mix to make sure
> that cleans up the way you're suggesting. And thanks for that wedge
> revision, I'll give that a try too.
> 
> > 
> > >   xe_exec_queue_kill
> > >   xe_guc_exec_queue_kill
> > >   xe_exec_queue_destroy
> > > 
> > > GuC CT response missing:
> > >   xe_exec_queue_create
> > >   xe_exec_queue_register
> > >   xe_exec_queue_scheduling_enable
> > >   xe_exec_queue_scheduling_done
> > >   xe_exec_queue_kill
> > >   xe_guc_exec_queue_kill
> > >   xe_exec_queue_close
> > >   xe_exec_queue_destroy
> > >   xe_exec_queue_cleanup_entity
> > >   xe_exec_queue_scheduling_disable
> > 
> > The ref count should be zero here — xe_exec_queue_scheduling_disable
> 
> I did confirm that it is (at least per the get_unless_zero() call
> below).
> 
> > is
> > called after this series [1]. I think we need to get this series in
> 
> Let me pull in that series. I'm still going through it on my side...
> thanks for the link!
> 
> > before making changes to the GuC submission state machine.
> > Technically,
> > all we need are the last three patches from that series, as they
> > simplify some things. I believe an upcoming Xe3 feature would also
> > benefit from getting these patches in too.
> > 
> > So that means in xe_guc_submit_wedge() the below if statement is
> > going
> > to fail.
> > 
> > 1006         mutex_lock(&guc->submission_state.lock);
> > 1007         xa_for_each(&guc->submission_state.exec_queue_lookup,
> > index, q)
> > 1008                 if (xe_exec_queue_get_unless_zero(q))
> > 1009                         set_exec_queue_wedged(q);
> > 1010         mutex_unlock(&guc->submission_state.lock);
> > 
> > I think we need...
> > 
> > else if (exec_queue_register(q)) 
> >         __guc_exec_queue_destroy(guc, q);
> 
> Right.. I remember you mentioning that also in a prior rev... let me
> confirm here. When I was testing, this wasn't working in all cases, but
> I'll double check and get back.
> 
> Also this was the point of the pending_disable here. We do explicitly
> set that in this flow whereas registered has a bunch of entry points
> and I was trying to isolate to the case of GuC dying mid-CT send. It
> seems to me if we have registered but not pending_disable, we have a
> bug in the sequence somewhere rather than an outside error injection
> (like GuC dying).
>

Part of my changes will tie the deregistration process directly to the
refcount — i.e., we deregister the process when the refcount reaches
zero. Previously, it was possible to deregister during TDRs, but that’s
no longer the case.

So, if we reach this point and the exec queue is present in the lookup
array with a refcount of zero, then either:

- The G2H for deregistration was lost as part of wedging, or
- The final worker for destruction is queued or currently running.

The exec_queue_registered flag will tell us whether the G2H was
processed, as we clear this bit upon receiving the G2H.

> > 
> > We also need to cleanup suspend fences too as those could be lost
> > under
> > the right race condition.
> > 
> > So prior to existing if statement, we also need:
> > 
> > if (q->guc->suspend_pending)
> >         suspend_fence_signal(q);
> 
> Ok
> 
> > 
> > [1] https://patchwork.freedesktop.org/series/155315/
> > 
> > > 
> > > The above traces depend also on inclusion of [1].
> > > 
> > > In the first scenario, the queue is created, but killed
> > > prior to completing the message cleanup. In the second,
> > > we go through a full registration before killing. The
> > > CT communication happens in that last call to
> > > xe_exec_queue_scheduling_disable.
> > > 
> > > We expect to then get a call to xe_guc_exec_queue_destroy
> > > in both cases if the aforementioned scheduler/GuC CT communication
> > > had happened, which we are missing here, hence missing any
> > > LRC/BO cleanup in the exec queues in question.
> > > 
> > > Since this sequence seems specific to the wedge case
> > > as described above, add a targeted scheduler start
> > > and guc deregistration handler to the wedged_fini()
> > > routine.
> > > 
> > > Without this change, if we inject wedges in the above scenarios
> > > we can expect the following when the DRM memory tracking is
> > > enabled (see CONFIG_DRM_DEBUG_MM):
> > > [  129.600285] [drm:drm_mm_takedown] *ERROR* node [00647000 +
> > > 00008000]: inserted at
> > >                 drm_mm_insert_node_in_range+0x2ec/0x4b0
> > >                 __xe_ggtt_insert_bo_at+0x10f/0x360 [xe]
> > >                 __xe_bo_create_locked+0x184/0x520 [xe]
> > >                 xe_bo_create_pin_map_at_aligned+0x3b/0x180 [xe]
> > >                 xe_bo_create_pin_map+0x13/0x20 [xe]
> > >                 xe_lrc_create+0x139/0x18e0 [xe]
> > >                 xe_exec_queue_create+0x22f/0x3e0 [xe]
> > >                 xe_exec_queue_create_ioctl+0x4e9/0xbf0 [xe]
> > >                 drm_ioctl_kernel+0x9f/0xf0
> > >                 drm_ioctl+0x20f/0x440
> > >                 xe_drm_ioctl+0x121/0x150 [xe]
> > >                 __x64_sys_ioctl+0x8c/0xe0
> > >                 do_syscall_64+0x4c/0x1d0
> > >                 entry_SYSCALL_64_after_hwframe+0x76/0x7e
> > > [  129.601966] [drm:drm_mm_takedown] *ERROR* node [0064f000 +
> > > 00008000]: inserted at
> > >                 drm_mm_insert_node_in_range+0x2ec/0x4b0
> > >                 __xe_ggtt_insert_bo_at+0x10f/0x360 [xe]
> > >                 __xe_bo_create_locked+0x184/0x520 [xe]
> > >                 xe_bo_create_pin_map_at_aligned+0x3b/0x180 [xe]
> > >                 xe_bo_create_pin_map+0x13/0x20 [xe]
> > >                 xe_lrc_create+0x139/0x18e0 [xe]
> > >                 xe_exec_queue_create+0x22f/0x3e0 [xe]
> > >                 xe_exec_queue_create_bind+0x7f/0xd0 [xe]
> > >                 xe_vm_create+0x4aa/0x8b0 [xe]
> > >                 xe_vm_create_ioctl+0x17b/0x420 [xe]
> > >                 drm_ioctl_kernel+0x9f/0xf0
> > >                 drm_ioctl+0x20f/0x440
> > >                 xe_drm_ioctl+0x121/0x150 [xe]
> > >                 __x64_sys_ioctl+0x8c/0xe0
> > >                 do_syscall_64+0x4c/0x1d0
> > >                 entry_SYSCALL_64_after_hwframe+0x76/0x7e
> > > 
> > > Signed-off-by: Stuart Summers <stuart.summers@intel.com>
> > > 
> > > [1]
> > > https://patchwork.freedesktop.org/patch/680852/?series=155352&rev=4
> > > ---
> > >  drivers/gpu/drm/xe/xe_guc_submit.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c
> > > b/drivers/gpu/drm/xe/xe_guc_submit.c
> > > index 5ec1e4a83d68..a11ae4e70809 100644
> > > --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> > > +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> > > @@ -287,6 +287,8 @@ static void guc_submit_fini(struct drm_device
> > > *drm, void *arg)
> > >         xa_destroy(&guc->submission_state.exec_queue_lookup);
> > >  }
> > >  
> > > +static void __guc_exec_queue_destroy(struct xe_guc *guc, struct
> > > xe_exec_queue *q);
> > > +
> > >  static void guc_submit_wedged_fini(void *arg)
> > >  {
> > >         struct xe_guc *guc = arg;
> > > @@ -299,6 +301,16 @@ static void guc_submit_wedged_fini(void *arg)
> > >                         mutex_unlock(&guc->submission_state.lock);
> > >                         xe_exec_queue_put(q);
> > >                         mutex_lock(&guc->submission_state.lock);
> > 
> > With everything above I don't think this new code below is needed.
> > 
> > But to make sure we know what we are doing, how about this from [2]
> > before the xe_exec_queue_put.
> > 
> > xe_gt_assert(..., !drm_sched_is_stopped(sched));
> 
> Yeah I agree this seems like a good idea. It also follows what we're
> doing in the other state changes.
> 
> > 
> > Wanna try out these suggestions? It is always possible I made a
> > mistake
> > here.
> 
> Really appreciate the feedback Matt. Yeah I'll take a look and get back
> if it still doesn't work here.
> 

Yea, the state machine is little hard to wrap your head around. Happy to
help.

Matt

> Thanks,
> Stuart
> 
> > 
> > Matt
> > 
> > [2]
> > https://patchwork.freedesktop.org/patch/681606/?series=155315&rev=3
> > 
> > > +               } else {
> > > +                       /*
> > > +                        * Make sure queues which were killed as
> > > part of a
> > > +                        * wedge are cleaned up properly. Clean up
> > > any
> > > +                        * dangling scheduler tasks and pending
> > > exec queue
> > > +                        * deregistration.
> > > +                        */
> > > +                       xe_sched_submission_start(&q->guc->sched);
> > > +                       if (exec_queue_pending_disable(q))
> > > +                               __guc_exec_queue_destroy(guc, q);
> > >                 }
> > >         }
> > >         mutex_unlock(&guc->submission_state.lock);
> > > -- 
> > > 2.34.1
> > > 
>