On 07-12-2022 23:41, Matt Roper wrote: > On Wed, Dec 07, 2022 at 12:56:44PM +0530, Iddamsetty, Aravind wrote: >> >> >> On 07-12-2022 05:09, Matt Roper wrote: >>> On Tue, Dec 06, 2022 at 01:07:28PM +0530, Aravind Iddamsetty wrote: >>>> Add a separate PTE encode function for MTL. The number of PAT registers >>>> have increased to 16 on MTL. All 16 PAT registers are available for >>>> PPGTT mapped pages, but only the lower 4 are available for GGTT mapped >>>> pages. >>>> >>>> BSPEC: 63884 >>> >>> I think you'll also want to include pages like 45015 (ggtt) and its >>> various equivalents for ppgtt since that's where the important layout >>> information is given. And likely 63019 as well. >>> >>>> >>>> Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx> >>>> Cc: Matt Roper <matthew.d.roper@xxxxxxxxx> >>>> Co-developed-by: Fei Yang <fei.yang@xxxxxxxxx> >>>> Signed-off-by: Fei Yang <fei.yang@xxxxxxxxx> >>>> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@xxxxxxxxx> >>>> --- >>>> drivers/gpu/drm/i915/gt/gen8_ppgtt.c | 33 +++++++++++++++++++++++++++- >>>> drivers/gpu/drm/i915/gt/gen8_ppgtt.h | 4 ++++ >>>> drivers/gpu/drm/i915/gt/intel_ggtt.c | 32 ++++++++++++++++++++++++++- >>>> drivers/gpu/drm/i915/gt/intel_gtt.h | 13 +++++++++-- >>>> 4 files changed, 78 insertions(+), 4 deletions(-) >>>> >> >> <snip> >>>> + >>>> + switch (level) { >>>> + case I915_CACHE_NONE: >>>> + pte |= GEN12_PPGTT_PTE_PAT1; >>>> + break; >>>> + case I915_CACHE_LLC: >>>> + case I915_CACHE_L3_LLC: >>>> + pte |= GEN12_PPGTT_PTE_PAT0 | GEN12_PPGTT_PTE_PAT1; >>>> + break; >>>> + case I915_CACHE_WT: >>>> + pte |= GEN12_PPGTT_PTE_PAT0; >>>> + break; >>>> + } >>> >>> I forget what the plan was...are we going to move away from 'enum >>> i915_cache_level' and start working with PAT indices directly soon >>> (especially since the set_caching/get_caching ioctls are getting axed >>> and vm_bind is supposed to start taking platform-specific indicies >>> directly)? If we're still using cache_level, then it's not clear to me >>> how the current platform-agnostic enum values (which talk about L3 and >>> LLC) are supposed to encode the L4 behavior we want on MTL. It seems >>> like we'd need to extend the enum to also somehow reflect L4 behavior if >>> we were going to keep using it? But given the continuing expansion of >>> caching functionality and complexity, I thought that was one of the >>> reasons why we wanted to get away from these platform-agnostic enums; >>> the userspace that actually cares about this stuff has the same PAT/MOCS >>> tables we do and knows the exact index it wants to use for an object >>> mapping, so eliminating the PAT idx -> cache_level -> PAT idx dance >>> would cut out a bunch of confusion. >> >> The current plan is not to expose PAT index setting via VM_BIND but go >> with the defaults. Hence using the i915_cache_level till we decide on >> enabling PAT index setting via VM_BIND. >> >> Also, IIUC the cache level we have in i915 apply to L4 as well (BSPEC 45101) >> >> I915_CACHE_NONE -> UC >> I915_CACHE_LLC/I915_CACHE_L3_LLC -> WB >> I915_CACHE_WT-> WT >> >> But I do not see a means why which we'll know that L4 cache is present >> on the platform to select the appropriate cache level. > > I may be misunderstanding since the caching isn't an area I've > worked with much in the past, from reading the kerneldoc descriptions on > this enum, it sounds like I915_CACHE_LLC would be be COH_2W? And > I915_CACHE_L3_LLC COH_1W? It looks like you're programming both as PAT > index 3 (i.e., 1W coherency) right now, which confuses me. Rereading the descriptions makes me feel what you mentioned is right. Also for I915_CACHE_L3_LLC i see a note it is considered only uptil gen7 so i believe this needn't be considered for MTL. @Matt Auld, could you please confirm on this. Thanks, Aravind. > >> >>> >>> It's also hard to follow these functions right now because it looks like >>> you're doing an implicit cache_level -> PAT index conversion, but also >>> mapping the PAT index bits into their placement in the PTE as part of >>> the same operation. The behavior might turn out to be correct, but it's >>> really hard to follow the process, even with all the bspec docs at hand. >>> So if we do keep using cache_level for now, I think it would be better >>> to split out a MTL function to translate cache level into PAT index >>> (which we can review independently) and then let these pte_encode >>> functions handle the next step of figuring out where those index bits >>> should land in the PTE. If the bits are contiguous, you can also just >>> define a mask and use REG_FIELD_PREP too. >> >> sure i'll translate cache_level to PAT index and then program the PTE >> using those. >> >>> >>>> + >>>> + return pte; >>>> +} >>>> + >>>> static void gen8_ppgtt_notify_vgt(struct i915_ppgtt *ppgtt, bool create) >>>> { >>>> struct drm_i915_private *i915 = ppgtt->vm.i915; >>>> @@ -963,7 +991,10 @@ struct i915_ppgtt *gen8_ppgtt_create(struct intel_gt *gt, >>>> */ >>>> ppgtt->vm.alloc_scratch_dma = alloc_pt_dma; >>>> >>>> - ppgtt->vm.pte_encode = gen8_pte_encode; >>>> + if (GRAPHICS_VER_FULL(gt->i915) >= IP_VER(12, 70)) >>>> + ppgtt->vm.pte_encode = mtl_pte_encode; >>>> + else >>>> + ppgtt->vm.pte_encode = gen8_pte_encode; >>>> >>>> ppgtt->vm.bind_async_flags = I915_VMA_LOCAL_BIND; >>>> ppgtt->vm.insert_entries = gen8_ppgtt_insert; >>>> diff --git a/drivers/gpu/drm/i915/gt/gen8_ppgtt.h b/drivers/gpu/drm/i915/gt/gen8_ppgtt.h >>>> index f541d19264b4..c48f1fc32909 100644 >>>> --- a/drivers/gpu/drm/i915/gt/gen8_ppgtt.h >>>> +++ b/drivers/gpu/drm/i915/gt/gen8_ppgtt.h >>>> @@ -19,4 +19,8 @@ u64 gen8_ggtt_pte_encode(dma_addr_t addr, >>>> enum i915_cache_level level, >>>> u32 flags); >>>> >>>> +u64 mtl_ggtt_pte_encode(dma_addr_t addr, >>>> + enum i915_cache_level level, >>>> + u32 flags); >>>> + >>>> #endif >>>> diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c >>>> index 82203ad85b0e..3b6f1f6f780a 100644 >>>> --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c >>>> +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c >>>> @@ -246,6 +246,33 @@ static void guc_ggtt_invalidate(struct i915_ggtt *ggtt) >>>> } >>>> } >>>> >>>> +u64 mtl_ggtt_pte_encode(dma_addr_t addr, >>>> + enum i915_cache_level level, >>>> + u32 flags) >>>> +{ >>>> + gen8_pte_t pte = addr | GEN8_PAGE_PRESENT; >>>> + >>>> + GEM_BUG_ON(addr & ~GEN12_GGTT_PTE_ADDR_MASK); >>>> + >>>> + if (flags & PTE_LM) >>>> + pte |= GEN12_GGTT_PTE_LM; >>>> + >>>> + switch (level) { >>>> + case I915_CACHE_NONE: >>>> + pte |= MTL_GGTT_PTE_PAT1; >>>> + break; >>>> + case I915_CACHE_LLC: >>>> + case I915_CACHE_L3_LLC: >>>> + pte |= MTL_GGTT_PTE_PAT0 | MTL_GGTT_PTE_PAT1; >>>> + break; >>>> + case I915_CACHE_WT: >>>> + pte |= MTL_GGTT_PTE_PAT0; >>>> + break; >>>> + } >>>> + >>>> + return pte; >>>> +} >>>> + >>>> u64 gen8_ggtt_pte_encode(dma_addr_t addr, >>>> enum i915_cache_level level, >>>> u32 flags) >>>> @@ -993,7 +1020,10 @@ static int gen8_gmch_probe(struct i915_ggtt *ggtt) >>>> ggtt->vm.vma_ops.bind_vma = intel_ggtt_bind_vma; >>>> ggtt->vm.vma_ops.unbind_vma = intel_ggtt_unbind_vma; >>>> >>>> - ggtt->vm.pte_encode = gen8_ggtt_pte_encode; >>>> + if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 70)) >>>> + ggtt->vm.pte_encode = mtl_ggtt_pte_encode; >>>> + else >>>> + ggtt->vm.pte_encode = gen8_ggtt_pte_encode; >>>> >>>> return ggtt_probe_common(ggtt, size); >>>> } >>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h b/drivers/gpu/drm/i915/gt/intel_gtt.h >>>> index 8a3e0a6793dd..4bb7a4005452 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_GGTT_PTE_LM BIT_ULL(1) >>>> +#define GEN12_PPGTT_PTE_PAT2 BIT_ULL(7) >>> >>> This bit is never used anywhere in the patch. >> correct the default cache level we have will map uptil PAT index 3 hence >> didn't use it and since platform supports it and in future when we have >> PAT index setting this will be used. >>> >>>> +#define GEN12_PPGTT_PTE_NC BIT_ULL(5) >>> >>> As noted above, >>> >>>> +#define GEN12_PPGTT_PTE_PAT1 BIT_ULL(4) >>>> +#define GEN12_PPGTT_PTE_PAT0 BIT_ULL(3) >>> >>> It sounds like these bits have been around since gen12; why didn't we >>> ever have to program them in the past? Is there something that causes >>> the PAT index to never get used on the pre-MTL platforms? >> these are mapped to _PAGE_PWT, _PAGE_PCD and being programmed in >> gen8_pte_encode. On the MTL we have new PAT bits in PTE and since the >> way these bits are programmed is different redefined for better >> understanding. > > In that case why does it still have a GEN12_ prefix? We should use > "MTL_" instead since this doesn't apply to any of the platforms that > used to be known as "gen12." > > > Matt > >> >> Thanks. >> Aravind. >> >> <snip> >