[Linaro-mm-sig] thoughts of looking at android fences
Rom Lemarchand
romlem at android.com
Fri Nov 1 14:03:57 PDT 2013
Sorry about the delay.
Hopefully other people from Android will also chime in.
We need the ability to merge sync fences and keep the sync_pt ordered: the
idea behind sync timelines is that we promise an ordering of operations.
Our reference device is Nexus 10: we need to make sure that any new
implementation satisfies the same requirements.
You can find sample use-cases here, we also use it in our hardware composer
libraries:
https://android.googlesource.com/platform/system/core/+/refs/heads/master/libsync/
https://android.googlesource.com/platform/frameworks/native/+/master/libs/ui/Fence.cpp
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:
> > op 09-10-13 16:39, Maarten Lankhorst schreef:
> >> Hey,
> >>
> >> op 08-10-13 19:37, John Stultz schreef:
> >>> On Wed, Oct 2, 2013 at 11:13 AM, Erik Gilling <konkers at android.com>
> wrote:
> >>>> On Wed, Oct 2, 2013 at 12:35 AM, Maarten Lankhorst
> >>>> <maarten.lankhorst at canonical.com> wrote:
> >>>>> Depending on feedback I'll try reflashing my nexus 7 to stock
> android, and work on trying to convert android
> >>>>> syncpoints to dma-fence, which I'll probably rename to syncpoints.
> >>>> I thought the plan decided at plumbers was to investigate backing
> >>>> dma_buf with the android sync solution not the other way around. It
> >>>> doesn't make sense to me to take a working, tested, end-to-end
> >>>> solution with a released compositing system built around it, throw it
> >>>> out, and replace it with new un-tested code to
> >>>> support a system which is not yet built.
> >>> Hey Erik,
> >>> Thanks for the clarifying points in your email, your insights and
> >>> feedback are critical, and I think having you and Maarten continue to
> >>> work out the details here will make this productive.
> >>>
> >>> My recollection from the discussion was that Rob was ok with trying to
> >>> pipe the sync arguments through the various interfaces in order to
> >>> support the explicit sync, but I think he did suggest having it backed
> >>> by the dma-buf fences underneath.
> >>>
> >>> I know this can be frustrating to watch things be reimplemented when
> >>> you have a pre-baked solution, but some compromise will be needed to
> >>> get things merged (and Maarten is taking the initiative here), but its
> >>> important to keep discussing this so the *right* compromises are made
> >>> that don't hurt performance, etc.
> >>>
> >>> My hope is Maarten's approach of getting the dma-fence core
> >>> integrated, and then moving the existing Android sync interface over
> >>> to the shared back-end, will allow for proper apples-to-apples
> >>> comparisons of the same interface. And if the functionality isn't
> >>> sufficient we can hold off on merging the sync interface conversion
> >>> until that gets resolved.
> >>>
> >> Yeah, I'm trying to understand the android side too. I think a unified
> interface would benefit both. I'm
> >> toying a bit with the sw_sync driver in staging because it's the
> easiest to try out on my desktop.
> >>
> >> The timeline stuff looks like it could be simplified. The main
> difference that there seems to be is that
> >> I didn't want to create a separate timeline struct for synchronization
> but let the drivers handle it.
> >>
> >> If you use rcu for reference lifetime management of timeline, the kref
> can be dropped. Signalling all
> >> syncpts on timeline destroy to a new destroyed state would kill the
> need for a destroyed member.
> >> The active list is unneeded and can be killed if only a linear
> progression of child_list is allowed.
> >>
> >> Which probably leaves this nice structure:
> >> struct sync_timeline {
> >> const struct sync_timeline_ops *ops;
> >> char name[32];
> >>
> >> struct list_head child_list_head;
> >> spinlock_t child_list_lock;
> >>
> >> struct list_head sync_timeline_list;
> >> };
> >>
> >> Where name, and sync_timeline_list are nice for debugging, but I guess
> not necessarily required. so that
> >> could be split out into a separate debugfs thing if required. I've
> moved the pointer to ops to the fence
> >> for dma-fence, which leaves this..
> >>
> >> struct sync_timeline {
> >> struct list_head child_list_head;
> >> spinlock_t child_list_lock;
> >>
> >> struct sync_timeline_debug {
> >> struct list_head sync_timeline_list;
> >> char name[32];
> >> };
> >> };
> >>
> >> Hm, this looks familiar, the drm drivers had some structure for
> protecting the active fence list that has
> >> an identical definition, but with a slightly different list offset..
> >>
> >> struct __wait_queue_head {
> >> spinlock_t lock;
> >> struct list_head task_list;
> >> };
> >>
> >> typedef struct __wait_queue_head wait_queue_head_t;
> >>
> >> This is nicer to convert the existing drm drivers, which already
> implement synchronous wait with a waitqueue.
> >> The default wait op is in fact
> >>
> >> Ok enough of this little excercise. I just wanted to see how different
> the 2 are. I think even if the
> >> fence interface will end up being incompatible it wouldn't be too hard
> to convert android..
> >>
> >> Main difference is the ops, android has a lot more than what I used for
> dma-fence:
> >>
> >> struct fence_ops {
> >> bool (*enable_signaling)(struct fence *fence); // required,
> callback called with fence->lock held,
> >> // fence->lock is a pointer passed to __fence_init. Callback
> should make sure that the fence will
> >> // be signaled asap.
> >> bool (*signaled)(struct fence *fence); // optional, but if set to
> NULL fence_is_signaled is not
> >> // required to ever return true, unless enable_signaling is
> called, similar to has_signaled
> >> long (*wait)(struct fence *fence, bool intr, signed long timeout);
> // required, but it can be set
> >> // to the default fence_default_wait implementation which is
> recommended. It calls enable_signaling
> >> // and appends itself to async callback list. Identical semantics
> to wait_event_interruptible_timeout.
> >> void (*release)(struct fence *fence); // free_pt
> >> };
> >>
> >> Because every fence has a stamp, there is no need for a compare op.
> >>
> >> struct sync_timeline_ops {
> >> const char *driver_name;
> >>
> >> /* required */
> >> struct sync_pt *(*dup)(struct sync_pt *pt);
> >>
> >> /* required */
> >> int (*has_signaled)(struct sync_pt *pt);
> >>
> >> /* required */
> >> int (*compare)(struct sync_pt *a, struct sync_pt *b);
> >>
> >> /* optional */
> >> void (*free_pt)(struct sync_pt *sync_pt);
> >>
> >> /* optional */
> >> void (*release_obj)(struct sync_timeline *sync_timeline);
> >>
> >> /* deprecated */
> >> void (*print_obj)(struct seq_file *s,
> >> struct sync_timeline *sync_timeline);
> >>
> >> /* deprecated */
> >> void (*print_pt)(struct seq_file *s, struct sync_pt *sync_pt);
> >>
> >> /* optional */
> >> int (*fill_driver_data)(struct sync_pt *syncpt, void *data, int
> size);
> >>
> >> /* optional */
> >> void (*timeline_value_str)(struct sync_timeline *timeline, char
> *str,
> >> int size);
> >>
> >> /* optional */
> >> void (*pt_value_str)(struct sync_pt *pt, char *str, int size);
> >> };
> >>
> >> The dup is weird, I have nothing like that. I do allow multiple
> callbacks to be added to the same
> >> dma-fence, and allow callbacks to be aborted, provided you still hold a
> refcount.
> >>
> >> So from the ops it looks like what's mostly missing is print_pt,
> fill_driver_data,
> >> timeline_value_str and pt_value_str.
> >>
> >> I have no idea how much of this is inaccurate. This is just an
> assessment based on me looking at
> >> the stuff in drivers/staging/android/sync for an afternoon and the
> earlier discussions. :)
> >>
> > 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
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20131101/d37d65a9/attachment-0001.html>
More information about the dri-devel
mailing list