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

[PATCH v3 1/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 f195899..c0ebbd6 100644
--- a/include/drm/ttm/ttm_bo_api.h
+++ b/include/drm/ttm/ttm_bo_api.h
@@ -476,7 +476,11 @@ size_t ttm_bo_dma_acc_size(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 v3 2/3] drm/ttm: add ttm_bo_init_reserved

[Nicolai Hähnle]

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

This variant of ttm_bo_init returns the validated buffer object with
the reservation lock held when resv == NULL. This is convenient for
callers that want to use the BO immediately, e.g. for initializing its
contents.

Signed-off-by: Nicolai Hähnle <nicolai.haehnle@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 59 +++++++++++++++++++++++++++++++++-----------
 include/drm/ttm/ttm_bo_api.h | 54 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 98 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index c438b04..989b98b 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -1119,18 +1119,18 @@ 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_reserved(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 ret = 0;
 	unsigned long num_pages;
@@ -1214,10 +1214,10 @@ int ttm_bo_init(struct ttm_bo_device *bdev,
 	if (likely(!ret))
 		ret = ttm_bo_validate(bo, placement, interruptible, false);
 
-	if (!resv)
-		ttm_bo_unreserve(bo);
-
 	if (unlikely(ret)) {
+		if (!resv)
+			ttm_bo_unreserve(bo);
+
 		ttm_bo_unref(&bo);
 		return ret;
 	}
@@ -1230,6 +1230,35 @@ int ttm_bo_init(struct ttm_bo_device *bdev,
 
 	return ret;
 }
+EXPORT_SYMBOL(ttm_bo_init_reserved);
+
+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 ret;
+
+	ret = ttm_bo_init_reserved(bdev, bo, size, type, placement,
+				   page_alignment, interruptible,
+				   persistent_swap_storage, acc_size,
+				   sg, resv, destroy);
+	if (ret)
+		return ret;
+
+	if (!resv)
+		ttm_bo_unreserve(bo);
+
+	return 0;
+}
 EXPORT_SYMBOL(ttm_bo_init);
 
 size_t ttm_bo_acc_size(struct ttm_bo_device *bdev,
diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
index c0ebbd6..2d0f63e 100644
--- a/include/drm/ttm/ttm_bo_api.h
+++ b/include/drm/ttm/ttm_bo_api.h
@@ -453,6 +453,60 @@ size_t ttm_bo_dma_acc_size(struct ttm_bo_device *bdev,
 			   unsigned struct_size);
 
 /**
+ * ttm_bo_init_reserved
+ *
+ * @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.
+ * @interruptible: If needing to sleep to wait for GPU resources,
+ * sleep interruptible.
+ * @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.
+ *
+ * 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.
+ * Furthermore, if resv == NULL, the buffer's reservation lock will be held,
+ * and it is the caller's responsibility to call ttm_bo_unreserve.
+ *
+ * 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.
+ *
+ * Returns
+ * -ENOMEM: Out of memory.
+ * -EINVAL: Invalid placement flags.
+ * -ERESTARTSYS: Interrupted by signal while sleeping waiting for resources.
+ */
+
+extern int ttm_bo_init_reserved(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 interrubtible,
+				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 v3 3/3] drm/amdgpu: simplify reservation handling during buffer creation

[Nicolai Hähnle]

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

By using ttm_bo_init_reserved instead of the manual initialization of
the reservation object, the reservation lock will be properly unlocked
and destroyed when the 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.

As an additional minor bonus, buffers created with resv == NULL and
the AMDGPU_GEM_CREATE_VRAM_CLEARED flag are now only added to the
global LRU list after the fill commands have been issued.

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 | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index c2e57f7..4d0536d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -395,19 +395,10 @@ int amdgpu_bo_create_restricted(struct amdgpu_device *adev,
 	amdgpu_fill_placement_to_bo(bo, placement);
 	/* Kernel allocation are uninterruptible */
 
-	if (!resv) {
-		bool locked;
-
-		reservation_object_init(&bo->tbo.ttm_resv);
-		locked = ww_mutex_trylock(&bo->tbo.ttm_resv.lock);
-		WARN_ON(!locked);
-	}
-
 	initial_bytes_moved = atomic64_read(&adev->num_bytes_moved);
-	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);
+	r = ttm_bo_init_reserved(&adev->mman.bdev, &bo->tbo, size, type,
+				 &bo->placement, page_align, !kernel, NULL,
+				 acc_size, sg, resv, &amdgpu_ttm_bo_destroy);
 	amdgpu_cs_report_moved_bytes(adev,
 		atomic64_read(&adev->num_bytes_moved) - initial_bytes_moved);
 
@@ -433,7 +424,7 @@ int amdgpu_bo_create_restricted(struct amdgpu_device *adev,
 		fence_put(fence);
 	}
 	if (!resv)
-		ww_mutex_unlock(&bo->tbo.resv->lock);
+		ttm_bo_unreserve(&bo->tbo);
 	*bo_ptr = bo;
 
 	trace_amdgpu_bo_create(bo);
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 3/3] drm/amdgpu: simplify reservation handling during buffer creation

[Christian König]

Am 16.02.2017 um 12:39 schrieb Nicolai Hähnle:
> From: Nicolai Hähnle <nicolai.haehnle@amd.com>
>
> By using ttm_bo_init_reserved instead of the manual initialization of
> the reservation object, the reservation lock will be properly unlocked
> and destroyed when the 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.
>
> As an additional minor bonus, buffers created with resv == NULL and
> the AMDGPU_GEM_CREATE_VRAM_CLEARED flag are now only added to the
> global LRU list after the fill commands have been issued.
>
> 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 | 17 ++++-------------
>   1 file changed, 4 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index c2e57f7..4d0536d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -395,19 +395,10 @@ int amdgpu_bo_create_restricted(struct amdgpu_device *adev,
>   	amdgpu_fill_placement_to_bo(bo, placement);
>   	/* Kernel allocation are uninterruptible */
>   
> -	if (!resv) {
> -		bool locked;
> -
> -		reservation_object_init(&bo->tbo.ttm_resv);
> -		locked = ww_mutex_trylock(&bo->tbo.ttm_resv.lock);
> -		WARN_ON(!locked);
> -	}
> -
>   	initial_bytes_moved = atomic64_read(&adev->num_bytes_moved);
> -	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);
> +	r = ttm_bo_init_reserved(&adev->mman.bdev, &bo->tbo, size, type,
> +				 &bo->placement, page_align, !kernel, NULL,
> +				 acc_size, sg, resv, &amdgpu_ttm_bo_destroy);
>   	amdgpu_cs_report_moved_bytes(adev,
>   		atomic64_read(&adev->num_bytes_moved) - initial_bytes_moved);
>   
> @@ -433,7 +424,7 @@ int amdgpu_bo_create_restricted(struct amdgpu_device *adev,
>   		fence_put(fence);
>   	}
>   	if (!resv)
> -		ww_mutex_unlock(&bo->tbo.resv->lock);
> +		ttm_bo_unreserve(&bo->tbo);

You can just use amdgpu_bo_unreserve() here. With that fixed the whole 
set is Reviewed-by: Christian König <christian.koenig@amd.com>.

Regards,
Christian.

>   	*bo_ptr = bo;
>   
>   	trace_amdgpu_bo_create(bo);


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

Re: [PATCH v3 3/3] drm/amdgpu: simplify reservation handling during buffer creation

[zhoucm1]

On 2017年02月16日 20:08, Christian König wrote:
> Am 16.02.2017 um 12:39 schrieb Nicolai Hähnle:
>> From: Nicolai Hähnle <nicolai.haehnle@amd.com>
>>
>> By using ttm_bo_init_reserved instead of the manual initialization of
>> the reservation object, the reservation lock will be properly unlocked
>> and destroyed when the 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.
>>
>> As an additional minor bonus, buffers created with resv == NULL and
>> the AMDGPU_GEM_CREATE_VRAM_CLEARED flag are now only added to the
>> global LRU list after the fill commands have been issued.
>>
>> Fixes: 12a852219583 ("drm/amdgpu: improve 
>> AMDGPU_GEM_CREATE_VRAM_CLEARED handling (v2)")
>> Signed-off-by: Nicolai Hähnle <nicolai.haehnle@amd.com>
Reviewed-by: Chunming Zhou <david1.zhou@amd.com> as well

>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 17 ++++-------------
>>   1 file changed, 4 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> index c2e57f7..4d0536d 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> @@ -395,19 +395,10 @@ int amdgpu_bo_create_restricted(struct 
>> amdgpu_device *adev,
>>       amdgpu_fill_placement_to_bo(bo, placement);
>>       /* Kernel allocation are uninterruptible */
>>   -    if (!resv) {
>> -        bool locked;
>> -
>> -        reservation_object_init(&bo->tbo.ttm_resv);
>> -        locked = ww_mutex_trylock(&bo->tbo.ttm_resv.lock);
>> -        WARN_ON(!locked);
>> -    }
>> -
>>       initial_bytes_moved = atomic64_read(&adev->num_bytes_moved);
>> -    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);
>> +    r = ttm_bo_init_reserved(&adev->mman.bdev, &bo->tbo, size, type,
>> +                 &bo->placement, page_align, !kernel, NULL,
>> +                 acc_size, sg, resv, &amdgpu_ttm_bo_destroy);
>>       amdgpu_cs_report_moved_bytes(adev,
>>           atomic64_read(&adev->num_bytes_moved) - initial_bytes_moved);
>>   @@ -433,7 +424,7 @@ int amdgpu_bo_create_restricted(struct 
>> amdgpu_device *adev,
>>           fence_put(fence);
>>       }
>>       if (!resv)
>> -        ww_mutex_unlock(&bo->tbo.resv->lock);
>> +        ttm_bo_unreserve(&bo->tbo);
>
> You can just use amdgpu_bo_unreserve() here. With that fixed the whole 
> set is Reviewed-by: Christian König <christian.koenig@amd.com>.
>
> Regards,
> Christian.
>
>>       *bo_ptr = bo;
>>         trace_amdgpu_bo_create(bo);
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel