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

Joshua Ashton joshua at froggi.es
Tue Oct 13 20:31:59 CDT 2020 


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

More information about the wine-devel mailing list