| Message ID | 20230224174016.2081135-1-jonathan.cavitt@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Use correct huge page manager for MTL | expand |
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. > > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@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);
-----Original Message----- From: Auld, Matthew <matthew.auld@intel.com> Sent: Monday, February 27, 2023 3:20 AM To: Cavitt, Jonathan <jonathan.cavitt@intel.com>; intel-gfx@lists.freedesktop.org Cc: Dutt, Sudeep <sudeep.dutt@intel.com>; thomas.hellstrom@linux.intel.com; maarten.lankhorst@linux.intel.com; Vetter, Daniel <daniel.vetter@intel.com>; De Marchi, Lucas <lucas.demarchi@intel.com>; chris.p.wilson@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@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); >
On 27/02/2023 15:06, Cavitt, Jonathan wrote: > -----Original Message----- > From: Auld, Matthew <matthew.auld@intel.com> > Sent: Monday, February 27, 2023 3:20 AM > To: Cavitt, Jonathan <jonathan.cavitt@intel.com>; intel-gfx@lists.freedesktop.org > Cc: Dutt, Sudeep <sudeep.dutt@intel.com>; thomas.hellstrom@linux.intel.com; maarten.lankhorst@linux.intel.com; Vetter, Daniel <daniel.vetter@intel.com>; De Marchi, Lucas <lucas.demarchi@intel.com>; chris.p.wilson@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? Yeah, we don't include the original changelog (if there is one) when just going from "internal to upstream", since that changelog is kind of meaningless from upstream pov. Normally that just gets scrubbed, along with any a-b/r-b etc. However, this patch has now gone through a few revisions on @intel-gfx, and each one those should have a version and changelog, as per the usual patch review process. Also please see my comments below, in case you missed them. > -Jonathan Cavitt > >> >>> >>> Signed-off-by: Jonathan Cavitt <jonathan.cavitt@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); >>
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, }, }; 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);
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. Signed-off-by: Jonathan Cavitt <jonathan.cavitt@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(-)