> 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/
Will update
>[...]
>
>> +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.
Will update
>[...]
>
>> @@ -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.
Will update.
>> + } 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?
I would prefer keeping it in this patch because the function is called by
intel_mocs_init(). Without it the MOCS initialization would be broken for
media GT.
> Andi
