Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix shader wrapper assert when pipeline binaries are used #1937

Merged
merged 1 commit into from
Jan 9, 2025
Merged

Fix shader wrapper assert when pipeline binaries are used #1937

merged 1 commit into from
Jan 9, 2025

Conversation

ziga-lunarg
Copy link
Contributor

@ziga-lunarg ziga-lunarg commented Dec 22, 2024 •
edited
Loading

From the spec:

If a VkPipelineBinaryInfoKHR structure with a binaryCount greater than 0 is included in the pNext chain of any Vk*PipelineCreateInfo structure when creating a pipeline, implementations must use the data in pPipelineBinaries instead of recalculating it. Any shader module identifiers or shader modules declared in VkPipelineShaderStageCreateInfo instances are ignored.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 330307.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5644 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5644 passed.

@bradgrantham-lunarg
Copy link
Contributor

raytracingpipelinesnv doesn't need this condition?

@ziga-lunarg
Copy link
Contributor Author

ziga-lunarg commented Dec 25, 2024 •
edited
Loading

raytracingpipelinesnv doesn't need this condition?

Good question, under the VK_KHR_pipeline_binary extension the spec says:

Extending VkGraphicsPipelineCreateInfo, VkComputePipelineCreateInfo, VkRayTracingPipelineCreateInfoKHR:

  • VkPipelineBinaryInfoKHR

So nothing about raytracingNV, but then there are VUIDs for raytracingNV and pipeline binaries. I guess I should add this for raytracingNV to be safe

Edit: Actually there is no PostProcess_vkCreateRayTracingPipelinesNV in VulkanCaptureManager

panos-lunarg
panos-lunarg reviewed Jan 9, 2025
View reviewed changes
framework/encode/vulkan_capture_manager.cpp Outdated
@@ -2779,14 +2779,18 @@ void VulkanCaptureManager::PostProcess_vkCreateGraphicsPipelines(VkResult
vulkan_wrappers::GetWrapper<vulkan_wrappers::PipelineWrapper>(pPipelines[p]);
assert(ppl_wrapper != nullptr);

for (uint32_t s = 0; s < pCreateInfos[p].stageCount; ++s)
const auto binary_info = graphics::vulkan_struct_get_pnext<VkPipelineBinaryInfoKHR>(&pCreateInfos[p]);
if (!binary_info || binary_info->binaryCount == 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit picking: According to our coding guidelines the checks should be the other way around:
if (binary_info == nullptr || !binary_info->binaryCount)
Same applies to the other 2 functions which have the same if condition

bradgrantham-lunarg
bradgrantham-lunarg approved these changes Jan 9, 2025
View reviewed changes
Copy link
Contributor

@bradgrantham-lunarg bradgrantham-lunarg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve with Panos' suggestion

@ziga-lunarg ziga-lunarg force-pushed the ziga-fix-pipeline-binaries-assert branch from 36c9abf to c825c15 Compare January 9, 2025 17:30
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 341502.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5756 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5756 passed.

@ziga-lunarg ziga-lunarg merged commit c13f87b into LunarG:dev Jan 9, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bradgrantham-lunarg bradgrantham-lunarg bradgrantham-lunarg approved these changes

@panos-lunarg panos-lunarg panos-lunarg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ziga-lunarg @ci-tester-lunarg @bradgrantham-lunarg @panos-lunarg