From patchwork Wed Jan 23 14:56:49 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Maarten Lankhorst X-Patchwork-Id: 14246 Return-Path: X-Original-To: patchwork@peony.canonical.com Delivered-To: patchwork@peony.canonical.com Received: from fiordland.canonical.com (fiordland.canonical.com [91.189.94.145]) by peony.canonical.com (Postfix) with ESMTP id 30CA223E33 for ; Wed, 23 Jan 2013 14:57:06 +0000 (UTC) Received: from mail-vb0-f52.google.com (mail-vb0-f52.google.com [209.85.212.52]) by fiordland.canonical.com (Postfix) with ESMTP id BE00BA18BE9 for ; Wed, 23 Jan 2013 14:57:05 +0000 (UTC) Received: by mail-vb0-f52.google.com with SMTP id fa15so4715702vbb.39 for ; Wed, 23 Jan 2013 06:57:05 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-received:x-forwarded-to:x-forwarded-for:delivered-to:x-received :received-spf:message-id:date:from:user-agent:mime-version:to :references:in-reply-to:cc:subject:x-beenthere:x-mailman-version :precedence:list-id:list-unsubscribe:list-archive:list-post :list-help:list-subscribe:content-type:content-transfer-encoding :sender:errors-to:x-gm-message-state; bh=JFhzCXS4dAoViIOUDUUF5AgzqpVYh7TRqfGtrkyWWFc=; b=RzW/CMqpt6oaDugYRziga2XJ84Dl1Emk8L/CfdN2Jy3qC5i0A3gtznzz1EJMBAQse/ /fWaDLi4mRc3nnHKqIXsM1X5aeUQdA8Y+pWmLq6VwCuzx67lBJOvy46Jt5Zts8WbeyXo OkWLM1iaZ/EXEuQi4H85jESTlpwH31ilBwZ6goimn6J/BssEjc2Y2S/SRElohaagWsoL m6AHQnYiwY2dbRwjMekWqNAb14ZTDtDH/OBE4+TyyhlJrr2ehXU9xYodJQHE/OXN8RIJ zUjodJ00IAqOQt4/2zVoqreMZQnLVu1FRaxPEV83k5lrt06seOa7DqkDx5vlwGAHqAGY vt3Q== X-Received: by 10.220.154.199 with SMTP id p7mr1559131vcw.48.1358953025206; Wed, 23 Jan 2013 06:57:05 -0800 (PST) X-Forwarded-To: linaro-patchwork@canonical.com X-Forwarded-For: patch@linaro.org linaro-patchwork@canonical.com Delivered-To: patches@linaro.org Received: by 10.58.145.101 with SMTP id st5csp14560veb; Wed, 23 Jan 2013 06:57:03 -0800 (PST) X-Received: by 10.14.216.70 with SMTP id f46mr4980059eep.12.1358953022698; Wed, 23 Jan 2013 06:57:02 -0800 (PST) Received: from mombin.canonical.com (mombin.canonical.com. [91.189.95.16]) by mx.google.com with ESMTP id g49si36117706eep.198.2013.01.23.06.56.57; Wed, 23 Jan 2013 06:57:02 -0800 (PST) Received-SPF: neutral (google.com: 91.189.95.16 is neither permitted nor denied by best guess record for domain of linaro-mm-sig-bounces@lists.linaro.org) client-ip=91.189.95.16; Authentication-Results: mx.google.com; spf=neutral (google.com: 91.189.95.16 is neither permitted nor denied by best guess record for domain of linaro-mm-sig-bounces@lists.linaro.org) smtp.mail=linaro-mm-sig-bounces@lists.linaro.org Received: from localhost ([127.0.0.1] helo=mombin.canonical.com) by mombin.canonical.com with esmtp (Exim 4.71) (envelope-from ) id 1Ty1kz-000589-V9; Wed, 23 Jan 2013 14:56:54 +0000 Received: from youngberry.canonical.com ([91.189.89.112]) by mombin.canonical.com with esmtp (Exim 4.71) (envelope-from ) id 1Ty1ky-000584-J1 for linaro-mm-sig@lists.linaro.org; Wed, 23 Jan 2013 14:56:52 +0000 Received: from 5ed48cef.cm-7-5c.dynamic.ziggo.nl ([94.212.140.239] helo=[192.168.1.128]) by youngberry.canonical.com with esmtpsa (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1Ty1kw-0002KV-De; Wed, 23 Jan 2013 14:56:50 +0000 Message-ID: <50FFFA31.6000101@canonical.com> Date: Wed, 23 Jan 2013 15:56:49 +0100 From: Maarten Lankhorst User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130106 Thunderbird/17.0.2 MIME-Version: 1.0 To: Francesco Lavra References: <1358253244-11453-1-git-send-email-maarten.lankhorst@canonical.com> <1358253244-11453-5-git-send-email-maarten.lankhorst@canonical.com> <50FEAC87.7090702@gmail.com> In-Reply-To: <50FEAC87.7090702@gmail.com> Cc: linaro-mm-sig@lists.linaro.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-media@vger.kernel.org Subject: Re: [Linaro-mm-sig] [PATCH 4/7] fence: dma-buf cross-device synchronization (v11) X-BeenThere: linaro-mm-sig@lists.linaro.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: "Unified memory management interest group." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linaro-mm-sig-bounces@lists.linaro.org Errors-To: linaro-mm-sig-bounces@lists.linaro.org X-Gm-Message-State: ALoCoQl72XFyO+LTI9oH57KxDi/72DE1PnPt5c1jIWIaglNpySRfbagQfR+6EBnclumGJxLMd59L 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 >> + * Maarten Lankhorst >> + * >> + * 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 . >> + */ >> + >> +#ifndef __LINUX_FENCE_H >> +#define __LINUX_FENCE_H >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +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;