[Intel-gfx] [RFC 2/2] drm/i915: Remove PAT hack from i915_gem_object_can_bypass_llc

[Matt Roper]

On Fri, Jul 14, 2023 at 11:11:30AM +0100, Tvrtko Ursulin wrote:
> 
> On 14/07/2023 06:43, Yang, Fei wrote:
> > > From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > > 
> > > According to the comment in i915_gem_object_can_bypass_llc the
> > > purpose of the function is to return false if the platform/object
> > > has a caching mode where GPU can bypass the LLC.
> > > 
> > > So far the only platforms which allegedly can do this are Jasperlake
> > > and Elkhartlake, and that via MOCS (not PAT).
> > > 
> > > Instead of blindly assuming that objects where userspace has set the
> > > PAT index can (bypass the LLC), question is is there a such PAT index
> > > on a platform. Probably starting with Meteorlake since that one is the
> > > only one where set PAT extension can be currently used. Or if there is
> > > a MOCS entry which can achieve the same thing on Meteorlake.
> > > 
> > > If there is such PAT, now that i915 can be made to understand them
> > > better, we can make the check more fine grained. Or if there is a MOCS
> > > entry then we probably should apply the blanket IS_METEORLAKE condition.
> > > 
> > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > > Fixes: 9275277d5324 ("drm/i915: use pat_index instead of cache_level")
> > > Cc: Chris Wilson <chris.p.wilson at linux.intel.com>
> > > Cc: Fei Yang <fei.yang at intel.com>
> > > Cc: Andi Shyti <andi.shyti at linux.intel.com>
> > > Cc: Matt Roper <matthew.d.roper at intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/gem/i915_gem_object.c | 6 ------
> > >   1 file changed, 6 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > > index 33a1e97d18b3..1e34171c4162 100644
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > > @@ -229,12 +229,6 @@ bool i915_gem_object_can_bypass_llc(struct drm_i915_gem_object *obj)
> > >        if (!(obj->flags & I915_BO_ALLOC_USER))
> > >                return false;
> > > 
> > > -     /*
> > > -      * Always flush cache for UMD objects at creation time.
> > > -      */
> > > -     if (obj->pat_set_by_user)
> > 
> > I'm afraid this is going to break MESA. Can we run MESA tests with this patch?
> 
> I can't, but question is why it would break Mesa which would need a nice
> comment here?
> 
> For instance should the check be IS_METEORLAKE?
> 
> Or should it be "is wb" && "not has 1-way coherent"?
> 
> Or both?
> 
> Or, given how Meteorlake does not have LLC, how can anything bypass it
> there? Or is it about snooping on Meteorlake and how?

I think the "LLC" in the function name is a bit misleading since this is
really all just about the ability to avoid coherency (which might come
from an LLC on some platforms or from snooping on others).

The concern is that the CPU writes to the buffer and those writes sit in
a CPU cache without making it to RAM immediately.  If the GPU then
reads the object with any of the non-coherent PAT settings that were
introduced in Xe_LPG, it will not snoop the CPU cache and will read old,
stale data from RAM.

So I think we'd want a condition like ("Xe_LPG or later" && "any non
coherent PAT").  The WB/WT/UC status of the GPU behavior shouldn't
matter here, just the coherency setting.


Matt

> 
> Regards,
> 
> Tvrtko
> 
> > 
> > >        /*
> > >         * EHL and JSL add the 'Bypass LLC' MOCS entry, which should make it
> > >         * possible for userspace to bypass the GTT caching bits set by the
> > > --
> > > 2.39.2

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation