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

[Chunming Zhou]

On 2017年12月08日 22:58, Alex Deucher wrote:
On Fri, Dec 8, 2017 at 5:56 AM, Chunming Zhou <david1.z...@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.

Regards,
David Zhou

Alex

Change-Id: I2d748e5e96cffe18294c104c4b192d910b2f8e6b
Signed-off-by: Chunming Zhou <david1.z...@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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx