[PATCH 1/7] drm/amdgpu: fix VM PD addr shift

[PATCH 1/7] drm/amdgpu: fix VM PD addr shift

[Christian König]

The block size only affects the leave nodes, everything else is fixed.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 122379dfc7d8..f1e541e9b514 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -139,6 +139,24 @@ struct amdgpu_prt_cb {
 };
 
 /**
+ * amdgpu_vm_level_shift - return the addr shift for each level
+ *
+ * @adev: amdgpu_device pointer
+ *
+ * Returns the number of bits the pfn needs to be right shifted for a level.
+ */
+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) +
+			adev->vm_manager.block_size;
+	else
+		/* For the page tables on the leaves */
+		return 0;
+}
+
+/**
  * amdgpu_vm_num_entries - return the number of entries in a PD/PT
  *
  * @adev: amdgpu_device pointer
@@ -288,8 +306,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
 				  uint64_t saddr, uint64_t eaddr,
 				  unsigned level)
 {
-	unsigned shift = (adev->vm_manager.num_level - level) *
-		adev->vm_manager.block_size;
+	unsigned shift = amdgpu_vm_level_shift(adev, level);
 	unsigned pt_idx, from, to;
 	int r;
 	u64 flags;
@@ -1302,18 +1319,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 idx, level = p->adev->vm_manager.num_level;
+	unsigned level = 0;
 
 	*parent = NULL;
 	*entry = &p->vm->root;
 	while ((*entry)->entries) {
-		idx = addr >> (p->adev->vm_manager.block_size * 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)
+	if (level != p->adev->vm_manager.num_level)
 		*entry = NULL;
 }
 
-- 
2.11.0

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

[PATCH 2/7] drm/amdgpu: fix amdgpu_vm_num_entries

[Christian König]

That somehow got mixed up.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index f1e541e9b514..771995093cac 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -166,17 +166,17 @@ static unsigned amdgpu_vm_level_shift(struct amdgpu_device *adev,
 static unsigned amdgpu_vm_num_entries(struct amdgpu_device *adev,
 				      unsigned level)
 {
+	unsigned shift = amdgpu_vm_level_shift(adev, 0);
+
 	if (level == 0)
 		/* For the root directory */
-		return adev->vm_manager.max_pfn >>
-			(adev->vm_manager.block_size *
-			 adev->vm_manager.num_level);
-	else if (level == adev->vm_manager.num_level)
+		return round_up(adev->vm_manager.max_pfn, 1 << shift) >> shift;
+	else if (level != adev->vm_manager.num_level)
+		/* Everything in between */
+		return 512;
+	else
 		/* For the page tables on the leaves */
 		return AMDGPU_VM_PTE_COUNT(adev);
-	else
-		/* Everything in between */
-		return 1 << adev->vm_manager.block_size;
 }
 
 /**
-- 
2.11.0

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

[PATCH 3/7] drm/amdgpu: unify VM size handling of Vega10 with older generation

[Christian König]

One function to rule them all.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 34 +++++++++++++---------------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |  4 +---
 drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c  |  3 +--
 drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c  |  3 +--
 drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c  |  3 +--
 drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c  | 22 +++++-----------------
 6 files changed, 22 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 771995093cac..c942f6b4be1b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2574,43 +2574,35 @@ static uint32_t amdgpu_vm_get_block_size(uint64_t vm_size)
 }
 
 /**
- * amdgpu_vm_set_fragment_size - adjust fragment size in PTE
- *
- * @adev: amdgpu_device pointer
- * @fragment_size_default: the default fragment size if it's set auto
- */
-void amdgpu_vm_set_fragment_size(struct amdgpu_device *adev,
-				 uint32_t fragment_size_default)
-{
-	if (amdgpu_vm_fragment_size == -1)
-		adev->vm_manager.fragment_size = fragment_size_default;
-	else
-		adev->vm_manager.fragment_size = amdgpu_vm_fragment_size;
-}
-
-/**
  * amdgpu_vm_adjust_size - adjust vm size, block size and fragment size
  *
  * @adev: amdgpu_device pointer
  * @vm_size: the default vm size if it's set auto
  */
 void amdgpu_vm_adjust_size(struct amdgpu_device *adev, uint32_t vm_size,
-			   uint32_t fragment_size_default)
+			   uint32_t fragment_size_default, unsigned max_level)
 {
-	/* adjust vm size firstly */
-	if (amdgpu_vm_size != -1)
+	/* adjust vm size first, but only for two level setups for now */
+	if (amdgpu_vm_size != -1 && max_level == 1)
 		vm_size = amdgpu_vm_size;
 
 	adev->vm_manager.max_pfn = (uint64_t)vm_size << 18;
+	adev->vm_manager.num_level = max_level;
 
-	/* block size depends on vm size */
-	if (amdgpu_vm_block_size == -1)
+	/* block size depends on vm size and hw setup*/
+	if (adev->vm_manager.num_level > 1)
+		/* Use fixed block_size for multi level page tables */
+		adev->vm_manager.block_size = 9;
+	else if (amdgpu_vm_block_size == -1)
 		adev->vm_manager.block_size =
 			amdgpu_vm_get_block_size(vm_size);
 	else
 		adev->vm_manager.block_size = amdgpu_vm_block_size;
 
-	amdgpu_vm_set_fragment_size(adev, fragment_size_default);
+	if (amdgpu_vm_fragment_size == -1)
+		adev->vm_manager.fragment_size = fragment_size_default;
+	else
+		adev->vm_manager.fragment_size = amdgpu_vm_fragment_size;
 
 	DRM_INFO("vm size is %u GB, block size is %u-bit, fragment size is %u-bit\n",
 		 vm_size, adev->vm_manager.block_size,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index c80d45dd2bd3..54e540d5e8d7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -324,10 +324,8 @@ struct amdgpu_bo_va_mapping *amdgpu_vm_bo_lookup_mapping(struct amdgpu_vm *vm,
 							 uint64_t addr);
 void amdgpu_vm_bo_rmv(struct amdgpu_device *adev,
 		      struct amdgpu_bo_va *bo_va);
-void amdgpu_vm_set_fragment_size(struct amdgpu_device *adev,
-				 uint32_t fragment_size_default);
 void amdgpu_vm_adjust_size(struct amdgpu_device *adev, uint32_t vm_size,
-			   uint32_t fragment_size_default);
+			   uint32_t fragment_size_default, unsigned max_level);
 int amdgpu_vm_ioctl(struct drm_device *dev, void *data, struct drm_file *filp);
 bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring *ring,
 				  struct amdgpu_job *job);
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
index 6098c773711f..49224bf38324 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
@@ -832,7 +832,7 @@ static int gmc_v6_0_sw_init(void *handle)
 	if (r)
 		return r;
 
