From 301c1dea2bd3e44d0335aa436f78234fe8749631 Mon Sep 17 00:00:00 2001 From: Panos Karabelas Date: Mon, 5 Feb 2024 15:23:28 +0000 Subject: [PATCH] [Vulkan] Fixed the last major validation error, it would manifest as flickering because sometimes an image would be acquired while it was still being presented --- runtime/RHI/RHI_SwapChain.h | 21 +-- runtime/RHI/Vulkan/Vulkan_CommandList.cpp | 1 - runtime/RHI/Vulkan/Vulkan_SwapChain.cpp | 152 +++++++++++++++------- runtime/Rendering/Renderer.cpp | 2 +- 4 files changed, 120 insertions(+), 56 deletions(-) diff --git a/runtime/RHI/RHI_SwapChain.h b/runtime/RHI/RHI_SwapChain.h index 807842173..655b3219d 100644 --- a/runtime/RHI/RHI_SwapChain.h +++ b/runtime/RHI/RHI_SwapChain.h @@ -49,21 +49,21 @@ namespace Spartan ); ~RHI_SwapChain(); - // Size + // size void Resize(uint32_t width, uint32_t height, const bool force = false); void ResizeToWindowSize(); - // HDR + // hdr void SetHdr(const bool enabled); bool IsHdr() const { return m_format == format_hdr; } - // VSync + // vsync void SetVsync(const bool enabled); bool GetVsync(); void Present(); - // Properties + // properties uint32_t GetWidth() const { return m_width; } uint32_t GetHeight() const { return m_height; } uint32_t GetBufferCount() const { return m_buffer_count; } @@ -71,7 +71,7 @@ namespace Spartan void* GetRhiRt() const { return m_rhi_rt[m_image_index]; } void* GetRhiRtv() const { return m_rhi_rtv[m_image_index]; } - // Layout + // layout RHI_Image_Layout GetLayout() const; void SetLayout(const RHI_Image_Layout& layout, RHI_CommandList* cmd_list); @@ -80,7 +80,7 @@ namespace Spartan void Destroy(); void AcquireNextImage(); - // Main + // main bool m_windowed = false; uint32_t m_buffer_count = 0; uint32_t m_width = 0; @@ -88,7 +88,7 @@ namespace Spartan RHI_Format m_format = RHI_Format::Max; RHI_Present_Mode m_present_mode = RHI_Present_Mode::Immediate; - // Misc + // misc uint32_t m_sync_index = std::numeric_limits::max(); uint32_t m_image_index = std::numeric_limits::max(); uint32_t m_image_index_previous = m_image_index; @@ -97,7 +97,12 @@ namespace Spartan std::array, max_buffer_count> m_acquire_semaphore; std::vector m_wait_semaphores; - // RHI + // wait for present capability + std::shared_ptr m_present_fence; + void* m_present_cmd_list = nullptr; + void* m_present_cmd_pool = nullptr; + + // rhi void* m_rhi_swapchain = nullptr; void* m_rhi_surface = nullptr; std::array m_rhi_rt = { nullptr, nullptr, nullptr}; diff --git a/runtime/RHI/Vulkan/Vulkan_CommandList.cpp b/runtime/RHI/Vulkan/Vulkan_CommandList.cpp index 949efceb5..ae47dfc72 100644 --- a/runtime/RHI/Vulkan/Vulkan_CommandList.cpp +++ b/runtime/RHI/Vulkan/Vulkan_CommandList.cpp @@ -464,7 +464,6 @@ namespace Spartan // begin command buffer VkCommandBufferBeginInfo begin_info = {}; begin_info.sType = VK_STRUCTURE_TYPE_COMMAND_BUFFER_BEGIN_INFO; - begin_info.flags = VK_COMMAND_BUFFER_USAGE_ONE_TIME_SUBMIT_BIT; SP_ASSERT_MSG(vkBeginCommandBuffer(static_cast(m_rhi_resource), &begin_info) == VK_SUCCESS, "Failed to begin command buffer"); // update states diff --git a/runtime/RHI/Vulkan/Vulkan_SwapChain.cpp b/runtime/RHI/Vulkan/Vulkan_SwapChain.cpp index c232a4022..dc46f1924 100644 --- a/runtime/RHI/Vulkan/Vulkan_SwapChain.cpp +++ b/runtime/RHI/Vulkan/Vulkan_SwapChain.cpp @@ -25,6 +25,7 @@ CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. #include "../RHI_Device.h" #include "../RHI_SwapChain.h" #include "../RHI_Implementation.h" +#include "../RHI_Fence.h" #include "../RHI_Semaphore.h" #include "../RHI_CommandPool.h" #include "../Display/Display.h" @@ -41,7 +42,7 @@ using namespace Spartan::Math; namespace Spartan { namespace - { + { VkColorSpaceKHR get_color_space(const RHI_Format format) { // VK_COLOR_SPACE_HDR10_ST2084_EXT represents the HDR10 color space with the ST.2084 (PQ)electro - optical transfer function. @@ -53,8 +54,8 @@ namespace Spartan // When displaying an image in sRGB, the values must be converted to linear space before they are displayed. VkColorSpaceKHR color_space = VK_COLOR_SPACE_SRGB_NONLINEAR_KHR; // SDR - color_space = format == RHI_Format::R16G16B16A16_Float ? VK_COLOR_SPACE_EXTENDED_SRGB_LINEAR_EXT : color_space; // HDR - color_space = format == RHI_Format::R10G10B10A2_Unorm ? VK_COLOR_SPACE_HDR10_ST2084_EXT : color_space; // HDR + color_space = format == RHI_Format::R16G16B16A16_Float ? VK_COLOR_SPACE_EXTENDED_SRGB_LINEAR_EXT : color_space; // HDR + color_space = format == RHI_Format::R10G10B10A2_Unorm ? VK_COLOR_SPACE_HDR10_ST2084_EXT : color_space; // HDR return color_space; } @@ -130,7 +131,7 @@ namespace Spartan for (const VkSurfaceFormatKHR& supported_format : supported_formats) { - bool support_format = supported_format.format == vulkan_format[rhi_format_to_index(*format)]; + bool support_format = supported_format.format == vulkan_format[rhi_format_to_index(*format)]; bool support_color_space = supported_format.colorSpace == color_space; if (support_format && support_color_space) @@ -179,14 +180,14 @@ namespace Spartan ) { SP_ASSERT_MSG(RHI_Device::IsValidResolution(width, height), "Invalid resolution"); - SP_ASSERT_MSG(buffer_count >= 2, "Buffer can't be less than 2"); + SP_ASSERT_MSG(buffer_count >= 2, "Buffer can't be less than 2"); - m_format = format_sdr; // for now, we use SDR by default as HDR doesn't look rigth - Display::GetHdr() ? format_hdr : format_sdr; + m_format = format_sdr; // for now, we use SDR by default as HDR doesn't look rigth - Display::GetHdr() ? format_hdr : format_sdr; m_buffer_count = buffer_count; - m_width = width; - m_height = height; - m_sdl_window = sdl_window; - m_object_name = name; + m_width = width; + m_height = height; + m_sdl_window = sdl_window; + m_object_name = name; m_present_mode = present_mode; Create(); @@ -230,42 +231,42 @@ namespace Spartan SP_ASSERT_MSG(is_format_and_color_space_supported(surface, &m_format, color_space), "The surface doesn't support the requested format"); // clamp size between the supported min and max - m_width = Math::Helper::Clamp(m_width, capabilities.minImageExtent.width, capabilities.maxImageExtent.width); + m_width = Math::Helper::Clamp(m_width, capabilities.minImageExtent.width, capabilities.maxImageExtent.width); m_height = Math::Helper::Clamp(m_height, capabilities.minImageExtent.height, capabilities.maxImageExtent.height); // swap chain VkSwapchainKHR swap_chain; { - VkSwapchainCreateInfoKHR create_info = {}; - create_info.sType = VK_STRUCTURE_TYPE_SWAPCHAIN_CREATE_INFO_KHR; - create_info.surface = surface; - create_info.minImageCount = m_buffer_count; - create_info.imageFormat = vulkan_format[rhi_format_to_index(m_format)]; - create_info.imageColorSpace = color_space; - create_info.imageExtent = { m_width, m_height }; - create_info.imageArrayLayers = 1; - create_info.imageUsage = VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT; // fer rendering on it - create_info.imageUsage |= VK_IMAGE_USAGE_TRANSFER_DST_BIT; // for blitting to it + VkSwapchainCreateInfoKHR create_info = {}; + create_info.sType = VK_STRUCTURE_TYPE_SWAPCHAIN_CREATE_INFO_KHR; + create_info.surface = surface; + create_info.minImageCount = m_buffer_count; + create_info.imageFormat = vulkan_format[rhi_format_to_index(m_format)]; + create_info.imageColorSpace = color_space; + create_info.imageExtent = { m_width, m_height }; + create_info.imageArrayLayers = 1; + create_info.imageUsage = VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT; // fer rendering on it + create_info.imageUsage |= VK_IMAGE_USAGE_TRANSFER_DST_BIT; // for blitting to it uint32_t queueFamilyIndices[] = { RHI_Device::QueueGetIndex(RHI_Queue_Type::Compute), RHI_Device::QueueGetIndex(RHI_Queue_Type::Graphics) }; if (queueFamilyIndices[0] != queueFamilyIndices[1]) { - create_info.imageSharingMode = VK_SHARING_MODE_CONCURRENT; + create_info.imageSharingMode = VK_SHARING_MODE_CONCURRENT; create_info.queueFamilyIndexCount = 2; - create_info.pQueueFamilyIndices = queueFamilyIndices; + create_info.pQueueFamilyIndices = queueFamilyIndices; } else { - create_info.imageSharingMode = VK_SHARING_MODE_EXCLUSIVE; + create_info.imageSharingMode = VK_SHARING_MODE_EXCLUSIVE; create_info.queueFamilyIndexCount = 0; - create_info.pQueueFamilyIndices = nullptr; + create_info.pQueueFamilyIndices = nullptr; } - create_info.preTransform = capabilities.currentTransform; + create_info.preTransform = capabilities.currentTransform; create_info.compositeAlpha = get_supported_composite_alpha_format(surface); - create_info.presentMode = get_present_mode(surface, m_present_mode); - create_info.clipped = VK_TRUE; - create_info.oldSwapchain = nullptr; + create_info.presentMode = get_present_mode(surface, m_present_mode); + create_info.clipped = VK_TRUE; + create_info.oldSwapchain = nullptr; SP_VK_ASSERT_MSG(vkCreateSwapchainKHR(RHI_Context::device, &create_info, nullptr, &swap_chain), "Failed to create swapchain"); @@ -306,26 +307,26 @@ namespace Spartan { RHI_Device::SetResourceName(m_rhi_rt[i], RHI_Resource_Type::Texture, string(string("swapchain_image_") + to_string(i))); - VkImageViewCreateInfo create_info = {}; - create_info.sType = VK_STRUCTURE_TYPE_IMAGE_VIEW_CREATE_INFO; - create_info.image = static_cast(m_rhi_rt[i]); - create_info.viewType = VK_IMAGE_VIEW_TYPE_2D; - create_info.format = vulkan_format[rhi_format_to_index(m_format)]; - create_info.subresourceRange.aspectMask = VK_IMAGE_ASPECT_COLOR_BIT; - create_info.subresourceRange.baseMipLevel = 0; - create_info.subresourceRange.levelCount = 1; + VkImageViewCreateInfo create_info = {}; + create_info.sType = VK_STRUCTURE_TYPE_IMAGE_VIEW_CREATE_INFO; + create_info.image = static_cast(m_rhi_rt[i]); + create_info.viewType = VK_IMAGE_VIEW_TYPE_2D; + create_info.format = vulkan_format[rhi_format_to_index(m_format)]; + create_info.subresourceRange.aspectMask = VK_IMAGE_ASPECT_COLOR_BIT; + create_info.subresourceRange.baseMipLevel = 0; + create_info.subresourceRange.levelCount = 1; create_info.subresourceRange.baseArrayLayer = 0; - create_info.subresourceRange.layerCount = 1; - create_info.components.r = VK_COMPONENT_SWIZZLE_IDENTITY; - create_info.components.g = VK_COMPONENT_SWIZZLE_IDENTITY; - create_info.components.b = VK_COMPONENT_SWIZZLE_IDENTITY; - create_info.components.a = VK_COMPONENT_SWIZZLE_IDENTITY; + create_info.subresourceRange.layerCount = 1; + create_info.components.r = VK_COMPONENT_SWIZZLE_IDENTITY; + create_info.components.g = VK_COMPONENT_SWIZZLE_IDENTITY; + create_info.components.b = VK_COMPONENT_SWIZZLE_IDENTITY; + create_info.components.a = VK_COMPONENT_SWIZZLE_IDENTITY; SP_ASSERT_MSG(vkCreateImageView(RHI_Context::device, &create_info, nullptr, reinterpret_cast(&m_rhi_rtv[i])) == VK_SUCCESS, "Failed to create swapchain RTV"); } } - m_rhi_surface = static_cast(surface); + m_rhi_surface = static_cast(surface); m_rhi_swapchain = static_cast(swap_chain); // semaphores @@ -334,6 +335,35 @@ namespace Spartan string name = (string("swapchain_image_acquired_") + to_string(i)); m_acquire_semaphore[i] = make_shared(false, name.c_str()); } + + // present wait capability + { + // fence + m_present_fence = make_shared("swapchain_present"); + + // cmd pool + VkCommandPoolCreateInfo cmd_pool_info = {}; + cmd_pool_info.sType = VK_STRUCTURE_TYPE_COMMAND_POOL_CREATE_INFO; + cmd_pool_info.queueFamilyIndex = RHI_Device::QueueGetIndex(RHI_Queue_Type::Graphics); + cmd_pool_info.flags = VK_COMMAND_POOL_CREATE_TRANSIENT_BIT | VK_COMMAND_POOL_CREATE_RESET_COMMAND_BUFFER_BIT; + + VkCommandPool cmd_pool = nullptr; + SP_VK_ASSERT_MSG(vkCreateCommandPool(RHI_Context::device, &cmd_pool_info, nullptr, &cmd_pool), "Failed to create command pool"); + RHI_Device::SetResourceName(cmd_pool, RHI_Resource_Type::CommandPool, "swapchain_present"); + m_present_cmd_pool = cmd_pool; + + // cmd list + VkCommandBufferAllocateInfo cmd_list_info = {}; + cmd_list_info.sType = VK_STRUCTURE_TYPE_COMMAND_BUFFER_ALLOCATE_INFO; + cmd_list_info.commandPool = cmd_pool; + cmd_list_info.level = VK_COMMAND_BUFFER_LEVEL_PRIMARY; + cmd_list_info.commandBufferCount = 1; + + VkCommandBuffer present_cmd_buffer; + SP_VK_ASSERT_MSG(vkAllocateCommandBuffers(RHI_Context::device, &cmd_list_info, &present_cmd_buffer), "Failed to allocate command buffer"); + RHI_Device::SetResourceName(present_cmd_buffer, RHI_Resource_Type::CommandList, "swapchain_present"); + m_present_cmd_list = present_cmd_buffer; + } } void RHI_SwapChain::Destroy() @@ -351,6 +381,13 @@ namespace Spartan RHI_Device::QueueWaitAll(); + // present wait capability + { + m_present_fence = nullptr; + vkFreeCommandBuffers(RHI_Context::device, static_cast(m_present_cmd_pool), 1, reinterpret_cast(&m_present_cmd_list)); + vkDestroyCommandPool(RHI_Context::device, static_cast(m_present_cmd_pool), nullptr); + } + vkDestroySwapchainKHR(RHI_Context::device, static_cast(m_rhi_swapchain), nullptr); m_rhi_swapchain = nullptr; @@ -391,14 +428,37 @@ namespace Spartan void RHI_SwapChain::AcquireNextImage() { - // get signal semaphore m_sync_index = (m_sync_index + 1) % m_buffer_count; + + // note: + // cpu tracking of semaphore states helps identify logic errors, but doesn't reflect real-time gpu execution + // a semaphore might be marked as 'submitted' cpu-side, yet pending gpu processing + // to ensure synchronization, we submit an empty command buffer with a fence, creating a reliable gpu wait mechanism + { + m_present_fence->Wait(); + m_present_fence->Reset(); + + // create no actual work + VkCommandBufferBeginInfo begin_info = {}; + begin_info.sType = VK_STRUCTURE_TYPE_COMMAND_BUFFER_BEGIN_INFO; + begin_info.flags = VK_COMMAND_BUFFER_USAGE_ONE_TIME_SUBMIT_BIT; + vkBeginCommandBuffer(static_cast(m_present_cmd_list), &begin_info); + vkEndCommandBuffer(static_cast(m_present_cmd_list)); + + // submit the fence + VkSubmitInfo submitInfo = {}; + submitInfo.sType = VK_STRUCTURE_TYPE_SUBMIT_INFO; + submitInfo.commandBufferCount = 1; + submitInfo.pCommandBuffers = reinterpret_cast(&m_present_cmd_list); + RHI_Device::QueueSubmit(RHI_Queue_Type::Graphics, 0, m_present_cmd_list, nullptr, nullptr, m_present_fence.get()); + } + + // get signal semaphore RHI_Semaphore* signal_semaphore = m_acquire_semaphore[m_sync_index].get(); SP_ASSERT_MSG(signal_semaphore->GetStateCpu() != RHI_Sync_State::Submitted, "The semaphore is already signaled"); - m_image_index_previous = m_image_index; - // acquire next image + m_image_index_previous = m_image_index; SP_VK_ASSERT_MSG(vkAcquireNextImageKHR( RHI_Context::device, // device static_cast(m_rhi_swapchain), // swapchain diff --git a/runtime/Rendering/Renderer.cpp b/runtime/Rendering/Renderer.cpp index 54d1a5482..aac0a27d9 100644 --- a/runtime/Rendering/Renderer.cpp +++ b/runtime/Rendering/Renderer.cpp @@ -307,7 +307,7 @@ namespace Spartan SetOption(Renderer_Option::Bloom, 0.03f); // non-zero values activate it and define the blend factor SetOption(Renderer_Option::MotionBlur, 1.0f); SetOption(Renderer_Option::ScreenSpaceGlobalIllumination, 1.0f); - SetOption(Renderer_Option::ScreenSpaceShadows, static_cast(Renderer_ScreenspaceShadow::Bend)); + SetOption(Renderer_Option::ScreenSpaceShadows, static_cast(Renderer_ScreenspaceShadow::Bend)); SetOption(Renderer_Option::ScreenSpaceReflections, 1.0f); SetOption(Renderer_Option::Anisotropy, 16.0f); SetOption(Renderer_Option::ShadowResolution, 2048.0f);