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

[Gustavo Padovan]

From: Gustavo Padovan <gustavo...@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...@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

[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'.

[Chris Wilson]

On Tue, Jul 12, 2016 at 03:08:45PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo...@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

[Gustavo Padovan]

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

> On Tue, Jul 12, 2016 at 03:08:45PM -0300, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo...@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

[Chris Wilson]

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.

[Gustavo Padovan]

From: Gustavo Padovan <gustavo...@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()

v4: use user bit from fence flags (comment from Chris Wilson)

Signed-off-by: Gustavo Padovan <gustavo...@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 2873760..ec61c50 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);

@@ -274,9 +272,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;

@@ -291,7 +286,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 (test_bit(POLL_ENABLED, &sync_file->fence->flags))

+ fence_remove_callback(sync_file->fence, &sync_file->cb);
fence_put(sync_file->fence);
kfree(sync_file);
}

@@ -311,13 +307,21 @@ static unsigned int sync_file_poll(struct file *file, poll_table *wait)

poll_wait(file, &sync_file->wq, wait);

+ 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);
+ }
+
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..aa17ccf 100644
--- a/include/linux/sync_file.h
+++ b/include/linux/sync_file.h
@@ -45,6 +45,8 @@ struct sync_file {
struct fence_cb cb;
};

+#define POLL_ENABLED FENCE_FLAG_USER_BITS
+
struct sync_file *sync_file_create(struct fence *fence);
struct fence *sync_file_get_fence(int fd);

--
2.5.5

[Chris Wilson]

On Thu, Aug 04, 2016 at 11:24:14PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo...@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()
>
> v4: use user bit from fence flags (comment from Chris Wilson)
>
> Signed-off-by: Gustavo Padovan <gustavo...@collabora.co.uk>
> ---

> + 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);
> + }
> +
> status = fence_is_signaled(sync_file->fence);

status can only be true/false here.
return fence_is_signaled(sync_file->fence) ? POLLIN : 0;

Reviwed-by: Chris Wilson <ch...@chris-wilson.co.uk>

[Chris Wilson]

Reviewed-by

/me goes back to drinking his coffee