<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 04/05/2023 00:02, fei.yang@intel.com wrote:
<div class="ContentPasted0">>> From: Fei Yang <fei.yang@intel.com></div>
<div class="ContentPasted0">>></div>
<div class="ContentPasted0">>> Currently the KMD is using enum i915_cache_level to set caching policy for</div>
<div class="ContentPasted0">>> buffer objects. This is flaky because the PAT index which really controls</div>
<div class="ContentPasted0">>> the caching behavior in PTE has far more levels than what's defined in the</div>
<div class="ContentPasted0">>> enum. In addition, the PAT index is platform dependent, having to translate</div>
<div class="ContentPasted0">>> between i915_cache_level and PAT index is not reliable, and makes the code</div>
<div class="ContentPasted0">>> more complicated.</div>
<div class="ContentPasted0">>></div>
<div class="ContentPasted0">>>>From UMD's perspective there is also a necessity to set caching policy for</div>
<div class="ContentPasted0">>> performance fine tuning. It's much easier for the UMD to directly use PAT</div>
<div class="ContentPasted0">>> index because the behavior of each PAT index is clearly defined in Bspec.</div>
<div class="ContentPasted0">>> Having the abstracted i915_cache_level sitting in between would only cause</div>
<div class="ContentPasted0">>> more ambiguity.</div>
<div class="ContentPasted0">>></div>
<div class="ContentPasted0">>> For these reasons this patch replaces i915_cache_level with PAT index. Also</div>
<div class="ContentPasted0">>> note, the cache_level is not completely removed yet, because the KMD still</div>
<div class="ContentPasted0">>> has the need of creating buffer objects with simple cache settings such as</div>
<div class="ContentPasted0">>> cached, uncached, or writethrough. For such simple cases, using cache_level</div>
<div class="ContentPasted0">>> would help simplify the code.</div>
<div class="ContentPasted0">>></div>
<div class="ContentPasted0">>> Cc: Chris Wilson <chris.p.wilson@linux.intel.com></div>
<div class="ContentPasted0">>> Cc: Matt Roper <matthew.d.roper@intel.com></div>
<div class="ContentPasted0">>> Signed-off-by: Fei Yang <fei.yang@intel.com></div>
<div class="ContentPasted0">>> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com></div>
<div class="ContentPasted0">></div>
<div class="ContentPasted0">> [snip]</div>
<div class="ContentPasted0">></div>
<div class="ContentPasted0">>> diff --git a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c</div>
<div class="ContentPasted0">>> index bb6998d67133..f2334a713c4e 100644</div>
<div class="ContentPasted0">>> --- 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">>> @@ -56,7 +56,7 @@ static u64 gen8_pte_encode(dma_addr_t addr,</div>
<div class="ContentPasted0">>> }</div>
<div class="ContentPasted0">></div>
<div class="ContentPasted0">>^^^</div>
<div class="ContentPasted0">></div>
<div class="ContentPasted0">> How come there are no changes to gen8_pte_encode?</div>
<div><br class="ContentPasted0">
</div>
<div class="ContentPasted0">For legacy platforms cache_level is equal to pat_index, so I was thinking</div>
more about reducing number of lines changed.
<div><br class="ContentPasted1">
</div>
<div class="ContentPasted1">>vvv</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">>> + unsigned int pat_index,</div>
<div class="ContentPasted1">>> u32 flags)</div>
<div class="ContentPasted1">></div>
<div class="ContentPasted1">> Prototype and implementation changed here for mtl_pte_encode.</div>
<div class="ContentPasted1">></div>
<div class="ContentPasted1">> And we have:</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>
<div class="ContentPasted1">> else</div>
<div class="ContentPasted1">> ppgtt->vm.pte_encode = gen8_pte_encode;</div>
<div class="ContentPasted1">></div>
<div class="ContentPasted1">> So should be same prototype. And:</div>
<div class="ContentPasted1">></div>
<div class="ContentPasted1">> u64 (*pte_encode)(dma_addr_t addr,</div>
<div class="ContentPasted1">>- enum i915_cache_level level,</div>
<div class="ContentPasted1">>+ unsigned int pat_index,</div>
<div class="ContentPasted1">> u32 flags); /* Create a valid PTE */</div>
<div class="ContentPasted1">></div>
<div class="ContentPasted1">> Patch relies on the compiler considering enum equal to unsigned int?</div>
<div><br class="ContentPasted1">
</div>
<div class="ContentPasted1">yes, caller is passing in unsigned int and gets used as enum.</div>
<div><br class="ContentPasted1">
</div>
<div class="ContentPasted1">> But the implementation of gen8_pte_encode and most ggtt counterparts is</div>
<div class="ContentPasted1">> looking at the passed in pat index and thinks it is cache level.</div>
<div class="ContentPasted1">></div>
<div class="ContentPasted1">> How is that supposed to work?! Or I am blind and am missing something?</div>
<div><br class="ContentPasted1">
</div>
<div class="ContentPasted1">For legacy platforms translation through LEGACY_CACHELEVEL would not</div>
<div class="ContentPasted1">change the value of cache_level. The cache_level and pat_index are basically</div>
<div class="ContentPasted1">the same for these platforms.</div>
<div><br class="ContentPasted1">
</div>
<div class="ContentPasted1">It is broken for gen12 here. I was asked to separate the gen12_pte_encode</div>
<div class="ContentPasted1">change to another patch in the series, but that breaks bisect. Should I</div>
squash 2/5 and 3/5?
<div><br class="ContentPasted2">
</div>
<div class="ContentPasted2">> Regards,</div>
<div class="ContentPasted2">></div>
<div class="ContentPasted2">> Tvrtko</div>
<br>
</div>
</body>
</html>