-----Original Message----- From: Auld, Matthew <[email protected]> Sent: Monday, February 27, 2023 3:20 AM To: Cavitt, Jonathan <[email protected]>; [email protected] Cc: Dutt, Sudeep <[email protected]>; [email protected]; [email protected]; Vetter, Daniel <[email protected]>; De Marchi, Lucas <[email protected]>; [email protected] Subject: Re: [PATCH] drm/i915: Use correct huge page manager for MTL > > On 24/02/2023 17:40, Jonathan Cavitt wrote: > > MTL currently uses gen8_ppgtt_insert_huge when managing huge pages. This > > is because > > MTL reports as not supporting 64K pages, or more accurately, the system > > that reports > > whether a platform has 64K pages reports false for MTL. This is only half > > correct, > > as the 64K page support reporting system only cares about 64K page support > > for LMEM, > > which MTL doesn't have. > > > > MTL should be using xehpsdv_ppgtt_insert_huge. However, simply changing > > over to > > using that manager doesn't resolve the issue because MTL is expecting the > > virtual > > address space for the page table to be flushed after initialization, so we > > must also > > add a flush statement there. > > > > The changes made to the huge page manager selection indirectly impacted > > some of the > > mock hugepage selftests. Due to the use of pte level hints, rather than > > pde level > > hints, we now expect 64K page sizes to be properly reported by the GTT, so > > we should > > correct the expected test results to reflect this change. > > For future submissions, please add the version number for each new > submission of the same patch, and also please include the changelog.
I thought we weren't supposed to include that information? Or is that just a "from internal to upstream" thing? -Jonathan Cavitt > > > > > Signed-off-by: Jonathan Cavitt <[email protected]> > > --- > > drivers/gpu/drm/i915/gem/selftests/huge_pages.c | 11 ++++------- > > drivers/gpu/drm/i915/gt/gen8_ppgtt.c | 3 ++- > > 2 files changed, 6 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c > > b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c > > index defece0bcb81..06554717495f 100644 > > --- a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c > > +++ b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c > > @@ -784,9 +784,6 @@ static int igt_mock_ppgtt_huge_fill(void *arg) > > GEM_BUG_ON(!expected_gtt); > > GEM_BUG_ON(size); > > > > - if (expected_gtt & I915_GTT_PAGE_SIZE_4K) > > - expected_gtt &= ~I915_GTT_PAGE_SIZE_64K; > > - > > i915_vma_unpin(vma); > > > > if (vma->page_sizes.sg & I915_GTT_PAGE_SIZE_64K) { > > @@ -849,7 +846,7 @@ static int igt_mock_ppgtt_64K(void *arg) > > }, > > { > > .size = SZ_64K + SZ_4K, > > - .gtt = I915_GTT_PAGE_SIZE_4K, > > + .gtt = I915_GTT_PAGE_SIZE_64K | I915_GTT_PAGE_SIZE_4K, > > .offset = 0, > > }, > > { > > @@ -864,7 +861,7 @@ static int igt_mock_ppgtt_64K(void *arg) > > }, > > { > > .size = SZ_2M - SZ_4K, > > - .gtt = I915_GTT_PAGE_SIZE_4K, > > + .gtt = I915_GTT_PAGE_SIZE_64K | I915_GTT_PAGE_SIZE_4K, > > .offset = 0, > > }, > > { > > @@ -886,12 +883,12 @@ static int igt_mock_ppgtt_64K(void *arg) > > { > > .size = SZ_64K, > > .offset = SZ_2M, > > - .gtt = I915_GTT_PAGE_SIZE_4K, > > + .gtt = I915_GTT_PAGE_SIZE_64K, > > }, > > { > > .size = SZ_128K, > > .offset = SZ_2M - SZ_64K, > > - .gtt = I915_GTT_PAGE_SIZE_4K, > > + .gtt = I915_GTT_PAGE_SIZE_64K, > > }, > > Did you consider the suggestion with possibly making this a live test > instead? > > The first comment in igt_mock_ppgtt_64K() describing the test is: > > /* > * Sanity check some of the trickiness with 64K pages -- either we can > * safely mark the whole page-table(2M block) as 64K, or we have to > * always fallback to 4K. > */ > > That doesn't really apply to the new 64K GTT model it seems (which is > why it now fails), so trying to adjust the test just because the mock > device underneath is now using the newer model doesn't seem correct to > me. If we instead make it a live test and only run it on devices with > the old 64K GTT model, then we still retain the same test coverage. > Alternatively, we could potentially run on both HW models with slightly > different test expectations. IMO the test is too HW specific for a mock > test. > > > }; > > struct i915_vma *vma; > > diff --git a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c > > b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c > > index 4daaa6f55668..9c571185395f 100644 > > --- a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c > > +++ b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c > > @@ -570,6 +570,7 @@ xehpsdv_ppgtt_insert_huge(struct i915_address_space *vm, > > } > > } while (rem >= page_size && index < max); > > > > + drm_clflush_virt_range(vaddr, PAGE_SIZE); > > vma_res->page_sizes_gtt |= page_size; > > } while (iter->sg && sg_dma_len(iter->sg)); > > } > > @@ -707,7 +708,7 @@ static void gen8_ppgtt_insert(struct i915_address_space > > *vm, > > struct sgt_dma iter = sgt_dma(vma_res); > > > > if (vma_res->bi.page_sizes.sg > I915_GTT_PAGE_SIZE) { > > - if (HAS_64K_PAGES(vm->i915)) > > + if (GRAPHICS_VER_FULL(vm->i915) >= IP_VER(12, 50)) > > xehpsdv_ppgtt_insert_huge(vm, vma_res, &iter, > > cache_level, flags); > > else > > gen8_ppgtt_insert_huge(vm, vma_res, &iter, cache_level, > > flags); >
