Re: [RFC PATCH 1/7] android: Support creating sync fence from drm fences

[Maarten Lankhorst <maarten.lankhorst@canonical.com>]

Hey,

On 26-09-14 12:00, Lauri Peltonen wrote:
> Modify sync_fence_create to accept an array of 'struct fence' objects.
> This will allow drm drivers to create sync_fence objects and pass sync
> fd's between user space with minimal modifications, without ever creating
> sync_timeline or sync_pt objects, and without implementing the
> sync_timeline_ops interface.
> 
> Modify the sync driver debug code to not assume that every 'struct fence'
> (that is associated with a 'struct sync_fence') is embedded within a
> 'struct sync_pt'.
> 
> Signed-off-by: Lauri Peltonen <lpeltonen@nvidia.com>
> ---
>  drivers/staging/android/sw_sync.c    |  3 ++-
>  drivers/staging/android/sync.c       | 34 ++++++++++++++++++---------------
>  drivers/staging/android/sync.h       | 11 ++++++-----
>  drivers/staging/android/sync_debug.c | 37 +++++++++++++++++-------------------
>  4 files changed, 44 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/staging/android/sw_sync.c b/drivers/staging/android/sw_sync.c
> index a76db3f..6949812 100644
> --- a/drivers/staging/android/sw_sync.c
> +++ b/drivers/staging/android/sw_sync.c
> @@ -184,7 +184,8 @@ static long sw_sync_ioctl_create_fence(struct sw_sync_timeline *obj,
>  	}
>  
>  	data.name[sizeof(data.name) - 1] = '\0';
> -	fence = sync_fence_create(data.name, pt);
> +	fence = sync_fence_create(data.name,
> +				  (struct fence *[]){ &pt->base }, 1);
>  	if (fence == NULL) {
>  		sync_pt_free(pt);
>  		err = -ENOMEM;
> diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c
> index e7b2e02..1d0d968 100644
> --- a/drivers/staging/android/sync.c
> +++ b/drivers/staging/android/sync.c
> @@ -187,28 +187,32 @@ static void fence_check_cb_func(struct fence *f, struct fence_cb *cb)
>  		wake_up_all(&fence->wq);
>  }
>  
> -/* TODO: implement a create which takes more that one sync_pt */
> -struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt)
> +struct sync_fence *sync_fence_create(const char *name,
> +				     struct fence **fences, int num_fences)
>  {
> -	struct sync_fence *fence;
> +	struct sync_fence *sync_fence;
> +	int size = offsetof(struct sync_fence, cbs[num_fences]);
> +	int i;
>  
> -	fence = sync_fence_alloc(offsetof(struct sync_fence, cbs[1]), name);
> -	if (fence == NULL)
> +	sync_fence = sync_fence_alloc(size, name);
> +	if (sync_fence == NULL)
>  		return NULL;
>  
> -	fence->num_fences = 1;
> -	atomic_set(&fence->status, 1);
> +	sync_fence->num_fences = num_fences;
> +	atomic_set(&sync_fence->status, 0);
>  
> -	fence_get(&pt->base);
> -	fence->cbs[0].sync_pt = &pt->base;
> -	fence->cbs[0].fence = fence;
> -	if (fence_add_callback(&pt->base, &fence->cbs[0].cb,
> -			       fence_check_cb_func))
> -		atomic_dec(&fence->status);
> +	for (i = 0; i < num_fences; i++) {
> +		struct fence *f = fences[i];
> +		struct sync_fence_cb *cb = &sync_fence->cbs[i];
>  
> -	sync_fence_debug_add(fence);
> +		cb->sync_pt = fence_get(f);
> +		cb->fence = sync_fence;
> +		if (!fence_add_callback(f, &cb->cb, fence_check_cb_func))
> +			atomic_inc(&sync_fence->status);
> +	}
> +	sync_fence_debug_add(sync_fence);
>  
> -	return fence;
> +	return sync_fence;
>  }
>  EXPORT_SYMBOL(sync_fence_create);
sync_fence_merge currently depends on the list of fences to be sorted to remove duplicates efficiently.
Feeding it a unsorted list will probably result in hard to debug bugs. :-)

