From 9c3967158ae099736ec6c3cbf7b134385e1724aa Mon Sep 17 00:00:00 2001 From: Kunal Mohan Date: Sat, 22 Aug 2020 13:52:27 +0530 Subject: [PATCH] defer encoding errors to finish() --- components/script/dom/gpucommandencoder.rs | 94 +++---------------- .../script/dom/gpucomputepassencoder.rs | 3 +- components/script/dom/gpudevice.rs | 9 -- components/script/dom/gpurenderpassencoder.rs | 3 +- components/script/dom/gpuswapchain.rs | 1 - components/webgpu/lib.rs | 60 +++++++----- servo-tidy.toml | 3 + 7 files changed, 54 insertions(+), 119 deletions(-) diff --git a/components/script/dom/gpucommandencoder.rs b/components/script/dom/gpucommandencoder.rs index 7b3a55d3aa8..60eabbf1973 100644 --- a/components/script/dom/gpucommandencoder.rs +++ b/components/script/dom/gpucommandencoder.rs @@ -29,7 +29,7 @@ use std::borrow::Cow; use std::cell::Cell; use std::collections::HashSet; use webgpu::wgpu::command as wgpu_com; -use webgpu::{self, identity::WebGPUOpResult, wgt, WebGPU, WebGPURequest}; +use webgpu::{self, wgt, WebGPU, WebGPURequest}; // https://gpuweb.github.io/gpuweb/#enumdef-encoder-state #[derive(MallocSizeOf, PartialEq)] @@ -58,7 +58,6 @@ impl GPUCommandEncoder { channel: WebGPU, device: &GPUDevice, encoder: webgpu::WebGPUCommandEncoder, - valid: bool, label: Option, ) -> Self { Self { @@ -69,7 +68,7 @@ impl GPUCommandEncoder { encoder, buffers: DomRefCell::new(HashSet::new()), state: DomRefCell::new(GPUCommandEncoderState::Open), - valid: Cell::new(valid), + valid: Cell::new(true), } } @@ -78,12 +77,11 @@ impl GPUCommandEncoder { channel: WebGPU, device: &GPUDevice, encoder: webgpu::WebGPUCommandEncoder, - valid: bool, label: Option, ) -> DomRoot { reflect_dom_object( Box::new(GPUCommandEncoder::new_inherited( - channel, device, encoder, valid, label, + channel, device, encoder, label, )), global, ) @@ -103,10 +101,6 @@ impl GPUCommandEncoder { *self.state.borrow_mut() = GPUCommandEncoderState::Closed; } } - - pub fn device(&self) -> &GPUDevice { - &*self.device - } } impl GPUCommandEncoderMethods for GPUCommandEncoder { @@ -125,26 +119,16 @@ impl GPUCommandEncoderMethods for GPUCommandEncoder { &self, descriptor: &GPUComputePassDescriptor, ) -> DomRoot { - let scope_id = self.device.use_current_scope(); self.set_state( GPUCommandEncoderState::EncodingComputePass, GPUCommandEncoderState::Open, ); - let (compute_pass, res) = if !self.valid.get() { - ( - None, - WebGPUOpResult::ValidationError(String::from( - "CommandEncoder is not in Open State", - )), - ) - } else { - ( - Some(wgpu_com::ComputePass::new(self.encoder.0)), - WebGPUOpResult::Success, - ) - }; - self.device.handle_server_msg(scope_id, res); + let compute_pass = if !self.valid.get() { + None + } else { + Some(wgpu_com::ComputePass::new(self.encoder.0)) + }; GPUComputePassEncoder::new( &self.global(), @@ -160,19 +144,13 @@ impl GPUCommandEncoderMethods for GPUCommandEncoder { &self, descriptor: &GPURenderPassDescriptor, ) -> DomRoot { - let scope_id = self.device.use_current_scope(); self.set_state( GPUCommandEncoderState::EncodingRenderPass, GPUCommandEncoderState::Open, ); - let (render_pass, res) = if !self.valid.get() { - ( - None, - WebGPUOpResult::ValidationError(String::from( - "CommandEncoder is not in Open State", - )), - ) + let render_pass = if !self.valid.get() { + None } else { let depth_stencil = descriptor.depthStencilAttachment.as_ref().map(|depth| { let (depth_load_op, clear_depth) = match depth.depthLoadValue { @@ -265,14 +243,9 @@ impl GPUCommandEncoderMethods for GPUCommandEncoder { ), depth_stencil_attachment: depth_stencil.as_ref(), }; - ( - Some(wgpu_com::RenderPass::new(self.encoder.0, desc)), - WebGPUOpResult::Success, - ) + Some(wgpu_com::RenderPass::new(self.encoder.0, desc)) }; - self.device.handle_server_msg(scope_id, res); - GPURenderPassEncoder::new( &self.global(), self.channel.clone(), @@ -291,16 +264,7 @@ impl GPUCommandEncoderMethods for GPUCommandEncoder { destination_offset: GPUSize64, size: GPUSize64, ) { - let scope_id = self.device.use_current_scope(); - println!("CopyBufferToBuffer scope_id {:?}", scope_id); - if !(*self.state.borrow() == GPUCommandEncoderState::Open) { - self.device.handle_server_msg( - scope_id, - WebGPUOpResult::ValidationError(String::from( - "CommandEncoder is not in Open State", - )), - ); self.valid.set(false); return; } @@ -312,10 +276,9 @@ impl GPUCommandEncoderMethods for GPUCommandEncoder { self.channel .0 .send(( - scope_id, + None, WebGPURequest::CopyBufferToBuffer { command_encoder_id: self.encoder.0, - device_id: self.device.id().0, source_id: source.id().0, source_offset, destination_id: destination.id().0, @@ -333,15 +296,7 @@ impl GPUCommandEncoderMethods for GPUCommandEncoder { destination: &GPUTextureCopyView, copy_size: GPUExtent3D, ) { - let scope_id = self.device.use_current_scope(); - if !(*self.state.borrow() == GPUCommandEncoderState::Open) { - self.device.handle_server_msg( - scope_id, - WebGPUOpResult::ValidationError(String::from( - "CommandEncoder is not in Open State", - )), - ); self.valid.set(false); return; } @@ -353,10 +308,9 @@ impl GPUCommandEncoderMethods for GPUCommandEncoder { self.channel .0 .send(( - scope_id, + None, WebGPURequest::CopyBufferToTexture { command_encoder_id: self.encoder.0, - device_id: self.device.id().0, source: convert_buffer_cv(source), destination: convert_texture_cv(destination), copy_size: convert_texture_size_to_wgt(&convert_texture_size_to_dict( @@ -374,15 +328,7 @@ impl GPUCommandEncoderMethods for GPUCommandEncoder { destination: &GPUBufferCopyView, copy_size: GPUExtent3D, ) { - let scope_id = self.device.use_current_scope(); - if !(*self.state.borrow() == GPUCommandEncoderState::Open) { - self.device.handle_server_msg( - scope_id, - WebGPUOpResult::ValidationError(String::from( - "CommandEncoder is not in Open State", - )), - ); self.valid.set(false); return; } @@ -394,10 +340,9 @@ impl GPUCommandEncoderMethods for GPUCommandEncoder { self.channel .0 .send(( - scope_id, + None, WebGPURequest::CopyTextureToBuffer { command_encoder_id: self.encoder.0, - device_id: self.device.id().0, source: convert_texture_cv(source), destination: convert_buffer_cv(destination), copy_size: convert_texture_size_to_wgt(&convert_texture_size_to_dict( @@ -415,15 +360,7 @@ impl GPUCommandEncoderMethods for GPUCommandEncoder { destination: &GPUTextureCopyView, copy_size: GPUExtent3D, ) { - let scope_id = self.device.use_current_scope(); - if !(*self.state.borrow() == GPUCommandEncoderState::Open) { - self.device.handle_server_msg( - scope_id, - WebGPUOpResult::ValidationError(String::from( - "CommandEncoder is not in Open State", - )), - ); self.valid.set(false); return; } @@ -431,10 +368,9 @@ impl GPUCommandEncoderMethods for GPUCommandEncoder { self.channel .0 .send(( - scope_id, + None, WebGPURequest::CopyTextureToTexture { command_encoder_id: self.encoder.0, - device_id: self.device.id().0, source: convert_texture_cv(source), destination: convert_texture_cv(destination), copy_size: convert_texture_size_to_wgt(&convert_texture_size_to_dict( diff --git a/components/script/dom/gpucomputepassencoder.rs b/components/script/dom/gpucomputepassencoder.rs index a73ee8bde34..f0f4325ae6d 100644 --- a/components/script/dom/gpucomputepassencoder.rs +++ b/components/script/dom/gpucomputepassencoder.rs @@ -99,10 +99,9 @@ impl GPUComputePassEncoderMethods for GPUComputePassEncoder { self.channel .0 .send(( - self.command_encoder.device().use_current_scope(), + None, WebGPURequest::RunComputePass { command_encoder_id: self.command_encoder.id().0, - device_id: self.command_encoder.device().id().0, compute_pass, }, )) diff --git a/components/script/dom/gpudevice.rs b/components/script/dom/gpudevice.rs index c8c8f9ff3b2..e1347d6daf5 100644 --- a/components/script/dom/gpudevice.rs +++ b/components/script/dom/gpudevice.rs @@ -178,7 +178,6 @@ impl GPUDevice { } pub fn handle_server_msg(&self, scope: Option, result: WebGPUOpResult) { - println!("handle_server_msg {:?}, {:?}", scope, result); let result = match result { WebGPUOpResult::Success => Ok(()), WebGPUOpResult::ValidationError(m) => { @@ -222,7 +221,6 @@ impl GPUDevice { } fn handle_error(&self, scope: ErrorScopeId, error: GPUError) { - println!("handle_error {}", scope); let mut context = self.scope_context.borrow_mut(); if let Some(mut err_scope) = context.error_scopes.get_mut(&scope) { if err_scope.error.is_none() { @@ -234,7 +232,6 @@ impl GPUDevice { } fn try_remove_scope(&self, scope: ErrorScopeId) { - println!("try_remove_scope {}", scope); let mut context = self.scope_context.borrow_mut(); let remove = if let Some(mut err_scope) = context.error_scopes.get_mut(&scope) { err_scope.op_count -= 1; @@ -253,7 +250,6 @@ impl GPUDevice { false }; if remove { - println!("remove {}", scope); let _ = context.error_scopes.remove(&scope); context.scope_stack.retain(|meta| meta.id != scope); } @@ -374,7 +370,6 @@ impl GPUDeviceMethods for GPUDevice { .create_buffer_id(self.device.0.backend()); let scope_id = self.use_current_scope(); - println!("CreateBuffer scope {:?}", scope_id); if desc.is_none() { self.handle_server_msg( scope_id, @@ -780,7 +775,6 @@ impl GPUDeviceMethods for GPUDevice { self.channel.clone(), &self, encoder, - true, descriptor.parent.label.as_ref().cloned(), ) } @@ -1093,7 +1087,6 @@ impl GPUDeviceMethods for GPUDevice { fn PushErrorScope(&self, filter: GPUErrorFilter) { let mut context = self.scope_context.borrow_mut(); let scope_id = context.next_scope_id; - println!("PushErrorScope {}", scope_id); context.next_scope_id = ErrorScopeId::new(scope_id.get() + 1).unwrap(); let err_scope = ErrorScopeInfo { op_count: 0, @@ -1121,7 +1114,6 @@ impl GPUDeviceMethods for GPUDevice { promise.reject_error(Error::Operation); return promise; }; - println!("PopErrorScope {}", scope_id); let remove = if let Some(mut err_scope) = context.error_scopes.get_mut(&scope_id) { if let Some(ref e) = err_scope.error { promise.resolve_native(e); @@ -1135,7 +1127,6 @@ impl GPUDeviceMethods for GPUDevice { false }; if remove { - println!("PopErrorScope remove {}", scope_id); let _ = context.error_scopes.remove(&scope_id); context.scope_stack.retain(|meta| meta.id != scope_id); } diff --git a/components/script/dom/gpurenderpassencoder.rs b/components/script/dom/gpurenderpassencoder.rs index 9d1abf12511..ac7424b1999 100644 --- a/components/script/dom/gpurenderpassencoder.rs +++ b/components/script/dom/gpurenderpassencoder.rs @@ -164,10 +164,9 @@ impl GPURenderPassEncoderMethods for GPURenderPassEncoder { self.channel .0 .send(( - self.command_encoder.device().use_current_scope(), + None, WebGPURequest::RunRenderPass { command_encoder_id: self.command_encoder.id().0, - device_id: self.command_encoder.device().id().0, render_pass, }, )) diff --git a/components/script/dom/gpuswapchain.rs b/components/script/dom/gpuswapchain.rs index 0552eea7564..a60449ae616 100644 --- a/components/script/dom/gpuswapchain.rs +++ b/components/script/dom/gpuswapchain.rs @@ -90,7 +90,6 @@ impl GPUSwapChainMethods for GPUSwapChain { /// https://gpuweb.github.io/gpuweb/#dom-gpuswapchain-getcurrenttexture fn GetCurrentTexture(&self) -> DomRoot { self.context.mark_as_dirty(); - //self.context.send_swap_chain_present(); DomRoot::from_ref(&*self.texture) } } diff --git a/components/webgpu/lib.rs b/components/webgpu/lib.rs index bb5d39a6b7a..f76840de396 100644 --- a/components/webgpu/lib.rs +++ b/components/webgpu/lib.rs @@ -20,7 +20,9 @@ use serde::{Deserialize, Serialize}; use servo_config::pref; use smallvec::SmallVec; use std::borrow::Cow; -use std::collections::{HashMap, HashSet}; +use std::cell::RefCell; +use std::collections::hash_map::Entry; +use std::collections::HashMap; use std::num::NonZeroU64; use std::rc::Rc; use std::slice; @@ -87,7 +89,6 @@ pub enum WebGPURequest { }, CopyBufferToBuffer { command_encoder_id: id::CommandEncoderId, - device_id: id::DeviceId, source_id: id::BufferId, source_offset: wgt::BufferAddress, destination_id: id::BufferId, @@ -96,21 +97,18 @@ pub enum WebGPURequest { }, CopyBufferToTexture { command_encoder_id: id::CommandEncoderId, - device_id: id::DeviceId, source: BufferCopyView, destination: TextureCopyView, copy_size: wgt::Extent3d, }, CopyTextureToBuffer { command_encoder_id: id::CommandEncoderId, - device_id: id::DeviceId, source: TextureCopyView, destination: BufferCopyView, copy_size: wgt::Extent3d, }, CopyTextureToTexture { command_encoder_id: id::CommandEncoderId, - device_id: id::DeviceId, source: TextureCopyView, destination: TextureCopyView, copy_size: wgt::Extent3d, @@ -214,12 +212,10 @@ pub enum WebGPURequest { }, RunComputePass { command_encoder_id: id::CommandEncoderId, - device_id: id::DeviceId, compute_pass: Option, }, RunRenderPass { command_encoder_id: id::CommandEncoderId, - device_id: id::DeviceId, render_pass: Option, }, Submit { @@ -346,7 +342,7 @@ struct WGPU<'a> { present_buffer_maps: HashMap, WebGPURequest)>>>, //TODO: Remove this (https://github.com/gfx-rs/wgpu/issues/867) - error_command_buffers: HashSet, + error_command_encoders: RefCell>, webrender_api: webrender_api::RenderApi, webrender_document: webrender_api::DocumentId, external_images: Arc>, @@ -378,7 +374,7 @@ impl<'a> WGPU<'a> { _invalid_adapters: Vec::new(), buffer_maps: HashMap::new(), present_buffer_maps: HashMap::new(), - error_command_buffers: HashSet::new(), + error_command_encoders: RefCell::new(HashMap::new()), webrender_api: webrender_api_sender.create_api(), webrender_document, external_images, @@ -466,6 +462,12 @@ impl<'a> WGPU<'a> { let global = &self.global; let result = if is_error { Err(String::from("Invalid GPUCommandEncoder")) + } else if let Some(err) = self + .error_command_encoders + .borrow() + .get(&command_encoder_id) + { + Err(err.clone()) } else { gfx_select!(command_encoder_id => global.command_encoder_finish( command_encoder_id, @@ -474,13 +476,12 @@ impl<'a> WGPU<'a> { .map_err(|e| format!("{:?}", e)) }; if result.is_err() { - self.error_command_buffers.insert(command_encoder_id); + self.encoder_record_error(command_encoder_id, result.clone()); } self.send_result(device_id, scope_id, result); }, WebGPURequest::CopyBufferToBuffer { command_encoder_id, - device_id, source_id, source_offset, destination_id, @@ -496,12 +497,10 @@ impl<'a> WGPU<'a> { destination_offset, size )); - println!("CopyBufferToBuffer result {:?}", result); - self.send_result(device_id, scope_id, result); + self.encoder_record_error(command_encoder_id, result); }, WebGPURequest::CopyBufferToTexture { command_encoder_id, - device_id, source, destination, copy_size, @@ -513,11 +512,10 @@ impl<'a> WGPU<'a> { &destination, ©_size )); - self.send_result(device_id, scope_id, result); + self.encoder_record_error(command_encoder_id, result); }, WebGPURequest::CopyTextureToBuffer { command_encoder_id, - device_id, source, destination, copy_size, @@ -529,11 +527,10 @@ impl<'a> WGPU<'a> { &destination, ©_size )); - self.send_result(device_id, scope_id, result); + self.encoder_record_error(command_encoder_id, result); }, WebGPURequest::CopyTextureToTexture { command_encoder_id, - device_id, source, destination, copy_size, @@ -545,7 +542,7 @@ impl<'a> WGPU<'a> { &destination, ©_size )); - self.send_result(device_id, scope_id, result); + self.encoder_record_error(command_encoder_id, result); }, WebGPURequest::CreateBindGroup { device_id, @@ -594,7 +591,6 @@ impl<'a> WGPU<'a> { self.send_result(device_id, scope_id, result); } else { let _ = gfx_select!(buffer_id => global.buffer_error(buffer_id)); - println!("CreateBuffer error {:?}", buffer_id); } }, WebGPURequest::CreateCommandEncoder { @@ -865,7 +861,9 @@ impl<'a> WGPU<'a> { return; }, WebGPURequest::FreeCommandBuffer(command_buffer_id) => { - self.error_command_buffers.remove(&command_buffer_id); + self.error_command_encoders + .borrow_mut() + .remove(&command_buffer_id); }, WebGPURequest::FreeDevice(device_id) => { let device = WebGPUDevice(device_id); @@ -976,7 +974,6 @@ impl<'a> WGPU<'a> { }, WebGPURequest::RunComputePass { command_encoder_id, - device_id, compute_pass, } => { let global = &self.global; @@ -988,11 +985,10 @@ impl<'a> WGPU<'a> { } else { Err(String::from("Invalid ComputePass")) }; - self.send_result(device_id, scope_id, result); + self.encoder_record_error(command_encoder_id, result); }, WebGPURequest::RunRenderPass { command_encoder_id, - device_id, render_pass, } => { let global = &self.global; @@ -1004,7 +1000,7 @@ impl<'a> WGPU<'a> { } else { Err(String::from("Invalid RenderPass")) }; - self.send_result(device_id, scope_id, result); + self.encoder_record_error(command_encoder_id, result); }, WebGPURequest::Submit { queue_id, @@ -1013,7 +1009,7 @@ impl<'a> WGPU<'a> { let global = &self.global; let cmd_id = command_buffers .iter() - .find(|id| self.error_command_buffers.contains(id)); + .find(|id| self.error_command_encoders.borrow().contains_key(id)); let result = if cmd_id.is_some() { Err(String::from("Invalid command buffer submitted")) } else { @@ -1276,6 +1272,18 @@ impl<'a> WGPU<'a> { warn!("Failed to send WebGPUOpResult ({})", w); } } + + fn encoder_record_error( + &self, + encoder_id: id::CommandEncoderId, + result: Result, + ) { + if let Err(e) = result { + if let Entry::Vacant(v) = self.error_command_encoders.borrow_mut().entry(encoder_id) { + v.insert(format!("{:?}", e)); + } + } + } } fn convert_to_pointer(obj: Rc) -> *mut u8 { diff --git a/servo-tidy.toml b/servo-tidy.toml index 0bb3eae35cb..e2eac2e9852 100644 --- a/servo-tidy.toml +++ b/servo-tidy.toml @@ -31,14 +31,17 @@ rand = [ packages = [ "arrayvec", "base64", + "cloudabi", "cocoa", "gleam", "libloading", + "lock_api", "metal", "miniz_oxide", "num-rational", "parking_lot", "parking_lot_core", + "ron", "wayland-sys", # https://github.com/servo/servo/issues/26933