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

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

This fixes the Vulkan native-buffer wrapper path.

The issue in #8715 started from trying to pass an application-owned VkBuffer
to Halide. The only way to make that work reliably was to mirror Halide's
private MemoryRegion layout in application code and pass that through
halide_vulkan_wrap_vk_buffer. That is not a good API boundary.

halide_vulkan_wrap_vk_buffer should accept a Vulkan buffer handle and let
Halide build whatever runtime metadata it needs internally.

This PR changes the wrapper path to do that. External VkBuffer handles are
wrapped in small runtime-owned MemoryRegion metadata. The Vulkan buffer
remains owned by the caller; Halide only owns the metadata needed to pass the
buffer through descriptor updates, copies, crops, and detach/free.

The intended flow is:

The application allocates and owns a VkBuffer.
The application wraps it into a halide_buffer_t.
Halide creates its own metadata for the wrapped buffer.
Halide uses that metadata internally.
Detach/free releases only Halide's wrapper metadata, not the external
VkBuffer.

This also adds halide_vulkan_wrap_vk_buffer_with_offset for views into larger
Vulkan buffers. The explicit external offset is included in descriptor updates
and copy paths. Regular Halide crops continue to use the existing indexing
offset path.

The GPU object lifetime test now covers Vulkan native buffer wrapping. It
rewraps a Vulkan buffer handle, then wraps the same buffer with an explicit byte
offset and verifies that the shifted view copies back the expected values.

Refs #8715

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.

halide_vulkan_wrap_vk_buffer stored the raw VkBuffer in halide_buffer_t::device, but the Vulkan runtime expects that field to point at MemoryRegion metadata. Wrap external VkBuffer handles in small runtime-owned MemoryRegion metadata instead. Detach/free release only that metadata, not the external buffer. Add an explicit-offset wrap helper for views into larger buffers, and include that offset in descriptor updates and copy paths. Refs halide#8715

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 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

Vulkan: test wrapped buffer offsets

fa83aad

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

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

xFile3160 commented Apr 25, 2026 •

edited

Loading