From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752251AbcDOS2Q (ORCPT ); Fri, 15 Apr 2016 14:28:16 -0400 Received: from mail-pa0-f45.google.com ([209.85.220.45]:33150 "EHLO mail-pa0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750873AbcDOS1x convert rfc822-to-8bit (ORCPT ); Fri, 15 Apr 2016 14:27:53 -0400 Date: Fri, 15 Apr 2016 11:27:50 -0700 From: Gustavo Padovan To: Christian =?iso-8859-1?Q?K=F6nig?= Cc: 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 Subject: Re: [RFC 1/8] dma-buf/fence: add fence_collection fences Message-ID: <20160415182750.GA23954@joana> Mail-Followup-To: Gustavo Padovan , Christian =?iso-8859-1?Q?K=F6nig?= , 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> <20160415080254.GQ2510@phenom.ffwll.local> <5710AE61.9040308@amd.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8BIT In-Reply-To: <5710AE61.9040308@amd.com> 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 2016-04-15 Christian König : > Am 15.04.2016 um 10:02 schrieb Daniel Vetter: > >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); > >>+} > > I would rather return some constant name here instead of relying that the > collection already has a fence added and that all fences are from the same > driver. If we merge _init and _add this will not be a problem anymore and we can return the actual driver name. > > >>+ > >>+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. > > Amdgpu also has an implementation for a fence collection which uses a a > hashtable to keep the fences grouped by context (e.g. only the latest fence > is keept for each context). See amdgpu_sync.c for reference. > > We should either make the collection similar in a way that you can add as > many fences as you want (like the amdgpu implementation) or make it static > and only add a fixed number of fences right from the beginning. > > I can certainly see use cases for both, but if you want to stick with a > static approach you should probably call the new object fence_array instead > of fence_collection and do as Daniel suggested. Maybe we can go for something in between. Have fence_collection_init() need at least two fences to create the fence_collection. Then fence_collection_add() would add more dinamically. > > >Other nitpick: Adding the callback should (I think) only be done in > >->enable_signalling. > > Yeah, I was about to complain on that as well. > > Enabling signaling can have a huge overhead for some fence implementations. > So it should only be used when needed Right, that makes sense. > > > > >Finally: Have you looked into stitching together a few unit tests for > >fence_collection? Not yet. It is something I plan to work soon. > > > >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. I'll fix this. > >>+ > >>+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. I've added this to let the user only deal with fence_collection, but I'm fine leaving it out too. Gustavo