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

[Georg Lehmann]

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.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?
>>>>>>>>
>>>>>
>>>>
>>
>