[PATCH 1/4] dma-fence: Propagate errors to dma-fence-array container

[PATCH 1/4] dma-fence: Propagate errors to dma-fence-array container

[Chris Wilson]

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.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Gustavo Padovan <gustavo@padovan.org>
---
 drivers/dma-buf/dma-fence-array.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c
index 12c6f64c0bc2..d90675bb4fcc 100644
--- a/drivers/dma-buf/dma-fence-array.c
+++ b/drivers/dma-buf/dma-fence-array.c
@@ -13,6 +13,12 @@
 #include <linux/slab.h>
 #include <linux/dma-fence-array.h>
 
+static void fence_set_error_once(struct dma_fence *fence, int error)
+{
+	if (!fence->error && error)
+		dma_fence_set_error(fence, error);
+}
+
 static const char *dma_fence_array_get_driver_name(struct dma_fence *fence)
 {
 	return "dma_fence_array";
@@ -38,6 +44,13 @@ 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;
 
+	/*
+	 * 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 (atomic_dec_and_test(&array->num_pending))
 		irq_work_queue(&array->work);
 	else
@@ -63,6 +76,8 @@ 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)) {
+			fence_set_error_once(&array->base,
+					     array->fences[i]->error);
 			dma_fence_put(&array->base);
 			if (atomic_dec_and_test(&array->num_pending))
 				return false;
-- 
2.23.0.rc1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/4] dma-fence: Report the composite sync_file status

[Chris Wilson]

Same as for the individual fences, we want to report the actual status
of the fence when queried.

Reported-by: Petri Latvala <petri.latvala@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: Petri Latvala <petri.latvala@intel.com>
---
 drivers/dma-buf/sync_file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index ee4d1a96d779..25c5c071645b 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -419,7 +419,7 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
 	 * info->num_fences.
 	 */
 	if (!info.num_fences) {
-		info.status = dma_fence_is_signaled(sync_file->fence);
+		info.status = dma_fence_get_status(sync_file->fence);
 		goto no_fences;
 	} else {
 		info.status = 1;
-- 
2.23.0.rc1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/4] dma-fence: Refactor signaling for manual invocation

[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 +----------------
 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);
 
-- 
2.23.0.rc1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/4] dma-fence: Always execute signal callbacks

[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);
 	return 0;
 }
 EXPORT_SYMBOL(dma_fence_signal);
-- 
2.23.0.rc1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] dma-fence: Propagate errors to dma-fence-array container

[Koenig, Christian]

Am 10.08.19 um 17:34 schrieb Chris Wilson:
> 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.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Gustavo Padovan <gustavo@padovan.org>
> ---
>   drivers/dma-buf/dma-fence-array.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
>
> diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c
> index 12c6f64c0bc2..d90675bb4fcc 100644
> --- a/drivers/dma-buf/dma-fence-array.c
> +++ b/drivers/dma-buf/dma-fence-array.c
> @@ -13,6 +13,12 @@
>   #include <linux/slab.h>
>   #include <linux/dma-fence-array.h>
>   
> +static void fence_set_error_once(struct dma_fence *fence, int error)

I would use a dma_fence_array prefix for all names in the file.

> +{
> +	if (!fence->error && error)
> +		dma_fence_set_error(fence, error);
> +}
> +
>   static const char *dma_fence_array_get_driver_name(struct dma_fence *fence)
>   {
>   	return "dma_fence_array";
> @@ -38,6 +44,13 @@ 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;
>   
> +	/*
> +	 * 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.

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.

Christian.

> +
>   	if (atomic_dec_and_test(&array->num_pending))
>   		irq_work_queue(&array->work);
>   	else
> @@ -63,6 +76,8 @@ 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)) {
> +			fence_set_error_once(&array->base,
> +					     array->fences[i]->error);
>   			dma_fence_put(&array->base);
>   			if (atomic_dec_and_test(&array->num_pending))
>   				return false;

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] dma-fence: Always execute signal callbacks

[Koenig, Christian]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/4] dma-fence: Have dma_fence_signal call signal_locked

[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>
---
 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);
 
-- 
2.23.0.rc1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] dma-fence: Propagate errors to dma-fence-array container

[Chris Wilson]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3] dma-fence: Propagate errors to dma-fence-array container

[Chris Wilson]

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

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 | 25 +++++++++++++++++++++++++
 include/linux/dma-fence-array.h   |  2 ++
 2 files changed, 27 insertions(+)

diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c
index 12c6f64c0bc2..e806c5fe9ec9 100644
--- a/drivers/dma-buf/dma-fence-array.c
+++ b/drivers/dma-buf/dma-fence-array.c
@@ -23,10 +23,19 @@ static const char *dma_fence_array_get_timeline_name(struct dma_fence *fence)
 	return "unbound";
 }
 
+static void dma_fence_array_set_error_once(struct dma_fence_array *array,
+					   int error)
+{
+	if (!array->base.error && error)
+		dma_fence_set_error(&array->base, error);
+}
+
 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_set_error_once(array, READ_ONCE(array->pending_error));
+
 	dma_fence_signal(&array->base);
 	dma_fence_put(&array->base);
 }
@@ -38,6 +47,19 @@ 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;
 
+	/*
+	 * Propagate the first error reported by any of our fences, but only
+	 * before we ourselves are signaled.
+	 *
+	 * Note that this may race with multiple fences completing
+	 * simultaneously in error, but only one error will be kept, not
+	 * necessarily the first. So long as we propagate an error if any
+	 * fences were in error before we are signaled we should be telling
+	 * an acceptable truth.
+	 */
+	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);
 	else
@@ -63,6 +85,8 @@ 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)) {
+			dma_fence_array_set_error_once(array,
+						       array->fences[i]->error);
 			dma_fence_put(&array->base);
 			if (atomic_dec_and_test(&array->num_pending))
 				return false;
@@ -141,6 +165,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;
 };
 
-- 
2.23.0.rc1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v4] dma-fence: Propagate errors to dma-fence-array container

[Chris Wilson]

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.

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 | 37 ++++++++++++++++++++++++++++++-
 include/linux/dma-fence-array.h   |  2 ++
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c
index 12c6f64c0bc2..4d574dff0ba9 100644
--- a/drivers/dma-buf/dma-fence-array.c
+++ b/drivers/dma-buf/dma-fence-array.c
@@ -23,10 +23,37 @@ static const char *dma_fence_array_get_timeline_name(struct dma_fence *fence)
 	return "unbound";
 }
 
+static void dma_fence_array_set_error(struct dma_fence_array *array)
+{
+	int error = READ_ONCE(array->pending_error);
+
+	if (!array->base.error && error)
+		dma_fence_set_error(&array->base, error);
+}
+
+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.
+	 *
+	 * Note that this may race with multiple fences completing
+	 * simultaneously in error, but only one error will be kept, not
+	 * necessarily the first. So long as we propagate an error if any
+	 * fences were in error before we are signaled we should be telling
+	 * an acceptable truth.
+	 */
+	if (error && !array->pending_error)
+		WRITE_ONCE(array->pending_error, error);
+}
+
 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_set_error(array);
+
 	dma_fence_signal(&array->base);
 	dma_fence_put(&array->base);
 }
@@ -38,6 +65,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 +92,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_set_error(array);
 				return false;
+			}
 		}
 	}
 
@@ -141,6 +175,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;
 };
 
-- 
2.23.0.rc1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4] dma-fence: Propagate errors to dma-fence-array container

[Koenig, Christian]

How about this instead:

Setting array->base.error = 1 during initialization.

Then cmpxchg(array->base.error, 1, error) whenever a fence in the array 
signals.

And then finally cmpxchg(array->base.error, 1, 0) when the array itself 
signals.

Christian.

