diff options
-rw-r--r-- | libs/gui/BLASTBufferQueue.cpp | 28 | ||||
-rw-r--r-- | libs/gui/SurfaceComposerClient.cpp | 6 | ||||
-rw-r--r-- | libs/gui/include/gui/BLASTBufferQueue.h | 9 | ||||
-rw-r--r-- | libs/gui/include/gui/SurfaceComposerClient.h | 7 | ||||
-rw-r--r-- | libs/gui/include/gui/test/CallbackUtils.h | 3 | ||||
-rw-r--r-- | services/inputflinger/reader/InputDevice.cpp | 4 | ||||
-rw-r--r-- | services/inputflinger/reader/controller/PeripheralController.cpp | 4 | ||||
-rw-r--r-- | services/inputflinger/reader/controller/PeripheralController.h | 2 | ||||
-rw-r--r-- | services/inputflinger/reader/controller/PeripheralControllerInterface.h | 2 | ||||
-rw-r--r-- | services/inputflinger/tests/InputReader_test.cpp | 18 |
10 files changed, 76 insertions, 7 deletions
diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp index aba81f60d8..a51bbb1553 100644 --- a/libs/gui/BLASTBufferQueue.cpp +++ b/libs/gui/BLASTBufferQueue.cpp @@ -307,7 +307,6 @@ void BLASTBufferQueue::transactionCommittedCallback(nsecs_t /*latchTime*/, BQA_LOGE("No matching SurfaceControls found: mSurfaceControlsWithPendingCallback was " "empty."); } - decStrong((void*)transactionCommittedCallbackThunk); } } @@ -350,6 +349,20 @@ void BLASTBufferQueue::transactionCallback(nsecs_t /*latchTime*/, const sp<Fence stat.latchTime, stat.frameEventStats.dequeueReadyTime); } + auto currFrameNumber = stat.frameEventStats.frameNumber; + std::vector<ReleaseCallbackId> staleReleases; + for (const auto& [key, value]: mSubmitted) { + if (currFrameNumber > key.framenumber) { + staleReleases.push_back(key); + } + } + for (const auto& staleRelease : staleReleases) { + BQA_LOGE("Faking releaseBufferCallback from transactionCompleteCallback"); + BBQ_TRACE("FakeReleaseCallback"); + releaseBufferCallbackLocked(staleRelease, + stat.previousReleaseFence ? stat.previousReleaseFence : Fence::NO_FENCE, + stat.currentMaxAcquiredBufferCount); + } } else { BQA_LOGE("Failed to find matching SurfaceControl in transactionCallback"); } @@ -390,7 +403,14 @@ void BLASTBufferQueue::releaseBufferCallback( const ReleaseCallbackId& id, const sp<Fence>& releaseFence, std::optional<uint32_t> currentMaxAcquiredBufferCount) { BBQ_TRACE(); + std::unique_lock _lock{mMutex}; + releaseBufferCallbackLocked(id, releaseFence, currentMaxAcquiredBufferCount); +} + +void BLASTBufferQueue::releaseBufferCallbackLocked(const ReleaseCallbackId& id, + const sp<Fence>& releaseFence, std::optional<uint32_t> currentMaxAcquiredBufferCount) { + ATRACE_CALL(); BQA_LOGV("releaseBufferCallback %s", id.to_string().c_str()); // Calculate how many buffers we need to hold before we release them back @@ -408,7 +428,11 @@ void BLASTBufferQueue::releaseBufferCallback( const auto numPendingBuffersToHold = isEGL ? std::max(0u, mMaxAcquiredBuffers - mCurrentMaxAcquiredBufferCount) : 0; - mPendingRelease.emplace_back(ReleasedBuffer{id, releaseFence}); + + auto rb = ReleasedBuffer{id, releaseFence}; + if (std::find(mPendingRelease.begin(), mPendingRelease.end(), rb) == mPendingRelease.end()) { + mPendingRelease.emplace_back(rb); + } // Release all buffers that are beyond the ones that we need to hold while (mPendingRelease.size() > numPendingBuffersToHold) { diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index 9358e29030..0f5192d41c 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -352,7 +352,8 @@ void TransactionCompletedListener::onTransactionCompleted(ListenerStats listener transactionStats.latchTime, surfaceStats.acquireTimeOrFence, transactionStats.presentFence, surfaceStats.previousReleaseFence, surfaceStats.transformHint, - surfaceStats.eventStats); + surfaceStats.eventStats, + surfaceStats.currentMaxAcquiredBufferCount); } callbackFunction(transactionStats.latchTime, transactionStats.presentFence, @@ -377,7 +378,8 @@ void TransactionCompletedListener::onTransactionCompleted(ListenerStats listener transactionStats.latchTime, surfaceStats.acquireTimeOrFence, transactionStats.presentFence, surfaceStats.previousReleaseFence, surfaceStats.transformHint, - surfaceStats.eventStats); + surfaceStats.eventStats, + surfaceStats.currentMaxAcquiredBufferCount); if (callbacksMap[callbackId].surfaceControls[surfaceStats.surfaceControl]) { callbacksMap[callbackId] .surfaceControls[surfaceStats.surfaceControl] diff --git a/libs/gui/include/gui/BLASTBufferQueue.h b/libs/gui/include/gui/BLASTBufferQueue.h index 9d287910a5..f5898d20f1 100644 --- a/libs/gui/include/gui/BLASTBufferQueue.h +++ b/libs/gui/include/gui/BLASTBufferQueue.h @@ -95,9 +95,12 @@ public: const std::vector<SurfaceControlStats>& stats); void releaseBufferCallback(const ReleaseCallbackId& id, const sp<Fence>& releaseFence, std::optional<uint32_t> currentMaxAcquiredBufferCount); + void releaseBufferCallbackLocked(const ReleaseCallbackId& id, const sp<Fence>& releaseFence, + std::optional<uint32_t> currentMaxAcquiredBufferCount); void syncNextTransaction(std::function<void(SurfaceComposerClient::Transaction*)> callback, bool acquireSingleBuffer = true); void stopContinuousSyncTransaction(); + void mergeWithNextTransaction(SurfaceComposerClient::Transaction* t, uint64_t frameNumber); void applyPendingTransactions(uint64_t frameNumber); SurfaceComposerClient::Transaction* gatherPendingTransactions(uint64_t frameNumber); @@ -177,6 +180,12 @@ private: struct ReleasedBuffer { ReleaseCallbackId callbackId; sp<Fence> releaseFence; + bool operator==(const ReleasedBuffer& rhs) const { + // Only compare Id so if we somehow got two callbacks + // with different fences we don't decrement mNumAcquired + // too far. + return rhs.callbackId == callbackId; + } }; std::deque<ReleasedBuffer> mPendingRelease GUARDED_BY(mMutex); diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h index b598b43403..9033e17c53 100644 --- a/libs/gui/include/gui/SurfaceComposerClient.h +++ b/libs/gui/include/gui/SurfaceComposerClient.h @@ -65,14 +65,16 @@ struct SurfaceControlStats { SurfaceControlStats(const sp<SurfaceControl>& sc, nsecs_t latchTime, std::variant<nsecs_t, sp<Fence>> acquireTimeOrFence, const sp<Fence>& presentFence, const sp<Fence>& prevReleaseFence, - uint32_t hint, FrameEventHistoryStats eventStats) + uint32_t hint, FrameEventHistoryStats eventStats, + uint32_t currentMaxAcquiredBufferCount) : surfaceControl(sc), latchTime(latchTime), acquireTimeOrFence(std::move(acquireTimeOrFence)), presentFence(presentFence), previousReleaseFence(prevReleaseFence), transformHint(hint), - frameEventStats(eventStats) {} + frameEventStats(eventStats), + currentMaxAcquiredBufferCount(currentMaxAcquiredBufferCount) {} sp<SurfaceControl> surfaceControl; nsecs_t latchTime = -1; @@ -81,6 +83,7 @@ struct SurfaceControlStats { sp<Fence> previousReleaseFence; uint32_t transformHint = 0; FrameEventHistoryStats frameEventStats; + uint32_t currentMaxAcquiredBufferCount = 0; }; using TransactionCompletedCallbackTakesContext = diff --git a/libs/gui/include/gui/test/CallbackUtils.h b/libs/gui/include/gui/test/CallbackUtils.h index 62d1496ccd..08785b49c1 100644 --- a/libs/gui/include/gui/test/CallbackUtils.h +++ b/libs/gui/include/gui/test/CallbackUtils.h @@ -135,7 +135,8 @@ private: void verifySurfaceControlStats(const SurfaceControlStats& surfaceControlStats, nsecs_t latchTime) const { const auto& [surfaceControl, latch, acquireTimeOrFence, presentFence, - previousReleaseFence, transformHint, frameEvents] = surfaceControlStats; + previousReleaseFence, transformHint, frameEvents, ignore] = + surfaceControlStats; ASSERT_TRUE(std::holds_alternative<nsecs_t>(acquireTimeOrFence)); ASSERT_EQ(std::get<nsecs_t>(acquireTimeOrFence) > 0, diff --git a/services/inputflinger/reader/InputDevice.cpp b/services/inputflinger/reader/InputDevice.cpp index a0119986a6..989700f6cf 100644 --- a/services/inputflinger/reader/InputDevice.cpp +++ b/services/inputflinger/reader/InputDevice.cpp @@ -232,6 +232,10 @@ void InputDevice::addEventHubDevice(int32_t eventHubId, bool populateMappers) { } void InputDevice::removeEventHubDevice(int32_t eventHubId) { + if (mController != nullptr && mController->getEventHubId() == eventHubId) { + // Delete mController, since the corresponding eventhub device is going away + mController = nullptr; + } mDevices.erase(eventHubId); } diff --git a/services/inputflinger/reader/controller/PeripheralController.cpp b/services/inputflinger/reader/controller/PeripheralController.cpp index a6934960c9..8065f57524 100644 --- a/services/inputflinger/reader/controller/PeripheralController.cpp +++ b/services/inputflinger/reader/controller/PeripheralController.cpp @@ -524,4 +524,8 @@ std::optional<int32_t> PeripheralController::getLightPlayerId(int32_t lightId) { return light->getLightPlayerId(); } +int32_t PeripheralController::getEventHubId() const { + return getDeviceContext().getEventHubId(); +} + } // namespace android diff --git a/services/inputflinger/reader/controller/PeripheralController.h b/services/inputflinger/reader/controller/PeripheralController.h index b1bc8c732c..ac951ebe2a 100644 --- a/services/inputflinger/reader/controller/PeripheralController.h +++ b/services/inputflinger/reader/controller/PeripheralController.h @@ -31,6 +31,7 @@ public: explicit PeripheralController(InputDeviceContext& deviceContext); ~PeripheralController() override; + int32_t getEventHubId() const override; void populateDeviceInfo(InputDeviceInfo* deviceInfo) override; void dump(std::string& dump) override; bool setLightColor(int32_t lightId, int32_t color) override; @@ -43,6 +44,7 @@ public: private: inline int32_t getDeviceId() { return mDeviceContext.getId(); } inline InputDeviceContext& getDeviceContext() { return mDeviceContext; } + inline InputDeviceContext& getDeviceContext() const { return mDeviceContext; } InputDeviceContext& mDeviceContext; void configureLights(); diff --git a/services/inputflinger/reader/controller/PeripheralControllerInterface.h b/services/inputflinger/reader/controller/PeripheralControllerInterface.h index 7688a431d1..306e36119b 100644 --- a/services/inputflinger/reader/controller/PeripheralControllerInterface.h +++ b/services/inputflinger/reader/controller/PeripheralControllerInterface.h @@ -33,6 +33,8 @@ public: PeripheralControllerInterface() {} virtual ~PeripheralControllerInterface() {} + virtual int32_t getEventHubId() const = 0; + // Interface methods for Battery virtual std::optional<int32_t> getBatteryCapacity(int32_t batteryId) = 0; virtual std::optional<int32_t> getBatteryStatus(int32_t batteryId) = 0; diff --git a/services/inputflinger/tests/InputReader_test.cpp b/services/inputflinger/tests/InputReader_test.cpp index e1befedcff..74d4f3b99f 100644 --- a/services/inputflinger/tests/InputReader_test.cpp +++ b/services/inputflinger/tests/InputReader_test.cpp @@ -2157,6 +2157,8 @@ public: ~FakePeripheralController() override {} + int32_t getEventHubId() const { return getDeviceContext().getEventHubId(); } + void populateDeviceInfo(InputDeviceInfo* deviceInfo) override {} void dump(std::string& dump) override {} @@ -2190,6 +2192,7 @@ private: InputDeviceContext& mDeviceContext; inline int32_t getDeviceId() { return mDeviceContext.getId(); } inline InputDeviceContext& getDeviceContext() { return mDeviceContext; } + inline InputDeviceContext& getDeviceContext() const { return mDeviceContext; } }; TEST_F(InputReaderTest, BatteryGetCapacity) { @@ -2931,6 +2934,21 @@ TEST_F(InputDeviceTest, Configure_UniqueId_CorrectlyMatches) { ASSERT_EQ(DISPLAY_UNIQUE_ID, mDevice->getAssociatedDisplayUniqueId()); } +/** + * This test reproduces a crash caused by a dangling reference that remains after device is added + * and removed. The reference is accessed in InputDevice::dump(..); + */ +TEST_F(InputDeviceTest, DumpDoesNotCrash) { + constexpr int32_t TEST_EVENTHUB_ID = 10; + mFakeEventHub->addDevice(TEST_EVENTHUB_ID, "Test EventHub device", InputDeviceClass::BATTERY); + + InputDevice device(mReader->getContext(), 1 /*id*/, 2 /*generation*/, {} /*identifier*/); + device.addEventHubDevice(TEST_EVENTHUB_ID, true /*populateMappers*/); + device.removeEventHubDevice(TEST_EVENTHUB_ID); + std::string dumpStr, eventHubDevStr; + device.dump(dumpStr, eventHubDevStr); +} + // --- InputMapperTest --- class InputMapperTest : public testing::Test { |