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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
