[PATCH 1/8] drm/amdgpu: add some VM PD/PT iterators v2

[PATCH 1/8] drm/amdgpu: add some VM PD/PT iterators v2

[Christian König]

Both a leaf as well as dfs iterator to walk over all the PDs/PTs.

v2: update comments and fix for_each_amdgpu_vm_pt_dfs_safe

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 136b00412dc8..787a200cf796 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -355,6 +355,230 @@ static struct amdgpu_vm_pt *amdgpu_vm_pt_parent(struct amdgpu_vm_pt *pt)
 	return list_first_entry(&parent->va, struct amdgpu_vm_pt, base.bo_list);
 }
 
+/**
+ * amdgpu_vm_pt_cursor - state for for_each_amdgpu_vm_pt
+ */
+struct amdgpu_vm_pt_cursor {
+	uint64_t pfn;
+	struct amdgpu_vm_pt *parent;
+	struct amdgpu_vm_pt *entry;
+	unsigned level;
+};
+
+/**
+ * amdgpu_vm_pt_start - start PD/PT walk
+ *
+ * @adev: amdgpu_device pointer
+ * @vm: amdgpu_vm structure
+ * @start: start address of the walk
+ * @cursor: state to initialize
+ *
+ * Initialize a amdgpu_vm_pt_cursor to start a walk.
+ */
+static void amdgpu_vm_pt_start(struct amdgpu_device *adev,
+			       struct amdgpu_vm *vm, uint64_t start,
+			       struct amdgpu_vm_pt_cursor *cursor)
+{
+	cursor->pfn = start;
+	cursor->parent = NULL;
+	cursor->entry = &vm->root;
+	cursor->level = adev->vm_manager.root_level;
+}
+
+/**
+ * amdgpu_vm_pt_descendant - go to child node
+ *
+ * @adev: amdgpu_device pointer
+ * @cursor: current state
+ *
+ * Walk to the child node of the current node.
+ * Returns:
+ * True if the walk was possible, false otherwise.
+ */
+static bool amdgpu_vm_pt_descendant(struct amdgpu_device *adev,
+				    struct amdgpu_vm_pt_cursor *cursor)
+{
+	unsigned num_entries, shift, idx;
+
+	if (!cursor->entry->entries)
+		return false;
+
+	BUG_ON(!cursor->entry->base.bo);
+	num_entries = amdgpu_vm_num_entries(adev, cursor->level);
+	shift = amdgpu_vm_level_shift(adev, cursor->level);
+
+	++cursor->level;
+	idx = (cursor->pfn >> shift) % num_entries;
+	cursor->parent = cursor->entry;
+	cursor->entry = &cursor->entry->entries[idx];
+	return true;
+}
+
+/**
+ * amdgpu_vm_pt_sibling - go to sibling node
+ *
+ * @adev: amdgpu_device pointer
+ * @cursor: current state
+ *
+ * Walk to the sibling node of the current node.
+ * Returns:
+ * True if the walk was possible, false otherwise.
+ */
+static bool amdgpu_vm_pt_sibling(struct amdgpu_device *adev,
+				 struct amdgpu_vm_pt_cursor *cursor)
+{
+	unsigned shift, num_entries;
+
+	/* Root doesn't have a sibling */
+	if (!cursor->parent)
+		return false;
+
+	/* Go to our parents and see if we got a sibling */
+	shift = amdgpu_vm_level_shift(adev, cursor->level - 1);
+	num_entries = amdgpu_vm_num_entries(adev, cursor->level - 1);
+
+	if (cursor->entry == &cursor->parent->entries[num_entries - 1])
+		return false;
+
+	cursor->pfn += 1ULL << shift;
+	cursor->pfn &= ~((1ULL << shift) - 1);
+	++cursor->entry;
+	return true;
+}
+
+/**
+ * amdgpu_vm_pt_ancestor - go to parent node
+ *
+ * @cursor: current state
+ *
+ * Walk to the parent node of the current node.
+ * Returns:
+ * True if the walk was possible, false otherwise.
+ */
+static bool amdgpu_vm_pt_ancestor(struct amdgpu_vm_pt_cursor *cursor)
+{
+	if (!cursor->parent)
+		return false;
+
+	--cursor->level;
+	cursor->entry = cursor->parent;
+	cursor->parent = amdgpu_vm_pt_parent(cursor->parent);
+	return true;
+}
+
+/**
+ * amdgpu_vm_pt_next - get next PD/PT in hieratchy
+ *
+ * @adev: amdgpu_device pointer
+ * @cursor: current state
+ *
+ * Walk the PD/PT tree to the next node.
+ */
+static void amdgpu_vm_pt_next(struct amdgpu_device *adev,
+			      struct amdgpu_vm_pt_cursor *cursor)
+{
+	/* First try a newborn child */
+	if (amdgpu_vm_pt_descendant(adev, cursor))
+		return;
+
+	/* If that didn't worked try to find a sibling */
+	while (!amdgpu_vm_pt_sibling(adev, cursor)) {
+		/* No sibling, go to our parents and grandparents */
+		if (!amdgpu_vm_pt_ancestor(cursor)) {
+			cursor->pfn = ~0ll;
+			return;
+		}
+	}
+}
+
+/**
+ * amdgpu_vm_pt_first_leaf - get first leaf PD/PT
+ *
+ * @adev: amdgpu_device pointer
+ * @vm: amdgpu_vm structure
+ * @start: start addr of the walk
+ * @cursor: state to initialize
+ *
+ * Start a walk and go directly to the leaf node.
+ */
+static void amdgpu_vm_pt_first_leaf(struct amdgpu_device *adev,
+				    struct amdgpu_vm *vm, uint64_t start,
+				    struct amdgpu_vm_pt_cursor *cursor)
+{
+	amdgpu_vm_pt_start(adev, vm, start, cursor);
+	while (amdgpu_vm_pt_descendant(adev, cursor));
+}
+
+/**
+ * amdgpu_vm_pt_next_leaf - get next leaf PD/PT
+ *
+ * @adev: amdgpu_device pointer
+ * @cursor: current state
+ *
+ * Walk the PD/PT tree to the next leaf node.
+ */
+static void amdgpu_vm_pt_next_leaf(struct amdgpu_device *adev,
+				   struct amdgpu_vm_pt_cursor *cursor)
+{
+	amdgpu_vm_pt_next(adev, cursor);
+	while (amdgpu_vm_pt_descendant(adev, cursor));
+}
+
+/**
+ * for_each_amdgpu_vm_pt_leaf - walk over all leaf PDs/PTs in the hierarchy
+ */
+#define for_each_amdgpu_vm_pt_leaf(adev, vm, start, end, cursor)		\
+	for (amdgpu_vm_pt_first_leaf((adev), (vm), (start), &(cursor));		\
+	     (cursor).pfn <= end; amdgpu_vm_pt_next_leaf((adev), &(cursor)))
+
+/**
+ * amdgpu_vm_pt_first_dfs - start a deep first search
+ *
+ * @adev: amdgpu_device structure
+ * @vm: amdgpu_vm structure
+ * @cursor: state to initialize
+ *
+ * Starts a deep first traversal of the PD/PT tree.
+ */
+static void amdgpu_vm_pt_first_dfs(struct amdgpu_device *adev,
+				   struct amdgpu_vm *vm,
+				   struct amdgpu_vm_pt_cursor *cursor)
+{
+	amdgpu_vm_pt_start(adev, vm, 0, cursor);
+	while (amdgpu_vm_pt_descendant(adev, cursor));
+}
+
+/**
+ * amdgpu_vm_pt_next_dfs - get the next node for a deep first search
+ *
+ * @adev: amdgpu_device structure
+ * @cursor: current state
+ *
+ * Move the cursor to the next node in a deep first search.
+ */
+static void amdgpu_vm_pt_next_dfs(struct amdgpu_device *adev,
+				  struct amdgpu_vm_pt_cursor *cursor)
+{
+	if (!cursor->entry)
+		return;
+
+	if (!cursor->parent)
+		cursor->entry = NULL;
+	else if (amdgpu_vm_pt_sibling(adev, cursor))
+		while (amdgpu_vm_pt_descendant(adev, cursor));
+	else
+		amdgpu_vm_pt_ancestor(cursor);
+}
+
+/**
+ * for_each_amdgpu_vm_pt_dfs_safe - safe deep first search of all PDs/PTs
+ */
+#define for_each_amdgpu_vm_pt_dfs_safe(adev, vm, cursor, entry)			\
+	for (amdgpu_vm_pt_first_dfs((adev), (vm), &(cursor)),			\
+	     (entry) = (cursor).entry, amdgpu_vm_pt_next_dfs((adev), &(cursor));\
+	     (entry); (entry) = (cursor).entry,					\
+	     amdgpu_vm_pt_next_dfs((adev), &(cursor)))
+
 /**
  * amdgpu_vm_get_pd_bo - add the VM PD to a validation list
  *
-- 
2.14.1

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

[PATCH 2/8] drm/amdgpu: use leaf iterator for allocating PD/PT

[Christian König]

Less code and allows for easier error handling.

Signed-off-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
Reviewed-by: Junwei Zhang <Jerry.Zhang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 156 ++++++++++++---------------------
 1 file changed, 55 insertions(+), 101 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 787a200cf796..2cc34d1b87e0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -844,103 +844,6 @@ static void amdgpu_vm_bo_param(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 		bp->resv = vm->root.base.bo->tbo.resv;
 }
 
-/**
- * amdgpu_vm_alloc_levels - allocate the PD/PT levels
- *
- * @adev: amdgpu_device pointer
- * @vm: requested vm
- * @parent: parent PT
- * @saddr: start of the address range
- * @eaddr: end of the address range
- * @level: VMPT level
- * @ats: indicate ATS support from PTE
- *
- * Make sure the page directories and page tables are allocated
- *
- * Returns:
- * 0 on success, errno otherwise.
- */
-static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
-				  struct amdgpu_vm *vm,
-				  struct amdgpu_vm_pt *parent,
-				  uint64_t saddr, uint64_t eaddr,
-				  unsigned level, bool ats)
-{
-	unsigned shift = amdgpu_vm_level_shift(adev, level);
-	struct amdgpu_bo_param bp;
-	unsigned pt_idx, from, to;
-	int r;
-
-	if (!parent->entries) {
-		unsigned num_entries = amdgpu_vm_num_entries(adev, level);
-
-		parent->entries = kvmalloc_array(num_entries,
-						   sizeof(struct amdgpu_vm_pt),
-						   GFP_KERNEL | __GFP_ZERO);
-		if (!parent->entries)
-			return -ENOMEM;
-	}
-
-	from = saddr >> shift;
-	to = eaddr >> shift;
-	if (from >= amdgpu_vm_num_entries(adev, level) ||
-	    to >= amdgpu_vm_num_entries(adev, level))
-		return -EINVAL;
-
-	++level;
-	saddr = saddr & ((1 << shift) - 1);
-	eaddr = eaddr & ((1 << shift) - 1);
-
-	amdgpu_vm_bo_param(adev, vm, level, &bp);
-
-	/* walk over the address space and allocate the page tables */
-	for (pt_idx = from; pt_idx <= to; ++pt_idx) {
-		struct amdgpu_vm_pt *entry = &parent->entries[pt_idx];
-		struct amdgpu_bo *pt;
-
-		if (!entry->base.bo) {
-			r = amdgpu_bo_create(adev, &bp, &pt);
-			if (r)
-				return r;
-
-			r = amdgpu_vm_clear_bo(adev, vm, pt, level, ats);
-			if (r) {
-				amdgpu_bo_unref(&pt->shadow);
-				amdgpu_bo_unref(&pt);
-				return r;
-			}
-
-			if (vm->use_cpu_for_update) {
-				r = amdgpu_bo_kmap(pt, NULL);
-				if (r) {
-					amdgpu_bo_unref(&pt->shadow);
-					amdgpu_bo_unref(&pt);
-					return r;
-				}
-			}
-
-			/* Keep a reference to the root directory to avoid
-			* freeing them up in the wrong order.
-			*/
-			pt->parent = amdgpu_bo_ref(parent->base.bo);
-
-			amdgpu_vm_bo_base_init(&entry->base, vm, pt);
-		}
-
-		if (level < AMDGPU_VM_PTB) {
-			uint64_t sub_saddr = (pt_idx == from) ? saddr : 0;
-			uint64_t sub_eaddr = (pt_idx == to) ? eaddr :
-				((1 << shift) - 1);
-			r = amdgpu_vm_alloc_levels(adev, vm, entry, sub_saddr,
-						   sub_eaddr, level, ats);
-			if (r)
-				return r;
-		}
-	}
-
-	return 0;
-}
-
 /**
  * amdgpu_vm_alloc_pts - Allocate page tables.
  *
@@ -949,7 +852,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
  * @saddr: Start address which needs to be allocated
  * @size: Size from start address we need.
  *
- * Make sure the page tables are allocated.
+ * Make sure the page directories and page tables are allocated
  *
  * Returns:
  * 0 on success, errno otherwise.
@@ -958,8 +861,11 @@ int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
 			struct amdgpu_vm *vm,
 			uint64_t saddr, uint64_t size)
 {
-	uint64_t eaddr;
+	struct amdgpu_vm_pt_cursor cursor;
+	struct amdgpu_bo *pt;
 	bool ats = false;
+	uint64_t eaddr;
+	int r;
 
 	/* validate the parameters */
 	if (saddr & AMDGPU_GPU_PAGE_MASK || size & AMDGPU_GPU_PAGE_MASK)
@@ -979,8 +885,56 @@ int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
 		return -EINVAL;
 	}
 
-	return amdgpu_vm_alloc_levels(adev, vm, &vm->root, saddr, eaddr,
-				      adev->vm_manager.root_level, ats);
+	for_each_amdgpu_vm_pt_leaf(adev, vm, saddr, eaddr, cursor) {
+		struct amdgpu_vm_pt *entry = cursor.entry;
+		struct amdgpu_bo_param bp;
+
+		if (cursor.level < AMDGPU_VM_PTB) {
+			unsigned num_entries;
+
+			num_entries = amdgpu_vm_num_entries(adev, cursor.level);
+			entry->entries = kvmalloc_array(num_entries,
+							sizeof(*entry->entries),
+							GFP_KERNEL |
+							__GFP_ZERO);
+			if (!entry->entries)
+				return -ENOMEM;
+		}
+
+
+		if (entry->base.bo)
+			continue;
+
+		amdgpu_vm_bo_param(adev, vm, cursor.level, &bp);
+
+		r = amdgpu_bo_create(adev, &bp, &pt);
+		if (r)
+			return r;
+
+		r = amdgpu_vm_clear_bo(adev, vm, pt, cursor.level, ats);
+		if (r)
+			goto error_free_pt;
+
+		if (vm->use_cpu_for_update) {
+			r = amdgpu_bo_kmap(pt, NULL);
+			if (r)
+				goto error_free_pt;
+		}
+
+		/* Keep a reference to the root directory to avoid
+		* freeing them up in the wrong order.
+		*/
+		pt->parent = amdgpu_bo_ref(cursor.parent->base.bo);
+
+		amdgpu_vm_bo_base_init(&entry->base, vm, pt);
+	}
+
+	return 0;
+
+error_free_pt:
+	amdgpu_bo_unref(&pt->shadow);
+	amdgpu_bo_unref(&pt);
+	return r;
 }
 
 /**
-- 
2.14.1

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

[PATCH 3/8] drm/amdgpu: use dfs iterator to free PDs/PTs

[Christian König]

Allows us to free all PDs/PTs without recursion.

Signed-off-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
Reviewed-by: Junwei Zhang <Jerry.Zhang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 62 ++++++++++++++++------------------
 1 file changed, 30 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 2cc34d1b87e0..a0a30416a490 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -937,6 +937,35 @@ int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
 	return r;
 }
 
+/**
+ * amdgpu_vm_free_pts - free PD/PT levels
+ *
+ * @adev: amdgpu device structure
+ * @parent: PD/PT starting level to free
+ * @level: level of parent structure
+ *
+ * Free the page directory or page table level and all sub levels.
+ */
+static void amdgpu_vm_free_pts(struct amdgpu_device *adev,
+			       struct amdgpu_vm *vm)
+{
+	struct amdgpu_vm_pt_cursor cursor;
+	struct amdgpu_vm_pt *entry;
+
+	for_each_amdgpu_vm_pt_dfs_safe(adev, vm, cursor, entry) {
+
+		if (entry->base.bo) {
+			list_del(&entry->base.bo_list);
+			list_del(&entry->base.vm_status);
+			amdgpu_bo_unref(&entry->base.bo->shadow);
+			amdgpu_bo_unref(&entry->base.bo);
+		}
+		kvfree(entry->entries);
+	}
+
+	BUG_ON(vm->root.base.bo);
+}
+
 /**
  * amdgpu_vm_check_compute_bug - check whether asic has compute vm bug
  *
@@ -3125,36 +3154,6 @@ void amdgpu_vm_release_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 	vm->pasid = 0;
 }
 
-/**
- * amdgpu_vm_free_levels - free PD/PT levels
- *
- * @adev: amdgpu device structure
- * @parent: PD/PT starting level to free
- * @level: level of parent structure
- *
- * Free the page directory or page table level and all sub levels.
- */
-static void amdgpu_vm_free_levels(struct amdgpu_device *adev,
-				  struct amdgpu_vm_pt *parent,
-				  unsigned level)
-{
-	unsigned i, num_entries = amdgpu_vm_num_entries(adev, level);
-
-	if (parent->base.bo) {
-		list_del(&parent->base.bo_list);
-		list_del(&parent->base.vm_status);
-		amdgpu_bo_unref(&parent->base.bo->shadow);
-		amdgpu_bo_unref(&parent->base.bo);
-	}
-
-	if (parent->entries)
-		for (i = 0; i < num_entries; i++)
-			amdgpu_vm_free_levels(adev, &parent->entries[i],
-					      level + 1);
-
-	kvfree(parent->entries);
-}
-
 /**
  * amdgpu_vm_fini - tear down a vm instance
  *
@@ -3212,8 +3211,7 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 	if (r) {
 		dev_err(adev->dev, "Leaking page tables because BO reservation failed\n");
 	} else {
-		amdgpu_vm_free_levels(adev, &vm->root,
-				      adev->vm_manager.root_level);
+		amdgpu_vm_free_pts(adev, vm);
 		amdgpu_bo_unreserve(root);
 	}
 	amdgpu_bo_unref(&root);
-- 
2.14.1

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

[PATCH 4/8] drm/amdgpu: use the DFS iterator in amdgpu_vm_invalidate_pds v2

[Christian König]

Less code and easier to maintain.

v2: rename the function as well

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index a0a30416a490..c0c97b1425fa 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1370,37 +1370,22 @@ static void amdgpu_vm_update_pde(struct amdgpu_pte_update_params *params,
 }
 
 /*
- * amdgpu_vm_invalidate_level - mark all PD levels as invalid
+ * amdgpu_vm_invalidate_pds - mark all PDs as invalid
  *
  * @adev: amdgpu_device pointer
  * @vm: related vm
- * @parent: parent PD
- * @level: VMPT level
  *
  * Mark all PD level as invalid after an error.
  */
-static void amdgpu_vm_invalidate_level(struct amdgpu_device *adev,
-				       struct amdgpu_vm *vm,
-				       struct amdgpu_vm_pt *parent,
-				       unsigned level)
+static void amdgpu_vm_invalidate_pds(struct amdgpu_device *adev,
+				     struct amdgpu_vm *vm)
 {
-	unsigned pt_idx, num_entries;
-
-	/*
-	 * Recurse into the subdirectories. This recursion is harmless because
-	 * we only have a maximum of 5 layers.
-	 */
-	num_entries = amdgpu_vm_num_entries(adev, level);
-	for (pt_idx = 0; pt_idx < num_entries; ++pt_idx) {
-		struct amdgpu_vm_pt *entry = &parent->entries[pt_idx];
-
-		if (!entry->base.bo)
-			continue;
+	struct amdgpu_vm_pt_cursor cursor;
+	struct amdgpu_vm_pt *entry;
 
-		if (!entry->base.moved)
+	for_each_amdgpu_vm_pt_dfs_safe(adev, vm, cursor, entry)
+		if (entry->base.bo && !entry->base.moved)
 			amdgpu_vm_bo_relocated(&entry->base);
-		amdgpu_vm_invalidate_level(adev, vm, entry, level + 1);
-	}
 }
 
 /*
@@ -1497,8 +1482,7 @@ int amdgpu_vm_update_directories(struct amdgpu_device *adev,
 	return 0;
 
 error:
-	amdgpu_vm_invalidate_level(adev, vm, &vm->root,
-				   adev->vm_manager.root_level);
+	amdgpu_vm_invalidate_pds(adev, vm);
 	amdgpu_job_free(job);
 	return r;
 }
-- 
2.14.1

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

[PATCH 5/8] drm/amdgpu: use leaf iterator for filling PTs

[Christian König]

Less overhead and is the starting point for further cleanups and
improvements.

Signed-off-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
Reviewed-by: Junwei Zhang <Jerry.Zhang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 62 ++++++++--------------------------
 1 file changed, 15 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index c0c97b1425fa..747b6ff6fea7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1487,36 +1487,6 @@ int amdgpu_vm_update_directories(struct amdgpu_device *adev,
 	return r;
 }
 
-/**
- * amdgpu_vm_find_entry - find the entry for an address
- *
- * @p: see amdgpu_pte_update_params definition
- * @addr: virtual address in question
- * @entry: resulting entry or NULL
- * @parent: parent entry
- *
- * Find the vm_pt entry and it's parent for the given address.
- */
-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 = p->adev->vm_manager.root_level;
-
-	*parent = NULL;
-	*entry = &p->vm->root;
-	while ((*entry)->entries) {
-		unsigned shift = amdgpu_vm_level_shift(p->adev, level++);
-
-		*parent = *entry;
-		*entry = &(*entry)->entries[addr >> shift];
-		addr &= (1ULL << shift) - 1;
-	}
-
-	if (level != AMDGPU_VM_PTB)
-		*entry = NULL;
-}
-
 /**
  * amdgpu_vm_handle_huge_pages - handle updating the PD with huge pages
  *
@@ -1580,36 +1550,34 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
 {
 	struct amdgpu_device *adev = params->adev;
 	const uint64_t mask = AMDGPU_VM_PTE_COUNT(adev) - 1;
-
-	uint64_t addr, pe_start;
-	struct amdgpu_bo *pt;
-	unsigned nptes;
+	struct amdgpu_vm_pt_cursor cursor;
 
 	/* walk over the address space and update the page tables */
