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

Tue Oct 13 21:23:21 CDT 2020

On 10/13/20 8:56 PM, Joshua Ashton wrote:
> 
> 
> 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.

Sorry, I missed that.

In that case it seems the simpler solution is to have a static const
array of strings in vulkan.c; make_vulkan still doesn't need to get
involved.

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20201013/22dffac6/attachment-0001.sig>