Re: [PATCH v2 1/2] drm/amdgpu: parameterize ttm BO destroy callback

["Das, Nirmoy" <nirmoy.das@xxxxxxx>]

On 6/15/2021 12:48 PM, Christian König wrote:
> Am 15.06.21 um 11:23 schrieb Nirmoy Das:
> > Make provision to pass different ttm BO destroy callback
> > while creating a amdgpu_bo.
> >
> > v2: remove whitespace.
> >      call amdgpu_bo_destroy_base() at the end for cleaner code.
> >
> > Signed-off-by: Nirmoy Das <nirmoy.das@xxxxxxx>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 48 ++++++++++++++++------
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  3 +-
> >   2 files changed, 38 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > index 9092ac12a270..8473d153650f 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > @@ -73,11 +73,9 @@ static void amdgpu_bo_subtract_pin_size(struct 
> > amdgpu_bo *bo)
> >       }
> >   }
> >
> > -static void amdgpu_bo_destroy(struct ttm_buffer_object *tbo)
> > +static void amdgpu_bo_destroy_base(struct ttm_buffer_object *tbo)
>
> I think you don't even need to rename the function.
>
> >   {
> > -    struct amdgpu_device *adev = amdgpu_ttm_adev(tbo->bdev);
> >       struct amdgpu_bo *bo = ttm_to_amdgpu_bo(tbo);
> > -    struct amdgpu_bo_user *ubo;
> >
> >       if (bo->tbo.pin_count > 0)
> >           amdgpu_bo_subtract_pin_size(bo);
> > @@ -87,20 +85,38 @@ static void amdgpu_bo_destroy(struct 
> > ttm_buffer_object *tbo)
> >       if (bo->tbo.base.import_attach)
> >           drm_prime_gem_destroy(&bo->tbo.base, bo->tbo.sg);
> >       drm_gem_object_release(&bo->tbo.base);
> > +    amdgpu_bo_unref(&bo->parent);
> > +    kvfree(bo);
> > +}
> > +
> > +static void amdgpu_bo_destroy(struct ttm_buffer_object *tbo)
> > +{
> > +    amdgpu_bo_destroy_base(tbo);
> > +}
>
> Nor has that wrapper here.
>
> Apart from that looks good to me.


Thanks, let me resend with that.


Nirmoy

> Christian.
>
> > +
> > +static void amdgpu_bo_user_destroy(struct ttm_buffer_object *tbo)
> > +{
> > +    struct amdgpu_bo *bo = ttm_to_amdgpu_bo(tbo);
> > +    struct amdgpu_bo_user *ubo;
> > +
> > +    ubo = to_amdgpu_bo_user(bo);
> > +    kfree(ubo->metadata);
> > +    amdgpu_bo_destroy_base(tbo);
> > +}
> > +
> > +static void amdgpu_bo_vm_destroy(struct ttm_buffer_object *tbo)
> > +{
> > +    struct amdgpu_device *adev = amdgpu_ttm_adev(tbo->bdev);
> > +    struct amdgpu_bo *bo = ttm_to_amdgpu_bo(tbo);
> > +
> >       /* in case amdgpu_device_recover_vram got NULL of bo->parent */
> >       if (!list_empty(&bo->shadow_list)) {
> >           mutex_lock(&adev->shadow_list_lock);
> >           list_del_init(&bo->shadow_list);
> >           mutex_unlock(&adev->shadow_list_lock);
> >       }
> > -    amdgpu_bo_unref(&bo->parent);
> > -
> > -    if (bo->tbo.type != ttm_bo_type_kernel) {
> > -        ubo = to_amdgpu_bo_user(bo);
> > -        kfree(ubo->metadata);
> > -    }
> >
> > -    kvfree(bo);
> > +    amdgpu_bo_destroy_base(tbo);
> >   }
> >
> >   /**
> > @@ -115,8 +131,11 @@ static void amdgpu_bo_destroy(struct 
> > ttm_buffer_object *tbo)
> >    */
> >   bool amdgpu_bo_is_amdgpu_bo(struct ttm_buffer_object *bo)
> >   {
> > -    if (bo->destroy == &amdgpu_bo_destroy)
> > +    if (bo->destroy == &amdgpu_bo_destroy ||
> > +        bo->destroy == &amdgpu_bo_user_destroy ||
> > +        bo->destroy == &amdgpu_bo_vm_destroy)
> >           return true;
> > +
> >       return false;
> >   }
> >
> > @@ -592,9 +611,12 @@ int amdgpu_bo_create(struct amdgpu_device *adev,
> >       if (bp->type == ttm_bo_type_kernel)
> >           bo->tbo.priority = 1;
> >
> > +    if (!bp->destroy)
> > +        bp->destroy = &amdgpu_bo_destroy;
> > +
> >       r = ttm_bo_init_reserved(&adev->mman.bdev, &bo->tbo, size, 
> > bp->type,
> >                    &bo->placement, page_align, &ctx, NULL,
> > -                 bp->resv, &amdgpu_bo_destroy);
> > +                 bp->resv, bp->destroy);
> >       if (unlikely(r != 0))
> >           return r;
> >
> > @@ -658,6 +680,7 @@ int amdgpu_bo_create_user(struct amdgpu_device 
> > *adev,
> >       int r;
> >
> >       bp->bo_ptr_size = sizeof(struct amdgpu_bo_user);
> > +    bp->destroy = &amdgpu_bo_user_destroy;
> >       r = amdgpu_bo_create(adev, bp, &bo_ptr);
> >       if (r)
> >           return r;
> > @@ -689,6 +712,7 @@ int amdgpu_bo_create_vm(struct amdgpu_device *adev,
> >        * num of amdgpu_vm_pt entries.
> >        */
> >       BUG_ON(bp->bo_ptr_size < sizeof(struct amdgpu_bo_vm));
> > +    bp->destroy = &amdgpu_bo_vm_destroy;
> >       r = amdgpu_bo_create(adev, bp, &bo_ptr);
> >       if (r)
> >           return r;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> > index e36b84516b4e..a8c702634e1b 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> > @@ -55,7 +55,8 @@ struct amdgpu_bo_param {
> >       u64                flags;
> >       enum ttm_bo_type        type;
> >       bool                no_wait_gpu;
> > -    struct dma_resv    *resv;
> > +    struct dma_resv            *resv;
> > +    void                (*destroy)(struct ttm_buffer_object *bo);
> >   };
> >
> >   /* bo virtual addresses in a vm */
> > --
> > 2.31.1
> >
> > _______________________________________________
> > amd-gfx mailing list
> > amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Cnirmoy.das%40amd.com%7Cb321ddc590404256e9c808d92feb2a0a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637593509331489349%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=Y3zQrnAfnzSalGRkNRp9Ocgr4lOZZ0MtCPLJoghvD0c%3D&amp;reserved=0 
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx