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. :)
