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

Groups > linux.kernel > #1386972 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC v2 1/8] dma-buf/fence: add fence_collection fences Gustavo Padovan <gustavo@padovan.org> - 2016-04-26 00:40 +0200
    Re: [RFC v2 1/8] dma-buf/fence: add fence_collection fences Daniel Vetter <daniel@ffwll.ch> - 2016-04-26 16:50 +0200
      Re: [RFC v2 1/8] dma-buf/fence: add fence_collection fences Gustavo Padovan <gustavo.padovan@collabora.co.uk> - 2016-04-26 17:10 +0200
        Re: [RFC v2 1/8] dma-buf/fence: add fence_collection fences Daniel Vetter <daniel@ffwll.ch> - 2016-04-27 08:40 +0200
    Re: [RFC v2 1/8] dma-buf/fence: add fence_collection fences Chris Wilson <chris@chris-wilson.co.uk> - 2016-04-26 17:10 +0200

#1386972 — [RFC v2 1/8] dma-buf/fence: add fence_collection fences

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 on DRM drivers. So even if there are many
fences in the sync_file that needs to waited for a commit to happen,
they all get added to the fence_collection and passed for DRM use as
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, so
fence_is_later() and fence_later() are not meant to be called with
fence_collections fences.

v2: Comments by Daniel Vetter:
	- merge fence_collection_init() and fence_collection_add()
	- only add callbacks at ->enable_signalling()
	- remove fence_collection_put()
	- check for type on to_fence_collection()
	- adjust fence_is_later() and fence_later() to WARN_ON() if they
	are used with collection fences.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/dma-buf/Makefile           |   2 +-
 drivers/dma-buf/fence-collection.c | 159 +++++++++++++++++++++++++++++++++++++
 drivers/dma-buf/fence.c            |   2 +-
 include/linux/fence-collection.h   |  73 +++++++++++++++++
 include/linux/fence.h              |   9 +++
 5 files changed, 243 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 4a424ec..52f818f 100644
--- a/drivers/dma-buf/Makefile
+++ b/drivers/dma-buf/Makefile
@@ -1,2 +1,2 @@
-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
diff --git a/drivers/dma-buf/fence-collection.c b/drivers/dma-buf/fence-collection.c
new file mode 100644
index 0000000..88872e5
--- /dev/null
+++ b/drivers/dma-buf/fence-collection.c
@@ -0,0 +1,159 @@
+/*
+ * 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 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);
+}
+
+static bool fence_collection_enable_signaling(struct fence *fence)
+{
+	struct fence_collection *collection = to_fence_collection(fence);
+	int i;
+
+	for (i = 0 ; i < collection->num_fences ; i++) {
+		if (fence_add_callback(collection->fences[i].fence,
+				       &collection->fences[i].cb,
+				       collection_check_cb_func)) {
+			atomic_dec(&collection->num_pending_fences);
+			return false;
+		}
+	}
+
+	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_put(collection->fences[i].fence);
+
+	kfree(collection->fences);
+	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;
+
+	if (test_bit(FENCE_FLAG_SIGNALED_BIT, &fence->flags))
+		return timeout;
+
+	fence_enable_sw_signaling(fence);
+
+	for (i = 0 ; i < collection->num_fences ; i++) {
+		if (test_bit(FENCE_FLAG_SIGNALED_BIT, &fence->flags))
+			return timeout;
+
+		timeout = fence_wait(collection->fences[i].fence, intr);
+		if (timeout < 0)
+			return timeout;
+	}
+
+	return timeout;
+}
+
+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,
+};
+
+/**
+ * fence_collection_create - Create a custom fence collection
+ * is signaled
+ * @num_fences:	[in]	number of fences to add in the collection
+ * @cb:		[in]	cb array containing the fences for the collection
+ *
+ * Allocate a fence_collection and initialize the base fence with fence_init()
+ * and FENCE_NO_CONTEXT. It also inits all fence_collections specific fields
+ * and return the created fence_collection. In case of error it returns NULL.
+ *
+ * The caller should allocte the fence_collection_cb array with num_fences size
+ * and fill the fence fields with all the fences it wants to add to the
+ * collection.
+ *
+ * Collections have no context, thus fence_later() and fence_is_later() shall
+ * not be used with fence_collection.
+ *
+ * fence_put() on the base fence should be used to release the collection.
+ */
+struct fence_collection *fence_collection_create(int num_fences,
+						 struct fence_collection_cb *cb)
+{
+	struct fence_collection *collection;
+	int i;
+
+	collection = kzalloc(sizeof(*collection), GFP_KERNEL);
+	if (!collection)
+		return NULL;
+
+	spin_lock_init(&collection->lock);
+	fence_init(&collection->base, &fence_collection_ops, &collection->lock,
+		   FENCE_NO_CONTEXT, 0);
+
+	collection->fences = cb;
+	collection->num_fences = num_fences;
+	atomic_set(&collection->num_pending_fences, num_fences);
+
+	for (i = 0 ; i < collection->num_fences ; i++) {
+		fence_get(collection->fences[i].fence);
+		collection->fences[i].collection = collection;
+	}
+
+	return collection;
+}
+EXPORT_SYMBOL(fence_collection_create);
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..4087622
--- /dev/null
+++ b/include/linux/fence-collection.h
@@ -0,0 +1,73 @@
+/*
+ * 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 - callback for fence_add_callback
+ * @cb: the base fence_cb object
+ * @fence: fence we are storing in the collection
+ * @collection: collection the fence belongs to
+ *
+ * This struct is at the same time the storage for the fences in the
+ * collection and the struct we pass to fence_add_callback()
+ */
+struct fence_collection_cb {
+	struct fence_cb cb;
+	struct fence *fence;
+	struct fence_collection *collection;
+};
+
+/**
+ * struct fence_collection - fence to represent a collection of fences
+ * @base: fence base class
+ * @lock: spinlock for fence handling
+ * @num_pending_fences: fences in the collection not signaled yet
+ * @num_fences: number of fences in the collection
+ * @fences: ponteir to fence_collection_cb with data about the fences
+ */
+struct fence_collection {
+	struct fence base;
+
+	spinlock_t lock;
+	atomic_t num_pending_fences;
+	int num_fences;
+	struct fence_collection_cb *fences;
+};
+
+extern const struct fence_ops fence_collection_ops;
+
+/**
+ * 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)
+{
+	if (fence->ops != &fence_collection_ops)
+		return NULL;
+	return container_of(fence, struct fence_collection, base);
+}
+
+struct fence_collection *fence_collection_create(int num_fences,
+						struct fence_collection_cb *cb);
+#endif /* __LINUX_FENCE_COLLECTION_H */
diff --git a/include/linux/fence.h b/include/linux/fence.h
index 5aa95eb..184ce79 100644
--- a/include/linux/fence.h
+++ b/include/linux/fence.h
@@ -30,6 +30,9 @@
 #include <linux/printk.h>
 #include <linux/rcupdate.h>
 
+/* context number for fences without context, e.g: fence_collection */
+#define FENCE_NO_CONTEXT 0
+
 struct fence;
 struct fence_ops;
 struct fence_cb;
@@ -292,6 +295,9 @@ static inline bool fence_is_later(struct fence *f1, struct fence *f2)
 	if (WARN_ON(f1->context != f2->context))
 		return false;
 
+	if (WARN_ON(f1->context == FENCE_NO_CONTEXT))
+		return false;
+
 	return (int)(f1->seqno - f2->seqno) > 0;
 }
 
@@ -309,6 +315,9 @@ static inline struct fence *fence_later(struct fence *f1, struct fence *f2)
 	if (WARN_ON(f1->context != f2->context))
 		return NULL;
 
