[RFC 0/5] rework fences on struct sync_file

[RFC 0/5] 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.

Two fence ops had to be created to help abstract the difference between handling
fences and fences_arrays: .teardown() and .get_fences(). The former run needed
on fence_array, and the latter just return a copy of all fences in the fence.
I'm not so sure about adding those two, speacially .get_fences(). What do you
think?

Please comment! Thanks.

	Gustavo
---

Gustavo Padovan (5):
  dma-buf/fence: add .teardown() ops
  dma-buf/fence-array: add fence_array_teardown()
  dma-buf/fence: add .get_fences() ops
  dma-buf/fence-array: add fence_array_get_fences()
  dma-buf/sync_file: rework fence storage in struct file

 drivers/dma-buf/fence-array.c        |  30 ++++++++
 drivers/dma-buf/fence.c              |  21 ++++++
 drivers/dma-buf/sync_file.c          | 129 +++++++++++++++++++++++++----------
 drivers/staging/android/sync_debug.c |   5 +-
 include/linux/fence.h                |  10 +++
 include/linux/sync_file.h            |  12 ++--
 6 files changed, 161 insertions(+), 46 deletions(-)

-- 
2.5.5

[RFC 1/5] dma-buf/fence: add .teardown() ops

[Gustavo Padovan]

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

fence_array requires a function to clean up its state before we
are able to call fence_put() and release it.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/dma-buf/fence.c | 7 +++++++
 include/linux/fence.h   | 7 +++++++
 2 files changed, 14 insertions(+)

diff --git a/drivers/dma-buf/fence.c b/drivers/dma-buf/fence.c
index 4d51f9e..4e61afb 100644
--- a/drivers/dma-buf/fence.c
+++ b/drivers/dma-buf/fence.c
@@ -185,6 +185,13 @@ void fence_release(struct kref *kref)
 }
 EXPORT_SYMBOL(fence_release);
 
+void fence_teardown(struct fence *fence)
+{
+	if (fence->ops->teardown)
+		fence->ops->teardown(fence);
+}
+EXPORT_SYMBOL(fence_teardown);
+
 void fence_free(struct fence *fence)
 {
 	kfree_rcu(fence, rcu);
diff --git a/include/linux/fence.h b/include/linux/fence.h
index 44d945e..1d3b671 100644
--- a/include/linux/fence.h
+++ b/include/linux/fence.h
@@ -114,6 +114,7 @@ struct fence_cb {
  * @enable_signaling: enable software signaling of fence.
  * @signaled: [optional] peek whether the fence is signaled, can be null.
  * @wait: custom wait implementation, or fence_default_wait.
+ * @teardown: [optional] teardown fence data but not put it
  * @release: [optional] called on destruction of fence, can be null
  * @fill_driver_data: [optional] callback to fill in free-form debug info
  * Returns amount of bytes filled, or -errno.
@@ -161,6 +162,10 @@ struct fence_cb {
  * which should be treated as if the fence is signaled. For example a hardware
  * lockup could be reported like that.
  *
+ * Notes on teardown:
+ * Can be NULL, this function clean ups the fence data before the fence_put
+ * call.
+ *
  * Notes on release:
  * Can be NULL, this function allows additional commands to run on
  * destruction of the fence. Can be called from irq context.
@@ -173,6 +178,7 @@ struct fence_ops {
 	bool (*enable_signaling)(struct fence *fence);
 	bool (*signaled)(struct fence *fence);
 	signed long (*wait)(struct fence *fence, bool intr, signed long timeout);
+	void (*teardown)(struct fence *fence);
 	void (*release)(struct fence *fence);
 
 	int (*fill_driver_data)(struct fence *fence, void *data, int size);
@@ -184,6 +190,7 @@ void fence_init(struct fence *fence, const struct fence_ops *ops,
 		spinlock_t *lock, u64 context, unsigned seqno);
 
 void fence_release(struct kref *kref);
+void fence_teardown(struct fence *fence);
 void fence_free(struct fence *fence);
 
 /**
-- 
2.5.5

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

[Gustavo Padovan]

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

When using fences in sync files we need to clean up everything when
the sync file needs to be freed, thus we need to teardown fence_array,
by removing the callback of its fences and putting extra references to the
fence_array base fence.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/dma-buf/fence-array.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/dma-buf/fence-array.c b/drivers/dma-buf/fence-array.c
index a8731c8..601448a 100644
--- a/drivers/dma-buf/fence-array.c
+++ b/drivers/dma-buf/fence-array.c
@@ -79,6 +79,21 @@ static bool fence_array_signaled(struct fence *fence)
 	return atomic_read(&array->num_pending) <= 0;
 }
 
+static void fence_array_teardown(struct fence *fence)
+{
+	struct fence_array *array = to_fence_array(fence);
+	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);
+	}
+}
+
 static void fence_array_release(struct fence *fence)
 {
 	struct fence_array *array = to_fence_array(fence);
@@ -97,6 +112,7 @@ const struct fence_ops fence_array_ops = {
 	.enable_signaling = fence_array_enable_signaling,
 	.signaled = fence_array_signaled,
 	.wait = fence_default_wait,
+	.teardown = fence_array_teardown,
 	.release = fence_array_release,
 };
 
-- 
2.5.5

[RFC 3/5] dma-buf/fence: add .get_fences() ops

[Gustavo Padovan]

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

get_fences() should return a copy of all fences in the fence as some
fence subclass (such as fence_array) can store more than one fence at
time.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/dma-buf/fence.c | 14 ++++++++++++++
 include/linux/fence.h   |  3 +++
 2 files changed, 17 insertions(+)

diff --git a/drivers/dma-buf/fence.c b/drivers/dma-buf/fence.c
index 4e61afb..f4094fd 100644
--- a/drivers/dma-buf/fence.c
+++ b/drivers/dma-buf/fence.c
@@ -185,6 +185,20 @@ void fence_release(struct kref *kref)
 }
 EXPORT_SYMBOL(fence_release);
 
+struct fence **fence_get_fences(struct fence *fence)
+{
+	if (fence->ops->get_fences) {
+		return fence->ops->get_fences(fence);
+	} else {
+		struct fence **fences = kmalloc(sizeof(**fences), GFP_KERNEL);
+		if (!fences)
+			return NULL;
+		fences[0] = fence;
+		return fences;
+	}
+}
+EXPORT_SYMBOL(fence_get_fences);
+
 void fence_teardown(struct fence *fence)
 {
 	if (fence->ops->teardown)
diff --git a/include/linux/fence.h b/include/linux/fence.h
index 1d3b671..a7a2fbc 100644
--- a/include/linux/fence.h
+++ b/include/linux/fence.h
@@ -111,6 +111,7 @@ struct fence_cb {
  * struct fence_ops - operations implemented for fence
  * @get_driver_name: returns the driver name.
  * @get_timeline_name: return the name of the context this fence belongs to.
+ * @get_fences: return an array with a copy of all fences in the fence.
  * @enable_signaling: enable software signaling of fence.
  * @signaled: [optional] peek whether the fence is signaled, can be null.
  * @wait: custom wait implementation, or fence_default_wait.
@@ -175,6 +176,7 @@ struct fence_cb {
 struct fence_ops {
 	const char * (*get_driver_name)(struct fence *fence);
 	const char * (*get_timeline_name)(struct fence *fence);
+	struct fence ** (*get_fences)(struct fence *fence);
 	bool (*enable_signaling)(struct fence *fence);
 	bool (*signaled)(struct fence *fence);
 	signed long (*wait)(struct fence *fence, bool intr, signed long timeout);
@@ -189,6 +191,7 @@ struct fence_ops {
 void fence_init(struct fence *fence, const struct fence_ops *ops,
 		spinlock_t *lock, u64 context, unsigned seqno);
 
+struct fence **fence_get_fences(struct fence *fence);
 void fence_release(struct kref *kref);
 void fence_teardown(struct fence *fence);
 void fence_free(struct fence *fence);
-- 
2.5.5

[RFC 4/5] dma-buf/fence-array: add fence_array_get_fences()

[Gustavo Padovan]

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

This function returns a copy of the array of fences.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/dma-buf/fence-array.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/dma-buf/fence-array.c b/drivers/dma-buf/fence-array.c
index 601448a..ce98249 100644
--- a/drivers/dma-buf/fence-array.c
+++ b/drivers/dma-buf/fence-array.c
@@ -33,6 +33,19 @@ static const char *fence_array_get_timeline_name(struct fence *fence)
 	return "unbound";
 }
 
+static struct fence **fence_array_get_fences(struct fence *fence)
+{
+	struct fence_array *array = to_fence_array(fence);
+	struct fence **fences;
+
+	fences = kmalloc(array->num_fences * sizeof(*fences), GFP_KERNEL);
+	if (!fences)
+		return NULL;
+
+	memcpy(fences, array->fences, array->num_fences * sizeof(*fences));
+	return fences;
+}
+
 static void fence_array_cb_func(struct fence *f, struct fence_cb *cb)
 {
 	struct fence_array_cb *array_cb =
@@ -109,6 +122,7 @@ static void fence_array_release(struct fence *fence)
 const struct fence_ops fence_array_ops = {
 	.get_driver_name = fence_array_get_driver_name,
 	.get_timeline_name = fence_array_get_timeline_name,
+	.get_fences = fence_array_get_fences,
 	.enable_signaling = fence_array_enable_signaling,
 	.signaled = fence_array_signaled,
 	.wait = fence_default_wait,
-- 
2.5.5

[RFC 5/5] 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 behaviour is transparent all sync_file functions, but
sync_file_set_fence() which sets the fence in the sync_file_merge(). It
is this functions that decides to use a fence or fence_array based on
num_fences.

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.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/dma-buf/sync_file.c          | 129 +++++++++++++++++++++++++----------
 drivers/staging/android/sync_debug.c |   5 +-
 include/linux/sync_file.h            |  12 ++--
 3 files changed, 100 insertions(+), 46 deletions(-)

diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index 9aaa608..5044ef2 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -25,14 +25,15 @@
 #include <linux/anon_inodes.h>
 #include <linux/sync_file.h>
 #include <uapi/linux/sync_file.h>
+#include <linux/fence-array.h>
 
 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 +46,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 +57,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 +77,20 @@ 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->fence = fence;
 	sync_file->num_fences = 1;
+
 	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 +121,31 @@ err:
 	return NULL;
 }
 
-static void sync_file_add_pt(struct sync_file *sync_file, int *i,
+static int sync_file_set_fence(struct sync_file *sync_file,
+			       struct fence **fences)
+{
+	struct fence_array *array;
+
+	if (sync_file->num_fences == 1) {
+		sync_file->fence = fences[0];
+	} else {
+		array = fence_array_create(sync_file->num_fences, fences,
+					   fence_context_alloc(1), 1, false);
+		if (!array)
+			return -ENOMEM;
+
+		sync_file->fence = &array->base;
+	}
+
+	return 0;
+}
+
+static void fences_add_fence(struct fence **fences, int *i,
 			     struct fence *fence)
 {
-	sync_file->cbs[*i].fence = fence;
-	sync_file->cbs[*i].sync_file = sync_file;
+	fences[*i] = fence;
 
-	if (!fence_add_callback(fence, &sync_file->cbs[*i].cb,
-				fence_check_cb_func)) {
+	if (!fence_is_signaled(fence)) {
 		fence_get(fence);
 		(*i)++;
 	}
@@ -149,14 +166,31 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
 {
 	int num_fences = a->num_fences + b->num_fences;
 	struct sync_file *sync_file;
+	struct fence **fences, **a_fences, **b_fences;
 	int i, i_a, i_b;
-	unsigned long size = offsetof(struct sync_file, cbs[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);
+	fences = kcalloc(num_fences, sizeof(**fences), GFP_KERNEL);
+	if (!fences)
+		goto err;
+
+	atomic_set(&sync_file->status, 1);
+
+	a_fences = fence_get_fences(a->fence);
+	if (!a_fences) {
+		kfree(fences);
+		goto err;
+	}
+
+	b_fences = fence_get_fences(b->fence);
+	if (!b_fences) {
+		kfree(a_fences);
+		kfree(fences);
+		goto err;
+	}
 
 	/*
 	 * Assume sync_file a and b are both ordered and have no
@@ -166,22 +200,22 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
 	 * 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;
+		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);
+			fences_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);
+			fences_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);
+				fences_add_fence(fences, &i, pt_a);
 			else
-				sync_file_add_pt(sync_file, &i, pt_b);
+				fences_add_fence(fences, &i, pt_b);
 
 			i_a++;
 			i_b++;
@@ -189,31 +223,49 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
 	}
 
 	for (; i_a < a->num_fences; i_a++)
-		sync_file_add_pt(sync_file, &i, a->cbs[i_a].fence);
+		fences_add_fence(fences, &i, a_fences[i_a]);
 
 	for (; i_b < b->num_fences; i_b++)
-		sync_file_add_pt(sync_file, &i, b->cbs[i_b].fence);
+		fences_add_fence(fences, &i, b_fences[i_b]);
 
-	if (num_fences > i)
-		atomic_sub(num_fences - i, &sync_file->status);
+	if (num_fences > i) {
+		fences = krealloc(fences, i * sizeof(**fences),
+				  GFP_KERNEL);
+		if (!fences)
+			goto ab_err;
+	}
 	sync_file->num_fences = i;
 
+	if (sync_file_set_fence(sync_file, fences) < 0) {
+		kfree(fences);
+		goto ab_err;
+	}
+
+	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;
+
+ab_err:
+	kfree(a_fences);
+	kfree(b_fences);
+
+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);
+	fence_teardown(sync_file->fence);
+	fence_put(sync_file->fence);
 	kfree(sync_file);
 }
 
@@ -315,6 +367,7 @@ 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;
 
@@ -341,8 +394,10 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
 	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]);
+	fences = fence_get_fences(sync_file->fence);
+	for (i = 0; i < sync_file->num_fences; i++)
+		sync_fill_fence_info(fences[i], &fence_info[i]);
+	kfree(fences);
 
 	if (copy_to_user(u64_to_user_ptr(info.sync_fence_info), fence_info,
 			 size)) {
diff --git a/drivers/staging/android/sync_debug.c b/drivers/staging/android/sync_debug.c
index 5f57499..5fb6d3d 100644
--- a/drivers/staging/android/sync_debug.c
+++ b/drivers/staging/android/sync_debug.c
@@ -156,13 +156,16 @@ static void sync_print_obj(struct seq_file *s, struct sync_timeline *obj)
 static void sync_print_sync_file(struct seq_file *s,
 				  struct sync_file *sync_file)
 {
+	struct fence **fences;
 	int i;
 
 	seq_printf(s, "[%p] %s: %s\n", sync_file, sync_file->name,
 		   sync_status_str(atomic_read(&sync_file->status)));
 
+	fences = fence_get_fences(sync_file->fence);
 	for (i = 0; i < sync_file->num_fences; ++i)
-		sync_print_fence(s, sync_file->cbs[i].fence, true);
+		sync_print_fence(s, fences[i], true);
+	kfree(fences);
 }
 
 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..d13885f 100644
--- a/include/linux/sync_file.h
+++ b/include/linux/sync_file.h
@@ -20,12 +20,6 @@
 #include <linux/spinlock.h>
 #include <linux/fence.h>
 
-struct sync_file_cb {
-	struct fence_cb cb;
-	struct fence *fence;
-	struct sync_file *sync_file;
-};
-
 /**
  * struct sync_file - sync file to export to the userspace
  * @file:		file representing this fence
@@ -35,7 +29,8 @@ struct sync_file_cb {
  * @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;
@@ -49,7 +44,8 @@ struct sync_file {
 	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 4/5] dma-buf/fence-array: add fence_array_get_fences()

[Chris Wilson]

On Thu, Jun 23, 2016 at 12:29:49PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> This function returns a copy of the array of fences.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/dma-buf/fence-array.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/dma-buf/fence-array.c b/drivers/dma-buf/fence-array.c
> index 601448a..ce98249 100644
> --- a/drivers/dma-buf/fence-array.c
> +++ b/drivers/dma-buf/fence-array.c
> @@ -33,6 +33,19 @@ static const char *fence_array_get_timeline_name(struct fence *fence)
>  	return "unbound";
>  }
>  
> +static struct fence **fence_array_get_fences(struct fence *fence)
> +{
> +	struct fence_array *array = to_fence_array(fence);
> +	struct fence **fences;
> +
> +	fences = kmalloc(array->num_fences * sizeof(*fences), GFP_KERNEL);
> +	if (!fences)
> +		return NULL;
> +
> +	memcpy(fences, array->fences, array->num_fences * sizeof(*fences));
> +	return fences;

	return kmemdup(array->fences,
		       array->num_fences * sizeof(*array->fences),
		       GFP_KERNEL);

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [RFC 3/5] dma-buf/fence: add .get_fences() ops

[Chris Wilson]

On Thu, Jun 23, 2016 at 12:29:48PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> get_fences() should return a copy of all fences in the fence as some
> fence subclass (such as fence_array) can store more than one fence at
> time.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/dma-buf/fence.c | 14 ++++++++++++++
>  include/linux/fence.h   |  3 +++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/drivers/dma-buf/fence.c b/drivers/dma-buf/fence.c
> index 4e61afb..f4094fd 100644
> --- a/drivers/dma-buf/fence.c
> +++ b/drivers/dma-buf/fence.c
> @@ -185,6 +185,20 @@ void fence_release(struct kref *kref)
>  }
>  EXPORT_SYMBOL(fence_release);
>  
> +struct fence **fence_get_fences(struct fence *fence)

Returning an array, but not telling the caller how many elements in the
array?

> +{
> +	if (fence->ops->get_fences) {
> +		return fence->ops->get_fences(fence);
> +	} else {
> +		struct fence **fences = kmalloc(sizeof(**fences), GFP_KERNEL);

One too many * (=> sizeof(struct fence), not sizeof(struct fence *))

return kmemdup(&fence, sizeof(fence), GFP_KERNEL);

The documentation should emphasize that the fences in the
returned array have a "borrowed" reference (i.e. it does not return a
new reference to each fence).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [RFC 1/5] dma-buf/fence: add .teardown() ops

[Chris Wilson]

On Thu, Jun 23, 2016 at 12:29:46PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> fence_array requires a function to clean up its state before we
> are able to call fence_put() and release it.

An explanation along the lines of:

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().

would help, it is not until you use it in 5/5 that it becomes apparent
why it is needed.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Chris Wilson]

On Thu, Jun 23, 2016 at 12:29:50PM -0300, Gustavo Padovan wrote:
> -static void sync_file_add_pt(struct sync_file *sync_file, int *i,
> +static int sync_file_set_fence(struct sync_file *sync_file,
> +			       struct fence **fences)
> +{
> +	struct fence_array *array;
> +
> +	if (sync_file->num_fences == 1) {
> +		sync_file->fence = fences[0];

Straightforward pointer assignment.

> +	} else {
> +		array = fence_array_create(sync_file->num_fences, fences,
> +					   fence_context_alloc(1), 1, false);
> +		if (!array)
> +			return -ENOMEM;
> +
> +		sync_file->fence = &array->base;

New reference.

Imbalance will promptly go bang after we release the single fence[0].

Would fence_array_create(1, fence) returning fence_get(fence) be too
much of a hack?

I would suggest dropping the exported fence_get_fences() and use a local
instead that could avoid the copy, e.g.

static struct fence *get_fences(struct fence **fence,
				unsigned int *num_fences)
{
	if (fence_is_array(*fence)) {
		struct fence_array *array = to_fence_array(*fence);
		*num_fences = array->num_fences;
		return array->fences;
	} else {
		*num_fences = 1;
		return fence;
	}
}

sync_file_merge() {
	int num_fences, num_a_fences, num_b_fences;
	struct fence **fences, **a_fences, **b_fences;

	a_fences = get_fences(&a, &num_a_fences);
	b_fences = get_fences(&b, &num_b_fences);

	num_fences = num_a_fences + num_b_fences;

>  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);
> +	fence_teardown(sync_file->fence);

Hmm. Could we detect the removal of the last callback and propagate that
to the fence_array? (Rather then introduce a manual call to
fence_teardown.)

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [RFC 0/5] rework fences on struct sync_file

[Christian König]

Am 23.06.2016 um 17:29 schrieb 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.
>
> Two fence ops had to be created to help abstract the difference between handling
> fences and fences_arrays: .teardown() and .get_fences(). The former run needed
> on fence_array, and the latter just return a copy of all fences in the fence.
> I'm not so sure about adding those two, speacially .get_fences(). What do you
> think?

Clearly not a good idea to add this a fence ops, cause those are 
specialized functions for only a certain fence implementation (the 
fence_array).

What you should do is try to cast the fence in your sync file using 
to_fence_array() and then you can access the fences in the array.

Regards,
Christian.

>
> Please comment! Thanks.
>
> 	Gustavo
> ---
>
> Gustavo Padovan (5):
>    dma-buf/fence: add .teardown() ops
>    dma-buf/fence-array: add fence_array_teardown()
>    dma-buf/fence: add .get_fences() ops
>    dma-buf/fence-array: add fence_array_get_fences()
>    dma-buf/sync_file: rework fence storage in struct file
>
>   drivers/dma-buf/fence-array.c        |  30 ++++++++
>   drivers/dma-buf/fence.c              |  21 ++++++
>   drivers/dma-buf/sync_file.c          | 129 +++++++++++++++++++++++++----------
>   drivers/staging/android/sync_debug.c |   5 +-
>   include/linux/fence.h                |  10 +++
>   include/linux/sync_file.h            |  12 ++--
>   6 files changed, 161 insertions(+), 46 deletions(-)
>

Re: [RFC 0/5] rework fences on struct sync_file

[Gustavo Padovan]

Hi Christian,

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

> Am 23.06.2016 um 17:29 schrieb 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.
> > 
> > Two fence ops had to be created to help abstract the difference between handling
> > fences and fences_arrays: .teardown() and .get_fences(). The former run needed
> > on fence_array, and the latter just return a copy of all fences in the fence.
> > I'm not so sure about adding those two, speacially .get_fences(). What do you
> > think?
> 
> Clearly not a good idea to add this a fence ops, cause those are specialized
> functions for only a certain fence implementation (the fence_array).

Are you refering only to .get_fences()?

> 
> What you should do is try to cast the fence in your sync file using
> to_fence_array() and then you can access the fences in the array.

Yes, that seems a better idea I think. The initial idea was to abstract         
the difference as much as possible, but it doesn't seem really worth            
for .get_fences().

	Gustavo

Re: [RFC 1/5] dma-buf/fence: add .teardown() ops

[Gustavo Padovan]

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

> On Thu, Jun 23, 2016 at 12:29:46PM -0300, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > fence_array requires a function to clean up its state before we
> > are able to call fence_put() and release it.
> 
> An explanation along the lines of:
> 
> 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().
> 
> would help, it is not until you use it in 5/5 that it becomes apparent
> why it is needed.

That is much better explanation. Thanks!

	Gustavo

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

[Gustavo Padovan]

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

> On Thu, Jun 23, 2016 at 12:29:50PM -0300, Gustavo Padovan wrote:
> > -static void sync_file_add_pt(struct sync_file *sync_file, int *i,
> > +static int sync_file_set_fence(struct sync_file *sync_file,
> > +			       struct fence **fences)
> > +{
> > +	struct fence_array *array;
> > +
> > +	if (sync_file->num_fences == 1) {
> > +		sync_file->fence = fences[0];
> 
> Straightforward pointer assignment.
> 
> > +	} else {
> > +		array = fence_array_create(sync_file->num_fences, fences,
> > +					   fence_context_alloc(1), 1, false);
> > +		if (!array)
> > +			return -ENOMEM;
> > +
> > +		sync_file->fence = &array->base;
> 
> New reference.
> 
> Imbalance will promptly go bang after we release the single fence[0].
> 
> Would fence_array_create(1, fence) returning fence_get(fence) be too
> much of a hack?
> 
> I would suggest dropping the exported fence_get_fences() and use a local
> instead that could avoid the copy, e.g.
> 
> static struct fence *get_fences(struct fence **fence,
> 				unsigned int *num_fences)
> {
> 	if (fence_is_array(*fence)) {
> 		struct fence_array *array = to_fence_array(*fence);
> 		*num_fences = array->num_fences;
> 		return array->fences;
> 	} else {
> 		*num_fences = 1;
> 		return fence;
> 	}
> }
> 
> sync_file_merge() {
> 	int num_fences, num_a_fences, num_b_fences;
> 	struct fence **fences, **a_fences, **b_fences;
> 
> 	a_fences = get_fences(&a, &num_a_fences);
> 	b_fences = get_fences(&b, &num_b_fences);
> 
> 	num_fences = num_a_fences + num_b_fences;


Yes. That is much cleaner solution. I did this initially but then tried
to come up with .get_fences(), but that was the wrong road.

> 
> >  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);
> > +	fence_teardown(sync_file->fence);
> 
> Hmm. Could we detect the removal of the last callback and propagate that
> to the fence_array? (Rather then introduce a manual call to
> fence_teardown.)

Maybe. I'll look into ways to identify that. What I did during the
development of this patch was to have a fence_array_destroy(), but then
I moved to .teardown() in the hope to abstract the diff between fences
and fence_arrays.

	Gustavo

Re: [RFC 0/5] rework fences on struct sync_file

[Christian König]

Am 24.06.2016 um 15:17 schrieb Gustavo Padovan:
> Hi Christian,
>
> 2016-06-24 Christian König <christian.koenig@amd.com>:
>
>> Am 23.06.2016 um 17:29 schrieb 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.
>>>
>>> Two fence ops had to be created to help abstract the difference between handling
>>> fences and fences_arrays: .teardown() and .get_fences(). The former run needed
>>> on fence_array, and the latter just return a copy of all fences in the fence.
>>> I'm not so sure about adding those two, speacially .get_fences(). What do you
>>> think?
>> Clearly not a good idea to add this a fence ops, cause those are specialized
>> functions for only a certain fence implementation (the fence_array).
> Are you refering only to .get_fences()?

That comment was only for the get_fences() operation, but the teardown() 
callback looks very suspicious to me as well.

Can you explain once more why that should be necessary?

Regards,
Christian.

>
>> What you should do is try to cast the fence in your sync file using
>> to_fence_array() and then you can access the fences in the array.
> Yes, that seems a better idea I think. The initial idea was to abstract
> the difference as much as possible, but it doesn't seem really worth
> for .get_fences().
>
> 	Gustavo
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 0/5] rework fences on struct sync_file

[Gustavo Padovan]

2016-06-24 Christian König <deathsimple@vodafone.de>:

> Am 24.06.2016 um 15:17 schrieb Gustavo Padovan:
> > Hi Christian,
> > 
> > 2016-06-24 Christian König <christian.koenig@amd.com>:
> > 
> > > Am 23.06.2016 um 17:29 schrieb 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.
> > > > 
> > > > Two fence ops had to be created to help abstract the difference between handling
> > > > fences and fences_arrays: .teardown() and .get_fences(). The former run needed
> > > > on fence_array, and the latter just return a copy of all fences in the fence.
> > > > I'm not so sure about adding those two, speacially .get_fences(). What do you
> > > > think?
> > > Clearly not a good idea to add this a fence ops, cause those are specialized
> > > functions for only a certain fence implementation (the fence_array).
> > Are you refering only to .get_fences()?
> 
> That comment was only for the get_fences() operation, but the teardown()
> callback looks very suspicious to me as well.
> 
> Can you explain once more why that should be necessary?

When the sync_file owner exits we need to clean up it and that means releasing
the fence too, however with fence_array we can't just call fence_put()
as a extra reference to array->base for each fence is held when enabling
signalling. Thus we need a prior step, that I called teardown(), to
remove the callback for not signaled fences and put the extra
references.

Another way to do this would be:

	if (fence_is_array(sync_file->fence))
		fence_array_destroy(to_fence_array(sync_file->fence));
	else
		fence_put(sync_file_fence);

This would avoid the extra ops, maybe we should go this way.

	Gustavo

Re: [RFC 0/5] rework fences on struct sync_file

[Christian König]

Am 24.06.2016 um 16:59 schrieb Gustavo Padovan:
> 2016-06-24 Christian König <deathsimple@vodafone.de>:
>
>> Am 24.06.2016 um 15:17 schrieb Gustavo Padovan:
>>> Hi Christian,
>>>
>>> 2016-06-24 Christian König <christian.koenig@amd.com>:
>>>
>>>> Am 23.06.2016 um 17:29 schrieb 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.
>>>>>
>>>>> Two fence ops had to be created to help abstract the difference between handling
>>>>> fences and fences_arrays: .teardown() and .get_fences(). The former run needed
>>>>> on fence_array, and the latter just return a copy of all fences in the fence.
>>>>> I'm not so sure about adding those two, speacially .get_fences(). What do you
>>>>> think?
>>>> Clearly not a good idea to add this a fence ops, cause those are specialized
>>>> functions for only a certain fence implementation (the fence_array).
>>> Are you refering only to .get_fences()?
>> That comment was only for the get_fences() operation, but the teardown()
>> callback looks very suspicious to me as well.
>>
>> Can you explain once more why that should be necessary?
> When the sync_file owner exits we need to clean up it and that means releasing
> the fence too, however with fence_array we can't just call fence_put()
> as a extra reference to array->base for each fence is held when enabling
> signalling. Thus we need a prior step, that I called teardown(), to
> remove the callback for not signaled fences and put the extra
> references.
>
> Another way to do this would be:
>
> 	if (fence_is_array(sync_file->fence))
> 		fence_array_destroy(to_fence_array(sync_file->fence));
> 	else
> 		fence_put(sync_file_fence);
>
> This would avoid the extra ops, maybe we should go this way.

NAK on both approaches. The fence array grabs another reference on 
itself for each callback it registers, so this isn't necessary:

>         for (i = 0; i < array->num_fences; ++i) {
>                 cb[i].array = array;
>                 /*
>                  * As we may report that the fence is signaled before all
>                  * callbacks are complete, we need to take an additional
>                  * reference count on the array so that we do not free 
> it too
>                  * early. The core fence handling will only hold the 
> reference
>                  * until we signal the array as complete (but that is now
>                  * insufficient).
>                  */
>                 fence_get(&array->base);
>                 if (fence_add_callback(array->fences[i], &cb[i].cb,
>                                        fence_array_cb_func)) {
>                         fence_put(&array->base);
>                         if (atomic_dec_and_test(&array->num_pending))
>                                 return false;
>                 }
>         }

So you can just use fence_remove_callback() and then fence_put() without 
worrying about the reference.

Regards,
Christian.

>
> 	Gustavo

Re: [RFC 0/5] rework fences on struct sync_file

[Gustavo Padovan]

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

> Am 24.06.2016 um 16:59 schrieb Gustavo Padovan:
> > 2016-06-24 Christian König <deathsimple@vodafone.de>:
> > 
> > > Am 24.06.2016 um 15:17 schrieb Gustavo Padovan:
> > > > Hi Christian,
> > > > 
> > > > 2016-06-24 Christian König <christian.koenig@amd.com>:
> > > > 
> > > > > Am 23.06.2016 um 17:29 schrieb 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.
> > > > > > 
> > > > > > Two fence ops had to be created to help abstract the difference between handling
> > > > > > fences and fences_arrays: .teardown() and .get_fences(). The former run needed
> > > > > > on fence_array, and the latter just return a copy of all fences in the fence.
> > > > > > I'm not so sure about adding those two, speacially .get_fences(). What do you
> > > > > > think?
> > > > > Clearly not a good idea to add this a fence ops, cause those are specialized
> > > > > functions for only a certain fence implementation (the fence_array).
> > > > Are you refering only to .get_fences()?
> > > That comment was only for the get_fences() operation, but the teardown()
> > > callback looks very suspicious to me as well.
> > > 
> > > Can you explain once more why that should be necessary?
> > When the sync_file owner exits we need to clean up it and that means releasing
> > the fence too, however with fence_array we can't just call fence_put()
> > as a extra reference to array->base for each fence is held when enabling
> > signalling. Thus we need a prior step, that I called teardown(), to
> > remove the callback for not signaled fences and put the extra
> > references.
> > 
> > Another way to do this would be:
> > 
> > 	if (fence_is_array(sync_file->fence))
> > 		fence_array_destroy(to_fence_array(sync_file->fence));
> > 	else
> > 		fence_put(sync_file_fence);
> > 
> > This would avoid the extra ops, maybe we should go this way.
> 
> NAK on both approaches. The fence array grabs another reference on itself
> for each callback it registers, so this isn't necessary:
> 
> >         for (i = 0; i < array->num_fences; ++i) {
> >                 cb[i].array = array;
> >                 /*
> >                  * As we may report that the fence is signaled before all
> >                  * callbacks are complete, we need to take an additional
> >                  * reference count on the array so that we do not free
> > it too
> >                  * early. The core fence handling will only hold the
> > reference
> >                  * until we signal the array as complete (but that is now
> >                  * insufficient).
> >                  */
> >                 fence_get(&array->base);
> >                 if (fence_add_callback(array->fences[i], &cb[i].cb,
> >                                        fence_array_cb_func)) {
> >                         fence_put(&array->base);
> >                         if (atomic_dec_and_test(&array->num_pending))
> >                                 return false;
> >                 }
> >         }
> 
> So you can just use fence_remove_callback() and then fence_put() without
> worrying about the reference.

Yes. That is what I have in mind for fence_array_destroy() in the
snippet of code in the last e-mail. That plus the last fence_put() to
release the fence_array().

	Gustavo

Re: [RFC 1/5] dma-buf/fence: add .teardown() ops

[Daniel Vetter]

On Fri, Jun 24, 2016 at 10:19:00AM -0300, Gustavo Padovan wrote:
> 2016-06-23 Chris Wilson <chris@chris-wilson.co.uk>:
> 
> > On Thu, Jun 23, 2016 at 12:29:46PM -0300, Gustavo Padovan wrote:
> > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > 
> > > fence_array requires a function to clean up its state before we
> > > are able to call fence_put() and release it.
> > 
> > An explanation along the lines of:
> > 
> > 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().
> > 
> > would help, it is not until you use it in 5/5 that it becomes apparent
> > why it is needed.
> 
> That is much better explanation. Thanks!

What happens if the owner of the fence_array isn't the last reference
holder any more? What if there's a 2nd sync_file that now stops working
because the callbacks went poof? Some other driver that registered
callbacks?

Generally mixing refcounting with explicit teardown is really tricky,
fragile and tends to not work. This smells fishy.

Why exactly do we have a reference count loop here in the first place that
we need to break up using fence_teardown?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [RFC 3/5] dma-buf/fence: add .get_fences() ops

[Daniel Vetter]

On Thu, Jun 23, 2016 at 12:29:48PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> get_fences() should return a copy of all fences in the fence as some
> fence subclass (such as fence_array) can store more than one fence at
> time.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/dma-buf/fence.c | 14 ++++++++++++++
>  include/linux/fence.h   |  3 +++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/drivers/dma-buf/fence.c b/drivers/dma-buf/fence.c
> index 4e61afb..f4094fd 100644
> --- a/drivers/dma-buf/fence.c
> +++ b/drivers/dma-buf/fence.c
> @@ -185,6 +185,20 @@ void fence_release(struct kref *kref)
>  }
>  EXPORT_SYMBOL(fence_release);
>  

kerneldoc missing.
-Daniel

> +struct fence **fence_get_fences(struct fence *fence)
> +{
> +	if (fence->ops->get_fences) {
> +		return fence->ops->get_fences(fence);
> +	} else {
> +		struct fence **fences = kmalloc(sizeof(**fences), GFP_KERNEL);
> +		if (!fences)
> +			return NULL;
> +		fences[0] = fence;
> +		return fences;
> +	}
> +}
> +EXPORT_SYMBOL(fence_get_fences);
> +
>  void fence_teardown(struct fence *fence)
>  {
>  	if (fence->ops->teardown)
> diff --git a/include/linux/fence.h b/include/linux/fence.h
> index 1d3b671..a7a2fbc 100644
> --- a/include/linux/fence.h
> +++ b/include/linux/fence.h
> @@ -111,6 +111,7 @@ struct fence_cb {
>   * struct fence_ops - operations implemented for fence
>   * @get_driver_name: returns the driver name.
>   * @get_timeline_name: return the name of the context this fence belongs to.
> + * @get_fences: return an array with a copy of all fences in the fence.
>   * @enable_signaling: enable software signaling of fence.
>   * @signaled: [optional] peek whether the fence is signaled, can be null.
>   * @wait: custom wait implementation, or fence_default_wait.
> @@ -175,6 +176,7 @@ struct fence_cb {
>  struct fence_ops {
>  	const char * (*get_driver_name)(struct fence *fence);
>  	const char * (*get_timeline_name)(struct fence *fence);
> +	struct fence ** (*get_fences)(struct fence *fence);
>  	bool (*enable_signaling)(struct fence *fence);
>  	bool (*signaled)(struct fence *fence);
>  	signed long (*wait)(struct fence *fence, bool intr, signed long timeout);
> @@ -189,6 +191,7 @@ struct fence_ops {
>  void fence_init(struct fence *fence, const struct fence_ops *ops,
>  		spinlock_t *lock, u64 context, unsigned seqno);
>  
> +struct fence **fence_get_fences(struct fence *fence);
>  void fence_release(struct kref *kref);
>  void fence_teardown(struct fence *fence);
>  void fence_free(struct fence *fence);
> -- 
> 2.5.5
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch