From 006ade87c958a559e674431b89cd80625add7beb Mon Sep 17 00:00:00 2001 From: Gray Olson Date: Wed, 10 May 2023 14:14:37 +0200 Subject: [PATCH] Add support for `presser` (#138) * Add `presser` support for vulkan Allocation * ignore tmp dir * fmt + slight doc update * user presser in imgui example renderer * make it prettier and add presser to dev-dependencies * fix example code a bit * add presser feature to ci * Update src/vulkan/presser.rs Co-authored-by: Marijn Suijten * > to greater than * add "presser" to vulkan-visualization required features * remove ```rust * make presser not optional * remove redundant presser mod * impl Slab on Allocation directly * revert gitignore --------- Co-authored-by: Marijn Suijten --- Cargo.toml | 1 + .../vulkan-visualization/imgui_renderer.rs | 86 ++++----- src/vulkan/mod.rs | 166 ++++++++++++++++++ 3 files changed, 202 insertions(+), 51 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 82054150..40043063 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -22,6 +22,7 @@ include = [ backtrace = "0.3" log = "0.4" thiserror = "1.0" +presser = { version = "0.3" } # Only needed for vulkan. Disable all default features as good practice, # such as the ability to link/load a Vulkan library. ash = { version = ">=0.34, <=0.37", optional = true, default-features = false, features = ["debug"] } diff --git a/examples/vulkan-visualization/imgui_renderer.rs b/examples/vulkan-visualization/imgui_renderer.rs index 931c3320..f32626fd 100644 --- a/examples/vulkan-visualization/imgui_renderer.rs +++ b/examples/vulkan-visualization/imgui_renderer.rs @@ -310,7 +310,7 @@ impl ImGuiRenderer { let image_view = unsafe { device.create_image_view(&view_create_info, None) }?; // Create upload buffer - let (upload_buffer, upload_buffer_memory) = { + let (upload_buffer, mut upload_buffer_memory) = { let create_info = vk::BufferCreateInfo::builder() .size((font_atlas.width * font_atlas.height * 4) as u64) .usage(vk::BufferUsageFlags::TRANSFER_SRC); @@ -340,14 +340,10 @@ impl ImGuiRenderer { }; // Copy font data to upload buffer - let dst = upload_buffer_memory.mapped_ptr().unwrap().cast().as_ptr(); - unsafe { - std::ptr::copy_nonoverlapping( - font_atlas.data.as_ptr(), - dst, - (font_atlas.width * font_atlas.height * 4) as usize, - ); - } + let copy_record = + presser::copy_from_slice_to_offset(font_atlas.data, &mut upload_buffer_memory, 0) + .unwrap(); + assert_eq!(copy_record.copy_start_offset, 0); // Copy upload buffer to image record_and_submit_command_buffer( @@ -638,13 +634,9 @@ impl ImGuiRenderer { ], }; - unsafe { - std::ptr::copy_nonoverlapping( - &cbuffer_data, - self.cb_allocation.mapped_ptr().unwrap().cast().as_ptr(), - 1, - ) - }; + let copy_record = + presser::copy_to_offset(&cbuffer_data, &mut self.cb_allocation, 0).unwrap(); + assert_eq!(copy_record.copy_start_offset, 0); } let render_pass_begin_info = vk::RenderPassBeginInfo::builder() @@ -721,49 +713,41 @@ impl ImGuiRenderer { let mut ib_offset = 0; for draw_list in imgui_draw_data.draw_lists() { - unsafe { - device.cmd_bind_vertex_buffers( - cmd, - 0, - &[self.vertex_buffer], - &[vb_offset as u64 * std::mem::size_of::() as u64], - ) - }; - unsafe { - device.cmd_bind_index_buffer( - cmd, - self.index_buffer, - ib_offset as u64 * std::mem::size_of::() as u64, - vk::IndexType::UINT16, - ) - }; - { let vertices = draw_list.vtx_buffer(); - let dst_ptr = self - .vb_allocation - .mapped_ptr() - .unwrap() - .cast::() - .as_ptr(); - let dst_ptr = unsafe { dst_ptr.offset(vb_offset) }; + let copy_record = presser::copy_from_slice_to_offset( + vertices, + &mut self.vb_allocation, + vb_offset, + ) + .unwrap(); + vb_offset = copy_record.copy_end_offset_padded; + unsafe { - std::ptr::copy_nonoverlapping(vertices.as_ptr(), dst_ptr, vertices.len()) + device.cmd_bind_vertex_buffers( + cmd, + 0, + &[self.vertex_buffer], + &[copy_record.copy_start_offset as _], + ) }; - vb_offset += vertices.len() as isize; } { let indices = draw_list.idx_buffer(); - let dst_ptr = self - .ib_allocation - .mapped_ptr() - .unwrap() - .cast::() - .as_ptr(); - let dst_ptr = unsafe { dst_ptr.offset(ib_offset) }; - unsafe { std::ptr::copy_nonoverlapping(indices.as_ptr(), dst_ptr, indices.len()) }; - ib_offset += indices.len() as isize; + let copy_record = + presser::copy_from_slice_to_offset(indices, &mut self.ib_allocation, ib_offset) + .unwrap(); + ib_offset = copy_record.copy_end_offset_padded; + + unsafe { + device.cmd_bind_index_buffer( + cmd, + self.index_buffer, + copy_record.copy_start_offset as _, + vk::IndexType::UINT16, + ) + }; } for command in draw_list.commands() { diff --git a/src/vulkan/mod.rs b/src/vulkan/mod.rs index 0a927879..5b01e9a2 100644 --- a/src/vulkan/mod.rs +++ b/src/vulkan/mod.rs @@ -62,6 +62,90 @@ pub struct AllocatorCreateDesc { pub buffer_device_address: bool, } +/// A piece of allocated memory. +/// +/// Could be contained in its own individual underlying memory object or as a sub-region +/// of a larger allocation. +/// +/// # Copying data into a CPU-mapped [`Allocation`] +/// +/// You'll very likely want to copy data into CPU-mapped [`Allocation`]s in order to send that data to the GPU. +/// Doing this data transfer correctly without invoking undefined behavior can be quite fraught and non-obvious[\[1\]]. +/// +/// To help you do this correctly, [`Allocation`] implements [`presser::Slab`], which means you can directly +/// pass it in to many of `presser`'s [helper functions] (for example, [`copy_from_slice_to_offset`]). +/// +/// In most cases, this will work perfectly. However, note that if you try to use an [`Allocation`] as a +/// [`Slab`] and it is not valid to do so (if it is not CPU-mapped or if its `size > isize::MAX`), +/// you will cause a panic. If you aren't sure about these conditions, you may use [`Allocation::try_as_mapped_slab`]. +/// +/// ## Example +/// +/// Say we've created an [`Allocation`] called `my_allocation`, which is CPU-mapped. +/// ```ignore +/// let mut my_allocation: Allocation = my_allocator.allocate(...)?; +/// ``` +/// +/// And we want to fill it with some data in the form of a `my_gpu_data: Vec`, defined as such: +/// +/// ```ignore +/// // note that this is size(12) but align(16), thus we have 4 padding bytes. +/// // this would mean a `&[MyGpuVector]` is invalid to cast as a `&[u8]`, but +/// // we can still use `presser` to copy it directly in a valid manner. +/// #[repr(C, align(16))] +/// #[derive(Clone, Copy)] +/// struct MyGpuVertex { +/// x: f32, +/// y: f32, +/// z: f32, +/// } +/// +/// let my_gpu_data: Vec = make_vertex_data(); +/// ``` +/// +/// Depending on how the data we're copying will be used, the vulkan device may have a minimum +/// alignment requirement for that data: +/// +/// ```ignore +/// let min_gpu_align = my_vulkan_device_specifications.min_alignment_thing; +/// ``` +/// +/// Finally, we can use [`presser::copy_from_slice_to_offset_with_align`] to perform the copy, +/// simply passing `&mut my_allocation` since [`Allocation`] implements [`Slab`]. +/// +/// ```ignore +/// let copy_record = presser::copy_from_slice_to_offset_with_align( +/// &my_gpu_data[..], // a slice containing all elements of my_gpu_data +/// &mut my_allocation, // our Allocation +/// 0, // start as close to the beginning of the allocation as possible +/// min_gpu_align, // the minimum alignment we queried previously +/// )?; +/// ``` +/// +/// It's important to note that the data may not have actually been copied starting at the requested +/// `start_offset` (0 in the example above) depending on the alignment of the underlying allocation +/// as well as the alignment requirements of `MyGpuVector` and the `min_gpu_align` we passed in. Thus, +/// we can query the `copy_record` for the actual starting offset: +/// +/// ```ignore +/// let actual_data_start_offset = copy_record.copy_start_offset; +/// ``` +/// +/// ## Safety +/// +/// It is technically not fully safe to use an [`Allocation`] as a [`presser::Slab`] because we can't validate that the +/// GPU is not using the data in the buffer while `self` is borrowed. However, trying +/// to validate this statically is really hard and the community has basically decided that +/// requiring `unsafe` for functions like this creates too much "unsafe-noise", ultimately making it +/// harder to debug more insidious unsafety that is unrelated to GPU-CPU sync issues. +/// +/// So, as would always be the case, you must ensure the GPU +/// is not using the data in `self` for the duration that you hold the returned [`MappedAllocationSlab`]. +/// +/// [`Slab`]: presser::Slab +/// [`copy_from_slice_to_offset`]: presser::copy_from_slice_to_offset +/// [helper functions]: presser#functions +/// [\[1\]]: presser#motivation #[derive(Debug)] pub struct Allocation { chunk_id: Option, @@ -77,6 +161,39 @@ pub struct Allocation { } impl Allocation { + /// Tries to borrow the CPU-mapped memory that backs this allocation as a [`presser::Slab`], which you can then + /// use to safely copy data into the raw, potentially-uninitialized buffer. + /// See [the documentation of Allocation][Allocation#example] for an example of this. + /// + /// Returns [`None`] if `self.mapped_ptr()` is `None`, or if `self.size()` is greater than `isize::MAX` because + /// this could lead to undefined behavior. + /// + /// Note that [`Allocation`] implements [`Slab`] natively, so you can actually pass this allocation as a [`Slab`] + /// directly. However, if `self` is not actually a valid [`Slab`] (this function would return `None` as described above), + /// then trying to use it as a [`Slab`] will panic. + /// + /// # Safety + /// + /// See the note about safety in [the documentation of Allocation][Allocation#safety] + /// + /// [`Slab`]: presser::Slab + pub fn try_as_mapped_slab(&mut self) -> Option> { + let mapped_ptr = self.mapped_ptr()?.cast().as_ptr(); + + if self.size > isize::MAX as _ { + return None; + } + + // this will always succeed since size is <= isize::MAX which is < usize::MAX + let size = self.size as usize; + + Some(MappedAllocationSlab { + _borrowed_alloc: self, + mapped_ptr, + size, + }) + } + pub fn chunk_id(&self) -> Option { self.chunk_id } @@ -152,6 +269,55 @@ impl Default for Allocation { } } +/// A wrapper struct over a borrowed [`Allocation`] that infallibly implements [`presser::Slab`]. +/// +/// This type should be acquired by calling [`Allocation::try_as_mapped_slab`]. +pub struct MappedAllocationSlab<'a> { + _borrowed_alloc: &'a mut Allocation, + mapped_ptr: *mut u8, + size: usize, +} + +// SAFETY: See the safety comment of Allocation::as_mapped_slab above. +unsafe impl<'a> presser::Slab for MappedAllocationSlab<'a> { + fn base_ptr(&self) -> *const u8 { + self.mapped_ptr + } + + fn base_ptr_mut(&mut self) -> *mut u8 { + self.mapped_ptr + } + + fn size(&self) -> usize { + self.size + } +} + +// SAFETY: See the safety comment of Allocation::as_mapped_slab above. +unsafe impl presser::Slab for Allocation { + fn base_ptr(&self) -> *const u8 { + self.mapped_ptr + .expect("tried to use a non-mapped Allocation as a Slab") + .as_ptr() + .cast() + } + + fn base_ptr_mut(&mut self) -> *mut u8 { + self.mapped_ptr + .expect("tried to use a non-mapped Allocation as a Slab") + .as_ptr() + .cast() + } + + fn size(&self) -> usize { + if self.size > isize::MAX as _ { + panic!("tried to use an Allocation with size > isize::MAX as a Slab") + } + // this will always work if the above passed + self.size as usize + } +} + #[derive(Debug)] pub(crate) struct MemoryBlock { pub(crate) device_memory: vk::DeviceMemory,