From 3e68cce46450bc156ed9ded9e3b0db0d58fdb87c Mon Sep 17 00:00:00 2001 From: Matt Buckley Date: Fri, 5 Aug 2022 06:51:43 +0000 Subject: Fix SF hint sessions for virtual multi-display case Currently timing for hint sessions in SurfaceFlinger do not work for multi-display case with virtual displays (eg: Android Auto). This patch fixes that by tracking true sf present completion time to supplement hwc time tracking in the sf main thread deadline case. Bug: b/241465879 Test: manual Change-Id: I169dd4f4afd2223ed0a648440d5f59dc19dc07f6 (cherry picked from commit 1809d90ed310216a4d0d8da38b63ecd17d8d6ed2) --- .../CompositionEngine/tests/MockPowerAdvisor.h | 3 ++- .../surfaceflinger/DisplayHardware/PowerAdvisor.cpp | 21 +++++++++++---------- .../surfaceflinger/DisplayHardware/PowerAdvisor.h | 10 +++++----- services/surfaceflinger/SurfaceFlinger.cpp | 3 ++- .../mock/DisplayHardware/MockPowerAdvisor.h | 3 ++- 5 files changed, 22 insertions(+), 18 deletions(-) diff --git a/services/surfaceflinger/CompositionEngine/tests/MockPowerAdvisor.h b/services/surfaceflinger/CompositionEngine/tests/MockPowerAdvisor.h index 8c164edded..c8bd5e436c 100644 --- a/services/surfaceflinger/CompositionEngine/tests/MockPowerAdvisor.h +++ b/services/surfaceflinger/CompositionEngine/tests/MockPowerAdvisor.h @@ -55,7 +55,8 @@ public: MOCK_METHOD(void, setRequiresClientComposition, (DisplayId displayId, bool requiresClientComposition), (override)); MOCK_METHOD(void, setExpectedPresentTime, (nsecs_t expectedPresentTime), (override)); - MOCK_METHOD(void, setPresentFenceTime, (nsecs_t presentFenceTime), (override)); + MOCK_METHOD(void, setSfPresentTiming, (nsecs_t presentFenceTime, nsecs_t presentEndTime), + (override)); MOCK_METHOD(void, setHwcPresentDelayedTime, (DisplayId displayId, std::chrono::steady_clock::time_point earliestFrameStartTime)); diff --git a/services/surfaceflinger/DisplayHardware/PowerAdvisor.cpp b/services/surfaceflinger/DisplayHardware/PowerAdvisor.cpp index b9d4753f45..ac46280331 100644 --- a/services/surfaceflinger/DisplayHardware/PowerAdvisor.cpp +++ b/services/surfaceflinger/DisplayHardware/PowerAdvisor.cpp @@ -315,7 +315,8 @@ void PowerAdvisor::setExpectedPresentTime(nsecs_t expectedPresentTime) { mExpectedPresentTimes.append(expectedPresentTime); } -void PowerAdvisor::setPresentFenceTime(nsecs_t presentFenceTime) { +void PowerAdvisor::setSfPresentTiming(nsecs_t presentFenceTime, nsecs_t presentEndTime) { + mLastSfPresentEndTime = presentEndTime; mLastPresentFenceTime = presentFenceTime; } @@ -334,13 +335,7 @@ void PowerAdvisor::setCommitStart(nsecs_t commitStartTime) { } void PowerAdvisor::setCompositeEnd(nsecs_t compositeEnd) { - mLastCompositeEndTime = compositeEnd; - // calculate the postcomp time here as well - std::vector&& displays = getOrderedDisplayIds(&DisplayTimingData::hwcPresentEndTime); - DisplayTimingData& timingData = mDisplayTimingData[displays.back()]; - mLastPostcompDuration = compositeEnd - - (timingData.skippedValidate ? *timingData.hwcValidateEndTime - : *timingData.hwcPresentEndTime); + mLastPostcompDuration = compositeEnd - mLastSfPresentEndTime; } void PowerAdvisor::setDisplays(std::vector& displayIds) { @@ -399,7 +394,7 @@ std::optional PowerAdvisor::estimateWorkDuration(bool earlyHint) { getOrderedDisplayIds(&DisplayTimingData::hwcPresentStartTime); DisplayTimeline referenceTiming, estimatedTiming; - // Iterate over the displays in the same order they are presented + // Iterate over the displays that use hwc in the same order they are presented for (DisplayId displayId : displayIds) { if (mDisplayTimingData.count(displayId) == 0) { continue; @@ -451,8 +446,11 @@ std::optional PowerAdvisor::estimateWorkDuration(bool earlyHint) { } ATRACE_INT64("Idle duration", idleDuration); + nsecs_t estimatedFlingerEndTime = earlyHint ? estimatedEndTime : mLastSfPresentEndTime; + // Don't count time spent idly waiting in the estimate as we could do more work in that time estimatedEndTime -= idleDuration; + estimatedFlingerEndTime -= idleDuration; // We finish the frame when both present and the gpu are done, so wait for the later of the two // Also add the frame delay duration since the target did not move while we were delayed @@ -460,7 +458,10 @@ std::optional PowerAdvisor::estimateWorkDuration(bool earlyHint) { std::max(estimatedEndTime, estimatedGpuEndTime.value_or(0)) - mCommitStartTimes[0]; // We finish SurfaceFlinger when post-composition finishes, so add that in here - nsecs_t flingerDuration = estimatedEndTime + mLastPostcompDuration - mCommitStartTimes[0]; + nsecs_t flingerDuration = + estimatedFlingerEndTime + mLastPostcompDuration - mCommitStartTimes[0]; + + // Combine the two timings into a single normalized one nsecs_t combinedDuration = combineTimingEstimates(totalDuration, flingerDuration); return std::make_optional(combinedDuration); diff --git a/services/surfaceflinger/DisplayHardware/PowerAdvisor.h b/services/surfaceflinger/DisplayHardware/PowerAdvisor.h index 98921b0861..71a1f8c03b 100644 --- a/services/surfaceflinger/DisplayHardware/PowerAdvisor.h +++ b/services/surfaceflinger/DisplayHardware/PowerAdvisor.h @@ -72,8 +72,8 @@ public: nsecs_t presentEndTime) = 0; // Reports the expected time that the current frame will present to the display virtual void setExpectedPresentTime(nsecs_t expectedPresentTime) = 0; - // Reports the most recent present fence time once it's known at the end of the frame - virtual void setPresentFenceTime(nsecs_t presentFenceTime) = 0; + // Reports the most recent present fence time and end time once known + virtual void setSfPresentTiming(nsecs_t presentFenceTime, nsecs_t presentEndTime) = 0; // Reports whether a display used client composition this frame virtual void setRequiresClientComposition(DisplayId displayId, bool requiresClientComposition) = 0; @@ -142,7 +142,7 @@ public: void setSkippedValidate(DisplayId displayId, bool skipped) override; void setRequiresClientComposition(DisplayId displayId, bool requiresClientComposition) override; void setExpectedPresentTime(nsecs_t expectedPresentTime) override; - void setPresentFenceTime(nsecs_t presentFenceTime) override; + void setSfPresentTiming(nsecs_t presentFenceTime, nsecs_t presentEndTime) override; void setHwcPresentDelayedTime( DisplayId displayId, std::chrono::steady_clock::time_point earliestFrameStartTime) override; @@ -245,8 +245,6 @@ private: // Current frame's delay nsecs_t mFrameDelayDuration = 0; - // Last frame's composite end time - nsecs_t mLastCompositeEndTime = -1; // Last frame's post-composition duration nsecs_t mLastPostcompDuration = 0; // Buffer of recent commit start times @@ -255,6 +253,8 @@ private: RingBuffer mExpectedPresentTimes; // Most recent present fence time, set at the end of the frame once known nsecs_t mLastPresentFenceTime = -1; + // Most recent present fence time, set at the end of the frame once known + nsecs_t mLastSfPresentEndTime = -1; // Target for the entire pipeline including gpu std::optional mTotalFrameTargetDuration; // Updated list of display IDs diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 8621d31d91..240991ccc6 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -2242,7 +2242,8 @@ void SurfaceFlinger::composite(nsecs_t frameTime, int64_t vsyncId) // Send a power hint hint after presentation is finished if (mPowerHintSessionEnabled) { - mPowerAdvisor->setPresentFenceTime(mPreviousPresentFences[0].fenceTime->getSignalTime()); + mPowerAdvisor->setSfPresentTiming(mPreviousPresentFences[0].fenceTime->getSignalTime(), + systemTime()); if (mPowerHintSessionMode.late) { mPowerAdvisor->sendActualWorkDuration(); } diff --git a/services/surfaceflinger/tests/unittests/mock/DisplayHardware/MockPowerAdvisor.h b/services/surfaceflinger/tests/unittests/mock/DisplayHardware/MockPowerAdvisor.h index d6dca45188..aede250db5 100644 --- a/services/surfaceflinger/tests/unittests/mock/DisplayHardware/MockPowerAdvisor.h +++ b/services/surfaceflinger/tests/unittests/mock/DisplayHardware/MockPowerAdvisor.h @@ -53,7 +53,8 @@ public: MOCK_METHOD(void, setRequiresClientComposition, (DisplayId displayId, bool requiresClientComposition), (override)); MOCK_METHOD(void, setExpectedPresentTime, (nsecs_t expectedPresentTime), (override)); - MOCK_METHOD(void, setPresentFenceTime, (nsecs_t presentFenceTime), (override)); + MOCK_METHOD(void, setSfPresentTiming, (nsecs_t presentFenceTime, nsecs_t presentEndTime), + (override)); MOCK_METHOD(void, setHwcPresentDelayedTime, (DisplayId displayId, std::chrono::steady_clock::time_point earliestFrameStartTime)); -- cgit v1.2.3-59-g8ed1b From 168609b590deea4774f2e9cc6295f1e4a219c971 Mon Sep 17 00:00:00 2001 From: Matt Buckley Date: Tue, 9 Aug 2022 22:19:51 +0000 Subject: Disable ADPF CPU hint session rate limiter After some testing, it has become clear that the rate limiter is unhelpful and possibly harmful for performance. Rate limiting also happens on PowerHAL side now, rendering this somewhat redundant, so this patch disables rate limiting and hint batching on SF side. Bug: b/241973353 Bug: b/195990840 Test: manual Change-Id: I7ae43d7c9bfc95c94963ae4852d3190ee987c9aa (cherry picked from commit 111de0342da565a667f5ccbc130a62e706ebbf89) --- services/surfaceflinger/DisplayHardware/PowerAdvisor.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/services/surfaceflinger/DisplayHardware/PowerAdvisor.cpp b/services/surfaceflinger/DisplayHardware/PowerAdvisor.cpp index b9d4753f45..ae90dc1a97 100644 --- a/services/surfaceflinger/DisplayHardware/PowerAdvisor.cpp +++ b/services/surfaceflinger/DisplayHardware/PowerAdvisor.cpp @@ -640,9 +640,8 @@ AidlPowerHalWrapper::AidlPowerHalWrapper(sp powerHal) : mPowerHal(std::m mSupportsPowerHint = checkPowerHintSessionSupported(); - mAllowedActualDeviation = - base::GetIntProperty("debug.sf.allowed_actual_deviation", - std::chrono::nanoseconds(250us).count()); + // Currently set to 0 to disable rate limiter by default + mAllowedActualDeviation = base::GetIntProperty("debug.sf.allowed_actual_deviation", 0); } AidlPowerHalWrapper::~AidlPowerHalWrapper() { -- cgit v1.2.3-59-g8ed1b From 5b59d94a5a6739416ecd00251616c437e6c84368 Mon Sep 17 00:00:00 2001 From: Matt Buckley Date: Tue, 2 Aug 2022 20:10:15 +0000 Subject: Disable ADPF CPU hints for SF unless active display is on Currently AOD is bugged and does not always match refresh rate between SF and HWC, which disrupts ADPF hint sessions in SF which are sensitive to refresh rate (b/218066882). This patch disables ADPF CPU hints for the AOD case until this issue is resolved. Bug: b/240619471 Test: atest libsurfaceflinger_unittest:libsurfaceflinger_unittest.SurfaceFlingerPowerHintTest Change-Id: I4fa617a108f14da9df278c4a449795363ec29cf3 Merged-In: I4fa617a108f14da9df278c4a449795363ec29cf3 (cherry picked from commit ba8d15a176ba57dfd692b8c98a7ad884b0d9260c) --- services/surfaceflinger/SurfaceFlinger.cpp | 6 +++++- .../unittests/SurfaceFlinger_PowerHintTest.cpp | 24 ++++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 8621d31d91..83ec4ec213 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -2085,7 +2085,11 @@ bool SurfaceFlinger::commit(nsecs_t frameTime, int64_t vsyncId, nsecs_t expected } // Save this once per commit + composite to ensure consistency - mPowerHintSessionEnabled = mPowerAdvisor->usePowerHintSession(); + // TODO (b/240619471): consider removing active display check once AOD is fixed + const auto activeDisplay = + FTL_FAKE_GUARD(mStateLock, getDisplayDeviceLocked(mActiveDisplayToken)); + mPowerHintSessionEnabled = mPowerAdvisor->usePowerHintSession() && activeDisplay && + activeDisplay->getPowerMode() == hal::PowerMode::ON; if (mPowerHintSessionEnabled) { const auto& display = FTL_FAKE_GUARD(mStateLock, getDefaultDisplayDeviceLocked()).get(); // get stable vsync period from display mode diff --git a/services/surfaceflinger/tests/unittests/SurfaceFlinger_PowerHintTest.cpp b/services/surfaceflinger/tests/unittests/SurfaceFlinger_PowerHintTest.cpp index c2d87f2484..2c9888dd8e 100644 --- a/services/surfaceflinger/tests/unittests/SurfaceFlinger_PowerHintTest.cpp +++ b/services/surfaceflinger/tests/unittests/SurfaceFlinger_PowerHintTest.cpp @@ -97,6 +97,7 @@ void SurfaceFlingerPowerHintTest::SetUp() { .setNativeWindow(mNativeWindow) .setPowerMode(hal::PowerMode::ON) .inject(); + mFlinger.mutableActiveDisplayToken() = mDisplay->getDisplayToken(); } void SurfaceFlingerPowerHintTest::setupScheduler() { @@ -148,5 +149,28 @@ TEST_F(SurfaceFlingerPowerHintTest, sendDurationsIncludingHwcWaitTime) { mFlinger.commitAndComposite(now, kVsyncId, now + mockVsyncPeriod.count()); } +TEST_F(SurfaceFlingerPowerHintTest, inactiveOnDisplayDoze) { + ON_CALL(*mPowerAdvisor, usePowerHintSession()).WillByDefault(Return(true)); + + mDisplay->setPowerMode(hal::PowerMode::DOZE); + + const std::chrono::nanoseconds mockVsyncPeriod = 15ms; + EXPECT_CALL(*mPowerAdvisor, setTargetWorkDuration(_)).Times(0); + + const nsecs_t now = systemTime(); + const std::chrono::nanoseconds mockHwcRunTime = 20ms; + EXPECT_CALL(*mDisplaySurface, + prepareFrame(compositionengine::DisplaySurface::CompositionType::Hwc)) + .Times(1); + EXPECT_CALL(*mComposer, presentOrValidateDisplay(HWC_DISPLAY, _, _, _, _, _)) + .WillOnce([mockHwcRunTime] { + std::this_thread::sleep_for(mockHwcRunTime); + return hardware::graphics::composer::V2_1::Error::NONE; + }); + EXPECT_CALL(*mPowerAdvisor, sendActualWorkDuration()).Times(0); + static constexpr bool kVsyncId = 123; // arbitrary + mFlinger.commitAndComposite(now, kVsyncId, now + mockVsyncPeriod.count()); +} + } // namespace } // namespace android -- cgit v1.2.3-59-g8ed1b From 88151b8fde4bb81f386644fc2671bd1d9563d5f2 Mon Sep 17 00:00:00 2001 From: Siarhei Vishniakou Date: Thu, 11 Aug 2022 00:53:38 +0000 Subject: Add test for heuristic palm rejection For short strokes, the palm rejection code has a heuristic - large touches are canceled. Since the behaviour is currently present, let's add an integration test for it. Separately, we will also remove 'getLinuxToolType' here because tool_type is not used in the palm rejection code. However, tool_code is used in that code. So let's add getLinuxToolCode instead. This piece does not affect the behaviour of the palm rejection model, but it makes our inputs more correct. Bug: 241935838 Test: atest inputflinger_tests Change-Id: Ied9e185bdfc6e892cf3a1069b98101ca8ae9c74b --- .../inputflinger/UnwantedInteractionBlocker.cpp | 26 ++++----- services/inputflinger/UnwantedInteractionBlocker.h | 2 +- services/inputflinger/reader/InputDevice.cpp | 9 +-- services/inputflinger/reader/InputReader.cpp | 9 +-- services/inputflinger/tests/Android.bp | 1 + services/inputflinger/tests/TestInputListener.cpp | 7 +++ services/inputflinger/tests/TestInputListener.h | 3 + .../tests/UnwantedInteractionBlocker_test.cpp | 65 ++++++++++++++++++---- 8 files changed, 88 insertions(+), 34 deletions(-) diff --git a/services/inputflinger/UnwantedInteractionBlocker.cpp b/services/inputflinger/UnwantedInteractionBlocker.cpp index 4e0f0c3bf5..1c27a52d2e 100644 --- a/services/inputflinger/UnwantedInteractionBlocker.cpp +++ b/services/inputflinger/UnwantedInteractionBlocker.cpp @@ -99,17 +99,15 @@ static bool isPalmRejectionEnabled() { return false; } -static int getLinuxToolType(int32_t toolType) { - switch (toolType) { - case AMOTION_EVENT_TOOL_TYPE_FINGER: - return MT_TOOL_FINGER; - case AMOTION_EVENT_TOOL_TYPE_STYLUS: - return MT_TOOL_PEN; - case AMOTION_EVENT_TOOL_TYPE_PALM: - return MT_TOOL_PALM; +static int getLinuxToolCode(int toolType) { + if (toolType == AMOTION_EVENT_TOOL_TYPE_STYLUS) { + return BTN_TOOL_PEN; } - ALOGW("Got tool type %" PRId32 ", converting to MT_TOOL_FINGER", toolType); - return MT_TOOL_FINGER; + if (toolType == AMOTION_EVENT_TOOL_TYPE_FINGER) { + return BTN_TOOL_FINGER; + } + ALOGW("Got tool type %" PRId32 ", converting to BTN_TOOL_FINGER", toolType); + return BTN_TOOL_FINGER; } static int32_t getActionUpForPointerId(const NotifyMotionArgs& args, int32_t pointerId) { @@ -562,7 +560,7 @@ std::vector<::ui::InProgressTouchEvdev> getTouches(const NotifyMotionArgs& args, touches.emplace_back(::ui::InProgressTouchEvdev()); touches.back().major = args.pointerCoords[i].getAxisValue(AMOTION_EVENT_AXIS_TOUCH_MAJOR); touches.back().minor = args.pointerCoords[i].getAxisValue(AMOTION_EVENT_AXIS_TOUCH_MINOR); - touches.back().tool_type = getLinuxToolType(args.pointerProperties[i].toolType); + // The field 'tool_type' is not used for palm rejection // Whether there is new information for the touch. touches.back().altered = true; @@ -609,10 +607,10 @@ std::vector<::ui::InProgressTouchEvdev> getTouches(const NotifyMotionArgs& args, // The fields 'radius_x' and 'radius_x' are not used for palm rejection touches.back().pressure = args.pointerCoords[i].getAxisValue(AMOTION_EVENT_AXIS_PRESSURE); - touches.back().tool_code = BTN_TOOL_FINGER; + touches.back().tool_code = getLinuxToolCode(args.pointerProperties[i].toolType); // The field 'orientation' is not used for palm rejection // The fields 'tilt_x' and 'tilt_y' are not used for palm rejection - touches.back().reported_tool_type = ::ui::EventPointerType::kTouch; + // The field 'reported_tool_type' is not used for palm rejection touches.back().stylus_button = false; } return touches; @@ -691,7 +689,7 @@ std::vector PalmRejector::processMotion(const NotifyMotionArgs return argsWithoutUnwantedPointers; } -const AndroidPalmFilterDeviceInfo& PalmRejector::getPalmFilterDeviceInfo() { +const AndroidPalmFilterDeviceInfo& PalmRejector::getPalmFilterDeviceInfo() const { return mDeviceInfo; } diff --git a/services/inputflinger/UnwantedInteractionBlocker.h b/services/inputflinger/UnwantedInteractionBlocker.h index 8ff965851f..1878849452 100644 --- a/services/inputflinger/UnwantedInteractionBlocker.h +++ b/services/inputflinger/UnwantedInteractionBlocker.h @@ -147,7 +147,7 @@ public: std::vector processMotion(const NotifyMotionArgs& args); // Get the device info of this device, for comparison purposes - const AndroidPalmFilterDeviceInfo& getPalmFilterDeviceInfo(); + const AndroidPalmFilterDeviceInfo& getPalmFilterDeviceInfo() const; std::string dump() const; private: diff --git a/services/inputflinger/reader/InputDevice.cpp b/services/inputflinger/reader/InputDevice.cpp index ba5083bec3..a0119986a6 100644 --- a/services/inputflinger/reader/InputDevice.cpp +++ b/services/inputflinger/reader/InputDevice.cpp @@ -110,7 +110,8 @@ void InputDevice::dump(std::string& dump, const std::string& eventHubDevStr) { dump += "\n"; } dump += StringPrintf(INDENT2 "HasMic: %s\n", toString(mHasMic)); - dump += StringPrintf(INDENT2 "Sources: 0x%08x\n", deviceInfo.getSources()); + dump += StringPrintf(INDENT2 "Sources: %s\n", + inputEventSourceToString(deviceInfo.getSources()).c_str()); dump += StringPrintf(INDENT2 "KeyboardType: %d\n", deviceInfo.getKeyboardType()); dump += StringPrintf(INDENT2 "ControllerNum: %d\n", deviceInfo.getControllerNumber()); @@ -128,10 +129,10 @@ void InputDevice::dump(std::string& dump, const std::string& eventHubDevStr) { snprintf(name, sizeof(name), "%d", range.axis); } dump += StringPrintf(INDENT3 - "%s: source=0x%08x, " + "%s: source=%s, " "min=%0.3f, max=%0.3f, flat=%0.3f, fuzz=%0.3f, resolution=%0.3f\n", - name, range.source, range.min, range.max, range.flat, range.fuzz, - range.resolution); + name, inputEventSourceToString(range.source).c_str(), range.min, + range.max, range.flat, range.fuzz, range.resolution); } } diff --git a/services/inputflinger/reader/InputReader.cpp b/services/inputflinger/reader/InputReader.cpp index 9c5a129213..9bcf463c36 100644 --- a/services/inputflinger/reader/InputReader.cpp +++ b/services/inputflinger/reader/InputReader.cpp @@ -196,9 +196,9 @@ void InputReader::addDeviceLocked(nsecs_t when, int32_t eventHubId) { "(ignored non-input device)", device->getId(), eventHubId, identifier.name.c_str(), identifier.descriptor.c_str()); } else { - ALOGI("Device added: id=%d, eventHubId=%d, name='%s', descriptor='%s',sources=0x%08x", + ALOGI("Device added: id=%d, eventHubId=%d, name='%s', descriptor='%s',sources=%s", device->getId(), eventHubId, identifier.name.c_str(), identifier.descriptor.c_str(), - device->getSources()); + inputEventSourceToString(device->getSources()).c_str()); } mDevices.emplace(eventHubId, device); @@ -250,9 +250,10 @@ void InputReader::removeDeviceLocked(nsecs_t when, int32_t eventHubId) { device->getId(), eventHubId, device->getName().c_str(), device->getDescriptor().c_str()); } else { - ALOGI("Device removed: id=%d, eventHubId=%d, name='%s', descriptor='%s', sources=0x%08x", + ALOGI("Device removed: id=%d, eventHubId=%d, name='%s', descriptor='%s', sources=%s", device->getId(), eventHubId, device->getName().c_str(), - device->getDescriptor().c_str(), device->getSources()); + device->getDescriptor().c_str(), + inputEventSourceToString(device->getSources()).c_str()); } device->removeEventHubDevice(eventHubId); diff --git a/services/inputflinger/tests/Android.bp b/services/inputflinger/tests/Android.bp index 76a7c19086..75cd9da782 100644 --- a/services/inputflinger/tests/Android.bp +++ b/services/inputflinger/tests/Android.bp @@ -60,6 +60,7 @@ cc_test { }, static_libs: [ "libc++fs", + "libgmock", ], require_root: true, test_suites: ["device-tests"], diff --git a/services/inputflinger/tests/TestInputListener.cpp b/services/inputflinger/tests/TestInputListener.cpp index 6a26c636d9..57b382c718 100644 --- a/services/inputflinger/tests/TestInputListener.cpp +++ b/services/inputflinger/tests/TestInputListener.cpp @@ -69,6 +69,13 @@ void TestInputListener::assertNotifyMotionWasCalled(NotifyMotionArgs* outEventAr "Expected notifyMotion() to have been called.")); } +void TestInputListener::assertNotifyMotionWasCalled( + const ::testing::Matcher& matcher) { + NotifyMotionArgs outEventArgs; + ASSERT_NO_FATAL_FAILURE(assertNotifyMotionWasCalled(&outEventArgs)); + ASSERT_THAT(outEventArgs, matcher); +} + void TestInputListener::assertNotifyMotionWasNotCalled() { ASSERT_NO_FATAL_FAILURE( assertNotCalled("notifyMotion() should not be called.")); diff --git a/services/inputflinger/tests/TestInputListener.h b/services/inputflinger/tests/TestInputListener.h index 626cdfc8c8..0bdfc6b9fb 100644 --- a/services/inputflinger/tests/TestInputListener.h +++ b/services/inputflinger/tests/TestInputListener.h @@ -18,6 +18,7 @@ #define _UI_TEST_INPUT_LISTENER_H #include +#include #include #include "InputListener.h" @@ -48,6 +49,8 @@ public: void assertNotifyMotionWasCalled(NotifyMotionArgs* outEventArgs = nullptr); + void assertNotifyMotionWasCalled(const ::testing::Matcher& matcher); + void assertNotifyMotionWasNotCalled(); void assertNotifySwitchWasCalled(NotifySwitchArgs* outEventArgs = nullptr); diff --git a/services/inputflinger/tests/UnwantedInteractionBlocker_test.cpp b/services/inputflinger/tests/UnwantedInteractionBlocker_test.cpp index 8af387174a..9313a4500d 100644 --- a/services/inputflinger/tests/UnwantedInteractionBlocker_test.cpp +++ b/services/inputflinger/tests/UnwantedInteractionBlocker_test.cpp @@ -16,13 +16,17 @@ #include "../UnwantedInteractionBlocker.h" #include +#include #include #include #include #include +#include "ui/events/ozone/evdev/touch_filter/neural_stylus_palm_detection_filter.h" #include "TestInputListener.h" +using ::testing::AllOf; + namespace android { constexpr int32_t DEVICE_ID = 3; @@ -30,6 +34,8 @@ constexpr int32_t X_RESOLUTION = 11; constexpr int32_t Y_RESOLUTION = 11; constexpr int32_t MAJOR_RESOLUTION = 1; +const nsecs_t RESAMPLE_PERIOD = ::ui::kResamplePeriod.InNanoseconds(); + constexpr int POINTER_0_DOWN = AMOTION_EVENT_ACTION_POINTER_DOWN | (0 << AMOTION_EVENT_ACTION_POINTER_INDEX_SHIFT); constexpr int POINTER_1_DOWN = @@ -47,6 +53,19 @@ constexpr int MOVE = AMOTION_EVENT_ACTION_MOVE; constexpr int UP = AMOTION_EVENT_ACTION_UP; constexpr int CANCEL = AMOTION_EVENT_ACTION_CANCEL; +constexpr int32_t FLAG_CANCELED = AMOTION_EVENT_FLAG_CANCELED; + +MATCHER_P(WithAction, action, "MotionEvent with specified action") { + bool result = true; + if (action == CANCEL) { + result &= (arg.flags & FLAG_CANCELED) != 0; + } + result &= arg.action == action; + *result_listener << "expected to receive " << MotionEvent::actionToString(action) + << " but received " << MotionEvent::actionToString(arg.action) << " instead."; + return result; +} + static nsecs_t toNs(std::chrono::nanoseconds duration) { return duration.count(); } @@ -256,7 +275,7 @@ TEST(CancelSuppressedPointersTest, NewlySuppressedPointerIsCanceled) { /*newSuppressedPointerIds*/ {1}); ASSERT_EQ(2u, result.size()); assertArgs(result[0], POINTER_1_UP, {{0, {1, 2, 3}}, {1, {4, 5, 6}}, {2, {7, 8, 9}}}); - ASSERT_EQ(AMOTION_EVENT_FLAG_CANCELED, result[0].flags); + ASSERT_EQ(FLAG_CANCELED, result[0].flags); assertArgs(result[1], MOVE, {{0, {1, 2, 3}}, {2, {7, 8, 9}}}); } @@ -271,7 +290,7 @@ TEST(CancelSuppressedPointersTest, SingleSuppressedPointerIsCanceled) { /*newSuppressedPointerIds*/ {0}); ASSERT_EQ(1u, result.size()); assertArgs(result[0], CANCEL, {{0, {1, 2, 3}}}); - ASSERT_EQ(AMOTION_EVENT_FLAG_CANCELED, result[0].flags); + ASSERT_EQ(FLAG_CANCELED, result[0].flags); } /** @@ -286,7 +305,7 @@ TEST(CancelSuppressedPointersTest, SuppressedPointer1GoingUpIsCanceled) { /*newSuppressedPointerIds*/ {1}); ASSERT_EQ(1u, result.size()); assertArgs(result[0], POINTER_1_UP, {{0, {1, 2, 3}}, {1, {4, 5, 6}}, {2, {7, 8, 9}}}); - ASSERT_EQ(AMOTION_EVENT_FLAG_CANCELED, result[0].flags); + ASSERT_EQ(FLAG_CANCELED, result[0].flags); } /** @@ -301,7 +320,7 @@ TEST(CancelSuppressedPointersTest, SuppressedPointer0GoingUpIsCanceled) { /*newSuppressedPointerIds*/ {0}); ASSERT_EQ(1u, result.size()); assertArgs(result[0], POINTER_0_UP, {{0, {1, 2, 3}}, {1, {4, 5, 6}}}); - ASSERT_EQ(AMOTION_EVENT_FLAG_CANCELED, result[0].flags); + ASSERT_EQ(FLAG_CANCELED, result[0].flags); } /** @@ -316,7 +335,7 @@ TEST(CancelSuppressedPointersTest, TwoNewlySuppressedPointersAreBothCanceled) { /*newSuppressedPointerIds*/ {0, 1}); ASSERT_EQ(1u, result.size()); assertArgs(result[0], CANCEL, {{0, {1, 2, 3}}, {1, {4, 5, 6}}}); - ASSERT_EQ(AMOTION_EVENT_FLAG_CANCELED, result[0].flags); + ASSERT_EQ(FLAG_CANCELED, result[0].flags); } /** @@ -332,7 +351,7 @@ TEST(CancelSuppressedPointersTest, TwoPointersAreCanceledDuringPointerUp) { /*newSuppressedPointerIds*/ {0, 1}); ASSERT_EQ(1u, result.size()); assertArgs(result[0], CANCEL, {{0, {1, 2, 3}}}); - ASSERT_EQ(AMOTION_EVENT_FLAG_CANCELED, result[0].flags); + ASSERT_EQ(FLAG_CANCELED, result[0].flags); } /** @@ -573,6 +592,30 @@ TEST_F(UnwantedInteractionBlockerTest, DumpCanBeAccessedOnAnotherThread) { dumpThread.join(); } +/** + * Heuristic filter that's present in the palm rejection model blocks touches early if the size + * of the touch is large. This is an integration test that checks that this filter kicks in. + */ +TEST_F(UnwantedInteractionBlockerTest, HeuristicFilterWorks) { + mBlocker->notifyInputDevicesChanged({generateTestDeviceInfo()}); + // Small touch down + NotifyMotionArgs args1 = generateMotionArgs(0 /*downTime*/, 0 /*eventTime*/, DOWN, {{1, 2, 3}}); + mBlocker->notifyMotion(&args1); + mTestListener.assertNotifyMotionWasCalled(WithAction(DOWN)); + + // Large touch oval on the next move + NotifyMotionArgs args2 = + generateMotionArgs(0 /*downTime*/, RESAMPLE_PERIOD, MOVE, {{4, 5, 200}}); + mBlocker->notifyMotion(&args2); + mTestListener.assertNotifyMotionWasCalled(WithAction(MOVE)); + + // Lift up the touch to force the model to decide on whether it's a palm + NotifyMotionArgs args3 = + generateMotionArgs(0 /*downTime*/, 2 * RESAMPLE_PERIOD, UP, {{4, 5, 200}}); + mBlocker->notifyMotion(&args3); + mTestListener.assertNotifyMotionWasCalled(WithAction(CANCEL)); +} + using UnwantedInteractionBlockerTestDeathTest = UnwantedInteractionBlockerTest; /** @@ -672,7 +715,7 @@ TEST_F(PalmRejectorTest, TwoPointersAreCanceled) { {{1433.0, 751.0, 44.0}, {1070.0, 771.0, 13.0}})); ASSERT_EQ(2u, argsList.size()); ASSERT_EQ(POINTER_0_UP, argsList[0].action); - ASSERT_EQ(AMOTION_EVENT_FLAG_CANCELED, argsList[0].flags); + ASSERT_EQ(FLAG_CANCELED, argsList[0].flags); ASSERT_EQ(MOVE, argsList[1].action); ASSERT_EQ(1u, argsList[1].pointerCount); ASSERT_EQ(0, argsList[1].flags); @@ -849,7 +892,7 @@ TEST_F(PalmRejectorFakeFilterTest, OneOfTwoPointersIsCanceled) { ASSERT_EQ(2u, argsList.size()); // First event - cancel pointer 1 ASSERT_EQ(POINTER_1_UP, argsList[0].action); - ASSERT_EQ(AMOTION_EVENT_FLAG_CANCELED, argsList[0].flags); + ASSERT_EQ(FLAG_CANCELED, argsList[0].flags); // Second event - send MOVE for the remaining pointer ASSERT_EQ(MOVE, argsList[1].action); ASSERT_EQ(0, argsList[1].flags); @@ -890,7 +933,7 @@ TEST_F(PalmRejectorFakeFilterTest, NewDownEventAfterCancel) { // Cancel all ASSERT_EQ(CANCEL, argsList[0].action); ASSERT_EQ(2u, argsList[0].pointerCount); - ASSERT_EQ(AMOTION_EVENT_FLAG_CANCELED, argsList[0].flags); + ASSERT_EQ(FLAG_CANCELED, argsList[0].flags); // Future move events are ignored argsList = mPalmRejector->processMotion( @@ -936,7 +979,7 @@ TEST_F(PalmRejectorFakeFilterTest, TwoPointersCanceledWhenOnePointerGoesUp) { {{1414.0, 702.0, 41.0}, {1059.0, 731.0, 12.0}})); ASSERT_EQ(1u, argsList.size()); ASSERT_EQ(CANCEL, argsList[0].action) << MotionEvent::actionToString(argsList[0].action); - ASSERT_EQ(AMOTION_EVENT_FLAG_CANCELED, argsList[0].flags); + ASSERT_EQ(FLAG_CANCELED, argsList[0].flags); // Future move events should not go to the listener. argsList = mPalmRejector->processMotion( @@ -970,7 +1013,7 @@ TEST_F(PalmRejectorFakeFilterTest, CancelTwoPointers) { {{1417.0, 685.0, 41.0}, {1060, 700, 10.0}})); ASSERT_EQ(2u, argsList.size()); ASSERT_EQ(POINTER_1_UP, argsList[0].action); - ASSERT_EQ(AMOTION_EVENT_FLAG_CANCELED, argsList[0].flags); + ASSERT_EQ(FLAG_CANCELED, argsList[0].flags); ASSERT_EQ(MOVE, argsList[1].action) << MotionEvent::actionToString(argsList[1].action); ASSERT_EQ(0, argsList[1].flags); -- cgit v1.2.3-59-g8ed1b From e491fb5ae1c602bc09271b8fa3456ee9af8a5a64 Mon Sep 17 00:00:00 2001 From: Siarhei Vishniakou Date: Thu, 11 Aug 2022 01:51:24 +0000 Subject: Call Filter from a separate function The function 'processMotion' has gotten too long. To make it easier to reason about, separate some code from the center of the function into a new function. Unfortunately, this new function will have side-effects, but it's still easier to understand and reduces the number of local vars. There's no functional change in this CL. Bug: 241935838 Test: atest inputflinger_tests Change-Id: I2157798b70299659791ae6ef85b2394e9130b4b3 --- .../inputflinger/UnwantedInteractionBlocker.cpp | 49 +++++++++++++--------- services/inputflinger/UnwantedInteractionBlocker.h | 8 ++++ 2 files changed, 37 insertions(+), 20 deletions(-) diff --git a/services/inputflinger/UnwantedInteractionBlocker.cpp b/services/inputflinger/UnwantedInteractionBlocker.cpp index 1c27a52d2e..fc8dfe678b 100644 --- a/services/inputflinger/UnwantedInteractionBlocker.cpp +++ b/services/inputflinger/UnwantedInteractionBlocker.cpp @@ -616,23 +616,7 @@ std::vector<::ui::InProgressTouchEvdev> getTouches(const NotifyMotionArgs& args, return touches; } -std::vector PalmRejector::processMotion(const NotifyMotionArgs& args) { - if (mPalmDetectionFilter == nullptr) { - return {args}; - } - const bool skipThisEvent = args.action == AMOTION_EVENT_ACTION_HOVER_ENTER || - args.action == AMOTION_EVENT_ACTION_HOVER_MOVE || - args.action == AMOTION_EVENT_ACTION_HOVER_EXIT || - args.action == AMOTION_EVENT_ACTION_BUTTON_PRESS || - args.action == AMOTION_EVENT_ACTION_BUTTON_RELEASE || - args.action == AMOTION_EVENT_ACTION_SCROLL; - if (skipThisEvent) { - // Lets not process hover events, button events, or scroll for now. - return {args}; - } - if (args.action == AMOTION_EVENT_ACTION_DOWN) { - mSuppressedPointerIds.clear(); - } +std::set PalmRejector::detectPalmPointers(const NotifyMotionArgs& args) { std::bitset<::ui::kNumTouchEvdevSlots> slotsToHold; std::bitset<::ui::kNumTouchEvdevSlots> slotsToSuppress; @@ -640,6 +624,7 @@ std::vector PalmRejector::processMotion(const NotifyMotionArgs // the slots that have been removed due to the incoming event. SlotState oldSlotState = mSlotState; mSlotState.update(args); + std::vector<::ui::InProgressTouchEvdev> touches = getTouches(args, mDeviceInfo, oldSlotState, mSlotState); ::base::TimeTicks chromeTimestamp = toChromeTimestamp(args.eventTime); @@ -651,14 +636,14 @@ std::vector PalmRejector::processMotion(const NotifyMotionArgs } ALOGD("Filter: touches = %s", touchesStream.str().c_str()); } + mPalmDetectionFilter->Filter(touches, chromeTimestamp, &slotsToHold, &slotsToSuppress); ALOGD_IF(DEBUG_MODEL, "Response: slotsToHold = %s, slotsToSuppress = %s", slotsToHold.to_string().c_str(), slotsToSuppress.to_string().c_str()); // Now that we know which slots should be suppressed, let's convert those to pointer id's. - std::set oldSuppressedIds; - std::swap(oldSuppressedIds, mSuppressedPointerIds); + std::set newSuppressedIds; for (size_t i = 0; i < args.pointerCount; i++) { const int32_t pointerId = args.pointerProperties[i].id; std::optional slot = oldSlotState.getSlotForPointerId(pointerId); @@ -667,9 +652,33 @@ std::vector PalmRejector::processMotion(const NotifyMotionArgs LOG_ALWAYS_FATAL_IF(!slot, "Could not find slot for pointer id %" PRId32, pointerId); } if (slotsToSuppress.test(*slot)) { - mSuppressedPointerIds.insert(pointerId); + newSuppressedIds.insert(pointerId); } } + return newSuppressedIds; +} + +std::vector PalmRejector::processMotion(const NotifyMotionArgs& args) { + if (mPalmDetectionFilter == nullptr) { + return {args}; + } + const bool skipThisEvent = args.action == AMOTION_EVENT_ACTION_HOVER_ENTER || + args.action == AMOTION_EVENT_ACTION_HOVER_MOVE || + args.action == AMOTION_EVENT_ACTION_HOVER_EXIT || + args.action == AMOTION_EVENT_ACTION_BUTTON_PRESS || + args.action == AMOTION_EVENT_ACTION_BUTTON_RELEASE || + args.action == AMOTION_EVENT_ACTION_SCROLL; + if (skipThisEvent) { + // Lets not process hover events, button events, or scroll for now. + return {args}; + } + if (args.action == AMOTION_EVENT_ACTION_DOWN) { + mSuppressedPointerIds.clear(); + } + + std::set oldSuppressedIds; + std::swap(oldSuppressedIds, mSuppressedPointerIds); + mSuppressedPointerIds = detectPalmPointers(args); std::vector argsWithoutUnwantedPointers = cancelSuppressedPointers(args, oldSuppressedIds, mSuppressedPointerIds); diff --git a/services/inputflinger/UnwantedInteractionBlocker.h b/services/inputflinger/UnwantedInteractionBlocker.h index 1878849452..a176a985c2 100644 --- a/services/inputflinger/UnwantedInteractionBlocker.h +++ b/services/inputflinger/UnwantedInteractionBlocker.h @@ -154,6 +154,14 @@ private: PalmRejector(const PalmRejector&) = delete; PalmRejector& operator=(const PalmRejector&) = delete; + /** + * Update the slot state and send this event to the palm rejection model for palm detection. + * Return the pointer ids that should be suppressed. + * + * This function is not const because it has side-effects. It will update the slot state using + * the incoming args! Also, it will call Filter(..), which has side-effects. + */ + std::set detectPalmPointers(const NotifyMotionArgs& args); std::unique_ptr<::ui::SharedPalmDetectionFilterState> mSharedPalmState; AndroidPalmFilterDeviceInfo mDeviceInfo; std::unique_ptr<::ui::PalmDetectionFilter> mPalmDetectionFilter; -- cgit v1.2.3-59-g8ed1b From 6573583a3b304dce8ff334df8d48d01b58ab2c84 Mon Sep 17 00:00:00 2001 From: Siarhei Vishniakou Date: Tue, 9 Aug 2022 19:18:37 +0000 Subject: Allow stylus events in PalmRejector After some recent changes, the touchscreen input device has source that is a combination of SOURCE_TOUCHSCREEN and SOURCE_STYLUS. Before this CL, this source is rejected, and therefore palm rejection feature is not enabled. With this CL, any source that has SOURCE_TOUCHSCREEN is allowed. That also means that we potentially would invoke the model for stylus events, especially if simultaneous touch and stylus are enabled. The model, however, was never trained on stylus, and is not designed to work with it. In preparation to upcoming simultaneous touch+stylus feature, in this CL we also remove the stylus pointers before sending the data to the model. The only case where we need to be careful is pointer-down and pointer-up events with stylus. In this CL, we drop these events, which should be a no-op from the palm rejection model's perpective. Bug: 241935838 Test: atest libpalmrejection_test inputflinger_tests Change-Id: I760207a82807e03802e72a318fca8b97a4fd7a24 --- .../inputflinger/UnwantedInteractionBlocker.cpp | 50 +++++++----- services/inputflinger/UnwantedInteractionBlocker.h | 19 +++++ .../tests/UnwantedInteractionBlocker_test.cpp | 88 ++++++++++++++++++++++ 3 files changed, 137 insertions(+), 20 deletions(-) diff --git a/services/inputflinger/UnwantedInteractionBlocker.cpp b/services/inputflinger/UnwantedInteractionBlocker.cpp index fc8dfe678b..ec41025e9d 100644 --- a/services/inputflinger/UnwantedInteractionBlocker.cpp +++ b/services/inputflinger/UnwantedInteractionBlocker.cpp @@ -77,8 +77,7 @@ static std::string toLower(std::string s) { } static bool isFromTouchscreen(int32_t source) { - return isFromSource(source, AINPUT_SOURCE_TOUCHSCREEN) && - !isFromSource(source, AINPUT_SOURCE_STYLUS); + return isFromSource(source, AINPUT_SOURCE_TOUCHSCREEN); } static ::base::TimeTicks toChromeTimestamp(nsecs_t eventTime) { @@ -142,23 +141,6 @@ static int32_t resolveActionForPointer(uint8_t pointerIndex, int32_t action) { return AMOTION_EVENT_ACTION_MOVE; } -/** - * Remove the data for the provided pointers from the args. The pointers are identified by their - * pointerId, not by the index inside the array. - * Return the new NotifyMotionArgs struct that has the remaining pointers. - * The only fields that may be different in the returned args from the provided args are: - * - action - * - pointerCount - * - pointerProperties - * - pointerCoords - * Action might change because it contains a pointer index. If another pointer is removed, the - * active pointer index would be shifted. - * Do not call this function for events with POINTER_UP or POINTER_DOWN events when removed pointer - * id is the acting pointer id. - * - * @param args the args from which the pointers should be removed - * @param pointerIds the pointer ids of the pointers that should be removed - */ NotifyMotionArgs removePointerIds(const NotifyMotionArgs& args, const std::set& pointerIds) { const uint8_t actionIndex = MotionEvent::getActionIndex(args.action); @@ -204,6 +186,26 @@ NotifyMotionArgs removePointerIds(const NotifyMotionArgs& args, return newArgs; } +/** + * Remove stylus pointers from the provided NotifyMotionArgs. + * + * Return NotifyMotionArgs where the stylus pointers have been removed. + * If this results in removal of the active pointer, then return nullopt. + */ +static std::optional removeStylusPointerIds(const NotifyMotionArgs& args) { + std::set stylusPointerIds; + for (uint32_t i = 0; i < args.pointerCount; i++) { + if (args.pointerProperties[i].toolType == AMOTION_EVENT_TOOL_TYPE_STYLUS) { + stylusPointerIds.insert(args.pointerProperties[i].id); + } + } + NotifyMotionArgs withoutStylusPointers = removePointerIds(args, stylusPointerIds); + if (withoutStylusPointers.pointerCount == 0 || withoutStylusPointers.action == ACTION_UNKNOWN) { + return std::nullopt; + } + return withoutStylusPointers; +} + std::optional createPalmFilterDeviceInfo( const InputDeviceInfo& deviceInfo) { if (!isFromTouchscreen(deviceInfo.getSources())) { @@ -678,7 +680,15 @@ std::vector PalmRejector::processMotion(const NotifyMotionArgs std::set oldSuppressedIds; std::swap(oldSuppressedIds, mSuppressedPointerIds); - mSuppressedPointerIds = detectPalmPointers(args); + + std::optional touchOnlyArgs = removeStylusPointerIds(args); + if (touchOnlyArgs) { + mSuppressedPointerIds = detectPalmPointers(*touchOnlyArgs); + } else { + // This is a stylus-only event. + // We can skip this event and just keep the suppressed pointer ids the same as before. + mSuppressedPointerIds = oldSuppressedIds; + } std::vector argsWithoutUnwantedPointers = cancelSuppressedPointers(args, oldSuppressedIds, mSuppressedPointerIds); diff --git a/services/inputflinger/UnwantedInteractionBlocker.h b/services/inputflinger/UnwantedInteractionBlocker.h index a176a985c2..5d0dde8e64 100644 --- a/services/inputflinger/UnwantedInteractionBlocker.h +++ b/services/inputflinger/UnwantedInteractionBlocker.h @@ -43,6 +43,25 @@ std::optional createPalmFilterDeviceInfo( static constexpr int32_t ACTION_UNKNOWN = -1; +/** + * Remove the data for the provided pointers from the args. The pointers are identified by their + * pointerId, not by the index inside the array. + * Return the new NotifyMotionArgs struct that has the remaining pointers. + * The only fields that may be different in the returned args from the provided args are: + * - action + * - pointerCount + * - pointerProperties + * - pointerCoords + * Action might change because it contains a pointer index. If another pointer is removed, the + * active pointer index would be shifted. + * + * If the active pointer id is removed (for example, for events like + * POINTER_UP or POINTER_DOWN), then the action is set to ACTION_UNKNOWN. It is up to the caller + * to set the action appropriately after the call. + * + * @param args the args from which the pointers should be removed + * @param pointerIds the pointer ids of the pointers that should be removed + */ NotifyMotionArgs removePointerIds(const NotifyMotionArgs& args, const std::set& pointerIds); diff --git a/services/inputflinger/tests/UnwantedInteractionBlocker_test.cpp b/services/inputflinger/tests/UnwantedInteractionBlocker_test.cpp index 9313a4500d..29fa001185 100644 --- a/services/inputflinger/tests/UnwantedInteractionBlocker_test.cpp +++ b/services/inputflinger/tests/UnwantedInteractionBlocker_test.cpp @@ -66,6 +66,10 @@ MATCHER_P(WithAction, action, "MotionEvent with specified action") { return result; } +MATCHER_P(WithFlags, flags, "MotionEvent with specified flags") { + return arg.flags == flags; +} + static nsecs_t toNs(std::chrono::nanoseconds duration) { return duration.count(); } @@ -616,6 +620,90 @@ TEST_F(UnwantedInteractionBlockerTest, HeuristicFilterWorks) { mTestListener.assertNotifyMotionWasCalled(WithAction(CANCEL)); } +/** + * Send a stylus event that would have triggered the heuristic palm detector if it were a touch + * event. However, since it's a stylus event, it should propagate without being canceled through + * the blocker. + * This is similar to `HeuristicFilterWorks` test, but for stylus tool. + */ +TEST_F(UnwantedInteractionBlockerTest, StylusIsNotBlocked) { + InputDeviceInfo info = generateTestDeviceInfo(); + info.addSource(AINPUT_SOURCE_STYLUS); + mBlocker->notifyInputDevicesChanged({info}); + NotifyMotionArgs args1 = generateMotionArgs(0 /*downTime*/, 0 /*eventTime*/, DOWN, {{1, 2, 3}}); + args1.pointerProperties[0].toolType = AMOTION_EVENT_TOOL_TYPE_STYLUS; + mBlocker->notifyMotion(&args1); + mTestListener.assertNotifyMotionWasCalled(WithAction(DOWN)); + + // Move the stylus, setting large TOUCH_MAJOR/TOUCH_MINOR dimensions + NotifyMotionArgs args2 = + generateMotionArgs(0 /*downTime*/, RESAMPLE_PERIOD, MOVE, {{4, 5, 200}}); + args2.pointerProperties[0].toolType = AMOTION_EVENT_TOOL_TYPE_STYLUS; + mBlocker->notifyMotion(&args2); + mTestListener.assertNotifyMotionWasCalled(WithAction(MOVE)); + + // Lift up the stylus. If it were a touch event, this would force the model to decide on whether + // it's a palm. + NotifyMotionArgs args3 = + generateMotionArgs(0 /*downTime*/, 2 * RESAMPLE_PERIOD, UP, {{4, 5, 200}}); + args3.pointerProperties[0].toolType = AMOTION_EVENT_TOOL_TYPE_STYLUS; + mBlocker->notifyMotion(&args3); + mTestListener.assertNotifyMotionWasCalled(WithAction(UP)); +} + +/** + * Send a mixed touch and stylus event. + * The touch event goes first, and is a palm. The stylus event goes down after. + * Stylus event should continue to work even after touch is detected as a palm. + */ +TEST_F(UnwantedInteractionBlockerTest, TouchIsBlockedWhenMixedWithStylus) { + InputDeviceInfo info = generateTestDeviceInfo(); + info.addSource(AINPUT_SOURCE_STYLUS); + mBlocker->notifyInputDevicesChanged({info}); + + // Touch down + NotifyMotionArgs args1 = generateMotionArgs(0 /*downTime*/, 0 /*eventTime*/, DOWN, {{1, 2, 3}}); + mBlocker->notifyMotion(&args1); + mTestListener.assertNotifyMotionWasCalled(WithAction(DOWN)); + + // Stylus pointer down + NotifyMotionArgs args2 = generateMotionArgs(0 /*downTime*/, RESAMPLE_PERIOD, POINTER_1_DOWN, + {{1, 2, 3}, {10, 20, 30}}); + args2.pointerProperties[1].toolType = AMOTION_EVENT_TOOL_TYPE_STYLUS; + mBlocker->notifyMotion(&args2); + mTestListener.assertNotifyMotionWasCalled(WithAction(POINTER_1_DOWN)); + + // Large touch oval on the next finger move + NotifyMotionArgs args3 = generateMotionArgs(0 /*downTime*/, 2 * RESAMPLE_PERIOD, MOVE, + {{1, 2, 300}, {11, 21, 30}}); + args3.pointerProperties[1].toolType = AMOTION_EVENT_TOOL_TYPE_STYLUS; + mBlocker->notifyMotion(&args3); + mTestListener.assertNotifyMotionWasCalled(WithAction(MOVE)); + + // Lift up the finger pointer. It should be canceled due to the heuristic filter. + NotifyMotionArgs args4 = generateMotionArgs(0 /*downTime*/, 3 * RESAMPLE_PERIOD, POINTER_0_UP, + {{1, 2, 300}, {11, 21, 30}}); + args4.pointerProperties[1].toolType = AMOTION_EVENT_TOOL_TYPE_STYLUS; + mBlocker->notifyMotion(&args4); + mTestListener.assertNotifyMotionWasCalled( + AllOf(WithAction(POINTER_0_UP), WithFlags(FLAG_CANCELED))); + + NotifyMotionArgs args5 = + generateMotionArgs(0 /*downTime*/, 4 * RESAMPLE_PERIOD, MOVE, {{12, 22, 30}}); + args5.pointerProperties[0].id = args4.pointerProperties[1].id; + args5.pointerProperties[0].toolType = AMOTION_EVENT_TOOL_TYPE_STYLUS; + mBlocker->notifyMotion(&args5); + mTestListener.assertNotifyMotionWasCalled(WithAction(MOVE)); + + // Lift up the stylus pointer + NotifyMotionArgs args6 = + generateMotionArgs(0 /*downTime*/, 5 * RESAMPLE_PERIOD, UP, {{4, 5, 200}}); + args6.pointerProperties[0].id = args4.pointerProperties[1].id; + args6.pointerProperties[0].toolType = AMOTION_EVENT_TOOL_TYPE_STYLUS; + mBlocker->notifyMotion(&args6); + mTestListener.assertNotifyMotionWasCalled(WithAction(UP)); +} + using UnwantedInteractionBlockerTestDeathTest = UnwantedInteractionBlockerTest; /** -- cgit v1.2.3-59-g8ed1b From 0d4ac562fbf70ea385de91a3e56bd4183f00052e Mon Sep 17 00:00:00 2001 From: Brian Duddie Date: Mon, 23 May 2022 17:47:50 -0700 Subject: Reset target SDK version cache on new connection Avoids possibility that a stale cache is used if cleanupConnection() does not get called for some reason. Bug: 230358834 Test: run POC from b/230358834#comment4 Change-Id: I2e7422d90cec87167968d56458af9aac6d525506 --- services/sensorservice/SensorService.cpp | 19 +++++++++++-------- services/sensorservice/SensorService.h | 1 + 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/services/sensorservice/SensorService.cpp b/services/sensorservice/SensorService.cpp index 948692bd47..e0a4f034cb 100644 --- a/services/sensorservice/SensorService.cpp +++ b/services/sensorservice/SensorService.cpp @@ -1328,6 +1328,7 @@ Vector SensorService::getSensorList(const String16& opPackageName) { mSensors.getUserDebugSensors() : mSensors.getUserSensors(); Vector accessibleSensorList; + resetTargetSdkVersionCache(opPackageName); bool isCapped = isRateCappedBasedOnPermission(opPackageName); for (size_t i = 0; i < initialSensorList.size(); i++) { Sensor sensor = initialSensorList[i]; @@ -1367,6 +1368,7 @@ sp SensorService::createSensorEventConnection(const Stri if (requestedMode != NORMAL && requestedMode != DATA_INJECTION) { return nullptr; } + resetTargetSdkVersionCache(opPackageName); Mutex::Autolock _l(mLock); // To create a client in DATA_INJECTION mode to inject data, SensorService should already be @@ -1402,6 +1404,7 @@ int SensorService::isDataInjectionEnabled() { sp SensorService::createSensorDirectConnection( const String16& opPackageName, uint32_t size, int32_t type, int32_t format, const native_handle *resource) { + resetTargetSdkVersionCache(opPackageName); ConnectionSafeAutolock connLock = mConnectionHolder.lock(mLock); // No new direct connections are allowed when sensor privacy is enabled @@ -1643,14 +1646,6 @@ void SensorService::cleanupConnection(SensorEventConnection* c) { checkWakeLockStateLocked(&connLock); } - { - Mutex::Autolock packageLock(sPackageTargetVersionLock); - auto iter = sPackageTargetVersion.find(c->mOpPackageName); - if (iter != sPackageTargetVersion.end()) { - sPackageTargetVersion.erase(iter); - } - } - SensorDevice& dev(SensorDevice::getInstance()); dev.notifyConnectionDestroyed(c); } @@ -2091,6 +2086,14 @@ int SensorService::getTargetSdkVersion(const String16& opPackageName) { return targetSdkVersion; } +void SensorService::resetTargetSdkVersionCache(const String16& opPackageName) { + Mutex::Autolock packageLock(sPackageTargetVersionLock); + auto iter = sPackageTargetVersion.find(opPackageName); + if (iter != sPackageTargetVersion.end()) { + sPackageTargetVersion.erase(iter); + } +} + void SensorService::checkWakeLockState() { ConnectionSafeAutolock connLock = mConnectionHolder.lock(mLock); checkWakeLockStateLocked(&connLock); diff --git a/services/sensorservice/SensorService.h b/services/sensorservice/SensorService.h index 234dc9cd7d..4ba3c51985 100644 --- a/services/sensorservice/SensorService.h +++ b/services/sensorservice/SensorService.h @@ -377,6 +377,7 @@ private: const String16& opPackageName); static bool hasPermissionForSensor(const Sensor& sensor); static int getTargetSdkVersion(const String16& opPackageName); + static void resetTargetSdkVersionCache(const String16& opPackageName); // SensorService acquires a partial wakelock for delivering events from wake up sensors. This // method checks whether all the events from these wake up sensors have been delivered to the // corresponding applications, if yes the wakelock is released. -- cgit v1.2.3-59-g8ed1b From 5727405482a5f4b2177f08d5c7d2c76b1632a232 Mon Sep 17 00:00:00 2001 From: Matt Buckley Date: Fri, 12 Aug 2022 21:54:23 +0000 Subject: Add additional tests for PowerAdvisor Adds additional tests for PowerAdvisor functionality regarding timing and multidisplay Bug: b/241465879 Test: atest libsurfaceflinger_unittest:libsurfaceflinger_unittest.PowerAdvisorTest Change-Id: I833e91efb5608d5972220c679a061c9bb51372fa --- .../DisplayHardware/PowerAdvisor.cpp | 37 ++-- .../surfaceflinger/DisplayHardware/PowerAdvisor.h | 7 + services/surfaceflinger/tests/unittests/Android.bp | 2 + .../tests/unittests/PowerAdvisorTest.cpp | 203 +++++++++++++++++++++ .../DisplayHardware/MockAidlPowerHalWrapper.cpp | 26 +++ .../mock/DisplayHardware/MockAidlPowerHalWrapper.h | 51 ++++++ 6 files changed, 306 insertions(+), 20 deletions(-) create mode 100644 services/surfaceflinger/tests/unittests/PowerAdvisorTest.cpp create mode 100644 services/surfaceflinger/tests/unittests/mock/DisplayHardware/MockAidlPowerHalWrapper.cpp create mode 100644 services/surfaceflinger/tests/unittests/mock/DisplayHardware/MockAidlPowerHalWrapper.h diff --git a/services/surfaceflinger/DisplayHardware/PowerAdvisor.cpp b/services/surfaceflinger/DisplayHardware/PowerAdvisor.cpp index 4018c6b1af..a0350b717a 100644 --- a/services/surfaceflinger/DisplayHardware/PowerAdvisor.cpp +++ b/services/surfaceflinger/DisplayHardware/PowerAdvisor.cpp @@ -838,10 +838,7 @@ const bool AidlPowerHalWrapper::sTraceHintSessionData = base::GetBoolProperty(std::string("debug.sf.trace_hint_sessions"), false); PowerAdvisor::HalWrapper* PowerAdvisor::getPowerHal() { - static std::unique_ptr sHalWrapper = nullptr; - static bool sHasHal = true; - - if (!sHasHal) { + if (!mHasHal) { return nullptr; } @@ -849,57 +846,57 @@ PowerAdvisor::HalWrapper* PowerAdvisor::getPowerHal() { std::vector oldPowerHintSessionThreadIds; std::optional oldTargetWorkDuration; - if (sHalWrapper != nullptr) { - oldPowerHintSessionThreadIds = sHalWrapper->getPowerHintSessionThreadIds(); - oldTargetWorkDuration = sHalWrapper->getTargetWorkDuration(); + if (mHalWrapper != nullptr) { + oldPowerHintSessionThreadIds = mHalWrapper->getPowerHintSessionThreadIds(); + oldTargetWorkDuration = mHalWrapper->getTargetWorkDuration(); } // If we used to have a HAL, but it stopped responding, attempt to reconnect if (mReconnectPowerHal) { - sHalWrapper = nullptr; + mHalWrapper = nullptr; mReconnectPowerHal = false; } - if (sHalWrapper != nullptr) { - auto wrapper = sHalWrapper.get(); + if (mHalWrapper != nullptr) { + auto wrapper = mHalWrapper.get(); // If the wrapper is fine, return it, but if it indicates a reconnect, remake it if (!wrapper->shouldReconnectHAL()) { return wrapper; } ALOGD("Reconnecting Power HAL"); - sHalWrapper = nullptr; + mHalWrapper = nullptr; } // At this point, we know for sure there is no running session mPowerHintSessionRunning = false; // First attempt to connect to the AIDL Power HAL - sHalWrapper = AidlPowerHalWrapper::connect(); + mHalWrapper = AidlPowerHalWrapper::connect(); // If that didn't succeed, attempt to connect to the HIDL Power HAL - if (sHalWrapper == nullptr) { - sHalWrapper = HidlPowerHalWrapper::connect(); + if (mHalWrapper == nullptr) { + mHalWrapper = HidlPowerHalWrapper::connect(); } else { ALOGD("Successfully connecting AIDL Power HAL"); // If AIDL, pass on any existing hint session values - sHalWrapper->setPowerHintSessionThreadIds(oldPowerHintSessionThreadIds); + mHalWrapper->setPowerHintSessionThreadIds(oldPowerHintSessionThreadIds); // Only set duration and start if duration is defined if (oldTargetWorkDuration.has_value()) { - sHalWrapper->setTargetWorkDuration(*oldTargetWorkDuration); + mHalWrapper->setTargetWorkDuration(*oldTargetWorkDuration); // Only start if possible to run and both threadids and duration are defined if (usePowerHintSession() && !oldPowerHintSessionThreadIds.empty()) { - mPowerHintSessionRunning = sHalWrapper->startPowerHintSession(); + mPowerHintSessionRunning = mHalWrapper->startPowerHintSession(); } } } // If we make it to this point and still don't have a HAL, it's unlikely we // will, so stop trying - if (sHalWrapper == nullptr) { - sHasHal = false; + if (mHalWrapper == nullptr) { + mHasHal = false; } - return sHalWrapper.get(); + return mHalWrapper.get(); } } // namespace impl diff --git a/services/surfaceflinger/DisplayHardware/PowerAdvisor.h b/services/surfaceflinger/DisplayHardware/PowerAdvisor.h index 71a1f8c03b..6e25f787d7 100644 --- a/services/surfaceflinger/DisplayHardware/PowerAdvisor.h +++ b/services/surfaceflinger/DisplayHardware/PowerAdvisor.h @@ -154,6 +154,13 @@ public: void setTotalFrameTargetWorkDuration(nsecs_t targetDuration) override; private: + friend class PowerAdvisorTest; + + // Tracks if powerhal exists + bool mHasHal = true; + // Holds the hal wrapper for getPowerHal + std::unique_ptr mHalWrapper GUARDED_BY(mPowerHalMutex) = nullptr; + HalWrapper* getPowerHal() REQUIRES(mPowerHalMutex); bool mReconnectPowerHal GUARDED_BY(mPowerHalMutex) = false; std::mutex mPowerHalMutex; diff --git a/services/surfaceflinger/tests/unittests/Android.bp b/services/surfaceflinger/tests/unittests/Android.bp index 7823363e1e..004f31c562 100644 --- a/services/surfaceflinger/tests/unittests/Android.bp +++ b/services/surfaceflinger/tests/unittests/Android.bp @@ -24,6 +24,7 @@ package { filegroup { name: "libsurfaceflinger_mock_sources", srcs: [ + "mock/DisplayHardware/MockAidlPowerHalWrapper.cpp", "mock/DisplayHardware/MockComposer.cpp", "mock/DisplayHardware/MockHWC2.cpp", "mock/DisplayHardware/MockIPower.cpp", @@ -95,6 +96,7 @@ cc_test { "LayerTest.cpp", "LayerTestUtils.cpp", "MessageQueueTest.cpp", + "PowerAdvisorTest.cpp", "SurfaceFlinger_CreateDisplayTest.cpp", "SurfaceFlinger_DestroyDisplayTest.cpp", "SurfaceFlinger_DisplayModeSwitching.cpp", diff --git a/services/surfaceflinger/tests/unittests/PowerAdvisorTest.cpp b/services/surfaceflinger/tests/unittests/PowerAdvisorTest.cpp new file mode 100644 index 0000000000..8711a42b2b --- /dev/null +++ b/services/surfaceflinger/tests/unittests/PowerAdvisorTest.cpp @@ -0,0 +1,203 @@ +/* + * Copyright 2022 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#undef LOG_TAG +#define LOG_TAG "PowerAdvisorTest" + +#include +#include +#include +#include +#include +#include +#include +#include "TestableSurfaceFlinger.h" +#include "mock/DisplayHardware/MockAidlPowerHalWrapper.h" + +using namespace android; +using namespace android::Hwc2::mock; +using namespace android::hardware::power; +using namespace std::chrono_literals; +using namespace testing; + +namespace android::Hwc2::impl { + +class PowerAdvisorTest : public testing::Test { +public: + void SetUp() override; + void startPowerHintSession(); + void fakeBasicFrameTiming(nsecs_t startTime, nsecs_t vsyncPeriod); + void setExpectedTiming(nsecs_t startTime, nsecs_t vsyncPeriod); + nsecs_t getFenceWaitDelayDuration(bool skipValidate); + +protected: + TestableSurfaceFlinger mFlinger; + std::unique_ptr mPowerAdvisor; + NiceMock* mMockAidlWrapper; + nsecs_t kErrorMargin = std::chrono::nanoseconds(1ms).count(); +}; + +void PowerAdvisorTest::SetUp() FTL_FAKE_GUARD(mPowerAdvisor->mPowerHalMutex) { + std::unique_ptr mockAidlWrapper = + std::make_unique>(); + mPowerAdvisor = std::make_unique(*mFlinger.flinger()); + ON_CALL(*mockAidlWrapper.get(), supportsPowerHintSession()).WillByDefault(Return(true)); + ON_CALL(*mockAidlWrapper.get(), startPowerHintSession()).WillByDefault(Return(true)); + mPowerAdvisor->mHalWrapper = std::move(mockAidlWrapper); + mMockAidlWrapper = + reinterpret_cast*>(mPowerAdvisor->mHalWrapper.get()); +} + +void PowerAdvisorTest::startPowerHintSession() { + const std::vector threadIds = {1, 2, 3}; + mPowerAdvisor->enablePowerHint(true); + mPowerAdvisor->startPowerHintSession(threadIds); +} + +void PowerAdvisorTest::setExpectedTiming(nsecs_t totalFrameTarget, nsecs_t expectedPresentTime) { + mPowerAdvisor->setTotalFrameTargetWorkDuration(totalFrameTarget); + mPowerAdvisor->setExpectedPresentTime(expectedPresentTime); +} + +void PowerAdvisorTest::fakeBasicFrameTiming(nsecs_t startTime, nsecs_t vsyncPeriod) { + mPowerAdvisor->setCommitStart(startTime); + mPowerAdvisor->setFrameDelay(0); + mPowerAdvisor->setTargetWorkDuration(vsyncPeriod); +} + +nsecs_t PowerAdvisorTest::getFenceWaitDelayDuration(bool skipValidate) { + return (skipValidate ? PowerAdvisor::kFenceWaitStartDelaySkippedValidate + : PowerAdvisor::kFenceWaitStartDelayValidated) + .count(); +} + +namespace { + +TEST_F(PowerAdvisorTest, hintSessionUseHwcDisplay) { + mPowerAdvisor->onBootFinished(); + startPowerHintSession(); + + std::vector displayIds{PhysicalDisplayId::fromPort(42u)}; + + // 60hz + const nsecs_t vsyncPeriod = std::chrono::nanoseconds(1s).count() / 60; + const nsecs_t presentDuration = std::chrono::nanoseconds(5ms).count(); + const nsecs_t postCompDuration = std::chrono::nanoseconds(1ms).count(); + + nsecs_t startTime = 100; + + // advisor only starts on frame 2 so do an initial no-op frame + fakeBasicFrameTiming(startTime, vsyncPeriod); + setExpectedTiming(vsyncPeriod, startTime + vsyncPeriod); + mPowerAdvisor->setDisplays(displayIds); + mPowerAdvisor->setSfPresentTiming(startTime, startTime + presentDuration); + mPowerAdvisor->setCompositeEnd(startTime + presentDuration + postCompDuration); + + // increment the frame + startTime += vsyncPeriod; + + const nsecs_t expectedDuration = kErrorMargin + presentDuration + postCompDuration; + EXPECT_CALL(*mMockAidlWrapper, sendActualWorkDuration(Eq(expectedDuration), _)).Times(1); + + fakeBasicFrameTiming(startTime, vsyncPeriod); + setExpectedTiming(vsyncPeriod, startTime + vsyncPeriod); + mPowerAdvisor->setDisplays(displayIds); + mPowerAdvisor->setHwcValidateTiming(displayIds[0], startTime + 1000000, startTime + 1500000); + mPowerAdvisor->setHwcPresentTiming(displayIds[0], startTime + 2000000, startTime + 2500000); + mPowerAdvisor->setSfPresentTiming(startTime, startTime + presentDuration); + mPowerAdvisor->sendActualWorkDuration(); +} + +TEST_F(PowerAdvisorTest, hintSessionSubtractsHwcFenceTime) { + mPowerAdvisor->onBootFinished(); + startPowerHintSession(); + + std::vector displayIds{PhysicalDisplayId::fromPort(42u)}; + + // 60hz + const nsecs_t vsyncPeriod = std::chrono::nanoseconds(1s).count() / 60; + const nsecs_t presentDuration = std::chrono::nanoseconds(5ms).count(); + const nsecs_t postCompDuration = std::chrono::nanoseconds(1ms).count(); + const nsecs_t hwcBlockedDuration = std::chrono::nanoseconds(500us).count(); + + nsecs_t startTime = 100; + + // advisor only starts on frame 2 so do an initial no-op frame + fakeBasicFrameTiming(startTime, vsyncPeriod); + setExpectedTiming(vsyncPeriod, startTime + vsyncPeriod); + mPowerAdvisor->setDisplays(displayIds); + mPowerAdvisor->setSfPresentTiming(startTime, startTime + presentDuration); + mPowerAdvisor->setCompositeEnd(startTime + presentDuration + postCompDuration); + + // increment the frame + startTime += vsyncPeriod; + + const nsecs_t expectedDuration = kErrorMargin + presentDuration + + getFenceWaitDelayDuration(false) - hwcBlockedDuration + postCompDuration; + EXPECT_CALL(*mMockAidlWrapper, sendActualWorkDuration(Eq(expectedDuration), _)).Times(1); + + fakeBasicFrameTiming(startTime, vsyncPeriod); + setExpectedTiming(vsyncPeriod, startTime + vsyncPeriod); + mPowerAdvisor->setDisplays(displayIds); + mPowerAdvisor->setHwcValidateTiming(displayIds[0], startTime + 1000000, startTime + 1500000); + mPowerAdvisor->setHwcPresentTiming(displayIds[0], startTime + 2000000, startTime + 3000000); + // now report the fence as having fired during the display HWC time + mPowerAdvisor->setSfPresentTiming(startTime + 2000000 + hwcBlockedDuration, + startTime + presentDuration); + mPowerAdvisor->sendActualWorkDuration(); +} + +TEST_F(PowerAdvisorTest, hintSessionUsingSecondaryVirtualDisplays) { + mPowerAdvisor->onBootFinished(); + startPowerHintSession(); + + std::vector displayIds{PhysicalDisplayId::fromPort(42u), GpuVirtualDisplayId(0), + GpuVirtualDisplayId(1)}; + + // 60hz + const nsecs_t vsyncPeriod = std::chrono::nanoseconds(1s).count() / 60; + // make present duration much later than the hwc display by itself will account for + const nsecs_t presentDuration = std::chrono::nanoseconds(10ms).count(); + const nsecs_t postCompDuration = std::chrono::nanoseconds(1ms).count(); + + nsecs_t startTime = 100; + + // advisor only starts on frame 2 so do an initial no-op frame + fakeBasicFrameTiming(startTime, vsyncPeriod); + setExpectedTiming(vsyncPeriod, startTime + vsyncPeriod); + mPowerAdvisor->setDisplays(displayIds); + mPowerAdvisor->setSfPresentTiming(startTime, startTime + presentDuration); + mPowerAdvisor->setCompositeEnd(startTime + presentDuration + postCompDuration); + + // increment the frame + startTime += vsyncPeriod; + + const nsecs_t expectedDuration = kErrorMargin + presentDuration + postCompDuration; + EXPECT_CALL(*mMockAidlWrapper, sendActualWorkDuration(Eq(expectedDuration), _)).Times(1); + + fakeBasicFrameTiming(startTime, vsyncPeriod); + setExpectedTiming(vsyncPeriod, startTime + vsyncPeriod); + mPowerAdvisor->setDisplays(displayIds); + + // don't report timing for the gpu displays since they don't use hwc + mPowerAdvisor->setHwcValidateTiming(displayIds[0], startTime + 1000000, startTime + 1500000); + mPowerAdvisor->setHwcPresentTiming(displayIds[0], startTime + 2000000, startTime + 2500000); + mPowerAdvisor->setSfPresentTiming(startTime, startTime + presentDuration); + mPowerAdvisor->sendActualWorkDuration(); +} + +} // namespace +} // namespace android::Hwc2::impl diff --git a/services/surfaceflinger/tests/unittests/mock/DisplayHardware/MockAidlPowerHalWrapper.cpp b/services/surfaceflinger/tests/unittests/mock/DisplayHardware/MockAidlPowerHalWrapper.cpp new file mode 100644 index 0000000000..5049b1d367 --- /dev/null +++ b/services/surfaceflinger/tests/unittests/mock/DisplayHardware/MockAidlPowerHalWrapper.cpp @@ -0,0 +1,26 @@ +/* + * Copyright (C) 2022 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "MockAidlPowerHalWrapper.h" +#include "MockIPower.h" + +namespace android::Hwc2::mock { + +MockAidlPowerHalWrapper::MockAidlPowerHalWrapper() + : AidlPowerHalWrapper(sp>::make()){}; +MockAidlPowerHalWrapper::~MockAidlPowerHalWrapper() = default; + +} // namespace android::Hwc2::mock diff --git a/services/surfaceflinger/tests/unittests/mock/DisplayHardware/MockAidlPowerHalWrapper.h b/services/surfaceflinger/tests/unittests/mock/DisplayHardware/MockAidlPowerHalWrapper.h new file mode 100644 index 0000000000..657ced3d08 --- /dev/null +++ b/services/surfaceflinger/tests/unittests/mock/DisplayHardware/MockAidlPowerHalWrapper.h @@ -0,0 +1,51 @@ +/* + * Copyright (C) 2022 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once + +#include + +#include "DisplayHardware/PowerAdvisor.h" + +namespace android { +namespace hardware { +namespace power { +class IPower; +} +} // namespace hardware +} // namespace android + +namespace android::Hwc2::mock { + +class MockAidlPowerHalWrapper : public Hwc2::impl::AidlPowerHalWrapper { +public: + MockAidlPowerHalWrapper(); + ~MockAidlPowerHalWrapper() override; + MOCK_METHOD(bool, setExpensiveRendering, (bool enabled), (override)); + MOCK_METHOD(bool, notifyDisplayUpdateImminent, (), (override)); + MOCK_METHOD(bool, supportsPowerHintSession, (), (override)); + MOCK_METHOD(bool, isPowerHintSessionRunning, (), (override)); + MOCK_METHOD(void, restartPowerHintSession, (), (override)); + MOCK_METHOD(void, setPowerHintSessionThreadIds, (const std::vector& threadIds), + (override)); + MOCK_METHOD(bool, startPowerHintSession, (), (override)); + MOCK_METHOD(void, setTargetWorkDuration, (nsecs_t targetDuration), (override)); + MOCK_METHOD(void, sendActualWorkDuration, (nsecs_t actualDuration, nsecs_t timestamp), + (override)); + MOCK_METHOD(bool, shouldReconnectHAL, (), (override)); +}; + +} // namespace android::Hwc2::mock \ No newline at end of file -- cgit v1.2.3-59-g8ed1b From ef1a90de7aa6e77ce90b8015011628014ede9f2c Mon Sep 17 00:00:00 2001 From: Siarhei Vishniakou Date: Wed, 18 May 2022 12:30:16 -0700 Subject: Refactor input code for require_kernel_config parameter As part of the next commit to allow kl files to specify a required kernel config, some small refactorings were done. Move those here to a separate CL to make it easier to review. Bug: 228005926 Test: atest libinput_tests Merged-In: Iab06bb6ef44807308ee2b3e6b8a0c780bb748f09 Change-Id: Iab06bb6ef44807308ee2b3e6b8a0c780bb748f09 (cherry picked from commit 5ed8eaab67ad7ec3ac1110d696f5d89596a5a887) --- include/input/KeyLayoutMap.h | 4 +- libs/input/KeyLayoutMap.cpp | 115 +++++++++++++++------------------- libs/input/tests/InputDevice_test.cpp | 4 +- 3 files changed, 52 insertions(+), 71 deletions(-) diff --git a/include/input/KeyLayoutMap.h b/include/input/KeyLayoutMap.h index b2bd535cbf..d1925f4eee 100644 --- a/include/input/KeyLayoutMap.h +++ b/include/input/KeyLayoutMap.h @@ -21,7 +21,6 @@ #include #include #include -#include #include #include @@ -66,7 +65,6 @@ struct AxisInfo { class KeyLayoutMap { public: static base::Result> load(const std::string& filename); - static base::Result> load(Tokenizer* tokenizer); static base::Result> loadContents(const std::string& filename, const char* contents); @@ -84,6 +82,8 @@ public: virtual ~KeyLayoutMap(); private: + static base::Result> load(Tokenizer* tokenizer); + struct Key { int32_t keyCode; uint32_t flags; diff --git a/libs/input/KeyLayoutMap.cpp b/libs/input/KeyLayoutMap.cpp index 7c25cda9ac..17c3bb36e3 100644 --- a/libs/input/KeyLayoutMap.cpp +++ b/libs/input/KeyLayoutMap.cpp @@ -21,8 +21,8 @@ #include #include #include +#include #include -#include #include #include @@ -30,15 +30,22 @@ #include #include -// Enables debug output for the parser. -#define DEBUG_PARSER 0 +/** + * Log debug output for the parser. + * Enable this via "adb shell setprop log.tag.KeyLayoutMapParser DEBUG" (requires restart) + */ +const bool DEBUG_PARSER = + __android_log_is_loggable(ANDROID_LOG_DEBUG, LOG_TAG "Parser", ANDROID_LOG_INFO); // Enables debug output for parser performance. #define DEBUG_PARSER_PERFORMANCE 0 -// Enables debug output for mapping. -#define DEBUG_MAPPING 0 - +/** + * Log debug output for mapping. + * Enable this via "adb shell setprop log.tag.KeyLayoutMapMapping DEBUG" (requires restart) + */ +const bool DEBUG_MAPPING = + __android_log_is_loggable(ANDROID_LOG_DEBUG, LOG_TAG "Mapping", ANDROID_LOG_INFO); namespace android { namespace { @@ -134,9 +141,8 @@ status_t KeyLayoutMap::mapKey(int32_t scanCode, int32_t usageCode, int32_t* outKeyCode, uint32_t* outFlags) const { const Key* key = getKey(scanCode, usageCode); if (!key) { -#if DEBUG_MAPPING - ALOGD("mapKey: scanCode=%d, usageCode=0x%08x ~ Failed.", scanCode, usageCode); -#endif + ALOGD_IF(DEBUG_MAPPING, "mapKey: scanCode=%d, usageCode=0x%08x ~ Failed.", scanCode, + usageCode); *outKeyCode = AKEYCODE_UNKNOWN; *outFlags = 0; return NAME_NOT_FOUND; @@ -145,10 +151,9 @@ status_t KeyLayoutMap::mapKey(int32_t scanCode, int32_t usageCode, *outKeyCode = key->keyCode; *outFlags = key->flags; -#if DEBUG_MAPPING - ALOGD("mapKey: scanCode=%d, usageCode=0x%08x ~ Result keyCode=%d, outFlags=0x%08x.", - scanCode, usageCode, *outKeyCode, *outFlags); -#endif + ALOGD_IF(DEBUG_MAPPING, + "mapKey: scanCode=%d, usageCode=0x%08x ~ Result keyCode=%d, outFlags=0x%08x.", + scanCode, usageCode, *outKeyCode, *outFlags); return NO_ERROR; } @@ -156,17 +161,12 @@ status_t KeyLayoutMap::mapKey(int32_t scanCode, int32_t usageCode, base::Result> KeyLayoutMap::mapSensor(int32_t absCode) { auto it = mSensorsByAbsCode.find(absCode); if (it == mSensorsByAbsCode.end()) { -#if DEBUG_MAPPING - ALOGD("mapSensor: absCode=%d, ~ Failed.", absCode); -#endif + ALOGD_IF(DEBUG_MAPPING, "mapSensor: absCode=%d, ~ Failed.", absCode); return Errorf("Can't find abs code {}.", absCode); } const Sensor& sensor = it->second; - -#if DEBUG_MAPPING - ALOGD("mapSensor: absCode=%d, sensorType=%s, sensorDataIndex=0x%x.", absCode, - ftl::enum_string(sensor.sensorType).c_str(), sensor.sensorDataIndex); -#endif + ALOGD_IF(DEBUG_MAPPING, "mapSensor: absCode=%d, sensorType=%s, sensorDataIndex=0x%x.", absCode, + ftl::enum_string(sensor.sensorType).c_str(), sensor.sensorDataIndex); return std::make_pair(sensor.sensorType, sensor.sensorDataIndex); } @@ -200,21 +200,18 @@ status_t KeyLayoutMap::findScanCodesForKey( status_t KeyLayoutMap::mapAxis(int32_t scanCode, AxisInfo* outAxisInfo) const { ssize_t index = mAxes.indexOfKey(scanCode); if (index < 0) { -#if DEBUG_MAPPING - ALOGD("mapAxis: scanCode=%d ~ Failed.", scanCode); -#endif + ALOGD_IF(DEBUG_MAPPING, "mapAxis: scanCode=%d ~ Failed.", scanCode); return NAME_NOT_FOUND; } *outAxisInfo = mAxes.valueAt(index); -#if DEBUG_MAPPING - ALOGD("mapAxis: scanCode=%d ~ Result mode=%d, axis=%d, highAxis=%d, " - "splitValue=%d, flatOverride=%d.", - scanCode, - outAxisInfo->mode, outAxisInfo->axis, outAxisInfo->highAxis, - outAxisInfo->splitValue, outAxisInfo->flatOverride); -#endif + ALOGD_IF(DEBUG_MAPPING, + "mapAxis: scanCode=%d ~ Result mode=%d, axis=%d, highAxis=%d, " + "splitValue=%d, flatOverride=%d.", + scanCode, outAxisInfo->mode, outAxisInfo->axis, outAxisInfo->highAxis, + outAxisInfo->splitValue, outAxisInfo->flatOverride); + return NO_ERROR; } @@ -223,15 +220,12 @@ status_t KeyLayoutMap::findScanCodeForLed(int32_t ledCode, int32_t* outScanCode) for (size_t i = 0; i < N; i++) { if (mLedsByScanCode.valueAt(i).ledCode == ledCode) { *outScanCode = mLedsByScanCode.keyAt(i); -#if DEBUG_MAPPING - ALOGD("findScanCodeForLed: ledCode=%d, scanCode=%d.", ledCode, *outScanCode); -#endif + ALOGD_IF(DEBUG_MAPPING, "findScanCodeForLed: ledCode=%d, scanCode=%d.", ledCode, + *outScanCode); return NO_ERROR; } } -#if DEBUG_MAPPING - ALOGD("findScanCodeForLed: ledCode=%d ~ Not found.", ledCode); -#endif + ALOGD_IF(DEBUG_MAPPING, "findScanCodeForLed: ledCode=%d ~ Not found.", ledCode); return NAME_NOT_FOUND; } @@ -240,15 +234,12 @@ status_t KeyLayoutMap::findUsageCodeForLed(int32_t ledCode, int32_t* outUsageCod for (size_t i = 0; i < N; i++) { if (mLedsByUsageCode.valueAt(i).ledCode == ledCode) { *outUsageCode = mLedsByUsageCode.keyAt(i); -#if DEBUG_MAPPING - ALOGD("findUsageForLed: ledCode=%d, usage=%x.", ledCode, *outUsageCode); -#endif + ALOGD_IF(DEBUG_MAPPING, "%s: ledCode=%d, usage=%x.", __func__, ledCode, *outUsageCode); return NO_ERROR; } } -#if DEBUG_MAPPING - ALOGD("findUsageForLed: ledCode=%d ~ Not found.", ledCode); -#endif + ALOGD_IF(DEBUG_MAPPING, "%s: ledCode=%d ~ Not found.", __func__, ledCode); + return NAME_NOT_FOUND; } @@ -264,10 +255,8 @@ KeyLayoutMap::Parser::~Parser() { status_t KeyLayoutMap::Parser::parse() { while (!mTokenizer->isEof()) { -#if DEBUG_PARSER - ALOGD("Parsing %s: '%s'.", mTokenizer->getLocation().string(), - mTokenizer->peekRemainderOfLine().string()); -#endif + ALOGD_IF(DEBUG_PARSER, "Parsing %s: '%s'.", mTokenizer->getLocation().string(), + mTokenizer->peekRemainderOfLine().string()); mTokenizer->skipDelimiters(WHITESPACE); @@ -361,10 +350,9 @@ status_t KeyLayoutMap::Parser::parseKey() { flags |= flag; } -#if DEBUG_PARSER - ALOGD("Parsed key %s: code=%d, keyCode=%d, flags=0x%08x.", - mapUsage ? "usage" : "scan code", code, keyCode, flags); -#endif + ALOGD_IF(DEBUG_PARSER, "Parsed key %s: code=%d, keyCode=%d, flags=0x%08x.", + mapUsage ? "usage" : "scan code", code, keyCode, flags); + Key key; key.keyCode = keyCode; key.flags = flags; @@ -462,13 +450,12 @@ status_t KeyLayoutMap::Parser::parseAxis() { } } -#if DEBUG_PARSER - ALOGD("Parsed axis: scanCode=%d, mode=%d, axis=%d, highAxis=%d, " - "splitValue=%d, flatOverride=%d.", - scanCode, - axisInfo.mode, axisInfo.axis, axisInfo.highAxis, - axisInfo.splitValue, axisInfo.flatOverride); -#endif + ALOGD_IF(DEBUG_PARSER, + "Parsed axis: scanCode=%d, mode=%d, axis=%d, highAxis=%d, " + "splitValue=%d, flatOverride=%d.", + scanCode, axisInfo.mode, axisInfo.axis, axisInfo.highAxis, axisInfo.splitValue, + axisInfo.flatOverride); + mMap->mAxes.add(scanCode, axisInfo); return NO_ERROR; } @@ -505,10 +492,8 @@ status_t KeyLayoutMap::Parser::parseLed() { return BAD_VALUE; } -#if DEBUG_PARSER - ALOGD("Parsed led %s: code=%d, ledCode=%d.", - mapUsage ? "usage" : "scan code", code, ledCode); -#endif + ALOGD_IF(DEBUG_PARSER, "Parsed led %s: code=%d, ledCode=%d.", mapUsage ? "usage" : "scan code", + code, ledCode); Led led; led.ledCode = ledCode; @@ -584,10 +569,8 @@ status_t KeyLayoutMap::Parser::parseSensor() { } int32_t sensorDataIndex = indexOpt.value(); -#if DEBUG_PARSER - ALOGD("Parsed sensor: abs code=%d, sensorType=%s, sensorDataIndex=%d.", code, - ftl::enum_string(sensorType).c_str(), sensorDataIndex); -#endif + ALOGD_IF(DEBUG_PARSER, "Parsed sensor: abs code=%d, sensorType=%s, sensorDataIndex=%d.", code, + ftl::enum_string(sensorType).c_str(), sensorDataIndex); Sensor sensor; sensor.sensorType = sensorType; diff --git a/libs/input/tests/InputDevice_test.cpp b/libs/input/tests/InputDevice_test.cpp index 61e88df11d..6b695c4581 100644 --- a/libs/input/tests/InputDevice_test.cpp +++ b/libs/input/tests/InputDevice_test.cpp @@ -64,13 +64,11 @@ protected: mKeyMap.keyCharacterMapFile = path; } - virtual void SetUp() override { + void SetUp() override { loadKeyLayout("Generic"); loadKeyCharacterMap("Generic"); } - virtual void TearDown() override {} - KeyMap mKeyMap; }; -- cgit v1.2.3-59-g8ed1b From 0ac62eb4b78df85291e73ab367ebba0e27cb7d76 Mon Sep 17 00:00:00 2001 From: Siarhei Vishniakou Date: Wed, 18 May 2022 09:42:52 -0700 Subject: Do not load keylayout if required kernel module is missing Some key layouts require the presence of a specific kernel module. If the kernel module is not present, the layout should not be loaded. In this CL, we add an option to specify which kernel modules / configs are needed inside the kl file. Bug: 228005926 Test: atest libinput_tests Merged-In: I0d2ab6298bd41df6dc56120bf0385e10da6c3bfe Change-Id: I0d2ab6298bd41df6dc56120bf0385e10da6c3bfe --- include/input/InputDevice.h | 6 +- include/input/KeyLayoutMap.h | 6 +- include/input/Keyboard.h | 4 +- libs/input/Android.bp | 1 + libs/input/InputDevice.cpp | 30 ++++---- libs/input/KeyLayoutMap.cpp | 82 +++++++++++++++++----- libs/input/Keyboard.cpp | 39 ++++++---- libs/input/tests/Android.bp | 3 +- libs/input/tests/InputDevice_test.cpp | 16 +++++ .../tests/data/kl_with_required_fake_config.kl | 20 ++++++ .../tests/data/kl_with_required_real_config.kl | 21 ++++++ 11 files changed, 180 insertions(+), 48 deletions(-) create mode 100644 libs/input/tests/data/kl_with_required_fake_config.kl create mode 100644 libs/input/tests/data/kl_with_required_real_config.kl diff --git a/include/input/InputDevice.h b/include/input/InputDevice.h index c4f03c9119..3585392c2b 100644 --- a/include/input/InputDevice.h +++ b/include/input/InputDevice.h @@ -300,6 +300,8 @@ enum class InputDeviceConfigurationFileType : int32_t { /* * Gets the path of an input device configuration file, if one is available. * Considers both system provided and user installed configuration files. + * The optional suffix is appended to the end of the file name (before the + * extension). * * The device identifier is used to construct several default configuration file * names to try based on the device name, vendor, product, and version. @@ -307,8 +309,8 @@ enum class InputDeviceConfigurationFileType : int32_t { * Returns an empty string if not found. */ extern std::string getInputDeviceConfigurationFilePathByDeviceIdentifier( - const InputDeviceIdentifier& deviceIdentifier, - InputDeviceConfigurationFileType type); + const InputDeviceIdentifier& deviceIdentifier, InputDeviceConfigurationFileType type, + const char* suffix = ""); /* * Gets the path of an input device configuration file, if one is available. diff --git a/include/input/KeyLayoutMap.h b/include/input/KeyLayoutMap.h index d1925f4eee..50849506a4 100644 --- a/include/input/KeyLayoutMap.h +++ b/include/input/KeyLayoutMap.h @@ -22,6 +22,7 @@ #include #include #include +#include #include @@ -64,7 +65,8 @@ struct AxisInfo { */ class KeyLayoutMap { public: - static base::Result> load(const std::string& filename); + static base::Result> load(const std::string& filename, + const char* contents = nullptr); static base::Result> loadContents(const std::string& filename, const char* contents); @@ -104,6 +106,7 @@ private: KeyedVector mLedsByScanCode; KeyedVector mLedsByUsageCode; std::unordered_map mSensorsByAbsCode; + std::set mRequiredKernelConfigs; std::string mLoadFileName; KeyLayoutMap(); @@ -124,6 +127,7 @@ private: status_t parseAxis(); status_t parseLed(); status_t parseSensor(); + status_t parseRequiredKernelConfig(); }; }; diff --git a/include/input/Keyboard.h b/include/input/Keyboard.h index 08ad8c6e5a..9a3e15f1cd 100644 --- a/include/input/Keyboard.h +++ b/include/input/Keyboard.h @@ -61,9 +61,7 @@ private: bool probeKeyMap(const InputDeviceIdentifier& deviceIdentifier, const std::string& name); status_t loadKeyLayout(const InputDeviceIdentifier& deviceIdentifier, const std::string& name); status_t loadKeyCharacterMap(const InputDeviceIdentifier& deviceIdentifier, - const std::string& name); - std::string getPath(const InputDeviceIdentifier& deviceIdentifier, - const std::string& name, InputDeviceConfigurationFileType type); + const std::string& name); }; /** diff --git a/libs/input/Android.bp b/libs/input/Android.bp index 5d7874af77..1335e4dfd0 100644 --- a/libs/input/Android.bp +++ b/libs/input/Android.bp @@ -67,6 +67,7 @@ cc_library { "libbase", "liblog", "libcutils", + "libvintf", ], static_libs: [ diff --git a/libs/input/InputDevice.cpp b/libs/input/InputDevice.cpp index 0bee1b6f2a..a9089690b0 100644 --- a/libs/input/InputDevice.cpp +++ b/libs/input/InputDevice.cpp @@ -53,33 +53,39 @@ static void appendInputDeviceConfigurationFileRelativePath(std::string& path, } std::string getInputDeviceConfigurationFilePathByDeviceIdentifier( - const InputDeviceIdentifier& deviceIdentifier, - InputDeviceConfigurationFileType type) { + const InputDeviceIdentifier& deviceIdentifier, InputDeviceConfigurationFileType type, + const char* suffix) { if (deviceIdentifier.vendor !=0 && deviceIdentifier.product != 0) { if (deviceIdentifier.version != 0) { // Try vendor product version. - std::string versionPath = getInputDeviceConfigurationFilePathByName( - StringPrintf("Vendor_%04x_Product_%04x_Version_%04x", - deviceIdentifier.vendor, deviceIdentifier.product, - deviceIdentifier.version), - type); + std::string versionPath = + getInputDeviceConfigurationFilePathByName(StringPrintf("Vendor_%04x_Product_%" + "04x_Version_%04x%s", + deviceIdentifier.vendor, + deviceIdentifier.product, + deviceIdentifier.version, + suffix), + type); if (!versionPath.empty()) { return versionPath; } } // Try vendor product. - std::string productPath = getInputDeviceConfigurationFilePathByName( - StringPrintf("Vendor_%04x_Product_%04x", - deviceIdentifier.vendor, deviceIdentifier.product), - type); + std::string productPath = + getInputDeviceConfigurationFilePathByName(StringPrintf("Vendor_%04x_Product_%04x%s", + deviceIdentifier.vendor, + deviceIdentifier.product, + suffix), + type); if (!productPath.empty()) { return productPath; } } // Try device name. - return getInputDeviceConfigurationFilePathByName(deviceIdentifier.getCanonicalName(), type); + return getInputDeviceConfigurationFilePathByName(deviceIdentifier.getCanonicalName() + suffix, + type); } std::string getInputDeviceConfigurationFilePathByName( diff --git a/libs/input/KeyLayoutMap.cpp b/libs/input/KeyLayoutMap.cpp index 17c3bb36e3..170e748ca6 100644 --- a/libs/input/KeyLayoutMap.cpp +++ b/libs/input/KeyLayoutMap.cpp @@ -25,6 +25,8 @@ #include #include #include +#include +#include #include #include @@ -76,6 +78,29 @@ static const std::unordered_map SENSOR_ sensorPair(), sensorPair()}; +bool kernelConfigsArePresent(const std::set& configs) { + std::shared_ptr runtimeInfo = + android::vintf::VintfObject::GetInstance()->getRuntimeInfo( + vintf::RuntimeInfo::FetchFlag::CONFIG_GZ); + LOG_ALWAYS_FATAL_IF(runtimeInfo == nullptr, "Kernel configs could not be fetched"); + + const std::map& kernelConfigs = runtimeInfo->kernelConfigs(); + for (const std::string& requiredConfig : configs) { + const auto configIt = kernelConfigs.find(requiredConfig); + if (configIt == kernelConfigs.end()) { + ALOGI("Required kernel config %s is not found", requiredConfig.c_str()); + return false; + } + const std::string& option = configIt->second; + if (option != "y" && option != "m") { + ALOGI("Required kernel config %s has option %s", requiredConfig.c_str(), + option.c_str()); + return false; + } + } + return true; +} + } // namespace KeyLayoutMap::KeyLayoutMap() = default; @@ -83,32 +108,34 @@ KeyLayoutMap::~KeyLayoutMap() = default; base::Result> KeyLayoutMap::loadContents(const std::string& filename, const char* contents) { - Tokenizer* tokenizer; - status_t status = Tokenizer::fromContents(String8(filename.c_str()), contents, &tokenizer); - if (status) { - ALOGE("Error %d opening key layout map.", status); - return Errorf("Error {} opening key layout map file {}.", status, filename.c_str()); - } - std::unique_ptr t(tokenizer); - auto ret = load(t.get()); - if (ret.ok()) { - (*ret)->mLoadFileName = filename; - } - return ret; + return load(filename, contents); } -base::Result> KeyLayoutMap::load(const std::string& filename) { +base::Result> KeyLayoutMap::load(const std::string& filename, + const char* contents) { Tokenizer* tokenizer; - status_t status = Tokenizer::open(String8(filename.c_str()), &tokenizer); + status_t status; + if (contents == nullptr) { + status = Tokenizer::open(String8(filename.c_str()), &tokenizer); + } else { + status = Tokenizer::fromContents(String8(filename.c_str()), contents, &tokenizer); + } if (status) { ALOGE("Error %d opening key layout map file %s.", status, filename.c_str()); return Errorf("Error {} opening key layout map file {}.", status, filename.c_str()); } std::unique_ptr t(tokenizer); auto ret = load(t.get()); - if (ret.ok()) { - (*ret)->mLoadFileName = filename; + if (!ret.ok()) { + return ret; + } + const std::shared_ptr& map = *ret; + LOG_ALWAYS_FATAL_IF(map == nullptr, "Returned map should not be null if there's no error"); + if (!kernelConfigsArePresent(map->mRequiredKernelConfigs)) { + ALOGI("Not loading %s because the required kernel configs are not set", filename.c_str()); + return Errorf("Missing kernel config"); } + map->mLoadFileName = filename; return ret; } @@ -278,6 +305,10 @@ status_t KeyLayoutMap::Parser::parse() { mTokenizer->skipDelimiters(WHITESPACE); status_t status = parseSensor(); if (status) return status; + } else if (keywordToken == "requires_kernel_config") { + mTokenizer->skipDelimiters(WHITESPACE); + status_t status = parseRequiredKernelConfig(); + if (status) return status; } else { ALOGE("%s: Expected keyword, got '%s'.", mTokenizer->getLocation().string(), keywordToken.string()); @@ -579,4 +610,23 @@ status_t KeyLayoutMap::Parser::parseSensor() { return NO_ERROR; } +// Parse the name of a required kernel config. +// The layout won't be used if the specified kernel config is not present +// Examples: +// requires_kernel_config CONFIG_HID_PLAYSTATION +status_t KeyLayoutMap::Parser::parseRequiredKernelConfig() { + String8 codeToken = mTokenizer->nextToken(WHITESPACE); + std::string configName = codeToken.string(); + + const auto result = mMap->mRequiredKernelConfigs.emplace(configName); + if (!result.second) { + ALOGE("%s: Duplicate entry for required kernel config %s.", + mTokenizer->getLocation().string(), configName.c_str()); + return BAD_VALUE; + } + + ALOGD_IF(DEBUG_PARSER, "Parsed required kernel config: name=%s", configName.c_str()); + return NO_ERROR; +} + } // namespace android diff --git a/libs/input/Keyboard.cpp b/libs/input/Keyboard.cpp index f0895b32ef..c3f5151fd1 100644 --- a/libs/input/Keyboard.cpp +++ b/libs/input/Keyboard.cpp @@ -20,16 +20,23 @@ #include #include -#include +#include #include -#include #include -#include +#include +#include +#include #include -#include namespace android { +static std::string getPath(const InputDeviceIdentifier& deviceIdentifier, const std::string& name, + InputDeviceConfigurationFileType type) { + return name.empty() + ? getInputDeviceConfigurationFilePathByDeviceIdentifier(deviceIdentifier, type) + : getInputDeviceConfigurationFilePathByName(name, type); +} + // --- KeyMap --- KeyMap::KeyMap() { @@ -111,11 +118,25 @@ status_t KeyMap::loadKeyLayout(const InputDeviceIdentifier& deviceIdentifier, } base::Result> ret = KeyLayoutMap::load(path); + if (ret.ok()) { + keyLayoutMap = *ret; + keyLayoutFile = path; + return OK; + } + + // Try to load fallback layout if the regular layout could not be loaded due to missing + // kernel modules + std::string fallbackPath( + getInputDeviceConfigurationFilePathByDeviceIdentifier(deviceIdentifier, + InputDeviceConfigurationFileType:: + KEY_LAYOUT, + "_fallback")); + ret = KeyLayoutMap::load(fallbackPath); if (!ret.ok()) { return ret.error().code(); } keyLayoutMap = *ret; - keyLayoutFile = path; + keyLayoutFile = fallbackPath; return OK; } @@ -137,14 +158,6 @@ status_t KeyMap::loadKeyCharacterMap(const InputDeviceIdentifier& deviceIdentifi return OK; } -std::string KeyMap::getPath(const InputDeviceIdentifier& deviceIdentifier, - const std::string& name, InputDeviceConfigurationFileType type) { - return name.empty() - ? getInputDeviceConfigurationFilePathByDeviceIdentifier(deviceIdentifier, type) - : getInputDeviceConfigurationFilePathByName(name, type); -} - - // --- Global functions --- bool isKeyboardSpecialFunction(const PropertyMap* config) { diff --git a/libs/input/tests/Android.bp b/libs/input/tests/Android.bp index 6ffe8518b6..d947cd99e8 100644 --- a/libs/input/tests/Android.bp +++ b/libs/input/tests/Android.bp @@ -36,8 +36,9 @@ cc_test { "liblog", "libui", "libutils", + "libvintf", ], - data: ["data/*.kcm"], + data: ["data/*"], test_suites: ["device-tests"], } diff --git a/libs/input/tests/InputDevice_test.cpp b/libs/input/tests/InputDevice_test.cpp index 6b695c4581..e872fa442b 100644 --- a/libs/input/tests/InputDevice_test.cpp +++ b/libs/input/tests/InputDevice_test.cpp @@ -130,4 +130,20 @@ TEST_F(InputDeviceKeyMapTest, keyCharacteMapApplyMultipleOverlaysTest) { ASSERT_EQ(*mKeyMap.keyCharacterMap, *frenchOverlaidKeyCharacterMap); } +TEST(InputDeviceKeyLayoutTest, DoesNotLoadWhenRequiredKernelConfigIsMissing) { + std::string klPath = base::GetExecutableDirectory() + "/data/kl_with_required_fake_config.kl"; + base::Result> ret = KeyLayoutMap::load(klPath); + ASSERT_FALSE(ret.ok()) << "Should not be able to load KeyLayout at " << klPath; + // We assert error message here because it's used by 'validatekeymaps' tool + ASSERT_EQ("Missing kernel config", ret.error().message()); +} + +TEST(InputDeviceKeyLayoutTest, LoadsWhenRequiredKernelConfigIsPresent) { + std::string klPath = base::GetExecutableDirectory() + "/data/kl_with_required_real_config.kl"; + base::Result> ret = KeyLayoutMap::load(klPath); + ASSERT_TRUE(ret.ok()) << "Cannot load KeyLayout at " << klPath; + const std::shared_ptr& map = *ret; + ASSERT_NE(nullptr, map) << "Map should be valid because CONFIG_UHID should always be present"; +} + } // namespace android diff --git a/libs/input/tests/data/kl_with_required_fake_config.kl b/libs/input/tests/data/kl_with_required_fake_config.kl new file mode 100644 index 0000000000..2d0a507fbd --- /dev/null +++ b/libs/input/tests/data/kl_with_required_fake_config.kl @@ -0,0 +1,20 @@ +# Copyright (C) 2022 The Android Open Source Project +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# This KL should not be loaded unless the below config is present in the kernel +# This config will never exist, and therefore this KL should never be loaded +requires_kernel_config CONFIG_HID_FAKEMODULE + +# An arbitrary mapping taken from another file +key 0x130 BUTTON_X \ No newline at end of file diff --git a/libs/input/tests/data/kl_with_required_real_config.kl b/libs/input/tests/data/kl_with_required_real_config.kl new file mode 100644 index 0000000000..303b23e48a --- /dev/null +++ b/libs/input/tests/data/kl_with_required_real_config.kl @@ -0,0 +1,21 @@ +# Copyright (C) 2022 The Android Open Source Project +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# This KL should not be loaded unless the below config is present in the kernel +# The CONFIG_UHID option has been required for a while, and therefore it's safe +# to assume that this will always be loaded +requires_kernel_config CONFIG_UHID + +# An arbitrary mapping taken from another file +key 0x130 BUTTON_X \ No newline at end of file -- cgit v1.2.3-59-g8ed1b From 947d2a482e414ac471e507c8fd76d743b9f3d035 Mon Sep 17 00:00:00 2001 From: Brian Johnson Date: Fri, 12 Aug 2022 22:20:19 +0000 Subject: Plumb through Output::getSkipColorTransform() into CachedSet, to match the behavior in Output::composeSurfaces(). Bug: 240293363 Test: atest libcompositionengine_test. Fixed tests, added CachedSetTest::renderWhitePointNoColorTransform Change-Id: Ic0317b34978c2ae8d5c057c0a39ed889b86b3da0 Merged-In: Ic0317b34978c2ae8d5c057c0a39ed889b86b3da0 --- .../compositionengine/impl/planner/CachedSet.h | 2 +- .../compositionengine/impl/planner/Flattener.h | 3 +- .../compositionengine/impl/planner/Planner.h | 3 +- .../CompositionEngine/src/Output.cpp | 3 +- .../CompositionEngine/src/planner/CachedSet.cpp | 5 +- .../CompositionEngine/src/planner/Flattener.cpp | 5 +- .../CompositionEngine/src/planner/Planner.cpp | 8 +- .../tests/planner/CachedSetTest.cpp | 65 +++++++++++++-- .../tests/planner/FlattenerTest.cpp | 96 +++++++++++----------- 9 files changed, 125 insertions(+), 65 deletions(-) diff --git a/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/planner/CachedSet.h b/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/planner/CachedSet.h index e65aa7393c..8e4e9af48c 100644 --- a/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/planner/CachedSet.h +++ b/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/planner/CachedSet.h @@ -115,7 +115,7 @@ public: // Renders the cached set with the supplied output composition state. void render(renderengine::RenderEngine& re, TexturePool& texturePool, - const OutputCompositionState& outputState); + const OutputCompositionState& outputState, bool deviceHandlesColorTransform); void dump(std::string& result) const; diff --git a/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/planner/Flattener.h b/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/planner/Flattener.h index 92cc484211..f934cb20a0 100644 --- a/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/planner/Flattener.h +++ b/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/planner/Flattener.h @@ -106,7 +106,8 @@ public: // Renders the newest cached sets with the supplied output composition state void renderCachedSets(const OutputCompositionState& outputState, - std::optional renderDeadline); + std::optional renderDeadline, + bool deviceHandlesColorTransform); void setTexturePoolEnabled(bool enabled) { mTexturePool.setEnabled(enabled); } diff --git a/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/planner/Planner.h b/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/planner/Planner.h index b7ebca60fd..c968df708f 100644 --- a/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/planner/Planner.h +++ b/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/planner/Planner.h @@ -65,7 +65,8 @@ public: // Rendering a pending cached set is optional: if the renderDeadline is not far enough in the // future then the planner may opt to skip rendering the cached set. void renderCachedSets(const OutputCompositionState& outputState, - std::optional renderDeadline); + std::optional renderDeadline, + bool deviceHandlesColorTransform); void setTexturePoolEnabled(bool enabled) { mFlattener.setTexturePoolEnabled(enabled); } diff --git a/services/surfaceflinger/CompositionEngine/src/Output.cpp b/services/surfaceflinger/CompositionEngine/src/Output.cpp index bc11d87ebd..b724daa8ce 100644 --- a/services/surfaceflinger/CompositionEngine/src/Output.cpp +++ b/services/surfaceflinger/CompositionEngine/src/Output.cpp @@ -1494,7 +1494,8 @@ void Output::postFramebuffer() { void Output::renderCachedSets(const CompositionRefreshArgs& refreshArgs) { if (mPlanner) { - mPlanner->renderCachedSets(getState(), refreshArgs.scheduledFrameTime); + mPlanner->renderCachedSets(getState(), refreshArgs.scheduledFrameTime, + getState().usesDeviceComposition || getSkipColorTransform()); } } diff --git a/services/surfaceflinger/CompositionEngine/src/planner/CachedSet.cpp b/services/surfaceflinger/CompositionEngine/src/planner/CachedSet.cpp index 641b806aec..d6f02ee42a 100644 --- a/services/surfaceflinger/CompositionEngine/src/planner/CachedSet.cpp +++ b/services/surfaceflinger/CompositionEngine/src/planner/CachedSet.cpp @@ -159,7 +159,8 @@ void CachedSet::updateAge(std::chrono::steady_clock::time_point now) { } void CachedSet::render(renderengine::RenderEngine& renderEngine, TexturePool& texturePool, - const OutputCompositionState& outputState) { + const OutputCompositionState& outputState, + bool deviceHandlesColorTransform) { ATRACE_CALL(); const Rect& viewport = outputState.layerStackSpace.getContent(); const ui::Dataspace& outputDataspace = outputState.dataspace; @@ -170,6 +171,8 @@ void CachedSet::render(renderengine::RenderEngine& renderEngine, TexturePool& te .physicalDisplay = outputState.framebufferSpace.getContent(), .clip = viewport, .outputDataspace = outputDataspace, + .colorTransform = outputState.colorTransformMatrix, + .deviceHandlesColorTransform = deviceHandlesColorTransform, .orientation = orientation, .targetLuminanceNits = outputState.displayBrightnessNits, }; diff --git a/services/surfaceflinger/CompositionEngine/src/planner/Flattener.cpp b/services/surfaceflinger/CompositionEngine/src/planner/Flattener.cpp index 1062b700dd..9175dd01a1 100644 --- a/services/surfaceflinger/CompositionEngine/src/planner/Flattener.cpp +++ b/services/surfaceflinger/CompositionEngine/src/planner/Flattener.cpp @@ -99,7 +99,8 @@ NonBufferHash Flattener::flattenLayers(const std::vector& lay void Flattener::renderCachedSets( const OutputCompositionState& outputState, - std::optional renderDeadline) { + std::optional renderDeadline, + bool deviceHandlesColorTransform) { ATRACE_CALL(); if (!mNewCachedSet) { @@ -136,7 +137,7 @@ void Flattener::renderCachedSets( } } - mNewCachedSet->render(mRenderEngine, mTexturePool, outputState); + mNewCachedSet->render(mRenderEngine, mTexturePool, outputState, deviceHandlesColorTransform); } void Flattener::dumpLayers(std::string& result) const { diff --git a/services/surfaceflinger/CompositionEngine/src/planner/Planner.cpp b/services/surfaceflinger/CompositionEngine/src/planner/Planner.cpp index c8413eb8bc..54133d92b0 100644 --- a/services/surfaceflinger/CompositionEngine/src/planner/Planner.cpp +++ b/services/surfaceflinger/CompositionEngine/src/planner/Planner.cpp @@ -201,11 +201,11 @@ void Planner::reportFinalPlan( finalPlan); } -void Planner::renderCachedSets( - const OutputCompositionState& outputState, - std::optional renderDeadline) { +void Planner::renderCachedSets(const OutputCompositionState& outputState, + std::optional renderDeadline, + bool deviceHandlesColorTransform) { ATRACE_CALL(); - mFlattener.renderCachedSets(outputState, renderDeadline); + mFlattener.renderCachedSets(outputState, renderDeadline, deviceHandlesColorTransform); } void Planner::dump(const Vector& args, std::string& result) { diff --git a/services/surfaceflinger/CompositionEngine/tests/planner/CachedSetTest.cpp b/services/surfaceflinger/CompositionEngine/tests/planner/CachedSetTest.cpp index 8a99e4e2e8..0e9db369e8 100644 --- a/services/surfaceflinger/CompositionEngine/tests/planner/CachedSetTest.cpp +++ b/services/surfaceflinger/CompositionEngine/tests/planner/CachedSetTest.cpp @@ -376,7 +376,7 @@ TEST_F(CachedSetTest, renderUnsecureOutput) { .WillOnce(Return(clientCompList2)); EXPECT_CALL(mRenderEngine, drawLayers(_, _, _, _, _)).WillOnce(Invoke(drawLayers)); mOutputState.isSecure = false; - cachedSet.render(mRenderEngine, mTexturePool, mOutputState); + cachedSet.render(mRenderEngine, mTexturePool, mOutputState, true); expectReadyBuffer(cachedSet); EXPECT_EQ(mOutputState.framebufferSpace, cachedSet.getOutputSpace()); @@ -429,7 +429,7 @@ TEST_F(CachedSetTest, renderSecureOutput) { .WillOnce(Return(clientCompList2)); EXPECT_CALL(mRenderEngine, drawLayers(_, _, _, _, _)).WillOnce(Invoke(drawLayers)); mOutputState.isSecure = true; - cachedSet.render(mRenderEngine, mTexturePool, mOutputState); + cachedSet.render(mRenderEngine, mTexturePool, mOutputState, true); expectReadyBuffer(cachedSet); EXPECT_EQ(mOutputState.framebufferSpace, cachedSet.getOutputSpace()); @@ -477,7 +477,58 @@ TEST_F(CachedSetTest, renderWhitePoint) { .WillOnce(Return(clientCompList2)); EXPECT_CALL(mRenderEngine, drawLayers(_, _, _, _, _)).WillOnce(Invoke(drawLayers)); mOutputState.isSecure = true; - cachedSet.render(mRenderEngine, mTexturePool, mOutputState); + cachedSet.render(mRenderEngine, mTexturePool, mOutputState, true); + expectReadyBuffer(cachedSet); + + EXPECT_EQ(mOutputState.framebufferSpace, cachedSet.getOutputSpace()); + EXPECT_EQ(Rect(kOutputSize.width, kOutputSize.height), cachedSet.getTextureBounds()); + + // Now check that appending a new cached set properly cleans up RenderEngine resources. + CachedSet::Layer& layer3 = *mTestLayers[2]->cachedSetLayer.get(); + cachedSet.append(CachedSet(layer3)); +} + +TEST_F(CachedSetTest, renderWhitePointNoColorTransform) { + // Skip the 0th layer to ensure that the bounding box of the layers is offset from (0, 0) + // This is a duplicate of the "renderWhitePoint" test, but setting "deviceHandlesColorTransform" + // to false, in the render call. + + CachedSet::Layer& layer1 = *mTestLayers[1]->cachedSetLayer.get(); + sp layerFE1 = mTestLayers[1]->layerFE; + CachedSet::Layer& layer2 = *mTestLayers[2]->cachedSetLayer.get(); + sp layerFE2 = mTestLayers[2]->layerFE; + + CachedSet cachedSet(layer1); + cachedSet.append(CachedSet(layer2)); + + std::vector clientCompList1; + clientCompList1.push_back({}); + + std::vector clientCompList2; + clientCompList2.push_back({}); + + mOutputState.displayBrightnessNits = 400.f; + + const auto drawLayers = + [&](const renderengine::DisplaySettings& displaySettings, + const std::vector&, + const std::shared_ptr&, const bool, + base::unique_fd&&) -> std::future { + EXPECT_EQ(mOutputState.displayBrightnessNits, displaySettings.targetLuminanceNits); + return futureOf({NO_ERROR, base::unique_fd()}); + }; + + EXPECT_CALL(*layerFE1, + prepareClientCompositionList(ClientCompositionTargetSettingsWhitePointEq( + mOutputState.displayBrightnessNits))) + .WillOnce(Return(clientCompList1)); + EXPECT_CALL(*layerFE2, + prepareClientCompositionList(ClientCompositionTargetSettingsWhitePointEq( + mOutputState.displayBrightnessNits))) + .WillOnce(Return(clientCompList2)); + EXPECT_CALL(mRenderEngine, drawLayers(_, _, _, _, _)).WillOnce(Invoke(drawLayers)); + mOutputState.isSecure = true; + cachedSet.render(mRenderEngine, mTexturePool, mOutputState, false); expectReadyBuffer(cachedSet); EXPECT_EQ(mOutputState.framebufferSpace, cachedSet.getOutputSpace()); @@ -527,7 +578,7 @@ TEST_F(CachedSetTest, rendersWithOffsetFramebufferContent) { EXPECT_CALL(*layerFE1, prepareClientCompositionList(_)).WillOnce(Return(clientCompList1)); EXPECT_CALL(*layerFE2, prepareClientCompositionList(_)).WillOnce(Return(clientCompList2)); EXPECT_CALL(mRenderEngine, drawLayers(_, _, _, _, _)).WillOnce(Invoke(drawLayers)); - cachedSet.render(mRenderEngine, mTexturePool, mOutputState); + cachedSet.render(mRenderEngine, mTexturePool, mOutputState, true); expectReadyBuffer(cachedSet); EXPECT_EQ(mOutputState.framebufferSpace, cachedSet.getOutputSpace()); @@ -767,7 +818,7 @@ TEST_F(CachedSetTest, addHolePunch) { }; EXPECT_CALL(mRenderEngine, drawLayers(_, _, _, _, _)).WillOnce(Invoke(drawLayers)); - cachedSet.render(mRenderEngine, mTexturePool, mOutputState); + cachedSet.render(mRenderEngine, mTexturePool, mOutputState, true); } TEST_F(CachedSetTest, addHolePunch_noBuffer) { @@ -829,7 +880,7 @@ TEST_F(CachedSetTest, addHolePunch_noBuffer) { }; EXPECT_CALL(mRenderEngine, drawLayers(_, _, _, _, _)).WillOnce(Invoke(drawLayers)); - cachedSet.render(mRenderEngine, mTexturePool, mOutputState); + cachedSet.render(mRenderEngine, mTexturePool, mOutputState, true); } TEST_F(CachedSetTest, append_removesHolePunch) { @@ -969,7 +1020,7 @@ TEST_F(CachedSetTest, addBlur) { }; EXPECT_CALL(mRenderEngine, drawLayers(_, _, _, _, _)).WillOnce(Invoke(drawLayers)); - cachedSet.render(mRenderEngine, mTexturePool, mOutputState); + cachedSet.render(mRenderEngine, mTexturePool, mOutputState, true); } } // namespace diff --git a/services/surfaceflinger/CompositionEngine/tests/planner/FlattenerTest.cpp b/services/surfaceflinger/CompositionEngine/tests/planner/FlattenerTest.cpp index 50e3a288cb..96021ec104 100644 --- a/services/surfaceflinger/CompositionEngine/tests/planner/FlattenerTest.cpp +++ b/services/surfaceflinger/CompositionEngine/tests/planner/FlattenerTest.cpp @@ -159,13 +159,13 @@ void FlattenerTest::initializeFlattener(const std::vector& la initializeOverrideBuffer(layers); EXPECT_EQ(getNonBufferHash(layers), mFlattener->flattenLayers(layers, getNonBufferHash(layers), mTime)); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); // same geometry, update the internal layer stack initializeOverrideBuffer(layers); EXPECT_EQ(getNonBufferHash(layers), mFlattener->flattenLayers(layers, getNonBufferHash(layers), mTime)); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); } void FlattenerTest::expectAllLayersFlattened(const std::vector& layers) { @@ -177,7 +177,7 @@ void FlattenerTest::expectAllLayersFlattened(const std::vectorflattenLayers(layers, getNonBufferHash(layers), mTime)); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); for (const auto layer : layers) { EXPECT_EQ(nullptr, layer->getOutputLayer()->getState().overrideInfo.buffer); @@ -187,7 +187,7 @@ void FlattenerTest::expectAllLayersFlattened(const std::vectorflattenLayers(layers, getNonBufferHash(layers), mTime)); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); const auto buffer = layers[0]->getOutputLayer()->getState().overrideInfo.buffer; EXPECT_NE(nullptr, buffer); @@ -222,7 +222,7 @@ TEST_F(FlattenerTest, flattenLayers_ActiveLayersAreNotFlattened) { initializeOverrideBuffer(layers); EXPECT_EQ(getNonBufferHash(layers), mFlattener->flattenLayers(layers, getNonBufferHash(layers), mTime)); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); } TEST_F(FlattenerTest, flattenLayers_ActiveLayersWithLowFpsAreFlattened) { @@ -284,7 +284,7 @@ TEST_F(FlattenerTest, flattenLayers_FlattenedLayersStayFlattenWhenNoUpdate) { initializeOverrideBuffer(layers); EXPECT_NE(getNonBufferHash(layers), mFlattener->flattenLayers(layers, getNonBufferHash(layers), mTime)); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); EXPECT_NE(nullptr, overrideBuffer1); EXPECT_EQ(overrideBuffer1, overrideBuffer2); @@ -389,7 +389,7 @@ TEST_F(FlattenerTest, flattenLayers_addLayerToFlattenedCauseReset) { initializeOverrideBuffer(layers); EXPECT_EQ(getNonBufferHash(layers), mFlattener->flattenLayers(layers, getNonBufferHash(layers), mTime)); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); EXPECT_EQ(nullptr, overrideBuffer1); EXPECT_EQ(nullptr, overrideBuffer2); @@ -428,7 +428,7 @@ TEST_F(FlattenerTest, flattenLayers_BufferUpdateToFlatten) { initializeOverrideBuffer(layers); EXPECT_EQ(getNonBufferHash(layers), mFlattener->flattenLayers(layers, getNonBufferHash(layers), mTime)); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); EXPECT_EQ(nullptr, overrideBuffer1); EXPECT_EQ(nullptr, overrideBuffer2); @@ -437,7 +437,7 @@ TEST_F(FlattenerTest, flattenLayers_BufferUpdateToFlatten) { initializeOverrideBuffer(layers); EXPECT_NE(getNonBufferHash(layers), mFlattener->flattenLayers(layers, getNonBufferHash(layers), mTime)); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); EXPECT_EQ(nullptr, overrideBuffer1); EXPECT_NE(nullptr, overrideBuffer2); @@ -452,7 +452,7 @@ TEST_F(FlattenerTest, flattenLayers_BufferUpdateToFlatten) { initializeOverrideBuffer(layers); EXPECT_NE(getNonBufferHash(layers), mFlattener->flattenLayers(layers, getNonBufferHash(layers), mTime)); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); EXPECT_EQ(nullptr, overrideBuffer1); EXPECT_NE(nullptr, overrideBuffer2); @@ -461,7 +461,7 @@ TEST_F(FlattenerTest, flattenLayers_BufferUpdateToFlatten) { initializeOverrideBuffer(layers); EXPECT_NE(getNonBufferHash(layers), mFlattener->flattenLayers(layers, getNonBufferHash(layers), mTime)); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); EXPECT_NE(nullptr, overrideBuffer1); EXPECT_EQ(overrideBuffer1, overrideBuffer2); @@ -505,7 +505,7 @@ TEST_F(FlattenerTest, flattenLayers_BufferUpdateForMiddleLayer) { initializeOverrideBuffer(layers); EXPECT_EQ(getNonBufferHash(layers), mFlattener->flattenLayers(layers, getNonBufferHash(layers), mTime)); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); EXPECT_EQ(nullptr, overrideBuffer1); EXPECT_EQ(nullptr, overrideBuffer2); @@ -521,7 +521,7 @@ TEST_F(FlattenerTest, flattenLayers_BufferUpdateForMiddleLayer) { EXPECT_NE(getNonBufferHash(layers), mFlattener->flattenLayers(layers, getNonBufferHash(layers), mTime)); mOutputState.framebufferSpace.setOrientation(ui::ROTATION_90); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); EXPECT_NE(nullptr, overrideBuffer1); EXPECT_EQ(overrideBuffer1, overrideBuffer2); @@ -534,7 +534,7 @@ TEST_F(FlattenerTest, flattenLayers_BufferUpdateForMiddleLayer) { EXPECT_NE(getNonBufferHash(layers), mFlattener->flattenLayers(layers, getNonBufferHash(layers), mTime)); mOutputState.framebufferSpace.setOrientation(ui::ROTATION_180); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); EXPECT_NE(nullptr, overrideBuffer1); EXPECT_EQ(overrideBuffer1, overrideBuffer2); @@ -550,7 +550,7 @@ TEST_F(FlattenerTest, flattenLayers_BufferUpdateForMiddleLayer) { initializeOverrideBuffer(layers); EXPECT_NE(getNonBufferHash(layers), mFlattener->flattenLayers(layers, getNonBufferHash(layers), mTime)); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); EXPECT_NE(nullptr, overrideBuffer1); EXPECT_EQ(overrideBuffer1, overrideBuffer2); @@ -562,7 +562,7 @@ TEST_F(FlattenerTest, flattenLayers_BufferUpdateForMiddleLayer) { EXPECT_NE(getNonBufferHash(layers), mFlattener->flattenLayers(layers, getNonBufferHash(layers), mTime)); mOutputState.framebufferSpace.setOrientation(ui::ROTATION_270); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); EXPECT_NE(nullptr, overrideBuffer1); EXPECT_EQ(overrideBuffer1, overrideBuffer2); @@ -603,7 +603,7 @@ TEST_F(FlattenerTest, flattenLayers_pipRequiresRoundedCorners) { EXPECT_CALL(mRenderEngine, drawLayers(_, _, _, _, _)) .WillOnce(Return(ByMove( futureOf({NO_ERROR, base::unique_fd()})))); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); // We've rendered a CachedSet, but we haven't merged it in. EXPECT_EQ(nullptr, overrideBuffer1); @@ -616,7 +616,7 @@ TEST_F(FlattenerTest, flattenLayers_pipRequiresRoundedCorners) { initializeOverrideBuffer(layers); EXPECT_NE(getNonBufferHash(layers), mFlattener->flattenLayers(layers, getNonBufferHash(layers), mTime)); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); EXPECT_NE(nullptr, overrideBuffer1); EXPECT_EQ(overrideBuffer1, overrideBuffer2); @@ -669,7 +669,7 @@ TEST_F(FlattenerTest, flattenLayers_pip) { EXPECT_CALL(mRenderEngine, drawLayers(_, _, _, _, _)) .WillOnce(Return(ByMove( futureOf({NO_ERROR, base::unique_fd()})))); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); // We've rendered a CachedSet, but we haven't merged it in. EXPECT_EQ(nullptr, overrideBuffer1); @@ -682,7 +682,7 @@ TEST_F(FlattenerTest, flattenLayers_pip) { initializeOverrideBuffer(layers); EXPECT_NE(getNonBufferHash(layers), mFlattener->flattenLayers(layers, getNonBufferHash(layers), mTime)); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); EXPECT_NE(nullptr, overrideBuffer1); EXPECT_EQ(overrideBuffer1, overrideBuffer2); @@ -743,7 +743,7 @@ TEST_F(FlattenerTest, flattenLayers_holePunchSingleLayer) { EXPECT_CALL(mRenderEngine, drawLayers(_, _, _, _, _)) .WillOnce(Return(ByMove( futureOf({NO_ERROR, base::unique_fd()})))); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); // We've rendered a CachedSet, but we haven't merged it in. EXPECT_EQ(nullptr, overrideBuffer0); @@ -753,7 +753,7 @@ TEST_F(FlattenerTest, flattenLayers_holePunchSingleLayer) { initializeOverrideBuffer(layers); EXPECT_EQ(getNonBufferHash(layers), mFlattener->flattenLayers(layers, getNonBufferHash(layers), mTime)); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); EXPECT_NE(nullptr, overrideBuffer0); // got overridden EXPECT_EQ(nullptr, overrideBuffer1); // did not @@ -815,7 +815,7 @@ TEST_F(FlattenerTest, flattenLayers_holePunchSingleColorLayer) { EXPECT_CALL(mRenderEngine, drawLayers(_, _, _, _, _)) .WillOnce(Return(ByMove( futureOf({NO_ERROR, base::unique_fd()})))); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); // We've rendered a CachedSet, but we haven't merged it in. EXPECT_EQ(nullptr, overrideBuffer0); @@ -825,7 +825,7 @@ TEST_F(FlattenerTest, flattenLayers_holePunchSingleColorLayer) { initializeOverrideBuffer(layers); EXPECT_EQ(getNonBufferHash(layers), mFlattener->flattenLayers(layers, getNonBufferHash(layers), mTime)); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); EXPECT_NE(nullptr, overrideBuffer0); // got overridden EXPECT_EQ(nullptr, overrideBuffer1); // did not @@ -871,7 +871,7 @@ TEST_F(FlattenerTest, flattenLayers_flattensBlurBehindRunIfFirstRun) { initializeOverrideBuffer(layers); EXPECT_EQ(getNonBufferHash(layers), mFlattener->flattenLayers(layers, getNonBufferHash(layers), mTime)); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); for (const auto layer : layers) { EXPECT_EQ(nullptr, layer->getOutputLayer()->getState().overrideInfo.buffer); @@ -881,7 +881,7 @@ TEST_F(FlattenerTest, flattenLayers_flattensBlurBehindRunIfFirstRun) { initializeOverrideBuffer(layers); EXPECT_NE(getNonBufferHash(layers), mFlattener->flattenLayers(layers, getNonBufferHash(layers), mTime)); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); EXPECT_NE(nullptr, overrideBuffer1); EXPECT_EQ(overrideBuffer1, overrideBuffer2); EXPECT_EQ(nullptr, overrideBuffer3); @@ -917,7 +917,7 @@ TEST_F(FlattenerTest, flattenLayers_doesNotFlattenBlurBehindRun) { initializeOverrideBuffer(layers); EXPECT_EQ(getNonBufferHash(layers), mFlattener->flattenLayers(layers, getNonBufferHash(layers), mTime)); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); for (const auto layer : layers) { EXPECT_EQ(nullptr, layer->getOutputLayer()->getState().overrideInfo.buffer); @@ -928,7 +928,7 @@ TEST_F(FlattenerTest, flattenLayers_doesNotFlattenBlurBehindRun) { initializeOverrideBuffer(layers); EXPECT_EQ(getNonBufferHash(layers), mFlattener->flattenLayers(layers, getNonBufferHash(layers), mTime)); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); for (const auto layer : layers) { EXPECT_EQ(nullptr, layer->getOutputLayer()->getState().overrideInfo.buffer); } @@ -971,7 +971,7 @@ TEST_F(FlattenerTest, flattenLayers_flattenSkipsLayerWithBlurBehind) { initializeOverrideBuffer(layers); EXPECT_EQ(getNonBufferHash(layers), mFlattener->flattenLayers(layers, getNonBufferHash(layers), mTime)); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); for (const auto layer : layers) { EXPECT_EQ(nullptr, layer->getOutputLayer()->getState().overrideInfo.buffer); @@ -981,7 +981,7 @@ TEST_F(FlattenerTest, flattenLayers_flattenSkipsLayerWithBlurBehind) { initializeOverrideBuffer(layers); EXPECT_NE(getNonBufferHash(layers), mFlattener->flattenLayers(layers, getNonBufferHash(layers), mTime)); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); EXPECT_EQ(nullptr, overrideBuffer1); EXPECT_EQ(nullptr, blurOverrideBuffer); EXPECT_NE(nullptr, overrideBuffer3); @@ -1020,7 +1020,7 @@ TEST_F(FlattenerTest, flattenLayers_whenBlurLayerIsChanging_appliesBlurToInactiv initializeOverrideBuffer(layers); EXPECT_EQ(getNonBufferHash(layers), mFlattener->flattenLayers(layers, getNonBufferHash(layers), mTime)); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); const auto& cachedSet = mFlattener->getNewCachedSetForTesting(); ASSERT_NE(std::nullopt, cachedSet); @@ -1034,7 +1034,7 @@ TEST_F(FlattenerTest, flattenLayers_whenBlurLayerIsChanging_appliesBlurToInactiv initializeOverrideBuffer(layers); EXPECT_NE(getNonBufferHash(layers), mFlattener->flattenLayers(layers, getNonBufferHash(layers), mTime)); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); EXPECT_NE(nullptr, overrideBuffer1); EXPECT_EQ(overrideBuffer2, overrideBuffer1); EXPECT_EQ(nullptr, blurOverrideBuffer); @@ -1063,7 +1063,7 @@ TEST_F(FlattenerTest, flattenLayers_renderCachedSets_doesNotRenderTwice) { initializeOverrideBuffer(layers); EXPECT_EQ(getNonBufferHash(layers), mFlattener->flattenLayers(layers, getNonBufferHash(layers), mTime)); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); EXPECT_EQ(nullptr, overrideBuffer1); EXPECT_EQ(nullptr, overrideBuffer2); @@ -1071,12 +1071,12 @@ TEST_F(FlattenerTest, flattenLayers_renderCachedSets_doesNotRenderTwice) { // Simulate attempting to render prior to merging the new cached set with the layer stack. // Here we should not try to re-render. EXPECT_CALL(mRenderEngine, drawLayers(_, _, _, _, _)).Times(0); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); // We provide the override buffer now that it's rendered EXPECT_NE(getNonBufferHash(layers), mFlattener->flattenLayers(layers, getNonBufferHash(layers), mTime)); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); EXPECT_NE(nullptr, overrideBuffer1); EXPECT_EQ(overrideBuffer2, overrideBuffer1); @@ -1120,7 +1120,8 @@ TEST_F(FlattenerRenderSchedulingTest, flattenLayers_renderCachedSets_defersUpToM EXPECT_CALL(mRenderEngine, drawLayers(_, _, _, _, _)).Times(0); mFlattener->renderCachedSets(mOutputState, std::chrono::steady_clock::now() - - (kCachedSetRenderDuration + 10ms)); + (kCachedSetRenderDuration + 10ms), + true); } EXPECT_CALL(mRenderEngine, drawLayers(_, _, _, _, _)) @@ -1128,7 +1129,8 @@ TEST_F(FlattenerRenderSchedulingTest, flattenLayers_renderCachedSets_defersUpToM futureOf({NO_ERROR, base::unique_fd()})))); mFlattener->renderCachedSets(mOutputState, std::chrono::steady_clock::now() - - (kCachedSetRenderDuration + 10ms)); + (kCachedSetRenderDuration + 10ms), + true); } TEST_F(FlattenerTest, flattenLayers_skipsBT601_625) { @@ -1162,7 +1164,7 @@ TEST_F(FlattenerTest, flattenLayers_skipsBT601_625) { EXPECT_CALL(mRenderEngine, drawLayers(_, _, _, _, _)) .WillOnce(Return(ByMove( futureOf({NO_ERROR, base::unique_fd()})))); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); // We've rendered a CachedSet, but we haven't merged it in. EXPECT_EQ(nullptr, overrideBuffer1); @@ -1175,7 +1177,7 @@ TEST_F(FlattenerTest, flattenLayers_skipsBT601_625) { initializeOverrideBuffer(layers); EXPECT_NE(getNonBufferHash(layers), mFlattener->flattenLayers(layers, getNonBufferHash(layers), mTime)); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); EXPECT_NE(nullptr, overrideBuffer1); EXPECT_EQ(overrideBuffer1, overrideBuffer2); @@ -1213,7 +1215,7 @@ TEST_F(FlattenerTest, flattenLayers_skipsHDR) { EXPECT_CALL(mRenderEngine, drawLayers(_, _, _, _, _)) .WillOnce(Return(ByMove( futureOf({NO_ERROR, base::unique_fd()})))); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); // We've rendered a CachedSet, but we haven't merged it in. EXPECT_EQ(nullptr, overrideBuffer1); @@ -1226,7 +1228,7 @@ TEST_F(FlattenerTest, flattenLayers_skipsHDR) { initializeOverrideBuffer(layers); EXPECT_NE(getNonBufferHash(layers), mFlattener->flattenLayers(layers, getNonBufferHash(layers), mTime)); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); EXPECT_NE(nullptr, overrideBuffer1); EXPECT_EQ(overrideBuffer1, overrideBuffer2); @@ -1264,7 +1266,7 @@ TEST_F(FlattenerTest, flattenLayers_skipsHDR2) { EXPECT_CALL(mRenderEngine, drawLayers(_, _, _, _, _)) .WillOnce(Return(ByMove( futureOf({NO_ERROR, base::unique_fd()})))); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); // We've rendered a CachedSet, but we haven't merged it in. EXPECT_EQ(nullptr, overrideBuffer1); @@ -1277,7 +1279,7 @@ TEST_F(FlattenerTest, flattenLayers_skipsHDR2) { initializeOverrideBuffer(layers); EXPECT_NE(getNonBufferHash(layers), mFlattener->flattenLayers(layers, getNonBufferHash(layers), mTime)); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); EXPECT_NE(nullptr, overrideBuffer1); EXPECT_EQ(overrideBuffer1, overrideBuffer2); @@ -1318,7 +1320,7 @@ TEST_F(FlattenerTest, flattenLayers_skipsColorLayers) { EXPECT_CALL(mRenderEngine, drawLayers(_, _, _, _, _)) .WillOnce(Return(ByMove( futureOf({NO_ERROR, base::unique_fd()})))); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); // We've rendered a CachedSet, but we haven't merged it in. EXPECT_EQ(nullptr, overrideBuffer1); @@ -1332,7 +1334,7 @@ TEST_F(FlattenerTest, flattenLayers_skipsColorLayers) { initializeOverrideBuffer(layers); EXPECT_NE(getNonBufferHash(layers), mFlattener->flattenLayers(layers, getNonBufferHash(layers), mTime)); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); EXPECT_EQ(nullptr, overrideBuffer1); EXPECT_EQ(nullptr, overrideBuffer2); @@ -1371,7 +1373,7 @@ TEST_F(FlattenerTest, flattenLayers_includes_DISPLAY_DECORATION) { EXPECT_CALL(mRenderEngine, drawLayers(_, _, _, _, _)) .WillOnce(Return(ByMove( futureOf({NO_ERROR, base::unique_fd()})))); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); // We've rendered a CachedSet, but we haven't merged it in. EXPECT_EQ(nullptr, overrideBuffer1); @@ -1384,7 +1386,7 @@ TEST_F(FlattenerTest, flattenLayers_includes_DISPLAY_DECORATION) { initializeOverrideBuffer(layers); EXPECT_NE(getNonBufferHash(layers), mFlattener->flattenLayers(layers, getNonBufferHash(layers), mTime)); - mFlattener->renderCachedSets(mOutputState, std::nullopt); + mFlattener->renderCachedSets(mOutputState, std::nullopt, true); EXPECT_NE(nullptr, overrideBuffer1); EXPECT_EQ(overrideBuffer1, overrideBuffer2); -- cgit v1.2.3-59-g8ed1b From fad66b2c0b0dba8e6df0e023b5f199dbf0598968 Mon Sep 17 00:00:00 2001 From: Ady Abraham Date: Sat, 13 Aug 2022 05:12:13 +0000 Subject: SF: fix refresh rate scoring when frameRateMultipleThreshold is used We can't exclude layers that are below the frameRateMultipleThreshold when scoring the higher refresh rates (the ones above the threshold) as it creates gives the lower refresh rates a higher score (as there are more layers that are allowed to score them). Instead we use a different approach which first scores the refresh rates based on all the layers but the fixed source ones, and then we add the fixed source ones for the refresh rates below the thresholds and for the above ones only when other layers already scored them. Bug: 242283390 Test: newly added unit tests scenarios SF: layers above the frameRateMultipleThreshold should always be counted An additional fix on top of ae2e3c741f0d15d0ed64afd2df93e8c4b2d76cfb which also include layers that have a desired refresh rate that was calculated hueristically and matches the max refresh rate. Bug: 242283390 Test: newly added unit tests scenarios Change-Id: I85534a3e004f372349dbf923ee6013855a54e4fa Merged-In: Ibe30ddd306265507ceedff6a6a725dadadc50af2 Merged-In: I85534a3e004f372349dbf923ee6013855a54e4fa --- .../Scheduler/RefreshRateConfigs.cpp | 142 ++++++++++++++------- .../surfaceflinger/Scheduler/RefreshRateConfigs.h | 3 - .../tests/unittests/RefreshRateConfigsTest.cpp | 45 ++++++- 3 files changed, 140 insertions(+), 50 deletions(-) diff --git a/services/surfaceflinger/Scheduler/RefreshRateConfigs.cpp b/services/surfaceflinger/Scheduler/RefreshRateConfigs.cpp index ca8349636b..a48c921378 100644 --- a/services/surfaceflinger/Scheduler/RefreshRateConfigs.cpp +++ b/services/surfaceflinger/Scheduler/RefreshRateConfigs.cpp @@ -40,22 +40,26 @@ namespace { struct RefreshRateScore { DisplayModeIterator modeIt; - float score; + float overallScore; + struct { + float modeBelowThreshold; + float modeAboveThreshold; + } fixedRateBelowThresholdLayersScore; }; template const DisplayModePtr& getMaxScoreRefreshRate(Iterator begin, Iterator end) { const auto it = std::max_element(begin, end, [](RefreshRateScore max, RefreshRateScore current) { - const auto& [modeIt, score] = current; + const auto& [modeIt, overallScore, _] = current; std::string name = to_string(modeIt->second->getFps()); - ALOGV("%s scores %.2f", name.c_str(), score); + ALOGV("%s scores %.2f", name.c_str(), overallScore); - ATRACE_INT(name.c_str(), static_cast(std::round(score * 100))); + ATRACE_INT(name.c_str(), static_cast(std::round(overallScore * 100))); constexpr float kEpsilon = 0.0001f; - return score > max.score * (1 + kEpsilon); + return overallScore > max.overallScore * (1 + kEpsilon); }); return it->modeIt->second; @@ -151,31 +155,6 @@ std::pair RefreshRateConfigs::getDisplayFrames(nsecs_t layerPe return {quotient, remainder}; } -bool RefreshRateConfigs::isVoteAllowed(const LayerRequirement& layer, Fps refreshRate) const { - using namespace fps_approx_ops; - - switch (layer.vote) { - case LayerVoteType::ExplicitExactOrMultiple: - case LayerVoteType::Heuristic: - if (mConfig.frameRateMultipleThreshold != 0 && - refreshRate >= Fps::fromValue(mConfig.frameRateMultipleThreshold) && - layer.desiredRefreshRate < Fps::fromValue(mConfig.frameRateMultipleThreshold / 2)) { - // Don't vote high refresh rates past the threshold for layers with a low desired - // refresh rate. For example, desired 24 fps with 120 Hz threshold means no vote for - // 120 Hz, but desired 60 fps should have a vote. - return false; - } - break; - case LayerVoteType::ExplicitDefault: - case LayerVoteType::ExplicitExact: - case LayerVoteType::Max: - case LayerVoteType::Min: - case LayerVoteType::NoVote: - break; - } - return true; -} - float RefreshRateConfigs::calculateNonExactMatchingLayerScoreLocked(const LayerRequirement& layer, Fps refreshRate) const { constexpr float kScoreForFractionalPairs = .8f; @@ -240,10 +219,6 @@ float RefreshRateConfigs::calculateNonExactMatchingLayerScoreLocked(const LayerR float RefreshRateConfigs::calculateLayerScoreLocked(const LayerRequirement& layer, Fps refreshRate, bool isSeamlessSwitch) const { - if (!isVoteAllowed(layer, refreshRate)) { - return 0; - } - // Slightly prefer seamless switches. constexpr float kSeamedSwitchPenalty = 0.95f; const float seamlessness = isSeamlessSwitch ? 1.0f : kSeamedSwitchPenalty; @@ -300,6 +275,7 @@ auto RefreshRateConfigs::getBestRefreshRate(const std::vector& auto RefreshRateConfigs::getBestRefreshRateLocked(const std::vector& layers, GlobalSignals signals) const -> std::pair { + using namespace fps_approx_ops; ATRACE_CALL(); ALOGV("%s: %zu layers", __func__, layers.size()); @@ -409,7 +385,7 @@ auto RefreshRateConfigs::getBestRefreshRateLocked(const std::vectorgetGroup() == mActiveModeIt->second->getGroup(); @@ -451,18 +427,92 @@ auto RefreshRateConfigs::getBestRefreshRateLocked(const std::vectorgetFps(), isSeamlessSwitch); - ALOGV("%s gives %s score of %.4f", formatLayerInfo(layer, weight).c_str(), - to_string(mode->getFps()).c_str(), layerScore); + const float weightedLayerScore = weight * layerScore; + + // Layer with fixed source has a special consideration which depends on the + // mConfig.frameRateMultipleThreshold. We don't want these layers to score + // refresh rates above the threshold, but we also don't want to favor the lower + // ones by having a greater number of layers scoring them. Instead, we calculate + // the score independently for these layers and later decide which + // refresh rates to add it. For example, desired 24 fps with 120 Hz threshold should not + // score 120 Hz, but desired 60 fps should contribute to the score. + const bool fixedSourceLayer = [](LayerVoteType vote) { + switch (vote) { + case LayerVoteType::ExplicitExactOrMultiple: + case LayerVoteType::Heuristic: + return true; + case LayerVoteType::NoVote: + case LayerVoteType::Min: + case LayerVoteType::Max: + case LayerVoteType::ExplicitDefault: + case LayerVoteType::ExplicitExact: + return false; + } + }(layer.vote); + const bool layerBelowThreshold = mConfig.frameRateMultipleThreshold != 0 && + layer.desiredRefreshRate < + Fps::fromValue(mConfig.frameRateMultipleThreshold / 2); + if (fixedSourceLayer && layerBelowThreshold) { + const bool modeAboveThreshold = + mode->getFps() >= Fps::fromValue(mConfig.frameRateMultipleThreshold); + if (modeAboveThreshold) { + ALOGV("%s gives %s fixed source (above threshold) score of %.4f", + formatLayerInfo(layer, weight).c_str(), to_string(mode->getFps()).c_str(), + layerScore); + fixedRateBelowThresholdLayersScore.modeAboveThreshold += weightedLayerScore; + } else { + ALOGV("%s gives %s fixed source (below threshold) score of %.4f", + formatLayerInfo(layer, weight).c_str(), to_string(mode->getFps()).c_str(), + layerScore); + fixedRateBelowThresholdLayersScore.modeBelowThreshold += weightedLayerScore; + } + } else { + ALOGV("%s gives %s score of %.4f", formatLayerInfo(layer, weight).c_str(), + to_string(mode->getFps()).c_str(), layerScore); + overallScore += weightedLayerScore; + } + } + } + + // We want to find the best refresh rate without the fixed source layers, + // so we could know whether we should add the modeAboveThreshold scores or not. + // If the best refresh rate is already above the threshold, it means that + // some non-fixed source layers already scored it, so we can just add the score + // for all fixed source layers, even the ones that are above the threshold. + const bool maxScoreAboveThreshold = [&] { + if (mConfig.frameRateMultipleThreshold == 0 || scores.empty()) { + return false; + } + + const auto maxScoreIt = + std::max_element(scores.begin(), scores.end(), + [](RefreshRateScore max, RefreshRateScore current) { + const auto& [modeIt, overallScore, _] = current; + return overallScore > max.overallScore; + }); + ALOGV("%s is the best refresh rate without fixed source layers. It is %s the threshold for " + "refresh rate multiples", + to_string(maxScoreIt->modeIt->second->getFps()).c_str(), + maxScoreAboveThreshold ? "above" : "below"); + return maxScoreIt->modeIt->second->getFps() >= + Fps::fromValue(mConfig.frameRateMultipleThreshold); + }(); - score += weight * layerScore; + // Now we can add the fixed rate layers score + for (auto& [modeIt, overallScore, fixedRateBelowThresholdLayersScore] : scores) { + overallScore += fixedRateBelowThresholdLayersScore.modeBelowThreshold; + if (maxScoreAboveThreshold) { + overallScore += fixedRateBelowThresholdLayersScore.modeAboveThreshold; } + ALOGV("%s adjusted overallScore is %.4f", to_string(modeIt->second->getFps()).c_str(), + overallScore); } - // Now that we scored all the refresh rates we need to pick the one that got the highest score. - // In case of a tie we will pick the higher refresh rate if any of the layers wanted Max, - // or the lower otherwise. + // Now that we scored all the refresh rates we need to pick the one that got the highest + // overallScore. In case of a tie we will pick the higher refresh rate if any of the layers + // wanted Max, or the lower otherwise. const DisplayModePtr& bestRefreshRate = maxVoteLayers > 0 ? getMaxScoreRefreshRate(scores.rbegin(), scores.rend()) : getMaxScoreRefreshRate(scores.begin(), scores.end()); @@ -471,7 +521,7 @@ auto RefreshRateConfigs::getBestRefreshRateLocked(const std::vectorgetFps()).c_str()); return {max, kNoSignals}; @@ -575,7 +625,7 @@ RefreshRateConfigs::UidToFrameRateOverride RefreshRateConfigs::getFrameRateOverr continue; } - for (auto& [_, score] : scores) { + for (auto& [_, score, _1] : scores) { score = 0; } @@ -587,7 +637,7 @@ RefreshRateConfigs::UidToFrameRateOverride RefreshRateConfigs::getFrameRateOverr LOG_ALWAYS_FATAL_IF(layer->vote != LayerVoteType::ExplicitDefault && layer->vote != LayerVoteType::ExplicitExactOrMultiple && layer->vote != LayerVoteType::ExplicitExact); - for (auto& [modeIt, score] : scores) { + for (auto& [modeIt, score, _] : scores) { constexpr bool isSeamlessSwitch = true; const auto layerScore = calculateLayerScoreLocked(*layer, modeIt->second->getFps(), isSeamlessSwitch); @@ -605,7 +655,7 @@ RefreshRateConfigs::UidToFrameRateOverride RefreshRateConfigs::getFrameRateOverr // If we never scored any layers, we don't have a preferred frame rate if (std::all_of(scores.begin(), scores.end(), - [](RefreshRateScore score) { return score.score == 0; })) { + [](RefreshRateScore score) { return score.overallScore == 0; })) { continue; } diff --git a/services/surfaceflinger/Scheduler/RefreshRateConfigs.h b/services/surfaceflinger/Scheduler/RefreshRateConfigs.h index 05a8692f51..a79002e959 100644 --- a/services/surfaceflinger/Scheduler/RefreshRateConfigs.h +++ b/services/surfaceflinger/Scheduler/RefreshRateConfigs.h @@ -353,9 +353,6 @@ private: const Policy* getCurrentPolicyLocked() const REQUIRES(mLock); bool isPolicyValidLocked(const Policy& policy) const REQUIRES(mLock); - // Returns whether the layer is allowed to vote for the given refresh rate. - bool isVoteAllowed(const LayerRequirement&, Fps) const; - // calculates a score for a layer. Used to determine the display refresh rate // and the frame rate override for certains applications. float calculateLayerScoreLocked(const LayerRequirement&, Fps refreshRate, diff --git a/services/surfaceflinger/tests/unittests/RefreshRateConfigsTest.cpp b/services/surfaceflinger/tests/unittests/RefreshRateConfigsTest.cpp index fcde532b85..188fd58dea 100644 --- a/services/surfaceflinger/tests/unittests/RefreshRateConfigsTest.cpp +++ b/services/surfaceflinger/tests/unittests/RefreshRateConfigsTest.cpp @@ -564,9 +564,10 @@ TEST_F(RefreshRateConfigsTest, getBestRefreshRate_30_60_90_120_DifferentTypes_mu TestableRefreshRateConfigs configs(kModes_30_60_72_90_120, kModeId60, {.frameRateMultipleThreshold = 120}); - std::vector layers = {{.weight = 1.f}, {.weight = 1.f}}; + std::vector layers = {{.weight = 1.f}, {.weight = 1.f}, {.weight = 1.f}}; auto& lr1 = layers[0]; auto& lr2 = layers[1]; + auto& lr3 = layers[2]; lr1.desiredRefreshRate = 24_Hz; lr1.vote = LayerVoteType::ExplicitDefault; @@ -639,6 +640,48 @@ TEST_F(RefreshRateConfigsTest, getBestRefreshRate_30_60_90_120_DifferentTypes_mu lr2.vote = LayerVoteType::ExplicitExactOrMultiple; lr2.name = "90Hz ExplicitExactOrMultiple"; EXPECT_EQ(kMode90, configs.getBestRefreshRate(layers)); + + lr1.desiredRefreshRate = 24_Hz; + lr1.vote = LayerVoteType::ExplicitExactOrMultiple; + lr1.name = "24Hz ExplicitExactOrMultiple"; + lr2.vote = LayerVoteType::Max; + lr2.name = "Max"; + EXPECT_EQ(kMode120, configs.getBestRefreshRate(layers)); + + lr1.desiredRefreshRate = 24_Hz; + lr1.vote = LayerVoteType::ExplicitExactOrMultiple; + lr1.name = "24Hz ExplicitExactOrMultiple"; + lr2.desiredRefreshRate = 120_Hz; + lr2.vote = LayerVoteType::ExplicitDefault; + lr2.name = "120Hz ExplicitDefault"; + EXPECT_EQ(kMode120, configs.getBestRefreshRate(layers)); + + lr1.desiredRefreshRate = 24_Hz; + lr1.vote = LayerVoteType::ExplicitExactOrMultiple; + lr1.name = "24Hz ExplicitExactOrMultiple"; + lr2.desiredRefreshRate = 120_Hz; + lr2.vote = LayerVoteType::ExplicitExact; + lr2.name = "120Hz ExplicitExact"; + EXPECT_EQ(kMode120, configs.getBestRefreshRate(layers)); + + lr1.desiredRefreshRate = 10_Hz; + lr1.vote = LayerVoteType::ExplicitExactOrMultiple; + lr1.name = "30Hz ExplicitExactOrMultiple"; + lr2.desiredRefreshRate = 120_Hz; + lr2.vote = LayerVoteType::Heuristic; + lr2.name = "120Hz ExplicitExact"; + EXPECT_EQ(kMode120, configs.getBestRefreshRate(layers)); + + lr1.desiredRefreshRate = 30_Hz; + lr1.vote = LayerVoteType::ExplicitExactOrMultiple; + lr1.name = "30Hz ExplicitExactOrMultiple"; + lr2.desiredRefreshRate = 30_Hz; + lr2.vote = LayerVoteType::ExplicitExactOrMultiple; + lr2.name = "30Hz ExplicitExactOrMultiple"; + lr3.vote = LayerVoteType::Heuristic; + lr3.desiredRefreshRate = 120_Hz; + lr3.name = "120Hz Heuristic"; + EXPECT_EQ(kMode120, configs.getBestRefreshRate(layers)); } TEST_F(RefreshRateConfigsTest, getBestRefreshRate_30_60) { -- cgit v1.2.3-59-g8ed1b From e80593238c3af1dea4885ca0e32d44392f3d42ee Mon Sep 17 00:00:00 2001 From: linpeter Date: Wed, 13 Jul 2022 19:20:48 +0800 Subject: Allow first power mode as off state Bug: 237745199 test: atest libsurfaceflinger_unittest:OnInitializeDisplaysTest atest libsurfaceflinger_unittest:SetPowerModeInternalTest Merged-In: I3a2eae4efc4a5c6113700a9ca9e9b261e364a878 Change-Id: I3a2eae4efc4a5c6113700a9ca9e9b261e364a878 --- services/surfaceflinger/DisplayDevice.cpp | 12 +++++++----- services/surfaceflinger/DisplayDevice.h | 9 ++++----- services/surfaceflinger/SurfaceFlinger.cpp | 13 +++++++------ .../surfaceflinger/tests/unittests/TestableSurfaceFlinger.h | 1 + 4 files changed, 19 insertions(+), 16 deletions(-) diff --git a/services/surfaceflinger/DisplayDevice.cpp b/services/surfaceflinger/DisplayDevice.cpp index 86809007b4..f3b6254b52 100644 --- a/services/surfaceflinger/DisplayDevice.cpp +++ b/services/surfaceflinger/DisplayDevice.cpp @@ -104,7 +104,7 @@ DisplayDevice::DisplayDevice(DisplayDeviceCreationArgs& args) mCompositionDisplay->getRenderSurface()->initialize(); - setPowerMode(args.initialPowerMode); + if (args.initialPowerMode.has_value()) setPowerMode(args.initialPowerMode.value()); // initialize the display orientation transform. setProjection(ui::ROTATION_0, Rect::INVALID_RECT, Rect::INVALID_RECT); @@ -174,19 +174,21 @@ auto DisplayDevice::getInputInfo() const -> InputInfo { void DisplayDevice::setPowerMode(hal::PowerMode mode) { mPowerMode = mode; - getCompositionDisplay()->setCompositionEnabled(mPowerMode != hal::PowerMode::OFF); + + getCompositionDisplay()->setCompositionEnabled(mPowerMode.has_value() && + *mPowerMode != hal::PowerMode::OFF); } void DisplayDevice::enableLayerCaching(bool enable) { getCompositionDisplay()->setLayerCachingEnabled(enable); } -hal::PowerMode DisplayDevice::getPowerMode() const { +std::optional DisplayDevice::getPowerMode() const { return mPowerMode; } bool DisplayDevice::isPoweredOn() const { - return mPowerMode != hal::PowerMode::OFF; + return mPowerMode && *mPowerMode != hal::PowerMode::OFF; } void DisplayDevice::setActiveMode(DisplayModeId id) { @@ -373,7 +375,7 @@ void DisplayDevice::dump(std::string& result) const { } result += "\n powerMode="s; - result += to_string(mPowerMode); + result += mPowerMode.has_value() ? to_string(mPowerMode.value()) : "OFF(reset)"; result += '\n'; if (mRefreshRateConfigs) { diff --git a/services/surfaceflinger/DisplayDevice.h b/services/surfaceflinger/DisplayDevice.h index 2161436d44..fc24a9ce49 100644 --- a/services/surfaceflinger/DisplayDevice.h +++ b/services/surfaceflinger/DisplayDevice.h @@ -181,7 +181,7 @@ public: /* ------------------------------------------------------------------------ * Display power mode management. */ - hardware::graphics::composer::hal::PowerMode getPowerMode() const; + std::optional getPowerMode() const; void setPowerMode(hardware::graphics::composer::hal::PowerMode mode); bool isPoweredOn() const; @@ -281,8 +281,8 @@ private: static ui::Transform::RotationFlags sPrimaryDisplayRotationFlags; - hardware::graphics::composer::hal::PowerMode mPowerMode = - hardware::graphics::composer::hal::PowerMode::OFF; + // allow initial power mode as null. + std::optional mPowerMode; DisplayModePtr mActiveMode; std::optional mStagedBrightness = std::nullopt; float mBrightness = -1.f; @@ -366,8 +366,7 @@ struct DisplayDeviceCreationArgs { HdrCapabilities hdrCapabilities; int32_t supportedPerFrameMetadata{0}; std::unordered_map> hwcColorModes; - hardware::graphics::composer::hal::PowerMode initialPowerMode{ - hardware::graphics::composer::hal::PowerMode::ON}; + std::optional initialPowerMode; bool isPrimary{false}; DisplayModes supportedModes; DisplayModeId activeModeId; diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index fd824dc502..db2447c40b 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -2889,7 +2889,8 @@ sp SurfaceFlinger::setupNewDisplayDeviceInternal( ALOGV("Display Orientation: %s", toCString(creationArgs.physicalOrientation)); // virtual displays are always considered enabled - creationArgs.initialPowerMode = state.isVirtual() ? hal::PowerMode::ON : hal::PowerMode::OFF; + creationArgs.initialPowerMode = + state.isVirtual() ? std::make_optional(hal::PowerMode::ON) : std::nullopt; sp display = getFactory().createDisplayDevice(creationArgs); @@ -4870,8 +4871,8 @@ void SurfaceFlinger::setPowerModeInternal(const sp& display, hal: const auto displayId = display->getPhysicalId(); ALOGD("Setting power mode %d on display %s", mode, to_string(displayId).c_str()); - const hal::PowerMode currentMode = display->getPowerMode(); - if (mode == currentMode) { + std::optional currentMode = display->getPowerMode(); + if (currentMode.has_value() && mode == *currentMode) { return; } @@ -4887,7 +4888,7 @@ void SurfaceFlinger::setPowerModeInternal(const sp& display, hal: mInterceptor->savePowerModeUpdate(display->getSequenceId(), static_cast(mode)); } const auto refreshRate = display->refreshRateConfigs().getActiveMode()->getFps(); - if (currentMode == hal::PowerMode::OFF) { + if (*currentMode == hal::PowerMode::OFF) { // Turn on the display if (display->isInternal() && (!activeDisplay || !activeDisplay->isPoweredOn())) { onActiveDisplayChangedLocked(display); @@ -4918,7 +4919,7 @@ void SurfaceFlinger::setPowerModeInternal(const sp& display, hal: if (SurfaceFlinger::setSchedAttr(false) != NO_ERROR) { ALOGW("Couldn't set uclamp.min on display off: %s\n", strerror(errno)); } - if (isDisplayActiveLocked(display) && currentMode != hal::PowerMode::DOZE_SUSPEND) { + if (isDisplayActiveLocked(display) && *currentMode != hal::PowerMode::DOZE_SUSPEND) { mScheduler->disableHardwareVsync(true); mScheduler->onScreenReleased(mAppConnectionHandle); } @@ -4932,7 +4933,7 @@ void SurfaceFlinger::setPowerModeInternal(const sp& display, hal: } else if (mode == hal::PowerMode::DOZE || mode == hal::PowerMode::ON) { // Update display while dozing getHwComposer().setPowerMode(displayId, mode); - if (isDisplayActiveLocked(display) && currentMode == hal::PowerMode::DOZE_SUSPEND) { + if (isDisplayActiveLocked(display) && *currentMode == hal::PowerMode::DOZE_SUSPEND) { mScheduler->onScreenAcquired(mAppConnectionHandle); mScheduler->resyncToHardwareVsync(true, refreshRate); } diff --git a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h index b70fdcd93e..283f9ca77b 100644 --- a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h +++ b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h @@ -745,6 +745,7 @@ public: mHwcDisplayId(hwcDisplayId) { mCreationArgs.connectionType = connectionType; mCreationArgs.isPrimary = isPrimary; + mCreationArgs.initialPowerMode = hal::PowerMode::ON; } sp token() const { return mDisplayToken; } -- cgit v1.2.3-59-g8ed1b From 83dde25b609e524ba3560b2cc2b6da01ebb26dd4 Mon Sep 17 00:00:00 2001 From: Brian Duddie Date: Thu, 25 Aug 2022 18:54:13 +0000 Subject: Fix double-close on direct channel registration In the AIDL sensor HAL wrapper, file descriptors associated with a direct channel were being wrapped in the AIDL NativeHandle type using makeToAidl(), which ends up taking ownership of the fds, and unintentionally closing them when the object goes out of scope (via ndk::ScopedFileDescriptor), so the same fds would be closed at a later point when the original native_handle_t is closed. Switch to dupToAidl() which does not take ownership of the input file handles. Bug: 234456046 Test: apply fdsan protection (in different CL), confirm via test-sensorservice that the file descriptor is not closed twice Change-Id: I51c0ba0f31b43c56bf055d186a599b289ca0065f --- services/sensorservice/AidlSensorHalWrapper.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/sensorservice/AidlSensorHalWrapper.cpp b/services/sensorservice/AidlSensorHalWrapper.cpp index 4d1de96caa..f67c610550 100644 --- a/services/sensorservice/AidlSensorHalWrapper.cpp +++ b/services/sensorservice/AidlSensorHalWrapper.cpp @@ -726,7 +726,7 @@ status_t AidlSensorHalWrapper::registerDirectChannel(const sensors_direct_mem_t .type = type, .format = format, .size = static_cast(memory->size), - .memoryHandle = makeToAidl(memory->handle), + .memoryHandle = dupToAidl(memory->handle), }; return convertToStatus(mSensors->registerDirectChannel(mem, channelHandle)); -- cgit v1.2.3-59-g8ed1b From a4aba5d7dd1467f9a4a80c7836e50b6465b735c4 Mon Sep 17 00:00:00 2001 From: Rachel Lee Date: Tue, 23 Aug 2022 14:37:59 -0700 Subject: Clarify new Choreographer Android 13 NDK docs. Bug: 227383300 Test: doxygen local generated docs Change-Id: Iee46c5b4ae053fb9a72511329d6e974afa59e478 (cherry picked from commit e81bf2616ea406d9ee9deb038c6978cdb84d6d35) --- include/android/choreographer.h | 62 ++++++++++++++++++++++++++++++++------- include/android/surface_control.h | 18 ++++++------ 2 files changed, 61 insertions(+), 19 deletions(-) diff --git a/include/android/choreographer.h b/include/android/choreographer.h index 63aa7ff6c0..cd8e63dec2 100644 --- a/include/android/choreographer.h +++ b/include/android/choreographer.h @@ -16,6 +16,28 @@ /** * @addtogroup Choreographer + * + * Choreographer coordinates the timing of frame rendering. This is the C version of the + * android.view.Choreographer object in Java. + * + * As of API level 33, apps can follow proper frame pacing and even choose a future frame to render. + * The API is used as follows: + * 1. The app posts an {@link AChoreographer_vsyncCallback} to Choreographer to run on the next + * frame. + * 2. The callback is called when it is the time to start the frame with an {@link + * AChoreographerFrameCallbackData} payload: information about multiple possible frame + * timelines. + * 3. Apps can choose a frame timeline from the {@link + * AChoreographerFrameCallbackData} payload, depending on the frame deadline they can meet when + * rendering the frame and their desired presentation time, and subsequently + * {@link ASurfaceTransaction_setFrameTimeline notify SurfaceFlinger} + * of the choice. Alternatively, for apps that do not choose a frame timeline, their frame would be + * presented at the earliest possible timeline. + * - The preferred frame timeline is the default frame + * timeline that the platform scheduled for the app, based on device configuration. + * 4. SurfaceFlinger attempts to follow the chosen frame timeline, by not applying transactions or + * latching buffers before the desired presentation time. + * * @{ */ @@ -47,7 +69,8 @@ typedef int64_t AVsyncId; struct AChoreographerFrameCallbackData; /** - * Opaque type that provides access to an AChoreographerFrameCallbackData object. + * Opaque type that provides access to an AChoreographerFrameCallbackData object, which contains + * various methods to extract frame information. */ typedef struct AChoreographerFrameCallbackData AChoreographerFrameCallbackData; @@ -73,8 +96,9 @@ typedef void (*AChoreographer_frameCallback64)(int64_t frameTimeNanos, void* dat /** * Prototype of the function that is called when a new frame is being rendered. - * It's passed the frame data that should not outlive the callback, as well as the data pointer - * provided by the application that registered a callback. + * It is called with \c callbackData describing multiple frame timelines, as well as the \c data + * pointer provided by the application that registered a callback. The \c callbackData does not + * outlive the callback. */ typedef void (*AChoreographer_vsyncCallback)( const AChoreographerFrameCallbackData* callbackData, void* data); @@ -110,7 +134,7 @@ void AChoreographer_postFrameCallbackDelayed(AChoreographer* choreographer, __DEPRECATED_IN(29); /** - * Power a callback to be run on the next frame. The data pointer provided will + * Post a callback to be run on the next frame. The data pointer provided will * be passed to the callback function when it's called. * * Available since API level 29. @@ -131,8 +155,10 @@ void AChoreographer_postFrameCallbackDelayed64(AChoreographer* choreographer, uint32_t delayMillis) __INTRODUCED_IN(29); /** - * Posts a callback to run on the next frame. The data pointer provided will + * Posts a callback to be run on the next frame. The data pointer provided will * be passed to the callback function when it's called. + * + * Available since API level 33. */ void AChoreographer_postVsyncCallback(AChoreographer* choreographer, AChoreographer_vsyncCallback callback, void* data) @@ -189,7 +215,10 @@ void AChoreographer_unregisterRefreshRateCallback(AChoreographer* choreographer, __INTRODUCED_IN(30); /** - * The time in nanoseconds when the frame started being rendered. + * The time in nanoseconds at which the frame started being rendered. + * + * Note that this time should \b not be used to advance animation clocks. + * Instead, see AChoreographerFrameCallbackData_getFrameTimelineExpectedPresentationTimeNanos(). */ int64_t AChoreographerFrameCallbackData_getFrameTimeNanos( const AChoreographerFrameCallbackData* data) __INTRODUCED_IN(33); @@ -201,25 +230,38 @@ size_t AChoreographerFrameCallbackData_getFrameTimelinesLength( const AChoreographerFrameCallbackData* data) __INTRODUCED_IN(33); /** - * Get index of the platform-preferred FrameTimeline. + * Gets the index of the platform-preferred frame timeline. + * The preferred frame timeline is the default + * by which the platform scheduled the app, based on the device configuration. */ size_t AChoreographerFrameCallbackData_getPreferredFrameTimelineIndex( const AChoreographerFrameCallbackData* data) __INTRODUCED_IN(33); /** - * The vsync ID token used to map Choreographer data. + * Gets the token used by the platform to identify the frame timeline at the given \c index. + * + * \param index index of a frame timeline, in \f( [0, FrameTimelinesLength) \f). See + * AChoreographerFrameCallbackData_getFrameTimelinesLength() */ AVsyncId AChoreographerFrameCallbackData_getFrameTimelineVsyncId( const AChoreographerFrameCallbackData* data, size_t index) __INTRODUCED_IN(33); /** - * The time in nanoseconds which the frame at given index is expected to be presented. + * Gets the time in nanoseconds at which the frame described at the given \c index is expected to + * be presented. This time should be used to advance any animation clocks. + * + * \param index index of a frame timeline, in \f( [0, FrameTimelinesLength) \f). See + * AChoreographerFrameCallbackData_getFrameTimelinesLength() */ int64_t AChoreographerFrameCallbackData_getFrameTimelineExpectedPresentationTimeNanos( const AChoreographerFrameCallbackData* data, size_t index) __INTRODUCED_IN(33); /** - * The time in nanoseconds which the frame at given index needs to be ready by. + * Gets the time in nanoseconds at which the frame described at the given \c index needs to be + * ready by in order to be presented on time. + * + * \param index index of a frame timeline, in \f( [0, FrameTimelinesLength) \f). See + * AChoreographerFrameCallbackData_getFrameTimelinesLength() */ int64_t AChoreographerFrameCallbackData_getFrameTimelineDeadlineNanos( const AChoreographerFrameCallbackData* data, size_t index) __INTRODUCED_IN(33); diff --git a/include/android/surface_control.h b/include/android/surface_control.h index 9a36ecb537..6223ef7f82 100644 --- a/include/android/surface_control.h +++ b/include/android/surface_control.h @@ -597,20 +597,20 @@ void ASurfaceTransaction_setEnableBackPressure(ASurfaceTransaction* transaction, __INTRODUCED_IN(31); /** - * Sets the frame timeline to use in Surface Flinger. + * Sets the frame timeline to use in SurfaceFlinger. * - * A frame timeline should be chosen based on what frame deadline the application - * can meet when rendering the frame and the application's desired present time. - * By setting a frame timeline, Surface Flinger tries to present the frame at the corresponding - * expected present time. + * A frame timeline should be chosen based on the frame deadline the application + * can meet when rendering the frame and the application's desired presentation time. + * By setting a frame timeline, SurfaceFlinger tries to present the frame at the corresponding + * expected presentation time. * * To receive frame timelines, a callback must be posted to Choreographer using - * AChoreographer_postExtendedFrameCallback(). The \a vsnycId can then be extracted from the + * AChoreographer_postVsyncCallback(). The \c vsyncId can then be extracted from the * callback payload using AChoreographerFrameCallbackData_getFrameTimelineVsyncId(). * - * \param vsyncId The vsync ID received from AChoreographer, setting the frame's present target to - * the corresponding expected present time and deadline from the frame to be rendered. A stale or - * invalid value will be ignored. + * \param vsyncId The vsync ID received from AChoreographer, setting the frame's presentation target + * to the corresponding expected presentation time and deadline from the frame to be rendered. A + * stale or invalid value will be ignored. */ void ASurfaceTransaction_setFrameTimeline(ASurfaceTransaction* transaction, AVsyncId vsyncId) __INTRODUCED_IN(33); -- cgit v1.2.3-59-g8ed1b From e74b35ffd574e4ddccb2112efcb9e337653cd9a6 Mon Sep 17 00:00:00 2001 From: lilinnan Date: Tue, 19 Jul 2022 16:00:50 +0800 Subject: Fix spot not disappear when display id changed Turn on show touches in settings, and finger press on screen, let the spot show, at now, display id changed, the spot will stay on screen. when display id changed, we should cancel touch event and update spot state. Bug: 239378650 Bug: 234662773 Test: atest inputflinger_tests Signed-off-by: lilinnan Change-Id: Ic289d772087e53716b4d63c431d9f41af721150e Merged-In: Ic289d772087e53716b4d63c431d9f41af721150e (cherry picked from commit 687e58faa44d7f5b4a5f21ca59442252716839ea) --- .../reader/mapper/TouchInputMapper.cpp | 14 ++++++++-- services/inputflinger/tests/InputReader_test.cpp | 32 ++++++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/services/inputflinger/reader/mapper/TouchInputMapper.cpp b/services/inputflinger/reader/mapper/TouchInputMapper.cpp index 637b1cb263..b5b2b45763 100644 --- a/services/inputflinger/reader/mapper/TouchInputMapper.cpp +++ b/services/inputflinger/reader/mapper/TouchInputMapper.cpp @@ -390,6 +390,10 @@ void TouchInputMapper::configure(nsecs_t when, const InputReaderConfiguration* c } if (changes && resetNeeded) { + // If device was reset, cancel touch event and update touch spot state. + cancelTouch(mCurrentRawState.when, mCurrentRawState.readTime); + mCurrentCookedState.clear(); + updateTouchSpots(); // Send reset, unless this is the first time the device has been configured, // in which case the reader will call reset itself after all mappers are ready. NotifyDeviceResetArgs args(getContext()->getNextId(), when, getDeviceId()); @@ -941,6 +945,7 @@ void TouchInputMapper::configureInputDevice(nsecs_t when, bool* outResetNeeded) bool skipViewportUpdate = false; if (viewportChanged) { const bool viewportOrientationChanged = mViewport.orientation != newViewport->orientation; + const bool viewportDisplayIdChanged = mViewport.displayId != newViewport->displayId; mViewport = *newViewport; if (mDeviceMode == DeviceMode::DIRECT || mDeviceMode == DeviceMode::POINTER) { @@ -1016,8 +1021,9 @@ void TouchInputMapper::configureInputDevice(nsecs_t when, bool* outResetNeeded) : getInverseRotation(mViewport.orientation); // For orientation-aware devices that work in the un-rotated coordinate space, the // viewport update should be skipped if it is only a change in the orientation. - skipViewportUpdate = mParameters.orientationAware && mDisplayWidth == oldDisplayWidth && - mDisplayHeight == oldDisplayHeight && viewportOrientationChanged; + skipViewportUpdate = !viewportDisplayIdChanged && mParameters.orientationAware && + mDisplayWidth == oldDisplayWidth && mDisplayHeight == oldDisplayHeight && + viewportOrientationChanged; // Apply the input device orientation for the device. mInputDeviceOrientation = @@ -1926,6 +1932,10 @@ void TouchInputMapper::dispatchVirtualKey(nsecs_t when, nsecs_t readTime, uint32 } void TouchInputMapper::abortTouches(nsecs_t when, nsecs_t readTime, uint32_t policyFlags) { + if (mCurrentMotionAborted) { + // Current motion event was already aborted. + return; + } BitSet32 currentIdBits = mCurrentCookedState.cookedPointerData.touchingIdBits; if (!currentIdBits.isEmpty()) { int32_t metaState = getContext()->getGlobalMetaState(); diff --git a/services/inputflinger/tests/InputReader_test.cpp b/services/inputflinger/tests/InputReader_test.cpp index c36704bf15..8065ad645c 100644 --- a/services/inputflinger/tests/InputReader_test.cpp +++ b/services/inputflinger/tests/InputReader_test.cpp @@ -6690,6 +6690,34 @@ TEST_F(SingleTouchInputMapperTest, Process_WhenAbsPressureIsPresent_HoversIfItsV toDisplayX(150), toDisplayY(250), 0, 0, 0, 0, 0, 0, 0, 0)); } +TEST_F(SingleTouchInputMapperTest, + Process_WhenViewportDisplayIdChanged_TouchIsCanceledAndDeviceIsReset) { + addConfigurationProperty("touch.deviceType", "touchScreen"); + prepareDisplay(DISPLAY_ORIENTATION_0); + prepareButtons(); + prepareAxes(POSITION); + SingleTouchInputMapper& mapper = addMapperAndConfigure(); + NotifyMotionArgs motionArgs; + + // Down. + processDown(mapper, 100, 200); + processSync(mapper); + + // We should receive a down event + ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyMotionWasCalled(&motionArgs)); + ASSERT_EQ(AMOTION_EVENT_ACTION_DOWN, motionArgs.action); + + // Change display id + clearViewports(); + prepareSecondaryDisplay(ViewportType::INTERNAL); + + // We should receive a cancel event + ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyMotionWasCalled(&motionArgs)); + ASSERT_EQ(AMOTION_EVENT_ACTION_CANCEL, motionArgs.action); + // Then receive reset called + ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled()); +} + // --- TouchDisplayProjectionTest --- class TouchDisplayProjectionTest : public SingleTouchInputMapperTest { @@ -8725,6 +8753,10 @@ TEST_F(MultiTouchInputMapperTest, VideoFrames_WhenNotOrientationAware_AreRotated // window's coordinate space. frames[0].rotate(getInverseRotation(orientation)); ASSERT_EQ(frames, motionArgs.videoFrames); + + // Release finger. + processSync(mapper); + ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyMotionWasCalled(&motionArgs)); } } -- cgit v1.2.3-59-g8ed1b From 366904197bc619c00e36403faf2b17ffc1b5c204 Mon Sep 17 00:00:00 2001 From: Prabir Pradhan Date: Fri, 5 Aug 2022 22:26:56 +0000 Subject: Fix issues with InputMapper tests - When an input device is added, a device reset notification is sent. This should be consumed when the device is added so that it does not need to be consumed in every test case. - The above change exposed an issue with a CursorInputMapper test case, where it was consuming the wrong device reset notification. - When a device is configured, it may produce device reset notifications. After configuring, we should loop the input reader so that the queued input listener is flushed so that these args can be sent out. Bug: 234662773 Test: atest inputflinger_tests Change-Id: Ic4979b91207a6abf4c4ac65fd3db30307cb53729 Merged-In: Ic4979b91207a6abf4c4ac65fd3db30307cb53729 (cherry picked from commit b5174de1a9ede697dfd2bf65e65b294321f9444d) --- services/inputflinger/tests/InputReader_test.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/services/inputflinger/tests/InputReader_test.cpp b/services/inputflinger/tests/InputReader_test.cpp index 8065ad645c..2c8afc7b89 100644 --- a/services/inputflinger/tests/InputReader_test.cpp +++ b/services/inputflinger/tests/InputReader_test.cpp @@ -2938,6 +2938,8 @@ protected: mReader = std::make_unique(mFakeEventHub, mFakePolicy, *mFakeListener); mDevice = newDevice(DEVICE_ID, DEVICE_NAME, DEVICE_LOCATION, EVENTHUB_ID, classes); + // Consume the device reset notification generated when adding a new device. + mFakeListener->assertNotifyDeviceResetWasCalled(); } void SetUp() override { @@ -2962,6 +2964,8 @@ protected: mReader->loopOnce(); } mDevice->configure(ARBITRARY_TIME, mFakePolicy->getReaderConfiguration(), changes); + // Loop the reader to flush the input listener queue. + mReader->loopOnce(); } std::shared_ptr newDevice(int32_t deviceId, const std::string& name, @@ -2985,6 +2989,8 @@ protected: configureDevice(0); mDevice->reset(ARBITRARY_TIME); mapper.reset(ARBITRARY_TIME); + // Loop the reader to flush the input listener queue. + mReader->loopOnce(); return mapper; } @@ -3010,6 +3016,7 @@ protected: event.code = code; event.value = value; mapper.process(&event); + // Loop the reader to flush the input listener queue. mReader->loopOnce(); } @@ -4907,7 +4914,6 @@ TEST_F(CursorInputMapperTest, Process_PointerCapture) { ASSERT_TRUE(mReader->getContext()->getGeneration() != generation); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled(&resetArgs)); - ASSERT_EQ(ARBITRARY_TIME, resetArgs.eventTime); ASSERT_EQ(DEVICE_ID, resetArgs.deviceId); process(mapper, ARBITRARY_TIME, READ_TIME, EV_REL, REL_X, 10); -- cgit v1.2.3-59-g8ed1b From f670dad1449dcce43494394789c55821a995d79a Mon Sep 17 00:00:00 2001 From: Prabir Pradhan Date: Fri, 5 Aug 2022 22:32:11 +0000 Subject: Reset the touch state when the active viewport is disabled When an active viewport becomes inactive, cancel any ongoing gestures immediately. When the viewport becomes active again, make sure state is reset again so that we eliminate any race conditions between the viewport becoming active and first touches coming in from the device. This is a preventative measure to defend against unexpected touch behavior around viewports being activated and deactivated. Bug: 234662773 Test: atest inputflinger_tests Change-Id: I111361f7470fdad39b493b516e8a8f167e0c681c Merged-In: I111361f7470fdad39b493b516e8a8f167e0c681c (cherry picked from commit c0bdeefdcb2f8d880389cf3aa0a0d8899f46f2e4) --- .../reader/mapper/TouchInputMapper.cpp | 42 +++++-------- services/inputflinger/tests/InputReader_test.cpp | 73 +++++++++++++++++++--- 2 files changed, 81 insertions(+), 34 deletions(-) diff --git a/services/inputflinger/reader/mapper/TouchInputMapper.cpp b/services/inputflinger/reader/mapper/TouchInputMapper.cpp index b5b2b45763..428fe10156 100644 --- a/services/inputflinger/reader/mapper/TouchInputMapper.cpp +++ b/services/inputflinger/reader/mapper/TouchInputMapper.cpp @@ -44,6 +44,8 @@ static constexpr nsecs_t STYLUS_DATA_LATENCY = ms2ns(10); // --- Static Definitions --- +static const DisplayViewport kUninitializedViewport; + template inline static void swap(T& a, T& b) { T temp = a; @@ -390,10 +392,10 @@ void TouchInputMapper::configure(nsecs_t when, const InputReaderConfiguration* c } if (changes && resetNeeded) { - // If device was reset, cancel touch event and update touch spot state. - cancelTouch(mCurrentRawState.when, mCurrentRawState.readTime); - mCurrentCookedState.clear(); - updateTouchSpots(); + // If the device needs to be reset, cancel any ongoing gestures and reset the state. + cancelTouch(when, when); + reset(when); + // Send reset, unless this is the first time the device has been configured, // in which case the reader will call reset itself after all mappers are ready. NotifyDeviceResetArgs args(getContext()->getNextId(), when, getDeviceId()); @@ -881,7 +883,7 @@ void TouchInputMapper::initializeOrientedRanges() { } void TouchInputMapper::configureInputDevice(nsecs_t when, bool* outResetNeeded) { - DeviceMode oldDeviceMode = mDeviceMode; + const DeviceMode oldDeviceMode = mDeviceMode; resolveExternalStylusPresence(); @@ -910,43 +912,37 @@ void TouchInputMapper::configureInputDevice(nsecs_t when, bool* outResetNeeded) mDeviceMode = DeviceMode::UNSCALED; } - // Ensure we have valid X and Y axes. + const std::optional newViewportOpt = findViewport(); + + // Ensure the device is valid and can be used. if (!mRawPointerAxes.x.valid || !mRawPointerAxes.y.valid) { ALOGW("Touch device '%s' did not report support for X or Y axis! " "The device will be inoperable.", getDeviceName().c_str()); mDeviceMode = DeviceMode::DISABLED; - return; - } - - // Get associated display dimensions. - std::optional newViewport = findViewport(); - if (!newViewport) { + } else if (!newViewportOpt) { ALOGI("Touch device '%s' could not query the properties of its associated " "display. The device will be inoperable until the display size " "becomes available.", getDeviceName().c_str()); mDeviceMode = DeviceMode::DISABLED; - return; - } - - if (!newViewport->isActive) { + } else if (!newViewportOpt->isActive) { ALOGI("Disabling %s (device %i) because the associated viewport is not active", getDeviceName().c_str(), getDeviceId()); mDeviceMode = DeviceMode::DISABLED; - return; } // Raw width and height in the natural orientation. const int32_t rawWidth = mRawPointerAxes.getRawWidth(); const int32_t rawHeight = mRawPointerAxes.getRawHeight(); - const bool viewportChanged = mViewport != *newViewport; + const DisplayViewport& newViewport = newViewportOpt.value_or(kUninitializedViewport); + const bool viewportChanged = mViewport != newViewport; bool skipViewportUpdate = false; if (viewportChanged) { - const bool viewportOrientationChanged = mViewport.orientation != newViewport->orientation; - const bool viewportDisplayIdChanged = mViewport.displayId != newViewport->displayId; - mViewport = *newViewport; + const bool viewportOrientationChanged = mViewport.orientation != newViewport.orientation; + const bool viewportDisplayIdChanged = mViewport.displayId != newViewport.displayId; + mViewport = newViewport; if (mDeviceMode == DeviceMode::DIRECT || mDeviceMode == DeviceMode::POINTER) { // Convert rotated viewport to the natural orientation. @@ -1100,10 +1096,6 @@ void TouchInputMapper::configureInputDevice(nsecs_t when, bool* outResetNeeded) // of the diagonal axis of the touch pad. Touches that are wider than this are // translated into freeform gestures. mPointerGestureMaxSwipeWidth = mConfig.pointerGestureSwipeMaxWidthRatio * rawDiagonal; - - // Abort current pointer usages because the state has changed. - const nsecs_t readTime = when; // synthetic event - abortPointerUsage(when, readTime, 0 /*policyFlags*/); } // Inform the dispatcher about the changes. diff --git a/services/inputflinger/tests/InputReader_test.cpp b/services/inputflinger/tests/InputReader_test.cpp index 2c8afc7b89..75b8dd1953 100644 --- a/services/inputflinger/tests/InputReader_test.cpp +++ b/services/inputflinger/tests/InputReader_test.cpp @@ -6724,6 +6724,61 @@ TEST_F(SingleTouchInputMapperTest, ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled()); } +TEST_F(SingleTouchInputMapperTest, + Process_WhenViewportActiveStatusChanged_TouchIsCanceledAndDeviceIsReset) { + addConfigurationProperty("touch.deviceType", "touchScreen"); + prepareDisplay(DISPLAY_ORIENTATION_0); + prepareButtons(); + prepareAxes(POSITION); + SingleTouchInputMapper& mapper = addMapperAndConfigure(); + ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled()); + NotifyMotionArgs motionArgs; + + // Start a new gesture. + processDown(mapper, 100, 200); + processSync(mapper); + ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyMotionWasCalled(&motionArgs)); + ASSERT_EQ(AMOTION_EVENT_ACTION_DOWN, motionArgs.action); + + // Make the viewport inactive. This will put the device in disabled mode. + auto viewport = mFakePolicy->getDisplayViewportByType(ViewportType::INTERNAL); + viewport->isActive = false; + mFakePolicy->updateViewport(*viewport); + configureDevice(InputReaderConfiguration::CHANGE_DISPLAY_INFO); + + // We should receive a cancel event for the ongoing gesture. + ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyMotionWasCalled(&motionArgs)); + ASSERT_EQ(AMOTION_EVENT_ACTION_CANCEL, motionArgs.action); + // Then we should be notified that the device was reset. + ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled()); + + // No events are generated while the viewport is inactive. + processMove(mapper, 101, 201); + processSync(mapper); + processDown(mapper, 102, 202); + processSync(mapper); + ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyMotionWasNotCalled()); + + // Make the viewport active again. The device should resume processing events. + viewport->isActive = true; + mFakePolicy->updateViewport(*viewport); + configureDevice(InputReaderConfiguration::CHANGE_DISPLAY_INFO); + + // The device is reset because it changes back to direct mode, without generating any events. + ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled()); + ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyMotionWasNotCalled()); + + // Start a new gesture. + processDown(mapper, 100, 200); + processSync(mapper); + ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyMotionWasCalled(&motionArgs)); + ASSERT_EQ(AMOTION_EVENT_ACTION_DOWN, motionArgs.action); + + // No more events. + ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyMotionWasNotCalled()); + ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasNotCalled()); +} + // --- TouchDisplayProjectionTest --- class TouchDisplayProjectionTest : public SingleTouchInputMapperTest { @@ -8581,27 +8636,27 @@ TEST_F(MultiTouchInputMapperTest, Process_DeactivateViewport_AbortTouches) { ASSERT_TRUE(mFakePolicy->updateViewport(displayViewport)); configureDevice(InputReaderConfiguration::CHANGE_DISPLAY_INFO); - // Finger move + // The ongoing touch should be canceled immediately + ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyMotionWasCalled(&motionArgs)); + EXPECT_EQ(AMOTION_EVENT_ACTION_CANCEL, motionArgs.action); + + // Finger move is ignored x += 10, y += 10; processPosition(mapper, x, y); processSync(mapper); - - ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyMotionWasCalled(&motionArgs)); - EXPECT_EQ(AMOTION_EVENT_ACTION_CANCEL, motionArgs.action); + ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyMotionWasNotCalled()); // Reactivate display viewport displayViewport.isActive = true; ASSERT_TRUE(mFakePolicy->updateViewport(displayViewport)); configureDevice(InputReaderConfiguration::CHANGE_DISPLAY_INFO); - // Finger move again + // Finger move again starts new gesture x += 10, y += 10; processPosition(mapper, x, y); processSync(mapper); - - // Gesture is aborted, so events after display is activated won't be dispatched until there is - // no pointer on the touch device. - mFakeListener->assertNotifyMotionWasNotCalled(); + ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyMotionWasCalled(&motionArgs)); + EXPECT_EQ(AMOTION_EVENT_ACTION_DOWN, motionArgs.action); } TEST_F(MultiTouchInputMapperTest, Process_Pointer_ShowTouches) { -- cgit v1.2.3-59-g8ed1b From e72ba5e27de8677448a1666c1fb122e5f8c1685f Mon Sep 17 00:00:00 2001 From: joenchen Date: Wed, 24 Aug 2022 13:08:58 +0000 Subject: CE: flush the staged brightness to HWC before power off After SurfaceFlinger receives the brightness commands, the commands are sent to HWC until the next frame presents. However, no following frames are present after the power mode is set to off, and HWC cannot receive the last brightness commands. This change forces SurfaceFlinger to flush the staged brightness to HWC prior to execution of power off. Bug: 239908529 Test: suspend and resume repeatedly Change-Id: Ia8fb04399e757de16e06e6516d86bdfcfabd5a2e --- .../include/compositionengine/Display.h | 3 +++ .../include/compositionengine/impl/Display.h | 1 + .../include/compositionengine/mock/Display.h | 1 + .../CompositionEngine/src/Display.cpp | 25 +++++++++++++--------- services/surfaceflinger/DisplayDevice.cpp | 9 ++++++++ 5 files changed, 29 insertions(+), 10 deletions(-) diff --git a/services/surfaceflinger/CompositionEngine/include/compositionengine/Display.h b/services/surfaceflinger/CompositionEngine/include/compositionengine/Display.h index 16cb41b024..5e84be1841 100644 --- a/services/surfaceflinger/CompositionEngine/include/compositionengine/Display.h +++ b/services/surfaceflinger/CompositionEngine/include/compositionengine/Display.h @@ -56,6 +56,9 @@ public: // similar requests if needed. virtual void createClientCompositionCache(uint32_t cacheSize) = 0; + // Sends the brightness setting to HWC + virtual void applyDisplayBrightness(const bool applyImmediately) = 0; + protected: ~Display() = default; }; diff --git a/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/Display.h b/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/Display.h index fa7bc5da7f..33a10a36a7 100644 --- a/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/Display.h +++ b/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/Display.h @@ -72,6 +72,7 @@ public: const compositionengine::DisplayColorProfileCreationArgs&) override; void createRenderSurface(const compositionengine::RenderSurfaceCreationArgs&) override; void createClientCompositionCache(uint32_t cacheSize) override; + void applyDisplayBrightness(const bool applyImmediately) override; // Internal helpers used by chooseCompositionStrategy() using ChangedTypes = android::HWComposer::DeviceRequestedChanges::ChangedTypes; diff --git a/services/surfaceflinger/CompositionEngine/include/compositionengine/mock/Display.h b/services/surfaceflinger/CompositionEngine/include/compositionengine/mock/Display.h index 72e6f3bdbb..7e99ec2f5a 100644 --- a/services/surfaceflinger/CompositionEngine/include/compositionengine/mock/Display.h +++ b/services/surfaceflinger/CompositionEngine/include/compositionengine/mock/Display.h @@ -41,6 +41,7 @@ public: MOCK_METHOD1(createDisplayColorProfile, void(const DisplayColorProfileCreationArgs&)); MOCK_METHOD1(createRenderSurface, void(const RenderSurfaceCreationArgs&)); MOCK_METHOD1(createClientCompositionCache, void(uint32_t)); + MOCK_METHOD1(applyDisplayBrightness, void(const bool)); MOCK_METHOD1(setPredictCompositionStrategy, void(bool)); }; diff --git a/services/surfaceflinger/CompositionEngine/src/Display.cpp b/services/surfaceflinger/CompositionEngine/src/Display.cpp index ea856e4859..1ec6449ed0 100644 --- a/services/surfaceflinger/CompositionEngine/src/Display.cpp +++ b/services/surfaceflinger/CompositionEngine/src/Display.cpp @@ -203,23 +203,16 @@ void Display::setReleasedLayers(const compositionengine::CompositionRefreshArgs& setReleasedLayers(std::move(releasedLayers)); } -void Display::beginFrame() { - Output::beginFrame(); - - // If we don't have a HWC display, then we are done. - const auto halDisplayId = HalDisplayId::tryCast(mId); - if (!halDisplayId) { - return; - } - +void Display::applyDisplayBrightness(const bool applyImmediately) { auto& hwc = getCompositionEngine().getHwComposer(); + const auto halDisplayId = HalDisplayId::tryCast(*getDisplayId()); if (const auto physicalDisplayId = PhysicalDisplayId::tryCast(*halDisplayId); physicalDisplayId && getState().displayBrightness) { const status_t result = hwc.setDisplayBrightness(*physicalDisplayId, *getState().displayBrightness, getState().displayBrightnessNits, Hwc2::Composer::DisplayBrightnessOptions{ - .applyImmediately = false}) + .applyImmediately = applyImmediately}) .get(); ALOGE_IF(result != NO_ERROR, "setDisplayBrightness failed for %s: %d, (%s)", getName().c_str(), result, strerror(-result)); @@ -228,6 +221,18 @@ void Display::beginFrame() { editState().displayBrightness.reset(); } +void Display::beginFrame() { + Output::beginFrame(); + + // If we don't have a HWC display, then we are done. + const auto halDisplayId = HalDisplayId::tryCast(mId); + if (!halDisplayId) { + return; + } + + applyDisplayBrightness(false); +} + bool Display::chooseCompositionStrategy( std::optional* outChanges) { ATRACE_CALL(); diff --git a/services/surfaceflinger/DisplayDevice.cpp b/services/surfaceflinger/DisplayDevice.cpp index 86809007b4..3bc3ae54b3 100644 --- a/services/surfaceflinger/DisplayDevice.cpp +++ b/services/surfaceflinger/DisplayDevice.cpp @@ -173,6 +173,15 @@ auto DisplayDevice::getInputInfo() const -> InputInfo { } void DisplayDevice::setPowerMode(hal::PowerMode mode) { + if (mode == hal::PowerMode::OFF || mode == hal::PowerMode::ON) { + if (mStagedBrightness && mBrightness != *mStagedBrightness) { + getCompositionDisplay()->setNextBrightness(*mStagedBrightness); + mBrightness = *mStagedBrightness; + } + mStagedBrightness = std::nullopt; + getCompositionDisplay()->applyDisplayBrightness(true); + } + mPowerMode = mode; getCompositionDisplay()->setCompositionEnabled(mPowerMode != hal::PowerMode::OFF); } -- cgit v1.2.3-59-g8ed1b From 4bce2e19ea8893805eae836b85730ffd302d9f3b Mon Sep 17 00:00:00 2001 From: Rachel Lee Date: Thu, 8 Sep 2022 19:51:32 -0700 Subject: Fix getLatestVsyncEventData deadline. Bug: 239775097 Test: atest libsurfaceflinger_unittest Test: pixel 4xl camera preview test (see bug) Change-Id: Ib92849291458f59c2ef2238d3586211b87174c7f (cherry picked from commit 0679cf200ea30c2e791cd79b27a26676e8848932) Merged-In: Ib92849291458f59c2ef2238d3586211b87174c7f --- services/surfaceflinger/Scheduler/DispSyncSource.cpp | 6 +++--- .../surfaceflinger/tests/unittests/DispSyncSourceTest.cpp | 11 +++++------ 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/services/surfaceflinger/Scheduler/DispSyncSource.cpp b/services/surfaceflinger/Scheduler/DispSyncSource.cpp index 747032be0d..4af1f5c65f 100644 --- a/services/surfaceflinger/Scheduler/DispSyncSource.cpp +++ b/services/surfaceflinger/Scheduler/DispSyncSource.cpp @@ -188,10 +188,10 @@ void DispSyncSource::onVsyncCallback(nsecs_t vsyncTime, nsecs_t targetWakeupTime VSyncSource::VSyncData DispSyncSource::getLatestVSyncData() const { std::lock_guard lock(mVsyncMutex); - nsecs_t expectedPresentTime = mVSyncTracker.nextAnticipatedVSyncTimeFrom( + nsecs_t expectedPresentationTime = mVSyncTracker.nextAnticipatedVSyncTimeFrom( systemTime() + mWorkDuration.get().count() + mReadyDuration.count()); - nsecs_t deadline = expectedPresentTime - mWorkDuration.get().count() - mReadyDuration.count(); - return {expectedPresentTime, deadline}; + nsecs_t deadline = expectedPresentationTime - mReadyDuration.count(); + return {expectedPresentationTime, deadline}; } void DispSyncSource::dump(std::string& result) const { diff --git a/services/surfaceflinger/tests/unittests/DispSyncSourceTest.cpp b/services/surfaceflinger/tests/unittests/DispSyncSourceTest.cpp index ec27edac6e..67ace1af83 100644 --- a/services/surfaceflinger/tests/unittests/DispSyncSourceTest.cpp +++ b/services/surfaceflinger/tests/unittests/DispSyncSourceTest.cpp @@ -292,9 +292,10 @@ TEST_F(DispSyncSourceTest, waitForCallbacksWithDurationChange) { TEST_F(DispSyncSourceTest, getLatestVsyncData) { const nsecs_t now = systemTime(); - const nsecs_t vsyncInternalDuration = mWorkDuration.count() + mReadyDuration.count(); + const nsecs_t expectedPresentationTime = + now + mWorkDuration.count() + mReadyDuration.count() + 1; EXPECT_CALL(*mVSyncTracker, nextAnticipatedVSyncTimeFrom(_)) - .WillOnce(Return(now + vsyncInternalDuration + 1)); + .WillOnce(Return(expectedPresentationTime)); { InSequence seq; EXPECT_CALL(*mVSyncDispatch, registerCallback(_, mName)).Times(1); @@ -306,10 +307,8 @@ TEST_F(DispSyncSourceTest, getLatestVsyncData) { EXPECT_TRUE(mDispSyncSource); const auto vsyncData = mDispSyncSource->getLatestVSyncData(); - ASSERT_GT(vsyncData.deadlineTimestamp, now); - ASSERT_GT(vsyncData.expectedPresentationTime, vsyncData.deadlineTimestamp); - EXPECT_EQ(vsyncData.deadlineTimestamp, - vsyncData.expectedPresentationTime - vsyncInternalDuration); + ASSERT_EQ(vsyncData.expectedPresentationTime, expectedPresentationTime); + EXPECT_EQ(vsyncData.deadlineTimestamp, expectedPresentationTime - mReadyDuration.count()); } } // namespace -- cgit v1.2.3-59-g8ed1b From c04d04de31c346c1ccfa9f94fed715c07ac42986 Mon Sep 17 00:00:00 2001 From: Prabir Pradhan Date: Thu, 8 Sep 2022 22:03:30 +0000 Subject: Resolve associated display and pointer display in CursorInputMapper An InputDevice can be associated with a display, in which case it should only generate events for that display. A mouse generally generates events for whichever display is currently showing the mouse cursor. In the case where a mouse InputDevice is associated with a display, we need to make sure that it only generates events when the mouse cursor is on the associated display. Additionally, any display-dependent configuration, such as orientation, should depend on whichever display the device is configured for. Bug: 236075874 Test: atest inputflinger_tests Change-Id: I1021c121c1eae768585124d312f5187be90da666 Merged-In: I1021c121c1eae768585124d312f5187be90da666 (cherry picked from commit c13ff081db31e93ee3b0a96bb007704c61d10a02) --- include/input/PrintTools.h | 10 ++ .../reader/mapper/CursorInputMapper.cpp | 60 ++++++----- .../inputflinger/reader/mapper/CursorInputMapper.h | 4 + services/inputflinger/tests/InputReader_test.cpp | 115 +++++++++++++++++---- 4 files changed, 142 insertions(+), 47 deletions(-) diff --git a/include/input/PrintTools.h b/include/input/PrintTools.h index 0a75278494..55f730b287 100644 --- a/include/input/PrintTools.h +++ b/include/input/PrintTools.h @@ -17,6 +17,7 @@ #pragma once #include +#include #include #include @@ -27,6 +28,15 @@ std::string constToString(const T& v) { return std::to_string(v); } +/** + * Convert an optional type to string. + */ +template +std::string toString(const std::optional& optional, + std::string (*toString)(const T&) = constToString) { + return optional ? toString(*optional) : ""; +} + /** * Convert a set of integral types to string. */ diff --git a/services/inputflinger/reader/mapper/CursorInputMapper.cpp b/services/inputflinger/reader/mapper/CursorInputMapper.cpp index 91dc61923b..8233682a32 100644 --- a/services/inputflinger/reader/mapper/CursorInputMapper.cpp +++ b/services/inputflinger/reader/mapper/CursorInputMapper.cpp @@ -25,6 +25,8 @@ #include "PointerControllerInterface.h" #include "TouchCursorInputMapperCommon.h" +#include "input/PrintTools.h" + namespace android { // The default velocity control parameters that has no effect. @@ -113,6 +115,7 @@ void CursorInputMapper::dump(std::string& dump) { toString(mCursorScrollAccumulator.haveRelativeHWheel())); dump += StringPrintf(INDENT3 "VWheelScale: %0.3f\n", mVWheelScale); dump += StringPrintf(INDENT3 "HWheelScale: %0.3f\n", mHWheelScale); + dump += StringPrintf(INDENT3 "DisplayId: %s\n", toString(mDisplayId).c_str()); dump += StringPrintf(INDENT3 "Orientation: %d\n", mOrientation); dump += StringPrintf(INDENT3 "ButtonState: 0x%08x\n", mButtonState); dump += StringPrintf(INDENT3 "Down: %s\n", toString(isPointerDown(mButtonState))); @@ -201,21 +204,34 @@ void CursorInputMapper::configure(nsecs_t when, const InputReaderConfiguration* if (!changes || (changes & InputReaderConfiguration::CHANGE_DISPLAY_INFO) || configurePointerCapture) { + const bool isPointer = mParameters.mode == Parameters::Mode::POINTER; + + mDisplayId = ADISPLAY_ID_NONE; + if (auto viewport = mDeviceContext.getAssociatedViewport(); viewport) { + // This InputDevice is associated with a viewport. + // Only generate events for the associated display. + const bool mismatchedPointerDisplay = + isPointer && (viewport->displayId != mPointerController->getDisplayId()); + mDisplayId = mismatchedPointerDisplay ? std::nullopt + : std::make_optional(viewport->displayId); + } else if (isPointer) { + // The InputDevice is not associated with a viewport, but it controls the mouse pointer. + mDisplayId = mPointerController->getDisplayId(); + } + mOrientation = DISPLAY_ORIENTATION_0; const bool isOrientedDevice = (mParameters.orientationAware && mParameters.hasAssociatedDisplay); - // InputReader works in the un-rotated display coordinate space, so we don't need to do // anything if the device is already orientation-aware. If the device is not // orientation-aware, then we need to apply the inverse rotation of the display so that // when the display rotation is applied later as a part of the per-window transform, we // get the expected screen coordinates. When pointer capture is enabled, we do not apply any // rotations and report values directly from the input device. - if (!isOrientedDevice && mParameters.mode != Parameters::Mode::POINTER_RELATIVE) { - std::optional internalViewport = - config->getDisplayViewportByType(ViewportType::INTERNAL); - if (internalViewport) { - mOrientation = getInverseRotation(internalViewport->orientation); + if (!isOrientedDevice && mDisplayId && + mParameters.mode != Parameters::Mode::POINTER_RELATIVE) { + if (auto viewport = config->getDisplayViewportById(*mDisplayId); viewport) { + mOrientation = getInverseRotation(viewport->orientation); } } @@ -279,6 +295,11 @@ void CursorInputMapper::process(const RawEvent* rawEvent) { } void CursorInputMapper::sync(nsecs_t when, nsecs_t readTime) { + if (!mDisplayId) { + // Ignore events when there is no target display configured. + return; + } + int32_t lastButtonState = mButtonState; int32_t currentButtonState = mCursorButtonAccumulator.getButtonState(); mButtonState = currentButtonState; @@ -324,7 +345,6 @@ void CursorInputMapper::sync(nsecs_t when, nsecs_t readTime) { mPointerVelocityControl.move(when, &deltaX, &deltaY); - int32_t displayId = ADISPLAY_ID_NONE; float xCursorPosition = AMOTION_EVENT_INVALID_CURSOR_POSITION; float yCursorPosition = AMOTION_EVENT_INVALID_CURSOR_POSITION; if (mSource == AINPUT_SOURCE_MOUSE) { @@ -348,7 +368,6 @@ void CursorInputMapper::sync(nsecs_t when, nsecs_t readTime) { pointerCoords.setAxisValue(AMOTION_EVENT_AXIS_Y, yCursorPosition); pointerCoords.setAxisValue(AMOTION_EVENT_AXIS_RELATIVE_X, deltaX); pointerCoords.setAxisValue(AMOTION_EVENT_AXIS_RELATIVE_Y, deltaY); - displayId = mPointerController->getDisplayId(); } else { // Pointer capture and navigation modes pointerCoords.setAxisValue(AMOTION_EVENT_AXIS_X, deltaX); @@ -370,7 +389,7 @@ void CursorInputMapper::sync(nsecs_t when, nsecs_t readTime) { // Synthesize key down from buttons if needed. synthesizeButtonKeys(getContext(), AKEY_EVENT_ACTION_DOWN, when, readTime, getDeviceId(), - mSource, displayId, policyFlags, lastButtonState, currentButtonState); + mSource, *mDisplayId, policyFlags, lastButtonState, currentButtonState); // Send motion event. if (downChanged || moved || scrolled || buttonsChanged) { @@ -391,7 +410,7 @@ void CursorInputMapper::sync(nsecs_t when, nsecs_t readTime) { int32_t actionButton = BitSet32::valueForBit(released.clearFirstMarkedBit()); buttonState &= ~actionButton; NotifyMotionArgs releaseArgs(getContext()->getNextId(), when, readTime, - getDeviceId(), mSource, displayId, policyFlags, + getDeviceId(), mSource, *mDisplayId, policyFlags, AMOTION_EVENT_ACTION_BUTTON_RELEASE, actionButton, 0, metaState, buttonState, MotionClassification::NONE, AMOTION_EVENT_EDGE_FLAG_NONE, 1, &pointerProperties, @@ -403,7 +422,7 @@ void CursorInputMapper::sync(nsecs_t when, nsecs_t readTime) { } NotifyMotionArgs args(getContext()->getNextId(), when, readTime, getDeviceId(), mSource, - displayId, policyFlags, motionEventAction, 0, 0, metaState, + *mDisplayId, policyFlags, motionEventAction, 0, 0, metaState, currentButtonState, MotionClassification::NONE, AMOTION_EVENT_EDGE_FLAG_NONE, 1, &pointerProperties, &pointerCoords, mXPrecision, mYPrecision, xCursorPosition, yCursorPosition, downTime, @@ -416,7 +435,7 @@ void CursorInputMapper::sync(nsecs_t when, nsecs_t readTime) { int32_t actionButton = BitSet32::valueForBit(pressed.clearFirstMarkedBit()); buttonState |= actionButton; NotifyMotionArgs pressArgs(getContext()->getNextId(), when, readTime, getDeviceId(), - mSource, displayId, policyFlags, + mSource, *mDisplayId, policyFlags, AMOTION_EVENT_ACTION_BUTTON_PRESS, actionButton, 0, metaState, buttonState, MotionClassification::NONE, AMOTION_EVENT_EDGE_FLAG_NONE, 1, &pointerProperties, @@ -432,7 +451,7 @@ void CursorInputMapper::sync(nsecs_t when, nsecs_t readTime) { // Send hover move after UP to tell the application that the mouse is hovering now. if (motionEventAction == AMOTION_EVENT_ACTION_UP && (mSource == AINPUT_SOURCE_MOUSE)) { NotifyMotionArgs hoverArgs(getContext()->getNextId(), when, readTime, getDeviceId(), - mSource, displayId, policyFlags, + mSource, *mDisplayId, policyFlags, AMOTION_EVENT_ACTION_HOVER_MOVE, 0, 0, metaState, currentButtonState, MotionClassification::NONE, AMOTION_EVENT_EDGE_FLAG_NONE, 1, &pointerProperties, @@ -447,7 +466,7 @@ void CursorInputMapper::sync(nsecs_t when, nsecs_t readTime) { pointerCoords.setAxisValue(AMOTION_EVENT_AXIS_HSCROLL, hscroll); NotifyMotionArgs scrollArgs(getContext()->getNextId(), when, readTime, getDeviceId(), - mSource, displayId, policyFlags, + mSource, *mDisplayId, policyFlags, AMOTION_EVENT_ACTION_SCROLL, 0, 0, metaState, currentButtonState, MotionClassification::NONE, AMOTION_EVENT_EDGE_FLAG_NONE, 1, &pointerProperties, @@ -459,7 +478,7 @@ void CursorInputMapper::sync(nsecs_t when, nsecs_t readTime) { // Synthesize key up from buttons if needed. synthesizeButtonKeys(getContext(), AKEY_EVENT_ACTION_UP, when, readTime, getDeviceId(), mSource, - displayId, policyFlags, lastButtonState, currentButtonState); + *mDisplayId, policyFlags, lastButtonState, currentButtonState); mCursorMotionAccumulator.finishSync(); mCursorScrollAccumulator.finishSync(); @@ -474,16 +493,7 @@ int32_t CursorInputMapper::getScanCodeState(uint32_t sourceMask, int32_t scanCod } std::optional CursorInputMapper::getAssociatedDisplayId() { - if (mParameters.hasAssociatedDisplay) { - if (mParameters.mode == Parameters::Mode::POINTER) { - return std::make_optional(mPointerController->getDisplayId()); - } else { - // If the device is orientationAware and not a mouse, - // it expects to dispatch events to any display - return std::make_optional(ADISPLAY_ID_NONE); - } - } - return std::nullopt; + return mDisplayId; } } // namespace android diff --git a/services/inputflinger/reader/mapper/CursorInputMapper.h b/services/inputflinger/reader/mapper/CursorInputMapper.h index 75aeffb846..60b3dd9ee0 100644 --- a/services/inputflinger/reader/mapper/CursorInputMapper.h +++ b/services/inputflinger/reader/mapper/CursorInputMapper.h @@ -111,6 +111,10 @@ private: VelocityControl mWheelXVelocityControl; VelocityControl mWheelYVelocityControl; + // The display that events generated by this mapper should target. This can be set to + // ADISPLAY_ID_NONE to target the focused display. If there is no display target (i.e. + // std::nullopt), all events will be ignored. + std::optional mDisplayId; int32_t mOrientation; std::shared_ptr mPointerController; diff --git a/services/inputflinger/tests/InputReader_test.cpp b/services/inputflinger/tests/InputReader_test.cpp index 75b8dd1953..e1befedcff 100644 --- a/services/inputflinger/tests/InputReader_test.cpp +++ b/services/inputflinger/tests/InputReader_test.cpp @@ -56,7 +56,9 @@ static constexpr nsecs_t READ_TIME = 4321; // Arbitrary display properties. static constexpr int32_t DISPLAY_ID = 0; +static const std::string DISPLAY_UNIQUE_ID = "local:1"; static constexpr int32_t SECONDARY_DISPLAY_ID = DISPLAY_ID + 1; +static const std::string SECONDARY_DISPLAY_UNIQUE_ID = "local:2"; static constexpr int32_t DISPLAY_WIDTH = 480; static constexpr int32_t DISPLAY_HEIGHT = 800; static constexpr int32_t VIRTUAL_DISPLAY_ID = 1; @@ -91,6 +93,24 @@ static constexpr int32_t ACTION_POINTER_1_UP = // Error tolerance for floating point assertions. static const float EPSILON = 0.001f; +using ::testing::AllOf; + +MATCHER_P(WithAction, action, "InputEvent with specified action") { + return arg.action == action; +} + +MATCHER_P(WithSource, source, "InputEvent with specified source") { + return arg.source == source; +} + +MATCHER_P(WithDisplayId, displayId, "InputEvent with specified displayId") { + return arg.displayId == displayId; +} + +MATCHER_P2(WithCoords, x, y, "MotionEvent with specified action") { + return arg.pointerCoords[0].getX() == x && arg.pointerCoords[0].getY(); +} + template static inline T min(T a, T b) { return a < b ? a : b; @@ -2872,7 +2892,6 @@ TEST_F(InputDeviceTest, Configure_AssignsDisplayUniqueId) { // Device should be disabled because it is associated with a specific display, but the // corresponding display is not found. - const std::string DISPLAY_UNIQUE_ID = "displayUniqueId"; mFakePolicy->addInputUniqueIdAssociation(DEVICE_LOCATION, DISPLAY_UNIQUE_ID); mDevice->configure(ARBITRARY_TIME, mFakePolicy->getReaderConfiguration(), InputReaderConfiguration::CHANGE_DISPLAY_INFO); @@ -2903,7 +2922,6 @@ TEST_F(InputDeviceTest, Configure_UniqueId_CorrectlyMatches) { mDevice->addMapper(EVENTHUB_ID, AINPUT_SOURCE_KEYBOARD); mDevice->configure(ARBITRARY_TIME, mFakePolicy->getReaderConfiguration(), 0); - const std::string DISPLAY_UNIQUE_ID = "displayUniqueId"; mFakePolicy->addInputUniqueIdAssociation(DEVICE_LOCATION, DISPLAY_UNIQUE_ID); mFakePolicy->addDisplayViewport(SECONDARY_DISPLAY_ID, DISPLAY_WIDTH, DISPLAY_HEIGHT, DISPLAY_ORIENTATION_0, /* isActive= */ true, DISPLAY_UNIQUE_ID, @@ -4188,10 +4206,14 @@ protected: int32_t rotatedX, int32_t rotatedY); void prepareDisplay(int32_t orientation) { - const std::string uniqueId = "local:0"; - const ViewportType viewportType = ViewportType::INTERNAL; - setDisplayInfoAndReconfigure(DISPLAY_ID, DISPLAY_WIDTH, DISPLAY_HEIGHT, - orientation, uniqueId, NO_PORT, viewportType); + setDisplayInfoAndReconfigure(DISPLAY_ID, DISPLAY_WIDTH, DISPLAY_HEIGHT, orientation, + DISPLAY_UNIQUE_ID, NO_PORT, ViewportType::INTERNAL); + } + + void prepareSecondaryDisplay() { + setDisplayInfoAndReconfigure(SECONDARY_DISPLAY_ID, DISPLAY_WIDTH, DISPLAY_HEIGHT, + DISPLAY_ORIENTATION_0, SECONDARY_DISPLAY_UNIQUE_ID, NO_PORT, + ViewportType::EXTERNAL); } static void assertCursorPointerCoords(const PointerCoords& coords, float x, float y, @@ -4468,6 +4490,7 @@ TEST_F(CursorInputMapperTest, Process_ShouldHandleCombinedXYAndButtonUpdates) { } TEST_F(CursorInputMapperTest, Process_WhenOrientationAware_ShouldNotRotateMotions) { + mFakePolicy->addInputUniqueIdAssociation(DEVICE_LOCATION, DISPLAY_UNIQUE_ID); addConfigurationProperty("cursor.mode", "navigation"); // InputReader works in the un-rotated coordinate space, so orientation-aware devices do not // need to be rotated. @@ -4486,11 +4509,13 @@ TEST_F(CursorInputMapperTest, Process_WhenOrientationAware_ShouldNotRotateMotion } TEST_F(CursorInputMapperTest, Process_WhenNotOrientationAware_ShouldRotateMotions) { + mFakePolicy->addInputUniqueIdAssociation(DEVICE_LOCATION, DISPLAY_UNIQUE_ID); addConfigurationProperty("cursor.mode", "navigation"); // Since InputReader works in the un-rotated coordinate space, only devices that are not // orientation-aware are affected by display rotation. CursorInputMapper& mapper = addMapperAndConfigure(); + clearViewports(); prepareDisplay(DISPLAY_ORIENTATION_0); ASSERT_NO_FATAL_FAILURE(testMotionRotation(mapper, 0, 1, 0, 1)); ASSERT_NO_FATAL_FAILURE(testMotionRotation(mapper, 1, 1, 1, 1)); @@ -4501,6 +4526,7 @@ TEST_F(CursorInputMapperTest, Process_WhenNotOrientationAware_ShouldRotateMotion ASSERT_NO_FATAL_FAILURE(testMotionRotation(mapper, -1, 0, -1, 0)); ASSERT_NO_FATAL_FAILURE(testMotionRotation(mapper, -1, 1, -1, 1)); + clearViewports(); prepareDisplay(DISPLAY_ORIENTATION_90); ASSERT_NO_FATAL_FAILURE(testMotionRotation(mapper, 0, 1, -1, 0)); ASSERT_NO_FATAL_FAILURE(testMotionRotation(mapper, 1, 1, -1, 1)); @@ -4511,6 +4537,7 @@ TEST_F(CursorInputMapperTest, Process_WhenNotOrientationAware_ShouldRotateMotion ASSERT_NO_FATAL_FAILURE(testMotionRotation(mapper, -1, 0, 0, -1)); ASSERT_NO_FATAL_FAILURE(testMotionRotation(mapper, -1, 1, -1, -1)); + clearViewports(); prepareDisplay(DISPLAY_ORIENTATION_180); ASSERT_NO_FATAL_FAILURE(testMotionRotation(mapper, 0, 1, 0, -1)); ASSERT_NO_FATAL_FAILURE(testMotionRotation(mapper, 1, 1, -1, -1)); @@ -4521,6 +4548,7 @@ TEST_F(CursorInputMapperTest, Process_WhenNotOrientationAware_ShouldRotateMotion ASSERT_NO_FATAL_FAILURE(testMotionRotation(mapper, -1, 0, 1, 0)); ASSERT_NO_FATAL_FAILURE(testMotionRotation(mapper, -1, 1, 1, -1)); + clearViewports(); prepareDisplay(DISPLAY_ORIENTATION_270); ASSERT_NO_FATAL_FAILURE(testMotionRotation(mapper, 0, 1, 1, 0)); ASSERT_NO_FATAL_FAILURE(testMotionRotation(mapper, 1, 1, 1, -1)); @@ -5017,33 +5045,76 @@ TEST_F(CursorInputMapperTest, PointerCaptureDisablesOrientationChanges) { ASSERT_EQ(20, args.pointerCoords[0].getY()); } -TEST_F(CursorInputMapperTest, Process_ShouldHandleDisplayId) { +TEST_F(CursorInputMapperTest, ConfigureDisplayId_NoAssociatedViewport) { CursorInputMapper& mapper = addMapperAndConfigure(); - // Setup for second display. - constexpr int32_t SECOND_DISPLAY_ID = 1; - const std::string SECOND_DISPLAY_UNIQUE_ID = "local:1"; - mFakePolicy->addDisplayViewport(SECOND_DISPLAY_ID, 800, 480, DISPLAY_ORIENTATION_0, - true /*isActive*/, SECOND_DISPLAY_UNIQUE_ID, NO_PORT, - ViewportType::EXTERNAL); - mFakePolicy->setDefaultPointerDisplayId(SECOND_DISPLAY_ID); + // Set up the default display. + prepareDisplay(DISPLAY_ORIENTATION_90); + + // Set up the secondary display as the display on which the pointer should be shown. + // The InputDevice is not associated with any display. + prepareSecondaryDisplay(); + mFakePolicy->setDefaultPointerDisplayId(SECONDARY_DISPLAY_ID); configureDevice(InputReaderConfiguration::CHANGE_DISPLAY_INFO); - mFakePointerController->setBounds(0, 0, 800 - 1, 480 - 1); + mFakePointerController->setBounds(0, 0, DISPLAY_WIDTH - 1, DISPLAY_HEIGHT - 1); mFakePointerController->setPosition(100, 200); mFakePointerController->setButtonState(0); - NotifyMotionArgs args; + // Ensure input events are generated for the secondary display. process(mapper, ARBITRARY_TIME, READ_TIME, EV_REL, REL_X, 10); process(mapper, ARBITRARY_TIME, READ_TIME, EV_REL, REL_Y, 20); process(mapper, ARBITRARY_TIME, READ_TIME, EV_SYN, SYN_REPORT, 0); - ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyMotionWasCalled(&args)); - ASSERT_EQ(AINPUT_SOURCE_MOUSE, args.source); - ASSERT_EQ(AMOTION_EVENT_ACTION_HOVER_MOVE, args.action); - ASSERT_NO_FATAL_FAILURE(assertPointerCoords(args.pointerCoords[0], - 110.0f, 220.0f, 0.0f, 0.0f, 0.0f, 0.0f, 0.0f, 0.0f, 0.0f, 0.0f)); + ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyMotionWasCalled( + AllOf(WithAction(AMOTION_EVENT_ACTION_HOVER_MOVE), WithSource(AINPUT_SOURCE_MOUSE), + WithDisplayId(SECONDARY_DISPLAY_ID), WithCoords(110.0f, 220.0f)))); ASSERT_NO_FATAL_FAILURE(assertPosition(*mFakePointerController, 110.0f, 220.0f)); - ASSERT_EQ(SECOND_DISPLAY_ID, args.displayId); +} + +TEST_F(CursorInputMapperTest, ConfigureDisplayId_WithAssociatedViewport) { + CursorInputMapper& mapper = addMapperAndConfigure(); + + // Set up the default display. + prepareDisplay(DISPLAY_ORIENTATION_90); + + // Set up the secondary display as the display on which the pointer should be shown, + // and associate the InputDevice with the secondary display. + prepareSecondaryDisplay(); + mFakePolicy->setDefaultPointerDisplayId(SECONDARY_DISPLAY_ID); + mFakePolicy->addInputUniqueIdAssociation(DEVICE_LOCATION, SECONDARY_DISPLAY_UNIQUE_ID); + configureDevice(InputReaderConfiguration::CHANGE_DISPLAY_INFO); + + mFakePointerController->setBounds(0, 0, DISPLAY_WIDTH - 1, DISPLAY_HEIGHT - 1); + mFakePointerController->setPosition(100, 200); + mFakePointerController->setButtonState(0); + + process(mapper, ARBITRARY_TIME, READ_TIME, EV_REL, REL_X, 10); + process(mapper, ARBITRARY_TIME, READ_TIME, EV_REL, REL_Y, 20); + process(mapper, ARBITRARY_TIME, READ_TIME, EV_SYN, SYN_REPORT, 0); + ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyMotionWasCalled( + AllOf(WithAction(AMOTION_EVENT_ACTION_HOVER_MOVE), WithSource(AINPUT_SOURCE_MOUSE), + WithDisplayId(SECONDARY_DISPLAY_ID), WithCoords(110.0f, 220.0f)))); + ASSERT_NO_FATAL_FAILURE(assertPosition(*mFakePointerController, 110.0f, 220.0f)); +} + +TEST_F(CursorInputMapperTest, ConfigureDisplayId_IgnoresEventsForMismatchedPointerDisplay) { + CursorInputMapper& mapper = addMapperAndConfigure(); + + // Set up the default display as the display on which the pointer should be shown. + prepareDisplay(DISPLAY_ORIENTATION_90); + mFakePolicy->setDefaultPointerDisplayId(DISPLAY_ID); + + // Associate the InputDevice with the secondary display. + prepareSecondaryDisplay(); + mFakePolicy->addInputUniqueIdAssociation(DEVICE_LOCATION, SECONDARY_DISPLAY_UNIQUE_ID); + configureDevice(InputReaderConfiguration::CHANGE_DISPLAY_INFO); + + // The mapper should not generate any events because it is associated with a display that is + // different from the pointer display. + process(mapper, ARBITRARY_TIME, READ_TIME, EV_REL, REL_X, 10); + process(mapper, ARBITRARY_TIME, READ_TIME, EV_REL, REL_Y, 20); + process(mapper, ARBITRARY_TIME, READ_TIME, EV_SYN, SYN_REPORT, 0); + ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyMotionWasNotCalled()); } // --- TouchInputMapperTest --- -- cgit v1.2.3-59-g8ed1b From 076dc75aaea9e68838eabb90722b88954bb634d5 Mon Sep 17 00:00:00 2001 From: Siarhei Vishniakou Date: Mon, 12 Sep 2022 15:07:16 +0000 Subject: Enable touch occlusion logs These logs were made optional in ag/17073502, but that was not intentional. These are useful for debugging obstructed touch issues, so let's bring them back. Bug: 246404700 Test: adb logcat Change-Id: Ieca99d790d2a1c9680de9a89e0113e177488dc7b --- services/inputflinger/dispatcher/DebugConfig.h | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/services/inputflinger/dispatcher/DebugConfig.h b/services/inputflinger/dispatcher/DebugConfig.h index 4f8995f43d..9a2aea69ec 100644 --- a/services/inputflinger/dispatcher/DebugConfig.h +++ b/services/inputflinger/dispatcher/DebugConfig.h @@ -75,11 +75,8 @@ const bool DEBUG_TOUCH_MODE = /** * Log debug messages about touch occlusion - * Enable this via "adb shell setprop log.tag.InputDispatcherTouchOcclusion DEBUG" (requires - * restart) */ -const bool DEBUG_TOUCH_OCCLUSION = - __android_log_is_loggable(ANDROID_LOG_DEBUG, LOG_TAG "TouchOcclusion", ANDROID_LOG_INFO); +constexpr bool DEBUG_TOUCH_OCCLUSION = true; /** * Log debug messages about the app switch latency optimization. -- cgit v1.2.3-59-g8ed1b From 4292810fbe008693390696ae0b9e91e9ffded231 Mon Sep 17 00:00:00 2001 From: joenchen Date: Tue, 6 Sep 2022 18:03:57 +0000 Subject: Set mBrightness when needsComposite is zero When needsComposite is zero in persistBrightness(), the mBrightness needs to be updated to avoid unexpected skipping. Bug: 244268674 Test: adb shell cmd device_state state 0 or 1 repeatly Change-Id: Iccf925c1407e4be489da2dba521b93eee1b1c4b5 --- services/surfaceflinger/DisplayDevice.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/services/surfaceflinger/DisplayDevice.cpp b/services/surfaceflinger/DisplayDevice.cpp index 86809007b4..269a1fa17b 100644 --- a/services/surfaceflinger/DisplayDevice.cpp +++ b/services/surfaceflinger/DisplayDevice.cpp @@ -325,8 +325,10 @@ void DisplayDevice::stageBrightness(float brightness) { } void DisplayDevice::persistBrightness(bool needsComposite) { - if (needsComposite && mStagedBrightness && mBrightness != *mStagedBrightness) { - getCompositionDisplay()->setNextBrightness(*mStagedBrightness); + if (mStagedBrightness && mBrightness != *mStagedBrightness) { + if (needsComposite) { + getCompositionDisplay()->setNextBrightness(*mStagedBrightness); + } mBrightness = *mStagedBrightness; } mStagedBrightness = std::nullopt; -- cgit v1.2.3-59-g8ed1b From 3b3e59185dc1e9a319d8ce20ac19c30a966a5a9c Mon Sep 17 00:00:00 2001 From: Alec Mouri Date: Mon, 26 Sep 2022 21:37:01 +0000 Subject: Fix use-after-free in SurfaceFlinger::doDump SurfaceFlinger::doDump previously contained two layer traversals, one on the main thread and one off the main thread. During concurrent dumpsys commands, the layer traversals may race with each other, which causes shared ownership of the underlying storage of a SortedVector containing the z-ordered list of layers. Because the implementation of SortedVector's STL iterators assumes that the underlying storage may be edited, this can cause the storage to be copied whenever SortedVector::begin and SortedVector::end are called, which means that SortedVector::begin() + SortedVector::size() == SortedVector::end() is not always true, which causes invalid iteration. In general, this use-after-free can happen as long as the off-main thread traversal exists in doDump(), because the traversal can run in parallel with any workload on the main thread that executes a layer traversal. So, this patch moves the traversal for dumping out the list of composition layers into the main thread. A future patch could explore either fixing SortedVector to fix judicious iterator invalidation, or building LayerVector on top of std::set, but either option is an invasive data structure change. Bug: 237291506 Test: Test script that calls dumpsys SurfaceFlinger on many threads Change-Id: I0748396519c924dc1b84113d44259f22d0d7ebd6 (cherry picked from commit 6761733ad1dd775f011588c59d5a6d210175c546) Merged-In: I0748396519c924dc1b84113d44259f22d0d7ebd6 --- services/surfaceflinger/SurfaceFlinger.cpp | 37 +++++++++++++++++++----------- services/surfaceflinger/SurfaceFlinger.h | 3 ++- 2 files changed, 25 insertions(+), 15 deletions(-) diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index db2447c40b..0e1acb4154 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -5005,6 +5005,25 @@ status_t SurfaceFlinger::doDump(int fd, const DumpArgs& args, bool asProto) { const auto flag = args.empty() ? ""s : std::string(String8(args[0])); + // Traversal of drawing state must happen on the main thread. + // Otherwise, SortedVector may have shared ownership during concurrent + // traversals, which can result in use-after-frees. + std::string compositionLayers; + mScheduler + ->schedule([&] { + StringAppendF(&compositionLayers, "Composition layers\n"); + mDrawingState.traverseInZOrder([&](Layer* layer) { + auto* compositionState = layer->getCompositionState(); + if (!compositionState || !compositionState->isVisible) return; + + android::base::StringAppendF(&compositionLayers, "* Layer %p (%s)\n", layer, + layer->getDebugName() ? layer->getDebugName() + : ""); + compositionState->dump(compositionLayers); + }); + }) + .get(); + bool dumpLayers = true; { TimedLock lock(mStateLock, s2ns(1), __func__); @@ -5017,7 +5036,7 @@ status_t SurfaceFlinger::doDump(int fd, const DumpArgs& args, bool asProto) { (it->second)(args, asProto, result); dumpLayers = false; } else if (!asProto) { - dumpAllLocked(args, result); + dumpAllLocked(args, compositionLayers, result); } } @@ -5316,7 +5335,8 @@ void SurfaceFlinger::dumpOffscreenLayers(std::string& result) { result.append(future.get()); } -void SurfaceFlinger::dumpAllLocked(const DumpArgs& args, std::string& result) const { +void SurfaceFlinger::dumpAllLocked(const DumpArgs& args, const std::string& compositionLayers, + std::string& result) const { const bool colorize = !args.empty() && args[0] == String16("--color"); Colorizer colorizer(colorize); @@ -5367,18 +5387,7 @@ void SurfaceFlinger::dumpAllLocked(const DumpArgs& args, std::string& result) co StringAppendF(&result, "Visible layers (count = %zu)\n", mNumLayers.load()); colorizer.reset(result); - { - StringAppendF(&result, "Composition layers\n"); - mDrawingState.traverseInZOrder([&](Layer* layer) { - auto* compositionState = layer->getCompositionState(); - if (!compositionState || !compositionState->isVisible) return; - - android::base::StringAppendF(&result, "* Layer %p (%s)\n", layer, - layer->getDebugName() ? layer->getDebugName() - : ""); - compositionState->dump(result); - }); - } + result.append(compositionLayers); colorizer.bold(result); StringAppendF(&result, "Displays (%zu entries)\n", mDisplays.size()); diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 83134a2ebc..9e0cee8fd4 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -1085,7 +1085,8 @@ private: /* * Debugging & dumpsys */ - void dumpAllLocked(const DumpArgs& args, std::string& result) const REQUIRES(mStateLock); + void dumpAllLocked(const DumpArgs& args, const std::string& compositionLayers, + std::string& result) const REQUIRES(mStateLock); void appendSfConfigString(std::string& result) const; void listLayersLocked(std::string& result) const; -- cgit v1.2.3-59-g8ed1b From 405e2f68fbe2992e3b06f1f0a55df331d5150a64 Mon Sep 17 00:00:00 2001 From: Robert Carr Date: Fri, 31 Dec 2021 16:59:34 -0800 Subject: BlastBufferQueue: Fake release if not received by complete This is a cherry-pick from of a CL from sc-v2 adding a safety mechanism for lost buffers. The original CL probably should have been merged in to master, and I can't relate to my thought process adding DO NOT MERGE on the original CL. Recently we have started seeing some of these ANRs again. Well actually we have seen them continuously, but most have been due to GPU hangs. Recently we started seeing some which aren't due to GPU hangs. We found one trace, which showed the sort of situation described below, which lead me to realizing this code was never merged in to master. It shoud be relatively safe since we released it on sc-v2. Perfetto telemetry shows a cluster where a transactionComplete is arriving but an earlier releaseBufferCallback has never arrived. This leads to blocking and ANRs. We try to work around this by generating a fake release buffer callback for previous buffers when we receive a TransactionCompleteCallback. How can we know this is safe? The criteria to safely release buffers are as follows: Condition 1. SurfaceFlinger does not plan to use the buffer Condition 2. Have the acquire fence (or a later signalling fence)if dropped, have the HWC release fence if presented. Now lets break down cases where the transaction complete callback arrives before the release buffer callback. All release buffer callbacks fall in to two categories: Category 1. In band with the complete callback. In which case the client library in SurfaceComposerClient always sends release callbacks first. Category 2. Out of band, e.g. from setBuffer or merge when dropping buffers In category 2 there are two scenarios: Scenario 1: Release callback was never going to arrive (bug) Scenario 2. Release callback descheduled, e.g. a. Transaction is filled with buffer and held in VRI/WM/SysUI b. A second transaction with buffer is merged in, the release buffer callback is emitted but not scheduled (async binder) c. The merged transaction is applied d. SurfaceFlinger processes a frame and emits the transaction complete callback e. The transaction complete callback is scheduled before the release callback is ever scheduled (since the 1 way binders are from different processes). In both scenarios, we can satisfy Conditions 1 and 2, because the HWC present fence is a later signalling fence than the acquire fence which the out of band callback will pass. While we may block extra on this fence. We will be safe by condition 2. Receiving the transaction complete callback should indicate condition 1 is satisfied. In category 1 there should only be a single scenario, the release buffer callback for frame N-1 is emitted immediately before the transaction callback for frame N, and so if it hasn't come at that time (and isn't out of band/scheduled e.g. category 2) then it will never come. We can further break this down in to two scenarios: 1. stat.previousReleaseFence is set correctly. This is the expected case. In this case, this is the same fence we would receive from the release buffer callback, and so we can satisfy condition 1 and 2 and safely release. 2. stat.previousReleaseFence is not set correctly. There is no known reason for this to happen, but there is also no known reason why we would receive a complete callback without the corresponding release, and so we have to explore the option as a potential risk/behavior change from the change. Hypothetically in category 1 case 2 we could experience some tearing. In category 1 we are currently experiencing ANR, and so the tradeoff is a risk of tearing vs a certainty of ANR. To tear the following would have to happen: 1. Enter the case where we previously ANRed 2. Enter the subcase where the release fence is not set correctly 3. Be scheduled very fast on the client side, in practicality HWC present has already returned and the fence should be firing very soon 4. The previous frame not be GL comp, in which case we were already done with the buffer and don't need the fence anyway. Conditions 2 and 3 should already make the occurence of tearing much lower than the occurence of ANRs. Furthermore 100% of observed ANRs were during GL comp (rotation animation) and so the telemetry indicates the risk of introducing tearing is very low. To review, we have shown that we can break down the side effects from the change in to two buckets (category 1, and category 2). In category 2, the possible negative side effect is to use too late of a fence. However this is still safe, and should be exceedingly rare. In category 1 we have shown that there are two scenarios, and in one of these scenarios we can experience tearing. We have furthermore argued that the risk of tearing should be exceedingly low even in this scenario, while the risk of ANR in this scenario was nearly 100%. Bug: 247246160 Bug: 244218818 Test: Existing tests pass Change-Id: I757ed132ac076aa811df816e04a68f57b38e47e7 --- libs/gui/BLASTBufferQueue.cpp | 28 ++++++++++++++++++++++++++-- libs/gui/SurfaceComposerClient.cpp | 6 ++++-- libs/gui/include/gui/BLASTBufferQueue.h | 9 +++++++++ libs/gui/include/gui/SurfaceComposerClient.h | 7 +++++-- libs/gui/include/gui/test/CallbackUtils.h | 3 ++- 5 files changed, 46 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 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& releaseFence, std::optional currentMaxAcquiredBufferCount) { BBQ_TRACE(); + std::unique_lock _lock{mMutex}; + releaseBufferCallbackLocked(id, releaseFence, currentMaxAcquiredBufferCount); +} + +void BLASTBufferQueue::releaseBufferCallbackLocked(const ReleaseCallbackId& id, + const sp& releaseFence, std::optional 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& stats); void releaseBufferCallback(const ReleaseCallbackId& id, const sp& releaseFence, std::optional currentMaxAcquiredBufferCount); + void releaseBufferCallbackLocked(const ReleaseCallbackId& id, const sp& releaseFence, + std::optional currentMaxAcquiredBufferCount); void syncNextTransaction(std::function 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 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 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& sc, nsecs_t latchTime, std::variant> acquireTimeOrFence, const sp& presentFence, const sp& 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; nsecs_t latchTime = -1; @@ -81,6 +83,7 @@ struct SurfaceControlStats { sp 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(acquireTimeOrFence)); ASSERT_EQ(std::get(acquireTimeOrFence) > 0, -- cgit v1.2.3-59-g8ed1b From 6c57b2f55412a04a3a9d738af0185d0ca26f932f Mon Sep 17 00:00:00 2001 From: Siarhei Vishniakou Date: Wed, 28 Sep 2022 10:48:29 -0700 Subject: Delete mController when eventHub device is going away When an event hub device is going away, the controller associated with it should be deleted. Before this CL, mController would remain, and its use would result in an illegal memory access, because it was storing a reference to a deleted context. Bug: 245770596 Test: atest inputflinger_tests:InputDeviceTest#DumpDoesNotCrash Merged-In: I298f43172472f74fa4d5d8e0b7f52bd5c13d14f7 Change-Id: I298f43172472f74fa4d5d8e0b7f52bd5c13d14f7 (cherry picked from commit 30feb8c162aa2e5348bba20e99e8db2a61bac6e7) --- services/inputflinger/reader/InputDevice.cpp | 4 ++++ .../reader/controller/PeripheralController.cpp | 4 ++++ .../reader/controller/PeripheralController.h | 2 ++ .../reader/controller/PeripheralControllerInterface.h | 2 ++ services/inputflinger/tests/InputReader_test.cpp | 18 ++++++++++++++++++ 5 files changed, 30 insertions(+) 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 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 getBatteryCapacity(int32_t batteryId) = 0; virtual std::optional 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 { -- cgit v1.2.3-59-g8ed1b