* [PATCH 1/5] drm/amdgpu: add amdgpu_bo_vm bo type
@ 2021-05-26 13:06 Nirmoy Das
2021-05-26 13:06 ` [PATCH v2 2/5] drm/amdgpu: move shadow bo validation to VM code Nirmoy Das
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Nirmoy Das @ 2021-05-26 13:06 UTC (permalink / raw)
To: amd-gfx; +Cc: alexander.deucher, Nirmoy Das, Christian König
Add new BO subclass that will be used by amdgpu vm code.
Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
Reviewed-by: Christian König <christian.koenig@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 32 ++++++++++++++++++++++
drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 10 +++++++
2 files changed, 42 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 3f85ba8222ef..6870cc297ae6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -694,6 +694,38 @@ int amdgpu_bo_create_user(struct amdgpu_device *adev,
*ubo_ptr = to_amdgpu_bo_user(bo_ptr);
return r;
}
+
+/**
+ * amdgpu_bo_create_vm - create an &amdgpu_bo_vm buffer object
+ * @adev: amdgpu device object
+ * @bp: parameters to be used for the buffer object
+ * @vmbo_ptr: pointer to the buffer object pointer
+ *
+ * Create a BO to be for GPUVM.
+ *
+ * Returns:
+ * 0 for success or a negative error code on failure.
+ */
+
+int amdgpu_bo_create_vm(struct amdgpu_device *adev,
+ struct amdgpu_bo_param *bp,
+ struct amdgpu_bo_vm **vmbo_ptr)
+{
+ struct amdgpu_bo *bo_ptr;
+ int r;
+
+ /* bo_ptr_size will be determined by the caller and it depends on
+ * num of amdgpu_vm_pt entries.
+ */
+ BUG_ON(bp->bo_ptr_size < sizeof(struct amdgpu_bo_vm));
+ r = amdgpu_bo_create(adev, bp, &bo_ptr);
+ if (r)
+ return r;
+
+ *vmbo_ptr = to_amdgpu_bo_vm(bo_ptr);
+ return r;
+}
+
/**
* amdgpu_bo_validate - validate an &amdgpu_bo buffer object
* @bo: pointer to the buffer object
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
index 11480c5a2716..a7fbf5f7051e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
@@ -44,6 +44,7 @@
#define AMDGPU_AMDKFD_CREATE_SVM_BO (1ULL << 62)
#define to_amdgpu_bo_user(abo) container_of((abo), struct amdgpu_bo_user, bo)
+#define to_amdgpu_bo_vm(abo) container_of((abo), struct amdgpu_bo_vm, bo)
struct amdgpu_bo_param {
unsigned long size;
@@ -125,6 +126,12 @@ struct amdgpu_bo_user {
};
+struct amdgpu_bo_vm {
+ struct amdgpu_bo bo;
+ struct amdgpu_bo *shadow;
+ struct amdgpu_vm_pt entries[];
+};
+
static inline struct amdgpu_bo *ttm_to_amdgpu_bo(struct ttm_buffer_object *tbo)
{
return container_of(tbo, struct amdgpu_bo, tbo);
@@ -272,6 +279,9 @@ int amdgpu_bo_create_kernel_at(struct amdgpu_device *adev,
int amdgpu_bo_create_user(struct amdgpu_device *adev,
struct amdgpu_bo_param *bp,
struct amdgpu_bo_user **ubo_ptr);
+int amdgpu_bo_create_vm(struct amdgpu_device *adev,
+ struct amdgpu_bo_param *bp,
+ struct amdgpu_bo_vm **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,
--
2.31.1
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 2/5] drm/amdgpu: move shadow bo validation to VM code
2021-05-26 13:06 [PATCH 1/5] drm/amdgpu: add amdgpu_bo_vm bo type Nirmoy Das
@ 2021-05-26 13:06 ` Nirmoy Das
2021-05-26 14:34 ` Christian König
2021-05-26 13:06 ` [PATCH v2 3/5] drm/amdgpu: switch to amdgpu_bo_vm for vm code Nirmoy Das
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Nirmoy Das @ 2021-05-26 13:06 UTC (permalink / raw)
To: amd-gfx; +Cc: alexander.deucher, Nirmoy Das, Christian.Koenig
Do the shadow bo validation in the VM code as
VM code knows/owns shadow BOs.
v2: Fix a typo.
Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 23 ++++-------------------
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 5 +++++
2 files changed, 9 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 90136f9dedd6..f6a8f0c5a52f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -396,10 +396,10 @@ void amdgpu_cs_report_moved_bytes(struct amdgpu_device *adev, u64 num_bytes,
spin_unlock(&adev->mm_stats.lock);
}
-static int amdgpu_cs_bo_validate(struct amdgpu_cs_parser *p,
- struct amdgpu_bo *bo)
+static int amdgpu_cs_bo_validate(void *param, struct amdgpu_bo *bo)
{
struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
+ struct amdgpu_cs_parser *p = param;
struct ttm_operation_ctx ctx = {
.interruptible = true,
.no_wait_gpu = false,
@@ -451,21 +451,6 @@ static int amdgpu_cs_bo_validate(struct amdgpu_cs_parser *p,
return r;
}
-static int amdgpu_cs_validate(void *param, struct amdgpu_bo *bo)
-{
- struct amdgpu_cs_parser *p = param;
- int r;
-
- r = amdgpu_cs_bo_validate(p, bo);
- if (r)
- return r;
-
- if (bo->shadow)
- r = amdgpu_cs_bo_validate(p, bo->shadow);
-
- return r;
-}
-
static int amdgpu_cs_list_validate(struct amdgpu_cs_parser *p,
struct list_head *validated)
{
@@ -493,7 +478,7 @@ static int amdgpu_cs_list_validate(struct amdgpu_cs_parser *p,
lobj->user_pages);
}
- r = amdgpu_cs_validate(p, bo);
+ r = amdgpu_cs_bo_validate(p, bo);
if (r)
return r;
@@ -593,7 +578,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
p->bytes_moved_vis = 0;
r = amdgpu_vm_validate_pt_bos(p->adev, &fpriv->vm,
- amdgpu_cs_validate, p);
+ amdgpu_cs_bo_validate, p);
if (r) {
DRM_ERROR("amdgpu_vm_validate_pt_bos() failed.\n");
goto error_validate;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index da155c276c51..6bc7566cc193 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -696,6 +696,11 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
r = validate(param, bo);
if (r)
return r;
+ if (bo->shadow) {
+ r = validate(param, bo->shadow);
+ if (r)
+ return r;
+ }
if (bo->tbo.type != ttm_bo_type_kernel) {
amdgpu_vm_bo_moved(bo_base);
--
2.31.1
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 3/5] drm/amdgpu: switch to amdgpu_bo_vm for vm code
2021-05-26 13:06 [PATCH 1/5] drm/amdgpu: add amdgpu_bo_vm bo type Nirmoy Das
2021-05-26 13:06 ` [PATCH v2 2/5] drm/amdgpu: move shadow bo validation to VM code Nirmoy Das
@ 2021-05-26 13:06 ` Nirmoy Das
2021-05-26 14:58 ` Christian König
2021-05-26 13:06 ` [PATCH v2 4/5] drm/amdgpu: remove unused code Nirmoy Das
2021-05-26 13:06 ` [PATCH 5/5] drm/amdgpu: do not allocate entries separately Nirmoy Das
3 siblings, 1 reply; 9+ messages in thread
From: Nirmoy Das @ 2021-05-26 13:06 UTC (permalink / raw)
To: amd-gfx; +Cc: alexander.deucher, Nirmoy Das, Christian.Koenig
The subclass, amdgpu_bo_vm is intended for PT/PD BOs which are also
shadowed, so switch to amdgpu_bo_vm BO for PT/PD BOs.
v2: squash three related patches.
Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 90 +++++++++++++--------
drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 16 ++--
2 files changed, 67 insertions(+), 39 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 6bc7566cc193..80d50e6d75f9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -658,9 +658,9 @@ void amdgpu_vm_move_to_lru_tail(struct amdgpu_device *adev,
ttm_bo_move_to_lru_tail(&bo->tbo, &bo->tbo.mem,
&vm->lru_bulk_move);
- if (bo->shadow)
- ttm_bo_move_to_lru_tail(&bo->shadow->tbo,
- &bo->shadow->tbo.mem,
+ if (bo->tbo.type == ttm_bo_type_kernel)
+ ttm_bo_move_to_lru_tail(&to_amdgpu_bo_vm(bo)->shadow->tbo,
+ &to_amdgpu_bo_vm(bo)->shadow->tbo.mem,
&vm->lru_bulk_move);
}
spin_unlock(&adev->mman.bdev.lru_lock);
@@ -696,8 +696,8 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
r = validate(param, bo);
if (r)
return r;
- if (bo->shadow) {
- r = validate(param, bo->shadow);
+ if (bo->tbo.type == ttm_bo_type_kernel) {
+ r = validate(param, to_amdgpu_bo_vm(bo)->shadow);
if (r)
return r;
}
@@ -793,8 +793,9 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
if (r)
return r;
- if (bo->shadow) {
- r = ttm_bo_validate(&bo->shadow->tbo, &bo->shadow->placement,
+ if (bo->tbo.type == ttm_bo_type_kernel) {
+ r = ttm_bo_validate(&to_amdgpu_bo_vm(bo)->shadow->tbo,
+ &to_amdgpu_bo_vm(bo)->shadow->placement,
&ctx);
if (r)
return r;
@@ -863,14 +864,17 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
* @vm: requesting vm
* @level: the page table level
* @immediate: use a immediate update
- * @bo: pointer to the buffer object pointer
+ * @vmbo: pointer to the buffer object pointer
*/
static int amdgpu_vm_pt_create(struct amdgpu_device *adev,
struct amdgpu_vm *vm,
int level, bool immediate,
- struct amdgpu_bo **bo)
+ struct amdgpu_bo_vm **vmbo)
{
struct amdgpu_bo_param bp;
+ struct amdgpu_bo *bo;
+ struct amdgpu_bo *shadow_bo;
+ struct dma_resv *resv;
int r;
memset(&bp, 0, sizeof(bp));
@@ -881,7 +885,7 @@ static int amdgpu_vm_pt_create(struct amdgpu_device *adev,
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_vm);
if (vm->use_cpu_for_update)
bp.flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
@@ -890,26 +894,43 @@ static int amdgpu_vm_pt_create(struct amdgpu_device *adev,
if (vm->root.base.bo)
bp.resv = vm->root.base.bo->tbo.base.resv;
- r = amdgpu_bo_create(adev, &bp, bo);
+ r = amdgpu_bo_create_vm(adev, &bp, vmbo);
if (r)
return r;
+ bo = &(*vmbo)->bo;
if (vm->is_compute_context && (adev->flags & AMD_IS_APU))
return 0;
if (!bp.resv)
- WARN_ON(dma_resv_lock((*bo)->tbo.base.resv,
+ WARN_ON(dma_resv_lock(bo->tbo.base.resv,
NULL));
- r = amdgpu_bo_create_shadow(adev, bp.size, *bo);
+ resv = bp.resv;
+ memset(&bp, 0, sizeof(bp));
+ bp.size = amdgpu_vm_bo_size(adev, level);
+ bp.domain = AMDGPU_GEM_DOMAIN_GTT;
+ 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);
- if (!bp.resv)
- dma_resv_unlock((*bo)->tbo.base.resv);
+ r = amdgpu_bo_create(adev, &bp, &shadow_bo);
+
+
+ if (!resv)
+ dma_resv_unlock(bo->tbo.base.resv);
if (r) {
- amdgpu_bo_unref(bo);
+ amdgpu_bo_unref(&bo);
return r;
}
+ shadow_bo->parent = amdgpu_bo_ref(bo);
+ mutex_lock(&adev->shadow_list_lock);
+ list_add_tail(&shadow_bo->shadow_list, &adev->shadow_list);
+ mutex_unlock(&adev->shadow_list_lock);
+ (*vmbo)->shadow = shadow_bo;
+
return 0;
}
@@ -933,7 +954,8 @@ static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
bool immediate)
{
struct amdgpu_vm_pt *entry = cursor->entry;
- struct amdgpu_bo *pt;
+ struct amdgpu_bo *pt_bo;
+ struct amdgpu_bo_vm *pt;
int r;
if (cursor->level < AMDGPU_VM_PTB && !entry->entries) {
@@ -957,10 +979,11 @@ static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
/* 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);
+ pt_bo = &pt->bo;
+ pt_bo->parent = amdgpu_bo_ref(cursor->parent->base.bo);
+ amdgpu_vm_bo_base_init(&entry->base, vm, pt_bo);
- r = amdgpu_vm_clear_bo(adev, vm, pt, immediate);
+ r = amdgpu_vm_clear_bo(adev, vm, pt_bo, immediate);
if (r)
goto error_free_pt;
@@ -968,7 +991,7 @@ static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
error_free_pt:
amdgpu_bo_unref(&pt->shadow);
- amdgpu_bo_unref(&pt);
+ amdgpu_bo_unref(&pt_bo);
return r;
}
@@ -982,7 +1005,8 @@ 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);
+ if (entry->base.bo->tbo.type == ttm_bo_type_kernel)
+ amdgpu_bo_unref(&to_amdgpu_bo_vm(entry->base.bo)->shadow);
amdgpu_bo_unref(&entry->base.bo);
}
kvfree(entry->entries);
@@ -2674,7 +2698,8 @@ void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
struct amdgpu_vm_bo_base *bo_base;
/* shadow bo doesn't have bo base, its validation needs its parent */
- if (bo->parent && bo->parent->shadow == bo)
+ if (bo->parent && bo->tbo.type == ttm_bo_type_kernel &&
+ to_amdgpu_bo_vm(bo->parent)->shadow == bo)
bo = bo->parent;
for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) {
@@ -2843,7 +2868,8 @@ long amdgpu_vm_wait_idle(struct amdgpu_vm *vm, long timeout)
*/
int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, u32 pasid)
{
- struct amdgpu_bo *root;
+ struct amdgpu_bo *root_bo;
+ struct amdgpu_bo_vm *root;
int r, i;
vm->va = RB_ROOT_CACHED;
@@ -2897,18 +2923,18 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, u32 pasid)
false, &root);
if (r)
goto error_free_delayed;
-
- r = amdgpu_bo_reserve(root, true);
+ root_bo = &root->bo;
+ r = amdgpu_bo_reserve(root_bo, true);
if (r)
goto error_free_root;
- r = dma_resv_reserve_shared(root->tbo.base.resv, 1);
+ r = dma_resv_reserve_shared(root_bo->tbo.base.resv, 1);
if (r)
goto error_unreserve;
- amdgpu_vm_bo_base_init(&vm->root.base, vm, root);
+ amdgpu_vm_bo_base_init(&vm->root.base, vm, root_bo);
- r = amdgpu_vm_clear_bo(adev, vm, root, false);
+ r = amdgpu_vm_clear_bo(adev, vm, root_bo, false);
if (r)
goto error_unreserve;
@@ -2935,8 +2961,8 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, u32 pasid)
amdgpu_bo_unreserve(vm->root.base.bo);
error_free_root:
- amdgpu_bo_unref(&vm->root.base.bo->shadow);
- amdgpu_bo_unref(&vm->root.base.bo);
+ amdgpu_bo_unref(&root->shadow);
+ amdgpu_bo_unref(&root_bo);
vm->root.base.bo = NULL;
error_free_delayed:
@@ -3078,7 +3104,7 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm,
}
/* Free the shadow bo for compute VM */
- amdgpu_bo_unref(&vm->root.base.bo->shadow);
+ amdgpu_bo_unref(&to_amdgpu_bo_vm(vm->root.base.bo)->shadow);
if (pasid)
vm->pasid = pasid;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
index a83a646759c5..3d9cff0c9dda 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
@@ -41,8 +41,8 @@ static int amdgpu_vm_sdma_map_table(struct amdgpu_bo *table)
if (r)
return r;
- if (table->shadow)
- r = amdgpu_ttm_alloc_gart(&table->shadow->tbo);
+ if (table->tbo.type == ttm_bo_type_kernel)
+ r = amdgpu_ttm_alloc_gart(&to_amdgpu_bo_vm(table)->shadow->tbo);
return r;
}
@@ -238,8 +238,9 @@ static int amdgpu_vm_sdma_update(struct amdgpu_vm_update_params *p,
if (!p->pages_addr) {
/* set page commands needed */
- if (bo->shadow)
- amdgpu_vm_sdma_set_ptes(p, bo->shadow, pe, addr,
+ if (bo->tbo.type == ttm_bo_type_kernel)
+ amdgpu_vm_sdma_set_ptes(p, to_amdgpu_bo_vm(bo)->shadow,
+ pe, addr,
count, incr, flags);
amdgpu_vm_sdma_set_ptes(p, bo, pe, addr, count,
incr, flags);
@@ -248,7 +249,7 @@ static int amdgpu_vm_sdma_update(struct amdgpu_vm_update_params *p,
/* copy commands needed */
ndw -= p->adev->vm_manager.vm_pte_funcs->copy_pte_num_dw *
- (bo->shadow ? 2 : 1);
+ ((bo->tbo.type == ttm_bo_type_kernel) ? 2 : 1);
/* for padding */
ndw -= 7;
@@ -263,8 +264,9 @@ static int amdgpu_vm_sdma_update(struct amdgpu_vm_update_params *p,
pte[i] |= flags;
}
- if (bo->shadow)
- amdgpu_vm_sdma_copy_ptes(p, bo->shadow, pe, nptes);
+ if (bo->tbo.type == ttm_bo_type_kernel)
+ amdgpu_vm_sdma_copy_ptes(p, to_amdgpu_bo_vm(bo)->shadow,
+ pe, nptes);
amdgpu_vm_sdma_copy_ptes(p, bo, pe, nptes);
pe += nptes * 8;
--
2.31.1
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 4/5] drm/amdgpu: remove unused code
2021-05-26 13:06 [PATCH 1/5] drm/amdgpu: add amdgpu_bo_vm bo type Nirmoy Das
2021-05-26 13:06 ` [PATCH v2 2/5] drm/amdgpu: move shadow bo validation to VM code Nirmoy Das
2021-05-26 13:06 ` [PATCH v2 3/5] drm/amdgpu: switch to amdgpu_bo_vm for vm code Nirmoy Das
@ 2021-05-26 13:06 ` Nirmoy Das
2021-05-26 13:06 ` [PATCH 5/5] drm/amdgpu: do not allocate entries separately Nirmoy Das
3 siblings, 0 replies; 9+ messages in thread
From: Nirmoy Das @ 2021-05-26 13:06 UTC (permalink / raw)
To: amd-gfx; +Cc: alexander.deucher, Nirmoy Das, Christian.Koenig
Remove unused code related to shadow BO.
v2: removing shadow bo ptr from base class.
Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 29 ----------------------
drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 6 -----
2 files changed, 35 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 6870cc297ae6..7930b7d9a3b1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -638,35 +638,6 @@ int amdgpu_bo_create(struct amdgpu_device *adev,
return r;
}
-int amdgpu_bo_create_shadow(struct amdgpu_device *adev,
- unsigned long size,
- struct amdgpu_bo *bo)
-{
- struct amdgpu_bo_param bp;
- int r;
-
- if (bo->shadow)
- return 0;
-
- memset(&bp, 0, sizeof(bp));
- bp.size = size;
- bp.domain = AMDGPU_GEM_DOMAIN_GTT;
- 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);
-
- r = amdgpu_bo_create(adev, &bp, &bo->shadow);
- if (!r) {
- bo->shadow->parent = amdgpu_bo_ref(bo);
- mutex_lock(&adev->shadow_list_lock);
- list_add_tail(&bo->shadow->shadow_list, &adev->shadow_list);
- mutex_unlock(&adev->shadow_list_lock);
- }
-
- return r;
-}
-
/**
* amdgpu_bo_create_user - create an &amdgpu_bo_user buffer object
* @adev: amdgpu device object
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
index a7fbf5f7051e..07478f86f05a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
@@ -104,9 +104,6 @@ struct amdgpu_bo {
struct amdgpu_vm_bo_base *vm_bo;
/* Constant after initialization */
struct amdgpu_bo *parent;
- struct amdgpu_bo *shadow;
-
-
#ifdef CONFIG_MMU_NOTIFIER
struct mmu_interval_notifier notifier;
@@ -284,9 +281,6 @@ int amdgpu_bo_create_vm(struct amdgpu_device *adev,
struct amdgpu_bo_vm **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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 5/5] drm/amdgpu: do not allocate entries separately
2021-05-26 13:06 [PATCH 1/5] drm/amdgpu: add amdgpu_bo_vm bo type Nirmoy Das
` (2 preceding siblings ...)
2021-05-26 13:06 ` [PATCH v2 4/5] drm/amdgpu: remove unused code Nirmoy Das
@ 2021-05-26 13:06 ` Nirmoy Das
3 siblings, 0 replies; 9+ messages in thread
From: Nirmoy Das @ 2021-05-26 13:06 UTC (permalink / raw)
To: amd-gfx; +Cc: alexander.deucher, Nirmoy Das, Christian.Koenig
Allocate PD/PT entries while allocating VM BOs and use that
instead of allocating those entries separately.
v2: create a new var for num entries.
Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 34 +++++++++++++++-----------
1 file changed, 20 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 80d50e6d75f9..de9dd882ecdc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -876,6 +876,7 @@ static int amdgpu_vm_pt_create(struct amdgpu_device *adev,
struct amdgpu_bo *shadow_bo;
struct dma_resv *resv;
int r;
+ unsigned int num_entries;
memset(&bp, 0, sizeof(bp));
@@ -885,7 +886,14 @@ static int amdgpu_vm_pt_create(struct amdgpu_device *adev,
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_vm);
+
+ if (level < AMDGPU_VM_PTB)
+ num_entries = amdgpu_vm_num_entries(adev, level);
+ else
+ num_entries = 0;
+
+ bp.bo_ptr_size = struct_size((*vmbo), entries, num_entries);
+
if (vm->use_cpu_for_update)
bp.flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
@@ -958,19 +966,14 @@ static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
struct amdgpu_bo_vm *pt;
int r;
- if (cursor->level < AMDGPU_VM_PTB && !entry->entries) {
- 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)
+ if (entry->base.bo) {
+ if (cursor->level < AMDGPU_VM_PTB)
+ entry->entries =
+ to_amdgpu_bo_vm(entry->base.bo)->entries;
+ else
+ entry->entries = NULL;
return 0;
+ }
r = amdgpu_vm_pt_create(adev, vm, cursor->level, immediate, &pt);
if (r)
@@ -982,6 +985,10 @@ static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
pt_bo = &pt->bo;
pt_bo->parent = amdgpu_bo_ref(cursor->parent->base.bo);
amdgpu_vm_bo_base_init(&entry->base, vm, pt_bo);
+ if (cursor->level < AMDGPU_VM_PTB)
+ entry->entries = pt->entries;
+ else
+ entry->entries = NULL;
r = amdgpu_vm_clear_bo(adev, vm, pt_bo, immediate);
if (r)
@@ -1009,7 +1016,6 @@ static void amdgpu_vm_free_table(struct amdgpu_vm_pt *entry)
amdgpu_bo_unref(&to_amdgpu_bo_vm(entry->base.bo)->shadow);
amdgpu_bo_unref(&entry->base.bo);
}
- kvfree(entry->entries);
entry->entries = NULL;
}
--
2.31.1
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/5] drm/amdgpu: move shadow bo validation to VM code
2021-05-26 13:06 ` [PATCH v2 2/5] drm/amdgpu: move shadow bo validation to VM code Nirmoy Das
@ 2021-05-26 14:34 ` Christian König
0 siblings, 0 replies; 9+ messages in thread
From: Christian König @ 2021-05-26 14:34 UTC (permalink / raw)
To: Nirmoy Das, amd-gfx; +Cc: alexander.deucher
Am 26.05.21 um 15:06 schrieb Nirmoy Das:
> Do the shadow bo validation in the VM code as
> VM code knows/owns shadow BOs.
>
> v2: Fix a typo.
>
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
Reviewed-by: Christian König <christian.koenig@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 23 ++++-------------------
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 5 +++++
> 2 files changed, 9 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 90136f9dedd6..f6a8f0c5a52f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -396,10 +396,10 @@ void amdgpu_cs_report_moved_bytes(struct amdgpu_device *adev, u64 num_bytes,
> spin_unlock(&adev->mm_stats.lock);
> }
>
> -static int amdgpu_cs_bo_validate(struct amdgpu_cs_parser *p,
> - struct amdgpu_bo *bo)
> +static int amdgpu_cs_bo_validate(void *param, struct amdgpu_bo *bo)
> {
> struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
> + struct amdgpu_cs_parser *p = param;
> struct ttm_operation_ctx ctx = {
> .interruptible = true,
> .no_wait_gpu = false,
> @@ -451,21 +451,6 @@ static int amdgpu_cs_bo_validate(struct amdgpu_cs_parser *p,
> return r;
> }
>
> -static int amdgpu_cs_validate(void *param, struct amdgpu_bo *bo)
> -{
> - struct amdgpu_cs_parser *p = param;
> - int r;
> -
> - r = amdgpu_cs_bo_validate(p, bo);
> - if (r)
> - return r;
> -
> - if (bo->shadow)
> - r = amdgpu_cs_bo_validate(p, bo->shadow);
> -
> - return r;
> -}
> -
> static int amdgpu_cs_list_validate(struct amdgpu_cs_parser *p,
> struct list_head *validated)
> {
> @@ -493,7 +478,7 @@ static int amdgpu_cs_list_validate(struct amdgpu_cs_parser *p,
> lobj->user_pages);
> }
>
> - r = amdgpu_cs_validate(p, bo);
> + r = amdgpu_cs_bo_validate(p, bo);
> if (r)
> return r;
>
> @@ -593,7 +578,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
> p->bytes_moved_vis = 0;
>
> r = amdgpu_vm_validate_pt_bos(p->adev, &fpriv->vm,
> - amdgpu_cs_validate, p);
> + amdgpu_cs_bo_validate, p);
> if (r) {
> DRM_ERROR("amdgpu_vm_validate_pt_bos() failed.\n");
> goto error_validate;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index da155c276c51..6bc7566cc193 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -696,6 +696,11 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> r = validate(param, bo);
> if (r)
> return r;
> + if (bo->shadow) {
> + r = validate(param, bo->shadow);
> + if (r)
> + return r;
> + }
>
> if (bo->tbo.type != ttm_bo_type_kernel) {
> amdgpu_vm_bo_moved(bo_base);
> --
> 2.31.1
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 3/5] drm/amdgpu: switch to amdgpu_bo_vm for vm code
2021-05-26 13:06 ` [PATCH v2 3/5] drm/amdgpu: switch to amdgpu_bo_vm for vm code Nirmoy Das
@ 2021-05-26 14:58 ` Christian König
2021-05-26 17:32 ` Nirmoy
0 siblings, 1 reply; 9+ messages in thread
From: Christian König @ 2021-05-26 14:58 UTC (permalink / raw)
To: Nirmoy Das, amd-gfx; +Cc: alexander.deucher
Am 26.05.21 um 15:06 schrieb Nirmoy Das:
> The subclass, amdgpu_bo_vm is intended for PT/PD BOs which are also
> shadowed, so switch to amdgpu_bo_vm BO for PT/PD BOs.
>
> v2: squash three related patches.
>
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 90 +++++++++++++--------
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 16 ++--
> 2 files changed, 67 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 6bc7566cc193..80d50e6d75f9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -658,9 +658,9 @@ void amdgpu_vm_move_to_lru_tail(struct amdgpu_device *adev,
>
> ttm_bo_move_to_lru_tail(&bo->tbo, &bo->tbo.mem,
> &vm->lru_bulk_move);
> - if (bo->shadow)
> - ttm_bo_move_to_lru_tail(&bo->shadow->tbo,
> - &bo->shadow->tbo.mem,
> + if (bo->tbo.type == ttm_bo_type_kernel)
> + ttm_bo_move_to_lru_tail(&to_amdgpu_bo_vm(bo)->shadow->tbo,
> + &to_amdgpu_bo_vm(bo)->shadow->tbo.mem,
Maybe use a local variable for the shadow BO here.
> &vm->lru_bulk_move);
> }
> spin_unlock(&adev->mman.bdev.lru_lock);
> @@ -696,8 +696,8 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> r = validate(param, bo);
> if (r)
> return r;
> - if (bo->shadow) {
> - r = validate(param, bo->shadow);
> + if (bo->tbo.type == ttm_bo_type_kernel) {
> + r = validate(param, to_amdgpu_bo_vm(bo)->shadow);
> if (r)
> return r;
> }
> @@ -793,8 +793,9 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
> if (r)
> return r;
>
> - if (bo->shadow) {
> - r = ttm_bo_validate(&bo->shadow->tbo, &bo->shadow->placement,
> + if (bo->tbo.type == ttm_bo_type_kernel) {
> + r = ttm_bo_validate(&to_amdgpu_bo_vm(bo)->shadow->tbo,
> + &to_amdgpu_bo_vm(bo)->shadow->placement,
Same here.
> &ctx);
> if (r)
> return r;
> @@ -863,14 +864,17 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
> * @vm: requesting vm
> * @level: the page table level
> * @immediate: use a immediate update
> - * @bo: pointer to the buffer object pointer
> + * @vmbo: pointer to the buffer object pointer
> */
> static int amdgpu_vm_pt_create(struct amdgpu_device *adev,
> struct amdgpu_vm *vm,
> int level, bool immediate,
> - struct amdgpu_bo **bo)
> + struct amdgpu_bo_vm **vmbo)
> {
> struct amdgpu_bo_param bp;
> + struct amdgpu_bo *bo;
> + struct amdgpu_bo *shadow_bo;
> + struct dma_resv *resv;
> int r;
>
> memset(&bp, 0, sizeof(bp));
> @@ -881,7 +885,7 @@ static int amdgpu_vm_pt_create(struct amdgpu_device *adev,
> 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_vm);
> if (vm->use_cpu_for_update)
> bp.flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
>
> @@ -890,26 +894,43 @@ static int amdgpu_vm_pt_create(struct amdgpu_device *adev,
> if (vm->root.base.bo)
> bp.resv = vm->root.base.bo->tbo.base.resv;
>
> - r = amdgpu_bo_create(adev, &bp, bo);
> + r = amdgpu_bo_create_vm(adev, &bp, vmbo);
> if (r)
> return r;
> + bo = &(*vmbo)->bo;
> if (vm->is_compute_context && (adev->flags & AMD_IS_APU))
> return 0;
>
> if (!bp.resv)
> - WARN_ON(dma_resv_lock((*bo)->tbo.base.resv,
> + WARN_ON(dma_resv_lock(bo->tbo.base.resv,
> NULL));
> - r = amdgpu_bo_create_shadow(adev, bp.size, *bo);
> + resv = bp.resv;
Maybe shuffle that code around a bit, then you only need the resv
variable and no longer the bo variable.
> + memset(&bp, 0, sizeof(bp));
> + bp.size = amdgpu_vm_bo_size(adev, level);
> + bp.domain = AMDGPU_GEM_DOMAIN_GTT;
> + 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);
>
> - if (!bp.resv)
> - dma_resv_unlock((*bo)->tbo.base.resv);
> + r = amdgpu_bo_create(adev, &bp, &shadow_bo);
> +
> +
Remove the two empty lines here
> + if (!resv)
> + dma_resv_unlock(bo->tbo.base.resv);
>
> if (r) {
> - amdgpu_bo_unref(bo);
> + amdgpu_bo_unref(&bo);
> return r;
> }
>
> + shadow_bo->parent = amdgpu_bo_ref(bo);
> + mutex_lock(&adev->shadow_list_lock);
> + list_add_tail(&shadow_bo->shadow_list, &adev->shadow_list);
> + mutex_unlock(&adev->shadow_list_lock);
> + (*vmbo)->shadow = shadow_bo;
Ok, we should either move the shadow_list into the vm_mgr structure or
keep that in the object code.
I think I prefer the later, something like amdgpu_bo_add_to_shadow_list().
> +
> return 0;
> }
>
> @@ -933,7 +954,8 @@ static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
> bool immediate)
> {
> struct amdgpu_vm_pt *entry = cursor->entry;
> - struct amdgpu_bo *pt;
> + struct amdgpu_bo *pt_bo;
> + struct amdgpu_bo_vm *pt;
> int r;
>
> if (cursor->level < AMDGPU_VM_PTB && !entry->entries) {
> @@ -957,10 +979,11 @@ static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
> /* 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);
> + pt_bo = &pt->bo;
> + pt_bo->parent = amdgpu_bo_ref(cursor->parent->base.bo);
> + amdgpu_vm_bo_base_init(&entry->base, vm, pt_bo);
>
> - r = amdgpu_vm_clear_bo(adev, vm, pt, immediate);
> + r = amdgpu_vm_clear_bo(adev, vm, pt_bo, immediate);
> if (r)
> goto error_free_pt;
>
> @@ -968,7 +991,7 @@ static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>
> error_free_pt:
> amdgpu_bo_unref(&pt->shadow);
> - amdgpu_bo_unref(&pt);
> + amdgpu_bo_unref(&pt_bo);
> return r;
> }
>
> @@ -982,7 +1005,8 @@ 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);
> + if (entry->base.bo->tbo.type == ttm_bo_type_kernel)
That should always be true, otherwise we have a rather big bug.
So no need to check that here.
> + amdgpu_bo_unref(&to_amdgpu_bo_vm(entry->base.bo)->shadow);
> amdgpu_bo_unref(&entry->base.bo);
> }
> kvfree(entry->entries);
> @@ -2674,7 +2698,8 @@ void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
> struct amdgpu_vm_bo_base *bo_base;
>
> /* shadow bo doesn't have bo base, its validation needs its parent */
> - if (bo->parent && bo->parent->shadow == bo)
> + if (bo->parent && bo->tbo.type == ttm_bo_type_kernel &&
> + to_amdgpu_bo_vm(bo->parent)->shadow == bo)
> bo = bo->parent;
>
> for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) {
> @@ -2843,7 +2868,8 @@ long amdgpu_vm_wait_idle(struct amdgpu_vm *vm, long timeout)
> */
> int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, u32 pasid)
> {
> - struct amdgpu_bo *root;
> + struct amdgpu_bo *root_bo;
> + struct amdgpu_bo_vm *root;
> int r, i;
>
> vm->va = RB_ROOT_CACHED;
> @@ -2897,18 +2923,18 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, u32 pasid)
> false, &root);
> if (r)
> goto error_free_delayed;
> -
> - r = amdgpu_bo_reserve(root, true);
> + root_bo = &root->bo;
> + r = amdgpu_bo_reserve(root_bo, true);
> if (r)
> goto error_free_root;
>
> - r = dma_resv_reserve_shared(root->tbo.base.resv, 1);
> + r = dma_resv_reserve_shared(root_bo->tbo.base.resv, 1);
> if (r)
> goto error_unreserve;
>
> - amdgpu_vm_bo_base_init(&vm->root.base, vm, root);
> + amdgpu_vm_bo_base_init(&vm->root.base, vm, root_bo);
>
> - r = amdgpu_vm_clear_bo(adev, vm, root, false);
> + r = amdgpu_vm_clear_bo(adev, vm, root_bo, false);
> if (r)
> goto error_unreserve;
>
> @@ -2935,8 +2961,8 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, u32 pasid)
> amdgpu_bo_unreserve(vm->root.base.bo);
>
> error_free_root:
> - amdgpu_bo_unref(&vm->root.base.bo->shadow);
> - amdgpu_bo_unref(&vm->root.base.bo);
> + amdgpu_bo_unref(&root->shadow);
> + amdgpu_bo_unref(&root_bo);
> vm->root.base.bo = NULL;
>
> error_free_delayed:
> @@ -3078,7 +3104,7 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> }
>
> /* Free the shadow bo for compute VM */
> - amdgpu_bo_unref(&vm->root.base.bo->shadow);
> + amdgpu_bo_unref(&to_amdgpu_bo_vm(vm->root.base.bo)->shadow);
>
> if (pasid)
> vm->pasid = pasid;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> index a83a646759c5..3d9cff0c9dda 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> @@ -41,8 +41,8 @@ static int amdgpu_vm_sdma_map_table(struct amdgpu_bo *table)
> if (r)
> return r;
>
> - if (table->shadow)
> - r = amdgpu_ttm_alloc_gart(&table->shadow->tbo);
> + if (table->tbo.type == ttm_bo_type_kernel)
Again that check should be unecessary.
> + r = amdgpu_ttm_alloc_gart(&to_amdgpu_bo_vm(table)->shadow->tbo);
>
> return r;
> }
> @@ -238,8 +238,9 @@ static int amdgpu_vm_sdma_update(struct amdgpu_vm_update_params *p,
>
> if (!p->pages_addr) {
> /* set page commands needed */
> - if (bo->shadow)
> - amdgpu_vm_sdma_set_ptes(p, bo->shadow, pe, addr,
> + if (bo->tbo.type == ttm_bo_type_kernel)
Same here.
> + amdgpu_vm_sdma_set_ptes(p, to_amdgpu_bo_vm(bo)->shadow,
> + pe, addr,
> count, incr, flags);
> amdgpu_vm_sdma_set_ptes(p, bo, pe, addr, count,
> incr, flags);
> @@ -248,7 +249,7 @@ static int amdgpu_vm_sdma_update(struct amdgpu_vm_update_params *p,
>
> /* copy commands needed */
> ndw -= p->adev->vm_manager.vm_pte_funcs->copy_pte_num_dw *
> - (bo->shadow ? 2 : 1);
> + ((bo->tbo.type == ttm_bo_type_kernel) ? 2 : 1);
And that here won't work and allocate to much SDMA space.
>
> /* for padding */
> ndw -= 7;
> @@ -263,8 +264,9 @@ static int amdgpu_vm_sdma_update(struct amdgpu_vm_update_params *p,
> pte[i] |= flags;
> }
>
> - if (bo->shadow)
> - amdgpu_vm_sdma_copy_ptes(p, bo->shadow, pe, nptes);
> + if (bo->tbo.type == ttm_bo_type_kernel)
And that is wrong as well.
Christian.
> + amdgpu_vm_sdma_copy_ptes(p, to_amdgpu_bo_vm(bo)->shadow,
> + pe, nptes);
> amdgpu_vm_sdma_copy_ptes(p, bo, pe, nptes);
>
> pe += nptes * 8;
> --
> 2.31.1
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 3/5] drm/amdgpu: switch to amdgpu_bo_vm for vm code
2021-05-26 14:58 ` Christian König
@ 2021-05-26 17:32 ` Nirmoy
2021-05-27 9:45 ` Christian König
0 siblings, 1 reply; 9+ messages in thread
From: Nirmoy @ 2021-05-26 17:32 UTC (permalink / raw)
To: Christian König, Nirmoy Das, amd-gfx; +Cc: alexander.deucher
Hi Christian,
On 5/26/21 4:58 PM, Christian König wrote:
> Am 26.05.21 um 15:06 schrieb Nirmoy Das:
>> The subclass, amdgpu_bo_vm is intended for PT/PD BOs which are also
>> shadowed, so switch to amdgpu_bo_vm BO for PT/PD BOs.
>>
>> v2: squash three related patches.
>>
>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 90 +++++++++++++--------
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 16 ++--
>> 2 files changed, 67 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 6bc7566cc193..80d50e6d75f9 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -658,9 +658,9 @@ void amdgpu_vm_move_to_lru_tail(struct
>> amdgpu_device *adev,
>>
>> ttm_bo_move_to_lru_tail(&bo->tbo, &bo->tbo.mem,
>> &vm->lru_bulk_move);
>> - if (bo->shadow)
>> - ttm_bo_move_to_lru_tail(&bo->shadow->tbo,
>> - &bo->shadow->tbo.mem,
>> + if (bo->tbo.type == ttm_bo_type_kernel)
>> + ttm_bo_move_to_lru_tail(&to_amdgpu_bo_vm(bo)->shadow->tbo,
>> + &to_amdgpu_bo_vm(bo)->shadow->tbo.mem,
>
> Maybe use a local variable for the shadow BO here.
>
>> &vm->lru_bulk_move);
>> }
>> spin_unlock(&adev->mman.bdev.lru_lock);
>> @@ -696,8 +696,8 @@ int amdgpu_vm_validate_pt_bos(struct
>> amdgpu_device *adev, struct amdgpu_vm *vm,
>> r = validate(param, bo);
>> if (r)
>> return r;
>> - if (bo->shadow) {
>> - r = validate(param, bo->shadow);
>> + if (bo->tbo.type == ttm_bo_type_kernel) {
>> + r = validate(param, to_amdgpu_bo_vm(bo)->shadow);
>> if (r)
>> return r;
>> }
>> @@ -793,8 +793,9 @@ static int amdgpu_vm_clear_bo(struct
>> amdgpu_device *adev,
>> if (r)
>> return r;
>>
>> - if (bo->shadow) {
>> - r = ttm_bo_validate(&bo->shadow->tbo, &bo->shadow->placement,
>> + if (bo->tbo.type == ttm_bo_type_kernel) {
>> + r = ttm_bo_validate(&to_amdgpu_bo_vm(bo)->shadow->tbo,
>> + &to_amdgpu_bo_vm(bo)->shadow->placement,
>
> Same here.
>
>> &ctx);
>> if (r)
>> return r;
>> @@ -863,14 +864,17 @@ static int amdgpu_vm_clear_bo(struct
>> amdgpu_device *adev,
>> * @vm: requesting vm
>> * @level: the page table level
>> * @immediate: use a immediate update
>> - * @bo: pointer to the buffer object pointer
>> + * @vmbo: pointer to the buffer object pointer
>> */
>> static int amdgpu_vm_pt_create(struct amdgpu_device *adev,
>> struct amdgpu_vm *vm,
>> int level, bool immediate,
>> - struct amdgpu_bo **bo)
>> + struct amdgpu_bo_vm **vmbo)
>> {
>> struct amdgpu_bo_param bp;
>> + struct amdgpu_bo *bo;
>> + struct amdgpu_bo *shadow_bo;
>> + struct dma_resv *resv;
>> int r;
>>
>> memset(&bp, 0, sizeof(bp));
>> @@ -881,7 +885,7 @@ static int amdgpu_vm_pt_create(struct
>> amdgpu_device *adev,
>> 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_vm);
>> if (vm->use_cpu_for_update)
>> bp.flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
>>
>> @@ -890,26 +894,43 @@ static int amdgpu_vm_pt_create(struct
>> amdgpu_device *adev,
>> if (vm->root.base.bo)
>> bp.resv = vm->root.base.bo->tbo.base.resv;
>>
>> - r = amdgpu_bo_create(adev, &bp, bo);
>> + r = amdgpu_bo_create_vm(adev, &bp, vmbo);
>> if (r)
>> return r;
>
>> + bo = &(*vmbo)->bo;
>> if (vm->is_compute_context && (adev->flags & AMD_IS_APU))
>> return 0;
>>
>> if (!bp.resv)
>> - WARN_ON(dma_resv_lock((*bo)->tbo.base.resv,
>> + WARN_ON(dma_resv_lock(bo->tbo.base.resv,
>> NULL));
>> - r = amdgpu_bo_create_shadow(adev, bp.size, *bo);
>> + resv = bp.resv;
>
> Maybe shuffle that code around a bit, then you only need the resv
> variable and no longer the bo variable.
I can remove the shadow_bo variable but I need the bo for
amdgpu_bo_unref(), as this takes amdgpu_bo ** as argument.
Regards,
Nirmoy
>
>> + memset(&bp, 0, sizeof(bp));
>> + bp.size = amdgpu_vm_bo_size(adev, level);
>> + bp.domain = AMDGPU_GEM_DOMAIN_GTT;
>> + 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);
>>
>> - if (!bp.resv)
>> - dma_resv_unlock((*bo)->tbo.base.resv);
>> + r = amdgpu_bo_create(adev, &bp, &shadow_bo);
>
>> +
>> +
>
> Remove the two empty lines here
>
>> + if (!resv)
>> + dma_resv_unlock(bo->tbo.base.resv);
>>
>> if (r) {
>> - amdgpu_bo_unref(bo);
>> + amdgpu_bo_unref(&bo);
>> return r;
>> }
>>
>> + shadow_bo->parent = amdgpu_bo_ref(bo);
>> + mutex_lock(&adev->shadow_list_lock);
>> + list_add_tail(&shadow_bo->shadow_list, &adev->shadow_list);
>> + mutex_unlock(&adev->shadow_list_lock);
>> + (*vmbo)->shadow = shadow_bo;
>
> Ok, we should either move the shadow_list into the vm_mgr structure or
> keep that in the object code.
>
> I think I prefer the later, something like
> amdgpu_bo_add_to_shadow_list().
>
>> +
>> return 0;
>> }
>>
>> @@ -933,7 +954,8 @@ static int amdgpu_vm_alloc_pts(struct
>> amdgpu_device *adev,
>> bool immediate)
>> {
>> struct amdgpu_vm_pt *entry = cursor->entry;
>> - struct amdgpu_bo *pt;
>> + struct amdgpu_bo *pt_bo;
>> + struct amdgpu_bo_vm *pt;
>> int r;
>>
>> if (cursor->level < AMDGPU_VM_PTB && !entry->entries) {
>> @@ -957,10 +979,11 @@ static int amdgpu_vm_alloc_pts(struct
>> amdgpu_device *adev,
>> /* 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);
>> + pt_bo = &pt->bo;
>> + pt_bo->parent = amdgpu_bo_ref(cursor->parent->base.bo);
>> + amdgpu_vm_bo_base_init(&entry->base, vm, pt_bo);
>>
>> - r = amdgpu_vm_clear_bo(adev, vm, pt, immediate);
>> + r = amdgpu_vm_clear_bo(adev, vm, pt_bo, immediate);
>> if (r)
>> goto error_free_pt;
>>
>> @@ -968,7 +991,7 @@ static int amdgpu_vm_alloc_pts(struct
>> amdgpu_device *adev,
>>
>> error_free_pt:
>> amdgpu_bo_unref(&pt->shadow);
>> - amdgpu_bo_unref(&pt);
>> + amdgpu_bo_unref(&pt_bo);
>> return r;
>> }
>>
>> @@ -982,7 +1005,8 @@ 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);
>> + if (entry->base.bo->tbo.type == ttm_bo_type_kernel)
>
> That should always be true, otherwise we have a rather big bug.
>
> So no need to check that here.
>
>> + amdgpu_bo_unref(&to_amdgpu_bo_vm(entry->base.bo)->shadow);
>> amdgpu_bo_unref(&entry->base.bo);
>> }
>> kvfree(entry->entries);
>> @@ -2674,7 +2698,8 @@ void amdgpu_vm_bo_invalidate(struct
>> amdgpu_device *adev,
>> struct amdgpu_vm_bo_base *bo_base;
>>
>> /* shadow bo doesn't have bo base, its validation needs its
>> parent */
>> - if (bo->parent && bo->parent->shadow == bo)
>> + if (bo->parent && bo->tbo.type == ttm_bo_type_kernel &&
>> + to_amdgpu_bo_vm(bo->parent)->shadow == bo)
>> bo = bo->parent;
>>
>> for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) {
>> @@ -2843,7 +2868,8 @@ long amdgpu_vm_wait_idle(struct amdgpu_vm *vm,
>> long timeout)
>> */
>> int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm
>> *vm, u32 pasid)
>> {
>> - struct amdgpu_bo *root;
>> + struct amdgpu_bo *root_bo;
>> + struct amdgpu_bo_vm *root;
>> int r, i;
>>
>> vm->va = RB_ROOT_CACHED;
>> @@ -2897,18 +2923,18 @@ int amdgpu_vm_init(struct amdgpu_device
>> *adev, struct amdgpu_vm *vm, u32 pasid)
>> false, &root);
>> if (r)
>> goto error_free_delayed;
>> -
>> - r = amdgpu_bo_reserve(root, true);
>> + root_bo = &root->bo;
>> + r = amdgpu_bo_reserve(root_bo, true);
>> if (r)
>> goto error_free_root;
>>
>> - r = dma_resv_reserve_shared(root->tbo.base.resv, 1);
>> + r = dma_resv_reserve_shared(root_bo->tbo.base.resv, 1);
>> if (r)
>> goto error_unreserve;
>>
>> - amdgpu_vm_bo_base_init(&vm->root.base, vm, root);
>> + amdgpu_vm_bo_base_init(&vm->root.base, vm, root_bo);
>>
>> - r = amdgpu_vm_clear_bo(adev, vm, root, false);
>> + r = amdgpu_vm_clear_bo(adev, vm, root_bo, false);
>> if (r)
>> goto error_unreserve;
>>
>> @@ -2935,8 +2961,8 @@ int amdgpu_vm_init(struct amdgpu_device *adev,
>> struct amdgpu_vm *vm, u32 pasid)
>> amdgpu_bo_unreserve(vm->root.base.bo);
>>
>> error_free_root:
>> - amdgpu_bo_unref(&vm->root.base.bo->shadow);
>> - amdgpu_bo_unref(&vm->root.base.bo);
>> + amdgpu_bo_unref(&root->shadow);
>> + amdgpu_bo_unref(&root_bo);
>> vm->root.base.bo = NULL;
>>
>> error_free_delayed:
>> @@ -3078,7 +3104,7 @@ int amdgpu_vm_make_compute(struct amdgpu_device
>> *adev, struct amdgpu_vm *vm,
>> }
>>
>> /* Free the shadow bo for compute VM */
>> - amdgpu_bo_unref(&vm->root.base.bo->shadow);
>> + amdgpu_bo_unref(&to_amdgpu_bo_vm(vm->root.base.bo)->shadow);
>>
>> if (pasid)
>> vm->pasid = pasid;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
>> index a83a646759c5..3d9cff0c9dda 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
>> @@ -41,8 +41,8 @@ static int amdgpu_vm_sdma_map_table(struct
>> amdgpu_bo *table)
>> if (r)
>> return r;
>>
>> - if (table->shadow)
>> - r = amdgpu_ttm_alloc_gart(&table->shadow->tbo);
>> + if (table->tbo.type == ttm_bo_type_kernel)
>
>
> Again that check should be unecessary.
>> + r =
>> amdgpu_ttm_alloc_gart(&to_amdgpu_bo_vm(table)->shadow->tbo);
>>
>> return r;
>> }
>> @@ -238,8 +238,9 @@ static int amdgpu_vm_sdma_update(struct
>> amdgpu_vm_update_params *p,
>>
>> if (!p->pages_addr) {
>> /* set page commands needed */
>> - if (bo->shadow)
>> - amdgpu_vm_sdma_set_ptes(p, bo->shadow, pe, addr,
>> + if (bo->tbo.type == ttm_bo_type_kernel)
>
> Same here.
>
>> + amdgpu_vm_sdma_set_ptes(p, to_amdgpu_bo_vm(bo)->shadow,
>> + pe, addr,
>> count, incr, flags);
>> amdgpu_vm_sdma_set_ptes(p, bo, pe, addr, count,
>> incr, flags);
>> @@ -248,7 +249,7 @@ static int amdgpu_vm_sdma_update(struct
>> amdgpu_vm_update_params *p,
>>
>> /* copy commands needed */
>> ndw -= p->adev->vm_manager.vm_pte_funcs->copy_pte_num_dw *
>> - (bo->shadow ? 2 : 1);
>> + ((bo->tbo.type == ttm_bo_type_kernel) ? 2 : 1);
>
> And that here won't work and allocate to much SDMA space.
>
>>
>> /* for padding */
>> ndw -= 7;
>> @@ -263,8 +264,9 @@ static int amdgpu_vm_sdma_update(struct
>> amdgpu_vm_update_params *p,
>> pte[i] |= flags;
>> }
>>
>> - if (bo->shadow)
>> - amdgpu_vm_sdma_copy_ptes(p, bo->shadow, pe, nptes);
>> + if (bo->tbo.type == ttm_bo_type_kernel)
> And that is wrong as well.
>
> Christian.
>
>> + amdgpu_vm_sdma_copy_ptes(p, to_amdgpu_bo_vm(bo)->shadow,
>> + pe, nptes);
>> amdgpu_vm_sdma_copy_ptes(p, bo, pe, nptes);
>>
>> pe += nptes * 8;
>> --
>> 2.31.1
>>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 3/5] drm/amdgpu: switch to amdgpu_bo_vm for vm code
2021-05-26 17:32 ` Nirmoy
@ 2021-05-27 9:45 ` Christian König
0 siblings, 0 replies; 9+ messages in thread
From: Christian König @ 2021-05-27 9:45 UTC (permalink / raw)
To: Nirmoy, Nirmoy Das, amd-gfx; +Cc: alexander.deucher
Am 26.05.21 um 19:32 schrieb Nirmoy:
> Hi Christian,
>
> On 5/26/21 4:58 PM, Christian König wrote:
>> Am 26.05.21 um 15:06 schrieb Nirmoy Das:
>>> The subclass, amdgpu_bo_vm is intended for PT/PD BOs which are also
>>> shadowed, so switch to amdgpu_bo_vm BO for PT/PD BOs.
>>>
>>> v2: squash three related patches.
>>>
>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 90
>>> +++++++++++++--------
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 16 ++--
>>> 2 files changed, 67 insertions(+), 39 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index 6bc7566cc193..80d50e6d75f9 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -658,9 +658,9 @@ void amdgpu_vm_move_to_lru_tail(struct
>>> amdgpu_device *adev,
>>>
>>> ttm_bo_move_to_lru_tail(&bo->tbo, &bo->tbo.mem,
>>> &vm->lru_bulk_move);
>>> - if (bo->shadow)
>>> - ttm_bo_move_to_lru_tail(&bo->shadow->tbo,
>>> - &bo->shadow->tbo.mem,
>>> + if (bo->tbo.type == ttm_bo_type_kernel)
>>> + ttm_bo_move_to_lru_tail(&to_amdgpu_bo_vm(bo)->shadow->tbo,
>>> + &to_amdgpu_bo_vm(bo)->shadow->tbo.mem,
>>
>> Maybe use a local variable for the shadow BO here.
>>
>>> &vm->lru_bulk_move);
>>> }
>>> spin_unlock(&adev->mman.bdev.lru_lock);
>>> @@ -696,8 +696,8 @@ int amdgpu_vm_validate_pt_bos(struct
>>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>> r = validate(param, bo);
>>> if (r)
>>> return r;
>>> - if (bo->shadow) {
>>> - r = validate(param, bo->shadow);
>>> + if (bo->tbo.type == ttm_bo_type_kernel) {
>>> + r = validate(param, to_amdgpu_bo_vm(bo)->shadow);
>>> if (r)
>>> return r;
>>> }
>>> @@ -793,8 +793,9 @@ static int amdgpu_vm_clear_bo(struct
>>> amdgpu_device *adev,
>>> if (r)
>>> return r;
>>>
>>> - if (bo->shadow) {
>>> - r = ttm_bo_validate(&bo->shadow->tbo, &bo->shadow->placement,
>>> + if (bo->tbo.type == ttm_bo_type_kernel) {
>>> + r = ttm_bo_validate(&to_amdgpu_bo_vm(bo)->shadow->tbo,
>>> + &to_amdgpu_bo_vm(bo)->shadow->placement,
>>
>> Same here.
>>
>>> &ctx);
>>> if (r)
>>> return r;
>>> @@ -863,14 +864,17 @@ static int amdgpu_vm_clear_bo(struct
>>> amdgpu_device *adev,
>>> * @vm: requesting vm
>>> * @level: the page table level
>>> * @immediate: use a immediate update
>>> - * @bo: pointer to the buffer object pointer
>>> + * @vmbo: pointer to the buffer object pointer
>>> */
>>> static int amdgpu_vm_pt_create(struct amdgpu_device *adev,
>>> struct amdgpu_vm *vm,
>>> int level, bool immediate,
>>> - struct amdgpu_bo **bo)
>>> + struct amdgpu_bo_vm **vmbo)
>>> {
>>> struct amdgpu_bo_param bp;
>>> + struct amdgpu_bo *bo;
>>> + struct amdgpu_bo *shadow_bo;
>>> + struct dma_resv *resv;
>>> int r;
>>>
>>> memset(&bp, 0, sizeof(bp));
>>> @@ -881,7 +885,7 @@ static int amdgpu_vm_pt_create(struct
>>> amdgpu_device *adev,
>>> 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_vm);
>>> if (vm->use_cpu_for_update)
>>> bp.flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
>>>
>>> @@ -890,26 +894,43 @@ static int amdgpu_vm_pt_create(struct
>>> amdgpu_device *adev,
>>> if (vm->root.base.bo)
>>> bp.resv = vm->root.base.bo->tbo.base.resv;
>>>
>>> - r = amdgpu_bo_create(adev, &bp, bo);
>>> + r = amdgpu_bo_create_vm(adev, &bp, vmbo);
>>> if (r)
>>> return r;
>>
>>> + bo = &(*vmbo)->bo;
>>> if (vm->is_compute_context && (adev->flags & AMD_IS_APU))
>>> return 0;
>>>
>>> if (!bp.resv)
>>> - WARN_ON(dma_resv_lock((*bo)->tbo.base.resv,
>>> + WARN_ON(dma_resv_lock(bo->tbo.base.resv,
>>> NULL));
>>> - r = amdgpu_bo_create_shadow(adev, bp.size, *bo);
>>> + resv = bp.resv;
>>
>> Maybe shuffle that code around a bit, then you only need the resv
>> variable and no longer the bo variable.
>
>
> I can remove the shadow_bo variable but I need the bo for
> amdgpu_bo_unref(), as this takes amdgpu_bo ** as argument.
Fine with me as well, just try to clean up the code a bit.
Regards,
Christian.
>
>
> Regards,
>
> Nirmoy
>
>
>>
>>> + memset(&bp, 0, sizeof(bp));
>>> + bp.size = amdgpu_vm_bo_size(adev, level);
>>> + bp.domain = AMDGPU_GEM_DOMAIN_GTT;
>>> + 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);
>>>
>>> - if (!bp.resv)
>>> - dma_resv_unlock((*bo)->tbo.base.resv);
>>> + r = amdgpu_bo_create(adev, &bp, &shadow_bo);
>>
>>> +
>>> +
>>
>> Remove the two empty lines here
>>
>>> + if (!resv)
>>> + dma_resv_unlock(bo->tbo.base.resv);
>>>
>>> if (r) {
>>> - amdgpu_bo_unref(bo);
>>> + amdgpu_bo_unref(&bo);
>>> return r;
>>> }
>>>
>>> + shadow_bo->parent = amdgpu_bo_ref(bo);
>>> + mutex_lock(&adev->shadow_list_lock);
>>> + list_add_tail(&shadow_bo->shadow_list, &adev->shadow_list);
>>> + mutex_unlock(&adev->shadow_list_lock);
>>> + (*vmbo)->shadow = shadow_bo;
>>
>> Ok, we should either move the shadow_list into the vm_mgr structure
>> or keep that in the object code.
>>
>> I think I prefer the later, something like
>> amdgpu_bo_add_to_shadow_list().
>>
>>> +
>>> return 0;
>>> }
>>>
>>> @@ -933,7 +954,8 @@ static int amdgpu_vm_alloc_pts(struct
>>> amdgpu_device *adev,
>>> bool immediate)
>>> {
>>> struct amdgpu_vm_pt *entry = cursor->entry;
>>> - struct amdgpu_bo *pt;
>>> + struct amdgpu_bo *pt_bo;
>>> + struct amdgpu_bo_vm *pt;
>>> int r;
>>>
>>> if (cursor->level < AMDGPU_VM_PTB && !entry->entries) {
>>> @@ -957,10 +979,11 @@ static int amdgpu_vm_alloc_pts(struct
>>> amdgpu_device *adev,
>>> /* 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);
>>> + pt_bo = &pt->bo;
>>> + pt_bo->parent = amdgpu_bo_ref(cursor->parent->base.bo);
>>> + amdgpu_vm_bo_base_init(&entry->base, vm, pt_bo);
>>>
>>> - r = amdgpu_vm_clear_bo(adev, vm, pt, immediate);
>>> + r = amdgpu_vm_clear_bo(adev, vm, pt_bo, immediate);
>>> if (r)
>>> goto error_free_pt;
>>>
>>> @@ -968,7 +991,7 @@ static int amdgpu_vm_alloc_pts(struct
>>> amdgpu_device *adev,
>>>
>>> error_free_pt:
>>> amdgpu_bo_unref(&pt->shadow);
>>> - amdgpu_bo_unref(&pt);
>>> + amdgpu_bo_unref(&pt_bo);
>>> return r;
>>> }
>>>
>>> @@ -982,7 +1005,8 @@ 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);
>>> + if (entry->base.bo->tbo.type == ttm_bo_type_kernel)
>>
>> That should always be true, otherwise we have a rather big bug.
>>
>> So no need to check that here.
>>
>>> + amdgpu_bo_unref(&to_amdgpu_bo_vm(entry->base.bo)->shadow);
>>> amdgpu_bo_unref(&entry->base.bo);
>>> }
>>> kvfree(entry->entries);
>>> @@ -2674,7 +2698,8 @@ void amdgpu_vm_bo_invalidate(struct
>>> amdgpu_device *adev,
>>> struct amdgpu_vm_bo_base *bo_base;
>>>
>>> /* shadow bo doesn't have bo base, its validation needs its
>>> parent */
>>> - if (bo->parent && bo->parent->shadow == bo)
>>> + if (bo->parent && bo->tbo.type == ttm_bo_type_kernel &&
>>> + to_amdgpu_bo_vm(bo->parent)->shadow == bo)
>>> bo = bo->parent;
>>>
>>> for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) {
>>> @@ -2843,7 +2868,8 @@ long amdgpu_vm_wait_idle(struct amdgpu_vm *vm,
>>> long timeout)
>>> */
>>> int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm
>>> *vm, u32 pasid)
>>> {
>>> - struct amdgpu_bo *root;
>>> + struct amdgpu_bo *root_bo;
>>> + struct amdgpu_bo_vm *root;
>>> int r, i;
>>>
>>> vm->va = RB_ROOT_CACHED;
>>> @@ -2897,18 +2923,18 @@ int amdgpu_vm_init(struct amdgpu_device
>>> *adev, struct amdgpu_vm *vm, u32 pasid)
>>> false, &root);
>>> if (r)
>>> goto error_free_delayed;
>>> -
>>> - r = amdgpu_bo_reserve(root, true);
>>> + root_bo = &root->bo;
>>> + r = amdgpu_bo_reserve(root_bo, true);
>>> if (r)
>>> goto error_free_root;
>>>
>>> - r = dma_resv_reserve_shared(root->tbo.base.resv, 1);
>>> + r = dma_resv_reserve_shared(root_bo->tbo.base.resv, 1);
>>> if (r)
>>> goto error_unreserve;
>>>
>>> - amdgpu_vm_bo_base_init(&vm->root.base, vm, root);
>>> + amdgpu_vm_bo_base_init(&vm->root.base, vm, root_bo);
>>>
>>> - r = amdgpu_vm_clear_bo(adev, vm, root, false);
>>> + r = amdgpu_vm_clear_bo(adev, vm, root_bo, false);
>>> if (r)
>>> goto error_unreserve;
>>>
>>> @@ -2935,8 +2961,8 @@ int amdgpu_vm_init(struct amdgpu_device *adev,
>>> struct amdgpu_vm *vm, u32 pasid)
>>> amdgpu_bo_unreserve(vm->root.base.bo);
>>>
>>> error_free_root:
>>> - amdgpu_bo_unref(&vm->root.base.bo->shadow);
>>> - amdgpu_bo_unref(&vm->root.base.bo);
>>> + amdgpu_bo_unref(&root->shadow);
>>> + amdgpu_bo_unref(&root_bo);
>>> vm->root.base.bo = NULL;
>>>
>>> error_free_delayed:
>>> @@ -3078,7 +3104,7 @@ int amdgpu_vm_make_compute(struct
>>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>> }
>>>
>>> /* Free the shadow bo for compute VM */
>>> - amdgpu_bo_unref(&vm->root.base.bo->shadow);
>>> + amdgpu_bo_unref(&to_amdgpu_bo_vm(vm->root.base.bo)->shadow);
>>>
>>> if (pasid)
>>> vm->pasid = pasid;
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
>>> index a83a646759c5..3d9cff0c9dda 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
>>> @@ -41,8 +41,8 @@ static int amdgpu_vm_sdma_map_table(struct
>>> amdgpu_bo *table)
>>> if (r)
>>> return r;
>>>
>>> - if (table->shadow)
>>> - r = amdgpu_ttm_alloc_gart(&table->shadow->tbo);
>>> + if (table->tbo.type == ttm_bo_type_kernel)
>>
>>
>> Again that check should be unecessary.
>>> + r =
>>> amdgpu_ttm_alloc_gart(&to_amdgpu_bo_vm(table)->shadow->tbo);
>>>
>>> return r;
>>> }
>>> @@ -238,8 +238,9 @@ static int amdgpu_vm_sdma_update(struct
>>> amdgpu_vm_update_params *p,
>>>
>>> if (!p->pages_addr) {
>>> /* set page commands needed */
>>> - if (bo->shadow)
>>> - amdgpu_vm_sdma_set_ptes(p, bo->shadow, pe, addr,
>>> + if (bo->tbo.type == ttm_bo_type_kernel)
>>
>> Same here.
>>
>>> + amdgpu_vm_sdma_set_ptes(p, to_amdgpu_bo_vm(bo)->shadow,
>>> + pe, addr,
>>> count, incr, flags);
>>> amdgpu_vm_sdma_set_ptes(p, bo, pe, addr, count,
>>> incr, flags);
>>> @@ -248,7 +249,7 @@ static int amdgpu_vm_sdma_update(struct
>>> amdgpu_vm_update_params *p,
>>>
>>> /* copy commands needed */
>>> ndw -= p->adev->vm_manager.vm_pte_funcs->copy_pte_num_dw *
>>> - (bo->shadow ? 2 : 1);
>>> + ((bo->tbo.type == ttm_bo_type_kernel) ? 2 : 1);
>>
>> And that here won't work and allocate to much SDMA space.
>>
>>>
>>> /* for padding */
>>> ndw -= 7;
>>> @@ -263,8 +264,9 @@ static int amdgpu_vm_sdma_update(struct
>>> amdgpu_vm_update_params *p,
>>> pte[i] |= flags;
>>> }
>>>
>>> - if (bo->shadow)
>>> - amdgpu_vm_sdma_copy_ptes(p, bo->shadow, pe, nptes);
>>> + if (bo->tbo.type == ttm_bo_type_kernel)
>> And that is wrong as well.
>>
>> Christian.
>>
>>> + amdgpu_vm_sdma_copy_ptes(p, to_amdgpu_bo_vm(bo)->shadow,
>>> + pe, nptes);
>>> amdgpu_vm_sdma_copy_ptes(p, bo, pe, nptes);
>>>
>>> pe += nptes * 8;
>>> --
>>> 2.31.1
>>>
>>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-05-27 9:45 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-26 13:06 [PATCH 1/5] drm/amdgpu: add amdgpu_bo_vm bo type Nirmoy Das
2021-05-26 13:06 ` [PATCH v2 2/5] drm/amdgpu: move shadow bo validation to VM code Nirmoy Das
2021-05-26 14:34 ` Christian König
2021-05-26 13:06 ` [PATCH v2 3/5] drm/amdgpu: switch to amdgpu_bo_vm for vm code Nirmoy Das
2021-05-26 14:58 ` Christian König
2021-05-26 17:32 ` Nirmoy
2021-05-27 9:45 ` Christian König
2021-05-26 13:06 ` [PATCH v2 4/5] drm/amdgpu: remove unused code Nirmoy Das
2021-05-26 13:06 ` [PATCH 5/5] drm/amdgpu: do not allocate entries separately Nirmoy Das
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.