From 9d9172fcbbb698770fcf8f1a20af5bc8066cd9b1 Mon Sep 17 00:00:00 2001 From: Normunds Rieksts Date: Fri, 31 Oct 2025 10:35:08 +0000 Subject: [PATCH] Add a workaround when wp_feedback events are not delivered Adds a workaround for compositors that do not deliver wp_presentation_feedback events when images are submitted to the compositor in quick succession resulting in some of them being discarded by handling it in buffer_release event instead. Additionally fixes a double-free bug when presentation_feedback was replaced using move semantics. Change-Id: I97715ffbc45c3c869f84e57dca532d3a58ad3b67 Signed-off-by: Normunds Rieksts --- util/ring_buffer.hpp | 20 ++++++++ wsi/extensions/present_id.hpp | 2 +- wsi/wayland/present_id_wayland.cpp | 41 ++++++++++++++-- wsi/wayland/present_id_wayland.hpp | 26 +++++++++- wsi/wayland/present_timing_handler.cpp | 25 +++++++++- wsi/wayland/present_timing_handler.hpp | 62 ++++++++++++++++-------- wsi/wayland/swapchain.cpp | 20 +++++++- wsi/wayland/wp_presentation_feedback.cpp | 8 +-- wsi/wayland/wp_presentation_feedback.hpp | 8 +-- 9 files changed, 171 insertions(+), 41 deletions(-) diff --git a/util/ring_buffer.hpp b/util/ring_buffer.hpp index e58b3fa..ee7314e 100644 --- a/util/ring_buffer.hpp +++ b/util/ring_buffer.hpp @@ -25,6 +25,7 @@ #include #include #include +#include #pragma once @@ -85,6 +86,25 @@ public: return get((m_begin + m_size + N - 1) % N); } + /** + * @brief Find element in the ring buffer + * + * @param predicate Predicate which returns a true value when element is matching. + * @return Pointer to the element, nullptr otherwise. + */ + T *find(std::function predicate) + { + for (size_t i = 0; i < size(); i++) + { + if (predicate(*m_data[(m_begin + i) % N])) + { + return get((m_begin + i) % N); + } + } + + return nullptr; + } + /** * @brief Pop the front of the ring buffer. * diff --git a/wsi/extensions/present_id.hpp b/wsi/extensions/present_id.hpp index cc81ea2..b19d462 100644 --- a/wsi/extensions/present_id.hpp +++ b/wsi/extensions/present_id.hpp @@ -59,7 +59,7 @@ public: /** * @brief Marks the given present ID delivered (i.e. its image has been displayed). */ - void mark_delivered(uint64_t present_id); + virtual void mark_delivered(uint64_t present_id); /** * @brief Sets the error state for all pending and future image requests. diff --git a/wsi/wayland/present_id_wayland.cpp b/wsi/wayland/present_id_wayland.cpp index 37f2d4f..ef5750a 100644 --- a/wsi/wayland/present_id_wayland.cpp +++ b/wsi/wayland/present_id_wayland.cpp @@ -37,8 +37,22 @@ namespace wsi namespace wayland { -presentation_feedback *wsi_ext_present_id_wayland::insert_into_pending_present_feedback_list( - uint64_t present_id, struct wp_presentation_feedback *feedback_obj) +void wsi_ext_present_id_wayland::mark_delivered(uint64_t present_id) +{ + wsi_ext_present_id::mark_delivered(present_id); + remove_from_pending_present_feedback_list(present_id); +} + +void wsi_ext_present_id_wayland::mark_buffer_release(uint32_t image_index) +{ + const auto present_id = get_present_id_from_image_index(image_index); + if (present_id > 0) + { + mark_delivered(present_id); + } +} + +uint64_t wsi_ext_present_id_wayland::get_present_id_from_image_index(uint32_t image_index) { util::unique_lock lock(m_pending_presents_lock); if (!lock) @@ -46,7 +60,28 @@ presentation_feedback *wsi_ext_present_id_wayland::insert_into_pending_present_f WSI_LOG_ERROR("Failed to acquire pending presents lock in insert_into_pending_present_feedback_list.\n"); abort(); } - bool ret = m_pending_presents.push_back(presentation_feedback(feedback_obj, this, present_id)); + + auto feedback = m_pending_presents.find( + [image_index](const presentation_feedback &feedback) { return feedback.get_image_index() == image_index; }); + + return feedback != nullptr ? feedback->get_present_id() : 0; +} + +presentation_feedback *wsi_ext_present_id_wayland::insert_into_pending_present_feedback_list( + uint64_t present_id, uint32_t image_index, struct wp_presentation_feedback *feedback_obj) +{ + util::unique_lock lock(m_pending_presents_lock); + if (!lock) + { + WSI_LOG_ERROR("Failed to acquire pending presents lock in insert_into_pending_present_feedback_list.\n"); + abort(); + } + + /* We should not be replacing pending entries. This most likely has happened due to events not being triggered + * that would discard or mark the pending present request as completed which could be an inidcation of a bug somewhere. */ + assert(m_pending_presents.size() != m_pending_presents.capacity()); + + bool ret = m_pending_presents.push_back(presentation_feedback(feedback_obj, this, present_id, image_index)); if (!ret) { return nullptr; diff --git a/wsi/wayland/present_id_wayland.hpp b/wsi/wayland/present_id_wayland.hpp index d4cb7b0..2d27f8d 100644 --- a/wsi/wayland/present_id_wayland.hpp +++ b/wsi/wayland/present_id_wayland.hpp @@ -53,15 +53,37 @@ public: /** * @brief Insert into pending present id list. */ - presentation_feedback *insert_into_pending_present_feedback_list(uint64_t present_id, + presentation_feedback *insert_into_pending_present_feedback_list(uint64_t present_id, uint32_t image_index, struct wp_presentation_feedback *feedback_obj); + /** + * @brief Marks the given present ID delivered (i.e. its image has been displayed). + * + * @param present_id Present ID to mark as delivered. + */ + virtual void mark_delivered(uint64_t present_id) override; + + /** + * @brief Marks the buffer as released for the given image index. + * + * @param image_index The index of the image in the swapchain. + */ + void mark_buffer_release(uint32_t image_index); + +private: /** * @brief Remove a present id from the pending present id list. */ void remove_from_pending_present_feedback_list(uint64_t present_id); -private: + /** + * @brief Get the present id from image index object + * + * @param image_index Image index to check for. + * @return Present ID associated with the image index or 0 otherwise. + */ + uint64_t get_present_id_from_image_index(uint32_t image_index); + /** * @brief Mutex for synchronising accesses to the pending present id list. */ diff --git a/wsi/wayland/present_timing_handler.cpp b/wsi/wayland/present_timing_handler.cpp index 19cc17a..bace268 100644 --- a/wsi/wayland/present_timing_handler.cpp +++ b/wsi/wayland/present_timing_handler.cpp @@ -97,6 +97,21 @@ VkResult wsi_ext_present_timing_wayland::get_swapchain_timing_properties( return VK_SUCCESS; } +void wsi_ext_present_timing_wayland::mark_delivered(uint32_t image_index, uint64_t time) +{ + pixelout_callback(image_index, time); + remove_from_pending_present_feedback_list(image_index); +} + +void wsi_ext_present_timing_wayland::mark_buffer_release(uint32_t image_index) +{ + auto was_removed = remove_from_pending_present_feedback_list(image_index); + if (was_removed) + { + pixelout_callback(image_index, 0); + } +} + presentation_feedback *wsi_ext_present_timing_wayland::insert_into_pending_present_feedback_list( uint32_t image_index, struct wp_presentation_feedback *feedback_obj) { @@ -107,11 +122,16 @@ presentation_feedback *wsi_ext_present_timing_wayland::insert_into_pending_prese WSI_LOG_ERROR("Failed to acquire pending presents lock in insert_into_pending_present_feedback_list.\n"); abort(); } + + /* We should not be replacing pending entries. This most likely has happened due to events not being triggered + * that would discard or mark the pending present request as completed which could be an inidcation of a bug somewhere. */ + assert(!m_pending_presents[image_index].has_value()); + m_pending_presents[image_index] = presentation_feedback(feedback_obj, this, image_index); return &m_pending_presents[image_index].value(); } -void wsi_ext_present_timing_wayland::remove_from_pending_present_feedback_list(uint32_t image_index) +bool wsi_ext_present_timing_wayland::remove_from_pending_present_feedback_list(uint32_t image_index) { util::unique_lock lock(m_pending_presents_lock); if (!lock) @@ -119,7 +139,10 @@ void wsi_ext_present_timing_wayland::remove_from_pending_present_feedback_list(u WSI_LOG_ERROR("Failed to acquire pending presents lock in remove_from_pending_present_feedback_list.\n"); abort(); } + + const bool has_entry = m_pending_presents[image_index].has_value(); m_pending_presents[image_index].reset(); + return has_entry; } void wsi_ext_present_timing_wayland::pixelout_callback(uint32_t image_index, uint64_t time) diff --git a/wsi/wayland/present_timing_handler.hpp b/wsi/wayland/present_timing_handler.hpp index 1303ec6..b3ced7e 100644 --- a/wsi/wayland/present_timing_handler.hpp +++ b/wsi/wayland/present_timing_handler.hpp @@ -31,12 +31,15 @@ #if VULKAN_WSI_LAYER_EXPERIMENTAL -#include #include +#include + +#include +#include + #include "surface_properties.hpp" #include "wp_presentation_feedback.hpp" #include "wl_helpers.hpp" -#include namespace wsi { @@ -58,6 +61,24 @@ public: VkResult get_swapchain_timing_properties(uint64_t &timing_properties_counter, VkSwapchainTimingPropertiesEXT &timing_properties) override; + /** + * @brief Marks the given present ID delivered (i.e. its image has been displayed). + * + * @param image_index The index of the image in the swapchain. + * @param time The time to set for the first pixel out stage. + * + * Note: Time value of 0 specifies that request was discarded. + * Note: Time value is in nanoseconds. + */ + void mark_delivered(uint32_t image_index, uint64_t time); + + /** + * @brief Marks the buffer as released for the given image index. + * + * @param image_index The index of the image in the swapchain. + */ + void mark_buffer_release(uint32_t image_index); + /** * @brief Insert into pending present id list. * @@ -69,24 +90,6 @@ public: */ presentation_feedback *insert_into_pending_present_feedback_list(uint32_t image_index, struct wp_presentation_feedback *feedback_obj); - /** - * @brief Remove a present id from the pending present id list. - * - * @param image_index The index of the image to be inserted in the list. - * - */ - void remove_from_pending_present_feedback_list(uint32_t image_index); - - /** - * @brief Updates the first pixel out timing in the internal array. - * - * @param image_index The index of the image in the swapchain. - * - * @param time The time to set for the first pixel out stage. - * - */ - void pixelout_callback(uint32_t image_index, uint64_t time); - /* * @brief Copies the pixel out timestamp from the internal array to the present timing queue. * @@ -122,6 +125,25 @@ private: wsi_ext_present_timing_wayland(const util::allocator &allocator, VkDevice device, uint32_t num_images, util::vector> &×tamp_first_pixel_out_storage); + /** + * @brief Updates the first pixel out timing in the internal array. + * + * @param image_index The index of the image in the swapchain. + * + * @param time The time to set for the first pixel out stage. + * + */ + void pixelout_callback(uint32_t image_index, uint64_t time); + + /** + * @brief Remove a present id from the pending present id list. + * + * @param image_index The index of the image to be inserted in the list. + * + * @return true if entry was removed, false if it was not found. + */ + bool remove_from_pending_present_feedback_list(uint32_t image_index); + wl_display *m_display{}; struct wl_event_queue *m_queue{}; /** diff --git a/wsi/wayland/swapchain.cpp b/wsi/wayland/swapchain.cpp index 23baca5..3b9ebec 100644 --- a/wsi/wayland/swapchain.cpp +++ b/wsi/wayland/swapchain.cpp @@ -235,6 +235,22 @@ void swapchain::release_buffer(struct wl_buffer *wayl_buffer) auto data = reinterpret_cast(m_swapchain_images[i].data); if (data && data->buffer == wayl_buffer) { +#if VULKAN_WSI_LAYER_EXPERIMENTAL + /* Some compositors might not deliver wp_presentation_feedback events if the images are pushed to compositor quick enough + * in presentation modes that allow it (mailbox). If that happens, double check if these images were submitted for feedback + * and handle it as a 'image discarded' event. */ + auto *present_timing_ext = get_swapchain_extension(); + if (present_timing_ext != nullptr) + { + present_timing_ext->mark_buffer_release(i); + } + auto *present_id_ext = get_swapchain_extension(); + if (present_id_ext != nullptr) + { + present_id_ext->mark_buffer_release(i); + } +#endif + unpresent_image(i); break; } @@ -603,8 +619,8 @@ void swapchain::present_image(const pending_present_request &pending_present) wp_presentation *pres = m_wsi_surface->get_presentation_time_interface(); struct wp_presentation_feedback *feedback = wp_presentation_feedback(pres, m_wsi_surface->get_wl_surface()); wl_proxy_set_queue(reinterpret_cast(feedback), m_buffer_queue); - presentation_feedback *feedback_obj = - present_id_ext->insert_into_pending_present_feedback_list(pending_present.present_id, feedback); + presentation_feedback *feedback_obj = present_id_ext->insert_into_pending_present_feedback_list( + pending_present.present_id, pending_present.image_index, feedback); if (feedback_obj == nullptr) { WSI_LOG_ERROR("Error adding to pending present feedback list"); diff --git a/wsi/wayland/wp_presentation_feedback.cpp b/wsi/wayland/wp_presentation_feedback.cpp index 4eaa3ec..951f3df 100644 --- a/wsi/wayland/wp_presentation_feedback.cpp +++ b/wsi/wayland/wp_presentation_feedback.cpp @@ -52,13 +52,11 @@ wp_presentation_feedback_presented(void *data, struct wp_presentation_feedback * timestamp_seconds = static_cast(std::floor(timestamp_limit)); } uint64_t timestamp_ns = static_cast(timestamp_seconds * 1e9) + tv_nsec; - feedback_obj->ext_present_timing()->pixelout_callback(feedback_obj->get_image_index(), timestamp_ns); - feedback_obj->ext_present_timing()->remove_from_pending_present_feedback_list(feedback_obj->get_image_index()); + feedback_obj->ext_present_timing()->mark_delivered(feedback_obj->get_image_index(), timestamp_ns); } else if (feedback_obj->ext_present_id() != nullptr) { feedback_obj->ext_present_id()->mark_delivered(feedback_obj->get_present_id()); - feedback_obj->ext_present_id()->remove_from_pending_present_feedback_list(feedback_obj->get_present_id()); } } @@ -72,12 +70,10 @@ wp_presentation_feedback_discarded(void *data, struct wp_presentation_feedback * if (feedback_obj->ext_present_id() != nullptr) { feedback_obj->ext_present_id()->mark_delivered(feedback_obj->get_present_id()); - feedback_obj->ext_present_id()->remove_from_pending_present_feedback_list(feedback_obj->get_present_id()); } if (feedback_obj->ext_present_timing() != nullptr) { - feedback_obj->ext_present_timing()->pixelout_callback(feedback_obj->get_image_index(), 0); - feedback_obj->ext_present_timing()->remove_from_pending_present_feedback_list(feedback_obj->get_image_index()); + feedback_obj->ext_present_timing()->mark_delivered(feedback_obj->get_image_index(), 0); } } diff --git a/wsi/wayland/wp_presentation_feedback.hpp b/wsi/wayland/wp_presentation_feedback.hpp index 5bc5268..396d5d8 100644 --- a/wsi/wayland/wp_presentation_feedback.hpp +++ b/wsi/wayland/wp_presentation_feedback.hpp @@ -66,12 +66,12 @@ public: { } presentation_feedback(struct wp_presentation_feedback *feedback, wsi_ext_present_id_wayland *ext_present_id, - uint64_t present_id) + uint64_t present_id, uint32_t image_index) : m_feedback(feedback) , m_ext_present_id(ext_present_id) , m_present_id(present_id) , m_ext_present_timing(nullptr) - , m_image_index(0) + , m_image_index(image_index) { } @@ -94,10 +94,6 @@ public: { if (this != &feedback_obj) { - if (m_feedback != nullptr) - { - wp_presentation_feedback_destroy(m_feedback.get()); - } m_present_id = feedback_obj.m_present_id; m_feedback = std::move(feedback_obj.m_feedback); m_ext_present_id = feedback_obj.m_ext_present_id;