From 8c7c5c4974717414d49ffbd19e91a337f2db7861 Mon Sep 17 00:00:00 2001 From: teoxoy <28601907+teoxoy@users.noreply.github.com> Date: Fri, 2 Aug 2024 13:54:32 +0200 Subject: [PATCH] decouple device and queue IDs Devices and queues can have different lifetimes, we shouldn't assume that their IDs match. --- player/src/bin/play.rs | 15 ++++++++------- player/src/lib.rs | 11 ++++++----- player/tests/test.rs | 5 +++-- tests/tests/device.rs | 27 ++++++++++++++++++++++++++- wgpu-core/src/device/global.rs | 15 +++------------ wgpu-core/src/device/life.rs | 5 ++--- wgpu-core/src/device/queue.rs | 14 ++------------ wgpu-core/src/device/resource.rs | 23 +++++++++++++++++------ wgpu-core/src/id.rs | 6 ------ wgpu-core/src/lib.rs | 2 +- wgpu/src/backend/wgpu_core.rs | 4 ++-- 11 files changed, 70 insertions(+), 57 deletions(-) diff --git a/player/src/bin/play.rs b/player/src/bin/play.rs index 6510ab23cd..8b6555369f 100644 --- a/player/src/bin/play.rs +++ b/player/src/bin/play.rs @@ -61,7 +61,7 @@ fn main() { } .unwrap(); - let device = match actions.pop() { + let (device, queue) = match actions.pop() { Some(trace::Action::Init { desc, backend }) => { log::info!("Initializing the device for backend: {:?}", backend); let adapter = global @@ -80,18 +80,19 @@ fn main() { let info = gfx_select!(adapter => global.adapter_get_info(adapter)).unwrap(); log::info!("Picked '{}'", info.name); - let id = wgc::id::Id::zip(1, 0, backend); + let device_id = wgc::id::Id::zip(1, 0, backend); + let queue_id = wgc::id::Id::zip(1, 0, backend); let (_, _, error) = gfx_select!(adapter => global.adapter_request_device( adapter, &desc, None, - Some(id), - Some(id.into_queue_id()) + Some(device_id), + Some(queue_id) )); if let Some(e) = error { panic!("{:?}", e); } - id + (device_id, queue_id) } _ => panic!("Expected Action::Init"), }; @@ -102,7 +103,7 @@ fn main() { gfx_select!(device => global.device_start_capture(device)); while let Some(action) = actions.pop() { - gfx_select!(device => global.process(device, action, &dir, &mut command_buffer_id_manager)); + gfx_select!(device => global.process(device, queue, action, &dir, &mut command_buffer_id_manager)); } gfx_select!(device => global.device_stop_capture(device)); @@ -156,7 +157,7 @@ fn main() { target.exit(); } Some(action) => { - gfx_select!(device => global.process(device, action, &dir, &mut command_buffer_id_manager)); + gfx_select!(device => global.process(device, queue, action, &dir, &mut command_buffer_id_manager)); } None => { if !done { diff --git a/player/src/lib.rs b/player/src/lib.rs index 4ec9116ead..5efeff1537 100644 --- a/player/src/lib.rs +++ b/player/src/lib.rs @@ -16,6 +16,7 @@ pub trait GlobalPlay { fn process( &self, device: wgc::id::DeviceId, + queue: wgc::id::QueueId, action: trace::Action, dir: &Path, comb_manager: &mut wgc::identity::IdentityManager, @@ -131,6 +132,7 @@ impl GlobalPlay for wgc::global::Global { fn process( &self, device: wgc::id::DeviceId, + queue: wgc::id::QueueId, action: trace::Action, dir: &Path, comb_manager: &mut wgc::identity::IdentityManager, @@ -327,7 +329,7 @@ impl GlobalPlay for wgc::global::Global { let bin = std::fs::read(dir.join(data)).unwrap(); let size = (range.end - range.start) as usize; if queued { - self.queue_write_buffer::(device.into_queue_id(), id, range.start, &bin) + self.queue_write_buffer::(queue, id, range.start, &bin) .unwrap(); } else { self.device_set_buffer_data::(id, range.start, &bin[..size]) @@ -341,11 +343,11 @@ impl GlobalPlay for wgc::global::Global { size, } => { let bin = std::fs::read(dir.join(data)).unwrap(); - self.queue_write_texture::(device.into_queue_id(), &to, &bin, &layout, &size) + self.queue_write_texture::(queue, &to, &bin, &layout, &size) .unwrap(); } Action::Submit(_index, ref commands) if commands.is_empty() => { - self.queue_submit::(device.into_queue_id(), &[]).unwrap(); + self.queue_submit::(queue, &[]).unwrap(); } Action::Submit(_index, commands) => { let (encoder, error) = self.device_create_command_encoder::( @@ -361,8 +363,7 @@ impl GlobalPlay for wgc::global::Global { panic!("{e}"); } let cmdbuf = self.encode_commands::(encoder, commands); - self.queue_submit::(device.into_queue_id(), &[cmdbuf]) - .unwrap(); + self.queue_submit::(queue, &[cmdbuf]).unwrap(); } } } diff --git a/player/tests/test.rs b/player/tests/test.rs index b3ca944921..f16e7fa32b 100644 --- a/player/tests/test.rs +++ b/player/tests/test.rs @@ -106,6 +106,7 @@ impl Test<'_> { ) { let backend = adapter.backend(); let device_id = wgc::id::Id::zip(test_num, 0, backend); + let queue_id = wgc::id::Id::zip(test_num, 0, backend); let (_, _, error) = wgc::gfx_select!(adapter => global.adapter_request_device( adapter, &wgt::DeviceDescriptor { @@ -116,7 +117,7 @@ impl Test<'_> { }, None, Some(device_id), - Some(device_id.into_queue_id()) + Some(queue_id) )); if let Some(e) = error { panic!("{:?}", e); @@ -125,7 +126,7 @@ impl Test<'_> { let mut command_buffer_id_manager = wgc::identity::IdentityManager::new(); println!("\t\t\tRunning..."); for action in self.actions { - wgc::gfx_select!(device_id => global.process(device_id, action, dir, &mut command_buffer_id_manager)); + wgc::gfx_select!(device_id => global.process(device_id, queue_id, action, dir, &mut command_buffer_id_manager)); } println!("\t\t\tMapping..."); for expect in &self.expectations { diff --git a/tests/tests/device.rs b/tests/tests/device.rs index a577379c20..d629f1b8e6 100644 --- a/tests/tests/device.rs +++ b/tests/tests/device.rs @@ -1,6 +1,8 @@ use std::sync::atomic::AtomicBool; -use wgpu_test::{fail, gpu_test, FailureCase, GpuTestConfiguration, TestParameters}; +use wgpu_test::{ + fail, gpu_test, FailureCase, GpuTestConfiguration, TestParameters, TestingContext, +}; #[gpu_test] static CROSS_DEVICE_BIND_GROUP_USAGE: GpuTestConfiguration = GpuTestConfiguration::new() @@ -908,3 +910,26 @@ static DEVICE_DESTROY_THEN_BUFFER_CLEANUP: GpuTestConfiguration = GpuTestConfigu // Poll the device, which should try to clean up its resources. ctx.instance.poll_all(true); }); + +#[gpu_test] +static DEVICE_AND_QUEUE_HAVE_DIFFERENT_IDS: GpuTestConfiguration = GpuTestConfiguration::new() + .parameters(TestParameters::default()) + .run_async(|ctx| async move { + let TestingContext { + adapter, + device_features, + device_limits, + device, + queue, + .. + } = ctx; + + drop(device); + + let (device2, queue2) = + wgpu_test::initialize_device(&adapter, device_features, device_limits).await; + + drop(queue); + drop(device2); + drop(queue2); // this would previously panic since we would try to use the Device ID to drop the Queue + }); diff --git a/wgpu-core/src/device/global.rs b/wgpu-core/src/device/global.rs index 0df0bc377a..cd3d8e5f20 100644 --- a/wgpu-core/src/device/global.rs +++ b/wgpu-core/src/device/global.rs @@ -7,7 +7,7 @@ use crate::{ ResolvedBindGroupEntry, ResolvedBindingResource, ResolvedBufferBinding, }, command, conv, - device::{bgl, life::WaitIdleError, queue, DeviceError, DeviceLostClosure, DeviceLostReason}, + device::{bgl, life::WaitIdleError, DeviceError, DeviceLostClosure, DeviceLostReason}, global::Global, hal_api::HalApi, id::{self, AdapterId, DeviceId, QueueId, SurfaceId}, @@ -2040,7 +2040,7 @@ impl Global { pub fn device_poll( &self, device_id: DeviceId, - maintain: wgt::Maintain, + maintain: wgt::Maintain, ) -> Result { api_log!("Device::poll {maintain:?}"); @@ -2050,15 +2050,6 @@ impl Global { .get(device_id) .map_err(|_| DeviceError::InvalidDeviceId)?; - if let wgt::Maintain::WaitForSubmissionIndex(submission_index) = maintain { - if submission_index.queue_id != device_id.into_queue_id() { - return Err(WaitIdleError::WrongSubmissionIndex( - submission_index.queue_id, - device_id, - )); - } - } - let DevicePoll { closures, queue_empty, @@ -2071,7 +2062,7 @@ impl Global { fn poll_single_device( device: &crate::device::Device, - maintain: wgt::Maintain, + maintain: wgt::Maintain, ) -> Result { let snatch_guard = device.snatchable_lock.read(); let fence = device.fence.read(); diff --git a/wgpu-core/src/device/life.rs b/wgpu-core/src/device/life.rs index b282775ac0..1bb687d7e2 100644 --- a/wgpu-core/src/device/life.rs +++ b/wgpu-core/src/device/life.rs @@ -4,7 +4,6 @@ use crate::{ DeviceError, DeviceLostClosure, }, hal_api::HalApi, - id, resource::{self, Buffer, Texture, Trackable}, snatch::SnatchGuard, SubmissionIndex, @@ -112,8 +111,8 @@ impl ActiveSubmission { pub enum WaitIdleError { #[error(transparent)] Device(#[from] DeviceError), - #[error("Tried to wait using a submission index from the wrong device. Submission index is from device {0:?}. Called poll on device {1:?}.")] - WrongSubmissionIndex(id::QueueId, id::DeviceId), + #[error("Tried to wait using a submission index ({0}) that has not been returned by a successful submission (last successful submission: {1})")] + WrongSubmissionIndex(SubmissionIndex, SubmissionIndex), #[error("GPU got stuck :(")] StuckGpu, } diff --git a/wgpu-core/src/device/queue.rs b/wgpu-core/src/device/queue.rs index 81c9729521..1b562d560c 100644 --- a/wgpu-core/src/device/queue.rs +++ b/wgpu-core/src/device/queue.rs @@ -131,13 +131,6 @@ impl SubmittedWorkDoneClosure { } } -#[repr(C)] -#[derive(Debug, Copy, Clone)] -pub struct WrappedSubmissionIndex { - pub queue_id: QueueId, - pub index: SubmissionIndex, -} - /// A texture or buffer to be freed soon. /// /// This is just a tagged raw texture or buffer, generally about to be added to @@ -1044,7 +1037,7 @@ impl Global { &self, queue_id: QueueId, command_buffer_ids: &[id::CommandBufferId], - ) -> Result { + ) -> Result { profiling::scope!("Queue::submit"); api_log!("Queue::submit {queue_id:?}"); @@ -1351,10 +1344,7 @@ impl Global { api_log!("Queue::submit to {queue_id:?} returned submit index {submit_index}"); - Ok(WrappedSubmissionIndex { - queue_id, - index: submit_index, - }) + Ok(submit_index) } pub fn queue_get_timestamp_period( diff --git a/wgpu-core/src/device/resource.rs b/wgpu-core/src/device/resource.rs index e66e452063..154430a2f8 100644 --- a/wgpu-core/src/device/resource.rs +++ b/wgpu-core/src/device/resource.rs @@ -55,8 +55,8 @@ use std::{ }; use super::{ - queue::{self, Queue}, - DeviceDescriptor, DeviceError, UserClosures, ENTRYPOINT_FAILURE_ERROR, ZERO_BUFFER_SIZE, + queue::Queue, DeviceDescriptor, DeviceError, UserClosures, ENTRYPOINT_FAILURE_ERROR, + ZERO_BUFFER_SIZE, }; /// Structure describing a logical device. Some members are internally mutable, @@ -407,7 +407,7 @@ impl Device { pub(crate) fn maintain<'this>( &'this self, fence_guard: crate::lock::RwLockReadGuard>, - maintain: wgt::Maintain, + maintain: wgt::Maintain, snatch_guard: SnatchGuard, ) -> Result<(UserClosures, bool), WaitIdleError> { profiling::scope!("Device::maintain"); @@ -417,9 +417,20 @@ impl Device { // Determine which submission index `maintain` represents. let submission_index = match maintain { wgt::Maintain::WaitForSubmissionIndex(submission_index) => { - // We don't need to check to see if the queue id matches - // as we already checked this from inside the poll call. - submission_index.index + let last_successful_submission_index = self + .last_successful_submission_index + .load(Ordering::Acquire); + + if let wgt::Maintain::WaitForSubmissionIndex(submission_index) = maintain { + if submission_index > last_successful_submission_index { + return Err(WaitIdleError::WrongSubmissionIndex( + submission_index, + last_successful_submission_index, + )); + } + } + + submission_index } wgt::Maintain::Wait => self .last_successful_submission_index diff --git a/wgpu-core/src/id.rs b/wgpu-core/src/id.rs index c795063da5..83b2494391 100644 --- a/wgpu-core/src/id.rs +++ b/wgpu-core/src/id.rs @@ -326,12 +326,6 @@ impl CommandBufferId { } } -impl DeviceId { - pub fn into_queue_id(self) -> QueueId { - Id(self.0, PhantomData) - } -} - #[test] fn test_id_backend() { for &b in &[ diff --git a/wgpu-core/src/lib.rs b/wgpu-core/src/lib.rs index c46a8f103a..351916002f 100644 --- a/wgpu-core/src/lib.rs +++ b/wgpu-core/src/lib.rs @@ -102,7 +102,7 @@ pub(crate) use hash_utils::*; /// The index of a queue submission. /// /// These are the values stored in `Device::fence`. -type SubmissionIndex = hal::FenceValue; +pub type SubmissionIndex = hal::FenceValue; type Index = u32; type Epoch = u32; diff --git a/wgpu/src/backend/wgpu_core.rs b/wgpu/src/backend/wgpu_core.rs index 0adf8c3e59..06632d68dd 100644 --- a/wgpu/src/backend/wgpu_core.rs +++ b/wgpu/src/backend/wgpu_core.rs @@ -552,7 +552,7 @@ impl crate::Context for ContextWgpuCore { type SurfaceId = wgc::id::SurfaceId; type SurfaceData = Surface; type SurfaceOutputDetail = SurfaceOutputDetail; - type SubmissionIndexData = wgc::device::queue::WrappedSubmissionIndex; + type SubmissionIndexData = wgc::SubmissionIndex; type RequestAdapterFuture = Ready>; @@ -666,7 +666,7 @@ impl crate::Context for ContextWgpuCore { id: queue_id, error_sink, }; - ready(Ok((device_id, device, device_id.into_queue_id(), queue))) + ready(Ok((device_id, device, queue_id, queue))) } fn instance_poll_all_devices(&self, force_wait: bool) -> bool {