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

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

On Fri, 2026-02-13 at 16:23 +0000, Upadhyay, Tejas wrote:
> 
> 
> > -----Original Message-----
> > From: Roper, Matthew D <matthew.d.roper@intel.com>
> > Sent: 12 February 2026 02:41
> > To: Upadhyay, Tejas <tejas.upadhyay@intel.com>
> > Cc: Brost, Matthew <matthew.brost@intel.com>; intel-
> > xe@lists.freedesktop.org; Auld, Matthew <matthew.auld@intel.com>;
> > thomas.hellstrom@linux.intel.com
> > Subject: Re: [PATCH 1/3] drm/xe/xe3p_lpg: flush userptr/shrinker bo
> > cachelines manually
> > 
> > On Wed, Feb 11, 2026 at 07:06:05PM +0000, Upadhyay, Tejas wrote:
> > > 
> > > 
> > > > -----Original Message-----
> > > > From: Brost, Matthew <matthew.brost@intel.com>
> > > > Sent: 11 February 2026 05:32
> > > > To: Roper, Matthew D <matthew.d.roper@intel.com>
> > > > Cc: Upadhyay, Tejas <tejas.upadhyay@intel.com>; intel-
> > > > xe@lists.freedesktop.org; Auld, Matthew
> > > > <matthew.auld@intel.com>;
> > > > thomas.hellstrom@linux.intel.com
> > > > Subject: Re: [PATCH 1/3] drm/xe/xe3p_lpg: flush
> > > > userptr/shrinker bo
> > > > cachelines manually
> > > > 
> > > > On Tue, Feb 10, 2026 at 01:05:25PM -0800, Matt Roper wrote:
> > > > > On Tue, Feb 10, 2026 at 06:21:22PM +0530, Tejas Upadhyay
> > > > > wrote:
> > > > > > "eXtended Architecture" (XA) tagged memory—memory shared
> > between
> > > > the
> > > > > > CPU and GPU
> > > > > 
> > > > > I'm pretty sure this expansion of "XA" is wrong; where are
> > > > > you
> > > > > seeing this definition?  Everything in the bspec indicates
> > > > > that XA
> > > > > means "wb
> > > > > - transient app" (similar to how "XD" is 'wb - transient
> > > > > display").
> > > > > I'm not sure why exactly they picked "X" to refer to
> > > > > transient in
> > > > > both of these cases, but I've never seen any documentation
> > > > > that
> > > > > refers to it as "extended."
> > > > > 
> > > > > > is treated differently from other GPU memory when the Media
> > > > > > engine is
> > > > power-gated.
> > > > > > 
> > > > > > XA is *always* flushed, like at the end-of-submssion (and
> > > > > > maybe
> > > > > > other
> > > > > 
> > > > > I assume you're referring to the fact that the driver
> > > > > performs
> > > > > flushes at the end of submission (via PIPE_CONTROL or
> > > > > MI_FLUSH_DW), and that depending on other state/optimizations
> > > > > in
> > > > > the system, those flushes may flush the entire device cache,
> > > > > or
> > > > > may only flush the subset of cache data that is not marked as
> > > > > transient.  The way you worded this was confusing since it
> > > > > makes
> > > > > it sound like cache flushes happen automatically somewhere in
> > hardware/firmware.
> > > > > 
> > > > > > 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. The main implication is
> > > > > > that
> > > > > > we now need to somehow flush non-XA before freeing system
> > > > > > memory
> > > > > > pages, otherwise dirty cachelines could be flushed after
> > > > > > the
> > > > > > free (like if Media suddenly turns on and does a full
> > > > > > flush)
> > > > > 
> > > > > This description seems really confusing.  My understanding is
> > > > > that
> > > > > marking something as wb-transient-app indicates that it might
> > > > > be
> > > > > accessed by something other than our graphics/media IP (i.e.,
> > > > > accessed from the CPU, exported to another device, etc.), so
> > > > > transient data truly does need to be flushed at the points in
> > > > > the
> > > > > driver where a flush typically happens.
> > > > > 
> > > > > However when something is _not_ transient, then either:
> > > > >  - it's "private" to the GPU and only our graphics/media IP
> > > > > will be
> > > > >    accessing it
> > > > >  - it's bound with a coherent PAT index so that outside
> > > > > observers like
> > > > >    the CPU can snoop the device cache, even when the cache
> > > > > hasn't been
> > > > >    flushed
> > > > > 
> > > > > If media is not active, then there's really no need to
> > > > > include
> > > > > non-transient data when an device cache flush happens since
> > > > > there's no real need for the data to get to RAM.  So that
> > > > > enables
> > > > > an optimization (which comes in your next patch), that allows
> > > > > flushes to only operate on the subset of the device cache
> > > > > tagged as
> > "transient" if media is idle.
> > > 
> > > But what If we have stale non-XA marked pages for userptr, and
> > > that
> > > object moves out and at the same time media comes back, will end
> > > up in
> > > full flush and flush the stale entry to RAM.
> > 
> > What makes userptr special here?  During general, active usage,
> > userptr would
> > be data that's accessible by the CPU, so it needs to either be
> > transient (so CPU
> > can see the data in RAM after explicit flushes) or it needs to be
> > using a
> > coherent PAT (so that the CPU can just snoop the GPU cache).  If
> > you marked
> > userptr as both non-XA and non-coherent, then that sounds likely to
> > be a
> > userspace bug (and probably something we can catch and reject as an
> > invalid
> > case on any Xe3p or later platforms that support this) since the
> > CPU wouldn't
> > have any reliable way of seeing GPU updates.
> 
> Right. FYI @Mrozek, Michal @Souza, Jose
> For userptr, as explained above, it needs to be either coherent or XA
> pat index, or else KMD will reject as invalid case. 
> 
> > 
> > If something happens that changes the GTT mapping of an object,
> > then
> > doesn't that already trigger a TLB invalidation when necessary in
> > the driver
> > today?  It was my understanding that "heavy" TLB invalidations wait
> > for data
> > values to be globally observable before starting, so I think that
> > would ensure
> > that any non-XA data makes it to RAM before any binding changes,
> > object,
> > destruction, etc.?  Is there something special about userptr that
> > makes that
> > case more of a problem?
> > 
> > I just found bspec page 74635 which gives an overview of the
> > various flush
> > and invalidate cases, and I don't see anything there that makes it
> > obvious to
> > me that userptr would be special.
> > 
> > 
> > > 
> > > > > 
> > > > > As you said, we eventually do want to force a flush of the
> > > > > non-transient data as well once we're freeing the underlying
> > > > > pages.
> > > > > So how do we do that?  It's not clear to me how the changes
> > > > > below
> > > > > are accomplishing that.  Is there a way to explicitly request
> > > > > a
> > > > > full device cache flush (ignoring the transient vs non-
> > > > > transient tagging)?
> > > > > Since the GuC handles the optimization in the next patch
> > > > > (toggling
> > > > > whether flushes are full flushes vs non-transient flushes
> > > > > depending on whether media is active), I thought there might
> > > > > be
> > > > > some kind of GuC interface to request "please do one full
> > > > > flush now, even
> > if media is idle."
> > > > > 
> > > > 
> > > > I’m not an expert here by any means, but everything above from
> > > > Matt
> > > > seems like valid concerns. Thomas also raised some concerns in
> > > > the
> > > > two previous revisions; again I’m not an expert, but reading
> > > > through
> > > > those, it doesn’t really seem like he received proper answers
> > > > to his
> > questions.
> > > 
> > > Its forcing flush via tlb invalidation PPC flag under
> > > xe_invalidate_vma( ).
> > 
> > By the way, what is "PPC?"  It seems like it's another new synonym
> > for the
> > device cache?  It's already really confusing that some of our
> > hardware docs use
> > a mix of both "L2" and "L3" to refer to the same device cache for
> > historical
> > reasons...
> > 
> > 
> > Matt
> > 
> > > 
> > > > 
> > > > A couple of comments below.
> > > > 
> > > > > 
> > > > > Matt
> > > > > 
> > > > > > 
> > > > > > V2(MattA): Expand commit description
> > > > > > 
> > > > > > Signed-off-by: Tejas Upadhyay <tejas.upadhyay@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/xe/xe_bo.c      |  3 ++-
> > > > > >  drivers/gpu/drm/xe/xe_device.c  | 23
> > > > > > +++++++++++++++++++++++
> > > > > > drivers/gpu/drm/xe/xe_device.h  |  1 +
> > > > > > drivers/gpu/drm/xe/xe_userptr.c |  3 ++-
> > > > > >  4 files changed, 28 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/xe/xe_bo.c
> > > > > > b/drivers/gpu/drm/xe/xe_bo.c index
> > > > > > e9180b01a4e4..4455886b211e
> > > > > > 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;
> 
> Matt R,
> This flush will be still needed as there can be non-xa buffers which
> can be evicted while media was off and stale entries can be flushed
> when media comes back on. Which was not case earlier as full flush
> was happening at regular sync points and that’s where this feature is
> bringing optimization now.
> 
> Tejas

This flush would effecively eliminate any asynchronous migration, so
again if this is only affecting IGFX we're synchronizing on shrinking
anyway, but in general I think we should avoid synchronizing here.

Could this instead be done asynchronously similar to how we flush TLB?
so it would be part of the migration fence?

/Thomas

> 
> > > > 
> > > > This will trigger a TLB invalidation (and I assume a cache
> > > > flush)
> > > > every time we move or free memory in the 3D stack if it has a
> > > > binding. It also performs a synchronous wait on the BO being
> > > > idle.
> > > > Both of these are very expensive operations. I can’t imagine
> > > > the
> > > > granularity we want here is to do this on every move/free with
> > > > bindings.
> > > > 
> > > > Also, for LR compute with preempt fences, we would trigger the
> > > > preempt fences during the wait, so a TLB invalidation after
> > > > this
> > > > seems unnecessary, though perhaps the cache flush is still
> > > > required?
> > > > 
> > > > I think this needs a bit more explanation, because without
> > > > knowing a
> > > > lot about the exact requirements, the implementation does not
> > > > look
> > correct.
> > > 
> > > The thing is that we are trying to solve problem with userptr
> > > with non-XA
> > pat, consider if that BO got moved while media is not active. As
> > soon as media
> > will come back active, stale cached entries of that object will be
> > flushed as part
> > of full flush , which may corrupt things.
> > > There was thinking that with this patch we would at least solve
> > > the problem
> > of corruption and later when page_reclamation feature comes in will
> > help in
> > performance as well. But now when page reclamation feature is
> > merged earlier
> > and it tightly coupled with bind/unbind some cases like discussed
> > above
> > (which are not doing unbind immediately on move/free) are missed in
> > reclamation.
> > > 
> > > So thought was to let this solution go in with little perf hit
> > > and discuss with
> > page reclamation owner to come with cleaner solution together.
> > > 
> > > Tejas
> > > > 
> > > > > >  		}
> > > > > > 
> > > > > >  		if (!idle) {
> > > > > > diff --git a/drivers/gpu/drm/xe/xe_device.c
> > > > > > b/drivers/gpu/drm/xe/xe_device.c index
> > > > > > 743c18e0c580..da2abed94bc0
> > > > > > 100644
> > > > > > --- a/drivers/gpu/drm/xe/xe_device.c
> > > > > > +++ b/drivers/gpu/drm/xe/xe_device.c
> > > > > > @@ -1097,6 +1097,29 @@ static void tdf_request_sync(struct
> > > > > > xe_device
> > > > *xe)
> > > > > >  	}
> > > > > >  }
> > > > > > 
> > > > > > +/**
> > > > > > + * 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) {
> > > > > > +	/* 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. The main implication is that we now
> > > > > > need to
> > > > > > +somehow
> > > > flush non-XA before
> > > > > > +	 * freeing system memory pages, otherwise dirty
> > > > > > cachelines
> > > > > > +could be
> > > > flushed after the free
> > > > > > +	 * (like if Media suddenly turns on and does a
> > > > > > full flush)
> > > > > > +	 */
> > > > > > +	if (GRAPHICS_VER(xe) >= 35 && !IS_DGFX(xe))
> > > > > > +		return true;
> > > > > > +	return false;
> > > > > > +}
> > > > > > +
> > > > > >  void xe_device_l2_flush(struct xe_device *xe)  {
> > > > > >  	struct xe_gt *gt;
> > > > > > diff --git a/drivers/gpu/drm/xe/xe_device.h
> > > > > > b/drivers/gpu/drm/xe/xe_device.h index
> > > > > > 39464650533b..baf386e0e037
> > > > > > 100644
> > > > > > --- a/drivers/gpu/drm/xe/xe_device.h
> > > > > > +++ b/drivers/gpu/drm/xe/xe_device.h
> > > > > > @@ -184,6 +184,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
> > > > > > e120323c43bc..b435ea7f9b66
> > > > > > 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) {
> > > > 
> > > > Same concern with the LR preempt fence as above — the hardware
> > > > will
> > > > be interrupted via preempt fences, so it doesn’t seem necessary
> > > > to
> > > > invalidate the TLBs but perhaps we need a cflush and TLB
> > > > invalidation is the mechanism for that too?
> > > > 
> > > > Matt
> > > > 
> > > > > >  		err = xe_vm_invalidate_vma(vma);
> > > > > >  		XE_WARN_ON(err);
> > > > > >  	}
> > > > > > --
> > > > > > 2.52.0
> > > > > > 
> > > > > 
> > > > > --
> > > > > Matt Roper
> > > > > Graphics Software Engineer
> > > > > Linux GPU Platform Enablement
> > > > > Intel Corporation
> > 
> > --
> > Matt Roper
> > Graphics Software Engineer
> > Linux GPU Platform Enablement
> > Intel Corporation