[PATCH 1/3] drm/ttm: split BO structure initialization into a separate function

[Nicolai Hähnle]

On 15.02.2017 04:16, zhoucm1 wrote:
> On 2017年02月14日 18:37, Nicolai Hähnle wrote:
>> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
>>
>> Allow callers to opt out of calling ttm_bo_validate immediately. This
>> allows more flexibility in how locking of the reservation object is
>> done, which is needed to fix a locking bug (destroy locked mutex)
>> in amdgpu.
>>
>> Signed-off-by: Nicolai Hähnle <nicolai.haehnle at amd.com>
>> ---
>>   drivers/gpu/drm/ttm/ttm_bo.c | 62
>> +++++++++++++++++++++++++++++---------------
>>   include/drm/ttm/ttm_bo_api.h | 45 ++++++++++++++++++++++++++++++++
>>   2 files changed, 86 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
>> index 76bee42..ce4c0f5 100644
>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>> @@ -1120,41 +1120,30 @@ int ttm_bo_validate(struct ttm_buffer_object *bo,
>>   }
>>   EXPORT_SYMBOL(ttm_bo_validate);
>>   -int ttm_bo_init(struct ttm_bo_device *bdev,
>> -        struct ttm_buffer_object *bo,
>> -        unsigned long size,
>> -        enum ttm_bo_type type,
>> -        struct ttm_placement *placement,
>> -        uint32_t page_alignment,
>> -        bool interruptible,
>> -        struct file *persistent_swap_storage,
>> -        size_t acc_size,
>> -        struct sg_table *sg,
>> -        struct reservation_object *resv,
>> -        void (*destroy) (struct ttm_buffer_object *))
>> +int ttm_bo_init_top(struct ttm_bo_device *bdev,
>> +            struct ttm_buffer_object *bo,
>> +            unsigned long size,
>> +            enum ttm_bo_type type,
>> +            uint32_t page_alignment,
>> +            struct file *persistent_swap_storage,
>> +            size_t acc_size,
>> +            struct sg_table *sg,
>> +            struct reservation_object *resv,
>> +            void (*destroy) (struct ttm_buffer_object *))
>>   {
>>       int ret = 0;
>>       unsigned long num_pages;
>>       struct ttm_mem_global *mem_glob = bdev->glob->mem_glob;
>> -    bool locked;
>>         ret = ttm_mem_global_alloc(mem_glob, acc_size, false, false);
>>       if (ret) {
>>           pr_err("Out of kernel memory\n");
>> -        if (destroy)
>> -            (*destroy)(bo);
>> -        else
>> -            kfree(bo);
>>           return -ENOMEM;
>>       }
>>         num_pages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
>>       if (num_pages == 0) {
>>           pr_err("Illegal buffer object size\n");
>> -        if (destroy)
>> -            (*destroy)(bo);
>> -        else
>> -            kfree(bo);
>>           ttm_mem_global_free(mem_glob, acc_size);
>>           return -EINVAL;
>>       }
>> @@ -1204,6 +1193,37 @@ int ttm_bo_init(struct ttm_bo_device *bdev,
>>           ret = drm_vma_offset_add(&bdev->vma_manager, &bo->vma_node,
>>                        bo->mem.num_pages);
> if (ret && !resv), we should call
> reservation_object_fini(&bo->ttm_resv), right?

Good point, though actually, ret can never be != 0 here, so this can be 
simplified a bit.


>>   +    return ret;
>> +}
>> +EXPORT_SYMBOL(ttm_bo_init_top);
>> +
>> +int ttm_bo_init(struct ttm_bo_device *bdev,
>> +        struct ttm_buffer_object *bo,
>> +        unsigned long size,
>> +        enum ttm_bo_type type,
>> +        struct ttm_placement *placement,
>> +        uint32_t page_alignment,
>> +        bool interruptible,
>> +        struct file *persistent_swap_storage,
>> +        size_t acc_size,
>> +        struct sg_table *sg,
>> +        struct reservation_object *resv,
>> +        void (*destroy) (struct ttm_buffer_object *))
>> +{
>> +    bool locked;
>> +    int ret;
>> +
> Can we lock resv anyway before ttm_bo_init_top like what you did in
> patch #3? if yes, seems we don't need patch#3 any more, right?
>
>
>         if (!resv) {
>                 bool locked;
>
>                 reservation_object_init(&bo->tbo.ttm_resv);
>                 locked = ww_mutex_trylock(&bo->tbo.ttm_resv.lock);
>                 WARN_ON(!locked);
>         }
>         r = ttm_bo_init_top(&adev->mman.bdev, &bo->tbo, size, type,
>                             page_align, NULL,
>                             acc_size, sg, resv ? resv : &bo->tbo.ttm_resv,
>                             &amdgpu_ttm_bo_destroy);

No, because there's still different behavior when it comes to unlocking. 
There are three different behaviors that are needed:

1. resv == NULL, and leaving ttm_bo_init with the BO unreserved.

2. resv != NULL, and not changing the reserved status during initialization.

3. resv != NULL, and leaving initialization with the BO reserved, but 
unlocking when the BO is destroyed.

1+2 can be expressed well with the ttm_bo_init interface, but 3 cannot.

I think a possible alternative would be to change ttm_bo_init so that it 
always returns on success with the BO reserved, but that would require 
changing all the drivers, and I don't really see the advantage over just 
being more explicit in each driver.

Cheers,
Nicolai

>
> Regards,
> David Zhou
>> +    ret = ttm_bo_init_top(bdev, bo, size, type, page_alignment,
>> +                  persistent_swap_storage, acc_size, sg, resv,
>> +                  destroy);
>> +    if (ret) {
>> +        if (destroy)
>> +            (*destroy)(bo);
>> +        else
>> +            kfree(bo);
>> +        return ret;
>> +    }
>> +
>>       /* passed reservation objects should already be locked,
>>        * since otherwise lockdep will be angered in radeon.
>>        */
>> diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
>> index f195899..d44b8e4 100644
>> --- a/include/drm/ttm/ttm_bo_api.h
>> +++ b/include/drm/ttm/ttm_bo_api.h
>> @@ -453,6 +453,51 @@ size_t ttm_bo_dma_acc_size(struct ttm_bo_device
>> *bdev,
>>                  unsigned struct_size);
>>     /**
>> + * ttm_bo_init_top
>> + *
>> + * @bdev: Pointer to a ttm_bo_device struct.
>> + * @bo: Pointer to a ttm_buffer_object to be initialized.
>> + * @size: Requested size of buffer object.
>> + * @type: Requested type of buffer object.
>> + * @flags: Initial placement flags.
>> + * @page_alignment: Data alignment in pages.
>> + * @persistent_swap_storage: Usually the swap storage is deleted for
>> buffers
>> + * pinned in physical memory. If this behaviour is not desired, this
>> member
>> + * holds a pointer to a persistent shmem object. Typically, this would
>> + * point to the shmem object backing a GEM object if TTM is used to
>> back a
>> + * GEM user interface.
>> + * @acc_size: Accounted size for this object.
>> + * @resv: Pointer to a reservation_object, or NULL to let ttm
>> allocate one.
>> + * @destroy: Destroy function. Use NULL for kfree().
>> + *
>> + * This function initializes a pre-allocated struct ttm_buffer_object.
>> + * As this object may be part of a larger structure, this function,
>> + * together with the @destroy function,
>> + * enables driver-specific objects derived from a ttm_buffer_object.
>> + *
>> + * Unlike ttm_bo_init, @bo is not validated, and when an error is
>> returned,
>> + * the caller is responsible for freeing @bo (but the setup performed by
>> + * ttm_bo_init_top itself is cleaned up).
>> + *
>> + * On successful return, the object kref and list_kref are set to 1.
>> + *
>> + * Returns
>> + * -ENOMEM: Out of memory.
>> + * -EINVAL: Invalid buffer size.
>> + */
>> +
>> +extern int ttm_bo_init_top(struct ttm_bo_device *bdev,
>> +               struct ttm_buffer_object *bo,
>> +               unsigned long size,
>> +               enum ttm_bo_type type,
>> +               uint32_t page_alignment,
>> +               struct file *persistent_swap_storage,
>> +               size_t acc_size,
>> +               struct sg_table *sg,
>> +               struct reservation_object *resv,
>> +               void (*destroy) (struct ttm_buffer_object *));
>> +
>> +/**
>>    * ttm_bo_init
>>    *
>>    * @bdev: Pointer to a ttm_bo_device struct.
>