dma-buf: make fence sequence numbers 64 bit v2

From: Christian König <[email protected]>

Lockless container implementation similar to a dma_fence_array, but with only two elements per node and automatic garbage collection.

v2: properly document dma_fence_chain_for_each, add dma_fence_chain_find_seqno,
drop prev reference during garbage collection if it's not a chain fence.
v3: use head and iterator for dma_fence_chain_for_each
v4: fix reference count in dma_fence_chain_enable_signaling

Signed-off-by Christian König <[email protected]>

---
  drivers/dma-buf/Makefile          |   3 +-
  drivers/dma-buf/dma-fence-chain.c | 241 ++++++++++++++++++++++++++++++
  include/linux/dma-fence-chain.h   |  81 ++++++++++
  3 files changed, 324 insertions(+), 1 deletion(-)

create mode 100644 drivers/dma-buf/dma-fence-chain.c create mode 100644 include/linux/dma-fence-chain.h

drivers/dma-buf/Makefile +2 -1

index 0913a6ccab5a..1f006e083eb9 100644
@@ -1,4 +1,5 @@
-obj-y := dma-buf.o dma-fence.o dma-fence-array.o reservation.o seqno-fence.o
+obj-y := dma-buf.o dma-fence.o dma-fence-array.o dma-fence-chain.o \
+	 reservation.o seqno-fence.o
  obj-$(CONFIG_SYNC_FILE)		+= sync_file.o
  obj-$(CONFIG_SW_SYNC)		+= sw_sync.o sync_debug.o
  obj-$(CONFIG_UDMABUF)		+= udmabuf.o

drivers/dma-buf/dma-fence-chain.c new file +241 -0

new file mode 100644
index 000000000000..0c5e3c902fa0
@@ -0,0 +1,241 @@
+/*
+ * fence-chain: chain fences together in a timeline
+ *
+ * Copyright (C) 2018 Advanced Micro Devices, Inc.
+ * Authors:
+ *	Christian König <[email protected]>
+ *
+ * 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/dma-fence-chain.h>
+
+static bool dma_fence_chain_enable_signaling(struct dma_fence *fence);
+
+/**
+ * dma_fence_chain_get_prev - use RCU to get a reference to the previous fence
+ * @chain: chain node to get the previous node from
+ *
+ * Use dma_fence_get_rcu_safe to get a reference to the previous fence of the
+ * chain node.
+ */
+static struct dma_fence *dma_fence_chain_get_prev(struct dma_fence_chain *chain)
+{
+	struct dma_fence *prev;
+
+	rcu_read_lock();
+	prev = dma_fence_get_rcu_safe(&chain->prev);
+	rcu_read_unlock();
+	return prev;
+}
+
+/**
+ * dma_fence_chain_walk - chain walking function
+ * @fence: current chain node
+ *
+ * Walk the chain to the next node. Returns the next fence or NULL if we are at
+ * the end of the chain. Garbage collects chain nodes which are already
+ * signaled.
+ */
+struct dma_fence *dma_fence_chain_walk(struct dma_fence *fence)
+{
+	struct dma_fence_chain *chain, *prev_chain;
+	struct dma_fence *prev, *replacement, *tmp;
+
+	chain = to_dma_fence_chain(fence);
+	if (!chain) {
+		dma_fence_put(fence);
+		return NULL;
+	}
+
+	while ((prev = dma_fence_chain_get_prev(chain))) {
+
+		prev_chain = to_dma_fence_chain(prev);
+		if (prev_chain) {
+			if (!dma_fence_is_signaled(prev_chain->fence))
+				break;
+
+			replacement = dma_fence_chain_get_prev(prev_chain);
+		} else {
+			if (!dma_fence_is_signaled(prev))
+				break;
+
+			replacement = NULL;
+		}
+
+		tmp = cmpxchg(&chain->prev, prev, replacement);
+		if (tmp == prev)
+			dma_fence_put(tmp);
+		else
+			dma_fence_put(replacement);
+		dma_fence_put(prev);
+	}
+
+	dma_fence_put(fence);
+	return prev;
+}
+EXPORT_SYMBOL(dma_fence_chain_walk);
+
+/**
+ * dma_fence_chain_find_seqno - find fence chain node by seqno
+ * @pfence: pointer to the chain node where to start
+ * @seqno: the sequence number to search for
+ *
+ * Advance the fence pointer to the chain node which will signal this sequence
+ * number. If no sequence number is provided then this is a no-op.
+ *
+ * Returns EINVAL if the fence is not a chain node or the sequence number has
+ * not yet advanced far enough.
+ */
+int dma_fence_chain_find_seqno(struct dma_fence **pfence, uint64_t seqno)
+{
+	struct dma_fence_chain *chain;
+
+	if (!seqno)
+		return 0;
+
+	chain = to_dma_fence_chain(*pfence);
+	if (!chain || chain->base.seqno < seqno)
+		return -EINVAL;
+
+	dma_fence_chain_for_each(*pfence, &chain->base) {
+		if ((*pfence)->context != chain->base.context ||
+		    to_dma_fence_chain(*pfence)->prev_seqno < seqno)
+			break;
+	}
+	dma_fence_put(&chain->base);
+
+	return 0;
+}
+EXPORT_SYMBOL(dma_fence_chain_find_seqno);
+
+static const char *dma_fence_chain_get_driver_name(struct dma_fence *fence)
+{
+        return "dma_fence_chain";
+}
+
+static const char *dma_fence_chain_get_timeline_name(struct dma_fence *fence)
+{
+        return "unbound";
+}
+
+static void dma_fence_chain_irq_work(struct irq_work *work)
+{
+	struct dma_fence_chain *chain;
+
+	chain = container_of(work, typeof(*chain), work);
+
+	/* Try to rearm the callback */
+	if (!dma_fence_chain_enable_signaling(&chain->base))
+		/* Ok, we are done. No more unsignaled fences left */
+		dma_fence_signal(&chain->base);
+	dma_fence_put(&chain->base);
+}
+
+static void dma_fence_chain_cb(struct dma_fence *f, struct dma_fence_cb *cb)
+{
+	struct dma_fence_chain *chain;
+
+	chain = container_of(cb, typeof(*chain), cb);
+	irq_work_queue(&chain->work);
+	dma_fence_put(f);
+}
+
+static bool dma_fence_chain_enable_signaling(struct dma_fence *fence)
+{
+	struct dma_fence_chain *head = to_dma_fence_chain(fence);
+
+	dma_fence_get(&head->base);
+	dma_fence_chain_for_each(fence, &head->base) {
+		struct dma_fence_chain *chain = to_dma_fence_chain(fence);
+		struct dma_fence *f = chain ? chain->fence : fence;
+
+		dma_fence_get(f);
+		if (!dma_fence_add_callback(f, &head->cb, dma_fence_chain_cb)) {
+			dma_fence_put(fence);
+			return true;
+		}
+		dma_fence_put(f);
+	}
+	dma_fence_put(&head->base);
+	return false;
+}
+
+static bool dma_fence_chain_signaled(struct dma_fence *fence)
+{
+	dma_fence_chain_for_each(fence, fence) {
+		struct dma_fence_chain *chain = to_dma_fence_chain(fence);
+		struct dma_fence *f = chain ? chain->fence : fence;
+
+		if (!dma_fence_is_signaled(f)) {
+			dma_fence_put(fence);
+			return false;
+		}
+	}
+
+	return true;
+}
+
+static void dma_fence_chain_release(struct dma_fence *fence)
+{
+	struct dma_fence_chain *chain = to_dma_fence_chain(fence);
+
+	dma_fence_put(chain->prev);
+	dma_fence_put(chain->fence);
+	dma_fence_free(fence);
+}
+
+const struct dma_fence_ops dma_fence_chain_ops = {
+	.get_driver_name = dma_fence_chain_get_driver_name,
+	.get_timeline_name = dma_fence_chain_get_timeline_name,
+	.enable_signaling = dma_fence_chain_enable_signaling,
+	.signaled = dma_fence_chain_signaled,
+	.release = dma_fence_chain_release,
+};
+EXPORT_SYMBOL(dma_fence_chain_ops);
+
+/**
+ * dma_fence_chain_init - initialize a fence chain
+ * @chain: the chain node to initialize
+ * @prev: the previous fence
+ * @fence: the current fence
+ *
+ * Initialize a new chain node and either start a new chain or add the node to
+ * the existing chain of the previous fence.
+ */
+void dma_fence_chain_init(struct dma_fence_chain *chain,
+			  struct dma_fence *prev,
+			  struct dma_fence *fence,
+			  uint64_t seqno)
+{
+	struct dma_fence_chain *prev_chain = to_dma_fence_chain(prev);
+	uint64_t context;
+
+	spin_lock_init(&chain->lock);
+	chain->prev = prev;
+	chain->fence = fence;
+	chain->prev_seqno = 0;
+	init_irq_work(&chain->work, dma_fence_chain_irq_work);
+
+	/* Try to reuse the context of the previous chain node. */
+	if (prev_chain && __dma_fence_is_later(seqno, prev->seqno)) {
+		context = prev->context;
+		chain->prev_seqno = prev->seqno;
+	} else {
+		context = dma_fence_context_alloc(1);
+		/* Make sure that we always have a valid sequence number. */
+		if (prev_chain)
+			seqno = max(prev->seqno, seqno);
+	}
+
+	dma_fence_init(&chain->base, &dma_fence_chain_ops,
+		       &chain->lock, context, seqno);
+}
+EXPORT_SYMBOL(dma_fence_chain_init);

include/linux/dma-fence-chain.h new file +71 -0

