[PATCH 1/3] drm/amdgpu: move size calculations to the front of the file again

[PATCH 1/3] drm/amdgpu: move size calculations to the front of the file again

[Christian König]

amdgpu_vm_bo_* functions should come much later.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index d59222fb5931..a9275a99d793 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -133,51 +133,6 @@ struct amdgpu_prt_cb {
 	struct dma_fence_cb cb;
 };
 
-/**
- * amdgpu_vm_bo_base_init - Adds bo to the list of bos associated with the vm
- *
- * @base: base structure for tracking BO usage in a VM
- * @vm: vm to which bo is to be added
- * @bo: amdgpu buffer object
- *
- * Initialize a bo_va_base structure and add it to the appropriate lists
- *
- */
-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;
-
-	vm->bulk_moveable = false;
-	if (bo->tbo.type == ttm_bo_type_kernel)
-		list_move(&base->vm_status, &vm->relocated);
-	else
-		list_move(&base->vm_status, &vm->idle);
-
-	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.
-	 * */
-	list_move_tail(&base->vm_status, &vm->evicted);
-	base->moved = true;
-}
-
 /**
  * amdgpu_vm_level_shift - return the addr shift for each level
  *
@@ -249,6 +204,51 @@ static unsigned amdgpu_vm_bo_size(struct amdgpu_device *adev, unsigned level)
 	return AMDGPU_GPU_PAGE_ALIGN(amdgpu_vm_num_entries(adev, level) * 8);
 }
 
+/**
+ * amdgpu_vm_bo_base_init - Adds bo to the list of bos associated with the vm
+ *
+ * @base: base structure for tracking BO usage in a VM
+ * @vm: vm to which bo is to be added
+ * @bo: amdgpu buffer object
+ *
+ * Initialize a bo_va_base structure and add it to the appropriate lists
+ *
+ */
+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;
+
+	vm->bulk_moveable = false;
+	if (bo->tbo.type == ttm_bo_type_kernel)
+		list_move(&base->vm_status, &vm->relocated);
+	else
+		list_move(&base->vm_status, &vm->idle);
+
+	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.
+	 * */
+	list_move_tail(&base->vm_status, &vm->evicted);
+	base->moved = true;
+}
+
 /**
  * amdgpu_vm_get_pd_bo - add the VM PD to a validation list
  *
-- 
2.14.1

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

[PATCH 2/3] drm/amdgpu: separate per VM BOs from normal in the moved state

[Christian König]

Allows us to avoid taking the spinlock in more places.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index a9275a99d793..65977e7c94dc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -342,9 +342,7 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 			break;
 
 		if (bo->tbo.type != ttm_bo_type_kernel) {
-			spin_lock(&vm->moved_lock);
 			list_move(&bo_base->vm_status, &vm->moved);
-			spin_unlock(&vm->moved_lock);
 		} else {
 			if (vm->use_cpu_for_update)
 				r = amdgpu_bo_kmap(bo, NULL);
@@ -1734,10 +1732,6 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev,
 		amdgpu_asic_flush_hdp(adev, NULL);
 	}
 
-	spin_lock(&vm->moved_lock);
-	list_del_init(&bo_va->base.vm_status);
-	spin_unlock(&vm->moved_lock);
-
 	/* If the BO is not in its preferred location add it back to
 	 * the evicted list so that it gets validated again on the
 	 * next command submission.
@@ -1746,9 +1740,13 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev,
 		uint32_t mem_type = bo->tbo.mem.mem_type;
 
 		if (!(bo->preferred_domains & amdgpu_mem_type_to_domain(mem_type)))
-			list_add_tail(&bo_va->base.vm_status, &vm->evicted);
+			list_move_tail(&bo_va->base.vm_status, &vm->evicted);
 		else
-			list_add(&bo_va->base.vm_status, &vm->idle);
+			list_move(&bo_va->base.vm_status, &vm->idle);
+	} else {
+		spin_lock(&vm->invalidated_lock);
+		list_del_init(&bo_va->base.vm_status);
+		spin_unlock(&vm->invalidated_lock);
 	}
 
 	list_splice_init(&bo_va->invalids, &bo_va->valids);
@@ -1974,40 +1972,40 @@ int amdgpu_vm_handle_moved(struct amdgpu_device *adev,
 			   struct amdgpu_vm *vm)
 {
 	struct amdgpu_bo_va *bo_va, *tmp;
-	struct list_head moved;
+	struct reservation_object *resv;
 	bool clear;
 	int r;
 
-	INIT_LIST_HEAD(&moved);
-	spin_lock(&vm->moved_lock);
-	list_splice_init(&vm->moved, &moved);
-	spin_unlock(&vm->moved_lock);
+	list_for_each_entry_safe(bo_va, tmp, &vm->moved, base.vm_status) {
+		/* Per VM BOs never need to bo cleared in the page tables */
+		r = amdgpu_vm_bo_update(adev, bo_va, false);
+		if (r)
+			return r;
+	}
 
