Update vkMapMemory to not hold lock when calling into enc

... as this can lead to a deadlock with the following sequence:

Time1: guest-thread-1: vkDestroyBuffer() called
Time2:                 VkEncoder grabs seqno 1

Time3: guest-thread-2: vkMapMemory() called
Time4:                 ResourceTracker::on_vkMapMemory() locks mLock
                       for using `info_VkDeviceMemory`
Time5:                 ResourceTracker::on_vkMapMemory() calls
                       enc->vkGetBlobGOOGLE()
Time6:                 VkEncoder grabs seqno 2
Time7:                 VkEncoder sends the vkGetBlobGOOGLE with seqno
                       2 via ASG to host
Time8:                 VkEncoder waits for the `VkResult` from the
                       host via `stream->read()`

Time9: guest-thread-1: VkEncoder calls sResourceTracker->destroyMapping()
                       ->mapHandles_VkBuffer((VkBuffer*)&buffer);
                       which calls ResourceTracker::unregister_VkBuffer()

                       ResourceTracker::unregister_VkBuffer() tries to
                       locks mLock to erase the buffer's info struct
                       from `info_VkBuffer`

!!! DEADLOCKED HERE !!!

guest-thread-1 is stuck waiting on mLock (currently locked by
guest-thread-2) before it would `stream->flush();` to finishing
sending the vkDestroyBuffer() command to the host and potentially
ping its corresponding host-render-thread-1.

guest-thread-2 stuck waiting on the result from host-render-thread-2
but host-render-thread-2 won't progress until host-render-thread-1
finishes seqno 1 which needs guest-thread-1 to finish sending/pinging.

Bug: b/337101904
Test: GfxstreamEnd2EndTests
Test: gfxbench's aztec ruins x5 in a row
Change-Id: I6053d31636c477abf6632c9308bcffda96402397

2 files changed, 99 insertions, 29 deletions

diff --git a/common/end2end/GfxstreamEnd2EndVkTests.cpp b/common/end2end/GfxstreamEnd2EndVkTests.cpp
index e3c52a52..5ff9ce9a 100644
--- a/common/end2end/GfxstreamEnd2EndVkTests.cpp
+++ b/common/end2end/GfxstreamEnd2EndVkTests.cpp
@@ -869,6 +869,71 @@ TEST_P(GfxstreamEnd2EndVkTest, DescriptorUpdateTemplateWithWrapping) {
                                             descriptorInfo.data());
 }
 
+TEST_P(GfxstreamEnd2EndVkTest, MultiThreadedVkMapMemory) {
+    auto [instance, physicalDevice, device, queue, queueFamilyIndex] =
+        VK_ASSERT(SetUpTypicalVkTestEnvironment());
+
+    static constexpr const vkhpp::DeviceSize kSize = 1024;
+    const vkhpp::BufferCreateInfo bufferCreateInfo = {
+        .size = kSize,
+        .usage = vkhpp::BufferUsageFlagBits::eTransferSrc,
+    };
+    auto buffer = device->createBufferUnique(bufferCreateInfo).value;
+
+    vkhpp::MemoryRequirements bufferMemoryRequirements{};
+    device->getBufferMemoryRequirements(*buffer, &bufferMemoryRequirements);
+
+    const uint32_t bufferMemoryIndex = GetMemoryType(
+        physicalDevice, bufferMemoryRequirements,
+        vkhpp::MemoryPropertyFlagBits::eHostVisible | vkhpp::MemoryPropertyFlagBits::eHostCoherent);
+    if (bufferMemoryIndex == -1) {
+        GTEST_SKIP() << "Skipping test due to no memory type with HOST_VISIBLE | HOST_COHERENT.";
+    }
+
+    std::vector<std::thread> threads;
+    std::atomic_int threadsReady{0};
+
+    constexpr const int kNumThreads = 2;
+    for (int t = 0; t < kNumThreads; t++) {
+        threads.emplace_back([&, this]() {
+            // Perform some work to ensure host RenderThread started.
+            auto buffer2 = device->createBufferUnique(bufferCreateInfo).value;
+            ASSERT_THAT(buffer2, IsValidHandle());
+
+            ++threadsReady;
+            while (threadsReady.load() != kNumThreads) {
+            }
+
+            constexpr const int kNumIterations = 100;
+            for (int i = 0; i < kNumIterations; i++) {
+                auto buffer3 = device->createBufferUnique(bufferCreateInfo).value;
+                ASSERT_THAT(buffer3, IsValidHandle());
+
+                const vkhpp::MemoryAllocateInfo buffer3MemoryAllocateInfo = {
+                    .allocationSize = bufferMemoryRequirements.size,
+                    .memoryTypeIndex = bufferMemoryIndex,
+                };
+                auto buffer3Memory = device->allocateMemoryUnique(buffer3MemoryAllocateInfo).value;
+                ASSERT_THAT(buffer3Memory, IsValidHandle());
+
+                ASSERT_THAT(device->bindBufferMemory(*buffer3, *buffer3Memory, 0), IsVkSuccess());
+
+                void* mapped = nullptr;
+                ASSERT_THAT(device->mapMemory(*buffer3Memory, 0, VK_WHOLE_SIZE,
+                                              vkhpp::MemoryMapFlags{}, &mapped),
+                            IsVkSuccess());
+                ASSERT_THAT(mapped, NotNull());
+
+                device->unmapMemory(*buffer3Memory);
+            }
+        });
+    }
+
+    for (auto& thread : threads) {
+        thread.join();
+    }
+}
+
 std::vector<TestParams> GenerateTestCases() {
     std::vector<TestParams> cases = {TestParams{
                                          .with_gl = false,
diff --git a/guest/vulkan_enc/ResourceTracker.cpp b/guest/vulkan_enc/ResourceTracker.cpp
index dcbef3c8..164ddc35 100644
--- a/guest/vulkan_enc/ResourceTracker.cpp
+++ b/guest/vulkan_enc/ResourceTracker.cpp
@@ -3936,68 +3936,73 @@ VkResult ResourceTracker::on_vkMapMemory(void* context, VkResult host_result, Vk
                                          VkDeviceMemory memory, VkDeviceSize offset,
                                          VkDeviceSize size, VkMemoryMapFlags, void** ppData) {
     if (host_result != VK_SUCCESS) {
-        ALOGE("%s: Host failed to map\n", __func__);
+        ALOGE("%s: Host failed to map", __func__);
         return host_result;
     }
 
     AutoLock<RecursiveLock> lock(mLock);
 
-    auto it = info_VkDeviceMemory.find(memory);
-    if (it == info_VkDeviceMemory.end()) {
-        ALOGE("%s: Could not find this device memory\n", __func__);
+    auto deviceMemoryInfoIt = info_VkDeviceMemory.find(memory);
+    if (deviceMemoryInfoIt == info_VkDeviceMemory.end()) {
+        ALOGE("%s: Failed to find VkDeviceMemory.", __func__);
         return VK_ERROR_MEMORY_MAP_FAILED;
     }
+    auto& deviceMemoryInfo = deviceMemoryInfoIt->second;
 
-    auto& info = it->second;
-
-    if (info.blobId && !info.coherentMemory && !mCaps.params[kParamCreateGuestHandle]) {
+    if (deviceMemoryInfo.blobId && !deviceMemoryInfo.coherentMemory &&
+        !mCaps.params[kParamCreateGuestHandle]) {
+        // NOTE: must not hold lock while calling into the encoder.
+        lock.unlock();
         VkEncoder* enc = (VkEncoder*)context;
-        VirtGpuResourceMappingPtr mapping;
-        VirtGpuDevice* instance = VirtGpuDevice::getInstance();
-
-        uint64_t offset;
-        uint8_t* ptr;
+        VkResult vkResult = enc->vkGetBlobGOOGLE(device, memory, /*doLock*/ false);
+        if (vkResult != VK_SUCCESS) {
+            ALOGE("%s: Failed to vkGetBlobGOOGLE().", __func__);
+            return vkResult;
+        }
+        lock.lock();
 
-        VkResult vkResult = enc->vkGetBlobGOOGLE(device, memory, false);
-        if (vkResult != VK_SUCCESS) return vkResult;
+        // NOTE: deviceMemoryInfoIt potentially invalidated but deviceMemoryInfo still okay.
 
         struct VirtGpuCreateBlob createBlob = {};
         createBlob.blobMem = kBlobMemHost3d;
         createBlob.flags = kBlobFlagMappable;
-        createBlob.blobId = info.blobId;
-        createBlob.size = info.coherentMemorySize;
+        createBlob.blobId = deviceMemoryInfo.blobId;
+        createBlob.size = deviceMemoryInfo.coherentMemorySize;
 
-        auto blob = instance->createBlob(createBlob);
+        auto blob = VirtGpuDevice::getInstance()->createBlob(createBlob);
         if (!blob) return VK_ERROR_OUT_OF_DEVICE_MEMORY;
 
-        mapping = blob->createMapping();
+        VirtGpuResourceMappingPtr mapping = blob->createMapping();
         if (!mapping) return VK_ERROR_OUT_OF_DEVICE_MEMORY;
 
         auto coherentMemory =
             std::make_shared<CoherentMemory>(mapping, createBlob.size, device, memory);
 
-        coherentMemory->subAllocate(info.allocationSize, &ptr, offset);
+        uint8_t* ptr;
+        uint64_t offset;
+        coherentMemory->subAllocate(deviceMemoryInfo.allocationSize, &ptr, offset);
 
-        info.coherentMemoryOffset = offset;
-        info.coherentMemory = coherentMemory;
-        info.ptr = ptr;
+        deviceMemoryInfo.coherentMemoryOffset = offset;
+        deviceMemoryInfo.coherentMemory = coherentMemory;
+        deviceMemoryInfo.ptr = ptr;
     }
 
-    if (!info.ptr) {
-        ALOGE("%s: ptr null\n", __func__);
+    if (!deviceMemoryInfo.ptr) {
+        ALOGE("%s: VkDeviceMemory has nullptr.", __func__);
         return VK_ERROR_MEMORY_MAP_FAILED;
     }
 
-    if (size != VK_WHOLE_SIZE && (info.ptr + offset + size > info.ptr + info.allocationSize)) {
+    if (size != VK_WHOLE_SIZE && (deviceMemoryInfo.ptr + offset + size >
+                                  deviceMemoryInfo.ptr + deviceMemoryInfo.allocationSize)) {
         ALOGE(
             "%s: size is too big. alloc size 0x%llx while we wanted offset 0x%llx size 0x%llx "
-            "total 0x%llx\n",
-            __func__, (unsigned long long)info.allocationSize, (unsigned long long)offset,
-            (unsigned long long)size, (unsigned long long)offset);
+            "total 0x%llx",
+            __func__, (unsigned long long)deviceMemoryInfo.allocationSize,
+            (unsigned long long)offset, (unsigned long long)size, (unsigned long long)offset);
         return VK_ERROR_MEMORY_MAP_FAILED;
     }
 
-    *ppData = info.ptr + offset;
+    *ppData = deviceMemoryInfo.ptr + offset;
 
     return host_result;
 }