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

[Maarten Lankhorst]

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

~Maarten