github/wgpu
1
1
Fork 0
mirror of https://github.com/gfx-rs/wgpu.git synced 2026-04-22 03:02:01 -04:00
Code Issues Packages Projects Releases Wiki Activity

Merge #1704

Browse Source 
1704: Fix buffer memory initialization to use the tracker consistently r=kvark a=kvark

**Connections**
Depends on https://github.com/gfx-rs/naga/pull/1125

**Description**
Contains a number of dx12 fixes in addition to the buffer initialization fix.
The problem there was - we used the "pending writes" command buffer to initialize buffers, but also used the device's resource tracker and its state to figure out the transitions *after* a command buffer is merged its states.

**Testing**
Ran the examples on Vulkan.

Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
This commit is contained in:
bors[bot]
2021-07-23 04:48:42 +00:00
committed by GitHub
parent a32310096e 09c9ab5e75
commit 5ef620ace8
3 changed files with 80 additions and 106 deletions
Download Patch File Download Diff File Expand all files Collapse all files

77
wgpu-core/src/command/mod.rs
View File

@@ -17,7 +17,7 @@ pub use self::transfer::*;
use crate::{
hub::{Global, GlobalIdentityHandlerFactory, HalApi, Storage, Token},
id,
memory_init_tracker::MemoryInitTrackerAction,
memory_init_tracker::{MemoryInitKind, MemoryInitTrackerAction},
resource::{Buffer, Texture},
track::{BufferState, ResourceTracker, TextureState, TrackerSet},
Label, Stored,
@@ -63,10 +63,76 @@ impl<A: hal::Api> CommandEncoder<A> {
}
}
pub struct BackedCommands<A: hal::Api> {
pub struct BakedCommands<A: hal::Api> {
pub(crate) encoder: A::CommandEncoder,
pub(crate) list: Vec<A::CommandBuffer>,
pub(crate) trackers: TrackerSet,
buffer_memory_init_actions: Vec<MemoryInitTrackerAction<id::BufferId>>,
}
pub(crate) struct DestroyedBufferError(pub id::BufferId);
impl<A: hal::Api> BakedCommands<A> {
pub(crate) fn initialize_buffer_memory(
&mut self,
device_tracker: &mut TrackerSet,
buffer_guard: &mut Storage<Buffer<A>, id::BufferId>,
) -> Result<(), DestroyedBufferError> {
let mut ranges = Vec::new();
for buffer_use in self.buffer_memory_init_actions.drain(..) {
let buffer = buffer_guard
.get_mut(buffer_use.id)
.map_err(|_| DestroyedBufferError(buffer_use.id))?;
let uninitialized_ranges = buffer.initialization_status.drain(buffer_use.range.clone());
match buffer_use.kind {
MemoryInitKind::ImplicitlyInitialized => {
uninitialized_ranges.for_each(drop);
}
MemoryInitKind::NeedsInitializedMemory => {
ranges.clear();
ranges.extend(uninitialized_ranges);
// Collapse touching ranges. We can't do this any earlier since we only now gathered ranges from several different command buffers!
ranges.sort_by(|a, b| a.start.cmp(&b.start));
for i in (1..ranges.len()).rev() {
assert!(ranges[i - 1].end <= ranges[i].start); // The memory init tracker made sure of this!
if ranges[i].start == ranges[i - 1].end {
ranges[i - 1].end = ranges[i].end;
ranges.swap_remove(i); // Ordering not important at this point
}
}
// Don't do use_replace since the buffer may already no longer have a ref_count.
// However, we *know* that it is currently in use, so the tracker must already know about it.
let transition = device_tracker.buffers.change_replace_tracked(
id::Valid(buffer_use.id),
(),
hal::BufferUses::COPY_DST,
);
let raw_buf = buffer
.raw
.as_ref()
.ok_or(DestroyedBufferError(buffer_use.id))?;
unsafe {
self.encoder
.transition_buffers(transition.map(|pending| pending.into_hal(buffer)));
}
for range in ranges.iter() {
assert!(range.start % 4 == 0, "Buffer {:?} has an uninitialized range with a start not aligned to 4 (start was {})", raw_buf, range.start);
assert!(range.end % 4 == 0, "Buffer {:?} has an uninitialized range with an end not aligned to 4 (end was {})", raw_buf, range.end);
unsafe {
self.encoder.fill_buffer(raw_buf, range.clone(), 0);
}
}
}
}
}
Ok(())
}
}
pub struct CommandBuffer<A: hal::Api> {
@@ -75,7 +141,7 @@ pub struct CommandBuffer<A: hal::Api> {
pub(crate) device_id: Stored<id::DeviceId>,
pub(crate) trackers: TrackerSet,
pub(crate) used_swap_chains: SmallVec<[Stored<id::SwapChainId>; 1]>,
pub(crate) buffer_memory_init_actions: Vec<MemoryInitTrackerAction<id::BufferId>>,
buffer_memory_init_actions: Vec<MemoryInitTrackerAction<id::BufferId>>,
limits: wgt::Limits,
support_fill_buffer_texture: bool,
#[cfg(feature = "trace")]
@@ -164,11 +230,12 @@ impl<A: hal::Api> CommandBuffer<A> {
}
}
pub(crate) fn into_baked(self) -> BackedCommands<A> {
BackedCommands {
pub(crate) fn into_baked(self) -> BakedCommands<A> {
BakedCommands {
encoder: self.encoder.raw,
list: self.encoder.list,
trackers: self.trackers,
buffer_memory_init_actions: self.buffer_memory_init_actions,
}
}
}

107
wgpu-core/src/device/queue.rs
View File

@@ -7,17 +7,16 @@ use crate::{
},
conv,
device::{DeviceError, WaitIdleError},
hub::{Global, GlobalIdentityHandlerFactory, HalApi, Storage, Token},
hub::{Global, GlobalIdentityHandlerFactory, HalApi, Token},
id,
memory_init_tracker::{MemoryInitKind, MemoryInitTrackerAction},
resource::{Buffer, BufferAccessError, BufferMapState},
FastHashMap, FastHashSet,
resource::{BufferAccessError, BufferMapState},
FastHashSet,
};
use hal::{CommandEncoder as _, Device as _, Queue as _};
use parking_lot::Mutex;
use smallvec::SmallVec;
use std::{iter, mem, num::NonZeroU32, ops::Range, ptr};
use std::{iter, mem, num::NonZeroU32, ptr};
use thiserror::Error;
/// Number of command buffers that we generate from the same pool
@@ -177,39 +176,6 @@ impl<A: hal::Api> PendingWrites<A> {
}
}
#[derive(Default)]
struct RequiredBufferInits {
map: FastHashMap<id::BufferId, Vec<Range<wgt::BufferAddress>>>,
}
impl RequiredBufferInits {
fn add<A: hal::Api>(
&mut self,
buffer_memory_init_actions: &[MemoryInitTrackerAction<id::BufferId>],
buffer_guard: &mut Storage<Buffer<A>, id::BufferId>,
) -> Result<(), QueueSubmitError> {
for buffer_use in buffer_memory_init_actions.iter() {
let buffer = buffer_guard
.get_mut(buffer_use.id)
.map_err(|_| QueueSubmitError::DestroyedBuffer(buffer_use.id))?;
let uninitialized_ranges = buffer.initialization_status.drain(buffer_use.range.clone());
match buffer_use.kind {
MemoryInitKind::ImplicitlyInitialized => {
uninitialized_ranges.for_each(drop);
}
MemoryInitKind::NeedsInitializedMemory => {
self.map
.entry(buffer_use.id)
.or_default()
.extend(uninitialized_ranges);
}
}
}
Ok(())
}
}
impl<A: hal::Api> super::Device<A> {
fn prepare_stage(&mut self, size: wgt::BufferAddress) -> Result<StagingData<A>, DeviceError> {
profiling::scope!("prepare_stage");
@@ -222,58 +188,6 @@ impl<A: hal::Api> super::Device<A> {
let buffer = unsafe { self.raw.create_buffer(&stage_desc)? };
Ok(StagingData { buffer })
}
fn initialize_buffer_memory(
&mut self,
mut required_buffer_inits: RequiredBufferInits,
buffer_guard: &mut Storage<Buffer<A>, id::BufferId>,
) -> Result<(), QueueSubmitError> {
self.pending_writes
.dst_buffers
.extend(required_buffer_inits.map.keys());
let encoder = self.pending_writes.activate();
let mut trackers = self.trackers.lock();
for (buffer_id, mut ranges) in required_buffer_inits.map.drain() {
// Collapse touching ranges. We can't do this any earlier since we only now gathered ranges from several different command buffers!
ranges.sort_by(|a, b| a.start.cmp(&b.start));
for i in (1..ranges.len()).rev() {
assert!(ranges[i - 1].end <= ranges[i].start); // The memory init tracker made sure of this!
if ranges[i].start == ranges[i - 1].end {
ranges[i - 1].end = ranges[i].end;
ranges.swap_remove(i); // Ordering not important at this point
}
}
// Don't do use_replace since the buffer may already no longer have a ref_count.
// However, we *know* that it is currently in use, so the tracker must already know about it.
let transition = trackers.buffers.change_replace_tracked(
id::Valid(buffer_id),
(),
hal::BufferUses::COPY_DST,
);
let buffer = buffer_guard.get(buffer_id).unwrap();
let raw_buf = buffer
.raw
.as_ref()
.ok_or(QueueSubmitError::DestroyedBuffer(buffer_id))?;
unsafe {
encoder.transition_buffers(transition.map(|pending| pending.into_hal(buffer)));
}
for range in ranges {
assert!(range.start % 4 == 0, "Buffer {:?} has an uninitialized range with a start not aligned to 4 (start was {})", raw_buf, range.start);
assert!(range.end % 4 == 0, "Buffer {:?} has an uninitialized range with an end not aligned to 4 (end was {})", raw_buf, range.end);
unsafe {
encoder.fill_buffer(raw_buf, range, 0);
}
}
}
Ok(())
}
}
#[derive(Clone, Debug, Error)]
@@ -624,7 +538,6 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
let (sampler_guard, mut token) = hub.samplers.read(&mut token);
let (query_set_guard, _) = hub.query_sets.read(&mut token);
let mut required_buffer_inits = RequiredBufferInits::default();
//Note: locking the trackers has to be done after the storages
let mut trackers = device.trackers.lock();
@@ -653,8 +566,6 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
continue;
}
required_buffer_inits
.add(&cmdbuf.buffer_memory_init_actions, &mut *buffer_guard)?;
// optimize the tracked states
cmdbuf.trackers.optimize();
@@ -756,7 +667,6 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
}
let mut baked = cmdbuf.into_baked();
// execute resource transitions
unsafe {
baked
@@ -765,6 +675,9 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
.map_err(DeviceError::from)?
};
log::trace!("Stitching command buffer {:?} before submission", cmb_id);
baked
.initialize_buffer_memory(&mut *trackers, &mut *buffer_guard)
.map_err(|err| QueueSubmitError::DestroyedBuffer(err.0))?;
//Note: stateless trackers are not merged:
// device already knows these resources exist.
CommandBuffer::insert_barriers(
@@ -775,6 +688,7 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
&*buffer_guard,
&*texture_guard,
);
let transit = unsafe { baked.encoder.end_encoding().unwrap() };
baked.list.insert(0, transit);
active_executions.push(EncoderInFlight {
@@ -784,11 +698,6 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
}
log::trace!("Device after submission {}: {:#?}", submit_index, trackers);
drop(trackers);
if !required_buffer_inits.map.is_empty() {
device
.initialize_buffer_memory(required_buffer_inits, &mut *buffer_guard)?;
}
}
let super::Device {

2
wgpu/src/backend/direct.rs
View File

@@ -1085,7 +1085,6 @@ impl crate::Context for Context {
if let wgc::pipeline::CreateRenderPipelineError::Internal { stage, ref error } = cause {
log::warn!("Shader translation error for stage {:?}: {}", stage, error);
log::warn!("Please report it to https://github.com/gfx-rs/naga");
log::warn!("Try enabling `wgpu/cross` feature as a workaround.");
}
self.handle_error(
&device.error_sink,
@@ -1136,7 +1135,6 @@ impl crate::Context for Context {
error
);
log::warn!("Please report it to https://github.com/gfx-rs/naga");
log::warn!("Try enabling `wgpu/cross` feature as a workaround.");
}
self.handle_error(
&device.error_sink,