Am 10.08.19 um 17:34 schrieb Chris Wilson:

Move the duplicated code within dma-fence.c into the header for wider reuse. In the process apply a small micro-optimisation to only prune the fence->cb_list once rather than use list_del on every entry.

Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Tvrtko Ursulin tvrtko.ursulin@intel.com

---

drivers/dma-buf/Makefile | 10 +- drivers/dma-buf/dma-fence-trace.c | 28 +++ drivers/dma-buf/dma-fence.c | 33 +-- drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 32 +-- include/linux/dma-fence-impl.h | 83 +++++++ include/linux/dma-fence-types.h | 258 ++++++++++++++++++++ include/linux/dma-fence.h | 228 +----------------

Mhm, I don't really see the value in creating more header files.

Especially I'm pretty sure that the types should stay in dma-fence.h

Christian.

7 files changed, 386 insertions(+), 286 deletions(-) create mode 100644 drivers/dma-buf/dma-fence-trace.c create mode 100644 include/linux/dma-fence-impl.h create mode 100644 include/linux/dma-fence-types.h

diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile index e8c7310cb800..65c43778e571 100644 --- a/drivers/dma-buf/Makefile +++ b/drivers/dma-buf/Makefile @@ -1,6 +1,12 @@ # SPDX-License-Identifier: GPL-2.0-only -obj-y := dma-buf.o dma-fence.o dma-fence-array.o dma-fence-chain.o \

• reservation.o seqno-fence.o
  

+obj-y := \

• dma-buf.o \
• dma-fence.o \
• dma-fence-array.o \
• dma-fence-chain.o \
• dma-fence-trace.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-trace.c b/drivers/dma-buf/dma-fence-trace.c new file mode 100644 index 000000000000..eb6f282be4c0 --- /dev/null +++ b/drivers/dma-buf/dma-fence-trace.c @@ -0,0 +1,28 @@ +/*

• • Fence mechanism for dma-buf and to allow for asynchronous dma access
• • Copyright (C) 2012 Canonical Ltd
• • Copyright (C) 2012 Texas Instruments
• • Authors:
• • Rob Clark robdclark@gmail.com
• • Maarten Lankhorst maarten.lankhorst@canonical.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-types.h>

+#define CREATE_TRACE_POINTS +#include <trace/events/dma_fence.h>

+EXPORT_TRACEPOINT_SYMBOL(dma_fence_emit); +EXPORT_TRACEPOINT_SYMBOL(dma_fence_enable_signal); +EXPORT_TRACEPOINT_SYMBOL(dma_fence_signaled); diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 59ac96ec7ba8..027a6a894abd 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -14,15 +14,9 @@ #include <linux/export.h> #include <linux/atomic.h> #include <linux/dma-fence.h> +#include <linux/dma-fence-impl.h> #include <linux/sched/signal.h>

-#define CREATE_TRACE_POINTS -#include <trace/events/dma_fence.h>

-EXPORT_TRACEPOINT_SYMBOL(dma_fence_emit); -EXPORT_TRACEPOINT_SYMBOL(dma_fence_enable_signal); -EXPORT_TRACEPOINT_SYMBOL(dma_fence_signaled);

• static DEFINE_SPINLOCK(dma_fence_stub_lock); static struct dma_fence dma_fence_stub;

@@ -128,7 +122,6 @@ EXPORT_SYMBOL(dma_fence_context_alloc); */ int dma_fence_signal_locked(struct dma_fence *fence) {

• struct dma_fence_cb *cur, *tmp; int ret = 0;

  lockdep_assert_held(fence->lock);

@@ -136,7 +129,7 @@ int dma_fence_signal_locked(struct dma_fence *fence) if (WARN_ON(!fence)) return -EINVAL;

• if (test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {

• if (!__dma_fence_signal(fence)) { ret = -EINVAL;

  /*

@@ -144,15 +137,10 @@ int dma_fence_signal_locked(struct dma_fence *fence) * still run through all callbacks */ } else {

• fence->timestamp = ktime_get();
  

• set_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags);
  

• trace_dma_fence_signaled(fence);
  

• __dma_fence_signal__timestamp(fence, ktime_get());
  

  }

• list_for_each_entry_safe(cur, tmp, &fence->cb_list, node) {

• list_del_init(&cur->node);
  

• cur->func(fence, cur);
  

• }

• __dma_fence_signal__notify(fence); return ret; } EXPORT_SYMBOL(dma_fence_signal_locked);

@@ -177,21 +165,14 @@ int dma_fence_signal(struct dma_fence *fence) if (!fence) return -EINVAL;

• if (test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))

• if (!__dma_fence_signal(fence)) return -EINVAL;

• fence->timestamp = ktime_get();
• set_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags);
• trace_dma_fence_signaled(fence);

• __dma_fence_signal__timestamp(fence, ktime_get());

  if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &fence->flags)) {

• struct dma_fence_cb *cur, *tmp;
  

• spin_lock_irqsave(fence->lock, flags);

• list_for_each_entry_safe(cur, tmp, &fence->cb_list, node) {
  

• 	list_del_init(&cur->node);
  

• 	cur->func(fence, cur);
  

• }
  

• __dma_fence_signal__notify(fence);
  

  spin_unlock_irqrestore(fence->lock, flags); } return 0;

diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c index e1bbc9b428cd..65de5c45a233 100644 --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c @@ -22,8 +22,7 @@

*/

-#include <linux/kthread.h> -#include <trace/events/dma_fence.h> +#include <linux/dma-fence-impl.h> #include <uapi/linux/sched/types.h>

#include "i915_drv.h" @@ -98,35 +97,6 @@ check_signal_order(struct intel_context *ce, struct i915_request *rq) return true; }

-static bool -__dma_fence_signal(struct dma_fence *fence) -{

• return !test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags);

-}

-static void -__dma_fence_signal__timestamp(struct dma_fence *fence, ktime_t timestamp) -{

• fence->timestamp = timestamp;
• set_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags);
• trace_dma_fence_signaled(fence);

-}

-static void -__dma_fence_signal__notify(struct dma_fence *fence) -{

• struct dma_fence_cb *cur, *tmp;
• lockdep_assert_held(fence->lock);
• lockdep_assert_irqs_disabled();
• list_for_each_entry_safe(cur, tmp, &fence->cb_list, node) {

• INIT_LIST_HEAD(&cur->node);
  

• cur->func(fence, cur);
  

• }
• INIT_LIST_HEAD(&fence->cb_list);

-}

• void intel_engine_breadcrumbs_irq(struct intel_engine_cs *engine) { struct intel_breadcrumbs *b = &engine->breadcrumbs;

diff --git a/include/linux/dma-fence-impl.h b/include/linux/dma-fence-impl.h new file mode 100644 index 000000000000..7372021cf082 --- /dev/null +++ b/include/linux/dma-fence-impl.h @@ -0,0 +1,83 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/*

• • Fence mechanism for dma-buf to allow for asynchronous dma access
• • Copyright (C) 2012 Canonical Ltd
• • Copyright (C) 2012 Texas Instruments
• • Authors:
• • Rob Clark robdclark@gmail.com
• • Maarten Lankhorst maarten.lankhorst@canonical.com
• */

+#ifndef __LINUX_DMA_FENCE_IMPL_H +#define __LINUX_DMA_FENCE_IMPL_H

+#include <linux/dma-fence.h> +#include <linux/lockdep.h> +#include <linux/list.h> +#include <linux/ktime.h>

+#include <trace/events/dma_fence.h>

+/**

• • __dma_fence_signal: Mark a fence as signaled
• • @fence: the dma fence to mark
• • The first step of the dma_fence_signal() implementation is to atomically
• • mark the fence as signaled.
• • Returns: true if the fence was not previously signaled, false if it was
• • already signaled.
• */

+static inline bool +__dma_fence_signal(struct dma_fence *fence) +{

• return !test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags);

+}

+/**

• • __dma_fence_signal__timestamp: sets the signaling timestamp
• • @fence: the dma fence
• • @timestamp: the monotonic timestamp (e.g. ktime_get_mono())
• • The second step of the dma_fence_signal() implementation it to record
• • the siganling timestamp.
• • The dma-fence stores a timestamp of when it was signaled for inspection
• • by userspace. This timestamp is typically the CPU time at which the
• • signal was raised, but could be a HW timestamp generated by the event
• • itself. Either way, it must be set on the signaled fence before
• • callbacks are notified.
• */

+static inline void +__dma_fence_signal__timestamp(struct dma_fence *fence, ktime_t timestamp) +{

• fence->timestamp = timestamp;
• set_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags);
• trace_dma_fence_signaled(fence);

+}

+/**

• • __dma_fence_signal__notify: notify observers of the signal event
• • @fence: the dma fence
• • The final step of the dma_fence_signal() implementation is to notify
• • all observers (dma_fence_add_callback()) of the signal event. This must
• • be called with the fence->lock already held and irqsoff.
• */

+static inline void +__dma_fence_signal__notify(struct dma_fence *fence) +{

• struct dma_fence_cb *cur, *tmp;
• lockdep_assert_held(fence->lock);
• list_for_each_entry_safe(cur, tmp, &fence->cb_list, node) {

• INIT_LIST_HEAD(&cur->node);
  

• cur->func(fence, cur);
  

• }
• INIT_LIST_HEAD(&fence->cb_list);

+}

+#endif /* __LINUX_DMA_FENCE_IMPL_H */ diff --git a/include/linux/dma-fence-types.h b/include/linux/dma-fence-types.h new file mode 100644 index 000000000000..81e22d9ed174 --- /dev/null +++ b/include/linux/dma-fence-types.h @@ -0,0 +1,258 @@ +/*

• • Fence mechanism for dma-buf to allow for asynchronous dma access
• • Copyright (C) 2012 Canonical Ltd
• • Copyright (C) 2012 Texas Instruments
• • Authors:
• • Rob Clark robdclark@gmail.com
• • Maarten Lankhorst maarten.lankhorst@canonical.com
• • This program is free software; you can redistribute it and/or modify it
• • under the terms of the GNU General Public License version 2 as published by
• • the Free Software Foundation.
• • This program is distributed in the hope that it will be useful, but WITHOUT
• • ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
• • FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
• • more details.
• */

+#ifndef __LINUX_DMA_FENCE_TYPES_H +#define __LINUX_DMA_FENCE_TYPES_H

+#include <linux/list.h> +#include <linux/kref.h> +#include <linux/ktime.h> +#include <linux/rcupdate.h> +#include <linux/spinlock.h> +#include <linux/types.h>

+struct dma_fence; +struct dma_fence_ops; +struct dma_fence_cb;

+/**

• • struct dma_fence - software synchronization primitive
• • @refcount: refcount for this fence
• • @ops: dma_fence_ops associated with this fence
• • @rcu: used for releasing fence with kfree_rcu
• • @cb_list: list of all callbacks to call
• • @lock: spin_lock_irqsave used for locking
• • @context: execution context this fence belongs to, returned by

• •       dma_fence_context_alloc()
    

• • @seqno: the sequence number of this fence inside the execution context,
• • can be compared to decide which fence would be signaled later.
• • @flags: A mask of DMA_FENCE_FLAG_* defined below
• • @timestamp: Timestamp when the fence was signaled.
• • @error: Optional, only valid if < 0, must be set before calling
• • dma_fence_signal, indicates that the fence has completed with an error.
• • the flags member must be manipulated and read using the appropriate
• • atomic ops (bit_*), so taking the spinlock will not be needed most
• • of the time.
• • DMA_FENCE_FLAG_SIGNALED_BIT - fence is already signaled
• • DMA_FENCE_FLAG_TIMESTAMP_BIT - timestamp recorded for fence signaling
• • DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT - enable_signaling might have been called
• • DMA_FENCE_FLAG_USER_BITS - start of the unused bits, can be used by the
• • implementer of the fence for its own purposes. Can be used in different
• • ways by different fence implementers, so do not rely on this.
• • Since atomic bitops are used, this is not guaranteed to be the case.
• • Particularly, if the bit was set, but dma_fence_signal was called right
• • before this bit was set, it would have been able to set the
• • DMA_FENCE_FLAG_SIGNALED_BIT, before enable_signaling was called.
• • Adding a check for DMA_FENCE_FLAG_SIGNALED_BIT after setting
• • DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT closes this race, and makes sure that
• • after dma_fence_signal was called, any enable_signaling call will have either
• • been completed, or never called at all.
• */

+struct dma_fence {

• struct kref refcount;
• const struct dma_fence_ops *ops;
• /* We clear the callback list on kref_put so that by the time we

• * release the fence it is unused. No one should be adding to the cb_list
  

• * that they don't themselves hold a reference for.
  

• */
  

• union {

• struct rcu_head rcu;
  

• struct list_head cb_list;
  

• };
• spinlock_t *lock;
• u64 context;
• u64 seqno;
• unsigned long flags;
• ktime_t timestamp;
• int error;

+};

+enum dma_fence_flag_bits {

• DMA_FENCE_FLAG_SIGNALED_BIT,
• DMA_FENCE_FLAG_TIMESTAMP_BIT,
• DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
• DMA_FENCE_FLAG_USER_BITS, /* must always be last member */

+};

+typedef void (*dma_fence_func_t)(struct dma_fence *fence,

• 		 struct dma_fence_cb *cb);
  

+/**

• • struct dma_fence_cb - callback for dma_fence_add_callback()
• • @node: used by dma_fence_add_callback() to append this struct to fence::cb_list
• • @func: dma_fence_func_t to call
• • This struct will be initialized by dma_fence_add_callback(), additional
• • data can be passed along by embedding dma_fence_cb in another struct.
• */

+struct dma_fence_cb {

• struct list_head node;
• dma_fence_func_t func;

+};

+/**

• • struct dma_fence_ops - operations implemented for fence
• */

+struct dma_fence_ops {

• /**

• * @use_64bit_seqno:
  

• *
  

• * True if this dma_fence implementation uses 64bit seqno, false
  

• * otherwise.
  

• */
  

• bool use_64bit_seqno;
• /**

• * @get_driver_name:
  

• *
  

• * Returns the driver name. This is a callback to allow drivers to
  

• * compute the name at runtime, without having it to store permanently
  

• * for each fence, or build a cache of some sort.
  

• *
  

• * This callback is mandatory.
  

• */
  

• const char * (*get_driver_name)(struct dma_fence *fence);
• /**

• * @get_timeline_name:
  

• *
  

• * Return the name of the context this fence belongs to. This is a
  

• * callback to allow drivers to compute the name at runtime, without
  

• * having it to store permanently for each fence, or build a cache of
  

• * some sort.
  

• *
  

• * This callback is mandatory.
  

• */
  

• const char * (*get_timeline_name)(struct dma_fence *fence);
• /**

• * @enable_signaling:
  

• *
  

• * Enable software signaling of fence.
  

• *
  

• * For fence implementations that have the capability for hw->hw
  

• * signaling, they can implement this op to enable the necessary
  

• * interrupts, or insert commands into cmdstream, etc, to avoid these
  

• * costly operations for the common case where only hw->hw
  

• * synchronization is required.  This is called in the first
  

• * dma_fence_wait() or dma_fence_add_callback() path to let the fence
  

• * implementation know that there is another driver waiting on the
  

• * signal (ie. hw->sw case).
  

• *
  

• * This function can be called from atomic context, but not
  

• * from irq context, so normal spinlocks can be used.
  

• *
  

• * A return value of false indicates the fence already passed,
  

• * or some failure occurred that made it impossible to enable
  

• * signaling. True indicates successful enabling.
  

• *
  

• * &dma_fence.error may be set in enable_signaling, but only when false
  

• * is returned.
  

• *
  

• * Since many implementations can call dma_fence_signal() even before
  

• * @enable_signaling has been called there's a race window, where the
  

• * dma_fence_signal() might result in the final fence reference being
  

• * released and its memory freed. To avoid this, implementations of
  

• * this callback should grab their own reference using dma_fence_get(),
  

• * to be released when the fence is signalled (through e.g. the
  

• * interrupt handler).
  

• *
  

• * This callback is optional. If this callback is not present, then the
  

• * driver must always have signaling enabled.
  

• */
  

• bool (*enable_signaling)(struct dma_fence *fence);
• /**

• * @signaled:
  

• *
  

• * Peek whether the fence is signaled, as a fastpath optimization for
  

• * e.g. dma_fence_wait() or dma_fence_add_callback(). Note that this
  

• * callback does not need to make any guarantees beyond that a fence
  

• * once indicates as signalled must always return true from this
  

• * callback. This callback may return false even if the fence has
  

• * completed already, in this case information hasn't propogated through
  

• * the system yet. See also dma_fence_is_signaled().
  

• *
  

• * May set &dma_fence.error if returning true.
  

• *
  

• * This callback is optional.
  

• */
  

• bool (*signaled)(struct dma_fence *fence);
• /**

• * @wait:
  

• *
  

• * Custom wait implementation, defaults to dma_fence_default_wait() if
  

• * not set.
  

• *
  

• * The dma_fence_default_wait implementation should work for any fence,
  

• * as long as @enable_signaling works correctly. This hook allows
  

• * drivers to have an optimized version for the case where a process
  

• * context is already available, e.g. if @enable_signaling for the
  

• * general case needs to set up a worker thread.
  

• *
  

• * Must return -ERESTARTSYS if the wait is intr = true and the wait was
  

• * interrupted, and remaining jiffies if fence has signaled, or 0 if
  

• * wait timed out. Can also return other error values on custom
  

• * implementations, which should be treated as if the fence is signaled.
  

• * For example a hardware lockup could be reported like that.
  

• *
  

• * This callback is optional.
  

• */
  

• signed long (*wait)(struct dma_fence *fence,

• 	    bool intr, signed long timeout);
  

• /**

• * @release:
  

• *
  

• * Called on destruction of fence to release additional resources.
  

• * Can be called from irq context.  This callback is optional. If it is
  

• * NULL, then dma_fence_free() is instead called as the default
  

• * implementation.
  

• */
  

• void (*release)(struct dma_fence *fence);
• /**

• * @fence_value_str:
  

• *
  

• * Callback to fill in free-form debug info specific to this fence, like
  

• * the sequence number.
  

• *
  

• * This callback is optional.
  

• */
  

• void (*fence_value_str)(struct dma_fence *fence, char *str, int size);
• /**

• * @timeline_value_str:
  

• *
  

• * Fills in the current value of the timeline as a string, like the
  

• * sequence number. Note that the specific fence passed to this function
  

• * should not matter, drivers should only use it to look up the
  

• * corresponding timeline structures.
  

• */
  

• void (*timeline_value_str)(struct dma_fence *fence,

• 		   char *str, int size);
  

+};

+#endif /* __LINUX_DMA_FENCE_TYPES_H */ diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h index bea1d05cf51e..1c8dd1fbafae 100644 --- a/include/linux/dma-fence.h +++ b/include/linux/dma-fence.h @@ -13,6 +13,7 @@ #ifndef __LINUX_DMA_FENCE_H #define __LINUX_DMA_FENCE_H

+#include <linux/dma-fence-types.h> #include <linux/err.h> #include <linux/wait.h> #include <linux/list.h> @@ -22,233 +23,6 @@ #include <linux/printk.h> #include <linux/rcupdate.h>

-struct dma_fence; -struct dma_fence_ops; -struct dma_fence_cb;

-/**

• • struct dma_fence - software synchronization primitive
• • @refcount: refcount for this fence
• • @ops: dma_fence_ops associated with this fence
• • @rcu: used for releasing fence with kfree_rcu
• • @cb_list: list of all callbacks to call
• • @lock: spin_lock_irqsave used for locking
• • @context: execution context this fence belongs to, returned by

• •       dma_fence_context_alloc()
    

• • @seqno: the sequence number of this fence inside the execution context,
• • can be compared to decide which fence would be signaled later.
• • @flags: A mask of DMA_FENCE_FLAG_* defined below
• • @timestamp: Timestamp when the fence was signaled.
• • @error: Optional, only valid if < 0, must be set before calling
• • dma_fence_signal, indicates that the fence has completed with an error.
• • the flags member must be manipulated and read using the appropriate
• • atomic ops (bit_*), so taking the spinlock will not be needed most
• • of the time.
• • DMA_FENCE_FLAG_SIGNALED_BIT - fence is already signaled
• • DMA_FENCE_FLAG_TIMESTAMP_BIT - timestamp recorded for fence signaling
• • DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT - enable_signaling might have been called
• • DMA_FENCE_FLAG_USER_BITS - start of the unused bits, can be used by the
• • implementer of the fence for its own purposes. Can be used in different
• • ways by different fence implementers, so do not rely on this.
• • Since atomic bitops are used, this is not guaranteed to be the case.
• • Particularly, if the bit was set, but dma_fence_signal was called right
• • before this bit was set, it would have been able to set the
• • DMA_FENCE_FLAG_SIGNALED_BIT, before enable_signaling was called.
• • Adding a check for DMA_FENCE_FLAG_SIGNALED_BIT after setting
• • DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT closes this race, and makes sure that
• • after dma_fence_signal was called, any enable_signaling call will have either
• • been completed, or never called at all.
• */

-struct dma_fence {

• struct kref refcount;
• const struct dma_fence_ops *ops;
• /* We clear the callback list on kref_put so that by the time we

• * release the fence it is unused. No one should be adding to the cb_list
  

• * that they don't themselves hold a reference for.
  

• */
  

• union {

• struct rcu_head rcu;
  

• struct list_head cb_list;
  

• };
• spinlock_t *lock;
• u64 context;
• u64 seqno;
• unsigned long flags;
• ktime_t timestamp;
• int error;

-};

-enum dma_fence_flag_bits {

• DMA_FENCE_FLAG_SIGNALED_BIT,
• DMA_FENCE_FLAG_TIMESTAMP_BIT,
• DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
• DMA_FENCE_FLAG_USER_BITS, /* must always be last member */

-};

-typedef void (*dma_fence_func_t)(struct dma_fence *fence,

• 		 struct dma_fence_cb *cb);
  

-/**

• • struct dma_fence_cb - callback for dma_fence_add_callback()
• • @node: used by dma_fence_add_callback() to append this struct to fence::cb_list
• • @func: dma_fence_func_t to call
• • This struct will be initialized by dma_fence_add_callback(), additional
• • data can be passed along by embedding dma_fence_cb in another struct.
• */

-struct dma_fence_cb {

• struct list_head node;
• dma_fence_func_t func;

-};

-/**

• • struct dma_fence_ops - operations implemented for fence
• */

-struct dma_fence_ops {

• /**

• * @use_64bit_seqno:
  

• *
  

• * True if this dma_fence implementation uses 64bit seqno, false
  

• * otherwise.
  

• */
  

• bool use_64bit_seqno;
• /**

• * @get_driver_name:
  

• *
  

• * Returns the driver name. This is a callback to allow drivers to
  

• * compute the name at runtime, without having it to store permanently
  

• * for each fence, or build a cache of some sort.
  

• *
  

• * This callback is mandatory.
  

• */
  

• const char * (*get_driver_name)(struct dma_fence *fence);
• /**

• * @get_timeline_name:
  

• *
  

• * Return the name of the context this fence belongs to. This is a
  

• * callback to allow drivers to compute the name at runtime, without
  

• * having it to store permanently for each fence, or build a cache of
  

• * some sort.
  

• *
  

• * This callback is mandatory.
  

• */
  

• const char * (*get_timeline_name)(struct dma_fence *fence);
• /**

• * @enable_signaling:
  

• *
  

• * Enable software signaling of fence.
  

• *
  

• * For fence implementations that have the capability for hw->hw
  

• * signaling, they can implement this op to enable the necessary
  

• * interrupts, or insert commands into cmdstream, etc, to avoid these
  

• * costly operations for the common case where only hw->hw
  

• * synchronization is required.  This is called in the first
  

• * dma_fence_wait() or dma_fence_add_callback() path to let the fence
  

• * implementation know that there is another driver waiting on the
  

• * signal (ie. hw->sw case).
  

• *
  

• * This function can be called from atomic context, but not
  

• * from irq context, so normal spinlocks can be used.
  

• *
  

• * A return value of false indicates the fence already passed,
  

• * or some failure occurred that made it impossible to enable
  

• * signaling. True indicates successful enabling.
  

• *
  

• * &dma_fence.error may be set in enable_signaling, but only when false
  

• * is returned.
  

• *
  

• * Since many implementations can call dma_fence_signal() even when before
  

• * @enable_signaling has been called there's a race window, where the
  

• * dma_fence_signal() might result in the final fence reference being
  

• * released and its memory freed. To avoid this, implementations of this
  

• * callback should grab their own reference using dma_fence_get(), to be
  

• * released when the fence is signalled (through e.g. the interrupt
  

• * handler).
  

• *
  

• * This callback is optional. If this callback is not present, then the
  

• * driver must always have signaling enabled.
  

• */
  

• bool (*enable_signaling)(struct dma_fence *fence);
• /**

• * @signaled:
  

• *
  

• * Peek whether the fence is signaled, as a fastpath optimization for
  

• * e.g. dma_fence_wait() or dma_fence_add_callback(). Note that this
  

• * callback does not need to make any guarantees beyond that a fence
  

• * once indicates as signalled must always return true from this
  

• * callback. This callback may return false even if the fence has
  

• * completed already, in this case information hasn't propogated throug
  

• * the system yet. See also dma_fence_is_signaled().
  

• *
  

• * May set &dma_fence.error if returning true.
  

• *
  

• * This callback is optional.
  

• */
  

• bool (*signaled)(struct dma_fence *fence);
• /**

• * @wait:
  

• *
  

• * Custom wait implementation, defaults to dma_fence_default_wait() if
  

• * not set.
  

• *
  

• * The dma_fence_default_wait implementation should work for any fence, as long
  

• * as @enable_signaling works correctly. This hook allows drivers to
  

• * have an optimized version for the case where a process context is
  

• * already available, e.g. if @enable_signaling for the general case
  

• * needs to set up a worker thread.
  

• *
  

• * Must return -ERESTARTSYS if the wait is intr = true and the wait was
  

• * interrupted, and remaining jiffies if fence has signaled, or 0 if wait
  

• * timed out. Can also return other error values on custom implementations,
  

• * which should be treated as if the fence is signaled. For example a hardware
  

• * lockup could be reported like that.
  

• *
  

• * This callback is optional.
  

• */
  

• signed long (*wait)(struct dma_fence *fence,

• 	    bool intr, signed long timeout);
  

• /**

• * @release:
  

• *
  

• * Called on destruction of fence to release additional resources.
  

• * Can be called from irq context.  This callback is optional. If it is
  

• * NULL, then dma_fence_free() is instead called as the default
  

• * implementation.
  

• */
  

• void (*release)(struct dma_fence *fence);
• /**

• * @fence_value_str:
  

• *
  

• * Callback to fill in free-form debug info specific to this fence, like
  

• * the sequence number.
  

• *
  

• * This callback is optional.
  

• */
  

• void (*fence_value_str)(struct dma_fence *fence, char *str, int size);
• /**

• * @timeline_value_str:
  

• *
  

• * Fills in the current value of the timeline as a string, like the
  

• * sequence number. Note that the specific fence passed to this function
  

• * should not matter, drivers should only use it to look up the
  

• * corresponding timeline structures.
  

• */
  

• void (*timeline_value_str)(struct dma_fence *fence,

• 		   char *str, int size);
  

-};

