On 2017年12月08日 22:06, Christian König wrote:
Am 08.12.2017 um 11:56 schrieb Chunming Zhou:
The hiberachy of page table is as below, which aligns hw names.
PDB2->PDB1->PDB0->PTB, accordingly:
level3 --- PDB2
level2 --- PDB1
level1 --- PDB0
level0 --- PTB
Change-Id: I2d748e5e96cffe18294c104c4b192d910b2f8e6b
Signed-off-by: Chunming Zhou <[email protected]>
Yeah, thought about that as well. But I'm not sure if that is a good
idea or not.
We should correct the confusing PDB0 concept, even spec directly uses it.
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 37
++++++++++++++++++++++------------
1 file changed, 24 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 3ecdbdfb04dd..8904ccf78fc9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -148,8 +148,8 @@ struct amdgpu_prt_cb {
static unsigned amdgpu_vm_level_shift(struct amdgpu_device *adev,
unsigned level)
{
- if (level != adev->vm_manager.num_level)
- return 9 * (adev->vm_manager.num_level - level - 1) +
+ if (level != 0)
+ return 9 * (level - 1) +
adev->vm_manager.block_size;
else
/* For the page tables on the leaves */
@@ -162,20 +162,27 @@ static unsigned amdgpu_vm_level_shift(struct
amdgpu_device *adev,
* @adev: amdgpu_device pointer
*
* Calculate the number of entries in a page directory or page table.
+ * The hiberachy of page table is as below, which aligns hw names.
+ * PDB2->PDB1->PDB0->PTB, accordingly:
+ * level3 --- PDB2
+ * level2 --- PDB1
+ * level1 --- PDB0
+ * level0 --- PTB
*/
static unsigned amdgpu_vm_num_entries(struct amdgpu_device *adev,
unsigned level)
{
- unsigned shift = amdgpu_vm_level_shift(adev, 0);
+ unsigned shift = amdgpu_vm_level_shift(adev,
+ adev->vm_manager.num_level);
- if (level == 0)
+ if (level == adev->vm_manager.num_level)
/* For the root directory */
return round_up(adev->vm_manager.max_pfn, 1 << shift) >>
shift;
- else if (level != adev->vm_manager.num_level)
+ else if (level != 0)
/* Everything in between */
return 512;
else
- /* For the page tables on the leaves */
+ /* For the page tables on the leaves(PTB) */
return AMDGPU_VM_PTE_COUNT(adev);
}
@@ -312,6 +319,8 @@ static int amdgpu_vm_alloc_levels(struct
amdgpu_device *adev,
u64 flags;
uint64_t init_value = 0;
+ BUG_ON(level > adev->vm_manager.num_level);
+
if (!parent->entries) {
unsigned num_entries = amdgpu_vm_num_entries(adev, level);
@@ -332,7 +341,7 @@ static int amdgpu_vm_alloc_levels(struct
amdgpu_device *adev,
if (to > parent->last_entry_used)
parent->last_entry_used = to;
- ++level;
+ level--;
Post decrement/increment please.
it's OK, but what the difference of `level--;` and `--level;` is? which
isn't in expression, but in alone line.
saddr = saddr & ((1 << shift) - 1);
eaddr = eaddr & ((1 << shift) - 1);
@@ -346,7 +355,8 @@ static int amdgpu_vm_alloc_levels(struct
amdgpu_device *adev,
if (vm->pte_support_ats) {
init_value = AMDGPU_PTE_DEFAULT_ATC;
- if (level != adev->vm_manager.num_level - 1)
BTW: If I'm not completely mistaken that code is incorrect and should
rather be "level != adev->vm_manager.num_level".
will separate a fix patch.
Regards,
David Zhou
+ /* != PDB0 */
+ if (level != 1)
Which would that make it level != 0.
Regards,
Christian.
init_value |= AMDGPU_PDE_PTE;
}
@@ -389,7 +399,7 @@ static int amdgpu_vm_alloc_levels(struct
amdgpu_device *adev,
entry->addr = 0;
}
- if (level < adev->vm_manager.num_level) {
+ if (level > 0) {
uint64_t sub_saddr = (pt_idx == from) ? saddr : 0;
uint64_t sub_eaddr = (pt_idx == to) ? eaddr :
((1 << shift) - 1);
@@ -435,7 +445,8 @@ int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
saddr /= AMDGPU_GPU_PAGE_SIZE;
eaddr /= AMDGPU_GPU_PAGE_SIZE;
- return amdgpu_vm_alloc_levels(adev, vm, &vm->root, saddr,
eaddr, 0);
+ return amdgpu_vm_alloc_levels(adev, vm, &vm->root, saddr, eaddr,
+ adev->vm_manager.num_level);
}
/**
@@ -1319,19 +1330,19 @@ void amdgpu_vm_get_entry(struct
amdgpu_pte_update_params *p, uint64_t addr,
struct amdgpu_vm_pt **entry,
struct amdgpu_vm_pt **parent)
{
- unsigned level = 0;
+ unsigned level = p->adev->vm_manager.num_level;
*parent = NULL;
*entry = &p->vm->root;
while ((*entry)->entries) {
- unsigned idx = addr >> amdgpu_vm_level_shift(p->adev, level++);
+ unsigned idx = addr >> amdgpu_vm_level_shift(p->adev, level--);
idx %= amdgpu_bo_size((*entry)->base.bo) / 8;
*parent = *entry;
*entry = &(*entry)->entries[idx];
}
- if (level != p->adev->vm_manager.num_level)
+ if (level != 0)
*entry = NULL;
}
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
