Re: [PATCH 1/3] drm/xe/xe3p_lpg: flush userptr/shrinker bo cachelines manually

["Souza, Jose" <jose.souza@intel.com>]

On Tue, 2025-11-25 at 10:17 +0000, Matthew Auld wrote:
> On 25/11/2025 09:43, Tejas Upadhyay wrote:
> > Starting NVL, HW will flush cachelines marked with XA only
> 
> I think would be good to give basic overview of what XA is?
> 
> > when media is off. We have few cases where kernel will have
> > non-XA cachelines which needs manual flush as we postpone
> > the invalidation. Flush asap from correctness POV to ensure
> > non accelerated CPU copy to swap/shmem file will see coherent
> > view of memory, but also from security POV where later flush
> > can't corrupt the next user of those pages.
> > 
> > Signed-off-by: Tejas Upadhyay <tejas.upadhyay@intel.com>
> > ---
> >   drivers/gpu/drm/xe/xe_bo.c      |  3 ++-
> >   drivers/gpu/drm/xe/xe_device.c  | 20 ++++++++++++++++++++
> >   drivers/gpu/drm/xe/xe_device.h  |  1 +
> >   drivers/gpu/drm/xe/xe_userptr.c |  3 ++-
> >   4 files changed, 25 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_bo.c
> > b/drivers/gpu/drm/xe/xe_bo.c
> > index 465cf9fc7ce9..97e1e9d40e96 100644
> > --- a/drivers/gpu/drm/xe/xe_bo.c
> > +++ b/drivers/gpu/drm/xe/xe_bo.c
> > @@ -689,7 +689,8 @@ static int xe_bo_trigger_rebind(struct
> > xe_device *xe, struct xe_bo *bo,
> >   
> >   		if (!xe_vm_in_fault_mode(vm)) {
> >   			drm_gpuvm_bo_evict(vm_bo, true);
> > -			continue;
> > +			if (!xe_device_needs_cache_flush(xe))
> > +				continue;
> >   		}
> >   
> >   		if (!idle) {
> > diff --git a/drivers/gpu/drm/xe/xe_device.c
> > b/drivers/gpu/drm/xe/xe_device.c
> > index 92f883dd8877..6e8335b493e8 100644
> > --- a/drivers/gpu/drm/xe/xe_device.c
> > +++ b/drivers/gpu/drm/xe/xe_device.c
> > @@ -1079,6 +1079,26 @@ void xe_device_l2_flush(struct xe_device
> > *xe)
> >   	spin_unlock(&gt->global_invl_lock);
> >   }
> >   
> > +/**
> > + * xe_device_needs_cache_flush - Whether the cache needs to be
> > flushed
> > + * @xe: The device to check.
> > + *
> > + * Return: true if the device needs cache flush, false otherwise.
> > + */
> > +bool xe_device_needs_cache_flush(struct xe_device *xe)
> > +{
> > +	/*
> > +	 * Starting NVL, HW will flush cachelines marked with XA
> > only when media is off. We have
> 
> I think the wording could be improved here (same for commit message).
> XA 
> is *always* flushed, like at the end-of-submssion (and maybe other 
> places), just that internally as an optimisation hw doesn't need to
> make 
> that a full flush (which will also include XA) when Media is 
> off/powergated, since it doesn't need to worry about GT caches vs
> Media 
> coherency, and only CPU vs GPU coherency, so can make that flush a 
> targeted XA flush, since stuff tagged with XA now means it's shared
> with 
> the CPU.
> 
> > +	 * few cases where kernel will have non-XA cachelines
> > which needs manual flush and this is
> > +	 * one of them as we postpone the invalidation. Flush asap
> > from correctness POV to ensure
> > +	 * non accelerated CPU copy to swap/shmem file will see
> > coherent view of memory, but also
> > +	 * from security POV where later flush can't corrupt the
> > next user of those pages.
> > +	 */
> > +	if (GRAPHICS_VER(xe) >= 35 && !IS_DGFX(xe))
> > +		return true;
> > +	return false;
> > +}
> > +
> >   /**
> >    * xe_device_td_flush() - Flush transient L3 cache entries
> >    * @xe: The device
> > diff --git a/drivers/gpu/drm/xe/xe_device.h
> > b/drivers/gpu/drm/xe/xe_device.h
> > index 32cc6323b7f6..15e67db44b56 100644
> > --- a/drivers/gpu/drm/xe/xe_device.h
> > +++ b/drivers/gpu/drm/xe/xe_device.h
> > @@ -179,6 +179,7 @@ void xe_device_snapshot_print(struct xe_device
> > *xe, struct drm_printer *p);
> >   u64 xe_device_canonicalize_addr(struct xe_device *xe, u64
> > address);
> >   u64 xe_device_uncanonicalize_addr(struct xe_device *xe, u64
> > address);
> >   
> > +bool xe_device_needs_cache_flush(struct xe_device *xe);
> >   void xe_device_td_flush(struct xe_device *xe);
> >   void xe_device_l2_flush(struct xe_device *xe);
> >   
> > diff --git a/drivers/gpu/drm/xe/xe_userptr.c
> > b/drivers/gpu/drm/xe/xe_userptr.c
> > index 0d9130b1958a..a93c7e887cca 100644
> > --- a/drivers/gpu/drm/xe/xe_userptr.c
> > +++ b/drivers/gpu/drm/xe/xe_userptr.c
> > @@ -114,7 +114,8 @@ static void __vma_userptr_invalidate(struct
> > xe_vm *vm, struct xe_userptr_vma *uv
> >   				    false, MAX_SCHEDULE_TIMEOUT);
> >   	XE_WARN_ON(err <= 0);
> >   
> > -	if (xe_vm_in_fault_mode(vm) && userptr->initial_bind) {
> > +	if ((xe_vm_in_fault_mode(vm) ||
> > xe_device_needs_cache_flush(vm->xe)) &&
> 
> Other option is to ban non-XA or non-2WAY at the uAPI level on such 
> platforms, but I guess also depends on what UMD wants here?
> 
> Jose, I assume Mesa is just going to use XA or 2WAY for userptr on
> such 
> hw? Or do you see a usecase for being more flexible?

For userptr we will continue to use 2way, no plans to change to XA for
now.

> 
> > +	    userptr->initial_bind) {
> >   		err = xe_vm_invalidate_vma(vma);
> >   		XE_WARN_ON(err);
> >   	}