[Intel-gfx] [PATCH] drm/i915: Use correct huge page manager for MTL

Fri Feb 24 16:51:34 UTC 2023

On 24/02/2023 16:13, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Auld, Matthew <matthew.auld at intel.com>
> Sent: Friday, February 24, 2023 7:51 AM
> To: Cavitt, Jonathan <jonathan.cavitt at intel.com>; intel-gfx at lists.freedesktop.org
> Cc: Dutt, Sudeep <sudeep.dutt at intel.com>; thomas.hellstrom at linux.intel.com; maarten.lankhorst at linux.intel.com; Vetter, Daniel <daniel.vetter at intel.com>; De Marchi, Lucas <lucas.demarchi at intel.com>; chris.p.wilson at 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 at intel.com>
>>>
>>> Hopefully CI will be happy now,
>>> Reviewed-by: Matthew Auld <matthew.auld at 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);
>>