From af3627ac2bd26bb077a8cdd03e96834d72c4e72b Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Wed, 28 May 2025 14:28:17 -0700 Subject: [PATCH] [Impeller] acquire the gpu sync switch when flush stored GPU tasks. (#169596) Potential fix for https://github.com/flutter/flutter/issues/166668 See: * https://github.com/flutter/flutter/blob/master/engine/src/flutter/impeller/renderer/backend/metal/context_mtl.mm#L429 * https://github.com/flutter/flutter/blob/5d013c73baa70a8b3e1c541cb63e4c22654aa3cc/engine/src/flutter/fml/synchronization/sync_switch.cc#L41 We don't hold the sync switch lock when we flush tasks. So if we start flushing then immediately go to the background, then we might execute while backgrounded. --- .../flutter/ci/licenses_golden/excluded_files | 1 + .../backend/metal/playground_impl_mtl.h | 5 +- .../backend/metal/playground_impl_mtl.mm | 4 ++ .../flutter/impeller/playground/playground.cc | 4 ++ .../flutter/impeller/playground/playground.h | 5 ++ .../impeller/playground/playground_impl.h | 2 + .../impeller/renderer/backend/metal/BUILD.gn | 1 + .../renderer/backend/metal/context_mtl.h | 5 +- .../renderer/backend/metal/context_mtl.mm | 20 +++++- .../backend/metal/context_mtl_unittests.mm | 64 +++++++++++++++++++ 10 files changed, 107 insertions(+), 4 deletions(-) create mode 100644 engine/src/flutter/impeller/renderer/backend/metal/context_mtl_unittests.mm diff --git a/engine/src/flutter/ci/licenses_golden/excluded_files b/engine/src/flutter/ci/licenses_golden/excluded_files index ac78b4d2afe..6d40f111216 100644 --- a/engine/src/flutter/ci/licenses_golden/excluded_files +++ b/engine/src/flutter/ci/licenses_golden/excluded_files @@ -191,6 +191,7 @@ ../../../flutter/impeller/renderer/backend/gles/test ../../../flutter/impeller/renderer/backend/gles/unique_handle_gles_unittests.cc ../../../flutter/impeller/renderer/backend/metal/allocator_mtl_unittests.mm +../../../flutter/impeller/renderer/backend/metal/context_mtl_unittests.mm ../../../flutter/impeller/renderer/backend/metal/swapchain_transients_mtl_unittests.mm ../../../flutter/impeller/renderer/backend/metal/texture_mtl_unittests.mm ../../../flutter/impeller/renderer/backend/vulkan/allocator_vk_unittests.cc diff --git a/engine/src/flutter/impeller/playground/backend/metal/playground_impl_mtl.h b/engine/src/flutter/impeller/playground/backend/metal/playground_impl_mtl.h index 0935e5c816e..2028a82529b 100644 --- a/engine/src/flutter/impeller/playground/backend/metal/playground_impl_mtl.h +++ b/engine/src/flutter/impeller/playground/backend/metal/playground_impl_mtl.h @@ -38,7 +38,7 @@ class PlaygroundImplMTL final : public PlaygroundImpl { std::shared_ptr context_; std::shared_ptr concurrent_loop_; std::shared_ptr swapchain_transients_; - std::shared_ptr is_gpu_disabled_sync_switch_; + std::shared_ptr is_gpu_disabled_sync_switch_; // |PlaygroundImpl| std::shared_ptr GetContext() const override; @@ -50,6 +50,9 @@ class PlaygroundImplMTL final : public PlaygroundImpl { std::unique_ptr AcquireSurfaceFrame( std::shared_ptr context) override; + // |PlaygroundImpl| + void SetGPUDisabled(bool disabled) const override; + PlaygroundImplMTL(const PlaygroundImplMTL&) = delete; PlaygroundImplMTL& operator=(const PlaygroundImplMTL&) = delete; diff --git a/engine/src/flutter/impeller/playground/backend/metal/playground_impl_mtl.mm b/engine/src/flutter/impeller/playground/backend/metal/playground_impl_mtl.mm index 978f01ab416..a276c11aeaa 100644 --- a/engine/src/flutter/impeller/playground/backend/metal/playground_impl_mtl.mm +++ b/engine/src/flutter/impeller/playground/backend/metal/playground_impl_mtl.mm @@ -138,4 +138,8 @@ fml::Status PlaygroundImplMTL::SetCapabilities( return fml::Status(); } +void PlaygroundImplMTL::SetGPUDisabled(bool disabled) const { + is_gpu_disabled_sync_switch_->SetSwitch(disabled); +} + } // namespace impeller diff --git a/engine/src/flutter/impeller/playground/playground.cc b/engine/src/flutter/impeller/playground/playground.cc index 3889d6d68d1..4b6d786c302 100644 --- a/engine/src/flutter/impeller/playground/playground.cc +++ b/engine/src/flutter/impeller/playground/playground.cc @@ -527,4 +527,8 @@ Playground::VKProcAddressResolver Playground::CreateVKProcAddressResolver() return impl_->CreateVKProcAddressResolver(); } +void Playground::SetGPUDisabled(bool value) const { + impl_->SetGPUDisabled(value); +} + } // namespace impeller diff --git a/engine/src/flutter/impeller/playground/playground.h b/engine/src/flutter/impeller/playground/playground.h index 236e6de89c6..6878a597515 100644 --- a/engine/src/flutter/impeller/playground/playground.h +++ b/engine/src/flutter/impeller/playground/playground.h @@ -122,6 +122,11 @@ class Playground { std::function; VKProcAddressResolver CreateVKProcAddressResolver() const; + /// @brief Mark the GPU as unavilable. + /// + /// Only supported on the Metal backend. + void SetGPUDisabled(bool disabled) const; + protected: const PlaygroundSwitches switches_; diff --git a/engine/src/flutter/impeller/playground/playground_impl.h b/engine/src/flutter/impeller/playground/playground_impl.h index 387a2cb5af4..ed2593fc98f 100644 --- a/engine/src/flutter/impeller/playground/playground_impl.h +++ b/engine/src/flutter/impeller/playground/playground_impl.h @@ -40,6 +40,8 @@ class PlaygroundImpl { virtual Playground::VKProcAddressResolver CreateVKProcAddressResolver() const; + virtual void SetGPUDisabled(bool disabled) const {} + protected: const PlaygroundSwitches switches_; diff --git a/engine/src/flutter/impeller/renderer/backend/metal/BUILD.gn b/engine/src/flutter/impeller/renderer/backend/metal/BUILD.gn index ac4a0004922..9103010bd1d 100644 --- a/engine/src/flutter/impeller/renderer/backend/metal/BUILD.gn +++ b/engine/src/flutter/impeller/renderer/backend/metal/BUILD.gn @@ -70,6 +70,7 @@ impeller_component("metal_unittests") { sources = [ "allocator_mtl_unittests.mm", + "context_mtl_unittests.mm", "swapchain_transients_mtl_unittests.mm", "texture_mtl_unittests.mm", ] diff --git a/engine/src/flutter/impeller/renderer/backend/metal/context_mtl.h b/engine/src/flutter/impeller/renderer/backend/metal/context_mtl.h index 712cf7c1f69..348830d0fe7 100644 --- a/engine/src/flutter/impeller/renderer/backend/metal/context_mtl.h +++ b/engine/src/flutter/impeller/renderer/backend/metal/context_mtl.h @@ -146,6 +146,9 @@ class ContextMTL final : public Context, void StoreTaskForGPU(const fml::closure& task, const fml::closure& failure) override; + // visible for testing. + void FlushTasksAwaitingGPU(); + private: class SyncSwitchObserver : public fml::SyncSwitch::Observer { public: @@ -191,8 +194,6 @@ class ContextMTL final : public Context, std::shared_ptr CreateCommandBufferInQueue( id queue) const; - void FlushTasksAwaitingGPU(); - ContextMTL(const ContextMTL&) = delete; ContextMTL& operator=(const ContextMTL&) = delete; diff --git a/engine/src/flutter/impeller/renderer/backend/metal/context_mtl.mm b/engine/src/flutter/impeller/renderer/backend/metal/context_mtl.mm index b30896f2b5d..b64547b93f2 100644 --- a/engine/src/flutter/impeller/renderer/backend/metal/context_mtl.mm +++ b/engine/src/flutter/impeller/renderer/backend/metal/context_mtl.mm @@ -418,8 +418,26 @@ void ContextMTL::FlushTasksAwaitingGPU() { Lock lock(tasks_awaiting_gpu_mutex_); std::swap(tasks_awaiting_gpu, tasks_awaiting_gpu_); } + std::vector tasks_to_queue; for (const auto& task : tasks_awaiting_gpu) { - task.task(); + is_gpu_disabled_sync_switch_->Execute(fml::SyncSwitch::Handlers() + .SetIfFalse([&] { task.task(); }) + .SetIfTrue([&] { + // Lost access to the GPU + // immediately after it was + // activated. This may happen if + // the app was quickly + // foregrounded/backgrounded + // from a push notification. + // Store the tasks on the + // context again. + tasks_to_queue.push_back(task); + })); + } + if (!tasks_to_queue.empty()) { + Lock lock(tasks_awaiting_gpu_mutex_); + tasks_awaiting_gpu_.insert(tasks_awaiting_gpu_.end(), + tasks_to_queue.begin(), tasks_to_queue.end()); } } diff --git a/engine/src/flutter/impeller/renderer/backend/metal/context_mtl_unittests.mm b/engine/src/flutter/impeller/renderer/backend/metal/context_mtl_unittests.mm new file mode 100644 index 00000000000..cb3ee72e599 --- /dev/null +++ b/engine/src/flutter/impeller/renderer/backend/metal/context_mtl_unittests.mm @@ -0,0 +1,64 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "flutter/testing/testing.h" +#include "impeller/core/device_buffer_descriptor.h" +#include "impeller/core/formats.h" +#include "impeller/core/texture_descriptor.h" +#include "impeller/playground/playground_test.h" +#include "impeller/renderer/backend/metal/allocator_mtl.h" +#include "impeller/renderer/backend/metal/context_mtl.h" +#include "impeller/renderer/backend/metal/formats_mtl.h" +#include "impeller/renderer/backend/metal/texture_mtl.h" +#include "impeller/renderer/capabilities.h" + +#include +#include +#include + +#include "gtest/gtest.h" + +namespace impeller { +namespace testing { + +using ContextMTLTest = PlaygroundTest; +INSTANTIATE_METAL_PLAYGROUND_SUITE(ContextMTLTest); + +TEST_P(ContextMTLTest, FlushTask) { + auto& context_mtl = ContextMTL::Cast(*GetContext()); + + int executed = 0; + int failed = 0; + context_mtl.StoreTaskForGPU([&]() { executed++; }, [&]() { failed++; }); + + context_mtl.FlushTasksAwaitingGPU(); + + EXPECT_EQ(executed, 1); + EXPECT_EQ(failed, 0); +} + +TEST_P(ContextMTLTest, FlushTaskWithGPULoss) { + auto& context_mtl = ContextMTL::Cast(*GetContext()); + + int executed = 0; + int failed = 0; + context_mtl.StoreTaskForGPU([&]() { executed++; }, [&]() { failed++; }); + + // If tasks are flushed while the GPU is disabled, then + // they should not be executed. + SetGPUDisabled(/*disabled=*/true); + context_mtl.FlushTasksAwaitingGPU(); + + EXPECT_EQ(executed, 0); + EXPECT_EQ(failed, 0); + + // Toggling availibility should flush tasks. + SetGPUDisabled(/*disabled=*/false); + + EXPECT_EQ(executed, 1); + EXPECT_EQ(failed, 0); +} + +} // namespace testing +} // namespace impeller