1729: Handle Multi-threaded EGL Context Access r=cwfitzgerald,kvark a=zicklag

**Connections**
https://github.com/gfx-rs/wgpu/discussions/1630, https://github.com/bevyengine/bevy/issues/841

**Description**
Implements the synchronization necessary to use the GL backend from multiple threads. Accomplishes this by using a mutex around the GL context with extra wrapping to bind and unbind the EGL context when locking and unlocking.

**Testing**
Tested on Ubunty 20.04 with a fork of the Bevy game engine and the WGPU examples ( not that the examples test the multi-threading ).

## Remaining Issues

There is only one Bevy example I cannot get to run yet and it's the `load_gltf` example. It fails with a shader translation error:

```
Jul 26 20:36:50.949 ERROR naga::back::glsl: Conflicting samplers for _group_3_binding_10    
Jul 26 20:36:50.950  WARN wgpu::backend::direct: Shader translation error for stage FRAGMENT: A image was used with multiple samplers    
Jul 26 20:36:50.950  WARN wgpu::backend::direct: Please report it to https://github.com/gfx-rs/naga    
Jul 26 20:36:50.950 ERROR wgpu::backend::direct: wgpu error: Validation Error

Caused by:
    In Device::create_render_pipeline
    Internal error in FRAGMENT shader: A image was used with multiple samplers
```

Interestingly, I think the shader in question doesn't have a `group(3), binding(10)` anywhere that I know of so I'm going to have to drill down a bit more and find out exactly which shader translation is failing more.

This could potentially be fixed in a separate PR. I think the rest of this PR is rather straight-forward and the fix for the error above is probably mostly unrelated to the primary changes made in this PR.

Co-authored-by: Zicklag <zicklag@katharostech.com>
This commit is contained in:
bors[bot]
2021-07-27 19:43:12 +00:00
committed by GitHub
6 changed files with 181 additions and 67 deletions

View File

