[Linaro-mm-sig] thoughts of looking at android fences

[Maarten Lankhorst]

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
Well, in the normal case status may not be set, but fence may still be signaled. Any status should be set before signaling. In cases where non-android dma-fences are used.
I deliberately tried not to depend on any android stuff in the android fences, this way it could be used outside android too.

I'm trying to only set status on error. If there is a race with 2 threads calling fence_signal, one with error, one without, then the error will always be visible in fence->status that way.
But I guess this might be a corner case we wouldn't worry about normally...
> 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
To be honest, I was surprised this code mostly worked for the sw_fence test code I wrote originally. :)
I think I'll have to add a __fence_is_signaled for 5, which can be called with fence->lock held.


>
> 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.
Yeah I've been looking into how to do that. It's very easy to optimize actually. The dma-fence code requires a context,
which is a number (even if it may become a pointer later, it could be seen as a number). If we order all fences based on number,
fences could have an order based on context number. Merging fences would simply become adding 2 ordered lists, and dropping
any duplicates and signaled points.

I left it out for the RFC I wanted to keep things as readable as possible. :)

~Maarten