[RFC v2 0/3] dma-buf/sync_file: rework fences on struct sync_file

[RFC v2 0/3] dma-buf/sync_file: rework fences on struct sync_file

[Gustavo Padovan]

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

Hi all,

This is an attempt to improve fence support on Sync File. The basic idea
is to have only sync_file->fence and store all fences there, either as
normal fences or fence_arrays. That way we can remove some potential
duplication when using fence_array with sync_file: the duplication of the array
of fences and the duplication of fence_add_callback() for all fences.

Now when creating a new sync_file during the merge process sync_file_set_fence()
will set sync_file->fence based on the number of fences for that sync_file. If
there is more than one fence a fence_array is created. One important advantage
approach is that we only add one fence callback now, no matter how many fences
there are in a sync_file - the individual callbacks are added by fence_array.

Please review! Thanks!

	Gustavo

Changes since v1 (Comments from Chris Wilson and Christian König):
	- Not using fence_ops anymore.
	- fence_is_array() was created to differentiate fence from fence_array
	- fence_array_teardown() is now exported and used under fence_is_array()
	- struct sync_file lost num_fences member

Gustavo Padovan (3):
  dma-buf/fence-array: add fence_is_array()
  dma-buf/fence-array: add fence_array_teardown()
  dma-buf/sync_file: rework fence storage in struct file

 drivers/dma-buf/fence-array.c        |  25 ++++++
 drivers/dma-buf/sync_file.c          | 148 +++++++++++++++++++++++------------
 drivers/staging/android/sync_debug.c |   9 ++-
 include/linux/fence-array.h          |  11 +++
 include/linux/sync_file.h            |  15 ++--
 5 files changed, 147 insertions(+), 61 deletions(-)

-- 
2.5.5

[RFC v2 1/3] dma-buf/fence-array: add fence_is_array()

[Gustavo Padovan]

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

Add helper to check if fence is array.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Christian König <christian.koenig@amd.com>
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 include/linux/fence-array.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/linux/fence-array.h b/include/linux/fence-array.h
index 86baaa4..d2e9f40 100644
--- a/include/linux/fence-array.h
+++ b/include/linux/fence-array.h
@@ -52,6 +52,16 @@ struct fence_array {
 extern const struct fence_ops fence_array_ops;
 
 /**
+ * fence_is_array - check if a fence is from the array subsclass
+ *
+ * Return true if it is a fence_array and false otherwise.
+ */
+static inline bool fence_is_array(struct fence *fence)
+{
+	return (fence->ops == &fence_array_ops) ? true : false;
+}
+
+/**
  * to_fence_array - cast a fence to a fence_array
  * @fence: fence to cast to a fence_array
  *
-- 
2.5.5

[RFC v2 2/3] dma-buf/fence-array: add fence_array_teardown()

[Gustavo Padovan]

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

As the array of fence callbacks held by an active struct fence_array
each has a reference to the struct fence_array, when the owner of the
fence_array is freed it must dispose of the callback references before
it can free the fence_array. This can not happen simply during
fence_release() because of the extra references and so we need a new
function to run before the final fence_put().

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Christian König <christian.koenig@amd.com>
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/dma-buf/fence-array.c | 25 +++++++++++++++++++++++++
 include/linux/fence-array.h   |  1 +
 2 files changed, 26 insertions(+)

diff --git a/drivers/dma-buf/fence-array.c b/drivers/dma-buf/fence-array.c
index a8731c8..8891357 100644
--- a/drivers/dma-buf/fence-array.c
+++ b/drivers/dma-buf/fence-array.c
@@ -142,3 +142,28 @@ struct fence_array *fence_array_create(int num_fences, struct fence **fences,
 	return array;
 }
 EXPORT_SYMBOL(fence_array_create);
+
+/**
+ * fence_array_teardown - Teardown and clean up a fence array
+ * @array:	[in]	the fence array to teardown
+ *
+ * This function removes callbacks and extra references to the base fence on
+ * the fence_array for each unsignalled fence in the array. It should be called
+ * before calling fence_put() to remove the last reference on a fence_array,
+ * otherwise the fence won't be released due to extra references holded by the
+ * the fences that still have signalling enabled.
+ */
+void fence_array_teardown(struct fence_array *array)
+{
+	struct fence_array_cb *cb = (void *)(&array[1]);
+	int i;
+
+	for (i = 0; i < array->num_fences; i++) {
+		if (fence_is_signaled(array->fences[i]))
+		    continue;
+
+		fence_remove_callback(array->fences[i], &cb[i].cb);
+		fence_put(&array->base);
+	}
+}
+EXPORT_SYMBOL(fence_array_teardown);
diff --git a/include/linux/fence-array.h b/include/linux/fence-array.h
index d2e9f40..9f1b923 100644
--- a/include/linux/fence-array.h
+++ b/include/linux/fence-array.h
@@ -79,5 +79,6 @@ static inline struct fence_array *to_fence_array(struct fence *fence)
 struct fence_array *fence_array_create(int num_fences, struct fence **fences,
 				       u64 context, unsigned seqno,
 				       bool signal_on_any);
+void fence_array_teardown(struct fence_array *array);
 
 #endif /* __LINUX_FENCE_ARRAY_H */
-- 
2.5.5

[RFC v2 3/3] dma-buf/sync_file: rework fence storage in struct file

[Gustavo Padovan]

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

Create sync_file->fence to abstract the type of fence we are using for
each sync_file. If only one fence is present we use a normal struct fence
but if there is more fences to be added to the sync_file a fence_array
is created.

This change cleans up sync_file a bit. We don't need to have sync_file_cb
array anymore. Instead, as we always have  one fence, only one fence
callback is registered per sync_file.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Christian König <christian.koenig@amd.com>
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/dma-buf/sync_file.c          | 148 +++++++++++++++++++++++------------
 drivers/staging/android/sync_debug.c |   9 ++-
 include/linux/sync_file.h            |  15 ++--
 3 files changed, 111 insertions(+), 61 deletions(-)

diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index 9aaa608..ca19d3a 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -28,11 +28,11 @@
 
 static const struct file_operations sync_file_fops;
 
-static struct sync_file *sync_file_alloc(int size)
+static struct sync_file *sync_file_alloc(void)
 {
 	struct sync_file *sync_file;
 
-	sync_file = kzalloc(size, GFP_KERNEL);
+	sync_file = kzalloc(sizeof(*sync_file), GFP_KERNEL);
 	if (!sync_file)
 		return NULL;
 
@@ -45,6 +45,8 @@ static struct sync_file *sync_file_alloc(int size)
 
 	init_waitqueue_head(&sync_file->wq);
 
+	INIT_LIST_HEAD(&sync_file->cb.node);
+
 	return sync_file;
 
 err:
@@ -54,11 +56,9 @@ err:
 
 static void fence_check_cb_func(struct fence *f, struct fence_cb *cb)
 {
-	struct sync_file_cb *check;
 	struct sync_file *sync_file;
 
-	check = container_of(cb, struct sync_file_cb, cb);
-	sync_file = check->sync_file;
+	sync_file = container_of(cb, struct sync_file, cb);
 
 	if (atomic_dec_and_test(&sync_file->status))
 		wake_up_all(&sync_file->wq);
@@ -76,21 +76,19 @@ struct sync_file *sync_file_create(struct fence *fence)
 {
 	struct sync_file *sync_file;
 
-	sync_file = sync_file_alloc(offsetof(struct sync_file, cbs[1]));
+	sync_file = sync_file_alloc();
 	if (!sync_file)
 		return NULL;
 
-	sync_file->num_fences = 1;
+	sync_file->fence = fence;
+
 	atomic_set(&sync_file->status, 1);
 	snprintf(sync_file->name, sizeof(sync_file->name), "%s-%s%llu-%d",
 		 fence->ops->get_driver_name(fence),
 		 fence->ops->get_timeline_name(fence), fence->context,
 		 fence->seqno);
 
-	sync_file->cbs[0].fence = fence;
-	sync_file->cbs[0].sync_file = sync_file;
-	if (fence_add_callback(fence, &sync_file->cbs[0].cb,
-			       fence_check_cb_func))
+	if (fence_add_callback(fence, &sync_file->cb, fence_check_cb_func))
 		atomic_dec(&sync_file->status);
 
 	return sync_file;
@@ -121,14 +119,42 @@ err:
 	return NULL;
 }
 
-static void sync_file_add_pt(struct sync_file *sync_file, int *i,
-			     struct fence *fence)
+static int sync_file_set_fence(struct sync_file *sync_file,
+			       struct fence **fences, int num_fences)
 {
-	sync_file->cbs[*i].fence = fence;
-	sync_file->cbs[*i].sync_file = sync_file;
+	struct fence_array *array;
+
+	if (num_fences == 1) {
+		sync_file->fence = fences[0];
+	} else {
+		array = fence_array_create(num_fences, fences,
+					   fence_context_alloc(1), 1, false);
+		if (!array)
+			return -ENOMEM;
+
+		sync_file->fence = (struct fence *)array;
+	}
+
+	return 0;
+}
 
-	if (!fence_add_callback(fence, &sync_file->cbs[*i].cb,
-				fence_check_cb_func)) {
+static struct fence **get_fences(struct sync_file *sync_file, int *num_fences)
+{
+	if (fence_is_array(sync_file->fence)) {
+		struct fence_array *array = to_fence_array(sync_file->fence);
+		*num_fences = array->num_fences;
+		return array->fences;
+	} else {
+		*num_fences = 1;
+		return &sync_file->fence;
+	}
+}
+
+static void add_fence(struct fence **fences, int *i, struct fence *fence)
+{
+	fences[*i] = fence;
+
+	if (!fence_is_signaled(fence)) {
 		fence_get(fence);
 		(*i)++;
 	}
@@ -147,16 +173,23 @@ static void sync_file_add_pt(struct sync_file *sync_file, int *i,
 static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
 					 struct sync_file *b)
 {
-	int num_fences = a->num_fences + b->num_fences;
 	struct sync_file *sync_file;
-	int i, i_a, i_b;
-	unsigned long size = offsetof(struct sync_file, cbs[num_fences]);
+	struct fence **fences, **a_fences, **b_fences;
+	int i, i_a, i_b, num_fences, a_num_fences, b_num_fences;
 
-	sync_file = sync_file_alloc(size);
+	sync_file = sync_file_alloc();
 	if (!sync_file)
 		return NULL;
 
-	atomic_set(&sync_file->status, num_fences);
+	a_fences = get_fences(a, &a_num_fences);
+	b_fences = get_fences(b, &b_num_fences);
+	num_fences = a_num_fences + b_num_fences;
+
+	fences = kcalloc(num_fences, sizeof(**fences), GFP_KERNEL);
+	if (!fences)
+		goto err;
+
+	atomic_set(&sync_file->status, 1);
 
 	/*
 	 * Assume sync_file a and b are both ordered and have no
@@ -165,55 +198,69 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
 	 * If a sync_file can only be created with sync_file_merge
 	 * and sync_file_create, this is a reasonable assumption.
 	 */
-	for (i = i_a = i_b = 0; i_a < a->num_fences && i_b < b->num_fences; ) {
-		struct fence *pt_a = a->cbs[i_a].fence;
-		struct fence *pt_b = b->cbs[i_b].fence;
+	for (i = i_a = i_b = 0; i_a < a_num_fences && i_b < b_num_fences; ) {
+		struct fence *pt_a = a_fences[i_a];
+		struct fence *pt_b = b_fences[i_b];
 
 		if (pt_a->context < pt_b->context) {
-			sync_file_add_pt(sync_file, &i, pt_a);
+			add_fence(fences, &i, pt_a);
 
 			i_a++;
 		} else if (pt_a->context > pt_b->context) {
-			sync_file_add_pt(sync_file, &i, pt_b);
+			add_fence(fences, &i, pt_b);
 
 			i_b++;
 		} else {
 			if (pt_a->seqno - pt_b->seqno <= INT_MAX)
-				sync_file_add_pt(sync_file, &i, pt_a);
+				add_fence(fences, &i, pt_a);
 			else
-				sync_file_add_pt(sync_file, &i, pt_b);
+				add_fence(fences, &i, pt_b);
 
 			i_a++;
 			i_b++;
 		}
 	}
 
-	for (; i_a < a->num_fences; i_a++)
-		sync_file_add_pt(sync_file, &i, a->cbs[i_a].fence);
+	for (; i_a < a_num_fences; i_a++)
+		add_fence(fences, &i, a_fences[i_a]);
+
+	for (; i_b < b_num_fences; i_b++)
+		add_fence(fences, &i, b_fences[i_b]);
+
+	if (num_fences > i) {
+		fences = krealloc(fences, i * sizeof(**fences),
+				  GFP_KERNEL);
+		if (!fences)
+			goto err;
+	}
 
-	for (; i_b < b->num_fences; i_b++)
-		sync_file_add_pt(sync_file, &i, b->cbs[i_b].fence);
+	if (sync_file_set_fence(sync_file, fences, i) < 0) {
+		kfree(fences);
+		goto err;
+	}
 
-	if (num_fences > i)
-		atomic_sub(num_fences - i, &sync_file->status);
-	sync_file->num_fences = i;
+	if (fence_add_callback(sync_file->fence, &sync_file->cb,
+			       fence_check_cb_func))
+		atomic_dec(&sync_file->status);
 
 	strlcpy(sync_file->name, name, sizeof(sync_file->name));
 	return sync_file;
+
+err:
+	fput(sync_file->file);
+	return NULL;
+
 }
 
 static void sync_file_free(struct kref *kref)
 {
 	struct sync_file *sync_file = container_of(kref, struct sync_file,
 						     kref);
-	int i;
-
-	for (i = 0; i < sync_file->num_fences; ++i) {
-		fence_remove_callback(sync_file->cbs[i].fence,
-				      &sync_file->cbs[i].cb);
-		fence_put(sync_file->cbs[i].fence);
-	}
 
+	fence_remove_callback(sync_file->fence, &sync_file->cb);
+	if (fence_is_array(sync_file->fence))
+		fence_array_teardown(to_fence_array(sync_file->fence));
+	fence_put(sync_file->fence);
 	kfree(sync_file);
 }
 
@@ -315,8 +362,9 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
 {
 	struct sync_file_info info;
 	struct sync_fence_info *fence_info = NULL;
+	struct fence **fences;
 	__u32 size;
-	int ret, i;
+	int num_fences, ret, i;
 
 	if (copy_from_user(&info, (void __user *)arg, sizeof(info)))
 		return -EFAULT;
@@ -324,6 +372,8 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
 	if (info.flags || info.pad)
 		return -EINVAL;
 
+	fences = get_fences(sync_file, &num_fences);
+
 	/*
 	 * Passing num_fences = 0 means that userspace doesn't want to
 	 * retrieve any sync_fence_info. If num_fences = 0 we skip filling
@@ -333,16 +383,16 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
 	if (!info.num_fences)
 		goto no_fences;
 
-	if (info.num_fences < sync_file->num_fences)
+	if (info.num_fences < num_fences)
 		return -EINVAL;
 
-	size = sync_file->num_fences * sizeof(*fence_info);
+	size = num_fences * sizeof(*fence_info);
 	fence_info = kzalloc(size, GFP_KERNEL);
 	if (!fence_info)
 		return -ENOMEM;
 
-	for (i = 0; i < sync_file->num_fences; ++i)
-		sync_fill_fence_info(sync_file->cbs[i].fence, &fence_info[i]);
+	for (i = 0; i < num_fences; i++)
+		sync_fill_fence_info(fences[i], &fence_info[i]);
 
 	if (copy_to_user(u64_to_user_ptr(info.sync_fence_info), fence_info,
 			 size)) {
@@ -356,7 +406,7 @@ no_fences:
 	if (info.status >= 0)
 		info.status = !info.status;
 
-	info.num_fences = sync_file->num_fences;
+	info.num_fences = num_fences;
 
 	if (copy_to_user((void __user *)arg, &info, sizeof(info)))
 		ret = -EFAULT;
diff --git a/drivers/staging/android/sync_debug.c b/drivers/staging/android/sync_debug.c
index 5f57499..259a20d 100644
--- a/drivers/staging/android/sync_debug.c
+++ b/drivers/staging/android/sync_debug.c
@@ -161,8 +161,13 @@ static void sync_print_sync_file(struct seq_file *s,
 	seq_printf(s, "[%p] %s: %s\n", sync_file, sync_file->name,
 		   sync_status_str(atomic_read(&sync_file->status)));
 
-	for (i = 0; i < sync_file->num_fences; ++i)
-		sync_print_fence(s, sync_file->cbs[i].fence, true);
+	if (fence_is_array(sync_file->fence)) {
+		struct fence_array *array = to_fence_array(sync_file->fence);
+		for (i = 0; i < array->num_fences; ++i)
+			sync_print_fence(s, array->fences[i], true);
+	} else {
+		sync_print_fence(s, sync_file->fence, true);
+	}
 }
 
 static int sync_debugfs_show(struct seq_file *s, void *unused)
diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
index c6ffe8b..7313208 100644
--- a/include/linux/sync_file.h
+++ b/include/linux/sync_file.h
@@ -19,12 +19,7 @@
 #include <linux/list.h>
 #include <linux/spinlock.h>
 #include <linux/fence.h>
-
-struct sync_file_cb {
-	struct fence_cb cb;
-	struct fence *fence;
-	struct sync_file *sync_file;
-};
+#include <linux/fence-array.h>
 
 /**
  * struct sync_file - sync file to export to the userspace
@@ -32,10 +27,10 @@ struct sync_file_cb {
  * @kref:		reference count on fence.
  * @name:		name of sync_file.  Useful for debugging
  * @sync_file_list:	membership in global file list
- * @num_fences:		number of sync_pts in the fence
  * @wq:			wait queue for fence signaling
  * @status:		0: signaled, >0:active, <0: error
- * @cbs:		sync_pts callback information
+ * @fence:		fence with the fences in the sync_file
+ * @cb:			fence callback information
  */
 struct sync_file {
 	struct file		*file;
@@ -44,12 +39,12 @@ struct sync_file {
 #ifdef CONFIG_DEBUG_FS
 	struct list_head	sync_file_list;
 #endif
-	int num_fences;
 
 	wait_queue_head_t	wq;
 	atomic_t		status;
 
-	struct sync_file_cb	cbs[];
+	struct fence		*fence;
+	struct fence_cb cb;
 };
 
 struct sync_file *sync_file_create(struct fence *fence);
-- 
2.5.5

Re: [RFC v2 3/3] dma-buf/sync_file: rework fence storage in struct file

[Chris Wilson]

On Mon, Jun 27, 2016 at 04:29:22PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Create sync_file->fence to abstract the type of fence we are using for
> each sync_file. If only one fence is present we use a normal struct fence
> but if there is more fences to be added to the sync_file a fence_array
> is created.
> 
> This change cleans up sync_file a bit. We don't need to have sync_file_cb
> array anymore. Instead, as we always have  one fence, only one fence
> callback is registered per sync_file.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Christian König <christian.koenig@amd.com>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
> @@ -76,21 +76,19 @@ struct sync_file *sync_file_create(struct fence *fence)
>  {
>  	struct sync_file *sync_file;
>  
> -	sync_file = sync_file_alloc(offsetof(struct sync_file, cbs[1]));
> +	sync_file = sync_file_alloc();
>  	if (!sync_file)
>  		return NULL;
>  
> -	sync_file->num_fences = 1;
> +	sync_file->fence = fence;
> +
>  	atomic_set(&sync_file->status, 1);

sync_file->status => fence_is_signaled(sync_file->fence);

Both should just be an atomic read, except fence_is_signaled() will then
do a secondary poll.

>  	snprintf(sync_file->name, sizeof(sync_file->name), "%s-%s%llu-%d",
>  		 fence->ops->get_driver_name(fence),
>  		 fence->ops->get_timeline_name(fence), fence->context,
>  		 fence->seqno);
>  
> -	sync_file->cbs[0].fence = fence;
> -	sync_file->cbs[0].sync_file = sync_file;
> -	if (fence_add_callback(fence, &sync_file->cbs[0].cb,
> -			       fence_check_cb_func))
> +	if (fence_add_callback(fence, &sync_file->cb, fence_check_cb_func))
>  		atomic_dec(&sync_file->status);
>  
>  	return sync_file;
> @@ -121,14 +119,42 @@ err:
>  	return NULL;
>  }
>  
> -static void sync_file_add_pt(struct sync_file *sync_file, int *i,
> -			     struct fence *fence)
> +static int sync_file_set_fence(struct sync_file *sync_file,
> +			       struct fence **fences, int num_fences)
>  {
> -	sync_file->cbs[*i].fence = fence;
> -	sync_file->cbs[*i].sync_file = sync_file;
> +	struct fence_array *array;
> +
> +	if (num_fences == 1) {
> +		sync_file->fence = fences[0];

This steals the references.

> +	} else {
> +		array = fence_array_create(num_fences, fences,
> +					   fence_context_alloc(1), 1, false);

This creates a reference.

When we call fence_put(sync_fence->fence) we release a reference we
never owned if num_fences == 1.

> +	struct fence **fences, **a_fences, **b_fences;
> +	int i, i_a, i_b, num_fences, a_num_fences, b_num_fences;
>  
> +	a_fences = get_fences(a, &a_num_fences);
> +	b_fences = get_fences(b, &b_num_fences);
> +	num_fences = a_num_fences + b_num_fences;
> +
> +	fences = kcalloc(num_fences, sizeof(**fences), GFP_KERNEL);

Just sizeof(*fences) (you want to allocate an array of pointers, not an
array of fence structs).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [RFC v2 2/3] dma-buf/fence-array: add fence_array_teardown()

[Christian König]

Am 27.06.2016 um 21:29 schrieb Gustavo Padovan:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>
> As the array of fence callbacks held by an active struct fence_array
> each has a reference to the struct fence_array, when the owner of the
> fence_array is freed it must dispose of the callback references before
> it can free the fence_array. This can not happen simply during
> fence_release() because of the extra references and so we need a new
> function to run before the final fence_put().

As I said previously as well, this is completely superfluous.

The fence array keeps a reference to itself as long as not all callbacks 
are signaled.

So you only need to unregister your callback from the array itself and 
drop your reference when you don't need it any more in the sync file.

Additional to that having a special cleanup function like this strongly 
contradicts the approach of having an implementation independent interface.

So this is a clear NAK from my side.

Regards,
Christian.

>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Christian König <christian.koenig@amd.com>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>   drivers/dma-buf/fence-array.c | 25 +++++++++++++++++++++++++
>   include/linux/fence-array.h   |  1 +
>   2 files changed, 26 insertions(+)
>
> diff --git a/drivers/dma-buf/fence-array.c b/drivers/dma-buf/fence-array.c
> index a8731c8..8891357 100644
> --- a/drivers/dma-buf/fence-array.c
> +++ b/drivers/dma-buf/fence-array.c
> @@ -142,3 +142,28 @@ struct fence_array *fence_array_create(int num_fences, struct fence **fences,
>   	return array;
>   }
>   EXPORT_SYMBOL(fence_array_create);
> +
> +/**
> + * fence_array_teardown - Teardown and clean up a fence array
> + * @array:	[in]	the fence array to teardown
> + *
> + * This function removes callbacks and extra references to the base fence on
> + * the fence_array for each unsignalled fence in the array. It should be called
> + * before calling fence_put() to remove the last reference on a fence_array,
> + * otherwise the fence won't be released due to extra references holded by the
> + * the fences that still have signalling enabled.
> + */
> +void fence_array_teardown(struct fence_array *array)
> +{
> +	struct fence_array_cb *cb = (void *)(&array[1]);
> +	int i;
> +
> +	for (i = 0; i < array->num_fences; i++) {
> +		if (fence_is_signaled(array->fences[i]))
> +		    continue;
> +
> +		fence_remove_callback(array->fences[i], &cb[i].cb);
> +		fence_put(&array->base);
> +	}
> +}
> +EXPORT_SYMBOL(fence_array_teardown);
> diff --git a/include/linux/fence-array.h b/include/linux/fence-array.h
> index d2e9f40..9f1b923 100644
> --- a/include/linux/fence-array.h
> +++ b/include/linux/fence-array.h
> @@ -79,5 +79,6 @@ static inline struct fence_array *to_fence_array(struct fence *fence)
>   struct fence_array *fence_array_create(int num_fences, struct fence **fences,
>   				       u64 context, unsigned seqno,
>   				       bool signal_on_any);
> +void fence_array_teardown(struct fence_array *array);
>   
>   #endif /* __LINUX_FENCE_ARRAY_H */

Re: [RFC v2 2/3] dma-buf/fence-array: add fence_array_teardown()

[Gustavo Padovan]

2016-06-28 Christian König <christian.koenig@amd.com>:

> Am 27.06.2016 um 21:29 schrieb Gustavo Padovan:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > As the array of fence callbacks held by an active struct fence_array
> > each has a reference to the struct fence_array, when the owner of the
> > fence_array is freed it must dispose of the callback references before
> > it can free the fence_array. This can not happen simply during
> > fence_release() because of the extra references and so we need a new
> > function to run before the final fence_put().
> 
> As I said previously as well, this is completely superfluous.
> 
> The fence array keeps a reference to itself as long as not all callbacks are
> signaled.
> 
> So you only need to unregister your callback from the array itself and drop
> your reference when you don't need it any more in the sync file.

Exactly, this should be called from sync_file_free() because of the
following use case:

	1. You create 2 sync_file with 1 fence each
	2. Merge both fences, which creates a fence array
	3. Close the sync_file fd without waiting for the fences to
	signal

At this point you leak the fence-array because the final fence_put() 
does not release it because of the extra references from the non
signalled fences so we need to clean up this somehow.

	Gustavo

Re: [RFC v2 3/3] dma-buf/sync_file: rework fence storage in struct file

[Gustavo Padovan]

2016-06-28 Chris Wilson <chris@chris-wilson.co.uk>:

> On Mon, Jun 27, 2016 at 04:29:22PM -0300, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > Create sync_file->fence to abstract the type of fence we are using for
> > each sync_file. If only one fence is present we use a normal struct fence
> > but if there is more fences to be added to the sync_file a fence_array
> > is created.
> > 
> > This change cleans up sync_file a bit. We don't need to have sync_file_cb
> > array anymore. Instead, as we always have  one fence, only one fence
> > callback is registered per sync_file.
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Christian König <christian.koenig@amd.com>
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > ---
> > @@ -76,21 +76,19 @@ struct sync_file *sync_file_create(struct fence *fence)
> >  {
> >  	struct sync_file *sync_file;
> >  
> > -	sync_file = sync_file_alloc(offsetof(struct sync_file, cbs[1]));
> > +	sync_file = sync_file_alloc();
> >  	if (!sync_file)
> >  		return NULL;
> >  
> > -	sync_file->num_fences = 1;
> > +	sync_file->fence = fence;
> > +
> >  	atomic_set(&sync_file->status, 1);
> 
> sync_file->status => fence_is_signaled(sync_file->fence);
> 
> Both should just be an atomic read, except fence_is_signaled() will then
> do a secondary poll.

Not sure I follow. I set it to 1 here, but below when we call
fence_add_callback() and the fence is already signalled atomic_dec sets
sync_file->status to 0.

> 
> >  	snprintf(sync_file->name, sizeof(sync_file->name), "%s-%s%llu-%d",
> >  		 fence->ops->get_driver_name(fence),
> >  		 fence->ops->get_timeline_name(fence), fence->context,
> >  		 fence->seqno);
> >  
> > -	sync_file->cbs[0].fence = fence;
> > -	sync_file->cbs[0].sync_file = sync_file;
> > -	if (fence_add_callback(fence, &sync_file->cbs[0].cb,
> > -			       fence_check_cb_func))
> > +	if (fence_add_callback(fence, &sync_file->cb, fence_check_cb_func))
> >  		atomic_dec(&sync_file->status);
> >  
> >  	return sync_file;
> > @@ -121,14 +119,42 @@ err:
> >  	return NULL;
> >  }
> >  
> > -static void sync_file_add_pt(struct sync_file *sync_file, int *i,
> > -			     struct fence *fence)
> > +static int sync_file_set_fence(struct sync_file *sync_file,
> > +			       struct fence **fences, int num_fences)
> >  {
> > -	sync_file->cbs[*i].fence = fence;
> > -	sync_file->cbs[*i].sync_file = sync_file;
> > +	struct fence_array *array;
> > +
> > +	if (num_fences == 1) {
> > +		sync_file->fence = fences[0];
> 
> This steals the references.
> 
> > +	} else {
> > +		array = fence_array_create(num_fences, fences,
> > +					   fence_context_alloc(1), 1, false);
> 
> This creates a reference.
> 
> When we call fence_put(sync_fence->fence) we release a reference we
> never owned if num_fences == 1.

No, sync_file_merge() gets a new reference for each fence it is going to
add to the new fence. So for num_fences == 1 when sync_file->fence is
set we already hold a reference to it, so no matter if it is a fence or
a array we own a reference.

> 
> > +	struct fence **fences, **a_fences, **b_fences;
> > +	int i, i_a, i_b, num_fences, a_num_fences, b_num_fences;
> >  
> > +	a_fences = get_fences(a, &a_num_fences);
> > +	b_fences = get_fences(b, &b_num_fences);
> > +	num_fences = a_num_fences + b_num_fences;
> > +
> > +	fences = kcalloc(num_fences, sizeof(**fences), GFP_KERNEL);
> 
> Just sizeof(*fences) (you want to allocate an array of pointers, not an
> array of fence structs).

Okay.

	Gustavo

Re: [RFC v2 2/3] dma-buf/fence-array: add fence_array_teardown()

[Christian König]

Am 28.06.2016 um 16:17 schrieb Gustavo Padovan:
> 2016-06-28 Christian König <christian.koenig@amd.com>:
>
>> Am 27.06.2016 um 21:29 schrieb Gustavo Padovan:
>>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>>
>>> As the array of fence callbacks held by an active struct fence_array
>>> each has a reference to the struct fence_array, when the owner of the
>>> fence_array is freed it must dispose of the callback references before
>>> it can free the fence_array. This can not happen simply during
>>> fence_release() because of the extra references and so we need a new
>>> function to run before the final fence_put().
>> As I said previously as well, this is completely superfluous.
>>
>> The fence array keeps a reference to itself as long as not all callbacks are
>> signaled.
>>
>> So you only need to unregister your callback from the array itself and drop
>> your reference when you don't need it any more in the sync file.
> Exactly, this should be called from sync_file_free() because of the
> following use case:
>
> 	1. You create 2 sync_file with 1 fence each
> 	2. Merge both fences, which creates a fence array
> 	3. Close the sync_file fd without waiting for the fences to
> 	signal
>
> At this point you leak the fence-array because the final fence_put()
> does not release it because of the extra references from the non
> signalled fences so we need to clean up this somehow.

No, there won't be any leak and you don't need to cleanup anything.

The fences contained in the fence array are already enabled for 
signaling. So they will eventually signal sooner or later and drop the 
reference to the fence array freeing it when the last one finished.

This was done to avoid the need to call fence_remove_callback() all the 
time on the fences in the array because of the warning on the function.

Regards,
Christian.

>
> 	Gustavo

Re: [RFC v2 3/3] dma-buf/sync_file: rework fence storage in struct file

[Chris Wilson]

On Tue, Jun 28, 2016 at 11:25:00AM -0300, Gustavo Padovan wrote:
> 2016-06-28 Chris Wilson <chris@chris-wilson.co.uk>:
> 
> > On Mon, Jun 27, 2016 at 04:29:22PM -0300, Gustavo Padovan wrote:
> > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > 
> > > Create sync_file->fence to abstract the type of fence we are using for
> > > each sync_file. If only one fence is present we use a normal struct fence
> > > but if there is more fences to be added to the sync_file a fence_array
> > > is created.
> > > 
> > > This change cleans up sync_file a bit. We don't need to have sync_file_cb
> > > array anymore. Instead, as we always have  one fence, only one fence
> > > callback is registered per sync_file.
> > > 
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Christian König <christian.koenig@amd.com>
> > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > ---
> > > @@ -76,21 +76,19 @@ struct sync_file *sync_file_create(struct fence *fence)
> > >  {
> > >  	struct sync_file *sync_file;
> > >  
> > > -	sync_file = sync_file_alloc(offsetof(struct sync_file, cbs[1]));
> > > +	sync_file = sync_file_alloc();
> > >  	if (!sync_file)
> > >  		return NULL;
> > >  
> > > -	sync_file->num_fences = 1;
> > > +	sync_file->fence = fence;
> > > +
> > >  	atomic_set(&sync_file->status, 1);
> > 
> > sync_file->status => fence_is_signaled(sync_file->fence);
> > 
> > Both should just be an atomic read, except fence_is_signaled() will then
> > do a secondary poll.
> 
> Not sure I follow. I set it to 1 here, but below when we call
> fence_add_callback() and the fence is already signalled atomic_dec sets
> sync_file->status to 0.

I'm just saying that usage sync_file->status is equivalent to
fence_is_signaled(), i.e. we reduce the amount of bookkeeping local to
sync_file.

> > >  	snprintf(sync_file->name, sizeof(sync_file->name), "%s-%s%llu-%d",
> > >  		 fence->ops->get_driver_name(fence),
> > >  		 fence->ops->get_timeline_name(fence), fence->context,
> > >  		 fence->seqno);
> > >  
> > > -	sync_file->cbs[0].fence = fence;
> > > -	sync_file->cbs[0].sync_file = sync_file;
> > > -	if (fence_add_callback(fence, &sync_file->cbs[0].cb,
> > > -			       fence_check_cb_func))
> > > +	if (fence_add_callback(fence, &sync_file->cb, fence_check_cb_func))
> > >  		atomic_dec(&sync_file->status);
> > >  
> > >  	return sync_file;
> > > @@ -121,14 +119,42 @@ err:
> > >  	return NULL;
> > >  }
> > >  
> > > -static void sync_file_add_pt(struct sync_file *sync_file, int *i,
> > > -			     struct fence *fence)
> > > +static int sync_file_set_fence(struct sync_file *sync_file,
> > > +			       struct fence **fences, int num_fences)
> > >  {
> > > -	sync_file->cbs[*i].fence = fence;
> > > -	sync_file->cbs[*i].sync_file = sync_file;
> > > +	struct fence_array *array;
> > > +
> > > +	if (num_fences == 1) {
> > > +		sync_file->fence = fences[0];
> > 
> > This steals the references.
> > 
> > > +	} else {
> > > +		array = fence_array_create(num_fences, fences,
> > > +					   fence_context_alloc(1), 1, false);
> > 
> > This creates a reference.
> > 
> > When we call fence_put(sync_fence->fence) we release a reference we
> > never owned if num_fences == 1.
> 
> No, sync_file_merge() gets a new reference for each fence it is going to
> add to the new fence. So for num_fences == 1 when sync_file->fence is
> set we already hold a reference to it, so no matter if it is a fence or
> a array we own a reference.

Ugh. Root cause appears to be that fence_array_create() does not behave
how I would expect, in that it borrows references to the fences and
not own a reference to the fences in its array. I beg for a comment as
this function is very counter-intuitive for me.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [RFC v2 2/3] dma-buf/fence-array: add fence_array_teardown()

[Gustavo Padovan]

2016-06-28 Christian König <christian.koenig@amd.com>:

> Am 28.06.2016 um 16:17 schrieb Gustavo Padovan:
> > 2016-06-28 Christian König <christian.koenig@amd.com>:
> > 
> > > Am 27.06.2016 um 21:29 schrieb Gustavo Padovan:
> > > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > > 
> > > > As the array of fence callbacks held by an active struct fence_array
> > > > each has a reference to the struct fence_array, when the owner of the
> > > > fence_array is freed it must dispose of the callback references before
> > > > it can free the fence_array. This can not happen simply during
> > > > fence_release() because of the extra references and so we need a new
> > > > function to run before the final fence_put().
> > > As I said previously as well, this is completely superfluous.
> > > 
> > > The fence array keeps a reference to itself as long as not all callbacks are
> > > signaled.
> > > 
> > > So you only need to unregister your callback from the array itself and drop
> > > your reference when you don't need it any more in the sync file.
> > Exactly, this should be called from sync_file_free() because of the
> > following use case:
> > 
> > 	1. You create 2 sync_file with 1 fence each
> > 	2. Merge both fences, which creates a fence array
> > 	3. Close the sync_file fd without waiting for the fences to
> > 	signal
> > 
> > At this point you leak the fence-array because the final fence_put()
> > does not release it because of the extra references from the non
> > signalled fences so we need to clean up this somehow.
> 
> No, there won't be any leak and you don't need to cleanup anything.
> 
> The fences contained in the fence array are already enabled for signaling.
> So they will eventually signal sooner or later and drop the reference to the
> fence array freeing it when the last one finished.
> 
> This was done to avoid the need to call fence_remove_callback() all the time
> on the fences in the array because of the warning on the function.

Right. I understand it now.

	Gustavo

Re: [RFC v2 3/3] dma-buf/sync_file: rework fence storage in struct file

[Gustavo Padovan]

2016-06-28 Chris Wilson <chris@chris-wilson.co.uk>:

> On Tue, Jun 28, 2016 at 11:25:00AM -0300, Gustavo Padovan wrote:
> > 2016-06-28 Chris Wilson <chris@chris-wilson.co.uk>:
> > 
> > > On Mon, Jun 27, 2016 at 04:29:22PM -0300, Gustavo Padovan wrote:
> > > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > > 
> > > > Create sync_file->fence to abstract the type of fence we are using for
> > > > each sync_file. If only one fence is present we use a normal struct fence
> > > > but if there is more fences to be added to the sync_file a fence_array
> > > > is created.
> > > > 
> > > > This change cleans up sync_file a bit. We don't need to have sync_file_cb
> > > > array anymore. Instead, as we always have  one fence, only one fence
> > > > callback is registered per sync_file.
> > > > 
> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: Christian König <christian.koenig@amd.com>
> > > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > > ---
> > > > @@ -76,21 +76,19 @@ struct sync_file *sync_file_create(struct fence *fence)
> > > >  {
> > > >  	struct sync_file *sync_file;
> > > >  
> > > > -	sync_file = sync_file_alloc(offsetof(struct sync_file, cbs[1]));
> > > > +	sync_file = sync_file_alloc();
> > > >  	if (!sync_file)
> > > >  		return NULL;
> > > >  
> > > > -	sync_file->num_fences = 1;
> > > > +	sync_file->fence = fence;
> > > > +
> > > >  	atomic_set(&sync_file->status, 1);
> > > 
> > > sync_file->status => fence_is_signaled(sync_file->fence);
> > > 
> > > Both should just be an atomic read, except fence_is_signaled() will then
> > > do a secondary poll.
> > 
> > Not sure I follow. I set it to 1 here, but below when we call
> > fence_add_callback() and the fence is already signalled atomic_dec sets
> > sync_file->status to 0.
> 
> I'm just saying that usage sync_file->status is equivalent to
> fence_is_signaled(), i.e. we reduce the amount of bookkeeping local to
> sync_file.

Indeed, that makes sense, I'll remove status from sync_file.

> 
> > > >  	snprintf(sync_file->name, sizeof(sync_file->name), "%s-%s%llu-%d",
> > > >  		 fence->ops->get_driver_name(fence),
> > > >  		 fence->ops->get_timeline_name(fence), fence->context,
> > > >  		 fence->seqno);
> > > >  
> > > > -	sync_file->cbs[0].fence = fence;
> > > > -	sync_file->cbs[0].sync_file = sync_file;
> > > > -	if (fence_add_callback(fence, &sync_file->cbs[0].cb,
> > > > -			       fence_check_cb_func))
> > > > +	if (fence_add_callback(fence, &sync_file->cb, fence_check_cb_func))
> > > >  		atomic_dec(&sync_file->status);
> > > >  
> > > >  	return sync_file;
> > > > @@ -121,14 +119,42 @@ err:
> > > >  	return NULL;
> > > >  }
> > > >  
> > > > -static void sync_file_add_pt(struct sync_file *sync_file, int *i,
> > > > -			     struct fence *fence)
> > > > +static int sync_file_set_fence(struct sync_file *sync_file,
> > > > +			       struct fence **fences, int num_fences)
> > > >  {
> > > > -	sync_file->cbs[*i].fence = fence;
> > > > -	sync_file->cbs[*i].sync_file = sync_file;
> > > > +	struct fence_array *array;
> > > > +
> > > > +	if (num_fences == 1) {
> > > > +		sync_file->fence = fences[0];
> > > 
> > > This steals the references.
> > > 
> > > > +	} else {
> > > > +		array = fence_array_create(num_fences, fences,
> > > > +					   fence_context_alloc(1), 1, false);
> > > 
> > > This creates a reference.
> > > 
> > > When we call fence_put(sync_fence->fence) we release a reference we
> > > never owned if num_fences == 1.
> > 
> > No, sync_file_merge() gets a new reference for each fence it is going to
> > add to the new fence. So for num_fences == 1 when sync_file->fence is
> > set we already hold a reference to it, so no matter if it is a fence or
> > a array we own a reference.
> 
> Ugh. Root cause appears to be that fence_array_create() does not behave
> how I would expect, in that it borrows references to the fences and
> not own a reference to the fences in its array. I beg for a comment as
> this function is very counter-intuitive for me.

There is this in fence_array_create():

 * The caller should allocte 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 take and fence_put() is used on each fence on release.     

	Gustavo