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

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On Tue, 2025-11-25 at 15:31 +0000, Upadhyay, Tejas wrote:
> 
> 
> > -----Original Message-----
> > From: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > Sent: 25 November 2025 20:37
> > To: Upadhyay, Tejas <tejas.upadhyay@intel.com>; intel-
> > xe@lists.freedesktop.org
> > Cc: Auld, Matthew <matthew.auld@intel.com>
> > Subject: Re: [PATCH 1/3] drm/xe/xe3p_lpg: flush userptr/shrinker bo
> > cachelines manually
> > 
> > Hi.
> > 
> > 
> > On Tue, 2025-11-25 at 15:13 +0530, Tejas Upadhyay wrote:
> > > Starting NVL, HW will flush cachelines marked with XA only 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>
> > 
> > I had a number of concerns last time this patch was sent to the
> > list, none of
> > which seems to have been addressed?
> 
> Sorry for missing to address your comments.
> 
> > 
> > https://lore.kernel.org/intel-
> > xe/d2517d66f571e11a760cb143981b7ca238f5cd58.camel@linux.intel.com/
> > 
> > The main concern is that the code indicates that not all GPU caches
> > are flushed
> > when all fences are signalled (bo / userptr idle)?
> 
> Xe3p is introducing feature that when media is off, only XA marked BO
> will be flushed not whole cache. From UMD perspective we might have
> non-XA buffers created which we would like to flush before
> buffer/user goes away during media off. 

So for non-XA buffers, how would coherency be maintained for
gpu_write() -> cpu_read() from UMD's perspective? For dma-buf?

Also flushing in move_notify() can't be done unless the bo is idle
first, and that would force us to unnecessarily synchronize.

/Thomas


> 
> Tejas
>  
> > 
> > Thanks,
> > Thomas
> > 
> > 
> > 
> > > ---
> > >  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
> > > +	 * 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)) &&
> > > +	    userptr->initial_bind) {
> > >  		err = xe_vm_invalidate_vma(vma);
> > >  		XE_WARN_ON(err);
> > >  	}
>