[PATCH 1/5] drm/amdgpu: fix braces in amdgpu_vm_update_ptes

[PATCH 1/5] drm/amdgpu: fix braces in amdgpu_vm_update_ptes

[Christian König]

For the root PD mask can be 0xffffffff as well which would
overrun to 0 if we don't cast it before we add one.

Signed-off-by: Christian König <christian.koenig@amd.com>
Tested-by: Tom St Denis <tom.stdenis@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 5cb182231f5d..4ba6a5e5d094 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1487,7 +1487,7 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
 		incr = (uint64_t)AMDGPU_GPU_PAGE_SIZE << shift;
 		mask = amdgpu_vm_entries_mask(adev, cursor.level);
 		pe_start = ((cursor.pfn >> shift) & mask) * 8;
-		entry_end = (uint64_t)(mask + 1) << shift;
+		entry_end = ((uint64_t)mask + 1) << shift;
 		entry_end += cursor.pfn & ~(entry_end - 1);
 		entry_end = min(entry_end, end);
 
-- 
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: return EINVAL instead of ENOENT in the VM code

[Christian König]

That we can't find a PD above the root is expected can only happen if
we try to update a larger range than actually managed by the VM.

Signed-off-by: Christian König <christian.koenig@amd.com>
Tested-by: Tom St Denis <tom.stdenis@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 4ba6a5e5d094..9705c961405b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1475,7 +1475,7 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
 			 * shift we should go up one level and check it again.
 			 */
 			if (!amdgpu_vm_pt_ancestor(&cursor))
-				return -ENOENT;
+				return -EINVAL;
 			continue;
 		}
 
-- 
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: return EINVAL instead of ENOENT in the VM code

[Felix Kuehling]

On 2020-01-30 7:49, Christian König wrote:
> That we can't find a PD above the root is expected can only happen if
> we try to update a larger range than actually managed by the VM.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Tested-by: Tom St Denis <tom.stdenis@amd.com>
Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 4ba6a5e5d094..9705c961405b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1475,7 +1475,7 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
>   			 * shift we should go up one level and check it again.
>   			 */
>   			if (!amdgpu_vm_pt_ancestor(&cursor))
> -				return -ENOENT;
> +				return -EINVAL;
>   			continue;
>   		}
>   
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 3/5] drm/amdgpu: allow higher level PD invalidations

[Christian König]

Allow partial invalidation on unallocated PDs. This is useful when we
need to silence faults to stop interrupt floods on Vega.

Signed-off-by: Christian König <christian.koenig@amd.com>
Tested-by: Tom St Denis <tom.stdenis@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 9705c961405b..6038b3c89633 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1467,9 +1467,8 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
 			 * smaller than the address shift. Go to the next
 			 * child entry and try again.
 			 */
-			if (!amdgpu_vm_pt_descendant(adev, &cursor))
-				return -ENOENT;
-			continue;
+			if (amdgpu_vm_pt_descendant(adev, &cursor))
+				continue;
 		} else if (frag >= parent_shift) {
 			/* If the fragment size is even larger than the parent
 			 * shift we should go up one level and check it again.
@@ -1480,8 +1479,19 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
 		}
 
 		pt = cursor.entry->base.bo;
-		if (!pt)
-			return -ENOENT;
+		if (!pt) {
+			/* We need all PDs and PTs for mapping something, */
+			if (flags & AMDGPU_PTE_VALID)
+				return -ENOENT;
+
+			/* but unmapping something can happen at a higher
+			 * level. */
+			if (!amdgpu_vm_pt_ancestor(&cursor))
+				return -EINVAL;
+
+			pt = cursor.entry->base.bo;
+			shift = parent_shift;
+		}
 
 		/* Looks good so far, calculate parameters for the update */
 		incr = (uint64_t)AMDGPU_GPU_PAGE_SIZE << shift;
@@ -1495,6 +1505,9 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
 			uint64_t upd_end = min(entry_end, frag_end);
 			unsigned nptes = (upd_end - frag_start) >> shift;
 
+			/* This can happen when we set higher level PDs to
+			 * silent to stop fault floods. */
+			nptes = max(nptes, 1u);
 			amdgpu_vm_update_flags(params, pt, cursor.level,
 					       pe_start, dst, nptes, incr,
 					       flags | AMDGPU_PTE_FRAG(frag));
-- 
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: allow higher level PD invalidations

[Felix Kuehling]

On 2020-01-30 7:49, Christian König wrote:
> Allow partial invalidation on unallocated PDs. This is useful when we
> need to silence faults to stop interrupt floods on Vega.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Tested-by: Tom St Denis <tom.stdenis@amd.com>

I already reviewed this a week ago. With two style nit-picks fixed, this 
patch is

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


> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 23 ++++++++++++++++++-----
>   1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 9705c961405b..6038b3c89633 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1467,9 +1467,8 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
>   			 * smaller than the address shift. Go to the next
>   			 * child entry and try again.
>   			 */
> -			if (!amdgpu_vm_pt_descendant(adev, &cursor))
> -				return -ENOENT;
> -			continue;
> +			if (amdgpu_vm_pt_descendant(adev, &cursor))
> +				continue;
>   		} else if (frag >= parent_shift) {
>   			/* If the fragment size is even larger than the parent
>   			 * shift we should go up one level and check it again.
> @@ -1480,8 +1479,19 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
>   		}
>   
>   		pt = cursor.entry->base.bo;
> -		if (!pt)
> -			return -ENOENT;
> +		if (!pt) {
> +			/* We need all PDs and PTs for mapping something, */
> +			if (flags & AMDGPU_PTE_VALID)
> +				return -ENOENT;
> +
> +			/* but unmapping something can happen at a higher
> +			 * level. */

checkpatch.pl complains about multi-line comments with the */ not on its 
own line.


> +			if (!amdgpu_vm_pt_ancestor(&cursor))
> +				return -EINVAL;
> +
> +			pt = cursor.entry->base.bo;
> +			shift = parent_shift;
> +		}
>   
>   		/* Looks good so far, calculate parameters for the update */
>   		incr = (uint64_t)AMDGPU_GPU_PAGE_SIZE << shift;
> @@ -1495,6 +1505,9 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
>   			uint64_t upd_end = min(entry_end, frag_end);
>   			unsigned nptes = (upd_end - frag_start) >> shift;
>   
> +			/* This can happen when we set higher level PDs to
> +			 * silent to stop fault floods. */

Same as above.


> +			nptes = max(nptes, 1u);
>   			amdgpu_vm_update_flags(params, pt, cursor.level,
>   					       pe_start, dst, nptes, incr,
>   					       flags | AMDGPU_PTE_FRAG(frag));
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 4/5] drm/amdgpu: simplify and fix amdgpu_sync_resv

[Christian König]

No matter what we always need to sync to moves.

Signed-off-by: Christian König <christian.koenig@amd.com>
Tested-by: Tom St Denis <tom.stdenis@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
index c124f64e7aae..9f42032676da 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
@@ -232,10 +232,19 @@ int amdgpu_sync_resv(struct amdgpu_device *adev, struct amdgpu_sync *sync,
 
 		f = rcu_dereference_protected(flist->shared[i],
 					      dma_resv_held(resv));
+
+		fence_owner = amdgpu_sync_get_owner(f);
+
+		/* Always sync to moves, no matter what */
+		if (fence_owner == AMDGPU_FENCE_OWNER_UNDEFINED) {
+			r = amdgpu_sync_fence(sync, f, false);
+			if (r)
+				break;
+		}
+
 		/* We only want to trigger KFD eviction fences on
 		 * evict or move jobs. Skip KFD fences otherwise.
 		 */
-		fence_owner = amdgpu_sync_get_owner(f);
 		if (fence_owner == AMDGPU_FENCE_OWNER_KFD &&
 		    owner != AMDGPU_FENCE_OWNER_UNDEFINED)
 			continue;
@@ -265,9 +274,7 @@ int amdgpu_sync_resv(struct amdgpu_device *adev, struct amdgpu_sync *sync,
 			break;
 
 		case AMDGPU_SYNC_EXPLICIT:
-			if (owner != AMDGPU_FENCE_OWNER_UNDEFINED)
-				continue;
-			break;
+			continue;
 		}
 
 		r = amdgpu_sync_fence(sync, f, false);
-- 
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: simplify and fix amdgpu_sync_resv

[Felix Kuehling]

On 2020-01-30 7:49, Christian König wrote:
> No matter what we always need to sync to moves.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Tested-by: Tom St Denis <tom.stdenis@amd.com>

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


> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c | 15 +++++++++++----
>   1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
> index c124f64e7aae..9f42032676da 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
> @@ -232,10 +232,19 @@ int amdgpu_sync_resv(struct amdgpu_device *adev, struct amdgpu_sync *sync,
>   
>   		f = rcu_dereference_protected(flist->shared[i],
>   					      dma_resv_held(resv));
> +
> +		fence_owner = amdgpu_sync_get_owner(f);
> +
> +		/* Always sync to moves, no matter what */
> +		if (fence_owner == AMDGPU_FENCE_OWNER_UNDEFINED) {
> +			r = amdgpu_sync_fence(sync, f, false);
> +			if (r)
> +				break;
> +		}
> +
>   		/* We only want to trigger KFD eviction fences on
>   		 * evict or move jobs. Skip KFD fences otherwise.
>   		 */
> -		fence_owner = amdgpu_sync_get_owner(f);
>   		if (fence_owner == AMDGPU_FENCE_OWNER_KFD &&
>   		    owner != AMDGPU_FENCE_OWNER_UNDEFINED)
>   			continue;
> @@ -265,9 +274,7 @@ int amdgpu_sync_resv(struct amdgpu_device *adev, struct amdgpu_sync *sync,
>   			break;
>   
>   		case AMDGPU_SYNC_EXPLICIT:
> -			if (owner != AMDGPU_FENCE_OWNER_UNDEFINED)
> -				continue;
> -			break;
> +			continue;
>   		}
>   
>   		r = amdgpu_sync_fence(sync, f, false);
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 5/5] drm/amdgpu: rework synchronization of VM updates v4

[Christian König]

If provided we only sync to the BOs reservation
object and no longer to the root PD.

v2: update comment, cleanup amdgpu_bo_sync_wait_resv
v3: use correct reservation object while clearing
v4: fix typo in amdgpu_bo_sync_wait_resv

Signed-off-by: Christian König <christian.koenig@amd.com>
Tested-by: Tom St Denis <tom.stdenis@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c  | 35 ++++++++++++++----
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.h  |  3 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c    |  7 ----
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c      | 41 ++++++++++++---------
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h      |  4 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c  | 22 +++--------
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 15 ++------
 7 files changed, 66 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 46c76e2e1281..6f60a581e3ba 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -1403,30 +1403,51 @@ void amdgpu_bo_fence(struct amdgpu_bo *bo, struct dma_fence *fence,
 }
 
 /**
- * amdgpu_sync_wait_resv - Wait for BO reservation fences
+ * amdgpu_bo_sync_wait_resv - Wait for BO reservation fences
  *
- * @bo: buffer object
+ * @adev: amdgpu device pointer
+ * @resv: reservation object to sync to
+ * @sync_mode: synchronization mode
  * @owner: fence owner
  * @intr: Whether the wait is interruptible
  *
+ * Extract the fences from the reservation object and waits for them to finish.
+ *
  * Returns:
  * 0 on success, errno otherwise.
  */
-int amdgpu_bo_sync_wait(struct amdgpu_bo *bo, void *owner, bool intr)
+int amdgpu_bo_sync_wait_resv(struct amdgpu_device *adev, struct dma_resv *resv,
+			     enum amdgpu_sync_mode sync_mode, void *owner,
+			     bool intr)
 {
-	struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
 	struct amdgpu_sync sync;
 	int r;
 
 	amdgpu_sync_create(&sync);
-	amdgpu_sync_resv(adev, &sync, bo->tbo.base.resv,
-			 AMDGPU_SYNC_NE_OWNER, owner);
+	amdgpu_sync_resv(adev, &sync, resv, sync_mode, owner);
 	r = amdgpu_sync_wait(&sync, intr);
 	amdgpu_sync_free(&sync);
-
 	return r;
 }
 
+/**
+ * amdgpu_bo_sync_wait - Wrapper for amdgpu_bo_sync_wait_resv
+ * @bo: buffer object to wait for
+ * @owner: fence owner
+ * @intr: Whether the wait is interruptible
+ *
+ * Wrapper to wait for fences in a BO.
+ * Returns:
+ * 0 on success, errno otherwise.
+ */
+int amdgpu_bo_sync_wait(struct amdgpu_bo *bo, void *owner, bool intr)
+{
+	struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
+
+	return amdgpu_bo_sync_wait_resv(adev, bo->tbo.base.resv,
+					AMDGPU_SYNC_NE_OWNER, owner, intr);
+}
+
 /**
  * amdgpu_bo_gpu_offset - return GPU offset of bo
  * @bo:	amdgpu object for which we query the offset
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
index 2eeafc77c9c1..96d805889e8d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
@@ -283,6 +283,9 @@ void amdgpu_bo_release_notify(struct ttm_buffer_object *bo);
 int amdgpu_bo_fault_reserve_notify(struct ttm_buffer_object *bo);
 void amdgpu_bo_fence(struct amdgpu_bo *bo, struct dma_fence *fence,
 		     bool shared);
+int amdgpu_bo_sync_wait_resv(struct amdgpu_device *adev, struct dma_resv *resv,
+			     enum amdgpu_sync_mode sync_mode, void *owner,
+			     bool intr);
 int amdgpu_bo_sync_wait(struct amdgpu_bo *bo, void *owner, bool intr);
 u64 amdgpu_bo_gpu_offset(struct amdgpu_bo *bo);
 int amdgpu_bo_validate(struct amdgpu_bo *bo);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
index 9f42032676da..b86392253696 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
@@ -249,13 +249,6 @@ int amdgpu_sync_resv(struct amdgpu_device *adev, struct amdgpu_sync *sync,
 		    owner != AMDGPU_FENCE_OWNER_UNDEFINED)
 			continue;
 
-		/* VM updates only sync with moves but not with user
-		 * command submissions or KFD evictions fences
-		 */
-		if (fence_owner != AMDGPU_FENCE_OWNER_UNDEFINED &&
-		    owner == AMDGPU_FENCE_OWNER_VM)
-			continue;
-
 		/* Ignore fences depending on the sync mode */
 		switch (mode) {
 		case AMDGPU_SYNC_ALWAYS:
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 6038b3c89633..aaae2b5e6eee 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -797,7 +797,7 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
 	params.vm = vm;
 	params.direct = direct;
 
-	r = vm->update_funcs->prepare(&params, AMDGPU_FENCE_OWNER_KFD, NULL);
+	r = vm->update_funcs->prepare(&params, NULL, AMDGPU_SYNC_EXPLICIT);
 	if (r)
 		return r;
 
@@ -1293,7 +1293,7 @@ int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
 	params.vm = vm;
 	params.direct = direct;
 
-	r = vm->update_funcs->prepare(&params, AMDGPU_FENCE_OWNER_VM, NULL);
+	r = vm->update_funcs->prepare(&params, NULL, AMDGPU_SYNC_EXPLICIT);
 	if (r)
 		return r;
 
@@ -1552,7 +1552,7 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
  * @adev: amdgpu_device pointer
  * @vm: requested vm
  * @direct: direct submission in a page fault
- * @exclusive: fence we need to sync to
+ * @resv: fences we need to sync to
  * @start: start of mapped range
  * @last: last mapped entry
  * @flags: flags for the entries
@@ -1567,14 +1567,14 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
  */
 static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
 				       struct amdgpu_vm *vm, bool direct,
-				       struct dma_fence *exclusive,
+				       struct dma_resv *resv,
 				       uint64_t start, uint64_t last,
 				       uint64_t flags, uint64_t addr,
 				       dma_addr_t *pages_addr,
 				       struct dma_fence **fence)
 {
 	struct amdgpu_vm_update_params params;
-	void *owner = AMDGPU_FENCE_OWNER_VM;
+	enum amdgpu_sync_mode sync_mode;
 	int r;
 
 	memset(&params, 0, sizeof(params));
@@ -1583,9 +1583,13 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
 	params.direct = direct;
 	params.pages_addr = pages_addr;
 
-	/* sync to everything except eviction fences on unmapping */
+	/* Implicitly sync to command submissions in the same VM before
+	 * unmapping. Sync to moving fences before mapping.
+	 */
 	if (!(flags & AMDGPU_PTE_VALID))
-		owner = AMDGPU_FENCE_OWNER_KFD;
+		sync_mode = AMDGPU_SYNC_EQ_OWNER;
+	else
+		sync_mode = AMDGPU_SYNC_EXPLICIT;
 
 	amdgpu_vm_eviction_lock(vm);
 	if (vm->evicting) {
@@ -1593,7 +1597,7 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
 		goto error_unlock;
 	}
 
-	r = vm->update_funcs->prepare(&params, owner, exclusive);
+	r = vm->update_funcs->prepare(&params, resv, sync_mode);
 	if (r)
 		goto error_unlock;
 
@@ -1612,7 +1616,7 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
  * amdgpu_vm_bo_split_mapping - split a mapping into smaller chunks
  *
  * @adev: amdgpu_device pointer
- * @exclusive: fence we need to sync to
+ * @resv: fences we need to sync to
  * @pages_addr: DMA addresses to use for mapping
  * @vm: requested vm
  * @mapping: mapped range and flags to use for the update
@@ -1628,7 +1632,7 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
  * 0 for success, -EINVAL for failure.
  */
 static int amdgpu_vm_bo_split_mapping(struct amdgpu_device *adev,
-				      struct dma_fence *exclusive,
+				      struct dma_resv *resv,
 				      dma_addr_t *pages_addr,
 				      struct amdgpu_vm *vm,
 				      struct amdgpu_bo_va_mapping *mapping,
@@ -1704,7 +1708,7 @@ static int amdgpu_vm_bo_split_mapping(struct amdgpu_device *adev,
 		}
 
 		last = min((uint64_t)mapping->last, start + max_entries - 1);
-		r = amdgpu_vm_bo_update_mapping(adev, vm, false, exclusive,
+		r = amdgpu_vm_bo_update_mapping(adev, vm, false, resv,
 						start, last, flags, addr,
 						dma_addr, fence);
 		if (r)
@@ -1743,7 +1747,8 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, struct amdgpu_bo_va *bo_va,
 	dma_addr_t *pages_addr = NULL;
 	struct ttm_mem_reg *mem;
 	struct drm_mm_node *nodes;
-	struct dma_fence *exclusive, **last_update;
+	struct dma_fence **last_update;
+	struct dma_resv *resv;
 	uint64_t flags;
 	struct amdgpu_device *bo_adev = adev;
 	int r;
@@ -1751,7 +1756,7 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, struct amdgpu_bo_va *bo_va,
 	if (clear || !bo) {
 		mem = NULL;
 		nodes = NULL;
-		exclusive = NULL;
+		resv = vm->root.base.bo->tbo.base.resv;
 	} else {
 		struct ttm_dma_tt *ttm;
 
@@ -1761,7 +1766,7 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, struct amdgpu_bo_va *bo_va,
 			ttm = container_of(bo->tbo.ttm, struct ttm_dma_tt, ttm);
 			pages_addr = ttm->dma_address;
 		}
-		exclusive = bo->tbo.moving;
+		resv = bo->tbo.base.resv;
 	}
 
 	if (bo) {
@@ -1775,7 +1780,8 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, struct amdgpu_bo_va *bo_va,
 		flags = 0x0;
 	}
 
-	if (clear || (bo && bo->tbo.base.resv == vm->root.base.bo->tbo.base.resv))
+	if (clear || (bo && bo->tbo.base.resv ==
+		      vm->root.base.bo->tbo.base.resv))
 		last_update = &vm->last_update;
 	else
 		last_update = &bo_va->last_pt_update;
@@ -1789,7 +1795,7 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, struct amdgpu_bo_va *bo_va,
 	}
 
 	list_for_each_entry(mapping, &bo_va->invalids, list) {
-		r = amdgpu_vm_bo_split_mapping(adev, exclusive, pages_addr, vm,
+		r = amdgpu_vm_bo_split_mapping(adev, resv, pages_addr, vm,
 					       mapping, flags, bo_adev, nodes,
 					       last_update);
 		if (r)
@@ -1984,6 +1990,7 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
 			  struct amdgpu_vm *vm,
 			  struct dma_fence **fence)
 {
+	struct dma_resv *resv = vm->root.base.bo->tbo.base.resv;
 	struct amdgpu_bo_va_mapping *mapping;
 	uint64_t init_pte_value = 0;
 	struct dma_fence *f = NULL;
@@ -1998,7 +2005,7 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
 		    mapping->start < AMDGPU_GMC_HOLE_START)
 			init_pte_value = AMDGPU_PTE_DEFAULT_ATC;
 
-		r = amdgpu_vm_bo_update_mapping(adev, vm, false, NULL,
+		r = amdgpu_vm_bo_update_mapping(adev, vm, false, resv,
 						mapping->start, mapping->last,
 						init_pte_value, 0, NULL, &f);
 		amdgpu_vm_free_mapping(adev, vm, mapping, f);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index c21a36bebc0c..b5705fcfc935 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -230,8 +230,8 @@ struct amdgpu_vm_update_params {
 
 struct amdgpu_vm_update_funcs {
 	int (*map_table)(struct amdgpu_bo *bo);
-	int (*prepare)(struct amdgpu_vm_update_params *p, void * owner,
-		       struct dma_fence *exclusive);
+	int (*prepare)(struct amdgpu_vm_update_params *p, struct dma_resv *resv,
+		       enum amdgpu_sync_mode sync_mode);
 	int (*update)(struct amdgpu_vm_update_params *p,
 		      struct amdgpu_bo *bo, uint64_t pe, uint64_t addr,
 		      unsigned count, uint32_t incr, uint64_t flags);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
index 68b013be3837..e38516304070 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
@@ -44,26 +44,14 @@ static int amdgpu_vm_cpu_map_table(struct amdgpu_bo *table)
  * Returns:
  * Negativ errno, 0 for success.
  */
-static int amdgpu_vm_cpu_prepare(struct amdgpu_vm_update_params *p, void *owner,
-				 struct dma_fence *exclusive)
+static int amdgpu_vm_cpu_prepare(struct amdgpu_vm_update_params *p,
+				 struct dma_resv *resv,
+				 enum amdgpu_sync_mode sync_mode)
 {
-	int r;
-
-	/* Wait for any BO move to be completed */
-	if (exclusive) {
-		r = dma_fence_wait(exclusive, true);
-		if (unlikely(r))
-			return r;
-	}
-
-	/* Don't wait for submissions during page fault */
-	if (p->direct)
+	if (!resv)
 		return 0;
 
-	/* Wait for PT BOs to be idle. PTs share the same resv. object
-	 * as the root PD BO
-	 */
-	return amdgpu_bo_sync_wait(p->vm->root.base.bo, owner, true);
+	return amdgpu_bo_sync_wait_resv(p->adev, resv, sync_mode, p->vm, true);
 }
 
 /**
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
index ab6481751763..4cc7881f438c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
@@ -58,9 +58,9 @@ static int amdgpu_vm_sdma_map_table(struct amdgpu_bo *table)
  * Negativ errno, 0 for success.
  */
 static int amdgpu_vm_sdma_prepare(struct amdgpu_vm_update_params *p,
-				  void *owner, struct dma_fence *exclusive)
+				  struct dma_resv *resv,
+				  enum amdgpu_sync_mode sync_mode)
 {
-	struct amdgpu_bo *root = p->vm->root.base.bo;
 	unsigned int ndw = AMDGPU_VM_SDMA_MIN_NUM_DW;
 	int r;
 
@@ -70,17 +70,10 @@ 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->job->sync, exclusive, false);
-	if (r)
-		return r;
-
-	/* Don't wait for any submissions during page fault handling */
-	if (p->direct)
+	if (!resv)
 		return 0;
 
-	return amdgpu_sync_resv(p->adev, &p->job->sync, root->tbo.base.resv,
-				AMDGPU_SYNC_NE_OWNER, owner);
+	return amdgpu_sync_resv(p->adev, &p->job->sync, resv, sync_mode, p->vm);
 }
 
 /**
-- 
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: rework synchronization of VM updates v4

[Felix Kuehling]

On 2020-01-30 7:49, Christian König wrote:
> If provided we only sync to the BOs reservation
> object and no longer to the root PD.
>
> v2: update comment, cleanup amdgpu_bo_sync_wait_resv
> v3: use correct reservation object while clearing
> v4: fix typo in amdgpu_bo_sync_wait_resv
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Tested-by: Tom St Denis <tom.stdenis@amd.com>

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


> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c  | 35 ++++++++++++++----
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.h  |  3 ++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c    |  7 ----
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c      | 41 ++++++++++++---------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h      |  4 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c  | 22 +++--------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 15 ++------
>   7 files changed, 66 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 46c76e2e1281..6f60a581e3ba 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -1403,30 +1403,51 @@ void amdgpu_bo_fence(struct amdgpu_bo *bo, struct dma_fence *fence,
>   }
>   
>   /**
> - * amdgpu_sync_wait_resv - Wait for BO reservation fences
> + * amdgpu_bo_sync_wait_resv - Wait for BO reservation fences
>    *
> - * @bo: buffer object
> + * @adev: amdgpu device pointer
> + * @resv: reservation object to sync to
> + * @sync_mode: synchronization mode
>    * @owner: fence owner
>    * @intr: Whether the wait is interruptible
>    *
> + * Extract the fences from the reservation object and waits for them to finish.
> + *
>    * Returns:
>    * 0 on success, errno otherwise.
>    */
> -int amdgpu_bo_sync_wait(struct amdgpu_bo *bo, void *owner, bool intr)
> +int amdgpu_bo_sync_wait_resv(struct amdgpu_device *adev, struct dma_resv *resv,
> +			     enum amdgpu_sync_mode sync_mode, void *owner,
> +			     bool intr)
>   {
> -	struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
>   	struct amdgpu_sync sync;
>   	int r;
>   
>   	amdgpu_sync_create(&sync);
> -	amdgpu_sync_resv(adev, &sync, bo->tbo.base.resv,
> -			 AMDGPU_SYNC_NE_OWNER, owner);
> +	amdgpu_sync_resv(adev, &sync, resv, sync_mode, owner);
>   	r = amdgpu_sync_wait(&sync, intr);
>   	amdgpu_sync_free(&sync);
> -
>   	return r;
>   }
>   
> +/**
> + * amdgpu_bo_sync_wait - Wrapper for amdgpu_bo_sync_wait_resv
> + * @bo: buffer object to wait for
> + * @owner: fence owner
> + * @intr: Whether the wait is interruptible
> + *
> + * Wrapper to wait for fences in a BO.
> + * Returns:
> + * 0 on success, errno otherwise.
> + */
> +int amdgpu_bo_sync_wait(struct amdgpu_bo *bo, void *owner, bool intr)
> +{
> +	struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
> +
> +	return amdgpu_bo_sync_wait_resv(adev, bo->tbo.base.resv,
> +					AMDGPU_SYNC_NE_OWNER, owner, intr);
> +}
> +
>   /**
>    * amdgpu_bo_gpu_offset - return GPU offset of bo
>    * @bo:	amdgpu object for which we query the offset
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> index 2eeafc77c9c1..96d805889e8d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> @@ -283,6 +283,9 @@ void amdgpu_bo_release_notify(struct ttm_buffer_object *bo);
>   int amdgpu_bo_fault_reserve_notify(struct ttm_buffer_object *bo);
>   void amdgpu_bo_fence(struct amdgpu_bo *bo, struct dma_fence *fence,
>   		     bool shared);
> +int amdgpu_bo_sync_wait_resv(struct amdgpu_device *adev, struct dma_resv *resv,
> +			     enum amdgpu_sync_mode sync_mode, void *owner,
> +			     bool intr);
>   int amdgpu_bo_sync_wait(struct amdgpu_bo *bo, void *owner, bool intr);
>   u64 amdgpu_bo_gpu_offset(struct amdgpu_bo *bo);
>   int amdgpu_bo_validate(struct amdgpu_bo *bo);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
> index 9f42032676da..b86392253696 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
> @@ -249,13 +249,6 @@ int amdgpu_sync_resv(struct amdgpu_device *adev, struct amdgpu_sync *sync,
>   		    owner != AMDGPU_FENCE_OWNER_UNDEFINED)
>   			continue;
>   
> -		/* VM updates only sync with moves but not with user
> -		 * command submissions or KFD evictions fences
> -		 */
> -		if (fence_owner != AMDGPU_FENCE_OWNER_UNDEFINED &&
> -		    owner == AMDGPU_FENCE_OWNER_VM)
> -			continue;
> -
>   		/* Ignore fences depending on the sync mode */
>   		switch (mode) {
>   		case AMDGPU_SYNC_ALWAYS:
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 6038b3c89633..aaae2b5e6eee 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -797,7 +797,7 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
>   	params.vm = vm;
>   	params.direct = direct;
>   
> -	r = vm->update_funcs->prepare(&params, AMDGPU_FENCE_OWNER_KFD, NULL);
> +	r = vm->update_funcs->prepare(&params, NULL, AMDGPU_SYNC_EXPLICIT);
>   	if (r)
>   		return r;
>   
> @@ -1293,7 +1293,7 @@ int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
>   	params.vm = vm;
>   	params.direct = direct;
>   
> -	r = vm->update_funcs->prepare(&params, AMDGPU_FENCE_OWNER_VM, NULL);
> +	r = vm->update_funcs->prepare(&params, NULL, AMDGPU_SYNC_EXPLICIT);
>   	if (r)
>   		return r;
>   
> @@ -1552,7 +1552,7 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
>    * @adev: amdgpu_device pointer
>    * @vm: requested vm
>    * @direct: direct submission in a page fault
> - * @exclusive: fence we need to sync to
> + * @resv: fences we need to sync to
>    * @start: start of mapped range
>    * @last: last mapped entry
>    * @flags: flags for the entries
> @@ -1567,14 +1567,14 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
>    */
>   static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
>   				       struct amdgpu_vm *vm, bool direct,
> -				       struct dma_fence *exclusive,
> +				       struct dma_resv *resv,
>   				       uint64_t start, uint64_t last,
>   				       uint64_t flags, uint64_t addr,
>   				       dma_addr_t *pages_addr,
>   				       struct dma_fence **fence)
>   {
>   	struct amdgpu_vm_update_params params;
> -	void *owner = AMDGPU_FENCE_OWNER_VM;
> +	enum amdgpu_sync_mode sync_mode;
>   	int r;
>   
>   	memset(&params, 0, sizeof(params));
> @@ -1583,9 +1583,13 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
>   	params.direct = direct;
>   	params.pages_addr = pages_addr;
>   
> -	/* sync to everything except eviction fences on unmapping */
> +	/* Implicitly sync to command submissions in the same VM before
> +	 * unmapping. Sync to moving fences before mapping.
> +	 */
>   	if (!(flags & AMDGPU_PTE_VALID))
> -		owner = AMDGPU_FENCE_OWNER_KFD;
> +		sync_mode = AMDGPU_SYNC_EQ_OWNER;
> +	else
> +		sync_mode = AMDGPU_SYNC_EXPLICIT;
>   
>   	amdgpu_vm_eviction_lock(vm);
>   	if (vm->evicting) {
> @@ -1593,7 +1597,7 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
>   		goto error_unlock;
>   	}
>   
> -	r = vm->update_funcs->prepare(&params, owner, exclusive);
> +	r = vm->update_funcs->prepare(&params, resv, sync_mode);
>   	if (r)
>   		goto error_unlock;
>   
> @@ -1612,7 +1616,7 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
>    * amdgpu_vm_bo_split_mapping - split a mapping into smaller chunks
>    *
>    * @adev: amdgpu_device pointer
> - * @exclusive: fence we need to sync to
> + * @resv: fences we need to sync to
>    * @pages_addr: DMA addresses to use for mapping
>    * @vm: requested vm
>    * @mapping: mapped range and flags to use for the update
> @@ -1628,7 +1632,7 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
>    * 0 for success, -EINVAL for failure.
>    */
>   static int amdgpu_vm_bo_split_mapping(struct amdgpu_device *adev,
> -				      struct dma_fence *exclusive,
> +				      struct dma_resv *resv,
>   				      dma_addr_t *pages_addr,
>   				      struct amdgpu_vm *vm,
>   				      struct amdgpu_bo_va_mapping *mapping,
> @@ -1704,7 +1708,7 @@ static int amdgpu_vm_bo_split_mapping(struct amdgpu_device *adev,
>   		}
>   
>   		last = min((uint64_t)mapping->last, start + max_entries - 1);
> -		r = amdgpu_vm_bo_update_mapping(adev, vm, false, exclusive,
> +		r = amdgpu_vm_bo_update_mapping(adev, vm, false, resv,
>   						start, last, flags, addr,
>   						dma_addr, fence);
>   		if (r)
> @@ -1743,7 +1747,8 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, struct amdgpu_bo_va *bo_va,
>   	dma_addr_t *pages_addr = NULL;
>   	struct ttm_mem_reg *mem;
>   	struct drm_mm_node *nodes;
> -	struct dma_fence *exclusive, **last_update;
> +	struct dma_fence **last_update;
> +	struct dma_resv *resv;
>   	uint64_t flags;
>   	struct amdgpu_device *bo_adev = adev;
>   	int r;
> @@ -1751,7 +1756,7 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, struct amdgpu_bo_va *bo_va,
>   	if (clear || !bo) {
>   		mem = NULL;
>   		nodes = NULL;
> -		exclusive = NULL;
> +		resv = vm->root.base.bo->tbo.base.resv;
>   	} else {
>   		struct ttm_dma_tt *ttm;
>   
> @@ -1761,7 +1766,7 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, struct amdgpu_bo_va *bo_va,
>   			ttm = container_of(bo->tbo.ttm, struct ttm_dma_tt, ttm);
>   			pages_addr = ttm->dma_address;
>   		}
> -		exclusive = bo->tbo.moving;
> +		resv = bo->tbo.base.resv;
>   	}
>   
>   	if (bo) {
> @@ -1775,7 +1780,8 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, struct amdgpu_bo_va *bo_va,
>   		flags = 0x0;
>   	}
>   
> -	if (clear || (bo && bo->tbo.base.resv == vm->root.base.bo->tbo.base.resv))
> +	if (clear || (bo && bo->tbo.base.resv ==
> +		      vm->root.base.bo->tbo.base.resv))
>   		last_update = &vm->last_update;
>   	else
>   		last_update = &bo_va->last_pt_update;
> @@ -1789,7 +1795,7 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, struct amdgpu_bo_va *bo_va,
>   	}
>   
>   	list_for_each_entry(mapping, &bo_va->invalids, list) {
> -		r = amdgpu_vm_bo_split_mapping(adev, exclusive, pages_addr, vm,
> +		r = amdgpu_vm_bo_split_mapping(adev, resv, pages_addr, vm,
>   					       mapping, flags, bo_adev, nodes,
>   					       last_update);
>   		if (r)
> @@ -1984,6 +1990,7 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
>   			  struct amdgpu_vm *vm,
>   			  struct dma_fence **fence)
>   {
> +	struct dma_resv *resv = vm->root.base.bo->tbo.base.resv;
>   	struct amdgpu_bo_va_mapping *mapping;
>   	uint64_t init_pte_value = 0;
>   	struct dma_fence *f = NULL;
> @@ -1998,7 +2005,7 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
>   		    mapping->start < AMDGPU_GMC_HOLE_START)
>   			init_pte_value = AMDGPU_PTE_DEFAULT_ATC;
>   
> -		r = amdgpu_vm_bo_update_mapping(adev, vm, false, NULL,
> +		r = amdgpu_vm_bo_update_mapping(adev, vm, false, resv,
>   						mapping->start, mapping->last,
>   						init_pte_value, 0, NULL, &f);
>   		amdgpu_vm_free_mapping(adev, vm, mapping, f);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index c21a36bebc0c..b5705fcfc935 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -230,8 +230,8 @@ struct amdgpu_vm_update_params {
>   
>   struct amdgpu_vm_update_funcs {
>   	int (*map_table)(struct amdgpu_bo *bo);
> -	int (*prepare)(struct amdgpu_vm_update_params *p, void * owner,
> -		       struct dma_fence *exclusive);
> +	int (*prepare)(struct amdgpu_vm_update_params *p, struct dma_resv *resv,
> +		       enum amdgpu_sync_mode sync_mode);
>   	int (*update)(struct amdgpu_vm_update_params *p,
>   		      struct amdgpu_bo *bo, uint64_t pe, uint64_t addr,
>   		      unsigned count, uint32_t incr, uint64_t flags);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
> index 68b013be3837..e38516304070 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
> @@ -44,26 +44,14 @@ static int amdgpu_vm_cpu_map_table(struct amdgpu_bo *table)
>    * Returns:
>    * Negativ errno, 0 for success.
>    */
> -static int amdgpu_vm_cpu_prepare(struct amdgpu_vm_update_params *p, void *owner,
> -				 struct dma_fence *exclusive)
> +static int amdgpu_vm_cpu_prepare(struct amdgpu_vm_update_params *p,
> +				 struct dma_resv *resv,
> +				 enum amdgpu_sync_mode sync_mode)
>   {
> -	int r;
> -
> -	/* Wait for any BO move to be completed */
> -	if (exclusive) {
> -		r = dma_fence_wait(exclusive, true);
> -		if (unlikely(r))
> -			return r;
> -	}
> -
> -	/* Don't wait for submissions during page fault */
> -	if (p->direct)
> +	if (!resv)
>   		return 0;
>   
> -	/* Wait for PT BOs to be idle. PTs share the same resv. object
> -	 * as the root PD BO
> -	 */
> -	return amdgpu_bo_sync_wait(p->vm->root.base.bo, owner, true);
> +	return amdgpu_bo_sync_wait_resv(p->adev, resv, sync_mode, p->vm, true);
>   }
>   
>   /**
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> index ab6481751763..4cc7881f438c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> @@ -58,9 +58,9 @@ static int amdgpu_vm_sdma_map_table(struct amdgpu_bo *table)
>    * Negativ errno, 0 for success.
>    */
>   static int amdgpu_vm_sdma_prepare(struct amdgpu_vm_update_params *p,
> -				  void *owner, struct dma_fence *exclusive)
> +				  struct dma_resv *resv,
> +				  enum amdgpu_sync_mode sync_mode)
>   {
> -	struct amdgpu_bo *root = p->vm->root.base.bo;
>   	unsigned int ndw = AMDGPU_VM_SDMA_MIN_NUM_DW;
>   	int r;
>   
> @@ -70,17 +70,10 @@ 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->job->sync, exclusive, false);
> -	if (r)
> -		return r;
> -
> -	/* Don't wait for any submissions during page fault handling */
> -	if (p->direct)
> +	if (!resv)
>   		return 0;
>   
> -	return amdgpu_sync_resv(p->adev, &p->job->sync, root->tbo.base.resv,
> -				AMDGPU_SYNC_NE_OWNER, owner);
> +	return amdgpu_sync_resv(p->adev, &p->job->sync, resv, sync_mode, p->vm);
>   }
>   
>   /**
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/5] drm/amdgpu: fix braces in amdgpu_vm_update_ptes

[Felix Kuehling]

On 2020-01-30 7:49, Christian König wrote:
> For the root PD mask can be 0xffffffff as well which would
> overrun to 0 if we don't cast it before we add one.
You're fixing parentheses, not braces.

Parentheses: ()
Brackets: []
Braces: {}

With the title fixed, this patch is

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

>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Tested-by: Tom St Denis <tom.stdenis@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 5cb182231f5d..4ba6a5e5d094 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1487,7 +1487,7 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
>   		incr = (uint64_t)AMDGPU_GPU_PAGE_SIZE << shift;
>   		mask = amdgpu_vm_entries_mask(adev, cursor.level);
>   		pe_start = ((cursor.pfn >> shift) & mask) * 8;
> -		entry_end = (uint64_t)(mask + 1) << shift;
> +		entry_end = ((uint64_t)mask + 1) << shift;
>   		entry_end += cursor.pfn & ~(entry_end - 1);
>   		entry_end = min(entry_end, end);
>   
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/5] drm/amdgpu: fix braces in amdgpu_vm_update_ptes

[Christian König]

Am 30.01.20 um 23:11 schrieb Felix Kuehling:
>
> On 2020-01-30 7:49, Christian König wrote:
>> For the root PD mask can be 0xffffffff as well which would
>> overrun to 0 if we don't cast it before we add one.
> You're fixing parentheses, not braces.
>
> Parentheses: ()
> Brackets: []
> Braces: {}

Yeah, I can't remember which is what in English. Need to double check 
that next time.

>
> With the title fixed, this patch is
>
> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>

Thanks for the review,
Christian.

>
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> Tested-by: Tom St Denis <tom.stdenis@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 5cb182231f5d..4ba6a5e5d094 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -1487,7 +1487,7 @@ static int amdgpu_vm_update_ptes(struct 
>> amdgpu_vm_update_params *params,
>>           incr = (uint64_t)AMDGPU_GPU_PAGE_SIZE << shift;
>>           mask = amdgpu_vm_entries_mask(adev, cursor.level);
>>           pe_start = ((cursor.pfn >> shift) & mask) * 8;
>> -        entry_end = (uint64_t)(mask + 1) << shift;
>> +        entry_end = ((uint64_t)mask + 1) << shift;
>>           entry_end += cursor.pfn & ~(entry_end - 1);
>>           entry_end = min(entry_end, end);

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