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

[zhoucm1 <david1.zhou-5C7GfCeVMHo@public.gmane.org>]

On 2017年02月15日 18:43, Nicolai Hähnle wrote:
> On 15.02.2017 04:16, zhoucm1 wrote:
>> On 2017年02月14日 18:37, Nicolai Hähnle wrote:
>>> 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>
>>> ---
>>>   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.
OK, make sense, then wait Alex to submit to staging branch and verify it.

Thanks,
David Zhou
>
> 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.
>>
>

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