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

2021-04-22 Thread 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 
---
  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(, 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, , 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, );
-
-    r = amdgpu_bo_create(adev, , );
+    r = amdgpu_vm_bo_create(adev, vm, cursor->level, immediate, );
  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(>eviction_lock);
  vm->evicting = false;
  -    amdgpu_vm_bo_param(adev, vm, adev->vm_manager.root_level, 
false, );

-    if (vm->is_compute_context)
-    bp.flags &= ~AMDGPU_GEM_CREATE_SHADOW;
-    r = amdgpu_bo_create(adev, , );
+    r = amdgpu_vm_bo_create(adev, vm, adev->vm_manager.root_level,
+    false, );
  if (r)
  goto error_free_delayed;

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

2021-04-22 Thread 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 
---
  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(, 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, , 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, );

-
-   r = amdgpu_bo_create(adev, , );
+   r = amdgpu_vm_bo_create(adev, vm, cursor->level, immediate, );
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(>eviction_lock);
vm->evicting = false;
  
-	amdgpu_vm_bo_param(adev, vm, adev->vm_manager.root_level, false, );

-   if (vm->is_compute_context)
-

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

2021-04-22 Thread 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 
---
 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(, 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, , 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, );
-
-   r = amdgpu_bo_create(adev, , );
+   r = amdgpu_vm_bo_create(adev, vm, cursor->level, immediate, );
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(>eviction_lock);
vm->evicting = false;
 
-   amdgpu_vm_bo_param(adev, vm, adev->vm_manager.root_level, false, );
-   if (vm->is_compute_context)
-   bp.flags &= ~AMDGPU_GEM_CREATE_SHADOW;
-   r = amdgpu_bo_create(adev, , );
+   r = amdgpu_vm_bo_create(adev, vm, adev->vm_manager.root_level,
+   false, );
if (r)
goto error_free_delayed;
 
-- 
2.31.1