[PATCH v4 1/5] dma-buf/fence-array: add fence_is_array()

[PATCH v4 1/5] dma-buf/fence-array: add fence_is_array()

[Gustavo Padovan]

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

Add helper to check if fence is array.

v2: Comments from Chris Wilson
	- remove ternary if from ops comparison
	- add EXPORT_SYMBOL(fence_array_ops)

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

diff --git a/drivers/dma-buf/fence-array.c b/drivers/dma-buf/fence-array.c
index a8731c8..ee50022 100644
--- a/drivers/dma-buf/fence-array.c
+++ b/drivers/dma-buf/fence-array.c
@@ -99,6 +99,7 @@ const struct fence_ops fence_array_ops = {
 	.wait = fence_default_wait,
 	.release = fence_array_release,
 };
+EXPORT_SYMBOL(fence_array_ops);
 
 /**
  * fence_array_create - Create a custom fence array
diff --git a/include/linux/fence-array.h b/include/linux/fence-array.h
index 86baaa4..a44794e 100644
--- a/include/linux/fence-array.h
+++ b/include/linux/fence-array.h
@@ -52,6 +52,16 @@ struct fence_array {
 extern const struct fence_ops fence_array_ops;
 
 /**
+ * fence_is_array - check if a fence is from the array subsclass
+ *
+ * Return true if it is a fence_array and false otherwise.
+ */
+static inline bool fence_is_array(struct fence *fence)
+{
+	return fence->ops == &fence_array_ops;
+}
+
+/**
  * to_fence_array - cast a fence to a fence_array
  * @fence: fence to cast to a fence_array
  *
-- 
2.5.5

[PATCH v4 2/5] dma-buf/sync_file: refactor fence storage in struct sync_file

[Gustavo Padovan]

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

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

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

v4: fixes checkpatch warnings

v4: Comments from Chris Wilson
	- use sizeof(*fence) to reallocate array
	- fix typo in comments
	- protect num_fences sum against overflows
	- use array->base instead of casting the to struct fence

v3: Comments from Chris Wilson and Christian König
	- struct sync_file lost status member in favor of fence_is_signaled()
	- drop use of fence_array_teardown()
	- use sizeof(*fence) to allocate only an array on fence pointers

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

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

diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index 9aaa608..a223f48 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -28,11 +28,11 @@
 
 static const struct file_operations sync_file_fops;
 
-static struct sync_file *sync_file_alloc(int size)
+static struct sync_file *sync_file_alloc(void)
 {
 	struct sync_file *sync_file;
 
-	sync_file = kzalloc(size, GFP_KERNEL);
+	sync_file = kzalloc(sizeof(*sync_file), GFP_KERNEL);
 	if (!sync_file)
 		return NULL;
 
@@ -45,6 +45,8 @@ static struct sync_file *sync_file_alloc(int size)
 
 	init_waitqueue_head(&sync_file->wq);
 
+	INIT_LIST_HEAD(&sync_file->cb.node);
+
 	return sync_file;
 
 err:
@@ -54,14 +56,11 @@ 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);
+	wake_up_all(&sync_file->wq);
 }
 
 /**
@@ -76,22 +75,18 @@ struct sync_file *sync_file_create(struct fence *fence)
 {
 	struct sync_file *sync_file;
 
-	sync_file = sync_file_alloc(offsetof(struct sync_file, cbs[1]));
+	sync_file = sync_file_alloc();
 	if (!sync_file)
 		return NULL;
 
-	sync_file->num_fences = 1;
-	atomic_set(&sync_file->status, 1);
+	sync_file->fence = fence;
+
 	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))
-		atomic_dec(&sync_file->status);
+	fence_add_callback(fence, &sync_file->cb, fence_check_cb_func);
 
 	return sync_file;
 }
@@ -121,14 +116,49 @@ err:
 	return NULL;
 }
 
-static void sync_file_add_pt(struct sync_file *sync_file, int *i,
-			     struct fence *fence)
+static int sync_file_set_fence(struct sync_file *sync_file,
+			       struct fence **fences, int num_fences)
+{
+	struct fence_array *array;
+
+	/*
+	 * The reference for the fences in the new sync_file and held
+	 * in add_fence() during the merge procedure, so for num_fences == 1
+	 * we already own a new reference to the fence. For num_fence > 1
+	 * we own the reference of the fence_array creation.
+	 */
+	if (num_fences == 1) {
+		sync_file->fence = fences[0];
+	} else {
+		array = fence_array_create(num_fences, fences,
+					   fence_context_alloc(1), 1, false);
+		if (!array)
+			return -ENOMEM;
+
+		sync_file->fence = &array->base;
+	}
+
+	return 0;
+}
+
+static struct fence **get_fences(struct sync_file *sync_file, int *num_fences)
 {
-	sync_file->cbs[*i].fence = fence;
-	sync_file->cbs[*i].sync_file = sync_file;
+	if (fence_is_array(sync_file->fence)) {
+		struct fence_array *array = to_fence_array(sync_file->fence);
 
-	if (!fence_add_callback(fence, &sync_file->cbs[*i].cb,
-				fence_check_cb_func)) {
+		*num_fences = array->num_fences;
+		return array->fences;
+	}
+
+	*num_fences = 1;
+	return &sync_file->fence;
+}
+
+static void add_fence(struct fence **fences, int *i, struct fence *fence)
+{
+	fences[*i] = fence;
+
+	if (!fence_is_signaled(fence)) {
 		fence_get(fence);
 		(*i)++;
 	}
@@ -147,16 +177,24 @@ static void sync_file_add_pt(struct sync_file *sync_file, int *i,
 static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
 					 struct sync_file *b)
 {
-	int num_fences = a->num_fences + b->num_fences;
 	struct sync_file *sync_file;
-	int i, i_a, i_b;
-	unsigned long size = offsetof(struct sync_file, cbs[num_fences]);
+	struct fence **fences, **nfences, **a_fences, **b_fences;
+	int i, i_a, i_b, num_fences, a_num_fences, b_num_fences;
 
-	sync_file = sync_file_alloc(size);
+	sync_file = sync_file_alloc();
 	if (!sync_file)
 		return NULL;
 
-	atomic_set(&sync_file->status, num_fences);
+	a_fences = get_fences(a, &a_num_fences);
+	b_fences = get_fences(b, &b_num_fences);
+	if (a_num_fences > INT_MAX - b_num_fences)
+		return NULL;
+
+	num_fences = a_num_fences + b_num_fences;
+
+	fences = kcalloc(num_fences, sizeof(*fences), GFP_KERNEL);
+	if (!fences)
+		goto err;
 
 	/*
 	 * Assume sync_file a and b are both ordered and have no
@@ -165,55 +203,68 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
 	 * If a sync_file can only be created with sync_file_merge
 	 * and sync_file_create, this is a reasonable assumption.
 	 */
-	for (i = i_a = i_b = 0; i_a < a->num_fences && i_b < b->num_fences; ) {
-		struct fence *pt_a = a->cbs[i_a].fence;
-		struct fence *pt_b = b->cbs[i_b].fence;
+	for (i = i_a = i_b = 0; i_a < a_num_fences && i_b < b_num_fences; ) {
+		struct fence *pt_a = a_fences[i_a];
+		struct fence *pt_b = b_fences[i_b];
 
 		if (pt_a->context < pt_b->context) {
-			sync_file_add_pt(sync_file, &i, pt_a);
+			add_fence(fences, &i, pt_a);
 
 			i_a++;
 		} else if (pt_a->context > pt_b->context) {
-			sync_file_add_pt(sync_file, &i, pt_b);
+			add_fence(fences, &i, pt_b);
 
 			i_b++;
 		} else {
 			if (pt_a->seqno - pt_b->seqno <= INT_MAX)
-				sync_file_add_pt(sync_file, &i, pt_a);
+				add_fence(fences, &i, pt_a);
 			else
-				sync_file_add_pt(sync_file, &i, pt_b);
+				add_fence(fences, &i, pt_b);
 
 			i_a++;
 			i_b++;
 		}
 	}
 
-	for (; i_a < a->num_fences; i_a++)
-		sync_file_add_pt(sync_file, &i, a->cbs[i_a].fence);
+	for (; i_a < a_num_fences; i_a++)
+		add_fence(fences, &i, a_fences[i_a]);
+
+	for (; i_b < b_num_fences; i_b++)
+		add_fence(fences, &i, b_fences[i_b]);
+
+	if (num_fences > i) {
+		nfences = krealloc(fences, i * sizeof(*fences),
+				  GFP_KERNEL);
+		if (!nfences)
+			goto err;
 
-	for (; i_b < b->num_fences; i_b++)
-		sync_file_add_pt(sync_file, &i, b->cbs[i_b].fence);
+		fences = nfences;
+	}
+
+	if (sync_file_set_fence(sync_file, fences, i) < 0) {
+		kfree(fences);
+		goto err;
+	}
 
-	if (num_fences > i)
-		atomic_sub(num_fences - i, &sync_file->status);
-	sync_file->num_fences = i;
+	fence_add_callback(sync_file->fence, &sync_file->cb,
+			   fence_check_cb_func);
 
 	strlcpy(sync_file->name, name, sizeof(sync_file->name));
 	return sync_file;
+
+err:
+	fput(sync_file->file);
+	return NULL;
+
 }
 
 static void sync_file_free(struct kref *kref)
 {
 	struct sync_file *sync_file = container_of(kref, struct sync_file,
 						     kref);
-	int i;
-
-	for (i = 0; i < sync_file->num_fences; ++i) {
-		fence_remove_callback(sync_file->cbs[i].fence,
-				      &sync_file->cbs[i].cb);
-		fence_put(sync_file->cbs[i].fence);
-	}
 
+	fence_remove_callback(sync_file->fence, &sync_file->cb);
+	fence_put(sync_file->fence);
 	kfree(sync_file);
 }
 
@@ -232,9 +283,9 @@ static unsigned int sync_file_poll(struct file *file, poll_table *wait)
 
 	poll_wait(file, &sync_file->wq, wait);
 
-	status = atomic_read(&sync_file->status);
+	status = fence_is_signaled(sync_file->fence);
 
-	if (!status)
+	if (status)
 		return POLLIN;
 	if (status < 0)
 		return POLLERR;
@@ -315,8 +366,9 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
 {
 	struct sync_file_info info;
 	struct sync_fence_info *fence_info = NULL;
+	struct fence **fences;
 	__u32 size;
-	int ret, i;
+	int num_fences, ret, i;
 
 	if (copy_from_user(&info, (void __user *)arg, sizeof(info)))
 		return -EFAULT;
@@ -324,6 +376,8 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
 	if (info.flags || info.pad)
 		return -EINVAL;
 
+	fences = get_fences(sync_file, &num_fences);
+
 	/*
 	 * Passing num_fences = 0 means that userspace doesn't want to
 	 * retrieve any sync_fence_info. If num_fences = 0 we skip filling
@@ -333,16 +387,16 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
 	if (!info.num_fences)
 		goto no_fences;
 
-	if (info.num_fences < sync_file->num_fences)
+	if (info.num_fences < num_fences)
 		return -EINVAL;
 
-	size = sync_file->num_fences * sizeof(*fence_info);
+	size = num_fences * sizeof(*fence_info);
 	fence_info = kzalloc(size, GFP_KERNEL);
 	if (!fence_info)
 		return -ENOMEM;
 
-	for (i = 0; i < sync_file->num_fences; ++i)
-		sync_fill_fence_info(sync_file->cbs[i].fence, &fence_info[i]);
+	for (i = 0; i < num_fences; i++)
+		sync_fill_fence_info(fences[i], &fence_info[i]);
 
 	if (copy_to_user(u64_to_user_ptr(info.sync_fence_info), fence_info,
 			 size)) {
@@ -352,11 +406,8 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
 
 no_fences:
 	strlcpy(info.name, sync_file->name, sizeof(info.name));
-	info.status = atomic_read(&sync_file->status);
-	if (info.status >= 0)
-		info.status = !info.status;
-
-	info.num_fences = sync_file->num_fences;
+	info.status = fence_is_signaled(sync_file->fence);
+	info.num_fences = num_fences;
 
 	if (copy_to_user((void __user *)arg, &info, sizeof(info)))
 		ret = -EFAULT;
diff --git a/drivers/staging/android/sync_debug.c b/drivers/staging/android/sync_debug.c
index 5f57499..e07958c 100644
--- a/drivers/staging/android/sync_debug.c
+++ b/drivers/staging/android/sync_debug.c
@@ -159,10 +159,16 @@ static void sync_print_sync_file(struct seq_file *s,
 	int i;
 
 	seq_printf(s, "[%p] %s: %s\n", sync_file, sync_file->name,
-		   sync_status_str(atomic_read(&sync_file->status)));
+		   sync_status_str(!fence_is_signaled(sync_file->fence)));
 
-	for (i = 0; i < sync_file->num_fences; ++i)
-		sync_print_fence(s, sync_file->cbs[i].fence, true);
+	if (fence_is_array(sync_file->fence)) {
+		struct fence_array *array = to_fence_array(sync_file->fence);
+
+		for (i = 0; i < array->num_fences; ++i)
+			sync_print_fence(s, array->fences[i], true);
+	} else {
+		sync_print_fence(s, sync_file->fence, true);
+	}
 }
 
 static int sync_debugfs_show(struct seq_file *s, void *unused)
diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
index c6ffe8b..2efc5ec 100644
--- a/include/linux/sync_file.h
+++ b/include/linux/sync_file.h
@@ -19,12 +19,7 @@
 #include <linux/list.h>
 #include <linux/spinlock.h>
 #include <linux/fence.h>
-
-struct sync_file_cb {
-	struct fence_cb cb;
-	struct fence *fence;
-	struct sync_file *sync_file;
-};
+#include <linux/fence-array.h>
 
 /**
  * struct sync_file - sync file to export to the userspace
@@ -32,10 +27,9 @@ struct sync_file_cb {
  * @kref:		reference count on fence.
  * @name:		name of sync_file.  Useful for debugging
  * @sync_file_list:	membership in global file list
- * @num_fences:		number of sync_pts in the fence
  * @wq:			wait queue for fence signaling
- * @status:		0: signaled, >0:active, <0: error
- * @cbs:		sync_pts callback information
+ * @fence:		fence with the fences in the sync_file
+ * @cb:			fence callback information
  */
 struct sync_file {
 	struct file		*file;
@@ -44,12 +38,11 @@ struct sync_file {
 #ifdef CONFIG_DEBUG_FS
 	struct list_head	sync_file_list;
 #endif
-	int num_fences;
 
 	wait_queue_head_t	wq;
-	atomic_t		status;
 
-	struct sync_file_cb	cbs[];
+	struct fence		*fence;
+	struct fence_cb cb;
 };
 
 struct sync_file *sync_file_create(struct fence *fence);
-- 
2.5.5

Re: [PATCH v4 2/5] dma-buf/sync_file: refactor fence storage in struct sync_file

[Sumit Semwal]

Hi Greg,


On 12 July 2016 at 23:38, Gustavo Padovan <gustavo@padovan.org> wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>
> Create sync_file->fence to abstract the type of fence we are using for
> each sync_file. If only one fence is present we use a normal struct fence
> but if there is more fences to be added to the sync_file a fence_array
> is created.
>
> This change cleans up sync_file a bit. We don't need to have sync_file_cb
> array anymore. Instead, as we always have  one fence, only one fence
> callback is registered per sync_file.
>
Since this is a simple change in sync_debug,c, may I request for your
Ack so I could take it along with the other dma-buf patches?

> v4: fixes checkpatch warnings
>
> v4: Comments from Chris Wilson
>         - use sizeof(*fence) to reallocate array
>         - fix typo in comments
>         - protect num_fences sum against overflows
>         - use array->base instead of casting the to struct fence
>
> v3: Comments from Chris Wilson and Christian König
>         - struct sync_file lost status member in favor of fence_is_signaled()
>         - drop use of fence_array_teardown()
>         - use sizeof(*fence) to allocate only an array on fence pointers
>
> v2: Comments from Chris Wilson and Christian König
>         - Not using fence_ops anymore
>         - fence_is_array() was created to differentiate fence from fence_array
>         - fence_array_teardown() is now exported and used under fence_is_array()
>         - struct sync_file lost num_fences member
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Christian König <christian.koenig@amd.com>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Acked-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/dma-buf/sync_file.c          | 169 +++++++++++++++++++++++------------
>  drivers/staging/android/sync_debug.c |  12 ++-
>  include/linux/sync_file.h            |  17 ++--
>  3 files changed, 124 insertions(+), 74 deletions(-)
>
<snip>

> diff --git a/drivers/staging/android/sync_debug.c b/drivers/staging/android/sync_debug.c
> index 5f57499..e07958c 100644
> --- a/drivers/staging/android/sync_debug.c
> +++ b/drivers/staging/android/sync_debug.c
> @@ -159,10 +159,16 @@ static void sync_print_sync_file(struct seq_file *s,
>         int i;
>
>         seq_printf(s, "[%p] %s: %s\n", sync_file, sync_file->name,
> -                  sync_status_str(atomic_read(&sync_file->status)));
> +                  sync_status_str(!fence_is_signaled(sync_file->fence)));
>
> -       for (i = 0; i < sync_file->num_fences; ++i)
> -               sync_print_fence(s, sync_file->cbs[i].fence, true);
> +       if (fence_is_array(sync_file->fence)) {
> +               struct fence_array *array = to_fence_array(sync_file->fence);
> +
> +               for (i = 0; i < array->num_fences; ++i)
> +                       sync_print_fence(s, array->fences[i], true);
> +       } else {
> +               sync_print_fence(s, sync_file->fence, true);
> +       }
>  }
>
>  static int sync_debugfs_show(struct seq_file *s, void *unused)
> diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
> index c6ffe8b..2efc5ec 100644
> --- a/include/linux/sync_file.h
> +++ b/include/linux/sync_file.h
> @@ -19,12 +19,7 @@
>  #include <linux/list.h>
>  #include <linux/spinlock.h>
>  #include <linux/fence.h>
> -
> -struct sync_file_cb {
> -       struct fence_cb cb;
> -       struct fence *fence;
> -       struct sync_file *sync_file;
> -};
> +#include <linux/fence-array.h>
>
>  /**
>   * struct sync_file - sync file to export to the userspace
> @@ -32,10 +27,9 @@ struct sync_file_cb {
>   * @kref:              reference count on fence.
>   * @name:              name of sync_file.  Useful for debugging
>   * @sync_file_list:    membership in global file list
> - * @num_fences:                number of sync_pts in the fence
>   * @wq:                        wait queue for fence signaling
> - * @status:            0: signaled, >0:active, <0: error
> - * @cbs:               sync_pts callback information
> + * @fence:             fence with the fences in the sync_file
> + * @cb:                        fence callback information
>   */
>  struct sync_file {
>         struct file             *file;
> @@ -44,12 +38,11 @@ struct sync_file {
>  #ifdef CONFIG_DEBUG_FS
>         struct list_head        sync_file_list;
>  #endif
> -       int num_fences;
>
>         wait_queue_head_t       wq;
> -       atomic_t                status;
>
> -       struct sync_file_cb     cbs[];
> +       struct fence            *fence;
> +       struct fence_cb cb;
>  };
>
>  struct sync_file *sync_file_create(struct fence *fence);
> --
> 2.5.5
>

BR,
~Sumit.

-- 
Thanks and regards,

Sumit Semwal
Linaro Mobile Group - Kernel Team Lead
Linaro.org │ Open source software for ARM SoCs

Re: [PATCH v4 2/5] dma-buf/sync_file: refactor fence storage in struct sync_file

[Sumit Semwal]

(Adding Greg KH)

Hi Greg,

On 19 July 2016 at 17:45, Sumit Semwal <sumit.semwal@linaro.org> wrote:
> Hi Greg,
>
>
> On 12 July 2016 at 23:38, Gustavo Padovan <gustavo@padovan.org> wrote:
>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>
>> Create sync_file->fence to abstract the type of fence we are using for
>> each sync_file. If only one fence is present we use a normal struct fence
>> but if there is more fences to be added to the sync_file a fence_array
>> is created.
>>
>> This change cleans up sync_file a bit. We don't need to have sync_file_cb
>> array anymore. Instead, as we always have  one fence, only one fence
>> callback is registered per sync_file.
>>
> Since this is a simple change in sync_debug,c, may I request for your
> Ack so I could take it along with the other dma-buf patches?
>
Missed the fact that you weren't CCed; for this simple update to
sync_debug,c, may I request for your Ack so I can take it with dam-buf
patches?
>> v4: fixes checkpatch warnings
>>
>> v4: Comments from Chris Wilson
>>         - use sizeof(*fence) to reallocate array
>>         - fix typo in comments
>>         - protect num_fences sum against overflows
>>         - use array->base instead of casting the to struct fence
>>
>> v3: Comments from Chris Wilson and Christian König
>>         - struct sync_file lost status member in favor of fence_is_signaled()
>>         - drop use of fence_array_teardown()
>>         - use sizeof(*fence) to allocate only an array on fence pointers
>>
>> v2: Comments from Chris Wilson and Christian König
>>         - Not using fence_ops anymore
>>         - fence_is_array() was created to differentiate fence from fence_array
>>         - fence_array_teardown() is now exported and used under fence_is_array()
>>         - struct sync_file lost num_fences member
>>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Christian König <christian.koenig@amd.com>
>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Acked-by: Christian König <christian.koenig@amd.com>
>> ---
>>  drivers/dma-buf/sync_file.c          | 169 +++++++++++++++++++++++------------
>>  drivers/staging/android/sync_debug.c |  12 ++-
>>  include/linux/sync_file.h            |  17 ++--
>>  3 files changed, 124 insertions(+), 74 deletions(-)
>>
> <snip>
>
>> diff --git a/drivers/staging/android/sync_debug.c b/drivers/staging/android/sync_debug.c
>> index 5f57499..e07958c 100644
>> --- a/drivers/staging/android/sync_debug.c
>> +++ b/drivers/staging/android/sync_debug.c
>> @@ -159,10 +159,16 @@ static void sync_print_sync_file(struct seq_file *s,
>>         int i;
>>
>>         seq_printf(s, "[%p] %s: %s\n", sync_file, sync_file->name,
>> -                  sync_status_str(atomic_read(&sync_file->status)));
>> +                  sync_status_str(!fence_is_signaled(sync_file->fence)));
>>
>> -       for (i = 0; i < sync_file->num_fences; ++i)
>> -               sync_print_fence(s, sync_file->cbs[i].fence, true);
>> +       if (fence_is_array(sync_file->fence)) {
>> +               struct fence_array *array = to_fence_array(sync_file->fence);
>> +
>> +               for (i = 0; i < array->num_fences; ++i)
>> +                       sync_print_fence(s, array->fences[i], true);
>> +       } else {
>> +               sync_print_fence(s, sync_file->fence, true);
>> +       }
>>  }
>>
>>  static int sync_debugfs_show(struct seq_file *s, void *unused)
>> diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
>> index c6ffe8b..2efc5ec 100644
>> --- a/include/linux/sync_file.h
>> +++ b/include/linux/sync_file.h
>> @@ -19,12 +19,7 @@
>>  #include <linux/list.h>
>>  #include <linux/spinlock.h>
>>  #include <linux/fence.h>
>> -
>> -struct sync_file_cb {
>> -       struct fence_cb cb;
>> -       struct fence *fence;
>> -       struct sync_file *sync_file;
>> -};
>> +#include <linux/fence-array.h>
>>
>>  /**
>>   * struct sync_file - sync file to export to the userspace
>> @@ -32,10 +27,9 @@ struct sync_file_cb {
>>   * @kref:              reference count on fence.
>>   * @name:              name of sync_file.  Useful for debugging
>>   * @sync_file_list:    membership in global file list
>> - * @num_fences:                number of sync_pts in the fence
>>   * @wq:                        wait queue for fence signaling
>> - * @status:            0: signaled, >0:active, <0: error
>> - * @cbs:               sync_pts callback information
>> + * @fence:             fence with the fences in the sync_file
>> + * @cb:                        fence callback information
>>   */
>>  struct sync_file {
>>         struct file             *file;
>> @@ -44,12 +38,11 @@ struct sync_file {
>>  #ifdef CONFIG_DEBUG_FS
>>         struct list_head        sync_file_list;
>>  #endif
>> -       int num_fences;
>>
>>         wait_queue_head_t       wq;
>> -       atomic_t                status;
>>
>> -       struct sync_file_cb     cbs[];
>> +       struct fence            *fence;
>> +       struct fence_cb cb;
>>  };
>>
>>  struct sync_file *sync_file_create(struct fence *fence);
>> --
>> 2.5.5
>>
>
> BR,
> ~Sumit.
>
> --
> Thanks and regards,
>
> Sumit Semwal
> Linaro Mobile Group - Kernel Team Lead
> Linaro.org │ Open source software for ARM SoCs



-- 
Thanks and regards,

Sumit Semwal
Linaro Mobile Group - Kernel Team Lead
Linaro.org │ Open source software for ARM SoCs

Re: [PATCH v4 2/5] dma-buf/sync_file: refactor fence storage in struct sync_file

[Sumit Semwal]

Hi Greg,

On 19 July 2016 at 17:51, Sumit Semwal <sumit.semwal@linaro.org> wrote:
> (Adding Greg KH)
>
> Hi Greg,
>
> On 19 July 2016 at 17:45, Sumit Semwal <sumit.semwal@linaro.org> wrote:
>> Hi Greg,
>>
>>
>> On 12 July 2016 at 23:38, Gustavo Padovan <gustavo@padovan.org> wrote:
>>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>>
>>> Create sync_file->fence to abstract the type of fence we are using for
>>> each sync_file. If only one fence is present we use a normal struct fence
>>> but if there is more fences to be added to the sync_file a fence_array
>>> is created.
>>>
>>> This change cleans up sync_file a bit. We don't need to have sync_file_cb
>>> array anymore. Instead, as we always have  one fence, only one fence
>>> callback is registered per sync_file.
>>>
>> Since this is a simple change in sync_debug,c, may I request for your
>> Ack so I could take it along with the other dma-buf patches?
>>
> Missed the fact that you weren't CCed; for this simple update to
> sync_debug,c, may I request for your Ack so I can take it with dam-buf
> patches?

Gentle reminder please: since it's a small change, if you could Ack
it, I'd be happy to take it along with the dma-buf patches and queue
it up.

>>> v4: fixes checkpatch warnings
>>>
>>> v4: Comments from Chris Wilson
>>>         - use sizeof(*fence) to reallocate array
>>>         - fix typo in comments
>>>         - protect num_fences sum against overflows
>>>         - use array->base instead of casting the to struct fence
>>>
>>> v3: Comments from Chris Wilson and Christian König
>>>         - struct sync_file lost status member in favor of fence_is_signaled()
>>>         - drop use of fence_array_teardown()
>>>         - use sizeof(*fence) to allocate only an array on fence pointers
>>>
>>> v2: Comments from Chris Wilson and Christian König
>>>         - Not using fence_ops anymore
>>>         - fence_is_array() was created to differentiate fence from fence_array
>>>         - fence_array_teardown() is now exported and used under fence_is_array()
>>>         - struct sync_file lost num_fences member
>>>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Christian König <christian.koenig@amd.com>
>>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Acked-by: Christian König <christian.koenig@amd.com>
>>> ---
>>>  drivers/dma-buf/sync_file.c          | 169 +++++++++++++++++++++++------------
>>>  drivers/staging/android/sync_debug.c |  12 ++-
>>>  include/linux/sync_file.h            |  17 ++--
>>>  3 files changed, 124 insertions(+), 74 deletions(-)
>>>
>> <snip>
>>
>>> diff --git a/drivers/staging/android/sync_debug.c b/drivers/staging/android/sync_debug.c
>>> index 5f57499..e07958c 100644
>>> --- a/drivers/staging/android/sync_debug.c
>>> +++ b/drivers/staging/android/sync_debug.c
>>> @@ -159,10 +159,16 @@ static void sync_print_sync_file(struct seq_file *s,
>>>         int i;
>>>
>>>         seq_printf(s, "[%p] %s: %s\n", sync_file, sync_file->name,
>>> -                  sync_status_str(atomic_read(&sync_file->status)));
>>> +                  sync_status_str(!fence_is_signaled(sync_file->fence)));
>>>
>>> -       for (i = 0; i < sync_file->num_fences; ++i)
>>> -               sync_print_fence(s, sync_file->cbs[i].fence, true);
>>> +       if (fence_is_array(sync_file->fence)) {
>>> +               struct fence_array *array = to_fence_array(sync_file->fence);
>>> +
>>> +               for (i = 0; i < array->num_fences; ++i)
>>> +                       sync_print_fence(s, array->fences[i], true);
>>> +       } else {
>>> +               sync_print_fence(s, sync_file->fence, true);
>>> +       }
>>>  }
>>>
>>>  static int sync_debugfs_show(struct seq_file *s, void *unused)
>>> diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
>>> index c6ffe8b..2efc5ec 100644
>>> --- a/include/linux/sync_file.h
>>> +++ b/include/linux/sync_file.h
>>> @@ -19,12 +19,7 @@
>>>  #include <linux/list.h>
>>>  #include <linux/spinlock.h>
>>>  #include <linux/fence.h>
>>> -
>>> -struct sync_file_cb {
>>> -       struct fence_cb cb;
>>> -       struct fence *fence;
>>> -       struct sync_file *sync_file;
>>> -};
>>> +#include <linux/fence-array.h>
>>>
>>>  /**
>>>   * struct sync_file - sync file to export to the userspace
>>> @@ -32,10 +27,9 @@ struct sync_file_cb {
>>>   * @kref:              reference count on fence.
>>>   * @name:              name of sync_file.  Useful for debugging
>>>   * @sync_file_list:    membership in global file list
>>> - * @num_fences:                number of sync_pts in the fence
>>>   * @wq:                        wait queue for fence signaling
>>> - * @status:            0: signaled, >0:active, <0: error
>>> - * @cbs:               sync_pts callback information
>>> + * @fence:             fence with the fences in the sync_file
>>> + * @cb:                        fence callback information
>>>   */
>>>  struct sync_file {
>>>         struct file             *file;
>>> @@ -44,12 +38,11 @@ struct sync_file {
>>>  #ifdef CONFIG_DEBUG_FS
>>>         struct list_head        sync_file_list;
>>>  #endif
>>> -       int num_fences;
>>>
>>>         wait_queue_head_t       wq;
>>> -       atomic_t                status;
>>>
>>> -       struct sync_file_cb     cbs[];
>>> +       struct fence            *fence;
>>> +       struct fence_cb cb;
>>>  };
>>>
>>>  struct sync_file *sync_file_create(struct fence *fence);
>>> --
>>> 2.5.5
>>>
>>
>> BR,
>> ~Sumit.
>>
Best,
Sumit.

Re: [PATCH v4 2/5] dma-buf/sync_file: refactor fence storage in struct sync_file

[Greg Kroah-Hartman]

On Thu, Jul 28, 2016 at 04:30:36PM +0530, Sumit Semwal wrote:
> Hi Greg,
> 
> On 19 July 2016 at 17:51, Sumit Semwal <sumit.semwal@linaro.org> wrote:
> > (Adding Greg KH)
> >
> > Hi Greg,
> >
> > On 19 July 2016 at 17:45, Sumit Semwal <sumit.semwal@linaro.org> wrote:
> >> Hi Greg,
> >>
> >>
> >> On 12 July 2016 at 23:38, Gustavo Padovan <gustavo@padovan.org> wrote:
> >>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> >>>
> >>> Create sync_file->fence to abstract the type of fence we are using for
> >>> each sync_file. If only one fence is present we use a normal struct fence
> >>> but if there is more fences to be added to the sync_file a fence_array
> >>> is created.
> >>>
> >>> This change cleans up sync_file a bit. We don't need to have sync_file_cb
> >>> array anymore. Instead, as we always have  one fence, only one fence
> >>> callback is registered per sync_file.
> >>>
> >> Since this is a simple change in sync_debug,c, may I request for your
> >> Ack so I could take it along with the other dma-buf patches?
> >>
> > Missed the fact that you weren't CCed; for this simple update to
> > sync_debug,c, may I request for your Ack so I can take it with dam-buf
> > patches?
> 
> Gentle reminder please: since it's a small change, if you could Ack
> it, I'd be happy to take it along with the dma-buf patches and queue
> it up.

Ugh, sorry, vacation and travel and merge window hasn't been good to me
this cycle...

This is fine with me, please take it through your tree if you want to,
and again, sorry for the delay in responding.  Usually, for staging
bits, if you want/need to merge something due to an api change, no need
to wait for me, I can handle any merge conflicts / issues that might
come up after the fact.

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

thanks,

greg k-h

[PATCH v4 3/5] dma-buf/sync_file: add sync_file_get_fence()

[Gustavo Padovan]

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

Creates a function that given an sync file descriptor returns a
fence containing all fences in the sync_file.

v2: Comments by Daniel Vetter
	- Adapt to new version of fence_collection_init()
	- Hold a reference for the fence we return

v3:
	- Adapt to use fput() directly
	- rename to sync_file_get_fence() as we always return one fence

v4: Adapt to use fence_array

v5: set fence through fence_get()

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

diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index a223f48..61a687c 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -116,6 +116,29 @@ err:
 	return NULL;
 }
 
+/**
+ * sync_file_get_fence - get the fence related to the sync_file fd
+ * @fd:		sync_file fd to get the fence from
+ *
+ * Ensures @fd references a valid sync_file and returns a fence that
+ * represents all fence in the sync_file. On error NULL is returned.
+ */
+struct fence *sync_file_get_fence(int fd)
+{
+	struct sync_file *sync_file;
+	struct fence *fence;
+
+	sync_file = sync_file_fdget(fd);
+	if (!sync_file)
+		return NULL;
+
+	fence = fence_get(sync_file->fence);
+	fput(sync_file->file);
+
+	return fence;
+}
+EXPORT_SYMBOL(sync_file_get_fence);
+
 static int sync_file_set_fence(struct sync_file *sync_file,
 			       struct fence **fences, int num_fences)
 {
diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
index 2efc5ec..f7de5a0 100644
--- a/include/linux/sync_file.h
+++ b/include/linux/sync_file.h
@@ -46,5 +46,6 @@ struct sync_file {
 };
 
 struct sync_file *sync_file_create(struct fence *fence);
+struct fence *sync_file_get_fence(int fd);
 
 #endif /* _LINUX_SYNC_H */
-- 
2.5.5

[PATCH v4 4/5] Documentation: add doc for sync_file_get_fence()

[Gustavo Padovan]

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

Document the new function added to sync_file.c

v2: Adapt to fence_array

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Acked-by: Christian König <christian.koenig@amd.com>
---
 Documentation/sync_file.txt | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/sync_file.txt b/Documentation/sync_file.txt
index e8e2eba..ae2dbad1 100644
--- a/Documentation/sync_file.txt
+++ b/Documentation/sync_file.txt
@@ -64,6 +64,21 @@ The sync_file fd now can be sent to userspace.
 If the creation process fail, or the sync_file needs to be released by any
 other reason fput(sync_file->file) should be used.
 
+Receiving Sync Files from Userspace
+-----------------------------------
+
+When userspace needs to send an in-fence to the driver it pass file descriptor
+of the Sync File to the kernel. The kernel then can retrieve the fences
+from it.
+
+Interface:
+	struct fence *sync_file_get_fence(int fd);
+
+
+The function return a struct fence pointer referencing the fence(s) in the Sync
+File.
+
+
 References:
 [1] struct sync_file in include/linux/sync_file.h
 [2] All interfaces mentioned above defined in include/linux/sync_file.h
-- 
2.5.5

[PATCH v4 5/5] dma-buf/sync_file: only enable fence signalling on poll()

[Gustavo Padovan]

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

Signalling doesn't need to be enabled at sync_file creation, it is only
required if userspace waiting the fence to signal through poll().

Thus we delay fence_add_callback() until poll is called. It only adds the
callback the first time poll() is called. This avoid re-adding the same
callback multiple times.

v2: rebase and update to work with new fence support for sync_file

v3: use atomic operation to set enabled and protect fence_add_callback()

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

diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index 61a687c..240ef22 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -86,8 +86,6 @@ struct sync_file *sync_file_create(struct fence *fence)
 		 fence->ops->get_timeline_name(fence), fence->context,
 		 fence->seqno);
 
-	fence_add_callback(fence, &sync_file->cb, fence_check_cb_func);
-
 	return sync_file;
 }
 EXPORT_SYMBOL(sync_file_create);
@@ -269,9 +267,6 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
 		goto err;
 	}
 
-	fence_add_callback(sync_file->fence, &sync_file->cb,
-			   fence_check_cb_func);
-
 	strlcpy(sync_file->name, name, sizeof(sync_file->name));
 	return sync_file;
 
@@ -286,7 +281,8 @@ static void sync_file_free(struct kref *kref)
 	struct sync_file *sync_file = container_of(kref, struct sync_file,
 						     kref);
 
-	fence_remove_callback(sync_file->fence, &sync_file->cb);
+	if (atomic_read(&sync_file->enabled))
+		fence_remove_callback(sync_file->fence, &sync_file->cb);
 	fence_put(sync_file->fence);
 	kfree(sync_file);
 }
@@ -306,13 +302,21 @@ static unsigned int sync_file_poll(struct file *file, poll_table *wait)
 
 	poll_wait(file, &sync_file->wq, wait);
 
+	if (!atomic_xchg(&sync_file->enabled, 1)) {
+		if (fence_add_callback(sync_file->fence, &sync_file->cb,
+				   fence_check_cb_func) < 0)
+			wake_up_all(&sync_file->wq);
+	}
+
 	status = fence_is_signaled(sync_file->fence);
 
-	if (status)
-		return POLLIN;
+	if (!status)
+		return 0;
+
 	if (status < 0)
 		return POLLERR;
-	return 0;
+
+	return POLLIN;
 }
 
 static long sync_file_ioctl_merge(struct sync_file *sync_file,
diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
index f7de5a0..4ced13b 100644
--- a/include/linux/sync_file.h
+++ b/include/linux/sync_file.h
@@ -28,6 +28,7 @@
  * @name:		name of sync_file.  Useful for debugging
  * @sync_file_list:	membership in global file list
  * @wq:			wait queue for fence signaling
+ * @enabled:		wether fence signal is enabled or not
  * @fence:		fence with the fences in the sync_file
  * @cb:			fence callback information
  */
@@ -40,6 +41,7 @@ struct sync_file {
 #endif
 
 	wait_queue_head_t	wq;
+	atomic_t		enabled;
 
 	struct fence		*fence;
 	struct fence_cb cb;
-- 
2.5.5

Re: [PATCH v4 5/5] dma-buf/sync_file: only enable fence signalling on poll()

[John Harrison]

On 12/07/2016 19:08, Gustavo Padovan wrote:
> ...
>
> +++ b/include/linux/sync_file.h
> @@ -28,6 +28,7 @@
>    * @name:		name of sync_file.  Useful for debugging
>    * @sync_file_list:	membership in global file list
>    * @wq:			wait queue for fence signaling
> + * @enabled:		wether fence signal is enabled or not
Minor typo: should be 'whether'.

Re: [PATCH v4 5/5] dma-buf/sync_file: only enable fence signalling on poll()

[Chris Wilson]

On Tue, Jul 12, 2016 at 03:08:45PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Signalling doesn't need to be enabled at sync_file creation, it is only
> required if userspace waiting the fence to signal through poll().
> 
> Thus we delay fence_add_callback() until poll is called. It only adds the
> callback the first time poll() is called. This avoid re-adding the same
> callback multiple times.
> 
> v2: rebase and update to work with new fence support for sync_file
> 
> v3: use atomic operation to set enabled and protect fence_add_callback()

There's actually a spare bit in fence->flags you can use for this.

#define POLL_ENABLED FENCE_FLAG_USER_BITS

if (test_bit(POLL_ENABLED, &sync_file->fence->flags))
	fence_remove_callback(sync_file->fence, &sync_file->cb);

...

if (!test_and_set_bit(POLL_ENABLED, &sync_file->fence->flags)) {
	if (fence_add_callback(sync_file->fence, &sync_file->cb,
			       fence_check_cb_func) < 0)
		wake_up_all(&sync_file->wq);
}

Saves adding a raw atomic.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v4 5/5] dma-buf/sync_file: only enable fence signalling on poll()

[Gustavo Padovan]

2016-08-03 Chris Wilson <chris@chris-wilson.co.uk>:

> On Tue, Jul 12, 2016 at 03:08:45PM -0300, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > Signalling doesn't need to be enabled at sync_file creation, it is only
> > required if userspace waiting the fence to signal through poll().
> > 
> > Thus we delay fence_add_callback() until poll is called. It only adds the
> > callback the first time poll() is called. This avoid re-adding the same
> > callback multiple times.
> > 
> > v2: rebase and update to work with new fence support for sync_file
> > 
> > v3: use atomic operation to set enabled and protect fence_add_callback()
> 
> There's actually a spare bit in fence->flags you can use for this.
> 
> #define POLL_ENABLED FENCE_FLAG_USER_BITS

Wouldn't it be better to add a new bit to fence_flags_bit?

	Gustavo

Re: [PATCH v4 5/5] dma-buf/sync_file: only enable fence signalling on poll()

[Chris Wilson]

On Thu, Aug 04, 2016 at 06:18:53PM -0300, Gustavo Padovan wrote:
> 2016-08-03 Chris Wilson <chris@chris-wilson.co.uk>:
> 
> > On Tue, Jul 12, 2016 at 03:08:45PM -0300, Gustavo Padovan wrote:
> > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > 
> > > Signalling doesn't need to be enabled at sync_file creation, it is only
> > > required if userspace waiting the fence to signal through poll().
> > > 
> > > Thus we delay fence_add_callback() until poll is called. It only adds the
> > > callback the first time poll() is called. This avoid re-adding the same
> > > callback multiple times.
> > > 
> > > v2: rebase and update to work with new fence support for sync_file
> > > 
> > > v3: use atomic operation to set enabled and protect fence_add_callback()
> > 
> > There's actually a spare bit in fence->flags you can use for this.
> > 
> > #define POLL_ENABLED FENCE_FLAG_USER_BITS
> 
> Wouldn't it be better to add a new bit to fence_flags_bit?

sync_file is a user of struct fence, so it should claim one of the bits
already reserved for users. Those reserved bits are meant only for the
owner of the fence, if we did indeed need to share that bit with other
consumers of the sync_file->fence_array then adding it to
fence_flags_bits make sense. I don't see any reason at present why it
should be anything other than a private bit to sync_file atm.

Promoting it later (from private to shared) would also not be an issue.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre