Skip to contentSkip to conversation

drm/amdgpu: abstract bo_base init function

PATCH In review Chunming Zhou 4 participants 7 messages Apr 24, 2018 – May 4, 2018 4 messages not in the archive

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/2 drm/amdgpu: abstract bo_base init function

Change-Id Ia3e57dbacff05f32b6c02e29aeadabd36f08028e
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 75 ++++++++++++++++++----------------
 1 file changed, 40 insertions(+), 35 deletions(-)
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +40 -35
index 8c34060e130f..c75b96433ee7 100644
@@ -94,6 +94,9 @@ struct amdgpu_prt_cb {
 	struct dma_fence_cb cb;
 };
 
+static void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base *base,
+				   struct amdgpu_vm *vm,
+				   struct amdgpu_bo *bo);
 /**
  * amdgpu_vm_level_shift - return the addr shift for each level
  *
@@ -446,11 +449,9 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
 			*/
 			pt->parent = amdgpu_bo_ref(parent->base.bo);
 
-			entry->base.vm = vm;
-			entry->base.bo = pt;
-			list_add_tail(&entry->base.bo_list, &pt->va);
+			amdgpu_vm_bo_base_init(&entry->base, vm, pt);
 			spin_lock(&vm->status_lock);
-			list_add(&entry->base.vm_status, &vm->relocated);
+			list_move(&entry->base.vm_status, &vm->relocated);
 			spin_unlock(&vm->status_lock);
 		}
 
@@ -1821,6 +1822,35 @@ int amdgpu_vm_handle_moved(struct amdgpu_device *adev,
 	return r;
 }
 
+static void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base *base,
+				   struct amdgpu_vm *vm,
+				   struct amdgpu_bo *bo)
+{
+	base->vm = vm;
+	base->bo = bo;
+	INIT_LIST_HEAD(&base->bo_list);
+	INIT_LIST_HEAD(&base->vm_status);
+
+	if (!bo)
+		return;
+	list_add_tail(&base->bo_list, &bo->va);
+
+	if (bo->tbo.resv != vm->root.base.bo->tbo.resv)
+		return;
+
+	if (bo->preferred_domains &
+	    amdgpu_mem_type_to_domain(bo->tbo.mem.mem_type))
+		return;
+
+	/*
+	 * we checked all the prerequisites, but it looks like this per vm bo
+	 * is currently evicted. add the bo to the evicted list to make sure it
+	 * is validated on next vm use to avoid fault.
+	 * */
+	spin_lock(&vm->status_lock);
+	list_move_tail(&base->vm_status, &vm->evicted);
+	spin_unlock(&vm->status_lock);
+}
 /**
  * amdgpu_vm_bo_add - add a bo to a specific vm
  *
@@ -1844,36 +1874,12 @@ struct amdgpu_bo_va *amdgpu_vm_bo_add(struct amdgpu_device *adev,
 	if (bo_va == NULL) {
 		return NULL;
 	}
-	bo_va->base.vm = vm;
-	bo_va->base.bo = bo;
-	INIT_LIST_HEAD(&bo_va->base.bo_list);
-	INIT_LIST_HEAD(&bo_va->base.vm_status);
+	amdgpu_vm_bo_base_init(&bo_va->base, vm, bo);
 
 	bo_va->ref_count = 1;
 	INIT_LIST_HEAD(&bo_va->valids);
 	INIT_LIST_HEAD(&bo_va->invalids);
 
-	if (!bo)
-		return bo_va;
-
-	list_add_tail(&bo_va->base.bo_list, &bo->va);
-
-	if (bo->tbo.resv != vm->root.base.bo->tbo.resv)
-		return bo_va;
-
-	if (bo->preferred_domains &
-	    amdgpu_mem_type_to_domain(bo->tbo.mem.mem_type))
-		return bo_va;
-
-	/*
-	 * We checked all the prerequisites, but it looks like this per VM BO
-	 * is currently evicted. add the BO to the evicted list to make sure it
-	 * is validated on next VM use to avoid fault.
-	 * */
-	spin_lock(&vm->status_lock);
-	list_move_tail(&bo_va->base.vm_status, &vm->evicted);
-	spin_unlock(&vm->status_lock);
-
 	return bo_va;
 }
 
