Re: [Intel-gfx] [PATCH v5 2/5] drm/i915: use pat_index instead of cache_level

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

On 04/05/2023 00:02, 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]

> diff --git a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> index bb6998d67133..f2334a713c4e 100644
> --- a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> +++ b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> @@ -56,7 +56,7 @@ static u64 gen8_pte_encode(dma_addr_t addr,
>   }

^^^

How come there are no changes to gen8_pte_encode?

vvv

>   
>   static u64 mtl_pte_encode(dma_addr_t addr,
> -			  enum i915_cache_level level,
> +			  unsigned int pat_index,
>   			  u32 flags)

Prototype and implementation changed here for mtl_pte_encode.

And we have:

	if (GRAPHICS_VER_FULL(gt->i915) >= IP_VER(12, 70))
		ppgtt->vm.pte_encode = mtl_pte_encode;
	else
		ppgtt->vm.pte_encode = gen8_pte_encode;

So should be same prototype. And:

  	u64 (*pte_encode)(dma_addr_t addr,
-			  enum i915_cache_level level,
+			  unsigned int pat_index,
  			  u32 flags); /* Create a valid PTE */

Patch relies on the compiler considering enum equal to unsigned int?

But the implementation of gen8_pte_encode and most ggtt counterparts is 
looking at the passed in pat index and thinks it is cache level.

How is that supposed to work?! Or I am blind and am missing something?

Regards,

Tvrtko