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

Tue Oct 13 20:44:35 CDT 2020

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