diff options
author | Jason Macnak <natsu@google.com> | 2024-04-26 15:54:09 -0700 |
---|---|---|
committer | Jason Macnak <natsu@google.com> | 2024-04-29 08:49:10 -0700 |
commit | 24e20f84e120777ea16d8b26737ace0a989363fc (patch) | |
tree | ec26c39743eecf8f5404e337a485c719e6d649ba | |
parent | e8475e055062bc0e5804bc6af8f48ca636777520 (diff) | |
download | gfxstream-24e20f84e120777ea16d8b26737ace0a989363fc.tar.gz |
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
-rw-r--r-- | common/end2end/GfxstreamEnd2EndVkTests.cpp | 65 | ||||
-rw-r--r-- | guest/vulkan_enc/ResourceTracker.cpp | 63 |
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; } |