Skip to content

Commit 9a27ba5

Browse files
WumpfWumpf
authored
Fix QuerySet ownership of ComputePass (#5671)
* add new tests for checking on query set lifetime * Fix ownership management of query sets on compute passes for write_timestamp, timestamp_writes (on desc) and pipeline statistic queries * changelog entry
1 parent d258d6c commit 9a27ba5

File tree

6 files changed

+338
-206
lines changed

6 files changed

+338
-206
lines changed

‎CHANGELOG.md

+2-1
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ fn independent_cpass<'enc>(encoder: &'enc mut wgpu::CommandEncoder) -> wgpu::Com
5858
This is very useful for library authors, but opens up an easy way for incorrect use, so use with care.
5959
`forget_lifetime` is zero overhead and has no side effects on pass recording.
6060

61-
By @wumpf in [#5569](https://github.com/gfx-rs/wgpu/pull/5569), [#5575](https://github.com/gfx-rs/wgpu/pull/5575), [#5620](https://github.com/gfx-rs/wgpu/pull/5620), [#5768](https://github.com/gfx-rs/wgpu/pull/5768) (together with @kpreid).
61+
By @wumpf in [#5569](https://github.com/gfx-rs/wgpu/pull/5569), [#5575](https://github.com/gfx-rs/wgpu/pull/5575), [#5620](https://github.com/gfx-rs/wgpu/pull/5620), [#5768](https://github.com/gfx-rs/wgpu/pull/5768) (together with @kpreid), [#5671](https://github.com/gfx-rs/wgpu/pull/5671).
6262

6363
#### Querying shader compilation errors
6464

@@ -116,6 +116,7 @@ By @stefnotch in [#5410](https://github.com/gfx-rs/wgpu/pull/5410)
116116
#### General
117117

118118
- Ensure render pipelines have at least 1 target. By @ErichDonGubler in [#5715](https://github.com/gfx-rs/wgpu/pull/5715)
119+
- `wgpu::ComputePass` now internally takes ownership of `QuerySet` for both `wgpu::ComputePassTimestampWrites` as well as timestamp writes and statistics query, fixing crashes when destroying `QuerySet` before ending the pass. By @wumpf in [#5671](https://github.com/gfx-rs/wgpu/pull/5671)
119120

120121
#### Metal
121122

‎tests/tests/compute_pass_ownership.rs

+130-24
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,5 @@
11
//! Tests that compute passes take ownership of resources that are associated with.
22
//! I.e. once a resource is passed in to a compute pass, it can be dropped.
3-
//!
4-
//! TODO: Also should test resource ownership for:
5-
//! * write_timestamp
6-
//! * begin_pipeline_statistics_query
73
84
use std::num::NonZeroU64;
95

@@ -36,15 +32,10 @@ async fn compute_pass_resource_ownership(ctx: TestingContext) {
3632

3733
let mut encoder = ctx
3834
.device
39-
.create_command_encoder(&wgpu::CommandEncoderDescriptor {
40-
label: Some("encoder"),
41-
});
35+
.create_command_encoder(&wgpu::CommandEncoderDescriptor::default());
4236

4337
{
44-
let mut cpass = encoder.begin_compute_pass(&wgpu::ComputePassDescriptor {
45-
label: Some("compute_pass"),
46-
timestamp_writes: None, // TODO: See description above, we should test this as well once we lift the lifetime bound.
47-
});
38+
let mut cpass = encoder.begin_compute_pass(&wgpu::ComputePassDescriptor::default());
4839
cpass.set_pipeline(&pipeline);
4940
cpass.set_bind_group(0, &bind_group, &[]);
5041
cpass.dispatch_workgroups_indirect(&indirect_buffer, 0);
@@ -58,18 +49,115 @@ async fn compute_pass_resource_ownership(ctx: TestingContext) {
5849
.panic_on_timeout();
5950
}
6051

61-
// Ensure that the compute pass still executed normally.
62-
encoder.copy_buffer_to_buffer(&gpu_buffer, 0, &cpu_buffer, 0, buffer_size);
63-
ctx.queue.submit([encoder.finish()]);
64-
cpu_buffer.slice(..).map_async(wgpu::MapMode::Read, |_| ());
65-
ctx.async_poll(wgpu::Maintain::wait())
66-
.await
67-
.panic_on_timeout();
52+
assert_compute_pass_executed_normally(encoder, gpu_buffer, cpu_buffer, buffer_size, ctx).await;
53+
}
6854

69-
let data = cpu_buffer.slice(..).get_mapped_range();
55+
#[gpu_test]
56+
static COMPUTE_PASS_QUERY_SET_OWNERSHIP_PIPELINE_STATISTICS: GpuTestConfiguration =
57+
GpuTestConfiguration::new()
58+
.parameters(
59+
TestParameters::default()
60+
.test_features_limits()
61+
.features(wgpu::Features::PIPELINE_STATISTICS_QUERY),
62+
)
63+
.run_async(compute_pass_query_set_ownership_pipeline_statistics);
64+
65+
async fn compute_pass_query_set_ownership_pipeline_statistics(ctx: TestingContext) {
66+
let ResourceSetup {
67+
gpu_buffer,
68+
cpu_buffer,
69+
buffer_size,
70+
indirect_buffer: _,
71+
bind_group,
72+
pipeline,
73+
} = resource_setup(&ctx);
7074

71-
let floats: &[f32] = bytemuck::cast_slice(&data);
72-
assert_eq!(floats, [2.0, 4.0, 6.0, 8.0]);
75+
let query_set = ctx.device.create_query_set(&wgpu::QuerySetDescriptor {
76+
label: Some("query_set"),
77+
ty: wgpu::QueryType::PipelineStatistics(
78+
wgpu::PipelineStatisticsTypes::COMPUTE_SHADER_INVOCATIONS,
79+
),
80+
count: 1,
81+
});
82+
83+
let mut encoder = ctx
84+
.device
85+
.create_command_encoder(&wgpu::CommandEncoderDescriptor::default());
86+
87+
{
88+
let mut cpass = encoder.begin_compute_pass(&wgpu::ComputePassDescriptor::default());
89+
cpass.set_pipeline(&pipeline);
90+
cpass.set_bind_group(0, &bind_group, &[]);
91+
cpass.begin_pipeline_statistics_query(&query_set, 0);
92+
cpass.dispatch_workgroups(1, 1, 1);
93+
cpass.end_pipeline_statistics_query();
94+
95+
// Drop the query set. Then do a device poll to make sure it's not dropped too early, no matter what.
96+
drop(query_set);
97+
ctx.async_poll(wgpu::Maintain::wait())
98+
.await
99+
.panic_on_timeout();
100+
}
101+
102+
assert_compute_pass_executed_normally(encoder, gpu_buffer, cpu_buffer, buffer_size, ctx).await;
103+
}
104+
105+
#[gpu_test]
106+
static COMPUTE_PASS_QUERY_TIMESTAMPS: GpuTestConfiguration =
107+
GpuTestConfiguration::new()
108+
.parameters(TestParameters::default().test_features_limits().features(
109+
wgpu::Features::TIMESTAMP_QUERY | wgpu::Features::TIMESTAMP_QUERY_INSIDE_PASSES,
110+
))
111+
.run_async(compute_pass_query_timestamps);
112+
113+
async fn compute_pass_query_timestamps(ctx: TestingContext) {
114+
let ResourceSetup {
115+
gpu_buffer,
116+
cpu_buffer,
117+
buffer_size,
118+
indirect_buffer: _,
119+
bind_group,
120+
pipeline,
121+
} = resource_setup(&ctx);
122+
123+
let query_set_timestamp_writes = ctx.device.create_query_set(&wgpu::QuerySetDescriptor {
124+
label: Some("query_set_timestamp_writes"),
125+
ty: wgpu::QueryType::Timestamp,
126+
count: 2,
127+
});
128+
let query_set_write_timestamp = ctx.device.create_query_set(&wgpu::QuerySetDescriptor {
129+
label: Some("query_set_write_timestamp"),
130+
ty: wgpu::QueryType::Timestamp,
131+
count: 1,
132+
});
133+
134+
let mut encoder = ctx
135+
.device
136+
.create_command_encoder(&wgpu::CommandEncoderDescriptor::default());
137+
138+
{
139+
let mut cpass = encoder.begin_compute_pass(&wgpu::ComputePassDescriptor {
140+
label: Some("compute_pass"),
141+
timestamp_writes: Some(wgpu::ComputePassTimestampWrites {
142+
query_set: &query_set_timestamp_writes,
143+
beginning_of_pass_write_index: Some(0),
144+
end_of_pass_write_index: Some(1),
145+
}),
146+
});
147+
cpass.set_pipeline(&pipeline);
148+
cpass.set_bind_group(0, &bind_group, &[]);
149+
cpass.write_timestamp(&query_set_write_timestamp, 0);
150+
cpass.dispatch_workgroups(1, 1, 1);
151+
152+
// Drop the query sets. Then do a device poll to make sure they're not dropped too early, no matter what.
153+
drop(query_set_timestamp_writes);
154+
drop(query_set_write_timestamp);
155+
ctx.async_poll(wgpu::Maintain::wait())
156+
.await
157+
.panic_on_timeout();
158+
}
159+
160+
assert_compute_pass_executed_normally(encoder, gpu_buffer, cpu_buffer, buffer_size, ctx).await;
73161
}
74162

75163
#[gpu_test]
@@ -89,9 +177,7 @@ async fn compute_pass_keep_encoder_alive(ctx: TestingContext) {
89177

90178
let mut encoder = ctx
91179
.device
92-
.create_command_encoder(&wgpu::CommandEncoderDescriptor {
93-
label: Some("encoder"),
94-
});
180+
.create_command_encoder(&wgpu::CommandEncoderDescriptor::default());
95181

96182
let cpass = encoder.begin_compute_pass(&wgpu::ComputePassDescriptor {
97183
label: Some("compute_pass"),
@@ -119,6 +205,26 @@ async fn compute_pass_keep_encoder_alive(ctx: TestingContext) {
119205
valid(&ctx.device, || drop(cpass));
120206
}
121207

208+
async fn assert_compute_pass_executed_normally(
209+
mut encoder: wgpu::CommandEncoder,
210+
gpu_buffer: wgpu::Buffer,
211+
cpu_buffer: wgpu::Buffer,
212+
buffer_size: u64,
213+
ctx: TestingContext,
214+
) {
215+
encoder.copy_buffer_to_buffer(&gpu_buffer, 0, &cpu_buffer, 0, buffer_size);
216+
ctx.queue.submit([encoder.finish()]);
217+
cpu_buffer.slice(..).map_async(wgpu::MapMode::Read, |_| ());
218+
ctx.async_poll(wgpu::Maintain::wait())
219+
.await
220+
.panic_on_timeout();
221+
222+
let data = cpu_buffer.slice(..).get_mapped_range();
223+
224+
let floats: &[f32] = bytemuck::cast_slice(&data);
225+
assert_eq!(floats, [2.0, 4.0, 6.0, 8.0]);
226+
}
227+
122228
// Setup ------------------------------------------------------------
123229

124230
struct ResourceSetup {

0 commit comments

Comments
 (0)