Am 11.12.2017 um 04:55 schrieb Alex Deucher:
> On Sun, Dec 10, 2017 at 10:01 PM, Chunming Zhou <zhoucm1 at amd.com> wrote:
>>
>> On 2017å¹´12æ??08æ?¥ 22:58, Alex Deucher wrote:
>>> On Fri, Dec 8, 2017 at 5:56 AM, Chunming Zhou <david1.zhou at amd.com> wrote:
>>>> 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
>>> What's the advantage of this change? It's not clear from the commit
>>> message.
>> Hi Alex,
>>
>> previous the level is increasing, 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'.
>>
>> Above is my starting point.
> Thanks for clarifying. Please include that information in the commit
> message for clarity.
We should document this, but I would keep the code as it is cause
mapping level1 to PDB0 doesn't make it any clearer if you ask me.
And additional to that as you said as well most people assume that zero
is the root page table.
The only people who doesn't do that are our hardware developers (and I
honestly doesn't understand why, cause this causes confusion all over
the place even internally to AMD).
Another problem are the page directory start value which is written into
the registers on Vega10. Those reassemble a page directory entry and I
referenced them as level -1 in the latest patches.
Ok we could change that to 0xff as well, but I find that even more
confusing.
Christian.
>
> Alex
>
>> Regards,
>> David Zhou
>>
>>> Alex
>>>
>>>> Change-Id: I2d748e5e96cffe18294c104c4b192d910b2f8e6b
>>>> Signed-off-by: Chunming Zhou <david1.zhou at amd.com>
>>>> ---
>>>> 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--;
>>>> 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)
>>>> + /* != PDB0 */
>>>> + if (level != 1)
>>>> 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;
>>>> }
>>>>
>>>> --
>>>> 2.14.1
>>>>
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
