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

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

[Nicolai Hähnle]

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);
 
+	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.
-- 
2.9.3

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

[PATCH 2/3] drm/ttm: fix the documentation of ttm_bo_init

[Nicolai Hähnle]

From: Nicolai Hähnle <nicolai.haehnle@amd.com>

As the comment says: callers of ttm_bo_init cannot rely on having the
only reference to the BO when the function returns successfully.

Signed-off-by: Nicolai Hähnle <nicolai.haehnle@amd.com>
---
 include/drm/ttm/ttm_bo_api.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
index d44b8e4..7b2807f 100644
--- a/include/drm/ttm/ttm_bo_api.h
+++ b/include/drm/ttm/ttm_bo_api.h
@@ -521,7 +521,11 @@ extern int ttm_bo_init_top(struct ttm_bo_device *bdev,
  * 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.
- * On successful return, the object kref and list_kref are set to 1.
+ *
+ * On successful return, the caller owns an object kref to @bo. The kref and
+ * list_kref are usually set to 1, but note that in some situations, other
+ * tasks may already be holding references to @bo as well.
+ *
  * If a failure occurs, the function will call the @destroy function, or
  * kfree() if @destroy is NULL. Thus, after a failure, dereferencing @bo is
  * illegal and will likely cause memory corruption.
-- 
2.9.3

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

[PATCH 3/3] drm/amdgpu: fix lock cleanup during buffer creation

[Nicolai Hähnle]

From: Nicolai Hähnle <nicolai.haehnle@amd.com>

Open-code the initial ttm_bo_validate call, so that we can properly
unlock the reservation lock when it fails. Also, properly destruct
the reservation object when the first part of TTM BO initialization
fails.

Actual deadlocks caused by the missing unlock should have been fixed
by "drm/ttm: never add BO that failed to validate to the LRU list",
superseding the flawed fix in commit 38fc4856ad98 ("drm/amdgpu: fix
a potential deadlock in amdgpu_bo_create_restricted()").

This change fixes remaining recursive locking errors that can be seen
with lock debugging enabled, and avoids the error of freeing a locked
mutex.

Fixes: 12a852219583 ("drm/amdgpu: improve AMDGPU_GEM_CREATE_VRAM_CLEARED handling (v2)")
Signed-off-by: Nicolai Hähnle <nicolai.haehnle@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 32 +++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index d1ef1d0..bea845f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -399,12 +399,34 @@ int amdgpu_bo_create_restricted(struct amdgpu_device *adev,
 		locked = ww_mutex_trylock(&bo->tbo.ttm_resv.lock);
 		WARN_ON(!locked);
 	}
-	r = ttm_bo_init(&adev->mman.bdev, &bo->tbo, size, type,
-			&bo->placement, page_align, !kernel, NULL,
-			acc_size, sg, resv ? resv : &bo->tbo.ttm_resv,
-			&amdgpu_ttm_bo_destroy);
-	if (unlikely(r != 0))
+	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);
+	if (unlikely(r != 0)) {
+		if (!resv) {
+			ww_mutex_unlock(&bo->tbo.ttm_resv.lock);
+			reservation_object_fini(&bo->tbo.ttm_resv);
+		}
+		amdgpu_ttm_bo_destroy(&bo->tbo);
+		return r;
+	}
+
+	r = ttm_bo_validate(&bo->tbo, &bo->placement, !kernel, false);
+	if (unlikely(r != 0)) {
+		struct ttm_buffer_object *tbo = &bo->tbo;
+
+		if (!resv)
+			ww_mutex_unlock(&bo->tbo.ttm_resv.lock);
+		ttm_bo_unref(&tbo);
 		return r;
+	}
+
+	if (!(bo->tbo.mem.placement & TTM_PL_FLAG_NO_EVICT)) {
+		spin_lock(&bo->tbo.glob->lru_lock);
+		ttm_bo_add_to_lru(&bo->tbo);
+		spin_unlock(&bo->tbo.glob->lru_lock);
+	}
 
 	bo->tbo.priority = ilog2(bo->tbo.num_pages);
 	if (kernel)
-- 
2.9.3

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

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

[Christian König]

Am 14.02.2017 um 11:37 schrieb Nicolai Hähnle:
> 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>

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.

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.

Regards,
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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[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@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>
>
> 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).


> 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,
Nicolai


>
> Regards,
> 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.
>
>

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

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

[Christian König]

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@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>
>>
>> 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@amd.com>.

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,
> Nicolai
>
>
>>
>> Regards,
>> 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.
>>
>>
>

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

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

[zhoucm1]

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

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

[Nicolai Hähnle]

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?

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.

Cheers,
Nicolai

>
> 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

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

[zhoucm1]

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@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?
>
> 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,
> Nicolai
>
>>
>> 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

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

[Nicolai Hähnle]

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@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>
>>>
>>> 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@amd.com>.

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,
>> Nicolai
>>
>>
>>>
>>> Regards,
>>> 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.
>>>
>>>
>>
>

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

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

[Nicolai Hähnle]

On 15.02.2017 14:35, Nicolai Hähnle wrote:
> 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@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>
>>>>
>>>> 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@amd.com>.
>
> Thanks. Does this apply to patches #2 and #3 as well?

Well, there's some minor necessary rebase fixes, so I'll probably just 
send out a new version once I got to test it.

Nicolai

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

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

[Nicolai Hähnle]

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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx