Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753042AbcDOIDE (ORCPT ); Fri, 15 Apr 2016 04:03:04 -0400 Received: from mail-wm0-f68.google.com ([74.125.82.68]:36577 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751745AbcDOIC7 (ORCPT ); Fri, 15 Apr 2016 04:02:59 -0400 Date: Fri, 15 Apr 2016 10:02:54 +0200 From: Daniel Vetter To: Gustavo Padovan Cc: dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Daniel Stone , Arve =?iso-8859-1?B?SGr4bm5lduVn?= , Riley Andrews , Daniel Vetter , Rob Clark , Greg Hackmann , John Harrison , laurent.pinchart@ideasonboard.com, seanpaul@google.com, marcheu@google.com, m.chehab@samsung.com, Maarten Lankhorst , Gustavo Padovan Subject: Re: [RFC 1/8] dma-buf/fence: add fence_collection fences Message-ID: <20160415080254.GQ2510@phenom.ffwll.local> Mail-Followup-To: Gustavo Padovan , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Daniel Stone , Arve =?iso-8859-1?B?SGr4bm5lduVn?= , Riley Andrews , Rob Clark , Greg Hackmann , John Harrison , laurent.pinchart@ideasonboard.com, seanpaul@google.com, marcheu@google.com, m.chehab@samsung.com, Maarten Lankhorst , Gustavo Padovan References: <1460683781-22535-1-git-send-email-gustavo@padovan.org> <1460683781-22535-2-git-send-email-gustavo@padovan.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1460683781-22535-2-git-send-email-gustavo@padovan.org> X-Operating-System: Linux phenom 4.4.0-1-amd64 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: 9840 Lines: 296 On Thu, Apr 14, 2016 at 06:29:34PM -0700, 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 from DRM drivers. So even if there are many > fences in the sync_file that needs to waited for a commit to happen > drivers would only worry about 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. > > Signed-off-by: Gustavo Padovan > --- > drivers/dma-buf/Makefile | 2 +- > drivers/dma-buf/fence-collection.c | 138 +++++++++++++++++++++++++++++++++++++ > drivers/dma-buf/fence.c | 2 +- > include/linux/fence-collection.h | 56 +++++++++++++++ > include/linux/fence.h | 2 + > 5 files changed, 198 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 43325a1..30b8464 100644 > --- a/drivers/dma-buf/Makefile > +++ b/drivers/dma-buf/Makefile > @@ -1,3 +1,3 @@ > -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 sync_timeline.o sync_debug.o > obj-$(CONFIG_SW_SYNC) += sw_sync.o > diff --git a/drivers/dma-buf/fence-collection.c b/drivers/dma-buf/fence-collection.c > new file mode 100644 > index 0000000..8a4ecb0 > --- /dev/null > +++ b/drivers/dma-buf/fence-collection.c > @@ -0,0 +1,138 @@ > +/* > + * 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 bool fence_collection_enable_signaling(struct fence *fence) > +{ > + struct fence_collection *collection = to_fence_collection(fence); > + > + 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_remove_callback(collection->fences[i].fence, > + &collection->fences[i].cb); > + fence_put(collection->fences[i].fence); > + } > + > + 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; > + > + for (i = 0 ; i < collection->num_fences ; i++) { > + timeout = fence_wait(collection->fences[i].fence, intr); > + if (timeout < 0) > + return timeout; > + } > + > + return timeout; > +} > + > +static 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, > +}; > + > +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); > +} > + > +void fence_collection_add(struct fence_collection *collection, > + struct fence *fence) > +{ > + int n = collection->num_fences; > + > + collection->fences[n].collection = collection; > + collection->fences[n].fence = fence; > + > + if (fence_add_callback(fence, &collection->fences[n].cb, > + collection_check_cb_func)) > + return; > + > + fence_get(fence); > + > + collection->num_fences++; > + atomic_inc(&collection->num_pending_fences); > +} For the interface I think we should not split it into _init and _add - it shouldn't be allowed to change a collection once it's created. So probably cleaner if we add an array of fence pointers to _init. Other nitpick: Adding the callback should (I think) only be done in ->enable_signalling. Finally: Have you looked into stitching together a few unit tests for fence_collection? Fence collections also break the assumption that every fence is on a timeline. fence_later and fence_is_later need to be adjusted. We also need a special collection context to filter these out. This means fence_collection isn't perfectly opaque abstraction. > + > +struct fence_collection *fence_collection_init(int num_fences) > +{ > + struct fence_collection *collection; > + > + collection = kzalloc(offsetof(struct fence_collection, > + fences[num_fences]), GFP_KERNEL); > + if (!collection) > + return NULL; > + > + spin_lock_init(&collection->lock); > + fence_init(&collection->base, &fence_collection_ops, &collection->lock, > + FENCE_NO_CONTEXT, 0); > + > + return collection; > +} > +EXPORT_SYMBOL(fence_collection_init); > + > +void fence_collection_put(struct fence_collection *collection) > +{ > + fence_put(&collection->base); Not sure a specialized _put function is useful, I'd leave it out. > +} > +EXPORT_SYMBOL(fence_collection_put); > 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..a798925 > --- /dev/null > +++ b/include/linux/fence-collection.h > @@ -0,0 +1,56 @@ > +/* > + * 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 { > + struct fence_cb cb; > + struct fence *fence; > + struct fence_collection *collection; > +}; > + > +struct fence_collection { > + struct fence base; > + > + spinlock_t lock; > + struct fence_cb fence_cb; > + atomic_t num_pending_fences; > + int num_fences; > + struct fence_collection_cb fences[]; > +}; > + > +/** > + * 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) > +{ Kerneldoc claims it, but you don't check that the fence is indeed a fence_collection. That's usually done by comparing the ops pointer. > + return container_of(fence, struct fence_collection, base); > +} > + > +struct fence_collection *fence_collection_init(int num_fences); > +void fence_collection_add(struct fence_collection *collection, > + struct fence *fence); > +void fence_collection_put(struct fence_collection *collection); > + > +#endif /* __LINUX_FENCE_COLLECTION_H */ > diff --git a/include/linux/fence.h b/include/linux/fence.h > index 2b17698..02170dd 100644 > --- a/include/linux/fence.h > +++ b/include/linux/fence.h > @@ -30,6 +30,8 @@ > #include > #include > > +#define FENCE_NO_CONTEXT 0 > + > struct fence; > struct fence_ops; > struct fence_cb; > -- > 2.5.5 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch