From 920c00931d3917a60badeb499b277a9b4549c685 Mon Sep 17 00:00:00 2001 From: Dzmitry Malyshau Date: Thu, 9 Apr 2020 23:25:16 -0400 Subject: [PATCH] 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 19235ce697..421e351636 100644 --- a/wgpu-core/src/command/render.rs +++ b/wgpu-core/src/command/render.rs @@ -1159,6 +1159,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, @@ -1168,7 +1172,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 1c05127a3a..dae5275868 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" + ); + } }