From 7410b700d63473d76004ddcfa2a935003dc006bd Mon Sep 17 00:00:00 2001 From: Dzmitry Malyshau Date: Wed, 16 Jun 2021 23:18:50 -0400 Subject: [PATCH] fix swapchain recycling, copies, device destruction, RODS --- wgpu-core/src/command/mod.rs | 12 ++---------- wgpu-core/src/command/transfer.rs | 4 ++-- wgpu-core/src/device/life.rs | 2 +- wgpu-core/src/device/mod.rs | 26 +++++++++++++++++++++++-- wgpu-core/src/hub.rs | 6 ++++-- wgpu-core/src/swap_chain.rs | 13 +++++++++++-- wgpu-core/src/track/mod.rs | 2 +- wgpu-hal/examples/halmark/main.rs | 2 +- wgpu-hal/src/empty.rs | 2 +- wgpu-hal/src/lib.rs | 3 ++- wgpu-hal/src/metal/adapter.rs | 2 -- wgpu-hal/src/metal/device.rs | 2 ++ wgpu-hal/src/vulkan/adapter.rs | 6 ------ wgpu-hal/src/vulkan/command.rs | 25 +++++++++++------------- wgpu-hal/src/vulkan/conv.rs | 32 ++++++++++++++++++++++--------- wgpu-hal/src/vulkan/device.rs | 30 +++++++++++++++++++---------- wgpu-hal/src/vulkan/mod.rs | 10 ++++++++-- 17 files changed, 113 insertions(+), 66 deletions(-) diff --git a/wgpu-core/src/command/mod.rs b/wgpu-core/src/command/mod.rs index a7cd8d38b7..19f78acdd6 100644 --- a/wgpu-core/src/command/mod.rs +++ b/wgpu-core/src/command/mod.rs @@ -269,22 +269,14 @@ impl Global { let hub = A::hub(self); let mut token = Token::root(); - let (swap_chain_guard, mut token) = hub.swap_chains.read(&mut token); - //TODO: actually close the last recorded command buffer let (mut cmd_buf_guard, _) = hub.command_buffers.write(&mut token); let error = match CommandBuffer::get_encoder_mut(&mut *cmd_buf_guard, encoder_id) { Ok(cmd_buf) => { cmd_buf.encoder.close(); cmd_buf.status = CommandEncoderStatus::Finished; - // stop tracking the swapchain image, if used - for sc_id in cmd_buf.used_swap_chains.iter() { - let &(ref view_id, _) = swap_chain_guard[sc_id.value] - .acquired_texture - .as_ref() - .expect("Used swap chain frame has already presented"); - cmd_buf.trackers.views.remove(view_id.value); - } + //Note: if we want to stop tracking the swapchain texture view, + // this is the place to do it. log::trace!("Command buffer {:?} {:#?}", encoder_id, cmd_buf.trackers); None } diff --git a/wgpu-core/src/command/transfer.rs b/wgpu-core/src/command/transfer.rs index 89540fbb04..5b13badcf4 100644 --- a/wgpu-core/src/command/transfer.rs +++ b/wgpu-core/src/command/transfer.rs @@ -235,7 +235,7 @@ pub(crate) fn validate_linear_texture_data( } /// Function copied with minor modifications from webgpu standard -/// Returns the mip level extent. +/// Returns the (virtual) mip level extent. pub(crate) fn validate_texture_copy_range( texture_copy_view: &ImageCopyTexture, desc: &wgt::TextureDescriptor<()>, @@ -299,7 +299,7 @@ pub(crate) fn validate_texture_copy_range( return Err(TransferError::UnalignedCopyHeight); } - Ok(extent) + Ok(extent_virtual) } impl Global { diff --git a/wgpu-core/src/device/life.rs b/wgpu-core/src/device/life.rs index 8566aa76bc..1f85487160 100644 --- a/wgpu-core/src/device/life.rs +++ b/wgpu-core/src/device/life.rs @@ -380,7 +380,7 @@ impl LifetimeTracker { resource::TextureViewSource::Native(source_id) => { self.suspected_resources.textures.push(source_id.value); } - resource::TextureViewSource::SwapChain { .. } => unreachable!(), + resource::TextureViewSource::SwapChain { .. } => {} }; let submit_index = res.life_guard.submission_index.load(Ordering::Acquire); diff --git a/wgpu-core/src/device/mod.rs b/wgpu-core/src/device/mod.rs index f3eecdd11f..35a2119fb2 100644 --- a/wgpu-core/src/device/mod.rs +++ b/wgpu-core/src/device/mod.rs @@ -202,6 +202,15 @@ impl CommandAllocator { fn release_encoder(&mut self, encoder: A::CommandEncoder) { self.free_encoders.push(encoder); } + + fn dispose(self, device: &A::Device) { + log::info!("Destroying {} command encoders", self.free_encoders.len()); + for cmd_encoder in self.free_encoders { + unsafe { + device.destroy_command_encoder(cmd_encoder); + } + } + } } /// Structure describing a logical device. Some members are internally mutable, @@ -329,6 +338,17 @@ impl Device { Self::lock_life_internal(&self.life_tracker, token) } + pub(crate) fn suspect_texture_view_for_destruction<'this, 'token: 'this>( + &'this self, + view_id: id::Valid, + token: &mut Token<'token, Self>, + ) { + self.lock_life(token) + .suspected_resources + .texture_views + .push(view_id); + } + fn maintain<'this, 'token: 'this, G: GlobalIdentityHandlerFactory>( &'this self, hub: &Hub, @@ -2299,10 +2319,12 @@ impl Device { } pub(crate) fn dispose(self) { + self.pending_writes.dispose(&self.raw); + self.command_allocator.into_inner().dispose(&self.raw); unsafe { self.raw.destroy_fence(self.fence); + self.raw.exit(); } - self.pending_writes.dispose(&self.raw); } } @@ -4239,7 +4261,7 @@ impl Global { // Adapter is only referenced by the device and itself. // This isn't a robust way to destroy them, we should find a better one. if device.adapter_id.ref_count.load() == 1 { - let (_adapter, _) = hub + let _ = hub .adapters .unregister(device.adapter_id.value.0, &mut token); } diff --git a/wgpu-core/src/hub.rs b/wgpu-core/src/hub.rs index bb44cf2a80..17dd39e746 100644 --- a/wgpu-core/src/hub.rs +++ b/wgpu-core/src/hub.rs @@ -254,7 +254,7 @@ impl Access> for PipelineLayout {} impl Access> for CommandBuffer {} impl Access> for Root {} impl Access> for Device {} -impl Access> for SwapChain {} +impl Access> for SwapChain {} //TODO: remove this (only used in `submit()`) impl Access for Device {} impl Access for CommandBuffer {} impl Access> for Device {} @@ -281,7 +281,7 @@ impl Access> for Root {} impl Access> for Device {} impl Access> for Buffer {} impl Access> for Root {} -impl Access> for SwapChain {} +impl Access> for SwapChain {} //TODO: remove this (only used in `get_next_texture()`) impl Access> for Device {} impl Access> for Texture {} impl Access> for Root {} @@ -708,7 +708,9 @@ impl Hub { device.dispose(); } } + if with_adapters { + drop(devices); self.adapters.data.write().map.clear(); } } diff --git a/wgpu-core/src/swap_chain.rs b/wgpu-core/src/swap_chain.rs index 59ab53168c..e70b930d67 100644 --- a/wgpu-core/src/swap_chain.rs +++ b/wgpu-core/src/swap_chain.rs @@ -253,7 +253,7 @@ impl Global { .get_mut(swap_chain_id.to_surface_id()) .map_err(|_| SwapChainError::InvalidSurface)?; let (mut device_guard, mut token) = hub.devices.write(&mut token); - let (mut swap_chain_guard, mut token) = hub.swap_chains.write(&mut token); + let (mut swap_chain_guard, _) = hub.swap_chains.write(&mut token); let sc = swap_chain_guard .get_mut(swap_chain_id) .map_err(|_| SwapChainError::Invalid)?; @@ -269,12 +269,21 @@ impl Global { .acquired_texture .take() .ok_or(SwapChainError::AlreadyAcquired)?; + + drop(swap_chain_guard); + device.suspect_texture_view_for_destruction(view_id.value, &mut token); + + let (mut view_guard, _) = hub.texture_views.write(&mut token); + let view = &mut view_guard[view_id.value]; + let _ = view.life_guard.ref_count.take(); + + /* let (view_maybe, _) = hub.texture_views.unregister(view_id.value.0, &mut token); drop(view_id); // contains the ref count let view = view_maybe.ok_or(SwapChainError::Invalid)?; if view.life_guard.ref_count.unwrap().load() != 1 { return Err(SwapChainError::StillReferenced); - } + }*/ suf_texture }; diff --git a/wgpu-core/src/track/mod.rs b/wgpu-core/src/track/mod.rs index 3fe3825d19..a7038a83cf 100644 --- a/wgpu-core/src/track/mod.rs +++ b/wgpu-core/src/track/mod.rs @@ -230,7 +230,7 @@ impl ResourceTracker { } /// Remove an id from the tracked map. - pub(crate) fn remove(&mut self, id: Valid) -> bool { + pub(crate) fn _remove(&mut self, id: Valid) -> bool { let (index, epoch, backend) = id.0.unzip(); debug_assert_eq!(backend, self.backend); match self.map.remove(&index) { diff --git a/wgpu-hal/examples/halmark/main.rs b/wgpu-hal/examples/halmark/main.rs index 3cd9902130..e75758e6c5 100644 --- a/wgpu-hal/examples/halmark/main.rs +++ b/wgpu-hal/examples/halmark/main.rs @@ -480,7 +480,7 @@ impl Example { .destroy_bind_group_layout(self.global_group_layout); self.device.destroy_pipeline_layout(self.pipeline_layout); - self.adapter.close(self.device); + self.device.exit(); self.instance.destroy_surface(self.surface); } } diff --git a/wgpu-hal/src/empty.rs b/wgpu-hal/src/empty.rs index 4130498da7..960b383057 100644 --- a/wgpu-hal/src/empty.rs +++ b/wgpu-hal/src/empty.rs @@ -77,7 +77,6 @@ impl crate::Adapter for Context { unsafe fn open(&self, features: wgt::Features) -> DeviceResult> { Err(crate::DeviceError::Lost) } - unsafe fn close(&self, device: Context) {} unsafe fn texture_format_capabilities( &self, format: wgt::TextureFormat, @@ -107,6 +106,7 @@ impl crate::Queue for Context { } impl crate::Device for Context { + unsafe fn exit(self) {} unsafe fn create_buffer(&self, desc: &crate::BufferDescriptor) -> DeviceResult { Ok(Resource) } diff --git a/wgpu-hal/src/lib.rs b/wgpu-hal/src/lib.rs index fe266b450c..af530229e0 100644 --- a/wgpu-hal/src/lib.rs +++ b/wgpu-hal/src/lib.rs @@ -180,7 +180,6 @@ pub trait Surface: Send + Sync { pub trait Adapter: Send + Sync { unsafe fn open(&self, features: wgt::Features) -> Result, DeviceError>; - unsafe fn close(&self, device: A::Device); /// Return the set of supported capabilities for a texture format. unsafe fn texture_format_capabilities( @@ -195,6 +194,8 @@ pub trait Adapter: Send + Sync { } pub trait Device: Send + Sync { + /// Exit connection to this logical device. + unsafe fn exit(self); /// Creates a new buffer. /// /// The initial usage is `BufferUse::empty()`. diff --git a/wgpu-hal/src/metal/adapter.rs b/wgpu-hal/src/metal/adapter.rs index 1c77dac0e1..a1c49a3b24 100644 --- a/wgpu-hal/src/metal/adapter.rs +++ b/wgpu-hal/src/metal/adapter.rs @@ -30,8 +30,6 @@ impl crate::Adapter for super::Adapter { }) } - unsafe fn close(&self, _device: super::Device) {} - unsafe fn texture_format_capabilities( &self, format: wgt::TextureFormat, diff --git a/wgpu-hal/src/metal/device.rs b/wgpu-hal/src/metal/device.rs index 9be08cea2a..3a6f80d669 100644 --- a/wgpu-hal/src/metal/device.rs +++ b/wgpu-hal/src/metal/device.rs @@ -132,6 +132,8 @@ impl super::Device { } impl crate::Device for super::Device { + unsafe fn exit(self) {} + unsafe fn create_buffer(&self, desc: &crate::BufferDescriptor) -> DeviceResult { let map_read = desc.usage.contains(crate::BufferUse::MAP_READ); let map_write = desc.usage.contains(crate::BufferUse::MAP_WRITE); diff --git a/wgpu-hal/src/vulkan/adapter.rs b/wgpu-hal/src/vulkan/adapter.rs index 7e1720ca44..8cacfc925c 100644 --- a/wgpu-hal/src/vulkan/adapter.rs +++ b/wgpu-hal/src/vulkan/adapter.rs @@ -774,12 +774,6 @@ impl crate::Adapter for super::Adapter { Ok(crate::OpenDevice { device, queue }) } - unsafe fn close(&self, device: super::Device) { - device.mem_allocator.lock().cleanup(&*device.shared); - device.desc_allocator.lock().cleanup(&*device.shared); - device.shared.free_resources(); - } - unsafe fn texture_format_capabilities( &self, format: wgt::TextureFormat, diff --git a/wgpu-hal/src/vulkan/command.rs b/wgpu-hal/src/vulkan/command.rs index 4551b428c3..a0991600d3 100644 --- a/wgpu-hal/src/vulkan/command.rs +++ b/wgpu-hal/src/vulkan/command.rs @@ -27,10 +27,9 @@ impl super::Texture { conv::map_subresource_layers(&r.texture_base, dim, aspects, layer_count); vk::BufferImageCopy { buffer_offset: r.buffer_layout.offset, - buffer_row_length: r - .buffer_layout - .bytes_per_row - .map_or(0, |bpr| bpr.get() / fi.block_size as u32), + buffer_row_length: r.buffer_layout.bytes_per_row.map_or(0, |bpr| { + fi.block_dimensions.0 as u32 * (bpr.get() / fi.block_size as u32) + }), buffer_image_height: r .buffer_layout .rows_per_image @@ -108,8 +107,9 @@ impl crate::CommandEncoder for super::CommandEncoder { where T: Iterator>, { - let mut src_stages = vk::PipelineStageFlags::empty(); - let mut dst_stages = vk::PipelineStageFlags::empty(); + //Note: this is done so that we never end up with empty stage flags + let mut src_stages = vk::PipelineStageFlags::TOP_OF_PIPE; + let mut dst_stages = vk::PipelineStageFlags::BOTTOM_OF_PIPE; let vk_barriers = &mut self.temp.buffer_barriers; vk_barriers.clear(); @@ -130,9 +130,6 @@ impl crate::CommandEncoder for super::CommandEncoder { } if !vk_barriers.is_empty() { - if src_stages.is_empty() { - src_stages = vk::PipelineStageFlags::TOP_OF_PIPE; - } self.device.raw.cmd_pipeline_barrier( self.active, src_stages, @@ -157,10 +154,10 @@ impl crate::CommandEncoder for super::CommandEncoder { for bar in barriers { let range = conv::map_subresource_range(&bar.range, bar.texture.aspects); let (src_stage, src_access) = conv::map_texture_usage_to_barrier(bar.usage.start); - let src_layout = conv::derive_image_layout(bar.usage.start); + let src_layout = conv::derive_image_layout(bar.usage.start, bar.texture.aspects); src_stages |= src_stage; let (dst_stage, dst_access) = conv::map_texture_usage_to_barrier(bar.usage.end); - let dst_layout = conv::derive_image_layout(bar.usage.end); + let dst_layout = conv::derive_image_layout(bar.usage.end, bar.texture.aspects); dst_stages |= dst_stage; vk_barriers.push( @@ -228,7 +225,7 @@ impl crate::CommandEncoder for super::CommandEncoder { ) where T: Iterator, { - let src_layout = conv::derive_image_layout(src_usage); + let src_layout = conv::derive_image_layout(src_usage, src.aspects); let vk_regions_iter = regions.map(|r| { let (layer_count, extent) = conv::map_extent(r.size, src.dim); @@ -287,7 +284,7 @@ impl crate::CommandEncoder for super::CommandEncoder { ) where T: Iterator, { - let src_layout = conv::derive_image_layout(src_usage); + let src_layout = conv::derive_image_layout(src_usage, src.aspects); let vk_regions_iter = src.map_buffer_copies(regions); inplace_or_alloc_from_iter(vk_regions_iter, |vk_regions| { @@ -734,7 +731,7 @@ impl crate::CommandEncoder for super::CommandEncoder { #[test] fn check_dst_image_layout() { assert_eq!( - conv::derive_image_layout(crate::TextureUse::COPY_DST), + conv::derive_image_layout(crate::TextureUse::COPY_DST, crate::FormatAspect::empty()), DST_IMAGE_LAYOUT ); } diff --git a/wgpu-hal/src/vulkan/conv.rs b/wgpu-hal/src/vulkan/conv.rs index 1dae8cdd09..3ce8f67750 100644 --- a/wgpu-hal/src/vulkan/conv.rs +++ b/wgpu-hal/src/vulkan/conv.rs @@ -116,11 +116,12 @@ impl crate::Attachment<'_, super::Api> { ops: crate::AttachmentOp, caps: &super::PrivateCapabilities, ) -> super::AttachmentKey { + let aspects = self.view.aspects(); super::AttachmentKey { format: caps.map_texture_format(self.view.attachment.view_format), - layout_pre: derive_image_layout(self.boundary_usage.start), - layout_in: derive_image_layout(self.usage), - layout_post: derive_image_layout(self.boundary_usage.end), + layout_pre: derive_image_layout(self.boundary_usage.start, aspects), + layout_in: derive_image_layout(self.usage, aspects), + layout_post: derive_image_layout(self.boundary_usage.end, aspects), ops, } } @@ -152,21 +153,26 @@ impl crate::ColorAttachment<'_, super::Api> { } } -pub fn derive_image_layout(usage: crate::TextureUse) -> vk::ImageLayout { +pub fn derive_image_layout( + usage: crate::TextureUse, + aspects: crate::FormatAspect, +) -> vk::ImageLayout { + //Note: depth textures are always sampled with RODS layout + let is_color = aspects.contains(crate::FormatAspect::COLOR); match usage { crate::TextureUse::UNINITIALIZED => vk::ImageLayout::UNDEFINED, crate::TextureUse::COPY_SRC => vk::ImageLayout::TRANSFER_SRC_OPTIMAL, crate::TextureUse::COPY_DST => vk::ImageLayout::TRANSFER_DST_OPTIMAL, - crate::TextureUse::SAMPLED => vk::ImageLayout::SHADER_READ_ONLY_OPTIMAL, + crate::TextureUse::SAMPLED if is_color => vk::ImageLayout::SHADER_READ_ONLY_OPTIMAL, crate::TextureUse::COLOR_TARGET => vk::ImageLayout::COLOR_ATTACHMENT_OPTIMAL, crate::TextureUse::DEPTH_STENCIL_WRITE => vk::ImageLayout::DEPTH_STENCIL_ATTACHMENT_OPTIMAL, _ => { - if usage.contains(crate::TextureUse::DEPTH_STENCIL_READ) { - vk::ImageLayout::DEPTH_STENCIL_READ_ONLY_OPTIMAL - } else if usage.is_empty() { + if usage.is_empty() { vk::ImageLayout::PRESENT_SRC_KHR - } else { + } else if is_color { vk::ImageLayout::GENERAL + } else { + vk::ImageLayout::DEPTH_STENCIL_READ_ONLY_OPTIMAL } } } @@ -659,6 +665,14 @@ pub fn map_topology(topology: wgt::PrimitiveTopology) -> vk::PrimitiveTopology { } } +pub fn map_polygon_mode(mode: wgt::PolygonMode) -> vk::PolygonMode { + match mode { + wgt::PolygonMode::Fill => vk::PolygonMode::FILL, + wgt::PolygonMode::Line => vk::PolygonMode::LINE, + wgt::PolygonMode::Point => vk::PolygonMode::POINT, + } +} + pub fn map_front_face(front_face: wgt::FrontFace) -> vk::FrontFace { match front_face { wgt::FrontFace::Cw => vk::FrontFace::CLOCKWISE, diff --git a/wgpu-hal/src/vulkan/device.rs b/wgpu-hal/src/vulkan/device.rs index d245ccb1d7..a07f1164dc 100644 --- a/wgpu-hal/src/vulkan/device.rs +++ b/wgpu-hal/src/vulkan/device.rs @@ -219,7 +219,7 @@ impl super::DeviceShared { }) } - pub(super) unsafe fn free_resources(&self) { + unsafe fn free_resources(&self) { for &raw in self.render_passes.lock().values() { self.raw.destroy_render_pass(raw, None); } @@ -518,6 +518,12 @@ impl super::Device { } impl crate::Device for super::Device { + unsafe fn exit(self) { + self.mem_allocator.into_inner().cleanup(&*self.shared); + self.desc_allocator.into_inner().cleanup(&*self.shared); + self.shared.free_resources(); + } + unsafe fn create_buffer( &self, desc: &crate::BufferDescriptor, @@ -834,12 +840,16 @@ impl crate::Device for super::Device { }) } unsafe fn destroy_command_encoder(&self, cmd_encoder: super::CommandEncoder) { - self.shared - .raw - .free_command_buffers(cmd_encoder.raw, &cmd_encoder.free); - self.shared - .raw - .free_command_buffers(cmd_encoder.raw, &cmd_encoder.discarded); + if !cmd_encoder.free.is_empty() { + self.shared + .raw + .free_command_buffers(cmd_encoder.raw, &cmd_encoder.free); + } + if !cmd_encoder.discarded.is_empty() { + self.shared + .raw + .free_command_buffers(cmd_encoder.raw, &cmd_encoder.discarded); + } self.shared.raw.destroy_command_pool(cmd_encoder.raw, None); } @@ -1012,7 +1022,7 @@ impl crate::Device for super::Device { vk::DescriptorType::SAMPLED_IMAGE | vk::DescriptorType::STORAGE_IMAGE => { let index = image_infos.len(); let binding = &desc.textures[index]; - let layout = conv::derive_image_layout(binding.usage); + let layout = conv::derive_image_layout(binding.usage, binding.view.aspects()); let vk_info = vk::DescriptorImageInfo::builder() .image_view(binding.view.raw) .image_layout(layout) @@ -1146,7 +1156,7 @@ impl crate::Device for super::Device { let mut vk_rasterization = vk::PipelineRasterizationStateCreateInfo::builder() .depth_clamp_enable(desc.primitive.clamp_depth) - .polygon_mode(vk::PolygonMode::FILL) + .polygon_mode(conv::map_polygon_mode(desc.primitive.polygon_mode)) .front_face(conv::map_front_face(desc.primitive.front_face)) .line_width(1.0); if let Some(face) = desc.primitive.cull_mode { @@ -1163,7 +1173,7 @@ impl crate::Device for super::Device { }; compatible_rp_key.depth_stencil = Some(super::DepthStencilAttachmentKey { base: super::AttachmentKey::compatible(vk_format, vk_layout), - stencil_ops: crate::AttachmentOp::empty(), + stencil_ops: crate::AttachmentOp::all(), }); if ds.is_depth_enabled() { diff --git a/wgpu-hal/src/vulkan/mod.rs b/wgpu-hal/src/vulkan/mod.rs index 7e4fafa058..bbf8902c81 100644 --- a/wgpu-hal/src/vulkan/mod.rs +++ b/wgpu-hal/src/vulkan/mod.rs @@ -130,7 +130,7 @@ struct PrivateCapabilities { texture_d24_s8: bool, } -#[derive(Clone, Eq, Hash, PartialEq)] +#[derive(Clone, Debug, Eq, Hash, PartialEq)] struct AttachmentKey { format: vk::Format, layout_pre: vk::ImageLayout, @@ -147,7 +147,7 @@ impl AttachmentKey { layout_pre: vk::ImageLayout::GENERAL, layout_in, layout_post: vk::ImageLayout::GENERAL, - ops: crate::AttachmentOp::empty(), + ops: crate::AttachmentOp::all(), } } } @@ -255,6 +255,12 @@ pub struct TextureView { render_size: wgt::Extent3d, } +impl TextureView { + fn aspects(&self) -> crate::FormatAspect { + self.attachment.view_format.into() + } +} + #[derive(Debug)] pub struct Sampler { raw: vk::Sampler,