Re: [Intel-gfx] [PATCH 5/7] drm/i915: use pat_index instead of cache_level

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Apr 03, 2023 at 04:57:21PM +0000, Yang, Fei wrote:
> > Subject: Re: [PATCH 5/7] drm/i915: use pat_index instead of cache_level
> >
> > On Fri, Mar 31, 2023 at 11:38:28PM -0700, fei.yang@intel.com wrote:
> >> From: Fei Yang <fei.yang@intel.com>
> >> 
> >> Currently the KMD is using enum i915_cache_level to set caching policy for
> >> buffer objects. This is flaky because the PAT index which really controls
> >> the caching behavior in PTE has far more levels than what's defined in the
> >> enum.
> >
> > Then just add more enum values.
> 
> That would be really messy because PAT index is platform dependent, you would
> have to maintain many tables for the the translation.
> 
> > 'pat_index' is absolutely meaningless to the reader, it's just an
> > arbitrary number. Whereas 'cache_level' conveys how the thing is
> > actually going to get used and thus how the caches should behave.
> 
> By design UMD's understand PAT index. Both UMD and KMD should stand on the
> same ground, the Bspec, to avoid any potential ambiguity.
> 
> >> In addition, the PAT index is platform dependent, having to translate
> >> between i915_cache_level and PAT index is not reliable,
> >
> >If it's not realiable then the code is clearly broken.
> 
> Perhaps the word "reliable" is a bit confusing here. What I really meant to
> say is 'difficult to maintain', or 'error-prone'.
> 
> >> and makes the code more complicated.
> >
> > You have to translate somewhere anyway. Looks like you're now adding
> > translations the other way (pat_index->cache_level). How is that better?
> 
> No, there is no pat_index->cache_level translation.

i915_gem_object_has_cache_level() is exactly that. And that one
does look actually fragile since it assumes only one PAT index
maps to each cache level. So if the user picks any other pat_index
anything using i915_gem_object_has_cache_level() is likely to
do the wrong thing.

If we do switch to pat_index then I think cache_level should
be made a purely uapi concept, and all the internal code should
instead be made to query various aspects of the caching behaviour
of the current pat_index (eg. is LLC caching enabled, and thus
do I need to clflush?).

-- 
Ville Syrjälä
Intel