Skip to content

Commit 33fa052

Browse files
Jasper-BekkersMarijnS95
Jasper-Bekkers
and
MarijnS95
authored
🦋 metal: Remove inconsistent descriptor creators from Allocation (#251)
* 🦋 Simplify the metal interface * Address review comments * Inline descriptor creators into example --------- Co-authored-by: Marijn Suijten <[email protected]>
1 parent 5e4191d commit 33fa052

File tree

6 files changed

+112
-60
lines changed

6 files changed

+112
-60
lines changed

‎README.md‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,15 @@ let allocation_desc = AllocationCreateDesc::buffer(
152152
MemoryLocation::GpuOnly,
153153
);
154154
let allocation = allocator.allocate(&allocation_desc).unwrap();
155-
let resource = allocation.make_buffer().unwrap();
155+
let heap = unsafe { allocation.heap() };
156+
let resource = unsafe {
157+
heap.newBufferWithLength_options_offset(
158+
allocation.size() as usize,
159+
heap.resourceOptions(),
160+
allocation.offset() as usize,
161+
)
162+
}
163+
.unwrap();
156164

157165
// Cleanup
158166
drop(resource);

‎examples/metal-buffer.rs‎

Lines changed: 51 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ use log::info;
33
use objc2::rc::Id;
44
use objc2_foundation::NSArray;
55
use objc2_metal::{
6-
MTLCreateSystemDefaultDevice, MTLDevice as _, MTLPixelFormat,
6+
MTLCreateSystemDefaultDevice, MTLDevice as _, MTLHeap, MTLPixelFormat,
77
MTLPrimitiveAccelerationStructureDescriptor, MTLStorageMode, MTLTextureDescriptor,
88
};
99

@@ -35,7 +35,17 @@ fn main() {
3535
gpu_allocator::MemoryLocation::GpuOnly,
3636
);
3737
let allocation = allocator.allocate(&allocation_desc).unwrap();
38-
let _buffer = allocation.make_buffer().unwrap();
38+
// SAFETY: We will only allocate objects on this heap within the returned offset and size
39+
let heap = unsafe { allocation.heap() };
40+
let buffer = unsafe {
41+
heap.newBufferWithLength_options_offset(
42+
allocation.size() as usize,
43+
heap.resourceOptions(),
44+
allocation.offset() as usize,
45+
)
46+
}
47+
.unwrap();
48+
drop(buffer);
3949
allocator.free(&allocation).unwrap();
4050
info!("Allocation and deallocation of GpuOnly memory was successful.");
4151
}
@@ -49,7 +59,17 @@ fn main() {
4959
gpu_allocator::MemoryLocation::CpuToGpu,
5060
);
5161
let allocation = allocator.allocate(&allocation_desc).unwrap();
52-
let _buffer = allocation.make_buffer().unwrap();
62+
// SAFETY: We will only allocate objects on this heap within the returned offset and size
63+
let heap = unsafe { allocation.heap() };
64+
let buffer = unsafe {
65+
heap.newBufferWithLength_options_offset(
66+
allocation.size() as usize,
67+
heap.resourceOptions(),
68+
allocation.offset() as usize,
69+
)
70+
}
71+
.unwrap();
72+
drop(buffer);
5373
allocator.free(&allocation).unwrap();
5474
info!("Allocation and deallocation of CpuToGpu memory was successful.");
5575
}
@@ -63,7 +83,17 @@ fn main() {
6383
gpu_allocator::MemoryLocation::GpuToCpu,
6484
);
6585
let allocation = allocator.allocate(&allocation_desc).unwrap();
66-
let _buffer = allocation.make_buffer().unwrap();
86+
// SAFETY: We will only allocate objects on this heap within the returned offset and size
87+
let heap = unsafe { allocation.heap() };
88+
let buffer = unsafe {
89+
heap.newBufferWithLength_options_offset(
90+
allocation.size() as usize,
91+
heap.resourceOptions(),
92+
allocation.offset() as usize,
93+
)
94+
}
95+
.unwrap();
96+
drop(buffer);
6797
allocator.free(&allocation).unwrap();
6898
info!("Allocation and deallocation of GpuToCpu memory was successful.");
6999
}
@@ -78,7 +108,13 @@ fn main() {
78108
let allocation_desc =
79109
AllocationCreateDesc::texture(&device, "Test allocation (Texture)", &texture_desc);
80110
let allocation = allocator.allocate(&allocation_desc).unwrap();
81-
let _texture = allocation.make_texture(&texture_desc).unwrap();
111+
// SAFETY: We will only allocate objects on this heap within the returned offset and size
112+
let heap = unsafe { allocation.heap() };
113+
let buffer = unsafe {
114+
heap.newTextureWithDescriptor_offset(&texture_desc, allocation.offset() as usize)
115+
}
116+
.unwrap();
117+
drop(buffer);
82118
allocator.free(&allocation).unwrap();
83119
info!("Allocation and deallocation of Texture was successful.");
84120
}
@@ -96,7 +132,16 @@ fn main() {
96132
gpu_allocator::MemoryLocation::GpuOnly,
97133
);
98134
let allocation = allocator.allocate(&allocation_desc).unwrap();
99-
let _acc_structure = allocation.make_acceleration_structure();
135+
// SAFETY: We will only allocate objects on this heap within the returned offset and size
136+
let heap = unsafe { allocation.heap() };
137+
let buffer = unsafe {
138+
heap.newAccelerationStructureWithSize_offset(
139+
allocation.size() as usize,
140+
allocation.offset() as usize,
141+
)
142+
}
143+
.unwrap();
144+
drop(buffer);
100145
allocator.free(&allocation).unwrap();
101146
info!("Allocation and deallocation of Acceleration structure was successful.");
102147
}

‎src/d3d12/mod.rs‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -335,18 +335,22 @@ impl Allocation {
335335
}
336336

337337
/// Returns the [`ID3D12Heap`] object that is backing this allocation.
338-
/// This heap object can be shared with multiple other allocations and shouldn't be freed (or allocated from)
338+
///
339+
/// This heap object can be shared with multiple other allocations and shouldn't be allocated from
339340
/// without this library, because that will lead to undefined behavior.
340341
///
341342
/// # Safety
342-
/// The result of this function be safely passed into [`ID3D12Device::CreatePlacedResource()`].
343-
/// It is exposed for this reason. Keep in mind to also pass [`Self::offset()`] along to it.
343+
/// The result of this function can safely be passed into [`ID3D12Device::CreatePlacedResource()`].
344+
/// It is exposed for this reason. Keep in mind to also pass [`Self::offset()`] along to it.
345+
///
346+
/// Also, this [`Allocation`] must not be [`Allocator::free()`]d while such a created resource
347+
/// on this [`ID3D12Heap`] is still live.
344348
pub unsafe fn heap(&self) -> &ID3D12Heap {
345349
&self.heap
346350
}
347351

348352
/// Returns the offset of the allocation on the [`ID3D12Heap`].
349-
/// When creating a placed resources, this offset needs to be supplied as well.
353+
/// When creating a placed resource, this offset needs to be supplied as well.
350354
pub fn offset(&self) -> u64 {
351355
self.offset
352356
}

‎src/lib.rs‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -197,7 +197,16 @@
197197
//! MemoryLocation::GpuOnly,
198198
//! );
199199
//! let allocation = allocator.allocate(&allocation_desc).unwrap();
200-
//! let resource = allocation.make_buffer().unwrap();
200+
//! # use objc2_metal::MTLHeap;
201+
//! let heap = unsafe { allocation.heap() };
202+
//! let resource = unsafe {
203+
//! heap.newBufferWithLength_options_offset(
204+
//! allocation.size() as usize,
205+
//! heap.resourceOptions(),
206+
//! allocation.offset() as usize,
207+
//! )
208+
//! }
209+
//! .unwrap();
201210
//!
202211
//! // Cleanup
203212
//! drop(resource);

‎src/metal/mod.rs‎

Lines changed: 26 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,8 @@ use log::debug;
99
use objc2::{rc::Retained, runtime::ProtocolObject};
1010
use objc2_foundation::NSString;
1111
use objc2_metal::{
12-
MTLAccelerationStructure, MTLBuffer, MTLCPUCacheMode, MTLDevice, MTLHeap, MTLHeapDescriptor,
13-
MTLHeapType, MTLResource, MTLResourceOptions, MTLStorageMode, MTLTexture, MTLTextureDescriptor,
12+
MTLCPUCacheMode, MTLDevice, MTLHeap, MTLHeapDescriptor, MTLHeapType, MTLResourceOptions,
13+
MTLStorageMode, MTLTextureDescriptor,
1414
};
1515

1616
use crate::{
@@ -39,55 +39,37 @@ pub struct Allocation {
3939
}
4040

4141
impl Allocation {
42-
pub fn heap(&self) -> &ProtocolObject<dyn MTLHeap> {
42+
/// Returns the [`MTLHeap`] object that is backing this allocation.
43+
///
44+
/// This heap object can be shared with multiple other allocations and shouldn't be allocated from
45+
/// without this library, because that will lead to undefined behavior.
46+
///
47+
/// # Safety
48+
/// When allocating new buffers, textures, or other resources on this [`MTLHeap`], be sure to
49+
/// pass [`Self::offset()`] and not exceed [`Self::size()`] to not allocate new resources on top
50+
/// of existing [`Allocation`]s.
51+
///
52+
/// Also, this [`Allocation`] must not be [`Allocator::free()`]d while such a created resource
53+
/// on this [`MTLHeap`] is still live.
54+
pub unsafe fn heap(&self) -> &ProtocolObject<dyn MTLHeap> {
4355
&self.heap
4456
}
4557

46-
pub fn make_buffer(&self) -> Option<Retained<ProtocolObject<dyn MTLBuffer>>> {
47-
let resource = unsafe {
48-
self.heap.newBufferWithLength_options_offset(
49-
self.size as usize,
50-
self.heap.resourceOptions(),
51-
self.offset as usize,
52-
)
53-
};
54-
if let Some(resource) = &resource {
55-
if let Some(name) = &self.name {
56-
resource.setLabel(Some(&NSString::from_str(name)));
57-
}
58-
}
59-
resource
58+
/// Returns the size of the allocation
59+
pub fn size(&self) -> u64 {
60+
self.size
6061
}
6162

62-
pub fn make_texture(
63-
&self,
64-
desc: &MTLTextureDescriptor,
65-
) -> Option<Retained<ProtocolObject<dyn MTLTexture>>> {
66-
let resource = unsafe {
67-
self.heap
68-
.newTextureWithDescriptor_offset(desc, self.offset as usize)
69-
};
70-
if let Some(resource) = &resource {
71-
if let Some(name) = &self.name {
72-
resource.setLabel(Some(&NSString::from_str(name)));
73-
}
74-
}
75-
resource
63+
/// Returns the offset of the allocation on the [`MTLHeap`].
64+
///
65+
/// Since all [`Allocation`]s are suballocated within a [`MTLHeap`], this offset always needs to
66+
/// be supplied. See the safety documentation on [`Self::heap()`].
67+
pub fn offset(&self) -> u64 {
68+
self.offset
7669
}
7770

78-
pub fn make_acceleration_structure(
79-
&self,
80-
) -> Option<Retained<ProtocolObject<dyn MTLAccelerationStructure>>> {
81-
let resource = unsafe {
82-
self.heap
83-
.newAccelerationStructureWithSize_offset(self.size as usize, self.offset as usize)
84-
};
85-
if let Some(resource) = &resource {
86-
if let Some(name) = &self.name {
87-
resource.setLabel(Some(&NSString::from_str(name)));
88-
}
89-
}
90-
resource
71+
pub fn name(&self) -> Option<&str> {
72+
self.name.as_deref()
9173
}
9274

9375
fn is_null(&self) -> bool {

‎src/vulkan/mod.rs‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -206,13 +206,17 @@ impl Allocation {
206206
}
207207

208208
/// Returns the [`vk::DeviceMemory`] object that is backing this allocation.
209-
/// This memory object can be shared with multiple other allocations and shouldn't be freed (or allocated from)
209+
///
210+
/// This memory object can be shared with multiple other allocations and shouldn't be freed or allocated from
210211
/// without this library, because that will lead to undefined behavior.
211212
///
212213
/// # Safety
213-
/// The result of this function can safely be used to pass into [`ash::Device::bind_buffer_memory()`],
214-
/// [`ash::Device::bind_image_memory()`] etc. It is exposed for this reason. Keep in mind to also
215-
/// pass [`Self::offset()`] along to those.
214+
/// The result of this function can safely be passed into [`ash::Device::bind_buffer_memory()`],
215+
/// [`ash::Device::bind_image_memory()`] etc. It is exposed for this reason. Keep in mind to
216+
/// also pass [`Self::offset()`] along to those.
217+
///
218+
/// Also, this [`Allocation`] must not be [`Allocator::free()`]d while such a created resource
219+
/// on this [`vk::DeviceMemory`] is still live.
216220
pub unsafe fn memory(&self) -> vk::DeviceMemory {
217221
self.device_memory
218222
}

0 commit comments

Comments
 (0)