From f0d628b2b04a13e265eeb579f7602567c53feeeb Mon Sep 17 00:00:00 2001 From: Bob Cao Date: Sun, 19 Feb 2023 16:57:14 -0800 Subject: [PATCH] [vulkan] Change command list submit error message & misc device API cleanups (#7395) ### Brief Summary The confusing "failed to submit command buffer" error message has been replaced with a device lost message (which is the typical type of failure that occurs when failing to submit cmdbuf). In addition, framebuffer caching code is removed, the reason not to do so is addressed in the comments. There are other misc changes / code quality changes included. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> --- taichi/rhi/vulkan/vulkan_device.cpp | 82 ++++++++++++----------------- taichi/rhi/vulkan/vulkan_device.h | 29 ++-------- 2 files changed, 40 insertions(+), 71 deletions(-) diff --git a/taichi/rhi/vulkan/vulkan_device.cpp b/taichi/rhi/vulkan/vulkan_device.cpp index 4594cf74942f9..eace551cf69a5 100644 --- a/taichi/rhi/vulkan/vulkan_device.cpp +++ b/taichi/rhi/vulkan/vulkan_device.cpp @@ -416,10 +416,10 @@ void VulkanPipeline::create_pipeline_layout() { } void VulkanPipeline::create_compute_pipeline(const Params ¶ms) { - char msg_buf[512]; - RHI_DEBUG_SNPRINTF(msg_buf, sizeof(msg_buf), "Compiling Vulkan pipeline %s", - params.name.data()); - RHI_LOG_DEBUG(msg_buf); + std::array msg_buf; + RHI_DEBUG_SNPRINTF(msg_buf.data(), msg_buf.size(), + "Compiling Vulkan pipeline %s", params.name.data()); + RHI_LOG_DEBUG(msg_buf.data()); pipeline_ = vkapi::create_compute_pipeline(device_, 0, shader_stages_[0], pipeline_layout_, params.cache); } @@ -554,11 +554,12 @@ void VulkanPipeline::create_graphics_pipeline( if (raster_params.blending.size()) { if (raster_params.blending.size() != color_blending.attachmentCount) { std::array buf; - snprintf(buf.data(), buf.size(), - "RasterParams::blending (size=%u) must either be zero sized " - "or match the number of fragment shader outputs (size=%u).", - uint32_t(raster_params.blending.size()), - uint32_t(color_blending.attachmentCount)); + RHI_DEBUG_SNPRINTF( + buf.data(), buf.size(), + "RasterParams::blending (size=%u) must either be zero sized " + "or match the number of fragment shader outputs (size=%u).", + uint32_t(raster_params.blending.size()), + uint32_t(color_blending.attachmentCount)); RHI_LOG_ERROR(buf.data()); RHI_ASSERT(false); } @@ -931,17 +932,19 @@ RhiResult VulkanCommandList::bind_shader_resources(ShaderResourceSet *res, VulkanResourceSet &set_template = templates.at(set_index); for (const auto &template_binding : set_template.get_bindings()) { - char msg[512]; - snprintf(msg, 512, "Template binding %d: (VkDescriptorType) %d", - template_binding.first, template_binding.second.type); - RHI_LOG_ERROR(msg); + std::array msg_buf; + RHI_DEBUG_SNPRINTF(msg_buf.data(), msg_buf.size(), + "Template binding %d: (VkDescriptorType) %d", + template_binding.first, template_binding.second.type); + RHI_LOG_ERROR(msg_buf.data()); } for (const auto &binding : set->get_bindings()) { - char msg[512]; - snprintf(msg, 512, "Binding %d: (VkDescriptorType) %d", binding.first, - binding.second.type); - RHI_LOG_ERROR(msg); + std::array msg_buf; + RHI_DEBUG_SNPRINTF(msg_buf.data(), msg_buf.size(), + "Binding %d: (VkDescriptorType) %d", binding.first, + binding.second.type); + RHI_LOG_ERROR(msg_buf.data()); } return RhiResult::invalid_usage; @@ -1612,7 +1615,6 @@ VulkanDevice::~VulkanDevice() { compute_streams_.reset(); graphics_streams_.reset(); - framebuffer_pools_.clear(); renderpass_pools_.clear(); desc_set_layouts_.clear(); desc_pool_ = nullptr; @@ -1826,10 +1828,11 @@ RhiResult VulkanDevice::map_internal(AllocationInternal &alloc_int, "wrapper)"); return RhiResult::invalid_usage; } else if (res != VK_SUCCESS) { - char msg_buf[256]; - snprintf(msg_buf, sizeof(msg_buf), - "failed to map memory for unknown reasons. VkResult = %d", res); - RHI_LOG_ERROR(msg_buf); + std::array msg_buf; + RHI_DEBUG_SNPRINTF( + msg_buf.data(), msg_buf.size(), + "failed to map memory for unknown reasons. VkResult = %d", res); + RHI_LOG_ERROR(msg_buf.data()); return RhiResult::error; } @@ -2021,23 +2024,12 @@ StreamSemaphore VulkanStream::submit( auto fence = vkapi::create_fence(buffer->device, 0); // Resource tracking, check previously submitted commands - // FIXME: Figure out why it doesn't work - /* - std::remove_if(submitted_cmdbuffers_.begin(), submitted_cmdbuffers_.end(), - [&](const TrackedCmdbuf &tracked) { - // If fence is signaled, cmdbuf has completed - VkResult res = - vkGetFenceStatus(buffer->device, tracked.fence->fence); - return res == VK_SUCCESS; - }); - */ - submitted_cmdbuffers_.push_back(TrackedCmdbuf{fence, buffer}); BAIL_ON_VK_BAD_RESULT_NO_RETURN( vkQueueSubmit(queue_, /*submitCount=*/1, &submit_info, /*fence=*/fence->fence), - "failed to submit command buffer"); + "Vulkan device might be lost (vkQueueSubmit failed)"); return std::make_shared(semaphore); } @@ -2053,9 +2045,6 @@ StreamSemaphore VulkanStream::submit_synced( void VulkanStream::command_sync() { vkQueueWaitIdle(queue_); - VkPhysicalDeviceProperties props{}; - vkGetPhysicalDeviceProperties(device_.vk_physical_device(), &props); - device_.profiler_sync(); submitted_cmdbuffers_.clear(); @@ -2130,15 +2119,13 @@ VulkanDevice::get_vk_image(const DeviceAllocation &alloc) const { vkapi::IVkFramebuffer VulkanDevice::get_framebuffer( const VulkanFramebufferDesc &desc) { - if (framebuffer_pools_.find(desc) != framebuffer_pools_.end()) { - return framebuffer_pools_.at(desc); - } - + // We won't pool framebuffer and resuse it, as doing so requires hashing the + // referenced IVkImageView objects, which might destruct unless we hold strong + // references. Thus doing so is way too ugly, and Vulkan is moving towards + // dynamic rendering anyways. vkapi::IVkFramebuffer framebuffer = vkapi::create_framebuffer( 0, desc.renderpass, desc.attachments, desc.width, desc.height, 1); - framebuffer_pools_.insert({desc, framebuffer}); - return framebuffer; } @@ -2688,10 +2675,11 @@ void VulkanSurface::create_swap_chain() { std::max(capabilities.minImageExtent.height, std::min(capabilities.maxImageExtent.height, extent.height)); { - char msg_buf[512]; - RHI_DEBUG_SNPRINTF(msg_buf, sizeof(msg_buf), "Creating suface of %u x %u", - extent.width, extent.height); - RHI_LOG_DEBUG(msg_buf); + std::array msg_buf; + RHI_DEBUG_SNPRINTF(msg_buf.data(), msg_buf.size(), + "Creating suface of %u x %u", extent.width, + extent.height); + RHI_LOG_DEBUG(msg_buf.data()); } VkImageUsageFlags usage = VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT | VK_IMAGE_USAGE_TRANSFER_SRC_BIT; diff --git a/taichi/rhi/vulkan/vulkan_device.h b/taichi/rhi/vulkan/vulkan_device.h index 167357c3bdb6e..c2fc1e689824f 100644 --- a/taichi/rhi/vulkan/vulkan_device.h +++ b/taichi/rhi/vulkan/vulkan_device.h @@ -51,14 +51,13 @@ struct VulkanRenderPassDesc { struct RenderPassDescHasher { std::size_t operator()(const VulkanRenderPassDesc &desc) const { - // TODO: Come up with a better hash - size_t hash = 0; + size_t hash = std::hash()((uint64_t(desc.depth_attachment) << 1) | + uint64_t(desc.clear_depth)); for (auto &pair : desc.color_attachments) { - hash ^= (size_t(pair.first) + pair.second); - hash = (hash << 3) || (hash >> 61); + size_t hash_pair = std::hash()((uint64_t(pair.first) << 1) | + uint64_t(pair.second)); + rhi_impl::hash_combine(hash, hash_pair); } - hash ^= (size_t(desc.depth_attachment) + desc.clear_depth); - hash = (hash << 3) || (hash >> 61); return hash; } }; @@ -75,20 +74,6 @@ struct VulkanFramebufferDesc { } }; -struct FramebufferDescHasher { - std::size_t operator()(const VulkanFramebufferDesc &desc) const { - size_t hash = 0; - for (auto &view : desc.attachments) { - hash ^= size_t(view->view); - hash = (hash << 3) || (hash >> 61); - } - hash ^= desc.width; - hash ^= desc.height; - hash ^= size_t(desc.renderpass->renderpass); - return hash; - } -}; - class VulkanResourceSet : public ShaderResourceSet { public: struct Buffer { @@ -808,10 +793,6 @@ class TI_DLL_EXPORT VulkanDevice : public GraphicsDevice { vkapi::IVkRenderPass, RenderPassDescHasher> renderpass_pools_; - unordered_map - framebuffer_pools_; // Descriptors / Layouts / Pools unordered_map