Skip to content

Commit

Permalink
[vulkan] Change command list submit error message & misc device API c…
Browse files Browse the repository at this point in the history 
…leanups (#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>
  • Loading branch information
@bobcao3 @pre-commit-ci
bobcao3 and pre-commit-ci[bot] authored Feb 20, 2023
1 parent a846330 commit f0d628b
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 71 deletions.
82 changes: 35 additions & 47 deletions taichi/rhi/vulkan/vulkan_device.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -416,10 +416,10 @@ void VulkanPipeline::create_pipeline_layout() {
}

void VulkanPipeline::create_compute_pipeline(const Params &params) {
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<char, 512> 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);
}
Expand Down Expand Up @@ -554,11 +554,12 @@ void VulkanPipeline::create_graphics_pipeline(
if (raster_params.blending.size()) {
if (raster_params.blending.size() != color_blending.attachmentCount) {
std::array<char, 256> 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);
}
Expand Down Expand Up @@ -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<char, 256> 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<char, 256> 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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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<char, 256> 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;
}

Expand Down Expand Up @@ -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<VulkanStreamSemaphoreObject>(semaphore);
}
Expand All @@ -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();
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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<char, 512> 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;
Expand Down
29 changes: 5 additions & 24 deletions taichi/rhi/vulkan/vulkan_device.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>()((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>()((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;
}
};
Expand All @@ -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 {
Expand Down Expand Up @@ -808,10 +793,6 @@ class TI_DLL_EXPORT VulkanDevice : public GraphicsDevice {
vkapi::IVkRenderPass,
RenderPassDescHasher>
renderpass_pools_;
unordered_map<VulkanFramebufferDesc,
vkapi::IVkFramebuffer,
FramebufferDescHasher>
framebuffer_pools_;

// Descriptors / Layouts / Pools
unordered_map<VulkanResourceSet,
Expand Down

0 comments on commit f0d628b

Please sign in to comment.