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)
diff --git a/libs/vr/libvrflinger/hardware_composer.cpp b/libs/vr/libvrflinger/hardware_composer.cpp
index cc08209..cf89ce3 100644
--- a/libs/vr/libvrflinger/hardware_composer.cpp
+++ b/libs/vr/libvrflinger/hardware_composer.cpp
@@ -107,8 +107,8 @@
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(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 @@
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 @@
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 @@
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 @@
// 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 @@
// 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 @@
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 @@
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 @@
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 @@
// 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 @@
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 @@
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 @@
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;