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

[Yang, Fei]

> On 04/05/2023 00:02, 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. 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.
>>
>> 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 such simple cases, using cache_level
>> would help simplify the code.
>>
>> Cc: Chris Wilson <chris.p.wilson at linux.intel.com>
>> Cc: Matt Roper <matthew.d.roper at intel.com>
>> Signed-off-by: Fei Yang <fei.yang at intel.com>
>> Reviewed-by: Andi Shyti <andi.shyti at linux.intel.com>
>
> [snip]
>
>> diff --git a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
>> index bb6998d67133..f2334a713c4e 100644
>> --- a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
>> +++ b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
>> @@ -56,7 +56,7 @@ static u64 gen8_pte_encode(dma_addr_t addr,
>>   }
>
>^^^
>
> How come there are no changes to gen8_pte_encode?

For legacy platforms cache_level is equal to pat_index, so I was thinking
more about reducing number of lines changed.

>vvv
>
>>
>>   static u64 mtl_pte_encode(dma_addr_t addr,
>> -                       enum i915_cache_level level,
>> +                       unsigned int pat_index,
>>                          u32 flags)
>
> Prototype and implementation changed here for mtl_pte_encode.
>
> And we have:
>
>        if (GRAPHICS_VER_FULL(gt->i915) >= IP_VER(12, 70))
>                ppgtt->vm.pte_encode = mtl_pte_encode;
>        else
>                ppgtt->vm.pte_encode = gen8_pte_encode;
>
> So should be same prototype. And:
>
>         u64 (*pte_encode)(dma_addr_t addr,
>-                         enum i915_cache_level level,
>+                         unsigned int pat_index,
>                           u32 flags); /* Create a valid PTE */
>
> Patch relies on the compiler considering enum equal to unsigned int?

yes, caller is passing in unsigned int and gets used as enum.

> But the implementation of gen8_pte_encode and most ggtt counterparts is
> looking at the passed in pat index and thinks it is cache level.
>
> How is that supposed to work?! Or I am blind and am missing something?

For legacy platforms translation through LEGACY_CACHELEVEL would not
change the value of cache_level. The cache_level and pat_index are basically
the same for these platforms.

It is broken for gen12 here. I was asked to separate the gen12_pte_encode
change to another patch in the series, but that breaks bisect. Should I
squash 2/5 and 3/5?

> Regards,
>
> Tvrtko

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20230504/79d59de2/attachment.htm>