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

[Daniel Vetter]

On Tue, Jan 19, 2016 at 03:52:26PM -0200, Gustavo Padovan wrote:
> 2016-01-19 John Harrison <John.C.Harrison@xxxxxxxxx>:
> 
> > 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.
> 
> 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).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch