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 🐸✨