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

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?

• 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);

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.