[PATCH 2/3] winevulkan: Implement extension faking mechanism

Joshua Ashton joshua at froggi.es
Tue Oct 13 21:07:47 CDT 2020 


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.html#fundamentals-validusage-pNext 
>>>
>>> [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 🐸✨

More information about the wine-devel mailing list