-	amdgpu_vm_adjust_size(adev, 64, 9);
+	amdgpu_vm_adjust_size(adev, 64, 9, 1);
 
 	adev->mc.mc_mask = 0xffffffffffULL;
 
@@ -877,7 +877,6 @@ static int gmc_v6_0_sw_init(void *handle)
 	 * amdkfd will use VMIDs 8-15
 	 */
 	adev->vm_manager.id_mgr[0].num_ids = AMDGPU_NUM_OF_VMIDS;
-	adev->vm_manager.num_level = 1;
 	amdgpu_vm_manager_init(adev);
 
 	/* base offset of vram pages */
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
index 8b460e9d4431..c39cf8440afd 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
@@ -971,7 +971,7 @@ static int gmc_v7_0_sw_init(void *handle)
 	 * Currently set to 4GB ((1 << 20) 4k pages).
 	 * Max GPUVM size for cayman and SI is 40 bits.
 	 */
-	amdgpu_vm_adjust_size(adev, 64, 9);
+	amdgpu_vm_adjust_size(adev, 64, 9, 1);
 
 	/* Set the internal MC address mask
 	 * This is the max address of the GPU's
@@ -1026,7 +1026,6 @@ static int gmc_v7_0_sw_init(void *handle)
 	 * amdkfd will use VMIDs 8-15
 	 */
 	adev->vm_manager.id_mgr[0].num_ids = AMDGPU_NUM_OF_VMIDS;
-	adev->vm_manager.num_level = 1;
 	amdgpu_vm_manager_init(adev);
 
 	/* base offset of vram pages */
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
index 1fd7f9daab0a..421e751a0464 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
@@ -1068,7 +1068,7 @@ static int gmc_v8_0_sw_init(void *handle)
 	 * Currently set to 4GB ((1 << 20) 4k pages).
 	 * Max GPUVM size for cayman and SI is 40 bits.
 	 */
-	amdgpu_vm_adjust_size(adev, 64, 9);
+	amdgpu_vm_adjust_size(adev, 64, 9, 1);
 
 	/* Set the internal MC address mask
 	 * This is the max address of the GPU's
@@ -1123,7 +1123,6 @@ static int gmc_v8_0_sw_init(void *handle)
 	 * amdkfd will use VMIDs 8-15
 	 */
 	adev->vm_manager.id_mgr[0].num_ids = AMDGPU_NUM_OF_VMIDS;
-	adev->vm_manager.num_level = 1;
 	amdgpu_vm_manager_init(adev);
 
 	/* base offset of vram pages */
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
index 30eb625a991c..729e4d591293 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
@@ -769,16 +769,11 @@ static int gmc_v9_0_sw_init(void *handle)
 	switch (adev->asic_type) {
 	case CHIP_RAVEN:
 		adev->mc.vram_type = AMDGPU_VRAM_TYPE_UNKNOWN;
-		if (adev->rev_id == 0x0 || adev->rev_id == 0x1) {
-			adev->vm_manager.max_pfn = 1ULL << 36;
-			adev->vm_manager.block_size = 9;
-			adev->vm_manager.num_level = 3;
-			amdgpu_vm_set_fragment_size(adev, 9);
-		} else {
+		if (adev->rev_id == 0x0 || adev->rev_id == 0x1)
+			amdgpu_vm_adjust_size(adev, 256 * 1024, 9, 3);
+		else
 			/* vm_size is 64GB for legacy 2-level page support */
-			amdgpu_vm_adjust_size(adev, 64, 9);
-			adev->vm_manager.num_level = 1;
-		}
+			amdgpu_vm_adjust_size(adev, 64, 9, 1);
 		break;
 	case CHIP_VEGA10:
 		/* XXX Don't know how to get VRAM type yet. */
@@ -788,19 +783,12 @@ static int gmc_v9_0_sw_init(void *handle)
 		 * vm size is 256TB (48bit), maximum size of Vega10,
 		 * block size 512 (9bit)
 		 */
-		adev->vm_manager.max_pfn = 1ULL << 36;
-		adev->vm_manager.block_size = 9;
-		adev->vm_manager.num_level = 3;
-		amdgpu_vm_set_fragment_size(adev, 9);
+		amdgpu_vm_adjust_size(adev, 256 * 1024, 9, 3);
 		break;
 	default:
 		break;
 	}
 
-	DRM_INFO("vm size is %llu GB, block size is %u-bit, fragment size is %u-bit\n",
-		 adev->vm_manager.max_pfn >> 18, adev->vm_manager.block_size,
-		 adev->vm_manager.fragment_size);
-
 	/* This interrupt is VMC page fault.*/
 	r = amdgpu_irq_add_id(adev, AMDGPU_IH_CLIENTID_VMC, 0,
 				&adev->mc.vm_fault);
-- 
2.11.0

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

[PATCH 4/7] drm/amdgpu: choose number of VM levels based on VM size

[Christian König]

