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

[Yang, Fei]

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

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