[PATCH 1/3] dma-buf/fence: make fence context 64 bit v2

[PATCH 1/3] dma-buf/fence: make fence context 64 bit v2

[Christian König]

From: Christian König <christian.koenig@amd.com>

Fence contexts are created on the fly (for example) by the GPU scheduler used
in the amdgpu driver as a result of an userspace request. Because of this
userspace could in theory force a wrap around of the 32bit context number
if it doesn't behave well.

Avoid this by increasing the context number to 64bits. This way even when
userspace manages to allocate a billion contexts per second it takes more
than 500 years for the context number to wrap around.

v2: fix printf formats as well.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/dma-buf/fence.c                 |  8 ++++----
 drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_sa.c  |  2 +-
 drivers/gpu/drm/etnaviv/etnaviv_gpu.h   |  2 +-
 drivers/gpu/drm/nouveau/nouveau_fence.h |  3 ++-
 drivers/gpu/drm/qxl/qxl_release.c       |  2 +-
 drivers/gpu/drm/radeon/radeon.h         |  2 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_fence.c   |  2 +-
 drivers/staging/android/sync.h          |  3 ++-
 include/linux/fence.h                   | 13 +++++++------
 10 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/dma-buf/fence.c b/drivers/dma-buf/fence.c
index 7b05dbe..4d51f9e 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 atomic64_t fence_context_counter = ATOMIC64_INIT(0);
 
 /**
  * fence_context_alloc - allocate an array of fence contexts
@@ -44,10 +44,10 @@ static atomic_t fence_context_counter = ATOMIC_INIT(0);
  * This function will return the first index of the number of fences allocated.
  * The fence context is used for setting fence->context to a unique number.
  */
-unsigned fence_context_alloc(unsigned num)
+u64 fence_context_alloc(unsigned num)
 {
 	BUG_ON(!num);
-	return atomic_add_return(num, &fence_context_counter) - num;
+	return atomic64_add_return(num, &fence_context_counter) - num;
 }
 EXPORT_SYMBOL(fence_context_alloc);
 
