From 5c9f7af77d0e66f96685cb46338abaf35a83ee3a Mon Sep 17 00:00:00 2001 From: Corentin Wallez Date: Fri, 12 Jun 2020 08:19:31 +0000 Subject: [PATCH] Vulkan: Properly handle Device extension dependencies The checks for dependencies of Device extensions were incomplete, makes sure the transitive dependencies of the extensions we care about are all known so they can participate in the dependency check. Also removes a workaround for surprising Vulkan behavior with instance extensions getting promoted as device functions. This should be handled correctly now as DeviceExt contains the physical device extensions as well. Bug: dawn:457 Change-Id: I4b79729d809c9edfedcb075a0e6aa5b4dd473ab3 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/22942 Commit-Queue: Corentin Wallez Reviewed-by: Austin Eng --- src/dawn_native/vulkan/BackendVk.cpp | 14 ++-- src/dawn_native/vulkan/VulkanExtensions.cpp | 85 +++++++++++++-------- src/dawn_native/vulkan/VulkanExtensions.h | 8 ++ 3 files changed, 68 insertions(+), 39 deletions(-) diff --git a/src/dawn_native/vulkan/BackendVk.cpp b/src/dawn_native/vulkan/BackendVk.cpp index c682b72453..0a68231e7b 100644 --- a/src/dawn_native/vulkan/BackendVk.cpp +++ b/src/dawn_native/vulkan/BackendVk.cpp @@ -210,22 +210,18 @@ namespace dawn_native { namespace vulkan { // cases are removed. InstanceExtSet extensionsToRequest = mGlobalInfo.extensions; - // TODO(cwallez@chromium.org): don't request extensions that have been promoted to Vulkan - // 1.1. This can only happen when we correctly detect and handle VkPhysicalDevice instance - // extensions that are promoted to be "device" extensions in the core Vulkan. If we don't - // do this there is a crash because a Vulkan 1.1 loader instance will not emulate the call - // on a Vulkan 1.0 ICD (and call nullptr). - // See https://github.com/KhronosGroup/Vulkan-Loader/issues/412. - if (!GetInstance()->IsBackendValidationEnabled()) { extensionsToRequest.Set(InstanceExt::DebugReport, false); } - usedKnobs.extensions = extensionsToRequest; std::vector extensionNames; for (uint32_t ext : IterateBitSet(extensionsToRequest.extensionBitSet)) { - extensionNames.push_back(GetInstanceExtInfo(static_cast(ext)).name); + const InstanceExtInfo& info = GetInstanceExtInfo(static_cast(ext)); + + if (info.versionPromoted > mGlobalInfo.apiVersion) { + extensionNames.push_back(info.name); + } } VkApplicationInfo appInfo; diff --git a/src/dawn_native/vulkan/VulkanExtensions.cpp b/src/dawn_native/vulkan/VulkanExtensions.cpp index 353f087802..9015a6dbb7 100644 --- a/src/dawn_native/vulkan/VulkanExtensions.cpp +++ b/src/dawn_native/vulkan/VulkanExtensions.cpp @@ -136,11 +136,23 @@ namespace dawn_native { namespace vulkan { static constexpr size_t kDeviceExtCount = static_cast(DeviceExt::EnumCount); static constexpr std::array sDeviceExtInfos{{ // + {DeviceExt::BindMemory2, "VK_KHR_bind_memory2", VulkanVersion_1_1}, {DeviceExt::Maintenance1, "VK_KHR_maintenance1", VulkanVersion_1_1}, + {DeviceExt::StorageBufferStorageClass, "VK_KHR_storage_buffer_storage_class", + VulkanVersion_1_1}, + {DeviceExt::GetPhysicalDeviceProperties2, "VK_KHR_get_physical_device_properties2", + VulkanVersion_1_1}, + {DeviceExt::GetMemoryRequirements2, "VK_KHR_get_memory_requirements2", VulkanVersion_1_1}, + {DeviceExt::ExternalMemoryCapabilities, "VK_KHR_external_memory_capabilities", + VulkanVersion_1_1}, + {DeviceExt::ExternalSemaphoreCapabilities, "VK_KHR_external_semaphore_capabilities", + VulkanVersion_1_1}, {DeviceExt::ExternalMemory, "VK_KHR_external_memory", VulkanVersion_1_1}, {DeviceExt::ExternalSemaphore, "VK_KHR_external_semaphore", VulkanVersion_1_1}, {DeviceExt::_16BitStorage, "VK_KHR_16bit_storage", VulkanVersion_1_1}, + {DeviceExt::SamplerYCbCrConversion, "VK_KHR_sampler_ycbcr_conversion", VulkanVersion_1_1}, + {DeviceExt::ImageFormatList, "VK_KHR_image_format_list", VulkanVersion_1_2}, {DeviceExt::ShaderFloat16Int8, "VK_KHR_shader_float16_int8", VulkanVersion_1_2}, {DeviceExt::ExternalMemoryFD, "VK_KHR_external_memory_fd", NeverPromoted}, @@ -195,6 +207,32 @@ namespace dawn_native { namespace vulkan { bool hasDependencies = false; switch (ext) { + // Happy extensions don't need anybody else! + case DeviceExt::BindMemory2: + case DeviceExt::GetMemoryRequirements2: + case DeviceExt::Maintenance1: + case DeviceExt::ImageFormatList: + case DeviceExt::StorageBufferStorageClass: + hasDependencies = true; + break; + + // Physical device extensions technically don't require the instance to support + // them but VulkanFunctions only loads the function pointers if the instance + // advertises the extension. So if we didn't have this check, we'd risk a calling + // a nullptr. + case DeviceExt::GetPhysicalDeviceProperties2: + hasDependencies = instanceExts.Has(InstanceExt::GetPhysicalDeviceProperties2); + break; + case DeviceExt::ExternalMemoryCapabilities: + hasDependencies = instanceExts.Has(InstanceExt::ExternalMemoryCapabilities) && + HasDep(DeviceExt::GetPhysicalDeviceProperties2); + break; + case DeviceExt::ExternalSemaphoreCapabilities: + hasDependencies = + instanceExts.Has(InstanceExt::ExternalSemaphoreCapabilities) && + HasDep(DeviceExt::GetPhysicalDeviceProperties2); + break; + case DeviceExt::DebugMarker: // TODO(cwallez@chromium.org): VK_KHR_debug_report is deprecated, switch to // using VK_KHR_debug_utils instead. @@ -202,51 +240,33 @@ namespace dawn_native { namespace vulkan { break; case DeviceExt::ImageDrmFormatModifier: - // TODO(cwallez@chromium.org): ImageDrmFormatModifier actually requires: - // - VK_KHR_bind_memory2 - // - VK_KHR_image_format_list - // - VK_KHR_sampler_ycbcr_conversion - // - // Also switch to using DeviceExt::GetPhysicalDeviceProperties2 when we decouple - // Instance / Device physical device extensions. - hasDependencies = instanceExts.Has(InstanceExt::GetPhysicalDeviceProperties2); + hasDependencies = HasDep(DeviceExt::BindMemory2) && + HasDep(DeviceExt::GetPhysicalDeviceProperties2) && + HasDep(DeviceExt::ImageFormatList) && + HasDep(DeviceExt::SamplerYCbCrConversion); break; case DeviceExt::Swapchain: hasDependencies = instanceExts.Has(InstanceExt::Surface); break; - case DeviceExt::Maintenance1: - hasDependencies = true; + case DeviceExt::SamplerYCbCrConversion: + hasDependencies = HasDep(DeviceExt::Maintenance1) && + HasDep(DeviceExt::BindMemory2) && + HasDep(DeviceExt::GetMemoryRequirements2) && + HasDep(DeviceExt::GetPhysicalDeviceProperties2); break; case DeviceExt::ShaderFloat16Int8: - // TODO(cwallez@chromium.org): switch to using - // DeviceExt::GetPhysicalDeviceProperties2 when we decouple Instance / Device - // physical device extensions. - hasDependencies = instanceExts.Has(InstanceExt::GetPhysicalDeviceProperties2); + hasDependencies = HasDep(DeviceExt::GetPhysicalDeviceProperties2); break; case DeviceExt::ExternalMemory: - // TODO(cwallez@chromium.org): switch to using - // DeviceExt::ExternalMemoryCapabilities when we decouple Instance / Device - // physical device extensions. - hasDependencies = instanceExts.Has(InstanceExt::ExternalMemoryCapabilities); + hasDependencies = HasDep(DeviceExt::ExternalMemoryCapabilities); break; case DeviceExt::ExternalSemaphore: - // TODO(cwallez@chromium.org): switch to using - // DeviceExt::ExternalSemaphoreCapabilities when we decouple Instance / Device - // physical device extensions. - hasDependencies = instanceExts.Has(InstanceExt::ExternalSemaphoreCapabilities); - break; - - case DeviceExt::_16BitStorage: - // TODO(cwallez@chromium.org): switch to using - // DeviceExt::GetPhysicalDeviceProperties2 when we decouple Instance / Device - // physical device extensions. - // Also depends on VK_KHR_storage_buffer_storage_class - hasDependencies = instanceExts.Has(InstanceExt::GetPhysicalDeviceProperties2); + hasDependencies = HasDep(DeviceExt::ExternalSemaphoreCapabilities); break; case DeviceExt::ExternalMemoryFD: @@ -263,6 +283,11 @@ namespace dawn_native { namespace vulkan { hasDependencies = HasDep(DeviceExt::ExternalSemaphore); break; + case DeviceExt::_16BitStorage: + hasDependencies = HasDep(DeviceExt::GetPhysicalDeviceProperties2) && + HasDep(DeviceExt::StorageBufferStorageClass); + break; + default: UNREACHABLE(); break; diff --git a/src/dawn_native/vulkan/VulkanExtensions.h b/src/dawn_native/vulkan/VulkanExtensions.h index 184df0b32d..5c59bc8311 100644 --- a/src/dawn_native/vulkan/VulkanExtensions.h +++ b/src/dawn_native/vulkan/VulkanExtensions.h @@ -74,12 +74,20 @@ namespace dawn_native { namespace vulkan { // inside EnsureDependencies) enum class DeviceExt { // Promoted to 1.1 + BindMemory2, Maintenance1, + StorageBufferStorageClass, + GetPhysicalDeviceProperties2, + GetMemoryRequirements2, + ExternalMemoryCapabilities, + ExternalSemaphoreCapabilities, ExternalMemory, ExternalSemaphore, _16BitStorage, + SamplerYCbCrConversion, // Promoted to 1.2 + ImageFormatList, ShaderFloat16Int8, // External* extensions