Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow external memory support with DeviceLocalBuffers #1506

Merged
merged 8 commits into from
Mar 14, 2021
Merged

Allow external memory support with DeviceLocalBuffers #1506

merged 8 commits into from
Mar 14, 2021

Conversation

hakolao
Copy link
Contributor

@hakolao hakolao commented Mar 9, 2021 •
edited
Loading

Motivation: DeviceLocalBuffer wraps a lot of functionality inside that would be convenient if it could be used with exported DeviceMemory. External memory support was added in #1467, but it didn't really expose easy ways to use this with existing buffer functionality (or at least did not document how). This PR binds this external memory support to DeviceLocalBuffer which is useful e.g. if one wants to fill / modify / compute with CUDA, but render with Vulkano. Currently though only on Linux.

  • Added an entry to CHANGELOG_VULKANO.md or CHANGELOG_VK_SYS.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes - in this repository
  • Updated documentation to reflect any user-facing changes - PR to the guide that fixes existing documentation invalidated by this PR
  • Ran cargo fmt on the changes

We use this for a CUDA-Vulkan interoperable buffer in the following manner:

// DeviceLocalBuffer for rendering, CUdeviceptr (u64) for filling vertices & indices (based on our computations)
fn create_shared_buffer<T>(
    device: Arc<Device>,
    length: usize,
) -> (Arc<DeviceLocalBuffer<[T]>>, CUdeviceptr) {
    let mem_size = length * std::mem::size_of::<T>();
    // Create vulkan device local buffer
    let vk_buffer = unsafe {
        DeviceLocalBuffer::<[T]>::raw_with_exportable_fd(
            device.clone(),
            mem_size,
            BufferUsage::all(),
            vec![],
        )
        .expect("Failed to allocate device local buffer")
    };
    // Create posix handle out of vk buffer
    let vk_handle = buffer.export_posix_fd().expect("Failed to export posix handle");
    // Create CUDA buffer: https://docs.nvidia.com/cuda/cuda-driver-api/group__CUDA__EXTRES__INTEROP.html
    // with cuImportExternalMemory and cuExternalMemoryGetMappedBuffer
    let cu_buffer = cu_import_external_memory(vk_handle, mem_size)
        .expect("Failed to import external memory to cuda");
    (vk_buffer, cu_buffer)
}

I'm still a Vulkano beginner and don't know if there's a better way to do this, but for our use case this would be nice. This PR is an attempt to achieve this without any breaking changes and changes should only apply on Linux. I considered other ways, such as conditional inputs or an additional field to MemoryRequirements but neither of those seemed to cut it.

Let me know what would be the right way to do this, if this isn't such. Feel free to modify this PR directly.

@hakolao hakolao marked this pull request as ready for review March 9, 2021 14:15
@gurchetansingh
Copy link
Contributor

Yeah, I think we need to eventually untie memory allocation, memory mapping and buffer creation in Vulkano, to give developers a lower level of control. In particular, we should not be doing separate operations "alloc_and_map". Ideally, the developer can use a builder to create a Vulkan object (memory and buffers are separate objects), and internal Vulkano state tracking would ensure safety or panic!() on a violation.

In your particular case, we would need a "Buffer" class and a "DeviceMemory" class that can be bound separately rather than a "DeviceLocalBuffer". We would also need a "DeviceMemoryMapping" to wrap the raw ptr.

This is a pre-existing problem and should not hold up your MR, but something to think about for Vulkano developers long term.

@Rua
Copy link
Contributor

Rua commented Mar 12, 2021

In your particular case, we would need a "Buffer" class and a "DeviceMemory" class that can be bound separately rather than a "DeviceLocalBuffer". We would also need a "DeviceMemoryMapping" to wrap the raw ptr.

I definitely think this is a good idea!

@hakolao
Copy link
Contributor Author

hakolao commented Mar 12, 2021

In your particular case, we would need a "Buffer" class and a "DeviceMemory" class that can be bound separately rather than a "DeviceLocalBuffer". We would also need a "DeviceMemoryMapping" to wrap the raw ptr.

Yes this would be great!

So what to do with this PR meanwhile?

@Eliah-Lakhin Eliah-Lakhin merged commit 0e581c0 into vulkano-rs:master Mar 14, 2021
@Eliah-Lakhin
Copy link
Contributor

@hakolao Pull Request reviewed and merged. Thank you very much for your contribution! 👍

@gurchetansingh
Copy link
Contributor

P.S: by popular demand, I added "DeviceMemoryMapping"

#1510

I'll leave the Buffer and Image work to someone else though :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hakolao @gurchetansingh @Rua @Eliah-Lakhin