-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Allow SDL GPU to opt into additional Vulkan features #14204
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
base: main
Are you sure you want to change the base?
Conversation
This adds SDL_HINT_VULKAN_REQUEST_API_VERSION to allow requesting a specific API version when SDL creates the Vulkan instance.
The patch version shouldn't be relevant to the API and a new major version will likely require an entirely new renderer anyway. The benefit of this approach is that it massively simplifies parsing of the hint.
We hardcode to 1.0 to provide maximum driver compatibility. I'm totally fine with allowing an opt-in to higher versions. |
include/SDL3/SDL_hints.h
Outdated
* This hint should be set before creating a Vulkan window. Expects a positive | ||
* integer. E.g. 3 for Vulkan 1.3. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's reasonable to allow specifying the full version and will be more intuitive for users. This is easily implemented with, e.g.
if (SDL_sscanf(hint, "%d.%d", &major, &minor) == 2) {
...
} else {
// Parse error...
}
Co-authored-by: Sam Lantinga <[email protected]>
This reverts commit 6c4d2c0.
This needs a bit more time in the oven, I think. |
Introduces a new property to use when creating a Vulkan renderer. SDL_PROP_GPU_DEVICE_CREATE_VULKAN_ADDITIONAL_FEATURES_POINTER
Since the developer can now pass additional features to be activated, this name makes more obvious that these get enabled by SDL regardless.
I added a workflow to opt-into Vulkan device features using properties. I made sure the default behavior is unaffected. The usage code in my application look like this: props := sdl.CreateProperties()
sdl.SetBooleanProperty(props, sdl.PROP_GPU_DEVICE_CREATE_SHADERS_SPIRV_BOOLEAN, true)
sdl.SetBooleanProperty(props, sdl.PROP_GPU_DEVICE_CREATE_DEBUGMODE_BOOLEAN, true)
sdl.SetStringProperty(props, sdl.PROP_GPU_DEVICE_CREATE_NAME_STRING, "vulkan")
// Enable Vulkan 1.1 feature
vulkan_11_features := vk.PhysicalDeviceVulkan11Features {
sType = vk.StructureType.PHYSICAL_DEVICE_VULKAN_1_1_FEATURES,
shaderDrawParameters = true,
}
sdl.SetPointerProperty(props, sdl.PROP_GPU_DEVICE_CREATE_VULKAN_ADDITIONAL_FEATURES_POINTER, rawptr(&vulkan_11_features))
result.gpu = sdl.CreateGPUDeviceWithProperties(props) EDIT: This still needs to handle errors from unsupported features. |
KHR_portability_subset structure was being overwritten by opt-in features.
This seems like a reasonable workflow to me. @thatcosmonaut, thoughts? |
Yeah that does seem reasonable. Is this stable if we have to add fields to the struct? |
If increasing the Vulkan version often means chaining additional device creation structures, maybe the Vulkan version should also be a property instead of a hint? |
I'm not sure I understand.
I was thinking about the same thing. Right now, I'm looking into error handling. As it stands, the vkCreateDevice() call crashes when unsupported features are requested. |
We guarantee ABI stability in SDL, so if we add something we can't make future code changes that will cause an executable to break if a client updates the SDL3 DLL for example. |
Yes, this change doesn't affect any public structures, so this is ABI safe. |
Oh I just realized it's literally a pointer to the Vulkan struct, that makes sense. |
My current solution looks like this: // SDL_gpu.h
#define SDL_PROP_GPU_DEVICE_CREATE_VULKAN_OPTIONS_POINTER "SDL.gpu.device.create.vulkan.options"
/**
* A structure specifying additional options when using Vulkan.
*
* When no such structure is provided, SDL will use Vulkan API version 1.0 and a minimal set of features.
*
* \since This struct is available since SDL 3.4.0.
*
*/
typedef struct SDL_GPUVulkanOptions
{
Uint32 vulkan_api_version; /**< The Vulkan API version to request for the instance. */
void *physical_device_features_1_0; /**< Pointer to a VkPhysicalDeviceFeatures struct. */
void *physical_device_features_1_1; /**< Pointer to a VkPhysicalDeviceVulkan11Features struct. */
void *physical_device_features_1_2; /**< Pointer to a VkPhysicalDeviceVulkan12Features struct. */
void *physical_device_features_1_3; /**< Pointer to a VkPhysicalDeviceVulkan13Features struct. */
} SDL_GPUVulkanOptions; Providing the various structs separately rather than in a chain makes validation and error-reporting much easier. Does this look like a workable solution? |
I would prefer your original implementation which just added a single chained structure to the options. It's more flexible, and once you're using properties, you're generally speaking outside the normal development path and responsible for doing so safely. |
Yeah, I started reworking it almost immediately after I posted that yesterday. It occurred to me that this structure wasn't very forward compatible. |
Also replaces the previously created hint
In that case, I think it would be better to remove the whitelist and allow passing arbitrary struct chains into device creation. As @slouken said, when you're using properties, you're responsible for using them correctly. |
Agreed. |
Looking a bit deeper into the specs, it looks like Vulkan 1.1 is an annoying special case. At that point, they still haven't quite figured out how they want to handle future version extensions yet. My current implementation works with 1.2+. Saying that using props means you're responsible for using them correctly is fine, but I'd still like SDL to do some sanity checking. At least enough to where it can fail gracefully and report useful error messages rather than just crashing. |
Next draft. (Still WIP.) /**
* A structure specifying additional options when using Vulkan.
*
* When no such structure is provided, SDL will use Vulkan API version 1.0 and a minimal set of features.
* The feature list gets passed to the vkCreateInstance function and allows requesting additional
* features.
*
* \since This struct is available since SDL 3.4.0.
*
*/
typedef struct SDL_GPUVulkanOptions
{
Uint32 vulkan_api_version; /**< The Vulkan API version to request for the instance. Use Vulkan's VK_MAKE_VERSION or VK_MAKE_API_VERSION. */
void *feature_list; /**< Pointer to the first element of a list of structs to be passed to device creation. */
void *vulkan_10_physical_device_features; /**< Pointer to a VkPhysicalDeviceFeatures struct to enable additional Vulkan 1.0 features. */
} SDL_GPUVulkanOptions;
|
E.g. Vulkan 1.3 features will be ignored if API version 1.2 or lower is requested.
This may have been answered already so apologies in advance, but is the slang compiler capable of using |
I am working on something like that. This pull request started off as a version selector, but morphed into something more flexible. If you're interested in specific features, please post them here. I'll see if they can be made to fit. @flibitijibibo |
Separate code-paths for 1.0, 1.1 and 1.2+
API version and device features seem to work fine now. I don't see a reason to leave extensions out. I'm extending the API to allow you to opt into those, too. |
Opt-in instance extensions are implemented now. I also noticed and fixed another bug while I was at it. Force-pushed to split it into a separate pull request. #14272 |
This pull request looks like its pretty much ready. // SDL_gpu.h
typedef struct SDL_GPUVulkanOptions
{
Uint32 vulkan_api_version; /**< The Vulkan API version to request for the instance. Use Vulkan's VK_MAKE_VERSION or VK_MAKE_API_VERSION. */
void *feature_list; /**< Pointer to the first element of a list of structs to be passed to device creation. */
void *vulkan_10_physical_device_features; /**< Pointer to a VkPhysicalDeviceFeatures struct to enable additional Vulkan 1.0 features. */
Uint32 device_extension_count; /**< Number of additional device extensions to require. */
const char **device_extension_names; /**< Pointer to a list of additional device extensions to require. */
Uint32 instance_extension_count; /**< Number of additional instance extensions to require. */
const char **instance_extension_names; /**< Pointer to a list of additional instance extensions to require. */
} SDL_GPUVulkanOptions; I made sure the device features get handled in separate code-paths depending on whether you request API version 1.0, 1.1 or 1.2+. I also went out of my way to add useful error logging. It'll report the name of the feature or extension that is unsupported. Does this look okay? @slouken @thatcosmonaut |
I will defer to @thatcosmonaut, but if he likes this patch, it can go in. |
Error checking no longer happens in VULKAN_INTERNAL_AddOptInVulkanOptions()
Since the user might pass a pointer to the stack, we better clear this after we're done with it.
Co-authored-by: Sam Lantinga <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will also defer to @thatcosmonaut here, but it does raise the question of how much stuff we could take out of the VulkanRenderer struct since a large percentage, with or without these changes, is only needed at init time. For now we can probably leave it alone, but we should check this out afterward.
} | ||
} | ||
|
||
renderer->additionalDeviceExtensionCount = options->device_extension_count; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we specify in the header that these pointers aren't deep-copied?
} | ||
|
||
// Make sure we don't hold onto potentially unsafe pointers after initialization | ||
renderer->additionalDeviceExtensionCount = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it's worth making these local variables somehow, so that the VulkanRenderer doesn't have to carry this all throughout runtime.
Currently
SDL_gpu_vulkan.c
hardcodes API version 1.0 when creating the Vulkan instance for the program. This creates a problem for my use-case.I'd like to use Slang for shader development, but SPIR-V 1.0 support is experimental. I tried two solutions and both work:
I looked at the Vulkan documentation (specifically this and this). It says "Deprecated items will still work in current Vulkan implementations", which makes me prefer option 2. A Vulkan application requesting 1.3, but using only 1.0 features should work just fine. And using a hint doesn't change SDL's default behavior.
Since I don't know why this was hardcoded, I figured I'd first ask if there's any chance of this merge request going anywhere. Before I waste time cleaning something up that's not going to be merged for reasons I didn't know of.