Re: [PATCH 4/6] android: convert sync to fence api, v4

[Thomas Hellstrom <thellstrom@vmware.com>]

On 02/17/2014 04:57 PM, Maarten Lankhorst wrote:
> Android syncpoints can be mapped to a timeline. This removes the need
> to maintain a separate api for synchronization. I've left the android
> trace events in place, but the core fence events should already be
> sufficient for debugging.
>
> v2:
> - Call fence_remove_callback in sync_fence_free if not all fences have fired.
> v3:
> - Merge Colin Cross' bugfixes, and the android fence merge optimization.
> v4:
> - Merge with the upstream fixes.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
> ---
>  drivers/staging/android/Kconfig      |    1 
>  drivers/staging/android/Makefile     |    2 
>  drivers/staging/android/sw_sync.c    |    4 
>  drivers/staging/android/sync.c       |  892 +++++++++++-----------------------
>  drivers/staging/android/sync.h       |   80 ++-
>  drivers/staging/android/sync_debug.c |  245 +++++++++
>  drivers/staging/android/trace/sync.h |   12 
>  7 files changed, 592 insertions(+), 644 deletions(-)
>  create mode 100644 drivers/staging/android/sync_debug.c
>
> diff --git a/drivers/staging/android/Kconfig b/drivers/staging/android/Kconfig
> index b91c758883bf..ecc8194242b5 100644
> --- a/drivers/staging/android/Kconfig
> +++ b/drivers/staging/android/Kconfig
> @@ -77,6 +77,7 @@ config SYNC
>  	bool "Synchronization framework"
>  	default n
>  	select ANON_INODES
> +	select DMA_SHARED_BUFFER
>  	---help---
>  	  This option enables the framework for synchronization between multiple
>  	  drivers.  Sync implementations can take advantage of hardware
> diff --git a/drivers/staging/android/Makefile b/drivers/staging/android/Makefile
> index 0a01e1914905..517ad5ffa429 100644
> --- a/drivers/staging/android/Makefile
> +++ b/drivers/staging/android/Makefile
> @@ -9,5 +9,5 @@ obj-$(CONFIG_ANDROID_TIMED_OUTPUT)	+= timed_output.o
>  obj-$(CONFIG_ANDROID_TIMED_GPIO)	+= timed_gpio.o
>  obj-$(CONFIG_ANDROID_LOW_MEMORY_KILLER)	+= lowmemorykiller.o
>  obj-$(CONFIG_ANDROID_INTF_ALARM_DEV)	+= alarm-dev.o
> -obj-$(CONFIG_SYNC)			+= sync.o
> +obj-$(CONFIG_SYNC)			+= sync.o sync_debug.o
>  obj-$(CONFIG_SW_SYNC)			+= sw_sync.o
> diff --git a/drivers/staging/android/sw_sync.c b/drivers/staging/android/sw_sync.c
> index f24493ac65e3..a76db3ff87cb 100644
> --- a/drivers/staging/android/sw_sync.c
> +++ b/drivers/staging/android/sw_sync.c
> @@ -50,7 +50,7 @@ static struct sync_pt *sw_sync_pt_dup(struct sync_pt *sync_pt)
>  {
>  	struct sw_sync_pt *pt = (struct sw_sync_pt *) sync_pt;
>  	struct sw_sync_timeline *obj =
> -		(struct sw_sync_timeline *)sync_pt->parent;
> +		(struct sw_sync_timeline *)sync_pt_parent(sync_pt);
>  
>  	return (struct sync_pt *) sw_sync_pt_create(obj, pt->value);
>  }
> @@ -59,7 +59,7 @@ static int sw_sync_pt_has_signaled(struct sync_pt *sync_pt)
>  {
>  	struct sw_sync_pt *pt = (struct sw_sync_pt *)sync_pt;
>  	struct sw_sync_timeline *obj =
> -		(struct sw_sync_timeline *)sync_pt->parent;
> +		(struct sw_sync_timeline *)sync_pt_parent(sync_pt);
>  
>  	return sw_sync_cmp(obj->value, pt->value) >= 0;
>  }
> diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c
> index 3d05f662110b..8e77cd73b739 100644
> --- a/drivers/staging/android/sync.c
> +++ b/drivers/staging/android/sync.c
> @@ -31,22 +31,13 @@
>  #define CREATE_TRACE_POINTS
>  #include "trace/sync.h"
>  
> -static void sync_fence_signal_pt(struct sync_pt *pt);
> -static int _sync_pt_has_signaled(struct sync_pt *pt);
> -static void sync_fence_free(struct kref *kref);
> -static void sync_dump(void);
> -
> -static LIST_HEAD(sync_timeline_list_head);
> -static DEFINE_SPINLOCK(sync_timeline_list_lock);
> -
> -static LIST_HEAD(sync_fence_list_head);
> -static DEFINE_SPINLOCK(sync_fence_list_lock);
> +static const struct fence_ops android_fence_ops;
> +static const struct file_operations sync_fence_fops;
>  
>  struct sync_timeline *sync_timeline_create(const struct sync_timeline_ops *ops,
>  					   int size, const char *name)
>  {
>  	struct sync_timeline *obj;
> -	unsigned long flags;
>  
>  	if (size < sizeof(struct sync_timeline))
>  		return NULL;
> @@ -57,17 +48,14 @@ struct sync_timeline *sync_timeline_create(const struct sync_timeline_ops *ops,
>  
>  	kref_init(&obj->kref);
>  	obj->ops = ops;
> +	obj->context = fence_context_alloc(1);
>  	strlcpy(obj->name, name, sizeof(obj->name));
>  
>  	INIT_LIST_HEAD(&obj->child_list_head);
> -	spin_lock_init(&obj->child_list_lock);
> -
>  	INIT_LIST_HEAD(&obj->active_list_head);
> -	spin_lock_init(&obj->active_list_lock);
> +	spin_lock_init(&obj->child_list_lock);
>  
> -	spin_lock_irqsave(&sync_timeline_list_lock, flags);
> -	list_add_tail(&obj->sync_timeline_list, &sync_timeline_list_head);
> -	spin_unlock_irqrestore(&sync_timeline_list_lock, flags);
> +	sync_timeline_debug_add(obj);
>  
>  	return obj;
>  }
> @@ -77,11 +65,8 @@ static void sync_timeline_free(struct kref *kref)
>  {
>  	struct sync_timeline *obj =
>  		container_of(kref, struct sync_timeline, kref);
> -	unsigned long flags;
>  
> -	spin_lock_irqsave(&sync_timeline_list_lock, flags);
> -	list_del(&obj->sync_timeline_list);
> -	spin_unlock_irqrestore(&sync_timeline_list_lock, flags);
> +	sync_timeline_debug_remove(obj);
>  
>  	if (obj->ops->release_obj)
>  		obj->ops->release_obj(obj);
> @@ -89,6 +74,16 @@ static void sync_timeline_free(struct kref *kref)
>  	kfree(obj);
>  }
>  
> +static void sync_timeline_get(struct sync_timeline *obj)
> +{
> +	kref_get(&obj->kref);
> +}
> +
> +static void sync_timeline_put(struct sync_timeline *obj)
> +{
> +	kref_put(&obj->kref, sync_timeline_free);
> +}
> +
>  void sync_timeline_destroy(struct sync_timeline *obj)
>  {
>  	obj->destroyed = true;
> @@ -98,75 +93,30 @@ void sync_timeline_destroy(struct sync_timeline *obj)
>  	 * signal any children that their parent is going away.
>  	 */
>  	sync_timeline_signal(obj);
> -
> -	kref_put(&obj->kref, sync_timeline_free);
> +	sync_timeline_put(obj);
>  }
>  EXPORT_SYMBOL(sync_timeline_destroy);
>  
> -static void sync_timeline_add_pt(struct sync_timeline *obj, struct sync_pt *pt)
> -{
> -	unsigned long flags;
> -
> -	pt->parent = obj;
> -
> -	spin_lock_irqsave(&obj->child_list_lock, flags);
> -	list_add_tail(&pt->child_list, &obj->child_list_head);
> -	spin_unlock_irqrestore(&obj->child_list_lock, flags);
> -}
> -
> -static void sync_timeline_remove_pt(struct sync_pt *pt)
> -{
> -	struct sync_timeline *obj = pt->parent;
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&obj->active_list_lock, flags);
> -	if (!list_empty(&pt->active_list))
> -		list_del_init(&pt->active_list);
> -	spin_unlock_irqrestore(&obj->active_list_lock, flags);
> -
> -	spin_lock_irqsave(&obj->child_list_lock, flags);
> -	if (!list_empty(&pt->child_list))
> -		list_del_init(&pt->child_list);
> -
> -	spin_unlock_irqrestore(&obj->child_list_lock, flags);
> -}
> -
>  void sync_timeline_signal(struct sync_timeline *obj)
>  {
>  	unsigned long flags;
>  	LIST_HEAD(signaled_pts);
> -	struct list_head *pos, *n;
> +	struct sync_pt *pt, *next;
>  
>  	trace_sync_timeline(obj);
>  
> -	spin_lock_irqsave(&obj->active_list_lock, flags);
> -
> -	list_for_each_safe(pos, n, &obj->active_list_head) {
> -		struct sync_pt *pt =
> -			container_of(pos, struct sync_pt, active_list);
> -
> -		if (_sync_pt_has_signaled(pt)) {
> -			list_del_init(pos);
> -			list_add(&pt->signaled_list, &signaled_pts);
> -			kref_get(&pt->fence->kref);
> -		}
> -	}
> -
> -	spin_unlock_irqrestore(&obj->active_list_lock, flags);
> -
> -	list_for_each_safe(pos, n, &signaled_pts) {
> -		struct sync_pt *pt =
> -			container_of(pos, struct sync_pt, signaled_list);
> -
> -		list_del_init(pos);
> -		sync_fence_signal_pt(pt);
> -		kref_put(&pt->fence->kref, sync_fence_free);
> +	spin_lock_irqsave(&obj->child_list_lock, flags);
> +	list_for_each_entry_safe(pt, next, &obj->active_list_head, active_list) {
> +		if (__fence_is_signaled(&pt->base))
> +			list_del(&pt->active_list);
>  	}
> +	spin_unlock_irqrestore(&obj->child_list_lock, flags);
>  }
>  EXPORT_SYMBOL(sync_timeline_signal);
>  
> -struct sync_pt *sync_pt_create(struct sync_timeline *parent, int size)
> +struct sync_pt *sync_pt_create(struct sync_timeline *obj, int size)
>  {
> +	unsigned long flags;
>  	struct sync_pt *pt;
>  
>  	if (size < sizeof(struct sync_pt))
> @@ -176,87 +126,27 @@ struct sync_pt *sync_pt_create(struct sync_timeline *parent, int size)
>  	if (pt == NULL)
>  		return NULL;
>  
> +	spin_lock_irqsave(&obj->child_list_lock, flags);
> +	sync_timeline_get(obj);
> +	__fence_init(&pt->base, &android_fence_ops, &obj->child_list_lock, obj->context, ++obj->value);
> +	list_add_tail(&pt->child_list, &obj->child_list_head);
>  	INIT_LIST_HEAD(&pt->active_list);
> -	kref_get(&parent->kref);
> -	sync_timeline_add_pt(parent, pt);
> -
> +	spin_unlock_irqrestore(&obj->child_list_lock, flags);
>  	return pt;
>  }
>  EXPORT_SYMBOL(sync_pt_create);
>  
>  void sync_pt_free(struct sync_pt *pt)
>  {
> -	if (pt->parent->ops->free_pt)
> -		pt->parent->ops->free_pt(pt);
> -
> -	sync_timeline_remove_pt(pt);
> -
> -	kref_put(&pt->parent->kref, sync_timeline_free);
> -
> -	kfree(pt);
> +	fence_put(&pt->base);
>  }
>  EXPORT_SYMBOL(sync_pt_free);
>  
> -/* call with pt->parent->active_list_lock held */
> -static int _sync_pt_has_signaled(struct sync_pt *pt)
> -{
> -	int old_status = pt->status;
> -
> -	if (!pt->status)
> -		pt->status = pt->parent->ops->has_signaled(pt);
> -
> -	if (!pt->status && pt->parent->destroyed)
> -		pt->status = -ENOENT;
> -
> -	if (pt->status != old_status)
> -		pt->timestamp = ktime_get();
> -
> -	return pt->status;
> -}
> -
> -static struct sync_pt *sync_pt_dup(struct sync_pt *pt)
> -{
> -	return pt->parent->ops->dup(pt);
> -}
> -
> -/* Adds a sync pt to the active queue.  Called when added to a fence */
> -static void sync_pt_activate(struct sync_pt *pt)
> -{
> -	struct sync_timeline *obj = pt->parent;
> -	unsigned long flags;
> -	int err;
> -
> -	spin_lock_irqsave(&obj->active_list_lock, flags);
> -
> -	err = _sync_pt_has_signaled(pt);
> -	if (err != 0)
> -		goto out;
> -
> -	list_add_tail(&pt->active_list, &obj->active_list_head);
> -
> -out:
> -	spin_unlock_irqrestore(&obj->active_list_lock, flags);
> -}
> -
> -static int sync_fence_release(struct inode *inode, struct file *file);
> -static unsigned int sync_fence_poll(struct file *file, poll_table *wait);
> -static long sync_fence_ioctl(struct file *file, unsigned int cmd,
> -			     unsigned long arg);
> -
> -
> -static const struct file_operations sync_fence_fops = {
> -	.release = sync_fence_release,
> -	.poll = sync_fence_poll,
> -	.unlocked_ioctl = sync_fence_ioctl,
> -	.compat_ioctl = sync_fence_ioctl,
> -};
> -
> -static struct sync_fence *sync_fence_alloc(const char *name)
> +static struct sync_fence *sync_fence_alloc(int size, const char *name)
>  {
>  	struct sync_fence *fence;
> -	unsigned long flags;
>  
> -	fence = kzalloc(sizeof(struct sync_fence), GFP_KERNEL);
> +	fence = kzalloc(size, GFP_KERNEL);
>  	if (fence == NULL)
>  		return NULL;
>  
> @@ -268,16 +158,8 @@ static struct sync_fence *sync_fence_alloc(const char *name)
>  	kref_init(&fence->kref);
>  	strlcpy(fence->name, name, sizeof(fence->name));
>  
> -	INIT_LIST_HEAD(&fence->pt_list_head);
> -	INIT_LIST_HEAD(&fence->waiter_list_head);
> -	spin_lock_init(&fence->waiter_list_lock);
> -
>  	init_waitqueue_head(&fence->wq);
>  
> -	spin_lock_irqsave(&sync_fence_list_lock, flags);
> -	list_add_tail(&fence->sync_fence_list, &sync_fence_list_head);
> -	spin_unlock_irqrestore(&sync_fence_list_lock, flags);
> -
>  	return fence;
>  
>  err:
> @@ -285,119 +167,40 @@ err:
>  	return NULL;
>  }
>  
> +static void fence_check_cb_func(struct fence *f, struct fence_cb *cb)
> +{
> +	struct sync_fence_cb *check = container_of(cb, struct sync_fence_cb, cb);
> +	struct sync_fence *fence = check->fence;
> +
> +	// TODO: Add a fence->status member and check it

Hmm, C++ / C99 style comments makes checkpatch.pl complain. Did you run
this series through checkpatch?

/Thomas