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

[zhoucm1 <david1.zhou-5C7GfCeVMHo@public.gmane.org>]

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.

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx