[PATCH 02/10] drm/amdgpu: reverse PDBs order v2

[Christian König]

Am 11.12.2017 um 09:16 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
>
> v2:
> previous the root pdb is level0, not only me, also many people
> thought the root PDB is PDB0, but our many hw documents assume
> PDB0 is pointing to PTB, that is said, which bring us unnecessary trouble on
> understanding our VM hiberachy implementation, especially for ramp up
> beginner.
> For last Christian BLOCK_SIZE patch example, the spec said it only effects
> the PTB pointed by PDB0, but we initially thought it effects all levels,
> which mainly because of confuse 'PDB0'.

I'm still not very keen about this.

How about we additional to this add an enum for the PDB2..PTB values?

Regards,
Christian.

>
> Change-Id: I2d748e5e96cffe18294c104c4b192d910b2f8e6b
> Signed-off-by: Chunming Zhou <david1.zhou at amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 36 ++++++++++++++++++++++------------
>   1 file changed, 23 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index bb191fca60f3..bdb7bdfee0fb 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;
>   	saddr = saddr & ((1 << shift) - 1);
>   	eaddr = eaddr & ((1 << shift) - 1);
>   
> @@ -346,7 +355,7 @@ 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)
> +		if (level != 0)
>   			init_value |= AMDGPU_PDE_PTE;
>   
>   	}
> @@ -389,7 +398,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 +444,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 +1329,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;
>   }
>