[PATCH 1/8] drm/amdgpu: fix waiting for BO moves with CPU based PD/PT updates

[PATCH 1/8] drm/amdgpu: fix waiting for BO moves with CPU based PD/PT updates

[Christian König]

Otherwise we open up the possibility to use uninitialized memory.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index c70621db3bb7..44950f3b8801 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1779,13 +1779,18 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
 		if (pages_addr)
 			params.src = ~0;
 
-		/* Wait for PT BOs to be free. PTs share the same resv. object
+		/* Wait for PT BOs to be idle. PTs share the same resv. object
 		 * as the root PD BO
 		 */
 		r = amdgpu_vm_wait_pd(adev, vm, owner);
 		if (unlikely(r))
 			return r;
 
+		/* Wait for any BO move to be completed */
+		r = dma_fence_wait(exclusive, true);
+		if (unlikely(r))
+			return r;
+
 		params.func = amdgpu_vm_cpu_set_ptes;
 		params.pages_addr = pages_addr;
 		return amdgpu_vm_update_ptes(&params, start, last + 1,
-- 
2.17.1

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

[PATCH 2/8] drm/amdgpu: cleanup VM dw estimation a bit

[Christian König]

No functional change.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 44950f3b8801..69b0bee0661e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1804,13 +1804,12 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
 	/*
 	 * reserve space for two commands every (1 << BLOCK_SIZE)
 	 *  entries or 2k dwords (whatever is smaller)
-         *
-         * The second command is for the shadow pagetables.
 	 */
+	ncmds = ((nptes >> min(adev->vm_manager.block_size, 11u)) + 1);
+
+         /* The second command is for the shadow pagetables. */
 	if (vm->root.base.bo->shadow)
-		ncmds = ((nptes >> min(adev->vm_manager.block_size, 11u)) + 1) * 2;
-	else
-		ncmds = ((nptes >> min(adev->vm_manager.block_size, 11u)) + 1);
+		ncmds *= 2;
 
 	/* padding, etc. */
 	ndw = 64;
@@ -1829,10 +1828,11 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
 		ndw += ncmds * 10;
 
 		/* extra commands for begin/end fragments */
+		ncmds = 2 * adev->vm_manager.fragment_size;
 		if (vm->root.base.bo->shadow)
-		        ndw += 2 * 10 * adev->vm_manager.fragment_size * 2;
-		else
-		        ndw += 2 * 10 * adev->vm_manager.fragment_size;
+			ncmds *= 2;
+
+		ndw += 10 * ncmds;
 
 		params.func = amdgpu_vm_do_set_ptes;
 	}
-- 
2.17.1

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

Re: [PATCH 2/8] drm/amdgpu: cleanup VM dw estimation a bit

[Kuehling, Felix]

On 2019-02-04 7:42 a.m., Christian König wrote:
> No functional change.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>

Minor indentation issue inline. With that fixed, this patch is 
Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>


> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 44950f3b8801..69b0bee0661e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1804,13 +1804,12 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
>   	/*
>   	 * reserve space for two commands every (1 << BLOCK_SIZE)
>   	 *  entries or 2k dwords (whatever is smaller)
> -         *
> -         * The second command is for the shadow pagetables.
>   	 */
> +	ncmds = ((nptes >> min(adev->vm_manager.block_size, 11u)) + 1);
> +
> +         /* The second command is for the shadow pagetables. */

Using space instead of TABs.


>   	if (vm->root.base.bo->shadow)
> -		ncmds = ((nptes >> min(adev->vm_manager.block_size, 11u)) + 1) * 2;
> -	else
> -		ncmds = ((nptes >> min(adev->vm_manager.block_size, 11u)) + 1);
> +		ncmds *= 2;
>   
>   	/* padding, etc. */
>   	ndw = 64;
> @@ -1829,10 +1828,11 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
>   		ndw += ncmds * 10;
>   
>   		/* extra commands for begin/end fragments */
> +		ncmds = 2 * adev->vm_manager.fragment_size;
>   		if (vm->root.base.bo->shadow)
> -		        ndw += 2 * 10 * adev->vm_manager.fragment_size * 2;
> -		else
> -		        ndw += 2 * 10 * adev->vm_manager.fragment_size;
> +			ncmds *= 2;
> +
> +		ndw += 10 * ncmds;
>   
>   		params.func = amdgpu_vm_do_set_ptes;
>   	}
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 3/8] drm/amdgpu: clear PDs/PTs only after initializing them

[Christian König]

Clear the VM PDs/PTs only after initializing all the structures.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 69b0bee0661e..a52444f4bd38 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -945,10 +945,6 @@ int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
 		if (r)
 			return r;
 
-		r = amdgpu_vm_clear_bo(adev, vm, pt, cursor.level, ats);
-		if (r)
-			goto error_free_pt;
-
 		if (vm->use_cpu_for_update) {
 			r = amdgpu_bo_kmap(pt, NULL);
 			if (r)
@@ -961,6 +957,10 @@ int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
 		pt->parent = amdgpu_bo_ref(cursor.parent->base.bo);
 
 		amdgpu_vm_bo_base_init(&entry->base, vm, pt);
+
+		r = amdgpu_vm_clear_bo(adev, vm, pt, cursor.level, ats);
+		if (r)
+			goto error_free_pt;
 	}
 
 	return 0;
@@ -3053,13 +3053,14 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 	if (r)
 		goto error_unreserve;
 
+	amdgpu_vm_bo_base_init(&vm->root.base, vm, root);
+
 	r = amdgpu_vm_clear_bo(adev, vm, root,
 			       adev->vm_manager.root_level,
 			       vm->pte_support_ats);
 	if (r)
 		goto error_unreserve;
 
-	amdgpu_vm_bo_base_init(&vm->root.base, vm, root);
 	amdgpu_bo_unreserve(vm->root.base.bo);
 
 	if (pasid) {
-- 
2.17.1

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

[PATCH 4/8] drm/amdgpu: rework shadow handling during PD clear

[Christian König]

This way we only deal with the real BO in here.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index a52444f4bd38..283a9e9878be 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -788,38 +788,58 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
 
 	r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
 	if (r)
-		goto error;
+		return r;
 
 	r = amdgpu_ttm_alloc_gart(&bo->tbo);
 	if (r)
 		return r;
 
+	if (bo->shadow) {
+		r = ttm_bo_validate(&bo->shadow->tbo, &bo->shadow->placement,
+				    &ctx);
+		if (r)
+			return r;
+
+		r = amdgpu_ttm_alloc_gart(&bo->shadow->tbo);
+		if (r)
+			return r;
+
+	}
+
 	r = amdgpu_job_alloc_with_ib(adev, 64, &job);
 	if (r)
-		goto error;
+		return r;
 
-	addr = amdgpu_bo_gpu_offset(bo);
-	if (ats_entries) {
-		uint64_t ats_value;
+	while (1) {
+		addr = amdgpu_bo_gpu_offset(bo);
+		if (ats_entries) {
+			uint64_t ats_value;
 
-		ats_value = AMDGPU_PTE_DEFAULT_ATC;
-		if (level != AMDGPU_VM_PTB)
-			ats_value |= AMDGPU_PDE_PTE;
+			ats_value = AMDGPU_PTE_DEFAULT_ATC;
+			if (level != AMDGPU_VM_PTB)
+				ats_value |= AMDGPU_PDE_PTE;
 
-		amdgpu_vm_set_pte_pde(adev, &job->ibs[0], addr, 0,
-				      ats_entries, 0, ats_value);
-		addr += ats_entries * 8;
-	}
+			amdgpu_vm_set_pte_pde(adev, &job->ibs[0], addr, 0,
+					      ats_entries, 0, ats_value);
+			addr += ats_entries * 8;
+		}
 
-	if (entries) {
-		uint64_t value = 0;
+		if (entries) {
+			uint64_t value = 0;
 
-		/* Workaround for fault priority problem on GMC9 */
-		if (level == AMDGPU_VM_PTB && adev->asic_type >= CHIP_VEGA10)
-			value = AMDGPU_PTE_EXECUTABLE;
+			/* Workaround for fault priority problem on GMC9 */
+			if (level == AMDGPU_VM_PTB &&
+			    adev->asic_type >= CHIP_VEGA10)
+				value = AMDGPU_PTE_EXECUTABLE;
+
+			amdgpu_vm_set_pte_pde(adev, &job->ibs[0], addr, 0,
+					      entries, 0, value);
+		}
 
-		amdgpu_vm_set_pte_pde(adev, &job->ibs[0], addr, 0,
-				      entries, 0, value);
+		if (bo->shadow)
+			bo = bo->shadow;
+		else
+			break;
 	}
 
 	amdgpu_ring_pad_ib(ring, &job->ibs[0]);
@@ -838,16 +858,10 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
 	amdgpu_bo_fence(bo, fence, true);
 	dma_fence_put(fence);
 
-	if (bo->shadow)
-		return amdgpu_vm_clear_bo(adev, vm, bo->shadow,
-					  level, pte_support_ats);
-
 	return 0;
 
 error_free:
 	amdgpu_job_free(job);
-
-error:
 	return r;
 }
 
-- 
2.17.1

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

[PATCH 5/8] drm/amdgpu: let amdgpu_vm_clear_bo figure out ats status

[Christian König]

Instead of providing it from outside figure out the ats status in the
function itself from the data structures.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 283a9e9878be..f7d410a5d848 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -747,8 +747,6 @@ bool amdgpu_vm_ready(struct amdgpu_vm *vm)
  * @adev: amdgpu_device pointer
  * @vm: VM to clear BO from
  * @bo: BO to clear
- * @level: level this BO is at
- * @pte_support_ats: indicate ATS support from PTE
  *
  * Root PD needs to be reserved when calling this.
  *
@@ -756,10 +754,11 @@ bool amdgpu_vm_ready(struct amdgpu_vm *vm)
  * 0 on success, errno otherwise.
  */
 static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
-			      struct amdgpu_vm *vm, struct amdgpu_bo *bo,
-			      unsigned level, bool pte_support_ats)
+			      struct amdgpu_vm *vm,
+			      struct amdgpu_bo *bo)
 {
 	struct ttm_operation_ctx ctx = { true, false };
+	unsigned level = adev->vm_manager.root_level;
 	struct dma_fence *fence = NULL;
 	unsigned entries, ats_entries;
 	struct amdgpu_ring *ring;
@@ -768,17 +767,32 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
 	int r;
 
 	entries = amdgpu_bo_size(bo) / 8;
+	if (vm->pte_support_ats) {
+		ats_entries = amdgpu_vm_level_shift(adev, level);
+		ats_entries += AMDGPU_GPU_PAGE_SHIFT;
+		ats_entries = AMDGPU_GMC_HOLE_START >> ats_entries;
 
-	if (pte_support_ats) {
-		if (level == adev->vm_manager.root_level) {
-			ats_entries = amdgpu_vm_level_shift(adev, level);
-			ats_entries += AMDGPU_GPU_PAGE_SHIFT;
-			ats_entries = AMDGPU_GMC_HOLE_START >> ats_entries;
+		if (!bo->parent) {
 			ats_entries = min(ats_entries, entries);
 			entries -= ats_entries;
 		} else {
-			ats_entries = entries;
-			entries = 0;
+			struct amdgpu_bo *ancestor = bo;
+			struct amdgpu_vm_pt *pt;
+
+			++level;
+			while (ancestor->parent->parent) {
+				ancestor = ancestor->parent;
+				++level;
+			}
+
+			pt = container_of(ancestor->vm_bo, struct amdgpu_vm_pt,
+					  base);
+			if ((pt - vm->root.entries) >= ats_entries) {
+				ats_entries = 0;
+			} else {
+				ats_entries = entries;
+				entries = 0;
+			}
 		}
 	} else {
 		ats_entries = 0;
@@ -911,7 +925,6 @@ int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
 {
 	struct amdgpu_vm_pt_cursor cursor;
 	struct amdgpu_bo *pt;
-	bool ats = false;
 	uint64_t eaddr;
 	int r;
 
@@ -921,9 +934,6 @@ int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
 
 	eaddr = saddr + size - 1;
 
-	if (vm->pte_support_ats)
-		ats = saddr < AMDGPU_GMC_HOLE_START;
-
 	saddr /= AMDGPU_GPU_PAGE_SIZE;
 	eaddr /= AMDGPU_GPU_PAGE_SIZE;
 
@@ -972,7 +982,7 @@ int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
 
 		amdgpu_vm_bo_base_init(&entry->base, vm, pt);
 
-		r = amdgpu_vm_clear_bo(adev, vm, pt, cursor.level, ats);
+		r = amdgpu_vm_clear_bo(adev, vm, pt);
 		if (r)
 			goto error_free_pt;
 	}
@@ -3069,9 +3079,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 
 	amdgpu_vm_bo_base_init(&vm->root.base, vm, root);
 
-	r = amdgpu_vm_clear_bo(adev, vm, root,
-			       adev->vm_manager.root_level,
-			       vm->pte_support_ats);
+	r = amdgpu_vm_clear_bo(adev, vm, root);
 	if (r)
 		goto error_unreserve;
 
@@ -3166,9 +3174,8 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm, uns
 	 * changing any other state, in case it fails.
 	 */
 	if (pte_support_ats != vm->pte_support_ats) {
-		r = amdgpu_vm_clear_bo(adev, vm, vm->root.base.bo,
-			       adev->vm_manager.root_level,
-			       pte_support_ats);
+		vm->pte_support_ats = pte_support_ats;
+		r = amdgpu_vm_clear_bo(adev, vm, vm->root.base.bo);
 		if (r)
 			goto free_idr;
 	}
@@ -3176,7 +3183,6 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm, uns
 	/* Update VM state */
 	vm->use_cpu_for_update = !!(adev->vm_manager.vm_update_mode &
 				    AMDGPU_VM_USE_CPU_FOR_COMPUTE);
-	vm->pte_support_ats = pte_support_ats;
 	DRM_DEBUG_DRIVER("VM update mode is %s\n",
 			 vm->use_cpu_for_update ? "CPU" : "SDMA");
 	WARN_ONCE((vm->use_cpu_for_update && !amdgpu_gmc_vram_full_visible(&adev->gmc)),
-- 
2.17.1

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

[PATCH 6/8] drm/amdgpu: allocate VM PDs/PTs on demand

[Christian König]

Let's start to allocate VM PDs/PTs on demand instead of pre-allocating
them during mapping.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  |   6 -
 drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c       |   9 --
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c       |  10 --
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        | 136 +++++-------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |   3 -
 5 files changed, 38 insertions(+), 126 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index d7b10d79f1de..2176c92f9377 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -492,12 +492,6 @@ static int add_bo_to_vm(struct amdgpu_device *adev, struct kgd_mem *mem,
 					vm->process_info->eviction_fence,
 					NULL, NULL);
 
-	ret = amdgpu_vm_alloc_pts(adev, vm, va, amdgpu_bo_size(bo));
-	if (ret) {
-		pr_err("Failed to allocate pts, err=%d\n", ret);
-		goto err_alloc_pts;
-	}
-
 	ret = vm_validate_pt_pd_bos(vm);
 	if (ret) {
 		pr_err("validate_pt_pd_bos() failed\n");
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
index 7e22be7ca68a..54dd02a898b9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
@@ -92,15 +92,6 @@ int amdgpu_map_static_csa(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 		return -ENOMEM;
 	}
 
-	r = amdgpu_vm_alloc_pts(adev, (*bo_va)->base.vm, csa_addr,
-				size);
-	if (r) {
-		DRM_ERROR("failed to allocate pts for static CSA, err=%d\n", r);
-		amdgpu_vm_bo_rmv(adev, *bo_va);
-		ttm_eu_backoff_reservation(&ticket, &list);
-		return r;
-	}
-
 	r = amdgpu_vm_bo_map(adev, *bo_va, csa_addr, 0, size,
 			     AMDGPU_PTE_READABLE | AMDGPU_PTE_WRITEABLE |
 			     AMDGPU_PTE_EXECUTABLE);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index d21dd2f369da..e141e3b13112 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -627,11 +627,6 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
 
 	switch (args->operation) {
 	case AMDGPU_VA_OP_MAP:
-		r = amdgpu_vm_alloc_pts(adev, bo_va->base.vm, args->va_address,
-					args->map_size);
-		if (r)
-			goto error_backoff;
-
 		va_flags = amdgpu_gmc_get_pte_flags(adev, args->flags);
 		r = amdgpu_vm_bo_map(adev, bo_va, args->va_address,
 				     args->offset_in_bo, args->map_size,
@@ -647,11 +642,6 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
 						args->map_size);
 		break;
 	case AMDGPU_VA_OP_REPLACE:
-		r = amdgpu_vm_alloc_pts(adev, bo_va->base.vm, args->va_address,
-					args->map_size);
-		if (r)
-			goto error_backoff;
-
 		va_flags = amdgpu_gmc_get_pte_flags(adev, args->flags);
 		r = amdgpu_vm_bo_replace_map(adev, bo_va, args->va_address,
 					     args->offset_in_bo, args->map_size,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index f7d410a5d848..0b8a1aa56c4a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -504,47 +504,6 @@ static void amdgpu_vm_pt_next(struct amdgpu_device *adev,
 	}
 }
 
-/**
- * amdgpu_vm_pt_first_leaf - get first leaf PD/PT
- *
- * @adev: amdgpu_device pointer
- * @vm: amdgpu_vm structure
- * @start: start addr of the walk
- * @cursor: state to initialize
- *
- * Start a walk and go directly to the leaf node.
- */
-static void amdgpu_vm_pt_first_leaf(struct amdgpu_device *adev,
-				    struct amdgpu_vm *vm, uint64_t start,
-				    struct amdgpu_vm_pt_cursor *cursor)
-{
-	amdgpu_vm_pt_start(adev, vm, start, cursor);
-	while (amdgpu_vm_pt_descendant(adev, cursor));
-}
-
-/**
- * amdgpu_vm_pt_next_leaf - get next leaf PD/PT
- *
- * @adev: amdgpu_device pointer
- * @cursor: current state
- *
- * Walk the PD/PT tree to the next leaf node.
- */
-static void amdgpu_vm_pt_next_leaf(struct amdgpu_device *adev,
-				   struct amdgpu_vm_pt_cursor *cursor)
-{
-	amdgpu_vm_pt_next(adev, cursor);
-	if (cursor->pfn != ~0ll)
-		while (amdgpu_vm_pt_descendant(adev, cursor));
-}
-
-/**
- * for_each_amdgpu_vm_pt_leaf - walk over all leaf PDs/PTs in the hierarchy
- */
-#define for_each_amdgpu_vm_pt_leaf(adev, vm, start, end, cursor)		\
-	for (amdgpu_vm_pt_first_leaf((adev), (vm), (start), &(cursor));		\
-	     (cursor).pfn <= end; amdgpu_vm_pt_next_leaf((adev), &(cursor)))
-
 /**
  * amdgpu_vm_pt_first_dfs - start a deep first search
  *
@@ -919,74 +878,51 @@ static void amdgpu_vm_bo_param(struct amdgpu_device *adev, struct amdgpu_vm *vm,
  * Returns:
  * 0 on success, errno otherwise.
  */
-int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
-			struct amdgpu_vm *vm,
-			uint64_t saddr, uint64_t size)
+static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
+			       struct amdgpu_vm *vm,
+			       struct amdgpu_vm_pt_cursor *cursor)
 {
-	struct amdgpu_vm_pt_cursor cursor;
+	struct amdgpu_vm_pt *entry = cursor->entry;
+	struct amdgpu_bo_param bp;
 	struct amdgpu_bo *pt;
-	uint64_t eaddr;
 	int r;
 
-	/* validate the parameters */
-	if (saddr & AMDGPU_GPU_PAGE_MASK || size & AMDGPU_GPU_PAGE_MASK)
-		return -EINVAL;
+	if (cursor->level < AMDGPU_VM_PTB && !entry->entries) {
+		unsigned num_entries;
 
-	eaddr = saddr + size - 1;
-
-	saddr /= AMDGPU_GPU_PAGE_SIZE;
-	eaddr /= AMDGPU_GPU_PAGE_SIZE;
-
-	if (eaddr >= adev->vm_manager.max_pfn) {
-		dev_err(adev->dev, "va above limit (0x%08llX >= 0x%08llX)\n",
-			eaddr, adev->vm_manager.max_pfn);
-		return -EINVAL;
+		num_entries = amdgpu_vm_num_entries(adev, cursor->level);
+		entry->entries = kvmalloc_array(num_entries,
+						sizeof(*entry->entries),
+						GFP_KERNEL | __GFP_ZERO);
+		if (!entry->entries)
+			return -ENOMEM;
 	}
 
-	for_each_amdgpu_vm_pt_leaf(adev, vm, saddr, eaddr, cursor) {
-		struct amdgpu_vm_pt *entry = cursor.entry;
-		struct amdgpu_bo_param bp;
-
-		if (cursor.level < AMDGPU_VM_PTB) {
-			unsigned num_entries;
-
-			num_entries = amdgpu_vm_num_entries(adev, cursor.level);
-			entry->entries = kvmalloc_array(num_entries,
-							sizeof(*entry->entries),
-							GFP_KERNEL |
-							__GFP_ZERO);
-			if (!entry->entries)
-				return -ENOMEM;
-		}
-
-
-		if (entry->base.bo)
-			continue;
-
-		amdgpu_vm_bo_param(adev, vm, cursor.level, &bp);
-
-		r = amdgpu_bo_create(adev, &bp, &pt);
-		if (r)
-			return r;
-
-		if (vm->use_cpu_for_update) {
-			r = amdgpu_bo_kmap(pt, NULL);
-			if (r)
-				goto error_free_pt;
-		}
+	if (entry->base.bo)
+		return 0;
 
-		/* Keep a reference to the root directory to avoid
-		* freeing them up in the wrong order.
-		*/
-		pt->parent = amdgpu_bo_ref(cursor.parent->base.bo);
+	amdgpu_vm_bo_param(adev, vm, cursor->level, &bp);
 
-		amdgpu_vm_bo_base_init(&entry->base, vm, pt);
+	r = amdgpu_bo_create(adev, &bp, &pt);
+	if (r)
+		return r;
 
-		r = amdgpu_vm_clear_bo(adev, vm, pt);
+	if (vm->use_cpu_for_update) {
+		r = amdgpu_bo_kmap(pt, NULL);
 		if (r)
 			goto error_free_pt;
 	}
 
+	/* Keep a reference to the root directory to avoid
+	 * freeing them up in the wrong order.
+	 */
+	pt->parent = amdgpu_bo_ref(cursor->parent->base.bo);
+	amdgpu_vm_bo_base_init(&entry->base, vm, pt);
+
+	r = amdgpu_vm_clear_bo(adev, vm, pt);
+	if (r)
+		goto error_free_pt;
+
 	return 0;
 
 error_free_pt:
@@ -1655,6 +1591,7 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
 	struct amdgpu_vm_pt_cursor cursor;
 	uint64_t frag_start = start, frag_end;
 	unsigned int frag;
+	int r;
 
 	/* figure out the initial fragment */
 	amdgpu_vm_fragment(params, frag_start, end, flags, &frag, &frag_end);
@@ -1662,12 +1599,15 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
 	/* walk over the address space and update the PTs */
 	amdgpu_vm_pt_start(adev, params->vm, start, &cursor);
 	while (cursor.pfn < end) {
-		struct amdgpu_bo *pt = cursor.entry->base.bo;
 		unsigned shift, parent_shift, mask;
 		uint64_t incr, entry_end, pe_start;
+		struct amdgpu_bo *pt;
 
-		if (!pt)
-			return -ENOENT;
+		r = amdgpu_vm_alloc_pts(params->adev, params->vm, &cursor);
+		if (r)
+			return r;
+
+		pt = cursor.entry->base.bo;
 
 		/* The root level can't be a huge page */
 		if (cursor.level == adev->vm_manager.root_level) {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 81ff8177f092..116605c038d2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -303,9 +303,6 @@ bool amdgpu_vm_ready(struct amdgpu_vm *vm);
 int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 			      int (*callback)(void *p, struct amdgpu_bo *bo),
 			      void *param);
-int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
-			struct amdgpu_vm *vm,
-			uint64_t saddr, uint64_t size);
 int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job, bool need_pipe_sync);
 int amdgpu_vm_update_directories(struct amdgpu_device *adev,
 				 struct amdgpu_vm *vm);
-- 
2.17.1

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

Re: [PATCH 6/8] drm/amdgpu: allocate VM PDs/PTs on demand

[Kuehling, Felix]

This may cause regressions in KFD if PT BO allocation can trigger 
eviction fences. The previous call to amdgpu_vm_alloc_pts was in a 
context where we had temporarily removed the eviction fence. PT BO 
allocation in amdgpu_vm_bo_update is not protected like that.

I vaguely remember looking into this last time you were working on our 
eviction fence code and thinking that most of the cases where we remove 
the eviction fence were no longer needed if fence synchronization 
happens through the amdgpu_sync-object API (rather than waiting on a 
reservation object directly). I think it's time I go and finally clean 
that up.

Do you know whether PT BO allocation would wait on the page-directory 
reservation object without the sync-object API anywhere?

Regards,
   Felix

On 2019-02-04 7:42 a.m., Christian König wrote:
> Let's start to allocate VM PDs/PTs on demand instead of pre-allocating
> them during mapping.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  |   6 -
>   drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c       |   9 --
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c       |  10 --
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        | 136 +++++-------------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |   3 -
>   5 files changed, 38 insertions(+), 126 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index d7b10d79f1de..2176c92f9377 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -492,12 +492,6 @@ static int add_bo_to_vm(struct amdgpu_device *adev, struct kgd_mem *mem,
>   					vm->process_info->eviction_fence,
>   					NULL, NULL);
>   
> -	ret = amdgpu_vm_alloc_pts(adev, vm, va, amdgpu_bo_size(bo));
> -	if (ret) {
> -		pr_err("Failed to allocate pts, err=%d\n", ret);
> -		goto err_alloc_pts;
> -	}
> -
>   	ret = vm_validate_pt_pd_bos(vm);
>   	if (ret) {
>   		pr_err("validate_pt_pd_bos() failed\n");
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
> index 7e22be7ca68a..54dd02a898b9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
> @@ -92,15 +92,6 @@ int amdgpu_map_static_csa(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   		return -ENOMEM;
>   	}
>   
> -	r = amdgpu_vm_alloc_pts(adev, (*bo_va)->base.vm, csa_addr,
> -				size);
> -	if (r) {
> -		DRM_ERROR("failed to allocate pts for static CSA, err=%d\n", r);
> -		amdgpu_vm_bo_rmv(adev, *bo_va);
> -		ttm_eu_backoff_reservation(&ticket, &list);
> -		return r;
> -	}
> -
>   	r = amdgpu_vm_bo_map(adev, *bo_va, csa_addr, 0, size,
>   			     AMDGPU_PTE_READABLE | AMDGPU_PTE_WRITEABLE |
>   			     AMDGPU_PTE_EXECUTABLE);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index d21dd2f369da..e141e3b13112 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -627,11 +627,6 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>   
>   	switch (args->operation) {
>   	case AMDGPU_VA_OP_MAP:
> -		r = amdgpu_vm_alloc_pts(adev, bo_va->base.vm, args->va_address,
> -					args->map_size);
> -		if (r)
> -			goto error_backoff;
> -
>   		va_flags = amdgpu_gmc_get_pte_flags(adev, args->flags);
>   		r = amdgpu_vm_bo_map(adev, bo_va, args->va_address,
>   				     args->offset_in_bo, args->map_size,
> @@ -647,11 +642,6 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>   						args->map_size);
>   		break;
>   	case AMDGPU_VA_OP_REPLACE:
> -		r = amdgpu_vm_alloc_pts(adev, bo_va->base.vm, args->va_address,
> -					args->map_size);
> -		if (r)
> -			goto error_backoff;
> -
>   		va_flags = amdgpu_gmc_get_pte_flags(adev, args->flags);
>   		r = amdgpu_vm_bo_replace_map(adev, bo_va, args->va_address,
>   					     args->offset_in_bo, args->map_size,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index f7d410a5d848..0b8a1aa56c4a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -504,47 +504,6 @@ static void amdgpu_vm_pt_next(struct amdgpu_device *adev,
>   	}
>   }
>   
> -/**
> - * amdgpu_vm_pt_first_leaf - get first leaf PD/PT
> - *
> - * @adev: amdgpu_device pointer
> - * @vm: amdgpu_vm structure
> - * @start: start addr of the walk
> - * @cursor: state to initialize
> - *
> - * Start a walk and go directly to the leaf node.
> - */
> -static void amdgpu_vm_pt_first_leaf(struct amdgpu_device *adev,
> -				    struct amdgpu_vm *vm, uint64_t start,
> -				    struct amdgpu_vm_pt_cursor *cursor)
> -{
> -	amdgpu_vm_pt_start(adev, vm, start, cursor);
> -	while (amdgpu_vm_pt_descendant(adev, cursor));
> -}
> -
> -/**
> - * amdgpu_vm_pt_next_leaf - get next leaf PD/PT
> - *
> - * @adev: amdgpu_device pointer
> - * @cursor: current state
> - *
> - * Walk the PD/PT tree to the next leaf node.
> - */
> -static void amdgpu_vm_pt_next_leaf(struct amdgpu_device *adev,
> -				   struct amdgpu_vm_pt_cursor *cursor)
> -{
> -	amdgpu_vm_pt_next(adev, cursor);
> -	if (cursor->pfn != ~0ll)
> -		while (amdgpu_vm_pt_descendant(adev, cursor));
> -}
> -
> -/**
> - * for_each_amdgpu_vm_pt_leaf - walk over all leaf PDs/PTs in the hierarchy
> - */
> -#define for_each_amdgpu_vm_pt_leaf(adev, vm, start, end, cursor)		\
> -	for (amdgpu_vm_pt_first_leaf((adev), (vm), (start), &(cursor));		\
> -	     (cursor).pfn <= end; amdgpu_vm_pt_next_leaf((adev), &(cursor)))
> -
>   /**
>    * amdgpu_vm_pt_first_dfs - start a deep first search
>    *
> @@ -919,74 +878,51 @@ static void amdgpu_vm_bo_param(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>    * Returns:
>    * 0 on success, errno otherwise.
>    */
> -int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
> -			struct amdgpu_vm *vm,
> -			uint64_t saddr, uint64_t size)
> +static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
> +			       struct amdgpu_vm *vm,
> +			       struct amdgpu_vm_pt_cursor *cursor)
>   {
> -	struct amdgpu_vm_pt_cursor cursor;
> +	struct amdgpu_vm_pt *entry = cursor->entry;
> +	struct amdgpu_bo_param bp;
>   	struct amdgpu_bo *pt;
> -	uint64_t eaddr;
>   	int r;
>   
> -	/* validate the parameters */
> -	if (saddr & AMDGPU_GPU_PAGE_MASK || size & AMDGPU_GPU_PAGE_MASK)
> -		return -EINVAL;
> +	if (cursor->level < AMDGPU_VM_PTB && !entry->entries) {
> +		unsigned num_entries;
>   
> -	eaddr = saddr + size - 1;
> -
> -	saddr /= AMDGPU_GPU_PAGE_SIZE;
> -	eaddr /= AMDGPU_GPU_PAGE_SIZE;
> -
> -	if (eaddr >= adev->vm_manager.max_pfn) {
> -		dev_err(adev->dev, "va above limit (0x%08llX >= 0x%08llX)\n",
> -			eaddr, adev->vm_manager.max_pfn);
> -		return -EINVAL;
> +		num_entries = amdgpu_vm_num_entries(adev, cursor->level);
> +		entry->entries = kvmalloc_array(num_entries,
> +						sizeof(*entry->entries),
> +						GFP_KERNEL | __GFP_ZERO);
> +		if (!entry->entries)
> +			return -ENOMEM;
>   	}
>   
> -	for_each_amdgpu_vm_pt_leaf(adev, vm, saddr, eaddr, cursor) {
> -		struct amdgpu_vm_pt *entry = cursor.entry;
> -		struct amdgpu_bo_param bp;
> -
> -		if (cursor.level < AMDGPU_VM_PTB) {
> -			unsigned num_entries;
> -
> -			num_entries = amdgpu_vm_num_entries(adev, cursor.level);
> -			entry->entries = kvmalloc_array(num_entries,
> -							sizeof(*entry->entries),
> -							GFP_KERNEL |
> -							__GFP_ZERO);
> -			if (!entry->entries)
> -				return -ENOMEM;
> -		}
> -
> -
> -		if (entry->base.bo)
> -			continue;
> -
> -		amdgpu_vm_bo_param(adev, vm, cursor.level, &bp);
> -
> -		r = amdgpu_bo_create(adev, &bp, &pt);
> -		if (r)
> -			return r;
> -
> -		if (vm->use_cpu_for_update) {
> -			r = amdgpu_bo_kmap(pt, NULL);
> -			if (r)
> -				goto error_free_pt;
> -		}
> +	if (entry->base.bo)
> +		return 0;
>   
> -		/* Keep a reference to the root directory to avoid
> -		* freeing them up in the wrong order.
> -		*/
> -		pt->parent = amdgpu_bo_ref(cursor.parent->base.bo);
> +	amdgpu_vm_bo_param(adev, vm, cursor->level, &bp);
>   
> -		amdgpu_vm_bo_base_init(&entry->base, vm, pt);
> +	r = amdgpu_bo_create(adev, &bp, &pt);
> +	if (r)
> +		return r;
>   
> -		r = amdgpu_vm_clear_bo(adev, vm, pt);
> +	if (vm->use_cpu_for_update) {
> +		r = amdgpu_bo_kmap(pt, NULL);
>   		if (r)
>   			goto error_free_pt;
>   	}
>   
> +	/* Keep a reference to the root directory to avoid
> +	 * freeing them up in the wrong order.
> +	 */
> +	pt->parent = amdgpu_bo_ref(cursor->parent->base.bo);
> +	amdgpu_vm_bo_base_init(&entry->base, vm, pt);
> +
> +	r = amdgpu_vm_clear_bo(adev, vm, pt);
> +	if (r)
> +		goto error_free_pt;
> +
>   	return 0;
>   
>   error_free_pt:
> @@ -1655,6 +1591,7 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>   	struct amdgpu_vm_pt_cursor cursor;
>   	uint64_t frag_start = start, frag_end;
>   	unsigned int frag;
> +	int r;
>   
>   	/* figure out the initial fragment */
>   	amdgpu_vm_fragment(params, frag_start, end, flags, &frag, &frag_end);
> @@ -1662,12 +1599,15 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>   	/* walk over the address space and update the PTs */
>   	amdgpu_vm_pt_start(adev, params->vm, start, &cursor);
>   	while (cursor.pfn < end) {
> -		struct amdgpu_bo *pt = cursor.entry->base.bo;
>   		unsigned shift, parent_shift, mask;
>   		uint64_t incr, entry_end, pe_start;
> +		struct amdgpu_bo *pt;
>   
> -		if (!pt)
> -			return -ENOENT;
> +		r = amdgpu_vm_alloc_pts(params->adev, params->vm, &cursor);
> +		if (r)
> +			return r;
> +
> +		pt = cursor.entry->base.bo;
>   
>   		/* The root level can't be a huge page */
>   		if (cursor.level == adev->vm_manager.root_level) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 81ff8177f092..116605c038d2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -303,9 +303,6 @@ bool amdgpu_vm_ready(struct amdgpu_vm *vm);
>   int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   			      int (*callback)(void *p, struct amdgpu_bo *bo),
>   			      void *param);
> -int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
> -			struct amdgpu_vm *vm,
> -			uint64_t saddr, uint64_t size);
>   int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job, bool need_pipe_sync);
>   int amdgpu_vm_update_directories(struct amdgpu_device *adev,
>   				 struct amdgpu_vm *vm);
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 6/8] drm/amdgpu: allocate VM PDs/PTs on demand

[Kuehling, Felix]

On 2019-02-04 3:17 p.m., Kuehling, Felix wrote:
> This may cause regressions in KFD if PT BO allocation can trigger
> eviction fences. The previous call to amdgpu_vm_alloc_pts was in a
> context where we had temporarily removed the eviction fence. PT BO
> allocation in amdgpu_vm_bo_update is not protected like that.
>
> I vaguely remember looking into this last time you were working on our
> eviction fence code and thinking that most of the cases where we remove
> the eviction fence were no longer needed if fence synchronization
> happens through the amdgpu_sync-object API (rather than waiting on a
> reservation object directly). I think it's time I go and finally clean
> that up.

I'm looking at this now. It's not working as I was hoping.


>
> Do you know whether PT BO allocation would wait on the page-directory
> reservation object without the sync-object API anywhere?

It doesn't even matter. Buffer moves (anything calling amdgpu_sync_resv 
with AMDGPU_FENCE_OWNER_UNDEFINED) are supposed to trigger eviction 
fences. So page table BO allocation triggers eviction fences on the PD 
reservation. I don't know how to avoid this other than by removing the 
eviction fence while allocating PT BOs. With your changes this will be 
potentially every time we map or unmap memory.

Any better ideas?

Regards,
   Felix


>
> Regards,
>     Felix
>
> On 2019-02-04 7:42 a.m., Christian König wrote:
>> Let's start to allocate VM PDs/PTs on demand instead of pre-allocating
>> them during mapping.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>    .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  |   6 -
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c       |   9 --
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c       |  10 --
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        | 136 +++++-------------
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |   3 -
>>    5 files changed, 38 insertions(+), 126 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> index d7b10d79f1de..2176c92f9377 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> @@ -492,12 +492,6 @@ static int add_bo_to_vm(struct amdgpu_device *adev, struct kgd_mem *mem,
>>    					vm->process_info->eviction_fence,
>>    					NULL, NULL);
>>    
>> -	ret = amdgpu_vm_alloc_pts(adev, vm, va, amdgpu_bo_size(bo));
>> -	if (ret) {
>> -		pr_err("Failed to allocate pts, err=%d\n", ret);
>> -		goto err_alloc_pts;
>> -	}
>> -
>>    	ret = vm_validate_pt_pd_bos(vm);
>>    	if (ret) {
>>    		pr_err("validate_pt_pd_bos() failed\n");
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>> index 7e22be7ca68a..54dd02a898b9 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>> @@ -92,15 +92,6 @@ int amdgpu_map_static_csa(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>    		return -ENOMEM;
>>    	}
>>    
>> -	r = amdgpu_vm_alloc_pts(adev, (*bo_va)->base.vm, csa_addr,
>> -				size);
>> -	if (r) {
>> -		DRM_ERROR("failed to allocate pts for static CSA, err=%d\n", r);
>> -		amdgpu_vm_bo_rmv(adev, *bo_va);
>> -		ttm_eu_backoff_reservation(&ticket, &list);
>> -		return r;
>> -	}
>> -
>>    	r = amdgpu_vm_bo_map(adev, *bo_va, csa_addr, 0, size,
>>    			     AMDGPU_PTE_READABLE | AMDGPU_PTE_WRITEABLE |
>>    			     AMDGPU_PTE_EXECUTABLE);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> index d21dd2f369da..e141e3b13112 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> @@ -627,11 +627,6 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>>    
>>    	switch (args->operation) {
>>    	case AMDGPU_VA_OP_MAP:
>> -		r = amdgpu_vm_alloc_pts(adev, bo_va->base.vm, args->va_address,
>> -					args->map_size);
>> -		if (r)
>> -			goto error_backoff;
>> -
>>    		va_flags = amdgpu_gmc_get_pte_flags(adev, args->flags);
>>    		r = amdgpu_vm_bo_map(adev, bo_va, args->va_address,
>>    				     args->offset_in_bo, args->map_size,
>> @@ -647,11 +642,6 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>>    						args->map_size);
>>    		break;
>>    	case AMDGPU_VA_OP_REPLACE:
>> -		r = amdgpu_vm_alloc_pts(adev, bo_va->base.vm, args->va_address,
>> -					args->map_size);
>> -		if (r)
>> -			goto error_backoff;
>> -
>>    		va_flags = amdgpu_gmc_get_pte_flags(adev, args->flags);
>>    		r = amdgpu_vm_bo_replace_map(adev, bo_va, args->va_address,
>>    					     args->offset_in_bo, args->map_size,
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index f7d410a5d848..0b8a1aa56c4a 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -504,47 +504,6 @@ static void amdgpu_vm_pt_next(struct amdgpu_device *adev,
>>    	}
>>    }
>>    
>> -/**
>> - * amdgpu_vm_pt_first_leaf - get first leaf PD/PT
>> - *
>> - * @adev: amdgpu_device pointer
>> - * @vm: amdgpu_vm structure
>> - * @start: start addr of the walk
>> - * @cursor: state to initialize
>> - *
>> - * Start a walk and go directly to the leaf node.
>> - */
>> -static void amdgpu_vm_pt_first_leaf(struct amdgpu_device *adev,
>> -				    struct amdgpu_vm *vm, uint64_t start,
>> -				    struct amdgpu_vm_pt_cursor *cursor)
>> -{
>> -	amdgpu_vm_pt_start(adev, vm, start, cursor);
>> -	while (amdgpu_vm_pt_descendant(adev, cursor));
>> -}
>> -
>> -/**
>> - * amdgpu_vm_pt_next_leaf - get next leaf PD/PT
>> - *
>> - * @adev: amdgpu_device pointer
>> - * @cursor: current state
>> - *
>> - * Walk the PD/PT tree to the next leaf node.
>> - */
>> -static void amdgpu_vm_pt_next_leaf(struct amdgpu_device *adev,
>> -				   struct amdgpu_vm_pt_cursor *cursor)
>> -{
>> -	amdgpu_vm_pt_next(adev, cursor);
>> -	if (cursor->pfn != ~0ll)
>> -		while (amdgpu_vm_pt_descendant(adev, cursor));
>> -}
>> -
>> -/**
>> - * for_each_amdgpu_vm_pt_leaf - walk over all leaf PDs/PTs in the hierarchy
>> - */
>> -#define for_each_amdgpu_vm_pt_leaf(adev, vm, start, end, cursor)		\
>> -	for (amdgpu_vm_pt_first_leaf((adev), (vm), (start), &(cursor));		\
>> -	     (cursor).pfn <= end; amdgpu_vm_pt_next_leaf((adev), &(cursor)))
>> -
>>    /**
>>     * amdgpu_vm_pt_first_dfs - start a deep first search
>>     *
>> @@ -919,74 +878,51 @@ static void amdgpu_vm_bo_param(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>     * Returns:
>>     * 0 on success, errno otherwise.
>>     */
>> -int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>> -			struct amdgpu_vm *vm,
>> -			uint64_t saddr, uint64_t size)
>> +static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>> +			       struct amdgpu_vm *vm,
>> +			       struct amdgpu_vm_pt_cursor *cursor)
>>    {
>> -	struct amdgpu_vm_pt_cursor cursor;
>> +	struct amdgpu_vm_pt *entry = cursor->entry;
>> +	struct amdgpu_bo_param bp;
>>    	struct amdgpu_bo *pt;
>> -	uint64_t eaddr;
>>    	int r;
>>    
>> -	/* validate the parameters */
>> -	if (saddr & AMDGPU_GPU_PAGE_MASK || size & AMDGPU_GPU_PAGE_MASK)
>> -		return -EINVAL;
>> +	if (cursor->level < AMDGPU_VM_PTB && !entry->entries) {
>> +		unsigned num_entries;
>>    
>> -	eaddr = saddr + size - 1;
>> -
>> -	saddr /= AMDGPU_GPU_PAGE_SIZE;
>> -	eaddr /= AMDGPU_GPU_PAGE_SIZE;
>> -
>> -	if (eaddr >= adev->vm_manager.max_pfn) {
>> -		dev_err(adev->dev, "va above limit (0x%08llX >= 0x%08llX)\n",
>> -			eaddr, adev->vm_manager.max_pfn);
>> -		return -EINVAL;
>> +		num_entries = amdgpu_vm_num_entries(adev, cursor->level);
>> +		entry->entries = kvmalloc_array(num_entries,
>> +						sizeof(*entry->entries),
>> +						GFP_KERNEL | __GFP_ZERO);
>> +		if (!entry->entries)
>> +			return -ENOMEM;
>>    	}
>>    
>> -	for_each_amdgpu_vm_pt_leaf(adev, vm, saddr, eaddr, cursor) {
>> -		struct amdgpu_vm_pt *entry = cursor.entry;
>> -		struct amdgpu_bo_param bp;
>> -
>> -		if (cursor.level < AMDGPU_VM_PTB) {
>> -			unsigned num_entries;
>> -
>> -			num_entries = amdgpu_vm_num_entries(adev, cursor.level);
>> -			entry->entries = kvmalloc_array(num_entries,
>> -							sizeof(*entry->entries),
>> -							GFP_KERNEL |
>> -							__GFP_ZERO);
>> -			if (!entry->entries)
>> -				return -ENOMEM;
>> -		}
>> -
>> -
>> -		if (entry->base.bo)
>> -			continue;
>> -
>> -		amdgpu_vm_bo_param(adev, vm, cursor.level, &bp);
>> -
>> -		r = amdgpu_bo_create(adev, &bp, &pt);
>> -		if (r)
>> -			return r;
>> -
>> -		if (vm->use_cpu_for_update) {
>> -			r = amdgpu_bo_kmap(pt, NULL);
>> -			if (r)
>> -				goto error_free_pt;
>> -		}
>> +	if (entry->base.bo)
>> +		return 0;
>>    
>> -		/* Keep a reference to the root directory to avoid
>> -		* freeing them up in the wrong order.
>> -		*/
>> -		pt->parent = amdgpu_bo_ref(cursor.parent->base.bo);
>> +	amdgpu_vm_bo_param(adev, vm, cursor->level, &bp);
>>    
>> -		amdgpu_vm_bo_base_init(&entry->base, vm, pt);
>> +	r = amdgpu_bo_create(adev, &bp, &pt);
>> +	if (r)
>> +		return r;
>>    
>> -		r = amdgpu_vm_clear_bo(adev, vm, pt);
>> +	if (vm->use_cpu_for_update) {
>> +		r = amdgpu_bo_kmap(pt, NULL);
>>    		if (r)
>>    			goto error_free_pt;
>>    	}
>>    
>> +	/* Keep a reference to the root directory to avoid
>> +	 * freeing them up in the wrong order.
>> +	 */
>> +	pt->parent = amdgpu_bo_ref(cursor->parent->base.bo);
>> +	amdgpu_vm_bo_base_init(&entry->base, vm, pt);
>> +
>> +	r = amdgpu_vm_clear_bo(adev, vm, pt);
>> +	if (r)
>> +		goto error_free_pt;
>> +
>>    	return 0;
>>    
>>    error_free_pt:
>> @@ -1655,6 +1591,7 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>>    	struct amdgpu_vm_pt_cursor cursor;
>>    	uint64_t frag_start = start, frag_end;
>>    	unsigned int frag;
>> +	int r;
>>    
>>    	/* figure out the initial fragment */
>>    	amdgpu_vm_fragment(params, frag_start, end, flags, &frag, &frag_end);
>> @@ -1662,12 +1599,15 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>>    	/* walk over the address space and update the PTs */
>>    	amdgpu_vm_pt_start(adev, params->vm, start, &cursor);
>>    	while (cursor.pfn < end) {
>> -		struct amdgpu_bo *pt = cursor.entry->base.bo;
>>    		unsigned shift, parent_shift, mask;
>>    		uint64_t incr, entry_end, pe_start;
>> +		struct amdgpu_bo *pt;
>>    
>> -		if (!pt)
>> -			return -ENOENT;
>> +		r = amdgpu_vm_alloc_pts(params->adev, params->vm, &cursor);
>> +		if (r)
>> +			return r;
>> +
>> +		pt = cursor.entry->base.bo;
>>    
>>    		/* The root level can't be a huge page */
>>    		if (cursor.level == adev->vm_manager.root_level) {
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> index 81ff8177f092..116605c038d2 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> @@ -303,9 +303,6 @@ bool amdgpu_vm_ready(struct amdgpu_vm *vm);
>>    int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>    			      int (*callback)(void *p, struct amdgpu_bo *bo),
>>    			      void *param);
>> -int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>> -			struct amdgpu_vm *vm,
>> -			uint64_t saddr, uint64_t size);
>>    int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job, bool need_pipe_sync);
>>    int amdgpu_vm_update_directories(struct amdgpu_device *adev,
>>    				 struct amdgpu_vm *vm);
> _______________________________________________
> 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 6/8] drm/amdgpu: allocate VM PDs/PTs on demand

[Christian König]

Am 05.02.19 um 01:33 schrieb Kuehling, Felix:
> On 2019-02-04 3:17 p.m., Kuehling, Felix wrote:
>> This may cause regressions in KFD if PT BO allocation can trigger
>> eviction fences. The previous call to amdgpu_vm_alloc_pts was in a
>> context where we had temporarily removed the eviction fence. PT BO
>> allocation in amdgpu_vm_bo_update is not protected like that.
>>
>> I vaguely remember looking into this last time you were working on our
>> eviction fence code and thinking that most of the cases where we remove
>> the eviction fence were no longer needed if fence synchronization
>> happens through the amdgpu_sync-object API (rather than waiting on a
>> reservation object directly). I think it's time I go and finally clean
>> that up.
> I'm looking at this now. It's not working as I was hoping.
>
>
>> Do you know whether PT BO allocation would wait on the page-directory
>> reservation object without the sync-object API anywhere?
> It doesn't even matter. Buffer moves (anything calling amdgpu_sync_resv
> with AMDGPU_FENCE_OWNER_UNDEFINED) are supposed to trigger eviction
> fences. So page table BO allocation triggers eviction fences on the PD
> reservation. I don't know how to avoid this other than by removing the
> eviction fence while allocating PT BOs. With your changes this will be
> potentially every time we map or unmap memory.
>
> Any better ideas?

Let me take a closer look what exactly is happening here.

Regards,
Christian.

>
> Regards,
>     Felix
>
>
>> Regards,
>>      Felix
>>
>> On 2019-02-04 7:42 a.m., Christian König wrote:
>>> Let's start to allocate VM PDs/PTs on demand instead of pre-allocating
>>> them during mapping.
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> ---
>>>     .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  |   6 -
>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c       |   9 --
>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c       |  10 --
>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        | 136 +++++-------------
>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |   3 -
>>>     5 files changed, 38 insertions(+), 126 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>> index d7b10d79f1de..2176c92f9377 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>> @@ -492,12 +492,6 @@ static int add_bo_to_vm(struct amdgpu_device *adev, struct kgd_mem *mem,
>>>     					vm->process_info->eviction_fence,
>>>     					NULL, NULL);
>>>     
>>> -	ret = amdgpu_vm_alloc_pts(adev, vm, va, amdgpu_bo_size(bo));
>>> -	if (ret) {
>>> -		pr_err("Failed to allocate pts, err=%d\n", ret);
>>> -		goto err_alloc_pts;
>>> -	}
>>> -
>>>     	ret = vm_validate_pt_pd_bos(vm);
>>>     	if (ret) {
>>>     		pr_err("validate_pt_pd_bos() failed\n");
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>>> index 7e22be7ca68a..54dd02a898b9 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>>> @@ -92,15 +92,6 @@ int amdgpu_map_static_csa(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>>     		return -ENOMEM;
>>>     	}
>>>     
>>> -	r = amdgpu_vm_alloc_pts(adev, (*bo_va)->base.vm, csa_addr,
>>> -				size);
>>> -	if (r) {
>>> -		DRM_ERROR("failed to allocate pts for static CSA, err=%d\n", r);
>>> -		amdgpu_vm_bo_rmv(adev, *bo_va);
>>> -		ttm_eu_backoff_reservation(&ticket, &list);
>>> -		return r;
>>> -	}
>>> -
>>>     	r = amdgpu_vm_bo_map(adev, *bo_va, csa_addr, 0, size,
>>>     			     AMDGPU_PTE_READABLE | AMDGPU_PTE_WRITEABLE |
>>>     			     AMDGPU_PTE_EXECUTABLE);
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> index d21dd2f369da..e141e3b13112 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> @@ -627,11 +627,6 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>>>     
>>>     	switch (args->operation) {
>>>     	case AMDGPU_VA_OP_MAP:
>>> -		r = amdgpu_vm_alloc_pts(adev, bo_va->base.vm, args->va_address,
>>> -					args->map_size);
>>> -		if (r)
>>> -			goto error_backoff;
>>> -
>>>     		va_flags = amdgpu_gmc_get_pte_flags(adev, args->flags);
>>>     		r = amdgpu_vm_bo_map(adev, bo_va, args->va_address,
>>>     				     args->offset_in_bo, args->map_size,
>>> @@ -647,11 +642,6 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>>>     						args->map_size);
>>>     		break;
>>>     	case AMDGPU_VA_OP_REPLACE:
>>> -		r = amdgpu_vm_alloc_pts(adev, bo_va->base.vm, args->va_address,
>>> -					args->map_size);
>>> -		if (r)
>>> -			goto error_backoff;
>>> -
>>>     		va_flags = amdgpu_gmc_get_pte_flags(adev, args->flags);
>>>     		r = amdgpu_vm_bo_replace_map(adev, bo_va, args->va_address,
>>>     					     args->offset_in_bo, args->map_size,
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index f7d410a5d848..0b8a1aa56c4a 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -504,47 +504,6 @@ static void amdgpu_vm_pt_next(struct amdgpu_device *adev,
>>>     	}
>>>     }
>>>     
>>> -/**
>>> - * amdgpu_vm_pt_first_leaf - get first leaf PD/PT
>>> - *
>>> - * @adev: amdgpu_device pointer
>>> - * @vm: amdgpu_vm structure
>>> - * @start: start addr of the walk
>>> - * @cursor: state to initialize
>>> - *
>>> - * Start a walk and go directly to the leaf node.
>>> - */
>>> -static void amdgpu_vm_pt_first_leaf(struct amdgpu_device *adev,
>>> -				    struct amdgpu_vm *vm, uint64_t start,
>>> -				    struct amdgpu_vm_pt_cursor *cursor)
>>> -{
>>> -	amdgpu_vm_pt_start(adev, vm, start, cursor);
>>> -	while (amdgpu_vm_pt_descendant(adev, cursor));
>>> -}
>>> -
>>> -/**
>>> - * amdgpu_vm_pt_next_leaf - get next leaf PD/PT
>>> - *
>>> - * @adev: amdgpu_device pointer
>>> - * @cursor: current state
>>> - *
>>> - * Walk the PD/PT tree to the next leaf node.
>>> - */
>>> -static void amdgpu_vm_pt_next_leaf(struct amdgpu_device *adev,
>>> -				   struct amdgpu_vm_pt_cursor *cursor)
>>> -{
>>> -	amdgpu_vm_pt_next(adev, cursor);
>>> -	if (cursor->pfn != ~0ll)
>>> -		while (amdgpu_vm_pt_descendant(adev, cursor));
>>> -}
>>> -
>>> -/**
>>> - * for_each_amdgpu_vm_pt_leaf - walk over all leaf PDs/PTs in the hierarchy
>>> - */
>>> -#define for_each_amdgpu_vm_pt_leaf(adev, vm, start, end, cursor)		\
>>> -	for (amdgpu_vm_pt_first_leaf((adev), (vm), (start), &(cursor));		\
>>> -	     (cursor).pfn <= end; amdgpu_vm_pt_next_leaf((adev), &(cursor)))
>>> -
>>>     /**
>>>      * amdgpu_vm_pt_first_dfs - start a deep first search
>>>      *
>>> @@ -919,74 +878,51 @@ static void amdgpu_vm_bo_param(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>>      * Returns:
>>>      * 0 on success, errno otherwise.
>>>      */
>>> -int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>>> -			struct amdgpu_vm *vm,
>>> -			uint64_t saddr, uint64_t size)
>>> +static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>>> +			       struct amdgpu_vm *vm,
>>> +			       struct amdgpu_vm_pt_cursor *cursor)
>>>     {
>>> -	struct amdgpu_vm_pt_cursor cursor;
>>> +	struct amdgpu_vm_pt *entry = cursor->entry;
>>> +	struct amdgpu_bo_param bp;
>>>     	struct amdgpu_bo *pt;
>>> -	uint64_t eaddr;
>>>     	int r;
>>>     
>>> -	/* validate the parameters */
>>> -	if (saddr & AMDGPU_GPU_PAGE_MASK || size & AMDGPU_GPU_PAGE_MASK)
>>> -		return -EINVAL;
>>> +	if (cursor->level < AMDGPU_VM_PTB && !entry->entries) {
>>> +		unsigned num_entries;
>>>     
>>> -	eaddr = saddr + size - 1;
>>> -
>>> -	saddr /= AMDGPU_GPU_PAGE_SIZE;
>>> -	eaddr /= AMDGPU_GPU_PAGE_SIZE;
>>> -
>>> -	if (eaddr >= adev->vm_manager.max_pfn) {
>>> -		dev_err(adev->dev, "va above limit (0x%08llX >= 0x%08llX)\n",
>>> -			eaddr, adev->vm_manager.max_pfn);
>>> -		return -EINVAL;
>>> +		num_entries = amdgpu_vm_num_entries(adev, cursor->level);
>>> +		entry->entries = kvmalloc_array(num_entries,
>>> +						sizeof(*entry->entries),
>>> +						GFP_KERNEL | __GFP_ZERO);
>>> +		if (!entry->entries)
>>> +			return -ENOMEM;
>>>     	}
>>>     
>>> -	for_each_amdgpu_vm_pt_leaf(adev, vm, saddr, eaddr, cursor) {
>>> -		struct amdgpu_vm_pt *entry = cursor.entry;
>>> -		struct amdgpu_bo_param bp;
>>> -
>>> -		if (cursor.level < AMDGPU_VM_PTB) {
>>> -			unsigned num_entries;
>>> -
>>> -			num_entries = amdgpu_vm_num_entries(adev, cursor.level);
>>> -			entry->entries = kvmalloc_array(num_entries,
>>> -							sizeof(*entry->entries),
>>> -							GFP_KERNEL |
>>> -							__GFP_ZERO);
>>> -			if (!entry->entries)
>>> -				return -ENOMEM;
>>> -		}
>>> -
>>> -
>>> -		if (entry->base.bo)
>>> -			continue;
>>> -
>>> -		amdgpu_vm_bo_param(adev, vm, cursor.level, &bp);
>>> -
>>> -		r = amdgpu_bo_create(adev, &bp, &pt);
>>> -		if (r)
>>> -			return r;
>>> -
>>> -		if (vm->use_cpu_for_update) {
>>> -			r = amdgpu_bo_kmap(pt, NULL);
>>> -			if (r)
>>> -				goto error_free_pt;
>>> -		}
>>> +	if (entry->base.bo)
>>> +		return 0;
>>>     
>>> -		/* Keep a reference to the root directory to avoid
>>> -		* freeing them up in the wrong order.
>>> -		*/
>>> -		pt->parent = amdgpu_bo_ref(cursor.parent->base.bo);
>>> +	amdgpu_vm_bo_param(adev, vm, cursor->level, &bp);
>>>     
>>> -		amdgpu_vm_bo_base_init(&entry->base, vm, pt);
>>> +	r = amdgpu_bo_create(adev, &bp, &pt);
>>> +	if (r)
>>> +		return r;
>>>     
>>> -		r = amdgpu_vm_clear_bo(adev, vm, pt);
>>> +	if (vm->use_cpu_for_update) {
>>> +		r = amdgpu_bo_kmap(pt, NULL);
>>>     		if (r)
>>>     			goto error_free_pt;
>>>     	}
>>>     
>>> +	/* Keep a reference to the root directory to avoid
>>> +	 * freeing them up in the wrong order.
>>> +	 */
>>> +	pt->parent = amdgpu_bo_ref(cursor->parent->base.bo);
>>> +	amdgpu_vm_bo_base_init(&entry->base, vm, pt);
>>> +
>>> +	r = amdgpu_vm_clear_bo(adev, vm, pt);
>>> +	if (r)
>>> +		goto error_free_pt;
>>> +
>>>     	return 0;
>>>     
>>>     error_free_pt:
>>> @@ -1655,6 +1591,7 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>>>     	struct amdgpu_vm_pt_cursor cursor;
>>>     	uint64_t frag_start = start, frag_end;
>>>     	unsigned int frag;
>>> +	int r;
>>>     
>>>     	/* figure out the initial fragment */
>>>     	amdgpu_vm_fragment(params, frag_start, end, flags, &frag, &frag_end);
>>> @@ -1662,12 +1599,15 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>>>     	/* walk over the address space and update the PTs */
>>>     	amdgpu_vm_pt_start(adev, params->vm, start, &cursor);
>>>     	while (cursor.pfn < end) {
>>> -		struct amdgpu_bo *pt = cursor.entry->base.bo;
>>>     		unsigned shift, parent_shift, mask;
>>>     		uint64_t incr, entry_end, pe_start;
>>> +		struct amdgpu_bo *pt;
>>>     
>>> -		if (!pt)
>>> -			return -ENOENT;
>>> +		r = amdgpu_vm_alloc_pts(params->adev, params->vm, &cursor);
>>> +		if (r)
>>> +			return r;
>>> +
>>> +		pt = cursor.entry->base.bo;
>>>     
>>>     		/* The root level can't be a huge page */
>>>     		if (cursor.level == adev->vm_manager.root_level) {
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> index 81ff8177f092..116605c038d2 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> @@ -303,9 +303,6 @@ bool amdgpu_vm_ready(struct amdgpu_vm *vm);
>>>     int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>>     			      int (*callback)(void *p, struct amdgpu_bo *bo),
>>>     			      void *param);
>>> -int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>>> -			struct amdgpu_vm *vm,
>>> -			uint64_t saddr, uint64_t size);
>>>     int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job, bool need_pipe_sync);
>>>     int amdgpu_vm_update_directories(struct amdgpu_device *adev,
>>>     				 struct amdgpu_vm *vm);
>> _______________________________________________
>> 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 6/8] drm/amdgpu: allocate VM PDs/PTs on demand

[Christian König]

> This may cause regressions in KFD if PT BO allocation can trigger
> eviction fences.
I don't think that can happen, but need to double check as well.

Otherwise allocating page tables could try to evict other page tables 
from the same process and that seriously doesn't make much sense.

> Do you know whether PT BO allocation would wait on the page-directory
> reservation object without the sync-object API anywhere?
For inside the kernel I can't think of any relevant use case, except for 
the eviction call path and there it is intentional.

One thing which will always wait for all fences is the display code 
path, but that is not relevant here. (Isn't it?).

What might be questionable is amdgpu_gem_wait_idle_ioctl(), but I think 
this is also completely intentional that we wait on everything here.

Regards,
Christian.

Am 04.02.19 um 21:17 schrieb Kuehling, Felix:
> This may cause regressions in KFD if PT BO allocation can trigger
> eviction fences. The previous call to amdgpu_vm_alloc_pts was in a
> context where we had temporarily removed the eviction fence. PT BO
> allocation in amdgpu_vm_bo_update is not protected like that.
>
> I vaguely remember looking into this last time you were working on our
> eviction fence code and thinking that most of the cases where we remove
> the eviction fence were no longer needed if fence synchronization
> happens through the amdgpu_sync-object API (rather than waiting on a
> reservation object directly). I think it's time I go and finally clean
> that up.
>
> Do you know whether PT BO allocation would wait on the page-directory
> reservation object without the sync-object API anywhere?
>
> Regards,
>     Felix
>
> On 2019-02-04 7:42 a.m., Christian König wrote:
>> Let's start to allocate VM PDs/PTs on demand instead of pre-allocating
>> them during mapping.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>    .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  |   6 -
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c       |   9 --
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c       |  10 --
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        | 136 +++++-------------
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |   3 -
>>    5 files changed, 38 insertions(+), 126 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> index d7b10d79f1de..2176c92f9377 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> @@ -492,12 +492,6 @@ static int add_bo_to_vm(struct amdgpu_device *adev, struct kgd_mem *mem,
>>    					vm->process_info->eviction_fence,
>>    					NULL, NULL);
>>    
>> -	ret = amdgpu_vm_alloc_pts(adev, vm, va, amdgpu_bo_size(bo));
>> -	if (ret) {
>> -		pr_err("Failed to allocate pts, err=%d\n", ret);
>> -		goto err_alloc_pts;
>> -	}
>> -
>>    	ret = vm_validate_pt_pd_bos(vm);
>>    	if (ret) {
>>    		pr_err("validate_pt_pd_bos() failed\n");
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>> index 7e22be7ca68a..54dd02a898b9 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>> @@ -92,15 +92,6 @@ int amdgpu_map_static_csa(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>    		return -ENOMEM;
>>    	}
>>    
>> -	r = amdgpu_vm_alloc_pts(adev, (*bo_va)->base.vm, csa_addr,
>> -				size);
>> -	if (r) {
>> -		DRM_ERROR("failed to allocate pts for static CSA, err=%d\n", r);
>> -		amdgpu_vm_bo_rmv(adev, *bo_va);
>> -		ttm_eu_backoff_reservation(&ticket, &list);
>> -		return r;
>> -	}
>> -
>>    	r = amdgpu_vm_bo_map(adev, *bo_va, csa_addr, 0, size,
>>    			     AMDGPU_PTE_READABLE | AMDGPU_PTE_WRITEABLE |
>>    			     AMDGPU_PTE_EXECUTABLE);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> index d21dd2f369da..e141e3b13112 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> @@ -627,11 +627,6 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>>    
>>    	switch (args->operation) {
>>    	case AMDGPU_VA_OP_MAP:
>> -		r = amdgpu_vm_alloc_pts(adev, bo_va->base.vm, args->va_address,
>> -					args->map_size);
>> -		if (r)
>> -			goto error_backoff;
>> -
>>    		va_flags = amdgpu_gmc_get_pte_flags(adev, args->flags);
>>    		r = amdgpu_vm_bo_map(adev, bo_va, args->va_address,
>>    				     args->offset_in_bo, args->map_size,
>> @@ -647,11 +642,6 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>>    						args->map_size);
>>    		break;
>>    	case AMDGPU_VA_OP_REPLACE:
>> -		r = amdgpu_vm_alloc_pts(adev, bo_va->base.vm, args->va_address,
>> -					args->map_size);
>> -		if (r)
>> -			goto error_backoff;
>> -
>>    		va_flags = amdgpu_gmc_get_pte_flags(adev, args->flags);
>>    		r = amdgpu_vm_bo_replace_map(adev, bo_va, args->va_address,
>>    					     args->offset_in_bo, args->map_size,
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index f7d410a5d848..0b8a1aa56c4a 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -504,47 +504,6 @@ static void amdgpu_vm_pt_next(struct amdgpu_device *adev,
>>    	}
>>    }
>>    
>> -/**
>> - * amdgpu_vm_pt_first_leaf - get first leaf PD/PT
>> - *
>> - * @adev: amdgpu_device pointer
>> - * @vm: amdgpu_vm structure
>> - * @start: start addr of the walk
>> - * @cursor: state to initialize
>> - *
>> - * Start a walk and go directly to the leaf node.
>> - */
>> -static void amdgpu_vm_pt_first_leaf(struct amdgpu_device *adev,
>> -				    struct amdgpu_vm *vm, uint64_t start,
>> -				    struct amdgpu_vm_pt_cursor *cursor)
>> -{
>> -	amdgpu_vm_pt_start(adev, vm, start, cursor);
>> -	while (amdgpu_vm_pt_descendant(adev, cursor));
>> -}
>> -
>> -/**
>> - * amdgpu_vm_pt_next_leaf - get next leaf PD/PT
>> - *
>> - * @adev: amdgpu_device pointer
>> - * @cursor: current state
>> - *
>> - * Walk the PD/PT tree to the next leaf node.
>> - */
>> -static void amdgpu_vm_pt_next_leaf(struct amdgpu_device *adev,
>> -				   struct amdgpu_vm_pt_cursor *cursor)
>> -{
>> -	amdgpu_vm_pt_next(adev, cursor);
>> -	if (cursor->pfn != ~0ll)
>> -		while (amdgpu_vm_pt_descendant(adev, cursor));
>> -}
>> -
>> -/**
>> - * for_each_amdgpu_vm_pt_leaf - walk over all leaf PDs/PTs in the hierarchy
>> - */
>> -#define for_each_amdgpu_vm_pt_leaf(adev, vm, start, end, cursor)		\
>> -	for (amdgpu_vm_pt_first_leaf((adev), (vm), (start), &(cursor));		\
>> -	     (cursor).pfn <= end; amdgpu_vm_pt_next_leaf((adev), &(cursor)))
>> -
>>    /**
>>     * amdgpu_vm_pt_first_dfs - start a deep first search
>>     *
>> @@ -919,74 +878,51 @@ static void amdgpu_vm_bo_param(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>     * Returns:
>>     * 0 on success, errno otherwise.
>>     */
>> -int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>> -			struct amdgpu_vm *vm,
>> -			uint64_t saddr, uint64_t size)
>> +static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>> +			       struct amdgpu_vm *vm,
>> +			       struct amdgpu_vm_pt_cursor *cursor)
>>    {
>> -	struct amdgpu_vm_pt_cursor cursor;
>> +	struct amdgpu_vm_pt *entry = cursor->entry;
>> +	struct amdgpu_bo_param bp;
>>    	struct amdgpu_bo *pt;
>> -	uint64_t eaddr;
>>    	int r;
>>    
>> -	/* validate the parameters */
>> -	if (saddr & AMDGPU_GPU_PAGE_MASK || size & AMDGPU_GPU_PAGE_MASK)
>> -		return -EINVAL;
>> +	if (cursor->level < AMDGPU_VM_PTB && !entry->entries) {
>> +		unsigned num_entries;
>>    
>> -	eaddr = saddr + size - 1;
>> -
>> -	saddr /= AMDGPU_GPU_PAGE_SIZE;
>> -	eaddr /= AMDGPU_GPU_PAGE_SIZE;
>> -
>> -	if (eaddr >= adev->vm_manager.max_pfn) {
>> -		dev_err(adev->dev, "va above limit (0x%08llX >= 0x%08llX)\n",
>> -			eaddr, adev->vm_manager.max_pfn);
>> -		return -EINVAL;
>> +		num_entries = amdgpu_vm_num_entries(adev, cursor->level);
>> +		entry->entries = kvmalloc_array(num_entries,
>> +						sizeof(*entry->entries),
>> +						GFP_KERNEL | __GFP_ZERO);
>> +		if (!entry->entries)
>> +			return -ENOMEM;
>>    	}
>>    
>> -	for_each_amdgpu_vm_pt_leaf(adev, vm, saddr, eaddr, cursor) {
>> -		struct amdgpu_vm_pt *entry = cursor.entry;
>> -		struct amdgpu_bo_param bp;
>> -
>> -		if (cursor.level < AMDGPU_VM_PTB) {
>> -			unsigned num_entries;
>> -
>> -			num_entries = amdgpu_vm_num_entries(adev, cursor.level);
>> -			entry->entries = kvmalloc_array(num_entries,
>> -							sizeof(*entry->entries),
>> -							GFP_KERNEL |
>> -							__GFP_ZERO);
>> -			if (!entry->entries)
>> -				return -ENOMEM;
>> -		}
>> -
>> -
>> -		if (entry->base.bo)
>> -			continue;
>> -
>> -		amdgpu_vm_bo_param(adev, vm, cursor.level, &bp);
>> -
>> -		r = amdgpu_bo_create(adev, &bp, &pt);
>> -		if (r)
>> -			return r;
>> -
>> -		if (vm->use_cpu_for_update) {
>> -			r = amdgpu_bo_kmap(pt, NULL);
>> -			if (r)
>> -				goto error_free_pt;
>> -		}
>> +	if (entry->base.bo)
>> +		return 0;
>>    
>> -		/* Keep a reference to the root directory to avoid
>> -		* freeing them up in the wrong order.
>> -		*/
>> -		pt->parent = amdgpu_bo_ref(cursor.parent->base.bo);
>> +	amdgpu_vm_bo_param(adev, vm, cursor->level, &bp);
>>    
>> -		amdgpu_vm_bo_base_init(&entry->base, vm, pt);
>> +	r = amdgpu_bo_create(adev, &bp, &pt);
>> +	if (r)
>> +		return r;
>>    
>> -		r = amdgpu_vm_clear_bo(adev, vm, pt);
>> +	if (vm->use_cpu_for_update) {
>> +		r = amdgpu_bo_kmap(pt, NULL);
>>    		if (r)
>>    			goto error_free_pt;
>>    	}
>>    
>> +	/* Keep a reference to the root directory to avoid
>> +	 * freeing them up in the wrong order.
>> +	 */
>> +	pt->parent = amdgpu_bo_ref(cursor->parent->base.bo);
>> +	amdgpu_vm_bo_base_init(&entry->base, vm, pt);
>> +
>> +	r = amdgpu_vm_clear_bo(adev, vm, pt);
>> +	if (r)
>> +		goto error_free_pt;
>> +
>>    	return 0;
>>    
>>    error_free_pt:
>> @@ -1655,6 +1591,7 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>>    	struct amdgpu_vm_pt_cursor cursor;
>>    	uint64_t frag_start = start, frag_end;
>>    	unsigned int frag;
>> +	int r;
>>    
>>    	/* figure out the initial fragment */
>>    	amdgpu_vm_fragment(params, frag_start, end, flags, &frag, &frag_end);
>> @@ -1662,12 +1599,15 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>>    	/* walk over the address space and update the PTs */
>>    	amdgpu_vm_pt_start(adev, params->vm, start, &cursor);
>>    	while (cursor.pfn < end) {
>> -		struct amdgpu_bo *pt = cursor.entry->base.bo;
>>    		unsigned shift, parent_shift, mask;
>>    		uint64_t incr, entry_end, pe_start;
>> +		struct amdgpu_bo *pt;
>>    
>> -		if (!pt)
>> -			return -ENOENT;
>> +		r = amdgpu_vm_alloc_pts(params->adev, params->vm, &cursor);
>> +		if (r)
>> +			return r;
>> +
>> +		pt = cursor.entry->base.bo;
>>    
>>    		/* The root level can't be a huge page */
>>    		if (cursor.level == adev->vm_manager.root_level) {
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> index 81ff8177f092..116605c038d2 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> @@ -303,9 +303,6 @@ bool amdgpu_vm_ready(struct amdgpu_vm *vm);
>>    int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>    			      int (*callback)(void *p, struct amdgpu_bo *bo),
>>    			      void *param);
>> -int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>> -			struct amdgpu_vm *vm,
>> -			uint64_t saddr, uint64_t size);
>>    int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job, bool need_pipe_sync);
>>    int amdgpu_vm_update_directories(struct amdgpu_device *adev,
>>    				 struct amdgpu_vm *vm);

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

RE: [PATCH 6/8] drm/amdgpu: allocate VM PDs/PTs on demand

[Kuehling, Felix]

> > This may cause regressions in KFD if PT BO allocation can trigger 
> > eviction fences.
> I don't think that can happen, but need to double check as well.
> 
> Otherwise allocating page tables could try to evict other page tables from the same process and that seriously doesn't make much sense.

Eviction fences don't cause actual memory evictions. They are the result of memory evictions. They cause KFD processes to be preempted in order to allow the fence to signal so memory can be evicted. The problem is that a page table allocation waits for the fences on the PD reservation, which looks like an eviction to KFD and triggers an unnecessary preemption. There is no actual memory eviction happening here.

Regards,
  Felix

-----Original Message-----
From: Christian König <ckoenig.leichtzumerken@gmail.com> 
Sent: Tuesday, February 05, 2019 6:37 AM
To: Kuehling, Felix <Felix.Kuehling@amd.com>; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH 6/8] drm/amdgpu: allocate VM PDs/PTs on demand

> This may cause regressions in KFD if PT BO allocation can trigger 
> eviction fences.
I don't think that can happen, but need to double check as well.

Otherwise allocating page tables could try to evict other page tables from the same process and that seriously doesn't make much sense.

> Do you know whether PT BO allocation would wait on the page-directory 
> reservation object without the sync-object API anywhere?
For inside the kernel I can't think of any relevant use case, except for the eviction call path and there it is intentional.

One thing which will always wait for all fences is the display code path, but that is not relevant here. (Isn't it?).

What might be questionable is amdgpu_gem_wait_idle_ioctl(), but I think this is also completely intentional that we wait on everything here.

Regards,
Christian.

Am 04.02.19 um 21:17 schrieb Kuehling, Felix:
> This may cause regressions in KFD if PT BO allocation can trigger 
> eviction fences. The previous call to amdgpu_vm_alloc_pts was in a 
> context where we had temporarily removed the eviction fence. PT BO 
> allocation in amdgpu_vm_bo_update is not protected like that.
>
> I vaguely remember looking into this last time you were working on our 
> eviction fence code and thinking that most of the cases where we 
> remove the eviction fence were no longer needed if fence 
> synchronization happens through the amdgpu_sync-object API (rather 
> than waiting on a reservation object directly). I think it's time I go 
> and finally clean that up.
>
> Do you know whether PT BO allocation would wait on the page-directory 
> reservation object without the sync-object API anywhere?
>
> Regards,
>     Felix
>
> On 2019-02-04 7:42 a.m., Christian König wrote:
>> Let's start to allocate VM PDs/PTs on demand instead of 
>> pre-allocating them during mapping.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>    .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  |   6 -
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c       |   9 --
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c       |  10 --
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        | 136 +++++-------------
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |   3 -
>>    5 files changed, 38 insertions(+), 126 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> index d7b10d79f1de..2176c92f9377 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> @@ -492,12 +492,6 @@ static int add_bo_to_vm(struct amdgpu_device *adev, struct kgd_mem *mem,
>>    					vm->process_info->eviction_fence,
>>    					NULL, NULL);
>>    
>> -	ret = amdgpu_vm_alloc_pts(adev, vm, va, amdgpu_bo_size(bo));
>> -	if (ret) {
>> -		pr_err("Failed to allocate pts, err=%d\n", ret);
>> -		goto err_alloc_pts;
>> -	}
>> -
>>    	ret = vm_validate_pt_pd_bos(vm);
>>    	if (ret) {
>>    		pr_err("validate_pt_pd_bos() failed\n"); diff --git 
>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>> index 7e22be7ca68a..54dd02a898b9 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>> @@ -92,15 +92,6 @@ int amdgpu_map_static_csa(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>    		return -ENOMEM;
>>    	}
>>    
>> -	r = amdgpu_vm_alloc_pts(adev, (*bo_va)->base.vm, csa_addr,
>> -				size);
>> -	if (r) {
>> -		DRM_ERROR("failed to allocate pts for static CSA, err=%d\n", r);
>> -		amdgpu_vm_bo_rmv(adev, *bo_va);
>> -		ttm_eu_backoff_reservation(&ticket, &list);
>> -		return r;
>> -	}
>> -
>>    	r = amdgpu_vm_bo_map(adev, *bo_va, csa_addr, 0, size,
>>    			     AMDGPU_PTE_READABLE | AMDGPU_PTE_WRITEABLE |
>>    			     AMDGPU_PTE_EXECUTABLE);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> index d21dd2f369da..e141e3b13112 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> @@ -627,11 +627,6 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, 
>> void *data,
>>    
>>    	switch (args->operation) {
>>    	case AMDGPU_VA_OP_MAP:
>> -		r = amdgpu_vm_alloc_pts(adev, bo_va->base.vm, args->va_address,
>> -					args->map_size);
>> -		if (r)
>> -			goto error_backoff;
>> -
>>    		va_flags = amdgpu_gmc_get_pte_flags(adev, args->flags);
>>    		r = amdgpu_vm_bo_map(adev, bo_va, args->va_address,
>>    				     args->offset_in_bo, args->map_size, @@ -647,11 +642,6 @@ 
>> int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>>    						args->map_size);
>>    		break;
>>    	case AMDGPU_VA_OP_REPLACE:
>> -		r = amdgpu_vm_alloc_pts(adev, bo_va->base.vm, args->va_address,
>> -					args->map_size);
>> -		if (r)
>> -			goto error_backoff;
>> -
>>    		va_flags = amdgpu_gmc_get_pte_flags(adev, args->flags);
>>    		r = amdgpu_vm_bo_replace_map(adev, bo_va, args->va_address,
>>    					     args->offset_in_bo, args->map_size, diff --git 
>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index f7d410a5d848..0b8a1aa56c4a 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -504,47 +504,6 @@ static void amdgpu_vm_pt_next(struct amdgpu_device *adev,
>>    	}
>>    }
>>    
>> -/**
>> - * amdgpu_vm_pt_first_leaf - get first leaf PD/PT
>> - *
>> - * @adev: amdgpu_device pointer
>> - * @vm: amdgpu_vm structure
>> - * @start: start addr of the walk
>> - * @cursor: state to initialize
>> - *
>> - * Start a walk and go directly to the leaf node.
>> - */
>> -static void amdgpu_vm_pt_first_leaf(struct amdgpu_device *adev,
>> -				    struct amdgpu_vm *vm, uint64_t start,
>> -				    struct amdgpu_vm_pt_cursor *cursor)
>> -{
>> -	amdgpu_vm_pt_start(adev, vm, start, cursor);
>> -	while (amdgpu_vm_pt_descendant(adev, cursor));
>> -}
>> -
>> -/**
>> - * amdgpu_vm_pt_next_leaf - get next leaf PD/PT
>> - *
>> - * @adev: amdgpu_device pointer
>> - * @cursor: current state
>> - *
>> - * Walk the PD/PT tree to the next leaf node.
>> - */
>> -static void amdgpu_vm_pt_next_leaf(struct amdgpu_device *adev,
>> -				   struct amdgpu_vm_pt_cursor *cursor)
>> -{
>> -	amdgpu_vm_pt_next(adev, cursor);
>> -	if (cursor->pfn != ~0ll)
>> -		while (amdgpu_vm_pt_descendant(adev, cursor));
>> -}
>> -
>> -/**
>> - * for_each_amdgpu_vm_pt_leaf - walk over all leaf PDs/PTs in the 
>> hierarchy
>> - */
>> -#define for_each_amdgpu_vm_pt_leaf(adev, vm, start, end, cursor)		\
>> -	for (amdgpu_vm_pt_first_leaf((adev), (vm), (start), &(cursor));		\
>> -	     (cursor).pfn <= end; amdgpu_vm_pt_next_leaf((adev), &(cursor)))
>> -
>>    /**
>>     * amdgpu_vm_pt_first_dfs - start a deep first search
>>     *
>> @@ -919,74 +878,51 @@ static void amdgpu_vm_bo_param(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>     * Returns:
>>     * 0 on success, errno otherwise.
>>     */
>> -int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>> -			struct amdgpu_vm *vm,
>> -			uint64_t saddr, uint64_t size)
>> +static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>> +			       struct amdgpu_vm *vm,
>> +			       struct amdgpu_vm_pt_cursor *cursor)
>>    {
>> -	struct amdgpu_vm_pt_cursor cursor;
>> +	struct amdgpu_vm_pt *entry = cursor->entry;
>> +	struct amdgpu_bo_param bp;
>>    	struct amdgpu_bo *pt;
>> -	uint64_t eaddr;
>>    	int r;
>>    
>> -	/* validate the parameters */
>> -	if (saddr & AMDGPU_GPU_PAGE_MASK || size & AMDGPU_GPU_PAGE_MASK)
>> -		return -EINVAL;
>> +	if (cursor->level < AMDGPU_VM_PTB && !entry->entries) {
>> +		unsigned num_entries;
>>    
>> -	eaddr = saddr + size - 1;
>> -
>> -	saddr /= AMDGPU_GPU_PAGE_SIZE;
>> -	eaddr /= AMDGPU_GPU_PAGE_SIZE;
>> -
>> -	if (eaddr >= adev->vm_manager.max_pfn) {
>> -		dev_err(adev->dev, "va above limit (0x%08llX >= 0x%08llX)\n",
>> -			eaddr, adev->vm_manager.max_pfn);
>> -		return -EINVAL;
>> +		num_entries = amdgpu_vm_num_entries(adev, cursor->level);
>> +		entry->entries = kvmalloc_array(num_entries,
>> +						sizeof(*entry->entries),
>> +						GFP_KERNEL | __GFP_ZERO);
>> +		if (!entry->entries)
>> +			return -ENOMEM;
>>    	}
>>    
>> -	for_each_amdgpu_vm_pt_leaf(adev, vm, saddr, eaddr, cursor) {
>> -		struct amdgpu_vm_pt *entry = cursor.entry;
>> -		struct amdgpu_bo_param bp;
>> -
>> -		if (cursor.level < AMDGPU_VM_PTB) {
>> -			unsigned num_entries;
>> -
>> -			num_entries = amdgpu_vm_num_entries(adev, cursor.level);
>> -			entry->entries = kvmalloc_array(num_entries,
>> -							sizeof(*entry->entries),
>> -							GFP_KERNEL |
>> -							__GFP_ZERO);
>> -			if (!entry->entries)
>> -				return -ENOMEM;
>> -		}
>> -
>> -
>> -		if (entry->base.bo)
>> -			continue;
>> -
>> -		amdgpu_vm_bo_param(adev, vm, cursor.level, &bp);
>> -
>> -		r = amdgpu_bo_create(adev, &bp, &pt);
>> -		if (r)
>> -			return r;
>> -
>> -		if (vm->use_cpu_for_update) {
>> -			r = amdgpu_bo_kmap(pt, NULL);
>> -			if (r)
>> -				goto error_free_pt;
>> -		}
>> +	if (entry->base.bo)
>> +		return 0;
>>    
>> -		/* Keep a reference to the root directory to avoid
>> -		* freeing them up in the wrong order.
>> -		*/
>> -		pt->parent = amdgpu_bo_ref(cursor.parent->base.bo);
>> +	amdgpu_vm_bo_param(adev, vm, cursor->level, &bp);
>>    
>> -		amdgpu_vm_bo_base_init(&entry->base, vm, pt);
>> +	r = amdgpu_bo_create(adev, &bp, &pt);
>> +	if (r)
>> +		return r;
>>    
>> -		r = amdgpu_vm_clear_bo(adev, vm, pt);
>> +	if (vm->use_cpu_for_update) {
>> +		r = amdgpu_bo_kmap(pt, NULL);
>>    		if (r)
>>    			goto error_free_pt;
>>    	}
>>    
>> +	/* Keep a reference to the root directory to avoid
>> +	 * freeing them up in the wrong order.
>> +	 */
>> +	pt->parent = amdgpu_bo_ref(cursor->parent->base.bo);
>> +	amdgpu_vm_bo_base_init(&entry->base, vm, pt);
>> +
>> +	r = amdgpu_vm_clear_bo(adev, vm, pt);
>> +	if (r)
>> +		goto error_free_pt;
>> +
>>    	return 0;
>>    
>>    error_free_pt:
>> @@ -1655,6 +1591,7 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>>    	struct amdgpu_vm_pt_cursor cursor;
>>    	uint64_t frag_start = start, frag_end;
>>    	unsigned int frag;
>> +	int r;
>>    
>>    	/* figure out the initial fragment */
>>    	amdgpu_vm_fragment(params, frag_start, end, flags, &frag, 
>> &frag_end); @@ -1662,12 +1599,15 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>>    	/* walk over the address space and update the PTs */
>>    	amdgpu_vm_pt_start(adev, params->vm, start, &cursor);
>>    	while (cursor.pfn < end) {
>> -		struct amdgpu_bo *pt = cursor.entry->base.bo;
>>    		unsigned shift, parent_shift, mask;
>>    		uint64_t incr, entry_end, pe_start;
>> +		struct amdgpu_bo *pt;
>>    
>> -		if (!pt)
>> -			return -ENOENT;
>> +		r = amdgpu_vm_alloc_pts(params->adev, params->vm, &cursor);
>> +		if (r)
>> +			return r;
>> +
>> +		pt = cursor.entry->base.bo;
>>    
>>    		/* The root level can't be a huge page */
>>    		if (cursor.level == adev->vm_manager.root_level) { diff --git 
>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> index 81ff8177f092..116605c038d2 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> @@ -303,9 +303,6 @@ bool amdgpu_vm_ready(struct amdgpu_vm *vm);
>>    int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>    			      int (*callback)(void *p, struct amdgpu_bo *bo),
>>    			      void *param);
>> -int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>> -			struct amdgpu_vm *vm,
>> -			uint64_t saddr, uint64_t size);
>>    int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job, bool need_pipe_sync);
>>    int amdgpu_vm_update_directories(struct amdgpu_device *adev,
>>    				 struct amdgpu_vm *vm);

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

Re: [PATCH 6/8] drm/amdgpu: allocate VM PDs/PTs on demand

[Christian König]

Am 05.02.19 um 16:20 schrieb Kuehling, Felix:
>>> This may cause regressions in KFD if PT BO allocation can trigger
>>> eviction fences.
>> I don't think that can happen, but need to double check as well.
>>
>> Otherwise allocating page tables could try to evict other page tables from the same process and that seriously doesn't make much sense.
> Eviction fences don't cause actual memory evictions. They are the result of memory evictions. They cause KFD processes to be preempted in order to allow the fence to signal so memory can be evicted. The problem is that a page table allocation waits for the fences on the PD reservation, which looks like an eviction to KFD and triggers an unnecessary preemption. There is no actual memory eviction happening here.

Yeah, but where is that actually coming from?

It sounds like we still unnecessary synchronize page table updates 
somewhere.

Do you have a call chain?

Regards,
Christian.

>
> Regards,
>    Felix
>
> -----Original Message-----
> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> Sent: Tuesday, February 05, 2019 6:37 AM
> To: Kuehling, Felix <Felix.Kuehling@amd.com>; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH 6/8] drm/amdgpu: allocate VM PDs/PTs on demand
>
>> This may cause regressions in KFD if PT BO allocation can trigger
>> eviction fences.
> I don't think that can happen, but need to double check as well.
>
> Otherwise allocating page tables could try to evict other page tables from the same process and that seriously doesn't make much sense.
>
>> Do you know whether PT BO allocation would wait on the page-directory
>> reservation object without the sync-object API anywhere?
> For inside the kernel I can't think of any relevant use case, except for the eviction call path and there it is intentional.
>
> One thing which will always wait for all fences is the display code path, but that is not relevant here. (Isn't it?).
>
> What might be questionable is amdgpu_gem_wait_idle_ioctl(), but I think this is also completely intentional that we wait on everything here.
>
> Regards,
> Christian.
>
> Am 04.02.19 um 21:17 schrieb Kuehling, Felix:
>> This may cause regressions in KFD if PT BO allocation can trigger
>> eviction fences. The previous call to amdgpu_vm_alloc_pts was in a
>> context where we had temporarily removed the eviction fence. PT BO
>> allocation in amdgpu_vm_bo_update is not protected like that.
>>
>> I vaguely remember looking into this last time you were working on our
>> eviction fence code and thinking that most of the cases where we
>> remove the eviction fence were no longer needed if fence
>> synchronization happens through the amdgpu_sync-object API (rather
>> than waiting on a reservation object directly). I think it's time I go
>> and finally clean that up.
>>
>> Do you know whether PT BO allocation would wait on the page-directory
>> reservation object without the sync-object API anywhere?
>>
>> Regards,
>>      Felix
>>
>> On 2019-02-04 7:42 a.m., Christian König wrote:
>>> Let's start to allocate VM PDs/PTs on demand instead of
>>> pre-allocating them during mapping.
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> ---
>>>     .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  |   6 -
>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c       |   9 --
>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c       |  10 --
>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        | 136 +++++-------------
>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |   3 -
>>>     5 files changed, 38 insertions(+), 126 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>> index d7b10d79f1de..2176c92f9377 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>> @@ -492,12 +492,6 @@ static int add_bo_to_vm(struct amdgpu_device *adev, struct kgd_mem *mem,
>>>     					vm->process_info->eviction_fence,
>>>     					NULL, NULL);
>>>     
>>> -	ret = amdgpu_vm_alloc_pts(adev, vm, va, amdgpu_bo_size(bo));
>>> -	if (ret) {
>>> -		pr_err("Failed to allocate pts, err=%d\n", ret);
>>> -		goto err_alloc_pts;
>>> -	}
>>> -
>>>     	ret = vm_validate_pt_pd_bos(vm);
>>>     	if (ret) {
>>>     		pr_err("validate_pt_pd_bos() failed\n"); diff --git
>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>>> index 7e22be7ca68a..54dd02a898b9 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>>> @@ -92,15 +92,6 @@ int amdgpu_map_static_csa(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>>     		return -ENOMEM;
>>>     	}
>>>     
>>> -	r = amdgpu_vm_alloc_pts(adev, (*bo_va)->base.vm, csa_addr,
>>> -				size);
>>> -	if (r) {
>>> -		DRM_ERROR("failed to allocate pts for static CSA, err=%d\n", r);
>>> -		amdgpu_vm_bo_rmv(adev, *bo_va);
>>> -		ttm_eu_backoff_reservation(&ticket, &list);
>>> -		return r;
>>> -	}
>>> -
>>>     	r = amdgpu_vm_bo_map(adev, *bo_va, csa_addr, 0, size,
>>>     			     AMDGPU_PTE_READABLE | AMDGPU_PTE_WRITEABLE |
>>>     			     AMDGPU_PTE_EXECUTABLE);
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> index d21dd2f369da..e141e3b13112 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> @@ -627,11 +627,6 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev,
>>> void *data,
>>>     
>>>     	switch (args->operation) {
>>>     	case AMDGPU_VA_OP_MAP:
>>> -		r = amdgpu_vm_alloc_pts(adev, bo_va->base.vm, args->va_address,
>>> -					args->map_size);
>>> -		if (r)
>>> -			goto error_backoff;
>>> -
>>>     		va_flags = amdgpu_gmc_get_pte_flags(adev, args->flags);
>>>     		r = amdgpu_vm_bo_map(adev, bo_va, args->va_address,
>>>     				     args->offset_in_bo, args->map_size, @@ -647,11 +642,6 @@
>>> int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>>>     						args->map_size);
>>>     		break;
>>>     	case AMDGPU_VA_OP_REPLACE:
>>> -		r = amdgpu_vm_alloc_pts(adev, bo_va->base.vm, args->va_address,
>>> -					args->map_size);
>>> -		if (r)
>>> -			goto error_backoff;
>>> -
>>>     		va_flags = amdgpu_gmc_get_pte_flags(adev, args->flags);
>>>     		r = amdgpu_vm_bo_replace_map(adev, bo_va, args->va_address,
>>>     					     args->offset_in_bo, args->map_size, diff --git
>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index f7d410a5d848..0b8a1aa56c4a 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -504,47 +504,6 @@ static void amdgpu_vm_pt_next(struct amdgpu_device *adev,
>>>     	}
>>>     }
>>>     
>>> -/**
>>> - * amdgpu_vm_pt_first_leaf - get first leaf PD/PT
>>> - *
>>> - * @adev: amdgpu_device pointer
>>> - * @vm: amdgpu_vm structure
>>> - * @start: start addr of the walk
>>> - * @cursor: state to initialize
>>> - *
>>> - * Start a walk and go directly to the leaf node.
>>> - */
>>> -static void amdgpu_vm_pt_first_leaf(struct amdgpu_device *adev,
>>> -				    struct amdgpu_vm *vm, uint64_t start,
>>> -				    struct amdgpu_vm_pt_cursor *cursor)
>>> -{
>>> -	amdgpu_vm_pt_start(adev, vm, start, cursor);
>>> -	while (amdgpu_vm_pt_descendant(adev, cursor));
>>> -}
>>> -
>>> -/**
>>> - * amdgpu_vm_pt_next_leaf - get next leaf PD/PT
>>> - *
>>> - * @adev: amdgpu_device pointer
>>> - * @cursor: current state
>>> - *
>>> - * Walk the PD/PT tree to the next leaf node.
>>> - */
>>> -static void amdgpu_vm_pt_next_leaf(struct amdgpu_device *adev,
>>> -				   struct amdgpu_vm_pt_cursor *cursor)
>>> -{
>>> -	amdgpu_vm_pt_next(adev, cursor);
>>> -	if (cursor->pfn != ~0ll)
>>> -		while (amdgpu_vm_pt_descendant(adev, cursor));
>>> -}
>>> -
>>> -/**
>>> - * for_each_amdgpu_vm_pt_leaf - walk over all leaf PDs/PTs in the
>>> hierarchy
>>> - */
>>> -#define for_each_amdgpu_vm_pt_leaf(adev, vm, start, end, cursor)		\
>>> -	for (amdgpu_vm_pt_first_leaf((adev), (vm), (start), &(cursor));		\
>>> -	     (cursor).pfn <= end; amdgpu_vm_pt_next_leaf((adev), &(cursor)))
>>> -
>>>     /**
>>>      * amdgpu_vm_pt_first_dfs - start a deep first search
>>>      *
>>> @@ -919,74 +878,51 @@ static void amdgpu_vm_bo_param(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>>      * Returns:
>>>      * 0 on success, errno otherwise.
>>>      */
>>> -int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>>> -			struct amdgpu_vm *vm,
>>> -			uint64_t saddr, uint64_t size)
>>> +static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>>> +			       struct amdgpu_vm *vm,
>>> +			       struct amdgpu_vm_pt_cursor *cursor)
>>>     {
>>> -	struct amdgpu_vm_pt_cursor cursor;
>>> +	struct amdgpu_vm_pt *entry = cursor->entry;
>>> +	struct amdgpu_bo_param bp;
>>>     	struct amdgpu_bo *pt;
>>> -	uint64_t eaddr;
>>>     	int r;
>>>     
>>> -	/* validate the parameters */
>>> -	if (saddr & AMDGPU_GPU_PAGE_MASK || size & AMDGPU_GPU_PAGE_MASK)
>>> -		return -EINVAL;
>>> +	if (cursor->level < AMDGPU_VM_PTB && !entry->entries) {
>>> +		unsigned num_entries;
>>>     
>>> -	eaddr = saddr + size - 1;
>>> -
>>> -	saddr /= AMDGPU_GPU_PAGE_SIZE;
>>> -	eaddr /= AMDGPU_GPU_PAGE_SIZE;
>>> -
>>> -	if (eaddr >= adev->vm_manager.max_pfn) {
>>> -		dev_err(adev->dev, "va above limit (0x%08llX >= 0x%08llX)\n",
>>> -			eaddr, adev->vm_manager.max_pfn);
>>> -		return -EINVAL;
>>> +		num_entries = amdgpu_vm_num_entries(adev, cursor->level);
>>> +		entry->entries = kvmalloc_array(num_entries,
>>> +						sizeof(*entry->entries),
>>> +						GFP_KERNEL | __GFP_ZERO);
>>> +		if (!entry->entries)
>>> +			return -ENOMEM;
>>>     	}
>>>     
>>> -	for_each_amdgpu_vm_pt_leaf(adev, vm, saddr, eaddr, cursor) {
>>> -		struct amdgpu_vm_pt *entry = cursor.entry;
>>> -		struct amdgpu_bo_param bp;
>>> -
>>> -		if (cursor.level < AMDGPU_VM_PTB) {
>>> -			unsigned num_entries;
>>> -
>>> -			num_entries = amdgpu_vm_num_entries(adev, cursor.level);
>>> -			entry->entries = kvmalloc_array(num_entries,
>>> -							sizeof(*entry->entries),
>>> -							GFP_KERNEL |
>>> -							__GFP_ZERO);
>>> -			if (!entry->entries)
>>> -				return -ENOMEM;
>>> -		}
>>> -
>>> -
>>> -		if (entry->base.bo)
>>> -			continue;
>>> -
>>> -		amdgpu_vm_bo_param(adev, vm, cursor.level, &bp);
>>> -
>>> -		r = amdgpu_bo_create(adev, &bp, &pt);
>>> -		if (r)
>>> -			return r;
>>> -
>>> -		if (vm->use_cpu_for_update) {
>>> -			r = amdgpu_bo_kmap(pt, NULL);
>>> -			if (r)
>>> -				goto error_free_pt;
>>> -		}
>>> +	if (entry->base.bo)
>>> +		return 0;
>>>     
>>> -		/* Keep a reference to the root directory to avoid
>>> -		* freeing them up in the wrong order.
>>> -		*/
>>> -		pt->parent = amdgpu_bo_ref(cursor.parent->base.bo);
>>> +	amdgpu_vm_bo_param(adev, vm, cursor->level, &bp);
>>>     
>>> -		amdgpu_vm_bo_base_init(&entry->base, vm, pt);
>>> +	r = amdgpu_bo_create(adev, &bp, &pt);
>>> +	if (r)
>>> +		return r;
>>>     
>>> -		r = amdgpu_vm_clear_bo(adev, vm, pt);
>>> +	if (vm->use_cpu_for_update) {
>>> +		r = amdgpu_bo_kmap(pt, NULL);
>>>     		if (r)
>>>     			goto error_free_pt;
>>>     	}
>>>     
>>> +	/* Keep a reference to the root directory to avoid
>>> +	 * freeing them up in the wrong order.
>>> +	 */
>>> +	pt->parent = amdgpu_bo_ref(cursor->parent->base.bo);
>>> +	amdgpu_vm_bo_base_init(&entry->base, vm, pt);
>>> +
>>> +	r = amdgpu_vm_clear_bo(adev, vm, pt);
>>> +	if (r)
>>> +		goto error_free_pt;
>>> +
>>>     	return 0;
>>>     
>>>     error_free_pt:
>>> @@ -1655,6 +1591,7 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>>>     	struct amdgpu_vm_pt_cursor cursor;
>>>     	uint64_t frag_start = start, frag_end;
>>>     	unsigned int frag;
>>> +	int r;
>>>     
>>>     	/* figure out the initial fragment */
>>>     	amdgpu_vm_fragment(params, frag_start, end, flags, &frag,
>>> &frag_end); @@ -1662,12 +1599,15 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>>>     	/* walk over the address space and update the PTs */
>>>     	amdgpu_vm_pt_start(adev, params->vm, start, &cursor);
>>>     	while (cursor.pfn < end) {
>>> -		struct amdgpu_bo *pt = cursor.entry->base.bo;
>>>     		unsigned shift, parent_shift, mask;
>>>     		uint64_t incr, entry_end, pe_start;
>>> +		struct amdgpu_bo *pt;
>>>     
>>> -		if (!pt)
>>> -			return -ENOENT;
>>> +		r = amdgpu_vm_alloc_pts(params->adev, params->vm, &cursor);
>>> +		if (r)
>>> +			return r;
>>> +
>>> +		pt = cursor.entry->base.bo;
>>>     
>>>     		/* The root level can't be a huge page */
>>>     		if (cursor.level == adev->vm_manager.root_level) { diff --git
>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> index 81ff8177f092..116605c038d2 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> @@ -303,9 +303,6 @@ bool amdgpu_vm_ready(struct amdgpu_vm *vm);
>>>     int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>>     			      int (*callback)(void *p, struct amdgpu_bo *bo),
>>>     			      void *param);
>>> -int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>>> -			struct amdgpu_vm *vm,
>>> -			uint64_t saddr, uint64_t size);
>>>     int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job, bool need_pipe_sync);
>>>     int amdgpu_vm_update_directories(struct amdgpu_device *adev,
>>>     				 struct amdgpu_vm *vm);
> _______________________________________________
> 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 6/8] drm/amdgpu: allocate VM PDs/PTs on demand

[Kuehling, Felix]

I saw a backtrace from the GPU scheduler when I was debugging this yesterday, but those backtraces never tell us where the command submission came from. It could be memory initialization, or some migration at buffer-validation. Basically, any command submission triggered by page table allocation that synchronizes with the PD reservation object is a problem.

Regards,
  Felix

-----Original Message-----
From: Christian König <ckoenig.leichtzumerken@gmail.com> 
Sent: Tuesday, February 05, 2019 10:40 AM
To: Kuehling, Felix <Felix.Kuehling@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH 6/8] drm/amdgpu: allocate VM PDs/PTs on demand

Am 05.02.19 um 16:20 schrieb Kuehling, Felix:
>>> This may cause regressions in KFD if PT BO allocation can trigger 
>>> eviction fences.
>> I don't think that can happen, but need to double check as well.
>>
>> Otherwise allocating page tables could try to evict other page tables from the same process and that seriously doesn't make much sense.
> Eviction fences don't cause actual memory evictions. They are the result of memory evictions. They cause KFD processes to be preempted in order to allow the fence to signal so memory can be evicted. The problem is that a page table allocation waits for the fences on the PD reservation, which looks like an eviction to KFD and triggers an unnecessary preemption. There is no actual memory eviction happening here.

Yeah, but where is that actually coming from?

It sounds like we still unnecessary synchronize page table updates somewhere.

Do you have a call chain?

Regards,
Christian.

>
> Regards,
>    Felix
>
> -----Original Message-----
> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> Sent: Tuesday, February 05, 2019 6:37 AM
> To: Kuehling, Felix <Felix.Kuehling@amd.com>; 
> amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH 6/8] drm/amdgpu: allocate VM PDs/PTs on demand
>
>> This may cause regressions in KFD if PT BO allocation can trigger 
>> eviction fences.
> I don't think that can happen, but need to double check as well.
>
> Otherwise allocating page tables could try to evict other page tables from the same process and that seriously doesn't make much sense.
>
>> Do you know whether PT BO allocation would wait on the page-directory 
>> reservation object without the sync-object API anywhere?
> For inside the kernel I can't think of any relevant use case, except for the eviction call path and there it is intentional.
>
> One thing which will always wait for all fences is the display code path, but that is not relevant here. (Isn't it?).
>
> What might be questionable is amdgpu_gem_wait_idle_ioctl(), but I think this is also completely intentional that we wait on everything here.
>
> Regards,
> Christian.
>
> Am 04.02.19 um 21:17 schrieb Kuehling, Felix:
>> This may cause regressions in KFD if PT BO allocation can trigger 
>> eviction fences. The previous call to amdgpu_vm_alloc_pts was in a 
>> context where we had temporarily removed the eviction fence. PT BO 
>> allocation in amdgpu_vm_bo_update is not protected like that.
>>
>> I vaguely remember looking into this last time you were working on 
>> our eviction fence code and thinking that most of the cases where we 
>> remove the eviction fence were no longer needed if fence 
>> synchronization happens through the amdgpu_sync-object API (rather 
>> than waiting on a reservation object directly). I think it's time I 
>> go and finally clean that up.
>>
>> Do you know whether PT BO allocation would wait on the page-directory 
>> reservation object without the sync-object API anywhere?
>>
>> Regards,
>>      Felix
>>
>> On 2019-02-04 7:42 a.m., Christian König wrote:
>>> Let's start to allocate VM PDs/PTs on demand instead of 
>>> pre-allocating them during mapping.
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> ---
>>>     .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  |   6 -
>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c       |   9 --
>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c       |  10 --
>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        | 136 +++++-------------
>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |   3 -
>>>     5 files changed, 38 insertions(+), 126 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>> index d7b10d79f1de..2176c92f9377 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>> @@ -492,12 +492,6 @@ static int add_bo_to_vm(struct amdgpu_device *adev, struct kgd_mem *mem,
>>>     					vm->process_info->eviction_fence,
>>>     					NULL, NULL);
>>>     
>>> -	ret = amdgpu_vm_alloc_pts(adev, vm, va, amdgpu_bo_size(bo));
>>> -	if (ret) {
>>> -		pr_err("Failed to allocate pts, err=%d\n", ret);
>>> -		goto err_alloc_pts;
>>> -	}
>>> -
>>>     	ret = vm_validate_pt_pd_bos(vm);
>>>     	if (ret) {
>>>     		pr_err("validate_pt_pd_bos() failed\n"); diff --git 
>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>>> index 7e22be7ca68a..54dd02a898b9 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>>> @@ -92,15 +92,6 @@ int amdgpu_map_static_csa(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>>     		return -ENOMEM;
>>>     	}
>>>     
>>> -	r = amdgpu_vm_alloc_pts(adev, (*bo_va)->base.vm, csa_addr,
>>> -				size);
>>> -	if (r) {
>>> -		DRM_ERROR("failed to allocate pts for static CSA, err=%d\n", r);
>>> -		amdgpu_vm_bo_rmv(adev, *bo_va);
>>> -		ttm_eu_backoff_reservation(&ticket, &list);
>>> -		return r;
>>> -	}
>>> -
>>>     	r = amdgpu_vm_bo_map(adev, *bo_va, csa_addr, 0, size,
>>>     			     AMDGPU_PTE_READABLE | AMDGPU_PTE_WRITEABLE |
>>>     			     AMDGPU_PTE_EXECUTABLE);
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> index d21dd2f369da..e141e3b13112 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> @@ -627,11 +627,6 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, 
>>> void *data,
>>>     
>>>     	switch (args->operation) {
>>>     	case AMDGPU_VA_OP_MAP:
>>> -		r = amdgpu_vm_alloc_pts(adev, bo_va->base.vm, args->va_address,
>>> -					args->map_size);
>>> -		if (r)
>>> -			goto error_backoff;
>>> -
>>>     		va_flags = amdgpu_gmc_get_pte_flags(adev, args->flags);
>>>     		r = amdgpu_vm_bo_map(adev, bo_va, args->va_address,
>>>     				     args->offset_in_bo, args->map_size, @@ -647,11 +642,6 
>>> @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>>>     						args->map_size);
>>>     		break;
>>>     	case AMDGPU_VA_OP_REPLACE:
>>> -		r = amdgpu_vm_alloc_pts(adev, bo_va->base.vm, args->va_address,
>>> -					args->map_size);
>>> -		if (r)
>>> -			goto error_backoff;
>>> -
>>>     		va_flags = amdgpu_gmc_get_pte_flags(adev, args->flags);
>>>     		r = amdgpu_vm_bo_replace_map(adev, bo_va, args->va_address,
>>>     					     args->offset_in_bo, args->map_size, diff --git 
>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index f7d410a5d848..0b8a1aa56c4a 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -504,47 +504,6 @@ static void amdgpu_vm_pt_next(struct amdgpu_device *adev,
>>>     	}
>>>     }
>>>     
>>> -/**
>>> - * amdgpu_vm_pt_first_leaf - get first leaf PD/PT
>>> - *
>>> - * @adev: amdgpu_device pointer
>>> - * @vm: amdgpu_vm structure
>>> - * @start: start addr of the walk
>>> - * @cursor: state to initialize
>>> - *
>>> - * Start a walk and go directly to the leaf node.
>>> - */
>>> -static void amdgpu_vm_pt_first_leaf(struct amdgpu_device *adev,
>>> -				    struct amdgpu_vm *vm, uint64_t start,
>>> -				    struct amdgpu_vm_pt_cursor *cursor)
>>> -{
>>> -	amdgpu_vm_pt_start(adev, vm, start, cursor);
>>> -	while (amdgpu_vm_pt_descendant(adev, cursor));
>>> -}
>>> -
>>> -/**
>>> - * amdgpu_vm_pt_next_leaf - get next leaf PD/PT
>>> - *
>>> - * @adev: amdgpu_device pointer
>>> - * @cursor: current state
>>> - *
>>> - * Walk the PD/PT tree to the next leaf node.
>>> - */
>>> -static void amdgpu_vm_pt_next_leaf(struct amdgpu_device *adev,
>>> -				   struct amdgpu_vm_pt_cursor *cursor)
>>> -{
>>> -	amdgpu_vm_pt_next(adev, cursor);
>>> -	if (cursor->pfn != ~0ll)
>>> -		while (amdgpu_vm_pt_descendant(adev, cursor));
>>> -}
>>> -
>>> -/**
>>> - * for_each_amdgpu_vm_pt_leaf - walk over all leaf PDs/PTs in the 
>>> hierarchy
>>> - */
>>> -#define for_each_amdgpu_vm_pt_leaf(adev, vm, start, end, cursor)		\
>>> -	for (amdgpu_vm_pt_first_leaf((adev), (vm), (start), &(cursor));		\
>>> -	     (cursor).pfn <= end; amdgpu_vm_pt_next_leaf((adev), &(cursor)))
>>> -
>>>     /**
>>>      * amdgpu_vm_pt_first_dfs - start a deep first search
>>>      *
>>> @@ -919,74 +878,51 @@ static void amdgpu_vm_bo_param(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>>      * Returns:
>>>      * 0 on success, errno otherwise.
>>>      */
>>> -int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>>> -			struct amdgpu_vm *vm,
>>> -			uint64_t saddr, uint64_t size)
>>> +static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>>> +			       struct amdgpu_vm *vm,
>>> +			       struct amdgpu_vm_pt_cursor *cursor)
>>>     {
>>> -	struct amdgpu_vm_pt_cursor cursor;
>>> +	struct amdgpu_vm_pt *entry = cursor->entry;
>>> +	struct amdgpu_bo_param bp;
>>>     	struct amdgpu_bo *pt;
>>> -	uint64_t eaddr;
>>>     	int r;
>>>     
>>> -	/* validate the parameters */
>>> -	if (saddr & AMDGPU_GPU_PAGE_MASK || size & AMDGPU_GPU_PAGE_MASK)
>>> -		return -EINVAL;
>>> +	if (cursor->level < AMDGPU_VM_PTB && !entry->entries) {
>>> +		unsigned num_entries;
>>>     
>>> -	eaddr = saddr + size - 1;
>>> -
>>> -	saddr /= AMDGPU_GPU_PAGE_SIZE;
>>> -	eaddr /= AMDGPU_GPU_PAGE_SIZE;
>>> -
>>> -	if (eaddr >= adev->vm_manager.max_pfn) {
>>> -		dev_err(adev->dev, "va above limit (0x%08llX >= 0x%08llX)\n",
>>> -			eaddr, adev->vm_manager.max_pfn);
>>> -		return -EINVAL;
>>> +		num_entries = amdgpu_vm_num_entries(adev, cursor->level);
>>> +		entry->entries = kvmalloc_array(num_entries,
>>> +						sizeof(*entry->entries),
>>> +						GFP_KERNEL | __GFP_ZERO);
>>> +		if (!entry->entries)
>>> +			return -ENOMEM;
>>>     	}
>>>     
>>> -	for_each_amdgpu_vm_pt_leaf(adev, vm, saddr, eaddr, cursor) {
>>> -		struct amdgpu_vm_pt *entry = cursor.entry;
>>> -		struct amdgpu_bo_param bp;
>>> -
>>> -		if (cursor.level < AMDGPU_VM_PTB) {
>>> -			unsigned num_entries;
>>> -
>>> -			num_entries = amdgpu_vm_num_entries(adev, cursor.level);
>>> -			entry->entries = kvmalloc_array(num_entries,
>>> -							sizeof(*entry->entries),
>>> -							GFP_KERNEL |
>>> -							__GFP_ZERO);
>>> -			if (!entry->entries)
>>> -				return -ENOMEM;
>>> -		}
>>> -
>>> -
>>> -		if (entry->base.bo)
>>> -			continue;
>>> -
>>> -		amdgpu_vm_bo_param(adev, vm, cursor.level, &bp);
>>> -
>>> -		r = amdgpu_bo_create(adev, &bp, &pt);
>>> -		if (r)
>>> -			return r;
>>> -
>>> -		if (vm->use_cpu_for_update) {
>>> -			r = amdgpu_bo_kmap(pt, NULL);
>>> -			if (r)
>>> -				goto error_free_pt;
>>> -		}
>>> +	if (entry->base.bo)
>>> +		return 0;
>>>     
>>> -		/* Keep a reference to the root directory to avoid
>>> -		* freeing them up in the wrong order.
>>> -		*/
>>> -		pt->parent = amdgpu_bo_ref(cursor.parent->base.bo);
>>> +	amdgpu_vm_bo_param(adev, vm, cursor->level, &bp);
>>>     
>>> -		amdgpu_vm_bo_base_init(&entry->base, vm, pt);
>>> +	r = amdgpu_bo_create(adev, &bp, &pt);
>>> +	if (r)
>>> +		return r;
>>>     
>>> -		r = amdgpu_vm_clear_bo(adev, vm, pt);
>>> +	if (vm->use_cpu_for_update) {
>>> +		r = amdgpu_bo_kmap(pt, NULL);
>>>     		if (r)
>>>     			goto error_free_pt;
>>>     	}
>>>     
>>> +	/* Keep a reference to the root directory to avoid
>>> +	 * freeing them up in the wrong order.
>>> +	 */
>>> +	pt->parent = amdgpu_bo_ref(cursor->parent->base.bo);
>>> +	amdgpu_vm_bo_base_init(&entry->base, vm, pt);
>>> +
>>> +	r = amdgpu_vm_clear_bo(adev, vm, pt);
>>> +	if (r)
>>> +		goto error_free_pt;
>>> +
>>>     	return 0;
>>>     
>>>     error_free_pt:
>>> @@ -1655,6 +1591,7 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>>>     	struct amdgpu_vm_pt_cursor cursor;
>>>     	uint64_t frag_start = start, frag_end;
>>>     	unsigned int frag;
>>> +	int r;
>>>     
>>>     	/* figure out the initial fragment */
>>>     	amdgpu_vm_fragment(params, frag_start, end, flags, &frag, 
>>> &frag_end); @@ -1662,12 +1599,15 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>>>     	/* walk over the address space and update the PTs */
>>>     	amdgpu_vm_pt_start(adev, params->vm, start, &cursor);
>>>     	while (cursor.pfn < end) {
>>> -		struct amdgpu_bo *pt = cursor.entry->base.bo;
>>>     		unsigned shift, parent_shift, mask;
>>>     		uint64_t incr, entry_end, pe_start;
>>> +		struct amdgpu_bo *pt;
>>>     
>>> -		if (!pt)
>>> -			return -ENOENT;
>>> +		r = amdgpu_vm_alloc_pts(params->adev, params->vm, &cursor);
>>> +		if (r)
>>> +			return r;
>>> +
>>> +		pt = cursor.entry->base.bo;
>>>     
>>>     		/* The root level can't be a huge page */
>>>     		if (cursor.level == adev->vm_manager.root_level) { diff --git 
>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> index 81ff8177f092..116605c038d2 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> @@ -303,9 +303,6 @@ bool amdgpu_vm_ready(struct amdgpu_vm *vm);
>>>     int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>>     			      int (*callback)(void *p, struct amdgpu_bo *bo),
>>>     			      void *param);
>>> -int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>>> -			struct amdgpu_vm *vm,
>>> -			uint64_t saddr, uint64_t size);
>>>     int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job, bool need_pipe_sync);
>>>     int amdgpu_vm_update_directories(struct amdgpu_device *adev,
>>>     				 struct amdgpu_vm *vm);
> _______________________________________________
> 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 6/8] drm/amdgpu: allocate VM PDs/PTs on demand

[Koenig, Christian]

Ah! The initial clear of the PDs is syncing to everything!

Ok, that is easy to fix.

Christian.

Am 05.02.19 um 16:49 schrieb Kuehling, Felix:
> I saw a backtrace from the GPU scheduler when I was debugging this yesterday, but those backtraces never tell us where the command submission came from. It could be memory initialization, or some migration at buffer-validation. Basically, any command submission triggered by page table allocation that synchronizes with the PD reservation object is a problem.
>
> Regards,
>    Felix
>
> -----Original Message-----
> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> Sent: Tuesday, February 05, 2019 10:40 AM
> To: Kuehling, Felix <Felix.Kuehling@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH 6/8] drm/amdgpu: allocate VM PDs/PTs on demand
>
> Am 05.02.19 um 16:20 schrieb Kuehling, Felix:
>>>> This may cause regressions in KFD if PT BO allocation can trigger
>>>> eviction fences.
>>> I don't think that can happen, but need to double check as well.
>>>
>>> Otherwise allocating page tables could try to evict other page tables from the same process and that seriously doesn't make much sense.
>> Eviction fences don't cause actual memory evictions. They are the result of memory evictions. They cause KFD processes to be preempted in order to allow the fence to signal so memory can be evicted. The problem is that a page table allocation waits for the fences on the PD reservation, which looks like an eviction to KFD and triggers an unnecessary preemption. There is no actual memory eviction happening here.
> Yeah, but where is that actually coming from?
>
> It sounds like we still unnecessary synchronize page table updates somewhere.
>
> Do you have a call chain?
>
> Regards,
> Christian.
>
>> Regards,
>>     Felix
>>
>> -----Original Message-----
>> From: Christian König <ckoenig.leichtzumerken@gmail.com>
>> Sent: Tuesday, February 05, 2019 6:37 AM
>> To: Kuehling, Felix <Felix.Kuehling@amd.com>;
>> amd-gfx@lists.freedesktop.org
>> Subject: Re: [PATCH 6/8] drm/amdgpu: allocate VM PDs/PTs on demand
>>
>>> This may cause regressions in KFD if PT BO allocation can trigger
>>> eviction fences.
>> I don't think that can happen, but need to double check as well.
>>
>> Otherwise allocating page tables could try to evict other page tables from the same process and that seriously doesn't make much sense.
>>
>>> Do you know whether PT BO allocation would wait on the page-directory
>>> reservation object without the sync-object API anywhere?
>> For inside the kernel I can't think of any relevant use case, except for the eviction call path and there it is intentional.
>>
>> One thing which will always wait for all fences is the display code path, but that is not relevant here. (Isn't it?).
>>
>> What might be questionable is amdgpu_gem_wait_idle_ioctl(), but I think this is also completely intentional that we wait on everything here.
>>
>> Regards,
>> Christian.
>>
>> Am 04.02.19 um 21:17 schrieb Kuehling, Felix:
>>> This may cause regressions in KFD if PT BO allocation can trigger
>>> eviction fences. The previous call to amdgpu_vm_alloc_pts was in a
>>> context where we had temporarily removed the eviction fence. PT BO
>>> allocation in amdgpu_vm_bo_update is not protected like that.
>>>
>>> I vaguely remember looking into this last time you were working on
>>> our eviction fence code and thinking that most of the cases where we
>>> remove the eviction fence were no longer needed if fence
>>> synchronization happens through the amdgpu_sync-object API (rather
>>> than waiting on a reservation object directly). I think it's time I
>>> go and finally clean that up.
>>>
>>> Do you know whether PT BO allocation would wait on the page-directory
>>> reservation object without the sync-object API anywhere?
>>>
>>> Regards,
>>>       Felix
>>>
>>> On 2019-02-04 7:42 a.m., Christian König wrote:
>>>> Let's start to allocate VM PDs/PTs on demand instead of
>>>> pre-allocating them during mapping.
>>>>
>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>> ---
>>>>      .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  |   6 -
>>>>      drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c       |   9 --
>>>>      drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c       |  10 --
>>>>      drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        | 136 +++++-------------
>>>>      drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |   3 -
>>>>      5 files changed, 38 insertions(+), 126 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>> index d7b10d79f1de..2176c92f9377 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>> @@ -492,12 +492,6 @@ static int add_bo_to_vm(struct amdgpu_device *adev, struct kgd_mem *mem,
>>>>      					vm->process_info->eviction_fence,
>>>>      					NULL, NULL);
>>>>      
>>>> -	ret = amdgpu_vm_alloc_pts(adev, vm, va, amdgpu_bo_size(bo));
>>>> -	if (ret) {
>>>> -		pr_err("Failed to allocate pts, err=%d\n", ret);
>>>> -		goto err_alloc_pts;
>>>> -	}
>>>> -
>>>>      	ret = vm_validate_pt_pd_bos(vm);
>>>>      	if (ret) {
>>>>      		pr_err("validate_pt_pd_bos() failed\n"); diff --git
>>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>>>> index 7e22be7ca68a..54dd02a898b9 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>>>> @@ -92,15 +92,6 @@ int amdgpu_map_static_csa(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>      		return -ENOMEM;
>>>>      	}
>>>>      
>>>> -	r = amdgpu_vm_alloc_pts(adev, (*bo_va)->base.vm, csa_addr,
>>>> -				size);
>>>> -	if (r) {
>>>> -		DRM_ERROR("failed to allocate pts for static CSA, err=%d\n", r);
>>>> -		amdgpu_vm_bo_rmv(adev, *bo_va);
>>>> -		ttm_eu_backoff_reservation(&ticket, &list);
>>>> -		return r;
>>>> -	}
>>>> -
>>>>      	r = amdgpu_vm_bo_map(adev, *bo_va, csa_addr, 0, size,
>>>>      			     AMDGPU_PTE_READABLE | AMDGPU_PTE_WRITEABLE |
>>>>      			     AMDGPU_PTE_EXECUTABLE);
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>> index d21dd2f369da..e141e3b13112 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>> @@ -627,11 +627,6 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev,
>>>> void *data,
>>>>      
>>>>      	switch (args->operation) {
>>>>      	case AMDGPU_VA_OP_MAP:
>>>> -		r = amdgpu_vm_alloc_pts(adev, bo_va->base.vm, args->va_address,
>>>> -					args->map_size);
>>>> -		if (r)
>>>> -			goto error_backoff;
>>>> -
>>>>      		va_flags = amdgpu_gmc_get_pte_flags(adev, args->flags);
>>>>      		r = amdgpu_vm_bo_map(adev, bo_va, args->va_address,
>>>>      				     args->offset_in_bo, args->map_size, @@ -647,11 +642,6
>>>> @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>>>>      						args->map_size);
>>>>      		break;
>>>>      	case AMDGPU_VA_OP_REPLACE:
>>>> -		r = amdgpu_vm_alloc_pts(adev, bo_va->base.vm, args->va_address,
>>>> -					args->map_size);
>>>> -		if (r)
>>>> -			goto error_backoff;
>>>> -
>>>>      		va_flags = amdgpu_gmc_get_pte_flags(adev, args->flags);
>>>>      		r = amdgpu_vm_bo_replace_map(adev, bo_va, args->va_address,
>>>>      					     args->offset_in_bo, args->map_size, diff --git
>>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> index f7d410a5d848..0b8a1aa56c4a 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> @@ -504,47 +504,6 @@ static void amdgpu_vm_pt_next(struct amdgpu_device *adev,
>>>>      	}
>>>>      }
>>>>      
>>>> -/**
>>>> - * amdgpu_vm_pt_first_leaf - get first leaf PD/PT
>>>> - *
>>>> - * @adev: amdgpu_device pointer
>>>> - * @vm: amdgpu_vm structure
>>>> - * @start: start addr of the walk
>>>> - * @cursor: state to initialize
>>>> - *
>>>> - * Start a walk and go directly to the leaf node.
>>>> - */
>>>> -static void amdgpu_vm_pt_first_leaf(struct amdgpu_device *adev,
>>>> -				    struct amdgpu_vm *vm, uint64_t start,
>>>> -				    struct amdgpu_vm_pt_cursor *cursor)
>>>> -{
>>>> -	amdgpu_vm_pt_start(adev, vm, start, cursor);
>>>> -	while (amdgpu_vm_pt_descendant(adev, cursor));
>>>> -}
>>>> -
>>>> -/**
>>>> - * amdgpu_vm_pt_next_leaf - get next leaf PD/PT
>>>> - *
>>>> - * @adev: amdgpu_device pointer
>>>> - * @cursor: current state
>>>> - *
>>>> - * Walk the PD/PT tree to the next leaf node.
>>>> - */
>>>> -static void amdgpu_vm_pt_next_leaf(struct amdgpu_device *adev,
>>>> -				   struct amdgpu_vm_pt_cursor *cursor)
>>>> -{
>>>> -	amdgpu_vm_pt_next(adev, cursor);
>>>> -	if (cursor->pfn != ~0ll)
>>>> -		while (amdgpu_vm_pt_descendant(adev, cursor));
>>>> -}
>>>> -
>>>> -/**
>>>> - * for_each_amdgpu_vm_pt_leaf - walk over all leaf PDs/PTs in the
>>>> hierarchy
>>>> - */
>>>> -#define for_each_amdgpu_vm_pt_leaf(adev, vm, start, end, cursor)		\
>>>> -	for (amdgpu_vm_pt_first_leaf((adev), (vm), (start), &(cursor));		\
>>>> -	     (cursor).pfn <= end; amdgpu_vm_pt_next_leaf((adev), &(cursor)))
>>>> -
>>>>      /**
>>>>       * amdgpu_vm_pt_first_dfs - start a deep first search
>>>>       *
>>>> @@ -919,74 +878,51 @@ static void amdgpu_vm_bo_param(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>       * Returns:
>>>>       * 0 on success, errno otherwise.
>>>>       */
>>>> -int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>>>> -			struct amdgpu_vm *vm,
>>>> -			uint64_t saddr, uint64_t size)
>>>> +static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>>>> +			       struct amdgpu_vm *vm,
>>>> +			       struct amdgpu_vm_pt_cursor *cursor)
>>>>      {
>>>> -	struct amdgpu_vm_pt_cursor cursor;
>>>> +	struct amdgpu_vm_pt *entry = cursor->entry;
>>>> +	struct amdgpu_bo_param bp;
>>>>      	struct amdgpu_bo *pt;
>>>> -	uint64_t eaddr;
>>>>      	int r;
>>>>      
>>>> -	/* validate the parameters */
>>>> -	if (saddr & AMDGPU_GPU_PAGE_MASK || size & AMDGPU_GPU_PAGE_MASK)
>>>> -		return -EINVAL;
>>>> +	if (cursor->level < AMDGPU_VM_PTB && !entry->entries) {
>>>> +		unsigned num_entries;
>>>>      
>>>> -	eaddr = saddr + size - 1;
>>>> -
>>>> -	saddr /= AMDGPU_GPU_PAGE_SIZE;
>>>> -	eaddr /= AMDGPU_GPU_PAGE_SIZE;
>>>> -
>>>> -	if (eaddr >= adev->vm_manager.max_pfn) {
>>>> -		dev_err(adev->dev, "va above limit (0x%08llX >= 0x%08llX)\n",
>>>> -			eaddr, adev->vm_manager.max_pfn);
>>>> -		return -EINVAL;
>>>> +		num_entries = amdgpu_vm_num_entries(adev, cursor->level);
>>>> +		entry->entries = kvmalloc_array(num_entries,
>>>> +						sizeof(*entry->entries),
>>>> +						GFP_KERNEL | __GFP_ZERO);
>>>> +		if (!entry->entries)
>>>> +			return -ENOMEM;
>>>>      	}
>>>>      
>>>> -	for_each_amdgpu_vm_pt_leaf(adev, vm, saddr, eaddr, cursor) {
>>>> -		struct amdgpu_vm_pt *entry = cursor.entry;
>>>> -		struct amdgpu_bo_param bp;
>>>> -
>>>> -		if (cursor.level < AMDGPU_VM_PTB) {
>>>> -			unsigned num_entries;
>>>> -
>>>> -			num_entries = amdgpu_vm_num_entries(adev, cursor.level);
>>>> -			entry->entries = kvmalloc_array(num_entries,
>>>> -							sizeof(*entry->entries),
>>>> -							GFP_KERNEL |
>>>> -							__GFP_ZERO);
>>>> -			if (!entry->entries)
>>>> -				return -ENOMEM;
>>>> -		}
>>>> -
>>>> -
>>>> -		if (entry->base.bo)
>>>> -			continue;
>>>> -
>>>> -		amdgpu_vm_bo_param(adev, vm, cursor.level, &bp);
>>>> -
>>>> -		r = amdgpu_bo_create(adev, &bp, &pt);
>>>> -		if (r)
>>>> -			return r;
>>>> -
>>>> -		if (vm->use_cpu_for_update) {
>>>> -			r = amdgpu_bo_kmap(pt, NULL);
>>>> -			if (r)
>>>> -				goto error_free_pt;
>>>> -		}
>>>> +	if (entry->base.bo)
>>>> +		return 0;
>>>>      
>>>> -		/* Keep a reference to the root directory to avoid
>>>> -		* freeing them up in the wrong order.
>>>> -		*/
>>>> -		pt->parent = amdgpu_bo_ref(cursor.parent->base.bo);
>>>> +	amdgpu_vm_bo_param(adev, vm, cursor->level, &bp);
>>>>      
>>>> -		amdgpu_vm_bo_base_init(&entry->base, vm, pt);
>>>> +	r = amdgpu_bo_create(adev, &bp, &pt);
>>>> +	if (r)
>>>> +		return r;
>>>>      
>>>> -		r = amdgpu_vm_clear_bo(adev, vm, pt);
>>>> +	if (vm->use_cpu_for_update) {
>>>> +		r = amdgpu_bo_kmap(pt, NULL);
>>>>      		if (r)
>>>>      			goto error_free_pt;
>>>>      	}
>>>>      
>>>> +	/* Keep a reference to the root directory to avoid
>>>> +	 * freeing them up in the wrong order.
>>>> +	 */
>>>> +	pt->parent = amdgpu_bo_ref(cursor->parent->base.bo);
>>>> +	amdgpu_vm_bo_base_init(&entry->base, vm, pt);
>>>> +
>>>> +	r = amdgpu_vm_clear_bo(adev, vm, pt);
>>>> +	if (r)
>>>> +		goto error_free_pt;
>>>> +
>>>>      	return 0;
>>>>      
>>>>      error_free_pt:
>>>> @@ -1655,6 +1591,7 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>>>>      	struct amdgpu_vm_pt_cursor cursor;
>>>>      	uint64_t frag_start = start, frag_end;
>>>>      	unsigned int frag;
>>>> +	int r;
>>>>      
>>>>      	/* figure out the initial fragment */
>>>>      	amdgpu_vm_fragment(params, frag_start, end, flags, &frag,
>>>> &frag_end); @@ -1662,12 +1599,15 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>>>>      	/* walk over the address space and update the PTs */
>>>>      	amdgpu_vm_pt_start(adev, params->vm, start, &cursor);
>>>>      	while (cursor.pfn < end) {
>>>> -		struct amdgpu_bo *pt = cursor.entry->base.bo;
>>>>      		unsigned shift, parent_shift, mask;
>>>>      		uint64_t incr, entry_end, pe_start;
>>>> +		struct amdgpu_bo *pt;
>>>>      
>>>> -		if (!pt)
>>>> -			return -ENOENT;
>>>> +		r = amdgpu_vm_alloc_pts(params->adev, params->vm, &cursor);
>>>> +		if (r)
>>>> +			return r;
>>>> +
>>>> +		pt = cursor.entry->base.bo;
>>>>      
>>>>      		/* The root level can't be a huge page */
>>>>      		if (cursor.level == adev->vm_manager.root_level) { diff --git
>>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>> index 81ff8177f092..116605c038d2 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>> @@ -303,9 +303,6 @@ bool amdgpu_vm_ready(struct amdgpu_vm *vm);
>>>>      int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>      			      int (*callback)(void *p, struct amdgpu_bo *bo),
>>>>      			      void *param);
>>>> -int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>>>> -			struct amdgpu_vm *vm,
>>>> -			uint64_t saddr, uint64_t size);
>>>>      int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job, bool need_pipe_sync);
>>>>      int amdgpu_vm_update_directories(struct amdgpu_device *adev,
>>>>      				 struct amdgpu_vm *vm);
>> _______________________________________________
>> 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 6/8] drm/amdgpu: allocate VM PDs/PTs on demand

[Kuehling, Felix]

On 2019-02-05 11:00 a.m., Koenig, Christian wrote:
> Ah! The initial clear of the PDs is syncing to everything!

Right. Using amdgpu_sync_resv(... AMDGPU_FENCE_OWNER_VM ...) in 
amdgpu_vm_clear_bo seems to help. But if I also change the 
amdgpu_job_submit to use AMDGPU_FENCE_OWNER_VM, I get a VM fault and CP 
hang very early in my test. Not sure what's happening there. This is 
what I changed:

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 8b240f9..06c28ac 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -826,17 +826,18 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
  
         WARN_ON(job->ibs[0].length_dw > 64);
         r = amdgpu_sync_resv(adev, &job->sync, bo->tbo.resv,
-                            AMDGPU_FENCE_OWNER_UNDEFINED, false);
+                            AMDGPU_FENCE_OWNER_VM, false);
         if (r)
                 goto error_free;
  
-       r = amdgpu_job_submit(job, &vm->entity, AMDGPU_FENCE_OWNER_UNDEFINED,
+       r = amdgpu_job_submit(job, &vm->entity, AMDGPU_FENCE_OWNER_VM,
                               &fence);
         if (r)
                 goto error_free;
  
         amdgpu_bo_fence(bo, fence, true);
-       dma_fence_put(fence);
+       dma_fence_put(vm->last_update);
+       vm->last_update = fence;
  
         if (bo->shadow)
                 return amdgpu_vm_clear_bo(adev, vm, bo->shadow,

Basically I tried to do the synchronization exactly like 
amdgpu_vm_update_directories.

The only way this clear could cause a VM fault is, if a subsequent 
PTE/PDS update happens too early and gets overwritten by the pending 
clear. But don't the clear and the next PTE/PDE update go to the same 
queue (vm->entity) and are implicitly synchronized?

Answer after another hour of pouring over code and history: No, sched 
entities are no longer equivalent to queues. The scheduler can itself 
load-balance VM updates using multiple SDMA queues. Sigh.

OK, so page table updates to different PTEs don't usually need to 
synchronize with each other. But how do page table updates to the same 
address get synchronized? How do you guarantee that two updates of the 
same PTE are processed by the hardware in the correct order, if 
AMDGPU_FENCE_OWNER_VM fences don't synchronize with each other?


>
> Ok, that is easy to fix.

Not that easy. See above. ;)

Regards,
   Felix



>
> Christian.
>
> Am 05.02.19 um 16:49 schrieb Kuehling, Felix:
>> I saw a backtrace from the GPU scheduler when I was debugging this yesterday, but those backtraces never tell us where the command submission came from. It could be memory initialization, or some migration at buffer-validation. Basically, any command submission triggered by page table allocation that synchronizes with the PD reservation object is a problem.
>>
>> Regards,
>>     Felix
>>
>> -----Original Message-----
>> From: Christian König <ckoenig.leichtzumerken@gmail.com>
>> Sent: Tuesday, February 05, 2019 10:40 AM
>> To: Kuehling, Felix <Felix.Kuehling@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; amd-gfx@lists.freedesktop.org
>> Subject: Re: [PATCH 6/8] drm/amdgpu: allocate VM PDs/PTs on demand
>>
>> Am 05.02.19 um 16:20 schrieb Kuehling, Felix:
>>>>> This may cause regressions in KFD if PT BO allocation can trigger
>>>>> eviction fences.
>>>> I don't think that can happen, but need to double check as well.
>>>>
>>>> Otherwise allocating page tables could try to evict other page tables from the same process and that seriously doesn't make much sense.
>>> Eviction fences don't cause actual memory evictions. They are the result of memory evictions. They cause KFD processes to be preempted in order to allow the fence to signal so memory can be evicted. The problem is that a page table allocation waits for the fences on the PD reservation, which looks like an eviction to KFD and triggers an unnecessary preemption. There is no actual memory eviction happening here.
>> Yeah, but where is that actually coming from?
>>
>> It sounds like we still unnecessary synchronize page table updates somewhere.
>>
>> Do you have a call chain?
>>
>> Regards,
>> Christian.
>>
>>> Regards,
>>>      Felix
>>>
>>> -----Original Message-----
>>> From: Christian König <ckoenig.leichtzumerken@gmail.com>
>>> Sent: Tuesday, February 05, 2019 6:37 AM
>>> To: Kuehling, Felix <Felix.Kuehling@amd.com>;
>>> amd-gfx@lists.freedesktop.org
>>> Subject: Re: [PATCH 6/8] drm/amdgpu: allocate VM PDs/PTs on demand
>>>
>>>> This may cause regressions in KFD if PT BO allocation can trigger
>>>> eviction fences.
>>> I don't think that can happen, but need to double check as well.
>>>
>>> Otherwise allocating page tables could try to evict other page tables from the same process and that seriously doesn't make much sense.
>>>
>>>> Do you know whether PT BO allocation would wait on the page-directory
>>>> reservation object without the sync-object API anywhere?
>>> For inside the kernel I can't think of any relevant use case, except for the eviction call path and there it is intentional.
>>>
>>> One thing which will always wait for all fences is the display code path, but that is not relevant here. (Isn't it?).
>>>
>>> What might be questionable is amdgpu_gem_wait_idle_ioctl(), but I think this is also completely intentional that we wait on everything here.
>>>
>>> Regards,
>>> Christian.
>>>
>>> Am 04.02.19 um 21:17 schrieb Kuehling, Felix:
>>>> This may cause regressions in KFD if PT BO allocation can trigger
>>>> eviction fences. The previous call to amdgpu_vm_alloc_pts was in a
>>>> context where we had temporarily removed the eviction fence. PT BO
>>>> allocation in amdgpu_vm_bo_update is not protected like that.
>>>>
>>>> I vaguely remember looking into this last time you were working on
>>>> our eviction fence code and thinking that most of the cases where we
>>>> remove the eviction fence were no longer needed if fence
>>>> synchronization happens through the amdgpu_sync-object API (rather
>>>> than waiting on a reservation object directly). I think it's time I
>>>> go and finally clean that up.
>>>>
>>>> Do you know whether PT BO allocation would wait on the page-directory
>>>> reservation object without the sync-object API anywhere?
>>>>
>>>> Regards,
>>>>        Felix
>>>>
>>>> On 2019-02-04 7:42 a.m., Christian König wrote:
>>>>> Let's start to allocate VM PDs/PTs on demand instead of
>>>>> pre-allocating them during mapping.
>>>>>
>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>> ---
>>>>>       .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  |   6 -
>>>>>       drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c       |   9 --
>>>>>       drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c       |  10 --
>>>>>       drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        | 136 +++++-------------
>>>>>       drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |   3 -
>>>>>       5 files changed, 38 insertions(+), 126 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>>> index d7b10d79f1de..2176c92f9377 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>>> @@ -492,12 +492,6 @@ static int add_bo_to_vm(struct amdgpu_device *adev, struct kgd_mem *mem,
>>>>>       					vm->process_info->eviction_fence,
>>>>>       					NULL, NULL);
>>>>>       
>>>>> -	ret = amdgpu_vm_alloc_pts(adev, vm, va, amdgpu_bo_size(bo));
>>>>> -	if (ret) {
>>>>> -		pr_err("Failed to allocate pts, err=%d\n", ret);
>>>>> -		goto err_alloc_pts;
>>>>> -	}
>>>>> -
>>>>>       	ret = vm_validate_pt_pd_bos(vm);
>>>>>       	if (ret) {
>>>>>       		pr_err("validate_pt_pd_bos() failed\n"); diff --git
>>>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>>>>> index 7e22be7ca68a..54dd02a898b9 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>>>>> @@ -92,15 +92,6 @@ int amdgpu_map_static_csa(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>>       		return -ENOMEM;
>>>>>       	}
>>>>>       
>>>>> -	r = amdgpu_vm_alloc_pts(adev, (*bo_va)->base.vm, csa_addr,
>>>>> -				size);
>>>>> -	if (r) {
>>>>> -		DRM_ERROR("failed to allocate pts for static CSA, err=%d\n", r);
>>>>> -		amdgpu_vm_bo_rmv(adev, *bo_va);
>>>>> -		ttm_eu_backoff_reservation(&ticket, &list);
>>>>> -		return r;
>>>>> -	}
>>>>> -
>>>>>       	r = amdgpu_vm_bo_map(adev, *bo_va, csa_addr, 0, size,
>>>>>       			     AMDGPU_PTE_READABLE | AMDGPU_PTE_WRITEABLE |
>>>>>       			     AMDGPU_PTE_EXECUTABLE);
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> index d21dd2f369da..e141e3b13112 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> @@ -627,11 +627,6 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev,
>>>>> void *data,
>>>>>       
>>>>>       	switch (args->operation) {
>>>>>       	case AMDGPU_VA_OP_MAP:
>>>>> -		r = amdgpu_vm_alloc_pts(adev, bo_va->base.vm, args->va_address,
>>>>> -					args->map_size);
>>>>> -		if (r)
>>>>> -			goto error_backoff;
>>>>> -
>>>>>       		va_flags = amdgpu_gmc_get_pte_flags(adev, args->flags);
>>>>>       		r = amdgpu_vm_bo_map(adev, bo_va, args->va_address,
>>>>>       				     args->offset_in_bo, args->map_size, @@ -647,11 +642,6
>>>>> @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>>>>>       						args->map_size);
>>>>>       		break;
>>>>>       	case AMDGPU_VA_OP_REPLACE:
>>>>> -		r = amdgpu_vm_alloc_pts(adev, bo_va->base.vm, args->va_address,
>>>>> -					args->map_size);
>>>>> -		if (r)
>>>>> -			goto error_backoff;
>>>>> -
>>>>>       		va_flags = amdgpu_gmc_get_pte_flags(adev, args->flags);
>>>>>       		r = amdgpu_vm_bo_replace_map(adev, bo_va, args->va_address,
>>>>>       					     args->offset_in_bo, args->map_size, diff --git
>>>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> index f7d410a5d848..0b8a1aa56c4a 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> @@ -504,47 +504,6 @@ static void amdgpu_vm_pt_next(struct amdgpu_device *adev,
>>>>>       	}
>>>>>       }
>>>>>       
>>>>> -/**
>>>>> - * amdgpu_vm_pt_first_leaf - get first leaf PD/PT
>>>>> - *
>>>>> - * @adev: amdgpu_device pointer
>>>>> - * @vm: amdgpu_vm structure
>>>>> - * @start: start addr of the walk
>>>>> - * @cursor: state to initialize
>>>>> - *
>>>>> - * Start a walk and go directly to the leaf node.
>>>>> - */
>>>>> -static void amdgpu_vm_pt_first_leaf(struct amdgpu_device *adev,
>>>>> -				    struct amdgpu_vm *vm, uint64_t start,
>>>>> -				    struct amdgpu_vm_pt_cursor *cursor)
>>>>> -{
>>>>> -	amdgpu_vm_pt_start(adev, vm, start, cursor);
>>>>> -	while (amdgpu_vm_pt_descendant(adev, cursor));
>>>>> -}
>>>>> -
>>>>> -/**
>>>>> - * amdgpu_vm_pt_next_leaf - get next leaf PD/PT
>>>>> - *
>>>>> - * @adev: amdgpu_device pointer
>>>>> - * @cursor: current state
>>>>> - *
>>>>> - * Walk the PD/PT tree to the next leaf node.
>>>>> - */
>>>>> -static void amdgpu_vm_pt_next_leaf(struct amdgpu_device *adev,
>>>>> -				   struct amdgpu_vm_pt_cursor *cursor)
>>>>> -{
>>>>> -	amdgpu_vm_pt_next(adev, cursor);
>>>>> -	if (cursor->pfn != ~0ll)
>>>>> -		while (amdgpu_vm_pt_descendant(adev, cursor));
>>>>> -}
>>>>> -
>>>>> -/**
>>>>> - * for_each_amdgpu_vm_pt_leaf - walk over all leaf PDs/PTs in the
>>>>> hierarchy
>>>>> - */
>>>>> -#define for_each_amdgpu_vm_pt_leaf(adev, vm, start, end, cursor)		\
>>>>> -	for (amdgpu_vm_pt_first_leaf((adev), (vm), (start), &(cursor));		\
>>>>> -	     (cursor).pfn <= end; amdgpu_vm_pt_next_leaf((adev), &(cursor)))
>>>>> -
>>>>>       /**
>>>>>        * amdgpu_vm_pt_first_dfs - start a deep first search
>>>>>        *
>>>>> @@ -919,74 +878,51 @@ static void amdgpu_vm_bo_param(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>>        * Returns:
>>>>>        * 0 on success, errno otherwise.
>>>>>        */
>>>>> -int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>>>>> -			struct amdgpu_vm *vm,
>>>>> -			uint64_t saddr, uint64_t size)
>>>>> +static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>>>>> +			       struct amdgpu_vm *vm,
>>>>> +			       struct amdgpu_vm_pt_cursor *cursor)
>>>>>       {
>>>>> -	struct amdgpu_vm_pt_cursor cursor;
>>>>> +	struct amdgpu_vm_pt *entry = cursor->entry;
>>>>> +	struct amdgpu_bo_param bp;
>>>>>       	struct amdgpu_bo *pt;
>>>>> -	uint64_t eaddr;
>>>>>       	int r;
>>>>>       
>>>>> -	/* validate the parameters */
>>>>> -	if (saddr & AMDGPU_GPU_PAGE_MASK || size & AMDGPU_GPU_PAGE_MASK)
>>>>> -		return -EINVAL;
>>>>> +	if (cursor->level < AMDGPU_VM_PTB && !entry->entries) {
>>>>> +		unsigned num_entries;
>>>>>       
>>>>> -	eaddr = saddr + size - 1;
>>>>> -
>>>>> -	saddr /= AMDGPU_GPU_PAGE_SIZE;
>>>>> -	eaddr /= AMDGPU_GPU_PAGE_SIZE;
>>>>> -
>>>>> -	if (eaddr >= adev->vm_manager.max_pfn) {
>>>>> -		dev_err(adev->dev, "va above limit (0x%08llX >= 0x%08llX)\n",
>>>>> -			eaddr, adev->vm_manager.max_pfn);
>>>>> -		return -EINVAL;
>>>>> +		num_entries = amdgpu_vm_num_entries(adev, cursor->level);
>>>>> +		entry->entries = kvmalloc_array(num_entries,
>>>>> +						sizeof(*entry->entries),
>>>>> +						GFP_KERNEL | __GFP_ZERO);
>>>>> +		if (!entry->entries)
>>>>> +			return -ENOMEM;
>>>>>       	}
>>>>>       
>>>>> -	for_each_amdgpu_vm_pt_leaf(adev, vm, saddr, eaddr, cursor) {
>>>>> -		struct amdgpu_vm_pt *entry = cursor.entry;
>>>>> -		struct amdgpu_bo_param bp;
>>>>> -
>>>>> -		if (cursor.level < AMDGPU_VM_PTB) {
>>>>> -			unsigned num_entries;
>>>>> -
>>>>> -			num_entries = amdgpu_vm_num_entries(adev, cursor.level);
>>>>> -			entry->entries = kvmalloc_array(num_entries,
>>>>> -							sizeof(*entry->entries),
>>>>> -							GFP_KERNEL |
>>>>> -							__GFP_ZERO);
>>>>> -			if (!entry->entries)
>>>>> -				return -ENOMEM;
>>>>> -		}
>>>>> -
>>>>> -
>>>>> -		if (entry->base.bo)
>>>>> -			continue;
>>>>> -
>>>>> -		amdgpu_vm_bo_param(adev, vm, cursor.level, &bp);
>>>>> -
>>>>> -		r = amdgpu_bo_create(adev, &bp, &pt);
>>>>> -		if (r)
>>>>> -			return r;
>>>>> -
>>>>> -		if (vm->use_cpu_for_update) {
>>>>> -			r = amdgpu_bo_kmap(pt, NULL);
>>>>> -			if (r)
>>>>> -				goto error_free_pt;
>>>>> -		}
>>>>> +	if (entry->base.bo)
>>>>> +		return 0;
>>>>>       
>>>>> -		/* Keep a reference to the root directory to avoid
>>>>> -		* freeing them up in the wrong order.
>>>>> -		*/
>>>>> -		pt->parent = amdgpu_bo_ref(cursor.parent->base.bo);
>>>>> +	amdgpu_vm_bo_param(adev, vm, cursor->level, &bp);
>>>>>       
>>>>> -		amdgpu_vm_bo_base_init(&entry->base, vm, pt);
>>>>> +	r = amdgpu_bo_create(adev, &bp, &pt);
>>>>> +	if (r)
>>>>> +		return r;
>>>>>       
>>>>> -		r = amdgpu_vm_clear_bo(adev, vm, pt);
>>>>> +	if (vm->use_cpu_for_update) {
>>>>> +		r = amdgpu_bo_kmap(pt, NULL);
>>>>>       		if (r)
>>>>>       			goto error_free_pt;
>>>>>       	}
>>>>>       
>>>>> +	/* Keep a reference to the root directory to avoid
>>>>> +	 * freeing them up in the wrong order.
>>>>> +	 */
>>>>> +	pt->parent = amdgpu_bo_ref(cursor->parent->base.bo);
>>>>> +	amdgpu_vm_bo_base_init(&entry->base, vm, pt);
>>>>> +
>>>>> +	r = amdgpu_vm_clear_bo(adev, vm, pt);
>>>>> +	if (r)
>>>>> +		goto error_free_pt;
>>>>> +
>>>>>       	return 0;
>>>>>       
>>>>>       error_free_pt:
>>>>> @@ -1655,6 +1591,7 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>>>>>       	struct amdgpu_vm_pt_cursor cursor;
>>>>>       	uint64_t frag_start = start, frag_end;
>>>>>       	unsigned int frag;
>>>>> +	int r;
>>>>>       
>>>>>       	/* figure out the initial fragment */
>>>>>       	amdgpu_vm_fragment(params, frag_start, end, flags, &frag,
>>>>> &frag_end); @@ -1662,12 +1599,15 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>>>>>       	/* walk over the address space and update the PTs */
>>>>>       	amdgpu_vm_pt_start(adev, params->vm, start, &cursor);
>>>>>       	while (cursor.pfn < end) {
>>>>> -		struct amdgpu_bo *pt = cursor.entry->base.bo;
>>>>>       		unsigned shift, parent_shift, mask;
>>>>>       		uint64_t incr, entry_end, pe_start;
>>>>> +		struct amdgpu_bo *pt;
>>>>>       
>>>>> -		if (!pt)
>>>>> -			return -ENOENT;
>>>>> +		r = amdgpu_vm_alloc_pts(params->adev, params->vm, &cursor);
>>>>> +		if (r)
>>>>> +			return r;
>>>>> +
>>>>> +		pt = cursor.entry->base.bo;
>>>>>       
>>>>>       		/* The root level can't be a huge page */
>>>>>       		if (cursor.level == adev->vm_manager.root_level) { diff --git
>>>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>> index 81ff8177f092..116605c038d2 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>> @@ -303,9 +303,6 @@ bool amdgpu_vm_ready(struct amdgpu_vm *vm);
>>>>>       int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>>       			      int (*callback)(void *p, struct amdgpu_bo *bo),
>>>>>       			      void *param);
>>>>> -int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>>>>> -			struct amdgpu_vm *vm,
>>>>> -			uint64_t saddr, uint64_t size);
>>>>>       int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job, bool need_pipe_sync);
>>>>>       int amdgpu_vm_update_directories(struct amdgpu_device *adev,
>>>>>       				 struct amdgpu_vm *vm);
>>> _______________________________________________
>>> 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 6/8] drm/amdgpu: allocate VM PDs/PTs on demand

[Koenig, Christian]

Am 06.02.19 um 00:51 schrieb Kuehling, Felix:
> On 2019-02-05 11:00 a.m., Koenig, Christian wrote:
>> Ah! The initial clear of the PDs is syncing to everything!
> Right. Using amdgpu_sync_resv(... AMDGPU_FENCE_OWNER_VM ...) in
> amdgpu_vm_clear_bo seems to help. But if I also change the
> amdgpu_job_submit to use AMDGPU_FENCE_OWNER_VM, I get a VM fault and CP
> hang very early in my test. Not sure what's happening there. This is
> what I changed:
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 8b240f9..06c28ac 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -826,17 +826,18 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
>    
>           WARN_ON(job->ibs[0].length_dw > 64);
>           r = amdgpu_sync_resv(adev, &job->sync, bo->tbo.resv,
> -                            AMDGPU_FENCE_OWNER_UNDEFINED, false);
> +                            AMDGPU_FENCE_OWNER_VM, false);

That's correct.

>           if (r)
>                   goto error_free;
>    
> -       r = amdgpu_job_submit(job, &vm->entity, AMDGPU_FENCE_OWNER_UNDEFINED,
> +       r = amdgpu_job_submit(job, &vm->entity, AMDGPU_FENCE_OWNER_VM,

This change most likely doesn't make a difference.

>                                 &fence);
>           if (r)
>                   goto error_free;
>    
>           amdgpu_bo_fence(bo, fence, true);
> -       dma_fence_put(fence);
> +       dma_fence_put(vm->last_update);
> +       vm->last_update = fence;

This is most likely incorrect.

>    
>           if (bo->shadow)
>                   return amdgpu_vm_clear_bo(adev, vm, bo->shadow,
>
> Basically I tried to do the synchronization exactly like
> amdgpu_vm_update_directories.
>
> The only way this clear could cause a VM fault is, if a subsequent
> PTE/PDS update happens too early and gets overwritten by the pending
> clear. But don't the clear and the next PTE/PDE update go to the same
> queue (vm->entity) and are implicitly synchronized?

Need to check where this is coming from.

> Answer after another hour of pouring over code and history: No, sched
> entities are no longer equivalent to queues. The scheduler can itself
> load-balance VM updates using multiple SDMA queues. Sigh.

That is unproblematic. Load balancing happens only when the entity is idle.

E.g. submissions to the same entity never run in parallel.

> OK, so page table updates to different PTEs don't usually need to
> synchronize with each other. But how do page table updates to the same
> address get synchronized? How do you guarantee that two updates of the
> same PTE are processed by the hardware in the correct order, if
> AMDGPU_FENCE_OWNER_VM fences don't synchronize with each other?

Mhm, need to double check but that explanation doesn't seem to fit. 
There must be something else going on.

>
>
>> Ok, that is easy to fix.
> Not that easy. See above. ;)

Well need to take a closer look why you run into VM faults, but 
basically replacing the parameter in amdgpu_sync_resv should already do 
the trick.

Regards,
Christian.

>
> Regards,
>     Felix
>
>
>
>> Christian.
>>
>> Am 05.02.19 um 16:49 schrieb Kuehling, Felix:
>>> I saw a backtrace from the GPU scheduler when I was debugging this yesterday, but those backtraces never tell us where the command submission came from. It could be memory initialization, or some migration at buffer-validation. Basically, any command submission triggered by page table allocation that synchronizes with the PD reservation object is a problem.
>>>
>>> Regards,
>>>      Felix
>>>
>>> -----Original Message-----
>>> From: Christian König <ckoenig.leichtzumerken@gmail.com>
>>> Sent: Tuesday, February 05, 2019 10:40 AM
>>> To: Kuehling, Felix <Felix.Kuehling@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; amd-gfx@lists.freedesktop.org
>>> Subject: Re: [PATCH 6/8] drm/amdgpu: allocate VM PDs/PTs on demand
>>>
>>> Am 05.02.19 um 16:20 schrieb Kuehling, Felix:
>>>>>> This may cause regressions in KFD if PT BO allocation can trigger
>>>>>> eviction fences.
>>>>> I don't think that can happen, but need to double check as well.
>>>>>
>>>>> Otherwise allocating page tables could try to evict other page tables from the same process and that seriously doesn't make much sense.
>>>> Eviction fences don't cause actual memory evictions. They are the result of memory evictions. They cause KFD processes to be preempted in order to allow the fence to signal so memory can be evicted. The problem is that a page table allocation waits for the fences on the PD reservation, which looks like an eviction to KFD and triggers an unnecessary preemption. There is no actual memory eviction happening here.
>>> Yeah, but where is that actually coming from?
>>>
>>> It sounds like we still unnecessary synchronize page table updates somewhere.
>>>
>>> Do you have a call chain?
>>>
>>> Regards,
>>> Christian.
>>>
>>>> Regards,
>>>>       Felix
>>>>
>>>> -----Original Message-----
>>>> From: Christian König <ckoenig.leichtzumerken@gmail.com>
>>>> Sent: Tuesday, February 05, 2019 6:37 AM
>>>> To: Kuehling, Felix <Felix.Kuehling@amd.com>;
>>>> amd-gfx@lists.freedesktop.org
>>>> Subject: Re: [PATCH 6/8] drm/amdgpu: allocate VM PDs/PTs on demand
>>>>
>>>>> This may cause regressions in KFD if PT BO allocation can trigger
>>>>> eviction fences.
>>>> I don't think that can happen, but need to double check as well.
>>>>
>>>> Otherwise allocating page tables could try to evict other page tables from the same process and that seriously doesn't make much sense.
>>>>
>>>>> Do you know whether PT BO allocation would wait on the page-directory
>>>>> reservation object without the sync-object API anywhere?
>>>> For inside the kernel I can't think of any relevant use case, except for the eviction call path and there it is intentional.
>>>>
>>>> One thing which will always wait for all fences is the display code path, but that is not relevant here. (Isn't it?).
>>>>
>>>> What might be questionable is amdgpu_gem_wait_idle_ioctl(), but I think this is also completely intentional that we wait on everything here.
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>> Am 04.02.19 um 21:17 schrieb Kuehling, Felix:
>>>>> This may cause regressions in KFD if PT BO allocation can trigger
>>>>> eviction fences. The previous call to amdgpu_vm_alloc_pts was in a
>>>>> context where we had temporarily removed the eviction fence. PT BO
>>>>> allocation in amdgpu_vm_bo_update is not protected like that.
>>>>>
>>>>> I vaguely remember looking into this last time you were working on
>>>>> our eviction fence code and thinking that most of the cases where we
>>>>> remove the eviction fence were no longer needed if fence
>>>>> synchronization happens through the amdgpu_sync-object API (rather
>>>>> than waiting on a reservation object directly). I think it's time I
>>>>> go and finally clean that up.
>>>>>
>>>>> Do you know whether PT BO allocation would wait on the page-directory
>>>>> reservation object without the sync-object API anywhere?
>>>>>
>>>>> Regards,
>>>>>         Felix
>>>>>
>>>>> On 2019-02-04 7:42 a.m., Christian König wrote:
>>>>>> Let's start to allocate VM PDs/PTs on demand instead of
>>>>>> pre-allocating them during mapping.
>>>>>>
>>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>>> ---
>>>>>>        .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  |   6 -
>>>>>>        drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c       |   9 --
>>>>>>        drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c       |  10 --
>>>>>>        drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        | 136 +++++-------------
>>>>>>        drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |   3 -
>>>>>>        5 files changed, 38 insertions(+), 126 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>>>> index d7b10d79f1de..2176c92f9377 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>>>> @@ -492,12 +492,6 @@ static int add_bo_to_vm(struct amdgpu_device *adev, struct kgd_mem *mem,
>>>>>>        					vm->process_info->eviction_fence,
>>>>>>        					NULL, NULL);
>>>>>>        
>>>>>> -	ret = amdgpu_vm_alloc_pts(adev, vm, va, amdgpu_bo_size(bo));
>>>>>> -	if (ret) {
>>>>>> -		pr_err("Failed to allocate pts, err=%d\n", ret);
>>>>>> -		goto err_alloc_pts;
>>>>>> -	}
>>>>>> -
>>>>>>        	ret = vm_validate_pt_pd_bos(vm);
>>>>>>        	if (ret) {
>>>>>>        		pr_err("validate_pt_pd_bos() failed\n"); diff --git
>>>>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>>>>>> index 7e22be7ca68a..54dd02a898b9 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>>>>>> @@ -92,15 +92,6 @@ int amdgpu_map_static_csa(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>>>        		return -ENOMEM;
>>>>>>        	}
>>>>>>        
>>>>>> -	r = amdgpu_vm_alloc_pts(adev, (*bo_va)->base.vm, csa_addr,
>>>>>> -				size);
>>>>>> -	if (r) {
>>>>>> -		DRM_ERROR("failed to allocate pts for static CSA, err=%d\n", r);
>>>>>> -		amdgpu_vm_bo_rmv(adev, *bo_va);
>>>>>> -		ttm_eu_backoff_reservation(&ticket, &list);
>>>>>> -		return r;
>>>>>> -	}
>>>>>> -
>>>>>>        	r = amdgpu_vm_bo_map(adev, *bo_va, csa_addr, 0, size,
>>>>>>        			     AMDGPU_PTE_READABLE | AMDGPU_PTE_WRITEABLE |
>>>>>>        			     AMDGPU_PTE_EXECUTABLE);
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>>> index d21dd2f369da..e141e3b13112 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>>> @@ -627,11 +627,6 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev,
>>>>>> void *data,
>>>>>>        
>>>>>>        	switch (args->operation) {
>>>>>>        	case AMDGPU_VA_OP_MAP:
>>>>>> -		r = amdgpu_vm_alloc_pts(adev, bo_va->base.vm, args->va_address,
>>>>>> -					args->map_size);
>>>>>> -		if (r)
>>>>>> -			goto error_backoff;
>>>>>> -
>>>>>>        		va_flags = amdgpu_gmc_get_pte_flags(adev, args->flags);
>>>>>>        		r = amdgpu_vm_bo_map(adev, bo_va, args->va_address,
>>>>>>        				     args->offset_in_bo, args->map_size, @@ -647,11 +642,6
>>>>>> @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>>>>>>        						args->map_size);
>>>>>>        		break;
>>>>>>        	case AMDGPU_VA_OP_REPLACE:
>>>>>> -		r = amdgpu_vm_alloc_pts(adev, bo_va->base.vm, args->va_address,
>>>>>> -					args->map_size);
>>>>>> -		if (r)
>>>>>> -			goto error_backoff;
>>>>>> -
>>>>>>        		va_flags = amdgpu_gmc_get_pte_flags(adev, args->flags);
>>>>>>        		r = amdgpu_vm_bo_replace_map(adev, bo_va, args->va_address,
>>>>>>        					     args->offset_in_bo, args->map_size, diff --git
>>>>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>> index f7d410a5d848..0b8a1aa56c4a 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>> @@ -504,47 +504,6 @@ static void amdgpu_vm_pt_next(struct amdgpu_device *adev,
>>>>>>        	}
>>>>>>        }
>>>>>>        
>>>>>> -/**
>>>>>> - * amdgpu_vm_pt_first_leaf - get first leaf PD/PT
>>>>>> - *
>>>>>> - * @adev: amdgpu_device pointer
>>>>>> - * @vm: amdgpu_vm structure
>>>>>> - * @start: start addr of the walk
>>>>>> - * @cursor: state to initialize
>>>>>> - *
>>>>>> - * Start a walk and go directly to the leaf node.
>>>>>> - */
>>>>>> -static void amdgpu_vm_pt_first_leaf(struct amdgpu_device *adev,
>>>>>> -				    struct amdgpu_vm *vm, uint64_t start,
>>>>>> -				    struct amdgpu_vm_pt_cursor *cursor)
>>>>>> -{
>>>>>> -	amdgpu_vm_pt_start(adev, vm, start, cursor);
>>>>>> -	while (amdgpu_vm_pt_descendant(adev, cursor));
>>>>>> -}
>>>>>> -
>>>>>> -/**
>>>>>> - * amdgpu_vm_pt_next_leaf - get next leaf PD/PT
>>>>>> - *
>>>>>> - * @adev: amdgpu_device pointer
>>>>>> - * @cursor: current state
>>>>>> - *
>>>>>> - * Walk the PD/PT tree to the next leaf node.
>>>>>> - */
>>>>>> -static void amdgpu_vm_pt_next_leaf(struct amdgpu_device *adev,
>>>>>> -				   struct amdgpu_vm_pt_cursor *cursor)
>>>>>> -{
>>>>>> -	amdgpu_vm_pt_next(adev, cursor);
>>>>>> -	if (cursor->pfn != ~0ll)
>>>>>> -		while (amdgpu_vm_pt_descendant(adev, cursor));
>>>>>> -}
>>>>>> -
>>>>>> -/**
>>>>>> - * for_each_amdgpu_vm_pt_leaf - walk over all leaf PDs/PTs in the
>>>>>> hierarchy
>>>>>> - */
>>>>>> -#define for_each_amdgpu_vm_pt_leaf(adev, vm, start, end, cursor)		\
>>>>>> -	for (amdgpu_vm_pt_first_leaf((adev), (vm), (start), &(cursor));		\
>>>>>> -	     (cursor).pfn <= end; amdgpu_vm_pt_next_leaf((adev), &(cursor)))
>>>>>> -
>>>>>>        /**
>>>>>>         * amdgpu_vm_pt_first_dfs - start a deep first search
>>>>>>         *
>>>>>> @@ -919,74 +878,51 @@ static void amdgpu_vm_bo_param(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>>>         * Returns:
>>>>>>         * 0 on success, errno otherwise.
>>>>>>         */
>>>>>> -int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>>>>>> -			struct amdgpu_vm *vm,
>>>>>> -			uint64_t saddr, uint64_t size)
>>>>>> +static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>>>>>> +			       struct amdgpu_vm *vm,
>>>>>> +			       struct amdgpu_vm_pt_cursor *cursor)
>>>>>>        {
>>>>>> -	struct amdgpu_vm_pt_cursor cursor;
>>>>>> +	struct amdgpu_vm_pt *entry = cursor->entry;
>>>>>> +	struct amdgpu_bo_param bp;
>>>>>>        	struct amdgpu_bo *pt;
>>>>>> -	uint64_t eaddr;
>>>>>>        	int r;
>>>>>>        
>>>>>> -	/* validate the parameters */
>>>>>> -	if (saddr & AMDGPU_GPU_PAGE_MASK || size & AMDGPU_GPU_PAGE_MASK)
>>>>>> -		return -EINVAL;
>>>>>> +	if (cursor->level < AMDGPU_VM_PTB && !entry->entries) {
>>>>>> +		unsigned num_entries;
>>>>>>        
>>>>>> -	eaddr = saddr + size - 1;
>>>>>> -
>>>>>> -	saddr /= AMDGPU_GPU_PAGE_SIZE;
>>>>>> -	eaddr /= AMDGPU_GPU_PAGE_SIZE;
>>>>>> -
>>>>>> -	if (eaddr >= adev->vm_manager.max_pfn) {
>>>>>> -		dev_err(adev->dev, "va above limit (0x%08llX >= 0x%08llX)\n",
>>>>>> -			eaddr, adev->vm_manager.max_pfn);
>>>>>> -		return -EINVAL;
>>>>>> +		num_entries = amdgpu_vm_num_entries(adev, cursor->level);
>>>>>> +		entry->entries = kvmalloc_array(num_entries,
>>>>>> +						sizeof(*entry->entries),
>>>>>> +						GFP_KERNEL | __GFP_ZERO);
>>>>>> +		if (!entry->entries)
>>>>>> +			return -ENOMEM;
>>>>>>        	}
>>>>>>        
>>>>>> -	for_each_amdgpu_vm_pt_leaf(adev, vm, saddr, eaddr, cursor) {
>>>>>> -		struct amdgpu_vm_pt *entry = cursor.entry;
>>>>>> -		struct amdgpu_bo_param bp;
>>>>>> -
>>>>>> -		if (cursor.level < AMDGPU_VM_PTB) {
>>>>>> -			unsigned num_entries;
>>>>>> -
>>>>>> -			num_entries = amdgpu_vm_num_entries(adev, cursor.level);
>>>>>> -			entry->entries = kvmalloc_array(num_entries,
>>>>>> -							sizeof(*entry->entries),
>>>>>> -							GFP_KERNEL |
>>>>>> -							__GFP_ZERO);
>>>>>> -			if (!entry->entries)
>>>>>> -				return -ENOMEM;
>>>>>> -		}
>>>>>> -
>>>>>> -
>>>>>> -		if (entry->base.bo)
>>>>>> -			continue;
>>>>>> -
>>>>>> -		amdgpu_vm_bo_param(adev, vm, cursor.level, &bp);
>>>>>> -
>>>>>> -		r = amdgpu_bo_create(adev, &bp, &pt);
>>>>>> -		if (r)
>>>>>> -			return r;
>>>>>> -
>>>>>> -		if (vm->use_cpu_for_update) {
>>>>>> -			r = amdgpu_bo_kmap(pt, NULL);
>>>>>> -			if (r)
>>>>>> -				goto error_free_pt;
>>>>>> -		}
>>>>>> +	if (entry->base.bo)
>>>>>> +		return 0;
>>>>>>        
>>>>>> -		/* Keep a reference to the root directory to avoid
>>>>>> -		* freeing them up in the wrong order.
>>>>>> -		*/
>>>>>> -		pt->parent = amdgpu_bo_ref(cursor.parent->base.bo);
>>>>>> +	amdgpu_vm_bo_param(adev, vm, cursor->level, &bp);
>>>>>>        
>>>>>> -		amdgpu_vm_bo_base_init(&entry->base, vm, pt);
>>>>>> +	r = amdgpu_bo_create(adev, &bp, &pt);
>>>>>> +	if (r)
>>>>>> +		return r;
>>>>>>        
>>>>>> -		r = amdgpu_vm_clear_bo(adev, vm, pt);
>>>>>> +	if (vm->use_cpu_for_update) {
>>>>>> +		r = amdgpu_bo_kmap(pt, NULL);
>>>>>>        		if (r)
>>>>>>        			goto error_free_pt;
>>>>>>        	}
>>>>>>        
>>>>>> +	/* Keep a reference to the root directory to avoid
>>>>>> +	 * freeing them up in the wrong order.
>>>>>> +	 */
>>>>>> +	pt->parent = amdgpu_bo_ref(cursor->parent->base.bo);
>>>>>> +	amdgpu_vm_bo_base_init(&entry->base, vm, pt);
>>>>>> +
>>>>>> +	r = amdgpu_vm_clear_bo(adev, vm, pt);
>>>>>> +	if (r)
>>>>>> +		goto error_free_pt;
>>>>>> +
>>>>>>        	return 0;
>>>>>>        
>>>>>>        error_free_pt:
>>>>>> @@ -1655,6 +1591,7 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>>>>>>        	struct amdgpu_vm_pt_cursor cursor;
>>>>>>        	uint64_t frag_start = start, frag_end;
>>>>>>        	unsigned int frag;
>>>>>> +	int r;
>>>>>>        
>>>>>>        	/* figure out the initial fragment */
>>>>>>        	amdgpu_vm_fragment(params, frag_start, end, flags, &frag,
>>>>>> &frag_end); @@ -1662,12 +1599,15 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>>>>>>        	/* walk over the address space and update the PTs */
>>>>>>        	amdgpu_vm_pt_start(adev, params->vm, start, &cursor);
>>>>>>        	while (cursor.pfn < end) {
>>>>>> -		struct amdgpu_bo *pt = cursor.entry->base.bo;
>>>>>>        		unsigned shift, parent_shift, mask;
>>>>>>        		uint64_t incr, entry_end, pe_start;
>>>>>> +		struct amdgpu_bo *pt;
>>>>>>        
>>>>>> -		if (!pt)
>>>>>> -			return -ENOENT;
>>>>>> +		r = amdgpu_vm_alloc_pts(params->adev, params->vm, &cursor);
>>>>>> +		if (r)
>>>>>> +			return r;
>>>>>> +
>>>>>> +		pt = cursor.entry->base.bo;
>>>>>>        
>>>>>>        		/* The root level can't be a huge page */
>>>>>>        		if (cursor.level == adev->vm_manager.root_level) { diff --git
>>>>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>>> index 81ff8177f092..116605c038d2 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>>> @@ -303,9 +303,6 @@ bool amdgpu_vm_ready(struct amdgpu_vm *vm);
>>>>>>        int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>>>        			      int (*callback)(void *p, struct amdgpu_bo *bo),
>>>>>>        			      void *param);
>>>>>> -int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>>>>>> -			struct amdgpu_vm *vm,
>>>>>> -			uint64_t saddr, uint64_t size);
>>>>>>        int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job, bool need_pipe_sync);
>>>>>>        int amdgpu_vm_update_directories(struct amdgpu_device *adev,
>>>>>>        				 struct amdgpu_vm *vm);
>>>> _______________________________________________
>>>> 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 6/8] drm/amdgpu: allocate VM PDs/PTs on demand

[Kuehling, Felix]

I mostly figured out what was going on. See my comments inline below.

I'll send out a patch series in a minute that cleans up and simplifies 
the eviction fence handling so we no longer need to remove eviction 
fences temporarily.

On 2019-02-06 4:06 a.m., Koenig, Christian wrote:
> Am 06.02.19 um 00:51 schrieb Kuehling, Felix:
>> On 2019-02-05 11:00 a.m., Koenig, Christian wrote:
>>> Ah! The initial clear of the PDs is syncing to everything!
>> Right. Using amdgpu_sync_resv(... AMDGPU_FENCE_OWNER_VM ...) in
>> amdgpu_vm_clear_bo seems to help. But if I also change the
>> amdgpu_job_submit to use AMDGPU_FENCE_OWNER_VM, I get a VM fault and CP
>> hang very early in my test. Not sure what's happening there. This is
>> what I changed:
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 8b240f9..06c28ac 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -826,17 +826,18 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
>>     
>>            WARN_ON(job->ibs[0].length_dw > 64);
>>            r = amdgpu_sync_resv(adev, &job->sync, bo->tbo.resv,
>> -                            AMDGPU_FENCE_OWNER_UNDEFINED, false);
>> +                            AMDGPU_FENCE_OWNER_VM, false);
> That's correct.

I changed this to FENCE_OWNER_KFD in my patch series. So it will still 
synchronize with everything, except eviction fences. The same treatment 
was needed for the synchronization when clearing page table entries in 
amdgpu_vm_bo_update_mapping.


>
>>            if (r)
>>                    goto error_free;
>>     
>> -       r = amdgpu_job_submit(job, &vm->entity, AMDGPU_FENCE_OWNER_UNDEFINED,
>> +       r = amdgpu_job_submit(job, &vm->entity, AMDGPU_FENCE_OWNER_VM,
> This change most likely doesn't make a difference.

It ended up affecting synchronization with subsequent CPU page table 
updates. See below.


>
>>                                  &fence);
>>            if (r)
>>                    goto error_free;
>>     
>>            amdgpu_bo_fence(bo, fence, true);
>> -       dma_fence_put(fence);
>> +       dma_fence_put(vm->last_update);
>> +       vm->last_update = fence;
> This is most likely incorrect.

This wasn't needed. I was hoping it would fix the problem, but didn't.


>
>>     
>>            if (bo->shadow)
>>                    return amdgpu_vm_clear_bo(adev, vm, bo->shadow,
>>
>> Basically I tried to do the synchronization exactly like
>> amdgpu_vm_update_directories.
>>
>> The only way this clear could cause a VM fault is, if a subsequent
>> PTE/PDS update happens too early and gets overwritten by the pending
>> clear. But don't the clear and the next PTE/PDE update go to the same
>> queue (vm->entity) and are implicitly synchronized?
> Need to check where this is coming from.
>
>> Answer after another hour of pouring over code and history: No, sched
>> entities are no longer equivalent to queues. The scheduler can itself
>> load-balance VM updates using multiple SDMA queues. Sigh.
> That is unproblematic. Load balancing happens only when the entity is idle.
>
> E.g. submissions to the same entity never run in parallel.

It was because of CPU page table updates that failed to synchronize with 
the vm_clear_bo when I changed the fence owner for this submission. Damn 
resizable BAR. ;) I wasn't expecting this on my small-BAR Fiji and ended 
up chasing my tail for a while.

Regards,
   Felix


>
>> OK, so page table updates to different PTEs don't usually need to
>> synchronize with each other. But how do page table updates to the same
>> address get synchronized? How do you guarantee that two updates of the
>> same PTE are processed by the hardware in the correct order, if
>> AMDGPU_FENCE_OWNER_VM fences don't synchronize with each other?
> Mhm, need to double check but that explanation doesn't seem to fit.
> There must be something else going on.
>
>>
>>> Ok, that is easy to fix.
>> Not that easy. See above. ;)
> Well need to take a closer look why you run into VM faults, but
> basically replacing the parameter in amdgpu_sync_resv should already do
> the trick.
>
> Regards,
> Christian.
>
>> Regards,
>>      Felix
>>
>>
>>
>>> Christian.
>>>
>>> Am 05.02.19 um 16:49 schrieb Kuehling, Felix:
>>>> I saw a backtrace from the GPU scheduler when I was debugging this yesterday, but those backtraces never tell us where the command submission came from. It could be memory initialization, or some migration at buffer-validation. Basically, any command submission triggered by page table allocation that synchronizes with the PD reservation object is a problem.
>>>>
>>>> Regards,
>>>>       Felix
>>>>
>>>> -----Original Message-----
>>>> From: Christian König <ckoenig.leichtzumerken@gmail.com>
>>>> Sent: Tuesday, February 05, 2019 10:40 AM
>>>> To: Kuehling, Felix <Felix.Kuehling@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; amd-gfx@lists.freedesktop.org
>>>> Subject: Re: [PATCH 6/8] drm/amdgpu: allocate VM PDs/PTs on demand
>>>>
>>>> Am 05.02.19 um 16:20 schrieb Kuehling, Felix:
>>>>>>> This may cause regressions in KFD if PT BO allocation can trigger
>>>>>>> eviction fences.
>>>>>> I don't think that can happen, but need to double check as well.
>>>>>>
>>>>>> Otherwise allocating page tables could try to evict other page tables from the same process and that seriously doesn't make much sense.
>>>>> Eviction fences don't cause actual memory evictions. They are the result of memory evictions. They cause KFD processes to be preempted in order to allow the fence to signal so memory can be evicted. The problem is that a page table allocation waits for the fences on the PD reservation, which looks like an eviction to KFD and triggers an unnecessary preemption. There is no actual memory eviction happening here.
>>>> Yeah, but where is that actually coming from?
>>>>
>>>> It sounds like we still unnecessary synchronize page table updates somewhere.
>>>>
>>>> Do you have a call chain?
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> Regards,
>>>>>        Felix
>>>>>
>>>>> -----Original Message-----
>>>>> From: Christian König <ckoenig.leichtzumerken@gmail.com>
>>>>> Sent: Tuesday, February 05, 2019 6:37 AM
>>>>> To: Kuehling, Felix <Felix.Kuehling@amd.com>;
>>>>> amd-gfx@lists.freedesktop.org
>>>>> Subject: Re: [PATCH 6/8] drm/amdgpu: allocate VM PDs/PTs on demand
>>>>>
>>>>>> This may cause regressions in KFD if PT BO allocation can trigger
>>>>>> eviction fences.
>>>>> I don't think that can happen, but need to double check as well.
>>>>>
>>>>> Otherwise allocating page tables could try to evict other page tables from the same process and that seriously doesn't make much sense.
>>>>>
>>>>>> Do you know whether PT BO allocation would wait on the page-directory
>>>>>> reservation object without the sync-object API anywhere?
>>>>> For inside the kernel I can't think of any relevant use case, except for the eviction call path and there it is intentional.
>>>>>
>>>>> One thing which will always wait for all fences is the display code path, but that is not relevant here. (Isn't it?).
>>>>>
>>>>> What might be questionable is amdgpu_gem_wait_idle_ioctl(), but I think this is also completely intentional that we wait on everything here.
>>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>> Am 04.02.19 um 21:17 schrieb Kuehling, Felix:
>>>>>> This may cause regressions in KFD if PT BO allocation can trigger
>>>>>> eviction fences. The previous call to amdgpu_vm_alloc_pts was in a
>>>>>> context where we had temporarily removed the eviction fence. PT BO
>>>>>> allocation in amdgpu_vm_bo_update is not protected like that.
>>>>>>
>>>>>> I vaguely remember looking into this last time you were working on
>>>>>> our eviction fence code and thinking that most of the cases where we
>>>>>> remove the eviction fence were no longer needed if fence
>>>>>> synchronization happens through the amdgpu_sync-object API (rather
>>>>>> than waiting on a reservation object directly). I think it's time I
>>>>>> go and finally clean that up.
>>>>>>
>>>>>> Do you know whether PT BO allocation would wait on the page-directory
>>>>>> reservation object without the sync-object API anywhere?
>>>>>>
>>>>>> Regards,
>>>>>>          Felix
>>>>>>
>>>>>> On 2019-02-04 7:42 a.m., Christian König wrote:
>>>>>>> Let's start to allocate VM PDs/PTs on demand instead of
>>>>>>> pre-allocating them during mapping.
>>>>>>>
>>>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>>>> ---
>>>>>>>         .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  |   6 -
>>>>>>>         drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c       |   9 --
>>>>>>>         drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c       |  10 --
>>>>>>>         drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        | 136 +++++-------------
>>>>>>>         drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |   3 -
>>>>>>>         5 files changed, 38 insertions(+), 126 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>>>>> index d7b10d79f1de..2176c92f9377 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>>>>> @@ -492,12 +492,6 @@ static int add_bo_to_vm(struct amdgpu_device *adev, struct kgd_mem *mem,
>>>>>>>         					vm->process_info->eviction_fence,
>>>>>>>         					NULL, NULL);
>>>>>>>         
>>>>>>> -	ret = amdgpu_vm_alloc_pts(adev, vm, va, amdgpu_bo_size(bo));
>>>>>>> -	if (ret) {
>>>>>>> -		pr_err("Failed to allocate pts, err=%d\n", ret);
>>>>>>> -		goto err_alloc_pts;
>>>>>>> -	}
>>>>>>> -
>>>>>>>         	ret = vm_validate_pt_pd_bos(vm);
>>>>>>>         	if (ret) {
>>>>>>>         		pr_err("validate_pt_pd_bos() failed\n"); diff --git
>>>>>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>>>>>>> index 7e22be7ca68a..54dd02a898b9 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>>>>>>> @@ -92,15 +92,6 @@ int amdgpu_map_static_csa(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>>>>         		return -ENOMEM;
>>>>>>>         	}
>>>>>>>         
>>>>>>> -	r = amdgpu_vm_alloc_pts(adev, (*bo_va)->base.vm, csa_addr,
>>>>>>> -				size);
>>>>>>> -	if (r) {
>>>>>>> -		DRM_ERROR("failed to allocate pts for static CSA, err=%d\n", r);
>>>>>>> -		amdgpu_vm_bo_rmv(adev, *bo_va);
>>>>>>> -		ttm_eu_backoff_reservation(&ticket, &list);
>>>>>>> -		return r;
>>>>>>> -	}
>>>>>>> -
>>>>>>>         	r = amdgpu_vm_bo_map(adev, *bo_va, csa_addr, 0, size,
>>>>>>>         			     AMDGPU_PTE_READABLE | AMDGPU_PTE_WRITEABLE |
>>>>>>>         			     AMDGPU_PTE_EXECUTABLE);
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>>>> index d21dd2f369da..e141e3b13112 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>>>> @@ -627,11 +627,6 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev,
>>>>>>> void *data,
>>>>>>>         
>>>>>>>         	switch (args->operation) {
>>>>>>>         	case AMDGPU_VA_OP_MAP:
>>>>>>> -		r = amdgpu_vm_alloc_pts(adev, bo_va->base.vm, args->va_address,
>>>>>>> -					args->map_size);
>>>>>>> -		if (r)
>>>>>>> -			goto error_backoff;
>>>>>>> -
>>>>>>>         		va_flags = amdgpu_gmc_get_pte_flags(adev, args->flags);
>>>>>>>         		r = amdgpu_vm_bo_map(adev, bo_va, args->va_address,
>>>>>>>         				     args->offset_in_bo, args->map_size, @@ -647,11 +642,6
>>>>>>> @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>>>>>>>         						args->map_size);
>>>>>>>         		break;
>>>>>>>         	case AMDGPU_VA_OP_REPLACE:
>>>>>>> -		r = amdgpu_vm_alloc_pts(adev, bo_va->base.vm, args->va_address,
>>>>>>> -					args->map_size);
>>>>>>> -		if (r)
>>>>>>> -			goto error_backoff;
>>>>>>> -
>>>>>>>         		va_flags = amdgpu_gmc_get_pte_flags(adev, args->flags);
>>>>>>>         		r = amdgpu_vm_bo_replace_map(adev, bo_va, args->va_address,
>>>>>>>         					     args->offset_in_bo, args->map_size, diff --git
>>>>>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>>> index f7d410a5d848..0b8a1aa56c4a 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>>> @@ -504,47 +504,6 @@ static void amdgpu_vm_pt_next(struct amdgpu_device *adev,
>>>>>>>         	}
>>>>>>>         }
>>>>>>>         
>>>>>>> -/**
>>>>>>> - * amdgpu_vm_pt_first_leaf - get first leaf PD/PT
>>>>>>> - *
>>>>>>> - * @adev: amdgpu_device pointer
>>>>>>> - * @vm: amdgpu_vm structure
>>>>>>> - * @start: start addr of the walk
>>>>>>> - * @cursor: state to initialize
>>>>>>> - *
>>>>>>> - * Start a walk and go directly to the leaf node.
>>>>>>> - */
>>>>>>> -static void amdgpu_vm_pt_first_leaf(struct amdgpu_device *adev,
>>>>>>> -				    struct amdgpu_vm *vm, uint64_t start,
>>>>>>> -				    struct amdgpu_vm_pt_cursor *cursor)
>>>>>>> -{
>>>>>>> -	amdgpu_vm_pt_start(adev, vm, start, cursor);
>>>>>>> -	while (amdgpu_vm_pt_descendant(adev, cursor));
>>>>>>> -}
>>>>>>> -
>>>>>>> -/**
>>>>>>> - * amdgpu_vm_pt_next_leaf - get next leaf PD/PT
>>>>>>> - *
>>>>>>> - * @adev: amdgpu_device pointer
>>>>>>> - * @cursor: current state
>>>>>>> - *
>>>>>>> - * Walk the PD/PT tree to the next leaf node.
>>>>>>> - */
>>>>>>> -static void amdgpu_vm_pt_next_leaf(struct amdgpu_device *adev,
>>>>>>> -				   struct amdgpu_vm_pt_cursor *cursor)
>>>>>>> -{
>>>>>>> -	amdgpu_vm_pt_next(adev, cursor);
>>>>>>> -	if (cursor->pfn != ~0ll)
>>>>>>> -		while (amdgpu_vm_pt_descendant(adev, cursor));
>>>>>>> -}
>>>>>>> -
>>>>>>> -/**
>>>>>>> - * for_each_amdgpu_vm_pt_leaf - walk over all leaf PDs/PTs in the
>>>>>>> hierarchy
>>>>>>> - */
>>>>>>> -#define for_each_amdgpu_vm_pt_leaf(adev, vm, start, end, cursor)		\
>>>>>>> -	for (amdgpu_vm_pt_first_leaf((adev), (vm), (start), &(cursor));		\
>>>>>>> -	     (cursor).pfn <= end; amdgpu_vm_pt_next_leaf((adev), &(cursor)))
>>>>>>> -
>>>>>>>         /**
>>>>>>>          * amdgpu_vm_pt_first_dfs - start a deep first search
>>>>>>>          *
>>>>>>> @@ -919,74 +878,51 @@ static void amdgpu_vm_bo_param(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>>>>          * Returns:
>>>>>>>          * 0 on success, errno otherwise.
>>>>>>>          */
>>>>>>> -int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>>>>>>> -			struct amdgpu_vm *vm,
>>>>>>> -			uint64_t saddr, uint64_t size)
>>>>>>> +static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>>>>>>> +			       struct amdgpu_vm *vm,
>>>>>>> +			       struct amdgpu_vm_pt_cursor *cursor)
>>>>>>>         {
>>>>>>> -	struct amdgpu_vm_pt_cursor cursor;
>>>>>>> +	struct amdgpu_vm_pt *entry = cursor->entry;
>>>>>>> +	struct amdgpu_bo_param bp;
>>>>>>>         	struct amdgpu_bo *pt;
>>>>>>> -	uint64_t eaddr;
>>>>>>>         	int r;
>>>>>>>         
>>>>>>> -	/* validate the parameters */
>>>>>>> -	if (saddr & AMDGPU_GPU_PAGE_MASK || size & AMDGPU_GPU_PAGE_MASK)
>>>>>>> -		return -EINVAL;
>>>>>>> +	if (cursor->level < AMDGPU_VM_PTB && !entry->entries) {
>>>>>>> +		unsigned num_entries;
>>>>>>>         
>>>>>>> -	eaddr = saddr + size - 1;
>>>>>>> -
>>>>>>> -	saddr /= AMDGPU_GPU_PAGE_SIZE;
>>>>>>> -	eaddr /= AMDGPU_GPU_PAGE_SIZE;
>>>>>>> -
>>>>>>> -	if (eaddr >= adev->vm_manager.max_pfn) {
>>>>>>> -		dev_err(adev->dev, "va above limit (0x%08llX >= 0x%08llX)\n",
>>>>>>> -			eaddr, adev->vm_manager.max_pfn);
>>>>>>> -		return -EINVAL;
>>>>>>> +		num_entries = amdgpu_vm_num_entries(adev, cursor->level);
>>>>>>> +		entry->entries = kvmalloc_array(num_entries,
>>>>>>> +						sizeof(*entry->entries),
>>>>>>> +						GFP_KERNEL | __GFP_ZERO);
>>>>>>> +		if (!entry->entries)
>>>>>>> +			return -ENOMEM;
>>>>>>>         	}
>>>>>>>         
>>>>>>> -	for_each_amdgpu_vm_pt_leaf(adev, vm, saddr, eaddr, cursor) {
>>>>>>> -		struct amdgpu_vm_pt *entry = cursor.entry;
>>>>>>> -		struct amdgpu_bo_param bp;
>>>>>>> -
>>>>>>> -		if (cursor.level < AMDGPU_VM_PTB) {
>>>>>>> -			unsigned num_entries;
>>>>>>> -
>>>>>>> -			num_entries = amdgpu_vm_num_entries(adev, cursor.level);
>>>>>>> -			entry->entries = kvmalloc_array(num_entries,
>>>>>>> -							sizeof(*entry->entries),
>>>>>>> -							GFP_KERNEL |
>>>>>>> -							__GFP_ZERO);
>>>>>>> -			if (!entry->entries)
>>>>>>> -				return -ENOMEM;
>>>>>>> -		}
>>>>>>> -
>>>>>>> -
>>>>>>> -		if (entry->base.bo)
>>>>>>> -			continue;
>>>>>>> -
>>>>>>> -		amdgpu_vm_bo_param(adev, vm, cursor.level, &bp);
>>>>>>> -
>>>>>>> -		r = amdgpu_bo_create(adev, &bp, &pt);
>>>>>>> -		if (r)
>>>>>>> -			return r;
>>>>>>> -
>>>>>>> -		if (vm->use_cpu_for_update) {
>>>>>>> -			r = amdgpu_bo_kmap(pt, NULL);
>>>>>>> -			if (r)
>>>>>>> -				goto error_free_pt;
>>>>>>> -		}
>>>>>>> +	if (entry->base.bo)
>>>>>>> +		return 0;
>>>>>>>         
>>>>>>> -		/* Keep a reference to the root directory to avoid
>>>>>>> -		* freeing them up in the wrong order.
>>>>>>> -		*/
>>>>>>> -		pt->parent = amdgpu_bo_ref(cursor.parent->base.bo);
>>>>>>> +	amdgpu_vm_bo_param(adev, vm, cursor->level, &bp);
>>>>>>>         
>>>>>>> -		amdgpu_vm_bo_base_init(&entry->base, vm, pt);
>>>>>>> +	r = amdgpu_bo_create(adev, &bp, &pt);
>>>>>>> +	if (r)
>>>>>>> +		return r;
>>>>>>>         
>>>>>>> -		r = amdgpu_vm_clear_bo(adev, vm, pt);
>>>>>>> +	if (vm->use_cpu_for_update) {
>>>>>>> +		r = amdgpu_bo_kmap(pt, NULL);
>>>>>>>         		if (r)
>>>>>>>         			goto error_free_pt;
>>>>>>>         	}
>>>>>>>         
>>>>>>> +	/* Keep a reference to the root directory to avoid
>>>>>>> +	 * freeing them up in the wrong order.
>>>>>>> +	 */
>>>>>>> +	pt->parent = amdgpu_bo_ref(cursor->parent->base.bo);
>>>>>>> +	amdgpu_vm_bo_base_init(&entry->base, vm, pt);
>>>>>>> +
>>>>>>> +	r = amdgpu_vm_clear_bo(adev, vm, pt);
>>>>>>> +	if (r)
>>>>>>> +		goto error_free_pt;
>>>>>>> +
>>>>>>>         	return 0;
>>>>>>>         
>>>>>>>         error_free_pt:
>>>>>>> @@ -1655,6 +1591,7 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>>>>>>>         	struct amdgpu_vm_pt_cursor cursor;
>>>>>>>         	uint64_t frag_start = start, frag_end;
>>>>>>>         	unsigned int frag;
>>>>>>> +	int r;
>>>>>>>         
>>>>>>>         	/* figure out the initial fragment */
>>>>>>>         	amdgpu_vm_fragment(params, frag_start, end, flags, &frag,
>>>>>>> &frag_end); @@ -1662,12 +1599,15 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>>>>>>>         	/* walk over the address space and update the PTs */
>>>>>>>         	amdgpu_vm_pt_start(adev, params->vm, start, &cursor);
>>>>>>>         	while (cursor.pfn < end) {
>>>>>>> -		struct amdgpu_bo *pt = cursor.entry->base.bo;
>>>>>>>         		unsigned shift, parent_shift, mask;
>>>>>>>         		uint64_t incr, entry_end, pe_start;
>>>>>>> +		struct amdgpu_bo *pt;
>>>>>>>         
>>>>>>> -		if (!pt)
>>>>>>> -			return -ENOENT;
>>>>>>> +		r = amdgpu_vm_alloc_pts(params->adev, params->vm, &cursor);
>>>>>>> +		if (r)
>>>>>>> +			return r;
>>>>>>> +
>>>>>>> +		pt = cursor.entry->base.bo;
>>>>>>>         
>>>>>>>         		/* The root level can't be a huge page */
>>>>>>>         		if (cursor.level == adev->vm_manager.root_level) { diff --git
>>>>>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>>>> index 81ff8177f092..116605c038d2 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>>>> @@ -303,9 +303,6 @@ bool amdgpu_vm_ready(struct amdgpu_vm *vm);
>>>>>>>         int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>>>>         			      int (*callback)(void *p, struct amdgpu_bo *bo),
>>>>>>>         			      void *param);
>>>>>>> -int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>>>>>>> -			struct amdgpu_vm *vm,
>>>>>>> -			uint64_t saddr, uint64_t size);
>>>>>>>         int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job, bool need_pipe_sync);
>>>>>>>         int amdgpu_vm_update_directories(struct amdgpu_device *adev,
>>>>>>>         				 struct amdgpu_vm *vm);
>>>>> _______________________________________________
>>>>> 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 6/8] drm/amdgpu: allocate VM PDs/PTs on demand

[Kuehling, Felix]

I pushed my patch series that simplifies eviction fence handling in KFD. 
If you rebase this, it should be OK now.

Regards,
   Felix

On 2019-02-04 7:42 a.m., Christian König wrote:
> Let's start to allocate VM PDs/PTs on demand instead of pre-allocating
> them during mapping.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  |   6 -
>   drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c       |   9 --
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c       |  10 --
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        | 136 +++++-------------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |   3 -
>   5 files changed, 38 insertions(+), 126 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index d7b10d79f1de..2176c92f9377 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -492,12 +492,6 @@ static int add_bo_to_vm(struct amdgpu_device *adev, struct kgd_mem *mem,
>   					vm->process_info->eviction_fence,
>   					NULL, NULL);
>   
> -	ret = amdgpu_vm_alloc_pts(adev, vm, va, amdgpu_bo_size(bo));
> -	if (ret) {
> -		pr_err("Failed to allocate pts, err=%d\n", ret);
> -		goto err_alloc_pts;
> -	}
> -
>   	ret = vm_validate_pt_pd_bos(vm);
>   	if (ret) {
>   		pr_err("validate_pt_pd_bos() failed\n");
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
> index 7e22be7ca68a..54dd02a898b9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
> @@ -92,15 +92,6 @@ int amdgpu_map_static_csa(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   		return -ENOMEM;
>   	}
>   
> -	r = amdgpu_vm_alloc_pts(adev, (*bo_va)->base.vm, csa_addr,
> -				size);
> -	if (r) {
> -		DRM_ERROR("failed to allocate pts for static CSA, err=%d\n", r);
> -		amdgpu_vm_bo_rmv(adev, *bo_va);
> -		ttm_eu_backoff_reservation(&ticket, &list);
> -		return r;
> -	}
> -
>   	r = amdgpu_vm_bo_map(adev, *bo_va, csa_addr, 0, size,
>   			     AMDGPU_PTE_READABLE | AMDGPU_PTE_WRITEABLE |
>   			     AMDGPU_PTE_EXECUTABLE);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index d21dd2f369da..e141e3b13112 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -627,11 +627,6 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>   
>   	switch (args->operation) {
>   	case AMDGPU_VA_OP_MAP:
> -		r = amdgpu_vm_alloc_pts(adev, bo_va->base.vm, args->va_address,
> -					args->map_size);
> -		if (r)
> -			goto error_backoff;
> -
>   		va_flags = amdgpu_gmc_get_pte_flags(adev, args->flags);
>   		r = amdgpu_vm_bo_map(adev, bo_va, args->va_address,
>   				     args->offset_in_bo, args->map_size,
> @@ -647,11 +642,6 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>   						args->map_size);
>   		break;
>   	case AMDGPU_VA_OP_REPLACE:
> -		r = amdgpu_vm_alloc_pts(adev, bo_va->base.vm, args->va_address,
> -					args->map_size);
> -		if (r)
> -			goto error_backoff;
> -
>   		va_flags = amdgpu_gmc_get_pte_flags(adev, args->flags);
>   		r = amdgpu_vm_bo_replace_map(adev, bo_va, args->va_address,
>   					     args->offset_in_bo, args->map_size,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index f7d410a5d848..0b8a1aa56c4a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -504,47 +504,6 @@ static void amdgpu_vm_pt_next(struct amdgpu_device *adev,
>   	}
>   }
>   
> -/**
> - * amdgpu_vm_pt_first_leaf - get first leaf PD/PT
> - *
> - * @adev: amdgpu_device pointer
> - * @vm: amdgpu_vm structure
> - * @start: start addr of the walk
> - * @cursor: state to initialize
> - *
> - * Start a walk and go directly to the leaf node.
> - */
> -static void amdgpu_vm_pt_first_leaf(struct amdgpu_device *adev,
> -				    struct amdgpu_vm *vm, uint64_t start,
> -				    struct amdgpu_vm_pt_cursor *cursor)
> -{
> -	amdgpu_vm_pt_start(adev, vm, start, cursor);
> -	while (amdgpu_vm_pt_descendant(adev, cursor));
> -}
> -
> -/**
> - * amdgpu_vm_pt_next_leaf - get next leaf PD/PT
> - *
> - * @adev: amdgpu_device pointer
> - * @cursor: current state
> - *
> - * Walk the PD/PT tree to the next leaf node.
> - */
> -static void amdgpu_vm_pt_next_leaf(struct amdgpu_device *adev,
> -				   struct amdgpu_vm_pt_cursor *cursor)
> -{
> -	amdgpu_vm_pt_next(adev, cursor);
> -	if (cursor->pfn != ~0ll)
> -		while (amdgpu_vm_pt_descendant(adev, cursor));
> -}
> -
> -/**
> - * for_each_amdgpu_vm_pt_leaf - walk over all leaf PDs/PTs in the hierarchy
> - */
> -#define for_each_amdgpu_vm_pt_leaf(adev, vm, start, end, cursor)		\
> -	for (amdgpu_vm_pt_first_leaf((adev), (vm), (start), &(cursor));		\
> -	     (cursor).pfn <= end; amdgpu_vm_pt_next_leaf((adev), &(cursor)))
> -
>   /**
>    * amdgpu_vm_pt_first_dfs - start a deep first search
>    *
> @@ -919,74 +878,51 @@ static void amdgpu_vm_bo_param(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>    * Returns:
>    * 0 on success, errno otherwise.
>    */
> -int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
> -			struct amdgpu_vm *vm,
> -			uint64_t saddr, uint64_t size)
> +static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
> +			       struct amdgpu_vm *vm,
> +			       struct amdgpu_vm_pt_cursor *cursor)
>   {
> -	struct amdgpu_vm_pt_cursor cursor;
> +	struct amdgpu_vm_pt *entry = cursor->entry;
> +	struct amdgpu_bo_param bp;
>   	struct amdgpu_bo *pt;
> -	uint64_t eaddr;
>   	int r;
>   
> -	/* validate the parameters */
> -	if (saddr & AMDGPU_GPU_PAGE_MASK || size & AMDGPU_GPU_PAGE_MASK)
> -		return -EINVAL;
> +	if (cursor->level < AMDGPU_VM_PTB && !entry->entries) {
> +		unsigned num_entries;
>   
> -	eaddr = saddr + size - 1;
> -
> -	saddr /= AMDGPU_GPU_PAGE_SIZE;
> -	eaddr /= AMDGPU_GPU_PAGE_SIZE;
> -
> -	if (eaddr >= adev->vm_manager.max_pfn) {
> -		dev_err(adev->dev, "va above limit (0x%08llX >= 0x%08llX)\n",
> -			eaddr, adev->vm_manager.max_pfn);
> -		return -EINVAL;
> +		num_entries = amdgpu_vm_num_entries(adev, cursor->level);
> +		entry->entries = kvmalloc_array(num_entries,
> +						sizeof(*entry->entries),
> +						GFP_KERNEL | __GFP_ZERO);
> +		if (!entry->entries)
> +			return -ENOMEM;
>   	}
>   
> -	for_each_amdgpu_vm_pt_leaf(adev, vm, saddr, eaddr, cursor) {
> -		struct amdgpu_vm_pt *entry = cursor.entry;
> -		struct amdgpu_bo_param bp;
> -
> -		if (cursor.level < AMDGPU_VM_PTB) {
> -			unsigned num_entries;
> -
> -			num_entries = amdgpu_vm_num_entries(adev, cursor.level);
> -			entry->entries = kvmalloc_array(num_entries,
> -							sizeof(*entry->entries),
> -							GFP_KERNEL |
> -							__GFP_ZERO);
> -			if (!entry->entries)
> -				return -ENOMEM;
> -		}
> -
> -
> -		if (entry->base.bo)
> -			continue;
> -
> -		amdgpu_vm_bo_param(adev, vm, cursor.level, &bp);
> -
> -		r = amdgpu_bo_create(adev, &bp, &pt);
> -		if (r)
> -			return r;
> -
> -		if (vm->use_cpu_for_update) {
> -			r = amdgpu_bo_kmap(pt, NULL);
> -			if (r)
> -				goto error_free_pt;
> -		}
> +	if (entry->base.bo)
> +		return 0;
>   
> -		/* Keep a reference to the root directory to avoid
> -		* freeing them up in the wrong order.
> -		*/
> -		pt->parent = amdgpu_bo_ref(cursor.parent->base.bo);
> +	amdgpu_vm_bo_param(adev, vm, cursor->level, &bp);
>   
> -		amdgpu_vm_bo_base_init(&entry->base, vm, pt);
> +	r = amdgpu_bo_create(adev, &bp, &pt);
> +	if (r)
> +		return r;
>   
> -		r = amdgpu_vm_clear_bo(adev, vm, pt);
> +	if (vm->use_cpu_for_update) {
> +		r = amdgpu_bo_kmap(pt, NULL);
>   		if (r)
>   			goto error_free_pt;
>   	}
>   
> +	/* Keep a reference to the root directory to avoid
> +	 * freeing them up in the wrong order.
> +	 */
> +	pt->parent = amdgpu_bo_ref(cursor->parent->base.bo);
> +	amdgpu_vm_bo_base_init(&entry->base, vm, pt);
> +
> +	r = amdgpu_vm_clear_bo(adev, vm, pt);
> +	if (r)
> +		goto error_free_pt;
> +
>   	return 0;
>   
>   error_free_pt:
> @@ -1655,6 +1591,7 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>   	struct amdgpu_vm_pt_cursor cursor;
>   	uint64_t frag_start = start, frag_end;
>   	unsigned int frag;
> +	int r;
>   
>   	/* figure out the initial fragment */
>   	amdgpu_vm_fragment(params, frag_start, end, flags, &frag, &frag_end);
> @@ -1662,12 +1599,15 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>   	/* walk over the address space and update the PTs */
>   	amdgpu_vm_pt_start(adev, params->vm, start, &cursor);
>   	while (cursor.pfn < end) {
> -		struct amdgpu_bo *pt = cursor.entry->base.bo;
>   		unsigned shift, parent_shift, mask;
>   		uint64_t incr, entry_end, pe_start;
> +		struct amdgpu_bo *pt;
>   
> -		if (!pt)
> -			return -ENOENT;
> +		r = amdgpu_vm_alloc_pts(params->adev, params->vm, &cursor);
> +		if (r)
> +			return r;
> +
> +		pt = cursor.entry->base.bo;
>   
>   		/* The root level can't be a huge page */
>   		if (cursor.level == adev->vm_manager.root_level) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 81ff8177f092..116605c038d2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -303,9 +303,6 @@ bool amdgpu_vm_ready(struct amdgpu_vm *vm);
>   int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   			      int (*callback)(void *p, struct amdgpu_bo *bo),
>   			      void *param);
> -int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
> -			struct amdgpu_vm *vm,
> -			uint64_t saddr, uint64_t size);
>   int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job, bool need_pipe_sync);
>   int amdgpu_vm_update_directories(struct amdgpu_device *adev,
>   				 struct amdgpu_vm *vm);
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 7/8] drm/amdgpu: free PDs/PTs on demand

[Christian König]

When something is unmapped we now free the affected PDs/PTs again.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 0b8a1aa56c4a..a3dd2b397299 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -515,12 +515,31 @@ static void amdgpu_vm_pt_next(struct amdgpu_device *adev,
  */
 static void amdgpu_vm_pt_first_dfs(struct amdgpu_device *adev,
 				   struct amdgpu_vm *vm,
+				   struct amdgpu_vm_pt_cursor *start,
 				   struct amdgpu_vm_pt_cursor *cursor)
 {
-	amdgpu_vm_pt_start(adev, vm, 0, cursor);
+	if (start)
+		*cursor = *start;
+	else
+		amdgpu_vm_pt_start(adev, vm, 0, cursor);
 	while (amdgpu_vm_pt_descendant(adev, cursor));
 }
 
+/**
+ * amdgpu_vm_pt_continue_dfs - check if the deep first search should continue
+ *
+ * @start: starting point for the search
+ * @entry: current entry
+ *
+ * Returns:
+ * True when the search should continue, false otherwise.
+ */
+static bool amdgpu_vm_pt_continue_dfs(struct amdgpu_vm_pt_cursor *start,
+				      struct amdgpu_vm_pt *entry)
+{
+	return entry && (!start || entry != start->entry);
+}
+
 /**
  * amdgpu_vm_pt_next_dfs - get the next node for a deep first search
  *
@@ -546,11 +565,11 @@ static void amdgpu_vm_pt_next_dfs(struct amdgpu_device *adev,
 /**
  * for_each_amdgpu_vm_pt_dfs_safe - safe deep first search of all PDs/PTs
  */
-#define for_each_amdgpu_vm_pt_dfs_safe(adev, vm, cursor, entry)			\
-	for (amdgpu_vm_pt_first_dfs((adev), (vm), &(cursor)),			\
+#define for_each_amdgpu_vm_pt_dfs_safe(adev, vm, start, cursor, entry)		\
+	for (amdgpu_vm_pt_first_dfs((adev), (vm), (start), &(cursor)),		\
 	     (entry) = (cursor).entry, amdgpu_vm_pt_next_dfs((adev), &(cursor));\
-	     (entry); (entry) = (cursor).entry,					\
-	     amdgpu_vm_pt_next_dfs((adev), &(cursor)))
+	     amdgpu_vm_pt_continue_dfs((start), (entry));			\
+	     (entry) = (cursor).entry, amdgpu_vm_pt_next_dfs((adev), &(cursor)))
 
 /**
  * amdgpu_vm_get_pd_bo - add the VM PD to a validation list
@@ -931,32 +950,46 @@ static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
 	return r;
 }
 
+/**
+ * amdgpu_vm_free_table - fre one PD/PT
+ *
+ * @entry: PDE to free
+ */
+static void amdgpu_vm_free_table(struct amdgpu_vm_pt *entry)
+{
+	if (entry->base.bo) {
+		entry->base.bo->vm_bo = NULL;
+		list_del(&entry->base.vm_status);
+		amdgpu_bo_unref(&entry->base.bo->shadow);
+		amdgpu_bo_unref(&entry->base.bo);
+	}
+	kvfree(entry->entries);
+	entry->entries = NULL;
+}
+
 /**
  * amdgpu_vm_free_pts - free PD/PT levels
  *
  * @adev: amdgpu device structure
  * @vm: amdgpu vm structure
+ * @start: optional cursor where to start freeing PDs/PTs
  *
  * Free the page directory or page table level and all sub levels.
  */
 static void amdgpu_vm_free_pts(struct amdgpu_device *adev,
-			       struct amdgpu_vm *vm)
+			       struct amdgpu_vm *vm,
+			       struct amdgpu_vm_pt_cursor *start)
 {
 	struct amdgpu_vm_pt_cursor cursor;
 	struct amdgpu_vm_pt *entry;
 
-	for_each_amdgpu_vm_pt_dfs_safe(adev, vm, cursor, entry) {
+	vm->bulk_moveable = false;
 
-		if (entry->base.bo) {
-			entry->base.bo->vm_bo = NULL;
-			list_del(&entry->base.vm_status);
-			amdgpu_bo_unref(&entry->base.bo->shadow);
-			amdgpu_bo_unref(&entry->base.bo);
-		}
-		kvfree(entry->entries);
-	}
+	for_each_amdgpu_vm_pt_dfs_safe(adev, vm, start, cursor, entry)
+		amdgpu_vm_free_table(entry);
 
-	BUG_ON(vm->root.base.bo);
+	if (start)
+		amdgpu_vm_free_table(start->entry);
 }
 
 /**
@@ -1377,7 +1410,7 @@ static void amdgpu_vm_invalidate_pds(struct amdgpu_device *adev,
 	struct amdgpu_vm_pt_cursor cursor;
 	struct amdgpu_vm_pt *entry;
 
-	for_each_amdgpu_vm_pt_dfs_safe(adev, vm, cursor, entry)
+	for_each_amdgpu_vm_pt_dfs_safe(adev, vm, NULL, cursor, entry)
 		if (entry->base.bo && !entry->base.moved)
 			amdgpu_vm_bo_relocated(&entry->base);
 }
@@ -1684,6 +1717,7 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
 			/* Mark all child entries as huge */
 			while (cursor.pfn < frag_start) {
 				cursor.entry->huge = true;
+				amdgpu_vm_free_pts(adev, params->vm, &cursor);
 				amdgpu_vm_pt_next(adev, &cursor);
 			}
 
@@ -3245,10 +3279,11 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 	if (r) {
 		dev_err(adev->dev, "Leaking page tables because BO reservation failed\n");
 	} else {
-		amdgpu_vm_free_pts(adev, vm);
+		amdgpu_vm_free_pts(adev, vm, NULL);
 		amdgpu_bo_unreserve(root);
 	}
 	amdgpu_bo_unref(&root);
+	WARN_ON(vm->root.base.bo);
 	dma_fence_put(vm->last_update);
 	for (i = 0; i < AMDGPU_MAX_VMHUBS; i++)
 		amdgpu_vmid_free_reserved(adev, vm, i);
-- 
2.17.1

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

[PATCH 8/8] drm/amdgpu: drop the huge page flag

[Christian König]

Not needed any more since we now free PDs/PTs on demand.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index a3dd2b397299..8f4fd8a7fb96 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1383,10 +1383,6 @@ static void amdgpu_vm_update_pde(struct amdgpu_pte_update_params *params,
 	uint64_t pde, pt, flags;
 	unsigned level;
 
-	/* Don't update huge pages here */
-	if (entry->huge)
-		return;
-
 	for (level = 0, pbo = bo->parent; pbo; ++level)
 		pbo = pbo->parent;
 
@@ -1649,13 +1645,6 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
 			continue;
 		}
 
-		/* If it isn't already handled it can't be a huge page */
-		if (cursor.entry->huge) {
-			/* Add the entry to the relocated list to update it. */
-			cursor.entry->huge = false;
-			amdgpu_vm_bo_relocated(&cursor.entry->base);
-		}
-
 		shift = amdgpu_vm_level_shift(adev, cursor.level);
 		parent_shift = amdgpu_vm_level_shift(adev, cursor.level - 1);
 		if (adev->asic_type < CHIP_VEGA10) {
@@ -1714,9 +1703,8 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
 		} while (frag_start < entry_end);
 
 		if (amdgpu_vm_pt_descendant(adev, &cursor)) {
-			/* Mark all child entries as huge */
+			/* Free all child entries */
 			while (cursor.pfn < frag_start) {
-				cursor.entry->huge = true;
 				amdgpu_vm_free_pts(adev, params->vm, &cursor);
 				amdgpu_vm_pt_next(adev, &cursor);
 			}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 116605c038d2..3c6537ef659c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -140,7 +140,6 @@ struct amdgpu_vm_bo_base {
 
 struct amdgpu_vm_pt {
 	struct amdgpu_vm_bo_base	base;
-	bool				huge;
 
 	/* array of page tables, one for each directory entry */
 	struct amdgpu_vm_pt		*entries;
-- 
2.17.1

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

Re: [PATCH 1/8] drm/amdgpu: fix waiting for BO moves with CPU based PD/PT updates

[Kuehling, Felix]

On 2019-02-04 7:42 a.m., Christian König wrote:
> Otherwise we open up the possibility to use uninitialized memory.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>

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


> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index c70621db3bb7..44950f3b8801 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1779,13 +1779,18 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
>   		if (pages_addr)
>   			params.src = ~0;
>   
> -		/* Wait for PT BOs to be free. PTs share the same resv. object
> +		/* Wait for PT BOs to be idle. PTs share the same resv. object
>   		 * as the root PD BO
>   		 */
>   		r = amdgpu_vm_wait_pd(adev, vm, owner);
>   		if (unlikely(r))
>   			return r;
>   
> +		/* Wait for any BO move to be completed */
> +		r = dma_fence_wait(exclusive, true);
> +		if (unlikely(r))
> +			return r;
> +
>   		params.func = amdgpu_vm_cpu_set_ptes;
>   		params.pages_addr = pages_addr;
>   		return amdgpu_vm_update_ptes(&params, start, last + 1,
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx