[PATCH] winevulkan: Implement vk_icdGetPhysicalDeviceProcAddr

[Joshua Ashton]

Needed for Red Dead Redemption 2 under vkd3d-proton.

RDR2 requires us to use the native Vulkan loader, and vkd3d-proton recently enabled VK_KHR_fragment_shading_rate which exposes physical device procs unknown to the loader.

Previously, when we queried vkGetPhysicalDeviceFragmentShadingRatesKHR via. vkGetInstanceProcAddr, the native loader tried to call vk_icdGetPhysicalDeviceProcAddr from the WineVulkan ICD to resolve this, which didn't exist causing the loader to nullptr itself and crash.

This commit implements the vk_icdGetPhysicalDeviceProcAddr export, required for ICD version 4, via. forwarding to wine_vkGetInstanceProcAddr and fixes that problem.

Signed-off-by: Joshua Ashton <joshua(a)froggi.es>
---
 dlls/winevulkan/make_vulkan |  1 +
 dlls/winevulkan/vulkan.c    | 15 ++++++++++++---
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/dlls/winevulkan/make_vulkan b/dlls/winevulkan/make_vulkan
index 3ad210e4701..f32ba9b90f1 100755
--- a/dlls/winevulkan/make_vulkan
+++ b/dlls/winevulkan/make_vulkan
@@ -2647,6 +2647,7 @@ class VkGenerator(object):
     def generate_vulkan_spec(self, f):
         self._generate_copyright(f, spec_file=True)
         f.write("@ stdcall -private vk_icdGetInstanceProcAddr(ptr str) wine_vk_icdGetInstanceProcAddr\n")
+        f.write("@ stdcall -private vk_icdGetPhysicalDeviceProcAddr(ptr str) wine_vk_icdGetPhysicalDeviceProcAddr\n")
         f.write("@ stdcall -private vk_icdNegotiateLoaderICDInterfaceVersion(ptr) wine_vk_icdNegotiateLoaderICDInterfaceVersion\n")
         f.write("@ cdecl -norelay native_vkGetInstanceProcAddrWINE(ptr str)\n")
 