> diff --git a/drivers/staging/android/sync.h b/drivers/staging/android/sync.h
> index 66b0f43..b8ad72c 100644
> --- a/drivers/staging/android/sync.h
> +++ b/drivers/staging/android/sync.h
> @@ -246,13 +246,14 @@ void sync_pt_free(struct sync_pt *pt);
>  
>  /**
>   * sync_fence_create() - creates a sync fence
> - * @name:	name of fence to create
> - * @pt:		sync_pt to add to the fence
> + * @name:	name of the sync fence to create
> + * @fences:	fences to add to the sync fence
> + * @num_fences:	the number of fences in the @fences array
>   *
> - * Creates a fence containg @pt.  Once this is called, the fence takes
> - * ownership of @pt.
> + * Creates a sync fence from an array of drm fences. Takes refs to @fences.
>   */
> -struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt);
> +struct sync_fence *sync_fence_create(const char *name,
> +				     struct fence **fences, int num_fences);
>  
>  /*
>   * API for sync_fence consumers
> diff --git a/drivers/staging/android/sync_debug.c b/drivers/staging/android/sync_debug.c
> index 257fc91..2d8873e 100644
> --- a/drivers/staging/android/sync_debug.c
> +++ b/drivers/staging/android/sync_debug.c
> @@ -81,33 +81,33 @@ static const char *sync_status_str(int status)
>  	return "error";
>  }
>  
> -static void sync_print_pt(struct seq_file *s, struct sync_pt *pt, bool fence)
> +static void sync_print_pt(struct seq_file *s, struct fence *pt, bool fence)
>  {
>  	int status = 1;
> -	struct sync_timeline *parent = sync_pt_parent(pt);
>  
> -	if (fence_is_signaled_locked(&pt->base))
> -		status = pt->base.status;
> +	if (fence_is_signaled_locked(pt))
> +		status = pt->status;
>  
> -	seq_printf(s, "  %s%spt %s",
> -		   fence ? parent->name : "",
> -		   fence ? "_" : "",
> -		   sync_status_str(status));
> +	if (fence)
> +		seq_printf(s, "  %d_pt %s", pt->context,
> +			   sync_status_str(status));
> +	else
> +		seq_printf(s, "  pt %s", sync_status_str(status));
>  
>  	if (status <= 0) {
> -		struct timeval tv = ktime_to_timeval(pt->base.timestamp);
> +		struct timeval tv = ktime_to_timeval(pt->timestamp);
>  
>  		seq_printf(s, "@%ld.%06ld", tv.tv_sec, tv.tv_usec);
>  	}
>  
> -	if (parent->ops->timeline_value_str &&
> -	    parent->ops->pt_value_str) {
> +	if (pt->ops->timeline_value_str &&
> +	    pt->ops->fence_value_str) {
>  		char value[64];
>  
> -		parent->ops->pt_value_str(pt, value, sizeof(value));
> +		pt->ops->fence_value_str(pt, value, sizeof(value));
>  		seq_printf(s, ": %s", value);
>  		if (fence) {
> -			parent->ops->timeline_value_str(parent, value,
> +			pt->ops->timeline_value_str(pt, value,
>  						    sizeof(value));
>  			seq_printf(s, " / %s", value);
>  		}
> @@ -121,7 +121,8 @@ static void sync_print_obj(struct seq_file *s, struct sync_timeline *obj)
>  	struct list_head *pos;
>  	unsigned long flags;
>  
> -	seq_printf(s, "%s %s", obj->name, obj->ops->driver_name);
> +	seq_printf(s, "%d %s %s", obj->context, obj->name,
> +		   obj->ops->driver_name);
>  
>  	if (obj->ops->timeline_value_str) {
>  		char value[64];
> @@ -136,7 +137,7 @@ static void sync_print_obj(struct seq_file *s, struct sync_timeline *obj)
>  	list_for_each(pos, &obj->child_list_head) {
>  		struct sync_pt *pt =
>  			container_of(pos, struct sync_pt, child_list);
> -		sync_print_pt(s, pt, false);
> +		sync_print_pt(s, &pt->base, false);
>  	}
>  	spin_unlock_irqrestore(&obj->child_list_lock, flags);
>  }
> @@ -151,11 +152,7 @@ static void sync_print_fence(struct seq_file *s, struct sync_fence *fence)
>  		   sync_status_str(atomic_read(&fence->status)));
>  
>  	for (i = 0; i < fence->num_fences; ++i) {
> -		struct sync_pt *pt =
> -			container_of(fence->cbs[i].sync_pt,
> -				     struct sync_pt, base);
> -
> -		sync_print_pt(s, pt, true);
> +		sync_print_pt(s, fence->cbs[i].sync_pt, true);
>  	}
>  
>  	spin_lock_irqsave(&fence->wq.lock, flags);
>