SF: Don't cache display modes in HWComposer
The display modes should be stored only in DisplayDevice.
Having the state also in HWComposer is unnesesary and hard to
keep in sync with SF, e.g. during hotplug processing of
displays which can change their supported modes.
Any HWC calls which need to validate their parameters need
to go through display device. This additinally makes the
code more undestandable.
Bug: 159590486
Test: presubmit
Change-Id: I40b03c09a5fd6092fca0682d602deb70db022fa5
diff --git a/services/surfaceflinger/tests/unittests/CompositionTest.cpp b/services/surfaceflinger/tests/unittests/CompositionTest.cpp
index f2051d9..b696a6d 100644
--- a/services/surfaceflinger/tests/unittests/CompositionTest.cpp
+++ b/services/surfaceflinger/tests/unittests/CompositionTest.cpp
@@ -62,15 +62,9 @@
using testing::_;
using testing::AtLeast;
-using testing::Between;
-using testing::ByMove;
using testing::DoAll;
-using testing::Field;
-using testing::Invoke;
using testing::IsNull;
using testing::Mock;
-using testing::NotNull;
-using testing::Ref;
using testing::Return;
using testing::ReturnRef;
using testing::SetArgPointee;
@@ -86,7 +80,6 @@
constexpr int DEFAULT_DISPLAY_WIDTH = 1920;
constexpr int DEFAULT_DISPLAY_HEIGHT = 1024;
-constexpr int DEFAULT_CONFIG_ID = 0;
constexpr int DEFAULT_TEXTURE_ID = 6000;
constexpr int DEFAULT_LAYER_STACK = 7000;
@@ -147,7 +140,7 @@
EXPECT_CALL(*vsyncTracker, nextAnticipatedVSyncTimeFrom(_)).WillRepeatedly(Return(0));
EXPECT_CALL(*vsyncTracker, currentPeriod())
- .WillRepeatedly(Return(FakeHwcDisplayInjector::DEFAULT_REFRESH_RATE));
+ .WillRepeatedly(Return(FakeHwcDisplayInjector::DEFAULT_VSYNC_PERIOD));
EXPECT_CALL(*vsyncTracker, nextAnticipatedVSyncTimeFrom(_)).WillRepeatedly(Return(0));
constexpr ISchedulerCallback* kCallback = nullptr;
@@ -548,12 +541,6 @@
setupLatchedBuffer(test, layer);
}
- static void setupBufferLayerPostFrameCallExpectations(CompositionTest* test) {
- // BufferLayer::onPostComposition(), when there is no present fence
- EXPECT_CALL(*test->mComposer, getActiveConfig(HWC_DISPLAY, _))
- .WillOnce(DoAll(SetArgPointee<1>(DEFAULT_CONFIG_ID), Return(Error::NONE)));
- }
-
static void setupHwcSetGeometryCallExpectations(CompositionTest* test) {
if (!test->mDisplayOff) {
// TODO: Coverage of other values
@@ -632,8 +619,6 @@
.Times(1);
EXPECT_CALL(*test->mComposer, setLayerBuffer(HWC_DISPLAY, HWC_LAYER, _, _, _)).Times(1);
}
-
- setupBufferLayerPostFrameCallExpectations(test);
}
static void setupREBufferCompositionCommonCallExpectations(CompositionTest* test) {
@@ -793,7 +778,6 @@
static void setupInsecureREBufferCompositionCallExpectations(CompositionTest* test) {
setupInsecureREBufferCompositionCommonCallExpectations(test);
- Base::setupBufferLayerPostFrameCallExpectations(test);
}
static void setupInsecureREBufferScreenshotCompositionCallExpectations(CompositionTest* test) {
diff --git a/services/surfaceflinger/tests/unittests/HWComposerTest.cpp b/services/surfaceflinger/tests/unittests/HWComposerTest.cpp
index 1e24c0a..6b82170 100644
--- a/services/surfaceflinger/tests/unittests/HWComposerTest.cpp
+++ b/services/surfaceflinger/tests/unittests/HWComposerTest.cpp
@@ -175,90 +175,5 @@
EXPECT_EQ(hal::Error::UNSUPPORTED, result);
}
-class HWComposerConfigsTest : public testing::Test {
-public:
- Hwc2::mock::Composer* mHal = new StrictMock<Hwc2::mock::Composer>();
- MockHWC2ComposerCallback mCallback;
-
- void setActiveConfig(Config config) {
- EXPECT_CALL(*mHal, getActiveConfig(_, _))
- .WillRepeatedly(DoAll(SetArgPointee<1>(config), Return(V2_1::Error::NONE)));
- }
-
- void setDisplayConfigs(std::vector<Config> configs) {
- EXPECT_CALL(*mHal, getDisplayConfigs(_, _))
- .WillOnce(DoAll(SetArgPointee<1>(configs), Return(V2_1::Error::NONE)));
- EXPECT_CALL(*mHal, getDisplayAttribute(_, _, _, _))
- .WillRepeatedly(DoAll(SetArgPointee<3>(1), Return(V2_1::Error::NONE)));
- }
-
- void testSetActiveModeWithConstraintsCommon(bool isVsyncPeriodSwitchSupported);
-};
-
-void HWComposerConfigsTest::testSetActiveModeWithConstraintsCommon(
- bool isVsyncPeriodSwitchSupported) {
- EXPECT_CALL(*mHal, getMaxVirtualDisplayCount()).WillOnce(Return(0));
- EXPECT_CALL(*mHal, getCapabilities()).WillOnce(Return(std::vector<hal::Capability>{}));
- EXPECT_CALL(*mHal, getLayerGenericMetadataKeys(_)).WillOnce(Return(V2_4::Error::UNSUPPORTED));
- EXPECT_CALL(*mHal, registerCallback(_));
- EXPECT_CALL(*mHal, setVsyncEnabled(_, _)).WillRepeatedly(Return(V2_1::Error::NONE));
- EXPECT_CALL(*mHal, getDisplayIdentificationData(_, _, _))
- .WillRepeatedly(Return(V2_1::Error::UNSUPPORTED));
- EXPECT_CALL(*mHal, setClientTargetSlotCount(_)).WillRepeatedly(Return(V2_1::Error::NONE));
-
- EXPECT_CALL(*mHal, isVsyncPeriodSwitchSupported())
- .WillRepeatedly(Return(isVsyncPeriodSwitchSupported));
-
- if (isVsyncPeriodSwitchSupported) {
- EXPECT_CALL(*mHal, setActiveConfigWithConstraints(_, _, _, _))
- .WillRepeatedly(Return(V2_4::Error::NONE));
- } else {
- EXPECT_CALL(*mHal, setActiveConfig(_, _)).WillRepeatedly(Return(V2_1::Error::NONE));
- }
-
- impl::HWComposer hwc{std::unique_ptr<Hwc2::Composer>(mHal)};
- hwc.setConfiguration(&mCallback, 123);
-
- setDisplayConfigs({15});
- setActiveConfig(15);
-
- const auto physicalId = PhysicalDisplayId::fromPort(0);
- const hal::HWDisplayId hwcId = 0;
- hwc.allocatePhysicalDisplay(hwcId, physicalId);
-
- hal::VsyncPeriodChangeConstraints constraints;
- constraints.desiredTimeNanos = systemTime();
- constraints.seamlessRequired = false;
-
- hal::VsyncPeriodChangeTimeline timeline = {0, 0, 0};
- constexpr Config kConfigIndex = 0;
- const auto status =
- hwc.setActiveModeWithConstraints(physicalId, kConfigIndex, constraints, &timeline);
- EXPECT_EQ(NO_ERROR, status);
-
- const std::vector<Config> kConfigs{7, 8, 9, 10, 11};
- // Change the set of supported modes.
- setDisplayConfigs(kConfigs);
- setActiveConfig(11);
- hwc.onHotplug(hwcId, hal::Connection::CONNECTED);
- hwc.allocatePhysicalDisplay(hwcId, physicalId);
-
- for (size_t configIndex = 0; configIndex < kConfigs.size(); configIndex++) {
- const auto status =
- hwc.setActiveModeWithConstraints(physicalId,
- static_cast<hal::HWConfigId>(configIndex),
- constraints, &timeline);
- EXPECT_EQ(NO_ERROR, status) << "Error when switching to config " << configIndex;
- }
-}
-
-TEST_F(HWComposerConfigsTest, setActiveModeWithConstraintsWithVsyncSwitchingSupported) {
- testSetActiveModeWithConstraintsCommon(/*supported=*/true);
-}
-
-TEST_F(HWComposerConfigsTest, setActiveModeWithConstraintsWithVsyncSwitchingNotSupported) {
- testSetActiveModeWithConstraintsCommon(/*supported=*/false);
-}
-
} // namespace
} // namespace android
\ No newline at end of file
diff --git a/services/surfaceflinger/tests/unittests/RefreshRateSelectionTest.cpp b/services/surfaceflinger/tests/unittests/RefreshRateSelectionTest.cpp
index c5deb7c..abecd4b 100644
--- a/services/surfaceflinger/tests/unittests/RefreshRateSelectionTest.cpp
+++ b/services/surfaceflinger/tests/unittests/RefreshRateSelectionTest.cpp
@@ -144,7 +144,7 @@
EXPECT_CALL(*vsyncTracker, nextAnticipatedVSyncTimeFrom(_)).WillRepeatedly(Return(0));
EXPECT_CALL(*vsyncTracker, currentPeriod())
- .WillRepeatedly(Return(FakeHwcDisplayInjector::DEFAULT_REFRESH_RATE));
+ .WillRepeatedly(Return(FakeHwcDisplayInjector::DEFAULT_VSYNC_PERIOD));
EXPECT_CALL(*vsyncTracker, nextAnticipatedVSyncTimeFrom(_)).WillRepeatedly(Return(0));
mFlinger.setupScheduler(std::move(vsyncController), std::move(vsyncTracker),
std::move(eventThread), std::move(sfEventThread));
diff --git a/services/surfaceflinger/tests/unittests/SetFrameRateTest.cpp b/services/surfaceflinger/tests/unittests/SetFrameRateTest.cpp
index e060df2..a6d07d0 100644
--- a/services/surfaceflinger/tests/unittests/SetFrameRateTest.cpp
+++ b/services/surfaceflinger/tests/unittests/SetFrameRateTest.cpp
@@ -185,7 +185,7 @@
EXPECT_CALL(*vsyncTracker, nextAnticipatedVSyncTimeFrom(_)).WillRepeatedly(Return(0));
EXPECT_CALL(*vsyncTracker, currentPeriod())
- .WillRepeatedly(Return(FakeHwcDisplayInjector::DEFAULT_REFRESH_RATE));
+ .WillRepeatedly(Return(FakeHwcDisplayInjector::DEFAULT_VSYNC_PERIOD));
EXPECT_CALL(*vsyncTracker, nextAnticipatedVSyncTimeFrom(_)).WillRepeatedly(Return(0));
mFlinger.setupScheduler(std::move(vsyncController), std::move(vsyncTracker),
std::move(eventThread), std::move(sfEventThread));
diff --git a/services/surfaceflinger/tests/unittests/SurfaceFlinger_HandleTransactionLockedTest.cpp b/services/surfaceflinger/tests/unittests/SurfaceFlinger_HandleTransactionLockedTest.cpp
index 8552e15..b713334 100644
--- a/services/surfaceflinger/tests/unittests/SurfaceFlinger_HandleTransactionLockedTest.cpp
+++ b/services/surfaceflinger/tests/unittests/SurfaceFlinger_HandleTransactionLockedTest.cpp
@@ -669,7 +669,7 @@
// --------------------------------------------------------------------
// Call Expectations
- EXPECT_CALL(*displaySurface, resizeBuffers(newWidth, oldHeight)).Times(1);
+ EXPECT_CALL(*displaySurface, resizeBuffers(ui::Size(newWidth, oldHeight))).Times(1);
// --------------------------------------------------------------------
// Invocation
@@ -714,7 +714,7 @@
// --------------------------------------------------------------------
// Call Expectations
- EXPECT_CALL(*displaySurface, resizeBuffers(oldWidth, newHeight)).Times(1);
+ EXPECT_CALL(*displaySurface, resizeBuffers(ui::Size(oldWidth, newHeight))).Times(1);
// --------------------------------------------------------------------
// Invocation
@@ -764,7 +764,7 @@
// --------------------------------------------------------------------
// Call Expectations
- EXPECT_CALL(*displaySurface, resizeBuffers(kNewWidth, kNewHeight)).Times(1);
+ EXPECT_CALL(*displaySurface, resizeBuffers(kNewSize.getSize())).Times(1);
// --------------------------------------------------------------------
// Invocation
diff --git a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h
index 3be45e2..16a76c2 100644
--- a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h
+++ b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h
@@ -41,6 +41,7 @@
#include "SurfaceFlingerDefaultFactory.h"
#include "SurfaceInterceptor.h"
#include "TestableScheduler.h"
+#include "mock/DisplayHardware/MockComposer.h"
#include "mock/MockDisplayIdGenerator.h"
#include "mock/MockFrameTimeline.h"
#include "mock/MockFrameTracer.h"
@@ -477,7 +478,7 @@
static constexpr hal::HWDisplayId DEFAULT_HWC_DISPLAY_ID = 1000;
static constexpr int32_t DEFAULT_WIDTH = 1920;
static constexpr int32_t DEFAULT_HEIGHT = 1280;
- static constexpr int32_t DEFAULT_REFRESH_RATE = 16'666'666;
+ static constexpr int32_t DEFAULT_VSYNC_PERIOD = 16'666'666;
static constexpr int32_t DEFAULT_CONFIG_GROUP = 7;
static constexpr int32_t DEFAULT_DPI = 320;
static constexpr hal::HWConfigId DEFAULT_ACTIVE_CONFIG = 0;
@@ -502,8 +503,8 @@
return *this;
}
- auto& setRefreshRate(int32_t refreshRate) {
- mRefreshRate = refreshRate;
+ auto& setVsyncPeriod(int32_t vsyncPeriod) {
+ mVsyncPeriod = vsyncPeriod;
return *this;
}
@@ -532,7 +533,12 @@
return *this;
}
- void inject(TestableSurfaceFlinger* flinger, Hwc2::Composer* composer) {
+ void inject(TestableSurfaceFlinger* flinger, Hwc2::mock::Composer* composer) {
+ using ::testing::_;
+ using ::testing::DoAll;
+ using ::testing::Return;
+ using ::testing::SetArgPointee;
+
static const std::unordered_set<hal::Capability> defaultCapabilities;
if (mCapabilities == nullptr) mCapabilities = &defaultCapabilities;
@@ -547,15 +553,39 @@
display->setPowerMode(mPowerMode);
flinger->mutableHwcDisplayData()[mDisplayId].hwcDisplay = std::move(display);
- auto config = DisplayMode::Builder(mActiveConfig)
- .setWidth(mWidth)
- .setHeight(mHeight)
- .setVsyncPeriod(mRefreshRate)
- .setDpiX(mDpiX)
- .setDpiY(mDpiY)
- .setConfigGroup(mConfigGroup)
- .build();
- flinger->mutableHwcDisplayData()[mDisplayId].modes.push_back(config);
+ EXPECT_CALL(*composer, getDisplayConfigs(mHwcDisplayId, _))
+ .WillRepeatedly(
+ DoAll(SetArgPointee<1>(std::vector<hal::HWConfigId>{mActiveConfig}),
+ Return(hal::Error::NONE)));
+
+ EXPECT_CALL(*composer,
+ getDisplayAttribute(mHwcDisplayId, mActiveConfig, hal::Attribute::WIDTH, _))
+ .WillRepeatedly(DoAll(SetArgPointee<3>(mWidth), Return(hal::Error::NONE)));
+
+ EXPECT_CALL(*composer,
+ getDisplayAttribute(mHwcDisplayId, mActiveConfig, hal::Attribute::HEIGHT,
+ _))
+ .WillRepeatedly(DoAll(SetArgPointee<3>(mHeight), Return(hal::Error::NONE)));
+
+ EXPECT_CALL(*composer,
+ getDisplayAttribute(mHwcDisplayId, mActiveConfig,
+ hal::Attribute::VSYNC_PERIOD, _))
+ .WillRepeatedly(
+ DoAll(SetArgPointee<3>(mVsyncPeriod), Return(hal::Error::NONE)));
+
+ EXPECT_CALL(*composer,
+ getDisplayAttribute(mHwcDisplayId, mActiveConfig, hal::Attribute::DPI_X, _))
+ .WillRepeatedly(DoAll(SetArgPointee<3>(mDpiX), Return(hal::Error::NONE)));
+
+ EXPECT_CALL(*composer,
+ getDisplayAttribute(mHwcDisplayId, mActiveConfig, hal::Attribute::DPI_Y, _))
+ .WillRepeatedly(DoAll(SetArgPointee<3>(mDpiY), Return(hal::Error::NONE)));
+
+ EXPECT_CALL(*composer,
+ getDisplayAttribute(mHwcDisplayId, mActiveConfig,
+ hal::Attribute::CONFIG_GROUP, _))
+ .WillRepeatedly(
+ DoAll(SetArgPointee<3>(mConfigGroup), Return(hal::Error::NONE)));
if (mHwcDisplayType == hal::DisplayType::PHYSICAL) {
const auto physicalId = PhysicalDisplayId::tryCast(mDisplayId);
@@ -581,10 +611,10 @@
hal::HWDisplayId mHwcDisplayId = DEFAULT_HWC_DISPLAY_ID;
int32_t mWidth = DEFAULT_WIDTH;
int32_t mHeight = DEFAULT_HEIGHT;
- int32_t mRefreshRate = DEFAULT_REFRESH_RATE;
+ int32_t mVsyncPeriod = DEFAULT_VSYNC_PERIOD;
int32_t mDpiX = DEFAULT_DPI;
- int32_t mConfigGroup = DEFAULT_CONFIG_GROUP;
int32_t mDpiY = DEFAULT_DPI;
+ int32_t mConfigGroup = DEFAULT_CONFIG_GROUP;
hal::HWConfigId mActiveConfig = DEFAULT_ACTIVE_CONFIG;
hal::PowerMode mPowerMode = DEFAULT_POWER_MODE;
const std::unordered_set<hal::Capability>* mCapabilities = nullptr;
@@ -602,6 +632,21 @@
mHwcDisplayId(hwcDisplayId) {
mCreationArgs.connectionType = connectionType;
mCreationArgs.isPrimary = isPrimary;
+
+ mActiveModeId = DisplayModeId(0);
+ DisplayModePtr activeMode =
+ DisplayMode::Builder(FakeHwcDisplayInjector::DEFAULT_ACTIVE_CONFIG)
+ .setId(mActiveModeId)
+ .setWidth(FakeHwcDisplayInjector::DEFAULT_WIDTH)
+ .setHeight(FakeHwcDisplayInjector::DEFAULT_HEIGHT)
+ .setVsyncPeriod(FakeHwcDisplayInjector::DEFAULT_VSYNC_PERIOD)
+ .setDpiX(FakeHwcDisplayInjector::DEFAULT_DPI)
+ .setDpiY(FakeHwcDisplayInjector::DEFAULT_DPI)
+ .setConfigGroup(0)
+ .build();
+
+ DisplayModes modes{activeMode};
+ mCreationArgs.supportedModes = modes;
}
sp<IBinder> token() const { return mDisplayToken; }
@@ -624,6 +669,16 @@
auto& mutableDisplayDevice() { return mFlinger.mutableDisplays()[mDisplayToken]; }
+ auto& setActiveMode(DisplayModeId mode) {
+ mActiveModeId = mode;
+ return *this;
+ }
+
+ auto& setSupportedModes(DisplayModes mode) {
+ mCreationArgs.supportedModes = mode;
+ return *this;
+ }
+
auto& setNativeWindow(const sp<ANativeWindow>& nativeWindow) {
mCreationArgs.nativeWindow = nativeWindow;
return *this;
@@ -676,6 +731,9 @@
state.isSecure = mCreationArgs.isSecure;
sp<DisplayDevice> device = new DisplayDevice(mCreationArgs);
+ if (!device->isVirtual()) {
+ device->setActiveMode(mActiveModeId);
+ }
mFlinger.mutableDisplays().emplace(mDisplayToken, device);
mFlinger.mutableCurrentState().displays.add(mDisplayToken, state);
mFlinger.mutableDrawingState().displays.add(mDisplayToken, state);
@@ -692,6 +750,7 @@
sp<BBinder> mDisplayToken = new BBinder();
DisplayDeviceCreationArgs mCreationArgs;
const std::optional<hal::HWDisplayId> mHwcDisplayId;
+ DisplayModeId mActiveModeId;
};
private:
diff --git a/services/surfaceflinger/tests/unittests/TransactionApplicationTest.cpp b/services/surfaceflinger/tests/unittests/TransactionApplicationTest.cpp
index 6d2f672..1ae42e7 100644
--- a/services/surfaceflinger/tests/unittests/TransactionApplicationTest.cpp
+++ b/services/surfaceflinger/tests/unittests/TransactionApplicationTest.cpp
@@ -72,7 +72,7 @@
EXPECT_CALL(*mVSyncTracker, nextAnticipatedVSyncTimeFrom(_)).WillRepeatedly(Return(0));
EXPECT_CALL(*mVSyncTracker, currentPeriod())
- .WillRepeatedly(Return(FakeHwcDisplayInjector::DEFAULT_REFRESH_RATE));
+ .WillRepeatedly(Return(FakeHwcDisplayInjector::DEFAULT_VSYNC_PERIOD));
mFlinger.setupScheduler(std::unique_ptr<mock::VsyncController>(mVsyncController),
std::unique_ptr<mock::VSyncTracker>(mVSyncTracker),