Fix potential hang on swapchain inheritance

Introduce a mutex to control access to a swapchain's image statuses,
also only wait for swapchain images marked as "PENDING"

Change-Id: I7bd530ea50eb44cb98ed4f674167d14de4a30d53
Signed-off-by: Ben Davis <ben.davis@arm.com>

diff --git a/wsi/headless/swapchain.cpp b/wsi/headless/swapchain.cpp
index fdf96272603f95f6472b5d8a70218c799575655e..c082b7c35907199cddabcf40ea67120bbb455bde 100644 (file)
--- a/wsi/headless/swapchain.cpp
+++ b/wsi/headless/swapchain.cpp
@@ -60,6 +60,8 @@ swapchain::~swapchain()
 VkResult swapchain::create_image(VkImageCreateInfo image_create, wsi::swapchain_image &image)
 {
    VkResult res = VK_SUCCESS;
+   const std::lock_guard<std::recursive_mutex> lock(m_image_status_mutex);
+
    res = m_device_data.disp.CreateImage(m_device, &image_create, nullptr, &image.image);
    if (res != VK_SUCCESS)
    {
@@ -131,6 +133,7 @@ void swapchain::present_image(uint32_t pending_index)
 
 void swapchain::destroy_image(wsi::swapchain_image &image)
 {
+   std::unique_lock<std::recursive_mutex> image_status_lock(m_image_status_mutex);
    if (image.status != wsi::swapchain_image::INVALID)
    {
       if (image.present_fence != VK_NULL_HANDLE)
@@ -144,8 +147,12 @@ void swapchain::destroy_image(wsi::swapchain_image &image)
          m_device_data.disp.DestroyImage(m_device, image.image, get_allocation_callbacks());
          image.image = VK_NULL_HANDLE;
       }
+
+      image.status = wsi::swapchain_image::INVALID;
    }
 
+   image_status_lock.unlock();
+
    if (image.data != nullptr)
    {
       auto *data = reinterpret_cast<image_data *>(image.data);
@@ -158,7 +165,6 @@ void swapchain::destroy_image(wsi::swapchain_image &image)
       image.data = nullptr;
    }
 
-   image.status = wsi::swapchain_image::INVALID;
 }
 
 } /* namespace headless */

diff --git a/wsi/swapchain_base.cpp b/wsi/swapchain_base.cpp
index 45cb161d6fcddaf7ffcd19773ff09b0d7528120d..1c60feb3ed603c12ae56fda4ff2991beff810a52 100644 (file)
--- a/wsi/swapchain_base.cpp
+++ b/wsi/swapchain_base.cpp
@@ -84,6 +84,8 @@ void swapchain_base::page_flip_thread()
          continue;
       }
 
+      std::unique_lock<std::recursive_mutex> image_status_lock(m_image_status_mutex);
+
       /* If the descendant has started presenting the queue_present operation has marked the image
        * as FREE so we simply release it and continue. */
       if (sc_images[pending_index].status == swapchain_image::FREE)
@@ -92,6 +94,7 @@ void swapchain_base::page_flip_thread()
          m_free_image_semaphore.post();
          continue;
       }
+      image_status_lock.unlock();
 
       /* First present of the swapchain. If it has an ancestor, wait until all the pending buffers
        * from the ancestor have finished page flipping before we set mode. */
@@ -119,13 +122,15 @@ void swapchain_base::page_flip_thread()
 
 void swapchain_base::unpresent_image(uint32_t presented_index)
 {
+   std::unique_lock<std::recursive_mutex> image_status_lock(m_image_status_mutex);
+
    m_swapchain_images[presented_index].status = swapchain_image::FREE;
 
    if (m_descendant != VK_NULL_HANDLE)
    {
       destroy_image(m_swapchain_images[presented_index]);
    }
-
+   image_status_lock.unlock();
    m_free_image_semaphore.post();
 }
 
