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

[Matthew Auld <matthew.auld@intel.com>]

On 17/02/2026 17:04, Thomas Hellström wrote:
> On Mon, 2026-02-16 at 16:41 +0000, Matthew Auld wrote:
>> On 16/02/2026 15:38, Thomas Hellström wrote:
>>> On Mon, 2026-02-16 at 14:55 +0000, Matthew Auld wrote:
>>>> On 16/02/2026 12:07, Thomas Hellström wrote:
>>>>> On Mon, 2026-02-16 at 10:58 +0000, Matthew Auld wrote:
>>>>>> On 16/02/2026 10:23, Thomas Hellström wrote:
>>>>>>> On Fri, 2026-02-13 at 17:31 +0000, Matthew Auld wrote:
>>>>>>>> On 13/02/2026 17:16, Matt Roper wrote:
>>>>>>>>> On Fri, Feb 13, 2026 at 04:48:39PM +0000, Souza, Jose
>>>>>>>>> wrote:
>>>>>>>>>> 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.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>> The coherency restriction is already in the uAPI:
>>>>>>>>>>
>>>>>>>>>> "Note: For userptr and externally imported dma-buf
>>>>>>>>>> the
>>>>>>>>>> kernel
>>>>>>>>>> expects
>>>>>>>>>> either 1WAY or 2WAY for the @pat_index."
>>>>>>>>>>
>>>>>>>>>> Using 1 way is enough as Xe KMD does a PIPE_CONTROL
>>>>>>>>>> flushing
>>>>>>>>>> GPU
>>>>>>>>>> caches
>>>>>>>>>> at the end of batch buffers.
>>>>>>>>>
>>>>>>>>> But isn't that what we're discussing here?  1-way
>>>>>>>>> *won't*
>>>>>>>>> necessarily be
>>>>>>>>> enough anymore because PIPE_CONTROL instructions don't
>>>>>>>>> flush
>>>>>>>>> the
>>>>>>>>> entire
>>>>>>>>> cache anymore.  Whenever the GuC determines that media
>>>>>>>>> is
>>>>>>>>> inactive
>>>>>>>>> and
>>>>>>>>> activates the optimization, PIPE_CONTROL, MI_FLUSH_DW,
>>>>>>>>> etc.
>>>>>>>>> change
>>>>>>>>> behavior to only flush out the subset of data that was
>>>>>>>>> marked
>>>>>>>>> as
>>>>>>>>> app-transient; anything not marked that way doesn't get
>>>>>>>>> flushed
>>>>>>>>> now.  So
>>>>>>>>> there's a new requirement here that you ensure you're
>>>>>>>>> using
>>>>>>>>> an
>>>>>>>>> XA
>>>>>>>>> PAT
>>>>>>>>> index, or you switch to use 2-way coherency which will
>>>>>>>>> allow
>>>>>>>>> the
>>>>>>>>> CPU to
>>>>>>>>> snoop the GPU's caches.
>>>>>>>>
>>>>>>>> That exactly matches my understanding also.
>>>>>>>
>>>>>>> This only ever affects IGFX, right? Since AFAIU we don't
>>>>>>> have
>>>>>>> 2-way
>>>>>>> coherency with DGFX?
>>>>>>
>>>>>> Yeah, this should be igpu only. I seem to also recall that on
>>>>>> dgpu,
>>>>>> Media is coherent with l2/l3, but also I don't think system
>>>>>> memory
>>>>>> can
>>>>>> be cached in l2/l3 (only VRAM), which I assume is why there
>>>>>> is
>>>>>> the
>>>>>> special SMRO (system-memory-read-only) cache only on dgpu,
>>>>>> which
>>>>>> is
>>>>>> flushed when the fence signals, unlike the l2/l3.
>>>>>
>>>>> Yes that sounds reasonable.
>>>>>
>>>>>>
>>>>>>>
>>>>>>> It sounds like the same PAT restriction is needed also for
>>>>>>> imported
>>>>>>> dma-buf, right?
>>>>>>
>>>>>> Good point. Looks like we are missing that still. Otherwise
>>>>>> we
>>>>>> can
>>>>>> run
>>>>>> into the same issues with stale l2/l3/ppc.
>>>>>
>>>>> So if this affects only system memory could we instead of
>>>>> relying
>>>>> on 2-
>>>>> way coherency or XA, just flush at dma unmap time, because
>>>>> that's
>>>>> typically just before releasing the pages.
>>>>
>>>> Yeah, I think we could make it work, from security pov, similar
>>>> to
>>>> userptr, with the right manual flushes in KMD. Maybe just a
>>>> question
>>>> if
>>>> userspace wants such a model? Anything cached in l2/l3 might
>>>> require
>>>> manual flushing by userspace (if that is even possible)?
>>>
>>> So that would mean if user-space wants gpu-cpu coherency at fence
>>> synchronization points, they'd have to use either 2-way or XA pat
>>> indices, but not enforced by KMD.
>>
>> Yeah, looking at BSpec 74635 (Media off case), I'm only really seeing
>> MEM_SET which userspace could potentially use by itself? But then
>> it's
>> unclear if they mean to actually clear-the-memory (which is not what
>> we
>> want) or using the special evict mode, but that seems to be talking
>> more
>> about flushing to local memory, so not completely sure what that does
>> on
>> igpu. If it's the evict mode then should in theory be possible for
>> userpace to do a manual flush, but that would have to be done per-
>> bo/vma?
>>
>>>
>>> For imported dma-buf kernel requires 2-way or XA for security due
>>> to
>>> the relaxed dma-buf unmap.
>>>
>>> For SVM/System allocator we'd require 2-way or XA.
>>>
>>> Otherwise KMD security is enforced by flush at dma-unmap time?
>>
>> Yeah, that is my understanding. Otherwise I don't currently see what
>> prevents the dirty non-XA cache lines being flushed at some random
>> point
>> later, after we have already freed the corresponding system memory,
>> potentially nuking the next user who allocates those pages.
> 
> So I've discussed a bit more with Tejas and since the virtual addresses
> are needed for the flush, flushing at dma-unmap time doesn't really
> work. And since this is IGFX only, where we sync on moves, a flush in
> xe_bo_trigger_rebind() should be completely ok, at least until affected
> DGFX occurs, where we might want to look at async TLB flushes.
> 
> And for simplicitly then go for the PAT restriction also for userptr,
> svm and imported dma-buf.
> 
> Thoughts?

Yeah agreed, I think that should be good enough and will hopefully cover 
all the missing cases.

Otherwise maybe we could somehow use a known dummy address range from 
the migrate vm or something, and use that to flush the PPC from unmap? I 
assume the address range doesn't actually matter if we just care about 
flushing entire PPC?

There is also the xe_page_reclaim stuff for a targeted flush instead of 
nuking entire PPC, which doesn't seem to need ppgtt virtual address, 
just a list of physical page addresses, which will be a lot better for 
the smaller BOs. But maybe this is more follow up stuff.

> 
> Thomas
> 
>>
>>>
>>> /Thomas
>>>
>>>>
>>>>>
>>>>> The exception, though, is dma-buf where the exporter can
>>>>> actually
>>>>> release memory before all importers have given up their dma-
>>>>> mappings.
>>>>>
>>>>> /Thomas
>>>>>
>>>>>>
>>>>>>>
>>>>>>> /Thomas
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Matt
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> 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_evi
>>>>>>>>>>>>>>>> ct(v
>>>>>>>>>>>>>>>> m_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 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
>>>>>>>>>