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@xxxxxxxxxx> >> + * Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxx> >> + * >> + * 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; -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
