[RFC 0/8] drm: explicit fencing support

[RFC 0/8] drm: explicit fencing support

[Gustavo Padovan]

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

Hi,

Currently the Linux Kernel only have an implicit fencing mechanism
where the fence are attached directly to buffers and userspace is unaware of
what is happening. On the other hand explicit fencing which is not supported
yet by Linux but it expose fences to the userspace to handle fencing between
producer/consumer explicitely.

For that we use the Android Sync Framework[1], a explicit fencing mechanism
that help the userspace handles fences directly. It has the concept of
sync_file (called sync_fence in Android) that expose the driver's fences to
userspace via file descriptors. File descriptors are useful because we can pass
them around between process.

The Sync Framework is currently in the staging tree and on the process to
be de-staged[2].

With explicit fencing we have a global mechanism that optimizes the flow of
buffers between consumers and producers, avoid a lot of waiting. So instead
of waiting for a buffer to be processed by the GPU before sending it to DRM
in an Atomic IOCTL we can get a sync_file fd from the GPU driver at the moment
we submit the buffer processing. The compositor then passes these fds to DRM in
a atomic commit request, that will not be displayed until the fences signal,
i.e, the GPU finished processing the buffer and it is ready to display. In DRM
the fences we wait on before displaying a buffer are called in-fences.

Vice-versa, we have out-fences, to sychronize the return of buffers to GPU
(producer) to be processed again. When DRM receives an atomic request with a
special flag set it generates one fence per-crtc and attach it to a per-crtc
sync_file.  It then returns the array of sync_file fds to userspace as an
atomic_ioctl out arg. With the fences available userspace can forward these
fences to the GPU, where it will wait the fence to signal before starting to
process on buffer again.

Explicit fencing with Sync Framework allows buffer suballocation. Userspace
get a large buffer and divides it into small ones and submit requests to
process them, each subbuffer gets and sync_file fd and can be processed in
parallel. This is not even possible with implicit fencing.

While these are out-fences in DRM (the consumer) they become in-fences once
they get to the GPU (the producer).

DRM explicit fences are opt-in, as the default will still be implicit fencing.
To enable explicit in-fences one just need to pass a sync_file fd in the
FENCE_FD plane property. *In-fences are per-plane*, i.e., per framebuffer.

For out-fences, just enabling DRM_MODE_ATOMIC_OUT_FENCE flag is enough.
*Out-fences are per-crtc*.

In-fences
---------

In the first discussions on #dri-devel on IRC we decided to hide the Sync
Framework from DRM drivers to reduce complexity, so as soon we get the fd
via FENCE_FD plane property we convert the sync_file fd to a struct fence.
However a sync_file might contain more than one fence, so we created the
fence_collection concept. struct fence_collection is a subclass of struct
fence and stores a group of fences that needs to be waited together, in
other words, all the fences in the sync_file.

Then we just use the already in place fence support to wait on those fences.
Once the producer calls fence_signal() for all fences on wait we can proceed
with the atomic commit and display the framebuffers. DRM drivers only needs to
be converted to struct fence to make use of this feature.

Out-fences
----------

Passing the DRM_MODE_ATOMIC_OUT_FENCE flag to an atomic request enables
out-fences. The kernel then creates a fence, attach it to a sync_file and
install this file on a unused fd for each crtc. Userspace get the fence back
as an array of per-crtc sync_file fds.

DRM core use the already in place drm_event infrastructure to help signal
fences, we've added a fence pointer to struct drm_pending_event. If the atomic
update received requested an PAGE_FLIP_EVENT we just use the same
drm_pending_event and set our fence there, otherwise we just create an event
with a NULL file_priv to set our fence. On vblank we just call fence_signal()
to signal that the buffer related to this fence is *now* on the screen.
Note that this is exactly the opposite behaviour from Android, where the fences
are signaled when they are not on display anymore, so free to be reused.

No changes are required to DRM drivers to have out-fences support, apart from
atomic support of course.

Open question
--------------

Should we use sync_timeline for out-fences? My feel is that sync_timeline
do a lot more than we need for DRM. Should we go for a small drm_timeline?
Any toughts on this?


Kernel tree
-----------

For those who want all patches on this RFC are in my tree. The tree includes
all sync frameworks patches needed at the moment:

https://git.kernel.org/cgit/linux/kernel/git/padovan/linux.git/log/?h=fences

I also hacked some poor some fake fences support to modetest here:

https://git.collabora.com/cgit/user/padovan/libdrm.git/log/?h=atomic


Regards,

	Gustavo
---

[1] https://source.android.com/devices/graphics/implement.html#vsync
[2] https://git.kernel.org/cgit/linux/kernel/git/padovan/linux.git/log/?h=sync

Gustavo Padovan (8):
  dma-buf/fence: add fence_collection fences
  dma-buf/sync_file: add sync_file_fences_get()
  drm/fence: allow fence waiting to be interrupted by userspace
  drm/fence: add in-fences support
  drm/fence: add fence to drm_pending_event
  drm/fence: create DRM_MODE_ATOMIC_OUT_FENCE flag
  drm/fence: create per-crtc sync_timeline
  drm/fence: add out-fences support

 drivers/dma-buf/Makefile            |   2 +-
 drivers/dma-buf/fence-collection.c  | 138 ++++++++++++++++++++++++++++++++++++
 drivers/dma-buf/fence.c             |   2 +-
 drivers/dma-buf/sync_file.c         |  37 ++++++++++
 drivers/gpu/drm/Kconfig             |   1 +
 drivers/gpu/drm/drm_atomic.c        | 136 +++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/drm_atomic_helper.c |   8 ++-
 drivers/gpu/drm/drm_crtc.c          |  16 +++++
 drivers/gpu/drm/drm_fops.c          |   5 +-
 drivers/gpu/drm/drm_irq.c           |   7 ++
 include/drm/drmP.h                  |   1 +
 include/drm/drm_crtc.h              |   8 +++
 include/linux/fence-collection.h    |  56 +++++++++++++++
 include/linux/fence.h               |   2 +
 include/linux/sync_file.h           |  10 +++
 include/uapi/drm/drm_mode.h         |  11 ++-
 16 files changed, 427 insertions(+), 13 deletions(-)
 create mode 100644 drivers/dma-buf/fence-collection.c
 create mode 100644 include/linux/fence-collection.h

-- 
2.5.5

[RFC 0/8] drm: explicit fencing support

[Gustavo Padovan]

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

Hi,

Currently the Linux Kernel only have an implicit fencing mechanism
where the fence are attached directly to buffers and userspace is unaware of
what is happening. On the other hand explicit fencing which is not supported
yet by Linux but it expose fences to the userspace to handle fencing between
producer/consumer explicitely.

For that we use the Android Sync Framework[1], a explicit fencing mechanism
that help the userspace handles fences directly. It has the concept of
sync_file (called sync_fence in Android) that expose the driver's fences to
userspace via file descriptors. File descriptors are useful because we can pass
them around between process.

The Sync Framework is currently in the staging tree and on the process to
be de-staged[2].

With explicit fencing we have a global mechanism that optimizes the flow of
buffers between consumers and producers, avoid a lot of waiting. So instead
of waiting for a buffer to be processed by the GPU before sending it to DRM
in an Atomic IOCTL we can get a sync_file fd from the GPU driver at the moment
we submit the buffer processing. The compositor then passes these fds to DRM in
a atomic commit request, that will not be displayed until the fences signal,
i.e, the GPU finished processing the buffer and it is ready to display. In DRM
the fences we wait on before displaying a buffer are called in-fences.

Vice-versa, we have out-fences, to sychronize the return of buffers to GPU
(producer) to be processed again. When DRM receives an atomic request with a
special flag set it generates one fence per-crtc and attach it to a per-crtc
sync_file.  It then returns the array of sync_file fds to userspace as an
atomic_ioctl out arg. With the fences available userspace can forward these
fences to the GPU, where it will wait the fence to signal before starting to
process on buffer again.

Explicit fencing with Sync Framework allows buffer suballocation. Userspace
get a large buffer and divides it into small ones and submit requests to
process them, each subbuffer gets and sync_file fd and can be processed in
parallel. This is not even possible with implicit fencing.

While these are out-fences in DRM (the consumer) they become in-fences once
they get to the GPU (the producer).

DRM explicit fences are opt-in, as the default will still be implicit fencing.
To enable explicit in-fences one just need to pass a sync_file fd in the
FENCE_FD plane property. *In-fences are per-plane*, i.e., per framebuffer.

For out-fences, just enabling DRM_MODE_ATOMIC_OUT_FENCE flag is enough.
*Out-fences are per-crtc*.

In-fences
---------

In the first discussions on #dri-devel on IRC we decided to hide the Sync
Framework from DRM drivers to reduce complexity, so as soon we get the fd
via FENCE_FD plane property we convert the sync_file fd to a struct fence.
However a sync_file might contain more than one fence, so we created the
fence_collection concept. struct fence_collection is a subclass of struct
fence and stores a group of fences that needs to be waited together, in
other words, all the fences in the sync_file.

Then we just use the already in place fence support to wait on those fences.
Once the producer calls fence_signal() for all fences on wait we can proceed
with the atomic commit and display the framebuffers. DRM drivers only needs to
be converted to struct fence to make use of this feature.

Out-fences
----------

Passing the DRM_MODE_ATOMIC_OUT_FENCE flag to an atomic request enables
out-fences. The kernel then creates a fence, attach it to a sync_file and
install this file on a unused fd for each crtc. Userspace get the fence back
as an array of per-crtc sync_file fds.

DRM core use the already in place drm_event infrastructure to help signal
fences, we've added a fence pointer to struct drm_pending_event. If the atomic
update received requested an PAGE_FLIP_EVENT we just use the same
drm_pending_event and set our fence there, otherwise we just create an event
with a NULL file_priv to set our fence. On vblank we just call fence_signal()
to signal that the buffer related to this fence is *now* on the screen.
Note that this is exactly the opposite behaviour from Android, where the fences
are signaled when they are not on display anymore, so free to be reused.

No changes are required to DRM drivers to have out-fences support, apart from
atomic support of course.

Open question
--------------

Should we use sync_timeline for out-fences? My feel is that sync_timeline
do a lot more than we need for DRM. Should we go for a small drm_timeline?
Any toughts on this?


Kernel tree
-----------

For those who want all patches on this RFC are in my tree. The tree includes
all sync frameworks patches needed at the moment:

https://git.kernel.org/cgit/linux/kernel/git/padovan/linux.git/log/?h=fences

I also hacked some poor some fake fences support to modetest here:

https://git.collabora.com/cgit/user/padovan/libdrm.git/log/?h=atomic


Regards,

	Gustavo
---

[1] https://source.android.com/devices/graphics/implement.html#vsync
[2] https://git.kernel.org/cgit/linux/kernel/git/padovan/linux.git/log/?h=sync

Gustavo Padovan (8):
  dma-buf/fence: add fence_collection fences
  dma-buf/sync_file: add sync_file_fences_get()
  drm/fence: allow fence waiting to be interrupted by userspace
  drm/fence: add in-fences support
  drm/fence: add fence to drm_pending_event
  drm/fence: create DRM_MODE_ATOMIC_OUT_FENCE flag
  drm/fence: create per-crtc sync_timeline
  drm/fence: add out-fences support

 drivers/dma-buf/Makefile            |   2 +-
 drivers/dma-buf/fence-collection.c  | 138 ++++++++++++++++++++++++++++++++++++
 drivers/dma-buf/fence.c             |   2 +-
 drivers/dma-buf/sync_file.c         |  37 ++++++++++
 drivers/gpu/drm/Kconfig             |   1 +
 drivers/gpu/drm/drm_atomic.c        | 136 +++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/drm_atomic_helper.c |   8 ++-
 drivers/gpu/drm/drm_crtc.c          |  16 +++++
 drivers/gpu/drm/drm_fops.c          |   5 +-
 drivers/gpu/drm/drm_irq.c           |   7 ++
 include/drm/drmP.h                  |   1 +
 include/drm/drm_crtc.h              |   8 +++
 include/linux/fence-collection.h    |  56 +++++++++++++++
 include/linux/fence.h               |   2 +
 include/linux/sync_file.h           |  10 +++
 include/uapi/drm/drm_mode.h         |  11 ++-
 16 files changed, 427 insertions(+), 13 deletions(-)
 create mode 100644 drivers/dma-buf/fence-collection.c
 create mode 100644 include/linux/fence-collection.h

-- 
2.5.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC 1/8] dma-buf/fence: add fence_collection fences

[Gustavo Padovan]

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

struct fence_collection inherits from struct fence and carries a
collection of fences that needs to be waited together.

It is useful to translate a sync_file to a fence to remove the complexity
of dealing with sync_files from DRM drivers. So even if there are many
fences in the sync_file that needs to waited for a commit to happen
drivers would only worry about a standard struct fence.That means that no
changes needed to any driver besides supporting fences.

fence_collection's fence doesn't belong to any timeline context.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/dma-buf/Makefile           |   2 +-
 drivers/dma-buf/fence-collection.c | 138 +++++++++++++++++++++++++++++++++++++
 drivers/dma-buf/fence.c            |   2 +-
 include/linux/fence-collection.h   |  56 +++++++++++++++
 include/linux/fence.h              |   2 +
 5 files changed, 198 insertions(+), 2 deletions(-)
 create mode 100644 drivers/dma-buf/fence-collection.c
 create mode 100644 include/linux/fence-collection.h

diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile
index 43325a1..30b8464 100644
--- a/drivers/dma-buf/Makefile
+++ b/drivers/dma-buf/Makefile
@@ -1,3 +1,3 @@
-obj-y := dma-buf.o fence.o reservation.o seqno-fence.o
+obj-y := dma-buf.o fence.o reservation.o seqno-fence.o fence-collection.o
 obj-$(CONFIG_SYNC_FILE)		+= sync_file.o sync_timeline.o sync_debug.o
 obj-$(CONFIG_SW_SYNC)		+= sw_sync.o
diff --git a/drivers/dma-buf/fence-collection.c b/drivers/dma-buf/fence-collection.c
new file mode 100644
index 0000000..8a4ecb0
--- /dev/null
+++ b/drivers/dma-buf/fence-collection.c
@@ -0,0 +1,138 @@
+/*
+ * fence-collection: aggregate fences to be waited together
+ *
+ * Copyright (C) 2016 Collabora Ltd
+ * Authors:
+ *	Gustavo Padovan <gustavo@padovan.org>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+
+#include <linux/export.h>
+#include <linux/slab.h>
+#include <linux/fence-collection.h>
+
+static const char *fence_collection_get_driver_name(struct fence *fence)
+{
+	struct fence_collection *collection = to_fence_collection(fence);
+	struct fence *f = collection->fences[0].fence;
+
+	return f->ops->get_driver_name(fence);
+}
+
+static const char *fence_collection_get_timeline_name(struct fence *fence)
+{
+	return "no context";
+}
+
+static bool fence_collection_enable_signaling(struct fence *fence)
+{
+	struct fence_collection *collection = to_fence_collection(fence);
+
+	return atomic_read(&collection->num_pending_fences);
+}
+
+static bool fence_collection_signaled(struct fence *fence)
+{
+	struct fence_collection *collection = to_fence_collection(fence);
+
+	return (atomic_read(&collection->num_pending_fences) == 0);
+}
+
+static void fence_collection_release(struct fence *fence)
+{
+	struct fence_collection *collection = to_fence_collection(fence);
+	int i;
+
+	for (i = 0 ; i < collection->num_fences ; i++) {
+		fence_remove_callback(collection->fences[i].fence,
+				      &collection->fences[i].cb);
+		fence_put(collection->fences[i].fence);
+	}
+
+	fence_free(fence);
+}
+
+static signed long fence_collection_wait(struct fence *fence, bool intr,
+					 signed long timeout)
+{
+	struct fence_collection *collection = to_fence_collection(fence);
+	int i;
+
+	for (i = 0 ; i < collection->num_fences ; i++) {
+		timeout = fence_wait(collection->fences[i].fence, intr);
+		if (timeout < 0)
+			return timeout;
+	}
+
+	return timeout;
+}
+
+static const struct fence_ops fence_collection_ops = {
+	.get_driver_name = fence_collection_get_driver_name,
+	.get_timeline_name = fence_collection_get_timeline_name,
+	.enable_signaling = fence_collection_enable_signaling,
+	.signaled = fence_collection_signaled,
+	.wait = fence_collection_wait,
+	.release = fence_collection_release,
+};
+
+static void collection_check_cb_func(struct fence *fence, struct fence_cb *cb)
+{
+	struct fence_collection_cb *f_cb;
+	struct fence_collection *collection;
+
+	f_cb = container_of(cb, struct fence_collection_cb, cb);
+	collection = f_cb->collection;
+
+	if (atomic_dec_and_test(&collection->num_pending_fences))
+		fence_signal(&collection->base);
+}
+
+void fence_collection_add(struct fence_collection *collection,
+			  struct fence *fence)
+{
+	int n = collection->num_fences;
+
+	collection->fences[n].collection = collection;
+	collection->fences[n].fence = fence;
+
+	if (fence_add_callback(fence, &collection->fences[n].cb,
+				 collection_check_cb_func))
+		return;
+
+	fence_get(fence);
+
+	collection->num_fences++;
+	atomic_inc(&collection->num_pending_fences);
+}
+
+struct fence_collection *fence_collection_init(int num_fences)
+{
+	struct fence_collection *collection;
+
+	collection = kzalloc(offsetof(struct fence_collection,
+				      fences[num_fences]), GFP_KERNEL);
+	if (!collection)
+		return NULL;
+
+	spin_lock_init(&collection->lock);
+	fence_init(&collection->base, &fence_collection_ops, &collection->lock,
+		   FENCE_NO_CONTEXT, 0);
+
+	return collection;
+}
+EXPORT_SYMBOL(fence_collection_init);
+
+void fence_collection_put(struct fence_collection *collection)
+{
+	fence_put(&collection->base);
+}
+EXPORT_SYMBOL(fence_collection_put);
diff --git a/drivers/dma-buf/fence.c b/drivers/dma-buf/fence.c
index 7b05dbe..486e95c 100644
--- a/drivers/dma-buf/fence.c
+++ b/drivers/dma-buf/fence.c
@@ -35,7 +35,7 @@ EXPORT_TRACEPOINT_SYMBOL(fence_emit);
  * context or not. One device can have multiple separate contexts,
  * and they're used if some engine can run independently of another.
  */
-static atomic_t fence_context_counter = ATOMIC_INIT(0);
+static atomic_t fence_context_counter = ATOMIC_INIT(1);
 
 /**
  * fence_context_alloc - allocate an array of fence contexts
diff --git a/include/linux/fence-collection.h b/include/linux/fence-collection.h
new file mode 100644
index 0000000..a798925
--- /dev/null
+++ b/include/linux/fence-collection.h
@@ -0,0 +1,56 @@
+/*
+ * fence-collection: aggregates fence to be waited together
+ *
+ * Copyright (C) 2016 Collabora Ltd
+ * Authors:
+ *	Gustavo Padovan <gustavo@padovan.org>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+
+#ifndef __LINUX_FENCE_COLLECTION_H
+#define __LINUX_FENCE_COLLECTION_H
+
+#include <linux/fence.h>
+
+struct fence_collection_cb {
+	struct fence_cb cb;
+	struct fence *fence;
+	struct fence_collection *collection;
+};
+
+struct fence_collection {
+	struct fence base;
+
+	spinlock_t lock;
+	struct fence_cb fence_cb;
+	atomic_t num_pending_fences;
+	int num_fences;
+	struct fence_collection_cb fences[];
+};
+
+/**
+ * to_fence_collection - cast a fence to a fence_collection
+ * @fence: fence to cast to a fence_collection
+ *
+ * Returns NULL if the fence is not a fence_collection,
+ * or the fence_collection otherwise.
+ */
+static inline struct fence_collection * to_fence_collection(struct fence *fence)
+{
+	return container_of(fence, struct fence_collection, base);
+}
+
+struct fence_collection *fence_collection_init(int num_fences);
+void fence_collection_add(struct fence_collection *collection,
+			  struct fence *fence);
+void fence_collection_put(struct fence_collection *collection);
+
+#endif /* __LINUX_FENCE_COLLECTION_H */
diff --git a/include/linux/fence.h b/include/linux/fence.h
index 2b17698..02170dd 100644
--- a/include/linux/fence.h
+++ b/include/linux/fence.h
@@ -30,6 +30,8 @@
 #include <linux/printk.h>
 #include <linux/rcupdate.h>
 
+#define FENCE_NO_CONTEXT 0
+
 struct fence;
 struct fence_ops;
 struct fence_cb;
-- 
2.5.5

[RFC 1/8] dma-buf/fence: add fence_collection fences

[Gustavo Padovan]

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

struct fence_collection inherits from struct fence and carries a
collection of fences that needs to be waited together.

It is useful to translate a sync_file to a fence to remove the complexity
of dealing with sync_files from DRM drivers. So even if there are many
fences in the sync_file that needs to waited for a commit to happen
drivers would only worry about a standard struct fence.That means that no
changes needed to any driver besides supporting fences.

fence_collection's fence doesn't belong to any timeline context.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/dma-buf/Makefile           |   2 +-
 drivers/dma-buf/fence-collection.c | 138 +++++++++++++++++++++++++++++++++++++
 drivers/dma-buf/fence.c            |   2 +-
 include/linux/fence-collection.h   |  56 +++++++++++++++
 include/linux/fence.h              |   2 +
 5 files changed, 198 insertions(+), 2 deletions(-)
 create mode 100644 drivers/dma-buf/fence-collection.c
 create mode 100644 include/linux/fence-collection.h

diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile
index 43325a1..30b8464 100644
--- a/drivers/dma-buf/Makefile
+++ b/drivers/dma-buf/Makefile
@@ -1,3 +1,3 @@
-obj-y := dma-buf.o fence.o reservation.o seqno-fence.o
+obj-y := dma-buf.o fence.o reservation.o seqno-fence.o fence-collection.o
 obj-$(CONFIG_SYNC_FILE)		+= sync_file.o sync_timeline.o sync_debug.o
 obj-$(CONFIG_SW_SYNC)		+= sw_sync.o
diff --git a/drivers/dma-buf/fence-collection.c b/drivers/dma-buf/fence-collection.c
new file mode 100644
index 0000000..8a4ecb0
--- /dev/null
+++ b/drivers/dma-buf/fence-collection.c
@@ -0,0 +1,138 @@
+/*
+ * fence-collection: aggregate fences to be waited together
+ *
+ * Copyright (C) 2016 Collabora Ltd
+ * Authors:
+ *	Gustavo Padovan <gustavo@padovan.org>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+
+#include <linux/export.h>
+#include <linux/slab.h>
+#include <linux/fence-collection.h>
+
+static const char *fence_collection_get_driver_name(struct fence *fence)
+{
+	struct fence_collection *collection = to_fence_collection(fence);
+	struct fence *f = collection->fences[0].fence;
+
+	return f->ops->get_driver_name(fence);
+}
+
+static const char *fence_collection_get_timeline_name(struct fence *fence)
+{
+	return "no context";
+}
+
+static bool fence_collection_enable_signaling(struct fence *fence)
+{
+	struct fence_collection *collection = to_fence_collection(fence);
+
+	return atomic_read(&collection->num_pending_fences);
+}
+
+static bool fence_collection_signaled(struct fence *fence)
+{
+	struct fence_collection *collection = to_fence_collection(fence);
+
+	return (atomic_read(&collection->num_pending_fences) == 0);
+}
+
+static void fence_collection_release(struct fence *fence)
+{
+	struct fence_collection *collection = to_fence_collection(fence);
+	int i;
+
+	for (i = 0 ; i < collection->num_fences ; i++) {
+		fence_remove_callback(collection->fences[i].fence,
+				      &collection->fences[i].cb);
+		fence_put(collection->fences[i].fence);
+	}
+
+	fence_free(fence);
+}
+
+static signed long fence_collection_wait(struct fence *fence, bool intr,
+					 signed long timeout)
+{
+	struct fence_collection *collection = to_fence_collection(fence);
+	int i;
+
+	for (i = 0 ; i < collection->num_fences ; i++) {
+		timeout = fence_wait(collection->fences[i].fence, intr);
+		if (timeout < 0)
+			return timeout;
+	}
+
+	return timeout;
+}
+
+static const struct fence_ops fence_collection_ops = {
+	.get_driver_name = fence_collection_get_driver_name,
+	.get_timeline_name = fence_collection_get_timeline_name,
+	.enable_signaling = fence_collection_enable_signaling,
+	.signaled = fence_collection_signaled,
+	.wait = fence_collection_wait,
+	.release = fence_collection_release,
+};
+
+static void collection_check_cb_func(struct fence *fence, struct fence_cb *cb)
+{
+	struct fence_collection_cb *f_cb;
+	struct fence_collection *collection;
+
+	f_cb = container_of(cb, struct fence_collection_cb, cb);
+	collection = f_cb->collection;
+
+	if (atomic_dec_and_test(&collection->num_pending_fences))
+		fence_signal(&collection->base);
+}
+
+void fence_collection_add(struct fence_collection *collection,
+			  struct fence *fence)
+{
+	int n = collection->num_fences;
+
+	collection->fences[n].collection = collection;
+	collection->fences[n].fence = fence;
+
+	if (fence_add_callback(fence, &collection->fences[n].cb,
+				 collection_check_cb_func))
+		return;
+
+	fence_get(fence);
+
+	collection->num_fences++;
+	atomic_inc(&collection->num_pending_fences);
+}
+
+struct fence_collection *fence_collection_init(int num_fences)
+{
+	struct fence_collection *collection;
+
+	collection = kzalloc(offsetof(struct fence_collection,
+				      fences[num_fences]), GFP_KERNEL);
+	if (!collection)
+		return NULL;
+
+	spin_lock_init(&collection->lock);
+	fence_init(&collection->base, &fence_collection_ops, &collection->lock,
+		   FENCE_NO_CONTEXT, 0);
+
+	return collection;
+}
+EXPORT_SYMBOL(fence_collection_init);
+
+void fence_collection_put(struct fence_collection *collection)
+{
+	fence_put(&collection->base);
+}
+EXPORT_SYMBOL(fence_collection_put);
diff --git a/drivers/dma-buf/fence.c b/drivers/dma-buf/fence.c
index 7b05dbe..486e95c 100644
--- a/drivers/dma-buf/fence.c
+++ b/drivers/dma-buf/fence.c
@@ -35,7 +35,7 @@ EXPORT_TRACEPOINT_SYMBOL(fence_emit);
  * context or not. One device can have multiple separate contexts,
  * and they're used if some engine can run independently of another.
  */
-static atomic_t fence_context_counter = ATOMIC_INIT(0);
+static atomic_t fence_context_counter = ATOMIC_INIT(1);
 
 /**
  * fence_context_alloc - allocate an array of fence contexts
diff --git a/include/linux/fence-collection.h b/include/linux/fence-collection.h
new file mode 100644
index 0000000..a798925
--- /dev/null
+++ b/include/linux/fence-collection.h
@@ -0,0 +1,56 @@
+/*
+ * fence-collection: aggregates fence to be waited together
+ *
+ * Copyright (C) 2016 Collabora Ltd
+ * Authors:
+ *	Gustavo Padovan <gustavo@padovan.org>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+
+#ifndef __LINUX_FENCE_COLLECTION_H
+#define __LINUX_FENCE_COLLECTION_H
+
+#include <linux/fence.h>
+
+struct fence_collection_cb {
+	struct fence_cb cb;
+	struct fence *fence;
+	struct fence_collection *collection;
+};
+
+struct fence_collection {
+	struct fence base;
+
+	spinlock_t lock;
+	struct fence_cb fence_cb;
+	atomic_t num_pending_fences;
+	int num_fences;
+	struct fence_collection_cb fences[];
+};
+
+/**
+ * to_fence_collection - cast a fence to a fence_collection
+ * @fence: fence to cast to a fence_collection
+ *
+ * Returns NULL if the fence is not a fence_collection,
+ * or the fence_collection otherwise.
+ */
+static inline struct fence_collection * to_fence_collection(struct fence *fence)
+{
+	return container_of(fence, struct fence_collection, base);
+}
+
+struct fence_collection *fence_collection_init(int num_fences);
+void fence_collection_add(struct fence_collection *collection,
+			  struct fence *fence);
+void fence_collection_put(struct fence_collection *collection);
+
+#endif /* __LINUX_FENCE_COLLECTION_H */
diff --git a/include/linux/fence.h b/include/linux/fence.h
index 2b17698..02170dd 100644
--- a/include/linux/fence.h
+++ b/include/linux/fence.h
@@ -30,6 +30,8 @@
 #include <linux/printk.h>
 #include <linux/rcupdate.h>
 
+#define FENCE_NO_CONTEXT 0
+
 struct fence;
 struct fence_ops;
 struct fence_cb;
-- 
2.5.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC 2/8] dma-buf/sync_file: add sync_file_fences_get()

[Gustavo Padovan]

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

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

If there is only one fence in the sync_file this fence itself is returned,
however if there is more than one, a fence_collection fence is returned.

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

diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index 4d2af24..926fafa 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -23,6 +23,7 @@
 #include <linux/slab.h>
 #include <linux/uaccess.h>
 #include <linux/anon_inodes.h>
+#include <linux/fence-collection.h>
 #include <linux/sync_file.h>
 #include <uapi/linux/sync_file.h>
 