• void dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, spinlock_t *lock, u64 context, u64 seqno);

Am 10.08.19 um 17:34 schrieb Chris Wilson:

Allow for some users to surreptitiously insert lazy signal callbacks that do not depend on enabling the signaling mechanism around every fence. (The cost of interrupts is too darn high, to revive an old meme.) This means that we may have a cb_list even if the signaling bit is not enabled, so always notify the callbacks.

The cost is that dma_fence_signal() must always acquire the spinlock to ensure that the cb_list is flushed.

Signed-off-by: Chris Wilson chris@chris-wilson.co.uk

drivers/dma-buf/dma-fence.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 027a6a894abd..ab4a456bba04 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -170,11 +170,9 @@ int dma_fence_signal(struct dma_fence *fence)

__dma_fence_signal__timestamp(fence, ktime_get());

• if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &fence->flags)) {

• spin_lock_irqsave(fence->lock, flags);
  

• __dma_fence_signal__notify(fence);
  

• spin_unlock_irqrestore(fence->lock, flags);
  

• }

• spin_lock_irqsave(fence->lock, flags);
• __dma_fence_signal__notify(fence);
• spin_unlock_irqrestore(fence->lock, flags);

If we now always grab the spinlock anyway I suggest to rather merge dma_fence_signal and dma_fence_signal_locked.

Christian.

return 0; } EXPORT_SYMBOL(dma_fence_signal);

Am 11.08.19 um 11:15 schrieb Chris Wilson:

Now that dma_fence_signal always takes the spinlock to flush the cb_list, simply take the spinlock and call dma_fence_signal_locked() to avoid code repetition.

Suggested-by: Christian König christian.koenig@amd.com Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Christian König christian.koenig@amd.com

Reviewed-by: Christian König christian.koenig@amd.com

---

drivers/dma-buf/dma-fence.c | 32 ++++++++++++-------------------- 1 file changed, 12 insertions(+), 20 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index ab4a456bba04..367b71084d34 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -122,26 +122,18 @@ EXPORT_SYMBOL(dma_fence_context_alloc); */ int dma_fence_signal_locked(struct dma_fence *fence) {

• int ret = 0;

• lockdep_assert_held(fence->lock);

• if (WARN_ON(!fence)) return -EINVAL;

• if (!__dma_fence_signal(fence)) {

• ret = -EINVAL;
  

• lockdep_assert_held(fence->lock);

• /*
  

•  * we might have raced with the unlocked dma_fence_signal,
  

•  * still run through all callbacks
  

•  */
  

• } else {

• __dma_fence_signal__timestamp(fence, ktime_get());
  

• }

• if (!__dma_fence_signal(fence))

• return -EINVAL;
  
  

• __dma_fence_signal__timestamp(fence, ktime_get()); __dma_fence_signal__notify(fence);

• return ret;

• return 0; } EXPORT_SYMBOL(dma_fence_signal_locked);

@@ -161,19 +153,19 @@ EXPORT_SYMBOL(dma_fence_signal_locked); int dma_fence_signal(struct dma_fence *fence) { unsigned long flags;

• int ret;

  if (!fence) return -EINVAL;

• if (!__dma_fence_signal(fence))

• return -EINVAL;
  

• __dma_fence_signal__timestamp(fence, ktime_get());

• if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))

• return 0;
  
  

  spin_lock_irqsave(fence->lock, flags);

• __dma_fence_signal__notify(fence);

• ret = dma_fence_signal_locked(fence); spin_unlock_irqrestore(fence->lock, flags);

• return 0;

• return ret; } EXPORT_SYMBOL(dma_fence_signal);

Quoting Daniel Vetter (2019-08-15 19:48:42)

On Thu, Aug 15, 2019 at 8:46 PM Chris Wilson chris@chris-wilson.co.uk wrote:

...

Quoting Daniel Vetter (2019-08-14 18:20:53)

...

On Sun, Aug 11, 2019 at 10:15:23AM +0100, Chris Wilson wrote:

...

Now that dma_fence_signal always takes the spinlock to flush the cb_list, simply take the spinlock and call dma_fence_signal_locked() to avoid code repetition.

Suggested-by: Christian König christian.koenig@amd.com Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Christian König christian.koenig@amd.com

Hm, I think this largely defeats the point of having the lockless signal enabling trickery in dma_fence. Maybe that part isn't needed by anyone, but feels like a thing that needs a notch more thought. And if we need it, maybe a bit more cleanup.

You mean dma_fence_enable_sw_signaling(). The only user appears to be to flush fences, which is actually the intent of always notifying the signal cb. By always doing the callbacks, we can avoid installing the interrupt and completely saturating CPUs with irqs, instead doing a batch in a leisurely timer callback if not flushed naturally.

Yeah I'm not against ditching this,

I was just thinking aloud working out what the current use case in ttm was for.

but can't we ditch a lot more if we just always take the spinlock in those paths now anyways? Kinda not worth having the complexity anymore.

