Re: [Linaro-mm-sig] [PATCH 4/7] fence: dma-buf cross-device synchronization (v11)

[Maarten Lankhorst <maarten.lankhorst@canonical.com>]

Op 22-01-13 16:13, Francesco Lavra schreef:
> Hi,
>
> On 01/15/2013 01:34 PM, Maarten Lankhorst wrote:
> [...]
>> diff --git a/include/linux/fence.h b/include/linux/fence.h
>> new file mode 100644
>> index 0000000..d9f091d
>> --- /dev/null
>> +++ b/include/linux/fence.h
>> @@ -0,0 +1,347 @@
>> +/*
>> + * Fence mechanism for dma-buf to allow for asynchronous dma access
>> + *
>> + * Copyright (C) 2012 Canonical Ltd
>> + * Copyright (C) 2012 Texas Instruments
>> + *
>> + * Authors:
>> + * Rob Clark <rob.clark@linaro.org>
>> + * 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.
>> + *
>> + * You should have received a copy of the GNU General Public License along with
>> + * this program.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#ifndef __LINUX_FENCE_H
>> +#define __LINUX_FENCE_H
>> +
>> +#include <linux/err.h>
>> +#include <linux/wait.h>
>> +#include <linux/list.h>
>> +#include <linux/bitops.h>
>> +#include <linux/kref.h>
>> +#include <linux/sched.h>
>> +#include <linux/printk.h>
>> +
>> +struct fence;
>> +struct fence_ops;
>> +struct fence_cb;
>> +
>> +/**
>> + * struct fence - software synchronization primitive
>> + * @refcount: refcount for this fence
>> + * @ops: fence_ops associated with this fence
>> + * @cb_list: list of all callbacks to call
>> + * @lock: spin_lock_irqsave used for locking
>> + * @priv: fence specific private data
>> + * @flags: A mask of FENCE_FLAG_* defined below
>> + *
>> + * 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.
>> + *
>> + * FENCE_FLAG_SIGNALED_BIT - fence is already signaled
>> + * FENCE_FLAG_ENABLE_SIGNAL_BIT - enable_signaling might have been called*
>> + * 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 fence_signal was called right
>> + * before this bit was set, it would have been able to set the
>> + * FENCE_FLAG_SIGNALED_BIT, before enable_signaling was called.
>> + * Adding a check for FENCE_FLAG_SIGNALED_BIT after setting
>> + * FENCE_FLAG_ENABLE_SIGNAL_BIT closes this race, and makes sure that
>> + * after fence_signal was called, any enable_signaling call will have either
>> + * been completed, or never called at all.
>> + */
>> +struct fence {
>> +	struct kref refcount;
>> +	const struct fence_ops *ops;
>> +	struct list_head cb_list;
>> +	spinlock_t *lock;
>> +	unsigned context, seqno;
>> +	unsigned long flags;
>> +};
> The documentation above should be updated with the new structure members
> context and seqno.
>
>> +
>> +enum fence_flag_bits {
>> +	FENCE_FLAG_SIGNALED_BIT,
>> +	FENCE_FLAG_ENABLE_SIGNAL_BIT,
>> +	FENCE_FLAG_USER_BITS, /* must always be last member */
>> +};
>> +
>> +typedef void (*fence_func_t)(struct fence *fence, struct fence_cb *cb, void *priv);
>> +
>> +/**
>> + * struct fence_cb - callback for fence_add_callback
>> + * @func: fence_func_t to call
>> + * @priv: value of priv to pass to function
>> + *
>> + * This struct will be initialized by fence_add_callback, additional
>> + * data can be passed along by embedding fence_cb in another struct.
>> + */
>> +struct fence_cb {
>> +	struct list_head node;
>> +	fence_func_t func;
>> +	void *priv;
>> +};
> Documentation should be updated here too.
>
>> +
>> +/**
>> + * struct fence_ops - operations implemented for fence
>> + * @enable_signaling: enable software signaling of fence
>> + * @signaled: [optional] peek whether the fence is signaled
>> + * @release: [optional] called on destruction of fence
>> + *
>> + * Notes on enable_signaling:
>> + * For fence implementations that have the capability for hw->hw
>> + * signaling, they can implement this op to enable the necessary
>> + * irqs, or insert commands into cmdstream, etc.  This is called
>> + * in the first wait() or 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 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 occured that made it impossible to enable
>> + * signaling. True indicates succesful enabling.
>> + *
>> + * Calling fence_signal before enable_signaling is called allows
>> + * for a tiny race window in which enable_signaling is called during,
>> + * before, or after fence_signal. To fight this, it is recommended
>> + * that before enable_signaling returns true an extra reference is
>> + * taken on the fence, to be released when the fence is signaled.
>> + * This will mean fence_signal will still be called twice, but
>> + * the second time will be a noop since it was already signaled.
>> + *
>> + * Notes on release:
>> + * Can be NULL, this function allows additional commands to run on
>> + * destruction of the fence. Can be called from irq context.
>> + * If pointer is set to NULL, kfree will get called instead.
>> + */
>> +
>> +struct fence_ops {
>> +	bool (*enable_signaling)(struct fence *fence);
>> +	bool (*signaled)(struct fence *fence);
>> +	long (*wait)(struct fence *fence, bool intr, signed long);
>> +	void (*release)(struct fence *fence);
>> +};
> Ditto.
>
>> +
>> +/**
>> + * __fence_init - Initialize a custom fence.
>> + * @fence:	[in]	the fence to initialize
>> + * @ops:	[in]	the 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
>> + *
>> + * 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 fence_ops.enable_signaling gets called. This can
>> + * be used for other implementing other types of fence.
>> + *
>> + * context and seqno are used for easy comparison between fences, allowing
>> + * to check which fence is later by simply using fence_later.
>> + */
>> +static inline void
>> +__fence_init(struct fence *fence, const struct fence_ops *ops,
>> +	     spinlock_t *lock, unsigned context, unsigned seqno)
>> +{
>> +	BUG_ON(!ops || !lock || !ops->enable_signaling || !ops->wait);
>> +
>> +	kref_init(&fence->refcount);
>> +	fence->ops = ops;
>> +	INIT_LIST_HEAD(&fence->cb_list);
>> +	fence->lock = lock;
>> +	fence->context = context;
>> +	fence->seqno = seqno;
>> +	fence->flags = 0UL;
>> +}
>> +
>> +/**
>> + * fence_get - increases refcount of the fence
>> + * @fence:	[in]	fence to increase refcount of
>> + */
>> +static inline void fence_get(struct fence *fence)
>> +{
>> +	if (WARN_ON(!fence))
>> +		return;
>> +	kref_get(&fence->refcount);
>> +}
>> +
>> +extern void release_fence(struct kref *kref);
>> +
>> +/**
>> + * fence_put - decreases refcount of the fence
>> + * @fence:	[in]	fence to reduce refcount of
>> + */
>> +static inline void fence_put(struct fence *fence)
>> +{
>> +	if (WARN_ON(!fence))
>> +		return;
>> +	kref_put(&fence->refcount, release_fence);
>> +}
>> +
>> +int fence_signal(struct fence *fence);
>> +int __fence_signal(struct fence *fence);
>> +long fence_default_wait(struct fence *fence, bool intr, signed long);
> In the parameter list the first two parameters are named, and the last
> one isn't. Feels a bit odd...
>
>> +int fence_add_callback(struct fence *fence, struct fence_cb *cb,
>> +		       fence_func_t func, void *priv);
>> +bool fence_remove_callback(struct fence *fence, struct fence_cb *cb);
>> +void fence_enable_sw_signaling(struct fence *fence);
>> +
>> +/**
>> + * fence_is_signaled - Return an indication if the fence is signaled yet.
>> + * @fence:	[in]	the fence to check
>> + *
>> + * Returns true if the fence was already signaled, false if not. Since this
>> + * function doesn't enable signaling, it is not guaranteed to ever return true
>> + * If fence_add_callback, fence_wait or fence_enable_sw_signaling
>> + * haven't been called before.
>> + *
>> + * It's recommended for seqno fences to call fence_signal when the
>> + * operation is complete, it makes it possible to prevent issues from
>> + * wraparound between time of issue and time of use by checking the return
>> + * value of this function before calling hardware-specific wait instructions.
>> + */
>> +static inline bool
>> +fence_is_signaled(struct fence *fence)
>> +{
>> +	if (test_bit(FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>> +		return true;
>> +
>> +	if (fence->ops->signaled && fence->ops->signaled(fence)) {
>> +		fence_signal(fence);
>> +		return true;
>> +	}
>> +
>> +	return false;
>> +}
>> +
>> +/**
>> + * fence_later - return the chronologically later fence
>> + * @f1:	[in]	the first fence from the same context
>> + * @f2:	[in]	the second fence from the same context
>> + *
>> + * Returns NULL if both fences are signaled, otherwise the fence that would be
>> + * signaled last. Both fences must be from the same context, since a seqno is
>> + * not re-used across contexts.
>> + */
>> +static inline struct fence *fence_later(struct fence *f1, struct fence *f2)
>> +{
>> +	bool sig1, sig2;
>> +
>> +	/*
>> +	 * can't check just FENCE_FLAG_SIGNALED_BIT here, it may never have been
>> +	 * set called if enable_signaling wasn't, and enabling that here is
>> +	 * overkill.
>> +	 */
>> +	sig1 = fence_is_signaled(f1);
>> +	sig2 = fence_is_signaled(f2);
>> +
>> +	if (sig1 && sig2)
>> +		return NULL;
>> +
>> +	BUG_ON(f1->context != f2->context);
>> +
>> +	if (sig1 || f2->seqno - f2->seqno <= INT_MAX)
> I guess you meant (f2->seqno - f1->seqno).
>
>> +		return f2;
>> +	else
>> +		return f1;
>> +}
> Regards,
> Francesco
>
Thanks for the review, how does this delta look?

diff --git a/include/linux/fence.h b/include/linux/fence.h
index d9f091d..831ed0a 100644
--- a/include/linux/fence.h
+++ b/include/linux/fence.h
@@ -42,7 +42,10 @@ struct fence_cb;
  * @ops: fence_ops associated with this fence
  * @cb_list: list of all callbacks to call
  * @lock: spin_lock_irqsave used for locking
- * @priv: fence specific private data
+ * @context: execution context this fence belongs to, returned by
+ *           fence_context_alloc()
+ * @seqno: the sequence number of this fence inside the executation context,
+ * can be compared to decide which fence would be signaled later.
  * @flags: A mask of FENCE_FLAG_* defined below
  *
  * the flags member must be manipulated and read using the appropriate
@@ -83,6 +86,7 @@ typedef void (*fence_func_t)(struct fence *fence, struct fence_cb *cb, void *pri
 
 /**
  * struct fence_cb - callback for fence_add_callback
+ * @node: used by fence_add_callback to append this struct to fence::cb_list
  * @func: fence_func_t to call
  * @priv: value of priv to pass to function
  *
@@ -98,8 +102,9 @@ struct fence_cb {
 /**
  * struct fence_ops - operations implemented for fence
  * @enable_signaling: enable software signaling of fence
- * @signaled: [optional] peek whether the fence is signaled
- * @release: [optional] called on destruction of fence
+ * @signaled: [optional] peek whether the fence is signaled, can be null
+ * @wait: custom wait implementation
+ * @release: [optional] called on destruction of fence, can be null
  *
  * Notes on enable_signaling:
  * For fence implementations that have the capability for hw->hw
@@ -124,6 +129,16 @@ struct fence_cb {
  * This will mean fence_signal will still be called twice, but
  * the second time will be a noop since it was already signaled.
  *
+ * Notes on wait:
+ * Must not be NULL, set to fence_default_wait for default implementation.
+ * the fence_default_wait implementation should work for any fence, as long
+ * as enable_signaling works correctly.
+ *
+ * Must return -ERESTARTSYS if the wait is intr = true and the wait was interrupted,
+ * and remaining jiffies if fence has signaled. 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.
+ *
  * Notes on release:
  * Can be NULL, this function allows additional commands to run on
  * destruction of the fence. Can be called from irq context.
@@ -133,7 +148,7 @@ struct fence_cb {
 struct fence_ops {
 	bool (*enable_signaling)(struct fence *fence);
 	bool (*signaled)(struct fence *fence);
-	long (*wait)(struct fence *fence, bool intr, signed long);
+	long (*wait)(struct fence *fence, bool intr, signed long timeout);
 	void (*release)(struct fence *fence);
 };
 
@@ -194,7 +209,7 @@ static inline void fence_put(struct fence *fence)
 
 int fence_signal(struct fence *fence);
 int __fence_signal(struct fence *fence);
-long fence_default_wait(struct fence *fence, bool intr, signed long);
+long fence_default_wait(struct fence *fence, bool intr, signed long timeout);
 int fence_add_callback(struct fence *fence, struct fence_cb *cb,
 		       fence_func_t func, void *priv);
 bool fence_remove_callback(struct fence *fence, struct fence_cb *cb);
@@ -254,7 +269,7 @@ static inline struct fence *fence_later(struct fence *f1, struct fence *f2)
 
 	BUG_ON(f1->context != f2->context);
 
-	if (sig1 || f2->seqno - f2->seqno <= INT_MAX)
+	if (sig1 || f2->seqno - f1->seqno <= INT_MAX)
 		return f2;
 	else
 		return f1;