Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752297AbcDZOlK (ORCPT ); Tue, 26 Apr 2016 10:41:10 -0400 Received: from mail-wm0-f66.google.com ([74.125.82.66]:36198 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750843AbcDZOlG (ORCPT ); Tue, 26 Apr 2016 10:41:06 -0400 Date: Tue, 26 Apr 2016 16:41:02 +0200 From: Daniel Vetter To: Gustavo Padovan Cc: Greg Kroah-Hartman , linux-kernel@vger.kernel.org, devel@driverdev.osuosl.org, dri-devel@lists.freedesktop.org, Daniel Stone , Arve =?iso-8859-1?B?SGr4bm5lduVn?= , Riley Andrews , Daniel Vetter , Rob Clark , Greg Hackmann , John Harrison , Maarten Lankhorst , Sumit Semwal , Gustavo Padovan Subject: Re: [RFC v2 1/8] dma-buf/fence: add fence_collection fences Message-ID: <20160426144102.GX8291@phenom.ffwll.local> Mail-Followup-To: Gustavo Padovan , Greg Kroah-Hartman , linux-kernel@vger.kernel.org, devel@driverdev.osuosl.org, dri-devel@lists.freedesktop.org, Daniel Stone , Arve =?iso-8859-1?B?SGr4bm5lduVn?= , Riley Andrews , Rob Clark , Greg Hackmann , John Harrison , Maarten Lankhorst , Sumit Semwal , Gustavo Padovan References: <1461623608-29538-1-git-send-email-gustavo@padovan.org> <1461623608-29538-2-git-send-email-gustavo@padovan.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1461623608-29538-2-git-send-email-gustavo@padovan.org> X-Operating-System: Linux phenom 4.6.0-rc5+ User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 12015 Lines: 350 On Mon, Apr 25, 2016 at 07:33:21PM -0300, Gustavo Padovan wrote: > From: Gustavo Padovan > > struct fence_collection inherits from struct fence and carries a > collection of fences that needs to be waited together. > > It is useful to translate a sync_file to a fence to remove the complexity > of dealing with sync_files on DRM drivers. So even if there are many > fences in the sync_file that needs to waited for a commit to happen, > they all get added to the fence_collection and passed for DRM use as > a standard struct fence. > > That means that no changes needed to any driver besides supporting fences. > > fence_collection's fence doesn't belong to any timeline context, so > fence_is_later() and fence_later() are not meant to be called with > fence_collections fences. > > v2: Comments by Daniel Vetter: > - merge fence_collection_init() and fence_collection_add() > - only add callbacks at ->enable_signalling() > - remove fence_collection_put() > - check for type on to_fence_collection() > - adjust fence_is_later() and fence_later() to WARN_ON() if they > are used with collection fences. > > Signed-off-by: Gustavo Padovan FENCE_NO_CONTEXT semantics needs an ack from amdgpu maintainers. I'm not entirely sure they might not hit the new WARN_ON by accident now. Please cc Alex Deucher & Christian K?nig. -Daniel > --- > drivers/dma-buf/Makefile | 2 +- > drivers/dma-buf/fence-collection.c | 159 +++++++++++++++++++++++++++++++++++++ > drivers/dma-buf/fence.c | 2 +- > include/linux/fence-collection.h | 73 +++++++++++++++++ > include/linux/fence.h | 9 +++ > 5 files changed, 243 insertions(+), 2 deletions(-) > create mode 100644 drivers/dma-buf/fence-collection.c > create mode 100644 include/linux/fence-collection.h > > diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile > index 4a424ec..52f818f 100644 > --- a/drivers/dma-buf/Makefile > +++ b/drivers/dma-buf/Makefile > @@ -1,2 +1,2 @@ > -obj-y := dma-buf.o fence.o reservation.o seqno-fence.o > +obj-y := dma-buf.o fence.o reservation.o seqno-fence.o fence-collection.o > obj-$(CONFIG_SYNC_FILE) += sync_file.o > diff --git a/drivers/dma-buf/fence-collection.c b/drivers/dma-buf/fence-collection.c > new file mode 100644 > index 0000000..88872e5 > --- /dev/null > +++ b/drivers/dma-buf/fence-collection.c > @@ -0,0 +1,159 @@ > +/* > + * fence-collection: aggregate fences to be waited together > + * > + * Copyright (C) 2016 Collabora Ltd > + * Authors: > + * Gustavo Padovan > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License version 2 as published by > + * the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + */ > + > +#include > +#include > +#include > + > +static const char *fence_collection_get_driver_name(struct fence *fence) > +{ > + struct fence_collection *collection = to_fence_collection(fence); > + struct fence *f = collection->fences[0].fence; > + > + return f->ops->get_driver_name(fence); > +} > + > +static const char *fence_collection_get_timeline_name(struct fence *fence) > +{ > + return "no context"; > +} > + > +static void collection_check_cb_func(struct fence *fence, struct fence_cb *cb) > +{ > + struct fence_collection_cb *f_cb; > + struct fence_collection *collection; > + > + f_cb = container_of(cb, struct fence_collection_cb, cb); > + collection = f_cb->collection; > + > + if (atomic_dec_and_test(&collection->num_pending_fences)) > + fence_signal(&collection->base); > +} > + > +static bool fence_collection_enable_signaling(struct fence *fence) > +{ > + struct fence_collection *collection = to_fence_collection(fence); > + int i; > + > + for (i = 0 ; i < collection->num_fences ; i++) { > + if (fence_add_callback(collection->fences[i].fence, > + &collection->fences[i].cb, > + collection_check_cb_func)) { > + atomic_dec(&collection->num_pending_fences); > + return false; > + } > + } > + > + return !!atomic_read(&collection->num_pending_fences); > +} > + > +static bool fence_collection_signaled(struct fence *fence) > +{ > + struct fence_collection *collection = to_fence_collection(fence); > + > + return (atomic_read(&collection->num_pending_fences) == 0); > +} > + > +static void fence_collection_release(struct fence *fence) > +{ > + struct fence_collection *collection = to_fence_collection(fence); > + int i; > + > + for (i = 0 ; i < collection->num_fences ; i++) > + fence_put(collection->fences[i].fence); > + > + kfree(collection->fences); > + fence_free(fence); > +} > + > +static signed long fence_collection_wait(struct fence *fence, bool intr, > + signed long timeout) > +{ > + struct fence_collection *collection = to_fence_collection(fence); > + int i; > + > + if (test_bit(FENCE_FLAG_SIGNALED_BIT, &fence->flags)) > + return timeout; > + > + fence_enable_sw_signaling(fence); > + > + for (i = 0 ; i < collection->num_fences ; i++) { > + if (test_bit(FENCE_FLAG_SIGNALED_BIT, &fence->flags)) > + return timeout; > + > + timeout = fence_wait(collection->fences[i].fence, intr); > + if (timeout < 0) > + return timeout; > + } > + > + return timeout; > +} > + > +const struct fence_ops fence_collection_ops = { > + .get_driver_name = fence_collection_get_driver_name, > + .get_timeline_name = fence_collection_get_timeline_name, > + .enable_signaling = fence_collection_enable_signaling, > + .signaled = fence_collection_signaled, > + .wait = fence_collection_wait, > + .release = fence_collection_release, > +}; > + > +/** > + * fence_collection_create - Create a custom fence collection > + * is signaled > + * @num_fences: [in] number of fences to add in the collection > + * @cb: [in] cb array containing the fences for the collection > + * > + * Allocate a fence_collection and initialize the base fence with fence_init() > + * and FENCE_NO_CONTEXT. It also inits all fence_collections specific fields > + * and return the created fence_collection. In case of error it returns NULL. > + * > + * The caller should allocte the fence_collection_cb array with num_fences size > + * and fill the fence fields with all the fences it wants to add to the > + * collection. > + * > + * Collections have no context, thus fence_later() and fence_is_later() shall > + * not be used with fence_collection. > + * > + * fence_put() on the base fence should be used to release the collection. > + */ > +struct fence_collection *fence_collection_create(int num_fences, > + struct fence_collection_cb *cb) > +{ > + struct fence_collection *collection; > + int i; > + > + collection = kzalloc(sizeof(*collection), GFP_KERNEL); > + if (!collection) > + return NULL; > + > + spin_lock_init(&collection->lock); > + fence_init(&collection->base, &fence_collection_ops, &collection->lock, > + FENCE_NO_CONTEXT, 0); > + > + collection->fences = cb; > + collection->num_fences = num_fences; > + atomic_set(&collection->num_pending_fences, num_fences); > + > + for (i = 0 ; i < collection->num_fences ; i++) { > + fence_get(collection->fences[i].fence); > + collection->fences[i].collection = collection; > + } > + > + return collection; > +} > +EXPORT_SYMBOL(fence_collection_create); > diff --git a/drivers/dma-buf/fence.c b/drivers/dma-buf/fence.c > index 7b05dbe..486e95c 100644 > --- a/drivers/dma-buf/fence.c > +++ b/drivers/dma-buf/fence.c > @@ -35,7 +35,7 @@ EXPORT_TRACEPOINT_SYMBOL(fence_emit); > * context or not. One device can have multiple separate contexts, > * and they're used if some engine can run independently of another. > */ > -static atomic_t fence_context_counter = ATOMIC_INIT(0); > +static atomic_t fence_context_counter = ATOMIC_INIT(1); > > /** > * fence_context_alloc - allocate an array of fence contexts > diff --git a/include/linux/fence-collection.h b/include/linux/fence-collection.h > new file mode 100644 > index 0000000..4087622 > --- /dev/null > +++ b/include/linux/fence-collection.h > @@ -0,0 +1,73 @@ > +/* > + * fence-collection: aggregates fence to be waited together > + * > + * Copyright (C) 2016 Collabora Ltd > + * Authors: > + * Gustavo Padovan > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License version 2 as published by > + * the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + */ > + > +#ifndef __LINUX_FENCE_COLLECTION_H > +#define __LINUX_FENCE_COLLECTION_H > + > +#include > + > +/** > + * struct fence_collection_cb - callback for fence_add_callback > + * @cb: the base fence_cb object > + * @fence: fence we are storing in the collection > + * @collection: collection the fence belongs to > + * > + * This struct is at the same time the storage for the fences in the > + * collection and the struct we pass to fence_add_callback() > + */ > +struct fence_collection_cb { > + struct fence_cb cb; > + struct fence *fence; > + struct fence_collection *collection; > +}; > + > +/** > + * struct fence_collection - fence to represent a collection of fences > + * @base: fence base class > + * @lock: spinlock for fence handling > + * @num_pending_fences: fences in the collection not signaled yet > + * @num_fences: number of fences in the collection > + * @fences: ponteir to fence_collection_cb with data about the fences > + */ > +struct fence_collection { > + struct fence base; > + > + spinlock_t lock; > + atomic_t num_pending_fences; > + int num_fences; > + struct fence_collection_cb *fences; > +}; > + > +extern const struct fence_ops fence_collection_ops; > + > +/** > + * to_fence_collection - cast a fence to a fence_collection > + * @fence: fence to cast to a fence_collection > + * > + * Returns NULL if the fence is not a fence_collection, > + * or the fence_collection otherwise. > + */ > +static inline struct fence_collection *to_fence_collection(struct fence *fence) > +{ > + if (fence->ops != &fence_collection_ops) > + return NULL; > + return container_of(fence, struct fence_collection, base); > +} > + > +struct fence_collection *fence_collection_create(int num_fences, > + struct fence_collection_cb *cb); > +#endif /* __LINUX_FENCE_COLLECTION_H */ > diff --git a/include/linux/fence.h b/include/linux/fence.h > index 5aa95eb..184ce79 100644 > --- a/include/linux/fence.h > +++ b/include/linux/fence.h > @@ -30,6 +30,9 @@ > #include > #include > > +/* context number for fences without context, e.g: fence_collection */ > +#define FENCE_NO_CONTEXT 0 > + > struct fence; > struct fence_ops; > struct fence_cb; > @@ -292,6 +295,9 @@ static inline bool fence_is_later(struct fence *f1, struct fence *f2) > if (WARN_ON(f1->context != f2->context)) > return false; > > + if (WARN_ON(f1->context == FENCE_NO_CONTEXT)) > + return false; > + > return (int)(f1->seqno - f2->seqno) > 0; > } > > @@ -309,6 +315,9 @@ static inline struct fence *fence_later(struct fence *f1, struct fence *f2) > if (WARN_ON(f1->context != f2->context)) > return NULL; > > + if (WARN_ON(f1->context == FENCE_NO_CONTEXT)) > + return NULL; > + > /* > * can't check just FENCE_FLAG_SIGNALED_BIT here, it may never have been > * set if enable_signaling wasn't called, and enabling that here is > -- > 2.5.5 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch