add VK_KHR_maintenance5 support #3251

qbojj · 2024-02-14T21:22:16Z

Description

This PR adds VK_KHR_maintenance5 support. (resolve #3244)

Spec: https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VK_KHR_maintenance5.html
Proposal: https://github.com/KhronosGroup/Vulkan-Docs/blob/main/proposals/VK_KHR_maintenance5.adoc

Here are features that are added in VK_KHR_maintenance5 and this PR should support (features that I am unsure of are not set):

qbojj · 2024-02-14T22:57:44Z

There is some things I do not know where to implement:

interaction of robustness and index buffer (size is not VK_WHOLE_SIZE):
- mesh viewer should show index=0 if robustness is enabled and is dereferencing beyond bound buffer
default size of PointSize (didn't look at shader debugging yet)

Also the point

vkGetDeviceProcAddr must return NULL for supported core functions beyond the version requested by the application.

is implemented as somewhat of a hack (though it makes this feature zero-maintainable).

I could change it to make version annotations in the HookInitVulkanDevice macro.

baldurk · 2024-02-19T13:05:50Z

Can you clarify how you want to handle this PR?

Do you want this partial support as-is reviewed and merged while you investigate the remaining issues, and then you'll PR those and enable the extension? Or are you wanting to continue working on this and complete it before you get a review?

qbojj · 2024-02-20T00:15:52Z

I have finished all parts of this extension, so this PR is ready for review.

qbojj · 2024-02-20T00:57:51Z

It looks like setting PointSize to 1.0f where I tried doesn't work... IDK how to do it

qbojj · 2024-02-20T06:18:57Z

I would like it merged even if partially.

qbojj · 2024-02-21T00:20:42Z

Default 1.0f for PointSize now works (everything is implemented).

baldurk · 2024-02-22T12:04:39Z

Just checking - is this PR ready for review? I've noticed you pushed and commented several times since you said it was complete. If you'd rather wait if you're still working on it, please mark it as draft or close and re-open it.

qbojj · 2024-02-22T19:05:27Z

Yes, those were some code style changes. Sorry for the confusion.

baldurk

I've left some notes on some things, but this PR also seems to be missing some parts - at least one is crucial.

There's no handling of the new structures defined in the extension, the standard features struct but also the new extended usage and pipeline create flags structs which must be used in preference to the normal enums if present. These structs aren't serialised or handled in next chains. I would also expect to see new handling for the new format enums.

Most importantly though the extension isn't whitelisted, so no application will be able to use it. Maybe that's deliberate if this is intended to be unfinished/unused, but you said the PR and extension support was complete so I'm assuming it's an omission.

On the last point, how did you test this code and the extension? Did you allow use of it locally and then remove that for this PR? I'm a little unclear on what the status of this is still.

qrenderdoc/Windows/PipelineState/VulkanPipelineStateViewer.cpp

renderdoc/driver/shaders/spirv/spirv_debug_setup.cpp

renderdoc/driver/vulkan/vk_core.h

baldurk · 2024-02-26T13:11:00Z

renderdoc/driver/vulkan/vk_layer.cpp

+  // if GPA returns NULL, that means that the standard prohibits non-NULL values for this pName
+  //   or that the return value is undefined
+  if(!gpa || !gpa(Unwrap(device), pName))
+    return NULL;


This change seems wrong to me as we should not rely on upstream layers/loader to define our correctness. Aside from that the loader interface document says that layers must not call down the chain inside vkGetDeviceProcAddr.

What is the problem you're trying to solve here? The behaviour difference defined in KHR_maintenance5 should already be correctly handled by RenderDoc.

changed it so functions explicitly say in which version of vulkan they were integrated into core

Version gating is already handled per extension by the condition, so I still don't see the purpose of this code change.

I asked above what the problem is that you're trying to solve, and you've not answered that question. Changing the code without answering the question or explaining yourself doesn't address this feedback.

I'm sorry. It is just if for example VK_KHR_dynamic_rendering is enabled with requested instance version of 1.2. RenderDoc will resolve vkCmdBeginRendering but it was not core in version 1.3 so it should be NULL. (https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/vkGetDeviceProcAddr.html 2nd annotation of Table 1.)

Ah OK I see what you mean. That's not really the problem case that the spec was clarifying, but you're right in that technically speaking this is wrong according to the letter of the spec although it shouldn't cause any problems in practice.

You don't need to manually annotate every function like this, it can be handled with the existing CheckExt macros which already annotate promotions. It just needs to be changed to not merge the check down to a single condition.

renderdoc/driver/vulkan/vk_overlay.cpp

renderdoc/driver/vulkan/wrappers/vk_cmd_funcs.cpp

qbojj · 2024-02-26T20:04:27Z

There's no handling of the new structures defined in the extension, the standard features struct but also the new extended usage and pipeline create flags structs which must be used in preference to the normal enums if present. These structs aren't serialised or handled in next chains. I would also expect to see new handling for the new format enums.

Oh... as I am not using this part of the extension I hadn't noticed the lack of support. I will look into it.

Most importantly though the extension isn't whitelisted, so no application will be able to use it. Maybe that's deliberate if this is intended to be unfinished/unused, but you said the PR and extension support was complete so I'm assuming it's an omission.

On the last point, how did you test this code and the extension? Did you allow use of it locally and then remove that for this PR? I'm a little unclear on what the status of this is still.

Strange. When I build it locally and run my program that enables this extension it works just fine (it enumerates the extension and enabling it works). I do test it with my program, but It doesn't use all of the features. I should probably make a test program that uses everything from this extension.

Anyways I have changed the parts You have commented upon but am yet to add support for new usage/flags, so when that will be done I will message You.

baldurk · 2024-02-26T21:42:34Z

Strange. When I build it locally and run my program that enables this extension it works just fine (it enumerates the extension and enabling it works). I do test it with my program, but It doesn't use all of the features. I should probably make a test program that uses everything from this extension.

I don't see any way that that is possible, unless you have local changes to RenderDoc that are not present in the PR here. Not only will RenderDoc only enumerate supported device extensions, but at vkCreateDevice time RenderDoc will check to see that all enabled extensions are supported and it will fail device creation if an unsupported device extension is passed.

The only possibilities I can see is that your application does not hard-require KHR_maintenance5 and when run under RenderDoc it is running the same as it would on any driver that didn't support the extension, or else it is not written legally and is relying on/using functionality from the extension without either enabling it or passing the feature struct.

qbojj · 2024-02-27T00:10:54Z

my bad. I forgot to uncomment the extension string, but it was still passing the features struct to create device. Now I have added it to whitelisted extensions.

qbojj · 2024-03-01T22:01:06Z

I have since tested this PR and found one problem with it (I am not sure how should I fix those):

https://github.com/qbojj/vk_khr_maintenance5_test/tree/master
When robustBufferAccess2 is enabled and index is used outside of bound region the mesh viewer shows a --- at the IDX column (this is expected), but the shader input columns are also written as --- (those should be pulled from index=0). Shader debugger pulls data as expected.

Also https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/vkGetDeviceProcAddr.html states that it should resolve (for a valid device handle) only for: "requested core version device-level dispatchable commands" and
"enabled extension device-level dispatchable commands". I have handled the first part, but from my understanding the second part doesn't include promoted extensions from core (only explicitly enabled extensions should be reported) (that it what mesa/radv follows).

On the other hand it looks like vertex buffer robustness is not working (even when not using it on the index buffer so unrelated to this PR):
https://github.com/qbojj/vk_khr_maintenance5_test/tree/bad-robustness
The state of the command vkBindVertexBuffers2(EXT) is not properly restored during the replay (see the call to vkDrawIndexed). Also when debugging the vertex shader instance that uses out-of-bounds vertex the input values are initialized as-if they were in-bounds (robustness2 should make them zero). Also runtime errors are thrown when OOB vertex is accessed even with robustness2 enabled.

renderdoc/driver/vulkan/vk_core.cpp

renderdoc/driver/vulkan/vk_info.cpp

renderdoc/driver/vulkan/vk_next_chains.cpp

renderdoc/driver/vulkan/vk_serialise.cpp

renderdoc/driver/vulkan/vk_shader_cache.cpp

renderdoc/driver/vulkan/vk_stringise.cpp

baldurk · 2024-03-05T16:32:11Z

As a heads up for you, once the review process has completed I'm going to keep this PR on hold until I can find the time to test this and make sure everything is working solidly.

baldurk · 2024-03-07T00:21:47Z

If you want to keep prototyping or iterating on this code that's fine but can you please either close this PR or do all of the work locally, make sure you are satisfied with it, and only push when it is finalised and ready for review? Having an ever moving target is not easy to know that it's ready for review and getting email notification spam is not very pleasant either.

qbojj · 2024-03-07T09:02:13Z

It is just that I have found a bug in my code, so I fixed it. Other than that I am satisfied with the state of this PR

qbojj · 2024-05-12T22:58:59Z

Rebased to resolve conflicts

qbojj force-pushed the v1.x branch 2 times, most recently from a49e3d1 to e63c3b7 Compare February 14, 2024 22:40

qbojj force-pushed the v1.x branch 11 times, most recently from 685bde6 to da299d4 Compare February 19, 2024 23:52

qbojj force-pushed the v1.x branch from da299d4 to fe203eb Compare February 21, 2024 00:19

qbojj force-pushed the v1.x branch 2 times, most recently from 96aa7e4 to 47b5ade Compare February 21, 2024 10:08

baldurk requested changes Feb 26, 2024

View reviewed changes

qbojj force-pushed the v1.x branch from 47b5ade to 4681b9b Compare February 26, 2024 19:56

qbojj force-pushed the v1.x branch from 4681b9b to 30d345a Compare February 26, 2024 20:08

qbojj force-pushed the v1.x branch 3 times, most recently from cab9fc3 to ad597cb Compare February 27, 2024 13:23

qbojj marked this pull request as draft February 27, 2024 18:59

qbojj force-pushed the v1.x branch 3 times, most recently from 70aaf3f to 21710b9 Compare February 28, 2024 02:24

qbojj marked this pull request as ready for review March 1, 2024 22:01

baldurk requested changes Mar 5, 2024

View reviewed changes

qbojj force-pushed the v1.x branch 6 times, most recently from 071e612 to 9833b87 Compare March 7, 2024 00:11

qbojj force-pushed the v1.x branch 2 times, most recently from 98067ab to a11b81f Compare May 12, 2024 22:47

add VK_KHR_maintenance5 support

897eda8

qbojj force-pushed the v1.x branch from a11b81f to 897eda8 Compare May 12, 2024 22:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add VK_KHR_maintenance5 support #3251

add VK_KHR_maintenance5 support #3251

qbojj commented Feb 14, 2024 •

edited

qbojj commented Feb 14, 2024 •

edited

baldurk commented Feb 19, 2024

qbojj commented Feb 20, 2024

qbojj commented Feb 20, 2024

qbojj commented Feb 20, 2024

qbojj commented Feb 21, 2024 •

edited

baldurk commented Feb 22, 2024

qbojj commented Feb 22, 2024 •

edited

baldurk left a comment

baldurk Feb 26, 2024

qbojj Feb 26, 2024

baldurk Feb 26, 2024

qbojj Feb 27, 2024 •

edited

baldurk Mar 5, 2024

qbojj commented Feb 26, 2024 •

edited

baldurk commented Feb 26, 2024

qbojj commented Feb 27, 2024 •

edited

qbojj commented Mar 1, 2024 •

edited

baldurk commented Mar 5, 2024

baldurk commented Mar 7, 2024

qbojj commented Mar 7, 2024

qbojj commented May 12, 2024

add VK_KHR_maintenance5 support #3251

Are you sure you want to change the base?

add VK_KHR_maintenance5 support #3251

Conversation

qbojj commented Feb 14, 2024 • edited

Description

qbojj commented Feb 14, 2024 • edited

baldurk commented Feb 19, 2024

qbojj commented Feb 20, 2024

qbojj commented Feb 20, 2024

qbojj commented Feb 20, 2024

qbojj commented Feb 21, 2024 • edited

baldurk commented Feb 22, 2024

qbojj commented Feb 22, 2024 • edited

baldurk left a comment

Choose a reason for hiding this comment

baldurk Feb 26, 2024

Choose a reason for hiding this comment

qbojj Feb 26, 2024

Choose a reason for hiding this comment

baldurk Feb 26, 2024

Choose a reason for hiding this comment

qbojj Feb 27, 2024 • edited

Choose a reason for hiding this comment

baldurk Mar 5, 2024

Choose a reason for hiding this comment

qbojj commented Feb 26, 2024 • edited

baldurk commented Feb 26, 2024

qbojj commented Feb 27, 2024 • edited

qbojj commented Mar 1, 2024 • edited

baldurk commented Mar 5, 2024

baldurk commented Mar 7, 2024

qbojj commented Mar 7, 2024

qbojj commented May 12, 2024

qbojj commented Feb 14, 2024 •

edited

qbojj commented Feb 14, 2024 •

edited

qbojj commented Feb 21, 2024 •

edited

qbojj commented Feb 22, 2024 •

edited

qbojj Feb 27, 2024 •

edited

qbojj commented Feb 26, 2024 •

edited

qbojj commented Feb 27, 2024 •

edited

qbojj commented Mar 1, 2024 •

edited