<div dir="ltr">Sorry about the delay.<div>Hopefully other people from Android will also chime in.</div><div>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.</div> <div> </div><div>Our reference device is Nexus 10: we need to make sure that any new implementation satisfies the same requirements.</div><div> </div><div>You can find sample use-cases here, we also use it in our hardware composer libraries:</div> <div><a href="https://android.googlesource.com/platform/system/core/+/refs/heads/master/libsync/" target="_blank">https://android.googlesource.com/platform/system/core/+/refs/heads/master/libsync/</a> </div><div><a href="https://android.googlesource.com/platform/frameworks/native/+/master/libs/ui/Fence.cpp" target="_blank">https://android.googlesource.com/platform/frameworks/native/+/master/libs/ui/Fence.cpp</a> </div><div> </div> </div><div class="gmail_extra"> <div class="gmail_quote">On Wed, Oct 30, 2013 at 5:17 AM, Maarten Lankhorst <<a href="mailto:maarten.lankhorst@canonical.com" target="_blank">maarten.lankhorst@canonical.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">op 24-10-13 14:13, Maarten Lankhorst schreef: <div><div class="h5">> 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 <<a href="mailto:konkers@android.com">konkers@android.com</a>> wrote: >>>> On Wed, Oct 2, 2013 at 12:35 AM, Maarten Lankhorst >>>> <<a href="mailto:maarten.lankhorst@canonical.com">maarten.lankhorst@canonical.com</a>> 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: > > <a href="http://cgit.freedesktop.org/~mlankhorst/linux" target="_blank">http://cgit.freedesktop.org/~mlankhorst/linux</a> > > 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. :) > </div></div>Come on, any feedback? I want to move the discussion forward. ~Maarten <div class="HOEnZb"><div class="h5"> _______________________________________________ dri-devel mailing list <a href="mailto:dri-devel@lists.freedesktop.org">dri-devel@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/dri-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/dri-devel</a> </div></div></blockquote></div> </div>