You would be able to drop the was_set from dma_fence_add_callback. Say

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 59ac96ec7ba8..e566445134a2 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -345,38 +345,31 @@ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb, dma_fence_func_t func) { unsigned long flags; - int ret = 0; - bool was_set; + int ret = -ENOENT;

if (WARN_ON(!fence || !func)) return -EINVAL;

- if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) { - INIT_LIST_HEAD(&cb->node); + INIT_LIST_HEAD(&cb->node); + cb->func = func; + + if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) return -ENOENT; - }

spin_lock_irqsave(fence->lock, flags); - - was_set = test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, - &fence->flags); - - if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) - ret = -ENOENT; - else if (!was_set && fence->ops->enable_signaling) { + if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags) && + !test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, + &fence->flags)) { trace_dma_fence_enable_signal(fence);

- if (!fence->ops->enable_signaling(fence)) { + if (!fence->ops->enable_signaling || + fence->ops->enable_signaling(fence)) { + list_add_tail(&cb->node, &fence->cb_list); + ret = 0; + } else { dma_fence_signal_locked(fence); - ret = -ENOENT; } } - - if (!ret) { - cb->func = func; - list_add_tail(&cb->node, &fence->cb_list); - } else - INIT_LIST_HEAD(&cb->node); spin_unlock_irqrestore(fence->lock, flags);

return ret;

Not a whole lot changes in terms of branches and serialising instructions. One less baffling sequence to worry about. -Chris

Am 15.08.19 um 21:29 schrieb Chris Wilson:

Quoting Chris Wilson (2019-08-15 20:03:13)

...

Quoting Daniel Vetter (2019-08-15 19:48:42)

...

On Thu, Aug 15, 2019 at 8:46 PM Chris Wilson chris@chris-wilson.co.uk wrote:

...

Quoting Daniel Vetter (2019-08-14 18:20:53)

...

On Sun, Aug 11, 2019 at 10:15:23AM +0100, Chris Wilson wrote:

...

Now that dma_fence_signal always takes the spinlock to flush the cb_list, simply take the spinlock and call dma_fence_signal_locked() to avoid code repetition.

Suggested-by: Christian König christian.koenig@amd.com Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Christian König christian.koenig@amd.com

Hm, I think this largely defeats the point of having the lockless signal enabling trickery in dma_fence. Maybe that part isn't needed by anyone, but feels like a thing that needs a notch more thought. And if we need it, maybe a bit more cleanup.

You mean dma_fence_enable_sw_signaling(). The only user appears to be to flush fences, which is actually the intent of always notifying the signal cb. By always doing the callbacks, we can avoid installing the interrupt and completely saturating CPUs with irqs, instead doing a batch in a leisurely timer callback if not flushed naturally.

Yeah I'm not against ditching this,

I was just thinking aloud working out what the current use case in ttm was for.

...

but can't we ditch a lot more if we just always take the spinlock in those paths now anyways? Kinda not worth having the complexity anymore.

You would be able to drop the was_set from dma_fence_add_callback. Say

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 59ac96ec7ba8..e566445134a2 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -345,38 +345,31 @@ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb, dma_fence_func_t func) { unsigned long flags;

•   int ret = 0;
  

•   bool was_set;
  

•   int ret = -ENOENT;
  
     if (WARN_ON(!fence || !func))
             return -EINVAL;
  
  

•   if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
  

•           INIT_LIST_HEAD(&cb->node);
  

•   INIT_LIST_HEAD(&cb->node);
  

•   cb->func = func;
  

•   if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
             return -ENOENT;
  

•   }
  
     spin_lock_irqsave(fence->lock, flags);
  

•   was_set = test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
  

•                              &fence->flags);
  

•   if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
  

•           ret = -ENOENT;
  

•   else if (!was_set && fence->ops->enable_signaling) {
  

•   if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags) &&
  

•       !test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
  