This allows us limiting the VM size for testing even of Vega10.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index c942f6b4be1b..82a6f6c86aaf 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2582,12 +2582,17 @@ static uint32_t amdgpu_vm_get_block_size(uint64_t vm_size)
 void amdgpu_vm_adjust_size(struct amdgpu_device *adev, uint32_t vm_size,
 			   uint32_t fragment_size_default, unsigned max_level)
 {
-	/* adjust vm size first, but only for two level setups for now */
-	if (amdgpu_vm_size != -1 && max_level == 1)
+	uint64_t tmp;
+
+	/* adjust vm size first */
+	if (amdgpu_vm_size != -1)
 		vm_size = amdgpu_vm_size;
 
 	adev->vm_manager.max_pfn = (uint64_t)vm_size << 18;
-	adev->vm_manager.num_level = max_level;
+
+	tmp = roundup_pow_of_two(adev->vm_manager.max_pfn);
+	tmp = DIV_ROUND_UP(fls64(tmp) - 1, 9) - 1;
+	adev->vm_manager.num_level = min(max_level, (unsigned)tmp);
 
 	/* block size depends on vm size and hw setup*/
 	if (adev->vm_manager.num_level > 1)
@@ -2604,8 +2609,9 @@ void amdgpu_vm_adjust_size(struct amdgpu_device *adev, uint32_t vm_size,
 	else
 		adev->vm_manager.fragment_size = amdgpu_vm_fragment_size;
 
-	DRM_INFO("vm size is %u GB, block size is %u-bit, fragment size is %u-bit\n",
-		 vm_size, adev->vm_manager.block_size,
+	DRM_INFO("vm size is %u GB, %u levels, block size is %u-bit, fragment size is %u-bit\n",
+		 vm_size, adev->vm_manager.num_level + 1,
+		 adev->vm_manager.block_size,
 		 adev->vm_manager.fragment_size);
 }
 
-- 
2.11.0

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

[PATCH 5/7] drm/amdgpu: allow non pot VM size values

[Christian König]

The VM size actually doesn't need to be a power of two.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 285daa811223..ed0ada549f9f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1184,12 +1184,6 @@ static void amdgpu_check_vm_size(struct amdgpu_device *adev)
 	if (amdgpu_vm_size == -1)
 		return;
 
-	if (!is_power_of_2(amdgpu_vm_size)) {
-		dev_warn(adev->dev, "VM size (%d) must be a power of 2\n",
-			 amdgpu_vm_size);
-		goto def_value;
-	}
-
 	if (amdgpu_vm_size < 1) {
 		dev_warn(adev->dev, "VM size (%d) too small, min is 1GB\n",
 			 amdgpu_vm_size);
-- 
2.11.0

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

[PATCH 6/7] drm/amdgpu: move validation of the VM size into the VM code

[Christian König]

This moves validation of the VM size parameter into amdgpu_vm_adjust_size().

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 16 +---------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     | 13 +++++++++++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h     |  3 ++-
 drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c      |  2 +-
 drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c      |  2 +-
 drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c      |  2 +-
 drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c      |  6 +++---
 7 files changed, 20 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index ed0ada549f9f..0212919a6dfa 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1187,22 +1187,8 @@ static void amdgpu_check_vm_size(struct amdgpu_device *adev)
 	if (amdgpu_vm_size < 1) {
 		dev_warn(adev->dev, "VM size (%d) too small, min is 1GB\n",
 			 amdgpu_vm_size);
-		goto def_value;
+		amdgpu_vm_size = -1;
 	}
-
-	/*
-	 * Max GPUVM size for Cayman, SI, CI VI are 40 bits.
-	 */
-	if (amdgpu_vm_size > 1024) {
-		dev_warn(adev->dev, "VM size (%d) too large, max is 1TB\n",
-			 amdgpu_vm_size);
-		goto def_value;
-	}
-
-	return;
-
-def_value:
-	amdgpu_vm_size = -1;
 }
 
 /**
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 82a6f6c86aaf..44430c4820cc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2580,13 +2580,22 @@ static uint32_t amdgpu_vm_get_block_size(uint64_t vm_size)
  * @vm_size: the default vm size if it's set auto
  */
 void amdgpu_vm_adjust_size(struct amdgpu_device *adev, uint32_t vm_size,
-			   uint32_t fragment_size_default, unsigned max_level)
+			   uint32_t fragment_size_default, unsigned max_level,
+			   unsigned max_bits)
 {
 	uint64_t tmp;
 
 	/* adjust vm size first */
-	if (amdgpu_vm_size != -1)
+	if (amdgpu_vm_size != -1) {
+		unsigned max_size = 1 << (max_bits - 30);
+
 		vm_size = amdgpu_vm_size;
+		if (vm_size > max_size) {
+			dev_warn(adev->dev, "VM size (%d) too large, max is %u GB\n",
+				 amdgpu_vm_size, max_size);
+			vm_size = max_size;
+		}
+	}
 
 	adev->vm_manager.max_pfn = (uint64_t)vm_size << 18;
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 54e540d5e8d7..43ea131dd411 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -325,7 +325,8 @@ struct amdgpu_bo_va_mapping *amdgpu_vm_bo_lookup_mapping(struct amdgpu_vm *vm,
 void amdgpu_vm_bo_rmv(struct amdgpu_device *adev,
 		      struct amdgpu_bo_va *bo_va);
 void amdgpu_vm_adjust_size(struct amdgpu_device *adev, uint32_t vm_size,
-			   uint32_t fragment_size_default, unsigned max_level);
+			   uint32_t fragment_size_default, unsigned max_level,
+			   unsigned max_bits);
 int amdgpu_vm_ioctl(struct drm_device *dev, void *data, struct drm_file *filp);
 bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring *ring,
 				  struct amdgpu_job *job);
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
index 49224bf38324..468281f10e8d 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
@@ -832,7 +832,7 @@ static int gmc_v6_0_sw_init(void *handle)
 	if (r)
 		return r;
 
-	amdgpu_vm_adjust_size(adev, 64, 9, 1);
+	amdgpu_vm_adjust_size(adev, 64, 9, 1, 40);
 
 	adev->mc.mc_mask = 0xffffffffffULL;
 
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
index c39cf8440afd..68a85051f4b7 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
@@ -971,7 +971,7 @@ static int gmc_v7_0_sw_init(void *handle)
 	 * Currently set to 4GB ((1 << 20) 4k pages).
 	 * Max GPUVM size for cayman and SI is 40 bits.
 	 */
-	amdgpu_vm_adjust_size(adev, 64, 9, 1);
+	amdgpu_vm_adjust_size(adev, 64, 9, 1, 40);
 
 	/* Set the internal MC address mask
 	 * This is the max address of the GPU's
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
index 421e751a0464..46ec97e70e5c 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
@@ -1068,7 +1068,7 @@ static int gmc_v8_0_sw_init(void *handle)
 	 * Currently set to 4GB ((1 << 20) 4k pages).
 	 * Max GPUVM size for cayman and SI is 40 bits.
 	 */
-	amdgpu_vm_adjust_size(adev, 64, 9, 1);
+	amdgpu_vm_adjust_size(adev, 64, 9, 1, 40);
 
 	/* Set the internal MC address mask
 	 * This is the max address of the GPU's
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
index 729e4d591293..cc972153d401 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
@@ -770,10 +770,10 @@ static int gmc_v9_0_sw_init(void *handle)
 	case CHIP_RAVEN:
 		adev->mc.vram_type = AMDGPU_VRAM_TYPE_UNKNOWN;
 		if (adev->rev_id == 0x0 || adev->rev_id == 0x1)
-			amdgpu_vm_adjust_size(adev, 256 * 1024, 9, 3);
+			amdgpu_vm_adjust_size(adev, 256 * 1024, 9, 3, 48);
 		else
 			/* vm_size is 64GB for legacy 2-level page support */
-			amdgpu_vm_adjust_size(adev, 64, 9, 1);
+			amdgpu_vm_adjust_size(adev, 64, 9, 1, 48);
 		break;
 	case CHIP_VEGA10:
 		/* XXX Don't know how to get VRAM type yet. */
@@ -783,7 +783,7 @@ static int gmc_v9_0_sw_init(void *handle)
 		 * vm size is 256TB (48bit), maximum size of Vega10,
 		 * block size 512 (9bit)
 		 */
-		amdgpu_vm_adjust_size(adev, 256 * 1024, 9, 3);
+		amdgpu_vm_adjust_size(adev, 256 * 1024, 9, 3, 48);
 		break;
 	default:
 		break;
-- 
2.11.0

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

[PATCH 7/7] drm/amdgpu: allow specifying vm_block_size for multi level PDs

[Christian König]

This patch allows specifying the vm_block_size even when multi level
page directories are active.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 14 +-------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     | 15 +++++++++------
 2 files changed, 10 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 0212919a6dfa..abd7fe077bdc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1162,20 +1162,8 @@ static void amdgpu_check_block_size(struct amdgpu_device *adev)
 	if (amdgpu_vm_block_size < 9) {
 		dev_warn(adev->dev, "VM page table size (%d) too small\n",
 			 amdgpu_vm_block_size);
-		goto def_value;
+		amdgpu_vm_block_size = -1;
 	}
-
-	if (amdgpu_vm_block_size > 24 ||
-	    (amdgpu_vm_size * 1024) < (1ull << amdgpu_vm_block_size)) {
-		dev_warn(adev->dev, "VM page table size (%d) too large\n",
-			 amdgpu_vm_block_size);
-		goto def_value;
-	}
-
-	return;
-
-def_value:
-	amdgpu_vm_block_size = -1;
 }
 
 static void amdgpu_check_vm_size(struct amdgpu_device *adev)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 44430c4820cc..24e93370115a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2600,18 +2600,21 @@ void amdgpu_vm_adjust_size(struct amdgpu_device *adev, uint32_t vm_size,
 	adev->vm_manager.max_pfn = (uint64_t)vm_size << 18;
 
 	tmp = roundup_pow_of_two(adev->vm_manager.max_pfn);
+	if (amdgpu_vm_block_size != -1)
+		tmp >>= amdgpu_vm_block_size - 9;
 	tmp = DIV_ROUND_UP(fls64(tmp) - 1, 9) - 1;
 	adev->vm_manager.num_level = min(max_level, (unsigned)tmp);
 
 	/* block size depends on vm size and hw setup*/
-	if (adev->vm_manager.num_level > 1)
-		/* Use fixed block_size for multi level page tables */
-		adev->vm_manager.block_size = 9;
-	else if (amdgpu_vm_block_size == -1)
+	if (amdgpu_vm_block_size != -1)
 		adev->vm_manager.block_size =
-			amdgpu_vm_get_block_size(vm_size);
+			min(amdgpu_vm_block_size, max_bits
+			    - AMDGPU_GPU_PAGE_SHIFT
+			    - 9 * adev->vm_manager.num_level);
+	else if (adev->vm_manager.num_level > 1)
+		adev->vm_manager.block_size = 9;
 	else
-		adev->vm_manager.block_size = amdgpu_vm_block_size;
+		adev->vm_manager.block_size = amdgpu_vm_get_block_size(tmp);
 
 	if (amdgpu_vm_fragment_size == -1)
 		adev->vm_manager.fragment_size = fragment_size_default;
-- 
2.11.0

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

Re: [PATCH 1/7] drm/amdgpu: fix VM PD addr shift

[Felix Kuehling]

On 2017-11-27 11:02 AM, Christian König wrote:
> The block size only affects the leave nodes, everything else is fixed.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 28 +++++++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 122379dfc7d8..f1e541e9b514 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -139,6 +139,24 @@ struct amdgpu_prt_cb {
>  };
>  
>  /**
> + * amdgpu_vm_level_shift - return the addr shift for each level
> + *
> + * @adev: amdgpu_device pointer
> + *
> + * Returns the number of bits the pfn needs to be right shifted for a level.
> + */
> +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) +
> +			adev->vm_manager.block_size;
> +	else
> +		/* For the page tables on the leaves */
> +		return 0;
> +}
> +
> +/**
>   * amdgpu_vm_num_entries - return the number of entries in a PD/PT
>   *
>   * @adev: amdgpu_device pointer
> @@ -288,8 +306,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>  				  uint64_t saddr, uint64_t eaddr,
>  				  unsigned level)
>  {
> -	unsigned shift = (adev->vm_manager.num_level - level) *
> -		adev->vm_manager.block_size;
> +	unsigned shift = amdgpu_vm_level_shift(adev, level);
>  	unsigned pt_idx, from, to;
>  	int r;
>  	u64 flags;
> @@ -1302,18 +1319,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 idx, level = p->adev->vm_manager.num_level;
> +	unsigned level = 0;

This generates a checkpatch.pl warning.

General question: Do you have a policy for running your patches through
checkpatch.pl before/during/after code review? I've noticed before that
some of your patches aren't always 100% compliant.

Regards,
  Felix

>  
>  	*parent = NULL;
>  	*entry = &p->vm->root;
>  	while ((*entry)->entries) {
> -		idx = addr >> (p->adev->vm_manager.block_size * 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)
> +	if (level != p->adev->vm_manager.num_level)
>  		*entry = NULL;
>  }
>  

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

Re: [PATCH 1/7] drm/amdgpu: fix VM PD addr shift

[Christian König]

Am 27.11.2017 um 17:40 schrieb Felix Kuehling:
> On 2017-11-27 11:02 AM, Christian König wrote:
>> The block size only affects the leave nodes, everything else is fixed.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 28 +++++++++++++++++++++++-----
>>   1 file changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 122379dfc7d8..f1e541e9b514 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -139,6 +139,24 @@ struct amdgpu_prt_cb {
>>   };
>>   
>>   /**
>> + * amdgpu_vm_level_shift - return the addr shift for each level
>> + *
>> + * @adev: amdgpu_device pointer
>> + *
>> + * Returns the number of bits the pfn needs to be right shifted for a level.
>> + */
>> +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) +
>> +			adev->vm_manager.block_size;
>> +	else
>> +		/* For the page tables on the leaves */
>> +		return 0;
>> +}
>> +
>> +/**
>>    * amdgpu_vm_num_entries - return the number of entries in a PD/PT
>>    *
>>    * @adev: amdgpu_device pointer
>> @@ -288,8 +306,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>>   				  uint64_t saddr, uint64_t eaddr,
>>   				  unsigned level)
>>   {
>> -	unsigned shift = (adev->vm_manager.num_level - level) *
>> -		adev->vm_manager.block_size;
>> +	unsigned shift = amdgpu_vm_level_shift(adev, level);
>>   	unsigned pt_idx, from, to;
>>   	int r;
>>   	u64 flags;
>> @@ -1302,18 +1319,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 idx, level = p->adev->vm_manager.num_level;
>> +	unsigned level = 0;
> This generates a checkpatch.pl warning.
>
> General question: Do you have a policy for running your patches through
> checkpatch.pl before/during/after code review?

No, I'm using editor settings which generate coding style compliant code 
most of the time and complains/shows when it isn't compliant.

> I've noticed before that some of your patches aren't always 100% compliant.

Yeah, amdgpu inherited a lot of code as well as coding style from radeon 
which isn't compliant.

For example in this case checkpatch.pl most likely complains that we 
should use "unsigned int" instead of just "unsigned".

I've tried to clean this up for years, but simply not enough time to 
handle everything.

Regards,
Christian.

>
> Regards,
>    Felix
>
>>   
>>   	*parent = NULL;
>>   	*entry = &p->vm->root;
>>   	while ((*entry)->entries) {
>> -		idx = addr >> (p->adev->vm_manager.block_size * 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)
>> +	if (level != p->adev->vm_manager.num_level)
>>   		*entry = NULL;
>>   }
>>   

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

Re: [PATCH 1/7] drm/amdgpu: fix VM PD addr shift

[Michel Dänzer]

On 2017-11-27 06:12 PM, Christian König wrote:
> Am 27.11.2017 um 17:40 schrieb Felix Kuehling:
>> On 2017-11-27 11:02 AM, Christian König wrote:
>>> The block size only affects the leave nodes, everything else is fixed.
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 28
>>> +++++++++++++++++++++++-----
>>>   1 file changed, 23 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index 122379dfc7d8..f1e541e9b514 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -139,6 +139,24 @@ struct amdgpu_prt_cb {
>>>   };
>>>     /**
>>> + * amdgpu_vm_level_shift - return the addr shift for each level
>>> + *
>>> + * @adev: amdgpu_device pointer
>>> + *
>>> + * Returns the number of bits the pfn needs to be right shifted for
>>> a level.
>>> + */
>>> +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) +
>>> +            adev->vm_manager.block_size;
>>> +    else
>>> +        /* For the page tables on the leaves */
>>> +        return 0;
>>> +}
>>> +
>>> +/**
>>>    * amdgpu_vm_num_entries - return the number of entries in a PD/PT
>>>    *
>>>    * @adev: amdgpu_device pointer
>>> @@ -288,8 +306,7 @@ static int amdgpu_vm_alloc_levels(struct
>>> amdgpu_device *adev,
>>>                     uint64_t saddr, uint64_t eaddr,
>>>                     unsigned level)
>>>   {
>>> -    unsigned shift = (adev->vm_manager.num_level - level) *
>>> -        adev->vm_manager.block_size;
>>> +    unsigned shift = amdgpu_vm_level_shift(adev, level);
>>>       unsigned pt_idx, from, to;
>>>       int r;
>>>       u64 flags;
>>> @@ -1302,18 +1319,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 idx, level = p->adev->vm_manager.num_level;
>>> +    unsigned level = 0;
>> This generates a checkpatch.pl warning.
>>
>> General question: Do you have a policy for running your patches through
>> checkpatch.pl before/during/after code review?
> 
> No, I'm using editor settings which generate coding style compliant code
> most of the time and complains/shows when it isn't compliant.
> 
>> I've noticed before that some of your patches aren't always 100%
>> compliant.
> 
> Yeah, amdgpu inherited a lot of code as well as coding style from radeon
> which isn't compliant.
> 
> For example in this case checkpatch.pl most likely complains that we
> should use "unsigned int" instead of just "unsigned".
> 
> I've tried to clean this up for years, but simply not enough time to
> handle everything.

Moreover, checkpatch.pl cannot always be 100% satisfied, let alone in a
way that actually makes sense for the code in question. Its reports
should be considered guidelines, not the law.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/7] drm/amdgpu: fix VM PD addr shift

[Chunming Zhou]

On 2017年11月28日 00:02, Christian König wrote:
> The block size only affects the leave nodes, everything else is fixed.
This is not true, block size affects every level entries, see the 
register explains for VM_CONTEXTx_CNTL.PAGE_TABLE_BLOCK_SIZE:
"LOG2(number of 2MB logical address ranges) pointed to by a PDE0 page 
text directory entry. The native page size for the component ptes comes 
from the PDE0.block_fragment_size field or BASE_ADDR.block_fragment_size 
field (for a flat page table). The PAGE_TABLE_BLOCK_SIZE field only has 
an effect on address calculations for >= 2 level page tables however. A 
flat page table will utilize as many ptes as are required to represent 
the logical address between START_ADDR and END_ADDR, not limited by 
PAGE_TABLE_BLOCK_SIZE."

Regards,
David Zhou
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 28 +++++++++++++++++++++++-----
>   1 file changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 122379dfc7d8..f1e541e9b514 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -139,6 +139,24 @@ struct amdgpu_prt_cb {
>   };
>   
>   /**
> + * amdgpu_vm_level_shift - return the addr shift for each level
> + *
> + * @adev: amdgpu_device pointer
> + *
> + * Returns the number of bits the pfn needs to be right shifted for a level.
> + */
> +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) +
> +			adev->vm_manager.block_size;
> +	else
> +		/* For the page tables on the leaves */
> +		return 0;
> +}
> +
> +/**
>    * amdgpu_vm_num_entries - return the number of entries in a PD/PT
>    *
>    * @adev: amdgpu_device pointer
> @@ -288,8 +306,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>   				  uint64_t saddr, uint64_t eaddr,
>   				  unsigned level)
>   {
> -	unsigned shift = (adev->vm_manager.num_level - level) *
> -		adev->vm_manager.block_size;
> +	unsigned shift = amdgpu_vm_level_shift(adev, level);
>   	unsigned pt_idx, from, to;
>   	int r;
>   	u64 flags;
> @@ -1302,18 +1319,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 idx, level = p->adev->vm_manager.num_level;
> +	unsigned level = 0;
>   
>   	*parent = NULL;
>   	*entry = &p->vm->root;
>   	while ((*entry)->entries) {
> -		idx = addr >> (p->adev->vm_manager.block_size * 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)
> +	if (level != p->adev->vm_manager.num_level)
>   		*entry = NULL;
>   }
>   

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

Re: [PATCH 1/7] drm/amdgpu: fix VM PD addr shift

[Christian König]

Am 28.11.2017 um 03:42 schrieb Chunming Zhou:
>
>
> On 2017年11月28日 00:02, Christian König wrote:
>> The block size only affects the leave nodes, everything else is fixed.
> This is not true, block size affects every level entries, see the 
> register explains for VM_CONTEXTx_CNTL.PAGE_TABLE_BLOCK_SIZE:
> "LOG2(number of 2MB logical address ranges) pointed to by a PDE0 page 
> text directory entry. The native page size for the component ptes 
> comes from the PDE0.block_fragment_size field or 
> BASE_ADDR.block_fragment_size field (for a flat page table). The 
> PAGE_TABLE_BLOCK_SIZE field only has an effect on address calculations 
> for >= 2 level page tables however. A flat page table will utilize as 
> many ptes as are required to represent the logical address between 
> START_ADDR and END_ADDR, not limited by PAGE_TABLE_BLOCK_SIZE."

The description of the register is incorrect or at least misleading. 
I've fallen into the same trap as well.

Take a look at the VM documentation, there is a small footnote that 
intermediate page directories are always 512 entries in size.

I've confirmed that by configuring a Vega10 into 3 level page directory 
instead of the usual 4 and then using a block size of 18 for the page 
tables.

Regards,
Christian.

>
> Regards,
> David Zhou
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 28 
>> +++++++++++++++++++++++-----
>>   1 file changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 122379dfc7d8..f1e541e9b514 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -139,6 +139,24 @@ struct amdgpu_prt_cb {
>>   };
>>     /**
>> + * amdgpu_vm_level_shift - return the addr shift for each level
>> + *
>> + * @adev: amdgpu_device pointer
>> + *
>> + * Returns the number of bits the pfn needs to be right shifted for 
>> a level.
>> + */
>> +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) +
>> +            adev->vm_manager.block_size;
>> +    else
>> +        /* For the page tables on the leaves */
>> +        return 0;
>> +}
>> +
>> +/**
>>    * amdgpu_vm_num_entries - return the number of entries in a PD/PT
>>    *
>>    * @adev: amdgpu_device pointer
>> @@ -288,8 +306,7 @@ static int amdgpu_vm_alloc_levels(struct 
>> amdgpu_device *adev,
>>                     uint64_t saddr, uint64_t eaddr,
>>                     unsigned level)
>>   {
>> -    unsigned shift = (adev->vm_manager.num_level - level) *
>> -        adev->vm_manager.block_size;
>> +    unsigned shift = amdgpu_vm_level_shift(adev, level);
>>       unsigned pt_idx, from, to;
>>       int r;
>>       u64 flags;
>> @@ -1302,18 +1319,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 idx, level = p->adev->vm_manager.num_level;
>> +    unsigned level = 0;
>>         *parent = NULL;
>>       *entry = &p->vm->root;
>>       while ((*entry)->entries) {
>> -        idx = addr >> (p->adev->vm_manager.block_size * 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)
>> +    if (level != p->adev->vm_manager.num_level)
>>           *entry = NULL;
>>   }
>

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

Re: [PATCH 1/7] drm/amdgpu: fix VM PD addr shift

[Felix Kuehling]

Hi Christian,

I've finally found the time to review the patch series. It makes sense
to me.

I think the explanation on patch 1 ("The block size only affects the
leave nodes, everything else is fixed.") also applies to patch 2 and
would make it easier to understand the motivation for the change.

Other than that, the series is Reviewed-by: Felix Kuehling
<Felix.Kuehling@amd.com>

Regards,
  Felix


On 2017-11-27 11:02 AM, Christian König wrote:
> The block size only affects the leave nodes, everything else is fixed.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 28 +++++++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 122379dfc7d8..f1e541e9b514 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -139,6 +139,24 @@ struct amdgpu_prt_cb {
>  };
>  
>  /**
> + * amdgpu_vm_level_shift - return the addr shift for each level
> + *
> + * @adev: amdgpu_device pointer
> + *
> + * Returns the number of bits the pfn needs to be right shifted for a level.
> + */
> +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) +
> +			adev->vm_manager.block_size;
> +	else
> +		/* For the page tables on the leaves */
> +		return 0;
> +}
> +
> +/**
>   * amdgpu_vm_num_entries - return the number of entries in a PD/PT
>   *
>   * @adev: amdgpu_device pointer
> @@ -288,8 +306,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>  				  uint64_t saddr, uint64_t eaddr,
>  				  unsigned level)
>  {
> -	unsigned shift = (adev->vm_manager.num_level - level) *
> -		adev->vm_manager.block_size;
> +	unsigned shift = amdgpu_vm_level_shift(adev, level);
>  	unsigned pt_idx, from, to;
>  	int r;
>  	u64 flags;
> @@ -1302,18 +1319,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 idx, level = p->adev->vm_manager.num_level;
> +	unsigned level = 0;
>  
>  	*parent = NULL;
>  	*entry = &p->vm->root;
>  	while ((*entry)->entries) {
> -		idx = addr >> (p->adev->vm_manager.block_size * 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)
> +	if (level != p->adev->vm_manager.num_level)
>  		*entry = NULL;
>  }
>  

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