Move queue for frame events to the surface object

Moves the handling of frame done events from the swapchain object to the
surface object. This will prevent failures where a frame done event
arrives after the swapchain has been destructed but the surface hasn't.

Uses wayland_owners objects for managing the surface event queue and the
frame done event callback.

Signed-off-by: Iason Paraskevopoulos <iason.paraskevopoulos@arm.com>
Change-Id: I61459440e7a6fe8d9447d13f65b797cfcf05f0ed

wsi/wayland/surface.cpp

@@ -130,6 +130,8 @@ surface::surface(const init_parameters &params)
    , supported_formats(params.allocator)
    , properties(*this, params.allocator)
    , surface_queue(nullptr)
+   , last_frame_callback(nullptr)
+   , present_pending(false)
 {
 }
 
@@ -169,15 +171,14 @@ void surface_registry_handler(void *data, struct wl_registry *wl_registry, uint3
 
 bool surface::init()
 {
-   surface_queue = wl_display_create_queue(wayland_display);
-
-   if (surface_queue == nullptr)
+   surface_queue.reset(wl_display_create_queue(wayland_display));
+   if (surface_queue.get() == nullptr)
    {
       WSI_LOG_ERROR("Failed to create wl surface queue.");
       return false;
    }
 
-   auto display_proxy = make_proxy_with_queue(wayland_display, surface_queue);
+   auto display_proxy = make_proxy_with_queue(wayland_display, surface_queue.get());
    if (display_proxy == nullptr)
    {
       WSI_LOG_ERROR("Failed to create wl display proxy.");
@@ -199,7 +200,7 @@ bool surface::init()
       return false;
    }
 
-   res = wl_display_roundtrip_queue(wayland_display, surface_queue);
+   res = wl_display_roundtrip_queue(wayland_display, surface_queue.get());
    if (res < 0)
    {
       WSI_LOG_ERROR("Roundtrip failed.");
@@ -228,8 +229,8 @@ bool surface::init()
 
    surface_sync_interface.reset(surface_sync_obj);
 
-   VkResult vk_res =
-      get_supported_formats_and_modifiers(wayland_display, surface_queue, dmabuf_interface.get(), supported_formats);
+   VkResult vk_res = get_supported_formats_and_modifiers(wayland_display, surface_queue.get(), dmabuf_interface.get(),
+                                                         supported_formats);
    if (vk_res != VK_SUCCESS)
    {
       return false;
@@ -255,10 +256,6 @@ util::unique_ptr<surface> surface::make_surface(const util::allocator &allocator
 
 surface::~surface()
 {
-   if (surface_queue != nullptr)
-   {
-      wl_event_queue_destroy(surface_queue);
-   }
 }
 
 wsi::surface_properties &surface::get_properties()
@@ -273,5 +270,73 @@ util::unique_ptr<swapchain_base> surface::allocate_swapchain(layer::device_priva
    return util::unique_ptr<swapchain_base>(alloc.make_unique<swapchain>(dev_data, allocator, *this));
 }
 
+static void frame_done(void *data, wl_callback *cb, uint32_t time)
+{
+   (void)time;
+
+   bool *present_pending = reinterpret_cast<bool *>(data);
+   assert(present_pending);
+
+   *present_pending = false;
+}
+
+bool surface::set_frame_callback()
+{
+   /* request a hint when we can present the _next_ frame */
+   auto surface_proxy = make_proxy_with_queue(wayland_surface, surface_queue.get());
+   if (surface_proxy == nullptr)
+   {
+      WSI_LOG_ERROR("failed to create wl_surface proxy");
+      return false;
+   }
+
+   /* Reset will also destroy the previous callback object. */
+   last_frame_callback.reset(wl_surface_frame(surface_proxy.get()));
+   if (last_frame_callback.get() == nullptr)
+   {
+      WSI_LOG_ERROR("Failed to create frame callback.");
+      return false;
+   }
+
+   static const wl_callback_listener frame_listener = { frame_done };
+   present_pending = true;
+   int res = wl_callback_add_listener(last_frame_callback.get(), &frame_listener, &present_pending);
+   if (res < 0)
+   {
+      WSI_LOG_ERROR("Failed to add frame done callback listener.");
+      return false;
+   }
+
+   return true;
+}
+
+bool surface::wait_next_frame_event()
+{
+   /*
+    * In a previous present call we sent a wl_surface::frame request, which will
+    * trigger an event when the compositor starts a redraw using the previous frame
+    * we sent. If the compositor isn't sending us frame events at least every second
+    * we don't wait indefinitely so we don't block the next image presentation if
+    * we are, e.g. minimised.
+    */
+   const int timeout = 1000;
+   while (present_pending)
+   {
+      int res = dispatch_queue(wayland_display, surface_queue.get(), timeout);
+      if (res < 0)
+      {
+         WSI_LOG_ERROR("Error while waiting for the compositor to send the next frame event.");
+         return false;
+      }
+      else if (res == 0)
+      {
+         WSI_LOG_INFO("Wait for frame event timed out, present anyway.");
+         present_pending = false;
+      }
+   }
+
+   return true;
+}
+
 } // namespace wayland
 } // namespace wsi

wsi/wayland/surface.hpp

@@ -122,6 +122,23 @@ public:
       return supported_formats;
    }
 
+   /**
+    * @brief Set the next frame callback.
+    *
+    * Make a frame request on the compositor which will be applied in the next
+    * wl_surface::commit. It overwrites previous requested frame events.
+    *
+    * @return true for success, false otherwise.
+    */
+   bool set_frame_callback();
+
+   /**
+    * @brief Wait for the compositor's last requested frame event.
+    *
+    * @return true for success, false otherwise.
+    */
+   bool wait_next_frame_event();
+
 private:
    /**
     * @brief Initialize the WSI surface by creating Wayland queues and linking to Wayland protocols.
@@ -150,8 +167,29 @@ private:
    /** Container for the surface specific zwp_linux_surface_synchronization_v1 interface. */
    wayland_owner<zwp_linux_surface_synchronization_v1> surface_sync_interface;
 
-   /** Private queue for surface events generated by the layer */
-   wl_event_queue *surface_queue;
+   /**
+    * Container for a private queue for surface events generated by the layer.
+    * The queue is also used for dispatching frame callback events.
+    */
+   wayland_owner<wl_event_queue> surface_queue;
+
+   /**
+    * Container for a callback object for the latest frame done event.
+    *
+    * The callback object should be destroyed before the queue so any new events
+    * on the queue will be discarded. If a proxy object is destroyed after a queue,
+    * it is possible in the meantime for a new event to arrive and processed.
+    * This will result in a use after free error.
+    */
+   wayland_owner<wl_callback> last_frame_callback;
+
+   /**
+    * @brief true when waiting for the server hint to present a buffer
+    *
+    * true if a buffer has been presented and we've not had a wl_surface::frame
+    * callback to indicate the server is ready for the next buffer.
+    */
+   bool present_pending;
 };
 
 } // namespace wayland

