From 51f4bf2d7f2d5168f45cb1137dbcebe08360ddf6 Mon Sep 17 00:00:00 2001 From: Imbris Date: Tue, 13 Sep 2022 00:23:23 -0400 Subject: [PATCH 1/5] Add missing BufferUsages mismatch check along with a test to ensure it works. --- wgpu-core/src/device/mod.rs | 14 +++++++ wgpu/tests/buffer_usages.rs | 74 +++++++++++++++++++++++++++++++++++++ wgpu/tests/root.rs | 1 + 3 files changed, 89 insertions(+) create mode 100644 wgpu/tests/buffer_usages.rs diff --git a/wgpu-core/src/device/mod.rs b/wgpu-core/src/device/mod.rs index a5c5cbe51c..c9542b3c61 100644 --- a/wgpu-core/src/device/mod.rs +++ b/wgpu-core/src/device/mod.rs @@ -588,6 +588,20 @@ impl Device { return Err(resource::CreateBufferError::EmptyUsage); } + if !self + .features + .contains(wgt::Features::MAPPABLE_PRIMARY_BUFFERS) + { + use wgt::BufferUsages as Bu; + let write_mismatch = desc.usage.contains(Bu::MAP_WRITE) + && !(Bu::MAP_WRITE | Bu::COPY_SRC).contains(desc.usage); + let read_mismatch = desc.usage.contains(Bu::MAP_READ) + && !(Bu::MAP_READ | Bu::COPY_DST).contains(desc.usage); + if write_mismatch || read_mismatch { + return Err(resource::CreateBufferError::UsageMismatch(desc.usage)); + } + } + if desc.mapped_at_creation { if desc.size % wgt::COPY_BUFFER_ALIGNMENT != 0 { return Err(resource::CreateBufferError::UnalignedSize); diff --git a/wgpu/tests/buffer_usages.rs b/wgpu/tests/buffer_usages.rs new file mode 100644 index 0000000000..25a40440e6 --- /dev/null +++ b/wgpu/tests/buffer_usages.rs @@ -0,0 +1,74 @@ +//! Tests for buffer usages validation. + +use wgt::BufferAddress; + +use crate::common::{initialize_test, TestParameters}; + +#[test] +fn buffer_usage() { + fn try_create( + usage: wgpu::BufferUsages, + enable_mappable_primary_buffers: bool, + should_panic: bool, + ) { + let mut parameters = TestParameters::default(); + if enable_mappable_primary_buffers { + parameters = parameters.features(wgpu::Features::MAPPABLE_PRIMARY_BUFFERS); + } + if should_panic { + parameters = parameters.failure(); + } + + initialize_test(parameters, |ctx| { + let _buffer = ctx.device.create_buffer(&wgpu::BufferDescriptor { + label: None, + size: BUFFER_SIZE, + usage, + mapped_at_creation: false, + }); + }); + } + + use wgpu::BufferUsages as Bu; + + // These are always valid + [ + Bu::MAP_READ, + Bu::MAP_WRITE, + Bu::MAP_READ | Bu::COPY_DST, + Bu::MAP_WRITE | Bu::COPY_SRC, + ] + .into_iter() + .for_each(|usages| { + // Should only pass validation when MAPPABLE_PRIMARY_BUFFERS is enabled. + try_create(usages, false, false); + try_create(usages, true, false); + }); + + // MAP_READ can only be paired with COPY_DST and MAP_WRITE can only be paired with COPY_SRC + // (unless Features::MAPPABlE_PRIMARY_BUFFERS is enabled). + [ + Bu::MAP_READ | Bu::COPY_DST | Bu::COPY_SRC, + Bu::MAP_WRITE | Bu::COPY_SRC | Bu::COPY_DST, + Bu::MAP_READ | Bu::MAP_WRITE, + Bu::MAP_WRITE | Bu::MAP_READ, + Bu::MAP_READ | Bu::COPY_DST | Bu::STORAGE, + Bu::MAP_WRITE | Bu::COPY_SRC | Bu::STORAGE, + wgpu::BufferUsages::all(), + ] + .into_iter() + .for_each(|usages| { + // Only valid when MAPPABLE_PRIMARY_BUFFERS is enabled. + try_create(usages, false, true); + try_create(usages, true, false); + }); + + // Buffers cannot have empty usage flags + [Bu::empty()].into_iter().for_each(|usages| { + // Should always fail + try_create(usages, false, true); + try_create(usages, true, true); + }); +} + +const BUFFER_SIZE: BufferAddress = 1234; diff --git a/wgpu/tests/root.rs b/wgpu/tests/root.rs index c8ad63e175..788767131e 100644 --- a/wgpu/tests/root.rs +++ b/wgpu/tests/root.rs @@ -2,6 +2,7 @@ mod common; mod buffer_copy; +mod buffer_usages; mod clear_texture; mod device; mod example_wgsl; From 14d260ac5385250583cd1620552c1c02eff5de57 Mon Sep 17 00:00:00 2001 From: Imbris Date: Tue, 13 Sep 2022 21:21:55 -0400 Subject: [PATCH 2/5] Document size requirement for mapped_at_creation buffers --- wgpu-types/src/lib.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/wgpu-types/src/lib.rs b/wgpu-types/src/lib.rs index 8718d79794..c48c93aaa8 100644 --- a/wgpu-types/src/lib.rs +++ b/wgpu-types/src/lib.rs @@ -2913,6 +2913,9 @@ pub struct BufferDescriptor { pub usage: BufferUsages, /// Allows a buffer to be mapped immediately after they are made. It does not have to be [`BufferUsages::MAP_READ`] or /// [`BufferUsages::MAP_WRITE`], all buffers are allowed to be mapped at creation. + /// + /// If this is `true`, [`size`](#structfield.size) must be a multiple of + /// [`COPY_BUFFER_ALIGNMENT`]. pub mapped_at_creation: bool, } From 36bf6b3c06fd64cdd164d6c44c7dc656360d2fee Mon Sep 17 00:00:00 2001 From: Imbris Date: Tue, 13 Sep 2022 21:50:40 -0400 Subject: [PATCH 3/5] Add changelog entry --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3555702a5a..aa1d7e4471 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -68,6 +68,8 @@ the same every time it is rendered, we now warn if it is missing. - Improve the validation and error reporting of buffer mappings by @nical in [#2848](https://github.com/gfx-rs/wgpu/pull/2848) - Fix compilation errors when using wgpu-core in isolation while targetting `wasm32-unknown-unknown` by @Seamooo in [#2922](https://github.com/gfx-rs/wgpu/pull/2922) - Fixed opening of RenderDoc library by @abuffseagull in [#2930](https://github.com/gfx-rs/wgpu/pull/2930) +- Added missing validation for `BufferUsages` mismatches when `Features::MAPPABLE_PRIMARY_BUFFERS` is not + enabled. By @imberflur in [#3023](https://github.com/gfx-rs/wgpu/pull/3023) #### Metal - Add the missing `msg_send![view, retain]` call within `from_view` by @jinleili in [#2976](https://github.com/gfx-rs/wgpu/pull/2976) From b924a4d534429f21eca15a9d6078d48fcdf76cf7 Mon Sep 17 00:00:00 2001 From: Imbris Date: Tue, 13 Sep 2022 22:27:57 -0400 Subject: [PATCH 4/5] Add MAPPABLE_PRIMARY_BUFFERS feature to player tests that were failing --- player/tests/data/buffer-copy.ron | 4 ++-- player/tests/data/clear-buffer-texture.ron | 2 +- player/tests/data/zero-init-buffer.ron | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/player/tests/data/buffer-copy.ron b/player/tests/data/buffer-copy.ron index c2fad09a28..58cce3b1c2 100644 --- a/player/tests/data/buffer-copy.ron +++ b/player/tests/data/buffer-copy.ron @@ -1,5 +1,5 @@ ( - features: 0x0, + features: 0x1_0000, expectations: [ ( name: "basic", @@ -29,4 +29,4 @@ ), Submit(1, []), ], -) \ No newline at end of file +) diff --git a/player/tests/data/clear-buffer-texture.ron b/player/tests/data/clear-buffer-texture.ron index fd54ca9ea5..023164282f 100644 --- a/player/tests/data/clear-buffer-texture.ron +++ b/player/tests/data/clear-buffer-texture.ron @@ -1,5 +1,5 @@ ( - features: 0x0000_0004_0000_0000, + features: 0x0000_0004_0001_0000, expectations: [ ( name: "Quad", diff --git a/player/tests/data/zero-init-buffer.ron b/player/tests/data/zero-init-buffer.ron index 1381ea7393..ca75c658fc 100644 --- a/player/tests/data/zero-init-buffer.ron +++ b/player/tests/data/zero-init-buffer.ron @@ -1,5 +1,5 @@ ( - features: 0x0, + features: 0x1_0000, expectations: [ // Ensuring that mapping zero-inits buffers. ( From 2b72e8c87380f61155a51c096738e25f0c58ccac Mon Sep 17 00:00:00 2001 From: Imbris Date: Tue, 13 Sep 2022 22:46:22 -0400 Subject: [PATCH 5/5] Refactor buffer_usages test to spend less time setting up to test each case --- wgpu/tests/buffer_usages.rs | 53 +++++++++++++++---------------------- 1 file changed, 22 insertions(+), 31 deletions(-) diff --git a/wgpu/tests/buffer_usages.rs b/wgpu/tests/buffer_usages.rs index 25a40440e6..ebf679ca05 100644 --- a/wgpu/tests/buffer_usages.rs +++ b/wgpu/tests/buffer_usages.rs @@ -7,7 +7,7 @@ use crate::common::{initialize_test, TestParameters}; #[test] fn buffer_usage() { fn try_create( - usage: wgpu::BufferUsages, + usages: &[wgpu::BufferUsages], enable_mappable_primary_buffers: bool, should_panic: bool, ) { @@ -20,34 +20,28 @@ fn buffer_usage() { } initialize_test(parameters, |ctx| { - let _buffer = ctx.device.create_buffer(&wgpu::BufferDescriptor { - label: None, - size: BUFFER_SIZE, - usage, - mapped_at_creation: false, - }); + for usage in usages.iter().copied() { + let _buffer = ctx.device.create_buffer(&wgpu::BufferDescriptor { + label: None, + size: BUFFER_SIZE, + usage, + mapped_at_creation: false, + }); + } }); } use wgpu::BufferUsages as Bu; - // These are always valid - [ + let always_valid = [ Bu::MAP_READ, Bu::MAP_WRITE, Bu::MAP_READ | Bu::COPY_DST, Bu::MAP_WRITE | Bu::COPY_SRC, - ] - .into_iter() - .for_each(|usages| { - // Should only pass validation when MAPPABLE_PRIMARY_BUFFERS is enabled. - try_create(usages, false, false); - try_create(usages, true, false); - }); - + ]; // MAP_READ can only be paired with COPY_DST and MAP_WRITE can only be paired with COPY_SRC // (unless Features::MAPPABlE_PRIMARY_BUFFERS is enabled). - [ + let needs_mappable_primary_buffers = [ Bu::MAP_READ | Bu::COPY_DST | Bu::COPY_SRC, Bu::MAP_WRITE | Bu::COPY_SRC | Bu::COPY_DST, Bu::MAP_READ | Bu::MAP_WRITE, @@ -55,20 +49,17 @@ fn buffer_usage() { Bu::MAP_READ | Bu::COPY_DST | Bu::STORAGE, Bu::MAP_WRITE | Bu::COPY_SRC | Bu::STORAGE, wgpu::BufferUsages::all(), - ] - .into_iter() - .for_each(|usages| { - // Only valid when MAPPABLE_PRIMARY_BUFFERS is enabled. - try_create(usages, false, true); - try_create(usages, true, false); - }); + ]; + let always_fail = [Bu::empty()]; + + try_create(&always_valid, false, false); + try_create(&always_valid, true, false); + + try_create(&needs_mappable_primary_buffers, false, true); + try_create(&needs_mappable_primary_buffers, true, false); - // Buffers cannot have empty usage flags - [Bu::empty()].into_iter().for_each(|usages| { - // Should always fail - try_create(usages, false, true); - try_create(usages, true, true); - }); + try_create(&always_fail, false, true); + try_create(&always_fail, true, true); } const BUFFER_SIZE: BufferAddress = 1234;