From 8722a310c00557195a0703e95564f31d908ab2d5 Mon Sep 17 00:00:00 2001 From: Tomasz Wasilczyk Date: Thu, 13 Apr 2017 19:14:30 +0000 Subject: [PATCH] Revert "Avoid a potential race condition on mDisplays" This reverts commit 8d6c16dc3dc8b88a0046f53668a4e3be074507ff. Bug: b/37282502 Change-Id: Ibf64607f9e14ede201510e2c1b502c49a31e9f2a --- .../surfaceflinger/DisplayHardware/HWComposer.cpp | 2 +- .../surfaceflinger/DisplayHardware/HWComposer.h | 2 +- .../DisplayHardware/HWComposer_hwc1.cpp | 2 +- .../DisplayHardware/HWComposer_hwc1.h | 2 +- services/surfaceflinger/SurfaceFlinger.cpp | 171 +++++++++------------ services/surfaceflinger/SurfaceFlinger.h | 13 +- services/surfaceflinger/SurfaceFlinger_hwc1.cpp | 6 +- 7 files changed, 82 insertions(+), 116 deletions(-) diff --git a/services/surfaceflinger/DisplayHardware/HWComposer.cpp b/services/surfaceflinger/DisplayHardware/HWComposer.cpp index 32739b6029..09434f68fa 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(this, disp, + mEventHandler->onHotplugReceived(disp, connected == HWC2::Connection::Connected); } diff --git a/services/surfaceflinger/DisplayHardware/HWComposer.h b/services/surfaceflinger/DisplayHardware/HWComposer.h index 78d03076c0..81f1619435 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(HWComposer* composer, int32_t disp, bool connected) = 0; + virtual void onHotplugReceived(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 dcb29135b9..5b5f1cfe73 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(this, disp, bool(connected)); + mEventHandler.onHotplugReceived(disp, bool(connected)); } } diff --git a/services/surfaceflinger/DisplayHardware/HWComposer_hwc1.h b/services/surfaceflinger/DisplayHardware/HWComposer_hwc1.h index 4bc63bbac8..f64d69a86a 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(HWComposer* composer, int disp, bool connected) = 0; + virtual void onHotplugReceived(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 76a5d060e6..26baaaecb2 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) - getDefaultDisplayDeviceLocked()->makeCurrent(mEGLDisplay, mEGLContext); + getDefaultDisplayDevice()->makeCurrent(mEGLDisplay, mEGLContext); mEventControlThread = new EventControlThread(this); mEventControlThread->run("EventControl", PRIORITY_URGENT_DISPLAY); @@ -714,11 +714,8 @@ status_t SurfaceFlinger::getDisplayConfigs(const sp& display, info.density = density; // TODO: this needs to go away (currently needed only by webkit) - { - Mutex::Autolock _l(mStateLock); - sp hw(getDefaultDisplayDeviceLocked()); - info.orientation = hw->getOrientation(); - } + sp hw(getDefaultDisplayDevice()); + info.orientation = hw->getOrientation(); } else { // TODO: where should this value come from? static const int TV_DENSITY = 213; @@ -775,13 +772,10 @@ 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; } @@ -869,7 +863,6 @@ 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(); @@ -1131,60 +1124,55 @@ void SurfaceFlinger::getCompositorTiming(CompositorTiming* compositorTiming) { *compositorTiming = mCompositorTiming; } -void SurfaceFlinger::createDefaultDisplayDevice() { - const int32_t type = DisplayDevice::DISPLAY_PRIMARY; - wp token = mBuiltinDisplays[type]; +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. - 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); -} + // All non-virtual displays are currently considered secure. + bool isSecure = true; -void SurfaceFlinger::onHotplugReceived(HWComposer* composer, int32_t disp, bool connected) { - ALOGV("onHotplugReceived(%d, %s)", disp, connected ? "true" : "false"); + int32_t type = DisplayDevice::DISPLAY_PRIMARY; - 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."); + // 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); } - return; - } - if (disp == DisplayDevice::DISPLAY_PRIMARY) { - Mutex::Autolock lock(mStateLock); - createBuiltinDisplayLocked(DisplayDevice::DISPLAY_PRIMARY); - createDefaultDisplayDevice(); + wp token = mBuiltinDisplays[type]; + + 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; + } + } + 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); } else { auto type = DisplayDevice::DISPLAY_EXTERNAL; Mutex::Autolock _l(mStateLock); @@ -1226,7 +1214,6 @@ 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); @@ -1247,46 +1234,36 @@ 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); - Mutex::Autolock _l(mStateLock); + if (vrFlingerRequestsDisplay) { + resetHwc(); - if (vrFlingerRequestsDisplay) { - resetHwc(); + mHwc = mVrHwc; + mVrFlinger->GrantDisplayOwnership(); + } else { + mVrFlinger->SeizeDisplayOwnership(); - mHwc = mVrHwc; - mVrFlinger->GrantDisplayOwnership(); + resetHwc(); - if (vrHwcNewlyInitialized) { - mVrHwc->setEventHandler( - static_cast(this)); + mHwc = mRealHwc; + enableHardwareVsync(); } - } else { - mVrFlinger->SeizeDisplayOwnership(); - - resetHwc(); - mHwc = mRealHwc; - enableHardwareVsync(); + mVisibleRegionsDirty = true; + invalidateHwcGeometry(); + android_atomic_or(1, &mRepaintEverything); + setTransactionFlags(eDisplayTransactionNeeded); + } + if (mVrHwc) { + mVrHwc->setEventHandler(static_cast(this)); } - - 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) { @@ -1496,8 +1473,7 @@ void SurfaceFlinger::postComposition(nsecs_t refreshStartTime) layer->releasePendingBuffer(dequeueReadyTime); } - // |mStateLock| not needed as we are on the main thread - const sp hw(getDefaultDisplayDeviceLocked()); + const sp hw(getDefaultDisplayDevice()); std::shared_ptr glCompositionDoneFenceTime; if (mHwc->hasClientComposition(HWC_DISPLAY_PRIMARY)) { @@ -1845,8 +1821,7 @@ void SurfaceFlinger::postFramebuffer() mLastSwapBufferTime = systemTime() - now; mDebugInSwapBuffers = 0; - // |mStateLock| not needed as we are on the main thread - uint32_t flipCount = getDefaultDisplayDeviceLocked()->getPageFlipCount(); + uint32_t flipCount = getDefaultDisplayDevice()->getPageFlipCount(); if (flipCount % LOG_FRAME_STATS_PERIOD == 0) { logFrameStats(); } @@ -1931,7 +1906,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(getDefaultDisplayDeviceLocked()); + const sp defaultDisplay(getDefaultDisplayDevice()); defaultDisplay->makeCurrent(mEGLDisplay, mEGLContext); sp hw(getDisplayDevice(draw.keyAt(i))); if (hw != NULL) @@ -2121,7 +2096,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 = getDefaultDisplayDeviceLocked(); + disp = getDefaultDisplayDevice(); } layer->updateTransformHint(disp); @@ -2477,9 +2452,7 @@ 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); - - // |mStateLock| not needed as we are on the main thread - if(!getDefaultDisplayDeviceLocked()->makeCurrent(mEGLDisplay, mEGLContext)) { + if(!getDefaultDisplayDevice()->makeCurrent(mEGLDisplay, mEGLContext)) { ALOGE("DisplayDevice::makeCurrent on default display failed. Aborting."); } return false; @@ -3566,7 +3539,7 @@ void SurfaceFlinger::dumpAllLocked(const Vector& args, size_t& index, colorizer.reset(result); HWComposer& hwc(getHwComposer()); - sp hw(getDefaultDisplayDeviceLocked()); + sp hw(getDefaultDisplayDevice()); colorizer.bold(result); result.appendFormat("EGL implementation : %s\n", @@ -3796,7 +3769,7 @@ status_t SurfaceFlinger::onTransact( return NO_ERROR; case 1013: { Mutex::Autolock _l(mStateLock); - sp hw(getDefaultDisplayDeviceLocked()); + sp hw(getDefaultDisplayDevice()); reply->writeInt32(hw->getPageFlipCount()); return NO_ERROR; } diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 219d662bbb..46121cfa63 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -184,9 +184,8 @@ public: void repaintEverything(); // returns the default Display - sp getDefaultDisplayDevice() { - Mutex::Autolock _l(mStateLock); - return getDefaultDisplayDeviceLocked(); + sp getDefaultDisplayDevice() const { + return getDisplayDevice(mBuiltinDisplays[DisplayDevice::DISPLAY_PRIMARY]); } // utility function to delete a texture on the main thread @@ -305,7 +304,7 @@ private: * HWComposer::EventHandler interface */ virtual void onVSyncReceived(HWComposer* composer, int type, nsecs_t timestamp); - virtual void onHotplugReceived(HWComposer* composer, int disp, bool connected); + virtual void onHotplugReceived(int disp, bool connected); virtual void onInvalidateReceived(HWComposer* composer); /* ------------------------------------------------------------------------ @@ -440,12 +439,6 @@ 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 cd2acefbcb..e640ef71ee 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) - getDefaultDisplayDeviceLocked()->makeCurrent(mEGLDisplay, mEGLContext); + getDefaultDisplayDevice()->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(HWComposer* /*composer*/, int type, bool connected) { +void SurfaceFlinger::onHotplugReceived(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(getDefaultDisplayDeviceLocked()); + sp hw(getDefaultDisplayDevice()); colorizer.bold(result); result.appendFormat("EGL implementation : %s\n", -- 2.11.0