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

From: "Yang, Fei" <fei.yang@intel.com>
To: Andi Shyti <andi.shyti@linux.intel.com>
Cc: "Roper, Matthew D" <matthew.d.roper@intel.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"De Marchi, Lucas" <lucas.demarchi@intel.com>,
	Madhumitha Tolakanahalli Pradeep
	<madhumitha.tolakanahalli.pradeep@intel.com>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>
Subject: RE: [Intel-gfx] [PATCH 2/8] drm/i915/mtl: Define MOCS and PAT tables for MTL
Date: Wed, 19 Apr 2023 16:00:28 +0000	[thread overview]
Message-ID: <SN6PR11MB2574A2515A7ABC4639252F519A629@SN6PR11MB2574.namprd11.prod.outlook.com> (raw)
In-Reply-To: <ZD/KFgdE4mtvKCPJ@ashyti-mobl2.lan>

> 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/

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