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

[Yang, Fei]

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