[PATCH 02/15] dma-fence: Make ->enable_signaling optional

[PATCH 02/15] dma-fence: Make ->enable_signaling optional

[Daniel Vetter]

Many drivers have a trivial implementation for ->enable_signaling.
Let's make it optional by assuming that signalling is already
available when the callback isn't present.

Reviewed-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: linux-media@vger.kernel.org
Cc: linaro-mm-sig@lists.linaro.org
---
 drivers/dma-buf/dma-fence.c | 13 ++++++++++++-
 include/linux/dma-fence.h   |  3 ++-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 4edb9fd3cf47..7b5b40d6b70e 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -181,6 +181,13 @@ void dma_fence_release(struct kref *kref)
 }
 EXPORT_SYMBOL(dma_fence_release);
 
+/**
+ * dma_fence_free - default release function for &dma_fence.
+ * @fence: fence to release
+ *
+ * This is the default implementation for &dma_fence_ops.release. It calls
+ * kfree_rcu() on @fence.
+ */
 void dma_fence_free(struct dma_fence *fence)
 {
 	kfree_rcu(fence, rcu);
@@ -560,7 +567,7 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
 	       spinlock_t *lock, u64 context, unsigned seqno)
 {
 	BUG_ON(!lock);
-	BUG_ON(!ops || !ops->wait || !ops->enable_signaling ||
+	BUG_ON(!ops || !ops->wait ||
 	       !ops->get_driver_name || !ops->get_timeline_name);
 
 	kref_init(&fence->refcount);
@@ -572,6 +579,10 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
 	fence->flags = 0UL;
 	fence->error = 0;
 
+	if (!ops->enable_signaling)
+		set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
+			&fence->flags);
+
 	trace_dma_fence_init(fence);
 }
 EXPORT_SYMBOL(dma_fence_init);
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 111aefe1c956..c053d19e1e24 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -166,7 +166,8 @@ struct dma_fence_ops {
 	 * released when the fence is signalled (through e.g. the interrupt
 	 * handler).
 	 *
-	 * This callback is mandatory.
+	 * 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);
 
-- 
2.17.0

[PATCH 03/15] dma-fence: Allow wait_any_timeout for all fences

[Daniel Vetter]

When this was introduced in

commit a519435a96597d8cd96123246fea4ae5a6c90b02
Author: Christian König <christian.koenig@amd.com>
Date:   Tue Oct 20 16:34:16 2015 +0200

    dma-buf/fence: add fence_wait_any_timeout function v2

there was a restriction added that this only works if the dma-fence
uses the dma_fence_default_wait hook. Which works for amdgpu, which is
the only caller. Well, until you share some buffers with e.g. i915,
then you get an -EINVAL.

But there's really no reason for this, because all drivers must
support callbacks. The special ->wait hook is only as an optimization;
if the driver needs to create a worker thread for an active callback,
then it can avoid to do that if it knows that there's a process
context available already. So ->wait is just an optimization, just
using the logic in dma_fence_default_wait() should work for all
drivers.

Let's remove this restriction.

Reviewed-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: linux-media@vger.kernel.org
Cc: linaro-mm-sig@lists.linaro.org
Cc: Christian König <christian.koenig@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/dma-buf/dma-fence.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 7b5b40d6b70e..59049375bd19 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -503,11 +503,6 @@ dma_fence_wait_any_timeout(struct dma_fence **fences, uint32_t count,
 	for (i = 0; i < count; ++i) {
 		struct dma_fence *fence = fences[i];
 
-		if (fence->ops->wait != dma_fence_default_wait) {
-			ret = -EINVAL;
-			goto fence_rm_cb;
-		}
-
 		cb[i].task = current;
 		if (dma_fence_add_callback(fence, &cb[i].base,
 					   dma_fence_default_wait_cb)) {
-- 
2.17.0

[PATCH 04/15] dma-fence: Make ->wait callback optional

[Daniel Vetter]

Almost everyone uses dma_fence_default_wait.

v2: Also remove the BUG_ON(!ops->wait) (Chris).

Reviewed-by: Christian König <christian.koenig@amd.com> (v1)
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: linux-media@vger.kernel.org
Cc: linaro-mm-sig@lists.linaro.org
---
 drivers/dma-buf/dma-fence-array.c |  1 -
 drivers/dma-buf/dma-fence.c       |  8 +++++---
 drivers/dma-buf/sw_sync.c         |  1 -
 include/linux/dma-fence.h         | 13 ++++++++-----
 4 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c
index dd1edfb27b61..a8c254497251 100644
--- a/drivers/dma-buf/dma-fence-array.c
+++ b/drivers/dma-buf/dma-fence-array.c
@@ -104,7 +104,6 @@ const struct dma_fence_ops dma_fence_array_ops = {
 	.get_timeline_name = dma_fence_array_get_timeline_name,
 	.enable_signaling = dma_fence_array_enable_signaling,
 	.signaled = dma_fence_array_signaled,
-	.wait = dma_fence_default_wait,
 	.release = dma_fence_array_release,
 };
 EXPORT_SYMBOL(dma_fence_array_ops);
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 59049375bd19..41ec19c9efc7 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -158,7 +158,10 @@ dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout)
 		return -EINVAL;
 
 	trace_dma_fence_wait_start(fence);
-	ret = fence->ops->wait(fence, intr, timeout);
+	if (fence->ops->wait)
+		ret = fence->ops->wait(fence, intr, timeout);
+	else
+		ret = dma_fence_default_wait(fence, intr, timeout);
 	trace_dma_fence_wait_end(fence);
 	return ret;
 }
@@ -562,8 +565,7 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
 	       spinlock_t *lock, u64 context, unsigned seqno)
 {
 	BUG_ON(!lock);
-	BUG_ON(!ops || !ops->wait ||
-	       !ops->get_driver_name || !ops->get_timeline_name);
+	BUG_ON(!ops || !ops->get_driver_name || !ops->get_timeline_name);
 
 	kref_init(&fence->refcount);
 	fence->ops = ops;
diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
index 3d78ca89a605..53c1d6d36a64 100644
--- a/drivers/dma-buf/sw_sync.c
+++ b/drivers/dma-buf/sw_sync.c
@@ -188,7 +188,6 @@ static const struct dma_fence_ops timeline_fence_ops = {
 	.get_timeline_name = timeline_fence_get_timeline_name,
 	.enable_signaling = timeline_fence_enable_signaling,
 	.signaled = timeline_fence_signaled,
-	.wait = dma_fence_default_wait,
 	.release = timeline_fence_release,
 	.fence_value_str = timeline_fence_value_str,
 	.timeline_value_str = timeline_fence_timeline_value_str,
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index c053d19e1e24..02dba8cd033d 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -191,11 +191,14 @@ struct dma_fence_ops {
 	/**
 	 * @wait:
 	 *
-	 * Custom wait implementation, or dma_fence_default_wait.
+	 * Custom wait implementation, defaults to dma_fence_default_wait() if
+	 * not set.
 	 *
-	 * Must not be NULL, set to dma_fence_default_wait for default implementation.
-	 * the dma_fence_default_wait implementation should work for any fence, as long
-	 * as enable_signaling works correctly.
+	 * 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
@@ -203,7 +206,7 @@ struct dma_fence_ops {
 	 * which should be treated as if the fence is signaled. For example a hardware
 	 * lockup could be reported like that.
 	 *
-	 * This callback is mandatory.
+	 * This callback is optional.
 	 */
 	signed long (*wait)(struct dma_fence *fence,
 			    bool intr, signed long timeout);
-- 
2.17.0

[PATCH 15/15] dma-fence: Polish kernel-doc for dma-fence.c

[Daniel Vetter]

- Intro section that links to how this is exposed to userspace.
- Lots more hyperlinks.
- Minor clarifications and style polish

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: linux-media@vger.kernel.org
Cc: linaro-mm-sig@lists.linaro.org
---
 Documentation/driver-api/dma-buf.rst |   6 ++
 drivers/dma-buf/dma-fence.c          | 140 ++++++++++++++++++---------
 2 files changed, 102 insertions(+), 44 deletions(-)

diff --git a/Documentation/driver-api/dma-buf.rst b/Documentation/driver-api/dma-buf.rst
index dc384f2f7f34..b541e97c7ab1 100644
--- a/Documentation/driver-api/dma-buf.rst
+++ b/Documentation/driver-api/dma-buf.rst
@@ -130,6 +130,12 @@ Reservation Objects
 DMA Fences
 ----------
 
+.. kernel-doc:: drivers/dma-buf/dma-fence.c
+   :doc: DMA fences overview
+
+DMA Fences Functions Reference
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
 .. kernel-doc:: drivers/dma-buf/dma-fence.c
    :export:
 
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 41ec19c9efc7..0387c6a59055 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -38,12 +38,43 @@ EXPORT_TRACEPOINT_SYMBOL(dma_fence_enable_signal);
  */
 static atomic64_t dma_fence_context_counter = ATOMIC64_INIT(0);
 
+/**
+ * DOC: DMA fences overview
+ *
+ * DMA fences, represented by &struct dma_fence, are the kernel internal
+ * synchronization primitive for DMA operations like GPU rendering, video
+ * encoding/decoding, or displaying buffers on a screen.
+ *
+ * A fence is initialized using dma_fence_init() and completed using
+ * dma_fence_signal(). Fences are associated with a context, allocated through
+ * dma_fence_context_alloc(), and all fences on the same context are
+ * fully ordered.
+ *
+ * Since the purposes of fences is to facilitate cross-device and
+ * cross-application synchronization, there's multiple ways to use one:
+ *
+ * - Individual fences can be exposed as a &sync_file, accessed as a file
+ *   descriptor from userspace, created by calling sync_file_create(). This is
+ *   called explicit fencing, since userspace passes around explicit
+ *   synchronization points.
+ *
+ * - Some subsystems also have their own explicit fencing primitives, like
+ *   &drm_syncobj. Compared to &sync_file, a &drm_syncobj allows the underlying
+ *   fence to be updated.
+ *
+ * - Then there's also implicit fencing, where the synchronization points are
+ *   implicitly passed around as part of shared &dma_buf instances. Such
+ *   implicit fences are stored in &struct reservation_object through the
+ *   &dma_buf.resv pointer.
+ */
+
 /**
  * dma_fence_context_alloc - allocate an array of fence contexts
- * @num:	[in]	amount of contexts to allocate
+ * @num: amount of contexts to allocate
  *
- * This function will return the first index of the number of fences allocated.
- * The fence context is used for setting fence->context to a unique number.
+ * This function will return the first index of the number of fence contexts
+ * allocated.  The fence context is used for setting &dma_fence.context to a
+ * unique number by passing the context to dma_fence_init().
  */
 u64 dma_fence_context_alloc(unsigned num)
 {
@@ -59,10 +90,14 @@ EXPORT_SYMBOL(dma_fence_context_alloc);
  * Signal completion for software callbacks on a fence, this will unblock
  * dma_fence_wait() calls and run all the callbacks added with
  * dma_fence_add_callback(). Can be called multiple times, but since a fence
- * can only go from unsignaled to signaled state, it will only be effective
- * the first time.
+ * can only go from the unsignaled to the signaled state and not back, it will
+ * only be effective the first time.
  *
- * Unlike dma_fence_signal, this function must be called with fence->lock held.
+ * Unlike dma_fence_signal(), this function must be called with &dma_fence.lock
+ * held.
+ *
+ * Returns 0 on success and a negative error value when @fence has been
+ * signalled already.
  */
 int dma_fence_signal_locked(struct dma_fence *fence)
 {
@@ -102,8 +137,11 @@ EXPORT_SYMBOL(dma_fence_signal_locked);
  * Signal completion for software callbacks on a fence, this will unblock
  * dma_fence_wait() calls and run all the callbacks added with
  * dma_fence_add_callback(). Can be called multiple times, but since a fence
- * can only go from unsignaled to signaled state, it will only be effective
- * the first time.
+ * can only go from the unsignaled to the signaled state and not back, it will
+ * only be effective the first time.
+ *
+ * Returns 0 on success and a negative error value when @fence has been
+ * signalled already.
  */
 int dma_fence_signal(struct dma_fence *fence)
 {
@@ -136,9 +174,9 @@ EXPORT_SYMBOL(dma_fence_signal);
 /**
  * dma_fence_wait_timeout - sleep until the fence gets signaled
  * or until timeout elapses
- * @fence:	[in]	the fence to wait on
- * @intr:	[in]	if true, do an interruptible wait
- * @timeout:	[in]	timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT
+ * @fence: the fence to wait on
+ * @intr: if true, do an interruptible wait
+ * @timeout: timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT
  *
  * Returns -ERESTARTSYS if interrupted, 0 if the wait timed out, or the
  * remaining timeout in jiffies on success. Other error values may be
@@ -148,6 +186,8 @@ EXPORT_SYMBOL(dma_fence_signal);
  * directly or indirectly (buf-mgr between reservation and committing)
  * holds a reference to the fence, otherwise the fence might be
  * freed before return, resulting in undefined behavior.
+ *
+ * See also dma_fence_wait() and dma_fence_wait_any_timeout().
  */
 signed long
 dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout)
@@ -167,6 +207,13 @@ dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout)
 }
 EXPORT_SYMBOL(dma_fence_wait_timeout);
 
+/**
+ * dma_fence_release - default relese function for fences
+ * @kref: &dma_fence.recfount
+ *
+ * This is the default release functions for &dma_fence. Drivers shouldn't call
+ * this directly, but instead call dma_fence_put().
+ */
 void dma_fence_release(struct kref *kref)
 {
 	struct dma_fence *fence =
@@ -199,10 +246,11 @@ EXPORT_SYMBOL(dma_fence_free);
 
 /**
  * dma_fence_enable_sw_signaling - enable signaling on fence
- * @fence:	[in]	the fence to enable
+ * @fence: the fence to enable
  *
- * this will request for sw signaling to be enabled, to make the fence
- * complete as soon as possible
+ * This will request for sw signaling to be enabled, to make the fence
+ * complete as soon as possible. This calls &dma_fence_ops.enable_signaling
+ * internally.
  */
 void dma_fence_enable_sw_signaling(struct dma_fence *fence)
 {
@@ -226,24 +274,24 @@ EXPORT_SYMBOL(dma_fence_enable_sw_signaling);
 /**
  * dma_fence_add_callback - add a callback to be called when the fence
  * is signaled
- * @fence:	[in]	the fence to wait on
- * @cb:		[in]	the callback to register
- * @func:	[in]	the function to call
+ * @fence: the fence to wait on
+ * @cb: the callback to register
+ * @func: the function to call
  *
- * cb will be initialized by dma_fence_add_callback, no initialization
+ * @cb will be initialized by dma_fence_add_callback(), no initialization
  * by the caller is required. Any number of callbacks can be registered
  * to a fence, but a callback can only be registered to one fence at a time.
  *
  * Note that the callback can be called from an atomic context.  If
  * fence is already signaled, this function will return -ENOENT (and
- * *not* call the callback)
+ * *not* call the callback).
  *
  * Add a software callback to the fence. Same restrictions apply to
- * refcount as it does to dma_fence_wait, however the caller doesn't need to
- * keep a refcount to fence afterwards: when software access is enabled,
- * the creator of the fence is required to keep the fence alive until
- * after it signals with dma_fence_signal. The callback itself can be called
- * from irq context.
+ * refcount as it does to dma_fence_wait(), however the caller doesn't need to
+ * keep a refcount to fence afterward dma_fence_add_callback() has returned:
+ * when software access is enabled, the creator of the fence is required to keep
+ * the fence alive until after it signals with dma_fence_signal(). The callback
+ * itself can be called from irq context.
  *
  * Returns 0 in case of success, -ENOENT if the fence is already signaled
  * and -EINVAL in case of error.
@@ -292,7 +340,7 @@ EXPORT_SYMBOL(dma_fence_add_callback);
 
 /**
  * dma_fence_get_status - returns the status upon completion
- * @fence: [in]	the dma_fence to query
+ * @fence: the dma_fence to query
  *
  * This wraps dma_fence_get_status_locked() to return the error status
  * condition on a signaled fence. See dma_fence_get_status_locked() for more
@@ -317,8 +365,8 @@ EXPORT_SYMBOL(dma_fence_get_status);
 
 /**
  * dma_fence_remove_callback - remove a callback from the signaling list
- * @fence:	[in]	the fence to wait on
- * @cb:		[in]	the callback to remove
+ * @fence: the fence to wait on
+ * @cb: the callback to remove
  *
  * Remove a previously queued callback from the fence. This function returns
  * true if the callback is successfully removed, or false if the fence has
@@ -329,6 +377,9 @@ EXPORT_SYMBOL(dma_fence_get_status);
  * doing, since deadlocks and race conditions could occur all too easily. For
  * this reason, it should only ever be done on hardware lockup recovery,
  * with a reference held to the fence.
+ *
+ * Behaviour is undefined if @cb has not been added to @fence using
+ * dma_fence_add_callback() beforehand.
  */
 bool
 dma_fence_remove_callback(struct dma_fence *fence, struct dma_fence_cb *cb)
@@ -365,9 +416,9 @@ dma_fence_default_wait_cb(struct dma_fence *fence, struct dma_fence_cb *cb)
 /**
  * dma_fence_default_wait - default sleep until the fence gets signaled
  * or until timeout elapses
- * @fence:	[in]	the fence to wait on
- * @intr:	[in]	if true, do an interruptible wait
- * @timeout:	[in]	timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT
+ * @fence: the fence to wait on
+ * @intr: if true, do an interruptible wait
+ * @timeout: timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT
  *
  * Returns -ERESTARTSYS if interrupted, 0 if the wait timed out, or the
  * remaining timeout in jiffies on success. If timeout is zero the value one is
@@ -460,12 +511,12 @@ dma_fence_test_signaled_any(struct dma_fence **fences, uint32_t count,
 /**
  * dma_fence_wait_any_timeout - sleep until any fence gets signaled
  * or until timeout elapses
- * @fences:	[in]	array of fences to wait on
- * @count:	[in]	number of fences to wait on
- * @intr:	[in]	if true, do an interruptible wait
- * @timeout:	[in]	timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT
- * @idx:       [out]	the first signaled fence index, meaningful only on
- *			positive return
+ * @fences: array of fences to wait on
+ * @count: number of fences to wait on
+ * @intr: if true, do an interruptible wait
+ * @timeout: timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT
+ * @idx: used to store the first signaled fence index, meaningful only on
+ *	positive return
  *
  * Returns -EINVAL on custom fence wait implementation, -ERESTARTSYS if
  * interrupted, 0 if the wait timed out, or the remaining timeout in jiffies
@@ -474,6 +525,8 @@ dma_fence_test_signaled_any(struct dma_fence **fences, uint32_t count,
  * Synchronous waits for the first fence in the array to be signaled. The
  * caller needs to hold a reference to all fences in the array, otherwise a
  * fence might be freed before return, resulting in undefined behavior.
+ *
+ * See also dma_fence_wait() and dma_fence_wait_timeout().
  */
 signed long
 dma_fence_wait_any_timeout(struct dma_fence **fences, uint32_t count,
@@ -546,19 +599,18 @@ EXPORT_SYMBOL(dma_fence_wait_any_timeout);
 
 /**
  * dma_fence_init - Initialize a custom fence.
- * @fence:	[in]	the fence to initialize
- * @ops:	[in]	the dma_fence_ops for operations on this fence
- * @lock:	[in]	the irqsafe spinlock to use for locking this fence
- * @context:	[in]	the execution context this fence is run on
- * @seqno:	[in]	a linear increasing sequence number for this context
+ * @fence: the fence to initialize
+ * @ops: the dma_fence_ops for operations on this fence
+ * @lock: the irqsafe spinlock to use for locking this fence
+ * @context: the execution context this fence is run on
+ * @seqno: a linear increasing sequence number for this context
  *
  * Initializes an allocated fence, the caller doesn't have to keep its
  * refcount after committing with this fence, but it will need to hold a
- * refcount again if dma_fence_ops.enable_signaling gets called. This can
- * be used for other implementing other types of fence.
+ * refcount again if &dma_fence_ops.enable_signaling gets called.
  *
  * context and seqno are used for easy comparison between fences, allowing
- * to check which fence is later by simply using dma_fence_later.
+ * to check which fence is later by simply using dma_fence_later().
  */
 void
 dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
-- 
2.17.0

Re: [PATCH 02/15] dma-fence: Make ->enable_signaling optional

[Chris Wilson]

Quoting Daniel Vetter (2018-05-03 15:25:50)
> @@ -560,7 +567,7 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
>                spinlock_t *lock, u64 context, unsigned seqno)
>  {
>         BUG_ON(!lock);
> -       BUG_ON(!ops || !ops->wait || !ops->enable_signaling ||
> +       BUG_ON(!ops || !ops->wait ||
>                !ops->get_driver_name || !ops->get_timeline_name);

One thing I was wondering about (following the discussion of rhashtable
on lwn) was inlining this function and passing dma_fence_ops by value.
And seeing if that eliminates the branch and makes smaller code
(probably not, mostly idling wondering about that technique) and kills
off the BUGs (can then be BUILD_BUG_ON).
-Chris

Re: [PATCH 04/15] dma-fence: Make ->wait callback optional

[Chris Wilson]

Quoting Daniel Vetter (2018-05-03 15:25:52)
> Almost everyone uses dma_fence_default_wait.
> 
> v2: Also remove the BUG_ON(!ops->wait) (Chris).

I just don't get the rationale for implicit over explicit.
-Chris

Re: [PATCH 04/15] dma-fence: Make ->wait callback optional

[Daniel Vetter]

On Fri, May 04, 2018 at 09:09:10AM +0100, Chris Wilson wrote:
> Quoting Daniel Vetter (2018-05-03 15:25:52)
> > Almost everyone uses dma_fence_default_wait.
> > 
> > v2: Also remove the BUG_ON(!ops->wait) (Chris).
> 
> I just don't get the rationale for implicit over explicit.

Closer approximation of dwim semantics. There's been tons of patch series
all over drm and related places to get there, once we have a big pile of
implementations and know what the dwim semantics should be. Individually
they're all not much, in aggregate they substantially simplify simple
drivers.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 04/15] dma-fence: Make ->wait callback optional

[Daniel Vetter]

On Fri, May 04, 2018 at 10:17:22AM +0200, Daniel Vetter wrote:
> On Fri, May 04, 2018 at 09:09:10AM +0100, Chris Wilson wrote:
> > Quoting Daniel Vetter (2018-05-03 15:25:52)
> > > Almost everyone uses dma_fence_default_wait.
> > > 
> > > v2: Also remove the BUG_ON(!ops->wait) (Chris).
> > 
> > I just don't get the rationale for implicit over explicit.
> 
> Closer approximation of dwim semantics. There's been tons of patch series
> all over drm and related places to get there, once we have a big pile of
> implementations and know what the dwim semantics should be. Individually
> they're all not much, in aggregate they substantially simplify simple
> drivers.

I also think clearer separation between optional optimization hooks and
mandatory core parts is useful in itself.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 04/15] dma-fence: Make ->wait callback optional

[Daniel Vetter]

On Fri, May 04, 2018 at 09:31:33AM +0100, Chris Wilson wrote:
> Quoting Daniel Vetter (2018-05-04 09:23:01)
> > On Fri, May 04, 2018 at 10:17:22AM +0200, Daniel Vetter wrote:
> > > On Fri, May 04, 2018 at 09:09:10AM +0100, Chris Wilson wrote:
> > > > Quoting Daniel Vetter (2018-05-03 15:25:52)
> > > > > Almost everyone uses dma_fence_default_wait.
> > > > > 
> > > > > v2: Also remove the BUG_ON(!ops->wait) (Chris).
> > > > 
> > > > I just don't get the rationale for implicit over explicit.
> > > 
> > > Closer approximation of dwim semantics. There's been tons of patch series
> > > all over drm and related places to get there, once we have a big pile of
> > > implementations and know what the dwim semantics should be. Individually
> > > they're all not much, in aggregate they substantially simplify simple
> > > drivers.
> > 
> > I also think clearer separation between optional optimization hooks and
> > mandatory core parts is useful in itself.
> 
> A new spelling of midlayer ;) I don't see the contradiction with a
> driver saying use the default and simplicity. (I know which one the
> compiler thinks is simpler ;)

If the compiler overhead is real then I guess it would makes to be
explicit. I don't expect that to be a problem though for a blocking
function.

I disagree on this being a midlayer - you can still overwrite everything
you please to. What it does help is people doing less copypasting (and
assorted bugs), at least in the grand scheme of things. And we do have a
_lot_ more random small drivers than just a few years ago. Reducing the
amount of explicit typing just to get default bahaviour has been an
ongoing theme for a few years now, and your objection here is about the
first that this is not a good idea. So I'm somewhat confused.

It's ofc not all that useful when looking only through the i915
perspective, where we overwrite almost everything anyway. But the
ecosystem is a bit bigger than just i915.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 04/15] dma-fence: Make ->wait callback optional

[Daniel Vetter]

On Fri, May 4, 2018 at 11:16 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Quoting Daniel Vetter (2018-05-04 09:57:59)
>> On Fri, May 04, 2018 at 09:31:33AM +0100, Chris Wilson wrote:
>> > Quoting Daniel Vetter (2018-05-04 09:23:01)
>> > > On Fri, May 04, 2018 at 10:17:22AM +0200, Daniel Vetter wrote:
>> > > > On Fri, May 04, 2018 at 09:09:10AM +0100, Chris Wilson wrote:
>> > > > > Quoting Daniel Vetter (2018-05-03 15:25:52)
>> > > > > > Almost everyone uses dma_fence_default_wait.
>> > > > > >
>> > > > > > v2: Also remove the BUG_ON(!ops->wait) (Chris).
>> > > > >
>> > > > > I just don't get the rationale for implicit over explicit.
>> > > >
>> > > > Closer approximation of dwim semantics. There's been tons of patch series
>> > > > all over drm and related places to get there, once we have a big pile of
>> > > > implementations and know what the dwim semantics should be. Individually
>> > > > they're all not much, in aggregate they substantially simplify simple
>> > > > drivers.
>> > >
>> > > I also think clearer separation between optional optimization hooks and
>> > > mandatory core parts is useful in itself.
>> >
>> > A new spelling of midlayer ;) I don't see the contradiction with a
>> > driver saying use the default and simplicity. (I know which one the
>> > compiler thinks is simpler ;)
>>
>> If the compiler overhead is real then I guess it would makes to be
>> explicit. I don't expect that to be a problem though for a blocking
>> function.
>>
>> I disagree on this being a midlayer - you can still overwrite everything
>> you please to. What it does help is people doing less copypasting (and
>> assorted bugs), at least in the grand scheme of things. And we do have a
>> _lot_ more random small drivers than just a few years ago. Reducing the
>> amount of explicit typing just to get default bahaviour has been an
>> ongoing theme for a few years now, and your objection here is about the
>> first that this is not a good idea. So I'm somewhat confused.
>
> I'm just saying I don't see any rationale for this patch.
>
>         "Almost everyone uses dma_fence_default_wait."
>
> Why change?
>
> Making it look simpler on the surface, so that you don't have to think
> about things straight away? I understand the appeal, but I do worry
> about it just being an illusion. (Cutting and pasting a line saying
> .wait = default_wait, doesn't feel that onerous, as you likely cut and
> paste the ops anyway, and at the very least you are reminded about some
> of the interactions. You could even have default initializers and/or
> magic macros to hide the cut and paste; maybe a simple_dma_fence [now
> that's a midlayer!] but I haven't looked.)

In really monolithic vtables like drm_driver we do use default
function macros, so you type 1 line, get them all. But dma_fence_ops
is pretty small, and most drivers only implement a few callbacks. Also
note that e.g. the ->release callback already works like that, so this
pattern is there already. I simply extended it to ->wait and
->enable_signaling. Also note that I leave the EXPORT_SYMBOL in place,
you can still wrap dma_fence_default_wait if you wish to do so.

But I just realized that I didn't clean out the optional release
hooks, I guess I should do that too (for the few cases it's not yet
done) and respin.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 04/15] dma-fence: Make ->wait callback optional

[Christian König]

Am 04.05.2018 um 11:25 schrieb Daniel Vetter:
> On Fri, May 4, 2018 at 11:16 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> Quoting Daniel Vetter (2018-05-04 09:57:59)
>>> On Fri, May 04, 2018 at 09:31:33AM +0100, Chris Wilson wrote:
>>>> Quoting Daniel Vetter (2018-05-04 09:23:01)
>>>>> On Fri, May 04, 2018 at 10:17:22AM +0200, Daniel Vetter wrote:
>>>>>> On Fri, May 04, 2018 at 09:09:10AM +0100, Chris Wilson wrote:
>>>>>>> Quoting Daniel Vetter (2018-05-03 15:25:52)
>>>>>>>> Almost everyone uses dma_fence_default_wait.
>>>>>>>>
>>>>>>>> v2: Also remove the BUG_ON(!ops->wait) (Chris).
>>>>>>> I just don't get the rationale for implicit over explicit.
>>>>>> Closer approximation of dwim semantics. There's been tons of patch series
>>>>>> all over drm and related places to get there, once we have a big pile of
>>>>>> implementations and know what the dwim semantics should be. Individually
>>>>>> they're all not much, in aggregate they substantially simplify simple
>>>>>> drivers.
>>>>> I also think clearer separation between optional optimization hooks and
>>>>> mandatory core parts is useful in itself.
>>>> A new spelling of midlayer ;) I don't see the contradiction with a
>>>> driver saying use the default and simplicity. (I know which one the
>>>> compiler thinks is simpler ;)
>>> If the compiler overhead is real then I guess it would makes to be
>>> explicit. I don't expect that to be a problem though for a blocking
>>> function.
>>>
>>> I disagree on this being a midlayer - you can still overwrite everything
>>> you please to. What it does help is people doing less copypasting (and
>>> assorted bugs), at least in the grand scheme of things. And we do have a
>>> _lot_ more random small drivers than just a few years ago. Reducing the
>>> amount of explicit typing just to get default bahaviour has been an
>>> ongoing theme for a few years now, and your objection here is about the
>>> first that this is not a good idea. So I'm somewhat confused.
>> I'm just saying I don't see any rationale for this patch.
>>
>>          "Almost everyone uses dma_fence_default_wait."
>>
>> Why change?
>>
>> Making it look simpler on the surface, so that you don't have to think
>> about things straight away? I understand the appeal, but I do worry
>> about it just being an illusion. (Cutting and pasting a line saying
>> .wait = default_wait, doesn't feel that onerous, as you likely cut and
>> paste the ops anyway, and at the very least you are reminded about some
>> of the interactions. You could even have default initializers and/or
>> magic macros to hide the cut and paste; maybe a simple_dma_fence [now
>> that's a midlayer!] but I haven't looked.)
> In really monolithic vtables like drm_driver we do use default
> function macros, so you type 1 line, get them all. But dma_fence_ops
> is pretty small, and most drivers only implement a few callbacks. Also
> note that e.g. the ->release callback already works like that, so this
> pattern is there already. I simply extended it to ->wait and
> ->enable_signaling. Also note that I leave the EXPORT_SYMBOL in place,
> you can still wrap dma_fence_default_wait if you wish to do so.
>
> But I just realized that I didn't clean out the optional release
> hooks, I guess I should do that too (for the few cases it's not yet
> done) and respin.

I kind of agree with Chris here, but also see the practical problem to 
copy the default function in all the implementations.

We had the same problem in TTM and I also don't really like the result 
to always have that "if (some_callback) default(); else some_callback();".

Might be that the run time overhead is negligible, but it doesn't feels 
right from the coding style perspective.

Christian.

> -Daniel

Re: [PATCH 04/15] dma-fence: Make ->wait callback optional

[Daniel Vetter]

On Fri, May 04, 2018 at 03:17:08PM +0200, Christian König wrote:
> Am 04.05.2018 um 11:25 schrieb Daniel Vetter:
> > On Fri, May 4, 2018 at 11:16 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > Quoting Daniel Vetter (2018-05-04 09:57:59)
> > > > On Fri, May 04, 2018 at 09:31:33AM +0100, Chris Wilson wrote:
> > > > > Quoting Daniel Vetter (2018-05-04 09:23:01)
> > > > > > On Fri, May 04, 2018 at 10:17:22AM +0200, Daniel Vetter wrote:
> > > > > > > On Fri, May 04, 2018 at 09:09:10AM +0100, Chris Wilson wrote:
> > > > > > > > Quoting Daniel Vetter (2018-05-03 15:25:52)
> > > > > > > > > Almost everyone uses dma_fence_default_wait.
> > > > > > > > > 
> > > > > > > > > v2: Also remove the BUG_ON(!ops->wait) (Chris).
> > > > > > > > I just don't get the rationale for implicit over explicit.
> > > > > > > Closer approximation of dwim semantics. There's been tons of patch series
> > > > > > > all over drm and related places to get there, once we have a big pile of
> > > > > > > implementations and know what the dwim semantics should be. Individually
> > > > > > > they're all not much, in aggregate they substantially simplify simple
> > > > > > > drivers.
> > > > > > I also think clearer separation between optional optimization hooks and
> > > > > > mandatory core parts is useful in itself.
> > > > > A new spelling of midlayer ;) I don't see the contradiction with a
> > > > > driver saying use the default and simplicity. (I know which one the
> > > > > compiler thinks is simpler ;)
> > > > If the compiler overhead is real then I guess it would makes to be
> > > > explicit. I don't expect that to be a problem though for a blocking
> > > > function.
> > > > 
> > > > I disagree on this being a midlayer - you can still overwrite everything
> > > > you please to. What it does help is people doing less copypasting (and
> > > > assorted bugs), at least in the grand scheme of things. And we do have a
> > > > _lot_ more random small drivers than just a few years ago. Reducing the
> > > > amount of explicit typing just to get default bahaviour has been an
> > > > ongoing theme for a few years now, and your objection here is about the
> > > > first that this is not a good idea. So I'm somewhat confused.
> > > I'm just saying I don't see any rationale for this patch.
> > > 
> > >          "Almost everyone uses dma_fence_default_wait."
> > > 
> > > Why change?
> > > 
> > > Making it look simpler on the surface, so that you don't have to think
> > > about things straight away? I understand the appeal, but I do worry
> > > about it just being an illusion. (Cutting and pasting a line saying
> > > .wait = default_wait, doesn't feel that onerous, as you likely cut and
> > > paste the ops anyway, and at the very least you are reminded about some
> > > of the interactions. You could even have default initializers and/or
> > > magic macros to hide the cut and paste; maybe a simple_dma_fence [now
> > > that's a midlayer!] but I haven't looked.)
> > In really monolithic vtables like drm_driver we do use default
> > function macros, so you type 1 line, get them all. But dma_fence_ops
> > is pretty small, and most drivers only implement a few callbacks. Also
> > note that e.g. the ->release callback already works like that, so this
> > pattern is there already. I simply extended it to ->wait and
> > ->enable_signaling. Also note that I leave the EXPORT_SYMBOL in place,
> > you can still wrap dma_fence_default_wait if you wish to do so.
> > 
> > But I just realized that I didn't clean out the optional release
> > hooks, I guess I should do that too (for the few cases it's not yet
> > done) and respin.
> 
> I kind of agree with Chris here, but also see the practical problem to copy
> the default function in all the implementations.
> 
> We had the same problem in TTM and I also don't really like the result to
> always have that "if (some_callback) default(); else some_callback();".
> 
> Might be that the run time overhead is negligible, but it doesn't feels
> right from the coding style perspective.

Hm, maybe I've seen too much bad code, but modeset helpers is choke full
of exactly that pattern. It's imo also a trade-off. If you have a fairly
specialized library like ttm that's used by relatively few things, doing
everything explicitly is probably better. It's also where kms started out
from.

But if you have a huge pile of fairly simple drivers, imo the balance
starts to tip the other way, and a bit of additional logic in the shared
code to make all the implementations a notch simpler is good. If we
wouldn't have acquired quite a pile of dma_fence implementations I
wouldn't have bothered with all this.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH] dma-fence: Polish kernel-doc for dma-fence.c

[Daniel Vetter]

- Intro section that links to how this is exposed to userspace.
- Lots more hyperlinks.
- Minor clarifications and style polish

v2: Add misplaced hunk of kerneldoc from a different patch.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: linux-media@vger.kernel.org
Cc: linaro-mm-sig@lists.linaro.org
---
 Documentation/driver-api/dma-buf.rst |   6 ++
 drivers/dma-buf/dma-fence.c          | 147 +++++++++++++++++++--------
 2 files changed, 109 insertions(+), 44 deletions(-)

diff --git a/Documentation/driver-api/dma-buf.rst b/Documentation/driver-api/dma-buf.rst
index dc384f2f7f34..b541e97c7ab1 100644
--- a/Documentation/driver-api/dma-buf.rst
+++ b/Documentation/driver-api/dma-buf.rst
@@ -130,6 +130,12 @@ Reservation Objects
 DMA Fences
 ----------
 
+.. kernel-doc:: drivers/dma-buf/dma-fence.c
+   :doc: DMA fences overview
+
+DMA Fences Functions Reference
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
 .. kernel-doc:: drivers/dma-buf/dma-fence.c
    :export:
 
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 7a92f85a4cec..1551ca7df394 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -38,12 +38,43 @@ EXPORT_TRACEPOINT_SYMBOL(dma_fence_enable_signal);
  */
 static atomic64_t dma_fence_context_counter = ATOMIC64_INIT(0);
 
+/**
+ * DOC: DMA fences overview
+ *
+ * DMA fences, represented by &struct dma_fence, are the kernel internal
+ * synchronization primitive for DMA operations like GPU rendering, video
+ * encoding/decoding, or displaying buffers on a screen.
+ *
+ * A fence is initialized using dma_fence_init() and completed using
+ * dma_fence_signal(). Fences are associated with a context, allocated through
+ * dma_fence_context_alloc(), and all fences on the same context are
+ * fully ordered.
+ *
+ * Since the purposes of fences is to facilitate cross-device and
+ * cross-application synchronization, there's multiple ways to use one:
+ *
+ * - Individual fences can be exposed as a &sync_file, accessed as a file
+ *   descriptor from userspace, created by calling sync_file_create(). This is
+ *   called explicit fencing, since userspace passes around explicit
+ *   synchronization points.
+ *
+ * - Some subsystems also have their own explicit fencing primitives, like
+ *   &drm_syncobj. Compared to &sync_file, a &drm_syncobj allows the underlying
+ *   fence to be updated.
+ *
+ * - Then there's also implicit fencing, where the synchronization points are
+ *   implicitly passed around as part of shared &dma_buf instances. Such
+ *   implicit fences are stored in &struct reservation_object through the
+ *   &dma_buf.resv pointer.
+ */
+
 /**
  * dma_fence_context_alloc - allocate an array of fence contexts
- * @num:	[in]	amount of contexts to allocate
+ * @num: amount of contexts to allocate
  *
- * This function will return the first index of the number of fences allocated.
- * The fence context is used for setting fence->context to a unique number.
+ * This function will return the first index of the number of fence contexts
+ * allocated.  The fence context is used for setting &dma_fence.context to a
+ * unique number by passing the context to dma_fence_init().
  */
 u64 dma_fence_context_alloc(unsigned num)
 {
@@ -59,10 +90,14 @@ EXPORT_SYMBOL(dma_fence_context_alloc);
  * Signal completion for software callbacks on a fence, this will unblock
  * dma_fence_wait() calls and run all the callbacks added with
  * dma_fence_add_callback(). Can be called multiple times, but since a fence
- * can only go from unsignaled to signaled state, it will only be effective
- * the first time.
+ * can only go from the unsignaled to the signaled state and not back, it will
+ * only be effective the first time.
+ *
+ * Unlike dma_fence_signal(), this function must be called with &dma_fence.lock
+ * held.
  *
- * Unlike dma_fence_signal, this function must be called with fence->lock held.
+ * Returns 0 on success and a negative error value when @fence has been
+ * signalled already.
  */
 int dma_fence_signal_locked(struct dma_fence *fence)
 {
@@ -102,8 +137,11 @@ EXPORT_SYMBOL(dma_fence_signal_locked);
  * Signal completion for software callbacks on a fence, this will unblock
  * dma_fence_wait() calls and run all the callbacks added with
  * dma_fence_add_callback(). Can be called multiple times, but since a fence
- * can only go from unsignaled to signaled state, it will only be effective
- * the first time.
+ * can only go from the unsignaled to the signaled state and not back, it will
+ * only be effective the first time.
+ *
+ * Returns 0 on success and a negative error value when @fence has been
+ * signalled already.
  */
 int dma_fence_signal(struct dma_fence *fence)
 {
@@ -136,9 +174,9 @@ EXPORT_SYMBOL(dma_fence_signal);
 /**
  * dma_fence_wait_timeout - sleep until the fence gets signaled
  * or until timeout elapses
- * @fence:	[in]	the fence to wait on
- * @intr:	[in]	if true, do an interruptible wait
- * @timeout:	[in]	timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT
+ * @fence: the fence to wait on
+ * @intr: if true, do an interruptible wait
+ * @timeout: timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT
  *
  * Returns -ERESTARTSYS if interrupted, 0 if the wait timed out, or the
  * remaining timeout in jiffies on success. Other error values may be
@@ -148,6 +186,8 @@ EXPORT_SYMBOL(dma_fence_signal);
  * directly or indirectly (buf-mgr between reservation and committing)
  * holds a reference to the fence, otherwise the fence might be
  * freed before return, resulting in undefined behavior.
+ *
+ * See also dma_fence_wait() and dma_fence_wait_any_timeout().
  */
 signed long
 dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout)
@@ -167,6 +207,13 @@ dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout)
 }
 EXPORT_SYMBOL(dma_fence_wait_timeout);
 
+/**
+ * dma_fence_release - default relese function for fences
+ * @kref: &dma_fence.recfount
+ *
+ * This is the default release functions for &dma_fence. Drivers shouldn't call
+ * this directly, but instead call dma_fence_put().
+ */
 void dma_fence_release(struct kref *kref)
 {
 	struct dma_fence *fence =
@@ -184,6 +231,13 @@ void dma_fence_release(struct kref *kref)
 }
 EXPORT_SYMBOL(dma_fence_release);
 
+/**
+ * dma_fence_free - default release function for &dma_fence.
+ * @fence: fence to release
+ *
+ * This is the default implementation for &dma_fence_ops.release. It calls
+ * kfree_rcu() on @fence.
+ */
 void dma_fence_free(struct dma_fence *fence)
 {
 	kfree_rcu(fence, rcu);
@@ -192,10 +246,11 @@ EXPORT_SYMBOL(dma_fence_free);
 
 /**
  * dma_fence_enable_sw_signaling - enable signaling on fence
- * @fence:	[in]	the fence to enable
+ * @fence: the fence to enable
  *
- * this will request for sw signaling to be enabled, to make the fence
- * complete as soon as possible
+ * This will request for sw signaling to be enabled, to make the fence
+ * complete as soon as possible. This calls &dma_fence_ops.enable_signaling
+ * internally.
  */
 void dma_fence_enable_sw_signaling(struct dma_fence *fence)
 {
@@ -220,24 +275,24 @@ EXPORT_SYMBOL(dma_fence_enable_sw_signaling);
 /**
  * dma_fence_add_callback - add a callback to be called when the fence
  * is signaled
- * @fence:	[in]	the fence to wait on
- * @cb:		[in]	the callback to register
- * @func:	[in]	the function to call
+ * @fence: the fence to wait on
+ * @cb: the callback to register
+ * @func: the function to call
  *
- * cb will be initialized by dma_fence_add_callback, no initialization
+ * @cb will be initialized by dma_fence_add_callback(), no initialization
  * by the caller is required. Any number of callbacks can be registered
  * to a fence, but a callback can only be registered to one fence at a time.
  *
  * Note that the callback can be called from an atomic context.  If
  * fence is already signaled, this function will return -ENOENT (and
- * *not* call the callback)
+ * *not* call the callback).
  *
  * Add a software callback to the fence. Same restrictions apply to
- * refcount as it does to dma_fence_wait, however the caller doesn't need to
- * keep a refcount to fence afterwards: when software access is enabled,
- * the creator of the fence is required to keep the fence alive until
- * after it signals with dma_fence_signal. The callback itself can be called
- * from irq context.
+ * refcount as it does to dma_fence_wait(), however the caller doesn't need to
+ * keep a refcount to fence afterward dma_fence_add_callback() has returned:
+ * when software access is enabled, the creator of the fence is required to keep
+ * the fence alive until after it signals with dma_fence_signal(). The callback
+ * itself can be called from irq context.
  *
  * Returns 0 in case of success, -ENOENT if the fence is already signaled
  * and -EINVAL in case of error.
@@ -286,7 +341,7 @@ EXPORT_SYMBOL(dma_fence_add_callback);
 
 /**
  * dma_fence_get_status - returns the status upon completion
- * @fence: [in]	the dma_fence to query
+ * @fence: the dma_fence to query
  *
  * This wraps dma_fence_get_status_locked() to return the error status
  * condition on a signaled fence. See dma_fence_get_status_locked() for more
@@ -311,8 +366,8 @@ EXPORT_SYMBOL(dma_fence_get_status);
 
 /**
  * dma_fence_remove_callback - remove a callback from the signaling list
- * @fence:	[in]	the fence to wait on
- * @cb:		[in]	the callback to remove
+ * @fence: the fence to wait on
+ * @cb: the callback to remove
  *
  * Remove a previously queued callback from the fence. This function returns
  * true if the callback is successfully removed, or false if the fence has
@@ -323,6 +378,9 @@ EXPORT_SYMBOL(dma_fence_get_status);
  * doing, since deadlocks and race conditions could occur all too easily. For
  * this reason, it should only ever be done on hardware lockup recovery,
  * with a reference held to the fence.
+ *
+ * Behaviour is undefined if @cb has not been added to @fence using
+ * dma_fence_add_callback() beforehand.
  */
 bool
 dma_fence_remove_callback(struct dma_fence *fence, struct dma_fence_cb *cb)
@@ -359,9 +417,9 @@ dma_fence_default_wait_cb(struct dma_fence *fence, struct dma_fence_cb *cb)
 /**
  * dma_fence_default_wait - default sleep until the fence gets signaled
  * or until timeout elapses
- * @fence:	[in]	the fence to wait on
- * @intr:	[in]	if true, do an interruptible wait
- * @timeout:	[in]	timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT
+ * @fence: the fence to wait on
+ * @intr: if true, do an interruptible wait
+ * @timeout: timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT
  *
  * Returns -ERESTARTSYS if interrupted, 0 if the wait timed out, or the
  * remaining timeout in jiffies on success. If timeout is zero the value one is
@@ -454,12 +512,12 @@ dma_fence_test_signaled_any(struct dma_fence **fences, uint32_t count,
 /**
  * dma_fence_wait_any_timeout - sleep until any fence gets signaled
  * or until timeout elapses
- * @fences:	[in]	array of fences to wait on
- * @count:	[in]	number of fences to wait on
- * @intr:	[in]	if true, do an interruptible wait
- * @timeout:	[in]	timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT
- * @idx:       [out]	the first signaled fence index, meaningful only on
- *			positive return
+ * @fences: array of fences to wait on
+ * @count: number of fences to wait on
+ * @intr: if true, do an interruptible wait
+ * @timeout: timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT
+ * @idx: used to store the first signaled fence index, meaningful only on
+ *	positive return
  *
  * Returns -EINVAL on custom fence wait implementation, -ERESTARTSYS if
  * interrupted, 0 if the wait timed out, or the remaining timeout in jiffies
@@ -468,6 +526,8 @@ dma_fence_test_signaled_any(struct dma_fence **fences, uint32_t count,
  * Synchronous waits for the first fence in the array to be signaled. The
  * caller needs to hold a reference to all fences in the array, otherwise a
  * fence might be freed before return, resulting in undefined behavior.
+ *
+ * See also dma_fence_wait() and dma_fence_wait_timeout().
  */
 signed long
 dma_fence_wait_any_timeout(struct dma_fence **fences, uint32_t count,
@@ -540,19 +600,18 @@ EXPORT_SYMBOL(dma_fence_wait_any_timeout);
 
 /**
  * dma_fence_init - Initialize a custom fence.
- * @fence:	[in]	the fence to initialize
- * @ops:	[in]	the dma_fence_ops for operations on this fence
- * @lock:	[in]	the irqsafe spinlock to use for locking this fence
- * @context:	[in]	the execution context this fence is run on
- * @seqno:	[in]	a linear increasing sequence number for this context
+ * @fence: the fence to initialize
+ * @ops: the dma_fence_ops for operations on this fence
+ * @lock: the irqsafe spinlock to use for locking this fence
+ * @context: the execution context this fence is run on
+ * @seqno: a linear increasing sequence number for this context
  *
  * Initializes an allocated fence, the caller doesn't have to keep its
  * refcount after committing with this fence, but it will need to hold a
- * refcount again if dma_fence_ops.enable_signaling gets called. This can
- * be used for other implementing other types of fence.
+ * refcount again if &dma_fence_ops.enable_signaling gets called.
  *
  * context and seqno are used for easy comparison between fences, allowing
- * to check which fence is later by simply using dma_fence_later.
+ * to check which fence is later by simply using dma_fence_later().
  */
 void
 dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
-- 
2.17.0

[PATCH] dma-fence: Make ->enable_signaling optional

[Daniel Vetter]

Many drivers have a trivial implementation for ->enable_signaling.
Let's make it optional by assuming that signalling is already
available when the callback isn't present.

v2: Don't do the trick to set the ENABLE_SIGNAL_BIT
unconditionally, it results in an expensive spinlock take for
everyone. Instead just check if the callback is present. Suggested by
Maarten.

Also move misplaced kerneldoc hunk to the right patch.

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Christian König <christian.koenig@amd.com> (v1)
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: linux-media@vger.kernel.org
Cc: linaro-mm-sig@lists.linaro.org
---
 drivers/dma-buf/dma-fence.c | 9 +++++----
 include/linux/dma-fence.h   | 3 ++-
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 4edb9fd3cf47..dd01a1720be9 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -200,7 +200,8 @@ void dma_fence_enable_sw_signaling(struct dma_fence *fence)
 
 	if (!test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
 			      &fence->flags) &&
-	    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
+	    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags) &&
+	    fence->ops->enable_signaling) {
 		trace_dma_fence_enable_signal(fence);
 
 		spin_lock_irqsave(fence->lock, flags);
@@ -260,7 +261,7 @@ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb,
 
 	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
 		ret = -ENOENT;
-	else if (!was_set) {
+	else if (!was_set && fence->ops->enable_signaling) {
 		trace_dma_fence_enable_signal(fence);
 
 		if (!fence->ops->enable_signaling(fence)) {
@@ -388,7 +389,7 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
 	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
 		goto out;
 
-	if (!was_set) {
+	if (!was_set && fence->ops->enable_signaling) {
 		trace_dma_fence_enable_signal(fence);
 
 		if (!fence->ops->enable_signaling(fence)) {
@@ -560,7 +561,7 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
 	       spinlock_t *lock, u64 context, unsigned seqno)
 {
 	BUG_ON(!lock);
-	BUG_ON(!ops || !ops->wait || !ops->enable_signaling ||
+	BUG_ON(!ops || !ops->wait ||
 	       !ops->get_driver_name || !ops->get_timeline_name);
 
 	kref_init(&fence->refcount);
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 111aefe1c956..c053d19e1e24 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -166,7 +166,8 @@ struct dma_fence_ops {
 	 * released when the fence is signalled (through e.g. the interrupt
 	 * handler).
 	 *
-	 * This callback is mandatory.
+	 * 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);
 
-- 
2.17.0

Re: [PATCH] dma-fence: Make ->enable_signaling optional

[Maarten Lankhorst]

Op 04-05-18 om 16:10 schreef Daniel Vetter:
> Many drivers have a trivial implementation for ->enable_signaling.
> Let's make it optional by assuming that signalling is already
> available when the callback isn't present.
>
> v2: Don't do the trick to set the ENABLE_SIGNAL_BIT
> unconditionally, it results in an expensive spinlock take for
> everyone. Instead just check if the callback is present. Suggested by
> Maarten.
>
> Also move misplaced kerneldoc hunk to the right patch.
>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Reviewed-by: Christian König <christian.koenig@amd.com> (v1)
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Gustavo Padovan <gustavo@padovan.org>
> Cc: linux-media@vger.kernel.org
> Cc: linaro-mm-sig@lists.linaro.org
> ---
>  drivers/dma-buf/dma-fence.c | 9 +++++----
>  include/linux/dma-fence.h   | 3 ++-
>  2 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index 4edb9fd3cf47..dd01a1720be9 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -200,7 +200,8 @@ void dma_fence_enable_sw_signaling(struct dma_fence *fence)
>  
>  	if (!test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
>  			      &fence->flags) &&
> -	    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
> +	    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags) &&
> +	    fence->ops->enable_signaling) {
>  		trace_dma_fence_enable_signal(fence);
>  
>  		spin_lock_irqsave(fence->lock, flags);
> @@ -260,7 +261,7 @@ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb,
>  
>  	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>  		ret = -ENOENT;
> -	else if (!was_set) {
> +	else if (!was_set && fence->ops->enable_signaling) {
>  		trace_dma_fence_enable_signal(fence);
>  
>  		if (!fence->ops->enable_signaling(fence)) {
> @@ -388,7 +389,7 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
>  	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>  		goto out;
>  
> -	if (!was_set) {
> +	if (!was_set && fence->ops->enable_signaling) {
>  		trace_dma_fence_enable_signal(fence);
>  
>  		if (!fence->ops->enable_signaling(fence)) {
> @@ -560,7 +561,7 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
>  	       spinlock_t *lock, u64 context, unsigned seqno)
>  {
>  	BUG_ON(!lock);
> -	BUG_ON(!ops || !ops->wait || !ops->enable_signaling ||
> +	BUG_ON(!ops || !ops->wait ||
>  	       !ops->get_driver_name || !ops->get_timeline_name);
>  
>  	kref_init(&fence->refcount);
> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index 111aefe1c956..c053d19e1e24 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -166,7 +166,8 @@ struct dma_fence_ops {
>  	 * released when the fence is signalled (through e.g. the interrupt
>  	 * handler).
>  	 *
> -	 * This callback is mandatory.
> +	 * 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);
>  

Much better. :)

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Re: [PATCH 04/15] dma-fence: Make ->wait callback optional

[Daniel Vetter]

On Fri, May 04, 2018 at 03:47:59PM +0200, Daniel Vetter wrote:
> On Fri, May 04, 2018 at 03:17:08PM +0200, Christian König wrote:
> > Am 04.05.2018 um 11:25 schrieb Daniel Vetter:
> > > On Fri, May 4, 2018 at 11:16 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > > Quoting Daniel Vetter (2018-05-04 09:57:59)
> > > > > On Fri, May 04, 2018 at 09:31:33AM +0100, Chris Wilson wrote:
> > > > > > Quoting Daniel Vetter (2018-05-04 09:23:01)
> > > > > > > On Fri, May 04, 2018 at 10:17:22AM +0200, Daniel Vetter wrote:
> > > > > > > > On Fri, May 04, 2018 at 09:09:10AM +0100, Chris Wilson wrote:
> > > > > > > > > Quoting Daniel Vetter (2018-05-03 15:25:52)
> > > > > > > > > > Almost everyone uses dma_fence_default_wait.
> > > > > > > > > > 
> > > > > > > > > > v2: Also remove the BUG_ON(!ops->wait) (Chris).
> > > > > > > > > I just don't get the rationale for implicit over explicit.
> > > > > > > > Closer approximation of dwim semantics. There's been tons of patch series
> > > > > > > > all over drm and related places to get there, once we have a big pile of
> > > > > > > > implementations and know what the dwim semantics should be. Individually
> > > > > > > > they're all not much, in aggregate they substantially simplify simple
> > > > > > > > drivers.
> > > > > > > I also think clearer separation between optional optimization hooks and
> > > > > > > mandatory core parts is useful in itself.
> > > > > > A new spelling of midlayer ;) I don't see the contradiction with a
> > > > > > driver saying use the default and simplicity. (I know which one the
> > > > > > compiler thinks is simpler ;)
> > > > > If the compiler overhead is real then I guess it would makes to be
> > > > > explicit. I don't expect that to be a problem though for a blocking
> > > > > function.
> > > > > 
> > > > > I disagree on this being a midlayer - you can still overwrite everything
> > > > > you please to. What it does help is people doing less copypasting (and
> > > > > assorted bugs), at least in the grand scheme of things. And we do have a
> > > > > _lot_ more random small drivers than just a few years ago. Reducing the
> > > > > amount of explicit typing just to get default bahaviour has been an
> > > > > ongoing theme for a few years now, and your objection here is about the
> > > > > first that this is not a good idea. So I'm somewhat confused.
> > > > I'm just saying I don't see any rationale for this patch.
> > > > 
> > > >          "Almost everyone uses dma_fence_default_wait."
> > > > 
> > > > Why change?
> > > > 
> > > > Making it look simpler on the surface, so that you don't have to think
> > > > about things straight away? I understand the appeal, but I do worry
> > > > about it just being an illusion. (Cutting and pasting a line saying
> > > > .wait = default_wait, doesn't feel that onerous, as you likely cut and
> > > > paste the ops anyway, and at the very least you are reminded about some
> > > > of the interactions. You could even have default initializers and/or
> > > > magic macros to hide the cut and paste; maybe a simple_dma_fence [now
> > > > that's a midlayer!] but I haven't looked.)
> > > In really monolithic vtables like drm_driver we do use default
> > > function macros, so you type 1 line, get them all. But dma_fence_ops
> > > is pretty small, and most drivers only implement a few callbacks. Also
> > > note that e.g. the ->release callback already works like that, so this
> > > pattern is there already. I simply extended it to ->wait and
> > > ->enable_signaling. Also note that I leave the EXPORT_SYMBOL in place,
> > > you can still wrap dma_fence_default_wait if you wish to do so.
> > > 
> > > But I just realized that I didn't clean out the optional release
> > > hooks, I guess I should do that too (for the few cases it's not yet
> > > done) and respin.
> > 
> > I kind of agree with Chris here, but also see the practical problem to copy
> > the default function in all the implementations.
> > 
> > We had the same problem in TTM and I also don't really like the result to
> > always have that "if (some_callback) default(); else some_callback();".
> > 
> > Might be that the run time overhead is negligible, but it doesn't feels
> > right from the coding style perspective.
> 
> Hm, maybe I've seen too much bad code, but modeset helpers is choke full
> of exactly that pattern. It's imo also a trade-off. If you have a fairly
> specialized library like ttm that's used by relatively few things, doing
> everything explicitly is probably better. It's also where kms started out
> from.
> 
> But if you have a huge pile of fairly simple drivers, imo the balance
> starts to tip the other way, and a bit of additional logic in the shared
> code to make all the implementations a notch simpler is good. If we
> wouldn't have acquired quite a pile of dma_fence implementations I
> wouldn't have bothered with all this.

So ack/nack on this (i.e. do you retract your original r-b or not)? It's
kinda holding up all the cleanup patches below ...

I went ahead and applied the first three patches of this series meanwhile.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 04/15] dma-fence: Make ->wait callback optional

[Christian König]

Am 02.07.2018 um 10:23 schrieb Daniel Vetter:
> On Fri, May 04, 2018 at 03:47:59PM +0200, Daniel Vetter wrote:
>> On Fri, May 04, 2018 at 03:17:08PM +0200, Christian König wrote:
>>> Am 04.05.2018 um 11:25 schrieb Daniel Vetter:
>>>> On Fri, May 4, 2018 at 11:16 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>>>>> Quoting Daniel Vetter (2018-05-04 09:57:59)
>>>>>> On Fri, May 04, 2018 at 09:31:33AM +0100, Chris Wilson wrote:
>>>>>>> Quoting Daniel Vetter (2018-05-04 09:23:01)
>>>>>>>> On Fri, May 04, 2018 at 10:17:22AM +0200, Daniel Vetter wrote:
>>>>>>>>> On Fri, May 04, 2018 at 09:09:10AM +0100, Chris Wilson wrote:
>>>>>>>>>> Quoting Daniel Vetter (2018-05-03 15:25:52)
>>>>>>>>>>> Almost everyone uses dma_fence_default_wait.
>>>>>>>>>>>
>>>>>>>>>>> v2: Also remove the BUG_ON(!ops->wait) (Chris).
>>>>>>>>>> I just don't get the rationale for implicit over explicit.
>>>>>>>>> Closer approximation of dwim semantics. There's been tons of patch series
>>>>>>>>> all over drm and related places to get there, once we have a big pile of
>>>>>>>>> implementations and know what the dwim semantics should be. Individually
>>>>>>>>> they're all not much, in aggregate they substantially simplify simple
>>>>>>>>> drivers.
>>>>>>>> I also think clearer separation between optional optimization hooks and
>>>>>>>> mandatory core parts is useful in itself.
>>>>>>> A new spelling of midlayer ;) I don't see the contradiction with a
>>>>>>> driver saying use the default and simplicity. (I know which one the
>>>>>>> compiler thinks is simpler ;)
>>>>>> If the compiler overhead is real then I guess it would makes to be
>>>>>> explicit. I don't expect that to be a problem though for a blocking
>>>>>> function.
>>>>>>
>>>>>> I disagree on this being a midlayer - you can still overwrite everything
>>>>>> you please to. What it does help is people doing less copypasting (and
>>>>>> assorted bugs), at least in the grand scheme of things. And we do have a
>>>>>> _lot_ more random small drivers than just a few years ago. Reducing the
>>>>>> amount of explicit typing just to get default bahaviour has been an
>>>>>> ongoing theme for a few years now, and your objection here is about the
>>>>>> first that this is not a good idea. So I'm somewhat confused.
>>>>> I'm just saying I don't see any rationale for this patch.
>>>>>
>>>>>           "Almost everyone uses dma_fence_default_wait."
>>>>>
>>>>> Why change?
>>>>>
>>>>> Making it look simpler on the surface, so that you don't have to think
>>>>> about things straight away? I understand the appeal, but I do worry
>>>>> about it just being an illusion. (Cutting and pasting a line saying
>>>>> .wait = default_wait, doesn't feel that onerous, as you likely cut and
>>>>> paste the ops anyway, and at the very least you are reminded about some
>>>>> of the interactions. You could even have default initializers and/or
>>>>> magic macros to hide the cut and paste; maybe a simple_dma_fence [now
>>>>> that's a midlayer!] but I haven't looked.)
>>>> In really monolithic vtables like drm_driver we do use default
>>>> function macros, so you type 1 line, get them all. But dma_fence_ops
>>>> is pretty small, and most drivers only implement a few callbacks. Also
>>>> note that e.g. the ->release callback already works like that, so this
>>>> pattern is there already. I simply extended it to ->wait and
>>>> ->enable_signaling. Also note that I leave the EXPORT_SYMBOL in place,
>>>> you can still wrap dma_fence_default_wait if you wish to do so.
>>>>
>>>> But I just realized that I didn't clean out the optional release
>>>> hooks, I guess I should do that too (for the few cases it's not yet
>>>> done) and respin.
>>> I kind of agree with Chris here, but also see the practical problem to copy
>>> the default function in all the implementations.
>>>
>>> We had the same problem in TTM and I also don't really like the result to
>>> always have that "if (some_callback) default(); else some_callback();".
>>>
>>> Might be that the run time overhead is negligible, but it doesn't feels
>>> right from the coding style perspective.
>> Hm, maybe I've seen too much bad code, but modeset helpers is choke full
>> of exactly that pattern. It's imo also a trade-off. If you have a fairly
>> specialized library like ttm that's used by relatively few things, doing
>> everything explicitly is probably better. It's also where kms started out
>> from.
>>
>> But if you have a huge pile of fairly simple drivers, imo the balance
>> starts to tip the other way, and a bit of additional logic in the shared
>> code to make all the implementations a notch simpler is good. If we
>> wouldn't have acquired quite a pile of dma_fence implementations I
>> wouldn't have bothered with all this.
> So ack/nack on this (i.e. do you retract your original r-b or not)? It's
> kinda holding up all the cleanup patches below ...

Feel free to add my Acked-by for now, but I still have a kind of a gut 
feeling that we might want to revisit this decision at some time.

Christian.

>
> I went ahead and applied the first three patches of this series meanwhile.
> -Daniel