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

From: "Yang, Fei" <fei.yang@intel.com>
To: "Roper, Matthew D" <matthew.d.roper@intel.com>
Cc: "intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"De Marchi, Lucas" <lucas.demarchi@intel.com>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>
Subject: RE: [Intel-gfx] [PATCH 1/8] drm/i915/mtl: Define MOCS and PAT tables for MTL
Date: Tue, 11 Apr 2023 21:54:41 +0000	[thread overview]
Message-ID: <BYAPR11MB2567BAE00F7E97F7A37A6F159A9A9@BYAPR11MB2567.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20230411192847.GY4085390@mdroper-desk1.amr.corp.intel.com>

[-- Attachment #1: Type: text/plain, Size: 5142 bytes --]

> Subject: Re: [Intel-gfx] [PATCH 1/8] drm/i915/mtl: Define MOCS and PAT tables for MTL
>
> On Mon, Apr 10, 2023 at 08:55:16PM -0700, Yang, Fei wrote:
> ...
>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h
>>
>>>> b/drivers/gpu/drm/i915/gt/intel_gtt.h
>>
>>>> index 69ce55f517f5..b632167eaf2e 100644
>>
>>>> --- a/drivers/gpu/drm/i915/gt/intel_gtt.h
>>
>>>> +++ b/drivers/gpu/drm/i915/gt/intel_gtt.h
>>
>>>> @@ -88,9 +88,18 @@ typedef u64 gen8_pte_t;
>>
>>>>  #define BYT_PTE_SNOOPED_BY_CPU_CACHES             REG_BIT(2)
>>
>>>>  #define BYT_PTE_WRITEABLE                            REG_BIT(1)
>>
>>>>
>>
>>>> +#define GEN12_PPGTT_PTE_PAT3    BIT_ULL(62)
>>
>>>>  #define GEN12_PPGTT_PTE_LM          BIT_ULL(11)
>>
>>>> +#define GEN12_PPGTT_PTE_PAT2    BIT_ULL(7)
>>
>>>> +#define GEN12_PPGTT_PTE_NC      BIT_ULL(5)
>>
>>>> +#define GEN12_PPGTT_PTE_PAT1    BIT_ULL(4)
>>
>>>> +#define GEN12_PPGTT_PTE_PAT0    BIT_ULL(3)
>>
>>> Which bspec page is this from?  The PPGTT descriptions in
>>
>>> the bspec are kind of hard to track down.
>>
>>
>>
>>    [9]https://gfxspecs.intel.com/Predator/Home/Index/53521
>
> The bspec tagging is a bit bizarre in this area, but I don't believe
> this page is intended to apply to MTL. Note that this page is inside
> a section specifically listed as "57b VA Support" --- i.e., this
> general section is for platforms like PVC rather than MTL.  MTL only
> has 48b virtual address space (bspec 55416), so I think one of the
> pages in the 48b sections is what we should be referencing instead.
>
> If they screwed up and put MTL-specific details only on a PVC-specific
> page of the bspec, we should probably file a bspec issue about that to
> get it fixed.

The Bspec is a bit confusing on these. Looked at the Bsec with filter set
to TGL/ADL/MTL/ALL respectively. Here are the differences,
>>    PAT_Index[2:0] = {PAT, PCD, PWT} = BIT(7, 4, 3)

These 3 PAT index bits are defined for all gen12+.
>>    PAT_Index[3] = BIT(62)

PAT_Index[3] is defined for MTL/ARL, will update this one to MTL_xxx

>>    PAT_Index[4] = BIT(61)

PAT_Index[4] shows up only when there is no filter set. And this bit is
marked as [NOT VALID FOR SPEC: GENERALASSETSXE], not sure how to interpret
this, but seems like it should not be used at all. Any suggestion?

>>
>>
>>> But if these only apply to MTL, why are they labelled as "GEN12?"
>>
>>    These apply to GEN12plus.
>
> That's not what your patch is doing; you're using them in a function
> that only gets called on MTL.

That PTE encode will be generalized to gen12 in a patch after after the
pat_index change.

> So the question is whether these
> definitions truly applied to older platforms like TGL/RKL/ADL/etc too
> (and we need to go back and fix that code), or whether they're
> Xe_LPG-specific, in which case the "GEN12_" prefix is incorrect.

The only difference is that MTL has PAT[3] defined, so we can still apply
the same PTE encode function for all gen12+.

> Also, handling the MTL-specific PTE encoding later in the series, after
> the transition from cache_level to PAT index, might be best since then
> you can just implement it correctly at the time the code is introduced;
> no need to add the cache_level implementation first (which can't even
> use all the bits) just to come back a few patches later and replace it
> all with PAT code.

I will squash the PTE encode patches.

>>>> -#define GEN12_GGTT_PTE_LM           BIT_ULL(1)
>>>> +#define GEN12_GGTT_PTE_LM                         BIT_ULL(1)
>>>> +#define MTL_GGTT_PTE_PAT0                          BIT_ULL(52)
>>>> +#define MTL_GGTT_PTE_PAT1                          BIT_ULL(53)
>>>> +#define GEN12_GGTT_PTE_ADDR_MASK       GENMASK_ULL(45, 12)
>>>> +#define MTL_GGTT_PTE_PAT_MASK          GENMASK_ULL(53, 52)
>>>>
>>>>  #define GEN12_PDE_64K BIT(6)
>>>>  #define GEN12_PTE_PS64 BIT(8)
>>>> @@ -147,6 +156,15 @@ typedef u64 gen8_pte_t;  #define
>> GEN8_PDE_IPS_64K
>>
>>>> BIT(11)
>>
>>>>  #define GEN8_PDE_PS_2M   BIT(7)
>>
>>>> +#define MTL_PPAT_L4_CACHE_POLICY_MASK
>> REG_GENMASK(3, 2)
>>>> +#define MTL_PAT_INDEX_COH_MODE_MASK              REG_GENMASK(1,
>> 0)
>>>> +#define MTL_PPAT_L4_3_UC
>>    REG_FIELD_PREP(MTL_PPAT_L4_CACHE_POLICY_MASK, 3)
>>>> +#define MTL_PPAT_L4_1_WT
>>    REG_FIELD_PREP(MTL_PPAT_L4_CACHE_POLICY_MASK, 1)
>>>> +#define MTL_PPAT_L4_0_WB
>>    REG_FIELD_PREP(MTL_PPAT_L4_CACHE_POLICY_MASK, 0)
>>>> +#define MTL_3_COH_2W     REG_FIELD_PREP(MTL_PAT_INDEX_COH_MODE_MASK,
>>    3)
>>>> +#define MTL_2_COH_1W     REG_FIELD_PREP(MTL_PAT_INDEX_COH_MODE_MASK,
>>    2)
>>>> +#define MTL_0_COH_NON
>> REG_FIELD_PREP(MTL_PAT_INDEX_COH_MODE_MASK, 0)
>>>
>>> The values for these definitions don't seem to be aligned.
>>    These are aligned with
>>    [10]https://gfxspecs.intel.com/Predator/Home/Index/45101
> I mean spacing aligned.  If your tabstops are set to 8, then the values don't line up visually.

Hmm... the three COH macro's are aligned, are you saying they should aligned with those PPAT macro's as well?

>
> Matt

[-- Attachment #2: Type: text/html, Size: 14689 bytes --]