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

[Yang, Fei]

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

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