Am 11.08.19 um 14:21 schrieb Chris Wilson:
> 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.
>
> 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 | 37 ++++++++++++++++++++++++++++++-
>   include/linux/dma-fence-array.h   |  2 ++
>   2 files changed, 38 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c
> index 12c6f64c0bc2..4d574dff0ba9 100644
> --- a/drivers/dma-buf/dma-fence-array.c
> +++ b/drivers/dma-buf/dma-fence-array.c
> @@ -23,10 +23,37 @@ static const char *dma_fence_array_get_timeline_name(struct dma_fence *fence)
>   	return "unbound";
>   }
>   
> +static void dma_fence_array_set_error(struct dma_fence_array *array)
> +{
> +	int error = READ_ONCE(array->pending_error);
> +
> +	if (!array->base.error && error)
> +		dma_fence_set_error(&array->base, error);
> +}
> +
> +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.
> +	 *
> +	 * Note that this may race with multiple fences completing
> +	 * simultaneously in error, but only one error will be kept, not
> +	 * necessarily the first. So long as we propagate an error if any
> +	 * fences were in error before we are signaled we should be telling
> +	 * an acceptable truth.
> +	 */
> +	if (error && !array->pending_error)
> +		WRITE_ONCE(array->pending_error, error);
> +}
> +
>   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_set_error(array);
> +
>   	dma_fence_signal(&array->base);
>   	dma_fence_put(&array->base);
>   }
> @@ -38,6 +65,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 +92,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_set_error(array);
>   				return false;
> +			}
>   		}
>   	}
>   
> @@ -141,6 +175,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;
>   };
>   

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/4] dma-fence: Have dma_fence_signal call signal_locked

[Koenig, Christian]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v5] dma-fence: Propagate errors to dma-fence-array container

[Chris Wilson]

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);
-- 
2.23.0.rc1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5] dma-fence: Propagate errors to dma-fence-array container

[Koenig, Christian]

Am 11.08.19 um 18:25 schrieb Chris Wilson:
> 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>

Reviewed-by: 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);

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4] dma-fence: Propagate errors to dma-fence-array container

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 12883 bytes --]

