[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 at 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 at 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) {