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