From patchwork Thu Nov 7 21:11:19 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Rom Lemarchand X-Patchwork-Id: 3154531 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 206F19F3C4 for ; Thu, 7 Nov 2013 21:11:36 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id BC0912028D for ; Thu, 7 Nov 2013 21:11:34 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id 1A06A20123 for ; Thu, 7 Nov 2013 21:11:33 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 930AEF0BCA; Thu, 7 Nov 2013 13:11:24 -0800 (PST) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-vb0-f44.google.com (mail-vb0-f44.google.com [209.85.212.44]) by gabe.freedesktop.org (Postfix) with ESMTP id EEB00F0BF1 for ; Thu, 7 Nov 2013 13:11:20 -0800 (PST) Received: by mail-vb0-f44.google.com with SMTP id 11so789259vbe.31 for ; Thu, 07 Nov 2013 13:11:20 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=9wMM5+zl/h6MWt3Y5ofB1xzHPKLR5XCubXwHwrUZUvk=; b=PP19hGlvlhOBIOlO7odemRR2WTS5fUStaMh6cwlzFsw3GI/sG6dDL6Zl46p5BQ4TGT QUgfd+JV7djgN0AqUUawj9z7wtjltb6ZMoMweE0UqgEHYoqentUHsrwe3JjxxgxbzBNS e1veR3UOYJeucrsIcXYXjQslHQ/Ih16rKFoSjuIajiZs43SOcC/hpyqFelEbX3PN0Qii k6gsBob0LOAtU+dqmrG2D1T+mWDo6ZJu4k/34QOwNHmP9BGhjrxFRCoMkUf7NpAre1R9 +X5JVPGlzitGbbl1F7sHOD2w+egnVSUyDaxYmRbixWlYT6u/2EqLRHSjGriGunGznl10 0BBA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=9wMM5+zl/h6MWt3Y5ofB1xzHPKLR5XCubXwHwrUZUvk=; b=HDORODpyACAiD2uu9SE3HfkSIoyZLKBeVCBkbq/ylETqfUcHARk77/pFtyiLL7oluY NKEWbGbMUuWEdof4ki+2Ru8S/tabFHaDFcM3qV4GRU5KT6712jL1hu/L9MgzGVM2V9Vz Yc49jvH2tC01/auxXAN67Zu0zMjSSotQ1m60w3n0sLjRQjZy5XvPyKJZ9Joh6XxkT3Ew xPhADmEw2IL/uzS6EgRppzvE3kZak6dmWk5zSQFGwn8DzrOLSd3j1TzGB9D7QP2izhOb P7fJIdtyCeoV7J6ItIhdYXBLb2wq2fyHTLkYYdlt+uW1WMDMQ13EvXhiR/TpnzJwC8K3 y1IA== X-Gm-Message-State: ALoCoQm6C9p0oez/ZrMTavqu8fE0Kw28S6mRYf/4iR+6lx4oNXxnVzi6e5YIA1GL6DP/DXeFV1bgIxK3bU2Pyb1W0Fapwh/jRAtYHjSQ4juCDhuRbRxDVhJOqrP2ywkn+lTS3uGtHOI16lvS6HDe0nwsW2I3pDeOAcNEgVxAS/EbWhpZEzTxKPCyJcOuv5n17OHPlUke94FwAHQpUrX1IJSsrnwB+YTW3Q== MIME-Version: 1.0 X-Received: by 10.52.32.66 with SMTP id g2mr7204757vdi.14.1383858680084; Thu, 07 Nov 2013 13:11:20 -0800 (PST) Received: by 10.52.106.230 with HTTP; Thu, 7 Nov 2013 13:11:19 -0800 (PST) In-Reply-To: <5277777E.5060904@canonical.com> References: <524BCCD0.90002@canonical.com> <52556ABE.2090201@canonical.com> <52690EEC.5000501@canonical.com> <5270F8D7.4040406@canonical.com> <5277777E.5060904@canonical.com> Date: Thu, 7 Nov 2013 13:11:19 -0800 Message-ID: Subject: Re: [Linaro-mm-sig] thoughts of looking at android fences From: Rom Lemarchand To: Maarten Lankhorst Cc: "linaro-mm-sig@lists.linaro.org" , Android Kernel Team , "dri-devel@lists.freedesktop.org" , Colin Cross X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces@lists.freedesktop.org Errors-To: dri-devel-bounces@lists.freedesktop.org X-Spam-Status: No, score=-4.1 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, HTML_MESSAGE, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, T_DKIM_INVALID, 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 Hi Maarten, I tested your changes and needed the attached patch: behavior now seems equivalent as android sync. I haven't tested performance. The issue resolved by this patch happens when i_b < b->num_fences and i_a >= a->num_fences (or vice versa). Then, pt_a is invalid and so dereferencing pt_a->context causes a crash. On Mon, Nov 4, 2013 at 2:31 AM, Maarten Lankhorst < maarten.lankhorst@canonical.com> wrote: > op 02-11-13 22:36, Colin Cross schreef: > > On Wed, Oct 30, 2013 at 5:17 AM, Maarten Lankhorst > > wrote: > >> op 24-10-13 14:13, Maarten Lankhorst schreef: > >>> So I actually tried to implement it now. I killed all the deprecated > members and assumed a linear timeline. > >>> This means that syncpoints can only be added at the end, not in > between. In particular it means sw_sync > >>> might be slightly broken. > >>> > >>> I only tested it with a simple program I wrote called ufence.c, it's > in drivers/staging/android/ufence.c in the following tree: > >>> > >>> http://cgit.freedesktop.org/~mlankhorst/linux > >>> > >>> the "rfc: convert android to fence api" has all the changes from my > dma-fence proposal to what android would need, > >>> it also converts the userspace fence api to use the dma-fence api. > >>> > >>> sync_pt is implemented as fence too. This meant not having to convert > all of android right away, though I did make some changes. > >>> I killed the deprecated members and made all the fence calls forward > to the sync_timeline_ops. dup and compare are no longer used. > >>> > >>> I haven't given this a spin on a full android kernel, only with the > components that are in mainline kernel under staging and my dumb test > program. > >>> > >>> ~Maarten > >>> > >>> PS: The nomenclature is very confusing. I want to rename dma-fence to > syncpoint, but I want some feedback from the android devs first. :) > >>> > >> Come on, any feedback? I want to move the discussion forward. > >> > >> ~Maarten > > I experimented with it a little on a device that uses sync and came > > across a few bugs: > > 1. sync_timeline_signal needs to call __fence_signal on all signaled > > points on the timeline, not just the first > > 2. fence_add_callback doesn't always initialize cb.node > > 3. sync_fence_wait should take ms > > 4. sync_print_pt status printing was incorrect > > 5. there is a deadlock: > > sync_print_obj takes obj->child_list_lock > > sync_print_pt > > fence_is_signaled > > fence_signal takes fence->lock == obj->child_list_lock > > 6. freeing a timeline before all the fences holding points on that > > timeline have timed out results in a crash > > > > With the attached patch to fix these issues, our libsync and sync_test > > give the same results as with our sync code. I haven't tested against > > the full Android framework yet. > > > > The compare op and timeline ordering is critical to the efficiency of > > sync points on Android. The compare op is used when merging fences to > > drop all but the latest point on the same timeline. This is necessary > > for example when the same buffer is submitted to the display on > > multiple frames, like when there is a live wallpaper in the background > > updating at 60 fps and a static screen of widgets on top of it. The > > static widget buffer is submitted on every frame, returning a new > > fence each time. The compositor merges the new fence with the fence > > for the previous buffer, and because they are on the same timeline it > > merges down to a single point. I experimented with disabling the > > merge optimization on a Nexus 10, and found that leaving the screen on > > running a live wallpaper eventually resulted in 100k outstanding sync > > points. > > Well, here I did the same for dma-fence, can you take a look? > > --- > > diff --git a/drivers/staging/android/sync.c > b/drivers/staging/android/sync.c > index 2c7fd3f2ab23..d1d89f1f8553 100644 > --- a/drivers/staging/android/sync.c > +++ b/drivers/staging/android/sync.c > @@ -232,39 +232,62 @@ void sync_fence_install(struct sync_fence *fence, > int fd) > } > EXPORT_SYMBOL(sync_fence_install); > > +static void sync_fence_add_pt(struct sync_fence *fence, int *i, struct > fence *pt) { > + fence->cbs[*i].sync_pt = pt; > + fence->cbs[*i].fence = fence; > + > + if (!fence_add_callback(pt, &fence->cbs[*i].cb, > fence_check_cb_func)) { > + fence_get(pt); > + (*i)++; > + } > +} > + > struct sync_fence *sync_fence_merge(const char *name, > struct sync_fence *a, struct > sync_fence *b) > { > int num_fences = a->num_fences + b->num_fences; > struct sync_fence *fence; > - int i; > + int i, i_a, i_b; > > fence = sync_fence_alloc(offsetof(struct sync_fence, > cbs[num_fences]), name); > if (fence == NULL) > return NULL; > > - fence->num_fences = num_fences; > atomic_set(&fence->status, num_fences); > > - for (i = 0; i < a->num_fences; ++i) { > - struct fence *pt = a->cbs[i].sync_pt; > - > - fence_get(pt); > - fence->cbs[i].sync_pt = pt; > - fence->cbs[i].fence = fence; > - if (fence_add_callback(pt, &fence->cbs[i].cb, > fence_check_cb_func)) > - atomic_dec(&fence->status); > + /* > + * Assume sync_fence a and b are both ordered and have no > + * duplicates with the same context. > + * > + * If a sync_fence can only be created with sync_fence_merge > + * and sync_fence_create, this is a reasonable assumption. > + */ > + for (i = i_a = i_b = 0; i_a < a->num_fences || i_b < > b->num_fences; ) { > + struct fence *pt_a = i_a < a->num_fences ? > a->cbs[i_a].sync_pt : NULL; > + struct fence *pt_b = i_b < b->num_fences ? > b->cbs[i_b].sync_pt : NULL; > + > + if (!pt_b || pt_a->context < pt_b->context) { > + sync_fence_add_pt(fence, &i, pt_a); > + > + i_a++; > + } else if (!pt_a || pt_a->context > pt_b->context) { > + sync_fence_add_pt(fence, &i, pt_b); > + > + i_b++; > + } else { > + if (pt_a->seqno - pt_b->seqno <= INT_MAX) > + sync_fence_add_pt(fence, &i, pt_a); > + else > + sync_fence_add_pt(fence, &i, pt_b); > + > + i_a++; > + i_b++; > + } > } > > - for (i = 0; i < b->num_fences; ++i) { > - struct fence *pt = b->cbs[i].sync_pt; > - > - fence_get(pt); > - fence->cbs[a->num_fences + i].sync_pt = pt; > - fence->cbs[a->num_fences + i].fence = fence; > - if (fence_add_callback(pt, &fence->cbs[a->num_fences + > i].cb, fence_check_cb_func)) > - atomic_dec(&fence->status); > - } > + if (num_fences > i) > + atomic_sub(num_fences - i, &fence->status); > + fence->num_fences = i; > > sync_fence_debug_add(fence); > return fence; > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel > From a440530a29682c595ad69b8cbb35c568228a8777 Mon Sep 17 00:00:00 2001 From: Rom Lemarchand Date: Thu, 7 Nov 2013 11:36:08 -0800 Subject: [PATCH] fix Change-Id: Ie8a10dc466462835456d12962b378158a917a33e --- drivers/staging/android/sync.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c index 1025857..04af0fe 100644 --- a/drivers/staging/android/sync.c +++ b/drivers/staging/android/sync.c @@ -266,11 +266,19 @@ struct sync_fence *sync_fence_merge(const char *name, struct fence *pt_a = a->cbs[i_a].sync_pt; struct fence *pt_b = b->cbs[i_b].sync_pt; - if (i_b >= b->num_fences || pt_a->context < pt_b->context) { + if (i_b >= b->num_fences) { sync_fence_add_pt(fence, &i, pt_a); i_a++; - } else if (i_a >= a->num_fences || pt_a->context > pt_b->context) { + } else if (i_a >= a->num_fences) { + sync_fence_add_pt(fence, &i, pt_b); + + i_b++; + } else if (pt_a->context < pt_b->context) { + sync_fence_add_pt(fence, &i, pt_a); + + i_a++; + } else if (pt_a->context > pt_b->context) { sync_fence_add_pt(fence, &i, pt_b); i_b++; -- 1.8.4.1