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

[Maarten Lankhorst]

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