[PATCH 1/2] dma-buf/sync_file: hold reference to fence when creating sync_file

[PATCH 1/2] dma-buf/sync_file: hold reference to fence when creating sync_file

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

fence referencing was out of balance. It was not taking any ref to the
fence at creating time, but it was putting a reference when freeing the
sync file.

This patch fixes the balancing issue by getting a reference for the fence
when creating the sync_file.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/dma-buf/sync_file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index b29a9e8..235f8ac 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -79,7 +79,7 @@ struct sync_file *sync_file_create(struct fence *fence)
 	if (!sync_file)
 		return NULL;
 
-	sync_file->fence = fence;
+	sync_file->fence = fence_get(fence);
 
 	snprintf(sync_file->name, sizeof(sync_file->name), "%s-%s%llu-%d",
 		 fence->ops->get_driver_name(fence),
-- 
2.5.5

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

[PATCH 2/2] dma-buf/fence-array: hold fences reference when creating an array

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

When creating fence arrays we were not holding references to the fences
in the array, however when destroy the array we were putting away a
reference to these fences.

This patch hold the ref for all fences in the array when creating the
array.

It then removes the code that was holding the fences on both amdgpu_vm and
sync_file. For sync_file, specially, we worked on small referencing
refactor for sync_file_merge().

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/dma-buf/fence-array.c          |  8 ++++----
 drivers/dma-buf/sync_file.c            | 14 +++-----------
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c |  3 ---
 3 files changed, 7 insertions(+), 18 deletions(-)

diff --git a/drivers/dma-buf/fence-array.c b/drivers/dma-buf/fence-array.c
index f1989fc..598737f 100644
--- a/drivers/dma-buf/fence-array.c
+++ b/drivers/dma-buf/fence-array.c
@@ -112,10 +112,6 @@ EXPORT_SYMBOL(fence_array_ops);
  * Allocate a fence_array object and initialize the base fence with fence_init().
  * In case of error it returns NULL.
  *
- * The caller should allocate the fences array with num_fences size
- * and fill it with the fences it wants to add to the object. Ownership of this
- * array is taken and fence_put() is used on each fence on release.
- *
  * If @signal_on_any is true the fence array signals if any fence in the array
  * signals, otherwise it signals when all fences in the array signal.
  */
@@ -125,6 +121,7 @@ struct fence_array *fence_array_create(int num_fences, struct fence **fences,
 {
 	struct fence_array *array;
 	size_t size = sizeof(*array);
+	int i;
 
 	/* Allocate the callback structures behind the array. */
 	size += num_fences * sizeof(struct fence_array_cb);
@@ -140,6 +137,9 @@ struct fence_array *fence_array_create(int num_fences, struct fence **fences,
 	atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences);
 	array->fences = fences;
 
+	for (i = 0; i < array->num_fences; ++i)
+		fence_get(array->fences[i]);
+
 	return array;
 }
 EXPORT_SYMBOL(fence_array_create);
diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index 235f8ac..678baaf 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -142,14 +142,8 @@ static int sync_file_set_fence(struct sync_file *sync_file,
 {
 	struct fence_array *array;
 
-	/*
-	 * The reference for the fences in the new sync_file and held
-	 * in add_fence() during the merge procedure, so for num_fences == 1
-	 * we already own a new reference to the fence. For num_fence > 1
-	 * we own the reference of the fence_array creation.
-	 */
 	if (num_fences == 1) {
-		sync_file->fence = fences[0];
+		sync_file->fence = fence_get(fences[0]);
 		kfree(fences);
 	} else {
 		array = fence_array_create(num_fences, fences,
@@ -180,10 +174,8 @@ static void add_fence(struct fence **fences, int *i, struct fence *fence)
 {
 	fences[*i] = fence;
 
-	if (!fence_is_signaled(fence)) {
-		fence_get(fence);
+	if (!fence_is_signaled(fence))
 		(*i)++;
-	}
 }
 
 /**
@@ -255,7 +247,7 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
 		add_fence(fences, &i, b_fences[i_b]);
 
 	if (i == 0)
-		fences[i++] = fence_get(a_fences[0]);
+		fences[i++] = a_fences[0];
 
 	if (num_fences > i) {
 		nfences = krealloc(fences, i * sizeof(*fences),
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index bc4b22c..4ee7988 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -228,9 +228,6 @@ int amdgpu_vm_grab_id(struct amdgpu_vm *vm, struct amdgpu_ring *ring,
 		struct fence_array *array;
 		unsigned j;
 
-		for (j = 0; j < i; ++j)
-			fence_get(fences[j]);
-
 		array = fence_array_create(i, fences, fence_context,
 					   seqno, true);
 		if (!array) {
-- 
2.5.5

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

Re: [PATCH 2/2] dma-buf/fence-array: hold fences reference when creating an array

[Christian König]

Am 19.10.2016 um 19:48 schrieb Gustavo Padovan:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>
> When creating fence arrays we were not holding references to the fences
> in the array, however when destroy the array we were putting away a
> reference to these fences.
>
> This patch hold the ref for all fences in the array when creating the
> array.
>
> It then removes the code that was holding the fences on both amdgpu_vm and
> sync_file. For sync_file, specially, we worked on small referencing
> refactor for sync_file_merge().
>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

I would prefer it to keep it like it is, cause this is a bit inconsistent.

With this change the fence array takes the ownership of the array, but 
not of the fences inside it.

> ---
>   drivers/dma-buf/fence-array.c          |  8 ++++----
>   drivers/dma-buf/sync_file.c            | 14 +++-----------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c |  3 ---
>   3 files changed, 7 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/dma-buf/fence-array.c b/drivers/dma-buf/fence-array.c
> index f1989fc..598737f 100644
> --- a/drivers/dma-buf/fence-array.c
> +++ b/drivers/dma-buf/fence-array.c
> @@ -112,10 +112,6 @@ EXPORT_SYMBOL(fence_array_ops);
>    * Allocate a fence_array object and initialize the base fence with fence_init().
>    * In case of error it returns NULL.
>    *
> - * The caller should allocate the fences array with num_fences size
> - * and fill it with the fences it wants to add to the object. Ownership of this
> - * array is taken and fence_put() is used on each fence on release.
> - *

At bare minimum you should keep this comment, cause ownership of the 
array is still taken and so it is released in the destructor.

Christian.

>    * If @signal_on_any is true the fence array signals if any fence in the array
>    * signals, otherwise it signals when all fences in the array signal.
>    */
> @@ -125,6 +121,7 @@ struct fence_array *fence_array_create(int num_fences, struct fence **fences,
>   {
>   	struct fence_array *array;
>   	size_t size = sizeof(*array);
> +	int i;
>   
>   	/* Allocate the callback structures behind the array. */
>   	size += num_fences * sizeof(struct fence_array_cb);
> @@ -140,6 +137,9 @@ struct fence_array *fence_array_create(int num_fences, struct fence **fences,
>   	atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences);
>   	array->fences = fences;
>   
> +	for (i = 0; i < array->num_fences; ++i)
> +		fence_get(array->fences[i]);
> +
>   	return array;
>   }
>   EXPORT_SYMBOL(fence_array_create);
> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> index 235f8ac..678baaf 100644
> --- a/drivers/dma-buf/sync_file.c
> +++ b/drivers/dma-buf/sync_file.c
> @@ -142,14 +142,8 @@ static int sync_file_set_fence(struct sync_file *sync_file,
>   {
>   	struct fence_array *array;
>   
> -	/*
> -	 * The reference for the fences in the new sync_file and held
> -	 * in add_fence() during the merge procedure, so for num_fences == 1
> -	 * we already own a new reference to the fence. For num_fence > 1
> -	 * we own the reference of the fence_array creation.
> -	 */
>   	if (num_fences == 1) {
> -		sync_file->fence = fences[0];
> +		sync_file->fence = fence_get(fences[0]);
>   		kfree(fences);
>   	} else {
>   		array = fence_array_create(num_fences, fences,
> @@ -180,10 +174,8 @@ static void add_fence(struct fence **fences, int *i, struct fence *fence)
>   {
>   	fences[*i] = fence;
>   
> -	if (!fence_is_signaled(fence)) {
> -		fence_get(fence);
> +	if (!fence_is_signaled(fence))
>   		(*i)++;
> -	}
>   }
>   
>   /**
> @@ -255,7 +247,7 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
>   		add_fence(fences, &i, b_fences[i_b]);
>   
>   	if (i == 0)
> -		fences[i++] = fence_get(a_fences[0]);
> +		fences[i++] = a_fences[0];
>   
>   	if (num_fences > i) {
>   		nfences = krealloc(fences, i * sizeof(*fences),
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index bc4b22c..4ee7988 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -228,9 +228,6 @@ int amdgpu_vm_grab_id(struct amdgpu_vm *vm, struct amdgpu_ring *ring,
>   		struct fence_array *array;
>   		unsigned j;
>   
> -		for (j = 0; j < i; ++j)
> -			fence_get(fences[j]);
> -
>   		array = fence_array_create(i, fences, fence_context,
>   					   seqno, true);
>   		if (!array) {


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

Re: [PATCH 2/2] dma-buf/fence-array: hold fences reference when creating an array

[Gustavo Padovan]

2016-10-19 Christian König <deathsimple@vodafone.de>:

> Am 19.10.2016 um 19:48 schrieb Gustavo Padovan:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > When creating fence arrays we were not holding references to the fences
> > in the array, however when destroy the array we were putting away a
> > reference to these fences.
> > 
> > This patch hold the ref for all fences in the array when creating the
> > array.
> > 
> > It then removes the code that was holding the fences on both amdgpu_vm and
> > sync_file. For sync_file, specially, we worked on small referencing
> > refactor for sync_file_merge().
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> I would prefer it to keep it like it is, cause this is a bit inconsistent.
> 
> With this change the fence array takes the ownership of the array, but not
> of the fences inside it.

I was thinking more in to keep consistency between all fence users. Every
user should hold a ref to the fence assigned to it. That is what patch
1 is doing for sync_file and think it is a good idea do the same here.

The array itself is not refcounted and the users calling
fence_array_create() doesn't store the allocated array anywhere. The
comment I errouneously removed already states that.

Gustavo

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

Re: [PATCH 2/2] dma-buf/fence-array: hold fences reference when creating an array

[Christian König]

Am 19.10.2016 um 20:35 schrieb Gustavo Padovan:
> 2016-10-19 Christian König <deathsimple@vodafone.de>:
>
>> Am 19.10.2016 um 19:48 schrieb Gustavo Padovan:
>>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>>
>>> When creating fence arrays we were not holding references to the fences
>>> in the array, however when destroy the array we were putting away a
>>> reference to these fences.
>>>
>>> This patch hold the ref for all fences in the array when creating the
>>> array.
>>>
>>> It then removes the code that was holding the fences on both amdgpu_vm and
>>> sync_file. For sync_file, specially, we worked on small referencing
>>> refactor for sync_file_merge().
>>>
>>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>> I would prefer it to keep it like it is, cause this is a bit inconsistent.
>>
>> With this change the fence array takes the ownership of the array, but not
>> of the fences inside it.
> I was thinking more in to keep consistency between all fence users. Every
> user should hold a ref to the fence assigned to it. That is what patch
> 1 is doing for sync_file and think it is a good idea do the same here.

This might make the code easier to follow, but isn't necessary a good idea.

Usually with reference counted objects you increase the count every time 
the pointer to the object is assigned to a container. E.g. member of a 
larger structure or in this case an array of pointers.

>
> The array itself is not refcounted and the users calling
> fence_array_create() doesn't store the allocated array anywhere. The
> comment I errouneously removed already states that.

And exactly that's the point here. The array is the container for the 
pointers referencing the objects, since you give the ownership of this 
container to the fence_array object it is now responsible for releasing 
that reference before it releases the array.

This is good coding practice as far as I know.

Regards,
Christian.

>
> Gustavo
>

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

Re: [PATCH 2/2] dma-buf/fence-array: hold fences reference when creating an array

[Gustavo Padovan]

2016-10-20 Christian König <deathsimple@vodafone.de>:

> Am 19.10.2016 um 20:35 schrieb Gustavo Padovan:
> > 2016-10-19 Christian König <deathsimple@vodafone.de>:
> > 
> > > Am 19.10.2016 um 19:48 schrieb Gustavo Padovan:
> > > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > > 
> > > > When creating fence arrays we were not holding references to the fences
> > > > in the array, however when destroy the array we were putting away a
> > > > reference to these fences.
> > > > 
> > > > This patch hold the ref for all fences in the array when creating the
> > > > array.
> > > > 
> > > > It then removes the code that was holding the fences on both amdgpu_vm and
> > > > sync_file. For sync_file, specially, we worked on small referencing
> > > > refactor for sync_file_merge().
> > > > 
> > > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > I would prefer it to keep it like it is, cause this is a bit inconsistent.
> > > 
> > > With this change the fence array takes the ownership of the array, but not
> > > of the fences inside it.
> > I was thinking more in to keep consistency between all fence users. Every
> > user should hold a ref to the fence assigned to it. That is what patch
> > 1 is doing for sync_file and think it is a good idea do the same here.
> 
> This might make the code easier to follow, but isn't necessary a good idea.
> 
> Usually with reference counted objects you increase the count every time the
> pointer to the object is assigned to a container. E.g. member of a larger
> structure or in this case an array of pointers.
> 
> > 
> > The array itself is not refcounted and the users calling
> > fence_array_create() doesn't store the allocated array anywhere. The
> > comment I errouneously removed already states that.
> 
> And exactly that's the point here. The array is the container for the
> pointers referencing the objects, since you give the ownership of this
> container to the fence_array object it is now responsible for releasing that
> reference before it releases the array.
> 
> This is good coding practice as far as I know.

Right, this makes sense. Let's keep this as is then.

Gustavo

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

Re: [PATCH 1/2] dma-buf/sync_file: hold reference to fence when creating sync_file

[Sean Paul]

On Wed, Oct 19, 2016 at 1:48 PM, Gustavo Padovan <gustavo@padovan.org> wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>
> fence referencing was out of balance. It was not taking any ref to the
> fence at creating time, but it was putting a reference when freeing the
> sync file.
>
> This patch fixes the balancing issue by getting a reference for the fence
> when creating the sync_file.
>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>


Applied to drm-misc, thanks

> ---
>  drivers/dma-buf/sync_file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> index b29a9e8..235f8ac 100644
> --- a/drivers/dma-buf/sync_file.c
> +++ b/drivers/dma-buf/sync_file.c
> @@ -79,7 +79,7 @@ struct sync_file *sync_file_create(struct fence *fence)
>         if (!sync_file)
>                 return NULL;
>
> -       sync_file->fence = fence;
> +       sync_file->fence = fence_get(fence);
>
>         snprintf(sync_file->name, sizeof(sync_file->name), "%s-%s%llu-%d",
>                  fence->ops->get_driver_name(fence),
> --
> 2.5.5
>
> _______________________________________________
> 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