From 9549d05062d390afac44b9a509b2cf99059c46ad Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 12 Sep 2023 12:30:29 -0700 Subject: [PATCH 1/3] Added test to assert the vulkan embedder threadsafe vkqueue usage --- shell/platform/embedder/BUILD.gn | 4 + shell/platform/embedder/embedder.h | 9 ++ .../embedder/tests/embedder_config_builder.cc | 20 +-- .../embedder/tests/embedder_config_builder.h | 5 +- .../tests/embedder_test_context_vulkan.cc | 10 ++ .../tests/embedder_test_context_vulkan.h | 4 + .../embedder/tests/embedder_vk_unittests.cc | 123 ++++++++++++++++++ 7 files changed, 165 insertions(+), 10 deletions(-) create mode 100644 shell/platform/embedder/tests/embedder_vk_unittests.cc diff --git a/shell/platform/embedder/BUILD.gn b/shell/platform/embedder/BUILD.gn index 74ff35fd7b02a..90f3d1d9c66ca 100644 --- a/shell/platform/embedder/BUILD.gn +++ b/shell/platform/embedder/BUILD.gn @@ -361,6 +361,10 @@ if (enable_unittests) { if (test_enable_metal) { sources += [ "tests/embedder_metal_unittests.mm" ] } + + if (test_enable_vulkan) { + sources += [ "tests/embedder_vk_unittests.cc" ] + } } executable("embedder_a11y_unittests") { diff --git a/shell/platform/embedder/embedder.h b/shell/platform/embedder/embedder.h index dbbb346dd8758..bbb89eced970c 100644 --- a/shell/platform/embedder/embedder.h +++ b/shell/platform/embedder/embedder.h @@ -757,6 +757,9 @@ typedef struct { /// The queue family index of the VkQueue supplied in the next field. uint32_t queue_family_index; /// VkQueue handle. + /// The queue should not be used without protection from a mutex to make sure + /// it is not used simultaneously with other threads. That mutex should match + /// the one injected via the |get_instance_proc_address_callback|. FlutterVulkanQueueHandle queue; /// The number of instance extensions available for enumerating in the next /// field. @@ -780,6 +783,12 @@ typedef struct { /// For example: VK_KHR_GET_MEMORY_REQUIREMENTS_2_EXTENSION_NAME const char** enabled_device_extensions; /// The callback invoked when resolving Vulkan function pointers. + /// At a bare minimum this should be used to swap out any calls that operate + /// on vkQueue's for threadsafe variants that obtain locks for their duration. + /// The functions to swap out are "vkQueueSubmit" and "vkQueueWaitIdle". An + /// example of how to do that can be found in the test + /// "EmbedderTest.CanSwapOutVulkanCalls" unit-test in + /// //shell/platform/embedder/tests/embedder_vk_unittests.cc. FlutterVulkanInstanceProcAddressCallback get_instance_proc_address_callback; /// The callback invoked when the engine requests a VkImage from the embedder /// for rendering the next frame. diff --git a/shell/platform/embedder/tests/embedder_config_builder.cc b/shell/platform/embedder/tests/embedder_config_builder.cc index 48b8ba3686a49..03844fffd3971 100644 --- a/shell/platform/embedder/tests/embedder_config_builder.cc +++ b/shell/platform/embedder/tests/embedder_config_builder.cc @@ -205,10 +205,18 @@ void EmbedderConfigBuilder::SetMetalRendererConfig(SkISize surface_size) { #endif } -void EmbedderConfigBuilder::SetVulkanRendererConfig(SkISize surface_size) { +void EmbedderConfigBuilder::SetVulkanRendererConfig( + SkISize surface_size, + std::optional + instance_proc_address_callback) { #ifdef SHELL_ENABLE_VULKAN renderer_config_.type = FlutterRendererType::kVulkan; - renderer_config_.vulkan = vulkan_renderer_config_; + FlutterVulkanRendererConfig vulkan_renderer_config = vulkan_renderer_config_; + if (instance_proc_address_callback.has_value()) { + vulkan_renderer_config.get_instance_proc_address_callback = + instance_proc_address_callback.value(); + } + renderer_config_.vulkan = vulkan_renderer_config; context_.SetupSurface(surface_size); #endif } @@ -519,13 +527,7 @@ void EmbedderConfigBuilder::InitializeVulkanRendererConfig() { static_cast(context_) .vulkan_context_->device_->GetQueueHandle(); vulkan_renderer_config_.get_instance_proc_address_callback = - [](void* context, FlutterVulkanInstanceHandle instance, - const char* name) -> void* { - auto proc_addr = reinterpret_cast(context) - ->vulkan_context_->vk_->GetInstanceProcAddr( - reinterpret_cast(instance), name); - return reinterpret_cast(proc_addr); - }; + EmbedderTestContextVulkan::InstanceProcAddr; vulkan_renderer_config_.get_next_image_callback = [](void* context, const FlutterFrameInfo* frame_info) -> FlutterVulkanImage { diff --git a/shell/platform/embedder/tests/embedder_config_builder.h b/shell/platform/embedder/tests/embedder_config_builder.h index dfa9ce4908f08..a79cedf812b2f 100644 --- a/shell/platform/embedder/tests/embedder_config_builder.h +++ b/shell/platform/embedder/tests/embedder_config_builder.h @@ -53,7 +53,10 @@ class EmbedderConfigBuilder { void SetMetalRendererConfig(SkISize surface_size); - void SetVulkanRendererConfig(SkISize surface_size); + void SetVulkanRendererConfig( + SkISize surface_size, + std::optional + instance_proc_address_callback = {}); // Used to explicitly set an `open_gl.fbo_callback`. Using this method will // cause your test to fail since the ctor for this class sets diff --git a/shell/platform/embedder/tests/embedder_test_context_vulkan.cc b/shell/platform/embedder/tests/embedder_test_context_vulkan.cc index c5f821a8088cc..4f3678597dda1 100644 --- a/shell/platform/embedder/tests/embedder_test_context_vulkan.cc +++ b/shell/platform/embedder/tests/embedder_test_context_vulkan.cc @@ -58,5 +58,15 @@ void EmbedderTestContextVulkan::SetupCompositor() { surface_size_, vulkan_context_->GetGrDirectContext()); } +void* EmbedderTestContextVulkan::InstanceProcAddr( + void* user_data, + FlutterVulkanInstanceHandle instance, + const char* name) { + auto proc_addr = reinterpret_cast(user_data) + ->vulkan_context_->vk_->GetInstanceProcAddr( + reinterpret_cast(instance), name); + return reinterpret_cast(proc_addr); +} + } // namespace testing } // namespace flutter diff --git a/shell/platform/embedder/tests/embedder_test_context_vulkan.h b/shell/platform/embedder/tests/embedder_test_context_vulkan.h index b08aa43941708..0a66b86528e09 100644 --- a/shell/platform/embedder/tests/embedder_test_context_vulkan.h +++ b/shell/platform/embedder/tests/embedder_test_context_vulkan.h @@ -33,6 +33,10 @@ class EmbedderTestContextVulkan : public EmbedderTestContext { bool PresentImage(VkImage image); + static void* InstanceProcAddr(void* user_data, + FlutterVulkanInstanceHandle instance, + const char* name); + private: std::unique_ptr surface_; diff --git a/shell/platform/embedder/tests/embedder_vk_unittests.cc b/shell/platform/embedder/tests/embedder_vk_unittests.cc new file mode 100644 index 0000000000000..eb27e07d72a93 --- /dev/null +++ b/shell/platform/embedder/tests/embedder_vk_unittests.cc @@ -0,0 +1,123 @@ +// 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. + +#define FML_USED_ON_EMBEDDER + +#include +#include +#include +#include + +#include "embedder.h" +#include "embedder_engine.h" +#include "flutter/fml/synchronization/count_down_latch.h" +#include "flutter/shell/platform/embedder/tests/embedder_config_builder.h" +#include "flutter/shell/platform/embedder/tests/embedder_test.h" +#include "flutter/shell/platform/embedder/tests/embedder_test_context_vulkan.h" +#include "flutter/shell/platform/embedder/tests/embedder_unittests_util.h" +#include "flutter/testing/testing.h" + +// CREATE_NATIVE_ENTRY is leaky by design +// NOLINTBEGIN(clang-analyzer-core.StackAddressEscape) + +namespace flutter { +namespace testing { + +using EmbedderTest = testing::EmbedderTest; + +//////////////////////////////////////////////////////////////////////////////// +// Notice: Other Vulkan unit tests exist in embedder_gl_unittests.cc. +// See https://github.com/flutter/flutter/issues/134322 +//////////////////////////////////////////////////////////////////////////////// + +namespace { + +struct VulkanProcInfo { + decltype(vkGetInstanceProcAddr)* get_instance_proc_addr = nullptr; + decltype(vkGetDeviceProcAddr)* get_device_proc_addr = nullptr; + decltype(vkQueueSubmit)* queue_submit_proc_addr = nullptr; + bool did_call_queue_submit = false; +}; + +static_assert(std::is_trivially_destructible_v); + +VulkanProcInfo g_vulkan_proc_info; + +VkResult QueueSubmit(VkQueue queue, + uint32_t submitCount, + const VkSubmitInfo* pSubmits, + VkFence fence) { + FML_DCHECK(g_vulkan_proc_info.queue_submit_proc_addr != nullptr); + g_vulkan_proc_info.did_call_queue_submit = true; + return g_vulkan_proc_info.queue_submit_proc_addr(queue, submitCount, pSubmits, + fence); +} + +PFN_vkVoidFunction GetDeviceProcAddr(VkDevice device, const char* pName) { + FML_DCHECK(g_vulkan_proc_info.get_device_proc_addr != nullptr); + if (strcmp(pName, "vkQueueSubmit") == 0) { + g_vulkan_proc_info.queue_submit_proc_addr = + reinterpret_cast( + g_vulkan_proc_info.get_device_proc_addr(device, pName)); + return reinterpret_cast(QueueSubmit); + } + return g_vulkan_proc_info.get_device_proc_addr(device, pName); +} + +PFN_vkVoidFunction GetInstanceProcAddr(VkInstance instance, const char* pName) { + FML_DCHECK(g_vulkan_proc_info.get_instance_proc_addr != nullptr); + if (strcmp(pName, "vkGetDeviceProcAddr") == 0) { + g_vulkan_proc_info.get_device_proc_addr = + reinterpret_cast( + g_vulkan_proc_info.get_instance_proc_addr(instance, pName)); + return reinterpret_cast(GetDeviceProcAddr); + } + return g_vulkan_proc_info.get_instance_proc_addr(instance, pName); +} + +template +struct CheckSameSignature : std::false_type {}; + +template +struct CheckSameSignature : std::true_type {}; + +static_assert(CheckSameSignature::value); +static_assert(CheckSameSignature::value); +static_assert( + CheckSameSignature::value); +} // namespace + +TEST_F(EmbedderTest, CanSwapOutVulkanCalls) { + auto& context = GetEmbedderContext(EmbedderTestContextType::kVulkanContext); + fml::AutoResetWaitableEvent latch; + context.AddIsolateCreateCallback([&latch]() { latch.Signal(); }); + EmbedderConfigBuilder builder(context); + builder.SetVulkanRendererConfig( + SkISize::Make(1024, 1024), + [](void* user_data, FlutterVulkanInstanceHandle instance, + const char* name) -> void* { + if (strcmp(name, "vkGetInstanceProcAddr") == 0) { + g_vulkan_proc_info.get_instance_proc_addr = + reinterpret_cast( + EmbedderTestContextVulkan::InstanceProcAddr(user_data, + instance, name)); + return reinterpret_cast(GetInstanceProcAddr); + } + return EmbedderTestContextVulkan::InstanceProcAddr(user_data, instance, + name); + }); + auto engine = builder.LaunchEngine(); + ASSERT_TRUE(engine.is_valid()); + // Wait for the root isolate to launch. + latch.Wait(); + engine.reset(); + EXPECT_TRUE(g_vulkan_proc_info.did_call_queue_submit); +} + +} // namespace testing +} // namespace flutter + +// NOLINTEND(clang-analyzer-core.StackAddressEscape) From aad78307f51ca2197eb2d93ccd589065b7323373 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 12 Sep 2023 14:03:31 -0700 Subject: [PATCH 2/3] added link to an issue --- shell/platform/embedder/embedder.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/shell/platform/embedder/embedder.h b/shell/platform/embedder/embedder.h index bbb89eced970c..cfef518dcf6fe 100644 --- a/shell/platform/embedder/embedder.h +++ b/shell/platform/embedder/embedder.h @@ -760,6 +760,8 @@ typedef struct { /// The queue should not be used without protection from a mutex to make sure /// it is not used simultaneously with other threads. That mutex should match /// the one injected via the |get_instance_proc_address_callback|. + /// There is a proposal to remove the need for the mutex at + /// https://github.com/flutter/flutter/issues/134573. FlutterVulkanQueueHandle queue; /// The number of instance extensions available for enumerating in the next /// field. From be68d0e2fc04d4bed0ca5f2204387ce909f8c87a Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 12 Sep 2023 14:19:09 -0700 Subject: [PATCH 3/3] switched to strncmp --- .../platform/embedder/tests/embedder_vk_unittests.cc | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/shell/platform/embedder/tests/embedder_vk_unittests.cc b/shell/platform/embedder/tests/embedder_vk_unittests.cc index eb27e07d72a93..d6209d2ed96d0 100644 --- a/shell/platform/embedder/tests/embedder_vk_unittests.cc +++ b/shell/platform/embedder/tests/embedder_vk_unittests.cc @@ -54,9 +54,14 @@ VkResult QueueSubmit(VkQueue queue, fence); } +template +int StrcmpFixed(const char* str1, const char (&str2)[N]) { + return strncmp(str1, str2, N - 1); +} + PFN_vkVoidFunction GetDeviceProcAddr(VkDevice device, const char* pName) { FML_DCHECK(g_vulkan_proc_info.get_device_proc_addr != nullptr); - if (strcmp(pName, "vkQueueSubmit") == 0) { + if (StrcmpFixed(pName, "vkQueueSubmit") == 0) { g_vulkan_proc_info.queue_submit_proc_addr = reinterpret_cast( g_vulkan_proc_info.get_device_proc_addr(device, pName)); @@ -67,7 +72,7 @@ PFN_vkVoidFunction GetDeviceProcAddr(VkDevice device, const char* pName) { PFN_vkVoidFunction GetInstanceProcAddr(VkInstance instance, const char* pName) { FML_DCHECK(g_vulkan_proc_info.get_instance_proc_addr != nullptr); - if (strcmp(pName, "vkGetDeviceProcAddr") == 0) { + if (StrcmpFixed(pName, "vkGetDeviceProcAddr") == 0) { g_vulkan_proc_info.get_device_proc_addr = reinterpret_cast( g_vulkan_proc_info.get_instance_proc_addr(instance, pName)); @@ -99,7 +104,7 @@ TEST_F(EmbedderTest, CanSwapOutVulkanCalls) { SkISize::Make(1024, 1024), [](void* user_data, FlutterVulkanInstanceHandle instance, const char* name) -> void* { - if (strcmp(name, "vkGetInstanceProcAddr") == 0) { + if (StrcmpFixed(name, "vkGetInstanceProcAddr") == 0) { g_vulkan_proc_info.get_instance_proc_addr = reinterpret_cast( EmbedderTestContextVulkan::InstanceProcAddr(user_data,