linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] dma-buf/fence: make fence context 64 bit v2
@ 2016-05-20 13:56 Christian König
  2016-05-20 13:56 ` [PATCH 2/2] dma-buf/fence: add fence_array fences v4 Christian König
  0 siblings, 1 reply; 10+ messages in thread
From: Christian König @ 2016-05-20 13:56 UTC (permalink / raw)
  To: dri-devel; +Cc: linux-kernel, daniel.vetter, gustavo

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/2] dma-buf/fence: add fence_array fences v4
  2016-05-20 13:56 [PATCH 1/2] dma-buf/fence: make fence context 64 bit v2 Christian König
@ 2016-05-20 13:56 ` Christian König
  2016-05-20 14:42   ` Chris Wilson
  2016-05-20 14:47   ` Gustavo Padovan
  0 siblings, 2 replies; 10+ messages in thread
From: Christian König @ 2016-05-20 13:56 UTC (permalink / raw)
  To: dri-devel; +Cc: linux-kernel, daniel.vetter, gustavo

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.

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 | 132 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/fence-array.h   |  62 ++++++++++++++++++++
 3 files changed, 195 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..a700c6e
--- /dev/null
+++ b/drivers/dma-buf/fence-array.c
@@ -0,0 +1,132 @@
+/*
+ * 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 bool fence_array_add_next_callback(struct fence_array *array)
+{
+	while (array->num_signaled < array->num_fences) {
+		struct fence *next = array->fences[array->num_signaled];
+
+		if (!fence_add_callback(next, &array->cb, fence_array_cb_func))
+			return true;
+
+		++array->num_signaled;
+	}
+
+	return false;
+}
+
+static void fence_array_cb_func(struct fence *f, struct fence_cb *cb)
+{
+	struct fence_array *array = container_of(cb, struct fence_array, cb);
+
+	++array->num_signaled;
+	if (!fence_array_add_next_callback(array))
+		fence_signal(&array->base);
+}
+
+static bool fence_array_enable_signaling(struct fence *fence)
+{
+	struct fence_array *array = to_fence_array(fence);
+
+	return fence_array_add_next_callback(array);
+}
+
+static bool fence_array_signaled(struct fence *fence)
+{
+	struct fence_array *array = to_fence_array(fence);
+
+	return ACCESS_ONCE(array->num_signaled) == array->num_fences;
+}
+
+static void fence_array_release(struct fence *fence)
+{
+	struct fence_array *array = to_fence_array(fence);
+	unsigned i;
+
+	i = ACCESS_ONCE(array->num_signaled);
+	if (i < array->num_fences) {
+		struct fence *last = array->fences[i];
+
+		fence_remove_callback(last, &array->cb);
+	}
+
+	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
+ *
+ * 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)
+{
+	struct fence_array *array;
+	int i;
+
+	array = kzalloc(sizeof(*array), GFP_KERNEL);
+	if (!array)
+		return NULL;
+
+	spin_lock_init(&array->lock);
+	fence_init(&array->base, &fence_array_ops, &array->lock,
+		   fence_context_alloc(1), 0);
+
+	array->num_signaled = 0;
+	array->num_fences = 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..f1daeb3
--- /dev/null
+++ b/include/linux/fence-array.h
@@ -0,0 +1,62 @@
+/*
+ * 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 - fence to represent an array of fences
+ * @base: fence base class
+ * @lock: spinlock for fence handling
+ * @cb: fence callback structure for signaling
+ * @num_signaled: fences in the array already signaled
+ * @num_fences: number of fences in the array
+ * @fences: array of the fences
+ */
+struct fence_array {
+	struct fence base;
+
+	spinlock_t lock;
+	struct fence_cb cb;
+	unsigned num_signaled;
+	unsigned num_fences;
+	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);
+
+#endif /* __LINUX_FENCE_ARRAY_H */
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] dma-buf/fence: add fence_array fences v4
  2016-05-20 13:56 ` [PATCH 2/2] dma-buf/fence: add fence_array fences v4 Christian König
@ 2016-05-20 14:42   ` Chris Wilson
  2016-05-23  7:32     ` Christian König
  2016-05-20 14:47   ` Gustavo Padovan
  1 sibling, 1 reply; 10+ messages in thread
From: Chris Wilson @ 2016-05-20 14:42 UTC (permalink / raw)
  To: Christian König; +Cc: dri-devel, daniel.vetter, linux-kernel

On Fri, May 20, 2016 at 03:56:11PM +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.

Why? Even within a driver I expected there to be some amoritization of
the signaling costs for handling multiple fences at once (at least the
driver I'm familar with!).

So more just curiousity as to your experience that favours sequential
enabling.

> +static bool fence_array_add_next_callback(struct fence_array *array)
> +{
> +	while (array->num_signaled < array->num_fences) {
> +		struct fence *next = array->fences[array->num_signaled];
> +
> +		if (!fence_add_callback(next, &array->cb, fence_array_cb_func))
> +			return true;
> +
> +		++array->num_signaled;
> +	}
> +
> +	return false;
> +}
> +
> +static void fence_array_cb_func(struct fence *f, struct fence_cb *cb)
> +{
> +	struct fence_array *array = container_of(cb, struct fence_array, cb);

Some chasing around would have been saved by a

	assert_spin_locked(&array->lock);

here.

> +
> +	++array->num_signaled;
> +	if (!fence_array_add_next_callback(array))
> +		fence_signal(&array->base);
> +}
> +
> +static bool fence_array_enable_signaling(struct fence *fence)
> +{
> +	struct fence_array *array = to_fence_array(fence);
> +
> +	return fence_array_add_next_callback(array);
> +}
> +
> +static bool fence_array_signaled(struct fence *fence)
> +{
> +	struct fence_array *array = to_fence_array(fence);
> +
> +	return ACCESS_ONCE(array->num_signaled) == array->num_fences;

Can just be READ_ONCE()
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] dma-buf/fence: add fence_array fences v4
  2016-05-20 13:56 ` [PATCH 2/2] dma-buf/fence: add fence_array fences v4 Christian König
  2016-05-20 14:42   ` Chris Wilson
@ 2016-05-20 14:47   ` Gustavo Padovan
  2016-05-20 17:53     ` Christian König
  2016-05-23  7:41     ` Daniel Vetter
  1 sibling, 2 replies; 10+ messages in thread
From: Gustavo Padovan @ 2016-05-20 14:47 UTC (permalink / raw)
  To: Christian König; +Cc: dri-devel, linux-kernel, daniel.vetter

2016-05-20 Christian König <deathsimple@vodafone.de>:

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

This looks good to me. Dropping NO_CONTEXT was a good idea, also
registering only one callback makes it looks better.

	Gustavo

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] dma-buf/fence: add fence_array fences v4
  2016-05-20 14:47   ` Gustavo Padovan
@ 2016-05-20 17:53     ` Christian König
  2016-05-23  7:41     ` Daniel Vetter
  1 sibling, 0 replies; 10+ messages in thread
From: Christian König @ 2016-05-20 17:53 UTC (permalink / raw)
  To: Gustavo Padovan, dri-devel, linux-kernel, daniel.vetter

Am 20.05.2016 um 16:47 schrieb Gustavo Padovan:
> 2016-05-20 Christian König <deathsimple@vodafone.de>:
>
>> 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.
> This looks good to me. Dropping NO_CONTEXT was a good idea, also
> registering only one callback makes it looks better.

Thinking about it a bit more I think we need to avoid removing the 
callback when the fence is released as well.

That stuff is just a bit to racy (see the comment on the 
fence_remove_callback function as well).

I will just grab a reference to the fence while there is any callback 
registered.

Also please note that this is only compile tested at the moment. I'm 
still working on integrating it into my code.

Regards,
Christian.

>
> 	Gustavo

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] dma-buf/fence: add fence_array fences v4
  2016-05-20 14:42   ` Chris Wilson
@ 2016-05-23  7:32     ` Christian König
  0 siblings, 0 replies; 10+ messages in thread
From: Christian König @ 2016-05-23  7:32 UTC (permalink / raw)
  To: Chris Wilson, dri-devel, daniel.vetter, linux-kernel

