Re: [PATCH 7/7] drm/xe: Check for GuC responses on disabling scheduling

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

On Fri, Oct 03, 2025 at 12:58:37PM -0600, Summers, Stuart wrote:
> On Fri, 2025-10-03 at 11:54 -0700, Matthew Brost wrote:
> > On Thu, Oct 02, 2025 at 11:04:44PM +0000, Stuart Summers wrote:
> > > In the event the GuC becomes unresponsive during a scheduling
> > > disable event, we still want the driver to be able to recover.
> > > This patch follows the same methodology we already have in place
> > > for TLB invalidation requests, where we send a request to GuC
> > > and wait for that invalidation done response. If the response
> > > doesn't come back in time we then at least print a message
> > > indicating the invalidation failed for some reason.
> > > 
> > > In this case, we send the schedule disable and the expectation
> > > is that GuC will respond with a schedule done response. The KMD
> > > then catches that response and in turn sends a context
> > > deregistration
> > > response. So in the event GuC becomes unresponsive after we send
> > > the schedule disable, we actually have two g2h responses that
> > > have been reserved but never received.
> > > 
> > > To handle this, make sure the pending disable event in the
> > > exec queue gets cleared (i.e. we received that response from
> > > GuC). If it doesn't in a reasonable amount of time, assume
> > > GuC is dead: ban the exec queue, queue up a GT reset, and
> > > manually call the schedule done handler. Then in the schedule
> > > done handler, in turn, check whether the context had been
> > > banned. If so, manually call the deregistration done handler
> > > to ensure all resources related to that exec queue get
> > > cleaned up properly. Without this, if the device becomes
> > > wedged after an exec queue has been created, the attached
> > > resources like the LRC will not get feed properly resulting
> > > in a memory leak.
> > > 
> > > Signed-off-by: Stuart Summers <stuart.summers@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_guc_submit.c | 23 ++++++++++++++++++++++-
> > >  1 file changed, 22 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c
> > > b/drivers/gpu/drm/xe/xe_guc_submit.c
> > > index 45b72bebfc63..a177d87c8524 100644
> > > --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> > > +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> > > @@ -939,6 +939,9 @@ int xe_guc_read_stopped(struct xe_guc *guc)
> > >                 GUC_CONTEXT_##enable_disable,                      
> > >      \
> > >         }
> > >  
> > > +static void handle_sched_done(struct xe_guc *guc, struct
> > > xe_exec_queue *q,
> > > +                             u32 runnable_state);
> > > +
> > >  static void disable_scheduling_deregister(struct xe_guc *guc,
> > >                                           struct xe_exec_queue *q)
> > >  {
> > > @@ -974,6 +977,17 @@ static void
> > > disable_scheduling_deregister(struct xe_guc *guc,
> > >         xe_guc_ct_send(&guc->ct, action, ARRAY_SIZE(action),
> > >                        G2H_LEN_DW_SCHED_CONTEXT_MODE_SET +
> > >                        G2H_LEN_DW_DEREGISTER_CONTEXT, 2);
> > > +
> > > +       ret = wait_event_timeout(guc->ct.wq,
> > > +                                !exec_queue_pending_disable(q) ||
> > > +                                xe_guc_read_stopped(guc),
> > > +                                HZ * 5);
> > 
> > This doesn't look right. Deregister is designed to be fully async. If
> > this flow stops working for whatever reason the GuC is dead and
> > eventually somewhere in driver will detect this and trigger a GT
> > reset
> > which is cleanup all lost H2G.
> > 
> > > +       if (!ret || xe_guc_read_stopped(guc)) {
> > > +               xe_gt_warn(guc_to_gt(guc), "Schedule disable failed
> > > to respond");
> > > +               set_exec_queue_banned(q);
> > > +               handle_sched_done(guc, q, 0);
> > > +               xe_gt_reset_async(q->gt);
> > > +       }
> > >  }
> > >  
> > >  static void xe_guc_exec_queue_trigger_cleanup(struct xe_exec_queue
> > > *q)
> > > @@ -2117,6 +2131,8 @@ g2h_exec_queue_lookup(struct xe_guc *guc, u32
> > > guc_id)
> > >         return q;
> > >  }
> > >  
> > > +static void handle_deregister_done(struct xe_guc *guc, struct
> > > xe_exec_queue *q);
> > > +
> > >  static void deregister_exec_queue(struct xe_guc *guc, struct
> > > xe_exec_queue *q)
> > >  {
> > >         u32 action[] = {
> > > @@ -2131,7 +2147,12 @@ static void deregister_exec_queue(struct
> > > xe_guc *guc, struct xe_exec_queue *q)
> > >  
> > >         trace_xe_exec_queue_deregister(q);
> > >  
> > > -       xe_guc_ct_send_g2h_handler(&guc->ct, action,
> > > ARRAY_SIZE(action));
> > > +       if (exec_queue_banned(q)) {
> > > +               handle_deregister_done(guc, q);
> > 
> > This would leave the GuC with reference to guc_id and subsequent
> > reuse
> > of the guc_id (i.e., next register) will fall.
> 
> But again, in this case the GuC is dead and we should be getting that
> reset event you had mentioned above. The issue I'm having is

Banned is a per thing queue and more than likely we wont be doing a GT
reset, thus we still need remove references to the queue from the GuC.

> specifically around wedge events. Without a GT wedge, we will normally
> go through the GT reset flow and recover like you mentioned. But in the

No. See above.

> case of a wedge, we don't redo the software part of the reset (i.e. we
> don't reset contexts, etc) per gt_reset():
> static int gt_reset(struct xe_gt *gt)
> {
>         unsigned int fw_ref;
>         int err;
> 
>         if (xe_device_wedged(gt_to_xe(gt)))
>                 return -ECANCELED;
> 
> Maybe instead of banned I can check for banned and wedged here? Or
> maybe we should rethink the software reset flow in the event of a
> wedge?

The idea with wedged is we leave all hardward state, including the GuC,
intacted for inspection. So I think a xe_device_wedged checked here
makes sense. This would cover the case where start a queue teardown via
a CLEANUP message and mid-flow we wedge the device.

Matt

> 
> Thanks,
> Stuart
> 
> > 
> > Matt
> > 
> > > +       } else {
> > > +               xe_guc_ct_send_g2h_handler(&guc->ct, action,
> > > +                                          ARRAY_SIZE(action));
> > > +       }
> > >  }
> > >  
> > >  static void handle_sched_done(struct xe_guc *guc, struct
> > > xe_exec_queue *q,
> > > -- 
> > > 2.34.1
> > > 
>