[Intel-gfx] [PATCH 3/8] drm/i915/mtl: Add PTE encode function

[Matt Roper]

On Sun, Apr 23, 2023 at 12:37:27AM -0700, Yang, Fei wrote:
> > On Fri, Apr 21, 2023 at 10:27:22AM -0700, Yang, Fei wrote:
> >>> On Wed, Apr 19, 2023 at 04:00:53PM -0700, fei.yang at intel.com wrote:
> >>>> From: Fei Yang <fei.yang at intel.com>
> >>>>
> >>>> PTE encode functions are platform dependent. This patch implements
> >>>> PTE functions for MTL, and ensures the correct PTE encode function
> >>>> is used by calling pte_encode function pointer instead of the
> >>>> hardcoded gen8 version of PTE encode.
> >>>>
> >>>> Signed-off-by: Fei Yang <fei.yang at intel.com>
> >>>> Reviewed-by: Andrzej Hajda <andrzej.hajda at intel.com>
> >>>> Reviewed-by: Andi Shyti <andi.shyti at linux.intel.com>
> >>>> Acked-by: Nirmoy Das <nirmoy.das at intel.com>
> >>>
> >>> Bspec: 45015, 45040
> >>>
> >>>> ---
> >>>>  drivers/gpu/drm/i915/display/intel_dpt.c |  2 +-
> >>>>  drivers/gpu/drm/i915/gt/gen8_ppgtt.c     | 45 ++++++++++++++++++++----
> >>>>  drivers/gpu/drm/i915/gt/intel_ggtt.c     | 36 +++++++++++++++++--
> >>>>  3 files changed, 72 insertions(+), 11 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/display/intel_dpt.c
> >>b/drivers/gpu/drm/i915/display/intel_dpt.c
> >>>> index b8027392144d..c5eacfdba1a5 100644
> >>>> --- a/drivers/gpu/drm/i915/display/intel_dpt.c
> >>>> +++ b/drivers/gpu/drm/i915/display/intel_dpt.c
> >>>> @@ -300,7 +300,7 @@ intel_dpt_create(struct intel_framebuffer *fb)
> >>>>        vm->vma_ops.bind_vma    = dpt_bind_vma;
> >>>>        vm->vma_ops.unbind_vma  = dpt_unbind_vma;
> >>>>
> >>>> -     vm->pte_encode = gen8_ggtt_pte_encode;
> >>>> +     vm->pte_encode = vm->gt->ggtt->vm.pte_encode;
> >>>>
> >>>>        dpt->obj = dpt_obj;
> >>>>        dpt->obj->is_dpt = true;
> >>>> diff --git a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> >>>>  b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> >>>> index 4daaa6f55668..11b91e0453c8 100644
> >>>> --- a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> >>>> +++ b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> >>>> @@ -55,6 +55,34 @@ static u64 gen8_pte_encode(dma_addr_t addr,
> >>>>        return pte;
> >>>>  }
> >>>>
> >>>> +static u64 mtl_pte_encode(dma_addr_t addr,
> >>>> +                       enum i915_cache_level level,
> >>>> +                       u32 flags)
> >>>> +{
> >>>> +     gen8_pte_t pte = addr | GEN8_PAGE_PRESENT | GEN8_PAGE_RW;
> >>>> +
> >>>> +     if (unlikely(flags & PTE_READ_ONLY))
> >>>> +             pte &= ~GEN8_PAGE_RW;
> >>>> +
> >>>> +     if (flags & PTE_LM)
> >>>> +             pte |= GEN12_PPGTT_PTE_LM | GEN12_PPGTT_PTE_NC;
> >>>
> >>> GEN12_PPGTT_PTE_NC got defined in the previous patch as BIT(5).  But
> >>> according to bspec 45040, bit 5 is ignored in the PTE encoding.  What is
> >>> this trying to do?
> >>
> >> This takes effect only for PTE_LM, doesn't affect MTL.
> >> PTE_NC is needed for PVC (use of access counter).
> >> I believe this function was writen based on the one for PVC. And this
> >> function did get extended to cover all gen12 in a later patch.
> >
> > Even though MTL doesn't have local memory, PTE_LM is supposed to be
> > used on MTL for access to BAR2 stolen memory.
> 
> You were right, but I still think this code is fine because this bit is
> ignored for MTL anyway and it is needed for other platforms with LMEM.
> Otherwise this code would have some sort of platform checking which is
> hard to do because we don't have platform info here.
> Or we would have to define another PTE encode function for platforms
> needing PTE_NC just for this one difference, then manage the function
> pointer correctly.

MTL is the only platform that uses this function right now:

   +       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;

If this is intended for PVC, then you have it in the wrong function to
begin with (and it also shouldn't be in a patch labelled "mtl").  If
you're trying to future-proof for some post-MTL discrete platform, then
such code should be saved until we enable that platform so that it can
be properly reviewed.


Matt

> 
> -Fei
> 
> > Matt
> >
> >> -Fei
> >>> Matt
> >>>
> >>>> +
> >>>> +     switch (level) {
> >>>> +     case I915_CACHE_NONE:
> >>>> +             pte |= GEN12_PPGTT_PTE_PAT1;
> >>>> +             break;

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation