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

From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: fei.yang@intel.com, intel-gfx@lists.freedesktop.org
Cc: Chris Wilson <chris.p.wilson@linux.intel.com>,
	Matt Roper <matthew.d.roper@intel.com>,
	dri-devel@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 7/8] drm/i915: use pat_index instead of cache_level
Date: Fri, 21 Apr 2023 11:17:00 +0100	[thread overview]
Message-ID: <705b5e5b-78cd-857f-ad26-80835217520c@linux.intel.com> (raw)
In-Reply-To: <20230419230058.2659455-8-fei.yang@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]

>   
>   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 after having set the PAT index explicitly this will make it silently succeed regardless of the cache level passed in, no? Because of:

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

Regards,

Tvrtko