-	list_for_each_entry_safe(bo_va, tmp, &moved, base.vm_status) {
-		struct reservation_object *resv = bo_va->base.bo->tbo.resv;
+	spin_lock(&vm->invalidated_lock);
+	while (!list_empty(&vm->invalidated)) {
+		bo_va = list_first_entry(&vm->invalidated, struct amdgpu_bo_va,
+					 base.vm_status);
+		resv = bo_va->base.bo->tbo.resv;
+		spin_unlock(&vm->invalidated_lock);
 
-		/* Per VM BOs never need to bo cleared in the page tables */
-		if (resv == vm->root.base.bo->tbo.resv)
-			clear = false;
 		/* Try to reserve the BO to avoid clearing its ptes */
-		else if (!amdgpu_vm_debug && reservation_object_trylock(resv))
+		if (!amdgpu_vm_debug && reservation_object_trylock(resv))
 			clear = false;
 		/* Somebody else is using the BO right now */
 		else
 			clear = true;
 
 		r = amdgpu_vm_bo_update(adev, bo_va, clear);
-		if (r) {
-			spin_lock(&vm->moved_lock);
-			list_splice(&moved, &vm->moved);
-			spin_unlock(&vm->moved_lock);
+		if (r)
 			return r;
-		}
 
-		if (!clear && resv != vm->root.base.bo->tbo.resv)
+		if (!clear)
 			reservation_object_unlock(resv);
-
+		spin_lock(&vm->invalidated_lock);
 	}
+	spin_unlock(&vm->invalidated_lock);
 
 	return 0;
 }
@@ -2072,9 +2070,7 @@ static void amdgpu_vm_bo_insert_map(struct amdgpu_device *adev,
 
 	if (bo && bo->tbo.resv == vm->root.base.bo->tbo.resv &&
 	    !bo_va->base.moved) {
-		spin_lock(&vm->moved_lock);
 		list_move(&bo_va->base.vm_status, &vm->moved);
-		spin_unlock(&vm->moved_lock);
 	}
 	trace_amdgpu_vm_bo_map(bo_va, mapping);
 }
@@ -2430,9 +2426,9 @@ void amdgpu_vm_bo_rmv(struct amdgpu_device *adev,
 
 	list_del(&bo_va->base.bo_list);
 
-	spin_lock(&vm->moved_lock);
+	spin_lock(&vm->invalidated_lock);
 	list_del(&bo_va->base.vm_status);
-	spin_unlock(&vm->moved_lock);
+	spin_unlock(&vm->invalidated_lock);
 
 	list_for_each_entry_safe(mapping, next, &bo_va->valids, list) {
 		list_del(&mapping->list);
@@ -2489,10 +2485,12 @@ void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
 
 		if (bo->tbo.type == ttm_bo_type_kernel) {
 			list_move(&bo_base->vm_status, &vm->relocated);
-		} else {
-			spin_lock(&bo_base->vm->moved_lock);
+		} else if (bo->tbo.resv == vm->root.base.bo->tbo.resv) {
 			list_move(&bo_base->vm_status, &vm->moved);
-			spin_unlock(&bo_base->vm->moved_lock);
+		} else {
+			spin_lock(&vm->invalidated_lock);
+			list_move(&bo_base->vm_status, &vm->invalidated);
+			spin_unlock(&vm->invalidated_lock);
 		}
 	}
 }
@@ -2637,9 +2635,10 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 		vm->reserved_vmid[i] = NULL;
 	INIT_LIST_HEAD(&vm->evicted);
 	INIT_LIST_HEAD(&vm->relocated);
-	spin_lock_init(&vm->moved_lock);
 	INIT_LIST_HEAD(&vm->moved);
 	INIT_LIST_HEAD(&vm->idle);
+	INIT_LIST_HEAD(&vm->invalidated);
+	spin_lock_init(&vm->invalidated_lock);
 	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 62116fa44718..6ea162ca296a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -201,13 +201,16 @@ struct amdgpu_vm {
 	/* PT BOs which relocated and their parent need an update */
 	struct list_head	relocated;
 
-	/* BOs moved, but not yet updated in the PT */
+	/* per VM BOs moved, but not yet updated in the PT */
 	struct list_head	moved;
-	spinlock_t		moved_lock;
 
 	/* All BOs of this VM not currently in the state machine */
 	struct list_head	idle;
 
+	/* regular invalidated BOs, but not yet updated in the PT */
+	struct list_head	invalidated;
+	spinlock_t		invalidated_lock;
+
 	/* BO mappings freed, but not yet updated in the PT */
 	struct list_head	freed;
 
-- 
2.14.1

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

Re: [PATCH 2/3] drm/amdgpu: separate per VM BOs from normal in the moved state

[Zhang, Jerry (Junwei)]

On 09/02/2018 02:05 AM, Christian König wrote:
> Allows us to avoid taking the spinlock in more places.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Junwei Zhang <Jerry.Zhang@amd.com>

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 67 +++++++++++++++++-----------------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |  7 +++-
>   2 files changed, 38 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index a9275a99d793..65977e7c94dc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -342,9 +342,7 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   			break;
>
>   		if (bo->tbo.type != ttm_bo_type_kernel) {
> -			spin_lock(&vm->moved_lock);
>   			list_move(&bo_base->vm_status, &vm->moved);
> -			spin_unlock(&vm->moved_lock);
>   		} else {
>   			if (vm->use_cpu_for_update)
>   				r = amdgpu_bo_kmap(bo, NULL);
> @@ -1734,10 +1732,6 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev,
>   		amdgpu_asic_flush_hdp(adev, NULL);
>   	}
>
> -	spin_lock(&vm->moved_lock);
> -	list_del_init(&bo_va->base.vm_status);
> -	spin_unlock(&vm->moved_lock);
> -
>   	/* If the BO is not in its preferred location add it back to
>   	 * the evicted list so that it gets validated again on the
>   	 * next command submission.
> @@ -1746,9 +1740,13 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev,
>   		uint32_t mem_type = bo->tbo.mem.mem_type;
>
>   		if (!(bo->preferred_domains & amdgpu_mem_type_to_domain(mem_type)))
> -			list_add_tail(&bo_va->base.vm_status, &vm->evicted);
> +			list_move_tail(&bo_va->base.vm_status, &vm->evicted);
>   		else
> -			list_add(&bo_va->base.vm_status, &vm->idle);
> +			list_move(&bo_va->base.vm_status, &vm->idle);
> +	} else {
> +		spin_lock(&vm->invalidated_lock);
> +		list_del_init(&bo_va->base.vm_status);
> +		spin_unlock(&vm->invalidated_lock);
>   	}
>
>   	list_splice_init(&bo_va->invalids, &bo_va->valids);
> @@ -1974,40 +1972,40 @@ int amdgpu_vm_handle_moved(struct amdgpu_device *adev,
>   			   struct amdgpu_vm *vm)
>   {
>   	struct amdgpu_bo_va *bo_va, *tmp;
> -	struct list_head moved;
> +	struct reservation_object *resv;
>   	bool clear;
>   	int r;
>
> -	INIT_LIST_HEAD(&moved);
> -	spin_lock(&vm->moved_lock);
> -	list_splice_init(&vm->moved, &moved);
> -	spin_unlock(&vm->moved_lock);
> +	list_for_each_entry_safe(bo_va, tmp, &vm->moved, base.vm_status) {
> +		/* Per VM BOs never need to bo cleared in the page tables */
> +		r = amdgpu_vm_bo_update(adev, bo_va, false);
> +		if (r)
> +			return r;
> +	}
>
> -	list_for_each_entry_safe(bo_va, tmp, &moved, base.vm_status) {
> -		struct reservation_object *resv = bo_va->base.bo->tbo.resv;
> +	spin_lock(&vm->invalidated_lock);
> +	while (!list_empty(&vm->invalidated)) {
> +		bo_va = list_first_entry(&vm->invalidated, struct amdgpu_bo_va,
> +					 base.vm_status);
> +		resv = bo_va->base.bo->tbo.resv;
> +		spin_unlock(&vm->invalidated_lock);
>
> -		/* Per VM BOs never need to bo cleared in the page tables */
> -		if (resv == vm->root.base.bo->tbo.resv)
> -			clear = false;
>   		/* Try to reserve the BO to avoid clearing its ptes */
> -		else if (!amdgpu_vm_debug && reservation_object_trylock(resv))
> +		if (!amdgpu_vm_debug && reservation_object_trylock(resv))
>   			clear = false;
>   		/* Somebody else is using the BO right now */
>   		else
>   			clear = true;
>
>   		r = amdgpu_vm_bo_update(adev, bo_va, clear);
> -		if (r) {
> -			spin_lock(&vm->moved_lock);
> -			list_splice(&moved, &vm->moved);
> -			spin_unlock(&vm->moved_lock);
> +		if (r)
>   			return r;
> -		}
>
> -		if (!clear && resv != vm->root.base.bo->tbo.resv)
> +		if (!clear)
>   			reservation_object_unlock(resv);
> -
> +		spin_lock(&vm->invalidated_lock);
>   	}
> +	spin_unlock(&vm->invalidated_lock);
>
>   	return 0;
>   }
> @@ -2072,9 +2070,7 @@ static void amdgpu_vm_bo_insert_map(struct amdgpu_device *adev,
>
>   	if (bo && bo->tbo.resv == vm->root.base.bo->tbo.resv &&
>   	    !bo_va->base.moved) {
> -		spin_lock(&vm->moved_lock);
>   		list_move(&bo_va->base.vm_status, &vm->moved);
> -		spin_unlock(&vm->moved_lock);
>   	}
>   	trace_amdgpu_vm_bo_map(bo_va, mapping);
>   }
> @@ -2430,9 +2426,9 @@ void amdgpu_vm_bo_rmv(struct amdgpu_device *adev,
>
>   	list_del(&bo_va->base.bo_list);
>
> -	spin_lock(&vm->moved_lock);
> +	spin_lock(&vm->invalidated_lock);
>   	list_del(&bo_va->base.vm_status);
> -	spin_unlock(&vm->moved_lock);
> +	spin_unlock(&vm->invalidated_lock);
>
>   	list_for_each_entry_safe(mapping, next, &bo_va->valids, list) {
>   		list_del(&mapping->list);
> @@ -2489,10 +2485,12 @@ void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
>
>   		if (bo->tbo.type == ttm_bo_type_kernel) {
>   			list_move(&bo_base->vm_status, &vm->relocated);
> -		} else {
> -			spin_lock(&bo_base->vm->moved_lock);
> +		} else if (bo->tbo.resv == vm->root.base.bo->tbo.resv) {
>   			list_move(&bo_base->vm_status, &vm->moved);
> -			spin_unlock(&bo_base->vm->moved_lock);
> +		} else {
> +			spin_lock(&vm->invalidated_lock);
> +			list_move(&bo_base->vm_status, &vm->invalidated);
> +			spin_unlock(&vm->invalidated_lock);
>   		}
>   	}
>   }
> @@ -2637,9 +2635,10 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   		vm->reserved_vmid[i] = NULL;
>   	INIT_LIST_HEAD(&vm->evicted);
>   	INIT_LIST_HEAD(&vm->relocated);
> -	spin_lock_init(&vm->moved_lock);
>   	INIT_LIST_HEAD(&vm->moved);
>   	INIT_LIST_HEAD(&vm->idle);
> +	INIT_LIST_HEAD(&vm->invalidated);
> +	spin_lock_init(&vm->invalidated_lock);
>   	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 62116fa44718..6ea162ca296a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -201,13 +201,16 @@ struct amdgpu_vm {
>   	/* PT BOs which relocated and their parent need an update */
>   	struct list_head	relocated;
>
> -	/* BOs moved, but not yet updated in the PT */
> +	/* per VM BOs moved, but not yet updated in the PT */
>   	struct list_head	moved;
> -	spinlock_t		moved_lock;
>
>   	/* All BOs of this VM not currently in the state machine */
>   	struct list_head	idle;
>
> +	/* regular invalidated BOs, but not yet updated in the PT */
> +	struct list_head	invalidated;
> +	spinlock_t		invalidated_lock;
> +
>   	/* 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

[PATCH 3/3] drm/amdgpu: improve VM state machine documentation

[Christian König]

Since we have a lot of FAQ on the VM state machine try to improve the
documentation by adding functions for each state move.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 65977e7c94dc..ce252ead2ee4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -204,6 +204,95 @@ static unsigned amdgpu_vm_bo_size(struct amdgpu_device *adev, unsigned level)
 	return AMDGPU_GPU_PAGE_ALIGN(amdgpu_vm_num_entries(adev, level) * 8);
 }
 
+/**
+ * amdgpu_vm_bo_evicted - vm_bo is evicted
+ *
+ * @vm_bo: vm_bo which is evicted
+ *
+ * State for PDs/PTs and per VM BOs which are not at the location they should
+ * be.
+ */
+static void amdgpu_vm_bo_evicted(struct amdgpu_vm_bo_base *vm_bo)
+{
+	struct amdgpu_vm *vm = vm_bo->vm;
+	struct amdgpu_bo *bo = vm_bo->bo;
+
+	vm_bo->moved = true;
+	if (bo->tbo.type == ttm_bo_type_kernel)
+		list_move(&vm_bo->vm_status, &vm->evicted);
+	else
+		list_move_tail(&vm_bo->vm_status, &vm->evicted);
+}
+
+/**
+ * amdgpu_vm_bo_relocated - vm_bo is reloacted
+ *
+ * @vm_bo: vm_bo which is relocated
+ *
+ * State for PDs/PTs which needs to update their parent PD.
+ */
+static void amdgpu_vm_bo_relocated(struct amdgpu_vm_bo_base *vm_bo)
+{
+	list_move(&vm_bo->vm_status, &vm_bo->vm->relocated);
+}
+
+/**
+ * amdgpu_vm_bo_moved - vm_bo is moved
+ *
+ * @vm_bo: vm_bo which is moved
+ *
+ * State for per VM BOs which are moved, but that change is not yet reflected
+ * in the page tables.
+ */
+static void amdgpu_vm_bo_moved(struct amdgpu_vm_bo_base *vm_bo)
+{
+	list_move(&vm_bo->vm_status, &vm_bo->vm->moved);
+}
+
+/**
+ * amdgpu_vm_bo_idle - vm_bo is idle
+ *
+ * @vm_bo: vm_bo which is now idle
+ *
+ * State for PDs/PTs and per VM BOs which have gone through the state machine
+ * and are now idle.
+ */
+static void amdgpu_vm_bo_idle(struct amdgpu_vm_bo_base *vm_bo)
+{
+	list_move(&vm_bo->vm_status, &vm_bo->vm->idle);
+	vm_bo->moved = false;
+}
+
+/**
+ * amdgpu_vm_bo_invalidated - vm_bo is invalidated
+ *
+ * @vm_bo: vm_bo which is now invalidated
+ *
+ * State for normal BOs which are invalidated and that change not yet reflected
+ * in the PTs.
+ */
+static void amdgpu_vm_bo_invalidated(struct amdgpu_vm_bo_base *vm_bo)
+{
+	spin_lock(&vm_bo->vm->invalidated_lock);
+	list_move(&vm_bo->vm_status, &vm_bo->vm->idle);
+	spin_unlock(&vm_bo->vm->invalidated_lock);
+}
+
+/**
+ * amdgpu_vm_bo_done - vm_bo is done
+ *
+ * @vm_bo: vm_bo which is now done
+ *
+ * State for normal BOs which are invalidated and that change has been updated
+ * in the PTs.
+ */
+static void amdgpu_vm_bo_done(struct amdgpu_vm_bo_base *vm_bo)
+{
+	spin_lock(&vm_bo->vm->invalidated_lock);
+	list_del_init(&vm_bo->vm_status);
+	spin_unlock(&vm_bo->vm->invalidated_lock);
+}
+
 /**
  * amdgpu_vm_bo_base_init - Adds bo to the list of bos associated with the vm
  *
@@ -232,9 +321,9 @@ static void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base *base,
 
 	vm->bulk_moveable = false;
 	if (bo->tbo.type == ttm_bo_type_kernel)
-		list_move(&base->vm_status, &vm->relocated);
+		amdgpu_vm_bo_relocated(base);
 	else
-		list_move(&base->vm_status, &vm->idle);
+		amdgpu_vm_bo_idle(base);
 
 	if (bo->preferred_domains &
 	    amdgpu_mem_type_to_domain(bo->tbo.mem.mem_type))
@@ -245,8 +334,7 @@ static void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base *base,
 	 * is currently evicted. add the bo to the evicted list to make sure it
 	 * is validated on next vm use to avoid fault.
 	 * */
-	list_move_tail(&base->vm_status, &vm->evicted);
-	base->moved = true;
+	amdgpu_vm_bo_evicted(base);
 }
 
 /**
@@ -342,7 +430,7 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 			break;
 
 		if (bo->tbo.type != ttm_bo_type_kernel) {
-			list_move(&bo_base->vm_status, &vm->moved);
+			amdgpu_vm_bo_moved(bo_base);
 		} else {
 			if (vm->use_cpu_for_update)
 				r = amdgpu_bo_kmap(bo, NULL);
@@ -350,7 +438,7 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 				r = amdgpu_ttm_alloc_gart(&bo->tbo);
 			if (r)
 				break;
-			list_move(&bo_base->vm_status, &vm->relocated);
+			amdgpu_vm_bo_relocated(bo_base);
 		}
 	}
 
@@ -1121,8 +1209,7 @@ int amdgpu_vm_update_directories(struct amdgpu_device *adev,
 		bo_base = list_first_entry(&vm->relocated,
 					   struct amdgpu_vm_bo_base,
 					   vm_status);
-		bo_base->moved = false;
-		list_move(&bo_base->vm_status, &vm->idle);
+		amdgpu_vm_bo_idle(bo_base);
 
 		bo = bo_base->bo->parent;
 		if (!bo)
@@ -1241,7 +1328,7 @@ static void amdgpu_vm_handle_huge_pages(struct amdgpu_pte_update_params *p,
 		if (entry->huge) {
 			/* Add the entry to the relocated list to update it. */
 			entry->huge = false;
-			list_move(&entry->base.vm_status, &p->vm->relocated);
+			amdgpu_vm_bo_relocated(&entry->base);
 		}
 		return;
 	}
@@ -1740,13 +1827,11 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev,
 		uint32_t mem_type = bo->tbo.mem.mem_type;
 
 		if (!(bo->preferred_domains & amdgpu_mem_type_to_domain(mem_type)))
-			list_move_tail(&bo_va->base.vm_status, &vm->evicted);
+			amdgpu_vm_bo_evicted(&bo_va->base);
 		else
-			list_move(&bo_va->base.vm_status, &vm->idle);
+			amdgpu_vm_bo_idle(&bo_va->base);
 	} else {
-		spin_lock(&vm->invalidated_lock);
-		list_del_init(&bo_va->base.vm_status);
-		spin_unlock(&vm->invalidated_lock);
+		amdgpu_vm_bo_done(&bo_va->base);
 	}
 
 	list_splice_init(&bo_va->invalids, &bo_va->valids);
@@ -2468,30 +2553,22 @@ void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
 
 	list_for_each_entry(bo_base, &bo->va, bo_list) {
 		struct amdgpu_vm *vm = bo_base->vm;
-		bool was_moved = bo_base->moved;
 
-		bo_base->moved = true;
 		if (evicted && bo->tbo.resv == vm->root.base.bo->tbo.resv) {
-			if (bo->tbo.type == ttm_bo_type_kernel)
-				list_move(&bo_base->vm_status, &vm->evicted);
-			else
-				list_move_tail(&bo_base->vm_status,
-					       &vm->evicted);
+			amdgpu_vm_bo_evicted(bo_base);
 			continue;
 		}
 
-		if (was_moved)
+		if (bo_base->moved)
 			continue;
+		bo_base->moved = true;
 
-		if (bo->tbo.type == ttm_bo_type_kernel) {
-			list_move(&bo_base->vm_status, &vm->relocated);
-		} else if (bo->tbo.resv == vm->root.base.bo->tbo.resv) {
-			list_move(&bo_base->vm_status, &vm->moved);
-		} else {
-			spin_lock(&vm->invalidated_lock);
-			list_move(&bo_base->vm_status, &vm->invalidated);
-			spin_unlock(&vm->invalidated_lock);
-		}
+		if (bo->tbo.type == ttm_bo_type_kernel)
+			amdgpu_vm_bo_relocated(bo_base);
+		else if (bo->tbo.resv == vm->root.base.bo->tbo.resv)
+			amdgpu_vm_bo_moved(bo_base);
+		else
+			amdgpu_vm_bo_invalidated(bo_base);
 	}
 }
 
-- 
2.14.1

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

Re: [PATCH 3/3] drm/amdgpu: improve VM state machine documentation

[Zhang, Jerry (Junwei)]

On 09/02/2018 02:05 AM, Christian König wrote:
> Since we have a lot of FAQ on the VM state machine try to improve the
> documentation by adding functions for each state move.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 139 +++++++++++++++++++++++++--------
>   1 file changed, 108 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 65977e7c94dc..ce252ead2ee4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -204,6 +204,95 @@ static unsigned amdgpu_vm_bo_size(struct amdgpu_device *adev, unsigned level)
>   	return AMDGPU_GPU_PAGE_ALIGN(amdgpu_vm_num_entries(adev, level) * 8);
>   }
>
> +/**
> + * amdgpu_vm_bo_evicted - vm_bo is evicted
> + *
> + * @vm_bo: vm_bo which is evicted
> + *
> + * State for PDs/PTs and per VM BOs which are not at the location they should
> + * be.
> + */
> +static void amdgpu_vm_bo_evicted(struct amdgpu_vm_bo_base *vm_bo)
> +{
> +	struct amdgpu_vm *vm = vm_bo->vm;
> +	struct amdgpu_bo *bo = vm_bo->bo;
> +
> +	vm_bo->moved = true;
> +	if (bo->tbo.type == ttm_bo_type_kernel)
> +		list_move(&vm_bo->vm_status, &vm->evicted);
> +	else
> +		list_move_tail(&vm_bo->vm_status, &vm->evicted);
> +}
> +
> +/**
> + * amdgpu_vm_bo_relocated - vm_bo is reloacted
> + *
> + * @vm_bo: vm_bo which is relocated
> + *
> + * State for PDs/PTs which needs to update their parent PD.
> + */
> +static void amdgpu_vm_bo_relocated(struct amdgpu_vm_bo_base *vm_bo)
> +{
> +	list_move(&vm_bo->vm_status, &vm_bo->vm->relocated);
> +}
> +
> +/**
> + * amdgpu_vm_bo_moved - vm_bo is moved
> + *
> + * @vm_bo: vm_bo which is moved
> + *
> + * State for per VM BOs which are moved, but that change is not yet reflected
> + * in the page tables.
> + */
> +static void amdgpu_vm_bo_moved(struct amdgpu_vm_bo_base *vm_bo)
> +{
> +	list_move(&vm_bo->vm_status, &vm_bo->vm->moved);
> +}
> +
> +/**
> + * amdgpu_vm_bo_idle - vm_bo is idle
> + *
> + * @vm_bo: vm_bo which is now idle
> + *
> + * State for PDs/PTs and per VM BOs which have gone through the state machine
> + * and are now idle.
> + */
> +static void amdgpu_vm_bo_idle(struct amdgpu_vm_bo_base *vm_bo)
> +{
> +	list_move(&vm_bo->vm_status, &vm_bo->vm->idle);
> +	vm_bo->moved = false;
> +}
> +
> +/**
> + * amdgpu_vm_bo_invalidated - vm_bo is invalidated
> + *
> + * @vm_bo: vm_bo which is now invalidated
> + *
> + * State for normal BOs which are invalidated and that change not yet reflected
> + * in the PTs.
> + */
> +static void amdgpu_vm_bo_invalidated(struct amdgpu_vm_bo_base *vm_bo)
> +{
> +	spin_lock(&vm_bo->vm->invalidated_lock);
> +	list_move(&vm_bo->vm_status, &vm_bo->vm->idle);

Is that a typo? move to vm->invalidated?

Apart from that, it's
Reviewed-by: Junwei Zhang <Jerry.Zhang@amd.com>

Regards,
Jerry

> +	spin_unlock(&vm_bo->vm->invalidated_lock);
> +}
> +
> +/**
> + * amdgpu_vm_bo_done - vm_bo is done
> + *
> + * @vm_bo: vm_bo which is now done
> + *
> + * State for normal BOs which are invalidated and that change has been updated
> + * in the PTs.
> + */
> +static void amdgpu_vm_bo_done(struct amdgpu_vm_bo_base *vm_bo)
> +{
> +	spin_lock(&vm_bo->vm->invalidated_lock);
> +	list_del_init(&vm_bo->vm_status);
> +	spin_unlock(&vm_bo->vm->invalidated_lock);
> +}
> +
>   /**
>    * amdgpu_vm_bo_base_init - Adds bo to the list of bos associated with the vm
>    *
> @@ -232,9 +321,9 @@ static void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base *base,
>
>   	vm->bulk_moveable = false;
>   	if (bo->tbo.type == ttm_bo_type_kernel)
> -		list_move(&base->vm_status, &vm->relocated);
> +		amdgpu_vm_bo_relocated(base);
>   	else
> -		list_move(&base->vm_status, &vm->idle);
> +		amdgpu_vm_bo_idle(base);
>
>   	if (bo->preferred_domains &
>   	    amdgpu_mem_type_to_domain(bo->tbo.mem.mem_type))
> @@ -245,8 +334,7 @@ static void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base *base,
>   	 * is currently evicted. add the bo to the evicted list to make sure it
>   	 * is validated on next vm use to avoid fault.
>   	 * */
> -	list_move_tail(&base->vm_status, &vm->evicted);
> -	base->moved = true;
> +	amdgpu_vm_bo_evicted(base);
>   }
>
>   /**
> @@ -342,7 +430,7 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   			break;
>
>   		if (bo->tbo.type != ttm_bo_type_kernel) {
> -			list_move(&bo_base->vm_status, &vm->moved);
> +			amdgpu_vm_bo_moved(bo_base);
>   		} else {
>   			if (vm->use_cpu_for_update)
>   				r = amdgpu_bo_kmap(bo, NULL);
> @@ -350,7 +438,7 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   				r = amdgpu_ttm_alloc_gart(&bo->tbo);
>   			if (r)
>   				break;
> -			list_move(&bo_base->vm_status, &vm->relocated);
> +			amdgpu_vm_bo_relocated(bo_base);
>   		}
>   	}
>
> @@ -1121,8 +1209,7 @@ int amdgpu_vm_update_directories(struct amdgpu_device *adev,
>   		bo_base = list_first_entry(&vm->relocated,
>   					   struct amdgpu_vm_bo_base,
>   					   vm_status);
> -		bo_base->moved = false;
> -		list_move(&bo_base->vm_status, &vm->idle);
> +		amdgpu_vm_bo_idle(bo_base);
>
>   		bo = bo_base->bo->parent;
>   		if (!bo)
> @@ -1241,7 +1328,7 @@ static void amdgpu_vm_handle_huge_pages(struct amdgpu_pte_update_params *p,
>   		if (entry->huge) {
>   			/* Add the entry to the relocated list to update it. */
>   			entry->huge = false;
> -			list_move(&entry->base.vm_status, &p->vm->relocated);
> +			amdgpu_vm_bo_relocated(&entry->base);
>   		}
>   		return;
>   	}
> @@ -1740,13 +1827,11 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev,
>   		uint32_t mem_type = bo->tbo.mem.mem_type;
>
>   		if (!(bo->preferred_domains & amdgpu_mem_type_to_domain(mem_type)))
> -			list_move_tail(&bo_va->base.vm_status, &vm->evicted);
> +			amdgpu_vm_bo_evicted(&bo_va->base);
>   		else
> -			list_move(&bo_va->base.vm_status, &vm->idle);
> +			amdgpu_vm_bo_idle(&bo_va->base);
>   	} else {
> -		spin_lock(&vm->invalidated_lock);
> -		list_del_init(&bo_va->base.vm_status);
> -		spin_unlock(&vm->invalidated_lock);
> +		amdgpu_vm_bo_done(&bo_va->base);
>   	}
>
>   	list_splice_init(&bo_va->invalids, &bo_va->valids);
> @@ -2468,30 +2553,22 @@ void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
>
>   	list_for_each_entry(bo_base, &bo->va, bo_list) {
>   		struct amdgpu_vm *vm = bo_base->vm;
> -		bool was_moved = bo_base->moved;
>
> -		bo_base->moved = true;
>   		if (evicted && bo->tbo.resv == vm->root.base.bo->tbo.resv) {
> -			if (bo->tbo.type == ttm_bo_type_kernel)
> -				list_move(&bo_base->vm_status, &vm->evicted);
> -			else
> -				list_move_tail(&bo_base->vm_status,
> -					       &vm->evicted);
> +			amdgpu_vm_bo_evicted(bo_base);
>   			continue;
>   		}
>
> -		if (was_moved)
> +		if (bo_base->moved)
>   			continue;
> +		bo_base->moved = true;
>
> -		if (bo->tbo.type == ttm_bo_type_kernel) {
> -			list_move(&bo_base->vm_status, &vm->relocated);
> -		} else if (bo->tbo.resv == vm->root.base.bo->tbo.resv) {
> -			list_move(&bo_base->vm_status, &vm->moved);
> -		} else {
> -			spin_lock(&vm->invalidated_lock);
> -			list_move(&bo_base->vm_status, &vm->invalidated);
> -			spin_unlock(&vm->invalidated_lock);
> -		}
> +		if (bo->tbo.type == ttm_bo_type_kernel)
> +			amdgpu_vm_bo_relocated(bo_base);
> +		else if (bo->tbo.resv == vm->root.base.bo->tbo.resv)
> +			amdgpu_vm_bo_moved(bo_base);
> +		else
> +			amdgpu_vm_bo_invalidated(bo_base);
>   	}
>   }
>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/3] drm/amdgpu: move size calculations to the front of the file again

[Zhang, Jerry (Junwei)]

On 09/02/2018 02:05 AM, Christian König wrote:
> amdgpu_vm_bo_* functions should come much later.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Junwei Zhang <Jerry.Zhang@amd.com>

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 90 +++++++++++++++++-----------------
>   1 file changed, 45 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index d59222fb5931..a9275a99d793 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -133,51 +133,6 @@ struct amdgpu_prt_cb {
>   	struct dma_fence_cb cb;
>   };
>
> -/**
> - * amdgpu_vm_bo_base_init - Adds bo to the list of bos associated with the vm
> - *
> - * @base: base structure for tracking BO usage in a VM
> - * @vm: vm to which bo is to be added
> - * @bo: amdgpu buffer object
> - *
> - * Initialize a bo_va_base structure and add it to the appropriate lists
> - *
> - */
> -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;
> -
> -	vm->bulk_moveable = false;
> -	if (bo->tbo.type == ttm_bo_type_kernel)
> -		list_move(&base->vm_status, &vm->relocated);
> -	else
> -		list_move(&base->vm_status, &vm->idle);
> -
> -	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.
> -	 * */
> -	list_move_tail(&base->vm_status, &vm->evicted);
> -	base->moved = true;
> -}
> -
>   /**
>    * amdgpu_vm_level_shift - return the addr shift for each level
>    *
> @@ -249,6 +204,51 @@ static unsigned amdgpu_vm_bo_size(struct amdgpu_device *adev, unsigned level)
>   	return AMDGPU_GPU_PAGE_ALIGN(amdgpu_vm_num_entries(adev, level) * 8);
>   }
>
> +/**
> + * amdgpu_vm_bo_base_init - Adds bo to the list of bos associated with the vm
> + *
> + * @base: base structure for tracking BO usage in a VM
> + * @vm: vm to which bo is to be added
> + * @bo: amdgpu buffer object
> + *
> + * Initialize a bo_va_base structure and add it to the appropriate lists
> + *
> + */
> +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;
> +
> +	vm->bulk_moveable = false;
> +	if (bo->tbo.type == ttm_bo_type_kernel)
> +		list_move(&base->vm_status, &vm->relocated);
> +	else
> +		list_move(&base->vm_status, &vm->idle);
> +
> +	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.
> +	 * */
> +	list_move_tail(&base->vm_status, &vm->evicted);
> +	base->moved = true;
> +}
> +
>   /**
>    * amdgpu_vm_get_pd_bo - add the VM PD to a validation list
>    *
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx