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