<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
<style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style>
</head>
<body dir="ltr">
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);" class="elementToProof ContentPasted0 ContentPasted1 ContentPasted2">
> On Sun, Apr 23, 2023 at 12:37:27AM -0700, Yang, Fei wrote:
<div class="ContentPasted0">>>> On Fri, Apr 21, 2023 at 10:27:22AM -0700, Yang, Fei wrote:</div>
<div class="ContentPasted0">>>>>> On Wed, Apr 19, 2023 at 04:00:53PM -0700, fei.yang@intel.com wrote:</div>
<div class="ContentPasted0">>>>>>> From: Fei Yang <fei.yang@intel.com></div>
<div class="ContentPasted0">>>>>>></div>
<div class="ContentPasted0">>>>>>> PTE encode functions are platform dependent. This patch implements</div>
<div class="ContentPasted0">>>>>>> PTE functions for MTL, and ensures the correct PTE encode function</div>
<div class="ContentPasted0">>>>>>> is used by calling pte_encode function pointer instead of the</div>
<div class="ContentPasted0">>>>>>> hardcoded gen8 version of PTE encode.</div>
<div class="ContentPasted0">>>>>>></div>
<div class="ContentPasted0">>>>>>> Signed-off-by: Fei Yang <fei.yang@intel.com></div>
<div class="ContentPasted0">>>>>>> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com></div>
<div class="ContentPasted0">>>>>>> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com></div>
<div class="ContentPasted0">>>>>>> Acked-by: Nirmoy Das <nirmoy.das@intel.com></div>
<div class="ContentPasted0">>>>>></div>
<div class="ContentPasted0">>>>>> Bspec: 45015, 45040</div>
<div class="ContentPasted0">>>>>></div>
<div class="ContentPasted0">>>>>>> ---</div>
<div class="ContentPasted0">>>>>>> drivers/gpu/drm/i915/display/intel_dpt.c | 2 +-</div>
<div class="ContentPasted0">>>>>>> drivers/gpu/drm/i915/gt/gen8_ppgtt.c | 45 ++++++++++++++++++++----</div>
<div class="ContentPasted0">>>>>>> drivers/gpu/drm/i915/gt/intel_ggtt.c | 36 +++++++++++++++++--</div>
<div class="ContentPasted0">>>>>>> 3 files changed, 72 insertions(+), 11 deletions(-)</div>
<div class="ContentPasted0">>>>>>></div>
<div class="ContentPasted0">>>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dpt.c</div>
<div class="ContentPasted0">>>>>b/drivers/gpu/drm/i915/display/intel_dpt.c</div>
<div class="ContentPasted0">>>>>>> index b8027392144d..c5eacfdba1a5 100644</div>
<div class="ContentPasted0">>>>>>> --- a/drivers/gpu/drm/i915/display/intel_dpt.c</div>
<div class="ContentPasted0">>>>>>> +++ b/drivers/gpu/drm/i915/display/intel_dpt.c</div>
<div class="ContentPasted0">>>>>>> @@ -300,7 +300,7 @@ intel_dpt_create(struct intel_framebuffer *fb)</div>
<div class="ContentPasted0">>>>>>> vm->vma_ops.bind_vma = dpt_bind_vma;</div>
<div class="ContentPasted0">>>>>>> vm->vma_ops.unbind_vma = dpt_unbind_vma;</div>
<div class="ContentPasted0">>>>>>></div>
<div class="ContentPasted0">>>>>>> - vm->pte_encode = gen8_ggtt_pte_encode;</div>
<div class="ContentPasted0">>>>>>> + vm->pte_encode = vm->gt->ggtt->vm.pte_encode;</div>
<div class="ContentPasted0">>>>>>></div>
<div class="ContentPasted0">>>>>>> dpt->obj = dpt_obj;</div>
<div class="ContentPasted0">>>>>>> dpt->obj->is_dpt = true;</div>
<div class="ContentPasted0">>>>>>> diff --git a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c</div>
<div class="ContentPasted0">>>>>>> b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c</div>
<div class="ContentPasted0">>>>>>> index 4daaa6f55668..11b91e0453c8 100644</div>
<div class="ContentPasted0">>>>>>> --- a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c</div>
>>>>>> +++ b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
<div class="ContentPasted1">>>>>>> @@ -55,6 +55,34 @@ static u64 gen8_pte_encode(dma_addr_t addr,</div>
<div class="ContentPasted1">>>>>>> return pte;</div>
<div class="ContentPasted1">>>>>>> }</div>
<div class="ContentPasted1">>>>>>></div>
<div class="ContentPasted1">>>>>>> +static u64 mtl_pte_encode(dma_addr_t addr,</div>
<div class="ContentPasted1">>>>>>> + enum i915_cache_level level,</div>
<div class="ContentPasted1">>>>>>> + u32 flags)</div>
<div class="ContentPasted1">>>>>>> +{</div>
<div class="ContentPasted1">>>>>>> + gen8_pte_t pte = addr | GEN8_PAGE_PRESENT | GEN8_PAGE_RW;</div>
<div class="ContentPasted1">>>>>>> +</div>
<div class="ContentPasted1">>>>>>> + if (unlikely(flags & PTE_READ_ONLY))</div>
<div class="ContentPasted1">>>>>>> + pte &= ~GEN8_PAGE_RW;</div>
<div class="ContentPasted1">>>>>>> +</div>
<div class="ContentPasted1">>>>>>> + if (flags & PTE_LM)</div>
<div class="ContentPasted1">>>>>>> + pte |= GEN12_PPGTT_PTE_LM | GEN12_PPGTT_PTE_NC;</div>
<div class="ContentPasted1">>>>>></div>
<div class="ContentPasted1">>>>>> GEN12_PPGTT_PTE_NC got defined in the previous patch as BIT(5). But</div>
<div class="ContentPasted1">>>>>> according to bspec 45040, bit 5 is ignored in the PTE encoding. What is</div>
<div class="ContentPasted1">>>>>> this trying to do?</div>
<div class="ContentPasted1">>>>></div>
<div class="ContentPasted1">>>>> This takes effect only for PTE_LM, doesn't affect MTL.</div>
<div class="ContentPasted1">>>>> PTE_NC is needed for PVC (use of access counter).</div>
<div class="ContentPasted1">>>>> I believe this function was writen based on the one for PVC. And this</div>
<div class="ContentPasted1">>>>> function did get extended to cover all gen12 in a later patch.</div>
<div class="ContentPasted1">>>></div>
<div class="ContentPasted1">>>> Even though MTL doesn't have local memory, PTE_LM is supposed to be</div>
<div class="ContentPasted1">>>> used on MTL for access to BAR2 stolen memory.</div>
<div class="ContentPasted1">>></div>
<div class="ContentPasted1">>> You were right, but I still think this code is fine because this bit is</div>
<div class="ContentPasted1">>> ignored for MTL anyway and it is needed for other platforms with LMEM.</div>
<div class="ContentPasted1">>> Otherwise this code would have some sort of platform checking which is</div>
<div class="ContentPasted1">>> hard to do because we don't have platform info here.</div>
<div class="ContentPasted1">>> Or we would have to define another PTE encode function for platforms</div>
<div class="ContentPasted1">>> needing PTE_NC just for this one difference, then manage the function</div>
<div class="ContentPasted1">>> pointer correctly.</div>
<div class="ContentPasted1">></div>
<div class="ContentPasted1">> MTL is the only platform that uses this function right now:</div>
<div class="ContentPasted1">></div>
<div class="ContentPasted1">> + if (GRAPHICS_VER_FULL(gt->i915) >= IP_VER(12, 70))</div>
<div class="ContentPasted1">> + ppgtt->vm.pte_encode = mtl_pte_encode;</div>
> + else
<div class="ContentPasted2">> + ppgtt->vm.pte_encode = gen8_pte_encode;</div>
<div class="ContentPasted2">></div>
<div class="ContentPasted2">> If this is intended for PVC, then you have it in the wrong function to</div>
<div class="ContentPasted2">> begin with (and it also shouldn't be in a patch labelled "mtl"). If</div>
<div class="ContentPasted2">> you're trying to future-proof for some post-MTL discrete platform, then</div>
<div class="ContentPasted2">> such code should be saved until we enable that platform so that it can</div>
<div class="ContentPasted2">> be properly reviewed.</div>
<div><br class="ContentPasted2">
</div>
<div class="ContentPasted2">dropped GEN12_PPGTT_PTE_NC bit in v2 of https://patchwork.freedesktop.org/series/116900/</div>
<div><br class="ContentPasted2">
</div>
<div class="ContentPasted2">> Matt</div>
<div class="ContentPasted2">></div>
<div class="ContentPasted2">>></div>
<div class="ContentPasted2">>> -Fei</div>
<div class="ContentPasted2">>></div>
<div class="ContentPasted2">>>> Matt</div>
<div class="ContentPasted2">>>></div>
<div class="ContentPasted2">>>>> -Fei</div>
<div class="ContentPasted2">>>>>> Matt</div>
<div class="ContentPasted2">>>>>></div>
<div class="ContentPasted2">>>>>>> +</div>
<div class="ContentPasted2">>>>>>> + switch (level) {</div>
<div class="ContentPasted2">>>>>>> + case I915_CACHE_NONE:</div>
<div class="ContentPasted2">>>>>>> + pte |= GEN12_PPGTT_PTE_PAT1;</div>
<div class="ContentPasted2">>>>>>> + break;</div>
</div>
</body>
</html>