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>
