[Intel-gfx] [PATCH] drm/i915: Use correct huge page manager for MTL

[Cavitt, Jonathan]

-----Original Message-----
From: Auld, Matthew <matthew.auld at intel.com> 
Sent: Monday, February 27, 2023 3:20 AM
To: Cavitt, Jonathan <jonathan.cavitt at intel.com>; intel-gfx at lists.freedesktop.org
Cc: Dutt, Sudeep <sudeep.dutt at intel.com>; thomas.hellstrom at linux.intel.com; maarten.lankhorst at linux.intel.com; Vetter, Daniel <daniel.vetter at intel.com>; De Marchi, Lucas <lucas.demarchi at intel.com>; chris.p.wilson at linux.intel.com
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 <jonathan.cavitt at intel.com>
> > ---
> >   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);
>