[Intel-gfx] [PATCH] drm/i915/mtl: Add engine TLB invalidation

[Andrzej Hajda]

On 17.02.2023 19:54, Matt Roper wrote:
> MTL's primary GT can continue to use the same engine TLB invalidation
> programming as past Xe_HP-based platforms.  However the media GT needs
> some special handling:
>   * Invalidation registers on the media GT are singleton registers
>     (unlike the primary GT where they are still MCR).
>   * Since the GSC is now exposed as an engine, there's a new register to
>     use for TLB invalidation.  The offset is identical to the compute
>     engine offset, but this is expected --- compute engines only exist on
>     the primary GT while the GSC only exists on the media GT.
>   * Although there's only a single GSC engine instance, it inexplicably
>     uses bit 1 to request invalidations rather than bit 0.
> 
> Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_engine_cs.c | 52 ++++++++++++++++-------
>   drivers/gpu/drm/i915/gt/intel_gt_regs.h   |  1 +
>   2 files changed, 38 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index f3a91e7f85f7..af8e158fbd84 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -1166,6 +1166,11 @@ static int intel_engine_init_tlb_invalidation(struct intel_engine_cs *engine)
>   		[COPY_ENGINE_CLASS].mcr_reg	  = XEHP_BLT_TLB_INV_CR,
>   		[COMPUTE_CLASS].mcr_reg		  = XEHP_COMPCTX_TLB_INV_CR,
>   	};
> +	static const union intel_engine_tlb_inv_reg xelpmp_regs[] = {
> +		[VIDEO_DECODE_CLASS].reg	  = GEN12_VD_TLB_INV_CR,
> +		[VIDEO_ENHANCEMENT_CLASS].reg     = GEN12_VE_TLB_INV_CR,
> +		[OTHER_CLASS].reg		  = XELPMP_GSC_TLB_INV_CR,
> +	};
>   	struct drm_i915_private *i915 = engine->i915;
>   	const unsigned int instance = engine->instance;
>   	const unsigned int class = engine->class;
> @@ -1185,19 +1190,28 @@ static int intel_engine_init_tlb_invalidation(struct intel_engine_cs *engine)
>   	 * 12.00 -> 12.50 transition multi cast handling is required too.
>   	 */
>   
> -	if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 50) ||
> -	    GRAPHICS_VER_FULL(i915) == IP_VER(12, 55)) {
> -		regs = xehp_regs;
> -		num = ARRAY_SIZE(xehp_regs);
> -	} else if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 0) ||
> -		   GRAPHICS_VER_FULL(i915) == IP_VER(12, 10)) {
> -		regs = gen12_regs;
> -		num = ARRAY_SIZE(gen12_regs);
> -	} else if (GRAPHICS_VER(i915) >= 8 && GRAPHICS_VER(i915) <= 11) {
> -		regs = gen8_regs;
> -		num = ARRAY_SIZE(gen8_regs);
> -	} else if (GRAPHICS_VER(i915) < 8) {
> -		return 0;
> +	if (engine->gt->type == GT_MEDIA) {
> +		if (MEDIA_VER_FULL(i915) == IP_VER(13, 0)) {
> +			regs = xelpmp_regs;
> +			num = ARRAY_SIZE(xelpmp_regs);
> +		}

As I understand currently only GEN13 of media can have GT_MEDIA, so 
fallback to gt_WARN_ONCE below is expected behavior.

> +	} else {
> +		if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 71) ||

12.71 is not yet present in i915_pci.c, maybe they should be added 
together, up to you.

> +		    GRAPHICS_VER_FULL(i915) == IP_VER(12, 70)  > +		    GRAPHICS_VER_FULL(i915) == IP_VER(12, 50) ||
> +		    GRAPHICS_VER_FULL(i915) == IP_VER(12, 55)) {
> +			regs = xehp_regs;
> +			num = ARRAY_SIZE(xehp_regs);
> +		} else if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 0) ||
> +			   GRAPHICS_VER_FULL(i915) == IP_VER(12, 10)) {
> +			regs = gen12_regs;
> +			num = ARRAY_SIZE(gen12_regs);
> +		} else if (GRAPHICS_VER(i915) >= 8 && GRAPHICS_VER(i915) <= 11) {
> +			regs = gen8_regs;
> +			num = ARRAY_SIZE(gen8_regs);
> +		} else if (GRAPHICS_VER(i915) < 8) {
> +			return 0;
> +		}
>   	}
>   
>   	if (gt_WARN_ONCE(engine->gt, !num,
> @@ -1212,7 +1226,14 @@ static int intel_engine_init_tlb_invalidation(struct intel_engine_cs *engine)
>   
>   	reg = regs[class];
>   
> -	if (regs == gen8_regs && class == VIDEO_DECODE_CLASS && instance == 1) {
> +	if (class == OTHER_CLASS) {

Maybe it would be safer:
	if (regs == xelpmp_regs && class == OTHER_CLASS)


> +		/*
> +		 * There's only a single GSC instance, but it uses register bit
> +		 * 1 instead of either 0 or OTHER_GSC_INSTANCE.
> +		 */
> +		GEM_WARN_ON(instance != OTHER_GSC_INSTANCE);
> +		val = 1;
> +	} else if (regs == gen8_regs && class == VIDEO_DECODE_CLASS && instance == 1) {
>   		reg.reg = GEN8_M2TCR;
>   		val = 0;
>   	} else {
> @@ -1228,7 +1249,8 @@ static int intel_engine_init_tlb_invalidation(struct intel_engine_cs *engine)
>   	if (GRAPHICS_VER(i915) >= 12 &&
>   	    (engine->class == VIDEO_DECODE_CLASS ||
>   	     engine->class == VIDEO_ENHANCEMENT_CLASS ||
> -	     engine->class == COMPUTE_CLASS))
> +	     engine->class == COMPUTE_CLASS ||
> +	     engine->class == OTHER_CLASS))
This is little surprise, I would expect non-masked reg for single 
instance, but it follows bspec, so OK.

Reviewed-by: Andrzej Hajda <andrzej.hajda at intel.com>

Regards
Andrzej

>   		engine->tlb_inv.request = _MASKED_BIT_ENABLE(val);
>   	else
>   		engine->tlb_inv.request = val;
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> index 416976d396ba..423e3e9c564b 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> @@ -1090,6 +1090,7 @@
>   #define XEHP_BLT_TLB_INV_CR			MCR_REG(0xcee4)
>   #define GEN12_COMPCTX_TLB_INV_CR		_MMIO(0xcf04)
>   #define XEHP_COMPCTX_TLB_INV_CR			MCR_REG(0xcf04)
> +#define XELPMP_GSC_TLB_INV_CR			_MMIO(0xcf04)   /* media GT only */
>   
>   #define XEHP_MERT_MOD_CTRL			MCR_REG(0xcf28)
>   #define RENDER_MOD_CTRL				MCR_REG(0xcf2c)