-	for (addr = start; addr < end; addr += nptes,
-	     dst += nptes * AMDGPU_GPU_PAGE_SIZE) {
-		struct amdgpu_vm_pt *entry, *parent;
+	for_each_amdgpu_vm_pt_leaf(adev, params->vm, start, end - 1, cursor) {
+		struct amdgpu_bo *pt = cursor.entry->base.bo;
+		uint64_t pe_start;
+		unsigned nptes;
 
-		amdgpu_vm_get_entry(params, addr, &entry, &parent);
-		if (!entry)
+		if (!pt || cursor.level != AMDGPU_VM_PTB)
 			return -ENOENT;
 
-		if ((addr & ~mask) == (end & ~mask))
-			nptes = end - addr;
+		if ((cursor.pfn & ~mask) == (end & ~mask))
+			nptes = end - cursor.pfn;
 		else
-			nptes = AMDGPU_VM_PTE_COUNT(adev) - (addr & mask);
+			nptes = AMDGPU_VM_PTE_COUNT(adev) - (cursor.pfn & mask);
 
-		amdgpu_vm_handle_huge_pages(params, entry, parent,
+		amdgpu_vm_handle_huge_pages(params, cursor.entry, cursor.parent,
 					    nptes, dst, flags);
 		/* We don't need to update PTEs for huge pages */
-		if (entry->huge)
+		if (cursor.entry->huge) {
+			dst += nptes * AMDGPU_GPU_PAGE_SIZE;
 			continue;
+		}
 
-		pt = entry->base.bo;
-		pe_start = (addr & mask) * 8;
+		pe_start = (cursor.pfn & mask) * 8;
 		amdgpu_vm_update_func(params, pt, pe_start, dst, nptes,
 				      AMDGPU_GPU_PAGE_SIZE, flags);
-
+		dst += nptes * AMDGPU_GPU_PAGE_SIZE;
 	}
 
 	return 0;
-- 
2.14.1

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

[PATCH 6/8] drm/amdgpu: meld together VM fragment and huge page handling

[Christian König]

This optimizes the generating of PTEs by walking the hierarchy only once
for a range and making changes as necessary.

It allows for both huge (2MB) as well giant (1GB) pages to be used on
Vega and Raven.

Signed-off-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
Acked-by: Junwei Zhang <Jerry.Zhang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 267 ++++++++++++++++++---------------
 1 file changed, 147 insertions(+), 120 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 747b6ff6fea7..328325324a1d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1488,46 +1488,76 @@ int amdgpu_vm_update_directories(struct amdgpu_device *adev,
 }
 
 /**
- * amdgpu_vm_handle_huge_pages - handle updating the PD with huge pages
+ * amdgpu_vm_update_huge - figure out parameters for PTE updates
  *
- * @p: see amdgpu_pte_update_params definition
- * @entry: vm_pt entry to check
- * @parent: parent entry
- * @nptes: number of PTEs updated with this operation
- * @dst: destination address where the PTEs should point to
- * @flags: access flags fro the PTEs
- *
- * Check if we can update the PD with a huge page.
+ * Make sure to set the right flags for the PTEs at the desired level.
  */
-static void amdgpu_vm_handle_huge_pages(struct amdgpu_pte_update_params *p,
-					struct amdgpu_vm_pt *entry,
-					struct amdgpu_vm_pt *parent,
-					unsigned nptes, uint64_t dst,
-					uint64_t flags)
-{
-	uint64_t pde;
+static void amdgpu_vm_update_huge(struct amdgpu_pte_update_params *params,
+				  struct amdgpu_bo *bo, unsigned level,
+				  uint64_t pe, uint64_t addr,
+				  unsigned count, uint32_t incr,
+				  uint64_t flags)
 
-	/* In the case of a mixed PT the PDE must point to it*/
-	if (p->adev->asic_type >= CHIP_VEGA10 && !p->src &&
-	    nptes == AMDGPU_VM_PTE_COUNT(p->adev)) {
-		/* Set the huge page flag to stop scanning at this PDE */
+{
+	if (level != AMDGPU_VM_PTB) {
 		flags |= AMDGPU_PDE_PTE;
+		amdgpu_gmc_get_vm_pde(params->adev, level, &addr, &flags);
 	}
 
-	if (!(flags & AMDGPU_PDE_PTE)) {
-		if (entry->huge) {
-			/* Add the entry to the relocated list to update it. */
-			entry->huge = false;
-			amdgpu_vm_bo_relocated(&entry->base);
-		}
+	amdgpu_vm_update_func(params, bo, pe, addr, count, incr, flags);
+}
+
+/**
+ * amdgpu_vm_fragment - get fragment for PTEs
+ *
+ * @params: see amdgpu_pte_update_params definition
+ * @start: first PTE to handle
+ * @end: last PTE to handle
+ * @flags: hw mapping flags
+ * @frag: resulting fragment size
+ * @frag_end: end of this fragment
+ *
+ * Returns the first possible fragment for the start and end address.
+ */
+static void amdgpu_vm_fragment(struct amdgpu_pte_update_params *params,
+			       uint64_t start, uint64_t end, uint64_t flags,
+			       unsigned int *frag, uint64_t *frag_end)
+{
+	/**
+	 * The MC L1 TLB supports variable sized pages, based on a fragment
+	 * field in the PTE. When this field is set to a non-zero value, page
+	 * granularity is increased from 4KB to (1 << (12 + frag)). The PTE
+	 * flags are considered valid for all PTEs within the fragment range
+	 * and corresponding mappings are assumed to be physically contiguous.
+	 *
+	 * The L1 TLB can store a single PTE for the whole fragment,
+	 * significantly increasing the space available for translation
+	 * caching. This leads to large improvements in throughput when the
+	 * TLB is under pressure.
+	 *
+	 * The L2 TLB distributes small and large fragments into two
+	 * asymmetric partitions. The large fragment cache is significantly
+	 * larger. Thus, we try to use large fragments wherever possible.
+	 * Userspace can support this by aligning virtual base address and
+	 * allocation size to the fragment size.
+	 */
+	unsigned max_frag = params->adev->vm_manager.fragment_size;
+
+	/* system pages are non continuously */
+	if (params->src || !(flags & AMDGPU_PTE_VALID)) {
+		*frag = 0;
+		*frag_end = end;
 		return;
 	}
 
-	entry->huge = true;
-	amdgpu_gmc_get_vm_pde(p->adev, AMDGPU_VM_PDB0, &dst, &flags);
-
-	pde = (entry - parent->entries) * 8;
-	amdgpu_vm_update_func(p, parent->base.bo, pde, dst, 1, 0, flags);
+	/* This intentionally wraps around if no bit is set */
+	*frag = min((unsigned)ffs(start) - 1, (unsigned)fls64(end - start) - 1);
+	if (*frag >= max_frag) {
+		*frag = max_frag;
+		*frag_end = end & ~((1ULL << max_frag) - 1);
+	} else {
+		*frag_end = start + (1 << *frag);
+	}
 }
 
 /**
@@ -1545,108 +1575,105 @@ static void amdgpu_vm_handle_huge_pages(struct amdgpu_pte_update_params *p,
  * 0 for success, -EINVAL for failure.
  */
 static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
-				  uint64_t start, uint64_t end,
-				  uint64_t dst, uint64_t flags)
+				 uint64_t start, uint64_t end,
+				 uint64_t dst, uint64_t flags)
 {
 	struct amdgpu_device *adev = params->adev;
-	const uint64_t mask = AMDGPU_VM_PTE_COUNT(adev) - 1;
 	struct amdgpu_vm_pt_cursor cursor;
+	uint64_t frag_start = start, frag_end;
+	unsigned int frag;
 
-	/* walk over the address space and update the page tables */
-	for_each_amdgpu_vm_pt_leaf(adev, params->vm, start, end - 1, cursor) {
+	/* figure out the initial fragment */
+	amdgpu_vm_fragment(params, frag_start, end, flags, &frag, &frag_end);
+
+	/* walk over the address space and update the PTs */
+	amdgpu_vm_pt_start(adev, params->vm, start, &cursor);
+	while (cursor.pfn < end) {
 		struct amdgpu_bo *pt = cursor.entry->base.bo;
-		uint64_t pe_start;
-		unsigned nptes;
+		unsigned shift, parent_shift, num_entries;
+		uint64_t incr, entry_end, pe_start;
 
-		if (!pt || cursor.level != AMDGPU_VM_PTB)
+		if (!pt)
 			return -ENOENT;
 
-		if ((cursor.pfn & ~mask) == (end & ~mask))
-			nptes = end - cursor.pfn;
-		else
-			nptes = AMDGPU_VM_PTE_COUNT(adev) - (cursor.pfn & mask);
-
-		amdgpu_vm_handle_huge_pages(params, cursor.entry, cursor.parent,
-					    nptes, dst, flags);
-		/* We don't need to update PTEs for huge pages */
-		if (cursor.entry->huge) {
-			dst += nptes * AMDGPU_GPU_PAGE_SIZE;
+		/* The root level can't be a huge page */
+		if (cursor.level == adev->vm_manager.root_level) {
+			if (!amdgpu_vm_pt_descendant(adev, &cursor))
+				return -ENOENT;
 			continue;
 		}
 
-		pe_start = (cursor.pfn & mask) * 8;
-		amdgpu_vm_update_func(params, pt, pe_start, dst, nptes,
-				      AMDGPU_GPU_PAGE_SIZE, flags);
-		dst += nptes * AMDGPU_GPU_PAGE_SIZE;
-	}
-
-	return 0;
-}
+		/* First check if the entry is already handled */
+		if (cursor.pfn < frag_start) {
+			cursor.entry->huge = true;
+			amdgpu_vm_pt_next(adev, &cursor);
+			continue;
+		}
 
-/*
- * amdgpu_vm_frag_ptes - add fragment information to PTEs
- *
- * @params: see amdgpu_pte_update_params definition
- * @vm: requested vm
- * @start: first PTE to handle
- * @end: last PTE to handle
- * @dst: addr those PTEs should point to
- * @flags: hw mapping flags
- *
- * Returns:
- * 0 for success, -EINVAL for failure.
- */
-static int amdgpu_vm_frag_ptes(struct amdgpu_pte_update_params	*params,
-				uint64_t start, uint64_t end,
-				uint64_t dst, uint64_t flags)
-{
-	/**
-	 * The MC L1 TLB supports variable sized pages, based on a fragment
-	 * field in the PTE. When this field is set to a non-zero value, page
-	 * granularity is increased from 4KB to (1 << (12 + frag)). The PTE
-	 * flags are considered valid for all PTEs within the fragment range
-	 * and corresponding mappings are assumed to be physically contiguous.
-	 *
-	 * The L1 TLB can store a single PTE for the whole fragment,
-	 * significantly increasing the space available for translation
-	 * caching. This leads to large improvements in throughput when the
-	 * TLB is under pressure.
-	 *
-	 * The L2 TLB distributes small and large fragments into two
-	 * asymmetric partitions. The large fragment cache is significantly
-	 * larger. Thus, we try to use large fragments wherever possible.
-	 * Userspace can support this by aligning virtual base address and
-	 * allocation size to the fragment size.
-	 */
-	unsigned max_frag = params->adev->vm_manager.fragment_size;
-	int r;
+		/* If it isn't already handled it can't be a huge page */
+		if (cursor.entry->huge) {
+			/* Add the entry to the relocated list to update it. */
+			cursor.entry->huge = false;
+			amdgpu_vm_bo_relocated(&cursor.entry->base);
+		}
 
-	/* system pages are non continuously */
-	if (params->src || !(flags & AMDGPU_PTE_VALID))
-		return amdgpu_vm_update_ptes(params, start, end, dst, flags);
-
-	while (start != end) {
-		uint64_t frag_flags, frag_end;
-		unsigned frag;
-
-		/* This intentionally wraps around if no bit is set */
-		frag = min((unsigned)ffs(start) - 1,
-			   (unsigned)fls64(end - start) - 1);
-		if (frag >= max_frag) {
-			frag_flags = AMDGPU_PTE_FRAG(max_frag);
-			frag_end = end & ~((1ULL << max_frag) - 1);
-		} else {
-			frag_flags = AMDGPU_PTE_FRAG(frag);
-			frag_end = start + (1 << frag);
+		shift = amdgpu_vm_level_shift(adev, cursor.level);
+		parent_shift = amdgpu_vm_level_shift(adev, cursor.level - 1);
+		if (adev->asic_type < CHIP_VEGA10) {
+			/* No huge page support before GMC v9 */
+			if (cursor.level != AMDGPU_VM_PTB) {
+				if (!amdgpu_vm_pt_descendant(adev, &cursor))
+					return -ENOENT;
+				continue;
+			}
+		} else if (frag < shift) {
+			/* We can't use this level when the fragment size is
+			 * smaller than the address shift. Go to the next
+			 * child entry and try again.
+			 */
+			if (!amdgpu_vm_pt_descendant(adev, &cursor))
+				return -ENOENT;
+			continue;
+		} else if (frag >= parent_shift) {
+			/* If the fragment size is even larger than the parent
+			 * shift we should go up one level and check it again.
+			 */
+			if (!amdgpu_vm_pt_ancestor(&cursor))
+				return -ENOENT;
+			continue;
 		}
 
-		r = amdgpu_vm_update_ptes(params, start, frag_end, dst,
-					  flags | frag_flags);
-		if (r)
-			return r;
+		/* Looks good so far, calculate parameters for the update */
+		incr = AMDGPU_GPU_PAGE_SIZE << shift;
+		num_entries = amdgpu_vm_num_entries(adev, cursor.level);
+		pe_start = ((cursor.pfn >> shift) & (num_entries - 1)) * 8;
+		entry_end = num_entries << shift;
+		entry_end += cursor.pfn & ~(entry_end - 1);
+		entry_end = min(entry_end, end);
+
+		do {
+			uint64_t upd_end = min(entry_end, frag_end);
+			unsigned nptes = (upd_end - frag_start) >> shift;
+
+			amdgpu_vm_update_huge(params, pt, cursor.level,
+					      pe_start, dst, nptes, incr,
+					      flags | AMDGPU_PTE_FRAG(frag));
+
+			pe_start += nptes * 8;
+			dst += nptes * AMDGPU_GPU_PAGE_SIZE << shift;
+
+			frag_start = upd_end;
+			if (frag_start >= frag_end) {
+				/* figure out the next fragment */
+				amdgpu_vm_fragment(params, frag_start, end,
+						   flags, &frag, &frag_end);
+				if (frag < shift)
+					break;
+			}
+		} while (frag_start < entry_end);
 
-		dst += (frag_end - start) * AMDGPU_GPU_PAGE_SIZE;
-		start = frag_end;
+		if (frag >= shift)
+			amdgpu_vm_pt_next(adev, &cursor);
 	}
 
 	return 0;
@@ -1708,8 +1735,8 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
 
 		params.func = amdgpu_vm_cpu_set_ptes;
 		params.pages_addr = pages_addr;
-		return amdgpu_vm_frag_ptes(&params, start, last + 1,
-					   addr, flags);
+		return amdgpu_vm_update_ptes(&params, start, last + 1,
+					     addr, flags);
 	}
 
 	ring = container_of(vm->entity.rq->sched, struct amdgpu_ring, sched);
@@ -1788,7 +1815,7 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
 	if (r)
 		goto error_free;
 
-	r = amdgpu_vm_frag_ptes(&params, start, last + 1, addr, flags);
+	r = amdgpu_vm_update_ptes(&params, start, last + 1, addr, flags);
 	if (r)
 		goto error_free;
 
-- 
2.14.1

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

[PATCH 7/8] drm/amdgpu: use the maximum possible fragment size on Vega/Raven

[Christian König]

The fragment size controls only the L1 on Vega/Raven and we now don't
have any extra overhead any more because of larger fragments.

Signed-off-by: Christian König <christian.koenig@amd.com>
Acked-by: Junwei Zhang <Jerry.Zhang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 328325324a1d..bc258c591589 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1540,8 +1540,16 @@ static void amdgpu_vm_fragment(struct amdgpu_pte_update_params *params,
 	 * larger. Thus, we try to use large fragments wherever possible.
 	 * Userspace can support this by aligning virtual base address and
 	 * allocation size to the fragment size.
+	 *
+	 * Starting with Vega10 the fragment size only controls the L1. The L2
+	 * is now directly feed with small/huge/giant pages from the walker.
 	 */
-	unsigned max_frag = params->adev->vm_manager.fragment_size;
+	unsigned max_frag;
+
+	if (params->adev->asic_type < CHIP_VEGA10)
+		max_frag = params->adev->vm_manager.fragment_size;
+	else
+		max_frag = 31;
 
 	/* system pages are non continuously */
 	if (params->src || !(flags & AMDGPU_PTE_VALID)) {
-- 
2.14.1

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

[PATCH 8/8] drm/amdgpu: allow fragment processing for invalid PTEs

[Christian König]

That should improve the PRT performance on Vega quite a bit.

Signed-off-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
Acked-by: Junwei Zhang <Jerry.Zhang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index bc258c591589..3e37b119371d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1552,7 +1552,7 @@ static void amdgpu_vm_fragment(struct amdgpu_pte_update_params *params,
 		max_frag = 31;
 
 	/* system pages are non continuously */
-	if (params->src || !(flags & AMDGPU_PTE_VALID)) {
+	if (params->src) {
 		*frag = 0;
 		*frag_end = end;
 		return;
-- 
2.14.1

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

Re: [PATCH 1/8] drm/amdgpu: add some VM PD/PT iterators v2

[Felix Kuehling]

The series is Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>


On 2018-09-12 04:54 AM, Christian König wrote:
> Both a leaf as well as dfs iterator to walk over all the PDs/PTs.
>
> v2: update comments and fix for_each_amdgpu_vm_pt_dfs_safe
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 224 +++++++++++++++++++++++++++++++++
>  1 file changed, 224 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 136b00412dc8..787a200cf796 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -355,6 +355,230 @@ static struct amdgpu_vm_pt *amdgpu_vm_pt_parent(struct amdgpu_vm_pt *pt)
>  	return list_first_entry(&parent->va, struct amdgpu_vm_pt, base.bo_list);
>  }
>  
> +/**
> + * amdgpu_vm_pt_cursor - state for for_each_amdgpu_vm_pt
> + */
> +struct amdgpu_vm_pt_cursor {
> +	uint64_t pfn;
> +	struct amdgpu_vm_pt *parent;
> +	struct amdgpu_vm_pt *entry;
> +	unsigned level;
> +};
> +
> +/**
> + * amdgpu_vm_pt_start - start PD/PT walk
> + *
> + * @adev: amdgpu_device pointer
> + * @vm: amdgpu_vm structure
> + * @start: start address of the walk
> + * @cursor: state to initialize
> + *
> + * Initialize a amdgpu_vm_pt_cursor to start a walk.
> + */
> +static void amdgpu_vm_pt_start(struct amdgpu_device *adev,
> +			       struct amdgpu_vm *vm, uint64_t start,
> +			       struct amdgpu_vm_pt_cursor *cursor)
> +{
> +	cursor->pfn = start;
> +	cursor->parent = NULL;
> +	cursor->entry = &vm->root;
> +	cursor->level = adev->vm_manager.root_level;
> +}
> +
> +/**
> + * amdgpu_vm_pt_descendant - go to child node
> + *
> + * @adev: amdgpu_device pointer
> + * @cursor: current state
> + *
> + * Walk to the child node of the current node.
> + * Returns:
> + * True if the walk was possible, false otherwise.
> + */
> +static bool amdgpu_vm_pt_descendant(struct amdgpu_device *adev,
> +				    struct amdgpu_vm_pt_cursor *cursor)
> +{
> +	unsigned num_entries, shift, idx;
> +
> +	if (!cursor->entry->entries)
> +		return false;
> +
> +	BUG_ON(!cursor->entry->base.bo);
> +	num_entries = amdgpu_vm_num_entries(adev, cursor->level);
> +	shift = amdgpu_vm_level_shift(adev, cursor->level);
> +
> +	++cursor->level;
> +	idx = (cursor->pfn >> shift) % num_entries;
> +	cursor->parent = cursor->entry;
> +	cursor->entry = &cursor->entry->entries[idx];
> +	return true;
> +}
> +
> +/**
> + * amdgpu_vm_pt_sibling - go to sibling node
> + *
> + * @adev: amdgpu_device pointer
> + * @cursor: current state
> + *
> + * Walk to the sibling node of the current node.
> + * Returns:
> + * True if the walk was possible, false otherwise.
> + */
> +static bool amdgpu_vm_pt_sibling(struct amdgpu_device *adev,
> +				 struct amdgpu_vm_pt_cursor *cursor)
> +{
> +	unsigned shift, num_entries;
> +
> +	/* Root doesn't have a sibling */
> +	if (!cursor->parent)
> +		return false;
> +
> +	/* Go to our parents and see if we got a sibling */
> +	shift = amdgpu_vm_level_shift(adev, cursor->level - 1);
> +	num_entries = amdgpu_vm_num_entries(adev, cursor->level - 1);
> +
> +	if (cursor->entry == &cursor->parent->entries[num_entries - 1])
> +		return false;
> +
> +	cursor->pfn += 1ULL << shift;
> +	cursor->pfn &= ~((1ULL << shift) - 1);
> +	++cursor->entry;
> +	return true;
> +}
> +
> +/**
> + * amdgpu_vm_pt_ancestor - go to parent node
> + *
> + * @cursor: current state
> + *
> + * Walk to the parent node of the current node.
> + * Returns:
> + * True if the walk was possible, false otherwise.
> + */
> +static bool amdgpu_vm_pt_ancestor(struct amdgpu_vm_pt_cursor *cursor)
> +{
> +	if (!cursor->parent)
> +		return false;
> +
> +	--cursor->level;
> +	cursor->entry = cursor->parent;
> +	cursor->parent = amdgpu_vm_pt_parent(cursor->parent);
> +	return true;
> +}
> +
> +/**
> + * amdgpu_vm_pt_next - get next PD/PT in hieratchy
> + *
> + * @adev: amdgpu_device pointer
> + * @cursor: current state
> + *
> + * Walk the PD/PT tree to the next node.
> + */
> +static void amdgpu_vm_pt_next(struct amdgpu_device *adev,
> +			      struct amdgpu_vm_pt_cursor *cursor)
> +{
> +	/* First try a newborn child */
> +	if (amdgpu_vm_pt_descendant(adev, cursor))
> +		return;
> +
> +	/* If that didn't worked try to find a sibling */
> +	while (!amdgpu_vm_pt_sibling(adev, cursor)) {
> +		/* No sibling, go to our parents and grandparents */
> +		if (!amdgpu_vm_pt_ancestor(cursor)) {
> +			cursor->pfn = ~0ll;
> +			return;
> +		}
> +	}
> +}
> +
> +/**
> + * amdgpu_vm_pt_first_leaf - get first leaf PD/PT
> + *
> + * @adev: amdgpu_device pointer
> + * @vm: amdgpu_vm structure
> + * @start: start addr of the walk
> + * @cursor: state to initialize
> + *
> + * Start a walk and go directly to the leaf node.
> + */
> +static void amdgpu_vm_pt_first_leaf(struct amdgpu_device *adev,
> +				    struct amdgpu_vm *vm, uint64_t start,
> +				    struct amdgpu_vm_pt_cursor *cursor)
> +{
> +	amdgpu_vm_pt_start(adev, vm, start, cursor);
> +	while (amdgpu_vm_pt_descendant(adev, cursor));
> +}
> +
> +/**
> + * amdgpu_vm_pt_next_leaf - get next leaf PD/PT
> + *
> + * @adev: amdgpu_device pointer
> + * @cursor: current state
> + *
> + * Walk the PD/PT tree to the next leaf node.
> + */
> +static void amdgpu_vm_pt_next_leaf(struct amdgpu_device *adev,
> +				   struct amdgpu_vm_pt_cursor *cursor)
> +{
> +	amdgpu_vm_pt_next(adev, cursor);
> +	while (amdgpu_vm_pt_descendant(adev, cursor));
> +}
> +
> +/**
> + * for_each_amdgpu_vm_pt_leaf - walk over all leaf PDs/PTs in the hierarchy
> + */
> +#define for_each_amdgpu_vm_pt_leaf(adev, vm, start, end, cursor)		\
> +	for (amdgpu_vm_pt_first_leaf((adev), (vm), (start), &(cursor));		\
> +	     (cursor).pfn <= end; amdgpu_vm_pt_next_leaf((adev), &(cursor)))
> +
> +/**
> + * amdgpu_vm_pt_first_dfs - start a deep first search
> + *
> + * @adev: amdgpu_device structure
> + * @vm: amdgpu_vm structure
> + * @cursor: state to initialize
> + *
> + * Starts a deep first traversal of the PD/PT tree.
> + */
> +static void amdgpu_vm_pt_first_dfs(struct amdgpu_device *adev,
> +				   struct amdgpu_vm *vm,
> +				   struct amdgpu_vm_pt_cursor *cursor)
> +{
> +	amdgpu_vm_pt_start(adev, vm, 0, cursor);
> +	while (amdgpu_vm_pt_descendant(adev, cursor));
> +}
> +
> +/**
> + * amdgpu_vm_pt_next_dfs - get the next node for a deep first search
> + *
> + * @adev: amdgpu_device structure
> + * @cursor: current state
> + *
> + * Move the cursor to the next node in a deep first search.
> + */
> +static void amdgpu_vm_pt_next_dfs(struct amdgpu_device *adev,
> +				  struct amdgpu_vm_pt_cursor *cursor)
> +{
> +	if (!cursor->entry)
> +		return;
> +
> +	if (!cursor->parent)
> +		cursor->entry = NULL;
> +	else if (amdgpu_vm_pt_sibling(adev, cursor))
> +		while (amdgpu_vm_pt_descendant(adev, cursor));
> +	else
> +		amdgpu_vm_pt_ancestor(cursor);
> +}
> +
> +/**
> + * for_each_amdgpu_vm_pt_dfs_safe - safe deep first search of all PDs/PTs
> + */
> +#define for_each_amdgpu_vm_pt_dfs_safe(adev, vm, cursor, entry)			\
> +	for (amdgpu_vm_pt_first_dfs((adev), (vm), &(cursor)),			\
> +	     (entry) = (cursor).entry, amdgpu_vm_pt_next_dfs((adev), &(cursor));\
> +	     (entry); (entry) = (cursor).entry,					\
> +	     amdgpu_vm_pt_next_dfs((adev), &(cursor)))
> +
>  /**
>   * amdgpu_vm_get_pd_bo - add the VM PD to a validation list
>   *

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

Re: [PATCH 1/8] drm/amdgpu: add some VM PD/PT iterators v2

[Zhang, Jerry(Junwei)]

[-- Attachment #1.1: Type: text/plain, Size: 7068 bytes --]

On 09/12/2018 04:54 PM, Christian König wrote:
> Both a leaf as well as dfs iterator to walk over all the PDs/PTs. v2: 
> update comments and fix for_each_amdgpu_vm_pt_dfs_safe Signed-off-by: 
> Christian König <christian.koenig-5C7GfCeVMHo@public.gmane.org>
Reviewed-by: Junwei Zhang <Jerry.Zhang-5C7GfCeVMHo@public.gmane.org>
> --- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 224 
> +++++++++++++++++++++++++++++++++ 1 file changed, 224 insertions(+) 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index 
> 136b00412dc8..787a200cf796 100644 --- 
> a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -355,6 +355,230 @@ static 
> struct amdgpu_vm_pt *amdgpu_vm_pt_parent(struct amdgpu_vm_pt *pt) 
> return list_first_entry(&parent->va, struct amdgpu_vm_pt, 
> base.bo_list); } +/** + * amdgpu_vm_pt_cursor - state for 
> for_each_amdgpu_vm_pt + */ +struct amdgpu_vm_pt_cursor { + uint64_t 
> pfn; + struct amdgpu_vm_pt *parent; + struct amdgpu_vm_pt *entry; + 
> unsigned level; +}; + +/** + * amdgpu_vm_pt_start - start PD/PT walk + 
> * + * @adev: amdgpu_device pointer + * @vm: amdgpu_vm structure + * 
> @start: start address of the walk + * @cursor: state to initialize + * 
> + * Initialize a amdgpu_vm_pt_cursor to start a walk. + */ +static 
> void amdgpu_vm_pt_start(struct amdgpu_device *adev, + struct amdgpu_vm 
> *vm, uint64_t start, + struct amdgpu_vm_pt_cursor *cursor) +{ + 
> cursor->pfn = start; + cursor->parent = NULL; + cursor->entry = 
> &vm->root; + cursor->level = adev->vm_manager.root_level; +} + +/** + 
> * amdgpu_vm_pt_descendant - go to child node + * + * @adev: 
> amdgpu_device pointer + * @cursor: current state + * + * Walk to the 
> child node of the current node. + * Returns: + * True if the walk was 
> possible, false otherwise. + */ +static bool 
> amdgpu_vm_pt_descendant(struct amdgpu_device *adev, + struct 
> amdgpu_vm_pt_cursor *cursor) +{ + unsigned num_entries, shift, idx; + 
> + if (!cursor->entry->entries) + return false; + + 
> BUG_ON(!cursor->entry->base.bo); + num_entries = 
> amdgpu_vm_num_entries(adev, cursor->level); + shift = 
> amdgpu_vm_level_shift(adev, cursor->level); + + ++cursor->level; + idx 
> = (cursor->pfn >> shift) % num_entries; + cursor->parent = 
> cursor->entry; + cursor->entry = &cursor->entry->entries[idx]; + 
> return true; +} + +/** + * amdgpu_vm_pt_sibling - go to sibling node + 
> * + * @adev: amdgpu_device pointer + * @cursor: current state + * + * 
> Walk to the sibling node of the current node. + * Returns: + * True if 
> the walk was possible, false otherwise. + */ +static bool 
> amdgpu_vm_pt_sibling(struct amdgpu_device *adev, + struct 
> amdgpu_vm_pt_cursor *cursor) +{ + unsigned shift, num_entries; + + /* 
> Root doesn't have a sibling */ + if (!cursor->parent) + return false; 
> + + /* Go to our parents and see if we got a sibling */ + shift = 
> amdgpu_vm_level_shift(adev, cursor->level - 1); + num_entries = 
> amdgpu_vm_num_entries(adev, cursor->level - 1); + + if (cursor->entry 
> == &cursor->parent->entries[num_entries - 1]) + return false; + + 
> cursor->pfn += 1ULL << shift; + cursor->pfn &= ~((1ULL << shift) - 1); 
> + ++cursor->entry; + return true; +} + +/** + * amdgpu_vm_pt_ancestor 
> - go to parent node + * + * @cursor: current state + * + * Walk to the 
> parent node of the current node. + * Returns: + * True if the walk was 
> possible, false otherwise. + */ +static bool 
> amdgpu_vm_pt_ancestor(struct amdgpu_vm_pt_cursor *cursor) +{ + if 
> (!cursor->parent) + return false; + + --cursor->level; + cursor->entry 
> = cursor->parent; + cursor->parent = 
> amdgpu_vm_pt_parent(cursor->parent); + return true; +} + +/** + * 
> amdgpu_vm_pt_next - get next PD/PT in hieratchy + * + * @adev: 
> amdgpu_device pointer + * @cursor: current state + * + * Walk the 
> PD/PT tree to the next node. + */ +static void 
> amdgpu_vm_pt_next(struct amdgpu_device *adev, + struct 
> amdgpu_vm_pt_cursor *cursor) +{ + /* First try a newborn child */ + if 
> (amdgpu_vm_pt_descendant(adev, cursor)) + return; + + /* If that 
> didn't worked try to find a sibling */ + while 
> (!amdgpu_vm_pt_sibling(adev, cursor)) { + /* No sibling, go to our 
> parents and grandparents */ + if (!amdgpu_vm_pt_ancestor(cursor)) { + 
> cursor->pfn = ~0ll; + return; + } + } +} + +/** + * 
> amdgpu_vm_pt_first_leaf - get first leaf PD/PT + * + * @adev: 
> amdgpu_device pointer + * @vm: amdgpu_vm structure + * @start: start 
> addr of the walk + * @cursor: state to initialize + * + * Start a walk 
> and go directly to the leaf node. + */ +static void 
> amdgpu_vm_pt_first_leaf(struct amdgpu_device *adev, + struct amdgpu_vm 
> *vm, uint64_t start, + struct amdgpu_vm_pt_cursor *cursor) +{ + 
> amdgpu_vm_pt_start(adev, vm, start, cursor); + while 
> (amdgpu_vm_pt_descendant(adev, cursor)); +} + +/** + * 
> amdgpu_vm_pt_next_leaf - get next leaf PD/PT + * + * @adev: 
> amdgpu_device pointer + * @cursor: current state + * + * Walk the 
> PD/PT tree to the next leaf node. + */ +static void 
> amdgpu_vm_pt_next_leaf(struct amdgpu_device *adev, + struct 
> amdgpu_vm_pt_cursor *cursor) +{ + amdgpu_vm_pt_next(adev, cursor); + 
> while (amdgpu_vm_pt_descendant(adev, cursor)); +} + +/** + * 
> for_each_amdgpu_vm_pt_leaf - walk over all leaf PDs/PTs in the 
> hierarchy + */ +#define for_each_amdgpu_vm_pt_leaf(adev, vm, start, 
> end, cursor) \ + for (amdgpu_vm_pt_first_leaf((adev), (vm), (start), 
> &(cursor)); \ + (cursor).pfn <= end; amdgpu_vm_pt_next_leaf((adev), 
> &(cursor))) + +/** + * amdgpu_vm_pt_first_dfs - start a deep first 
> search + * + * @adev: amdgpu_device structure + * @vm: amdgpu_vm 
> structure + * @cursor: state to initialize + * + * Starts a deep first 
> traversal of the PD/PT tree. + */ +static void 
> amdgpu_vm_pt_first_dfs(struct amdgpu_device *adev, + struct amdgpu_vm 
> *vm, + struct amdgpu_vm_pt_cursor *cursor) +{ + 
> amdgpu_vm_pt_start(adev, vm, 0, cursor); + while 
> (amdgpu_vm_pt_descendant(adev, cursor)); +} + +/** + * 
> amdgpu_vm_pt_next_dfs - get the next node for a deep first search + * 
> + * @adev: amdgpu_device structure + * @cursor: current state + * + * 
> Move the cursor to the next node in a deep first search. + */ +static 
> void amdgpu_vm_pt_next_dfs(struct amdgpu_device *adev, + struct 
> amdgpu_vm_pt_cursor *cursor) +{ + if (!cursor->entry) + return; + + if 
> (!cursor->parent) + cursor->entry = NULL; + else if 
> (amdgpu_vm_pt_sibling(adev, cursor)) + while 
> (amdgpu_vm_pt_descendant(adev, cursor)); + else + 
> amdgpu_vm_pt_ancestor(cursor); +} + +/** + * 
> for_each_amdgpu_vm_pt_dfs_safe - safe deep first search of all PDs/PTs 
> + */ +#define for_each_amdgpu_vm_pt_dfs_safe(adev, vm, cursor, entry) 
> \ + for (amdgpu_vm_pt_first_dfs((adev), (vm), &(cursor)), \ + (entry) 
> = (cursor).entry, amdgpu_vm_pt_next_dfs((adev), &(cursor));\ + 
> (entry); (entry) = (cursor).entry, \ + amdgpu_vm_pt_next_dfs((adev), 
> &(cursor))) + /** * amdgpu_vm_get_pd_bo - add the VM PD to a 
> validation list *


[-- Attachment #1.2: Type: text/html, Size: 7943 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

Re: [PATCH 4/8] drm/amdgpu: use the DFS iterator in amdgpu_vm_invalidate_pds v2

[Zhang, Jerry(Junwei)]

On 09/12/2018 04:54 PM, Christian König wrote:
> Less code and easier to maintain.
>
> v2: rename the function as well
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Junwei Zhang <Jerry.Zhang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 32 ++++++++------------------------
>   1 file changed, 8 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index a0a30416a490..c0c97b1425fa 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1370,37 +1370,22 @@ static void amdgpu_vm_update_pde(struct amdgpu_pte_update_params *params,
>   }
>   
>   /*
> - * amdgpu_vm_invalidate_level - mark all PD levels as invalid
> + * amdgpu_vm_invalidate_pds - mark all PDs as invalid
>    *
>    * @adev: amdgpu_device pointer
>    * @vm: related vm
> - * @parent: parent PD
> - * @level: VMPT level
>    *
>    * Mark all PD level as invalid after an error.
>    */
> -static void amdgpu_vm_invalidate_level(struct amdgpu_device *adev,
> -				       struct amdgpu_vm *vm,
> -				       struct amdgpu_vm_pt *parent,
> -				       unsigned level)
> +static void amdgpu_vm_invalidate_pds(struct amdgpu_device *adev,
> +				     struct amdgpu_vm *vm)
>   {
> -	unsigned pt_idx, num_entries;
> -
> -	/*
> -	 * Recurse into the subdirectories. This recursion is harmless because
> -	 * we only have a maximum of 5 layers.
> -	 */
> -	num_entries = amdgpu_vm_num_entries(adev, level);
> -	for (pt_idx = 0; pt_idx < num_entries; ++pt_idx) {
> -		struct amdgpu_vm_pt *entry = &parent->entries[pt_idx];
> -
> -		if (!entry->base.bo)
> -			continue;
> +	struct amdgpu_vm_pt_cursor cursor;
> +	struct amdgpu_vm_pt *entry;
>   
> -		if (!entry->base.moved)
> +	for_each_amdgpu_vm_pt_dfs_safe(adev, vm, cursor, entry)
> +		if (entry->base.bo && !entry->base.moved)
>   			amdgpu_vm_bo_relocated(&entry->base);
> -		amdgpu_vm_invalidate_level(adev, vm, entry, level + 1);
> -	}
>   }
>   
>   /*
> @@ -1497,8 +1482,7 @@ int amdgpu_vm_update_directories(struct amdgpu_device *adev,
>   	return 0;
>   
>   error:
> -	amdgpu_vm_invalidate_level(adev, vm, &vm->root,
> -				   adev->vm_manager.root_level);
> +	amdgpu_vm_invalidate_pds(adev, vm);
>   	amdgpu_job_free(job);
>   	return r;
>   }

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

Re: [PATCH 1/8] drm/amdgpu: add some VM PD/PT iterators v2

[Huang Rui]

On Wed, Sep 12, 2018 at 10:54:38AM +0200, Christian König wrote:
> Both a leaf as well as dfs iterator to walk over all the PDs/PTs.
> 
> v2: update comments and fix for_each_amdgpu_vm_pt_dfs_safe
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>

Series are Reviewed-by: Huang Rui <ray.huang@amd.com>

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 224 +++++++++++++++++++++++++++++++++
>  1 file changed, 224 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 136b00412dc8..787a200cf796 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -355,6 +355,230 @@ static struct amdgpu_vm_pt *amdgpu_vm_pt_parent(struct amdgpu_vm_pt *pt)
>  	return list_first_entry(&parent->va, struct amdgpu_vm_pt, base.bo_list);
>  }
>  
> +/**
> + * amdgpu_vm_pt_cursor - state for for_each_amdgpu_vm_pt
> + */
> +struct amdgpu_vm_pt_cursor {
> +	uint64_t pfn;
> +	struct amdgpu_vm_pt *parent;
> +	struct amdgpu_vm_pt *entry;
> +	unsigned level;
> +};
> +
> +/**
> + * amdgpu_vm_pt_start - start PD/PT walk
> + *
> + * @adev: amdgpu_device pointer
> + * @vm: amdgpu_vm structure
> + * @start: start address of the walk
> + * @cursor: state to initialize
> + *
> + * Initialize a amdgpu_vm_pt_cursor to start a walk.
> + */
> +static void amdgpu_vm_pt_start(struct amdgpu_device *adev,
> +			       struct amdgpu_vm *vm, uint64_t start,
> +			       struct amdgpu_vm_pt_cursor *cursor)
> +{
> +	cursor->pfn = start;
> +	cursor->parent = NULL;
> +	cursor->entry = &vm->root;
> +	cursor->level = adev->vm_manager.root_level;
> +}
> +
> +/**
> + * amdgpu_vm_pt_descendant - go to child node
> + *
> + * @adev: amdgpu_device pointer
> + * @cursor: current state
> + *
> + * Walk to the child node of the current node.
> + * Returns:
> + * True if the walk was possible, false otherwise.
> + */
> +static bool amdgpu_vm_pt_descendant(struct amdgpu_device *adev,
> +				    struct amdgpu_vm_pt_cursor *cursor)
> +{
> +	unsigned num_entries, shift, idx;
> +
> +	if (!cursor->entry->entries)
> +		return false;
> +
> +	BUG_ON(!cursor->entry->base.bo);
> +	num_entries = amdgpu_vm_num_entries(adev, cursor->level);
> +	shift = amdgpu_vm_level_shift(adev, cursor->level);
> +
> +	++cursor->level;
> +	idx = (cursor->pfn >> shift) % num_entries;
> +	cursor->parent = cursor->entry;
> +	cursor->entry = &cursor->entry->entries[idx];
> +	return true;
> +}
> +
> +/**
> + * amdgpu_vm_pt_sibling - go to sibling node
> + *
> + * @adev: amdgpu_device pointer
> + * @cursor: current state
> + *
> + * Walk to the sibling node of the current node.
> + * Returns:
> + * True if the walk was possible, false otherwise.
> + */
> +static bool amdgpu_vm_pt_sibling(struct amdgpu_device *adev,
> +				 struct amdgpu_vm_pt_cursor *cursor)
> +{
> +	unsigned shift, num_entries;
> +
> +	/* Root doesn't have a sibling */
> +	if (!cursor->parent)
> +		return false;
> +
> +	/* Go to our parents and see if we got a sibling */
> +	shift = amdgpu_vm_level_shift(adev, cursor->level - 1);
> +	num_entries = amdgpu_vm_num_entries(adev, cursor->level - 1);
> +
> +	if (cursor->entry == &cursor->parent->entries[num_entries - 1])
> +		return false;
> +
> +	cursor->pfn += 1ULL << shift;
> +	cursor->pfn &= ~((1ULL << shift) - 1);
> +	++cursor->entry;
> +	return true;
> +}
> +
> +/**
> + * amdgpu_vm_pt_ancestor - go to parent node
> + *
> + * @cursor: current state
> + *
> + * Walk to the parent node of the current node.
> + * Returns:
> + * True if the walk was possible, false otherwise.
> + */
> +static bool amdgpu_vm_pt_ancestor(struct amdgpu_vm_pt_cursor *cursor)
> +{
> +	if (!cursor->parent)
> +		return false;
> +
> +	--cursor->level;
> +	cursor->entry = cursor->parent;
> +	cursor->parent = amdgpu_vm_pt_parent(cursor->parent);
> +	return true;
> +}
> +
> +/**
> + * amdgpu_vm_pt_next - get next PD/PT in hieratchy
> + *
> + * @adev: amdgpu_device pointer
> + * @cursor: current state
> + *
> + * Walk the PD/PT tree to the next node.
> + */
> +static void amdgpu_vm_pt_next(struct amdgpu_device *adev,
> +			      struct amdgpu_vm_pt_cursor *cursor)
> +{
> +	/* First try a newborn child */
> +	if (amdgpu_vm_pt_descendant(adev, cursor))
> +		return;
> +
> +	/* If that didn't worked try to find a sibling */
> +	while (!amdgpu_vm_pt_sibling(adev, cursor)) {
> +		/* No sibling, go to our parents and grandparents */
> +		if (!amdgpu_vm_pt_ancestor(cursor)) {
> +			cursor->pfn = ~0ll;
> +			return;
> +		}
> +	}
> +}
> +
> +/**
> + * amdgpu_vm_pt_first_leaf - get first leaf PD/PT
> + *
> + * @adev: amdgpu_device pointer
> + * @vm: amdgpu_vm structure
> + * @start: start addr of the walk
> + * @cursor: state to initialize
> + *
> + * Start a walk and go directly to the leaf node.
> + */
> +static void amdgpu_vm_pt_first_leaf(struct amdgpu_device *adev,
> +				    struct amdgpu_vm *vm, uint64_t start,
> +				    struct amdgpu_vm_pt_cursor *cursor)
> +{
> +	amdgpu_vm_pt_start(adev, vm, start, cursor);
> +	while (amdgpu_vm_pt_descendant(adev, cursor));
> +}
> +
> +/**
> + * amdgpu_vm_pt_next_leaf - get next leaf PD/PT
> + *
> + * @adev: amdgpu_device pointer
> + * @cursor: current state
> + *
> + * Walk the PD/PT tree to the next leaf node.
> + */
> +static void amdgpu_vm_pt_next_leaf(struct amdgpu_device *adev,
> +				   struct amdgpu_vm_pt_cursor *cursor)
> +{
> +	amdgpu_vm_pt_next(adev, cursor);
> +	while (amdgpu_vm_pt_descendant(adev, cursor));
> +}
> +
> +/**
> + * for_each_amdgpu_vm_pt_leaf - walk over all leaf PDs/PTs in the hierarchy
> + */
> +#define for_each_amdgpu_vm_pt_leaf(adev, vm, start, end, cursor)		\
> +	for (amdgpu_vm_pt_first_leaf((adev), (vm), (start), &(cursor));		\
> +	     (cursor).pfn <= end; amdgpu_vm_pt_next_leaf((adev), &(cursor)))
> +
> +/**
> + * amdgpu_vm_pt_first_dfs - start a deep first search
> + *
> + * @adev: amdgpu_device structure
> + * @vm: amdgpu_vm structure
> + * @cursor: state to initialize
> + *
> + * Starts a deep first traversal of the PD/PT tree.
> + */
> +static void amdgpu_vm_pt_first_dfs(struct amdgpu_device *adev,
> +				   struct amdgpu_vm *vm,
> +				   struct amdgpu_vm_pt_cursor *cursor)
> +{
> +	amdgpu_vm_pt_start(adev, vm, 0, cursor);
> +	while (amdgpu_vm_pt_descendant(adev, cursor));
> +}
> +
> +/**
> + * amdgpu_vm_pt_next_dfs - get the next node for a deep first search
> + *
> + * @adev: amdgpu_device structure
> + * @cursor: current state
> + *
> + * Move the cursor to the next node in a deep first search.
> + */
> +static void amdgpu_vm_pt_next_dfs(struct amdgpu_device *adev,
> +				  struct amdgpu_vm_pt_cursor *cursor)
> +{
> +	if (!cursor->entry)
> +		return;
> +
> +	if (!cursor->parent)
> +		cursor->entry = NULL;
> +	else if (amdgpu_vm_pt_sibling(adev, cursor))
> +		while (amdgpu_vm_pt_descendant(adev, cursor));
> +	else
> +		amdgpu_vm_pt_ancestor(cursor);
> +}
> +
> +/**
> + * for_each_amdgpu_vm_pt_dfs_safe - safe deep first search of all PDs/PTs
> + */
> +#define for_each_amdgpu_vm_pt_dfs_safe(adev, vm, cursor, entry)			\
> +	for (amdgpu_vm_pt_first_dfs((adev), (vm), &(cursor)),			\
> +	     (entry) = (cursor).entry, amdgpu_vm_pt_next_dfs((adev), &(cursor));\
> +	     (entry); (entry) = (cursor).entry,					\
> +	     amdgpu_vm_pt_next_dfs((adev), &(cursor)))
> +
>  /**
>   * amdgpu_vm_get_pd_bo - add the VM PD to a validation list
>   *
> -- 
> 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

Re: [PATCH 6/8] drm/amdgpu: meld together VM fragment and huge page handling

[Samuel Pitoiset]

Hi,

This change introduces GPU hangs with F1 2017 and Rise of Tomb Raider on 
RADV/GFX9. I bisected the issue.

Can you have a look?

Thanks!

On 9/12/18 10:54 AM, Christian König wrote:
> This optimizes the generating of PTEs by walking the hierarchy only once
> for a range and making changes as necessary.
> 
> It allows for both huge (2MB) as well giant (1GB) pages to be used on
> Vega and Raven.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
> Acked-by: Junwei Zhang <Jerry.Zhang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 267 ++++++++++++++++++---------------
>   1 file changed, 147 insertions(+), 120 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 747b6ff6fea7..328325324a1d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1488,46 +1488,76 @@ int amdgpu_vm_update_directories(struct amdgpu_device *adev,
>   }
>   
>   /**
> - * amdgpu_vm_handle_huge_pages - handle updating the PD with huge pages
> + * amdgpu_vm_update_huge - figure out parameters for PTE updates
>    *
> - * @p: see amdgpu_pte_update_params definition
> - * @entry: vm_pt entry to check
> - * @parent: parent entry
> - * @nptes: number of PTEs updated with this operation
> - * @dst: destination address where the PTEs should point to
> - * @flags: access flags fro the PTEs
> - *
> - * Check if we can update the PD with a huge page.
> + * Make sure to set the right flags for the PTEs at the desired level.
>    */
> -static void amdgpu_vm_handle_huge_pages(struct amdgpu_pte_update_params *p,
> -					struct amdgpu_vm_pt *entry,
> -					struct amdgpu_vm_pt *parent,
> -					unsigned nptes, uint64_t dst,
> -					uint64_t flags)
> -{
> -	uint64_t pde;
> +static void amdgpu_vm_update_huge(struct amdgpu_pte_update_params *params,
> +				  struct amdgpu_bo *bo, unsigned level,
> +				  uint64_t pe, uint64_t addr,
> +				  unsigned count, uint32_t incr,
> +				  uint64_t flags)
>   
> -	/* In the case of a mixed PT the PDE must point to it*/
> -	if (p->adev->asic_type >= CHIP_VEGA10 && !p->src &&
> -	    nptes == AMDGPU_VM_PTE_COUNT(p->adev)) {
> -		/* Set the huge page flag to stop scanning at this PDE */
> +{
> +	if (level != AMDGPU_VM_PTB) {
>   		flags |= AMDGPU_PDE_PTE;
> +		amdgpu_gmc_get_vm_pde(params->adev, level, &addr, &flags);
>   	}
>   
> -	if (!(flags & AMDGPU_PDE_PTE)) {
> -		if (entry->huge) {
> -			/* Add the entry to the relocated list to update it. */
> -			entry->huge = false;
> -			amdgpu_vm_bo_relocated(&entry->base);
> -		}
> +	amdgpu_vm_update_func(params, bo, pe, addr, count, incr, flags);
> +}
> +
> +/**
> + * amdgpu_vm_fragment - get fragment for PTEs
> + *
> + * @params: see amdgpu_pte_update_params definition
> + * @start: first PTE to handle
> + * @end: last PTE to handle
> + * @flags: hw mapping flags
> + * @frag: resulting fragment size
> + * @frag_end: end of this fragment
> + *
> + * Returns the first possible fragment for the start and end address.
> + */
> +static void amdgpu_vm_fragment(struct amdgpu_pte_update_params *params,
> +			       uint64_t start, uint64_t end, uint64_t flags,
> +			       unsigned int *frag, uint64_t *frag_end)
> +{
> +	/**
> +	 * The MC L1 TLB supports variable sized pages, based on a fragment
> +	 * field in the PTE. When this field is set to a non-zero value, page
> +	 * granularity is increased from 4KB to (1 << (12 + frag)). The PTE
> +	 * flags are considered valid for all PTEs within the fragment range
> +	 * and corresponding mappings are assumed to be physically contiguous.
> +	 *
> +	 * The L1 TLB can store a single PTE for the whole fragment,
> +	 * significantly increasing the space available for translation
> +	 * caching. This leads to large improvements in throughput when the
> +	 * TLB is under pressure.
> +	 *
> +	 * The L2 TLB distributes small and large fragments into two
> +	 * asymmetric partitions. The large fragment cache is significantly
> +	 * larger. Thus, we try to use large fragments wherever possible.
> +	 * Userspace can support this by aligning virtual base address and
> +	 * allocation size to the fragment size.
> +	 */
> +	unsigned max_frag = params->adev->vm_manager.fragment_size;
> +
> +	/* system pages are non continuously */
> +	if (params->src || !(flags & AMDGPU_PTE_VALID)) {
> +		*frag = 0;
> +		*frag_end = end;
>   		return;
>   	}
>   
> -	entry->huge = true;
> -	amdgpu_gmc_get_vm_pde(p->adev, AMDGPU_VM_PDB0, &dst, &flags);
> -
> -	pde = (entry - parent->entries) * 8;
> -	amdgpu_vm_update_func(p, parent->base.bo, pde, dst, 1, 0, flags);
> +	/* This intentionally wraps around if no bit is set */
> +	*frag = min((unsigned)ffs(start) - 1, (unsigned)fls64(end - start) - 1);
> +	if (*frag >= max_frag) {
> +		*frag = max_frag;
> +		*frag_end = end & ~((1ULL << max_frag) - 1);
> +	} else {
> +		*frag_end = start + (1 << *frag);
> +	}
>   }
>   
>   /**
> @@ -1545,108 +1575,105 @@ static void amdgpu_vm_handle_huge_pages(struct amdgpu_pte_update_params *p,
>    * 0 for success, -EINVAL for failure.
>    */
>   static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
> -				  uint64_t start, uint64_t end,
> -				  uint64_t dst, uint64_t flags)
> +				 uint64_t start, uint64_t end,
> +				 uint64_t dst, uint64_t flags)
>   {
>   	struct amdgpu_device *adev = params->adev;
> -	const uint64_t mask = AMDGPU_VM_PTE_COUNT(adev) - 1;
>   	struct amdgpu_vm_pt_cursor cursor;
> +	uint64_t frag_start = start, frag_end;
> +	unsigned int frag;
>   
> -	/* walk over the address space and update the page tables */
> -	for_each_amdgpu_vm_pt_leaf(adev, params->vm, start, end - 1, cursor) {
> +	/* figure out the initial fragment */
> +	amdgpu_vm_fragment(params, frag_start, end, flags, &frag, &frag_end);
> +
> +	/* walk over the address space and update the PTs */
> +	amdgpu_vm_pt_start(adev, params->vm, start, &cursor);
> +	while (cursor.pfn < end) {
>   		struct amdgpu_bo *pt = cursor.entry->base.bo;
> -		uint64_t pe_start;
> -		unsigned nptes;
> +		unsigned shift, parent_shift, num_entries;
> +		uint64_t incr, entry_end, pe_start;
>   
> -		if (!pt || cursor.level != AMDGPU_VM_PTB)
> +		if (!pt)
>   			return -ENOENT;
>   
> -		if ((cursor.pfn & ~mask) == (end & ~mask))
> -			nptes = end - cursor.pfn;
> -		else
> -			nptes = AMDGPU_VM_PTE_COUNT(adev) - (cursor.pfn & mask);
> -
> -		amdgpu_vm_handle_huge_pages(params, cursor.entry, cursor.parent,
> -					    nptes, dst, flags);
> -		/* We don't need to update PTEs for huge pages */
> -		if (cursor.entry->huge) {
> -			dst += nptes * AMDGPU_GPU_PAGE_SIZE;
> +		/* The root level can't be a huge page */
> +		if (cursor.level == adev->vm_manager.root_level) {
> +			if (!amdgpu_vm_pt_descendant(adev, &cursor))
> +				return -ENOENT;
>   			continue;
>   		}
>   
> -		pe_start = (cursor.pfn & mask) * 8;
> -		amdgpu_vm_update_func(params, pt, pe_start, dst, nptes,
> -				      AMDGPU_GPU_PAGE_SIZE, flags);
> -		dst += nptes * AMDGPU_GPU_PAGE_SIZE;
> -	}
> -
> -	return 0;
> -}
> +		/* First check if the entry is already handled */
> +		if (cursor.pfn < frag_start) {
> +			cursor.entry->huge = true;
> +			amdgpu_vm_pt_next(adev, &cursor);
> +			continue;
> +		}
>   
> -/*
> - * amdgpu_vm_frag_ptes - add fragment information to PTEs
> - *
> - * @params: see amdgpu_pte_update_params definition
> - * @vm: requested vm
> - * @start: first PTE to handle
> - * @end: last PTE to handle
> - * @dst: addr those PTEs should point to
> - * @flags: hw mapping flags
> - *
> - * Returns:
> - * 0 for success, -EINVAL for failure.
> - */
> -static int amdgpu_vm_frag_ptes(struct amdgpu_pte_update_params	*params,
> -				uint64_t start, uint64_t end,
> -				uint64_t dst, uint64_t flags)
> -{
> -	/**
> -	 * The MC L1 TLB supports variable sized pages, based on a fragment
> -	 * field in the PTE. When this field is set to a non-zero value, page
> -	 * granularity is increased from 4KB to (1 << (12 + frag)). The PTE
> -	 * flags are considered valid for all PTEs within the fragment range
> -	 * and corresponding mappings are assumed to be physically contiguous.
> -	 *
> -	 * The L1 TLB can store a single PTE for the whole fragment,
> -	 * significantly increasing the space available for translation
> -	 * caching. This leads to large improvements in throughput when the
> -	 * TLB is under pressure.
> -	 *
> -	 * The L2 TLB distributes small and large fragments into two
> -	 * asymmetric partitions. The large fragment cache is significantly
> -	 * larger. Thus, we try to use large fragments wherever possible.
> -	 * Userspace can support this by aligning virtual base address and
> -	 * allocation size to the fragment size.
> -	 */
> -	unsigned max_frag = params->adev->vm_manager.fragment_size;
> -	int r;
> +		/* If it isn't already handled it can't be a huge page */
> +		if (cursor.entry->huge) {
> +			/* Add the entry to the relocated list to update it. */
> +			cursor.entry->huge = false;
> +			amdgpu_vm_bo_relocated(&cursor.entry->base);
> +		}
>   
> -	/* system pages are non continuously */
> -	if (params->src || !(flags & AMDGPU_PTE_VALID))
> -		return amdgpu_vm_update_ptes(params, start, end, dst, flags);
> -
> -	while (start != end) {
> -		uint64_t frag_flags, frag_end;
> -		unsigned frag;
> -
> -		/* This intentionally wraps around if no bit is set */
> -		frag = min((unsigned)ffs(start) - 1,
> -			   (unsigned)fls64(end - start) - 1);
> -		if (frag >= max_frag) {
> -			frag_flags = AMDGPU_PTE_FRAG(max_frag);
> -			frag_end = end & ~((1ULL << max_frag) - 1);
> -		} else {
> -			frag_flags = AMDGPU_PTE_FRAG(frag);
> -			frag_end = start + (1 << frag);
> +		shift = amdgpu_vm_level_shift(adev, cursor.level);
> +		parent_shift = amdgpu_vm_level_shift(adev, cursor.level - 1);
> +		if (adev->asic_type < CHIP_VEGA10) {
> +			/* No huge page support before GMC v9 */
> +			if (cursor.level != AMDGPU_VM_PTB) {
> +				if (!amdgpu_vm_pt_descendant(adev, &cursor))
> +					return -ENOENT;
> +				continue;
> +			}
> +		} else if (frag < shift) {
> +			/* We can't use this level when the fragment size is
> +			 * smaller than the address shift. Go to the next
> +			 * child entry and try again.
> +			 */
> +			if (!amdgpu_vm_pt_descendant(adev, &cursor))
> +				return -ENOENT;
> +			continue;
> +		} else if (frag >= parent_shift) {
> +			/* If the fragment size is even larger than the parent
> +			 * shift we should go up one level and check it again.
> +			 */
> +			if (!amdgpu_vm_pt_ancestor(&cursor))
> +				return -ENOENT;
> +			continue;
>   		}
>   
> -		r = amdgpu_vm_update_ptes(params, start, frag_end, dst,
> -					  flags | frag_flags);
> -		if (r)
> -			return r;
> +		/* Looks good so far, calculate parameters for the update */
> +		incr = AMDGPU_GPU_PAGE_SIZE << shift;
> +		num_entries = amdgpu_vm_num_entries(adev, cursor.level);
> +		pe_start = ((cursor.pfn >> shift) & (num_entries - 1)) * 8;
> +		entry_end = num_entries << shift;
> +		entry_end += cursor.pfn & ~(entry_end - 1);
> +		entry_end = min(entry_end, end);
> +
> +		do {
> +			uint64_t upd_end = min(entry_end, frag_end);
> +			unsigned nptes = (upd_end - frag_start) >> shift;
> +
> +			amdgpu_vm_update_huge(params, pt, cursor.level,
> +					      pe_start, dst, nptes, incr,
> +					      flags | AMDGPU_PTE_FRAG(frag));
> +
> +			pe_start += nptes * 8;
> +			dst += nptes * AMDGPU_GPU_PAGE_SIZE << shift;
> +
> +			frag_start = upd_end;
> +			if (frag_start >= frag_end) {
> +				/* figure out the next fragment */
> +				amdgpu_vm_fragment(params, frag_start, end,
> +						   flags, &frag, &frag_end);
> +				if (frag < shift)
> +					break;
> +			}
> +		} while (frag_start < entry_end);
>   
> -		dst += (frag_end - start) * AMDGPU_GPU_PAGE_SIZE;
> -		start = frag_end;
> +		if (frag >= shift)
> +			amdgpu_vm_pt_next(adev, &cursor);
>   	}
>   
>   	return 0;
> @@ -1708,8 +1735,8 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
>   
>   		params.func = amdgpu_vm_cpu_set_ptes;
>   		params.pages_addr = pages_addr;
> -		return amdgpu_vm_frag_ptes(&params, start, last + 1,
> -					   addr, flags);
> +		return amdgpu_vm_update_ptes(&params, start, last + 1,
> +					     addr, flags);
>   	}
>   
>   	ring = container_of(vm->entity.rq->sched, struct amdgpu_ring, sched);
> @@ -1788,7 +1815,7 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
>   	if (r)
>   		goto error_free;
>   
> -	r = amdgpu_vm_frag_ptes(&params, start, last + 1, addr, flags);
> +	r = amdgpu_vm_update_ptes(&params, start, last + 1, addr, flags);
>   	if (r)
>   		goto error_free;
>   
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 6/8] drm/amdgpu: meld together VM fragment and huge page handling

[Christian König]

Hi Samuel,

yeah, that is a known issue and I was already looking into that all day 
today. Problem is that I can't reproduce it reliable.

Do you have any hint how to trigger that? E.g. memory exhaustion maybe?

Thanks,
Christian.

Am 08.11.18 um 17:17 schrieb Samuel Pitoiset:
> Hi,
>
> This change introduces GPU hangs with F1 2017 and Rise of Tomb Raider 
> on RADV/GFX9. I bisected the issue.
>
> Can you have a look?
>
> Thanks!
>
> On 9/12/18 10:54 AM, Christian König wrote:
>> This optimizes the generating of PTEs by walking the hierarchy only once
>> for a range and making changes as necessary.
>>
>> It allows for both huge (2MB) as well giant (1GB) pages to be used on
>> Vega and Raven.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
>> Acked-by: Junwei Zhang <Jerry.Zhang@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 267 
>> ++++++++++++++++++---------------
>>   1 file changed, 147 insertions(+), 120 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 747b6ff6fea7..328325324a1d 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -1488,46 +1488,76 @@ int amdgpu_vm_update_directories(struct 
>> amdgpu_device *adev,
>>   }
>>     /**
>> - * amdgpu_vm_handle_huge_pages - handle updating the PD with huge pages
>> + * amdgpu_vm_update_huge - figure out parameters for PTE updates
>>    *
>> - * @p: see amdgpu_pte_update_params definition
>> - * @entry: vm_pt entry to check
>> - * @parent: parent entry
>> - * @nptes: number of PTEs updated with this operation
>> - * @dst: destination address where the PTEs should point to
>> - * @flags: access flags fro the PTEs
>> - *
>> - * Check if we can update the PD with a huge page.
>> + * Make sure to set the right flags for the PTEs at the desired level.
>>    */
>> -static void amdgpu_vm_handle_huge_pages(struct 
>> amdgpu_pte_update_params *p,
>> -                    struct amdgpu_vm_pt *entry,
>> -                    struct amdgpu_vm_pt *parent,
>> -                    unsigned nptes, uint64_t dst,
>> -                    uint64_t flags)
>> -{
>> -    uint64_t pde;
>> +static void amdgpu_vm_update_huge(struct amdgpu_pte_update_params 
>> *params,
>> +                  struct amdgpu_bo *bo, unsigned level,
>> +                  uint64_t pe, uint64_t addr,
>> +                  unsigned count, uint32_t incr,
>> +                  uint64_t flags)
>>   -    /* In the case of a mixed PT the PDE must point to it*/
>> -    if (p->adev->asic_type >= CHIP_VEGA10 && !p->src &&
>> -        nptes == AMDGPU_VM_PTE_COUNT(p->adev)) {
>> -        /* Set the huge page flag to stop scanning at this PDE */
>> +{
>> +    if (level != AMDGPU_VM_PTB) {
>>           flags |= AMDGPU_PDE_PTE;
>> +        amdgpu_gmc_get_vm_pde(params->adev, level, &addr, &flags);
>>       }
>>   -    if (!(flags & AMDGPU_PDE_PTE)) {
>> -        if (entry->huge) {
>> -            /* Add the entry to the relocated list to update it. */
>> -            entry->huge = false;
>> -            amdgpu_vm_bo_relocated(&entry->base);
>> -        }
>> +    amdgpu_vm_update_func(params, bo, pe, addr, count, incr, flags);
>> +}
>> +
>> +/**
>> + * amdgpu_vm_fragment - get fragment for PTEs
>> + *
>> + * @params: see amdgpu_pte_update_params definition
>> + * @start: first PTE to handle
>> + * @end: last PTE to handle
>> + * @flags: hw mapping flags
>> + * @frag: resulting fragment size
>> + * @frag_end: end of this fragment
>> + *
>> + * Returns the first possible fragment for the start and end address.
>> + */
>> +static void amdgpu_vm_fragment(struct amdgpu_pte_update_params *params,
>> +                   uint64_t start, uint64_t end, uint64_t flags,
>> +                   unsigned int *frag, uint64_t *frag_end)
>> +{
>> +    /**
>> +     * The MC L1 TLB supports variable sized pages, based on a fragment
>> +     * field in the PTE. When this field is set to a non-zero value, 
>> page
>> +     * granularity is increased from 4KB to (1 << (12 + frag)). The PTE
>> +     * flags are considered valid for all PTEs within the fragment 
>> range
>> +     * and corresponding mappings are assumed to be physically 
>> contiguous.
>> +     *
>> +     * The L1 TLB can store a single PTE for the whole fragment,
>> +     * significantly increasing the space available for translation
>> +     * caching. This leads to large improvements in throughput when the
>> +     * TLB is under pressure.
>> +     *
>> +     * The L2 TLB distributes small and large fragments into two
>> +     * asymmetric partitions. The large fragment cache is significantly
>> +     * larger. Thus, we try to use large fragments wherever possible.
>> +     * Userspace can support this by aligning virtual base address and
>> +     * allocation size to the fragment size.
>> +     */
>> +    unsigned max_frag = params->adev->vm_manager.fragment_size;
>> +
>> +    /* system pages are non continuously */
>> +    if (params->src || !(flags & AMDGPU_PTE_VALID)) {
>> +        *frag = 0;
>> +        *frag_end = end;
>>           return;
>>       }
>>   -    entry->huge = true;
>> -    amdgpu_gmc_get_vm_pde(p->adev, AMDGPU_VM_PDB0, &dst, &flags);
>> -
>> -    pde = (entry - parent->entries) * 8;
>> -    amdgpu_vm_update_func(p, parent->base.bo, pde, dst, 1, 0, flags);
>> +    /* This intentionally wraps around if no bit is set */
>> +    *frag = min((unsigned)ffs(start) - 1, (unsigned)fls64(end - 
>> start) - 1);
>> +    if (*frag >= max_frag) {
>> +        *frag = max_frag;
>> +        *frag_end = end & ~((1ULL << max_frag) - 1);
>> +    } else {
>> +        *frag_end = start + (1 << *frag);
>> +    }
>>   }
>>     /**
>> @@ -1545,108 +1575,105 @@ static void 
>> amdgpu_vm_handle_huge_pages(struct amdgpu_pte_update_params *p,
>>    * 0 for success, -EINVAL for failure.
>>    */
>>   static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params 
>> *params,
>> -                  uint64_t start, uint64_t end,
>> -                  uint64_t dst, uint64_t flags)
>> +                 uint64_t start, uint64_t end,
>> +                 uint64_t dst, uint64_t flags)
>>   {
>>       struct amdgpu_device *adev = params->adev;
>> -    const uint64_t mask = AMDGPU_VM_PTE_COUNT(adev) - 1;
>>       struct amdgpu_vm_pt_cursor cursor;
>> +    uint64_t frag_start = start, frag_end;
>> +    unsigned int frag;
>>   -    /* walk over the address space and update the page tables */
>> -    for_each_amdgpu_vm_pt_leaf(adev, params->vm, start, end - 1, 
>> cursor) {
>> +    /* figure out the initial fragment */
>> +    amdgpu_vm_fragment(params, frag_start, end, flags, &frag, 
>> &frag_end);
>> +
>> +    /* walk over the address space and update the PTs */
>> +    amdgpu_vm_pt_start(adev, params->vm, start, &cursor);
>> +    while (cursor.pfn < end) {
>>           struct amdgpu_bo *pt = cursor.entry->base.bo;
>> -        uint64_t pe_start;
>> -        unsigned nptes;
>> +        unsigned shift, parent_shift, num_entries;
>> +        uint64_t incr, entry_end, pe_start;
>>   -        if (!pt || cursor.level != AMDGPU_VM_PTB)
>> +        if (!pt)
>>               return -ENOENT;
>>   -        if ((cursor.pfn & ~mask) == (end & ~mask))
>> -            nptes = end - cursor.pfn;
>> -        else
>> -            nptes = AMDGPU_VM_PTE_COUNT(adev) - (cursor.pfn & mask);
>> -
>> -        amdgpu_vm_handle_huge_pages(params, cursor.entry, 
>> cursor.parent,
>> -                        nptes, dst, flags);
>> -        /* We don't need to update PTEs for huge pages */
>> -        if (cursor.entry->huge) {
>> -            dst += nptes * AMDGPU_GPU_PAGE_SIZE;
>> +        /* The root level can't be a huge page */
>> +        if (cursor.level == adev->vm_manager.root_level) {
>> +            if (!amdgpu_vm_pt_descendant(adev, &cursor))
>> +                return -ENOENT;
>>               continue;
>>           }
>>   -        pe_start = (cursor.pfn & mask) * 8;
>> -        amdgpu_vm_update_func(params, pt, pe_start, dst, nptes,
>> -                      AMDGPU_GPU_PAGE_SIZE, flags);
>> -        dst += nptes * AMDGPU_GPU_PAGE_SIZE;
>> -    }
>> -
>> -    return 0;
>> -}
>> +        /* First check if the entry is already handled */
>> +        if (cursor.pfn < frag_start) {
>> +            cursor.entry->huge = true;
>> +            amdgpu_vm_pt_next(adev, &cursor);
>> +            continue;
>> +        }
>>   -/*
>> - * amdgpu_vm_frag_ptes - add fragment information to PTEs
>> - *
>> - * @params: see amdgpu_pte_update_params definition
>> - * @vm: requested vm
>> - * @start: first PTE to handle
>> - * @end: last PTE to handle
>> - * @dst: addr those PTEs should point to
>> - * @flags: hw mapping flags
>> - *
>> - * Returns:
>> - * 0 for success, -EINVAL for failure.
>> - */
>> -static int amdgpu_vm_frag_ptes(struct amdgpu_pte_update_params    
>> *params,
>> -                uint64_t start, uint64_t end,
>> -                uint64_t dst, uint64_t flags)
>> -{
>> -    /**
>> -     * The MC L1 TLB supports variable sized pages, based on a fragment
>> -     * field in the PTE. When this field is set to a non-zero value, 
>> page
>> -     * granularity is increased from 4KB to (1 << (12 + frag)). The PTE
>> -     * flags are considered valid for all PTEs within the fragment 
>> range
>> -     * and corresponding mappings are assumed to be physically 
>> contiguous.
>> -     *
>> -     * The L1 TLB can store a single PTE for the whole fragment,
>> -     * significantly increasing the space available for translation
>> -     * caching. This leads to large improvements in throughput when the
>> -     * TLB is under pressure.
>> -     *
>> -     * The L2 TLB distributes small and large fragments into two
>> -     * asymmetric partitions. The large fragment cache is significantly
>> -     * larger. Thus, we try to use large fragments wherever possible.
>> -     * Userspace can support this by aligning virtual base address and
>> -     * allocation size to the fragment size.
>> -     */
>> -    unsigned max_frag = params->adev->vm_manager.fragment_size;
>> -    int r;
>> +        /* If it isn't already handled it can't be a huge page */
>> +        if (cursor.entry->huge) {
>> +            /* Add the entry to the relocated list to update it. */
>> +            cursor.entry->huge = false;
>> +            amdgpu_vm_bo_relocated(&cursor.entry->base);
>> +        }
>>   -    /* system pages are non continuously */
>> -    if (params->src || !(flags & AMDGPU_PTE_VALID))
>> -        return amdgpu_vm_update_ptes(params, start, end, dst, flags);
>> -
>> -    while (start != end) {
>> -        uint64_t frag_flags, frag_end;
>> -        unsigned frag;
>> -
>> -        /* This intentionally wraps around if no bit is set */
>> -        frag = min((unsigned)ffs(start) - 1,
>> -               (unsigned)fls64(end - start) - 1);
>> -        if (frag >= max_frag) {
>> -            frag_flags = AMDGPU_PTE_FRAG(max_frag);
>> -            frag_end = end & ~((1ULL << max_frag) - 1);
>> -        } else {
>> -            frag_flags = AMDGPU_PTE_FRAG(frag);
>> -            frag_end = start + (1 << frag);
>> +        shift = amdgpu_vm_level_shift(adev, cursor.level);
>> +        parent_shift = amdgpu_vm_level_shift(adev, cursor.level - 1);
>> +        if (adev->asic_type < CHIP_VEGA10) {
>> +            /* No huge page support before GMC v9 */
>> +            if (cursor.level != AMDGPU_VM_PTB) {
>> +                if (!amdgpu_vm_pt_descendant(adev, &cursor))
>> +                    return -ENOENT;
>> +                continue;
>> +            }
>> +        } else if (frag < shift) {
>> +            /* We can't use this level when the fragment size is
>> +             * smaller than the address shift. Go to the next
>> +             * child entry and try again.
>> +             */
>> +            if (!amdgpu_vm_pt_descendant(adev, &cursor))
>> +                return -ENOENT;
>> +            continue;
>> +        } else if (frag >= parent_shift) {
>> +            /* If the fragment size is even larger than the parent
>> +             * shift we should go up one level and check it again.
>> +             */
>> +            if (!amdgpu_vm_pt_ancestor(&cursor))
>> +                return -ENOENT;
>> +            continue;
>>           }
>>   -        r = amdgpu_vm_update_ptes(params, start, frag_end, dst,
>> -                      flags | frag_flags);
>> -        if (r)
>> -            return r;
>> +        /* Looks good so far, calculate parameters for the update */
>> +        incr = AMDGPU_GPU_PAGE_SIZE << shift;
>> +        num_entries = amdgpu_vm_num_entries(adev, cursor.level);
>> +        pe_start = ((cursor.pfn >> shift) & (num_entries - 1)) * 8;
>> +        entry_end = num_entries << shift;
>> +        entry_end += cursor.pfn & ~(entry_end - 1);
>> +        entry_end = min(entry_end, end);
>> +
>> +        do {
>> +            uint64_t upd_end = min(entry_end, frag_end);
>> +            unsigned nptes = (upd_end - frag_start) >> shift;
>> +
>> +            amdgpu_vm_update_huge(params, pt, cursor.level,
>> +                          pe_start, dst, nptes, incr,
>> +                          flags | AMDGPU_PTE_FRAG(frag));
>> +
>> +            pe_start += nptes * 8;
>> +            dst += nptes * AMDGPU_GPU_PAGE_SIZE << shift;
>> +
>> +            frag_start = upd_end;
>> +            if (frag_start >= frag_end) {
>> +                /* figure out the next fragment */
>> +                amdgpu_vm_fragment(params, frag_start, end,
>> +                           flags, &frag, &frag_end);
>> +                if (frag < shift)
>> +                    break;
>> +            }
>> +        } while (frag_start < entry_end);
>>   -        dst += (frag_end - start) * AMDGPU_GPU_PAGE_SIZE;
>> -        start = frag_end;
>> +        if (frag >= shift)
>> +            amdgpu_vm_pt_next(adev, &cursor);
>>       }
>>         return 0;
>> @@ -1708,8 +1735,8 @@ static int amdgpu_vm_bo_update_mapping(struct 
>> amdgpu_device *adev,
>>             params.func = amdgpu_vm_cpu_set_ptes;
>>           params.pages_addr = pages_addr;
>> -        return amdgpu_vm_frag_ptes(&params, start, last + 1,
>> -                       addr, flags);
>> +        return amdgpu_vm_update_ptes(&params, start, last + 1,
>> +                         addr, flags);
>>       }
>>         ring = container_of(vm->entity.rq->sched, struct amdgpu_ring, 
>> sched);
>> @@ -1788,7 +1815,7 @@ static int amdgpu_vm_bo_update_mapping(struct 
>> amdgpu_device *adev,
>>       if (r)
>>           goto error_free;
>>   -    r = amdgpu_vm_frag_ptes(&params, start, last + 1, addr, flags);
>> +    r = amdgpu_vm_update_ptes(&params, start, last + 1, addr, flags);
>>       if (r)
>>           goto error_free;
>>

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

Re: [PATCH 6/8] drm/amdgpu: meld together VM fragment and huge page handling

[Samuel Pitoiset]

On 11/8/18 5:50 PM, Christian König wrote:
> Hi Samuel,
> 
> yeah, that is a known issue and I was already looking into that all day 
> today. Problem is that I can't reproduce it reliable.
> 
> Do you have any hint how to trigger that? E.g. memory exhaustion maybe?

Hi Christian,

Good to hear that you are already working on.

The GPU hang with Rise of Tomb Raider is reliable on my side. It always 
hangs while loading the benchmark. I use the Ultra High preset.

Do you have access to that game? As mentioned, F1 2017 is also affected 
but I used RoTR during my bisect.

Thanks.

> 
> Thanks,
> Christian.
> 
> Am 08.11.18 um 17:17 schrieb Samuel Pitoiset:
>> Hi,
>>
>> This change introduces GPU hangs with F1 2017 and Rise of Tomb Raider 
>> on RADV/GFX9. I bisected the issue.
>>
>> Can you have a look?
>>
>> Thanks!
>>
>> On 9/12/18 10:54 AM, Christian König wrote:
>>> This optimizes the generating of PTEs by walking the hierarchy only once
>>> for a range and making changes as necessary.
>>>
>>> It allows for both huge (2MB) as well giant (1GB) pages to be used on
>>> Vega and Raven.
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
>>> Acked-by: Junwei Zhang <Jerry.Zhang@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 267 
>>> ++++++++++++++++++---------------
>>>   1 file changed, 147 insertions(+), 120 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index 747b6ff6fea7..328325324a1d 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -1488,46 +1488,76 @@ int amdgpu_vm_update_directories(struct 
>>> amdgpu_device *adev,
>>>   }
>>>     /**
>>> - * amdgpu_vm_handle_huge_pages - handle updating the PD with huge pages
>>> + * amdgpu_vm_update_huge - figure out parameters for PTE updates
>>>    *
>>> - * @p: see amdgpu_pte_update_params definition
>>> - * @entry: vm_pt entry to check
>>> - * @parent: parent entry
>>> - * @nptes: number of PTEs updated with this operation
>>> - * @dst: destination address where the PTEs should point to
>>> - * @flags: access flags fro the PTEs
>>> - *
>>> - * Check if we can update the PD with a huge page.
>>> + * Make sure to set the right flags for the PTEs at the desired level.
>>>    */
>>> -static void amdgpu_vm_handle_huge_pages(struct 
>>> amdgpu_pte_update_params *p,
>>> -                    struct amdgpu_vm_pt *entry,
>>> -                    struct amdgpu_vm_pt *parent,
>>> -                    unsigned nptes, uint64_t dst,
>>> -                    uint64_t flags)
>>> -{
>>> -    uint64_t pde;
>>> +static void amdgpu_vm_update_huge(struct amdgpu_pte_update_params 
>>> *params,
>>> +                  struct amdgpu_bo *bo, unsigned level,
>>> +                  uint64_t pe, uint64_t addr,
>>> +                  unsigned count, uint32_t incr,
>>> +                  uint64_t flags)
>>>   -    /* In the case of a mixed PT the PDE must point to it*/
>>> -    if (p->adev->asic_type >= CHIP_VEGA10 && !p->src &&
>>> -        nptes == AMDGPU_VM_PTE_COUNT(p->adev)) {
>>> -        /* Set the huge page flag to stop scanning at this PDE */
>>> +{
>>> +    if (level != AMDGPU_VM_PTB) {
>>>           flags |= AMDGPU_PDE_PTE;
>>> +        amdgpu_gmc_get_vm_pde(params->adev, level, &addr, &flags);
>>>       }
>>>   -    if (!(flags & AMDGPU_PDE_PTE)) {
>>> -        if (entry->huge) {
>>> -            /* Add the entry to the relocated list to update it. */
>>> -            entry->huge = false;
>>> -            amdgpu_vm_bo_relocated(&entry->base);
>>> -        }
>>> +    amdgpu_vm_update_func(params, bo, pe, addr, count, incr, flags);
>>> +}
>>> +
>>> +/**
>>> + * amdgpu_vm_fragment - get fragment for PTEs
>>> + *
>>> + * @params: see amdgpu_pte_update_params definition
>>> + * @start: first PTE to handle
>>> + * @end: last PTE to handle
>>> + * @flags: hw mapping flags
>>> + * @frag: resulting fragment size
>>> + * @frag_end: end of this fragment
>>> + *
>>> + * Returns the first possible fragment for the start and end address.
>>> + */
>>> +static void amdgpu_vm_fragment(struct amdgpu_pte_update_params *params,
>>> +                   uint64_t start, uint64_t end, uint64_t flags,
>>> +                   unsigned int *frag, uint64_t *frag_end)
>>> +{
>>> +    /**
>>> +     * The MC L1 TLB supports variable sized pages, based on a fragment
>>> +     * field in the PTE. When this field is set to a non-zero value, 
>>> page
>>> +     * granularity is increased from 4KB to (1 << (12 + frag)). The PTE
>>> +     * flags are considered valid for all PTEs within the fragment 
>>> range
>>> +     * and corresponding mappings are assumed to be physically 
>>> contiguous.
>>> +     *
>>> +     * The L1 TLB can store a single PTE for the whole fragment,
>>> +     * significantly increasing the space available for translation
>>> +     * caching. This leads to large improvements in throughput when the
>>> +     * TLB is under pressure.
>>> +     *
>>> +     * The L2 TLB distributes small and large fragments into two
>>> +     * asymmetric partitions. The large fragment cache is significantly
>>> +     * larger. Thus, we try to use large fragments wherever possible.
>>> +     * Userspace can support this by aligning virtual base address and
>>> +     * allocation size to the fragment size.
>>> +     */
>>> +    unsigned max_frag = params->adev->vm_manager.fragment_size;
>>> +
>>> +    /* system pages are non continuously */
>>> +    if (params->src || !(flags & AMDGPU_PTE_VALID)) {
>>> +        *frag = 0;
>>> +        *frag_end = end;
>>>           return;
>>>       }
>>>   -    entry->huge = true;
>>> -    amdgpu_gmc_get_vm_pde(p->adev, AMDGPU_VM_PDB0, &dst, &flags);
>>> -
>>> -    pde = (entry - parent->entries) * 8;
>>> -    amdgpu_vm_update_func(p, parent->base.bo, pde, dst, 1, 0, flags);
>>> +    /* This intentionally wraps around if no bit is set */
>>> +    *frag = min((unsigned)ffs(start) - 1, (unsigned)fls64(end - 
>>> start) - 1);
>>> +    if (*frag >= max_frag) {
>>> +        *frag = max_frag;
>>> +        *frag_end = end & ~((1ULL << max_frag) - 1);
>>> +    } else {
>>> +        *frag_end = start + (1 << *frag);
>>> +    }
>>>   }
>>>     /**
>>> @@ -1545,108 +1575,105 @@ static void 
>>> amdgpu_vm_handle_huge_pages(struct amdgpu_pte_update_params *p,
>>>    * 0 for success, -EINVAL for failure.
>>>    */
>>>   static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params 
>>> *params,
>>> -                  uint64_t start, uint64_t end,
>>> -                  uint64_t dst, uint64_t flags)
>>> +                 uint64_t start, uint64_t end,
>>> +                 uint64_t dst, uint64_t flags)
>>>   {
>>>       struct amdgpu_device *adev = params->adev;
>>> -    const uint64_t mask = AMDGPU_VM_PTE_COUNT(adev) - 1;
>>>       struct amdgpu_vm_pt_cursor cursor;
>>> +    uint64_t frag_start = start, frag_end;
>>> +    unsigned int frag;
>>>   -    /* walk over the address space and update the page tables */
>>> -    for_each_amdgpu_vm_pt_leaf(adev, params->vm, start, end - 1, 
>>> cursor) {
>>> +    /* figure out the initial fragment */
>>> +    amdgpu_vm_fragment(params, frag_start, end, flags, &frag, 
>>> &frag_end);
>>> +
>>> +    /* walk over the address space and update the PTs */
>>> +    amdgpu_vm_pt_start(adev, params->vm, start, &cursor);
>>> +    while (cursor.pfn < end) {
>>>           struct amdgpu_bo *pt = cursor.entry->base.bo;
>>> -        uint64_t pe_start;
>>> -        unsigned nptes;
>>> +        unsigned shift, parent_shift, num_entries;
>>> +        uint64_t incr, entry_end, pe_start;
>>>   -        if (!pt || cursor.level != AMDGPU_VM_PTB)
>>> +        if (!pt)
>>>               return -ENOENT;
>>>   -        if ((cursor.pfn & ~mask) == (end & ~mask))
>>> -            nptes = end - cursor.pfn;
>>> -        else
>>> -            nptes = AMDGPU_VM_PTE_COUNT(adev) - (cursor.pfn & mask);
>>> -
>>> -        amdgpu_vm_handle_huge_pages(params, cursor.entry, 
>>> cursor.parent,
>>> -                        nptes, dst, flags);
>>> -        /* We don't need to update PTEs for huge pages */
>>> -        if (cursor.entry->huge) {
>>> -            dst += nptes * AMDGPU_GPU_PAGE_SIZE;
>>> +        /* The root level can't be a huge page */
>>> +        if (cursor.level == adev->vm_manager.root_level) {
>>> +            if (!amdgpu_vm_pt_descendant(adev, &cursor))
>>> +                return -ENOENT;
>>>               continue;
>>>           }
>>>   -        pe_start = (cursor.pfn & mask) * 8;
>>> -        amdgpu_vm_update_func(params, pt, pe_start, dst, nptes,
>>> -                      AMDGPU_GPU_PAGE_SIZE, flags);
>>> -        dst += nptes * AMDGPU_GPU_PAGE_SIZE;
>>> -    }
>>> -
>>> -    return 0;
>>> -}
>>> +        /* First check if the entry is already handled */
>>> +        if (cursor.pfn < frag_start) {
>>> +            cursor.entry->huge = true;
>>> +            amdgpu_vm_pt_next(adev, &cursor);
>>> +            continue;
>>> +        }
>>>   -/*
>>> - * amdgpu_vm_frag_ptes - add fragment information to PTEs
>>> - *
>>> - * @params: see amdgpu_pte_update_params definition
>>> - * @vm: requested vm
>>> - * @start: first PTE to handle
>>> - * @end: last PTE to handle
>>> - * @dst: addr those PTEs should point to
>>> - * @flags: hw mapping flags
>>> - *
>>> - * Returns:
>>> - * 0 for success, -EINVAL for failure.
>>> - */
>>> -static int amdgpu_vm_frag_ptes(struct amdgpu_pte_update_params *params,
>>> -                uint64_t start, uint64_t end,
>>> -                uint64_t dst, uint64_t flags)
>>> -{
>>> -    /**
>>> -     * The MC L1 TLB supports variable sized pages, based on a fragment
>>> -     * field in the PTE. When this field is set to a non-zero value, 
>>> page
>>> -     * granularity is increased from 4KB to (1 << (12 + frag)). The PTE
>>> -     * flags are considered valid for all PTEs within the fragment 
>>> range
>>> -     * and corresponding mappings are assumed to be physically 
>>> contiguous.
>>> -     *
>>> -     * The L1 TLB can store a single PTE for the whole fragment,
>>> -     * significantly increasing the space available for translation
>>> -     * caching. This leads to large improvements in throughput when the
>>> -     * TLB is under pressure.
>>> -     *
>>> -     * The L2 TLB distributes small and large fragments into two
>>> -     * asymmetric partitions. The large fragment cache is significantly
>>> -     * larger. Thus, we try to use large fragments wherever possible.
>>> -     * Userspace can support this by aligning virtual base address and
>>> -     * allocation size to the fragment size.
>>> -     */
>>> -    unsigned max_frag = params->adev->vm_manager.fragment_size;
>>> -    int r;
>>> +        /* If it isn't already handled it can't be a huge page */
>>> +        if (cursor.entry->huge) {
>>> +            /* Add the entry to the relocated list to update it. */
>>> +            cursor.entry->huge = false;
>>> +            amdgpu_vm_bo_relocated(&cursor.entry->base);
>>> +        }
>>>   -    /* system pages are non continuously */
>>> -    if (params->src || !(flags & AMDGPU_PTE_VALID))
>>> -        return amdgpu_vm_update_ptes(params, start, end, dst, flags);
>>> -
>>> -    while (start != end) {
>>> -        uint64_t frag_flags, frag_end;
>>> -        unsigned frag;
>>> -
>>> -        /* This intentionally wraps around if no bit is set */
>>> -        frag = min((unsigned)ffs(start) - 1,
>>> -               (unsigned)fls64(end - start) - 1);
>>> -        if (frag >= max_frag) {
>>> -            frag_flags = AMDGPU_PTE_FRAG(max_frag);
>>> -            frag_end = end & ~((1ULL << max_frag) - 1);
>>> -        } else {
>>> -            frag_flags = AMDGPU_PTE_FRAG(frag);
>>> -            frag_end = start + (1 << frag);
>>> +        shift = amdgpu_vm_level_shift(adev, cursor.level);
>>> +        parent_shift = amdgpu_vm_level_shift(adev, cursor.level - 1);
>>> +        if (adev->asic_type < CHIP_VEGA10) {
>>> +            /* No huge page support before GMC v9 */
>>> +            if (cursor.level != AMDGPU_VM_PTB) {
>>> +                if (!amdgpu_vm_pt_descendant(adev, &cursor))
>>> +                    return -ENOENT;
>>> +                continue;
>>> +            }
>>> +        } else if (frag < shift) {
>>> +            /* We can't use this level when the fragment size is
>>> +             * smaller than the address shift. Go to the next
>>> +             * child entry and try again.
>>> +             */
>>> +            if (!amdgpu_vm_pt_descendant(adev, &cursor))
>>> +                return -ENOENT;
>>> +            continue;
>>> +        } else if (frag >= parent_shift) {
>>> +            /* If the fragment size is even larger than the parent
>>> +             * shift we should go up one level and check it again.
>>> +             */
>>> +            if (!amdgpu_vm_pt_ancestor(&cursor))
>>> +                return -ENOENT;
>>> +            continue;
>>>           }
>>>   -        r = amdgpu_vm_update_ptes(params, start, frag_end, dst,
>>> -                      flags | frag_flags);
>>> -        if (r)
>>> -            return r;
>>> +        /* Looks good so far, calculate parameters for the update */
>>> +        incr = AMDGPU_GPU_PAGE_SIZE << shift;
>>> +        num_entries = amdgpu_vm_num_entries(adev, cursor.level);
>>> +        pe_start = ((cursor.pfn >> shift) & (num_entries - 1)) * 8;
>>> +        entry_end = num_entries << shift;
>>> +        entry_end += cursor.pfn & ~(entry_end - 1);
>>> +        entry_end = min(entry_end, end);
>>> +
>>> +        do {
>>> +            uint64_t upd_end = min(entry_end, frag_end);
>>> +            unsigned nptes = (upd_end - frag_start) >> shift;
>>> +
>>> +            amdgpu_vm_update_huge(params, pt, cursor.level,
>>> +                          pe_start, dst, nptes, incr,
>>> +                          flags | AMDGPU_PTE_FRAG(frag));
>>> +
>>> +            pe_start += nptes * 8;
>>> +            dst += nptes * AMDGPU_GPU_PAGE_SIZE << shift;
>>> +
>>> +            frag_start = upd_end;
>>> +            if (frag_start >= frag_end) {
>>> +                /* figure out the next fragment */
>>> +                amdgpu_vm_fragment(params, frag_start, end,
>>> +                           flags, &frag, &frag_end);
>>> +                if (frag < shift)
>>> +                    break;
>>> +            }
>>> +        } while (frag_start < entry_end);
>>>   -        dst += (frag_end - start) * AMDGPU_GPU_PAGE_SIZE;
>>> -        start = frag_end;
>>> +        if (frag >= shift)
>>> +            amdgpu_vm_pt_next(adev, &cursor);
>>>       }
>>>         return 0;
>>> @@ -1708,8 +1735,8 @@ static int amdgpu_vm_bo_update_mapping(struct 
>>> amdgpu_device *adev,
>>>             params.func = amdgpu_vm_cpu_set_ptes;
>>>           params.pages_addr = pages_addr;
>>> -        return amdgpu_vm_frag_ptes(&params, start, last + 1,
>>> -                       addr, flags);
>>> +        return amdgpu_vm_update_ptes(&params, start, last + 1,
>>> +                         addr, flags);
>>>       }
>>>         ring = container_of(vm->entity.rq->sched, struct amdgpu_ring, 
>>> sched);
>>> @@ -1788,7 +1815,7 @@ static int amdgpu_vm_bo_update_mapping(struct 
>>> amdgpu_device *adev,
>>>       if (r)
>>>           goto error_free;
>>>   -    r = amdgpu_vm_frag_ptes(&params, start, last + 1, addr, flags);
>>> +    r = amdgpu_vm_update_ptes(&params, start, last + 1, addr, flags);
>>>       if (r)
>>>           goto error_free;
>>>
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 6/8] drm/amdgpu: meld together VM fragment and huge page handling

[Koenig, Christian]

Adding Aaron who initially reported the problem with this patch and 
suspend/resume.

Am 08.11.18 um 20:35 schrieb Samuel Pitoiset:
> On 11/8/18 5:50 PM, Christian König wrote:
>> Hi Samuel,
>>
>> yeah, that is a known issue and I was already looking into that all 
>> day today. Problem is that I can't reproduce it reliable.
>>
>> Do you have any hint how to trigger that? E.g. memory exhaustion maybe?
>
> Hi Christian,
>
> Good to hear that you are already working on.
>
> The GPU hang with Rise of Tomb Raider is reliable on my side. It 
> always hangs while loading the benchmark. I use the Ultra High preset.
>
> Do you have access to that game? As mentioned, F1 2017 is also 
> affected but I used RoTR during my bisect.

I was able to reproduce the problem with a memory stress test, but still 
have not the slightest idea what is going wrong here.

Going to investigate further,
Christian.

>
> Thanks.
>
>>
>> Thanks,
>> Christian.
>>
>> Am 08.11.18 um 17:17 schrieb Samuel Pitoiset:
>>> Hi,
>>>
>>> This change introduces GPU hangs with F1 2017 and Rise of Tomb 
>>> Raider on RADV/GFX9. I bisected the issue.
>>>
>>> Can you have a look?
>>>
>>> Thanks!
>>>
>>> On 9/12/18 10:54 AM, Christian König wrote:
>>>> This optimizes the generating of PTEs by walking the hierarchy only 
>>>> once
>>>> for a range and making changes as necessary.
>>>>
>>>> It allows for both huge (2MB) as well giant (1GB) pages to be used on
>>>> Vega and Raven.
>>>>
>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
>>>> Acked-by: Junwei Zhang <Jerry.Zhang@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 267 
>>>> ++++++++++++++++++---------------
>>>>   1 file changed, 147 insertions(+), 120 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> index 747b6ff6fea7..328325324a1d 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> @@ -1488,46 +1488,76 @@ int amdgpu_vm_update_directories(struct 
>>>> amdgpu_device *adev,
>>>>   }
>>>>     /**
>>>> - * amdgpu_vm_handle_huge_pages - handle updating the PD with huge 
>>>> pages
>>>> + * amdgpu_vm_update_huge - figure out parameters for PTE updates
>>>>    *
>>>> - * @p: see amdgpu_pte_update_params definition
>>>> - * @entry: vm_pt entry to check
>>>> - * @parent: parent entry
>>>> - * @nptes: number of PTEs updated with this operation
>>>> - * @dst: destination address where the PTEs should point to
>>>> - * @flags: access flags fro the PTEs
>>>> - *
>>>> - * Check if we can update the PD with a huge page.
>>>> + * Make sure to set the right flags for the PTEs at the desired 
>>>> level.
>>>>    */
>>>> -static void amdgpu_vm_handle_huge_pages(struct 
>>>> amdgpu_pte_update_params *p,
>>>> -                    struct amdgpu_vm_pt *entry,
>>>> -                    struct amdgpu_vm_pt *parent,
>>>> -                    unsigned nptes, uint64_t dst,
>>>> -                    uint64_t flags)
>>>> -{
>>>> -    uint64_t pde;
>>>> +static void amdgpu_vm_update_huge(struct amdgpu_pte_update_params 
>>>> *params,
>>>> +                  struct amdgpu_bo *bo, unsigned level,
>>>> +                  uint64_t pe, uint64_t addr,
>>>> +                  unsigned count, uint32_t incr,
>>>> +                  uint64_t flags)
>>>>   -    /* In the case of a mixed PT the PDE must point to it*/
>>>> -    if (p->adev->asic_type >= CHIP_VEGA10 && !p->src &&
>>>> -        nptes == AMDGPU_VM_PTE_COUNT(p->adev)) {
>>>> -        /* Set the huge page flag to stop scanning at this PDE */
>>>> +{
>>>> +    if (level != AMDGPU_VM_PTB) {
>>>>           flags |= AMDGPU_PDE_PTE;
>>>> +        amdgpu_gmc_get_vm_pde(params->adev, level, &addr, &flags);
>>>>       }
>>>>   -    if (!(flags & AMDGPU_PDE_PTE)) {
>>>> -        if (entry->huge) {
>>>> -            /* Add the entry to the relocated list to update it. */
>>>> -            entry->huge = false;
>>>> -            amdgpu_vm_bo_relocated(&entry->base);
>>>> -        }
>>>> +    amdgpu_vm_update_func(params, bo, pe, addr, count, incr, flags);
>>>> +}
>>>> +
>>>> +/**
>>>> + * amdgpu_vm_fragment - get fragment for PTEs
>>>> + *
>>>> + * @params: see amdgpu_pte_update_params definition
>>>> + * @start: first PTE to handle
>>>> + * @end: last PTE to handle
>>>> + * @flags: hw mapping flags
>>>> + * @frag: resulting fragment size
>>>> + * @frag_end: end of this fragment
>>>> + *
>>>> + * Returns the first possible fragment for the start and end address.
>>>> + */
>>>> +static void amdgpu_vm_fragment(struct amdgpu_pte_update_params 
>>>> *params,
>>>> +                   uint64_t start, uint64_t end, uint64_t flags,
>>>> +                   unsigned int *frag, uint64_t *frag_end)
>>>> +{
>>>> +    /**
>>>> +     * The MC L1 TLB supports variable sized pages, based on a 
>>>> fragment
>>>> +     * field in the PTE. When this field is set to a non-zero 
>>>> value, page
>>>> +     * granularity is increased from 4KB to (1 << (12 + frag)). 
>>>> The PTE
>>>> +     * flags are considered valid for all PTEs within the fragment 
>>>> range
>>>> +     * and corresponding mappings are assumed to be physically 
>>>> contiguous.
>>>> +     *
>>>> +     * The L1 TLB can store a single PTE for the whole fragment,
>>>> +     * significantly increasing the space available for translation
>>>> +     * caching. This leads to large improvements in throughput 
>>>> when the
>>>> +     * TLB is under pressure.
>>>> +     *
>>>> +     * The L2 TLB distributes small and large fragments into two
>>>> +     * asymmetric partitions. The large fragment cache is 
>>>> significantly
>>>> +     * larger. Thus, we try to use large fragments wherever possible.
>>>> +     * Userspace can support this by aligning virtual base address 
>>>> and
>>>> +     * allocation size to the fragment size.
>>>> +     */
>>>> +    unsigned max_frag = params->adev->vm_manager.fragment_size;
>>>> +
>>>> +    /* system pages are non continuously */
>>>> +    if (params->src || !(flags & AMDGPU_PTE_VALID)) {
>>>> +        *frag = 0;
>>>> +        *frag_end = end;
>>>>           return;
>>>>       }
>>>>   -    entry->huge = true;
>>>> -    amdgpu_gmc_get_vm_pde(p->adev, AMDGPU_VM_PDB0, &dst, &flags);
>>>> -
>>>> -    pde = (entry - parent->entries) * 8;
>>>> -    amdgpu_vm_update_func(p, parent->base.bo, pde, dst, 1, 0, flags);
>>>> +    /* This intentionally wraps around if no bit is set */
>>>> +    *frag = min((unsigned)ffs(start) - 1, (unsigned)fls64(end - 
>>>> start) - 1);
>>>> +    if (*frag >= max_frag) {
>>>> +        *frag = max_frag;
>>>> +        *frag_end = end & ~((1ULL << max_frag) - 1);
>>>> +    } else {
>>>> +        *frag_end = start + (1 << *frag);
>>>> +    }
>>>>   }
>>>>     /**
>>>> @@ -1545,108 +1575,105 @@ static void 
>>>> amdgpu_vm_handle_huge_pages(struct amdgpu_pte_update_params *p,
>>>>    * 0 for success, -EINVAL for failure.
>>>>    */
>>>>   static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params 
>>>> *params,
>>>> -                  uint64_t start, uint64_t end,
>>>> -                  uint64_t dst, uint64_t flags)
>>>> +                 uint64_t start, uint64_t end,
>>>> +                 uint64_t dst, uint64_t flags)
>>>>   {
>>>>       struct amdgpu_device *adev = params->adev;
>>>> -    const uint64_t mask = AMDGPU_VM_PTE_COUNT(adev) - 1;
>>>>       struct amdgpu_vm_pt_cursor cursor;
>>>> +    uint64_t frag_start = start, frag_end;
>>>> +    unsigned int frag;
>>>>   -    /* walk over the address space and update the page tables */
>>>> -    for_each_amdgpu_vm_pt_leaf(adev, params->vm, start, end - 1, 
>>>> cursor) {
>>>> +    /* figure out the initial fragment */
>>>> +    amdgpu_vm_fragment(params, frag_start, end, flags, &frag, 
>>>> &frag_end);
>>>> +
>>>> +    /* walk over the address space and update the PTs */
>>>> +    amdgpu_vm_pt_start(adev, params->vm, start, &cursor);
>>>> +    while (cursor.pfn < end) {
>>>>           struct amdgpu_bo *pt = cursor.entry->base.bo;
>>>> -        uint64_t pe_start;
>>>> -        unsigned nptes;
>>>> +        unsigned shift, parent_shift, num_entries;
>>>> +        uint64_t incr, entry_end, pe_start;
>>>>   -        if (!pt || cursor.level != AMDGPU_VM_PTB)
>>>> +        if (!pt)
>>>>               return -ENOENT;
>>>>   -        if ((cursor.pfn & ~mask) == (end & ~mask))
>>>> -            nptes = end - cursor.pfn;
>>>> -        else
>>>> -            nptes = AMDGPU_VM_PTE_COUNT(adev) - (cursor.pfn & mask);
>>>> -
>>>> -        amdgpu_vm_handle_huge_pages(params, cursor.entry, 
>>>> cursor.parent,
>>>> -                        nptes, dst, flags);
>>>> -        /* We don't need to update PTEs for huge pages */
>>>> -        if (cursor.entry->huge) {
>>>> -            dst += nptes * AMDGPU_GPU_PAGE_SIZE;
>>>> +        /* The root level can't be a huge page */
>>>> +        if (cursor.level == adev->vm_manager.root_level) {
>>>> +            if (!amdgpu_vm_pt_descendant(adev, &cursor))
>>>> +                return -ENOENT;
>>>>               continue;
>>>>           }
>>>>   -        pe_start = (cursor.pfn & mask) * 8;
>>>> -        amdgpu_vm_update_func(params, pt, pe_start, dst, nptes,
>>>> -                      AMDGPU_GPU_PAGE_SIZE, flags);
>>>> -        dst += nptes * AMDGPU_GPU_PAGE_SIZE;
>>>> -    }
>>>> -
>>>> -    return 0;
>>>> -}
>>>> +        /* First check if the entry is already handled */
>>>> +        if (cursor.pfn < frag_start) {
>>>> +            cursor.entry->huge = true;
>>>> +            amdgpu_vm_pt_next(adev, &cursor);
>>>> +            continue;
>>>> +        }
>>>>   -/*
>>>> - * amdgpu_vm_frag_ptes - add fragment information to PTEs
>>>> - *
>>>> - * @params: see amdgpu_pte_update_params definition
>>>> - * @vm: requested vm
>>>> - * @start: first PTE to handle
>>>> - * @end: last PTE to handle
>>>> - * @dst: addr those PTEs should point to
>>>> - * @flags: hw mapping flags
>>>> - *
>>>> - * Returns:
>>>> - * 0 for success, -EINVAL for failure.
>>>> - */
>>>> -static int amdgpu_vm_frag_ptes(struct amdgpu_pte_update_params 
>>>> *params,
>>>> -                uint64_t start, uint64_t end,
>>>> -                uint64_t dst, uint64_t flags)
>>>> -{
>>>> -    /**
>>>> -     * The MC L1 TLB supports variable sized pages, based on a 
>>>> fragment
>>>> -     * field in the PTE. When this field is set to a non-zero 
>>>> value, page
>>>> -     * granularity is increased from 4KB to (1 << (12 + frag)). 
>>>> The PTE
>>>> -     * flags are considered valid for all PTEs within the fragment 
>>>> range
>>>> -     * and corresponding mappings are assumed to be physically 
>>>> contiguous.
>>>> -     *
>>>> -     * The L1 TLB can store a single PTE for the whole fragment,
>>>> -     * significantly increasing the space available for translation
>>>> -     * caching. This leads to large improvements in throughput 
>>>> when the
>>>> -     * TLB is under pressure.
>>>> -     *
>>>> -     * The L2 TLB distributes small and large fragments into two
>>>> -     * asymmetric partitions. The large fragment cache is 
>>>> significantly
>>>> -     * larger. Thus, we try to use large fragments wherever possible.
>>>> -     * Userspace can support this by aligning virtual base address 
>>>> and
>>>> -     * allocation size to the fragment size.
>>>> -     */
>>>> -    unsigned max_frag = params->adev->vm_manager.fragment_size;
>>>> -    int r;
>>>> +        /* If it isn't already handled it can't be a huge page */
>>>> +        if (cursor.entry->huge) {
>>>> +            /* Add the entry to the relocated list to update it. */
>>>> +            cursor.entry->huge = false;
>>>> + amdgpu_vm_bo_relocated(&cursor.entry->base);
>>>> +        }
>>>>   -    /* system pages are non continuously */
>>>> -    if (params->src || !(flags & AMDGPU_PTE_VALID))
>>>> -        return amdgpu_vm_update_ptes(params, start, end, dst, flags);
>>>> -
>>>> -    while (start != end) {
>>>> -        uint64_t frag_flags, frag_end;
>>>> -        unsigned frag;
>>>> -
>>>> -        /* This intentionally wraps around if no bit is set */
>>>> -        frag = min((unsigned)ffs(start) - 1,
>>>> -               (unsigned)fls64(end - start) - 1);
>>>> -        if (frag >= max_frag) {
>>>> -            frag_flags = AMDGPU_PTE_FRAG(max_frag);
>>>> -            frag_end = end & ~((1ULL << max_frag) - 1);
>>>> -        } else {
>>>> -            frag_flags = AMDGPU_PTE_FRAG(frag);
>>>> -            frag_end = start + (1 << frag);
>>>> +        shift = amdgpu_vm_level_shift(adev, cursor.level);
>>>> +        parent_shift = amdgpu_vm_level_shift(adev, cursor.level - 1);
>>>> +        if (adev->asic_type < CHIP_VEGA10) {
>>>> +            /* No huge page support before GMC v9 */
>>>> +            if (cursor.level != AMDGPU_VM_PTB) {
>>>> +                if (!amdgpu_vm_pt_descendant(adev, &cursor))
>>>> +                    return -ENOENT;
>>>> +                continue;
>>>> +            }
>>>> +        } else if (frag < shift) {
>>>> +            /* We can't use this level when the fragment size is
>>>> +             * smaller than the address shift. Go to the next
>>>> +             * child entry and try again.
>>>> +             */
>>>> +            if (!amdgpu_vm_pt_descendant(adev, &cursor))
>>>> +                return -ENOENT;
>>>> +            continue;
>>>> +        } else if (frag >= parent_shift) {
>>>> +            /* If the fragment size is even larger than the parent
>>>> +             * shift we should go up one level and check it again.
>>>> +             */
>>>> +            if (!amdgpu_vm_pt_ancestor(&cursor))
>>>> +                return -ENOENT;
>>>> +            continue;
>>>>           }
>>>>   -        r = amdgpu_vm_update_ptes(params, start, frag_end, dst,
>>>> -                      flags | frag_flags);
>>>> -        if (r)
>>>> -            return r;
>>>> +        /* Looks good so far, calculate parameters for the update */
>>>> +        incr = AMDGPU_GPU_PAGE_SIZE << shift;
>>>> +        num_entries = amdgpu_vm_num_entries(adev, cursor.level);
>>>> +        pe_start = ((cursor.pfn >> shift) & (num_entries - 1)) * 8;
>>>> +        entry_end = num_entries << shift;
>>>> +        entry_end += cursor.pfn & ~(entry_end - 1);
>>>> +        entry_end = min(entry_end, end);
>>>> +
>>>> +        do {
>>>> +            uint64_t upd_end = min(entry_end, frag_end);
>>>> +            unsigned nptes = (upd_end - frag_start) >> shift;
>>>> +
>>>> +            amdgpu_vm_update_huge(params, pt, cursor.level,
>>>> +                          pe_start, dst, nptes, incr,
>>>> +                          flags | AMDGPU_PTE_FRAG(frag));
>>>> +
>>>> +            pe_start += nptes * 8;
>>>> +            dst += nptes * AMDGPU_GPU_PAGE_SIZE << shift;
>>>> +
>>>> +            frag_start = upd_end;
>>>> +            if (frag_start >= frag_end) {
>>>> +                /* figure out the next fragment */
>>>> +                amdgpu_vm_fragment(params, frag_start, end,
>>>> +                           flags, &frag, &frag_end);
>>>> +                if (frag < shift)
>>>> +                    break;
>>>> +            }
>>>> +        } while (frag_start < entry_end);
>>>>   -        dst += (frag_end - start) * AMDGPU_GPU_PAGE_SIZE;
>>>> -        start = frag_end;
>>>> +        if (frag >= shift)
>>>> +            amdgpu_vm_pt_next(adev, &cursor);
>>>>       }
>>>>         return 0;
>>>> @@ -1708,8 +1735,8 @@ static int amdgpu_vm_bo_update_mapping(struct 
>>>> amdgpu_device *adev,
>>>>             params.func = amdgpu_vm_cpu_set_ptes;
>>>>           params.pages_addr = pages_addr;
>>>> -        return amdgpu_vm_frag_ptes(&params, start, last + 1,
>>>> -                       addr, flags);
>>>> +        return amdgpu_vm_update_ptes(&params, start, last + 1,
>>>> +                         addr, flags);
>>>>       }
>>>>         ring = container_of(vm->entity.rq->sched, struct 
>>>> amdgpu_ring, sched);
>>>> @@ -1788,7 +1815,7 @@ static int amdgpu_vm_bo_update_mapping(struct 
>>>> amdgpu_device *adev,
>>>>       if (r)
>>>>           goto error_free;
>>>>   -    r = amdgpu_vm_frag_ptes(&params, start, last + 1, addr, flags);
>>>> +    r = amdgpu_vm_update_ptes(&params, start, last + 1, addr, flags);
>>>>       if (r)
>>>>           goto error_free;
>>>>
>>

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

Re: [PATCH 6/8] drm/amdgpu: meld together VM fragment and huge page handling

[Christian König]

Am 09.11.18 um 13:13 schrieb Koenig, Christian:
> Adding Aaron who initially reported the problem with this patch and
> suspend/resume.
>
> Am 08.11.18 um 20:35 schrieb Samuel Pitoiset:
>> On 11/8/18 5:50 PM, Christian König wrote:
>>> Hi Samuel,
>>>
>>> yeah, that is a known issue and I was already looking into that all
>>> day today. Problem is that I can't reproduce it reliable.
>>>
>>> Do you have any hint how to trigger that? E.g. memory exhaustion maybe?
>> Hi Christian,
>>
>> Good to hear that you are already working on.
>>
>> The GPU hang with Rise of Tomb Raider is reliable on my side. It
>> always hangs while loading the benchmark. I use the Ultra High preset.
>>
>> Do you have access to that game? As mentioned, F1 2017 is also
>> affected but I used RoTR during my bisect.
> I was able to reproduce the problem with a memory stress test, but still
> have not the slightest idea what is going wrong here.

Ok, found the root cause. We are setting the huge page flag on the wrong 
level (at the parent instead of the child).

Currently testing a fix for this,
Christian.

>
> Going to investigate further,
> Christian.
>
>> Thanks.
>>
>>> Thanks,
>>> Christian.
>>>
>>> Am 08.11.18 um 17:17 schrieb Samuel Pitoiset:
>>>> Hi,
>>>>
>>>> This change introduces GPU hangs with F1 2017 and Rise of Tomb
>>>> Raider on RADV/GFX9. I bisected the issue.
>>>>
>>>> Can you have a look?
>>>>
>>>> Thanks!
>>>>
>>>> On 9/12/18 10:54 AM, Christian König wrote:
>>>>> This optimizes the generating of PTEs by walking the hierarchy only
>>>>> once
>>>>> for a range and making changes as necessary.
>>>>>
>>>>> It allows for both huge (2MB) as well giant (1GB) pages to be used on
>>>>> Vega and Raven.
>>>>>
>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
>>>>> Acked-by: Junwei Zhang <Jerry.Zhang@amd.com>
>>>>> ---
>>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 267
>>>>> ++++++++++++++++++---------------
>>>>>    1 file changed, 147 insertions(+), 120 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> index 747b6ff6fea7..328325324a1d 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> @@ -1488,46 +1488,76 @@ int amdgpu_vm_update_directories(struct
>>>>> amdgpu_device *adev,
>>>>>    }
>>>>>      /**
>>>>> - * amdgpu_vm_handle_huge_pages - handle updating the PD with huge
>>>>> pages
>>>>> + * amdgpu_vm_update_huge - figure out parameters for PTE updates
>>>>>     *
>>>>> - * @p: see amdgpu_pte_update_params definition
>>>>> - * @entry: vm_pt entry to check
>>>>> - * @parent: parent entry
>>>>> - * @nptes: number of PTEs updated with this operation
>>>>> - * @dst: destination address where the PTEs should point to
>>>>> - * @flags: access flags fro the PTEs
>>>>> - *
>>>>> - * Check if we can update the PD with a huge page.
>>>>> + * Make sure to set the right flags for the PTEs at the desired
>>>>> level.
>>>>>     */
>>>>> -static void amdgpu_vm_handle_huge_pages(struct
>>>>> amdgpu_pte_update_params *p,
>>>>> -                    struct amdgpu_vm_pt *entry,
>>>>> -                    struct amdgpu_vm_pt *parent,
>>>>> -                    unsigned nptes, uint64_t dst,
>>>>> -                    uint64_t flags)
>>>>> -{
>>>>> -    uint64_t pde;
>>>>> +static void amdgpu_vm_update_huge(struct amdgpu_pte_update_params
>>>>> *params,
>>>>> +                  struct amdgpu_bo *bo, unsigned level,
>>>>> +                  uint64_t pe, uint64_t addr,
>>>>> +                  unsigned count, uint32_t incr,
>>>>> +                  uint64_t flags)
>>>>>    -    /* In the case of a mixed PT the PDE must point to it*/
>>>>> -    if (p->adev->asic_type >= CHIP_VEGA10 && !p->src &&
>>>>> -        nptes == AMDGPU_VM_PTE_COUNT(p->adev)) {
>>>>> -        /* Set the huge page flag to stop scanning at this PDE */
>>>>> +{
>>>>> +    if (level != AMDGPU_VM_PTB) {
>>>>>            flags |= AMDGPU_PDE_PTE;
>>>>> +        amdgpu_gmc_get_vm_pde(params->adev, level, &addr, &flags);
>>>>>        }
>>>>>    -    if (!(flags & AMDGPU_PDE_PTE)) {
>>>>> -        if (entry->huge) {
>>>>> -            /* Add the entry to the relocated list to update it. */
>>>>> -            entry->huge = false;
>>>>> -            amdgpu_vm_bo_relocated(&entry->base);
>>>>> -        }
>>>>> +    amdgpu_vm_update_func(params, bo, pe, addr, count, incr, flags);
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * amdgpu_vm_fragment - get fragment for PTEs
>>>>> + *
>>>>> + * @params: see amdgpu_pte_update_params definition
>>>>> + * @start: first PTE to handle
>>>>> + * @end: last PTE to handle
>>>>> + * @flags: hw mapping flags
>>>>> + * @frag: resulting fragment size
>>>>> + * @frag_end: end of this fragment
>>>>> + *
>>>>> + * Returns the first possible fragment for the start and end address.
>>>>> + */
>>>>> +static void amdgpu_vm_fragment(struct amdgpu_pte_update_params
>>>>> *params,
>>>>> +                   uint64_t start, uint64_t end, uint64_t flags,
>>>>> +                   unsigned int *frag, uint64_t *frag_end)
>>>>> +{
>>>>> +    /**
>>>>> +     * The MC L1 TLB supports variable sized pages, based on a
>>>>> fragment
>>>>> +     * field in the PTE. When this field is set to a non-zero
>>>>> value, page
>>>>> +     * granularity is increased from 4KB to (1 << (12 + frag)).
>>>>> The PTE
>>>>> +     * flags are considered valid for all PTEs within the fragment
>>>>> range
>>>>> +     * and corresponding mappings are assumed to be physically
>>>>> contiguous.
>>>>> +     *
>>>>> +     * The L1 TLB can store a single PTE for the whole fragment,
>>>>> +     * significantly increasing the space available for translation
>>>>> +     * caching. This leads to large improvements in throughput
>>>>> when the
>>>>> +     * TLB is under pressure.
>>>>> +     *
>>>>> +     * The L2 TLB distributes small and large fragments into two
>>>>> +     * asymmetric partitions. The large fragment cache is
>>>>> significantly
>>>>> +     * larger. Thus, we try to use large fragments wherever possible.
>>>>> +     * Userspace can support this by aligning virtual base address
>>>>> and
>>>>> +     * allocation size to the fragment size.
>>>>> +     */
>>>>> +    unsigned max_frag = params->adev->vm_manager.fragment_size;
>>>>> +
>>>>> +    /* system pages are non continuously */
>>>>> +    if (params->src || !(flags & AMDGPU_PTE_VALID)) {
>>>>> +        *frag = 0;
>>>>> +        *frag_end = end;
>>>>>            return;
>>>>>        }
>>>>>    -    entry->huge = true;
>>>>> -    amdgpu_gmc_get_vm_pde(p->adev, AMDGPU_VM_PDB0, &dst, &flags);
>>>>> -
>>>>> -    pde = (entry - parent->entries) * 8;
>>>>> -    amdgpu_vm_update_func(p, parent->base.bo, pde, dst, 1, 0, flags);
>>>>> +    /* This intentionally wraps around if no bit is set */
>>>>> +    *frag = min((unsigned)ffs(start) - 1, (unsigned)fls64(end -
>>>>> start) - 1);
>>>>> +    if (*frag >= max_frag) {
>>>>> +        *frag = max_frag;
>>>>> +        *frag_end = end & ~((1ULL << max_frag) - 1);
>>>>> +    } else {
>>>>> +        *frag_end = start + (1 << *frag);
>>>>> +    }
>>>>>    }
>>>>>      /**
>>>>> @@ -1545,108 +1575,105 @@ static void
>>>>> amdgpu_vm_handle_huge_pages(struct amdgpu_pte_update_params *p,
>>>>>     * 0 for success, -EINVAL for failure.
>>>>>     */
>>>>>    static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params
>>>>> *params,
>>>>> -                  uint64_t start, uint64_t end,
>>>>> -                  uint64_t dst, uint64_t flags)
>>>>> +                 uint64_t start, uint64_t end,
>>>>> +                 uint64_t dst, uint64_t flags)
>>>>>    {
>>>>>        struct amdgpu_device *adev = params->adev;
>>>>> -    const uint64_t mask = AMDGPU_VM_PTE_COUNT(adev) - 1;
>>>>>        struct amdgpu_vm_pt_cursor cursor;
>>>>> +    uint64_t frag_start = start, frag_end;
>>>>> +    unsigned int frag;
>>>>>    -    /* walk over the address space and update the page tables */
>>>>> -    for_each_amdgpu_vm_pt_leaf(adev, params->vm, start, end - 1,
>>>>> cursor) {
>>>>> +    /* figure out the initial fragment */
>>>>> +    amdgpu_vm_fragment(params, frag_start, end, flags, &frag,
>>>>> &frag_end);
>>>>> +
>>>>> +    /* walk over the address space and update the PTs */
>>>>> +    amdgpu_vm_pt_start(adev, params->vm, start, &cursor);
>>>>> +    while (cursor.pfn < end) {
>>>>>            struct amdgpu_bo *pt = cursor.entry->base.bo;
>>>>> -        uint64_t pe_start;
>>>>> -        unsigned nptes;
>>>>> +        unsigned shift, parent_shift, num_entries;
>>>>> +        uint64_t incr, entry_end, pe_start;
>>>>>    -        if (!pt || cursor.level != AMDGPU_VM_PTB)
>>>>> +        if (!pt)
>>>>>                return -ENOENT;
>>>>>    -        if ((cursor.pfn & ~mask) == (end & ~mask))
>>>>> -            nptes = end - cursor.pfn;
>>>>> -        else
>>>>> -            nptes = AMDGPU_VM_PTE_COUNT(adev) - (cursor.pfn & mask);
>>>>> -
>>>>> -        amdgpu_vm_handle_huge_pages(params, cursor.entry,
>>>>> cursor.parent,
>>>>> -                        nptes, dst, flags);
>>>>> -        /* We don't need to update PTEs for huge pages */
>>>>> -        if (cursor.entry->huge) {
>>>>> -            dst += nptes * AMDGPU_GPU_PAGE_SIZE;
>>>>> +        /* The root level can't be a huge page */
>>>>> +        if (cursor.level == adev->vm_manager.root_level) {
>>>>> +            if (!amdgpu_vm_pt_descendant(adev, &cursor))
>>>>> +                return -ENOENT;
>>>>>                continue;
>>>>>            }
>>>>>    -        pe_start = (cursor.pfn & mask) * 8;
>>>>> -        amdgpu_vm_update_func(params, pt, pe_start, dst, nptes,
>>>>> -                      AMDGPU_GPU_PAGE_SIZE, flags);
>>>>> -        dst += nptes * AMDGPU_GPU_PAGE_SIZE;
>>>>> -    }
>>>>> -
>>>>> -    return 0;
>>>>> -}
>>>>> +        /* First check if the entry is already handled */
>>>>> +        if (cursor.pfn < frag_start) {
>>>>> +            cursor.entry->huge = true;
>>>>> +            amdgpu_vm_pt_next(adev, &cursor);
>>>>> +            continue;
>>>>> +        }
>>>>>    -/*
>>>>> - * amdgpu_vm_frag_ptes - add fragment information to PTEs
>>>>> - *
>>>>> - * @params: see amdgpu_pte_update_params definition
>>>>> - * @vm: requested vm
>>>>> - * @start: first PTE to handle
>>>>> - * @end: last PTE to handle
>>>>> - * @dst: addr those PTEs should point to
>>>>> - * @flags: hw mapping flags
>>>>> - *
>>>>> - * Returns:
>>>>> - * 0 for success, -EINVAL for failure.
>>>>> - */
>>>>> -static int amdgpu_vm_frag_ptes(struct amdgpu_pte_update_params
>>>>> *params,
>>>>> -                uint64_t start, uint64_t end,
>>>>> -                uint64_t dst, uint64_t flags)
>>>>> -{
>>>>> -    /**
>>>>> -     * The MC L1 TLB supports variable sized pages, based on a
>>>>> fragment
>>>>> -     * field in the PTE. When this field is set to a non-zero
>>>>> value, page
>>>>> -     * granularity is increased from 4KB to (1 << (12 + frag)).
>>>>> The PTE
>>>>> -     * flags are considered valid for all PTEs within the fragment
>>>>> range
>>>>> -     * and corresponding mappings are assumed to be physically
>>>>> contiguous.
>>>>> -     *
>>>>> -     * The L1 TLB can store a single PTE for the whole fragment,
>>>>> -     * significantly increasing the space available for translation
>>>>> -     * caching. This leads to large improvements in throughput
>>>>> when the
>>>>> -     * TLB is under pressure.
>>>>> -     *
>>>>> -     * The L2 TLB distributes small and large fragments into two
>>>>> -     * asymmetric partitions. The large fragment cache is
>>>>> significantly
>>>>> -     * larger. Thus, we try to use large fragments wherever possible.
>>>>> -     * Userspace can support this by aligning virtual base address
>>>>> and
>>>>> -     * allocation size to the fragment size.
>>>>> -     */
>>>>> -    unsigned max_frag = params->adev->vm_manager.fragment_size;
>>>>> -    int r;
>>>>> +        /* If it isn't already handled it can't be a huge page */
>>>>> +        if (cursor.entry->huge) {
>>>>> +            /* Add the entry to the relocated list to update it. */
>>>>> +            cursor.entry->huge = false;
>>>>> + amdgpu_vm_bo_relocated(&cursor.entry->base);
>>>>> +        }
>>>>>    -    /* system pages are non continuously */
>>>>> -    if (params->src || !(flags & AMDGPU_PTE_VALID))
>>>>> -        return amdgpu_vm_update_ptes(params, start, end, dst, flags);
>>>>> -
>>>>> -    while (start != end) {
>>>>> -        uint64_t frag_flags, frag_end;
>>>>> -        unsigned frag;
>>>>> -
>>>>> -        /* This intentionally wraps around if no bit is set */
>>>>> -        frag = min((unsigned)ffs(start) - 1,
>>>>> -               (unsigned)fls64(end - start) - 1);
>>>>> -        if (frag >= max_frag) {
>>>>> -            frag_flags = AMDGPU_PTE_FRAG(max_frag);
>>>>> -            frag_end = end & ~((1ULL << max_frag) - 1);
>>>>> -        } else {
>>>>> -            frag_flags = AMDGPU_PTE_FRAG(frag);
>>>>> -            frag_end = start + (1 << frag);
>>>>> +        shift = amdgpu_vm_level_shift(adev, cursor.level);
>>>>> +        parent_shift = amdgpu_vm_level_shift(adev, cursor.level - 1);
>>>>> +        if (adev->asic_type < CHIP_VEGA10) {
>>>>> +            /* No huge page support before GMC v9 */
>>>>> +            if (cursor.level != AMDGPU_VM_PTB) {
>>>>> +                if (!amdgpu_vm_pt_descendant(adev, &cursor))
>>>>> +                    return -ENOENT;
>>>>> +                continue;
>>>>> +            }
>>>>> +        } else if (frag < shift) {
>>>>> +            /* We can't use this level when the fragment size is
>>>>> +             * smaller than the address shift. Go to the next
>>>>> +             * child entry and try again.
>>>>> +             */
>>>>> +            if (!amdgpu_vm_pt_descendant(adev, &cursor))
>>>>> +                return -ENOENT;
>>>>> +            continue;
>>>>> +        } else if (frag >= parent_shift) {
>>>>> +            /* If the fragment size is even larger than the parent
>>>>> +             * shift we should go up one level and check it again.
>>>>> +             */
>>>>> +            if (!amdgpu_vm_pt_ancestor(&cursor))
>>>>> +                return -ENOENT;
>>>>> +            continue;
>>>>>            }
>>>>>    -        r = amdgpu_vm_update_ptes(params, start, frag_end, dst,
>>>>> -                      flags | frag_flags);
>>>>> -        if (r)
>>>>> -            return r;
>>>>> +        /* Looks good so far, calculate parameters for the update */
>>>>> +        incr = AMDGPU_GPU_PAGE_SIZE << shift;
>>>>> +        num_entries = amdgpu_vm_num_entries(adev, cursor.level);
>>>>> +        pe_start = ((cursor.pfn >> shift) & (num_entries - 1)) * 8;
>>>>> +        entry_end = num_entries << shift;
>>>>> +        entry_end += cursor.pfn & ~(entry_end - 1);
>>>>> +        entry_end = min(entry_end, end);
>>>>> +
>>>>> +        do {
>>>>> +            uint64_t upd_end = min(entry_end, frag_end);
>>>>> +            unsigned nptes = (upd_end - frag_start) >> shift;
>>>>> +
>>>>> +            amdgpu_vm_update_huge(params, pt, cursor.level,
>>>>> +                          pe_start, dst, nptes, incr,
>>>>> +                          flags | AMDGPU_PTE_FRAG(frag));
>>>>> +
>>>>> +            pe_start += nptes * 8;
>>>>> +            dst += nptes * AMDGPU_GPU_PAGE_SIZE << shift;
>>>>> +
>>>>> +            frag_start = upd_end;
>>>>> +            if (frag_start >= frag_end) {
>>>>> +                /* figure out the next fragment */
>>>>> +                amdgpu_vm_fragment(params, frag_start, end,
>>>>> +                           flags, &frag, &frag_end);
>>>>> +                if (frag < shift)
>>>>> +                    break;
>>>>> +            }
>>>>> +        } while (frag_start < entry_end);
>>>>>    -        dst += (frag_end - start) * AMDGPU_GPU_PAGE_SIZE;
>>>>> -        start = frag_end;
>>>>> +        if (frag >= shift)
>>>>> +            amdgpu_vm_pt_next(adev, &cursor);
>>>>>        }
>>>>>          return 0;
>>>>> @@ -1708,8 +1735,8 @@ static int amdgpu_vm_bo_update_mapping(struct
>>>>> amdgpu_device *adev,
>>>>>              params.func = amdgpu_vm_cpu_set_ptes;
>>>>>            params.pages_addr = pages_addr;
>>>>> -        return amdgpu_vm_frag_ptes(&params, start, last + 1,
>>>>> -                       addr, flags);
>>>>> +        return amdgpu_vm_update_ptes(&params, start, last + 1,
>>>>> +                         addr, flags);
>>>>>        }
>>>>>          ring = container_of(vm->entity.rq->sched, struct
>>>>> amdgpu_ring, sched);
>>>>> @@ -1788,7 +1815,7 @@ static int amdgpu_vm_bo_update_mapping(struct
>>>>> amdgpu_device *adev,
>>>>>        if (r)
>>>>>            goto error_free;
>>>>>    -    r = amdgpu_vm_frag_ptes(&params, start, last + 1, addr, flags);
>>>>> +    r = amdgpu_vm_update_ptes(&params, start, last + 1, addr, flags);
>>>>>        if (r)
>>>>>            goto error_free;
>>>>>
> _______________________________________________
> 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