<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"> <head> <meta http-equiv="Content-Type" content="text/html; charset=us-ascii"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style></style> </head> <body lang="EN-US" link="#0563C1" vlink="#954F72" style="word-wrap:break-word"> <div class="WordSection1"> > Subject: Re: [Intel-gfx] [PATCH 1/8] drm/i915/mtl: Define MOCS and PAT tables for MTL<o:p></o:p> ><o:p> </o:p> > On Mon, Apr 10, 2023 at 08:55:16PM -0700, Yang, Fei wrote:<o:p></o:p> > ...<o:p></o:p> >>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h<o:p></o:p> >><o:p> </o:p> >>>> b/drivers/gpu/drm/i915/gt/intel_gtt.h<o:p></o:p> >><o:p> </o:p> >>>> index 69ce55f517f5..b632167eaf2e 100644<o:p></o:p> >><o:p> </o:p> >>>> --- a/drivers/gpu/drm/i915/gt/intel_gtt.h<o:p></o:p> >><o:p> </o:p> >>>> +++ b/drivers/gpu/drm/i915/gt/intel_gtt.h<o:p></o:p> >><o:p> </o:p> >>>> @@ -88,9 +88,18 @@ typedef u64 gen8_pte_t;<o:p></o:p> >><o:p> </o:p> >>>> #define BYT_PTE_SNOOPED_BY_CPU_CACHES REG_BIT(2)<o:p></o:p> >><o:p> </o:p> >>>> #define BYT_PTE_WRITEABLE REG_BIT(1)<o:p></o:p> >><o:p> </o:p> >>>><o:p> </o:p> >><o:p> </o:p> >>>> +#define GEN12_PPGTT_PTE_PAT3 BIT_ULL(62)<o:p></o:p> >><o:p> </o:p> >>>> #define GEN12_PPGTT_PTE_LM BIT_ULL(11)<o:p></o:p> >><o:p> </o:p> >>>> +#define GEN12_PPGTT_PTE_PAT2 BIT_ULL(7)<o:p></o:p> >><o:p> </o:p> >>>> +#define GEN12_PPGTT_PTE_NC BIT_ULL(5)<o:p></o:p> >><o:p> </o:p> >>>> +#define GEN12_PPGTT_PTE_PAT1 BIT_ULL(4)<o:p></o:p> >><o:p> </o:p> >>>> +#define GEN12_PPGTT_PTE_PAT0 BIT_ULL(3)<o:p></o:p> >><o:p> </o:p> >>> Which bspec page is this from? The PPGTT descriptions in<o:p></o:p> >><o:p> </o:p> >>> the bspec are kind of hard to track down.<o:p></o:p> >> <o:p></o:p> >> <o:p></o:p> >> <o:p></o:p> >> [9]<a href="https://gfxspecs.intel.com/Predator/Home/Index/53521">https://gfxspecs.intel.com/Predator/Home/Index/53521</a><o:p></o:p> ><o:p> </o:p> > The bspec tagging is a bit bizarre in this area, but I don't believe<o:p></o:p> > this page is intended to apply to MTL. Note that this page is inside<o:p></o:p> > a section specifically listed as "57b VA Support" --- i.e., this<o:p></o:p> > general section is for platforms like PVC rather than MTL. MTL only<o:p></o:p> > has 48b virtual address space (bspec 55416), so I think one of the<o:p></o:p> > pages in the 48b sections is what we should be referencing instead.<o:p></o:p> ><o:p> </o:p> > If they screwed up and put MTL-specific details only on a PVC-specific<o:p></o:p> > page of the bspec, we should probably file a bspec issue about that to<o:p></o:p> > get it fixed.<o:p></o:p> <o:p> </o:p> The Bspec is a bit confusing on these. Looked at the Bsec with filter set<o:p></o:p> to TGL/ADL/MTL/ALL respectively. Here are the differences,<o:p></o:p> <o:p></o:p> >> PAT_Index[2:0] = {PAT, PCD, PWT} = BIT(7, 4, 3)<o:p></o:p> <o:p> </o:p> These 3 PAT index bits are defined for all gen12+.<o:p></o:p> <o:p></o:p> >> PAT_Index[3] = BIT(62)<o:p></o:p> <o:p> </o:p> PAT_Index[3] is defined for MTL/ARL, will update this one to MTL_xxx<o:p></o:p> <o:p> </o:p> >> PAT_Index[4] = BIT(61)<o:p></o:p> <o:p> </o:p> PAT_Index[4] shows up only when there is no filter set. And this bit is<o:p></o:p> marked as [NOT VALID FOR SPEC: GENERALASSETSXE], not sure how to interpret<o:p></o:p> this, but seems like it should not be used at all. Any suggestion?<o:p></o:p> <o:p> </o:p> >> <o:p></o:p> >> <o:p></o:p> >>> But if these only apply to MTL, why are they labelled as "GEN12?"<o:p></o:p> >> <o:p></o:p> >> These apply to GEN12plus.<o:p></o:p> ><o:p> </o:p> > That's not what your patch is doing; you're using them in a function<o:p></o:p> > that only gets called on MTL.<o:p></o:p> <o:p> </o:p> That PTE encode will be generalized to gen12 in a patch after after the<o:p></o:p> pat_index change.<o:p></o:p> <o:p> </o:p> > So the question is whether these<o:p></o:p> > definitions truly applied to older platforms like TGL/RKL/ADL/etc too<o:p></o:p> > (and we need to go back and fix that code), or whether they're<o:p></o:p> > Xe_LPG-specific, in which case the "GEN12_" prefix is incorrect.<o:p></o:p> <o:p> </o:p> The only difference is that MTL has PAT[3] defined, so we can still apply<o:p></o:p> the same PTE encode function for all gen12+.<o:p></o:p> <o:p> </o:p> > Also, handling the MTL-specific PTE encoding later in the series, after<o:p></o:p> > the transition from cache_level to PAT index, might be best since then<o:p></o:p> > you can just implement it correctly at the time the code is introduced;<o:p></o:p> > no need to add the cache_level implementation first (which can't even<o:p></o:p> > use all the bits) just to come back a few patches later and replace it<o:p></o:p> > all with PAT code.<o:p></o:p> <o:p> </o:p> I will squash the PTE encode patches.<o:p></o:p> <o:p> </o:p> >>>> -#define GEN12_GGTT_PTE_LM BIT_ULL(1)<o:p></o:p> >>>> +#define GEN12_GGTT_PTE_LM BIT_ULL(1)<o:p></o:p> >>>> +#define MTL_GGTT_PTE_PAT0 BIT_ULL(52)<o:p></o:p> >>>> +#define MTL_GGTT_PTE_PAT1 BIT_ULL(53)<o:p></o:p> >>>> +#define GEN12_GGTT_PTE_ADDR_MASK GENMASK_ULL(45, 12)<o:p></o:p> >>>> +#define MTL_GGTT_PTE_PAT_MASK GENMASK_ULL(53, 52)<o:p></o:p> >>>><o:p> </o:p> >>>> #define GEN12_PDE_64K BIT(6)<o:p></o:p> >>>> #define GEN12_PTE_PS64 BIT(8)<o:p></o:p> >>>> @@ -147,6 +156,15 @@ typedef u64 gen8_pte_t; #define <o:p> </o:p> >> GEN8_PDE_IPS_64K<o:p></o:p> >> <o:p></o:p> >>>> BIT(11)<o:p></o:p> >><o:p> </o:p> >>>> #define GEN8_PDE_PS_2M BIT(7)<o:p></o:p> >><o:p> </o:p> >>>> +#define MTL_PPAT_L4_CACHE_POLICY_MASK <o:p> </o:p> >> REG_GENMASK(3, 2)<o:p></o:p> >>>> +#define MTL_PAT_INDEX_COH_MODE_MASK REG_GENMASK(1, <o:p></o:p> >> 0)<o:p></o:p> >>>> +#define MTL_PPAT_L4_3_UC <o:p></o:p> >> REG_FIELD_PREP(MTL_PPAT_L4_CACHE_POLICY_MASK, 3)<o:p></o:p> >>>> +#define MTL_PPAT_L4_1_WT <o:p></o:p> >> REG_FIELD_PREP(MTL_PPAT_L4_CACHE_POLICY_MASK, 1)<o:p></o:p> >>>> +#define MTL_PPAT_L4_0_WB <o:p></o:p> >> REG_FIELD_PREP(MTL_PPAT_L4_CACHE_POLICY_MASK, 0)<o:p></o:p> >>>> +#define MTL_3_COH_2W REG_FIELD_PREP(MTL_PAT_INDEX_COH_MODE_MASK,<o:p></o:p> >> 3)<o:p></o:p> >>>> +#define MTL_2_COH_1W REG_FIELD_PREP(MTL_PAT_INDEX_COH_MODE_MASK,<o:p></o:p> >> 2)<o:p></o:p> >>>> +#define MTL_0_COH_NON <o:p></o:p> >> REG_FIELD_PREP(MTL_PAT_INDEX_COH_MODE_MASK, 0)<o:p></o:p> >>><o:p> </o:p> >>> The values for these definitions don't seem to be aligned.<o:p></o:p> >> These are aligned with<o:p></o:p> >> [10]<a href="https://gfxspecs.intel.com/Predator/Home/Index/45101">https://gfxspecs.intel.com/Predator/Home/Index/45101</a><o:p></o:p> > I mean spacing aligned. If your tabstops are set to 8, then the values don't line up visually.<o:p></o:p> <o:p> </o:p> Hmm… the three COH macro’s are aligned, are you saying they should aligned with those PPAT macro’s as well?<o:p></o:p> <o:p> </o:p> ><o:p> </o:p> > Matt<o:p></o:p> <o:p> </o:p> </div> </body> </html>