Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1310196 > unrolled thread
| Started by | Gustavo Padovan <gustavo@padovan.org> |
|---|---|
| First post | 2016-01-15 16:00 +0100 |
| Last post | 2016-01-20 19:30 +0100 |
| Articles | 9 on this page of 29 — 7 participants |
Back to article view | Back to linux.kernel
[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
Page 2 of 2 — ← Prev page 1 [2]
| From | Daniel Vetter <daniel@ffwll.ch> |
|---|---|
| Date | 2016-01-19 19:10 +0100 |
| Message-ID | <qSIT9-33H-31@gated-at.bofh.it> |
| In reply to | #1312408 |
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). -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[toc] | [prev] | [next] | [standalone]
| From | Gustavo Padovan <gustavo@padovan.org> |
|---|---|
| Date | 2016-01-19 19:20 +0100 |
| Message-ID | <qSJ2O-36W-13@gated-at.bofh.it> |
| In reply to | #1312416 |
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
[toc] | [prev] | [next] | [standalone]
| From | Gustavo Padovan <gustavo@padovan.org> |
|---|---|
| Date | 2016-01-19 21:20 +0100 |
| Message-ID | <qSKUX-4lv-27@gated-at.bofh.it> |
| In reply to | #1312122 |
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
[toc] | [prev] | [next] | [standalone]
| From | Daniel Vetter <daniel@ffwll.ch> |
|---|---|
| Date | 2016-01-19 21:40 +0100 |
| Message-ID | <qSLej-4ue-25@gated-at.bofh.it> |
| In reply to | #1312485 |
On Tue, Jan 19, 2016 at 06:10:40PM -0200, Gustavo Padovan wrote: > 2016-01-19 Daniel Vetter <daniel@ffwll.ch>: > > - 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. Makes sense. And yeah I only realized after sending that this wasn't added by your patches, just that your patches added the (core) users for it. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[toc] | [prev] | [next] | [standalone]
| From | Maarten Lankhorst <maarten.lankhorst@linux.intel.com> |
|---|---|
| Date | 2016-01-20 11:30 +0100 |
| Message-ID | <qSYbw-51L-9@gated-at.bofh.it> |
| In reply to | #1310196 |
Hey, Op 15-01-16 om 15:55 schreef Gustavo Padovan: > 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 > > Gustavo Padovan (29): > staging/android: fix sync framework documentation > staging/android: fix checkpatch warning > staging/android: rename sync_fence_release > staging/android: rename 'android_fence' to 'sync_fence' > staging/android: remove not used sync_timeline ops > staging/android: create a 'sync' dir for debugfs information > staging/android: move sw_sync file to debugfs file > staging/android: Remove WARN_ON_ONCE when releasing sync_fence > staging/android: rename struct sync_fence's variables to 'sync_fence' > staging/android: rename 'sync_pt' to 'fence' in struct sync_fence_cb > dma-buf/fence: move sync_timeline to fence_timeline > staging/android: remove struct sync_pt > dma-buf/fence: create fence_default_enable_signaling() > dma-buf/fence: create fence_default_release() > dma-buf/fence: create fence_default_get_driver_name() > dma-buf/fence: create fence_default_timeline_name() This is misleading. I think timeline_fence prefix would be more appropriate here. I also believe this should be done in multiple series. First series should de-stage the userspace fence framework. The next series should fix up android_fence and maybe rename it to timeline_fence since sync_fence is already used for the userspace fd, which would add more confusion? > dma-buf/fence: store last signaled value on fence timeline > dma-buf/fence: create default .fence_value_str() and > .timeline_value_str() > dma-buf/fence: create fence_default_fill_driver_data() > dma-buf/fence: remove fence_timeline_ops > dma-buf/fence: add fence_create_on_timeline() > staging/android: remove sync_pt_create() > staging/android: remove sw_sync_timeline and sw_sync_pt > dma-buf/fence: add debug to fence timeline > dma-buf/fence: remove unused var from fence_timeline_signal() > dma-buf/fence: remove pointless fence_timeline_signal at destroy phase > dma-buf/fence: add .cleanup() callback > staging/android: use .cleanup() to interrupt any sync_fence waiter > dma-buf/fence: de-stage sync framework >
[toc] | [prev] | [next] | [standalone]
| From | Gustavo Padovan <gustavo@padovan.org> |
|---|---|
| Date | 2016-01-20 15:40 +0100 |
| Message-ID | <qT25t-7Pe-35@gated-at.bofh.it> |
| In reply to | #1313084 |
2016-01-20 Maarten Lankhorst <maarten.lankhorst@linux.intel.com>: > Hey, > > Op 15-01-16 om 15:55 schreef Gustavo Padovan: > > 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 > > > > Gustavo Padovan (29): > > staging/android: fix sync framework documentation > > staging/android: fix checkpatch warning > > staging/android: rename sync_fence_release > > staging/android: rename 'android_fence' to 'sync_fence' > > staging/android: remove not used sync_timeline ops > > staging/android: create a 'sync' dir for debugfs information > > staging/android: move sw_sync file to debugfs file > > staging/android: Remove WARN_ON_ONCE when releasing sync_fence > > staging/android: rename struct sync_fence's variables to 'sync_fence' > > staging/android: rename 'sync_pt' to 'fence' in struct sync_fence_cb > > dma-buf/fence: move sync_timeline to fence_timeline > > staging/android: remove struct sync_pt > > dma-buf/fence: create fence_default_enable_signaling() > > dma-buf/fence: create fence_default_release() > > dma-buf/fence: create fence_default_get_driver_name() > > dma-buf/fence: create fence_default_timeline_name() > This is misleading. I think timeline_fence prefix would be more appropriate here. Why? These fence_default_.. functions are fence_ops and not related to fence_timeline in any way. > > I also believe this should be done in multiple series. First series should de-stage the userspace fence framework. The next series should fix up android_fence and maybe rename it to timeline_fence since sync_fence is already used for the userspace fd, which would add more confusion? Sure. I've been thinking on how to split this properly. I'm trying to add a bunch of clean up/renaming first, eg the sync_fence rename to sync_file that Daniel Vetter and I discussed. Next my plan would be move sync_timeline to fence_timeline, add the fence_default.. fence_ops, clean up sw_sync and finally merge fence_context and fence_timeline. Looking at how sync and fence It looks easier to me to de-stage sync_timeline first than userspace fence. Gustavo
[toc] | [prev] | [next] | [standalone]
| From | Maarten Lankhorst <maarten.lankhorst@linux.intel.com> |
|---|---|
| Date | 2016-01-20 16:10 +0100 |
| Message-ID | <qT2yt-8hL-3@gated-at.bofh.it> |
| In reply to | #1313259 |
Op 20-01-16 om 15:32 schreef Gustavo Padovan: > 2016-01-20 Maarten Lankhorst <maarten.lankhorst@linux.intel.com>: > >> Hey, >> >> Op 15-01-16 om 15:55 schreef Gustavo Padovan: >>> 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 >>> >>> Gustavo Padovan (29): >>> staging/android: fix sync framework documentation >>> staging/android: fix checkpatch warning >>> staging/android: rename sync_fence_release >>> staging/android: rename 'android_fence' to 'sync_fence' >>> staging/android: remove not used sync_timeline ops >>> staging/android: create a 'sync' dir for debugfs information >>> staging/android: move sw_sync file to debugfs file >>> staging/android: Remove WARN_ON_ONCE when releasing sync_fence >>> staging/android: rename struct sync_fence's variables to 'sync_fence' >>> staging/android: rename 'sync_pt' to 'fence' in struct sync_fence_cb >>> dma-buf/fence: move sync_timeline to fence_timeline >>> staging/android: remove struct sync_pt >>> dma-buf/fence: create fence_default_enable_signaling() >>> dma-buf/fence: create fence_default_release() >>> dma-buf/fence: create fence_default_get_driver_name() >>> dma-buf/fence: create fence_default_timeline_name() >> This is misleading. I think timeline_fence prefix would be more appropriate here. > Why? These fence_default_.. functions are fence_ops and not related to > fence_timeline in any way. Because they're using fence_parent, which should probably be renamed to fence_to_timeline() The name makes it sound as if they could apply to all type of fences, I don't think this is the case. >> I also believe this should be done in multiple series. First series should de-stage the userspace fence framework. The next series should fix up android_fence and maybe rename it to timeline_fence since sync_fence is already used for the userspace fd, which would add more confusion? > Sure. I've been thinking on how to split this properly. I'm trying to > add a bunch of clean up/renaming first, eg the sync_fence rename to > sync_file that Daniel Vetter and I discussed. > > Next my plan would be move sync_timeline to fence_timeline, add the > fence_default.. fence_ops, clean up sw_sync and finally merge > fence_context and fence_timeline. > > Looking at how sync and fence It looks easier to me to de-stage sync_timeline first than userspace > fence. There's already code to add a sync_fence_create_dma export [1][2]. So if you want to de-stage it then there will be users for it. sync_pt otoh has no upstream in-kernel users. It was a wrapper to keep android drivers api compatible with the fence api. [1] https://patchwork.freedesktop.org/patch/67845/ [2] https://patchwork.freedesktop.org/patch/67846/
[toc] | [prev] | [next] | [standalone]
| From | Daniel Vetter <daniel@ffwll.ch> |
|---|---|
| Date | 2016-01-20 17:30 +0100 |
| Message-ID | <qT3NU-CC-17@gated-at.bofh.it> |
| In reply to | #1313274 |
On Wed, Jan 20, 2016 at 04:02:31PM +0100, Maarten Lankhorst wrote: > Op 20-01-16 om 15:32 schreef Gustavo Padovan: > > 2016-01-20 Maarten Lankhorst <maarten.lankhorst@linux.intel.com>: > > > >> Hey, > >> > >> Op 15-01-16 om 15:55 schreef Gustavo Padovan: > >>> 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 > >>> > >>> Gustavo Padovan (29): > >>> staging/android: fix sync framework documentation > >>> staging/android: fix checkpatch warning > >>> staging/android: rename sync_fence_release > >>> staging/android: rename 'android_fence' to 'sync_fence' > >>> staging/android: remove not used sync_timeline ops > >>> staging/android: create a 'sync' dir for debugfs information > >>> staging/android: move sw_sync file to debugfs file > >>> staging/android: Remove WARN_ON_ONCE when releasing sync_fence > >>> staging/android: rename struct sync_fence's variables to 'sync_fence' > >>> staging/android: rename 'sync_pt' to 'fence' in struct sync_fence_cb > >>> dma-buf/fence: move sync_timeline to fence_timeline > >>> staging/android: remove struct sync_pt > >>> dma-buf/fence: create fence_default_enable_signaling() > >>> dma-buf/fence: create fence_default_release() > >>> dma-buf/fence: create fence_default_get_driver_name() > >>> dma-buf/fence: create fence_default_timeline_name() > >> This is misleading. I think timeline_fence prefix would be more appropriate here. > > Why? These fence_default_.. functions are fence_ops and not related to > > fence_timeline in any way. > Because they're using fence_parent, which should probably be renamed to fence_to_timeline() > > The name makes it sound as if they could apply to all type of fences, I don't think this is the case. > >> I also believe this should be done in multiple series. First series should de-stage the userspace fence framework. The next series should fix up android_fence and maybe rename it to timeline_fence since sync_fence is already used for the userspace fd, which would add more confusion? > > Sure. I've been thinking on how to split this properly. I'm trying to > > add a bunch of clean up/renaming first, eg the sync_fence rename to > > sync_file that Daniel Vetter and I discussed. > > > > Next my plan would be move sync_timeline to fence_timeline, add the > > fence_default.. fence_ops, clean up sw_sync and finally merge > > fence_context and fence_timeline. > > > > Looking at how sync and fence It looks easier to me to de-stage sync_timeline first than userspace > > fence. > There's already code to add a sync_fence_create_dma export [1][2]. So if you want to de-stage it then there will be users for it. > > sync_pt otoh has no upstream in-kernel users. It was a wrapper to keep android drivers api compatible with the fence api. > > [1] https://patchwork.freedesktop.org/patch/67845/ > [2] https://patchwork.freedesktop.org/patch/67846/ We kinda don't have open-source userspace for this stuf though ... -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[toc] | [prev] | [next] | [standalone]
| From | Gustavo Padovan <gustavo@padovan.org> |
|---|---|
| Date | 2016-01-20 19:30 +0100 |
| Message-ID | <qT5G2-1Wb-17@gated-at.bofh.it> |
| In reply to | #1313274 |
2016-01-20 Maarten Lankhorst <maarten.lankhorst@linux.intel.com>: > Op 20-01-16 om 15:32 schreef Gustavo Padovan: > > 2016-01-20 Maarten Lankhorst <maarten.lankhorst@linux.intel.com>: > > > >> Hey, > >> > >> Op 15-01-16 om 15:55 schreef Gustavo Padovan: > >>> 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 > >>> > >>> Gustavo Padovan (29): > >>> staging/android: fix sync framework documentation > >>> staging/android: fix checkpatch warning > >>> staging/android: rename sync_fence_release > >>> staging/android: rename 'android_fence' to 'sync_fence' > >>> staging/android: remove not used sync_timeline ops > >>> staging/android: create a 'sync' dir for debugfs information > >>> staging/android: move sw_sync file to debugfs file > >>> staging/android: Remove WARN_ON_ONCE when releasing sync_fence > >>> staging/android: rename struct sync_fence's variables to 'sync_fence' > >>> staging/android: rename 'sync_pt' to 'fence' in struct sync_fence_cb > >>> dma-buf/fence: move sync_timeline to fence_timeline > >>> staging/android: remove struct sync_pt > >>> dma-buf/fence: create fence_default_enable_signaling() > >>> dma-buf/fence: create fence_default_release() > >>> dma-buf/fence: create fence_default_get_driver_name() > >>> dma-buf/fence: create fence_default_timeline_name() > >> This is misleading. I think timeline_fence prefix would be more appropriate here. > > Why? These fence_default_.. functions are fence_ops and not related to > > fence_timeline in any way. > Because they're using fence_parent, which should probably be renamed to fence_to_timeline() > > The name makes it sound as if they could apply to all type of fences, I don't think this is the case. Fair enough. I can change the naming. Also the final idea is to move fence_ops to be fence_timeline_ops so the naming makes sense. > >> I also believe this should be done in multiple series. First series should de-stage the userspace fence framework. The next series should fix up android_fence and maybe rename it to timeline_fence since sync_fence is already used for the userspace fd, which would add more confusion? > > Sure. I've been thinking on how to split this properly. I'm trying to > > add a bunch of clean up/renaming first, eg the sync_fence rename to > > sync_file that Daniel Vetter and I discussed. > > > > Next my plan would be move sync_timeline to fence_timeline, add the > > fence_default.. fence_ops, clean up sw_sync and finally merge > > fence_context and fence_timeline. > > > > Looking at how sync and fence It looks easier to me to de-stage sync_timeline first than userspace > > fence. > There's already code to add a sync_fence_create_dma export [1][2]. So if you want to de-stage it then there will be users for it. > > sync_pt otoh has no upstream in-kernel users. It was a wrapper to keep android drivers api compatible with the fence api. One of the first patches of my series is removing struct sync_pt, that means I am also removing sync_fence_create_dma() because sync_fence_create() now accepts struct fence as parameter. I don't sync_pt being useful for any in-kernel driver as it is just a wrapper on top of struct fence. Gustavo
[toc] | [prev] | [standalone]
Page 2 of 2 — ← Prev page 1 [2]
Back to top | Article view | linux.kernel
csiph-web