@@ -513,7 +513,7 @@ EXPORT_SYMBOL(fence_wait_any_timeout);
  */
 void
 fence_init(struct fence *fence, const struct fence_ops *ops,
-	     spinlock_t *lock, unsigned context, unsigned seqno)
+	     spinlock_t *lock, u64 context, unsigned seqno)
 {
 	BUG_ON(!lock);
 	BUG_ON(!ops || !ops->wait || !ops->enable_signaling ||
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 992f00b..da3d021 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -2032,7 +2032,7 @@ struct amdgpu_device {
 	struct amdgpu_irq_src		hpd_irq;
 
 	/* rings */
-	unsigned			fence_context;
+	u64				fence_context;
 	unsigned			num_rings;
 	struct amdgpu_ring		*rings[AMDGPU_MAX_RINGS];
 	bool				ib_pool_ready;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sa.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sa.c
index 8bf84ef..b16366c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sa.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sa.c
@@ -427,7 +427,7 @@ void amdgpu_sa_bo_dump_debug_info(struct amdgpu_sa_manager *sa_manager,
 			   soffset, eoffset, eoffset - soffset);
 
 		if (i->fence)
-			seq_printf(m, " protected by 0x%08x on context %d",
+			seq_printf(m, " protected by 0x%08x on context %llu",
 				   i->fence->seqno, i->fence->context);
 
 		seq_printf(m, "\n");
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
index f5321e2..a69cdd5 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
@@ -125,7 +125,7 @@ struct etnaviv_gpu {
 	u32 completed_fence;
 	u32 retired_fence;
 	wait_queue_head_t fence_event;
-	unsigned int fence_context;
+	u64 fence_context;
 	spinlock_t fence_spinlock;
 
 	/* worker for handling active-list retiring: */
diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.h b/drivers/gpu/drm/nouveau/nouveau_fence.h
index 2e3a62d..64c4ce7 100644
--- a/drivers/gpu/drm/nouveau/nouveau_fence.h
+++ b/drivers/gpu/drm/nouveau/nouveau_fence.h
@@ -57,7 +57,8 @@ struct nouveau_fence_priv {
 	int  (*context_new)(struct nouveau_channel *);
 	void (*context_del)(struct nouveau_channel *);
 
-	u32 contexts, context_base;
+	u32 contexts;
+	u64 context_base;
 	bool uevent;
 };
 
diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c
index 4efa8e2..f599cd0 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -96,7 +96,7 @@ retry:
 			return 0;
 
 		if (have_drawable_releases && sc > 300) {
-			FENCE_WARN(fence, "failed to wait on release %d "
+			FENCE_WARN(fence, "failed to wait on release %llu "
 					  "after spincount %d\n",
 					  fence->context & ~0xf0000000, sc);
 			goto signaled;
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
index 80b24a4..5633ee3 100644
--- a/drivers/gpu/drm/radeon/radeon.h
+++ b/drivers/gpu/drm/radeon/radeon.h
@@ -2386,7 +2386,7 @@ struct radeon_device {
 	struct radeon_mman		mman;
 	struct radeon_fence_driver	fence_drv[RADEON_NUM_RINGS];
 	wait_queue_head_t		fence_queue;
-	unsigned			fence_context;
+	u64				fence_context;
 	struct mutex			ring_lock;
 	struct radeon_ring		ring[RADEON_NUM_RINGS];
 	bool				ib_pool_ready;
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c b/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c
index e959df6..26ac8e8 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c
@@ -46,7 +46,7 @@ struct vmw_fence_manager {
 	bool goal_irq_on; /* Protected by @goal_irq_mutex */
 	bool seqno_valid; /* Protected by @lock, and may not be set to true
 			     without the @goal_irq_mutex held. */
-	unsigned ctx;
+	u64 ctx;
 };
 
 struct vmw_user_fence {
diff --git a/drivers/staging/android/sync.h b/drivers/staging/android/sync.h
index d2a1734..ef1f7d4 100644
--- a/drivers/staging/android/sync.h
+++ b/drivers/staging/android/sync.h
@@ -68,7 +68,8 @@ struct sync_timeline {
 
 	/* protected by child_list_lock */
 	bool			destroyed;
-	int			context, value;
+	u64			context;
+	int			value;
 
 	struct list_head	child_list_head;
 	spinlock_t		child_list_lock;
diff --git a/include/linux/fence.h b/include/linux/fence.h
index 2b17698..18a97c6 100644
--- a/include/linux/fence.h
+++ b/include/linux/fence.h
@@ -75,7 +75,8 @@ struct fence {
 	struct rcu_head rcu;
 	struct list_head cb_list;
 	spinlock_t *lock;
-	unsigned context, seqno;
+	u64 context;
+	unsigned seqno;
 	unsigned long flags;
 	ktime_t timestamp;
 	int status;
@@ -178,7 +179,7 @@ struct fence_ops {
 };
 
 void fence_init(struct fence *fence, const struct fence_ops *ops,
-		spinlock_t *lock, unsigned context, unsigned seqno);
+		spinlock_t *lock, u64 context, unsigned seqno);
 
 void fence_release(struct kref *kref);
 void fence_free(struct fence *fence);
@@ -352,27 +353,27 @@ static inline signed long fence_wait(struct fence *fence, bool intr)
 	return ret < 0 ? ret : 0;
 }
 
-unsigned fence_context_alloc(unsigned num);
+u64 fence_context_alloc(unsigned num);
 
 #define FENCE_TRACE(f, fmt, args...) \
 	do {								\
 		struct fence *__ff = (f);				\
 		if (config_enabled(CONFIG_FENCE_TRACE))			\
-			pr_info("f %u#%u: " fmt,			\
+			pr_info("f %llu#%u: " fmt,			\
 				__ff->context, __ff->seqno, ##args);	\
 	} while (0)
 
 #define FENCE_WARN(f, fmt, args...) \
 	do {								\
 		struct fence *__ff = (f);				\
-		pr_warn("f %u#%u: " fmt, __ff->context, __ff->seqno,	\
+		pr_warn("f %llu#%u: " fmt, __ff->context, __ff->seqno,	\
 			 ##args);					\
 	} while (0)
 
 #define FENCE_ERR(f, fmt, args...) \
 	do {								\
 		struct fence *__ff = (f);				\
-		pr_err("f %u#%u: " fmt, __ff->context, __ff->seqno,	\
+		pr_err("f %llu#%u: " fmt, __ff->context, __ff->seqno,	\
 			##args);					\
 	} while (0)
 
-- 
2.5.0

[PATCH 2/3] dma-buf/fence: add fence_array fences v5

[Christian König]

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.

v3: - Initialize fence_cb.node at fence init.

    Comments by Chris Wilson:
	- return "unbound" on fence_collection_get_timeline_name()
	- don't stop adding callbacks if one fails
	- remove redundant !! on fence_collection_enable_signaling()
	- remove redundant () on fence_collection_signaled
	- use fence_default_wait() instead

v4 (chk): Rework, simplification and cleanup:
	- Drop FENCE_NO_CONTEXT handling, always allocate a context.
	- Rename to fence_array.
	- Return fixed driver name.
	- Register only one callback at a time.
	- Document that create function takes ownership of array.

v5 (chk): More work and fixes:
	- Avoid deadlocks by adding all callbacks at once again.
	- Stop trying to remove the callbacks.
	- Provide context and sequence number for the array fence.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/dma-buf/Makefile      |   2 +-
 drivers/dma-buf/fence-array.c | 127 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/fence-array.h   |  72 ++++++++++++++++++++++++
 3 files changed, 200 insertions(+), 1 deletion(-)
 create mode 100644 drivers/dma-buf/fence-array.c
 create mode 100644 include/linux/fence-array.h

diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile
index 57a675f..85f6928 100644
--- a/drivers/dma-buf/Makefile
+++ b/drivers/dma-buf/Makefile
@@ -1 +1 @@
-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-array.o
diff --git a/drivers/dma-buf/fence-array.c b/drivers/dma-buf/fence-array.c
new file mode 100644
index 0000000..bbcf79f
--- /dev/null
+++ b/drivers/dma-buf/fence-array.c
@@ -0,0 +1,127 @@
+/*
+ * fence-array: aggregate fences to be waited together
+ *
+ * Copyright (C) 2016 Collabora Ltd
+ * Copyright (C) 2016 Advanced Micro Devices, Inc.
+ * Authors:
+ *	Gustavo Padovan <gustavo@padovan.org>
+ *	Christian König <christian.koenig@amd.com>
+ *
+ * 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-array.h>
+
+static void fence_array_cb_func(struct fence *f, struct fence_cb *cb);
+
+static const char *fence_array_get_driver_name(struct fence *fence)
+{
+	return "fence_array";
+}
+
+static const char *fence_array_get_timeline_name(struct fence *fence)
+{
+	return "unbound";
+}
+
+static void fence_array_cb_func(struct fence *f, struct fence_cb *cb)
+{
+	struct fence_array_cb *array_cb =
+		container_of(cb, struct fence_array_cb, cb);
+	struct fence_array *array = array_cb->array;
+
+	if (atomic_dec_and_test(&array->num_pending))
+		fence_signal(&array->base);
+}
+
+static bool fence_array_enable_signaling(struct fence *fence)
+{
+	struct fence_array *array = to_fence_array(fence);
+	struct fence_array_cb *cb = (void *)(&array[1]);
+	unsigned i;
+
+	for (i = 0; i < array->num_fences; ++i) {
+		cb[i].array = array;
+		if (!fence_add_callback(array->fences[i], &cb[i].cb,
+					fence_array_cb_func))
+			if (atomic_dec_and_test(&array->num_pending))
+				return false;
+	}
+
+	return true;
+}
+
+static bool fence_array_signaled(struct fence *fence)
+{
+	struct fence_array *array = to_fence_array(fence);
+
+	return atomic_read(&array->num_pending) == 0;
+}
+
+static void fence_array_release(struct fence *fence)
+{
+	struct fence_array *array = to_fence_array(fence);
+	unsigned i;
+
+	for (i = 0; i < array->num_fences; ++i)
+		fence_put(array->fences[i]);
+
+	kfree(array->fences);
+	fence_free(fence);
+}
+
+const struct fence_ops fence_array_ops = {
+	.get_driver_name = fence_array_get_driver_name,
+	.get_timeline_name = fence_array_get_timeline_name,
+	.enable_signaling = fence_array_enable_signaling,
+	.signaled = fence_array_signaled,
+	.wait = fence_default_wait,
+	.release = fence_array_release,
+};
+
+/**
+ * fence_array_create - Create a custom fence array
+ * @num_fences:	[in]	number of fences to add in the array
+ * @fences:	[in]	array containing the fences
+ * @context:	[in]	fence context to use
+ * @seqno:	[in]	sequence number to use
+ *
+ * Allocate a fence_array object and initialize the base fence with fence_init().
+ * In case of error it returns NULL.
+ *
+ * The caller should allocte the fences array with num_fences size
+ * and fill it with the fences it wants to add to the object. Ownership of this
+ * array is take and fence_put() is used on each fence on release.
+ */
+struct fence_array *fence_array_create(int num_fences, struct fence **fences,
+				       u64 context, unsigned seqno)
+{
+	struct fence_array *array;
+	size_t size = sizeof(*array);
+
+	/* Allocate the callback structures behind the array. */
+	size += num_fences * sizeof(struct fence_array_cb);
+	array = kzalloc(size, GFP_KERNEL);
+	if (!array)
+		return NULL;
+
+	spin_lock_init(&array->lock);
+	fence_init(&array->base, &fence_array_ops, &array->lock,
+		   context, seqno);
+
+	array->num_fences = num_fences;
+	atomic_set(&array->num_pending, num_fences);
+	array->fences = fences;
+
+	return array;
+}
+EXPORT_SYMBOL(fence_array_create);
diff --git a/include/linux/fence-array.h b/include/linux/fence-array.h
new file mode 100644
index 0000000..593ab98
--- /dev/null
+++ b/include/linux/fence-array.h
@@ -0,0 +1,72 @@
+/*
+ * fence-array: aggregates fence to be waited together
+ *
+ * Copyright (C) 2016 Collabora Ltd
+ * Copyright (C) 2016 Advanced Micro Devices, Inc.
+ * Authors:
+ *	Gustavo Padovan <gustavo@padovan.org>
+ *	Christian König <christian.koenig@amd.com>
+ *
+ * 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_ARRAY_H
+#define __LINUX_FENCE_ARRAY_H
+
+#include <linux/fence.h>
+
+/**
+ * struct fence_array_cb - callback helper for fence array
+ * @cb: fence callback structure for signaling
+ * @array: reference to the parent fence array object
+ */
+struct fence_array_cb {
+	struct fence_cb cb;
+	struct fence_array *array;
+};
+
+/**
+ * struct fence_array - fence to represent an array of fences
+ * @base: fence base class
+ * @lock: spinlock for fence handling
+ * @num_fences: number of fences in the array
+ * @num_pending: fences in the array still pending
+ * @fences: array of the fences
+ */
+struct fence_array {
+	struct fence base;
+
+	spinlock_t lock;
+	unsigned num_fences;
+	atomic_t num_pending;
+	struct fence **fences;
+};
+
+extern const struct fence_ops fence_array_ops;
+
+/**
+ * to_fence_array - cast a fence to a fence_array
+ * @fence: fence to cast to a fence_array
+ *
+ * Returns NULL if the fence is not a fence_array,
+ * or the fence_array otherwise.
+ */
+static inline struct fence_array *to_fence_array(struct fence *fence)
+{
+	if (fence->ops != &fence_array_ops)
+		return NULL;
+
+	return container_of(fence, struct fence_array, base);
+}
+
+struct fence_array *fence_array_create(int num_fences, struct fence **fences,
+				       u64 context, unsigned seqno);
+
+#endif /* __LINUX_FENCE_ARRAY_H */
-- 
2.5.0

[PATCH 3/3] dma-buf/fence: add signal_on_any to the fence array

[Christian König]

From: Christian König <christian.koenig@amd.com>

If @signal_on_any is true the fence array signals if any fence in the array
signals, otherwise it signals when all fences in the array signal.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/dma-buf/fence-array.c | 23 ++++++++++++++++-------
 include/linux/fence-array.h   |  3 ++-
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/dma-buf/fence-array.c b/drivers/dma-buf/fence-array.c
index bbcf79f..726e840 100644
--- a/drivers/dma-buf/fence-array.c
+++ b/drivers/dma-buf/fence-array.c
@@ -41,6 +41,7 @@ static void fence_array_cb_func(struct fence *f, struct fence_cb *cb)
 
 	if (atomic_dec_and_test(&array->num_pending))
 		fence_signal(&array->base);
+	fence_put(&array->base);
 }
 
 static bool fence_array_enable_signaling(struct fence *fence)
@@ -51,10 +52,13 @@ static bool fence_array_enable_signaling(struct fence *fence)
 
 	for (i = 0; i < array->num_fences; ++i) {
 		cb[i].array = array;
+		fence_get(&array->base);
 		if (!fence_add_callback(array->fences[i], &cb[i].cb,
-					fence_array_cb_func))
+					fence_array_cb_func)) {
+			fence_put(&array->base);
 			if (atomic_dec_and_test(&array->num_pending))
 				return false;
+		}
 	}
 
 	return true;
@@ -90,10 +94,11 @@ const struct fence_ops fence_array_ops = {
 
 /**
  * fence_array_create - Create a custom fence array
- * @num_fences:	[in]	number of fences to add in the array
- * @fences:	[in]	array containing the fences
- * @context:	[in]	fence context to use
- * @seqno:	[in]	sequence number to use
+ * @num_fences:		[in]	number of fences to add in the array
+ * @fences:		[in]	array containing the fences
+ * @context:		[in]	fence context to use
+ * @seqno:		[in]	sequence number to use
+ * @signal_on_any	[in]	signal on any fence in the array
  *
  * Allocate a fence_array object and initialize the base fence with fence_init().
  * In case of error it returns NULL.
@@ -101,9 +106,13 @@ const struct fence_ops fence_array_ops = {
  * The caller should allocte the fences array with num_fences size
  * and fill it with the fences it wants to add to the object. Ownership of this
  * array is take and fence_put() is used on each fence on release.
+ *
+ * If @signal_on_any is true the fence array signals if any fence in the array
+ * signals, otherwise it signals when all fences in the array signal.
  */
 struct fence_array *fence_array_create(int num_fences, struct fence **fences,
-				       u64 context, unsigned seqno)
+				       u64 context, unsigned seqno,
+				       bool signal_on_any)
 {
 	struct fence_array *array;
 	size_t size = sizeof(*array);
@@ -119,7 +128,7 @@ struct fence_array *fence_array_create(int num_fences, struct fence **fences,
 		   context, seqno);
 
 	array->num_fences = num_fences;
-	atomic_set(&array->num_pending, num_fences);
+	atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences);
 	array->fences = fences;
 
 	return array;
diff --git a/include/linux/fence-array.h b/include/linux/fence-array.h
index 593ab98..86baaa4 100644
--- a/include/linux/fence-array.h
+++ b/include/linux/fence-array.h
@@ -67,6 +67,7 @@ static inline struct fence_array *to_fence_array(struct fence *fence)
 }
 
 struct fence_array *fence_array_create(int num_fences, struct fence **fences,
-				       u64 context, unsigned seqno);
+				       u64 context, unsigned seqno,
+				       bool signal_on_any);
 
 #endif /* __LINUX_FENCE_ARRAY_H */
-- 
2.5.0

Re: [PATCH 3/3] dma-buf/fence: add signal_on_any to the fence array

[Chris Wilson]

On Mon, May 23, 2016 at 01:25:22PM +0200, Christian König wrote:
> From: Christian König <christian.koenig@amd.com>
> 
> If @signal_on_any is true the fence array signals if any fence in the array
> signals, otherwise it signals when all fences in the array signal.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/dma-buf/fence-array.c | 23 ++++++++++++++++-------
>  include/linux/fence-array.h   |  3 ++-
>  2 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/dma-buf/fence-array.c b/drivers/dma-buf/fence-array.c
> index bbcf79f..726e840 100644
> --- a/drivers/dma-buf/fence-array.c
> +++ b/drivers/dma-buf/fence-array.c
> @@ -41,6 +41,7 @@ static void fence_array_cb_func(struct fence *f, struct fence_cb *cb)
>  
>  	if (atomic_dec_and_test(&array->num_pending))
>  		fence_signal(&array->base);
> +	fence_put(&array->base);
>  }
>  
>  static bool fence_array_enable_signaling(struct fence *fence)
> @@ -51,10 +52,13 @@ static bool fence_array_enable_signaling(struct fence *fence)
>  
>  	for (i = 0; i < array->num_fences; ++i) {
>  		cb[i].array = array;
> +		fence_get(&array->base);
>  		if (!fence_add_callback(array->fences[i], &cb[i].cb,
> -					fence_array_cb_func))
> +					fence_array_cb_func)) {
> +			fence_put(&array->base);

Ok, yes, that is an artifact of reporting signaled before all cb are
complete.

/* As we may report that the fence is signaled before all callbacks are
 * complete, we need to take an additional reference count on the array
 * so that we do not free it too early. The core fence handling will
 * only hold the reference until we signal the array as complete (but
 * that is now insufficient).
 */

fence_array_signaled() should now be
	return atomic_read(&array->num_pending) <= 0;
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/3] dma-buf/fence: add signal_on_any to the fence array

[Daniel Vetter]

On Mon, May 23, 2016 at 01:25:22PM +0200, Christian König wrote:
> From: Christian König <christian.koenig@amd.com>
> 
> If @signal_on_any is true the fence array signals if any fence in the array
> signals, otherwise it signals when all fences in the array signal.

What do you need this for? Sounds neat, but I can't come up with a use
case. And that use case imo should be both in the commit message and
kerneldoc.
-Daniel

> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/dma-buf/fence-array.c | 23 ++++++++++++++++-------
>  include/linux/fence-array.h   |  3 ++-
>  2 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/dma-buf/fence-array.c b/drivers/dma-buf/fence-array.c
> index bbcf79f..726e840 100644
> --- a/drivers/dma-buf/fence-array.c
> +++ b/drivers/dma-buf/fence-array.c
> @@ -41,6 +41,7 @@ static void fence_array_cb_func(struct fence *f, struct fence_cb *cb)
>  
>  	if (atomic_dec_and_test(&array->num_pending))
>  		fence_signal(&array->base);
> +	fence_put(&array->base);
>  }
>  
>  static bool fence_array_enable_signaling(struct fence *fence)
> @@ -51,10 +52,13 @@ static bool fence_array_enable_signaling(struct fence *fence)
>  
>  	for (i = 0; i < array->num_fences; ++i) {
>  		cb[i].array = array;
> +		fence_get(&array->base);
>  		if (!fence_add_callback(array->fences[i], &cb[i].cb,
> -					fence_array_cb_func))
> +					fence_array_cb_func)) {
> +			fence_put(&array->base);
>  			if (atomic_dec_and_test(&array->num_pending))
>  				return false;
> +		}
>  	}
>  
>  	return true;
> @@ -90,10 +94,11 @@ const struct fence_ops fence_array_ops = {
>  
>  /**
>   * fence_array_create - Create a custom fence array
> - * @num_fences:	[in]	number of fences to add in the array
> - * @fences:	[in]	array containing the fences
> - * @context:	[in]	fence context to use
> - * @seqno:	[in]	sequence number to use
> + * @num_fences:		[in]	number of fences to add in the array
> + * @fences:		[in]	array containing the fences
> + * @context:		[in]	fence context to use
> + * @seqno:		[in]	sequence number to use
> + * @signal_on_any	[in]	signal on any fence in the array
>   *
>   * Allocate a fence_array object and initialize the base fence with fence_init().
>   * In case of error it returns NULL.
> @@ -101,9 +106,13 @@ const struct fence_ops fence_array_ops = {
>   * The caller should allocte the fences array with num_fences size
>   * and fill it with the fences it wants to add to the object. Ownership of this
>   * array is take and fence_put() is used on each fence on release.
> + *
> + * If @signal_on_any is true the fence array signals if any fence in the array
> + * signals, otherwise it signals when all fences in the array signal.
>   */
>  struct fence_array *fence_array_create(int num_fences, struct fence **fences,
> -				       u64 context, unsigned seqno)
> +				       u64 context, unsigned seqno,
> +				       bool signal_on_any)
>  {
>  	struct fence_array *array;
>  	size_t size = sizeof(*array);
> @@ -119,7 +128,7 @@ struct fence_array *fence_array_create(int num_fences, struct fence **fences,
>  		   context, seqno);
>  
>  	array->num_fences = num_fences;
> -	atomic_set(&array->num_pending, num_fences);
> +	atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences);
>  	array->fences = fences;
>  
>  	return array;
> diff --git a/include/linux/fence-array.h b/include/linux/fence-array.h
> index 593ab98..86baaa4 100644
> --- a/include/linux/fence-array.h
> +++ b/include/linux/fence-array.h
> @@ -67,6 +67,7 @@ static inline struct fence_array *to_fence_array(struct fence *fence)
>  }
>  
>  struct fence_array *fence_array_create(int num_fences, struct fence **fences,
> -				       u64 context, unsigned seqno);
> +				       u64 context, unsigned seqno,
> +				       bool signal_on_any);
>  
>  #endif /* __LINUX_FENCE_ARRAY_H */
> -- 
> 2.5.0
> 

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

Re: [PATCH 2/3] dma-buf/fence: add fence_array fences v5

[Daniel Vetter]

On Mon, May 23, 2016 at 01:25:21PM +0200, Christian König 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.
> 
> v3: - Initialize fence_cb.node at fence init.
> 
>     Comments by Chris Wilson:
> 	- return "unbound" on fence_collection_get_timeline_name()
> 	- don't stop adding callbacks if one fails
> 	- remove redundant !! on fence_collection_enable_signaling()
> 	- remove redundant () on fence_collection_signaled
> 	- use fence_default_wait() instead
> 
> v4 (chk): Rework, simplification and cleanup:
> 	- Drop FENCE_NO_CONTEXT handling, always allocate a context.
> 	- Rename to fence_array.
> 	- Return fixed driver name.
> 	- Register only one callback at a time.
> 	- Document that create function takes ownership of array.
> 
> v5 (chk): More work and fixes:
> 	- Avoid deadlocks by adding all callbacks at once again.
> 	- Stop trying to remove the callbacks.
> 	- Provide context and sequence number for the array fence.

That looks like even more a step backwards - I don't see how it's used in
your amdgpu code, but now callers need to either allocate a context
unconditionally (more boilerplate code) or do some fragile dance to figure
out whether maybe they can share the context with some other, previous
fence_array.

If you just want to avoid allocating a new context if all the fences in
the array are from the same one, then I'd say detect that and just return
that single fence directly (with an additional reference ofc).

Or I'm not entirely understanding things, or probably not at all ;-)
-Daniel

> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/dma-buf/Makefile      |   2 +-
>  drivers/dma-buf/fence-array.c | 127 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/fence-array.h   |  72 ++++++++++++++++++++++++
>  3 files changed, 200 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/dma-buf/fence-array.c
>  create mode 100644 include/linux/fence-array.h
> 
> diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile
> index 57a675f..85f6928 100644
> --- a/drivers/dma-buf/Makefile
> +++ b/drivers/dma-buf/Makefile
> @@ -1 +1 @@
> -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-array.o
> diff --git a/drivers/dma-buf/fence-array.c b/drivers/dma-buf/fence-array.c
> new file mode 100644
> index 0000000..bbcf79f
> --- /dev/null
> +++ b/drivers/dma-buf/fence-array.c
> @@ -0,0 +1,127 @@
> +/*
> + * fence-array: aggregate fences to be waited together
> + *
> + * Copyright (C) 2016 Collabora Ltd
> + * Copyright (C) 2016 Advanced Micro Devices, Inc.
> + * Authors:
> + *	Gustavo Padovan <gustavo@padovan.org>
> + *	Christian König <christian.koenig@amd.com>
> + *
> + * 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-array.h>
> +
> +static void fence_array_cb_func(struct fence *f, struct fence_cb *cb);
> +
> +static const char *fence_array_get_driver_name(struct fence *fence)
> +{
> +	return "fence_array";
> +}
> +
> +static const char *fence_array_get_timeline_name(struct fence *fence)
> +{
> +	return "unbound";
> +}
> +
> +static void fence_array_cb_func(struct fence *f, struct fence_cb *cb)
> +{
> +	struct fence_array_cb *array_cb =
> +		container_of(cb, struct fence_array_cb, cb);
> +	struct fence_array *array = array_cb->array;
> +
> +	if (atomic_dec_and_test(&array->num_pending))
> +		fence_signal(&array->base);
> +}
> +
> +static bool fence_array_enable_signaling(struct fence *fence)
> +{
> +	struct fence_array *array = to_fence_array(fence);
> +	struct fence_array_cb *cb = (void *)(&array[1]);
> +	unsigned i;
> +
> +	for (i = 0; i < array->num_fences; ++i) {
> +		cb[i].array = array;
> +		if (!fence_add_callback(array->fences[i], &cb[i].cb,
> +					fence_array_cb_func))
> +			if (atomic_dec_and_test(&array->num_pending))
> +				return false;
> +	}
> +
> +	return true;
> +}
> +
> +static bool fence_array_signaled(struct fence *fence)
> +{
> +	struct fence_array *array = to_fence_array(fence);
> +
> +	return atomic_read(&array->num_pending) == 0;
> +}
> +
> +static void fence_array_release(struct fence *fence)
> +{
> +	struct fence_array *array = to_fence_array(fence);
> +	unsigned i;
> +
> +	for (i = 0; i < array->num_fences; ++i)
> +		fence_put(array->fences[i]);
> +
> +	kfree(array->fences);
> +	fence_free(fence);
> +}
> +
> +const struct fence_ops fence_array_ops = {
> +	.get_driver_name = fence_array_get_driver_name,
> +	.get_timeline_name = fence_array_get_timeline_name,
> +	.enable_signaling = fence_array_enable_signaling,
> +	.signaled = fence_array_signaled,
> +	.wait = fence_default_wait,
> +	.release = fence_array_release,
> +};
> +
> +/**
> + * fence_array_create - Create a custom fence array
> + * @num_fences:	[in]	number of fences to add in the array
> + * @fences:	[in]	array containing the fences
> + * @context:	[in]	fence context to use
> + * @seqno:	[in]	sequence number to use
> + *
> + * Allocate a fence_array object and initialize the base fence with fence_init().
> + * In case of error it returns NULL.
> + *
> + * The caller should allocte the fences array with num_fences size
> + * and fill it with the fences it wants to add to the object. Ownership of this
> + * array is take and fence_put() is used on each fence on release.
> + */
> +struct fence_array *fence_array_create(int num_fences, struct fence **fences,
> +				       u64 context, unsigned seqno)
> +{
> +	struct fence_array *array;
> +	size_t size = sizeof(*array);
> +
> +	/* Allocate the callback structures behind the array. */
> +	size += num_fences * sizeof(struct fence_array_cb);
> +	array = kzalloc(size, GFP_KERNEL);
> +	if (!array)
> +		return NULL;
> +
> +	spin_lock_init(&array->lock);
> +	fence_init(&array->base, &fence_array_ops, &array->lock,
> +		   context, seqno);
> +
> +	array->num_fences = num_fences;
> +	atomic_set(&array->num_pending, num_fences);
> +	array->fences = fences;
> +
> +	return array;
> +}
> +EXPORT_SYMBOL(fence_array_create);
> diff --git a/include/linux/fence-array.h b/include/linux/fence-array.h
> new file mode 100644
> index 0000000..593ab98
> --- /dev/null
> +++ b/include/linux/fence-array.h
> @@ -0,0 +1,72 @@
> +/*
> + * fence-array: aggregates fence to be waited together
> + *
> + * Copyright (C) 2016 Collabora Ltd
> + * Copyright (C) 2016 Advanced Micro Devices, Inc.
> + * Authors:
> + *	Gustavo Padovan <gustavo@padovan.org>
> + *	Christian König <christian.koenig@amd.com>
> + *
> + * 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_ARRAY_H
> +#define __LINUX_FENCE_ARRAY_H
> +
> +#include <linux/fence.h>
> +
> +/**
> + * struct fence_array_cb - callback helper for fence array
> + * @cb: fence callback structure for signaling
> + * @array: reference to the parent fence array object
> + */
> +struct fence_array_cb {
> +	struct fence_cb cb;
> +	struct fence_array *array;
> +};
> +
> +/**
> + * struct fence_array - fence to represent an array of fences
> + * @base: fence base class
> + * @lock: spinlock for fence handling
> + * @num_fences: number of fences in the array
> + * @num_pending: fences in the array still pending
> + * @fences: array of the fences
> + */
> +struct fence_array {
> +	struct fence base;
> +
> +	spinlock_t lock;
> +	unsigned num_fences;
> +	atomic_t num_pending;
> +	struct fence **fences;
> +};
> +
> +extern const struct fence_ops fence_array_ops;
> +
> +/**
> + * to_fence_array - cast a fence to a fence_array
> + * @fence: fence to cast to a fence_array
> + *
> + * Returns NULL if the fence is not a fence_array,
> + * or the fence_array otherwise.
> + */
> +static inline struct fence_array *to_fence_array(struct fence *fence)
> +{
> +	if (fence->ops != &fence_array_ops)
> +		return NULL;
> +
> +	return container_of(fence, struct fence_array, base);
> +}
> +
> +struct fence_array *fence_array_create(int num_fences, struct fence **fences,
> +				       u64 context, unsigned seqno);
> +
> +#endif /* __LINUX_FENCE_ARRAY_H */
> -- 
> 2.5.0
> 

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

Re: [PATCH 3/3] dma-buf/fence: add signal_on_any to the fence array

[Christian König]

Am 23.05.2016 um 15:50 schrieb Daniel Vetter:
> On Mon, May 23, 2016 at 01:25:22PM +0200, Christian König wrote:
>> From: Christian König <christian.koenig@amd.com>
>>
>> If @signal_on_any is true the fence array signals if any fence in the array
>> signals, otherwise it signals when all fences in the array signal.
> What do you need this for? Sounds neat, but I can't come up with a use
> case. And that use case imo should be both in the commit message and
> kerneldoc.

Good point, going to fix this when if we really use the patch. Just 
wanted to get opinions on this.

I'm currently working on making some of the resource management in 
amdgpu more friendly to our GPU scheduler.

Waiting for resources in the GPU scheduler is implemented as waiting for 
a fence to fire. So when you want to wait for any resource to become 
available (like in our case the VMIDs) you could make good use of 
something like this.

Christian.

> -Daniel
>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/dma-buf/fence-array.c | 23 ++++++++++++++++-------
>>   include/linux/fence-array.h   |  3 ++-
>>   2 files changed, 18 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/dma-buf/fence-array.c b/drivers/dma-buf/fence-array.c
>> index bbcf79f..726e840 100644
>> --- a/drivers/dma-buf/fence-array.c
>> +++ b/drivers/dma-buf/fence-array.c
>> @@ -41,6 +41,7 @@ static void fence_array_cb_func(struct fence *f, struct fence_cb *cb)
>>   
>>   	if (atomic_dec_and_test(&array->num_pending))
>>   		fence_signal(&array->base);
>> +	fence_put(&array->base);
>>   }
>>   
>>   static bool fence_array_enable_signaling(struct fence *fence)
>> @@ -51,10 +52,13 @@ static bool fence_array_enable_signaling(struct fence *fence)
>>   
>>   	for (i = 0; i < array->num_fences; ++i) {
>>   		cb[i].array = array;
>> +		fence_get(&array->base);
>>   		if (!fence_add_callback(array->fences[i], &cb[i].cb,
>> -					fence_array_cb_func))
>> +					fence_array_cb_func)) {
>> +			fence_put(&array->base);
>>   			if (atomic_dec_and_test(&array->num_pending))
>>   				return false;
>> +		}
>>   	}
>>   
>>   	return true;
>> @@ -90,10 +94,11 @@ const struct fence_ops fence_array_ops = {
>>   
>>   /**
>>    * fence_array_create - Create a custom fence array
>> - * @num_fences:	[in]	number of fences to add in the array
>> - * @fences:	[in]	array containing the fences
>> - * @context:	[in]	fence context to use
>> - * @seqno:	[in]	sequence number to use
>> + * @num_fences:		[in]	number of fences to add in the array
>> + * @fences:		[in]	array containing the fences
>> + * @context:		[in]	fence context to use
>> + * @seqno:		[in]	sequence number to use
>> + * @signal_on_any	[in]	signal on any fence in the array
>>    *
>>    * Allocate a fence_array object and initialize the base fence with fence_init().
>>    * In case of error it returns NULL.
>> @@ -101,9 +106,13 @@ const struct fence_ops fence_array_ops = {
>>    * The caller should allocte the fences array with num_fences size
>>    * and fill it with the fences it wants to add to the object. Ownership of this
>>    * array is take and fence_put() is used on each fence on release.
>> + *
>> + * If @signal_on_any is true the fence array signals if any fence in the array
>> + * signals, otherwise it signals when all fences in the array signal.
>>    */
>>   struct fence_array *fence_array_create(int num_fences, struct fence **fences,
>> -				       u64 context, unsigned seqno)
>> +				       u64 context, unsigned seqno,
>> +				       bool signal_on_any)
>>   {
>>   	struct fence_array *array;
>>   	size_t size = sizeof(*array);
>> @@ -119,7 +128,7 @@ struct fence_array *fence_array_create(int num_fences, struct fence **fences,
>>   		   context, seqno);
>>   
>>   	array->num_fences = num_fences;
>> -	atomic_set(&array->num_pending, num_fences);
>> +	atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences);
>>   	array->fences = fences;
>>   
>>   	return array;
>> diff --git a/include/linux/fence-array.h b/include/linux/fence-array.h
>> index 593ab98..86baaa4 100644
>> --- a/include/linux/fence-array.h
>> +++ b/include/linux/fence-array.h
>> @@ -67,6 +67,7 @@ static inline struct fence_array *to_fence_array(struct fence *fence)
>>   }
>>   
>>   struct fence_array *fence_array_create(int num_fences, struct fence **fences,
>> -				       u64 context, unsigned seqno);
>> +				       u64 context, unsigned seqno,
>> +				       bool signal_on_any);
>>   
>>   #endif /* __LINUX_FENCE_ARRAY_H */
>> -- 
>> 2.5.0
>>

