From cad08497b1b0a3f1bc6dcfb91065ab77cef4f882 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Tue, 17 Nov 2020 13:57:25 +0100 Subject: [PATCH] Vk: DeviceCreateInfo::addQueues() overload taking QueueFlags directly. More convenient to use since one doesn't have to explicitly store a DeviceProperties instance to call pickQueueFamily() on it and then move it to DeviceCreateInfo to keep it efficient. This required a slight redesign of how DeviceProperties are stored in DeviceCreateInfo, now we need the instance to be always valid (but it can get never used). The wrap() API isn't doing any extra work so this won't add any inefficiency. --- doc/snippets/MagnumVk.cpp | 18 ++++------ src/Magnum/Vk/Device.cpp | 54 +++++++++++++++++++++-------- src/Magnum/Vk/Device.h | 49 ++++++++++++++++++-------- src/Magnum/Vk/Test/DeviceVkTest.cpp | 31 +++++++++++++++-- 4 files changed, 109 insertions(+), 43 deletions(-) diff --git a/doc/snippets/MagnumVk.cpp b/doc/snippets/MagnumVk.cpp index b3dd5514f..d21090a31 100644 --- a/doc/snippets/MagnumVk.cpp +++ b/doc/snippets/MagnumVk.cpp @@ -79,24 +79,20 @@ Vk::CommandBuffer commandBuffer = graphicsCommandPool.allocate(); { Vk::Instance instance; -/* [Device-usage-pick] */ -Vk::DeviceProperties props = Vk::pickDevice(instance); -/* [Device-usage-pick] */ - /* [Device-usage-construct-queue] */ Vk::Queue queue{NoCreate}; -Vk::Device device{instance, Vk::DeviceCreateInfo{props} - .addQueues(props.pickQueueFamily(Vk::QueueFlag::Graphics), {0.0f}, {queue}) +Vk::Device device{instance, Vk::DeviceCreateInfo{Vk::pickDevice(instance)} + .addQueues(Vk::QueueFlag::Graphics, {0.0f}, {queue}) }; /* [Device-usage-construct-queue] */ } { Vk::Instance instance; -Vk::DeviceProperties props{NoCreate}; +Vk::DeviceProperties properties{NoCreate}; using namespace Containers::Literals; /* [Device-usage-extensions] */ -Vk::Device device{instance, Vk::DeviceCreateInfo{props} +Vk::Device device{instance, Vk::DeviceCreateInfo{DOXYGEN_IGNORE(properties)} DOXYGEN_IGNORE() .addEnabledExtensions< // predefined extensions Vk::Extensions::EXT::index_type_uint8, @@ -108,12 +104,12 @@ Vk::Device device{instance, Vk::DeviceCreateInfo{props} { Vk::Instance instance; -Vk::DeviceProperties props{NoCreate}; using namespace Containers::Literals; /* [Device-usage-check-supported] */ -Vk::ExtensionProperties extensions = props.enumerateExtensionProperties(); +Vk::DeviceProperties properties = Vk::pickDevice(instance); +Vk::ExtensionProperties extensions = properties.enumerateExtensionProperties(); -Vk::DeviceCreateInfo info{props}; +Vk::DeviceCreateInfo info{properties}; if(extensions.isSupported()) info.addEnabledExtensions(); if(extensions.isSupported("VK_NV_mesh_shader"_s)) diff --git a/src/Magnum/Vk/Device.cpp b/src/Magnum/Vk/Device.cpp index 20b26f70f..12c7eb360 100644 --- a/src/Magnum/Vk/Device.cpp +++ b/src/Magnum/Vk/Device.cpp @@ -63,10 +63,9 @@ struct DeviceCreateInfo::State { std::size_t nextQueuePriority = 0; bool quietLog = false; Version version = Version::None; - /* Gets populated in the DeviceCreateInfo constructor that takes a - DeviceProperties&&, in which case it's then moved to the newly created - Device instance. If not populated, the Device instance gets nothing and - it'll gets populated on first access to Device::properties(). */ + /* Gets populated at the very end of DeviceCreateInfo(DeviceProperties&) + and then possibly overwritten in DeviceCreateInfo(DeviceProperties&&). + Either way, it's meant to be valid after the constructor exits. */ DeviceProperties properties{NoCreate}; }; @@ -122,6 +121,16 @@ DeviceCreateInfo::DeviceCreateInfo(DeviceProperties& deviceProperties, const Ext if(_state->version < Version::Vk11 && extensionProperties->isSupported()) addEnabledExtensions(); } + + /* Conservatively populate the device properties. + - In case the DeviceCreateInfo(DeviceProperties&&) constructor is used, + it'll get overwritten straight away with a populated instance. + - In case the addQueues(QueueFlags) API is not used and DeviceCreateInfo + isn't subsequently moved to the Device, it'll never get touched again + and Device will wrap() its own. + - In case addQueues(QueueFlags) is used it'll get populated and then + possibly discarded if it isn't subsequently moved to the Device. */ + _state->properties = DeviceProperties::wrap(*deviceProperties._instance, deviceProperties._handle); } DeviceCreateInfo::DeviceCreateInfo(DeviceProperties&& deviceProperties, const ExtensionProperties* extensionProperties, const Flags flags): DeviceCreateInfo{deviceProperties, extensionProperties, flags} { @@ -260,6 +269,24 @@ DeviceCreateInfo&& DeviceCreateInfo::addQueues(const UnsignedInt family, const s return std::move(*this); } +DeviceCreateInfo& DeviceCreateInfo::addQueues(const QueueFlags flags, const Containers::ArrayView priorities, const Containers::ArrayView> output) & { + return addQueues(_state->properties.pickQueueFamily(flags), priorities, output); +} + +DeviceCreateInfo&& DeviceCreateInfo::addQueues(const QueueFlags flags, const Containers::ArrayView priorities, const Containers::ArrayView> output) && { + addQueues(flags, priorities, output); + return std::move(*this); +} + +DeviceCreateInfo& DeviceCreateInfo::addQueues(const QueueFlags flags, const std::initializer_list priorities, const std::initializer_list> output) & { + return addQueues(flags, Containers::arrayView(priorities), Containers::arrayView(output)); +} + +DeviceCreateInfo&& DeviceCreateInfo::addQueues(const QueueFlags flags, const std::initializer_list priorities, const std::initializer_list> output) && { + addQueues(flags, priorities, output); + return std::move(*this); +} + DeviceCreateInfo& DeviceCreateInfo::addQueues(const VkDeviceQueueCreateInfo& info) & { /* This can happen in case we used the NoInit or VkDeviceCreateInfo constructor */ @@ -295,7 +322,7 @@ Device Device::wrap(Instance& instance, const VkDevice handle, const Version ver return wrap(instance, handle, version, Containers::arrayView(enabledExtensions), flags); } -Device::Device(Instance& instance, const DeviceCreateInfo& info): Device{instance, info, DeviceProperties{NoCreate}} {} +Device::Device(Instance& instance, const DeviceCreateInfo& info): Device{instance, info, DeviceProperties::wrap(instance, info._physicalDevice)} {} Device::Device(Instance& instance, DeviceCreateInfo&& info): Device{instance, info, std::move(info._state->properties)} {} @@ -303,19 +330,18 @@ Device::Device(Instance& instance, const DeviceCreateInfo& info, DevicePropertie #ifdef CORRADE_GRACEFUL_ASSERT _handle{}, /* Otherwise the destructor dies if we hit the queue assert */ #endif - _flags{HandleFlag::DestroyOnDestruction} + _flags{HandleFlag::DestroyOnDestruction}, + _properties{Containers::InPlaceInit, std::move(properties)} { + /* The properties should always be a valid instance, either moved from + outside or created again from VkPhysicalDevice, in case it couldn't be + moved. If it's not, something in DeviceCreateInfo or here got messed + up. */ + CORRADE_INTERNAL_ASSERT(_properties->handle()); + CORRADE_ASSERT(info._info.queueCreateInfoCount, "Vk::Device: needs to be created with at least one queue", ); - /* If the passed properties are populated, use them. Otherwise create a new - instance as we'd have no other way to remember the VkPhysicalDevice - handle otherwise */ - if(properties.handle()) - _properties.emplace(std::move(properties)); - else - _properties.emplace(DeviceProperties::wrap(instance, info._physicalDevice)); - const Version version = info._state->version != Version::None ? info._state->version : _properties->version(); diff --git a/src/Magnum/Vk/Device.h b/src/Magnum/Vk/Device.h index ca2104cbd..9e0e72f18 100644 --- a/src/Magnum/Vk/Device.h +++ b/src/Magnum/Vk/Device.h @@ -207,7 +207,8 @@ class MAGNUM_VK_EXPORT DeviceCreateInfo { * @return Reference to self (for method chaining) * * At least one queue has to be added. - * @see @ref DeviceProperties::pickQueueFamily() + * @see @ref DeviceProperties::pickQueueFamily(), + * @ref addQueues(QueueFlags, Containers::ArrayView, Containers::ArrayView>) */ DeviceCreateInfo& addQueues(UnsignedInt family, Containers::ArrayView priorities, Containers::ArrayView> output) &; /** @overload */ @@ -217,6 +218,27 @@ class MAGNUM_VK_EXPORT DeviceCreateInfo { /** @overload */ DeviceCreateInfo&& addQueues(UnsignedInt family, std::initializer_list priorities, std::initializer_list> output) &&; + /** + * @brief Add queues of family matching given flags + * + * Equivalent to picking a queue family first using + * @ref DeviceProperties::pickQueueFamily() based on @p flags and then + * calling @ref addQueues(UnsignedInt, Containers::ArrayView, Containers::ArrayView>) + * with the family index. + * + * Note that @ref DeviceProperties::pickQueueFamily() exits in case it + * doesn't find any family satisfying given @p flags --- for a + * failproof scenario you may want to go with the above overload and + * @ref DeviceProperties::tryPickQueueFamily() instead. + */ + DeviceCreateInfo& addQueues(QueueFlags flags, Containers::ArrayView priorities, Containers::ArrayView> output) &; + /** @overload */ + DeviceCreateInfo&& addQueues(QueueFlags flags, Containers::ArrayView priorities, Containers::ArrayView> output) &&; + /** @overload */ + DeviceCreateInfo& addQueues(QueueFlags flags, std::initializer_list priorities, std::initializer_list> output) &; + /** @overload */ + DeviceCreateInfo&& addQueues(QueueFlags flags, std::initializer_list priorities, std::initializer_list> output) &&; + /** * @brief Add queues using raw info * @return Reference to self (for method chaining) @@ -269,23 +291,20 @@ even a software implementation. If the application needs something specific, you can use @ref enumerateDevices() instead, pick a device from the list manually, provide the users with a list to choose from etc. -@snippet MagnumVk.cpp Device-usage-pick - -After a device is picked, a @ref Device can be created using -@ref DeviceCreateInfo. At the very least you'll need to set up queues, as every -Vulkan device needs at least one. That's done by creating an empty @ref Queue -instance and then referencing it from @ref DeviceCreateInfo::addQueues(). After -the device is constructed, the queue gets populated and is ready to be used. +The picked device is then passed to @ref DeviceCreateInfo. At the very least +you'll also need to set up queues, as every Vulkan device needs at least one. +That's done by creating an empty @ref Queue instance and then referencing it +from @ref DeviceCreateInfo::addQueues(). After the device is constructed, the +queue gets populated and is ready to be used. @snippet MagnumVk.cpp Device-usage-construct-queue -In the above snippet, we requested a graphics queue --- the -@ref DeviceProperties instance we made earlier acts as knowledge base for a -particular device, providing info about available queues, extensions, features, -memory heaps and implementation limits --- and we used a convenience API to -pick the first available graphics queue. As with device picking, you can -also iterate through all @ref DeviceProperties::queueFamilyCount() and choose -one manually. +In the above snippet, we requested a graphics queue via a convenience API. The +information about available queues and other device properties is stored in a +@ref DeviceProperties that got returned from @ref pickDevice() and +@ref DeviceCreateInfo called @ref DeviceProperties::pickQueueFamily() for us. +As with device picking, you can also iterate through all +@ref DeviceProperties::queueFamilyCount() and choose one manually. Same as with @ref Instance, the above won't enable any additional extensions except for what the engine itself needs or what's supplied on the command line. Use @ref DeviceCreateInfo::addEnabledExtensions() to enable them, you can use diff --git a/src/Magnum/Vk/Test/DeviceVkTest.cpp b/src/Magnum/Vk/Test/DeviceVkTest.cpp index 0c4276940..16e6a3bc7 100644 --- a/src/Magnum/Vk/Test/DeviceVkTest.cpp +++ b/src/Magnum/Vk/Test/DeviceVkTest.cpp @@ -24,6 +24,7 @@ */ #include +#include #include #include #include @@ -57,6 +58,7 @@ struct DeviceVkTest: VulkanTester { void createInfoRvalue(); void construct(); + void constructQueueFromFlags(); void constructExtensions(); void constructTransferDeviceProperties(); void constructExtensionsCommandLineDisable(); @@ -137,6 +139,7 @@ DeviceVkTest::DeviceVkTest(): VulkanTester{NoCreate} { &DeviceVkTest::createInfoRvalue, &DeviceVkTest::construct, + &DeviceVkTest::constructQueueFromFlags, &DeviceVkTest::constructExtensions, &DeviceVkTest::constructTransferDeviceProperties}); @@ -244,9 +247,14 @@ void DeviceVkTest::createInfoWrongQueueOutputCount() { } void DeviceVkTest::createInfoRvalue() { + /* Verify that there actually are graphics queues so we don't exit inside + addQueues() */ + CORRADE_VERIFY(pickDevice(instance()).tryPickQueueFamily(QueueFlag::Graphics)); + Float zero[1]{}; - Queue a{NoCreate}, b{NoCreate}; - Containers::Reference reference[1]{a}; + Queue a{NoCreate}, b{NoCreate}, c{NoCreate}, d{NoCreate}; + Containers::Reference referenceA[1]{a}; + Containers::Reference referenceC[1]{c}; VkDeviceQueueCreateInfo rawQueueInfo{}; rawQueueInfo.sType = VK_STRUCTURE_TYPE_DEVICE_QUEUE_CREATE_INFO; @@ -260,8 +268,10 @@ void DeviceVkTest::createInfoRvalue() { .addEnabledExtensions(Containers::ArrayView{}) .addEnabledExtensions(std::initializer_list{}) .addEnabledExtensions<>() - .addQueues(0, zero, reference) + .addQueues(0, zero, referenceA) .addQueues(0, {0.0f}, {b}) + .addQueues(QueueFlag::Graphics, zero, referenceC) + .addQueues(QueueFlag::Graphics, {0.0f}, {d}) .addQueues(rawQueueInfo); /* Just to test something, main point is that the above compiles, links and @@ -306,6 +316,21 @@ void DeviceVkTest::construct() { CORRADE_VERIFY(true); } +void DeviceVkTest::constructQueueFromFlags() { + DeviceProperties deviceProperties = pickDevice(instance()); + + /* Verify that there actually are graphics queues so we don't exit after */ + CORRADE_VERIFY(deviceProperties.tryPickQueueFamily(QueueFlag::Graphics)); + + Queue queue{NoCreate}; + Device device{instance(), DeviceCreateInfo{deviceProperties} + .addQueues(QueueFlag::Graphics, {0.0f}, {queue})}; + CORRADE_VERIFY(device.handle()); + + /* The queue should be filled in like usual */ + CORRADE_VERIFY(queue.handle()); +} + void DeviceVkTest::constructExtensions() { if(std::getenv("MAGNUM_DISABLE_EXTENSIONS")) CORRADE_SKIP("Can't test with the MAGNUM_DISABLE_EXTENSIONS environment variable set");