From 3e5e6fce107b7d7b6d29e8f949139ea8d2d8c3cc Mon Sep 17 00:00:00 2001 From: Brad Werth Date: Wed, 6 Mar 2024 15:52:51 -0800 Subject: [PATCH] Invoke a DeviceLostClosure immediately if set on an invalid device. To make the device invalid, this defines an explicit, test-only method make_invalid. It also modifies calls that expect to always retrieve a valid device. --- CHANGELOG.md | 1 + tests/tests/device.rs | 31 +++++++++++++++++++++++++++++++ wgpu-core/src/device/global.rs | 17 ++++++++++++++++- wgpu-types/src/lib.rs | 6 ++++++ wgpu/src/backend/webgpu.rs | 4 ++++ wgpu/src/backend/wgpu_core.rs | 10 ++++++---- wgpu/src/context.rs | 8 ++++++++ wgpu/src/lib.rs | 5 +++++ 8 files changed, 77 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3061ec4421..40b241d976 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -143,6 +143,7 @@ By @cwfitzgerald in [#5325](https://github.com/gfx-rs/wgpu/pull/5325). - Fix behavior of integer `clamp` when `min` argument > `max` argument. By @cwfitzgerald in [#5300](https://github.com/gfx-rs/wgpu/pull/5300). - Fix missing validation for `Device::clear_buffer` where `offset + size buffer.size` was not checked when `size` was omitted. By @ErichDonGubler in [#5282](https://github.com/gfx-rs/wgpu/pull/5282). - Fix linking when targeting android. By @ashdnazg in [#5326](https://github.com/gfx-rs/wgpu/pull/5326). +- Failing to set the device lost closure will call the closure before returning. By @bradwerth in [#5358](https://github.com/gfx-rs/wgpu/pull/5358). #### glsl-in diff --git a/tests/tests/device.rs b/tests/tests/device.rs index 02e2d1fb83..33ef180847 100644 --- a/tests/tests/device.rs +++ b/tests/tests/device.rs @@ -580,6 +580,37 @@ static DEVICE_DROP_THEN_LOST: GpuTestConfiguration = GpuTestConfiguration::new() ); }); +#[gpu_test] +static DEVICE_INVALID_THEN_SET_LOST_CALLBACK: GpuTestConfiguration = GpuTestConfiguration::new() + .parameters(TestParameters::default().expect_fail(FailureCase::webgl2())) + .run_sync(|ctx| { + // This test checks that when the device is invalid, a subsequent call + // to set the device lost callback will immediately call the callback. + // Invalidating the device is done via a testing-only method. Fails on + // webgl because webgl doesn't implement make_invalid. + + // Make the device invalid. + ctx.device.make_invalid(); + + let was_called = std::sync::Arc::::new(false.into()); + + // Set a LoseDeviceCallback on the device. + let was_called_clone = was_called.clone(); + let callback = Box::new(move |reason, _m| { + was_called_clone.store(true, std::sync::atomic::Ordering::SeqCst); + assert!( + matches!(reason, wgt::DeviceLostReason::DeviceInvalid), + "Device lost info reason should match DeviceLostReason::DeviceInvalid." + ); + }); + ctx.device.set_device_lost_callback(callback); + + assert!( + was_called.load(std::sync::atomic::Ordering::SeqCst), + "Device lost callback should have been called." + ); + }); + #[gpu_test] static DEVICE_LOST_REPLACED_CALLBACK: GpuTestConfiguration = GpuTestConfiguration::new() .parameters(TestParameters::default()) diff --git a/wgpu-core/src/device/global.rs b/wgpu-core/src/device/global.rs index 539b92e0f3..1914ceb05f 100644 --- a/wgpu-core/src/device/global.rs +++ b/wgpu-core/src/device/global.rs @@ -2240,6 +2240,15 @@ impl Global { } } + // This is a test-only function to force the device into an + // invalid state by inserting an error value in its place in + // the registry. + pub fn device_make_invalid(&self, device_id: DeviceId) { + let hub = A::hub(self); + hub.devices + .force_replace_with_error(device_id, "Made invalid."); + } + pub fn device_drop(&self, device_id: DeviceId) { profiling::scope!("Device::drop"); api_log!("Device::drop {device_id:?}"); @@ -2275,7 +2284,7 @@ impl Global { ) { let hub = A::hub(self); - if let Ok(device) = hub.devices.get(device_id) { + if let Ok(Some(device)) = hub.devices.try_get(device_id) { let mut life_tracker = device.lock_life(); if let Some(existing_closure) = life_tracker.device_lost_closure.take() { // It's important to not hold the lock while calling the closure. @@ -2284,6 +2293,12 @@ impl Global { life_tracker = device.lock_life(); } life_tracker.device_lost_closure = Some(device_lost_closure); + } else { + // No device? Okay. Just like we have to call any existing closure + // before we drop it, we need to call this closure before we exit + // this function, because there's no device that is ever going to + // call it. + device_lost_closure.call(DeviceLostReason::DeviceInvalid, "".to_string()); } } diff --git a/wgpu-types/src/lib.rs b/wgpu-types/src/lib.rs index 347aad76f9..f8b0108505 100644 --- a/wgpu-types/src/lib.rs +++ b/wgpu-types/src/lib.rs @@ -7203,4 +7203,10 @@ pub enum DeviceLostReason { /// exactly once before it is dropped, which helps with managing the /// memory owned by the callback. ReplacedCallback = 3, + /// When setting the callback, but the device is already invalid + /// + /// As above, when the callback is provided, wgpu guarantees that it + /// will eventually be called. If the device is already invalid, wgpu + /// will call the callback immediately, with this reason. + DeviceInvalid = 4, } diff --git a/wgpu/src/backend/webgpu.rs b/wgpu/src/backend/webgpu.rs index 6aeacd555e..05f39beb4a 100644 --- a/wgpu/src/backend/webgpu.rs +++ b/wgpu/src/backend/webgpu.rs @@ -1948,6 +1948,10 @@ impl crate::context::Context for ContextWebGpu { create_identified(device_data.0.create_render_bundle_encoder(&mapped_desc)) } + fn device_make_invalid(&self, _device: &Self::DeviceId, _device_data: &Self::DeviceData) { + // Unimplemented + } + fn device_drop(&self, _device: &Self::DeviceId, _device_data: &Self::DeviceData) { // Device is dropped automatically } diff --git a/wgpu/src/backend/wgpu_core.rs b/wgpu/src/backend/wgpu_core.rs index 14afcb9e1f..728f515915 100644 --- a/wgpu/src/backend/wgpu_core.rs +++ b/wgpu/src/backend/wgpu_core.rs @@ -1346,14 +1346,16 @@ impl crate::Context for ContextWgpuCore { Err(e) => panic!("Error in Device::create_render_bundle_encoder: {e}"), } } + fn device_make_invalid(&self, device: &Self::DeviceId, _device_data: &Self::DeviceData) { + wgc::gfx_select!(device => self.0.device_make_invalid(*device)); + } #[cfg_attr(not(any(native, Emscripten)), allow(unused))] fn device_drop(&self, device: &Self::DeviceId, _device_data: &Self::DeviceData) { #[cfg(any(native, Emscripten))] { - match wgc::gfx_select!(device => self.0.device_poll(*device, wgt::Maintain::wait())) { - Ok(_) => {} - Err(err) => self.handle_error_fatal(err, "Device::drop"), - } + // Call device_poll, but don't check for errors. We have to use its + // return value, but we just drop it. + let _ = wgc::gfx_select!(device => self.0.device_poll(*device, wgt::Maintain::wait())); wgc::gfx_select!(device => self.0.device_drop(*device)); } } diff --git a/wgpu/src/context.rs b/wgpu/src/context.rs index ba1e52ef71..529c45c2b3 100644 --- a/wgpu/src/context.rs +++ b/wgpu/src/context.rs @@ -267,6 +267,7 @@ pub trait Context: Debug + WasmNotSendSync + Sized { device_data: &Self::DeviceData, desc: &RenderBundleEncoderDescriptor<'_>, ) -> (Self::RenderBundleEncoderId, Self::RenderBundleEncoderData); + fn device_make_invalid(&self, device: &Self::DeviceId, device_data: &Self::DeviceData); fn device_drop(&self, device: &Self::DeviceId, device_data: &Self::DeviceData); fn device_set_device_lost_callback( &self, @@ -1293,6 +1294,7 @@ pub(crate) trait DynContext: Debug + WasmNotSendSync { device_data: &crate::Data, desc: &RenderBundleEncoderDescriptor<'_>, ) -> (ObjectId, Box); + fn device_make_invalid(&self, device: &ObjectId, device_data: &crate::Data); fn device_drop(&self, device: &ObjectId, device_data: &crate::Data); fn device_set_device_lost_callback( &self, @@ -2350,6 +2352,12 @@ where (render_bundle_encoder.into(), Box::new(data) as _) } + fn device_make_invalid(&self, device: &ObjectId, device_data: &crate::Data) { + let device = ::from(*device); + let device_data = downcast_ref(device_data); + Context::device_make_invalid(self, &device, device_data) + } + fn device_drop(&self, device: &ObjectId, device_data: &crate::Data) { let device = ::from(*device); let device_data = downcast_ref(device_data); diff --git a/wgpu/src/lib.rs b/wgpu/src/lib.rs index ba4f0f052f..6e8536597c 100644 --- a/wgpu/src/lib.rs +++ b/wgpu/src/lib.rs @@ -2703,6 +2703,11 @@ impl Device { Box::new(callback), ) } + + /// Test-only function to make this device invalid. + pub fn make_invalid(&self) { + DynContext::device_make_invalid(&*self.context, &self.id, self.data.as_ref()) + } } impl Drop for Device {