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