+	if (WARN_ON(f1->context == FENCE_NO_CONTEXT))
+		return NULL;
+
 	/*
 	 * can't check just FENCE_FLAG_SIGNALED_BIT here, it may never have been
 	 * set if enable_signaling wasn't called, and enabling that here is
-- 
2.5.5

[toc] | [next] | [standalone]

#1387563

On Mon, Apr 25, 2016 at 07:33:21PM -0300, 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 on DRM drivers. So even if there are many
> fences in the sync_file that needs to waited for a commit to happen,
> they all get added to the fence_collection and passed for DRM use as
> 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, so
> fence_is_later() and fence_later() are not meant to be called with
> fence_collections fences.
> 
> v2: Comments by Daniel Vetter:
> 	- merge fence_collection_init() and fence_collection_add()
> 	- only add callbacks at ->enable_signalling()
> 	- remove fence_collection_put()
> 	- check for type on to_fence_collection()
> 	- adjust fence_is_later() and fence_later() to WARN_ON() if they
> 	are used with collection fences.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

FENCE_NO_CONTEXT semantics needs an ack from amdgpu maintainers. I'm not
entirely sure they might not hit the new WARN_ON by accident now. Please
cc Alex Deucher & Christian König.
-Daniel

> ---
>  drivers/dma-buf/Makefile           |   2 +-
>  drivers/dma-buf/fence-collection.c | 159 +++++++++++++++++++++++++++++++++++++
>  drivers/dma-buf/fence.c            |   2 +-
>  include/linux/fence-collection.h   |  73 +++++++++++++++++
>  include/linux/fence.h              |   9 +++
>  5 files changed, 243 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 4a424ec..52f818f 100644
> --- a/drivers/dma-buf/Makefile
> +++ b/drivers/dma-buf/Makefile
> @@ -1,2 +1,2 @@
> -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
> diff --git a/drivers/dma-buf/fence-collection.c b/drivers/dma-buf/fence-collection.c
> new file mode 100644
> index 0000000..88872e5
> --- /dev/null
> +++ b/drivers/dma-buf/fence-collection.c
> @@ -0,0 +1,159 @@
> +/*
> + * 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 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);
> +}
> +
> +static bool fence_collection_enable_signaling(struct fence *fence)
> +{
> +	struct fence_collection *collection = to_fence_collection(fence);
> +	int i;
> +
> +	for (i = 0 ; i < collection->num_fences ; i++) {
> +		if (fence_add_callback(collection->fences[i].fence,
> +				       &collection->fences[i].cb,
> +				       collection_check_cb_func)) {
> +			atomic_dec(&collection->num_pending_fences);
> +			return false;
> +		}
> +	}
> +
> +	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_put(collection->fences[i].fence);
> +
> +	kfree(collection->fences);
> +	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;
> +
> +	if (test_bit(FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> +		return timeout;
> +
> +	fence_enable_sw_signaling(fence);
> +
> +	for (i = 0 ; i < collection->num_fences ; i++) {
> +		if (test_bit(FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> +			return timeout;
> +
> +		timeout = fence_wait(collection->fences[i].fence, intr);
> +		if (timeout < 0)
> +			return timeout;
> +	}
> +
> +	return timeout;
> +}
> +
> +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,
> +};
> +
> +/**
> + * fence_collection_create - Create a custom fence collection
> + * is signaled
> + * @num_fences:	[in]	number of fences to add in the collection
> + * @cb:		[in]	cb array containing the fences for the collection
> + *
> + * Allocate a fence_collection and initialize the base fence with fence_init()
> + * and FENCE_NO_CONTEXT. It also inits all fence_collections specific fields
> + * and return the created fence_collection. In case of error it returns NULL.
> + *
> + * The caller should allocte the fence_collection_cb array with num_fences size
> + * and fill the fence fields with all the fences it wants to add to the
> + * collection.
> + *
> + * Collections have no context, thus fence_later() and fence_is_later() shall
> + * not be used with fence_collection.
> + *
> + * fence_put() on the base fence should be used to release the collection.
> + */
> +struct fence_collection *fence_collection_create(int num_fences,
> +						 struct fence_collection_cb *cb)
> +{
> +	struct fence_collection *collection;
> +	int i;
> +
> +	collection = kzalloc(sizeof(*collection), GFP_KERNEL);
> +	if (!collection)
> +		return NULL;
> +
> +	spin_lock_init(&collection->lock);
> +	fence_init(&collection->base, &fence_collection_ops, &collection->lock,
> +		   FENCE_NO_CONTEXT, 0);
> +
> +	collection->fences = cb;
> +	collection->num_fences = num_fences;
> +	atomic_set(&collection->num_pending_fences, num_fences);
> +
> +	for (i = 0 ; i < collection->num_fences ; i++) {
> +		fence_get(collection->fences[i].fence);
> +		collection->fences[i].collection = collection;
> +	}
> +
> +	return collection;
> +}
> +EXPORT_SYMBOL(fence_collection_create);
> 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..4087622
> --- /dev/null
> +++ b/include/linux/fence-collection.h
> @@ -0,0 +1,73 @@
> +/*
> + * 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 - callback for fence_add_callback
> + * @cb: the base fence_cb object
> + * @fence: fence we are storing in the collection
> + * @collection: collection the fence belongs to
> + *
> + * This struct is at the same time the storage for the fences in the
> + * collection and the struct we pass to fence_add_callback()
> + */
> +struct fence_collection_cb {
> +	struct fence_cb cb;
> +	struct fence *fence;
> +	struct fence_collection *collection;
> +};
> +
> +/**
> + * struct fence_collection - fence to represent a collection of fences
> + * @base: fence base class
> + * @lock: spinlock for fence handling
> + * @num_pending_fences: fences in the collection not signaled yet
> + * @num_fences: number of fences in the collection
> + * @fences: ponteir to fence_collection_cb with data about the fences
> + */
> +struct fence_collection {
> +	struct fence base;
> +
> +	spinlock_t lock;
> +	atomic_t num_pending_fences;
> +	int num_fences;
> +	struct fence_collection_cb *fences;
> +};
> +
> +extern const struct fence_ops fence_collection_ops;
> +
> +/**
> + * 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)
> +{
> +	if (fence->ops != &fence_collection_ops)
> +		return NULL;
> +	return container_of(fence, struct fence_collection, base);
> +}
> +
> +struct fence_collection *fence_collection_create(int num_fences,
> +						struct fence_collection_cb *cb);
> +#endif /* __LINUX_FENCE_COLLECTION_H */
> diff --git a/include/linux/fence.h b/include/linux/fence.h
> index 5aa95eb..184ce79 100644
> --- a/include/linux/fence.h
> +++ b/include/linux/fence.h
> @@ -30,6 +30,9 @@
>  #include <linux/printk.h>
>  #include <linux/rcupdate.h>
>  
> +/* context number for fences without context, e.g: fence_collection */
> +#define FENCE_NO_CONTEXT 0
> +
>  struct fence;
>  struct fence_ops;
>  struct fence_cb;
> @@ -292,6 +295,9 @@ static inline bool fence_is_later(struct fence *f1, struct fence *f2)
>  	if (WARN_ON(f1->context != f2->context))
>  		return false;
>  
> +	if (WARN_ON(f1->context == FENCE_NO_CONTEXT))
> +		return false;
> +
>  	return (int)(f1->seqno - f2->seqno) > 0;
>  }
>  
> @@ -309,6 +315,9 @@ static inline struct fence *fence_later(struct fence *f1, struct fence *f2)
>  	if (WARN_ON(f1->context != f2->context))
>  		return NULL;
>  
> +	if (WARN_ON(f1->context == FENCE_NO_CONTEXT))
> +		return NULL;
> +
>  	/*
>  	 * can't check just FENCE_FLAG_SIGNALED_BIT here, it may never have been
>  	 * set if enable_signaling wasn't called, and enabling that here is
> -- 
> 2.5.5
> 

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

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

#1387592

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

> On Mon, Apr 25, 2016 at 07:33:21PM -0300, 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 on DRM drivers. So even if there are many
> > fences in the sync_file that needs to waited for a commit to happen,
> > they all get added to the fence_collection and passed for DRM use as
> > 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, so
> > fence_is_later() and fence_later() are not meant to be called with
> > fence_collections fences.
> > 
> > v2: Comments by Daniel Vetter:
> > 	- merge fence_collection_init() and fence_collection_add()
> > 	- only add callbacks at ->enable_signalling()
> > 	- remove fence_collection_put()
> > 	- check for type on to_fence_collection()
> > 	- adjust fence_is_later() and fence_later() to WARN_ON() if they
> > 	are used with collection fences.
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> FENCE_NO_CONTEXT semantics needs an ack from amdgpu maintainers. I'm not
> entirely sure they might not hit the new WARN_ON by accident now. Please
> cc Alex Deucher & Christian König.

Sure, I'll Cc then in the revision. But if they use
fence_context_alloc() to get the context they should never hit any
WARN_ON as context numbers now starts at 1. 0 is reserved for
FENCE_NO_CONTEXT.

	Gustavo

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

#1388697

On Tue, Apr 26, 2016 at 12:02:08PM -0300, Gustavo Padovan wrote:
> 2016-04-26 Daniel Vetter <daniel@ffwll.ch>:
> 
> > On Mon, Apr 25, 2016 at 07:33:21PM -0300, 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 on DRM drivers. So even if there are many
> > > fences in the sync_file that needs to waited for a commit to happen,
> > > they all get added to the fence_collection and passed for DRM use as
> > > 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, so
> > > fence_is_later() and fence_later() are not meant to be called with
> > > fence_collections fences.
> > > 
> > > v2: Comments by Daniel Vetter:
> > > 	- merge fence_collection_init() and fence_collection_add()
> > > 	- only add callbacks at ->enable_signalling()
> > > 	- remove fence_collection_put()
> > > 	- check for type on to_fence_collection()
> > > 	- adjust fence_is_later() and fence_later() to WARN_ON() if they
> > > 	are used with collection fences.
> > > 
> > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > FENCE_NO_CONTEXT semantics needs an ack from amdgpu maintainers. I'm not
> > entirely sure they might not hit the new WARN_ON by accident now. Please
> > cc Alex Deucher & Christian König.
> 
> Sure, I'll Cc then in the revision. But if they use
> fence_context_alloc() to get the context they should never hit any
> WARN_ON as context numbers now starts at 1. 0 is reserved for
> FENCE_NO_CONTEXT.

I was more concerned whether the codepaths could accidentally walk over
non-amdgpu fences (through prime buffer sharing for example). Otoh that
would be a preexisting bug I think ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

#1387597

On Mon, Apr 25, 2016 at 07:33:21PM -0300, Gustavo Padovan wrote:
> +static const char *fence_collection_get_timeline_name(struct fence *fence)
> +{
> +	return "no context";

"unbound" to distinguish from fence contexts within a timeline?

> +static bool fence_collection_enable_signaling(struct fence *fence)
> +{
> +	struct fence_collection *collection = to_fence_collection(fence);
> +	int i;
> +
> +	for (i = 0 ; i < collection->num_fences ; i++) {
> +		if (fence_add_callback(collection->fences[i].fence,
> +				       &collection->fences[i].cb,
> +				       collection_check_cb_func)) {
> +			atomic_dec(&collection->num_pending_fences);
> +			return false;

Don't stop, we need to enable all the others!

> +		}
> +	}
> +
> +	return !!atomic_read(&collection->num_pending_fences);

Redundant !!

> +}
> +
> +static bool fence_collection_signaled(struct fence *fence)
> +{
> +	struct fence_collection *collection = to_fence_collection(fence);
> +
> +	return (atomic_read(&collection->num_pending_fences) == 0);

Redundant ()

> +static signed long fence_collection_wait(struct fence *fence, bool intr,
> +					 signed long timeout)
> +{

What advantage does this have over fence_default_wait? You enable
signaling on all, then wait sequentially. The code looks redundant and
could just use fence_default_wait instead.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web