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

[Yang, Fei]

> On 20/04/2023 00:00, 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]
>
>>
>>   bool i915_gem_cpu_write_needs_clflush(struct drm_i915_gem_object
>> *obj) @@ -267,7 +267,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
>>   {
>>      int ret;
>>
>> -    if (obj->cache_level == cache_level)
>> +    if (i915_gem_object_has_cache_level(obj, cache_level))
>>              return 0;
>
> When userspace calls i915_gem_set_caching_ioctl

We are ending the support for set_caching_ioctl.

> after having set the PAT index explicitly this will make it silently succeed
> regardless of the cache level passed in, no? Because of:

Yes, that's the point. For objects created by userspace with PAT index set,
KMD is not supposed to touch the setting.

> +bool i915_gem_object_has_cache_level(const struct drm_i915_gem_object *obj,
> +                                  enum i915_cache_level lvl)
> +{
> +     /*
> +      * cache_level == I915_CACHE_INVAL indicates the UMD's have set the
> +      * caching policy through pat_index, in which case the KMD should
> +      * leave the coherency to be managed by user space, simply return
> +      * true here.
> +      */
> +     if (obj->cache_level == I915_CACHE_INVAL)
> +             return true;
>
> I think we need to let it know it is doing it wrong with an error.

This is not an error, by design userspace should know exactly what it's doing.

-Fei

> Regards,
>
> Tvrtko