From 6d8110b1708171da278782d18886fa1a21971cd9 Mon Sep 17 00:00:00 2001 From: Steven Thomas Date: Thu, 31 Aug 2017 18:24:21 -0700 Subject: [PATCH] Fix race conditions when using mHwc off the main thread In a few cases we were accessing mHwc off the main thread without acquiring mStateLock, resulting in crashes and other incorrect behavior. This CL adds the missing locks. Since the locking semantics are somewhat hard to understand, also add a clarifying comment to the mHwc member declaration in SurfaceFlinger.h. Bug: 64586546 Test: I manually tested normal surface flinger operation and vr behavior on different devices, and confirmed everything looks fine. The crashes we saw that were caused by these mHwc race conditions are hard to reproduce, so I couldn't empirically verify this fixes the crash. I'm relying on manual code inspection to confirm the issue is in fact fixed. Regarding performance, I added profiling code (not part of this CL) to check for lock contention and hold times with the newly added locks. I confirmed that contention is low, so these calls shouldn't be significantly slower as a result of adding the locks. The time spent holding these new locks is also low, except for getDisplayColorModes(), which makes a call to hardware composer service and can in some cases take over a millisecond. That function is called so rarely though, only once at boot, or twice at boot after a fresh flash, that it's not worth optimizing. Change-Id: I3854779c12a61983aaaecddb9f6316f218e519e3 --- services/surfaceflinger/SurfaceFlinger.cpp | 28 +++++++++++++++++++++++----- services/surfaceflinger/SurfaceFlinger.h | 27 +++++++++++++++++++++++++-- 2 files changed, 48 insertions(+), 7 deletions(-) diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 736346497d..12205af919 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -606,6 +606,8 @@ void SurfaceFlinger::init() { 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(), @@ -693,6 +695,8 @@ status_t SurfaceFlinger::getSupportedFrameTimestamps( 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); @@ -740,6 +744,8 @@ status_t SurfaceFlinger::getDisplayConfigs(const sp& display, configs->clear(); + ConditionalLock _l(mStateLock, + std::this_thread::get_id() != mMainThreadId); for (const auto& hwConfig : getHwComposer().getConfigs(type)) { DisplayInfo info = DisplayInfo(); @@ -763,7 +769,7 @@ status_t SurfaceFlinger::getDisplayConfigs(const sp& display, info.density = density; // TODO: this needs to go away (currently needed only by webkit) - sp hw(getDefaultDisplayDevice()); + sp hw(getDefaultDisplayDeviceLocked()); info.orientation = hw->getOrientation(); } else { // TODO: where should this value come from? @@ -906,7 +912,12 @@ status_t SurfaceFlinger::getDisplayColorModes(const sp& display, return type; } - std::vector modes = getHwComposer().getColorModes(type); + std::vector 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)); @@ -1287,7 +1298,7 @@ void SurfaceFlinger::onRefreshReceived(int sequenceId, if (sequenceId != mComposerSequenceId) { return; } - repaintEverything(); + repaintEverythingLocked(); } void SurfaceFlinger::setVsyncEnabled(int disp, int enabled) { @@ -3291,7 +3302,7 @@ void SurfaceFlinger::setPowerModeInternal(const sp& hw, mVisibleRegionsDirty = true; mHasPoweredOff = true; - repaintEverything(); + repaintEverythingLocked(); struct sched_param param = {0}; param.sched_priority = 1; @@ -3935,6 +3946,7 @@ status_t SurfaceFlinger::onTransact( return NO_ERROR; } case 1005:{ // force transaction + Mutex::Autolock _l(mStateLock); setTransactionFlags( eTransactionNeeded| eDisplayTransactionNeeded| @@ -4071,11 +4083,17 @@ status_t SurfaceFlinger::onTransact( 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& displayDevice, diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 025da0e827..13a057f2b6 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -192,6 +192,8 @@ public: // 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 getDefaultDisplayDevice() const { @@ -343,7 +345,9 @@ private: * 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(); @@ -386,6 +390,7 @@ private: */ 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, const layer_state_t& s); @@ -642,8 +647,26 @@ private: // 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 mHwc; // constant members (no synchronization needed for access) -- 2.11.0