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 VK_KHR_maintenance5 support #3251

Open
wants to merge 1 commit into
base: v1.x
Choose a base branch
from
Open

add VK_KHR_maintenance5 support #3251

wants to merge 1 commit into from

Conversation

qbojj
Copy link

@qbojj qbojj commented Feb 14, 2024

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):

  • A new VK_FORMAT_A1B5G5R5_UNORM_PACK16_KHR format
  • A new VK_FORMAT_A8_UNORM_KHR format
  • A property to indicate that multisample coverage operations are performed after sample counting in EarlyFragmentTests mode
  • Relax VkBufferView creation requirements by allowing subsets of the associated VkBuffer usage using VkBufferUsageFlags2CreateInfoKHR
  • A new entry point vkCmdBindIndexBuffer2KHR, allowing a range of memory to be bound as an index buffer
  • vkGetDeviceProcAddr must return NULL for supported core functions beyond the version requested by the application.
  • A property to indicate that the sample mask test is performed after sample counting in EarlyFragmentTests mode
  • vkCmdBindVertexBuffers2 now supports using VK_WHOLE_SIZE in the pSizes parameter.
  • A default size of 1.0 is used if PointSize is not written
  • Shader modules are deprecated - applications can now pass VkShaderModuleCreateInfo as a chained struct to pipeline creation via VkPipelineShaderStageCreateInfo
  • A function vkGetRenderingAreaGranularityKHR to query the optimal render area for a dynamic rendering instance.
  • A property to indicate that depth/stencil texturing operations with VK_COMPONENT_SWIZZLE_ONE have defined behavior
  • Add vkGetImageSubresourceLayout2KHR and a new function vkGetDeviceImageSubresourceLayoutKHR to allow the application to query the image memory layout without having to create an image object and query it.
  • Allow VK_REMAINING_ARRAY_LAYERS as the layerCount member of VkImageSubresourceLayers
  • Adds stronger guarantees for propagation of VK_ERROR_DEVICE_LOST return values
  • A property to indicate whether PointSize controls the final rasterization of polygons if polygon mode is VK_POLYGON_MODE_POINT
  • Two properties to indicate the non-strict line rasterization algorithm used
  • Two new flags words VkPipelineCreateFlagBits2KHR and VkBufferUsageFlagBits2KHR
  • Physical-device-level functions can now be called with any value in the valid range for a type beyond the defined enumerants, such that applications can avoid checking individual features, extensions, or versions before querying supported properties of a particular enumerant.
  • Clarification that copies between images of any type are allowed, treating 1D images as 2D images with a height of 1.

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

qbojj commented Feb 14, 2024

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
Copy link
Owner

baldurk commented Feb 19, 2024

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 qbojj force-pushed the v1.x branch 11 times, most recently from 685bde6 to da299d4 Compare February 19, 2024 23:52
@qbojj
Copy link
Author

qbojj commented Feb 20, 2024

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

@qbojj
Copy link
Author

qbojj commented Feb 20, 2024

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

@qbojj
Copy link
Author

qbojj commented Feb 20, 2024

I would like it merged even if partially.

@qbojj
Copy link
Author

qbojj commented Feb 21, 2024

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

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

baldurk commented Feb 22, 2024

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
Copy link
Author

qbojj commented Feb 22, 2024

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

Copy link
Owner

@baldurk baldurk left a comment

Choose a reason for hiding this comment

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

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.

renderdoc/driver/shaders/spirv/spirv_debug_setup.cpp Outdated Show resolved Hide resolved
renderdoc/driver/vulkan/vk_core.h Outdated Show resolved Hide resolved
// 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;
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Author

@qbojj qbojj Feb 27, 2024

Choose a reason for hiding this comment

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

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.)

Copy link
Owner

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
@qbojj
Copy link
Author

qbojj commented Feb 26, 2024

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
Copy link
Owner

baldurk commented Feb 26, 2024

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
Copy link
Author

qbojj commented Feb 27, 2024

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
Copy link
Author

qbojj commented Mar 1, 2024

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.

@qbojj qbojj marked this pull request as ready for review March 1, 2024 22:01
renderdoc/driver/vulkan/vk_core.cpp Outdated Show resolved Hide resolved
renderdoc/driver/vulkan/vk_info.cpp Outdated Show resolved Hide resolved
renderdoc/driver/vulkan/vk_next_chains.cpp Outdated Show resolved Hide resolved
renderdoc/driver/vulkan/vk_serialise.cpp Show resolved Hide resolved
renderdoc/driver/vulkan/vk_serialise.cpp Show resolved Hide resolved
renderdoc/driver/vulkan/vk_shader_cache.cpp Outdated Show resolved Hide resolved
renderdoc/driver/vulkan/vk_stringise.cpp Outdated Show resolved Hide resolved
@baldurk
Copy link
Owner

baldurk commented Mar 5, 2024

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.

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

baldurk commented Mar 7, 2024

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
Copy link
Author

qbojj commented Mar 7, 2024

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 qbojj force-pushed the v1.x branch 3 times, most recently from a11b81f to 897eda8 Compare May 12, 2024 22:51
@qbojj
Copy link
Author

qbojj commented May 12, 2024

Rebased to resolve conflicts

@qbojj qbojj marked this pull request as draft June 25, 2024 13:36
@qbojj qbojj force-pushed the v1.x branch 2 times, most recently from eb7c362 to 11ca89f Compare June 25, 2024 13:46
@qbojj qbojj marked this pull request as ready for review June 25, 2024 14:19
@qbojj qbojj requested a review from baldurk June 25, 2024 14:23
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.

support for VK_KHR_maintenance5
2 participants