From fc6581cf3be54d8a578f83f1fee71d63a779a8bf Mon Sep 17 00:00:00 2001 From: Benjamin Otte Date: Wed, 12 Nov 2025 09:18:27 +0100 Subject: [PATCH] vulkan: Rework swapchain present implementation I had for some inexplicable reason assumed that the swapchain uses the same queue as we submit on and therefor no synchronization is necessary. That's not how it works though. Well, it was technically true with most drivers, so nothing bad happened. Until Mesa decided to make things run faster by using different queues. Then all hell broke loose and GTK exploded left and right. The actual implementation of proper synchronization was originally very underspecified and was only properly specified with the VK_EXT_swapchain_maintenance1 extension. See https://www.khronos.org/blog/resolving-longstanding-issues-with-wsi for a discussion of the problems. That's why we need 2 different codepaths: One when running with the extension that uses fences to track lifetimes of objects and one that uses best practices when fences aren't available and hopes for the best. During this refactoring, I also untied the swapchain from the context, so that GTK can now recreate the swapchain while there are still outstanding images - we just keep the old swapchain alive. Now that we do proper tracking, that's not too hard. Unfortunately all of this is rather entangled, so I didn't manage to split it into multiple commits. This also fixes all previous validation layer complaints when running with both VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation VK_LAYERS_ENABLES=VK_VALIDATION_FEATURE_ENABLE_SYNCHRONIZATION_VALIDATION_EXT Fixes: https://gitlab.gnome.org/GNOME/gtk/-/issues/7866 Fixes: https://gitlab.freedesktop.org/mesa/mesa/-/issues/14087 Part-of: --- gdk/gdkvulkancontext.c | 277 ++++++++++++++++++++++++++++++---- gdk/gdkvulkancontextprivate.h | 1 + gsk/gpu/gskvulkanframe.c | 29 +--- 3 files changed, 251 insertions(+), 56 deletions(-) diff --git a/gdk/gdkvulkancontext.c b/gdk/gdkvulkancontext.c index 67c8b48a1a8..fe59058df62 100644 --- a/gdk/gdkvulkancontext.c +++ b/gdk/gdkvulkancontext.c @@ -51,6 +51,18 @@ const GdkDebugKey gdk_vulkan_feature_keys[] = { { "swapchain-maintenance", GDK_VULKAN_FEATURE_SWAPCHAIN_MAINTENANCE, "Do not use advanced swapchain features" }, { "portability-subset", GDK_VULKAN_FEATURE_PORTABILITY_SUBSET, "Vulkan implementation is non-conformant" }, }; + +/* arbitrarily chosen to be 2 * GSK_GPU_MAX_FRAMES */ +#define MAX_PRESENTS 8 + +typedef struct _GdkVulkanPresent GdkVulkanPresent; +struct _GdkVulkanPresent +{ + VkSemaphore vk_semaphore; + VkFence vk_fence; + VkSwapchainKHR vk_swapchain; + guint32 image_index; +}; #endif /** @@ -85,9 +97,10 @@ struct _GdkVulkanContextPrivate { guint n_images; VkImage *images; cairo_region_t **regions; -#endif - guint32 draw_index; + GdkVulkanPresent presents[MAX_PRESENTS]; + gsize latest_present; +#endif guint vulkan_ref: 1; }; @@ -367,6 +380,35 @@ fallback_present_mode: return VK_PRESENT_MODE_FIFO_KHR; } +/* This is a quite hacky way to avoid proper refcounting... */ +static void +gdk_vulkan_context_unref_swapchain (GdkVulkanContext *self, + VkSwapchainKHR vk_swapchain) +{ + GdkVulkanContextPrivate *priv = gdk_vulkan_context_get_instance_private (self); + gsize i, count; + + count = 0; + + if (priv->swapchain == vk_swapchain) + count++; + + for (i = 0; i < G_N_ELEMENTS (priv->presents); i++) + { + if (priv->presents[i].vk_swapchain == vk_swapchain) + count++; + } + + g_assert (count > 0); + + if (count > 1) + return; + + vkDestroySwapchainKHR (gdk_vulkan_context_get_device (self), + vk_swapchain, + NULL); +} + static gboolean gdk_vulkan_context_check_swapchain (GdkVulkanContext *context, GError **error) @@ -493,9 +535,7 @@ gdk_vulkan_context_check_swapchain (GdkVulkanContext *context, if (priv->swapchain != VK_NULL_HANDLE) { - vkDestroySwapchainKHR (device, - priv->swapchain, - NULL); + gdk_vulkan_context_unref_swapchain (context, priv->swapchain); for (i = 0; i < priv->n_images; i++) { cairo_region_destroy (priv->regions[i]); @@ -649,6 +689,124 @@ physical_device_check_features (VkPhysicalDevice device) return features; } +static gboolean +gdk_vulkan_present_is_busy (GdkVulkanContext *self, + GdkVulkanPresent *present) +{ + VkResult res; + + if (present->vk_swapchain == NULL) + return FALSE; + + if (!present->vk_fence) + return TRUE; + + res = vkGetFenceStatus (gdk_vulkan_context_get_device (self), present->vk_fence); + if (res != VK_SUCCESS) + return TRUE; + + GDK_VK_CHECK (vkResetFences, gdk_vulkan_context_get_device (self), + 1, + &present->vk_fence); + + gdk_vulkan_context_unref_swapchain (self, present->vk_swapchain); + present->vk_swapchain = VK_NULL_HANDLE; + + return FALSE; +} + +static void +gdk_vulkan_context_wait_present (GdkVulkanContext *self, + gboolean wait_all) +{ + GdkVulkanContextPrivate *priv = gdk_vulkan_context_get_instance_private (self); + + if (!gdk_vulkan_context_has_feature (self, GDK_VULKAN_FEATURE_SWAPCHAIN_MAINTENANCE)) + { + gsize i; + /* See https://www.khronos.org/blog/resolving-longstanding-issues-with-wsi for + * why this is necessary without the extension. + */ + vkDeviceWaitIdle (gdk_vulkan_context_get_device (self)); + + for (i = 0; i < G_N_ELEMENTS (priv->presents); i++) + { + if (priv->presents[i].vk_swapchain) + { + gdk_vulkan_context_unref_swapchain (self, priv->presents[i].vk_swapchain); + priv->presents[i].vk_swapchain = VK_NULL_HANDLE; + } + } + } + else + { + VkFence fences[G_N_ELEMENTS (priv->presents)]; + gsize i, n_fences; + + n_fences = 0; + for (i = 0; i < G_N_ELEMENTS (priv->presents); i++) + { + if (priv->presents[i].vk_swapchain) + { + fences[n_fences++] = priv->presents[i].vk_fence; + } + } + + GDK_VK_CHECK (vkWaitForFences, gdk_vulkan_context_get_device (self), + n_fences, + fences, + wait_all, + INT64_MAX); + } +} + +static GdkVulkanPresent * +gdk_vulkan_context_start_present (GdkVulkanContext *self) +{ + GdkVulkanContextPrivate *priv = gdk_vulkan_context_get_instance_private (self); + gsize i; + + while (TRUE) + { + for (i = 0; i < G_N_ELEMENTS (priv->presents); i++) + { + if (!gdk_vulkan_present_is_busy (self, &priv->presents[i])) + { + GdkVulkanPresent *result = &priv->presents[i]; + priv->latest_present = i; + + return result; + } + } + + gdk_vulkan_context_wait_present (self, FALSE); + } +} + +static void +gdk_vulkan_context_release_presents (GdkVulkanContext *self, + guint32 image_index) +{ + GdkVulkanContextPrivate *priv = gdk_vulkan_context_get_instance_private (self); + gsize i; + + /* We use fences with swapchain-maintenance */ + if (gdk_vulkan_context_has_feature (self, GDK_VULKAN_FEATURE_SWAPCHAIN_MAINTENANCE)) + return; + + for (i = 0; i < G_N_ELEMENTS (priv->presents); i++) + { + if (priv->presents[i].vk_swapchain != priv->swapchain) + continue; + + if (priv->presents[i].image_index == image_index) + { + gdk_vulkan_context_unref_swapchain (self, priv->presents[i].vk_swapchain); + priv->presents[i].vk_swapchain = VK_NULL_HANDLE; + } + } +} + static void gdk_vulkan_context_begin_frame (GdkDrawContext *draw_context, gpointer context_data, @@ -663,6 +821,7 @@ gdk_vulkan_context_begin_frame (GdkDrawContext *draw_context, GdkColorState *color_state; VkResult acquire_result; VkSemaphore draw_semaphore; + GdkVulkanPresent *present; guint i; g_assert (context_data != NULL); @@ -695,6 +854,8 @@ gdk_vulkan_context_begin_frame (GdkDrawContext *draw_context, cairo_region_union (priv->regions[i], region); } + present = gdk_vulkan_context_start_present (context); + while (TRUE) { acquire_result = GDK_VK_CHECK (vkAcquireNextImageKHR, gdk_vulkan_context_get_device (context), @@ -702,7 +863,7 @@ gdk_vulkan_context_begin_frame (GdkDrawContext *draw_context, UINT64_MAX, draw_semaphore, VK_NULL_HANDLE, - &priv->draw_index); + &present->image_index); if ((acquire_result == VK_ERROR_OUT_OF_DATE_KHR) || (acquire_result == VK_SUBOPTIMAL_KHR)) { @@ -721,7 +882,6 @@ gdk_vulkan_context_begin_frame (GdkDrawContext *draw_context, .pWaitDstStageMask = &mask, }, VK_NULL_HANDLE); - vkQueueWaitIdle (gdk_vulkan_context_get_queue (context)); if (gdk_vulkan_context_has_feature (context, GDK_VULKAN_FEATURE_SWAPCHAIN_MAINTENANCE)) { @@ -735,7 +895,7 @@ gdk_vulkan_context_begin_frame (GdkDrawContext *draw_context, .pNext = NULL, .swapchain = priv->swapchain, .imageIndexCount = 1, - .pImageIndices = &priv->draw_index, + .pImageIndices = &present->image_index, }); } } @@ -750,7 +910,10 @@ gdk_vulkan_context_begin_frame (GdkDrawContext *draw_context, break; } - cairo_region_union (region, priv->regions[priv->draw_index]); + gdk_vulkan_context_release_presents (context, present->image_index); + present->vk_swapchain = priv->swapchain; + + cairo_region_union (region, priv->regions[present->image_index]); if (priv->current_depth == GDK_MEMORY_U8_SRGB) *out_color_state = gdk_color_state_get_no_srgb_tf (color_state); @@ -766,20 +929,23 @@ gdk_vulkan_context_end_frame (GdkDrawContext *draw_context, { GdkVulkanContext *context = GDK_VULKAN_CONTEXT (draw_context); GdkVulkanContextPrivate *priv = gdk_vulkan_context_get_instance_private (context); + GdkVulkanPresent *present; VkPresentRegionsKHR present_regions; VkPresentRegionKHR present_region; VkSwapchainPresentFenceInfoEXT fence_info; void *pNext = NULL; - g_assert (context_data != NULL); + g_assert (context_data == NULL); - if (gdk_vulkan_context_has_feature (context, GDK_VULKAN_FEATURE_SWAPCHAIN_MAINTENANCE)) + present = &priv->presents[priv->latest_present]; + + if (present->vk_fence) { fence_info = (VkSwapchainPresentFenceInfoEXT) { .sType = VK_STRUCTURE_TYPE_SWAPCHAIN_PRESENT_FENCE_INFO_EXT, .pNext = pNext, .swapchainCount = 1, - .pFences = context_data, + .pFences = &present->vk_fence }; pNext = &fence_info; } @@ -815,27 +981,22 @@ gdk_vulkan_context_end_frame (GdkDrawContext *draw_context, GDK_VK_CHECK (vkQueuePresentKHR, gdk_vulkan_context_get_queue (context), &(VkPresentInfoKHR) { .sType = VK_STRUCTURE_TYPE_PRESENT_INFO_KHR, - .waitSemaphoreCount = 0, - .pWaitSemaphores = NULL, + .waitSemaphoreCount = 1, + .pWaitSemaphores = (VkSemaphore[]) { + present->vk_semaphore + }, .swapchainCount = 1, .pSwapchains = (VkSwapchainKHR[]) { priv->swapchain }, .pImageIndices = (uint32_t[]) { - priv->draw_index + present->image_index }, .pNext = pNext, }); - cairo_region_destroy (priv->regions[priv->draw_index]); - priv->regions[priv->draw_index] = cairo_region_create (); - - if (!gdk_vulkan_context_has_feature (context, GDK_VULKAN_FEATURE_SWAPCHAIN_MAINTENANCE)) - { - GDK_VK_CHECK (vkQueueSubmit, gdk_vulkan_context_get_queue (context), - 0, NULL, - *(VkFence *) context_data); - } + cairo_region_destroy (priv->regions[present->image_index]); + priv->regions[present->image_index] = cairo_region_create (); } static gboolean @@ -888,6 +1049,7 @@ gdk_vulkan_context_surface_attach (GdkDrawContext *context, } else { + VkDevice vk_device; uint32_t n_formats; GDK_VK_CHECK (vkGetPhysicalDeviceSurfaceFormatsKHR, gdk_vulkan_context_get_physical_device (self), @@ -987,6 +1149,28 @@ gdk_vulkan_context_surface_attach (GdkDrawContext *context, priv->formats[GDK_MEMORY_FLOAT16] = priv->formats[GDK_MEMORY_FLOAT32]; priv->formats[GDK_MEMORY_NONE] = priv->formats[GDK_MEMORY_U8]; + vk_device = gdk_vulkan_context_get_device (self); + + for (i = 0; i < G_N_ELEMENTS (priv->presents); i++) + { + GDK_VK_CHECK (vkCreateSemaphore, vk_device, + &(VkSemaphoreCreateInfo) { + .sType = VK_STRUCTURE_TYPE_SEMAPHORE_CREATE_INFO, + }, + NULL, + &priv->presents[i].vk_semaphore); + + if (gdk_vulkan_context_has_feature (self, GDK_VULKAN_FEATURE_SWAPCHAIN_MAINTENANCE)) + { + GDK_VK_CHECK (vkCreateFence, vk_device, + &(VkFenceCreateInfo) { + .sType = VK_STRUCTURE_TYPE_FENCE_CREATE_INFO, + }, + NULL, + &priv->presents[i].vk_fence); + } + } + if (!gdk_vulkan_context_check_swapchain (self, error)) goto out_surface; @@ -1006,9 +1190,34 @@ gdk_vulkan_context_surface_detach (GdkDrawContext *context) { GdkVulkanContext *self = GDK_VULKAN_CONTEXT (context); GdkVulkanContextPrivate *priv = gdk_vulkan_context_get_instance_private (self); - VkDevice device; + VkDevice vk_device; guint i; + vk_device = gdk_vulkan_context_get_device (self); + + gdk_vulkan_context_wait_present (self, TRUE); + + for (i = 0; i < G_N_ELEMENTS (priv->presents); i++) + { + g_assert (!gdk_vulkan_present_is_busy (self, &priv->presents[i])); + if (priv->presents[i].vk_swapchain) + { + gdk_vulkan_context_unref_swapchain (self, priv->presents[i].vk_swapchain); + priv->presents[i].vk_swapchain = NULL; + } + vkDestroySemaphore (vk_device, + priv->presents[i].vk_semaphore, + NULL); + priv->presents[i].vk_semaphore = VK_NULL_HANDLE; + if (priv->presents[i].vk_fence) + { + vkDestroyFence (vk_device, + priv->presents[i].vk_fence, + NULL); + priv->presents[i].vk_fence = VK_NULL_HANDLE; + } + } + for (i = 0; i < priv->n_images; i++) { cairo_region_destroy (priv->regions[i]); @@ -1017,13 +1226,9 @@ gdk_vulkan_context_surface_detach (GdkDrawContext *context) g_clear_pointer (&priv->images, g_free); priv->n_images = 0; - device = gdk_vulkan_context_get_device (self); - if (priv->swapchain != VK_NULL_HANDLE) { - vkDestroySwapchainKHR (device, - priv->swapchain, - NULL); + gdk_vulkan_context_unref_swapchain (self, priv->swapchain); priv->swapchain = VK_NULL_HANDLE; } @@ -1470,7 +1675,17 @@ gdk_vulkan_context_get_draw_index (GdkVulkanContext *context) g_return_val_if_fail (GDK_IS_VULKAN_CONTEXT (context), 0); - return priv->draw_index; + return priv->presents[priv->latest_present].image_index; +} + +VkSemaphore +gdk_vulkan_context_get_present_semaphore (GdkVulkanContext *context) +{ + GdkVulkanContextPrivate *priv = gdk_vulkan_context_get_instance_private (context); + + g_return_val_if_fail (GDK_IS_VULKAN_CONTEXT (context), 0); + + return priv->presents[priv->latest_present].vk_semaphore; } static gboolean diff --git a/gdk/gdkvulkancontextprivate.h b/gdk/gdkvulkancontextprivate.h index ac926485d6e..478cf67847f 100644 --- a/gdk/gdkvulkancontextprivate.h +++ b/gdk/gdkvulkancontextprivate.h @@ -95,6 +95,7 @@ uint32_t gdk_vulkan_context_get_n_images (GdkVulk VkImage gdk_vulkan_context_get_image (GdkVulkanContext *context, guint id); uint32_t gdk_vulkan_context_get_draw_index (GdkVulkanContext *context); +VkSemaphore gdk_vulkan_context_get_present_semaphore (GdkVulkanContext *context); #else /* !GDK_RENDERING_VULKAN */ diff --git a/gsk/gpu/gskvulkanframe.c b/gsk/gpu/gskvulkanframe.c index 9b73c0c50e2..0c2b2d49522 100644 --- a/gsk/gpu/gskvulkanframe.c +++ b/gsk/gpu/gskvulkanframe.c @@ -176,28 +176,6 @@ gsk_vulkan_frame_begin (GskGpuFrame *frame, opaque); } -static void -gsk_vulkan_frame_end (GskGpuFrame *frame, - GdkDrawContext *context) -{ - GskVulkanFrame *self = GSK_VULKAN_FRAME (frame); - - gdk_draw_context_end_frame_full (context, &self->vk_fence); -} - -static void -gsk_vulkan_frame_sync (GskGpuFrame *frame) -{ - GskVulkanFrame *self = GSK_VULKAN_FRAME (frame); - GskVulkanDevice *device; - - device = GSK_VULKAN_DEVICE (gsk_gpu_frame_get_device (frame)); - - GSK_VK_CHECK (vkQueueSubmit, gsk_vulkan_device_get_vk_queue (device), - 0, NULL, - self->vk_fence); -} - static GskGpuImage * gsk_vulkan_frame_upload_texture (GskGpuFrame *frame, gboolean with_mipmap, @@ -349,10 +327,13 @@ gsk_vulkan_frame_submit (GskGpuFrame *frame, if (pass_type == GSK_RENDER_PASS_PRESENT) { + GdkVulkanContext *context = GDK_VULKAN_CONTEXT (gsk_gpu_frame_get_context (frame)); gsk_vulkan_semaphores_add_wait (&semaphores, self->vk_acquire_semaphore, 0, VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT); + gsk_vulkan_semaphores_add_signal (&semaphores, + gdk_vulkan_context_get_present_semaphore (context)); } state.vk_command_buffer = self->vk_command_buffer; @@ -385,7 +366,7 @@ gsk_vulkan_frame_submit (GskGpuFrame *frame, .pWaitSemaphoreValues = gsk_semaphore_values_get_data (&semaphores.wait_semaphore_values), } : NULL, }, - VK_NULL_HANDLE); + self->vk_fence); gsk_semaphores_clear (&semaphores.wait_semaphores); gsk_semaphore_values_clear (&semaphores.wait_semaphore_values); @@ -429,8 +410,6 @@ gsk_vulkan_frame_class_init (GskVulkanFrameClass *klass) gpu_frame_class->setup = gsk_vulkan_frame_setup; gpu_frame_class->cleanup = gsk_vulkan_frame_cleanup; gpu_frame_class->begin = gsk_vulkan_frame_begin; - gpu_frame_class->end = gsk_vulkan_frame_end; - gpu_frame_class->sync = gsk_vulkan_frame_sync; gpu_frame_class->upload_texture = gsk_vulkan_frame_upload_texture; gpu_frame_class->create_vertex_buffer = gsk_vulkan_frame_create_vertex_buffer; gpu_frame_class->create_globals_buffer = gsk_vulkan_frame_create_globals_buffer; -- GitLab