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

[Nirmoy <nirmodas@xxxxxxx>]

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@xxxxxxx>
> > ---
> >   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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx