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 01:42:26PM -0600, Summers, Stuart wrote:
> On Fri, 2025-10-03 at 12:38 -0700, Matthew Brost wrote:
> > 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.
> 
> Yeah this makes sense to me. My use of "banned" here was probably not
> ideal.
> 
> > 
> > > 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.
> 
> But hardware state doesn't mean software state. Are you saying when the
> device is wedged we want the memory to all be intact as well? And how
> do we determine when that gets freed? On unbind?

When we wedge a device we take a reference to all queues which perserves
the software queue state and also prevents any queue cleanup flows
regardless of what a user application does. On driver unbind after wedge
we drop wedge reference which should free all memory in the KMD.

Matt

> 
> Thanks,
> Stuart
> 
> > 
> > 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
> > > > > 
> > > 
>