Re: [PATCH] winex11.drv: Handle Vulkan surface creation with NULL hwnd.

[Rémi Bernon]

On 4/30/21 4:00 PM, Paul Gofman wrote:
> Signed-off-by: Paul Gofman
>  For Red Dead Redemption 2.
>
> dlls/vulkan-1/tests/vulkan.c | 112 ++++++++++++++++++++++++++++++++++-
>   dlls/winex11.drv/vulkan.c    |  38 +++++++++---
>   dlls/winex11.drv/window.c    |  20 +++++++
>   dlls/winex11.drv/x11drv.h    |   1 +
>   4 files changed, 160 insertions(+), 11 deletions(-)
>
> diff --git a/dlls/vulkan-1/tests/vulkan.c b/dlls/vulkan-1/tests/vulkan.c
> index 9061b2b6db8..e516d0d61c8 100644
> --- a/dlls/vulkan-1/tests/vulkan.c
> +++ b/dlls/vulkan-1/tests/vulkan.c
> @@ -437,7 +437,102 @@ static void test_private_data(VkPhysicalDevice vk_physical_device)
>       vkDestroyDevice(vk_device, NULL);
>   }
>
> -static void for_each_device(void (*test_func)(VkPhysicalDevice))
> +static const char *test_null_hwnd_extensions[] =
> +{
> - "VK_KHR_surface",
> - "VK_KHR_win32_surface",
>
> +};
> -
>
> +static void test_null_hwnd(VkInstance vk_instance, VkPhysicalDevice vk_physical_device)
> +{
> - PFN_vkGetPhysicalDeviceSurfacePresentModesKHR pvkGetPhysicalDeviceSurfacePresentModesKHR;
> - VkDeviceGroupPresentModeFlagsKHR present_mode_flags;
> - VkWin32SurfaceCreateInfoKHR surface_create_info;
> - VkSurfaceCapabilitiesKHR surf_caps;
> - VkSurfaceFormatKHR *formats;
> - uint32_t queue_family_index;
> - VkPresentModeKHR *modes;
> - VkSurfaceKHR surface;
> - VkDevice vk_device;
> - uint32_t count;
> - VkRect2D rect;
> - VkBool32 bval;
> - VkResult vr;
> -
> - pvkGetPhysicalDeviceSurfacePresentModesKHR = (void *)vkGetInstanceProcAddr(vk_instance,
> -
>        "vkGetPhysicalDeviceSurfacePresentModesKHR");
> - surface_create_info.sType = VK_STRUCTURE_TYPE_WIN32_SURFACE_CREATE_INFO_KHR;
> - surface_create_info.pNext = NULL;
> - surface_create_info.flags = 0;
> - surface_create_info.hinstance = NULL;
> - surface_create_info.hwnd = NULL;
> -
> - bval = find_queue_family(vk_physical_device, VK_QUEUE_GRAPHICS_BIT, &queue_family_index);
> - ok(bval, "Could not find presentation queue.\n");
> -
> - surface = 0xdeadbeef;
> - vr = vkCreateWin32SurfaceKHR(vk_instance, &surface_create_info, NULL, &surface);
> - ok(vr == VK_SUCCESS, "Got unexpected vr %d.\n", vr);
> - ok(surface != 0xdeadbeef, "Surface not created.\n");
> -
> - count = 0;
> - vr = vkGetPhysicalDeviceSurfaceFormatsKHR(vk_physical_device, surface, &count, NULL);
> - ok(vr == VK_SUCCESS, "Got unexpected vr %d.\n", vr);
> - ok(count, "Got zero count.\n");
> - formats = heap_alloc(sizeof(*formats) * count);
> - vr = vkGetPhysicalDeviceSurfaceFormatsKHR(vk_physical_device, surface, &count, formats);
> - ok(vr == VK_SUCCESS, "Got unexpected vr %d.\n", vr);
> - heap_free(formats);
> -
> - vr = vkGetPhysicalDeviceSurfaceSupportKHR(vk_physical_device, queue_family_index, surface, &bval);
> - ok(vr == VK_SUCCESS, "Got unexpected vr %d.\n", vr);
> -
> - vr = vkGetPhysicalDeviceSurfaceCapabilitiesKHR(vk_physical_device, surface, &surf_caps);
> - ok(vr, "vkGetPhysicalDeviceSurfaceCapabilitiesKHR succeeded.\n");
> -
> - count = 0;
> - vr = pvkGetPhysicalDeviceSurfacePresentModesKHR(vk_physical_device, surface, &count, NULL);
> - ok(vr == VK_SUCCESS, "Got unexpected vr %d.\n", vr);
> - ok(count, "Got zero count.\n");
> - modes = heap_alloc(sizeof(*modes) * count);
> - vr = pvkGetPhysicalDeviceSurfacePresentModesKHR(vk_physical_device, surface, &count, modes);
> - ok(vr == VK_SUCCESS, "Got unexpected vr %d.\n", vr);
> - heap_free(modes);
> -
> - count = 0;
> - vr = vkGetPhysicalDevicePresentRectanglesKHR(vk_physical_device, surface, &count, NULL);
> - ok(vr == VK_SUCCESS, "Got unexpected vr %d.\n", vr);
> - ok(count == 1, "Got unexpected count %u.\n", count);
> - memset(&rect, 0xcc, sizeof(rect));
> - vr = vkGetPhysicalDevicePresentRectanglesKHR(vk_physical_device, surface, &count, &rect);
> - if (vr == VK_SUCCESS) /* Fails on AMD, succeeds on Nvidia. */
> - {
> -
>    ok(count == 1, "Got unexpected count %u.\n", count);
> -
>    ok(!rect.offset.x && !rect.offset.y && !rect.extent.width && !rect.extent.height,
> -
>            "Got unexpected rect %d, %d, %u, %u.\n",
> -
>            rect.offset.x, rect.offset.y, rect.extent.width, rect.extent.height);
> - }
> -
> - if ((vr = create_device(vk_physical_device, 0, NULL, NULL, &vk_device)) < 0)
> - {
> -
>    skip("Failed to create device, vr %d.\n", vr);
> -
>    vkDestroySurfaceKHR(vk_instance, surface, NULL);
> -
>    return;
> - }
> -
> - if (0)
> - {
> -
>    /* Causes access violation on Windows. */
> -
>    vr = vkGetDeviceGroupSurfacePresentModesKHR(vk_device, surface, &present_mode_flags);
> -
>    ok(vr == VK_SUCCESS, "Got unexpected vr %d.\n", vr);
> - }
> -
> - vkDestroyDevice(vk_device, NULL);
> - vkDestroySurfaceKHR(vk_instance, surface, NULL);
>
> +}
> -
>
> +static void for_each_device_instance(uint32_t extension_count, const char * const *enabled_extensions,
> -
>    void (*test_func_instance)(VkInstance, VkPhysicalDevice), void (*test_func)(VkPhysicalDevice))
> {
>     VkPhysicalDevice *vk_physical_devices;
>     VkInstance vk_instance;
>
> @@ -445,7 +540,7 @@ static void for_each_device(void (*test_func)(VkPhysicalDevice))
>       uint32_t count;
>       VkResult vr;
> - if ((vr = create_instance_skip(0, NULL, &vk_instance)) < 0)
> - if ((vr = create_instance_skip(extension_count, enabled_extensions, &vk_instance)) < 0)
>      return;
>  ok(vr == VK_SUCCESS, "Got unexpected VkResult %d.\n", vr);
>
> @@ -463,13 +558,23 @@ static void for_each_device(void (*test_func)(VkPhysicalDevice))
>       ok(vr == VK_SUCCESS, "Got unexpected VkResult %d.\n", vr);
>   for (i = 0; i < count; ++i)
> -
>    test_func(vk_physical_devices[i]);
> -
>
> {
> -
>    if (test_func_instance)
> -
>        test_func_instance(vk_instance, vk_physical_devices[i]);
> -
>    else
> -
>        test_func(vk_physical_devices[i]);
> -
>
> }
>
> heap_free(vk_physical_devices);
>
> vkDestroyInstance(vk_instance, NULL);
> }
>
> +static void for_each_device(void (*test_func)(VkPhysicalDevice))
> +{
> - for_each_device_instance(0, NULL, NULL, test_func);
>
> +}
> - START_TEST(vulkan)
> {
>     test_instance_version();
>
> @@ -481,4 +586,5 @@ START_TEST(vulkan)
>       test_unsupported_instance_extensions();
>       for_each_device(test_unsupported_device_extensions);
>       for_each_device(test_private_data);
> - for_each_device_instance(ARRAY_SIZE(test_null_hwnd_extensions), test_null_hwnd_extensions, test_null_hwnd, NULL);
> }
>
> diff --git a/dlls/winex11.drv/vulkan.c b/dlls/winex11.drv/vulkan.c
> index 139faf6b407..c8ba68c888f 100644
> --- a/dlls/winex11.drv/vulkan.c
> +++ b/dlls/winex11.drv/vulkan.c
> @@ -62,6 +62,7 @@ struct wine_vk_surface
>       LONG ref;
>       Window window;
>       VkSurfaceKHR surface; /* native surface */
> -
>
> BOOL dummy_surface;
> };
>
> typedef struct VkXlibSurfaceCreateInfoKHR
>
> @@ -255,14 +256,18 @@ static VkResult X11DRV_vkCreateSwapchainKHR(VkDevice device,
>           const VkSwapchainCreateInfoKHR *create_info,
>           const VkAllocationCallbacks *allocator, VkSwapchainKHR *swapchain)
>   {
> -
>
> struct wine_vk_surface *x11_surface = surface_from_handle(create_info->surface);
>  VkSwapchainCreateInfoKHR create_info_host;
>  TRACE("%p %p %p %p\n", device, create_info, allocator, swapchain);
>
> if (allocator)
>      FIXME("Support for allocation callbacks not implemented yet\n");
> -
>
> if (x11_surface->dummy_surface)
> -
>    return VK_ERROR_SURFACE_LOST_KHR;
> -
> create_info_host = *create_info;
> - create_info_host.surface = surface_from_handle(create_info->surface)->surface;
> -
>
> create_info_host.surface = x11_surface->surface;
>
> return pvkCreateSwapchainKHR(device, &create_info_host, NULL /* allocator */, swapchain);
> }
>
> @@ -281,7 +286,7 @@ static VkResult X11DRV_vkCreateWin32SurfaceKHR(VkInstance instance,
>           FIXME("Support for allocation callbacks not implemented yet\n");
>   /* TODO: support child window rendering. */
> - if (GetAncestor(create_info->hwnd, GA_PARENT) != GetDesktopWindow())
> - if (create_info->hwnd && GetAncestor(create_info->hwnd, GA_PARENT) != GetDesktopWindow())
>  {
>      FIXME("Application requires child window rendering, which is not implemented yet!\n");
>      return VK_ERROR_INCOMPATIBLE_DRIVER;
>
> @@ -292,8 +297,10 @@ static VkResult X11DRV_vkCreateWin32SurfaceKHR(VkInstance instance,
>           return VK_ERROR_OUT_OF_HOST_MEMORY;
>   x11_surface->ref = 1;
> - x11_surface->dummy_surface = !create_info->hwnd;
> - x11_surface->window = x11_surface->dummy_surface ? create_dummy_client_window()
> -
>        : create_client_window(create_info->hwnd, &default_visual);
>
> - x11_surface->window = create_client_window(create_info->hwnd, &default_visual);
>  if (!x11_surface->window)
>  {
>      ERR("Failed to allocate client window for hwnd=%p\n", create_info->hwnd);

Maybe we could just use the dummy parent window as a backing window 
instead of creating a new one, returning it from create_client_window 
when hwnd parameter is NULL?

It would just have to avoid calling XDestroyWindow on it if 
dummy_surface is set.

I also think that keeping the associated HWND with the surface could be 
useful in the future, so that could possibly replace the dummy_surface 
flag. But as it's not useful immediately it may be changed later as well.