|
> Subject: Re: [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 |
