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

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

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]

> @@ -306,20 +304,13 @@ int i915_gem_get_caching_ioctl(struct drm_device *dev, void *data,
>   		goto out;
>   	}
>   
> -	switch (obj->cache_level) {
> -	case I915_CACHE_LLC:
> -	case I915_CACHE_L3_LLC:
> +	if (i915_gem_object_has_cache_level(obj, I915_CACHE_LLC) ||
> +	    i915_gem_object_has_cache_level(obj, I915_CACHE_L3_LLC))
>   		args->caching = I915_CACHING_CACHED;
> -		break;
> -
> -	case I915_CACHE_WT:
> +	else if (i915_gem_object_has_cache_level(obj, I915_CACHE_WT))
>   		args->caching = I915_CACHING_DISPLAY;
> -		break;
> -
> -	default:
> +	else
>   		args->caching = I915_CACHING_NONE;
> -		break;
> -	}
>   out:
>   	rcu_read_unlock();
>   	return err;

[snip]

> +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;
> +
> +	/*
> +	 * Otherwise the pat_index should have been converted from cache_level
> +	 * so that the following comparison is valid.
> +	 */
> +	return obj->pat_index == i915_gem_get_pat_index(obj_to_i915(obj), lvl);
> +}
> +

Isn't i915_gem_get_caching_ioctl always going to report 
I915_CACHING_CACHED if any PAT index has been set?

Not sure if that is okay or not, or if it only needs mentioning in the 
commit, I am still reading through it all.

Regards,

Tvrtko