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

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

On Mon, 2025-10-13 at 19:09 -0700, Matthew Brost wrote:
> On Mon, Oct 13, 2025 at 10:31:35PM +0000, Stuart Summers wrote:
> > Currently if the GuC becomes unresponsive during a schedule
> > disable event, after we send the CT request, the driver
> > doesn't have a good way to recover. In most other cases,
> > we explicitly wait for GuC to respond by checking either
> > pending_enable, pending_disable, or some other state change
> > that we expect to be set after the response from GuC is
> > received for that particular request. Add a similar check
> > on the schedule disable side and make sure the queue state
> > for the queue being disabled is reset properly in that case.
> > 
> > v2: Only call the deregistration sequence since in this
> >     case the scheduling handler should be reset during
> >     the GT reset.
> >     By doing that, we don't need a way to track the scheduling
> >     disable request handler for that queue, making this sequence
> >     simpler. As a result, don't mark the queue as banned.
> > 
> > Signed-off-by: Stuart Summers <stuart.summers@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_guc_submit.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c
> > b/drivers/gpu/drm/xe/xe_guc_submit.c
> > index c923f13a13ef..ca37c7a8c5ed 100644
> > --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> > +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> > @@ -924,6 +924,8 @@ int xe_guc_read_stopped(struct xe_guc *guc)
> >                 GUC_CONTEXT_##enable_disable,                      
> >      \
> >         }
> >  
> > +static void handle_deregister_done(struct xe_guc *guc, struct
> > xe_exec_queue *q);
> > +
> >  static void disable_scheduling_deregister(struct xe_guc *guc,
> >                                           struct xe_exec_queue *q)
> >  {
> > @@ -961,6 +963,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);
> > +       if (!ret || xe_guc_read_stopped(guc)) {
> > +               xe_gt_warn(guc_to_gt(guc), "Schedule disable failed
> > to respond");
> > +               handle_deregister_done(guc, q);
> > +               xe_gt_reset_async(q->gt);
> > +       }
> > +
> 
> I thought I already stated this in eariler reviews - this path is
> designed to be fully async (i.e., no blocking here). If H2G get lost
> for
> whatever reason the driver should be smart enough to cleanup the
> memory
> references assicoiated for the lost H2G.

No you're right and I remembered your initial comment here. I was
thinking since we already have a wait earlier in this function and we
have similar waits for most of the other GuC communications in the
registration/submit code that having that here follows the same
thought. I get your point though. I'll have a more async focused
solution in the next rev.

Thanks for the feedback!

-Stuart

> 
> Matt
> 
> >  }
> >  
> >  static void xe_guc_exec_queue_trigger_cleanup(struct xe_exec_queue
> > *q)
> > -- 
> > 2.34.1
> >