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.

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

Fix fragment_shading_rate_dynamic sample #916

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jherico
Copy link
Contributor

@jherico jherico commented Feb 16, 2024

Description

Fix the declaration of the compute_buffer in a loop as an r-value reference to a simple reference.

Fixes #913

General Checklist:

Please ensure the following points are checked:

  • My code follows the coding style
  • I have reviewed file licenses
  • I have commented any added functions (in line with Doxygen)
  • I have commented any code that could be hard to understand
  • My changes do not add any new compiler warnings
  • My changes do not add any new validation layer errors or warnings
  • I have used existing framework/helper functions where possible
  • My changes do not add any regressions
  • I have tested every sample to ensure everything runs correctly
  • This PR describes the scope and expected impact of the changes I am making

Note: The Samples CI runs a number of checks including:

  • I have updated the header Copyright to reflect the current year (CI build will fail if Copyright is out of date)
  • My changes build on Windows, Linux, macOS and Android. Otherwise I have documented any exceptions

Sample Checklist

If your PR contains a new or modified sample, these further checks must be carried out in addition to the General Checklist:

  • I have tested the sample on at least one compliant Vulkan implementation

Copy link
Collaborator

@SaschaWillems SaschaWillems left a comment

Choose a reason for hiding this comment

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

Sorry, was a bit hasty with my approval. Even with this fix applied, the sample crashes for me with the following error:

error] [framework\core\instance.cpp:50] -1360208885 - VUID-vkQueueSubmit-pCommandBuffers-00070: Validation Error: [ VUID-vkQueueSubmit-pCommandBuffers-00070 ] Object 0: handle = 0x247718630e0, type = VK_OBJECT_TYPE_COMMAND_BUFFER; | MessageID = 0xaeecdc0b | vkQueueSubmit(): pSubmits[0].pCommandBuffers[0] VkCommandBuffer 0x247718630e0[] is unrecorded and contains no commands. The Vulkan spec states: Each element of the pCommandBuffers member of each element of pSubmits must be in the pending or executable state (https://vulkan.lunarg.com/doc/view/1.3.275.0/windows/1.3-extensions/vkspec.html#VUID-vkQueueSubmit-pCommandBuffers-00070)

@SaschaWillems
Copy link
Collaborator

SaschaWillems commented Feb 19, 2024

I'm actually not sure why that sample works at all. It allocates the compute command buffers from the base class command pool, which may be limited to the graphics queue family index.

Imo it should be adjusted to create it's own compute command pool, like we do in the other compute related samples.

@jherico
Copy link
Contributor Author

jherico commented Feb 19, 2024

I'm actually a bit confused, because I recently did a batch test against my allocated-refactor branch which does NOT have this fix and it passed without crashing. And the error you're seeing is what I was getting before I made this change on the sample, so I think the sample is more subtle than I suspected at first...

Imo it should be adjusted to create it's own compute command pool, like we do in the other compute related samples.

I initially tried that approach because I noticed that FragmentShadingRateDynamic::build_command_buffers resets this command pool, which invalidates any of the compute buffers allocated from it, but that didn't seem to help. That's also where my investigation with git bisect led me.

I'm going to give that another shot and test it again checking both release and debug modes this time.

@jherico
Copy link
Contributor Author

jherico commented Feb 20, 2024

OK, so using a dedicated compute queue for the compute operations would require significant refactoring of the example. While the acquire and release barriers in the compute command buffers are pretty straight-forward, it's not immediately clear to me where they'd go on the graphics side. It looks like the acquire would have to go in the draw_cmd_buffers but the release would have to go in the small_command_buffers, I think. At any rate, I'm going to just try to resolve this with a unified queue for now.

@jherico jherico marked this pull request as draft February 22, 2024 20:03
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.

fragment_shading_rate_dynamic crashes
2 participants