From patchwork Thu Jun 19 16:12:32 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Maarten Lankhorst X-Patchwork-Id: 4384901 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 8682FBEEAA for ; Thu, 19 Jun 2014 16:12:44 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 8638520395 for ; Thu, 19 Jun 2014 16:12:43 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id 436B22038C for ; Thu, 19 Jun 2014 16:12:42 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id ECD5F6E2FE; Thu, 19 Jun 2014 09:12:40 -0700 (PDT) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from youngberry.canonical.com (youngberry.canonical.com [91.189.89.112]) by gabe.freedesktop.org (Postfix) with ESMTP id A34AD6E2FE for ; Thu, 19 Jun 2014 09:12:39 -0700 (PDT) Received: from 5ed49945.cm-7-5c.dynamic.ziggo.nl ([94.212.153.69] helo=[192.168.1.128]) by youngberry.canonical.com with esmtpsa (TLS1.0:DHE_RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1Wxewz-0000JU-OW; Thu, 19 Jun 2014 16:12:33 +0000 Message-ID: <53A30BF0.1060109@canonical.com> Date: Thu, 19 Jun 2014 18:12:32 +0200 From: Maarten Lankhorst User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 MIME-Version: 1.0 To: Colin Cross , Greg KH , "open list:GENERIC INCLUDE/A..." , thellstrom@vmware.com, lkml , "dri-devel@lists.freedesktop.org" , "linaro-mm-sig@lists.linaro.org" , Rob Clark , thierry.reding@gmail.com, Sumit Semwal , "open list:DMA BUFFER SHARIN..." Subject: Re: [REPOST PATCH 4/8] android: convert sync to fence api, v5 References: <20140618102957.15728.43525.stgit@patser> <20140618103711.15728.97842.stgit@patser> <20140619011556.GE10921@kroah.com> <20140619063727.GL5821@phenom.ffwll.local> In-Reply-To: X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP op 19-06-14 17:22, Colin Cross schreef: > On Wed, Jun 18, 2014 at 11:37 PM, Daniel Vetter wrote: >> On Wed, Jun 18, 2014 at 06:15:56PM -0700, Greg KH wrote: >>> On Wed, Jun 18, 2014 at 12:37:11PM +0200, Maarten Lankhorst wrote: >>>> Just to show it's easy. >>>> >>>> Android syncpoints can be mapped to a timeline. This removes the need >>>> to maintain a separate api for synchronization. I've left the android >>>> trace events in place, but the core fence events should already be >>>> sufficient for debugging. >>>> >>>> v2: >>>> - Call fence_remove_callback in sync_fence_free if not all fences have fired. >>>> v3: >>>> - Merge Colin Cross' bugfixes, and the android fence merge optimization. >>>> v4: >>>> - Merge with the upstream fixes. >>>> v5: >>>> - Fix small style issues pointed out by Thomas Hellstrom. >>>> >>>> Signed-off-by: Maarten Lankhorst >>>> Acked-by: John Stultz >>>> --- >>>> drivers/staging/android/Kconfig | 1 >>>> drivers/staging/android/Makefile | 2 >>>> drivers/staging/android/sw_sync.c | 6 >>>> drivers/staging/android/sync.c | 913 +++++++++++----------------------- >>>> drivers/staging/android/sync.h | 79 ++- >>>> drivers/staging/android/sync_debug.c | 247 +++++++++ >>>> drivers/staging/android/trace/sync.h | 12 >>>> 7 files changed, 609 insertions(+), 651 deletions(-) >>>> create mode 100644 drivers/staging/android/sync_debug.c >>> With these changes, can we pull the android sync logic out of >>> drivers/staging/ now? >> Afaik the google guys never really looked at this and acked it. So I'm not >> sure whether they'll follow along. The other issue I have as the >> maintainer of gfx driver is that I don't want to implement support for two >> different sync object primitives (once for dma-buf and once for android >> syncpts), and my impression thus far has been that even with this we're >> not there. > We have tested these patches to use dma fences to back the android > sync driver and not found any major issues. However, my understanding > is that dma fences are designed for implicit sync, and explicit sync > through the android sync driver is bolted on the side to share code. > Android is not moving away from explicit sync, but we do wrap all of > our userspace sync accesses through libsync > (https://android.googlesource.com/platform/system/core/+/master/libsync/sync.c, > ignore the sw_sync parts), so if the kernel supported a slightly > different userspace explicit sync interface we could adapt to it > fairly easily. All we require is that individual kernel drivers need > to be able to accept work alongisde an fd to wait on, and to return an > fd that will signal when the work is done, and that userspace has some > way to merge two of those fds, wait on an fd, and get some debugging > info from an fd. However, this patch set doesn't do that, it has no > way to export a dma fence as an fd except through the android sync > driver, so it is not yet ready to fully replace android sync. > Dma fences can be exported as android fences, just didn't see a need for it yet. :-) To wait on all implicit fences attached to a dma-buf one could simply poll the dma-buf directly, or use something like a android userspace fence. sync_fence_create takes a sync_pt as function argument, but I kept that to keep source code compatibility, not because it uses any sync_pt functions. Here's a patch to create a userspace fd for dma-fence instead of a android fence, applied on top of "android: convert sync to fence api". diff --git a/drivers/staging/android/sw_sync.c b/drivers/staging/android/sw_sync.c index a76db3ff87cb..afc3c63b0438 100644 --- a/drivers/staging/android/sw_sync.c +++ b/drivers/staging/android/sw_sync.c @@ -184,7 +184,7 @@ static long sw_sync_ioctl_create_fence(struct sw_sync_timeline *obj, } data.name[sizeof(data.name) - 1] = '\0'; - fence = sync_fence_create(data.name, pt); + fence = sync_fence_create(data.name, &pt->base); if (fence == NULL) { sync_pt_free(pt); err = -ENOMEM; diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c index 70b09b5001ba..c89a6f954e41 100644 --- a/drivers/staging/android/sync.c +++ b/drivers/staging/android/sync.c @@ -188,7 +188,7 @@ static void fence_check_cb_func(struct fence *f, struct fence_cb *cb) } /* TODO: implement a create which takes more that one sync_pt */ -struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt) +struct sync_fence *sync_fence_create(const char *name, struct fence *pt) { struct sync_fence *fence; @@ -199,10 +199,10 @@ struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt) fence->num_fences = 1; atomic_set(&fence->status, 1); - fence_get(&pt->base); - fence->cbs[0].sync_pt = &pt->base; + fence_get(pt); + fence->cbs[0].sync_pt = pt; fence->cbs[0].fence = fence; - if (fence_add_callback(&pt->base, &fence->cbs[0].cb, + if (fence_add_callback(pt, &fence->cbs[0].cb, fence_check_cb_func)) atomic_dec(&fence->status); diff --git a/drivers/staging/android/sync.h b/drivers/staging/android/sync.h index 66b0f431f63e..3020751f3c5d 100644 --- a/drivers/staging/android/sync.h +++ b/drivers/staging/android/sync.h @@ -252,7 +252,7 @@ void sync_pt_free(struct sync_pt *pt); * Creates a fence containg @pt. Once this is called, the fence takes * ownership of @pt. */ -struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt); +struct sync_fence *sync_fence_create(const char *name, struct fence *fence); /* * API for sync_fence consumers