[PATCH 1/5] drm/amdgpu: move VM eviction decision into amdgpu_vm.c

[PATCH 1/5] drm/amdgpu: move VM eviction decision into amdgpu_vm.c

[Christian König]

When a page tables needs to be evicted the VM code should
decide if that is possible or not.

Signed-off-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c |  5 +----
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 22 ++++++++++++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h  |  1 +
 3 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 19ffe00d9072..81f6764f1ba6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -1489,11 +1489,8 @@ static bool amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
 	struct dma_fence *f;
 	int i;
 
-	/* Don't evict VM page tables while they are busy, otherwise we can't
-	 * cleanly handle page faults.
-	 */
 	if (bo->type == ttm_bo_type_kernel &&
-	    !dma_resv_test_signaled_rcu(bo->base.resv, true))
+	    !amdgpu_vm_evictable(ttm_to_amdgpu_bo(bo)))
 		return false;
 
 	/* If bo is a KFD BO, check if the bo belongs to the current process.
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index a94c4faa5af1..a22bd57129d1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2503,6 +2503,28 @@ void amdgpu_vm_bo_rmv(struct amdgpu_device *adev,
 	kfree(bo_va);
 }
 
+/**
+ * amdgpu_vm_evictable - check if we can evict a VM
+ *
+ * @bo: A page table of the VM.
+ *
+ * Check if it is possible to evict a VM.
+ */
+bool amdgpu_vm_evictable(struct amdgpu_bo *bo)
+{
+	struct amdgpu_vm_bo_base *bo_base = bo->vm_bo;
+
+	/* Page tables of a destroyed VM can go away immediately */
+	if (!bo_base || !bo_base->vm)
+		return true;
+
+	/* Don't evict VM page tables while they are busy */
+	if (!dma_resv_test_signaled_rcu(bo->tbo.base.resv, true))
+		return false;
+
+	return true;
+}
+
 /**
  * amdgpu_vm_bo_invalidate - mark the bo as invalid
  *
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 76fcf853035c..db561765453b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -381,6 +381,7 @@ int amdgpu_vm_handle_moved(struct amdgpu_device *adev,
 int amdgpu_vm_bo_update(struct amdgpu_device *adev,
 			struct amdgpu_bo_va *bo_va,
 			bool clear);
+bool amdgpu_vm_evictable(struct amdgpu_bo *bo);
 void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
 			     struct amdgpu_bo *bo, bool evicted);
 uint64_t amdgpu_vm_map_gart(const dma_addr_t *pages_addr, uint64_t addr);
-- 
2.17.1

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

[PATCH 2/5] drm/amdgpu: explicitely sync to VM updates v2

[Christian König]

Allows us to reduce the overhead while syncing to fences a bit.

v2: also drop adev parameter from the functions

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  |  8 ++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c        | 19 +++-------
 drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c       | 13 +++----
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c       |  3 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c      | 38 ++++++++++++++-----
 drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h      |  8 ++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c   |  2 +-
 7 files changed, 51 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index b6d1958d514f..d8db5ecdf9c1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -358,7 +358,7 @@ static int vm_update_pds(struct amdgpu_vm *vm, struct amdgpu_sync *sync)
 	if (ret)
 		return ret;
 
-	return amdgpu_sync_fence(NULL, sync, vm->last_update, false);
+	return amdgpu_sync_fence(sync, vm->last_update, false);
 }
 
 static uint64_t get_pte_flags(struct amdgpu_device *adev, struct kgd_mem *mem)
@@ -751,7 +751,7 @@ static int unmap_bo_from_gpuvm(struct amdgpu_device *adev,
 
 	amdgpu_vm_clear_freed(adev, vm, &bo_va->last_pt_update);
 
-	amdgpu_sync_fence(NULL, sync, bo_va->last_pt_update, false);
+	amdgpu_sync_fence(sync, bo_va->last_pt_update, false);
 
 	return 0;
 }
@@ -770,7 +770,7 @@ static int update_gpuvm_pte(struct amdgpu_device *adev,
 		return ret;
 	}
 
-	return amdgpu_sync_fence(NULL, sync, bo_va->last_pt_update, false);
+	return amdgpu_sync_fence(sync, bo_va->last_pt_update, false);
 }
 
 static int map_bo_to_gpuvm(struct amdgpu_device *adev,
@@ -2045,7 +2045,7 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *info, struct dma_fence **ef)
 			pr_debug("Memory eviction: Validate BOs failed. Try again\n");
 			goto validate_map_fail;
 		}
-		ret = amdgpu_sync_fence(NULL, &sync_obj, bo->tbo.moving, false);
+		ret = amdgpu_sync_fence(&sync_obj, bo->tbo.moving, false);
 		if (ret) {
 			pr_debug("Memory eviction: Sync BO fence failed. Try again\n");
 			goto validate_map_fail;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 9e0c99760367..614919f265b9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -799,29 +799,23 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
 	if (r)
 		return r;
 
-	r = amdgpu_sync_fence(adev, &p->job->sync,
-			      fpriv->prt_va->last_pt_update, false);
+	r = amdgpu_sync_vm_fence(&p->job->sync, fpriv->prt_va->last_pt_update);
 	if (r)
 		return r;
 
 	if (amdgpu_mcbp || amdgpu_sriov_vf(adev)) {
-		struct dma_fence *f;
-
 		bo_va = fpriv->csa_va;
 		BUG_ON(!bo_va);
 		r = amdgpu_vm_bo_update(adev, bo_va, false);
 		if (r)
 			return r;
 
-		f = bo_va->last_pt_update;
-		r = amdgpu_sync_fence(adev, &p->job->sync, f, false);
+		r = amdgpu_sync_vm_fence(&p->job->sync, bo_va->last_pt_update);
 		if (r)
 			return r;
 	}
 
 	amdgpu_bo_list_for_each_entry(e, p->bo_list) {
-		struct dma_fence *f;
-
 		/* ignore duplicates */
 		bo = ttm_to_amdgpu_bo(e->tv.bo);
 		if (!bo)
@@ -835,8 +829,7 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
 		if (r)
 			return r;
 
-		f = bo_va->last_pt_update;
-		r = amdgpu_sync_fence(adev, &p->job->sync, f, false);
+		r = amdgpu_sync_vm_fence(&p->job->sync, bo_va->last_pt_update);
 		if (r)
 			return r;
 	}
@@ -849,7 +842,7 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
 	if (r)
 		return r;
 
-	r = amdgpu_sync_fence(adev, &p->job->sync, vm->last_update, false);
+	r = amdgpu_sync_vm_fence(&p->job->sync, vm->last_update);
 	if (r)
 		return r;
 
@@ -991,7 +984,7 @@ static int amdgpu_cs_process_fence_dep(struct amdgpu_cs_parser *p,
 			dma_fence_put(old);
 		}
 
-		r = amdgpu_sync_fence(p->adev, &p->job->sync, fence, true);
+		r = amdgpu_sync_fence(&p->job->sync, fence, true);
 		dma_fence_put(fence);
 		if (r)
 			return r;
@@ -1013,7 +1006,7 @@ static int amdgpu_syncobj_lookup_and_add_to_sync(struct amdgpu_cs_parser *p,
 		return r;
 	}
 
-	r = amdgpu_sync_fence(p->adev, &p->job->sync, fence, true);
+	r = amdgpu_sync_fence(&p->job->sync, fence, true);
 	dma_fence_put(fence);
 
 	return r;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
