Re: [PATCH 1/2] drm/xe: Add feature to force ring-based TLB invalidations

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

On Wed, 2026-01-14 at 19:22 +0000, Summers, Stuart wrote:
> On Tue, 2026-01-13 at 19:40 -0800, Matthew Brost wrote:
> > On Tue, Jan 13, 2026 at 07:26:52PM -0800, Matthew Brost wrote:
> > > On Wed, Jan 14, 2026 at 12:04:20AM +0000, Stuart Summers wrote:
> > > > Current hardware does an implicit TLB invalidation on
> > > > context switch. This is required for various use cases like
> > > > preempt fences where a context switch will happen without
> > > > any explicit TLB invalidation from the KMD.
> > > > 
> > > > Add a hook to make this explicit as part of the ring
> > > > prior to batch execution to allow for more explicit testing
> > > > and sequencing of user batch execution.
> > > > 
> > > 
> > > I don't this patch is needed rather this an existing bug in
> > > xe_pt.c...
> > > 
> > > 1974                 if ((!pt_op->rebind && xe_vm_has_scratch(vm)
> > > &&
> > > 1975                      xe_vm_in_lr_mode(vm)))
> > > 1976                         pt_update_ops->needs_invalidation =
> > > true;
> > > 1977                 else if (pt_op->rebind &&
> > > !xe_vm_in_lr_mode(vm))
> > > 1978                         /* We bump also if
> > > batch_invalidate_tlb is true */
> > > 1979                         vm->tlb_flush_seqno++;
> > > 
> > > I believe we also need this at the end:
> > > 
> > > else if (pt_op->rebind)
> > >         pt_update_ops->needs_invalidation = true;
> > > 
> > 
> > Ugh, actually this isn't right either this will issue a TLB
> > invalidation
> > on page fault fix or VF resume which isnt right either.
> > 
> > I think it is the right behavior for VM bind IOCTLs, but now that I
> > think about it we'd get a UNMAP op there which would set
> > needs_invalidation - only on page fault fix or VF resume or exec
> > IOCTL
> > page fixup we only issue a MAP op - the latter correctly increments
> > tlb_flush_seqno. The prior cases we assume no invalidation is
> > needed
> > but
> > a invalidation at start of batch wouldn't help in all cases there
> > either.
> > 
> > > My question is: does this patch internally change failure rates?
> > > If
> > > so,
> > > I believe implicit TLB invalidation is hiding a bug in the
> > > current
> > > IGT suite/KMD/hardware that just hasn’t been caught yet.
> > > 
> > 
> > So let's start here.
> 
> Ok so just to be clear, is the expectation here that we aren't doing
> any implicit TLB invalidations generally? Or are there specific cases
> where we want to rely on these (like preempt fences) and any other
> cases where we aren't doing this explicitly is considered a bug?

Ok I had mentioned in review comments to [1], but this is definitely
required from my testing today at least - without any other fixes.

I'd kind of like to move forward with [1] if possible though. What do
you think of including this series on top functionally and then follow
up with the full fixes later so we remove the ring-based invalidations?
These are held under an unused (outside of the for-ci patch in this
series) feature flag and of course no direct user interface.

Let me know what you think.

Thanks,
Stuart

[1]: https://patchwork.freedesktop.org/series/156874/#rev4

> 
> Thanks,
> Stuart
> 
> > 
> > > Matt
> > > 
> > > > Signed-off-by: Akshata Jahagirdar
> > > > <akshata.jahagirdar@intel.com>
> > > > Signed-off-by: Stuart Summers <stuart.summers@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/xe/xe_device_types.h | 2 ++
> > > >  drivers/gpu/drm/xe/xe_pci.c          | 1 +
> > > >  drivers/gpu/drm/xe/xe_pci_types.h    | 1 +
> > > >  drivers/gpu/drm/xe/xe_sched_job.c    | 7 +++++--
> > > >  drivers/gpu/drm/xe/xe_vm.c           | 3 +++
> > > >  5 files changed, 12 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/xe/xe_device_types.h
> > > > b/drivers/gpu/drm/xe/xe_device_types.h
> > > > index f689766adcb1..fa193e0b906d 100644
> > > > --- a/drivers/gpu/drm/xe/xe_device_types.h
> > > > +++ b/drivers/gpu/drm/xe/xe_device_types.h
> > > > @@ -355,6 +355,8 @@ struct xe_device {
> > > >                 u8 has_pxp:1;
> > > >                 /** @info.has_range_tlb_inval: Has range based
> > > > TLB invalidations */
> > > >                 u8 has_range_tlb_inval:1;
> > > > +               /** @info.has_ring_tlb_inval: Performs TLB
> > > > invalidations on context switch */
> > > > +               u8 has_ring_tlb_inval:1;
> > > >                 /** @info.has_soc_remapper_sysctrl: Has SoC
> > > > remapper system controller */
> > > >                 u8 has_soc_remapper_sysctrl:1;
> > > >                 /** @info.has_soc_remapper_telem: Has SoC
> > > > remapper telemetry support */
> > > > diff --git a/drivers/gpu/drm/xe/xe_pci.c
> > > > b/drivers/gpu/drm/xe/xe_pci.c
> > > > index 5c705124270e..a2ce9da2e3cb 100644
> > > > --- a/drivers/gpu/drm/xe/xe_pci.c
> > > > +++ b/drivers/gpu/drm/xe/xe_pci.c
> > > > @@ -893,6 +893,7 @@ static int xe_info_init(struct xe_device
> > > > *xe,
> > > >                 xe->info.has_device_atomics_on_smem = 1;
> > > >  
> > > >         xe->info.has_range_tlb_inval = graphics_desc-
> > > > > has_range_tlb_inval;
> > > > +       xe->info.has_ring_tlb_inval = graphics_desc-
> > > > > has_ring_tlb_inval;
> > > >         xe->info.has_usm = graphics_desc->has_usm;
> > > >         xe->info.has_64bit_timestamp = graphics_desc-
> > > > > has_64bit_timestamp;
> > > >  
> > > > diff --git a/drivers/gpu/drm/xe/xe_pci_types.h
> > > > b/drivers/gpu/drm/xe/xe_pci_types.h
> > > > index 20acc5349ee6..783ebda6eae1 100644
> > > > --- a/drivers/gpu/drm/xe/xe_pci_types.h
> > > > +++ b/drivers/gpu/drm/xe/xe_pci_types.h
> > > > @@ -72,6 +72,7 @@ struct xe_graphics_desc {
> > > >         u8 has_atomic_enable_pte_bit:1;
> > > >         u8 has_indirect_ring_state:1;
> > > >         u8 has_range_tlb_inval:1;
> > > > +       u8 has_ring_tlb_inval:1;
> > > >         u8 has_usm:1;
> > > >         u8 has_64bit_timestamp:1;
> > > >  };
> > > > diff --git a/drivers/gpu/drm/xe/xe_sched_job.c
> > > > b/drivers/gpu/drm/xe/xe_sched_job.c
> > > > index 39aec7f6d86d..c3b3d9c56dbb 100644
> > > > --- a/drivers/gpu/drm/xe/xe_sched_job.c
> > > > +++ b/drivers/gpu/drm/xe/xe_sched_job.c
> > > > @@ -246,6 +246,7 @@ bool xe_sched_job_completed(struct
> > > > xe_sched_job *job)
> > > >  void xe_sched_job_arm(struct xe_sched_job *job)
> > > >  {
> > > >         struct xe_exec_queue *q = job->q;
> > > > +       struct xe_device *xe = gt_to_xe(q->gt);
> > > >         struct dma_fence *fence, *prev;
> > > >         struct xe_vm *vm = q->vm;
> > > >         u64 seqno = 0;
> > > > @@ -259,9 +260,11 @@ void xe_sched_job_arm(struct xe_sched_job
> > > > *job)
> > > >                         xe_vm_assert_held(q->vm);
> > > >         }
> > > >  
> > > > -       if (vm && !xe_sched_job_is_migration(q) &&
> > > > !xe_vm_in_lr_mode(vm) &&
> > > > +       if (vm && !xe_sched_job_is_migration(q) &&
> > > > +           (xe->info.has_ring_tlb_inval ||
> > > > !xe_vm_in_lr_mode(vm)) &&
> > > >             (vm->batch_invalidate_tlb || vm->tlb_flush_seqno !=
> > > > q->tlb_flush_seqno)) {
> > > > -               xe_vm_assert_held(vm);
> > > > +               if (!xe->info.has_ring_tlb_inval)
> > > > +                       xe_vm_assert_held(vm);
> > > >                 q->tlb_flush_seqno = vm->tlb_flush_seqno;
> > > >                 job->ring_ops_flush_tlb = true;
> > > >         }
> > > > diff --git a/drivers/gpu/drm/xe/xe_vm.c
> > > > b/drivers/gpu/drm/xe/xe_vm.c
> > > > index 694f592a0f01..8e0ba2eba21e 100644
> > > > --- a/drivers/gpu/drm/xe/xe_vm.c
> > > > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > > > @@ -1619,6 +1619,9 @@ struct xe_vm *xe_vm_create(struct
> > > > xe_device
> > > > *xe, u32 flags, struct xe_file *xef)
> > > >                         vm->batch_invalidate_tlb = false;
> > > >                 }
> > > >  
> > > > +               if (xe->info.has_ring_tlb_inval)
> > > > +                       vm->batch_invalidate_tlb = true;
> > 
> > This definitely isn’t right or desired.
> > 
> > Matt
> > 
> > > > +
> > > >                 /* Fill pt_root after allocating scratch tables
> > > > */
> > > >                 for_each_tile(tile, xe, id) {
> > > >                         if (!vm->pt_root[id])
> > > > -- 
> > > > 2.34.1
> > > > 
>