From patchwork Sat Nov 2 21:36:35 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Colin Cross X-Patchwork-Id: 21320 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-pd0-f199.google.com (mail-pd0-f199.google.com [209.85.192.199]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id DFDF225B39 for ; Sun, 3 Nov 2013 03:35:07 +0000 (UTC) Received: by mail-pd0-f199.google.com with SMTP id y10sf9418577pdj.2 for ; Sat, 02 Nov 2013 20:35:06 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:mime-version:in-reply-to:references :date:message-id:from:to:cc:subject:precedence:list-id :list-unsubscribe:list-archive:list-post:list-help:list-subscribe :errors-to:sender:x-original-sender :x-original-authentication-results:mailing-list:content-type; bh=8ER1X0pvmtznAKERVoj4N3AIKIb+4xsGBZdc4FBkosU=; b=Gq5C4THMCGDCxjmRqWk0R3uNPkutqRIGOlD9DX0lBB0KJJwjssW9LJ1qDGdUuuwote 56lT7+LEuQH0U77yuCdWYQ47YtWd+EkqArSvmawAcIxkk6PFw29YBIl0DT5xsUykpx1I B+/GAoEfGVuEquURdoqo4OsfLWn6W9douUxqkAzAN4OCqf6F1FbbwRGeYkSsZqZgmcZS QvLzbp+BroWloPR8Z3YVYCxTpOzP4DM/m+ou9Tfj42xzZ48z5qx5MXMsGtO43qpawo1b 013OGjHm+vKQlbi7tZcl1AL30X0Q9fw0avVFMWQTzuzIjmRqzhFYP/PVck7MMegdKbXI IIlw== X-Gm-Message-State: ALoCoQlYVQb0FQzYVep2JHKoMRO2kjBIsNUs1eVUQnwtnmzGJojRL3u9EP+XlQPN3xhsvqY2qIp/ X-Received: by 10.66.102.69 with SMTP id fm5mr3439046pab.24.1383449706669; Sat, 02 Nov 2013 20:35:06 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.49.16.100 with SMTP id f4ls1126868qed.35.gmail; Sat, 02 Nov 2013 20:35:06 -0700 (PDT) X-Received: by 10.220.147.20 with SMTP id j20mr6725562vcv.21.1383449706476; Sat, 02 Nov 2013 20:35:06 -0700 (PDT) Received: from mail-vc0-x233.google.com (mail-vc0-x233.google.com [2607:f8b0:400c:c03::233]) by mx.google.com with ESMTPS id rp4si3754598vcb.36.2013.11.02.20.35.06 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Sat, 02 Nov 2013 20:35:06 -0700 (PDT) Received-SPF: neutral (google.com: 2607:f8b0:400c:c03::233 is neither permitted nor denied by best guess record for domain of patch+caf_=patchwork-forward=linaro.org@linaro.org) client-ip=2607:f8b0:400c:c03::233; Received: by mail-vc0-f179.google.com with SMTP id hz11so3912786vcb.10 for ; Sat, 02 Nov 2013 20:35:06 -0700 (PDT) X-Received: by 10.220.174.200 with SMTP id u8mr6609584vcz.6.1383449706276; Sat, 02 Nov 2013 20:35:06 -0700 (PDT) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patches@linaro.org Received: by 10.220.174.196 with SMTP id u4csp52029vcz; Sat, 2 Nov 2013 20:35:05 -0700 (PDT) X-Received: by 10.229.182.69 with SMTP id cb5mr13837263qcb.19.1383449704660; Sat, 02 Nov 2013 20:35:04 -0700 (PDT) Received: from ip-10-141-164-156.ec2.internal (lists.linaro.org. [54.225.227.206]) by mx.google.com with ESMTPS id w2si6602955qef.87.2013.11.02.20.35.03 for (version=TLSv1 cipher=RC4-SHA bits=128/128); Sat, 02 Nov 2013 20:35:04 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linaro-mm-sig-bounces@lists.linaro.org designates 54.225.227.206 as permitted sender) client-ip=54.225.227.206; Received: from localhost ([127.0.0.1] helo=ip-10-141-164-156.ec2.internal) by ip-10-141-164-156.ec2.internal with esmtp (Exim 4.76) (envelope-from ) id 1VcoQR-0006z0-MK; Sun, 03 Nov 2013 03:32:31 +0000 Received: from mail-ob0-f178.google.com ([209.85.214.178]) by ip-10-141-164-156.ec2.internal with esmtp (Exim 4.76) (envelope-from ) id 1Vcipo-0004rZ-Nb for linaro-mm-sig@lists.linaro.org; Sat, 02 Nov 2013 21:34:20 +0000 Received: by mail-ob0-f178.google.com with SMTP id wm4so5766647obc.23 for ; Sat, 02 Nov 2013 14:36:36 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.60.58.166 with SMTP id s6mr7582812oeq.40.1383428195893; Sat, 02 Nov 2013 14:36:35 -0700 (PDT) Received: by 10.182.140.82 with HTTP; Sat, 2 Nov 2013 14:36:35 -0700 (PDT) In-Reply-To: <5270F8D7.4040406@canonical.com> References: <524BCCD0.90002@canonical.com> <52556ABE.2090201@canonical.com> <52690EEC.5000501@canonical.com> <5270F8D7.4040406@canonical.com> Date: Sat, 2 Nov 2013 14:36:35 -0700 Message-ID: From: Colin Cross To: Maarten Lankhorst X-Mailman-Approved-At: Sun, 03 Nov 2013 03:32:30 +0000 Cc: "linaro-mm-sig@lists.linaro.org" , Android Kernel Team , John Stultz , "dri-devel@lists.freedesktop.org" Subject: Re: [Linaro-mm-sig] thoughts of looking at android fences X-BeenThere: linaro-mm-sig@lists.linaro.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: , List-Help: , List-Subscribe: , Errors-To: linaro-mm-sig-bounces@lists.linaro.org Sender: linaro-mm-sig-bounces@lists.linaro.org X-Original-Sender: ccross@google.com X-Original-Authentication-Results: mx.google.com; spf=neutral (google.com: 2607:f8b0:400c:c03::233 is neither permitted nor denied by best guess record for domain of patch+caf_=patchwork-forward=linaro.org@linaro.org) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org; dkim=neutral (bad format) header.i=@google.com; dmarc=fail (p=REJECT dis=NONE) header.from=google.com Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org X-Google-Group-Id: 836684582541 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. >From e8f89ae535e227490d3116a03f2be5dc780e5be9 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Fri, 1 Nov 2013 19:26:54 -0700 Subject: [PATCH] dma fence fixes Change-Id: I9308b51c23e762fde95106db301cf6aedd8845f5 --- drivers/staging/android/sync.c | 27 +++++++++++++++++++++------ drivers/staging/android/sync_debug.c | 10 ++++------ include/linux/fence.h | 4 ++-- 3 files changed, 27 insertions(+), 14 deletions(-) diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c index 110a9e99cb71..672020d5f566 100644 --- a/drivers/staging/android/sync.c +++ b/drivers/staging/android/sync.c @@ -74,6 +74,16 @@ static void sync_timeline_free(struct kref *kref) kfree(obj); } +static void sync_timeline_get(struct sync_timeline *obj) +{ + kref_get(&obj->kref); +} + +static void sync_timeline_put(struct sync_timeline *obj) +{ + kref_put(&obj->kref, sync_timeline_free); +} + void sync_timeline_destroy(struct sync_timeline *obj) { obj->destroyed = true; @@ -83,8 +93,8 @@ void sync_timeline_destroy(struct sync_timeline *obj) * that their parent is going away. */ - if (!kref_put(&obj->kref, sync_timeline_free)) - sync_timeline_signal(obj); + sync_timeline_signal(obj); + sync_timeline_put(obj); } EXPORT_SYMBOL(sync_timeline_destroy); @@ -98,9 +108,7 @@ void sync_timeline_signal(struct sync_timeline *obj) spin_lock_irqsave(&obj->child_list_lock, flags); list_for_each_entry_safe(pt, next, &obj->active_list_head, active_list) { - if (!pt->base.ops->signaled(&pt->base)) - break; - else { + if (pt->base.ops->signaled(&pt->base)) { __fence_signal(&pt->base); list_del(&pt->active_list); } @@ -122,6 +130,7 @@ struct sync_pt *sync_pt_create(struct sync_timeline *obj, int size) return NULL; spin_lock_irqsave(&obj->child_list_lock, flags); + sync_timeline_get(obj); __fence_init(&pt->base, &android_fence_ops, &obj->child_list_lock, obj->context, ++obj->value); list_add_tail(&pt->child_list, &obj->child_list_head); INIT_LIST_HEAD(&pt->active_list); @@ -186,6 +195,7 @@ struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt) fence_get(&pt->base); fence->cbs[0].sync_pt = &pt->base; fence->cbs[0].fence = fence; + INIT_LIST_HEAD(&fence->cbs[0].cb.node); if (fence_add_callback(&pt->base, &fence->cbs[0].cb, fence_check_cb_func)) atomic_dec(&fence->status); @@ -245,6 +255,7 @@ struct sync_fence *sync_fence_merge(const char *name, fence_get(pt); fence->cbs[i].sync_pt = pt; fence->cbs[i].fence = fence; + INIT_LIST_HEAD(&fence->cbs[i].cb.node); if (fence_add_callback(pt, &fence->cbs[i].cb, fence_check_cb_func)) atomic_dec(&fence->status); } @@ -255,7 +266,8 @@ struct sync_fence *sync_fence_merge(const char *name, 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[i].cb, fence_check_cb_func)) + INIT_LIST_HEAD(&fence->cbs[a->num_fences + i].cb.node); + if (fence_add_callback(pt, &fence->cbs[a->num_fences + i].cb, fence_check_cb_func)) atomic_dec(&fence->status); } @@ -325,6 +337,8 @@ int sync_fence_wait(struct sync_fence *fence, long timeout) if (timeout < 0) timeout = MAX_SCHEDULE_TIMEOUT; + else + timeout = msecs_to_jiffies(timeout); trace_sync_wait(fence, 1); for (i = 0; i < fence->num_fences; ++i) @@ -383,6 +397,7 @@ static void android_fence_release(struct fence *fence) if (parent->ops->free_pt) parent->ops->free_pt(pt); + sync_timeline_put(parent); kfree(pt); } diff --git a/drivers/staging/android/sync_debug.c b/drivers/staging/android/sync_debug.c index 55ad34085f2f..8671fbb7d143 100644 --- a/drivers/staging/android/sync_debug.c +++ b/drivers/staging/android/sync_debug.c @@ -82,18 +82,16 @@ static const char *sync_status_str(int status) static void sync_print_pt(struct seq_file *s, struct sync_pt *pt, bool fence) { - int status = 0; + int status = 1; struct sync_timeline *parent = sync_pt_parent(pt); - if (fence_is_signaled(&pt->base)) { + if (fence_is_signaled(&pt->base)) status = pt->base.status; - if (!status) - status = 1; - } + seq_printf(s, " %s%spt %s", fence ? parent->name : "", fence ? "_" : "", sync_status_str(status)); - if (status) { + if (!status) { struct timeval tv = ktime_to_timeval(pt->base.timestamp); seq_printf(s, "@%ld.%06ld", tv.tv_sec, tv.tv_usec); } diff --git a/include/linux/fence.h b/include/linux/fence.h index 2beb3b0ff2a3..e1b2fc6e8007 100644 --- a/include/linux/fence.h +++ b/include/linux/fence.h @@ -257,10 +257,10 @@ fence_is_signaled(struct fence *fence) if (test_bit(FENCE_FLAG_SIGNALED_BIT, &fence->flags)) return true; - if (fence->ops->signaled && fence->ops->signaled(fence)) { + /*if (fence->ops->signaled && fence->ops->signaled(fence)) { fence_signal(fence); return true; - } + }*/ return false; } -- 1.8.4.1