SF: do not skip validate if it is called more than a vsync earlier
To prevent an early present, we don't want to skip validate and
present immediately if we started the frame to early. Instead we will
do the validate and wait before calling to present. The reason why we
can't wait before validate, is that we might need to do client
composition after validate (based on hwc composition strategy)
and we might miss a frame if we waited before validate.
Bug: 187871031
Test: observe systraces
Change-Id: I9b4b0d4c9c58ddc703bcddadd44dfb0e7eff1cbd
diff --git a/services/surfaceflinger/CompositionEngine/src/Display.cpp b/services/surfaceflinger/CompositionEngine/src/Display.cpp
index 1ffb1c8..8c8331c 100644
--- a/services/surfaceflinger/CompositionEngine/src/Display.cpp
+++ b/services/surfaceflinger/CompositionEngine/src/Display.cpp
@@ -267,7 +267,7 @@
auto& hwc = getCompositionEngine().getHwComposer();
if (status_t result =
hwc.getDeviceCompositionChanges(*halDisplayId, anyLayersRequireClientComposition(),
- &changes);
+ getState().earliestPresentTime, &changes);
result != NO_ERROR) {
ALOGE("chooseCompositionStrategy failed for %s: %d (%s)", getName().c_str(), result,
strerror(-result));
@@ -367,13 +367,8 @@
return fences;
}
- {
- ATRACE_NAME("wait for earliest present time");
- std::this_thread::sleep_until(getState().earliestPresentTime);
- }
-
auto& hwc = getCompositionEngine().getHwComposer();
- hwc.presentAndGetReleaseFences(*halDisplayIdOpt);
+ hwc.presentAndGetReleaseFences(*halDisplayIdOpt, getState().earliestPresentTime);
fences.presentFence = hwc.getPresentFence(*halDisplayIdOpt);
diff --git a/services/surfaceflinger/CompositionEngine/tests/DisplayTest.cpp b/services/surfaceflinger/CompositionEngine/tests/DisplayTest.cpp
index 8a83639..e63d09a 100644
--- a/services/surfaceflinger/CompositionEngine/tests/DisplayTest.cpp
+++ b/services/surfaceflinger/CompositionEngine/tests/DisplayTest.cpp
@@ -627,7 +627,7 @@
TEST_F(DisplayChooseCompositionStrategyTest, takesEarlyOutOnHwcError) {
EXPECT_CALL(*mDisplay, anyLayersRequireClientComposition()).WillOnce(Return(false));
EXPECT_CALL(mHwComposer,
- getDeviceCompositionChanges(HalDisplayId(DEFAULT_DISPLAY_ID), false, _))
+ getDeviceCompositionChanges(HalDisplayId(DEFAULT_DISPLAY_ID), false, _, _))
.WillOnce(Return(INVALID_OPERATION));
mDisplay->chooseCompositionStrategy();
@@ -649,7 +649,8 @@
.InSequence(s)
.WillOnce(Return(false));
- EXPECT_CALL(mHwComposer, getDeviceCompositionChanges(HalDisplayId(DEFAULT_DISPLAY_ID), true, _))
+ EXPECT_CALL(mHwComposer,
+ getDeviceCompositionChanges(HalDisplayId(DEFAULT_DISPLAY_ID), true, _, _))
.WillOnce(Return(NO_ERROR));
EXPECT_CALL(*mDisplay, allLayersRequireClientComposition()).WillOnce(Return(false));
@@ -679,8 +680,9 @@
.InSequence(s)
.WillOnce(Return(false));
- EXPECT_CALL(mHwComposer, getDeviceCompositionChanges(HalDisplayId(DEFAULT_DISPLAY_ID), true, _))
- .WillOnce(DoAll(SetArgPointee<2>(changes), Return(NO_ERROR)));
+ EXPECT_CALL(mHwComposer,
+ getDeviceCompositionChanges(HalDisplayId(DEFAULT_DISPLAY_ID), true, _, _))
+ .WillOnce(DoAll(SetArgPointee<3>(changes), Return(NO_ERROR)));
EXPECT_CALL(*mDisplay, applyChangedTypesToLayers(changes.changedTypes)).Times(1);
EXPECT_CALL(*mDisplay, applyDisplayRequests(changes.displayRequests)).Times(1);
EXPECT_CALL(*mDisplay, applyLayerRequestsToLayers(changes.layerRequests)).Times(1);
@@ -867,7 +869,8 @@
sp<Fence> layer1Fence = new Fence();
sp<Fence> layer2Fence = new Fence();
- EXPECT_CALL(mHwComposer, presentAndGetReleaseFences(HalDisplayId(DEFAULT_DISPLAY_ID))).Times(1);
+ EXPECT_CALL(mHwComposer, presentAndGetReleaseFences(HalDisplayId(DEFAULT_DISPLAY_ID), _))
+ .Times(1);
EXPECT_CALL(mHwComposer, getPresentFence(HalDisplayId(DEFAULT_DISPLAY_ID)))
.WillOnce(Return(presentFence));
EXPECT_CALL(mHwComposer,
@@ -1039,7 +1042,7 @@
mDisplay->editState().isEnabled = true;
- EXPECT_CALL(mHwComposer, presentAndGetReleaseFences(_));
+ EXPECT_CALL(mHwComposer, presentAndGetReleaseFences(_, _));
EXPECT_CALL(*mDisplaySurface, onFrameCommitted());
mDisplay->postFramebuffer();
diff --git a/services/surfaceflinger/CompositionEngine/tests/MockHWComposer.h b/services/surfaceflinger/CompositionEngine/tests/MockHWComposer.h
index bac894a..74d4b92 100644
--- a/services/surfaceflinger/CompositionEngine/tests/MockHWComposer.h
+++ b/services/surfaceflinger/CompositionEngine/tests/MockHWComposer.h
@@ -50,13 +50,14 @@
MOCK_METHOD2(allocatePhysicalDisplay, void(hal::HWDisplayId, PhysicalDisplayId));
MOCK_METHOD1(createLayer, HWC2::Layer*(HalDisplayId));
MOCK_METHOD2(destroyLayer, void(HalDisplayId, HWC2::Layer*));
- MOCK_METHOD3(getDeviceCompositionChanges,
- status_t(HalDisplayId, bool,
+ MOCK_METHOD4(getDeviceCompositionChanges,
+ status_t(HalDisplayId, bool, std::chrono::steady_clock::time_point,
std::optional<android::HWComposer::DeviceRequestedChanges>*));
MOCK_METHOD5(setClientTarget,
status_t(HalDisplayId, uint32_t, const sp<Fence>&, const sp<GraphicBuffer>&,
ui::Dataspace));
- MOCK_METHOD1(presentAndGetReleaseFences, status_t(HalDisplayId));
+ MOCK_METHOD2(presentAndGetReleaseFences,
+ status_t(HalDisplayId, std::chrono::steady_clock::time_point));
MOCK_METHOD2(setPowerMode, status_t(PhysicalDisplayId, hal::PowerMode));
MOCK_METHOD2(setActiveConfig, status_t(HalDisplayId, size_t));
MOCK_METHOD2(setColorTransform, status_t(HalDisplayId, const mat4&));
diff --git a/services/surfaceflinger/DisplayHardware/HWComposer.cpp b/services/surfaceflinger/DisplayHardware/HWComposer.cpp
index b73d032..dc4839e 100644
--- a/services/surfaceflinger/DisplayHardware/HWComposer.cpp
+++ b/services/surfaceflinger/DisplayHardware/HWComposer.cpp
@@ -471,6 +471,7 @@
status_t HWComposer::getDeviceCompositionChanges(
HalDisplayId displayId, bool frameUsesClientComposition,
+ std::chrono::steady_clock::time_point earliestPresentTime,
std::optional<android::HWComposer::DeviceRequestedChanges>* outChanges) {
ATRACE_CALL();
@@ -487,12 +488,14 @@
hal::Error error = hal::Error::NONE;
- // First try to skip validate altogether when there is no client
- // composition. When there is client composition, since we haven't
- // rendered to the client target yet, we should not attempt to skip
- // validate.
+ // First try to skip validate altogether when we passed the earliest time
+ // to present and there is no client. Otherwise, we may present a frame too
+ // early or in case of client composition we first need to render the
+ // client target buffer.
+ const bool canSkipValidate =
+ std::chrono::steady_clock::now() >= earliestPresentTime && !frameUsesClientComposition;
displayData.validateWasSkipped = false;
- if (!frameUsesClientComposition) {
+ if (canSkipValidate) {
sp<Fence> outPresentFence;
uint32_t state = UINT32_MAX;
error = hwcDisplay->presentOrValidate(&numTypes, &numRequests, &outPresentFence , &state);
@@ -556,7 +559,8 @@
return fence->second;
}
-status_t HWComposer::presentAndGetReleaseFences(HalDisplayId displayId) {
+status_t HWComposer::presentAndGetReleaseFences(
+ HalDisplayId displayId, std::chrono::steady_clock::time_point earliestPresentTime) {
ATRACE_CALL();
RETURN_IF_INVALID_DISPLAY(displayId, BAD_INDEX);
@@ -572,6 +576,11 @@
return NO_ERROR;
}
+ {
+ ATRACE_NAME("wait for earliest present time");
+ std::this_thread::sleep_until(earliestPresentTime);
+ }
+
auto error = hwcDisplay->present(&displayData.lastPresentFence);
RETURN_IF_HWC_ERROR_FOR("present", error, displayId, UNKNOWN_ERROR);
diff --git a/services/surfaceflinger/DisplayHardware/HWComposer.h b/services/surfaceflinger/DisplayHardware/HWComposer.h
index f532e50..5bad529 100644
--- a/services/surfaceflinger/DisplayHardware/HWComposer.h
+++ b/services/surfaceflinger/DisplayHardware/HWComposer.h
@@ -129,13 +129,15 @@
// expected.
virtual status_t getDeviceCompositionChanges(
HalDisplayId, bool frameUsesClientComposition,
+ std::chrono::steady_clock::time_point earliestPresentTime,
std::optional<DeviceRequestedChanges>* outChanges) = 0;
virtual status_t setClientTarget(HalDisplayId, uint32_t slot, const sp<Fence>& acquireFence,
const sp<GraphicBuffer>& target, ui::Dataspace) = 0;
// Present layers to the display and read releaseFences.
- virtual status_t presentAndGetReleaseFences(HalDisplayId) = 0;
+ virtual status_t presentAndGetReleaseFences(
+ HalDisplayId, std::chrono::steady_clock::time_point earliestPresentTime) = 0;
// set power mode
virtual status_t setPowerMode(PhysicalDisplayId, hal::PowerMode) = 0;
@@ -268,13 +270,15 @@
status_t getDeviceCompositionChanges(
HalDisplayId, bool frameUsesClientComposition,
+ std::chrono::steady_clock::time_point earliestPresentTime,
std::optional<DeviceRequestedChanges>* outChanges) override;
status_t setClientTarget(HalDisplayId, uint32_t slot, const sp<Fence>& acquireFence,
const sp<GraphicBuffer>& target, ui::Dataspace) override;
// Present layers to the display and read releaseFences.
- status_t presentAndGetReleaseFences(HalDisplayId) override;
+ status_t presentAndGetReleaseFences(
+ HalDisplayId, std::chrono::steady_clock::time_point earliestPresentTime) override;
// set power mode
status_t setPowerMode(PhysicalDisplayId, hal::PowerMode mode) override;