exo: Add explicit synchronization support to motion event client.

components/exo/wayland/clients/motion_events.cc

Index: components/exo/wayland/clients/motion_events.cc
diff --git a/components/exo/wayland/clients/motion_events.cc b/components/exo/wayland/clients/motion_events.cc
index 6d913b4ae4e73f0e37166c904fccd34c7bd4f438..a2a80e45613c0734ed13376a8b9e53fe3d663bbf 100644
--- a/components/exo/wayland/clients/motion_events.cc
+++ b/components/exo/wayland/clients/motion_events.cc
@@ -8,6 +8,7 @@
 
 #include <fcntl.h>
 #include <linux-dmabuf-unstable-v1-client-protocol.h>
+#include <linux-explicit-synchronization-unstable-v1-client-protocol.h>
 #include <wayland-client-core.h>
 #include <wayland-client-protocol.h>
 
@@ -68,6 +69,9 @@ DEFAULT_DELETER(wl_seat, wl_seat_destroy)
 DEFAULT_DELETER(wl_pointer, wl_pointer_destroy)
 DEFAULT_DELETER(wl_touch, wl_touch_destroy)
 DEFAULT_DELETER(wl_callback, wl_callback_destroy)
+DEFAULT_DELETER(zcr_linux_explicit_synchronization_v1,
+                zcr_linux_explicit_synchronization_v1_destroy)
+DEFAULT_DELETER(zcr_synchronization_v1, zcr_synchronization_v1_destroy)
 DEFAULT_DELETER(zwp_linux_buffer_params_v1, zwp_linux_buffer_params_v1_destroy)
 DEFAULT_DELETER(zwp_linux_dmabuf_v1, zwp_linux_dmabuf_v1_destroy)
 
@@ -110,6 +114,8 @@ const int kBenchmarkInterval = 5;
 struct Globals {
   std::unique_ptr<wl_compositor> compositor;
   std::unique_ptr<wl_shm> shm;
+  std::unique_ptr<zcr_linux_explicit_synchronization_v1>
+      linux_explicit_synchronization;
   std::unique_ptr<zwp_linux_dmabuf_v1> linux_dmabuf;
   std::unique_ptr<wl_shell> shell;
   std::unique_ptr<wl_seat> seat;
@@ -134,6 +140,11 @@ void RegistryHandler(void* data,
   } else if (strcmp(interface, "wl_seat") == 0) {
     globals->seat.reset(static_cast<wl_seat*>(
         wl_registry_bind(registry, id, &wl_seat_interface, 5)));
+  } else if (strcmp(interface, "zcr_linux_explicit_synchronization_v1") == 0) {
+    globals->linux_explicit_synchronization.reset(
+        static_cast<zcr_linux_explicit_synchronization_v1*>(wl_registry_bind(
+            registry, id, &zcr_linux_explicit_synchronization_v1_interface,
+            1)));
   } else if (strcmp(interface, "zwp_linux_dmabuf_v1") == 0) {
     globals->linux_dmabuf.reset(static_cast<zwp_linux_dmabuf_v1*>(
         wl_registry_bind(registry, id, &zwp_linux_dmabuf_v1_interface, 1)));
@@ -158,6 +169,14 @@ struct DeleteEglImageTraits {
   }
 };
 using ScopedEglImage = base::ScopedGeneric<EGLImageKHR, DeleteEglImageTraits>;
+
+struct DeleteEglSyncTraits {
+  static EGLSyncKHR InvalidValue() { return 0; }
+  static void Free(EGLSyncKHR sync) {
+    eglDestroySyncKHR(eglGetCurrentDisplay(), sync);
+  }
+};
+using ScopedEglSync = base::ScopedGeneric<EGLSyncKHR, DeleteEglSyncTraits>;
 #endif
 
 struct Buffer {
@@ -166,6 +185,7 @@ struct Buffer {
 #if defined(OZONE_PLATFORM_GBM)
   std::unique_ptr<gbm_bo> bo;
   std::unique_ptr<ScopedEglImage> egl_image;
+  std::unique_ptr<ScopedEglSync> egl_sync;
   std::unique_ptr<ScopedTexture> texture;
 #endif
   std::unique_ptr<zwp_linux_buffer_params_v1> params;
@@ -303,6 +323,7 @@ class MotionEvents {
                size_t num_rects,
                size_t max_frames_pending,
                bool fullscreen,
+               bool use_explicit_synchronization,
                const std::string* use_drm)
       : width_(width),
         height_(height),
@@ -310,6 +331,7 @@ class MotionEvents {
         num_rects_(num_rects),
         max_frames_pending_(max_frames_pending),
         fullscreen_(fullscreen),
+        use_explicit_synchronization_(use_explicit_synchronization),
         use_drm_(use_drm) {}
 
   // Initialize and run client main loop.
@@ -324,6 +346,7 @@ class MotionEvents {
   const size_t num_rects_;
   const size_t max_frames_pending_;
   const bool fullscreen_;
+  const bool use_explicit_synchronization_;
   const std::string* use_drm_;
 
   Globals globals_;
@@ -376,6 +399,11 @@ int MotionEvents::Run() {
     LOG(ERROR) << "Can't find shm interface";
     return 1;
   }
+  if (use_explicit_synchronization_ &&

[Daniele Castagna, 2016/11/14 04:08:52]

Maybe we should also check that if use-explicit-synchronization is passed
use-drm should be passed too.

[reveman, 2016/11/14 16:32:09]

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.

+      !globals_.linux_explicit_synchronization) {
+    LOG(ERROR) << "Can't find linux_explicit_synchronization interface";
+    return 1;
+  }
   if (use_drm_ && !globals_.linux_dmabuf) {
     LOG(ERROR) << "Can't find linux_dmabuf interface";
     return 1;
@@ -389,7 +417,6 @@ int MotionEvents::Run() {
     return 1;
   }
 
-  EGLenum egl_sync_type = 0;
 #if defined(OZONE_PLATFORM_GBM)
   if (use_drm_) {
     // Number of files to look for when discovering DRM devices.
@@ -435,13 +462,13 @@ int MotionEvents::Run() {
       reinterpret_cast<GrBackendContext>(native_interface.get())));
   DCHECK(gr_context_);
 
+  EGLenum egl_sync_type = 0;
   if (gl::GLSurfaceEGL::HasEGLExtension("EGL_EXT_image_flush_external") ||
       gl::GLSurfaceEGL::HasEGLExtension("EGL_ARM_implicit_external_sync")) {
     egl_sync_type = EGL_SYNC_FENCE_KHR;
   }
-  if (gl::GLSurfaceEGL::HasEGLExtension("EGL_ANDROID_native_fence_sync")) {
+  if (gl::g_driver_egl.ext.b_EGL_ANDROID_native_fence_sync)

[Daniele Castagna, 2016/11/14 04:08:52]

Why did you change this?
I'd keep it coherent with the other two extension checks.

[reveman, 2016/11/14 16:32:09]

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

     egl_sync_type = EGL_SYNC_NATIVE_FENCE_ANDROID;
-  }
 #endif
 
   wl_buffer_listener buffer_listener = {BufferRelease};
@@ -476,6 +503,21 @@ int MotionEvents::Run() {
   wl_region_add(opaque_region.get(), 0, 0, width_, height_);
   wl_surface_set_opaque_region(surface.get(), opaque_region.get());
 
+  std::unique_ptr<zcr_synchronization_v1> synchronization;

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

surface_synchronization might be better?

[reveman, 2016/11/14 16:32:09]

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.

+  if (use_explicit_synchronization_) {
+    synchronization.reset(static_cast<zcr_synchronization_v1*>(
+        zcr_linux_explicit_synchronization_v1_get_synchronization(
+            globals_.linux_explicit_synchronization.get(), surface.get())));
+    if (!synchronization) {
+      LOG(ERROR) << "Can't get synchronization";
+      return 1;
+    }
+    if (!gl::g_driver_egl.ext.b_EGL_ANDROID_native_fence_sync) {

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

You could check egl_sync_type != EGL_SYNC_NATIVE_FENCE_ANDROID instead. (You'd
need to #ifdef this though.)

[reveman, 2016/11/14 16:32:09]

Done.

+      LOG(ERROR) << "EGL_ANDROID_native_fence_sync missing";
+      return 1;
+    }
+  }
+
   std::unique_ptr<wl_shell_surface> shell_surface(
       static_cast<wl_shell_surface*>(
           wl_shell_get_shell_surface(globals_.shell.get(), surface.get())));
@@ -523,7 +565,7 @@ int MotionEvents::Run() {
   Frame frame;
   std::unique_ptr<wl_callback> frame_callback;
   wl_callback_listener frame_listener = {FrameCallback};
-  std::deque<wl_buffer*> pending_frames;
+  std::deque<Buffer*> pending_frames;
 
   uint32_t frames = 0;
   base::TimeTicks benchmark_time = base::TimeTicks::Now();
@@ -610,21 +652,20 @@ int MotionEvents::Run() {
       canvas->restore();
       if (gr_context_) {
         gr_context_->flush();
-        glFlush();
 
+#if defined(OZONE_PLATFORM_GBM)
         if (egl_sync_type) {
-          EGLSyncKHR sync =
-              eglCreateSyncKHR(eglGetCurrentDisplay(), egl_sync_type, nullptr);
-          DCHECK(sync != EGL_NO_SYNC_KHR);
-          eglClientWaitSyncKHR(eglGetCurrentDisplay(), sync,
-                               EGL_SYNC_FLUSH_COMMANDS_BIT_KHR,
-                               EGL_FOREVER_KHR);
-          eglDestroySyncKHR(eglGetCurrentDisplay(), sync);
+          buffer->egl_sync.reset(new ScopedEglSync(eglCreateSyncKHR(
+              eglGetCurrentDisplay(), egl_sync_type, nullptr)));
+          DCHECK(buffer->egl_sync);

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

DCHECK(buffer->egl_sync->is_valid());

[reveman, 2016/11/14 16:32:09]

Done.

         }
+#endif
+
+        glFlush();

[Daniele Castagna, 2016/11/14 04:08:52]

Why do we need this flush at all?

[reveman, 2016/11/14 16:32:09]

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.

       }
 
       buffer->busy = true;
-      pending_frames.push_back(buffer->buffer.get());
+      pending_frames.push_back(buffer);
 
       ++frames;
       benchmark_wall_time += base::TimeTicks::Now() - wall_time_start;
@@ -634,10 +675,30 @@ int MotionEvents::Run() {
 
     if (!frame.callback_pending) {
       DCHECK_GT(pending_frames.size(), 0u);
-      wl_surface_set_buffer_scale(surface.get(), scale_);
-      wl_surface_attach(surface.get(), pending_frames.front(), 0, 0);
+      Buffer* buffer = pending_frames.front();
       pending_frames.pop_front();
 
+      wl_surface_set_buffer_scale(surface.get(), scale_);
+      wl_surface_attach(surface.get(), buffer->buffer.get(), 0, 0);
+
+#if defined(OZONE_PLATFORM_GBM)
+      if (buffer->egl_sync) {

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

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.

[reveman, 2016/11/14 16:32:09]

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.

+        if (synchronization) {

[Daniele Castagna, 2016/11/14 04:08:52]

I'd still check use_explicit_synchronization_ here, it'd make this piece of code
easier to read without context.

+          DCHECK_EQ(egl_sync_type,
+                    static_cast<EGLenum>(EGL_SYNC_NATIVE_FENCE_ANDROID));
+          base::ScopedFD fence_fd(eglDupNativeFenceFDANDROID(
+              eglGetCurrentDisplay(), buffer->egl_sync->get()));
+          DCHECK(fence_fd.is_valid());
+          zcr_synchronization_v1_set_acquire_fence(synchronization.get(),
+                                                   fence_fd.get());
+        } else {
+          eglClientWaitSyncKHR(eglGetCurrentDisplay(), buffer->egl_sync->get(),
+                               EGL_SYNC_FLUSH_COMMANDS_BIT_KHR,
+                               EGL_FOREVER_KHR);
+        }
+      }
+#endif
+
       frame_callback.reset(wl_surface_frame(surface.get()));
       wl_callback_add_listener(frame_callback.get(), &frame_listener, &frame);
       frame.callback_pending = true;
@@ -760,6 +821,9 @@ const char kMaxFramesPending[] = "max-frames-pending";
 // Specifies if client should be fullscreen.
 const char kFullscreen[] = "fullscreen";
 
+// Specifies if client should be using explicit synchronization.
+const char kUseExplicitSynchronization[] = "use-explicit-synchronization";
+
 // Use drm buffer instead of shared memory.
 const char kUseDrm[] = "use-drm";
 
@@ -813,6 +877,8 @@ int main(int argc, char* argv[]) {
 
   exo::wayland::clients::MotionEvents client(
       width, height, scale, num_rects, max_frames_pending,
-      command_line->HasSwitch(switches::kFullscreen), use_drm.get());
+      command_line->HasSwitch(switches::kFullscreen),
+      command_line->HasSwitch(switches::kUseExplicitSynchronization),
+      use_drm.get());
   return client.Run();
 }