drm_hwcomposer: Clean up VsyncWorker destruction
With VsyncWorker no longer acquiring the main lock, we can use
std::thread::join on the main thread to ensure that the vsync thread is
stopped cleanly in the VSyncWorker destructor.
- Don't pass a shared_ptr<> into VSyncWorker::ThreadFn to simplify
lifetime management
- Change to unique_ptr to simplify lifetime management
- Remove the hack to destroy the HwcDisplays in two stages when
destructing ComposerClient
- Add std::thread::join to VSyncWorker destructor
Change-Id: I25a34fd304c7b2ec48e43d538bf15794bdc9d68e
Signed-off-by: Drew Davenport <ddavenport@google.com>
diff --git a/drm/VSyncWorker.cpp b/drm/VSyncWorker.cpp
index ac80da4..64152ab 100644
--- a/drm/VSyncWorker.cpp
+++ b/drm/VSyncWorker.cpp
@@ -31,8 +31,8 @@
namespace android {
auto VSyncWorker::CreateInstance(std::shared_ptr<DrmDisplayPipeline> &pipe)
- -> std::shared_ptr<VSyncWorker> {
- auto vsw = std::shared_ptr<VSyncWorker>(new VSyncWorker());
+ -> std::unique_ptr<VSyncWorker> {
+ auto vsw = std::unique_ptr<VSyncWorker>(new VSyncWorker());
if (pipe) {
vsw->high_crtc_ = pipe->crtc->Get()->GetIndexInResArray()
@@ -40,11 +40,17 @@
vsw->drm_fd_ = pipe->device->GetFd();
}
- std::thread(&VSyncWorker::ThreadFn, vsw.get(), vsw).detach();
+ vsw->vswt_ = std::thread(&VSyncWorker::ThreadFn, vsw.get());
return vsw;
}
+VSyncWorker::~VSyncWorker() {
+ StopThread();
+
+ vswt_.join();
+}
+
void VSyncWorker::UpdateVSyncControl() {
{
const std::lock_guard<std::mutex> lock(mutex_);
@@ -143,17 +149,17 @@
return 0;
}
-void VSyncWorker::ThreadFn(const std::shared_ptr<VSyncWorker> &vsw) {
+void VSyncWorker::ThreadFn() {
int ret = 0;
for (;;) {
{
- std::unique_lock<std::mutex> lock(vsw->mutex_);
+ std::unique_lock<std::mutex> lock(mutex_);
if (thread_exit_)
break;
if (!enabled_)
- vsw->cv_.wait(lock);
+ cv_.wait(lock);
if (!enabled_)
continue;
diff --git a/drm/VSyncWorker.h b/drm/VSyncWorker.h
index 0809f48..c76dd14 100644
--- a/drm/VSyncWorker.h
+++ b/drm/VSyncWorker.h
@@ -31,10 +31,10 @@
using VsyncTimestampCallback = std::function<void(int64_t /*timestamp*/,
uint32_t /*period*/)>;
- ~VSyncWorker() = default;
+ ~VSyncWorker();
auto static CreateInstance(std::shared_ptr<DrmDisplayPipeline> &pipe)
- -> std::shared_ptr<VSyncWorker>;
+ -> std::unique_ptr<VSyncWorker>;
// Set the expected vsync period.
void SetVsyncPeriodNs(uint32_t vsync_period_ns);
@@ -53,7 +53,7 @@
private:
VSyncWorker() = default;
- void ThreadFn(const std::shared_ptr<VSyncWorker> &vsw);
+ void ThreadFn();
int64_t GetPhasedVSync(int64_t frame_ns, int64_t current) const;
int SyntheticWaitVBlank(int64_t *timestamp);
diff --git a/hwc2_device/HwcDisplay.cpp b/hwc2_device/HwcDisplay.cpp
index ff8de47..60861af 100644
--- a/hwc2_device/HwcDisplay.cpp
+++ b/hwc2_device/HwcDisplay.cpp
@@ -301,8 +301,6 @@
}
if (vsync_worker_) {
- // TODO: There should be a mechanism to wait for this worker to complete,
- // otherwise there is a race condition while destructing the HwcDisplay.
vsync_worker_->StopThread();
vsync_worker_ = {};
}
diff --git a/hwc2_device/HwcDisplay.h b/hwc2_device/HwcDisplay.h
index 5d593bb..acefff8 100644
--- a/hwc2_device/HwcDisplay.h
+++ b/hwc2_device/HwcDisplay.h
@@ -246,7 +246,7 @@
std::unique_ptr<Backend> backend_;
std::shared_ptr<FlatteningController> flatcon_;
- std::shared_ptr<VSyncWorker> vsync_worker_;
+ std::unique_ptr<VSyncWorker> vsync_worker_;
bool vsync_event_en_{};
const hwc2_display_t handle_;
diff --git a/hwc3/ComposerClient.cpp b/hwc3/ComposerClient.cpp
index 0d3f9c0..5366943 100644
--- a/hwc3/ComposerClient.cpp
+++ b/hwc3/ComposerClient.cpp
@@ -320,17 +320,8 @@
ComposerClient::~ComposerClient() {
DEBUG_FUNC();
{
- // First Deinit the displays to start shutting down the Display's dependent
- // threads such as VSyncWorker.
const std::unique_lock lock(hwc_->GetResMan().GetMainLock());
hwc_->DeinitDisplays();
- }
- // Sleep to wait for threads to complete and exit.
- const int time_for_threads_to_exit_us = 200000;
- usleep(time_for_threads_to_exit_us);
- {
- // Hold the lock while destructing the hwc_ and the objects that it owns.
- const std::unique_lock lock(hwc_->GetResMan().GetMainLock());
hwc_.reset();
}
LOG(DEBUG) << "removed composer client";