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

[Matthew Brost <matthew.brost@intel.com>]

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.

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