[PATCH 1/3] drm/ttm: split BO structure initialization into a separate function
Christian König
deathsimple at vodafone.de
Tue Feb 14 12:51:51 UTC 2017
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 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>
>>
>> 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 at amd.com>.
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.
>>
>>
>
More information about the amd-gfx
mailing list