From: Stephen Kiazyk Date: Wed, 5 Apr 2017 23:46:49 +0000 (-0700) Subject: Avoid a potential race condition on mDisplays X-Git-Tag: android-x86-9.0-r1~523^2~108^2~1 X-Git-Url: http://git.osdn.net/view?a=commitdiff_plain;h=8d6c16dc3dc8b88a0046f53668a4e3be074507ff;p=android-x86%2Fframeworks-native.git Avoid a potential race condition on mDisplays Update: The HWC1 path needed to be updated in light of a change to SurfaceFlinger.h. The build now works for both paths. Original Message: The race could occur when transitioning in/out of VR flinger mode. It is now avoided by ensuring that the primary |DisplayDevice| is always created once |mStateLock| is released, and ensuring that all accesses to the primary |DisplayDevice| are guarded by |mStateLock|. Bug: 36194616 Bug: 37249613 Test: Compiled for both HWC1 and HWC2 surface flinger builds. Normal behavior is unchanged. Explicitly testing the race condition required instrumenting code with sleep statements. Change-Id: I0b00e857a3b2fd01948a888db2f0715d2a8204c1 --- diff --git a/services/surfaceflinger/DisplayHardware/HWComposer.cpp b/services/surfaceflinger/DisplayHardware/HWComposer.cpp index 09434f68fa..32739b6029 100644 --- a/services/surfaceflinger/DisplayHardware/HWComposer.cpp +++ b/services/surfaceflinger/DisplayHardware/HWComposer.cpp @@ -206,7 +206,7 @@ void HWComposer::hotplug(const std::shared_ptr& display, } disp = DisplayDevice::DISPLAY_EXTERNAL; } - mEventHandler->onHotplugReceived(disp, + mEventHandler->onHotplugReceived(this, disp, connected == HWC2::Connection::Connected); } diff --git a/services/surfaceflinger/DisplayHardware/HWComposer.h b/services/surfaceflinger/DisplayHardware/HWComposer.h index 81f1619435..78d03076c0 100644 --- a/services/surfaceflinger/DisplayHardware/HWComposer.h +++ b/services/surfaceflinger/DisplayHardware/HWComposer.h @@ -70,7 +70,7 @@ public: friend class HWComposer; virtual void onVSyncReceived( HWComposer* composer, int32_t disp, nsecs_t timestamp) = 0; - virtual void onHotplugReceived(int32_t disp, bool connected) = 0; + virtual void onHotplugReceived(HWComposer* composer, int32_t disp, bool connected) = 0; virtual void onInvalidateReceived(HWComposer* composer) = 0; protected: virtual ~EventHandler() {} diff --git a/services/surfaceflinger/DisplayHardware/HWComposer_hwc1.cpp b/services/surfaceflinger/DisplayHardware/HWComposer_hwc1.cpp index 5b5f1cfe73..dcb29135b9 100644 --- a/services/surfaceflinger/DisplayHardware/HWComposer_hwc1.cpp +++ b/services/surfaceflinger/DisplayHardware/HWComposer_hwc1.cpp @@ -315,7 +315,7 @@ void HWComposer::hotplug(int disp, int connected) { queryDisplayProperties(disp); // Do not teardown or recreate the primary display if (disp != HWC_DISPLAY_PRIMARY) { - mEventHandler.onHotplugReceived(disp, bool(connected)); + mEventHandler.onHotplugReceived(this, disp, bool(connected)); } } diff --git a/services/surfaceflinger/DisplayHardware/HWComposer_hwc1.h b/services/surfaceflinger/DisplayHardware/HWComposer_hwc1.h index f64d69a86a..4bc63bbac8 100644 --- a/services/surfaceflinger/DisplayHardware/HWComposer_hwc1.h +++ b/services/surfaceflinger/DisplayHardware/HWComposer_hwc1.h @@ -62,7 +62,7 @@ public: friend class HWComposer; virtual void onVSyncReceived( HWComposer* composer, int32_t disp, nsecs_t timestamp) = 0; - virtual void onHotplugReceived(int disp, bool connected) = 0; + virtual void onHotplugReceived(HWComposer* composer, int disp, bool connected) = 0; virtual void onInvalidateReceived(HWComposer* composer) = 0; protected: virtual ~EventHandler() {} diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 26baaaecb2..76a5d060e6 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -598,7 +598,7 @@ void SurfaceFlinger::init() { // make the GLContext current so that we can create textures when creating // Layers (which may happens before we render something) - getDefaultDisplayDevice()->makeCurrent(mEGLDisplay, mEGLContext); + getDefaultDisplayDeviceLocked()->makeCurrent(mEGLDisplay, mEGLContext); mEventControlThread = new EventControlThread(this); mEventControlThread->run("EventControl", PRIORITY_URGENT_DISPLAY); @@ -714,8 +714,11 @@ status_t SurfaceFlinger::getDisplayConfigs(const sp& display, info.density = density; // TODO: this needs to go away (currently needed only by webkit) - sp hw(getDefaultDisplayDevice()); - info.orientation = hw->getOrientation(); + { + Mutex::Autolock _l(mStateLock); + sp hw(getDefaultDisplayDeviceLocked()); + info.orientation = hw->getOrientation(); + } } else { // TODO: where should this value come from? static const int TV_DENSITY = 213; @@ -772,10 +775,13 @@ int SurfaceFlinger::getActiveConfig(const sp& display) { ALOGE("%s : display is NULL", __func__); return BAD_VALUE; } + + Mutex::Autolock _l(mStateLock); sp device(getDisplayDevice(display)); if (device != NULL) { return device->getActiveConfig(); } + return BAD_VALUE; } @@ -863,6 +869,7 @@ status_t SurfaceFlinger::getDisplayColorModes(const sp& display, } android_color_mode_t SurfaceFlinger::getActiveColorMode(const sp& display) { + Mutex::Autolock _l(mStateLock); sp device(getDisplayDevice(display)); if (device != nullptr) { return device->getActiveColorMode(); @@ -1124,55 +1131,60 @@ void SurfaceFlinger::getCompositorTiming(CompositorTiming* compositorTiming) { *compositorTiming = mCompositorTiming; } -void SurfaceFlinger::onHotplugReceived(int32_t disp, bool connected) { - ALOGV("onHotplugReceived(%d, %s)", disp, connected ? "true" : "false"); - if (disp == DisplayDevice::DISPLAY_PRIMARY) { - Mutex::Autolock lock(mStateLock); - - // All non-virtual displays are currently considered secure. - bool isSecure = true; - - int32_t type = DisplayDevice::DISPLAY_PRIMARY; +void SurfaceFlinger::createDefaultDisplayDevice() { + const int32_t type = DisplayDevice::DISPLAY_PRIMARY; + wp token = mBuiltinDisplays[type]; - // When we're using the vr composer, the assumption is that we've - // already created the IBinder object for the primary display. - if (!mHwc->isUsingVrComposer()) { - createBuiltinDisplayLocked(DisplayDevice::DISPLAY_PRIMARY); + // All non-virtual displays are currently considered secure. + const bool isSecure = true; + + sp producer; + sp consumer; + BufferQueue::createBufferQueue(&producer, &consumer, new GraphicBufferAlloc()); + + sp fbs = new FramebufferSurface(*mHwc, type, consumer); + + bool hasWideColorModes = false; + std::vector modes = getHwComposer().getColorModes( + type); + for (android_color_mode_t colorMode : modes) { + switch (colorMode) { + case HAL_COLOR_MODE_DISPLAY_P3: + case HAL_COLOR_MODE_ADOBE_RGB: + case HAL_COLOR_MODE_DCI_P3: + hasWideColorModes = true; + break; + default: + break; } + } + sp hw = new DisplayDevice(this, DisplayDevice::DISPLAY_PRIMARY, type, isSecure, + token, fbs, producer, mRenderEngine->getEGLConfig(), + hasWideColorModes && hasWideColorDisplay); + mDisplays.add(token, hw); + android_color_mode defaultColorMode = HAL_COLOR_MODE_NATIVE; + if (hasWideColorModes && hasWideColorDisplay) { + defaultColorMode = HAL_COLOR_MODE_SRGB; + } + setActiveColorModeInternal(hw, defaultColorMode); +} - wp token = mBuiltinDisplays[type]; +void SurfaceFlinger::onHotplugReceived(HWComposer* composer, int32_t disp, bool connected) { + ALOGV("onHotplugReceived(%d, %s)", disp, connected ? "true" : "false"); - sp producer; - sp consumer; - BufferQueue::createBufferQueue(&producer, &consumer, - new GraphicBufferAlloc()); - - sp fbs = new FramebufferSurface(*mHwc, - DisplayDevice::DISPLAY_PRIMARY, consumer); - - bool hasWideColorModes = false; - std::vector modes = getHwComposer().getColorModes(type); - for (android_color_mode_t colorMode : modes) { - switch (colorMode) { - case HAL_COLOR_MODE_DISPLAY_P3: - case HAL_COLOR_MODE_ADOBE_RGB: - case HAL_COLOR_MODE_DCI_P3: - hasWideColorModes = true; - break; - default: - break; - } + if (composer->isUsingVrComposer()) { + // We handle initializing the primary display device for the VR + // window manager hwc explicitly at the time of transition. + if (disp != DisplayDevice::DISPLAY_PRIMARY) { + ALOGE("External displays are not supported by the vr hardware composer."); } - sp hw = - new DisplayDevice(this, DisplayDevice::DISPLAY_PRIMARY, disp, isSecure, token, fbs, - producer, mRenderEngine->getEGLConfig(), - hasWideColorModes && hasWideColorDisplay); - mDisplays.add(token, hw); - android_color_mode defaultColorMode = HAL_COLOR_MODE_NATIVE; - if (hasWideColorModes && hasWideColorDisplay) { - defaultColorMode = HAL_COLOR_MODE_SRGB; - } - setActiveColorModeInternal(hw, defaultColorMode); + return; + } + + if (disp == DisplayDevice::DISPLAY_PRIMARY) { + Mutex::Autolock lock(mStateLock); + createBuiltinDisplayLocked(DisplayDevice::DISPLAY_PRIMARY); + createDefaultDisplayDevice(); } else { auto type = DisplayDevice::DISPLAY_EXTERNAL; Mutex::Autolock _l(mStateLock); @@ -1214,6 +1226,7 @@ void SurfaceFlinger::clearHwcLayers(const LayerVector& layers) { } } +// Note: it is assumed the caller holds |mStateLock| when this is called void SurfaceFlinger::resetHwc() { disableHardwareVsync(true); clearHwcLayers(mDrawingState.layersSortedByZ); @@ -1234,36 +1247,46 @@ void SurfaceFlinger::updateVrFlinger() { if (vrFlingerRequestsDisplay == mHwc->isUsingVrComposer()) { return; } + + bool vrHwcNewlyInitialized = false; + if (vrFlingerRequestsDisplay && !mVrHwc) { // Construct new HWComposer without holding any locks. mVrHwc = new HWComposer(true); + vrHwcNewlyInitialized = true; ALOGV("Vr HWC created"); } - { - Mutex::Autolock _l(mStateLock); - if (vrFlingerRequestsDisplay) { - resetHwc(); + Mutex::Autolock _l(mStateLock); - mHwc = mVrHwc; - mVrFlinger->GrantDisplayOwnership(); - } else { - mVrFlinger->SeizeDisplayOwnership(); + if (vrFlingerRequestsDisplay) { + resetHwc(); - resetHwc(); + mHwc = mVrHwc; + mVrFlinger->GrantDisplayOwnership(); - mHwc = mRealHwc; - enableHardwareVsync(); + if (vrHwcNewlyInitialized) { + mVrHwc->setEventHandler( + static_cast(this)); } + } else { + mVrFlinger->SeizeDisplayOwnership(); - mVisibleRegionsDirty = true; - invalidateHwcGeometry(); - android_atomic_or(1, &mRepaintEverything); - setTransactionFlags(eDisplayTransactionNeeded); - } - if (mVrHwc) { - mVrHwc->setEventHandler(static_cast(this)); + resetHwc(); + + mHwc = mRealHwc; + enableHardwareVsync(); } + + mVisibleRegionsDirty = true; + invalidateHwcGeometry(); + + // Explicitly re-initialize the primary display. This is because some other + // parts of this class rely on the primary display always being available. + createDefaultDisplayDevice(); + + android_atomic_or(1, &mRepaintEverything); + setTransactionFlags(eDisplayTransactionNeeded); } void SurfaceFlinger::onMessageReceived(int32_t what) { @@ -1473,7 +1496,8 @@ void SurfaceFlinger::postComposition(nsecs_t refreshStartTime) layer->releasePendingBuffer(dequeueReadyTime); } - const sp hw(getDefaultDisplayDevice()); + // |mStateLock| not needed as we are on the main thread + const sp hw(getDefaultDisplayDeviceLocked()); std::shared_ptr glCompositionDoneFenceTime; if (mHwc->hasClientComposition(HWC_DISPLAY_PRIMARY)) { @@ -1821,7 +1845,8 @@ void SurfaceFlinger::postFramebuffer() mLastSwapBufferTime = systemTime() - now; mDebugInSwapBuffers = 0; - uint32_t flipCount = getDefaultDisplayDevice()->getPageFlipCount(); + // |mStateLock| not needed as we are on the main thread + uint32_t flipCount = getDefaultDisplayDeviceLocked()->getPageFlipCount(); if (flipCount % LOG_FRAME_STATS_PERIOD == 0) { logFrameStats(); } @@ -1906,7 +1931,7 @@ void SurfaceFlinger::handleTransactionLocked(uint32_t transactionFlags) // Call makeCurrent() on the primary display so we can // be sure that nothing associated with this display // is current. - const sp defaultDisplay(getDefaultDisplayDevice()); + const sp defaultDisplay(getDefaultDisplayDeviceLocked()); defaultDisplay->makeCurrent(mEGLDisplay, mEGLContext); sp hw(getDisplayDevice(draw.keyAt(i))); if (hw != NULL) @@ -2096,7 +2121,7 @@ void SurfaceFlinger::handleTransactionLocked(uint32_t transactionFlags) // could be null when this layer is using a layerStack // that is not visible on any display. Also can occur at // screen off/on times. - disp = getDefaultDisplayDevice(); + disp = getDefaultDisplayDeviceLocked(); } layer->updateTransformHint(disp); @@ -2452,7 +2477,9 @@ bool SurfaceFlinger::doComposeSurfaces( ALOGW("DisplayDevice::makeCurrent failed. Aborting surface composition for display %s", displayDevice->getDisplayName().string()); eglMakeCurrent(mEGLDisplay, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT); - if(!getDefaultDisplayDevice()->makeCurrent(mEGLDisplay, mEGLContext)) { + + // |mStateLock| not needed as we are on the main thread + if(!getDefaultDisplayDeviceLocked()->makeCurrent(mEGLDisplay, mEGLContext)) { ALOGE("DisplayDevice::makeCurrent on default display failed. Aborting."); } return false; @@ -3539,7 +3566,7 @@ void SurfaceFlinger::dumpAllLocked(const Vector& args, size_t& index, colorizer.reset(result); HWComposer& hwc(getHwComposer()); - sp hw(getDefaultDisplayDevice()); + sp hw(getDefaultDisplayDeviceLocked()); colorizer.bold(result); result.appendFormat("EGL implementation : %s\n", @@ -3769,7 +3796,7 @@ status_t SurfaceFlinger::onTransact( return NO_ERROR; case 1013: { Mutex::Autolock _l(mStateLock); - sp hw(getDefaultDisplayDevice()); + sp hw(getDefaultDisplayDeviceLocked()); reply->writeInt32(hw->getPageFlipCount()); return NO_ERROR; } diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 46121cfa63..219d662bbb 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -184,8 +184,9 @@ public: void repaintEverything(); // returns the default Display - sp getDefaultDisplayDevice() const { - return getDisplayDevice(mBuiltinDisplays[DisplayDevice::DISPLAY_PRIMARY]); + sp getDefaultDisplayDevice() { + Mutex::Autolock _l(mStateLock); + return getDefaultDisplayDeviceLocked(); } // utility function to delete a texture on the main thread @@ -304,7 +305,7 @@ private: * HWComposer::EventHandler interface */ virtual void onVSyncReceived(HWComposer* composer, int type, nsecs_t timestamp); - virtual void onHotplugReceived(int disp, bool connected); + virtual void onHotplugReceived(HWComposer* composer, int disp, bool connected); virtual void onInvalidateReceived(HWComposer* composer); /* ------------------------------------------------------------------------ @@ -439,6 +440,12 @@ private: return mDisplays.valueFor(dpy); } + sp getDefaultDisplayDeviceLocked() const { + return getDisplayDevice(mBuiltinDisplays[DisplayDevice::DISPLAY_PRIMARY]); + } + + void createDefaultDisplayDevice(); + int32_t getDisplayType(const sp& display) { if (!display.get()) return NAME_NOT_FOUND; for (int i = 0; i < DisplayDevice::NUM_BUILTIN_DISPLAY_TYPES; ++i) { diff --git a/services/surfaceflinger/SurfaceFlinger_hwc1.cpp b/services/surfaceflinger/SurfaceFlinger_hwc1.cpp index e640ef71ee..cd2acefbcb 100644 --- a/services/surfaceflinger/SurfaceFlinger_hwc1.cpp +++ b/services/surfaceflinger/SurfaceFlinger_hwc1.cpp @@ -584,7 +584,7 @@ void SurfaceFlinger::init() { // make the GLContext current so that we can create textures when creating Layers // (which may happens before we render something) - getDefaultDisplayDevice()->makeCurrent(mEGLDisplay, mEGLContext); + getDefaultDisplayDeviceLocked()->makeCurrent(mEGLDisplay, mEGLContext); mEventControlThread = new EventControlThread(this); mEventControlThread->run("EventControl", PRIORITY_URGENT_DISPLAY); @@ -1040,7 +1040,7 @@ void SurfaceFlinger::getCompositorTiming(CompositorTiming* compositorTiming) { *compositorTiming = mCompositorTiming; } -void SurfaceFlinger::onHotplugReceived(int type, bool connected) { +void SurfaceFlinger::onHotplugReceived(HWComposer* /*composer*/, int type, bool connected) { if (mEventThread == NULL) { // This is a temporary workaround for b/7145521. A non-null pointer // does not mean EventThread has finished initializing, so this @@ -3222,7 +3222,7 @@ void SurfaceFlinger::dumpAllLocked(const Vector& args, size_t& index, colorizer.reset(result); HWComposer& hwc(getHwComposer()); - sp hw(getDefaultDisplayDevice()); + sp hw(getDefaultDisplayDeviceLocked()); colorizer.bold(result); result.appendFormat("EGL implementation : %s\n",