[PATCH 1/2] drm/amdgpu: reverse PDBs order

[Chunming Zhou]

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 <david1.zhou at amd.com>
>
> 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;
>>   }
>