[ozone/wayland] Wait for buffer.release in gpu process collectively.

Multiple buffer acks arrive from Wayland server for a frame.
BufferQueue only expects one swap completion callback and one
presentation feedback per frame. This CL groups buffers acks in the same
frame and run callbacks when buffers in 1 frame are all ack'ed.

This is 5/? CL for overlay forwarding using wl_subsurface.

Bug: 1063865
Change-Id: I026eb1aba38288c5436c672af4101d61495031b8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2343545Reviewed-by: Robert Kroeger <rjkroege@chromium.org>
Reviewed-by: Maksim Sisov (GMT+3) <msisov@igalia.com>
Commit-Queue: Kramer Ge <fangzhoug@chromium.org>
Cr-Commit-Position: refs/heads/master@{#810373}

ui/ozone/platform/wayland/gpu/gbm_surfaceless_wayland.cc

@@ -46,8 +46,10 @@ GbmSurfacelessWayland::GbmSurfacelessWayland(
 }
 
 void GbmSurfacelessWayland::QueueOverlayPlane(OverlayPlane plane,
-                                              uint32_t buffer_id) {
-  unsubmitted_frames_.back()->planes.push_back({std::move(plane), buffer_id});
+                                              BufferId buffer_id) {
+  auto result =
+      unsubmitted_frames_.back()->planes.emplace(buffer_id, std::move(plane));
+  DCHECK(result.second);
 }
 
 bool GbmSurfacelessWayland::ScheduleOverlayPlane(
@@ -100,7 +102,7 @@ void GbmSurfacelessWayland::SwapBuffersAsync(
     return;
   }
 
-  // TODO(dcastagna): remove glFlush since eglImageFlushExternalEXT called on
+  // TODO(fangzhoug): remove glFlush since eglImageFlushExternalEXT called on
   // the image should be enough (https://crbug.com/720045).
   if (!no_gl_flush_for_tests_)
     glFlush();
@@ -123,16 +125,16 @@ void GbmSurfacelessWayland::SwapBuffersAsync(
   // Uset in-fences provided in the overlays. If there are none, we insert our
   // own fence and wait.
   for (auto& plane : frame->planes) {
-    if (plane.plane.gpu_fence)
-      fences.push_back(std::move(plane.plane.gpu_fence));
+    if (plane.second.gpu_fence)
+      fences.push_back(std::move(plane.second.gpu_fence));
   }
 
   base::OnceClosure fence_wait_task;
   if (!fences.empty()) {
     fence_wait_task = base::BindOnce(&WaitForGpuFences, std::move(fences));
   } else {
-    // TODO: the following should be replaced by a per surface flush as it gets
-    // implemented in GL drivers.
+    // TODO(fangzhoug): the following should be replaced by a per surface flush
+    // as it gets implemented in GL drivers.
     EGLSyncKHR fence = InsertFence(has_implicit_external_sync_);
     CHECK_NE(fence, EGL_NO_SYNC_KHR) << "eglCreateSyncKHR failed";
 
@@ -238,18 +240,14 @@ void GbmSurfacelessWayland::MaybeSubmitFrames() {
     std::vector<ui::ozone::mojom::WaylandOverlayConfigPtr> overlay_configs;
     for (const auto& plane : submitted_frame->planes) {
       overlay_configs.push_back(
-          ui::ozone::mojom::WaylandOverlayConfig::From(plane.plane));
-      overlay_configs.back()->buffer_id = plane.buffer_id;
-      if (plane.plane.z_order == 0) {
+          ui::ozone::mojom::WaylandOverlayConfig::From(plane.second));
+      overlay_configs.back()->buffer_id = plane.first;
+      if (plane.second.z_order == 0) {
         overlay_configs.back()->damage_region = submitted_frame->damage_region_;
-        submitted_frame->buffer_id = plane.buffer_id;
+        submitted_frame->buffer_id = plane.first;
       }
     }
     buffer_manager_->CommitOverlays(widget_, std::move(overlay_configs));
-
-    submitted_frame->unacked_submissions = submitted_frame->planes.size();
-    submitted_frame->unacked_presentations = submitted_frame->planes.size();
-    submitted_frame->planes.clear();
     submitted_frames_.push_back(std::move(submitted_frame));
   }
 }
@@ -271,22 +269,46 @@ void GbmSurfacelessWayland::SetNoGLFlushForTests() {
   no_gl_flush_for_tests_ = true;
 }
 
-void GbmSurfacelessWayland::OnSubmission(uint32_t buffer_id,
+void GbmSurfacelessWayland::OnSubmission(BufferId buffer_id,
                                          const gfx::SwapResult& swap_result) {
   // submitted_frames_ may temporarily have more than one buffer in it if
   // buffers are released out of order by the Wayland server.
   DCHECK(!submitted_frames_.empty());
-  if (--submitted_frames_.front()->unacked_submissions)
-    return;
 
+  size_t erased = 0;
+  for (auto& submitted_frame : submitted_frames_) {
+    if ((erased = submitted_frame->planes.erase(buffer_id)) > 0) {
+      // |completion_callback| only takes 1 SwapResult. It's possible that only
+      // one of the buffers in a frame gets a SWAP_FAILED or
+      // SWAP_NAK_RECREATE_BUFFERS. Don't replace a failed swap_result with
+      // SWAP_ACK. If both SWAP_FAILED and SWAP_NAK_RECREATE_BUFFERS happens,
+      // this swap is treated as SWAP_FAILED.
+      if (submitted_frame->swap_result == gfx::SwapResult::SWAP_ACK ||
+          swap_result == gfx::SwapResult::SWAP_FAILED) {
+        submitted_frame->swap_result = swap_result;
+      }
+      submitted_frame->pending_presentation_buffers.insert(buffer_id);
+      break;
+    }
+  }
+  DCHECK(erased);
+
+  // Following while loop covers below scenario:
+  //   frame_1 submitted a buffer_1 for overlay; frame_2 submitted a buffer_2
+  //   for primary plane. This can happen at the end of a single-on-top overlay.
+  //   buffer_1 is not attached immediately due to unack'ed wl_frame_callback.
+  //   buffer_2 is attached immediately Onsubmission() of buffer_2 runs.
+  while (!submitted_frames_.empty() &&
+         submitted_frames_.front()->planes.empty()) {
     auto submitted_frame = std::move(submitted_frames_.front());
     submitted_frames_.erase(submitted_frames_.begin());
     submitted_frame->overlays.clear();
 
     std::move(submitted_frame->completion_callback)
-      .Run(gfx::SwapCompletionResult(swap_result));
+        .Run(gfx::SwapCompletionResult(submitted_frame->swap_result));
 
     pending_presentation_frames_.push_back(std::move(submitted_frame));
+  }
 
   if (swap_result != gfx::SwapResult::SWAP_ACK) {
     last_swap_buffers_result_ = false;
@@ -297,10 +319,20 @@ void GbmSurfacelessWayland::OnSubmission(uint32_t buffer_id,
 }
 
 void GbmSurfacelessWayland::OnPresentation(
-    uint32_t buffer_id,
+    BufferId buffer_id,
     const gfx::PresentationFeedback& feedback) {
+  DCHECK(!submitted_frames_.empty() || !pending_presentation_frames_.empty());
+
+  size_t erased = 0;
+  for (auto& frame : pending_presentation_frames_) {
+    if ((erased = frame->pending_presentation_buffers.erase(buffer_id)) > 0) {
+      frame->feedback = feedback;
+      break;
+    }
+  }
+
   // Items in |submitted_frames_| will not be moved to
-  // |pending_presentation_frames_| until |unacked_submissions| decrements to 0.
+  // |pending_presentation_frames_| until |planes| is empty.
   // Example:
   //    A SwapBuffers that submitted 2 buffers (buffer_1 and buffer_2) will push
   //    a submitted_frame expecting 2 submission feedbacks and 2 presentation
@@ -312,24 +344,26 @@ void GbmSurfacelessWayland::OnPresentation(
   //      buffer_1:submission > buffer_1:presentation > buffer_2:submission >
   //      buffer_2:presentation
   //    In this case, we have to find the item in |submitted_frames_| and
-  //    decrement |unacked_presentations| there.
-  // TODO(fangzhoug): This solution is sub-optimal and confusing. It increases
-  // the number of IPCs from browser to gpu. The barrier logic should be in the
-  // browser process.
-  if (pending_presentation_frames_.empty()) {
-    auto it = submitted_frames_.begin();
-    for (; !(*it)->unacked_presentations; ++it)
-      ;
-    --(*it)->unacked_presentations;
-    return;
+  //    remove from |pending_presentation_buffers| there.
+  if (!erased) {
+    for (auto& frame : submitted_frames_) {
+      if ((erased = frame->pending_presentation_buffers.erase(buffer_id)) > 0) {
+        frame->feedback = feedback;
+        break;
+      }
+    }
   }
 
-  auto* frame = pending_presentation_frames_.front().get();
-  if (--frame->unacked_presentations)
-    return;
+  DCHECK(erased);
 
-  std::move(frame->presentation_callback).Run(feedback);
+  while (!pending_presentation_frames_.empty() &&
+         pending_presentation_frames_.front()
+             ->pending_presentation_buffers.empty()) {
+    auto* frame = pending_presentation_frames_.front().get();
+    DCHECK(frame->planes.empty());
+    std::move(frame->presentation_callback).Run(frame->feedback);
     pending_presentation_frames_.erase(pending_presentation_frames_.begin());
+  }
 }
 
 }  // namespace ui

ui/ozone/platform/wayland/gpu/gbm_surfaceless_wayland.h

@@ -7,6 +7,7 @@
 
 #include <memory>
 
+#include "base/containers/small_map.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "ui/gfx/native_widget_types.h"
@@ -19,6 +20,8 @@ namespace ui {
 
 class WaylandBufferManagerGpu;
 
+using BufferId = uint32_t;
+
 // A GLSurface for Wayland Ozone platform that uses surfaceless drawing. Drawing
 // and displaying happens directly through NativePixmap buffers. CC would call
 // into SurfaceFactoryOzone to allocate the buffers and then call
@@ -29,7 +32,7 @@ class GbmSurfacelessWayland : public gl::SurfacelessEGL,
   GbmSurfacelessWayland(WaylandBufferManagerGpu* buffer_manager,
                         gfx::AcceleratedWidget widget);
 
-  void QueueOverlayPlane(OverlayPlane plane, uint32_t buffer_id);
+  void QueueOverlayPlane(OverlayPlane plane, BufferId buffer_id);
 
   // gl::GLSurface:
   bool ScheduleOverlayPlane(int z_order,
@@ -62,22 +65,19 @@ class GbmSurfacelessWayland : public gl::SurfacelessEGL,
  private:
   FRIEND_TEST_ALL_PREFIXES(WaylandSurfaceFactoryTest,
                            GbmSurfacelessWaylandCheckOrderOfCallbacksTest);
+  FRIEND_TEST_ALL_PREFIXES(WaylandSurfaceFactoryTest,
+                           GbmSurfacelessWaylandCommitOverlaysCallbacksTest);
+  FRIEND_TEST_ALL_PREFIXES(WaylandSurfaceFactoryTest,
+                           GbmSurfacelessWaylandGroupOnSubmissionCallbacksTest);
 
   ~GbmSurfacelessWayland() override;
 
   // WaylandSurfaceGpu overrides:
-  void OnSubmission(uint32_t buffer_id,
+  void OnSubmission(BufferId buffer_id,
                     const gfx::SwapResult& swap_result) override;
-  void OnPresentation(uint32_t buffer_id,
+  void OnPresentation(BufferId buffer_id,
                       const gfx::PresentationFeedback& feedback) override;
 
-  struct PlaneData {
-    OverlayPlane plane;
-    // The id of the buffer, which represents buffer that backs this overlay
-    // plane.
-    const uint32_t buffer_id;
-  };
-
   struct PendingFrame {
     PendingFrame();
     ~PendingFrame();
@@ -89,24 +89,25 @@ class GbmSurfacelessWayland : public gl::SurfacelessEGL,
     bool ready = false;
 
     // The id of the buffer, which represents this frame.
-    uint32_t buffer_id = 0;
+    BufferId buffer_id = 0;
 
     // A region of the updated content in a corresponding frame. It's used to
     // advice Wayland which part of a buffer is going to be updated. Passing {0,
     // 0, 0, 0} results in a whole buffer update on the Wayland compositor side.
     gfx::Rect damage_region_ = gfx::Rect();
+    // TODO(fangzhoug): This should be changed to support Vulkan.
     std::vector<gl::GLSurfaceOverlay> overlays;
     SwapCompletionCallback completion_callback;
     PresentationCallback presentation_callback;
 
     bool schedule_planes_succeeded = false;
-    std::vector<PlaneData> planes;
 
-    // TODO(fangzhoug): This is a temporary solution to barrier swap/present
-    // acks of a frame that contains multiple buffer commits. Next step is to
-    // barrier in browser process to avoid extra IPC hops.
-    size_t unacked_submissions;
-    size_t unacked_presentations;
+    // Maps |buffer_id| to an OverlayPlane, used for committing overlays and
+    // wait for OnSubmission's.
+    base::small_map<std::map<BufferId, OverlayPlane>> planes;
+    base::flat_set<BufferId> pending_presentation_buffers;
+    gfx::SwapResult swap_result = gfx::SwapResult::SWAP_ACK;
+    gfx::PresentationFeedback feedback;
   };
 
   void MaybeSubmitFrames();

ui/ozone/platform/wayland/host/wayland_buffer_manager_host.cc

@@ -25,6 +25,9 @@ namespace ui {
 
 namespace {
 
+// Use |kInvalidBufferId| to commit surface state without updating wl_buffer.
+constexpr uint32_t kInvalidBufferId = 0u;
+
 uint32_t GetPresentationKindFlags(uint32_t flags) {
   // Wayland spec has different meaning of VSync. In Chromium, VSync means to
   // update the begin frame vsync timing based on presentation feedback.
@@ -65,11 +68,20 @@ class WaylandBufferManagerHost::Surface {
         buffer_manager_(buffer_manager) {}
   ~Surface() = default;
 
-  bool CommitBuffer(uint32_t buffer_id, const gfx::Rect& damage_region) {
+  bool CommitBuffer(uint32_t buffer_id,
+                    const gfx::Rect& damage_region,
+                    bool wait_for_frame_callback) {
     // The window has already been destroyed.
     if (!wayland_surface_)
       return true;
 
+    // This is a buffer-less commit, do not lookup buffers.
+    if (buffer_id == kInvalidBufferId) {
+      pending_commits_.push_back({nullptr, wait_for_frame_callback});
+      MaybeProcessPendingBuffer();
+      return true;
+    }
+
     WaylandBuffer* buffer = GetBuffer(buffer_id);
     if (!buffer) {
       // Get the anonymous_wl_buffer aka the buffer that has not been attached
@@ -95,7 +107,7 @@ class WaylandBufferManagerHost::Surface {
     if (buffer->attached && !buffer->wl_buffer)
       return false;
 
-    pending_buffers_.push_back(buffer);
+    pending_commits_.push_back({buffer, wait_for_frame_callback});
     MaybeProcessPendingBuffer();
     return true;
   }
@@ -108,7 +120,11 @@ class WaylandBufferManagerHost::Surface {
     if (buffer) {
       buffer->released = true;
       MaybeProcessSubmittedBuffers();
-      base::Erase(pending_buffers_, buffer);
+      for (auto it = pending_commits_.begin(); it != pending_commits_.end();
+           ++it) {
+        if (it->buffer == buffer)
+          pending_commits_.erase(it++);
+      }
     }
 
     return buffers_.erase(buffer_id);
@@ -139,7 +155,7 @@ class WaylandBufferManagerHost::Surface {
     ResetSurfaceContents();
 
     submitted_buffers_.clear();
-    pending_buffers_.clear();
+    pending_commits_.clear();
 
     connection_->ScheduleFlush();
   }
@@ -204,7 +220,16 @@ class WaylandBufferManagerHost::Surface {
     bool acked;
   };
 
-  bool CommitBufferInternal(WaylandBuffer* buffer) {
+  // Represents a pending surface commit.
+  struct PendingCommit {
+    // If null, means this commit will not attach buffer.
+    WaylandBuffer* buffer = nullptr;
+    // Whether this commit must wait for a wl_frame_callback and setup another
+    // wl_frame_callback.
+    bool wait_for_callback = false;
+  };
+
+  bool CommitBufferInternal(WaylandBuffer* buffer, bool wait_for_callback) {
     DCHECK(buffer && wayland_surface_);
 
     // If the same buffer has been submitted again right after the client
@@ -225,6 +250,7 @@ class WaylandBufferManagerHost::Surface {
 
     DamageBuffer(buffer);
 
+    if (wait_for_callback)
       SetupFrameCallback();
     SetupPresentationFeedback(buffer->buffer_id);
 
@@ -504,10 +530,10 @@ class WaylandBufferManagerHost::Surface {
   }
 
   void MaybeProcessPendingBuffer() {
-    DCHECK_LE(pending_buffers_.size(), 6u);
+    DCHECK_LE(pending_commits_.size(), 6u);
     // There is nothing to process if there is no pending buffer or the window
     // has been destroyed.
-    if (pending_buffers_.empty() || !wayland_surface_)
+    if (pending_commits_.empty() || !wayland_surface_)
       return;
 
     // This request may come earlier than the Wayland compositor has imported a
@@ -524,12 +550,27 @@ class WaylandBufferManagerHost::Surface {
     //
     // The third case happens if the window hasn't been configured until a
     // request to attach a buffer to its surface is sent.
-    auto* pending_buffer = pending_buffers_.front();
-    if (!pending_buffer->wl_buffer || wl_frame_callback_ || !configured_)
+    auto pending_commit = std::move(pending_commits_.front());
+    if ((pending_commit.buffer && !pending_commit.buffer->wl_buffer) ||
+        (wl_frame_callback_ && pending_commit.wait_for_callback) ||
+        !configured_) {
+      return;
+    }
+
+    // A Commit without attaching buffers only needs to setup wl_frame_callback.
+    if (!pending_commit.buffer) {
+      pending_commits_.erase(pending_commits_.begin());
+      if (pending_commit.wait_for_callback)
+        SetupFrameCallback();
+      CommitSurface();
+      connection_->ScheduleFlush();
+      MaybeProcessSubmittedBuffers();
       return;
+    }
 
-    pending_buffers_.erase(pending_buffers_.begin());
-    CommitBufferInternal(pending_buffer);
+    pending_commits_.erase(pending_commits_.begin());
+    CommitBufferInternal(pending_commit.buffer,
+                         pending_commit.wait_for_callback);
   }
 
   // Widget this helper surface backs and has 1:1 relationship with the
@@ -552,9 +593,9 @@ class WaylandBufferManagerHost::Surface {
   // operation.
   wl::Object<wl_callback> wl_frame_callback_;
 
-  // Queue of buffers which are pending to be submitted (look the comment
+  // Queue of commits which are pending to be submitted (look the comment
   // in the CommitBuffer method).
-  std::vector<WaylandBuffer*> pending_buffers_;
+  std::list<PendingCommit> pending_commits_;
 
   // Queue of buffers which have been submitted and are waiting to be
   // acked (send OnSubmission)
@@ -763,14 +804,16 @@ void WaylandBufferManagerHost::CreateShmBasedBuffer(mojo::PlatformHandle shm_fd,
 bool WaylandBufferManagerHost::CommitBufferInternal(
     WaylandSurface* wayland_surface,
     uint32_t buffer_id,
-    const gfx::Rect& damage_region) {
+    const gfx::Rect& damage_region,
+    bool wait_for_frame_callback) {
   DCHECK(base::CurrentUIThread::IsSet());
 
   Surface* surface = GetSurface(wayland_surface);
   if (!surface || !ValidateBufferIdFromGpu(buffer_id))
     return false;
 
-  if (!surface->CommitBuffer(buffer_id, damage_region)) {
+  if (!surface->CommitBuffer(buffer_id, damage_region,
+                             wait_for_frame_callback)) {
     error_message_ =
         base::StrCat({"Buffer with ", NumberToString(buffer_id),
                       " id does not exist or failed to be created."});
@@ -781,6 +824,24 @@ bool WaylandBufferManagerHost::CommitBufferInternal(
   return true;
 }
 
+bool WaylandBufferManagerHost::CommitWithoutBufferInternal(
+    WaylandSurface* wayland_surface,
+    bool wait_for_frame_callback) {
+  DCHECK(base::CurrentUIThread::IsSet());
+
+  Surface* surface = GetSurface(wayland_surface);
+  if (!surface)
+    return false;
+
+  bool result = surface->CommitBuffer(kInvalidBufferId, gfx::Rect(),
+                                      wait_for_frame_callback);
+  DCHECK(result);
+
+  if (!error_message_.empty())
+    TerminateGpuProcess();
+  return true;
+}
+
 void WaylandBufferManagerHost::CommitBuffer(gfx::AcceleratedWidget widget,
                                             uint32_t buffer_id,
                                             const gfx::Rect& damage_region) {

ui/ozone/platform/wayland/host/wayland_buffer_manager_host.h

@@ -157,9 +157,22 @@ class WaylandBufferManagerHost : public ozone::mojom::WaylandBufferManagerHost,
   // |buffer_id| to a WaylandSurface.
   // Calls OnSubmission and OnPresentation on successful swap and pixels
   // presented.
+  // |wait_for_frame_callback| instructs that a surface should wait for previous
+  // wl_frame_callback. This is primarily used for sync wl_subsurfaces case
+  // where buffer updates within a frame should be seen together. A root_surface
+  // commit will move an entire wl_surface tree from pending state to ready
+  // state. This root_surface commit must wait for wl_frame_callback, such that
+  // in effect all other surface updates wait for this wl_frame_callback, too.
   bool CommitBufferInternal(WaylandSurface* wayland_surface,
                             uint32_t buffer_id,
-                            const gfx::Rect& damage_region);
+                            const gfx::Rect& damage_region,
+                            bool wait_for_frame_callback = true);
+
+  // Does a wl_surface commit without attaching any buffers. This commit will
+  // still wait for previous wl_frame_callback. Similar to above but for
+  // commits that do not change the root_surface.
+  bool CommitWithoutBufferInternal(WaylandSurface* wayland_surface,
+                                   bool wait_for_frame_callback = true);
 
   // When a surface is hidden, the client may want to detach the buffer attached
   // to the surface to ensure Wayland does not present those contents and do not
@@ -239,8 +252,13 @@ class WaylandBufferManagerHost : public ozone::mojom::WaylandBufferManagerHost,
   base::OnceCallback<void(std::string)> terminate_gpu_cb_;
 
   // Contains anonymous buffers aka buffers that are not attached to any of the
-  // existing surfaces and that will be mapped to surfaces later.  Typically
-  // created when CreateAnonymousImage is called on the gpu process side.
+  // existing surfaces and that will be mapped to surfaces later.
+  // Typically created when CreateAnonymousImage is called on the gpu process
+  // side.
+  // We assume that a buffer_id/wl_buffer will never be used on multiple
+  // wl_surfaces so we never re-map buffers to surfaces. If we ever need to use
+  // the same buffer for 2 surfaces at the same time, create multiple wl_buffers
+  // referencing the same dmabuf or underlying storage.
   base::flat_map<uint32_t, std::unique_ptr<WaylandBuffer>> anonymous_buffers_;
 
   base::WeakPtrFactory<WaylandBufferManagerHost> weak_factory_;

ui/ozone/platform/wayland/host/wayland_window.cc

@@ -541,8 +541,9 @@ bool WaylandWindow::CommitOverlays(
       ozone::mojom::WaylandOverlayConfig::New();
   auto split = std::lower_bound(overlays.begin(), overlays.end(), value,
                                 OverlayStackOrderCompare);
-  CHECK((*split)->z_order >= 0);
-  size_t num_primary_planes = (*split)->z_order == 0 ? 1 : 0;
+  CHECK(split == overlays.end() || (*split)->z_order >= 0);
+  size_t num_primary_planes =
+      (split != overlays.end() && (*split)->z_order == 0) ? 1 : 0;
 
   size_t above = (overlays.end() - split) - num_primary_planes;
   size_t below = split - overlays.begin();
@@ -572,8 +573,8 @@ bool WaylandWindow::CommitOverlays(
             (*overlay_iter)->bounds_rect, (*overlay_iter)->enable_blend,
             nullptr, reference_above);
         connection_->buffer_manager_host()->CommitBufferInternal(
-            (*iter)->wayland_surface(), (*overlay_iter)->buffer_id,
-            gfx::Rect());
+            (*iter)->wayland_surface(), (*overlay_iter)->buffer_id, gfx::Rect(),
+            /*wait_for_frame_callback=*/false);
       } else {
         // If there're more subsurfaces requested that we don't need at the
         // moment, hide them.
@@ -601,8 +602,8 @@ bool WaylandWindow::CommitOverlays(
             (*overlay_iter)->bounds_rect, (*overlay_iter)->enable_blend,
             reference_below, nullptr);
         connection_->buffer_manager_host()->CommitBufferInternal(
-            (*iter)->wayland_surface(), (*overlay_iter)->buffer_id,
-            gfx::Rect());
+            (*iter)->wayland_surface(), (*overlay_iter)->buffer_id, gfx::Rect(),
+            /*wait_for_frame_callback=*/false);
       } else {
         // If there're more subsurfaces requested that we don't need at the
         // moment, hide them.
@@ -612,16 +613,16 @@ bool WaylandWindow::CommitOverlays(
   }
 
   if (num_primary_planes) {
-    // TODO: forward fence.
     connection_->buffer_manager_host()->CommitBufferInternal(
-        root_surface(), (*split)->buffer_id, (*split)->damage_region);
+        root_surface(), (*split)->buffer_id, (*split)->damage_region,
+        /*wait_for_frame_callback=*/true);
   } else {
-    // Subsurfaces are set to desync, above operations will only take effects
+    // Subsurfaces are set to sync, above operations will only take effects
     // when root_surface is committed.
-    root_surface()->Commit();
+    connection_->buffer_manager_host()->CommitWithoutBufferInternal(
+        root_surface(), /*wait_for_frame_callback=*/true);
   }
 
-  // commit all;
   return true;
 }