Hi Chris,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[cannot apply to v5.3-rc3 next-20190809]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Chris-Wilson/dma-fence-Propagate-errors-to-dma-fence-array-container/20190812-022143
reproduce: make htmldocs

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   Warning: The Sphinx 'sphinx_rtd_theme' HTML theme was not found. Make sure you have the theme installed to produce pretty HTML output. Falling back to the default theme.
   WARNING: dot(1) not found, for better output quality install graphviz from http://www.graphviz.org
   WARNING: convert(1) not found, for SVG to PDF conversion install ImageMagick (https://www.imagemagick.org)
>> include/linux/dma-fence-array.h:49: warning: Function parameter or member 'pending_error' not described in 'dma_fence_array'
   include/linux/w1.h:272: warning: Function parameter or member 'of_match_table' not described in 'w1_family'
   include/linux/i2c.h:337: warning: Function parameter or member 'init_irq' not described in 'i2c_client'
   lib/genalloc.c:1: warning: 'gen_pool_add_virt' not found
   lib/genalloc.c:1: warning: 'gen_pool_alloc' not found
   lib/genalloc.c:1: warning: 'gen_pool_free' not found
   lib/genalloc.c:1: warning: 'gen_pool_alloc_algo' not found
   include/linux/regulator/machine.h:196: warning: Function parameter or member 'max_uV_step' not described in 'regulation_constraints'
   include/linux/regulator/driver.h:223: warning: Function parameter or member 'resume' not described in 'regulator_ops'
   include/linux/spi/spi.h:190: warning: Function parameter or member 'driver_override' not described in 'spi_device'
   drivers/usb/typec/bus.c:1: warning: 'typec_altmode_register_driver' not found
   drivers/usb/typec/bus.c:1: warning: 'typec_altmode_unregister_driver' not found
   drivers/usb/typec/class.c:1: warning: 'typec_altmode_unregister_notifier' not found
   drivers/usb/typec/class.c:1: warning: 'typec_altmode_register_notifier' not found
   fs/direct-io.c:258: warning: Excess function parameter 'offset' description in 'dio_complete'
   fs/libfs.c:496: warning: Excess function parameter 'available' description in 'simple_write_end'
   fs/posix_acl.c:647: warning: Function parameter or member 'inode' not described in 'posix_acl_update_mode'
   fs/posix_acl.c:647: warning: Function parameter or member 'mode_p' not described in 'posix_acl_update_mode'
   fs/posix_acl.c:647: warning: Function parameter or member 'acl' not described in 'posix_acl_update_mode'
   include/linux/input/sparse-keymap.h:43: warning: Function parameter or member 'sw' not described in 'key_entry'
   drivers/gpu/drm/mcde/mcde_drv.c:1: warning: 'ST-Ericsson MCDE DRM Driver' not found
   include/net/mac80211.h:2006: warning: Function parameter or member 'txpwr' not described in 'ieee80211_sta'
   mm/util.c:1: warning: 'get_user_pages_fast' not found
   mm/slab.c:4215: warning: Function parameter or member 'objp' not described in '__ksize'
   include/linux/skbuff.h:893: warning: Function parameter or member 'dev_scratch' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'list' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'ip_defrag_offset' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'skb_mstamp_ns' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member '__cloned_offset' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'head_frag' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member '__pkt_type_offset' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'encapsulation' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'encap_hdr_csum' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'csum_valid' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member '__pkt_vlan_present_offset' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'vlan_present' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'csum_complete_sw' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'csum_level' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'inner_protocol_type' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'remcsum_offload' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'sender_cpu' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'reserved_tailroom' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'inner_ipproto' not described in 'sk_buff'
   include/net/sock.h:233: warning: Function parameter or member 'skc_addrpair' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_portpair' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_ipv6only' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_net_refcnt' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_v6_daddr' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_v6_rcv_saddr' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_cookie' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_listener' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_tw_dr' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_rcv_wnd' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_tw_rcv_nxt' not described in 'sock_common'
   include/net/sock.h:515: warning: Function parameter or member 'sk_rx_skb_cache' not described in 'sock'
   include/net/sock.h:515: warning: Function parameter or member 'sk_wq_raw' not described in 'sock'
   include/net/sock.h:515: warning: Function parameter or member 'tcp_rtx_queue' not described in 'sock'
   include/net/sock.h:515: warning: Function parameter or member 'sk_tx_skb_cache' not described in 'sock'
   include/net/sock.h:515: warning: Function parameter or member 'sk_route_forced_caps' not described in 'sock'
   include/net/sock.h:515: warning: Function parameter or member 'sk_txtime_report_errors' not described in 'sock'
   include/net/sock.h:515: warning: Function parameter or member 'sk_validate_xmit_skb' not described in 'sock'
   include/net/sock.h:515: warning: Function parameter or member 'sk_bpf_storage' not described in 'sock'
   include/net/sock.h:2439: warning: Function parameter or member 'tcp_rx_skb_cache_key' not described in 'DECLARE_STATIC_KEY_FALSE'
   include/net/sock.h:2439: warning: Excess function parameter 'sk' description in 'DECLARE_STATIC_KEY_FALSE'
   include/net/sock.h:2439: warning: Excess function parameter 'skb' description in 'DECLARE_STATIC_KEY_FALSE'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'gso_partial_features' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'l3mdev_ops' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'xfrmdev_ops' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'tlsdev_ops' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'name_assign_type' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'ieee802154_ptr' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'mpls_ptr' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'xdp_prog' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'gro_flush_timeout' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'nf_hooks_ingress' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member '____cacheline_aligned_in_smp' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'qdisc_hash' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'xps_cpus_map' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'xps_rxqs_map' not described in 'net_device'
   include/linux/phylink.h:56: warning: Function parameter or member '__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising' not described in 'phylink_link_state'
   include/linux/phylink.h:56: warning: Function parameter or member '__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising' not described in 'phylink_link_state'
   drivers/net/phy/phylink.c:595: warning: Function parameter or member 'config' not described in 'phylink_create'
   drivers/net/phy/phylink.c:595: warning: Excess function parameter 'ndev' description in 'phylink_create'
   include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'quotactl' not described in 'security_list_options'
   include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'quota_on' not described in 'security_list_options'
   include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'sb_free_mnt_opts' not described in 'security_list_options'
   include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'sb_eat_lsm_opts' not described in 'security_list_options'
   include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'sb_kern_mount' not described in 'security_list_options'
   include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'sb_show_options' not described in 'security_list_options'
   include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'sb_add_mnt_opt' not described in 'security_list_options'
   include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'd_instantiate' not described in 'security_list_options'
   include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'getprocattr' not described in 'security_list_options'
   include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'setprocattr' not described in 'security_list_options'
   drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c:142: warning: Function parameter or member 'blockable' not described in 'amdgpu_mn_read_lock'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:347: warning: cannot understand function prototype: 'struct amdgpu_vm_pt_cursor '
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:348: warning: cannot understand function prototype: 'struct amdgpu_vm_pt_cursor '
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:494: warning: Function parameter or member 'start' not described in 'amdgpu_vm_pt_first_dfs'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'adev' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'vm' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'start' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'cursor' not described in 'for_each_amdgpu_vm_pt_dfs_safe'

vim +49 include/linux/dma-fence-array.h

f54d1867005c33 Chris Wilson 2016-10-25 @49  

:::::: The code at line 49 was first introduced by commit
:::::: f54d1867005c3323f5d8ad83eed823e84226c429 dma-buf: Rename struct fence to dma_fence

:::::: TO: Chris Wilson <chris@chris-wilson.co.uk>
:::::: CC: Daniel Vetter <daniel.vetter@ffwll.ch>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 7282 bytes --]

[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 2/4] dma-fence: Report the composite sync_file status

[Matthew Auld]

On Sat, 10 Aug 2019 at 16:36, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> Same as for the individual fences, we want to report the actual status
> of the fence when queried.
>
> Reported-by: Petri Latvala <petri.latvala@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Gustavo Padovan <gustavo@padovan.org>
> Cc: Petri Latvala <petri.latvala@intel.com>
Reviewed-by: Matthew Auld <matthew.auld@intel.com>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] dma-fence: Refactor signaling for manual invocation

[Koenig, Christian]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] dma-fence: Refactor signaling for manual invocation

[Chris Wilson]

Quoting Koenig, Christian (2019-08-12 15:34:32)
> 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

iirc, when I included the trace.h from dma-fence.h or dma-fence-impl.h
without separating the types, amdgpu failed to compile (which is more
than likely to be simply due to be first drm in the list to compile).

Doing more work wasn't through choice.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] dma-fence: Refactor signaling for manual invocation

[Koenig, Christian]

Am 12.08.19 um 16:43 schrieb Chris Wilson:
> Quoting Koenig, Christian (2019-08-12 15:34:32)
>> 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
> iirc, when I included the trace.h from dma-fence.h or dma-fence-impl.h
> without separating the types, amdgpu failed to compile (which is more
> than likely to be simply due to be first drm in the list to compile).

Ah, but why do you want to include trace.h in a header in the first place?

That's usually not something I would recommend either.

Christian.

>
> Doing more work wasn't through choice.
> -Chris

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] dma-fence: Refactor signaling for manual invocation

[Chris Wilson]

Quoting Koenig, Christian (2019-08-12 15:50:59)
> Am 12.08.19 um 16:43 schrieb Chris Wilson:
> > Quoting Koenig, Christian (2019-08-12 15:34:32)
> >> 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
> > iirc, when I included the trace.h from dma-fence.h or dma-fence-impl.h
> > without separating the types, amdgpu failed to compile (which is more
> > than likely to be simply due to be first drm in the list to compile).
> 
> Ah, but why do you want to include trace.h in a header in the first place?
> 
> That's usually not something I would recommend either.

The problem is that we do emit a tracepoint as part of the sequence I
want to put into the reusable chunk of code.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] dma-fence: Refactor signaling for manual invocation

[Koenig, Christian]

Am 12.08.19 um 16:53 schrieb Chris Wilson:
> Quoting Koenig, Christian (2019-08-12 15:50:59)
>> Am 12.08.19 um 16:43 schrieb Chris Wilson:
>>> Quoting Koenig, Christian (2019-08-12 15:34:32)
>>>> 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
>>> iirc, when I included the trace.h from dma-fence.h or dma-fence-impl.h
>>> without separating the types, amdgpu failed to compile (which is more
>>> than likely to be simply due to be first drm in the list to compile).
>> Ah, but why do you want to include trace.h in a header in the first place?
>>
>> That's usually not something I would recommend either.
> The problem is that we do emit a tracepoint as part of the sequence I
> want to put into the reusable chunk of code.

Ok, we are going in circles here. Why do you want to reuse the code then?

Christian.

> -Chris

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] dma-fence: Refactor signaling for manual invocation

[Chris Wilson]

Quoting Koenig, Christian (2019-08-13 07:59:28)
> Am 12.08.19 um 16:53 schrieb Chris Wilson:
> > Quoting Koenig, Christian (2019-08-12 15:50:59)
> >> Am 12.08.19 um 16:43 schrieb Chris Wilson:
> >>> Quoting Koenig, Christian (2019-08-12 15:34:32)
> >>>> 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
> >>> iirc, when I included the trace.h from dma-fence.h or dma-fence-impl.h
> >>> without separating the types, amdgpu failed to compile (which is more
> >>> than likely to be simply due to be first drm in the list to compile).
> >> Ah, but why do you want to include trace.h in a header in the first place?
> >>
> >> That's usually not something I would recommend either.
> > The problem is that we do emit a tracepoint as part of the sequence I
> > want to put into the reusable chunk of code.
> 
> Ok, we are going in circles here. Why do you want to reuse the code then?

I am reusing the code to avoid fun and games with signal-vs-free.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] dma-fence: Refactor signaling for manual invocation

[Koenig, Christian]

Am 13.08.19 um 10:25 schrieb Chris Wilson:
> Quoting Koenig, Christian (2019-08-13 07:59:28)
>> Am 12.08.19 um 16:53 schrieb Chris Wilson:
>>> Quoting Koenig, Christian (2019-08-12 15:50:59)
>>>> Am 12.08.19 um 16:43 schrieb Chris Wilson:
>>>>> Quoting Koenig, Christian (2019-08-12 15:34:32)
>>>>>> 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
>>>>> iirc, when I included the trace.h from dma-fence.h or dma-fence-impl.h
>>>>> without separating the types, amdgpu failed to compile (which is more
>>>>> than likely to be simply due to be first drm in the list to compile).
>>>> Ah, but why do you want to include trace.h in a header in the first place?
>>>>
>>>> That's usually not something I would recommend either.
>>> The problem is that we do emit a tracepoint as part of the sequence I
>>> want to put into the reusable chunk of code.
>> Ok, we are going in circles here. Why do you want to reuse the code then?
> I am reusing the code to avoid fun and games with signal-vs-free.

Yeah, but that doesn't seems to be valid.

See the dma_fence should more or less be a contained object and you now 
expose quite a bit of the internal functionality inside a headers.

And creating headers which when included make other drivers fail to 
compile sounds like a rather bad idea to me.

Please explain the background a bit more.

Thanks,
Christian.

> -Chris

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/4] dma-fence: Have dma_fence_signal call signal_locked

[Daniel Vetter]

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.

I guess with the addition of more fancy atomic BITs we could avoid this
too ... but no idea whether that's worth the effort.
-Daniel

> ---
>  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);
>  
> -- 
> 2.23.0.rc1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/4] dma-fence: Have dma_fence_signal call signal_locked

[Chris Wilson]

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.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/4] dma-fence: Have dma_fence_signal call signal_locked

[Daniel Vetter]

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, 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.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/4] dma-fence: Have dma_fence_signal call signal_locked

[Chris Wilson]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/4] dma-fence: Have dma_fence_signal call signal_locked

[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

Almost certainly more shaving if you stare.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/4] dma-fence: Have dma_fence_signal call signal_locked

[Koenig, Christian]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel