Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ensure the BufferMapCallback is always called. #2848

Merged
merged 2 commits into from
Jul 8, 2022
Merged

Ensure the BufferMapCallback is always called. #2848

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
75e0a24
Ensure the BufferMapAsyncCallback is always called.
nical Jul 4, 2022
d8565c9
Update the changelog.
nical Jul 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ Bottom level categories:
- Allow running `get_texture_format_features` on unsupported texture formats (returning no flags) by @cwfitzgerald in [#2856](https://github.com/gfx-rs/wgpu/pull/2856)
- Allow multi-sampled textures that are supported by the device but not WebGPU if `TEXTURE_ADAPTER_SPECIFIC_FORMAT_FEATURES` is enabled by @cwfitzgerald in [#2856](https://github.com/gfx-rs/wgpu/pull/2856)
- `get_texture_format_features` only lists the COPY_* usages if the adapter actually supports that usage by @cwfitzgerald in [#2856](https://github.com/gfx-rs/wgpu/pull/2856)
- Improve the validation and error reporting of buffer mappings by @nical in [#2848](https://github.com/gfx-rs/wgpu/pull/2848)

#### DX12
- `DownlevelCapabilities::default()` now returns the `ANISOTROPIC_FILTERING` flag set to true so DX12 lists `ANISOTROPIC_FILTERING` as true again by @cwfitzgerald in [#2851](https://github.com/gfx-rs/wgpu/pull/2851)
Expand Down
221 changes: 149 additions & 72 deletions wgpu-core/src/device/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@ use crate::{
BufferInitTracker, BufferInitTrackerAction, MemoryInitKind, TextureInitRange,
TextureInitTracker, TextureInitTrackerAction,
},
instance, pipeline, present, resource,
instance, pipeline, present,
resource::{self, BufferMapState},
resource::{BufferAccessError, BufferMapAsyncStatus, BufferMapOperation},
track::{BindGroupStates, TextureSelector, Tracker},
validation::{self, check_buffer_usage, check_texture_usage},
FastHashMap, Label, LabelHelpers as _, LifeGuard, MultiRefCount, RefCount, Stored,
Expand Down Expand Up @@ -127,7 +129,7 @@ impl RenderPassContext {
}
}

pub type BufferMapPendingClosure = (resource::BufferMapOperation, resource::BufferMapAsyncStatus);
pub type BufferMapPendingClosure = (BufferMapOperation, BufferMapAsyncStatus);

#[derive(Default)]
pub struct UserClosures {
Expand Down Expand Up @@ -159,7 +161,7 @@ fn map_buffer<A: hal::Api>(
offset: BufferAddress,
size: BufferAddress,
kind: HostMap,
) -> Result<ptr::NonNull<u8>, resource::BufferAccessError> {
) -> Result<ptr::NonNull<u8>, BufferAccessError> {
let mapping = unsafe {
raw.map_buffer(buffer.raw.as_ref().unwrap(), offset..offset + size)
.map_err(DeviceError::from)?
Expand Down Expand Up @@ -3409,7 +3411,7 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
buffer_id: id::BufferId,
offset: BufferAddress,
data: &[u8],
) -> Result<(), resource::BufferAccessError> {
) -> Result<(), BufferAccessError> {
profiling::scope!("Device::set_buffer_sub_data");

let hub = A::hub(self);
Expand All @@ -3422,7 +3424,7 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
.map_err(|_| DeviceError::Invalid)?;
let buffer = buffer_guard
.get_mut(buffer_id)
.map_err(|_| resource::BufferAccessError::Invalid)?;
.map_err(|_| BufferAccessError::Invalid)?;
check_buffer_usage(buffer.usage, wgt::BufferUsages::MAP_WRITE)?;
//assert!(buffer isn't used by the GPU);

Expand Down Expand Up @@ -3466,7 +3468,7 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
buffer_id: id::BufferId,
offset: BufferAddress,
data: &mut [u8],
) -> Result<(), resource::BufferAccessError> {
) -> Result<(), BufferAccessError> {
profiling::scope!("Device::get_buffer_sub_data");

let hub = A::hub(self);
Expand All @@ -3479,7 +3481,7 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
.map_err(|_| DeviceError::Invalid)?;
let buffer = buffer_guard
.get_mut(buffer_id)
.map_err(|_| resource::BufferAccessError::Invalid)?;
.map_err(|_| BufferAccessError::Invalid)?;
check_buffer_usage(buffer.usage, wgt::BufferUsages::MAP_READ)?;
//assert!(buffer isn't used by the GPU);

Expand Down Expand Up @@ -3515,39 +3517,59 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
) -> Result<(), resource::DestroyError> {
profiling::scope!("Buffer::destroy");

let hub = A::hub(self);
let mut token = Token::root();
let map_closure;
// Restrict the locks to this scope.
{
let hub = A::hub(self);
let mut token = Token::root();

//TODO: lock pending writes separately, keep the device read-only
let (mut device_guard, mut token) = hub.devices.write(&mut token);
//TODO: lock pending writes separately, keep the device read-only
let (mut device_guard, mut token) = hub.devices.write(&mut token);

log::info!("Buffer {:?} is destroyed", buffer_id);
let (mut buffer_guard, _) = hub.buffers.write(&mut token);
let buffer = buffer_guard
.get_mut(buffer_id)
.map_err(|_| resource::DestroyError::Invalid)?;
log::info!("Buffer {:?} is destroyed", buffer_id);
let (mut buffer_guard, _) = hub.buffers.write(&mut token);
let buffer = buffer_guard
.get_mut(buffer_id)
.map_err(|_| resource::DestroyError::Invalid)?;

let device = &mut device_guard[buffer.device_id.value];
let device = &mut device_guard[buffer.device_id.value];

#[cfg(feature = "trace")]
if let Some(ref trace) = device.trace {
trace.lock().add(trace::Action::FreeBuffer(buffer_id));
}
map_closure = match &buffer.map_state {
&BufferMapState::Waiting(..) // To get the proper callback behavior.
| &BufferMapState::Init { .. }
| &BufferMapState::Active { .. }
=> {
self.buffer_unmap_inner(buffer_id, buffer, device)
.unwrap_or(None)
}
_ => None,
};

let raw = buffer
.raw
.take()
.ok_or(resource::DestroyError::AlreadyDestroyed)?;
let temp = queue::TempResource::Buffer(raw);
#[cfg(feature = "trace")]
if let Some(ref trace) = device.trace {
trace.lock().add(trace::Action::FreeBuffer(buffer_id));
}

if device.pending_writes.dst_buffers.contains(&buffer_id) {
device.pending_writes.temp_resources.push(temp);
} else {
let last_submit_index = buffer.life_guard.life_count();
drop(buffer_guard);
device
.lock_life(&mut token)
.schedule_resource_destruction(temp, last_submit_index);
let raw = buffer
.raw
.take()
.ok_or(resource::DestroyError::AlreadyDestroyed)?;
let temp = queue::TempResource::Buffer(raw);

if device.pending_writes.dst_buffers.contains(&buffer_id) {
device.pending_writes.temp_resources.push(temp);
} else {
let last_submit_index = buffer.life_guard.life_count();
drop(buffer_guard);
device
.lock_life(&mut token)
.schedule_resource_destruction(temp, last_submit_index);
}
}

// Note: outside the scope where locks are held when calling the callback
if let Some((operation, status)) = map_closure {
operation.callback.call(status);
}

Ok(())
Expand Down Expand Up @@ -5331,8 +5353,50 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
&self,
buffer_id: id::BufferId,
range: Range<BufferAddress>,
op: resource::BufferMapOperation,
) -> Result<(), resource::BufferAccessError> {
op: BufferMapOperation,
) -> Result<(), BufferAccessError> {
// User callbacks must not be called while holding buffer_map_async_inner's locks, so we
// defer the error callback if it needs to be called immediately (typically when running
// into errors).
if let Err((op, err)) = self.buffer_map_async_inner::<A>(buffer_id, range, op) {
let status = match &err {
&BufferAccessError::Device(_) => BufferMapAsyncStatus::ContextLost,
&BufferAccessError::Invalid | &BufferAccessError::Destroyed => {
BufferMapAsyncStatus::Invalid
}
&BufferAccessError::AlreadyMapped => BufferMapAsyncStatus::AlreadyMapped,
&BufferAccessError::MapAlreadyPending => BufferMapAsyncStatus::MapAlreadyPending,
&BufferAccessError::MissingBufferUsage(_) => {
BufferMapAsyncStatus::InvalidUsageFlags
}
&BufferAccessError::UnalignedRange
| &BufferAccessError::UnalignedRangeSize { .. }
| &BufferAccessError::UnalignedOffset { .. } => {
BufferMapAsyncStatus::InvalidAlignment
}
&BufferAccessError::OutOfBoundsUnderrun { .. }
| &BufferAccessError::OutOfBoundsOverrun { .. } => {
BufferMapAsyncStatus::InvalidRange
}
_ => BufferMapAsyncStatus::Error,
};

op.callback.call(status);

return Err(err);
}

Ok(())
}

// Returns the mapping callback in case of error so that the callback can be fired outside
// of the locks that are held in this function.
fn buffer_map_async_inner<A: HalApi>(
&self,
buffer_id: id::BufferId,
range: Range<BufferAddress>,
op: BufferMapOperation,
) -> Result<(), (BufferMapOperation, BufferAccessError)> {
profiling::scope!("Buffer::map_async");

let hub = A::hub(self);
Expand All @@ -5344,23 +5408,32 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
};

if range.start % wgt::MAP_ALIGNMENT != 0 || range.end % wgt::COPY_BUFFER_ALIGNMENT != 0 {
return Err(resource::BufferAccessError::UnalignedRange);
return Err((op, BufferAccessError::UnalignedRange));
}

let (device_id, ref_count) = {
let (mut buffer_guard, _) = hub.buffers.write(&mut token);
let buffer = buffer_guard
.get_mut(buffer_id)
.map_err(|_| resource::BufferAccessError::Invalid)?;
.map_err(|_| BufferAccessError::Invalid);

let buffer = match buffer {
Ok(b) => b,
Err(e) => {
return Err((op, e));
}
};

if let Err(e) = check_buffer_usage(buffer.usage, pub_usage) {
return Err((op, e.into()));
}

check_buffer_usage(buffer.usage, pub_usage)?;
buffer.map_state = match buffer.map_state {
resource::BufferMapState::Init { .. } | resource::BufferMapState::Active { .. } => {
return Err(resource::BufferAccessError::AlreadyMapped);
return Err((op, BufferAccessError::AlreadyMapped));
}
resource::BufferMapState::Waiting(_) => {
op.callback.call_error();
return Ok(());
return Err((op, BufferAccessError::MapAlreadyPending));
}
resource::BufferMapState::Idle => {
resource::BufferMapState::Waiting(resource::BufferPendingMapping {
Expand Down Expand Up @@ -5399,15 +5472,15 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
buffer_id: id::BufferId,
offset: BufferAddress,
size: Option<BufferAddress>,
) -> Result<(*mut u8, u64), resource::BufferAccessError> {
) -> Result<(*mut u8, u64), BufferAccessError> {
profiling::scope!("Buffer::get_mapped_range");

let hub = A::hub(self);
let mut token = Token::root();
let (buffer_guard, _) = hub.buffers.read(&mut token);
let buffer = buffer_guard
.get(buffer_id)
.map_err(|_| resource::BufferAccessError::Invalid)?;
.map_err(|_| BufferAccessError::Invalid)?;

let range_size = if let Some(size) = size {
size
Expand All @@ -5418,17 +5491,17 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
};

if offset % wgt::MAP_ALIGNMENT != 0 {
return Err(resource::BufferAccessError::UnalignedOffset { offset });
return Err(BufferAccessError::UnalignedOffset { offset });
}
if range_size % wgt::COPY_BUFFER_ALIGNMENT != 0 {
return Err(resource::BufferAccessError::UnalignedRangeSize { range_size });
return Err(BufferAccessError::UnalignedRangeSize { range_size });
}

match buffer.map_state {
resource::BufferMapState::Init { ptr, .. } => {
// offset (u64) can not be < 0, so no need to validate the lower bound
if offset + range_size > buffer.size {
return Err(resource::BufferAccessError::OutOfBoundsOverrun {
return Err(BufferAccessError::OutOfBoundsOverrun {
index: offset + range_size - 1,
max: buffer.size,
});
Expand All @@ -5437,41 +5510,31 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
}
resource::BufferMapState::Active { ptr, ref range, .. } => {
if offset < range.start {
return Err(resource::BufferAccessError::OutOfBoundsUnderrun {
return Err(BufferAccessError::OutOfBoundsUnderrun {
jimblandy marked this conversation as resolved.
Show resolved Hide resolved
index: offset,
min: range.start,
});
}
if offset + range_size > range.end {
return Err(resource::BufferAccessError::OutOfBoundsOverrun {
return Err(BufferAccessError::OutOfBoundsOverrun {
index: offset + range_size - 1,
max: range.end,
});
}
unsafe { Ok((ptr.as_ptr().offset(offset as isize), range_size)) }
}
resource::BufferMapState::Idle | resource::BufferMapState::Waiting(_) => {
Err(resource::BufferAccessError::NotMapped)
Err(BufferAccessError::NotMapped)
}
}
}

fn buffer_unmap_inner<A: HalApi>(
&self,
buffer_id: id::BufferId,
) -> Result<Option<BufferMapPendingClosure>, resource::BufferAccessError> {
profiling::scope!("Buffer::unmap");

let hub = A::hub(self);
let mut token = Token::root();

let (mut device_guard, mut token) = hub.devices.write(&mut token);
let (mut buffer_guard, _) = hub.buffers.write(&mut token);
let buffer = buffer_guard
.get_mut(buffer_id)
.map_err(|_| resource::BufferAccessError::Invalid)?;
let device = &mut device_guard[buffer.device_id.value];

buffer: &mut resource::Buffer<A>,
device: &mut Device<A>,
) -> Result<Option<BufferMapPendingClosure>, BufferAccessError> {
log::debug!("Buffer {:?} map state -> Idle", buffer_id);
match mem::replace(&mut buffer.map_state, resource::BufferMapState::Idle) {
resource::BufferMapState::Init {
Expand Down Expand Up @@ -5501,10 +5564,7 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
}
}

let raw_buf = buffer
.raw
.as_ref()
.ok_or(resource::BufferAccessError::Destroyed)?;
let raw_buf = buffer.raw.as_ref().ok_or(BufferAccessError::Destroyed)?;

buffer.life_guard.use_at(device.active_submission_index + 1);
let region = wgt::BufferSize::new(buffer.size).map(|size| hal::BufferCopy {
Expand Down Expand Up @@ -5535,7 +5595,7 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
device.pending_writes.dst_buffers.insert(buffer_id);
}
resource::BufferMapState::Idle => {
return Err(resource::BufferAccessError::NotMapped);
return Err(BufferAccessError::NotMapped);
}
resource::BufferMapState::Waiting(pending) => {
return Ok(Some((pending.op, resource::BufferMapAsyncStatus::Aborted)));
Expand Down Expand Up @@ -5572,10 +5632,27 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
pub fn buffer_unmap<A: HalApi>(
&self,
buffer_id: id::BufferId,
) -> Result<(), resource::BufferAccessError> {
//Note: outside inner function so no locks are held when calling the callback
let closure = self.buffer_unmap_inner::<A>(buffer_id)?;
if let Some((operation, status)) = closure {
) -> Result<(), BufferAccessError> {
profiling::scope!("unmap", "Buffer");

let closure;
{
// Restrict the locks to this scope.
let hub = A::hub(self);
let mut token = Token::root();

let (mut device_guard, mut token) = hub.devices.write(&mut token);
let (mut buffer_guard, _) = hub.buffers.write(&mut token);
let buffer = buffer_guard
.get_mut(buffer_id)
.map_err(|_| BufferAccessError::Invalid)?;
let device = &mut device_guard[buffer.device_id.value];

closure = self.buffer_unmap_inner(buffer_id, buffer, device)
}

// Note: outside the scope where locks are held when calling the callback
if let Some((operation, status)) = closure? {
operation.callback.call(status);
}
Ok(())
Expand Down
Loading