Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 282a5ed4 authored by Steven Thomas's avatar Steven Thomas
Browse files

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)
parent ba7a67e6

libs/vr/libvrflinger/hardware_composer.cpp

+59 −59
Original line number Diff line number Diff line
@@ -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();

}

}



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,13 +895,20 @@ void HardwareComposer::PostThread() { 
  while (1) {

    ATRACE_NAME("HardwareComposer::PostThread");



    while (IsSuspended()) {

    {

      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.");

      thread_pause_semaphore_.wait(thread_lock);

        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;

libs/vr/libvrflinger/hardware_composer.h

+26 −10
Original line number Diff line number Diff line
@@ -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_;