From 9e55766f630dff107c2b007c92a53506d30ac126 Mon Sep 17 00:00:00 2001 From: Hans-Kristian Arntzen Date: Thu, 24 Nov 2022 14:11:05 +0100 Subject: [PATCH] wsi/x11: Fix possible deadlock with wait_ready. With the introduction of locks around the XCB polling mechanism, a possible deadlock was introduced. If all 5 images were rapidly acquired and presented before the FIFO thread had the chance to submit a present, we would deadlock. Before the lock however, it was still buggy since the two threads would race to poll events and update internal state. The fix is to just ensure that there are pending presentation requests in flight, so that forward progress is guaranteed before we take the poll lock. Also, use a timedlock for acquire next image. Similar as WaitForPresentKHR. Also need to make the busy flag atomic to actually allow acquire thread and present threads to access the busy flag. Take advantage of busy flag being atomic so that we can gracefully handle timeout == 0 scenarios where there actually are images available. Signed-off-by: Hans-Kristian Arntzen Fixes: 8fc7927787 ("wsi/x11: Implement VK_KHR_present_wait on X11.) Reviewed-by: Bas Nieuwenhuizen Part-of: --- src/vulkan/wsi/wsi_common_x11.c | 177 ++++++++++++++++++++++++++++---- 1 file changed, 155 insertions(+), 22 deletions(-) diff --git a/src/vulkan/wsi/wsi_common_x11.c b/src/vulkan/wsi/wsi_common_x11.c index fb0ec25d9c6..3bc10ab08fa 100644 --- a/src/vulkan/wsi/wsi_common_x11.c +++ b/src/vulkan/wsi/wsi_common_x11.c @@ -930,7 +930,7 @@ struct x11_image { xcb_pixmap_t pixmap; xcb_xfixes_region_t update_region; /* long lived XID */ xcb_xfixes_region_t update_area; /* the above or None */ - bool busy; + atomic_bool busy; bool present_queued; struct xshmfence * shm_fence; uint32_t sync_fence; @@ -975,6 +975,11 @@ struct x11_swapchain { uint64_t present_id_pending; VkResult present_id_error; + /* For handling wait_ready scenario where two different threads can pump the connection. */ + uint64_t present_submitted_count; + uint64_t present_queue_push_count; + uint64_t present_poll_acquire_count; + struct x11_image images[0]; }; VK_DEFINE_NONDISP_HANDLE_CASTS(x11_swapchain, base.base, VkSwapchainKHR, @@ -996,13 +1001,18 @@ static void x11_present_complete(struct x11_swapchain *swapchain, static void x11_notify_pending_present(struct x11_swapchain *swapchain, struct x11_image *image) { - if (image->present_id) { + if (image->present_id || !swapchain->has_acquire_queue) { pthread_mutex_lock(&swapchain->present_id_mutex); if (image->present_id > swapchain->present_id_pending) { /* Unblock any thread waiting for a presentID out of order. */ swapchain->present_id_pending = image->present_id; - pthread_cond_broadcast(&swapchain->present_id_cond); } + + /* If we don't have an acquire queue, we might need to let + * vkAcquireNextImageKHR call know that it is safe to poll for presentation events. */ + swapchain->present_submitted_count++; + + pthread_cond_broadcast(&swapchain->present_id_cond); pthread_mutex_unlock(&swapchain->present_id_mutex); } } @@ -1223,6 +1233,65 @@ x11_poll_for_special_event(struct x11_swapchain *chain, uint64_t abs_timeout, xc return event ? VK_SUCCESS : VK_TIMEOUT; } +static bool +x11_acquire_next_image_poll_has_forward_progress(struct x11_swapchain *chain) +{ + /* We have forward progress in the sense that we just error out. */ + if (chain->present_id_error != VK_SUCCESS) + return true; + + /* If we got here, there are no available images. + * Some images might be acquired, but not submitted. + * Some images might be submitted to FIFO thread, but not submitted to X yet. */ + + /* If application holds on to images without presenting, it affects forward progress. + * If application holds on to too many images, forward progress may be impossible. + * Application is allowed to call acquire with timeout in these scenarios, but not UINT64_MAX, since it may deadlock. */ + assert(chain->present_poll_acquire_count >= chain->present_queue_push_count); + unsigned application_owned_images = chain->present_poll_acquire_count - chain->present_queue_push_count; + assert(application_owned_images <= chain->base.image_count); + + const unsigned minimum_images = 2; + + /* To observe an IDLE event, we must have submitted at least 2 present requests to X. + * The first present may replace another swapchain's image, but it cannot IDLE one of our own. + * Refuse forward progress until we have observed two completed present requests. + * If we are in a steady state, we only need one present to be able to idle the current image. + * In a blit style composition (windowed mode), images may be idled immediately, so this requirement is relaxed, + * but we have to assume the worst case of FLIP model where the front buffer holds on to one of the swapchain images. */ + if (chain->present_submitted_count < minimum_images) + return false; + + /* Since there are no available images, all images not owned by application have been pushed to FIFO thread. + * There must be at least 2 presents queued up. */ + unsigned present_queued_images = chain->base.image_count - application_owned_images; + if (present_queued_images < minimum_images) + return false; + + /* Present queue must have caught up. */ + return (chain->present_queue_push_count - chain->present_submitted_count) <= + (present_queued_images - minimum_images); +} + +static VkResult +x11_acquire_next_image_poll_find_index(struct x11_swapchain *chain, uint32_t *image_index) +{ + /* We don't need a lock here. AcquireNextImageKHR cannot be called concurrently, + * and busy flag is atomic. */ + for (uint32_t i = 0; i < chain->base.image_count; i++) { + if (!chain->images[i].busy) { + /* We found a non-busy image */ + xshmfence_await(chain->images[i].shm_fence); + *image_index = i; + chain->images[i].busy = true; + chain->present_poll_acquire_count++; + return x11_swapchain_result(chain, VK_SUCCESS); + } + } + + return x11_swapchain_result(chain, VK_NOT_READY); +} + /** * Acquire a ready-to-use image directly from our swapchain. If all images are * busy wait until one is not anymore or till timeout. @@ -1231,7 +1300,15 @@ static VkResult x11_acquire_next_image_poll_x11(struct x11_swapchain *chain, uint32_t *image_index, uint64_t timeout) { + struct timespec rel_timeout, abs_timespec_realtime, start_time; xcb_generic_event_t *event; + VkResult result; + + /* If another thread is pumping the event queue, and we're polling with timeout == 0, + * try a quick poll before we try to take any locks. */ + result = x11_acquire_next_image_poll_find_index(chain, image_index); + if (result != VK_NOT_READY) + return result; uint64_t atimeout; if (timeout == 0 || timeout == UINT64_MAX) @@ -1239,44 +1316,101 @@ x11_acquire_next_image_poll_x11(struct x11_swapchain *chain, else atimeout = os_time_get_absolute_timeout(timeout); - while (1) { - for (uint32_t i = 0; i < chain->base.image_count; i++) { - if (!chain->images[i].busy) { - /* We found a non-busy image */ - xshmfence_await(chain->images[i].shm_fence); - *image_index = i; - chain->images[i].busy = true; - return x11_swapchain_result(chain, VK_SUCCESS); + /* Mutex abs_timeout is in REALTIME timebase. */ + timespec_from_nsec(&rel_timeout, timeout); + clock_gettime(CLOCK_REALTIME, &start_time); + timespec_add(&abs_timespec_realtime, &rel_timeout, &start_time); + + if (chain->has_present_queue) { + /* If we have a present queue (but no acquire queue), + * we might need the present queue to complete + * a request before we can guarantee forward progress in the poll loop below. + * We take the poll_mutex, but so does the present queue. */ + pthread_mutex_lock(&chain->present_id_mutex); + + /* There must be at least one present in-flight that has been committed to X, + * otherwise we can never satisfy the acquire operation if all images are busy, + * since we would be waiting on an event that will never happen. */ + struct timespec abs_timespec; + timespec_from_nsec(&abs_timespec, atimeout); + result = VK_SUCCESS; + + while (!x11_acquire_next_image_poll_has_forward_progress(chain)) { + int ret = pthread_cond_timedwait(&chain->present_id_cond, &chain->present_id_mutex, &abs_timespec); + + if (ret == ETIMEDOUT) { + result = x11_swapchain_result(chain, timeout == 0 ? VK_NOT_READY : VK_TIMEOUT); + break; + } + + if (ret) { + result = VK_ERROR_DEVICE_LOST; + break; } } + if (result == VK_SUCCESS && chain->present_id_error != VK_SUCCESS) + result = chain->present_id_error; + + pthread_mutex_unlock(&chain->present_id_mutex); + + if (result != VK_SUCCESS) + return result; + } + + int ret; + if (timeout == UINT64_MAX) + ret = pthread_mutex_lock(&chain->present_id_poll_mutex); + else + ret = pthread_mutex_timedlock(&chain->present_id_poll_mutex, &abs_timespec_realtime); + + if (ret) { + if (ret == ETIMEDOUT) + return timeout == 0 ? VK_NOT_READY : VK_TIMEOUT; + else + return VK_ERROR_DEVICE_LOST; + } + + while (1) { + result = x11_acquire_next_image_poll_find_index(chain, image_index); + if (result != VK_NOT_READY) + goto out_unlock; + xcb_flush(chain->conn); if (timeout == UINT64_MAX) { /* See comments in x11_manage_fifo_queues about problem scenarios with this call. */ event = xcb_wait_for_special_event(chain->conn, chain->special_event); - if (!event) - return x11_swapchain_result(chain, VK_ERROR_SURFACE_LOST_KHR); + if (!event) { + result = x11_swapchain_result(chain, VK_ERROR_SURFACE_LOST_KHR); + goto out_unlock; + } } else { - VkResult result = x11_poll_for_special_event(chain, atimeout, &event); + result = x11_poll_for_special_event(chain, atimeout, &event); if (result == VK_TIMEOUT) { /* AcquireNextImageKHR reserves a special return value for 0 timeouts. */ - return x11_swapchain_result(chain, timeout == 0 ? VK_NOT_READY : VK_TIMEOUT); + result = x11_swapchain_result(chain, timeout == 0 ? VK_NOT_READY : VK_TIMEOUT); + goto out_unlock; } else if (result != VK_SUCCESS) { - return x11_swapchain_result(chain, result); + result = x11_swapchain_result(chain, result); + goto out_unlock; } } /* Update the swapchain status here. We may catch non-fatal errors here, * in which case we need to update the status and continue. */ - VkResult result = x11_handle_dri3_present_event(chain, (void *)event); + result = x11_handle_dri3_present_event(chain, (void *)event); /* Ensure that VK_SUBOPTIMAL_KHR is reported to the application */ result = x11_swapchain_result(chain, result); free(event); if (result < 0) - return result; + goto out_unlock; } + +out_unlock: + pthread_mutex_unlock(&chain->present_id_poll_mutex); + return result; } /** @@ -1485,6 +1619,7 @@ x11_acquire_next_image(struct wsi_swapchain *anv_chain, if (!chain->images[i].busy) { *image_index = i; chain->images[i].busy = true; + chain->present_poll_acquire_count++; xcb_generic_error_t *err; xcb_get_geometry_cookie_t geom_cookie = xcb_get_geometry(chain->conn, chain->window); @@ -1508,10 +1643,7 @@ x11_acquire_next_image(struct wsi_swapchain *anv_chain, if (chain->has_acquire_queue) { return x11_acquire_next_image_from_queue(chain, image_index, timeout); } else { - pthread_mutex_lock(&chain->present_id_poll_mutex); - VkResult result = x11_acquire_next_image_poll_x11(chain, image_index, timeout); - pthread_mutex_unlock(&chain->present_id_poll_mutex); - return result; + return x11_acquire_next_image_poll_x11(chain, image_index, timeout); } } @@ -1558,6 +1690,7 @@ x11_queue_present(struct wsi_swapchain *anv_chain, chain->images[image_index].busy = true; if (chain->has_present_queue) { wsi_queue_push(&chain->present_queue, image_index); + chain->present_queue_push_count++; return chain->status; } else { /* No present queue means immedate mode, so we present immediately. */