[PATCH 5/7] drm/i915: use pat_index instead of cache_level

[Ville Syrjälä]

On Mon, Apr 03, 2023 at 07:39:37PM +0000, Yang, Fei wrote:
> >Subject: Re: [PATCH 5/7] drm/i915: use pat_index instead of cache_level
> >
> >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 at intel.com wrote:
> >>>> From: Fei Yang <fei.yang at 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.
> 
> That is still one way transaltion, from cache_level to pat_index.

Not really. The actual input to the thing is obj->pat_index.
And as stated, the whole thing is simply broken whenever
obj->pat_index isn't one of the magic numbers that you get
back from i915_gem_get_pat_index().

-- 
Ville Syrjälä
Intel