Re: [Intel-gfx] [PATCH v7 2/2] drm/i915: use pat_index instead of cache_level

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 09/05/2023 18:12, Yang, Fei wrote:
>  > On 09/05/2023 00:48, fei.yang@intel.com wrote:
>  >> From: Fei Yang <fei.yang@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. In addition, the PAT index is platform dependent, having to 
> translate
>  >> between i915_cache_level and PAT index is not reliable, and makes 
> the code
>  >> more complicated.
>  >>
>  >> From UMD's perspective there is also a necessity to set caching 
> policy for
>  >> performance fine tuning. It's much easier for the UMD to directly 
> use PAT
>  >> index because the behavior of each PAT index is clearly defined in 
> Bspec.
>  >> Having the abstracted i915_cache_level sitting in between would only 
> cause
>  >> more ambiguity. PAT is expected to work much like MOCS already works 
> today,
>  >> and by design userspace is expected to select the index that exactly
>  >> matches the desired behavior described in the hardware specification.
>  >>
>  >> For these reasons this patch replaces i915_cache_level with PAT 
> index. Also
>  >> note, the cache_level is not completely removed yet, because the KMD 
> still
>  >> has the need of creating buffer objects with simple cache settings 
> such as
>  >> cached, uncached, or writethrough. For kernel objects, cache_level 
> is used
>  >> for simplicity and backward compatibility. For Pre-gen12 platforms 
> PAT can
>  >> have 1:1 mapping to i915_cache_level, so these two are 
> interchangeable. see
>  >> the use of LEGACY_CACHELEVEL.
>  >>
>  >> One consequence of this change is that gen8_pte_encode is no longer 
> working
>  >> for gen12 platforms due to the fact that gen12 platforms has 
> different PAT
>  >> definitions. In the meantime the mtl_pte_encode introduced 
> specfically for
>  >> MTL becomes generic for all gen12 platforms. This patch renames the MTL
>  >> PTE encode function into gen12_pte_encode and apply it to all gen12. 
> Even
>  >> though this change looks unrelated, but separating them would 
> temporarily
>  >> break gen12 PTE encoding, thus squash them in one patch.
>  >>
>  >> Special note: this patch changes the way caching behavior is 
> controlled in
>  >> the sense that some objects are left to be managed by userspace. For 
> such
>  >> objects we need to be careful not to change the userspace settings.There
>  >> are kerneldoc and comments added around obj->cache_coherent, 
> cache_dirty,
>  >> and how to bypass the checkings by i915_gem_object_has_cache_level. For
>  >> full understanding, these changes need to be looked at together with the
>  >> two follow-up patches, one disables the {set|get}_caching ioctl's 
> and the
>  >> other adds set_pat extension to the GEM_CREATE uAPI.
>  >>
>  >> Bspec: 63019
>  >>
>  >> Cc: Chris Wilson <chris.p.wilson@linux.intel.com>
>  >> Signed-off-by: Fei Yang <fei.yang@intel.com>
>  >> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
>  >> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

[snip]

>  >> +                                          node.start,
>  >> +                                          i915_gem_get_pat_index(i915,
>  >> +                                                                 
> I915_CACHE_NONE), 0);
>  >>                        wmb(); /* flush modifications to the GGTT 
> (insert_page) */
>  >>                } else {
>  >>                        page_base += offset & PAGE_MASK;
>  >> @@ -1142,6 +1148,19 @@ int i915_gem_init(struct drm_i915_private 
> *dev_priv)
>  >>        unsigned int i;
>  >>        int ret;
>  >>
>  >> +     /*
>  >> +      * In the proccess of replacing cache_level with pat_index a 
> tricky
>  >> +      * dependency is created on the definition of the enum 
> i915_cache_level.
>  >> +      * in case this enum is changed, PTE encode would be broken.
>  >
>  >_I_n
> 
> Sorry, what does this mean?

Start of sentence, capital 'i'.

[snip]

>  > With a pinky promise to improve this all in the near future I won't
>  > grumble to loudly. :) I haven't read all the details, I leave that to
>  > other reviewers, and also assuming some final tweaks as indicated above
>  > please.
> 
> Thanks for all the suggestions, really appreciated.
> May I add your Acked-by?

I can't make myself do it since I really don't like the design that 
much. That's why I said I will not grumble too loudly.

Jira for follow up clean since we both agreed something more elegant is 
possible would be appreciated though.

Regards,

Tvrtko