@@ -163,7 +163,10 @@ impl super::Adapter {
}
}
pub(super) unsafe fn expose(gl: glow::Context) -> Option<crate::ExposedAdapter<super::Api>> {
pub(super) unsafe fn expose(
context: super::AdapterContext,
) -> Option<crate::ExposedAdapter<super::Api>> {
let gl = context.lock();
let extensions = gl.supported_extensions();
let (vendor_const, renderer_const) = if extensions.contains("WEBGL_debug_renderer_info") {
@@ -333,10 +336,13 @@ impl super::Adapter {
let downlevel_defaults = wgt::DownlevelLimits {};
// Drop the GL guard so we can move the context into AdapterShared
drop(gl);
Some(crate::ExposedAdapter {
adapter: super::Adapter {
shared: Arc::new(super::AdapterShared {
context: gl,
context,
private_caps,
workarounds,
shading_language_version,
@@ -401,7 +407,7 @@ impl crate::Adapter<super::Api> for super::Adapter {
&self,
features: wgt::Features,
) -> Result<crate::OpenDevice<super::Api>, crate::DeviceError> {
let gl = &self.shared.context;
let gl = &self.shared.context.lock();
gl.pixel_store_i32(glow::UNPACK_ALIGNMENT, 1);
gl.pixel_store_i32(glow::PACK_ALIGNMENT, 1);
let main_vao = gl

View File

@@ -73,12 +73,11 @@ impl CompilationContext<'_> {
impl super::Device {
unsafe fn compile_shader(
&self,
gl: &glow::Context,
shader: &str,
naga_stage: naga::ShaderStage,
label: Option<&str>,
) -> Result<glow::Shader, crate::PipelineError> {
let gl = &self.shared.context;
let target = match naga_stage {
naga::ShaderStage::Vertex => glow::VERTEX_SHADER,
naga::ShaderStage::Fragment => glow::FRAGMENT_SHADER,
@@ -111,7 +110,7 @@ impl super::Device {
}
fn create_shader(
&self,
gl: &glow::Context,
naga_stage: naga::ShaderStage,
stage: &crate::ProgrammableStage<super::Api>,
context: CompilationContext,
@@ -156,16 +155,16 @@ impl super::Device {
reflection_info,
);
unsafe { self.compile_shader(&output, naga_stage, stage.module.label.as_deref()) }
unsafe { Self::compile_shader(gl, &output, naga_stage, stage.module.label.as_deref()) }
}
unsafe fn create_pipeline<'a, I: Iterator<Item = ShaderStage<'a>>>(
&self,
gl: &glow::Context,
shaders: I,
layout: &super::PipelineLayout,
label: crate::Label,
) -> Result<super::PipelineInner, crate::PipelineError> {
let gl = &self.shared.context;
let program = gl.create_program().unwrap();
if let Some(label) = label {
if gl.supports_debug() {
@@ -186,7 +185,7 @@ impl super::Device {
name_binding_map: &mut name_binding_map,
};
let shader = self.create_shader(naga_stage, stage, context)?;
let shader = Self::create_shader(gl, naga_stage, stage, context)?;
shaders_to_delete.push(shader);
}
@@ -199,7 +198,7 @@ impl super::Device {
let shader_src = format!("#version {} es \n void main(void) {{}}", version,);
log::info!("Only vertex shader is present. Creating an empty fragment shader",);
let shader =
self.compile_shader(&shader_src, naga::ShaderStage::Fragment, Some("_dummy"))?;
Self::compile_shader(gl, &shader_src, naga::ShaderStage::Fragment, Some("_dummy"))?;
shaders_to_delete.push(shader);
}
@@ -292,7 +291,7 @@ impl super::Device {
impl crate::Device<super::Api> for super::Device {
unsafe fn exit(self, queue: super::Queue) {
let gl = &self.shared.context;
let gl = &self.shared.context.lock();
gl.delete_vertex_array(self.main_vao);
gl.delete_framebuffer(queue.draw_fbo);
gl.delete_framebuffer(queue.copy_fbo);
@@ -303,7 +302,7 @@ impl crate::Device<super::Api> for super::Device {
&self,
desc: &crate::BufferDescriptor,
) -> Result<super::Buffer, crate::DeviceError> {
let gl = &self.shared.context;
let gl = &self.shared.context.lock();
let target = if desc.usage.contains(crate::BufferUses::INDEX) {
glow::ELEMENT_ARRAY_BUFFER
@@ -360,7 +359,7 @@ impl crate::Device<super::Api> for super::Device {
})
}
unsafe fn destroy_buffer(&self, buffer: super::Buffer) {
let gl = &self.shared.context;
let gl = &self.shared.context.lock();
gl.delete_buffer(buffer.raw);
}
@@ -369,7 +368,7 @@ impl crate::Device<super::Api> for super::Device {
buffer: &super::Buffer,
range: crate::MemoryRange,
) -> Result<crate::BufferMapping, crate::DeviceError> {
let gl = &self.shared.context;
let gl = &self.shared.context.lock();
let is_coherent = buffer.map_flags & glow::MAP_COHERENT_BIT != 0;
@@ -388,7 +387,7 @@ impl crate::Device<super::Api> for super::Device {
})
}
unsafe fn unmap_buffer(&self, buffer: &super::Buffer) -> Result<(), crate::DeviceError> {
let gl = &self.shared.context;
let gl = &self.shared.context.lock();
gl.bind_buffer(buffer.target, Some(buffer.raw));
gl.unmap_buffer(buffer.target);
gl.bind_buffer(buffer.target, None);
@@ -398,7 +397,7 @@ impl crate::Device<super::Api> for super::Device {
where
I: Iterator<Item = crate::MemoryRange>,
{
let gl = &self.shared.context;
let gl = &self.shared.context.lock();
gl.bind_buffer(buffer.target, Some(buffer.raw));
for range in ranges {
gl.flush_mapped_buffer_range(
@@ -416,7 +415,7 @@ impl crate::Device<super::Api> for super::Device {
&self,
desc: &crate::TextureDescriptor,
) -> Result<super::Texture, crate::DeviceError> {
let gl = &self.shared.context;
let gl = &self.shared.context.lock();
let render_usage = crate::TextureUses::COLOR_TARGET
| crate::TextureUses::DEPTH_STENCIL_WRITE
@@ -559,7 +558,7 @@ impl crate::Device<super::Api> for super::Device {
})
}
unsafe fn destroy_texture(&self, texture: super::Texture) {
let gl = &self.shared.context;
let gl = &self.shared.context.lock();
match texture.inner {
super::TextureInner::Renderbuffer { raw, .. } => {
gl.delete_renderbuffer(raw);
@@ -600,7 +599,7 @@ impl crate::Device<super::Api> for super::Device {
&self,
desc: &crate::SamplerDescriptor,
) -> Result<super::Sampler, crate::DeviceError> {
let gl = &self.shared.context;
let gl = &self.shared.context.lock();
let raw = gl.create_sampler().unwrap();
@@ -667,7 +666,7 @@ impl crate::Device<super::Api> for super::Device {
Ok(super::Sampler { raw })
}
unsafe fn destroy_sampler(&self, sampler: super::Sampler) {
let gl = &self.shared.context;
let gl = &self.shared.context.lock();
gl.delete_sampler(sampler.raw);
}
@@ -862,12 +861,13 @@ impl crate::Device<super::Api> for super::Device {
&self,
desc: &crate::RenderPipelineDescriptor<super::Api>,
) -> Result<super::RenderPipeline, crate::PipelineError> {
let gl = &self.shared.context.lock();
let shaders = iter::once((naga::ShaderStage::Vertex, &desc.vertex_stage)).chain(
desc.fragment_stage
.as_ref()
.map(|fs| (naga::ShaderStage::Fragment, fs)),
);
let inner = self.create_pipeline(shaders, desc.layout, desc.label)?;
let inner = self.create_pipeline(gl, shaders, desc.layout, desc.label)?;
let (vertex_buffers, vertex_attributes) = {
let mut buffers = Vec::new();
@@ -925,7 +925,7 @@ impl crate::Device<super::Api> for super::Device {
})
}
unsafe fn destroy_render_pipeline(&self, pipeline: super::RenderPipeline) {
let gl = &self.shared.context;
let gl = &self.shared.context.lock();
gl.delete_program(pipeline.inner.program);
}
@@ -933,13 +933,14 @@ impl crate::Device<super::Api> for super::Device {
&self,
desc: &crate::ComputePipelineDescriptor<super::Api>,
) -> Result<super::ComputePipeline, crate::PipelineError> {
let gl = &self.shared.context.lock();
let shaders = iter::once((naga::ShaderStage::Compute, &desc.stage));
let inner = self.create_pipeline(shaders, desc.layout, desc.label)?;
let inner = self.create_pipeline(gl, shaders, desc.layout, desc.label)?;
Ok(super::ComputePipeline { inner })
}
unsafe fn destroy_compute_pipeline(&self, pipeline: super::ComputePipeline) {
let gl = &self.shared.context;
let gl = &self.shared.context.lock();
gl.delete_program(pipeline.inner.program);
}
@@ -948,7 +949,7 @@ impl crate::Device<super::Api> for super::Device {
desc: &wgt::QuerySetDescriptor<crate::Label>,
) -> Result<super::QuerySet, crate::DeviceError> {
use std::fmt::Write;
let gl = &self.shared.context;
let gl = &self.shared.context.lock();
let mut temp_string = String::new();
let mut queries = Vec::with_capacity(desc.count as usize);
@@ -975,7 +976,7 @@ impl crate::Device<super::Api> for super::Device {
})
}
unsafe fn destroy_query_set(&self, set: super::QuerySet) {
let gl = &self.shared.context;
let gl = &self.shared.context.lock();
for &query in set.queries.iter() {
gl.delete_query(query);
}
@@ -987,7 +988,7 @@ impl crate::Device<super::Api> for super::Device {
})
}
unsafe fn destroy_fence(&self, fence: super::Fence) {
let gl = &self.shared.context;
let gl = &self.shared.context.lock();
for (_, sync) in fence.pending {
gl.delete_sync(sync);
}
@@ -996,7 +997,7 @@ impl crate::Device<super::Api> for super::Device {
&self,
fence: &super::Fence,
) -> Result<crate::FenceValue, crate::DeviceError> {
Ok(fence.get_latest(&self.shared.context))
Ok(fence.get_latest(&self.shared.context.lock()))
}
unsafe fn wait(
&self,
@@ -1005,7 +1006,7 @@ impl crate::Device<super::Api> for super::Device {
timeout_ms: u32,
) -> Result<bool, crate::DeviceError> {
if fence.last_completed < wait_value {
let gl = &self.shared.context;
let gl = &self.shared.context.lock();
let timeout_ns = (timeout_ms as u64 * 1_000_000).min(!0u32 as u64);
let &(_, sync) = fence
.pending

View File

@@ -1,7 +1,10 @@
use glow::HasContext;
use parking_lot::Mutex;
use parking_lot::{Mutex, MutexGuard};
use std::{ffi::CStr, os::raw, ptr, sync::Arc};
use std::{ffi::CStr, os::raw, ptr, sync::Arc, time::Duration};
/// The amount of time to wait while trying to obtain a lock to the adapter context
const CONTEXT_LOCK_TIMEOUT_SECS: u64 = 1;
const EGL_CONTEXT_FLAGS_KHR: i32 = 0x30FC;
const EGL_CONTEXT_OPENGL_DEBUG_BIT_KHR: i32 = 0x0001;
@@ -225,6 +228,90 @@ enum SrgbFrameBufferKind {
Khr,
}
/// A wrapper around a [`glow::Context`] and the required EGL context that uses locking to guarantee
/// exclusive access when shared with multiple threads.
pub struct AdapterContext {
glow_context: Mutex<glow::Context>,
egl: Arc<egl::DynamicInstance<egl::EGL1_4>>,
egl_display: egl::Display,
egl_context: egl::Context,
egl_pbuffer: Option<egl::Surface>,
}
unsafe impl Sync for AdapterContext {}
unsafe impl Send for AdapterContext {}
/// A guard containing a lock to an [`AdapterContext`]
pub struct AdapterContextLock<'a> {
glow_context: MutexGuard<'a, glow::Context>,
egl: &'a Arc<egl::DynamicInstance<egl::EGL1_4>>,
egl_display: egl::Display,
}
impl<'a> std::ops::Deref for AdapterContextLock<'a> {
type Target = glow::Context;
fn deref(&self) -> &Self::Target {
&self.glow_context
}
}
impl<'a> Drop for AdapterContextLock<'a> {
fn drop(&mut self) {
// Make the EGL context *not* current on this thread
self.egl
.make_current(self.egl_display, None, None, None)
.expect("Cannot make EGL context not current");
}
}
impl AdapterContext {
/// Get's the [`glow::Context`] without waiting for a lock
///
/// # Safety
///
/// This should only be called when you have manually made sure that the current thread has made
/// the EGL context current and that no other thread also has the EGL context current.
/// Additionally, you must manually make the EGL context **not** current after you are done with
/// it, so that future calls to `lock()` will not fail.
///
/// > **Note:** Calling this function **will** still lock the [`glow::Context`] which adds an
/// > extra safe-guard against accidental concurrent access to the context.
pub unsafe fn get_without_egl_lock(&self) -> MutexGuard<glow::Context> {
self.glow_context
.try_lock_for(Duration::from_secs(CONTEXT_LOCK_TIMEOUT_SECS))
.expect("Could not lock adapter context. This is most-likely a deadlcok.")
}
/// Obtain a lock to the EGL context and get handle to the [`glow::Context`] that can be used to
/// do rendering.
#[track_caller]
pub fn lock<'a>(&'a self) -> AdapterContextLock<'a> {
let glow_context = self
.glow_context
// Don't lock forever. If it takes longer than 1 second to get the lock we've got a
// deadlock and should panic to show where we got stuck
.try_lock_for(Duration::from_secs(CONTEXT_LOCK_TIMEOUT_SECS))
.expect("Could not lock adapter context. This is most-likely a deadlcok.");
// Make the EGL context current on this thread
self.egl
.make_current(
self.egl_display,
self.egl_pbuffer,
self.egl_pbuffer,
Some(self.egl_context),
)
.expect("Cannot make EGL context current");
AdapterContextLock {
glow_context,
egl: &self.egl,
egl_display: self.egl_display,
}
}
}
#[derive(Debug)]
struct Inner {
egl: Arc<egl::DynamicInstance<egl::EGL1_4>>,
@@ -329,7 +416,10 @@ impl Inner {
// Testing if context can be binded without surface
// and creating dummy pbuffer surface if not.
let pbuffer =
if version < (1, 5) || !display_extensions.contains("EGL_KHR_surfaceless_context") {
if version >= (1, 5) || display_extensions.contains("EGL_KHR_surfaceless_context") {
log::info!("\tEGL context: +surfaceless");
None
} else {
let attributes = [egl::WIDTH, 1, egl::HEIGHT, 1, egl::NONE];
egl.create_pbuffer_surface(display, config, &attributes)
.map(Some)
@@ -337,9 +427,6 @@ impl Inner {
log::warn!("Error in create_pbuffer_surface: {:?}", e);
crate::InstanceError
})?
} else {
log::info!("\tEGL context: +surfaceless");
None
};
let srgb_kind = if version >= (1, 5) {
@@ -623,6 +710,11 @@ impl crate::Instance<super::Api> for Instance {
}
}
inner
.egl
.make_current(inner.display, None, None, None)
.unwrap();
Ok(Surface {
egl: Arc::clone(&inner.egl),
raw,
@@ -684,7 +776,20 @@ impl crate::Instance<super::Api> for Instance {
gl.debug_message_callback(gl_debug_message_callback);
}
super::Adapter::expose(gl).into_iter().collect()
inner
.egl
.make_current(inner.display, None, None, None)
.unwrap();
super::Adapter::expose(AdapterContext {
glow_context: Mutex::new(gl),
egl: inner.egl.clone(),
egl_display: inner.display,
egl_context: inner.context,
egl_pbuffer: inner.pbuffer,
})
.into_iter()
.collect()
}
}
@@ -759,7 +864,7 @@ impl Surface {
crate::SurfaceError::Lost
})?;
self.egl
.make_current(self.display, self.pbuffer, self.pbuffer, Some(self.context))
.make_current(self.display, None, None, None)
.map_err(|e| {
log::error!("make_current(null) failed: {}", e);
crate::SurfaceError::Lost
@@ -791,7 +896,7 @@ impl crate::Surface<super::Api> for Surface {
}
let format_desc = device.shared.describe_texture_format(config.format);
let gl = &device.shared.context;
let gl = &device.shared.context.lock();
let renderbuffer = gl.create_renderbuffer().unwrap();
gl.bind_renderbuffer(glow::RENDERBUFFER, Some(renderbuffer));
gl.renderbuffer_storage(
@@ -824,7 +929,7 @@ impl crate::Surface<super::Api> for Surface {
}
unsafe fn unconfigure(&mut self, device: &super::Device) {
let gl = &device.shared.context;
let gl = &device.shared.context.lock();
if let Some(sc) = self.swapchain.take() {
gl.delete_renderbuffer(sc.renderbuffer);
gl.delete_framebuffer(sc.framebuffer);

View File

@@ -66,7 +66,7 @@ mod device;
mod queue;
#[cfg(not(target_arch = "wasm32"))]
use self::egl::{Instance, Surface};
use self::egl::{AdapterContext, Instance, Surface};
use arrayvec::ArrayVec;
@@ -161,7 +161,7 @@ struct TextureFormatDesc {
}
struct AdapterShared {
context: glow::Context,
context: AdapterContext,
private_caps: PrivateCapabilities,
workarounds: Workarounds,
shading_language_version: naga::back::glsl::Version,

View File

@@ -1,7 +1,7 @@
use super::Command as C;
use arrayvec::ArrayVec;
use glow::HasContext;
use std::{mem, ops::Range, slice};
use std::{mem, ops::Range, slice, sync::Arc};
const DEBUG_ID: u32 = 0;
@@ -27,9 +27,7 @@ fn is_3d_target(target: super::BindTarget) -> bool {
impl super::Queue {
/// Performs a manual shader clear, used as a workaround for a clearing bug on mesa
unsafe fn perform_shader_clear(&self, draw_buffer: u32, color: [f32; 4]) {
let gl = &self.shared.context;
unsafe fn perform_shader_clear(&self, gl: &glow::Context, draw_buffer: u32, color: [f32; 4]) {
gl.use_program(Some(self.shader_clear_program));
gl.uniform_4_f32(
Some(&self.shader_clear_program_color_uniform_location),
@@ -56,8 +54,7 @@ impl super::Queue {
}
}
unsafe fn reset_state(&self) {
let gl = &self.shared.context;
unsafe fn reset_state(&self, gl: &glow::Context) {
gl.use_program(None);
gl.bind_framebuffer(glow::FRAMEBUFFER, None);
gl.disable(glow::DEPTH_TEST);
@@ -71,8 +68,13 @@ impl super::Queue {
}
}
unsafe fn set_attachment(&self, fbo_target: u32, attachment: u32, view: &super::TextureView) {
let gl = &self.shared.context;
unsafe fn set_attachment(
&self,
gl: &glow::Context,
fbo_target: u32,
attachment: u32,
view: &super::TextureView,
) {
match view.inner {
super::TextureInner::Renderbuffer { raw } => {
gl.framebuffer_renderbuffer(fbo_target, attachment, glow::RENDERBUFFER, Some(raw));
@@ -99,8 +101,13 @@ impl super::Queue {
}
}
unsafe fn process(&mut self, command: &C, data_bytes: &[u8], data_words: &[u32]) {
let gl = &self.shared.context;
unsafe fn process(
&mut self,
gl: &glow::Context,
command: &C,
data_bytes: &[u8],
data_words: &[u32],
) {
match *command {
C::Draw {
topology,
@@ -551,7 +558,7 @@ impl super::Queue {
attachment,
ref view,
} => {
self.set_attachment(glow::DRAW_FRAMEBUFFER, attachment, view);
self.set_attachment(gl, glow::DRAW_FRAMEBUFFER, attachment, view);
}
C::ResolveAttachment {
attachment,
@@ -561,7 +568,7 @@ impl super::Queue {
gl.bind_framebuffer(glow::READ_FRAMEBUFFER, Some(self.draw_fbo));
gl.read_buffer(attachment);
gl.bind_framebuffer(glow::DRAW_FRAMEBUFFER, Some(self.copy_fbo));
self.set_attachment(glow::DRAW_FRAMEBUFFER, glow::COLOR_ATTACHMENT0, dst);
self.set_attachment(gl, glow::DRAW_FRAMEBUFFER, glow::COLOR_ATTACHMENT0, dst);
gl.blit_framebuffer(
0,
0,
@@ -601,7 +608,7 @@ impl super::Queue {
.contains(super::Workarounds::MESA_I915_SRGB_SHADER_CLEAR)
&& is_srgb
{
self.perform_shader_clear(draw_buffer, *color);
self.perform_shader_clear(gl, draw_buffer, *color);
} else {
gl.clear_buffer_f32_slice(glow::COLOR, draw_buffer, color);
}
@@ -933,25 +940,22 @@ impl crate::Queue<super::Api> for super::Queue {
command_buffers: &[&super::CommandBuffer],
signal_fence: Option<(&mut super::Fence, crate::FenceValue)>,
) -> Result<(), crate::DeviceError> {
self.reset_state();
let shared = Arc::clone(&self.shared);
let gl = &shared.context.lock();
self.reset_state(gl);
for cmd_buf in command_buffers.iter() {
if let Some(ref label) = cmd_buf.label {
self.shared.context.push_debug_group(
glow::DEBUG_SOURCE_APPLICATION,
DEBUG_ID,
label,
);
gl.push_debug_group(glow::DEBUG_SOURCE_APPLICATION, DEBUG_ID, label);
}
for command in cmd_buf.commands.iter() {
self.process(command, &cmd_buf.data_bytes, &cmd_buf.data_words);
self.process(gl, command, &cmd_buf.data_bytes, &cmd_buf.data_words);
}
if cmd_buf.label.is_some() {
self.shared.context.pop_debug_group();
gl.pop_debug_group();
}
}
if let Some((fence, value)) = signal_fence {
let gl = &self.shared.context;
fence.maintain(gl);
let sync = gl
.fence_sync(glow::SYNC_GPU_COMMANDS_COMPLETE, 0)
@@ -967,7 +971,7 @@ impl crate::Queue<super::Api> for super::Queue {
surface: &mut super::Surface,
texture: super::Texture,
) -> Result<(), crate::SurfaceError> {
let gl = &self.shared.context;
let gl = &self.shared.context.get_without_egl_lock();
surface.present(texture, gl)
}
}

View File

@@ -59,9 +59,7 @@ fn test_compute_overflow() {
#[test]
fn test_multithreaded_compute() {
initialize_test(
TestParameters::default()
.backend_failure(wgpu::Backends::GL)
.specific_failure(None, None, Some("V3D"), true),
TestParameters::default().specific_failure(None, None, Some("V3D"), true),
|ctx| {
use std::{sync::mpsc, thread, time::Duration};