Re: [PATCH 3/3] dma-buf/fence: add signal_on_any to the fence array

[Daniel Vetter]

On Mon, May 23, 2016 at 03:58:41PM +0200, Christian König wrote:
> Am 23.05.2016 um 15:50 schrieb Daniel Vetter:
> >On Mon, May 23, 2016 at 01:25:22PM +0200, Christian König wrote:
> >>From: Christian König <christian.koenig@amd.com>
> >>
> >>If @signal_on_any is true the fence array signals if any fence in the array
> >>signals, otherwise it signals when all fences in the array signal.
> >What do you need this for? Sounds neat, but I can't come up with a use
> >case. And that use case imo should be both in the commit message and
> >kerneldoc.
> 
> Good point, going to fix this when if we really use the patch. Just wanted
> to get opinions on this.

For getting opinions on this I get a bit the impression we're slightly
talking past each another. Having the actual user of these functions handy
should help a lot for this discussion.

> I'm currently working on making some of the resource management in amdgpu
> more friendly to our GPU scheduler.
> 
> Waiting for resources in the GPU scheduler is implemented as waiting for a
> fence to fire. So when you want to wait for any resource to become available
> (like in our case the VMIDs) you could make good use of something like this.

Hm, ok, if you want a fence that fires when any resource becomes available
of a given type becomes available that might be useful. But how do you
handle the next work item that needs such a resource? Add _all_ previous
fences that use it (there's not much ordering with the scheduler) and let
them race, and retry if they fail?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch