Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1312485

Re: [RFC 00/29] De-stage android's sync framework

Show all headers | View raw

2016-01-19 Daniel Vetter <daniel@ffwll.ch>:

> On Fri, Jan 15, 2016 at 12:55:10PM -0200, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > This patch series de-stage the sync framework, and in order to accomplish that
> > a bunch of cleanups/improvements on the sync and fence were made.
> > 
> > The sync framework contained some abstractions around struct fence and those
> > were removed in the de-staging process among other changes:
> > 
> > Userspace visible changes
> > -------------------------
> > 
> >  * The sw_sync file was moved from /dev/sw_sync to <debugfs>/sync/sw_sync. No
> >  other change.
> > 
> > Kernel API changes
> > ------------------
> > 
> >  * struct sync_timeline is now struct fence_timeline
> >  * sync_timeline_ops is now fence_timeline_ops and they now carry struct
> >  fence as parameter instead of struct sync_pt
> >  * a .cleanup() fence op was added to allow sync_fence to run a cleanup when
> >  the fence_timeline is destroyed
> >  * added fence_add_used_data() to pass a private point to struct fence. This
> >  pointer is sent back on the .cleanup op.
> >  * The sync timeline function were moved to be fence_timeline functions:
> > 	 - sync_timeline_create()	-> fence_timeline_create()
> > 	 - sync_timeline_get()		-> fence_timeline_get()
> > 	 - sync_timeline_put()		-> fence_timeline_put()
> > 	 - sync_timeline_destroy()	-> fence_timeline_destroy()
> > 	 - sync_timeline_signal()	-> fence_timeline_signal()
> > 
> >   * sync_pt_create() was replaced be fence_create_on_timeline()
> > 
> > Internal changes
> > ----------------
> > 
> >  * fence_timeline_ops was removed in favor of direct use fence_ops
> >  * fence default functions were created for fence_ops
> >  * removed structs sync_pt, sw_sync_timeline and sw_sync_pt
> 
> Bunch of fairly random comments all over:
> 
> - include/uapi/linux/sw_sync.h imo should be dropped, it's just a private
>   debugfs interface between fence fds and the testsuite. Since the plan is
>   to have the testcases integrated into the kernel tree too we don't need
>   a public header.
> 
> - similar for include/linux/sw_sync.h Imo that should all be moved into
>   sync_debug.c. Same for sw_sync.c, that should all land in sync_debug
>   imo, and made optional with a Kconfig option. At least we should reuse
>   CONFIG_DEBUGFS.
> 
> - fence_context and fence_timeline are really the same. timeline has some
>   super-basic support for doing sw-only fence timelines, but imo that's
>   not really worth keeping (and if so better to keep seperate in a
>   sw-fence.c or similar, like seqno-fence.c). The other main thing
>   timeline provides is support to clean up fences on a timeline. And imo
>   that cleanup should be done by the core fence support, not by the add-on
>   stuff.
> 
> Interlude about fence cleanup on driver unload:
> 
> Working drivers imo should never call timeline_destroy when there's still
> an unsignalled fence around for that timeline/context. That just means
> they're broken and failed to clean up all the pending work. So the problem
> really is only what to do with fences where the driver disappeared, and
> for that we essentially need a fence_revoke() function (which could be
> called internally from timeline_free). So here's what I think
> timeline_free should do:
> 
> for_each_fence_on_timel() {
> 	WARN_ON(!fence_is_signalled());
> 
> 	fence_revoke(fence);
> }
> 
> Implementing fence_revoke is a bit tricky since we need to make sure the
> memory contained ->ops and similar stuff doesn't disappear. Simplest
> option might be to grab a temporary reference (using
> kref_get_unless_zero), and then exchange ->ops with one that has only a
> release function. We don't need anything else as long as all fence_*
> functions the kernel might call check for signalling correctly first
> (fence_wait is broken at least).
> 
> Or we just give up (for now) and declare module unload as slightly racy.
> dma-buf is similar. An intermediate option might be to at least add a
> THIS_MODULE reference to each fence (but that's a bit expensive ...).
> 
> - back to timeline vs. context: I have no idea how to best clean up this
>   mess, but least painful option long-term is probably to switch over all
>   current users of fence_context_alloc to timelines and remove the plain
>   context interface.
> 
> - Imo the interface in include/linux/sync.h is duplicating too much of
>   fence.h. I think the only bits we need are the refcounting, creating,
>   fd-install and that's it. Plus a macro to loop over all the fences in a
>   sync_fence. With that drivers will only ever deal with a pile of
>   struct fence, making implicit fencing (using the fence list in dma-buf)
>   and explicit fencing (using the fence list in sync_fence) much more
>   similar.
> 
>   And we can easily do that since no internal users ;-)
> 
> - get_timeline_name and get_driver_name are imo too much indirection, just
>   add ->(drv_)name field to each of these.

I don't think is a good idea to change that now as there are other fence
users in the kernel using get_timeline_name and get_driver_name. What I
propose is try get rid of this when moving ops from fences to
fence_timeline.

	Gustavo

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[RFC 00/29] De-stage android's sync framework Gustavo Padovan <gustavo@padovan.org> - 2016-01-15 16:00 +0100
  [RFC 19/29] dma-buf/fence: create fence_default_fill_driver_data() Gustavo Padovan <gustavo@padovan.org> - 2016-01-15 16:00 +0100
  [RFC 15/29] dma-buf/fence: create fence_default_get_driver_name() Gustavo Padovan <gustavo@padovan.org> - 2016-01-15 16:00 +0100
  [RFC 29/29] dma-buf/fence: de-stage sync framework Gustavo Padovan <gustavo@padovan.org> - 2016-01-15 16:00 +0100
  [RFC 26/29] dma-buf/fence: remove pointless fence_timeline_signal at destroy phase Gustavo Padovan <gustavo@padovan.org> - 2016-01-15 16:00 +0100
    Re: [RFC 26/29] dma-buf/fence: remove pointless fence_timeline_signal  at destroy phase John Harrison <John.C.Harrison@Intel.com> - 2016-01-15 18:50 +0100
      Re: [RFC 26/29] dma-buf/fence: remove pointless  fence_timeline_signal at destroy phase Gustavo Padovan <gustavo.padovan@collabora.co.uk> - 2016-01-15 19:10 +0100
        Re: [RFC 26/29] dma-buf/fence: remove pointless fence_timeline_signal  at destroy phase Greg Hackmann <ghackmann@google.com> - 2016-01-16 00:50 +0100
  [RFC 08/29] staging/android: Remove WARN_ON_ONCE when releasing sync_fence Gustavo Padovan <gustavo@padovan.org> - 2016-01-15 16:10 +0100
  [RFC 02/29] staging/android: fix checkpatch warning Gustavo Padovan <gustavo@padovan.org> - 2016-01-15 16:10 +0100
  [RFC 11/29] dma-buf/fence: move sync_timeline to fence_timeline Gustavo Padovan <gustavo@padovan.org> - 2016-01-15 16:10 +0100
    Re: [RFC 11/29] dma-buf/fence: move sync_timeline to fence_timeline Greg Hackmann <ghackmann@google.com> - 2016-01-20 02:00 +0100
  [RFC 13/29] dma-buf/fence: create fence_default_enable_signaling() Gustavo Padovan <gustavo@padovan.org> - 2016-01-15 16:10 +0100
  [RFC 05/29] staging/android: remove not used sync_timeline ops Gustavo Padovan <gustavo@padovan.org> - 2016-01-15 16:10 +0100
  [RFC 14/29] dma-buf/fence: create fence_default_release() Gustavo Padovan <gustavo@padovan.org> - 2016-01-15 16:10 +0100
  Re: [RFC 00/29] De-stage android's sync framework Joe Perches <joe@perches.com> - 2016-01-15 20:20 +0100
  Re: [RFC 00/29] De-stage android's sync framework Daniel Vetter <daniel@ffwll.ch> - 2016-01-19 12:10 +0100
    Re: [RFC 00/29] De-stage android's sync framework Gustavo Padovan <gustavo@padovan.org> - 2016-01-19 16:30 +0100
      Re: [RFC 00/29] De-stage android's sync framework John Harrison <John.C.Harrison@Intel.com> - 2016-01-19 17:20 +0100
        Re: [RFC 00/29] De-stage android's sync framework Gustavo Padovan <gustavo@padovan.org> - 2016-01-19 19:00 +0100
          Re: [RFC 00/29] De-stage android's sync framework Daniel Vetter <daniel@ffwll.ch> - 2016-01-19 19:10 +0100
            Re: [RFC 00/29] De-stage android's sync framework Gustavo Padovan <gustavo@padovan.org> - 2016-01-19 19:20 +0100
    Re: [RFC 00/29] De-stage android's sync framework Gustavo Padovan <gustavo@padovan.org> - 2016-01-19 21:20 +0100
      Re: [RFC 00/29] De-stage android's sync framework Daniel Vetter <daniel@ffwll.ch> - 2016-01-19 21:40 +0100
  Re: [RFC 00/29] De-stage android's sync framework Maarten Lankhorst <maarten.lankhorst@linux.intel.com> - 2016-01-20 11:30 +0100
    Re: [RFC 00/29] De-stage android's sync framework Gustavo Padovan <gustavo@padovan.org> - 2016-01-20 15:40 +0100
      Re: [RFC 00/29] De-stage android's sync framework Maarten Lankhorst <maarten.lankhorst@linux.intel.com> - 2016-01-20 16:10 +0100
        Re: [RFC 00/29] De-stage android's sync framework Daniel Vetter <daniel@ffwll.ch> - 2016-01-20 17:30 +0100
        Re: [RFC 00/29] De-stage android's sync framework Gustavo Padovan <gustavo@padovan.org> - 2016-01-20 19:30 +0100

csiph-web