wsi/wayland/swapchain.cpp

@@ -81,7 +81,6 @@ swapchain::swapchain(layer::device_private_data &dev_data, const VkAllocationCal
    , m_buffer_queue(nullptr)
    , m_wsi_allocator(nullptr)
    , m_image_creation_parameters({}, {}, m_allocator, {}, {})
-   , m_present_pending(false)
 {
    m_image_creation_parameters.m_image_create_info.format = VK_FORMAT_UNDEFINED;
 }
@@ -646,42 +645,15 @@ VkResult swapchain::create_and_bind_swapchain_image(VkImageCreateInfo image_crea
    return VK_SUCCESS;
 }
 
-static void frame_done(void *data, wl_callback *cb, uint32_t cb_data)
-{
-   (void)cb_data;
-
-   bool *present_pending = reinterpret_cast<bool *>(data);
-   assert(present_pending);
-
-   *present_pending = false;
-
-   wl_callback_destroy(cb);
-}
-
 void swapchain::present_image(uint32_t pendingIndex)
 {
    int res;
    wayland_image_data *image_data = reinterpret_cast<wayland_image_data *>(m_swapchain_images[pendingIndex].data);
-   /* if a frame is already pending, wait for a hint to present again */
-   if (m_present_pending)
-   {
-      assert(m_present_mode == VK_PRESENT_MODE_FIFO_KHR);
-      do
-      {
-         /* block waiting for the compositor to return the wl_surface::frame
-          * callback. We may want to change this to timeout after a period of
-          * time if the compositor isn't responding (perhaps because the
-          * window is hidden).
-          */
-         res = dispatch_queue(m_display, m_swapchain_queue, -1);
-      } while (res > 0 && m_present_pending);
 
-      if (res <= 0)
-      {
-         WSI_LOG_ERROR("error waiting for Wayland compositor frame hint");
-         m_is_valid = false;
-         /* try to present anyway */
-      }
+   /* if a frame is already pending, wait for a hint to present again */
+   if (!m_wsi_surface->wait_next_frame_event())
+   {
+      m_is_valid = false;
    }
 
    wl_surface_attach(m_surface, image_data->buffer, 0, 0);
@@ -703,21 +675,9 @@ void swapchain::present_image(uint32_t pendingIndex)
 
    if (m_present_mode == VK_PRESENT_MODE_FIFO_KHR)
    {
-      /* request a hint when we can present the _next_ frame */
-      auto surface_proxy = make_proxy_with_queue(m_surface, m_swapchain_queue);
-      if (surface_proxy == nullptr)
+      if (!m_wsi_surface->set_frame_callback())
       {
-         WSI_LOG_ERROR("failed to create wl_surface proxy");
          m_is_valid = false;
-         return;
-      }
-
-      wl_callback *cb = wl_surface_frame(surface_proxy.get());
-      if (cb != nullptr)
-      {
-         static const wl_callback_listener frame_listener = { frame_done };
-         m_present_pending = true;
-         wl_callback_add_listener(cb, &frame_listener, &m_present_pending);
       }
    }
 

wsi/wayland/swapchain.hpp

@@ -189,14 +189,6 @@ private:
     */
    struct image_creation_parameters m_image_creation_parameters;
 
-   /**
-    * @brief true when waiting for the server hint to present a buffer
-    *
-    * true if a buffer has been presented and we've not had a wl_surface::frame
-    * callback to indicate the server is ready for the next buffer.
-    */
-   bool m_present_pending;
-
    /*
     * @brief Allocate memory for an image plane.
     *

wsi/wayland/wl_object_owner.hpp

@@ -55,6 +55,16 @@ static inline void wayland_object_destroy(zwp_linux_surface_synchronization_v1 *
    zwp_linux_surface_synchronization_v1_destroy(obj);
 }
 
+static inline void wayland_object_destroy(wl_callback *obj)
+{
+   wl_callback_destroy(obj);
+}
+
+static inline void wayland_object_destroy(wl_event_queue *obj)
+{
+   wl_event_queue_destroy(obj);
+}
+
 template <typename T>
 struct wayland_deleter
 {