From 294c1378b502f1136be8c0273ded54aa69c6dbb0 Mon Sep 17 00:00:00 2001 From: catbot Date: Tue, 16 Jun 2026 16:03:50 +0000 Subject: [PATCH] fix(device): add preferred mask + required-only fallback to GetMemoryType (#59) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GetMemoryType was a bare first-superset match that threw whenever no memory type satisfied the requested property flags. Callers combining a mandatory flag with a perf-only one (HOST_VISIBLE | DEVICE_LOCAL for the descriptor heaps) would then fail allocation on any device without a host-visible device-local heap (no resizable BAR). It now takes a `preferred` mask distinct from `required`: a type satisfying both is chosen first, falling back to a required-only match when the preference is unavailable, and throwing only when even `required` cannot be met. VulkanBuffer::Create/Resize gain an optional trailing `preferredPropertyFlags`, and the descriptor heaps now treat DEVICE_LOCAL as a preference on top of mandatory HOST_VISIBLE. This does not touch any flush/barrier behaviour — coherency is not assumed anywhere here. Adds tests/MemoryTypeFallback, a pure-CPU test that installs synthetic memory layouts and exercises selection, preference, fallback, and the unsatisfiable-required throw. Co-Authored-By: Claude Opus 4.8 --- implementations/Crafter.Graphics-Device.cpp | 27 +++- ...Crafter.Graphics-DescriptorHeapVulkan.cppm | 9 +- interfaces/Crafter.Graphics-Device.cppm | 7 +- interfaces/Crafter.Graphics-VulkanBuffer.cppm | 13 +- project.cpp | 29 ++++ tests/MemoryTypeFallback/main.cpp | 146 ++++++++++++++++++ 6 files changed, 218 insertions(+), 13 deletions(-) create mode 100644 tests/MemoryTypeFallback/main.cpp diff --git a/implementations/Crafter.Graphics-Device.cpp b/implementations/Crafter.Graphics-Device.cpp index 41bf2b1..3a30ed4 100644 --- a/implementations/Crafter.Graphics-Device.cpp +++ b/implementations/Crafter.Graphics-Device.cpp @@ -811,17 +811,32 @@ void Device::Initialize() { } } -std::uint32_t Device::GetMemoryType(uint32_t typeBits, VkMemoryPropertyFlags properties) { - for (uint32_t i = 0; i < memoryProperties.memoryTypeCount; i++) +std::uint32_t Device::GetMemoryType(uint32_t typeBits, VkMemoryPropertyFlags required, VkMemoryPropertyFlags preferred) { + // First pass: honour the preferred bits on top of the required ones. + // Second pass (when nothing matched, or no preferences were given): drop + // the preferences and match the required bits alone. Vulkan orders the + // memory types so first-superset-match is generally optimal, so a plain + // linear scan suffices for each pass. We only throw when even `required` + // cannot be satisfied — i.e. no valid memory exists for the allocation. + for (VkMemoryPropertyFlags wanted : {required | preferred, required}) { - if ((typeBits & 1) == 1) + uint32_t bits = typeBits; + for (uint32_t i = 0; i < memoryProperties.memoryTypeCount; i++) { - if ((memoryProperties.memoryTypes[i].propertyFlags & properties) == properties) + if ((bits & 1) == 1) { - return i; + if ((memoryProperties.memoryTypes[i].propertyFlags & wanted) == wanted) + { + return i; + } } + bits >>= 1; + } + + // No preferences to fall back from — the second pass would be identical. + if (preferred == 0) { + break; } - typeBits >>= 1; } throw std::runtime_error("Could not find a matching memory type"); diff --git a/interfaces/Crafter.Graphics-DescriptorHeapVulkan.cppm b/interfaces/Crafter.Graphics-DescriptorHeapVulkan.cppm index 2350b53..1cd2c7e 100644 --- a/interfaces/Crafter.Graphics-DescriptorHeapVulkan.cppm +++ b/interfaces/Crafter.Graphics-DescriptorHeapVulkan.cppm @@ -83,9 +83,14 @@ export namespace Crafter { samplerFreelist.clear(); samplerFreelist.reserve(samplers); + // The heaps are written by the CPU every frame (HOST_VISIBLE is + // mandatory) and read by the GPU, so DEVICE_LOCAL is a perf + // preference, not a requirement: devices without a host-visible + // device-local heap (no resizable BAR) get a host-visible-only + // allocation instead of a hard allocation failure. for(std::uint8_t i = 0; i < Window::numFrames; i++) { - resourceHeap[i].Resize(VK_BUFFER_USAGE_SHADER_DEVICE_ADDRESS_BIT | VK_BUFFER_USAGE_DESCRIPTOR_HEAP_BIT_EXT, VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT | VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT, resourceSize); - samplerHeap[i].Resize(VK_BUFFER_USAGE_SHADER_DEVICE_ADDRESS_BIT | VK_BUFFER_USAGE_DESCRIPTOR_HEAP_BIT_EXT, VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT | VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT, samplerSize); + resourceHeap[i].Resize(VK_BUFFER_USAGE_SHADER_DEVICE_ADDRESS_BIT | VK_BUFFER_USAGE_DESCRIPTOR_HEAP_BIT_EXT, VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT, resourceSize, VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT); + samplerHeap[i].Resize(VK_BUFFER_USAGE_SHADER_DEVICE_ADDRESS_BIT | VK_BUFFER_USAGE_DESCRIPTOR_HEAP_BIT_EXT, VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT, samplerSize, VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT); } } diff --git a/interfaces/Crafter.Graphics-Device.cppm b/interfaces/Crafter.Graphics-Device.cppm index c1a39c1..a36fa96 100644 --- a/interfaces/Crafter.Graphics-Device.cppm +++ b/interfaces/Crafter.Graphics-Device.cppm @@ -199,7 +199,12 @@ export namespace Crafter { // ComputeShader read the offset off the pipeline they record. static void CheckVkResult(VkResult result); - static std::uint32_t GetMemoryType(std::uint32_t typeBits, VkMemoryPropertyFlags properties); + // Selects a memory type index from typeBits that satisfies `required`. + // When `preferred` bits are also given, a type satisfying both is + // chosen first; if none exists we fall back to required-only rather + // than throwing. Throws only when even `required` cannot be met (no + // valid memory exists for the allocation). + static std::uint32_t GetMemoryType(std::uint32_t typeBits, VkMemoryPropertyFlags required, VkMemoryPropertyFlags preferred = 0); // ─── Wayland key repeat ──────────────────────────────────────── // TickKeyRepeats fires onRawKeyDown / onRawKeyHold / onTextInput on diff --git a/interfaces/Crafter.Graphics-VulkanBuffer.cppm b/interfaces/Crafter.Graphics-VulkanBuffer.cppm index c2b1a4d..c1d0c25 100644 --- a/interfaces/Crafter.Graphics-VulkanBuffer.cppm +++ b/interfaces/Crafter.Graphics-VulkanBuffer.cppm @@ -55,7 +55,12 @@ namespace Crafter { class VulkanBuffer : public VulkanBufferBase, public VulkanBufferMappedConditional { public: VulkanBuffer() = default; - void Create(VkBufferUsageFlags2 usageFlags, VkMemoryPropertyFlags memoryPropertyFlags, std::uint32_t count) { + // `preferredPropertyFlags` are best-effort: the allocation prefers a + // memory type satisfying them on top of `memoryPropertyFlags`, but + // falls back to the required flags alone rather than failing. Use it + // for perf hints (e.g. DEVICE_LOCAL on a mapped buffer) that aren't + // available on every device — see Device::GetMemoryType. + void Create(VkBufferUsageFlags2 usageFlags, VkMemoryPropertyFlags memoryPropertyFlags, std::uint32_t count, VkMemoryPropertyFlags preferredPropertyFlags = 0) { size = count * sizeof(T); // Carry usage in the maintenance5 flags2 chain so 64-bit bits @@ -77,7 +82,7 @@ namespace Crafter { VkMemoryAllocateInfo memAlloc { .sType = VK_STRUCTURE_TYPE_MEMORY_ALLOCATE_INFO, .allocationSize = memReqs.size, - .memoryTypeIndex = Device::GetMemoryType(memReqs.memoryTypeBits, memoryPropertyFlags) + .memoryTypeIndex = Device::GetMemoryType(memReqs.memoryTypeBits, memoryPropertyFlags, preferredPropertyFlags) }; VkMemoryAllocateFlagsInfoKHR allocFlagsInfo { @@ -109,11 +114,11 @@ namespace Crafter { buffer = VK_NULL_HANDLE; } - void Resize(VkBufferUsageFlags2 usageFlags, VkMemoryPropertyFlags memoryPropertyFlags, std::uint32_t count) { + void Resize(VkBufferUsageFlags2 usageFlags, VkMemoryPropertyFlags memoryPropertyFlags, std::uint32_t count, VkMemoryPropertyFlags preferredPropertyFlags = 0) { if(buffer != VK_NULL_HANDLE) { Clear(); } - Create(usageFlags, memoryPropertyFlags, count); + Create(usageFlags, memoryPropertyFlags, count, preferredPropertyFlags); } void Copy(VkCommandBuffer cmd, VulkanBuffer& dst) { diff --git a/project.cpp b/project.cpp index ad17e7f..d678456 100644 --- a/project.cpp +++ b/project.cpp @@ -384,6 +384,35 @@ extern "C" Configuration CrafterBuildProject(std::span a textImpls.emplace_back("tests/ShapeTextCache/main"); xc.GetInterfacesAndImplementations(ifaces, textImpls); cfg.tests.push_back(std::move(textTest)); + + // Issue #59: Device::GetMemoryType gained a `preferred` mask and a + // required-only fallback so callers combining a mandatory flag with a + // perf-only one (HOST_VISIBLE | DEVICE_LOCAL descriptor heaps) no + // longer throw on devices without a host-visible device-local heap. + // The selection is pure CPU logic over Device::memoryProperties, so + // this test installs synthetic memory layouts and drives it directly — + // no GPU device needed at runtime. + Test memTest; + Configuration& mc = memTest.config; + mc.path = cfg.path; + mc.name = "MemoryTypeFallback"; + mc.outputName = "MemoryTypeFallback"; + mc.type = ConfigurationType::Executable; + mc.target = cfg.target; + mc.march = cfg.march; + mc.mtune = cfg.mtune; + mc.debug = cfg.debug; + mc.sysroot = cfg.sysroot; + mc.dependencies = cfg.dependencies; + mc.externalDependencies = cfg.externalDependencies; + mc.compileFlags = cfg.compileFlags; + mc.linkFlags = cfg.linkFlags; + mc.defines = cfg.defines; + mc.cFiles = cfg.cFiles; + std::vector memImpls(impls.begin(), impls.end()); + memImpls.emplace_back("tests/MemoryTypeFallback/main"); + mc.GetInterfacesAndImplementations(ifaces, memImpls); + cfg.tests.push_back(std::move(memTest)); } return cfg; diff --git a/tests/MemoryTypeFallback/main.cpp b/tests/MemoryTypeFallback/main.cpp new file mode 100644 index 0000000..a0c0ee9 --- /dev/null +++ b/tests/MemoryTypeFallback/main.cpp @@ -0,0 +1,146 @@ +/* +Crafter®.Graphics +Copyright (C) 2026 Catcrafts® +catcrafts.net + +This library is free software; you can redistribute it and/or +modify it under the terms of the GNU Lesser General Public +License version 3.0 as published by the Free Software Foundation; + +This library is distributed in the hope that it will be useful, +but WITHOUT ANY WARRANTY; without even the implied warranty of +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +Lesser General Public License for more details. + +You should have received a copy of the GNU Lesser General Public +License along with this library; if not, write to the Free Software +Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA +*/ + +// Regression test for issue #59: Device::GetMemoryType used to be a bare +// first-superset match that threw whenever no memory type satisfied the +// requested property flags. Callers that combine a mandatory flag with a +// perf-only one (e.g. HOST_VISIBLE | DEVICE_LOCAL for the descriptor heaps) +// would then crash at allocation time on any device without a host-visible +// device-local heap (no resizable BAR). +// +// GetMemoryType now takes a `preferred` mask distinct from `required`: +// - prefers a type satisfying required+preferred, +// - falls back to a required-only match when the preference is unavailable, +// - still throws only when even `required` cannot be met. +// +// The selection is pure CPU logic over Device::memoryProperties, so this test +// drives it directly with synthetic memory layouts — no Vulkan device needed. + +#include +#include "vulkan/vulkan.h" + +import Crafter.Graphics; +import std; +using namespace Crafter; + +namespace { + +int failures = 0; + +void Check(bool ok, std::string_view what) { + std::println("{} {}", ok ? "PASS" : "FAIL", what); + if (!ok) ++failures; +} + +// Install a synthetic memory layout: one VkMemoryType per entry, in order. +void SetMemoryTypes(std::initializer_list flags) { + Device::memoryProperties = {}; + Device::memoryProperties.memoryTypeCount = static_cast(flags.size()); + std::uint32_t i = 0; + for (VkMemoryPropertyFlags f : flags) { + Device::memoryProperties.memoryTypes[i].propertyFlags = f; + Device::memoryProperties.memoryTypes[i].heapIndex = 0; + ++i; + } +} + +constexpr auto DEVICE_LOCAL = VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT; +constexpr auto HOST_VISIBLE = VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT; +constexpr auto HOST_COHERENT = VK_MEMORY_PROPERTY_HOST_COHERENT_BIT; +constexpr auto HOST_CACHED = VK_MEMORY_PROPERTY_HOST_CACHED_BIT; + +// Call GetMemoryType, reporting whether it threw rather than returning. +bool Threw(std::uint32_t typeBits, VkMemoryPropertyFlags required, VkMemoryPropertyFlags preferred) { + try { + Device::GetMemoryType(typeBits, required, preferred); + return false; + } catch (const std::runtime_error&) { + return true; + } +} + +} // namespace + +int main() { + constexpr std::uint32_t kAll = ~0u; + + // --- required-only: first superset match wins ------------------------- + { + SetMemoryTypes({DEVICE_LOCAL, HOST_VISIBLE, HOST_VISIBLE | DEVICE_LOCAL}); + Check(Device::GetMemoryType(kAll, HOST_VISIBLE) == 1, + "required-only picks the first type satisfying the required bits"); + Check(Device::GetMemoryType(kAll, DEVICE_LOCAL) == 0, + "required-only honours superset ordering for a different mask"); + } + + // --- typeBits gates which types are eligible -------------------------- + { + SetMemoryTypes({HOST_VISIBLE, HOST_VISIBLE, HOST_VISIBLE}); + // Only bit 2 is set -> type index 2 is the sole candidate. + Check(Device::GetMemoryType(0b100, HOST_VISIBLE) == 2, + "typeBits mask excludes types the allocation can't use"); + } + + // --- preferred bits are honoured when available ----------------------- + { + SetMemoryTypes({HOST_VISIBLE, HOST_VISIBLE | HOST_COHERENT, HOST_VISIBLE | DEVICE_LOCAL}); + Check(Device::GetMemoryType(kAll, HOST_VISIBLE, DEVICE_LOCAL) == 2, + "preferred DEVICE_LOCAL selects the host-visible device-local type"); + Check(Device::GetMemoryType(kAll, HOST_VISIBLE, HOST_COHERENT) == 1, + "preferred HOST_COHERENT selects the host-visible coherent type"); + } + + // --- preferred unavailable: fall back to required-only ---------------- + { + // No host-visible type is also device-local (the no-resizable-BAR case). + SetMemoryTypes({DEVICE_LOCAL, HOST_VISIBLE | HOST_COHERENT}); + Check(Device::GetMemoryType(kAll, HOST_VISIBLE, DEVICE_LOCAL) == 1, + "unsatisfiable preference falls back to a required-only match instead of throwing"); + Check(!Threw(kAll, HOST_VISIBLE, DEVICE_LOCAL), + "the fallback path does not throw when required is still satisfiable"); + } + + // --- preferred is satisfiable only via the second-best type ----------- + { + // First host-visible type lacks the preference; a later one has it. + SetMemoryTypes({HOST_VISIBLE, HOST_VISIBLE | DEVICE_LOCAL}); + Check(Device::GetMemoryType(kAll, HOST_VISIBLE, DEVICE_LOCAL) == 1, + "preference outranks ordering: skips an earlier required-only type"); + } + + // --- required unsatisfiable still throws ------------------------------ + { + SetMemoryTypes({DEVICE_LOCAL, HOST_VISIBLE | HOST_COHERENT}); + Check(Threw(kAll, HOST_CACHED, 0), + "no type satisfies the required bits -> throws (no valid memory)"); + Check(Threw(kAll, HOST_CACHED, DEVICE_LOCAL), + "unsatisfiable required throws even when a preference is given"); + // typeBits that masks out the only matching type is also unsatisfiable. + SetMemoryTypes({HOST_VISIBLE}); + Check(Threw(0b10, HOST_VISIBLE, 0), + "typeBits excluding every match -> throws"); + } + + if (failures != 0) { + std::println("{} check(s) failed", failures); + return EXIT_FAILURE; + } + std::println("all checks passed"); + return EXIT_SUCCESS; +} -- 2.54.0