From b17c30338d37a15fcf0ee737de4d9aff6d3baeac Mon Sep 17 00:00:00 2001 From: Erich Gubler Date: Thu, 21 Nov 2024 09:16:40 -0500 Subject: [PATCH 1/4] fix(core): validate `TIMESTAMP_QUERY` feature before other query set validation in pass creation --- CHANGELOG.md | 2 ++ wgpu-core/src/command/compute.rs | 16 +++++++++------- wgpu-core/src/command/render.rs | 3 ++- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dee57f6e78..137724e88b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -131,6 +131,8 @@ By @ErichDonGubler in [#6456](https://github.com/gfx-rs/wgpu/pull/6456), [#6148] - Lower `QUERY_SET_MAX_QUERIES` (and enforced limits) from 8192 to 4096 to match WebGPU spec. By @ErichDonGubler in [#6525](https://github.com/gfx-rs/wgpu/pull/6525). - Allow non-filterable float on texture bindings never used with samplers when using a derived bind group layout. By @ErichDonGubler in [#6531](https://github.com/gfx-rs/wgpu/pull/6531/). - Replace potentially unsound usage of `PreHashedMap` with `FastHashMap`. By @jamienicol in [#6541](https://github.com/gfx-rs/wgpu/pull/6541). +- Add missing validation for timestamp writes in compute and render passes. By @ErichDonGubler in [#6578](https://github.com/gfx-rs/wgpu/pull/6578). + - Check the status of the `TIMESTAMP_QUERY` feature before other validation. #### Naga diff --git a/wgpu-core/src/command/compute.rs b/wgpu-core/src/command/compute.rs index 28e76c1115..747fddb0d0 100644 --- a/wgpu-core/src/command/compute.rs +++ b/wgpu-core/src/command/compute.rs @@ -309,21 +309,23 @@ impl Global { }; arc_desc.timestamp_writes = if let Some(tw) = desc.timestamp_writes { + match cmd_buf + .device + .require_features(wgt::Features::TIMESTAMP_QUERY) + { + Ok(()) => (), + Err(e) => return make_err(e.into(), arc_desc), + } + let query_set = match hub.query_sets.get(tw.query_set).get() { Ok(query_set) => query_set, Err(e) => return make_err(e.into(), arc_desc), }; + match query_set.same_device(&cmd_buf.device) { Ok(()) => (), Err(e) => return make_err(e.into(), arc_desc), } - match cmd_buf - .device - .require_features(wgt::Features::TIMESTAMP_QUERY) - { - Ok(()) => (), - Err(e) => return make_err(e.into(), arc_desc), - } Some(ArcPassTimestampWrites { query_set, diff --git a/wgpu-core/src/command/render.rs b/wgpu-core/src/command/render.rs index ad3a5b6ca3..db03e1c156 100644 --- a/wgpu-core/src/command/render.rs +++ b/wgpu-core/src/command/render.rs @@ -1393,10 +1393,11 @@ impl Global { arc_desc.timestamp_writes = if let Some(tw) = desc.timestamp_writes { let query_set = query_sets.get(tw.query_set).get()?; - query_set.same_device(device)?; device.require_features(wgt::Features::TIMESTAMP_QUERY)?; + query_set.same_device(device)?; + Some(ArcPassTimestampWrites { query_set, beginning_of_pass_write_index: tw.beginning_of_pass_write_index, From ce1d6e4173b9f11a33b21a01c25382d39a17d1ab Mon Sep 17 00:00:00 2001 From: Erich Gubler Date: Thu, 21 Nov 2024 09:34:44 -0500 Subject: [PATCH 2/4] refactor: destructure timestamp writes in `command_encoder_create_{compute,render}_pass` --- wgpu-core/src/command/compute.rs | 16 ++++++++++++---- wgpu-core/src/command/render.rs | 12 +++++++++--- 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/wgpu-core/src/command/compute.rs b/wgpu-core/src/command/compute.rs index 747fddb0d0..745fe50150 100644 --- a/wgpu-core/src/command/compute.rs +++ b/wgpu-core/src/command/compute.rs @@ -28,7 +28,9 @@ use crate::{ use thiserror::Error; use wgt::{BufferAddress, DynamicOffset}; -use super::{bind::BinderError, memory_init::CommandBufferTextureMemoryActions}; +use super::{ + bind::BinderError, memory_init::CommandBufferTextureMemoryActions, SimplifiedQueryType, +}; use crate::ray_tracing::TlasAction; use std::sync::Arc; use std::{fmt, mem::size_of, str}; @@ -309,6 +311,12 @@ impl Global { }; arc_desc.timestamp_writes = if let Some(tw) = desc.timestamp_writes { + let &PassTimestampWrites { + query_set, + beginning_of_pass_write_index, + end_of_pass_write_index, + } = tw; + match cmd_buf .device .require_features(wgt::Features::TIMESTAMP_QUERY) @@ -317,7 +325,7 @@ impl Global { Err(e) => return make_err(e.into(), arc_desc), } - let query_set = match hub.query_sets.get(tw.query_set).get() { + let query_set = match hub.query_sets.get(query_set).get() { Ok(query_set) => query_set, Err(e) => return make_err(e.into(), arc_desc), }; @@ -329,8 +337,8 @@ impl Global { Some(ArcPassTimestampWrites { query_set, - beginning_of_pass_write_index: tw.beginning_of_pass_write_index, - end_of_pass_write_index: tw.end_of_pass_write_index, + beginning_of_pass_write_index, + end_of_pass_write_index, }) } else { None diff --git a/wgpu-core/src/command/render.rs b/wgpu-core/src/command/render.rs index db03e1c156..9c2119430b 100644 --- a/wgpu-core/src/command/render.rs +++ b/wgpu-core/src/command/render.rs @@ -1392,7 +1392,13 @@ impl Global { }; arc_desc.timestamp_writes = if let Some(tw) = desc.timestamp_writes { - let query_set = query_sets.get(tw.query_set).get()?; + let &PassTimestampWrites { + query_set, + beginning_of_pass_write_index, + end_of_pass_write_index, + } = tw; + + let query_set = query_sets.get(query_set).get()?; device.require_features(wgt::Features::TIMESTAMP_QUERY)?; @@ -1400,8 +1406,8 @@ impl Global { Some(ArcPassTimestampWrites { query_set, - beginning_of_pass_write_index: tw.beginning_of_pass_write_index, - end_of_pass_write_index: tw.end_of_pass_write_index, + beginning_of_pass_write_index, + end_of_pass_write_index, }) } else { None From be50bdfc181cea50a4b0b7f84942a76568ee762b Mon Sep 17 00:00:00 2001 From: Erich Gubler Date: Thu, 21 Nov 2024 09:34:44 -0500 Subject: [PATCH 3/4] fix(core): validate bounds of pass timestamp writes' indices --- CHANGELOG.md | 1 + wgpu-core/src/command/compute.rs | 10 ++++++++++ wgpu-core/src/command/mod.rs | 2 ++ wgpu-core/src/command/query.rs | 2 +- wgpu-core/src/command/render.rs | 8 ++++++++ 5 files changed, 22 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 137724e88b..00b81f6a50 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -133,6 +133,7 @@ By @ErichDonGubler in [#6456](https://github.com/gfx-rs/wgpu/pull/6456), [#6148] - Replace potentially unsound usage of `PreHashedMap` with `FastHashMap`. By @jamienicol in [#6541](https://github.com/gfx-rs/wgpu/pull/6541). - Add missing validation for timestamp writes in compute and render passes. By @ErichDonGubler in [#6578](https://github.com/gfx-rs/wgpu/pull/6578). - Check the status of the `TIMESTAMP_QUERY` feature before other validation. + - Check that indices are in-bounds for the query set. #### Naga diff --git a/wgpu-core/src/command/compute.rs b/wgpu-core/src/command/compute.rs index 745fe50150..dc8c6055c5 100644 --- a/wgpu-core/src/command/compute.rs +++ b/wgpu-core/src/command/compute.rs @@ -335,6 +335,16 @@ impl Global { Err(e) => return make_err(e.into(), arc_desc), } + for idx in [beginning_of_pass_write_index, end_of_pass_write_index] + .into_iter() + .flatten() + { + match query_set.validate_query(SimplifiedQueryType::Timestamp, idx, None) { + Ok(()) => (), + Err(e) => return make_err(e.into(), arc_desc), + } + } + Some(ArcPassTimestampWrites { query_set, beginning_of_pass_write_index, diff --git a/wgpu-core/src/command/mod.rs b/wgpu-core/src/command/mod.rs index 6a06d1f83d..6c7b26b786 100644 --- a/wgpu-core/src/command/mod.rs +++ b/wgpu-core/src/command/mod.rs @@ -654,6 +654,8 @@ pub enum CommandEncoderError { InvalidResource(#[from] InvalidResourceError), #[error(transparent)] MissingFeatures(#[from] MissingFeatures), + #[error(transparent)] + TimestampWritesInvalid(#[from] QueryUseError), } impl Global { diff --git a/wgpu-core/src/command/query.rs b/wgpu-core/src/command/query.rs index d783721fb4..b2dff276eb 100644 --- a/wgpu-core/src/command/query.rs +++ b/wgpu-core/src/command/query.rs @@ -160,7 +160,7 @@ pub enum ResolveError { } impl QuerySet { - fn validate_query( + pub(crate) fn validate_query( self: &Arc, query_type: SimplifiedQueryType, query_index: u32, diff --git a/wgpu-core/src/command/render.rs b/wgpu-core/src/command/render.rs index 9c2119430b..ffec953888 100644 --- a/wgpu-core/src/command/render.rs +++ b/wgpu-core/src/command/render.rs @@ -1,6 +1,7 @@ use crate::binding_model::BindGroup; use crate::command::{ validate_and_begin_occlusion_query, validate_and_begin_pipeline_statistics_query, + SimplifiedQueryType, }; use crate::init_tracker::BufferInitTrackerAction; use crate::pipeline::RenderPipeline; @@ -1404,6 +1405,13 @@ impl Global { query_set.same_device(device)?; + for idx in [beginning_of_pass_write_index, end_of_pass_write_index] + .into_iter() + .flatten() + { + query_set.validate_query(SimplifiedQueryType::Timestamp, idx, None)?; + } + Some(ArcPassTimestampWrites { query_set, beginning_of_pass_write_index, From f1ec934dbc58958b621a9212168e450ce9496c6d Mon Sep 17 00:00:00 2001 From: Erich Gubler Date: Thu, 21 Nov 2024 09:49:20 -0500 Subject: [PATCH 4/4] fix(core): validate that begin and end indices of pass timestamp writes are not equal --- CHANGELOG.md | 1 + wgpu-core/src/command/compute.rs | 9 +++++++++ wgpu-core/src/command/mod.rs | 4 ++++ wgpu-core/src/command/render.rs | 8 ++++++++ 4 files changed, 22 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 00b81f6a50..ee63a2bde9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -134,6 +134,7 @@ By @ErichDonGubler in [#6456](https://github.com/gfx-rs/wgpu/pull/6456), [#6148] - Add missing validation for timestamp writes in compute and render passes. By @ErichDonGubler in [#6578](https://github.com/gfx-rs/wgpu/pull/6578). - Check the status of the `TIMESTAMP_QUERY` feature before other validation. - Check that indices are in-bounds for the query set. + - Check that begin and end indices are not equal. #### Naga diff --git a/wgpu-core/src/command/compute.rs b/wgpu-core/src/command/compute.rs index dc8c6055c5..f27ad1a8bb 100644 --- a/wgpu-core/src/command/compute.rs +++ b/wgpu-core/src/command/compute.rs @@ -345,6 +345,15 @@ impl Global { } } + if let Some((begin, end)) = beginning_of_pass_write_index.zip(end_of_pass_write_index) { + if begin == end { + return make_err( + CommandEncoderError::TimestampWriteIndicesEqual { idx: begin }, + arc_desc, + ); + } + } + Some(ArcPassTimestampWrites { query_set, beginning_of_pass_write_index, diff --git a/wgpu-core/src/command/mod.rs b/wgpu-core/src/command/mod.rs index 6c7b26b786..1f1cafaf5e 100644 --- a/wgpu-core/src/command/mod.rs +++ b/wgpu-core/src/command/mod.rs @@ -654,6 +654,10 @@ pub enum CommandEncoderError { InvalidResource(#[from] InvalidResourceError), #[error(transparent)] MissingFeatures(#[from] MissingFeatures), + #[error( + "begin and end indices of pass timestamp writes are both set to {idx}, which is not allowed" + )] + TimestampWriteIndicesEqual { idx: u32 }, #[error(transparent)] TimestampWritesInvalid(#[from] QueryUseError), } diff --git a/wgpu-core/src/command/render.rs b/wgpu-core/src/command/render.rs index ffec953888..21c9a75f5b 100644 --- a/wgpu-core/src/command/render.rs +++ b/wgpu-core/src/command/render.rs @@ -1412,6 +1412,14 @@ impl Global { query_set.validate_query(SimplifiedQueryType::Timestamp, idx, None)?; } + if let Some((begin, end)) = + beginning_of_pass_write_index.zip(end_of_pass_write_index) + { + if begin == end { + return Err(CommandEncoderError::TimestampWriteIndicesEqual { idx: begin }); + } + } + Some(ArcPassTimestampWrites { query_set, beginning_of_pass_write_index,