summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
author Melody Hsu <melodymhsu@google.com> 2024-11-07 00:05:03 +0000
committer Melody Hsu <melodymhsu@google.com> 2024-12-09 18:04:46 +0000
commit1e431a4bbe407c86120420fc54321aa038c23504 (patch)
tree973e9cdf6206c4e3430446dd9984f6e0d09e5334
parentcb47c6c92cdcdc140400b9de1c946d628960a267 (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.cpp37
-rw-r--r--services/surfaceflinger/common/FlagManager.cpp2
-rw-r--r--services/surfaceflinger/common/include/common/FlagManager.h1
-rw-r--r--services/surfaceflinger/surfaceflinger_flags_new.aconfig11
-rw-r--r--services/surfaceflinger/tests/Android.bp2
-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