[AMD Official Use Only]
Thanks Christian, I will resend with your suggestions included.
Nirmoy
-----Original Message-----
From: Koenig, Christian <christian.koe...@amd.com>
Sent: Friday, May 28, 2021 10:01 AM
To: Das, Nirmoy <nirmoy....@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander <alexander.deuc...@amd.com>
Subject: Re: [PATCH v3 4/6] drm/amdgpu: switch to amdgpu_bo_vm for vm code
Am 27.05.21 um 13:53 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.
>
> v3: simplify code.
> check also if shadow bo exist, instead of checking only bo's type.
> v2: squash three related patches.
>
> Signed-off-by: Nirmoy Das <nirmoy....@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 93 +++++++++++++--------
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 18 ++--
> 2 files changed, 68 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 6bc7566cc193..d723873df765 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -652,15 +652,15 @@ void amdgpu_vm_move_to_lru_tail(struct amdgpu_device
> *adev,
> spin_lock(&adev->mman.bdev.lru_lock);
> list_for_each_entry(bo_base, &vm->idle, vm_status) {
> struct amdgpu_bo *bo = bo_base->bo;
> + struct amdgpu_bo *shadow = amdgpu_bo_shadowed(bo);
>
> if (!bo->parent)
> continue;
>
> 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 (shadow)
> + ttm_bo_move_to_lru_tail(&shadow->tbo, &shadow->tbo.mem,
> &vm->lru_bulk_move);
> }
> spin_unlock(&adev->mman.bdev.lru_lock);
> @@ -692,12 +692,13 @@ int amdgpu_vm_validate_pt_bos(struct
> amdgpu_device *adev, struct amdgpu_vm *vm,
>
> list_for_each_entry_safe(bo_base, tmp, &vm->evicted, vm_status) {
> struct amdgpu_bo *bo = bo_base->bo;
> + struct amdgpu_bo *shadow = amdgpu_bo_shadowed(bo);
>
> r = validate(param, bo);
> if (r)
> return r;
> - if (bo->shadow) {
> - r = validate(param, bo->shadow);
> + if (shadow) {
> + r = validate(param, shadow);
> if (r)
> return r;
> }
> @@ -754,6 +755,7 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
> unsigned level = adev->vm_manager.root_level;
> struct amdgpu_vm_update_params params;
> struct amdgpu_bo *ancestor = bo;
> + struct amdgpu_bo *shadow;
> unsigned entries, ats_entries;
> uint64_t addr;
> int r;
> @@ -793,9 +795,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,
> - &ctx);
> + shadow = amdgpu_bo_shadowed(bo);
> + if (shadow) {
> + r = ttm_bo_validate(&shadow->tbo, &shadow->placement, &ctx);
> if (r)
> return r;
> }
> @@ -863,14 +865,16 @@ 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 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,41 @@ 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;
>
> - if (vm->is_compute_context && (adev->flags & AMD_IS_APU))
> + bo = &(*vmbo)->bo;
> + if (vm->is_compute_context && (adev->flags & AMD_IS_APU)) {
> + (*vmbo)->shadow = NULL;
> 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, &(*vmbo)->shadow);
> +
> + if (!resv)
> + dma_resv_unlock(bo->tbo.base.resv);
>
> if (r) {
> - amdgpu_bo_unref(bo);
> + amdgpu_bo_unref(&bo);
> return r;
> }
>
> + (*vmbo)->shadow->parent = amdgpu_bo_ref(bo);
> + amdgpu_bo_add_to_shadow_list((*vmbo)->shadow);
> +
> return 0;
> }
>
> @@ -933,7 +952,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
> +977,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 +989,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;
> }
>
> @@ -979,10 +1000,13 @@ static int amdgpu_vm_alloc_pts(struct amdgpu_device
> *adev,
> */
> static void amdgpu_vm_free_table(struct amdgpu_vm_pt *entry)
> {
> + struct amdgpu_bo *shadow;
> +
> if (entry->base.bo) {
> + shadow = amdgpu_bo_shadowed(entry->base.bo);
> entry->base.bo->vm_bo = NULL;
> list_del(&entry->base.vm_status);
> - amdgpu_bo_unref(&entry->base.bo->shadow);
> + amdgpu_bo_unref(&shadow);
> amdgpu_bo_unref(&entry->base.bo);
> }
> kvfree(entry->entries);
> @@ -2674,7 +2698,7 @@ 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 && (amdgpu_bo_shadowed(bo->parent) == bo))
> bo = bo->parent;
>
> for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) { @@
> -2843,7 +2867,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 +2922,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 +2960,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 +3103,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..c9ef025c43f4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> @@ -41,10 +41,7 @@ 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);
> -
> - return r;
> + return amdgpu_ttm_alloc_gart(&to_amdgpu_bo_vm(table)->shadow->tbo);
Here you also need to check if shadow isn't NULL.
I think it would be simpler if you make the parameter to
amdgpu_vm_sdma_map_table a vmbo in the first place.
> }
>
> /**
> @@ -201,17 +198,20 @@ static int amdgpu_vm_sdma_update(struct
> amdgpu_vm_update_params *p,
> uint64_t addr, unsigned count, uint32_t incr,
> uint64_t flags)
> {
> + struct amdgpu_bo *shadow;
> enum amdgpu_ib_pool_type pool = p->immediate ? AMDGPU_IB_POOL_IMMEDIATE
> : AMDGPU_IB_POOL_DELAYED;
> unsigned int i, ndw, nptes;
> uint64_t *pte;
> int r;
>
> + shadow = amdgpu_bo_shadowed(bo);
Same for amdgpu_vm_sdma_map_table(), just make the parameter a vmbo in the
first place.
Apart from that patch looks good to me.
Christian.
> /* Wait for PD/PT moves to be completed */
> r = amdgpu_sync_fence(&p->job->sync, bo->tbo.moving);
> if (r)
> return r;
>
> +
> do {
> ndw = p->num_dw_left;
> ndw -= p->job->ibs->length_dw;
> @@ -238,8 +238,8 @@ 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 (shadow)
> + amdgpu_vm_sdma_set_ptes(p, shadow, pe, addr,
> count, incr, flags);
> amdgpu_vm_sdma_set_ptes(p, bo, pe, addr, count,
> incr, flags);
> @@ -248,7 +248,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);
> + (shadow ? 2 : 1);
>
> /* for padding */
> ndw -= 7;
> @@ -263,8 +263,8 @@ 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 (shadow)
> + amdgpu_vm_sdma_copy_ptes(p, 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
