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 <david1.zhou at amd.com>
Yeah, thought about that as well. But I'm not sure if that is a good
idea or not.
> ---
> 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.
> 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".
> + /* != 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;
> }
>