new file mode 100644
index 000000000000..a5c2e8c6915c
@@ -0,0 +1,81 @@
+/*
+ * fence-chain: chain fences together in a timeline
+ *
+ * Copyright (C) 2018 Advanced Micro Devices, Inc.
+ * Authors:
+ *	Christian König <[email protected]>
+ *
+ * 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_DMA_FENCE_CHAIN_H
+#define __LINUX_DMA_FENCE_CHAIN_H
+
+#include <linux/dma-fence.h>
+#include <linux/irq_work.h>
+
+/**
+ * struct dma_fence_chain - fence to represent an node of a fence chain
+ * @base: fence base class
+ * @lock: spinlock for fence handling
+ * @prev: previous fence of the chain
+ * @prev_seqno: original previous seqno before garbage collection
+ * @fence: encapsulated fence
+ * @cb: callback structure for signaling
+ * @work: irq work item for signaling
+ */
+struct dma_fence_chain {
+	struct dma_fence base;
+	spinlock_t lock;
+	struct dma_fence *prev;
+	u64 prev_seqno;
+	struct dma_fence *fence;
+	struct dma_fence_cb cb;
+	struct irq_work work;
+};
+
+extern const struct dma_fence_ops dma_fence_chain_ops;
+
+/**
+ * to_dma_fence_chain - cast a fence to a dma_fence_chain
+ * @fence: fence to cast to a dma_fence_array
+ *
+ * Returns NULL if the fence is not a dma_fence_chain,
+ * or the dma_fence_chain otherwise.
+ */
+static inline struct dma_fence_chain *
+to_dma_fence_chain(struct dma_fence *fence)
+{
+	if (!fence || fence->ops != &dma_fence_chain_ops)
+		return NULL;
+
+	return container_of(fence, struct dma_fence_chain, base);
+}
+
+/**
+ * dma_fence_chain_for_each - iterate over all fences in chain
+ * @iter: current fence
+ * @head: starting point
+ *
+ * Iterate over all fences in the chain. We keep a reference to the current
+ * fence while inside the loop which must be dropped when breaking out.
+ */
+#define dma_fence_chain_for_each(iter, head)	\
+	for (iter = dma_fence_get(head); iter; \

Hi Christian, David,

For timeline semaphore we need points to signaled in order. I'm struggling to understand how this fence-chain implementation preserves ordering of the seqnos.

One of the scenario I can see an issue happening is when you have a timeline with points 1 & 2 and userspace submits for 2 different engines :
    - first with let's say a blitter style engine on point 2
    - then a 3d style engine on point 1

Another scenario would be signaling a timeline with points 1 & 2 with those points in reverse order in the submission array.

-Lionel

Quoted 385 lines · Chunming Zhou

From: Christian König <[email protected]>

Lockless container implementation similar to a dma_fence_array, but with only two elements per node and automatic garbage collection.

v2: properly document dma_fence_chain_for_each, add dma_fence_chain_find_seqno,
     drop prev reference during garbage collection if it's not a chain fence.
v3: use head and iterator for dma_fence_chain_for_each
v4: fix reference count in dma_fence_chain_enable_signaling

Signed-off-by Christian König <[email protected]>

---
  drivers/dma-buf/Makefile          |   3 +- drivers/dma-buf/dma-fence-chain.c | 241 ++++++++++++++++++++++++++++++ include/linux/dma-fence-chain.h   |  81 ++++++++++

  3 files changed, 324 insertions(+), 1 deletion(-)

  create mode 100644 drivers/dma-buf/dma-fence-chain.c create mode 100644 include/linux/dma-fence-chain.h

drivers/dma-buf/Makefile +0 -1

index 0913a6ccab5a..1f006e083eb9 100644
@@ -1,4 +1,5 @@
-obj-y := dma-buf.o dma-fence.o dma-fence-array.o reservation.o 

seqno-fence.o
+obj-y := dma-buf.o dma-fence.o dma-fence-array.o dma-fence-chain.o \
+     reservation.o seqno-fence.o
  obj-$(CONFIG_SYNC_FILE)        += sync_file.o obj-$(CONFIG_SW_SYNC)        += sw_sync.o sync_debug.o obj-$(CONFIG_UDMABUF)        += udmabuf.o

drivers/dma-buf/dma-fence-chain.c new file +8 -0

new file mode 100644
index 000000000000..0c5e3c902fa0
@@ -0,0 +1,241 @@
+/*
+ * fence-chain: chain fences together in a timeline
+ *
+ * Copyright (C) 2018 Advanced Micro Devices, Inc.
+ * Authors:
+ *    Christian König <[email protected]>
+ *
+ * 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.
+ */

drivers/dma-buf/dma-fence-chain.c new file +7 -0

+
+#include <linux/dma-fence-chain.h>
+
+static bool dma_fence_chain_enable_signaling(struct dma_fence *fence);
+
+/**
+ * dma_fence_chain_get_prev - use RCU to get a reference to the 

previous fence
+ * @chain: chain node to get the previous node from
+ *
+ * Use dma_fence_get_rcu_safe to get a reference to the previous fence of the
+ * chain node.
+ */
+static struct dma_fence *dma_fence_chain_get_prev(struct dma_fence_chain *chain)

drivers/dma-buf/dma-fence-chain.c new file +14 -0

+{
+    struct dma_fence *prev;
+
+    rcu_read_lock();
+    prev = dma_fence_get_rcu_safe(&chain->prev);
+    rcu_read_unlock();
+    return prev;
+}
+
+/**
+ * dma_fence_chain_walk - chain walking function
+ * @fence: current chain node
+ *
+ * Walk the chain to the next node. Returns the next fence or NULL 

if we are at
+ * the end of the chain. Garbage collects chain nodes which are already
+ * signaled.
+ */

drivers/dma-buf/dma-fence-chain.c new file +45 -0

+struct dma_fence *dma_fence_chain_walk(struct dma_fence *fence)
+{
+    struct dma_fence_chain *chain, *prev_chain;
+    struct dma_fence *prev, *replacement, *tmp;
+
+    chain = to_dma_fence_chain(fence);
+    if (!chain) {
+        dma_fence_put(fence);
+        return NULL;
+    }
+
+    while ((prev = dma_fence_chain_get_prev(chain))) {
+
+        prev_chain = to_dma_fence_chain(prev);
+        if (prev_chain) {
+            if (!dma_fence_is_signaled(prev_chain->fence))
+                break;
+
+            replacement = dma_fence_chain_get_prev(prev_chain);
+        } else {
+            if (!dma_fence_is_signaled(prev))
+                break;
+
+            replacement = NULL;
+        }
+
+        tmp = cmpxchg(&chain->prev, prev, replacement);
+        if (tmp == prev)
+            dma_fence_put(tmp);
+        else
+            dma_fence_put(replacement);
+        dma_fence_put(prev);
+    }
+
+    dma_fence_put(fence);
+    return prev;
+}
+EXPORT_SYMBOL(dma_fence_chain_walk);
+
+/**
+ * dma_fence_chain_find_seqno - find fence chain node by seqno
+ * @pfence: pointer to the chain node where to start
+ * @seqno: the sequence number to search for
+ *
+ * Advance the fence pointer to the chain node which will signal 

this sequence
+ * number. If no sequence number is provided then this is a no-op.
+ *
+ * Returns EINVAL if the fence is not a chain node or the sequence number has
+ * not yet advanced far enough.
+ */
+int dma_fence_chain_find_seqno(struct dma_fence **pfence, uint64_t seqno)

drivers/dma-buf/dma-fence-chain.c new file +22 -0

+{
+    struct dma_fence_chain *chain;
+
+    if (!seqno)
+        return 0;
+
+    chain = to_dma_fence_chain(*pfence);
+    if (!chain || chain->base.seqno < seqno)
+        return -EINVAL;
+
+    dma_fence_chain_for_each(*pfence, &chain->base) {
+        if ((*pfence)->context != chain->base.context ||
+            to_dma_fence_chain(*pfence)->prev_seqno < seqno)
+            break;
+    }
+    dma_fence_put(&chain->base);
+
+    return 0;
+}
+EXPORT_SYMBOL(dma_fence_chain_find_seqno);
+
+static const char *dma_fence_chain_get_driver_name(struct dma_fence 

*fence)

drivers/dma-buf/dma-fence-chain.c new file +5 -0

+{
+        return "dma_fence_chain";
+}
+
+static const char *dma_fence_chain_get_timeline_name(struct 

dma_fence *fence)

drivers/dma-buf/dma-fence-chain.c new file +18 -0

+{
+        return "unbound";
+}
+
+static void dma_fence_chain_irq_work(struct irq_work *work)
+{
+    struct dma_fence_chain *chain;
+
+    chain = container_of(work, typeof(*chain), work);
+
+    /* Try to rearm the callback */
+    if (!dma_fence_chain_enable_signaling(&chain->base))
+        /* Ok, we are done. No more unsignaled fences left */
+        dma_fence_signal(&chain->base);
+    dma_fence_put(&chain->base);
+}
+
+static void dma_fence_chain_cb(struct dma_fence *f, struct 

dma_fence_cb *cb)

drivers/dma-buf/dma-fence-chain.c new file +19 -0

+{
+    struct dma_fence_chain *chain;
+
+    chain = container_of(cb, typeof(*chain), cb);
+    irq_work_queue(&chain->work);
+    dma_fence_put(f);
+}
+
+static bool dma_fence_chain_enable_signaling(struct dma_fence *fence)
+{
+    struct dma_fence_chain *head = to_dma_fence_chain(fence);
+
+    dma_fence_get(&head->base);
+    dma_fence_chain_for_each(fence, &head->base) {
+        struct dma_fence_chain *chain = to_dma_fence_chain(fence);
+        struct dma_fence *f = chain ? chain->fence : fence;
+
+        dma_fence_get(f);
+        if (!dma_fence_add_callback(f, &head->cb, 

dma_fence_chain_cb)) {
+            dma_fence_put(fence);
+            return true;
+        }
+        dma_fence_put(f);
+    }
+    dma_fence_put(&head->base);
+    return false;

drivers/dma-buf/dma-fence-chain.c new file +42 -0

+}
+
+static bool dma_fence_chain_signaled(struct dma_fence *fence)
+{
+    dma_fence_chain_for_each(fence, fence) {
+        struct dma_fence_chain *chain = to_dma_fence_chain(fence);
+        struct dma_fence *f = chain ? chain->fence : fence;
+
+        if (!dma_fence_is_signaled(f)) {
+            dma_fence_put(fence);
+            return false;
+        }
+    }
+
+    return true;
+}
+
+static void dma_fence_chain_release(struct dma_fence *fence)
+{
+    struct dma_fence_chain *chain = to_dma_fence_chain(fence);
+
+    dma_fence_put(chain->prev);
+    dma_fence_put(chain->fence);
+    dma_fence_free(fence);
+}
+
+const struct dma_fence_ops dma_fence_chain_ops = {
+    .get_driver_name = dma_fence_chain_get_driver_name,
+    .get_timeline_name = dma_fence_chain_get_timeline_name,
+    .enable_signaling = dma_fence_chain_enable_signaling,
+    .signaled = dma_fence_chain_signaled,
+    .release = dma_fence_chain_release,
+};
+EXPORT_SYMBOL(dma_fence_chain_ops);
+
+/**
+ * dma_fence_chain_init - initialize a fence chain
+ * @chain: the chain node to initialize
+ * @prev: the previous fence
+ * @fence: the current fence
+ *
+ * Initialize a new chain node and either start a new chain or add 

the node to
+ * the existing chain of the previous fence.
+ */

drivers/dma-buf/dma-fence-chain.c new file +29 -0

+void dma_fence_chain_init(struct dma_fence_chain *chain,
+              struct dma_fence *prev,
+              struct dma_fence *fence,
+              uint64_t seqno)
+{
+    struct dma_fence_chain *prev_chain = to_dma_fence_chain(prev);
+    uint64_t context;
+
+    spin_lock_init(&chain->lock);
+    chain->prev = prev;
+    chain->fence = fence;
+    chain->prev_seqno = 0;
+    init_irq_work(&chain->work, dma_fence_chain_irq_work);
+
+    /* Try to reuse the context of the previous chain node. */
+    if (prev_chain && __dma_fence_is_later(seqno, prev->seqno)) {
+        context = prev->context;
+        chain->prev_seqno = prev->seqno;
+    } else {
+        context = dma_fence_context_alloc(1);
+        /* Make sure that we always have a valid sequence number. */
+        if (prev_chain)
+            seqno = max(prev->seqno, seqno);
+    }
+
+    dma_fence_init(&chain->base, &dma_fence_chain_ops,
+               &chain->lock, context, seqno);
+}
+EXPORT_SYMBOL(dma_fence_chain_init);

include/linux/dma-fence-chain.h new file +8 -0

new file mode 100644
index 000000000000..a5c2e8c6915c
@@ -0,0 +1,81 @@
+/*
+ * fence-chain: chain fences together in a timeline
+ *
+ * Copyright (C) 2018 Advanced Micro Devices, Inc.
+ * Authors:
+ *    Christian König <[email protected]>
+ *
+ * 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/dma-fence-chain.h new file +51 -0

+
+#ifndef __LINUX_DMA_FENCE_CHAIN_H
+#define __LINUX_DMA_FENCE_CHAIN_H
+
+#include <linux/dma-fence.h>
+#include <linux/irq_work.h>
+
+/**
+ * struct dma_fence_chain - fence to represent an node of a fence chain
+ * @base: fence base class
+ * @lock: spinlock for fence handling
+ * @prev: previous fence of the chain
+ * @prev_seqno: original previous seqno before garbage collection
+ * @fence: encapsulated fence
+ * @cb: callback structure for signaling
+ * @work: irq work item for signaling
+ */
+struct dma_fence_chain {
+    struct dma_fence base;
+    spinlock_t lock;
+    struct dma_fence *prev;
+    u64 prev_seqno;
+    struct dma_fence *fence;
+    struct dma_fence_cb cb;
+    struct irq_work work;
+};
+
+extern const struct dma_fence_ops dma_fence_chain_ops;
+
+/**
+ * to_dma_fence_chain - cast a fence to a dma_fence_chain
+ * @fence: fence to cast to a dma_fence_array
+ *
+ * Returns NULL if the fence is not a dma_fence_chain,
+ * or the dma_fence_chain otherwise.
+ */
+static inline struct dma_fence_chain *
+to_dma_fence_chain(struct dma_fence *fence)
+{
+    if (!fence || fence->ops != &dma_fence_chain_ops)
+        return NULL;
+
+    return container_of(fence, struct dma_fence_chain, base);
+}
+
+/**
+ * dma_fence_chain_for_each - iterate over all fences in chain
+ * @iter: current fence
+ * @head: starting point
+ *
+ * Iterate over all fences in the chain. We keep a reference to the 

current
+ * fence while inside the loop which must be dropped when breaking out.
+ */

include/linux/dma-fence-chain.h new file +5 -0

+#define dma_fence_chain_for_each(iter, head)    \
+    for (iter = dma_fence_get(head); iter; \
+         iter = dma_fence_chain_walk(head))
+
+struct dma_fence *dma_fence_chain_walk(struct dma_fence *fence);

include/linux/dma-fence-chain.h line 81

Quoted 8 lines

+int dma_fence_chain_find_seqno(struct dma_fence **pfence, uint64_t seqno);

include/linux/dma-fence-chain.h new file +6 -0

+void dma_fence_chain_init(struct dma_fence_chain *chain,
+              struct dma_fence *prev,
+              struct dma_fence *fence,
+              uint64_t seqno);
+
+#endif /* __LINUX_DMA_FENCE_CHAIN_H */

Am 15.02.19 um 15:23 schrieb Lionel Landwerlin:

Quoted 11 lines

Hi Christian, David,

For timeline semaphore we need points to signaled in order. I'm struggling to understand how this fence-chain implementation preserves ordering of the seqnos.

One of the scenario I can see an issue happening is when you have a timeline with points 1 & 2 and userspace submits for 2 different engines :
    - first with let's say a blitter style engine on point 2
    - then a 3d style engine on point 1

Yeah, and where exactly is the problem?

Seqno 1 will signal when the 3d style engine finishes work.

And seqno 2 will signal when both seqno 1 is signaled and the blitter style engine has finished its work.

Quoted 3 lines

Another scenario would be signaling a timeline with points 1 & 2 with those points in reverse order in the submission array.

That is actually illegal in the spec, but actually handled gracefully as well.

E.g. when you add seqno 1 to the syncobj container it will only signal when 2 is signaled as well.

Regards,
Christian.

Quoted 397 lines

-Lionel

Quoted 393 lines · Chunming Zhou

From: Christian König <[email protected]>

Lockless container implementation similar to a dma_fence_array, but with only two elements per node and automatic garbage collection.

v2: properly document dma_fence_chain_for_each, add dma_fence_chain_find_seqno,
     drop prev reference during garbage collection if it's not a
chain fence.
v3: use head and iterator for dma_fence_chain_for_each
v4: fix reference count in dma_fence_chain_enable_signaling

Signed-off-by Christian König <[email protected]>

---
   drivers/dma-buf/Makefile          |   3 +-
   drivers/dma-buf/dma-fence-chain.c | 241 ++++++++++++++++++++++++++++++
   include/linux/dma-fence-chain.h   |  81 ++++++++++
   3 files changed, 324 insertions(+), 1 deletion(-)

  create mode 100644 drivers/dma-buf/dma-fence-chain.c create mode 100644 include/linux/dma-fence-chain.h

drivers/dma-buf/Makefile +0 -1

index 0913a6ccab5a..1f006e083eb9 100644
@@ -1,4 +1,5 @@
-obj-y := dma-buf.o dma-fence.o dma-fence-array.o reservation.o

seqno-fence.o

drivers/dma-buf/Makefile +2 -0

+obj-y := dma-buf.o dma-fence.o dma-fence-array.o dma-fence-chain.o \
+     reservation.o seqno-fence.o
   obj-$(CONFIG_SYNC_FILE)        += sync_file.o
   obj-$(CONFIG_SW_SYNC)        += sw_sync.o sync_debug.o
   obj-$(CONFIG_UDMABUF)        += udmabuf.o

drivers/dma-buf/dma-fence-chain.c new file +8 -0

new file mode 100644
index 000000000000..0c5e3c902fa0
@@ -0,0 +1,241 @@
+/*
+ * fence-chain: chain fences together in a timeline
+ *
+ * Copyright (C) 2018 Advanced Micro Devices, Inc.
+ * Authors:
+ *    Christian König <[email protected]>
+ *
+ * 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.
+ */

drivers/dma-buf/dma-fence-chain.c new file +7 -0

+
+#include <linux/dma-fence-chain.h>
+
+static bool dma_fence_chain_enable_signaling(struct dma_fence *fence);
+
+/**
+ * dma_fence_chain_get_prev - use RCU to get a reference to the

previous fence
+ * @chain: chain node to get the previous node from
+ *
+ * Use dma_fence_get_rcu_safe to get a reference to the previous fence of the
+ * chain node.
+ */
+static struct dma_fence *dma_fence_chain_get_prev(struct dma_fence_chain *chain)

drivers/dma-buf/dma-fence-chain.c new file +14 -0

+{
+    struct dma_fence *prev;
+
+    rcu_read_lock();
+    prev = dma_fence_get_rcu_safe(&chain->prev);
+    rcu_read_unlock();
+    return prev;
+}
+
+/**
+ * dma_fence_chain_walk - chain walking function
+ * @fence: current chain node
+ *
+ * Walk the chain to the next node. Returns the next fence or NULL

if we are at
+ * the end of the chain. Garbage collects chain nodes which are already
+ * signaled.
+ */

drivers/dma-buf/dma-fence-chain.c new file +45 -0

+struct dma_fence *dma_fence_chain_walk(struct dma_fence *fence)
+{
+    struct dma_fence_chain *chain, *prev_chain;
+    struct dma_fence *prev, *replacement, *tmp;
+
+    chain = to_dma_fence_chain(fence);
+    if (!chain) {
+        dma_fence_put(fence);
+        return NULL;
+    }
+
+    while ((prev = dma_fence_chain_get_prev(chain))) {
+
+        prev_chain = to_dma_fence_chain(prev);
+        if (prev_chain) {
+            if (!dma_fence_is_signaled(prev_chain->fence))
+                break;
+
+            replacement = dma_fence_chain_get_prev(prev_chain);
+        } else {
+            if (!dma_fence_is_signaled(prev))
+                break;
+
+            replacement = NULL;
+        }
+
+        tmp = cmpxchg(&chain->prev, prev, replacement);
+        if (tmp == prev)
+            dma_fence_put(tmp);
+        else
+            dma_fence_put(replacement);
+        dma_fence_put(prev);
+    }
+
+    dma_fence_put(fence);
+    return prev;
+}
+EXPORT_SYMBOL(dma_fence_chain_walk);
+
+/**
+ * dma_fence_chain_find_seqno - find fence chain node by seqno
+ * @pfence: pointer to the chain node where to start
+ * @seqno: the sequence number to search for
+ *
+ * Advance the fence pointer to the chain node which will signal

this sequence
+ * number. If no sequence number is provided then this is a no-op.
+ *
+ * Returns EINVAL if the fence is not a chain node or the sequence number has
+ * not yet advanced far enough.
+ */
+int dma_fence_chain_find_seqno(struct dma_fence **pfence, uint64_t seqno)

drivers/dma-buf/dma-fence-chain.c new file +22 -0

+{
+    struct dma_fence_chain *chain;
+
+    if (!seqno)
+        return 0;
+
+    chain = to_dma_fence_chain(*pfence);
+    if (!chain || chain->base.seqno < seqno)
+        return -EINVAL;
+
+    dma_fence_chain_for_each(*pfence, &chain->base) {
+        if ((*pfence)->context != chain->base.context ||
+            to_dma_fence_chain(*pfence)->prev_seqno < seqno)
+            break;
+    }
+    dma_fence_put(&chain->base);
+
+    return 0;
+}
+EXPORT_SYMBOL(dma_fence_chain_find_seqno);
+
+static const char *dma_fence_chain_get_driver_name(struct dma_fence

*fence)

drivers/dma-buf/dma-fence-chain.c new file +5 -0

+{
+        return "dma_fence_chain";
+}
+
+static const char *dma_fence_chain_get_timeline_name(struct

dma_fence *fence)

drivers/dma-buf/dma-fence-chain.c new file +18 -0

+{
+        return "unbound";
+}
+
+static void dma_fence_chain_irq_work(struct irq_work *work)
+{
+    struct dma_fence_chain *chain;
+
+    chain = container_of(work, typeof(*chain), work);
+
+    /* Try to rearm the callback */
+    if (!dma_fence_chain_enable_signaling(&chain->base))
+        /* Ok, we are done. No more unsignaled fences left */
+        dma_fence_signal(&chain->base);
+    dma_fence_put(&chain->base);
+}
+
+static void dma_fence_chain_cb(struct dma_fence *f, struct

dma_fence_cb *cb)

drivers/dma-buf/dma-fence-chain.c new file +19 -0

+{
+    struct dma_fence_chain *chain;
+
+    chain = container_of(cb, typeof(*chain), cb);
+    irq_work_queue(&chain->work);
+    dma_fence_put(f);
+}
+
+static bool dma_fence_chain_enable_signaling(struct dma_fence *fence)
+{
+    struct dma_fence_chain *head = to_dma_fence_chain(fence);
+
+    dma_fence_get(&head->base);
+    dma_fence_chain_for_each(fence, &head->base) {
+        struct dma_fence_chain *chain = to_dma_fence_chain(fence);
+        struct dma_fence *f = chain ? chain->fence : fence;
+
+        dma_fence_get(f);
+        if (!dma_fence_add_callback(f, &head->cb,

dma_fence_chain_cb)) {
+            dma_fence_put(fence);
+            return true;
+        }
+        dma_fence_put(f);
+    }
+    dma_fence_put(&head->base);
+    return false;

drivers/dma-buf/dma-fence-chain.c new file +42 -0

+}
+
+static bool dma_fence_chain_signaled(struct dma_fence *fence)
+{
+    dma_fence_chain_for_each(fence, fence) {
+        struct dma_fence_chain *chain = to_dma_fence_chain(fence);
+        struct dma_fence *f = chain ? chain->fence : fence;
+
+        if (!dma_fence_is_signaled(f)) {
+            dma_fence_put(fence);
+            return false;
+        }
+    }
+
+    return true;
+}
+
+static void dma_fence_chain_release(struct dma_fence *fence)
+{
+    struct dma_fence_chain *chain = to_dma_fence_chain(fence);
+
+    dma_fence_put(chain->prev);
+    dma_fence_put(chain->fence);
+    dma_fence_free(fence);
+}
+
+const struct dma_fence_ops dma_fence_chain_ops = {
+    .get_driver_name = dma_fence_chain_get_driver_name,
+    .get_timeline_name = dma_fence_chain_get_timeline_name,
+    .enable_signaling = dma_fence_chain_enable_signaling,
+    .signaled = dma_fence_chain_signaled,
+    .release = dma_fence_chain_release,
+};
+EXPORT_SYMBOL(dma_fence_chain_ops);
+
+/**
+ * dma_fence_chain_init - initialize a fence chain
+ * @chain: the chain node to initialize
+ * @prev: the previous fence
+ * @fence: the current fence
+ *
+ * Initialize a new chain node and either start a new chain or add

the node to
+ * the existing chain of the previous fence.
+ */

drivers/dma-buf/dma-fence-chain.c new file +29 -0

+void dma_fence_chain_init(struct dma_fence_chain *chain,
+              struct dma_fence *prev,
+              struct dma_fence *fence,
+              uint64_t seqno)
+{
+    struct dma_fence_chain *prev_chain = to_dma_fence_chain(prev);
+    uint64_t context;
+
+    spin_lock_init(&chain->lock);
+    chain->prev = prev;
+    chain->fence = fence;
+    chain->prev_seqno = 0;
+    init_irq_work(&chain->work, dma_fence_chain_irq_work);
+
+    /* Try to reuse the context of the previous chain node. */
+    if (prev_chain && __dma_fence_is_later(seqno, prev->seqno)) {
+        context = prev->context;
+        chain->prev_seqno = prev->seqno;
+    } else {
+        context = dma_fence_context_alloc(1);
+        /* Make sure that we always have a valid sequence number. */
+        if (prev_chain)
+            seqno = max(prev->seqno, seqno);
+    }
+
+    dma_fence_init(&chain->base, &dma_fence_chain_ops,
+               &chain->lock, context, seqno);
+}
+EXPORT_SYMBOL(dma_fence_chain_init);

include/linux/dma-fence-chain.h new file +8 -0

new file mode 100644
index 000000000000..a5c2e8c6915c
@@ -0,0 +1,81 @@
+/*
+ * fence-chain: chain fences together in a timeline
+ *
+ * Copyright (C) 2018 Advanced Micro Devices, Inc.
+ * Authors:
+ *    Christian König <[email protected]>
+ *
+ * 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/dma-fence-chain.h new file +51 -0

+
+#ifndef __LINUX_DMA_FENCE_CHAIN_H
+#define __LINUX_DMA_FENCE_CHAIN_H
+
+#include <linux/dma-fence.h>
+#include <linux/irq_work.h>
+
+/**
+ * struct dma_fence_chain - fence to represent an node of a fence chain
+ * @base: fence base class
+ * @lock: spinlock for fence handling
+ * @prev: previous fence of the chain
+ * @prev_seqno: original previous seqno before garbage collection
+ * @fence: encapsulated fence
+ * @cb: callback structure for signaling
+ * @work: irq work item for signaling
+ */
+struct dma_fence_chain {
+    struct dma_fence base;
+    spinlock_t lock;
+    struct dma_fence *prev;
+    u64 prev_seqno;
+    struct dma_fence *fence;
+    struct dma_fence_cb cb;
+    struct irq_work work;
+};
+
+extern const struct dma_fence_ops dma_fence_chain_ops;
+
+/**
+ * to_dma_fence_chain - cast a fence to a dma_fence_chain
+ * @fence: fence to cast to a dma_fence_array
+ *
+ * Returns NULL if the fence is not a dma_fence_chain,
+ * or the dma_fence_chain otherwise.
+ */
+static inline struct dma_fence_chain *
+to_dma_fence_chain(struct dma_fence *fence)
+{
+    if (!fence || fence->ops != &dma_fence_chain_ops)
+        return NULL;
+
+    return container_of(fence, struct dma_fence_chain, base);
+}
+
+/**
+ * dma_fence_chain_for_each - iterate over all fences in chain
+ * @iter: current fence
+ * @head: starting point
+ *
+ * Iterate over all fences in the chain. We keep a reference to the

current
+ * fence while inside the loop which must be dropped when breaking out.
+ */

include/linux/dma-fence-chain.h new file +6 -0

+#define dma_fence_chain_for_each(iter, head)    \
+    for (iter = dma_fence_get(head); iter; \
+         iter = dma_fence_chain_walk(head))
+
+struct dma_fence *dma_fence_chain_walk(struct dma_fence *fence);
+int dma_fence_chain_find_seqno(struct dma_fence **pfence, uint64_t

seqno);

include/linux/dma-fence-chain.h new file +6 -0

+void dma_fence_chain_init(struct dma_fence_chain *chain,
+              struct dma_fence *prev,
+              struct dma_fence *fence,
+              uint64_t seqno);
+
+#endif /* __LINUX_DMA_FENCE_CHAIN_H */

Quoted 12 lines

Am 15.02.19 um 15:23 schrieb Lionel Landwerlin:

Quoted 11 lines

Hi Christian, David,

For timeline semaphore we need points to signaled in order. I'm struggling to understand how this fence-chain implementation preserves ordering of the seqnos.

One of the scenario I can see an issue happening is when you have a timeline with points 1 & 2 and userspace submits for 2 different engines :
     - first with let's say a blitter style engine on point 2
     - then a 3d style engine on point 1

Quoted 6 lines

Yeah, and where exactly is the problem?

Seqno 1 will signal when the 3d style engine finishes work.

And seqno 2 will signal when both seqno 1 is signaled and the blitter style engine has finished its work.

That's not really how I understood the spec, but I might be wrong.

What makes me thing 1 should be signaled as soon as 2 is signaled (regardless of whether the fence attached on point 1 is been signaled), is that the spec defines wait & signal operations in term of the value of the timeline.

-Lionel

Quoted 418 lines

Quoted 3 lines

Another scenario would be signaling a timeline with points 1 & 2 with those points in reverse order in the submission array.

That is actually illegal in the spec, but actually handled gracefully as well.

E.g. when you add seqno 1 to the syncobj container it will only signal when 2 is signaled as well.

Regards,
Christian.

Quoted 402 lines

-Lionel

Quoted 398 lines · Chunming Zhou

From: Christian König <[email protected]>

Lockless container implementation similar to a dma_fence_array, but with only two elements per node and automatic garbage collection.

v2: properly document dma_fence_chain_for_each, add dma_fence_chain_find_seqno,
      drop prev reference during garbage collection if it's not a chain fence.
v3: use head and iterator for dma_fence_chain_for_each
v4: fix reference count in dma_fence_chain_enable_signaling

Signed-off-by Christian König <[email protected]>

---
   drivers/dma-buf/Makefile          |   3 +- drivers/dma-buf/dma-fence-chain.c | 241
++++++++++++++++++++++++++++++
   include/linux/dma-fence-chain.h   |  81 ++++++++++

   3 files changed, 324 insertions(+), 1 deletion(-)

   create mode 100644 drivers/dma-buf/dma-fence-chain.c create mode 100644 include/linux/dma-fence-chain.h

drivers/dma-buf/Makefile +0 -1

index 0913a6ccab5a..1f006e083eb9 100644
@@ -1,4 +1,5 @@
-obj-y := dma-buf.o dma-fence.o dma-fence-array.o reservation.o

seqno-fence.o
+obj-y := dma-buf.o dma-fence.o dma-fence-array.o dma-fence-chain.o \
+     reservation.o seqno-fence.o
   obj-$(CONFIG_SYNC_FILE)        += sync_file.o obj-$(CONFIG_SW_SYNC)        += sw_sync.o sync_debug.o obj-$(CONFIG_UDMABUF)        += udmabuf.o

drivers/dma-buf/dma-fence-chain.c new file +8 -0

new file mode 100644
index 000000000000..0c5e3c902fa0
@@ -0,0 +1,241 @@
+/*
+ * fence-chain: chain fences together in a timeline
+ *
+ * Copyright (C) 2018 Advanced Micro Devices, Inc.
+ * Authors:
+ *    Christian König <[email protected]>
+ *
+ * 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.
+ */

drivers/dma-buf/dma-fence-chain.c new file +4 -0

+
+#include <linux/dma-fence-chain.h>
+
+static bool dma_fence_chain_enable_signaling(struct dma_fence 

*fence);

drivers/dma-buf/dma-fence-chain.c new file +3 -0

+
+/**
+ * dma_fence_chain_get_prev - use RCU to get a reference to the

previous fence
+ * @chain: chain node to get the previous node from
+ *
+ * Use dma_fence_get_rcu_safe to get a reference to the previous fence of the
+ * chain node.
+ */
+static struct dma_fence *dma_fence_chain_get_prev(struct dma_fence_chain *chain)

drivers/dma-buf/dma-fence-chain.c new file +14 -0

+{
+    struct dma_fence *prev;
+
+    rcu_read_lock();
+    prev = dma_fence_get_rcu_safe(&chain->prev);
+    rcu_read_unlock();
+    return prev;
+}
+
+/**
+ * dma_fence_chain_walk - chain walking function
+ * @fence: current chain node
+ *
+ * Walk the chain to the next node. Returns the next fence or NULL

if we are at
+ * the end of the chain. Garbage collects chain nodes which are already
+ * signaled.
+ */

drivers/dma-buf/dma-fence-chain.c new file +45 -0

+struct dma_fence *dma_fence_chain_walk(struct dma_fence *fence)
+{
+    struct dma_fence_chain *chain, *prev_chain;
+    struct dma_fence *prev, *replacement, *tmp;
+
+    chain = to_dma_fence_chain(fence);
+    if (!chain) {
+        dma_fence_put(fence);
+        return NULL;
+    }
+
+    while ((prev = dma_fence_chain_get_prev(chain))) {
+
+        prev_chain = to_dma_fence_chain(prev);
+        if (prev_chain) {
+            if (!dma_fence_is_signaled(prev_chain->fence))
+                break;
+
+            replacement = dma_fence_chain_get_prev(prev_chain);
+        } else {
+            if (!dma_fence_is_signaled(prev))
+                break;
+
+            replacement = NULL;
+        }
+
+        tmp = cmpxchg(&chain->prev, prev, replacement);
+        if (tmp == prev)
+            dma_fence_put(tmp);
+        else
+            dma_fence_put(replacement);
+        dma_fence_put(prev);
+    }
+
+    dma_fence_put(fence);
+    return prev;
+}
+EXPORT_SYMBOL(dma_fence_chain_walk);
+
+/**
+ * dma_fence_chain_find_seqno - find fence chain node by seqno
+ * @pfence: pointer to the chain node where to start
+ * @seqno: the sequence number to search for
+ *
+ * Advance the fence pointer to the chain node which will signal

this sequence
+ * number. If no sequence number is provided then this is a no-op.
+ *
+ * Returns EINVAL if the fence is not a chain node or the sequence number has
+ * not yet advanced far enough.
+ */
+int dma_fence_chain_find_seqno(struct dma_fence **pfence, uint64_t seqno)

drivers/dma-buf/dma-fence-chain.c new file +22 -0

+{
+    struct dma_fence_chain *chain;
+
+    if (!seqno)
+        return 0;
+
+    chain = to_dma_fence_chain(*pfence);
+    if (!chain || chain->base.seqno < seqno)
+        return -EINVAL;
+
+    dma_fence_chain_for_each(*pfence, &chain->base) {
+        if ((*pfence)->context != chain->base.context ||
+            to_dma_fence_chain(*pfence)->prev_seqno < seqno)
+            break;
+    }
+    dma_fence_put(&chain->base);
+
+    return 0;
+}
+EXPORT_SYMBOL(dma_fence_chain_find_seqno);
+
+static const char *dma_fence_chain_get_driver_name(struct dma_fence

*fence)

drivers/dma-buf/dma-fence-chain.c new file +5 -0

+{
+        return "dma_fence_chain";
+}
+
+static const char *dma_fence_chain_get_timeline_name(struct

dma_fence *fence)

drivers/dma-buf/dma-fence-chain.c new file +18 -0

+{
+        return "unbound";
+}
+
+static void dma_fence_chain_irq_work(struct irq_work *work)
+{
+    struct dma_fence_chain *chain;
+
+    chain = container_of(work, typeof(*chain), work);
+
+    /* Try to rearm the callback */
+    if (!dma_fence_chain_enable_signaling(&chain->base))
+        /* Ok, we are done. No more unsignaled fences left */
+        dma_fence_signal(&chain->base);
+    dma_fence_put(&chain->base);
+}
+
+static void dma_fence_chain_cb(struct dma_fence *f, struct

dma_fence_cb *cb)

drivers/dma-buf/dma-fence-chain.c new file +19 -0

+{
+    struct dma_fence_chain *chain;
+
+    chain = container_of(cb, typeof(*chain), cb);
+    irq_work_queue(&chain->work);
+    dma_fence_put(f);
+}
+
+static bool dma_fence_chain_enable_signaling(struct dma_fence *fence)
+{
+    struct dma_fence_chain *head = to_dma_fence_chain(fence);
+
+    dma_fence_get(&head->base);
+    dma_fence_chain_for_each(fence, &head->base) {
+        struct dma_fence_chain *chain = to_dma_fence_chain(fence);
+        struct dma_fence *f = chain ? chain->fence : fence;
+
+        dma_fence_get(f);
+        if (!dma_fence_add_callback(f, &head->cb,

dma_fence_chain_cb)) {
+            dma_fence_put(fence);
+            return true;
+        }
+        dma_fence_put(f);
+    }
+    dma_fence_put(&head->base);
+    return false;

drivers/dma-buf/dma-fence-chain.c new file +42 -0

+}
+
+static bool dma_fence_chain_signaled(struct dma_fence *fence)
+{
+    dma_fence_chain_for_each(fence, fence) {
+        struct dma_fence_chain *chain = to_dma_fence_chain(fence);
+        struct dma_fence *f = chain ? chain->fence : fence;
+
+        if (!dma_fence_is_signaled(f)) {
+            dma_fence_put(fence);
+            return false;
+        }
+    }
+
+    return true;
+}
+
+static void dma_fence_chain_release(struct dma_fence *fence)
+{
+    struct dma_fence_chain *chain = to_dma_fence_chain(fence);
+
+    dma_fence_put(chain->prev);
+    dma_fence_put(chain->fence);
+    dma_fence_free(fence);
+}
+
+const struct dma_fence_ops dma_fence_chain_ops = {
+    .get_driver_name = dma_fence_chain_get_driver_name,
+    .get_timeline_name = dma_fence_chain_get_timeline_name,
+    .enable_signaling = dma_fence_chain_enable_signaling,
+    .signaled = dma_fence_chain_signaled,
+    .release = dma_fence_chain_release,
+};
+EXPORT_SYMBOL(dma_fence_chain_ops);
+
+/**
+ * dma_fence_chain_init - initialize a fence chain
+ * @chain: the chain node to initialize
+ * @prev: the previous fence
+ * @fence: the current fence
+ *
+ * Initialize a new chain node and either start a new chain or add

the node to
+ * the existing chain of the previous fence.
+ */

drivers/dma-buf/dma-fence-chain.c new file +29 -0

+void dma_fence_chain_init(struct dma_fence_chain *chain,
+              struct dma_fence *prev,
+              struct dma_fence *fence,
+              uint64_t seqno)
+{
+    struct dma_fence_chain *prev_chain = to_dma_fence_chain(prev);
+    uint64_t context;
+
+    spin_lock_init(&chain->lock);
+    chain->prev = prev;
+    chain->fence = fence;
+    chain->prev_seqno = 0;
+    init_irq_work(&chain->work, dma_fence_chain_irq_work);
+
+    /* Try to reuse the context of the previous chain node. */
+    if (prev_chain && __dma_fence_is_later(seqno, prev->seqno)) {
+        context = prev->context;
+        chain->prev_seqno = prev->seqno;
+    } else {
+        context = dma_fence_context_alloc(1);
+        /* Make sure that we always have a valid sequence number. */
+        if (prev_chain)
+            seqno = max(prev->seqno, seqno);
+    }
+
+    dma_fence_init(&chain->base, &dma_fence_chain_ops,
+               &chain->lock, context, seqno);
+}
+EXPORT_SYMBOL(dma_fence_chain_init);

include/linux/dma-fence-chain.h new file +8 -0

new file mode 100644
index 000000000000..a5c2e8c6915c
@@ -0,0 +1,81 @@
+/*
+ * fence-chain: chain fences together in a timeline
+ *
+ * Copyright (C) 2018 Advanced Micro Devices, Inc.
+ * Authors:
+ *    Christian König <[email protected]>
+ *
+ * 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/dma-fence-chain.h new file +9 -0

+
+#ifndef __LINUX_DMA_FENCE_CHAIN_H
+#define __LINUX_DMA_FENCE_CHAIN_H
+
+#include <linux/dma-fence.h>
+#include <linux/irq_work.h>
+
+/**
+ * struct dma_fence_chain - fence to represent an node of a fence 

chain
+ * @base: fence base class
+ * @lock: spinlock for fence handling
+ * @prev: previous fence of the chain
+ * @prev_seqno: original previous seqno before garbage collection
+ * @fence: encapsulated fence
+ * @cb: callback structure for signaling
+ * @work: irq work item for signaling
+ */

include/linux/dma-fence-chain.h new file +34 -0

+struct dma_fence_chain {
+    struct dma_fence base;
+    spinlock_t lock;
+    struct dma_fence *prev;
+    u64 prev_seqno;
+    struct dma_fence *fence;
+    struct dma_fence_cb cb;
+    struct irq_work work;
+};
+
+extern const struct dma_fence_ops dma_fence_chain_ops;
+
+/**
+ * to_dma_fence_chain - cast a fence to a dma_fence_chain
+ * @fence: fence to cast to a dma_fence_array
+ *
+ * Returns NULL if the fence is not a dma_fence_chain,
+ * or the dma_fence_chain otherwise.
+ */
+static inline struct dma_fence_chain *
+to_dma_fence_chain(struct dma_fence *fence)
+{
+    if (!fence || fence->ops != &dma_fence_chain_ops)
+        return NULL;
+
+    return container_of(fence, struct dma_fence_chain, base);
+}
+
+/**
+ * dma_fence_chain_for_each - iterate over all fences in chain
+ * @iter: current fence
+ * @head: starting point
+ *
+ * Iterate over all fences in the chain. We keep a reference to the

current
+ * fence while inside the loop which must be dropped when breaking out.
+ */

include/linux/dma-fence-chain.h new file +6 -0

+#define dma_fence_chain_for_each(iter, head)    \
+    for (iter = dma_fence_get(head); iter; \
+         iter = dma_fence_chain_walk(head))
+
+struct dma_fence *dma_fence_chain_walk(struct dma_fence *fence);
+int dma_fence_chain_find_seqno(struct dma_fence **pfence, uint64_t

seqno);

include/linux/dma-fence-chain.h new file +6 -0

+void dma_fence_chain_init(struct dma_fence_chain *chain,
+              struct dma_fence *prev,
+              struct dma_fence *fence,
+              uint64_t seqno);
+
+#endif /* __LINUX_DMA_FENCE_CHAIN_H */

Quoted 12 lines · Christian

Am 15.02.19 um 15:23 schrieb Lionel Landwerlin:

Quoted 11 lines

Hi Christian, David,

For timeline semaphore we need points to signaled in order. I'm struggling to understand how this fence-chain implementation preserves ordering of the seqnos.

One of the scenario I can see an issue happening is when you have a timeline with points 1 & 2 and userspace submits for 2 different engines :
- first with let's say a blitter style engine on point 2
- then a 3d style engine on point 1

Quoted 6 lines

Yeah, and where exactly is the problem?

Seqno 1 will signal when the 3d style engine finishes work.

And seqno 2 will signal when both seqno 1 is signaled and the blitter style engine has finished its work.

That's not really how I understood the spec, but I might be wrong.

What makes me thing 1 should be signaled as soon as 2 is signaled (regardless of whether the fence attached on point 1 is been signaled), is that the spec defines wait & signal operations in term of the value of the timeline.

-Lionel

Quoted 8 lines

Quoted 3 lines

Another scenario would be signaling a timeline with points 1 & 2 with those points in reverse order in the submission array.

That is actually illegal in the spec, but actually handled gracefully as well.

E.g. when you add seqno 1 to the syncobj container it will only signal when 2 is signaled as well.

Quoted 10 lines

Regards,
Christian.

Quoted 6 lines

-Lionel

Quoted 3 lines · Chunming Zhou

From: Christian König <[email protected]>

Lockless container implementation similar to a dma_fence_array, but

with

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

only two elements per node and automatic garbage collection.

v2: properly document dma_fence_chain_for_each, add dma_fence_chain_find_seqno,
drop prev reference during garbage collection if it's not a
chain fence.
v3: use head and iterator for dma_fence_chain_for_each
v4: fix reference count in dma_fence_chain_enable_signaling

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Signed-off-by Christian König <[email protected]>

---
   drivers/dma-buf/Makefile          |   3 +-
   drivers/dma-buf/dma-fence-chain.c | 241

++++++++++++++++++++++++++++++

Quoted 69 lines

Quoted 69 lines

Quoted 69 lines

   include/linux/dma-fence-chain.h   |  81 ++++++++++
   3 files changed, 324 insertions(+), 1 deletion(-)

create mode 100644 drivers/dma-buf/dma-fence-chain.c create mode 100644 include/linux/dma-fence-chain.h

drivers/dma-buf/Makefile +0 -1

index 0913a6ccab5a..1f006e083eb9 100644
@@ -1,4 +1,5 @@
-obj-y := dma-buf.o dma-fence.o dma-fence-array.o reservation.o

seqno-fence.o

drivers/dma-buf/Makefile +2 -0

+obj-y := dma-buf.o dma-fence.o dma-fence-array.o dma-fence-chain.o \
+     reservation.o seqno-fence.o
   obj-$(CONFIG_SYNC_FILE)        += sync_file.o
   obj-$(CONFIG_SW_SYNC)        += sw_sync.o sync_debug.o
   obj-$(CONFIG_UDMABUF)        += udmabuf.o

drivers/dma-buf/dma-fence-chain.c new file +8 -0

new file mode 100644
index 000000000000..0c5e3c902fa0
@@ -0,0 +1,241 @@
+/*
+ * fence-chain: chain fences together in a timeline
+ *
+ * Copyright (C) 2018 Advanced Micro Devices, Inc.
+ * Authors:
+ *    Christian König <[email protected]>
+ *
+ * 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.
+ */

drivers/dma-buf/dma-fence-chain.c new file +7 -0

+
+#include <linux/dma-fence-chain.h>
+
+static bool dma_fence_chain_enable_signaling(struct dma_fence *fence);
+
+/**
+ * dma_fence_chain_get_prev - use RCU to get a reference to the

previous fence
+ * @chain: chain node to get the previous node from
+ *
+ * Use dma_fence_get_rcu_safe to get a reference to the previous fence of the
+ * chain node.
+ */
+static struct dma_fence *dma_fence_chain_get_prev(struct dma_fence_chain *chain)

drivers/dma-buf/dma-fence-chain.c new file +8 -0

+{
+    struct dma_fence *prev;
+
+    rcu_read_lock();
+    prev = dma_fence_get_rcu_safe(&chain->prev);
+    rcu_read_unlock();
+    return prev;
+}

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/dma-buf/dma-fence-chain.c new file +6 -0

+
+/**
+ * dma_fence_chain_walk - chain walking function
+ * @fence: current chain node
+ *
+ * Walk the chain to the next node. Returns the next fence or NULL

if we are at
+ * the end of the chain. Garbage collects chain nodes which are

already

Quoted 234 lines

Quoted 234 lines

Quoted 234 lines

+ * signaled.
+ */

drivers/dma-buf/dma-fence-chain.c new file +45 -0

+struct dma_fence *dma_fence_chain_walk(struct dma_fence *fence)
+{
+    struct dma_fence_chain *chain, *prev_chain;
+    struct dma_fence *prev, *replacement, *tmp;
+
+    chain = to_dma_fence_chain(fence);
+    if (!chain) {
+        dma_fence_put(fence);
+        return NULL;
+    }
+
+    while ((prev = dma_fence_chain_get_prev(chain))) {
+
+        prev_chain = to_dma_fence_chain(prev);
+        if (prev_chain) {
+            if (!dma_fence_is_signaled(prev_chain->fence))
+                break;
+
+            replacement = dma_fence_chain_get_prev(prev_chain);
+        } else {
+            if (!dma_fence_is_signaled(prev))
+                break;
+
+            replacement = NULL;
+        }
+
+        tmp = cmpxchg(&chain->prev, prev, replacement);
+        if (tmp == prev)
+            dma_fence_put(tmp);
+        else
+            dma_fence_put(replacement);
+        dma_fence_put(prev);
+    }
+
+    dma_fence_put(fence);
+    return prev;
+}
+EXPORT_SYMBOL(dma_fence_chain_walk);
+
+/**
+ * dma_fence_chain_find_seqno - find fence chain node by seqno
+ * @pfence: pointer to the chain node where to start
+ * @seqno: the sequence number to search for
+ *
+ * Advance the fence pointer to the chain node which will signal

this sequence
+ * number. If no sequence number is provided then this is a no-op.
+ *
+ * Returns EINVAL if the fence is not a chain node or the sequence number has
+ * not yet advanced far enough.
+ */
+int dma_fence_chain_find_seqno(struct dma_fence **pfence, uint64_t seqno)

drivers/dma-buf/dma-fence-chain.c new file +22 -0

+{
+    struct dma_fence_chain *chain;
+
+    if (!seqno)
+        return 0;
+
+    chain = to_dma_fence_chain(*pfence);
+    if (!chain || chain->base.seqno < seqno)
+        return -EINVAL;
+
+    dma_fence_chain_for_each(*pfence, &chain->base) {
+        if ((*pfence)->context != chain->base.context ||
+            to_dma_fence_chain(*pfence)->prev_seqno < seqno)
+            break;
+    }
+    dma_fence_put(&chain->base);
+
+    return 0;
+}
+EXPORT_SYMBOL(dma_fence_chain_find_seqno);
+
+static const char *dma_fence_chain_get_driver_name(struct dma_fence

*fence)

drivers/dma-buf/dma-fence-chain.c new file +5 -0

+{
+        return "dma_fence_chain";
+}
+
+static const char *dma_fence_chain_get_timeline_name(struct

dma_fence *fence)

drivers/dma-buf/dma-fence-chain.c new file +18 -0

+{
+        return "unbound";
+}
+
+static void dma_fence_chain_irq_work(struct irq_work *work)
+{
+    struct dma_fence_chain *chain;
+
+    chain = container_of(work, typeof(*chain), work);
+
+    /* Try to rearm the callback */
+    if (!dma_fence_chain_enable_signaling(&chain->base))
+        /* Ok, we are done. No more unsignaled fences left */
+        dma_fence_signal(&chain->base);
+    dma_fence_put(&chain->base);
+}
+
+static void dma_fence_chain_cb(struct dma_fence *f, struct

dma_fence_cb *cb)

drivers/dma-buf/dma-fence-chain.c new file +19 -0

+{
+    struct dma_fence_chain *chain;
+
+    chain = container_of(cb, typeof(*chain), cb);
+    irq_work_queue(&chain->work);
+    dma_fence_put(f);
+}
+
+static bool dma_fence_chain_enable_signaling(struct dma_fence *fence)
+{
+    struct dma_fence_chain *head = to_dma_fence_chain(fence);
+
+    dma_fence_get(&head->base);
+    dma_fence_chain_for_each(fence, &head->base) {
+        struct dma_fence_chain *chain = to_dma_fence_chain(fence);
+        struct dma_fence *f = chain ? chain->fence : fence;
+
+        dma_fence_get(f);
+        if (!dma_fence_add_callback(f, &head->cb,

dma_fence_chain_cb)) {

drivers/dma-buf/dma-fence-chain.c new file +49 -0

+            dma_fence_put(fence);
+            return true;
+        }
+        dma_fence_put(f);
+    }
+    dma_fence_put(&head->base);
+    return false;
+}
+
+static bool dma_fence_chain_signaled(struct dma_fence *fence)
+{
+    dma_fence_chain_for_each(fence, fence) {
+        struct dma_fence_chain *chain = to_dma_fence_chain(fence);
+        struct dma_fence *f = chain ? chain->fence : fence;
+
+        if (!dma_fence_is_signaled(f)) {
+            dma_fence_put(fence);
+            return false;
+        }
+    }
+
+    return true;
+}
+
+static void dma_fence_chain_release(struct dma_fence *fence)
+{
+    struct dma_fence_chain *chain = to_dma_fence_chain(fence);
+
+    dma_fence_put(chain->prev);
+    dma_fence_put(chain->fence);
+    dma_fence_free(fence);
+}
+
+const struct dma_fence_ops dma_fence_chain_ops = {
+    .get_driver_name = dma_fence_chain_get_driver_name,
+    .get_timeline_name = dma_fence_chain_get_timeline_name,
+    .enable_signaling = dma_fence_chain_enable_signaling,
+    .signaled = dma_fence_chain_signaled,
+    .release = dma_fence_chain_release,
+};
+EXPORT_SYMBOL(dma_fence_chain_ops);
+
+/**
+ * dma_fence_chain_init - initialize a fence chain
+ * @chain: the chain node to initialize
+ * @prev: the previous fence
+ * @fence: the current fence
+ *
+ * Initialize a new chain node and either start a new chain or add

the node to
+ * the existing chain of the previous fence.
+ */

drivers/dma-buf/dma-fence-chain.c new file +29 -0

+void dma_fence_chain_init(struct dma_fence_chain *chain,
+              struct dma_fence *prev,
+              struct dma_fence *fence,
+              uint64_t seqno)
+{
+    struct dma_fence_chain *prev_chain = to_dma_fence_chain(prev);
+    uint64_t context;
+
+    spin_lock_init(&chain->lock);
+    chain->prev = prev;
+    chain->fence = fence;
+    chain->prev_seqno = 0;
+    init_irq_work(&chain->work, dma_fence_chain_irq_work);
+
+    /* Try to reuse the context of the previous chain node. */
+    if (prev_chain && __dma_fence_is_later(seqno, prev->seqno)) {
+        context = prev->context;
+        chain->prev_seqno = prev->seqno;
+    } else {
+        context = dma_fence_context_alloc(1);
+        /* Make sure that we always have a valid sequence number. */
+        if (prev_chain)
+            seqno = max(prev->seqno, seqno);
+    }
+
+    dma_fence_init(&chain->base, &dma_fence_chain_ops,
+               &chain->lock, context, seqno);
+}
+EXPORT_SYMBOL(dma_fence_chain_init);

include/linux/dma-fence-chain.h new file +8 -0

new file mode 100644
index 000000000000..a5c2e8c6915c
@@ -0,0 +1,81 @@
+/*
+ * fence-chain: chain fences together in a timeline
+ *
+ * Copyright (C) 2018 Advanced Micro Devices, Inc.
+ * Authors:
+ *    Christian König <[email protected]>
+ *
+ * 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.
+ */
+

Quoted 8 lines

Quoted 8 lines

include/linux/dma-fence-chain.h

Quoted 8 lines

include/linux/dma-fence-chain.h new file +8 -0

+#ifndef __LINUX_DMA_FENCE_CHAIN_H
+#define __LINUX_DMA_FENCE_CHAIN_H
+
+#include <linux/dma-fence.h>
+#include <linux/irq_work.h>
+
+/**
+ * struct dma_fence_chain - fence to represent an node of a fence

chain

Quoted 36 lines

Quoted 36 lines

Quoted 36 lines

+ * @base: fence base class
+ * @lock: spinlock for fence handling
+ * @prev: previous fence of the chain
+ * @prev_seqno: original previous seqno before garbage collection
+ * @fence: encapsulated fence
+ * @cb: callback structure for signaling
+ * @work: irq work item for signaling
+ */

include/linux/dma-fence-chain.h new file +28 -0

+struct dma_fence_chain {
+    struct dma_fence base;
+    spinlock_t lock;
+    struct dma_fence *prev;
+    u64 prev_seqno;
+    struct dma_fence *fence;
+    struct dma_fence_cb cb;
+    struct irq_work work;
+};
+
+extern const struct dma_fence_ops dma_fence_chain_ops;
+
+/**
+ * to_dma_fence_chain - cast a fence to a dma_fence_chain
+ * @fence: fence to cast to a dma_fence_array
+ *
+ * Returns NULL if the fence is not a dma_fence_chain,
+ * or the dma_fence_chain otherwise.
+ */
+static inline struct dma_fence_chain *
+to_dma_fence_chain(struct dma_fence *fence)
+{
+    if (!fence || fence->ops != &dma_fence_chain_ops)
+        return NULL;
+
+    return container_of(fence, struct dma_fence_chain, base);
+}
+

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

+/**
+ * dma_fence_chain_for_each - iterate over all fences in chain
+ * @iter: current fence
+ * @head: starting point
+ *
+ * Iterate over all fences in the chain. We keep a reference to the current
+ * fence while inside the loop which must be dropped when breaking

out.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

+ */

include/linux/dma-fence-chain.h new file +6 -0

+#define dma_fence_chain_for_each(iter, head)    \
+    for (iter = dma_fence_get(head); iter; \
+         iter = dma_fence_chain_walk(head))
+
+struct dma_fence *dma_fence_chain_walk(struct dma_fence *fence);
+int dma_fence_chain_find_seqno(struct dma_fence **pfence, uint64_t

Quoted 8 lines

Quoted 8 lines

include/linux/dma-fence-chain.h line 81

Quoted 7 lines

seqno);

include/linux/dma-fence-chain.h new file +6 -0

+void dma_fence_chain_init(struct dma_fence_chain *chain,
+              struct dma_fence *prev,
+              struct dma_fence *fence,
+              uint64_t seqno);
+
+#endif /* __LINUX_DMA_FENCE_CHAIN_H */

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

On 15/02/2019 14:32, Koenig, Christian wrote:
> Am 15.02.19 um 15:23 schrieb Lionel Landwerlin:
>> Hi Christian, David,
>>
>> For timeline semaphore we need points to signaled in order. >> I'm struggling to understand how this fence-chain implementation >> preserves ordering of the seqnos.
>>
>> One of the scenario I can see an issue happening is when you have a >> timeline with points 1 & 2 and userspace submits for 2 different >> engines :
>>      - first with let's say a blitter style engine on point 2 >>      - then a 3d style engine on point 1
> Yeah, and where exactly is the problem?
>

> Seqno 1 will signal when the 3d style engine finishes work. >
> And seqno 2 will signal when both seqno 1 is signaled and the blitter
> style engine has finished its work.

That's an interesting interpretation of the spec.  I think it's legal and I could see that behavior may be desirable in some ways.

I think what Christian is suggesting is a valid interpretation of the spec though it is rather unconventional.  The Vulkan spec, as it stands today, requires that the application ensure that at the time of signaling, the timeline semaphore value increases.  This means that all of the above possible cases are technically illegal in Vulkan and so it doesn't really matter what we do as long as we don't do anyting especially stupid.

My understanding of how this works on Windows is that a wait operation on 3 is a wait until x >= 3 where x is a 64-bit value and a signal operation is simply a write to x. This means that, in the above cases, waits on 1 will be triggered immediately when 2 is written but waits on 2 may or may not happen at all depending on whether the GPU write which overwrites x to 1 or the CPU (or potentially GPU in a different context) read gets there first such that the reader observes 2.  If you mess this up and something isn't signaled, that's your fault.

 4. If you do get into a sticky situation, you can unblock an entire timeline by using the CPU signal ioctl to set it to a high value.

Of all these reasons, I think 1 and 2 carry the most weight.  2, in particular, is interesting if we one day want to implement the same behavior with a simple 64-bit value like Windows does.  Immagine, for instance, a scenario where the GPU is doing it's own scheduling or command buffers are submitted ahead of the signal operation being available and told to just sit on the GPU until they see x >= 3.  (Yes, there are issues here with residency, contention, etc.  I'm asking you to use your immagination.)  Assuming you can do 64-bit atomics (there are aparently issues here with PCIe that make things sticky), the behavior I'm suggesting is completely implementable in that way whereas the behavior Christian is suggesting is only implementable if you're maintaining a CPU-side list of fences.  I don't think we want to paint ourselves into that corner.

