> 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
