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

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

On 23/04/2023 07:12, Yang, Fei wrote:
>> On 20/04/2023 00:00, 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.
>>>
>>> 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@linux.intel.com>
>>> Cc: Matt Roper <matthew.d.roper@intel.com>
>>> Signed-off-by: Fei Yang <fei.yang@intel.com>
>>> Reviewed-by: Andi Shyti <andi.shyti@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.

Not on all platforms.

>> 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.

Why would that be a reason to lie to it? What would would be the problem 
with telling it of the mistake?

>> +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.

IMO when return values can be misleading that means the API is not great.

I don't see a good reason to lie to both in kernel callers and to 
userspace (set_caching).

Regards,

Tvrtko