From b6b15a0abea87ef87d9c0dfd4989d286b72aa66d Mon Sep 17 00:00:00 2001 From: Ming Zheng Date: Fri, 5 Jan 2024 09:22:05 -0500 Subject: [PATCH 1/2] Fix trim raytracing issue Trim raytracing handling use map_device of memory object wrapper which is not initialized, the error make some title run into crash with GFXR trim capturing. The commit fixed the issue. --- framework/encode/vulkan_handle_wrapper_util.h | 11 +++++++++++ framework/encode/vulkan_handle_wrappers.h | 8 +++++--- framework/encode/vulkan_state_tracker.cpp | 5 ++--- framework/encode/vulkan_state_writer.cpp | 2 +- 4 files changed, 19 insertions(+), 7 deletions(-) diff --git a/framework/encode/vulkan_handle_wrapper_util.h b/framework/encode/vulkan_handle_wrapper_util.h index 1d66daf783..fae3d154fc 100644 --- a/framework/encode/vulkan_handle_wrapper_util.h +++ b/framework/encode/vulkan_handle_wrapper_util.h @@ -279,6 +279,17 @@ inline void CreateWrappedHandle(VK_NULL_HANDLE, handle, get_id); } +template <> +inline void CreateWrappedHandle(VkDevice device, + NoParentWrapper::HandleType, + VkDeviceMemory* handle, + PFN_GetHandleId get_id) +{ + CreateWrappedNonDispatchHandle(handle, get_id); + auto memory_wrapper = GetWrapper(*handle); + memory_wrapper->device = GetWrapper(device); +} + template <> inline void CreateWrappedHandle( VkDevice parent, diff --git a/framework/encode/vulkan_handle_wrappers.h b/framework/encode/vulkan_handle_wrappers.h index ad2d433d96..8c6542f1aa 100644 --- a/framework/encode/vulkan_handle_wrappers.h +++ b/framework/encode/vulkan_handle_wrappers.h @@ -162,9 +162,11 @@ struct EventWrapper : public HandleWrapper struct DeviceMemoryWrapper : public HandleWrapper { - uint32_t memory_type_index{ std::numeric_limits::max() }; - VkDeviceSize allocation_size{ 0 }; - DeviceWrapper* map_device{ nullptr }; + uint32_t memory_type_index{ std::numeric_limits::max() }; + VkDeviceSize allocation_size{ 0 }; + // This is the device which was used to allocate the memory. By doc, + // if the memory can be mapped, map device must be this device. + DeviceWrapper* device{ nullptr }; const void* mapped_data{ nullptr }; VkDeviceSize mapped_offset{ 0 }; VkDeviceSize mapped_size{ 0 }; diff --git a/framework/encode/vulkan_state_tracker.cpp b/framework/encode/vulkan_state_tracker.cpp index a3712a0474..fd6ff31d69 100644 --- a/framework/encode/vulkan_state_tracker.cpp +++ b/framework/encode/vulkan_state_tracker.cpp @@ -433,7 +433,6 @@ void VulkanStateTracker::TrackMappedMemory(VkDevice device, assert((device != VK_NULL_HANDLE) && (memory != VK_NULL_HANDLE)); auto wrapper = GetWrapper(memory); - wrapper->map_device = GetWrapper(device); wrapper->mapped_data = mapped_data; wrapper->mapped_offset = mapped_offset; wrapper->mapped_size = mapped_size; @@ -1561,7 +1560,7 @@ void VulkanStateTracker::TrackTlasToBlasDependencies(uint32_t comm { // If PageGuardManager is not used or if it couldn't find the memory id it means that // we need to map the memory. - VkDevice device = dev_mem_wrapper->map_device->handle; + VkDevice device = dev_mem_wrapper->device->handle; const DeviceTable* device_table = GetDeviceTable(device); const VkDeviceSize map_size = sizeof(VkAccelerationStructureInstanceKHR) * blas_count; void* mapped_memory = nullptr; @@ -1596,7 +1595,7 @@ void VulkanStateTracker::TrackTlasToBlasDependencies(uint32_t comm // If we had to map the device memory unmap it now if (needs_unmapping) { - VkDevice device = dev_mem_wrapper->map_device->handle; + VkDevice device = dev_mem_wrapper->device->handle; const DeviceTable* device_table = GetDeviceTable(device); device_table->UnmapMemory(device, dev_mem_wrapper->handle); } diff --git a/framework/encode/vulkan_state_writer.cpp b/framework/encode/vulkan_state_writer.cpp index f9064b3be4..2c965457f9 100644 --- a/framework/encode/vulkan_state_writer.cpp +++ b/framework/encode/vulkan_state_writer.cpp @@ -1816,7 +1816,7 @@ void VulkanStateWriter::WriteMappedMemoryState(const VulkanStateTable& state_tab if (wrapper->mapped_data != nullptr) { const VkResult result = VK_SUCCESS; - const auto device_wrapper = wrapper->map_device; + const auto device_wrapper = wrapper->device; // Map the replay memory. encoder_.EncodeHandleIdValue(device_wrapper->handle_id); From 7f72bb7bff9bf36af270d58798173695d04685b7 Mon Sep 17 00:00:00 2001 From: Ming Zheng Date: Wed, 10 Jan 2024 09:29:28 -0500 Subject: [PATCH 2/2] Address code review comments Add some changes to address code review comments. --- framework/encode/vulkan_handle_wrapper_util.h | 4 ++-- framework/encode/vulkan_handle_wrappers.h | 9 ++++++--- framework/encode/vulkan_state_tracker.cpp | 4 ++-- framework/encode/vulkan_state_writer.cpp | 2 +- 4 files changed, 11 insertions(+), 8 deletions(-) diff --git a/framework/encode/vulkan_handle_wrapper_util.h b/framework/encode/vulkan_handle_wrapper_util.h index fae3d154fc..dd7c194f8d 100644 --- a/framework/encode/vulkan_handle_wrapper_util.h +++ b/framework/encode/vulkan_handle_wrapper_util.h @@ -286,8 +286,8 @@ inline void CreateWrappedHandle(handle, get_id); - auto memory_wrapper = GetWrapper(*handle); - memory_wrapper->device = GetWrapper(device); + auto memory_wrapper = GetWrapper(*handle); + memory_wrapper->parent_device = GetWrapper(device); } template <> diff --git a/framework/encode/vulkan_handle_wrappers.h b/framework/encode/vulkan_handle_wrappers.h index 8c6542f1aa..a085b8b7fe 100644 --- a/framework/encode/vulkan_handle_wrappers.h +++ b/framework/encode/vulkan_handle_wrappers.h @@ -164,9 +164,12 @@ struct DeviceMemoryWrapper : public HandleWrapper { uint32_t memory_type_index{ std::numeric_limits::max() }; VkDeviceSize allocation_size{ 0 }; - // This is the device which was used to allocate the memory. By doc, - // if the memory can be mapped, map device must be this device. - DeviceWrapper* device{ nullptr }; + // This is the device which was used to allocate the memory. + // Spec states if the memory can be mapped, the mapping device must be this device. + // The device wrapper will be initialized when allocating the memory. Some handling + // like VulkanStateTracker::TrackTlasToBlasDependencies may use it before mapping + // the memory. + DeviceWrapper* parent_device{ nullptr }; const void* mapped_data{ nullptr }; VkDeviceSize mapped_offset{ 0 }; VkDeviceSize mapped_size{ 0 }; diff --git a/framework/encode/vulkan_state_tracker.cpp b/framework/encode/vulkan_state_tracker.cpp index fd6ff31d69..22850b7aa8 100644 --- a/framework/encode/vulkan_state_tracker.cpp +++ b/framework/encode/vulkan_state_tracker.cpp @@ -1560,7 +1560,7 @@ void VulkanStateTracker::TrackTlasToBlasDependencies(uint32_t comm { // If PageGuardManager is not used or if it couldn't find the memory id it means that // we need to map the memory. - VkDevice device = dev_mem_wrapper->device->handle; + VkDevice device = dev_mem_wrapper->parent_device->handle; const DeviceTable* device_table = GetDeviceTable(device); const VkDeviceSize map_size = sizeof(VkAccelerationStructureInstanceKHR) * blas_count; void* mapped_memory = nullptr; @@ -1595,7 +1595,7 @@ void VulkanStateTracker::TrackTlasToBlasDependencies(uint32_t comm // If we had to map the device memory unmap it now if (needs_unmapping) { - VkDevice device = dev_mem_wrapper->device->handle; + VkDevice device = dev_mem_wrapper->parent_device->handle; const DeviceTable* device_table = GetDeviceTable(device); device_table->UnmapMemory(device, dev_mem_wrapper->handle); } diff --git a/framework/encode/vulkan_state_writer.cpp b/framework/encode/vulkan_state_writer.cpp index 2c965457f9..34b7b2fd71 100644 --- a/framework/encode/vulkan_state_writer.cpp +++ b/framework/encode/vulkan_state_writer.cpp @@ -1816,7 +1816,7 @@ void VulkanStateWriter::WriteMappedMemoryState(const VulkanStateTable& state_tab if (wrapper->mapped_data != nullptr) { const VkResult result = VK_SUCCESS; - const auto device_wrapper = wrapper->device; + const auto device_wrapper = wrapper->parent_device; // Map the replay memory. encoder_.EncodeHandleIdValue(device_wrapper->handle_id);