Closed w-pearson closed 4 months ago
Good catch, indeed by the time we get to replaying we've "baked" down any libraries into their final PSOs so patching the shaders in these component libraries is not necessary.
Even with this PR, renderdoc does still create a pipeline with an invalid topology when editing a shader in a pipeline with mesh shaders, but the validation layers don't care; I suspect the same thing happens with extended_dynamic_state3.
This should be covered by the VU you quoted which states that input assembly is ignored entirely if mesh shaders are present. Unfortunately the vulkan convention is that pointers that aren't used can be garbage not just can be NULL, so implementations and validation can't even read from it to see what the topology is. I think VUID-VkGraphicsPipelineCreateInfo-pInputAssemblyState-09032
probably isn't properly accounting for mesh shaders and I don't think it was intended to require a non-NULL pointer.
Description
Before, when replacing shaders, RenderDoc would treat these incomplete pipeline libraries as if they were complete pipelines, and thus attempt to create pipelines with incomplete data, producing several validation errors and on some devices crashes.
For instance, a pipeline library that only contains a vertex or fragment shader but not input assembly state would use invalid input assembly state (specifically,
VulkanShaderCache::MakeGraphicsPipelineInfo
would use the topology value fromVulkanCreationInfo::Pipeline
, which is set toVK_PRIMITIVE_TOPOLOGY_MAX_ENUM
(0x7fffffff) if the pipeline has no input assembly state byVulkanCreationInfo::Pipeline::Init
).Additionally, those pipeline libraries were treated as complete graphics pipelines, not libraries, as
VulkanShaderCache::MakeGraphicsPipelineInfo
removesVK_PIPELINE_CREATE_LIBRARY_BIT_KHR
and noVkGraphicsPipelineLibraryCreateInfoEXT
was added to thepNext
chain.RenderDoc already merges the pipeline libraries into complete graphics pipelines, so there is no reason to recreate the libraries with modified shaders.
This change removes the following validation warnings after replacing a shader that draws a teapot on the
graphics_pipeline_library
Khronos sample:The Vulkan spec is a bit ambiguous about whether a topology of
VK_PRIMITIVE_TOPOLOGY_MAX_ENUM
is ever legal. A pipeline library withoutVK_GRAPHICS_PIPELINE_LIBRARY_VERTEX_INPUT_INTERFACE_BIT_EXT
passesVUID-VkGraphicsPipelineCreateInfo-dynamicPrimitiveTopologyUnrestricted-09031
sopInputAssemblyState
can be null, but if it is not null thenVUID-VkGraphicsPipelineCreateInfo-pInputAssemblyState-09032
says it needs to be valid. But the members section says thatpInputAssemblyState
"is ignored if the pipeline includes a mesh shader stage", not just "can be null". The description section says that members being null "is optional so the application can still use a valid pointer if it needs to set thepNext
orflags
fields to specify state for other extensions".In practice, as of Vulkan SDK 1.3.280.0, it seems like the validation layers do not care about invalid topologies in
pInputAssemblyState
for a pipeline library without the vertex input interface bit or for pipelines containing mesh shaders (tested with themesh_shading
Khronos sample). I didn't try to test theextended_dynamic_state3
case as none of the Khronos samples usedynamicPrimitiveTopologyUnrestricted
. Even with this PR, renderdoc does still create a pipeline with an invalid topology when editing a shader in a pipeline with mesh shaders, but the validation layers don't care; I suspect the same thing happens withextended_dynamic_state3
.An alternative implementation of this PR that re-created pipeline libraries with the appropriate flags would also pass the validation layers, but is more awkward to implement (especially since prior to Vulkan 1.3.260,
VkGraphicsPipelineLibraryCreateInfoEXT::pNext
was a non-const
pointer, which meansconst_cast
is needed for inserting it to thepNext
chain). Another alternative option would be to only setpInputAssemblyState
if the originalpInputAssemblyState
was non-null, but that is quite messy when applied to the other fields too.