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

Khronos validation layer #84

Closed
wants to merge 9 commits into from
Closed

Khronos validation layer #84

wants to merge 9 commits into from

Conversation

simonbuchan
Copy link

@simonbuchan simonbuchan commented Sep 20, 2020 •
edited
Loading

Wanted to get validation going, but from what I can tell, the LUNARG_standard_validation is no longer included in the 1.2 SDK. The new hotness KHRONOS_validation is an easy switch, and has a much more descriptive and helpful message by default. The fancy thing here would be to try in order, but that seems to be a bit too fancy for something that's commented out by default. (Perhaps #define VKPT_ENABLE_VALIDATION KHRONOS to get the new layer, if that's wanted?)

This makes booting into a map clean, but there's missing deletes for samplers on quit still, and it's possible there are other paths that are not exercised (cinematics? glass/water etc?)

Suggestions welcome on what to test and any style / etc. things I missed (the malloc() usage in particular)

@simonbuchan
Switch validation layer to standard KHRONOS_validation.
53edbb8 
Has much more verbose message now, don't need additional logging.
@simonbuchan
Add additional debugging information for existing validation errors.
f2a21da
@simonbuchan
Correct pipeline layouts in asvgf.c.
938b8d5
@simonbuchan
Enable indexing feature used by shaders.
96c3466
@simonbuchan
Align RT shader group handles on reported base alignment.
06de60d 
On my RTX 2070 SUPER HandleSize is 32, while BaseAlignment is 64.
It still works on this driver, but it is a spec error to use
unaligned shader binding offsets in CmdTraceRays.

Unfortunately this means we have to expand the handles out into
an aligned table, as far as I could tell from the usage.
@simonbuchan
Fix texture DESTROY_LATENCY.
b2fbbfa 
The curse of macros.
@simonbuchan
Add CMake CONFIG_VKPT_ENABLE_VALIDATION
72d2b07 
To avoid needing to comment the define in/out.
@simonbuchan
While I'm in here, fix /MT(d) flag messages.
34e3c84 
Split out so it can be dropped if old CMake support is needed for some reason.
@simonbuchan simonbuchan marked this pull request as ready for review September 22, 2020 11:33
@simonbuchan
Copy link
Author

simonbuchan commented Sep 22, 2020 •
edited
Loading

Can't reproduce sampler error again (argh!), but did find an issue with the unused texture cleanup (due to the DESTROY_LATENCY macro expansion it would only have two frames). I only saw this with the cinematics, which mark the last frame for unload immediately, but in theory could reproduce elsewhere if the user did something weird, I guess.

@simonbuchan
Copy link
Author

simonbuchan commented Sep 22, 2020 •
edited
Loading

Ah the god rays shadow map sampler was leaked on swapchain recreation (eg. going into / out of windowed).

I realized I haven't shown the new validation messages, so here's an example:

Validation Error: [ VUID-vkDestroyDevice-device-00378 ] Object 0: handle = 0x16935ce7890, type = VK_OBJECT_TYPE_DEVICE; Object 1: handle = 0x4176e9000000037d, name = god_rays.shadow_sampler, type = VK_OBJECT_TYPE_SAMPLER; | MessageID = 0x71500fba | OBJ ERROR : For VkDevice 0x16935ce7890[], VkSampler 0x4176e9000000037d[god_rays.shadow_sampler] has not been destroyed. The Vulkan spec states: All child objects created on device must have been destroyed prior to destroying device (https://vulkan.lunarg.com/doc/view/1.2.141.0/windows/1.2-extensions/vkspec.html#VUID-vkDestroyDevice-device-00378)

@simonbuchan
Copy link
Author

The last issue I've seen is warnings when windows reports a size of 0x0, which happens when the window is minimized. This one is a bit annoying as SDL_GetWindowSize() is lagging behind vkGetSurfaceCapabilities(), and I'm not sure I get all the logic in create_swapchain() enough right now.

@apanteleev
Copy link
Collaborator

The validation layer has been updated, and many errors and warnings fixed with the release 1.4.0. Closing this PR, please open a new one if there are any similar issues in the new version.

@apanteleev apanteleev closed this Dec 15, 2020
@simonbuchan
Copy link
Author

Sounds good!

@simonbuchan simonbuchan deleted the khronos_validation branch December 15, 2020 21:13
@simonbuchan
Copy link
Author

Only thing obvious in a quick look over of the diff on my phone is the texture.c DESTROY_LATENCY is still not parenthesised and thus expands to an incorrect index: it seems this was fixed some other way, but perhaps it's something to check?

@apanteleev
Copy link
Collaborator

DESTROY_LATENCY is still not parenthesised

Wow, how did I miss that. Will fix soon, thank you!

apanteleev added a commit that referenced this pull request Dec 17, 2020
@apanteleev
Added parentheses around the DESTROY_LATENCY macro, as noted in #84
7940f07
@apanteleev apanteleev mentioned this pull request Dec 17, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@simonbuchan @apanteleev