Re: [PATCH 2/2] dma-buf/fence: add fence_array fences v4

[Christian KÃnig]

Am 20.05.2016 um 16:42 schrieb Chris Wilson:
> On Fri, May 20, 2016 at 03:56:11PM +0200, Christian König wrote:
> > From: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
> >
> > struct fence_collection inherits from struct fence and carries a
> > collection of fences that needs to be waited together.
> >
> > It is useful to translate a sync_file to a fence to remove the complexity
> > of dealing with sync_files on DRM drivers. So even if there are many
> > fences in the sync_file that needs to waited for a commit to happen,
> > they all get added to the fence_collection and passed for DRM use as
> > a standard struct fence.
> >
> > That means that no changes needed to any driver besides supporting fences.
> >
> > fence_collection's fence doesn't belong to any timeline context, so
> > fence_is_later() and fence_later() are not meant to be called with
> > fence_collections fences.
> >
> > v2: Comments by Daniel Vetter:
> > 	- merge fence_collection_init() and fence_collection_add()
> > 	- only add callbacks at ->enable_signalling()
> > 	- remove fence_collection_put()
> > 	- check for type on to_fence_collection()
> > 	- adjust fence_is_later() and fence_later() to WARN_ON() if they
> > 	are used with collection fences.
> >
> > v3: - Initialize fence_cb.node at fence init.
> >
> >      Comments by Chris Wilson:
> > 	- return "unbound" on fence_collection_get_timeline_name()
> > 	- don't stop adding callbacks if one fails
> > 	- remove redundant !! on fence_collection_enable_signaling()
> > 	- remove redundant () on fence_collection_signaled
> > 	- use fence_default_wait() instead
> >
> > v4 (chk): Rework, simplification and cleanup:
> > 	- Drop FENCE_NO_CONTEXT handling, always allocate a context.
> > 	- Rename to fence_array.
> > 	- Return fixed driver name.
> > 	- Register only one callback at a time.
> Why? Even within a driver I expected there to be some amoritization of
> the signaling costs for handling multiple fences at once (at least the
> driver I'm familar with!).
>
> So more just curiousity as to your experience that favours sequential
> enabling.

Just the profane reason that I want to save the memory for all the 
callbacks.

But thinking about it you are probably right that we should enable the 
signaling for all fences immediately. Going to fix this in the next 
version of the patch.

> > +static bool fence_array_add_next_callback(struct fence_array *array)
> > +{
> > +	while (array->num_signaled < array->num_fences) {
> > +		struct fence *next = array->fences[array->num_signaled];
> > +
> > +		if (!fence_add_callback(next, &array->cb, fence_array_cb_func))
> > +			return true;
> > +
> > +		++array->num_signaled;
> > +	}
> > +
> > +	return false;
> > +}
> > +
> > +static void fence_array_cb_func(struct fence *f, struct fence_cb *cb)
> > +{
> > +	struct fence_array *array = container_of(cb, struct fence_array, cb);
> Some chasing around would have been saved by a
>
> 	assert_spin_locked(&array->lock);
>
> here.

Mhm, actually the array lock isn't held here. Thinking more about it 
adding a new callback from a fence callback can badly deadlock under 
certain situations.

I need to double check why the callback is called with the fence lock 
held here.

> > +
> > +	++array->num_signaled;
> > +	if (!fence_array_add_next_callback(array))
> > +		fence_signal(&array->base);
> > +}
> > +
> > +static bool fence_array_enable_signaling(struct fence *fence)
> > +{
> > +	struct fence_array *array = to_fence_array(fence);
> > +
> > +	return fence_array_add_next_callback(array);
> > +}
> > +
> > +static bool fence_array_signaled(struct fence *fence)
> > +{
> > +	struct fence_array *array = to_fence_array(fence);
> > +
> > +	return ACCESS_ONCE(array->num_signaled) == array->num_fences;
> Can just be READ_ONCE()

Good point, going to fix that.

Christian.

> -Chris