Avoid a potential race condition on mDisplays
I've now run this on a HWC1 and HWC2 build. Both appear to be running
correctly.
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
Bug: 37288476
Test: Compiled, installed, and ran with both HWC1 and HWC2 variants.
HWC1 was tested on Nexus 6P. Was able to boot, install apps, run apps,
turn screen on/off, and reboot phone.
HWC2 was tested on sailfish. Was able to boot, install apps, run apps,
run VR apps using both N path, and O1 path, turn screen on/off, and
reboot phone.
Change-Id: I0e80c2553f40cce2116b718bbb0d2566679f794a
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 09a4124..2a43910 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -599,7 +599,7 @@
// 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);
@@ -792,10 +792,12 @@
ALOGE("%s : display is NULL", __func__);
return BAD_VALUE;
}
- sp<DisplayDevice> device(getDisplayDevice(display));
+
+ sp<const DisplayDevice> device(getDisplayDevice(display));
if (device != NULL) {
return device->getActiveConfig();
}
+
return BAD_VALUE;
}
@@ -883,7 +885,7 @@
}
android_color_mode_t SurfaceFlinger::getActiveColorMode(const sp<IBinder>& display) {
- sp<DisplayDevice> device(getDisplayDevice(display));
+ sp<const DisplayDevice> device(getDisplayDevice(display));
if (device != nullptr) {
return device->getActiveColorMode();
}
@@ -965,7 +967,7 @@
HdrCapabilities* outCapabilities) const {
Mutex::Autolock _l(mStateLock);
- sp<const DisplayDevice> displayDevice(getDisplayDevice(display));
+ sp<const DisplayDevice> displayDevice(getDisplayDeviceLocked(display));
if (displayDevice == nullptr) {
ALOGE("getHdrCapabilities: Invalid display %p", displayDevice.get());
return BAD_VALUE;
@@ -1144,54 +1146,59 @@
*compositorTiming = mCompositorTiming;
}
-void SurfaceFlinger::onHotplugReceived(int32_t disp, bool connected) {
+void SurfaceFlinger::createDefaultDisplayDevice() {
+ const int32_t type = DisplayDevice::DISPLAY_PRIMARY;
+ wp<IBinder> token = mBuiltinDisplays[type];
+
+ // All non-virtual displays are currently considered secure.
+ const bool isSecure = true;
+
+ sp<IGraphicBufferProducer> producer;
+ sp<IGraphicBufferConsumer> consumer;
+ BufferQueue::createBufferQueue(&producer, &consumer);
+
+ sp<FramebufferSurface> fbs = new FramebufferSurface(*mHwc, type, consumer);
+
+ bool hasWideColorModes = false;
+ std::vector<android_color_mode_t> 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<DisplayDevice> 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);
+}
+
+void SurfaceFlinger::onHotplugReceived(HWComposer* composer, int32_t disp, bool connected) {
ALOGV("onHotplugReceived(%d, %s)", disp, connected ? "true" : "false");
+
+ 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.");
+ }
+ return;
+ }
+
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;
-
- // 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);
- }
-
- wp<IBinder> token = mBuiltinDisplays[type];
-
- sp<IGraphicBufferProducer> producer;
- sp<IGraphicBufferConsumer> consumer;
- BufferQueue::createBufferQueue(&producer, &consumer);
-
- sp<FramebufferSurface> fbs = new FramebufferSurface(*mHwc,
- DisplayDevice::DISPLAY_PRIMARY, consumer);
-
- bool hasWideColorModes = false;
- std::vector<android_color_mode_t> 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<DisplayDevice> 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);
+ createBuiltinDisplayLocked(DisplayDevice::DISPLAY_PRIMARY);
+ createDefaultDisplayDevice();
} else {
auto type = DisplayDevice::DISPLAY_EXTERNAL;
Mutex::Autolock _l(mStateLock);
@@ -1233,7 +1240,8 @@
}
}
-void SurfaceFlinger::resetHwc() {
+// Note: it is assumed the caller holds |mStateLock| when this is called
+void SurfaceFlinger::resetHwcLocked() {
disableHardwareVsync(true);
clearHwcLayers(mDrawingState.layersSortedByZ);
clearHwcLayers(mCurrentState.layersSortedByZ);
@@ -1253,36 +1261,45 @@
if (vrFlingerRequestsDisplay == mHwc->isUsingVrComposer()) {
return;
}
+
if (vrFlingerRequestsDisplay && !mVrHwc) {
// Construct new HWComposer without holding any locks.
mVrHwc = new HWComposer(true);
+
+ // Set up the event handlers. This step is neccessary to initialize the internal state of
+ // the hardware composer object properly. Our callbacks are designed such that if they are
+ // triggered between now and the point where the display is properly re-initialized, they
+ // will not have any effect, so this is safe to do here, before the lock is aquired.
+ mVrHwc->setEventHandler(static_cast<HWComposer::EventHandler*>(this));
ALOGV("Vr HWC created");
}
- {
- Mutex::Autolock _l(mStateLock);
- if (vrFlingerRequestsDisplay) {
- resetHwc();
+ Mutex::Autolock _l(mStateLock);
- mHwc = mVrHwc;
- mVrFlinger->GrantDisplayOwnership();
- } else {
- mVrFlinger->SeizeDisplayOwnership();
+ if (vrFlingerRequestsDisplay) {
+ resetHwcLocked();
- resetHwc();
+ mHwc = mVrHwc;
+ mVrFlinger->GrantDisplayOwnership();
- mHwc = mRealHwc;
- enableHardwareVsync();
- }
+ } else {
+ mVrFlinger->SeizeDisplayOwnership();
- mVisibleRegionsDirty = true;
- invalidateHwcGeometry();
- android_atomic_or(1, &mRepaintEverything);
- setTransactionFlags(eDisplayTransactionNeeded);
+ resetHwcLocked();
+
+ mHwc = mRealHwc;
+ enableHardwareVsync();
}
- if (mVrHwc) {
- mVrHwc->setEventHandler(static_cast<HWComposer::EventHandler*>(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) {
@@ -1492,7 +1509,8 @@
layer->releasePendingBuffer(dequeueReadyTime);
}
- const sp<const DisplayDevice> hw(getDefaultDisplayDevice());
+ // |mStateLock| not needed as we are on the main thread
+ const sp<const DisplayDevice> hw(getDefaultDisplayDeviceLocked());
std::shared_ptr<FenceTime> glCompositionDoneFenceTime;
if (mHwc->hasClientComposition(HWC_DISPLAY_PRIMARY)) {
@@ -1840,7 +1858,8 @@
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();
}
@@ -1925,9 +1944,9 @@
// Call makeCurrent() on the primary display so we can
// be sure that nothing associated with this display
// is current.
- const sp<const DisplayDevice> defaultDisplay(getDefaultDisplayDevice());
+ const sp<const DisplayDevice> defaultDisplay(getDefaultDisplayDeviceLocked());
defaultDisplay->makeCurrent(mEGLDisplay, mEGLContext);
- sp<DisplayDevice> hw(getDisplayDevice(draw.keyAt(i)));
+ sp<DisplayDevice> hw(getDisplayDeviceLocked(draw.keyAt(i)));
if (hw != NULL)
hw->disconnect(getHwComposer());
if (draw[i].type < DisplayDevice::NUM_BUILTIN_DISPLAY_TYPES)
@@ -1947,7 +1966,7 @@
// recreating the DisplayDevice, so we just remove it
// from the drawing state, so that it get re-added
// below.
- sp<DisplayDevice> hw(getDisplayDevice(display));
+ sp<DisplayDevice> hw(getDisplayDeviceLocked(display));
if (hw != NULL)
hw->disconnect(getHwComposer());
mDisplays.removeItem(display);
@@ -1957,7 +1976,7 @@
continue;
}
- const sp<DisplayDevice> disp(getDisplayDevice(display));
+ const sp<DisplayDevice> disp(getDisplayDeviceLocked(display));
if (disp != NULL) {
if (state.layerStack != draw[i].layerStack) {
disp->setLayerStack(state.layerStack);
@@ -2114,7 +2133,7 @@
// 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);
@@ -2470,7 +2489,9 @@
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;
@@ -3562,7 +3583,7 @@
colorizer.reset(result);
HWComposer& hwc(getHwComposer());
- sp<const DisplayDevice> hw(getDefaultDisplayDevice());
+ sp<const DisplayDevice> hw(getDefaultDisplayDeviceLocked());
colorizer.bold(result);
result.appendFormat("EGL implementation : %s\n",
@@ -3656,7 +3677,7 @@
// Just use the primary display so we have something to return
dpy = getBuiltInDisplay(DisplayDevice::DISPLAY_PRIMARY);
}
- return getDisplayDevice(dpy)->getVisibleLayersSortedByZ();
+ return getDisplayDeviceLocked(dpy)->getVisibleLayersSortedByZ();
}
bool SurfaceFlinger::startDdmConnection()
@@ -3791,7 +3812,6 @@
reply->writeInt32(mDebugDisableHWC);
return NO_ERROR;
case 1013: {
- Mutex::Autolock _l(mStateLock);
sp<const DisplayDevice> hw(getDefaultDisplayDevice());
reply->writeInt32(hw->getPageFlipCount());
return NO_ERROR;
@@ -4078,7 +4098,7 @@
}
virtual bool handler() {
Mutex::Autolock _l(flinger->mStateLock);
- sp<const DisplayDevice> hw(flinger->getDisplayDevice(display));
+ sp<const DisplayDevice> hw(flinger->getDisplayDeviceLocked(display));
result = flinger->captureScreenImplLocked(hw, producer,
sourceCrop, reqWidth, reqHeight, minLayerZ, maxLayerZ,
useIdentityTransform, rotation, isLocalScreenshot);