From b4f0d81a1ae10e14d7f60bdc55940ed51e3789dc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Petit?= <kpet@free.fr>
Date: Fri, 2 Jun 2023 09:06:37 +0100
Subject: [PATCH] Protect changes to memory object mappings with a mutex (#541)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Both command enqueue and execution need to mutate mappings and they
can be concurrent.

Change-Id: Ic94b2a4ddb6170e1ed1b74399d51e9f39df1d3b7

Signed-off-by: Kévin Petit <kpet@free.fr>
---
 src/memory.hpp                            |  8 +++++
 tests/conformance/results-amd-7950x.json  | 28 ++++++++++-----
 tests/conformance/results-intel-a750.json | 42 ++++++++++++++++-------
 3 files changed, 58 insertions(+), 20 deletions(-)

diff --git a/src/memory.hpp b/src/memory.hpp
index 8f7455b5..60d262be 100644
--- a/src/memory.hpp
+++ b/src/memory.hpp
@@ -373,6 +373,7 @@ struct cvk_buffer : public cvk_mem {
     }
 
     bool insert_mapping(const cvk_buffer_mapping& mapping) {
+        std::lock_guard<std::mutex> lock(m_mappings_lock);
         auto num_mappings_with_same_pointer = m_mappings.count(mapping.ptr);
         // TODO support multiple mappings with the same pointer
         if (num_mappings_with_same_pointer != 0) {
@@ -385,6 +386,7 @@ struct cvk_buffer : public cvk_mem {
     }
 
     cvk_buffer_mapping remove_mapping(void* ptr) {
+        std::lock_guard<std::mutex> lock(m_mappings_lock);
         CVK_ASSERT(m_mappings.count(ptr) > 0);
         auto mapping = m_mappings.at(ptr);
         m_mappings.erase(ptr);
@@ -409,6 +411,7 @@ struct cvk_buffer : public cvk_mem {
 
     VkBuffer m_buffer;
     std::unordered_map<void*, cvk_buffer_mapping> m_mappings;
+    std::mutex m_mappings_lock;
 };
 
 using cvk_buffer_holder = refcounted_holder<cvk_buffer>;
@@ -581,6 +584,7 @@ struct cvk_image : public cvk_mem {
                                 std::array<size_t, 3> origin,
                                 std::array<size_t, 3> region,
                                 cl_map_flags flags, bool handle_host_ptr) {
+        std::lock_guard<std::mutex> lock(m_mappings_lock);
         // TODO try to reuse existing mappings
         // TODO add overlap checks
 
@@ -623,12 +627,14 @@ struct cvk_image : public cvk_mem {
             return false;
         }
 
+        // TODO should insertion be deferred, as done for buffers?
         m_mappings[mapping.ptr].push_back(mapping);
 
         return true;
     }
 
     cvk_image_mapping remove_mapping(void* ptr) {
+        std::lock_guard<std::mutex> lock(m_mappings_lock);
         CVK_ASSERT(m_mappings.count(ptr) > 0);
         auto mapping = m_mappings.at(ptr).front();
         m_mappings.at(ptr).pop_front();
@@ -643,6 +649,7 @@ struct cvk_image : public cvk_mem {
     }
 
     cvk_image_mapping mapping_for(void* ptr) {
+        std::lock_guard<std::mutex> lock(m_mappings_lock);
         CVK_ASSERT(m_mappings.count(ptr) > 0);
         auto mapping = m_mappings.at(ptr).front();
         return mapping;
@@ -738,6 +745,7 @@ struct cvk_image : public cvk_mem {
     VkImageView m_sampled_view;
     VkImageView m_storage_view;
     std::unordered_map<void*, std::list<cvk_image_mapping>> m_mappings;
+    std::mutex m_mappings_lock;
     std::unique_ptr<cvk_buffer> m_init_data;
 };
 
diff --git a/tests/conformance/results-amd-7950x.json b/tests/conformance/results-amd-7950x.json
index e46a4c78..e2bea847 100644
--- a/tests/conformance/results-amd-7950x.json
+++ b/tests/conformance/results-amd-7950x.json
@@ -759,16 +759,28 @@
       "retcode": 1
     },
     "Images (clFillImage max size)": {
-      "duration": "00:00:03.073710",
-      "has_results": false,
-      "results": {},
-      "retcode": -6
+      "duration": "00:00:12.015982",
+      "has_results": true,
+      "results": {
+        "1D": "pass",
+        "1Darray": "pass",
+        "2D": "pass",
+        "2Darray": "pass",
+        "3D": "pass"
+      },
+      "retcode": 0
     },
     "Images (clFillImage pitch)": {
-      "duration": "00:00:05.749350",
-      "has_results": false,
-      "results": {},
-      "retcode": -6
+      "duration": "00:00:11.917343",
+      "has_results": true,
+      "results": {
+        "1D": "pass",
+        "1Darray": "pass",
+        "2D": "pass",
+        "2Darray": "pass",
+        "3D": "pass"
+      },
+      "retcode": 0
     },
     "Images (clFillImage)": {
       "duration": "00:00:07.716860",
diff --git a/tests/conformance/results-intel-a750.json b/tests/conformance/results-intel-a750.json
index aa0b620f..4cc004d6 100644
--- a/tests/conformance/results-intel-a750.json
+++ b/tests/conformance/results-intel-a750.json
@@ -759,22 +759,40 @@
       "retcode": 1
     },
     "Images (clFillImage max size)": {
-      "duration": "00:00:11.033466",
-      "has_results": false,
-      "results": {},
-      "retcode": -11
+      "duration": "00:00:34.566830",
+      "has_results": true,
+      "results": {
+        "1D": "pass",
+        "1Darray": "pass",
+        "2D": "pass",
+        "2Darray": "pass",
+        "3D": "pass"
+      },
+      "retcode": 0
     },
     "Images (clFillImage pitch)": {
-      "duration": "00:00:26.927571",
-      "has_results": false,
-      "results": {},
-      "retcode": -11
+      "duration": "00:00:32.019595",
+      "has_results": true,
+      "results": {
+        "1D": "pass",
+        "1Darray": "pass",
+        "2D": "pass",
+        "2Darray": "pass",
+        "3D": "pass"
+      },
+      "retcode": 0
     },
     "Images (clFillImage)": {
-      "duration": "00:00:13.602608",
-      "has_results": false,
-      "results": {},
-      "retcode": -6
+      "duration": "00:00:26.987112",
+      "has_results": true,
+      "results": {
+        "1D": "pass",
+        "1Darray": "pass",
+        "2D": "pass",
+        "2Darray": "pass",
+        "3D": "pass"
+      },
+      "retcode": 0
     },
     "Images (clReadWriteImage max size)": {
       "duration": "00:00:08.666396",