Skip to content

Commit

Permalink
decouple device and queue IDs
Browse files Browse the repository at this point in the history
Devices and queues can have different lifetimes, we shouldn't assume that their IDs match.
  • Loading branch information
teoxoy committed Aug 5, 2024
1 parent de960cc commit 8c7c5c4
Show file tree
Hide file tree
Showing 11 changed files with 70 additions and 57 deletions.
15 changes: 8 additions & 7 deletions player/src/bin/play.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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"),
};
Expand All @@ -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));
Expand Down Expand Up @@ -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 {
Expand Down
11 changes: 6 additions & 5 deletions player/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ pub trait GlobalPlay {
fn process<A: wgc::hal_api::HalApi>(
&self,
device: wgc::id::DeviceId,
queue: wgc::id::QueueId,
action: trace::Action,
dir: &Path,
comb_manager: &mut wgc::identity::IdentityManager<wgc::id::markers::CommandBuffer>,
Expand Down Expand Up @@ -131,6 +132,7 @@ impl GlobalPlay for wgc::global::Global {
fn process<A: wgc::hal_api::HalApi>(
&self,
device: wgc::id::DeviceId,
queue: wgc::id::QueueId,
action: trace::Action,
dir: &Path,
comb_manager: &mut wgc::identity::IdentityManager<wgc::id::markers::CommandBuffer>,
Expand Down Expand Up @@ -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::<A>(device.into_queue_id(), id, range.start, &bin)
self.queue_write_buffer::<A>(queue, id, range.start, &bin)
.unwrap();
} else {
self.device_set_buffer_data::<A>(id, range.start, &bin[..size])
Expand All @@ -341,11 +343,11 @@ impl GlobalPlay for wgc::global::Global {
size,
} => {
let bin = std::fs::read(dir.join(data)).unwrap();
self.queue_write_texture::<A>(device.into_queue_id(), &to, &bin, &layout, &size)
self.queue_write_texture::<A>(queue, &to, &bin, &layout, &size)
.unwrap();
}
Action::Submit(_index, ref commands) if commands.is_empty() => {
self.queue_submit::<A>(device.into_queue_id(), &[]).unwrap();
self.queue_submit::<A>(queue, &[]).unwrap();
}
Action::Submit(_index, commands) => {
let (encoder, error) = self.device_create_command_encoder::<A>(
Expand All @@ -361,8 +363,7 @@ impl GlobalPlay for wgc::global::Global {
panic!("{e}");
}
let cmdbuf = self.encode_commands::<A>(encoder, commands);
self.queue_submit::<A>(device.into_queue_id(), &[cmdbuf])
.unwrap();
self.queue_submit::<A>(queue, &[cmdbuf]).unwrap();
}
}
}
Expand Down
5 changes: 3 additions & 2 deletions player/tests/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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);
Expand All @@ -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 {
Expand Down
27 changes: 26 additions & 1 deletion tests/tests/device.rs
Original file line number Diff line number Diff line change
@@ -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()
Expand Down Expand Up @@ -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
});
15 changes: 3 additions & 12 deletions wgpu-core/src/device/global.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -2040,7 +2040,7 @@ impl Global {
pub fn device_poll<A: HalApi>(
&self,
device_id: DeviceId,
maintain: wgt::Maintain<queue::WrappedSubmissionIndex>,
maintain: wgt::Maintain<crate::SubmissionIndex>,
) -> Result<bool, WaitIdleError> {
api_log!("Device::poll {maintain:?}");

Expand All @@ -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,
Expand All @@ -2071,7 +2062,7 @@ impl Global {

fn poll_single_device<A: HalApi>(
device: &crate::device::Device<A>,
maintain: wgt::Maintain<queue::WrappedSubmissionIndex>,
maintain: wgt::Maintain<crate::SubmissionIndex>,
) -> Result<DevicePoll, WaitIdleError> {
let snatch_guard = device.snatchable_lock.read();
let fence = device.fence.read();
Expand Down
5 changes: 2 additions & 3 deletions wgpu-core/src/device/life.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ use crate::{
DeviceError, DeviceLostClosure,
},
hal_api::HalApi,
id,
resource::{self, Buffer, Texture, Trackable},
snatch::SnatchGuard,
SubmissionIndex,
Expand Down Expand Up @@ -112,8 +111,8 @@ impl<A: HalApi> ActiveSubmission<A> {
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,
}
Expand Down
14 changes: 2 additions & 12 deletions wgpu-core/src/device/queue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -1044,7 +1037,7 @@ impl Global {
&self,
queue_id: QueueId,
command_buffer_ids: &[id::CommandBufferId],
) -> Result<WrappedSubmissionIndex, QueueSubmitError> {
) -> Result<SubmissionIndex, QueueSubmitError> {
profiling::scope!("Queue::submit");
api_log!("Queue::submit {queue_id:?}");

Expand Down Expand Up @@ -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<A: HalApi>(
Expand Down
23 changes: 17 additions & 6 deletions wgpu-core/src/device/resource.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -407,7 +407,7 @@ impl<A: HalApi> Device<A> {
pub(crate) fn maintain<'this>(
&'this self,
fence_guard: crate::lock::RwLockReadGuard<Option<A::Fence>>,
maintain: wgt::Maintain<queue::WrappedSubmissionIndex>,
maintain: wgt::Maintain<crate::SubmissionIndex>,
snatch_guard: SnatchGuard,
) -> Result<(UserClosures, bool), WaitIdleError> {
profiling::scope!("Device::maintain");
Expand All @@ -417,9 +417,20 @@ impl<A: HalApi> Device<A> {
// 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
Expand Down
6 changes: 0 additions & 6 deletions wgpu-core/src/id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 &[
Expand Down
2 changes: 1 addition & 1 deletion wgpu-core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions wgpu/src/backend/wgpu_core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Option<(Self::AdapterId, Self::AdapterData)>>;

Expand Down Expand Up @@ -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 {
Expand Down

0 comments on commit 8c7c5c4

Please sign in to comment.