I've figured out what's going wrong here.
Your analysis of the problem is correct, but the proposed fix just
doesn't handle everything.
The issue is that I assumed in the replace operation that the existing
lose end of the mapping can stay as they are, but that assumption is
actually not correct because of huge page handling.
This not only affects PRTs, but all mappings in general, so that's a
rather severe bug we need to fix.
Just send a only compile tested patch with subject "drm/amdgpu: fix
clear_all and replace handling in the VM" to the list, please give that
one a try and with your change.
Thanks,
Christian.
Am 04.06.2018 um 11:51 schrieb Christian König:
Am 04.06.2018 um 10:19 schrieb Zhang, Jerry (Junwei):
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.
I've just confirmed that huge pages and PRTs should work together. We
even have an unit test for that.
I mean we could ping Wade as well, but I'm pretty sure that he will
tell you just the same.
I think by disabling huge pages for PRTs we actually hide the real
issue somehow, probably some problem with updating PRTs after they are
split up.
Can you check if the problem also vanishes when you disable the
following optimization in amdgpu_vm_update_ptes?
/* We don't need to update PTEs for huge pages */
if (entry->huge)
continue;
Just comment this out for a test.
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.
Well disabling huge pages for PRTs can definitely cause a huge
performance problem because it increases the TLB misses for PRTs by a
factor of 512.
And since PRTs are usually used for large only sparse allocated
textures that is really really important here.
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.
That should already be the case when everything works as expected.
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
That sounds like something I should be able to reproduce when your
quick test doesn't bring the desired result.
Christian.
Jerry
Christian.
Jerry
Christian.
Am 04.06.2018 um 07:59 schrieb Zhou, David(ChunMing):
Good catch, Reviewed-by: Chunming Zhou <david1.z...@amd.com>
-----Original Message-----
From: amd-gfx [mailto:amd-gfx-boun...@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.zh...@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.zh...@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
