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

On 15.02.2017 04:16, zhoucm1 wrote:

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?

FWIW, you were right about this (and also mutex_destroy needs to be called for wu_mutex, etc.). But I'm following Christian's suggestion of having the caller use ttm_bo_unref for error cleanup, so all this error cleanup needn't be duplicated.

Cheers, Nicola