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

[Gustavo Padovan <gustavo@padovan.org>]

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

> On Tue, Jan 19, 2016 at 03:52:26PM -0200, Gustavo Padovan wrote:
> > 2016-01-19 John Harrison <John.C.Harrison@Intel.com>:
> > 
> > > On 19/01/2016 15:23, Gustavo Padovan wrote:
> > > >Hi Daniel,
> > > >
> > > >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.
> > > >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.
> > 
> > I agree with you, we should allow IGT and other test tools to access
> > sw_sync. include/linux/sw_sync.h can be kept private, but the uapi one
> > needs wil be needed for testing, unless we replicate the header file
> > inside IGT, but not sure if it is a good idea.
> 
> We replicate all the debugfs stuff in igt that igt needs. uapi really only
> should be stuff that's guaranteed to stick around, not debug interfaces we
> are ok with breaking (if needed).

Okay, that sounds quite good for me.

	Gustavo