From 8e5c5b963ba7626149b8a568f6c6023e8d57561c Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Wed, 22 Nov 2023 15:25:21 +0100 Subject: [PATCH] extensions: Always return `pipeline`/`shaders`, even on error In `ash::Device` `create_compute_pipeline()` and `create_graphics_pipeline()` already return the list of pipelines regardless of the error code, as [documented in the Multiple Pipeline Creation chapter]. Callers are expected to scan the returned array for valid pipeline handles and either use or destroy them (when handling an error). Furthermore, the caller is guaranteed that all handles will be NULL after the first returned NULL handle when `VK_PIPELINE_CREATE_EARLY_RETURN_ON_FAILURE_BIT` is set. Unfortunately this is not documented on the individual pipeline creation functions, but the overarching documentation does state that this applies to the raytracing pipeline creation functions in addition to graphics and compute pipeline creation. Now `VK_AMDX_shader_enqueue`'s `vkCreateExecutionGraphPipelinesAMDX()` is already defining much more clearly on its own page that _all_ output pipelines in the array will be written, but might be set to `vk::Pipeline::null()` if creation for it failed. However `vkCreateShadersEXT` from `VK_EXT_shader_object` takes the crown by additionally detailing that the caller has to destroy any non-`vk::Handle::is_null()` `vk::ShaderEXT` in the resulting array when the caller does not wish to use this impartial result when an error was returned. All of this is being tackled upstream, where the documentation for all these functions except Shader Object creation will be made to reference the Multiple Pipeline Creation chapter. [documented in the Multiple Pipeline Creation chapter]: https://registry.khronos.org/vulkan/specs/1.3-extensions/html/chap10.html#pipelines-multiple --- Changelog.md | 5 +++++ ash/src/extensions/amdx/shader_enqueue.rs | 20 ++++++++++++++---- ash/src/extensions/ext/shader_object.rs | 21 +++++++++++++++---- .../extensions/khr/ray_tracing_pipeline.rs | 20 +++++++++++------- ash/src/extensions/nv/ray_tracing.rs | 20 +++++++++++------- 5 files changed, 62 insertions(+), 24 deletions(-) diff --git a/Changelog.md b/Changelog.md index 99b391b69..466085dfe 100644 --- a/Changelog.md +++ b/Changelog.md @@ -49,6 +49,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `VK_KHR_device_group_creation`: Take borrow of `Entry` in `fn new()` (#753) - `VK_KHR_device_group_creation`: Rename `vk::Instance`-returning function from `device()` to `instance()` (#759) - Windows `HANDLE` types (`HWND`, `HINSTANCE`, `HMONITOR`) are now defined as `isize` instead of `*const c_void` (#797) +- extensions: Make all `vk::Pipeline` and `vk::ShaderEXT` creation functions return their impartial result on error (#828) + - `VK_AMDX_shader_enqueue` + - `VK_EXT_shader_object` + - `VK_KHR_ray_tracing_pipeline` + - `VK_NV_ray_tracing` - extensions/ext/ray_tracing_pipeline: Pass indirect SBT regions as single item reference (#829) - Replaced `c_char` array setters with `CStr` setters (#831) diff --git a/ash/src/extensions/amdx/shader_enqueue.rs b/ash/src/extensions/amdx/shader_enqueue.rs index d034a111c..760277b7b 100644 --- a/ash/src/extensions/amdx/shader_enqueue.rs +++ b/ash/src/extensions/amdx/shader_enqueue.rs @@ -22,23 +22,35 @@ impl ShaderEnqueue { } /// + /// + /// The implementation will create a pipeline for each element in `create_infos`. If creation of + /// any pipeline fails, that pipeline will be set to [`vk::Pipeline::null()`]. + /// + /// If creation fails for a pipeline create info with a + /// [`vk::ExecutionGraphPipelineCreateInfoAMDX::flags`] value that included + /// [`vk::PipelineCreateFlags::EARLY_RETURN_ON_FAILURE`], all pipelines at a greater index all + /// automatically fail. #[inline] pub unsafe fn create_execution_graph_pipelines( &self, pipeline_cache: vk::PipelineCache, create_infos: &[vk::ExecutionGraphPipelineCreateInfoAMDX<'_>], allocation_callbacks: Option<&vk::AllocationCallbacks<'_>>, - ) -> VkResult> { + ) -> Result, (Vec, vk::Result)> { let mut pipelines = Vec::with_capacity(create_infos.len()); - (self.fp.create_execution_graph_pipelines_amdx)( + let err_code = (self.fp.create_execution_graph_pipelines_amdx)( self.handle, pipeline_cache, create_infos.len() as u32, create_infos.as_ptr(), allocation_callbacks.as_raw_ptr(), pipelines.as_mut_ptr(), - ) - .set_vec_len_on_success(pipelines, create_infos.len()) + ); + pipelines.set_len(create_infos.len()); + match err_code { + vk::Result::SUCCESS => Ok(pipelines), + _ => Err((pipelines, err_code)), + } } /// diff --git a/ash/src/extensions/ext/shader_object.rs b/ash/src/extensions/ext/shader_object.rs index baaad721a..d03cb5eaf 100644 --- a/ash/src/extensions/ext/shader_object.rs +++ b/ash/src/extensions/ext/shader_object.rs @@ -23,21 +23,34 @@ impl ShaderObject { } /// + /// + /// When this function returns, whether or not it succeeds, it is guaranteed that every returned + /// element is either [`vk::ShaderEXT::null()`] or a valid [`vk::ShaderEXT`] handle. + /// + /// This means that whenever shader creation fails, the application can determine which shader + /// the returned error pertains to by locating the first [`vk::Handle::is_null()`] element + /// in the returned [`Vec`]. It also means that an application can reliably clean up from a + /// failed call by iterating over the returned [`Vec`] and destroying every element that is not + /// [`vk::Handle::is_null()`]. #[inline] pub unsafe fn create_shaders( &self, create_infos: &[vk::ShaderCreateInfoEXT<'_>], allocator: Option<&vk::AllocationCallbacks<'_>>, - ) -> VkResult> { + ) -> Result, (Vec, vk::Result)> { let mut shaders = Vec::with_capacity(create_infos.len()); - (self.fp.create_shaders_ext)( + let err_code = (self.fp.create_shaders_ext)( self.handle, create_infos.len() as u32, create_infos.as_ptr(), allocator.as_raw_ptr(), shaders.as_mut_ptr(), - ) - .set_vec_len_on_success(shaders, create_infos.len()) + ); + shaders.set_len(create_infos.len()); + match err_code { + vk::Result::SUCCESS => Ok(shaders), + _ => Err((shaders, err_code)), + } } /// diff --git a/ash/src/extensions/khr/ray_tracing_pipeline.rs b/ash/src/extensions/khr/ray_tracing_pipeline.rs index e29cde482..2fdcbc109 100644 --- a/ash/src/extensions/khr/ray_tracing_pipeline.rs +++ b/ash/src/extensions/khr/ray_tracing_pipeline.rs @@ -51,20 +51,24 @@ impl RayTracingPipeline { &self, deferred_operation: vk::DeferredOperationKHR, pipeline_cache: vk::PipelineCache, - create_info: &[vk::RayTracingPipelineCreateInfoKHR<'_>], + create_infos: &[vk::RayTracingPipelineCreateInfoKHR<'_>], allocation_callbacks: Option<&vk::AllocationCallbacks<'_>>, - ) -> VkResult> { - let mut pipelines = Vec::with_capacity(create_info.len()); - (self.fp.create_ray_tracing_pipelines_khr)( + ) -> Result, (Vec, vk::Result)> { + let mut pipelines = Vec::with_capacity(create_infos.len()); + let err_code = (self.fp.create_ray_tracing_pipelines_khr)( self.handle, deferred_operation, pipeline_cache, - create_info.len() as u32, - create_info.as_ptr(), + create_infos.len() as u32, + create_infos.as_ptr(), allocation_callbacks.as_raw_ptr(), pipelines.as_mut_ptr(), - ) - .set_vec_len_on_success(pipelines, create_info.len()) + ); + pipelines.set_len(create_infos.len()); + match err_code { + vk::Result::SUCCESS => Ok(pipelines), + _ => Err((pipelines, err_code)), + } } /// diff --git a/ash/src/extensions/nv/ray_tracing.rs b/ash/src/extensions/nv/ray_tracing.rs index d09a70959..35a56ec91 100755 --- a/ash/src/extensions/nv/ray_tracing.rs +++ b/ash/src/extensions/nv/ray_tracing.rs @@ -163,19 +163,23 @@ impl RayTracing { pub unsafe fn create_ray_tracing_pipelines( &self, pipeline_cache: vk::PipelineCache, - create_info: &[vk::RayTracingPipelineCreateInfoNV<'_>], + create_infos: &[vk::RayTracingPipelineCreateInfoNV<'_>], allocation_callbacks: Option<&vk::AllocationCallbacks<'_>>, - ) -> VkResult> { - let mut pipelines = Vec::with_capacity(create_info.len()); - (self.fp.create_ray_tracing_pipelines_nv)( + ) -> Result, (Vec, vk::Result)> { + let mut pipelines = Vec::with_capacity(create_infos.len()); + let err_code = (self.fp.create_ray_tracing_pipelines_nv)( self.handle, pipeline_cache, - create_info.len() as u32, - create_info.as_ptr(), + create_infos.len() as u32, + create_infos.as_ptr(), allocation_callbacks.as_raw_ptr(), pipelines.as_mut_ptr(), - ) - .set_vec_len_on_success(pipelines, create_info.len()) + ); + pipelines.set_len(create_infos.len()); + match err_code { + vk::Result::SUCCESS => Ok(pipelines), + _ => Err((pipelines, err_code)), + } } ///