@@ -283,7 +288,7 @@ void swapchain_base::teardown()
 {
    /* This method will block until all resources associated with this swapchain
     * are released. Images in the ACQUIRED or FREE state can be freed
-    * immediately. For images in the PRESENTED state, we will block until the
+    * immediately. For images in the PENDING state, we will block until the
     * presentation engine is finished with them. */
 
    int res;
@@ -376,6 +381,8 @@ VkResult swapchain_base::acquire_next_image(uint64_t timeout, VkSemaphore semaph
       return VK_ERROR_OUT_OF_HOST_MEMORY;
    }
 
+   std::unique_lock<std::recursive_mutex> image_status_lock(m_image_status_mutex);
+
    uint32_t i;
    for (i = 0; i < m_swapchain_images.size(); ++i)
    {
@@ -389,6 +396,8 @@ VkResult swapchain_base::acquire_next_image(uint64_t timeout, VkSemaphore semaph
 
    assert(i < m_swapchain_images.size());
 
+   image_status_lock.unlock();
+
    if (VK_NULL_HANDLE != semaphore || VK_NULL_HANDLE != fence)
    {
       VkSubmitInfo submit = { VK_STRUCTURE_TYPE_SUBMIT_INFO };
@@ -452,6 +461,7 @@ VkResult swapchain_base::queue_present(VkQueue queue, const VkPresentInfoKHR *pr
 {
    VkResult result;
    bool descendent_started_presenting = false;
+   const std::lock_guard<std::recursive_mutex> lock(m_image_status_mutex);
 
    if (m_descendant != VK_NULL_HANDLE)
    {
@@ -505,7 +515,6 @@ VkResult swapchain_base::queue_present(VkQueue queue, const VkPresentInfoKHR *pr
 
       m_pending_buffer_pool.ring[m_pending_buffer_pool.tail] = image_index;
       m_pending_buffer_pool.tail = (m_pending_buffer_pool.tail + 1) % m_pending_buffer_pool.size;
-
       m_page_flip_semaphore.post();
 
       return VK_ERROR_OUT_OF_DATE_KHR;
@@ -536,21 +545,21 @@ void swapchain_base::deprecate(VkSwapchainKHR descendant)
 
 void swapchain_base::wait_for_pending_buffers()
 {
-   int num_acquired_images = 0;
    int wait;
+   int pending_images = 0;
+
+   std::unique_lock<std::recursive_mutex> image_status_lock(m_image_status_mutex);
 
    for (auto& img : m_swapchain_images)
    {
-      if (img.status == swapchain_image::ACQUIRED)
+      if (img.status == swapchain_image::PENDING)
       {
-         ++num_acquired_images;
+         ++pending_images;
       }
    }
 
-   /* Once all the pending buffers are flipped, the swapchain should have images
-    * in ACQUIRED (application fails to queue them back for presentation), FREE
-    * and one and only one in PRESENTED. */
-   wait = m_swapchain_images.size() - num_acquired_images - 1;
+   wait = pending_images;
+   image_status_lock.unlock();
 
    while (wait > 0)
    {

diff --git a/wsi/swapchain_base.hpp b/wsi/swapchain_base.hpp
index 7411b8616198c064372f32f38f1f2463e9ea324d..c800e5a512236b5c3c71314a5fc535bf8c35f46a 100644 (file)
--- a/wsi/swapchain_base.hpp
+++ b/wsi/swapchain_base.hpp
@@ -185,6 +185,16 @@ protected:
     */
    sem_t m_start_present_semaphore;
 
+   /**
+    * @brief A mutex to protect access to the statuses of the swapchain's images and
+    * any code paths that rely on this information. We use a recursive mutex as some
+    * functions such as 'destroy_image' both change an image's status and are called
+    * conditionally based on an image's status in some cases. A recursive mutex allows
+    * these functions to be called both with and without the mutex already locked in the
+    * same thread.
+    */
+   std::recursive_mutex m_image_status_mutex;
+
    /**
     * @brief Defines if the pthread_t and sem_t members of the class are defined.
     *

diff --git a/wsi/wayland/swapchain.cpp b/wsi/wayland/swapchain.cpp
index 35846261796914dd08a2de51c43dd31c20c24084..d7a00f72b4a785111865cdde1ff596187966776e 100644 (file)
--- a/wsi/wayland/swapchain.cpp
+++ b/wsi/wayland/swapchain.cpp
@@ -578,9 +578,14 @@ VkResult swapchain::create_image(VkImageCreateInfo image_create_info, swapchain_
       return VK_ERROR_OUT_OF_HOST_MEMORY;
    }
 
+   std::unique_lock<std::recursive_mutex> image_status_lock(m_image_status_mutex);
+
    image.data = image_data;
    image.status = swapchain_image::FREE;
    VkResult result = allocate_image(image_create_info, image_data, &image.image);
+
+   image_status_lock.unlock();
+
    if (result != VK_SUCCESS)
    {
       WSI_LOG_ERROR("Failed to allocate image.");
@@ -721,6 +726,8 @@ void swapchain::present_image(uint32_t pendingIndex)
 
 void swapchain::destroy_image(swapchain_image &image)
 {
+   std::unique_lock<std::recursive_mutex> image_status_lock(m_image_status_mutex);
+
    if (image.status != swapchain_image::INVALID)
    {
       if (image.present_fence != VK_NULL_HANDLE)
@@ -734,7 +741,12 @@ void swapchain::destroy_image(swapchain_image &image)
          m_device_data.disp.DestroyImage(m_device, image.image, get_allocation_callbacks());
          image.image = VK_NULL_HANDLE;
       }
+
+      image.status = swapchain_image::INVALID;
    }
+
+   image_status_lock.unlock();
+
    if (image.data != nullptr)
    {
       auto image_data = reinterpret_cast<wayland_image_data *>(image.data);
@@ -762,8 +774,6 @@ void swapchain::destroy_image(swapchain_image &image)
       m_allocator.destroy(1, image_data);
       image.data = nullptr;
    }
-
-   image.status = swapchain_image::INVALID;
 }
 
 bool swapchain::free_image_found()