From 48b281275e8f9b8dd039e878da55abe40c9b618f Mon Sep 17 00:00:00 2001 From: Brad Werth Date: Mon, 25 Sep 2023 12:27:52 -0700 Subject: [PATCH] Update test to use device.destroy, and remove the public exposure of the lose function. --- CHANGELOG.md | 6 +- tests/tests/device.rs | 198 +++++++++++++++++++++++++++++-- wgpu-core/src/device/global.rs | 34 +++++- wgpu-core/src/device/resource.rs | 21 +++- wgpu/src/backend/direct.rs | 8 +- wgpu/src/backend/web.rs | 7 +- wgpu/src/context.rs | 8 ++ wgpu/src/lib.rs | 7 +- 8 files changed, 258 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b52e449cac..d424038c9b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -101,7 +101,7 @@ By @wumpf in [#4147](https://github.com/gfx-rs/wgpu/pull/4147) - `wgpu::CreateSurfaceError` and `wgpu::RequestDeviceError` now give details of the failure, but no longer implement `PartialEq` and cannot be constructed. By @kpreid in [#4066](https://github.com/gfx-rs/wgpu/pull/4066) and [#4145](https://github.com/gfx-rs/wgpu/pull/4145) - Make `WGPU_POWER_PREF=none` a valid value. By @fornwall in [4076](https://github.com/gfx-rs/wgpu/pull/4076) - Support dual source blending in OpenGL ES, Metal, Vulkan & DX12. By @freqmod in [4022](https://github.com/gfx-rs/wgpu/pull/4022) -- Add stub support for "lose the device" and device validity. By @bradwerth in [4163](https://github.com/gfx-rs/wgpu/pull/4163) +- Add stub support for device destroy and device validity. By @bradwerth in [4163](https://github.com/gfx-rs/wgpu/pull/4163) #### Vulkan @@ -118,10 +118,6 @@ By @wumpf in [#4147](https://github.com/gfx-rs/wgpu/pull/4147) - Support for timestamp queries on encoders and passes. By @wumpf in [#4008](https://github.com/gfx-rs/wgpu/pull/4008) -#### Metal - -- Support for timestamp queries on encoders and passes. By @wumpf in [#4008](https://github.com/gfx-rs/wgpu/pull/4008) - ### Bug Fixes #### General diff --git a/tests/tests/device.rs b/tests/tests/device.rs index e2d87e3c15..98eb5bba78 100644 --- a/tests/tests/device.rs +++ b/tests/tests/device.rs @@ -2,6 +2,7 @@ use wasm_bindgen_test::*; use wgpu_test::{fail, initialize_test, FailureCase, TestParameters}; + #[test] #[wasm_bindgen_test] fn device_initialization() { @@ -93,28 +94,26 @@ async fn request_device_error_message() { } #[test] -fn device_lose_then_more() { - // This is a test of device behavior after "lose the device". Specifically, all operations - // should trigger errors. To test this, it calls an explicit function to lose the device, - // which is not part of normal use and ideally wouldn't be exposed at all. - // - // TODO: Figure out how to make device.lose a private function only available to the test - // harness. +fn device_destroy_then_more() { + // This is a test of device behavior after device.destroy. Specifically, all operations + // should trigger errors since the device is lost. // // On DX12 this test fails with a validation error in the very artifical actions taken // after lose the device. The error is "ID3D12CommandAllocator::Reset: The command // allocator cannot be reset because a command list is currently being recorded with the // allocator." That may indicate that DX12 doesn't like opened command buffers staying // open even after they return an error. For now, this test is skipped on DX12. + // + // The DX12 issue may be related to https://github.com/gfx-rs/wgpu/issues/3193. initialize_test( TestParameters::default() .features(wgpu::Features::CLEAR_TEXTURE) .skip(FailureCase::backend(wgpu::Backends::DX12)), |ctx| { - // Create some resources on the device that we will attempt to use *after* losing the - // device. + // Create some resources on the device that we will attempt to use *after* losing + // the device. - // Create a 512 x 512 2D texture, and a target view of it. + // Create some 512 x 512 2D textures. let texture_extent = wgpu::Extent3d { width: 512, height: 512, @@ -132,6 +131,17 @@ fn device_lose_then_more() { }); let target_view = texture_for_view.create_view(&wgpu::TextureViewDescriptor::default()); + let texture_for_read = ctx.device.create_texture(&wgpu::TextureDescriptor { + label: None, + size: texture_extent, + mip_level_count: 2, + sample_count: 1, + dimension: wgpu::TextureDimension::D2, + format: wgpu::TextureFormat::Rg8Uint, + usage: wgpu::TextureUsages::COPY_SRC, + view_formats: &[], + }); + let texture_for_write = ctx.device.create_texture(&wgpu::TextureDescriptor { label: None, size: texture_extent, @@ -157,6 +167,18 @@ fn device_lose_then_more() { mapped_at_creation: false, }); + // Create a bind group layout. + let bind_group_layout = ctx.device.create_bind_group_layout(&wgpu::BindGroupLayoutDescriptor { + label: None, + entries: &[], + }); + + // Create a shader module. + let shader_module = ctx.device.create_shader_module(wgpu::ShaderModuleDescriptor { + label: None, + source: wgpu::ShaderSource::Wgsl(std::borrow::Cow::Borrowed("")), + }); + // Create some command encoders. let mut encoder_for_clear = ctx .device @@ -178,13 +200,55 @@ fn device_lose_then_more() { .device .create_command_encoder(&wgpu::CommandEncoderDescriptor::default()); - // Lose the device. This will cause all other requests to return some variation of a - // device invalid error. - ctx.device.lose(); + let mut encoder_for_texture_buffer_copy = ctx + .device + .create_command_encoder(&wgpu::CommandEncoderDescriptor::default()); + + let mut encoder_for_texture_texture_copy = ctx + .device + .create_command_encoder(&wgpu::CommandEncoderDescriptor::default()); + + // Destroy the device. This will cause all other requests to return some variation of + // a device invalid error. + ctx.device.destroy(); + + // TODO: verify the following operations will return an invalid device error: + // * Run a compute pass + // * Run a render pass + // * Finish a render bundle encoder + // * Create a texture from HAL + // * Create a buffer from HAL + // * Create a sampler + // * Validate a surface configuration + // * Start capture + // * Stop capture + // * Buffer map + + // TODO: figure out how to structure a test around these operations which panic when + // the device is invalid: + // * device.features() + // * device.limits() + // * device.downlevel_properties() + // * device.create_query_set() // TODO: change these fail calls to check for the specific errors which indicate that // the device is not valid. + // Creating a commmand encoder should fail. + fail(&ctx.device, || { + ctx.device.create_command_encoder(&wgpu::CommandEncoderDescriptor::default()); + }); + + // Creating a buffer should fail. + fail(&ctx.device, || { + ctx.device.create_buffer(&wgpu::BufferDescriptor { + label: None, + size: 256, + usage: wgpu::BufferUsages::MAP_WRITE | wgpu::BufferUsages::COPY_SRC, + mapped_at_creation: false, + }); + }); + // Creating a texture should fail. fail(&ctx.device, || { ctx.device.create_texture(&wgpu::TextureDescriptor { @@ -266,6 +330,114 @@ fn device_lose_then_more() { texture_extent, ); }); + + // Copying a texture to a buffer should fail. + fail(&ctx.device, || { + encoder_for_texture_buffer_copy.copy_texture_to_buffer( + texture_for_read.as_image_copy(), + wgpu::ImageCopyBuffer { + buffer: &buffer_source, + layout: wgpu::ImageDataLayout { + offset: 0, + bytes_per_row: Some(4), + rows_per_image: None, + }, + }, + texture_extent, + ); + }); + + // Copying a texture to a texture should fail. + fail(&ctx.device, || { + encoder_for_texture_texture_copy.copy_texture_to_texture( + texture_for_read.as_image_copy(), + texture_for_write.as_image_copy(), + texture_extent, + ); + }); + + // Creating a bind group layout should fail. + fail(&ctx.device, || { + ctx + .device + .create_bind_group_layout(&wgpu::BindGroupLayoutDescriptor { + label: None, + entries: &[], + }); + }); + + // Creating a bind group should fail. + fail(&ctx.device, || { + ctx.device.create_bind_group(&wgpu::BindGroupDescriptor { + label: None, + layout: &bind_group_layout, + entries: &[wgpu::BindGroupEntry { + binding: 0, + resource: wgpu::BindingResource::Buffer(buffer_source.as_entire_buffer_binding()), + }], + }); + }); + + // Creating a pipeline layout should fail. + fail(&ctx.device, || { + ctx + .device + .create_pipeline_layout(&wgpu::PipelineLayoutDescriptor { + label: None, + bind_group_layouts: &[], + push_constant_ranges: &[], + }); + }); + + // Creating a shader module should fail. + fail(&ctx.device, || { + ctx.device.create_shader_module(wgpu::ShaderModuleDescriptor { + label: None, + source: wgpu::ShaderSource::Wgsl(std::borrow::Cow::Borrowed("")), + }); + }); + + // Creating a shader module spirv should fail. + fail(&ctx.device, || { + unsafe { + ctx.device.create_shader_module_spirv(&wgpu::ShaderModuleDescriptorSpirV { + label: None, + source: std::borrow::Cow::Borrowed(&[]), + }); + } + }); + + // Creating a render pipeline should fail. + fail(&ctx.device, || { + ctx + .device + .create_render_pipeline(&wgpu::RenderPipelineDescriptor { + label: None, + layout: None, + vertex: wgpu::VertexState { + module: &shader_module, + entry_point: "", + buffers: &[], + }, + primitive: wgpu::PrimitiveState::default(), + depth_stencil: None, + multisample: wgpu::MultisampleState::default(), + fragment: None, + multiview: None, + }); + }); + + // Creating a compute pipeline should fail. + fail(&ctx.device, || { + ctx + .device + .create_compute_pipeline(&wgpu::ComputePipelineDescriptor{ + label: None, + layout: None, + module: &shader_module, + entry_point: "", + }); + }); }, ) } diff --git a/wgpu-core/src/device/global.rs b/wgpu-core/src/device/global.rs index c7003c61ce..eee1f6e054 100644 --- a/wgpu-core/src/device/global.rs +++ b/wgpu-core/src/device/global.rs @@ -2510,13 +2510,43 @@ impl Global { } } - pub fn device_lose(&self, device_id: DeviceId) { + pub fn device_destroy(&self, device_id: DeviceId) { let hub = A::hub(self); let mut token = Token::root(); let (mut device_guard, _) = hub.devices.write(&mut token); if let Ok(device) = device_guard.get_mut(device_id) { - device.lose(); + // Follow the steps at + // https://gpuweb.github.io/gpuweb/#dom-gpudevice-destroy. + + // It's legal to call destroy multiple times, but if the device + // is already invalid, there's nothing more to do. There's also + // no need to return an error. + if !device.valid { + return; + } + + // The last part of destroy is to lose the device. The spec says + // delay that until all "currently-enqueued operations on any + // queue on this device are completed." + + // TODO: implement this delay. + + // Finish by losing the device. + + // TODO: associate this "destroyed" reason more tightly with + // the GPUDeviceLostReason defined in webgpu.idl. + device.lose(Some("destroyed")); + } + } + + pub fn device_lose(&self, device_id: DeviceId, reason: Option<&str>) { + let hub = A::hub(self); + let mut token = Token::root(); + + let (mut device_guard, _) = hub.devices.write(&mut token); + if let Ok(device) = device_guard.get_mut(device_id) { + device.lose(reason); } } diff --git a/wgpu-core/src/device/resource.rs b/wgpu-core/src/device/resource.rs index bd974112d5..8f37f68be9 100644 --- a/wgpu-core/src/device/resource.rs +++ b/wgpu-core/src/device/resource.rs @@ -3174,12 +3174,25 @@ impl Device { }) } - pub(crate) fn lose(&mut self) { + pub(crate) fn lose(&mut self, _reason: Option<&str>) { + // Follow the steps at https://gpuweb.github.io/gpuweb/#lose-the-device. + // Mark the device explicitly as invalid. This is checked in various - // places to prevent new work from being submitted, allowing a later - // call to `poll_devices` to drop this device when the work queues - // are cleared. + // places to prevent new work from being submitted. self.valid = false; + + // The following steps remain in "lose the device": + // 1) Resolve the GPUDevice device.lost promise. + + // TODO: triggger this passively or actively, and supply the reason. + + // 2) Complete any outstanding mapAsync() steps. + // 3) Complete any outstanding onSubmittedWorkDone() steps. + + // These parts are passively accomplished by setting valid to false, + // since that will prevent any new work from being added to the queues. + // Future calls to poll_devices will continue to check the work queues + // until they are cleared, and then drop the device. } } diff --git a/wgpu/src/backend/direct.rs b/wgpu/src/backend/direct.rs index 5961168326..e8776f881a 100644 --- a/wgpu/src/backend/direct.rs +++ b/wgpu/src/backend/direct.rs @@ -1439,10 +1439,14 @@ impl crate::Context for Context { wgc::gfx_select!(device => global.device_drop(*device)); } + fn device_destroy(&self, device: &Self::DeviceId, _device_data: &Self::DeviceData) { + let global = &self.0; + wgc::gfx_select!(device => global.device_destroy(*device)); + } fn device_lose(&self, device: &Self::DeviceId, _device_data: &Self::DeviceData) { + // TODO: accept a reason, and pass it to device_lose. let global = &self.0; - wgc::gfx_select!(device => global.device_lose(*device)); - // TODO: resolve the device.lost promise. + wgc::gfx_select!(device => global.device_lose(*device, None)); } fn device_poll( &self, diff --git a/wgpu/src/backend/web.rs b/wgpu/src/backend/web.rs index 387d03d507..3868a01fcd 100644 --- a/wgpu/src/backend/web.rs +++ b/wgpu/src/backend/web.rs @@ -1914,9 +1914,14 @@ impl crate::context::Context for Context { // Device is dropped automatically } + fn device_destroy(&self, _buffer: &Self::DeviceId, device_data: &Self::DeviceData) { + device_data.0.destroy(); + } + fn device_lose(&self, _device: &Self::DeviceId, _device_data: &Self::DeviceData) { // TODO: figure out the GPUDevice implementation of this, including resolving - // the device.lost promise. + // the device.lost promise, which will require a different invocation pattern + // with a callback. } fn device_poll( diff --git a/wgpu/src/context.rs b/wgpu/src/context.rs index a44a36840a..bfcd4ae2ec 100644 --- a/wgpu/src/context.rs +++ b/wgpu/src/context.rs @@ -269,6 +269,7 @@ pub trait Context: Debug + WasmNotSend + WasmNotSync + Sized { desc: &RenderBundleEncoderDescriptor, ) -> (Self::RenderBundleEncoderId, Self::RenderBundleEncoderData); fn device_drop(&self, device: &Self::DeviceId, device_data: &Self::DeviceData); + fn device_destroy(&self, device: &Self::DeviceId, device_data: &Self::DeviceData); fn device_lose(&self, device: &Self::DeviceId, device_data: &Self::DeviceData); fn device_poll( &self, @@ -1364,6 +1365,7 @@ pub(crate) trait DynContext: Debug + WasmNotSend + WasmNotSync { desc: &RenderBundleEncoderDescriptor, ) -> (ObjectId, Box); fn device_drop(&self, device: &ObjectId, device_data: &crate::Data); + fn device_destroy(&self, device: &ObjectId, device_data: &crate::Data); fn device_lose(&self, device: &ObjectId, device_data: &crate::Data); fn device_poll(&self, device: &ObjectId, device_data: &crate::Data, maintain: Maintain) -> bool; @@ -2426,6 +2428,12 @@ where Context::device_drop(self, &device, device_data) } + fn device_destroy(&self, device: &ObjectId, device_data: &crate::Data) { + let device = ::from(*device); + let device_data = downcast_ref(device_data); + Context::device_destroy(self, &device, device_data) + } + fn device_lose(&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 6f897d7c64..c2090c7f4f 100644 --- a/wgpu/src/lib.rs +++ b/wgpu/src/lib.rs @@ -2765,10 +2765,9 @@ impl Device { } } - /// Trigger the "lose the device" steps, which makes the Device invalid - /// and prevents further operations from succeeding. - pub fn lose(&self) { - self.context.device_lose(&self.id, self.data.as_ref()) + /// Destroy this device. + pub fn destroy(&self) { + DynContext::device_destroy(&*self.context, &self.id, self.data.as_ref()) } }