SF: Cleanup EventThread Part 1
The cleanups in this CL are primarily about switching from
android::Thread to std::thread.
1) Convert from android::Thread to std::thread, along with using
std::mutex and std::condition_variable to keep consistency.
2) Switch the header to #pragma once.
3) Added Clang thread annotations and enabled the corresponding warning.
4) Added proper thread shutdown handling (invoked by dtor).
Test: No issues observed on Pixel XL
Bug: None
Change-Id: I2ef0ad18ef75e139f70d856031991f87d187efe6
diff --git a/services/surfaceflinger/EventThread.cpp b/services/surfaceflinger/EventThread.cpp
index 6b92cc8..2cbc59e 100644
--- a/services/surfaceflinger/EventThread.cpp
+++ b/services/surfaceflinger/EventThread.cpp
@@ -16,10 +16,14 @@
#define ATRACE_TAG ATRACE_TAG_GRAPHICS
-#include <stdint.h>
+#include <pthread.h>
+#include <sched.h>
#include <sys/types.h>
+#include <chrono>
+#include <cstdint>
#include <cutils/compiler.h>
+#include <cutils/sched_policy.h>
#include <gui/DisplayEventReceiver.h>
#include <gui/IDisplayEventConnection.h>
@@ -31,105 +35,126 @@
#include "EventThread.h"
#include "SurfaceFlinger.h"
-// ---------------------------------------------------------------------------
-namespace android {
+using namespace std::chrono_literals;
+
// ---------------------------------------------------------------------------
-EventThread::EventThread(const sp<VSyncSource>& src, SurfaceFlinger& flinger, bool interceptVSyncs)
- : mVSyncSource(src),
- mFlinger(flinger),
- mUseSoftwareVSync(false),
- mVsyncEnabled(false),
- mDebugVsyncEnabled(false),
- mInterceptVSyncs(interceptVSyncs) {
- for (int32_t i = 0; i < DisplayDevice::NUM_BUILTIN_DISPLAY_TYPES; i++) {
- mVSyncEvent[i].header.type = DisplayEventReceiver::DISPLAY_EVENT_VSYNC;
- mVSyncEvent[i].header.id = 0;
- mVSyncEvent[i].header.timestamp = 0;
- mVSyncEvent[i].vsync.count = 0;
+namespace android {
+
+// ---------------------------------------------------------------------------
+
+EventThread::EventThread(const sp<VSyncSource>& src, SurfaceFlinger& flinger, bool interceptVSyncs,
+ const char* threadName)
+ : mVSyncSource(src), mFlinger(flinger), mInterceptVSyncs(interceptVSyncs) {
+ for (auto& event : mVSyncEvent) {
+ event.header.type = DisplayEventReceiver::DISPLAY_EVENT_VSYNC;
+ event.header.id = 0;
+ event.header.timestamp = 0;
+ event.vsync.count = 0;
}
+
+ mThread = std::thread(&EventThread::threadMain, this);
+
+ pthread_setname_np(mThread.native_handle(), threadName);
+
+ pid_t tid = pthread_gettid_np(mThread.native_handle());
+
+ // Use SCHED_FIFO to minimize jitter
+ constexpr int EVENT_THREAD_PRIORITY = 2;
+ struct sched_param param = {0};
+ param.sched_priority = EVENT_THREAD_PRIORITY;
+ if (pthread_setschedparam(mThread.native_handle(), SCHED_FIFO, ¶m) != 0) {
+ ALOGE("Couldn't set SCHED_FIFO for EventThread");
+ }
+
+ set_sched_policy(tid, SP_FOREGROUND);
+}
+
+EventThread::~EventThread() {
+ {
+ std::lock_guard<std::mutex> lock(mMutex);
+ mKeepRunning = false;
+ mCondition.notify_all();
+ }
+ mThread.join();
}
void EventThread::setPhaseOffset(nsecs_t phaseOffset) {
- Mutex::Autolock _l(mLock);
+ std::lock_guard<std::mutex> lock(mMutex);
mVSyncSource->setPhaseOffset(phaseOffset);
}
-void EventThread::onFirstRef() {
- run("EventThread", PRIORITY_URGENT_DISPLAY + PRIORITY_MORE_FAVORABLE);
-}
-
sp<EventThread::Connection> EventThread::createEventConnection() const {
return new Connection(const_cast<EventThread*>(this));
}
status_t EventThread::registerDisplayEventConnection(
const sp<EventThread::Connection>& connection) {
- Mutex::Autolock _l(mLock);
+ std::lock_guard<std::mutex> lock(mMutex);
mDisplayEventConnections.add(connection);
- mCondition.broadcast();
+ mCondition.notify_all();
return NO_ERROR;
}
void EventThread::removeDisplayEventConnection(const wp<EventThread::Connection>& connection) {
- Mutex::Autolock _l(mLock);
+ std::lock_guard<std::mutex> lock(mMutex);
mDisplayEventConnections.remove(connection);
}
void EventThread::setVsyncRate(uint32_t count, const sp<EventThread::Connection>& connection) {
if (int32_t(count) >= 0) { // server must protect against bad params
- Mutex::Autolock _l(mLock);
+ std::lock_guard<std::mutex> lock(mMutex);
const int32_t new_count = (count == 0) ? -1 : count;
if (connection->count != new_count) {
connection->count = new_count;
- mCondition.broadcast();
+ mCondition.notify_all();
}
}
}
void EventThread::requestNextVsync(const sp<EventThread::Connection>& connection) {
- Mutex::Autolock _l(mLock);
+ std::lock_guard<std::mutex> lock(mMutex);
mFlinger.resyncWithRateLimit();
if (connection->count < 0) {
connection->count = 0;
- mCondition.broadcast();
+ mCondition.notify_all();
}
}
void EventThread::onScreenReleased() {
- Mutex::Autolock _l(mLock);
+ std::lock_guard<std::mutex> lock(mMutex);
if (!mUseSoftwareVSync) {
// disable reliance on h/w vsync
mUseSoftwareVSync = true;
- mCondition.broadcast();
+ mCondition.notify_all();
}
}
void EventThread::onScreenAcquired() {
- Mutex::Autolock _l(mLock);
+ std::lock_guard<std::mutex> lock(mMutex);
if (mUseSoftwareVSync) {
// resume use of h/w vsync
mUseSoftwareVSync = false;
- mCondition.broadcast();
+ mCondition.notify_all();
}
}
void EventThread::onVSyncEvent(nsecs_t timestamp) {
- Mutex::Autolock _l(mLock);
+ std::lock_guard<std::mutex> lock(mMutex);
mVSyncEvent[0].header.type = DisplayEventReceiver::DISPLAY_EVENT_VSYNC;
mVSyncEvent[0].header.id = 0;
mVSyncEvent[0].header.timestamp = timestamp;
mVSyncEvent[0].vsync.count++;
- mCondition.broadcast();
+ mCondition.notify_all();
}
void EventThread::onHotplugReceived(int type, bool connected) {
ALOGE_IF(type >= DisplayDevice::NUM_BUILTIN_DISPLAY_TYPES,
"received hotplug event for an invalid display (id=%d)", type);
- Mutex::Autolock _l(mLock);
+ std::lock_guard<std::mutex> lock(mMutex);
if (type < DisplayDevice::NUM_BUILTIN_DISPLAY_TYPES) {
DisplayEventReceiver::Event event;
event.header.type = DisplayEventReceiver::DISPLAY_EVENT_HOTPLUG;
@@ -137,46 +162,48 @@
event.header.timestamp = systemTime();
event.hotplug.connected = connected;
mPendingEvents.add(event);
- mCondition.broadcast();
+ mCondition.notify_all();
}
}
-bool EventThread::threadLoop() {
- DisplayEventReceiver::Event event;
- Vector<sp<EventThread::Connection> > signalConnections;
- signalConnections = waitForEvent(&event);
+void EventThread::threadMain() NO_THREAD_SAFETY_ANALYSIS {
+ std::unique_lock<std::mutex> lock(mMutex);
+ while (mKeepRunning) {
+ DisplayEventReceiver::Event event;
+ Vector<sp<EventThread::Connection> > signalConnections;
+ signalConnections = waitForEventLocked(&lock, &event);
- // dispatch events to listeners...
- const size_t count = signalConnections.size();
- for (size_t i = 0; i < count; i++) {
- const sp<Connection>& conn(signalConnections[i]);
- // now see if we still need to report this event
- status_t err = conn->postEvent(event);
- if (err == -EAGAIN || err == -EWOULDBLOCK) {
- // The destination doesn't accept events anymore, it's probably
- // full. For now, we just drop the events on the floor.
- // FIXME: Note that some events cannot be dropped and would have
- // to be re-sent later.
- // Right-now we don't have the ability to do this.
- ALOGW("EventThread: dropping event (%08x) for connection %p", event.header.type,
- conn.get());
- } else if (err < 0) {
- // handle any other error on the pipe as fatal. the only
- // reasonable thing to do is to clean-up this connection.
- // The most common error we'll get here is -EPIPE.
- removeDisplayEventConnection(signalConnections[i]);
+ // dispatch events to listeners...
+ const size_t count = signalConnections.size();
+ for (size_t i = 0; i < count; i++) {
+ const sp<Connection>& conn(signalConnections[i]);
+ // now see if we still need to report this event
+ status_t err = conn->postEvent(event);
+ if (err == -EAGAIN || err == -EWOULDBLOCK) {
+ // The destination doesn't accept events anymore, it's probably
+ // full. For now, we just drop the events on the floor.
+ // FIXME: Note that some events cannot be dropped and would have
+ // to be re-sent later.
+ // Right-now we don't have the ability to do this.
+ ALOGW("EventThread: dropping event (%08x) for connection %p", event.header.type,
+ conn.get());
+ } else if (err < 0) {
+ // handle any other error on the pipe as fatal. the only
+ // reasonable thing to do is to clean-up this connection.
+ // The most common error we'll get here is -EPIPE.
+ removeDisplayEventConnection(signalConnections[i]);
+ }
}
}
- return true;
}
// This will return when (1) a vsync event has been received, and (2) there was
// at least one connection interested in receiving it when we started waiting.
-Vector<sp<EventThread::Connection> > EventThread::waitForEvent(DisplayEventReceiver::Event* event) {
- Mutex::Autolock _l(mLock);
+Vector<sp<EventThread::Connection> > EventThread::waitForEventLocked(
+ std::unique_lock<std::mutex>* lock, DisplayEventReceiver::Event* event) {
Vector<sp<EventThread::Connection> > signalConnections;
- do {
+ while (signalConnections.isEmpty() && mKeepRunning) {
bool eventPending = false;
bool waitForVSync = false;
@@ -279,8 +306,8 @@
// use a (long) timeout when waiting for h/w vsync, and
// generate fake events when necessary.
bool softwareSync = mUseSoftwareVSync;
- nsecs_t timeout = softwareSync ? ms2ns(16) : ms2ns(1000);
- if (mCondition.waitRelative(mLock, timeout) == TIMED_OUT) {
+ auto timeout = softwareSync ? 16ms : 1000ms;
+ if (mCondition.wait_for(*lock, timeout) == std::cv_status::timeout) {
if (!softwareSync) {
ALOGW("Timed out waiting for hw vsync; faking it");
}
@@ -296,10 +323,10 @@
// h/w vsync should be disabled, so this will wait until we
// get a new connection, or an existing connection becomes
// interested in receiving vsync again.
- mCondition.wait(mLock);
+ mCondition.wait(*lock);
}
}
- } while (signalConnections.isEmpty());
+ }
// here we're guaranteed to have a timestamp and some connections to signal
// (The connections might have dropped out of mDisplayEventConnections
@@ -328,7 +355,7 @@
}
void EventThread::dump(String8& result) const {
- Mutex::Autolock _l(mLock);
+ std::lock_guard<std::mutex> lock(mMutex);
result.appendFormat("VSYNC state: %s\n", mDebugVsyncEnabled ? "enabled" : "disabled");
result.appendFormat(" soft-vsync: %s\n", mUseSoftwareVSync ? "enabled" : "disabled");
result.appendFormat(" numListeners=%zu,\n events-delivered: %u\n",
@@ -377,4 +404,4 @@
// ---------------------------------------------------------------------------
-}; // namespace android
+} // namespace android
diff --git a/services/surfaceflinger/EventThread.h b/services/surfaceflinger/EventThread.h
index b4d718c..63cdb54 100644
--- a/services/surfaceflinger/EventThread.h
+++ b/services/surfaceflinger/EventThread.h
@@ -14,19 +14,23 @@
* limitations under the License.
*/
-#ifndef ANDROID_SURFACE_FLINGER_EVENT_THREAD_H
-#define ANDROID_SURFACE_FLINGER_EVENT_THREAD_H
+#pragma once
#include <stdint.h>
#include <sys/types.h>
+#include <condition_variable>
+#include <mutex>
+#include <thread>
+
+#include <android-base/thread_annotations.h>
#include <gui/DisplayEventReceiver.h>
#include <gui/IDisplayEventConnection.h>
#include <private/gui/BitTube.h>
#include <utils/Errors.h>
+#include <utils/RefBase.h>
#include <utils/SortedVector.h>
-#include <utils/threads.h>
#include "DisplayDevice.h"
@@ -53,7 +57,7 @@
virtual void setPhaseOffset(nsecs_t phaseOffset) = 0;
};
-class EventThread : public Thread, private VSyncSource::Callback {
+class EventThread : public virtual RefBase, private VSyncSource::Callback {
class Connection : public BnDisplayEventConnection {
public:
explicit Connection(const sp<EventThread>& eventThread);
@@ -75,7 +79,9 @@
};
public:
- EventThread(const sp<VSyncSource>& src, SurfaceFlinger& flinger, bool interceptVSyncs);
+ EventThread(const sp<VSyncSource>& src, SurfaceFlinger& flinger, bool interceptVSyncs,
+ const char* threadName);
+ ~EventThread();
sp<Connection> createEventConnection() const;
status_t registerDisplayEventConnection(const sp<Connection>& connection);
@@ -92,46 +98,46 @@
// called when receiving a hotplug event
void onHotplugReceived(int type, bool connected);
- Vector<sp<EventThread::Connection> > waitForEvent(DisplayEventReceiver::Event* event);
-
void dump(String8& result) const;
void setPhaseOffset(nsecs_t phaseOffset);
private:
- virtual bool threadLoop();
- virtual void onFirstRef();
-
- virtual void onVSyncEvent(nsecs_t timestamp);
+ void threadMain();
+ Vector<sp<EventThread::Connection>> waitForEventLocked(std::unique_lock<std::mutex>* lock,
+ DisplayEventReceiver::Event* event)
+ REQUIRES(mMutex);
void removeDisplayEventConnection(const wp<Connection>& connection);
- void enableVSyncLocked();
- void disableVSyncLocked();
+ void enableVSyncLocked() REQUIRES(mMutex);
+ void disableVSyncLocked() REQUIRES(mMutex);
+
+ // Implements VSyncSource::Callback
+ void onVSyncEvent(nsecs_t timestamp) override;
// constants
- sp<VSyncSource> mVSyncSource;
+ sp<VSyncSource> mVSyncSource GUARDED_BY(mMutex);
SurfaceFlinger& mFlinger;
- mutable Mutex mLock;
- mutable Condition mCondition;
+ std::thread mThread;
+ mutable std::mutex mMutex;
+ mutable std::condition_variable mCondition;
// protected by mLock
- SortedVector<wp<Connection> > mDisplayEventConnections;
- Vector<DisplayEventReceiver::Event> mPendingEvents;
- DisplayEventReceiver::Event mVSyncEvent[DisplayDevice::NUM_BUILTIN_DISPLAY_TYPES];
- bool mUseSoftwareVSync;
- bool mVsyncEnabled;
+ SortedVector<wp<Connection>> mDisplayEventConnections GUARDED_BY(mMutex);
+ Vector<DisplayEventReceiver::Event> mPendingEvents GUARDED_BY(mMutex);
+ DisplayEventReceiver::Event mVSyncEvent[DisplayDevice::NUM_BUILTIN_DISPLAY_TYPES] GUARDED_BY(
+ mMutex);
+ bool mUseSoftwareVSync GUARDED_BY(mMutex) = false;
+ bool mVsyncEnabled GUARDED_BY(mMutex) = false;
+ bool mKeepRunning GUARDED_BY(mMutex) = true;
// for debugging
- bool mDebugVsyncEnabled;
+ bool mDebugVsyncEnabled GUARDED_BY(mMutex) = false;
- const bool mInterceptVSyncs;
+ const bool mInterceptVSyncs = false;
};
// ---------------------------------------------------------------------------
}; // namespace android
-
-// ---------------------------------------------------------------------------
-
-#endif /* ANDROID_SURFACE_FLINGER_EVENT_THREAD_H */
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 70a282d..3774ab1 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -579,22 +579,12 @@
// start the EventThread
sp<VSyncSource> vsyncSrc =
new DispSyncSource(&mPrimaryDispSync, SurfaceFlinger::vsyncPhaseOffsetNs, true, "app");
- mEventThread = new EventThread(vsyncSrc, *this, false);
+ mEventThread = new EventThread(vsyncSrc, *this, false, "appEventThread");
sp<VSyncSource> sfVsyncSrc =
new DispSyncSource(&mPrimaryDispSync, SurfaceFlinger::sfVsyncPhaseOffsetNs, true, "sf");
- mSFEventThread = new EventThread(sfVsyncSrc, *this, true);
+ mSFEventThread = new EventThread(sfVsyncSrc, *this, true, "sfEventThread");
mEventQueue.setEventThread(mSFEventThread);
- // set EventThread and SFEventThread to SCHED_FIFO to minimize jitter
- struct sched_param param = {0};
- param.sched_priority = 2;
- if (sched_setscheduler(mSFEventThread->getTid(), SCHED_FIFO, ¶m) != 0) {
- ALOGE("Couldn't set SCHED_FIFO for SFEventThread");
- }
- if (sched_setscheduler(mEventThread->getTid(), SCHED_FIFO, ¶m) != 0) {
- ALOGE("Couldn't set SCHED_FIFO for EventThread");
- }
-
// Get a RenderEngine for the given display / config (can't fail)
getBE().mRenderEngine = RenderEngine::create(HAL_PIXEL_FORMAT_RGBA_8888,
hasWideColorDisplay ? RenderEngine::WIDE_COLOR_SUPPORT : 0);
@@ -1074,7 +1064,8 @@
ALOGV("VSync Injections enabled");
if (mVSyncInjector.get() == nullptr) {
mVSyncInjector = new InjectVSyncSource();
- mInjectorEventThread = new EventThread(mVSyncInjector, *this, false);
+ mInjectorEventThread = new EventThread(mVSyncInjector, *this, false,
+ "injEventThread");
}
mEventQueue.setEventThread(mInjectorEventThread);
} else {