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

["Summers, Stuart" <stuart.summers@intel.com>]

On Wed, 2025-10-15 at 20:42 -0700, Matthew Brost wrote:
> On Wed, Oct 15, 2025 at 01:45:35PM -0600, Summers, Stuart wrote:
> > On Tue, 2025-10-14 at 18:09 +0000, Stuart Summers wrote:
> > > Comments also added to the code, but in the event of
> > > a wedge or a hardware failure while communication with
> > > GuC is outstanding (e.g. during a schedule disable or
> > > context deregistration), the driver doesn't automatically
> > > reset the software state as it would in a typical GT reset
> > > since we are trying to save the state for debug. However
> > > once the user unbinds the driver we still need to go through
> > > and clean everything up for these exec queues so we don't
> > > leak memory on the DRM side (e.g. LRC or LRC BO).
> > > 
> > > Add a kick start to the DRM scheduler to handle any outstanding
> > > messages on hold during the wedge and go through the GuC stop
> > > flow to simulate that reset on teardown.
> > > 
> > > Signed-off-by: Stuart Summers <stuart.summers@intel.com>
> > 
> > Please hold on review here. I think there's still something missing
> > on
> > the wedge cleanup case that causes issues after wedging and then
> > doing
> > binds/unbinds in a loop after. I'm working to resolve that and I'll
> > post a new series after.
> > 
> 
> Sure. Just couple of thoughts below which might help.
> 
> > Thanks,
> > Stuart
> > 
> > > ---
> > >  drivers/gpu/drm/xe/xe_guc_submit.c | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c
> > > b/drivers/gpu/drm/xe/xe_guc_submit.c
> > > index 5ec1e4a83d68..0bbae336c722 100644
> > > --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> > > +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> > > @@ -276,6 +276,14 @@ static void guc_submit_fini(struct
> > > drm_device
> > > *drm, void *arg)
> > >         struct xe_gt *gt = guc_to_gt(guc);
> > >         int ret;
> > >  
> > > +       /*
> > > +        * If GuC stopped responding during deregistration
> > > +        * some queues can be left in a bad state. Ensure
> > > +        * these are all cleaned up by going through the
> > > +        * GuC software reset flow.
> > > +        */
> > > +       xe_guc_stop(guc);
> > > +
> 
> I think we'd need to restart here at minimuum after cleanup.

Yeah this was one of the key issues, just trying to find the right
pieces to insert here..

> 
> > >         ret = wait_event_timeout(guc->submission_state.fini_wq,
> > >                                  xa_empty(&guc-
> > > > submission_state.exec_queue_lookup),
> > >                                  HZ * 5);
> > > @@ -295,6 +303,13 @@ static void guc_submit_wedged_fini(void
> > > *arg)
> > >  
> > >         mutex_lock(&guc->submission_state.lock);
> > >         xa_for_each(&guc->submission_state.exec_queue_lookup,
> > > index,
> > > q) {
> > > +               /*
> > > +                * Kick start the scheduler since some messages
> > > +                * might have been added while the scheduler was
> > > +                * stopped during a wedge event.
> > > +                */
> > > +               xe_sched_submission_start(&q->guc->sched);
> > > +
> 
> I don't think possible to get here with a stopped queue, I think it a
> bug elsewhere if we do.

Hm.. so this would either be we went through a GT reset prior to the
wedge (which should have restarted at the end of that) - which isn't
the case here - or the job timed out and stopped the scheduler for that
queue. Seems like there might be a bug in that path...

Thanks,
Stuart

> 
> Matt
> 
> > >                 if (exec_queue_wedged(q)) {
> > >                         mutex_unlock(&guc-
> > > >submission_state.lock);
> > >                         xe_exec_queue_put(q);
> >