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

[Maarten Lankhorst <maarten.lankhorst@canonical.com>]

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@android.com> wrote:
>>> On Wed, Oct 2, 2013 at 12:35 AM, Maarten Lankhorst
>>> <maarten.lankhorst@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. :)