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

[Gustavo Padovan]

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

> 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@xxxxxxxxxxxxxxx>
> >>
> >>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@xxxxxxxxxxxxxxx>
> >>---
> >>  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@xxxxxxxxxxx>
> >>+ *
> >>+ * 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