From e0a962c8e30c0c775c7365eb97bce1755575cc48 Mon Sep 17 00:00:00 2001 From: Joen Chen Date: Mon, 28 Aug 2023 09:49:11 +0000 Subject: [PATCH] Reland "Add a DisplayCapability for multi-threaded present" Originally landed as I5dbb01fc23abd5e0108c565f96d25e62c77fc16d and previously reverted in I6265f8de3db31f07506906cee82a91fe3baac0bc due to timeline constraints. Add a test presenting from multiple threads. Split up execute() so that it can be called for a single display. Update MultiThreadedPresent (test) to offload presentDisplay to a separate thread, similar to how Ib9d074671e32c95875ef7e0791dd95d6e595e47a does it, as described in go/multi-threaded-present. Bug: 259132483 Bug: 284156408 Fixes: 251842321 Fixes: 295841597 Test: VtsHalGraphicsComposer3_TargetTest Change-Id: If975ee9bb0b9c6f64ef50401e2aee32f934e3f08 --- .../graphics/composer3/DisplayCapability.aidl | 1 + .../graphics/composer3/DisplayCapability.aidl | 16 ++ .../VtsHalGraphicsComposer3_TargetTest.cpp | 142 ++++++++++++++++-- 3 files changed, 147 insertions(+), 12 deletions(-) diff --git a/graphics/composer/aidl/aidl_api/android.hardware.graphics.composer3/current/android/hardware/graphics/composer3/DisplayCapability.aidl b/graphics/composer/aidl/aidl_api/android.hardware.graphics.composer3/current/android/hardware/graphics/composer3/DisplayCapability.aidl index 6eba887aef..0e2d72bae0 100644 --- a/graphics/composer/aidl/aidl_api/android.hardware.graphics.composer3/current/android/hardware/graphics/composer3/DisplayCapability.aidl +++ b/graphics/composer/aidl/aidl_api/android.hardware.graphics.composer3/current/android/hardware/graphics/composer3/DisplayCapability.aidl @@ -42,4 +42,5 @@ enum DisplayCapability { AUTO_LOW_LATENCY_MODE = 5, SUSPEND = 6, DISPLAY_IDLE_TIMER = 7, + MULTI_THREADED_PRESENT = 8, } diff --git a/graphics/composer/aidl/android/hardware/graphics/composer3/DisplayCapability.aidl b/graphics/composer/aidl/android/hardware/graphics/composer3/DisplayCapability.aidl index f4b29843b8..7154d74499 100644 --- a/graphics/composer/aidl/android/hardware/graphics/composer3/DisplayCapability.aidl +++ b/graphics/composer/aidl/android/hardware/graphics/composer3/DisplayCapability.aidl @@ -80,4 +80,20 @@ enum DisplayCapability { * IComposerCallback.onVsyncIdle. */ DISPLAY_IDLE_TIMER = 7, + /** + * Indicates that both the composer HAL implementation and the given display + * support calling executeCommands concurrently from separate threads. + * executeCommands for a particular display will never run concurrently to + * any other executeCommands for the same display. In addition, the + * CommandResultPayload must only reference displays included in the + * DisplayCommands passed to executeCommands. Displays referenced from + * separate threads must have minimal interference with one another. If a + * HWC-managed display has this capability, SurfaceFlinger can run + * executeCommands for this display concurrently with other displays with the + * same capability. + * @see IComposerClient.executeCommands + * @see DisplayCommand.presentDisplay + * @see DisplayCommand.validateDisplay + */ + MULTI_THREADED_PRESENT = 8, } diff --git a/graphics/composer/aidl/vts/VtsHalGraphicsComposer3_TargetTest.cpp b/graphics/composer/aidl/vts/VtsHalGraphicsComposer3_TargetTest.cpp index 1e6f34b534..67c7b5a9be 100644 --- a/graphics/composer/aidl/vts/VtsHalGraphicsComposer3_TargetTest.cpp +++ b/graphics/composer/aidl/vts/VtsHalGraphicsComposer3_TargetTest.cpp @@ -34,6 +34,7 @@ #include #include #include +#include #include #include #include @@ -1381,21 +1382,17 @@ class GraphicsComposerAidlCommandTest : public GraphicsComposerAidlTest { void execute() { std::vector payloads; for (auto& [_, writer] : mWriters) { - auto commands = writer.takePendingCommands(); - if (commands.empty()) { - continue; - } - - auto [status, results] = mComposerClient->executeCommands(commands); - ASSERT_TRUE(status.isOk()) << "executeCommands failed " << status.getDescription(); - - payloads.reserve(payloads.size() + results.size()); - payloads.insert(payloads.end(), std::make_move_iterator(results.begin()), - std::make_move_iterator(results.end())); + executeInternal(writer, payloads); } mReader.parse(std::move(payloads)); } + void execute(ComposerClientWriter& writer, ComposerClientReader& reader) { + std::vector payloads; + executeInternal(writer, payloads); + reader.parse(std::move(payloads)); + } + static inline auto toTimePoint(nsecs_t time) { return std::chrono::time_point(std::chrono::nanoseconds(time)); } @@ -1720,6 +1717,7 @@ class GraphicsComposerAidlCommandTest : public GraphicsComposerAidlTest { // clang-format on ComposerClientWriter& getWriter(int64_t display) { + std::lock_guard guard{mWritersMutex}; auto [it, _] = mWriters.try_emplace(display, display); return it->second; } @@ -1727,7 +1725,27 @@ class GraphicsComposerAidlCommandTest : public GraphicsComposerAidlTest { ComposerClientReader mReader; private: - std::unordered_map mWriters; + void executeInternal(ComposerClientWriter& writer, + std::vector& payloads) { + auto commands = writer.takePendingCommands(); + if (commands.empty()) { + return; + } + + auto [status, results] = mComposerClient->executeCommands(commands); + ASSERT_TRUE(status.isOk()) << "executeCommands failed " << status.getDescription(); + + payloads.reserve(payloads.size() + results.size()); + payloads.insert(payloads.end(), std::make_move_iterator(results.begin()), + std::make_move_iterator(results.end())); + } + + // Guards access to the map itself. Callers must ensure not to attempt to + // - modify the same writer from multiple threads + // - insert a new writer into the map during concurrent access, which would invalidate + // references from other threads + std::mutex mWritersMutex; + std::unordered_map mWriters GUARDED_BY(mWritersMutex); }; TEST_P(GraphicsComposerAidlCommandTest, SetColorTransform) { @@ -2785,6 +2803,106 @@ TEST_P(GraphicsComposerAidlCommandV2Test, } } +TEST_P(GraphicsComposerAidlCommandTest, MultiThreadedPresent) { + std::vector displays; + for (auto& display : mDisplays) { + if (hasDisplayCapability(display.getDisplayId(), + DisplayCapability::MULTI_THREADED_PRESENT)) { + displays.push_back(&display); + } + } + + const size_t numDisplays = displays.size(); + if (numDisplays <= 1u) { + GTEST_SKIP(); + } + + // When multi-threaded, use a reader per display. As with mWriters, this mutex + // guards access to the map. + std::mutex readersMutex; + std::unordered_map readers; + std::vector threads; + threads.reserve(numDisplays); + + // Each display will have a layer to present. This maps from the display to + // the layer, so we can properly destroy each layer at the end. + std::unordered_map layers; + + for (auto* const display : displays) { + const int64_t displayId = display->getDisplayId(); + + // Ensure that all writers and readers have been added to their respective + // maps initially, so that the following loop never modifies the maps. The + // maps are accessed from different threads, and if the maps were modified, + // this would invalidate their iterators, and therefore references to the + // writers and readers. + auto& writer = getWriter(displayId); + { + std::lock_guard guard{readersMutex}; + readers.try_emplace(displayId, displayId); + } + + EXPECT_TRUE(mComposerClient->setPowerMode(displayId, PowerMode::ON).isOk()); + + const auto& [status, layer] = mComposerClient->createLayer(displayId, kBufferSlotCount); + const auto buffer = allocate(::android::PIXEL_FORMAT_RGBA_8888); + ASSERT_NE(nullptr, buffer); + ASSERT_EQ(::android::OK, buffer->initCheck()); + ASSERT_NE(nullptr, buffer->handle); + + configureLayer(*display, layer, Composition::DEVICE, display->getFrameRect(), + display->getCrop()); + writer.setLayerBuffer(displayId, layer, /*slot*/ 0, buffer->handle, + /*acquireFence*/ -1); + writer.setLayerDataspace(displayId, layer, common::Dataspace::UNKNOWN); + layers.try_emplace(displayId, layer); + } + + for (auto* const display : displays) { + const int64_t displayId = display->getDisplayId(); + auto& writer = getWriter(displayId); + std::unique_lock lock{readersMutex}; + auto& reader = readers.at(displayId); + lock.unlock(); + + writer.validateDisplay(displayId, ComposerClientWriter::kNoTimestamp); + execute(writer, reader); + + threads.emplace_back([this, displayId, &readers, &readersMutex]() { + auto& writer = getWriter(displayId); + std::unique_lock lock{readersMutex}; + ComposerClientReader& reader = readers.at(displayId); + lock.unlock(); + + writer.presentDisplay(displayId); + execute(writer, reader); + ASSERT_TRUE(reader.takeErrors().empty()); + + auto presentFence = reader.takePresentFence(displayId); + // take ownership + const int fenceOwner = presentFence.get(); + *presentFence.getR() = -1; + EXPECT_NE(-1, fenceOwner); + const auto presentFence2 = sp<::android::Fence>::make(fenceOwner); + presentFence2->waitForever(LOG_TAG); + }); + } + + for (auto& thread : threads) { + thread.join(); + } + + for (auto& [displayId, layer] : layers) { + EXPECT_TRUE(mComposerClient->destroyLayer(displayId, layer).isOk()); + } + + std::lock_guard guard{readersMutex}; + for (auto& [displayId, reader] : readers) { + ASSERT_TRUE(reader.takeErrors().empty()); + ASSERT_TRUE(reader.takeChangedCompositionTypes(displayId).empty()); + } +} + GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST(GraphicsComposerAidlCommandTest); INSTANTIATE_TEST_SUITE_P( PerInstance, GraphicsComposerAidlCommandTest,