Re: [Intel-gfx] [PATCH 2/8] drm/i915/mtl: Define MOCS and PAT tables for MTL

[Andi Shyti <andi.shyti@linux.intel.com>]

Hi Fei,

On Sun, Apr 16, 2023 at 11:24:57PM -0700, fei.yang@intel.com wrote:
> From: Madhumitha Tolakanahalli Pradeep <madhumitha.tolakanahalli.pradeep@intel.com>
> 
> On MTL, GT can no longer allocate on LLC - only the CPU can.
> This, along with addition of support for L4 cache calls a
> MOCS/PAT table update.
> Alos the PAT index registers are multicasted for primary GT,

/Alos/Also/

[...]

> +static void xelpmp_setup_private_ppat(struct intel_uncore *uncore)
> +{
> +	intel_uncore_write(uncore, XELPMP_PAT_INDEX(0), MTL_PPAT_L4_0_WB);
> +	intel_uncore_write(uncore, XELPMP_PAT_INDEX(1), MTL_PPAT_L4_1_WT);
> +	intel_uncore_write(uncore, XELPMP_PAT_INDEX(2), MTL_PPAT_L4_3_UC);
> +	intel_uncore_write(uncore, XELPMP_PAT_INDEX(3),
> +			   MTL_PPAT_L4_0_WB | MTL_2_COH_1W);
> +	intel_uncore_write(uncore, XELPMP_PAT_INDEX(4),
> +			   MTL_PPAT_L4_0_WB | MTL_3_COH_2W);

nit: I think it's more readable if we either keep everything in
one line or we break the line for everyone. Even if we break the
80 characters rule.

[...]

> @@ -603,16 +639,22 @@ void setup_private_pat(struct intel_gt *gt)
>  
>  	GEM_BUG_ON(GRAPHICS_VER(i915) < 8);
>  
> -	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50))
> -		xehp_setup_private_ppat(gt);
> -	else if (GRAPHICS_VER(i915) >= 12)
> -		tgl_setup_private_ppat(uncore);
> -	else if (GRAPHICS_VER(i915) >= 11)
> -		icl_setup_private_ppat(uncore);
> -	else if (IS_CHERRYVIEW(i915) || IS_GEN9_LP(i915))
> -		chv_setup_private_ppat(uncore);
> -	else
> -		bdw_setup_private_ppat(uncore);
> +	if (gt->type == GT_MEDIA) {
> +		xelpmp_setup_private_ppat(gt->uncore);

nit: if you add a return here you save the else.

> +	} else {
> +		if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 70))
> +			xelpg_setup_private_ppat(gt);
> +		else if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50))
> +			xehp_setup_private_ppat(gt);
> +		else if (GRAPHICS_VER(i915) >= 12)
> +			tgl_setup_private_ppat(uncore);
> +		else if (GRAPHICS_VER(i915) >= 11)
> +			icl_setup_private_ppat(uncore);
> +		else if (IS_CHERRYVIEW(i915) || IS_GEN9_LP(i915))
> +			chv_setup_private_ppat(uncore);
> +		else
> +			bdw_setup_private_ppat(uncore);
> +	}

[...]

> -static u32 global_mocs_offset(void)
> +static u32 global_mocs_offset(struct intel_gt *gt)
>  {
> -	return i915_mmio_reg_offset(GEN12_GLOBAL_MOCS(0));
> +	return i915_mmio_reg_offset(GEN12_GLOBAL_MOCS(0)) + gt->uncore->gsi_offset;

There is one open question here coming from one of previous
Matt's reviews. Would it make sense to have this in a different
patch?

Andi