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

[Daniel Vetter]

On Mon, May 23, 2016 at 01:29:11PM +0200, Christian König wrote:
> Am 23.05.2016 um 09:41 schrieb Daniel Vetter:
> >On Fri, May 20, 2016 at 11:47:28AM -0300, Gustavo Padovan wrote:
> >>2016-05-20 Christian König <deathsimple@xxxxxxxxxxx>:
> >>
> >>>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.
> >>>	- Document that create function takes ownership of array.
> >>This looks good to me. Dropping NO_CONTEXT was a good idea, also
> >>registering only one callback makes it looks better.
> >This will make it even harder to eventually add a real fence_context
> >structure for tracking and debugging. I know you don't care for amdgpu
> >since you have amdgpu-specific debug files, and there's some lifetime fun
> >that makes it not immediately obvious how to resolve it.
> 
> Completely independent of my work on amdgpu I still think that it's not such
> a good idea to use a complex structure for the fence context.
> 
> Especially on SoCs and small embedded systems you probably don't want to
> overhead associated with that only for debugging purposes in a production
> environment.

At least in all the drivers I've seen you have to allocate a little bit of
stuff _anyway_ to store that context id, plus a bunch of hw state. So the
allocation itsel shouldn't be a problem at all, since that can be handled
by embedding.

If it's the atomic inc/dec for refcounting you're worried about, then that
could be made dependent on CONFIG_FENCE_DEBUGGING. And android didn't have
that Kconfig knob even, and people seemingly didn't care about the
overhead on arm socs.

> >But on "lots of
> >shitty little drivers" systems aka SoCs generic debugging information is
> >crucial I think. Not liking too much where this is going.
> 
> Yeah I agree that generic debugging information is usually crucial, but the
> lifetime issues indeed can't be solved without reference counting and a hole
> bunch of overhead.
> 
> How about V5 of the patch I've just send out? Apart from fixing a few issues
> I've made the context and sequence number parameters of the fence_array
> object.
> 
> This way you don't need to always allocate a new context for each object,
> but just enough to keep your timelines straight.
> 
> E.g. you don't get a lot of contexts only used once. This is at least
> sufficient for my amdgpu use case.

Well my idea behind NO_CONTEXT was that this way there'd be only one
special context for merged fences, which could have a linked list of all
fences ever (with debugging stuff). That way there's no need to allocate
one context per fence_array. It has the downside of a slightly leaky
abstraction, as in you must use the provided interface functions to figure
out whether a fence is on the same timeline, and if so, which one is
later.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch