[PATCH 1/5] drm/amdgpu: expose amdgpu_bo_create_shadow()

[PATCH 1/5] drm/amdgpu: expose amdgpu_bo_create_shadow()

[Nirmoy Das]

Exposed amdgpu_bo_create_shadow() will be needed
for amdgpu_vm handling.

Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 6 +++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 3 +++
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 1345f7eba011..9cdeb20fb6cd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -625,9 +625,9 @@ static int amdgpu_bo_do_create(struct amdgpu_device *adev,
 	return r;
 }
 
-static int amdgpu_bo_create_shadow(struct amdgpu_device *adev,
-				   unsigned long size,
-				   struct amdgpu_bo *bo)
+int amdgpu_bo_create_shadow(struct amdgpu_device *adev,
+			    unsigned long size,
+			    struct amdgpu_bo *bo)
 {
 	struct amdgpu_bo_param bp;
 	int r;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
index 973c88bdf37b..e0ec48d6a3fd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
@@ -271,6 +271,9 @@ int amdgpu_bo_create_user(struct amdgpu_device *adev,
 			  struct amdgpu_bo_user **ubo_ptr);
 void amdgpu_bo_free_kernel(struct amdgpu_bo **bo, u64 *gpu_addr,
 			   void **cpu_addr);
+int amdgpu_bo_create_shadow(struct amdgpu_device *adev,
+			    unsigned long size,
+			    struct amdgpu_bo *bo);
 int amdgpu_bo_kmap(struct amdgpu_bo *bo, void **ptr);
 void *amdgpu_bo_kptr(struct amdgpu_bo *bo);
 void amdgpu_bo_kunmap(struct amdgpu_bo *bo);
-- 
2.31.1

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

[PATCH 2/5] drm/amdgpu: initialize vm->is_compute_context properly

[Nirmoy Das]

Fix vm->is_compute_context initialization in amdgpu_vm_init().

Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index f95bcda8463f..6f0a6011cb3d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2815,9 +2815,9 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 		goto error_free_immediate;
 
 	vm->pte_support_ats = false;
-	vm->is_compute_context = false;
+	vm->is_compute_context = vm_context == AMDGPU_VM_CONTEXT_COMPUTE;
 
-	if (vm_context == AMDGPU_VM_CONTEXT_COMPUTE) {
+	if (vm->is_compute_context) {
 		vm->use_cpu_for_update = !!(adev->vm_manager.vm_update_mode &
 						AMDGPU_VM_USE_CPU_FOR_COMPUTE);
 
@@ -2844,7 +2844,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 	vm->evicting = false;
 
 	amdgpu_vm_bo_param(adev, vm, adev->vm_manager.root_level, false, &bp);
-	if (vm_context == AMDGPU_VM_CONTEXT_COMPUTE)
+	if (vm->is_compute_context)
 		bp.flags &= ~AMDGPU_GEM_CREATE_SHADOW;
 	r = amdgpu_bo_create(adev, &bp, &root);
 	if (r)
-- 
2.31.1

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

[PATCH 3/5] drm/amdgpu: create shadow bo using amdgpu_bo_create_shadow()

[Nirmoy Das]

Shadow BOs are only needed for vm code so call amdgpu_bo_create_shadow()
directly instead of depending on amdgpu_bo_create().

Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 75 +++++++++++++++++---------
 1 file changed, 49 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 6f0a6011cb3d..0e1d08a88f54 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -850,35 +850,64 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
 }
 
 /**
- * amdgpu_vm_bo_param - fill in parameters for PD/PT allocation
+ * amdgpu_vm_bo_create - create bo for PD/PT
  *
  * @adev: amdgpu_device pointer
  * @vm: requesting vm
  * @level: the page table level
  * @immediate: use a immediate update
- * @bp: resulting BO allocation parameters
+ * @bo: pointer to the buffer object pointer
  */
-static void amdgpu_vm_bo_param(struct amdgpu_device *adev, struct amdgpu_vm *vm,
-			       int level, bool immediate,
-			       struct amdgpu_bo_param *bp)
+static int amdgpu_vm_bo_create(struct amdgpu_device *adev,
+				struct amdgpu_vm *vm,
+				int level, bool immediate,
+				struct amdgpu_bo **bo)
 {
-	memset(bp, 0, sizeof(*bp));
+	struct amdgpu_bo_param bp;
+	bool create_shadow = false;
+	int r;
 
-	bp->size = amdgpu_vm_bo_size(adev, level);
-	bp->byte_align = AMDGPU_GPU_PAGE_SIZE;
-	bp->domain = AMDGPU_GEM_DOMAIN_VRAM;
-	bp->domain = amdgpu_bo_get_preferred_pin_domain(adev, bp->domain);
-	bp->flags = AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS |
+	memset(&bp, 0, sizeof(bp));
+
+	bp.size = amdgpu_vm_bo_size(adev, level);
+	bp.byte_align = AMDGPU_GPU_PAGE_SIZE;
+	bp.domain = AMDGPU_GEM_DOMAIN_VRAM;
+	bp.domain = amdgpu_bo_get_preferred_pin_domain(adev, bp.domain);
+	bp.flags = AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS |
 		AMDGPU_GEM_CREATE_CPU_GTT_USWC;
-	bp->bo_ptr_size = sizeof(struct amdgpu_bo);
+	bp.bo_ptr_size = sizeof(struct amdgpu_bo);
 	if (vm->use_cpu_for_update)
-		bp->flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
+		bp.flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
 	else if (!vm->root.base.bo || vm->root.base.bo->shadow)
-		bp->flags |= AMDGPU_GEM_CREATE_SHADOW;
-	bp->type = ttm_bo_type_kernel;
-	bp->no_wait_gpu = immediate;
+		create_shadow = true;
+
+	bp.type = ttm_bo_type_kernel;
+	bp.no_wait_gpu = immediate;
 	if (vm->root.base.bo)
-		bp->resv = vm->root.base.bo->tbo.base.resv;
+		bp.resv = vm->root.base.bo->tbo.base.resv;
+
+
+	r = amdgpu_bo_create(adev, &bp, bo);
+	if (r)
+		return r;
+	if (!vm->is_compute_context &&
+	    !(adev->flags & AMD_IS_APU) &&
+	    create_shadow) {
+		if (!bp.resv)
+			WARN_ON(dma_resv_lock((*bo)->tbo.base.resv,
+					      NULL));
+		r = amdgpu_bo_create_shadow(adev, bp.size, *bo);
+
+		if (!bp.resv)
+			dma_resv_unlock((*bo)->tbo.base.resv);
+
+		if (r) {
+			amdgpu_bo_unref(bo);
+			return r;
+		}
+	}
+
+	return 0;
 }
 
 /**
@@ -901,7 +930,6 @@ static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
 			       bool immediate)
 {
 	struct amdgpu_vm_pt *entry = cursor->entry;
-	struct amdgpu_bo_param bp;
 	struct amdgpu_bo *pt;
 	int r;
 
@@ -919,9 +947,7 @@ static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
 	if (entry->base.bo)
 		return 0;
 
-	amdgpu_vm_bo_param(adev, vm, cursor->level, immediate, &bp);
-
-	r = amdgpu_bo_create(adev, &bp, &pt);
+	r = amdgpu_vm_bo_create(adev, vm, cursor->level, immediate, &pt);
 	if (r)
 		return r;
 
@@ -2785,7 +2811,6 @@ long amdgpu_vm_wait_idle(struct amdgpu_vm *vm, long timeout)
 int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 		   int vm_context, u32 pasid)
 {
-	struct amdgpu_bo_param bp;
 	struct amdgpu_bo *root;
 	int r, i;
 
@@ -2843,10 +2868,8 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 	mutex_init(&vm->eviction_lock);
 	vm->evicting = false;
 
-	amdgpu_vm_bo_param(adev, vm, adev->vm_manager.root_level, false, &bp);
-	if (vm->is_compute_context)
-		bp.flags &= ~AMDGPU_GEM_CREATE_SHADOW;
-	r = amdgpu_bo_create(adev, &bp, &root);
+	r = amdgpu_vm_bo_create(adev, vm, adev->vm_manager.root_level,
+				false, &root);
 	if (r)
 		goto error_free_delayed;
 
-- 
2.31.1

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

[PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_create()

[Nirmoy Das]

Remove shadow bo related code as vm code is creating shadow bo
using proper API.

Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 30 ++--------------------
 1 file changed, 2 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 9cdeb20fb6cd..4256cbfec5eb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -661,10 +661,7 @@ int amdgpu_bo_create_shadow(struct amdgpu_device *adev,
  * @bp: parameters to be used for the buffer object
  * @bo_ptr: pointer to the buffer object pointer
  *
- * Creates an &amdgpu_bo buffer object; and if requested, also creates a
- * shadow object.
- * Shadow object is used to backup the original buffer object, and is always
- * in GTT.
+ * Creates an &amdgpu_bo buffer object.
  *
  * Returns:
  * 0 for success or a negative error code on failure.
@@ -673,30 +670,7 @@ int amdgpu_bo_create(struct amdgpu_device *adev,
 		     struct amdgpu_bo_param *bp,
 		     struct amdgpu_bo **bo_ptr)
 {
-	u64 flags = bp->flags;
-	int r;
-
-	bp->flags = bp->flags & ~AMDGPU_GEM_CREATE_SHADOW;
-
-	r = amdgpu_bo_do_create(adev, bp, bo_ptr);
-	if (r)
-		return r;
-
-	if ((flags & AMDGPU_GEM_CREATE_SHADOW) && !(adev->flags & AMD_IS_APU)) {
-		if (!bp->resv)
-			WARN_ON(dma_resv_lock((*bo_ptr)->tbo.base.resv,
-							NULL));
-
-		r = amdgpu_bo_create_shadow(adev, bp->size, *bo_ptr);
-
-		if (!bp->resv)
-			dma_resv_unlock((*bo_ptr)->tbo.base.resv);
-
-		if (r)
-			amdgpu_bo_unref(bo_ptr);
-	}
-
-	return r;
+	return  amdgpu_bo_do_create(adev, bp, bo_ptr);
 }
 
 /**
-- 
2.31.1

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

[PATCH 5/5] drm/amdgpu: remove AMDGPU_GEM_CREATE_SHADOW flag

[Nirmoy Das]

Remove unused AMDGPU_GEM_CREATE_SHADOW flag.

Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 5 +----
 include/uapi/drm/amdgpu_drm.h              | 2 --
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 4256cbfec5eb..2d9bd0d4f81c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -638,8 +638,7 @@ int amdgpu_bo_create_shadow(struct amdgpu_device *adev,
 	memset(&bp, 0, sizeof(bp));
 	bp.size = size;
 	bp.domain = AMDGPU_GEM_DOMAIN_GTT;
-	bp.flags = AMDGPU_GEM_CREATE_CPU_GTT_USWC |
-		AMDGPU_GEM_CREATE_SHADOW;
+	bp.flags = AMDGPU_GEM_CREATE_CPU_GTT_USWC;
 	bp.type = ttm_bo_type_kernel;
 	bp.resv = bo->tbo.base.resv;
 	bp.bo_ptr_size = sizeof(struct amdgpu_bo);
@@ -692,7 +691,6 @@ int amdgpu_bo_create_user(struct amdgpu_device *adev,
 	struct amdgpu_bo *bo_ptr;
 	int r;
 
-	bp->flags = bp->flags & ~AMDGPU_GEM_CREATE_SHADOW;
 	bp->bo_ptr_size = sizeof(struct amdgpu_bo_user);
 	r = amdgpu_bo_do_create(adev, bp, &bo_ptr);
 	if (r)
@@ -1566,7 +1564,6 @@ u64 amdgpu_bo_print_info(int id, struct amdgpu_bo *bo, struct seq_file *m)
 	amdgpu_bo_print_flag(m, bo, NO_CPU_ACCESS);
 	amdgpu_bo_print_flag(m, bo, CPU_GTT_USWC);
 	amdgpu_bo_print_flag(m, bo, VRAM_CLEARED);
-	amdgpu_bo_print_flag(m, bo, SHADOW);
 	amdgpu_bo_print_flag(m, bo, VRAM_CONTIGUOUS);
 	amdgpu_bo_print_flag(m, bo, VM_ALWAYS_VALID);
 	amdgpu_bo_print_flag(m, bo, EXPLICIT_SYNC);
diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
index 8b832f7458f2..9169df7fadee 100644
--- a/include/uapi/drm/amdgpu_drm.h
+++ b/include/uapi/drm/amdgpu_drm.h
@@ -119,8 +119,6 @@ extern "C" {
 #define AMDGPU_GEM_CREATE_CPU_GTT_USWC		(1 << 2)
 /* Flag that the memory should be in VRAM and cleared */
 #define AMDGPU_GEM_CREATE_VRAM_CLEARED		(1 << 3)
-/* Flag that create shadow bo(GTT) while allocating vram bo */
-#define AMDGPU_GEM_CREATE_SHADOW		(1 << 4)
 /* Flag that allocating the BO should use linear VRAM */
 #define AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS	(1 << 5)
 /* Flag that BO is always valid in this VM */
-- 
2.31.1

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

Re: [PATCH 3/5] drm/amdgpu: create shadow bo using amdgpu_bo_create_shadow()

[Christian König]

Am 22.04.21 um 14:35 schrieb Nirmoy Das:
> Shadow BOs are only needed for vm code so call amdgpu_bo_create_shadow()
> directly instead of depending on amdgpu_bo_create().
>
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 75 +++++++++++++++++---------
>   1 file changed, 49 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 6f0a6011cb3d..0e1d08a88f54 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -850,35 +850,64 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
>   }
>   
>   /**
> - * amdgpu_vm_bo_param - fill in parameters for PD/PT allocation
> + * amdgpu_vm_bo_create - create bo for PD/PT

Better name that amdgpu_vm_pt_create.

>    *
>    * @adev: amdgpu_device pointer
>    * @vm: requesting vm
>    * @level: the page table level
>    * @immediate: use a immediate update
> - * @bp: resulting BO allocation parameters
> + * @bo: pointer to the buffer object pointer
>    */
> -static void amdgpu_vm_bo_param(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> -			       int level, bool immediate,
> -			       struct amdgpu_bo_param *bp)
> +static int amdgpu_vm_bo_create(struct amdgpu_device *adev,
> +				struct amdgpu_vm *vm,
> +				int level, bool immediate,
> +				struct amdgpu_bo **bo)
>   {
> -	memset(bp, 0, sizeof(*bp));
> +	struct amdgpu_bo_param bp;
> +	bool create_shadow = false;
> +	int r;
>   
> -	bp->size = amdgpu_vm_bo_size(adev, level);
> -	bp->byte_align = AMDGPU_GPU_PAGE_SIZE;
> -	bp->domain = AMDGPU_GEM_DOMAIN_VRAM;
> -	bp->domain = amdgpu_bo_get_preferred_pin_domain(adev, bp->domain);
> -	bp->flags = AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS |
> +	memset(&bp, 0, sizeof(bp));
> +
> +	bp.size = amdgpu_vm_bo_size(adev, level);
> +	bp.byte_align = AMDGPU_GPU_PAGE_SIZE;
> +	bp.domain = AMDGPU_GEM_DOMAIN_VRAM;
> +	bp.domain = amdgpu_bo_get_preferred_pin_domain(adev, bp.domain);
> +	bp.flags = AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS |
>   		AMDGPU_GEM_CREATE_CPU_GTT_USWC;
> -	bp->bo_ptr_size = sizeof(struct amdgpu_bo);
> +	bp.bo_ptr_size = sizeof(struct amdgpu_bo);
>   	if (vm->use_cpu_for_update)
> -		bp->flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
> +		bp.flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
>   	else if (!vm->root.base.bo || vm->root.base.bo->shadow)
> -		bp->flags |= AMDGPU_GEM_CREATE_SHADOW;
> -	bp->type = ttm_bo_type_kernel;
> -	bp->no_wait_gpu = immediate;
> +		create_shadow = true;
> +
> +	bp.type = ttm_bo_type_kernel;
> +	bp.no_wait_gpu = immediate;
>   	if (vm->root.base.bo)
> -		bp->resv = vm->root.base.bo->tbo.base.resv;
> +		bp.resv = vm->root.base.bo->tbo.base.resv;
> +
> +
> +	r = amdgpu_bo_create(adev, &bp, bo);
> +	if (r)
> +		return r;
> +	if (!vm->is_compute_context &&
> +	    !(adev->flags & AMD_IS_APU) &&
> +	    create_shadow) {

Better drop the create_show flag and just always check it like this:

if (vm->is_compute_context || adev->flags & AMD_IS_APU)
     return 0;

Apart from that looks good to me.

Christian.

> +		if (!bp.resv)
> +			WARN_ON(dma_resv_lock((*bo)->tbo.base.resv,
> +					      NULL));
> +		r = amdgpu_bo_create_shadow(adev, bp.size, *bo);
> +
> +		if (!bp.resv)
> +			dma_resv_unlock((*bo)->tbo.base.resv);
> +
> +		if (r) {
> +			amdgpu_bo_unref(bo);
> +			return r;
> +		}
> +	}
> +
> +	return 0;
>   }
>   
>   /**
> @@ -901,7 +930,6 @@ static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>   			       bool immediate)
>   {
>   	struct amdgpu_vm_pt *entry = cursor->entry;
> -	struct amdgpu_bo_param bp;
>   	struct amdgpu_bo *pt;
>   	int r;
>   
> @@ -919,9 +947,7 @@ static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>   	if (entry->base.bo)
>   		return 0;
>   
> -	amdgpu_vm_bo_param(adev, vm, cursor->level, immediate, &bp);
> -
> -	r = amdgpu_bo_create(adev, &bp, &pt);
> +	r = amdgpu_vm_bo_create(adev, vm, cursor->level, immediate, &pt);
>   	if (r)
>   		return r;
>   
> @@ -2785,7 +2811,6 @@ long amdgpu_vm_wait_idle(struct amdgpu_vm *vm, long timeout)
>   int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   		   int vm_context, u32 pasid)
>   {
> -	struct amdgpu_bo_param bp;
>   	struct amdgpu_bo *root;
>   	int r, i;
>   
> @@ -2843,10 +2868,8 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   	mutex_init(&vm->eviction_lock);
>   	vm->evicting = false;
>   
> -	amdgpu_vm_bo_param(adev, vm, adev->vm_manager.root_level, false, &bp);
> -	if (vm->is_compute_context)
> -		bp.flags &= ~AMDGPU_GEM_CREATE_SHADOW;
> -	r = amdgpu_bo_create(adev, &bp, &root);
> +	r = amdgpu_vm_bo_create(adev, vm, adev->vm_manager.root_level,
> +				false, &root);
>   	if (r)
>   		goto error_free_delayed;
>   

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

Re: [PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_create()

[Christian König]

Am 22.04.21 um 14:35 schrieb Nirmoy Das:
> Remove shadow bo related code as vm code is creating shadow bo
> using proper API.
>
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 30 ++--------------------
>   1 file changed, 2 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 9cdeb20fb6cd..4256cbfec5eb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -661,10 +661,7 @@ int amdgpu_bo_create_shadow(struct amdgpu_device *adev,
>    * @bp: parameters to be used for the buffer object
>    * @bo_ptr: pointer to the buffer object pointer
>    *
> - * Creates an &amdgpu_bo buffer object; and if requested, also creates a
> - * shadow object.
> - * Shadow object is used to backup the original buffer object, and is always
> - * in GTT.
> + * Creates an &amdgpu_bo buffer object.
>    *
>    * Returns:
>    * 0 for success or a negative error code on failure.
> @@ -673,30 +670,7 @@ int amdgpu_bo_create(struct amdgpu_device *adev,
>   		     struct amdgpu_bo_param *bp,
>   		     struct amdgpu_bo **bo_ptr)
>   {
> -	u64 flags = bp->flags;
> -	int r;
> -
> -	bp->flags = bp->flags & ~AMDGPU_GEM_CREATE_SHADOW;
> -
> -	r = amdgpu_bo_do_create(adev, bp, bo_ptr);
> -	if (r)
> -		return r;
> -
> -	if ((flags & AMDGPU_GEM_CREATE_SHADOW) && !(adev->flags & AMD_IS_APU)) {
> -		if (!bp->resv)
> -			WARN_ON(dma_resv_lock((*bo_ptr)->tbo.base.resv,
> -							NULL));
> -
> -		r = amdgpu_bo_create_shadow(adev, bp->size, *bo_ptr);
> -
> -		if (!bp->resv)
> -			dma_resv_unlock((*bo_ptr)->tbo.base.resv);
> -
> -		if (r)
> -			amdgpu_bo_unref(bo_ptr);
> -	}
> -
> -	return r;
> +	return  amdgpu_bo_do_create(adev, bp, bo_ptr);

You can just rename amdgpu_bo_do_create() into amdgpu_bo_create() now 
and drop the wrapper as far as I can see.

Christian.

>   }
>   
>   /**

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

Re: [PATCH 3/5] drm/amdgpu: create shadow bo using amdgpu_bo_create_shadow()

[Nirmoy]

On 4/22/21 2:48 PM, Christian König wrote:
> Am 22.04.21 um 14:35 schrieb Nirmoy Das:
>> Shadow BOs are only needed for vm code so call amdgpu_bo_create_shadow()
>> directly instead of depending on amdgpu_bo_create().
>>
>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 75 +++++++++++++++++---------
>>   1 file changed, 49 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 6f0a6011cb3d..0e1d08a88f54 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -850,35 +850,64 @@ static int amdgpu_vm_clear_bo(struct 
>> amdgpu_device *adev,
>>   }
>>     /**
>> - * amdgpu_vm_bo_param - fill in parameters for PD/PT allocation
>> + * amdgpu_vm_bo_create - create bo for PD/PT
>
> Better name that amdgpu_vm_pt_create.
>
>>    *
>>    * @adev: amdgpu_device pointer
>>    * @vm: requesting vm
>>    * @level: the page table level
>>    * @immediate: use a immediate update
>> - * @bp: resulting BO allocation parameters
>> + * @bo: pointer to the buffer object pointer
>>    */
>> -static void amdgpu_vm_bo_param(struct amdgpu_device *adev, struct 
>> amdgpu_vm *vm,
>> -                   int level, bool immediate,
>> -                   struct amdgpu_bo_param *bp)
>> +static int amdgpu_vm_bo_create(struct amdgpu_device *adev,
>> +                struct amdgpu_vm *vm,
>> +                int level, bool immediate,
>> +                struct amdgpu_bo **bo)
>>   {
>> -    memset(bp, 0, sizeof(*bp));
>> +    struct amdgpu_bo_param bp;
>> +    bool create_shadow = false;
>> +    int r;
>>   -    bp->size = amdgpu_vm_bo_size(adev, level);
>> -    bp->byte_align = AMDGPU_GPU_PAGE_SIZE;
>> -    bp->domain = AMDGPU_GEM_DOMAIN_VRAM;
>> -    bp->domain = amdgpu_bo_get_preferred_pin_domain(adev, bp->domain);
>> -    bp->flags = AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS |
>> +    memset(&bp, 0, sizeof(bp));
>> +
>> +    bp.size = amdgpu_vm_bo_size(adev, level);
>> +    bp.byte_align = AMDGPU_GPU_PAGE_SIZE;
>> +    bp.domain = AMDGPU_GEM_DOMAIN_VRAM;
>> +    bp.domain = amdgpu_bo_get_preferred_pin_domain(adev, bp.domain);
>> +    bp.flags = AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS |
>>           AMDGPU_GEM_CREATE_CPU_GTT_USWC;
>> -    bp->bo_ptr_size = sizeof(struct amdgpu_bo);
>> +    bp.bo_ptr_size = sizeof(struct amdgpu_bo);
>>       if (vm->use_cpu_for_update)
>> -        bp->flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
>> +        bp.flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
>>       else if (!vm->root.base.bo || vm->root.base.bo->shadow)
>> -        bp->flags |= AMDGPU_GEM_CREATE_SHADOW;
>> -    bp->type = ttm_bo_type_kernel;
>> -    bp->no_wait_gpu = immediate;
>> +        create_shadow = true;
>> +
>> +    bp.type = ttm_bo_type_kernel;
>> +    bp.no_wait_gpu = immediate;
>>       if (vm->root.base.bo)
>> -        bp->resv = vm->root.base.bo->tbo.base.resv;
>> +        bp.resv = vm->root.base.bo->tbo.base.resv;
>> +
>> +
>> +    r = amdgpu_bo_create(adev, &bp, bo);
>> +    if (r)
>> +        return r;
>> +    if (!vm->is_compute_context &&
>> +        !(adev->flags & AMD_IS_APU) &&
>> +        create_shadow) {
>
> Better drop the create_show flag and just always check it like this:
>
> if (vm->is_compute_context || adev->flags & AMD_IS_APU)
>     return 0;
>
> Apart from that looks good to me.


Thanks Christian, I will resend with your suggestions.


Nirmoy

>
> Christian.
>
>> +        if (!bp.resv)
>> +            WARN_ON(dma_resv_lock((*bo)->tbo.base.resv,
>> +                          NULL));
>> +        r = amdgpu_bo_create_shadow(adev, bp.size, *bo);
>> +
>> +        if (!bp.resv)
>> +            dma_resv_unlock((*bo)->tbo.base.resv);
>> +
>> +        if (r) {
>> +            amdgpu_bo_unref(bo);
>> +            return r;
>> +        }
>> +    }
>> +
>> +    return 0;
>>   }
>>     /**
>> @@ -901,7 +930,6 @@ static int amdgpu_vm_alloc_pts(struct 
>> amdgpu_device *adev,
>>                      bool immediate)
>>   {
>>       struct amdgpu_vm_pt *entry = cursor->entry;
>> -    struct amdgpu_bo_param bp;
>>       struct amdgpu_bo *pt;
>>       int r;
>>   @@ -919,9 +947,7 @@ static int amdgpu_vm_alloc_pts(struct 
>> amdgpu_device *adev,
>>       if (entry->base.bo)
>>           return 0;
>>   -    amdgpu_vm_bo_param(adev, vm, cursor->level, immediate, &bp);
>> -
>> -    r = amdgpu_bo_create(adev, &bp, &pt);
>> +    r = amdgpu_vm_bo_create(adev, vm, cursor->level, immediate, &pt);
>>       if (r)
>>           return r;
>>   @@ -2785,7 +2811,6 @@ long amdgpu_vm_wait_idle(struct amdgpu_vm 
>> *vm, long timeout)
>>   int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>              int vm_context, u32 pasid)
>>   {
>> -    struct amdgpu_bo_param bp;
>>       struct amdgpu_bo *root;
>>       int r, i;
>>   @@ -2843,10 +2868,8 @@ int amdgpu_vm_init(struct amdgpu_device 
>> *adev, struct amdgpu_vm *vm,
>>       mutex_init(&vm->eviction_lock);
>>       vm->evicting = false;
>>   -    amdgpu_vm_bo_param(adev, vm, adev->vm_manager.root_level, 
>> false, &bp);
>> -    if (vm->is_compute_context)
>> -        bp.flags &= ~AMDGPU_GEM_CREATE_SHADOW;
>> -    r = amdgpu_bo_create(adev, &bp, &root);
>> +    r = amdgpu_vm_bo_create(adev, vm, adev->vm_manager.root_level,
>> +                false, &root);
>>       if (r)
>>           goto error_free_delayed;
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/5] drm/amdgpu: initialize vm->is_compute_context properly

[Felix Kuehling]

Am 2021-04-22 um 8:35 a.m. schrieb Nirmoy Das:
> Fix vm->is_compute_context initialization in amdgpu_vm_init().
>
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index f95bcda8463f..6f0a6011cb3d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2815,9 +2815,9 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>  		goto error_free_immediate;
>  
>  	vm->pte_support_ats = false;
> -	vm->is_compute_context = false;
> +	vm->is_compute_context = vm_context == AMDGPU_VM_CONTEXT_COMPUTE;
>  
> -	if (vm_context == AMDGPU_VM_CONTEXT_COMPUTE) {
> +	if (vm->is_compute_context) {

A weak ago or so, I submitted a patch that removed the last call to
amdgpu_vm_init with vm_context == AMDGPU_VM_CONTEXT_COMPUTE. We could
probably clean this up now and remove the vm_context parameter and the
AMDGPU_VM_CONTEXT* definitions. The only way to get a compute VM now is
through amdgpu_vm_make_compute.

Regards,
  Felix


>  		vm->use_cpu_for_update = !!(adev->vm_manager.vm_update_mode &
>  						AMDGPU_VM_USE_CPU_FOR_COMPUTE);
>  
> @@ -2844,7 +2844,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>  	vm->evicting = false;
>  
>  	amdgpu_vm_bo_param(adev, vm, adev->vm_manager.root_level, false, &bp);
> -	if (vm_context == AMDGPU_VM_CONTEXT_COMPUTE)
> +	if (vm->is_compute_context)
>  		bp.flags &= ~AMDGPU_GEM_CREATE_SHADOW;
>  	r = amdgpu_bo_create(adev, &bp, &root);
>  	if (r)
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/5] drm/amdgpu: initialize vm->is_compute_context properly

[Nirmoy]

On 4/22/21 3:56 PM, Felix Kuehling wrote:
> Am 2021-04-22 um 8:35 a.m. schrieb Nirmoy Das:
>> Fix vm->is_compute_context initialization in amdgpu_vm_init().
>>
>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index f95bcda8463f..6f0a6011cb3d 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -2815,9 +2815,9 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>   		goto error_free_immediate;
>>   
>>   	vm->pte_support_ats = false;
>> -	vm->is_compute_context = false;
>> +	vm->is_compute_context = vm_context == AMDGPU_VM_CONTEXT_COMPUTE;
>>   
>> -	if (vm_context == AMDGPU_VM_CONTEXT_COMPUTE) {
>> +	if (vm->is_compute_context) {
> A weak ago or so, I submitted a patch that removed the last call to
> amdgpu_vm_init with vm_context == AMDGPU_VM_CONTEXT_COMPUTE. We could
> probably clean this up now and remove the vm_context parameter and the
> AMDGPU_VM_CONTEXT* definitions. The only way to get a compute VM now is
> through amdgpu_vm_make_compute.


I was wondering about that. I will remove vm_context.


Thanks,

Nirmoy


> Regards,
>    Felix
>
>
>>   		vm->use_cpu_for_update = !!(adev->vm_manager.vm_update_mode &
>>   						AMDGPU_VM_USE_CPU_FOR_COMPUTE);
>>   
>> @@ -2844,7 +2844,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>   	vm->evicting = false;
>>   
>>   	amdgpu_vm_bo_param(adev, vm, adev->vm_manager.root_level, false, &bp);
>> -	if (vm_context == AMDGPU_VM_CONTEXT_COMPUTE)
>> +	if (vm->is_compute_context)
>>   		bp.flags &= ~AMDGPU_GEM_CREATE_SHADOW;
>>   	r = amdgpu_bo_create(adev, &bp, &root);
>>   	if (r)
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx