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

[Joshua Ashton]

On 10/14/20 2:50 AM, Zebediah Figura wrote:
> On 10/13/20 8:05 PM, 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.
>>
>>>>>
>>>>>
>>>>> 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".
> 
> I guess the point is it's not clear to me that a list like
> 
> FAKED_EXTENSIONS = [
> 	"EXT_foo",
> 	"EXT_bar",
> ]
> 
> provides any benefit over
> 
>      const char *name = properties[i].extensionName;
> 
>      if (strcmp(name, "VK_EXT_foo")
>              && strcmp(name, "VK_EXT_bar")
>      {
>          /* Copy the extension... */
>      }
> 
> given that in both cases, adding a new extension to the list comprises
> only one line, and the former requires more support code.
> 
> It makes sense to me to handle blacklisted extensions in make_vulkan,
> since we do multiple nontrivial things with the list, but we're only
> using this "faked extension" list in one place.
> 
We aren't using it in one place, we're using it in two places, once for 
the device create info fixup to remove them being passed through and the 
other for reporting whether device extensions are supported.

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