Re: [RFC v2 3/3] dma-buf/sync_file: rework fence storage in struct file

[Gustavo Padovan <gustavo.padovan@collabora.com>]

2016-06-28 Chris Wilson <chris@chris-wilson.co.uk>:

> On Mon, Jun 27, 2016 at 04:29:22PM -0300, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > Create sync_file->fence to abstract the type of fence we are using for
> > each sync_file. If only one fence is present we use a normal struct fence
> > but if there is more fences to be added to the sync_file a fence_array
> > is created.
> > 
> > This change cleans up sync_file a bit. We don't need to have sync_file_cb
> > array anymore. Instead, as we always have  one fence, only one fence
> > callback is registered per sync_file.
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Christian König <christian.koenig@amd.com>
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > ---
> > @@ -76,21 +76,19 @@ struct sync_file *sync_file_create(struct fence *fence)
> >  {
> >  	struct sync_file *sync_file;
> >  
> > -	sync_file = sync_file_alloc(offsetof(struct sync_file, cbs[1]));
> > +	sync_file = sync_file_alloc();
> >  	if (!sync_file)
> >  		return NULL;
> >  
> > -	sync_file->num_fences = 1;
> > +	sync_file->fence = fence;
> > +
> >  	atomic_set(&sync_file->status, 1);
> 
> sync_file->status => fence_is_signaled(sync_file->fence);
> 
> Both should just be an atomic read, except fence_is_signaled() will then
> do a secondary poll.

Not sure I follow. I set it to 1 here, but below when we call
fence_add_callback() and the fence is already signalled atomic_dec sets
sync_file->status to 0.

> 
> >  	snprintf(sync_file->name, sizeof(sync_file->name), "%s-%s%llu-%d",
> >  		 fence->ops->get_driver_name(fence),
> >  		 fence->ops->get_timeline_name(fence), fence->context,
> >  		 fence->seqno);
> >  
> > -	sync_file->cbs[0].fence = fence;
> > -	sync_file->cbs[0].sync_file = sync_file;
> > -	if (fence_add_callback(fence, &sync_file->cbs[0].cb,
> > -			       fence_check_cb_func))
> > +	if (fence_add_callback(fence, &sync_file->cb, fence_check_cb_func))
> >  		atomic_dec(&sync_file->status);
> >  
> >  	return sync_file;
> > @@ -121,14 +119,42 @@ err:
> >  	return NULL;
> >  }
> >  
> > -static void sync_file_add_pt(struct sync_file *sync_file, int *i,
> > -			     struct fence *fence)
> > +static int sync_file_set_fence(struct sync_file *sync_file,
> > +			       struct fence **fences, int num_fences)
> >  {
> > -	sync_file->cbs[*i].fence = fence;
> > -	sync_file->cbs[*i].sync_file = sync_file;
> > +	struct fence_array *array;
> > +
> > +	if (num_fences == 1) {
> > +		sync_file->fence = fences[0];
> 
> This steals the references.
> 
> > +	} else {
> > +		array = fence_array_create(num_fences, fences,
> > +					   fence_context_alloc(1), 1, false);
> 
> This creates a reference.
> 
> When we call fence_put(sync_fence->fence) we release a reference we
> never owned if num_fences == 1.

No, sync_file_merge() gets a new reference for each fence it is going to
add to the new fence. So for num_fences == 1 when sync_file->fence is
set we already hold a reference to it, so no matter if it is a fence or
a array we own a reference.

> 
> > +	struct fence **fences, **a_fences, **b_fences;
> > +	int i, i_a, i_b, num_fences, a_num_fences, b_num_fences;
> >  
> > +	a_fences = get_fences(a, &a_num_fences);
> > +	b_fences = get_fences(b, &b_num_fences);
> > +	num_fences = a_num_fences + b_num_fences;
> > +
> > +	fences = kcalloc(num_fences, sizeof(**fences), GFP_KERNEL);
> 
> Just sizeof(*fences) (you want to allocate an array of pointers, not an
> array of fence structs).

Okay.

	Gustavo