From 6c46605945b02ded697b493d066d41f6919dc210 Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Wed, 31 May 2023 19:15:11 -0700 Subject: [PATCH] Don't report violations of VUID-vkCmdEndDebugUtilsLabelEXT-commandBuffer-01912. As described in [Vulkan-ValidationLayers#5671], the validation layers don't understand debug ranges paired across different command buffers on the same queue, even though the Vulkan spec says: > An application may open a debug label region in one command buffer and close it in another, or otherwise split debug label regions across multiple command buffers or multiple queue submissions. When viewed from the linear series of submissions to a single queue, the calls to vkCmdBeginDebugUtilsLabelEXT and vkCmdEndDebugUtilsLabelEXT must be matched and balanced. Until this is fixed, wgpu should ignore this validation error to reduce noise in test runs. Consult the validation layer's description and version, to filter out only affected versions of the validation layer. Fixes #3733. [Vulkan-ValidationLayers#5671]: https://github.com/KhronosGroup/Vulkan-ValidationLayers/issues/5671, --- CHANGELOG.md | 4 + wgpu-hal/src/vulkan/instance.rs | 169 ++++++++++++++++++++------------ wgpu-hal/src/vulkan/mod.rs | 21 ++++ 3 files changed, 133 insertions(+), 61 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index abccdca4a92..2ea94cc1f96 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -50,6 +50,10 @@ Bottom level categories: - Increase the `max_storage_buffers_per_shader_stage` and `max_storage_textures_per_shader_stage` limits based on what the hardware supports. by @Elabajaba in [#3798]https://github.com/gfx-rs/wgpu/pull/3798 +#### Vulkan + +- Work around [Vulkan-ValidationLayers#5671](https://github.com/KhronosGroup/Vulkan-ValidationLayers/issues/5671) by ignoring reports of violations of [VUID-vkCmdEndDebugUtilsLabelEXT-commandBuffer-01912](https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-vkCmdEndDebugUtilsLabelEXT-commandBuffer-01912). By @jimblandy in [#3809](https://github.com/gfx-rs/wgpu/pull/3809). + ### Documentation #### General diff --git a/wgpu-hal/src/vulkan/instance.rs b/wgpu-hal/src/vulkan/instance.rs index 101f303c16a..d34197cf9fe 100644 --- a/wgpu-hal/src/vulkan/instance.rs +++ b/wgpu-hal/src/vulkan/instance.rs @@ -14,15 +14,39 @@ unsafe extern "system" fn debug_utils_messenger_callback( message_severity: vk::DebugUtilsMessageSeverityFlagsEXT, message_type: vk::DebugUtilsMessageTypeFlagsEXT, callback_data_ptr: *const vk::DebugUtilsMessengerCallbackDataEXT, - _user_data: *mut c_void, + user_data: *mut c_void, ) -> vk::Bool32 { - const VUID_VKSWAPCHAINCREATEINFOKHR_IMAGEEXTENT_01274: i32 = 0x7cd0911d; use std::borrow::Cow; if thread::panicking() { return vk::FALSE; } + let cd = unsafe { &*callback_data_ptr }; + let user_data = unsafe { &*(user_data as *mut super::DebugUtilsMessengerUserData) }; + + const VUID_VKCMDENDDEBUGUTILSLABELEXT_COMMANDBUFFER_01912: i32 = 0x56146426; + if cd.message_id_number == VUID_VKCMDENDDEBUGUTILSLABELEXT_COMMANDBUFFER_01912 { + // https://github.com/KhronosGroup/Vulkan-ValidationLayers/issues/5671 + // Versions 1.3.240 through 1.3.250 return a spurious error here if + // the debug range start and end appear in different command buffers. + let khronos_validation_layer = + std::ffi::CStr::from_bytes_with_nul(b"Khronos Validation Layer\0").unwrap(); + if user_data.description.as_ref() == khronos_validation_layer + && user_data.spec_version >= vk::make_api_version(0, 1, 3, 240) + && user_data.spec_version <= vk::make_api_version(0, 1, 3, 250) + { + return vk::FALSE; + } + } + + // Silence Vulkan Validation error "VUID-VkSwapchainCreateInfoKHR-imageExtent-01274" + // - it's a false positive due to the inherent racy-ness of surface resizing + const VUID_VKSWAPCHAINCREATEINFOKHR_IMAGEEXTENT_01274: i32 = 0x7cd0911d; + if cd.message_id_number == VUID_VKSWAPCHAINCREATEINFOKHR_IMAGEEXTENT_01274 { + return vk::FALSE; + } + let level = match message_severity { vk::DebugUtilsMessageSeverityFlagsEXT::VERBOSE => log::Level::Debug, vk::DebugUtilsMessageSeverityFlagsEXT::INFO => log::Level::Info, @@ -31,8 +55,6 @@ unsafe extern "system" fn debug_utils_messenger_callback( _ => log::Level::Warn, }; - let cd = unsafe { &*callback_data_ptr }; - let message_id_name = if cd.p_message_id_name.is_null() { Cow::from("") } else { @@ -44,12 +66,6 @@ unsafe extern "system" fn debug_utils_messenger_callback( unsafe { CStr::from_ptr(cd.p_message) }.to_string_lossy() }; - // Silence Vulkan Validation error "VUID-VkSwapchainCreateInfoKHR-imageExtent-01274" - // - it's a false positive due to the inherent racy-ness of surface resizing - if cd.message_id_number == VUID_VKSWAPCHAINCREATEINFOKHR_IMAGEEXTENT_01274 { - return vk::FALSE; - } - let _ = std::panic::catch_unwind(|| { log::log!( level, @@ -236,12 +252,16 @@ impl super::Instance { /// - `extensions` must be a superset of `required_extensions()` and must be created from the /// same entry, driver_api_version and flags. /// - `android_sdk_version` is ignored and can be `0` for all platforms besides Android + /// + /// If `debug_utils_user_data` is `Some`, then the validation layer is + /// available, so create a [`vk::DebugUtilsMessengerExt`]. #[allow(clippy::too_many_arguments)] pub unsafe fn from_raw( entry: ash::Entry, raw_instance: ash::Instance, driver_api_version: u32, android_sdk_version: u32, + debug_utils_user_data: Option, extensions: Vec<&'static CStr>, flags: crate::InstanceFlags, has_nv_optimus: bool, @@ -249,35 +269,50 @@ impl super::Instance { ) -> Result { log::info!("Instance version: 0x{:x}", driver_api_version); - let debug_utils = if extensions.contains(&ext::DebugUtils::name()) { - log::info!("Enabling debug utils"); - let extension = ext::DebugUtils::new(&entry, &raw_instance); - // having ERROR unconditionally because Vk doesn't like empty flags - let mut severity = vk::DebugUtilsMessageSeverityFlagsEXT::ERROR; - if log::max_level() >= log::LevelFilter::Debug { - severity |= vk::DebugUtilsMessageSeverityFlagsEXT::VERBOSE; - } - if log::max_level() >= log::LevelFilter::Info { - severity |= vk::DebugUtilsMessageSeverityFlagsEXT::INFO; - } - if log::max_level() >= log::LevelFilter::Warn { - severity |= vk::DebugUtilsMessageSeverityFlagsEXT::WARNING; + eprintln!( + "JIMB: Instance::from_raw(debug_utils_user_data = {:?})", + debug_utils_user_data + ); + let debug_utils = if let Some(debug_callback_user_data) = debug_utils_user_data { + if extensions.contains(&ext::DebugUtils::name()) { + log::info!("Enabling debug utils"); + // Move the callback data to the heap, to ensure it will never be + // moved. + let callback_data = Box::new(debug_callback_user_data); + + let extension = ext::DebugUtils::new(&entry, &raw_instance); + // having ERROR unconditionally because Vk doesn't like empty flags + let mut severity = vk::DebugUtilsMessageSeverityFlagsEXT::ERROR; + if log::max_level() >= log::LevelFilter::Debug { + severity |= vk::DebugUtilsMessageSeverityFlagsEXT::VERBOSE; + } + if log::max_level() >= log::LevelFilter::Info { + severity |= vk::DebugUtilsMessageSeverityFlagsEXT::INFO; + } + if log::max_level() >= log::LevelFilter::Warn { + severity |= vk::DebugUtilsMessageSeverityFlagsEXT::WARNING; + } + let user_data_ptr: *const super::DebugUtilsMessengerUserData = &*callback_data; + let vk_info = vk::DebugUtilsMessengerCreateInfoEXT::builder() + .flags(vk::DebugUtilsMessengerCreateFlagsEXT::empty()) + .message_severity(severity) + .message_type( + vk::DebugUtilsMessageTypeFlagsEXT::GENERAL + | vk::DebugUtilsMessageTypeFlagsEXT::VALIDATION + | vk::DebugUtilsMessageTypeFlagsEXT::PERFORMANCE, + ) + .pfn_user_callback(Some(debug_utils_messenger_callback)) + .user_data(user_data_ptr as *mut _); + let messenger = + unsafe { extension.create_debug_utils_messenger(&vk_info, None) }.unwrap(); + Some(super::DebugUtils { + extension, + messenger, + callback_data, + }) + } else { + None } - let vk_info = vk::DebugUtilsMessengerCreateInfoEXT::builder() - .flags(vk::DebugUtilsMessengerCreateFlagsEXT::empty()) - .message_severity(severity) - .message_type( - vk::DebugUtilsMessageTypeFlagsEXT::GENERAL - | vk::DebugUtilsMessageTypeFlagsEXT::VALIDATION - | vk::DebugUtilsMessageTypeFlagsEXT::PERFORMANCE, - ) - .pfn_user_callback(Some(debug_utils_messenger_callback)); - let messenger = - unsafe { extension.create_debug_utils_messenger(&vk_info, None) }.unwrap(); - Some(super::DebugUtils { - extension, - messenger, - }) } else { None }; @@ -542,32 +577,43 @@ impl crate::Instance for super::Instance { log::info!("enumerate_instance_layer_properties: {:?}", e); crate::InstanceError })?; + eprintln!("JIMB: instance_layers =\n{instance_layers:#?}"); - let nv_optimus_layer = CStr::from_bytes_with_nul(b"VK_LAYER_NV_optimus\0").unwrap(); - let has_nv_optimus = instance_layers.iter().any(|inst_layer| { - cstr_from_bytes_until_nul(&inst_layer.layer_name) == Some(nv_optimus_layer) - }); + fn find_layer<'layers>( + instance_layers: &'layers [vk::LayerProperties], + name: &CStr, + ) -> Option<&'layers vk::LayerProperties> { + instance_layers + .iter() + .find(|inst_layer| cstr_from_bytes_until_nul(&inst_layer.layer_name) == Some(name)) + } - // Check requested layers against the available layers - let layers = { - let mut layers: Vec<&'static CStr> = Vec::new(); - if desc.flags.contains(crate::InstanceFlags::VALIDATION) { - layers.push(CStr::from_bytes_with_nul(b"VK_LAYER_KHRONOS_validation\0").unwrap()); + let nv_optimus_layer = CStr::from_bytes_with_nul(b"VK_LAYER_NV_optimus\0").unwrap(); + let has_nv_optimus = find_layer(&instance_layers, nv_optimus_layer).is_some(); + + let mut layers: Vec<&'static CStr> = Vec::new(); + + // Request validation layer if asked. + let mut debug_callback_user_data = None; + if desc.flags.contains(crate::InstanceFlags::VALIDATION) { + let validation_layer = + CStr::from_bytes_with_nul(b"VK_LAYER_KHRONOS_validation\0").unwrap(); + eprintln!("JIMB: validation_layer = {validation_layer:?}"); + if let Some(layer_properties) = find_layer(&instance_layers, validation_layer) { + layers.push(validation_layer); + debug_callback_user_data = Some(super::DebugUtilsMessengerUserData { + description: cstr_from_bytes_until_nul(&layer_properties.description) + .unwrap() + .to_owned(), + spec_version: layer_properties.spec_version, + }); + } else { + log::warn!( + "Unable to find layer: {}", + validation_layer.to_string_lossy() + ); } - - // Only keep available layers. - layers.retain(|&layer| { - if instance_layers.iter().any(|inst_layer| { - cstr_from_bytes_until_nul(&inst_layer.layer_name) == Some(layer) - }) { - true - } else { - log::warn!("Unable to find layer: {}", layer.to_string_lossy()); - false - } - }); - layers - }; + } #[cfg(target_os = "android")] let android_sdk_version = { @@ -619,6 +665,7 @@ impl crate::Instance for super::Instance { vk_instance, driver_api_version, android_sdk_version, + debug_callback_user_data, extensions, desc.flags, has_nv_optimus, diff --git a/wgpu-hal/src/vulkan/mod.rs b/wgpu-hal/src/vulkan/mod.rs index 27200dc4e01..4bba32cb03b 100644 --- a/wgpu-hal/src/vulkan/mod.rs +++ b/wgpu-hal/src/vulkan/mod.rs @@ -75,6 +75,27 @@ impl crate::Api for Api { struct DebugUtils { extension: ext::DebugUtils, messenger: vk::DebugUtilsMessengerEXT, + + /// Owning pointer to the debug messenger callback user data. + /// + /// `InstanceShared::drop` destroys the debug messenger before + /// dropping this, so the callback should never receive a dangling + /// user data pointer. + #[allow(dead_code)] + callback_data: Box, +} + +/// User data needed by `instance::debug_utils_messenger_callback`. +/// +/// When we create the [`DebugUtilsMessengerEXT`], the `pUserData` +/// pointer refers to one of these values. +#[derive(Debug)] +pub struct DebugUtilsMessengerUserData { + /// Validation layer description, from `vk::LayerProperties`. + description: std::ffi::CString, + + /// Validation layer specification version, from `vk::LayerProperties`. + spec_version: u32, } pub struct InstanceShared {