From 64f3b1e7bc7754cc26e0a7b9e5c90a9591a8bdf5 Mon Sep 17 00:00:00 2001 From: Martin Charles Date: Fri, 7 Jun 2024 05:00:16 -0500 Subject: [PATCH] add memoryMapPlaced and memoryMapRangePlaced features of VK_EXT_map_memory_placed extension (#2514) * add partial support for VK_EXT_map_memory_placed extension implements the memoryMapPlaced and memoryMapRangePlaced features * fix clippy lint * add tests I don't love these tests, they probably fail silently in CI * fix conflicting requirement * update test * fmt * fix memory selection in test intersects does not do what I thought it does * remove incorrect assertion I don't think this is correct. The offset and size are both DeviceSize or u64. They are added together as positive numbers. * remove unused * put back non-range test * only run test on unix * wrap stuff to 100 cols this is actually really nice for reading * clippy * tweak impl * scope * remove unused * use NonNull * fmt * add flags field * update position * PLACED_EXT -> PLACED * removed unnecessary validation * Update vulkano/src/memory/device_memory.rs Co-authored-by: marc0246 <40955683+marc0246@users.noreply.github.com> * Update vulkano/src/memory/device_memory.rs Co-authored-by: marc0246 <40955683+marc0246@users.noreply.github.com> * Update vulkano/src/memory/device_memory.rs Co-authored-by: marc0246 <40955683+marc0246@users.noreply.github.com> * Update vulkano/src/memory/device_memory.rs Co-authored-by: marc0246 <40955683+marc0246@users.noreply.github.com> * Update vulkano/src/memory/device_memory.rs Co-authored-by: marc0246 <40955683+marc0246@users.noreply.github.com> * remove more * typo * Update vulkano/src/memory/device_memory.rs Co-authored-by: Rua * add validate_device call * Update vulkano/src/memory/device_memory.rs Co-authored-by: marc0246 <40955683+marc0246@users.noreply.github.com> * Update vulkano/src/memory/device_memory.rs Co-authored-by: marc0246 <40955683+marc0246@users.noreply.github.com> * Update vulkano/src/memory/device_memory.rs Co-authored-by: marc0246 <40955683+marc0246@users.noreply.github.com> * fix links * remove extra branch * use WHOLE_SIZE * use getter * fix boolean condition * Update vulkano/src/memory/device_memory.rs Co-authored-by: marc0246 <40955683+marc0246@users.noreply.github.com> * add specific changes marc asked for i've concluded that this choice is roughly semantically equivalent and this comes down to opinion. I can't be bothered to have an opinion here, imo this is a bikeshed. also it is temporary. until the docs are updated and the impls are updated. also i don't even really care about the ranged version of this feature, implemented it mostly for completeness than anything else * Incorporate latest spec updates --------- Co-authored-by: marc0246 <40955683+marc0246@users.noreply.github.com> Co-authored-by: Rua --- Cargo.lock | 5 +- vulkano/Cargo.toml | 3 + vulkano/autogen/properties.rs | 3 +- vulkano/src/memory/allocator/mod.rs | 5 +- vulkano/src/memory/device_memory.rs | 284 +++++++++++++++++++++++++++- vulkano/src/tests.rs | 13 +- 6 files changed, 299 insertions(+), 14 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3f55ff61a..19b7964f7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1069,9 +1069,9 @@ checksum = "e2abad23fbc42b3700f2f279844dc832adb2b2eb069b2df918f455c4e18cc646" [[package]] name = "libc" -version = "0.2.152" +version = "0.2.153" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "13e3bf6590cbc649f4d1a3eefc9d5d6eb746f5200ffb04e5e142700b8faa56e7" +checksum = "9c198f91728a82281a64e1f4f9eeb25d82cb32a5de251c6bd1b5154d63a8e7bd" [[package]] name = "libloading" @@ -2347,6 +2347,7 @@ dependencies = [ "half", "heck", "indexmap", + "libc", "libloading 0.8.1", "nom", "objc", diff --git a/vulkano/Cargo.toml b/vulkano/Cargo.toml index af9c6d09e..8c59d7b08 100644 --- a/vulkano/Cargo.toml +++ b/vulkano/Cargo.toml @@ -46,6 +46,9 @@ serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } vk-parse = { workspace = true } +[dev-dependencies] +libc = "0.2.153" + [features] default = ["macros"] macros = ["dep:vulkano-macros"] diff --git a/vulkano/autogen/properties.rs b/vulkano/autogen/properties.rs index a0c39d17d..3d71c03f0 100644 --- a/vulkano/autogen/properties.rs +++ b/vulkano/autogen/properties.rs @@ -231,7 +231,8 @@ fn properties_members(types: &HashMap<&str, (&Type, Vec<&str>)>) -> Vec { + | "uniformTexelBufferOffsetAlignmentBytes" + | "minPlacedMemoryMapAlignment" => { quote! { DeviceAlignment } } _ => vulkano_type(ty, len), diff --git a/vulkano/src/memory/allocator/mod.rs b/vulkano/src/memory/allocator/mod.rs index 09a6e37ea..44d4462a9 100644 --- a/vulkano/src/memory/allocator/mod.rs +++ b/vulkano/src/memory/allocator/mod.rs @@ -219,8 +219,8 @@ pub use self::{ }; use super::{ DedicatedAllocation, DeviceAlignment, DeviceMemory, ExternalMemoryHandleTypes, - MemoryAllocateFlags, MemoryAllocateInfo, MemoryMapInfo, MemoryProperties, MemoryPropertyFlags, - MemoryRequirements, MemoryType, + MemoryAllocateFlags, MemoryAllocateInfo, MemoryMapFlags, MemoryMapInfo, MemoryProperties, + MemoryPropertyFlags, MemoryRequirements, MemoryType, }; use crate::{ device::{Device, DeviceOwned}, @@ -1094,6 +1094,7 @@ impl GenericMemoryAllocator { // - Mapping the whole range is always valid. unsafe { memory.map_unchecked(MemoryMapInfo { + flags: MemoryMapFlags::empty(), offset: 0, size: memory.allocation_size(), _ne: crate::NonExhaustive(()), diff --git a/vulkano/src/memory/device_memory.rs b/vulkano/src/memory/device_memory.rs index 292c07bce..10b73198a 100644 --- a/vulkano/src/memory/device_memory.rs +++ b/vulkano/src/memory/device_memory.rs @@ -418,12 +418,61 @@ impl DeviceMemory { /// `self` must not be host-mapped already and must be allocated from host-visible memory. #[inline] pub fn map(&mut self, map_info: MemoryMapInfo) -> Result<(), Validated> { - self.validate_map(&map_info)?; + self.validate_map(&map_info, None)?; unsafe { Ok(self.map_unchecked(map_info)?) } } - fn validate_map(&self, map_info: &MemoryMapInfo) -> Result<(), Box> { + #[cfg_attr(not(feature = "document_unchecked"), doc(hidden))] + #[inline] + pub unsafe fn map_unchecked(&mut self, map_info: MemoryMapInfo) -> Result<(), VulkanError> { + unsafe { self.map_unchecked_inner(map_info, None) } + } + + /// Maps a range of memory to be accessed by the host at the specified `placed_address`. + /// + /// Requires the [`memory_map_placed`] feature to be enabled on the device. + /// + /// `placed_address` must be aligned to the [`min_placed_memory_map_alignment`] device + /// property. + /// + /// `self` must not be host-mapped already and must be allocated from host-visible memory. + /// + /// # Safety + /// + /// - The memory mapping specified by `placed_address` and `map_info.size` will be replaced, + /// including mappings made by the Rust allocator, a library, or your application itself. + /// - The memory mapping specified by `placed_address` and `map_info.size` must not overlap any + /// existing Vulkan memory mapping. + /// + /// [`memory_map_placed`]: crate::device::DeviceFeatures::memory_map_placed + /// [`min_placed_memory_map_alignment`]: crate::device::DeviceProperties::min_placed_memory_map_alignment + #[inline] + pub unsafe fn map_placed( + &mut self, + map_info: MemoryMapInfo, + placed_address: NonNull, + ) -> Result<(), Validated> { + self.validate_map(&map_info, Some(placed_address))?; + + unsafe { Ok(self.map_placed_unchecked(map_info, placed_address)?) } + } + + #[cfg_attr(not(feature = "document_unchecked"), doc(hidden))] + #[inline] + pub unsafe fn map_placed_unchecked( + &mut self, + map_info: MemoryMapInfo, + placed_address: NonNull, + ) -> Result<(), VulkanError> { + unsafe { self.map_unchecked_inner(map_info, Some(placed_address)) } + } + + fn validate_map( + &self, + map_info: &MemoryMapInfo, + placed_address: Option>, + ) -> Result<(), Box> { if self.mapping_state.is_some() { return Err(Box::new(ValidationError { problem: "this device memory is already host-mapped".into(), @@ -433,7 +482,7 @@ impl DeviceMemory { } map_info - .validate(self) + .validate(self, placed_address) .map_err(|err| err.add_context("map_info"))?; let memory_type = &self @@ -458,9 +507,13 @@ impl DeviceMemory { Ok(()) } - #[cfg_attr(not(feature = "document_unchecked"), doc(hidden))] - pub unsafe fn map_unchecked(&mut self, map_info: MemoryMapInfo) -> Result<(), VulkanError> { + unsafe fn map_unchecked_inner( + &mut self, + map_info: MemoryMapInfo, + placed_address: Option>, + ) -> Result<(), VulkanError> { let MemoryMapInfo { + flags, offset, size, _ne: _, @@ -477,13 +530,22 @@ impl DeviceMemory { if device.enabled_extensions().khr_map_memory2 { let map_info_vk = ash::vk::MemoryMapInfoKHR { - flags: ash::vk::MemoryMapFlags::empty(), + flags: flags.into(), memory: self.handle(), offset, size, ..Default::default() }; + let mut map_placed_info_vk = ash::vk::MemoryMapPlacedInfoEXT::default(); + + let map_info_vk = if let Some(it) = placed_address { + map_placed_info_vk.p_placed_address = it.as_ptr(); + map_info_vk.push_next(&mut map_placed_info_vk) + } else { + map_info_vk + }; + (fns.khr_map_memory2.map_memory2_khr)( device.handle(), &map_info_vk, @@ -1363,6 +1425,8 @@ vulkan_bitflags! { /// Parameters of a memory map operation. #[derive(Debug)] pub struct MemoryMapInfo { + pub flags: MemoryMapFlags, + /// The offset (in bytes) from the beginning of the `DeviceMemory`, where the mapping starts. /// /// Must be less than the [`allocation_size`] of the device memory. If the the memory was not @@ -1392,13 +1456,25 @@ pub struct MemoryMapInfo { } impl MemoryMapInfo { - pub(crate) fn validate(&self, memory: &DeviceMemory) -> Result<(), Box> { + pub(crate) fn validate( + &self, + memory: &DeviceMemory, + placed_address: Option>, + ) -> Result<(), Box> { let &Self { + flags, offset, size, _ne: _, } = self; + let device = memory.device(); + + flags.validate_device(device).map_err(|err| { + err.add_context("flags") + .set_vuids(&["VUID-VkMemoryMapInfoKHR-flags-parameter"]) + })?; + if !(offset < memory.allocation_size()) { return Err(Box::new(ValidationError { context: "offset".into(), @@ -1427,6 +1503,124 @@ impl MemoryMapInfo { })); } + if flags.contains(MemoryMapFlags::PLACED) { + if !device.enabled_features().memory_map_placed { + return Err(Box::new(ValidationError { + context: "flags".into(), + problem: "contains `MemoryMapFlags::PLACED`".into(), + requires_one_of: RequiresOneOf(&[RequiresAllOf(&[Requires::DeviceFeature( + "memory_map_placed", + )])]), + vuids: &["VUID-VkMemoryMapInfoKHR-flags-09569"], + })); + } + + let Some(placed_address) = placed_address else { + return Err(Box::new(ValidationError { + context: "flags".into(), + problem: + "contains `MemoryMapFlags::PLACED`, but `DeviceMemory::map_placed` isn't \ + used to specify the placed address" + .into(), + vuids: &["VUID-VkMemoryMapInfoKHR-flags-09570"], + ..Default::default() + })); + }; + + // min_placed_memory_map_alignment is always provided when the device extension + // ext_map_memory_placed is available. + let min_placed_memory_map_alignment = device + .physical_device() + .properties() + .min_placed_memory_map_alignment + .unwrap(); + + if !is_aligned( + placed_address.as_ptr() as DeviceSize, + min_placed_memory_map_alignment, + ) { + return Err(Box::new(ValidationError { + context: "placed_address".into(), + problem: "is not aligned to an integer multiple of the \ + `min_placed_memory_map_alignment` device property" + .into(), + vuids: &["VUID-VkMemoryMapPlacedInfoEXT-pPlacedAddress-09577"], + ..Default::default() + })); + } + + if device.enabled_features().memory_map_range_placed { + if !is_aligned(offset, min_placed_memory_map_alignment) { + return Err(Box::new(ValidationError { + context: "offset".into(), + problem: "is not aligned to an integer multiple of the \ + `min_placed_memory_map_alignment` device property" + .into(), + vuids: &["VUID-VkMemoryMapInfoKHR-flags-09573"], + ..Default::default() + })); + } + + if !is_aligned(size, min_placed_memory_map_alignment) { + return Err(Box::new(ValidationError { + context: "size".into(), + problem: "is not aligned to an integer multiple of the \ + `min_placed_memory_map_alignment` device property" + .into(), + vuids: &["VUID-VkMemoryMapInfoKHR-flags-09574"], + ..Default::default() + })); + } + } else { + if offset != 0 { + return Err(Box::new(ValidationError { + context: "offset".into(), + problem: "is not zero".into(), + vuids: &["VUID-VkMemoryMapInfoKHR-flags-09571"], + ..Default::default() + })); + } + + if size != memory.allocation_size() { + return Err(Box::new(ValidationError { + context: "size".into(), + problem: "is not `self.allocation_size()`".into(), + vuids: &["VUID-VkMemoryMapInfoKHR-flags-09572"], + ..Default::default() + })); + } + + if !is_aligned(memory.allocation_size(), min_placed_memory_map_alignment) { + return Err(Box::new(ValidationError { + context: "flags".into(), + problem: "contains `MemoryMapFlags::PLACED`, but `self.allocation_size()` \ + is not aligned to an integer multiple of the \ + `min_placed_memory_map_alignment` device property" + .into(), + vuids: &["VUID-VkMemoryMapInfoKHR-flags-09651"], + ..Default::default() + })); + } + } + + if let Some(handle_type) = memory.imported_handle_type() { + if handle_type == ExternalMemoryHandleType::HostAllocation + || handle_type == ExternalMemoryHandleType::HostMappedForeignMemory + { + return Err(Box::new(ValidationError { + context: "flags".into(), + problem: "contains `MemoryMapFlags::PLACED`, but \ + `self.imported_handle_type()` is \ + `ExternalMemoryHandleType::HostAllocation` or \ + `ExternalMemoryHandleType::HostMappedForeignMemory`" + .into(), + vuids: &["VUID-VkMemoryMapInfoKHR-flags-09575"], + ..Default::default() + })); + } + } + } + let atom_size = memory.atom_size(); // Not required for merely mapping, but without this check the user can end up with @@ -1458,6 +1652,7 @@ impl Default for MemoryMapInfo { #[inline] fn default() -> Self { MemoryMapInfo { + flags: MemoryMapFlags::empty(), offset: 0, size: 0, _ne: crate::NonExhaustive(()), @@ -1465,6 +1660,13 @@ impl Default for MemoryMapInfo { } } +vulkan_bitflags! { + #[non_exhaustive] + MemoryMapFlags = MemoryMapFlags(u32); + + PLACED = PLACED_EXT, +} + /// Parameters of a memory unmap operation. #[derive(Debug)] pub struct MemoryUnmapInfo { @@ -2142,7 +2344,8 @@ unsafe impl Sync for MappedDeviceMemory {} #[cfg(test)] mod tests { use super::MemoryAllocateInfo; - use crate::memory::{DeviceMemory, MemoryPropertyFlags}; + use crate::memory::{DeviceMemory, MemoryMapFlags, MemoryMapInfo, MemoryPropertyFlags}; + use std::{ptr, ptr::NonNull}; #[test] fn create() { @@ -2271,4 +2474,69 @@ mod tests { } assert_eq!(device.allocation_count(), 1); } + + #[test] + #[cfg(unix)] + fn map_placed() { + let (device, _) = gfx_dev_and_queue!(memory_map_placed; ext_map_memory_placed); + + let memory_type_index = { + let physical_device = device.physical_device(); + let memory_properties = physical_device.memory_properties(); + let (idx, _) = memory_properties + .memory_types + .iter() + .enumerate() + .find(|(_idx, it)| { + it.property_flags.contains( + MemoryPropertyFlags::HOST_COHERENT + | MemoryPropertyFlags::HOST_VISIBLE + | MemoryPropertyFlags::DEVICE_LOCAL, + ) + }) + .unwrap(); + + idx as u32 + }; + + let mut memory = DeviceMemory::allocate( + device.clone(), + MemoryAllocateInfo { + allocation_size: 16 * 1024, + memory_type_index, + ..Default::default() + }, + ) + .unwrap(); + + let address = unsafe { + let address = libc::mmap( + ptr::null_mut(), + 16 * 1024, + libc::PROT_READ | libc::PROT_WRITE, + libc::MAP_PRIVATE | libc::MAP_ANONYMOUS, + -1, + 0, + ); + + if address as i64 == -1 { + panic!("failed to map memory") + } + + address + }; + + unsafe { + memory + .map_placed( + MemoryMapInfo { + flags: MemoryMapFlags::PLACED, + size: memory.allocation_size, + ..Default::default() + }, + NonNull::new(address).unwrap(), + ) + .unwrap(); + } + } } diff --git a/vulkano/src/tests.rs b/vulkano/src/tests.rs index b36c4e1cf..abf1067c7 100644 --- a/vulkano/src/tests.rs +++ b/vulkano/src/tests.rs @@ -19,13 +19,24 @@ macro_rules! instance { /// Creates a device and a queue for graphics operations. macro_rules! gfx_dev_and_queue { + () => ({ + gfx_dev_and_queue!(;) + }); ($($feature:ident),*) => ({ + gfx_dev_and_queue!($($feature),*;) + }); + ($($feature:ident),*; $($extension:ident),*) => ({ use crate::device::physical::PhysicalDeviceType; use crate::device::{Device, DeviceCreateInfo, DeviceExtensions, QueueCreateInfo}; use crate::device::DeviceFeatures; let instance = instance!(); - let enabled_extensions = DeviceExtensions::empty(); + let enabled_extensions = DeviceExtensions { + $( + $extension: true, + )* + .. DeviceExtensions::empty() + }; let enabled_features = DeviceFeatures { $( $feature: true,