Re: [REPOST PATCH 4/8] android: convert sync to fence api, v5
On Mon, Jun 23, 2014 at 10:45 AM, Maarten Lankhorst maarten.lankho...@canonical.com wrote: But in drivers/drm I can encounter a similar issue, people expect to be able to overwrite the contents of the currently displayed buffer, so I 'solved' it by not adding a fence on the buffer, only by waiting for buffer idle before page flipping. The rationale is that the buffer is pinned internally, and the backing storage cannot go away until dma_buf_unmap_attachment is called. So when you render to the current front buffer without queuing a page flip you get exactly what you expect. ;-) Yeah, scanout buffers are special and imo we should only uses fences as barriers just around the flip, but _not_ to prevent frontbuffer in general. Userspace is after all allowed to do that (e.g. with the dumb bo ioctls). If we'd premanently lock scanout buffers that would indeed result in hilarity all over the place, no surprises there. And all current drivers with dynamic memory management solve this through pinning, but not by restricting write access at all. So I think we can shrug this scenario off as a non-issue of the don't do this kind ;-) Thierry, is there anything else you've stumbled over in the tegra k1 enabling work? I still get the impression that there's an awful lot of misunderstandings between the explicit and implicit syncing camps and that we need to do a lot more talking for a better understanding ... Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REPOST PATCH 4/8] android: convert sync to fence api, v5
Hey, op 20-06-14 22:52, Thierry Reding schreef: On Thu, Jun 19, 2014 at 02:28:14PM +0200, Daniel Vetter wrote: On Thu, Jun 19, 2014 at 1:48 PM, Thierry Reding thierry.red...@gmail.com wrote: With these changes, can we pull the android sync logic out of drivers/staging/ now? Afaik the google guys never really looked at this and acked it. So I'm not sure whether they'll follow along. The other issue I have as the maintainer of gfx driver is that I don't want to implement support for two different sync object primitives (once for dma-buf and once for android syncpts), and my impression thus far has been that even with this we're not there. I'm trying to get our own android guys to upstream their i915 syncpts support, but thus far I haven't managed to convince them to throw people's time at this. This has been discussed a fair bit internally recently and some of our GPU experts have raised concerns that this may result in seriously degraded performance in our proprietary graphics stack. Now I don't care very much for the proprietary graphics stack, but by extension I would assume that the same restrictions are relevant for any open-source driver as well. I'm still trying to fully understand all the implications and at the same time get some of the people who raised concerns to join in this discussion. As I understand it the concern is mostly about explicit vs. implicit synchronization and having this mechanism in the kernel will implicitly synchronize all accesses to these buffers even in cases where it's not needed (read vs. write locks, etc.). In one particular instance it was even mentioned that this kind of implicit synchronization can lead to deadlocks in some use-cases (this was mentioned for Android compositing, but I suspect that the same may happen for Wayland or X compositors). Well the implicit fences here actually can't deadlock. That's the entire point behind using ww mutexes. I've also heard tons of complaints about implicit enforced syncing (especially from opencl people), but in the end drivers and always expose unsynchronized access for specific cases. We do that in i915 for upload buffers and other fun stuff. This is about shared stuff across different drivers and different processes. Tegra K1 needs to share buffers across different drivers even for very basic use-cases since the GPU and display drivers are separate. So while I agree that the GPU driver can still use explicit synchronization for internal work, things aren't that simple in general. Let me try to reconstruct the use-case that caused the lock on Android: the compositor uses a hardware overlay to display an image. The system detects that there's little activity and instructs the compositor to put everything into one image and scan out only that (for power efficiency). Now with implicit locking the display driver has a lock on the image, so the GPU (used for compositing) needs to wait for it before it can composite everything into one image. But the display driver cannot release the lock on the image until the final image has been composited and can be displayed instead. This may not be technically a deadlock, but it's still a stalemate. Unless I'm missing something fundamental about DMA fences and ww mutexes I don't see how you can get out of this situation. This sounds like a case for implicit shared fences. ;-) Reading and scanning out would only wait for the last 'exclusive' fence, not on each other. But in drivers/drm I can encounter a similar issue, people expect to be able to overwrite the contents of the currently displayed buffer, so I 'solved' it by not adding a fence on the buffer, only by waiting for buffer idle before page flipping. The rationale is that the buffer is pinned internally, and the backing storage cannot go away until dma_buf_unmap_attachment is called. So when you render to the current front buffer without queuing a page flip you get exactly what you expect. ;-) Explicit vs. implicit synchronization may also become more of an issue as buffers are imported from other sources (such as cameras). Yeah, but the kernel space primitives would in both cases be the same, so drivers don't need to implement 2 separate fencing mechanisms for that. :-) ~Maarten -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REPOST PATCH 4/8] android: convert sync to fence api, v5
On Thu, Jun 19, 2014 at 02:28:14PM +0200, Daniel Vetter wrote: On Thu, Jun 19, 2014 at 1:48 PM, Thierry Reding thierry.red...@gmail.com wrote: With these changes, can we pull the android sync logic out of drivers/staging/ now? Afaik the google guys never really looked at this and acked it. So I'm not sure whether they'll follow along. The other issue I have as the maintainer of gfx driver is that I don't want to implement support for two different sync object primitives (once for dma-buf and once for android syncpts), and my impression thus far has been that even with this we're not there. I'm trying to get our own android guys to upstream their i915 syncpts support, but thus far I haven't managed to convince them to throw people's time at this. This has been discussed a fair bit internally recently and some of our GPU experts have raised concerns that this may result in seriously degraded performance in our proprietary graphics stack. Now I don't care very much for the proprietary graphics stack, but by extension I would assume that the same restrictions are relevant for any open-source driver as well. I'm still trying to fully understand all the implications and at the same time get some of the people who raised concerns to join in this discussion. As I understand it the concern is mostly about explicit vs. implicit synchronization and having this mechanism in the kernel will implicitly synchronize all accesses to these buffers even in cases where it's not needed (read vs. write locks, etc.). In one particular instance it was even mentioned that this kind of implicit synchronization can lead to deadlocks in some use-cases (this was mentioned for Android compositing, but I suspect that the same may happen for Wayland or X compositors). Well the implicit fences here actually can't deadlock. That's the entire point behind using ww mutexes. I've also heard tons of complaints about implicit enforced syncing (especially from opencl people), but in the end drivers and always expose unsynchronized access for specific cases. We do that in i915 for upload buffers and other fun stuff. This is about shared stuff across different drivers and different processes. Tegra K1 needs to share buffers across different drivers even for very basic use-cases since the GPU and display drivers are separate. So while I agree that the GPU driver can still use explicit synchronization for internal work, things aren't that simple in general. Let me try to reconstruct the use-case that caused the lock on Android: the compositor uses a hardware overlay to display an image. The system detects that there's little activity and instructs the compositor to put everything into one image and scan out only that (for power efficiency). Now with implicit locking the display driver has a lock on the image, so the GPU (used for compositing) needs to wait for it before it can composite everything into one image. But the display driver cannot release the lock on the image until the final image has been composited and can be displayed instead. This may not be technically a deadlock, but it's still a stalemate. Unless I'm missing something fundamental about DMA fences and ww mutexes I don't see how you can get out of this situation. Explicit vs. implicit synchronization may also become more of an issue as buffers are imported from other sources (such as cameras). Finally I've never seen anyone from google or any android product guy push a real driver enabling for syncpts to upstream, and google itself has a bit a history of constantly exchanging their gfx framework for the next best thing. So I really doubt this is worthwhile to pursue in upstream with our essentially eternal api guarantees. At least until we see serious uptake from vendors and gfx driver guys. Unfortunately the Intel android folks are no exception here and haven't pushed anything like this in my direction yet at all. Despite multiple pokes from my side. So from my side I think we should move ahead with Maarten's work and figure the android side out once there's real interest. The downside of that is that we may end up with two different ways to synchronize buffers if it turns out that we can't make Android (or other use-cases) work with DMA fence. However I don't think that justifies postponing this patch set indefinitely. Thierry pgp0OB8pDlJzP.pgp Description: PGP signature
Re: [REPOST PATCH 4/8] android: convert sync to fence api, v5
On Wed, Jun 18, 2014 at 06:15:56PM -0700, Greg KH wrote: On Wed, Jun 18, 2014 at 12:37:11PM +0200, Maarten Lankhorst wrote: Just to show it's easy. Android syncpoints can be mapped to a timeline. This removes the need to maintain a separate api for synchronization. I've left the android trace events in place, but the core fence events should already be sufficient for debugging. v2: - Call fence_remove_callback in sync_fence_free if not all fences have fired. v3: - Merge Colin Cross' bugfixes, and the android fence merge optimization. v4: - Merge with the upstream fixes. v5: - Fix small style issues pointed out by Thomas Hellstrom. Signed-off-by: Maarten Lankhorst maarten.lankho...@canonical.com Acked-by: John Stultz john.stu...@linaro.org --- drivers/staging/android/Kconfig |1 drivers/staging/android/Makefile |2 drivers/staging/android/sw_sync.c|6 drivers/staging/android/sync.c | 913 +++--- drivers/staging/android/sync.h | 79 ++- drivers/staging/android/sync_debug.c | 247 + drivers/staging/android/trace/sync.h | 12 7 files changed, 609 insertions(+), 651 deletions(-) create mode 100644 drivers/staging/android/sync_debug.c With these changes, can we pull the android sync logic out of drivers/staging/ now? Afaik the google guys never really looked at this and acked it. So I'm not sure whether they'll follow along. The other issue I have as the maintainer of gfx driver is that I don't want to implement support for two different sync object primitives (once for dma-buf and once for android syncpts), and my impression thus far has been that even with this we're not there. I'm trying to get our own android guys to upstream their i915 syncpts support, but thus far I haven't managed to convince them to throw people's time at this. It looks like a step into the right direction, but until I have the proof in the form of i915 patches that I won't have to support 2 gfx fencing frameworks I'm opposed to de-staging android syncpts. Ofc someone else could do that too, but besides i915 I don't see a full-fledged (modeset side only kinda doesn't count) upstream gfx driver shipping on android. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REPOST PATCH 4/8] android: convert sync to fence api, v5
On Thu, Jun 19, 2014 at 08:37:27AM +0200, Daniel Vetter wrote: On Wed, Jun 18, 2014 at 06:15:56PM -0700, Greg KH wrote: On Wed, Jun 18, 2014 at 12:37:11PM +0200, Maarten Lankhorst wrote: Just to show it's easy. Android syncpoints can be mapped to a timeline. This removes the need to maintain a separate api for synchronization. I've left the android trace events in place, but the core fence events should already be sufficient for debugging. v2: - Call fence_remove_callback in sync_fence_free if not all fences have fired. v3: - Merge Colin Cross' bugfixes, and the android fence merge optimization. v4: - Merge with the upstream fixes. v5: - Fix small style issues pointed out by Thomas Hellstrom. Signed-off-by: Maarten Lankhorst maarten.lankho...@canonical.com Acked-by: John Stultz john.stu...@linaro.org --- drivers/staging/android/Kconfig |1 drivers/staging/android/Makefile |2 drivers/staging/android/sw_sync.c|6 drivers/staging/android/sync.c | 913 +++--- drivers/staging/android/sync.h | 79 ++- drivers/staging/android/sync_debug.c | 247 + drivers/staging/android/trace/sync.h | 12 7 files changed, 609 insertions(+), 651 deletions(-) create mode 100644 drivers/staging/android/sync_debug.c With these changes, can we pull the android sync logic out of drivers/staging/ now? Afaik the google guys never really looked at this and acked it. So I'm not sure whether they'll follow along. The other issue I have as the maintainer of gfx driver is that I don't want to implement support for two different sync object primitives (once for dma-buf and once for android syncpts), and my impression thus far has been that even with this we're not there. I'm trying to get our own android guys to upstream their i915 syncpts support, but thus far I haven't managed to convince them to throw people's time at this. This has been discussed a fair bit internally recently and some of our GPU experts have raised concerns that this may result in seriously degraded performance in our proprietary graphics stack. Now I don't care very much for the proprietary graphics stack, but by extension I would assume that the same restrictions are relevant for any open-source driver as well. I'm still trying to fully understand all the implications and at the same time get some of the people who raised concerns to join in this discussion. As I understand it the concern is mostly about explicit vs. implicit synchronization and having this mechanism in the kernel will implicitly synchronize all accesses to these buffers even in cases where it's not needed (read vs. write locks, etc.). In one particular instance it was even mentioned that this kind of implicit synchronization can lead to deadlocks in some use-cases (this was mentioned for Android compositing, but I suspect that the same may happen for Wayland or X compositors). Thierry pgpJIsKfAjjEU.pgp Description: PGP signature
Re: [REPOST PATCH 4/8] android: convert sync to fence api, v5
On Thu, Jun 19, 2014 at 1:48 PM, Thierry Reding thierry.red...@gmail.com wrote: With these changes, can we pull the android sync logic out of drivers/staging/ now? Afaik the google guys never really looked at this and acked it. So I'm not sure whether they'll follow along. The other issue I have as the maintainer of gfx driver is that I don't want to implement support for two different sync object primitives (once for dma-buf and once for android syncpts), and my impression thus far has been that even with this we're not there. I'm trying to get our own android guys to upstream their i915 syncpts support, but thus far I haven't managed to convince them to throw people's time at this. This has been discussed a fair bit internally recently and some of our GPU experts have raised concerns that this may result in seriously degraded performance in our proprietary graphics stack. Now I don't care very much for the proprietary graphics stack, but by extension I would assume that the same restrictions are relevant for any open-source driver as well. I'm still trying to fully understand all the implications and at the same time get some of the people who raised concerns to join in this discussion. As I understand it the concern is mostly about explicit vs. implicit synchronization and having this mechanism in the kernel will implicitly synchronize all accesses to these buffers even in cases where it's not needed (read vs. write locks, etc.). In one particular instance it was even mentioned that this kind of implicit synchronization can lead to deadlocks in some use-cases (this was mentioned for Android compositing, but I suspect that the same may happen for Wayland or X compositors). Well the implicit fences here actually can't deadlock. That's the entire point behind using ww mutexes. I've also heard tons of complaints about implicit enforced syncing (especially from opencl people), but in the end drivers and always expose unsynchronized access for specific cases. We do that in i915 for upload buffers and other fun stuff. This is about shared stuff across different drivers and different processes. I also expect that i915 will loose implicit syncing in a few upcoming hw modes because explicit syncing is a more natural fit there. All that isn't about the discussion at hand imo since no matter what i915 needs to have on internal representation for a bit of gpu work, and afaics right now we don't have that. With this patch android syncpts use Maarten's fences internally, but I can't freely exchange one for the other. So in i915 I still expect to get stuck with both of them, which is one too many. The other issue (and I haven't dug into details that much) I have with syncpts are some of the interface choices. Apparently you can commit a fence after creation (or at least the hw composer interface works like that) which means userspace can construct deadlocks with android syncpts if I'm not super careful in my driver. I haven't seen any generic code to do that, so I presume everyone just blindly trusts surface-flinger to not do that. Speaks of the average quality of an android gfx driver if the kernel is less trusted than the compositor in userspace ... There's a few other things like exposing timestamps (which are tricky to do right, our driver is littered with wrong attempts) and other details. Finally I've never seen anyone from google or any android product guy push a real driver enabling for syncpts to upstream, and google itself has a bit a history of constantly exchanging their gfx framework for the next best thing. So I really doubt this is worthwhile to pursue in upstream with our essentially eternal api guarantees. At least until we see serious uptake from vendors and gfx driver guys. Unfortunately the Intel android folks are no exception here and haven't pushed anything like this in my direction yet at all. Despite multiple pokes from my side. So from my side I think we should move ahead with Maarten's work and figure the android side out once there's real interest. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REPOST PATCH 4/8] android: convert sync to fence api, v5
On Wed, Jun 18, 2014 at 11:37 PM, Daniel Vetter dan...@ffwll.ch wrote: On Wed, Jun 18, 2014 at 06:15:56PM -0700, Greg KH wrote: On Wed, Jun 18, 2014 at 12:37:11PM +0200, Maarten Lankhorst wrote: Just to show it's easy. Android syncpoints can be mapped to a timeline. This removes the need to maintain a separate api for synchronization. I've left the android trace events in place, but the core fence events should already be sufficient for debugging. v2: - Call fence_remove_callback in sync_fence_free if not all fences have fired. v3: - Merge Colin Cross' bugfixes, and the android fence merge optimization. v4: - Merge with the upstream fixes. v5: - Fix small style issues pointed out by Thomas Hellstrom. Signed-off-by: Maarten Lankhorst maarten.lankho...@canonical.com Acked-by: John Stultz john.stu...@linaro.org --- drivers/staging/android/Kconfig |1 drivers/staging/android/Makefile |2 drivers/staging/android/sw_sync.c|6 drivers/staging/android/sync.c | 913 +++--- drivers/staging/android/sync.h | 79 ++- drivers/staging/android/sync_debug.c | 247 + drivers/staging/android/trace/sync.h | 12 7 files changed, 609 insertions(+), 651 deletions(-) create mode 100644 drivers/staging/android/sync_debug.c With these changes, can we pull the android sync logic out of drivers/staging/ now? Afaik the google guys never really looked at this and acked it. So I'm not sure whether they'll follow along. The other issue I have as the maintainer of gfx driver is that I don't want to implement support for two different sync object primitives (once for dma-buf and once for android syncpts), and my impression thus far has been that even with this we're not there. We have tested these patches to use dma fences to back the android sync driver and not found any major issues. However, my understanding is that dma fences are designed for implicit sync, and explicit sync through the android sync driver is bolted on the side to share code. Android is not moving away from explicit sync, but we do wrap all of our userspace sync accesses through libsync (https://android.googlesource.com/platform/system/core/+/master/libsync/sync.c, ignore the sw_sync parts), so if the kernel supported a slightly different userspace explicit sync interface we could adapt to it fairly easily. All we require is that individual kernel drivers need to be able to accept work alongisde an fd to wait on, and to return an fd that will signal when the work is done, and that userspace has some way to merge two of those fds, wait on an fd, and get some debugging info from an fd. However, this patch set doesn't do that, it has no way to export a dma fence as an fd except through the android sync driver, so it is not yet ready to fully replace android sync. I'm trying to get our own android guys to upstream their i915 syncpts support, but thus far I haven't managed to convince them to throw people's time at this. It looks like a step into the right direction, but until I have the proof in the form of i915 patches that I won't have to support 2 gfx fencing frameworks I'm opposed to de-staging android syncpts. Ofc someone else could do that too, but besides i915 I don't see a full-fledged (modeset side only kinda doesn't count) upstream gfx driver shipping on android. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REPOST PATCH 4/8] android: convert sync to fence api, v5
On Thu, Jun 19, 2014 at 5:28 AM, Daniel Vetter dan...@ffwll.ch wrote: On Thu, Jun 19, 2014 at 1:48 PM, Thierry Reding thierry.red...@gmail.com wrote: With these changes, can we pull the android sync logic out of drivers/staging/ now? Afaik the google guys never really looked at this and acked it. So I'm not sure whether they'll follow along. The other issue I have as the maintainer of gfx driver is that I don't want to implement support for two different sync object primitives (once for dma-buf and once for android syncpts), and my impression thus far has been that even with this we're not there. I'm trying to get our own android guys to upstream their i915 syncpts support, but thus far I haven't managed to convince them to throw people's time at this. This has been discussed a fair bit internally recently and some of our GPU experts have raised concerns that this may result in seriously degraded performance in our proprietary graphics stack. Now I don't care very much for the proprietary graphics stack, but by extension I would assume that the same restrictions are relevant for any open-source driver as well. I'm still trying to fully understand all the implications and at the same time get some of the people who raised concerns to join in this discussion. As I understand it the concern is mostly about explicit vs. implicit synchronization and having this mechanism in the kernel will implicitly synchronize all accesses to these buffers even in cases where it's not needed (read vs. write locks, etc.). In one particular instance it was even mentioned that this kind of implicit synchronization can lead to deadlocks in some use-cases (this was mentioned for Android compositing, but I suspect that the same may happen for Wayland or X compositors). Well the implicit fences here actually can't deadlock. That's the entire point behind using ww mutexes. I've also heard tons of complaints about implicit enforced syncing (especially from opencl people), but in the end drivers and always expose unsynchronized access for specific cases. We do that in i915 for upload buffers and other fun stuff. This is about shared stuff across different drivers and different processes. I also expect that i915 will loose implicit syncing in a few upcoming hw modes because explicit syncing is a more natural fit there. All that isn't about the discussion at hand imo since no matter what i915 needs to have on internal representation for a bit of gpu work, and afaics right now we don't have that. With this patch android syncpts use Maarten's fences internally, but I can't freely exchange one for the other. So in i915 I still expect to get stuck with both of them, which is one too many. The other issue (and I haven't dug into details that much) I have with syncpts are some of the interface choices. Apparently you can commit a fence after creation (or at least the hw composer interface works like that) which means userspace can construct deadlocks with android syncpts if I'm not super careful in my driver. I haven't seen any generic code to do that, so I presume everyone just blindly trusts surface-flinger to not do that. Speaks of the average quality of an android gfx driver if the kernel is less trusted than the compositor in userspace ... Android sync is designed not to allow userspace to deadlock the kernel, a sync_pt should only get created by the kernel when it has received a chunk of work that it expects to complete in the near future. The CONFIG_SW_SYNC_USER driver violates that by allowing userspace to create and signal arbitrary sync points, but that is intended only for testing sync. There's a few other things like exposing timestamps (which are tricky to do right, our driver is littered with wrong attempts) and other details. Timestamps are necessary for vsync synchronization to reduce the frame latency. Finally I've never seen anyone from google or any android product guy push a real driver enabling for syncpts to upstream, and google itself has a bit a history of constantly exchanging their gfx framework for the next best thing. So I really doubt this is worthwhile to pursue in upstream with our essentially eternal api guarantees. At least until we see serious uptake from vendors and gfx driver guys. Unfortunately the Intel android folks are no exception here and haven't pushed anything like this in my direction yet at all. Despite multiple pokes from my side. As far as I know, every SoC vendor that supports android is using sync now, but none of them have succeeded in pushing their drivers upstream for a variety of other reasons (interfaces only used by closed source userspaces, KMS/DRM vs ADF, ION, etc.). So from my side I think we should move ahead with Maarten's work and figure the android side out once there's real interest. As long as that doesn't involve removing the Android sync interfaces from staging until dma fence fds are
Re: [REPOST PATCH 4/8] android: convert sync to fence api, v5
op 19-06-14 17:22, Colin Cross schreef:
On Wed, Jun 18, 2014 at 11:37 PM, Daniel Vetter dan...@ffwll.ch wrote:
On Wed, Jun 18, 2014 at 06:15:56PM -0700, Greg KH wrote:
On Wed, Jun 18, 2014 at 12:37:11PM +0200, Maarten Lankhorst wrote:
Just to show it's easy.
Android syncpoints can be mapped to a timeline. This removes the need
to maintain a separate api for synchronization. I've left the android
trace events in place, but the core fence events should already be
sufficient for debugging.
v2:
- Call fence_remove_callback in sync_fence_free if not all fences have
fired.
v3:
- Merge Colin Cross' bugfixes, and the android fence merge optimization.
v4:
- Merge with the upstream fixes.
v5:
- Fix small style issues pointed out by Thomas Hellstrom.
Signed-off-by: Maarten Lankhorst maarten.lankho...@canonical.com
Acked-by: John Stultz john.stu...@linaro.org
---
drivers/staging/android/Kconfig |1
drivers/staging/android/Makefile |2
drivers/staging/android/sw_sync.c|6
drivers/staging/android/sync.c | 913
+++---
drivers/staging/android/sync.h | 79 ++-
drivers/staging/android/sync_debug.c | 247 +
drivers/staging/android/trace/sync.h | 12
7 files changed, 609 insertions(+), 651 deletions(-)
create mode 100644 drivers/staging/android/sync_debug.c
With these changes, can we pull the android sync logic out of
drivers/staging/ now?
Afaik the google guys never really looked at this and acked it. So I'm not
sure whether they'll follow along. The other issue I have as the
maintainer of gfx driver is that I don't want to implement support for two
different sync object primitives (once for dma-buf and once for android
syncpts), and my impression thus far has been that even with this we're
not there.
We have tested these patches to use dma fences to back the android
sync driver and not found any major issues. However, my understanding
is that dma fences are designed for implicit sync, and explicit sync
through the android sync driver is bolted on the side to share code.
Android is not moving away from explicit sync, but we do wrap all of
our userspace sync accesses through libsync
(https://android.googlesource.com/platform/system/core/+/master/libsync/sync.c,
ignore the sw_sync parts), so if the kernel supported a slightly
different userspace explicit sync interface we could adapt to it
fairly easily. All we require is that individual kernel drivers need
to be able to accept work alongisde an fd to wait on, and to return an
fd that will signal when the work is done, and that userspace has some
way to merge two of those fds, wait on an fd, and get some debugging
info from an fd. However, this patch set doesn't do that, it has no
way to export a dma fence as an fd except through the android sync
driver, so it is not yet ready to fully replace android sync.
Dma fences can be exported as android fences, just didn't see a need for it
yet. :-)
To wait on all implicit fences attached to a dma-buf one could simply poll the
dma-buf directly,
or use something like a android userspace fence.
sync_fence_create takes a sync_pt as function argument, but I kept that to keep
source code
compatibility, not because it uses any sync_pt functions. Here's a patch to
create a userspace
fd for dma-fence instead of a android fence, applied on top of android:
convert sync to fence api.
diff --git a/drivers/staging/android/sw_sync.c
b/drivers/staging/android/sw_sync.c
index a76db3ff87cb..afc3c63b0438 100644
--- a/drivers/staging/android/sw_sync.c
+++ b/drivers/staging/android/sw_sync.c
@@ -184,7 +184,7 @@ static long sw_sync_ioctl_create_fence(struct
sw_sync_timeline *obj,
}
data.name[sizeof(data.name) - 1] = '\0';
- fence = sync_fence_create(data.name, pt);
+ fence = sync_fence_create(data.name, pt-base);
if (fence == NULL) {
sync_pt_free(pt);
err = -ENOMEM;
diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c
index 70b09b5001ba..c89a6f954e41 100644
--- a/drivers/staging/android/sync.c
+++ b/drivers/staging/android/sync.c
@@ -188,7 +188,7 @@ static void fence_check_cb_func(struct fence *f, struct
fence_cb *cb)
}
/* TODO: implement a create which takes more that one sync_pt */
-struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt)
+struct sync_fence *sync_fence_create(const char *name, struct fence *pt)
{
struct sync_fence *fence;
@@ -199,10 +199,10 @@ struct sync_fence *sync_fence_create(const char *name,
struct sync_pt *pt)
fence-num_fences = 1;
atomic_set(fence-status, 1);
- fence_get(pt-base);
- fence-cbs[0].sync_pt = pt-base;
+ fence_get(pt);
+ fence-cbs[0].sync_pt = pt;
fence-cbs[0].fence = fence;
- if (fence_add_callback(pt-base, fence-cbs[0].cb,
+ if (fence_add_callback(pt, fence-cbs[0].cb,
[REPOST PATCH 4/8] android: convert sync to fence api, v5
Just to show it's easy.
Android syncpoints can be mapped to a timeline. This removes the need
to maintain a separate api for synchronization. I've left the android
trace events in place, but the core fence events should already be
sufficient for debugging.
v2:
- Call fence_remove_callback in sync_fence_free if not all fences have fired.
v3:
- Merge Colin Cross' bugfixes, and the android fence merge optimization.
v4:
- Merge with the upstream fixes.
v5:
- Fix small style issues pointed out by Thomas Hellstrom.
Signed-off-by: Maarten Lankhorst maarten.lankho...@canonical.com
Acked-by: John Stultz john.stu...@linaro.org
---
drivers/staging/android/Kconfig |1
drivers/staging/android/Makefile |2
drivers/staging/android/sw_sync.c|6
drivers/staging/android/sync.c | 913 +++---
drivers/staging/android/sync.h | 79 ++-
drivers/staging/android/sync_debug.c | 247 +
drivers/staging/android/trace/sync.h | 12
7 files changed, 609 insertions(+), 651 deletions(-)
create mode 100644 drivers/staging/android/sync_debug.c
diff --git a/drivers/staging/android/Kconfig b/drivers/staging/android/Kconfig
index 99e484f845f2..51607e9aa049 100644
--- a/drivers/staging/android/Kconfig
+++ b/drivers/staging/android/Kconfig
@@ -88,6 +88,7 @@ config SYNC
bool Synchronization framework
default n
select ANON_INODES
+ select DMA_SHARED_BUFFER
---help---
This option enables the framework for synchronization between multiple
drivers. Sync implementations can take advantage of hardware
diff --git a/drivers/staging/android/Makefile b/drivers/staging/android/Makefile
index 0a01e1914905..517ad5ffa429 100644
--- a/drivers/staging/android/Makefile
+++ b/drivers/staging/android/Makefile
@@ -9,5 +9,5 @@ obj-$(CONFIG_ANDROID_TIMED_OUTPUT) += timed_output.o
obj-$(CONFIG_ANDROID_TIMED_GPIO) += timed_gpio.o
obj-$(CONFIG_ANDROID_LOW_MEMORY_KILLER)+= lowmemorykiller.o
obj-$(CONFIG_ANDROID_INTF_ALARM_DEV) += alarm-dev.o
-obj-$(CONFIG_SYNC) += sync.o
+obj-$(CONFIG_SYNC) += sync.o sync_debug.o
obj-$(CONFIG_SW_SYNC) += sw_sync.o
diff --git a/drivers/staging/android/sw_sync.c
b/drivers/staging/android/sw_sync.c
index 12a136ec1cec..a76db3ff87cb 100644
--- a/drivers/staging/android/sw_sync.c
+++ b/drivers/staging/android/sw_sync.c
@@ -50,7 +50,7 @@ static struct sync_pt *sw_sync_pt_dup(struct sync_pt *sync_pt)
{
struct sw_sync_pt *pt = (struct sw_sync_pt *) sync_pt;
struct sw_sync_timeline *obj =
- (struct sw_sync_timeline *)sync_pt-parent;
+ (struct sw_sync_timeline *)sync_pt_parent(sync_pt);
return (struct sync_pt *) sw_sync_pt_create(obj, pt-value);
}
@@ -59,7 +59,7 @@ static int sw_sync_pt_has_signaled(struct sync_pt *sync_pt)
{
struct sw_sync_pt *pt = (struct sw_sync_pt *)sync_pt;
struct sw_sync_timeline *obj =
- (struct sw_sync_timeline *)sync_pt-parent;
+ (struct sw_sync_timeline *)sync_pt_parent(sync_pt);
return sw_sync_cmp(obj-value, pt-value) = 0;
}
@@ -97,7 +97,6 @@ static void sw_sync_pt_value_str(struct sync_pt *sync_pt,
char *str, int size)
{
struct sw_sync_pt *pt = (struct sw_sync_pt *)sync_pt;
-
snprintf(str, size, %d, pt-value);
}
@@ -157,7 +156,6 @@ static int sw_sync_open(struct inode *inode, struct file
*file)
static int sw_sync_release(struct inode *inode, struct file *file)
{
struct sw_sync_timeline *obj = file-private_data;
-
sync_timeline_destroy(obj-obj);
return 0;
}
diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c
index 18174f7c871c..70b09b5001ba 100644
--- a/drivers/staging/android/sync.c
+++ b/drivers/staging/android/sync.c
@@ -31,22 +31,13 @@
#define CREATE_TRACE_POINTS
#include trace/sync.h
-static void sync_fence_signal_pt(struct sync_pt *pt);
-static int _sync_pt_has_signaled(struct sync_pt *pt);
-static void sync_fence_free(struct kref *kref);
-static void sync_dump(void);
-
-static LIST_HEAD(sync_timeline_list_head);
-static DEFINE_SPINLOCK(sync_timeline_list_lock);
-
-static LIST_HEAD(sync_fence_list_head);
-static DEFINE_SPINLOCK(sync_fence_list_lock);
+static const struct fence_ops android_fence_ops;
+static const struct file_operations sync_fence_fops;
struct sync_timeline *sync_timeline_create(const struct sync_timeline_ops *ops,
int size, const char *name)
{
struct sync_timeline *obj;
- unsigned long flags;
if (size sizeof(struct sync_timeline))
return NULL;
@@ -57,17 +48,14 @@ struct sync_timeline *sync_timeline_create(const struct
sync_timeline_ops *ops,
kref_init(obj-kref);
obj-ops = ops;
+ obj-context = fence_context_alloc(1);
Re: [REPOST PATCH 4/8] android: convert sync to fence api, v5
On Wed, Jun 18, 2014 at 12:37:11PM +0200, Maarten Lankhorst wrote: Just to show it's easy. Android syncpoints can be mapped to a timeline. This removes the need to maintain a separate api for synchronization. I've left the android trace events in place, but the core fence events should already be sufficient for debugging. v2: - Call fence_remove_callback in sync_fence_free if not all fences have fired. v3: - Merge Colin Cross' bugfixes, and the android fence merge optimization. v4: - Merge with the upstream fixes. v5: - Fix small style issues pointed out by Thomas Hellstrom. Signed-off-by: Maarten Lankhorst maarten.lankho...@canonical.com Acked-by: John Stultz john.stu...@linaro.org --- drivers/staging/android/Kconfig |1 drivers/staging/android/Makefile |2 drivers/staging/android/sw_sync.c|6 drivers/staging/android/sync.c | 913 +++--- drivers/staging/android/sync.h | 79 ++- drivers/staging/android/sync_debug.c | 247 + drivers/staging/android/trace/sync.h | 12 7 files changed, 609 insertions(+), 651 deletions(-) create mode 100644 drivers/staging/android/sync_debug.c With these changes, can we pull the android sync logic out of drivers/staging/ now? thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
