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 <normunds.rieksts@arm.com>

util/ring_buffer.hpp

@@ -25,6 +25,7 @@
 #include <array>
 #include <memory>
 #include <optional>
+#include <functional>
 
 #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<bool(const T &el)> 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.
     *

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.

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<util::mutex> 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<util::mutex> 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;

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.
     */

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<util::mutex> 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)

wsi/wayland/present_timing_handler.hpp

@@ -31,12 +31,15 @@
 
 #if VULKAN_WSI_LAYER_EXPERIMENTAL
 
-#include <wsi/extensions/present_timing.hpp>
 #include <optional>
+#include <array>
+
+#include <wsi/extensions/present_timing.hpp>
+#include <util/custom_mutex.hpp>
+
 #include "surface_properties.hpp"
 #include "wp_presentation_feedback.hpp"
 #include "wl_helpers.hpp"
-#include <util/custom_mutex.hpp>
 
 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<std::optional<uint64_t>> &&timestamp_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{};
    /**

wsi/wayland/swapchain.cpp

@@ -235,6 +235,22 @@ void swapchain::release_buffer(struct wl_buffer *wayl_buffer)
       auto data = reinterpret_cast<wayland_image_data *>(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<wsi_ext_present_timing_wayland>();
+         if (present_timing_ext != nullptr)
+         {
+            present_timing_ext->mark_buffer_release(i);
+         }
+         auto *present_id_ext = get_swapchain_extension<wsi_ext_present_id_wayland>();
+         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<wl_proxy *>(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");

wsi/wayland/wp_presentation_feedback.cpp

@@ -52,13 +52,11 @@ wp_presentation_feedback_presented(void *data, struct wp_presentation_feedback *
          timestamp_seconds = static_cast<uint64_t>(std::floor(timestamp_limit));
       }
       uint64_t timestamp_ns = static_cast<uint64_t>(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);
    }
 }
 

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;