From b6e41ab1a7ba6d26e2626167eceec77e0660658d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Tue, 10 Nov 2020 15:10:20 +0100 Subject: [PATCH] Vk: initial APIs for binding a memory to an image. You won't believe it, but it took me over a month of sitting on the shitter until this design idea materialized out of [..] air. The whole story, in order: - Vulkan doesn't allow one VkDeviceMemory to be mapped more than once. This is rather sad, because since Vulkan best practices suggest to allocate a large block and suballocate from that, the engine needs an extra layer that "emulates" mapping the suballocations for the users but behind the scenes it inevitably has to map the whole VkDeviceMemory anyway and keep it mapped for as long as any of the sub-mappings is active. - Because if it would map just a certain suballocation and then the user would want to map another suballocation, it would have to discard the original mapping and create a new one spanning both suballocations and that has a risk of suddenly being in a different VM block, making all pointers to the previous mapping invalid. - The Vulkan Memory Allocator implements this approach of mapping the whole thing and because of all the bookkeeping it doesn't give a direct access to the underlying VkDeviceMemory, making it rather hard to integrate. Here I realized that: - Most allocations won't need to be mapped ever, so the hiding and obfuscation done by VMA isn't needed for those --- and we want interoperability with 3rd party code, so preventing access to VkDeviceMemory is out of question. - There's KHR_dedicated_allocation, which (probably?) wasn't around when VMA was originally designed. The extension was created because a dedicated allocation actually *does* make sense in certain cases and on certain architectures. Providing a way to make those thus shouldn't be something "temporary, until a real allocator exists" but rather a well-designed API that's there to stay. - Except for iGPUs, the usual way to populate a GPU buffer would be to first copy the data to some host-accessible scratch buffer and then do a GPU-side copy of that buffer to a device-local memory. The scratch buffer is very likely to have a vastly different suballocation scheme than GPU buffers (grow & discard everything once it's all uploaded, for example) so again trying to put the two under the same allocator umbrella doesn't make sense. Thus: - To avoid implementing a full-blown allocator right from the start, we'll first provide convenience APIs only for dedicated allocations -- making it possible to transfer memory ownership to an Image/Buffer so it can be treated the same way as in GL, and later having the Image/Buffer constructor implicitly allocate a dedicated VkDeviceMemory. - This default allocation will be subsequently equipped with KHR_dedicated_allocation bits. - Thanks to the extensible/layered nature of the design, the user is still capable of being completely in control of allocations, managing VkDeviceMemory sub-allocations by hand. Finally, once allocator APIs are figured out, the default Buffer/Image behavior gets switched from a dedicated allocation to using an allocator, and dedicated allocation will be only used if the KHR_dedicated_allocation bit is requested. --- doc/vulkan-mapping.dox | 2 +- src/Magnum/Vk/CMakeLists.txt | 2 +- src/Magnum/Vk/Image.cpp | 47 +++++++++++++-- src/Magnum/Vk/Image.h | 45 ++++++++++++++- src/Magnum/Vk/Implementation/DeviceState.cpp | 8 +++ src/Magnum/Vk/Implementation/DeviceState.h | 1 + src/Magnum/Vk/Test/CMakeLists.txt | 2 +- src/Magnum/Vk/Test/ImageTest.cpp | 22 ++++++- src/Magnum/Vk/Test/ImageVkTest.cpp | 60 +++++++++++++++++++- 9 files changed, 179 insertions(+), 10 deletions(-) diff --git a/doc/vulkan-mapping.dox b/doc/vulkan-mapping.dox index 439cb6ccb..26ab918ad 100644 --- a/doc/vulkan-mapping.dox +++ b/doc/vulkan-mapping.dox @@ -63,7 +63,7 @@ Vulkan function | Matching API --------------------------------------- | ------------ @fn_vk{BeginCommandBuffer}, \n \fn_vk{EndCommandBuffer} | | @fn_vk{BindBufferMemory}, \n @fn_vk{BindBufferMemory2} @m_class{m-label m-flat m-success} **KHR, 1.1** | | -@fn_vk{BindImageMemory}, \n @fn_vk{BindImageMemory2} @m_class{m-label m-flat m-success} **KHR, 1.1** | | +@fn_vk{BindImageMemory}, \n @fn_vk{BindImageMemory2} @m_class{m-label m-flat m-success} **KHR, 1.1** | @ref Image::bindMemory() @subsection vulkan-mapping-functions-c C diff --git a/src/Magnum/Vk/CMakeLists.txt b/src/Magnum/Vk/CMakeLists.txt index afc2038da..a709e2c58 100644 --- a/src/Magnum/Vk/CMakeLists.txt +++ b/src/Magnum/Vk/CMakeLists.txt @@ -31,7 +31,6 @@ set(MagnumVk_SRCS CommandPool.cpp Extensions.cpp Handle.cpp - Image.cpp Instance.cpp Memory.cpp Result.cpp @@ -46,6 +45,7 @@ set(MagnumVk_GracefulAssert_SRCS DeviceProperties.cpp Enums.cpp ExtensionProperties.cpp + Image.cpp LayerProperties.cpp) set(MagnumVk_HEADERS diff --git a/src/Magnum/Vk/Image.cpp b/src/Magnum/Vk/Image.cpp index 0ffc09e99..9757e351a 100644 --- a/src/Magnum/Vk/Image.cpp +++ b/src/Magnum/Vk/Image.cpp @@ -28,7 +28,6 @@ #include "Magnum/Vk/Device.h" #include "Magnum/Vk/Handle.h" #include "Magnum/Vk/Integration.h" -#include "Magnum/Vk/Memory.h" #include "Magnum/Vk/Result.h" #include "Magnum/Vk/Implementation/DeviceState.h" @@ -75,13 +74,13 @@ Image Image::wrap(Device& device, const VkImage handle, const HandleFlags flags) return out; } -Image::Image(Device& device, const ImageCreateInfo& info, NoAllocateT): _device{&device}, _flags{HandleFlag::DestroyOnDestruction} { +Image::Image(Device& device, const ImageCreateInfo& info, NoAllocateT): _device{&device}, _flags{HandleFlag::DestroyOnDestruction}, _dedicatedMemory{NoCreate} { MAGNUM_VK_INTERNAL_ASSERT_RESULT(device->CreateImage(device, info, nullptr, &_handle)); } -Image::Image(NoCreateT): _device{}, _handle{} {} +Image::Image(NoCreateT): _device{}, _handle{}, _dedicatedMemory{NoCreate} {} -Image::Image(Image&& other) noexcept: _device{other._device}, _handle{other._handle}, _flags{other._flags} { +Image::Image(Image&& other) noexcept: _device{other._device}, _handle{other._handle}, _flags{other._flags}, _dedicatedMemory{std::move(other._dedicatedMemory)} { other._handle = {}; } @@ -95,6 +94,7 @@ Image& Image::operator=(Image&& other) noexcept { swap(other._device, _device); swap(other._handle, _handle); swap(other._flags, _flags); + swap(other._dedicatedMemory, _dedicatedMemory); return *this; } @@ -107,6 +107,32 @@ MemoryRequirements Image::memoryRequirements() const { return requirements; } +void Image::bindMemory(Memory& memory, const UnsignedLong offset) { + VkBindImageMemoryInfo info{}; + info.sType = VK_STRUCTURE_TYPE_BIND_IMAGE_MEMORY_INFO; + info.image = _handle; + info.memory = memory; + info.memoryOffset = offset; + _device->state().bindImageMemoryImplementation(*_device, 1, &info); +} + +void Image::bindDedicatedMemory(Memory&& memory) { + bindMemory(memory, 0); + _dedicatedMemory = std::move(memory); +} + +bool Image::hasDedicatedMemory() const { + /* Sigh. Though better than needing to have `const handle()` overloads + returning `const VkDeviceMemory_T*` */ + return const_cast(*this)._dedicatedMemory.handle(); +} + +Memory& Image::dedicatedMemory() { + CORRADE_ASSERT(_dedicatedMemory.handle(), + "Vk::Image::dedicatedMemory(): image doesn't have a dedicated memory", _dedicatedMemory); + return _dedicatedMemory; +} + VkImage Image::release() { const VkImage handle = _handle; _handle = {}; @@ -125,4 +151,17 @@ void Image::getMemoryRequirementsImplementation11(Device& device, const VkImageM device->GetImageMemoryRequirements2(device, &info, &requirements); } +void Image::bindMemoryImplementationDefault(Device& device, UnsignedInt count, const VkBindImageMemoryInfo* const infos) { + for(std::size_t i = 0; i != count; ++i) + device->BindImageMemory(device, infos[i].image, infos[i].memory, infos[i].memoryOffset); +} + +void Image::bindMemoryImplementationKHR(Device& device, UnsignedInt count, const VkBindImageMemoryInfo* const infos) { + device->BindImageMemory2KHR(device, count, infos); +} + +void Image::bindMemoryImplementation11(Device& device, UnsignedInt count, const VkBindImageMemoryInfo* const infos) { + device->BindImageMemory2(device, count, infos); +} + }} diff --git a/src/Magnum/Vk/Image.h b/src/Magnum/Vk/Image.h index 776cc8f00..6b381f817 100644 --- a/src/Magnum/Vk/Image.h +++ b/src/Magnum/Vk/Image.h @@ -35,6 +35,7 @@ #include "Magnum/DimensionTraits.h" #include "Magnum/Magnum.h" #include "Magnum/Math/Vector3.h" +#include "Magnum/Vk/Memory.h" #include "Magnum/Vk/Vk.h" #include "Magnum/Vk/Vulkan.h" #include "Magnum/Vk/visibility.h" @@ -400,11 +401,48 @@ class MAGNUM_VK_EXPORT Image { /** * @brief Image memory requirements * - * @see @fn_vk_keyword{GetImageMemoryRequirements2}, + * @see @ref bindMemory(), @fn_vk_keyword{GetImageMemoryRequirements2}, * @fn_vk_keyword{GetImageMemoryRequirements} */ MemoryRequirements memoryRequirements() const; + /** + * @brief Bind image memory + * + * Assumes that @p memory type, the amount of @p memory at @p offset + * and @p offset alignment corresponds to image memory requirements. + * @see @ref memoryRequirements(), @ref bindDedicatedMemory(), + * @fn_vk_keyword{BindImageMemory2}, + * @fn_vk_keyword{BindImageMemory} + */ + void bindMemory(Memory& memory, UnsignedLong offset); + + /** + * @brief Bind a dedicated image memory + * + * Equivalent to @ref bindMemory() with @p offset set to @cpp 0 @ce, + * with the additional effect that @p memory ownership transfers to the + * image and is then available through @ref dedicatedMemory(). + */ + void bindDedicatedMemory(Memory&& memory); + + /** + * @brief Whether the image has a dedicated memory + * + * Returns @cpp true @ce if the image memory was bound using + * @ref bindDedicatedMemory(), @cpp false @ce otherwise. + * @see @ref dedicatedMemory() + */ + bool hasDedicatedMemory() const; + + /** + * @brief Dedicated image memory + * + * Expects that the image has a dedicated memory. + * @see @ref hasDedicatedMemory() + */ + Memory& dedicatedMemory(); + /** * @brief Release the underlying Vulkan image * @@ -422,11 +460,16 @@ class MAGNUM_VK_EXPORT Image { MAGNUM_VK_LOCAL static void getMemoryRequirementsImplementationKHR(Device& device, const VkImageMemoryRequirementsInfo2& info, VkMemoryRequirements2& requirements); MAGNUM_VK_LOCAL static void getMemoryRequirementsImplementation11(Device& device, const VkImageMemoryRequirementsInfo2& info, VkMemoryRequirements2& requirements); + MAGNUM_VK_LOCAL static void bindMemoryImplementationDefault(Device& device, UnsignedInt count, const VkBindImageMemoryInfo* infos); + MAGNUM_VK_LOCAL static void bindMemoryImplementationKHR(Device& device, UnsignedInt count, const VkBindImageMemoryInfo* infos); + MAGNUM_VK_LOCAL static void bindMemoryImplementation11(Device& device, UnsignedInt count, const VkBindImageMemoryInfo* infos); + /* Can't be a reference because of the NoCreate constructor */ Device* _device; VkImage _handle; HandleFlags _flags; + Memory _dedicatedMemory; }; }} diff --git a/src/Magnum/Vk/Implementation/DeviceState.cpp b/src/Magnum/Vk/Implementation/DeviceState.cpp index b6fd25e04..6ebc74cd4 100644 --- a/src/Magnum/Vk/Implementation/DeviceState.cpp +++ b/src/Magnum/Vk/Implementation/DeviceState.cpp @@ -46,6 +46,14 @@ DeviceState::DeviceState(Device& device) { } else { getImageMemoryRequirementsImplementation = &Image::getMemoryRequirementsImplementationDefault; } + + if(device.isVersionSupported(Version::Vk11)) { + bindImageMemoryImplementation = &Image::bindMemoryImplementation11; + } else if(device.isExtensionEnabled()) { + bindImageMemoryImplementation = &Image::bindMemoryImplementationKHR; + } else { + bindImageMemoryImplementation = &Image::bindMemoryImplementationDefault; + } } }}} diff --git a/src/Magnum/Vk/Implementation/DeviceState.h b/src/Magnum/Vk/Implementation/DeviceState.h index 446f1f0ef..dea6b1856 100644 --- a/src/Magnum/Vk/Implementation/DeviceState.h +++ b/src/Magnum/Vk/Implementation/DeviceState.h @@ -36,6 +36,7 @@ struct DeviceState { void(*getDeviceQueueImplementation)(Device&, const VkDeviceQueueInfo2&, VkQueue&); /** @todo put this eventually into a dedicated image state struct? */ void(*getImageMemoryRequirementsImplementation)(Device&, const VkImageMemoryRequirementsInfo2&, VkMemoryRequirements2&); + void(*bindImageMemoryImplementation)(Device&, UnsignedInt, const VkBindImageMemoryInfo*); }; }}} diff --git a/src/Magnum/Vk/Test/CMakeLists.txt b/src/Magnum/Vk/Test/CMakeLists.txt index 9f52fd331..3e8bd4270 100644 --- a/src/Magnum/Vk/Test/CMakeLists.txt +++ b/src/Magnum/Vk/Test/CMakeLists.txt @@ -32,7 +32,7 @@ corrade_add_test(VkEnumsTest EnumsTest.cpp LIBRARIES MagnumVkTestLib) corrade_add_test(VkExtensionsTest ExtensionsTest.cpp LIBRARIES MagnumVk) corrade_add_test(VkExtensionPropertiesTest ExtensionPropertiesTest.cpp LIBRARIES MagnumVk) corrade_add_test(VkHandleTest HandleTest.cpp LIBRARIES MagnumVk) -corrade_add_test(VkImageTest ImageTest.cpp LIBRARIES MagnumVk) +corrade_add_test(VkImageTest ImageTest.cpp LIBRARIES MagnumVkTestLib) corrade_add_test(VkInstanceTest InstanceTest.cpp LIBRARIES MagnumVk) corrade_add_test(VkIntegrationTest IntegrationTest.cpp LIBRARIES MagnumVk) corrade_add_test(VkLayerPropertiesTest LayerPropertiesTest.cpp LIBRARIES MagnumVk) diff --git a/src/Magnum/Vk/Test/ImageTest.cpp b/src/Magnum/Vk/Test/ImageTest.cpp index ec69b2b86..7f523e892 100644 --- a/src/Magnum/Vk/Test/ImageTest.cpp +++ b/src/Magnum/Vk/Test/ImageTest.cpp @@ -24,7 +24,9 @@ */ #include +#include #include +#include #include "Magnum/Vk/Image.h" #include "Magnum/Vk/Integration.h" @@ -47,6 +49,8 @@ struct ImageTest: TestSuite::Tester { void constructNoCreate(); void constructCopy(); + + void dedicatedMemoryNotDedicated(); }; ImageTest::ImageTest() { @@ -62,7 +66,9 @@ ImageTest::ImageTest() { &ImageTest::createInfoConstructFromVk, &ImageTest::constructNoCreate, - &ImageTest::constructCopy}); + &ImageTest::constructCopy, + + &ImageTest::dedicatedMemoryNotDedicated}); } void ImageTest::createInfoConstruct() { @@ -199,6 +205,20 @@ void ImageTest::constructCopy() { CORRADE_VERIFY(!(std::is_assignable{})); } +void ImageTest::dedicatedMemoryNotDedicated() { + #ifdef CORRADE_NO_ASSERT + CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions"); + #endif + + Image image{NoCreate}; + CORRADE_VERIFY(!image.hasDedicatedMemory()); + + std::ostringstream out; + Error redirectError{&out}; + image.dedicatedMemory(); + CORRADE_COMPARE(out.str(), "Vk::Image::dedicatedMemory(): image doesn't have a dedicated memory\n"); +} + }}}} CORRADE_TEST_MAIN(Magnum::Vk::Test::ImageTest) diff --git a/src/Magnum/Vk/Test/ImageVkTest.cpp b/src/Magnum/Vk/Test/ImageVkTest.cpp index 01043d635..8bed7d8b0 100644 --- a/src/Magnum/Vk/Test/ImageVkTest.cpp +++ b/src/Magnum/Vk/Test/ImageVkTest.cpp @@ -23,6 +23,9 @@ DEALINGS IN THE SOFTWARE. */ +#include + +#include "Magnum/Vk/DeviceProperties.h" #include "Magnum/Vk/Handle.h" #include "Magnum/Vk/Image.h" #include "Magnum/Vk/Memory.h" @@ -46,6 +49,9 @@ struct ImageVkTest: VulkanTester { void wrap(); void memoryRequirements(); + + void bindMemory(); + void bindDedicatedMemory(); }; ImageVkTest::ImageVkTest() { @@ -60,7 +66,10 @@ ImageVkTest::ImageVkTest() { &ImageVkTest::wrap, - &ImageVkTest::memoryRequirements}); + &ImageVkTest::memoryRequirements, + + &ImageVkTest::bindMemory, + &ImageVkTest::bindDedicatedMemory}); } void ImageVkTest::construct1D() { @@ -152,17 +161,29 @@ void ImageVkTest::constructMove() { VK_FORMAT_R8G8B8A8_UNORM, {256, 256}, 1}, NoAllocate}; VkImage handle = a.handle(); + /* Verify that also the dedicated memory gets moved */ + MemoryRequirements requirements = a.memoryRequirements(); + a.bindDedicatedMemory(Vk::Memory{device(), Vk::MemoryAllocateInfo{requirements.size(), + deviceProperties().pickMemory(Vk::MemoryFlag::DeviceLocal, requirements.memories())}}); + VkDeviceMemory memoryHandle = a.dedicatedMemory().handle(); + Image b = std::move(a); CORRADE_VERIFY(!a.handle()); + CORRADE_VERIFY(!a.hasDedicatedMemory()); CORRADE_COMPARE(b.handle(), handle); CORRADE_COMPARE(b.handleFlags(), HandleFlag::DestroyOnDestruction); + CORRADE_VERIFY(b.hasDedicatedMemory()); + CORRADE_COMPARE(b.dedicatedMemory().handle(), memoryHandle); Image c{NoCreate}; c = std::move(b); CORRADE_VERIFY(!b.handle()); + CORRADE_VERIFY(!b.hasDedicatedMemory()); CORRADE_COMPARE(b.handleFlags(), HandleFlags{}); CORRADE_COMPARE(c.handle(), handle); CORRADE_COMPARE(c.handleFlags(), HandleFlag::DestroyOnDestruction); + CORRADE_VERIFY(c.hasDedicatedMemory()); + CORRADE_COMPARE(c.dedicatedMemory().handle(), memoryHandle); CORRADE_VERIFY(std::is_nothrow_move_constructible::value); CORRADE_VERIFY(std::is_nothrow_move_assignable::value); @@ -195,6 +216,43 @@ void ImageVkTest::memoryRequirements() { CORRADE_COMPARE(requirements.size(), 128*64*4); } +void ImageVkTest::bindMemory() { + Image image{device(), ImageCreateInfo2D{ImageUsage::Sampled, + VK_FORMAT_R8G8B8A8_UNORM, {256, 256}, 8}, NoAllocate}; + MemoryRequirements requirements = image.memoryRequirements(); + + /* We're testing the offset, so ensure what we hardcode is correctly + aligned */ + constexpr UnsignedLong offset = 4096; + CORRADE_COMPARE_AS(offset, requirements.alignment(), + TestSuite::Compare::Divisible); + + Vk::Memory memory{device(), Vk::MemoryAllocateInfo{ + requirements.size() + offset, + deviceProperties().pickMemory(Vk::MemoryFlag::DeviceLocal, requirements.memories())}}; + + image.bindMemory(memory, offset); + CORRADE_VERIFY(!image.hasDedicatedMemory()); +} + +void ImageVkTest::bindDedicatedMemory() { + Image image{device(), ImageCreateInfo2D{ImageUsage::Sampled, + VK_FORMAT_R8G8B8A8_UNORM, {256, 256}, 8}, NoAllocate}; + MemoryRequirements requirements = image.memoryRequirements(); + + /** @todo expand once KHR_dedicated_allocation is implemented */ + + Vk::Memory memory{device(), Vk::MemoryAllocateInfo{ + requirements.size(), + deviceProperties().pickMemory(Vk::MemoryFlag::DeviceLocal, requirements.memories())}}; + VkDeviceMemory handle = memory.handle(); + CORRADE_VERIFY(handle); + + image.bindDedicatedMemory(std::move(memory)); + CORRADE_VERIFY(image.hasDedicatedMemory()); + CORRADE_COMPARE(image.dedicatedMemory().handle(), handle); +} + }}}} CORRADE_TEST_MAIN(Magnum::Vk::Test::ImageVkTest)