Re: [PATCH v4 10/12] drm/xe: Add exec queue active vfunc

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

On Tue, 2026-01-13 at 14:16 -0800, Matthew Brost wrote:
> On Tue, Jan 13, 2026 at 02:32:58PM -0700, Summers, Stuart wrote:
> > On Mon, 2026-01-12 at 18:52 -0800, Matthew Brost wrote:
> > > If an exec queue is inactive (e.g., not registered or scheduling
> > > is
> > > disabled), TLB invalidations are not issued for that queue. Add a
> > > virtual function to determine the active state, which TLB
> > > invalidation
> > > logic can hook into.
> > > 
> > > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_exec_queue_types.h | 2 ++
> > >  drivers/gpu/drm/xe/xe_execlist.c         | 7 +++++++
> > >  drivers/gpu/drm/xe/xe_guc_submit.c       | 6 ++++++
> > >  3 files changed, 15 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_exec_queue_types.h
> > > b/drivers/gpu/drm/xe/xe_exec_queue_types.h
> > > index d3e2789cf5bc..9cca558c5809 100644
> > > --- a/drivers/gpu/drm/xe/xe_exec_queue_types.h
> > > +++ b/drivers/gpu/drm/xe/xe_exec_queue_types.h
> > > @@ -298,6 +298,8 @@ struct xe_exec_queue_ops {
> > >         void (*resume)(struct xe_exec_queue *q);
> > >         /** @reset_status: check exec queue reset status */
> > >         bool (*reset_status)(struct xe_exec_queue *q);
> > > +       /** @active: check exec queue is active */
> > > +       bool (*active)(struct xe_exec_queue *q);
> > >  };
> > >  
> > >  #endif
> > > diff --git a/drivers/gpu/drm/xe/xe_execlist.c
> > > b/drivers/gpu/drm/xe/xe_execlist.c
> > > index 46c17a18a3f4..e4a2674f48c7 100644
> > > --- a/drivers/gpu/drm/xe/xe_execlist.c
> > > +++ b/drivers/gpu/drm/xe/xe_execlist.c
> > > @@ -469,6 +469,12 @@ static bool
> > > execlist_exec_queue_reset_status(struct xe_exec_queue *q)
> > >         return false;
> > >  }
> > >  
> > > +static bool execlist_exec_queue_active(struct xe_exec_queue *q)
> > > +{
> > > +       /* NIY */
> > > +       return false;
> > > +}
> > > +
> > >  static const struct xe_exec_queue_ops execlist_exec_queue_ops =
> > > {
> > >         .init = execlist_exec_queue_init,
> > >         .kill = execlist_exec_queue_kill,
> > > @@ -481,6 +487,7 @@ static const struct xe_exec_queue_ops
> > > execlist_exec_queue_ops = {
> > >         .suspend_wait = execlist_exec_queue_suspend_wait,
> > >         .resume = execlist_exec_queue_resume,
> > >         .reset_status = execlist_exec_queue_reset_status,
> > > +       .active = execlist_exec_queue_active,
> > >  };
> > >  
> > >  int xe_execlist_init(struct xe_gt *gt)
> > > diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c
> > > b/drivers/gpu/drm/xe/xe_guc_submit.c
> > > index be8fa76baf1d..19282e5ca897 100644
> > > --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> > > +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> > > @@ -2203,6 +2203,11 @@ static bool
> > > guc_exec_queue_reset_status(struct
> > > xe_exec_queue *q)
> > >         return exec_queue_reset(q) ||
> > > exec_queue_killed_or_banned_or_wedged(q);
> > >  }
> > >  
> > > +static bool guc_exec_queue_active(struct xe_exec_queue *q)
> > > +{
> > > +       return exec_queue_enabled(q) &&
> > > !exec_queue_pending_disable(q);
> > 
> > So I think the requirement here is !exec_queue_pending_disable &&
> > !exec_queue_destroyed rather than exec_queue_enabled... But if the
> 
> If destroyed is set, the refcounting exchange in the following patch
> will fail, so exec_queue_destroyed check here isn't needed.

Ok. Still looking over that other comment but it looks promising...

> 
> > context isn't enabled (set runnable to true in GuC), it means
> > hardware
> > shouldn't be touching any of the associated memory anyway and we
> > shouldn't be issuing any invalidations, so this still looks ok to
> > me.
> > It means we'll be doing full invalidations for those I think though
> > which could add some performance impact in the time between
> > register
> > and enable. For now though as long as this is working (still doing
> > some
> > testing on my end and I had some questions in a later patch), we
> > can go
> > with this.
> 
> My reasoning here is that the context is not enabled, so we don’t
> need
> to issue an invalidation because the next context switch to the
> hardware
> will issue one—that’s the current hardware behavior. If that changes,
> this could simply become an exec_queue_registered check. I forget if
> this is changing in future hardware.

Ok... yeah I guess we can address this if it comes up. I was more
worried about this falling back to the full invalidation if the list
was empty here.

> 
> I also noticed I have a multi-queue bug here too. This needs to check
> the primary queue’s state for multi-queue.

Ah true, good catch...

Thanks,
Stuart

> 
> Matt
> 
> > 
> > Thanks,
> > Stuart
> > 
> > > +}
> > > +
> > >  /*
> > >   * All of these functions are an abstraction layer which other
> > > parts
> > > of Xe can
> > >   * use to trap into the GuC backend. All of these functions,
> > > aside
> > > from init,
> > > @@ -2222,6 +2227,7 @@ static const struct xe_exec_queue_ops
> > > guc_exec_queue_ops = {
> > >         .suspend_wait = guc_exec_queue_suspend_wait,
> > >         .resume = guc_exec_queue_resume,
> > >         .reset_status = guc_exec_queue_reset_status,
> > > +       .active = guc_exec_queue_active,
> > >  };
> > >  
> > >  static void guc_exec_queue_stop(struct xe_guc *guc, struct
> > > xe_exec_queue *q)
> >