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@xxxxxxxxx wrote: > >>>> From: Fei Yang <fei.yang@xxxxxxxxx> > >>>> > >>>> 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
