only init features/properties if ext is enabled #443
Conversation
Some driver (like swiftshader) seems to be exposing a vulkan version but do not support all the core features yet. To avoid error message, make sure that the extension is enabled before initializing it. Ref kpet#441
| } else if ((ext != nullptr) && is_vulkan_extension_enabled(ext)) { | ||
| if (((ext != nullptr) && is_vulkan_extension_enabled(ext)) || | ||
| ((ext == nullptr) && (corever != 0) && | ||
| (m_properties.apiVersion >= corever))) { |
There was a problem hiding this comment.
This doesn't look right to me. It means we're only using core features for which clvk has no knowledge of a corresponding extension. I don't think implementations are required to report extensions that were promoted (worth double-checking though).
There was a problem hiding this comment.
I understand.
It is an improvement over what we had before a55be50. But it is definitely not what we would prefer (which is the actual version).
If we are to keep it that way, I don't know what I will do for google infra. I don't want to disable all swiftshader tests, but I am not sure if anything else will be possible to do easily.
There was a problem hiding this comment.
Just a thought: would it be possible to actually implement those features in swiftshader?
There was a problem hiding this comment.
Good point, I'll look at it
There was a problem hiding this comment.
One of those has already been implemented upstream, we just need to rebuild swiftshader.
The other is PhysicalDevice8BitStorageFeatures.
There was a problem hiding this comment.
So I was right and reporting the extension is not required (at least for that one but that's a good hint that there's no blanket requirement). We can't make the change proposed in this PR then.
There was a problem hiding this comment.
If I understand correctly, our way forward is to add support for VkPhysicalDevice8BitStorageFeatures in swiftshader. But it will just report that nothing is supported, right?
There was a problem hiding this comment.
Yes, that's the cheapest possible change but maybe swiftshader actually supports those features? You'd have to check (with them). BTW, I've now updated switshader in the CI, that should remove one of the failures.
There was a problem hiding this comment.
Update:
I was mistaken, nothing has been added upstream. I was only seeing one of them in google infra because of the stop at first error.
The other one is PhysicalDeviceShaderFloat16Int8Features.
Both of them are not supported in swiftshader. But I feel like they are just missing from one switch-case.
I am testing it, and will propose a change in swiftshader.
swiftshader reports supporting vulkan 1.2. As such vkCreateDevice needs to accept VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_8BIT_STORAGE_FEATURES and VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_SHADER_FLOAT16_INT8_FEATURES, even so those features are not supported. This CL aims at fixing clvk (OpenCL over Vulkan): kpet/clvk#441 kpet/clvk#443 https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VK_KHR_shader_float16_int8.html#_promotion_to_vulkan_1_2 https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VK_KHR_8bit_storage.html#_promotion_to_vulkan_1_2 Bug: b/181875303 Change-Id: I82ed41f84c30aa7ce73ffcc9f3e5fe0b763c5d73 Reviewed-on: https://swiftshader-review.googlesource.com/c/SwiftShader/+/69328 Tested-by: Romaric Jodin <rjodin@chromium.org> Reviewed-by: Alexis Hétu <sugoi@google.com> Reviewed-by: Nicolas Capens <nicolascapens@google.com> Commit-Queue: Romaric Jodin <rjodin@chromium.org>
Some driver (like swiftshader) seems to be exposing a vulkan version but do not support all the core features yet.
To avoid error message, make sure that the extension is enabled before initializing it.
Ref #441