video: adjust device_limits/features to the updated profiles #829
Conversation
Signed-off-by: Ali Cheraghi <[email protected]>
Signed-off-by: Ali Cheraghi <[email protected]>
Signed-off-by: Ali Cheraghi <[email protected]>
| if (!vulkan12Properties.shaderSignedZeroInfNanPreserveFloat64) | ||
| return nullptr; | ||
| if (!vulkan12Properties.shaderDenormPreserveFloat16) | ||
| return nullptr; | ||
| if (!vulkan12Properties.shaderDenormPreserveFloat32) | ||
| return nullptr; | ||
| if (!vulkan12Properties.shaderDenormPreserveFloat64) | ||
| return nullptr; | ||
| if (!vulkan12Properties.shaderDenormFlushToZeroFloat16) | ||
| return nullptr; | ||
| if (!vulkan12Properties.shaderDenormFlushToZeroFloat32) | ||
| return nullptr; | ||
| if (!vulkan12Properties.shaderDenormFlushToZeroFloat64) | ||
| return nullptr; | ||
| if (!vulkan12Properties.shaderRoundingModeRTEFloat16) | ||
| return nullptr; | ||
| if (!vulkan12Properties.shaderRoundingModeRTEFloat32) | ||
| return nullptr; | ||
| if (!vulkan12Properties.shaderRoundingModeRTEFloat64) | ||
| return nullptr; | ||
| if (!vulkan12Properties.shaderRoundingModeRTZFloat16) | ||
| return nullptr; | ||
| if (!vulkan12Properties.shaderRoundingModeRTZFloat32) | ||
| return nullptr; | ||
| if (!vulkan12Properties.shaderRoundingModeRTZFloat64) | ||
| return nullptr; |
There was a problem hiding this comment.
the 16 and 32 should only be checked if the float16 and float64 feature is available.
There was a problem hiding this comment.
but properties are declared before features (in a different scope). wanna move them out?
There was a problem hiding this comment.
you want to move the checks of the limits into where the features are already known, yes
| }, | ||
| { | ||
| "type": "bool", | ||
| "name": "shaderSignedZeroInfNanPreserveFloat32", | ||
| "value": true, | ||
| "expose": "DISABLE" | ||
| "expose": "REQUIRE" | ||
| }, | ||
| { | ||
| "type": "bool", | ||
| "name": "shaderSignedZeroInfNanPreserveFloat64", | ||
| "value": false | ||
| "value": true, | ||
| "expose": "REQUIRE" | ||
| }, | ||
| { | ||
| "type": "bool", | ||
| "name": "shaderDenormPreserveFloat16", | ||
| "value": false | ||
| "value": true, | ||
| "expose": "REQUIRE" | ||
| }, | ||
| { | ||
| "type": "bool", | ||
| "name": "shaderDenormPreserveFloat32", | ||
| "value": false | ||
| "value": true, | ||
| "expose": "REQUIRE" | ||
| }, | ||
| { | ||
| "type": "bool", | ||
| "name": "shaderDenormPreserveFloat64", | ||
| "value": false | ||
| "value": true, | ||
| "expose": "REQUIRE" | ||
| }, | ||
| { | ||
| "type": "bool", | ||
| "name": "shaderDenormFlushToZeroFloat16", | ||
| "value": false | ||
| "value": true, | ||
| "expose": "REQUIRE" | ||
| }, | ||
| { | ||
| "type": "bool", | ||
| "name": "shaderDenormFlushToZeroFloat32", | ||
| "value": false | ||
| "value": true, | ||
| "expose": "REQUIRE" | ||
| }, | ||
| { | ||
| "type": "bool", | ||
| "name": "shaderDenormFlushToZeroFloat64", | ||
| "value": false | ||
| "value": true, | ||
| "expose": "REQUIRE" | ||
| }, | ||
| { | ||
| "type": "bool", | ||
| "name": "shaderRoundingModeRTEFloat16", | ||
| "value": false, | ||
| "comment": ["ROADMAP2024 but no good support yet"] | ||
| "value": true, | ||
| "expose": "REQUIRE" | ||
| }, | ||
| { | ||
| "type": "bool", | ||
| "name": "shaderRoundingModeRTEFloat32", | ||
| "value": false, | ||
| "comment": ["ROADMAP2024 but no good support yet"] | ||
| "value": true, | ||
| "expose": "REQUIRE" | ||
| }, | ||
| { | ||
| "type": "bool", | ||
| "name": "shaderRoundingModeRTEFloat64", | ||
| "value": false | ||
| "value": true, | ||
| "expose": "REQUIRE" | ||
| }, | ||
| { | ||
| "type": "bool", | ||
| "name": "shaderRoundingModeRTZFloat16", | ||
| "value": false | ||
| "value": true, | ||
| "expose": "REQUIRE" | ||
| }, | ||
| { | ||
| "type": "bool", | ||
| "name": "shaderRoundingModeRTZFloat32", | ||
| "value": false | ||
| "value": true, | ||
| "expose": "REQUIRE" | ||
| }, | ||
| { | ||
| "type": "bool", | ||
| "name": "shaderRoundingModeRTZFloat64", | ||
| "value": false | ||
| "value": true, | ||
| "expose": "REQUIRE" |
There was a problem hiding this comment.
do not require limits that are contingent on features (in this case shaderFloat16 and shaderFloat32)
Signed-off-by: Ali Cheraghi <[email protected]>
| "type": "bool", | ||
| "name": "shaderSignedZeroInfNanPreserveFloat16", | ||
| "value": true, | ||
| "expose": "DISABLE" | ||
| "value": true | ||
| }, | ||
| { | ||
| "type": "bool", | ||
| "name": "shaderSignedZeroInfNanPreserveFloat32", | ||
| "value": true, | ||
| "expose": "DISABLE" | ||
| "value": true | ||
| }, | ||
| { | ||
| "type": "bool", | ||
| "name": "shaderSignedZeroInfNanPreserveFloat64", | ||
| "value": false | ||
| "value": true | ||
| }, | ||
| { | ||
| "type": "bool", | ||
| "name": "shaderDenormPreserveFloat16", | ||
| "value": false | ||
| "value": true | ||
| }, | ||
| { | ||
| "type": "bool", | ||
| "name": "shaderDenormPreserveFloat32", | ||
| "value": false | ||
| "value": true | ||
| }, | ||
| { | ||
| "type": "bool", | ||
| "name": "shaderDenormPreserveFloat64", | ||
| "value": false | ||
| "value": true | ||
| }, | ||
| { | ||
| "type": "bool", | ||
| "name": "shaderDenormFlushToZeroFloat16", | ||
| "value": false | ||
| "value": true | ||
| }, | ||
| { | ||
| "type": "bool", | ||
| "name": "shaderDenormFlushToZeroFloat32", | ||
| "value": false | ||
| "value": true | ||
| }, | ||
| { | ||
| "type": "bool", | ||
| "name": "shaderDenormFlushToZeroFloat64", | ||
| "value": false | ||
| "value": true | ||
| }, | ||
| { | ||
| "type": "bool", | ||
| "name": "shaderRoundingModeRTEFloat16", | ||
| "value": false, | ||
| "comment": ["ROADMAP2024 but no good support yet"] | ||
| "value": true | ||
| }, | ||
| { | ||
| "type": "bool", | ||
| "name": "shaderRoundingModeRTEFloat32", | ||
| "value": false, | ||
| "comment": ["ROADMAP2024 but no good support yet"] | ||
| "value": true | ||
| }, | ||
| { | ||
| "type": "bool", | ||
| "name": "shaderRoundingModeRTEFloat64", | ||
| "value": false | ||
| "value": true | ||
| }, | ||
| { | ||
| "type": "bool", | ||
| "name": "shaderRoundingModeRTZFloat16", | ||
| "value": false | ||
| "value": true | ||
| }, | ||
| { | ||
| "type": "bool", | ||
| "name": "shaderRoundingModeRTZFloat32", | ||
| "value": false | ||
| "value": true | ||
| }, | ||
| { | ||
| "type": "bool", | ||
| "name": "shaderRoundingModeRTZFloat64", | ||
| "value": false | ||
| "value": true |
There was a problem hiding this comment.
this is a mess and needs to get fixed
There was a problem hiding this comment.
you can only DISABLE and value=true the 32bit ones, rest you need to DISABLE and value=false
| properties.limits.shaderUniformBufferArrayNonUniformIndexingNative = vulkan12Properties.shaderUniformBufferArrayNonUniformIndexingNative; | ||
| properties.limits.shaderSampledImageArrayNonUniformIndexingNative = vulkan12Properties.shaderSampledImageArrayNonUniformIndexingNative; | ||
| properties.limits.shaderStorageBufferArrayNonUniformIndexingNative = vulkan12Properties.shaderStorageBufferArrayNonUniformIndexingNative; | ||
| properties.limits.shaderStorageImageArrayNonUniformIndexingNative = vulkan12Properties.shaderStorageImageArrayNonUniformIndexingNative; |
There was a problem hiding this comment.
where do you check they're not false?
| if (!properties.limits.shaderSignedZeroInfNanPreserveFloat32) | ||
| return nullptr; | ||
| if (!properties.limits.shaderDenormPreserveFloat32) | ||
| return nullptr; | ||
| if (!properties.limits.shaderDenormFlushToZeroFloat32) | ||
| return nullptr; | ||
| if (!properties.limits.shaderRoundingModeRTEFloat32) | ||
| return nullptr; | ||
| if (!properties.limits.shaderRoundingModeRTZFloat32) | ||
| return nullptr; | ||
|
|
||
| if (vulkan12Features.shaderFloat16) { | ||
| if (!properties.limits.shaderSignedZeroInfNanPreserveFloat16) | ||
| return nullptr; | ||
| if (!properties.limits.shaderDenormPreserveFloat16) | ||
| return nullptr; | ||
| if (!properties.limits.shaderDenormFlushToZeroFloat16) | ||
| return nullptr; | ||
| if (!properties.limits.shaderRoundingModeRTEFloat16) | ||
| return nullptr; | ||
| if (!properties.limits.shaderRoundingModeRTZFloat16) | ||
| return nullptr; | ||
| } | ||
|
|
||
| if (deviceFeatures.features.shaderFloat64) { | ||
| if (!properties.limits.shaderSignedZeroInfNanPreserveFloat64) | ||
| return nullptr; | ||
| if (!properties.limits.shaderDenormPreserveFloat64) | ||
| return nullptr; | ||
| if (!properties.limits.shaderDenormFlushToZeroFloat64) | ||
| return nullptr; | ||
| if (!properties.limits.shaderRoundingModeRTEFloat64) | ||
| return nullptr; | ||
| if (!properties.limits.shaderRoundingModeRTZFloat64) | ||
| return nullptr; | ||
| } |
There was a problem hiding this comment.
if you don't expose them, then you need to move up the Vk Structs and check from the VkStructs, not our own limits struct
There was a problem hiding this comment.
so query vk features in Get physical device's limits/properties scope?
There was a problem hiding this comment.
for a limit thats required pending a feature, don't do Vulkan Limits -> Nabla Limit to then check Nabla Feature Predicate && not Nabla Limit do Nabla Feature && ! Vulkan Limit
because keeping it as is, would require you to expose a limit to Nabla which has no business being exposed (i.e. we can assume the limit is fullfilled if we have the feature).
Example in plain english, if you have 16 bit floats, you can acount on having float controls for float16 type ops. If you don't then whether you would have had controls is irrelevant because you can't use 16bit floats.
There was a problem hiding this comment.
by "Nabla feature", you mean features defined in device_features.json? shaderFloat16/64 are not defined there but in device_limit.json so they are a limit
| // Enable maintenance5 and graphics pipeline libraries as backup if available | ||
| if (!enableExtensionIfAvailable(VK_KHR_MAINTENANCE_5_EXTENSION_NAME, &maintenance5Features)) | ||
| { | ||
| enableExtensionIfAvailable(VK_KHR_PIPELINE_LIBRARY_EXTENSION_NAME, nullptr); |
There was a problem hiding this comment.
why do you need to enable the pipeline library extension? does the Graphics Pipeline Lib Extension require it?
btw I think its best to actually pass a struct pointer, not sure how well passing nullptr works on the pNext chains :uneasy_smile:
There was a problem hiding this comment.
i saw it being added in Vulkan-Samples. also passing nullptr here is fine. it won't be added to pNext chains but the extension will be inserted.
There was a problem hiding this comment.
i saw it being added in Vulkan-Samples.
its better to quote straight from the spec, instead of relying on someone else not messing up
P.S. unrelated note, this is why I dislike devs trusting ChatGPT and asking it to distil the WebGPU spec for them
There was a problem hiding this comment.
Ok in this case I think its best to enable the pipeline_library extension independently if its there and pass the struct, because at some point someone will want to add that extensions and add it a second time 500 lines away and we'll have a debugging nightmare why the feature struct isn't in pNext (we have a sentinel that prevents double add)
There was a problem hiding this comment.
VK_KHR_pipeline_library has no property/feature structs btw
| maintenance5Features.maintenance5 = true; | ||
| graphicsPipelineLibraryFeatures.graphicsPipelineLibrary = maintenance5Features.maintenance5 == 0; |
There was a problem hiding this comment.
erm but you just set maintenance5 to true, maybe you should incorporate the feature struct bool setting into the ig statements at https://github.com/Devsh-Graphics-Programming/Nabla/pull/829/files#r1947966841
Description
Testing
TODO list: