From 913eed37e330081839669ef0e7db251e27722dd2 Mon Sep 17 00:00:00 2001 From: Dominik Witczak Date: Mon, 20 Aug 2018 13:28:24 +0200 Subject: [PATCH] Fix an issue where gfx pipeline create info structs would be malformed if not all attachments had image views bound. Fix an issue where memory ranges would not be aligned to required boundaries at invalidation / flush time --- include/misc/buffer_create_info.h | 9 +++++ include/misc/render_pass_create_info.h | 10 +++++ include/misc/types_struct.h | 2 + include/wrappers/command_buffer.h | 2 +- src/misc/render_pass_create_info.cpp | 44 ++++++++++++++++++++-- src/misc/types_struct.cpp | 25 ++++++++++++ src/wrappers/graphics_pipeline_manager.cpp | 40 ++++++++++++-------- src/wrappers/memory_block.cpp | 30 ++++++++++++--- src/wrappers/pipeline_layout.cpp | 2 +- 9 files changed, 138 insertions(+), 26 deletions(-) diff --git a/include/misc/buffer_create_info.h b/include/misc/buffer_create_info.h index 0801f803..19cde495 100644 --- a/include/misc/buffer_create_info.h +++ b/include/misc/buffer_create_info.h @@ -212,6 +212,15 @@ namespace Anvil return m_usage_flags; } + /** Use to specify contents which should be uploaded to a buffer at memory block assignment time. + * + * Note that this setting will be ignored for partially-resident buffers. + * + * The specified pointer must remain valid until Buffer::set_nonsparse_memory() call time. + * + * @param in_client_data_ptr Pointer to data storage holding contents the created buffer should be filled with. + * Must remain valid until memory block assignment time. + */ void set_client_data(const void* const in_client_data_ptr) { m_client_data_ptr = in_client_data_ptr; diff --git a/include/misc/render_pass_create_info.h b/include/misc/render_pass_create_info.h index 76bc5331..ae91e27a 100644 --- a/include/misc/render_pass_create_info.h +++ b/include/misc/render_pass_create_info.h @@ -377,6 +377,16 @@ namespace Anvil return m_device_ptr; } + /* Returns the largest location assigned to color attachments for the specified subpass. + * + * If no color attachments have been defined for the queried subpass, UINT32_MAX is returned. + * + * @param in_subpass_id ID of the subpass to use for the query. + * + * @return As per description. + **/ + uint32_t get_max_color_location_used_by_subpass(const SubPassID& in_subpass_id) const; + /** Returns the number of added attachments */ uint32_t get_n_attachments() const { diff --git a/include/misc/types_struct.h b/include/misc/types_struct.h index 42900f2f..6da2fa14 100644 --- a/include/misc/types_struct.h +++ b/include/misc/types_struct.h @@ -731,6 +731,8 @@ namespace Anvil return &image_barrier_vk; } + bool operator==(const ImageBarrier& in_barrier) const; + private: ImageBarrier& operator=(const ImageBarrier&); } ImageBarrier; diff --git a/include/wrappers/command_buffer.h b/include/wrappers/command_buffer.h index 9d540765..5ea2cfaa 100644 --- a/include/wrappers/command_buffer.h +++ b/include/wrappers/command_buffer.h @@ -481,7 +481,7 @@ namespace Anvil * Calling this function for a command buffer which has not been put into a recording mode * (by issuing a start_recording() call earlier) will result in an assertion failure. * - * It is also illegal to call this function when not recording renderpass commands. Doing so + * It is also illegal to call this function when recording renderpass commands. Doing so * will also result in an assertion failure. * * Any Vulkan object wrapper instances passed to this function are going to be retained, diff --git a/src/misc/render_pass_create_info.cpp b/src/misc/render_pass_create_info.cpp index 975e63a8..ea0bb8e6 100644 --- a/src/misc/render_pass_create_info.cpp +++ b/src/misc/render_pass_create_info.cpp @@ -25,6 +25,9 @@ #include #include +#if defined(max) + #undef max +#endif Anvil::RenderPassCreateInfo::RenderPassCreateInfo(const Anvil::BaseDevice* in_device_ptr) :m_device_ptr (in_device_ptr), @@ -715,6 +718,31 @@ bool Anvil::RenderPassCreateInfo::get_depth_stencil_attachment_properties(Render return result; } +/* Please see header for specification */ +uint32_t Anvil::RenderPassCreateInfo::get_max_color_location_used_by_subpass(const SubPassID& in_subpass_id) const +{ + uint32_t result = UINT32_MAX; + auto subpass_iterator = (m_subpasses.size() > in_subpass_id) ? m_subpasses.begin() + in_subpass_id : m_subpasses.end(); + + if (subpass_iterator == m_subpasses.end() ) + { + anvil_assert(subpass_iterator != m_subpasses.end() ); + + goto end; + } + + result = 0; + + for (const auto& current_color_attachment_data : (*subpass_iterator)->color_attachments_map) + { + result = std::max(result, + current_color_attachment_data.first); + } + +end: + return result; +} + /* Please see header for specification */ bool Anvil::RenderPassCreateInfo::get_subpass_n_attachments(SubPassID in_subpass_id, AttachmentType in_attachment_type, @@ -809,12 +837,22 @@ bool Anvil::RenderPassCreateInfo::get_subpass_attachment_properties(SubPassID : (in_attachment_type == ATTACHMENT_TYPE_INPUT) ? &subpass_ptr->input_attachments_map : &subpass_ptr->resolved_attachments_map; - auto iterator = subpass_attachments_ptr->find(in_n_subpass_attachment); + auto iterator = subpass_attachments_ptr->begin(); - if (iterator == subpass_attachments_ptr->end() ) + for (uint32_t n_key = 0; + n_key < in_n_subpass_attachment; + ++n_key) { - anvil_assert_fail(); + if (iterator == subpass_attachments_ptr->end() ) + { + goto end; + } + + iterator ++; + } + if (iterator == subpass_attachments_ptr->end() ) + { goto end; } diff --git a/src/misc/types_struct.cpp b/src/misc/types_struct.cpp index 6e6066b2..f10806a0 100644 --- a/src/misc/types_struct.cpp +++ b/src/misc/types_struct.cpp @@ -745,6 +745,7 @@ Anvil::ImageFormatProperties::ImageFormatProperties(const VkImageFormatPropertie /** Please see header for specification */ Anvil::ImageBarrier::ImageBarrier(const ImageBarrier& in) { + by_region = in.by_region; dst_access_mask = in.dst_access_mask; dst_queue_family_index = in.dst_queue_family_index; image = in.image; @@ -805,6 +806,30 @@ Anvil::ImageBarrier::~ImageBarrier() /* Stub */ } +bool Anvil::ImageBarrier::operator==(const ImageBarrier& in_barrier) const +{ + bool result = true; + + result &= (dst_access_mask == in_barrier.dst_access_mask); + result &= (src_access_mask == in_barrier.src_access_mask); + + result &= (by_region == in_barrier.by_region); + result &= (dst_queue_family_index == in_barrier.dst_queue_family_index); + result &= (image == in_barrier.image); + result &= (image_ptr == in_barrier.image_ptr); + result &= (new_layout == in_barrier.new_layout); + result &= (old_layout == in_barrier.old_layout); + result &= (src_queue_family_index == in_barrier.src_queue_family_index); + + result &= (subresource_range.aspectMask == in_barrier.subresource_range.aspectMask); + result &= (subresource_range.baseArrayLayer == in_barrier.subresource_range.baseArrayLayer); + result &= (subresource_range.baseMipLevel == in_barrier.subresource_range.baseMipLevel); + result &= (subresource_range.layerCount == in_barrier.subresource_range.layerCount); + result &= (subresource_range.levelCount == in_barrier.subresource_range.levelCount); + + return result; +} + Anvil::KHR16BitStorageFeatures::KHR16BitStorageFeatures() { is_input_output_storage_supported = false; diff --git a/src/wrappers/graphics_pipeline_manager.cpp b/src/wrappers/graphics_pipeline_manager.cpp index 7371c8fe..9626a4f3 100644 --- a/src/wrappers/graphics_pipeline_manager.cpp +++ b/src/wrappers/graphics_pipeline_manager.cpp @@ -240,15 +240,18 @@ bool Anvil::GraphicsPipelineManager::bake() VkPipelineColorBlendStateCreateInfo color_blend_state_create_info; VkLogicOp logic_op; bool logic_op_enabled = false; + uint32_t max_location_index = UINT32_MAX; const uint32_t start_offset = static_cast(color_blend_attachment_states_vk_cache.size() ); + max_location_index = current_pipeline_renderpass_ptr->get_render_pass_create_info()->get_max_color_location_used_by_subpass(current_pipeline_subpass_id); + current_pipeline_create_info_ptr->get_blending_properties(&blend_constant_ptr, nullptr); /* out_opt_n_blend_attachments_ptr */ current_pipeline_create_info_ptr->get_logic_op_state(&logic_op_enabled, &logic_op); - color_blend_state_create_info.attachmentCount = subpass_n_color_attachments; + color_blend_state_create_info.attachmentCount = max_location_index + 1; color_blend_state_create_info.flags = 0; color_blend_state_create_info.logicOp = logic_op; color_blend_state_create_info.logicOpEnable = (logic_op_enabled) ? VK_TRUE : VK_FALSE; @@ -260,24 +263,33 @@ bool Anvil::GraphicsPipelineManager::bake() blend_constant_ptr, sizeof(color_blend_state_create_info.blendConstants) ); + anvil_assert(subpass_n_color_attachments <= current_pipeline_renderpass_ptr->get_render_pass_create_info()->get_n_attachments() ); + for (uint32_t n_subpass_color_attachment = 0; - n_subpass_color_attachment < subpass_n_color_attachments; + n_subpass_color_attachment <= max_location_index; ++n_subpass_color_attachment) { color_blend_attachment_states_vk_cache.push_back(VkPipelineColorBlendAttachmentState() ); VkPipelineColorBlendAttachmentState* blend_attachment_state_ptr = &color_blend_attachment_states_vk_cache.back(); + VkImageLayout dummy = VK_IMAGE_LAYOUT_MAX_ENUM; bool is_blending_enabled_for_attachment = false; - - if (!current_pipeline_create_info_ptr->get_color_blend_attachment_properties(n_subpass_color_attachment, - &is_blending_enabled_for_attachment, - &blend_attachment_state_ptr->colorBlendOp, - &blend_attachment_state_ptr->alphaBlendOp, - &blend_attachment_state_ptr->srcColorBlendFactor, - &blend_attachment_state_ptr->dstColorBlendFactor, - &blend_attachment_state_ptr->srcAlphaBlendFactor, - &blend_attachment_state_ptr->dstAlphaBlendFactor, - &blend_attachment_state_ptr->colorWriteMask) ) + Anvil::RenderPassAttachmentID rp_attachment_id = UINT32_MAX; + + if (!current_pipeline_renderpass_ptr->get_render_pass_create_info()->get_subpass_attachment_properties(current_pipeline_subpass_id, + Anvil::ATTACHMENT_TYPE_COLOR, + n_subpass_color_attachment, + &rp_attachment_id, + &dummy) || /* out_layout_ptr */ + !current_pipeline_create_info_ptr->get_color_blend_attachment_properties (rp_attachment_id, + &is_blending_enabled_for_attachment, + &blend_attachment_state_ptr->colorBlendOp, + &blend_attachment_state_ptr->alphaBlendOp, + &blend_attachment_state_ptr->srcColorBlendFactor, + &blend_attachment_state_ptr->dstColorBlendFactor, + &blend_attachment_state_ptr->srcAlphaBlendFactor, + &blend_attachment_state_ptr->dstAlphaBlendFactor, + &blend_attachment_state_ptr->colorWriteMask) ) { /* The user has not defined blending properties for current color attachment. Use default state values .. */ blend_attachment_state_ptr->blendEnable = VK_FALSE; @@ -294,9 +306,7 @@ bool Anvil::GraphicsPipelineManager::bake() } else { - anvil_assert(is_blending_enabled_for_attachment); - - blend_attachment_state_ptr->blendEnable = VK_TRUE; + blend_attachment_state_ptr->blendEnable = (is_blending_enabled_for_attachment) ? VK_TRUE : VK_FALSE; } } diff --git a/src/wrappers/memory_block.cpp b/src/wrappers/memory_block.cpp index eb46c00d..7050bd5f 100644 --- a/src/wrappers/memory_block.cpp +++ b/src/wrappers/memory_block.cpp @@ -656,17 +656,26 @@ bool Anvil::MemoryBlock::read(VkDeviceSize in_start_offset, if ((m_create_info_ptr->get_memory_features() & Anvil::MEMORY_FEATURE_FLAG_HOST_COHERENT) == 0) { VkMappedMemoryRange mapped_memory_range; - VkResult result_vk (VK_ERROR_INITIALIZATION_FAILED); + const auto mem_block_size = m_create_info_ptr->get_size(); + const auto non_coherent_atom_size = m_create_info_ptr->get_device()->get_physical_device_properties().core_vk1_0_properties_ptr->limits.non_coherent_atom_size; + VkResult result_vk = VK_ERROR_INITIALIZATION_FAILED; anvil_assert (m_start_offset == 0); ANVIL_REDUNDANT_VARIABLE(result_vk); mapped_memory_range.memory = m_memory; - mapped_memory_range.offset = in_start_offset; + mapped_memory_range.offset = Anvil::Utils::round_down(in_start_offset, + non_coherent_atom_size); mapped_memory_range.pNext = nullptr; - mapped_memory_range.size = in_size; + mapped_memory_range.size = Anvil::Utils::round_up(in_size, + non_coherent_atom_size); mapped_memory_range.sType = VK_STRUCTURE_TYPE_MAPPED_MEMORY_RANGE; + if (mapped_memory_range.size + mapped_memory_range.offset > mem_block_size) + { + mapped_memory_range.size = mem_block_size - mapped_memory_range.offset; + } + result_vk = vkInvalidateMappedMemoryRanges(m_create_info_ptr->get_device()->get_device_vk(), 1, /* memRangeCount */ &mapped_memory_range); @@ -746,16 +755,25 @@ bool Anvil::MemoryBlock::write(VkDeviceSize in_start_offset, if ((m_create_info_ptr->get_memory_features() & Anvil::MEMORY_FEATURE_FLAG_HOST_COHERENT) == 0) { VkMappedMemoryRange mapped_memory_range; - VkResult result_vk (VK_ERROR_INITIALIZATION_FAILED); + const auto mem_block_size = m_create_info_ptr->get_size(); + const auto non_coherent_atom_size = m_create_info_ptr->get_device()->get_physical_device_properties().core_vk1_0_properties_ptr->limits.non_coherent_atom_size; + auto result_vk = VkResult(VK_ERROR_INITIALIZATION_FAILED); ANVIL_REDUNDANT_VARIABLE(result_vk); mapped_memory_range.memory = m_memory; - mapped_memory_range.offset = in_start_offset; + mapped_memory_range.offset = Anvil::Utils::round_down(in_start_offset, + non_coherent_atom_size); mapped_memory_range.pNext = nullptr; - mapped_memory_range.size = in_size; + mapped_memory_range.size = Anvil::Utils::round_up(in_size, + non_coherent_atom_size); mapped_memory_range.sType = VK_STRUCTURE_TYPE_MAPPED_MEMORY_RANGE; + if (mapped_memory_range.size + mapped_memory_range.offset > mem_block_size) + { + mapped_memory_range.size = mem_block_size - mapped_memory_range.offset; + } + result_vk = vkFlushMappedMemoryRanges(m_create_info_ptr->get_device()->get_device_vk(), 1, /* memRangeCount */ &mapped_memory_range); diff --git a/src/wrappers/pipeline_layout.cpp b/src/wrappers/pipeline_layout.cpp index 10f47052..a73bb457 100644 --- a/src/wrappers/pipeline_layout.cpp +++ b/src/wrappers/pipeline_layout.cpp @@ -74,7 +74,7 @@ bool Anvil::PipelineLayout::bake(const std::vectorget_dummy_descriptor_set_layout()->get_layout(); - ds_layouts_vk.resize(in_ds_create_info_items_ptr->size() + 1, + ds_layouts_vk.resize(in_ds_create_info_items_ptr->size(), dummy_ds_layout); if (in_ds_create_info_items_ptr != nullptr &&