Fix race conditions when using mHwc off the main thread
am: 6d8110b170
Change-Id: I1da28635e48581339f67c01306602605935184b2
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 959d22f..78af35b 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -632,6 +632,8 @@
auto vrFlingerRequestDisplayCallback = [this] (bool requestDisplay) {
ALOGI("VR request display mode: requestDisplay=%d", requestDisplay);
mVrFlingerRequestsDisplay = requestDisplay;
+ ConditionalLock _l(mStateLock,
+ std::this_thread::get_id() != mMainThreadId);
signalTransaction();
};
mVrFlinger = dvr::VrFlinger::Create(mHwc->getComposer(),
@@ -719,6 +721,8 @@
FrameEvent::DEQUEUE_READY,
FrameEvent::RELEASE,
};
+ ConditionalLock _l(mStateLock,
+ std::this_thread::get_id() != mMainThreadId);
if (!getHwComposer().hasCapability(
HWC2::Capability::PresentFenceIsNotReliable)) {
outSupported->push_back(FrameEvent::DISPLAY_PRESENT);
@@ -766,6 +770,8 @@
configs->clear();
+ ConditionalLock _l(mStateLock,
+ std::this_thread::get_id() != mMainThreadId);
for (const auto& hwConfig : getHwComposer().getConfigs(type)) {
DisplayInfo info = DisplayInfo();
@@ -789,7 +795,7 @@
info.density = density;
// TODO: this needs to go away (currently needed only by webkit)
- sp<const DisplayDevice> hw(getDefaultDisplayDevice());
+ sp<const DisplayDevice> hw(getDefaultDisplayDeviceLocked());
info.orientation = hw->getOrientation();
} else {
// TODO: where should this value come from?
@@ -932,7 +938,12 @@
return type;
}
- std::vector<android_color_mode_t> modes = getHwComposer().getColorModes(type);
+ std::vector<android_color_mode_t> modes;
+ {
+ ConditionalLock _l(mStateLock,
+ std::this_thread::get_id() != mMainThreadId);
+ modes = getHwComposer().getColorModes(type);
+ }
outColorModes->clear();
std::copy(modes.cbegin(), modes.cend(), std::back_inserter(*outColorModes));
@@ -1340,7 +1351,7 @@
if (sequenceId != mComposerSequenceId) {
return;
}
- repaintEverything();
+ repaintEverythingLocked();
}
void SurfaceFlinger::setVsyncEnabled(int disp, int enabled) {
@@ -3348,7 +3359,7 @@
mVisibleRegionsDirty = true;
mHasPoweredOff = true;
- repaintEverything();
+ repaintEverythingLocked();
struct sched_param param = {0};
param.sched_priority = 1;
@@ -3992,6 +4003,7 @@
return NO_ERROR;
}
case 1005:{ // force transaction
+ Mutex::Autolock _l(mStateLock);
setTransactionFlags(
eTransactionNeeded|
eDisplayTransactionNeeded|
@@ -4128,11 +4140,17 @@
return err;
}
-void SurfaceFlinger::repaintEverything() {
+void SurfaceFlinger::repaintEverythingLocked() {
android_atomic_or(1, &mRepaintEverything);
signalTransaction();
}
+void SurfaceFlinger::repaintEverything() {
+ ConditionalLock _l(mStateLock,
+ std::this_thread::get_id() != mMainThreadId);
+ repaintEverythingLocked();
+}
+
// Checks that the requested width and height are valid and updates them to the display dimensions
// if they are set to 0
static status_t updateDimensionsLocked(const sp<const DisplayDevice>& displayDevice,
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index 058f4a1..1b77aaf 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -192,6 +192,8 @@
// force full composition on all displays
void repaintEverything();
+ // Can only be called from the main thread or with mStateLock held
+ void repaintEverythingLocked();
// returns the default Display
sp<const DisplayDevice> getDefaultDisplayDevice() const {
@@ -344,7 +346,9 @@
* Message handling
*/
void waitForEvent();
+ // Can only be called from the main thread or with mStateLock held
void signalTransaction();
+ // Can only be called from the main thread or with mStateLock held
void signalLayerUpdate();
void signalRefresh();
@@ -387,6 +391,7 @@
*/
uint32_t getTransactionFlags(uint32_t flags);
uint32_t peekTransactionFlags();
+ // Can only be called from the main thread or with mStateLock held
uint32_t setTransactionFlags(uint32_t flags);
void commitTransaction();
uint32_t setClientStateLocked(const sp<Client>& client, const layer_state_t& s);
@@ -643,8 +648,26 @@
// access must be protected by mInvalidateLock
volatile int32_t mRepaintEverything;
- // The current hardware composer interface. When switching into and out of
- // vr, our HWComposer instance will be recreated.
+ // The current hardware composer interface.
+ //
+ // The following thread safety rules apply when accessing mHwc, either
+ // directly or via getHwComposer():
+ //
+ // 1. When recreating mHwc, acquire mStateLock. We currently recreate mHwc
+ // only when switching into and out of vr. Recreating mHwc must only be
+ // done on the main thread.
+ //
+ // 2. When accessing mHwc on the main thread, it's not necessary to acquire
+ // mStateLock.
+ //
+ // 3. When accessing mHwc on a thread other than the main thread, we always
+ // need to acquire mStateLock. This is because the main thread could be
+ // in the process of destroying the current mHwc instance.
+ //
+ // The above thread safety rules only apply to SurfaceFlinger.cpp. In
+ // SurfaceFlinger_hwc1.cpp we create mHwc at surface flinger init and never
+ // destroy it, so it's always safe to access mHwc from any thread without
+ // acquiring mStateLock.
std::unique_ptr<HWComposer> mHwc;
#ifdef USE_HWC2