From 932b7794ac8613eaca9b570457a3af500333f412 Mon Sep 17 00:00:00 2001 From: Charles Giessen Date: Mon, 9 Jan 2023 15:15:10 -0700 Subject: [PATCH] Refactor how GetPhysDevFeats2 works Cleaned up the logic for handling when to use the core, the KHR, and when not to use vkGetPhysicalDeviceFeatures2. This should make the logic easier to follow with better names, and handle a few cases that were wrong, like not caring if the extension is enabled by vulkan 1.1 is not. --- src/VkBootstrap.cpp | 69 ++++++++++++++++++++------------------- src/VkBootstrap.h | 11 ++++--- tests/bootstrap_tests.cpp | 55 +++++++++++++++++++++++++++---- 3 files changed, 90 insertions(+), 45 deletions(-) diff --git a/src/VkBootstrap.cpp b/src/VkBootstrap.cpp index e33f8a8..0b5b4e6 100644 --- a/src/VkBootstrap.cpp +++ b/src/VkBootstrap.cpp @@ -587,11 +587,11 @@ Result InstanceBuilder::build() const { if (info.debug_callback != nullptr && system.debug_utils_available) { extensions.push_back(VK_EXT_DEBUG_UTILS_EXTENSION_NAME); } - bool supports_properties2_ext = - detail::check_extension_supported(system.available_extensions, VK_KHR_GET_PHYSICAL_DEVICE_PROPERTIES_2_EXTENSION_NAME); - - if (supports_properties2_ext && api_version < VKB_VK_API_VERSION_1_1) { + bool properties2_ext_enabled = false; + if (detail::check_extension_supported(system.available_extensions, VK_KHR_GET_PHYSICAL_DEVICE_PROPERTIES_2_EXTENSION_NAME) && + api_version < VKB_VK_API_VERSION_1_1) { extensions.push_back(VK_KHR_GET_PHYSICAL_DEVICE_PROPERTIES_2_EXTENSION_NAME); + properties2_ext_enabled = true; } #if defined(VK_KHR_portability_enumeration) @@ -717,7 +717,7 @@ Result InstanceBuilder::build() const { } instance.headless = info.headless_context; - instance.supports_properties2_ext = supports_properties2_ext; + instance.properties2_ext_enabled = properties2_ext_enabled; instance.allocation_callbacks = info.allocation_callbacks; instance.instance_version = instance_version; instance.api_version = api_version; @@ -1010,16 +1010,14 @@ PhysicalDevice PhysicalDeviceSelector::populate_device_details(VkPhysicalDevice for (const auto& ext : available_extensions) { physical_device.extensions.push_back(&ext.extensionName[0]); } - -#if defined(VKB_VK_API_VERSION_1_1) - physical_device.features2.sType = VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_FEATURES_2; -#else - physical_device.features2.sType = VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_FEATURES_2_KHR; +#if defined(VK_KHR_get_physical_device_properties2) + physical_device.features2.sType = VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_FEATURES_2_KHR; // same value as the non-KHR version #endif + physical_device.properties2_ext_enabled = instance_info.properties2_ext_enabled; auto fill_chain = src_extended_features_chain; - if (!fill_chain.empty() && (instance_info.version >= VKB_VK_API_VERSION_1_1 || instance_info.supports_properties2_ext)) { + if (!fill_chain.empty() && (instance_info.version >= VKB_VK_API_VERSION_1_1 || instance_info.properties2_ext_enabled)) { detail::GenericFeaturesPNextNode* prev = nullptr; for (auto& extension : fill_chain) { @@ -1029,25 +1027,28 @@ PhysicalDevice PhysicalDeviceSelector::populate_device_details(VkPhysicalDevice prev = &extension; } + bool phys_dev_is_1_1 = instance_info.version >= VKB_VK_API_VERSION_1_1 && + physical_device.properties.apiVersion >= VKB_VK_API_VERSION_1_1; + #if defined(VKB_VK_API_VERSION_1_1) - if (instance_info.version >= VKB_VK_API_VERSION_1_1 && physical_device.properties.apiVersion >= VKB_VK_API_VERSION_1_1) { + if (phys_dev_is_1_1 || instance_info.properties2_ext_enabled) { VkPhysicalDeviceFeatures2 local_features{}; - local_features.sType = VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_FEATURES_2; + local_features.sType = VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_FEATURES_2; // KHR is same as core here local_features.pNext = &fill_chain.front(); - detail::vulkan_functions().fp_vkGetPhysicalDeviceFeatures2(vk_phys_device, &local_features); - } else if (instance_info.supports_properties2_ext) { + // Use KHR function if not able to use the core function + if (!phys_dev_is_1_1) { + detail::vulkan_functions().fp_vkGetPhysicalDeviceFeatures2KHR(vk_phys_device, &local_features); + } else { + detail::vulkan_functions().fp_vkGetPhysicalDeviceFeatures2(vk_phys_device, &local_features); + } + } +#elif defined(VK_KHR_get_physical_device_properties2) + if (instance_info.properties2_ext_enabled) { VkPhysicalDeviceFeatures2KHR local_features_khr{}; local_features_khr.sType = VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_FEATURES_2_KHR; local_features_khr.pNext = &fill_chain.front(); detail::vulkan_functions().fp_vkGetPhysicalDeviceFeatures2KHR(vk_phys_device, &local_features_khr); } -#else - VkPhysicalDeviceFeatures2KHR local_features_khr{}; - local_features_khr.sType = VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_FEATURES_2_KHR; - local_features_khr.pNext = &fill_chain.front(); - if (instance_info.supports_properties2_ext) { - detail::vulkan_functions().fp_vkGetPhysicalDeviceFeatures2KHR(vk_phys_device, &local_features_khr); - } #endif physical_device.extended_features_chain = fill_chain; } @@ -1135,7 +1136,7 @@ PhysicalDeviceSelector::PhysicalDeviceSelector(Instance const& instance) PhysicalDeviceSelector::PhysicalDeviceSelector(Instance const& instance, VkSurfaceKHR surface) { instance_info.instance = instance.instance; instance_info.version = instance.instance_version; - instance_info.supports_properties2_ext = instance.supports_properties2_ext; + instance_info.properties2_ext_enabled = instance.properties2_ext_enabled; instance_info.surface = surface; criteria.require_present = !instance.headless; criteria.required_version = instance.api_version; @@ -1492,12 +1493,11 @@ Result DeviceBuilder::build() const { if (physical_device.surface != VK_NULL_HANDLE || physical_device.defer_surface_initialization) extensions.push_back({ VK_KHR_SWAPCHAIN_EXTENSION_NAME }); - bool has_phys_dev_features_2 = false; - bool user_defined_phys_dev_features_2 = false; std::vector final_pnext_chain; VkDeviceCreateInfo device_create_info = {}; -#if defined(VKB_VK_API_VERSION_1_1) +#if defined(VK_KHR_get_physical_device_properties2) + bool user_defined_phys_dev_features_2 = false; for (auto& pnext : info.pNext_chain) { if (pnext->sType == VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_FEATURES_2) { user_defined_phys_dev_features_2 = true; @@ -1505,25 +1505,26 @@ Result DeviceBuilder::build() const { } } + if (user_defined_phys_dev_features_2 && physical_device.extended_features_chain.size() > 0) { + return { DeviceError::VkPhysicalDeviceFeatures2_in_pNext_chain_while_using_add_required_extension_features }; + } + + // These objects must be alive during the call to vkCreateDevice auto physical_device_extension_features_copy = physical_device.extended_features_chain; VkPhysicalDeviceFeatures2 local_features2{}; local_features2.sType = VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_FEATURES_2; if (!user_defined_phys_dev_features_2) { - if (physical_device.instance_version >= VKB_VK_API_VERSION_1_1) { + if (physical_device.instance_version >= VKB_VK_API_VERSION_1_1 || physical_device.properties2_ext_enabled) { local_features2.features = physical_device.features; final_pnext_chain.push_back(reinterpret_cast(&local_features2)); - has_phys_dev_features_2 = true; for (auto& features_node : physical_device_extension_features_copy) { final_pnext_chain.push_back(reinterpret_cast(&features_node)); } + } else { + // Only set device_create_info.pEnabledFeatures when the pNext chain does not contain a VkPhysicalDeviceFeatures2 structure + device_create_info.pEnabledFeatures = &physical_device.features; } - } else { - return { DeviceError::VkPhysicalDeviceFeatures2_in_pNext_chain_while_using_add_required_extension_features }; - } - - if (!user_defined_phys_dev_features_2 && !has_phys_dev_features_2) { - device_create_info.pEnabledFeatures = &physical_device.features; } #endif diff --git a/src/VkBootstrap.h b/src/VkBootstrap.h index f03294f..908961a 100644 --- a/src/VkBootstrap.h +++ b/src/VkBootstrap.h @@ -288,7 +288,7 @@ struct Instance { private: bool headless = false; - bool supports_properties2_ext = false; + bool properties2_ext_enabled = false; uint32_t instance_version = VKB_VK_API_VERSION_1_0; uint32_t api_version = VKB_VK_API_VERSION_1_0; @@ -507,6 +507,7 @@ struct PhysicalDevice { VkPhysicalDeviceFeatures2KHR features2{}; #endif bool defer_surface_initialization = false; + bool properties2_ext_enabled = false; enum class Suitable { yes, partial, no }; Suitable suitable = Suitable::yes; friend class PhysicalDeviceSelector; @@ -633,7 +634,7 @@ class PhysicalDeviceSelector { VkSurfaceKHR surface = VK_NULL_HANDLE; uint32_t version = VKB_VK_API_VERSION_1_0; bool headless = false; - bool supports_properties2_ext = false; + bool properties2_ext_enabled = false; } instance_info; // We copy the extension features stored in the selector criteria under the prose of a @@ -658,10 +659,10 @@ class PhysicalDeviceSelector { uint32_t desired_version = VKB_VK_API_VERSION_1_0; VkPhysicalDeviceFeatures required_features{}; -#if defined(VKB_VK_API_VERSION_1_1) - VkPhysicalDeviceFeatures2 required_features2{}; - std::vector extended_features_chain; +#if defined(VK_KHR_get_physical_device_properties2) + VkPhysicalDeviceFeatures2KHR required_features2{}; #endif + std::vector extended_features_chain; bool defer_surface_initialization = false; bool use_first_gpu_unconditionally = false; bool enable_portability_subset = true; diff --git a/tests/bootstrap_tests.cpp b/tests/bootstrap_tests.cpp index 30bf10f..829c5b4 100644 --- a/tests/bootstrap_tests.cpp +++ b/tests/bootstrap_tests.cpp @@ -566,8 +566,7 @@ TEST_CASE("Passing vkb classes to Vulkan handles", "[VkBootstrap.pass_class_to_h TEST_CASE("Querying Required Extension Features in 1.1", "[VkBootstrap.version]") { GIVEN("A working instance") { auto instance = get_headless_instance(); - // Requires a device that supports runtime descriptor arrays via descriptor indexing extension. - { + SECTION("Requires a device that supports runtime descriptor arrays via descriptor indexing extension.") { VkPhysicalDeviceDescriptorIndexingFeaturesEXT descriptor_indexing_features{}; descriptor_indexing_features.sType = VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_DESCRIPTOR_INDEXING_FEATURES_EXT; descriptor_indexing_features.runtimeDescriptorArray = true; @@ -585,6 +584,52 @@ TEST_CASE("Querying Required Extension Features in 1.1", "[VkBootstrap.version]" REQUIRE(device_ret.has_value()); vkb::destroy_device(device_ret.value()); } + SECTION("Add a custom VkPhysicalDeviceFeatures2 pNext chain to the DeviceBuilder, without using " + "add_required_extension_features()") { + VkPhysicalDeviceDescriptorIndexingFeaturesEXT descriptor_indexing_features{}; + descriptor_indexing_features.sType = VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_DESCRIPTOR_INDEXING_FEATURES_EXT; + descriptor_indexing_features.runtimeDescriptorArray = true; + + vkb::PhysicalDeviceSelector selector(instance); + auto phys_dev_ret = selector.add_required_extension(VK_EXT_DESCRIPTOR_INDEXING_EXTENSION_NAME) + .add_required_extension(VK_KHR_MAINTENANCE3_EXTENSION_NAME) + .select(); + // Ignore if hardware support isn't true + REQUIRE(phys_dev_ret.has_value()); + + VkPhysicalDeviceFeatures2 phys_dev_feats2{}; + phys_dev_feats2.sType = VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_FEATURES_2; + phys_dev_feats2.pNext = reinterpret_cast(&descriptor_indexing_features); + + vkb::DeviceBuilder device_builder(phys_dev_ret.value()); + auto device_ret = device_builder.add_pNext(&phys_dev_feats2).build(); + REQUIRE(device_ret.has_value()); + vkb::destroy_device(device_ret.value()); + } + SECTION( + "Try to add a custom VkPhysicalDeviceFeatures2 pNext chain to the DeviceBuilder, resulting in an error") { + VkPhysicalDeviceDescriptorIndexingFeaturesEXT descriptor_indexing_features{}; + descriptor_indexing_features.sType = VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_DESCRIPTOR_INDEXING_FEATURES_EXT; + descriptor_indexing_features.runtimeDescriptorArray = true; + + vkb::PhysicalDeviceSelector selector(instance); + auto phys_dev_ret = selector.add_required_extension(VK_EXT_DESCRIPTOR_INDEXING_EXTENSION_NAME) + .add_required_extension(VK_KHR_MAINTENANCE3_EXTENSION_NAME) + .add_required_extension_features(descriptor_indexing_features) + .select(); + // Ignore if hardware support isn't true + REQUIRE(phys_dev_ret.has_value()); + + VkPhysicalDeviceFeatures2 phys_dev_feats2{}; + phys_dev_feats2.sType = VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_FEATURES_2; + phys_dev_feats2.pNext = reinterpret_cast(&descriptor_indexing_features); + + vkb::DeviceBuilder device_builder(phys_dev_ret.value()); + auto device_ret = device_builder.add_pNext(&phys_dev_feats2).build(); + REQUIRE(device_ret.error() == + vkb::DeviceError::VkPhysicalDeviceFeatures2_in_pNext_chain_while_using_add_required_extension_features); + } + vkb::destroy_instance(instance); } } @@ -595,8 +640,7 @@ TEST_CASE("Querying Vulkan 1.1 and 1.2 features", "[VkBootstrap.version]") { GIVEN("A working instance") { vkb::InstanceBuilder builder; auto instance = get_headless_instance(2); // make sure we use 1.2 - // Requires a device that supports multiview and bufferDeviceAddress - { + SECTION("Requires a device that supports multiview and bufferDeviceAddress") { VkPhysicalDeviceVulkan11Features features_11{}; features_11.sType = VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_VULKAN_1_1_FEATURES; features_11.multiview = true; @@ -614,8 +658,7 @@ TEST_CASE("Querying Vulkan 1.1 and 1.2 features", "[VkBootstrap.version]") { REQUIRE(device_ret.has_value()); vkb::destroy_device(device_ret.value()); } - // protectedMemory should NOT be supported - { + SECTION("protectedMemory should NOT be supported") { VkPhysicalDeviceVulkan11Features features_11{}; features_11.sType = VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_VULKAN_1_1_FEATURES; features_11.protectedMemory = true;