JBR-7237 Fix cyclic dependency of Wayland and Vulkan initialization #396
Conversation
| @@ -543,6 +550,16 @@ jboolean VK_FindDevices() { | |||
| geInstance->vkGetPhysicalDeviceQueueFamilyProperties( | |||
| geInstance->physicalDevices[i], &queueFamilyCount, queueFamilies); | |||
| int64_t queueFamily = -1; | |||
| #if defined(VK_USE_PLATFORM_WAYLAND_KHR) | |||
| while (!wl_display_initialized) { | |||
There was a problem hiding this comment.
Did you measure the performance gain from parallel initialization?
There was a problem hiding this comment.
I didn't because I made these changes to solve the cyclic dependency in the first place. Original solution just crashes, so I cannot measure real gain, but I can get a rough estimate.
| @@ -741,6 +744,8 @@ Java_sun_awt_wl_WLToolkit_initIDs | |||
| (JNIEnv *env, jclass clazz) | |||
| { | |||
| wl_display = wl_display_connect(NULL); | |||
| wl_display_initialized = 1; | |||
| syscall(SYS_futex, &wl_display_initialized, FUTEX_WAKE_PRIVATE, INT_MAX, NULL, NULL, 0); | |||
There was a problem hiding this comment.
Why did you use such a kind of synchronization here? (there is only one place in hotspot where JDK use it)
There was a problem hiding this comment.
IDK, I just though futex is an easy, lightweight and already available tool for the case (waiting for the variable to change).
| @@ -0,0 +1,76 @@ | |||
| /* | |||
| * Copyright 2024 JetBrains s.r.o. | |||
There was a problem hiding this comment.
Oracle copyright is also needed here.
| final int deviceNumber = parsedDeviceNumber; | ||
| final boolean verbose = "True".equals(vulkanOption); | ||
| System.loadLibrary("awt"); | ||
| new Thread(() -> { |
There was a problem hiding this comment.
Let's use InnocuousThread for all our JRE-internal threads.
| @@ -741,6 +744,8 @@ Java_sun_awt_wl_WLToolkit_initIDs | |||
| (JNIEnv *env, jclass clazz) | |||
| { | |||
| wl_display = wl_display_connect(NULL); | |||
| wl_display_initialized = 1; | |||
There was a problem hiding this comment.
Seems to early to be certain that it was indeed initialized; wl_display could be NULL at this point.
There was a problem hiding this comment.
Yes, but Vulkan initialization may already be waiting for the wl_display, so we need to notify it even if Wayland initialization failed. There is a null check in Vulkan code too.
| @@ -44,14 +44,16 @@ static const uint32_t REQUIRED_VULKAN_VERSION = VK_MAKE_API_VERSION(0, 1, 2, 0); | |||
| #define VALIDATION_LAYER_NAME "VK_LAYER_KHRONOS_validation" | |||
| #define COUNT_OF(x) (sizeof(x)/sizeof(x[0])) | |||
| #if defined(VK_USE_PLATFORM_WAYLAND_KHR) | |||
| #include <sys/syscall.h> | |||
| #include <linux/futex.h> | |||
| extern volatile uint32_t wl_display_initialized; | |||
There was a problem hiding this comment.
When inter-thread synchronization is spread between the native and Java worlds it becomes especially hard to follow. Maybe we can stay within Java in this instance? By the time WLToolkit::initIDs() exits normally (i.e. not through an exception), the connection to the Wayland server is guaranteed to exist. Possibly this synchronization point can be placed there?
There was a problem hiding this comment.
Yes, that's why I asked about it. Also, we want to extend the Vulkan pipeline to windows, so it's better to have a more general synchronization mechanism.
There was a problem hiding this comment.
By the time WLToolkit::initIDs() exits normally, Vulkan should already be initialized, because it goes through initIDs->notifyOutputConfigured and creates a WLGraphicsDevice, which needs to know about the Vulkan support to create either WLVKGraphicsConfig, or WLSMGraphicsConfig. And Vulkan initialization needs a wl_display connection to query for presentation support, that's why the synchronization is placed here. This is only specific to Linux, as Windows does not depend on any external connection and therefore does not need such synchronization.
|
NB: we decided to split WLToolkit initialization in two phases so that the dependency of the toolkit initialization and Vulkan initialization on the Wayland display can be made apparent and easy to impose. |
YaaZ
changed the title
| @@ -44,14 +44,13 @@ static const uint32_t REQUIRED_VULKAN_VERSION = VK_MAKE_API_VERSION(0, 1, 2, 0); | |||
| #define VALIDATION_LAYER_NAME "VK_LAYER_KHRONOS_validation" | |||
| #define COUNT_OF(x) (sizeof(x)/sizeof(x[0])) | |||
| #if defined(VK_USE_PLATFORM_WAYLAND_KHR) | |||
| extern struct wl_display *wl_display; | |||
| static struct wl_display *wl_display; | |||
There was a problem hiding this comment.
This name clashes with extern struct wl_display *wl_display declared in WLToolkit.c
Maybe rename this one with a different prefix?
There was a problem hiding this comment.
Maybe it's even better to put this info as a field into VKGraphicsEnvironment, it's already platform-dependent so one more field would not hurt.
| @@ -44,14 +44,13 @@ static const uint32_t REQUIRED_VULKAN_VERSION = VK_MAKE_API_VERSION(0, 1, 2, 0); | |||
| #define VALIDATION_LAYER_NAME "VK_LAYER_KHRONOS_validation" | |||
| #define COUNT_OF(x) (sizeof(x)/sizeof(x[0])) | |||
| #if defined(VK_USE_PLATFORM_WAYLAND_KHR) | |||
| extern struct wl_display *wl_display; | |||
| static struct wl_display *wl_display; | |||
There was a problem hiding this comment.
Maybe it's even better to put this info as a field into VKGraphicsEnvironment, it's already platform-dependent so one more field would not hurt.
This adds asynchronous Vulkan initialization.
WLGraphicsEnvironmentstarts Vulkan initialization in separate thread and proceeds with Wayland initializationvkGetPhysicalDeviceWaylandPresentationSupportKHRVulkan init waits for thewl_displayto become availableVKInstance.isVulkanEnabled()which is used later byWLGraphicsDevicewaits for the Vulkan initialization to finish