Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1429926 > unrolled thread

[RFC 0/5] rework fences on struct sync_file

Back to article view | Back to linux.kernel

Contents

  [RFC 0/5] rework fences on struct sync_file Gustavo Padovan <gustavo@padovan.org> - 2016-06-23 17:40 +0200
    [RFC 3/5] dma-buf/fence: add .get_fences() ops Gustavo Padovan <gustavo@padovan.org> - 2016-06-23 17:40 +0200
      Re: [RFC 3/5] dma-buf/fence: add .get_fences() ops Chris Wilson <chris@chris-wilson.co.uk> - 2016-06-23 22:50 +0200
    [RFC 4/5] dma-buf/fence-array: add fence_array_get_fences() Gustavo Padovan <gustavo@padovan.org> - 2016-06-23 17:40 +0200
      Re: [RFC 4/5] dma-buf/fence-array: add fence_array_get_fences() Chris Wilson <chris@chris-wilson.co.uk> - 2016-06-23 22:40 +0200
    [RFC 5/5] dma-buf/sync_file: rework fence storage in struct file Gustavo Padovan <gustavo@padovan.org> - 2016-06-23 17:40 +0200
      Re: [RFC 5/5] dma-buf/sync_file: rework fence storage in struct file Chris Wilson <chris@chris-wilson.co.uk> - 2016-06-23 23:30 +0200
        Re: [RFC 5/5] dma-buf/sync_file: rework fence storage in struct file Gustavo Padovan <gustavo@padovan.org> - 2016-06-24 15:30 +0200
    [RFC 2/5] dma-buf/fence-array: add fence_array_teardown() Gustavo Padovan <gustavo@padovan.org> - 2016-06-23 17:40 +0200
    [RFC 1/5] dma-buf/fence: add .teardown() ops Gustavo Padovan <gustavo@padovan.org> - 2016-06-23 17:40 +0200
      Re: [RFC 1/5] dma-buf/fence: add .teardown() ops Chris Wilson <chris@chris-wilson.co.uk> - 2016-06-23 22:50 +0200
        Re: [RFC 1/5] dma-buf/fence: add .teardown() ops Gustavo Padovan <gustavo@padovan.org> - 2016-06-24 15:20 +0200
    Re: [RFC 0/5] rework fences on struct sync_file Gustavo Padovan <gustavo@padovan.org> - 2016-06-24 15:20 +0200
      Re: [RFC 0/5] rework fences on struct sync_file Christian König <deathsimple@vodafone.de> - 2016-06-24 16:20 +0200
        Re: [RFC 0/5] rework fences on struct sync_file Gustavo Padovan <gustavo.padovan@collabora.com> - 2016-06-24 17:00 +0200
          Re: [RFC 0/5] rework fences on struct sync_file Gustavo Padovan <gustavo.padovan@collabora.com> - 2016-06-24 17:20 +0200

#1429926 — [RFC 0/5] rework fences on struct sync_file

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1430651

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

[toc] | [prev] | [next] | [standalone]

#1430693

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

[toc] | [prev] | [next] | [standalone]

#1430738

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

[toc] | [prev] | [next] | [standalone]

#1430751

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web