Hi Fei,
On Sun, Apr 16, 2023 at 11:24:57PM -0700, fei.yang@xxxxxxxxx wrote:
> From: Madhumitha Tolakanahalli Pradeep <madhumitha.tolakanahalli.pradeep@xxxxxxxxx>
>
> 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
