Fix vr flinger deadlock and race condition

While investigating hangs when transitioning from 2d --> vr and back I
found a deadlock and race condition in the vr flinger pause/resume
handling. This CL should fix both issues.

Unfortunately there's still another deadlock related to multiple threads
trying to suspend/resume vr flinger, but considering how vr flinger is
currently used by surface flinger we shouldn't ever hit that scenario in
practice.

Bug: None

Test: I was able to reliably get a hang when starting/stopping vr
launcher a few times, but with this CL applied and another CL to remove
calls to SetPowerMode() applied (that CL will be submitted separately),
I no longer see any hangs.

Change-Id: Ie842bf9fb00e4e2937769ed7e1e2ec9cc47861f7
(cherry picked from commit cf921a3919d68d8c8d1b8be39e03a372f6346f57)

2 files changed, 85 insertions, 69 deletions

diff --git a/libs/vr/libvrflinger/hardware_composer.cpp b/libs/vr/libvrflinger/hardware_composer.cpp
index cc082091be..cf89ce3ea2 100644
--- a/libs/vr/libvrflinger/hardware_composer.cpp
+++ b/libs/vr/libvrflinger/hardware_composer.cpp
@@ -107,8 +107,8 @@ HardwareComposer::HardwareComposer(Hwc2::Composer* hwc2_hidl)
       display_on_(false),
       active_layer_count_(0),
       gpu_layer_(nullptr),
+      post_thread_state_(PostThreadState::kPaused),
       terminate_post_thread_event_fd_(-1),
-      pause_post_thread_(true),
       backlight_brightness_fd_(-1),
       primary_display_vsync_event_fd_(-1),
       primary_display_wait_pp_fd_(-1),
@@ -124,19 +124,17 @@ HardwareComposer::HardwareComposer(Hwc2::Composer* hwc2_hidl)
 }
 
 HardwareComposer::~HardwareComposer(void) {
-  if (!IsSuspended()) {
-    Suspend();
-  }
+  Suspend();
 }
 
 bool HardwareComposer::Resume() {
-  std::lock_guard<std::mutex> autolock(layer_mutex_);
-
-  if (!IsSuspended()) {
-    ALOGE("HardwareComposer::Resume: HardwareComposer is already running.");
+  std::lock_guard<std::mutex> post_thread_lock(post_thread_state_mutex_);
+  if (post_thread_state_ == PostThreadState::kRunning) {
     return false;
   }
 
+  std::lock_guard<std::mutex> layer_lock(layer_mutex_);
+
   int32_t ret = HWC2_ERROR_NONE;
 
   static const uint32_t attributes[] = {
@@ -250,36 +248,47 @@ bool HardwareComposer::Resume() {
   pose_client_ = dvrPoseCreate();
   ALOGE_IF(!pose_client_, "HardwareComposer: Failed to create pose client");
 
-  // Variables used to control the post thread state
-  pause_post_thread_ = false;
-  terminate_post_thread_event_fd_.Reset(eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK));
-
+  terminate_post_thread_event_fd_.Reset(
+      eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK));
   LOG_ALWAYS_FATAL_IF(
       !terminate_post_thread_event_fd_,
       "HardwareComposer: Failed to create terminate PostThread event fd : %s",
       strerror(errno));
 
+  post_thread_state_ = PostThreadState::kRunning;
+  post_thread_state_cond_var_.notify_all();
+
   // If get_id() is the default thread::id object, it has not been created yet
   if (post_thread_.get_id() == std::thread::id()) {
     post_thread_ = std::thread(&HardwareComposer::PostThread, this);
   } else {
     UpdateDisplayState();
-    thread_pause_semaphore_.notify_one();
   }
 
   return true;
 }
 
 bool HardwareComposer::Suspend() {
-  // Wait for any pending layer operations to finish
-  std::unique_lock<std::mutex> layer_lock(layer_mutex_);
-
-  if (IsSuspended()) {
-    ALOGE("HardwareComposer::Suspend: HardwareComposer is already suspended.");
+  std::unique_lock<std::mutex> post_thread_lock(post_thread_state_mutex_);
+  if (post_thread_state_ == PostThreadState::kPaused) {
     return false;
   }
 
-  PausePostThread();
+  post_thread_state_ = PostThreadState::kPauseRequested;
+
+  int error = eventfd_write(terminate_post_thread_event_fd_.Get(), 1);
+  ALOGE_IF(error,
+           "HardwareComposer::Suspend: could not write post "
+           "thread termination event fd : %d",
+           error);
+
+  post_thread_state_cond_var_.wait(
+      post_thread_lock,
+      [this] { return post_thread_state_ == PostThreadState::kPaused; });
+  terminate_post_thread_event_fd_.Close();
+
+  // Wait for any pending layer operations to finish
+  std::lock_guard<std::mutex> layer_lock(layer_mutex_);
 
   EnableVsync(false);
   SetPowerMode(HWC_DISPLAY_PRIMARY, HWC2_POWER_MODE_OFF);
@@ -308,19 +317,6 @@ bool HardwareComposer::Suspend() {
   return true;
 }
 
-void HardwareComposer::PausePostThread() {
-  pause_post_thread_ = true;
-
-  int error = eventfd_write(terminate_post_thread_event_fd_.Get(), 1);
-  ALOGE_IF(error,
-           "HardwareComposer::PausePostThread: could not write post "
-           "thread termination event fd : %d",
-           error);
-
-  std::unique_lock<std::mutex> wait_for_thread(thread_pause_mutex_);
-  terminate_post_thread_event_fd_.Close();
-}
-
 DisplayMetrics HardwareComposer::GetHmdDisplayMetrics() const {
   vec2i screen_size(display_metrics_.width, display_metrics_.height);
   DisplayOrientation orientation =
@@ -580,7 +576,10 @@ void HardwareComposer::UpdateDisplayState() {
 
 int HardwareComposer::SetDisplaySurfaces(
     std::vector<std::shared_ptr<DisplaySurface>> surfaces) {
-  std::lock_guard<std::mutex> autolock(layer_mutex_);
+  // The double lock is necessary because we access both the display surfaces
+  // and post_thread_state_.
+  std::lock_guard<std::mutex> post_thread_state_lock(post_thread_state_mutex_);
+  std::lock_guard<std::mutex> layer_lock(layer_mutex_);
 
   ALOGI("HardwareComposer::SetDisplaySurfaces: surface count=%zd",
         surfaces.size());
@@ -626,7 +625,7 @@ int HardwareComposer::SetDisplaySurfaces(
 
   // TODO(skiazyk): fix this so that it is handled seamlessly with dormant/non-
   // dormant state.
-  if (!IsSuspended()) {
+  if (post_thread_state_ == PostThreadState::kRunning) {
     UpdateDisplayState();
   }
 
@@ -726,7 +725,8 @@ int HardwareComposer::ReadVSyncTimestamp(int64_t* timestamp) {
 // Blocks until the next vsync event is signaled by the display driver.
 // TODO(eieio): This is pretty driver specific, this should be moved to a
 // separate class eventually.
-int HardwareComposer::BlockUntilVSync() {
+int HardwareComposer::BlockUntilVSync(/*out*/ bool* suspend_requested) {
+  *suspend_requested = false;
   const int event_fd = primary_display_vsync_event_fd_.Get();
   pollfd pfd[2] = {
       {
@@ -751,6 +751,8 @@ int HardwareComposer::BlockUntilVSync() {
              strerror(error), error);
   } while (ret < 0 && error == EINTR);
 
+  if (ret >= 0 && pfd[1].revents != 0)
+    *suspend_requested = true;
   return ret < 0 ? -error : 0;
 }
 
@@ -773,15 +775,11 @@ int HardwareComposer::WaitForVSync(int64_t* timestamp) {
 
     if (error == -EAGAIN) {
       // Vsync was turned off, wait for the next vsync event.
-      error = BlockUntilVSync();
-      if (error < 0)
+      bool suspend_requested = false;
+      error = BlockUntilVSync(&suspend_requested);
+      if (error < 0 || suspend_requested)
         return error;
 
-      // If a request to pause the post thread was given, exit immediately
-      if (IsSuspended()) {
-        return 0;
-      }
-
       // Try again to get the timestamp for this new vsync interval.
       continue;
     }
@@ -802,14 +800,10 @@ int HardwareComposer::WaitForVSync(int64_t* timestamp) {
 
     if (distance_to_vsync_est > threshold_ns) {
       // Wait for vsync event notification.
-      error = BlockUntilVSync();
-      if (error < 0)
+      bool suspend_requested = false;
+      error = BlockUntilVSync(&suspend_requested);
+      if (error < 0 || suspend_requested)
         return error;
-
-      // Again, exit immediately if the thread was requested to pause
-      if (IsSuspended()) {
-        return 0;
-      }
     } else {
       // Sleep for a short time before retrying.
       std::this_thread::sleep_for(std::chrono::milliseconds(1));
@@ -848,8 +842,6 @@ void HardwareComposer::PostThread() {
   // NOLINTNEXTLINE(runtime/int)
   prctl(PR_SET_NAME, reinterpret_cast<unsigned long>("PostThread"), 0, 0, 0);
 
-  std::unique_lock<std::mutex> thread_lock(thread_pause_mutex_);
-
   // Set the scheduler to SCHED_FIFO with high priority.
   int error = dvrSetSchedulerClass(0, "graphics:high");
   LOG_ALWAYS_FATAL_IF(
@@ -903,12 +895,19 @@ void HardwareComposer::PostThread() {
   while (1) {
     ATRACE_NAME("HardwareComposer::PostThread");
 
-    while (IsSuspended()) {
-      ALOGI("HardwareComposer::PostThread: Post thread pause requested.");
-      thread_pause_semaphore_.wait(thread_lock);
-      // The layers will need to be updated since they were deleted previously
-      display_surfaces_updated_ = true;
-      hardware_layers_need_update_ = true;
+    {
+      std::unique_lock<std::mutex> post_thread_lock(post_thread_state_mutex_);
+      if (post_thread_state_ == PostThreadState::kPauseRequested) {
+        ALOGI("HardwareComposer::PostThread: Post thread pause requested.");
+        post_thread_state_ = PostThreadState::kPaused;
+        post_thread_state_cond_var_.notify_all();
+        post_thread_state_cond_var_.wait(
+            post_thread_lock,
+            [this] { return post_thread_state_ == PostThreadState::kRunning; });
+        // The layers will need to be updated since they were deleted previously
+        display_surfaces_updated_ = true;
+        hardware_layers_need_update_ = true;
+      }
     }
 
     int64_t vsync_timestamp = 0;
@@ -925,7 +924,8 @@ void HardwareComposer::PostThread() {
           strerror(-error));
 
       // Don't bother processing this frame if a pause was requested
-      if (IsSuspended()) {
+      std::lock_guard<std::mutex> post_thread_lock(post_thread_state_mutex_);
+      if (post_thread_state_ == PostThreadState::kPauseRequested) {
         continue;
       }
     }
@@ -1055,7 +1055,7 @@ void HardwareComposer::PostThread() {
 
 bool HardwareComposer::UpdateLayerConfig(
     std::vector<std::shared_ptr<DisplaySurface>>* compositor_surfaces) {
-  std::lock_guard<std::mutex> autolock(layer_mutex_);
+  std::lock_guard<std::mutex> layer_lock(layer_mutex_);
 
   if (!display_surfaces_updated_)
     return false;
diff --git a/libs/vr/libvrflinger/hardware_composer.h b/libs/vr/libvrflinger/hardware_composer.h
index cfe8c84ae7..567af5c704 100644
--- a/libs/vr/libvrflinger/hardware_composer.h
+++ b/libs/vr/libvrflinger/hardware_composer.h
@@ -191,7 +191,6 @@ class HardwareComposer {
 
   bool Suspend();
   bool Resume();
-  bool IsSuspended() const { return pause_post_thread_; }
 
   // Get the HMD display metrics for the current display.
   DisplayMetrics GetHmdDisplayMetrics() const;
@@ -263,7 +262,7 @@ class HardwareComposer {
   void PostThread();
 
   int ReadWaitPPState();
-  int BlockUntilVSync();
+  int BlockUntilVSync(/*out*/ bool* suspend_requested);
   int ReadVSyncTimestamp(int64_t* timestamp);
   int WaitForVSync(int64_t* timestamp);
   int SleepUntil(int64_t wakeup_timestamp);
@@ -303,8 +302,6 @@ class HardwareComposer {
 
   void HandlePendingScreenshots();
 
-  void PausePostThread();
-
   // Hardware composer HAL device.
   std::unique_ptr<Hwc2::Composer> hwc2_hidl_;
   sp<ComposerCallback> callbacks_;
@@ -347,16 +344,35 @@ class HardwareComposer {
   // Handler to hook vsync events outside of this class.
   VSyncCallback vsync_callback_;
 
-  // Thread and condition for managing the layer posting thread. This thread
-  // wakes up a short time before vsync to hand buffers to post processing and
-  // the results to hardware composer.
+  // The layer posting thread. This thread wakes up a short time before vsync to
+  // hand buffers to post processing and the results to hardware composer.
   std::thread post_thread_;
 
+  enum class PostThreadState {
+    // post_thread_state_ starts off paused. When suspending, the control thread
+    // will block until post_thread_state_ == kPaused, indicating the post
+    // thread has completed the transition to paused (most importantly: no more
+    // hardware composer calls).
+    kPaused,
+    // post_thread_state_ is set to kRunning by the control thread (either
+    // surface flinger's main thread or the vr flinger dispatcher thread). The
+    // post thread blocks until post_thread_state_ == kRunning.
+    kRunning,
+    // Set by the control thread to indicate the post thread should pause. The
+    // post thread will change post_thread_state_ from kPauseRequested to
+    // kPaused when it stops.
+    kPauseRequested
+  };
   // Control variables to control the state of the post thread
+  PostThreadState post_thread_state_;
+  // Used to wake the post thread up while it's waiting for vsync, for faster
+  // transition to the paused state.
   pdx::LocalHandle terminate_post_thread_event_fd_;
-  bool pause_post_thread_;
-  std::mutex thread_pause_mutex_;
-  std::condition_variable thread_pause_semaphore_;
+  // post_thread_state_mutex_ should be held before checking or modifying
+  // post_thread_state_.
+  std::mutex post_thread_state_mutex_;
+  // Used to communicate between the control thread and the post thread.
+  std::condition_variable post_thread_state_cond_var_;
 
   // Backlight LED brightness sysfs node.
   pdx::LocalHandle backlight_brightness_fd_;