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

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

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?

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