op 02-11-13 22:36, Colin Cross schreef:
> On Wed, Oct 30, 2013 at 5:17 AM, Maarten Lankhorst
> <maarten.lankhorst at canonical.com> 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.
Hey,
fence_add_callback will now always initialize cb->node, even on failure.
I added __fence_is_signaled, to be used with the lock held.
sync_print_pt didn't work when the fence was signaled with an error, I fixed
that.
So I reworked patch below, no merge optimization yet. It will be done as a
separate patch. :)
---
diff --git a/drivers/base/fence.c b/drivers/base/fence.c
index 89c89ae19f58..9e7a63c4b07f 100644
--- a/drivers/base/fence.c
+++ b/drivers/base/fence.c
@@ -185,8 +185,10 @@ int fence_add_callback(struct fence *fence, struct
fence_cb *cb,
if (WARN_ON(!fence || !func))
return -EINVAL;
- if (test_bit(FENCE_FLAG_SIGNALED_BIT, &fence->flags))
+ if (test_bit(FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
+ LIST_INIT_HEAD(&cb->node);
return -ENOENT;
+ }
spin_lock_irqsave(fence->lock, flags);
@@ -202,7 +204,8 @@ int fence_add_callback(struct fence *fence, struct fence_cb
*cb,
if (!ret) {
cb->func = func;
list_add_tail(&cb->node, &fence->cb_list);
- }
+ } else
+ LIST_INIT_HEAD(&cb->node);
spin_unlock_irqrestore(fence->lock, flags);
return ret;
diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c
index 110a9e99cb71..2c7fd3f2ab23 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,12 +108,8 @@ 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 {
- __fence_signal(&pt->base);
+ if (__fence_is_signaled(&pt->base))
list_del(&pt->active_list);
- }
}
spin_unlock_irqrestore(&obj->child_list_lock, flags);
}
@@ -122,6 +128,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);
@@ -255,7 +262,7 @@ 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))
+ if (fence_add_callback(pt, &fence->cbs[a->num_fences + i].cb,
fence_check_cb_func))
atomic_dec(&fence->status);
}
@@ -325,6 +332,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 +392,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..2ef6496c7cd0 100644
--- a/drivers/staging/android/sync_debug.c
+++ b/drivers/staging/android/sync_debug.c
@@ -82,18 +82,18 @@ 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 <= 0) {
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..dd1639ff96c7 100644
--- a/include/linux/fence.h
+++ b/include/linux/fence.h
@@ -237,6 +237,20 @@ int fence_add_callback(struct fence *fence, struct
fence_cb *cb,
bool fence_remove_callback(struct fence *fence, struct fence_cb *cb);
void fence_enable_sw_signaling(struct fence *fence);
+static inline bool
+__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)) {
+ __fence_signal(fence);
+ return true;
+ }
+
+ return false;
+}
+
/**
* fence_is_signaled - Return an indication if the fence is signaled yet.
* @fence: [in] the fence to check
