diff options
author | 2024-11-07 00:05:03 +0000 | |
---|---|---|
committer | 2024-12-09 18:04:46 +0000 | |
commit | 1e431a4bbe407c86120420fc54321aa038c23504 (patch) | |
tree | 973e9cdf6206c4e3430446dd9984f6e0d09e5334 | |
parent | cb47c6c92cdcdc140400b9de1c946d628960a267 (diff) |
Reject duplicate layer stacks in SF
Different displays could try to assign the same layer stack ID,
causing potential race conditions when releasing fences.
Fixes: b/370358572
Test: atest SurfaceFlinger_test, atest MultiDisplayTest
Flag: com.android.graphics.surfaceflinger.flags.reject_dupe_layerstacks
Change-Id: I2e59daa184d888a1804d8250ba6ce5037542a7d1
-rw-r--r-- | services/surfaceflinger/SurfaceFlinger.cpp | 37 | ||||
-rw-r--r-- | services/surfaceflinger/common/FlagManager.cpp | 2 | ||||
-rw-r--r-- | services/surfaceflinger/common/include/common/FlagManager.h | 1 | ||||
-rw-r--r-- | services/surfaceflinger/surfaceflinger_flags_new.aconfig | 11 | ||||
-rw-r--r-- | services/surfaceflinger/tests/Android.bp | 2 | ||||
-rw-r--r-- | services/surfaceflinger/tests/MultiDisplay_test.cpp (renamed from services/surfaceflinger/tests/MultiDisplayLayerBounds_test.cpp) | 53 |
6 files changed, 99 insertions, 7 deletions
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 59b1917740..0c5323c8ad 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -2785,12 +2785,43 @@ CompositeResultsPerDisplay SurfaceFlinger::composite( const auto& displays = FTL_FAKE_GUARD(mStateLock, mDisplays); refreshArgs.outputs.reserve(displays.size()); + // Track layer stacks of physical displays that might be added to CompositionEngine + // output. Layer stacks are not tracked in Display when we iterate through + // frameTargeters. Cross-referencing layer stacks allows us to filter out displays + // by ID with duplicate layer stacks before adding them to CompositionEngine output. + ui::DisplayMap<DisplayId, ui::LayerStack> physicalDisplayLayerStacks; + for (auto& [_, display] : displays) { + const auto id = PhysicalDisplayId::tryCast(display->getId()); + if (id && frameTargeters.contains(*id)) { + physicalDisplayLayerStacks.try_emplace(*id, display->getLayerStack()); + } + } + + // Tracks layer stacks of displays that are added to CompositionEngine output. + ui::DisplayMap<ui::LayerStack, ftl::Unit> outputLayerStacks; + auto isOutputLayerStack = [&outputLayerStacks](DisplayId id, ui::LayerStack layerStack) { + if (FlagManager::getInstance().reject_dupe_layerstacks() && + outputLayerStacks.contains(layerStack)) { + // TODO: remove log and DisplayId from params once reject_dupe_layerstacks flag is + // removed + ALOGD("Existing layer stack ID %d output to another display %" PRIu64 + ", dropping display from outputs", + layerStack.id, id.value); + return true; + } + outputLayerStacks.try_emplace(layerStack); + return false; + }; + // Add outputs for physical displays. for (const auto& [id, targeter] : frameTargeters) { ftl::FakeGuard guard(mStateLock); if (const auto display = getCompositionDisplayLocked(id)) { - refreshArgs.outputs.push_back(display); + const auto layerStack = physicalDisplayLayerStacks.get(id)->get(); + if (!isOutputLayerStack(display->getId(), layerStack)) { + refreshArgs.outputs.push_back(display); + } } refreshArgs.frameTargets.try_emplace(id, &targeter->target()); @@ -2807,7 +2838,9 @@ CompositeResultsPerDisplay SurfaceFlinger::composite( if (!refreshRate.isValid() || mScheduler->isVsyncInPhase(pacesetterTarget.frameBeginTime(), refreshRate)) { - refreshArgs.outputs.push_back(display->getCompositionDisplay()); + if (!isOutputLayerStack(display->getId(), display->getLayerStack())) { + refreshArgs.outputs.push_back(display->getCompositionDisplay()); + } } } } diff --git a/services/surfaceflinger/common/FlagManager.cpp b/services/surfaceflinger/common/FlagManager.cpp index 5e78426c77..7f3936ba8f 100644 --- a/services/surfaceflinger/common/FlagManager.cpp +++ b/services/surfaceflinger/common/FlagManager.cpp @@ -166,6 +166,7 @@ void FlagManager::dump(std::string& result) const { DUMP_ACONFIG_FLAG(skip_invisible_windows_in_input); DUMP_ACONFIG_FLAG(begone_bright_hlg); DUMP_ACONFIG_FLAG(window_blur_kawase2); + DUMP_ACONFIG_FLAG(reject_dupe_layerstacks); #undef DUMP_ACONFIG_FLAG #undef DUMP_LEGACY_SERVER_FLAG @@ -266,6 +267,7 @@ FLAG_MANAGER_ACONFIG_FLAG(deprecate_frame_tracker, ""); FLAG_MANAGER_ACONFIG_FLAG(skip_invisible_windows_in_input, ""); FLAG_MANAGER_ACONFIG_FLAG(begone_bright_hlg, "debug.sf.begone_bright_hlg"); FLAG_MANAGER_ACONFIG_FLAG(window_blur_kawase2, ""); +FLAG_MANAGER_ACONFIG_FLAG(reject_dupe_layerstacks, ""); /// Trunk stable server (R/W) flags /// FLAG_MANAGER_ACONFIG_FLAG(refresh_rate_overlay_on_external_display, "") diff --git a/services/surfaceflinger/common/include/common/FlagManager.h b/services/surfaceflinger/common/include/common/FlagManager.h index d8887f538f..e42aa68208 100644 --- a/services/surfaceflinger/common/include/common/FlagManager.h +++ b/services/surfaceflinger/common/include/common/FlagManager.h @@ -104,6 +104,7 @@ public: bool begone_bright_hlg() const; bool luts_api() const; bool window_blur_kawase2() const; + bool reject_dupe_layerstacks() const; protected: // overridden for unit tests diff --git a/services/surfaceflinger/surfaceflinger_flags_new.aconfig b/services/surfaceflinger/surfaceflinger_flags_new.aconfig index bdd826d084..b28d2697c5 100644 --- a/services/surfaceflinger/surfaceflinger_flags_new.aconfig +++ b/services/surfaceflinger/surfaceflinger_flags_new.aconfig @@ -217,6 +217,17 @@ flag { } # no_vsyncs_on_screen_off flag { + name: "reject_dupe_layerstacks" + namespace: "window_surfaces" + description: "Reject duplicate layerstacks for displays" + bug: "370358572" + is_fixed_read_only: true + metadata { + purpose: PURPOSE_BUGFIX + } + } # reject_dupe_layerstacks + +flag { name: "single_hop_screenshot" namespace: "window_surfaces" description: "Only access SF main thread once during a screenshot" diff --git a/services/surfaceflinger/tests/Android.bp b/services/surfaceflinger/tests/Android.bp index 4d5c0fd1de..b5f7a74347 100644 --- a/services/surfaceflinger/tests/Android.bp +++ b/services/surfaceflinger/tests/Android.bp @@ -52,7 +52,7 @@ cc_test { "LayerTypeTransaction_test.cpp", "LayerUpdate_test.cpp", "MirrorLayer_test.cpp", - "MultiDisplayLayerBounds_test.cpp", + "MultiDisplay_test.cpp", "RefreshRateOverlay_test.cpp", "RelativeZ_test.cpp", "ReleaseBufferCallback_test.cpp", diff --git a/services/surfaceflinger/tests/MultiDisplayLayerBounds_test.cpp b/services/surfaceflinger/tests/MultiDisplay_test.cpp index 65add63165..54bb253526 100644 --- a/services/surfaceflinger/tests/MultiDisplayLayerBounds_test.cpp +++ b/services/surfaceflinger/tests/MultiDisplay_test.cpp @@ -33,7 +33,7 @@ using android::hardware::graphics::common::V1_1::BufferUsage; ::testing::Environment* const binderEnv = ::testing::AddGlobalTestEnvironment(new BinderEnvironment()); -class MultiDisplayLayerBoundsTest : public LayerTransactionTest { +class MultiDisplayTest : public LayerTransactionTest { protected: virtual void SetUp() { LayerTransactionTest::SetUp(); @@ -105,7 +105,7 @@ protected: Color mExpectedColor = {63, 63, 195, 255}; }; -TEST_F(MultiDisplayLayerBoundsTest, RenderLayerInVirtualDisplay) { +TEST_F(MultiDisplayTest, RenderLayerInVirtualDisplay) { constexpr ui::LayerStack kLayerStack{1u}; createDisplay(mMainDisplayState.layerStackSpaceRect, kLayerStack); createColorLayer(kLayerStack); @@ -124,7 +124,7 @@ TEST_F(MultiDisplayLayerBoundsTest, RenderLayerInVirtualDisplay) { sc->expectColor(Rect(1, 1, 9, 9), {0, 0, 0, 255}); } -TEST_F(MultiDisplayLayerBoundsTest, RenderLayerInMirroredVirtualDisplay) { +TEST_F(MultiDisplayTest, RenderLayerInMirroredVirtualDisplay) { // Create a display and set its layer stack to the main display's layer stack so // the contents of the main display are mirrored on to the virtual display. @@ -150,7 +150,7 @@ TEST_F(MultiDisplayLayerBoundsTest, RenderLayerInMirroredVirtualDisplay) { sc->expectColor(Rect(0, 0, 9, 9), {0, 0, 0, 255}); } -TEST_F(MultiDisplayLayerBoundsTest, RenderLayerWithPromisedFenceInMirroredVirtualDisplay) { +TEST_F(MultiDisplayTest, RenderLayerWithPromisedFenceInMirroredVirtualDisplay) { // Create a display and use a unique layerstack ID for mirrorDisplay() so // the contents of the main display are mirrored on to the virtual display. @@ -181,6 +181,51 @@ TEST_F(MultiDisplayLayerBoundsTest, RenderLayerWithPromisedFenceInMirroredVirtua sc->expectColor(Rect(0, 0, 9, 9), {0, 0, 0, 255}); } +#if COM_ANDROID_GRAPHICS_LIBGUI_FLAGS(WB_CONSUMER_BASE_OWNS_BQ) +TEST_F(MultiDisplayTest, rejectDuplicateLayerStacks) { + if (!FlagManager::getInstance().reject_dupe_layerstacks()) return; + + // Setup + sp<CpuConsumer> cpuConsumer1 = sp<CpuConsumer>::make(static_cast<size_t>(1)); + cpuConsumer1->setName(String8("consumer 1")); + cpuConsumer1->setDefaultBufferSize(100, 100); + sp<IGraphicBufferProducer> cpuProducer1 = + cpuConsumer1->getSurface()->getIGraphicBufferProducer(); + CpuConsumer::LockedBuffer buffer1; + + sp<CpuConsumer> cpuConsumer2 = sp<CpuConsumer>::make(static_cast<size_t>(1)); + cpuConsumer2->setName(String8("consumer 2")); + cpuConsumer2->setDefaultBufferSize(100, 100); + sp<IGraphicBufferProducer> cpuProducer2 = + cpuConsumer2->getSurface()->getIGraphicBufferProducer(); + CpuConsumer::LockedBuffer buffer2; + + SurfaceComposerClient::Transaction t; + constexpr ui::LayerStack layerStack = {123u}; + createColorLayer(layerStack); + + static const std::string kDisplayName1("VirtualDisplay1 - rejectDuplicateLayerStacks"); + sp<IBinder> virtualDisplay1 = + SurfaceComposerClient::createVirtualDisplay(kDisplayName1, false /*isSecure*/); + + t.setDisplaySurface(virtualDisplay1, cpuProducer1); + t.setDisplayLayerStack(virtualDisplay1, layerStack); + t.apply(true); + + static const std::string kDisplayName2("VirtualDisplay2 - rejectDuplicateLayerStacks"); + sp<IBinder> virtualDisplay2 = + SurfaceComposerClient::createVirtualDisplay(kDisplayName2, false /*isSecure*/); + + t.setDisplaySurface(virtualDisplay2, cpuProducer2); + t.setDisplayLayerStack(virtualDisplay2, layerStack); + t.apply(true); + + // The second consumer will not be able to lock a buffer because + // the duplicate layer stack should be rejected. + ASSERT_EQ(NO_ERROR, cpuConsumer1->lockNextBuffer(&buffer1)); + ASSERT_NE(NO_ERROR, cpuConsumer2->lockNextBuffer(&buffer2)); +} +#endif // COM_ANDROID_GRAPHICS_LIBGUI_FLAGS(WB_CONSUMER_BASE_OWNS_BQ) } // namespace android // TODO(b/129481165): remove the #pragma below and fix conversion issues |