[PATCH 2/3] winevulkan: Implement extension faking mechanism
On 13.10.20 03:50, Joshua Ashton wrote:
Needed for VK_EXT_full_screen_exclusive
Signed-off-by: Joshua Ashton <joshua(a)froggi.es> --- dlls/winevulkan/make_vulkan | 21 +++++++++++++++++++ dlls/winevulkan/vulkan.c | 36 ++++++++++++++++++++++++++++++++ dlls/winevulkan/vulkan_private.h | 3 +++ 3 files changed, 60 insertions(+)
diff --git a/dlls/winevulkan/make_vulkan b/dlls/winevulkan/make_vulkan index db1d8edcf3..70d8fa5516 100755 --- a/dlls/winevulkan/make_vulkan +++ b/dlls/winevulkan/make_vulkan @@ -232,6 +232,10 @@ STRUCT_CHAIN_CONVERSIONS = [ "VkInstanceCreateInfo", ]
+# List of device extensions to ensure that we support regardless of whether +# the actual driver supports them or not. +FAKED_EXTENSIONS = [
I think the name should reflect that these are only device extensions. e.g. FAKED_DEVICE_EXTENSIONS
+]
class Direction(Enum): """ Parameter direction: input, output, input_output. """ @@ -2247,6 +2251,13 @@ class VkGenerator(object): f.write(" \"{0}\",\n".format(ext["name"])) f.write("};\n\n")
+ # Create array of faked device extensions. + f.write("static const VkExtensionProperties vk_device_extension_faked[] =\n{\n") + for ext in FAKED_EXTENSIONS: + f.write(" {{\"{0}\", {1}}},\n".format(ext["name"], ext["version"])) + f.write(" {\"\", 0}\n") + f.write("};\n\n") + # Create array of instance extensions. f.write("static const char * const vk_instance_extensions[] =\n{\n") for ext in self.registry.extensions: @@ -2276,6 +2287,16 @@ class VkGenerator(object): f.write(" return TRUE;\n") f.write(" }\n") f.write(" return FALSE;\n") + f.write("}\n\n") + + f.write("unsigned int wine_vk_device_extension_faked_count(void)\n") + f.write("{\n") + f.write(" return ARRAY_SIZE(vk_device_extension_faked) - 1;\n") + f.write("}\n\n") + + f.write("const VkExtensionProperties* wine_vk_device_extension_faked_idx(unsigned int idx)\n") + f.write("{\n") + f.write(" return &vk_device_extension_faked[idx];\n") f.write("}\n")
def generate_thunks_h(self, f, prefix): diff --git a/dlls/winevulkan/vulkan.c b/dlls/winevulkan/vulkan.c index 4352c5536e..177aa3a8bf 100644 --- a/dlls/winevulkan/vulkan.c +++ b/dlls/winevulkan/vulkan.c @@ -129,6 +129,8 @@ static struct VkPhysicalDevice_T *wine_vk_physical_device_alloc(struct VkInstanc TRACE("Skipping extension '%s', no implementation found in winevulkan.\n", host_properties[i].extensionName); } } +
white space error Thanks, Georg Lehmann
+ num_properties += wine_vk_device_extension_faked_count();
TRACE("Host supported extensions %u, Wine supported extensions %u\n", num_host_properties, num_properties);
@@ -146,6 +148,11 @@ static struct VkPhysicalDevice_T *wine_vk_physical_device_alloc(struct VkInstanc j++; } } + for (i = 0; i < wine_vk_device_extension_faked_count(); i++) + { + object->extensions[j] = *wine_vk_device_extension_faked_idx(i); + j++; + } object->extension_count = num_properties;
heap_free(host_properties); @@ -226,13 +233,27 @@ static void wine_vk_device_free_create_info(VkDeviceCreateInfo *create_info) heap_free((void *)group_info->pPhysicalDevices); }
+ heap_free((void *)create_info->ppEnabledExtensionNames); + free_VkDeviceCreateInfo_struct_chain(create_info); }
+static BOOL wine_vk_device_extension_faked(const char *name) +{ + unsigned int i; + for (i = 0; i < wine_vk_device_extension_faked_count(); i++) + { + if (strcmp(wine_vk_device_extension_faked_idx(i)->extensionName, name) == 0) + return TRUE; + } + return FALSE; +} + static VkResult wine_vk_device_convert_create_info(const VkDeviceCreateInfo *src, VkDeviceCreateInfo *dst) { VkDeviceGroupDeviceCreateInfo *group_info; + const char** extensions; unsigned int i; VkResult res;
@@ -261,6 +282,21 @@ static VkResult wine_vk_device_convert_create_info(const VkDeviceCreateInfo *src group_info->pPhysicalDevices = physical_devices; }
+ + /* Allocate our own extension list, and remove any faked extensions + * so they don't get passed through to the driver. */ + extensions = heap_alloc(sizeof(const char*) * src->enabledExtensionCount); + dst->ppEnabledExtensionNames = extensions; + dst->enabledExtensionCount = 0; + for (i = 0; i < src->enabledExtensionCount; i++) { + const char *extension_name = src->ppEnabledExtensionNames[i]; + + if (!wine_vk_device_extension_faked(extension_name)) { + extensions[dst->enabledExtensionCount] = extension_name; + dst->enabledExtensionCount++; + } + } + /* Should be filtered out by loader as ICDs don't support layers. */ dst->enabledLayerCount = 0; dst->ppEnabledLayerNames = NULL; diff --git a/dlls/winevulkan/vulkan_private.h b/dlls/winevulkan/vulkan_private.h index 4bcc4de440..bb4a3e7ded 100644 --- a/dlls/winevulkan/vulkan_private.h +++ b/dlls/winevulkan/vulkan_private.h @@ -135,6 +135,9 @@ static inline VkCommandPool wine_cmd_pool_to_handle(struct wine_cmd_pool *cmd_po void *wine_vk_get_device_proc_addr(const char *name) DECLSPEC_HIDDEN; void *wine_vk_get_instance_proc_addr(const char *name) DECLSPEC_HIDDEN;
+unsigned int wine_vk_device_extension_faked_count(void) DECLSPEC_HIDDEN; +const VkExtensionProperties* wine_vk_device_extension_faked_idx(unsigned int idx) DECLSPEC_HIDDEN; + BOOL wine_vk_device_extension_supported(const char *name) DECLSPEC_HIDDEN; BOOL wine_vk_instance_extension_supported(const char *name) DECLSPEC_HIDDEN;
Why not do this the normal way, i.e. by modifying the code in winex11 and winemac? Also, what's the point of faking the extension like this? Why not just pass it through?
winex11/winemac only does this for instance extensions. VK_EXT_full_screen_exclusive is a device extension. - Joshie 🐸✨ On 10/13/20 10:35 PM, Zebediah Figura wrote:
Why not do this the normal way, i.e. by modifying the code in winex11 and winemac?
Also, what's the point of faking the extension like this? Why not just pass it through?
Figured I should add the reason why, is because just passing it through will cause the device to fail to be created because nobody supports VK_EXT_full_screen_exclusive on Linux. It's an entirely Windows-centric extension. - Joshie 🐸✨ On 10/13/20 10:42 PM, Joshua Ashton wrote:
winex11/winemac only does this for instance extensions.
VK_EXT_full_screen_exclusive is a device extension.
- Joshie 🐸✨
On 10/13/20 10:35 PM, Zebediah Figura wrote:
Why not do this the normal way, i.e. by modifying the code in winex11 and winemac?
Also, what's the point of faking the extension like this? Why not just pass it through?
October 13, 2020 4:55 PM, "Joshua Ashton" <joshua(a)froggi.es> wrote:
Figured I should add the reason why, is because just passing it through will cause the device to fail to be created because nobody supports VK_EXT_full_screen_exclusive on Linux.
It's an entirely Windows-centric extension.
I don't know. I'm thinking about implementing it in MoltenVK, so we can at least say, "these are the surface formats that support exclusive fullscreen." If you use a compatible drawable format, Metal handles exclusive fullscreen automatically. Chip
The extension *entirely* resides in vulkan_win32.h, which would make it impossible for you to implement. If you really want to implement it, you'd need to make an issue about splitting up the extension into different platformed extensions (ie. VK_EXT_win32_full_screen_exclusive), which would require a VK_EXT_full_screen_exclusive2. So it's a non issue that we're entirely tossing this out for non-Windows platforms. fwiw, the extension is mostly used on Windows just to *avoid* full screen exclusive for borderless window modes anyway. (ie. no hitches when alt-tabbing, which is a problem we don't have on Linux anyway) - Joshie 🐸✨ On 10/13/20 11:23 PM, Chip Davis wrote:
October 13, 2020 4:55 PM, "Joshua Ashton" <joshua(a)froggi.es> wrote:
Figured I should add the reason why, is because just passing it through will cause the device to fail to be created because nobody supports VK_EXT_full_screen_exclusive on Linux.
It's an entirely Windows-centric extension.
I don't know. I'm thinking about implementing it in MoltenVK, so we can at least say, "these are the surface formats that support exclusive fullscreen." If you use a compatible drawable format, Metal handles exclusive fullscreen automatically.
Chip
On 14.10.20 00:23, Chip Davis wrote:
October 13, 2020 4:55 PM, "Joshua Ashton" <joshua(a)froggi.es> wrote:
Figured I should add the reason why, is because just passing it through will cause the device to fail to be created because nobody supports VK_EXT_full_screen_exclusive on Linux.
It's an entirely Windows-centric extension. I don't know. I'm thinking about implementing it in MoltenVK, so we can at least say, "these are the surface formats that support exclusive fullscreen." If you use a compatible drawable format, Metal handles exclusive fullscreen automatically.
Chip
Even if you implement it, faking it in wine is the best solution. Winevulkan couldn't pass it through to MoltenVK since VkSurfaceFullScreenExclusiveWin32InfoEXT isn't supported for obvious reasons. Georg
On 10/13/20 4:54 PM, Joshua Ashton wrote:
Figured I should add the reason why, is because just passing it through will cause the device to fail to be created because nobody supports VK_EXT_full_screen_exclusive on Linux.
Sure, but they could.
It's an entirely Windows-centric extension.
Well, no, it's not really, but it does include some API-level integration with win32 monitors *if* KHR_win32_surface is supported. In that case, as far as I can see, we don't even need to do anything, because the VkSurfaceFullScreenExclusiveWin32InfoEXT structure would just be ignored. Not that this necessarily means that lower-level drivers should be the ones to implement the extension, but if nothing else, this information isn't present at all in the patch. Also, do we really need a generic mechanism for this, wired into make_vulkan? Can't we just handle this extension specially in wine_vk_device_convert_create_info()?
- Joshie 🐸✨
On 10/13/20 10:42 PM, Joshua Ashton wrote:
winex11/winemac only does this for instance extensions.
VK_EXT_full_screen_exclusive is a device extension.
- Joshie 🐸✨
On 10/13/20 10:35 PM, Zebediah Figura wrote:
Why not do this the normal way, i.e. by modifying the code in winex11 and winemac?
Also, what's the point of faking the extension like this? Why not just pass it through?
On 14.10.20 01:23, Zebediah Figura wrote:
On 10/13/20 4:54 PM, Joshua Ashton wrote:
Figured I should add the reason why, is because just passing it through will cause the device to fail to be created because nobody supports VK_EXT_full_screen_exclusive on Linux. Sure, but they could.
It's an entirely Windows-centric extension. Well, no, it's not really, but it does include some API-level integration with win32 monitors *if* KHR_win32_surface is supported. In that case, as far as I can see, we don't even need to do anything, because the VkSurfaceFullScreenExclusiveWin32InfoEXT structure would just be ignored.
No, according to the vulkan spec a VkSurfaceFullScreenExclusiveWin32InfoEXT is illegal in any chain unless the full screen + win32 surface exts are enabled. In practice, this will break layers like renderdoc. So we would still need code to remove that struct from pNext chains. Georg
Not that this necessarily means that lower-level drivers should be the ones to implement the extension, but if nothing else, this information isn't present at all in the patch.
Also, do we really need a generic mechanism for this, wired into make_vulkan? Can't we just handle this extension specially in wine_vk_device_convert_create_info()?
- Joshie 🐸✨
On 10/13/20 10:42 PM, Joshua Ashton wrote:
winex11/winemac only does this for instance extensions.
VK_EXT_full_screen_exclusive is a device extension.
- Joshie 🐸✨
On 10/13/20 10:35 PM, Zebediah Figura wrote:
Why not do this the normal way, i.e. by modifying the code in winex11 and winemac?
Also, what's the point of faking the extension like this? Why not just pass it through?
Zeb, it's in vulkan_win32.h. You can't include it on Linux or any other platform for that matter. If you read the xml, it's marked as being `platform="win32"`. It's a Windows extension. To move it out of there on a Vulkan level, we'd need to make a sequel and different platform-specific extensions. Inherently making this version unsupportable in non-Windows drivers. It makes more sense to read the line of VK_KHR_win32_surface support more as if you want surfaces (ie. not headless) than focusing on the platform part of things. - Joshie 🐸✨ On 10/14/20 12:37 AM, Georg Lehmann wrote:
On 14.10.20 01:23, Zebediah Figura wrote:
On 10/13/20 4:54 PM, Joshua Ashton wrote:
Figured I should add the reason why, is because just passing it through will cause the device to fail to be created because nobody supports VK_EXT_full_screen_exclusive on Linux. Sure, but they could.
It's an entirely Windows-centric extension. Well, no, it's not really, but it does include some API-level integration with win32 monitors *if* KHR_win32_surface is supported. In that case, as far as I can see, we don't even need to do anything, because the VkSurfaceFullScreenExclusiveWin32InfoEXT structure would just be ignored.
No, according to the vulkan spec a VkSurfaceFullScreenExclusiveWin32InfoEXT is illegal in any chain unless the full screen + win32 surface exts are enabled. In practice, this will break layers like renderdoc. So we would still need code to remove that struct from pNext chains.
Georg
Not that this necessarily means that lower-level drivers should be the ones to implement the extension, but if nothing else, this information isn't present at all in the patch.
Also, do we really need a generic mechanism for this, wired into make_vulkan? Can't we just handle this extension specially in wine_vk_device_convert_create_info()?
- Joshie 🐸✨
On 10/13/20 10:42 PM, Joshua Ashton wrote:
winex11/winemac only does this for instance extensions.
VK_EXT_full_screen_exclusive is a device extension.
- Joshie 🐸✨
On 10/13/20 10:35 PM, Zebediah Figura wrote:
Why not do this the normal way, i.e. by modifying the code in winex11 and winemac?
Also, what's the point of faking the extension like this? Why not just pass it through?
To add on about the layer thing, not removing random unsupported pNext elements has and will break layers like RenderDoc and OBS and potentially drivers down the line. The typical pattern I see is if (info->pNext->sType == VK_STRUCTURE_TYPE_MY_UNSUPPORTED_STRUCTURE_FROG) { #ifdef _WIN32 // Handle whatever this is... info->pNext->... #else // Uh oh! Something went wrong here... abort(1); #endif } The problem here is that the structure type is still defined in vulkan_core.h despite the structs, etc being defined in vulkan_win32.h which causes this pattern. Many Layers (and potentially drivers) do stuff this, and not filtering out the pNexts will break them. - Joshie 🐸✨ On 10/14/20 1:02 AM, Joshua Ashton wrote:
Zeb, it's in vulkan_win32.h. You can't include it on Linux or any other platform for that matter. If you read the xml, it's marked as being `platform="win32"`. It's a Windows extension.
To move it out of there on a Vulkan level, we'd need to make a sequel and different platform-specific extensions. Inherently making this version unsupportable in non-Windows drivers.
It makes more sense to read the line of VK_KHR_win32_surface support more as if you want surfaces (ie. not headless) than focusing on the platform part of things.
- Joshie 🐸✨
On 10/14/20 12:37 AM, Georg Lehmann wrote:
On 14.10.20 01:23, Zebediah Figura wrote:
On 10/13/20 4:54 PM, Joshua Ashton wrote:
Figured I should add the reason why, is because just passing it through will cause the device to fail to be created because nobody supports VK_EXT_full_screen_exclusive on Linux. Sure, but they could.
It's an entirely Windows-centric extension. Well, no, it's not really, but it does include some API-level integration with win32 monitors *if* KHR_win32_surface is supported. In that case, as far as I can see, we don't even need to do anything, because the VkSurfaceFullScreenExclusiveWin32InfoEXT structure would just be ignored.
No, according to the vulkan spec a VkSurfaceFullScreenExclusiveWin32InfoEXT is illegal in any chain unless the full screen + win32 surface exts are enabled. In practice, this will break layers like renderdoc. So we would still need code to remove that struct from pNext chains.
Georg
Not that this necessarily means that lower-level drivers should be the ones to implement the extension, but if nothing else, this information isn't present at all in the patch.
Also, do we really need a generic mechanism for this, wired into make_vulkan? Can't we just handle this extension specially in wine_vk_device_convert_create_info()?
- Joshie 🐸✨
On 10/13/20 10:42 PM, Joshua Ashton wrote:
winex11/winemac only does this for instance extensions.
VK_EXT_full_screen_exclusive is a device extension.
- Joshie 🐸✨
On 10/13/20 10:35 PM, Zebediah Figura wrote:
Why not do this the normal way, i.e. by modifying the code in winex11 and winemac?
Also, what's the point of faking the extension like this? Why not just pass it through?
On 10/13/20 5:10 PM, Joshua Ashton wrote:
To add on about the layer thing, not removing random unsupported pNext elements has and will break layers like RenderDoc and OBS and potentially drivers down the line.
The typical pattern I see is if (info->pNext->sType == VK_STRUCTURE_TYPE_MY_UNSUPPORTED_STRUCTURE_FROG) { #ifdef _WIN32 // Handle whatever this is... info->pNext->... #else // Uh oh! Something went wrong here... abort(1); #endif }
The problem here is that the structure type is still defined in vulkan_core.h despite the structs, etc being defined in vulkan_win32.h which causes this pattern.
Many Layers (and potentially drivers) do stuff this, and not filtering out the pNexts will break them.
As noted below, the Vulkan specification covers this behavior. If observed behavior contradicts this, then there is a bug.
- Joshie 🐸✨
On 10/14/20 1:02 AM, Joshua Ashton wrote:
Zeb, it's in vulkan_win32.h. You can't include it on Linux or any other platform for that matter. If you read the xml, it's marked as being `platform="win32"`. It's a Windows extension.
To move it out of there on a Vulkan level, we'd need to make a sequel and different platform-specific extensions. Inherently making this version unsupportable in non-Windows drivers.
It makes more sense to read the line of VK_KHR_win32_surface support more as if you want surfaces (ie. not headless) than focusing on the platform part of things.
Could you please keep replies inline, top-posting makes it hard to correlate replies to contexts (although I figured it out in this case)?
- Joshie 🐸✨
On 10/14/20 12:37 AM, Georg Lehmann wrote:
On 14.10.20 01:23, Zebediah Figura wrote:
On 10/13/20 4:54 PM, Joshua Ashton wrote:
Figured I should add the reason why, is because just passing it through will cause the device to fail to be created because nobody supports VK_EXT_full_screen_exclusive on Linux. Sure, but they could.
It's an entirely Windows-centric extension. Well, no, it's not really, but it does include some API-level integration with win32 monitors *if* KHR_win32_surface is supported. In that case, as far as I can see, we don't even need to do anything, because the VkSurfaceFullScreenExclusiveWin32InfoEXT structure would just be ignored.
No, according to the vulkan spec a VkSurfaceFullScreenExclusiveWin32InfoEXT is illegal in any chain unless the full screen + win32 surface exts are enabled. In practice, this will break layers like renderdoc. So we would still need code to remove that struct from pNext chains.
Where are you seeing this (please cite the specification text/section specifically)? Pulling up the latest spec [0] I see the following under "Valid Usage for Structure Pointer Chains":
Any component of the implementation (the loader, any enabled layers, and drivers) must skip over, without processing (other than reading the sType and pNext members) any extending structures in the chain not defined by core versions or extensions supported by that component.
Which I understand as, filtering of these pNext chains is not necessary. Sorry for any past confusion where I may have implied otherwise.
Georg
Not that this necessarily means that lower-level drivers should be the ones to implement the extension, but if nothing else, this information isn't present at all in the patch.
I had replied to v2 of this patch before I got to this thread. I think this could be resolved with sufficient comments/documentation around this concept of faked extensions.
Also, do we really need a generic mechanism for this, wired into make_vulkan? Can't we just handle this extension specially in wine_vk_device_convert_create_info()?
Having a generated solution for extensions which cannot be supported by the underlying ICD would be useful in other cases also. Looking back at a patchset from about a year ago, Derek Lesho had a proof-of-concept which implemented memory import/export [1], it had to manually replace the extension name for VK_KHR_external_memory_win32 with VK_KHR_external_memory_fd. Having that kind of manual step isn't inherently bad, but I'd think it could be done in a cleaner way with having a generator do the work. That said, I think if it's decided to go with a faking mechanism for device extensions, that a mechanism for instance extensions should be "on the horizon".
- Joshie 🐸✨
On 10/13/20 10:42 PM, Joshua Ashton wrote:
winex11/winemac only does this for instance extensions.
VK_EXT_full_screen_exclusive is a device extension.
It seems like winex11/winemac are the correct place for WSI related extensions though, and that the implementation from patch 3/3 should take place in there. In my original review of patch 3/3 (then 5/5) I asked a few questions about your thought process here:
I agree with Zebediah that this should also include changes to winex11 and winemac. Is your plan currently that because fullscreen exclusive isn't a thing on Linux, we should just say it's supported and that's good enough? Is there any concept of FSE currently in WINE that we could reference as prior art for a more full implementation here (even if we step towards that incrementally)?
Although maybe those questions specifically are more appropriate for patch 3/3's thread. Thanks, Liam Middlebrook [0] - https://www.khronos.org/registry/vulkan/specs/1.2-extensions/html/vkspec.htm... [1] - https://www.winehq.org/pipermail/wine-devel/2019-October/153447.html
- Joshie 🐸✨
On 10/13/20 10:35 PM, Zebediah Figura wrote: > Why not do this the normal way, i.e. by modifying the code in > winex11 > and winemac? > > Also, what's the point of faking the extension like this? Why not > just > pass it through? >
- Joshie 🐸✨ On 10/14/20 2:05 AM, Liam Middlebrook wrote:
On 10/13/20 5:10 PM, Joshua Ashton wrote:
To add on about the layer thing, not removing random unsupported pNext elements has and will break layers like RenderDoc and OBS and potentially drivers down the line.
The typical pattern I see is if (info->pNext->sType == VK_STRUCTURE_TYPE_MY_UNSUPPORTED_STRUCTURE_FROG) { #ifdef _WIN32 // Handle whatever this is... info->pNext->... #else // Uh oh! Something went wrong here... abort(1); #endif }
The problem here is that the structure type is still defined in vulkan_core.h despite the structs, etc being defined in vulkan_win32.h which causes this pattern.
Many Layers (and potentially drivers) do stuff this, and not filtering out the pNexts will break them.
As noted below, the Vulkan specification covers this behavior. If observed behavior contradicts this, then there is a bug.
- Joshie 🐸✨
On 10/14/20 1:02 AM, Joshua Ashton wrote:
Zeb, it's in vulkan_win32.h. You can't include it on Linux or any other platform for that matter. If you read the xml, it's marked as being `platform="win32"`. It's a Windows extension.
To move it out of there on a Vulkan level, we'd need to make a sequel and different platform-specific extensions. Inherently making this version unsupportable in non-Windows drivers.
It makes more sense to read the line of VK_KHR_win32_surface support more as if you want surfaces (ie. not headless) than focusing on the platform part of things.
Could you please keep replies inline, top-posting makes it hard to correlate replies to contexts (although I figured it out in this case)?
- Joshie 🐸✨
On 10/14/20 12:37 AM, Georg Lehmann wrote:
On 14.10.20 01:23, Zebediah Figura wrote:
On 10/13/20 4:54 PM, Joshua Ashton wrote:
Figured I should add the reason why, is because just passing it through will cause the device to fail to be created because nobody supports VK_EXT_full_screen_exclusive on Linux. Sure, but they could.
It's an entirely Windows-centric extension. Well, no, it's not really, but it does include some API-level integration with win32 monitors *if* KHR_win32_surface is supported. In that case, as far as I can see, we don't even need to do anything, because the VkSurfaceFullScreenExclusiveWin32InfoEXT structure would just be ignored.
No, according to the vulkan spec a VkSurfaceFullScreenExclusiveWin32InfoEXT is illegal in any chain unless the full screen + win32 surface exts are enabled. In practice, this will break layers like renderdoc. So we would still need code to remove that struct from pNext chains.
Where are you seeing this (please cite the specification text/section specifically)? Pulling up the latest spec [0] I see the following under "Valid Usage for Structure Pointer Chains":
Any component of the implementation (the loader, any enabled layers, and drivers) must skip over, without processing (other than reading the sType and pNext members) any extending structures in the chain not defined by core versions or extensions supported by that component.
Which I understand as, filtering of these pNext chains is not necessary. Sorry for any past confusion where I may have implied otherwise.
Although this is true, we have had issues wrt. RenderDoc and other layers before and unsupported pNexts being passed in. I'd imagine other layers also have problems, especially ones that do struct/chain conversion. I think pretending that broken layers, etc don't exist is, although pleasant, not realistic.
Georg
Not that this necessarily means that lower-level drivers should be the ones to implement the extension, but if nothing else, this information isn't present at all in the patch.
I had replied to v2 of this patch before I got to this thread. I think this could be resolved with sufficient comments/documentation around this concept of faked extensions.
Also, do we really need a generic mechanism for this, wired into make_vulkan? Can't we just handle this extension specially in wine_vk_device_convert_create_info()?
Having a generated solution for extensions which cannot be supported by the underlying ICD would be useful in other cases also. Looking back at a patchset from about a year ago, Derek Lesho had a proof-of-concept which implemented memory import/export [1], it had to manually replace the extension name for VK_KHR_external_memory_win32 with VK_KHR_external_memory_fd.
Having that kind of manual step isn't inherently bad, but I'd think it could be done in a cleaner way with having a generator do the work. That said, I think if it's decided to go with a faking mechanism for device extensions, that a mechanism for instance extensions should be "on the horizon".
VK_KHR_external_memory_win32 and VK_KHR_external_memory_fd are device extensions so they could be implemented using this new fake system in winevulkan (it would be trivial to add a replacement step instead) It should be possible to extend it to instance extensions though...
- Joshie 🐸✨
On 10/13/20 10:42 PM, Joshua Ashton wrote: > winex11/winemac only does this for instance extensions. > > VK_EXT_full_screen_exclusive is a device extension.
It seems like winex11/winemac are the correct place for WSI related extensions though, and that the implementation from patch 3/3 should take place in there.
In my original review of patch 3/3 (then 5/5) I asked a few questions about your thought process here:
I agree with Zebediah that this should also include changes to winex11 and winemac. Is your plan currently that because fullscreen exclusive isn't a thing on Linux, we should just say it's supported and that's good enough? Is there any concept of FSE currently in WINE that we could reference as prior art for a more full implementation here (even if we step towards that incrementally)?
Although maybe those questions specifically are more appropriate for patch 3/3's thread.
winex11.drv doesn't touch anything related to devices or device extensions. It also doesn't can't hook any functions that aren't exported by the loader. It being implemented in winevulkan eliminates what essentially would end up being a duplicate implementation in winemac.drv also. - Joshie 🐸✨
Thanks,
Liam Middlebrook
[0] - https://www.khronos.org/registry/vulkan/specs/1.2-extensions/html/vkspec.htm...
[1] - https://www.winehq.org/pipermail/wine-devel/2019-October/153447.html
> > - Joshie 🐸✨ > > On 10/13/20 10:35 PM, Zebediah Figura wrote: >> Why not do this the normal way, i.e. by modifying the code in >> winex11 >> and winemac? >> >> Also, what's the point of faking the extension like this? Why >> not just >> pass it through? >>
On 10/13/20 6:31 PM, Joshua Ashton wrote:
- Joshie 🐸✨
On 10/14/20 2:05 AM, Liam Middlebrook wrote:
On 10/13/20 5:10 PM, Joshua Ashton wrote:
To add on about the layer thing, not removing random unsupported pNext elements has and will break layers like RenderDoc and OBS and potentially drivers down the line.
The typical pattern I see is if (info->pNext->sType == VK_STRUCTURE_TYPE_MY_UNSUPPORTED_STRUCTURE_FROG) { #ifdef _WIN32 // Handle whatever this is... info->pNext->... #else // Uh oh! Something went wrong here... abort(1); #endif }
The problem here is that the structure type is still defined in vulkan_core.h despite the structs, etc being defined in vulkan_win32.h which causes this pattern.
Many Layers (and potentially drivers) do stuff this, and not filtering out the pNexts will break them.
As noted below, the Vulkan specification covers this behavior. If observed behavior contradicts this, then there is a bug.
- Joshie 🐸✨
On 10/14/20 1:02 AM, Joshua Ashton wrote:
Zeb, it's in vulkan_win32.h. You can't include it on Linux or any other platform for that matter. If you read the xml, it's marked as being `platform="win32"`. It's a Windows extension.
To move it out of there on a Vulkan level, we'd need to make a sequel and different platform-specific extensions. Inherently making this version unsupportable in non-Windows drivers.
It makes more sense to read the line of VK_KHR_win32_surface support more as if you want surfaces (ie. not headless) than focusing on the platform part of things.
Could you please keep replies inline, top-posting makes it hard to correlate replies to contexts (although I figured it out in this case)?
- Joshie 🐸✨
On 10/14/20 12:37 AM, Georg Lehmann wrote:
On 14.10.20 01:23, Zebediah Figura wrote:
On 10/13/20 4:54 PM, Joshua Ashton wrote: > Figured I should add the reason why, is because just passing it > through > will cause the device to fail to be created because nobody supports > VK_EXT_full_screen_exclusive on Linux. Sure, but they could.
> It's an entirely Windows-centric extension. Well, no, it's not really, but it does include some API-level integration with win32 monitors *if* KHR_win32_surface is supported. In that case, as far as I can see, we don't even need to do anything, because the VkSurfaceFullScreenExclusiveWin32InfoEXT structure would just be ignored.
No, according to the vulkan spec a VkSurfaceFullScreenExclusiveWin32InfoEXT is illegal in any chain unless the full screen + win32 surface exts are enabled. In practice, this will break layers like renderdoc. So we would still need code to remove that struct from pNext chains.
Where are you seeing this (please cite the specification text/section specifically)? Pulling up the latest spec [0] I see the following under "Valid Usage for Structure Pointer Chains":
Any component of the implementation (the loader, any enabled layers, and drivers) must skip over, without processing (other than reading the sType and pNext members) any extending structures in the chain not defined by core versions or extensions supported by that component.
Which I understand as, filtering of these pNext chains is not necessary. Sorry for any past confusion where I may have implied otherwise.
Although this is true, we have had issues wrt. RenderDoc and other layers before and unsupported pNexts being passed in.
I'd imagine other layers also have problems, especially ones that do struct/chain conversion.
I think pretending that broken layers, etc don't exist is, although pleasant, not realistic.
Then let's fix and report bugs when they are found? Pretending for a moment that this filtering was desired/required. Doing proper filtering of pNext chains won't be a small endeavor. We can't just prune an entire pNext tree and hope for the best.
Georg
Not that this necessarily means that lower-level drivers should be the ones to implement the extension, but if nothing else, this information isn't present at all in the patch.
I had replied to v2 of this patch before I got to this thread. I think this could be resolved with sufficient comments/documentation around this concept of faked extensions.
Also, do we really need a generic mechanism for this, wired into make_vulkan? Can't we just handle this extension specially in wine_vk_device_convert_create_info()?
Having a generated solution for extensions which cannot be supported by the underlying ICD would be useful in other cases also. Looking back at a patchset from about a year ago, Derek Lesho had a proof-of-concept which implemented memory import/export [1], it had to manually replace the extension name for VK_KHR_external_memory_win32 with VK_KHR_external_memory_fd.
Having that kind of manual step isn't inherently bad, but I'd think it could be done in a cleaner way with having a generator do the work. That said, I think if it's decided to go with a faking mechanism for device extensions, that a mechanism for instance extensions should be "on the horizon".
VK_KHR_external_memory_win32 and VK_KHR_external_memory_fd are device extensions so they could be implemented using this new fake system in winevulkan (it would be trivial to add a replacement step instead)
It should be possible to extend it to instance extensions though...
> - Joshie 🐸✨ > > On 10/13/20 10:42 PM, Joshua Ashton wrote: >> winex11/winemac only does this for instance extensions. >> >> VK_EXT_full_screen_exclusive is a device extension.
It seems like winex11/winemac are the correct place for WSI related extensions though, and that the implementation from patch 3/3 should take place in there.
In my original review of patch 3/3 (then 5/5) I asked a few questions about your thought process here:
I agree with Zebediah that this should also include changes to winex11 and winemac. Is your plan currently that because fullscreen exclusive isn't a thing on Linux, we should just say it's supported and that's good enough? Is there any concept of FSE currently in WINE that we could reference as prior art for a more full implementation here (even if we step towards that incrementally)?
Although maybe those questions specifically are more appropriate for patch 3/3's thread.
winex11.drv doesn't touch anything related to devices or device extensions. It also doesn't can't hook any functions that aren't exported by the loader.
Just because code does not yet exist, doesn't mean that it should not exist.
It being implemented in winevulkan eliminates what essentially would end up being a duplicate implementation in winemac.drv also.
Yes, that's my understanding also. There will always be some amount of code duplication when implementing support for a common API across diverging platforms. Thanks, Liam Middlebrook
- Joshie 🐸✨
Thanks,
Liam Middlebrook
[0] - https://www.khronos.org/registry/vulkan/specs/1.2-extensions/html/vkspec.htm...
[1] - https://www.winehq.org/pipermail/wine-devel/2019-October/153447.html
>> >> - Joshie 🐸✨ >> >> On 10/13/20 10:35 PM, Zebediah Figura wrote: >>> Why not do this the normal way, i.e. by modifying the code in >>> winex11 >>> and winemac? >>> >>> Also, what's the point of faking the extension like this? Why >>> not just >>> pass it through? >>>
On 10/14/20 2:44 AM, Liam Middlebrook wrote:
On 10/13/20 6:31 PM, Joshua Ashton wrote:
- Joshie 🐸✨
On 10/14/20 2:05 AM, Liam Middlebrook wrote:
On 10/13/20 5:10 PM, Joshua Ashton wrote:
To add on about the layer thing, not removing random unsupported pNext elements has and will break layers like RenderDoc and OBS and potentially drivers down the line.
The typical pattern I see is if (info->pNext->sType == VK_STRUCTURE_TYPE_MY_UNSUPPORTED_STRUCTURE_FROG) { #ifdef _WIN32 // Handle whatever this is... info->pNext->... #else // Uh oh! Something went wrong here... abort(1); #endif }
The problem here is that the structure type is still defined in vulkan_core.h despite the structs, etc being defined in vulkan_win32.h which causes this pattern.
Many Layers (and potentially drivers) do stuff this, and not filtering out the pNexts will break them.
As noted below, the Vulkan specification covers this behavior. If observed behavior contradicts this, then there is a bug.
- Joshie 🐸✨
On 10/14/20 1:02 AM, Joshua Ashton wrote:
Zeb, it's in vulkan_win32.h. You can't include it on Linux or any other platform for that matter. If you read the xml, it's marked as being `platform="win32"`. It's a Windows extension.
To move it out of there on a Vulkan level, we'd need to make a sequel and different platform-specific extensions. Inherently making this version unsupportable in non-Windows drivers.
It makes more sense to read the line of VK_KHR_win32_surface support more as if you want surfaces (ie. not headless) than focusing on the platform part of things.
Could you please keep replies inline, top-posting makes it hard to correlate replies to contexts (although I figured it out in this case)?
- Joshie 🐸✨
On 10/14/20 12:37 AM, Georg Lehmann wrote:
On 14.10.20 01:23, Zebediah Figura wrote: > On 10/13/20 4:54 PM, Joshua Ashton wrote: >> Figured I should add the reason why, is because just passing it >> through >> will cause the device to fail to be created because nobody supports >> VK_EXT_full_screen_exclusive on Linux. > Sure, but they could. > >> It's an entirely Windows-centric extension. > Well, no, it's not really, but it does include some API-level > integration with win32 monitors *if* KHR_win32_surface is > supported. In > that case, as far as I can see, we don't even need to do anything, > because the VkSurfaceFullScreenExclusiveWin32InfoEXT structure would > just be ignored.
No, according to the vulkan spec a VkSurfaceFullScreenExclusiveWin32InfoEXT is illegal in any chain unless the full screen + win32 surface exts are enabled. In practice, this will break layers like renderdoc. So we would still need code to remove that struct from pNext chains.
Where are you seeing this (please cite the specification text/section specifically)? Pulling up the latest spec [0] I see the following under "Valid Usage for Structure Pointer Chains":
Any component of the implementation (the loader, any enabled layers, and drivers) must skip over, without processing (other than reading the sType and pNext members) any extending structures in the chain not defined by core versions or extensions supported by that component.
Which I understand as, filtering of these pNext chains is not necessary. Sorry for any past confusion where I may have implied otherwise.
Although this is true, we have had issues wrt. RenderDoc and other layers before and unsupported pNexts being passed in.
I'd imagine other layers also have problems, especially ones that do struct/chain conversion.
I think pretending that broken layers, etc don't exist is, although pleasant, not realistic.
Then let's fix and report bugs when they are found? Pretending for a moment that this filtering was desired/required. Doing proper filtering of pNext chains won't be a small endeavor. We can't just prune an entire pNext tree and hope for the best.
Georg
> Not that this necessarily means that lower-level drivers should > be the > ones to implement the extension, but if nothing else, this > information > isn't present at all in the patch.
I had replied to v2 of this patch before I got to this thread. I think this could be resolved with sufficient comments/documentation around this concept of faked extensions.
> > Also, do we really need a generic mechanism for this, wired into > make_vulkan? Can't we just handle this extension specially in > wine_vk_device_convert_create_info()?
Having a generated solution for extensions which cannot be supported by the underlying ICD would be useful in other cases also. Looking back at a patchset from about a year ago, Derek Lesho had a proof-of-concept which implemented memory import/export [1], it had to manually replace the extension name for VK_KHR_external_memory_win32 with VK_KHR_external_memory_fd.
Having that kind of manual step isn't inherently bad, but I'd think it could be done in a cleaner way with having a generator do the work. That said, I think if it's decided to go with a faking mechanism for device extensions, that a mechanism for instance extensions should be "on the horizon".
VK_KHR_external_memory_win32 and VK_KHR_external_memory_fd are device extensions so they could be implemented using this new fake system in winevulkan (it would be trivial to add a replacement step instead)
It should be possible to extend it to instance extensions though...
> >> - Joshie 🐸✨ >> >> On 10/13/20 10:42 PM, Joshua Ashton wrote: >>> winex11/winemac only does this for instance extensions. >>> >>> VK_EXT_full_screen_exclusive is a device extension.
It seems like winex11/winemac are the correct place for WSI related extensions though, and that the implementation from patch 3/3 should take place in there.
In my original review of patch 3/3 (then 5/5) I asked a few questions about your thought process here:
I agree with Zebediah that this should also include changes to winex11 and winemac. Is your plan currently that because fullscreen exclusive isn't a thing on Linux, we should just say it's supported and that's good enough? Is there any concept of FSE currently in WINE that we could reference as prior art for a more full implementation here (even if we step towards that incrementally)?
Although maybe those questions specifically are more appropriate for patch 3/3's thread.
winex11.drv doesn't touch anything related to devices or device extensions. It also doesn't can't hook any functions that aren't exported by the loader.
Just because code does not yet exist, doesn't mean that it should not exist.
I think it shouldn't exist. You're essentially advocating to have two systems that do the exact same thing in different places for no reason. Adding support for exporting non-loader functions to WineX11 is it's whole separate thing. (Even more-so for non-instance functions)
It being implemented in winevulkan eliminates what essentially would end up being a duplicate implementation in winemac.drv also.
Yes, that's my understanding also. There will always be some amount of code duplication when implementing support for a common API across diverging platforms.
That code duplication can be easily avoided however. Technically this isn't a 'WSI' extension in Vulkan terms as it isn't exported by the loader. It just really doesn't fit in winex11/winemac, to add a whole extension faking system in two places and also teach winex11/winemac about devices and device extensions and non-loader functions. This also means I'd need to implement this for winemac (for parity), I don't own a Mac, I have no plans to ever own a Mac or develop software for Mac again. It's just a complete waste of time and effort when you can get both here for free here with 10000% less effort (the hard part is teaching the wine drvs about non-instance funcs and non-loader funcs.) - Joshie 🐸✨
Thanks,
Liam Middlebrook
- Joshie 🐸✨
Thanks,
Liam Middlebrook
[0] - https://www.khronos.org/registry/vulkan/specs/1.2-extensions/html/vkspec.htm...
[1] - https://www.winehq.org/pipermail/wine-devel/2019-October/153447.html
>>> >>> - Joshie 🐸✨ >>> >>> On 10/13/20 10:35 PM, Zebediah Figura wrote: >>>> Why not do this the normal way, i.e. by modifying the code in >>>> winex11 >>>> and winemac? >>>> >>>> Also, what's the point of faking the extension like this? Why >>>> not just >>>> pass it through? >>>> >
- Joshie 🐸✨
On 10/13/20 8:05 PM, Liam Middlebrook wrote:
On 10/13/20 5:10 PM, Joshua Ashton wrote:
To add on about the layer thing, not removing random unsupported pNext elements has and will break layers like RenderDoc and OBS and potentially drivers down the line.
The typical pattern I see is if (info->pNext->sType == VK_STRUCTURE_TYPE_MY_UNSUPPORTED_STRUCTURE_FROG) { #ifdef _WIN32 // Handle whatever this is... info->pNext->... #else // Uh oh! Something went wrong here... abort(1); #endif }
The problem here is that the structure type is still defined in vulkan_core.h despite the structs, etc being defined in vulkan_win32.h which causes this pattern.
Many Layers (and potentially drivers) do stuff this, and not filtering out the pNexts will break them.
As noted below, the Vulkan specification covers this behavior. If observed behavior contradicts this, then there is a bug.
- Joshie 🐸✨
On 10/14/20 1:02 AM, Joshua Ashton wrote:
Zeb, it's in vulkan_win32.h. You can't include it on Linux or any other platform for that matter. If you read the xml, it's marked as being `platform="win32"`. It's a Windows extension.
To move it out of there on a Vulkan level, we'd need to make a sequel and different platform-specific extensions. Inherently making this version unsupportable in non-Windows drivers.
It makes more sense to read the line of VK_KHR_win32_surface support more as if you want surfaces (ie. not headless) than focusing on the platform part of things.
Could you please keep replies inline, top-posting makes it hard to correlate replies to contexts (although I figured it out in this case)?
- Joshie 🐸✨
On 10/14/20 12:37 AM, Georg Lehmann wrote:
On 14.10.20 01:23, Zebediah Figura wrote:
On 10/13/20 4:54 PM, Joshua Ashton wrote:
Figured I should add the reason why, is because just passing it through will cause the device to fail to be created because nobody supports VK_EXT_full_screen_exclusive on Linux. Sure, but they could.
It's an entirely Windows-centric extension. Well, no, it's not really, but it does include some API-level integration with win32 monitors *if* KHR_win32_surface is supported. In that case, as far as I can see, we don't even need to do anything, because the VkSurfaceFullScreenExclusiveWin32InfoEXT structure would just be ignored.
No, according to the vulkan spec a VkSurfaceFullScreenExclusiveWin32InfoEXT is illegal in any chain unless the full screen + win32 surface exts are enabled. In practice, this will break layers like renderdoc. So we would still need code to remove that struct from pNext chains.
Where are you seeing this (please cite the specification text/section specifically)? Pulling up the latest spec [0] I see the following under "Valid Usage for Structure Pointer Chains":
Any component of the implementation (the loader, any enabled layers, and drivers) must skip over, without processing (other than reading the sType and pNext members) any extending structures in the chain not defined by core versions or extensions supported by that component.
Which I understand as, filtering of these pNext chains is not necessary. Sorry for any past confusion where I may have implied otherwise.
Georg
Not that this necessarily means that lower-level drivers should be the ones to implement the extension, but if nothing else, this information isn't present at all in the patch.
I had replied to v2 of this patch before I got to this thread. I think this could be resolved with sufficient comments/documentation around this concept of faked extensions.
Also, do we really need a generic mechanism for this, wired into make_vulkan? Can't we just handle this extension specially in wine_vk_device_convert_create_info()?
Having a generated solution for extensions which cannot be supported by the underlying ICD would be useful in other cases also. Looking back at a patchset from about a year ago, Derek Lesho had a proof-of-concept which implemented memory import/export [1], it had to manually replace the extension name for VK_KHR_external_memory_win32 with VK_KHR_external_memory_fd.
Having that kind of manual step isn't inherently bad, but I'd think it could be done in a cleaner way with having a generator do the work. That said, I think if it's decided to go with a faking mechanism for device extensions, that a mechanism for instance extensions should be "on the horizon".
I guess the point is it's not clear to me that a list like FAKED_EXTENSIONS = [ "EXT_foo", "EXT_bar", ] provides any benefit over const char *name = properties[i].extensionName; if (strcmp(name, "VK_EXT_foo") && strcmp(name, "VK_EXT_bar") { /* Copy the extension... */ } given that in both cases, adding a new extension to the list comprises only one line, and the former requires more support code. It makes sense to me to handle blacklisted extensions in make_vulkan, since we do multiple nontrivial things with the list, but we're only using this "faked extension" list in one place.
- Joshie 🐸✨
On 10/13/20 10:42 PM, Joshua Ashton wrote: > winex11/winemac only does this for instance extensions. > > VK_EXT_full_screen_exclusive is a device extension.
It seems like winex11/winemac are the correct place for WSI related extensions though, and that the implementation from patch 3/3 should take place in there.
In my original review of patch 3/3 (then 5/5) I asked a few questions about your thought process here:
I agree with Zebediah that this should also include changes to winex11 and winemac. Is your plan currently that because fullscreen exclusive isn't a thing on Linux, we should just say it's supported and that's good enough? Is there any concept of FSE currently in WINE that we could reference as prior art for a more full implementation here (even if we step towards that incrementally)?
Although maybe those questions specifically are more appropriate for patch 3/3's thread.
Thanks,
Liam Middlebrook
[0] - https://www.khronos.org/registry/vulkan/specs/1.2-extensions/html/vkspec.htm...
[1] - https://www.winehq.org/pipermail/wine-devel/2019-October/153447.html
> > - Joshie 🐸✨ > > On 10/13/20 10:35 PM, Zebediah Figura wrote: >> Why not do this the normal way, i.e. by modifying the code in >> winex11 >> and winemac? >> >> Also, what's the point of faking the extension like this? Why >> not just >> pass it through? >>
On 10/14/20 2:50 AM, Zebediah Figura wrote:
On 10/13/20 8:05 PM, Liam Middlebrook wrote:
On 10/13/20 5:10 PM, Joshua Ashton wrote:
To add on about the layer thing, not removing random unsupported pNext elements has and will break layers like RenderDoc and OBS and potentially drivers down the line.
The typical pattern I see is if (info->pNext->sType == VK_STRUCTURE_TYPE_MY_UNSUPPORTED_STRUCTURE_FROG) { #ifdef _WIN32 // Handle whatever this is... info->pNext->... #else // Uh oh! Something went wrong here... abort(1); #endif }
The problem here is that the structure type is still defined in vulkan_core.h despite the structs, etc being defined in vulkan_win32.h which causes this pattern.
Many Layers (and potentially drivers) do stuff this, and not filtering out the pNexts will break them.
As noted below, the Vulkan specification covers this behavior. If observed behavior contradicts this, then there is a bug.
- Joshie 🐸✨
On 10/14/20 1:02 AM, Joshua Ashton wrote:
Zeb, it's in vulkan_win32.h. You can't include it on Linux or any other platform for that matter. If you read the xml, it's marked as being `platform="win32"`. It's a Windows extension.
To move it out of there on a Vulkan level, we'd need to make a sequel and different platform-specific extensions. Inherently making this version unsupportable in non-Windows drivers.
It makes more sense to read the line of VK_KHR_win32_surface support more as if you want surfaces (ie. not headless) than focusing on the platform part of things.
Could you please keep replies inline, top-posting makes it hard to correlate replies to contexts (although I figured it out in this case)?
- Joshie 🐸✨
On 10/14/20 12:37 AM, Georg Lehmann wrote:
On 14.10.20 01:23, Zebediah Figura wrote:
On 10/13/20 4:54 PM, Joshua Ashton wrote: > Figured I should add the reason why, is because just passing it > through > will cause the device to fail to be created because nobody supports > VK_EXT_full_screen_exclusive on Linux. Sure, but they could.
> It's an entirely Windows-centric extension. Well, no, it's not really, but it does include some API-level integration with win32 monitors *if* KHR_win32_surface is supported. In that case, as far as I can see, we don't even need to do anything, because the VkSurfaceFullScreenExclusiveWin32InfoEXT structure would just be ignored.
No, according to the vulkan spec a VkSurfaceFullScreenExclusiveWin32InfoEXT is illegal in any chain unless the full screen + win32 surface exts are enabled. In practice, this will break layers like renderdoc. So we would still need code to remove that struct from pNext chains.
Where are you seeing this (please cite the specification text/section specifically)? Pulling up the latest spec [0] I see the following under "Valid Usage for Structure Pointer Chains":
Any component of the implementation (the loader, any enabled layers, and drivers) must skip over, without processing (other than reading the sType and pNext members) any extending structures in the chain not defined by core versions or extensions supported by that component.
Which I understand as, filtering of these pNext chains is not necessary. Sorry for any past confusion where I may have implied otherwise.
Georg
Not that this necessarily means that lower-level drivers should be the ones to implement the extension, but if nothing else, this information isn't present at all in the patch.
I had replied to v2 of this patch before I got to this thread. I think this could be resolved with sufficient comments/documentation around this concept of faked extensions.
Also, do we really need a generic mechanism for this, wired into make_vulkan? Can't we just handle this extension specially in wine_vk_device_convert_create_info()?
Having a generated solution for extensions which cannot be supported by the underlying ICD would be useful in other cases also. Looking back at a patchset from about a year ago, Derek Lesho had a proof-of-concept which implemented memory import/export [1], it had to manually replace the extension name for VK_KHR_external_memory_win32 with VK_KHR_external_memory_fd.
Having that kind of manual step isn't inherently bad, but I'd think it could be done in a cleaner way with having a generator do the work. That said, I think if it's decided to go with a faking mechanism for device extensions, that a mechanism for instance extensions should be "on the horizon".
I guess the point is it's not clear to me that a list like
FAKED_EXTENSIONS = [ "EXT_foo", "EXT_bar", ]
provides any benefit over
const char *name = properties[i].extensionName;
if (strcmp(name, "VK_EXT_foo") && strcmp(name, "VK_EXT_bar") { /* Copy the extension... */ }
given that in both cases, adding a new extension to the list comprises only one line, and the former requires more support code.
It makes sense to me to handle blacklisted extensions in make_vulkan, since we do multiple nontrivial things with the list, but we're only using this "faked extension" list in one place.
We aren't using it in one place, we're using it in two places, once for the device create info fixup to remove them being passed through and the other for reporting whether device extensions are supported. - Joshie 🐸✨
> - Joshie 🐸✨ > > On 10/13/20 10:42 PM, Joshua Ashton wrote: >> winex11/winemac only does this for instance extensions. >> >> VK_EXT_full_screen_exclusive is a device extension.
It seems like winex11/winemac are the correct place for WSI related extensions though, and that the implementation from patch 3/3 should take place in there.
In my original review of patch 3/3 (then 5/5) I asked a few questions about your thought process here:
I agree with Zebediah that this should also include changes to winex11 and winemac. Is your plan currently that because fullscreen exclusive isn't a thing on Linux, we should just say it's supported and that's good enough? Is there any concept of FSE currently in WINE that we could reference as prior art for a more full implementation here (even if we step towards that incrementally)?
Although maybe those questions specifically are more appropriate for patch 3/3's thread.
Thanks,
Liam Middlebrook
[0] - https://www.khronos.org/registry/vulkan/specs/1.2-extensions/html/vkspec.htm...
[1] - https://www.winehq.org/pipermail/wine-devel/2019-October/153447.html
>> >> - Joshie 🐸✨ >> >> On 10/13/20 10:35 PM, Zebediah Figura wrote: >>> Why not do this the normal way, i.e. by modifying the code in >>> winex11 >>> and winemac? >>> >>> Also, what's the point of faking the extension like this? Why >>> not just >>> pass it through? >>>
On 10/13/20 8:56 PM, Joshua Ashton wrote:
On 10/14/20 2:50 AM, Zebediah Figura wrote:
On 10/13/20 8:05 PM, Liam Middlebrook wrote:
On 10/13/20 5:10 PM, Joshua Ashton wrote:
To add on about the layer thing, not removing random unsupported pNext elements has and will break layers like RenderDoc and OBS and potentially drivers down the line.
The typical pattern I see is if (info->pNext->sType == VK_STRUCTURE_TYPE_MY_UNSUPPORTED_STRUCTURE_FROG) { #ifdef _WIN32 // Handle whatever this is... info->pNext->... #else // Uh oh! Something went wrong here... abort(1); #endif }
The problem here is that the structure type is still defined in vulkan_core.h despite the structs, etc being defined in vulkan_win32.h which causes this pattern.
Many Layers (and potentially drivers) do stuff this, and not filtering out the pNexts will break them.
As noted below, the Vulkan specification covers this behavior. If observed behavior contradicts this, then there is a bug.
- Joshie 🐸✨
On 10/14/20 1:02 AM, Joshua Ashton wrote:
Zeb, it's in vulkan_win32.h. You can't include it on Linux or any other platform for that matter. If you read the xml, it's marked as being `platform="win32"`. It's a Windows extension.
To move it out of there on a Vulkan level, we'd need to make a sequel and different platform-specific extensions. Inherently making this version unsupportable in non-Windows drivers.
It makes more sense to read the line of VK_KHR_win32_surface support more as if you want surfaces (ie. not headless) than focusing on the platform part of things.
Could you please keep replies inline, top-posting makes it hard to correlate replies to contexts (although I figured it out in this case)?
- Joshie 🐸✨
On 10/14/20 12:37 AM, Georg Lehmann wrote:
On 14.10.20 01:23, Zebediah Figura wrote: > On 10/13/20 4:54 PM, Joshua Ashton wrote: >> Figured I should add the reason why, is because just passing it >> through >> will cause the device to fail to be created because nobody supports >> VK_EXT_full_screen_exclusive on Linux. > Sure, but they could. > >> It's an entirely Windows-centric extension. > Well, no, it's not really, but it does include some API-level > integration with win32 monitors *if* KHR_win32_surface is > supported. In > that case, as far as I can see, we don't even need to do anything, > because the VkSurfaceFullScreenExclusiveWin32InfoEXT structure would > just be ignored.
No, according to the vulkan spec a VkSurfaceFullScreenExclusiveWin32InfoEXT is illegal in any chain unless the full screen + win32 surface exts are enabled. In practice, this will break layers like renderdoc. So we would still need code to remove that struct from pNext chains.
Where are you seeing this (please cite the specification text/section specifically)? Pulling up the latest spec [0] I see the following under "Valid Usage for Structure Pointer Chains":
Any component of the implementation (the loader, any enabled layers, and drivers) must skip over, without processing (other than reading the sType and pNext members) any extending structures in the chain not defined by core versions or extensions supported by that component.
Which I understand as, filtering of these pNext chains is not necessary. Sorry for any past confusion where I may have implied otherwise.
Georg
> Not that this necessarily means that lower-level drivers should > be the > ones to implement the extension, but if nothing else, this > information > isn't present at all in the patch.
I had replied to v2 of this patch before I got to this thread. I think this could be resolved with sufficient comments/documentation around this concept of faked extensions.
> > Also, do we really need a generic mechanism for this, wired into > make_vulkan? Can't we just handle this extension specially in > wine_vk_device_convert_create_info()?
Having a generated solution for extensions which cannot be supported by the underlying ICD would be useful in other cases also. Looking back at a patchset from about a year ago, Derek Lesho had a proof-of-concept which implemented memory import/export [1], it had to manually replace the extension name for VK_KHR_external_memory_win32 with VK_KHR_external_memory_fd.
Having that kind of manual step isn't inherently bad, but I'd think it could be done in a cleaner way with having a generator do the work. That said, I think if it's decided to go with a faking mechanism for device extensions, that a mechanism for instance extensions should be "on the horizon".
I guess the point is it's not clear to me that a list like
FAKED_EXTENSIONS = [ "EXT_foo", "EXT_bar", ]
provides any benefit over
const char *name = properties[i].extensionName;
if (strcmp(name, "VK_EXT_foo") && strcmp(name, "VK_EXT_bar") { /* Copy the extension... */ }
given that in both cases, adding a new extension to the list comprises only one line, and the former requires more support code.
It makes sense to me to handle blacklisted extensions in make_vulkan, since we do multiple nontrivial things with the list, but we're only using this "faked extension" list in one place.
We aren't using it in one place, we're using it in two places, once for the device create info fixup to remove them being passed through and the other for reporting whether device extensions are supported.
Sorry, I missed that. In that case it seems the simpler solution is to have a static const array of strings in vulkan.c; make_vulkan still doesn't need to get involved.
- Joshie 🐸✨
> >> - Joshie 🐸✨ >> >> On 10/13/20 10:42 PM, Joshua Ashton wrote: >>> winex11/winemac only does this for instance extensions. >>> >>> VK_EXT_full_screen_exclusive is a device extension.
It seems like winex11/winemac are the correct place for WSI related extensions though, and that the implementation from patch 3/3 should take place in there.
In my original review of patch 3/3 (then 5/5) I asked a few questions about your thought process here:
I agree with Zebediah that this should also include changes to winex11 and winemac. Is your plan currently that because fullscreen exclusive isn't a thing on Linux, we should just say it's supported and that's good enough? Is there any concept of FSE currently in WINE that we could reference as prior art for a more full implementation here (even if we step towards that incrementally)?
Although maybe those questions specifically are more appropriate for patch 3/3's thread.
Thanks,
Liam Middlebrook
[0] - https://www.khronos.org/registry/vulkan/specs/1.2-extensions/html/vkspec.htm...
[1] - https://www.winehq.org/pipermail/wine-devel/2019-October/153447.html
>>> >>> - Joshie 🐸✨ >>> >>> On 10/13/20 10:35 PM, Zebediah Figura wrote: >>>> Why not do this the normal way, i.e. by modifying the code in >>>> winex11 >>>> and winemac? >>>> >>>> Also, what's the point of faking the extension like this? Why >>>> not just >>>> pass it through? >>>> >
On 14.10.20 03:05, Liam Middlebrook wrote:
On 10/13/20 5:10 PM, Joshua Ashton wrote:
To add on about the layer thing, not removing random unsupported pNext elements has and will break layers like RenderDoc and OBS and potentially drivers down the line.
The typical pattern I see is if (info->pNext->sType == VK_STRUCTURE_TYPE_MY_UNSUPPORTED_STRUCTURE_FROG) { #ifdef _WIN32 // Handle whatever this is... info->pNext->... #else // Uh oh! Something went wrong here... abort(1); #endif }
The problem here is that the structure type is still defined in vulkan_core.h despite the structs, etc being defined in vulkan_win32.h which causes this pattern.
Many Layers (and potentially drivers) do stuff this, and not filtering out the pNexts will break them.
As noted below, the Vulkan specification covers this behavior. If observed behavior contradicts this, then there is a bug.
- Joshie 🐸✨
On 10/14/20 1:02 AM, Joshua Ashton wrote:
Zeb, it's in vulkan_win32.h. You can't include it on Linux or any other platform for that matter. If you read the xml, it's marked as being `platform="win32"`. It's a Windows extension.
To move it out of there on a Vulkan level, we'd need to make a sequel and different platform-specific extensions. Inherently making this version unsupportable in non-Windows drivers.
It makes more sense to read the line of VK_KHR_win32_surface support more as if you want surfaces (ie. not headless) than focusing on the platform part of things.
Could you please keep replies inline, top-posting makes it hard to correlate replies to contexts (although I figured it out in this case)?
- Joshie 🐸✨
On 10/14/20 12:37 AM, Georg Lehmann wrote:
On 14.10.20 01:23, Zebediah Figura wrote:
On 10/13/20 4:54 PM, Joshua Ashton wrote:
Figured I should add the reason why, is because just passing it through will cause the device to fail to be created because nobody supports VK_EXT_full_screen_exclusive on Linux. Sure, but they could.
It's an entirely Windows-centric extension. Well, no, it's not really, but it does include some API-level integration with win32 monitors *if* KHR_win32_surface is supported. In that case, as far as I can see, we don't even need to do anything, because the VkSurfaceFullScreenExclusiveWin32InfoEXT structure would just be ignored.
No, according to the vulkan spec a VkSurfaceFullScreenExclusiveWin32InfoEXT is illegal in any chain unless the full screen + win32 surface exts are enabled. In practice, this will break layers like renderdoc. So we would still need code to remove that struct from pNext chains.
Where are you seeing this (please cite the specification text/section specifically)? Pulling up the latest spec [0] I see the following under "Valid Usage for Structure Pointer Chains":
Any component of the implementation (the loader, any enabled layers, and drivers) must skip over, without processing (other than reading the sType and pNext members) any extending structures in the chain not defined by core versions or extensions supported by that component.
Which I understand as, filtering of these pNext chains is not necessary. Sorry for any past confusion where I may have implied otherwise.
You are right, the other components are required to ignored unknown structure types. But the spec in "Valid Usage for Structure Pointer Chains" also says:
Each structure present in the pNext chain *must* be defined at runtime by either: - a core version which is supported - an extension which is enabled - a supported device extension in the case of physical-device-level functionality added by the device extension
VkSurfaceFullScreenExclusiveWin32InfoEXT isn't added by an extension that's supported, so it's not allowed in any chain. Therefore if we don't filter out the struct, this is invalid application behavior. Georg
Georg
Not that this necessarily means that lower-level drivers should be the ones to implement the extension, but if nothing else, this information isn't present at all in the patch.
I had replied to v2 of this patch before I got to this thread. I think this could be resolved with sufficient comments/documentation around this concept of faked extensions.
Also, do we really need a generic mechanism for this, wired into make_vulkan? Can't we just handle this extension specially in wine_vk_device_convert_create_info()?
Having a generated solution for extensions which cannot be supported by the underlying ICD would be useful in other cases also. Looking back at a patchset from about a year ago, Derek Lesho had a proof-of-concept which implemented memory import/export [1], it had to manually replace the extension name for VK_KHR_external_memory_win32 with VK_KHR_external_memory_fd.
Having that kind of manual step isn't inherently bad, but I'd think it could be done in a cleaner way with having a generator do the work. That said, I think if it's decided to go with a faking mechanism for device extensions, that a mechanism for instance extensions should be "on the horizon".
- Joshie 🐸✨
On 10/13/20 10:42 PM, Joshua Ashton wrote: > winex11/winemac only does this for instance extensions. > > VK_EXT_full_screen_exclusive is a device extension.
It seems like winex11/winemac are the correct place for WSI related extensions though, and that the implementation from patch 3/3 should take place in there.
In my original review of patch 3/3 (then 5/5) I asked a few questions about your thought process here:
I agree with Zebediah that this should also include changes to winex11 and winemac. Is your plan currently that because fullscreen exclusive isn't a thing on Linux, we should just say it's supported and that's good enough? Is there any concept of FSE currently in WINE that we could reference as prior art for a more full implementation here (even if we step towards that incrementally)?
Although maybe those questions specifically are more appropriate for patch 3/3's thread.
Thanks,
Liam Middlebrook
[0] - https://www.khronos.org/registry/vulkan/specs/1.2-extensions/html/vkspec.htm... [1] - https://www.winehq.org/pipermail/wine-devel/2019-October/153447.html
> > - Joshie 🐸✨ > > On 10/13/20 10:35 PM, Zebediah Figura wrote: >> Why not do this the normal way, i.e. by modifying the code in >> winex11 >> and winemac? >> >> Also, what's the point of faking the extension like this? Why >> not just >> pass it through? >>
On 10/13/20 7:02 PM, Joshua Ashton wrote:
Zeb, it's in vulkan_win32.h. You can't include it on Linux or any other platform for that matter. If you read the xml, it's marked as being `platform="win32"`. It's a Windows extension.
To move it out of there on a Vulkan level, we'd need to make a sequel and different platform-specific extensions. Inherently making this version unsupportable in non-Windows drivers.
It makes more sense to read the line of VK_KHR_win32_surface support more as if you want surfaces (ie. not headless) than focusing on the platform part of things.
Can you please explain why this is the case? The extension seems to have been intentionally written in a platform-agnostic manner—except for the parts which interact with platform-specific extensions, and those parts are pretty clearly described as optional. If "platform=win32" has relevance to the API, and can't be changed without breaking backwards-compatibility, this fact should really be reflected in the documentation. As it is, it's not obvious to me that this problem can't be resolved by simply moving those definitions out of the "vulkan_win32.h" header and into "vulkan_core.h". Perhaps there's places that I haven't found to look, though...
- Joshie 🐸✨
On 10/14/20 12:37 AM, Georg Lehmann wrote:
On 14.10.20 01:23, Zebediah Figura wrote:
On 10/13/20 4:54 PM, Joshua Ashton wrote:
Figured I should add the reason why, is because just passing it through will cause the device to fail to be created because nobody supports VK_EXT_full_screen_exclusive on Linux. Sure, but they could.
It's an entirely Windows-centric extension. Well, no, it's not really, but it does include some API-level integration with win32 monitors *if* KHR_win32_surface is supported. In that case, as far as I can see, we don't even need to do anything, because the VkSurfaceFullScreenExclusiveWin32InfoEXT structure would just be ignored.
No, according to the vulkan spec a VkSurfaceFullScreenExclusiveWin32InfoEXT is illegal in any chain unless the full screen + win32 surface exts are enabled. In practice, this will break layers like renderdoc. So we would still need code to remove that struct from pNext chains.
Georg
Not that this necessarily means that lower-level drivers should be the ones to implement the extension, but if nothing else, this information isn't present at all in the patch.
Also, do we really need a generic mechanism for this, wired into make_vulkan? Can't we just handle this extension specially in wine_vk_device_convert_create_info()?
- Joshie 🐸✨
On 10/13/20 10:42 PM, Joshua Ashton wrote:
winex11/winemac only does this for instance extensions.
VK_EXT_full_screen_exclusive is a device extension.
- Joshie 🐸✨
On 10/13/20 10:35 PM, Zebediah Figura wrote:
Why not do this the normal way, i.e. by modifying the code in winex11 and winemac?
Also, what's the point of faking the extension like this? Why not just pass it through?
On 10/14/20 2:42 AM, Zebediah Figura wrote:
On 10/13/20 7:02 PM, Joshua Ashton wrote:
Zeb, it's in vulkan_win32.h. You can't include it on Linux or any other platform for that matter. If you read the xml, it's marked as being `platform="win32"`. It's a Windows extension.
To move it out of there on a Vulkan level, we'd need to make a sequel and different platform-specific extensions. Inherently making this version unsupportable in non-Windows drivers.
It makes more sense to read the line of VK_KHR_win32_surface support more as if you want surfaces (ie. not headless) than focusing on the platform part of things.
Can you please explain why this is the case? The extension seems to have been intentionally written in a platform-agnostic manner—except for the parts which interact with platform-specific extensions, and those parts are pretty clearly described as optional.
If "platform=win32" has relevance to the API, and can't be changed without breaking backwards-compatibility, this fact should really be reflected in the documentation. As it is, it's not obvious to me that this problem can't be resolved by simply moving those definitions out of the "vulkan_win32.h" header and into "vulkan_core.h". Perhaps there's places that I haven't found to look, though...
It's more the fact that this extension was made wrong. The platform specific parts should be parts of their own extensions that are dependent on the base. Currently all platform-specific structures in other extensions are exposed this way (ie. win32_surface, and stuff.) This also plays into how the script to build the headers works. Also for some reason your emails tend to take like an hour to get to me through the mailing list but everyone else's is fine? I'm not sure why... - Joshie 🐸✨
- Joshie 🐸✨
On 10/14/20 12:37 AM, Georg Lehmann wrote:
On 14.10.20 01:23, Zebediah Figura wrote:
On 10/13/20 4:54 PM, Joshua Ashton wrote:
Figured I should add the reason why, is because just passing it through will cause the device to fail to be created because nobody supports VK_EXT_full_screen_exclusive on Linux. Sure, but they could.
It's an entirely Windows-centric extension. Well, no, it's not really, but it does include some API-level integration with win32 monitors *if* KHR_win32_surface is supported. In that case, as far as I can see, we don't even need to do anything, because the VkSurfaceFullScreenExclusiveWin32InfoEXT structure would just be ignored.
No, according to the vulkan spec a VkSurfaceFullScreenExclusiveWin32InfoEXT is illegal in any chain unless the full screen + win32 surface exts are enabled. In practice, this will break layers like renderdoc. So we would still need code to remove that struct from pNext chains.
Georg
Not that this necessarily means that lower-level drivers should be the ones to implement the extension, but if nothing else, this information isn't present at all in the patch.
Also, do we really need a generic mechanism for this, wired into make_vulkan? Can't we just handle this extension specially in wine_vk_device_convert_create_info()?
- Joshie 🐸✨
On 10/13/20 10:42 PM, Joshua Ashton wrote:
winex11/winemac only does this for instance extensions.
VK_EXT_full_screen_exclusive is a device extension.
- Joshie 🐸✨
On 10/13/20 10:35 PM, Zebediah Figura wrote: > Why not do this the normal way, i.e. by modifying the code in winex11 > and winemac? > > Also, what's the point of faking the extension like this? Why not > just > pass it through? >
On 10/13/20 9:34 PM, Joshua Ashton wrote:
On 10/14/20 2:42 AM, Zebediah Figura wrote:
On 10/13/20 7:02 PM, Joshua Ashton wrote:
Zeb, it's in vulkan_win32.h. You can't include it on Linux or any other platform for that matter. If you read the xml, it's marked as being `platform="win32"`. It's a Windows extension.
To move it out of there on a Vulkan level, we'd need to make a sequel and different platform-specific extensions. Inherently making this version unsupportable in non-Windows drivers.
It makes more sense to read the line of VK_KHR_win32_surface support more as if you want surfaces (ie. not headless) than focusing on the platform part of things.
Can you please explain why this is the case? The extension seems to have been intentionally written in a platform-agnostic manner—except for the parts which interact with platform-specific extensions, and those parts are pretty clearly described as optional.
If "platform=win32" has relevance to the API, and can't be changed without breaking backwards-compatibility, this fact should really be reflected in the documentation. As it is, it's not obvious to me that this problem can't be resolved by simply moving those definitions out of the "vulkan_win32.h" header and into "vulkan_core.h". Perhaps there's places that I haven't found to look, though...
It's more the fact that this extension was made wrong. The platform specific parts should be parts of their own extensions that are dependent on the base.
Currently all platform-specific structures in other extensions are exposed this way (ie. win32_surface, and stuff.)
This also plays into how the script to build the headers works.
Maybe so (though if what I said is correct, it's not obvious to me why there's anything fundamentally wrong with the design), but that's somewhat orthogonal to the point here. Provided the Vulkan headers are fixed so that non-win32 drivers can use the relevant types, is there any reason why this extension can't be implemented?
Also for some reason your emails tend to take like an hour to get to me through the mailing list but everyone else's is fine? I'm not sure why...
I can't think of anything particularly unusual I'm doing, but I'll keep it in mind in case I get more reports of this, thanks.
- Joshie 🐸✨
- Joshie 🐸✨
On 10/14/20 12:37 AM, Georg Lehmann wrote:
On 14.10.20 01:23, Zebediah Figura wrote:
On 10/13/20 4:54 PM, Joshua Ashton wrote:
Figured I should add the reason why, is because just passing it through will cause the device to fail to be created because nobody supports VK_EXT_full_screen_exclusive on Linux. Sure, but they could.
It's an entirely Windows-centric extension. Well, no, it's not really, but it does include some API-level integration with win32 monitors *if* KHR_win32_surface is supported. In that case, as far as I can see, we don't even need to do anything, because the VkSurfaceFullScreenExclusiveWin32InfoEXT structure would just be ignored.
No, according to the vulkan spec a VkSurfaceFullScreenExclusiveWin32InfoEXT is illegal in any chain unless the full screen + win32 surface exts are enabled. In practice, this will break layers like renderdoc. So we would still need code to remove that struct from pNext chains.
Georg
Not that this necessarily means that lower-level drivers should be the ones to implement the extension, but if nothing else, this information isn't present at all in the patch.
Also, do we really need a generic mechanism for this, wired into make_vulkan? Can't we just handle this extension specially in wine_vk_device_convert_create_info()?
- Joshie 🐸✨
On 10/13/20 10:42 PM, Joshua Ashton wrote: > winex11/winemac only does this for instance extensions. > > VK_EXT_full_screen_exclusive is a device extension. > > - Joshie 🐸✨ > > On 10/13/20 10:35 PM, Zebediah Figura wrote: >> Why not do this the normal way, i.e. by modifying the code in >> winex11 >> and winemac? >> >> Also, what's the point of faking the extension like this? Why not >> just >> pass it through? >>
On 10/14/20 3:45 AM, Zebediah Figura wrote:
On 10/13/20 9:34 PM, Joshua Ashton wrote:
On 10/14/20 2:42 AM, Zebediah Figura wrote:
On 10/13/20 7:02 PM, Joshua Ashton wrote:
Zeb, it's in vulkan_win32.h. You can't include it on Linux or any other platform for that matter. If you read the xml, it's marked as being `platform="win32"`. It's a Windows extension.
To move it out of there on a Vulkan level, we'd need to make a sequel and different platform-specific extensions. Inherently making this version unsupportable in non-Windows drivers.
It makes more sense to read the line of VK_KHR_win32_surface support more as if you want surfaces (ie. not headless) than focusing on the platform part of things.
Can you please explain why this is the case? The extension seems to have been intentionally written in a platform-agnostic manner—except for the parts which interact with platform-specific extensions, and those parts are pretty clearly described as optional.
If "platform=win32" has relevance to the API, and can't be changed without breaking backwards-compatibility, this fact should really be reflected in the documentation. As it is, it's not obvious to me that this problem can't be resolved by simply moving those definitions out of the "vulkan_win32.h" header and into "vulkan_core.h". Perhaps there's places that I haven't found to look, though...
It's more the fact that this extension was made wrong. The platform specific parts should be parts of their own extensions that are dependent on the base.
Currently all platform-specific structures in other extensions are exposed this way (ie. win32_surface, and stuff.)
This also plays into how the script to build the headers works.
Maybe so (though if what I said is correct, it's not obvious to me why there's anything fundamentally wrong with the design), but that's somewhat orthogonal to the point here. Provided the Vulkan headers are fixed so that non-win32 drivers can use the relevant types, is there any reason why this extension can't be implemented?
It's not just the headers, the whole extension is marked as win32. Taking it out of platform=win32 would make that structure try to use a HMONITOR on Linux/Mac/whatever which would obviously not work. Which is why the extensions need to be split up. If somehow work was put in to conditionally only enable this structure specifically for win32 as a one-off then sure. But I really doubt that's going to happen. - Joshie 🐸✨
Also for some reason your emails tend to take like an hour to get to me through the mailing list but everyone else's is fine? I'm not sure why...
I can't think of anything particularly unusual I'm doing, but I'll keep it in mind in case I get more reports of this, thanks.
- Joshie 🐸✨
- Joshie 🐸✨
On 10/14/20 12:37 AM, Georg Lehmann wrote:
On 14.10.20 01:23, Zebediah Figura wrote:
On 10/13/20 4:54 PM, Joshua Ashton wrote: > Figured I should add the reason why, is because just passing it > through > will cause the device to fail to be created because nobody supports > VK_EXT_full_screen_exclusive on Linux. Sure, but they could.
> It's an entirely Windows-centric extension. Well, no, it's not really, but it does include some API-level integration with win32 monitors *if* KHR_win32_surface is supported. In that case, as far as I can see, we don't even need to do anything, because the VkSurfaceFullScreenExclusiveWin32InfoEXT structure would just be ignored.
No, according to the vulkan spec a VkSurfaceFullScreenExclusiveWin32InfoEXT is illegal in any chain unless the full screen + win32 surface exts are enabled. In practice, this will break layers like renderdoc. So we would still need code to remove that struct from pNext chains.
Georg
Not that this necessarily means that lower-level drivers should be the ones to implement the extension, but if nothing else, this information isn't present at all in the patch.
Also, do we really need a generic mechanism for this, wired into make_vulkan? Can't we just handle this extension specially in wine_vk_device_convert_create_info()?
> - Joshie 🐸✨ > > On 10/13/20 10:42 PM, Joshua Ashton wrote: >> winex11/winemac only does this for instance extensions. >> >> VK_EXT_full_screen_exclusive is a device extension. >> >> - Joshie 🐸✨ >> >> On 10/13/20 10:35 PM, Zebediah Figura wrote: >>> Why not do this the normal way, i.e. by modifying the code in >>> winex11 >>> and winemac? >>> >>> Also, what's the point of faking the extension like this? Why not >>> just >>> pass it through? >>>
participants (6)
-
Chip Davis -
Georg Lehmann -
Joshua Ashton -
Liam Middlebrook -
Zebediah Figura -
Zebediah Figura