Vulkan: wrap external buffers as regions by xFile3160 · Pull Request #9110 · halide/Halide

xFile3160 · 2026-04-25T08:55:39Z

This lets callers wrap an external VkBuffer directly. halide_vulkan_wrap_vk_buffer(..., offset) creates Halide-owned metadata for the buffer. The caller still owns the VkBuffer, so detach/free only releases Halide metadata.
The offset is used in descriptor and copy paths. Crops are view metadata that point back to the owned or wrapped root buffer. Mapping wrapped external buffers stays unsupported because Halide only has the VkBuffer; it does not know the backing VkDeviceMemory, memory properties, bind offset, host visibility/coherency, or ownership.
Added Vulkan buffer wrap/get/offset/detach coverage.

AI disclosure:
I used OpenAI Codex to help inspect code, draft/revise the patch. I reviewed the final code and take responsibility for it.

alexreinking · 2026-04-25T18:08:41Z

This description sounds written by AI. Per our contribution guidelines, please disclose any AI tool usage or assert that none was used.

xFile3160 · 2026-04-25T20:55:10Z

Indeed. I did not review yet. Please don't until I do and I update description. Those things work on their own now

alexreinking · 2026-04-27T17:43:46Z

@xFile3160 -- when ready for review, change the status away from Draft.

derek-gerstmann

Thanks for the contribution! I suggested a few minor changes, but otherwise looks great.

@alexreinking FYI ... this PR adds new methods to the runtime api, so we should flag this for the release notes.

derek-gerstmann · 2026-05-04T19:05:40Z

    int32_t offset = 0;  //< indexing offset from start of region (used to adjust indices in compute shader to avoid alignment constraints for arbitrary crops)
 };

+enum class MemoryRegionKind {


'Kind' is pretty vague. Perhaps rename this enum to something like MemoryOwnership.

I'd change AllocatorOwned to just Owned, and ExternalWrapped to Wrapped.

yep I agree will change this.

derek-gerstmann · 2026-05-04T19:10:04Z

+ * owned. The device field of the halide_buffer_t must be 0 when this routine
+ * is called.
+ */
+extern int halide_vulkan_wrap_vk_buffer_with_offset(void *user_context,


We don't need two methods for this ... just keep this one with the offset parameter and rename it halide_vulkan_wrap_vk_buffer and delete the other one.

yea I agree. I did the split because I wanted to preserve the zero-offset call.

derek-gerstmann · 2026-05-04T19:10:44Z

    (void *)&halide_vulkan_initialize_kernels,
    (void *)&halide_vulkan_release_context,
    (void *)&halide_vulkan_run,
+    (void *)&halide_vulkan_wrap_vk_buffer,


We only need one of these, remove the second.

derek-gerstmann · 2026-05-04T19:12:54Z

+namespace Internal {
+namespace Vulkan {
+
+ALWAYS_INLINE const MemoryRegion *vk_region_root(const MemoryRegion *region) {


This isn't returning the root ... it's returning the owner. Rename it vk_region_owner

How is this different than ctx.allocator->owner_of()?

So I needed an helper to follow crop aliases back to the owning region. A cropped buffer gets a MemoryRegion that is just metadata pointing back to the owning region. When we wrap buffers, we need to walk through any crop aliases to find the actual external wrapper region that owns the vkBuffer metadata and base offset. I think I encountered this specifically when dealing with nv21/nv12.

derek-gerstmann · 2026-05-04T19:14:29Z

+    return current;
+}
+
+ALWAYS_INLINE MemoryRegion *vk_region_root(MemoryRegion *region) {


Remove this ... overloading the same method is error prone, and the const casting is bad.

derek-gerstmann · 2026-05-04T19:22:32Z

+    }
+
+    if (region != root && region->kind == MemoryRegionKind::CropAlias) {
+        free(region);


You shouldn't be calling free() directly ... use vk_host_free() and pass on the allocation callbacks for the context.

yep agreed, you're right. This was host metadata tho, but I will use vk_host_malloc and vk_host_free

derek-gerstmann · 2026-05-04T19:23:09Z

+        return nullptr;
+    }
+
+    MemoryRegion *region = reinterpret_cast<MemoryRegion *>(malloc(sizeof(MemoryRegion)));


You shouldn't be calling malloc() or free() directly ... use vk_host_malloc() and vk_host_free() and pass on the allocation callbacks for the context.

derek-gerstmann · 2026-05-04T19:26:03Z

    // get the allocated region for the device
    MemoryRegion *device_region = reinterpret_cast<MemoryRegion *>(halide_buffer->device);
+#ifdef DEBUG_RUNTIME
+    const uint64_t device_region_size = device_region->allocation.size;


If this is only for printing a debug message, then move the declaration and the ifdef down below and wrap the debug() call.

derek-gerstmann · 2026-05-04T19:30:49Z

    return halide_error_code_success;
 }

+WEAK int halide_vulkan_wrap_vk_buffer(void *user_context, struct halide_buffer_t *buf, uint64_t vk_buffer) {


We don't need two methods for this. Remove this one, and rename the other one to halide_vulkan_wrap_vk_buffer

derek-gerstmann · 2026-05-04T19:32:25Z

    }

    MemoryRegion *owner = owner_of(user_context, region);
+    if (owner != nullptr && owner->kind == MemoryRegionKind::ExternalWrapped) {


What's wrong with mapping an externally wrapped buffer()?

so the halide allocator map() expects allovcator owned BlockResource and VkDeviceMemory. For external wrapped buffer, Halide does not know memory handle, bind offset, memory properties, host visibility, coherency and ownership. Wouldn't be unsafe to allow this?

xFile3160 · 2026-05-18T06:45:19Z

@derek-gerstmann I also updated this one, rebased it on main.
I've addressed the naming feedback for MemoryOwnership, Owned and Wrapped. I have also cropAlias: crops are view metadata not root allocations. A crop can point back to either owned or wrapped root.
I kept the wrap_vk_buffer(..., offset) and the old one now is internal glue calling this with offset = 0.
I also replaced direct malloc/free with vk_host_malloc/free like you pointed out. I reused the owner_of of the VulkanMemoryAllocator because the helper wasn't needed, was doing exactly same thing. The detach/free only releases halide metadata not the vkBuffer when external, and added test coverage.
To answer your question about map/unmap: it's not safe to map wrapped external buffers because halide has only the extenral vkbuffer. It doesn't know or own VkDeviceMemory, properties, host visibility etc.
Thanks again for all the feedback.

xFile3160 mentioned this pull request Apr 25, 2026

Vulkan: export allocator for custom contexts #8871

Open

alexreinking marked this pull request as draft April 27, 2026 17:43

xFile3160 force-pushed the vulkan-import-buffer-offsets branch from 1398255 to d7ca1b1 Compare April 28, 2026 19:27

xFile3160 marked this pull request as ready for review April 28, 2026 20:28

alexreinking requested review from derek-gerstmann and halidebuildbots May 4, 2026 17:20

derek-gerstmann requested changes May 4, 2026

View reviewed changes

derek-gerstmann added enhancement New user-visible features or improvements to existing features. gpu labels May 4, 2026

alexreinking added the release_notes For changes that may warrant a note in README for official releases. label May 4, 2026

Vulkan: wrap external buffers as regions

a4dac01

xFile3160 force-pushed the vulkan-import-buffer-offsets branch from fa83aad to a4dac01 Compare May 18, 2026 06:45

Conversation

xFile3160 commented Apr 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexreinking commented Apr 25, 2026

Uh oh!

xFile3160 commented Apr 25, 2026

Uh oh!

alexreinking commented Apr 27, 2026

Uh oh!

derek-gerstmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xFile3160 commented May 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

xFile3160 commented Apr 25, 2026 •

edited

Loading