-----Original Message-----
From: Auld, Matthew <[email protected]>
Sent: Friday, February 24, 2023 7:51 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 22/02/2023 15:56, Matthew Auld wrote:
> > On 22/02/2023 15:26, 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
> >> remove the check that asserts otherwise.
> >>
> >> Signed-off-by: Jonathan Cavitt <[email protected]>
> >
> > Hopefully CI will be happy now,
> > Reviewed-by: Matthew Auld <[email protected]>
>
> Looks it passes now, but then fails on the next subtest:
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114259v1/shard-apl7/igt@i915_selftest@[email protected]
>
> Maybe it's not too outrageous to just move igt_mock_ppgtt_64K() to the
> live test section and skip running it with graphics ver >= (12, 50).
> IIUC that subtest is mostly only interesting with the old model anyway.
> I guess that test is much too HW specific to be considered good mock
> test. And maybe we should do the same for igt_mock_ppgtt_huge_fill().
> But there we can just make the expected_gtt &= ~I915_GTT_PAGE_SIZE_64K
> conditional on graphics ver < (12, 50). Also maybe split this into two
> patches. Thoughts?
IMO, no matter how important it is to get this patch upstreamed, introducing
regressions to "fix later"
with this patch is just going to erode the trust of the upstream Linux team
more than it's already been.
So, frustratingly, I think we need to do what's necessary to get this resolved
in the same patch.
Thankfully, this probably won't be too difficult: igt_mock_ppgtt_64K is the
last mock test, and we just
need to mark every object .gtt with I915_GTT_PAGE_SIZE_64K when .size & SZ_64K
is not zero. We
can do this in one of two ways:
1. Mark it in the object (I.E. adding it directly to .gtt). This makes the
most sense, but is the most difficult.
2. Mark it separately (I.E. expected_gtt |= object[i].size & SZ_64K ?
I915_GTT_PAGE_SIZE_64K : 0).
This is the easiest, but is very likely to be rejected.
We probably have to do fix #1, and a direct fix here probably won't repair the
issue 100% (There might be
some other objects in the test that aren't correctly formatted). However, if
we take this one step at a time,
we'll eventually get this patch upstream-ready.
-Jonathan Cavitt
>
> >
> >> ---
> >> drivers/gpu/drm/i915/gem/selftests/huge_pages.c | 3 ---
> >> drivers/gpu/drm/i915/gt/gen8_ppgtt.c | 3 ++-
> >> 2 files changed, 2 insertions(+), 4 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..1659ada4ce33 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) {
> >> 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);
>
