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

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

On Thu, 2025-10-16 at 15:10 -0700, Matthew Brost wrote:
> On Thu, Oct 16, 2025 at 11:50:44AM -0600, Summers, Stuart wrote:
> > 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...
> > 
> 
> The job timeout mechanism should always restart the queue. I just
> checked, and it looks like there's a bug in the vf_recovery flows,
> but
> that shouldn't affect this case. I did review the GT reset path, and
> if
> that fails after xe_uc_stop is called, the queues are not
> restarted—which is a problem. We likely need to call something like
> xe_guc_submit_pause_abort (or a GT-layer equivalent) to restart the
> queues. If we don’t it could lead to memory-unsafe behavior as things
> will not get freed. This call should happen after wedging the device.

We should not be calling uc_stop() though if we're in a wedge state.
The uc_stop_prepare() and uc_stop() are part of the software reset that
is skipped when we're wedged. In the case here, I'm explicitly stopping
to try to reset the queue in the patch here and that's why it needs to
be restarted. The stop was intended to fix one bug and the restart
after stop fixes a second, but it still isn't the right fix as
discussed.

Thanks,
Stuart

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