Skip to content

fix(gles): Tidy locking in struct Buffer #9505

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

Open
andyleiserson wants to merge 1 commit into
base: trunk
Choose a base branch
from
+58 −44
Open

fix(gles): Tidy locking in struct Buffer #9505

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions wgpu-hal/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,7 @@ bytemuck = { workspace = true, optional = true, features = ["derive"] }
hashbrown = { workspace = true }
ordered-float = { workspace = true, optional = true }
profiling = { workspace = true, optional = true, default-features = false }
static_assertions = { workspace = true }

# Backend: GLES
glow = { workspace = true, optional = true }
Expand Down
49 changes: 28 additions & 21 deletions wgpu-hal/src/gles/device.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -579,9 +579,11 @@ impl crate::Device for super::Device {
target,
size: desc.size,
map_flags: 0,
mapped: false.into(),
data: Some(Arc::new(MaybeMutex::new(vec![0; desc.size as usize]))),
offset_of_current_mapping: Arc::new(MaybeMutex::new(0)),
map_state: Arc::new(MaybeMutex::new(super::BufferMapState {
mapped: false,
data: Some(vec![0; desc.size as usize]),
offset_of_current_mapping: 0,
})),
});
}

Expand Down Expand Up @@ -668,7 +670,7 @@ impl crate::Device for super::Device {
}

let data = if emulate_map && desc.usage.contains(wgt::BufferUses::MAP_READ) {
Some(Arc::new(MaybeMutex::new(vec![0; desc.size as usize])))
Some(vec![0; desc.size as usize])
} else {
None
};
Expand All @@ -679,10 +681,12 @@ impl crate::Device for super::Device {
raw,
target,
size: desc.size,
mapped: false.into(),
map_flags,
data,
offset_of_current_mapping: Arc::new(MaybeMutex::new(0)),
map_state: Arc::new(MaybeMutex::new(super::BufferMapState {
mapped: false,
data,
offset_of_current_mapping: 0,
})),
})
}

Expand All @@ -707,20 +711,21 @@ impl crate::Device for super::Device {
let is_coherent = buffer.map_flags & glow::MAP_COHERENT_BIT != 0;
let ptr = match buffer.raw {
None => {
let mut vec = lock(buffer.data.as_ref().unwrap());
let mut map_state = lock(&buffer.map_state);
let vec = map_state.data.as_mut().unwrap();
let slice = &mut vec.as_mut_slice()[range.start as usize..range.end as usize];
slice.as_mut_ptr()
}
Some(raw) => {
let gl = &self.shared.context.lock();
unsafe { gl.bind_buffer(buffer.target, Some(raw)) };
let ptr = if let Some(ref map_read_allocation) = buffer.data {
let mut guard = lock(map_read_allocation);
let slice = guard.as_mut_slice();
let mut map_state = lock(&buffer.map_state);
let ptr = if let Some(map_read_allocation) = map_state.data.as_mut() {
let slice = map_read_allocation.as_mut_slice();
unsafe { self.shared.get_buffer_sub_data(gl, buffer.target, 0, slice) };
slice.as_mut_ptr()
} else {
*lock(&buffer.offset_of_current_mapping) = range.start;
map_state.offset_of_current_mapping = range.start;
// glMapBufferRange throws an error if length is 0.
// We want to allow mapping 0-sized buffer slices, so perform a workaround
// if the range length is 0. The resulting pointer must never be dereferenced.
Expand All @@ -732,7 +737,7 @@ impl crate::Device for super::Device {
.try_into()
.expect("Buffer range invalid for GLES");
if range_length != 0 {
buffer.mapped.set(true);
map_state.mapped = true;
unsafe {
gl.map_buffer_range(
buffer.target,
Expand All @@ -755,14 +760,15 @@ impl crate::Device for super::Device {
})
}
unsafe fn unmap_buffer(&self, buffer: &super::Buffer) {
if buffer.mapped.replace(false) {
let gl = &self.shared.context.lock();
let mut map_state = lock(&buffer.map_state);
if core::mem::replace(&mut map_state.mapped, false) {
if let Some(raw) = buffer.raw {
if buffer.data.is_none() {
let gl = &self.shared.context.lock();
if map_state.data.is_none() {
unsafe { gl.bind_buffer(buffer.target, Some(raw)) };
unsafe { gl.unmap_buffer(buffer.target) };
unsafe { gl.bind_buffer(buffer.target, None) };
*lock(&buffer.offset_of_current_mapping) = 0;
map_state.offset_of_current_mapping = 0;
}
}
}
Expand All @@ -771,13 +777,14 @@ impl crate::Device for super::Device {
where
I: Iterator<Item = crate::MemoryRange>,
{
if buffer.mapped.get() {
let gl = &self.shared.context.lock();
let map_state = lock(&buffer.map_state);
if map_state.mapped {
if let Some(raw) = buffer.raw {
if buffer.data.is_none() {
let gl = &self.shared.context.lock();
if map_state.data.is_none() {
unsafe { gl.bind_buffer(buffer.target, Some(raw)) };
for range in ranges {
let offset_of_current_mapping = *lock(&buffer.offset_of_current_mapping);
let offset_of_current_mapping = map_state.offset_of_current_mapping;
unsafe {
gl.flush_mapped_buffer_range(
buffer.target,
Expand Down
16 changes: 9 additions & 7 deletions wgpu-hal/src/gles/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,6 @@ pub use self::wgl::{Instance, Surface};

use alloc::{boxed::Box, string::String, string::ToString as _, sync::Arc, vec::Vec};
use core::{
cell::Cell,
fmt,
ops::Range,
sync::atomic::{AtomicU32, AtomicU8},
Expand Down Expand Up @@ -345,17 +344,20 @@ pub struct Buffer {
size: wgt::BufferAddress,
/// Flags to use within calls to [`Device::map_buffer`](crate::Device::map_buffer).
map_flags: u32,
map_state: Arc<MaybeMutex<BufferMapState>>,
}

#[derive(Clone, Debug)]
struct BufferMapState {
/// True if the GL buffer is actually mapped, i.e. not "fake-mapped" with
/// an empty slice
mapped: Cell<bool>,
Copy link
Copy Markdown
Contributor Author

@andyleiserson andyleiserson May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the key piece in all of this. Cell is not Sync and should not be used here.

data: Option<Arc<MaybeMutex<Vec<u8>>>>,
offset_of_current_mapping: Arc<MaybeMutex<wgt::BufferAddress>>,
mapped: bool,
data: Option<Vec<u8>>,
offset_of_current_mapping: wgt::BufferAddress,
}

#[cfg(send_sync)]
unsafe impl Sync for Buffer {}
#[cfg(send_sync)]
unsafe impl Send for Buffer {}
static_assertions::assert_impl_all!(Buffer: Send, Sync);

impl crate::DynBuffer for Buffer {}

Expand Down
35 changes: 19 additions & 16 deletions wgpu-hal/src/gles/queue.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,8 @@ impl super::Queue {
}
}
None => {
lock(dst.data.as_ref().unwrap()).as_mut_slice()
let mut map_state = lock(&dst.map_state);
map_state.data.as_mut().unwrap().as_mut_slice()
[range.start as usize..range.end as usize]
.fill(0);
}
Expand Down Expand Up @@ -419,8 +420,8 @@ impl super::Queue {
};
}
(Some(src), None) => {
let mut data = lock(dst.data.as_ref().unwrap());
let dst_data = &mut data.as_mut_slice()
let mut map_state = lock(&dst.map_state);
let dst_data = &mut map_state.data.as_mut().unwrap().as_mut_slice()
[copy.dst_offset as usize..copy.dst_offset as usize + size];

unsafe { gl.bind_buffer(copy_src_target, Some(src)) };
Expand All @@ -434,8 +435,8 @@ impl super::Queue {
};
}
(None, Some(dst)) => {
let data = lock(src.data.as_ref().unwrap());
let src_data = &data.as_slice()
let map_state = lock(&src.map_state);
let src_data = &map_state.data.as_ref().unwrap().as_slice()
[copy.src_offset as usize..copy.src_offset as usize + size];
unsafe { gl.bind_buffer(copy_dst_target, Some(dst)) };
unsafe {
Expand Down Expand Up @@ -775,17 +776,17 @@ impl super::Queue {
unsafe { gl.pixel_store_i32(glow::UNPACK_IMAGE_HEIGHT, column_texels as i32) };
let mut unbind_unpack_buffer = false;
if !dst_format.is_compressed() {
let buffer_data;
let map_state;
let unpack_data = match src.raw {
Some(buffer) => {
unsafe { gl.bind_buffer(glow::PIXEL_UNPACK_BUFFER, Some(buffer)) };
unbind_unpack_buffer = true;
glow::PixelUnpackData::BufferOffset(copy.buffer_layout.offset as u32)
}
None => {
buffer_data = lock(src.data.as_ref().unwrap());
let src_data =
&buffer_data.as_slice()[copy.buffer_layout.offset as usize..];
map_state = lock(&src.map_state);
let src_data = &map_state.data.as_ref().unwrap().as_slice()
[copy.buffer_layout.offset as usize..];
glow::PixelUnpackData::Slice(Some(src_data))
}
};
Expand Down Expand Up @@ -837,7 +838,7 @@ impl super::Queue {
(bytes_per_image * (copy.size.depth - 1)) + minimum_bytes_per_image;
let offset = copy.buffer_layout.offset as u32;

let buffer_data;
let map_state;
let unpack_data = match src.raw {
Some(buffer) => {
unsafe { gl.bind_buffer(glow::PIXEL_UNPACK_BUFFER, Some(buffer)) };
Expand All @@ -847,8 +848,8 @@ impl super::Queue {
)
}
None => {
buffer_data = lock(src.data.as_ref().unwrap());
let src_data = &buffer_data.as_slice()
map_state = lock(&src.map_state);
let src_data = &map_state.data.as_ref().unwrap().as_slice()
[(offset as usize)..(offset + bytes_in_upload) as usize];
glow::CompressedPixelUnpackData::Slice(src_data)
}
Expand Down Expand Up @@ -920,16 +921,17 @@ impl super::Queue {
unsafe { gl.bind_framebuffer(glow::READ_FRAMEBUFFER, Some(self.copy_fbo)) };

let read_pixels = |offset| {
let mut buffer_data;
let mut map_state;
let unpack_data = match dst.raw {
Some(buffer) => {
unsafe { gl.pixel_store_i32(glow::PACK_ROW_LENGTH, row_texels as i32) };
unsafe { gl.bind_buffer(glow::PIXEL_PACK_BUFFER, Some(buffer)) };
glow::PixelPackData::BufferOffset(offset as u32)
}
None => {
buffer_data = lock(dst.data.as_ref().unwrap());
let dst_data = &mut buffer_data.as_mut_slice()[offset as usize..];
map_state = lock(&dst.map_state);
let dst_data = &mut map_state.data.as_mut().unwrap().as_mut_slice()
[offset as usize..];
glow::PixelPackData::Slice(Some(dst_data))
}
};
Expand Down Expand Up @@ -1094,7 +1096,8 @@ impl super::Queue {
};
}
None => {
let data = &mut lock(dst.data.as_ref().unwrap());
let mut map_state = lock(&dst.map_state);
let data = map_state.data.as_mut().unwrap();
let len = query_data.len().min(data.len());
data[..len].copy_from_slice(&query_data[..len]);
}
Expand Down
Loading