index 6f9289735e31..3a67f6c046d4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
@@ -206,7 +206,7 @@ static int amdgpu_vmid_grab_idle(struct amdgpu_vm *vm,
 	int r;
 
 	if (ring->vmid_wait && !dma_fence_is_signaled(ring->vmid_wait))
-		return amdgpu_sync_fence(adev, sync, ring->vmid_wait, false);
+		return amdgpu_sync_fence(sync, ring->vmid_wait, false);
 
 	fences = kmalloc_array(sizeof(void *), id_mgr->num_ids, GFP_KERNEL);
 	if (!fences)
@@ -241,7 +241,7 @@ static int amdgpu_vmid_grab_idle(struct amdgpu_vm *vm,
 			return -ENOMEM;
 		}
 
-		r = amdgpu_sync_fence(adev, sync, &array->base, false);
+		r = amdgpu_sync_fence(sync, &array->base, false);
 		dma_fence_put(ring->vmid_wait);
 		ring->vmid_wait = &array->base;
 		return r;
@@ -294,7 +294,7 @@ static int amdgpu_vmid_grab_reserved(struct amdgpu_vm *vm,
 		tmp = amdgpu_sync_peek_fence(&(*id)->active, ring);
 		if (tmp) {
 			*id = NULL;
-			r = amdgpu_sync_fence(adev, sync, tmp, false);
+			r = amdgpu_sync_fence(sync, tmp, false);
 			return r;
 		}
 		needs_flush = true;
@@ -303,7 +303,7 @@ static int amdgpu_vmid_grab_reserved(struct amdgpu_vm *vm,
 	/* Good we can use this VMID. Remember this submission as
 	* user of the VMID.
 	*/
-	r = amdgpu_sync_fence(ring->adev, &(*id)->active, fence, false);
+	r = amdgpu_sync_fence(&(*id)->active, fence, false);
 	if (r)
 		return r;
 
@@ -375,7 +375,7 @@ static int amdgpu_vmid_grab_used(struct amdgpu_vm *vm,
 		/* Good, we can use this VMID. Remember this submission as
 		 * user of the VMID.
 		 */
-		r = amdgpu_sync_fence(ring->adev, &(*id)->active, fence, false);
+		r = amdgpu_sync_fence(&(*id)->active, fence, false);
 		if (r)
 			return r;
 
@@ -435,8 +435,7 @@ int amdgpu_vmid_grab(struct amdgpu_vm *vm, struct amdgpu_ring *ring,
 			id = idle;
 
 			/* Remember this submission as user of the VMID */
-			r = amdgpu_sync_fence(ring->adev, &id->active,
-					      fence, false);
+			r = amdgpu_sync_fence(&id->active, fence, false);
 			if (r)
 				goto error;
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
index 4fb20e870e63..73328d0c741d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
@@ -193,8 +193,7 @@ static struct dma_fence *amdgpu_job_dependency(struct drm_sched_job *sched_job,
 	fence = amdgpu_sync_get_fence(&job->sync, &explicit);
 	if (fence && explicit) {
 		if (drm_sched_dependency_optimized(fence, s_entity)) {
-			r = amdgpu_sync_fence(ring->adev, &job->sched_sync,
-					      fence, false);
+			r = amdgpu_sync_fence(&job->sched_sync, fence, false);
 			if (r)
 				DRM_ERROR("Error adding fence (%d)\n", r);
 		}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
index 95e5e93edd18..f1e5fbef54d8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
@@ -129,7 +129,8 @@ static void amdgpu_sync_keep_later(struct dma_fence **keep,
  * Tries to add the fence to an existing hash entry. Returns true when an entry
  * was found, false otherwise.
  */
-static bool amdgpu_sync_add_later(struct amdgpu_sync *sync, struct dma_fence *f, bool explicit)
+static bool amdgpu_sync_add_later(struct amdgpu_sync *sync, struct dma_fence *f,
+				  bool explicit)
 {
 	struct amdgpu_sync_entry *e;
 
@@ -151,19 +152,18 @@ static bool amdgpu_sync_add_later(struct amdgpu_sync *sync, struct dma_fence *f,
  * amdgpu_sync_fence - remember to sync to this fence
  *
  * @sync: sync object to add fence to
- * @fence: fence to sync to
+ * @f: fence to sync to
+ * @explicit: if this is an explicit dependency
  *
+ * Add the fence to the sync object.
  */
-int amdgpu_sync_fence(struct amdgpu_device *adev, struct amdgpu_sync *sync,
-		      struct dma_fence *f, bool explicit)
+int amdgpu_sync_fence(struct amdgpu_sync *sync, struct dma_fence *f,
+		      bool explicit)
 {
 	struct amdgpu_sync_entry *e;
 
 	if (!f)
 		return 0;
-	if (amdgpu_sync_same_dev(adev, f) &&
-	    amdgpu_sync_get_owner(f) == AMDGPU_FENCE_OWNER_VM)
-		amdgpu_sync_keep_later(&sync->last_vm_update, f);
 
 	if (amdgpu_sync_add_later(sync, f, explicit))
 		return 0;
@@ -179,6 +179,24 @@ int amdgpu_sync_fence(struct amdgpu_device *adev, struct amdgpu_sync *sync,
 	return 0;
 }
 
+/**
+ * amdgpu_sync_vm_fence - remember to sync to this VM fence
+ *
+ * @adev: amdgpu device
+ * @sync: sync object to add fence to
+ * @fence: the VM fence to add
+ *
+ * Add the fence to the sync object and remember it as VM update.
+ */
+int amdgpu_sync_vm_fence(struct amdgpu_sync *sync, struct dma_fence *fence)
+{
+	if (!fence)
+		return 0;
+
+	amdgpu_sync_keep_later(&sync->last_vm_update, fence);
+	return amdgpu_sync_fence(sync, fence, false);
+}
+
 /**
  * amdgpu_sync_resv - sync to a reservation object
  *
@@ -204,7 +222,7 @@ int amdgpu_sync_resv(struct amdgpu_device *adev,
 
 	/* always sync to the exclusive fence */
 	f = dma_resv_get_excl(resv);
-	r = amdgpu_sync_fence(adev, sync, f, false);
+	r = amdgpu_sync_fence(sync, f, false);
 
 	flist = dma_resv_get_list(resv);
 	if (!flist || r)
@@ -239,7 +257,7 @@ int amdgpu_sync_resv(struct amdgpu_device *adev,
 				continue;
 		}
 
-		r = amdgpu_sync_fence(adev, sync, f, false);
+		r = amdgpu_sync_fence(sync, f, false);
 		if (r)
 			break;
 	}
@@ -340,7 +358,7 @@ int amdgpu_sync_clone(struct amdgpu_sync *source, struct amdgpu_sync *clone)
 	hash_for_each_safe(source->fences, i, tmp, e, node) {
 		f = e->fence;
 		if (!dma_fence_is_signaled(f)) {
-			r = amdgpu_sync_fence(NULL, clone, f, e->explicit);
+			r = amdgpu_sync_fence(clone, f, e->explicit);
 			if (r)
 				return r;
 		} else {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
index b5f1778a2319..d62c2b81d92b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
@@ -40,8 +40,9 @@ struct amdgpu_sync {
 };
 
 void amdgpu_sync_create(struct amdgpu_sync *sync);
-int amdgpu_sync_fence(struct amdgpu_device *adev, struct amdgpu_sync *sync,
-		      struct dma_fence *f, bool explicit);
+int amdgpu_sync_fence(struct amdgpu_sync *sync, struct dma_fence *f,
+		      bool explicit);
+int amdgpu_sync_vm_fence(struct amdgpu_sync *sync, struct dma_fence *fence);
 int amdgpu_sync_resv(struct amdgpu_device *adev,
 		     struct amdgpu_sync *sync,
 		     struct dma_resv *resv,
@@ -49,7 +50,8 @@ int amdgpu_sync_resv(struct amdgpu_device *adev,
 		     bool explicit_sync);
 struct dma_fence *amdgpu_sync_peek_fence(struct amdgpu_sync *sync,
 				     struct amdgpu_ring *ring);
-struct dma_fence *amdgpu_sync_get_fence(struct amdgpu_sync *sync, bool *explicit);
+struct dma_fence *amdgpu_sync_get_fence(struct amdgpu_sync *sync,
+					bool *explicit);
 int amdgpu_sync_clone(struct amdgpu_sync *source, struct amdgpu_sync *clone);
 int amdgpu_sync_wait(struct amdgpu_sync *sync, bool intr);
 void amdgpu_sync_free(struct amdgpu_sync *sync);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
index 832db59f441e..50f487666977 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
@@ -71,7 +71,7 @@ static int amdgpu_vm_sdma_prepare(struct amdgpu_vm_update_params *p,
 	p->num_dw_left = ndw;
 
 	/* Wait for moves to be completed */
-	r = amdgpu_sync_fence(p->adev, &p->job->sync, exclusive, false);
+	r = amdgpu_sync_fence(&p->job->sync, exclusive, false);
 	if (r)
 		return r;
 
-- 
2.17.1

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

Re: [PATCH 2/5] drm/amdgpu: explicitely sync to VM updates v2

[Felix Kuehling]

On 2019-12-05 8:39 a.m., Christian König wrote:
> Allows us to reduce the overhead while syncing to fences a bit.
>
> v2: also drop adev parameter from the functions
>
> Signed-off-by: Christian König <christian.koenig@amd.com>

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


> ---
>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  |  8 ++--
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c        | 19 +++-------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c       | 13 +++----
>   drivers/gpu/drm/amd/amdgpu/amdgpu_job.c       |  3 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c      | 38 ++++++++++++++-----
>   drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h      |  8 ++--
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c   |  2 +-
>   7 files changed, 51 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index b6d1958d514f..d8db5ecdf9c1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -358,7 +358,7 @@ static int vm_update_pds(struct amdgpu_vm *vm, struct amdgpu_sync *sync)
>   	if (ret)
>   		return ret;
>   
> -	return amdgpu_sync_fence(NULL, sync, vm->last_update, false);
> +	return amdgpu_sync_fence(sync, vm->last_update, false);
>   }
>   
>   static uint64_t get_pte_flags(struct amdgpu_device *adev, struct kgd_mem *mem)
> @@ -751,7 +751,7 @@ static int unmap_bo_from_gpuvm(struct amdgpu_device *adev,
>   
>   	amdgpu_vm_clear_freed(adev, vm, &bo_va->last_pt_update);
>   
> -	amdgpu_sync_fence(NULL, sync, bo_va->last_pt_update, false);
> +	amdgpu_sync_fence(sync, bo_va->last_pt_update, false);
>   
>   	return 0;
>   }
> @@ -770,7 +770,7 @@ static int update_gpuvm_pte(struct amdgpu_device *adev,
>   		return ret;
>   	}
>   
> -	return amdgpu_sync_fence(NULL, sync, bo_va->last_pt_update, false);
> +	return amdgpu_sync_fence(sync, bo_va->last_pt_update, false);
>   }
>   
>   static int map_bo_to_gpuvm(struct amdgpu_device *adev,
> @@ -2045,7 +2045,7 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *info, struct dma_fence **ef)
>   			pr_debug("Memory eviction: Validate BOs failed. Try again\n");
>   			goto validate_map_fail;
>   		}
> -		ret = amdgpu_sync_fence(NULL, &sync_obj, bo->tbo.moving, false);
> +		ret = amdgpu_sync_fence(&sync_obj, bo->tbo.moving, false);
>   		if (ret) {
>   			pr_debug("Memory eviction: Sync BO fence failed. Try again\n");
>   			goto validate_map_fail;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 9e0c99760367..614919f265b9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -799,29 +799,23 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
>   	if (r)
>   		return r;
>   
> -	r = amdgpu_sync_fence(adev, &p->job->sync,
> -			      fpriv->prt_va->last_pt_update, false);
> +	r = amdgpu_sync_vm_fence(&p->job->sync, fpriv->prt_va->last_pt_update);
>   	if (r)
>   		return r;
>   
>   	if (amdgpu_mcbp || amdgpu_sriov_vf(adev)) {
> -		struct dma_fence *f;
> -
>   		bo_va = fpriv->csa_va;
>   		BUG_ON(!bo_va);
>   		r = amdgpu_vm_bo_update(adev, bo_va, false);
>   		if (r)
>   			return r;
>   
> -		f = bo_va->last_pt_update;
> -		r = amdgpu_sync_fence(adev, &p->job->sync, f, false);
> +		r = amdgpu_sync_vm_fence(&p->job->sync, bo_va->last_pt_update);
>   		if (r)
>   			return r;
>   	}
>   
>   	amdgpu_bo_list_for_each_entry(e, p->bo_list) {
> -		struct dma_fence *f;
> -
>   		/* ignore duplicates */
>   		bo = ttm_to_amdgpu_bo(e->tv.bo);
>   		if (!bo)
> @@ -835,8 +829,7 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
>   		if (r)
>   			return r;
>   
> -		f = bo_va->last_pt_update;
> -		r = amdgpu_sync_fence(adev, &p->job->sync, f, false);
> +		r = amdgpu_sync_vm_fence(&p->job->sync, bo_va->last_pt_update);
>   		if (r)
>   			return r;
>   	}
> @@ -849,7 +842,7 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
>   	if (r)
>   		return r;
>   
> -	r = amdgpu_sync_fence(adev, &p->job->sync, vm->last_update, false);
> +	r = amdgpu_sync_vm_fence(&p->job->sync, vm->last_update);
>   	if (r)
>   		return r;
>   
> @@ -991,7 +984,7 @@ static int amdgpu_cs_process_fence_dep(struct amdgpu_cs_parser *p,
>   			dma_fence_put(old);
>   		}
>   
> -		r = amdgpu_sync_fence(p->adev, &p->job->sync, fence, true);
> +		r = amdgpu_sync_fence(&p->job->sync, fence, true);
>   		dma_fence_put(fence);
>   		if (r)
>   			return r;
> @@ -1013,7 +1006,7 @@ static int amdgpu_syncobj_lookup_and_add_to_sync(struct amdgpu_cs_parser *p,
>   		return r;
>   	}
>   
> -	r = amdgpu_sync_fence(p->adev, &p->job->sync, fence, true);
> +	r = amdgpu_sync_fence(&p->job->sync, fence, true);
>   	dma_fence_put(fence);
>   
>   	return r;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> index 6f9289735e31..3a67f6c046d4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> @@ -206,7 +206,7 @@ static int amdgpu_vmid_grab_idle(struct amdgpu_vm *vm,
>   	int r;
>   
>   	if (ring->vmid_wait && !dma_fence_is_signaled(ring->vmid_wait))
> -		return amdgpu_sync_fence(adev, sync, ring->vmid_wait, false);
> +		return amdgpu_sync_fence(sync, ring->vmid_wait, false);
>   
>   	fences = kmalloc_array(sizeof(void *), id_mgr->num_ids, GFP_KERNEL);
>   	if (!fences)
> @@ -241,7 +241,7 @@ static int amdgpu_vmid_grab_idle(struct amdgpu_vm *vm,
>   			return -ENOMEM;
>   		}
>   
> -		r = amdgpu_sync_fence(adev, sync, &array->base, false);
> +		r = amdgpu_sync_fence(sync, &array->base, false);
>   		dma_fence_put(ring->vmid_wait);
>   		ring->vmid_wait = &array->base;
>   		return r;
> @@ -294,7 +294,7 @@ static int amdgpu_vmid_grab_reserved(struct amdgpu_vm *vm,
>   		tmp = amdgpu_sync_peek_fence(&(*id)->active, ring);
>   		if (tmp) {
>   			*id = NULL;
> -			r = amdgpu_sync_fence(adev, sync, tmp, false);
> +			r = amdgpu_sync_fence(sync, tmp, false);
>   			return r;
>   		}
>   		needs_flush = true;
> @@ -303,7 +303,7 @@ static int amdgpu_vmid_grab_reserved(struct amdgpu_vm *vm,
>   	/* Good we can use this VMID. Remember this submission as
>   	* user of the VMID.
>   	*/
> -	r = amdgpu_sync_fence(ring->adev, &(*id)->active, fence, false);
> +	r = amdgpu_sync_fence(&(*id)->active, fence, false);
>   	if (r)
>   		return r;
>   
> @@ -375,7 +375,7 @@ static int amdgpu_vmid_grab_used(struct amdgpu_vm *vm,
>   		/* Good, we can use this VMID. Remember this submission as
>   		 * user of the VMID.
>   		 */
> -		r = amdgpu_sync_fence(ring->adev, &(*id)->active, fence, false);
> +		r = amdgpu_sync_fence(&(*id)->active, fence, false);
>   		if (r)
>   			return r;
>   
> @@ -435,8 +435,7 @@ int amdgpu_vmid_grab(struct amdgpu_vm *vm, struct amdgpu_ring *ring,
>   			id = idle;
>   
>   			/* Remember this submission as user of the VMID */
> -			r = amdgpu_sync_fence(ring->adev, &id->active,
> -					      fence, false);
> +			r = amdgpu_sync_fence(&id->active, fence, false);
>   			if (r)
>   				goto error;
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> index 4fb20e870e63..73328d0c741d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> @@ -193,8 +193,7 @@ static struct dma_fence *amdgpu_job_dependency(struct drm_sched_job *sched_job,
>   	fence = amdgpu_sync_get_fence(&job->sync, &explicit);
>   	if (fence && explicit) {
>   		if (drm_sched_dependency_optimized(fence, s_entity)) {
> -			r = amdgpu_sync_fence(ring->adev, &job->sched_sync,
> -					      fence, false);
> +			r = amdgpu_sync_fence(&job->sched_sync, fence, false);
>   			if (r)
>   				DRM_ERROR("Error adding fence (%d)\n", r);
>   		}
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
> index 95e5e93edd18..f1e5fbef54d8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
> @@ -129,7 +129,8 @@ static void amdgpu_sync_keep_later(struct dma_fence **keep,
>    * Tries to add the fence to an existing hash entry. Returns true when an entry
>    * was found, false otherwise.
>    */
> -static bool amdgpu_sync_add_later(struct amdgpu_sync *sync, struct dma_fence *f, bool explicit)
> +static bool amdgpu_sync_add_later(struct amdgpu_sync *sync, struct dma_fence *f,
> +				  bool explicit)
>   {
>   	struct amdgpu_sync_entry *e;
>   
> @@ -151,19 +152,18 @@ static bool amdgpu_sync_add_later(struct amdgpu_sync *sync, struct dma_fence *f,
>    * amdgpu_sync_fence - remember to sync to this fence
>    *
>    * @sync: sync object to add fence to
> - * @fence: fence to sync to
> + * @f: fence to sync to
> + * @explicit: if this is an explicit dependency
>    *
> + * Add the fence to the sync object.
>    */
> -int amdgpu_sync_fence(struct amdgpu_device *adev, struct amdgpu_sync *sync,
> -		      struct dma_fence *f, bool explicit)
> +int amdgpu_sync_fence(struct amdgpu_sync *sync, struct dma_fence *f,
> +		      bool explicit)
>   {
>   	struct amdgpu_sync_entry *e;
>   
>   	if (!f)
>   		return 0;
> -	if (amdgpu_sync_same_dev(adev, f) &&
> -	    amdgpu_sync_get_owner(f) == AMDGPU_FENCE_OWNER_VM)
> -		amdgpu_sync_keep_later(&sync->last_vm_update, f);
>   
>   	if (amdgpu_sync_add_later(sync, f, explicit))
>   		return 0;
> @@ -179,6 +179,24 @@ int amdgpu_sync_fence(struct amdgpu_device *adev, struct amdgpu_sync *sync,
>   	return 0;
>   }
>   
> +/**
> + * amdgpu_sync_vm_fence - remember to sync to this VM fence
> + *
> + * @adev: amdgpu device
> + * @sync: sync object to add fence to
> + * @fence: the VM fence to add
> + *
> + * Add the fence to the sync object and remember it as VM update.
> + */
> +int amdgpu_sync_vm_fence(struct amdgpu_sync *sync, struct dma_fence *fence)
> +{
> +	if (!fence)
> +		return 0;
> +
> +	amdgpu_sync_keep_later(&sync->last_vm_update, fence);
> +	return amdgpu_sync_fence(sync, fence, false);
> +}
> +
>   /**
>    * amdgpu_sync_resv - sync to a reservation object
>    *
> @@ -204,7 +222,7 @@ int amdgpu_sync_resv(struct amdgpu_device *adev,
>   
>   	/* always sync to the exclusive fence */
>   	f = dma_resv_get_excl(resv);
> -	r = amdgpu_sync_fence(adev, sync, f, false);
> +	r = amdgpu_sync_fence(sync, f, false);
>   
>   	flist = dma_resv_get_list(resv);
>   	if (!flist || r)
> @@ -239,7 +257,7 @@ int amdgpu_sync_resv(struct amdgpu_device *adev,
>   				continue;
>   		}
>   
> -		r = amdgpu_sync_fence(adev, sync, f, false);
> +		r = amdgpu_sync_fence(sync, f, false);
>   		if (r)
>   			break;
>   	}
> @@ -340,7 +358,7 @@ int amdgpu_sync_clone(struct amdgpu_sync *source, struct amdgpu_sync *clone)
>   	hash_for_each_safe(source->fences, i, tmp, e, node) {
>   		f = e->fence;
>   		if (!dma_fence_is_signaled(f)) {
> -			r = amdgpu_sync_fence(NULL, clone, f, e->explicit);
> +			r = amdgpu_sync_fence(clone, f, e->explicit);
>   			if (r)
>   				return r;
>   		} else {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
> index b5f1778a2319..d62c2b81d92b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
> @@ -40,8 +40,9 @@ struct amdgpu_sync {
>   };
>   
>   void amdgpu_sync_create(struct amdgpu_sync *sync);
> -int amdgpu_sync_fence(struct amdgpu_device *adev, struct amdgpu_sync *sync,
> -		      struct dma_fence *f, bool explicit);
> +int amdgpu_sync_fence(struct amdgpu_sync *sync, struct dma_fence *f,
> +		      bool explicit);
> +int amdgpu_sync_vm_fence(struct amdgpu_sync *sync, struct dma_fence *fence);
>   int amdgpu_sync_resv(struct amdgpu_device *adev,
>   		     struct amdgpu_sync *sync,
>   		     struct dma_resv *resv,
> @@ -49,7 +50,8 @@ int amdgpu_sync_resv(struct amdgpu_device *adev,
>   		     bool explicit_sync);
>   struct dma_fence *amdgpu_sync_peek_fence(struct amdgpu_sync *sync,
>   				     struct amdgpu_ring *ring);
> -struct dma_fence *amdgpu_sync_get_fence(struct amdgpu_sync *sync, bool *explicit);
> +struct dma_fence *amdgpu_sync_get_fence(struct amdgpu_sync *sync,
> +					bool *explicit);
>   int amdgpu_sync_clone(struct amdgpu_sync *source, struct amdgpu_sync *clone);
>   int amdgpu_sync_wait(struct amdgpu_sync *sync, bool intr);
>   void amdgpu_sync_free(struct amdgpu_sync *sync);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> index 832db59f441e..50f487666977 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> @@ -71,7 +71,7 @@ static int amdgpu_vm_sdma_prepare(struct amdgpu_vm_update_params *p,
>   	p->num_dw_left = ndw;
>   
>   	/* Wait for moves to be completed */
> -	r = amdgpu_sync_fence(p->adev, &p->job->sync, exclusive, false);
> +	r = amdgpu_sync_fence(&p->job->sync, exclusive, false);
>   	if (r)
>   		return r;
>   
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 3/5] drm/amdgpu: stop adding VM updates fences to the resv obj

[Christian König]

Don't add the VM update fences to the resv object and remove
the handling to stop implicitely syncing to them.

Ongoing updates prevent page tables from being evicted and we manually
block for all updates to complete before releasing PDs and PTS.

This way we can do updates even without the resv obj locked.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c    |  6 ++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c      | 30 ++++++++++++++++++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h      |  4 +++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 11 +++++---
 4 files changed, 40 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
index f1e5fbef54d8..ae8bc766215c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
@@ -243,10 +243,8 @@ int amdgpu_sync_resv(struct amdgpu_device *adev,
 			/* VM updates are only interesting
 			 * for other VM updates and moves.
 			 */
-			if ((owner != AMDGPU_FENCE_OWNER_UNDEFINED) &&
-			    (fence_owner != AMDGPU_FENCE_OWNER_UNDEFINED) &&
-			    ((owner == AMDGPU_FENCE_OWNER_VM) !=
-			     (fence_owner == AMDGPU_FENCE_OWNER_VM)))
+			if (owner == AMDGPU_FENCE_OWNER_VM &&
+			    fence_owner != AMDGPU_FENCE_OWNER_UNDEFINED)
 				continue;
 
 			/* Ignore fence from the same owner and explicit one as
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index a22bd57129d1..0d700e8154c4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -562,8 +562,8 @@ void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
 {
 	entry->priority = 0;
 	entry->tv.bo = &vm->root.base.bo->tbo;
-	/* One for the VM updates, one for TTM and one for the CS job */
-	entry->tv.num_shared = 3;
+	/* One for TTM and one for the CS job */
+	entry->tv.num_shared = 2;
 	entry->user_pages = NULL;
 	list_add(&entry->tv.head, validated);
 }
@@ -2522,6 +2522,11 @@ bool amdgpu_vm_evictable(struct amdgpu_bo *bo)
 	if (!dma_resv_test_signaled_rcu(bo->tbo.base.resv, true))
 		return false;
 
+	/* Don't evict VM page tables while they are updated */
+	if (!dma_fence_is_signaled(bo_base->vm->last_direct) ||
+	    !dma_fence_is_signaled(bo_base->vm->last_delayed))
+		return false;
+
 	return true;
 }
 
@@ -2687,8 +2692,16 @@ void amdgpu_vm_adjust_size(struct amdgpu_device *adev, uint32_t min_vm_size,
  */
 long amdgpu_vm_wait_idle(struct amdgpu_vm *vm, long timeout)
 {
-	return dma_resv_wait_timeout_rcu(vm->root.base.bo->tbo.base.resv,
-						   true, true, timeout);
+	timeout = dma_resv_wait_timeout_rcu(vm->root.base.bo->tbo.base.resv,
+					    true, true, timeout);
+	if (timeout <= 0)
+		return timeout;
+
+	timeout = dma_fence_wait_timeout(vm->last_direct, true, timeout);
+	if (timeout <= 0)
+		return timeout;
+
+	return dma_fence_wait_timeout(vm->last_delayed, true, timeout);
 }
 
 /**
@@ -2757,6 +2770,8 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 	else
 		vm->update_funcs = &amdgpu_vm_sdma_funcs;
 	vm->last_update = NULL;
+	vm->last_direct = dma_fence_get_stub();
+	vm->last_delayed = dma_fence_get_stub();
 
 	amdgpu_vm_bo_param(adev, vm, adev->vm_manager.root_level, false, &bp);
 	if (vm_context == AMDGPU_VM_CONTEXT_COMPUTE)
@@ -2807,6 +2822,8 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 	vm->root.base.bo = NULL;
 
 error_free_delayed:
+	dma_fence_put(vm->last_direct);
+	dma_fence_put(vm->last_delayed);
 	drm_sched_entity_destroy(&vm->delayed);
 
 error_free_direct:
@@ -3007,6 +3024,11 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 		vm->pasid = 0;
 	}
 
+	dma_fence_wait(vm->last_direct, false);
+	dma_fence_put(vm->last_direct);
+	dma_fence_wait(vm->last_delayed, false);
+	dma_fence_put(vm->last_delayed);
+
 	list_for_each_entry_safe(mapping, tmp, &vm->freed, list) {
 		if (mapping->flags & AMDGPU_PTE_PRT && prt_fini_needed) {
 			amdgpu_vm_prt_fini(adev, vm);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index db561765453b..d93ea9ad879e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -269,6 +269,10 @@ struct amdgpu_vm {
 	struct drm_sched_entity	direct;
 	struct drm_sched_entity	delayed;
 
+	/* Last submission to the scheduler entities */
+	struct dma_fence	*last_direct;
+	struct dma_fence	*last_delayed;
+
 	unsigned int		pasid;
 	/* dedicated to vm */
 	struct amdgpu_vmid	*reserved_vmid[AMDGPU_MAX_VMHUBS];
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
index 50f487666977..19b7f80758f1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
@@ -95,11 +95,10 @@ static int amdgpu_vm_sdma_prepare(struct amdgpu_vm_update_params *p,
 static int amdgpu_vm_sdma_commit(struct amdgpu_vm_update_params *p,
 				 struct dma_fence **fence)
 {
-	struct amdgpu_bo *root = p->vm->root.base.bo;
 	struct amdgpu_ib *ib = p->job->ibs;
 	struct drm_sched_entity *entity;
+	struct dma_fence *f, *tmp;
 	struct amdgpu_ring *ring;
-	struct dma_fence *f;
 	int r;
 
 	entity = p->direct ? &p->vm->direct : &p->vm->delayed;
@@ -112,7 +111,13 @@ static int amdgpu_vm_sdma_commit(struct amdgpu_vm_update_params *p,
 	if (r)
 		goto error;
 
-	amdgpu_bo_fence(root, f, true);
+	tmp = dma_fence_get(f);
+	if (p->direct)
+		swap(p->vm->last_direct, tmp);
+	else
+		swap(p->vm->last_delayed, tmp);
+	dma_fence_put(tmp);
+
 	if (fence && !p->direct)
 		swap(*fence, f);
 	dma_fence_put(f);
-- 
2.17.1

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

Re: [PATCH 3/5] drm/amdgpu: stop adding VM updates fences to the resv obj

[Felix Kuehling]

On 2019-12-05 8:39 a.m., Christian König wrote:
> Don't add the VM update fences to the resv object and remove
> the handling to stop implicitely syncing to them.
>
> Ongoing updates prevent page tables from being evicted and we manually
> block for all updates to complete before releasing PDs and PTS.
>
> This way we can do updates even without the resv obj locked.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c    |  6 ++---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c      | 30 ++++++++++++++++++---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h      |  4 +++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 11 +++++---
>   4 files changed, 40 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
> index f1e5fbef54d8..ae8bc766215c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
> @@ -243,10 +243,8 @@ int amdgpu_sync_resv(struct amdgpu_device *adev,
>   			/* VM updates are only interesting
>   			 * for other VM updates and moves.

Thanks for updating the commit description. I think this comment should 
also be updated because "other VM updates" fences are no longer in the 
resv. Something like this: VM updates only sync with moves but not with 
user command submissions or KFD evictions fences. With that fixed, this 
patch is

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

>   			 */
> -			if ((owner != AMDGPU_FENCE_OWNER_UNDEFINED) &&
> -			    (fence_owner != AMDGPU_FENCE_OWNER_UNDEFINED) &&
> -			    ((owner == AMDGPU_FENCE_OWNER_VM) !=
> -			     (fence_owner == AMDGPU_FENCE_OWNER_VM)))
> +			if (owner == AMDGPU_FENCE_OWNER_VM &&
> +			    fence_owner != AMDGPU_FENCE_OWNER_UNDEFINED)
>   				continue;
>   
>   			/* Ignore fence from the same owner and explicit one as
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index a22bd57129d1..0d700e8154c4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -562,8 +562,8 @@ void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
>   {
>   	entry->priority = 0;
>   	entry->tv.bo = &vm->root.base.bo->tbo;
> -	/* One for the VM updates, one for TTM and one for the CS job */
> -	entry->tv.num_shared = 3;
> +	/* One for TTM and one for the CS job */
> +	entry->tv.num_shared = 2;
>   	entry->user_pages = NULL;
>   	list_add(&entry->tv.head, validated);
>   }
> @@ -2522,6 +2522,11 @@ bool amdgpu_vm_evictable(struct amdgpu_bo *bo)
>   	if (!dma_resv_test_signaled_rcu(bo->tbo.base.resv, true))
>   		return false;
>   
> +	/* Don't evict VM page tables while they are updated */
> +	if (!dma_fence_is_signaled(bo_base->vm->last_direct) ||
> +	    !dma_fence_is_signaled(bo_base->vm->last_delayed))
> +		return false;
> +
>   	return true;
>   }
>   
> @@ -2687,8 +2692,16 @@ void amdgpu_vm_adjust_size(struct amdgpu_device *adev, uint32_t min_vm_size,
>    */
>   long amdgpu_vm_wait_idle(struct amdgpu_vm *vm, long timeout)
>   {
> -	return dma_resv_wait_timeout_rcu(vm->root.base.bo->tbo.base.resv,
> -						   true, true, timeout);
> +	timeout = dma_resv_wait_timeout_rcu(vm->root.base.bo->tbo.base.resv,
> +					    true, true, timeout);
> +	if (timeout <= 0)
> +		return timeout;
> +
> +	timeout = dma_fence_wait_timeout(vm->last_direct, true, timeout);
> +	if (timeout <= 0)
> +		return timeout;
> +
> +	return dma_fence_wait_timeout(vm->last_delayed, true, timeout);
>   }
>   
>   /**
> @@ -2757,6 +2770,8 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   	else
>   		vm->update_funcs = &amdgpu_vm_sdma_funcs;
>   	vm->last_update = NULL;
> +	vm->last_direct = dma_fence_get_stub();
> +	vm->last_delayed = dma_fence_get_stub();
>   
>   	amdgpu_vm_bo_param(adev, vm, adev->vm_manager.root_level, false, &bp);
>   	if (vm_context == AMDGPU_VM_CONTEXT_COMPUTE)
> @@ -2807,6 +2822,8 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   	vm->root.base.bo = NULL;
>   
>   error_free_delayed:
> +	dma_fence_put(vm->last_direct);
> +	dma_fence_put(vm->last_delayed);
>   	drm_sched_entity_destroy(&vm->delayed);
>   
>   error_free_direct:
> @@ -3007,6 +3024,11 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>   		vm->pasid = 0;
>   	}
>   
> +	dma_fence_wait(vm->last_direct, false);
> +	dma_fence_put(vm->last_direct);
> +	dma_fence_wait(vm->last_delayed, false);
> +	dma_fence_put(vm->last_delayed);
> +
>   	list_for_each_entry_safe(mapping, tmp, &vm->freed, list) {
>   		if (mapping->flags & AMDGPU_PTE_PRT && prt_fini_needed) {
>   			amdgpu_vm_prt_fini(adev, vm);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index db561765453b..d93ea9ad879e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -269,6 +269,10 @@ struct amdgpu_vm {
>   	struct drm_sched_entity	direct;
>   	struct drm_sched_entity	delayed;
>   
> +	/* Last submission to the scheduler entities */
> +	struct dma_fence	*last_direct;
> +	struct dma_fence	*last_delayed;
> +
>   	unsigned int		pasid;
>   	/* dedicated to vm */
>   	struct amdgpu_vmid	*reserved_vmid[AMDGPU_MAX_VMHUBS];
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> index 50f487666977..19b7f80758f1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> @@ -95,11 +95,10 @@ static int amdgpu_vm_sdma_prepare(struct amdgpu_vm_update_params *p,
>   static int amdgpu_vm_sdma_commit(struct amdgpu_vm_update_params *p,
>   				 struct dma_fence **fence)
>   {
> -	struct amdgpu_bo *root = p->vm->root.base.bo;
>   	struct amdgpu_ib *ib = p->job->ibs;
>   	struct drm_sched_entity *entity;
> +	struct dma_fence *f, *tmp;
>   	struct amdgpu_ring *ring;
> -	struct dma_fence *f;
>   	int r;
>   
>   	entity = p->direct ? &p->vm->direct : &p->vm->delayed;
> @@ -112,7 +111,13 @@ static int amdgpu_vm_sdma_commit(struct amdgpu_vm_update_params *p,
>   	if (r)
>   		goto error;
>   
> -	amdgpu_bo_fence(root, f, true);
> +	tmp = dma_fence_get(f);
> +	if (p->direct)
> +		swap(p->vm->last_direct, tmp);
> +	else
> +		swap(p->vm->last_delayed, tmp);
> +	dma_fence_put(tmp);
> +
>   	if (fence && !p->direct)
>   		swap(*fence, f);
>   	dma_fence_put(f);
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 4/5] drm/amdgpu: add VM eviction lock v2

[Christian König]

This allows to invalidate VM entries without taking the reservation lock.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 0d700e8154c4..839d6df394fc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -656,7 +656,7 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 			      void *param)
 {
 	struct amdgpu_vm_bo_base *bo_base, *tmp;
-	int r = 0;
+	int r;
 
 	vm->bulk_moveable &= list_empty(&vm->evicted);
 
@@ -665,7 +665,7 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 
 		r = validate(param, bo);
 		if (r)
-			break;
+			return r;
 
 		if (bo->tbo.type != ttm_bo_type_kernel) {
 			amdgpu_vm_bo_moved(bo_base);
@@ -678,7 +678,11 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 		}
 	}
 
-	return r;
+	mutex_lock(&vm->eviction_lock);
+	vm->evicting = false;
+	mutex_unlock(&vm->eviction_lock);
+
+	return 0;
 }
 
 /**
@@ -1555,15 +1559,25 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
 	if (!(flags & AMDGPU_PTE_VALID))
 		owner = AMDGPU_FENCE_OWNER_KFD;
 
+	mutex_lock(&vm->eviction_lock);
+	if (vm->evicting) {
+		r = EINPROGRESS;
+		goto error_unlock;
+	}
+
 	r = vm->update_funcs->prepare(&params, owner, exclusive);
 	if (r)
-		return r;
+		goto error_unlock;
 
 	r = amdgpu_vm_update_ptes(&params, start, last + 1, addr, flags);
 	if (r)
-		return r;
+		goto error_unlock;
 
-	return vm->update_funcs->commit(&params, fence);
+	r = vm->update_funcs->commit(&params, fence);
+
+error_unlock:
+	mutex_unlock(&vm->eviction_lock);
+	return r;
 }
 
 /**
@@ -2522,11 +2536,19 @@ bool amdgpu_vm_evictable(struct amdgpu_bo *bo)
 	if (!dma_resv_test_signaled_rcu(bo->tbo.base.resv, true))
 		return false;
 
+	/* Try to block ongoing updates */
+	if (!mutex_trylock(&bo_base->vm->eviction_lock))
+		return false;
+
 	/* Don't evict VM page tables while they are updated */
 	if (!dma_fence_is_signaled(bo_base->vm->last_direct) ||
-	    !dma_fence_is_signaled(bo_base->vm->last_delayed))
+	    !dma_fence_is_signaled(bo_base->vm->last_delayed)) {
+		mutex_unlock(&bo_base->vm->eviction_lock);
 		return false;
+	}
 
+	bo_base->vm->evicting = true;
+	mutex_unlock(&bo_base->vm->eviction_lock);
 	return true;
 }
 
@@ -2773,6 +2795,9 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 	vm->last_direct = dma_fence_get_stub();
 	vm->last_delayed = dma_fence_get_stub();
 
+	mutex_init(&vm->eviction_lock);
+	vm->evicting = false;
+
 	amdgpu_vm_bo_param(adev, vm, adev->vm_manager.root_level, false, &bp);
 	if (vm_context == AMDGPU_VM_CONTEXT_COMPUTE)
 		bp.flags &= ~AMDGPU_GEM_CREATE_SHADOW;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index d93ea9ad879e..d5613d184e99 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -242,6 +242,10 @@ struct amdgpu_vm {
 	/* tree of virtual addresses mapped */
 	struct rb_root_cached	va;
 
+	/* Lock to prevent eviction while we are updating page tables */
+	struct mutex		eviction_lock;
+	bool			evicting;
+
 	/* BOs who needs a validation */
 	struct list_head	evicted;
 
-- 
2.17.1

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

Re: [PATCH 4/5] drm/amdgpu: add VM eviction lock v2

[Felix Kuehling]

On 2019-12-05 8:39 a.m., Christian König wrote:
> This allows to invalidate VM entries without taking the reservation lock.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 39 +++++++++++++++++++++-----
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |  4 +++
>   2 files changed, 36 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 0d700e8154c4..839d6df394fc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -656,7 +656,7 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   			      void *param)
>   {
>   	struct amdgpu_vm_bo_base *bo_base, *tmp;
> -	int r = 0;
> +	int r;
>   
>   	vm->bulk_moveable &= list_empty(&vm->evicted);
>   
> @@ -665,7 +665,7 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   
>   		r = validate(param, bo);
>   		if (r)
> -			break;
> +			return r;
>   
>   		if (bo->tbo.type != ttm_bo_type_kernel) {
>   			amdgpu_vm_bo_moved(bo_base);
> @@ -678,7 +678,11 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   		}
>   	}
>   
> -	return r;
> +	mutex_lock(&vm->eviction_lock);
> +	vm->evicting = false;
> +	mutex_unlock(&vm->eviction_lock);
> +
> +	return 0;
>   }
>   
>   /**
> @@ -1555,15 +1559,25 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
>   	if (!(flags & AMDGPU_PTE_VALID))
>   		owner = AMDGPU_FENCE_OWNER_KFD;
>   
> +	mutex_lock(&vm->eviction_lock);
> +	if (vm->evicting) {
> +		r = EINPROGRESS;

Not sure about this error code. As far as I can find, this is normally 
used on non-blocking sockets. I found this explanation: 
https://stackoverflow.com/questions/8277970/what-are-possible-reason-for-socket-error-einprogress-in-solaris

Quote: "so there is another error for non-blocking connect: EINPROGRESS, 
which tells you that the operation is in progress and you should check 
its status later."

This call is neither non-blocking nor is the requested page table update 
in progress when this error is returned. So I'd think a better error to 
return here would be EBUSY.

Other than that, this patch is

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


> +		goto error_unlock;
> +	}
> +
>   	r = vm->update_funcs->prepare(&params, owner, exclusive);
>   	if (r)
> -		return r;
> +		goto error_unlock;
>   
>   	r = amdgpu_vm_update_ptes(&params, start, last + 1, addr, flags);
>   	if (r)
> -		return r;
> +		goto error_unlock;
>   
> -	return vm->update_funcs->commit(&params, fence);
> +	r = vm->update_funcs->commit(&params, fence);
> +
> +error_unlock:
> +	mutex_unlock(&vm->eviction_lock);
> +	return r;
>   }
>   
>   /**
> @@ -2522,11 +2536,19 @@ bool amdgpu_vm_evictable(struct amdgpu_bo *bo)
>   	if (!dma_resv_test_signaled_rcu(bo->tbo.base.resv, true))
>   		return false;
>   
> +	/* Try to block ongoing updates */
> +	if (!mutex_trylock(&bo_base->vm->eviction_lock))
> +		return false;
> +
>   	/* Don't evict VM page tables while they are updated */
>   	if (!dma_fence_is_signaled(bo_base->vm->last_direct) ||
> -	    !dma_fence_is_signaled(bo_base->vm->last_delayed))
> +	    !dma_fence_is_signaled(bo_base->vm->last_delayed)) {
> +		mutex_unlock(&bo_base->vm->eviction_lock);
>   		return false;
> +	}
>   
> +	bo_base->vm->evicting = true;
> +	mutex_unlock(&bo_base->vm->eviction_lock);
>   	return true;
>   }
>   
> @@ -2773,6 +2795,9 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   	vm->last_direct = dma_fence_get_stub();
>   	vm->last_delayed = dma_fence_get_stub();
>   
> +	mutex_init(&vm->eviction_lock);
> +	vm->evicting = false;
> +
>   	amdgpu_vm_bo_param(adev, vm, adev->vm_manager.root_level, false, &bp);
>   	if (vm_context == AMDGPU_VM_CONTEXT_COMPUTE)
>   		bp.flags &= ~AMDGPU_GEM_CREATE_SHADOW;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index d93ea9ad879e..d5613d184e99 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -242,6 +242,10 @@ struct amdgpu_vm {
>   	/* tree of virtual addresses mapped */
>   	struct rb_root_cached	va;
>   
> +	/* Lock to prevent eviction while we are updating page tables */
> +	struct mutex		eviction_lock;
> +	bool			evicting;
> +
>   	/* BOs who needs a validation */
>   	struct list_head	evicted;
>   
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 5/5] drm/amdgpu: immedially invalidate PTEs

[Christian König]

When a BO is evicted immedially invalidate the mapped PTEs.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 839d6df394fc..e578113bfd55 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2565,6 +2565,7 @@ void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
 			     struct amdgpu_bo *bo, bool evicted)
 {
 	struct amdgpu_vm_bo_base *bo_base;
+	int r;
 
 	/* shadow bo doesn't have bo base, its validation needs its parent */
 	if (bo->parent && bo->parent->shadow == bo)
@@ -2572,8 +2573,22 @@ void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
 
 	for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) {
 		struct amdgpu_vm *vm = bo_base->vm;
+		struct dma_resv *resv = vm->root.base.bo->tbo.base.resv;
+
+		if (bo->tbo.type != ttm_bo_type_kernel) {
+			struct amdgpu_bo_va *bo_va;
+
+			bo_va = container_of(bo_base, struct amdgpu_bo_va,
+					     base);
+			r = amdgpu_vm_bo_update(adev, bo_va,
+						bo->tbo.base.resv != resv);
+			if (!r) {
+				amdgpu_vm_bo_idle(bo_base);
+				continue;
+			}
+		}
 
-		if (evicted && bo->tbo.base.resv == vm->root.base.bo->tbo.base.resv) {
+		if (evicted && bo->tbo.base.resv == resv) {
 			amdgpu_vm_bo_evicted(bo_base);
 			continue;
 		}
-- 
2.17.1

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

Re: [PATCH 5/5] drm/amdgpu: immedially invalidate PTEs

[Felix Kuehling]

On 2019-12-05 8:39 a.m., Christian König wrote:
> When a BO is evicted immedially invalidate the mapped PTEs.

I think you mentioned that this is just a proof of concept. I wouldn't 
submit the patch like this because it's overkill for VMs that don't want 
to use recoverable page faults and probably has a performance impact. I 
would do something specific to compute VMs in our MMU notifier.

Regards,
   Felix

>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 17 ++++++++++++++++-
>   1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 839d6df394fc..e578113bfd55 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2565,6 +2565,7 @@ void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
>   			     struct amdgpu_bo *bo, bool evicted)
>   {
>   	struct amdgpu_vm_bo_base *bo_base;
> +	int r;
>   
>   	/* shadow bo doesn't have bo base, its validation needs its parent */
>   	if (bo->parent && bo->parent->shadow == bo)
> @@ -2572,8 +2573,22 @@ void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
>   
>   	for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) {
>   		struct amdgpu_vm *vm = bo_base->vm;
> +		struct dma_resv *resv = vm->root.base.bo->tbo.base.resv;
> +
> +		if (bo->tbo.type != ttm_bo_type_kernel) {
> +			struct amdgpu_bo_va *bo_va;
> +
> +			bo_va = container_of(bo_base, struct amdgpu_bo_va,
> +					     base);
> +			r = amdgpu_vm_bo_update(adev, bo_va,
> +						bo->tbo.base.resv != resv);
> +			if (!r) {
> +				amdgpu_vm_bo_idle(bo_base);
> +				continue;
> +			}
> +		}
>   
> -		if (evicted && bo->tbo.base.resv == vm->root.base.bo->tbo.base.resv) {
> +		if (evicted && bo->tbo.base.resv == resv) {
>   			amdgpu_vm_bo_evicted(bo_base);
>   			continue;
>   		}
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 5/5] drm/amdgpu: immedially invalidate PTEs

[Christian König]

Am 05.12.19 um 17:45 schrieb Felix Kuehling:
> On 2019-12-05 8:39 a.m., Christian König wrote:
>> When a BO is evicted immedially invalidate the mapped PTEs.
>
> I think you mentioned that this is just a proof of concept.

I also need this for immediately getting rid of mappings in DMA-buf 
based P2P.

> I wouldn't submit the patch like this because it's overkill for VMs 
> that don't want to use recoverable page faults and probably has a 
> performance impact. I would do something specific to compute VMs in 
> our MMU notifier.

I still need to test this, but I hope that it doesn't have that much 
impact on performance.

Regards,
Christian.

>
> Regards,
>   Felix
>
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 17 ++++++++++++++++-
>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 839d6df394fc..e578113bfd55 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -2565,6 +2565,7 @@ void amdgpu_vm_bo_invalidate(struct 
>> amdgpu_device *adev,
>>                    struct amdgpu_bo *bo, bool evicted)
>>   {
>>       struct amdgpu_vm_bo_base *bo_base;
>> +    int r;
>>         /* shadow bo doesn't have bo base, its validation needs its 
>> parent */
>>       if (bo->parent && bo->parent->shadow == bo)
>> @@ -2572,8 +2573,22 @@ void amdgpu_vm_bo_invalidate(struct 
>> amdgpu_device *adev,
>>         for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) {
>>           struct amdgpu_vm *vm = bo_base->vm;
>> +        struct dma_resv *resv = vm->root.base.bo->tbo.base.resv;
>> +
>> +        if (bo->tbo.type != ttm_bo_type_kernel) {
>> +            struct amdgpu_bo_va *bo_va;
>> +
>> +            bo_va = container_of(bo_base, struct amdgpu_bo_va,
>> +                         base);
>> +            r = amdgpu_vm_bo_update(adev, bo_va,
>> +                        bo->tbo.base.resv != resv);
>> +            if (!r) {
>> +                amdgpu_vm_bo_idle(bo_base);
>> +                continue;
>> +            }
>> +        }
>>   -        if (evicted && bo->tbo.base.resv == 
>> vm->root.base.bo->tbo.base.resv) {
>> +        if (evicted && bo->tbo.base.resv == resv) {
>>               amdgpu_vm_bo_evicted(bo_base);
>>               continue;
>>           }

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

Re: [PATCH 5/5] drm/amdgpu: immedially invalidate PTEs

[Felix Kuehling]

Hi Christian,

Alex started trying to invalidate PTEs in the MMU notifiers and we're 
finding that we still need to reserve the VM reservation for 
amdgpu_sync_resv in amdgpu_vm_sdma_prepare. Is that sync_resv still 
needed now, given that VM fences aren't in that reservation object any more?

Regards,
   Felix

On 2019-12-05 5:39, Christian König wrote:
> When a BO is evicted immedially invalidate the mapped PTEs.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 17 ++++++++++++++++-
>   1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 839d6df394fc..e578113bfd55 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2565,6 +2565,7 @@ void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
>   			     struct amdgpu_bo *bo, bool evicted)
>   {
>   	struct amdgpu_vm_bo_base *bo_base;
> +	int r;
>   
>   	/* shadow bo doesn't have bo base, its validation needs its parent */
>   	if (bo->parent && bo->parent->shadow == bo)
> @@ -2572,8 +2573,22 @@ void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
>   
>   	for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) {
>   		struct amdgpu_vm *vm = bo_base->vm;
> +		struct dma_resv *resv = vm->root.base.bo->tbo.base.resv;
> +
> +		if (bo->tbo.type != ttm_bo_type_kernel) {
> +			struct amdgpu_bo_va *bo_va;
> +
> +			bo_va = container_of(bo_base, struct amdgpu_bo_va,
> +					     base);
> +			r = amdgpu_vm_bo_update(adev, bo_va,
> +						bo->tbo.base.resv != resv);
> +			if (!r) {
> +				amdgpu_vm_bo_idle(bo_base);
> +				continue;
> +			}
> +		}
>   
> -		if (evicted && bo->tbo.base.resv == vm->root.base.bo->tbo.base.resv) {
> +		if (evicted && bo->tbo.base.resv == resv) {
>   			amdgpu_vm_bo_evicted(bo_base);
>   			continue;
>   		}
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 5/5] drm/amdgpu: immedially invalidate PTEs

[Christian König]

Hi Felix,

yeah, I've also found a corner case which would raise a warning now.

Need to rework how dependencies for the PTE update are generated.

Going to take care of this in the next few days,
Christian.

Am 12.12.19 um 01:20 schrieb Felix Kuehling:
> Hi Christian,
>
> Alex started trying to invalidate PTEs in the MMU notifiers and we're 
> finding that we still need to reserve the VM reservation for 
> amdgpu_sync_resv in amdgpu_vm_sdma_prepare. Is that sync_resv still 
> needed now, given that VM fences aren't in that reservation object any 
> more?
>
> Regards,
>   Felix
>
> On 2019-12-05 5:39, Christian König wrote:
>> When a BO is evicted immedially invalidate the mapped PTEs.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 17 ++++++++++++++++-
>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 839d6df394fc..e578113bfd55 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -2565,6 +2565,7 @@ void amdgpu_vm_bo_invalidate(struct 
>> amdgpu_device *adev,
>>                    struct amdgpu_bo *bo, bool evicted)
>>   {
>>       struct amdgpu_vm_bo_base *bo_base;
>> +    int r;
>>         /* shadow bo doesn't have bo base, its validation needs its 
>> parent */
>>       if (bo->parent && bo->parent->shadow == bo)
>> @@ -2572,8 +2573,22 @@ void amdgpu_vm_bo_invalidate(struct 
>> amdgpu_device *adev,
>>         for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) {
>>           struct amdgpu_vm *vm = bo_base->vm;
>> +        struct dma_resv *resv = vm->root.base.bo->tbo.base.resv;
>> +
>> +        if (bo->tbo.type != ttm_bo_type_kernel) {
>> +            struct amdgpu_bo_va *bo_va;
>> +
>> +            bo_va = container_of(bo_base, struct amdgpu_bo_va,
>> +                         base);
>> +            r = amdgpu_vm_bo_update(adev, bo_va,
>> +                        bo->tbo.base.resv != resv);
>> +            if (!r) {
>> +                amdgpu_vm_bo_idle(bo_base);
>> +                continue;
>> +            }
>> +        }
>>   -        if (evicted && bo->tbo.base.resv == 
>> vm->root.base.bo->tbo.base.resv) {
>> +        if (evicted && bo->tbo.base.resv == resv) {
>>               amdgpu_vm_bo_evicted(bo_base);
>>               continue;
>>           }

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

Re: [PATCH 5/5] drm/amdgpu: immedially invalidate PTEs

[Philip Yang]

Hi Christian,

FYI, remove amdgpu_bo_reserve(root, true) before calling 
amdgpu_vm_bo_update_mapping, I got this warning backtrace:

     [  182.390072] WARNING: CPU: 12 PID: 4376 at
 
/home/yangp/git/compute_staging/kernel/drivers/gpu/drm/ttm/ttm_bo.c:1229
     ttm_bo_validate+0x14d/0x1b0 [ttm]
     [  182.390085] Modules linked in: fuse ip6table_filter ip6_tables
     iptable_filter amdgpu amd_iommu_v2 gpu_sched ast drm_vram_helper
     drm_ttm_helper ttm k10temp ip_tables x_tables i2c_piix4
     [  182.390123] CPU: 12 PID: 4376 Comm: kfdtest Tainted: G        W
     5.4.0-rc7-kfd-yangp #1
     [  182.390133] Hardware name: GIGABYTE MZ01-CE0-00/MZ01-CE0-00, 
BIOS F12
     08/05/2019
     [  182.390146] RIP: 0010:ttm_bo_validate+0x14d/0x1b0 [ttm]
     [  182.390153] Code: 40 ff 52 18 8b 44 24 04 e9 4f ff ff ff 48 8b 87 20
     01 00 00 be ff ff ff ff 48 8d 78 60 e8 5b 3a e1 d4 85 c0 0f 85 e7 fe ff
     ff <0f> 0b e9 e0 fe ff ff be 01 00 00 00 48 89 df e8 2f c4 ff ff e9 19
     [  182.390161] RSP: 0018:ffffab7a032f3990 EFLAGS: 00010246
     [  182.390166] RAX: 0000000000000000 RBX: ffff943c59b37850 RCX:
     ffff943c59b35000
     [  182.390171] RDX: ffff943c539daf00 RSI: ffff943c56fb31d8 RDI:
     ffff943c539db790
     [  182.390178] RBP: ffff943c59b37830 R08: 0000000000000200 R09:
     0000000000000000
     [  182.390184] R10: 0000000000000000 R11: 00000000001fee0e R12:
     ffffab7a032f3a50
     [  182.390194] R13: 0000000000000200 R14: ffff943c59b37800 R15:
     0000000000000000
     [  182.390197] FS:  00007f0d27f41780(0000) GS:ffff943c9e900000(0000)
     knlGS:0000000000000000
     [  182.390203] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
     [  182.390209] CR2: 00007fba7a1010a0 CR3: 00000007f2624000 CR4:
     00000000003406e0
     [  182.390212] Call Trace:
     [  182.390219]  ? rcu_read_lock_sched_held+0x52/0x80
     [  182.390223]  ? _raw_spin_unlock+0x24/0x30
     [  182.390267]  ? amdgpu_bo_do_create+0x4d1/0x5d0 [amdgpu]
     [  182.390319]  amdgpu_vm_clear_bo+0x13d/0x3a0 [amdgpu]
     [  182.390371]  ? amdgpu_vm_num_entries+0x1e/0x70 [amdgpu]
     [  182.390424]  amdgpu_vm_update_ptes+0x561/0x5d0 [amdgpu]
     [  182.390480]  amdgpu_vm_bo_update_mapping+0xfd/0x130 [amdgpu]
     [  182.390530]  amdgpu_vm_bo_split_mapping+0x1ea/0x2c0 [amdgpu]
     [  182.390591]  svm_range_map_to_gpus+0x160/0x310 [amdgpu]
     [  182.390650]  kfd_register_svm+0xb8/0x2b0 [amdgpu]
     [  182.390708]  kfd_ioctl_register_svm+0xe8/0x110 [amdgpu]
     [  182.390765]  kfd_ioctl+0x232/0x3d0 [amdgpu]
     [  182.390823]  ? kfd_ioctl_get_process_apertures_new+0x310/0x310
     [amdgpu]
     [  182.390838]  ? selinux_file_ioctl+0x153/0x210
     [  182.390845]  do_vfs_ioctl+0xa2/0x6e0
     [  182.390854]  ksys_ioctl+0x70/0x80
     [  182.390862]  __x64_sys_ioctl+0x16/0x20
     [  182.390869]  do_syscall_64+0x4a/0x1b0
     [  182.390879]  entry_SYSCALL_64_after_hwframe+0x49/0xbe

Philip

On 2019-12-12 3:51 a.m., Christian König wrote:
> Hi Felix,
> 
> yeah, I've also found a corner case which would raise a warning now.
> 
> Need to rework how dependencies for the PTE update are generated.
> 
> Going to take care of this in the next few days,
> Christian.
> 
> Am 12.12.19 um 01:20 schrieb Felix Kuehling:
>> Hi Christian,
>>
>> Alex started trying to invalidate PTEs in the MMU notifiers and we're 
>> finding that we still need to reserve the VM reservation for 
>> amdgpu_sync_resv in amdgpu_vm_sdma_prepare. Is that sync_resv still 
>> needed now, given that VM fences aren't in that reservation object any 
>> more?
>>
>> Regards,
>>   Felix
>>
>> On 2019-12-05 5:39, Christian König wrote:
>>> When a BO is evicted immedially invalidate the mapped PTEs.
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 17 ++++++++++++++++-
>>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index 839d6df394fc..e578113bfd55 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -2565,6 +2565,7 @@ void amdgpu_vm_bo_invalidate(struct 
>>> amdgpu_device *adev,
>>>                    struct amdgpu_bo *bo, bool evicted)
>>>   {
>>>       struct amdgpu_vm_bo_base *bo_base;
>>> +    int r;
>>>         /* shadow bo doesn't have bo base, its validation needs its 
>>> parent */
>>>       if (bo->parent && bo->parent->shadow == bo)
>>> @@ -2572,8 +2573,22 @@ void amdgpu_vm_bo_invalidate(struct 
>>> amdgpu_device *adev,
>>>         for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) {
>>>           struct amdgpu_vm *vm = bo_base->vm;
>>> +        struct dma_resv *resv = vm->root.base.bo->tbo.base.resv;
>>> +
>>> +        if (bo->tbo.type != ttm_bo_type_kernel) {
>>> +            struct amdgpu_bo_va *bo_va;
>>> +
>>> +            bo_va = container_of(bo_base, struct amdgpu_bo_va,
>>> +                         base);
>>> +            r = amdgpu_vm_bo_update(adev, bo_va,
>>> +                        bo->tbo.base.resv != resv);
>>> +            if (!r) {
>>> +                amdgpu_vm_bo_idle(bo_base);
>>> +                continue;
>>> +            }
>>> +        }
>>>   -        if (evicted && bo->tbo.base.resv == 
>>> vm->root.base.bo->tbo.base.resv) {
>>> +        if (evicted && bo->tbo.base.resv == resv) {
>>>               amdgpu_vm_bo_evicted(bo_base);
>>>               continue;
>>>           }
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 5/5] drm/amdgpu: immedially invalidate PTEs

[Christian König]

Hi Philip,

that is an expected result. You can only invalidate page tables without 
holding the reservation lock.

What you do here is adding a new mapping and that one needs to allocate 
a new page tables and won't work like this.

Regards,
Christian.

Am 12.12.19 um 15:38 schrieb Philip Yang:
> Hi Christian,
>
> FYI, remove amdgpu_bo_reserve(root, true) before calling 
> amdgpu_vm_bo_update_mapping, I got this warning backtrace:
>
>     [  182.390072] WARNING: CPU: 12 PID: 4376 at
>
> /home/yangp/git/compute_staging/kernel/drivers/gpu/drm/ttm/ttm_bo.c:1229
>     ttm_bo_validate+0x14d/0x1b0 [ttm]
>     [  182.390085] Modules linked in: fuse ip6table_filter ip6_tables
>     iptable_filter amdgpu amd_iommu_v2 gpu_sched ast drm_vram_helper
>     drm_ttm_helper ttm k10temp ip_tables x_tables i2c_piix4
>     [  182.390123] CPU: 12 PID: 4376 Comm: kfdtest Tainted: G        W
>     5.4.0-rc7-kfd-yangp #1
>     [  182.390133] Hardware name: GIGABYTE MZ01-CE0-00/MZ01-CE0-00, 
> BIOS F12
>     08/05/2019
>     [  182.390146] RIP: 0010:ttm_bo_validate+0x14d/0x1b0 [ttm]
>     [  182.390153] Code: 40 ff 52 18 8b 44 24 04 e9 4f ff ff ff 48 8b 
> 87 20
>     01 00 00 be ff ff ff ff 48 8d 78 60 e8 5b 3a e1 d4 85 c0 0f 85 e7 
> fe ff
>     ff <0f> 0b e9 e0 fe ff ff be 01 00 00 00 48 89 df e8 2f c4 ff ff 
> e9 19
>     [  182.390161] RSP: 0018:ffffab7a032f3990 EFLAGS: 00010246
>     [  182.390166] RAX: 0000000000000000 RBX: ffff943c59b37850 RCX:
>     ffff943c59b35000
>     [  182.390171] RDX: ffff943c539daf00 RSI: ffff943c56fb31d8 RDI:
>     ffff943c539db790
>     [  182.390178] RBP: ffff943c59b37830 R08: 0000000000000200 R09:
>     0000000000000000
>     [  182.390184] R10: 0000000000000000 R11: 00000000001fee0e R12:
>     ffffab7a032f3a50
>     [  182.390194] R13: 0000000000000200 R14: ffff943c59b37800 R15:
>     0000000000000000
>     [  182.390197] FS:  00007f0d27f41780(0000) GS:ffff943c9e900000(0000)
>     knlGS:0000000000000000
>     [  182.390203] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>     [  182.390209] CR2: 00007fba7a1010a0 CR3: 00000007f2624000 CR4:
>     00000000003406e0
>     [  182.390212] Call Trace:
>     [  182.390219]  ? rcu_read_lock_sched_held+0x52/0x80
>     [  182.390223]  ? _raw_spin_unlock+0x24/0x30
>     [  182.390267]  ? amdgpu_bo_do_create+0x4d1/0x5d0 [amdgpu]
>     [  182.390319]  amdgpu_vm_clear_bo+0x13d/0x3a0 [amdgpu]
>     [  182.390371]  ? amdgpu_vm_num_entries+0x1e/0x70 [amdgpu]
>     [  182.390424]  amdgpu_vm_update_ptes+0x561/0x5d0 [amdgpu]
>     [  182.390480]  amdgpu_vm_bo_update_mapping+0xfd/0x130 [amdgpu]
>     [  182.390530]  amdgpu_vm_bo_split_mapping+0x1ea/0x2c0 [amdgpu]
>     [  182.390591]  svm_range_map_to_gpus+0x160/0x310 [amdgpu]
>     [  182.390650]  kfd_register_svm+0xb8/0x2b0 [amdgpu]
>     [  182.390708]  kfd_ioctl_register_svm+0xe8/0x110 [amdgpu]
>     [  182.390765]  kfd_ioctl+0x232/0x3d0 [amdgpu]
>     [  182.390823]  ? kfd_ioctl_get_process_apertures_new+0x310/0x310
>     [amdgpu]
>     [  182.390838]  ? selinux_file_ioctl+0x153/0x210
>     [  182.390845]  do_vfs_ioctl+0xa2/0x6e0
>     [  182.390854]  ksys_ioctl+0x70/0x80
>     [  182.390862]  __x64_sys_ioctl+0x16/0x20
>     [  182.390869]  do_syscall_64+0x4a/0x1b0
>     [  182.390879]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> Philip
>
> On 2019-12-12 3:51 a.m., Christian König wrote:
>> Hi Felix,
>>
>> yeah, I've also found a corner case which would raise a warning now.
>>
>> Need to rework how dependencies for the PTE update are generated.
>>
>> Going to take care of this in the next few days,
>> Christian.
>>
>> Am 12.12.19 um 01:20 schrieb Felix Kuehling:
>>> Hi Christian,
>>>
>>> Alex started trying to invalidate PTEs in the MMU notifiers and 
>>> we're finding that we still need to reserve the VM reservation for 
>>> amdgpu_sync_resv in amdgpu_vm_sdma_prepare. Is that sync_resv still 
>>> needed now, given that VM fences aren't in that reservation object 
>>> any more?
>>>
>>> Regards,
>>>   Felix
>>>
>>> On 2019-12-05 5:39, Christian König wrote:
>>>> When a BO is evicted immedially invalidate the mapped PTEs.
>>>>
>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 17 ++++++++++++++++-
>>>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> index 839d6df394fc..e578113bfd55 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> @@ -2565,6 +2565,7 @@ void amdgpu_vm_bo_invalidate(struct 
>>>> amdgpu_device *adev,
>>>>                    struct amdgpu_bo *bo, bool evicted)
>>>>   {
>>>>       struct amdgpu_vm_bo_base *bo_base;
>>>> +    int r;
>>>>         /* shadow bo doesn't have bo base, its validation needs its 
>>>> parent */
>>>>       if (bo->parent && bo->parent->shadow == bo)
>>>> @@ -2572,8 +2573,22 @@ void amdgpu_vm_bo_invalidate(struct 
>>>> amdgpu_device *adev,
>>>>         for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) {
>>>>           struct amdgpu_vm *vm = bo_base->vm;
>>>> +        struct dma_resv *resv = vm->root.base.bo->tbo.base.resv;
>>>> +
>>>> +        if (bo->tbo.type != ttm_bo_type_kernel) {
>>>> +            struct amdgpu_bo_va *bo_va;
>>>> +
>>>> +            bo_va = container_of(bo_base, struct amdgpu_bo_va,
>>>> +                         base);
>>>> +            r = amdgpu_vm_bo_update(adev, bo_va,
>>>> +                        bo->tbo.base.resv != resv);
>>>> +            if (!r) {
>>>> +                amdgpu_vm_bo_idle(bo_base);
>>>> +                continue;
>>>> +            }
>>>> +        }
>>>>   -        if (evicted && bo->tbo.base.resv == 
>>>> vm->root.base.bo->tbo.base.resv) {
>>>> +        if (evicted && bo->tbo.base.resv == resv) {
>>>>               amdgpu_vm_bo_evicted(bo_base);
>>>>               continue;
>>>>           }
>>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

RE: [PATCH 5/5] drm/amdgpu: immedially invalidate PTEs

[Sierra Guiza, Alejandro (Alex)]

[AMD Official Use Only - Internal Distribution Only]

Hi Christian, 
I wonder if you had a chance to look into this warning. 
Please let me know if there's something we could help with.

Regards,
Alejandro

-----Original Message-----
From: Christian König <ckoenig.leichtzumerken@gmail.com> 
Sent: Thursday, December 12, 2019 2:52 AM
To: Kuehling, Felix <Felix.Kuehling@amd.com>; amd-gfx@lists.freedesktop.org; Yang, Philip <Philip.Yang@amd.com>; Sierra Guiza, Alejandro (Alex) <Alex.Sierra@amd.com>
Subject: Re: [PATCH 5/5] drm/amdgpu: immedially invalidate PTEs

[CAUTION: External Email]

Hi Felix,

yeah, I've also found a corner case which would raise a warning now.

Need to rework how dependencies for the PTE update are generated.

Going to take care of this in the next few days, Christian.

Am 12.12.19 um 01:20 schrieb Felix Kuehling:
> Hi Christian,
>
> Alex started trying to invalidate PTEs in the MMU notifiers and we're 
> finding that we still need to reserve the VM reservation for 
> amdgpu_sync_resv in amdgpu_vm_sdma_prepare. Is that sync_resv still 
> needed now, given that VM fences aren't in that reservation object any 
> more?
>
> Regards,
>   Felix
>
> On 2019-12-05 5:39, Christian König wrote:
>> When a BO is evicted immedially invalidate the mapped PTEs.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 17 ++++++++++++++++-
>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 839d6df394fc..e578113bfd55 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -2565,6 +2565,7 @@ void amdgpu_vm_bo_invalidate(struct 
>> amdgpu_device *adev,
>>                    struct amdgpu_bo *bo, bool evicted)
>>   {
>>       struct amdgpu_vm_bo_base *bo_base;
>> +    int r;
>>         /* shadow bo doesn't have bo base, its validation needs its 
>> parent */
>>       if (bo->parent && bo->parent->shadow == bo) @@ -2572,8 +2573,22 
>> @@ void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
>>         for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) {
>>           struct amdgpu_vm *vm = bo_base->vm;
>> +        struct dma_resv *resv = vm->root.base.bo->tbo.base.resv;
>> +
>> +        if (bo->tbo.type != ttm_bo_type_kernel) {
>> +            struct amdgpu_bo_va *bo_va;
>> +
>> +            bo_va = container_of(bo_base, struct amdgpu_bo_va,
>> +                         base);
>> +            r = amdgpu_vm_bo_update(adev, bo_va,
>> +                        bo->tbo.base.resv != resv);
>> +            if (!r) {
>> +                amdgpu_vm_bo_idle(bo_base);
>> +                continue;
>> +            }
>> +        }
>>   -        if (evicted && bo->tbo.base.resv ==
>> vm->root.base.bo->tbo.base.resv) {
>> +        if (evicted && bo->tbo.base.resv == resv) {
>>               amdgpu_vm_bo_evicted(bo_base);
>>               continue;
>>           }
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 5/5] drm/amdgpu: immedially invalidate PTEs

[Christian König]

Hi Alejandro,

yes I've implemented this, but I'm still hunting down some bugs with the 
new locks.

I will send out the patches on Monday even if I can't narrow that 
problem down further.

Regards,
Christian.

Am 03.01.20 um 05:20 schrieb Sierra Guiza, Alejandro (Alex):
> [AMD Official Use Only - Internal Distribution Only]
>
> Hi Christian,
> I wonder if you had a chance to look into this warning.
> Please let me know if there's something we could help with.
>
> Regards,
> Alejandro
>
> -----Original Message-----
> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> Sent: Thursday, December 12, 2019 2:52 AM
> To: Kuehling, Felix <Felix.Kuehling@amd.com>; amd-gfx@lists.freedesktop.org; Yang, Philip <Philip.Yang@amd.com>; Sierra Guiza, Alejandro (Alex) <Alex.Sierra@amd.com>
> Subject: Re: [PATCH 5/5] drm/amdgpu: immedially invalidate PTEs
>
> [CAUTION: External Email]
>
> Hi Felix,
>
> yeah, I've also found a corner case which would raise a warning now.
>
> Need to rework how dependencies for the PTE update are generated.
>
> Going to take care of this in the next few days, Christian.
>
> Am 12.12.19 um 01:20 schrieb Felix Kuehling:
>> Hi Christian,
>>
>> Alex started trying to invalidate PTEs in the MMU notifiers and we're
>> finding that we still need to reserve the VM reservation for
>> amdgpu_sync_resv in amdgpu_vm_sdma_prepare. Is that sync_resv still
>> needed now, given that VM fences aren't in that reservation object any
>> more?
>>
>> Regards,
>>    Felix
>>
>> On 2019-12-05 5:39, Christian König wrote:
>>> When a BO is evicted immedially invalidate the mapped PTEs.
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 17 ++++++++++++++++-
>>>    1 file changed, 16 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index 839d6df394fc..e578113bfd55 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -2565,6 +2565,7 @@ void amdgpu_vm_bo_invalidate(struct
>>> amdgpu_device *adev,
>>>                     struct amdgpu_bo *bo, bool evicted)
>>>    {
>>>        struct amdgpu_vm_bo_base *bo_base;
>>> +    int r;
>>>          /* shadow bo doesn't have bo base, its validation needs its
>>> parent */
>>>        if (bo->parent && bo->parent->shadow == bo) @@ -2572,8 +2573,22
>>> @@ void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
>>>          for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) {
>>>            struct amdgpu_vm *vm = bo_base->vm;
>>> +        struct dma_resv *resv = vm->root.base.bo->tbo.base.resv;
>>> +
>>> +        if (bo->tbo.type != ttm_bo_type_kernel) {
>>> +            struct amdgpu_bo_va *bo_va;
>>> +
>>> +            bo_va = container_of(bo_base, struct amdgpu_bo_va,
>>> +                         base);
>>> +            r = amdgpu_vm_bo_update(adev, bo_va,
>>> +                        bo->tbo.base.resv != resv);
>>> +            if (!r) {
>>> +                amdgpu_vm_bo_idle(bo_base);
>>> +                continue;
>>> +            }
>>> +        }
>>>    -        if (evicted && bo->tbo.base.resv ==
>>> vm->root.base.bo->tbo.base.resv) {
>>> +        if (evicted && bo->tbo.base.resv == resv) {
>>>                amdgpu_vm_bo_evicted(bo_base);
>>>                continue;
>>>            }
> _______________________________________________
> 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