diff --git a/dlls/winevulkan/vulkan.c b/dlls/winevulkan/vulkan.c
index 971394eb9dd..2e02714a9fd 100644
--- a/dlls/winevulkan/vulkan.c
+++ b/dlls/winevulkan/vulkan.c
@@ -37,9 +37,7 @@ DEFINE_DEVPROPKEY(DEVPROPKEY_GPU_LUID, 0x60b193cb, 0x5276, 0x4d0f, 0x96, 0xfc, 0
 DEFINE_DEVPROPKEY(WINE_DEVPROPKEY_GPU_VULKAN_UUID, 0x233a9ef3, 0xafc4, 0x4abd, 0xb5, 0x64, 0xc3, 0x2f, 0x21, 0xf1, 0x53, 0x5c, 2);
 
 /* For now default to 4 as it felt like a reasonable version feature wise to support.
- * Don't support the optional vk_icdGetPhysicalDeviceProcAddr introduced in this version
- * as it is unlikely we will implement physical device extensions, which the loader is not
- * aware of. Version 5 adds more extensive version checks. Something to tackle later.
+ * Version 5 adds more extensive version checks. Something to tackle later.
  */
 #define WINE_VULKAN_ICD_VERSION 4
 
@@ -1239,6 +1237,17 @@ void * WINAPI wine_vk_icdGetInstanceProcAddr(VkInstance instance, const char *na
     return wine_vkGetInstanceProcAddr(instance, name);
 }
 
+void * WINAPI wine_vk_icdGetPhysicalDeviceProcAddr(VkInstance instance, const char *name)
+{
+    TRACE("%p, %s\n", instance, debugstr_a(name));
+
+    /* This is exported directly by the ICD, despite the loader documentation
+     * stating that this is queried via vk_icdGetInstanceProcAddr
+     */
+
+    return wine_vkGetInstanceProcAddr(instance, name);
+}
+
 VkResult WINAPI wine_vk_icdNegotiateLoaderICDInterfaceVersion(uint32_t *supported_version)
 {
     uint32_t req_version;
-- 
2.31.0

[Liam Middlebrook]

Signed-off-by: Liam Middlebrook <lmiddlebrook(a)nvidia.com>

One non-review comment written inline.

On 3/22/21 6:02 PM, Joshua Ashton wrote:
> Needed for Red Dead Redemption 2 under vkd3d-proton.
>
> RDR2 requires us to use the native Vulkan loader, and vkd3d-proton recently enabled VK_KHR_fragment_shading_rate which exposes physical device procs unknown to the loader.
>
> Previously, when we queried vkGetPhysicalDeviceFragmentShadingRatesKHR via. vkGetInstanceProcAddr, the native loader tried to call vk_icdGetPhysicalDeviceProcAddr from the WineVulkan ICD to resolve this, which didn't exist causing the loader to nullptr itself and crash.
>
> This commit implements the vk_icdGetPhysicalDeviceProcAddr export, required for ICD version 4, via. forwarding to wine_vkGetInstanceProcAddr and fixes that problem.
>
> Signed-off-by: Joshua Ashton <joshua(a)froggi.es>
> ---
>   dlls/winevulkan/make_vulkan |  1 +
>   dlls/winevulkan/vulkan.c    | 15 ++++++++++++---
>   2 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/dlls/winevulkan/make_vulkan b/dlls/winevulkan/make_vulkan
> index 3ad210e4701..f32ba9b90f1 100755
> --- a/dlls/winevulkan/make_vulkan
> +++ b/dlls/winevulkan/make_vulkan
> @@ -2647,6 +2647,7 @@ class VkGenerator(object):
>       def generate_vulkan_spec(self, f):
>           self._generate_copyright(f, spec_file=True)
>           f.write("@ stdcall -private vk_icdGetInstanceProcAddr(ptr str) wine_vk_icdGetInstanceProcAddr\n")
> +        f.write("@ stdcall -private vk_icdGetPhysicalDeviceProcAddr(ptr str) wine_vk_icdGetPhysicalDeviceProcAddr\n")
>           f.write("@ stdcall -private vk_icdNegotiateLoaderICDInterfaceVersion(ptr) wine_vk_icdNegotiateLoaderICDInterfaceVersion\n")
>           f.write("@ cdecl -norelay native_vkGetInstanceProcAddrWINE(ptr str)\n")
>
> diff --git a/dlls/winevulkan/vulkan.c b/dlls/winevulkan/vulkan.c
> index 971394eb9dd..2e02714a9fd 100644
> --- a/dlls/winevulkan/vulkan.c
> +++ b/dlls/winevulkan/vulkan.c
> @@ -37,9 +37,7 @@ DEFINE_DEVPROPKEY(DEVPROPKEY_GPU_LUID, 0x60b193cb, 0x5276, 0x4d0f, 0x96, 0xfc, 0
>   DEFINE_DEVPROPKEY(WINE_DEVPROPKEY_GPU_VULKAN_UUID, 0x233a9ef3, 0xafc4, 0x4abd, 0xb5, 0x64, 0xc3, 0x2f, 0x21, 0xf1, 0x53, 0x5c, 2);
>
> /* For now default to 4 as it felt like a reasonable version feature wise to support.
> - * Don't support the optional vk_icdGetPhysicalDeviceProcAddr introduced in this version
> - * as it is unlikely we will implement physical device extensions, which the loader is not
> - * aware of. Version 5 adds more extensive version checks. Something to tackle later.
> + * Version 5 adds more extensive version checks. Something to tackle later.
>    */
>   #define WINE_VULKAN_ICD_VERSION 4
>
> @@ -1239,6 +1237,17 @@ void * WINAPI wine_vk_icdGetInstanceProcAddr(VkInstance instance, const char *na
>       return wine_vkGetInstanceProcAddr(instance, name);
>   }
>
> +void * WINAPI wine_vk_icdGetPhysicalDeviceProcAddr(VkInstance instance, const char *name)
> +{
> +    TRACE("%p, %s\n", instance, debugstr_a(name));
> +
> +    /* This is exported directly by the ICD, despite the loader documentation
> +     * stating that this is queried via vk_icdGetInstanceProcAddr
> +     */

Could you file an issue / send a pull-request to the loader repository 
that addresses this hole in documentation?


Thanks,

Liam Middlebrook
> +
> +    return wine_vkGetInstanceProcAddr(instance, name);
> +}
> +
>   VkResult WINAPI wine_vk_icdNegotiateLoaderICDInterfaceVersion(uint32_t *supported_version)
>   {
>       uint32_t req_version;

[Joshua Ashton]

On 3/23/21 1:31 AM, Liam Middlebrook wrote:
> Signed-off-by: Liam Middlebrook <lmiddlebrook(a)nvidia.com>
>
> One non-review comment written inline.
>
> On 3/22/21 6:02 PM, Joshua Ashton wrote:
> > Needed for Red Dead Redemption 2 under vkd3d-proton.
> >
> > RDR2 requires us to use the native Vulkan loader, and vkd3d-proton 
> > recently enabled VK_KHR_fragment_shading_rate which exposes physical 
> > device procs unknown to the loader.
> >
> > Previously, when we queried vkGetPhysicalDeviceFragmentShadingRatesKHR 
> > via. vkGetInstanceProcAddr, the native loader tried to call 
> > vk_icdGetPhysicalDeviceProcAddr from the WineVulkan ICD to resolve 
> > this, which didn't exist causing the loader to nullptr itself and crash.
> >
> > This commit implements the vk_icdGetPhysicalDeviceProcAddr export, 
> > required for ICD version 4, via. forwarding to 
> > wine_vkGetInstanceProcAddr and fixes that problem.
> >
> > Signed-off-by: Joshua Ashton <joshua(a)froggi.es>
> > ---
> >   dlls/winevulkan/make_vulkan |  1 +
> >   dlls/winevulkan/vulkan.c    | 15 ++++++++++++---
> >   2 files changed, 13 insertions(+), 3 deletions(-)
> >
> > diff --git a/dlls/winevulkan/make_vulkan b/dlls/winevulkan/make_vulkan
> > index 3ad210e4701..f32ba9b90f1 100755
> > --- a/dlls/winevulkan/make_vulkan
> > +++ b/dlls/winevulkan/make_vulkan
> > @@ -2647,6 +2647,7 @@ class VkGenerator(object):
> >       def generate_vulkan_spec(self, f):
> >           self._generate_copyright(f, spec_file=True)
> >           f.write("@ stdcall -private vk_icdGetInstanceProcAddr(ptr 
> > str) wine_vk_icdGetInstanceProcAddr\n")
> > +        f.write("@ stdcall -private 
> > vk_icdGetPhysicalDeviceProcAddr(ptr str) 
> > wine_vk_icdGetPhysicalDeviceProcAddr\n")
> >           f.write("@ stdcall -private 
> > vk_icdNegotiateLoaderICDInterfaceVersion(ptr) 
> > wine_vk_icdNegotiateLoaderICDInterfaceVersion\n")
> >           f.write("@ cdecl -norelay 
> > native_vkGetInstanceProcAddrWINE(ptr str)\n")
> > diff --git a/dlls/winevulkan/vulkan.c b/dlls/winevulkan/vulkan.c
> > index 971394eb9dd..2e02714a9fd 100644
> > --- a/dlls/winevulkan/vulkan.c
> > +++ b/dlls/winevulkan/vulkan.c
> > @@ -37,9 +37,7 @@ DEFINE_DEVPROPKEY(DEVPROPKEY_GPU_LUID, 0x60b193cb, 
> > 0x5276, 0x4d0f, 0x96, 0xfc, 0
> >   DEFINE_DEVPROPKEY(WINE_DEVPROPKEY_GPU_VULKAN_UUID, 0x233a9ef3, 
> > 0xafc4, 0x4abd, 0xb5, 0x64, 0xc3, 0x2f, 0x21, 0xf1, 0x53, 0x5c, 2);
> >   /* For now default to 4 as it felt like a reasonable version feature 
> > wise to support.
> > - * Don't support the optional vk_icdGetPhysicalDeviceProcAddr 
> > introduced in this version
> > - * as it is unlikely we will implement physical device extensions, 
> > which the loader is not
> > - * aware of. Version 5 adds more extensive version checks. Something 
> > to tackle later.
> > + * Version 5 adds more extensive version checks. Something to tackle 
> > later.
> >    */
> >   #define WINE_VULKAN_ICD_VERSION 4
> > @@ -1239,6 +1237,17 @@ void * WINAPI 
> > wine_vk_icdGetInstanceProcAddr(VkInstance instance, const char *na
> >       return wine_vkGetInstanceProcAddr(instance, name);
> >   }
> > +void * WINAPI wine_vk_icdGetPhysicalDeviceProcAddr(VkInstance 
> > instance, const char *name)
> > +{
> > +    TRACE("%p, %s\n", instance, debugstr_a(name));
> > +
> > +    /* This is exported directly by the ICD, despite the loader 
> > documentation
> > +     * stating that this is queried via vk_icdGetInstanceProcAddr
> > +     */
>
> Could you file an issue / send a pull-request to the loader repository 
> that addresses this hole in documentation?

https://github.com/KhronosGroup/Vulkan-Loader/issues/558

Done.

- Joshie 🐸✨
> Thanks,
>
> Liam Middlebrook
> > +
> > +    return wine_vkGetInstanceProcAddr(instance, name);
> > +}
> > +
> >   VkResult WINAPI 
> > wine_vk_icdNegotiateLoaderICDInterfaceVersion(uint32_t 
> > *supported_version)
> >   {
> >       uint32_t req_version;

[Georg Lehmann]

On 23.03.21 02:02, Joshua Ashton wrote:
> Needed for Red Dead Redemption 2 under vkd3d-proton.
>
> RDR2 requires us to use the native Vulkan loader, and vkd3d-proton recently enabled VK_KHR_fragment_shading_rate which exposes physical device procs unknown to the loader.
>
> Previously, when we queried vkGetPhysicalDeviceFragmentShadingRatesKHR via. vkGetInstanceProcAddr, the native loader tried to call vk_icdGetPhysicalDeviceProcAddr from the WineVulkan ICD to resolve this, which didn't exist causing the loader to nullptr itself and crash.
>
> This commit implements the vk_icdGetPhysicalDeviceProcAddr export, required for ICD version 4, via. forwarding to wine_vkGetInstanceProcAddr and fixes that problem.
>
> Signed-off-by: Joshua Ashton <joshua(a)froggi.es>
> ---
>   dlls/winevulkan/make_vulkan |  1 +
>   dlls/winevulkan/vulkan.c    | 15 ++++++++++++---
>   2 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/dlls/winevulkan/make_vulkan b/dlls/winevulkan/make_vulkan
> index 3ad210e4701..f32ba9b90f1 100755
> --- a/dlls/winevulkan/make_vulkan
> +++ b/dlls/winevulkan/make_vulkan
> @@ -2647,6 +2647,7 @@ class VkGenerator(object):
>       def generate_vulkan_spec(self, f):
>           self._generate_copyright(f, spec_file=True)
>           f.write("@ stdcall -private vk_icdGetInstanceProcAddr(ptr str) wine_vk_icdGetInstanceProcAddr\n")
> +        f.write("@ stdcall -private vk_icdGetPhysicalDeviceProcAddr(ptr str) wine_vk_icdGetPhysicalDeviceProcAddr\n")
>           f.write("@ stdcall -private vk_icdNegotiateLoaderICDInterfaceVersion(ptr) wine_vk_icdNegotiateLoaderICDInterfaceVersion\n")
>           f.write("@ cdecl -norelay native_vkGetInstanceProcAddrWINE(ptr str)\n")
>
> diff --git a/dlls/winevulkan/vulkan.c b/dlls/winevulkan/vulkan.c
> index 971394eb9dd..2e02714a9fd 100644
> --- a/dlls/winevulkan/vulkan.c
> +++ b/dlls/winevulkan/vulkan.c
> @@ -37,9 +37,7 @@ DEFINE_DEVPROPKEY(DEVPROPKEY_GPU_LUID, 0x60b193cb, 0x5276, 0x4d0f, 0x96, 0xfc, 0
>   DEFINE_DEVPROPKEY(WINE_DEVPROPKEY_GPU_VULKAN_UUID, 0x233a9ef3, 0xafc4, 0x4abd, 0xb5, 0x64, 0xc3, 0x2f, 0x21, 0xf1, 0x53, 0x5c, 2);
>
> /* For now default to 4 as it felt like a reasonable version feature wise to support.
> - * Don't support the optional vk_icdGetPhysicalDeviceProcAddr introduced in this version
> - * as it is unlikely we will implement physical device extensions, which the loader is not
> - * aware of. Version 5 adds more extensive version checks. Something to tackle later.
> + * Version 5 adds more extensive version checks. Something to tackle later.
>    */
>   #define WINE_VULKAN_ICD_VERSION 4
>
> @@ -1239,6 +1237,17 @@ void * WINAPI wine_vk_icdGetInstanceProcAddr(VkInstance instance, const char *na
>       return wine_vkGetInstanceProcAddr(instance, name);
>   }
>
> +void * WINAPI wine_vk_icdGetPhysicalDeviceProcAddr(VkInstance instance, const char *name)
> +{
> +    TRACE("%p, %s\n", instance, debugstr_a(name));
> +
> +    /* This is exported directly by the ICD, despite the loader documentation
> +     * stating that this is queried via vk_icdGetInstanceProcAddr
> +     */

We should still support getting it via vk_icdGetInstanceProcAddr.
> +
> +    return wine_vkGetInstanceProcAddr(instance, name);

This is not correct. vk_icdGetPhysicalDeviceProcAddr should only return
function pointers for physical device level functions. This returns
function pointers for *all* functions, so the loader will treat every
unknown function as a physical device function. [1]

The proper way to do this is by generating a new dispatch table similar
to vk_instance_dispatch_table in vulkan_thunks.c with only functions
that take a physical device as the first argument.

Thanks,

Georg Lehmann

[1] 
https://github.com/KhronosGroup/Vulkan-Loader/blob/master/loader/LoaderAndLa...
> +}
> +
>   VkResult WINAPI wine_vk_icdNegotiateLoaderICDInterfaceVersion(uint32_t *supported_version)
>   {
>       uint32_t req_version;

[Joshua Ashton]

Thanks for the feedback, after discussion we found a few places where 
this could patch break that needs addressing.

- Joshie 🐸✨

On 3/23/21 9:31 AM, Georg Lehmann wrote:
> On 23.03.21 02:02, Joshua Ashton wrote:
> > Needed for Red Dead Redemption 2 under vkd3d-proton.
> >
> > RDR2 requires us to use the native Vulkan loader, and vkd3d-proton 
> > recently enabled VK_KHR_fragment_shading_rate which exposes physical 
> > device procs unknown to the loader.
> >
> > Previously, when we queried vkGetPhysicalDeviceFragmentShadingRatesKHR 
> > via. vkGetInstanceProcAddr, the native loader tried to call 
> > vk_icdGetPhysicalDeviceProcAddr from the WineVulkan ICD to resolve 
> > this, which didn't exist causing the loader to nullptr itself and crash.
> >
> > This commit implements the vk_icdGetPhysicalDeviceProcAddr export, 
> > required for ICD version 4, via. forwarding to 
> > wine_vkGetInstanceProcAddr and fixes that problem.
> >
> > Signed-off-by: Joshua Ashton <joshua(a)froggi.es>
> > ---
> >   dlls/winevulkan/make_vulkan |  1 +
> >   dlls/winevulkan/vulkan.c    | 15 ++++++++++++---
> >   2 files changed, 13 insertions(+), 3 deletions(-)
> >
> > diff --git a/dlls/winevulkan/make_vulkan b/dlls/winevulkan/make_vulkan
> > index 3ad210e4701..f32ba9b90f1 100755
> > --- a/dlls/winevulkan/make_vulkan
> > +++ b/dlls/winevulkan/make_vulkan
> > @@ -2647,6 +2647,7 @@ class VkGenerator(object):
> >       def generate_vulkan_spec(self, f):
> >           self._generate_copyright(f, spec_file=True)
> >           f.write("@ stdcall -private vk_icdGetInstanceProcAddr(ptr 
> > str) wine_vk_icdGetInstanceProcAddr\n")
> > +        f.write("@ stdcall -private 
> > vk_icdGetPhysicalDeviceProcAddr(ptr str) 
> > wine_vk_icdGetPhysicalDeviceProcAddr\n")
> >           f.write("@ stdcall -private 
> > vk_icdNegotiateLoaderICDInterfaceVersion(ptr) 
> > wine_vk_icdNegotiateLoaderICDInterfaceVersion\n")
> >           f.write("@ cdecl -norelay 
> > native_vkGetInstanceProcAddrWINE(ptr str)\n")
> > diff --git a/dlls/winevulkan/vulkan.c b/dlls/winevulkan/vulkan.c
> > index 971394eb9dd..2e02714a9fd 100644
> > --- a/dlls/winevulkan/vulkan.c
> > +++ b/dlls/winevulkan/vulkan.c
> > @@ -37,9 +37,7 @@ DEFINE_DEVPROPKEY(DEVPROPKEY_GPU_LUID, 0x60b193cb, 
> > 0x5276, 0x4d0f, 0x96, 0xfc, 0
> >   DEFINE_DEVPROPKEY(WINE_DEVPROPKEY_GPU_VULKAN_UUID, 0x233a9ef3, 
> > 0xafc4, 0x4abd, 0xb5, 0x64, 0xc3, 0x2f, 0x21, 0xf1, 0x53, 0x5c, 2);
> >   /* For now default to 4 as it felt like a reasonable version feature 
> > wise to support.
> > - * Don't support the optional vk_icdGetPhysicalDeviceProcAddr 
> > introduced in this version
> > - * as it is unlikely we will implement physical device extensions, 
> > which the loader is not
> > - * aware of. Version 5 adds more extensive version checks. Something 
> > to tackle later.
> > + * Version 5 adds more extensive version checks. Something to tackle 
> > later.
> >    */
> >   #define WINE_VULKAN_ICD_VERSION 4
> > @@ -1239,6 +1237,17 @@ void * WINAPI 
> > wine_vk_icdGetInstanceProcAddr(VkInstance instance, const char *na
> >       return wine_vkGetInstanceProcAddr(instance, name);
> >   }
> > +void * WINAPI wine_vk_icdGetPhysicalDeviceProcAddr(VkInstance 
> > instance, const char *name)
> > +{
> > +    TRACE("%p, %s\n", instance, debugstr_a(name));
> > +
> > +    /* This is exported directly by the ICD, despite the loader 
> > documentation
> > +     * stating that this is queried via vk_icdGetInstanceProcAddr
> > +     */
>
> We should still support getting it via vk_icdGetInstanceProcAddr.
> > +
> > +    return wine_vkGetInstanceProcAddr(instance, name);
>
> This is not correct. vk_icdGetPhysicalDeviceProcAddr should only return
> function pointers for physical device level functions. This returns
> function pointers for *all* functions, so the loader will treat every
> unknown function as a physical device function. [1]
>
> The proper way to do this is by generating a new dispatch table similar
> to vk_instance_dispatch_table in vulkan_thunks.c with only functions
> that take a physical device as the first argument.
>
> Thanks,
>
> Georg Lehmann
>
> [1] 
> https://github.com/KhronosGroup/Vulkan-Loader/blob/master/loader/LoaderAndLa...
> > +}
> > +
> >   VkResult WINAPI 
> > wine_vk_icdNegotiateLoaderICDInterfaceVersion(uint32_t 
> > *supported_version)
> >   {
> >       uint32_t req_version;