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

["Nicolai Hähnle" <nhaehnle-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On 14.02.2017 13:51, Christian König wrote:
> Am 14.02.2017 um 13:00 schrieb Nicolai Hähnle:
>> On 14.02.2017 11:49, Christian König wrote:
>>> Am 14.02.2017 um 11:37 schrieb Nicolai Hähnle:
>>>> From: Nicolai Hähnle <nicolai.haehnle@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@amd.com>
>>>
>>> Please squash that into your other patch. It fixes another bug, but I
>>> don't think fixing one bug just to run into another is really a good
>>> idea.
>>
>> I don't understand. I'm not aware that this patch fixes anything, it
>> just enables the subsequent fix in amdgpu in patch #2. I don't think
>> squashing those together is a good idea (one is in ttm, the other in
>> amdgpu).
>
> Ok, forget it I've messed up the different reference count.
>
> With at least initializing bo->kref and bo->destroy before returning the
> first error the patch is Reviewed-by: Christian König
> <christian.koenig@amd.com>.

Thanks. Does this apply to patches #2 and #3 as well?

Cheers,
Nicolai


>
> Regards,
> Christian.
>
>>
>>
>>> Additional to that one comment below.
>>>
>>>> ---
>>>>   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;
>>>>       }
>>>
>>> I would move those checks after all the field initializations. This way
>>> the structure has at least a valid content and we can safely use
>>> ttm_bo_unref on it.
>>
>> That feels odd to me, since the return value indicates that the buffer
>> wasn't properly initialized, but I don't feel strongly about it.
>>
>> Cheers,
>> Nicolai
>>
>>
>>>
>>> Regards,
>>> Christian.
>>>
>>>> @@ -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);
>>>>   +    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;
>>>> +
>>>> +    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.
>>>
>>>
>>
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx