surfaceflinger: flush HWC commands when present is skipped
This is how SF talks to HWC in each frame
1) queue state changes for display
2) queue validate display
3) flush
4) queue more state changes for display
5) queue present display
6) another flush
When validate implies present, we skip all steps after 3). This can
be less desirable because, when SF (incorrectly) queues more state
changes in 4), we still want to flush them to HWC such that SF
states and HWC states are consistent. The only step we want to skip
is 5) in that case. Note that we still skip 6) when 4) and 5) do
not queue anything.
The problem is much more worse in a multiple display setup
1) queue state changes for display 1
2) queue validate display 1
3) flush
4) queue state changes for display 2
5) queue validate display 2
6) flush
7) queue more state changes for display 1
8) queue more state changes for display 2
9) queue present display 1
a) flush
b) queue present display 2
c) flush
When validate implies present for display 1, but not for display 2,
we skip steps 7) to a), including the valid state changes for
display 2 in 8)! The only step we want to skip should be 9). Maybe
a) as well, but that is an optimization.
In both cases, what we really want is to skip the unnecessary
present and nothing else.
Bug: 67512553
Test: manual
Change-Id: I32d6218cda5c953e0359fecda94d2bb5482e061e
diff --git a/services/surfaceflinger/DisplayHardware/ComposerHal.cpp b/services/surfaceflinger/DisplayHardware/ComposerHal.cpp
index 7d6d988..c707e3c 100644
--- a/services/surfaceflinger/DisplayHardware/ComposerHal.cpp
+++ b/services/surfaceflinger/DisplayHardware/ComposerHal.cpp
@@ -223,6 +223,10 @@
mWriter.reset();
}
+Error Composer::executeCommands() {
+ return execute();
+}
+
uint32_t Composer::getMaxVirtualDisplayCount()
{
auto ret = mClient->getMaxVirtualDisplayCount();
@@ -750,6 +754,11 @@
}
}
+ if (commandLength == 0) {
+ mWriter.reset();
+ return Error::NONE;
+ }
+
Error error = kDefaultError;
auto ret = mClient->executeCommands(commandLength, commandHandles,
[&](const auto& tmpError, const auto& tmpOutChanged,
diff --git a/services/surfaceflinger/DisplayHardware/ComposerHal.h b/services/surfaceflinger/DisplayHardware/ComposerHal.h
index 31a3c1d..104ca60 100644
--- a/services/surfaceflinger/DisplayHardware/ComposerHal.h
+++ b/services/surfaceflinger/DisplayHardware/ComposerHal.h
@@ -152,6 +152,9 @@
// skip a frame but have already queued some commands.
void resetCommands();
+ // Explicitly flush all pending commands in the command buffer.
+ Error executeCommands();
+
uint32_t getMaxVirtualDisplayCount();
bool isUsingVrComposer() const { return mIsUsingVrComposer; }
Error createVirtualDisplay(uint32_t width, uint32_t height,
diff --git a/services/surfaceflinger/DisplayHardware/HWC2.cpp b/services/surfaceflinger/DisplayHardware/HWC2.cpp
index 78c0c85..ab4a4b2 100644
--- a/services/surfaceflinger/DisplayHardware/HWC2.cpp
+++ b/services/surfaceflinger/DisplayHardware/HWC2.cpp
@@ -221,6 +221,11 @@
}
}
+Error Device::flushCommands()
+{
+ return static_cast<Error>(mComposer->executeCommands());
+}
+
// Display methods
Display::Display(android::Hwc2::Composer& composer,
@@ -632,11 +637,6 @@
return error;
}
-void Display::discardCommands()
-{
- mComposer.resetCommands();
-}
-
// For use by Device
void Display::setConnected(bool connected) {
diff --git a/services/surfaceflinger/DisplayHardware/HWC2.h b/services/surfaceflinger/DisplayHardware/HWC2.h
index fbe4c7e..a15c6d9 100644
--- a/services/surfaceflinger/DisplayHardware/HWC2.h
+++ b/services/surfaceflinger/DisplayHardware/HWC2.h
@@ -106,6 +106,11 @@
android::Hwc2::Composer* getComposer() { return mComposer.get(); }
+ // We buffer most state changes and flush them implicitly with
+ // Display::validate, Display::present, and Display::presentOrValidate.
+ // This method provides an explicit way to flush state changes to HWC.
+ Error flushCommands();
+
private:
// Initialization methods
@@ -244,12 +249,6 @@
uint32_t* outNumRequests,
android::sp<android::Fence>* outPresentFence, uint32_t* state);
- // Most methods in this class write a command to a command buffer. The
- // command buffer is implicitly submitted in validate, present, and
- // presentOrValidate. This method provides a way to discard the commands,
- // which can be used to discard stale commands.
- void discardCommands();
-
// Other Display methods
hwc2_display_t getId() const { return mId; }
diff --git a/services/surfaceflinger/DisplayHardware/HWComposer.cpp b/services/surfaceflinger/DisplayHardware/HWComposer.cpp
index b096a3a..5328a22 100644
--- a/services/surfaceflinger/DisplayHardware/HWComposer.cpp
+++ b/services/surfaceflinger/DisplayHardware/HWComposer.cpp
@@ -605,8 +605,11 @@
auto& hwcDisplay = displayData.hwcDisplay;
if (displayData.validateWasSkipped) {
- hwcDisplay->discardCommands();
- auto error = displayData.presentError;
+ // explicitly flush all pending commands
+ auto error = mHwcDevice->flushCommands();
+ if (displayData.presentError != HWC2::Error::None) {
+ error = displayData.presentError;
+ }
if (error != HWC2::Error::None) {
ALOGE("skipValidate: failed for display %d: %s (%d)",
displayId, to_string(error).c_str(), static_cast<int32_t>(error));