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

[John Harrison]

On 19/01/2016 15:23, Gustavo Padovan wrote:
> Hi Daniel,
>
> 2016-01-19 Daniel Vetter <daniel@xxxxxxxx>:
>
> > On Fri, Jan 15, 2016 at 12:55:10PM -0200, Gustavo Padovan wrote:
> > > From: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
> > >
> > > 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.
> These two items sounds reasonable to me.

I have just posted our in-progress IGT for testing i915 syncs (with a CC 
of Gustavo). It uses the sw_sync mechanisms. Can you take a quick look 
and see if it is the kind of thing you would expect us to be doing? Or 
is it using interfaces that you are planning to remove and/or make 
kernel only?

I'm not sure having a kernel only test is the best way to go. Having 
user land tests like IGT would be much more versatile.


> > - 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.
> Yes, they are. But I currently doesn't know how to merge them best, so I
> decided to go for a RFC instead of trying some crazy solution touching
> all fence_context users.
>
> > 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 ...).
> I'd say we just give up for now as we don't have any driver using
> timeline_destroy for now. So we could go for other improvements first.
>
> > - 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.
> Agreed.
>
> > - 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.
> Yes, most of the sync_fence waiting should not be exported. Drivers
> should only wait for fence imo, not sync_fences.
>
> >    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.
> >
> > - struct sync_fence is a major confusion imo against struct fence. It
> >    made much more sense in the pure-android world where fence == sync_pt.
> >    Maybe we can rename sync_fence to sync_fence_fd (a bit long, and fd is a
> >    bit inaccurate), sync_file (like this best), fence_file (sounds silly
> >    imo), or something else?
> sync_file sounds good for me. fence_file feels like it a file for a
> single fence but we may have many fences on one sync_file.
>
> > - I guess just not yet part of this rfc, but moving the testsuite and
> >    adding kerneldoc for this is planned I guess? If you feel like I think
> >    it'd be best. We pull the current dma-buf stuff into
> >    device-drivers.tmpl, but it's completely lacking overview docs and all
> >    that. And I'd like to duplicate at least the dma-buf/fence sections into
> >    the gpu.tmpl docbook.
> We have converted testsuite from android's libsync but we need to wait
> for Google to re-license it to send it upstream.
>
> kerneldoc is planned for sure, but I'd say it will be better to have
> some users first, DRM for example.
>
> > - If we make timelines first class objects I think we could move some of
> >    the fields from struct fence to struct fence_timeline. E.g. the ops
> >    struct. That also makes it clearer that some of the vfuncs really should
> >    be taking a struct fence_timeline *timeline instead of a struct fence
> >    *fence as their primary parameter.
> I'll keep that as a final goal and work RFC v2 and see how far we can
> get.
>
> 	Gustavo