>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. The cache_level is only a KMD concept now. And inside the KMD, we have one table to translate between cache_level to pat_index. Only KMD would be able to trigger a comparison on pat_index for a KMD allocated BO. User is not allowed to set pat_index dynamically any more. By design the cahce setting for user space BO's should be immutable. That's why even the set caching ioctl has been killed (from MTL onward). > If we do switch to pat_index then I think cache_level should be made a > purely uapi concept, UMD's directly use pat_index because they are supposed to follow the b-spec. The abstracted cache_level is no longer exposed to user space. -Fei > 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