Am 20.05.2016 um 16:42 schrieb Chris Wilson:
> On Fri, May 20, 2016 at 03:56:11PM +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.
> Why? Even within a driver I expected there to be some amoritization of
> the signaling costs for handling multiple fences at once (at least the
> driver I'm familar with!).
>
> So more just curiousity as to your experience that favours sequential
> enabling.

Just the profane reason that I want to save the memory for all the 
callbacks.

But thinking about it you are probably right that we should enable the 
signaling for all fences immediately. Going to fix this in the next 
version of the patch.

>
>> +static bool fence_array_add_next_callback(struct fence_array *array)
>> +{
>> +	while (array->num_signaled < array->num_fences) {
>> +		struct fence *next = array->fences[array->num_signaled];
>> +
>> +		if (!fence_add_callback(next, &array->cb, fence_array_cb_func))
>> +			return true;
>> +
>> +		++array->num_signaled;
>> +	}
>> +
>> +	return false;
>> +}
>> +
>> +static void fence_array_cb_func(struct fence *f, struct fence_cb *cb)
>> +{
>> +	struct fence_array *array = container_of(cb, struct fence_array, cb);
> Some chasing around would have been saved by a
>
> 	assert_spin_locked(&array->lock);
>
> here.

Mhm, actually the array lock isn't held here. Thinking more about it 
adding a new callback from a fence callback can badly deadlock under 
certain situations.

I need to double check why the callback is called with the fence lock 
held here.

>
>> +
>> +	++array->num_signaled;
>> +	if (!fence_array_add_next_callback(array))
>> +		fence_signal(&array->base);
>> +}
>> +
>> +static bool fence_array_enable_signaling(struct fence *fence)
>> +{
>> +	struct fence_array *array = to_fence_array(fence);
>> +
>> +	return fence_array_add_next_callback(array);
>> +}
>> +
>> +static bool fence_array_signaled(struct fence *fence)
>> +{
>> +	struct fence_array *array = to_fence_array(fence);
>> +
>> +	return ACCESS_ONCE(array->num_signaled) == array->num_fences;
> Can just be READ_ONCE()

Good point, going to fix that.

Christian.

> -Chris
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] dma-buf/fence: add fence_array fences v4
  2016-05-20 14:47   ` Gustavo Padovan
  2016-05-20 17:53     ` Christian König
@ 2016-05-23  7:41     ` Daniel Vetter
  2016-05-23 11:29       ` Christian König
  1 sibling, 1 reply; 10+ messages in thread
From: Daniel Vetter @ 2016-05-23  7:41 UTC (permalink / raw)
  To: Gustavo Padovan, Christian König, dri-devel, linux-kernel,
	daniel.vetter

On Fri, May 20, 2016 at 11:47:28AM -0300, Gustavo Padovan wrote:
> 2016-05-20 Christian König <deathsimple@vodafone.de>:
> 
> > 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.
> 
> This looks good to me. Dropping NO_CONTEXT was a good idea, also
> registering only one callback makes it looks better.

This will make it even harder to eventually add a real fence_context
structure for tracking and debugging. I know you don't care for amdgpu
since you have amdgpu-specific debug files, and there's some lifetime fun
that makes it not immediately obvious how to resolve it. But on "lots of
shitty little drivers" systems aka SoCs generic debugging information is
crucial I think. Not liking too much where this is going.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] dma-buf/fence: add fence_array fences v4
  2016-05-23  7:41     ` Daniel Vetter
@ 2016-05-23 11:29       ` Christian König
  2016-05-23 14:00         ` Daniel Vetter
  0 siblings, 1 reply; 10+ messages in thread
From: Christian König @ 2016-05-23 11:29 UTC (permalink / raw)
  To: Gustavo Padovan, dri-devel, linux-kernel

Am 23.05.2016 um 09:41 schrieb Daniel Vetter:
> On Fri, May 20, 2016 at 11:47:28AM -0300, Gustavo Padovan wrote:
>> 2016-05-20 Christian König <deathsimple@vodafone.de>:
>>
>>> 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.
>> This looks good to me. Dropping NO_CONTEXT was a good idea, also
>> registering only one callback makes it looks better.
> This will make it even harder to eventually add a real fence_context
> structure for tracking and debugging. I know you don't care for amdgpu
> since you have amdgpu-specific debug files, and there's some lifetime fun
> that makes it not immediately obvious how to resolve it.

Completely independent of my work on amdgpu I still think that it's not 
such a good idea to use a complex structure for the fence context.

Especially on SoCs and small embedded systems you probably don't want to 
overhead associated with that only for debugging purposes in a 
production environment.

> But on "lots of
> shitty little drivers" systems aka SoCs generic debugging information is
> crucial I think. Not liking too much where this is going.

Yeah I agree that generic debugging information is usually crucial, but 
the lifetime issues indeed can't be solved without reference counting 
and a hole bunch of overhead.

How about V5 of the patch I've just send out? Apart from fixing a few 
issues I've made the context and sequence number parameters of the 
fence_array object.

This way you don't need to always allocate a new context for each 
object, but just enough to keep your timelines straight.

E.g. you don't get a lot of contexts only used once. This is at least 
sufficient for my amdgpu use case.

Regards,
Christian.

> -Daniel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] dma-buf/fence: add fence_array fences v4
  2016-05-23 11:29       ` Christian König
