Sorry about the delay. Hopefully other people from Android will also chime in. We need the ability to merge sync fences and keep the sync_pt ordered: the idea behind sync timelines is that we promise an ordering of operations.
Our reference device is Nexus 10: we need to make sure that any new implementation satisfies the same requirements.
You can find sample use-cases here, we also use it in our hardware composer libraries: https://android.googlesource.com/platform/system/core/+/refs/heads/master/li... https://android.googlesource.com/platform/frameworks/native/+/master/libs/ui...
On Wed, Oct 30, 2013 at 5:17 AM, Maarten Lankhorst < maarten.lankhorst@canonical.com> wrote:
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@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. Callbackshould make sure that the fence will
// be signaled asap. bool (*signaled)(struct fence *fence); // optional, but if set toNULL fence_is_signaled is not
// required to ever return true, unless enable_signaling iscalled, 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 isrecommended. It calls enable_signaling
// and appends itself to async callback list. Identical semanticsto 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, intsize);
/* 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
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel