-
Notifications
You must be signed in to change notification settings - Fork 1.2k
staging belt: allow creation with custom buffer usages #8580
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.
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
base: trunk
Are you sure you want to change the base?
Changes from 3 commits
62899a2
df52452
4a2c1c6
ae51eed
5353eda
98f74e8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| //! Tests of [`wgpu::util`]. | ||
|
|
||
| use nanorand::Rng; | ||
| use wgpu::BufferUsages; | ||
|
|
||
| /// Generate (deterministic) random staging belt operations to exercise its logic. | ||
| #[test] | ||
|
|
@@ -39,3 +40,44 @@ fn staging_belt_random_test() { | |
| belt.recall(); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn staging_belt_panics_with_invalid_buffer_usages() { | ||
| fn test_if_panics(usage: BufferUsages) -> bool { | ||
| std::panic::catch_unwind(|| { | ||
| let (device, _queue) = wgpu::Device::noop(&wgpu::DeviceDescriptor::default()); | ||
| let _belt = wgpu::util::StagingBelt::new_with_buffer_usages(device.clone(), 512, usage); | ||
| }) | ||
| .is_err() | ||
|
||
| } | ||
|
|
||
| for mut usage in BufferUsages::all() | ||
| .difference(BufferUsages::COPY_SRC | BufferUsages::MAP_WRITE) | ||
| .iter() | ||
| { | ||
| assert!(test_if_panics(usage), "StagingBelt::new_with_buffer_usages should panic without MAPPABLE_PRIMARY_BUFFERS with usage={usage:?}"); | ||
|
|
||
| usage.insert(BufferUsages::MAP_WRITE); | ||
| assert!(test_if_panics(usage), "StagingBelt::new_with_buffer_usages should panic without MAPPABLE_PRIMARY_BUFFERS with usage={usage:?}"); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn staging_belt_works_with_non_exclusive_buffer_usages() { | ||
jgraef marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| let (device, _queue) = wgpu::Device::noop(&wgpu::DeviceDescriptor::default()); | ||
| let _belt = wgpu::util::StagingBelt::new_with_buffer_usages( | ||
| device.clone(), | ||
| 512, | ||
| BufferUsages::COPY_SRC, | ||
| ); | ||
| let _belt = wgpu::util::StagingBelt::new_with_buffer_usages( | ||
| device.clone(), | ||
| 512, | ||
| BufferUsages::COPY_SRC | BufferUsages::MAP_WRITE, | ||
| ); | ||
| let _belt = wgpu::util::StagingBelt::new_with_buffer_usages( | ||
| device.clone(), | ||
| 512, | ||
| BufferUsages::MAP_WRITE, | ||
| ); | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,6 +5,7 @@ use crate::{ | |||||||||||||||
| use alloc::vec::Vec; | ||||||||||||||||
| use core::fmt; | ||||||||||||||||
| use std::sync::mpsc; | ||||||||||||||||
| use wgt::Features; | ||||||||||||||||
|
|
||||||||||||||||
| use crate::COPY_BUFFER_ALIGNMENT; | ||||||||||||||||
|
|
||||||||||||||||
|
|
@@ -26,6 +27,11 @@ use crate::COPY_BUFFER_ALIGNMENT; | |||||||||||||||
| pub struct StagingBelt { | ||||||||||||||||
| device: Device, | ||||||||||||||||
| chunk_size: BufferAddress, | ||||||||||||||||
| /// User-specified [`BufferUsages`] with which the chunk buffers are created. | ||||||||||||||||
jgraef marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||
| /// | ||||||||||||||||
| /// [`new`](Self::new) guarantees that this always contains | ||||||||||||||||
| /// [`MAP_WRITE`](BufferUsages::MAP_WRITE). | ||||||||||||||||
| buffer_usages: BufferUsages, | ||||||||||||||||
| /// Chunks into which we are accumulating data to be transferred. | ||||||||||||||||
| active_chunks: Vec<Chunk>, | ||||||||||||||||
| /// Chunks that have scheduled transfers already; they are unmapped and some | ||||||||||||||||
|
|
@@ -51,11 +57,54 @@ impl StagingBelt { | |||||||||||||||
| /// * 1-4 times less than the total amount of data uploaded per submission | ||||||||||||||||
| /// (per [`StagingBelt::finish()`]); and | ||||||||||||||||
| /// * bigger is better, within these bounds. | ||||||||||||||||
| /// | ||||||||||||||||
| /// The buffers returned by this staging belt will have the buffer usages | ||||||||||||||||
| /// [`MAP_READ | MAP_WRITE`](BufferUsages). | ||||||||||||||||
jgraef marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||
| pub fn new(device: Device, chunk_size: BufferAddress) -> Self { | ||||||||||||||||
| Self::new_with_buffer_usages(device, chunk_size, BufferUsages::COPY_SRC) | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| /// Create a new staging belt. | ||||||||||||||||
| /// | ||||||||||||||||
| /// The `chunk_size` is the unit of internal buffer allocation; writes will be | ||||||||||||||||
| /// sub-allocated within each chunk. Therefore, for optimal use of memory, the | ||||||||||||||||
| /// chunk size should be: | ||||||||||||||||
| /// | ||||||||||||||||
| /// * larger than the largest single [`StagingBelt::write_buffer()`] operation; | ||||||||||||||||
| /// * 1-4 times less than the total amount of data uploaded per submission | ||||||||||||||||
| /// (per [`StagingBelt::finish()`]); and | ||||||||||||||||
| /// * bigger is better, within these bounds. | ||||||||||||||||
| /// | ||||||||||||||||
| /// `buffer_usages` specifies with which [`BufferUsages`] the staging buffers | ||||||||||||||||
| /// will be created. [`MAP_WRITE`](BufferUsages::MAP_WRITE) will be added | ||||||||||||||||
jgraef marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||
| /// automatically. The method will panic if the combination of usages is not | ||||||||||||||||
| /// supported. Because [`MAP_WRITE`](BufferUsages::MAP_WRITE) is implied, only | ||||||||||||||||
| /// [`COPY_SRC`](BufferUsages::COPY_SRC) can be used, except if | ||||||||||||||||
| /// [`Features::MAPPABLE_PRIMARY_BUFFERS`] is enabled. | ||||||||||||||||
|
||||||||||||||||
| /// supported. Because [`MAP_WRITE`](BufferUsages::MAP_WRITE) is implied, only | |
| /// [`COPY_SRC`](BufferUsages::COPY_SRC) can be used, except if | |
| /// [`Features::MAPPABLE_PRIMARY_BUFFERS`] is enabled. | |
| /// supported. Because [`MAP_WRITE`](BufferUsages::MAP_WRITE) is implied, the allowed usages | |
| /// depends on if [`Features::MAPPABLE_PRIMARY_BUFFERS`] is enabled. | |
| /// - If enabled: any usage is valid. | |
| /// - If disabled: only [`COPY_SRC`](BufferUsages::COPY_SRC) can be used. |
How does that sound, I think this is a bit clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better :)
I strictly any usage allowed with MAPPABLE_PRIMARY_BUFFERS? And what if that would change in the future? Maybe we should just link to the documentation of that feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any anything is allowed with MAPPABLE_PRIMARY_BUFFERS - that shouldn't change in the future. This was one of the first features we had and has remained largely unchanged since.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we have a set style on this yet, but since the file currently uses all qualified paths for
wgpu, please stick to that; that is, remove thisuseand update the code to work without it.