-----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. 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); >
