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. 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