--Jason

Quoted 8 lines

Regards,
Christian.

Quoted 4 lines

-Lionel

Quoted 1 line · Chunming Zhou

From: Christian König <[email protected]

    <mailto:[email protected]>>
    >>>
    >>> Lockless container implementation similar to a
    dma_fence_array, but with
    >>> only two elements per node and automatic garbage collection.
    >>>
    >>> v2: properly document dma_fence_chain_for_each, add
    >>> dma_fence_chain_find_seqno,
    >>>       drop prev reference during garbage collection if it's not a
    >>> chain fence.
    >>> v3: use head and iterator for dma_fence_chain_for_each
    >>> v4: fix reference count in dma_fence_chain_enable_signaling
    >>>
    >>> Signed-off-by: Christian König <[email protected]
    <mailto:[email protected]>>
    >>> ---
    >>>    drivers/dma-buf/Makefile          |   3 +-
    >>>    drivers/dma-buf/dma-fence-chain.c | 241
    ++++++++++++++++++++++++++++++
    >>>    include/linux/dma-fence-chain.h   |  81 ++++++++++
    >>>    3 files changed, 324 insertions(+), 1 deletion(-)
    >>>    create mode 100644 drivers/dma-buf/dma-fence-chain.c
    >>>    create mode 100644 include/linux/dma-fence-chain.h
    >>>
    >>> diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile
    >>> index 0913a6ccab5a..1f006e083eb9 100644
    >>> --- a/drivers/dma-buf/Makefile
    >>> +++ b/drivers/dma-buf/Makefile
    >>> @@ -1,4 +1,5 @@
    >>> -obj-y := dma-buf.o dma-fence.o dma-fence-array.o reservation.o
    >>> seqno-fence.o
    >>> +obj-y := dma-buf.o dma-fence.o dma-fence-array.o
    dma-fence-chain.o \
    >>> +     reservation.o seqno-fence.o
    >>>    obj-$(CONFIG_SYNC_FILE)        += sync_file.o
    >>>    obj-$(CONFIG_SW_SYNC)        += sw_sync.o sync_debug.o
    >>>    obj-$(CONFIG_UDMABUF)        += udmabuf.o
    >>> diff --git a/drivers/dma-buf/dma-fence-chain.c
    >>> b/drivers/dma-buf/dma-fence-chain.c
    >>> new file mode 100644
    >>> index 000000000000..0c5e3c902fa0
    >>> --- /dev/null
    >>> +++ b/drivers/dma-buf/dma-fence-chain.c
    >>> @@ -0,0 +1,241 @@
    >>> +/*
    >>> + * fence-chain: chain fences together in a timeline
    >>> + *
    >>> + * Copyright (C) 2018 Advanced Micro Devices, Inc.
    >>> + * Authors:
    >>> + *    Christian König <[email protected]
    <mailto:[email protected]>>
    >>> + *
    >>> + * 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/dma-fence-chain.h>
    >>> +
    >>> +static bool dma_fence_chain_enable_signaling(struct dma_fence
    *fence);
    >>> +
    >>> +/**
    >>> + * dma_fence_chain_get_prev - use RCU to get a reference to the
    >>> previous fence
    >>> + * @chain: chain node to get the previous node from
    >>> + *
    >>> + * Use dma_fence_get_rcu_safe to get a reference to the previous
    >>> fence of the
    >>> + * chain node.
    >>> + */
    >>> +static struct dma_fence *dma_fence_chain_get_prev(struct
    >>> dma_fence_chain *chain)
    >>> +{
    >>> +    struct dma_fence *prev;
    >>> +
    >>> +    rcu_read_lock();
    >>> +    prev = dma_fence_get_rcu_safe(&chain->prev);
    >>> +    rcu_read_unlock();
    >>> +    return prev;
    >>> +}
    >>> +
    >>> +/**
    >>> + * dma_fence_chain_walk - chain walking function
    >>> + * @fence: current chain node
    >>> + *
    >>> + * Walk the chain to the next node. Returns the next fence or
    NULL
    >>> if we are at
    >>> + * the end of the chain. Garbage collects chain nodes which
    are already
    >>> + * signaled.
    >>> + */
    >>> +struct dma_fence *dma_fence_chain_walk(struct dma_fence *fence)
    >>> +{
    >>> +    struct dma_fence_chain *chain, *prev_chain;
    >>> +    struct dma_fence *prev, *replacement, *tmp;
    >>> +
    >>> +    chain = to_dma_fence_chain(fence);
    >>> +    if (!chain) {
    >>> +        dma_fence_put(fence);
    >>> +        return NULL;
    >>> +    }
    >>> +
    >>> +    while ((prev = dma_fence_chain_get_prev(chain))) {
    >>> +
    >>> +        prev_chain = to_dma_fence_chain(prev);
    >>> +        if (prev_chain) {
    >>> +            if (!dma_fence_is_signaled(prev_chain->fence))
    >>> +                break;
    >>> +
    >>> +            replacement = dma_fence_chain_get_prev(prev_chain);
    >>> +        } else {
    >>> +            if (!dma_fence_is_signaled(prev))
    >>> +                break;
    >>> +
    >>> +            replacement = NULL;
    >>> +        }
    >>> +
    >>> +        tmp = cmpxchg(&chain->prev, prev, replacement);
    >>> +        if (tmp == prev)
    >>> +            dma_fence_put(tmp);
    >>> +        else
    >>> +            dma_fence_put(replacement);
    >>> +        dma_fence_put(prev);
    >>> +    }
    >>> +
    >>> +    dma_fence_put(fence);
    >>> +    return prev;
    >>> +}
    >>> +EXPORT_SYMBOL(dma_fence_chain_walk);
    >>> +
    >>> +/**
    >>> + * dma_fence_chain_find_seqno - find fence chain node by seqno
    >>> + * @pfence: pointer to the chain node where to start
    >>> + * @seqno: the sequence number to search for
    >>> + *
    >>> + * Advance the fence pointer to the chain node which will signal
    >>> this sequence
    >>> + * number. If no sequence number is provided then this is a
    no-op.
    >>> + *
    >>> + * Returns EINVAL if the fence is not a chain node or the
    sequence
    >>> number has
    >>> + * not yet advanced far enough.
    >>> + */
    >>> +int dma_fence_chain_find_seqno(struct dma_fence **pfence,
    uint64_t
    >>> seqno)
    >>> +{
    >>> +    struct dma_fence_chain *chain;
    >>> +
    >>> +    if (!seqno)
    >>> +        return 0;
    >>> +
    >>> +    chain = to_dma_fence_chain(*pfence);
    >>> +    if (!chain || chain->base.seqno < seqno)
    >>> +        return -EINVAL;
    >>> +
    >>> +    dma_fence_chain_for_each(*pfence, &chain->base) {
    >>> +        if ((*pfence)->context != chain->base.context ||
    >>> + to_dma_fence_chain(*pfence)->prev_seqno < seqno)
    >>> +            break;
    >>> +    }
    >>> +    dma_fence_put(&chain->base);
    >>> +
    >>> +    return 0;
    >>> +}
    >>> +EXPORT_SYMBOL(dma_fence_chain_find_seqno);
    >>> +
    >>> +static const char *dma_fence_chain_get_driver_name(struct
    dma_fence
    >>> *fence)
    >>> +{
    >>> +        return "dma_fence_chain";
    >>> +}
    >>> +
    >>> +static const char *dma_fence_chain_get_timeline_name(struct
    >>> dma_fence *fence)
    >>> +{
    >>> +        return "unbound";
    >>> +}
    >>> +
    >>> +static void dma_fence_chain_irq_work(struct irq_work *work)
    >>> +{
    >>> +    struct dma_fence_chain *chain;
    >>> +
    >>> +    chain = container_of(work, typeof(*chain), work);
    >>> +
    >>> +    /* Try to rearm the callback */
    >>> +    if (!dma_fence_chain_enable_signaling(&chain->base))
    >>> +        /* Ok, we are done. No more unsignaled fences left */
    >>> +        dma_fence_signal(&chain->base);
    >>> +    dma_fence_put(&chain->base);
    >>> +}
    >>> +
    >>> +static void dma_fence_chain_cb(struct dma_fence *f, struct
    >>> dma_fence_cb *cb)
    >>> +{
    >>> +    struct dma_fence_chain *chain;
    >>> +
    >>> +    chain = container_of(cb, typeof(*chain), cb);
    >>> +    irq_work_queue(&chain->work);
    >>> +    dma_fence_put(f);
    >>> +}
    >>> +
    >>> +static bool dma_fence_chain_enable_signaling(struct dma_fence
    *fence)
    >>> +{
    >>> +    struct dma_fence_chain *head = to_dma_fence_chain(fence);
    >>> +
    >>> +    dma_fence_get(&head->base);
    >>> +    dma_fence_chain_for_each(fence, &head->base) {
    >>> +        struct dma_fence_chain *chain =
    to_dma_fence_chain(fence);
    >>> +        struct dma_fence *f = chain ? chain->fence : fence;
    >>> +
    >>> +        dma_fence_get(f);
    >>> +        if (!dma_fence_add_callback(f, &head->cb,
    >>> dma_fence_chain_cb)) {
    >>> +            dma_fence_put(fence);
    >>> +            return true;
    >>> +        }
    >>> +        dma_fence_put(f);
    >>> +    }
    >>> +    dma_fence_put(&head->base);
    >>> +    return false;
    >>> +}
    >>> +
    >>> +static bool dma_fence_chain_signaled(struct dma_fence *fence)
    >>> +{
    >>> +    dma_fence_chain_for_each(fence, fence) {
    >>> +        struct dma_fence_chain *chain =
    to_dma_fence_chain(fence);
    >>> +        struct dma_fence *f = chain ? chain->fence : fence;
    >>> +
    >>> +        if (!dma_fence_is_signaled(f)) {
    >>> +            dma_fence_put(fence);
    >>> +            return false;
    >>> +        }
    >>> +    }
    >>> +
    >>> +    return true;
    >>> +}
    >>> +
    >>> +static void dma_fence_chain_release(struct dma_fence *fence)
    >>> +{
    >>> +    struct dma_fence_chain *chain = to_dma_fence_chain(fence);
    >>> +
    >>> +    dma_fence_put(chain->prev);
    >>> +    dma_fence_put(chain->fence);
    >>> +    dma_fence_free(fence);
    >>> +}
    >>> +
    >>> +const struct dma_fence_ops dma_fence_chain_ops = {
    >>> +    .get_driver_name = dma_fence_chain_get_driver_name,
    >>> +    .get_timeline_name = dma_fence_chain_get_timeline_name,
    >>> +    .enable_signaling = dma_fence_chain_enable_signaling,
    >>> +    .signaled = dma_fence_chain_signaled,
    >>> +    .release = dma_fence_chain_release,
    >>> +};
    >>> +EXPORT_SYMBOL(dma_fence_chain_ops);
    >>> +
    >>> +/**
    >>> + * dma_fence_chain_init - initialize a fence chain
    >>> + * @chain: the chain node to initialize
    >>> + * @prev: the previous fence
    >>> + * @fence: the current fence
    >>> + *
    >>> + * Initialize a new chain node and either start a new chain
    or add
    >>> the node to
    >>> + * the existing chain of the previous fence.
    >>> + */
    >>> +void dma_fence_chain_init(struct dma_fence_chain *chain,
    >>> +              struct dma_fence *prev,
    >>> +              struct dma_fence *fence,
    >>> +              uint64_t seqno)
    >>> +{
    >>> +    struct dma_fence_chain *prev_chain =
    to_dma_fence_chain(prev);
    >>> +    uint64_t context;
    >>> +
    >>> +    spin_lock_init(&chain->lock);
    >>> +    chain->prev = prev;
    >>> +    chain->fence = fence;
    >>> +    chain->prev_seqno = 0;
    >>> +    init_irq_work(&chain->work, dma_fence_chain_irq_work);
    >>> +
    >>> +    /* Try to reuse the context of the previous chain node. */
    >>> +    if (prev_chain && __dma_fence_is_later(seqno, prev->seqno)) {
    >>> +        context = prev->context;
    >>> +        chain->prev_seqno = prev->seqno;
    >>> +    } else {
    >>> +        context = dma_fence_context_alloc(1);
    >>> +        /* Make sure that we always have a valid sequence
    number. */
    >>> +        if (prev_chain)
    >>> +            seqno = max(prev->seqno, seqno);
    >>> +    }
    >>> +
    >>> +    dma_fence_init(&chain->base, &dma_fence_chain_ops,
    >>> +               &chain->lock, context, seqno);
    >>> +}
    >>> +EXPORT_SYMBOL(dma_fence_chain_init);
    >>> diff --git a/include/linux/dma-fence-chain.h
    >>> b/include/linux/dma-fence-chain.h
    >>> new file mode 100644
    >>> index 000000000000..a5c2e8c6915c
    >>> --- /dev/null
    >>> +++ b/include/linux/dma-fence-chain.h
    >>> @@ -0,0 +1,81 @@
    >>> +/*
    >>> + * fence-chain: chain fences together in a timeline
    >>> + *
    >>> + * Copyright (C) 2018 Advanced Micro Devices, Inc.
    >>> + * Authors:
    >>> + *    Christian König <[email protected]
    <mailto:[email protected]>>
    >>> + *
    >>> + * 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_DMA_FENCE_CHAIN_H
    >>> +#define __LINUX_DMA_FENCE_CHAIN_H
    >>> +
    >>> +#include <linux/dma-fence.h>
    >>> +#include <linux/irq_work.h>
    >>> +
    >>> +/**
    >>> + * struct dma_fence_chain - fence to represent an node of a
    fence chain
    >>> + * @base: fence base class
    >>> + * @lock: spinlock for fence handling
    >>> + * @prev: previous fence of the chain
    >>> + * @prev_seqno: original previous seqno before garbage collection
    >>> + * @fence: encapsulated fence
    >>> + * @cb: callback structure for signaling
    >>> + * @work: irq work item for signaling
    >>> + */
    >>> +struct dma_fence_chain {
    >>> +    struct dma_fence base;
    >>> +    spinlock_t lock;
    >>> +    struct dma_fence *prev;
    >>> +    u64 prev_seqno;
    >>> +    struct dma_fence *fence;
    >>> +    struct dma_fence_cb cb;
    >>> +    struct irq_work work;
    >>> +};
    >>> +
    >>> +extern const struct dma_fence_ops dma_fence_chain_ops;
    >>> +
    >>> +/**
    >>> + * to_dma_fence_chain - cast a fence to a dma_fence_chain
    >>> + * @fence: fence to cast to a dma_fence_array
    >>> + *
    >>> + * Returns NULL if the fence is not a dma_fence_chain,
    >>> + * or the dma_fence_chain otherwise.
    >>> + */
    >>> +static inline struct dma_fence_chain *
    >>> +to_dma_fence_chain(struct dma_fence *fence)
    >>> +{
    >>> +    if (!fence || fence->ops != &dma_fence_chain_ops)
    >>> +        return NULL;
    >>> +
    >>> +    return container_of(fence, struct dma_fence_chain, base);
    >>> +}
    >>> +
    >>> +/**
    >>> + * dma_fence_chain_for_each - iterate over all fences in chain
    >>> + * @iter: current fence
    >>> + * @head: starting point
    >>> + *
    >>> + * Iterate over all fences in the chain. We keep a reference
    to the
    >>> current
    >>> + * fence while inside the loop which must be dropped when
    breaking out.
    >>> + */
    >>> +#define dma_fence_chain_for_each(iter, head)    \
    >>> +    for (iter = dma_fence_get(head); iter; \
    >>> +         iter = dma_fence_chain_walk(head))
    >>> +
    >>> +struct dma_fence *dma_fence_chain_walk(struct dma_fence *fence);
    >>> +int dma_fence_chain_find_seqno(struct dma_fence **pfence,
    uint64_t
    >>> seqno);
    >>> +void dma_fence_chain_init(struct dma_fence_chain *chain,
    >>> +              struct dma_fence *prev,
    >>> +              struct dma_fence *fence,
    >>> +              uint64_t seqno);
    >>> +

    >>> +#endif /* __LINUX_DMA_FENCE_CHAIN_H */
    >>

    _______________________________________________
    dri-devel mailing list
    [email protected]
    <mailto:[email protected]>
    https://lists.freedesktop.org/mailman/listinfo/dri-devel

