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

[Zebediah Figura]

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.

> 
>>>>>
>>>>>> - 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/fdac4aff/attachment-0001.sig>