From 8e15707631f1db5612824dea122ea8b6a4e9b271 Mon Sep 17 00:00:00 2001 From: Eshed Schacham Date: Sun, 10 Mar 2024 22:12:51 +0200 Subject: [PATCH] gles: fix crash when holding multiple devices on wayland/surfaceless. (#5351) --- CHANGELOG.md | 3 +- tests/tests/device.rs | 86 +++++++++++++++++++++++++++------------- wgpu-hal/src/gles/egl.rs | 49 +++++++++++++++++++++-- 3 files changed, 105 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a126071c2..fa6a1d214f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,7 +41,7 @@ Bottom level categories: ### Major Changes -#### Vendored WebGPU Bindings from `web_sys` +#### Vendored WebGPU Bindings from `web_sys` **`--cfg=web_sys_unstable_apis` is no longer needed in your `RUSTFLAGS` to compile for WebGPU!!!** @@ -165,6 +165,7 @@ By @cwfitzgerald in [#5325](https://github.com/gfx-rs/wgpu/pull/5325). - Fixes for being able to use an OpenGL 4.1 core context provided by macOS with wgpu. By @bes in [#5331](https://github.com/gfx-rs/wgpu/pull/5331). - Don't create a program for shader-clearing if that workaround isn't required. By @Dinnerbone in [#5348](https://github.com/gfx-rs/wgpu/pull/5348). +- Fix crash when holding multiple devices on wayland/surfaceless. By @ashdnazg in [#5351](https://github.com/gfx-rs/wgpu/pull/5351). #### Vulkan diff --git a/tests/tests/device.rs b/tests/tests/device.rs index f6c42736a7..02e2d1fb83 100644 --- a/tests/tests/device.rs +++ b/tests/tests/device.rs @@ -29,39 +29,69 @@ static CROSS_DEVICE_BIND_GROUP_USAGE: GpuTestConfiguration = GpuTestConfiguratio }); #[cfg(not(all(target_arch = "wasm32", not(target_os = "emscripten"))))] -#[test] -fn device_lifetime_check() { - use pollster::FutureExt as _; - - env_logger::init(); - let instance = wgpu::Instance::new(wgpu::InstanceDescriptor { - backends: wgpu::util::backend_bits_from_env().unwrap_or(wgpu::Backends::all()), - dx12_shader_compiler: wgpu::util::dx12_shader_compiler_from_env().unwrap_or_default(), - gles_minor_version: wgpu::util::gles_minor_version_from_env().unwrap_or_default(), - flags: wgpu::InstanceFlags::advanced_debugging().with_env(), - }); +#[gpu_test] +static DEVICE_LIFETIME_CHECK: GpuTestConfiguration = GpuTestConfiguration::new() + .parameters(TestParameters::default()) + .run_sync(|_| { + use pollster::FutureExt as _; + + let instance = wgpu::Instance::new(wgpu::InstanceDescriptor { + backends: wgpu::util::backend_bits_from_env().unwrap_or(wgpu::Backends::all()), + dx12_shader_compiler: wgpu::util::dx12_shader_compiler_from_env().unwrap_or_default(), + gles_minor_version: wgpu::util::gles_minor_version_from_env().unwrap_or_default(), + flags: wgpu::InstanceFlags::advanced_debugging().with_env(), + }); - let adapter = wgpu::util::initialize_adapter_from_env_or_default(&instance, None) - .block_on() - .expect("failed to create adapter"); + let adapter = wgpu::util::initialize_adapter_from_env_or_default(&instance, None) + .block_on() + .expect("failed to create adapter"); - let (device, queue) = adapter - .request_device(&wgpu::DeviceDescriptor::default(), None) - .block_on() - .expect("failed to create device"); + let (device, queue) = adapter + .request_device(&wgpu::DeviceDescriptor::default(), None) + .block_on() + .expect("failed to create device"); - instance.poll_all(false); + instance.poll_all(false); - let pre_report = instance.generate_report().unwrap().unwrap(); + let pre_report = instance.generate_report().unwrap(); - drop(queue); - drop(device); - let post_report = instance.generate_report().unwrap().unwrap(); - assert_ne!( - pre_report, post_report, - "Queue and Device has not been dropped as expected" - ); -} + drop(queue); + drop(device); + let post_report = instance.generate_report().unwrap(); + assert_ne!( + pre_report, post_report, + "Queue and Device has not been dropped as expected" + ); + }); + +#[cfg(not(all(target_arch = "wasm32", not(target_os = "emscripten"))))] +#[gpu_test] +static MULTIPLE_DEVICES: GpuTestConfiguration = GpuTestConfiguration::new() + .parameters(TestParameters::default()) + .run_sync(|_| { + use pollster::FutureExt as _; + + fn create_device_and_queue() -> (wgpu::Device, wgpu::Queue) { + let instance = wgpu::Instance::new(wgpu::InstanceDescriptor { + backends: wgpu::util::backend_bits_from_env().unwrap_or(wgpu::Backends::all()), + dx12_shader_compiler: wgpu::util::dx12_shader_compiler_from_env() + .unwrap_or_default(), + gles_minor_version: wgpu::util::gles_minor_version_from_env().unwrap_or_default(), + flags: wgpu::InstanceFlags::advanced_debugging().with_env(), + }); + + let adapter = wgpu::util::initialize_adapter_from_env_or_default(&instance, None) + .block_on() + .expect("failed to create adapter"); + + adapter + .request_device(&wgpu::DeviceDescriptor::default(), None) + .block_on() + .expect("failed to create device") + } + + let _ = vec![create_device_and_queue(), create_device_and_queue()]; + }); #[cfg(not(all(target_arch = "wasm32", not(target_os = "emscripten"))))] #[gpu_test] diff --git a/wgpu-hal/src/gles/egl.rs b/wgpu-hal/src/gles/egl.rs index 874ad1729f..f4bfcf5487 100644 --- a/wgpu-hal/src/gles/egl.rs +++ b/wgpu-hal/src/gles/egl.rs @@ -1,7 +1,8 @@ use glow::HasContext; +use once_cell::sync::Lazy; use parking_lot::{Mutex, MutexGuard, RwLock}; -use std::{ffi, os::raw, ptr, rc::Rc, sync::Arc, time::Duration}; +use std::{collections::HashMap, ffi, os::raw, ptr, rc::Rc, 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; @@ -432,6 +433,45 @@ struct Inner { srgb_kind: SrgbFrameBufferKind, } +// Different calls to `eglGetPlatformDisplay` may return the same `Display`, making it a global +// state of all our `EglContext`s. This forces us to track the number of such context to prevent +// terminating the display if it's currently used by another `EglContext`. +static DISPLAYS_REFERENCE_COUNT: Lazy>> = Lazy::new(Default::default); + +fn initialize_display( + egl: &EglInstance, + display: khronos_egl::Display, +) -> Result<(i32, i32), khronos_egl::Error> { + let mut guard = DISPLAYS_REFERENCE_COUNT.lock(); + *guard.entry(display.as_ptr() as usize).or_default() += 1; + + // We don't need to check the reference count here since according to the `eglInitialize` + // documentation, initializing an already initialized EGL display connection has no effect + // besides returning the version numbers. + egl.initialize(display) +} + +fn terminate_display( + egl: &EglInstance, + display: khronos_egl::Display, +) -> Result<(), khronos_egl::Error> { + let key = &(display.as_ptr() as usize); + let mut guard = DISPLAYS_REFERENCE_COUNT.lock(); + let count_ref = guard + .get_mut(key) + .expect("Attempted to decref a display before incref was called"); + + if *count_ref > 1 { + *count_ref -= 1; + + Ok(()) + } else { + guard.remove(key); + + egl.terminate(display) + } +} + impl Inner { fn create( flags: wgt::InstanceFlags, @@ -439,7 +479,7 @@ impl Inner { display: khronos_egl::Display, force_gles_minor_version: wgt::Gles3MinorVersion, ) -> Result { - let version = egl.initialize(display).map_err(|e| { + let version = initialize_display(&egl, display).map_err(|e| { crate::InstanceError::with_source( String::from("failed to initialize EGL display connection"), e, @@ -608,7 +648,8 @@ impl Drop for Inner { { log::warn!("Error in destroy_context: {:?}", e); } - if let Err(e) = self.egl.instance.terminate(self.egl.display) { + + if let Err(e) = terminate_display(&self.egl.instance, self.egl.display) { log::warn!("Error in terminate: {:?}", e); } } @@ -778,7 +819,7 @@ impl crate::Instance for Instance { let display = unsafe { egl.get_platform_display( EGL_PLATFORM_SURFACELESS_MESA, - std::ptr::null_mut(), + khronos_egl::DEFAULT_DISPLAY, &[khronos_egl::ATTRIB_NONE], ) }