Am 15.02.19 um 17:49 schrieb Jason Ekstrand:

Quoted 13 lines · Lionel Landwerlin via dri-devel

Quoted 12 lines · Christian

Am 15.02.19 um 15:23 schrieb Lionel Landwerlin:

Quoted 11 lines

Hi Christian, David,

For timeline semaphore we need points to signaled in order. I'm struggling to understand how this fence-chain implementation preserves ordering of the seqnos.

One of the scenario I can see an issue happening is when you have a timeline with points 1 & 2 and userspace submits for 2 different engines :
- first with let's say a blitter style engine on point 2
- then a 3d style engine on point 1

Quoted 7 lines

Quoted 6 lines

Yeah, and where exactly is the problem?

Seqno 1 will signal when the 3d style engine finishes work.

And seqno 2 will signal when both seqno 1 is signaled and the blitter style engine has finished its work.

That's an interesting interpretation of the spec. I think it's legal and I could see that behavior may be desirable in some ways.

Well we actually had this discussion multiple times now, both internally as well as on the mailing list. Please also see the previous mails with Daniel on this topic.

My initial suggestion was actually to exactly what Leonid suggested as well.

And following this I used a rather simple container for the implementation, e.g. just a ring buffer indexed by the sequence number. In this scenario userspace can specify on syncobj creation time how big the window for sequence numbers should be, e.g. in this implementation how big the ring buffer would be.

This was rejected by our guys who actually wrote a good part of the Vulkan specification. Daniel then has gone into the same direction during the public discussion.

[SNIP]

I think what Christian is suggesting is a valid interpretation of the spec though it is rather unconventional. The Vulkan spec, as it stands today, requires that the application ensure that at the time of signaling, the timeline semaphore value increases. This means that all of the above possible cases are technically illegal in Vulkan and so it doesn't really matter what we do as long as we don't do anyting especially stupid.

And exactly that's the point. When an application does something stupid with its own submissions then this is not much of a problem.

But this interface is meant to be made for communication between processes, and here we want to be sure that nobody can do anything stupid.

My understanding of how this works on Windows is that a wait operation on 3 is a wait until x >= 3 where x is a 64-bit value and a signal operation is simply a write to x. This means that, in the above cases, waits on 1 will be triggered immediately when 2 is written but waits on 2 may or may not happen at all depending on whether the GPU write which overwrites x to 1 or the CPU (or potentially GPU in a different context) read gets there first such that the reader observes 2. If you mess this up and something isn't signaled, that's your fault.

Yeah and I think that this is actually not a good idea at all. Implementing it like this ultimately means that you can only use polling on the number.

timeline by using the CPU signal ioctl to set it to a high value.

Well I think that this could be problematic as well. Keep in mind that main use case for this is sharing timelines between processes.

In other words you don't want applications to be able to mess with it to much.

Of all these reasons, I think 1 and 2 carry the most weight. 2, in particular, is interesting if we one day want to implement the same behavior with a simple 64-bit value like Windows does. Immagine, for instance, a scenario where the GPU is doing it's own scheduling or command buffers are submitted ahead of the signal operation being available and told to just sit on the GPU until they see x >= 3. (Yes, there are issues here with residency, contention, etc. I'm asking you to use your immagination.) Assuming you can do 64-bit atomics (there are aparently issues here with PCIe that make things sticky), the behavior I'm suggesting is completely implementable in that way whereas the behavior Christian is suggesting is only implementable if you're maintaining a CPU-side list of fences. I don't think we want to paint ourselves into that corner.

Actually we already had such an implementation with radeon. And I can only say that it was a totally PAIN IN THE A* to maintain.

This is one of the reason why we are not using hardware semaphores any more with amdgpu.

Regards,
Christian.

--Jason

Quoted 412 lines

Quoted 10 lines

Regards,
Christian.

Quoted 6 lines

-Lionel

Quoted 3 lines · Chunming Zhou

From: Christian König <[email protected]>

Lockless container implementation similar to a dma_fence_array, but

with

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

only two elements per node and automatic garbage collection.

v2: properly document dma_fence_chain_for_each, add dma_fence_chain_find_seqno,
drop prev reference during garbage collection if it's not a
chain fence.
v3: use head and iterator for dma_fence_chain_for_each
v4: fix reference count in dma_fence_chain_enable_signaling

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Signed-off-by Christian König <[email protected]>

---
   drivers/dma-buf/Makefile          |   3 +-
   drivers/dma-buf/dma-fence-chain.c | 241

++++++++++++++++++++++++++++++

Quoted 41 lines

Quoted 41 lines

Quoted 41 lines

   include/linux/dma-fence-chain.h   |  81 ++++++++++
   3 files changed, 324 insertions(+), 1 deletion(-)

create mode 100644 drivers/dma-buf/dma-fence-chain.c create mode 100644 include/linux/dma-fence-chain.h

drivers/dma-buf/Makefile +0 -1

index 0913a6ccab5a..1f006e083eb9 100644
@@ -1,4 +1,5 @@
-obj-y := dma-buf.o dma-fence.o dma-fence-array.o reservation.o

seqno-fence.o

drivers/dma-buf/Makefile +2 -0

+obj-y := dma-buf.o dma-fence.o dma-fence-array.o dma-fence-chain.o \
+     reservation.o seqno-fence.o
   obj-$(CONFIG_SYNC_FILE)        += sync_file.o
   obj-$(CONFIG_SW_SYNC)        += sw_sync.o sync_debug.o
   obj-$(CONFIG_UDMABUF)        += udmabuf.o

drivers/dma-buf/dma-fence-chain.c new file +8 -0

new file mode 100644
index 000000000000..0c5e3c902fa0
@@ -0,0 +1,241 @@
+/*
+ * fence-chain: chain fences together in a timeline
+ *
+ * Copyright (C) 2018 Advanced Micro Devices, Inc.
+ * Authors:
+ *    Christian König <[email protected]>
+ *
+ * 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

Quoted 8 lines

Quoted 8 lines

drivers/dma-buf/dma-fence-chain.c

Quoted 8 lines

+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ */

drivers/dma-buf/dma-fence-chain.c new file +4 -0

+
+#include <linux/dma-fence-chain.h>
+
+static bool dma_fence_chain_enable_signaling(struct dma_fence

*fence);

Quoted 20 lines

Quoted 20 lines

Quoted 20 lines

drivers/dma-buf/dma-fence-chain.c new file +3 -0

+
+/**
+ * dma_fence_chain_get_prev - use RCU to get a reference to the

previous fence
+ * @chain: chain node to get the previous node from
+ *
+ * Use dma_fence_get_rcu_safe to get a reference to the previous fence of the
+ * chain node.
+ */
+static struct dma_fence *dma_fence_chain_get_prev(struct dma_fence_chain *chain)

drivers/dma-buf/dma-fence-chain.c new file +8 -0

+{
+    struct dma_fence *prev;
+
+    rcu_read_lock();
+    prev = dma_fence_get_rcu_safe(&chain->prev);
+    rcu_read_unlock();
+    return prev;
+}

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/dma-buf/dma-fence-chain.c new file +6 -0

+
+/**
+ * dma_fence_chain_walk - chain walking function
+ * @fence: current chain node
+ *
+ * Walk the chain to the next node. Returns the next fence or NULL

if we are at
+ * the end of the chain. Garbage collects chain nodes which are

already

Quoted 234 lines

Quoted 234 lines

Quoted 234 lines

+ * signaled.
+ */

drivers/dma-buf/dma-fence-chain.c new file +45 -0

+struct dma_fence *dma_fence_chain_walk(struct dma_fence *fence)
+{
+    struct dma_fence_chain *chain, *prev_chain;
+    struct dma_fence *prev, *replacement, *tmp;
+
+    chain = to_dma_fence_chain(fence);
+    if (!chain) {
+        dma_fence_put(fence);
+        return NULL;
+    }
+
+    while ((prev = dma_fence_chain_get_prev(chain))) {
+
+        prev_chain = to_dma_fence_chain(prev);
+        if (prev_chain) {
+            if (!dma_fence_is_signaled(prev_chain->fence))
+                break;
+
+            replacement = dma_fence_chain_get_prev(prev_chain);
+        } else {
+            if (!dma_fence_is_signaled(prev))
+                break;
+
+            replacement = NULL;
+        }
+
+        tmp = cmpxchg(&chain->prev, prev, replacement);
+        if (tmp == prev)
+            dma_fence_put(tmp);
+        else
+            dma_fence_put(replacement);
+        dma_fence_put(prev);
+    }
+
+    dma_fence_put(fence);
+    return prev;
+}
+EXPORT_SYMBOL(dma_fence_chain_walk);
+
+/**
+ * dma_fence_chain_find_seqno - find fence chain node by seqno
+ * @pfence: pointer to the chain node where to start
+ * @seqno: the sequence number to search for
+ *
+ * Advance the fence pointer to the chain node which will signal

this sequence
+ * number. If no sequence number is provided then this is a no-op.
+ *
+ * Returns EINVAL if the fence is not a chain node or the sequence number has
+ * not yet advanced far enough.
+ */
+int dma_fence_chain_find_seqno(struct dma_fence **pfence, uint64_t seqno)

drivers/dma-buf/dma-fence-chain.c new file +22 -0

+{
+    struct dma_fence_chain *chain;
+
+    if (!seqno)
+        return 0;
+
+    chain = to_dma_fence_chain(*pfence);
+    if (!chain || chain->base.seqno < seqno)
+        return -EINVAL;
+
+    dma_fence_chain_for_each(*pfence, &chain->base) {
+        if ((*pfence)->context != chain->base.context ||
+            to_dma_fence_chain(*pfence)->prev_seqno < seqno)
+            break;
+    }
+    dma_fence_put(&chain->base);
+
+    return 0;
+}
+EXPORT_SYMBOL(dma_fence_chain_find_seqno);
+
+static const char *dma_fence_chain_get_driver_name(struct dma_fence

*fence)

drivers/dma-buf/dma-fence-chain.c new file +5 -0

+{
+        return "dma_fence_chain";
+}
+
+static const char *dma_fence_chain_get_timeline_name(struct

dma_fence *fence)

drivers/dma-buf/dma-fence-chain.c new file +18 -0

+{
+        return "unbound";
+}
+
+static void dma_fence_chain_irq_work(struct irq_work *work)
+{
+    struct dma_fence_chain *chain;
+
+    chain = container_of(work, typeof(*chain), work);
+
+    /* Try to rearm the callback */
+    if (!dma_fence_chain_enable_signaling(&chain->base))
+        /* Ok, we are done. No more unsignaled fences left */
+        dma_fence_signal(&chain->base);
+    dma_fence_put(&chain->base);
+}
+
+static void dma_fence_chain_cb(struct dma_fence *f, struct

dma_fence_cb *cb)

drivers/dma-buf/dma-fence-chain.c new file +19 -0

+{
+    struct dma_fence_chain *chain;
+
+    chain = container_of(cb, typeof(*chain), cb);
+    irq_work_queue(&chain->work);
+    dma_fence_put(f);
+}
+
+static bool dma_fence_chain_enable_signaling(struct dma_fence *fence)
+{
+    struct dma_fence_chain *head = to_dma_fence_chain(fence);
+
+    dma_fence_get(&head->base);
+    dma_fence_chain_for_each(fence, &head->base) {
+        struct dma_fence_chain *chain = to_dma_fence_chain(fence);
+        struct dma_fence *f = chain ? chain->fence : fence;
+
+        dma_fence_get(f);
+        if (!dma_fence_add_callback(f, &head->cb,

dma_fence_chain_cb)) {

drivers/dma-buf/dma-fence-chain.c new file +49 -0

+            dma_fence_put(fence);
+            return true;
+        }
+        dma_fence_put(f);
+    }
+    dma_fence_put(&head->base);
+    return false;
+}
+
+static bool dma_fence_chain_signaled(struct dma_fence *fence)
+{
+    dma_fence_chain_for_each(fence, fence) {
+        struct dma_fence_chain *chain = to_dma_fence_chain(fence);
+        struct dma_fence *f = chain ? chain->fence : fence;
+
+        if (!dma_fence_is_signaled(f)) {
+            dma_fence_put(fence);
+            return false;
+        }
+    }
+
+    return true;
+}
+
+static void dma_fence_chain_release(struct dma_fence *fence)
+{
+    struct dma_fence_chain *chain = to_dma_fence_chain(fence);
+
+    dma_fence_put(chain->prev);
+    dma_fence_put(chain->fence);
+    dma_fence_free(fence);
+}
+
+const struct dma_fence_ops dma_fence_chain_ops = {
+    .get_driver_name = dma_fence_chain_get_driver_name,
+    .get_timeline_name = dma_fence_chain_get_timeline_name,
+    .enable_signaling = dma_fence_chain_enable_signaling,
+    .signaled = dma_fence_chain_signaled,
+    .release = dma_fence_chain_release,
+};
+EXPORT_SYMBOL(dma_fence_chain_ops);
+
+/**
+ * dma_fence_chain_init - initialize a fence chain
+ * @chain: the chain node to initialize
+ * @prev: the previous fence
+ * @fence: the current fence
+ *
+ * Initialize a new chain node and either start a new chain or add

the node to
+ * the existing chain of the previous fence.
+ */

drivers/dma-buf/dma-fence-chain.c new file +29 -0

+void dma_fence_chain_init(struct dma_fence_chain *chain,
+              struct dma_fence *prev,
+              struct dma_fence *fence,
+              uint64_t seqno)
+{
+    struct dma_fence_chain *prev_chain = to_dma_fence_chain(prev);
+    uint64_t context;
+
+    spin_lock_init(&chain->lock);
+    chain->prev = prev;
+    chain->fence = fence;
+    chain->prev_seqno = 0;
+    init_irq_work(&chain->work, dma_fence_chain_irq_work);
+
+    /* Try to reuse the context of the previous chain node. */
+    if (prev_chain && __dma_fence_is_later(seqno, prev->seqno)) {
+        context = prev->context;
+        chain->prev_seqno = prev->seqno;
+    } else {
+        context = dma_fence_context_alloc(1);
+        /* Make sure that we always have a valid sequence number. */
+        if (prev_chain)
+            seqno = max(prev->seqno, seqno);
+    }
+
+    dma_fence_init(&chain->base, &dma_fence_chain_ops,
+               &chain->lock, context, seqno);
+}
+EXPORT_SYMBOL(dma_fence_chain_init);

include/linux/dma-fence-chain.h new file +8 -0

new file mode 100644
index 000000000000..a5c2e8c6915c
@@ -0,0 +1,81 @@
+/*
+ * fence-chain: chain fences together in a timeline
+ *
+ * Copyright (C) 2018 Advanced Micro Devices, Inc.
+ * Authors:
+ *    Christian König <[email protected]>
+ *
+ * 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.
+ */
+

Quoted 8 lines

Quoted 8 lines

include/linux/dma-fence-chain.h

Quoted 8 lines

include/linux/dma-fence-chain.h new file +8 -0

+#ifndef __LINUX_DMA_FENCE_CHAIN_H
+#define __LINUX_DMA_FENCE_CHAIN_H
+
+#include <linux/dma-fence.h>
+#include <linux/irq_work.h>
+
+/**
+ * struct dma_fence_chain - fence to represent an node of a fence

chain

Quoted 36 lines

Quoted 36 lines

Quoted 36 lines

+ * @base: fence base class
+ * @lock: spinlock for fence handling
+ * @prev: previous fence of the chain
+ * @prev_seqno: original previous seqno before garbage collection
+ * @fence: encapsulated fence
+ * @cb: callback structure for signaling
+ * @work: irq work item for signaling
+ */

include/linux/dma-fence-chain.h new file +28 -0

+struct dma_fence_chain {
+    struct dma_fence base;
+    spinlock_t lock;
+    struct dma_fence *prev;
+    u64 prev_seqno;
+    struct dma_fence *fence;
+    struct dma_fence_cb cb;
+    struct irq_work work;
+};
+
+extern const struct dma_fence_ops dma_fence_chain_ops;
+
+/**
+ * to_dma_fence_chain - cast a fence to a dma_fence_chain
+ * @fence: fence to cast to a dma_fence_array
+ *
+ * Returns NULL if the fence is not a dma_fence_chain,
+ * or the dma_fence_chain otherwise.
+ */
+static inline struct dma_fence_chain *
+to_dma_fence_chain(struct dma_fence *fence)
+{
+    if (!fence || fence->ops != &dma_fence_chain_ops)
+        return NULL;
+
+    return container_of(fence, struct dma_fence_chain, base);
+}
+

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

+/**
+ * dma_fence_chain_for_each - iterate over all fences in chain
+ * @iter: current fence
+ * @head: starting point
+ *
+ * Iterate over all fences in the chain. We keep a reference to the current
+ * fence while inside the loop which must be dropped when breaking

out.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

+ */

include/linux/dma-fence-chain.h new file +6 -0

+#define dma_fence_chain_for_each(iter, head)    \
+    for (iter = dma_fence_get(head); iter; \
+         iter = dma_fence_chain_walk(head))
+
+struct dma_fence *dma_fence_chain_walk(struct dma_fence *fence);
+int dma_fence_chain_find_seqno(struct dma_fence **pfence, uint64_t

Quoted 8 lines

Quoted 8 lines

include/linux/dma-fence-chain.h

Quoted 7 lines

seqno);

include/linux/dma-fence-chain.h new file +6 -0

+void dma_fence_chain_init(struct dma_fence_chain *chain,
+              struct dma_fence *prev,
+              struct dma_fence *fence,
+              uint64_t seqno);
+
+#endif /* __LINUX_DMA_FENCE_CHAIN_H */

_______________________________________________
dri-devel mailing list [email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Am 15.02.19 um 15:23 schrieb Lionel Landwerlin:

Quoted 11 lines

Hi Christian, David,

For timeline semaphore we need points to signaled in order. I'm struggling to understand how this fence-chain implementation preserves ordering of the seqnos.

One of the scenario I can see an issue happening is when you have a timeline with points 1 & 2 and userspace submits for 2 different engines :
- first with let's say a blitter style engine on point 2
- then a 3d style engine on point 1

Yeah, and where exactly is the problem?

Seqno 1 will signal when the 3d style engine finishes work.

And seqno 2 will signal when both seqno 1 is signaled and the blitter style engine has finished its work.

Am 15.02.19 um 19:16 schrieb Jason Ekstrand:

Quoted 26 lines · Christian König

Am 15.02.19 um 17:49 schrieb Jason Ekstrand:

Quoted 13 lines · Lionel Landwerlin via dri-devel

Quoted 12 lines · Christian

Am 15.02.19 um 15:23 schrieb Lionel Landwerlin:

Quoted 11 lines

Hi Christian, David,

For timeline semaphore we need points to signaled in order. I'm struggling to understand how this fence-chain implementation preserves ordering of the seqnos.

One of the scenario I can see an issue happening is when you have a timeline with points 1 & 2 and userspace submits for 2 different engines :
- first with let's say a blitter style engine on point 2
- then a 3d style engine on point 1

Quoted 7 lines

Quoted 6 lines

Yeah, and where exactly is the problem?

Seqno 1 will signal when the 3d style engine finishes work.

And seqno 2 will signal when both seqno 1 is signaled and the blitter style engine has finished its work.

Quoted 8 lines

That's an interesting interpretation of the spec. I think it's legal and I could see that behavior may be desirable in some ways.

Well we actually had this discussion multiple times now, both internally as well as on the mailing list. Please also see the previous mails with Daniel on this topic.

I dug through dri-devel and read everything I could find with a search for "timeline semaphore" I didn't find all that much but this did come up once.

Need to dig through my mails as well, that was back in November/December last year.

Quoted 9 lines

My initial suggestion was actually to exactly what Leonid suggested as well.

And following this I used a rather simple container for the implementation, e.g. just a ring buffer indexed by the sequence number. In this scenario userspace can specify on syncobj creation time how big the window for sequence numbers should be, e.g. in this implementation how big the ring buffer would be.

Quoted 4 lines

This was rejected by our guys who actually wrote a good part of the Vulkan specification. Daniel then has gone into the same direction during the public discussion.

I agree with whoever said that specifying a ringbuffer size is unacceptable. I'm not really sure how that's relevant though. Is a ringbuffer required to implement the behavior that is being suggested here? Genuine question; I'm trying to get back up to speed.

Using a ring buffer was just an example how we could do it if we follow my and Lionel's suggestion.

Key point is that we could simplify the implementation massively if sequence numbers don't need to depend on each other.

In other words we just see the syncobj as container where fences are added and retrieved from instead of something actively involved in the signaling.

Main reason we didn't do it this way is because the AMD Vulkan team has rejected this approach.

Additional to that chaining sequence numbers together is really the more defensive approach, e.g. it is less likely that applications can shoot themselves in the foot.

4. If you do get into a sticky situation, you can unblock an entire

Quoted 9 lines

timeline by using the CPU signal ioctl to set it to a high value.

Well I think that this could be problematic as well. Keep in mind that main use case for this is sharing timelines between processes.

In other words you don't want applications to be able to mess with it to much.

Cross-process is exactly why you want it. Suppose you're a compositor and you have a timeline shared with another application and you've submitted work which waits on it. Then you get a notification somehow (SIGHUP?) that the client has died leaving you hanging. What do you do? You take the semaphore that's shared with you and the client and whack it to UINT64_MAX to unblock yourself. Of course, this can be abused and that's always the risk you take with timelines.

My last status is that basically everybody agrees now that wait before signal in the kernel is forbidden.

user mode can query timeline payload.
v2: check return value of copy_to_user
v3: handle querying entry by entry
v4: rebase on new chain container, simplify interface

Signed-off-by Chunming Zhou <[email protected]>

Cc Daniel Rakos <[email protected]>

Cc Jason Ekstrand <[email protected]>

Cc Bas Nieuwenhuizen <[email protected]>

Cc Dave Airlie <[email protected]>

Cc Christian König <[email protected]>

Cc Chris Wilson <[email protected]>

---
  drivers/gpu/drm/drm_internal.h |  2 ++
  drivers/gpu/drm/drm_ioctl.c    |  2 ++
  drivers/gpu/drm/drm_syncobj.c  | 43 ++++++++++++++++++++++++++++++++++
  include/uapi/drm/drm.h         | 10 ++++++++
  4 files changed, 57 insertions(+)

drivers/gpu/drm/drm_internal.h +2 -0

index 18b41e10195c..dab4d5936441 100644
@@ -184,6 +184,8 @@ int drm_syncobj_reset_ioctl(struct drm_device *dev, void *data,
  			    struct drm_file *file_private);
  int drm_syncobj_signal_ioctl(struct drm_device *dev, void *data,
  			     struct drm_file *file_private);
+int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
+			    struct drm_file *file_private);
 
  /* drm_framebuffer.c */
  void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,

drivers/gpu/drm/drm_ioctl.c +2 -0

index a9a17ed35cc4..7578ef6dc1d1 100644
@@ -681,6 +681,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
  		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
  	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_SIGNAL, drm_syncobj_signal_ioctl,
  		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_QUERY, drm_syncobj_query_ioctl,
+		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
  	DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE, drm_crtc_get_sequence_ioctl, DRM_UNLOCKED),
  	DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE, drm_crtc_queue_sequence_ioctl, DRM_UNLOCKED),
  	DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, drm_mode_create_lease_ioctl, DRM_MASTER|DRM_UNLOCKED),

drivers/gpu/drm/drm_syncobj.c +24 -0

index 348079bb0965..f97fa00ca1d0 100644
@@ -1061,3 +1061,46 @@ drm_syncobj_signal_ioctl(struct drm_device *dev, void *data,
 
  	return ret;
  }
+
+int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
+			    struct drm_file *file_private)
+{
+	struct drm_syncobj_timeline_array *args = data;
+	struct drm_syncobj **syncobjs;
+	uint64_t __user *points = u64_to_user_ptr(args->points);
+	uint32_t i;
+	int ret;
+
+	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+		return -ENODEV;
+
+	if (args->pad != 0)
+		return -EINVAL;
+
+	if (args->count_handles == 0)
+		return -EINVAL;
+
+	ret = drm_syncobj_array_find(file_private,
+				     u64_to_user_ptr(args->handles),
+				     args->count_handles,
+				     &syncobjs);
+	if (ret < 0)

drivers/gpu/drm/drm_syncobj.c +9 -0

+		ret = copy_to_user(&points[i], &point, sizeof(uint64_t));
+		ret = ret ? -EFAULT : 0;
+		if (ret)
+			break;
+	}
+	drm_syncobj_array_free(syncobjs, args->count_handles);
+
+	return ret;
+}

include/uapi/drm/drm.h +8 -0

index 0092111d002c..b2c36f2b2599 100644
@@ -767,6 +767,14 @@ struct drm_syncobj_array {
  	__u32 pad;
  };
 
+struct drm_syncobj_timeline_array {
+	__u64 handles;
+	__u64 points;
+	__u32 count_handles;
+	__u32 pad;
+};
+
+
  /* Query current scanout sequence number */
  struct drm_crtc_get_sequence {
  	__u32 crtc_id;		/* requested crtc_id */

Quoted 82 lines

user mode can query timeline payload.
v2: check return value of copy_to_user
v3: handle querying entry by entry
v4: rebase on new chain container, simplify interface

Signed-off-by Chunming Zhou <[email protected]>

Cc Daniel Rakos <[email protected]>

Cc Jason Ekstrand <[email protected]>

Cc Bas Nieuwenhuizen <[email protected]>

Cc Dave Airlie <[email protected]>

Cc Christian König <[email protected]>

Cc Chris Wilson <[email protected]>

---
  drivers/gpu/drm/drm_internal.h |  2 ++ drivers/gpu/drm/drm_ioctl.c    |  2 ++ drivers/gpu/drm/drm_syncobj.c  | 43 ++++++++++++++++++++++++++++++++++ include/uapi/drm/drm.h         | 10 ++++++++

  4 files changed, 57 insertions(+)

drivers/gpu/drm/drm_internal.h +2 -0

index 18b41e10195c..dab4d5936441 100644
@@ -184,6 +184,8 @@ int drm_syncobj_reset_ioctl(struct drm_device 
 *dev, void *data,
                   struct drm_file *file_private);
   int drm_syncobj_signal_ioctl(struct drm_device *dev, void *data,
                    struct drm_file *file_private);
+int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
+                struct drm_file *file_private);
     /* drm_framebuffer.c */
   void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,

drivers/gpu/drm/drm_ioctl.c +2 -0

index a9a17ed35cc4..7578ef6dc1d1 100644
@@ -681,6 +681,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
                 DRM_UNLOCKED|DRM_RENDER_ALLOW),
       DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_SIGNAL, drm_syncobj_signal_ioctl,
                 DRM_UNLOCKED|DRM_RENDER_ALLOW),
+    DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_QUERY, drm_syncobj_query_ioctl,
+              DRM_UNLOCKED|DRM_RENDER_ALLOW),
       DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE, drm_crtc_get_sequence_ioctl, DRM_UNLOCKED),
       DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE, drm_crtc_queue_sequence_ioctl, DRM_UNLOCKED),
       DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, drm_mode_create_lease_ioctl, DRM_MASTER|DRM_UNLOCKED),

drivers/gpu/drm/drm_syncobj.c +29 -0

index 348079bb0965..f97fa00ca1d0 100644
@@ -1061,3 +1061,46 @@ drm_syncobj_signal_ioctl(struct drm_device 
 *dev, void *data,
         return ret;
   }
+
+int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
+                struct drm_file *file_private)
+{
+    struct drm_syncobj_timeline_array *args = data;
+    struct drm_syncobj **syncobjs;
+    uint64_t __user *points = u64_to_user_ptr(args->points);
+    uint32_t i;
+    int ret;
+
+    if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+        return -ENODEV;
+
+    if (args->pad != 0)
+        return -EINVAL;
+
+    if (args->count_handles == 0)
+        return -EINVAL;
+
+    ret = drm_syncobj_array_find(file_private,
+                     u64_to_user_ptr(args->handles),
+                     args->count_handles,
+                     &syncobjs);
+    if (ret < 0)
+        return ret;
+
+    for (i = 0; i < args->count_handles; i++) {
+        struct dma_fence_chain *chain;
+        struct dma_fence *fence;

drivers/gpu/drm/drm_syncobj.c drm_syncobj_signal_ioctl(struct drm_device *dev, void *data, · line 1097

Quoted 5 lines

drivers/gpu/drm/drm_syncobj.c +5 -0

+        uint64_t point;
+
+        fence = drm_syncobj_fence_get(syncobjs[i]);
+        chain = to_dma_fence_chain(fence);
+        point = chain ? fence->seqno : 0;

Sorry, I don' t want to sound annoying, but this looks like this could report values going backward.

Anything add a point X to a timeline that has reached value Y with X < Y would trigger that.

Either through the submission or userspace signaling or importing another syncpoint's fence.

-Lionel

Quoted 34 lines

drivers/gpu/drm/drm_syncobj.c +9 -0

+        ret = copy_to_user(&points[i], &point, sizeof(uint64_t));
+        ret = ret ? -EFAULT : 0;
+        if (ret)
+            break;
+    }
+    drm_syncobj_array_free(syncobjs, args->count_handles);
+
+    return ret;
+}

include/uapi/drm/drm.h +7 -0

index 0092111d002c..b2c36f2b2599 100644
@@ -767,6 +767,14 @@ struct drm_syncobj_array {
       __u32 pad;
   };
   +struct drm_syncobj_timeline_array {
+    __u64 handles;
+    __u64 points;
+    __u32 count_handles;
+    __u32 pad;
+};
+
+
   /* Query current scanout sequence number */
   struct drm_crtc_get_sequence {

      __u32 crtc_id;        /* requested crtc_id */

include/uapi/drm/drm.h +2 -0

@@ -924,6 +932,8 @@ extern "C" {
   #define DRM_IOCTL_MODE_REVOKE_LEASE    DRM_IOWR(0xC9, struct drm_mode_revoke_lease)
     #define DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT    DRM_IOWR(0xCA, struct drm_syncobj_timeline_wait)
+#define DRM_IOCTL_SYNCOBJ_QUERY        DRM_IOWR(0xCB, struct 
 drm_syncobj_timeline_array)
+
   /**

include/uapi/drm/drm.h extern "C" { · line 939

Quoted 2 lines

include/uapi/drm/drm.h +0 -0

    * Device specific ioctls should only be in their respective headers
    * The device specific ioctl range is from 0x40 to 0x9f.

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Am 15.02.19 um 20:31 schrieb Lionel Landwerlin via amd-gfx:

Quoted 84 lines · Chunming Zhou

Quoted 83 lines

user mode can query timeline payload.
v2: check return value of copy_to_user
v3: handle querying entry by entry
v4: rebase on new chain container, simplify interface

Signed-off-by Chunming Zhou <[email protected]>

Cc Daniel Rakos <[email protected]>

Cc Jason Ekstrand <[email protected]>

Cc Bas Nieuwenhuizen <[email protected]>

Cc Dave Airlie <[email protected]>

Cc Christian König <[email protected]>

Cc Chris Wilson <[email protected]>

---
  drivers/gpu/drm/drm_internal.h |  2 ++ drivers/gpu/drm/drm_ioctl.c    |  2 ++ drivers/gpu/drm/drm_syncobj.c  | 43
++++++++++++++++++++++++++++++++++
  include/uapi/drm/drm.h         | 10 ++++++++

  4 files changed, 57 insertions(+)

drivers/gpu/drm/drm_internal.h +2 -0

index 18b41e10195c..dab4d5936441 100644
@@ -184,6 +184,8 @@ int drm_syncobj_reset_ioctl(struct drm_device 
 *dev, void *data,
                   struct drm_file *file_private);
   int drm_syncobj_signal_ioctl(struct drm_device *dev, void *data,
                    struct drm_file *file_private);
+int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
+                struct drm_file *file_private);
     /* drm_framebuffer.c */
   void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,

drivers/gpu/drm/drm_ioctl.c +2 -0

index a9a17ed35cc4..7578ef6dc1d1 100644
@@ -681,6 +681,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
                 DRM_UNLOCKED|DRM_RENDER_ALLOW),
       DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_SIGNAL, drm_syncobj_signal_ioctl,
                 DRM_UNLOCKED|DRM_RENDER_ALLOW),
+    DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_QUERY, drm_syncobj_query_ioctl,
+              DRM_UNLOCKED|DRM_RENDER_ALLOW),
       DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE, drm_crtc_get_sequence_ioctl, DRM_UNLOCKED),
       DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE, drm_crtc_queue_sequence_ioctl, DRM_UNLOCKED),
       DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, drm_mode_create_lease_ioctl, DRM_MASTER|DRM_UNLOCKED),

drivers/gpu/drm/drm_syncobj.c +29 -0

index 348079bb0965..f97fa00ca1d0 100644
@@ -1061,3 +1061,46 @@ drm_syncobj_signal_ioctl(struct drm_device 
 *dev, void *data,
         return ret;
   }
+
+int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
+                struct drm_file *file_private)
+{
+    struct drm_syncobj_timeline_array *args = data;
+    struct drm_syncobj **syncobjs;
+    uint64_t __user *points = u64_to_user_ptr(args->points);
+    uint32_t i;
+    int ret;
+
+    if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+        return -ENODEV;
+
+    if (args->pad != 0)
+        return -EINVAL;
+
+    if (args->count_handles == 0)
+        return -EINVAL;
+
+    ret = drm_syncobj_array_find(file_private,
+                     u64_to_user_ptr(args->handles),
+                     args->count_handles,
+                     &syncobjs);
+    if (ret < 0)
+        return ret;
+
+    for (i = 0; i < args->count_handles; i++) {
+        struct dma_fence_chain *chain;
+        struct dma_fence *fence;

Quoted 8 lines

drivers/gpu/drm/drm_syncobj.c drm_syncobj_signal_ioctl(struct drm_device *dev, void *data, · line 1097

Quoted 5 lines

drivers/gpu/drm/drm_syncobj.c +5 -0

+        uint64_t point;
+
+        fence = drm_syncobj_fence_get(syncobjs[i]);
+        chain = to_dma_fence_chain(fence);
+        point = chain ? fence->seqno : 0;

Sorry, I don' t want to sound annoying, but this looks like this could report values going backward.

Well please be annoying as much as you can :) But yeah all that stuff has been discussed before as well.

Quoted 2 lines

Anything add a point X to a timeline that has reached value Y with X < Y would trigger that.

Yes, that can indeed happen.

But adding a timeline point X which is before the already added point Y is illegal in the first place :)

So when the application does something stupid and breaks it can just keep the pieces.

In the kernel we still do the most defensive thing and sync to everything in this case.

I'm just not sure if we should print an error into syslog or just continue silently.

Regards,
Christian.

Quoted 49 lines

Either through the submission or userspace signaling or importing another syncpoint's fence.

-Lionel

Quoted 28 lines

drivers/gpu/drm/drm_syncobj.c +9 -0

+        ret = copy_to_user(&points[i], &point, sizeof(uint64_t));
+        ret = ret ? -EFAULT : 0;
+        if (ret)
+            break;
+    }
+    drm_syncobj_array_free(syncobjs, args->count_handles);
+
+    return ret;
+}

include/uapi/drm/drm.h +7 -0

index 0092111d002c..b2c36f2b2599 100644
@@ -767,6 +767,14 @@ struct drm_syncobj_array {
       __u32 pad;
   };
   +struct drm_syncobj_timeline_array {
+    __u64 handles;
+    __u64 points;
+    __u32 count_handles;
+    __u32 pad;
+};
+
+
   /* Query current scanout sequence number */
   struct drm_crtc_get_sequence {

      __u32 crtc_id;        /* requested crtc_id */

include/uapi/drm/drm.h extern "C" { · line 939

Quoted 8 lines

include/uapi/drm/drm.h +2 -0

@@ -924,6 +932,8 @@ extern "C" {
   #define DRM_IOCTL_MODE_REVOKE_LEASE    DRM_IOWR(0xC9, struct drm_mode_revoke_lease)
     #define DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT    DRM_IOWR(0xCA, struct drm_syncobj_timeline_wait)
+#define DRM_IOCTL_SYNCOBJ_QUERY        DRM_IOWR(0xCB, struct 
 drm_syncobj_timeline_array)
+
   /**
    * Device specific ioctls should only be in their respective headers
    * The device specific ioctl range is from 0x40 to 0x9f.

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Quoted 107 lines

Am 15.02.19 um 20:31 schrieb Lionel Landwerlin via amd-gfx:

Quoted 93 lines · Chunming Zhou

Quoted 92 lines

user mode can query timeline payload.
v2: check return value of copy_to_user
v3: handle querying entry by entry
v4: rebase on new chain container, simplify interface

Signed-off-by Chunming Zhou <[email protected]>

Cc Daniel Rakos <[email protected]>

Cc Jason Ekstrand <[email protected]>

Cc Bas Nieuwenhuizen <[email protected]>

Cc Dave Airlie <[email protected]>

Cc Christian König <[email protected]>

Cc Chris Wilson <[email protected]>

---
  drivers/gpu/drm/drm_internal.h |  2 ++ drivers/gpu/drm/drm_ioctl.c    |  2 ++ drivers/gpu/drm/drm_syncobj.c  | 43
++++++++++++++++++++++++++++++++++
  include/uapi/drm/drm.h         | 10 ++++++++

  4 files changed, 57 insertions(+)

drivers/gpu/drm/drm_internal.h +2 -0

index 18b41e10195c..dab4d5936441 100644
@@ -184,6 +184,8 @@ int drm_syncobj_reset_ioctl(struct drm_device 
 *dev, void *data,
                   struct drm_file *file_private);
   int drm_syncobj_signal_ioctl(struct drm_device *dev, void *data,
                    struct drm_file *file_private);
+int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
+                struct drm_file *file_private);
     /* drm_framebuffer.c */

  void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,

drivers/gpu/drm/drm_ioctl.c +2 -0

index a9a17ed35cc4..7578ef6dc1d1 100644
@@ -681,6 +681,8 @@ static const struct drm_ioctl_desc drm_ioctls[] 
 = {
                 DRM_UNLOCKED|DRM_RENDER_ALLOW),
       DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_SIGNAL, 
 drm_syncobj_signal_ioctl,
                 DRM_UNLOCKED|DRM_RENDER_ALLOW),
+    DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_QUERY, drm_syncobj_query_ioctl,
+              DRM_UNLOCKED|DRM_RENDER_ALLOW),
       DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE, 

drm_crtc_get_sequence_ioctl, DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE, drm_crtc_queue_sequence_ioctl, DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, drm_mode_create_lease_ioctl, DRM_MASTER|DRM_UNLOCKED),

drivers/gpu/drm/drm_syncobj.c +30 -0

index 348079bb0965..f97fa00ca1d0 100644
@@ -1061,3 +1061,46 @@ drm_syncobj_signal_ioctl(struct drm_device 
 *dev, void *data,
         return ret;
   }
+
+int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
+                struct drm_file *file_private)
+{
+    struct drm_syncobj_timeline_array *args = data;
+    struct drm_syncobj **syncobjs;
+    uint64_t __user *points = u64_to_user_ptr(args->points);
+    uint32_t i;
+    int ret;
+
+    if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+        return -ENODEV;
+
+    if (args->pad != 0)
+        return -EINVAL;
+
+    if (args->count_handles == 0)
+        return -EINVAL;
+
+    ret = drm_syncobj_array_find(file_private,
+                     u64_to_user_ptr(args->handles),
+                     args->count_handles,
+                     &syncobjs);
+    if (ret < 0)
+        return ret;
+
+    for (i = 0; i < args->count_handles; i++) {
+        struct dma_fence_chain *chain;
+        struct dma_fence *fence;
+        uint64_t point;

Quoted 8 lines

drivers/gpu/drm/drm_syncobj.c drm_syncobj_signal_ioctl(struct drm_device · line 1097

Quoted 4 lines

drivers/gpu/drm/drm_syncobj.c +4 -0

+
+        fence = drm_syncobj_fence_get(syncobjs[i]);
+        chain = to_dma_fence_chain(fence);
+        point = chain ? fence->seqno : 0;

Sorry, I don' t want to sound annoying, but this looks like this could report values going backward.

Well please be annoying as much as you can :) But yeah all that stuff has been discussed before as well.

Quoted 1 line

Quoted 4 lines

Quoted 2 lines

Anything add a point X to a timeline that has reached value Y with X < Y would trigger that.

Yes, that can indeed happen.

trigger what? when adding x (x < y), then return 0 when query? Why would this happen?
No, syncobj->fence should always be there and the last chain node, if it is ever added.

-David

Quoted 68 lines

But adding a timeline point X which is before the already added point Y is illegal in the first place :)

So when the application does something stupid and breaks it can just keep the pieces.

In the kernel we still do the most defensive thing and sync to everything in this case.

I'm just not sure if we should print an error into syslog or just continue silently.

Regards,
Christian.

Quoted 52 lines

Either through the submission or userspace signaling or importing another syncpoint's fence.

-Lionel

Quoted 36 lines

drivers/gpu/drm/drm_syncobj.c +9 -0

+        ret = copy_to_user(&points[i], &point, sizeof(uint64_t));
+        ret = ret ? -EFAULT : 0;
+        if (ret)
+            break;
+    }
+    drm_syncobj_array_free(syncobjs, args->count_handles);
+
+    return ret;
+}

include/uapi/drm/drm.h +7 -0

index 0092111d002c..b2c36f2b2599 100644
@@ -767,6 +767,14 @@ struct drm_syncobj_array {
       __u32 pad;
   };
   +struct drm_syncobj_timeline_array {
+    __u64 handles;
+    __u64 points;
+    __u32 count_handles;
+    __u32 pad;
+};
+
+
   /* Query current scanout sequence number */
   struct drm_crtc_get_sequence {

      __u32 crtc_id;        /* requested crtc_id */

include/uapi/drm/drm.h +2 -0

@@ -924,6 +932,8 @@ extern "C" {
   #define DRM_IOCTL_MODE_REVOKE_LEASE    DRM_IOWR(0xC9, struct 
 drm_mode_revoke_lease)
     #define DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT DRM_IOWR(0xCA, struct 
 drm_syncobj_timeline_wait)
+#define DRM_IOCTL_SYNCOBJ_QUERY        DRM_IOWR(0xCB, struct 
 drm_syncobj_timeline_array)
+
   /**

Quoted 2 lines

   * Device specific ioctls should only be in their respective headers
   * The device specific ioctl range is from 0x40 to 0x9f.

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Am 18.02.19 um 04:10 schrieb zhoucm1:

Quoted 106 lines · Christian König

Quoted 104 lines

Am 15.02.19 um 20:31 schrieb Lionel Landwerlin via amd-gfx:

Quoted 92 lines · Chunming Zhou

Quoted 91 lines

user mode can query timeline payload.
v2: check return value of copy_to_user
v3: handle querying entry by entry
v4: rebase on new chain container, simplify interface

Signed-off-by Chunming Zhou <[email protected]>

Cc Daniel Rakos <[email protected]>

Cc Jason Ekstrand <[email protected]>

Cc Bas Nieuwenhuizen <[email protected]>

Cc Dave Airlie <[email protected]>

Cc Christian König <[email protected]>

Cc Chris Wilson <[email protected]>

---
   drivers/gpu/drm/drm_internal.h |  2 ++
   drivers/gpu/drm/drm_ioctl.c    |  2 ++
   drivers/gpu/drm/drm_syncobj.c  | 43
++++++++++++++++++++++++++++++++++
   include/uapi/drm/drm.h         | 10 ++++++++
   4 files changed, 57 insertions(+)

drivers/gpu/drm/drm_internal.h +2 -0

index 18b41e10195c..dab4d5936441 100644
@@ -184,6 +184,8 @@ int drm_syncobj_reset_ioctl(struct drm_device
 *dev, void *data,
                   struct drm_file *file_private);
   int drm_syncobj_signal_ioctl(struct drm_device *dev, void *data,
                    struct drm_file *file_private);
+int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
+                struct drm_file *file_private);
     /* drm_framebuffer.c */
   void drm_framebuffer_print_info(struct drm_printer *p, unsigned

int indent,

drivers/gpu/drm/drm_ioctl.c +2 -0

index a9a17ed35cc4..7578ef6dc1d1 100644
@@ -681,6 +681,8 @@ static const struct drm_ioctl_desc drm_ioctls[]
 = {
                 DRM_UNLOCKED|DRM_RENDER_ALLOW),
       DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_SIGNAL,
 drm_syncobj_signal_ioctl,
                 DRM_UNLOCKED|DRM_RENDER_ALLOW),
+    DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_QUERY, drm_syncobj_query_ioctl,
+              DRM_UNLOCKED|DRM_RENDER_ALLOW),
       DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE,

drm_crtc_get_sequence_ioctl, DRM_UNLOCKED),
      DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE,
drm_crtc_queue_sequence_ioctl, DRM_UNLOCKED),
      DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE,
drm_mode_create_lease_ioctl, DRM_MASTER|DRM_UNLOCKED),

drivers/gpu/drm/drm_syncobj.c +29 -0

index 348079bb0965..f97fa00ca1d0 100644
@@ -1061,3 +1061,46 @@ drm_syncobj_signal_ioctl(struct drm_device
 *dev, void *data,
         return ret;
   }
+
+int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
+                struct drm_file *file_private)
+{
+    struct drm_syncobj_timeline_array *args = data;
+    struct drm_syncobj **syncobjs;
+    uint64_t __user *points = u64_to_user_ptr(args->points);
+    uint32_t i;
+    int ret;
+
+    if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+        return -ENODEV;
+
+    if (args->pad != 0)
+        return -EINVAL;
+
+    if (args->count_handles == 0)
+        return -EINVAL;
+
+    ret = drm_syncobj_array_find(file_private,
+                     u64_to_user_ptr(args->handles),
+                     args->count_handles,
+                     &syncobjs);
+    if (ret < 0)
+        return ret;
+
+    for (i = 0; i < args->count_handles; i++) {
+        struct dma_fence_chain *chain;
+        struct dma_fence *fence;

Quoted 8 lines

drivers/gpu/drm/drm_syncobj.c drm_syncobj_signal_ioctl(struct drm_device · line 1097

Quoted 5 lines

drivers/gpu/drm/drm_syncobj.c +5 -0

+        uint64_t point;
+
+        fence = drm_syncobj_fence_get(syncobjs[i]);
+        chain = to_dma_fence_chain(fence);
+        point = chain ? fence->seqno : 0;

Sorry, I don' t want to sound annoying, but this looks like this could report values going backward.

Well please be annoying as much as you can :) But yeah all that stuff has been discussed before as well.

Quoted 7 lines

Quoted 3 lines

Quoted 2 lines

Anything add a point X to a timeline that has reached value Y with X < Y would trigger that.

Yes, that can indeed happen.

trigger what? when adding x (x < y), then return 0 when query? Why would this happen?
No, syncobj->fence should always be there and the last chain node, if it is ever added.