| Message ID | 20230222152641.1491307-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 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@intel.com> Hopefully CI will be happy now, Reviewed-by: Matthew Auld <matthew.auld@intel.com> > --- > 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);
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@intel.com> > > Hopefully CI will be happy now, > Reviewed-by: Matthew Auld <matthew.auld@intel.com> 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@hugepages.html 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? > >> --- >> 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);
-----Original Message----- From: Auld, Matthew <matthew.auld@intel.com> Sent: Friday, February 24, 2023 7:51 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 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@intel.com> > > > > Hopefully CI will be happy now, > > Reviewed-by: Matthew Auld <matthew.auld@intel.com> > > 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@hugepages.html > > 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); >
On 24/02/2023 16:13, Cavitt, Jonathan wrote: > -----Original Message----- > From: Auld, Matthew <matthew.auld@intel.com> > Sent: Friday, February 24, 2023 7:51 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 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@intel.com> >>> >>> Hopefully CI will be happy now, >>> Reviewed-by: Matthew Auld <matthew.auld@intel.com> >> >> 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@hugepages.html >> >> 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); >>
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);
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@intel.com> --- 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(-)