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@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> --- 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.cindex 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, NicolaiRegards, 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.hindex 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
