Re: [PATCH] drm/amdgpu: skip huge page for PRT mapping

["Zhang, Jerry (Junwei)" <Jerry.Zhang-5C7GfCeVMHo@public.gmane.org>]

On 06/04/2018 03:48 PM, Christian König wrote:
> Am 04.06.2018 um 09:02 schrieb Zhang, Jerry (Junwei):
>> On 06/04/2018 02:43 PM, Christian König wrote:
>>> Actually that is not correct. According to the documentation the PRT flag should
>>> work for huge pages as well.
>>
>> Mmm, I checked the doc earlier, didn't find the PRT flag for PDE.
>
> The PDE indeed doesn't have the PRT flag, but it also doesn't have the fragment
> and MTYPE fields and those still work normally when you enable huge page handling.
>
> The trick is that when you set the huge page flag then the PDE is handled like a
> PTE and so all the extra fields (PRT, fragment size, MTYPE etc...) should now be
> handled correctly.
>
> Could be that there is a hardware bug related to PRT handling and the huge page
> flag, but at least in theory it should work fine.

The doc just skips these fields in PDE, if those fields really works expectedly, 
even if it doesn't describe the details(but we could refer to PTE fields), we 
may have to confirm that with HW guys.

In my view in current stage, PDE doesn't support PRT and it may not make much 
sense to support PRT either.
The huge page always happens when reserving PRT range, but later UMD is likely 
to bind a/some tiled bo(s) inside this range, that will break the huge page 
mapping and split into several pieces mappings, representing in PTE instead of 
huge page.
In this case, just skipping the huge page for PRT may be an acceptable way.

>
>>
>> In CTS PRT test, the reserved PRT mapping introduces huge page mapping, so
>> later tiled bo mapping cannot make sure the corresponding PTE is set as PRT.
>> Then following access triggers VM fault.
>
> Interesting, but that rather sounds like a bug in the handling instead of a
> hardware problem.

On the 2nd thinking, we may handle that for huge page.
e.g.when binding tiled bo in PRT range, we could split the huge page into pieces 
PTE mappings.
However, it may just make the code more complex and get the same results as 
current fix.

>
> Can you narrow down the CTS test further into a libdrm unit test? Or in other
> words what exactly does the CTS test do?

The issue could be reproduced by below command:
{{{
deqp-vk -n dEQP-VK.sparse_resources.buffer.ssbo.sparse_residency.buffer_size_2_24
}}}

In KMD view, the main process(with amdgpu mainline driver) is like below:
1) reserve PRT range [0x300400000, 0x300400000 + 0x1000000)
2) bind tiled bos each other page
    0x300400000 ~ 0x300401000,
    0x300402000 ~ 0x300403000,
    0x300404000 ~ 0x300405000,
    ...
3) access them all, VM fault at 0x300401000

Jerry

>
> Christian.
>
>>
>> Jerry
>>
>>>
>>> Christian.
>>>
>>> Am 04.06.2018 um 07:59 schrieb Zhou, David(ChunMing):
>>>> Good catch, Reviewed-by: Chunming  Zhou <david1.zhou@amd.com>
>>>>
>>>> -----Original Message-----
>>>> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf Of
>>>> Junwei Zhang
>>>> Sent: Monday, June 04, 2018 10:04 AM
>>>> To: amd-gfx@lists.freedesktop.org
>>>> Cc: Zhang, Jerry <Jerry.Zhang@amd.com>
>>>> Subject: [PATCH] drm/amdgpu: skip huge page for PRT mapping
>>>>
>>>> PRT mapping doesn't support huge page, since it's per PTE basis.
>>>>
>>>> Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 3 ++-
>>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> index 850cd66..4ce8bb0 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> @@ -1111,7 +1111,8 @@ static void amdgpu_vm_handle_huge_pages(struct
>>>> amdgpu_pte_update_params *p,
>>>>       /* In the case of a mixed PT the PDE must point to it*/
>>>>       if (p->adev->asic_type >= CHIP_VEGA10 && !p->src &&
>>>> -        nptes == AMDGPU_VM_PTE_COUNT(p->adev)) {
>>>> +        nptes == AMDGPU_VM_PTE_COUNT(p->adev) &&
>>>> +        !(flags & AMDGPU_PTE_PRT)) {
>>>>           /* Set the huge page flag to stop scanning at this PDE */
>>>>           flags |= AMDGPU_PDE_PTE;
>>>>       }
>>>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx