From 127ffeefa80e27f1d176d81c111a8a082d5c9e68 Mon Sep 17 00:00:00 2001 From: Gray Olson Date: Mon, 17 Oct 2022 18:29:31 +0200 Subject: [PATCH 01/15] Add `presser` support for vulkan Allocation --- Cargo.toml | 3 +- src/vulkan/mod.rs | 3 ++ src/vulkan/presser.rs | 83 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 88 insertions(+), 1 deletion(-) create mode 100644 src/vulkan/presser.rs diff --git a/Cargo.toml b/Cargo.toml index 17c8ad38..c6210f21 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", optional = true } # 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"] } @@ -94,4 +95,4 @@ d3d12 = ["windows"] # Expose helper functionality for winapi types to interface with gpu-allocator, which is primarily windows-rs driven public-winapi = ["dep:winapi"] -default = ["d3d12", "vulkan"] +default = ["d3d12", "vulkan", "presser"] diff --git a/src/vulkan/mod.rs b/src/vulkan/mod.rs index 6e543501..fb59a635 100644 --- a/src/vulkan/mod.rs +++ b/src/vulkan/mod.rs @@ -5,6 +5,9 @@ mod visualizer; #[cfg(feature = "visualizer")] pub use visualizer::AllocatorVisualizer; +#[cfg(feature = "presser")] +mod presser; + use super::allocator; use super::allocator::AllocationType; use ash::vk; diff --git a/src/vulkan/presser.rs b/src/vulkan/presser.rs new file mode 100644 index 00000000..623318d7 --- /dev/null +++ b/src/vulkan/presser.rs @@ -0,0 +1,83 @@ +use super::Allocation; +use core::convert::TryFrom; + +impl Allocation { + /// 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. + /// + /// # Example + /// + /// ```rust,ignore + /// #[repr(C, align(16))] + /// #[derive(Clone, Copy)] + /// struct MyGpuVector { + /// x: f32, + /// y: f32, + /// z: f32, + /// } + /// + /// // Create some data to be sent to the GPU. Note this must be formatted correctly in terms of + /// // alignment of individual items and etc, as usual. + /// let my_gpu_data: &[MyGpuVector] = get_vertex_data(); + /// + /// // Get a `presser::Slab` from our gpu_allocator::Allocation + /// let alloc_slab = my_allocation.as_mapped_slab(); + /// + /// // depending on the type of data you're copying, your vulkan device may have a minimum + /// // alignment requirement for that data + /// let min_gpu_align = my_vulkan_device_specifications.min_alignment_thing; + /// + /// let copy_record = presser::copy_from_slice_to_offset_with_align( + /// my_gpu_data, + /// alloc_slab, + /// 0, // start as close to the beginning of the allocation as possible + /// min_gpu_align, + /// ); + /// + /// // the data may not have actually been copied starting at the requested start offset + /// // depending on the alignment of the underlying allocation, as well as the alignment requirements of + /// // `MyGpuVector` and the `min_gpu_align` we passed in + /// let actual_data_start_offset = copy_record.copy_start_offset; + /// ``` + /// + /// # Safety + /// + /// This is technically not fully safe 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 just calling stuff like this is fine. So, as would always be the case, ensure the GPU + /// is not using the data in `self` before calling this function. + pub fn as_mapped_slab(&mut self) -> Option> { + let mapped_ptr = self.mapped_ptr()?.cast().as_ptr(); + // size > isize::MAX is disallowed by `Slab` for safety reasons + let size = isize::try_from(self.size()).ok()?; + // this will always succeed since size can only be positive and < isize::MAX + let size = size as usize; + + Some(MappedAllocationSlab { _borrowed_alloc: self, mapped_ptr, size }) + } +} + +/// A wrapper struct over a borrowed [`Allocation`] that implements [`presser::Slab`]. +/// +/// This type should be acquired by calling [`Allocation::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 + } +} \ No newline at end of file From d3db3bbf175401b1b8528354a00acea1e9d4d8c0 Mon Sep 17 00:00:00 2001 From: Gray Olson Date: Mon, 17 Oct 2022 18:29:39 +0200 Subject: [PATCH 02/15] ignore tmp dir --- .gitignore | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.gitignore b/.gitignore index 877682d7..811f57e2 100644 --- a/.gitignore +++ b/.gitignore @@ -2,6 +2,10 @@ # will have compiled files and executables /target/ +# A semi-common pattern is to have rust-analyzer compile into a separate target +# directory like /tmp so that it doesn't conflict with other instances of cargo +/tmp + # Remove Cargo.lock from gitignore if creating an executable, leave it for libraries # More information here https://doc.rust-lang.org/cargo/guide/cargo-toml-vs-cargo-lock.html Cargo.lock From 2b5e06095a787d784d1cc0ab6d6d55a43d67d905 Mon Sep 17 00:00:00 2001 From: Gray Olson Date: Mon, 17 Oct 2022 18:33:26 +0200 Subject: [PATCH 03/15] fmt + slight doc update --- src/vulkan/presser.rs | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/src/vulkan/presser.rs b/src/vulkan/presser.rs index 623318d7..855f9f35 100644 --- a/src/vulkan/presser.rs +++ b/src/vulkan/presser.rs @@ -4,9 +4,12 @@ use core::convert::TryFrom; impl Allocation { /// 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. - /// + /// + /// Returns `None` if `self.mapped_ptr()` is `None`, or if `self.size()` is > `isize::MAX` because + /// this could lead to undefined behavior. + /// /// # Example - /// + /// /// ```rust,ignore /// #[repr(C, align(16))] /// #[derive(Clone, Copy)] @@ -15,33 +18,33 @@ impl Allocation { /// y: f32, /// z: f32, /// } - /// + /// /// // Create some data to be sent to the GPU. Note this must be formatted correctly in terms of /// // alignment of individual items and etc, as usual. /// let my_gpu_data: &[MyGpuVector] = get_vertex_data(); - /// + /// /// // Get a `presser::Slab` from our gpu_allocator::Allocation - /// let alloc_slab = my_allocation.as_mapped_slab(); - /// + /// let alloc_slab = my_allocation.as_mapped_slab().unwrap(); + /// /// // depending on the type of data you're copying, your vulkan device may have a minimum /// // alignment requirement for that data /// let min_gpu_align = my_vulkan_device_specifications.min_alignment_thing; - /// + /// /// let copy_record = presser::copy_from_slice_to_offset_with_align( /// my_gpu_data, /// alloc_slab, /// 0, // start as close to the beginning of the allocation as possible /// min_gpu_align, /// ); - /// + /// /// // the data may not have actually been copied starting at the requested start offset /// // depending on the alignment of the underlying allocation, as well as the alignment requirements of /// // `MyGpuVector` and the `min_gpu_align` we passed in /// let actual_data_start_offset = copy_record.copy_start_offset; /// ``` - /// + /// /// # Safety - /// + /// /// This is technically not fully safe 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 @@ -54,12 +57,16 @@ impl Allocation { // this will always succeed since size can only be positive and < isize::MAX let size = size as usize; - Some(MappedAllocationSlab { _borrowed_alloc: self, mapped_ptr, size }) + Some(MappedAllocationSlab { + _borrowed_alloc: self, + mapped_ptr, + size, + }) } } /// A wrapper struct over a borrowed [`Allocation`] that implements [`presser::Slab`]. -/// +/// /// This type should be acquired by calling [`Allocation::as_mapped_slab`]. pub struct MappedAllocationSlab<'a> { _borrowed_alloc: &'a mut Allocation, @@ -80,4 +87,4 @@ unsafe impl<'a> presser::Slab for MappedAllocationSlab<'a> { fn size(&self) -> usize { self.size } -} \ No newline at end of file +} From 75e2e8c6608e63e367e062765498d8c0f958d5ab Mon Sep 17 00:00:00 2001 From: Gray Olson Date: Tue, 18 Oct 2022 13:17:45 +0200 Subject: [PATCH 04/15] user presser in imgui example renderer --- .../vulkan-visualization/imgui_renderer.rs | 57 +++++++------------ 1 file changed, 21 insertions(+), 36 deletions(-) diff --git a/examples/vulkan-visualization/imgui_renderer.rs b/examples/vulkan-visualization/imgui_renderer.rs index 9f30ab9e..d07504af 100644 --- a/examples/vulkan-visualization/imgui_renderer.rs +++ b/examples/vulkan-visualization/imgui_renderer.rs @@ -309,7 +309,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); @@ -338,14 +338,8 @@ 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 mut slab = upload_buffer_memory.as_mapped_slab().unwrap(); + presser::copy_from_slice_to_offset(font_atlas.data, &mut slab, 0).unwrap(); // Copy upload buffer to image record_and_submit_command_buffer( @@ -633,13 +627,13 @@ 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.as_mapped_slab().unwrap(), + 0, + ) + .unwrap(); + assert_eq!(copy_record.copy_start_offset, 0); } let render_pass_begin_info = vk::RenderPassBeginInfo::builder() @@ -715,6 +709,9 @@ impl ImGuiRenderer { let mut vb_offset = 0; let mut ib_offset = 0; + let mut vb_slab = self.vb_allocation.as_mapped_slab().unwrap(); + let mut ib_slab = self.ib_allocation.as_mapped_slab().unwrap(); + for draw_list in imgui_draw_data.draw_lists() { unsafe { device.cmd_bind_vertex_buffers( @@ -735,30 +732,18 @@ impl ImGuiRenderer { { 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) }; - unsafe { - std::ptr::copy_nonoverlapping(vertices.as_ptr(), dst_ptr, vertices.len()) - }; - vb_offset += vertices.len() as isize; + let copy_record = + presser::copy_from_slice_to_offset(vertices, &mut vb_slab, vb_offset).unwrap(); + assert_eq!(copy_record.copy_start_offset, vb_offset); + vb_offset = copy_record.copy_end_offset_padded; } { 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 ib_slab, ib_offset).unwrap(); + assert_eq!(copy_record.copy_start_offset, ib_offset); + ib_offset = copy_record.copy_end_offset_padded; } for command in draw_list.commands() { From 293c3d9e35c57d5106b54d41645c81019b0f30a2 Mon Sep 17 00:00:00 2001 From: Gray Olson Date: Tue, 18 Oct 2022 13:38:44 +0200 Subject: [PATCH 05/15] make it prettier and add presser to dev-dependencies --- Cargo.toml | 1 + .../vulkan-visualization/imgui_renderer.rs | 37 +++++++++---------- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index c6210f21..64d557e8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -52,6 +52,7 @@ ash-window = "0.10.0" raw-window-handle = "0.4" winit = "0.26" imgui-winit-support = { version = "0.8", default-features = false, features = ["winit-26"] } +presser = "0.3" [target.'cfg(windows)'.dev-dependencies] winapi = { version = "0.3.9", features = ["d3d12", "d3d12sdklayers", "dxgi1_6", "winerror", "impl-default", "impl-debug", "winuser", "windowsx", "libloaderapi"] } diff --git a/examples/vulkan-visualization/imgui_renderer.rs b/examples/vulkan-visualization/imgui_renderer.rs index d07504af..d164f1ae 100644 --- a/examples/vulkan-visualization/imgui_renderer.rs +++ b/examples/vulkan-visualization/imgui_renderer.rs @@ -713,37 +713,36 @@ impl ImGuiRenderer { let mut ib_slab = self.ib_allocation.as_mapped_slab().unwrap(); 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 copy_record = presser::copy_from_slice_to_offset(vertices, &mut vb_slab, vb_offset).unwrap(); - assert_eq!(copy_record.copy_start_offset, vb_offset); vb_offset = copy_record.copy_end_offset_padded; + + unsafe { + device.cmd_bind_vertex_buffers( + cmd, + 0, + &[self.vertex_buffer], + &[copy_record.copy_start_offset as _], + ) + }; } { let indices = draw_list.idx_buffer(); let copy_record = presser::copy_from_slice_to_offset(indices, &mut ib_slab, ib_offset).unwrap(); - assert_eq!(copy_record.copy_start_offset, ib_offset); 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() { From be71ac2ebd1d039d2dfd663a6daa2fbb1e6be025 Mon Sep 17 00:00:00 2001 From: Gray Olson Date: Tue, 18 Oct 2022 13:40:43 +0200 Subject: [PATCH 06/15] fix example code a bit --- src/vulkan/presser.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/vulkan/presser.rs b/src/vulkan/presser.rs index 855f9f35..8c1c7e9a 100644 --- a/src/vulkan/presser.rs +++ b/src/vulkan/presser.rs @@ -24,7 +24,7 @@ impl Allocation { /// let my_gpu_data: &[MyGpuVector] = get_vertex_data(); /// /// // Get a `presser::Slab` from our gpu_allocator::Allocation - /// let alloc_slab = my_allocation.as_mapped_slab().unwrap(); + /// let mut alloc_slab = my_allocation.as_mapped_slab().unwrap(); /// /// // depending on the type of data you're copying, your vulkan device may have a minimum /// // alignment requirement for that data @@ -32,7 +32,7 @@ impl Allocation { /// /// let copy_record = presser::copy_from_slice_to_offset_with_align( /// my_gpu_data, - /// alloc_slab, + /// &mut alloc_slab, /// 0, // start as close to the beginning of the allocation as possible /// min_gpu_align, /// ); From 1b577ca9dc7cbd8d077ea63bdc41c46695c66da6 Mon Sep 17 00:00:00 2001 From: Gray Olson Date: Tue, 18 Oct 2022 13:43:54 +0200 Subject: [PATCH 07/15] add presser feature to ci --- .github/workflows/ci.yml | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0b94ae19..7f7d408a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -9,9 +9,9 @@ jobs: matrix: include: - os: ubuntu-latest - features: vulkan,visualizer + features: vulkan,visualizer,presser - os: windows-latest - features: vulkan,visualizer,d3d12,public-winapi + features: vulkan,visualizer,d3d12,public-winapi,presser runs-on: ${{ matrix.os }} steps: - uses: actions/checkout@v3 @@ -31,9 +31,9 @@ jobs: matrix: include: - os: ubuntu-latest - features: vulkan,visualizer + features: vulkan,visualizer,presser - os: windows-latest - features: vulkan,visualizer,d3d12,public-winapi + features: vulkan,visualizer,d3d12,public-winapi,presser runs-on: ${{ matrix.os }} steps: - uses: actions/checkout@v3 @@ -75,9 +75,9 @@ jobs: matrix: include: - os: ubuntu-latest - features: vulkan,visualizer + features: vulkan,visualizer,presser - os: windows-latest - features: vulkan,visualizer,d3d12,public-winapi + features: vulkan,visualizer,d3d12,public-winapi,presser runs-on: ${{ matrix.os }} steps: - uses: actions/checkout@v3 From 2c6e4b9deb1424c108cb7cd5409250ccc3e2ebc4 Mon Sep 17 00:00:00 2001 From: Gray Olson Date: Tue, 18 Oct 2022 15:26:55 +0200 Subject: [PATCH 08/15] Update src/vulkan/presser.rs Co-authored-by: Marijn Suijten --- src/vulkan/presser.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/vulkan/presser.rs b/src/vulkan/presser.rs index 8c1c7e9a..b4caa8fd 100644 --- a/src/vulkan/presser.rs +++ b/src/vulkan/presser.rs @@ -5,7 +5,7 @@ impl Allocation { /// 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. /// - /// Returns `None` if `self.mapped_ptr()` is `None`, or if `self.size()` is > `isize::MAX` because + /// Returns [`None`] if `self.mapped_ptr()` is `None`, or if `self.size()` is > `isize::MAX` because /// this could lead to undefined behavior. /// /// # Example From b19754ac9240fbfc80866209a02c0861bdff7529 Mon Sep 17 00:00:00 2001 From: Gray Olson Date: Tue, 18 Oct 2022 15:28:58 +0200 Subject: [PATCH 09/15] > to greater than --- src/vulkan/presser.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/vulkan/presser.rs b/src/vulkan/presser.rs index b4caa8fd..33deff96 100644 --- a/src/vulkan/presser.rs +++ b/src/vulkan/presser.rs @@ -5,7 +5,7 @@ impl Allocation { /// 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. /// - /// Returns [`None`] if `self.mapped_ptr()` is `None`, or if `self.size()` is > `isize::MAX` because + /// Returns [`None`] if `self.mapped_ptr()` is `None`, or if `self.size()` is greater than `isize::MAX` because /// this could lead to undefined behavior. /// /// # Example From 7575efacc3f7b08264b4aa5f0a6687c4cd19245d Mon Sep 17 00:00:00 2001 From: Gray Olson Date: Tue, 18 Oct 2022 15:29:22 +0200 Subject: [PATCH 10/15] add "presser" to vulkan-visualization required features --- Cargo.toml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 64d557e8..b82b3cc9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -52,7 +52,6 @@ ash-window = "0.10.0" raw-window-handle = "0.4" winit = "0.26" imgui-winit-support = { version = "0.8", default-features = false, features = ["winit-26"] } -presser = "0.3" [target.'cfg(windows)'.dev-dependencies] winapi = { version = "0.3.9", features = ["d3d12", "d3d12sdklayers", "dxgi1_6", "winerror", "impl-default", "impl-debug", "winuser", "windowsx", "libloaderapi"] } @@ -74,7 +73,7 @@ required-features = ["vulkan", "ash/loaded"] [[example]] name = "vulkan-visualization" -required-features = ["vulkan", "ash/loaded", "visualizer"] +required-features = ["vulkan", "ash/loaded", "visualizer", "presser"] [[example]] name = "d3d12-buffer" From 8376990f8b38aa26f00cbc581eb3c96e6c77ec92 Mon Sep 17 00:00:00 2001 From: Gray Olson Date: Tue, 18 Oct 2022 15:29:55 +0200 Subject: [PATCH 11/15] remove ```rust --- src/vulkan/presser.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/vulkan/presser.rs b/src/vulkan/presser.rs index 33deff96..1f50286b 100644 --- a/src/vulkan/presser.rs +++ b/src/vulkan/presser.rs @@ -10,7 +10,7 @@ impl Allocation { /// /// # Example /// - /// ```rust,ignore + /// ```ignore /// #[repr(C, align(16))] /// #[derive(Clone, Copy)] /// struct MyGpuVector { From 710fa71d3045c268556644a6a9b462d2237f6128 Mon Sep 17 00:00:00 2001 From: Gray Olson Date: Wed, 19 Oct 2022 14:16:15 +0200 Subject: [PATCH 12/15] make presser not optional --- .github/workflows/ci.yml | 12 ++++++------ Cargo.toml | 6 +++--- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7f7d408a..0b94ae19 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -9,9 +9,9 @@ jobs: matrix: include: - os: ubuntu-latest - features: vulkan,visualizer,presser + features: vulkan,visualizer - os: windows-latest - features: vulkan,visualizer,d3d12,public-winapi,presser + features: vulkan,visualizer,d3d12,public-winapi runs-on: ${{ matrix.os }} steps: - uses: actions/checkout@v3 @@ -31,9 +31,9 @@ jobs: matrix: include: - os: ubuntu-latest - features: vulkan,visualizer,presser + features: vulkan,visualizer - os: windows-latest - features: vulkan,visualizer,d3d12,public-winapi,presser + features: vulkan,visualizer,d3d12,public-winapi runs-on: ${{ matrix.os }} steps: - uses: actions/checkout@v3 @@ -75,9 +75,9 @@ jobs: matrix: include: - os: ubuntu-latest - features: vulkan,visualizer,presser + features: vulkan,visualizer - os: windows-latest - features: vulkan,visualizer,d3d12,public-winapi,presser + features: vulkan,visualizer,d3d12,public-winapi runs-on: ${{ matrix.os }} steps: - uses: actions/checkout@v3 diff --git a/Cargo.toml b/Cargo.toml index b82b3cc9..584be034 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -22,7 +22,7 @@ include = [ backtrace = "0.3" log = "0.4" thiserror = "1.0" -presser = { version = "0.3", optional = true } +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"] } @@ -73,7 +73,7 @@ required-features = ["vulkan", "ash/loaded"] [[example]] name = "vulkan-visualization" -required-features = ["vulkan", "ash/loaded", "visualizer", "presser"] +required-features = ["vulkan", "ash/loaded", "visualizer"] [[example]] name = "d3d12-buffer" @@ -95,4 +95,4 @@ d3d12 = ["windows"] # Expose helper functionality for winapi types to interface with gpu-allocator, which is primarily windows-rs driven public-winapi = ["dep:winapi"] -default = ["d3d12", "vulkan", "presser"] +default = ["d3d12", "vulkan"] From df1894dda5b041cfd832d30717b2f06b6ef7245c Mon Sep 17 00:00:00 2001 From: Gray Olson Date: Wed, 19 Oct 2022 15:17:33 +0200 Subject: [PATCH 13/15] remove redundant presser mod --- src/vulkan/mod.rs | 90 +++++++++++++++++++++++++++++++++++++++++++ src/vulkan/presser.rs | 90 ------------------------------------------- 2 files changed, 90 insertions(+), 90 deletions(-) delete mode 100644 src/vulkan/presser.rs diff --git a/src/vulkan/mod.rs b/src/vulkan/mod.rs index fb59a635..8548ed2a 100644 --- a/src/vulkan/mod.rs +++ b/src/vulkan/mod.rs @@ -13,6 +13,7 @@ use super::allocator::AllocationType; use ash::vk; use log::{debug, Level}; use std::fmt; +use core::convert::TryFrom; use crate::{ allocator::fmt_bytes, AllocationError, AllocatorDebugSettings, MemoryLocation, Result, @@ -60,6 +61,71 @@ unsafe impl Send for Allocation {} unsafe impl Sync for Allocation {} impl Allocation { + /// 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. + /// + /// Returns [`None`] if `self.mapped_ptr()` is `None`, or if `self.size()` is greater than `isize::MAX` because + /// this could lead to undefined behavior. + /// + /// # Example + /// + /// ```ignore + /// #[repr(C, align(16))] + /// #[derive(Clone, Copy)] + /// struct MyGpuVector { + /// x: f32, + /// y: f32, + /// z: f32, + /// } + /// + /// // Create some data to be sent to the GPU. Note this must be formatted correctly in terms of + /// // alignment of individual items and etc, as usual. + /// let my_gpu_data: &[MyGpuVector] = get_vertex_data(); + /// + /// // Get a `presser::Slab` from our gpu_allocator::Allocation + /// let mut alloc_slab = my_allocation.as_mapped_slab().unwrap(); + /// + /// // depending on the type of data you're copying, your vulkan device may have a minimum + /// // alignment requirement for that data + /// let min_gpu_align = my_vulkan_device_specifications.min_alignment_thing; + /// + /// let copy_record = presser::copy_from_slice_to_offset_with_align( + /// my_gpu_data, + /// &mut alloc_slab, + /// 0, // start as close to the beginning of the allocation as possible + /// min_gpu_align, + /// ); + /// + /// // the data may not have actually been copied starting at the requested start offset + /// // depending on the alignment of the underlying allocation, as well as the alignment requirements of + /// // `MyGpuVector` and the `min_gpu_align` we passed in. + /// let actual_data_start_offset = copy_record.copy_start_offset; + /// ``` + /// + /// # Safety + /// + /// This is technically not fully safe 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`]. + pub fn as_mapped_slab(&mut self) -> Option> { + let mapped_ptr = self.mapped_ptr()?.cast().as_ptr(); + // size > isize::MAX is disallowed by `Slab` for safety reasons + let size = isize::try_from(self.size()).ok()?; + // this will always succeed since size can only be positive and < isize::MAX + let size = size as usize; + + Some(MappedAllocationSlab { + _borrowed_alloc: self, + mapped_ptr, + size, + }) + } + pub fn chunk_id(&self) -> Option { self.chunk_id } @@ -129,6 +195,30 @@ impl Default for Allocation { } } +/// A wrapper struct over a borrowed [`Allocation`] that implements [`presser::Slab`]. +/// +/// This type should be acquired by calling [`Allocation::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 + } +} + #[derive(Debug)] pub(crate) struct MemoryBlock { pub(crate) device_memory: vk::DeviceMemory, diff --git a/src/vulkan/presser.rs b/src/vulkan/presser.rs deleted file mode 100644 index 1f50286b..00000000 --- a/src/vulkan/presser.rs +++ /dev/null @@ -1,90 +0,0 @@ -use super::Allocation; -use core::convert::TryFrom; - -impl Allocation { - /// 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. - /// - /// Returns [`None`] if `self.mapped_ptr()` is `None`, or if `self.size()` is greater than `isize::MAX` because - /// this could lead to undefined behavior. - /// - /// # Example - /// - /// ```ignore - /// #[repr(C, align(16))] - /// #[derive(Clone, Copy)] - /// struct MyGpuVector { - /// x: f32, - /// y: f32, - /// z: f32, - /// } - /// - /// // Create some data to be sent to the GPU. Note this must be formatted correctly in terms of - /// // alignment of individual items and etc, as usual. - /// let my_gpu_data: &[MyGpuVector] = get_vertex_data(); - /// - /// // Get a `presser::Slab` from our gpu_allocator::Allocation - /// let mut alloc_slab = my_allocation.as_mapped_slab().unwrap(); - /// - /// // depending on the type of data you're copying, your vulkan device may have a minimum - /// // alignment requirement for that data - /// let min_gpu_align = my_vulkan_device_specifications.min_alignment_thing; - /// - /// let copy_record = presser::copy_from_slice_to_offset_with_align( - /// my_gpu_data, - /// &mut alloc_slab, - /// 0, // start as close to the beginning of the allocation as possible - /// min_gpu_align, - /// ); - /// - /// // the data may not have actually been copied starting at the requested start offset - /// // depending on the alignment of the underlying allocation, as well as the alignment requirements of - /// // `MyGpuVector` and the `min_gpu_align` we passed in - /// let actual_data_start_offset = copy_record.copy_start_offset; - /// ``` - /// - /// # Safety - /// - /// This is technically not fully safe 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 just calling stuff like this is fine. So, as would always be the case, ensure the GPU - /// is not using the data in `self` before calling this function. - pub fn as_mapped_slab(&mut self) -> Option> { - let mapped_ptr = self.mapped_ptr()?.cast().as_ptr(); - // size > isize::MAX is disallowed by `Slab` for safety reasons - let size = isize::try_from(self.size()).ok()?; - // this will always succeed since size can only be positive and < isize::MAX - let size = size as usize; - - Some(MappedAllocationSlab { - _borrowed_alloc: self, - mapped_ptr, - size, - }) - } -} - -/// A wrapper struct over a borrowed [`Allocation`] that implements [`presser::Slab`]. -/// -/// This type should be acquired by calling [`Allocation::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 - } -} From bea8b3b3aa01388659ff0194c4c7391aa83038f4 Mon Sep 17 00:00:00 2001 From: Gray Olson Date: Wed, 19 Oct 2022 17:30:15 +0200 Subject: [PATCH 14/15] impl Slab on Allocation directly --- .../vulkan-visualization/imgui_renderer.rs | 28 +-- src/vulkan/mod.rs | 181 ++++++++++++------ 2 files changed, 141 insertions(+), 68 deletions(-) diff --git a/examples/vulkan-visualization/imgui_renderer.rs b/examples/vulkan-visualization/imgui_renderer.rs index d164f1ae..a0862de4 100644 --- a/examples/vulkan-visualization/imgui_renderer.rs +++ b/examples/vulkan-visualization/imgui_renderer.rs @@ -338,8 +338,10 @@ impl ImGuiRenderer { }; // Copy font data to upload buffer - let mut slab = upload_buffer_memory.as_mapped_slab().unwrap(); - presser::copy_from_slice_to_offset(font_atlas.data, &mut slab, 0).unwrap(); + 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( @@ -627,12 +629,8 @@ impl ImGuiRenderer { ], }; - let copy_record = presser::copy_to_offset( - &cbuffer_data, - &mut self.cb_allocation.as_mapped_slab().unwrap(), - 0, - ) - .unwrap(); + let copy_record = + presser::copy_to_offset(&cbuffer_data, &mut self.cb_allocation, 0).unwrap(); assert_eq!(copy_record.copy_start_offset, 0); } @@ -709,14 +707,15 @@ impl ImGuiRenderer { let mut vb_offset = 0; let mut ib_offset = 0; - let mut vb_slab = self.vb_allocation.as_mapped_slab().unwrap(); - let mut ib_slab = self.ib_allocation.as_mapped_slab().unwrap(); - for draw_list in imgui_draw_data.draw_lists() { { let vertices = draw_list.vtx_buffer(); - let copy_record = - presser::copy_from_slice_to_offset(vertices, &mut vb_slab, vb_offset).unwrap(); + 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 { @@ -732,7 +731,8 @@ impl ImGuiRenderer { { let indices = draw_list.idx_buffer(); let copy_record = - presser::copy_from_slice_to_offset(indices, &mut ib_slab, ib_offset).unwrap(); + presser::copy_from_slice_to_offset(indices, &mut self.ib_allocation, ib_offset) + .unwrap(); ib_offset = copy_record.copy_end_offset_padded; unsafe { diff --git a/src/vulkan/mod.rs b/src/vulkan/mod.rs index 8548ed2a..01303eb8 100644 --- a/src/vulkan/mod.rs +++ b/src/vulkan/mod.rs @@ -5,15 +5,11 @@ mod visualizer; #[cfg(feature = "visualizer")] pub use visualizer::AllocatorVisualizer; -#[cfg(feature = "presser")] -mod presser; - use super::allocator; use super::allocator::AllocationType; use ash::vk; use log::{debug, Level}; use std::fmt; -use core::convert::TryFrom; use crate::{ allocator::fmt_bytes, AllocationError, AllocatorDebugSettings, MemoryLocation, Result, @@ -39,6 +35,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, @@ -61,63 +141,31 @@ unsafe impl Send for Allocation {} unsafe impl Sync for Allocation {} impl Allocation { - /// Borrow the CPU-mapped memory that backs this allocation as a [`presser::Slab`], which you can then + /// 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. /// - /// # Example - /// - /// ```ignore - /// #[repr(C, align(16))] - /// #[derive(Clone, Copy)] - /// struct MyGpuVector { - /// x: f32, - /// y: f32, - /// z: f32, - /// } - /// - /// // Create some data to be sent to the GPU. Note this must be formatted correctly in terms of - /// // alignment of individual items and etc, as usual. - /// let my_gpu_data: &[MyGpuVector] = get_vertex_data(); - /// - /// // Get a `presser::Slab` from our gpu_allocator::Allocation - /// let mut alloc_slab = my_allocation.as_mapped_slab().unwrap(); - /// - /// // depending on the type of data you're copying, your vulkan device may have a minimum - /// // alignment requirement for that data - /// let min_gpu_align = my_vulkan_device_specifications.min_alignment_thing; - /// - /// let copy_record = presser::copy_from_slice_to_offset_with_align( - /// my_gpu_data, - /// &mut alloc_slab, - /// 0, // start as close to the beginning of the allocation as possible - /// min_gpu_align, - /// ); - /// - /// // the data may not have actually been copied starting at the requested start offset - /// // depending on the alignment of the underlying allocation, as well as the alignment requirements of - /// // `MyGpuVector` and the `min_gpu_align` we passed in. - /// let actual_data_start_offset = copy_record.copy_start_offset; - /// ``` + /// 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 /// - /// This is technically not fully safe 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`]. - pub fn as_mapped_slab(&mut self) -> Option> { + /// 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(); - // size > isize::MAX is disallowed by `Slab` for safety reasons - let size = isize::try_from(self.size()).ok()?; - // this will always succeed since size can only be positive and < isize::MAX - let size = size as usize; + + 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, @@ -195,9 +243,9 @@ impl Default for Allocation { } } -/// A wrapper struct over a borrowed [`Allocation`] that implements [`presser::Slab`]. +/// A wrapper struct over a borrowed [`Allocation`] that infallibly implements [`presser::Slab`]. /// -/// This type should be acquired by calling [`Allocation::as_mapped_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, @@ -219,6 +267,31 @@ unsafe impl<'a> presser::Slab for MappedAllocationSlab<'a> { } } +// 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, From 4a26a32261455cce63b5a2709f8a4d6ce5c1f451 Mon Sep 17 00:00:00 2001 From: Gray Olson Date: Wed, 19 Oct 2022 17:32:47 +0200 Subject: [PATCH 15/15] revert gitignore --- .gitignore | 4 ---- 1 file changed, 4 deletions(-) diff --git a/.gitignore b/.gitignore index 811f57e2..877682d7 100644 --- a/.gitignore +++ b/.gitignore @@ -2,10 +2,6 @@ # will have compiled files and executables /target/ -# A semi-common pattern is to have rust-analyzer compile into a separate target -# directory like /tmp so that it doesn't conflict with other instances of cargo -/tmp - # Remove Cargo.lock from gitignore if creating an executable, leave it for libraries # More information here https://doc.rust-lang.org/cargo/guide/cargo-toml-vs-cargo-lock.html Cargo.lock