•                         &fence->flags)) {
             trace_dma_fence_enable_signal(fence);
  
  

•           if (!fence->ops->enable_signaling(fence)) {
  

•           if (!fence->ops->enable_signaling ||
  

•               fence->ops->enable_signaling(fence)) {
  

•                   list_add_tail(&cb->node, &fence->cb_list);
  

•                   ret = 0;
  

•           } else {
                     dma_fence_signal_locked(fence);
  

•                   ret = -ENOENT;
             }
     }
  

•   if (!ret) {
  

•           cb->func = func;
  

•           list_add_tail(&cb->node, &fence->cb_list);
  

•   } else
  

•           INIT_LIST_HEAD(&cb->node);
     spin_unlock_irqrestore(fence->lock, flags);
  
     return ret;
  
  

Not a whole lot changes in terms of branches and serialising instructions. One less baffling sequence to worry about.

Fwiw, Function old new delta dma_fence_add_callback 338 302 -36

Well since the sequence number change didn't worked out I'm now working on something where I replaced the shared fences list with a reference counted version where we also have an active and staged view of the fences.

This removed the whole problem of keeping things alive while inside the RCU and also removes the retry looping etc.. Additional to that we can also get rid of most of the memory barriers while adding and manipulating fences.

The end result in a totally artificial command submission test case is a 61% performance improvement. This is so much that I'm actually still searching if that is not caused by bug somewhere.

Will probably need some more weeks till this is done, but yeah there is a huge potential for optimization here, Christian.

Almost certainly more shaving if you stare. -Chris

Quoting Koenig, Christian (2019-08-11 09:58:31)

Am 10.08.19 um 17:34 schrieb Chris Wilson:

...

• /*
  

•  * Propagate the first error reported by any of our fences, but only
  

•  * before we ourselves are signaled.
  

•  */
  

• if (atomic_read(&array->num_pending) > 0)
  

•         fence_set_error_once(&array->base, f->error);
  

That is racy even if you check the atomic because num_pending can be initialized to 1 for signal any arrays as well.

We both agree that we don't care about the potential write tearing if two errors occur simultaneous, either error will do for our error?

So it's just the matter of not marking the array as being in error if we have already signaled.

I suggest to rather test in dma_fence_array_set_error_once if we got an error and if yes grab the sequence lock and test if we are already signaled or not.

How about embracing the race with something like,

diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c index d90675bb4fcc..c71c57d25e48 100644 --- a/drivers/dma-buf/dma-fence-array.c +++ b/drivers/dma-buf/dma-fence-array.c @@ -33,6 +33,8 @@ static void irq_dma_fence_array_work(struct irq_work *wrk) { struct dma_fence_array *array = container_of(wrk, typeof(*array), work);

+ fence_set_error_once(&array->base, READ_ONCE(array->pending_error)); + dma_fence_signal(&array->base); dma_fence_put(&array->base); } @@ -48,8 +50,8 @@ static void dma_fence_array_cb_func(struct dma_fence *f, * Propagate the first error reported by any of our fences, but only * before we ourselves are signaled. */ - if (atomic_read(&array->num_pending) > 0) - fence_set_error_once(&array->base, f->error); + if (f->error && !array->pending_error) + WRITE_ONCE(array->pending_error, f->error);

if (atomic_dec_and_test(&array->num_pending)) irq_work_queue(&array->work); @@ -156,6 +158,7 @@ struct dma_fence_array *dma_fence_array_create(int num_fences, array->num_fences = num_fences; atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences); array->fences = fences; + array->pending_error = 0;

return array; } diff --git a/include/linux/dma-fence-array.h b/include/linux/dma-fence-array.h index 303dd712220f..faaf70c524ae 100644 --- a/include/linux/dma-fence-array.h +++ b/include/linux/dma-fence-array.h @@ -42,6 +42,8 @@ struct dma_fence_array { atomic_t num_pending; struct dma_fence **fences;

+ int pending_error; + struct irq_work work; };

That ensures there is no race between signaling and raising the error, but accepts that multiple fences may try and raise an error. There is still the potential for the signal-on-any to be flagged as an error by the second fence, but I claim that race is immaterial as the second fence could have been the signaler. -Chris

When one of the array of fences is signaled, propagate its errors to the parent fence-array (keeping the first error to be raised).

v2: Opencode cmpxchg_local to avoid compiler freakout. v3: Be careful not to flag an error if we race against signal-on-any. v4: Same applies to installing the signal cb. v5: Use cmpxchg to only set the error once before using a nifty idea by Christian to avoid changing the status after emitting the signal.

Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Sumit Semwal sumit.semwal@linaro.org Cc: Gustavo Padovan gustavo@padovan.org Cc: Christian König christian.koenig@amd.com --- drivers/dma-buf/dma-fence-array.c | 32 ++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c index 12c6f64c0bc2..d3fbd950be94 100644 --- a/drivers/dma-buf/dma-fence-array.c +++ b/drivers/dma-buf/dma-fence-array.c @@ -13,6 +13,8 @@ #include <linux/slab.h> #include <linux/dma-fence-array.h>

+#define PENDING_ERROR 1 + static const char *dma_fence_array_get_driver_name(struct dma_fence *fence) { return "dma_fence_array"; @@ -23,10 +25,29 @@ static const char *dma_fence_array_get_timeline_name(struct dma_fence *fence) return "unbound"; }

+static void dma_fence_array_set_pending_error(struct dma_fence_array *array, + int error) +{ + /* + * Propagate the first error reported by any of our fences, but only + * before we ourselves are signaled. + */ + if (error) + cmpxchg(&array->base.error, PENDING_ERROR, error); +} + +static void dma_fence_array_clear_pending_error(struct dma_fence_array *array) +{ + /* Clear the error flag if not actually set. */ + cmpxchg(&array->base.error, PENDING_ERROR, 0); +} + static void irq_dma_fence_array_work(struct irq_work *wrk) { struct dma_fence_array *array = container_of(wrk, typeof(*array), work);

+ dma_fence_array_clear_pending_error(array); + dma_fence_signal(&array->base); dma_fence_put(&array->base); } @@ -38,6 +59,8 @@ static void dma_fence_array_cb_func(struct dma_fence *f, container_of(cb, struct dma_fence_array_cb, cb); struct dma_fence_array *array = array_cb->array;

+ dma_fence_array_set_pending_error(array, f->error); + if (atomic_dec_and_test(&array->num_pending)) irq_work_queue(&array->work); else @@ -63,9 +86,14 @@ static bool dma_fence_array_enable_signaling(struct dma_fence *fence) dma_fence_get(&array->base); if (dma_fence_add_callback(array->fences[i], &cb[i].cb, dma_fence_array_cb_func)) { + int error = array->fences[i]->error; + + dma_fence_array_set_pending_error(array, error); dma_fence_put(&array->base); - if (atomic_dec_and_test(&array->num_pending)) + if (atomic_dec_and_test(&array->num_pending)) { + dma_fence_array_clear_pending_error(array); return false; + } } }

@@ -142,6 +170,8 @@ struct dma_fence_array *dma_fence_array_create(int num_fences, atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences); array->fences = fences;

+ array->base.error = PENDING_ERROR; + return array; } EXPORT_SYMBOL(dma_fence_array_create);