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

@@ -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<Implementation::ExtensionCount> featuresRequiredExtensions;
+    #endif
 
     Containers::String disabledExtensionsStorage;
     Containers::Array<Containers::StringView> 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<const Containers::StringView> 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<const char*>({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<Implementation::ExtensionCount> 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<class T> void Device::initializeExtensions(const Containers::ArrayView<const T> enabledExtensions) {
     /* Mark all known extensions as enabled */
     for(const T extension: enabledExtensions) {
-        for(Containers::ArrayView<const Extension> knownExtensions: {
+        for(const Version version: KnownVersionsForExtensions) {
-            Extension::extensions(Version::None),
+            const Containers::ArrayView<const Extension> knownExtensions =
-          /*Extension::extensions(Version::Vk10), is empty */
+                Extension::extensions(version);
-            Extension::extensions(Version::Vk11),
-            Extension::extensions(Version::Vk12)
-        }) {
             auto found = std::lower_bound(knownExtensions.begin(), knownExtensions.end(), extension, [](const Extension& a, const T& b) {
                 return a.string() < static_cast<const Containers::StringView&>(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)};
+        .setEnabledFeatures(DeviceFeature::SparseBinding|DeviceFeature::SparseResidency16Samples)};
-    CORRADE_COMPARE(out.str(), "TODO");
+    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<Extensions::KHR::get_physical_device_properties2>())
         CORRADE_SKIP("Neither Vulkan 1.1 nor KHR_get_physical_device_properties2 is supported, can't test");
-    if(properties.features() & DeviceFeature::TextureCompressionAstcHdr)
+    if(!(properties.features() & DeviceFeature::SamplerYcbcrConversion))
-        CORRADE_SKIP("The TextureCompressionAstcHdr feature is supported, can't test");
+        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);
+        .setEnabledFeatures(DeviceFeature::SamplerYcbcrConversion);
-
-    /* Just to verify we're doing the correct thing */
-    CORRADE_VERIFY(info->pNext);
-    CORRADE_COMPARE(static_cast<const VkBaseInStructure*>(info->pNext)->sType, VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_TEXTURE_COMPRESSION_ASTC_HDR_FEATURES_EXT);
-    CORRADE_VERIFY(static_cast<const VkPhysicalDeviceTextureCompressionASTCHDRFeaturesEXT*>(info->pNext)->textureCompressionASTC_HDR);
 
     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_COMPARE(out.str(), "Vk::Device: some enabled features need VK_KHR_sampler_ycbcr_conversion enabled\n");
-    CORRADE_VERIFY(!out.str().empty());
 }
 
 void DeviceVkTest::constructNoQueue() {