@ 2016-05-23 14:00         ` Daniel Vetter
  0 siblings, 0 replies; 10+ messages in thread
From: Daniel Vetter @ 2016-05-23 14:00 UTC (permalink / raw)
  To: Christian König; +Cc: Gustavo Padovan, dri-devel, linux-kernel

On Mon, May 23, 2016 at 01:29:11PM +0200, Christian König wrote:
> Am 23.05.2016 um 09:41 schrieb Daniel Vetter:
> >On Fri, May 20, 2016 at 11:47:28AM -0300, Gustavo Padovan wrote:
> >>2016-05-20 Christian König <deathsimple@vodafone.de>:
> >>
> >>>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.
> >>This looks good to me. Dropping NO_CONTEXT was a good idea, also
> >>registering only one callback makes it looks better.
> >This will make it even harder to eventually add a real fence_context
> >structure for tracking and debugging. I know you don't care for amdgpu
> >since you have amdgpu-specific debug files, and there's some lifetime fun
> >that makes it not immediately obvious how to resolve it.
> 
> Completely independent of my work on amdgpu I still think that it's not such
> a good idea to use a complex structure for the fence context.
> 
> Especially on SoCs and small embedded systems you probably don't want to
> overhead associated with that only for debugging purposes in a production
> environment.

At least in all the drivers I've seen you have to allocate a little bit of
stuff _anyway_ to store that context id, plus a bunch of hw state. So the
allocation itsel shouldn't be a problem at all, since that can be handled
by embedding.

If it's the atomic inc/dec for refcounting you're worried about, then that
could be made dependent on CONFIG_FENCE_DEBUGGING. And android didn't have
that Kconfig knob even, and people seemingly didn't care about the
overhead on arm socs.

> >But on "lots of
> >shitty little drivers" systems aka SoCs generic debugging information is
> >crucial I think. Not liking too much where this is going.
> 
> Yeah I agree that generic debugging information is usually crucial, but the
> lifetime issues indeed can't be solved without reference counting and a hole
> bunch of overhead.
> 
> How about V5 of the patch I've just send out? Apart from fixing a few issues
> I've made the context and sequence number parameters of the fence_array
> object.
> 
> This way you don't need to always allocate a new context for each object,
> but just enough to keep your timelines straight.
> 
> E.g. you don't get a lot of contexts only used once. This is at least
> sufficient for my amdgpu use case.

Well my idea behind NO_CONTEXT was that this way there'd be only one
special context for merged fences, which could have a linked list of all
fences ever (with debugging stuff). That way there's no need to allocate
one context per fence_array. It has the downside of a slightly leaky
abstraction, as in you must use the provided interface functions to figure
out whether a fence is on the same timeline, and if so, which one is
later.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/2] dma-buf/fence: add fence_array fences v4
  2016-05-20 13:53 [PATCH 1/2] drm/amd/powerplay: fix bugs of checking if dpm is running on Tonga Christian König
@ 2016-05-20 13:53 ` Christian König
  0 siblings, 0 replies; 10+ messages in thread
From: Christian König @ 2016-05-20 13:53 UTC (permalink / raw)
  To: dri-devel; +Cc: linux-kernel, daniel.vetter, gustavo

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.

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 | 132 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/fence-array.h   |  62 ++++++++++++++++++++
 3 files changed, 195 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..a700c6e
--- /dev/null
+++ b/drivers/dma-buf/fence-array.c
@@ -0,0 +1,132 @@
+/*
+ * 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 bool fence_array_add_next_callback(struct fence_array *array)
+{
+	while (array->num_signaled < array->num_fences) {
+		struct fence *next = array->fences[array->num_signaled];
+
+		if (!fence_add_callback(next, &array->cb, fence_array_cb_func))
+			return true;
+
+		++array->num_signaled;
+	}
+
+	return false;
+}
+
+static void fence_array_cb_func(struct fence *f, struct fence_cb *cb)
+{
+	struct fence_array *array = container_of(cb, struct fence_array, cb);
+
+	++array->num_signaled;
+	if (!fence_array_add_next_callback(array))
+		fence_signal(&array->base);
+}
+
+static bool fence_array_enable_signaling(struct fence *fence)
+{
+	struct fence_array *array = to_fence_array(fence);
+
+	return fence_array_add_next_callback(array);
+}
+
+static bool fence_array_signaled(struct fence *fence)
+{
+	struct fence_array *array = to_fence_array(fence);
+
+	return ACCESS_ONCE(array->num_signaled) == array->num_fences;
+}
+
+static void fence_array_release(struct fence *fence)
+{
+	struct fence_array *array = to_fence_array(fence);
+	unsigned i;
+
+	i = ACCESS_ONCE(array->num_signaled);
+	if (i < array->num_fences) {
+		struct fence *last = array->fences[i];
+
+		fence_remove_callback(last, &array->cb);
+	}
+
+	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
+ *
+ * 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)
+{
+	struct fence_array *array;
+	int i;
+
+	array = kzalloc(sizeof(*array), GFP_KERNEL);
+	if (!array)
+		return NULL;
+
+	spin_lock_init(&array->lock);
+	fence_init(&array->base, &fence_array_ops, &array->lock,
+		   fence_context_alloc(1), 0);
+
+	array->num_signaled = 0;
+	array->num_fences = 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..f1daeb3
--- /dev/null
+++ b/include/linux/fence-array.h
@@ -0,0 +1,62 @@
+/*
+ * 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 - fence to represent an array of fences
+ * @base: fence base class
+ * @lock: spinlock for fence handling
+ * @cb: fence callback structure for signaling
+ * @num_signaled: fences in the array already signaled
+ * @num_fences: number of fences in the array
+ * @fences: array of the fences
+ */
+struct fence_array {
+	struct fence base;
+
+	spinlock_t lock;
+	struct fence_cb cb;
+	unsigned num_signaled;
+	unsigned num_fences;
+	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);
+
+#endif /* __LINUX_FENCE_ARRAY_H */
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2016-05-23 14:00 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-20 13:56 [PATCH 1/2] dma-buf/fence: make fence context 64 bit v2 Christian König
2016-05-20 13:56 ` [PATCH 2/2] dma-buf/fence: add fence_array fences v4 Christian König
2016-05-20 14:42   ` Chris Wilson
2016-05-23  7:32     ` Christian König
2016-05-20 14:47   ` Gustavo Padovan
2016-05-20 17:53     ` Christian König
2016-05-23  7:41     ` Daniel Vetter
2016-05-23 11:29       ` Christian König
2016-05-23 14:00         ` Daniel Vetter
  -- strict thread matches above, loose matches on Subject: below --
2016-05-20 13:53 [PATCH 1/2] drm/amd/powerplay: fix bugs of checking if dpm is running on Tonga Christian König
2016-05-20 13:53 ` [PATCH 2/2] dma-buf/fence: add fence_array fences v4 Christian König

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).