Skip to content

Fix pipeline validation on Intel GPU#97

Merged
anunknowperson merged 5 commits into
mainfrom
intel_pp_fix
May 21, 2025
Merged

Fix pipeline validation on Intel GPU#97
anunknowperson merged 5 commits into
mainfrom
intel_pp_fix

Conversation

@anunknowperson

Copy link
Copy Markdown
Owner

No description provided.

@anunknowperson anunknowperson changed the title Intel pp fix Fix pipeline validation on Intel GPU May 21, 2025
@anunknowperson anunknowperson requested a review from Copilot May 21, 2025 20:18

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes pipeline validation on Intel GPUs by updating the vcpkg subproject commit and revising pipeline configuration defaults and error handling in Vulkan pipeline code.

  • Updated the vcpkg submodule commit.
  • Refactored PipelineBuilder::clear and build_pipeline to initialize state structs with proper defaults and local copies.
  • Adjusted graphics pipeline configurations for triangle, mesh, and compute pipelines.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
vcpkg Updated subproject commit for Intel GPU fixes
src/graphics/vulkan/vk_pipelines.cpp Made state initialization explicit and improved error handling
src/graphics/vulkan/pipelines.cpp Revised pipeline configurations with custom setup lambdas
Comments suppressed due to low confidence (1)

src/graphics/vulkan/vk_pipelines.cpp:272

  • Since an invalid depth format is silently reset to VK_FORMAT_UNDEFINED, consider adding a comment or log to clarify this behavior for future maintainers.
if (format != VK_FORMAT_D16_UNORM && ... ) {

Comment thread src/graphics/vulkan/vk_pipelines.cpp Outdated
return VK_NULL_HANDLE; // failed to create graphics pipeline
} else {
return newPipeline;
// Error handling

Copilot AI May 21, 2025

Copy link

Choose a reason for hiding this comment

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

[nitpick] Consider adding logging or additional error reporting when vkCreateGraphicsPipelines fails, so that debugging pipeline creation issues becomes easier.

Suggested change
// Error handling
spdlog::error("Failed to create graphics pipeline in build_pipeline.");

Copilot uses AI. Check for mistakes.
meshConfig.topology = VK_PRIMITIVE_TOPOLOGY_TRIANGLE_LIST;

// Explicitly setup pipeline details via callback
meshConfig.customPipelineSetup = [](PipelineBuilder& builder) {

Copilot AI May 21, 2025

Copy link

Choose a reason for hiding this comment

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

[nitpick] Consider adding an inline comment within this lambda to explain the rationale behind using VK_COMPARE_OP_GREATER for depth testing in the mesh pipeline configuration.

Copilot uses AI. Check for mistakes.
@anunknowperson anunknowperson requested review from Fra1sse and Copilot May 21, 2025 20:19

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the Vulkan pipeline builder to explicitly initialize state structs with sensible defaults and adds validation checks to prevent invalid pipeline configurations, targeting Intel GPU compatibility.

  • Populate and reset all pipeline state structs in clear()
  • Add runtime validation and fallback logic in build_pipeline()
  • Sanitize depth format inputs and expose custom setup callbacks in pipelines.cpp

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
vcpkg Bump submodule commit for updated dependencies
src/graphics/vulkan/vk_pipelines.cpp Refactor default state initialization and add validation in pipeline builder
src/graphics/vulkan/pipelines.cpp Update pipeline configuration and introduce customPipelineSetup callbacks
Comments suppressed due to low confidence (2)

src/graphics/vulkan/vk_pipelines.cpp:273

  • The sanitization logic in set_depth_format should be covered by unit tests to verify that invalid formats correctly fall back to VK_FORMAT_UNDEFINED.
if (format != VK_FORMAT_D16_UNORM && 

src/graphics/vulkan/vk_pipelines.cpp:245

  • [nitpick] The pNext member isn't explicitly set here (relies on zero-initialization). For consistency and clarity, explicitly set pNext = nullptr on all Vulkan structs.
_multisampling.sType = VK_STRUCTURE_TYPE_PIPELINE_MULTISAMPLE_STATE_CREATE_INFO;

Comment on lines +134 to +136
if (renderInfo.depthAttachmentFormat == 0) {
renderInfo.depthAttachmentFormat = VK_FORMAT_UNDEFINED;
}

Copilot AI May 21, 2025

Copy link

Choose a reason for hiding this comment

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

The check for == 0 is redundant since VK_FORMAT_UNDEFINED is defined as 0. This branch will never change the value. Consider removing this check or updating it to catch truly invalid formats.

Suggested change
if (renderInfo.depthAttachmentFormat == 0) {
renderInfo.depthAttachmentFormat = VK_FORMAT_UNDEFINED;
}
// Removed redundant check and assignment for depthAttachmentFormat.

Copilot uses AI. Check for mistakes.
triangleConfig.topology = VK_PRIMITIVE_TOPOLOGY_TRIANGLE_LIST;

// Ensure blending and depth testing setup is consistent
triangleConfig.customPipelineSetup = [](PipelineBuilder& builder) {

Copilot AI May 21, 2025

Copy link

Choose a reason for hiding this comment

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

[nitpick] Consider documenting the purpose of the customPipelineSetup callback parameter in GraphicsPipelineConfig, so future maintainers understand when and how to use it.

Copilot uses AI. Check for mistakes.
@anunknowperson anunknowperson merged commit fe3f8e6 into main May 21, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants