Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(24)

Issue 2497173002: exo: Add explicit synchronization support to motion event client.

Created:
4 years, 1 month ago by reveman
Modified:
4 years, 1 month ago
CC:
chromium-reviews
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 15

Patch Set 2 : refactor #

Unified diffs Side-by-side diffs Delta from patch set Stats (+87 lines, -22 lines) Patch
M components/exo/wayland/BUILD.gn View 1 chunk +1 line, -0 lines 0 comments Download
M components/exo/wayland/clients/motion_events.cc View 1 19 chunks +86 lines, -22 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 5 (2 generated)
reveman
4 years, 1 month ago (2016-11-14 00:42:23 UTC) #2
Daniele Castagna
What was the problem you were observing with DupNativeFence and how did you solve it? ...
4 years, 1 month ago (2016-11-14 04:08:53 UTC) #3
reveman
4 years, 1 month ago (2016-11-14 16:32:10 UTC) #5
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.

Powered by Google App Engine
This is Rietveld 408576698