@@ -2373,6 +2379,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 		   int vm_context, unsigned int pasid)
 {
 	struct amdgpu_bo_param bp;
+	struct amdgpu_bo *root;
 	const unsigned align = min(AMDGPU_VM_PTB_ALIGN_SIZE,
 		AMDGPU_VM_PTE_COUNT(adev) * 8);
 	unsigned ring_instance;
@@ -2434,23 +2441,21 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 	bp.flags = flags;
 	bp.type = ttm_bo_type_kernel;
 	bp.resv = NULL;
-	r = amdgpu_bo_create(adev, &bp, &vm->root.base.bo);
+	r = amdgpu_bo_create(adev, &bp, &root);
 	if (r)
 		goto error_free_sched_entity;
 
-	r = amdgpu_bo_reserve(vm->root.base.bo, true);
+	r = amdgpu_bo_reserve(root, true);
 	if (r)
 		goto error_free_root;
 
-	r = amdgpu_vm_clear_bo(adev, vm, vm->root.base.bo,
+	r = amdgpu_vm_clear_bo(adev, vm, root,
 			       adev->vm_manager.root_level,
 			       vm->pte_support_ats);
 	if (r)
 		goto error_unreserve;
 
-	vm->root.base.vm = vm;
-	list_add_tail(&vm->root.base.bo_list, &vm->root.base.bo->va);
-	list_add_tail(&vm->root.base.vm_status, &vm->evicted);
+	amdgpu_vm_bo_base_init(&vm->root.base, vm, root);
 	amdgpu_bo_unreserve(vm->root.base.bo);
 
 	if (pasid) {
Signature · 6 lines
2.14.1

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

A message that is not in the archive (sent off-list or not yet received): [email protected]

#2 lore raw

PATCH 2/2 drm/amdgpu: link shadow bo as well

Shadow BO is located on GTT and its parent (PT and PD) BO could located on VRAM. In some case, the BO on GTT could be evicted but the parent did not. This may cause the shadow BO not be put in the evict list and could not be invalidate correctly.

Change-Id Iad10d9a3031fa2b243879b9e58ee4d8c527eb433
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c     |  5 -----
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     | 11 +++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h     |  1 +
 4 files changed, 13 insertions(+), 5 deletions(-)
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +0 -5
index e1756b68a17b..9c9f6fc5c994 100644
@@ -462,11 +462,6 @@ static int amdgpu_cs_validate(void *param, struct amdgpu_bo *bo)
 	do {
 		r = amdgpu_cs_bo_validate(p, bo);
 	} while (r == -ENOMEM && amdgpu_cs_try_evict(p, bo));
-	if (r)
-		return r;
-
-	if (bo->shadow)
-		r = amdgpu_cs_bo_validate(p, bo->shadow);
 
 	return r;
 }
drivers/gpu/drm/amd/amdgpu/amdgpu_object.h +1 -0
index 540e03fa159f..8078da36aec7 100644
@@ -58,6 +58,7 @@ struct amdgpu_bo_va_mapping {
 /* User space allocated BO in a VM */
 struct amdgpu_bo_va {
 	struct amdgpu_vm_bo_base	base;
+	struct amdgpu_vm_bo_base	shadow_base;
 
 	/* protected by bo being reserved */
 	unsigned			ref_count;
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +11 -0
index c75b96433ee7..026147cc5104 100644
@@ -450,8 +450,13 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
 			pt->parent = amdgpu_bo_ref(parent->base.bo);
 
 			amdgpu_vm_bo_base_init(&entry->base, vm, pt);
+			amdgpu_vm_bo_base_init(&entry->shadow_base, vm,
+					       pt->shadow);
 			spin_lock(&vm->status_lock);
 			list_move(&entry->base.vm_status, &vm->relocated);
+			if (pt->shadow)
+				list_move(&entry->shadow_base.vm_status,
+					  &vm->relocated);
 			spin_unlock(&vm->status_lock);
 		}
 
@@ -1875,6 +1880,7 @@ struct amdgpu_bo_va *amdgpu_vm_bo_add(struct amdgpu_device *adev,
 		return NULL;
 	}
 	amdgpu_vm_bo_base_init(&bo_va->base, vm, bo);
+	amdgpu_vm_bo_base_init(&bo_va->shadow_base, vm, bo ? bo->shadow : NULL);
 
 	bo_va->ref_count = 1;
 	INIT_LIST_HEAD(&bo_va->valids);
@@ -2220,9 +2226,11 @@ void amdgpu_vm_bo_rmv(struct amdgpu_device *adev,
 	struct amdgpu_vm *vm = bo_va->base.vm;
 
 	list_del(&bo_va->base.bo_list);
+	list_del(&bo_va->shadow_base.bo_list);
 
 	spin_lock(&vm->status_lock);
 	list_del(&bo_va->base.vm_status);
+	list_del(&bo_va->shadow_base.vm_status);
 	spin_unlock(&vm->status_lock);
 
 	list_for_each_entry_safe(mapping, next, &bo_va->valids, list) {
@@ -2456,6 +2464,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 		goto error_unreserve;
 
 	amdgpu_vm_bo_base_init(&vm->root.base, vm, root);
+	amdgpu_vm_bo_base_init(&vm->root.shadow_base, vm, root->shadow);
 	amdgpu_bo_unreserve(vm->root.base.bo);
 
 	if (pasid) {
@@ -2575,6 +2584,8 @@ static void amdgpu_vm_free_levels(struct amdgpu_device *adev,
 	if (parent->base.bo) {
 		list_del(&parent->base.bo_list);
 		list_del(&parent->base.vm_status);
+		list_del(&parent->shadow_base.bo_list);
+		list_del(&parent->shadow_base.vm_status);
 		amdgpu_bo_unref(&parent->base.bo->shadow);
 		amdgpu_bo_unref(&parent->base.bo);
 	}
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h +1 -0
index 30f080364c97..f4ae6c6b28b8 100644
@@ -153,6 +153,7 @@ struct amdgpu_vm_bo_base {
 
 struct amdgpu_vm_pt {
 	struct amdgpu_vm_bo_base	base;
+	struct amdgpu_vm_bo_base	shadow_base;
 	bool				huge;
 
 	/* array of page tables, one for each directory entry */
Signature · 6 lines
2.14.1

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

A message that is not in the archive (sent off-list or not yet received): [email protected]

#4 lore raw

Re: [PATCH 2/2] drm/amdgpu: link shadow bo as well

Quoted 48 lines · Chunming Zhou

Shadow BO is located on GTT and its parent (PT and PD) BO could located on VRAM. In some case, the BO on GTT could be evicted but the parent did not. This may cause the shadow BO not be put in the evict list and could not be invalidate correctly.

Change-Id Iad10d9a3031fa2b243879b9e58ee4d8c527eb433
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c     |  5 -----
  drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  1 +
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     | 11 +++++++++++
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h     |  1 +
  4 files changed, 13 insertions(+), 5 deletions(-)
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +0 -5
index e1756b68a17b..9c9f6fc5c994 100644
@@ -462,11 +462,6 @@ static int amdgpu_cs_validate(void *param, struct amdgpu_bo *bo)
  	do {
  		r = amdgpu_cs_bo_validate(p, bo);
  	} while (r == -ENOMEM && amdgpu_cs_try_evict(p, bo));
-	if (r)
-		return r;
-
-	if (bo->shadow)
-		r = amdgpu_cs_bo_validate(p, bo->shadow);
 
  	return r;
  }
drivers/gpu/drm/amd/amdgpu/amdgpu_object.h +1 -0
index 540e03fa159f..8078da36aec7 100644
@@ -58,6 +58,7 @@ struct amdgpu_bo_va_mapping {
  /* User space allocated BO in a VM */
  struct amdgpu_bo_va {
  	struct amdgpu_vm_bo_base	base;
+	struct amdgpu_vm_bo_base	shadow_base;
 
  	/* protected by bo being reserved */
  	unsigned			ref_count;
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +0 -0
index c75b96433ee7..026147cc5104 100644

drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev, · line 459

Quoted 10 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +5 -0
@@ -450,8 +450,13 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
  			pt->parent = amdgpu_bo_ref(parent->base.bo);
 
  			amdgpu_vm_bo_base_init(&entry->base, vm, pt);
+			amdgpu_vm_bo_base_init(&entry->shadow_base, vm,
+					       pt->shadow);
  			spin_lock(&vm->status_lock);
  			list_move(&entry->base.vm_status, &vm->relocated);
+			if (pt->shadow)
+				list_move(&entry->shadow_base.vm_status,
+					  &vm->relocated);

I'm afraid the PD shadow bo could not be updated correctly.

When update PDs, we need get PD and PD's parent for PDE update.
For PD shadow bo, it's parent is the PD in VRAM rather than the PD's upper level PD bo.

Please confirm that.

Jerry

Quoted 46 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +6 -0
  			spin_unlock(&vm->status_lock);
  		}
 
@@ -1875,6 +1880,7 @@ struct amdgpu_bo_va *amdgpu_vm_bo_add(struct amdgpu_device *adev,
  		return NULL;
  	}
  	amdgpu_vm_bo_base_init(&bo_va->base, vm, bo);
+	amdgpu_vm_bo_base_init(&bo_va->shadow_base, vm, bo ? bo->shadow : NULL);
 
  	bo_va->ref_count = 1;
  	INIT_LIST_HEAD(&bo_va->valids);
@@ -2220,9 +2226,11 @@ void amdgpu_vm_bo_rmv(struct amdgpu_device *adev,
  	struct amdgpu_vm *vm = bo_va->base.vm;
 
  	list_del(&bo_va->base.bo_list);
+	list_del(&bo_va->shadow_base.bo_list);
 
  	spin_lock(&vm->status_lock);
  	list_del(&bo_va->base.vm_status);
+	list_del(&bo_va->shadow_base.vm_status);
  	spin_unlock(&vm->status_lock);
 
  	list_for_each_entry_safe(mapping, next, &bo_va->valids, list) {
@@ -2456,6 +2464,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
  		goto error_unreserve;
 
  	amdgpu_vm_bo_base_init(&vm->root.base, vm, root);
+	amdgpu_vm_bo_base_init(&vm->root.shadow_base, vm, root->shadow);
  	amdgpu_bo_unreserve(vm->root.base.bo);
 
  	if (pasid) {
@@ -2575,6 +2584,8 @@ static void amdgpu_vm_free_levels(struct amdgpu_device *adev,
  	if (parent->base.bo) {
  		list_del(&parent->base.bo_list);
  		list_del(&parent->base.vm_status);
+		list_del(&parent->shadow_base.bo_list);
+		list_del(&parent->shadow_base.vm_status);
  		amdgpu_bo_unref(&parent->base.bo->shadow);
  		amdgpu_bo_unref(&parent->base.bo);
  	}
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h +0 -0
index 30f080364c97..f4ae6c6b28b8 100644

drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h struct amdgpu_vm_bo_base { · line 159

Quoted 7 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h +1 -0
@@ -153,6 +153,7 @@ struct amdgpu_vm_bo_base {
 
  struct amdgpu_vm_pt {
  	struct amdgpu_vm_bo_base	base;
+	struct amdgpu_vm_bo_base	shadow_base;
  	bool				huge;
 
  	/* array of page tables, one for each directory entry */

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

#5 lore raw

Re: [PATCH 2/2] drm/amdgpu: link shadow bo as well

In reply to a message not in the archive

Am 24.04.2018 um 09:35 schrieb Chunming Zhou:

Quoted 8 lines

Shadow BO is located on GTT and its parent (PT and PD) BO could located on VRAM. In some case, the BO on GTT could be evicted but the parent did not. This may cause the shadow BO not be put in the evict list and could not be invalidate correctly.

Change-Id Iad10d9a3031fa2b243879b9e58ee4d8c527eb433

Way to much memory usage and to complicated.

What we need to do is just to handle the real BO as evicted when the shadow BO is evicted.

Something like the following at the start of amdgpu_vm_bo_invalidate() should be sufficient:

if (bo->parent && bo->parent->shadow == bo)
    bo = bo->parent;

Regards,
Christian.

Quoted 95 lines
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c     |  5 -----
  drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  1 +
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     | 11 +++++++++++
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h     |  1 +
  4 files changed, 13 insertions(+), 5 deletions(-)
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +0 -5
index e1756b68a17b..9c9f6fc5c994 100644
@@ -462,11 +462,6 @@ static int amdgpu_cs_validate(void *param, struct amdgpu_bo *bo)
  	do {
  		r = amdgpu_cs_bo_validate(p, bo);
  	} while (r == -ENOMEM && amdgpu_cs_try_evict(p, bo));
-	if (r)
-		return r;
-
-	if (bo->shadow)
-		r = amdgpu_cs_bo_validate(p, bo->shadow);
 
  	return r;
  }
drivers/gpu/drm/amd/amdgpu/amdgpu_object.h +1 -0
index 540e03fa159f..8078da36aec7 100644
@@ -58,6 +58,7 @@ struct amdgpu_bo_va_mapping {
  /* User space allocated BO in a VM */
  struct amdgpu_bo_va {
  	struct amdgpu_vm_bo_base	base;
+	struct amdgpu_vm_bo_base	shadow_base;
 
  	/* protected by bo being reserved */
  	unsigned			ref_count;
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +11 -0
index c75b96433ee7..026147cc5104 100644
@@ -450,8 +450,13 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
  			pt->parent = amdgpu_bo_ref(parent->base.bo);
 
  			amdgpu_vm_bo_base_init(&entry->base, vm, pt);
+			amdgpu_vm_bo_base_init(&entry->shadow_base, vm,
+					       pt->shadow);
  			spin_lock(&vm->status_lock);
  			list_move(&entry->base.vm_status, &vm->relocated);
+			if (pt->shadow)
+				list_move(&entry->shadow_base.vm_status,
+					  &vm->relocated);
  			spin_unlock(&vm->status_lock);
  		}
 
@@ -1875,6 +1880,7 @@ struct amdgpu_bo_va *amdgpu_vm_bo_add(struct amdgpu_device *adev,
  		return NULL;
  	}
  	amdgpu_vm_bo_base_init(&bo_va->base, vm, bo);
+	amdgpu_vm_bo_base_init(&bo_va->shadow_base, vm, bo ? bo->shadow : NULL);
 
  	bo_va->ref_count = 1;
  	INIT_LIST_HEAD(&bo_va->valids);
@@ -2220,9 +2226,11 @@ void amdgpu_vm_bo_rmv(struct amdgpu_device *adev,
  	struct amdgpu_vm *vm = bo_va->base.vm;
 
  	list_del(&bo_va->base.bo_list);
+	list_del(&bo_va->shadow_base.bo_list);
 
  	spin_lock(&vm->status_lock);
  	list_del(&bo_va->base.vm_status);
+	list_del(&bo_va->shadow_base.vm_status);
  	spin_unlock(&vm->status_lock);
 
  	list_for_each_entry_safe(mapping, next, &bo_va->valids, list) {
@@ -2456,6 +2464,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
  		goto error_unreserve;
 
  	amdgpu_vm_bo_base_init(&vm->root.base, vm, root);
+	amdgpu_vm_bo_base_init(&vm->root.shadow_base, vm, root->shadow);
  	amdgpu_bo_unreserve(vm->root.base.bo);
 
  	if (pasid) {
@@ -2575,6 +2584,8 @@ static void amdgpu_vm_free_levels(struct amdgpu_device *adev,
  	if (parent->base.bo) {
  		list_del(&parent->base.bo_list);
  		list_del(&parent->base.vm_status);
+		list_del(&parent->shadow_base.bo_list);
+		list_del(&parent->shadow_base.vm_status);
  		amdgpu_bo_unref(&parent->base.bo->shadow);
  		amdgpu_bo_unref(&parent->base.bo);
  	}
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h +0 -0
index 30f080364c97..f4ae6c6b28b8 100644

drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h struct amdgpu_vm_bo_base { · line 159

Quoted 7 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h +1 -0
@@ -153,6 +153,7 @@ struct amdgpu_vm_bo_base {
 
  struct amdgpu_vm_pt {
  	struct amdgpu_vm_bo_base	base;
+	struct amdgpu_vm_bo_base	shadow_base;
  	bool				huge;
 
  	/* array of page tables, one for each directory entry */

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

A message that is not in the archive (sent off-list or not yet received): 9146e34c-85f7-5eca-1fa1-50584512658c-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

#7 lore raw

Re: [PATCH 2/2] drm/amdgpu: link shadow bo as well

Quoted 10 lines · Christian König

Am 24.04.2018 um 09:35 schrieb Chunming Zhou:

Quoted 8 lines

Shadow BO is located on GTT and its parent (PT and PD) BO could located on VRAM. In some case, the BO on GTT could be evicted but the parent did not. This may cause the shadow BO not be put in the evict list and could not be invalidate correctly.

Change-Id Iad10d9a3031fa2b243879b9e58ee4d8c527eb433
Quoted 8 lines

Way to much memory usage and to complicated.

What we need to do is just to handle the real BO as evicted when the shadow BO is evicted.

Something like the following at the start of amdgpu_vm_bo_invalidate() should be sufficient:

if (bo->parent && bo->parent->shadow == bo)
    bo = bo->parent;

clever, will send a patch soon.

Regards,
David Zhou

Quoted 92 lines

Regards,
Christian.

Quoted 87 lines

---
  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c     |  5 ----- drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  1 + drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     | 11 +++++++++++ drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h     |  1 +

  4 files changed, 13 insertions(+), 5 deletions(-)
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +0 -5
index e1756b68a17b..9c9f6fc5c994 100644
@@ -462,11 +462,6 @@ static int amdgpu_cs_validate(void *param, 
 struct amdgpu_bo *bo)
       do {
           r = amdgpu_cs_bo_validate(p, bo);
       } while (r == -ENOMEM && amdgpu_cs_try_evict(p, bo));
-    if (r)
-        return r;
-
-    if (bo->shadow)
-        r = amdgpu_cs_bo_validate(p, bo->shadow);
         return r;
   }
drivers/gpu/drm/amd/amdgpu/amdgpu_object.h +1 -0
index 540e03fa159f..8078da36aec7 100644
@@ -58,6 +58,7 @@ struct amdgpu_bo_va_mapping {
   /* User space allocated BO in a VM */
   struct amdgpu_bo_va {
       struct amdgpu_vm_bo_base    base;
+    struct amdgpu_vm_bo_base    shadow_base;
         /* protected by bo being reserved */
       unsigned            ref_count;
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +5 -0
index c75b96433ee7..026147cc5104 100644
@@ -450,8 +450,13 @@ static int amdgpu_vm_alloc_levels(struct 
 amdgpu_device *adev,
               pt->parent = amdgpu_bo_ref(parent->base.bo);
                 amdgpu_vm_bo_base_init(&entry->base, vm, pt);
+            amdgpu_vm_bo_base_init(&entry->shadow_base, vm,
+                           pt->shadow);
               spin_lock(&vm->status_lock);
               list_move(&entry->base.vm_status, &vm->relocated);
+            if (pt->shadow)
+                list_move(&entry->shadow_base.vm_status,
+                      &vm->relocated);
               spin_unlock(&vm->status_lock);
           }
   @@ -1875,6 +1880,7 @@ struct amdgpu_bo_va *amdgpu_vm_bo_add(struct amdgpu_device *adev,

          return NULL;
      }
      amdgpu_vm_bo_base_init(&bo_va->base, vm, bo);
+    amdgpu_vm_bo_base_init(&bo_va->shadow_base, vm, bo ? bo->shadow : NULL);
        bo_va->ref_count = 1; INIT_LIST_HEAD(&bo_va->valids);

drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +5 -0
@@ -2220,9 +2226,11 @@ void amdgpu_vm_bo_rmv(struct amdgpu_device *adev,
       struct amdgpu_vm *vm = bo_va->base.vm;
         list_del(&bo_va->base.bo_list);
+    list_del(&bo_va->shadow_base.bo_list);
         spin_lock(&vm->status_lock);
       list_del(&bo_va->base.vm_status);
+    list_del(&bo_va->shadow_base.vm_status);
       spin_unlock(&vm->status_lock);
         list_for_each_entry_safe(mapping, next, &bo_va->valids, list) {
@@ -2456,6 +2464,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, 
 struct amdgpu_vm *vm,
           goto error_unreserve;
         amdgpu_vm_bo_base_init(&vm->root.base, vm, root);
+    amdgpu_vm_bo_base_init(&vm->root.shadow_base, vm, root->shadow);
       amdgpu_bo_unreserve(vm->root.base.bo);
         if (pasid) {
@@ -2575,6 +2584,8 @@ static void amdgpu_vm_free_levels(struct 
 amdgpu_device *adev,
       if (parent->base.bo) {
           list_del(&parent->base.bo_list);
           list_del(&parent->base.vm_status);
+        list_del(&parent->shadow_base.bo_list);
+        list_del(&parent->shadow_base.vm_status);
           amdgpu_bo_unref(&parent->base.bo->shadow);
           amdgpu_bo_unref(&parent->base.bo);

      }

drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h +0 -0
index 30f080364c97..f4ae6c6b28b8 100644
Quoted 9 lines

drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h struct amdgpu_vm_bo_base { · line 157

Quoted 8 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h +1 -0
@@ -153,6 +153,7 @@ struct amdgpu_vm_bo_base {
     struct amdgpu_vm_pt {
       struct amdgpu_vm_bo_base    base;
+    struct amdgpu_vm_bo_base    shadow_base;
       bool                huge;
         /* array of page tables, one for each directory entry */

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Reviewed-by Junwei Zhang
#8 lore raw
In reply to a message not in the archive
Quoted 2 lines · Chunming Zhou
Change-Id Ia3e57dbacff05f32b6c02e29aeadabd36f08028e

some trivial comments.

Quoted 9 lines
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 75 ++++++++++++++++++----------------
  1 file changed, 40 insertions(+), 35 deletions(-)
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +0 -0
index 8c34060e130f..c75b96433ee7 100644

drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c struct amdgpu_prt_cb { · line 99

Quoted 6 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +3 -0
@@ -94,6 +94,9 @@ struct amdgpu_prt_cb {
  	struct dma_fence_cb cb;
  };
 
+static void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base *base,
+				   struct amdgpu_vm *vm,
+				   struct amdgpu_bo *bo);

Move the function upper, we could avoid such declaration.

Quoted 36 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +17 -4
  /**
   * amdgpu_vm_level_shift - return the addr shift for each level
   *
@@ -446,11 +449,9 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
  			*/
  			pt->parent = amdgpu_bo_ref(parent->base.bo);
 
-			entry->base.vm = vm;
-			entry->base.bo = pt;
-			list_add_tail(&entry->base.bo_list, &pt->va);
+			amdgpu_vm_bo_base_init(&entry->base, vm, pt);
  			spin_lock(&vm->status_lock);
-			list_add(&entry->base.vm_status, &vm->relocated);
+			list_move(&entry->base.vm_status, &vm->relocated);
  			spin_unlock(&vm->status_lock);
  		}
 
@@ -1821,6 +1822,35 @@ int amdgpu_vm_handle_moved(struct amdgpu_device *adev,
  	return r;
  }
 
+static void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base *base,
+				   struct amdgpu_vm *vm,
+				   struct amdgpu_bo *bo)
+{
+	base->vm = vm;
+	base->bo = bo;
+	INIT_LIST_HEAD(&base->bo_list);
+	INIT_LIST_HEAD(&base->vm_status);
+
+	if (!bo)
+		return;
+	list_add_tail(&base->bo_list, &bo->va);
+
+	if (bo->tbo.resv != vm->root.base.bo->tbo.resv)
+		return;

drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c int amdgpu_vm_handle_moved(struct amdgpu_device *adev, · line 1849

Quoted 10 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +10 -0
+
+	if (bo->preferred_domains &
+	    amdgpu_mem_type_to_domain(bo->tbo.mem.mem_type))
+		return;
+
+	/*
+	 * we checked all the prerequisites, but it looks like this per vm bo
+	 * is currently evicted. add the bo to the evicted list to make sure it
+	 * is validated on next vm use to avoid fault.
+	 * */

/*
*
*/ is expected

Quoted 72 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +9 -28
+	spin_lock(&vm->status_lock);
+	list_move_tail(&base->vm_status, &vm->evicted);
+	spin_unlock(&vm->status_lock);
+}
  /**
   * amdgpu_vm_bo_add - add a bo to a specific vm
   *
@@ -1844,36 +1874,12 @@ struct amdgpu_bo_va *amdgpu_vm_bo_add(struct amdgpu_device *adev,
  	if (bo_va == NULL) {
  		return NULL;
  	}
-	bo_va->base.vm = vm;
-	bo_va->base.bo = bo;
-	INIT_LIST_HEAD(&bo_va->base.bo_list);
-	INIT_LIST_HEAD(&bo_va->base.vm_status);
+	amdgpu_vm_bo_base_init(&bo_va->base, vm, bo);
 
  	bo_va->ref_count = 1;
  	INIT_LIST_HEAD(&bo_va->valids);
  	INIT_LIST_HEAD(&bo_va->invalids);
 
-	if (!bo)
-		return bo_va;
-
-	list_add_tail(&bo_va->base.bo_list, &bo->va);
-
-	if (bo->tbo.resv != vm->root.base.bo->tbo.resv)
-		return bo_va;
-
-	if (bo->preferred_domains &
-	    amdgpu_mem_type_to_domain(bo->tbo.mem.mem_type))
-		return bo_va;
-
-	/*
-	 * We checked all the prerequisites, but it looks like this per VM BO
-	 * is currently evicted. add the BO to the evicted list to make sure it
-	 * is validated on next VM use to avoid fault.
-	 * */
-	spin_lock(&vm->status_lock);
-	list_move_tail(&bo_va->base.vm_status, &vm->evicted);
-	spin_unlock(&vm->status_lock);
-
  	return bo_va;
  }
 
@@ -2373,6 +2379,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
  		   int vm_context, unsigned int pasid)
  {
  	struct amdgpu_bo_param bp;
+	struct amdgpu_bo *root;
  	const unsigned align = min(AMDGPU_VM_PTB_ALIGN_SIZE,
  		AMDGPU_VM_PTE_COUNT(adev) * 8);
  	unsigned ring_instance;
@@ -2434,23 +2441,21 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
  	bp.flags = flags;
  	bp.type = ttm_bo_type_kernel;
  	bp.resv = NULL;
-	r = amdgpu_bo_create(adev, &bp, &vm->root.base.bo);
+	r = amdgpu_bo_create(adev, &bp, &root);
  	if (r)
  		goto error_free_sched_entity;
 
-	r = amdgpu_bo_reserve(vm->root.base.bo, true);
+	r = amdgpu_bo_reserve(root, true);
  	if (r)
  		goto error_free_root;
 
-	r = amdgpu_vm_clear_bo(adev, vm, vm->root.base.bo,
+	r = amdgpu_vm_clear_bo(adev, vm, root,
  			       adev->vm_manager.root_level,
  			       vm->pte_support_ats);

drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, · line 2461

Quoted 10 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +1 -3
  	if (r)
  		goto error_unreserve;
 
-	vm->root.base.vm = vm;
-	list_add_tail(&vm->root.base.bo_list, &vm->root.base.bo->va);
-	list_add_tail(&vm->root.base.vm_status, &vm->evicted);
+	amdgpu_vm_bo_base_init(&vm->root.base, vm, root);
  	amdgpu_bo_unreserve(vm->root.base.bo);
 
  	if (pasid) {

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

A message that is not in the archive (sent off-list or not yet received): [email protected]

#10 lore raw

Am 26.04.2018 um 05:01 schrieb Zhang, Jerry (Junwei):

Quoted 6 lines · Chunming Zhou
Quoted 2 lines
Change-Id Ia3e57dbacff05f32b6c02e29aeadabd36f08028e

some trivial comments.

Both a nice cleanup as well as the right comments.

With Jerry's comments addressed Reviewed-by: Christian König <[email protected]> as well.

Quoted 154 lines
Quoted 11 lines

---
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 75
++++++++++++++++++----------------

  1 file changed, 40 insertions(+), 35 deletions(-)
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +0 -0
index 8c34060e130f..c75b96433ee7 100644

drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c struct amdgpu_prt_cb { · line 99

Quoted 6 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +3 -0
@@ -94,6 +94,9 @@ struct amdgpu_prt_cb {
       struct dma_fence_cb cb;
   };
 
+static void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base *base,
+                   struct amdgpu_vm *vm,
+                   struct amdgpu_bo *bo);

Move the function upper, we could avoid such declaration.

Quoted 45 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +24 -4
   /**
    * amdgpu_vm_level_shift - return the addr shift for each level
    *
@@ -446,11 +449,9 @@ static int amdgpu_vm_alloc_levels(struct 
 amdgpu_device *adev,
               */
               pt->parent = amdgpu_bo_ref(parent->base.bo);
 
-            entry->base.vm = vm;
-            entry->base.bo = pt;
-            list_add_tail(&entry->base.bo_list, &pt->va);
+            amdgpu_vm_bo_base_init(&entry->base, vm, pt);
               spin_lock(&vm->status_lock);
-            list_add(&entry->base.vm_status, &vm->relocated);
+            list_move(&entry->base.vm_status, &vm->relocated);
               spin_unlock(&vm->status_lock);
           }
@@ -1821,6 +1822,35 @@ int amdgpu_vm_handle_moved(struct 
 amdgpu_device *adev,
       return r;
   }
 
+static void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base *base,
+                   struct amdgpu_vm *vm,
+                   struct amdgpu_bo *bo)
+{
+    base->vm = vm;
+    base->bo = bo;
+    INIT_LIST_HEAD(&base->bo_list);
+    INIT_LIST_HEAD(&base->vm_status);
+
+    if (!bo)
+        return;
+    list_add_tail(&base->bo_list, &bo->va);
+
+    if (bo->tbo.resv != vm->root.base.bo->tbo.resv)
+        return;
+
+    if (bo->preferred_domains &
+        amdgpu_mem_type_to_domain(bo->tbo.mem.mem_type))
+        return;
+
+    /*
+     * we checked all the prerequisites, but it looks like this per 
Quoted 5 lines

vm bo
+     * is currently evicted. add the bo to the evicted list to make sure it
+     * is validated on next vm use to avoid fault.
+     * */

/*
 *
 */  is expected

Quoted 79 lines

+    spin_lock(&vm->status_lock);
+    list_move_tail(&base->vm_status, &vm->evicted); +    spin_unlock(&vm->status_lock);
+}
  /**
   * amdgpu_vm_bo_add - add a bo to a specific vm *

drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +1 -18
@@ -1844,36 +1874,12 @@ struct amdgpu_bo_va *amdgpu_vm_bo_add(struct 
 amdgpu_device *adev,
       if (bo_va == NULL) {
           return NULL;
       }
-    bo_va->base.vm = vm;
-    bo_va->base.bo = bo;
-    INIT_LIST_HEAD(&bo_va->base.bo_list);
-    INIT_LIST_HEAD(&bo_va->base.vm_status);
+    amdgpu_vm_bo_base_init(&bo_va->base, vm, bo);
 
       bo_va->ref_count = 1;
       INIT_LIST_HEAD(&bo_va->valids);
       INIT_LIST_HEAD(&bo_va->invalids);
 
-    if (!bo)
-        return bo_va;
-
-    list_add_tail(&bo_va->base.bo_list, &bo->va);
-
-    if (bo->tbo.resv != vm->root.base.bo->tbo.resv)
-        return bo_va;
-
-    if (bo->preferred_domains &
-        amdgpu_mem_type_to_domain(bo->tbo.mem.mem_type))
-        return bo_va;
-
-    /*
-     * We checked all the prerequisites, but it looks like this per 

VM BO
-     * is currently evicted. add the BO to the evicted list to make sure it
-     * is validated on next VM use to avoid fault.
-     * */
-    spin_lock(&vm->status_lock);
-    list_move_tail(&bo_va->base.vm_status, &vm->evicted); -    spin_unlock(&vm->status_lock);
-
      return bo_va;
  }

drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +1 -0
@@ -2373,6 +2379,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, 
 struct amdgpu_vm *vm,
              int vm_context, unsigned int pasid)
   {
       struct amdgpu_bo_param bp;
+    struct amdgpu_bo *root;
       const unsigned align = min(AMDGPU_VM_PTB_ALIGN_SIZE,
           AMDGPU_VM_PTE_COUNT(adev) * 8);

      unsigned ring_instance;

drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +3 -3
@@ -2434,23 +2441,21 @@ int amdgpu_vm_init(struct amdgpu_device 
 *adev, struct amdgpu_vm *vm,
       bp.flags = flags;
       bp.type = ttm_bo_type_kernel;
       bp.resv = NULL;
-    r = amdgpu_bo_create(adev, &bp, &vm->root.base.bo);
+    r = amdgpu_bo_create(adev, &bp, &root);
       if (r)
           goto error_free_sched_entity;
 
-    r = amdgpu_bo_reserve(vm->root.base.bo, true);
+    r = amdgpu_bo_reserve(root, true);
       if (r)
           goto error_free_root;
 
-    r = amdgpu_vm_clear_bo(adev, vm, vm->root.base.bo,
+    r = amdgpu_vm_clear_bo(adev, vm, root,
                      adev->vm_manager.root_level,
                      vm->pte_support_ats);
       if (r)
           goto error_unreserve;
 
Quoted 8 lines
Quoted 8 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +1 -3
-    vm->root.base.vm = vm;
-    list_add_tail(&vm->root.base.bo_list, &vm->root.base.bo->va);
-    list_add_tail(&vm->root.base.vm_status, &vm->evicted);
+    amdgpu_vm_bo_base_init(&vm->root.base, vm, root);
       amdgpu_bo_unreserve(vm->root.base.bo);
 

      if (pasid) {

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx