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).
> 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.
>
>
