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