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

From: "Yang, Fei" <fei.yang@intel.com>
To: "Roper, Matthew D" <matthew.d.roper@intel.com>
Cc: "intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"Hajda, Andrzej" <andrzej.hajda@intel.com>,
	"Das, Nirmoy" <nirmoy.das@intel.com>
Subject: Re: [Intel-gfx] [PATCH 3/8] drm/i915/mtl: Add PTE encode function
Date: Mon, 24 Apr 2023 18:41:14 +0000	[thread overview]
Message-ID: <BYAPR11MB25678D775EF934B410E6B5E09A679@BYAPR11MB2567.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20230424172029.GA6250@mdroper-desk1.amr.corp.intel.com>

[-- Attachment #1: Type: text/plain, Size: 4540 bytes --]

> 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@intel.com wrote:
>>>>>> From: Fei Yang <fei.yang@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@intel.com>
>>>>>> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
>>>>>> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
>>>>>> Acked-by: Nirmoy Das <nirmoy.das@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.

dropped GEN12_PPGTT_PTE_NC bit in v2 of https://patchwork.freedesktop.org/series/116900/

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

[-- Attachment #2: Type: text/html, Size: 10619 bytes --]