From eb1caaea6970d2cf870d26cadb40871f6b9784ab Mon Sep 17 00:00:00 2001 From: Andy McFadden Date: Tue, 2 Oct 2012 19:04:45 -0700 Subject: [PATCH 1/2] Fix crashes after HDMI disconnect The display was being removed from SurfaceFlinger's list before we had a chance to reset HWComposer's layer list, so we were passing stale data into the hardware composer (which has its own per-display data). This resulted in "invalid gralloc handle" complaints. We now clear the layer list immediately after removing the display. The display was being removed while its EGLSurface was still "current", resulting in "cancelBuffer: BufferQueue has been abandoned" complaints. We now call makeCurrent on the primary display before removing the external display. Bug 7274254 Change-Id: Ia59e3a61d7ec46488b96bf93ec5e4ed3488b70e4 --- .../DisplayHardware/HWComposer.cpp | 16 ++++++++++++++++ .../surfaceflinger/DisplayHardware/HWComposer.h | 3 +++ services/surfaceflinger/SurfaceFlinger.cpp | 6 ++++++ 3 files changed, 25 insertions(+) diff --git a/services/surfaceflinger/DisplayHardware/HWComposer.cpp b/services/surfaceflinger/DisplayHardware/HWComposer.cpp index fb93c477d..302ce8255 100644 --- a/services/surfaceflinger/DisplayHardware/HWComposer.cpp +++ b/services/surfaceflinger/DisplayHardware/HWComposer.cpp @@ -540,8 +540,13 @@ status_t HWComposer::prepare() { // DO NOT reset the handle field to NULL, because it's possible // that we have nothing to redraw (eg: eglSwapBuffers() not called) // in which case, we should continue to use the same buffer. + LOG_FATAL_IF(disp.list == NULL); disp.framebufferTarget->compositionType = HWC_FRAMEBUFFER_TARGET; } + if (!disp.connected && disp.list != NULL) { + ALOGW("WARNING: disp %d: connected, non-null list, layers=%d", + i, disp.list->numHwLayers); + } mLists[i] = disp.list; if (mLists[i]) { if (hwcHasApiVersion(mHwc, HWC_DEVICE_API_VERSION_1_2)) { @@ -664,6 +669,17 @@ status_t HWComposer::acquire(int disp) const { return NO_ERROR; } +void HWComposer::disconnectDisplay(int disp) { + LOG_ALWAYS_FATAL_IF(disp < 0 || disp == HWC_DISPLAY_PRIMARY || + disp >= HWC_NUM_DISPLAY_TYPES); + DisplayData& dd(mDisplayData[disp]); + if (dd.list != NULL) { + free(dd.list); + dd.list = NULL; + dd.framebufferTarget = NULL; // points into dd.list + } +} + int HWComposer::getVisualID() const { if (mHwc && hwcHasApiVersion(mHwc, HWC_DEVICE_API_VERSION_1_1)) { // FIXME: temporary hack until HAL_PIXEL_FORMAT_IMPLEMENTATION_DEFINED diff --git a/services/surfaceflinger/DisplayHardware/HWComposer.h b/services/surfaceflinger/DisplayHardware/HWComposer.h index 633ca9c13..af33999dc 100644 --- a/services/surfaceflinger/DisplayHardware/HWComposer.h +++ b/services/surfaceflinger/DisplayHardware/HWComposer.h @@ -97,6 +97,9 @@ public: // acquire hardware resources and unblank screen status_t acquire(int disp) const; + // reset state when an external, non-virtual display is disconnected + void disconnectDisplay(int disp); + // create a work list for numLayers layer. sets HWC_GEOMETRY_CHANGED. status_t createWorkList(int32_t id, size_t numLayers); diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index a8d20bb1b..ebec4cb82 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -1123,7 +1123,13 @@ void SurfaceFlinger::handleTransactionLocked(uint32_t transactionFlags) if (j < 0) { // in drawing state but not in current state if (!draw[i].isMainDisplay()) { + // Call makeCurrent() on the primary display so we can + // be sure that nothing associated with this display + // is current. + const sp& hw(getDefaultDisplayDevice()); + DisplayDevice::makeCurrent(mEGLDisplay, hw, mEGLContext); mDisplays.removeItem(draw.keyAt(i)); + getHwComposer().disconnectDisplay(draw[i].type); } else { ALOGW("trying to remove the main display"); } From 947abbdd2cd6bf0143b659415ff67a6940d5c16f Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Thu, 4 Oct 2012 02:34:38 -0700 Subject: [PATCH 2/2] make sure we don't call into the HWC HAL when not needed when enabling/disabling vsync we now make sure to not call into the HAL if the state wouldn't change. Bug: 7274951 Change-Id: Ie24a6d68888a51b577acf9c2a973d85437cbacaf --- .../DisplayHardware/HWComposer.cpp | 48 +++++++++++++------ .../DisplayHardware/HWComposer.h | 13 +++-- services/surfaceflinger/EventThread.cpp | 4 +- services/surfaceflinger/SurfaceFlinger.cpp | 4 +- services/surfaceflinger/SurfaceFlinger.h | 2 +- 5 files changed, 47 insertions(+), 24 deletions(-) diff --git a/services/surfaceflinger/DisplayHardware/HWComposer.cpp b/services/surfaceflinger/DisplayHardware/HWComposer.cpp index 302ce8255..597e5d630 100644 --- a/services/surfaceflinger/DisplayHardware/HWComposer.cpp +++ b/services/surfaceflinger/DisplayHardware/HWComposer.cpp @@ -147,7 +147,7 @@ HWComposer::HWComposer( // don't need a vsync thread if we have a hardware composer needVSyncThread = false; // always turn vsync off when we start - mHwc->eventControl(mHwc, HWC_DISPLAY_PRIMARY, HWC_EVENT_VSYNC, 0); + eventControl(HWC_DISPLAY_PRIMARY, HWC_EVENT_VSYNC, 0); // these IDs are always reserved for (size_t i=0 ; ieventControl(mHwc, HWC_DISPLAY_PRIMARY, HWC_EVENT_VSYNC, 0); + eventControl(HWC_DISPLAY_PRIMARY, HWC_EVENT_VSYNC, 0); } if (mVSyncThread != NULL) { mVSyncThread->requestExitAndWait(); @@ -443,16 +443,32 @@ bool HWComposer::isConnected(int disp) const { return mDisplayData[disp].connected; } -void HWComposer::eventControl(int event, int enabled) { +void HWComposer::eventControl(int disp, int event, int enabled) { + if (uint32_t(disp)>31 || !mAllocatedDisplayIDs.hasBit(disp)) { + return; + } status_t err = NO_ERROR; - if (mHwc) { - if (!mDebugForceFakeVSync) { - err = mHwc->eventControl(mHwc, 0, event, enabled); - // error here should not happen -- not sure what we should - // do if it does. - ALOGE_IF(err, "eventControl(%d, %d) failed %s", - event, enabled, strerror(-err)); + if (mHwc && !mDebugForceFakeVSync) { + // NOTE: we use our own internal lock here because we have to call + // into the HWC with the lock held, and we want to make sure + // that even if HWC blocks (which it shouldn't), it won't + // affect other threads. + Mutex::Autolock _l(mEventControlLock); + const int32_t eventBit = 1UL << event; + const int32_t newValue = enabled ? eventBit : 0; + const int32_t oldValue = mDisplayData[disp].events & eventBit; + if (newValue != oldValue) { + ATRACE_CALL(); + err = mHwc->eventControl(mHwc, disp, event, enabled); + if (!err) { + int32_t& events(mDisplayData[disp].events); + events = (events & ~eventBit) | newValue; + } } + // error here should not happen -- not sure what we should + // do if it does. + ALOGE_IF(err, "eventControl(%d, %d) failed %s", + event, enabled, strerror(-err)); } if (err == NO_ERROR && mVSyncThread != NULL) { @@ -652,16 +668,16 @@ status_t HWComposer::commit() { return (status_t)err; } -status_t HWComposer::release(int disp) const { +status_t HWComposer::release(int disp) { LOG_FATAL_IF(disp >= HWC_NUM_DISPLAY_TYPES); if (mHwc) { - mHwc->eventControl(mHwc, disp, HWC_EVENT_VSYNC, 0); + eventControl(disp, HWC_EVENT_VSYNC, 0); return (status_t)mHwc->blank(mHwc, disp, 1); } return NO_ERROR; } -status_t HWComposer::acquire(int disp) const { +status_t HWComposer::acquire(int disp) { LOG_FATAL_IF(disp >= HWC_NUM_DISPLAY_TYPES); if (mHwc) { return (status_t)mHwc->blank(mHwc, disp, 0); @@ -965,8 +981,10 @@ HWComposer::VSyncThread::VSyncThread(HWComposer& hwc) void HWComposer::VSyncThread::setEnabled(bool enabled) { Mutex::Autolock _l(mLock); - mEnabled = enabled; - mCondition.signal(); + if (mEnabled != enabled) { + mEnabled = enabled; + mCondition.signal(); + } } void HWComposer::VSyncThread::onFirstRef() { diff --git a/services/surfaceflinger/DisplayHardware/HWComposer.h b/services/surfaceflinger/DisplayHardware/HWComposer.h index af33999dc..269e1472a 100644 --- a/services/surfaceflinger/DisplayHardware/HWComposer.h +++ b/services/surfaceflinger/DisplayHardware/HWComposer.h @@ -92,10 +92,10 @@ public: status_t commit(); // release hardware resources and blank screen - status_t release(int disp) const; + status_t release(int disp); // acquire hardware resources and unblank screen - status_t acquire(int disp) const; + status_t acquire(int disp); // reset state when an external, non-virtual display is disconnected void disconnectDisplay(int disp); @@ -226,7 +226,7 @@ public: EVENT_VSYNC = HWC_EVENT_VSYNC }; - void eventControl(int event, int enabled); + void eventControl(int disp, int event, int enabled); // Query display parameters. Pass in a display index (e.g. // HWC_DISPLAY_PRIMARY). @@ -289,7 +289,7 @@ private: DisplayData() : xdpi(0), ydpi(0), refresh(0), connected(false), hasFbComp(false), hasOvComp(false), capacity(0), list(NULL), - framebufferTarget(NULL), fbTargetHandle(NULL) { } + framebufferTarget(NULL), fbTargetHandle(NULL), events(0) { } ~DisplayData() { free(list); } @@ -306,6 +306,8 @@ private: hwc_display_contents_1* list; hwc_layer_1* framebufferTarget; buffer_handle_t fbTargetHandle; + // protected by mEventControlLock + int32_t events; }; sp mFlinger; @@ -327,6 +329,9 @@ private: // protected by mLock mutable Mutex mLock; mutable nsecs_t mLastHwVSync; + + // thread-safe + mutable Mutex mEventControlLock; }; // --------------------------------------------------------------------------- diff --git a/services/surfaceflinger/EventThread.cpp b/services/surfaceflinger/EventThread.cpp index 6b7c78bbd..edb9fa580 100644 --- a/services/surfaceflinger/EventThread.cpp +++ b/services/surfaceflinger/EventThread.cpp @@ -308,14 +308,14 @@ Vector< sp > EventThread::waitForEvent( void EventThread::enableVSyncLocked() { if (!mUseSoftwareVSync) { // never enable h/w VSYNC when screen is off - mFlinger->eventControl(SurfaceFlinger::EVENT_VSYNC, true); + mFlinger->eventControl(HWC_DISPLAY_PRIMARY, SurfaceFlinger::EVENT_VSYNC, true); mPowerHAL.vsyncHint(true); } mDebugVsyncEnabled = true; } void EventThread::disableVSyncLocked() { - mFlinger->eventControl(SurfaceFlinger::EVENT_VSYNC, false); + mFlinger->eventControl(HWC_DISPLAY_PRIMARY, SurfaceFlinger::EVENT_VSYNC, false); mPowerHAL.vsyncHint(false); mDebugVsyncEnabled = false; } diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index ebec4cb82..8160a7aba 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -777,8 +777,8 @@ void SurfaceFlinger::onHotplugReceived(int type, bool connected) { } } -void SurfaceFlinger::eventControl(int event, int enabled) { - getHwComposer().eventControl(event, enabled); +void SurfaceFlinger::eventControl(int disp, int event, int enabled) { + getHwComposer().eventControl(disp, event, enabled); } void SurfaceFlinger::onMessageReceived(int32_t what) { diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 82728484a..5bb370368 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -123,7 +123,7 @@ public: // enable/disable h/w composer event // TODO: this should be made accessible only to EventThread - void eventControl(int event, int enabled); + void eventControl(int disp, int event, int enabled); // called on the main thread by MessageQueue when an internal message // is received