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

[Das, Nirmoy]

On 6/15/2021 8:53 AM, Christian König wrote:
> Am 14.06.21 um 21:26 schrieb Nirmoy Das:
>> Make provision to pass different ttm BO destroy callback
>> while creating a amdgpu_bo.
>>
>> v2: pass destroy callback using amdgpu_bo_param.
>>
>> Signed-off-by: Nirmoy Das <nirmoy.das at amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 52 +++++++++++++++++-----
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  3 +-
>>   2 files changed, 42 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..f4f57a73d095 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> @@ -12,7 +12,7 @@
>>    *
>>    * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
>> EXPRESS OR
>>    * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
>> MERCHANTABILITY,
>> - * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO 
>> EVENT SHALL
>> + * FITNEsS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO 
>> EVENT SHALL
>>    * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE 
>> FOR ANY CLAIM,
>>    * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, 
>> TORT OR
>>    * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE 
>> SOFTWARE OR THE
>> @@ -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)
>>   {
>> -    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,18 +85,40 @@ 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);
>> +}
>> +
>> +static void amdgpu_bo_destroy(struct ttm_buffer_object *tbo)
>> +{
>> +    struct amdgpu_bo *bo = ttm_to_amdgpu_bo(tbo);
>> +
>> +    amdgpu_bo_destroy_base(tbo);
>> +    kvfree(bo);
>> +}
>> +
>> +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;
>> +
>> +    amdgpu_bo_destroy_base(tbo);
>> +    ubo = to_amdgpu_bo_user(bo);
>> +    kfree(ubo->metadata);
>> +    kvfree(bo);
>
> Why not freeing the metadata first and having the kvfree() in 
> amdgpu_bo_destroy()?


Didn't think about that, it would be me much cleaner.


Thanks,

Nirmoy

>
> Apart from that looks good to me.
>
> Christian.
>
>> +}
>> +
>> +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);
>> +
>> +    amdgpu_bo_destroy_base(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);
>>   }
>> @@ -115,8 +135,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 +615,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 +684,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 +716,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
>>
>