Re: [PATCH] drm/xe: Skip GT TLB invalidation when VM has no queues mapped

On Thu, 2026-08-06 at 12:14 -0700, Matthew Brost wrote:
> On Thu, Aug 06, 2026 at 01:04:28PM -0600, Summers, Stuart wrote:
> > On Wed, 2026-08-05 at 20:32 -0700, Matthew Brost wrote:
> > > If no exec queues from a VM are mapped on a GT, issuing a PPGTT
> > > TLB
> > > invalidation for that GT can require an rc6 wake which is
> > > expensive.
> > > 
> > > Skip the media TLB invalidation when the VM has no exec queues
> > > mapped on it. If TLB invalidations are already in-flight on that
> > > GT
> > > we can't break fence ordering, so issue a dummy GGTT invalidation
> > > instead to maintain seqno ordering.
> > > 
> > > This optimization is particularly impactful for SVM workloads
> > > which
> > > may or may not use the media GT. Average TLB invalidation time
> > > drops
> > > from ~75us to ~18us in such benchmarks on certain BMG parts - the
> > > improvement varies based on platform.
> > 
> > Still going through the code changes, but is there a chance we
> > could
> > deregister a context (so no queues exist), then mmap to invalidate,
> > then register a new context and read stale data here? I think in
> > the
> > context invalidation case where we don't have queues we would
> > normally
> > send a full invalidation instead rather than just skipping it. That
> > seems risky...
> 
> We don't call `xe_vm_remove_exec_queue()` until
> `__xe_exec_queue_free()`. The latter is only called after all GuC
> references are gone (i.e., deregistration has completed, or GuC state
> has otherwise been torn down due to PM events or a GT reset).
> Therefore,
> there is no race where we accidentally fail to send a required TLB
> invalidation. If anything, the opposite can happen: we may send an
> unnecessary TLB invalidation.
> 
> We don't need to use the queue reference-counting trick here to
> prevent
> deregistration, because whether a queue is registered is immaterial
> to
> successful TLB invalidation. The interface invalidates an ASID rather
> than the CTXID that the GuC tracks and requires a valid queue.

Ok.. so in the event that something like this does become required, the
expectation is we'd add the required reference counts at that time?

I'm just a little worried we're overoptimising here and will cause some
issues down the road.

Thanks,
Stuart

> 
> So all of this is safe.
> 
> Matt 
> 
> > 
> > Thanks,
> > Stuart
> > 
> > > 
> > > Signed-off-by: Matthew Brost <[email protected]>
> > > 
> > > ---
> > > v2:
> > >  - Make GT generic rather than just media GT (Thomas)
> > >  - Fix accounting bug in empty vs non-empty (CI)
> > > ---
> > >  drivers/gpu/drm/xe/xe_guc_tlb_inval.c | 20 ++++++++++++++++++--
> > >  drivers/gpu/drm/xe/xe_vm.c            | 12 ++----------
> > >  2 files changed, 20 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_guc_tlb_inval.c
> > > b/drivers/gpu/drm/xe/xe_guc_tlb_inval.c
> > > index 046d0655122f..ab04b87cf1c3 100644
> > > --- a/drivers/gpu/drm/xe/xe_guc_tlb_inval.c
> > > +++ b/drivers/gpu/drm/xe/xe_guc_tlb_inval.c
> > > @@ -205,11 +205,27 @@ static int send_tlb_inval_asid_ppgtt(struct
> > > xe_tlb_inval *tlb_inval, u32 seqno,
> > >                                      struct drm_suballoc *prl_sa)
> > >  {
> > >         struct xe_guc *guc = tlb_inval->private;
> > > +       struct xe_device *xe = guc_to_xe(guc);
> > > +       struct xe_vm *vm;
> > > +       int err, id = guc_to_gt(guc)->info.id;
> > >  
> > >         lockdep_assert_held(&tlb_inval->seqno_lock);
> > >  
> > > -       return send_tlb_inval_ppgtt(guc, seqno, start, end, asid,
> > > -                                  
> > > XE_GUC_TLB_INVAL_PAGE_SELECTIVE,
> > > prl_sa);
> > > +       vm = xe_device_asid_to_vm(xe, asid);
> > > +       if (IS_ERR(vm))
> > > +               return PTR_ERR(vm);
> > > +
> > > +       down_read(&vm->exec_queues.lock);
> > > +       if (!vm->exec_queues.count[id] &&
> > > xe_tlb_inval_idle(tlb_inval))
> > > +               err = -ECANCELED;
> > > +       else
> > > +               err = send_tlb_inval_ppgtt(guc, seqno, start,
> > > end,
> > > asid,
> > > +                                         
> > > XE_GUC_TLB_INVAL_PAGE_SELECTIVE,
> > > +                                          prl_sa);
> > > +       up_read(&vm->exec_queues.lock);
> > > +       xe_vm_put(vm);
> > > +
> > > +       return err;
> > >  }
> > >  
> > >  static int send_tlb_inval_ctx_ppgtt(struct xe_tlb_inval
> > > *tlb_inval,
> > > u32 seqno,
> > > diff --git a/drivers/gpu/drm/xe/xe_vm.c
> > > b/drivers/gpu/drm/xe/xe_vm.c
> > > index 9e0176861cb6..e2667200462c 100644
> > > --- a/drivers/gpu/drm/xe/xe_vm.c
> > > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > > @@ -4946,8 +4946,7 @@ int xe_vm_alloc_cpu_addr_mirror_vma(struct
> > > xe_vm *vm, uint64_t start, uint64_t r
> > >   * @vm: The VM.
> > >   * @q: The exec_queue
> > >   *
> > > - * Add exec queue to VM, skipped if the device does not have
> > > context
> > > based TLB
> > > - * invalidations.
> > > + * Add exec queue to VM.
> > >   */
> > >  void xe_vm_add_exec_queue(struct xe_vm *vm, struct xe_exec_queue
> > > *q)
> > >  {
> > > @@ -4961,9 +4960,6 @@ void xe_vm_add_exec_queue(struct xe_vm *vm,
> > > struct xe_exec_queue *q)
> > >         xe_assert(xe, vm->xef);
> > >         xe_assert(xe, vm == q->vm);
> > >  
> > > -       if (!xe->info.has_ctx_tlb_inval)
> > > -               return;
> > > -
> > >         down_write(&vm->exec_queues.lock);
> > >         list_add(&q->vm_exec_queue_link, &vm->exec_queues.list[q-
> > > >gt-
> > > > info.id]);
> > >         ++vm->exec_queues.count[q->gt->info.id];
> > > @@ -4975,14 +4971,10 @@ void xe_vm_add_exec_queue(struct xe_vm
> > > *vm,
> > > struct xe_exec_queue *q)
> > >   * @vm: The VM.
> > >   * @q: The exec_queue
> > >   *
> > > - * Remove exec queue from VM, skipped if the device does not
> > > have
> > > context based
> > > - * TLB invalidations.
> > > + * Remove exec queue from VM.
> > >   */
> > >  void xe_vm_remove_exec_queue(struct xe_vm *vm, struct
> > > xe_exec_queue
> > > *q)
> > >  {
> > > -       if (!vm->xe->info.has_ctx_tlb_inval)
> > > -               return;
> > > -
> > >         down_write(&vm->exec_queues.lock);
> > >         if (!list_empty(&q->vm_exec_queue_link)) {
> > >                 list_del(&q->vm_exec_queue_link);
> >