@@ -122,6 +123,39 @@ void sync_file_install(struct sync_file *sync_file, int fd)
 }
 EXPORT_SYMBOL(sync_file_install);
 
+struct fence *sync_file_fences_get(int fd)
+{
+	struct sync_file *sync_file;
+	struct fence_collection *collection;
+	int i;
+
+	sync_file = sync_file_fdget(fd);
+	if (!sync_file)
+		return NULL;
+
+	if (sync_file->num_fences == 1) {
+		struct fence *fence = sync_file->cbs[0].fence;
+
+		sync_file_put(sync_file);
+		return fence;
+	}
+
+	collection = fence_collection_init(sync_file->num_fences);
+	if (!collection) {
+		sync_file_put(sync_file);
+		return NULL;
+	}
+
+	for (i = 0 ; i < sync_file->num_fences ; i++)
+		fence_collection_add(collection, sync_file->cbs[i].fence);
+
+	sync_file->collection = collection;
+	sync_file_put(sync_file);
+
+	return &collection->base;
+}
+EXPORT_SYMBOL(sync_file_fences_get);
+
 static void sync_file_add_pt(struct sync_file *sync_file, int *i,
 			     struct fence *fence)
 {
@@ -200,6 +234,9 @@ static void sync_file_free(struct kref *kref)
 						     kref);
 	int i;
 
+	if (sync_file->collection)
+		fence_collection_put(sync_file->collection);
+
 	for (i = 0; i < sync_file->num_fences; ++i) {
 		fence_remove_callback(sync_file->cbs[i].fence,
 				      &sync_file->cbs[i].cb);
diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
index 7b7a89d..2cb0486 100644
--- a/include/linux/sync_file.h
+++ b/include/linux/sync_file.h
@@ -49,6 +49,7 @@ struct sync_file {
 	wait_queue_head_t	wq;
 	atomic_t		status;
 
+	struct fence_collection *collection;
 	struct sync_file_cb	cbs[];
 };
 
@@ -103,4 +104,13 @@ void sync_file_put(struct sync_file *sync_file);
  */
 void sync_file_install(struct sync_file *sync_file, int fd);
 
+/**
+ * sync_file_fences_get - get the fence related to the fd
+ * @fd:		file descriptor to look for a fence collection
+ *
+ * Ensures @fd references a valid sync_file and returns the base object
+ * of the fence_collection that contains all fences in the sync_file
+ * or NULL in case of error.
+ */
+struct fence *sync_file_fences_get(int fd);
 #endif /* _LINUX_SYNC_H */
-- 
2.5.5

[RFC 2/8] dma-buf/sync_file: add sync_file_fences_get()

[Gustavo Padovan]

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

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

If there is only one fence in the sync_file this fence itself is returned,
however if there is more than one, a fence_collection fence is returned.

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

diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index 4d2af24..926fafa 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -23,6 +23,7 @@
 #include <linux/slab.h>
 #include <linux/uaccess.h>
 #include <linux/anon_inodes.h>
+#include <linux/fence-collection.h>
 #include <linux/sync_file.h>
 #include <uapi/linux/sync_file.h>
 
@@ -122,6 +123,39 @@ void sync_file_install(struct sync_file *sync_file, int fd)
 }
 EXPORT_SYMBOL(sync_file_install);
 
+struct fence *sync_file_fences_get(int fd)
+{
+	struct sync_file *sync_file;
+	struct fence_collection *collection;
+	int i;
+
+	sync_file = sync_file_fdget(fd);
+	if (!sync_file)
+		return NULL;
+
+	if (sync_file->num_fences == 1) {
+		struct fence *fence = sync_file->cbs[0].fence;
+
+		sync_file_put(sync_file);
+		return fence;
+	}
+
+	collection = fence_collection_init(sync_file->num_fences);
+	if (!collection) {
+		sync_file_put(sync_file);
+		return NULL;
+	}
+
+	for (i = 0 ; i < sync_file->num_fences ; i++)
+		fence_collection_add(collection, sync_file->cbs[i].fence);
+
+	sync_file->collection = collection;
+	sync_file_put(sync_file);
+
+	return &collection->base;
+}
+EXPORT_SYMBOL(sync_file_fences_get);
+
 static void sync_file_add_pt(struct sync_file *sync_file, int *i,
 			     struct fence *fence)
 {
@@ -200,6 +234,9 @@ static void sync_file_free(struct kref *kref)
 						     kref);
 	int i;
 
+	if (sync_file->collection)
+		fence_collection_put(sync_file->collection);
+
 	for (i = 0; i < sync_file->num_fences; ++i) {
 		fence_remove_callback(sync_file->cbs[i].fence,
 				      &sync_file->cbs[i].cb);
diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
index 7b7a89d..2cb0486 100644
--- a/include/linux/sync_file.h
+++ b/include/linux/sync_file.h
@@ -49,6 +49,7 @@ struct sync_file {
 	wait_queue_head_t	wq;
 	atomic_t		status;
 
+	struct fence_collection *collection;
 	struct sync_file_cb	cbs[];
 };
 
@@ -103,4 +104,13 @@ void sync_file_put(struct sync_file *sync_file);
  */
 void sync_file_install(struct sync_file *sync_file, int fd);
 
+/**
+ * sync_file_fences_get - get the fence related to the fd
+ * @fd:		file descriptor to look for a fence collection
+ *
+ * Ensures @fd references a valid sync_file and returns the base object
+ * of the fence_collection that contains all fences in the sync_file
+ * or NULL in case of error.
+ */
+struct fence *sync_file_fences_get(int fd);
 #endif /* _LINUX_SYNC_H */
-- 
2.5.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC 3/8] drm/fence: allow fence waiting to be interrupted by userspace

[Gustavo Padovan]

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

If userspace is running an synchronously atomic commit and interrupts the
atomic operation during fence_wait() it will hang until the timer expires,
so here we change the wait to be interruptible so it stop immediately when
userspace wants to quit.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/drm_atomic_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 7bf678e..f85ef8c 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1007,7 +1007,7 @@ void drm_atomic_helper_wait_for_fences(struct drm_device *dev,
 
 		WARN_ON(!plane->state->fb);
 
-		fence_wait(plane->state->fence, false);
+		fence_wait(plane->state->fence, true);
 		fence_put(plane->state->fence);
 		plane->state->fence = NULL;
 	}
-- 
2.5.5

[RFC 3/8] drm/fence: allow fence waiting to be interrupted by userspace

[Gustavo Padovan]

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

If userspace is running an synchronously atomic commit and interrupts the
atomic operation during fence_wait() it will hang until the timer expires,
so here we change the wait to be interruptible so it stop immediately when
userspace wants to quit.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/drm_atomic_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 7bf678e..f85ef8c 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1007,7 +1007,7 @@ void drm_atomic_helper_wait_for_fences(struct drm_device *dev,
 
 		WARN_ON(!plane->state->fb);
 
-		fence_wait(plane->state->fence, false);
+		fence_wait(plane->state->fence, true);
 		fence_put(plane->state->fence);
 		plane->state->fence = NULL;
 	}
-- 
2.5.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC 4/8] drm/fence: add in-fences support

[Gustavo Padovan]

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

There is now a new property called FENCE_FD attached to every plane
state that receives the sync_file fd from userspace via the atomic commit
IOCTL.

The fd is then translated to a fence (that may be a fence_collection
subclass or just a normal fence) and then used by DRM to fence_wait() for
all fences in the sync_file to signal. So it only commits when all
framebuffers are ready to scanout.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/Kconfig             | 1 +
 drivers/gpu/drm/drm_atomic.c        | 8 ++++++++
 drivers/gpu/drm/drm_atomic_helper.c | 5 +++++
 drivers/gpu/drm/drm_crtc.c          | 7 +++++++
 include/drm/drm_crtc.h              | 1 +
 5 files changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index f2a74d0..3c987e3 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -12,6 +12,7 @@ menuconfig DRM
 	select I2C
 	select I2C_ALGOBIT
 	select DMA_SHARED_BUFFER
+	select SYNC_FILE
 	help
 	  Kernel-level support for the Direct Rendering Infrastructure (DRI)
 	  introduced in XFree86 4.0. If you say Y here, you need to select
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 8ee1db8..6702502 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -30,6 +30,7 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_mode.h>
 #include <drm/drm_plane_helper.h>
+#include <linux/sync_file.h>
 
 /**
  * drm_atomic_state_default_release -
@@ -680,6 +681,11 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
 		drm_atomic_set_fb_for_plane(state, fb);
 		if (fb)
 			drm_framebuffer_unreference(fb);
+	} else if (property == config->prop_fence_fd) {
+		state->fence = sync_file_fences_get(val);
+		if (!state->fence)
+			return -EINVAL;
+		fence_get(state->fence);
 	} else if (property == config->prop_crtc_id) {
 		struct drm_crtc *crtc = drm_crtc_find(dev, val);
 		return drm_atomic_set_crtc_for_plane(state, crtc);
@@ -737,6 +743,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
 
 	if (property == config->prop_fb_id) {
 		*val = (state->fb) ? state->fb->base.id : 0;
+	} else if (property == config->prop_fence_fd) {
+		*val = -1;
 	} else if (property == config->prop_crtc_id) {
 		*val = (state->crtc) ? state->crtc->base.id : 0;
 	} else if (property == config->prop_crtc_x) {
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index f85ef8c..6ed8339 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2687,6 +2687,11 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane *plane,
 {
 	if (state->fb)
 		drm_framebuffer_unreference(state->fb);
+
+	if (state->fence) {
+		fence_put(state->fence);
+		state->fence = NULL;
+	}
 }
 EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
 
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 55ffde5..65212ce 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1278,6 +1278,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
 
 	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
 		drm_object_attach_property(&plane->base, config->prop_fb_id, 0);
+		drm_object_attach_property(&plane->base, config->prop_fence_fd, -1);
 		drm_object_attach_property(&plane->base, config->prop_crtc_id, 0);
 		drm_object_attach_property(&plane->base, config->prop_crtc_x, 0);
 		drm_object_attach_property(&plane->base, config->prop_crtc_y, 0);
@@ -1533,6 +1534,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
 		return -ENOMEM;
 	dev->mode_config.prop_fb_id = prop;
 
+	prop = drm_property_create_signed_range(dev, DRM_MODE_PROP_ATOMIC,
+			"FENCE_FD", -1, INT_MAX);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.prop_fence_fd = prop;
+
 	prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
 			"CRTC_ID", DRM_MODE_OBJECT_CRTC);
 	if (!prop)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 8cb377c..5ba3cda 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -2122,6 +2122,7 @@ struct drm_mode_config {
 	struct drm_property *prop_crtc_w;
 	struct drm_property *prop_crtc_h;
 	struct drm_property *prop_fb_id;
+	struct drm_property *prop_fence_fd;
 	struct drm_property *prop_crtc_id;
 	struct drm_property *prop_active;
 	struct drm_property *prop_mode_id;
-- 
2.5.5

[RFC 4/8] drm/fence: add in-fences support

[Gustavo Padovan]

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

There is now a new property called FENCE_FD attached to every plane
state that receives the sync_file fd from userspace via the atomic commit
IOCTL.

The fd is then translated to a fence (that may be a fence_collection
subclass or just a normal fence) and then used by DRM to fence_wait() for
all fences in the sync_file to signal. So it only commits when all
framebuffers are ready to scanout.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/Kconfig             | 1 +
 drivers/gpu/drm/drm_atomic.c        | 8 ++++++++
 drivers/gpu/drm/drm_atomic_helper.c | 5 +++++
 drivers/gpu/drm/drm_crtc.c          | 7 +++++++
 include/drm/drm_crtc.h              | 1 +
 5 files changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index f2a74d0..3c987e3 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -12,6 +12,7 @@ menuconfig DRM
 	select I2C
 	select I2C_ALGOBIT
 	select DMA_SHARED_BUFFER
+	select SYNC_FILE
 	help
 	  Kernel-level support for the Direct Rendering Infrastructure (DRI)
 	  introduced in XFree86 4.0. If you say Y here, you need to select
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 8ee1db8..6702502 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -30,6 +30,7 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_mode.h>
 #include <drm/drm_plane_helper.h>
+#include <linux/sync_file.h>
 
 /**
  * drm_atomic_state_default_release -
@@ -680,6 +681,11 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
 		drm_atomic_set_fb_for_plane(state, fb);
 		if (fb)
 			drm_framebuffer_unreference(fb);
+	} else if (property == config->prop_fence_fd) {
+		state->fence = sync_file_fences_get(val);
+		if (!state->fence)
+			return -EINVAL;
+		fence_get(state->fence);
 	} else if (property == config->prop_crtc_id) {
 		struct drm_crtc *crtc = drm_crtc_find(dev, val);
 		return drm_atomic_set_crtc_for_plane(state, crtc);
@@ -737,6 +743,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
 
 	if (property == config->prop_fb_id) {
 		*val = (state->fb) ? state->fb->base.id : 0;
+	} else if (property == config->prop_fence_fd) {
+		*val = -1;
 	} else if (property == config->prop_crtc_id) {
 		*val = (state->crtc) ? state->crtc->base.id : 0;
 	} else if (property == config->prop_crtc_x) {
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index f85ef8c..6ed8339 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2687,6 +2687,11 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane *plane,
 {
 	if (state->fb)
 		drm_framebuffer_unreference(state->fb);
+
+	if (state->fence) {
+		fence_put(state->fence);
+		state->fence = NULL;
+	}
 }
 EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
 
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 55ffde5..65212ce 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1278,6 +1278,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
 
 	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
 		drm_object_attach_property(&plane->base, config->prop_fb_id, 0);
+		drm_object_attach_property(&plane->base, config->prop_fence_fd, -1);
 		drm_object_attach_property(&plane->base, config->prop_crtc_id, 0);
 		drm_object_attach_property(&plane->base, config->prop_crtc_x, 0);
 		drm_object_attach_property(&plane->base, config->prop_crtc_y, 0);
@@ -1533,6 +1534,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
 		return -ENOMEM;
 	dev->mode_config.prop_fb_id = prop;
 
+	prop = drm_property_create_signed_range(dev, DRM_MODE_PROP_ATOMIC,
+			"FENCE_FD", -1, INT_MAX);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.prop_fence_fd = prop;
+
 	prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
 			"CRTC_ID", DRM_MODE_OBJECT_CRTC);
 	if (!prop)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 8cb377c..5ba3cda 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -2122,6 +2122,7 @@ struct drm_mode_config {
 	struct drm_property *prop_crtc_w;
 	struct drm_property *prop_crtc_h;
 	struct drm_property *prop_fb_id;
+	struct drm_property *prop_fence_fd;
 	struct drm_property *prop_crtc_id;
 	struct drm_property *prop_active;
 	struct drm_property *prop_mode_id;
-- 
2.5.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC 5/8] drm/fence: add fence to drm_pending_event

[Gustavo Padovan]

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

Now a drm_pending_event can either send a real drm_event or signal a
fence, or both. It allow us to signal via fences when the buffer is
displayed on the screen. Which in turn means that the previous buffer
is not in use anymore and can be freed or sent back to another driver
for processing.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/drm_atomic.c | 19 +++++++++++++------
 drivers/gpu/drm/drm_fops.c   |  5 +++--
 drivers/gpu/drm/drm_irq.c    |  7 +++++++
 include/drm/drmP.h           |  1 +
 4 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 6702502..0b95526 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1431,7 +1431,8 @@ EXPORT_SYMBOL(drm_atomic_async_commit);
  */
 
 static struct drm_pending_vblank_event *create_vblank_event(
-		struct drm_device *dev, struct drm_file *file_priv, uint64_t user_data)
+		struct drm_device *dev, struct drm_file *file_priv,
+		struct fence *fence, uint64_t user_data)
 {
 	struct drm_pending_vblank_event *e = NULL;
 	int ret;
@@ -1444,12 +1445,17 @@ static struct drm_pending_vblank_event *create_vblank_event(
 	e->event.base.length = sizeof(e->event);
 	e->event.user_data = user_data;
 
-	ret = drm_event_reserve_init(dev, file_priv, &e->base, &e->event.base);
-	if (ret) {
-		kfree(e);
-		return NULL;
+	if (file_priv) {
+		ret = drm_event_reserve_init(dev, file_priv, &e->base,
+					     &e->event.base);
+		if (ret) {
+			kfree(e);
+			return NULL;
+		}
 	}
 
+	e->base.fence = fence;
+
 	return e;
 }
 
@@ -1676,7 +1682,8 @@ retry:
 		for_each_crtc_in_state(state, crtc, crtc_state, i) {
 			struct drm_pending_vblank_event *e;
 
-			e = create_vblank_event(dev, file_priv, arg->user_data);
+			e = create_vblank_event(dev, file_priv, NULL,
+						arg->user_data);
 			if (!e) {
 				ret = -ENOMEM;
 				goto out;
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index aeef58e..38def49 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -801,8 +801,9 @@ void drm_send_event_locked(struct drm_device *dev, struct drm_pending_event *e)
 {
 	assert_spin_locked(&dev->event_lock);
 
-	if (!e->file_priv) {
-		e->destroy(e);
+	if (!e->file_priv || !e->event) {
+		if (e->destroy)
+			e->destroy(e);
 		return;
 	}
 
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 3c1a6f1..0c5d7cb 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -41,6 +41,7 @@
 
 #include <linux/vgaarb.h>
 #include <linux/export.h>
+#include <linux/fence.h>
 
 /* Access macro for slots in vblank timestamp ringbuffer. */
 #define vblanktimestamp(dev, pipe, count) \
@@ -1124,6 +1125,12 @@ void drm_send_vblank_event(struct drm_device *dev, unsigned int pipe,
 		now = get_drm_timestamp();
 	}
 	e->pipe = pipe;
+
+	if (e->base.fence) {
+		fence_signal(e->base.fence);
+		fence_put(e->base.fence);
+	}
+
 	send_vblank_event(dev, e, seq, &now);
 }
 EXPORT_SYMBOL(drm_send_vblank_event);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 3c8422c..8f83c2a 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -282,6 +282,7 @@ struct drm_ioctl_desc {
 /* Event queued up for userspace to read */
 struct drm_pending_event {
 	struct drm_event *event;
+	struct fence *fence;
 	struct list_head link;
 	struct list_head pending_link;
 	struct drm_file *file_priv;
-- 
2.5.5

[RFC 5/8] drm/fence: add fence to drm_pending_event

[Gustavo Padovan]

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

Now a drm_pending_event can either send a real drm_event or signal a
fence, or both. It allow us to signal via fences when the buffer is
displayed on the screen. Which in turn means that the previous buffer
is not in use anymore and can be freed or sent back to another driver
for processing.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/drm_atomic.c | 19 +++++++++++++------
 drivers/gpu/drm/drm_fops.c   |  5 +++--
 drivers/gpu/drm/drm_irq.c    |  7 +++++++
 include/drm/drmP.h           |  1 +
 4 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 6702502..0b95526 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1431,7 +1431,8 @@ EXPORT_SYMBOL(drm_atomic_async_commit);
  */
 
 static struct drm_pending_vblank_event *create_vblank_event(
-		struct drm_device *dev, struct drm_file *file_priv, uint64_t user_data)
+		struct drm_device *dev, struct drm_file *file_priv,
+		struct fence *fence, uint64_t user_data)
 {
 	struct drm_pending_vblank_event *e = NULL;
 	int ret;
@@ -1444,12 +1445,17 @@ static struct drm_pending_vblank_event *create_vblank_event(
 	e->event.base.length = sizeof(e->event);
 	e->event.user_data = user_data;
 
-	ret = drm_event_reserve_init(dev, file_priv, &e->base, &e->event.base);
-	if (ret) {
-		kfree(e);
-		return NULL;
+	if (file_priv) {
+		ret = drm_event_reserve_init(dev, file_priv, &e->base,
+					     &e->event.base);
+		if (ret) {
+			kfree(e);
+			return NULL;
+		}
 	}
 
+	e->base.fence = fence;
+
 	return e;
 }
 
@@ -1676,7 +1682,8 @@ retry:
 		for_each_crtc_in_state(state, crtc, crtc_state, i) {
 			struct drm_pending_vblank_event *e;
 
-			e = create_vblank_event(dev, file_priv, arg->user_data);
+			e = create_vblank_event(dev, file_priv, NULL,
+						arg->user_data);
 			if (!e) {
 				ret = -ENOMEM;
 				goto out;
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index aeef58e..38def49 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -801,8 +801,9 @@ void drm_send_event_locked(struct drm_device *dev, struct drm_pending_event *e)
 {
 	assert_spin_locked(&dev->event_lock);
 
-	if (!e->file_priv) {
-		e->destroy(e);
+	if (!e->file_priv || !e->event) {
+		if (e->destroy)
+			e->destroy(e);
 		return;
 	}
 
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 3c1a6f1..0c5d7cb 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -41,6 +41,7 @@
 
 #include <linux/vgaarb.h>
 #include <linux/export.h>
+#include <linux/fence.h>
 
 /* Access macro for slots in vblank timestamp ringbuffer. */
 #define vblanktimestamp(dev, pipe, count) \
@@ -1124,6 +1125,12 @@ void drm_send_vblank_event(struct drm_device *dev, unsigned int pipe,
 		now = get_drm_timestamp();
 	}
 	e->pipe = pipe;
+
+	if (e->base.fence) {
+		fence_signal(e->base.fence);
+		fence_put(e->base.fence);
+	}
+
 	send_vblank_event(dev, e, seq, &now);
 }
 EXPORT_SYMBOL(drm_send_vblank_event);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 3c8422c..8f83c2a 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -282,6 +282,7 @@ struct drm_ioctl_desc {
 /* Event queued up for userspace to read */
 struct drm_pending_event {
 	struct drm_event *event;
+	struct fence *fence;
 	struct list_head link;
 	struct list_head pending_link;
 	struct drm_file *file_priv;
-- 
2.5.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC 6/8] drm/fence: create DRM_MODE_ATOMIC_OUT_FENCE flag

[Gustavo Padovan]

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

This flag tells drm_atomic_ioctl that we want to get a per-crtc out-fence
fd back.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 include/uapi/drm/drm_mode.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 7a7856e..39905cc 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -582,13 +582,15 @@ struct drm_mode_destroy_dumb {
 #define DRM_MODE_ATOMIC_TEST_ONLY 0x0100
 #define DRM_MODE_ATOMIC_NONBLOCK  0x0200
 #define DRM_MODE_ATOMIC_ALLOW_MODESET 0x0400
+#define DRM_MODE_ATOMIC_OUT_FENCE 0x0800
 
 #define DRM_MODE_ATOMIC_FLAGS (\
 		DRM_MODE_PAGE_FLIP_EVENT |\
 		DRM_MODE_PAGE_FLIP_ASYNC |\
 		DRM_MODE_ATOMIC_TEST_ONLY |\
 		DRM_MODE_ATOMIC_NONBLOCK |\
-		DRM_MODE_ATOMIC_ALLOW_MODESET)
+		DRM_MODE_ATOMIC_ALLOW_MODESET |\
+		DRM_MODE_ATOMIC_OUT_FENCE)
 
 struct drm_mode_atomic {
 	__u32 flags;
-- 
2.5.5

[RFC 6/8] drm/fence: create DRM_MODE_ATOMIC_OUT_FENCE flag

[Gustavo Padovan]

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

This flag tells drm_atomic_ioctl that we want to get a per-crtc out-fence
fd back.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 include/uapi/drm/drm_mode.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 7a7856e..39905cc 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -582,13 +582,15 @@ struct drm_mode_destroy_dumb {
 #define DRM_MODE_ATOMIC_TEST_ONLY 0x0100
 #define DRM_MODE_ATOMIC_NONBLOCK  0x0200
 #define DRM_MODE_ATOMIC_ALLOW_MODESET 0x0400
+#define DRM_MODE_ATOMIC_OUT_FENCE 0x0800
 
 #define DRM_MODE_ATOMIC_FLAGS (\
 		DRM_MODE_PAGE_FLIP_EVENT |\
 		DRM_MODE_PAGE_FLIP_ASYNC |\
 		DRM_MODE_ATOMIC_TEST_ONLY |\
 		DRM_MODE_ATOMIC_NONBLOCK |\
-		DRM_MODE_ATOMIC_ALLOW_MODESET)
+		DRM_MODE_ATOMIC_ALLOW_MODESET |\
+		DRM_MODE_ATOMIC_OUT_FENCE)
 
 struct drm_mode_atomic {
 	__u32 flags;
-- 
2.5.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC 7/8] drm/fence: create per-crtc sync_timeline

[Gustavo Padovan]

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

Add one timeline per-crtc that will be use to handle fence signalling
to userspace via sync_files.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/drm_crtc.c | 9 +++++++++
 include/drm/drm_crtc.h     | 4 ++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 65212ce..e1204cf 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -709,6 +709,13 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
 		return -ENOMEM;
 	}
 
+	crtc->timeline = sync_timeline_create(dev->driver->name, crtc->name);
+	if (!crtc->timeline) {
+		drm_mode_object_put(dev, &crtc->base);
+		kfree(crtc->name);
+		return -ENOMEM;
+	}
+
 	crtc->base.properties = &crtc->properties;
 
 	list_add_tail(&crtc->head, &config->crtc_list);
@@ -755,6 +762,8 @@ void drm_crtc_cleanup(struct drm_crtc *crtc)
 	if (crtc->state && crtc->funcs->atomic_destroy_state)
 		crtc->funcs->atomic_destroy_state(crtc, crtc->state);
 
+	sync_timeline_destroy(crtc->timeline);
+
 	kfree(crtc->name);
 
 	memset(crtc, 0, sizeof(*crtc));
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 5ba3cda..7934178 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -32,6 +32,7 @@
 #include <linux/fb.h>
 #include <linux/hdmi.h>
 #include <linux/media-bus-format.h>
+#include <linux/sync_timeline.h>
 #include <uapi/drm/drm_mode.h>
 #include <uapi/drm/drm_fourcc.h>
 #include <drm/drm_modeset_lock.h>
@@ -715,6 +716,7 @@ struct drm_crtc_funcs {
  * @helper_private: mid-layer private data
  * @properties: property tracking for this CRTC
  * @state: current atomic state for this CRTC
+ * @timeline: sync timeline for fence sigalling
  * @acquire_ctx: per-CRTC implicit acquire context used by atomic drivers for
  * 	legacy IOCTLs
  *
@@ -771,6 +773,8 @@ struct drm_crtc {
 
 	struct drm_crtc_state *state;
 
+	struct sync_timeline *timeline;
+
 	/*
 	 * For legacy crtc IOCTLs so that atomic drivers can get at the locking
 	 * acquire context.
-- 
2.5.5

[RFC 7/8] drm/fence: create per-crtc sync_timeline

[Gustavo Padovan]

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

Add one timeline per-crtc that will be use to handle fence signalling
to userspace via sync_files.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/drm_crtc.c | 9 +++++++++
 include/drm/drm_crtc.h     | 4 ++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 65212ce..e1204cf 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -709,6 +709,13 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
 		return -ENOMEM;
 	}
 
+	crtc->timeline = sync_timeline_create(dev->driver->name, crtc->name);
+	if (!crtc->timeline) {
+		drm_mode_object_put(dev, &crtc->base);
+		kfree(crtc->name);
+		return -ENOMEM;
+	}
+
 	crtc->base.properties = &crtc->properties;
 
 	list_add_tail(&crtc->head, &config->crtc_list);
@@ -755,6 +762,8 @@ void drm_crtc_cleanup(struct drm_crtc *crtc)
 	if (crtc->state && crtc->funcs->atomic_destroy_state)
 		crtc->funcs->atomic_destroy_state(crtc, crtc->state);
 
+	sync_timeline_destroy(crtc->timeline);
+
 	kfree(crtc->name);
 
 	memset(crtc, 0, sizeof(*crtc));
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 5ba3cda..7934178 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -32,6 +32,7 @@
 #include <linux/fb.h>
 #include <linux/hdmi.h>
 #include <linux/media-bus-format.h>
+#include <linux/sync_timeline.h>
 #include <uapi/drm/drm_mode.h>
 #include <uapi/drm/drm_fourcc.h>
 #include <drm/drm_modeset_lock.h>
@@ -715,6 +716,7 @@ struct drm_crtc_funcs {
  * @helper_private: mid-layer private data
  * @properties: property tracking for this CRTC
  * @state: current atomic state for this CRTC
+ * @timeline: sync timeline for fence sigalling
  * @acquire_ctx: per-CRTC implicit acquire context used by atomic drivers for
  * 	legacy IOCTLs
  *
@@ -771,6 +773,8 @@ struct drm_crtc {
 
 	struct drm_crtc_state *state;
 
+	struct sync_timeline *timeline;
+
 	/*
 	 * For legacy crtc IOCTLs so that atomic drivers can get at the locking
 	 * acquire context.
-- 
2.5.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC 8/8] drm/fence: add out-fences support

[Gustavo Padovan]

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

Support DRM out-fences creating a sync_file with a fence for each crtc
update with the DRM_MODE_ATOMIC_OUT_FENCE flag.

We then send an struct drm_out_fences array with the out-fences fds back in
the drm_atomic_ioctl() as an out arg in the out_fences_ptr field.

struct drm_out_fences {
	__u32   crtc_id;
	__u32   fd;
};

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/drm_atomic.c        | 109 +++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/drm_atomic_helper.c |   1 +
 include/drm/drm_crtc.h              |   3 +
 include/uapi/drm/drm_mode.h         |   7 +++
 4 files changed, 119 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 0b95526..af6e051 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1560,6 +1560,103 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_atomic_clean_old_fb);
 
+static int drm_atomic_get_out_fences(struct drm_device *dev,
+				     struct drm_atomic_state *state,
+				     uint32_t __user *out_fences_ptr,
+				     uint64_t count_out_fences,
+				     uint64_t user_data)
+{
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
+	struct drm_out_fences *out_fences;
+	struct sync_file **sync_file;
+	int num_fences = 0;
+	int i, ret;
+
+	out_fences = kcalloc(count_out_fences, sizeof(*out_fences),
+			     GFP_KERNEL);
+	if (!out_fences)
+		return -ENOMEM;
+
+	sync_file = kcalloc(count_out_fences, sizeof(*sync_file),
+			     GFP_KERNEL);
+	if (!sync_file) {
+		kfree(out_fences);
+		return -ENOMEM;
+	}
+
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		struct drm_pending_vblank_event *e;
+		struct fence *fence;
+		char name[32];
+		int fd;
+
+		fence = sync_timeline_create_fence(crtc->timeline,
+						   crtc->fence_seqno);
+		if (!fence) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		snprintf(name, sizeof(name), "crtc-%d_%lu",
+			 drm_crtc_index(crtc), crtc->fence_seqno++);
+
+		sync_file[i] = sync_file_create(name, fence);
+		if(!sync_file[i]) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		fd = get_unused_fd_flags(O_CLOEXEC);
+		if (fd < 0) {
+			ret = fd;
+			goto out;
+		}
+
+		sync_file_install(sync_file[i], fd);
+
+		if (crtc_state->event) {
+			crtc_state->event->base.fence = fence;
+		} else {
+			e = create_vblank_event(dev, NULL, fence, user_data);
+			if (!e) {
+				put_unused_fd(fd);
+				ret = -ENOMEM;
+				goto out;
+			}
+
+			crtc_state->event = e;
+		}
+		if (num_fences > count_out_fences) {
+			put_unused_fd(fd);
+			ret = -EINVAL;
+			goto out;
+		}
+
+		fence_get(fence);
+
+		out_fences[num_fences].crtc_id = crtc->base.id;
+		out_fences[num_fences].fd = fd;
+		num_fences++;
+	}
+
+	if (copy_to_user(out_fences_ptr, out_fences,
+			 num_fences * sizeof(*out_fences))) {
+		ret = -EFAULT;
+		goto out;
+	}
+
+	return 0;
+
+out:
+	for (i = 0 ; i < count_out_fences ; i++) {
+		if (sync_file[i])
+			sync_file_put(sync_file[i]);
+	}
+
+	return ret;
+}
+
 int drm_mode_atomic_ioctl(struct drm_device *dev,
 			  void *data, struct drm_file *file_priv)
 {
@@ -1568,6 +1665,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	uint32_t __user *count_props_ptr = (uint32_t __user *)(unsigned long)(arg->count_props_ptr);
 	uint32_t __user *props_ptr = (uint32_t __user *)(unsigned long)(arg->props_ptr);
 	uint64_t __user *prop_values_ptr = (uint64_t __user *)(unsigned long)(arg->prop_values_ptr);
+	uint32_t __user *out_fences_ptr = (uint32_t __user *)(unsigned long)(arg->out_fences_ptr);
 	unsigned int copied_objs, copied_props;
 	struct drm_atomic_state *state;
 	struct drm_modeset_acquire_ctx ctx;
@@ -1601,7 +1699,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 
 	/* can't test and expect an event at the same time. */
 	if ((arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) &&
-			(arg->flags & DRM_MODE_PAGE_FLIP_EVENT))
+			(arg->flags & (DRM_MODE_PAGE_FLIP_EVENT
+			 | DRM_MODE_ATOMIC_OUT_FENCE)))
 		return -EINVAL;
 
 	drm_modeset_acquire_init(&ctx, 0);
@@ -1693,6 +1792,14 @@ retry:
 		}
 	}
 
+	if (arg->flags & DRM_MODE_ATOMIC_OUT_FENCE) {
+		ret = drm_atomic_get_out_fences(dev, state, out_fences_ptr,
+						arg->count_out_fences,
+						arg->user_data);
+		if (ret < 0)
+			goto out;
+	}
+
 	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
 		/*
 		 * Unlike commit, check_only does not clean up state.
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 6ed8339..15ba3a8 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -31,6 +31,7 @@
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_atomic_helper.h>
 #include <linux/fence.h>
+#include <linux/sync_file.h>
 
 /**
  * DOC: overview
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 7934178..53e4e71 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -717,6 +717,7 @@ struct drm_crtc_funcs {
  * @properties: property tracking for this CRTC
  * @state: current atomic state for this CRTC
  * @timeline: sync timeline for fence sigalling
+ * @fence_seqno: seqno variable to create fences
  * @acquire_ctx: per-CRTC implicit acquire context used by atomic drivers for
  * 	legacy IOCTLs
  *
@@ -773,7 +774,9 @@ struct drm_crtc {
 
 	struct drm_crtc_state *state;
 
+	/* for out-fences */
 	struct sync_timeline *timeline;
+	unsigned long fence_seqno;
 
 	/*
 	 * For legacy crtc IOCTLs so that atomic drivers can get at the locking
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 39905cc..4cdcd22 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -592,6 +592,11 @@ struct drm_mode_destroy_dumb {
 		DRM_MODE_ATOMIC_ALLOW_MODESET |\
 		DRM_MODE_ATOMIC_OUT_FENCE)
 
+struct drm_out_fences {
+	__u32	crtc_id;
+	__u32	fd;
+};
+
 struct drm_mode_atomic {
 	__u32 flags;
 	__u32 count_objs;
@@ -601,6 +606,8 @@ struct drm_mode_atomic {
 	__u64 prop_values_ptr;
 	__u64 reserved;
 	__u64 user_data;
+	__u64 count_out_fences;
+	__u64 out_fences_ptr;
 };
 
 /**
-- 
2.5.5

[RFC 8/8] drm/fence: add out-fences support

[Gustavo Padovan]

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

Support DRM out-fences creating a sync_file with a fence for each crtc
update with the DRM_MODE_ATOMIC_OUT_FENCE flag.

We then send an struct drm_out_fences array with the out-fences fds back in
the drm_atomic_ioctl() as an out arg in the out_fences_ptr field.

struct drm_out_fences {
	__u32   crtc_id;
	__u32   fd;
};

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/drm_atomic.c        | 109 +++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/drm_atomic_helper.c |   1 +
 include/drm/drm_crtc.h              |   3 +
 include/uapi/drm/drm_mode.h         |   7 +++
 4 files changed, 119 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 0b95526..af6e051 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1560,6 +1560,103 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_atomic_clean_old_fb);
 
+static int drm_atomic_get_out_fences(struct drm_device *dev,
+				     struct drm_atomic_state *state,
+				     uint32_t __user *out_fences_ptr,
+				     uint64_t count_out_fences,
+				     uint64_t user_data)
+{
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
+	struct drm_out_fences *out_fences;
+	struct sync_file **sync_file;
+	int num_fences = 0;
+	int i, ret;
+
+	out_fences = kcalloc(count_out_fences, sizeof(*out_fences),
+			     GFP_KERNEL);
+	if (!out_fences)
+		return -ENOMEM;
+
+	sync_file = kcalloc(count_out_fences, sizeof(*sync_file),
+			     GFP_KERNEL);
+	if (!sync_file) {
+		kfree(out_fences);
+		return -ENOMEM;
+	}
+
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		struct drm_pending_vblank_event *e;
+		struct fence *fence;
+		char name[32];
+		int fd;
+
+		fence = sync_timeline_create_fence(crtc->timeline,
+						   crtc->fence_seqno);
+		if (!fence) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		snprintf(name, sizeof(name), "crtc-%d_%lu",
+			 drm_crtc_index(crtc), crtc->fence_seqno++);
+
+		sync_file[i] = sync_file_create(name, fence);
+		if(!sync_file[i]) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		fd = get_unused_fd_flags(O_CLOEXEC);
+		if (fd < 0) {
+			ret = fd;
+			goto out;
+		}
+
+		sync_file_install(sync_file[i], fd);
+
+		if (crtc_state->event) {
+			crtc_state->event->base.fence = fence;
+		} else {
+			e = create_vblank_event(dev, NULL, fence, user_data);
+			if (!e) {
+				put_unused_fd(fd);
+				ret = -ENOMEM;
+				goto out;
+			}
+
+			crtc_state->event = e;
+		}
+		if (num_fences > count_out_fences) {
+			put_unused_fd(fd);
+			ret = -EINVAL;
+			goto out;
+		}
+
+		fence_get(fence);
+
+		out_fences[num_fences].crtc_id = crtc->base.id;
+		out_fences[num_fences].fd = fd;
+		num_fences++;
+	}
+
+	if (copy_to_user(out_fences_ptr, out_fences,
+			 num_fences * sizeof(*out_fences))) {
+		ret = -EFAULT;
+		goto out;
+	}
+
+	return 0;
+
+out:
+	for (i = 0 ; i < count_out_fences ; i++) {
+		if (sync_file[i])
+			sync_file_put(sync_file[i]);
+	}
+
+	return ret;
+}
+
 int drm_mode_atomic_ioctl(struct drm_device *dev,
 			  void *data, struct drm_file *file_priv)
 {
@@ -1568,6 +1665,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	uint32_t __user *count_props_ptr = (uint32_t __user *)(unsigned long)(arg->count_props_ptr);
 	uint32_t __user *props_ptr = (uint32_t __user *)(unsigned long)(arg->props_ptr);
 	uint64_t __user *prop_values_ptr = (uint64_t __user *)(unsigned long)(arg->prop_values_ptr);
+	uint32_t __user *out_fences_ptr = (uint32_t __user *)(unsigned long)(arg->out_fences_ptr);
 	unsigned int copied_objs, copied_props;
 	struct drm_atomic_state *state;
 	struct drm_modeset_acquire_ctx ctx;
@@ -1601,7 +1699,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 
 	/* can't test and expect an event at the same time. */
 	if ((arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) &&
-			(arg->flags & DRM_MODE_PAGE_FLIP_EVENT))
+			(arg->flags & (DRM_MODE_PAGE_FLIP_EVENT
+			 | DRM_MODE_ATOMIC_OUT_FENCE)))
 		return -EINVAL;
 
 	drm_modeset_acquire_init(&ctx, 0);
@@ -1693,6 +1792,14 @@ retry:
 		}
 	}
 
+	if (arg->flags & DRM_MODE_ATOMIC_OUT_FENCE) {
+		ret = drm_atomic_get_out_fences(dev, state, out_fences_ptr,
+						arg->count_out_fences,
+						arg->user_data);
+		if (ret < 0)
+			goto out;
+	}
+
 	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
 		/*
 		 * Unlike commit, check_only does not clean up state.
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 6ed8339..15ba3a8 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -31,6 +31,7 @@
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_atomic_helper.h>
 #include <linux/fence.h>
+#include <linux/sync_file.h>
 
 /**
  * DOC: overview
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 7934178..53e4e71 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -717,6 +717,7 @@ struct drm_crtc_funcs {
  * @properties: property tracking for this CRTC
  * @state: current atomic state for this CRTC
  * @timeline: sync timeline for fence sigalling
+ * @fence_seqno: seqno variable to create fences
  * @acquire_ctx: per-CRTC implicit acquire context used by atomic drivers for
  * 	legacy IOCTLs
  *
@@ -773,7 +774,9 @@ struct drm_crtc {
 
 	struct drm_crtc_state *state;
 
+	/* for out-fences */
 	struct sync_timeline *timeline;
+	unsigned long fence_seqno;
 
 	/*
 	 * For legacy crtc IOCTLs so that atomic drivers can get at the locking
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 39905cc..4cdcd22 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -592,6 +592,11 @@ struct drm_mode_destroy_dumb {
 		DRM_MODE_ATOMIC_ALLOW_MODESET |\
 		DRM_MODE_ATOMIC_OUT_FENCE)
 
+struct drm_out_fences {
+	__u32	crtc_id;
+	__u32	fd;
+};
+
 struct drm_mode_atomic {
 	__u32 flags;
 	__u32 count_objs;
@@ -601,6 +606,8 @@ struct drm_mode_atomic {
 	__u64 prop_values_ptr;
 	__u64 reserved;
 	__u64 user_data;
+	__u64 count_out_fences;
+	__u64 out_fences_ptr;
 };
 
 /**
-- 
2.5.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 6/8] drm/fence: create DRM_MODE_ATOMIC_OUT_FENCE flag

[Rob Clark]

On Thu, Apr 14, 2016 at 9:29 PM, Gustavo Padovan <gustavo@padovan.org> wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>
> This flag tells drm_atomic_ioctl that we want to get a per-crtc out-fence
> fd back.
>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  include/uapi/drm/drm_mode.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 7a7856e..39905cc 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -582,13 +582,15 @@ struct drm_mode_destroy_dumb {
>  #define DRM_MODE_ATOMIC_TEST_ONLY 0x0100
>  #define DRM_MODE_ATOMIC_NONBLOCK  0x0200
>  #define DRM_MODE_ATOMIC_ALLOW_MODESET 0x0400
> +#define DRM_MODE_ATOMIC_OUT_FENCE 0x0800
>
>  #define DRM_MODE_ATOMIC_FLAGS (\
>                 DRM_MODE_PAGE_FLIP_EVENT |\
>                 DRM_MODE_PAGE_FLIP_ASYNC |\
>                 DRM_MODE_ATOMIC_TEST_ONLY |\
>                 DRM_MODE_ATOMIC_NONBLOCK |\
> -               DRM_MODE_ATOMIC_ALLOW_MODESET)
> +               DRM_MODE_ATOMIC_ALLOW_MODESET |\
> +               DRM_MODE_ATOMIC_OUT_FENCE)

just to be pedantic / bisectable, perhaps this should be squashed in
to patch that actually starts using this flag?  Otherwise there is an
intermediate state in git where the flag is accepted but ignored..

BR,
-R


>  struct drm_mode_atomic {
>         __u32 flags;
> --
> 2.5.5
>

Re: [RFC 6/8] drm/fence: create DRM_MODE_ATOMIC_OUT_FENCE flag

[Rob Clark]

On Thu, Apr 14, 2016 at 9:29 PM, Gustavo Padovan <gustavo@padovan.org> wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>
> This flag tells drm_atomic_ioctl that we want to get a per-crtc out-fence
> fd back.
>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  include/uapi/drm/drm_mode.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 7a7856e..39905cc 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -582,13 +582,15 @@ struct drm_mode_destroy_dumb {
>  #define DRM_MODE_ATOMIC_TEST_ONLY 0x0100
>  #define DRM_MODE_ATOMIC_NONBLOCK  0x0200
>  #define DRM_MODE_ATOMIC_ALLOW_MODESET 0x0400
> +#define DRM_MODE_ATOMIC_OUT_FENCE 0x0800
>
>  #define DRM_MODE_ATOMIC_FLAGS (\
>                 DRM_MODE_PAGE_FLIP_EVENT |\
>                 DRM_MODE_PAGE_FLIP_ASYNC |\
>                 DRM_MODE_ATOMIC_TEST_ONLY |\
>                 DRM_MODE_ATOMIC_NONBLOCK |\
> -               DRM_MODE_ATOMIC_ALLOW_MODESET)
> +               DRM_MODE_ATOMIC_ALLOW_MODESET |\
> +               DRM_MODE_ATOMIC_OUT_FENCE)

just to be pedantic / bisectable, perhaps this should be squashed in
to patch that actually starts using this flag?  Otherwise there is an
intermediate state in git where the flag is accepted but ignored..

BR,
-R


>  struct drm_mode_atomic {
>         __u32 flags;
> --
> 2.5.5
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 3/8] drm/fence: allow fence waiting to be interrupted by userspace

[Daniel Vetter]

On Thu, Apr 14, 2016 at 06:29:36PM -0700, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> If userspace is running an synchronously atomic commit and interrupts the
> atomic operation during fence_wait() it will hang until the timer expires,
> so here we change the wait to be interruptible so it stop immediately when
> userspace wants to quit.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 7bf678e..f85ef8c 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1007,7 +1007,7 @@ void drm_atomic_helper_wait_for_fences(struct drm_device *dev,
>  
>  		WARN_ON(!plane->state->fb);
>  
> -		fence_wait(plane->state->fence, false);
> +		fence_wait(plane->state->fence, true);

You need to propagate the error code when allowing the wait to be
interrupted - we can't proceed with the atomic update in that case, but
need to bail out. And userspace needs to restart the ioctl.

Also, needs a testcase I think.
-Daniel

>  		fence_put(plane->state->fence);
>  		plane->state->fence = NULL;
>  	}
> -- 
> 2.5.5
> 

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

Re: [RFC 3/8] drm/fence: allow fence waiting to be interrupted by userspace

[Daniel Vetter]

On Thu, Apr 14, 2016 at 06:29:36PM -0700, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> If userspace is running an synchronously atomic commit and interrupts the
> atomic operation during fence_wait() it will hang until the timer expires,
> so here we change the wait to be interruptible so it stop immediately when
> userspace wants to quit.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 7bf678e..f85ef8c 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1007,7 +1007,7 @@ void drm_atomic_helper_wait_for_fences(struct drm_device *dev,
>  
>  		WARN_ON(!plane->state->fb);
>  
> -		fence_wait(plane->state->fence, false);
> +		fence_wait(plane->state->fence, true);

You need to propagate the error code when allowing the wait to be
interrupted - we can't proceed with the atomic update in that case, but
need to bail out. And userspace needs to restart the ioctl.

Also, needs a testcase I think.
-Daniel

>  		fence_put(plane->state->fence);
>  		plane->state->fence = NULL;
>  	}
> -- 
> 2.5.5
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 2/8] dma-buf/sync_file: add sync_file_fences_get()

[Daniel Vetter]

On Thu, Apr 14, 2016 at 06:29:35PM -0700, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Creates a function that given an sync file descriptor returns a
> fence_collection containing all fences in the sync_file.
> 
> If there is only one fence in the sync_file this fence itself is returned,
> however if there is more than one, a fence_collection fence is returned.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/dma-buf/sync_file.c | 37 +++++++++++++++++++++++++++++++++++++
>  include/linux/sync_file.h   | 10 ++++++++++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> index 4d2af24..926fafa 100644
> --- a/drivers/dma-buf/sync_file.c
> +++ b/drivers/dma-buf/sync_file.c
> @@ -23,6 +23,7 @@
>  #include <linux/slab.h>
>  #include <linux/uaccess.h>
>  #include <linux/anon_inodes.h>
> +#include <linux/fence-collection.h>
>  #include <linux/sync_file.h>
>  #include <uapi/linux/sync_file.h>
>  
> @@ -122,6 +123,39 @@ void sync_file_install(struct sync_file *sync_file, int fd)
>  }
>  EXPORT_SYMBOL(sync_file_install);
>  
> +struct fence *sync_file_fences_get(int fd)
> +{
> +	struct sync_file *sync_file;
> +	struct fence_collection *collection;
> +	int i;
> +
> +	sync_file = sync_file_fdget(fd);
> +	if (!sync_file)
> +		return NULL;
> +
> +	if (sync_file->num_fences == 1) {
> +		struct fence *fence = sync_file->cbs[0].fence;
> +
> +		sync_file_put(sync_file);
> +		return fence;
> +	}
> +
> +	collection = fence_collection_init(sync_file->num_fences);
> +	if (!collection) {
> +		sync_file_put(sync_file);
> +		return NULL;
> +	}
> +
> +	for (i = 0 ; i < sync_file->num_fences ; i++)
> +		fence_collection_add(collection, sync_file->cbs[i].fence);
> +
> +	sync_file->collection = collection;
> +	sync_file_put(sync_file);
> +
> +	return &collection->base;
> +}

This function should probably acquire a reference for the returned fence
for the caller.
-Daniel

> +EXPORT_SYMBOL(sync_file_fences_get);
> +
>  static void sync_file_add_pt(struct sync_file *sync_file, int *i,
>  			     struct fence *fence)
>  {
> @@ -200,6 +234,9 @@ static void sync_file_free(struct kref *kref)
>  						     kref);
>  	int i;
>  
> +	if (sync_file->collection)
> +		fence_collection_put(sync_file->collection);
> +
>  	for (i = 0; i < sync_file->num_fences; ++i) {
>  		fence_remove_callback(sync_file->cbs[i].fence,
>  				      &sync_file->cbs[i].cb);
> diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
> index 7b7a89d..2cb0486 100644
> --- a/include/linux/sync_file.h
> +++ b/include/linux/sync_file.h
> @@ -49,6 +49,7 @@ struct sync_file {
>  	wait_queue_head_t	wq;
>  	atomic_t		status;
>  
> +	struct fence_collection *collection;
>  	struct sync_file_cb	cbs[];
>  };
>  
> @@ -103,4 +104,13 @@ void sync_file_put(struct sync_file *sync_file);
>   */
>  void sync_file_install(struct sync_file *sync_file, int fd);
>  
> +/**
> + * sync_file_fences_get - get the fence related to the fd
> + * @fd:		file descriptor to look for a fence collection
> + *
> + * Ensures @fd references a valid sync_file and returns the base object
> + * of the fence_collection that contains all fences in the sync_file
> + * or NULL in case of error.
> + */
> +struct fence *sync_file_fences_get(int fd);
>  #endif /* _LINUX_SYNC_H */
> -- 
> 2.5.5
> 

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

Re: [RFC 2/8] dma-buf/sync_file: add sync_file_fences_get()

[Daniel Vetter]

On Thu, Apr 14, 2016 at 06:29:35PM -0700, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Creates a function that given an sync file descriptor returns a
> fence_collection containing all fences in the sync_file.
> 
> If there is only one fence in the sync_file this fence itself is returned,
> however if there is more than one, a fence_collection fence is returned.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/dma-buf/sync_file.c | 37 +++++++++++++++++++++++++++++++++++++
>  include/linux/sync_file.h   | 10 ++++++++++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> index 4d2af24..926fafa 100644
> --- a/drivers/dma-buf/sync_file.c
> +++ b/drivers/dma-buf/sync_file.c
> @@ -23,6 +23,7 @@
>  #include <linux/slab.h>
>  #include <linux/uaccess.h>
>  #include <linux/anon_inodes.h>
> +#include <linux/fence-collection.h>
>  #include <linux/sync_file.h>
>  #include <uapi/linux/sync_file.h>
>  
> @@ -122,6 +123,39 @@ void sync_file_install(struct sync_file *sync_file, int fd)
>  }
>  EXPORT_SYMBOL(sync_file_install);
>  
> +struct fence *sync_file_fences_get(int fd)
> +{
> +	struct sync_file *sync_file;
> +	struct fence_collection *collection;
> +	int i;
> +
> +	sync_file = sync_file_fdget(fd);
> +	if (!sync_file)
> +		return NULL;
> +
> +	if (sync_file->num_fences == 1) {
> +		struct fence *fence = sync_file->cbs[0].fence;
> +
> +		sync_file_put(sync_file);
> +		return fence;
> +	}
> +
> +	collection = fence_collection_init(sync_file->num_fences);
> +	if (!collection) {
> +		sync_file_put(sync_file);
> +		return NULL;
> +	}
> +
> +	for (i = 0 ; i < sync_file->num_fences ; i++)
> +		fence_collection_add(collection, sync_file->cbs[i].fence);
> +
> +	sync_file->collection = collection;
> +	sync_file_put(sync_file);
> +
> +	return &collection->base;
> +}

This function should probably acquire a reference for the returned fence
for the caller.
-Daniel

> +EXPORT_SYMBOL(sync_file_fences_get);
> +
>  static void sync_file_add_pt(struct sync_file *sync_file, int *i,
>  			     struct fence *fence)
>  {
> @@ -200,6 +234,9 @@ static void sync_file_free(struct kref *kref)
>  						     kref);
>  	int i;
>  
> +	if (sync_file->collection)
> +		fence_collection_put(sync_file->collection);
> +
>  	for (i = 0; i < sync_file->num_fences; ++i) {
>  		fence_remove_callback(sync_file->cbs[i].fence,
>  				      &sync_file->cbs[i].cb);
> diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
> index 7b7a89d..2cb0486 100644
> --- a/include/linux/sync_file.h
> +++ b/include/linux/sync_file.h
> @@ -49,6 +49,7 @@ struct sync_file {
>  	wait_queue_head_t	wq;
>  	atomic_t		status;
>  
> +	struct fence_collection *collection;
>  	struct sync_file_cb	cbs[];
>  };
>  
> @@ -103,4 +104,13 @@ void sync_file_put(struct sync_file *sync_file);
>   */
>  void sync_file_install(struct sync_file *sync_file, int fd);
>  
> +/**
> + * sync_file_fences_get - get the fence related to the fd
> + * @fd:		file descriptor to look for a fence collection
> + *
> + * Ensures @fd references a valid sync_file and returns the base object
> + * of the fence_collection that contains all fences in the sync_file
> + * or NULL in case of error.
> + */
> +struct fence *sync_file_fences_get(int fd);
>  #endif /* _LINUX_SYNC_H */
> -- 
> 2.5.5
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 1/8] dma-buf/fence: add fence_collection fences

[Daniel Vetter]

On Thu, Apr 14, 2016 at 06:29:34PM -0700, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> struct fence_collection inherits from struct fence and carries a
> collection of fences that needs to be waited together.
> 
> It is useful to translate a sync_file to a fence to remove the complexity
> of dealing with sync_files from DRM drivers. So even if there are many
> fences in the sync_file that needs to waited for a commit to happen
> drivers would only worry about a standard struct fence.That means that no
> changes needed to any driver besides supporting fences.
> 
> fence_collection's fence doesn't belong to any timeline context.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/dma-buf/Makefile           |   2 +-
>  drivers/dma-buf/fence-collection.c | 138 +++++++++++++++++++++++++++++++++++++
>  drivers/dma-buf/fence.c            |   2 +-
>  include/linux/fence-collection.h   |  56 +++++++++++++++
>  include/linux/fence.h              |   2 +
>  5 files changed, 198 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/dma-buf/fence-collection.c
>  create mode 100644 include/linux/fence-collection.h
> 
> diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile
> index 43325a1..30b8464 100644
> --- a/drivers/dma-buf/Makefile
> +++ b/drivers/dma-buf/Makefile
> @@ -1,3 +1,3 @@
> -obj-y := dma-buf.o fence.o reservation.o seqno-fence.o
> +obj-y := dma-buf.o fence.o reservation.o seqno-fence.o fence-collection.o
>  obj-$(CONFIG_SYNC_FILE)		+= sync_file.o sync_timeline.o sync_debug.o
>  obj-$(CONFIG_SW_SYNC)		+= sw_sync.o
> diff --git a/drivers/dma-buf/fence-collection.c b/drivers/dma-buf/fence-collection.c
> new file mode 100644
> index 0000000..8a4ecb0
> --- /dev/null
> +++ b/drivers/dma-buf/fence-collection.c
> @@ -0,0 +1,138 @@
> +/*
> + * fence-collection: aggregate fences to be waited together
> + *
> + * Copyright (C) 2016 Collabora Ltd
> + * Authors:
> + *	Gustavo Padovan <gustavo@padovan.org>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */
> +
> +#include <linux/export.h>
> +#include <linux/slab.h>
> +#include <linux/fence-collection.h>
> +
> +static const char *fence_collection_get_driver_name(struct fence *fence)
> +{
> +	struct fence_collection *collection = to_fence_collection(fence);
> +	struct fence *f = collection->fences[0].fence;
> +
> +	return f->ops->get_driver_name(fence);
> +}
> +
> +static const char *fence_collection_get_timeline_name(struct fence *fence)
> +{
> +	return "no context";
> +}
> +
> +static bool fence_collection_enable_signaling(struct fence *fence)
> +{
> +	struct fence_collection *collection = to_fence_collection(fence);
> +
> +	return atomic_read(&collection->num_pending_fences);
> +}
> +
> +static bool fence_collection_signaled(struct fence *fence)
> +{
> +	struct fence_collection *collection = to_fence_collection(fence);
> +
> +	return (atomic_read(&collection->num_pending_fences) == 0);
> +}
> +
> +static void fence_collection_release(struct fence *fence)
> +{
> +	struct fence_collection *collection = to_fence_collection(fence);
> +	int i;
> +
> +	for (i = 0 ; i < collection->num_fences ; i++) {
> +		fence_remove_callback(collection->fences[i].fence,
> +				      &collection->fences[i].cb);
> +		fence_put(collection->fences[i].fence);
> +	}
> +
> +	fence_free(fence);
> +}
> +
> +static signed long fence_collection_wait(struct fence *fence, bool intr,
> +					 signed long timeout)
> +{
> +	struct fence_collection *collection = to_fence_collection(fence);
> +	int i;
> +
> +	for (i = 0 ; i < collection->num_fences ; i++) {
> +		timeout = fence_wait(collection->fences[i].fence, intr);
> +		if (timeout < 0)
> +			return timeout;
> +	}
> +
> +	return timeout;
> +}
> +
> +static const struct fence_ops fence_collection_ops = {
> +	.get_driver_name = fence_collection_get_driver_name,
> +	.get_timeline_name = fence_collection_get_timeline_name,
> +	.enable_signaling = fence_collection_enable_signaling,
> +	.signaled = fence_collection_signaled,
> +	.wait = fence_collection_wait,
> +	.release = fence_collection_release,
> +};
> +
> +static void collection_check_cb_func(struct fence *fence, struct fence_cb *cb)
> +{
> +	struct fence_collection_cb *f_cb;
> +	struct fence_collection *collection;
> +
> +	f_cb = container_of(cb, struct fence_collection_cb, cb);
> +	collection = f_cb->collection;
> +
> +	if (atomic_dec_and_test(&collection->num_pending_fences))
> +		fence_signal(&collection->base);
> +}
> +
> +void fence_collection_add(struct fence_collection *collection,
> +			  struct fence *fence)
> +{
> +	int n = collection->num_fences;
> +
> +	collection->fences[n].collection = collection;
> +	collection->fences[n].fence = fence;
> +
> +	if (fence_add_callback(fence, &collection->fences[n].cb,
> +				 collection_check_cb_func))
> +		return;
> +
> +	fence_get(fence);
> +
> +	collection->num_fences++;
> +	atomic_inc(&collection->num_pending_fences);
> +}

For the interface I think we should not split it into _init and _add - it
shouldn't be allowed to change a collection once it's created. So probably
cleaner if we add an array of fence pointers to _init.

Other nitpick: Adding the callback should (I think) only be done in
->enable_signalling.

Finally: Have you looked into stitching together a few unit tests for
fence_collection?

Fence collections also break the assumption that every fence is on a
timeline. fence_later and fence_is_later need to be adjusted. We also need
a special collection context to filter these out. This means
fence_collection isn't perfectly opaque abstraction.
> +
> +struct fence_collection *fence_collection_init(int num_fences)
> +{
> +	struct fence_collection *collection;
> +
> +	collection = kzalloc(offsetof(struct fence_collection,
> +				      fences[num_fences]), GFP_KERNEL);
> +	if (!collection)
> +		return NULL;
> +
> +	spin_lock_init(&collection->lock);
> +	fence_init(&collection->base, &fence_collection_ops, &collection->lock,
> +		   FENCE_NO_CONTEXT, 0);
> +
> +	return collection;
> +}
> +EXPORT_SYMBOL(fence_collection_init);
> +
> +void fence_collection_put(struct fence_collection *collection)
> +{
> +	fence_put(&collection->base);

Not sure a specialized _put function is useful, I'd leave it out.

> +}
> +EXPORT_SYMBOL(fence_collection_put);
> diff --git a/drivers/dma-buf/fence.c b/drivers/dma-buf/fence.c
> index 7b05dbe..486e95c 100644
> --- a/drivers/dma-buf/fence.c
> +++ b/drivers/dma-buf/fence.c
> @@ -35,7 +35,7 @@ EXPORT_TRACEPOINT_SYMBOL(fence_emit);
>   * context or not. One device can have multiple separate contexts,
>   * and they're used if some engine can run independently of another.
>   */
> -static atomic_t fence_context_counter = ATOMIC_INIT(0);
> +static atomic_t fence_context_counter = ATOMIC_INIT(1);
>  
>  /**
>   * fence_context_alloc - allocate an array of fence contexts
> diff --git a/include/linux/fence-collection.h b/include/linux/fence-collection.h
> new file mode 100644
> index 0000000..a798925
> --- /dev/null
> +++ b/include/linux/fence-collection.h
> @@ -0,0 +1,56 @@
> +/*
> + * fence-collection: aggregates fence to be waited together
> + *
> + * Copyright (C) 2016 Collabora Ltd
> + * Authors:
> + *	Gustavo Padovan <gustavo@padovan.org>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */
> +
> +#ifndef __LINUX_FENCE_COLLECTION_H
> +#define __LINUX_FENCE_COLLECTION_H
> +
> +#include <linux/fence.h>
> +
> +struct fence_collection_cb {
> +	struct fence_cb cb;
> +	struct fence *fence;
> +	struct fence_collection *collection;
> +};
> +
> +struct fence_collection {
> +	struct fence base;
> +
> +	spinlock_t lock;
> +	struct fence_cb fence_cb;
> +	atomic_t num_pending_fences;
> +	int num_fences;
> +	struct fence_collection_cb fences[];
> +};
> +
> +/**
> + * to_fence_collection - cast a fence to a fence_collection
> + * @fence: fence to cast to a fence_collection
> + *
> + * Returns NULL if the fence is not a fence_collection,
> + * or the fence_collection otherwise.
> + */
> +static inline struct fence_collection * to_fence_collection(struct fence *fence)
> +{

Kerneldoc claims it, but you don't check that the fence is indeed a
fence_collection. That's usually done by comparing the ops pointer.

> +	return container_of(fence, struct fence_collection, base);
> +}
> +
> +struct fence_collection *fence_collection_init(int num_fences);
> +void fence_collection_add(struct fence_collection *collection,
> +			  struct fence *fence);
> +void fence_collection_put(struct fence_collection *collection);
> +
> +#endif /* __LINUX_FENCE_COLLECTION_H */
> diff --git a/include/linux/fence.h b/include/linux/fence.h
> index 2b17698..02170dd 100644
> --- a/include/linux/fence.h
> +++ b/include/linux/fence.h
> @@ -30,6 +30,8 @@
>  #include <linux/printk.h>
>  #include <linux/rcupdate.h>
>  
> +#define FENCE_NO_CONTEXT 0
> +
>  struct fence;
>  struct fence_ops;
>  struct fence_cb;
> -- 
> 2.5.5
> 

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

Re: [RFC 1/8] dma-buf/fence: add fence_collection fences

[Daniel Vetter]

On Thu, Apr 14, 2016 at 06:29:34PM -0700, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> struct fence_collection inherits from struct fence and carries a
> collection of fences that needs to be waited together.
> 
> It is useful to translate a sync_file to a fence to remove the complexity
> of dealing with sync_files from DRM drivers. So even if there are many
> fences in the sync_file that needs to waited for a commit to happen
> drivers would only worry about a standard struct fence.That means that no
> changes needed to any driver besides supporting fences.
> 
> fence_collection's fence doesn't belong to any timeline context.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/dma-buf/Makefile           |   2 +-
>  drivers/dma-buf/fence-collection.c | 138 +++++++++++++++++++++++++++++++++++++
>  drivers/dma-buf/fence.c            |   2 +-
>  include/linux/fence-collection.h   |  56 +++++++++++++++
>  include/linux/fence.h              |   2 +
>  5 files changed, 198 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/dma-buf/fence-collection.c
>  create mode 100644 include/linux/fence-collection.h
> 
> diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile
> index 43325a1..30b8464 100644
> --- a/drivers/dma-buf/Makefile
> +++ b/drivers/dma-buf/Makefile
> @@ -1,3 +1,3 @@
> -obj-y := dma-buf.o fence.o reservation.o seqno-fence.o
> +obj-y := dma-buf.o fence.o reservation.o seqno-fence.o fence-collection.o
>  obj-$(CONFIG_SYNC_FILE)		+= sync_file.o sync_timeline.o sync_debug.o
>  obj-$(CONFIG_SW_SYNC)		+= sw_sync.o
> diff --git a/drivers/dma-buf/fence-collection.c b/drivers/dma-buf/fence-collection.c
> new file mode 100644
> index 0000000..8a4ecb0
> --- /dev/null
> +++ b/drivers/dma-buf/fence-collection.c
> @@ -0,0 +1,138 @@
> +/*
> + * fence-collection: aggregate fences to be waited together
> + *
> + * Copyright (C) 2016 Collabora Ltd
> + * Authors:
> + *	Gustavo Padovan <gustavo@padovan.org>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */
> +
> +#include <linux/export.h>
> +#include <linux/slab.h>
> +#include <linux/fence-collection.h>
> +
> +static const char *fence_collection_get_driver_name(struct fence *fence)
> +{
> +	struct fence_collection *collection = to_fence_collection(fence);
> +	struct fence *f = collection->fences[0].fence;
> +
> +	return f->ops->get_driver_name(fence);
> +}
> +
> +static const char *fence_collection_get_timeline_name(struct fence *fence)
> +{
> +	return "no context";
> +}
> +
> +static bool fence_collection_enable_signaling(struct fence *fence)
> +{
> +	struct fence_collection *collection = to_fence_collection(fence);
> +
> +	return atomic_read(&collection->num_pending_fences);
> +}
> +
> +static bool fence_collection_signaled(struct fence *fence)
> +{
> +	struct fence_collection *collection = to_fence_collection(fence);
> +
> +	return (atomic_read(&collection->num_pending_fences) == 0);
> +}
> +
> +static void fence_collection_release(struct fence *fence)
> +{
> +	struct fence_collection *collection = to_fence_collection(fence);
> +	int i;
> +
> +	for (i = 0 ; i < collection->num_fences ; i++) {
> +		fence_remove_callback(collection->fences[i].fence,
> +				      &collection->fences[i].cb);
> +		fence_put(collection->fences[i].fence);
> +	}
> +
> +	fence_free(fence);
> +}
> +
> +static signed long fence_collection_wait(struct fence *fence, bool intr,
> +					 signed long timeout)
> +{
> +	struct fence_collection *collection = to_fence_collection(fence);
> +	int i;
> +
> +	for (i = 0 ; i < collection->num_fences ; i++) {
> +		timeout = fence_wait(collection->fences[i].fence, intr);
> +		if (timeout < 0)
> +			return timeout;
> +	}
> +
> +	return timeout;
> +}
> +
> +static const struct fence_ops fence_collection_ops = {
> +	.get_driver_name = fence_collection_get_driver_name,
> +	.get_timeline_name = fence_collection_get_timeline_name,
> +	.enable_signaling = fence_collection_enable_signaling,
> +	.signaled = fence_collection_signaled,
> +	.wait = fence_collection_wait,
> +	.release = fence_collection_release,
> +};
> +
> +static void collection_check_cb_func(struct fence *fence, struct fence_cb *cb)
> +{
> +	struct fence_collection_cb *f_cb;
> +	struct fence_collection *collection;
> +
> +	f_cb = container_of(cb, struct fence_collection_cb, cb);
> +	collection = f_cb->collection;
> +
> +	if (atomic_dec_and_test(&collection->num_pending_fences))
> +		fence_signal(&collection->base);
> +}
> +
> +void fence_collection_add(struct fence_collection *collection,
> +			  struct fence *fence)
> +{
> +	int n = collection->num_fences;
> +
> +	collection->fences[n].collection = collection;
> +	collection->fences[n].fence = fence;
> +
> +	if (fence_add_callback(fence, &collection->fences[n].cb,
> +				 collection_check_cb_func))
> +		return;
> +
> +	fence_get(fence);
> +
> +	collection->num_fences++;
> +	atomic_inc(&collection->num_pending_fences);
> +}

For the interface I think we should not split it into _init and _add - it
shouldn't be allowed to change a collection once it's created. So probably
cleaner if we add an array of fence pointers to _init.

Other nitpick: Adding the callback should (I think) only be done in
->enable_signalling.

Finally: Have you looked into stitching together a few unit tests for
fence_collection?

Fence collections also break the assumption that every fence is on a
timeline. fence_later and fence_is_later need to be adjusted. We also need
a special collection context to filter these out. This means
fence_collection isn't perfectly opaque abstraction.
> +
> +struct fence_collection *fence_collection_init(int num_fences)
> +{
> +	struct fence_collection *collection;
> +
> +	collection = kzalloc(offsetof(struct fence_collection,
> +				      fences[num_fences]), GFP_KERNEL);
> +	if (!collection)
> +		return NULL;
> +
> +	spin_lock_init(&collection->lock);
> +	fence_init(&collection->base, &fence_collection_ops, &collection->lock,
> +		   FENCE_NO_CONTEXT, 0);
> +
> +	return collection;
> +}
> +EXPORT_SYMBOL(fence_collection_init);
> +
> +void fence_collection_put(struct fence_collection *collection)
> +{
> +	fence_put(&collection->base);

Not sure a specialized _put function is useful, I'd leave it out.

> +}
> +EXPORT_SYMBOL(fence_collection_put);
> diff --git a/drivers/dma-buf/fence.c b/drivers/dma-buf/fence.c
> index 7b05dbe..486e95c 100644
> --- a/drivers/dma-buf/fence.c
> +++ b/drivers/dma-buf/fence.c
> @@ -35,7 +35,7 @@ EXPORT_TRACEPOINT_SYMBOL(fence_emit);
>   * context or not. One device can have multiple separate contexts,
>   * and they're used if some engine can run independently of another.
>   */
> -static atomic_t fence_context_counter = ATOMIC_INIT(0);
> +static atomic_t fence_context_counter = ATOMIC_INIT(1);
>  
>  /**
>   * fence_context_alloc - allocate an array of fence contexts
> diff --git a/include/linux/fence-collection.h b/include/linux/fence-collection.h
> new file mode 100644
> index 0000000..a798925
> --- /dev/null
> +++ b/include/linux/fence-collection.h
> @@ -0,0 +1,56 @@
> +/*
> + * fence-collection: aggregates fence to be waited together
> + *
> + * Copyright (C) 2016 Collabora Ltd
> + * Authors:
> + *	Gustavo Padovan <gustavo@padovan.org>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */
> +
> +#ifndef __LINUX_FENCE_COLLECTION_H
> +#define __LINUX_FENCE_COLLECTION_H
> +
> +#include <linux/fence.h>
> +
> +struct fence_collection_cb {
> +	struct fence_cb cb;
> +	struct fence *fence;
> +	struct fence_collection *collection;
> +};
> +
> +struct fence_collection {
> +	struct fence base;
> +
> +	spinlock_t lock;
> +	struct fence_cb fence_cb;
> +	atomic_t num_pending_fences;
> +	int num_fences;
> +	struct fence_collection_cb fences[];
> +};
> +
> +/**
> + * to_fence_collection - cast a fence to a fence_collection
> + * @fence: fence to cast to a fence_collection
> + *
> + * Returns NULL if the fence is not a fence_collection,
> + * or the fence_collection otherwise.
> + */
> +static inline struct fence_collection * to_fence_collection(struct fence *fence)
> +{

Kerneldoc claims it, but you don't check that the fence is indeed a
fence_collection. That's usually done by comparing the ops pointer.

> +	return container_of(fence, struct fence_collection, base);
> +}
> +
> +struct fence_collection *fence_collection_init(int num_fences);
> +void fence_collection_add(struct fence_collection *collection,
> +			  struct fence *fence);
> +void fence_collection_put(struct fence_collection *collection);
> +
> +#endif /* __LINUX_FENCE_COLLECTION_H */
> diff --git a/include/linux/fence.h b/include/linux/fence.h
> index 2b17698..02170dd 100644
> --- a/include/linux/fence.h
> +++ b/include/linux/fence.h
> @@ -30,6 +30,8 @@
>  #include <linux/printk.h>
>  #include <linux/rcupdate.h>
>  
> +#define FENCE_NO_CONTEXT 0
> +
>  struct fence;
>  struct fence_ops;
>  struct fence_cb;
> -- 
> 2.5.5
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 4/8] drm/fence: add in-fences support

[Daniel Vetter]

On Thu, Apr 14, 2016 at 06:29:37PM -0700, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> There is now a new property called FENCE_FD attached to every plane
> state that receives the sync_file fd from userspace via the atomic commit
> IOCTL.
> 
> The fd is then translated to a fence (that may be a fence_collection
> subclass or just a normal fence) and then used by DRM to fence_wait() for
> all fences in the sync_file to signal. So it only commits when all
> framebuffers are ready to scanout.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/Kconfig             | 1 +
>  drivers/gpu/drm/drm_atomic.c        | 8 ++++++++
>  drivers/gpu/drm/drm_atomic_helper.c | 5 +++++
>  drivers/gpu/drm/drm_crtc.c          | 7 +++++++
>  include/drm/drm_crtc.h              | 1 +
>  5 files changed, 22 insertions(+)
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index f2a74d0..3c987e3 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -12,6 +12,7 @@ menuconfig DRM
>  	select I2C
>  	select I2C_ALGOBIT
>  	select DMA_SHARED_BUFFER
> +	select SYNC_FILE
>  	help
>  	  Kernel-level support for the Direct Rendering Infrastructure (DRI)
>  	  introduced in XFree86 4.0. If you say Y here, you need to select
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 8ee1db8..6702502 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -30,6 +30,7 @@
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_mode.h>
>  #include <drm/drm_plane_helper.h>
> +#include <linux/sync_file.h>
>  
>  /**
>   * drm_atomic_state_default_release -
> @@ -680,6 +681,11 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>  		drm_atomic_set_fb_for_plane(state, fb);
>  		if (fb)
>  			drm_framebuffer_unreference(fb);
> +	} else if (property == config->prop_fence_fd) {
> +		state->fence = sync_file_fences_get(val);
> +		if (!state->fence)
> +			return -EINVAL;
> +		fence_get(state->fence);

Yeah, this fence_get must be part of sync_file_fences_get, this code here
has a race (exercise for the reader to describe where things go wrong
here). Also, you need to explicitly filter out -1 here first I think.

Needs an atomic testcase to make sure setting FENCE_FD to -1 doesn't fall
over (since generic userspace might do this in an attempt to restore all
the state).

>  	} else if (property == config->prop_crtc_id) {
>  		struct drm_crtc *crtc = drm_crtc_find(dev, val);
>  		return drm_atomic_set_crtc_for_plane(state, crtc);
> @@ -737,6 +743,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>  
>  	if (property == config->prop_fb_id) {
>  		*val = (state->fb) ? state->fb->base.id : 0;
> +	} else if (property == config->prop_fence_fd) {
> +		*val = -1;
>  	} else if (property == config->prop_crtc_id) {
>  		*val = (state->crtc) ? state->crtc->base.id : 0;
>  	} else if (property == config->prop_crtc_x) {
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index f85ef8c..6ed8339 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2687,6 +2687,11 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane *plane,
>  {
>  	if (state->fb)
>  		drm_framebuffer_unreference(state->fb);
> +
> +	if (state->fence) {
> +		fence_put(state->fence);
> +		state->fence = NULL;

No need to set to NULL, we don't do that for ->fb either.

> +	}
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>  
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 55ffde5..65212ce 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1278,6 +1278,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  
>  	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
>  		drm_object_attach_property(&plane->base, config->prop_fb_id, 0);
> +		drm_object_attach_property(&plane->base, config->prop_fence_fd, -1);
>  		drm_object_attach_property(&plane->base, config->prop_crtc_id, 0);
>  		drm_object_attach_property(&plane->base, config->prop_crtc_x, 0);
>  		drm_object_attach_property(&plane->base, config->prop_crtc_y, 0);
> @@ -1533,6 +1534,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>  		return -ENOMEM;
>  	dev->mode_config.prop_fb_id = prop;
>  
> +	prop = drm_property_create_signed_range(dev, DRM_MODE_PROP_ATOMIC,
> +			"FENCE_FD", -1, INT_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_fence_fd = prop;
> +
>  	prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
>  			"CRTC_ID", DRM_MODE_OBJECT_CRTC);
>  	if (!prop)
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 8cb377c..5ba3cda 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -2122,6 +2122,7 @@ struct drm_mode_config {
>  	struct drm_property *prop_crtc_w;
>  	struct drm_property *prop_crtc_h;
>  	struct drm_property *prop_fb_id;
> +	struct drm_property *prop_fence_fd;
>  	struct drm_property *prop_crtc_id;
>  	struct drm_property *prop_active;
>  	struct drm_property *prop_mode_id;
> -- 
> 2.5.5
> 

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

Re: [RFC 4/8] drm/fence: add in-fences support

[Daniel Vetter]

On Thu, Apr 14, 2016 at 06:29:37PM -0700, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> There is now a new property called FENCE_FD attached to every plane
> state that receives the sync_file fd from userspace via the atomic commit
> IOCTL.
> 
> The fd is then translated to a fence (that may be a fence_collection
> subclass or just a normal fence) and then used by DRM to fence_wait() for
> all fences in the sync_file to signal. So it only commits when all
> framebuffers are ready to scanout.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/Kconfig             | 1 +
>  drivers/gpu/drm/drm_atomic.c        | 8 ++++++++
>  drivers/gpu/drm/drm_atomic_helper.c | 5 +++++
>  drivers/gpu/drm/drm_crtc.c          | 7 +++++++
>  include/drm/drm_crtc.h              | 1 +
>  5 files changed, 22 insertions(+)
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index f2a74d0..3c987e3 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -12,6 +12,7 @@ menuconfig DRM
>  	select I2C
>  	select I2C_ALGOBIT
>  	select DMA_SHARED_BUFFER
> +	select SYNC_FILE
>  	help
>  	  Kernel-level support for the Direct Rendering Infrastructure (DRI)
>  	  introduced in XFree86 4.0. If you say Y here, you need to select
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 8ee1db8..6702502 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -30,6 +30,7 @@
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_mode.h>
>  #include <drm/drm_plane_helper.h>
> +#include <linux/sync_file.h>
>  
>  /**
>   * drm_atomic_state_default_release -
> @@ -680,6 +681,11 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>  		drm_atomic_set_fb_for_plane(state, fb);
>  		if (fb)
>  			drm_framebuffer_unreference(fb);
> +	} else if (property == config->prop_fence_fd) {
> +		state->fence = sync_file_fences_get(val);
> +		if (!state->fence)
> +			return -EINVAL;
> +		fence_get(state->fence);

Yeah, this fence_get must be part of sync_file_fences_get, this code here
has a race (exercise for the reader to describe where things go wrong
here). Also, you need to explicitly filter out -1 here first I think.

Needs an atomic testcase to make sure setting FENCE_FD to -1 doesn't fall
over (since generic userspace might do this in an attempt to restore all
the state).

>  	} else if (property == config->prop_crtc_id) {
>  		struct drm_crtc *crtc = drm_crtc_find(dev, val);
>  		return drm_atomic_set_crtc_for_plane(state, crtc);
> @@ -737,6 +743,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>  
>  	if (property == config->prop_fb_id) {
>  		*val = (state->fb) ? state->fb->base.id : 0;
> +	} else if (property == config->prop_fence_fd) {
> +		*val = -1;
>  	} else if (property == config->prop_crtc_id) {
>  		*val = (state->crtc) ? state->crtc->base.id : 0;
>  	} else if (property == config->prop_crtc_x) {
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index f85ef8c..6ed8339 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2687,6 +2687,11 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane *plane,
>  {
>  	if (state->fb)
>  		drm_framebuffer_unreference(state->fb);
> +
> +	if (state->fence) {
> +		fence_put(state->fence);
> +		state->fence = NULL;

No need to set to NULL, we don't do that for ->fb either.

> +	}
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>  
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 55ffde5..65212ce 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1278,6 +1278,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  
>  	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
>  		drm_object_attach_property(&plane->base, config->prop_fb_id, 0);
> +		drm_object_attach_property(&plane->base, config->prop_fence_fd, -1);
>  		drm_object_attach_property(&plane->base, config->prop_crtc_id, 0);
>  		drm_object_attach_property(&plane->base, config->prop_crtc_x, 0);
>  		drm_object_attach_property(&plane->base, config->prop_crtc_y, 0);
> @@ -1533,6 +1534,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>  		return -ENOMEM;
>  	dev->mode_config.prop_fb_id = prop;
>  
> +	prop = drm_property_create_signed_range(dev, DRM_MODE_PROP_ATOMIC,
> +			"FENCE_FD", -1, INT_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_fence_fd = prop;
> +
>  	prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
>  			"CRTC_ID", DRM_MODE_OBJECT_CRTC);
>  	if (!prop)
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 8cb377c..5ba3cda 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -2122,6 +2122,7 @@ struct drm_mode_config {
>  	struct drm_property *prop_crtc_w;
>  	struct drm_property *prop_crtc_h;
>  	struct drm_property *prop_fb_id;
> +	struct drm_property *prop_fence_fd;
>  	struct drm_property *prop_crtc_id;
>  	struct drm_property *prop_active;
>  	struct drm_property *prop_mode_id;
> -- 
> 2.5.5
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 5/8] drm/fence: add fence to drm_pending_event

[Daniel Vetter]

On Thu, Apr 14, 2016 at 06:29:38PM -0700, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Now a drm_pending_event can either send a real drm_event or signal a
> fence, or both. It allow us to signal via fences when the buffer is
> displayed on the screen. Which in turn means that the previous buffer
> is not in use anymore and can be freed or sent back to another driver
> for processing.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Do you have atomic igt testcase that exercise all the combinations of
drm_event and in/out-fences and make sure it all keeps working? Tomeu
already converted that over to be a generic testcase.

> ---
>  drivers/gpu/drm/drm_atomic.c | 19 +++++++++++++------
>  drivers/gpu/drm/drm_fops.c   |  5 +++--
>  drivers/gpu/drm/drm_irq.c    |  7 +++++++
>  include/drm/drmP.h           |  1 +
>  4 files changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 6702502..0b95526 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1431,7 +1431,8 @@ EXPORT_SYMBOL(drm_atomic_async_commit);
>   */
>  
>  static struct drm_pending_vblank_event *create_vblank_event(
> -		struct drm_device *dev, struct drm_file *file_priv, uint64_t user_data)
> +		struct drm_device *dev, struct drm_file *file_priv,
> +		struct fence *fence, uint64_t user_data)
>  {
>  	struct drm_pending_vblank_event *e = NULL;
>  	int ret;
> @@ -1444,12 +1445,17 @@ static struct drm_pending_vblank_event *create_vblank_event(
>  	e->event.base.length = sizeof(e->event);
>  	e->event.user_data = user_data;
>  
> -	ret = drm_event_reserve_init(dev, file_priv, &e->base, &e->event.base);
> -	if (ret) {
> -		kfree(e);
> -		return NULL;
> +	if (file_priv) {
> +		ret = drm_event_reserve_init(dev, file_priv, &e->base,
> +					     &e->event.base);
> +		if (ret) {
> +			kfree(e);
> +			return NULL;
> +		}
>  	}
>  
> +	e->base.fence = fence;
> +
>  	return e;
>  }
>  
> @@ -1676,7 +1682,8 @@ retry:
>  		for_each_crtc_in_state(state, crtc, crtc_state, i) {
>  			struct drm_pending_vblank_event *e;
>  
> -			e = create_vblank_event(dev, file_priv, arg->user_data);
> +			e = create_vblank_event(dev, file_priv, NULL,
> +						arg->user_data);
>  			if (!e) {
>  				ret = -ENOMEM;
>  				goto out;
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index aeef58e..38def49 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -801,8 +801,9 @@ void drm_send_event_locked(struct drm_device *dev, struct drm_pending_event *e)
>  {
>  	assert_spin_locked(&dev->event_lock);
>  
> -	if (!e->file_priv) {
> -		e->destroy(e);
> +	if (!e->file_priv || !e->event) {

This would be a bug: e->file_priv != NULL iff e->event != NULL. How did
this happen?

> +		if (e->destroy)
> +			e->destroy(e);
>  		return;
>  	}
>  
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 3c1a6f1..0c5d7cb 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -41,6 +41,7 @@
>  
>  #include <linux/vgaarb.h>
>  #include <linux/export.h>
> +#include <linux/fence.h>
>  
>  /* Access macro for slots in vblank timestamp ringbuffer. */
>  #define vblanktimestamp(dev, pipe, count) \
> @@ -1124,6 +1125,12 @@ void drm_send_vblank_event(struct drm_device *dev, unsigned int pipe,
>  		now = get_drm_timestamp();
>  	}
>  	e->pipe = pipe;
> +
> +	if (e->base.fence) {
> +		fence_signal(e->base.fence);
> +		fence_put(e->base.fence);
> +	}

I'd put this into drm_send_event_locked even. In case we send out fences
for other events too (e.g. exynos uses drm_event for rendering ...).
-Daniel

> +
>  	send_vblank_event(dev, e, seq, &now);
>  }
>  EXPORT_SYMBOL(drm_send_vblank_event);
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 3c8422c..8f83c2a 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -282,6 +282,7 @@ struct drm_ioctl_desc {
>  /* Event queued up for userspace to read */
>  struct drm_pending_event {
>  	struct drm_event *event;
> +	struct fence *fence;
>  	struct list_head link;
>  	struct list_head pending_link;
>  	struct drm_file *file_priv;
> -- 
> 2.5.5
> 

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

Re: [RFC 5/8] drm/fence: add fence to drm_pending_event

[Daniel Vetter]

On Thu, Apr 14, 2016 at 06:29:38PM -0700, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Now a drm_pending_event can either send a real drm_event or signal a
> fence, or both. It allow us to signal via fences when the buffer is
> displayed on the screen. Which in turn means that the previous buffer
> is not in use anymore and can be freed or sent back to another driver
> for processing.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Do you have atomic igt testcase that exercise all the combinations of
drm_event and in/out-fences and make sure it all keeps working? Tomeu
already converted that over to be a generic testcase.

> ---
>  drivers/gpu/drm/drm_atomic.c | 19 +++++++++++++------
>  drivers/gpu/drm/drm_fops.c   |  5 +++--
>  drivers/gpu/drm/drm_irq.c    |  7 +++++++
>  include/drm/drmP.h           |  1 +
>  4 files changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 6702502..0b95526 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1431,7 +1431,8 @@ EXPORT_SYMBOL(drm_atomic_async_commit);
>   */
>  
>  static struct drm_pending_vblank_event *create_vblank_event(
> -		struct drm_device *dev, struct drm_file *file_priv, uint64_t user_data)
> +		struct drm_device *dev, struct drm_file *file_priv,
> +		struct fence *fence, uint64_t user_data)
>  {
>  	struct drm_pending_vblank_event *e = NULL;
>  	int ret;
> @@ -1444,12 +1445,17 @@ static struct drm_pending_vblank_event *create_vblank_event(
>  	e->event.base.length = sizeof(e->event);
>  	e->event.user_data = user_data;
>  
> -	ret = drm_event_reserve_init(dev, file_priv, &e->base, &e->event.base);
> -	if (ret) {
> -		kfree(e);
> -		return NULL;
> +	if (file_priv) {
> +		ret = drm_event_reserve_init(dev, file_priv, &e->base,
> +					     &e->event.base);
> +		if (ret) {
> +			kfree(e);
> +			return NULL;
> +		}
>  	}
>  
> +	e->base.fence = fence;
> +
>  	return e;
>  }
>  
> @@ -1676,7 +1682,8 @@ retry:
>  		for_each_crtc_in_state(state, crtc, crtc_state, i) {
>  			struct drm_pending_vblank_event *e;
>  
> -			e = create_vblank_event(dev, file_priv, arg->user_data);
> +			e = create_vblank_event(dev, file_priv, NULL,
> +						arg->user_data);
>  			if (!e) {
>  				ret = -ENOMEM;
>  				goto out;
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index aeef58e..38def49 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -801,8 +801,9 @@ void drm_send_event_locked(struct drm_device *dev, struct drm_pending_event *e)
>  {
>  	assert_spin_locked(&dev->event_lock);
>  
> -	if (!e->file_priv) {
> -		e->destroy(e);
> +	if (!e->file_priv || !e->event) {

This would be a bug: e->file_priv != NULL iff e->event != NULL. How did
this happen?

> +		if (e->destroy)
> +			e->destroy(e);
>  		return;
>  	}
>  
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 3c1a6f1..0c5d7cb 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -41,6 +41,7 @@
>  
>  #include <linux/vgaarb.h>
>  #include <linux/export.h>
> +#include <linux/fence.h>
>  
>  /* Access macro for slots in vblank timestamp ringbuffer. */
>  #define vblanktimestamp(dev, pipe, count) \
> @@ -1124,6 +1125,12 @@ void drm_send_vblank_event(struct drm_device *dev, unsigned int pipe,
>  		now = get_drm_timestamp();
>  	}
>  	e->pipe = pipe;
> +
> +	if (e->base.fence) {
> +		fence_signal(e->base.fence);
> +		fence_put(e->base.fence);
> +	}

I'd put this into drm_send_event_locked even. In case we send out fences
for other events too (e.g. exynos uses drm_event for rendering ...).
-Daniel

> +
>  	send_vblank_event(dev, e, seq, &now);
>  }
>  EXPORT_SYMBOL(drm_send_vblank_event);
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 3c8422c..8f83c2a 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -282,6 +282,7 @@ struct drm_ioctl_desc {
>  /* Event queued up for userspace to read */
>  struct drm_pending_event {
>  	struct drm_event *event;
> +	struct fence *fence;
>  	struct list_head link;
>  	struct list_head pending_link;
>  	struct drm_file *file_priv;
> -- 
> 2.5.5
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 4/8] drm/fence: add in-fences support

[Daniel Vetter]

On Thu, Apr 14, 2016 at 06:29:37PM -0700, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> There is now a new property called FENCE_FD attached to every plane
> state that receives the sync_file fd from userspace via the atomic commit
> IOCTL.
> 
> The fd is then translated to a fence (that may be a fence_collection
> subclass or just a normal fence) and then used by DRM to fence_wait() for
> all fences in the sync_file to signal. So it only commits when all
> framebuffers are ready to scanout.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/Kconfig             | 1 +
>  drivers/gpu/drm/drm_atomic.c        | 8 ++++++++
>  drivers/gpu/drm/drm_atomic_helper.c | 5 +++++
>  drivers/gpu/drm/drm_crtc.c          | 7 +++++++
>  include/drm/drm_crtc.h              | 1 +
>  5 files changed, 22 insertions(+)
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index f2a74d0..3c987e3 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -12,6 +12,7 @@ menuconfig DRM
>  	select I2C
>  	select I2C_ALGOBIT
>  	select DMA_SHARED_BUFFER
> +	select SYNC_FILE
>  	help
>  	  Kernel-level support for the Direct Rendering Infrastructure (DRI)
>  	  introduced in XFree86 4.0. If you say Y here, you need to select
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 8ee1db8..6702502 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -30,6 +30,7 @@
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_mode.h>
>  #include <drm/drm_plane_helper.h>
> +#include <linux/sync_file.h>
>  
>  /**
>   * drm_atomic_state_default_release -
> @@ -680,6 +681,11 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>  		drm_atomic_set_fb_for_plane(state, fb);
>  		if (fb)
>  			drm_framebuffer_unreference(fb);
> +	} else if (property == config->prop_fence_fd) {

Need to fence_put if state->fence is non-NULL already. Could happen if
userspace sets FENCE_FD more than once (another case for your testcase I'd
say).
-Daniel

> +		state->fence = sync_file_fences_get(val);
> +		if (!state->fence)
> +			return -EINVAL;
> +		fence_get(state->fence);
>  	} else if (property == config->prop_crtc_id) {
>  		struct drm_crtc *crtc = drm_crtc_find(dev, val);
>  		return drm_atomic_set_crtc_for_plane(state, crtc);
> @@ -737,6 +743,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>  
>  	if (property == config->prop_fb_id) {
>  		*val = (state->fb) ? state->fb->base.id : 0;
> +	} else if (property == config->prop_fence_fd) {
> +		*val = -1;
>  	} else if (property == config->prop_crtc_id) {
>  		*val = (state->crtc) ? state->crtc->base.id : 0;
>  	} else if (property == config->prop_crtc_x) {
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index f85ef8c..6ed8339 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2687,6 +2687,11 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane *plane,
>  {
>  	if (state->fb)
>  		drm_framebuffer_unreference(state->fb);
> +
> +	if (state->fence) {
> +		fence_put(state->fence);
> +		state->fence = NULL;
> +	}
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>  
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 55ffde5..65212ce 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1278,6 +1278,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  
>  	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
>  		drm_object_attach_property(&plane->base, config->prop_fb_id, 0);
> +		drm_object_attach_property(&plane->base, config->prop_fence_fd, -1);
>  		drm_object_attach_property(&plane->base, config->prop_crtc_id, 0);
>  		drm_object_attach_property(&plane->base, config->prop_crtc_x, 0);
>  		drm_object_attach_property(&plane->base, config->prop_crtc_y, 0);
> @@ -1533,6 +1534,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>  		return -ENOMEM;
>  	dev->mode_config.prop_fb_id = prop;
>  
> +	prop = drm_property_create_signed_range(dev, DRM_MODE_PROP_ATOMIC,
> +			"FENCE_FD", -1, INT_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_fence_fd = prop;
> +
>  	prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
>  			"CRTC_ID", DRM_MODE_OBJECT_CRTC);
>  	if (!prop)
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 8cb377c..5ba3cda 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -2122,6 +2122,7 @@ struct drm_mode_config {
>  	struct drm_property *prop_crtc_w;
>  	struct drm_property *prop_crtc_h;
>  	struct drm_property *prop_fb_id;
> +	struct drm_property *prop_fence_fd;
>  	struct drm_property *prop_crtc_id;
>  	struct drm_property *prop_active;
>  	struct drm_property *prop_mode_id;
> -- 
> 2.5.5
> 

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

Re: [RFC 4/8] drm/fence: add in-fences support

[Daniel Vetter]

On Thu, Apr 14, 2016 at 06:29:37PM -0700, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> There is now a new property called FENCE_FD attached to every plane
> state that receives the sync_file fd from userspace via the atomic commit
> IOCTL.
> 
> The fd is then translated to a fence (that may be a fence_collection
> subclass or just a normal fence) and then used by DRM to fence_wait() for
> all fences in the sync_file to signal. So it only commits when all
> framebuffers are ready to scanout.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/Kconfig             | 1 +
>  drivers/gpu/drm/drm_atomic.c        | 8 ++++++++
>  drivers/gpu/drm/drm_atomic_helper.c | 5 +++++
>  drivers/gpu/drm/drm_crtc.c          | 7 +++++++
>  include/drm/drm_crtc.h              | 1 +
>  5 files changed, 22 insertions(+)
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index f2a74d0..3c987e3 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -12,6 +12,7 @@ menuconfig DRM
>  	select I2C
>  	select I2C_ALGOBIT
>  	select DMA_SHARED_BUFFER
> +	select SYNC_FILE
>  	help
>  	  Kernel-level support for the Direct Rendering Infrastructure (DRI)
>  	  introduced in XFree86 4.0. If you say Y here, you need to select
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 8ee1db8..6702502 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -30,6 +30,7 @@
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_mode.h>
>  #include <drm/drm_plane_helper.h>
> +#include <linux/sync_file.h>
>  
>  /**
>   * drm_atomic_state_default_release -
> @@ -680,6 +681,11 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>  		drm_atomic_set_fb_for_plane(state, fb);
>  		if (fb)
>  			drm_framebuffer_unreference(fb);
> +	} else if (property == config->prop_fence_fd) {

Need to fence_put if state->fence is non-NULL already. Could happen if
userspace sets FENCE_FD more than once (another case for your testcase I'd
say).
-Daniel

> +		state->fence = sync_file_fences_get(val);
> +		if (!state->fence)
> +			return -EINVAL;
> +		fence_get(state->fence);
>  	} else if (property == config->prop_crtc_id) {
>  		struct drm_crtc *crtc = drm_crtc_find(dev, val);
>  		return drm_atomic_set_crtc_for_plane(state, crtc);
> @@ -737,6 +743,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>  
>  	if (property == config->prop_fb_id) {
>  		*val = (state->fb) ? state->fb->base.id : 0;
> +	} else if (property == config->prop_fence_fd) {
> +		*val = -1;
>  	} else if (property == config->prop_crtc_id) {
>  		*val = (state->crtc) ? state->crtc->base.id : 0;
>  	} else if (property == config->prop_crtc_x) {
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index f85ef8c..6ed8339 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2687,6 +2687,11 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane *plane,
>  {
>  	if (state->fb)
>  		drm_framebuffer_unreference(state->fb);
> +
> +	if (state->fence) {
> +		fence_put(state->fence);
> +		state->fence = NULL;
> +	}
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>  
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 55ffde5..65212ce 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1278,6 +1278,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  
>  	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
>  		drm_object_attach_property(&plane->base, config->prop_fb_id, 0);
> +		drm_object_attach_property(&plane->base, config->prop_fence_fd, -1);
>  		drm_object_attach_property(&plane->base, config->prop_crtc_id, 0);
>  		drm_object_attach_property(&plane->base, config->prop_crtc_x, 0);
>  		drm_object_attach_property(&plane->base, config->prop_crtc_y, 0);
> @@ -1533,6 +1534,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>  		return -ENOMEM;
>  	dev->mode_config.prop_fb_id = prop;
>  
> +	prop = drm_property_create_signed_range(dev, DRM_MODE_PROP_ATOMIC,
> +			"FENCE_FD", -1, INT_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_fence_fd = prop;
> +
>  	prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
>  			"CRTC_ID", DRM_MODE_OBJECT_CRTC);
>  	if (!prop)
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 8cb377c..5ba3cda 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -2122,6 +2122,7 @@ struct drm_mode_config {
>  	struct drm_property *prop_crtc_w;
>  	struct drm_property *prop_crtc_h;
>  	struct drm_property *prop_fb_id;
> +	struct drm_property *prop_fence_fd;
>  	struct drm_property *prop_crtc_id;
>  	struct drm_property *prop_active;
>  	struct drm_property *prop_mode_id;
> -- 
> 2.5.5
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 8/8] drm/fence: add out-fences support

[Daniel Vetter]

On Thu, Apr 14, 2016 at 06:29:41PM -0700, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Support DRM out-fences creating a sync_file with a fence for each crtc
> update with the DRM_MODE_ATOMIC_OUT_FENCE flag.
> 
> We then send an struct drm_out_fences array with the out-fences fds back in
> the drm_atomic_ioctl() as an out arg in the out_fences_ptr field.
> 
> struct drm_out_fences {
> 	__u32   crtc_id;
> 	__u32   fd;
> };
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/drm_atomic.c        | 109 +++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/drm_atomic_helper.c |   1 +
>  include/drm/drm_crtc.h              |   3 +
>  include/uapi/drm/drm_mode.h         |   7 +++
>  4 files changed, 119 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 0b95526..af6e051 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1560,6 +1560,103 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_atomic_clean_old_fb);
>  
> +static int drm_atomic_get_out_fences(struct drm_device *dev,
> +				     struct drm_atomic_state *state,
> +				     uint32_t __user *out_fences_ptr,
> +				     uint64_t count_out_fences,
> +				     uint64_t user_data)
> +{
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_out_fences *out_fences;
> +	struct sync_file **sync_file;
> +	int num_fences = 0;
> +	int i, ret;
> +
> +	out_fences = kcalloc(count_out_fences, sizeof(*out_fences),
> +			     GFP_KERNEL);
> +	if (!out_fences)
> +		return -ENOMEM;
> +
> +	sync_file = kcalloc(count_out_fences, sizeof(*sync_file),
> +			     GFP_KERNEL);
> +	if (!sync_file) {
> +		kfree(out_fences);
> +		return -ENOMEM;
> +	}
> +
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		struct drm_pending_vblank_event *e;
> +		struct fence *fence;
> +		char name[32];
> +		int fd;
> +
> +		fence = sync_timeline_create_fence(crtc->timeline,
> +						   crtc->fence_seqno);
> +		if (!fence) {
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +
> +		snprintf(name, sizeof(name), "crtc-%d_%lu",
> +			 drm_crtc_index(crtc), crtc->fence_seqno++);
> +
> +		sync_file[i] = sync_file_create(name, fence);
> +		if(!sync_file[i]) {
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +
> +		fd = get_unused_fd_flags(O_CLOEXEC);
> +		if (fd < 0) {
> +			ret = fd;
> +			goto out;
> +		}
> +
> +		sync_file_install(sync_file[i], fd);
> +
> +		if (crtc_state->event) {
> +			crtc_state->event->base.fence = fence;
> +		} else {
> +			e = create_vblank_event(dev, NULL, fence, user_data);
> +			if (!e) {
> +				put_unused_fd(fd);
> +				ret = -ENOMEM;
> +				goto out;
> +			}
> +
> +			crtc_state->event = e;
> +		}
> +		if (num_fences > count_out_fences) {
> +			put_unused_fd(fd);
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
> +		fence_get(fence);
> +
> +		out_fences[num_fences].crtc_id = crtc->base.id;
> +		out_fences[num_fences].fd = fd;
> +		num_fences++;
> +	}
> +
> +	if (copy_to_user(out_fences_ptr, out_fences,
> +			 num_fences * sizeof(*out_fences))) {
> +		ret = -EFAULT;
> +		goto out;
> +	}
> +
> +	return 0;
> +
> +out:
> +	for (i = 0 ; i < count_out_fences ; i++) {
> +		if (sync_file[i])
> +			sync_file_put(sync_file[i]);
> +	}
> +
> +	return ret;
> +}
> +
>  int drm_mode_atomic_ioctl(struct drm_device *dev,
>  			  void *data, struct drm_file *file_priv)
>  {
> @@ -1568,6 +1665,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  	uint32_t __user *count_props_ptr = (uint32_t __user *)(unsigned long)(arg->count_props_ptr);
>  	uint32_t __user *props_ptr = (uint32_t __user *)(unsigned long)(arg->props_ptr);
>  	uint64_t __user *prop_values_ptr = (uint64_t __user *)(unsigned long)(arg->prop_values_ptr);
> +	uint32_t __user *out_fences_ptr = (uint32_t __user *)(unsigned long)(arg->out_fences_ptr);
>  	unsigned int copied_objs, copied_props;
>  	struct drm_atomic_state *state;
>  	struct drm_modeset_acquire_ctx ctx;
> @@ -1601,7 +1699,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  
>  	/* can't test and expect an event at the same time. */
>  	if ((arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) &&
> -			(arg->flags & DRM_MODE_PAGE_FLIP_EVENT))
> +			(arg->flags & (DRM_MODE_PAGE_FLIP_EVENT
> +			 | DRM_MODE_ATOMIC_OUT_FENCE)))
>  		return -EINVAL;
>  
>  	drm_modeset_acquire_init(&ctx, 0);
> @@ -1693,6 +1792,14 @@ retry:
>  		}
>  	}
>  
> +	if (arg->flags & DRM_MODE_ATOMIC_OUT_FENCE) {

OUT_FENCE and TEST_ONLY probably don't make sense, and need to be
rejected. Needs a testcase, too.

> +		ret = drm_atomic_get_out_fences(dev, state, out_fences_ptr,
> +						arg->count_out_fences,
> +						arg->user_data);
> +		if (ret < 0)
> +			goto out;
> +	}
> +
>  	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {

If anything fails below this point we need to clean up the sync_file/fd
mess. Might be easier to first create sync_file objects only, and only
install the fd once atomic has succeeded. You probably want to reserve the
fd slots beforehand though.

That means a bunch more per-crtc state in drm_atomic_state. We should
probably take all the per-crtc pointers and throw them into a small
struct, to avoid allocating individual arrays for everything. So

struct drm_atomic_state_per_crtc {
	struct drm_crtc *crtc;
	struct drm_crtc_state *state;
	struct sync_file *sync_file;
	int fd;
};

Sorry if this means a bit a sprawling upfront refactor :(

Wrt testcase: Need evil ones that pass invalid pointer to out_fences_ptr
or too small array size in count_out_fences and all that ofc.
-Daniel

>  		/*
>  		 * Unlike commit, check_only does not clean up state.
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 6ed8339..15ba3a8 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -31,6 +31,7 @@
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <linux/fence.h>
> +#include <linux/sync_file.h>
>  
>  /**
>   * DOC: overview
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 7934178..53e4e71 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -717,6 +717,7 @@ struct drm_crtc_funcs {
>   * @properties: property tracking for this CRTC
>   * @state: current atomic state for this CRTC
>   * @timeline: sync timeline for fence sigalling
> + * @fence_seqno: seqno variable to create fences
>   * @acquire_ctx: per-CRTC implicit acquire context used by atomic drivers for
>   * 	legacy IOCTLs
>   *
> @@ -773,7 +774,9 @@ struct drm_crtc {
>  
>  	struct drm_crtc_state *state;
>  
> +	/* for out-fences */
>  	struct sync_timeline *timeline;
> +	unsigned long fence_seqno;
>  
>  	/*
>  	 * For legacy crtc IOCTLs so that atomic drivers can get at the locking
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 39905cc..4cdcd22 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -592,6 +592,11 @@ struct drm_mode_destroy_dumb {
>  		DRM_MODE_ATOMIC_ALLOW_MODESET |\
>  		DRM_MODE_ATOMIC_OUT_FENCE)
>  
> +struct drm_out_fences {
> +	__u32	crtc_id;
> +	__u32	fd;
> +};
> +
>  struct drm_mode_atomic {
>  	__u32 flags;
>  	__u32 count_objs;
> @@ -601,6 +606,8 @@ struct drm_mode_atomic {
>  	__u64 prop_values_ptr;
>  	__u64 reserved;
>  	__u64 user_data;
> +	__u64 count_out_fences;
> +	__u64 out_fences_ptr;
>  };
>  
>  /**
> -- 
> 2.5.5
> 

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

Re: [RFC 8/8] drm/fence: add out-fences support

[Daniel Vetter]

On Thu, Apr 14, 2016 at 06:29:41PM -0700, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Support DRM out-fences creating a sync_file with a fence for each crtc
> update with the DRM_MODE_ATOMIC_OUT_FENCE flag.
> 
> We then send an struct drm_out_fences array with the out-fences fds back in
> the drm_atomic_ioctl() as an out arg in the out_fences_ptr field.
> 
> struct drm_out_fences {
> 	__u32   crtc_id;
> 	__u32   fd;
> };
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/drm_atomic.c        | 109 +++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/drm_atomic_helper.c |   1 +
>  include/drm/drm_crtc.h              |   3 +
>  include/uapi/drm/drm_mode.h         |   7 +++
>  4 files changed, 119 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 0b95526..af6e051 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1560,6 +1560,103 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_atomic_clean_old_fb);
>  
> +static int drm_atomic_get_out_fences(struct drm_device *dev,
> +				     struct drm_atomic_state *state,
> +				     uint32_t __user *out_fences_ptr,
> +				     uint64_t count_out_fences,
> +				     uint64_t user_data)
> +{
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_out_fences *out_fences;
> +	struct sync_file **sync_file;
> +	int num_fences = 0;
> +	int i, ret;
> +
> +	out_fences = kcalloc(count_out_fences, sizeof(*out_fences),
> +			     GFP_KERNEL);
> +	if (!out_fences)
> +		return -ENOMEM;
> +
> +	sync_file = kcalloc(count_out_fences, sizeof(*sync_file),
> +			     GFP_KERNEL);
> +	if (!sync_file) {
> +		kfree(out_fences);
> +		return -ENOMEM;
> +	}
> +
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		struct drm_pending_vblank_event *e;
> +		struct fence *fence;
> +		char name[32];
> +		int fd;
> +
> +		fence = sync_timeline_create_fence(crtc->timeline,
> +						   crtc->fence_seqno);
> +		if (!fence) {
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +
> +		snprintf(name, sizeof(name), "crtc-%d_%lu",
> +			 drm_crtc_index(crtc), crtc->fence_seqno++);
> +
> +		sync_file[i] = sync_file_create(name, fence);
> +		if(!sync_file[i]) {
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +
> +		fd = get_unused_fd_flags(O_CLOEXEC);
> +		if (fd < 0) {
> +			ret = fd;
> +			goto out;
> +		}
> +
> +		sync_file_install(sync_file[i], fd);
> +
> +		if (crtc_state->event) {
> +			crtc_state->event->base.fence = fence;
> +		} else {
> +			e = create_vblank_event(dev, NULL, fence, user_data);
> +			if (!e) {
> +				put_unused_fd(fd);
> +				ret = -ENOMEM;
> +				goto out;
> +			}
> +
> +			crtc_state->event = e;
> +		}
> +		if (num_fences > count_out_fences) {
> +			put_unused_fd(fd);
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
> +		fence_get(fence);
> +
> +		out_fences[num_fences].crtc_id = crtc->base.id;
> +		out_fences[num_fences].fd = fd;
> +		num_fences++;
> +	}
> +
> +	if (copy_to_user(out_fences_ptr, out_fences,
> +			 num_fences * sizeof(*out_fences))) {
> +		ret = -EFAULT;
> +		goto out;
> +	}
> +
> +	return 0;
> +
> +out:
> +	for (i = 0 ; i < count_out_fences ; i++) {
> +		if (sync_file[i])
> +			sync_file_put(sync_file[i]);
> +	}
> +
> +	return ret;
> +}
> +
>  int drm_mode_atomic_ioctl(struct drm_device *dev,
>  			  void *data, struct drm_file *file_priv)
>  {
> @@ -1568,6 +1665,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  	uint32_t __user *count_props_ptr = (uint32_t __user *)(unsigned long)(arg->count_props_ptr);
>  	uint32_t __user *props_ptr = (uint32_t __user *)(unsigned long)(arg->props_ptr);
>  	uint64_t __user *prop_values_ptr = (uint64_t __user *)(unsigned long)(arg->prop_values_ptr);
> +	uint32_t __user *out_fences_ptr = (uint32_t __user *)(unsigned long)(arg->out_fences_ptr);
>  	unsigned int copied_objs, copied_props;
>  	struct drm_atomic_state *state;
>  	struct drm_modeset_acquire_ctx ctx;
> @@ -1601,7 +1699,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  
>  	/* can't test and expect an event at the same time. */
>  	if ((arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) &&
> -			(arg->flags & DRM_MODE_PAGE_FLIP_EVENT))
> +			(arg->flags & (DRM_MODE_PAGE_FLIP_EVENT
> +			 | DRM_MODE_ATOMIC_OUT_FENCE)))
>  		return -EINVAL;
>  
>  	drm_modeset_acquire_init(&ctx, 0);
> @@ -1693,6 +1792,14 @@ retry:
>  		}
>  	}
>  
> +	if (arg->flags & DRM_MODE_ATOMIC_OUT_FENCE) {

OUT_FENCE and TEST_ONLY probably don't make sense, and need to be
rejected. Needs a testcase, too.

> +		ret = drm_atomic_get_out_fences(dev, state, out_fences_ptr,
> +						arg->count_out_fences,
> +						arg->user_data);
> +		if (ret < 0)
> +			goto out;
> +	}
> +
>  	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {

If anything fails below this point we need to clean up the sync_file/fd
mess. Might be easier to first create sync_file objects only, and only
install the fd once atomic has succeeded. You probably want to reserve the
fd slots beforehand though.

That means a bunch more per-crtc state in drm_atomic_state. We should
probably take all the per-crtc pointers and throw them into a small
struct, to avoid allocating individual arrays for everything. So

struct drm_atomic_state_per_crtc {
	struct drm_crtc *crtc;
	struct drm_crtc_state *state;
	struct sync_file *sync_file;
	int fd;
};

Sorry if this means a bit a sprawling upfront refactor :(

Wrt testcase: Need evil ones that pass invalid pointer to out_fences_ptr
or too small array size in count_out_fences and all that ofc.
-Daniel

>  		/*
>  		 * Unlike commit, check_only does not clean up state.
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 6ed8339..15ba3a8 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -31,6 +31,7 @@
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <linux/fence.h>
> +#include <linux/sync_file.h>
>  
>  /**
>   * DOC: overview
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 7934178..53e4e71 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -717,6 +717,7 @@ struct drm_crtc_funcs {
>   * @properties: property tracking for this CRTC
>   * @state: current atomic state for this CRTC
>   * @timeline: sync timeline for fence sigalling
> + * @fence_seqno: seqno variable to create fences
>   * @acquire_ctx: per-CRTC implicit acquire context used by atomic drivers for
>   * 	legacy IOCTLs
>   *
> @@ -773,7 +774,9 @@ struct drm_crtc {
>  
>  	struct drm_crtc_state *state;
>  
> +	/* for out-fences */
>  	struct sync_timeline *timeline;
> +	unsigned long fence_seqno;
>  
>  	/*
>  	 * For legacy crtc IOCTLs so that atomic drivers can get at the locking
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 39905cc..4cdcd22 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -592,6 +592,11 @@ struct drm_mode_destroy_dumb {
>  		DRM_MODE_ATOMIC_ALLOW_MODESET |\
>  		DRM_MODE_ATOMIC_OUT_FENCE)
>  
> +struct drm_out_fences {
> +	__u32	crtc_id;
> +	__u32	fd;
> +};
> +
>  struct drm_mode_atomic {
>  	__u32 flags;
>  	__u32 count_objs;
> @@ -601,6 +606,8 @@ struct drm_mode_atomic {
>  	__u64 prop_values_ptr;
>  	__u64 reserved;
>  	__u64 user_data;
> +	__u64 count_out_fences;
> +	__u64 out_fences_ptr;
>  };
>  
>  /**
> -- 
> 2.5.5
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 1/8] dma-buf/fence: add fence_collection fences

[Christian König]

Am 15.04.2016 um 10:02 schrieb Daniel Vetter:
> On Thu, Apr 14, 2016 at 06:29:34PM -0700, Gustavo Padovan wrote:
>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>
>> struct fence_collection inherits from struct fence and carries a
>> collection of fences that needs to be waited together.
>>
>> It is useful to translate a sync_file to a fence to remove the complexity
>> of dealing with sync_files from DRM drivers. So even if there are many
>> fences in the sync_file that needs to waited for a commit to happen
>> drivers would only worry about a standard struct fence.That means that no
>> changes needed to any driver besides supporting fences.
>>
>> fence_collection's fence doesn't belong to any timeline context.
>>
>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>> ---
>>   drivers/dma-buf/Makefile           |   2 +-
>>   drivers/dma-buf/fence-collection.c | 138 +++++++++++++++++++++++++++++++++++++
>>   drivers/dma-buf/fence.c            |   2 +-
>>   include/linux/fence-collection.h   |  56 +++++++++++++++
>>   include/linux/fence.h              |   2 +
>>   5 files changed, 198 insertions(+), 2 deletions(-)
>>   create mode 100644 drivers/dma-buf/fence-collection.c
>>   create mode 100644 include/linux/fence-collection.h
>>
>> diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile
>> index 43325a1..30b8464 100644
>> --- a/drivers/dma-buf/Makefile
>> +++ b/drivers/dma-buf/Makefile
>> @@ -1,3 +1,3 @@
>> -obj-y := dma-buf.o fence.o reservation.o seqno-fence.o
>> +obj-y := dma-buf.o fence.o reservation.o seqno-fence.o fence-collection.o
>>   obj-$(CONFIG_SYNC_FILE)		+= sync_file.o sync_timeline.o sync_debug.o
>>   obj-$(CONFIG_SW_SYNC)		+= sw_sync.o
>> diff --git a/drivers/dma-buf/fence-collection.c b/drivers/dma-buf/fence-collection.c
>> new file mode 100644
>> index 0000000..8a4ecb0
>> --- /dev/null
>> +++ b/drivers/dma-buf/fence-collection.c
>> @@ -0,0 +1,138 @@
>> +/*
>> + * fence-collection: aggregate fences to be waited together
>> + *
>> + * Copyright (C) 2016 Collabora Ltd
>> + * Authors:
>> + *	Gustavo Padovan <gustavo@padovan.org>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published by
>> + * the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>> + * more details.
>> + */
>> +
>> +#include <linux/export.h>
>> +#include <linux/slab.h>
>> +#include <linux/fence-collection.h>
>> +
>> +static const char *fence_collection_get_driver_name(struct fence *fence)
>> +{
>> +	struct fence_collection *collection = to_fence_collection(fence);
>> +	struct fence *f = collection->fences[0].fence;
>> +
>> +	return f->ops->get_driver_name(fence);
>> +}

I would rather return some constant name here instead of relying that 
the collection already has a fence added and that all fences are from 
the same driver.

>> +
>> +static const char *fence_collection_get_timeline_name(struct fence *fence)
>> +{
>> +	return "no context";
>> +}
>> +
>> +static bool fence_collection_enable_signaling(struct fence *fence)
>> +{
>> +	struct fence_collection *collection = to_fence_collection(fence);
>> +
>> +	return atomic_read(&collection->num_pending_fences);
>> +}
>> +
>> +static bool fence_collection_signaled(struct fence *fence)
>> +{
>> +	struct fence_collection *collection = to_fence_collection(fence);
>> +
>> +	return (atomic_read(&collection->num_pending_fences) == 0);
>> +}
>> +
>> +static void fence_collection_release(struct fence *fence)
>> +{
>> +	struct fence_collection *collection = to_fence_collection(fence);
>> +	int i;
>> +
>> +	for (i = 0 ; i < collection->num_fences ; i++) {
>> +		fence_remove_callback(collection->fences[i].fence,
>> +				      &collection->fences[i].cb);
>> +		fence_put(collection->fences[i].fence);
>> +	}
>> +
>> +	fence_free(fence);
>> +}
>> +
>> +static signed long fence_collection_wait(struct fence *fence, bool intr,
>> +					 signed long timeout)
>> +{
>> +	struct fence_collection *collection = to_fence_collection(fence);
>> +	int i;
>> +
>> +	for (i = 0 ; i < collection->num_fences ; i++) {
>> +		timeout = fence_wait(collection->fences[i].fence, intr);
>> +		if (timeout < 0)
>> +			return timeout;
>> +	}
>> +
>> +	return timeout;
>> +}
>> +
>> +static const struct fence_ops fence_collection_ops = {
>> +	.get_driver_name = fence_collection_get_driver_name,
>> +	.get_timeline_name = fence_collection_get_timeline_name,
>> +	.enable_signaling = fence_collection_enable_signaling,
>> +	.signaled = fence_collection_signaled,
>> +	.wait = fence_collection_wait,
>> +	.release = fence_collection_release,
>> +};
>> +
>> +static void collection_check_cb_func(struct fence *fence, struct fence_cb *cb)
>> +{
>> +	struct fence_collection_cb *f_cb;
>> +	struct fence_collection *collection;
>> +
>> +	f_cb = container_of(cb, struct fence_collection_cb, cb);
>> +	collection = f_cb->collection;
>> +
>> +	if (atomic_dec_and_test(&collection->num_pending_fences))
>> +		fence_signal(&collection->base);
>> +}
>> +
>> +void fence_collection_add(struct fence_collection *collection,
>> +			  struct fence *fence)
>> +{
>> +	int n = collection->num_fences;
>> +
>> +	collection->fences[n].collection = collection;
>> +	collection->fences[n].fence = fence;
>> +
>> +	if (fence_add_callback(fence, &collection->fences[n].cb,
>> +				 collection_check_cb_func))
>> +		return;
>> +
>> +	fence_get(fence);
>> +
>> +	collection->num_fences++;
>> +	atomic_inc(&collection->num_pending_fences);
>> +}
> For the interface I think we should not split it into _init and _add - it
> shouldn't be allowed to change a collection once it's created. So probably
> cleaner if we add an array of fence pointers to _init.

Amdgpu also has an implementation for a fence collection which uses a a 
hashtable to keep the fences grouped by context (e.g. only the latest 
fence is keept for each context). See amdgpu_sync.c for reference.

We should either make the collection similar in a way that you can add 
as many fences as you want (like the amdgpu implementation) or make it 
static and only add a fixed number of fences right from the beginning.

I can certainly see use cases for both, but if you want to stick with a 
static approach you should probably call the new object fence_array 
instead of fence_collection and do as Daniel suggested.

> Other nitpick: Adding the callback should (I think) only be done in
> ->enable_signalling.

Yeah, I was about to complain on that as well.

Enabling signaling can have a huge overhead for some fence 
implementations. So it should only be used when needed

>
> Finally: Have you looked into stitching together a few unit tests for
> fence_collection?
>
> Fence collections also break the assumption that every fence is on a
> timeline. fence_later and fence_is_later need to be adjusted. We also need
> a special collection context to filter these out. This means
> fence_collection isn't perfectly opaque abstraction.
>> +
>> +struct fence_collection *fence_collection_init(int num_fences)
>> +{
>> +	struct fence_collection *collection;
>> +
>> +	collection = kzalloc(offsetof(struct fence_collection,
>> +				      fences[num_fences]), GFP_KERNEL);
>> +	if (!collection)
>> +		return NULL;
>> +
>> +	spin_lock_init(&collection->lock);
>> +	fence_init(&collection->base, &fence_collection_ops, &collection->lock,
>> +		   FENCE_NO_CONTEXT, 0);
>> +
>> +	return collection;
>> +}
>> +EXPORT_SYMBOL(fence_collection_init);
>> +
>> +void fence_collection_put(struct fence_collection *collection)
>> +{
>> +	fence_put(&collection->base);
> Not sure a specialized _put function is useful, I'd leave it out.
>
>> +}
>> +EXPORT_SYMBOL(fence_collection_put);
>> diff --git a/drivers/dma-buf/fence.c b/drivers/dma-buf/fence.c
>> index 7b05dbe..486e95c 100644
>> --- a/drivers/dma-buf/fence.c
>> +++ b/drivers/dma-buf/fence.c
>> @@ -35,7 +35,7 @@ EXPORT_TRACEPOINT_SYMBOL(fence_emit);
>>    * context or not. One device can have multiple separate contexts,
>>    * and they're used if some engine can run independently of another.
>>    */
>> -static atomic_t fence_context_counter = ATOMIC_INIT(0);
>> +static atomic_t fence_context_counter = ATOMIC_INIT(1);
>>   
>>   /**
>>    * fence_context_alloc - allocate an array of fence contexts
>> diff --git a/include/linux/fence-collection.h b/include/linux/fence-collection.h
>> new file mode 100644
>> index 0000000..a798925
>> --- /dev/null
>> +++ b/include/linux/fence-collection.h
>> @@ -0,0 +1,56 @@
>> +/*
>> + * fence-collection: aggregates fence to be waited together
>> + *
>> + * Copyright (C) 2016 Collabora Ltd
>> + * Authors:
>> + *	Gustavo Padovan <gustavo@padovan.org>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published by
>> + * the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>> + * more details.
>> + */
>> +
>> +#ifndef __LINUX_FENCE_COLLECTION_H
>> +#define __LINUX_FENCE_COLLECTION_H
>> +
>> +#include <linux/fence.h>
>> +
>> +struct fence_collection_cb {
>> +	struct fence_cb cb;
>> +	struct fence *fence;
>> +	struct fence_collection *collection;
>> +};
>> +
>> +struct fence_collection {
>> +	struct fence base;
>> +
>> +	spinlock_t lock;
>> +	struct fence_cb fence_cb;
>> +	atomic_t num_pending_fences;
>> +	int num_fences;
>> +	struct fence_collection_cb fences[];
>> +};
>> +
>> +/**
>> + * to_fence_collection - cast a fence to a fence_collection
>> + * @fence: fence to cast to a fence_collection
>> + *
>> + * Returns NULL if the fence is not a fence_collection,
>> + * or the fence_collection otherwise.
>> + */
>> +static inline struct fence_collection * to_fence_collection(struct fence *fence)
>> +{
> Kerneldoc claims it, but you don't check that the fence is indeed a
> fence_collection. That's usually done by comparing the ops pointer.
>
>> +	return container_of(fence, struct fence_collection, base);
>> +}
>> +
>> +struct fence_collection *fence_collection_init(int num_fences);
>> +void fence_collection_add(struct fence_collection *collection,
>> +			  struct fence *fence);
>> +void fence_collection_put(struct fence_collection *collection);
>> +
>> +#endif /* __LINUX_FENCE_COLLECTION_H */
>> diff --git a/include/linux/fence.h b/include/linux/fence.h
>> index 2b17698..02170dd 100644
>> --- a/include/linux/fence.h
>> +++ b/include/linux/fence.h
>> @@ -30,6 +30,8 @@
>>   #include <linux/printk.h>
>>   #include <linux/rcupdate.h>
>>   
>> +#define FENCE_NO_CONTEXT 0

Might be that how amdgpu uses the fence context and sequence number is a 
bit questionable, but this will completely break it.

So we need a fix for amdgpu as well before this can go upstream.

Regards,
Christian.

>> +
>>   struct fence;
>>   struct fence_ops;
>>   struct fence_cb;
>> -- 
>> 2.5.5
>>

Re: [RFC 1/8] dma-buf/fence: add fence_collection fences

[Christian König]

Am 15.04.2016 um 10:02 schrieb Daniel Vetter:
> On Thu, Apr 14, 2016 at 06:29:34PM -0700, Gustavo Padovan wrote:
>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>
>> struct fence_collection inherits from struct fence and carries a
>> collection of fences that needs to be waited together.
>>
>> It is useful to translate a sync_file to a fence to remove the complexity
>> of dealing with sync_files from DRM drivers. So even if there are many
>> fences in the sync_file that needs to waited for a commit to happen
>> drivers would only worry about a standard struct fence.That means that no
>> changes needed to any driver besides supporting fences.
>>
>> fence_collection's fence doesn't belong to any timeline context.
>>
>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>> ---
>>   drivers/dma-buf/Makefile           |   2 +-
>>   drivers/dma-buf/fence-collection.c | 138 +++++++++++++++++++++++++++++++++++++
>>   drivers/dma-buf/fence.c            |   2 +-
>>   include/linux/fence-collection.h   |  56 +++++++++++++++
>>   include/linux/fence.h              |   2 +
>>   5 files changed, 198 insertions(+), 2 deletions(-)
>>   create mode 100644 drivers/dma-buf/fence-collection.c
>>   create mode 100644 include/linux/fence-collection.h
>>
>> diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile
>> index 43325a1..30b8464 100644
>> --- a/drivers/dma-buf/Makefile
>> +++ b/drivers/dma-buf/Makefile
>> @@ -1,3 +1,3 @@
>> -obj-y := dma-buf.o fence.o reservation.o seqno-fence.o
>> +obj-y := dma-buf.o fence.o reservation.o seqno-fence.o fence-collection.o
>>   obj-$(CONFIG_SYNC_FILE)		+= sync_file.o sync_timeline.o sync_debug.o
>>   obj-$(CONFIG_SW_SYNC)		+= sw_sync.o
>> diff --git a/drivers/dma-buf/fence-collection.c b/drivers/dma-buf/fence-collection.c
>> new file mode 100644
>> index 0000000..8a4ecb0
>> --- /dev/null
>> +++ b/drivers/dma-buf/fence-collection.c
>> @@ -0,0 +1,138 @@
>> +/*
>> + * fence-collection: aggregate fences to be waited together
>> + *
>> + * Copyright (C) 2016 Collabora Ltd
>> + * Authors:
>> + *	Gustavo Padovan <gustavo@padovan.org>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published by
>> + * the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>> + * more details.
>> + */
>> +
>> +#include <linux/export.h>
>> +#include <linux/slab.h>
>> +#include <linux/fence-collection.h>
>> +
>> +static const char *fence_collection_get_driver_name(struct fence *fence)
>> +{
>> +	struct fence_collection *collection = to_fence_collection(fence);
>> +	struct fence *f = collection->fences[0].fence;
>> +
>> +	return f->ops->get_driver_name(fence);
>> +}

I would rather return some constant name here instead of relying that 
the collection already has a fence added and that all fences are from 
the same driver.

>> +
>> +static const char *fence_collection_get_timeline_name(struct fence *fence)
>> +{
>> +	return "no context";
>> +}
>> +
>> +static bool fence_collection_enable_signaling(struct fence *fence)
>> +{
>> +	struct fence_collection *collection = to_fence_collection(fence);
>> +
>> +	return atomic_read(&collection->num_pending_fences);
>> +}
>> +
>> +static bool fence_collection_signaled(struct fence *fence)
>> +{
>> +	struct fence_collection *collection = to_fence_collection(fence);
>> +
>> +	return (atomic_read(&collection->num_pending_fences) == 0);
>> +}
>> +
>> +static void fence_collection_release(struct fence *fence)
>> +{
>> +	struct fence_collection *collection = to_fence_collection(fence);
>> +	int i;
>> +
>> +	for (i = 0 ; i < collection->num_fences ; i++) {
>> +		fence_remove_callback(collection->fences[i].fence,
>> +				      &collection->fences[i].cb);
>> +		fence_put(collection->fences[i].fence);
>> +	}
>> +
>> +	fence_free(fence);
>> +}
>> +
>> +static signed long fence_collection_wait(struct fence *fence, bool intr,
>> +					 signed long timeout)
>> +{
>> +	struct fence_collection *collection = to_fence_collection(fence);
>> +	int i;
>> +
>> +	for (i = 0 ; i < collection->num_fences ; i++) {
>> +		timeout = fence_wait(collection->fences[i].fence, intr);
>> +		if (timeout < 0)
>> +			return timeout;
>> +	}
>> +
>> +	return timeout;
>> +}
>> +
>> +static const struct fence_ops fence_collection_ops = {
>> +	.get_driver_name = fence_collection_get_driver_name,
>> +	.get_timeline_name = fence_collection_get_timeline_name,
>> +	.enable_signaling = fence_collection_enable_signaling,
>> +	.signaled = fence_collection_signaled,
>> +	.wait = fence_collection_wait,
>> +	.release = fence_collection_release,
>> +};
>> +
>> +static void collection_check_cb_func(struct fence *fence, struct fence_cb *cb)
>> +{
>> +	struct fence_collection_cb *f_cb;
>> +	struct fence_collection *collection;
>> +
>> +	f_cb = container_of(cb, struct fence_collection_cb, cb);
>> +	collection = f_cb->collection;
>> +
>> +	if (atomic_dec_and_test(&collection->num_pending_fences))
>> +		fence_signal(&collection->base);
>> +}
>> +
>> +void fence_collection_add(struct fence_collection *collection,
>> +			  struct fence *fence)
>> +{
>> +	int n = collection->num_fences;
>> +
>> +	collection->fences[n].collection = collection;
>> +	collection->fences[n].fence = fence;
>> +
>> +	if (fence_add_callback(fence, &collection->fences[n].cb,
>> +				 collection_check_cb_func))
>> +		return;
>> +
>> +	fence_get(fence);
>> +
>> +	collection->num_fences++;
>> +	atomic_inc(&collection->num_pending_fences);
>> +}
> For the interface I think we should not split it into _init and _add - it
> shouldn't be allowed to change a collection once it's created. So probably
> cleaner if we add an array of fence pointers to _init.

Amdgpu also has an implementation for a fence collection which uses a a 
hashtable to keep the fences grouped by context (e.g. only the latest 
fence is keept for each context). See amdgpu_sync.c for reference.

We should either make the collection similar in a way that you can add 
as many fences as you want (like the amdgpu implementation) or make it 
static and only add a fixed number of fences right from the beginning.

I can certainly see use cases for both, but if you want to stick with a 
static approach you should probably call the new object fence_array 
instead of fence_collection and do as Daniel suggested.

> Other nitpick: Adding the callback should (I think) only be done in
> ->enable_signalling.

Yeah, I was about to complain on that as well.

Enabling signaling can have a huge overhead for some fence 
implementations. So it should only be used when needed

>
> Finally: Have you looked into stitching together a few unit tests for
> fence_collection?
>
> Fence collections also break the assumption that every fence is on a
> timeline. fence_later and fence_is_later need to be adjusted. We also need
> a special collection context to filter these out. This means
> fence_collection isn't perfectly opaque abstraction.
>> +
>> +struct fence_collection *fence_collection_init(int num_fences)
>> +{
>> +	struct fence_collection *collection;
>> +
>> +	collection = kzalloc(offsetof(struct fence_collection,
>> +				      fences[num_fences]), GFP_KERNEL);
>> +	if (!collection)
>> +		return NULL;
>> +
>> +	spin_lock_init(&collection->lock);
>> +	fence_init(&collection->base, &fence_collection_ops, &collection->lock,
>> +		   FENCE_NO_CONTEXT, 0);
>> +
>> +	return collection;
>> +}
>> +EXPORT_SYMBOL(fence_collection_init);
>> +
>> +void fence_collection_put(struct fence_collection *collection)
>> +{
>> +	fence_put(&collection->base);
> Not sure a specialized _put function is useful, I'd leave it out.
>
>> +}
>> +EXPORT_SYMBOL(fence_collection_put);
>> diff --git a/drivers/dma-buf/fence.c b/drivers/dma-buf/fence.c
>> index 7b05dbe..486e95c 100644
>> --- a/drivers/dma-buf/fence.c
>> +++ b/drivers/dma-buf/fence.c
>> @@ -35,7 +35,7 @@ EXPORT_TRACEPOINT_SYMBOL(fence_emit);
>>    * context or not. One device can have multiple separate contexts,
>>    * and they're used if some engine can run independently of another.
>>    */
>> -static atomic_t fence_context_counter = ATOMIC_INIT(0);
>> +static atomic_t fence_context_counter = ATOMIC_INIT(1);
>>   
>>   /**
>>    * fence_context_alloc - allocate an array of fence contexts
>> diff --git a/include/linux/fence-collection.h b/include/linux/fence-collection.h
>> new file mode 100644
>> index 0000000..a798925
>> --- /dev/null
>> +++ b/include/linux/fence-collection.h
>> @@ -0,0 +1,56 @@
>> +/*
>> + * fence-collection: aggregates fence to be waited together
>> + *
>> + * Copyright (C) 2016 Collabora Ltd
>> + * Authors:
>> + *	Gustavo Padovan <gustavo@padovan.org>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published by
>> + * the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>> + * more details.
>> + */
>> +
>> +#ifndef __LINUX_FENCE_COLLECTION_H
>> +#define __LINUX_FENCE_COLLECTION_H
>> +
>> +#include <linux/fence.h>
>> +
>> +struct fence_collection_cb {
>> +	struct fence_cb cb;
>> +	struct fence *fence;
>> +	struct fence_collection *collection;
>> +};
>> +
>> +struct fence_collection {
>> +	struct fence base;
>> +
>> +	spinlock_t lock;
>> +	struct fence_cb fence_cb;
>> +	atomic_t num_pending_fences;
>> +	int num_fences;
>> +	struct fence_collection_cb fences[];
>> +};
>> +
>> +/**
>> + * to_fence_collection - cast a fence to a fence_collection
>> + * @fence: fence to cast to a fence_collection
>> + *
>> + * Returns NULL if the fence is not a fence_collection,
>> + * or the fence_collection otherwise.
>> + */
>> +static inline struct fence_collection * to_fence_collection(struct fence *fence)
>> +{
> Kerneldoc claims it, but you don't check that the fence is indeed a
> fence_collection. That's usually done by comparing the ops pointer.
>
>> +	return container_of(fence, struct fence_collection, base);
>> +}
>> +
>> +struct fence_collection *fence_collection_init(int num_fences);
>> +void fence_collection_add(struct fence_collection *collection,
>> +			  struct fence *fence);
>> +void fence_collection_put(struct fence_collection *collection);
>> +
>> +#endif /* __LINUX_FENCE_COLLECTION_H */
>> diff --git a/include/linux/fence.h b/include/linux/fence.h
>> index 2b17698..02170dd 100644
>> --- a/include/linux/fence.h
>> +++ b/include/linux/fence.h
>> @@ -30,6 +30,8 @@
>>   #include <linux/printk.h>
>>   #include <linux/rcupdate.h>
>>   
>> +#define FENCE_NO_CONTEXT 0

Might be that how amdgpu uses the fence context and sequence number is a 
bit questionable, but this will completely break it.

So we need a fix for amdgpu as well before this can go upstream.

Regards,
Christian.

>> +
>>   struct fence;
>>   struct fence_ops;
>>   struct fence_cb;
>> -- 
>> 2.5.5
>>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 1/8] dma-buf/fence: add fence_collection fences

[Daniel Vetter]

On Fri, Apr 15, 2016 at 11:03 AM, Christian König
<christian.koenig@amd.com> wrote:
> Might be that how amdgpu uses the fence context and sequence number is a bit
> questionable, but this will completely break it.

You mean it tries to qualesce fences in the same context down to just
the last one? That's how it's supposed to be done, and
fence_collections do break this somewhat. Without fixing up
fence_is_later and friends. Sounds like amdgpu is a good use case to
make sure the changes in semantics in these functions result in
sensible code. In a way a fence_collection is a fence where the
timeline never matches with any other timeline (since it's a
combiation).

And yeah I think fence_collection should probably compress down the
fences to 1 per timeline. But then that's just an implementation
detail we can fix later on.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC 1/8] dma-buf/fence: add fence_collection fences

[Daniel Vetter]

On Fri, Apr 15, 2016 at 11:03 AM, Christian König
<christian.koenig@amd.com> wrote:
> Might be that how amdgpu uses the fence context and sequence number is a bit
> questionable, but this will completely break it.

You mean it tries to qualesce fences in the same context down to just
the last one? That's how it's supposed to be done, and
fence_collections do break this somewhat. Without fixing up
fence_is_later and friends. Sounds like amdgpu is a good use case to
make sure the changes in semantics in these functions result in
sensible code. In a way a fence_collection is a fence where the
timeline never matches with any other timeline (since it's a
combiation).

And yeah I think fence_collection should probably compress down the
fences to 1 per timeline. But then that's just an implementation
detail we can fix later on.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 1/8] dma-buf/fence: add fence_collection fences

[Gustavo Padovan]

2016-04-15 Christian König <christian.koenig@amd.com>:

> Am 15.04.2016 um 10:02 schrieb Daniel Vetter:
> >On Thu, Apr 14, 2016 at 06:29:34PM -0700, Gustavo Padovan wrote:
> >>From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> >>
> >>struct fence_collection inherits from struct fence and carries a
> >>collection of fences that needs to be waited together.
> >>
> >>It is useful to translate a sync_file to a fence to remove the complexity
> >>of dealing with sync_files from DRM drivers. So even if there are many
> >>fences in the sync_file that needs to waited for a commit to happen
> >>drivers would only worry about a standard struct fence.That means that no
> >>changes needed to any driver besides supporting fences.
> >>
> >>fence_collection's fence doesn't belong to any timeline context.
> >>
> >>Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> >>---
> >>  drivers/dma-buf/Makefile           |   2 +-
> >>  drivers/dma-buf/fence-collection.c | 138 +++++++++++++++++++++++++++++++++++++
> >>  drivers/dma-buf/fence.c            |   2 +-
> >>  include/linux/fence-collection.h   |  56 +++++++++++++++
> >>  include/linux/fence.h              |   2 +
> >>  5 files changed, 198 insertions(+), 2 deletions(-)
> >>  create mode 100644 drivers/dma-buf/fence-collection.c
> >>  create mode 100644 include/linux/fence-collection.h
> >>
> >>diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile
> >>index 43325a1..30b8464 100644
> >>--- a/drivers/dma-buf/Makefile
> >>+++ b/drivers/dma-buf/Makefile
> >>@@ -1,3 +1,3 @@
> >>-obj-y := dma-buf.o fence.o reservation.o seqno-fence.o
> >>+obj-y := dma-buf.o fence.o reservation.o seqno-fence.o fence-collection.o
> >>  obj-$(CONFIG_SYNC_FILE)		+= sync_file.o sync_timeline.o sync_debug.o
> >>  obj-$(CONFIG_SW_SYNC)		+= sw_sync.o
> >>diff --git a/drivers/dma-buf/fence-collection.c b/drivers/dma-buf/fence-collection.c
> >>new file mode 100644
> >>index 0000000..8a4ecb0
> >>--- /dev/null
> >>+++ b/drivers/dma-buf/fence-collection.c
> >>@@ -0,0 +1,138 @@
> >>+/*
> >>+ * fence-collection: aggregate fences to be waited together
> >>+ *
> >>+ * Copyright (C) 2016 Collabora Ltd
> >>+ * Authors:
> >>+ *	Gustavo Padovan <gustavo@padovan.org>
> >>+ *
> >>+ * This program is free software; you can redistribute it and/or modify it
> >>+ * under the terms of the GNU General Public License version 2 as published by
> >>+ * the Free Software Foundation.
> >>+ *
> >>+ * This program is distributed in the hope that it will be useful, but WITHOUT
> >>+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> >>+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> >>+ * more details.
> >>+ */
> >>+
> >>+#include <linux/export.h>
> >>+#include <linux/slab.h>
> >>+#include <linux/fence-collection.h>
> >>+
> >>+static const char *fence_collection_get_driver_name(struct fence *fence)
> >>+{
> >>+	struct fence_collection *collection = to_fence_collection(fence);
> >>+	struct fence *f = collection->fences[0].fence;
> >>+
> >>+	return f->ops->get_driver_name(fence);
> >>+}
> 
> I would rather return some constant name here instead of relying that the
> collection already has a fence added and that all fences are from the same
> driver.

If we merge _init and _add this will not be a problem anymore and we can
return the actual driver name.

> 
> >>+
> >>+static const char *fence_collection_get_timeline_name(struct fence *fence)
> >>+{
> >>+	return "no context";
> >>+}
> >>+
> >>+static bool fence_collection_enable_signaling(struct fence *fence)
> >>+{
> >>+	struct fence_collection *collection = to_fence_collection(fence);
> >>+
> >>+	return atomic_read(&collection->num_pending_fences);
> >>+}
> >>+
> >>+static bool fence_collection_signaled(struct fence *fence)
> >>+{
> >>+	struct fence_collection *collection = to_fence_collection(fence);
> >>+
> >>+	return (atomic_read(&collection->num_pending_fences) == 0);
> >>+}
> >>+
> >>+static void fence_collection_release(struct fence *fence)
> >>+{
> >>+	struct fence_collection *collection = to_fence_collection(fence);
> >>+	int i;
> >>+
> >>+	for (i = 0 ; i < collection->num_fences ; i++) {
> >>+		fence_remove_callback(collection->fences[i].fence,
> >>+				      &collection->fences[i].cb);
> >>+		fence_put(collection->fences[i].fence);
> >>+	}
> >>+
> >>+	fence_free(fence);
> >>+}
> >>+
> >>+static signed long fence_collection_wait(struct fence *fence, bool intr,
> >>+					 signed long timeout)
> >>+{
> >>+	struct fence_collection *collection = to_fence_collection(fence);
> >>+	int i;
> >>+
> >>+	for (i = 0 ; i < collection->num_fences ; i++) {
> >>+		timeout = fence_wait(collection->fences[i].fence, intr);
> >>+		if (timeout < 0)
> >>+			return timeout;
> >>+	}
> >>+
> >>+	return timeout;
> >>+}
> >>+
> >>+static const struct fence_ops fence_collection_ops = {
> >>+	.get_driver_name = fence_collection_get_driver_name,
> >>+	.get_timeline_name = fence_collection_get_timeline_name,
> >>+	.enable_signaling = fence_collection_enable_signaling,
> >>+	.signaled = fence_collection_signaled,
> >>+	.wait = fence_collection_wait,
> >>+	.release = fence_collection_release,
> >>+};
> >>+
> >>+static void collection_check_cb_func(struct fence *fence, struct fence_cb *cb)
> >>+{
> >>+	struct fence_collection_cb *f_cb;
> >>+	struct fence_collection *collection;
> >>+
> >>+	f_cb = container_of(cb, struct fence_collection_cb, cb);
> >>+	collection = f_cb->collection;
> >>+
> >>+	if (atomic_dec_and_test(&collection->num_pending_fences))
> >>+		fence_signal(&collection->base);
> >>+}
> >>+
> >>+void fence_collection_add(struct fence_collection *collection,
> >>+			  struct fence *fence)
> >>+{
> >>+	int n = collection->num_fences;
> >>+
> >>+	collection->fences[n].collection = collection;
> >>+	collection->fences[n].fence = fence;
> >>+
> >>+	if (fence_add_callback(fence, &collection->fences[n].cb,
> >>+				 collection_check_cb_func))
> >>+		return;
> >>+
> >>+	fence_get(fence);
> >>+
> >>+	collection->num_fences++;
> >>+	atomic_inc(&collection->num_pending_fences);
> >>+}
> >For the interface I think we should not split it into _init and _add - it
> >shouldn't be allowed to change a collection once it's created. So probably
> >cleaner if we add an array of fence pointers to _init.
> 
> Amdgpu also has an implementation for a fence collection which uses a a
> hashtable to keep the fences grouped by context (e.g. only the latest fence
> is keept for each context). See amdgpu_sync.c for reference.
> 
> We should either make the collection similar in a way that you can add as
> many fences as you want (like the amdgpu implementation) or make it static
> and only add a fixed number of fences right from the beginning.
> 
> I can certainly see use cases for both, but if you want to stick with a
> static approach you should probably call the new object fence_array instead
> of fence_collection and do as Daniel suggested.

Maybe we can go for something in between. Have fence_collection_init()
need at least two fences to create the fence_collection. Then
fence_collection_add() would add more dinamically.

> 
> >Other nitpick: Adding the callback should (I think) only be done in
> >->enable_signalling.
> 
> Yeah, I was about to complain on that as well.
> 
> Enabling signaling can have a huge overhead for some fence implementations.
> So it should only be used when needed

Right, that makes sense.

> 
> >
> >Finally: Have you looked into stitching together a few unit tests for
> >fence_collection?

Not yet. It is something I plan to work soon.

> >
> >Fence collections also break the assumption that every fence is on a
> >timeline. fence_later and fence_is_later need to be adjusted. We also need
> >a special collection context to filter these out. This means
> >fence_collection isn't perfectly opaque abstraction.

I'll fix this.

> >>+
> >>+struct fence_collection *fence_collection_init(int num_fences)
> >>+{
> >>+	struct fence_collection *collection;
> >>+
> >>+	collection = kzalloc(offsetof(struct fence_collection,
> >>+				      fences[num_fences]), GFP_KERNEL);
> >>+	if (!collection)
> >>+		return NULL;
> >>+
> >>+	spin_lock_init(&collection->lock);
> >>+	fence_init(&collection->base, &fence_collection_ops, &collection->lock,
> >>+		   FENCE_NO_CONTEXT, 0);
> >>+
> >>+	return collection;
> >>+}
> >>+EXPORT_SYMBOL(fence_collection_init);
> >>+
> >>+void fence_collection_put(struct fence_collection *collection)
> >>+{
> >>+	fence_put(&collection->base);
> >Not sure a specialized _put function is useful, I'd leave it out.

I've added this to let the user only deal with fence_collection, but 
I'm fine leaving it out too.

	Gustavo

Re: [RFC 1/8] dma-buf/fence: add fence_collection fences

[Gustavo Padovan]

2016-04-15 Christian König <christian.koenig@amd.com>:

> Am 15.04.2016 um 10:02 schrieb Daniel Vetter:
> >On Thu, Apr 14, 2016 at 06:29:34PM -0700, Gustavo Padovan wrote:
> >>From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> >>
> >>struct fence_collection inherits from struct fence and carries a
> >>collection of fences that needs to be waited together.
> >>
> >>It is useful to translate a sync_file to a fence to remove the complexity
> >>of dealing with sync_files from DRM drivers. So even if there are many
> >>fences in the sync_file that needs to waited for a commit to happen
> >>drivers would only worry about a standard struct fence.That means that no
> >>changes needed to any driver besides supporting fences.
> >>
> >>fence_collection's fence doesn't belong to any timeline context.
> >>
> >>Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> >>---
> >>  drivers/dma-buf/Makefile           |   2 +-
> >>  drivers/dma-buf/fence-collection.c | 138 +++++++++++++++++++++++++++++++++++++
> >>  drivers/dma-buf/fence.c            |   2 +-
> >>  include/linux/fence-collection.h   |  56 +++++++++++++++
> >>  include/linux/fence.h              |   2 +
> >>  5 files changed, 198 insertions(+), 2 deletions(-)
> >>  create mode 100644 drivers/dma-buf/fence-collection.c
> >>  create mode 100644 include/linux/fence-collection.h
> >>
> >>diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile
> >>index 43325a1..30b8464 100644
> >>--- a/drivers/dma-buf/Makefile
> >>+++ b/drivers/dma-buf/Makefile
> >>@@ -1,3 +1,3 @@
> >>-obj-y := dma-buf.o fence.o reservation.o seqno-fence.o
> >>+obj-y := dma-buf.o fence.o reservation.o seqno-fence.o fence-collection.o
> >>  obj-$(CONFIG_SYNC_FILE)		+= sync_file.o sync_timeline.o sync_debug.o
> >>  obj-$(CONFIG_SW_SYNC)		+= sw_sync.o
> >>diff --git a/drivers/dma-buf/fence-collection.c b/drivers/dma-buf/fence-collection.c
> >>new file mode 100644
> >>index 0000000..8a4ecb0
> >>--- /dev/null
> >>+++ b/drivers/dma-buf/fence-collection.c
> >>@@ -0,0 +1,138 @@
> >>+/*
> >>+ * fence-collection: aggregate fences to be waited together
> >>+ *
> >>+ * Copyright (C) 2016 Collabora Ltd
> >>+ * Authors:
> >>+ *	Gustavo Padovan <gustavo@padovan.org>
> >>+ *
> >>+ * This program is free software; you can redistribute it and/or modify it
> >>+ * under the terms of the GNU General Public License version 2 as published by
> >>+ * the Free Software Foundation.
> >>+ *
> >>+ * This program is distributed in the hope that it will be useful, but WITHOUT
> >>+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> >>+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> >>+ * more details.
> >>+ */
> >>+
> >>+#include <linux/export.h>
> >>+#include <linux/slab.h>
> >>+#include <linux/fence-collection.h>
> >>+
> >>+static const char *fence_collection_get_driver_name(struct fence *fence)
> >>+{
> >>+	struct fence_collection *collection = to_fence_collection(fence);
> >>+	struct fence *f = collection->fences[0].fence;
> >>+
> >>+	return f->ops->get_driver_name(fence);
> >>+}
> 
> I would rather return some constant name here instead of relying that the
> collection already has a fence added and that all fences are from the same
> driver.

If we merge _init and _add this will not be a problem anymore and we can
return the actual driver name.

> 
> >>+
> >>+static const char *fence_collection_get_timeline_name(struct fence *fence)
> >>+{
> >>+	return "no context";
> >>+}
> >>+
> >>+static bool fence_collection_enable_signaling(struct fence *fence)
> >>+{
> >>+	struct fence_collection *collection = to_fence_collection(fence);
> >>+
> >>+	return atomic_read(&collection->num_pending_fences);
> >>+}
> >>+
> >>+static bool fence_collection_signaled(struct fence *fence)
> >>+{
> >>+	struct fence_collection *collection = to_fence_collection(fence);
> >>+
> >>+	return (atomic_read(&collection->num_pending_fences) == 0);
> >>+}
> >>+
> >>+static void fence_collection_release(struct fence *fence)
> >>+{
> >>+	struct fence_collection *collection = to_fence_collection(fence);
> >>+	int i;
> >>+
> >>+	for (i = 0 ; i < collection->num_fences ; i++) {
> >>+		fence_remove_callback(collection->fences[i].fence,
> >>+				      &collection->fences[i].cb);
> >>+		fence_put(collection->fences[i].fence);
> >>+	}
> >>+
> >>+	fence_free(fence);
> >>+}
> >>+
> >>+static signed long fence_collection_wait(struct fence *fence, bool intr,
> >>+					 signed long timeout)
> >>+{
> >>+	struct fence_collection *collection = to_fence_collection(fence);
> >>+	int i;
> >>+
> >>+	for (i = 0 ; i < collection->num_fences ; i++) {
> >>+		timeout = fence_wait(collection->fences[i].fence, intr);
> >>+		if (timeout < 0)
> >>+			return timeout;
> >>+	}
> >>+
> >>+	return timeout;
> >>+}
> >>+
> >>+static const struct fence_ops fence_collection_ops = {
> >>+	.get_driver_name = fence_collection_get_driver_name,
> >>+	.get_timeline_name = fence_collection_get_timeline_name,
> >>+	.enable_signaling = fence_collection_enable_signaling,
> >>+	.signaled = fence_collection_signaled,
> >>+	.wait = fence_collection_wait,
> >>+	.release = fence_collection_release,
> >>+};
> >>+
> >>+static void collection_check_cb_func(struct fence *fence, struct fence_cb *cb)
> >>+{
> >>+	struct fence_collection_cb *f_cb;
> >>+	struct fence_collection *collection;
> >>+
> >>+	f_cb = container_of(cb, struct fence_collection_cb, cb);
> >>+	collection = f_cb->collection;
> >>+
> >>+	if (atomic_dec_and_test(&collection->num_pending_fences))
> >>+		fence_signal(&collection->base);
> >>+}
> >>+
> >>+void fence_collection_add(struct fence_collection *collection,
> >>+			  struct fence *fence)
> >>+{
> >>+	int n = collection->num_fences;
> >>+
> >>+	collection->fences[n].collection = collection;
> >>+	collection->fences[n].fence = fence;
> >>+
> >>+	if (fence_add_callback(fence, &collection->fences[n].cb,
> >>+				 collection_check_cb_func))
> >>+		return;
> >>+
> >>+	fence_get(fence);
> >>+
> >>+	collection->num_fences++;
> >>+	atomic_inc(&collection->num_pending_fences);
> >>+}
> >For the interface I think we should not split it into _init and _add - it
> >shouldn't be allowed to change a collection once it's created. So probably
> >cleaner if we add an array of fence pointers to _init.
> 
> Amdgpu also has an implementation for a fence collection which uses a a
> hashtable to keep the fences grouped by context (e.g. only the latest fence
> is keept for each context). See amdgpu_sync.c for reference.
> 
> We should either make the collection similar in a way that you can add as
> many fences as you want (like the amdgpu implementation) or make it static
> and only add a fixed number of fences right from the beginning.
> 
> I can certainly see use cases for both, but if you want to stick with a
> static approach you should probably call the new object fence_array instead
> of fence_collection and do as Daniel suggested.

Maybe we can go for something in between. Have fence_collection_init()
need at least two fences to create the fence_collection. Then
fence_collection_add() would add more dinamically.

> 
> >Other nitpick: Adding the callback should (I think) only be done in
> >->enable_signalling.
> 
> Yeah, I was about to complain on that as well.
> 
> Enabling signaling can have a huge overhead for some fence implementations.
> So it should only be used when needed

Right, that makes sense.

> 
> >
> >Finally: Have you looked into stitching together a few unit tests for
> >fence_collection?

Not yet. It is something I plan to work soon.

> >
> >Fence collections also break the assumption that every fence is on a
> >timeline. fence_later and fence_is_later need to be adjusted. We also need
> >a special collection context to filter these out. This means
> >fence_collection isn't perfectly opaque abstraction.

I'll fix this.

> >>+
> >>+struct fence_collection *fence_collection_init(int num_fences)
> >>+{
> >>+	struct fence_collection *collection;
> >>+
> >>+	collection = kzalloc(offsetof(struct fence_collection,
> >>+				      fences[num_fences]), GFP_KERNEL);
> >>+	if (!collection)
> >>+		return NULL;
> >>+
> >>+	spin_lock_init(&collection->lock);
> >>+	fence_init(&collection->base, &fence_collection_ops, &collection->lock,
> >>+		   FENCE_NO_CONTEXT, 0);
> >>+
> >>+	return collection;
> >>+}
> >>+EXPORT_SYMBOL(fence_collection_init);
> >>+
> >>+void fence_collection_put(struct fence_collection *collection)
> >>+{
> >>+	fence_put(&collection->base);
> >Not sure a specialized _put function is useful, I'd leave it out.

I've added this to let the user only deal with fence_collection, but 
I'm fine leaving it out too.

	Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 1/8] dma-buf/fence: add fence_collection fences

[Gustavo Padovan]

2016-04-15 Daniel Vetter <daniel@ffwll.ch>:

> On Fri, Apr 15, 2016 at 11:03 AM, Christian König
> <christian.koenig@amd.com> wrote:
> > Might be that how amdgpu uses the fence context and sequence number is a bit
> > questionable, but this will completely break it.
> 
> You mean it tries to qualesce fences in the same context down to just
> the last one? That's how it's supposed to be done, and
> fence_collections do break this somewhat. Without fixing up
> fence_is_later and friends. Sounds like amdgpu is a good use case to
> make sure the changes in semantics in these functions result in
> sensible code. In a way a fence_collection is a fence where the
> timeline never matches with any other timeline (since it's a
> combiation).
> 
> And yeah I think fence_collection should probably compress down the
> fences to 1 per timeline. But then that's just an implementation
> detail we can fix later on.

You mean asking for a new context for every collection?

	Gustavo

Re: [RFC 1/8] dma-buf/fence: add fence_collection fences

[Gustavo Padovan]

2016-04-15 Daniel Vetter <daniel@ffwll.ch>:

> On Fri, Apr 15, 2016 at 11:03 AM, Christian König
> <christian.koenig@amd.com> wrote:
> > Might be that how amdgpu uses the fence context and sequence number is a bit
> > questionable, but this will completely break it.
> 
> You mean it tries to qualesce fences in the same context down to just
> the last one? That's how it's supposed to be done, and
> fence_collections do break this somewhat. Without fixing up
> fence_is_later and friends. Sounds like amdgpu is a good use case to
> make sure the changes in semantics in these functions result in
> sensible code. In a way a fence_collection is a fence where the
> timeline never matches with any other timeline (since it's a
> combiation).
> 
> And yeah I think fence_collection should probably compress down the
> fences to 1 per timeline. But then that's just an implementation
> detail we can fix later on.

You mean asking for a new context for every collection?

	Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 4/8] drm/fence: add in-fences support

[Gustavo Padovan]

2016-04-15 Daniel Vetter <daniel@ffwll.ch>:

> On Thu, Apr 14, 2016 at 06:29:37PM -0700, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > There is now a new property called FENCE_FD attached to every plane
> > state that receives the sync_file fd from userspace via the atomic commit
> > IOCTL.
> > 
> > The fd is then translated to a fence (that may be a fence_collection
> > subclass or just a normal fence) and then used by DRM to fence_wait() for
> > all fences in the sync_file to signal. So it only commits when all
> > framebuffers are ready to scanout.
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > ---
> >  drivers/gpu/drm/Kconfig             | 1 +
> >  drivers/gpu/drm/drm_atomic.c        | 8 ++++++++
> >  drivers/gpu/drm/drm_atomic_helper.c | 5 +++++
> >  drivers/gpu/drm/drm_crtc.c          | 7 +++++++
> >  include/drm/drm_crtc.h              | 1 +
> >  5 files changed, 22 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index f2a74d0..3c987e3 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -12,6 +12,7 @@ menuconfig DRM
> >  	select I2C
> >  	select I2C_ALGOBIT
> >  	select DMA_SHARED_BUFFER
> > +	select SYNC_FILE
> >  	help
> >  	  Kernel-level support for the Direct Rendering Infrastructure (DRI)
> >  	  introduced in XFree86 4.0. If you say Y here, you need to select
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 8ee1db8..6702502 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -30,6 +30,7 @@
> >  #include <drm/drm_atomic.h>
> >  #include <drm/drm_mode.h>
> >  #include <drm/drm_plane_helper.h>
> > +#include <linux/sync_file.h>
> >  
> >  /**
> >   * drm_atomic_state_default_release -
> > @@ -680,6 +681,11 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
> >  		drm_atomic_set_fb_for_plane(state, fb);
> >  		if (fb)
> >  			drm_framebuffer_unreference(fb);
> > +	} else if (property == config->prop_fence_fd) {
> > +		state->fence = sync_file_fences_get(val);
> > +		if (!state->fence)
> > +			return -EINVAL;
> > +		fence_get(state->fence);
> 
> Yeah, this fence_get must be part of sync_file_fences_get, this code here
> has a race (exercise for the reader to describe where things go wrong
> here). Also, you need to explicitly filter out -1 here first I think.

Right, I missed this race. Move it sync_file_fences_get() just fixes it.
I'll add the filter for -1 too and a testcase.

> 
> Needs an atomic testcase to make sure setting FENCE_FD to -1 doesn't fall
> over (since generic userspace might do this in an attempt to restore all
> the state).
> 
> >  	} else if (property == config->prop_crtc_id) {
> >  		struct drm_crtc *crtc = drm_crtc_find(dev, val);
> >  		return drm_atomic_set_crtc_for_plane(state, crtc);
> > @@ -737,6 +743,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> >  
> >  	if (property == config->prop_fb_id) {
> >  		*val = (state->fb) ? state->fb->base.id : 0;
> > +	} else if (property == config->prop_fence_fd) {
> > +		*val = -1;
> >  	} else if (property == config->prop_crtc_id) {
> >  		*val = (state->crtc) ? state->crtc->base.id : 0;
> >  	} else if (property == config->prop_crtc_x) {
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index f85ef8c..6ed8339 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -2687,6 +2687,11 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane *plane,
> >  {
> >  	if (state->fb)
> >  		drm_framebuffer_unreference(state->fb);
> > +
> > +	if (state->fence) {
> > +		fence_put(state->fence);
> > +		state->fence = NULL;
> 
> No need to set to NULL, we don't do that for ->fb either.

Ok.

	Gustavo

Re: [RFC 4/8] drm/fence: add in-fences support

[Gustavo Padovan]

2016-04-15 Daniel Vetter <daniel@ffwll.ch>:

> On Thu, Apr 14, 2016 at 06:29:37PM -0700, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > There is now a new property called FENCE_FD attached to every plane
> > state that receives the sync_file fd from userspace via the atomic commit
> > IOCTL.
> > 
> > The fd is then translated to a fence (that may be a fence_collection
> > subclass or just a normal fence) and then used by DRM to fence_wait() for
> > all fences in the sync_file to signal. So it only commits when all
> > framebuffers are ready to scanout.
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > ---
> >  drivers/gpu/drm/Kconfig             | 1 +
> >  drivers/gpu/drm/drm_atomic.c        | 8 ++++++++
> >  drivers/gpu/drm/drm_atomic_helper.c | 5 +++++
> >  drivers/gpu/drm/drm_crtc.c          | 7 +++++++
> >  include/drm/drm_crtc.h              | 1 +
> >  5 files changed, 22 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index f2a74d0..3c987e3 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -12,6 +12,7 @@ menuconfig DRM
> >  	select I2C
> >  	select I2C_ALGOBIT
> >  	select DMA_SHARED_BUFFER
> > +	select SYNC_FILE
> >  	help
> >  	  Kernel-level support for the Direct Rendering Infrastructure (DRI)
> >  	  introduced in XFree86 4.0. If you say Y here, you need to select
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 8ee1db8..6702502 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -30,6 +30,7 @@
> >  #include <drm/drm_atomic.h>
> >  #include <drm/drm_mode.h>
> >  #include <drm/drm_plane_helper.h>
> > +#include <linux/sync_file.h>
> >  
> >  /**
> >   * drm_atomic_state_default_release -
> > @@ -680,6 +681,11 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
> >  		drm_atomic_set_fb_for_plane(state, fb);
> >  		if (fb)
> >  			drm_framebuffer_unreference(fb);
> > +	} else if (property == config->prop_fence_fd) {
> > +		state->fence = sync_file_fences_get(val);
> > +		if (!state->fence)
> > +			return -EINVAL;
> > +		fence_get(state->fence);
> 
> Yeah, this fence_get must be part of sync_file_fences_get, this code here
> has a race (exercise for the reader to describe where things go wrong
> here). Also, you need to explicitly filter out -1 here first I think.

Right, I missed this race. Move it sync_file_fences_get() just fixes it.
I'll add the filter for -1 too and a testcase.

> 
> Needs an atomic testcase to make sure setting FENCE_FD to -1 doesn't fall
> over (since generic userspace might do this in an attempt to restore all
> the state).
> 
> >  	} else if (property == config->prop_crtc_id) {
> >  		struct drm_crtc *crtc = drm_crtc_find(dev, val);
> >  		return drm_atomic_set_crtc_for_plane(state, crtc);
> > @@ -737,6 +743,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> >  
> >  	if (property == config->prop_fb_id) {
> >  		*val = (state->fb) ? state->fb->base.id : 0;
> > +	} else if (property == config->prop_fence_fd) {
> > +		*val = -1;
> >  	} else if (property == config->prop_crtc_id) {
> >  		*val = (state->crtc) ? state->crtc->base.id : 0;
> >  	} else if (property == config->prop_crtc_x) {
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index f85ef8c..6ed8339 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -2687,6 +2687,11 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane *plane,
> >  {
> >  	if (state->fb)
> >  		drm_framebuffer_unreference(state->fb);
> > +
> > +	if (state->fence) {
> > +		fence_put(state->fence);
> > +		state->fence = NULL;
> 
> No need to set to NULL, we don't do that for ->fb either.

Ok.

	Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 5/8] drm/fence: add fence to drm_pending_event

[Gustavo Padovan]

2016-04-15 Daniel Vetter <daniel@ffwll.ch>:

> On Thu, Apr 14, 2016 at 06:29:38PM -0700, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > Now a drm_pending_event can either send a real drm_event or signal a
> > fence, or both. It allow us to signal via fences when the buffer is
> > displayed on the screen. Which in turn means that the previous buffer
> > is not in use anymore and can be freed or sent back to another driver
> > for processing.
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Do you have atomic igt testcase that exercise all the combinations of
> drm_event and in/out-fences and make sure it all keeps working? Tomeu
> already converted that over to be a generic testcase.

Not yet. I'll check for test cases.

> 
> > ---
> >  drivers/gpu/drm/drm_atomic.c | 19 +++++++++++++------
> >  drivers/gpu/drm/drm_fops.c   |  5 +++--
> >  drivers/gpu/drm/drm_irq.c    |  7 +++++++
> >  include/drm/drmP.h           |  1 +
> >  4 files changed, 24 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 6702502..0b95526 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1431,7 +1431,8 @@ EXPORT_SYMBOL(drm_atomic_async_commit);
> >   */
> >  
> >  static struct drm_pending_vblank_event *create_vblank_event(
> > -		struct drm_device *dev, struct drm_file *file_priv, uint64_t user_data)
> > +		struct drm_device *dev, struct drm_file *file_priv,
> > +		struct fence *fence, uint64_t user_data)
> >  {
> >  	struct drm_pending_vblank_event *e = NULL;
> >  	int ret;
> > @@ -1444,12 +1445,17 @@ static struct drm_pending_vblank_event *create_vblank_event(
> >  	e->event.base.length = sizeof(e->event);
> >  	e->event.user_data = user_data;
> >  
> > -	ret = drm_event_reserve_init(dev, file_priv, &e->base, &e->event.base);
> > -	if (ret) {
> > -		kfree(e);
> > -		return NULL;
> > +	if (file_priv) {
> > +		ret = drm_event_reserve_init(dev, file_priv, &e->base,
> > +					     &e->event.base);
> > +		if (ret) {
> > +			kfree(e);
> > +			return NULL;
> > +		}
> >  	}
> >  
> > +	e->base.fence = fence;
> > +
> >  	return e;
> >  }
> >  
> > @@ -1676,7 +1682,8 @@ retry:
> >  		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> >  			struct drm_pending_vblank_event *e;
> >  
> > -			e = create_vblank_event(dev, file_priv, arg->user_data);
> > +			e = create_vblank_event(dev, file_priv, NULL,
> > +						arg->user_data);
> >  			if (!e) {
> >  				ret = -ENOMEM;
> >  				goto out;
> > diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> > index aeef58e..38def49 100644
> > --- a/drivers/gpu/drm/drm_fops.c
> > +++ b/drivers/gpu/drm/drm_fops.c
> > @@ -801,8 +801,9 @@ void drm_send_event_locked(struct drm_device *dev, struct drm_pending_event *e)
> >  {
> >  	assert_spin_locked(&dev->event_lock);
> >  
> > -	if (!e->file_priv) {
> > -		e->destroy(e);
> > +	if (!e->file_priv || !e->event) {
> 
> This would be a bug: e->file_priv != NULL iff e->event != NULL. How did
> this happen?

Not sure now. But I needed this to prevent a crash, I don't have logs of
it anymore, I'll check this again.

> 
> > +		if (e->destroy)
> > +			e->destroy(e);
> >  		return;
> >  	}
> >  
> > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > index 3c1a6f1..0c5d7cb 100644
> > --- a/drivers/gpu/drm/drm_irq.c
> > +++ b/drivers/gpu/drm/drm_irq.c
> > @@ -41,6 +41,7 @@
> >  
> >  #include <linux/vgaarb.h>
> >  #include <linux/export.h>
> > +#include <linux/fence.h>
> >  
> >  /* Access macro for slots in vblank timestamp ringbuffer. */
> >  #define vblanktimestamp(dev, pipe, count) \
> > @@ -1124,6 +1125,12 @@ void drm_send_vblank_event(struct drm_device *dev, unsigned int pipe,
> >  		now = get_drm_timestamp();
> >  	}
> >  	e->pipe = pipe;
> > +
> > +	if (e->base.fence) {
> > +		fence_signal(e->base.fence);
> > +		fence_put(e->base.fence);
> > +	}
> 
> I'd put this into drm_send_event_locked even. In case we send out fences
> for other events too (e.g. exynos uses drm_event for rendering ...).

Okay. Looking into that.

	Gustavo

Re: [RFC 5/8] drm/fence: add fence to drm_pending_event

[Gustavo Padovan]

2016-04-15 Daniel Vetter <daniel@ffwll.ch>:

> On Thu, Apr 14, 2016 at 06:29:38PM -0700, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > Now a drm_pending_event can either send a real drm_event or signal a
> > fence, or both. It allow us to signal via fences when the buffer is
> > displayed on the screen. Which in turn means that the previous buffer
> > is not in use anymore and can be freed or sent back to another driver
> > for processing.
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Do you have atomic igt testcase that exercise all the combinations of
> drm_event and in/out-fences and make sure it all keeps working? Tomeu
> already converted that over to be a generic testcase.

Not yet. I'll check for test cases.

> 
> > ---
> >  drivers/gpu/drm/drm_atomic.c | 19 +++++++++++++------
> >  drivers/gpu/drm/drm_fops.c   |  5 +++--
> >  drivers/gpu/drm/drm_irq.c    |  7 +++++++
> >  include/drm/drmP.h           |  1 +
> >  4 files changed, 24 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 6702502..0b95526 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1431,7 +1431,8 @@ EXPORT_SYMBOL(drm_atomic_async_commit);
> >   */
> >  
> >  static struct drm_pending_vblank_event *create_vblank_event(
> > -		struct drm_device *dev, struct drm_file *file_priv, uint64_t user_data)
> > +		struct drm_device *dev, struct drm_file *file_priv,
> > +		struct fence *fence, uint64_t user_data)
> >  {
> >  	struct drm_pending_vblank_event *e = NULL;
> >  	int ret;
> > @@ -1444,12 +1445,17 @@ static struct drm_pending_vblank_event *create_vblank_event(
> >  	e->event.base.length = sizeof(e->event);
> >  	e->event.user_data = user_data;
> >  
> > -	ret = drm_event_reserve_init(dev, file_priv, &e->base, &e->event.base);
> > -	if (ret) {
> > -		kfree(e);
> > -		return NULL;
> > +	if (file_priv) {
> > +		ret = drm_event_reserve_init(dev, file_priv, &e->base,
> > +					     &e->event.base);
> > +		if (ret) {
> > +			kfree(e);
> > +			return NULL;
> > +		}
> >  	}
> >  
> > +	e->base.fence = fence;
> > +
> >  	return e;
> >  }
> >  
> > @@ -1676,7 +1682,8 @@ retry:
> >  		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> >  			struct drm_pending_vblank_event *e;
> >  
> > -			e = create_vblank_event(dev, file_priv, arg->user_data);
> > +			e = create_vblank_event(dev, file_priv, NULL,
> > +						arg->user_data);
> >  			if (!e) {
> >  				ret = -ENOMEM;
> >  				goto out;
> > diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> > index aeef58e..38def49 100644
> > --- a/drivers/gpu/drm/drm_fops.c
> > +++ b/drivers/gpu/drm/drm_fops.c
> > @@ -801,8 +801,9 @@ void drm_send_event_locked(struct drm_device *dev, struct drm_pending_event *e)
> >  {
> >  	assert_spin_locked(&dev->event_lock);
> >  
> > -	if (!e->file_priv) {
> > -		e->destroy(e);
> > +	if (!e->file_priv || !e->event) {
> 
> This would be a bug: e->file_priv != NULL iff e->event != NULL. How did
> this happen?

Not sure now. But I needed this to prevent a crash, I don't have logs of
it anymore, I'll check this again.

> 
> > +		if (e->destroy)
> > +			e->destroy(e);
> >  		return;
> >  	}
> >  
> > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > index 3c1a6f1..0c5d7cb 100644
> > --- a/drivers/gpu/drm/drm_irq.c
> > +++ b/drivers/gpu/drm/drm_irq.c
> > @@ -41,6 +41,7 @@
> >  
> >  #include <linux/vgaarb.h>
> >  #include <linux/export.h>
> > +#include <linux/fence.h>
> >  
> >  /* Access macro for slots in vblank timestamp ringbuffer. */
> >  #define vblanktimestamp(dev, pipe, count) \
> > @@ -1124,6 +1125,12 @@ void drm_send_vblank_event(struct drm_device *dev, unsigned int pipe,
> >  		now = get_drm_timestamp();
> >  	}
> >  	e->pipe = pipe;
> > +
> > +	if (e->base.fence) {
> > +		fence_signal(e->base.fence);
> > +		fence_put(e->base.fence);
> > +	}
> 
> I'd put this into drm_send_event_locked even. In case we send out fences
> for other events too (e.g. exynos uses drm_event for rendering ...).

Okay. Looking into that.

	Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 6/8] drm/fence: create DRM_MODE_ATOMIC_OUT_FENCE flag

[Gustavo Padovan]

2016-04-14 Rob Clark <robdclark@gmail.com>:

> On Thu, Apr 14, 2016 at 9:29 PM, Gustavo Padovan <gustavo@padovan.org> wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> >
> > This flag tells drm_atomic_ioctl that we want to get a per-crtc out-fence
> > fd back.
> >
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > ---
> >  include/uapi/drm/drm_mode.h | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > index 7a7856e..39905cc 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -582,13 +582,15 @@ struct drm_mode_destroy_dumb {
> >  #define DRM_MODE_ATOMIC_TEST_ONLY 0x0100
> >  #define DRM_MODE_ATOMIC_NONBLOCK  0x0200
> >  #define DRM_MODE_ATOMIC_ALLOW_MODESET 0x0400
> > +#define DRM_MODE_ATOMIC_OUT_FENCE 0x0800
> >
> >  #define DRM_MODE_ATOMIC_FLAGS (\
> >                 DRM_MODE_PAGE_FLIP_EVENT |\
> >                 DRM_MODE_PAGE_FLIP_ASYNC |\
> >                 DRM_MODE_ATOMIC_TEST_ONLY |\
> >                 DRM_MODE_ATOMIC_NONBLOCK |\
> > -               DRM_MODE_ATOMIC_ALLOW_MODESET)
> > +               DRM_MODE_ATOMIC_ALLOW_MODESET |\
> > +               DRM_MODE_ATOMIC_OUT_FENCE)
> 
> just to be pedantic / bisectable, perhaps this should be squashed in
> to patch that actually starts using this flag?  Otherwise there is an
> intermediate state in git where the flag is accepted but ignored..

Sure, I totally agree.

	Gustavo

Re: [RFC 6/8] drm/fence: create DRM_MODE_ATOMIC_OUT_FENCE flag

[Gustavo Padovan]

2016-04-14 Rob Clark <robdclark@gmail.com>:

> On Thu, Apr 14, 2016 at 9:29 PM, Gustavo Padovan <gustavo@padovan.org> wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> >
> > This flag tells drm_atomic_ioctl that we want to get a per-crtc out-fence
> > fd back.
> >
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > ---
> >  include/uapi/drm/drm_mode.h | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > index 7a7856e..39905cc 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -582,13 +582,15 @@ struct drm_mode_destroy_dumb {
> >  #define DRM_MODE_ATOMIC_TEST_ONLY 0x0100
> >  #define DRM_MODE_ATOMIC_NONBLOCK  0x0200
> >  #define DRM_MODE_ATOMIC_ALLOW_MODESET 0x0400
> > +#define DRM_MODE_ATOMIC_OUT_FENCE 0x0800
> >
> >  #define DRM_MODE_ATOMIC_FLAGS (\
> >                 DRM_MODE_PAGE_FLIP_EVENT |\
> >                 DRM_MODE_PAGE_FLIP_ASYNC |\
> >                 DRM_MODE_ATOMIC_TEST_ONLY |\
> >                 DRM_MODE_ATOMIC_NONBLOCK |\
> > -               DRM_MODE_ATOMIC_ALLOW_MODESET)
> > +               DRM_MODE_ATOMIC_ALLOW_MODESET |\
> > +               DRM_MODE_ATOMIC_OUT_FENCE)
> 
> just to be pedantic / bisectable, perhaps this should be squashed in
> to patch that actually starts using this flag?  Otherwise there is an
> intermediate state in git where the flag is accepted but ignored..

Sure, I totally agree.

	Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 8/8] drm/fence: add out-fences support

[Gustavo Padovan]

2016-04-15 Daniel Vetter <daniel@ffwll.ch>:

> On Thu, Apr 14, 2016 at 06:29:41PM -0700, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > Support DRM out-fences creating a sync_file with a fence for each crtc
> > update with the DRM_MODE_ATOMIC_OUT_FENCE flag.
> > 
> > We then send an struct drm_out_fences array with the out-fences fds back in
> > the drm_atomic_ioctl() as an out arg in the out_fences_ptr field.
> > 
> > struct drm_out_fences {
> > 	__u32   crtc_id;
> > 	__u32   fd;
> > };
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > ---
> >  drivers/gpu/drm/drm_atomic.c        | 109 +++++++++++++++++++++++++++++++++++-
> >  drivers/gpu/drm/drm_atomic_helper.c |   1 +
> >  include/drm/drm_crtc.h              |   3 +
> >  include/uapi/drm/drm_mode.h         |   7 +++
> >  4 files changed, 119 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 0b95526..af6e051 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1560,6 +1560,103 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
> >  }
> >  EXPORT_SYMBOL(drm_atomic_clean_old_fb);
> >  
> > +static int drm_atomic_get_out_fences(struct drm_device *dev,
> > +				     struct drm_atomic_state *state,
> > +				     uint32_t __user *out_fences_ptr,
> > +				     uint64_t count_out_fences,
> > +				     uint64_t user_data)
> > +{
> > +	struct drm_crtc *crtc;
> > +	struct drm_crtc_state *crtc_state;
> > +	struct drm_out_fences *out_fences;
> > +	struct sync_file **sync_file;
> > +	int num_fences = 0;
> > +	int i, ret;
> > +
> > +	out_fences = kcalloc(count_out_fences, sizeof(*out_fences),
> > +			     GFP_KERNEL);
> > +	if (!out_fences)
> > +		return -ENOMEM;
> > +
> > +	sync_file = kcalloc(count_out_fences, sizeof(*sync_file),
> > +			     GFP_KERNEL);
> > +	if (!sync_file) {
> > +		kfree(out_fences);
> > +		return -ENOMEM;
> > +	}
> > +
> > +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > +		struct drm_pending_vblank_event *e;
> > +		struct fence *fence;
> > +		char name[32];
> > +		int fd;
> > +
> > +		fence = sync_timeline_create_fence(crtc->timeline,
> > +						   crtc->fence_seqno);
> > +		if (!fence) {
> > +			ret = -ENOMEM;
> > +			goto out;
> > +		}
> > +
> > +		snprintf(name, sizeof(name), "crtc-%d_%lu",
> > +			 drm_crtc_index(crtc), crtc->fence_seqno++);
> > +
> > +		sync_file[i] = sync_file_create(name, fence);
> > +		if(!sync_file[i]) {
> > +			ret = -ENOMEM;
> > +			goto out;
> > +		}
> > +
> > +		fd = get_unused_fd_flags(O_CLOEXEC);
> > +		if (fd < 0) {
> > +			ret = fd;
> > +			goto out;
> > +		}
> > +
> > +		sync_file_install(sync_file[i], fd);
> > +
> > +		if (crtc_state->event) {
> > +			crtc_state->event->base.fence = fence;
> > +		} else {
> > +			e = create_vblank_event(dev, NULL, fence, user_data);
> > +			if (!e) {
> > +				put_unused_fd(fd);
> > +				ret = -ENOMEM;
> > +				goto out;
> > +			}
> > +
> > +			crtc_state->event = e;
> > +		}
> > +		if (num_fences > count_out_fences) {
> > +			put_unused_fd(fd);
> > +			ret = -EINVAL;
> > +			goto out;
> > +		}
> > +
> > +		fence_get(fence);
> > +
> > +		out_fences[num_fences].crtc_id = crtc->base.id;
> > +		out_fences[num_fences].fd = fd;
> > +		num_fences++;
> > +	}
> > +
> > +	if (copy_to_user(out_fences_ptr, out_fences,
> > +			 num_fences * sizeof(*out_fences))) {
> > +		ret = -EFAULT;
> > +		goto out;
> > +	}
> > +
> > +	return 0;
> > +
> > +out:
> > +	for (i = 0 ; i < count_out_fences ; i++) {
> > +		if (sync_file[i])
> > +			sync_file_put(sync_file[i]);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >  int drm_mode_atomic_ioctl(struct drm_device *dev,
> >  			  void *data, struct drm_file *file_priv)
> >  {
> > @@ -1568,6 +1665,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
> >  	uint32_t __user *count_props_ptr = (uint32_t __user *)(unsigned long)(arg->count_props_ptr);
> >  	uint32_t __user *props_ptr = (uint32_t __user *)(unsigned long)(arg->props_ptr);
> >  	uint64_t __user *prop_values_ptr = (uint64_t __user *)(unsigned long)(arg->prop_values_ptr);
> > +	uint32_t __user *out_fences_ptr = (uint32_t __user *)(unsigned long)(arg->out_fences_ptr);
> >  	unsigned int copied_objs, copied_props;
> >  	struct drm_atomic_state *state;
> >  	struct drm_modeset_acquire_ctx ctx;
> > @@ -1601,7 +1699,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
> >  
> >  	/* can't test and expect an event at the same time. */
> >  	if ((arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) &&
> > -			(arg->flags & DRM_MODE_PAGE_FLIP_EVENT))
> > +			(arg->flags & (DRM_MODE_PAGE_FLIP_EVENT
> > +			 | DRM_MODE_ATOMIC_OUT_FENCE)))
> >  		return -EINVAL;
> >  
> >  	drm_modeset_acquire_init(&ctx, 0);
> > @@ -1693,6 +1792,14 @@ retry:
> >  		}
> >  	}
> >  
> > +	if (arg->flags & DRM_MODE_ATOMIC_OUT_FENCE) {
> 
> OUT_FENCE and TEST_ONLY probably don't make sense, and need to be
> rejected. Needs a testcase, too.

I've added the check for this above. But a test case is still missing.

> 
> > +		ret = drm_atomic_get_out_fences(dev, state, out_fences_ptr,
> > +						arg->count_out_fences,
> > +						arg->user_data);
> > +		if (ret < 0)
> > +			goto out;
> > +	}
> > +
> >  	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
> 
> If anything fails below this point we need to clean up the sync_file/fd
> mess. Might be easier to first create sync_file objects only, and only
> install the fd once atomic has succeeded. You probably want to reserve the
> fd slots beforehand though.
> 
> That means a bunch more per-crtc state in drm_atomic_state. We should
> probably take all the per-crtc pointers and throw them into a small
> struct, to avoid allocating individual arrays for everything. So
> 
> struct drm_atomic_state_per_crtc {
> 	struct drm_crtc *crtc;
> 	struct drm_crtc_state *state;
> 	struct sync_file *sync_file;
> 	int fd;
> };

That is good idea. I've left the clean up out for this RFC because I
didn't had any good approach on how to do it.

Thanks for this suggestion and all the other comments in the patches.
They were really helpful to improve this work.

	Gustavo

Re: [RFC 8/8] drm/fence: add out-fences support

[Gustavo Padovan]

2016-04-15 Daniel Vetter <daniel@ffwll.ch>:

> On Thu, Apr 14, 2016 at 06:29:41PM -0700, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > Support DRM out-fences creating a sync_file with a fence for each crtc
> > update with the DRM_MODE_ATOMIC_OUT_FENCE flag.
> > 
> > We then send an struct drm_out_fences array with the out-fences fds back in
> > the drm_atomic_ioctl() as an out arg in the out_fences_ptr field.
> > 
> > struct drm_out_fences {
> > 	__u32   crtc_id;
> > 	__u32   fd;
> > };
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > ---
> >  drivers/gpu/drm/drm_atomic.c        | 109 +++++++++++++++++++++++++++++++++++-
> >  drivers/gpu/drm/drm_atomic_helper.c |   1 +
> >  include/drm/drm_crtc.h              |   3 +
> >  include/uapi/drm/drm_mode.h         |   7 +++
> >  4 files changed, 119 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 0b95526..af6e051 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1560,6 +1560,103 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
> >  }
> >  EXPORT_SYMBOL(drm_atomic_clean_old_fb);
> >  
> > +static int drm_atomic_get_out_fences(struct drm_device *dev,
> > +				     struct drm_atomic_state *state,
> > +				     uint32_t __user *out_fences_ptr,
> > +				     uint64_t count_out_fences,
> > +				     uint64_t user_data)
> > +{
> > +	struct drm_crtc *crtc;
> > +	struct drm_crtc_state *crtc_state;
> > +	struct drm_out_fences *out_fences;
> > +	struct sync_file **sync_file;
> > +	int num_fences = 0;
> > +	int i, ret;
> > +
> > +	out_fences = kcalloc(count_out_fences, sizeof(*out_fences),
> > +			     GFP_KERNEL);
> > +	if (!out_fences)
> > +		return -ENOMEM;
> > +
> > +	sync_file = kcalloc(count_out_fences, sizeof(*sync_file),
> > +			     GFP_KERNEL);
> > +	if (!sync_file) {
> > +		kfree(out_fences);
> > +		return -ENOMEM;
> > +	}
> > +
> > +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > +		struct drm_pending_vblank_event *e;
> > +		struct fence *fence;
> > +		char name[32];
> > +		int fd;
> > +
> > +		fence = sync_timeline_create_fence(crtc->timeline,
> > +						   crtc->fence_seqno);
> > +		if (!fence) {
> > +			ret = -ENOMEM;
> > +			goto out;
> > +		}
> > +
> > +		snprintf(name, sizeof(name), "crtc-%d_%lu",
> > +			 drm_crtc_index(crtc), crtc->fence_seqno++);
> > +
> > +		sync_file[i] = sync_file_create(name, fence);
> > +		if(!sync_file[i]) {
> > +			ret = -ENOMEM;
> > +			goto out;
> > +		}
> > +
> > +		fd = get_unused_fd_flags(O_CLOEXEC);
> > +		if (fd < 0) {
> > +			ret = fd;
> > +			goto out;
> > +		}
> > +
> > +		sync_file_install(sync_file[i], fd);
> > +
> > +		if (crtc_state->event) {
> > +			crtc_state->event->base.fence = fence;
> > +		} else {
> > +			e = create_vblank_event(dev, NULL, fence, user_data);
> > +			if (!e) {
> > +				put_unused_fd(fd);
> > +				ret = -ENOMEM;
> > +				goto out;
> > +			}
> > +
> > +			crtc_state->event = e;
> > +		}
> > +		if (num_fences > count_out_fences) {
> > +			put_unused_fd(fd);
> > +			ret = -EINVAL;
> > +			goto out;
> > +		}
> > +
> > +		fence_get(fence);
> > +
> > +		out_fences[num_fences].crtc_id = crtc->base.id;
> > +		out_fences[num_fences].fd = fd;
> > +		num_fences++;
> > +	}
> > +
> > +	if (copy_to_user(out_fences_ptr, out_fences,
> > +			 num_fences * sizeof(*out_fences))) {
> > +		ret = -EFAULT;
> > +		goto out;
> > +	}
> > +
> > +	return 0;
> > +
> > +out:
> > +	for (i = 0 ; i < count_out_fences ; i++) {
> > +		if (sync_file[i])
> > +			sync_file_put(sync_file[i]);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >  int drm_mode_atomic_ioctl(struct drm_device *dev,
> >  			  void *data, struct drm_file *file_priv)
> >  {
> > @@ -1568,6 +1665,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
> >  	uint32_t __user *count_props_ptr = (uint32_t __user *)(unsigned long)(arg->count_props_ptr);
> >  	uint32_t __user *props_ptr = (uint32_t __user *)(unsigned long)(arg->props_ptr);
> >  	uint64_t __user *prop_values_ptr = (uint64_t __user *)(unsigned long)(arg->prop_values_ptr);
> > +	uint32_t __user *out_fences_ptr = (uint32_t __user *)(unsigned long)(arg->out_fences_ptr);
> >  	unsigned int copied_objs, copied_props;
> >  	struct drm_atomic_state *state;
> >  	struct drm_modeset_acquire_ctx ctx;
> > @@ -1601,7 +1699,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
> >  
> >  	/* can't test and expect an event at the same time. */
> >  	if ((arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) &&
> > -			(arg->flags & DRM_MODE_PAGE_FLIP_EVENT))
> > +			(arg->flags & (DRM_MODE_PAGE_FLIP_EVENT
> > +			 | DRM_MODE_ATOMIC_OUT_FENCE)))
> >  		return -EINVAL;
> >  
> >  	drm_modeset_acquire_init(&ctx, 0);
> > @@ -1693,6 +1792,14 @@ retry:
> >  		}
> >  	}
> >  
> > +	if (arg->flags & DRM_MODE_ATOMIC_OUT_FENCE) {
> 
> OUT_FENCE and TEST_ONLY probably don't make sense, and need to be
> rejected. Needs a testcase, too.

I've added the check for this above. But a test case is still missing.

> 
> > +		ret = drm_atomic_get_out_fences(dev, state, out_fences_ptr,
> > +						arg->count_out_fences,
> > +						arg->user_data);
> > +		if (ret < 0)
> > +			goto out;
> > +	}
> > +
> >  	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
> 
> If anything fails below this point we need to clean up the sync_file/fd
> mess. Might be easier to first create sync_file objects only, and only
> install the fd once atomic has succeeded. You probably want to reserve the
> fd slots beforehand though.
> 
> That means a bunch more per-crtc state in drm_atomic_state. We should
> probably take all the per-crtc pointers and throw them into a small
> struct, to avoid allocating individual arrays for everything. So
> 
> struct drm_atomic_state_per_crtc {
> 	struct drm_crtc *crtc;
> 	struct drm_crtc_state *state;
> 	struct sync_file *sync_file;
> 	int fd;
> };

That is good idea. I've left the clean up out for this RFC because I
didn't had any good approach on how to do it.

Thanks for this suggestion and all the other comments in the patches.
They were really helpful to improve this work.

	Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 1/8] dma-buf/fence: add fence_collection fences

[Daniel Vetter]

On Fri, Apr 15, 2016 at 11:29:34AM -0700, Gustavo Padovan wrote:
> 2016-04-15 Daniel Vetter <daniel@ffwll.ch>:
> 
> > On Fri, Apr 15, 2016 at 11:03 AM, Christian König
> > <christian.koenig@amd.com> wrote:
> > > Might be that how amdgpu uses the fence context and sequence number is a bit
> > > questionable, but this will completely break it.
> > 
> > You mean it tries to qualesce fences in the same context down to just
> > the last one? That's how it's supposed to be done, and
> > fence_collections do break this somewhat. Without fixing up
> > fence_is_later and friends. Sounds like amdgpu is a good use case to
> > make sure the changes in semantics in these functions result in
> > sensible code. In a way a fence_collection is a fence where the
> > timeline never matches with any other timeline (since it's a
> > combiation).
> > 
> > And yeah I think fence_collection should probably compress down the
> > fences to 1 per timeline. But then that's just an implementation
> > detail we can fix later on.
> 
> You mean asking for a new context for every collection?

That would be one solution, but I fear it's a bit expensive. Having a
special-case context for collections might be the better approach.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [RFC 1/8] dma-buf/fence: add fence_collection fences

[Daniel Vetter]

On Fri, Apr 15, 2016 at 11:29:34AM -0700, Gustavo Padovan wrote:
> 2016-04-15 Daniel Vetter <daniel@ffwll.ch>:
> 
> > On Fri, Apr 15, 2016 at 11:03 AM, Christian König
> > <christian.koenig@amd.com> wrote:
> > > Might be that how amdgpu uses the fence context and sequence number is a bit
> > > questionable, but this will completely break it.
> > 
> > You mean it tries to qualesce fences in the same context down to just
> > the last one? That's how it's supposed to be done, and
> > fence_collections do break this somewhat. Without fixing up
> > fence_is_later and friends. Sounds like amdgpu is a good use case to
> > make sure the changes in semantics in these functions result in
> > sensible code. In a way a fence_collection is a fence where the
> > timeline never matches with any other timeline (since it's a
> > combiation).
> > 
> > And yeah I think fence_collection should probably compress down the
> > fences to 1 per timeline. But then that's just an implementation
> > detail we can fix later on.
> 
> You mean asking for a new context for every collection?

That would be one solution, but I fear it's a bit expensive. Having a
special-case context for collections might be the better approach.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 1/8] dma-buf/fence: add fence_collection fences

[Daniel Vetter]

On Fri, Apr 15, 2016 at 11:27:50AM -0700, Gustavo Padovan wrote:
> 2016-04-15 Christian König <christian.koenig@amd.com>:
> > Amdgpu also has an implementation for a fence collection which uses a a
> > hashtable to keep the fences grouped by context (e.g. only the latest fence
> > is keept for each context). See amdgpu_sync.c for reference.
> > 
> > We should either make the collection similar in a way that you can add as
> > many fences as you want (like the amdgpu implementation) or make it static
> > and only add a fixed number of fences right from the beginning.
> > 
> > I can certainly see use cases for both, but if you want to stick with a
> > static approach you should probably call the new object fence_array instead
> > of fence_collection and do as Daniel suggested.
> 
> Maybe we can go for something in between. Have fence_collection_init()
> need at least two fences to create the fence_collection. Then
> fence_collection_add() would add more dinamically.

The problem with adding fences later on is that it makes it trivial to add
deadlocks and loops. Just add the fence collection to itself, boom. From
that pov it's an unsafe api, and hence something to avoid.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [RFC 1/8] dma-buf/fence: add fence_collection fences

[Daniel Vetter]

On Fri, Apr 15, 2016 at 11:27:50AM -0700, Gustavo Padovan wrote:
> 2016-04-15 Christian König <christian.koenig@amd.com>:
> > Amdgpu also has an implementation for a fence collection which uses a a
> > hashtable to keep the fences grouped by context (e.g. only the latest fence
> > is keept for each context). See amdgpu_sync.c for reference.
> > 
> > We should either make the collection similar in a way that you can add as
> > many fences as you want (like the amdgpu implementation) or make it static
> > and only add a fixed number of fences right from the beginning.
> > 
> > I can certainly see use cases for both, but if you want to stick with a
> > static approach you should probably call the new object fence_array instead
> > of fence_collection and do as Daniel suggested.
> 
> Maybe we can go for something in between. Have fence_collection_init()
> need at least two fences to create the fence_collection. Then
> fence_collection_add() would add more dinamically.

The problem with adding fences later on is that it makes it trivial to add
deadlocks and loops. Just add the fence collection to itself, boom. From
that pov it's an unsafe api, and hence something to avoid.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 5/8] drm/fence: add fence to drm_pending_event

[Daniel Vetter]

On Fri, Apr 15, 2016 at 11:59:00AM -0700, Gustavo Padovan wrote:
> 2016-04-15 Daniel Vetter <daniel@ffwll.ch>:
> > On Thu, Apr 14, 2016 at 06:29:38PM -0700, Gustavo Padovan wrote:
> > > diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> > > index aeef58e..38def49 100644
> > > --- a/drivers/gpu/drm/drm_fops.c
> > > +++ b/drivers/gpu/drm/drm_fops.c
> > > @@ -801,8 +801,9 @@ void drm_send_event_locked(struct drm_device *dev, struct drm_pending_event *e)
> > >  {
> > >  	assert_spin_locked(&dev->event_lock);
> > >  
> > > -	if (!e->file_priv) {
> > > -		e->destroy(e);
> > > +	if (!e->file_priv || !e->event) {
> > 
> > This would be a bug: e->file_priv != NULL iff e->event != NULL. How did
> > this happen?
> 
> Not sure now. But I needed this to prevent a crash, I don't have logs of
> it anymore, I'll check this again.

There was a massive irc discussion with Daniel Stone, so I'll try to
summarize it here. There are 3 possible cases:

e->file_priv == NULL && e->event == NULL:
This is a drm_event without a drm_event. Probably e->fence is set, if not
then it's a completeley useless thing (but not forbidden).

e->file_priv != NULL && e->event != NULL:
drm_event with an event for the given file_priv attached.

e->file_priv == NULL && e->event != NULL:
Above case, but with the file_priv closed and unlinked from the event.

The 4th case, which is the only case things will change with the above
hunk, is not allowed. If you hit it, there's a bug somewhere.

This is all completely idependent of e->fence, which this patch adds.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [RFC 5/8] drm/fence: add fence to drm_pending_event

[Daniel Vetter]

On Fri, Apr 15, 2016 at 11:59:00AM -0700, Gustavo Padovan wrote:
> 2016-04-15 Daniel Vetter <daniel@ffwll.ch>:
> > On Thu, Apr 14, 2016 at 06:29:38PM -0700, Gustavo Padovan wrote:
> > > diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> > > index aeef58e..38def49 100644
> > > --- a/drivers/gpu/drm/drm_fops.c
> > > +++ b/drivers/gpu/drm/drm_fops.c
> > > @@ -801,8 +801,9 @@ void drm_send_event_locked(struct drm_device *dev, struct drm_pending_event *e)
> > >  {
> > >  	assert_spin_locked(&dev->event_lock);
> > >  
> > > -	if (!e->file_priv) {
> > > -		e->destroy(e);
> > > +	if (!e->file_priv || !e->event) {
> > 
> > This would be a bug: e->file_priv != NULL iff e->event != NULL. How did
> > this happen?
> 
> Not sure now. But I needed this to prevent a crash, I don't have logs of
> it anymore, I'll check this again.

There was a massive irc discussion with Daniel Stone, so I'll try to
summarize it here. There are 3 possible cases:

e->file_priv == NULL && e->event == NULL:
This is a drm_event without a drm_event. Probably e->fence is set, if not
then it's a completeley useless thing (but not forbidden).

e->file_priv != NULL && e->event != NULL:
drm_event with an event for the given file_priv attached.

e->file_priv == NULL && e->event != NULL:
Above case, but with the file_priv closed and unlinked from the event.

The 4th case, which is the only case things will change with the above
hunk, is not allowed. If you hit it, there's a bug somewhere.

This is all completely idependent of e->fence, which this patch adds.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 1/8] dma-buf/fence: add fence_collection fences

[Christian König]

Am 15.04.2016 um 21:25 schrieb Daniel Vetter:
> On Fri, Apr 15, 2016 at 11:27:50AM -0700, Gustavo Padovan wrote:
>> 2016-04-15 Christian König <christian.koenig@amd.com>:
>>> Amdgpu also has an implementation for a fence collection which uses a a
>>> hashtable to keep the fences grouped by context (e.g. only the latest fence
>>> is keept for each context). See amdgpu_sync.c for reference.
>>>
>>> We should either make the collection similar in a way that you can add as
>>> many fences as you want (like the amdgpu implementation) or make it static
>>> and only add a fixed number of fences right from the beginning.
>>>
>>> I can certainly see use cases for both, but if you want to stick with a
>>> static approach you should probably call the new object fence_array instead
>>> of fence_collection and do as Daniel suggested.
>> Maybe we can go for something in between. Have fence_collection_init()
>> need at least two fences to create the fence_collection. Then
>> fence_collection_add() would add more dinamically.
> The problem with adding fences later on is that it makes it trivial to add
> deadlocks and loops. Just add the fence collection to itself, boom. From
> that pov it's an unsafe api, and hence something to avoid.
> -Daniel

Any conclusion on this? Did any version of the patch made it upstream?

I'm in the need of an array based fence collection right now as well. 
Any objection that I just take the patch proposed here and fix the 
comments or are you still else working on this right now?

Regards,
Christian.

Re: [RFC 1/8] dma-buf/fence: add fence_collection fences

[Christian König]

Am 15.04.2016 um 21:25 schrieb Daniel Vetter:
> On Fri, Apr 15, 2016 at 11:27:50AM -0700, Gustavo Padovan wrote:
>> 2016-04-15 Christian König <christian.koenig@amd.com>:
>>> Amdgpu also has an implementation for a fence collection which uses a a
>>> hashtable to keep the fences grouped by context (e.g. only the latest fence
>>> is keept for each context). See amdgpu_sync.c for reference.
>>>
>>> We should either make the collection similar in a way that you can add as
>>> many fences as you want (like the amdgpu implementation) or make it static
>>> and only add a fixed number of fences right from the beginning.
>>>
>>> I can certainly see use cases for both, but if you want to stick with a
>>> static approach you should probably call the new object fence_array instead
>>> of fence_collection and do as Daniel suggested.
>> Maybe we can go for something in between. Have fence_collection_init()
>> need at least two fences to create the fence_collection. Then
>> fence_collection_add() would add more dinamically.
> The problem with adding fences later on is that it makes it trivial to add
> deadlocks and loops. Just add the fence collection to itself, boom. From
> that pov it's an unsafe api, and hence something to avoid.
> -Daniel

Any conclusion on this? Did any version of the patch made it upstream?

I'm in the need of an array based fence collection right now as well. 
Any objection that I just take the patch proposed here and fix the 
comments or are you still else working on this right now?

Regards,
Christian.


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 1/8] dma-buf/fence: add fence_collection fences

[Gustavo Padovan]

Hi Christian,

2016-05-18 Christian König <christian.koenig@amd.com>:

> Am 15.04.2016 um 21:25 schrieb Daniel Vetter:
> > On Fri, Apr 15, 2016 at 11:27:50AM -0700, Gustavo Padovan wrote:
> > > 2016-04-15 Christian König <christian.koenig@amd.com>:
> > > > Amdgpu also has an implementation for a fence collection which uses a a
> > > > hashtable to keep the fences grouped by context (e.g. only the latest fence
> > > > is keept for each context). See amdgpu_sync.c for reference.
> > > > 
> > > > We should either make the collection similar in a way that you can add as
> > > > many fences as you want (like the amdgpu implementation) or make it static
> > > > and only add a fixed number of fences right from the beginning.
> > > > 
> > > > I can certainly see use cases for both, but if you want to stick with a
> > > > static approach you should probably call the new object fence_array instead
> > > > of fence_collection and do as Daniel suggested.
> > > Maybe we can go for something in between. Have fence_collection_init()
> > > need at least two fences to create the fence_collection. Then
> > > fence_collection_add() would add more dinamically.
> > The problem with adding fences later on is that it makes it trivial to add
> > deadlocks and loops. Just add the fence collection to itself, boom. From
> > that pov it's an unsafe api, and hence something to avoid.
> > -Daniel
> 
> Any conclusion on this? Did any version of the patch made it upstream?
> 
> I'm in the need of an array based fence collection right now as well. Any
> objection that I just take the patch proposed here and fix the comments or
> are you still else working on this right now?

I have a new version of this patch that I didn't send upstream yet
because it is part of a bigger patchset. But I can split it and send
what I have for fence_collection later today.

	Gustavo