Re: [Intel-gfx] [PATCH 01/10] dma-buf: add new dma_fence_chain container v4
Quoting Chunming Zhou (2018-12-11 10:34:40)
> From: Christian König
>
> 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
> ---
> 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 ..0c5e3c902fa0
> --- /dev/null
> +++ b/drivers/dma-buf/dma-fence-chain.c
> @@ -0,0 +1,241 @@
> +/*
SPDX-License-Identifier: GPL-2.0
> + * fence-chain: chain fences together in a timeline
> + *
> + * Copyright (C) 2018 Advanced Micro Devices, Inc.
> + * Authors:
> + * Christian König
> + *
> + * 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
> +
> +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(>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(>prev, prev, replacement);
> + if (tmp == prev)
> + dma_fence_put(tmp);
> + else
> + dma_fence_put(replacement);
if (cmpxchg(>prev, prev, replacement) == prev)
dma_fence_put(prev)
else
dma_fence_put(replacement);
would be a little easier for me to read.
> + dma_fence_put(prev);
> + }
> +
> + dma_fence_put(fence);
Putting the caller's fence caught me by surprise.
I'd be tempted to do
> +
> +/**
> + * 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
RE: [PATCH 01/10] dma-buf: add new dma_fence_chain container v4
Hi Daniel and Chris,
Could you take a look on all the patches? Can we get your RB or AB on all
patches including igt patch before we submit to drm-misc?
We already fix all existing issues, and also add test case in IGT as your
required.
Btw, the patch set is tested by below tests:
a. vulkan cts " ./deqp-vk -n dEQP-VK. *semaphore*"
b. internal vulkan timeline test
c. libdrm test "sudo ./amdgpu_test -s 9"
d. IGT test, "sudo ./syncobj_basic"
e. IGT test, "sudo ./syncobj_wait"
f. IGT test, "sudo ./syncobj_timeline"
Any other suggestion or requirement is welcome.
-David
> -Original Message-
> From: dri-devel On Behalf Of
> Chunming Zhou
> Sent: Tuesday, December 11, 2018 6:35 PM
> To: Koenig, Christian ; dri-
> de...@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; intel-
> g...@lists.freedesktop.org
> Cc: Christian König ; Koenig, Christian
>
> Subject: [PATCH 01/10] dma-buf: add new dma_fence_chain container v4
>
> From: Christian König
>
> 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
> ---
> 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 ..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
> + *
> + * 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
> +
> +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(>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
[PATCH 01/10] dma-buf: add new dma_fence_chain container v4
From: Christian König
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
---
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 ..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
+ *
+ * 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
+
+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(>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(>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, >base) {
+ if ((*pfence)->context !=
[PATCH 01/10] dma-buf: add new dma_fence_chain container v4
From: Christian König
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
---
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 ..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
+ *
+ * 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
+
+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(>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(>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, >base) {
+ if ((*pfence)->context !=
