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

Add vulkan error checks in command_queue_execute_and_present #98883

Merged
merged 1 commit into from
Nov 10, 2024

Conversation

ducklin5
Copy link
Contributor

@ducklin5 ducklin5 commented Nov 6, 2024

This PR adds error checks for all possible errors that can arise from vkQueuePresentKHR

Currently when device_functions.QueuePresentKHR() returns an error, the error message looks like this:

ERROR: Condition "err != VK_SUCCESS && err != VK_SUBOPTIMAL_KHR" is true. Returning: FAILED
   at: command_queue_execute_and_present (drivers/vulkan/rendering_device_driver_vulkan.cpp:2344)

According to the vulkan docs (here): vkQueuePresentKHR returns one of these error values:

On success, this command returns:

    VK_SUCCESS
    VK_SUBOPTIMAL_KHR

On failure, this command returns:

    VK_ERROR_OUT_OF_HOST_MEMORY
    VK_ERROR_OUT_OF_DEVICE_MEMORY
    VK_ERROR_DEVICE_LOST
    VK_ERROR_OUT_OF_DATE_KHR
    VK_ERROR_SURFACE_LOST_KHR
    VK_ERROR_FULL_SCREEN_EXCLUSIVE_MODE_LOST_EXT

Note: VK_ERROR_OUT_OF_DATE_KHR is already handled in command_queue_execute_and_present.
So the error has to be one of these:

VK_ERROR_OUT_OF_HOST_MEMORY
VK_ERROR_OUT_OF_DEVICE_MEMORY
VK_ERROR_DEVICE_LOST
VK_ERROR_SURFACE_LOST_KHR
VK_ERROR_FULL_SCREEN_EXCLUSIVE_MODE_LOST_EXT

Unfortunately there's no way to know which error it is from the editor.
This PR ensures that the error message includes the error type to allow for further troubleshooting.

Comment on lines 2614 to 2619
ERR_FAIL_COND_V(err == VK_ERROR_OUT_OF_HOST_MEMORY, FAILED);
ERR_FAIL_COND_V(err == VK_ERROR_OUT_OF_DEVICE_MEMORY, FAILED);
ERR_FAIL_COND_V(err == VK_ERROR_DEVICE_LOST, FAILED);
ERR_FAIL_COND_V(err == VK_ERROR_SURFACE_LOST_KHR, FAILED);
ERR_FAIL_COND_V(err == VK_ERROR_FULL_SCREEN_EXCLUSIVE_MODE_LOST_EXT, FAILED);
ERR_FAIL_COND_V(err != VK_SUCCESS && err != VK_SUBOPTIMAL_KHR, FAILED);
Copy link
Member

Choose a reason for hiding this comment

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

Instead of checking every condition it would be best to do the following:

  1. print a string using ERR_FAIL_COND_V_MSG()
  2. Create a function that takes in a VkResult and returns a string. The implementation of the function can be hidden behind an #ifdef DEBUG so that the full error handling is only done for debug builds. we use this pattern in a few places in the source (here for example:
    inline String TextureStorage::get_framebuffer_error(GLenum p_status) {
    )

@Chaosus Chaosus added this to the 4.4 milestone Nov 6, 2024
@fire fire changed the title fix: add vulkan error checks in command_queue_execute_and_present Add vulkan error checks in command_queue_execute_and_present Nov 6, 2024
@ducklin5
Copy link
Contributor Author

ducklin5 commented Nov 6, 2024

After adding this messaging and running it on my project. I found out that I'm getting a VK_ERROR_DEVICE_LOST error for my particular case. (I think this is because my gdextension is stealing the device by calling vkCreateDevice for tensor processing... still not sure yet 🤷‍♂️ )

Anyway, I also noticed that command_queue_execute_and_present is handling the VK_ERROR_DEVICE_LOST error case for a previous call to vkQueueSubmit:

if (err == VK_ERROR_DEVICE_LOST) {

I'm just wondering if we should be doing the same thing after the call to device_functions.QueuePresentKHR

@clayjohn
Copy link
Member

clayjohn commented Nov 6, 2024

I'm just wondering if we should be doing the same thing after the call to device_functions.QueuePresentKHR

I don't think so, that branch there is a special debug section to help us find out what was the last command in the command queue that ran before things crashed. In this case, we know the error came from QueuePresentKHR, so we don't need more information

@clayjohn
Copy link
Member

clayjohn commented Nov 6, 2024

It looks like this PR is failing on style checks. Here are our docs on our code style guidelines https://docs.godotengine.org/en/latest/contributing/development/code_style_guidelines.html#c-and-objective-c

Once you make those changes you will need to squash your commits together, how to do that is described here: https://docs.godotengine.org/en/latest/contributing/workflow/pr_workflow.html#the-interactive-rebase

@ducklin5
Copy link
Contributor Author

ducklin5 commented Nov 6, 2024

Oh shoot, I assumed the PR would be squashed with the merge. Okay, I'll squash my commits. Thank you

fix: add debug helper functoin and update messaging vulkan result
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thank you for making such quick changes

@ducklin5
Copy link
Contributor Author

ducklin5 commented Nov 6, 2024

Thanks for the quick reviews!

@Repiteo Repiteo merged commit bbde4ed into godotengine:master Nov 10, 2024
21 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 10, 2024

Thanks! Congratulations on your first contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants