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
