From 2adb2ebb00eae46ad0c30b617254d9fefc6d2a7c Mon Sep 17 00:00:00 2001 From: kd-11 Date: Fri, 25 May 2018 15:34:48 +0300 Subject: [PATCH] overlays: Avoid race condition on remove-on-update views - Improves cleanup code to consist of 2 parts, remove then dispose. Remove does not deallocate the item until dispose is called on it, allowing the backends to first deallocate external references. - Caller is responsible for managing list locking and tracking disposable list of items when external references have been cleaned up before using dispose method. --- rpcs3/Emu/RSX/GL/GLGSRender.cpp | 12 ++- rpcs3/Emu/RSX/Overlays/overlays.h | 150 ++++++++++++++++++++++++------ rpcs3/Emu/RSX/VK/VKGSRender.cpp | 12 ++- 3 files changed, 146 insertions(+), 28 deletions(-) diff --git a/rpcs3/Emu/RSX/GL/GLGSRender.cpp b/rpcs3/Emu/RSX/GL/GLGSRender.cpp index 96c57e5ce5..2c414a4bce 100644 --- a/rpcs3/Emu/RSX/GL/GLGSRender.cpp +++ b/rpcs3/Emu/RSX/GL/GLGSRender.cpp @@ -1468,12 +1468,19 @@ void GLGSRender::flip(int buffer) { if (m_overlay_manager->has_dirty()) { + m_overlay_manager->lock(); + + std::vector uids_to_dispose; + uids_to_dispose.reserve(m_overlay_manager->get_dirty().size()); + for (const auto& view : m_overlay_manager->get_dirty()) { m_ui_renderer.remove_temp_resources(view->uid); + uids_to_dispose.push_back(view->uid); } - m_overlay_manager->clear_dirty(); + m_overlay_manager->unlock(); + m_overlay_manager->dispose(uids_to_dispose); } if (m_overlay_manager->has_visible()) @@ -1481,6 +1488,9 @@ void GLGSRender::flip(int buffer) gl::screen.bind(); glViewport(0, 0, m_frame->client_width(), m_frame->client_height()); + // Lock to avoid modification during run-update chain + std::lock_guard lock(*m_overlay_manager); + for (const auto& view : m_overlay_manager->get_views()) { m_ui_renderer.run(m_frame->client_width(), m_frame->client_height(), 0, *view.get()); diff --git a/rpcs3/Emu/RSX/Overlays/overlays.h b/rpcs3/Emu/RSX/Overlays/overlays.h index d774bad781..ad38a3feb4 100644 --- a/rpcs3/Emu/RSX/Overlays/overlays.h +++ b/rpcs3/Emu/RSX/Overlays/overlays.h @@ -171,13 +171,73 @@ namespace rsx std::vector> m_iface_list; std::vector> m_dirty_list; + shared_mutex m_list_mutex; + std::vector m_uids_to_remove; + std::vector m_type_ids_to_remove; + + bool remove_type(u32 type_id) + { + bool found = false; + for (auto It = m_iface_list.begin(); It != m_iface_list.end();) + { + if (It->get()->type_index == type_id) + { + m_dirty_list.push_back(std::move(*It)); + It = m_iface_list.erase(It); + found = true; + } + else + { + ++It; + } + } + + return found; + } + + bool remove_uid(u32 uid) + { + for (auto It = m_iface_list.begin(); It != m_iface_list.end(); It++) + { + const auto e = It->get(); + if (e->uid == uid) + { + m_dirty_list.push_back(std::move(*It)); + m_iface_list.erase(It); + return true; + } + } + + return false; + } + + void cleanup_internal() + { + for (const auto &uid : m_uids_to_remove) + { + remove_uid(uid); + } + + for (const auto &type_id : m_type_ids_to_remove) + { + remove_type(type_id); + } + + m_uids_to_remove.clear(); + m_type_ids_to_remove.clear(); + } + public: display_manager() {} ~display_manager() {} + // Adds an object to the internal list. Optionally removes other objects of the same type. + // Original handle loses ownership but a usable pointer is returned template T* add(std::unique_ptr& entry, bool remove_existing = true) { + writer_lock lock(m_list_mutex); + T* e = entry.get(); e->uid = m_uid_ctr.fetch_add(1); e->type_index = id_manager::typeinfo::get_index(); @@ -201,6 +261,7 @@ namespace rsx return e; } + // Allocates object and adds to internal list. Returns pointer to created object template T* create(Args&&... args) { @@ -208,37 +269,33 @@ namespace rsx return add(object); } - bool remove(u32 uid) + // Removes item from list if it matches the uid + void remove(u32 uid) { - for (auto It = m_iface_list.begin(); It != m_iface_list.end(); It++) + if (m_list_mutex.try_lock()) { - const auto e = It->get(); - if (e->uid == uid) - { - m_dirty_list.push_back(std::move(*It)); - m_iface_list.erase(It); - return true; - } + remove_uid(uid); + m_list_mutex.unlock(); + } + else + { + m_uids_to_remove.push_back(uid); } - - return false; } + // Removes all objects of this type from the list template - bool remove() + void remove() { const auto type_id = id_manager::typeinfo::get_index(); - for (auto It = m_iface_list.begin(); It != m_iface_list.end();) + if (m_list_mutex.try_lock()) { - if (It->get()->type_index == type_id) - { - m_dirty_list.push_back(std::move(*It)); - It = m_iface_list.erase(It); - } - else - { - ++It; - } + remove_type(type_id); + m_list_mutex.unlock(); + } + else + { + m_type_ids_to_remove.push_back(type_id); } } @@ -254,23 +311,43 @@ namespace rsx return !m_dirty_list.empty(); } + // Returns current list for reading. Caller must ensure synchronization by first locking the list const std::vector>& get_views() const { return m_iface_list; } + // Returns current list of removed objects not yet deallocated for reading. + // Caller must ensure synchronization by first locking the list const std::vector>& get_dirty() const { return m_dirty_list; } - void clear_dirty() + // Deallocate object. Object must first be removed via the remove() functions + void dispose(const std::vector& uids) { - m_dirty_list.clear(); + writer_lock lock(m_list_mutex); + + if (!m_uids_to_remove.empty() || !m_type_ids_to_remove.empty()) + { + cleanup_internal(); + } + + m_dirty_list.erase + ( + std::remove_if(m_dirty_list.begin(), m_dirty_list.end(), [&uids](std::unique_ptr& e) + { + return std::find(uids.begin(), uids.end(), e->uid) != uids.end(); + }) + ); } - user_interface* get(u32 uid) const + // Returns pointer to the object matching the given uid + user_interface* get(u32 uid) { + reader_lock lock(m_list_mutex); + for (const auto& iface : m_iface_list) { if (iface->uid == uid) @@ -280,9 +357,12 @@ namespace rsx return nullptr; } + // Returns pointer to the first object matching the given type template - T* get() const + T* get() { + reader_lock lock(m_list_mutex); + const auto type_id = id_manager::typeinfo::get_index(); for (const auto& iface : m_iface_list) { @@ -294,6 +374,24 @@ namespace rsx return nullptr; } + + // Lock for read-only access (BasicLockable) + void lock() + { + m_list_mutex.lock_shared(); + } + + // Release read-only lock (BasicLockable). May perform internal cleanup before returning + void unlock() + { + m_list_mutex.unlock_shared(); + + if (!m_uids_to_remove.empty() || !m_type_ids_to_remove.empty()) + { + writer_lock lock(m_list_mutex); + cleanup_internal(); + } + } }; struct fps_display : user_interface diff --git a/rpcs3/Emu/RSX/VK/VKGSRender.cpp b/rpcs3/Emu/RSX/VK/VKGSRender.cpp index a9db46ba81..2d90083d3e 100644 --- a/rpcs3/Emu/RSX/VK/VKGSRender.cpp +++ b/rpcs3/Emu/RSX/VK/VKGSRender.cpp @@ -2044,12 +2044,19 @@ void VKGSRender::process_swap_request(frame_context_t *ctx, bool free_resources) if (m_overlay_manager && m_overlay_manager->has_dirty()) { + m_overlay_manager->lock(); + + std::vector uids_to_dispose; + uids_to_dispose.reserve(m_overlay_manager->get_dirty().size()); + for (const auto& view : m_overlay_manager->get_dirty()) { m_ui_renderer->remove_temp_resources(view->uid); + uids_to_dispose.push_back(view->uid); } - m_overlay_manager->clear_dirty(); + m_overlay_manager->unlock(); + m_overlay_manager->dispose(uids_to_dispose); } m_attachment_clear_pass->free_resources(); @@ -3239,6 +3246,9 @@ void VKGSRender::flip(int buffer) if (has_overlay) { + // Lock to avoid modification during run-update chain + std::lock_guard lock(*m_overlay_manager); + for (const auto& view : m_overlay_manager->get_views()) { m_ui_renderer->run(*m_current_command_buffer, direct_fbo->width(), direct_fbo->height(), direct_fbo.get(), single_target_pass, m_texture_upload_buffer_ring_info, *view.get());