On 09/05/2023 18:12, Yang, Fei wrote:
> On 09/05/2023 00:48, 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. 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@xxxxxxxxxxxxxxx>
>> Signed-off-by: Fei Yang <fei.yang@xxxxxxxxx>
>> Reviewed-by: Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>
>> Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
[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
