From 3f1a3eb1c2c4a89be6f98141450fbfe354928f3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Wed, 16 Dec 2020 22:31:40 +0100 Subject: [PATCH] Vk: a bit more robust feature enablement. Because the drivers don't seem to check that features are actually supported for anything beyond VkPhysicalDeviceFeatures2, we have to do that instead. Additionally, we also check if a feature is enabled but the extension that needs it is not (which isn't checked by drivers either). --- src/Magnum/Vk/Device.cpp | 70 ++++++++++++++++++++++++++--- src/Magnum/Vk/Test/DeviceVkTest.cpp | 21 ++++----- 2 files changed, 72 insertions(+), 19 deletions(-) diff --git a/src/Magnum/Vk/Device.cpp b/src/Magnum/Vk/Device.cpp index 9a35dc4cd..45bc29781 100644 --- a/src/Magnum/Vk/Device.cpp +++ b/src/Magnum/Vk/Device.cpp @@ -86,6 +86,10 @@ struct DeviceCreateInfo::State { Implementation::DeviceFeatures features{}; DeviceFeatures enabledFeatures; void* firstEnabledFeature{}; + /* Used for checking if the device enables extensions required by features */ + #ifndef CORRADE_NO_ASSERT + Math::BoolVector featuresRequiredExtensions; + #endif Containers::String disabledExtensionsStorage; Containers::Array disabledExtensions; @@ -371,12 +375,28 @@ DeviceCreateInfo& DeviceCreateInfo::setEnabledFeatures(const DeviceFeatures& fea _state->features2.sType = VkStructureType(1); \ _state->features2.features.field = VK_TRUE; \ } + #ifdef CORRADE_NO_ASSERT #define _cver(value, field, suffix, version) \ if(features & DeviceFeature::value) { \ _state->features.suffix.sType = VkStructureType(1); \ _state->features.suffix.field = VK_TRUE; \ } #define _cext _cver + #else + /* Not checking anything for the version, since if a device doesn't support + given version, it simply won't report the feature as supported */ + #define _cver(value, field, suffix, version) \ + if(features & DeviceFeature::value) { \ + _state->features.suffix.sType = VkStructureType(1); \ + _state->features.suffix.field = VK_TRUE; \ + } + #define _cext(value, field, suffix, extension) \ + if(features & DeviceFeature::value) { \ + _state->features.suffix.sType = VkStructureType(1); \ + _state->features.suffix.field = VK_TRUE; \ + _state->featuresRequiredExtensions.set(Extensions::extension::Index, true); \ + } + #endif #include "Magnum/Vk/Implementation/deviceFeatureMapping.hpp" #undef _c #undef _cver @@ -529,6 +549,17 @@ DeviceCreateInfo&& DeviceCreateInfo::addQueues(const VkDeviceQueueCreateInfo& in return std::move(addQueues(info)); } +namespace { + +constexpr Version KnownVersionsForExtensions[]{ + Version::None, + /*Version::Vk10, has no extensions */ + Version::Vk11, + Version::Vk12 +}; + +} + Device Device::wrap(Instance& instance, const VkDevice handle, const Version version, const Containers::ArrayView enabledExtensions, const DeviceFeatures& enabledFeatures, const HandleFlags flags) { /* Compared to the constructor nothing is printed here as it would be just repeating what was passed to the constructor */ @@ -564,6 +595,20 @@ Device::Device(Instance& instance, const DeviceCreateInfo& info, DevicePropertie CORRADE_ASSERT(info._info.queueCreateInfoCount, "Vk::Device: needs to be created with at least one queue", ); + /* Check that all enabled features were actually reported as supported. + I happily assumed the drivers would do that, but as far as my testing + goes it happens only for the VkPhysicalDeviceFeatures2, and not for + anything added by extensions after that, which is quite disappointing + -- I expected those to be checked as strictly as extensions, but not + even the validation layers seem to check those. + + Making this silently pass isn't a good idea because it might (or might + not) fail later in an unpredictable way. Fortunately it's rather easy + to check thanks to how these are designed :D */ + CORRADE_ASSERT(info._state->enabledFeatures <= _properties->features(), + "Vk::Device: some enabled features are not supported:" << + (info._state->enabledFeatures & ~_properties->features()), ); + const Version version = info._state->version != Version::None ? info._state->version : _properties->version(); @@ -592,6 +637,22 @@ Device::Device(Instance& instance, const DeviceCreateInfo& info, DevicePropertie initializeExtensions({info->ppEnabledExtensionNames, info->enabledExtensionCount}); initialize(instance, version, info._state->enabledFeatures); + #ifndef CORRADE_NO_ASSERT + /* This is a dumb O(n^2) search but in an assert that's completely fine */ + const Math::BoolVector missingExtensions = ~_enabledExtensions & info._state->featuresRequiredExtensions; + if(missingExtensions.any()) { + for(std::size_t i = 0; i != Implementation::ExtensionCount; ++i) { + if(!missingExtensions[i]) continue; + for(const Version version: KnownVersionsForExtensions) { + for(const Extension extension: Extension::extensions(version)) { + if(extension.index() != i) continue; + CORRADE_ASSERT_UNREACHABLE("Vk::Device: some enabled features need" << extension.string() << "enabled", ); + } + } + } + } + #endif + /* Extension-dependent state is initialized, now we can retrieve the queues from the device and save them to the outputs specified in addQueues(). Each of those calls added one or more entries into _state->queueOutput, @@ -658,12 +719,9 @@ Device& Device::operator=(Device&& other) noexcept { template void Device::initializeExtensions(const Containers::ArrayView enabledExtensions) { /* Mark all known extensions as enabled */ for(const T extension: enabledExtensions) { - for(Containers::ArrayView knownExtensions: { - Extension::extensions(Version::None), - /*Extension::extensions(Version::Vk10), is empty */ - Extension::extensions(Version::Vk11), - Extension::extensions(Version::Vk12) - }) { + for(const Version version: KnownVersionsForExtensions) { + const Containers::ArrayView knownExtensions = + Extension::extensions(version); auto found = std::lower_bound(knownExtensions.begin(), knownExtensions.end(), extension, [](const Extension& a, const T& b) { return a.string() < static_cast(b); }); diff --git a/src/Magnum/Vk/Test/DeviceVkTest.cpp b/src/Magnum/Vk/Test/DeviceVkTest.cpp index 9e05ce252..746becad4 100644 --- a/src/Magnum/Vk/Test/DeviceVkTest.cpp +++ b/src/Magnum/Vk/Test/DeviceVkTest.cpp @@ -938,39 +938,34 @@ void DeviceVkTest::constructFeatureNotSupported() { DeviceProperties properties = pickDevice(instance()); if(properties.features() & DeviceFeature::SparseBinding) CORRADE_SKIP("The SparseBinding feature is supported, can't test"); - CORRADE_SKIP("Currently this hits an internal assert, which can't be tested."); + if(properties.features() & DeviceFeature::SparseResidency16Samples) + CORRADE_SKIP("The SparseResidency16Samples feature is supported, can't test"); std::ostringstream out; Error redirectError{&out}; Queue queue{NoCreate}; Device device{instance(), DeviceCreateInfo{properties} .addQueues(0, {0.0f}, {queue}) - .setEnabledFeatures(DeviceFeature::SparseBinding)}; - CORRADE_COMPARE(out.str(), "TODO"); + .setEnabledFeatures(DeviceFeature::SparseBinding|DeviceFeature::SparseResidency16Samples)}; + CORRADE_COMPARE(out.str(), "Vk::Device: some enabled features are not supported: Vk::DeviceFeature::SparseBinding|Vk::DeviceFeature::SparseResidency16Samples\n"); } void DeviceVkTest::constructFeatureWithoutExtension() { DeviceProperties properties = pickDevice(instance()); if((!instance().isVersionSupported(Version::Vk11) || !properties.isVersionSupported(Version::Vk11)) && !instance().isExtensionEnabled()) CORRADE_SKIP("Neither Vulkan 1.1 nor KHR_get_physical_device_properties2 is supported, can't test"); - if(properties.features() & DeviceFeature::TextureCompressionAstcHdr) - CORRADE_SKIP("The TextureCompressionAstcHdr feature is supported, can't test"); + if(!(properties.features() & DeviceFeature::SamplerYcbcrConversion)) + CORRADE_SKIP("The SamplerYcbcrConversion feature is not supported, can't test"); Queue queue{NoCreate}; DeviceCreateInfo info{properties}; info.addQueues(0, {0.0f}, {queue}) - .setEnabledFeatures(DeviceFeature::TextureCompressionAstcHdr); - - /* Just to verify we're doing the correct thing */ - CORRADE_VERIFY(info->pNext); - CORRADE_COMPARE(static_cast(info->pNext)->sType, VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_TEXTURE_COMPRESSION_ASTC_HDR_FEATURES_EXT); - CORRADE_VERIFY(static_cast(info->pNext)->textureCompressionASTC_HDR); + .setEnabledFeatures(DeviceFeature::SamplerYcbcrConversion); std::ostringstream out; Error redirectError{&out}; Device device{instance(), info}; - CORRADE_EXPECT_FAIL("For some reason it doesn't complain when a feature that needs an extension is enabled. Am I stupid?"); - CORRADE_VERIFY(!out.str().empty()); + CORRADE_COMPARE(out.str(), "Vk::Device: some enabled features need VK_KHR_sampler_ycbcr_conversion enabled\n"); } void DeviceVkTest::constructNoQueue() {