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@xxxxxxx> 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@xxxxxxx>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@xxxxxxx>.
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, NicolaiRegards, 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.
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
