From 3e7682757d83594b176fe6d5e487be92439e029b Mon Sep 17 00:00:00 2001 From: Dzmitry Malyshau Date: Wed, 8 Apr 2020 17:28:10 -0400 Subject: [PATCH 1/3] Refactor tracking of swapchain images as attachments We were improperly detecting if a swapchain image has already been used by a command buffer. In this case, we need to assume that it's already in the PRESENT state. --- wgpu-core/src/command/render.rs | 36 ++++++++++++++------------------- 1 file changed, 15 insertions(+), 21 deletions(-) diff --git a/wgpu-core/src/command/render.rs b/wgpu-core/src/command/render.rs index 41e82c7809..bb210f1007 100644 --- a/wgpu-core/src/command/render.rs +++ b/wgpu-core/src/command/render.rs @@ -29,9 +29,7 @@ use wgt::{ TextureUsage, BIND_BUFFER_ALIGNMENT, }; -use std::{ - borrow::Borrow, collections::hash_map::Entry, iter, marker::PhantomData, mem, ops::Range, slice, -}; +use std::{borrow::Borrow, collections::hash_map::Entry, iter, mem, ops::Range, slice}; pub type RenderPassColorAttachmentDescriptor = RenderPassColorAttachmentDescriptorBase; @@ -438,7 +436,10 @@ impl Global { let mut resolves = ArrayVec::new(); for at in &color_attachments { - let view = &view_guard[at.attachment]; + let view = trackers + .views + .use_extend(&*view_guard, at.attachment, (), ()) + .unwrap(); if let Some(ex) = extent { assert_eq!(ex, view.extent); } else { @@ -448,10 +449,6 @@ impl Global { view.samples, sample_count, "All attachments must have the same sample_count" ); - let first_use = trackers - .views - .init(at.attachment, view.life_guard.add_ref(), PhantomData) - .is_ok(); let layouts = match view.inner { TextureViewInner::Native { ref source_id, .. } => { @@ -477,10 +474,9 @@ impl Global { } let end = hal::image::Layout::Present; - let start = if first_use { - hal::image::Layout::Undefined - } else { - end + let start = match base_trackers.views.query(at.attachment, ()) { + Some(_) => end, + None => hal::image::Layout::Undefined, }; start..end } @@ -496,16 +492,15 @@ impl Global { } for resolve_target in color_attachments.iter().flat_map(|at| at.resolve_target) { - let view = &view_guard[resolve_target]; + let view = trackers + .views + .use_extend(&*view_guard, resolve_target, (), ()) + .unwrap(); assert_eq!(extent, Some(view.extent)); assert_eq!( view.samples, 1, "All resolve_targets must have a sample_count of 1" ); - let first_use = trackers - .views - .init(resolve_target, view.life_guard.add_ref(), PhantomData) - .is_ok(); let layouts = match view.inner { TextureViewInner::Native { ref source_id, .. } => { @@ -531,10 +526,9 @@ impl Global { } let end = hal::image::Layout::Present; - let start = if first_use { - hal::image::Layout::Undefined - } else { - end + let start = match base_trackers.views.query(resolve_target, ()) { + Some(_) => end, + None => hal::image::Layout::Undefined, }; start..end } From ee6ef901e5a41f71b3f8f16b9f1fab8499bbeba6 Mon Sep 17 00:00:00 2001 From: Dzmitry Malyshau Date: Thu, 9 Apr 2020 23:25:16 -0400 Subject: [PATCH 2/3] Fix tracking of the initial state during replacement. When multiple "replace" style transitions are happening, we weren't properly retaining the "first" state, which is required for proper stitching of command buffers. This logic is fixed and fortified with a new set of "change" and "merge" tests in the track module. --- wgpu-core/src/command/render.rs | 5 +- wgpu-core/src/device/mod.rs | 5 +- wgpu-core/src/id.rs | 5 +- wgpu-core/src/track/buffer.rs | 113 ++++++++++++++----- wgpu-core/src/track/mod.rs | 2 +- wgpu-core/src/track/range.rs | 2 +- wgpu-core/src/track/texture.rs | 188 +++++++++++++++++++++++++++++--- 7 files changed, 274 insertions(+), 46 deletions(-) diff --git a/wgpu-core/src/command/render.rs b/wgpu-core/src/command/render.rs index bb210f1007..3bd16fd86b 100644 --- a/wgpu-core/src/command/render.rs +++ b/wgpu-core/src/command/render.rs @@ -1152,6 +1152,10 @@ impl Global { } } + log::trace!("Merging {:?} with the render pass", encoder_id); + unsafe { + raw.end_render_pass(); + } super::CommandBuffer::insert_barriers( cmb.raw.last_mut().unwrap(), &mut cmb.trackers, @@ -1161,7 +1165,6 @@ impl Global { ); unsafe { cmb.raw.last_mut().unwrap().finish(); - raw.end_render_pass(); } cmb.raw.push(raw); } diff --git a/wgpu-core/src/device/mod.rs b/wgpu-core/src/device/mod.rs index 2091b521b4..2888a2310f 100644 --- a/wgpu-core/src/device/mod.rs +++ b/wgpu-core/src/device/mod.rs @@ -1503,6 +1503,8 @@ impl Global { // execute resource transitions let mut transit = device.com_allocator.extend(comb); unsafe { + // the last buffer was open, closing now + comb.raw.last_mut().unwrap().finish(); transit.begin_primary(hal::command::CommandBufferFlags::ONE_TIME_SUBMIT); } log::trace!("Stitching command buffer {:?} before submission", cmb_id); @@ -1517,9 +1519,6 @@ impl Global { transit.finish(); } comb.raw.insert(0, transit); - unsafe { - comb.raw.last_mut().unwrap().finish(); - } } log::debug!("Device after submission {}: {:#?}", submit_index, trackers); diff --git a/wgpu-core/src/id.rs b/wgpu-core/src/id.rs index 897829e56d..a2bae01dc7 100644 --- a/wgpu-core/src/id.rs +++ b/wgpu-core/src/id.rs @@ -23,7 +23,10 @@ pub struct Id(NonZeroU64, PhantomData); // required for PeekPoke impl Default for Id { fn default() -> Self { - Id(unsafe { NonZeroU64::new_unchecked(!0) }, PhantomData) + Id( + unsafe { NonZeroU64::new_unchecked(!0 >> BACKEND_BITS) }, + PhantomData, + ) } } diff --git a/wgpu-core/src/track/buffer.rs b/wgpu-core/src/track/buffer.rs index fb882c3c68..fbeb3d640e 100644 --- a/wgpu-core/src/track/buffer.rs +++ b/wgpu-core/src/track/buffer.rs @@ -55,22 +55,28 @@ impl ResourceState for BufferState { output: Option<&mut Vec>>, ) -> Result<(), PendingTransition> { let old = self.last; - if usage != old || !BufferUsage::ORDERED.contains(usage) { + if old != usage || !BufferUsage::ORDERED.contains(usage) { let pending = PendingTransition { id, selector: (), usage: old..usage, }; - self.last = match output { - None => pending.collapse()?, + *self = match output { + None => { + assert_eq!( + self.first, None, + "extending a state that is already a transition" + ); + Unit::new(pending.collapse()?) + } Some(transitions) => { transitions.push(pending); - if self.first.is_none() { - self.first = Some(old); + Unit { + first: self.first.or(Some(old)), + last: usage, } - usage } - } + }; } Ok(()) } @@ -83,28 +89,33 @@ impl ResourceState for BufferState { ) -> Result<(), PendingTransition> { let old = self.last; let new = other.port(); - self.last = if old == new && BufferUsage::ORDERED.contains(new) { - if self.first.is_none() { + if old == new && BufferUsage::ORDERED.contains(new) { + if output.is_some() && self.first.is_none() { self.first = Some(old); } - other.last } else { let pending = PendingTransition { id, selector: (), usage: old..new, }; - match output { - None => pending.collapse()?, + *self = match output { + None => { + assert_eq!( + self.first, None, + "extending a state that is already a transition" + ); + Unit::new(pending.collapse()?) + } Some(transitions) => { transitions.push(pending); - if self.first.is_none() { - self.first = Some(old); + Unit { + first: self.first.or(Some(old)), + last: other.last, } - other.last } - } - }; + }; + } Ok(()) } @@ -114,19 +125,71 @@ impl ResourceState for BufferState { #[cfg(test)] mod test { use super::*; - use crate::id::TypedId; + use crate::id::Id; #[test] - fn change() { + fn change_extend() { let mut bs = Unit { - first: Some(BufferUsage::INDEX), + first: None, + last: BufferUsage::INDEX, + }; + let id = Id::default(); + assert_eq!( + bs.change(id, (), BufferUsage::STORAGE, None), + Err(PendingTransition { + id, + selector: (), + usage: BufferUsage::INDEX..BufferUsage::STORAGE, + }), + ); + bs.change(id, (), BufferUsage::VERTEX, None).unwrap(); + bs.change(id, (), BufferUsage::INDEX, None).unwrap(); + assert_eq!(bs, Unit::new(BufferUsage::VERTEX | BufferUsage::INDEX)); + } + + #[test] + fn change_replace() { + let mut bs = Unit { + first: None, last: BufferUsage::STORAGE, }; - let id = TypedId::zip(1, 0, wgt::Backend::Empty); - assert!(bs.change(id, (), BufferUsage::VERTEX, None).is_err()); - bs.change(id, (), BufferUsage::VERTEX, Some(&mut Vec::new())) + let id = Id::default(); + let mut list = Vec::new(); + bs.change(id, (), BufferUsage::VERTEX, Some(&mut list)) .unwrap(); - bs.change(id, (), BufferUsage::INDEX, None).unwrap(); - assert_eq!(bs.last, BufferUsage::VERTEX | BufferUsage::INDEX); + assert_eq!( + &list, + &[PendingTransition { + id, + selector: (), + usage: BufferUsage::STORAGE..BufferUsage::VERTEX, + }], + ); + assert_eq!( + bs, + Unit { + first: Some(BufferUsage::STORAGE), + last: BufferUsage::VERTEX, + } + ); + + list.clear(); + bs.change(id, (), BufferUsage::STORAGE, Some(&mut list)) + .unwrap(); + assert_eq!( + &list, + &[PendingTransition { + id, + selector: (), + usage: BufferUsage::VERTEX..BufferUsage::STORAGE, + }], + ); + assert_eq!( + bs, + Unit { + first: Some(BufferUsage::STORAGE), + last: BufferUsage::STORAGE, + } + ); } } diff --git a/wgpu-core/src/track/mod.rs b/wgpu-core/src/track/mod.rs index 1faefeb4c7..576a3d1f12 100644 --- a/wgpu-core/src/track/mod.rs +++ b/wgpu-core/src/track/mod.rs @@ -111,7 +111,7 @@ struct Resource { /// A structure containing all the information about a particular resource /// transition. User code should be able to generate a pipeline barrier /// based on the contents. -#[derive(Debug)] +#[derive(Debug, PartialEq)] pub struct PendingTransition { pub id: S::Id, pub selector: S::Selector, diff --git a/wgpu-core/src/track/range.rs b/wgpu-core/src/track/range.rs index 0aa9a3602a..13376d97b0 100644 --- a/wgpu-core/src/track/range.rs +++ b/wgpu-core/src/track/range.rs @@ -9,7 +9,7 @@ use std::{cmp::Ordering, fmt::Debug, iter, ops::Range, slice::Iter}; /// Structure that keeps track of a I -> T mapping, /// optimized for a case where keys of the same values /// are often grouped together linearly. -#[derive(Clone, Debug)] +#[derive(Clone, Debug, PartialEq)] pub struct RangedStates { /// List of ranges, each associated with a singe value. /// Ranges of keys have to be non-intersecting and ordered. diff --git a/wgpu-core/src/track/texture.rs b/wgpu-core/src/track/texture.rs index d5010a8291..17ec6ac49f 100644 --- a/wgpu-core/src/track/texture.rs +++ b/wgpu-core/src/track/texture.rs @@ -13,7 +13,7 @@ use std::{iter, ops::Range}; //TODO: store `hal::image::State` here to avoid extra conversions type PlaneStates = RangedStates>; -#[derive(Clone, Debug, Default)] +#[derive(Clone, Debug, Default, PartialEq)] pub struct TextureState { mips: ArrayVec<[PlaneStates; MAX_MIP_LEVELS]>, /// True if we have the information about all the subresources here @@ -116,14 +116,20 @@ impl ResourceState for TextureState { usage: unit.last..usage, }; - unit.last = match output { - None => pending.collapse()?, + *unit = match output { + None => { + assert_eq!( + unit.first, None, + "extending a state that is already a transition" + ); + Unit::new(pending.collapse()?) + } Some(ref mut out) => { out.push(pending); - if unit.first.is_none() { - unit.first = Some(unit.last); + Unit { + first: unit.first.or(Some(unit.last)), + last: usage, } - usage } }; } @@ -172,7 +178,10 @@ impl ResourceState for TextureState { let to_usage = end.port(); if start.last == to_usage && TextureUsage::ORDERED.contains(to_usage) { Unit { - first: start.first, + first: match output { + None => start.first, + Some(_) => start.first.or(Some(start.last)), + }, last: end.last, } } else { @@ -191,14 +200,18 @@ impl ResourceState for TextureState { }; match output { - None => Unit { - first: start.first, - last: pending.collapse()?, - }, + None => { + assert_eq!( + start.first, None, + "extending a state that is already a transition" + ); + Unit::new(pending.collapse()?) + } Some(ref mut out) => { out.push(pending); Unit { - first: Some(start.last), + // this has to leave a valid `first` state + first: start.first.or(Some(start.last)), last: end.last, } } @@ -222,9 +235,9 @@ impl ResourceState for TextureState { #[cfg(test)] mod test { - //TODO: change() and merge() tests - //use crate::TypedId; + //TODO: change() tests use super::*; + use crate::id::Id; use hal::{format::Aspects, image::SubresourceRange}; #[test] @@ -274,4 +287,151 @@ mod test { None, ); } + + #[test] + fn merge() { + let id = Id::default(); + let mut ts1 = TextureState::default(); + ts1.mips.push(PlaneStates::from_slice(&[( + 1..3, + Unit::new(TextureUsage::SAMPLED), + )])); + let mut ts2 = TextureState::default(); + assert_eq!( + ts1.merge(id, &ts2, None), + Ok(()), + "failed to merge with an emtpy" + ); + + ts2.mips.push(PlaneStates::from_slice(&[( + 1..2, + Unit::new(TextureUsage::COPY_SRC), + )])); + assert_eq!( + ts1.merge(Id::default(), &ts2, None), + Ok(()), + "failed to extend a compatible state" + ); + assert_eq!( + ts1.mips[0].query(&(1..2), |&v| v), + Some(Ok(Unit { + first: None, + last: TextureUsage::SAMPLED | TextureUsage::COPY_SRC, + })), + "wrong extension result" + ); + + ts2.mips[0] = PlaneStates::from_slice(&[(1..2, Unit::new(TextureUsage::COPY_DST))]); + assert_eq!( + ts1.clone().merge(Id::default(), &ts2, None), + Err(PendingTransition { + id, + selector: SubresourceRange { + aspects: Aspects::empty(), + levels: 0..1, + layers: 1..2, + }, + usage: TextureUsage::SAMPLED | TextureUsage::COPY_SRC..TextureUsage::COPY_DST, + }), + "wrong error on extending with incompatible state" + ); + + let mut list = Vec::new(); + ts2.mips[0] = PlaneStates::from_slice(&[ + (1..2, Unit::new(TextureUsage::COPY_DST)), + ( + 2..3, + Unit { + first: Some(TextureUsage::COPY_SRC), + last: TextureUsage::OUTPUT_ATTACHMENT, + }, + ), + ]); + ts1.merge(Id::default(), &ts2, Some(&mut list)).unwrap(); + assert_eq!( + &list, + &[ + PendingTransition { + id, + selector: SubresourceRange { + aspects: Aspects::empty(), + levels: 0..1, + layers: 1..2, + }, + usage: TextureUsage::SAMPLED | TextureUsage::COPY_SRC..TextureUsage::COPY_DST, + }, + PendingTransition { + id, + selector: SubresourceRange { + aspects: Aspects::empty(), + levels: 0..1, + layers: 2..3, + }, + // the transition links the end of the base rage (..SAMPLED) + // with the start of the next range (COPY_SRC..) + usage: TextureUsage::SAMPLED..TextureUsage::COPY_SRC, + }, + ], + "replacing produced wrong transitions" + ); + assert_eq!( + ts1.mips[0].query(&(1..2), |&v| v), + Some(Ok(Unit { + first: Some(TextureUsage::SAMPLED | TextureUsage::COPY_SRC), + last: TextureUsage::COPY_DST, + })), + "wrong final layer 1 state" + ); + assert_eq!( + ts1.mips[0].query(&(2..3), |&v| v), + Some(Ok(Unit { + first: Some(TextureUsage::SAMPLED), + last: TextureUsage::OUTPUT_ATTACHMENT, + })), + "wrong final layer 2 state" + ); + + list.clear(); + ts2.mips[0] = PlaneStates::from_slice(&[( + 2..3, + Unit { + first: Some(TextureUsage::OUTPUT_ATTACHMENT), + last: TextureUsage::COPY_SRC, + }, + )]); + ts1.merge(Id::default(), &ts2, Some(&mut list)).unwrap(); + assert_eq!(&list, &[], "unexpected replacing transition"); + + list.clear(); + ts2.mips[0] = PlaneStates::from_slice(&[( + 2..3, + Unit { + first: Some(TextureUsage::COPY_DST), + last: TextureUsage::COPY_DST, + }, + )]); + ts1.merge(Id::default(), &ts2, Some(&mut list)).unwrap(); + assert_eq!( + &list, + &[PendingTransition { + id, + selector: SubresourceRange { + aspects: Aspects::empty(), + levels: 0..1, + layers: 2..3, + }, + usage: TextureUsage::COPY_SRC..TextureUsage::COPY_DST, + },], + "invalid replacing transition" + ); + assert_eq!( + ts1.mips[0].query(&(2..3), |&v| v), + Some(Ok(Unit { + // the initial state here is never expected to change + first: Some(TextureUsage::SAMPLED), + last: TextureUsage::COPY_DST, + })), + "wrong final layer 2 state" + ); + } } From 1303d4907c790a5057438ad4a1eab5983c40fa57 Mon Sep 17 00:00:00 2001 From: Dzmitry Malyshau Date: Fri, 10 Apr 2020 00:43:21 -0400 Subject: [PATCH 3/3] Version bump to 0.5.1 --- CHANGELOG.md | 4 ++++ Cargo.lock | 6 +++--- wgpu-core/Cargo.toml | 2 +- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b6871a07de..00d5d5c8be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Change Log +## v0.5.1 (10-04-2020) + - fix tracking of swapchain images that are used multiple times in a command buffer + - fix tracking of initial usage of a resource across a command buffer + ## v0.5 (06-04-2020) - Crates: - `wgpu-types`: common types between native and web targets diff --git a/Cargo.lock b/Cargo.lock index d55279d6f8..7e76dac1a9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -653,7 +653,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" [[package]] name = "wgpu-core" -version = "0.5.0" +version = "0.5.1" dependencies = [ "arrayvec 0.5.1 (registry+https://github.com/rust-lang/crates.io-index)", "battery 0.7.5 (registry+https://github.com/rust-lang/crates.io-index)", @@ -687,7 +687,7 @@ dependencies = [ "objc 0.2.7 (registry+https://github.com/rust-lang/crates.io-index)", "parking_lot 0.10.0 (registry+https://github.com/rust-lang/crates.io-index)", "raw-window-handle 0.3.3 (registry+https://github.com/rust-lang/crates.io-index)", - "wgpu-core 0.5.0", + "wgpu-core 0.5.1", "wgpu-types 0.5.0", ] @@ -697,7 +697,7 @@ version = "0.1.0" dependencies = [ "log 0.4.8 (registry+https://github.com/rust-lang/crates.io-index)", "parking_lot 0.10.0 (registry+https://github.com/rust-lang/crates.io-index)", - "wgpu-core 0.5.0", + "wgpu-core 0.5.1", "wgpu-types 0.5.0", ] diff --git a/wgpu-core/Cargo.toml b/wgpu-core/Cargo.toml index 0b1185bbd5..839d752f32 100644 --- a/wgpu-core/Cargo.toml +++ b/wgpu-core/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "wgpu-core" -version = "0.5.0" +version = "0.5.1" authors = [ "Dzmitry Malyshau ", "Joshua Groves ",