> 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(). I proposed a patch for diic which is directly applicable to drm-tip as well. Could you review http://intel-gfx-pw.fi.intel.com/series/19405/ and let me know if that would address your concern here? -Fei > -- > Ville Syrjälä > Intel
