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

RayTraversalPrimitiveCullingKHR capability not allowed in ray _query_ shaders #4278

Closed
MarijnS95 opened this issue Jul 6, 2022 · 13 comments
Closed

Comments

@MarijnS95
Copy link
Contributor

Describe the Issue

When using a shader with ray queries, that have SkipAABBsKHR set (RAY_FLAG_SKIP_PROCEDURAL_PRIMITIVES in HLSL) and its RayTraversalPrimitiveCullingKHR emitted in SPIR-V, the following error appears:

Validation Error: [ VUID-VkShaderModuleCreateInfo-pCode-01091 ] Object 0: handle = 0xb400007394d724d0, type = VK_OBJECT_TYPE_DEVICE; | MessageID = 0xa7bb8db6 | vkCreateShaderModule(): The SPIR-V Capability (RayTraversalPrimitiveCullingKHR) was declared, but none of the requirements were met to use it. The Vulkan spec states: If pCode declares any of the capabilities listed in the SPIR-V Environment appendix, one of the corresponding requirements must be satisfied (https://www.khronos.org/registry/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-VkShaderModuleCreateInfo-pCode-01091)

Note that this appears when exclusively using VK_KHR_ray_query and not enabling/having VK_KHR_ray_tracing_pipeline - hence not enabling the flag in VkPhysicalDeviceRayTracingPipelineFeaturesKHR.

It appears the spec and this validation layer only have a Vulkan feature for Ray Tracing pipelines, but not for Ray Query shaders?

A possible workaround is passing VkPhysicalDeviceRayTracingPipelineFeaturesKHR regardless of the VK_KHR_ray_tracing_pipeline feature, but that seems wrong and I'm surprised the validation layer doesn't complain about feature structs being used without the appropriate extension being enabled.

Valid Usage ID

VUID-VkShaderModuleCreateInfo-pCode-01091

Environment

  • OS: Any
  • GPU: Any with Ray Query support
  • SDK or header version if building from repo: 1.3.216.0
  • Options enabled (synchronization, best practices, etc.): None.

Additional context

@sjfricke
Copy link
Contributor

sjfricke commented Jul 6, 2022

This is a spec bug (Vulkan-Docs) because all the validation layers stuff (and the table in the spec) are generated from the vk.xml file

        <spirvcapability name="RayTraversalPrimitiveCullingKHR">
            <enable struct="VkPhysicalDeviceRayTracingPipelineFeaturesKHR" feature="rayTraversalPrimitiveCulling" requires="VK_KHR_ray_tracing_pipeline"/>
        </spirvcapability>

I do agree looking at this now, it seems to be a mistake that there is no way to enable RayTraversalPrimitiveCullingKHR with only VK_KHR_ray_query

Also it does say in the SPV_KHR_ray_query spec

can be enabled by either extension’s capability

@MarijnS95
Copy link
Contributor Author

@sjfricke Thanks for confirming that it should indeed be usable on SPV_KHR_ray_query; I should have clarified that I was also on the verge of punting this on a spec bug as well.

Will file this upstream :)

@dgkoch
Copy link

dgkoch commented Jul 7, 2022

RayTraversalPrimitiveCullingKHR is optional for VK_KHR_ray_tracing_pipeline, but required for VK_KHR_ray_query, which is why there is no feature flag in VK_KHR_ray_query. I think this bug came about when we tried to represent all the capabilities in the XML.

@sjfricke
Copy link
Contributor

sjfricke commented Jul 7, 2022

but required for VK_KHR_ray_query

then the simple fix is just to add a line to the XML

        <spirvcapability name="RayTraversalPrimitiveCullingKHR">
            <enable struct="VkPhysicalDeviceRayTracingPipelineFeaturesKHR" feature="rayTraversalPrimitiveCulling" requires="VK_KHR_ray_tracing_pipeline"/>
+           <enable extension="VK_KHR_ray_query"/>
        </spirvcapability>

and everything will just work (trivial fix, can make PR or if @dgkoch wants to do it internally if it is faster)

@dgkoch
Copy link

dgkoch commented Jul 7, 2022

I can do it

@dgkoch
Copy link

dgkoch commented Jul 7, 2022

  <spirvcapability name="RayTraversalPrimitiveCullingKHR">
       <enable struct="VkPhysicalDeviceRayTracingPipelineFeaturesKHR" feature="rayTraversalPrimitiveCulling" requires="VK_KHR_ray_tracing_pipeline"/>
       <enable extension="VK_KHR_ray_query"/>
    </spirvcapability>

@sjfricke Actually we need the rayQuery feature to be enabled as well, right?

@MarijnS95
Copy link
Contributor Author

@dgkoch Isn't the same implied for VkPhysicalDeviceRayTracingPipelineFeaturesKHR's rayTracingPipeline boolean?

@sjfricke
Copy link
Contributor

sjfricke commented Jul 7, 2022

Actually we need the rayQuery feature to be enabled as well, right?

yes so it would be

        <spirvcapability name="RayTraversalPrimitiveCullingKHR">
            <enable struct="VkPhysicalDeviceRayTracingPipelineFeaturesKHR" feature="rayTraversalPrimitiveCulling" requires="VK_KHR_ray_tracing_pipeline"/>
+           <enable struct="VkPhysicalDeviceRayQueryFeaturesKHR" feature="rayQuery" requires="VK_KHR_ray_query"/>
        </spirvcapability>

@MarijnS95
Copy link
Contributor Author

Just wondering, if VkPhysicalDeviceRayTracingPipelineFeaturesKHR::rayTraversalPrimitiveCulling is enabled does that require VkPhysicalDeviceRayTracingPipelineFeaturesKHR::rayTracingPipeline to be enabled accordingly?

@dgkoch
Copy link

dgkoch commented Jul 8, 2022

Just wondering, if VkPhysicalDeviceRayTracingPipelineFeaturesKHR::rayTraversalPrimitiveCulling is enabled does that require VkPhysicalDeviceRayTracingPipelineFeaturesKHR::rayTracingPipeline to be enabled accordingly?

I don't think we technically have anything that explicitly says that rayTracingPipeline must be enabled to use rayTraversalPrimitiveCulling, but there's nothing useful you can do with it if you don't have rayTracingPipeline enabled.

@dgkoch
Copy link

dgkoch commented Jul 15, 2022

Spec fix released in VK 1.3.221

@sjfricke
Copy link
Contributor

Pulled in and should be good now https://github.com/KhronosGroup/Vulkan-ValidationLayers/pull/4314/files#diff-7d48a2b188a782c63658a6716d3b99ded72d38c774bbc647272646552d4e198e

@ncesario-lunarg we can close this issue now

@MarijnS95
Copy link
Contributor Author

Thanks a lot! I'll test this out in the coming days.

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

No branches or pull requests

3 participants