[PATCH 1/5] drm/amdgpu: rework moved handling in the VM v2

[PATCH 1/5] drm/amdgpu: rework moved handling in the VM v2

[Christian König]

From: Christian König <christian.koenig@amd.com>

Instead of using the vm_state use a separate flag to note
that the BO was moved.

v2: reorder patches to avoid temporary lockless access

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index f621dba..ee53293 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1787,10 +1787,16 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev,
 	else
 		flags = 0x0;
 
-	spin_lock(&vm->status_lock);
-	if (!list_empty(&bo_va->base.vm_status))
+	if (!clear && bo_va->base.moved) {
+		bo_va->base.moved = false;
 		list_splice_init(&bo_va->valids, &bo_va->invalids);
-	spin_unlock(&vm->status_lock);
+
+	} else {
+		spin_lock(&vm->status_lock);
+		if (!list_empty(&bo_va->base.vm_status))
+			list_splice_init(&bo_va->valids, &bo_va->invalids);
+		spin_unlock(&vm->status_lock);
+	}
 
 	list_for_each_entry(mapping, &bo_va->invalids, list) {
 		r = amdgpu_vm_bo_split_mapping(adev, exclusive, pages_addr, vm,
@@ -2418,6 +2424,7 @@ void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
 	struct amdgpu_vm_bo_base *bo_base;
 
 	list_for_each_entry(bo_base, &bo->va, bo_list) {
+		bo_base->moved = true;
 		spin_lock(&bo_base->vm->status_lock);
 		if (list_empty(&bo_base->vm_status))
 			list_add(&bo_base->vm_status,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 9347d28..1b478e6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -105,6 +105,9 @@ struct amdgpu_vm_bo_base {
 
 	/* protected by spinlock */
 	struct list_head		vm_status;
+
+	/* protected by the BO being reserved */
+	bool				moved;
 };
 
 struct amdgpu_vm_pt {
-- 
2.7.4

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

[PATCH 2/5] drm/amdgpu: add bo_va cleared flag again v2

[Christian König]

From: Christian König <christian.koenig@amd.com>

We changed this to use an extra list a while back, but for the next
series I need a separate flag again.

v2: reorder to avoid unlocked list access

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  3 +++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     | 35 +++++++++++-------------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h     |  3 ---
 3 files changed, 16 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
index a288fa6..e613ba4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
@@ -55,6 +55,9 @@ struct amdgpu_bo_va {
 	/* mappings for this bo_va */
 	struct list_head		invalids;
 	struct list_head		valids;
+
+	/* If the mappings are cleared or filled */
+	bool				cleared;
 };
 
 struct amdgpu_bo {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index ee53293..85189f1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1791,11 +1791,8 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev,
 		bo_va->base.moved = false;
 		list_splice_init(&bo_va->valids, &bo_va->invalids);
 
-	} else {
-		spin_lock(&vm->status_lock);
-		if (!list_empty(&bo_va->base.vm_status))
-			list_splice_init(&bo_va->valids, &bo_va->invalids);
-		spin_unlock(&vm->status_lock);
+	} else if (bo_va->cleared != clear) {
+		list_splice_init(&bo_va->valids, &bo_va->invalids);
 	}
 
 	list_for_each_entry(mapping, &bo_va->invalids, list) {
@@ -1806,25 +1803,22 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev,
 			return r;
 	}
 
-	if (trace_amdgpu_vm_bo_mapping_enabled()) {
-		list_for_each_entry(mapping, &bo_va->valids, list)
-			trace_amdgpu_vm_bo_mapping(mapping);
-
-		list_for_each_entry(mapping, &bo_va->invalids, list)
-			trace_amdgpu_vm_bo_mapping(mapping);
+	if (vm->use_cpu_for_update) {
+		/* Flush HDP */
+		mb();
+		amdgpu_gart_flush_gpu_tlb(adev, 0);
 	}
 
 	spin_lock(&vm->status_lock);
-	list_splice_init(&bo_va->invalids, &bo_va->valids);
 	list_del_init(&bo_va->base.vm_status);
-	if (clear)
-		list_add(&bo_va->base.vm_status, &vm->cleared);
 	spin_unlock(&vm->status_lock);
 
-	if (vm->use_cpu_for_update) {
-		/* Flush HDP */
-		mb();
-		amdgpu_gart_flush_gpu_tlb(adev, 0);
+	list_splice_init(&bo_va->invalids, &bo_va->valids);
+	bo_va->cleared = clear;
+
+	if (trace_amdgpu_vm_bo_mapping_enabled()) {
+		list_for_each_entry(mapping, &bo_va->valids, list)
+			trace_amdgpu_vm_bo_mapping(mapping);
 	}
 
 	return 0;
@@ -2426,9 +2420,7 @@ void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
 	list_for_each_entry(bo_base, &bo->va, bo_list) {
 		bo_base->moved = true;
 		spin_lock(&bo_base->vm->status_lock);
-		if (list_empty(&bo_base->vm_status))
-			list_add(&bo_base->vm_status,
-				 &bo_base->vm->moved);
+		list_move(&bo_base->vm_status, &bo_base->vm->moved);
 		spin_unlock(&bo_base->vm->status_lock);
 	}
 }
@@ -2515,7 +2507,6 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 		vm->reserved_vmid[i] = NULL;
 	spin_lock_init(&vm->status_lock);
 	INIT_LIST_HEAD(&vm->moved);
-	INIT_LIST_HEAD(&vm->cleared);
 	INIT_LIST_HEAD(&vm->freed);
 
 	/* create scheduler entity for page table updates */
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 1b478e6..ff093d4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -129,9 +129,6 @@ struct amdgpu_vm {
 	/* BOs moved, but not yet updated in the PT */
 	struct list_head	moved;
 
-	/* BOs cleared in the PT because of a move */
-	struct list_head	cleared;
-
 	/* BO mappings freed, but not yet updated in the PT */
 	struct list_head	freed;
 
-- 
2.7.4

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

[PATCH 3/5] drm/amdgpu: fix comment on amdgpu_bo_va

[Christian König]

From: Christian König <christian.koenig@amd.com>

Except for the reference count all other members are protected
by the VM PD being reserved.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
index e613ba4..42492e6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
@@ -49,9 +49,11 @@ struct amdgpu_bo_va {
 	struct amdgpu_vm_bo_base	base;
 
 	/* protected by bo being reserved */
-	struct dma_fence	        *last_pt_update;
 	unsigned			ref_count;
 
+	/* all other members protected by the VM PD being reserved */
+	struct dma_fence	        *last_pt_update;
+
 	/* mappings for this bo_va */
 	struct list_head		invalids;
 	struct list_head		valids;
-- 
2.7.4

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

[PATCH 4/5] drm/amdgpu: track evicted page tables v2

[Christian König]

From: Christian König <christian.koenig@amd.com>

Instead of validating all page tables when one was evicted,
track which one needs a validation.

v2: simplify amdgpu_vm_ready as well

Signed-off-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com> (v1)
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c     |   7 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c    |   8 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |   2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     | 227 +++++++++++++----------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h     |  16 +-
 5 files changed, 119 insertions(+), 141 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 3f46b5a..f68ac56 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -632,9 +632,6 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
 
 	amdgpu_cs_report_moved_bytes(p->adev, p->bytes_moved,
 				     p->bytes_moved_vis);
-	fpriv->vm.last_eviction_counter =
-		atomic64_read(&p->adev->num_evictions);
-
 	if (p->bo_list) {
 		struct amdgpu_bo *gds = p->bo_list->gds_obj;
 		struct amdgpu_bo *gws = p->bo_list->gws_obj;
@@ -826,7 +823,7 @@ static int amdgpu_bo_vm_update_pte(struct amdgpu_cs_parser *p)
 			if (!bo)
 				continue;
 
-			amdgpu_vm_bo_invalidate(adev, bo);
+			amdgpu_vm_bo_invalidate(adev, bo, false);
 		}
 	}
 
@@ -851,7 +848,7 @@ static int amdgpu_cs_ib_vm_chunk(struct amdgpu_device *adev,
 	}
 
 	if (p->job->vm) {
-		p->job->vm_pd_addr = amdgpu_bo_gpu_offset(vm->root.bo);
+		p->job->vm_pd_addr = amdgpu_bo_gpu_offset(vm->root.base.bo);
 
 		r = amdgpu_bo_vm_update_pte(p);
 		if (r)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index ba01293..d028806 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -160,7 +160,7 @@ void amdgpu_gem_object_close(struct drm_gem_object *obj,
 	if (bo_va && --bo_va->ref_count == 0) {
 		amdgpu_vm_bo_rmv(adev, bo_va);
 
-		if (amdgpu_vm_ready(adev, vm)) {
+		if (amdgpu_vm_ready(vm)) {
 			struct dma_fence *fence = NULL;
 
 			r = amdgpu_vm_clear_freed(adev, vm, &fence);
@@ -481,10 +481,10 @@ static void amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
 				    struct list_head *list,
 				    uint32_t operation)
 {
-	int r = -ERESTARTSYS;
+	int r;
 
-	if (!amdgpu_vm_ready(adev, vm))
-		goto error;
+	if (!amdgpu_vm_ready(vm))
+		return;
 
 	r = amdgpu_vm_update_directories(adev, vm);
 	if (r)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 9e495da..52d0109 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -929,7 +929,7 @@ void amdgpu_bo_move_notify(struct ttm_buffer_object *bo,
 		return;
 
 	abo = container_of(bo, struct amdgpu_bo, tbo);
-	amdgpu_vm_bo_invalidate(adev, abo);
+	amdgpu_vm_bo_invalidate(adev, abo, evict);
 
 	amdgpu_bo_kunmap(abo);
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 85189f1..592c3e7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -140,7 +140,7 @@ void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
 			 struct list_head *validated,
 			 struct amdgpu_bo_list_entry *entry)
 {
-	entry->robj = vm->root.bo;
+	entry->robj = vm->root.base.bo;
 	entry->priority = 0;
 	entry->tv.bo = &entry->robj->tbo;
 	entry->tv.shared = true;
@@ -149,61 +149,6 @@ void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
 }
 
 /**
- * amdgpu_vm_validate_layer - validate a single page table level
- *
- * @parent: parent page table level
- * @validate: callback to do the validation
- * @param: parameter for the validation callback
- *
- * Validate the page table BOs on command submission if neccessary.
- */
-static int amdgpu_vm_validate_level(struct amdgpu_vm_pt *parent,
-				    int (*validate)(void *, struct amdgpu_bo *),
-				    void *param, bool use_cpu_for_update,
-				    struct ttm_bo_global *glob)
-{
-	unsigned i;
-	int r;
-
-	if (use_cpu_for_update) {
-		r = amdgpu_bo_kmap(parent->bo, NULL);
-		if (r)
-			return r;
-	}
-
-	if (!parent->entries)
-		return 0;
-
-	for (i = 0; i <= parent->last_entry_used; ++i) {
-		struct amdgpu_vm_pt *entry = &parent->entries[i];
-
-		if (!entry->bo)
-			continue;
-
-		r = validate(param, entry->bo);
-		if (r)
-			return r;
-
-		spin_lock(&glob->lru_lock);
-		ttm_bo_move_to_lru_tail(&entry->bo->tbo);
-		if (entry->bo->shadow)
-			ttm_bo_move_to_lru_tail(&entry->bo->shadow->tbo);
-		spin_unlock(&glob->lru_lock);
-
-		/*
-		 * Recurse into the sub directory. This is harmless because we
-		 * have only a maximum of 5 layers.
-		 */
-		r = amdgpu_vm_validate_level(entry, validate, param,
-					     use_cpu_for_update, glob);
-		if (r)
-			return r;
-	}
-
-	return r;
-}
-
-/**
  * amdgpu_vm_validate_pt_bos - validate the page table BOs
  *
  * @adev: amdgpu device pointer
@@ -217,32 +162,43 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 			      int (*validate)(void *p, struct amdgpu_bo *bo),
 			      void *param)
 {
-	uint64_t num_evictions;
+	struct ttm_bo_global *glob = adev->mman.bdev.glob;
+	int r;
 
-	/* We only need to validate the page tables
-	 * if they aren't already valid.
-	 */
-	num_evictions = atomic64_read(&adev->num_evictions);
-	if (num_evictions == vm->last_eviction_counter)
-		return 0;
+	spin_lock(&vm->status_lock);
+	while (!list_empty(&vm->evicted)) {
+		struct amdgpu_vm_bo_base *bo_base;
+		struct amdgpu_bo *bo;
 
-	return amdgpu_vm_validate_level(&vm->root, validate, param,
-					vm->use_cpu_for_update,
-					adev->mman.bdev.glob);
-}
+		bo_base = list_first_entry(&vm->evicted,
+					   struct amdgpu_vm_bo_base,
+					   vm_status);
+		spin_unlock(&vm->status_lock);
 
-/**
- * amdgpu_vm_check - helper for amdgpu_vm_ready
- */
-static int amdgpu_vm_check(void *param, struct amdgpu_bo *bo)
-{
-	/* if anything is swapped out don't swap it in here,
-	   just abort and wait for the next CS */
-	if (!amdgpu_bo_gpu_accessible(bo))
-		return -ERESTARTSYS;
+		bo = bo_base->bo;
+		BUG_ON(!bo);
+		if (bo->parent) {
+			r = validate(param, bo);
+			if (r)
+				return r;
 
-	if (bo->shadow && !amdgpu_bo_gpu_accessible(bo->shadow))
-		return -ERESTARTSYS;
+			spin_lock(&glob->lru_lock);
+			ttm_bo_move_to_lru_tail(&bo->tbo);
+			if (bo->shadow)
+				ttm_bo_move_to_lru_tail(&bo->shadow->tbo);
+			spin_unlock(&glob->lru_lock);
+		}
+
+		if (vm->use_cpu_for_update) {
+			r = amdgpu_bo_kmap(bo, NULL);
+			if (r)
+				return r;
+		}
+
+		spin_lock(&vm->status_lock);
+		list_del_init(&bo_base->vm_status);
+	}
+	spin_unlock(&vm->status_lock);
 
 	return 0;
 }
@@ -250,17 +206,19 @@ static int amdgpu_vm_check(void *param, struct amdgpu_bo *bo)
 /**
  * amdgpu_vm_ready - check VM is ready for updates
  *
- * @adev: amdgpu device
  * @vm: VM to check
  *
  * Check if all VM PDs/PTs are ready for updates
  */
-bool amdgpu_vm_ready(struct amdgpu_device *adev, struct amdgpu_vm *vm)
+bool amdgpu_vm_ready(struct amdgpu_vm *vm)
 {
-	if (amdgpu_vm_check(NULL, vm->root.bo))
-		return false;
+	bool ready;
+
+	spin_lock(&vm->status_lock);
+	ready = list_empty(&vm->evicted);
+	spin_unlock(&vm->status_lock);
 
-	return !amdgpu_vm_validate_pt_bos(adev, vm, amdgpu_vm_check, NULL);
+	return ready;
 }
 
 /**
@@ -325,11 +283,11 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
 
 	/* walk over the address space and allocate the page tables */
 	for (pt_idx = from; pt_idx <= to; ++pt_idx) {
-		struct reservation_object *resv = vm->root.bo->tbo.resv;
+		struct reservation_object *resv = vm->root.base.bo->tbo.resv;
 		struct amdgpu_vm_pt *entry = &parent->entries[pt_idx];
 		struct amdgpu_bo *pt;
 
-		if (!entry->bo) {
+		if (!entry->base.bo) {
 			r = amdgpu_bo_create(adev,
 					     amdgpu_vm_bo_size(adev, level),
 					     AMDGPU_GPU_PAGE_SIZE, true,
@@ -350,9 +308,12 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
 			/* Keep a reference to the root directory to avoid
 			* freeing them up in the wrong order.
 			*/
-			pt->parent = amdgpu_bo_ref(vm->root.bo);
+			pt->parent = amdgpu_bo_ref(vm->root.base.bo);
 
-			entry->bo = pt;
+			entry->base.vm = vm;
+			entry->base.bo = pt;
+			list_add_tail(&entry->base.bo_list, &pt->va);
+			INIT_LIST_HEAD(&entry->base.vm_status);
 			entry->addr = 0;
 		}
 
@@ -1019,7 +980,7 @@ static int amdgpu_vm_wait_pd(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 	int r;
 
 	amdgpu_sync_create(&sync);
-	amdgpu_sync_resv(adev, &sync, vm->root.bo->tbo.resv, owner);
+	amdgpu_sync_resv(adev, &sync, vm->root.base.bo->tbo.resv, owner);
 	r = amdgpu_sync_wait(&sync, true);
 	amdgpu_sync_free(&sync);
 
@@ -1058,10 +1019,10 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
 
 	memset(&params, 0, sizeof(params));
 	params.adev = adev;
-	shadow = parent->bo->shadow;
+	shadow = parent->base.bo->shadow;
 
 	if (vm->use_cpu_for_update) {
-		pd_addr = (unsigned long)amdgpu_bo_kptr(parent->bo);
+		pd_addr = (unsigned long)amdgpu_bo_kptr(parent->base.bo);
 		r = amdgpu_vm_wait_pd(adev, vm, AMDGPU_FENCE_OWNER_VM);
 		if (unlikely(r))
 			return r;
@@ -1077,7 +1038,7 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
 		/* assume the worst case */
 		ndw += parent->last_entry_used * 6;
 
-		pd_addr = amdgpu_bo_gpu_offset(parent->bo);
+		pd_addr = amdgpu_bo_gpu_offset(parent->base.bo);
 
 		if (shadow) {
 			shadow_addr = amdgpu_bo_gpu_offset(shadow);
@@ -1097,7 +1058,7 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
 
 	/* walk over the address space and update the directory */
 	for (pt_idx = 0; pt_idx <= parent->last_entry_used; ++pt_idx) {
-		struct amdgpu_bo *bo = parent->entries[pt_idx].bo;
+		struct amdgpu_bo *bo = parent->entries[pt_idx].base.bo;
 		uint64_t pde, pt;
 
 		if (bo == NULL)
@@ -1140,7 +1101,7 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
 	}
 
 	if (count) {
-		if (vm->root.bo->shadow)
+		if (vm->root.base.bo->shadow)
 			params.func(&params, last_shadow, last_pt,
 				    count, incr, AMDGPU_PTE_VALID);
 
@@ -1153,7 +1114,8 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
 			amdgpu_job_free(job);
 		} else {
 			amdgpu_ring_pad_ib(ring, params.ib);
-			amdgpu_sync_resv(adev, &job->sync, parent->bo->tbo.resv,
+			amdgpu_sync_resv(adev, &job->sync,
+					 parent->base.bo->tbo.resv,
 					 AMDGPU_FENCE_OWNER_VM);
 			if (shadow)
 				amdgpu_sync_resv(adev, &job->sync,
@@ -1166,7 +1128,7 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
 			if (r)
 				goto error_free;
 
-			amdgpu_bo_fence(parent->bo, fence, true);
+			amdgpu_bo_fence(parent->base.bo, fence, true);
 			dma_fence_put(vm->last_dir_update);
 			vm->last_dir_update = dma_fence_get(fence);
 			dma_fence_put(fence);
@@ -1179,7 +1141,7 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
 	for (pt_idx = 0; pt_idx <= parent->last_entry_used; ++pt_idx) {
 		struct amdgpu_vm_pt *entry = &parent->entries[pt_idx];
 
-		if (!entry->bo)
+		if (!entry->base.bo)
 			continue;
 
 		r = amdgpu_vm_update_level(adev, vm, entry, level + 1);
@@ -1212,7 +1174,7 @@ static void amdgpu_vm_invalidate_level(struct amdgpu_vm_pt *parent)
 	for (pt_idx = 0; pt_idx <= parent->last_entry_used; ++pt_idx) {
 		struct amdgpu_vm_pt *entry = &parent->entries[pt_idx];
 
-		if (!entry->bo)
+		if (!entry->base.bo)
 			continue;
 
 		entry->addr = ~0ULL;
@@ -1267,7 +1229,7 @@ void amdgpu_vm_get_entry(struct amdgpu_pte_update_params *p, uint64_t addr,
 	*entry = &p->vm->root;
 	while ((*entry)->entries) {
 		idx = addr >> (p->adev->vm_manager.block_size * level--);
-		idx %= amdgpu_bo_size((*entry)->bo) / 8;
+		idx %= amdgpu_bo_size((*entry)->base.bo) / 8;
 		*parent = *entry;
 		*entry = &(*entry)->entries[idx];
 	}
@@ -1303,7 +1265,7 @@ static void amdgpu_vm_handle_huge_pages(struct amdgpu_pte_update_params *p,
 	    p->src ||
 	    !(flags & AMDGPU_PTE_VALID)) {
 
-		dst = amdgpu_bo_gpu_offset(entry->bo);
+		dst = amdgpu_bo_gpu_offset(entry->base.bo);
 		dst = amdgpu_gart_get_vm_pde(p->adev, dst);
 		flags = AMDGPU_PTE_VALID;
 	} else {
@@ -1329,18 +1291,18 @@ static void amdgpu_vm_handle_huge_pages(struct amdgpu_pte_update_params *p,
 		tmp = p->pages_addr;
 		p->pages_addr = NULL;
 
-		pd_addr = (unsigned long)amdgpu_bo_kptr(parent->bo);
+		pd_addr = (unsigned long)amdgpu_bo_kptr(parent->base.bo);
 		pde = pd_addr + (entry - parent->entries) * 8;
 		amdgpu_vm_cpu_set_ptes(p, pde, dst, 1, 0, flags);
 
 		p->pages_addr = tmp;
 	} else {
-		if (parent->bo->shadow) {
-			pd_addr = amdgpu_bo_gpu_offset(parent->bo->shadow);
+		if (parent->base.bo->shadow) {
+			pd_addr = amdgpu_bo_gpu_offset(parent->base.bo->shadow);
 			pde = pd_addr + (entry - parent->entries) * 8;
 			amdgpu_vm_do_set_ptes(p, pde, dst, 1, 0, flags);
 		}
-		pd_addr = amdgpu_bo_gpu_offset(parent->bo);
+		pd_addr = amdgpu_bo_gpu_offset(parent->base.bo);
 		pde = pd_addr + (entry - parent->entries) * 8;
 		amdgpu_vm_do_set_ptes(p, pde, dst, 1, 0, flags);
 	}
@@ -1391,7 +1353,7 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
 		if (entry->addr & AMDGPU_PDE_PTE)
 			continue;
 
-		pt = entry->bo;
+		pt = entry->base.bo;
 		if (use_cpu_update) {
 			pe_start = (unsigned long)amdgpu_bo_kptr(pt);
 		} else {
@@ -1611,12 +1573,12 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
 	if (r)
 		goto error_free;
 
-	r = amdgpu_sync_resv(adev, &job->sync, vm->root.bo->tbo.resv,
+	r = amdgpu_sync_resv(adev, &job->sync, vm->root.base.bo->tbo.resv,
 			     owner);
 	if (r)
 		goto error_free;
 
-	r = reservation_object_reserve_shared(vm->root.bo->tbo.resv);
+	r = reservation_object_reserve_shared(vm->root.base.bo->tbo.resv);
 	if (r)
 		goto error_free;
 
@@ -1631,7 +1593,7 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
 	if (r)
 		goto error_free;
 
-	amdgpu_bo_fence(vm->root.bo, f, true);
+	amdgpu_bo_fence(vm->root.base.bo, f, true);
 	dma_fence_put(*fence);
 	*fence = f;
 	return 0;
@@ -1926,7 +1888,7 @@ static void amdgpu_vm_free_mapping(struct amdgpu_device *adev,
  */
 static void amdgpu_vm_prt_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 {
-	struct reservation_object *resv = vm->root.bo->tbo.resv;
+	struct reservation_object *resv = vm->root.base.bo->tbo.resv;
 	struct dma_fence *excl, **shared;
 	unsigned i, shared_count;
 	int r;
@@ -2413,12 +2375,25 @@ void amdgpu_vm_bo_rmv(struct amdgpu_device *adev,
  * Mark @bo as invalid.
  */
 void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
-			     struct amdgpu_bo *bo)
+			     struct amdgpu_bo *bo, bool evicted)
 {
 	struct amdgpu_vm_bo_base *bo_base;
 
 	list_for_each_entry(bo_base, &bo->va, bo_list) {
+		struct amdgpu_vm *vm = bo_base->vm;
+
 		bo_base->moved = true;
+		if (evicted && bo->tbo.resv == vm->root.base.bo->tbo.resv) {
+			spin_lock(&bo_base->vm->status_lock);
+			list_move(&bo_base->vm_status, &vm->evicted);
+			spin_unlock(&bo_base->vm->status_lock);
+			continue;
+		}
+
+		/* Don't add page tables to the moved state */
+		if (bo->tbo.type == ttm_bo_type_kernel)
+			continue;
+
 		spin_lock(&bo_base->vm->status_lock);
 		list_move(&bo_base->vm_status, &bo_base->vm->moved);
 		spin_unlock(&bo_base->vm->status_lock);
@@ -2506,6 +2481,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 	for (i = 0; i < AMDGPU_MAX_VMHUBS; i++)
 		vm->reserved_vmid[i] = NULL;
 	spin_lock_init(&vm->status_lock);
+	INIT_LIST_HEAD(&vm->evicted);
 	INIT_LIST_HEAD(&vm->moved);
 	INIT_LIST_HEAD(&vm->freed);
 
@@ -2550,30 +2526,31 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 	r = amdgpu_bo_create(adev, amdgpu_vm_bo_size(adev, 0), align, true,
 			     AMDGPU_GEM_DOMAIN_VRAM,
 			     flags,
-			     NULL, NULL, init_pde_value, &vm->root.bo);
+			     NULL, NULL, init_pde_value, &vm->root.base.bo);
 	if (r)
 		goto error_free_sched_entity;
 
-	r = amdgpu_bo_reserve(vm->root.bo, false);
-	if (r)
-		goto error_free_root;
-
-	vm->last_eviction_counter = atomic64_read(&adev->num_evictions);
+	vm->root.base.vm = vm;
+	list_add_tail(&vm->root.base.bo_list, &vm->root.base.bo->va);
+	INIT_LIST_HEAD(&vm->root.base.vm_status);
 
 	if (vm->use_cpu_for_update) {
-		r = amdgpu_bo_kmap(vm->root.bo, NULL);
+		r = amdgpu_bo_reserve(vm->root.base.bo, false);
 		if (r)
 			goto error_free_root;
-	}
 
-	amdgpu_bo_unreserve(vm->root.bo);
+		r = amdgpu_bo_kmap(vm->root.base.bo, NULL);
+		if (r)
+			goto error_free_root;
+		amdgpu_bo_unreserve(vm->root.base.bo);
+	}
 
 	return 0;
 
 error_free_root:
-	amdgpu_bo_unref(&vm->root.bo->shadow);
-	amdgpu_bo_unref(&vm->root.bo);
-	vm->root.bo = NULL;
+	amdgpu_bo_unref(&vm->root.base.bo->shadow);
+	amdgpu_bo_unref(&vm->root.base.bo);
+	vm->root.base.bo = NULL;
 
 error_free_sched_entity:
 	amd_sched_entity_fini(&ring->sched, &vm->entity);
@@ -2592,9 +2569,11 @@ static void amdgpu_vm_free_levels(struct amdgpu_vm_pt *level)
 {
 	unsigned i;
 
-	if (level->bo) {
-		amdgpu_bo_unref(&level->bo->shadow);
-		amdgpu_bo_unref(&level->bo);
+	if (level->base.bo) {
+		list_del(&level->base.bo_list);
+		list_del(&level->base.vm_status);
+		amdgpu_bo_unref(&level->base.bo->shadow);
+		amdgpu_bo_unref(&level->base.bo);
 	}
 
 	if (level->entries)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index ff093d4..4e465e8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -111,12 +111,12 @@ struct amdgpu_vm_bo_base {
 };
 
 struct amdgpu_vm_pt {
-	struct amdgpu_bo	*bo;
-	uint64_t		addr;
+	struct amdgpu_vm_bo_base	base;
+	uint64_t			addr;
 
 	/* array of page tables, one for each directory entry */
-	struct amdgpu_vm_pt	*entries;
-	unsigned		last_entry_used;
+	struct amdgpu_vm_pt		*entries;
+	unsigned			last_entry_used;
 };
 
 struct amdgpu_vm {
@@ -126,6 +126,9 @@ struct amdgpu_vm {
 	/* protecting invalidated */
 	spinlock_t		status_lock;
 
+	/* BOs who needs a validation */
+	struct list_head	evicted;
+
 	/* BOs moved, but not yet updated in the PT */
 	struct list_head	moved;
 
@@ -135,7 +138,6 @@ struct amdgpu_vm {
 	/* contains the page directory */
 	struct amdgpu_vm_pt     root;
 	struct dma_fence	*last_dir_update;
-	uint64_t		last_eviction_counter;
 
 	/* protecting freed */
 	spinlock_t		freed_lock;
@@ -225,7 +227,7 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm);
 void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
 			 struct list_head *validated,
 			 struct amdgpu_bo_list_entry *entry);
-bool amdgpu_vm_ready(struct amdgpu_device *adev, struct amdgpu_vm *vm);
+bool amdgpu_vm_ready(struct amdgpu_vm *vm);
 int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 			      int (*callback)(void *p, struct amdgpu_bo *bo),
 			      void *param);
@@ -250,7 +252,7 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev,
 			struct amdgpu_bo_va *bo_va,
 			bool clear);
 void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
-			     struct amdgpu_bo *bo);
+			     struct amdgpu_bo *bo, bool evicted);
 struct amdgpu_bo_va *amdgpu_vm_bo_find(struct amdgpu_vm *vm,
 				       struct amdgpu_bo *bo);
 struct amdgpu_bo_va *amdgpu_vm_bo_add(struct amdgpu_device *adev,
-- 
2.7.4

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

[PATCH 5/5] drm/amdgpu: rework page directory filling v2

[Christian König]

From: Christian König <christian.koenig@amd.com>

Keep track off relocated PDs/PTs instead of walking and checking all PDs.

v2: fix root PD handling

Signed-off-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com> (v1)
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 89 +++++++++++++++++++++++-----------
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |  3 ++
 2 files changed, 63 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 592c3e7..4cdfb70 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -196,7 +196,7 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 		}
 
 		spin_lock(&vm->status_lock);
-		list_del_init(&bo_base->vm_status);
+		list_move(&bo_base->vm_status, &vm->relocated);
 	}
 	spin_unlock(&vm->status_lock);
 
@@ -313,8 +313,10 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
 			entry->base.vm = vm;
 			entry->base.bo = pt;
 			list_add_tail(&entry->base.bo_list, &pt->va);
-			INIT_LIST_HEAD(&entry->base.vm_status);
-			entry->addr = 0;
+			spin_lock(&vm->status_lock);
+			list_add(&entry->base.vm_status, &vm->relocated);
+			spin_unlock(&vm->status_lock);
+			entry->addr = ~0ULL;
 		}
 
 		if (level < adev->vm_manager.num_level) {
@@ -999,18 +1001,17 @@ static int amdgpu_vm_wait_pd(struct amdgpu_device *adev, struct amdgpu_vm *vm,
  */
 static int amdgpu_vm_update_level(struct amdgpu_device *adev,
 				  struct amdgpu_vm *vm,
-				  struct amdgpu_vm_pt *parent,
-				  unsigned level)
+				  struct amdgpu_vm_pt *parent)
 {
 	struct amdgpu_bo *shadow;
 	struct amdgpu_ring *ring = NULL;
 	uint64_t pd_addr, shadow_addr = 0;
-	uint32_t incr = amdgpu_vm_bo_size(adev, level + 1);
 	uint64_t last_pde = ~0, last_pt = ~0, last_shadow = ~0;
 	unsigned count = 0, pt_idx, ndw = 0;
 	struct amdgpu_job *job;
 	struct amdgpu_pte_update_params params;
 	struct dma_fence *fence = NULL;
+	uint32_t incr;
 
 	int r;
 
@@ -1058,12 +1059,17 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
 
 	/* walk over the address space and update the directory */
 	for (pt_idx = 0; pt_idx <= parent->last_entry_used; ++pt_idx) {
-		struct amdgpu_bo *bo = parent->entries[pt_idx].base.bo;
+		struct amdgpu_vm_pt *entry = &parent->entries[pt_idx];
+		struct amdgpu_bo *bo = entry->base.bo;
 		uint64_t pde, pt;
 
 		if (bo == NULL)
 			continue;
 
+		spin_lock(&vm->status_lock);
+		list_del_init(&entry->base.vm_status);
+		spin_unlock(&vm->status_lock);
+
 		pt = amdgpu_bo_gpu_offset(bo);
 		pt = amdgpu_gart_get_vm_pde(adev, pt);
 		/* Don't update huge pages here */
@@ -1074,6 +1080,7 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
 		parent->entries[pt_idx].addr = pt | AMDGPU_PTE_VALID;
 
 		pde = pd_addr + pt_idx * 8;
+		incr = amdgpu_bo_size(bo);
 		if (((last_pde + 8 * count) != pde) ||
 		    ((last_pt + incr * count) != pt) ||
 		    (count == AMDGPU_VM_MAX_UPDATE_SIZE)) {
@@ -1134,20 +1141,6 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
 			dma_fence_put(fence);
 		}
 	}
-	/*
-	 * Recurse into the subdirectories. This recursion is harmless because
-	 * we only have a maximum of 5 layers.
-	 */
-	for (pt_idx = 0; pt_idx <= parent->last_entry_used; ++pt_idx) {
-		struct amdgpu_vm_pt *entry = &parent->entries[pt_idx];
-
-		if (!entry->base.bo)
-			continue;
-
-		r = amdgpu_vm_update_level(adev, vm, entry, level + 1);
-		if (r)
-			return r;
-	}
 
 	return 0;
 
@@ -1163,7 +1156,8 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
  *
  * Mark all PD level as invalid after an error.
  */
-static void amdgpu_vm_invalidate_level(struct amdgpu_vm_pt *parent)
+static void amdgpu_vm_invalidate_level(struct amdgpu_vm *vm,
+				       struct amdgpu_vm_pt *parent)
 {
 	unsigned pt_idx;
 
@@ -1178,7 +1172,10 @@ static void amdgpu_vm_invalidate_level(struct amdgpu_vm_pt *parent)
 			continue;
 
 		entry->addr = ~0ULL;
-		amdgpu_vm_invalidate_level(entry);
+		spin_lock(&vm->status_lock);
+		list_move(&entry->base.vm_status, &vm->relocated);
+		spin_unlock(&vm->status_lock);
+		amdgpu_vm_invalidate_level(vm, entry);
 	}
 }
 
@@ -1196,9 +1193,38 @@ int amdgpu_vm_update_directories(struct amdgpu_device *adev,
 {
 	int r;
 
-	r = amdgpu_vm_update_level(adev, vm, &vm->root, 0);
-	if (r)
-		amdgpu_vm_invalidate_level(&vm->root);
+	spin_lock(&vm->status_lock);
+	while (!list_empty(&vm->relocated)) {
+		struct amdgpu_vm_bo_base *bo_base;
+		struct amdgpu_bo *bo;
+
+		bo_base = list_first_entry(&vm->relocated,
+					   struct amdgpu_vm_bo_base,
+					   vm_status);
+		spin_unlock(&vm->status_lock);
+
+		bo = bo_base->bo->parent;
+		if (bo) {
+			struct amdgpu_vm_bo_base *parent;
+			struct amdgpu_vm_pt *pt;
+
+			parent = list_first_entry(&bo->va,
+						  struct amdgpu_vm_bo_base,
+						  bo_list);
+			pt = container_of(parent, struct amdgpu_vm_pt, base);
+
+			r = amdgpu_vm_update_level(adev, vm, pt);
+			if (r) {
+				amdgpu_vm_invalidate_level(vm, &vm->root);
+				return r;
+			}
+			spin_lock(&vm->status_lock);
+		} else {
+			spin_lock(&vm->status_lock);
+			list_del_init(&bo_base->vm_status);
+		}
+	}
+	spin_unlock(&vm->status_lock);
 
 	if (vm->use_cpu_for_update) {
 		/* Flush HDP */
@@ -1600,7 +1626,7 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
 
 error_free:
 	amdgpu_job_free(job);
-	amdgpu_vm_invalidate_level(&vm->root);
+	amdgpu_vm_invalidate_level(vm, &vm->root);
 	return r;
 }
 
@@ -2390,9 +2416,13 @@ void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
 			continue;
 		}
 
-		/* Don't add page tables to the moved state */
-		if (bo->tbo.type == ttm_bo_type_kernel)
+		if (bo->tbo.type == ttm_bo_type_kernel) {
+			spin_lock(&bo_base->vm->status_lock);
+			if (list_empty(&bo_base->vm_status))
+				list_add(&bo_base->vm_status, &vm->relocated);
+			spin_unlock(&bo_base->vm->status_lock);
 			continue;
+		}
 
 		spin_lock(&bo_base->vm->status_lock);
 		list_move(&bo_base->vm_status, &bo_base->vm->moved);
@@ -2482,6 +2512,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 		vm->reserved_vmid[i] = NULL;
 	spin_lock_init(&vm->status_lock);
 	INIT_LIST_HEAD(&vm->evicted);
+	INIT_LIST_HEAD(&vm->relocated);
 	INIT_LIST_HEAD(&vm->moved);
 	INIT_LIST_HEAD(&vm->freed);
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 4e465e8..c3753af 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -129,6 +129,9 @@ struct amdgpu_vm {
 	/* BOs who needs a validation */
 	struct list_head	evicted;
 
+	/* PT BOs which relocated and their parent need an update */
+	struct list_head	relocated;
+
 	/* BOs moved, but not yet updated in the PT */
 	struct list_head	moved;
 
-- 
2.7.4

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

Re: [PATCH 1/5] drm/amdgpu: rework moved handling in the VM v2

[zhoucm1]

Hi Christian,

I asked you how you keep the access of base.moved is safely in last 
thread, I check it just now, it depends on the shared resv lock.

For patch itself, Reviewed-by: Chunming Zhou <david1.zhou@amd.com>

another question comes up to me, I find most of all vm lists are 
protected by the shared resv lock, whether the status lock can be 
removed as well? seems everything of vm is protected by that big resv lock.

Regards,
David Zhou

On 2017年08月29日 02:50, Christian König wrote:
> From: Christian König <christian.koenig@amd.com>
>
> Instead of using the vm_state use a separate flag to note
> that the BO was moved.
>
> v2: reorder patches to avoid temporary lockless access
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 13 ++++++++++---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |  3 +++
>   2 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index f621dba..ee53293 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1787,10 +1787,16 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev,
>   	else
>   		flags = 0x0;
>   
> -	spin_lock(&vm->status_lock);
> -	if (!list_empty(&bo_va->base.vm_status))
> +	if (!clear && bo_va->base.moved) {
> +		bo_va->base.moved = false;
>   		list_splice_init(&bo_va->valids, &bo_va->invalids);
> -	spin_unlock(&vm->status_lock);
> +
> +	} else {
> +		spin_lock(&vm->status_lock);
> +		if (!list_empty(&bo_va->base.vm_status))
> +			list_splice_init(&bo_va->valids, &bo_va->invalids);
> +		spin_unlock(&vm->status_lock);
> +	}
>   
>   	list_for_each_entry(mapping, &bo_va->invalids, list) {
>   		r = amdgpu_vm_bo_split_mapping(adev, exclusive, pages_addr, vm,
> @@ -2418,6 +2424,7 @@ void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
>   	struct amdgpu_vm_bo_base *bo_base;
>   
>   	list_for_each_entry(bo_base, &bo->va, bo_list) {
> +		bo_base->moved = true;
>   		spin_lock(&bo_base->vm->status_lock);
>   		if (list_empty(&bo_base->vm_status))
>   			list_add(&bo_base->vm_status,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 9347d28..1b478e6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -105,6 +105,9 @@ struct amdgpu_vm_bo_base {
>   
>   	/* protected by spinlock */
>   	struct list_head		vm_status;
> +
> +	/* protected by the BO being reserved */
> +	bool				moved;
>   };
>   
>   struct amdgpu_vm_pt {

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

Re: [PATCH 2/5] drm/amdgpu: add bo_va cleared flag again v2

[zhoucm1]

On 2017年08月29日 02:50, Christian König wrote:
> From: Christian König <christian.koenig@amd.com>
>
> We changed this to use an extra list a while back, but for the next
> series I need a separate flag again.
>
> v2: reorder to avoid unlocked list access
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Chunming Zhou <david1.zhou@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  3 +++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     | 35 +++++++++++-------------------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h     |  3 ---
>   3 files changed, 16 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> index a288fa6..e613ba4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> @@ -55,6 +55,9 @@ struct amdgpu_bo_va {
>   	/* mappings for this bo_va */
>   	struct list_head		invalids;
>   	struct list_head		valids;
> +
> +	/* If the mappings are cleared or filled */
> +	bool				cleared;
>   };
>   
>   struct amdgpu_bo {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index ee53293..85189f1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1791,11 +1791,8 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev,
>   		bo_va->base.moved = false;
>   		list_splice_init(&bo_va->valids, &bo_va->invalids);
>   
> -	} else {
> -		spin_lock(&vm->status_lock);
> -		if (!list_empty(&bo_va->base.vm_status))
> -			list_splice_init(&bo_va->valids, &bo_va->invalids);
> -		spin_unlock(&vm->status_lock);
> +	} else if (bo_va->cleared != clear) {
> +		list_splice_init(&bo_va->valids, &bo_va->invalids);
>   	}
>   
>   	list_for_each_entry(mapping, &bo_va->invalids, list) {
> @@ -1806,25 +1803,22 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev,
>   			return r;
>   	}
>   
> -	if (trace_amdgpu_vm_bo_mapping_enabled()) {
> -		list_for_each_entry(mapping, &bo_va->valids, list)
> -			trace_amdgpu_vm_bo_mapping(mapping);
> -
> -		list_for_each_entry(mapping, &bo_va->invalids, list)
> -			trace_amdgpu_vm_bo_mapping(mapping);
> +	if (vm->use_cpu_for_update) {
> +		/* Flush HDP */
> +		mb();
> +		amdgpu_gart_flush_gpu_tlb(adev, 0);
>   	}
>   
>   	spin_lock(&vm->status_lock);
> -	list_splice_init(&bo_va->invalids, &bo_va->valids);
>   	list_del_init(&bo_va->base.vm_status);
> -	if (clear)
> -		list_add(&bo_va->base.vm_status, &vm->cleared);
>   	spin_unlock(&vm->status_lock);
>   
> -	if (vm->use_cpu_for_update) {
> -		/* Flush HDP */
> -		mb();
> -		amdgpu_gart_flush_gpu_tlb(adev, 0);
> +	list_splice_init(&bo_va->invalids, &bo_va->valids);
> +	bo_va->cleared = clear;
> +
> +	if (trace_amdgpu_vm_bo_mapping_enabled()) {
> +		list_for_each_entry(mapping, &bo_va->valids, list)
> +			trace_amdgpu_vm_bo_mapping(mapping);
>   	}
>   
>   	return 0;
> @@ -2426,9 +2420,7 @@ void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
>   	list_for_each_entry(bo_base, &bo->va, bo_list) {
>   		bo_base->moved = true;
>   		spin_lock(&bo_base->vm->status_lock);
> -		if (list_empty(&bo_base->vm_status))
> -			list_add(&bo_base->vm_status,
> -				 &bo_base->vm->moved);
> +		list_move(&bo_base->vm_status, &bo_base->vm->moved);
>   		spin_unlock(&bo_base->vm->status_lock);
>   	}
>   }
> @@ -2515,7 +2507,6 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   		vm->reserved_vmid[i] = NULL;
>   	spin_lock_init(&vm->status_lock);
>   	INIT_LIST_HEAD(&vm->moved);
> -	INIT_LIST_HEAD(&vm->cleared);
>   	INIT_LIST_HEAD(&vm->freed);
>   
>   	/* create scheduler entity for page table updates */
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 1b478e6..ff093d4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -129,9 +129,6 @@ struct amdgpu_vm {
>   	/* BOs moved, but not yet updated in the PT */
>   	struct list_head	moved;
>   
> -	/* BOs cleared in the PT because of a move */
> -	struct list_head	cleared;
> -
>   	/* BO mappings freed, but not yet updated in the PT */
>   	struct list_head	freed;
>   

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

Re: [PATCH 3/5] drm/amdgpu: fix comment on amdgpu_bo_va

[zhoucm1]

Reviewed-by: Chunming Zhou <david1.zhou@amd.com>


On 2017年08月29日 02:50, Christian König wrote:
> From: Christian König <christian.koenig@amd.com>
>
> Except for the reference count all other members are protected
> by the VM PD being reserved.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> index e613ba4..42492e6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> @@ -49,9 +49,11 @@ struct amdgpu_bo_va {
>   	struct amdgpu_vm_bo_base	base;
>   
>   	/* protected by bo being reserved */
> -	struct dma_fence	        *last_pt_update;
>   	unsigned			ref_count;
>   
> +	/* all other members protected by the VM PD being reserved */
> +	struct dma_fence	        *last_pt_update;
> +
>   	/* mappings for this bo_va */
>   	struct list_head		invalids;
>   	struct list_head		valids;

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

Re: [PATCH 4/5] drm/amdgpu: track evicted page tables v2

[zhoucm1]

On 2017年08月29日 02:50, Christian König wrote:
> From: Christian König <christian.koenig@amd.com>
>
> Instead of validating all page tables when one was evicted,
> track which one needs a validation.
>
> v2: simplify amdgpu_vm_ready as well
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com> (v1)
Reviewed-by: Chunming Zhou <david1.zhou@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c     |   7 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c    |   8 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |   2 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     | 227 +++++++++++++----------------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h     |  16 +-
>   5 files changed, 119 insertions(+), 141 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 3f46b5a..f68ac56 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -632,9 +632,6 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>   
>   	amdgpu_cs_report_moved_bytes(p->adev, p->bytes_moved,
>   				     p->bytes_moved_vis);
> -	fpriv->vm.last_eviction_counter =
> -		atomic64_read(&p->adev->num_evictions);
> -
>   	if (p->bo_list) {
>   		struct amdgpu_bo *gds = p->bo_list->gds_obj;
>   		struct amdgpu_bo *gws = p->bo_list->gws_obj;
> @@ -826,7 +823,7 @@ static int amdgpu_bo_vm_update_pte(struct amdgpu_cs_parser *p)
>   			if (!bo)
>   				continue;
>   
> -			amdgpu_vm_bo_invalidate(adev, bo);
> +			amdgpu_vm_bo_invalidate(adev, bo, false);
>   		}
>   	}
>   
> @@ -851,7 +848,7 @@ static int amdgpu_cs_ib_vm_chunk(struct amdgpu_device *adev,
>   	}
>   
>   	if (p->job->vm) {
> -		p->job->vm_pd_addr = amdgpu_bo_gpu_offset(vm->root.bo);
> +		p->job->vm_pd_addr = amdgpu_bo_gpu_offset(vm->root.base.bo);
>   
>   		r = amdgpu_bo_vm_update_pte(p);
>   		if (r)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index ba01293..d028806 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -160,7 +160,7 @@ void amdgpu_gem_object_close(struct drm_gem_object *obj,
>   	if (bo_va && --bo_va->ref_count == 0) {
>   		amdgpu_vm_bo_rmv(adev, bo_va);
>   
> -		if (amdgpu_vm_ready(adev, vm)) {
> +		if (amdgpu_vm_ready(vm)) {
>   			struct dma_fence *fence = NULL;
>   
>   			r = amdgpu_vm_clear_freed(adev, vm, &fence);
> @@ -481,10 +481,10 @@ static void amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
>   				    struct list_head *list,
>   				    uint32_t operation)
>   {
> -	int r = -ERESTARTSYS;
> +	int r;
>   
> -	if (!amdgpu_vm_ready(adev, vm))
> -		goto error;
> +	if (!amdgpu_vm_ready(vm))
> +		return;
>   
>   	r = amdgpu_vm_update_directories(adev, vm);
>   	if (r)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 9e495da..52d0109 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -929,7 +929,7 @@ void amdgpu_bo_move_notify(struct ttm_buffer_object *bo,
>   		return;
>   
>   	abo = container_of(bo, struct amdgpu_bo, tbo);
> -	amdgpu_vm_bo_invalidate(adev, abo);
> +	amdgpu_vm_bo_invalidate(adev, abo, evict);
>   
>   	amdgpu_bo_kunmap(abo);
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 85189f1..592c3e7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -140,7 +140,7 @@ void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
>   			 struct list_head *validated,
>   			 struct amdgpu_bo_list_entry *entry)
>   {
> -	entry->robj = vm->root.bo;
> +	entry->robj = vm->root.base.bo;
>   	entry->priority = 0;
>   	entry->tv.bo = &entry->robj->tbo;
>   	entry->tv.shared = true;
> @@ -149,61 +149,6 @@ void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
>   }
>   
>   /**
> - * amdgpu_vm_validate_layer - validate a single page table level
> - *
> - * @parent: parent page table level
> - * @validate: callback to do the validation
> - * @param: parameter for the validation callback
> - *
> - * Validate the page table BOs on command submission if neccessary.
> - */
> -static int amdgpu_vm_validate_level(struct amdgpu_vm_pt *parent,
> -				    int (*validate)(void *, struct amdgpu_bo *),
> -				    void *param, bool use_cpu_for_update,
> -				    struct ttm_bo_global *glob)
> -{
> -	unsigned i;
> -	int r;
> -
> -	if (use_cpu_for_update) {
> -		r = amdgpu_bo_kmap(parent->bo, NULL);
> -		if (r)
> -			return r;
> -	}
> -
> -	if (!parent->entries)
> -		return 0;
> -
> -	for (i = 0; i <= parent->last_entry_used; ++i) {
> -		struct amdgpu_vm_pt *entry = &parent->entries[i];
> -
> -		if (!entry->bo)
> -			continue;
> -
> -		r = validate(param, entry->bo);
> -		if (r)
> -			return r;
> -
> -		spin_lock(&glob->lru_lock);
> -		ttm_bo_move_to_lru_tail(&entry->bo->tbo);
> -		if (entry->bo->shadow)
> -			ttm_bo_move_to_lru_tail(&entry->bo->shadow->tbo);
> -		spin_unlock(&glob->lru_lock);
> -
> -		/*
> -		 * Recurse into the sub directory. This is harmless because we
> -		 * have only a maximum of 5 layers.
> -		 */
> -		r = amdgpu_vm_validate_level(entry, validate, param,
> -					     use_cpu_for_update, glob);
> -		if (r)
> -			return r;
> -	}
> -
> -	return r;
> -}
> -
> -/**
>    * amdgpu_vm_validate_pt_bos - validate the page table BOs
>    *
>    * @adev: amdgpu device pointer
> @@ -217,32 +162,43 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   			      int (*validate)(void *p, struct amdgpu_bo *bo),
>   			      void *param)
>   {
> -	uint64_t num_evictions;
> +	struct ttm_bo_global *glob = adev->mman.bdev.glob;
> +	int r;
>   
> -	/* We only need to validate the page tables
> -	 * if they aren't already valid.
> -	 */
> -	num_evictions = atomic64_read(&adev->num_evictions);
> -	if (num_evictions == vm->last_eviction_counter)
> -		return 0;
> +	spin_lock(&vm->status_lock);
> +	while (!list_empty(&vm->evicted)) {
> +		struct amdgpu_vm_bo_base *bo_base;
> +		struct amdgpu_bo *bo;
>   
> -	return amdgpu_vm_validate_level(&vm->root, validate, param,
> -					vm->use_cpu_for_update,
> -					adev->mman.bdev.glob);
> -}
> +		bo_base = list_first_entry(&vm->evicted,
> +					   struct amdgpu_vm_bo_base,
> +					   vm_status);
> +		spin_unlock(&vm->status_lock);
>   
> -/**
> - * amdgpu_vm_check - helper for amdgpu_vm_ready
> - */
> -static int amdgpu_vm_check(void *param, struct amdgpu_bo *bo)
> -{
> -	/* if anything is swapped out don't swap it in here,
> -	   just abort and wait for the next CS */
> -	if (!amdgpu_bo_gpu_accessible(bo))
> -		return -ERESTARTSYS;
> +		bo = bo_base->bo;
> +		BUG_ON(!bo);
> +		if (bo->parent) {
> +			r = validate(param, bo);
> +			if (r)
> +				return r;
>   
> -	if (bo->shadow && !amdgpu_bo_gpu_accessible(bo->shadow))
> -		return -ERESTARTSYS;
> +			spin_lock(&glob->lru_lock);
> +			ttm_bo_move_to_lru_tail(&bo->tbo);
> +			if (bo->shadow)
> +				ttm_bo_move_to_lru_tail(&bo->shadow->tbo);
> +			spin_unlock(&glob->lru_lock);
> +		}
> +
> +		if (vm->use_cpu_for_update) {
> +			r = amdgpu_bo_kmap(bo, NULL);
> +			if (r)
> +				return r;
> +		}
> +
> +		spin_lock(&vm->status_lock);
> +		list_del_init(&bo_base->vm_status);
> +	}
> +	spin_unlock(&vm->status_lock);
>   
>   	return 0;
>   }
> @@ -250,17 +206,19 @@ static int amdgpu_vm_check(void *param, struct amdgpu_bo *bo)
>   /**
>    * amdgpu_vm_ready - check VM is ready for updates
>    *
> - * @adev: amdgpu device
>    * @vm: VM to check
>    *
>    * Check if all VM PDs/PTs are ready for updates
>    */
> -bool amdgpu_vm_ready(struct amdgpu_device *adev, struct amdgpu_vm *vm)
> +bool amdgpu_vm_ready(struct amdgpu_vm *vm)
>   {
> -	if (amdgpu_vm_check(NULL, vm->root.bo))
> -		return false;
> +	bool ready;
> +
> +	spin_lock(&vm->status_lock);
> +	ready = list_empty(&vm->evicted);
> +	spin_unlock(&vm->status_lock);
>   
> -	return !amdgpu_vm_validate_pt_bos(adev, vm, amdgpu_vm_check, NULL);
> +	return ready;
>   }
>   
>   /**
> @@ -325,11 +283,11 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>   
>   	/* walk over the address space and allocate the page tables */
>   	for (pt_idx = from; pt_idx <= to; ++pt_idx) {
> -		struct reservation_object *resv = vm->root.bo->tbo.resv;
> +		struct reservation_object *resv = vm->root.base.bo->tbo.resv;
>   		struct amdgpu_vm_pt *entry = &parent->entries[pt_idx];
>   		struct amdgpu_bo *pt;
>   
> -		if (!entry->bo) {
> +		if (!entry->base.bo) {
>   			r = amdgpu_bo_create(adev,
>   					     amdgpu_vm_bo_size(adev, level),
>   					     AMDGPU_GPU_PAGE_SIZE, true,
> @@ -350,9 +308,12 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>   			/* Keep a reference to the root directory to avoid
>   			* freeing them up in the wrong order.
>   			*/
> -			pt->parent = amdgpu_bo_ref(vm->root.bo);
> +			pt->parent = amdgpu_bo_ref(vm->root.base.bo);
>   
> -			entry->bo = pt;
> +			entry->base.vm = vm;
> +			entry->base.bo = pt;
> +			list_add_tail(&entry->base.bo_list, &pt->va);
> +			INIT_LIST_HEAD(&entry->base.vm_status);
>   			entry->addr = 0;
>   		}
>   
> @@ -1019,7 +980,7 @@ static int amdgpu_vm_wait_pd(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   	int r;
>   
>   	amdgpu_sync_create(&sync);
> -	amdgpu_sync_resv(adev, &sync, vm->root.bo->tbo.resv, owner);
> +	amdgpu_sync_resv(adev, &sync, vm->root.base.bo->tbo.resv, owner);
>   	r = amdgpu_sync_wait(&sync, true);
>   	amdgpu_sync_free(&sync);
>   
> @@ -1058,10 +1019,10 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
>   
>   	memset(&params, 0, sizeof(params));
>   	params.adev = adev;
> -	shadow = parent->bo->shadow;
> +	shadow = parent->base.bo->shadow;
>   
>   	if (vm->use_cpu_for_update) {
> -		pd_addr = (unsigned long)amdgpu_bo_kptr(parent->bo);
> +		pd_addr = (unsigned long)amdgpu_bo_kptr(parent->base.bo);
>   		r = amdgpu_vm_wait_pd(adev, vm, AMDGPU_FENCE_OWNER_VM);
>   		if (unlikely(r))
>   			return r;
> @@ -1077,7 +1038,7 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
>   		/* assume the worst case */
>   		ndw += parent->last_entry_used * 6;
>   
> -		pd_addr = amdgpu_bo_gpu_offset(parent->bo);
> +		pd_addr = amdgpu_bo_gpu_offset(parent->base.bo);
>   
>   		if (shadow) {
>   			shadow_addr = amdgpu_bo_gpu_offset(shadow);
> @@ -1097,7 +1058,7 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
>   
>   	/* walk over the address space and update the directory */
>   	for (pt_idx = 0; pt_idx <= parent->last_entry_used; ++pt_idx) {
> -		struct amdgpu_bo *bo = parent->entries[pt_idx].bo;
> +		struct amdgpu_bo *bo = parent->entries[pt_idx].base.bo;
>   		uint64_t pde, pt;
>   
>   		if (bo == NULL)
> @@ -1140,7 +1101,7 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
>   	}
>   
>   	if (count) {
> -		if (vm->root.bo->shadow)
> +		if (vm->root.base.bo->shadow)
>   			params.func(&params, last_shadow, last_pt,
>   				    count, incr, AMDGPU_PTE_VALID);
>   
> @@ -1153,7 +1114,8 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
>   			amdgpu_job_free(job);
>   		} else {
>   			amdgpu_ring_pad_ib(ring, params.ib);
> -			amdgpu_sync_resv(adev, &job->sync, parent->bo->tbo.resv,
> +			amdgpu_sync_resv(adev, &job->sync,
> +					 parent->base.bo->tbo.resv,
>   					 AMDGPU_FENCE_OWNER_VM);
>   			if (shadow)
>   				amdgpu_sync_resv(adev, &job->sync,
> @@ -1166,7 +1128,7 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
>   			if (r)
>   				goto error_free;
>   
> -			amdgpu_bo_fence(parent->bo, fence, true);
> +			amdgpu_bo_fence(parent->base.bo, fence, true);
>   			dma_fence_put(vm->last_dir_update);
>   			vm->last_dir_update = dma_fence_get(fence);
>   			dma_fence_put(fence);
> @@ -1179,7 +1141,7 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
>   	for (pt_idx = 0; pt_idx <= parent->last_entry_used; ++pt_idx) {
>   		struct amdgpu_vm_pt *entry = &parent->entries[pt_idx];
>   
> -		if (!entry->bo)
> +		if (!entry->base.bo)
>   			continue;
>   
>   		r = amdgpu_vm_update_level(adev, vm, entry, level + 1);
> @@ -1212,7 +1174,7 @@ static void amdgpu_vm_invalidate_level(struct amdgpu_vm_pt *parent)
>   	for (pt_idx = 0; pt_idx <= parent->last_entry_used; ++pt_idx) {
>   		struct amdgpu_vm_pt *entry = &parent->entries[pt_idx];
>   
> -		if (!entry->bo)
> +		if (!entry->base.bo)
>   			continue;
>   
>   		entry->addr = ~0ULL;
> @@ -1267,7 +1229,7 @@ void amdgpu_vm_get_entry(struct amdgpu_pte_update_params *p, uint64_t addr,
>   	*entry = &p->vm->root;
>   	while ((*entry)->entries) {
>   		idx = addr >> (p->adev->vm_manager.block_size * level--);
> -		idx %= amdgpu_bo_size((*entry)->bo) / 8;
> +		idx %= amdgpu_bo_size((*entry)->base.bo) / 8;
>   		*parent = *entry;
>   		*entry = &(*entry)->entries[idx];
>   	}
> @@ -1303,7 +1265,7 @@ static void amdgpu_vm_handle_huge_pages(struct amdgpu_pte_update_params *p,
>   	    p->src ||
>   	    !(flags & AMDGPU_PTE_VALID)) {
>   
> -		dst = amdgpu_bo_gpu_offset(entry->bo);
> +		dst = amdgpu_bo_gpu_offset(entry->base.bo);
>   		dst = amdgpu_gart_get_vm_pde(p->adev, dst);
>   		flags = AMDGPU_PTE_VALID;
>   	} else {
> @@ -1329,18 +1291,18 @@ static void amdgpu_vm_handle_huge_pages(struct amdgpu_pte_update_params *p,
>   		tmp = p->pages_addr;
>   		p->pages_addr = NULL;
>   
> -		pd_addr = (unsigned long)amdgpu_bo_kptr(parent->bo);
> +		pd_addr = (unsigned long)amdgpu_bo_kptr(parent->base.bo);
>   		pde = pd_addr + (entry - parent->entries) * 8;
>   		amdgpu_vm_cpu_set_ptes(p, pde, dst, 1, 0, flags);
>   
>   		p->pages_addr = tmp;
>   	} else {
> -		if (parent->bo->shadow) {
> -			pd_addr = amdgpu_bo_gpu_offset(parent->bo->shadow);
> +		if (parent->base.bo->shadow) {
> +			pd_addr = amdgpu_bo_gpu_offset(parent->base.bo->shadow);
>   			pde = pd_addr + (entry - parent->entries) * 8;
>   			amdgpu_vm_do_set_ptes(p, pde, dst, 1, 0, flags);
>   		}
> -		pd_addr = amdgpu_bo_gpu_offset(parent->bo);
> +		pd_addr = amdgpu_bo_gpu_offset(parent->base.bo);
>   		pde = pd_addr + (entry - parent->entries) * 8;
>   		amdgpu_vm_do_set_ptes(p, pde, dst, 1, 0, flags);
>   	}
> @@ -1391,7 +1353,7 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>   		if (entry->addr & AMDGPU_PDE_PTE)
>   			continue;
>   
> -		pt = entry->bo;
> +		pt = entry->base.bo;
>   		if (use_cpu_update) {
>   			pe_start = (unsigned long)amdgpu_bo_kptr(pt);
>   		} else {
> @@ -1611,12 +1573,12 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
>   	if (r)
>   		goto error_free;
>   
> -	r = amdgpu_sync_resv(adev, &job->sync, vm->root.bo->tbo.resv,
> +	r = amdgpu_sync_resv(adev, &job->sync, vm->root.base.bo->tbo.resv,
>   			     owner);
>   	if (r)
>   		goto error_free;
>   
> -	r = reservation_object_reserve_shared(vm->root.bo->tbo.resv);
> +	r = reservation_object_reserve_shared(vm->root.base.bo->tbo.resv);
>   	if (r)
>   		goto error_free;
>   
> @@ -1631,7 +1593,7 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
>   	if (r)
>   		goto error_free;
>   
> -	amdgpu_bo_fence(vm->root.bo, f, true);
> +	amdgpu_bo_fence(vm->root.base.bo, f, true);
>   	dma_fence_put(*fence);
>   	*fence = f;
>   	return 0;
> @@ -1926,7 +1888,7 @@ static void amdgpu_vm_free_mapping(struct amdgpu_device *adev,
>    */
>   static void amdgpu_vm_prt_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>   {
> -	struct reservation_object *resv = vm->root.bo->tbo.resv;
> +	struct reservation_object *resv = vm->root.base.bo->tbo.resv;
>   	struct dma_fence *excl, **shared;
>   	unsigned i, shared_count;
>   	int r;
> @@ -2413,12 +2375,25 @@ void amdgpu_vm_bo_rmv(struct amdgpu_device *adev,
>    * Mark @bo as invalid.
>    */
>   void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
> -			     struct amdgpu_bo *bo)
> +			     struct amdgpu_bo *bo, bool evicted)
>   {
>   	struct amdgpu_vm_bo_base *bo_base;
>   
>   	list_for_each_entry(bo_base, &bo->va, bo_list) {
> +		struct amdgpu_vm *vm = bo_base->vm;
> +
>   		bo_base->moved = true;
> +		if (evicted && bo->tbo.resv == vm->root.base.bo->tbo.resv) {
> +			spin_lock(&bo_base->vm->status_lock);
> +			list_move(&bo_base->vm_status, &vm->evicted);
> +			spin_unlock(&bo_base->vm->status_lock);
> +			continue;
> +		}
> +
> +		/* Don't add page tables to the moved state */
> +		if (bo->tbo.type == ttm_bo_type_kernel)
> +			continue;
> +
>   		spin_lock(&bo_base->vm->status_lock);
>   		list_move(&bo_base->vm_status, &bo_base->vm->moved);
>   		spin_unlock(&bo_base->vm->status_lock);
> @@ -2506,6 +2481,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   	for (i = 0; i < AMDGPU_MAX_VMHUBS; i++)
>   		vm->reserved_vmid[i] = NULL;
>   	spin_lock_init(&vm->status_lock);
> +	INIT_LIST_HEAD(&vm->evicted);
>   	INIT_LIST_HEAD(&vm->moved);
>   	INIT_LIST_HEAD(&vm->freed);
>   
> @@ -2550,30 +2526,31 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   	r = amdgpu_bo_create(adev, amdgpu_vm_bo_size(adev, 0), align, true,
>   			     AMDGPU_GEM_DOMAIN_VRAM,
>   			     flags,
> -			     NULL, NULL, init_pde_value, &vm->root.bo);
> +			     NULL, NULL, init_pde_value, &vm->root.base.bo);
>   	if (r)
>   		goto error_free_sched_entity;
>   
> -	r = amdgpu_bo_reserve(vm->root.bo, false);
> -	if (r)
> -		goto error_free_root;
> -
> -	vm->last_eviction_counter = atomic64_read(&adev->num_evictions);
> +	vm->root.base.vm = vm;
> +	list_add_tail(&vm->root.base.bo_list, &vm->root.base.bo->va);
> +	INIT_LIST_HEAD(&vm->root.base.vm_status);
>   
>   	if (vm->use_cpu_for_update) {
> -		r = amdgpu_bo_kmap(vm->root.bo, NULL);
> +		r = amdgpu_bo_reserve(vm->root.base.bo, false);
>   		if (r)
>   			goto error_free_root;
> -	}
>   
> -	amdgpu_bo_unreserve(vm->root.bo);
> +		r = amdgpu_bo_kmap(vm->root.base.bo, NULL);
> +		if (r)
> +			goto error_free_root;
> +		amdgpu_bo_unreserve(vm->root.base.bo);
> +	}
>   
>   	return 0;
>   
>   error_free_root:
> -	amdgpu_bo_unref(&vm->root.bo->shadow);
> -	amdgpu_bo_unref(&vm->root.bo);
> -	vm->root.bo = NULL;
> +	amdgpu_bo_unref(&vm->root.base.bo->shadow);
> +	amdgpu_bo_unref(&vm->root.base.bo);
> +	vm->root.base.bo = NULL;
>   
>   error_free_sched_entity:
>   	amd_sched_entity_fini(&ring->sched, &vm->entity);
> @@ -2592,9 +2569,11 @@ static void amdgpu_vm_free_levels(struct amdgpu_vm_pt *level)
>   {
>   	unsigned i;
>   
> -	if (level->bo) {
> -		amdgpu_bo_unref(&level->bo->shadow);
> -		amdgpu_bo_unref(&level->bo);
> +	if (level->base.bo) {
> +		list_del(&level->base.bo_list);
> +		list_del(&level->base.vm_status);
> +		amdgpu_bo_unref(&level->base.bo->shadow);
> +		amdgpu_bo_unref(&level->base.bo);
>   	}
>   
>   	if (level->entries)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index ff093d4..4e465e8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -111,12 +111,12 @@ struct amdgpu_vm_bo_base {
>   };
>   
>   struct amdgpu_vm_pt {
> -	struct amdgpu_bo	*bo;
> -	uint64_t		addr;
> +	struct amdgpu_vm_bo_base	base;
> +	uint64_t			addr;
>   
>   	/* array of page tables, one for each directory entry */
> -	struct amdgpu_vm_pt	*entries;
> -	unsigned		last_entry_used;
> +	struct amdgpu_vm_pt		*entries;
> +	unsigned			last_entry_used;
>   };
>   
>   struct amdgpu_vm {
> @@ -126,6 +126,9 @@ struct amdgpu_vm {
>   	/* protecting invalidated */
>   	spinlock_t		status_lock;
>   
> +	/* BOs who needs a validation */
> +	struct list_head	evicted;
> +
>   	/* BOs moved, but not yet updated in the PT */
>   	struct list_head	moved;
>   
> @@ -135,7 +138,6 @@ struct amdgpu_vm {
>   	/* contains the page directory */
>   	struct amdgpu_vm_pt     root;
>   	struct dma_fence	*last_dir_update;
> -	uint64_t		last_eviction_counter;
>   
>   	/* protecting freed */
>   	spinlock_t		freed_lock;
> @@ -225,7 +227,7 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm);
>   void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
>   			 struct list_head *validated,
>   			 struct amdgpu_bo_list_entry *entry);
> -bool amdgpu_vm_ready(struct amdgpu_device *adev, struct amdgpu_vm *vm);
> +bool amdgpu_vm_ready(struct amdgpu_vm *vm);
>   int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   			      int (*callback)(void *p, struct amdgpu_bo *bo),
>   			      void *param);
> @@ -250,7 +252,7 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev,
>   			struct amdgpu_bo_va *bo_va,
>   			bool clear);
>   void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
> -			     struct amdgpu_bo *bo);
> +			     struct amdgpu_bo *bo, bool evicted);
>   struct amdgpu_bo_va *amdgpu_vm_bo_find(struct amdgpu_vm *vm,
>   				       struct amdgpu_bo *bo);
>   struct amdgpu_bo_va *amdgpu_vm_bo_add(struct amdgpu_device *adev,

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

Re: [PATCH 5/5] drm/amdgpu: rework page directory filling v2

[zhoucm1]

On 2017年08月29日 02:50, Christian König wrote:
> From: Christian König <christian.koenig@amd.com>
>
> Keep track off relocated PDs/PTs instead of walking and checking all PDs.
>
> v2: fix root PD handling
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com> (v1)
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 89 +++++++++++++++++++++++-----------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |  3 ++
>   2 files changed, 63 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 592c3e7..4cdfb70 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -196,7 +196,7 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   		}
>   
>   		spin_lock(&vm->status_lock);
> -		list_del_init(&bo_base->vm_status);
> +		list_move(&bo_base->vm_status, &vm->relocated);
>   	}
>   	spin_unlock(&vm->status_lock);
>   
> @@ -313,8 +313,10 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>   			entry->base.vm = vm;
>   			entry->base.bo = pt;
>   			list_add_tail(&entry->base.bo_list, &pt->va);
> -			INIT_LIST_HEAD(&entry->base.vm_status);
> -			entry->addr = 0;
> +			spin_lock(&vm->status_lock);
> +			list_add(&entry->base.vm_status, &vm->relocated);
> +			spin_unlock(&vm->status_lock);
> +			entry->addr = ~0ULL;
>   		}
>   
>   		if (level < adev->vm_manager.num_level) {
> @@ -999,18 +1001,17 @@ static int amdgpu_vm_wait_pd(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>    */
>   static int amdgpu_vm_update_level(struct amdgpu_device *adev,
>   				  struct amdgpu_vm *vm,
> -				  struct amdgpu_vm_pt *parent,
> -				  unsigned level)
> +				  struct amdgpu_vm_pt *parent)
>   {
>   	struct amdgpu_bo *shadow;
>   	struct amdgpu_ring *ring = NULL;
>   	uint64_t pd_addr, shadow_addr = 0;
> -	uint32_t incr = amdgpu_vm_bo_size(adev, level + 1);
>   	uint64_t last_pde = ~0, last_pt = ~0, last_shadow = ~0;
>   	unsigned count = 0, pt_idx, ndw = 0;
>   	struct amdgpu_job *job;
>   	struct amdgpu_pte_update_params params;
>   	struct dma_fence *fence = NULL;
> +	uint32_t incr;
>   
>   	int r;
>   
> @@ -1058,12 +1059,17 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
>   
>   	/* walk over the address space and update the directory */
>   	for (pt_idx = 0; pt_idx <= parent->last_entry_used; ++pt_idx) {
> -		struct amdgpu_bo *bo = parent->entries[pt_idx].base.bo;
> +		struct amdgpu_vm_pt *entry = &parent->entries[pt_idx];
> +		struct amdgpu_bo *bo = entry->base.bo;
>   		uint64_t pde, pt;
>   
>   		if (bo == NULL)
>   			continue;
>   
> +		spin_lock(&vm->status_lock);
> +		list_del_init(&entry->base.vm_status);
> +		spin_unlock(&vm->status_lock);
> +
>   		pt = amdgpu_bo_gpu_offset(bo);
>   		pt = amdgpu_gart_get_vm_pde(adev, pt);
>   		/* Don't update huge pages here */
> @@ -1074,6 +1080,7 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
>   		parent->entries[pt_idx].addr = pt | AMDGPU_PTE_VALID;
>   
>   		pde = pd_addr + pt_idx * 8;
> +		incr = amdgpu_bo_size(bo);
>   		if (((last_pde + 8 * count) != pde) ||
>   		    ((last_pt + incr * count) != pt) ||
>   		    (count == AMDGPU_VM_MAX_UPDATE_SIZE)) {
> @@ -1134,20 +1141,6 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
>   			dma_fence_put(fence);
>   		}
>   	}
> -	/*
> -	 * Recurse into the subdirectories. This recursion is harmless because
> -	 * we only have a maximum of 5 layers.
> -	 */
> -	for (pt_idx = 0; pt_idx <= parent->last_entry_used; ++pt_idx) {
> -		struct amdgpu_vm_pt *entry = &parent->entries[pt_idx];
> -
> -		if (!entry->base.bo)
> -			continue;
> -
> -		r = amdgpu_vm_update_level(adev, vm, entry, level + 1);
> -		if (r)
> -			return r;
> -	}
>   
>   	return 0;
>   
> @@ -1163,7 +1156,8 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
>    *
>    * Mark all PD level as invalid after an error.
>    */
> -static void amdgpu_vm_invalidate_level(struct amdgpu_vm_pt *parent)
> +static void amdgpu_vm_invalidate_level(struct amdgpu_vm *vm,
> +				       struct amdgpu_vm_pt *parent)
>   {
>   	unsigned pt_idx;
>   
> @@ -1178,7 +1172,10 @@ static void amdgpu_vm_invalidate_level(struct amdgpu_vm_pt *parent)
>   			continue;
>   
>   		entry->addr = ~0ULL;
> -		amdgpu_vm_invalidate_level(entry);
> +		spin_lock(&vm->status_lock);
> +		list_move(&entry->base.vm_status, &vm->relocated);
> +		spin_unlock(&vm->status_lock);
> +		amdgpu_vm_invalidate_level(vm, entry);
>   	}
>   }
>   
> @@ -1196,9 +1193,38 @@ int amdgpu_vm_update_directories(struct amdgpu_device *adev,
>   {
>   	int r;
>   
> -	r = amdgpu_vm_update_level(adev, vm, &vm->root, 0);
> -	if (r)
> -		amdgpu_vm_invalidate_level(&vm->root);
> +	spin_lock(&vm->status_lock);
> +	while (!list_empty(&vm->relocated)) {
There are many implicit dependency for calling sequence, like reloclated 
list member is from evicted list after validating,
so seem we'd better add a WARN for if evicted list is empty.
But consider this code path is critical, alternative way is ok to me.

Reviewed-by: Chunming Zhou <david1.zhou@amd.com>

> +		struct amdgpu_vm_bo_base *bo_base;
> +		struct amdgpu_bo *bo;
> +
> +		bo_base = list_first_entry(&vm->relocated,
> +					   struct amdgpu_vm_bo_base,
> +					   vm_status);
> +		spin_unlock(&vm->status_lock);
> +
> +		bo = bo_base->bo->parent;
> +		if (bo) {
> +			struct amdgpu_vm_bo_base *parent;
> +			struct amdgpu_vm_pt *pt;
> +
> +			parent = list_first_entry(&bo->va,
> +						  struct amdgpu_vm_bo_base,
> +						  bo_list);
> +			pt = container_of(parent, struct amdgpu_vm_pt, base);
> +
> +			r = amdgpu_vm_update_level(adev, vm, pt);
> +			if (r) {
> +				amdgpu_vm_invalidate_level(vm, &vm->root);
> +				return r;
> +			}
> +			spin_lock(&vm->status_lock);
> +		} else {
> +			spin_lock(&vm->status_lock);
> +			list_del_init(&bo_base->vm_status);
> +		}
> +	}
> +	spin_unlock(&vm->status_lock);
>   
>   	if (vm->use_cpu_for_update) {
>   		/* Flush HDP */
> @@ -1600,7 +1626,7 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
>   
>   error_free:
>   	amdgpu_job_free(job);
> -	amdgpu_vm_invalidate_level(&vm->root);
> +	amdgpu_vm_invalidate_level(vm, &vm->root);
>   	return r;
>   }
>   
> @@ -2390,9 +2416,13 @@ void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
>   			continue;
>   		}
>   
> -		/* Don't add page tables to the moved state */
> -		if (bo->tbo.type == ttm_bo_type_kernel)
> +		if (bo->tbo.type == ttm_bo_type_kernel) {
> +			spin_lock(&bo_base->vm->status_lock);
> +			if (list_empty(&bo_base->vm_status))
> +				list_add(&bo_base->vm_status, &vm->relocated);
> +			spin_unlock(&bo_base->vm->status_lock);
>   			continue;
> +		}
>   
>   		spin_lock(&bo_base->vm->status_lock);
>   		list_move(&bo_base->vm_status, &bo_base->vm->moved);
> @@ -2482,6 +2512,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   		vm->reserved_vmid[i] = NULL;
>   	spin_lock_init(&vm->status_lock);
>   	INIT_LIST_HEAD(&vm->evicted);
> +	INIT_LIST_HEAD(&vm->relocated);
>   	INIT_LIST_HEAD(&vm->moved);
>   	INIT_LIST_HEAD(&vm->freed);
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 4e465e8..c3753af 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -129,6 +129,9 @@ struct amdgpu_vm {
>   	/* BOs who needs a validation */
>   	struct list_head	evicted;
>   
> +	/* PT BOs which relocated and their parent need an update */
> +	struct list_head	relocated;
> +
>   	/* BOs moved, but not yet updated in the PT */
>   	struct list_head	moved;
>   

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

Re: [PATCH 1/5] drm/amdgpu: rework moved handling in the VM v2

[Christian König]

> I asked you how you keep the access of base.moved is safely in last 
> thread,
Sorry, I've missed that question somehow.

> I check it just now, it depends on the shared resv lock.
Actually that's not 100% correct. The moved member is protected by the 
BOs resv lock.

That can be the shared one (in the case of PDs/PTs/local BOs), but can 
also be a separate lock.

> I find most of all vm lists are protected by the shared resv lock, 
> whether the status lock can be removed as well? seems everything of vm 
> is protected by that big resv lock. 
Nope, that won't work. When amdgpu_vm_bo_invalidate() is called only the 
BO itself is locked, but not something from the VM.

So we need a separate lock to protect this list.

Thanks for the review,
Christian.

Am 29.08.2017 um 04:13 schrieb zhoucm1:
> Hi Christian,
>
> I asked you how you keep the access of base.moved is safely in last 
> thread, I check it just now, it depends on the shared resv lock.
>
> For patch itself, Reviewed-by: Chunming Zhou <david1.zhou@amd.com>
>
> another question comes up to me, I find most of all vm lists are 
> protected by the shared resv lock, whether the status lock can be 
> removed as well? seems everything of vm is protected by that big resv 
> lock.
>
> Regards,
> David Zhou
>
> On 2017年08月29日 02:50, Christian König wrote:
>> From: Christian König <christian.koenig@amd.com>
>>
>> Instead of using the vm_state use a separate flag to note
>> that the BO was moved.
>>
>> v2: reorder patches to avoid temporary lockless access
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 13 ++++++++++---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |  3 +++
>>   2 files changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index f621dba..ee53293 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -1787,10 +1787,16 @@ int amdgpu_vm_bo_update(struct amdgpu_device 
>> *adev,
>>       else
>>           flags = 0x0;
>>   -    spin_lock(&vm->status_lock);
>> -    if (!list_empty(&bo_va->base.vm_status))
>> +    if (!clear && bo_va->base.moved) {
>> +        bo_va->base.moved = false;
>>           list_splice_init(&bo_va->valids, &bo_va->invalids);
>> -    spin_unlock(&vm->status_lock);
>> +
>> +    } else {
>> +        spin_lock(&vm->status_lock);
>> +        if (!list_empty(&bo_va->base.vm_status))
>> +            list_splice_init(&bo_va->valids, &bo_va->invalids);
>> +        spin_unlock(&vm->status_lock);
>> +    }
>>         list_for_each_entry(mapping, &bo_va->invalids, list) {
>>           r = amdgpu_vm_bo_split_mapping(adev, exclusive, pages_addr, 
>> vm,
>> @@ -2418,6 +2424,7 @@ void amdgpu_vm_bo_invalidate(struct 
>> amdgpu_device *adev,
>>       struct amdgpu_vm_bo_base *bo_base;
>>         list_for_each_entry(bo_base, &bo->va, bo_list) {
>> +        bo_base->moved = true;
>>           spin_lock(&bo_base->vm->status_lock);
>>           if (list_empty(&bo_base->vm_status))
>>               list_add(&bo_base->vm_status,
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> index 9347d28..1b478e6 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> @@ -105,6 +105,9 @@ struct amdgpu_vm_bo_base {
>>         /* protected by spinlock */
>>       struct list_head        vm_status;
>> +
>> +    /* protected by the BO being reserved */
>> +    bool                moved;
>>   };
>>     struct amdgpu_vm_pt {
>
> _______________________________________________
> 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