[Intel-gfx] [PATCH 01/10] dma-buf: add new dma_fence_chain container v4

[Chris Wilson]

Quoting Chunming Zhou (2018-12-11 10:34:40)
> From: Christian König <ckoenig.leichtzumerken at gmail.com>
> 
> 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 <christian.koenig at amd.com>
> ---
>  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 @@
> +/*

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 <christian.koenig at amd.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */
> +
> +#include <linux/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);

if (cmpxchg(&chain->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 dma_fence_chain_for_each(iter, head)   \
> +       for (iter = dma_fence_get(head); iter; \
> +            iter = dma_fence_chain_walk(head))

               dma_fence_chain_walk(head) <-- should be iter?

#define dma_fence_chain_for_each(iter, head)   \
       for (iter = dma_fence_get(head); iter; \
            ({ dma_fence_put(iter); iter = dma_fence_chain_walk(iter)))

just so that dma_fence_chain_walk() didn't look unbalanced and the
put/get more clearly inside the for_each.

> +       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)

That inout parameter took me by surprise. "find" I expected to return
the fence.

Not used in this patch, so I need to read on to see if its use
simplifies the code or is equally surprising ;)
-Chris