On 24/02/2023 16:13, Cavitt, Jonathan wrote:
-----Original Message-----
From: Auld, Matthew <matthew.auld@xxxxxxxxx>
Sent: Friday, February 24, 2023 7:51 AM
To: Cavitt, Jonathan <jonathan.cavitt@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
Cc: Dutt, Sudeep <sudeep.dutt@xxxxxxxxx>; thomas.hellstrom@xxxxxxxxxxxxxxx; maarten.lankhorst@xxxxxxxxxxxxxxx; Vetter, Daniel <daniel.vetter@xxxxxxxxx>; De Marchi, Lucas <lucas.demarchi@xxxxxxxxx>; chris.p.wilson@xxxxxxxxxxxxxxx
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 <jonathan.cavitt@xxxxxxxxx>
Hopefully CI will be happy now,
Reviewed-by: Matthew Auld <matthew.auld@xxxxxxxxx>
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@mock@xxxxxxxxxxxxxx
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.
Not sure I follow. With "split this into two patches", I just meant one
patch to convert the troublesome mock tests into live tests
(igt_mock_ppgtt_64K + igt_mock_ppgtt_huge_fill). Those shouldn't be mock
tests any more, since the mock device can't really cover two different
HW models (this is already an existing issue since supporting ATS/DG2),
and likely means those tests are just too HW specific, and so live test
would be much more suitable (perhaps with slightly different test
expectations depending on the graphics ver). This can be considered a
prep patch (could even be merged without the MTL stuff), and should be
standalone. But how much (in any) of igt_mock_ppgtt_64K() we care about
for ver >= (12, 50) I'm not so sure...
In another patch (as part of the same patch series) we then add the MTL
stuff.
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);
