Fix issue around incorrect Vulkan version #69322
Conversation
BastiaanOlij
force-pushed
the
fix_vulkan_versions
branch
from
07ab06f
to
52ffb58
Compare
|
One other note, it seems unused at the moment but Vulkan introduced a |
There was a problem hiding this comment.
I may be misunderstanding the issue, but I thought the issue for users is that we were using the instance version which typically reports a higher version than the device version. However, in this code, we are capping the device version to ensure that it doesn't go higher than the instance version. Shouldn't it be the other way around, or alternatively, shouldn't we be taking the lower of the two?
|
@clayjohn yes indeed, we were using the instance version, now we're using the device version, but I'm capping the device version to the instance version just in case the device version is higher (well I'm actually capping by the application version which is the lower of instance version and our headers version). Like I mentioned in my comments, according to the spec, if the device version is higher we should be able to use the higher Vulkan version, the instance version doesn't seem to mean much. But I was mostly playing it safe. |
Ah right, and the problems right now are coming from a low device version, in which case it should just work fine. That makes sense! |
There was a problem hiding this comment.
Seems fine to me. Would be nice to have @RandomShaper's review as well
BastiaanOlij
force-pushed
the
fix_vulkan_versions
branch
from
52ffb58
to
0781136
Compare
|
Ok simplified this a bit based on the feedback from @RandomShaper and @sakrel, please review again :) |
drivers/vulkan/vulkan_context.cpp
Outdated
| /*pEngineName*/ VERSION_NAME, | ||
| /*engineVersion*/ VK_MAKE_VERSION(VERSION_MAJOR, VERSION_MINOR, VERSION_PATCH), | ||
| /*apiVersion*/ VK_MAKE_VERSION(vulkan_major, vulkan_minor, 0) | ||
| /*apiVersion*/ VK_API_VERSION_1_2 // We're targetting Vulkan 1.2, we can support older versions but we don't use features for newer versions. Note that setting this is purely used by validation layers. |
There was a problem hiding this comment.
We can only set apiVersion to VK_API_VERSION_1_2 when vkEnumerateInstanceVersion returns a version >= VK_API_VERSION_1_1. On a Vulkan 1.0 implementation this needs to be VK_API_VERSION_1_0. https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VkApplicationInfo.html#_description
Now that apiVersion can either be VK_API_VERSION_1_2 or VK_API_VERSION_1_0, we actually do need to cap device_api_version because apiVersion is the highest API version the application can use.
There was a problem hiding this comment.
Ok that seemed to contradict what @RandomShaper said, I'm happy to put back the code that checks instance version and cap this by the instance version.
We can't cap by device version because we don't learn about the device version until after the instance is created.
That is also how I understood apiVersion, where we specify what version the API is build for and that this value is only used by the validation layer to warn us when we're calling an API that is not supported.
There was a problem hiding this comment.
I wasn't aware of the Vulkan 1.0 exception. Under Vulkan 1.0, you can't set apiVersion to anything but API_VERSION_1_0. On Vulkan 1.1+, we're fine with API_VERSION_1_2.
There was a problem hiding this comment.
@sakrel @RandomShaper have a look at my latest changes and see if this is better?
There was a problem hiding this comment.
Looks great! Still, I think device_api_version should be capped. The Vulkan spec says:
Device-level functionality or behavior added by a new core version of the API must not be used unless it is supported by the device as determined by VkPhysicalDeviceProperties::apiVersion and the specified version of VkApplicationInfo::apiVersion.
So, if application_api_version is 1.0 because we are on a Vulkan 1.0 instance, we can't use Vulkan 1.1 device features even when device_api_version is 1.1 or higher. Therefore, I think device_api_version should be set to min(gpu_props.apiVersion, application_api_version).
There was a problem hiding this comment.
But AFAIK we enable the use of 1.1 and 1.2 features conditionally. So we shouldn't mandate 1.2 support at this stage I suppose?
For the record, I would be fine with outright dropping Vulkan 1.0 support - the drivers from that era are too buggy to bother IMO. At least for now.
There was a problem hiding this comment.
Quoting the spec about VkApplicationInfo:
apiVersionmust be the highest version of Vulkan that the application is designed to use, encoded as described in Version Numbers. [...]
Vulkan 1.0 implementations were required to return
VK_ERROR_INCOMPATIBLE_DRIVERifapiVersionwas larger than 1.0. Implementations that support Vulkan 1.1 or later must not returnVK_ERROR_INCOMPATIBLE_DRIVERfor any value ofapiVersion.
I believe the current code in this PR correctly accounts for the above. VkApplicationInfo::apiVersion is more informative than prescriptive.
There was a problem hiding this comment.
That's my understanding, too. We ask Vulkan for devices supporting 1.2 at least. If we get a 1.3 device, no problem because of backwards compatibility.
Yes, there's no requirement to cap the device version on a Vulkan 1.1+ instance. I just figured that it would be safer to cap the version anyway. This way if somebody adds if (device_api_version >= VK_API_VERSION_1_3) checks, they would be forced to first raise VkApplicationInfo::apiVersion to get the check to pass. On second thought, this is something validation layers (if they are enabled) should catch anyway, so capping device_api_version like that is, as both of you already pointed out, unnecessary.
There was a problem hiding this comment.
Yup indeed, thats really the only thing that VkApplicationInfo::apiVersion does. Throws up validation errors if you attempt to use features of newer Vulkan versions.
In all likelihood, provided the feature is enabled, it would still work.
There was a problem hiding this comment.
Do we have a comment somewhere saying all of this you discovered now, because in a couple of months someone will likely be retreading the same steps?
…f instance version for Vulkan
BastiaanOlij
force-pushed
the
fix_vulkan_versions
branch
from
0781136
to
2562c06
Compare
|
Thanks! |
With Vulkan we are dealing with two different version numbers (well 3 actually for completeness)
We were purely retrieving the instance version and checking capabilities against that however often the GPU may be an older device with older drivers that do not support the Vulkan version the OS supports and thus we were enabling things that shouldn't be enabled.
This PR cleans up the version checks and now tracks both instance and device versions and checks the device version where applicable. The device version is also what we expose to the rest of the application.
Note that when creating our instance we tell Vulkan what API we're developing against. We set this to the instance API version but cap it to the version of the Vulkan header we include. Note that Vulkan only uses this version for validation checking. If we tell it we only support Vulkan 1.1, and we use Vulkan 1.2 functions, fool on us I guess :)