exo: Add explicit synchronization support to motion event client.

exo: Add explicit synchronization support to motion event client.

This allows the motion event client to use native acquire fences
and explicit synchronization when submitting frames to the
compositor.

BUG=653908

[reveman, 2016-11-14 00:42:23]

reveman@chromium.org changed reviewers:
+ daniel@fooishbar.org, dcastagna@chromium.org

[Daniele Castagna, 2016-11-14 04:08:53]

What was the problem you were observing with DupNativeFence and how did you
solve it?

Do you mind being a little bit more descriptive in the description of the CL. In
particular, this adds support only for the acquire fence.
Also, at this point crbug.com/661010 is not really related to this. Is there a
bug about explicit sync in exo (crbug.com/653908 is related.)

https://codereview.chromium.org/2497173002/diff/1/components/exo/wayland/clie...
File components/exo/wayland/clients/motion_events.cc (right):

https://codereview.chromium.org/2497173002/diff/1/components/exo/wayland/clie...
components/exo/wayland/clients/motion_events.cc:402: if
(use_explicit_synchronization_ &&
Maybe we should also check that if use-explicit-synchronization is passed
use-drm should be passed too.

https://codereview.chromium.org/2497173002/diff/1/components/exo/wayland/clie...
components/exo/wayland/clients/motion_events.cc:470: if
(gl::g_driver_egl.ext.b_EGL_ANDROID_native_fence_sync)
Why did you change this?
I'd keep it coherent with the other two extension checks.

https://codereview.chromium.org/2497173002/diff/1/components/exo/wayland/clie...
components/exo/wayland/clients/motion_events.cc:506:
std::unique_ptr<zcr_synchronization_v1> synchronization;
surface_synchronization might be better?

https://codereview.chromium.org/2497173002/diff/1/components/exo/wayland/clie...
components/exo/wayland/clients/motion_events.cc:515: if
(!gl::g_driver_egl.ext.b_EGL_ANDROID_native_fence_sync) {
You could check egl_sync_type != EGL_SYNC_NATIVE_FENCE_ANDROID instead. (You'd
need to #ifdef this though.)

https://codereview.chromium.org/2497173002/diff/1/components/exo/wayland/clie...
components/exo/wayland/clients/motion_events.cc:660: DCHECK(buffer->egl_sync);
DCHECK(buffer->egl_sync->is_valid());

https://codereview.chromium.org/2497173002/diff/1/components/exo/wayland/clie...
components/exo/wayland/clients/motion_events.cc:664: glFlush();
Why do we need this flush at all?

https://codereview.chromium.org/2497173002/diff/1/components/exo/wayland/clie...
components/exo/wayland/clients/motion_events.cc:685: if (buffer->egl_sync) {
Could this be something like:

if (use_explicit_synchronization_) {
  DCHECK(synchronization);
  DCHECK_EQ(egl_sync_type, ...ANDROID);
  ...
} else if (buffer->egl_sync) {
  eglClientWaitSyncKHR(...)
}

In this way you avoid the inner if and you avoid mixing assumptions with actual
logic.

https://codereview.chromium.org/2497173002/diff/1/components/exo/wayland/clie...
components/exo/wayland/clients/motion_events.cc:686: if (synchronization) {
I'd still check use_explicit_synchronization_ here, it'd make this piece of code
easier to read without context.

[reveman, 2016-11-14 16:30:09]

Description was changed from

==========
exo: Add explicit synchronization support to motion event client.

BUG=661010
==========

to

==========
exo: Add explicit synchronization support to motion event client.

This allows the motion event client to use native acquire fences
and explicit synchronization when submitting frames to the
compositor.

BUG=653908
==========

[reveman, 2016-11-14 16:32:10]

On 2016/11/14 at 04:08:53, dcastagna wrote:
> What was the problem you were observing with DupNativeFence and how did you
solve it?

Just a smart pointer typo. EGLSyncKHR being typedef-ed to void* makes it easy to
pass the wrong thing without noticing.

> 
> Do you mind being a little bit more descriptive in the description of the CL.
In particular, this adds support only for the acquire fence.

Done.

> Also, at this point crbug.com/661010 is not really related to this. Is there a
bug about explicit sync in exo (crbug.com/653908 is related.)

Done.

https://codereview.chromium.org/2497173002/diff/1/components/exo/wayland/clie...
File components/exo/wayland/clients/motion_events.cc (right):

https://codereview.chromium.org/2497173002/diff/1/components/exo/wayland/clie...
components/exo/wayland/clients/motion_events.cc:402: if
(use_explicit_synchronization_ &&
On 2016/11/14 at 04:08:52, Daniele Castagna wrote:
> Maybe we should also check that if use-explicit-synchronization is passed
use-drm should be passed too.

Explicit vs implicit doesn't matter in the sw raster until we have release fence
support. When we have release fence support then we want to use this even
without drm.

https://codereview.chromium.org/2497173002/diff/1/components/exo/wayland/clie...
components/exo/wayland/clients/motion_events.cc:470: if
(gl::g_driver_egl.ext.b_EGL_ANDROID_native_fence_sync)
On 2016/11/14 at 04:08:52, Daniele Castagna wrote:
> Why did you change this?
> I'd keep it coherent with the other two extension checks.

This is now available as a result of the patches this depends on but I changed
back for consistency in latest patch.

https://codereview.chromium.org/2497173002/diff/1/components/exo/wayland/clie...
components/exo/wayland/clients/motion_events.cc:506:
std::unique_ptr<zcr_synchronization_v1> synchronization;
On 2016/11/14 at 04:08:53, Daniele Castagna wrote:
> surface_synchronization might be better?

I prefer to keep it consistent with the interface name. We can change the name
of zcr_synchronization to zcr_surface_synchronization but I think it would be
inconsistent with some other surface extension interfaces. Anyhow, this is
unlikely the final solution for wayland explicit synchronization and more for
testing purposes.

https://codereview.chromium.org/2497173002/diff/1/components/exo/wayland/clie...
components/exo/wayland/clients/motion_events.cc:515: if
(!gl::g_driver_egl.ext.b_EGL_ANDROID_native_fence_sync) {
On 2016/11/14 at 04:08:53, Daniele Castagna wrote:
> You could check egl_sync_type != EGL_SYNC_NATIVE_FENCE_ANDROID instead. (You'd
need to #ifdef this though.)

Done.

https://codereview.chromium.org/2497173002/diff/1/components/exo/wayland/clie...
components/exo/wayland/clients/motion_events.cc:660: DCHECK(buffer->egl_sync);
On 2016/11/14 at 04:08:53, Daniele Castagna wrote:
> DCHECK(buffer->egl_sync->is_valid());

Done.

https://codereview.chromium.org/2497173002/diff/1/components/exo/wayland/clie...
components/exo/wayland/clients/motion_events.cc:664: glFlush();
On 2016/11/14 at 04:08:52, Daniele Castagna wrote:
> Why do we need this flush at all?

1. To have issued commands be executed and complete in finite time. No purpose
to allow pending frames to be enqueued otherwise.
2. It is required before eglDupNativeFenceFDANDROID can be called as otherwise
the fence is not guaranteed to ever signal.

https://codereview.chromium.org/2497173002/diff/1/components/exo/wayland/clie...
components/exo/wayland/clients/motion_events.cc:685: if (buffer->egl_sync) {
On 2016/11/14 at 04:08:53, Daniele Castagna wrote:
> Could this be something like:
> 
> if (use_explicit_synchronization_) {
>   DCHECK(synchronization);
>   DCHECK_EQ(egl_sync_type, ...ANDROID);
>   ...
> } else if (buffer->egl_sync) {
>   eglClientWaitSyncKHR(...)
> }
> 
> In this way you avoid the inner if and you avoid mixing assumptions with
actual logic.

Take a look at the refactor in latest patch. The idea is that
use_explicit_synchronization_ should not fail when synchronization is not
needed. E.g. sw raster or glFlush is sufficient.