From e9eff9744a5db04b573b85bf2c23d6237428a884 Mon Sep 17 00:00:00 2001 From: Lloyd Pique Date: Tue, 5 May 2020 12:36:44 -0700 Subject: [PATCH] CE: Must dequeue a buffer if flipClientTarget requested To match the logic that always queues a buffer if the HWC has a flipClientTarget request for the current frame, we must also always dequeue a buffer, even if no client composition is being performed. This is an port of an (as yet uncommitted) AOSP change with a ChangeId of I933cbae2f09f81eef6555b1bb1e5991d2c450930, due to the underlying code having been refactored. This version also adds and updates unit tests which were added as part of the refactoring. Bug: 151698217 Test: libcompositionengine_test Test: libsurfaceflinger_test Change-Id: Ia8a1470affb2596b27986cc4153417f48cf4ed1c --- .../CompositionEngine/src/Output.cpp | 66 +++++++++++-------- .../CompositionEngine/tests/DisplayTest.cpp | 4 ++ .../CompositionEngine/tests/OutputTest.cpp | 36 ++++++++-- .../surfaceflinger/DisplayHardware/HWC2.cpp | 2 +- 4 files changed, 71 insertions(+), 37 deletions(-) diff --git a/services/surfaceflinger/CompositionEngine/src/Output.cpp b/services/surfaceflinger/CompositionEngine/src/Output.cpp index 34d0cb2b67..cb7ddda662 100644 --- a/services/surfaceflinger/CompositionEngine/src/Output.cpp +++ b/services/surfaceflinger/CompositionEngine/src/Output.cpp @@ -815,6 +815,43 @@ std::optional Output::composeSurfaces( OutputCompositionState& outputCompositionState = editState(); const TracedOrdinal hasClientComposition = {"hasClientComposition", outputState.usesClientComposition}; + + auto& renderEngine = getCompositionEngine().getRenderEngine(); + const bool supportsProtectedContent = renderEngine.supportsProtectedContent(); + + // If we the display is secure, protected content support is enabled, and at + // least one layer has protected content, we need to use a secure back + // buffer. + if (outputState.isSecure && supportsProtectedContent) { + auto layers = getOutputLayersOrderedByZ(); + bool needsProtected = std::any_of(layers.begin(), layers.end(), [](auto* layer) { + return layer->getLayerFE().getCompositionState()->hasProtectedContent; + }); + if (needsProtected != renderEngine.isProtected()) { + renderEngine.useProtectedContext(needsProtected); + } + if (needsProtected != mRenderSurface->isProtected() && + needsProtected == renderEngine.isProtected()) { + mRenderSurface->setProtected(needsProtected); + } + } + + base::unique_fd fd; + sp buf; + + // If we aren't doing client composition on this output, but do have a + // flipClientTarget request for this frame on this output, we still need to + // dequeue a buffer. + if (hasClientComposition || outputState.flipClientTarget) { + buf = mRenderSurface->dequeueBuffer(&fd); + if (buf == nullptr) { + ALOGW("Dequeuing buffer for display [%s] failed, bailing out of " + "client composition for this frame", + mName.c_str()); + return {}; + } + } + base::unique_fd readyFence; if (!hasClientComposition) { setExpensiveRenderingExpected(false); @@ -823,9 +860,6 @@ std::optional Output::composeSurfaces( ALOGV("hasClientComposition"); - auto& renderEngine = getCompositionEngine().getRenderEngine(); - const bool supportsProtectedContent = renderEngine.supportsProtectedContent(); - renderengine::DisplaySettings clientCompositionDisplay; clientCompositionDisplay.physicalDisplay = outputState.destinationClip; clientCompositionDisplay.clip = outputState.sourceClip; @@ -851,32 +885,6 @@ std::optional Output::composeSurfaces( clientCompositionDisplay.outputDataspace); appendRegionFlashRequests(debugRegion, clientCompositionLayers); - // If we the display is secure, protected content support is enabled, and at - // least one layer has protected content, we need to use a secure back - // buffer. - if (outputState.isSecure && supportsProtectedContent) { - auto layers = getOutputLayersOrderedByZ(); - bool needsProtected = std::any_of(layers.begin(), layers.end(), [](auto* layer) { - return layer->getLayerFE().getCompositionState()->hasProtectedContent; - }); - if (needsProtected != renderEngine.isProtected()) { - renderEngine.useProtectedContext(needsProtected); - } - if (needsProtected != mRenderSurface->isProtected() && - needsProtected == renderEngine.isProtected()) { - mRenderSurface->setProtected(needsProtected); - } - } - - base::unique_fd fd; - sp buf = mRenderSurface->dequeueBuffer(&fd); - if (buf == nullptr) { - ALOGW("Dequeuing buffer for display [%s] failed, bailing out of " - "client composition for this frame", - mName.c_str()); - return std::nullopt; - } - // Check if the client composition requests were rendered into the provided graphic buffer. If // so, we can reuse the buffer and avoid client composition. if (mClientCompositionRequestCache) { diff --git a/services/surfaceflinger/CompositionEngine/tests/DisplayTest.cpp b/services/surfaceflinger/CompositionEngine/tests/DisplayTest.cpp index 59889b696a..62977a4335 100644 --- a/services/surfaceflinger/CompositionEngine/tests/DisplayTest.cpp +++ b/services/surfaceflinger/CompositionEngine/tests/DisplayTest.cpp @@ -30,6 +30,7 @@ #include #include #include +#include #include #include @@ -156,6 +157,8 @@ struct DisplayTestCommon : public testing::Test { DisplayTestCommon() { EXPECT_CALL(mCompositionEngine, getHwComposer()).WillRepeatedly(ReturnRef(mHwComposer)); + EXPECT_CALL(mCompositionEngine, getRenderEngine()).WillRepeatedly(ReturnRef(mRenderEngine)); + EXPECT_CALL(mRenderEngine, supportsProtectedContent()).WillRepeatedly(Return(false)); } DisplayCreationArgs getDisplayCreationArgsForPhysicalHWCDisplay() { @@ -182,6 +185,7 @@ struct DisplayTestCommon : public testing::Test { StrictMock mHwComposer; StrictMock mPowerAdvisor; + StrictMock mRenderEngine; StrictMock mCompositionEngine; sp mNativeWindow = new StrictMock(); }; diff --git a/services/surfaceflinger/CompositionEngine/tests/OutputTest.cpp b/services/surfaceflinger/CompositionEngine/tests/OutputTest.cpp index 1c9cd9c0c0..762d76c6ad 100644 --- a/services/surfaceflinger/CompositionEngine/tests/OutputTest.cpp +++ b/services/surfaceflinger/CompositionEngine/tests/OutputTest.cpp @@ -2797,6 +2797,7 @@ struct OutputComposeSurfacesTest : public testing::Test { mOutput.mState.usesClientComposition = true; mOutput.mState.usesDeviceComposition = false; mOutput.mState.reusedClientComposition = false; + mOutput.mState.flipClientTarget = false; EXPECT_CALL(mOutput, getCompositionEngine()).WillRepeatedly(ReturnRef(mCompositionEngine)); EXPECT_CALL(mCompositionEngine, getRenderEngine()).WillRepeatedly(ReturnRef(mRenderEngine)); @@ -2868,19 +2869,40 @@ const HdrCapabilities OutputComposeSurfacesTest:: TEST_F(OutputComposeSurfacesTest, doesNothingButSignalNoExpensiveRenderingIfNoClientComposition) { mOutput.mState.usesClientComposition = false; + EXPECT_CALL(mRenderEngine, supportsProtectedContent()).WillRepeatedly(Return(false)); + EXPECT_CALL(mOutput, setExpensiveRenderingExpected(false)); verify().execute().expectAFenceWasReturned(); } -TEST_F(OutputComposeSurfacesTest, doesMinimalWorkIfDequeueBufferFails) { - EXPECT_CALL(mOutput, getSkipColorTransform()).WillRepeatedly(Return(false)); - EXPECT_CALL(*mDisplayColorProfile, hasWideColorGamut()).WillRepeatedly(Return(true)); +TEST_F(OutputComposeSurfacesTest, + dequeuesABufferIfNoClientCompositionButFlipClientTargetRequested) { + mOutput.mState.usesClientComposition = false; + mOutput.mState.flipClientTarget = true; + + EXPECT_CALL(mRenderEngine, supportsProtectedContent()).WillRepeatedly(Return(false)); + + EXPECT_CALL(*mRenderSurface, dequeueBuffer(_)).WillOnce(Return(mOutputBuffer)); + EXPECT_CALL(mOutput, setExpensiveRenderingExpected(false)); + + verify().execute().expectAFenceWasReturned(); +} + +TEST_F(OutputComposeSurfacesTest, doesMinimalWorkIfDequeueBufferFailsForClientComposition) { + EXPECT_CALL(mRenderEngine, supportsProtectedContent()).WillRepeatedly(Return(false)); + + EXPECT_CALL(*mRenderSurface, dequeueBuffer(_)).WillOnce(Return(nullptr)); + + verify().execute().expectNoFenceWasReturned(); +} + +TEST_F(OutputComposeSurfacesTest, + doesMinimalWorkIfDequeueBufferFailsForNoClientCompositionButFlipClientTargetRequested) { + mOutput.mState.usesClientComposition = false; + mOutput.mState.flipClientTarget = true; + EXPECT_CALL(mRenderEngine, supportsProtectedContent()).WillRepeatedly(Return(false)); - EXPECT_CALL(mOutput, generateClientCompositionRequests(_, _, kDefaultOutputDataspace)) - .WillRepeatedly(Return(std::vector{})); - EXPECT_CALL(mOutput, appendRegionFlashRequests(RegionEq(kDebugRegion), _)) - .WillRepeatedly(Return()); EXPECT_CALL(*mRenderSurface, dequeueBuffer(_)).WillOnce(Return(nullptr)); diff --git a/services/surfaceflinger/DisplayHardware/HWC2.cpp b/services/surfaceflinger/DisplayHardware/HWC2.cpp index 0ea33408db..fb82033dfa 100644 --- a/services/surfaceflinger/DisplayHardware/HWC2.cpp +++ b/services/surfaceflinger/DisplayHardware/HWC2.cpp @@ -349,7 +349,7 @@ Error Display::getName(std::string* outName) const Error Display::getRequests(HWC2::DisplayRequest* outDisplayRequests, std::unordered_map* outLayerRequests) { - uint32_t intDisplayRequests; + uint32_t intDisplayRequests = 0; std::vector layerIds; std::vector layerRequests; auto intError = mComposer.getDisplayRequests(