Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decouple device and queue IDs #6070

Merged
merged 1 commit into from
Aug 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Loading