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. > > PAT_Index[2:0] = {PAT, PCD, PWT} = BIT(7, 4, 3) > > PAT_Index[3] = BIT(62) > > PAT_Index[4] = BIT(61) > > > > > 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. 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. 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. > > > > >> > > >> -#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. Matt -- Matt Roper Graphics Software Engineer Linux GPU Platform Enablement Intel Corporation
