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

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

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?

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
> > > > 
> >