From 9e43ea53634e4729bd814c1ee67d6bad14aabf9f Mon Sep 17 00:00:00 2001 From: Tobin Ehlis Date: Wed, 2 Aug 2017 16:09:28 -0600 Subject: [PATCH 01/28] layers:Limited POC of synch object tracking This is the initial version of a single-case update to handle synchronization tracking for the cmd sequence from GH892. As the code stands it's only primarily useful for this single cmd sequence and isn't catching any variety of memory conflicts. The intention is that if the general idea is sound then I will build on this initial code to expand/generalize the algorithms used here for all memory conflict cases. --- layers/core_validation.cpp | 156 +++++++++++++++++++++++++-- layers/core_validation_error_enums.h | 1 + layers/core_validation_types.h | 45 ++++++++ 3 files changed, 194 insertions(+), 8 deletions(-) diff --git a/layers/core_validation.cpp b/layers/core_validation.cpp index 9a6dd942e7..08d8c06796 100644 --- a/layers/core_validation.cpp +++ b/layers/core_validation.cpp @@ -1725,6 +1725,8 @@ static void resetCB(layer_data *dev_data, const VkCommandBuffer cb) { pCB->activeSubpassContents = VK_SUBPASS_CONTENTS_INLINE; pCB->activeSubpass = 0; pCB->broken_bindings.clear(); + pCB->commands.clear(); + pCB->memory_accesses.clear(); pCB->waitedEvents.clear(); pCB->events.clear(); pCB->writeEventsBeforeWait.clear(); @@ -5646,6 +5648,47 @@ static void MarkStoreImagesAndBuffersAsWritten(layer_data *dev_data, GLOBAL_CB_N } } +// The first arg is an existing memory access that hasn't been verified by a synch object +// Check if the 2nd access conflicts with the first and return true if there's a conflict +// Pre: Both accesses must occur on the same VkDeviceMemory object +static bool MemoryConflict(MemoryAccess *initial_access, MemoryAccess *second_access) { + assert(initial_access->location.mem == second_access->location.mem); + // read/read is ok + // TODO: What to do for write/write? Warn? Nothing for now. Just allow RaR & WaW cases. + if (initial_access->write == second_access->write) return false; + // RaW & WaR can present conflicts + // If the second access is outside of the range of initial access, then no conflict + if ((initial_access->location.size != VK_WHOLE_SIZE && second_access->location.size != VK_WHOLE_SIZE) && + ((second_access->location.offset + second_access->location.size) <= initial_access->location.offset) && + (second_access->location.offset >= (initial_access->location.offset + initial_access->location.size))) { + return false; + } + // Without appropriate barrier we'll have a conflict + if (initial_access->mem_barrier || (initial_access->pipe_barrier && second_access->write)) return false; + // there's some amount of overlap in the accesses so flag conflict + return true; +} + +// Check memory accesses in cmd buffer up to this point and flag any conflicts +static bool ValidateMemoryAccess(layer_data *dev_data, GLOBAL_CB_NODE *cb_state, MemoryAccess *mem_access, const char *caller) { + bool skip = false; + auto mem_obj = mem_access->location.mem; + auto mem_access_pair = cb_state->memory_accesses.find(mem_obj); + if (mem_access_pair != cb_state->memory_accesses.end()) { + for (auto earlier_access : mem_access_pair->second) { + if (MemoryConflict(&earlier_access, mem_access)) { + const char *access1 = earlier_access.write ? "write" : "read"; + const char *access2 = mem_access->write ? "write" : "read"; + skip |= log_msg(dev_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_DEVICE_MEMORY_EXT, + HandleToUint64(mem_access->location.mem), 0, MEMTRACK_SYNCHRONIZATION_ERROR, "DS", + "%s called on VkCommandBuffer 0x%" PRIxLEAST64 " causes a %s after %s conflict.", caller, + HandleToUint64(cb_state->commandBuffer), access2, access1); + } + } + } + return skip; +} + // Generic function to handle validation for all CmdDraw* type functions static bool ValidateCmdDrawType(layer_data *dev_data, VkCommandBuffer cmd_buffer, bool indexed, VkPipelineBindPoint bind_point, CMD_TYPE cmd_type, GLOBAL_CB_NODE **cb_state, const char *caller, VkQueueFlags queue_flags, @@ -5729,21 +5772,43 @@ VKAPI_ATTR void VKAPI_CALL CmdDrawIndexed(VkCommandBuffer commandBuffer, uint32_ static bool PreCallValidateCmdDrawIndirect(layer_data *dev_data, VkCommandBuffer cmd_buffer, VkBuffer buffer, bool indexed, VkPipelineBindPoint bind_point, GLOBAL_CB_NODE **cb_state, BUFFER_STATE **buffer_state, - const char *caller) { + std::vector *mem_accesses, VkDeviceSize offset, uint32_t count, + uint32_t stride, const char *caller) { bool skip = ValidateCmdDrawType(dev_data, cmd_buffer, indexed, bind_point, CMD_DRAWINDIRECT, cb_state, caller, VK_QUEUE_GRAPHICS_BIT, VALIDATION_ERROR_1aa02415, VALIDATION_ERROR_1aa00017, VALIDATION_ERROR_1aa003cc); *buffer_state = GetBufferState(dev_data, buffer); skip |= ValidateMemoryIsBoundToBuffer(dev_data, *buffer_state, caller, VALIDATION_ERROR_1aa003b4); + // TODO: This is temp code to test specific case that needs to be generalized for memory dependency checks + MemoryAccess mem_access; + mem_access.write = false; + auto mem_obj = (*buffer_state)->binding.mem; + mem_access.location.mem = mem_obj; + mem_access.location.offset = offset; + // make sure size is non-zero for count of 1 + auto size = stride * (count-1) + sizeof(VkDrawIndirectCommand); + mem_access.location.size = size; + mem_access.src_stage_flags = VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_DRAW_INDIRECT_BIT; + mem_access.src_access_flags = VK_ACCESS_INDIRECT_COMMAND_READ_BIT | VK_ACCESS_MEMORY_READ_BIT; + mem_accesses->emplace_back(mem_access); + skip |= ValidateMemoryAccess(dev_data, *cb_state, &mem_access, caller); // TODO: If the drawIndirectFirstInstance feature is not enabled, all the firstInstance members of the // VkDrawIndirectCommand structures accessed by this command must be 0, which will require access to the contents of 'buffer'. return skip; } static void PostCallRecordCmdDrawIndirect(layer_data *dev_data, GLOBAL_CB_NODE *cb_state, VkPipelineBindPoint bind_point, - BUFFER_STATE *buffer_state) { + BUFFER_STATE *buffer_state, std::vector *mem_accesses) { UpdateStateCmdDrawType(dev_data, cb_state, bind_point); AddCommandBufferBindingBuffer(dev_data, cb_state, buffer_state); + // TODO: This is temporary to test out potential synch design + cb_state->commands.emplace_back(unique_ptr(new Command(CMD_DRAWINDIRECT))); + Command *cmd_ptr = cb_state->commands.back().get(); + for (auto mem_access : *mem_accesses) { + mem_access.cmd = cmd_ptr; + cmd_ptr->AddMemoryAccess(mem_access); + cb_state->memory_accesses[mem_access.location.mem].push_back(mem_access); + } } VKAPI_ATTR void VKAPI_CALL CmdDrawIndirect(VkCommandBuffer commandBuffer, VkBuffer buffer, VkDeviceSize offset, uint32_t count, @@ -5751,14 +5816,15 @@ VKAPI_ATTR void VKAPI_CALL CmdDrawIndirect(VkCommandBuffer commandBuffer, VkBuff layer_data *dev_data = GetLayerDataPtr(get_dispatch_key(commandBuffer), layer_data_map); GLOBAL_CB_NODE *cb_state = nullptr; BUFFER_STATE *buffer_state = nullptr; + std::vector mem_accesses; unique_lock_t lock(global_lock); bool skip = PreCallValidateCmdDrawIndirect(dev_data, commandBuffer, buffer, false, VK_PIPELINE_BIND_POINT_GRAPHICS, &cb_state, - &buffer_state, "vkCmdDrawIndirect()"); + &buffer_state, &mem_accesses, offset, count, stride, "vkCmdDrawIndirect()"); lock.unlock(); if (!skip) { dev_data->dispatch_table.CmdDrawIndirect(commandBuffer, buffer, offset, count, stride); lock.lock(); - PostCallRecordCmdDrawIndirect(dev_data, cb_state, VK_PIPELINE_BIND_POINT_GRAPHICS, buffer_state); + PostCallRecordCmdDrawIndirect(dev_data, cb_state, VK_PIPELINE_BIND_POINT_GRAPHICS, buffer_state, &mem_accesses); lock.unlock(); } } @@ -6003,7 +6069,8 @@ static bool PreCallCmdUpdateBuffer(layer_data *device_data, const GLOBAL_CB_NODE return skip; } -static void PostCallRecordCmdUpdateBuffer(layer_data *device_data, GLOBAL_CB_NODE *cb_state, BUFFER_STATE *dst_buffer_state) { +static void PostCallRecordCmdUpdateBuffer(layer_data *device_data, GLOBAL_CB_NODE *cb_state, BUFFER_STATE *dst_buffer_state, + VkDeviceSize offset, VkDeviceSize data_size) { // Update bindings between buffer and cmd buffer AddCommandBufferBindingBuffer(device_data, cb_state, dst_buffer_state); std::function function = [=]() { @@ -6011,6 +6078,20 @@ static void PostCallRecordCmdUpdateBuffer(layer_data *device_data, GLOBAL_CB_NOD return false; }; cb_state->queue_submit_functions.push_back(function); + // TODO: This is simple hack to test a single case, need to generalize for all cmds + cb_state->commands.emplace_back(unique_ptr(new Command(CMD_UPDATEBUFFER))); + Command *cmd_ptr = cb_state->commands.back().get(); + MemoryAccess mem_access; + mem_access.write = true; + auto mem_obj = dst_buffer_state->binding.mem; + mem_access.location.mem = mem_obj; + mem_access.location.offset = offset; + mem_access.location.size = data_size; + mem_access.cmd = cmd_ptr; + mem_access.src_stage_flags = VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_TRANSFER_BIT; + mem_access.src_access_flags = VK_ACCESS_TRANSFER_WRITE_BIT | VK_ACCESS_MEMORY_WRITE_BIT; + cmd_ptr->AddMemoryAccess(mem_access); + cb_state->memory_accesses[mem_obj].push_back(mem_access); } VKAPI_ATTR void VKAPI_CALL CmdUpdateBuffer(VkCommandBuffer commandBuffer, VkBuffer dstBuffer, VkDeviceSize dstOffset, @@ -6028,7 +6109,7 @@ VKAPI_ATTR void VKAPI_CALL CmdUpdateBuffer(VkCommandBuffer commandBuffer, VkBuff if (!skip) { dev_data->dispatch_table.CmdUpdateBuffer(commandBuffer, dstBuffer, dstOffset, dataSize, pData); lock.lock(); - PostCallRecordCmdUpdateBuffer(dev_data, cb_state, dst_buff_state); + PostCallRecordCmdUpdateBuffer(dev_data, cb_state, dst_buff_state, dstOffset, dataSize); lock.unlock(); } } @@ -6849,8 +6930,65 @@ static bool PreCallValidateCmdPipelineBarrier(layer_data *device_data, GLOBAL_CB } static void PreCallRecordCmdPipelineBarrier(layer_data *device_data, GLOBAL_CB_NODE *cb_state, VkCommandBuffer commandBuffer, - uint32_t imageMemoryBarrierCount, const VkImageMemoryBarrier *pImageMemoryBarriers) { + VkPipelineStageFlags srcStageMask, VkPipelineStageFlags dstStageMask, + VkDependencyFlags dependencyFlags, uint32_t mem_barrier_count, + const VkMemoryBarrier *mem_barriers, uint32_t buffer_mem_barrier_count, + const VkBufferMemoryBarrier *buffer_mem_barriers, uint32_t imageMemoryBarrierCount, + const VkImageMemoryBarrier *pImageMemoryBarriers) { TransitionImageLayouts(device_data, commandBuffer, imageMemoryBarrierCount, pImageMemoryBarriers); + cb_state->commands.emplace_back(unique_ptr(new Command(CMD_PIPELINEBARRIER))); + Command *cmd_ptr = cb_state->commands.back().get(); + // TODO: Make this barrier/access parsing smarter + if (mem_barrier_count) { + for (auto &mem_access_pair : cb_state->memory_accesses) { + // Global barriers affect all outstanding memory accesses that match access mask + // TODO: For now just flagging all accesses, but need to store mask w/ access and check it here + // against each individual global mem barrier + for (auto &mem_access : mem_access_pair.second) { + // For every global barrier that matches access mask, record the barrier + for (uint32_t i = 0; i < mem_barrier_count; ++i) { + const auto &mem_barrier = mem_barriers[i]; + if ((mem_barrier.srcAccessMask & mem_access.src_access_flags) != 0) { + // This memory barrier applies to earlier access so record details + mem_access.mem_barrier = true; + mem_access.dst_access_flags |= mem_barrier.dstAccessMask; + mem_access.synch_commands.push_back(cmd_ptr); + } + } + } + } + } + for (uint32_t i = 0; i < buffer_mem_barrier_count; ++i) { + const auto &buff_barrier = buffer_mem_barriers[i]; + const auto &buff_state = GetBufferState(device_data, buff_barrier.buffer); + const auto mem_obj = buff_state->binding.mem; + // Make an access struct with barrier range details to check for overlap + // TODO: This is hacky, creating tmp access struct from barrier to check conflict + // need to rework original function so this makes sense or write alternate function + MemoryAccess barrier_access = {}; + barrier_access.location.mem = mem_obj; + barrier_access.location.offset = buff_barrier.offset; + barrier_access.location.size = buff_barrier.size; + barrier_access.mem_barrier = false; + barrier_access.pipe_barrier = false; + const auto &mem_access_pair = cb_state->memory_accesses.find(mem_obj); + if (mem_access_pair != cb_state->memory_accesses.end()) { + for (auto &mem_access : mem_access_pair->second) { + // If the access or pipe masks overlap, then barrier applies + if (((mem_access.src_access_flags & buff_barrier.srcAccessMask) != 0) || + ((mem_access.src_stage_flags & srcStageMask) != 0)) { + // Set buff access write opposite of actual access so conflict is flagged on overlap + barrier_access.write = !mem_access.write; + if (MemoryConflict(&barrier_access, &mem_access)) { + mem_access.mem_barrier = true; + mem_access.dst_stage_flags |= dstStageMask; + mem_access.dst_access_flags |= buff_barrier.dstAccessMask; + mem_access.synch_commands.push_back(cmd_ptr); + } + } + } + } + } } VKAPI_ATTR void VKAPI_CALL CmdPipelineBarrier(VkCommandBuffer commandBuffer, VkPipelineStageFlags srcStageMask, @@ -6867,7 +7005,9 @@ VKAPI_ATTR void VKAPI_CALL CmdPipelineBarrier(VkCommandBuffer commandBuffer, VkP memoryBarrierCount, pMemoryBarriers, bufferMemoryBarrierCount, pBufferMemoryBarriers, imageMemoryBarrierCount, pImageMemoryBarriers); if (!skip) { - PreCallRecordCmdPipelineBarrier(device_data, cb_state, commandBuffer, imageMemoryBarrierCount, pImageMemoryBarriers); + PreCallRecordCmdPipelineBarrier(device_data, cb_state, commandBuffer, srcStageMask, dstStageMask, dependencyFlags, + memoryBarrierCount, pMemoryBarriers, bufferMemoryBarrierCount, pBufferMemoryBarriers, + imageMemoryBarrierCount, pImageMemoryBarriers); } } else { assert(0); diff --git a/layers/core_validation_error_enums.h b/layers/core_validation_error_enums.h index 12099c3275..746f7783f0 100644 --- a/layers/core_validation_error_enums.h +++ b/layers/core_validation_error_enums.h @@ -29,6 +29,7 @@ enum MEM_TRACK_ERROR { MEMTRACK_INVALID_CB, MEMTRACK_INVALID_MEM_OBJ, MEMTRACK_INVALID_ALIASING, + MEMTRACK_SYNCHRONIZATION_ERROR, MEMTRACK_INTERNAL_ERROR, MEMTRACK_FREED_MEM_REF, MEMTRACK_INVALID_OBJECT, diff --git a/layers/core_validation_types.h b/layers/core_validation_types.h index 5c14d95f38..3e62cfb4ae 100644 --- a/layers/core_validation_types.h +++ b/layers/core_validation_types.h @@ -440,6 +440,47 @@ enum CMD_TYPE { CMD_END, // Should be last command in any RECORDED cmd buffer }; +// fwd decl class for ptr below +class Command; +// Store details of memory access by a cmd +struct MemoryAccess { + MemoryAccess() + : write(false), + cmd(nullptr), + location{0, 0, 0}, + src_stage_flags(0), + src_access_flags(0), + dst_stage_flags(0), + dst_access_flags(0), + mem_barrier(false), + pipe_barrier(false), + synch_commands{} {}; + bool write; // False for read access + Command *cmd; // Ptr to cmd that makes this access + MEM_BINDING location; // mem/offset/size being accessed + // The source flags cover all access bits for the original memory access + // These flags are checked against synch objects to check if scope covers this access + VkPipelineStageFlags src_stage_flags; // Stage flags within scope of this access + VkAccessFlags src_access_flags; // Access flags within scope of this access + // The following members are only set when a valid synch object is added following this access + // When such an event occurs, the relevant dest scope flags are recorded here + VkPipelineStageFlags dst_stage_flags; // Stage flags within scope of this access + VkAccessFlags dst_access_flags; // Access flags within scope of this access + // These bool shortcuts indicate if memory and/or pipeline barriers have occurred since this access + bool mem_barrier; // True after relevant mem barrier added to CB following this access + bool pipe_barrier; // True after relevant mem barrier added to CB following this access + // A vector of all synch commands that affect this memory access + std::vector synch_commands; // Relevent synch commands +}; + +class Command { + public: + Command(CMD_TYPE type) : type(type){}; + void AddMemoryAccess(MemoryAccess access) { mem_accesses.push_back(access); }; + CMD_TYPE type; + std::vector mem_accesses; // vector of all mem accesses by this cmd +}; + enum CB_STATE { CB_NEW, // Newly created CB w/o any cmds CB_RECORDING, // BeginCB has been called on this CB @@ -663,6 +704,10 @@ struct GLOBAL_CB_NODE : public BASE_NODE { // dependencies that have been broken : either destroyed objects, or updated descriptor sets std::unordered_set object_bindings; std::vector broken_bindings; + // + std::vector> commands; // Commands in this command buffer + // Store all memory accesses per memory object made in this command buffer + std::unordered_map> memory_accesses; std::unordered_set waitedEvents; std::vector writeEventsBeforeWait; From 53adc7378466ca0997485d34b747c0e32b091732 Mon Sep 17 00:00:00 2001 From: Tobin Ehlis Date: Wed, 2 Aug 2017 16:13:14 -0600 Subject: [PATCH 02/28] tests:Add positive barrier test This is a simple test positive case based on GH892. For this initial test the pipeline barrier is correctly in place so no errors should be signalled. This is the basic case that will be used to generate some more test cases, both positive and negative. --- tests/layer_validation_tests.cpp | 57 ++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/tests/layer_validation_tests.cpp b/tests/layer_validation_tests.cpp index a363377ad2..0e6322b699 100644 --- a/tests/layer_validation_tests.cpp +++ b/tests/layer_validation_tests.cpp @@ -10201,6 +10201,63 @@ TEST_F(VkLayerTest, UpdateBufferWithinRenderPass) { m_errorMonitor->VerifyFound(); } +TEST_F(VkPositiveLayerTest, UpdateBufferRaWDependencyWithBarrier) { + TEST_DESCRIPTION("Update buffer used in CmdDrawIndirect w/ barrier before use"); + + m_errorMonitor->ExpectSuccess(); + ASSERT_NO_FATAL_FAILURE(Init()); + ASSERT_NO_FATAL_FAILURE(InitRenderTarget()); + + m_commandBuffer->begin(); + + VkMemoryPropertyFlags reqs = VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT; + vk_testing::Buffer dst_buffer; + dst_buffer.init_as_dst(*m_device, (VkDeviceSize)1024, reqs); + + VkDeviceSize dstOffset = 0; + uint32_t Data[] = {1, 2, 3, 4, 5, 6, 7, 8}; + VkDeviceSize dataSize = sizeof(Data) / sizeof(uint32_t); + vkCmdUpdateBuffer(m_commandBuffer->handle(), dst_buffer.handle(), dstOffset, dataSize, &Data); + // Global Barrier to make sure buffer update completed + VkMemoryBarrier mem_barrier = {}; + mem_barrier.sType = VK_STRUCTURE_TYPE_MEMORY_BARRIER; + mem_barrier.srcAccessMask = VK_ACCESS_TRANSFER_WRITE_BIT; + mem_barrier.dstAccessMask = VK_ACCESS_INDIRECT_COMMAND_READ_BIT; + vkCmdPipelineBarrier(m_commandBuffer->handle(), VK_PIPELINE_STAGE_TRANSFER_BIT, VK_PIPELINE_STAGE_DRAW_INDIRECT_BIT, 0, 1, + &mem_barrier, 0, nullptr, 0, nullptr); + // Start renderpass for DrawIndirect + m_commandBuffer->BeginRenderPass(m_renderPassBeginInfo); + // Bind gfx PSO to prevent unexpected errors + // Create PSO to be used for draw-time errors below + VkShaderObj vs(m_device, bindStateVertShaderText, VK_SHADER_STAGE_VERTEX_BIT, this); + VkShaderObj fs(m_device, bindStateFragShaderText, VK_SHADER_STAGE_FRAGMENT_BIT, this); + VkPipelineObj pipe(m_device); + pipe.AddShader(&vs); + pipe.AddShader(&fs); + VkViewport view_port = {}; + m_viewports.push_back(view_port); + pipe.SetViewport(m_viewports); + VkRect2D rect = {}; + m_scissors.push_back(rect); + pipe.SetScissor(m_scissors); + pipe.AddColorAttachment(); + VkPipelineLayoutCreateInfo pipeline_layout_ci = {}; + pipeline_layout_ci.sType = VK_STRUCTURE_TYPE_PIPELINE_LAYOUT_CREATE_INFO; + pipeline_layout_ci.setLayoutCount = 0; + pipeline_layout_ci.pSetLayouts = NULL; + + VkPipelineLayout pipeline_layout; + VkResult err = vkCreatePipelineLayout(m_device->device(), &pipeline_layout_ci, NULL, &pipeline_layout); + ASSERT_VK_SUCCESS(err); + pipe.CreateVKPipeline(pipeline_layout, renderPass()); + + vkCmdBindPipeline(m_commandBuffer->handle(), VK_PIPELINE_BIND_POINT_GRAPHICS, pipe.handle()); + // Now issue indirect draw + vkCmdDrawIndirect(m_commandBuffer->handle(), dst_buffer.handle(), 0, 1, 0); + m_errorMonitor->VerifyNotFound(); + vkDestroyPipelineLayout(m_device->device(), pipeline_layout, nullptr); +} + TEST_F(VkLayerTest, ClearColorImageWithBadRange) { TEST_DESCRIPTION("Record clear color with an invalid VkImageSubresourceRange"); From 0075d3f29bd60dfb0410cfa42c59a79019c452fc Mon Sep 17 00:00:00 2001 From: Tobin Ehlis Date: Wed, 2 Aug 2017 16:22:48 -0600 Subject: [PATCH 03/28] tests:Add UpdateBufferRaWDependencyMissingBarrier This is a negative test featuring a RaW dependency with no synch object in-between to regulate the read. --- tests/layer_validation_tests.cpp | 52 ++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/tests/layer_validation_tests.cpp b/tests/layer_validation_tests.cpp index 0e6322b699..ea0d2afd45 100644 --- a/tests/layer_validation_tests.cpp +++ b/tests/layer_validation_tests.cpp @@ -10258,6 +10258,58 @@ TEST_F(VkPositiveLayerTest, UpdateBufferRaWDependencyWithBarrier) { vkDestroyPipelineLayout(m_device->device(), pipeline_layout, nullptr); } +TEST_F(VkLayerTest, UpdateBufferRaWDependencyMissingBarrier) { + TEST_DESCRIPTION("Update buffer used in CmdDrawIndirect without a barrier before use"); + + ASSERT_NO_FATAL_FAILURE(Init()); + ASSERT_NO_FATAL_FAILURE(InitRenderTarget()); + + m_commandBuffer->begin(); + + VkMemoryPropertyFlags reqs = VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT; + vk_testing::Buffer dst_buffer; + dst_buffer.init_as_dst(*m_device, (VkDeviceSize)1024, reqs); + + VkDeviceSize dstOffset = 0; + uint32_t Data[] = {1, 2, 3, 4, 5, 6, 7, 8}; + VkDeviceSize dataSize = sizeof(Data) / sizeof(uint32_t); + vkCmdUpdateBuffer(m_commandBuffer->handle(), dst_buffer.handle(), dstOffset, dataSize, &Data); + // No barrier present here should lead to RaW error + + // Start renderpass for DrawIndirect + m_commandBuffer->BeginRenderPass(m_renderPassBeginInfo); + // Bind gfx PSO to prevent unexpected errors + // Create PSO to be used for draw-time errors below + VkShaderObj vs(m_device, bindStateVertShaderText, VK_SHADER_STAGE_VERTEX_BIT, this); + VkShaderObj fs(m_device, bindStateFragShaderText, VK_SHADER_STAGE_FRAGMENT_BIT, this); + VkPipelineObj pipe(m_device); + pipe.AddShader(&vs); + pipe.AddShader(&fs); + VkViewport view_port = {}; + m_viewports.push_back(view_port); + pipe.SetViewport(m_viewports); + VkRect2D rect = {}; + m_scissors.push_back(rect); + pipe.SetScissor(m_scissors); + pipe.AddColorAttachment(); + VkPipelineLayoutCreateInfo pipeline_layout_ci = {}; + pipeline_layout_ci.sType = VK_STRUCTURE_TYPE_PIPELINE_LAYOUT_CREATE_INFO; + pipeline_layout_ci.setLayoutCount = 0; + pipeline_layout_ci.pSetLayouts = NULL; + + VkPipelineLayout pipeline_layout; + VkResult err = vkCreatePipelineLayout(m_device->device(), &pipeline_layout_ci, NULL, &pipeline_layout); + ASSERT_VK_SUCCESS(err); + pipe.CreateVKPipeline(pipeline_layout, renderPass()); + + vkCmdBindPipeline(m_commandBuffer->handle(), VK_PIPELINE_BIND_POINT_GRAPHICS, pipe.handle()); + // Now issue indirect draw + m_errorMonitor->SetDesiredFailureMsg(VK_DEBUG_REPORT_WARNING_BIT_EXT, " causes a read after write conflict with "); + vkCmdDrawIndirect(m_commandBuffer->handle(), dst_buffer.handle(), 0, 1, 0); + m_errorMonitor->VerifyFound(); + vkDestroyPipelineLayout(m_device->device(), pipeline_layout, nullptr); +} + TEST_F(VkLayerTest, ClearColorImageWithBadRange) { TEST_DESCRIPTION("Record clear color with an invalid VkImageSubresourceRange"); From 8ad10cc3c57dbca769da4bafaacac66f2f7d01fe Mon Sep 17 00:00:00 2001 From: Tobin Ehlis Date: Fri, 4 Aug 2017 08:53:27 -0600 Subject: [PATCH 04/28] layers:Generalizing code to add MemAccess per cmd Added a look-up-table for fixed pipe stage and access masks per cmd. Added some functions to generalize adding/validating memory accesses per command and migrated the checks into buffer_validation files. Added CmdCopyImage tracking. Initially treating all image accesses conservatively and assuming they touch full size of image memory. To deal with this and further-such issues that will arise I updated MemoryAccess to have a "precise" bool that indicates when Access details are exact. Only when both Accesses are precise can we issue an error, otherwise just issue a warning. --- layers/buffer_validation.cpp | 111 ++++++++++++++++++---- layers/buffer_validation.h | 18 +++- layers/core_validation.cpp | 132 +++++++++---------------- layers/core_validation_types.h | 169 ++++++++++++++++++++++++++++++++- 4 files changed, 322 insertions(+), 108 deletions(-) diff --git a/layers/buffer_validation.cpp b/layers/buffer_validation.cpp index 1d1b9038cb..78ebc7d34a 100644 --- a/layers/buffer_validation.cpp +++ b/layers/buffer_validation.cpp @@ -635,6 +635,71 @@ void TransitionFinalSubpassLayouts(layer_data *device_data, GLOBAL_CB_NODE *pCB, } } +// START: Memory Access Validation Functions +void AddMemoryAccess(CMD_TYPE cmd, std::vector *mem_accesses, MemoryAccess *mem_access, bool write, + uint32_t rw_index) { + mem_access->write = write; + mem_access->src_stage_flags = CommandToFlags[cmd][rw_index].stage_flags; + mem_access->src_access_flags = CommandToFlags[cmd][rw_index].access_flags; + // TODO: If dynamic true lookup dynamic flag status + mem_accesses->emplace_back(*mem_access); +} + +void AddReadMemoryAccess(CMD_TYPE cmd, std::vector *mem_accesses, MemoryAccess *mem_access) { + AddMemoryAccess(cmd, mem_accesses, mem_access, false, 0); +} + +void AddWriteMemoryAccess(CMD_TYPE cmd, std::vector *mem_accesses, MemoryAccess *mem_access) { + AddMemoryAccess(cmd, mem_accesses, mem_access, true, 1); +} + +// The first arg is an existing memory access that hasn't been verified by a synch object +// Check if the 2nd access conflicts with the first and return true if there's a conflict +// Pre: Both accesses must occur on the same VkDeviceMemory object +bool MemoryConflict(MemoryAccess const *initial_access, MemoryAccess const *second_access) { + assert(initial_access->location.mem == second_access->location.mem); + // read/read is ok + // TODO: What to do for write/write? Warn? Nothing for now. Just allow RaR & WaW cases. + if (initial_access->write == second_access->write) return false; + // RaW & WaR can present conflicts + // If the second access is outside of the range of initial access, then no conflict + if ((initial_access->location.size != VK_WHOLE_SIZE && second_access->location.size != VK_WHOLE_SIZE) && + ((second_access->location.offset + second_access->location.size) <= initial_access->location.offset) && + (second_access->location.offset >= (initial_access->location.offset + initial_access->location.size))) { + return false; + } + // Without appropriate barrier we'll have a conflict + if (initial_access->mem_barrier || (initial_access->pipe_barrier && second_access->write)) return false; + // there's some amount of overlap in the accesses so flag conflict + return true; +} + +// Check given set of mem_accesses against memory accesses in cmd buffer up to this point and flag any conflicts +bool ValidateMemoryAccesses(debug_report_data const *report_data, GLOBAL_CB_NODE const *cb_state, + std::vector *mem_accesses, const char *caller) { + bool skip = false; + for (const auto &mem_access : *mem_accesses) { + auto mem_obj = mem_access.location.mem; + auto mem_access_pair = cb_state->memory_accesses.find(mem_obj); + if (mem_access_pair != cb_state->memory_accesses.end()) { + for (const auto &earlier_access : mem_access_pair->second) { + if (MemoryConflict(&earlier_access, &mem_access)) { + const char *access1 = earlier_access.write ? "write" : "read"; + const char *access2 = mem_access.write ? "write" : "read"; + // If either access is imprecise can only warn, otherwise can error + auto level = VK_DEBUG_REPORT_WARNING_BIT_EXT; + if (earlier_access.precise && mem_access.precise) level = VK_DEBUG_REPORT_ERROR_BIT_EXT; + skip |= log_msg(report_data, level, VK_DEBUG_REPORT_OBJECT_TYPE_DEVICE_MEMORY_EXT, + HandleToUint64(mem_access.location.mem), 0, MEMTRACK_SYNCHRONIZATION_ERROR, "DS", + "%s called on VkCommandBuffer 0x%" PRIxLEAST64 " causes a %s after %s conflict.", caller, + HandleToUint64(cb_state->commandBuffer), access2, access1); + } + } + } + } + return skip; +} + bool PreCallValidateCreateImage(layer_data *device_data, const VkImageCreateInfo *pCreateInfo, const VkAllocationCallbacks *pAllocator, VkImage *pImage) { bool skip = false; @@ -1637,14 +1702,15 @@ bool ValidateImageCopyData(const layer_data *device_data, const debug_report_dat return skip; } -bool PreCallValidateCmdCopyImage(layer_data *device_data, GLOBAL_CB_NODE *cb_node, IMAGE_STATE *src_image_state, +bool PreCallValidateCmdCopyImage(layer_data *device_data, GLOBAL_CB_NODE *cb_state, IMAGE_STATE *src_image_state, IMAGE_STATE *dst_image_state, uint32_t region_count, const VkImageCopy *regions, - VkImageLayout src_image_layout, VkImageLayout dst_image_layout) { + VkImageLayout src_image_layout, VkImageLayout dst_image_layout, + std::vector *mem_accesses) { bool skip = false; const debug_report_data *report_data = core_validation::GetReportData(device_data); skip = ValidateImageCopyData(device_data, report_data, region_count, regions, src_image_state, dst_image_state); - VkCommandBuffer command_buffer = cb_node->commandBuffer; + VkCommandBuffer command_buffer = cb_state->commandBuffer; for (uint32_t i = 0; i < region_count; i++) { bool slice_override = false; @@ -1900,6 +1966,15 @@ bool PreCallValidateCmdCopyImage(layer_data *device_data, GLOBAL_CB_NODE *cb_nod } } } + // Add memory access for this image copy + // TODO: Currently treating all image accesses as imprecise & covering the whole image area + const auto &src_binding = src_image_state->binding; + MemoryAccess src_mem_access(src_binding.mem, src_binding.offset, src_binding.size, false); + AddReadMemoryAccess(CMD_COPYIMAGE, mem_accesses, &src_mem_access); + const auto &dst_binding = dst_image_state->binding; + MemoryAccess dst_mem_access(dst_binding.mem, dst_binding.offset, dst_binding.size, false); + AddWriteMemoryAccess(CMD_COPYIMAGE, mem_accesses, &dst_mem_access); + skip |= ValidateMemoryAccesses(report_data, cb_state, mem_accesses, "vkCmdCopyImage()"); } // The formats of src_image and dst_image must be compatible. Formats are considered compatible if their texel size in bytes @@ -1937,41 +2012,43 @@ bool PreCallValidateCmdCopyImage(layer_data *device_data, GLOBAL_CB_NODE *cb_nod "vkCmdCopyImage()", "VK_IMAGE_USAGE_TRANSFER_SRC_BIT"); skip |= ValidateImageUsageFlags(device_data, dst_image_state, VK_IMAGE_USAGE_TRANSFER_DST_BIT, true, VALIDATION_ERROR_19000106, "vkCmdCopyImage()", "VK_IMAGE_USAGE_TRANSFER_DST_BIT"); - skip |= ValidateCmdQueueFlags(device_data, cb_node, "vkCmdCopyImage()", + skip |= ValidateCmdQueueFlags(device_data, cb_state, "vkCmdCopyImage()", VK_QUEUE_TRANSFER_BIT | VK_QUEUE_GRAPHICS_BIT | VK_QUEUE_COMPUTE_BIT, VALIDATION_ERROR_19002415); - skip |= ValidateCmd(device_data, cb_node, CMD_COPYIMAGE, "vkCmdCopyImage()"); - skip |= insideRenderPass(device_data, cb_node, "vkCmdCopyImage()", VALIDATION_ERROR_19000017); + skip |= ValidateCmd(device_data, cb_state, CMD_COPYIMAGE, "vkCmdCopyImage()"); + skip |= insideRenderPass(device_data, cb_state, "vkCmdCopyImage()", VALIDATION_ERROR_19000017); bool hit_error = false; for (uint32_t i = 0; i < region_count; ++i) { - skip |= VerifyImageLayout(device_data, cb_node, src_image_state, regions[i].srcSubresource, src_image_layout, + skip |= VerifyImageLayout(device_data, cb_state, src_image_state, regions[i].srcSubresource, src_image_layout, VK_IMAGE_LAYOUT_TRANSFER_SRC_OPTIMAL, "vkCmdCopyImage()", VALIDATION_ERROR_19000102, &hit_error); - skip |= VerifyImageLayout(device_data, cb_node, dst_image_state, regions[i].dstSubresource, dst_image_layout, + skip |= VerifyImageLayout(device_data, cb_state, dst_image_state, regions[i].dstSubresource, dst_image_layout, VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL, "vkCmdCopyImage()", VALIDATION_ERROR_1900010c, &hit_error); - skip |= ValidateCopyImageTransferGranularityRequirements(device_data, cb_node, src_image_state, dst_image_state, + skip |= ValidateCopyImageTransferGranularityRequirements(device_data, cb_state, src_image_state, dst_image_state, ®ions[i], i, "vkCmdCopyImage()"); } return skip; } -void PreCallRecordCmdCopyImage(layer_data *device_data, GLOBAL_CB_NODE *cb_node, IMAGE_STATE *src_image_state, +void PreCallRecordCmdCopyImage(layer_data *device_data, GLOBAL_CB_NODE *cb_state, IMAGE_STATE *src_image_state, IMAGE_STATE *dst_image_state, uint32_t region_count, const VkImageCopy *regions, - VkImageLayout src_image_layout, VkImageLayout dst_image_layout) { + VkImageLayout src_image_layout, VkImageLayout dst_image_layout, + std::vector *mem_accesses) { // Make sure that all image slices are updated to correct layout for (uint32_t i = 0; i < region_count; ++i) { - SetImageLayout(device_data, cb_node, src_image_state, regions[i].srcSubresource, src_image_layout); - SetImageLayout(device_data, cb_node, dst_image_state, regions[i].dstSubresource, dst_image_layout); + SetImageLayout(device_data, cb_state, src_image_state, regions[i].srcSubresource, src_image_layout); + SetImageLayout(device_data, cb_state, dst_image_state, regions[i].dstSubresource, dst_image_layout); } // Update bindings between images and cmd buffer - AddCommandBufferBindingImage(device_data, cb_node, src_image_state); - AddCommandBufferBindingImage(device_data, cb_node, dst_image_state); + AddCommandBufferBindingImage(device_data, cb_state, src_image_state); + AddCommandBufferBindingImage(device_data, cb_state, dst_image_state); std::function function = [=]() { return ValidateImageMemoryIsValid(device_data, src_image_state, "vkCmdCopyImage()"); }; - cb_node->queue_submit_functions.push_back(function); + cb_state->queue_submit_functions.push_back(function); function = [=]() { SetImageMemoryValid(device_data, dst_image_state, true); return false; }; - cb_node->queue_submit_functions.push_back(function); + cb_state->queue_submit_functions.push_back(function); + AddCommandBufferCommandMemoryAccesses(cb_state, CMD_COPYIMAGE, mem_accesses); } // Returns true if sub_rect is entirely contained within rect diff --git a/layers/buffer_validation.h b/layers/buffer_validation.h index dbc0127ee8..3ec972c4c5 100644 --- a/layers/buffer_validation.h +++ b/layers/buffer_validation.h @@ -133,9 +133,22 @@ bool VerifyDestImageLayout(layer_data *dev_data, GLOBAL_CB_NODE *cb_node, VkImag void TransitionFinalSubpassLayouts(layer_data *dev_data, GLOBAL_CB_NODE *pCB, const VkRenderPassBeginInfo *pRenderPassBegin, FRAMEBUFFER_STATE *framebuffer_state); +void AddMemoryAccess(CMD_TYPE cmd, std::vector *mem_accesses, MemoryAccess *mem_access, bool write, + uint32_t rw_index); + +void AddReadMemoryAccess(CMD_TYPE cmd, std::vector *mem_accesses, MemoryAccess *mem_access); + +void AddWriteMemoryAccess(CMD_TYPE cmd, std::vector *mem_accesses, MemoryAccess *mem_access); + +bool MemoryConflict(MemoryAccess const *initial_access, MemoryAccess const *second_access); + +bool ValidateMemoryAccesses(debug_report_data const *report_data, GLOBAL_CB_NODE const *cb_state, std::vector *mem_accesses, + const char *caller); + bool PreCallValidateCmdCopyImage(layer_data *device_data, GLOBAL_CB_NODE *cb_node, IMAGE_STATE *src_image_state, IMAGE_STATE *dst_image_state, uint32_t region_count, const VkImageCopy *regions, - VkImageLayout src_image_layout, VkImageLayout dst_image_layout); + VkImageLayout src_image_layout, VkImageLayout dst_image_layout, + std::vector *mem_accesses); bool PreCallValidateCmdClearAttachments(layer_data *device_data, VkCommandBuffer commandBuffer, uint32_t attachmentCount, const VkClearAttachment *pAttachments, uint32_t rectCount, const VkClearRect *pRects); @@ -209,7 +222,8 @@ bool ValidateCopyBufferImageTransferGranularityRequirements(layer_data *device_d void PreCallRecordCmdCopyImage(layer_data *device_data, GLOBAL_CB_NODE *cb_node, IMAGE_STATE *src_image_state, IMAGE_STATE *dst_image_state, uint32_t region_count, const VkImageCopy *regions, - VkImageLayout src_image_layout, VkImageLayout dst_image_layout); + VkImageLayout src_image_layout, VkImageLayout dst_image_layout, + std::vector *mem_accesses); bool PreCallValidateCmdCopyBuffer(layer_data *device_data, GLOBAL_CB_NODE *cb_node, BUFFER_STATE *src_buffer_state, BUFFER_STATE *dst_buffer_state); diff --git a/layers/core_validation.cpp b/layers/core_validation.cpp index 08d8c06796..f820b787db 100644 --- a/layers/core_validation.cpp +++ b/layers/core_validation.cpp @@ -5648,47 +5648,6 @@ static void MarkStoreImagesAndBuffersAsWritten(layer_data *dev_data, GLOBAL_CB_N } } -// The first arg is an existing memory access that hasn't been verified by a synch object -// Check if the 2nd access conflicts with the first and return true if there's a conflict -// Pre: Both accesses must occur on the same VkDeviceMemory object -static bool MemoryConflict(MemoryAccess *initial_access, MemoryAccess *second_access) { - assert(initial_access->location.mem == second_access->location.mem); - // read/read is ok - // TODO: What to do for write/write? Warn? Nothing for now. Just allow RaR & WaW cases. - if (initial_access->write == second_access->write) return false; - // RaW & WaR can present conflicts - // If the second access is outside of the range of initial access, then no conflict - if ((initial_access->location.size != VK_WHOLE_SIZE && second_access->location.size != VK_WHOLE_SIZE) && - ((second_access->location.offset + second_access->location.size) <= initial_access->location.offset) && - (second_access->location.offset >= (initial_access->location.offset + initial_access->location.size))) { - return false; - } - // Without appropriate barrier we'll have a conflict - if (initial_access->mem_barrier || (initial_access->pipe_barrier && second_access->write)) return false; - // there's some amount of overlap in the accesses so flag conflict - return true; -} - -// Check memory accesses in cmd buffer up to this point and flag any conflicts -static bool ValidateMemoryAccess(layer_data *dev_data, GLOBAL_CB_NODE *cb_state, MemoryAccess *mem_access, const char *caller) { - bool skip = false; - auto mem_obj = mem_access->location.mem; - auto mem_access_pair = cb_state->memory_accesses.find(mem_obj); - if (mem_access_pair != cb_state->memory_accesses.end()) { - for (auto earlier_access : mem_access_pair->second) { - if (MemoryConflict(&earlier_access, mem_access)) { - const char *access1 = earlier_access.write ? "write" : "read"; - const char *access2 = mem_access->write ? "write" : "read"; - skip |= log_msg(dev_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_DEVICE_MEMORY_EXT, - HandleToUint64(mem_access->location.mem), 0, MEMTRACK_SYNCHRONIZATION_ERROR, "DS", - "%s called on VkCommandBuffer 0x%" PRIxLEAST64 " causes a %s after %s conflict.", caller, - HandleToUint64(cb_state->commandBuffer), access2, access1); - } - } - } - return skip; -} - // Generic function to handle validation for all CmdDraw* type functions static bool ValidateCmdDrawType(layer_data *dev_data, VkCommandBuffer cmd_buffer, bool indexed, VkPipelineBindPoint bind_point, CMD_TYPE cmd_type, GLOBAL_CB_NODE **cb_state, const char *caller, VkQueueFlags queue_flags, @@ -5780,29 +5739,22 @@ static bool PreCallValidateCmdDrawIndirect(layer_data *dev_data, VkCommandBuffer *buffer_state = GetBufferState(dev_data, buffer); skip |= ValidateMemoryIsBoundToBuffer(dev_data, *buffer_state, caller, VALIDATION_ERROR_1aa003b4); // TODO: This is temp code to test specific case that needs to be generalized for memory dependency checks - MemoryAccess mem_access; - mem_access.write = false; - auto mem_obj = (*buffer_state)->binding.mem; - mem_access.location.mem = mem_obj; - mem_access.location.offset = offset; + // First add mem read for indirect buffer // make sure size is non-zero for count of 1 auto size = stride * (count-1) + sizeof(VkDrawIndirectCommand); - mem_access.location.size = size; - mem_access.src_stage_flags = VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_DRAW_INDIRECT_BIT; - mem_access.src_access_flags = VK_ACCESS_INDIRECT_COMMAND_READ_BIT | VK_ACCESS_MEMORY_READ_BIT; - mem_accesses->emplace_back(mem_access); - skip |= ValidateMemoryAccess(dev_data, *cb_state, &mem_access, caller); + MemoryAccess mem_access((*buffer_state)->binding.mem, offset, size, true); + AddReadMemoryAccess(CMD_DRAWINDIRECT, mem_accesses, &mem_access); + // TODO: Need a special draw mem access call here (or in existing shared function) that analyzes state and adds related R/W + // accesses + skip |= ValidateMemoryAccesses(dev_data->report_data, *cb_state, mem_accesses, caller); // TODO: If the drawIndirectFirstInstance feature is not enabled, all the firstInstance members of the // VkDrawIndirectCommand structures accessed by this command must be 0, which will require access to the contents of 'buffer'. return skip; } -static void PostCallRecordCmdDrawIndirect(layer_data *dev_data, GLOBAL_CB_NODE *cb_state, VkPipelineBindPoint bind_point, - BUFFER_STATE *buffer_state, std::vector *mem_accesses) { - UpdateStateCmdDrawType(dev_data, cb_state, bind_point); - AddCommandBufferBindingBuffer(dev_data, cb_state, buffer_state); - // TODO: This is temporary to test out potential synch design - cb_state->commands.emplace_back(unique_ptr(new Command(CMD_DRAWINDIRECT))); +// Add the given cmd and its mem_accesses to the command buffer +static void AddCommandBufferCommandMemoryAccesses(GLOBAL_CB_NODE *cb_state, CMD_TYPE cmd, std::vector *mem_accesses) { + cb_state->commands.emplace_back(unique_ptr(new Command(cmd))); Command *cmd_ptr = cb_state->commands.back().get(); for (auto mem_access : *mem_accesses) { mem_access.cmd = cmd_ptr; @@ -5811,6 +5763,14 @@ static void PostCallRecordCmdDrawIndirect(layer_data *dev_data, GLOBAL_CB_NODE * } } +static void PostCallRecordCmdDrawIndirect(layer_data *dev_data, GLOBAL_CB_NODE *cb_state, VkPipelineBindPoint bind_point, + BUFFER_STATE *buffer_state, std::vector *mem_accesses) { + UpdateStateCmdDrawType(dev_data, cb_state, bind_point); + AddCommandBufferBindingBuffer(dev_data, cb_state, buffer_state); + // TODO: MemoryAccess can be merged with memory binding tracking + AddCommandBufferCommandMemoryAccesses(cb_state, CMD_DRAWINDIRECT, mem_accesses); +} + VKAPI_ATTR void VKAPI_CALL CmdDrawIndirect(VkCommandBuffer commandBuffer, VkBuffer buffer, VkDeviceSize offset, uint32_t count, uint32_t stride) { layer_data *dev_data = GetLayerDataPtr(get_dispatch_key(commandBuffer), layer_data_map); @@ -5951,6 +5911,7 @@ VKAPI_ATTR void VKAPI_CALL CmdCopyImage(VkCommandBuffer commandBuffer, VkImage s const VkImageCopy *pRegions) { bool skip = false; layer_data *device_data = GetLayerDataPtr(get_dispatch_key(commandBuffer), layer_data_map); + std::vector mem_accesses; unique_lock_t lock(global_lock); auto cb_node = GetCBNode(device_data, commandBuffer); @@ -5958,10 +5919,10 @@ VKAPI_ATTR void VKAPI_CALL CmdCopyImage(VkCommandBuffer commandBuffer, VkImage s auto dst_image_state = GetImageState(device_data, dstImage); if (cb_node && src_image_state && dst_image_state) { skip = PreCallValidateCmdCopyImage(device_data, cb_node, src_image_state, dst_image_state, regionCount, pRegions, - srcImageLayout, dstImageLayout); + srcImageLayout, dstImageLayout, &mem_accesses); if (!skip) { PreCallRecordCmdCopyImage(device_data, cb_node, src_image_state, dst_image_state, regionCount, pRegions, srcImageLayout, - dstImageLayout); + dstImageLayout, &mem_accesses); lock.unlock(); device_data->dispatch_table.CmdCopyImage(commandBuffer, srcImage, srcImageLayout, dstImage, dstImageLayout, regionCount, pRegions); @@ -6056,21 +6017,32 @@ VKAPI_ATTR void VKAPI_CALL CmdCopyImageToBuffer(VkCommandBuffer commandBuffer, V } } -static bool PreCallCmdUpdateBuffer(layer_data *device_data, const GLOBAL_CB_NODE *cb_state, const BUFFER_STATE *dst_buffer_state) { +static bool PreCallCmdUpdateBuffer(layer_data *device_data, VkCommandBuffer commandBuffer, VkBuffer dstBuffer, VkDeviceSize offset, + VkDeviceSize size, GLOBAL_CB_NODE **cb_state, BUFFER_STATE **dst_buffer_state, + std::vector *mem_accesses) { bool skip = false; - skip |= ValidateMemoryIsBoundToBuffer(device_data, dst_buffer_state, "vkCmdUpdateBuffer()", VALIDATION_ERROR_1e400046); + *cb_state = GetCBNode(device_data, commandBuffer); + assert(*cb_state); + *dst_buffer_state = GetBufferState(device_data, dstBuffer); + assert(*dst_buffer_state); + skip |= ValidateMemoryIsBoundToBuffer(device_data, *dst_buffer_state, "vkCmdUpdateBuffer()", VALIDATION_ERROR_1e400046); // Validate that DST buffer has correct usage flags set - skip |= ValidateBufferUsageFlags(device_data, dst_buffer_state, VK_BUFFER_USAGE_TRANSFER_DST_BIT, true, + skip |= ValidateBufferUsageFlags(device_data, *dst_buffer_state, VK_BUFFER_USAGE_TRANSFER_DST_BIT, true, VALIDATION_ERROR_1e400044, "vkCmdUpdateBuffer()", "VK_BUFFER_USAGE_TRANSFER_DST_BIT"); - skip |= ValidateCmdQueueFlags(device_data, cb_state, "vkCmdUpdateBuffer()", + skip |= ValidateCmdQueueFlags(device_data, *cb_state, "vkCmdUpdateBuffer()", VK_QUEUE_TRANSFER_BIT | VK_QUEUE_GRAPHICS_BIT | VK_QUEUE_COMPUTE_BIT, VALIDATION_ERROR_1e402415); - skip |= ValidateCmd(device_data, cb_state, CMD_UPDATEBUFFER, "vkCmdUpdateBuffer()"); - skip |= insideRenderPass(device_data, cb_state, "vkCmdUpdateBuffer()", VALIDATION_ERROR_1e400017); + skip |= ValidateCmd(device_data, *cb_state, CMD_UPDATEBUFFER, "vkCmdUpdateBuffer()"); + skip |= insideRenderPass(device_data, *cb_state, "vkCmdUpdateBuffer()", VALIDATION_ERROR_1e400017); + // Add mem access for writing buffer + auto buff_binding = (*dst_buffer_state)->binding; + MemoryAccess mem_access(buff_binding.mem, offset, size, true); + AddWriteMemoryAccess(CMD_UPDATEBUFFER, mem_accesses, &mem_access); + skip |= ValidateMemoryAccesses(device_data->report_data, *cb_state, mem_accesses, "vkCmdUpdateBuffer()"); return skip; } static void PostCallRecordCmdUpdateBuffer(layer_data *device_data, GLOBAL_CB_NODE *cb_state, BUFFER_STATE *dst_buffer_state, - VkDeviceSize offset, VkDeviceSize data_size) { + std::vector *mem_accesses) { // Update bindings between buffer and cmd buffer AddCommandBufferBindingBuffer(device_data, cb_state, dst_buffer_state); std::function function = [=]() { @@ -6078,38 +6050,24 @@ static void PostCallRecordCmdUpdateBuffer(layer_data *device_data, GLOBAL_CB_NOD return false; }; cb_state->queue_submit_functions.push_back(function); - // TODO: This is simple hack to test a single case, need to generalize for all cmds - cb_state->commands.emplace_back(unique_ptr(new Command(CMD_UPDATEBUFFER))); - Command *cmd_ptr = cb_state->commands.back().get(); - MemoryAccess mem_access; - mem_access.write = true; - auto mem_obj = dst_buffer_state->binding.mem; - mem_access.location.mem = mem_obj; - mem_access.location.offset = offset; - mem_access.location.size = data_size; - mem_access.cmd = cmd_ptr; - mem_access.src_stage_flags = VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_TRANSFER_BIT; - mem_access.src_access_flags = VK_ACCESS_TRANSFER_WRITE_BIT | VK_ACCESS_MEMORY_WRITE_BIT; - cmd_ptr->AddMemoryAccess(mem_access); - cb_state->memory_accesses[mem_obj].push_back(mem_access); + AddCommandBufferCommandMemoryAccesses(cb_state, CMD_UPDATEBUFFER, mem_accesses); } VKAPI_ATTR void VKAPI_CALL CmdUpdateBuffer(VkCommandBuffer commandBuffer, VkBuffer dstBuffer, VkDeviceSize dstOffset, VkDeviceSize dataSize, const uint32_t *pData) { bool skip = false; layer_data *dev_data = GetLayerDataPtr(get_dispatch_key(commandBuffer), layer_data_map); + GLOBAL_CB_NODE *cb_state = nullptr; + BUFFER_STATE *dst_buff_state = nullptr; + std::vector mem_accesses; unique_lock_t lock(global_lock); - - auto cb_state = GetCBNode(dev_data, commandBuffer); - assert(cb_state); - auto dst_buff_state = GetBufferState(dev_data, dstBuffer); - assert(dst_buff_state); - skip |= PreCallCmdUpdateBuffer(dev_data, cb_state, dst_buff_state); + skip |= + PreCallCmdUpdateBuffer(dev_data, commandBuffer, dstBuffer, dstOffset, dataSize, &cb_state, &dst_buff_state, &mem_accesses); lock.unlock(); if (!skip) { dev_data->dispatch_table.CmdUpdateBuffer(commandBuffer, dstBuffer, dstOffset, dataSize, pData); lock.lock(); - PostCallRecordCmdUpdateBuffer(dev_data, cb_state, dst_buff_state, dstOffset, dataSize); + PostCallRecordCmdUpdateBuffer(dev_data, cb_state, dst_buff_state, &mem_accesses); lock.unlock(); } } diff --git a/layers/core_validation_types.h b/layers/core_validation_types.h index 3e62cfb4ae..07062ad876 100644 --- a/layers/core_validation_types.h +++ b/layers/core_validation_types.h @@ -413,7 +413,6 @@ enum CMD_TYPE { CMD_BLITIMAGE, CMD_COPYBUFFERTOIMAGE, CMD_COPYIMAGETOBUFFER, - CMD_CLONEIMAGEDATA, CMD_UPDATEBUFFER, CMD_FILLBUFFER, CMD_CLEARCOLORIMAGE, @@ -438,6 +437,158 @@ enum CMD_TYPE { CMD_ENDRENDERPASS, CMD_EXECUTECOMMANDS, CMD_END, // Should be last command in any RECORDED cmd buffer + CMD_COUNT +}; + +// Struct for mapping a cmd to its permanent default pipe/access flags +// Draw/Dispatch/ClearAttachments commands will also add in some flags dynamically based on current state +struct CmdFlags { + VkPipelineStageFlags stage_flags; + VkAccessFlags access_flags; +}; + +// Per-cmd read/write flags Read flags in slot 0, Write in 1 +static const CmdFlags CommandToFlags[CMD_COUNT][2] = { + // CMD_NONE, + {{0, 0}, {0, 0}}, + // CMD_BINDPIPELINE, + {{0, 0}, {0, 0}}, + // CMD_BINDPIPELINEDELTA, + {{0, 0}, {0, 0}}, + // CMD_SETVIEWPORTSTATE, + {{0, 0}, {0, 0}}, + // CMD_SETSCISSORSTATE, + {{0, 0}, {0, 0}}, + // CMD_SETLINEWIDTHSTATE, + {{0, 0}, {0, 0}}, + // CMD_SETDEPTHBIASSTATE, + {{0, 0}, {0, 0}}, + // CMD_SETBLENDSTATE, + {{0, 0}, {0, 0}}, + // CMD_SETDEPTHBOUNDSSTATE, + {{0, 0}, {0, 0}}, + // CMD_SETSTENCILREADMASKSTATE, + {{0, 0}, {0, 0}}, + // CMD_SETSTENCILWRITEMASKSTATE, + {{0, 0}, {0, 0}}, + // CMD_SETSTENCILREFERENCESTATE, + {{0, 0}, {0, 0}}, + // CMD_BINDDESCRIPTORSETS, + {{0, 0}, {0, 0}}, + // CMD_BINDINDEXBUFFER, + {{0, 0}, {0, 0}}, + // CMD_BINDVERTEXBUFFER, + {{0, 0}, {0, 0}}, + // CMD_DRAW, + {{VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_ALL_GRAPHICS_BIT | VK_PIPELINE_STAGE_VERTEX_INPUT_BIT | + VK_PIPELINE_STAGE_VERTEX_SHADER_BIT, + VK_ACCESS_MEMORY_READ_BIT | VK_ACCESS_VERTEX_ATTRIBUTE_READ_BIT | VK_ACCESS_SHADER_READ_BIT}, + {VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_ALL_GRAPHICS_BIT, 0}}, + // CMD_DRAWINDEXED, + {{VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_ALL_GRAPHICS_BIT | VK_PIPELINE_STAGE_VERTEX_INPUT_BIT | + VK_PIPELINE_STAGE_VERTEX_SHADER_BIT, + VK_ACCESS_MEMORY_READ_BIT | VK_ACCESS_INDEX_READ_BIT | VK_ACCESS_VERTEX_ATTRIBUTE_READ_BIT | VK_ACCESS_SHADER_READ_BIT}, + {VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_ALL_GRAPHICS_BIT, 0}}, + // CMD_DRAWINDIRECT, + {{VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_ALL_GRAPHICS_BIT | VK_PIPELINE_STAGE_VERTEX_INPUT_BIT | + VK_PIPELINE_STAGE_VERTEX_SHADER_BIT, + VK_ACCESS_MEMORY_READ_BIT | VK_ACCESS_INDIRECT_COMMAND_READ_BIT | VK_ACCESS_VERTEX_ATTRIBUTE_READ_BIT | + VK_ACCESS_SHADER_READ_BIT}, + {VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_ALL_GRAPHICS_BIT, 0}}, + // CMD_DRAWINDEXEDINDIRECT, + {{VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_ALL_GRAPHICS_BIT | VK_PIPELINE_STAGE_VERTEX_INPUT_BIT | + VK_PIPELINE_STAGE_VERTEX_SHADER_BIT, + VK_ACCESS_MEMORY_READ_BIT | VK_ACCESS_INDEX_READ_BIT | VK_ACCESS_INDIRECT_COMMAND_READ_BIT | + VK_ACCESS_VERTEX_ATTRIBUTE_READ_BIT | VK_ACCESS_SHADER_READ_BIT}, + {VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_ALL_GRAPHICS_BIT, 0}}, + // CMD_DISPATCH, + {{VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_COMPUTE_SHADER_BIT, + VK_ACCESS_MEMORY_READ_BIT | VK_ACCESS_SHADER_READ_BIT}, + {VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_COMPUTE_SHADER_BIT, 0}}, + // CMD_DISPATCHINDIRECT, + {{VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_COMPUTE_SHADER_BIT, + VK_ACCESS_MEMORY_READ_BIT | VK_ACCESS_SHADER_READ_BIT | VK_ACCESS_INDIRECT_COMMAND_READ_BIT}, + {VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_COMPUTE_SHADER_BIT, 0}}, + // CMD_COPYBUFFER, + {{VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_TRANSFER_BIT, VK_ACCESS_MEMORY_READ_BIT | VK_ACCESS_TRANSFER_READ_BIT}, + {VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_TRANSFER_BIT, + VK_ACCESS_MEMORY_WRITE_BIT | VK_ACCESS_TRANSFER_WRITE_BIT}}, + // CMD_COPYIMAGE, + {{VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_TRANSFER_BIT, VK_ACCESS_MEMORY_READ_BIT | VK_ACCESS_TRANSFER_READ_BIT}, + {VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_TRANSFER_BIT, + VK_ACCESS_MEMORY_WRITE_BIT | VK_ACCESS_TRANSFER_WRITE_BIT}}, + // CMD_BLITIMAGE, + {{VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_TRANSFER_BIT, VK_ACCESS_MEMORY_READ_BIT | VK_ACCESS_TRANSFER_READ_BIT}, + {VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_TRANSFER_BIT, + VK_ACCESS_MEMORY_WRITE_BIT | VK_ACCESS_TRANSFER_WRITE_BIT}}, + // CMD_COPYBUFFERTOIMAGE, + {{VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_TRANSFER_BIT, VK_ACCESS_MEMORY_READ_BIT | VK_ACCESS_TRANSFER_READ_BIT}, + {VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_TRANSFER_BIT, + VK_ACCESS_MEMORY_WRITE_BIT | VK_ACCESS_TRANSFER_WRITE_BIT}}, + // CMD_COPYIMAGETOBUFFER, + {{VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_TRANSFER_BIT, VK_ACCESS_MEMORY_READ_BIT | VK_ACCESS_TRANSFER_READ_BIT}, + {VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_TRANSFER_BIT, + VK_ACCESS_MEMORY_WRITE_BIT | VK_ACCESS_TRANSFER_WRITE_BIT}}, + // CMD_UPDATEBUFFER, + {{0, 0}, + {VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_TRANSFER_BIT, + VK_ACCESS_MEMORY_WRITE_BIT | VK_ACCESS_TRANSFER_WRITE_BIT}}, + // CMD_FILLBUFFER, + {{0, 0}, + {VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_TRANSFER_BIT, + VK_ACCESS_MEMORY_WRITE_BIT | VK_ACCESS_TRANSFER_WRITE_BIT}}, + // CMD_CLEARCOLORIMAGE, + {{0, 0}, + {VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_TRANSFER_BIT, + VK_ACCESS_MEMORY_WRITE_BIT | VK_ACCESS_TRANSFER_WRITE_BIT}}, + // CMD_CLEARATTACHMENTS, + {{0, 0}, {VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_ALL_GRAPHICS_BIT, 0}}, + // CMD_CLEARDEPTHSTENCILIMAGE, + {{0, 0}, + {VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_TRANSFER_BIT, + VK_ACCESS_MEMORY_WRITE_BIT | VK_ACCESS_TRANSFER_WRITE_BIT}}, + // CMD_RESOLVEIMAGE, + {{VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_TRANSFER_BIT, VK_ACCESS_MEMORY_READ_BIT | VK_ACCESS_TRANSFER_READ_BIT}, + {VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_TRANSFER_BIT, + VK_ACCESS_MEMORY_WRITE_BIT | VK_ACCESS_TRANSFER_WRITE_BIT}}, + // CMD_SETEVENT, + {{0, 0}, {0, 0}}, + // CMD_RESETEVENT, + {{0, 0}, {0, 0}}, + // CMD_WAITEVENTS, + {{0, 0}, {0, 0}}, + // CMD_PIPELINEBARRIER, + {{0, 0}, {0, 0}}, + // CMD_BEGINQUERY, + {{0, 0}, {0, 0}}, + // CMD_ENDQUERY, + {{0, 0}, {0, 0}}, + // CMD_RESETQUERYPOOL, + {{0, 0}, {0, 0}}, + // CMD_COPYQUERYPOOLRESULTS, + {{0, 0}, + {VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_TRANSFER_BIT, + VK_ACCESS_MEMORY_WRITE_BIT | VK_ACCESS_TRANSFER_WRITE_BIT}}, + // CMD_WRITETIMESTAMP, + {{0, 0}, {0, 0}}, + // CMD_PUSHCONSTANTS, + {{0, 0}, {0, 0}}, + // CMD_INITATOMICCOUNTERS, + {{0, 0}, {0, 0}}, + // CMD_LOADATOMICCOUNTERS, + {{0, 0}, {0, 0}}, + // CMD_SAVEATOMICCOUNTERS, + {{0, 0}, {0, 0}}, + // CMD_BEGINRENDERPASS, + {{VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_ALL_GRAPHICS_BIT, 0}, {0, 0}}, + // CMD_NEXTSUBPASS, + {{0, 0}, {0, 0}}, + // CMD_ENDRENDERPASS, + {{0, 0}, {0, 0}}, + // CMD_EXECUTECOMMANDS, + {{0, 0}, {0, 0}}, + // CMD_END, // Should be last command in any RECORDED cmd buffer + {{0, 0}, {0, 0}}, }; // fwd decl class for ptr below @@ -445,7 +596,8 @@ class Command; // Store details of memory access by a cmd struct MemoryAccess { MemoryAccess() - : write(false), + : precise(false), + write(false), cmd(nullptr), location{0, 0, 0}, src_stage_flags(0), @@ -455,6 +607,19 @@ struct MemoryAccess { mem_barrier(false), pipe_barrier(false), synch_commands{} {}; + MemoryAccess(VkDeviceMemory mem, VkDeviceSize offset, VkDeviceSize size, bool precise) + : precise(precise), + write(false), + cmd(nullptr), + location{mem, offset, size}, + src_stage_flags(0), + src_access_flags(0), + dst_stage_flags(0), + dst_access_flags(0), + mem_barrier(false), + pipe_barrier(false), + synch_commands{} {}; + bool precise; // True if exact bounds of mem access are known, false if bounds are conservative bool write; // False for read access Command *cmd; // Ptr to cmd that makes this access MEM_BINDING location; // mem/offset/size being accessed From c4ef0a6cbc80ac82c808ec95b2acf473e81a9d35 Mon Sep 17 00:00:00 2001 From: Tobin Ehlis Date: Fri, 4 Aug 2017 16:23:56 -0600 Subject: [PATCH 05/28] layers:Add mem access for CmdCopyBuffer Integrate CmdCopyBuffer into MemoryAccess recording and validation framework. Refactored recording of read/write memory accesses to use the binding which saves some repeated lines of code. --- layers/buffer_validation.cpp | 84 +++++++++++++++++++++++++----------- layers/buffer_validation.h | 11 +++-- layers/core_validation.cpp | 24 +++-------- 3 files changed, 72 insertions(+), 47 deletions(-) diff --git a/layers/buffer_validation.cpp b/layers/buffer_validation.cpp index 78ebc7d34a..209b38c77c 100644 --- a/layers/buffer_validation.cpp +++ b/layers/buffer_validation.cpp @@ -645,12 +645,16 @@ void AddMemoryAccess(CMD_TYPE cmd, std::vector *mem_accesses, Memo mem_accesses->emplace_back(*mem_access); } -void AddReadMemoryAccess(CMD_TYPE cmd, std::vector *mem_accesses, MemoryAccess *mem_access) { - AddMemoryAccess(cmd, mem_accesses, mem_access, false, 0); +// Create read memory access for given binding and add to mem_accesses +void AddReadMemoryAccess(CMD_TYPE cmd, std::vector *mem_accesses, MEM_BINDING const &binding, bool precise) { + MemoryAccess mem_access(binding.mem, binding.offset, binding.size, precise); + AddMemoryAccess(cmd, mem_accesses, &mem_access, false, 0); } -void AddWriteMemoryAccess(CMD_TYPE cmd, std::vector *mem_accesses, MemoryAccess *mem_access) { - AddMemoryAccess(cmd, mem_accesses, mem_access, true, 1); +// Create write memory access for given binding and add to mem_accesses +void AddWriteMemoryAccess(CMD_TYPE cmd, std::vector *mem_accesses, MEM_BINDING const &binding, bool precise) { + MemoryAccess mem_access(binding.mem, binding.offset, binding.size, precise); + AddMemoryAccess(cmd, mem_accesses, &mem_access, true, 1); } // The first arg is an existing memory access that hasn't been verified by a synch object @@ -668,8 +672,19 @@ bool MemoryConflict(MemoryAccess const *initial_access, MemoryAccess const *seco (second_access->location.offset >= (initial_access->location.offset + initial_access->location.size))) { return false; } - // Without appropriate barrier we'll have a conflict - if (initial_access->mem_barrier || (initial_access->pipe_barrier && second_access->write)) return false; + // Without appropriate barrier & appropriate scope overlap we'll have a conflict + if (initial_access->mem_barrier) { + // If the initial dst masks match second access src masks, scopes are good + if ((0 != (initial_access->dst_stage_flags & second_access->src_stage_flags)) && + (0 != (initial_access->dst_access_flags & second_access->src_access_flags))) { + return false; + } + } else if (initial_access->pipe_barrier && second_access->write) { + // Just need an execution dep here so check pipe masks + if (0 != (initial_access->dst_stage_flags & second_access->src_stage_flags)) { + return false; + } + } // there's some amount of overlap in the accesses so flag conflict return true; } @@ -691,8 +706,10 @@ bool ValidateMemoryAccesses(debug_report_data const *report_data, GLOBAL_CB_NODE if (earlier_access.precise && mem_access.precise) level = VK_DEBUG_REPORT_ERROR_BIT_EXT; skip |= log_msg(report_data, level, VK_DEBUG_REPORT_OBJECT_TYPE_DEVICE_MEMORY_EXT, HandleToUint64(mem_access.location.mem), 0, MEMTRACK_SYNCHRONIZATION_ERROR, "DS", - "%s called on VkCommandBuffer 0x%" PRIxLEAST64 " causes a %s after %s conflict.", caller, - HandleToUint64(cb_state->commandBuffer), access2, access1); + "%s called on VkCommandBuffer 0x%" PRIxLEAST64 + " causes a %s after %s conflict with memory object 0x%" PRIxLEAST64 ".", + caller, HandleToUint64(cb_state->commandBuffer), access2, access1, + HandleToUint64(mem_access.location.mem)); } } } @@ -700,6 +717,17 @@ bool ValidateMemoryAccesses(debug_report_data const *report_data, GLOBAL_CB_NODE return skip; } +// Add the given cmd and its mem_accesses to the command buffer +void AddCommandBufferCommandMemoryAccesses(GLOBAL_CB_NODE *cb_state, CMD_TYPE cmd, std::vector *mem_accesses) { + cb_state->commands.emplace_back(std::unique_ptr(new Command(cmd))); + Command *cmd_ptr = cb_state->commands.back().get(); + for (auto &mem_access : *mem_accesses) { + mem_access.cmd = cmd_ptr; + cmd_ptr->AddMemoryAccess(mem_access); + cb_state->memory_accesses[mem_access.location.mem].push_back(mem_access); + } +} + bool PreCallValidateCreateImage(layer_data *device_data, const VkImageCreateInfo *pCreateInfo, const VkAllocationCallbacks *pAllocator, VkImage *pImage) { bool skip = false; @@ -1968,12 +1996,8 @@ bool PreCallValidateCmdCopyImage(layer_data *device_data, GLOBAL_CB_NODE *cb_sta } // Add memory access for this image copy // TODO: Currently treating all image accesses as imprecise & covering the whole image area - const auto &src_binding = src_image_state->binding; - MemoryAccess src_mem_access(src_binding.mem, src_binding.offset, src_binding.size, false); - AddReadMemoryAccess(CMD_COPYIMAGE, mem_accesses, &src_mem_access); - const auto &dst_binding = dst_image_state->binding; - MemoryAccess dst_mem_access(dst_binding.mem, dst_binding.offset, dst_binding.size, false); - AddWriteMemoryAccess(CMD_COPYIMAGE, mem_accesses, &dst_mem_access); + AddReadMemoryAccess(CMD_COPYIMAGE, mem_accesses, src_image_state->binding, false); + AddWriteMemoryAccess(CMD_COPYIMAGE, mem_accesses, dst_image_state->binding, false); skip |= ValidateMemoryAccesses(report_data, cb_state, mem_accesses, "vkCmdCopyImage()"); } @@ -3536,8 +3560,9 @@ void PostCallRecordCreateImageView(layer_data *device_data, const VkImageViewCre sub_res_range.layerCount = ResolveRemainingLayers(&sub_res_range, image_state->createInfo.arrayLayers); } -bool PreCallValidateCmdCopyBuffer(layer_data *device_data, GLOBAL_CB_NODE *cb_node, BUFFER_STATE *src_buffer_state, - BUFFER_STATE *dst_buffer_state) { +bool PreCallValidateCmdCopyBuffer(layer_data *device_data, GLOBAL_CB_NODE *cb_state, BUFFER_STATE *src_buffer_state, + BUFFER_STATE *dst_buffer_state, uint32_t region_count, const VkBufferCopy *regions, + std::vector *mem_accesses) { bool skip = false; skip |= ValidateMemoryIsBoundToBuffer(device_data, src_buffer_state, "vkCmdCopyBuffer()", VALIDATION_ERROR_18c000ee); skip |= ValidateMemoryIsBoundToBuffer(device_data, dst_buffer_state, "vkCmdCopyBuffer()", VALIDATION_ERROR_18c000f2); @@ -3546,28 +3571,37 @@ bool PreCallValidateCmdCopyBuffer(layer_data *device_data, GLOBAL_CB_NODE *cb_no VALIDATION_ERROR_18c000ec, "vkCmdCopyBuffer()", "VK_BUFFER_USAGE_TRANSFER_SRC_BIT"); skip |= ValidateBufferUsageFlags(device_data, dst_buffer_state, VK_BUFFER_USAGE_TRANSFER_DST_BIT, true, VALIDATION_ERROR_18c000f0, "vkCmdCopyBuffer()", "VK_BUFFER_USAGE_TRANSFER_DST_BIT"); - skip |= ValidateCmdQueueFlags(device_data, cb_node, "vkCmdCopyBuffer()", + skip |= ValidateCmdQueueFlags(device_data, cb_state, "vkCmdCopyBuffer()", VK_QUEUE_TRANSFER_BIT | VK_QUEUE_GRAPHICS_BIT | VK_QUEUE_COMPUTE_BIT, VALIDATION_ERROR_18c02415); - skip |= ValidateCmd(device_data, cb_node, CMD_COPYBUFFER, "vkCmdCopyBuffer()"); - skip |= insideRenderPass(device_data, cb_node, "vkCmdCopyBuffer()", VALIDATION_ERROR_18c00017); + skip |= ValidateCmd(device_data, cb_state, CMD_COPYBUFFER, "vkCmdCopyBuffer()"); + skip |= insideRenderPass(device_data, cb_state, "vkCmdCopyBuffer()", VALIDATION_ERROR_18c00017); + const auto src_mem_obj = src_buffer_state->binding.mem; + const auto dst_mem_obj = dst_buffer_state->binding.mem; + for (uint32_t i = 0; i < region_count; ++i) { + const auto ® = regions[i]; + AddReadMemoryAccess(CMD_COPYBUFFER, mem_accesses, {src_mem_obj, reg.srcOffset, reg.size}, true); + AddWriteMemoryAccess(CMD_COPYBUFFER, mem_accesses, {dst_mem_obj, reg.dstOffset, reg.size}, true); + } + skip |= ValidateMemoryAccesses(core_validation::GetReportData(device_data), cb_state, mem_accesses, "vkCmdCopyBuffer()"); return skip; } -void PreCallRecordCmdCopyBuffer(layer_data *device_data, GLOBAL_CB_NODE *cb_node, BUFFER_STATE *src_buffer_state, - BUFFER_STATE *dst_buffer_state) { +void PreCallRecordCmdCopyBuffer(layer_data *device_data, GLOBAL_CB_NODE *cb_state, BUFFER_STATE *src_buffer_state, + BUFFER_STATE *dst_buffer_state, std::vector *mem_accesses) { // Update bindings between buffers and cmd buffer - AddCommandBufferBindingBuffer(device_data, cb_node, src_buffer_state); - AddCommandBufferBindingBuffer(device_data, cb_node, dst_buffer_state); + AddCommandBufferBindingBuffer(device_data, cb_state, src_buffer_state); + AddCommandBufferBindingBuffer(device_data, cb_state, dst_buffer_state); std::function function = [=]() { return ValidateBufferMemoryIsValid(device_data, src_buffer_state, "vkCmdCopyBuffer()"); }; - cb_node->queue_submit_functions.push_back(function); + cb_state->queue_submit_functions.push_back(function); function = [=]() { SetBufferMemoryValid(device_data, dst_buffer_state, true); return false; }; - cb_node->queue_submit_functions.push_back(function); + cb_state->queue_submit_functions.push_back(function); + AddCommandBufferCommandMemoryAccesses(cb_state, CMD_COPYBUFFER, mem_accesses); } static bool validateIdleBuffer(layer_data *device_data, VkBuffer buffer) { diff --git a/layers/buffer_validation.h b/layers/buffer_validation.h index 3ec972c4c5..6cc0902d18 100644 --- a/layers/buffer_validation.h +++ b/layers/buffer_validation.h @@ -136,15 +136,17 @@ void TransitionFinalSubpassLayouts(layer_data *dev_data, GLOBAL_CB_NODE *pCB, co void AddMemoryAccess(CMD_TYPE cmd, std::vector *mem_accesses, MemoryAccess *mem_access, bool write, uint32_t rw_index); -void AddReadMemoryAccess(CMD_TYPE cmd, std::vector *mem_accesses, MemoryAccess *mem_access); +void AddReadMemoryAccess(CMD_TYPE cmd, std::vector *mem_accesses, MEM_BINDING const &binding, bool precise); -void AddWriteMemoryAccess(CMD_TYPE cmd, std::vector *mem_accesses, MemoryAccess *mem_access); +void AddWriteMemoryAccess(CMD_TYPE cmd, std::vector *mem_accesses, MEM_BINDING const &binding, bool precise); bool MemoryConflict(MemoryAccess const *initial_access, MemoryAccess const *second_access); bool ValidateMemoryAccesses(debug_report_data const *report_data, GLOBAL_CB_NODE const *cb_state, std::vector *mem_accesses, const char *caller); +void AddCommandBufferCommandMemoryAccesses(GLOBAL_CB_NODE *cb_state, CMD_TYPE cmd, std::vector *mem_accesses); + bool PreCallValidateCmdCopyImage(layer_data *device_data, GLOBAL_CB_NODE *cb_node, IMAGE_STATE *src_image_state, IMAGE_STATE *dst_image_state, uint32_t region_count, const VkImageCopy *regions, VkImageLayout src_image_layout, VkImageLayout dst_image_layout, @@ -226,10 +228,11 @@ void PreCallRecordCmdCopyImage(layer_data *device_data, GLOBAL_CB_NODE *cb_node, std::vector *mem_accesses); bool PreCallValidateCmdCopyBuffer(layer_data *device_data, GLOBAL_CB_NODE *cb_node, BUFFER_STATE *src_buffer_state, - BUFFER_STATE *dst_buffer_state); + BUFFER_STATE *dst_buffer_state, uint32_t region_count, const VkBufferCopy *regions, + std::vector *mem_accesses); void PreCallRecordCmdCopyBuffer(layer_data *device_data, GLOBAL_CB_NODE *cb_node, BUFFER_STATE *src_buffer_state, - BUFFER_STATE *dst_buffer_state); + BUFFER_STATE *dst_buffer_state, std::vector *mem_accesses); bool PreCallValidateDestroyImageView(layer_data *device_data, VkImageView image_view, IMAGE_VIEW_STATE **image_view_state, VK_OBJECT *obj_struct); diff --git a/layers/core_validation.cpp b/layers/core_validation.cpp index f820b787db..440ad0c5ca 100644 --- a/layers/core_validation.cpp +++ b/layers/core_validation.cpp @@ -5742,8 +5742,7 @@ static bool PreCallValidateCmdDrawIndirect(layer_data *dev_data, VkCommandBuffer // First add mem read for indirect buffer // make sure size is non-zero for count of 1 auto size = stride * (count-1) + sizeof(VkDrawIndirectCommand); - MemoryAccess mem_access((*buffer_state)->binding.mem, offset, size, true); - AddReadMemoryAccess(CMD_DRAWINDIRECT, mem_accesses, &mem_access); + AddReadMemoryAccess(CMD_DRAWINDIRECT, mem_accesses, {(*buffer_state)->binding.mem, offset, size}, false); // TODO: Need a special draw mem access call here (or in existing shared function) that analyzes state and adds related R/W // accesses skip |= ValidateMemoryAccesses(dev_data->report_data, *cb_state, mem_accesses, caller); @@ -5752,17 +5751,6 @@ static bool PreCallValidateCmdDrawIndirect(layer_data *dev_data, VkCommandBuffer return skip; } -// Add the given cmd and its mem_accesses to the command buffer -static void AddCommandBufferCommandMemoryAccesses(GLOBAL_CB_NODE *cb_state, CMD_TYPE cmd, std::vector *mem_accesses) { - cb_state->commands.emplace_back(unique_ptr(new Command(cmd))); - Command *cmd_ptr = cb_state->commands.back().get(); - for (auto mem_access : *mem_accesses) { - mem_access.cmd = cmd_ptr; - cmd_ptr->AddMemoryAccess(mem_access); - cb_state->memory_accesses[mem_access.location.mem].push_back(mem_access); - } -} - static void PostCallRecordCmdDrawIndirect(layer_data *dev_data, GLOBAL_CB_NODE *cb_state, VkPipelineBindPoint bind_point, BUFFER_STATE *buffer_state, std::vector *mem_accesses) { UpdateStateCmdDrawType(dev_data, cb_state, bind_point); @@ -5887,6 +5875,7 @@ VKAPI_ATTR void VKAPI_CALL CmdDispatchIndirect(VkCommandBuffer commandBuffer, Vk VKAPI_ATTR void VKAPI_CALL CmdCopyBuffer(VkCommandBuffer commandBuffer, VkBuffer srcBuffer, VkBuffer dstBuffer, uint32_t regionCount, const VkBufferCopy *pRegions) { layer_data *device_data = GetLayerDataPtr(get_dispatch_key(commandBuffer), layer_data_map); + std::vector mem_accesses; unique_lock_t lock(global_lock); auto cb_node = GetCBNode(device_data, commandBuffer); @@ -5894,9 +5883,10 @@ VKAPI_ATTR void VKAPI_CALL CmdCopyBuffer(VkCommandBuffer commandBuffer, VkBuffer auto dst_buffer_state = GetBufferState(device_data, dstBuffer); if (cb_node && src_buffer_state && dst_buffer_state) { - bool skip = PreCallValidateCmdCopyBuffer(device_data, cb_node, src_buffer_state, dst_buffer_state); + bool skip = PreCallValidateCmdCopyBuffer(device_data, cb_node, src_buffer_state, dst_buffer_state, regionCount, pRegions, + &mem_accesses); if (!skip) { - PreCallRecordCmdCopyBuffer(device_data, cb_node, src_buffer_state, dst_buffer_state); + PreCallRecordCmdCopyBuffer(device_data, cb_node, src_buffer_state, dst_buffer_state, &mem_accesses); lock.unlock(); device_data->dispatch_table.CmdCopyBuffer(commandBuffer, srcBuffer, dstBuffer, regionCount, pRegions); } @@ -6034,9 +6024,7 @@ static bool PreCallCmdUpdateBuffer(layer_data *device_data, VkCommandBuffer comm skip |= ValidateCmd(device_data, *cb_state, CMD_UPDATEBUFFER, "vkCmdUpdateBuffer()"); skip |= insideRenderPass(device_data, *cb_state, "vkCmdUpdateBuffer()", VALIDATION_ERROR_1e400017); // Add mem access for writing buffer - auto buff_binding = (*dst_buffer_state)->binding; - MemoryAccess mem_access(buff_binding.mem, offset, size, true); - AddWriteMemoryAccess(CMD_UPDATEBUFFER, mem_accesses, &mem_access); + AddWriteMemoryAccess(CMD_UPDATEBUFFER, mem_accesses, (*dst_buffer_state)->binding, true); skip |= ValidateMemoryAccesses(device_data->report_data, *cb_state, mem_accesses, "vkCmdUpdateBuffer()"); return skip; } From ab748c97fdbaefc905c384c9012b370c6f8b756c Mon Sep 17 00:00:00 2001 From: Tobin Ehlis Date: Mon, 7 Aug 2017 16:46:13 -0600 Subject: [PATCH 06/28] layers:Check for barrier dependency chains Update synchronization modeling to handle barrier dependency chains. Initially only handling CmdPipelineBarriers. Create separate synch_commands vector in cmd buffer for quick parsing of just synch commands. For any previous synch commands whose destination synch scope overlaps the current synch command's source synch scope, merge the 1st cmds dest mask into current commands src mask and pull previous barriers into the current synch command. Updated existing PipelineBarrier algorithm for global and buffer barriers to make sure that pipeline masks overlap and only check access masks in that case --- layers/core_validation.cpp | 52 ++++++++++++++++++++++++++-------- layers/core_validation_types.h | 13 +++++++++ 2 files changed, 53 insertions(+), 12 deletions(-) diff --git a/layers/core_validation.cpp b/layers/core_validation.cpp index 440ad0c5ca..439d58d58f 100644 --- a/layers/core_validation.cpp +++ b/layers/core_validation.cpp @@ -1726,6 +1726,7 @@ static void resetCB(layer_data *dev_data, const VkCommandBuffer cb) { pCB->activeSubpass = 0; pCB->broken_bindings.clear(); pCB->commands.clear(); + pCB->synch_commands.clear(); pCB->memory_accesses.clear(); pCB->waitedEvents.clear(); pCB->events.clear(); @@ -6876,21 +6877,48 @@ static bool PreCallValidateCmdPipelineBarrier(layer_data *device_data, GLOBAL_CB } static void PreCallRecordCmdPipelineBarrier(layer_data *device_data, GLOBAL_CB_NODE *cb_state, VkCommandBuffer commandBuffer, - VkPipelineStageFlags srcStageMask, VkPipelineStageFlags dstStageMask, + VkPipelineStageFlags src_stage_mask, VkPipelineStageFlags dst_stage_mask, VkDependencyFlags dependencyFlags, uint32_t mem_barrier_count, const VkMemoryBarrier *mem_barriers, uint32_t buffer_mem_barrier_count, const VkBufferMemoryBarrier *buffer_mem_barriers, uint32_t imageMemoryBarrierCount, const VkImageMemoryBarrier *pImageMemoryBarriers) { TransitionImageLayouts(device_data, commandBuffer, imageMemoryBarrierCount, pImageMemoryBarriers); - cb_state->commands.emplace_back(unique_ptr(new Command(CMD_PIPELINEBARRIER))); + cb_state->commands.emplace_back( + unique_ptr(new SynchCommand(CMD_PIPELINEBARRIER, src_stage_mask, dst_stage_mask))); Command *cmd_ptr = cb_state->commands.back().get(); + SynchCommand *synch_cmd_ptr = static_cast(cmd_ptr); + + // First thing to do is parse through any previous barriers. If this barriers srcMasks match with + // previous barriers destination masks, then incorporate the previous barrier srcMasks into our srcMask + // and merge in all of the previous barriers. This will account for dependency chains. + for (const auto &synch_cmd : cb_state->synch_commands) { + if (synch_cmd->dst_stage_flags & src_stage_mask) { + // scopes overlap so merge prev barrier masks into current masks + src_stage_mask |= synch_cmd->src_stage_flags; + synch_cmd_ptr->src_stage_flags = src_stage_mask; + dst_stage_mask |= synch_cmd->dst_stage_flags; + synch_cmd_ptr->dst_stage_flags = dst_stage_mask; + // Pull all previous overlapping barriers into this barrier + for (const auto &global_barrier : synch_cmd->global_barriers) { + synch_cmd_ptr->global_barriers.emplace_back(global_barrier); + } + for (const auto &buff_barrier : synch_cmd->buffer_barriers) { + synch_cmd_ptr->buffer_barriers.emplace_back(buff_barrier); + } + for (const auto &img_barrier : synch_cmd->image_barriers) { + synch_cmd_ptr->image_barriers.emplace_back(img_barrier); + } + } + } + // Now that we've merged previous synch commands, append this cmd to existing synch commands + cb_state->synch_commands.emplace_back(synch_cmd_ptr); // TODO: Make this barrier/access parsing smarter - if (mem_barrier_count) { - for (auto &mem_access_pair : cb_state->memory_accesses) { - // Global barriers affect all outstanding memory accesses that match access mask - // TODO: For now just flagging all accesses, but need to store mask w/ access and check it here - // against each individual global mem barrier - for (auto &mem_access : mem_access_pair.second) { + for (auto &mem_access_pair : cb_state->memory_accesses) { + for (auto &mem_access : mem_access_pair.second) { + if (mem_access.src_stage_flags & src_stage_mask) { + // Record any execution barrier overlaps + mem_access.pipe_barrier = true; + mem_access.dst_stage_flags |= dst_stage_mask; // For every global barrier that matches access mask, record the barrier for (uint32_t i = 0; i < mem_barrier_count; ++i) { const auto &mem_barrier = mem_barriers[i]; @@ -6920,14 +6948,14 @@ static void PreCallRecordCmdPipelineBarrier(layer_data *device_data, GLOBAL_CB_N const auto &mem_access_pair = cb_state->memory_accesses.find(mem_obj); if (mem_access_pair != cb_state->memory_accesses.end()) { for (auto &mem_access : mem_access_pair->second) { - // If the access or pipe masks overlap, then barrier applies - if (((mem_access.src_access_flags & buff_barrier.srcAccessMask) != 0) || - ((mem_access.src_stage_flags & srcStageMask) != 0)) { + // If the pipe & access masks overlap, then barrier applies + if (((mem_access.src_access_flags & buff_barrier.srcAccessMask) != 0) && + ((mem_access.src_stage_flags & src_stage_mask) != 0)) { // Set buff access write opposite of actual access so conflict is flagged on overlap barrier_access.write = !mem_access.write; if (MemoryConflict(&barrier_access, &mem_access)) { mem_access.mem_barrier = true; - mem_access.dst_stage_flags |= dstStageMask; + mem_access.dst_stage_flags |= dst_stage_mask; mem_access.dst_access_flags |= buff_barrier.dstAccessMask; mem_access.synch_commands.push_back(cmd_ptr); } diff --git a/layers/core_validation_types.h b/layers/core_validation_types.h index 07062ad876..91da1bab21 100644 --- a/layers/core_validation_types.h +++ b/layers/core_validation_types.h @@ -641,11 +641,23 @@ struct MemoryAccess { class Command { public: Command(CMD_TYPE type) : type(type){}; + virtual ~Command() {} void AddMemoryAccess(MemoryAccess access) { mem_accesses.push_back(access); }; CMD_TYPE type; std::vector mem_accesses; // vector of all mem accesses by this cmd }; +class SynchCommand : public Command { + public: + SynchCommand(CMD_TYPE type, VkPipelineStageFlags src_stage_flags, VkPipelineStageFlags dst_stage_flags) + : Command(type), src_stage_flags(src_stage_flags), dst_stage_flags(dst_stage_flags){}; + VkPipelineStageFlags src_stage_flags; + VkPipelineStageFlags dst_stage_flags; + std::vector global_barriers; + std::vector buffer_barriers; + std::vector image_barriers; +}; + enum CB_STATE { CB_NEW, // Newly created CB w/o any cmds CB_RECORDING, // BeginCB has been called on this CB @@ -871,6 +883,7 @@ struct GLOBAL_CB_NODE : public BASE_NODE { std::vector broken_bindings; // std::vector> commands; // Commands in this command buffer + std::vector synch_commands; // Synch Commands in this command buffer // Store all memory accesses per memory object made in this command buffer std::unordered_map> memory_accesses; From 15c294a73cde2b8f91b913b4d07c565b782032d7 Mon Sep 17 00:00:00 2001 From: Tobin Ehlis Date: Tue, 8 Aug 2017 14:52:24 -0600 Subject: [PATCH 07/28] tests:UpdateBufferWaRDependencyWithTwoBarrierChain Adding positive test UpdateBufferWaRDependencyWithTwoBarrierChain. This features a WaR dependency that's cleared by a chain of two pipeline barriers. This confirms that validation correctly supports the merging of overlapping barriers to correctly account for synchronization chains. --- tests/layer_validation_tests.cpp | 38 ++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/tests/layer_validation_tests.cpp b/tests/layer_validation_tests.cpp index ea0d2afd45..77700bf9d6 100644 --- a/tests/layer_validation_tests.cpp +++ b/tests/layer_validation_tests.cpp @@ -10258,6 +10258,44 @@ TEST_F(VkPositiveLayerTest, UpdateBufferRaWDependencyWithBarrier) { vkDestroyPipelineLayout(m_device->device(), pipeline_layout, nullptr); } +TEST_F(VkPositiveLayerTest, UpdateBufferWaRDependencyWithTwoBarrierChain) { + TEST_DESCRIPTION("Update buffer used in CmdDrawIndirect w/ barrier before use"); + + m_errorMonitor->ExpectSuccess(); + ASSERT_NO_FATAL_FAILURE(Init()); + ASSERT_NO_FATAL_FAILURE(InitRenderTarget()); + + m_commandBuffer->begin(); + + VkDeviceSize buff_size = 1024; + + VkMemoryPropertyFlags reqs = VK_MEMORY_HEAP_DEVICE_LOCAL_BIT; + vk_testing::Buffer src_buffer; + src_buffer.init_as_src_and_dst(*m_device, buff_size, reqs); + + reqs = VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT; + vk_testing::Buffer dst_buffer; + dst_buffer.init_as_dst(*m_device, buff_size, reqs); + + VkBufferCopy buff_copy = {0, // srcOffset + 0, // dstOffset + buff_size}; + // Copy src to dst + vkCmdCopyBuffer(m_commandBuffer->handle(), src_buffer.handle(), dst_buffer.handle(), 1, &buff_copy); + + // Create an execution barrier chain (no mem barrier needed for WaR dependency) + vkCmdPipelineBarrier(m_commandBuffer->handle(), VK_PIPELINE_STAGE_TRANSFER_BIT, VK_PIPELINE_STAGE_COMPUTE_SHADER_BIT, 0, 0, + nullptr, 0, nullptr, 0, nullptr); + // Now 2nd global barrier introduces dependency chain with first barrier that should handle following WaR dependency + vkCmdPipelineBarrier(m_commandBuffer->handle(), VK_PIPELINE_STAGE_COMPUTE_SHADER_BIT, VK_PIPELINE_STAGE_TRANSFER_BIT, 0, 0, + nullptr, 0, nullptr, 0, nullptr); + // Now write into dst buffer that was read above + uint32_t Data[] = {1, 2, 3, 4, 5, 6, 7, 8}; + VkDeviceSize dataSize = sizeof(Data) / sizeof(uint32_t); + vkCmdUpdateBuffer(m_commandBuffer->handle(), src_buffer.handle(), 0, dataSize, &Data); + m_errorMonitor->VerifyNotFound(); +} + TEST_F(VkLayerTest, UpdateBufferRaWDependencyMissingBarrier) { TEST_DESCRIPTION("Update buffer used in CmdDrawIndirect without a barrier before use"); From 41677d8d89d4ab6fc3c0982498198c00663a1600 Mon Sep 17 00:00:00 2001 From: Tobin Ehlis Date: Wed, 9 Aug 2017 14:09:31 -0600 Subject: [PATCH 08/28] layers:Add CmdBlitImage memory access tracking Add tracking for the memory accesses in CmdBlitImage. Initially just adding a read & write access for the src & dest image memory that conservatively references whole binding and is recorded as imprecise so that it only will cause warnings. --- layers/buffer_validation.cpp | 103 +++++++++++++++++++---------------- layers/buffer_validation.h | 5 +- layers/core_validation.cpp | 6 +- 3 files changed, 62 insertions(+), 52 deletions(-) diff --git a/layers/buffer_validation.cpp b/layers/buffer_validation.cpp index 209b38c77c..5fe4d95693 100644 --- a/layers/buffer_validation.cpp +++ b/layers/buffer_validation.cpp @@ -2291,12 +2291,13 @@ void PreCallRecordCmdResolveImage(layer_data *device_data, GLOBAL_CB_NODE *cb_no cb_node->queue_submit_functions.push_back(function); } -bool PreCallValidateCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_node, IMAGE_STATE *src_image_state, - IMAGE_STATE *dst_image_state, uint32_t regionCount, const VkImageBlit *pRegions, VkFilter filter) { +bool PreCallValidateCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_state, IMAGE_STATE *src_image_state, + IMAGE_STATE *dst_image_state, uint32_t regionCount, const VkImageBlit *pRegions, VkFilter filter, + std::vector *mem_accesses) { const debug_report_data *report_data = core_validation::GetReportData(device_data); bool skip = false; - if (cb_node && src_image_state && dst_image_state) { + if (cb_state && src_image_state && dst_image_state) { skip |= ValidateImageSampleCount(device_data, src_image_state, VK_SAMPLE_COUNT_1_BIT, "vkCmdBlitImage(): srcImage", VALIDATION_ERROR_184001d2); skip |= ValidateImageSampleCount(device_data, dst_image_state, VK_SAMPLE_COUNT_1_BIT, "vkCmdBlitImage(): dstImage", @@ -2307,9 +2308,9 @@ bool PreCallValidateCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_nod VALIDATION_ERROR_184001b6, "vkCmdBlitImage()", "VK_IMAGE_USAGE_TRANSFER_SRC_BIT"); skip |= ValidateImageUsageFlags(device_data, dst_image_state, VK_IMAGE_USAGE_TRANSFER_DST_BIT, true, VALIDATION_ERROR_184001c0, "vkCmdBlitImage()", "VK_IMAGE_USAGE_TRANSFER_DST_BIT"); - skip |= ValidateCmdQueueFlags(device_data, cb_node, "vkCmdBlitImage()", VK_QUEUE_GRAPHICS_BIT, VALIDATION_ERROR_18402415); - skip |= ValidateCmd(device_data, cb_node, CMD_BLITIMAGE, "vkCmdBlitImage()"); - skip |= insideRenderPass(device_data, cb_node, "vkCmdBlitImage()", VALIDATION_ERROR_18400017); + skip |= ValidateCmdQueueFlags(device_data, cb_state, "vkCmdBlitImage()", VK_QUEUE_GRAPHICS_BIT, VALIDATION_ERROR_18402415); + skip |= ValidateCmd(device_data, cb_state, CMD_BLITIMAGE, "vkCmdBlitImage()"); + skip |= insideRenderPass(device_data, cb_state, "vkCmdBlitImage()", VALIDATION_ERROR_18400017); // TODO: Need to validate image layouts, which will include layout validation for shared presentable images VkFormat src_format = src_image_state->createInfo.format; @@ -2323,7 +2324,7 @@ bool PreCallValidateCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_nod (tiling == VK_IMAGE_TILING_LINEAR ? props.linearTilingFeatures : props.optimalTilingFeatures); if (VK_FORMAT_FEATURE_BLIT_SRC_BIT != (flags & VK_FORMAT_FEATURE_BLIT_SRC_BIT)) { skip |= log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, VALIDATION_ERROR_184001b4, "IMAGE", + HandleToUint64(cb_state->commandBuffer), __LINE__, VALIDATION_ERROR_184001b4, "IMAGE", "vkCmdBlitImage: source image format %s does not support VK_FORMAT_FEATURE_BLIT_SRC_BIT feature. %s", string_VkFormat(src_format), validation_error_map[VALIDATION_ERROR_184001b4]); } @@ -2331,7 +2332,7 @@ bool PreCallValidateCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_nod if ((VK_FILTER_LINEAR == filter) && (VK_FORMAT_FEATURE_SAMPLED_IMAGE_FILTER_LINEAR_BIT != (flags & VK_FORMAT_FEATURE_SAMPLED_IMAGE_FILTER_LINEAR_BIT))) { skip |= log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, VALIDATION_ERROR_184001d6, "IMAGE", + HandleToUint64(cb_state->commandBuffer), __LINE__, VALIDATION_ERROR_184001d6, "IMAGE", "vkCmdBlitImage: source image format %s does not support linear filtering. %s", string_VkFormat(src_format), validation_error_map[VALIDATION_ERROR_184001d6]); } @@ -2339,14 +2340,14 @@ bool PreCallValidateCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_nod if ((VK_FILTER_CUBIC_IMG == filter) && (VK_FORMAT_FEATURE_SAMPLED_IMAGE_FILTER_CUBIC_BIT_IMG != (flags & VK_FORMAT_FEATURE_SAMPLED_IMAGE_FILTER_CUBIC_BIT_IMG))) { skip |= log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, VALIDATION_ERROR_184001d8, "IMAGE", + HandleToUint64(cb_state->commandBuffer), __LINE__, VALIDATION_ERROR_184001d8, "IMAGE", "vkCmdBlitImage: source image format %s does not support cubic filtering. %s", string_VkFormat(src_format), validation_error_map[VALIDATION_ERROR_184001d8]); } if ((VK_FILTER_CUBIC_IMG == filter) && (VK_IMAGE_TYPE_3D != src_type)) { skip |= log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, VALIDATION_ERROR_184001da, "IMAGE", + HandleToUint64(cb_state->commandBuffer), __LINE__, VALIDATION_ERROR_184001da, "IMAGE", "vkCmdBlitImage: source image type must be VK_IMAGE_TYPE_3D when cubic filtering is specified. %s", validation_error_map[VALIDATION_ERROR_184001da]); } @@ -2357,7 +2358,7 @@ bool PreCallValidateCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_nod if (VK_FORMAT_FEATURE_BLIT_DST_BIT != (flags & VK_FORMAT_FEATURE_BLIT_DST_BIT)) { skip |= log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, VALIDATION_ERROR_184001be, "IMAGE", + HandleToUint64(cb_state->commandBuffer), __LINE__, VALIDATION_ERROR_184001be, "IMAGE", "vkCmdBlitImage: destination image format %s does not support VK_FORMAT_FEATURE_BLIT_DST_BIT feature. %s", string_VkFormat(dst_format), validation_error_map[VALIDATION_ERROR_184001be]); } @@ -2365,7 +2366,7 @@ bool PreCallValidateCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_nod if ((VK_SAMPLE_COUNT_1_BIT != src_image_state->createInfo.samples) || (VK_SAMPLE_COUNT_1_BIT != dst_image_state->createInfo.samples)) { skip |= log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, VALIDATION_ERROR_184001c8, "IMAGE", + HandleToUint64(cb_state->commandBuffer), __LINE__, VALIDATION_ERROR_184001c8, "IMAGE", "vkCmdBlitImage: source or dest image has sample count other than VK_SAMPLE_COUNT_1_BIT. %s", validation_error_map[VALIDATION_ERROR_184001c8]); } @@ -2377,7 +2378,7 @@ bool PreCallValidateCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_nod << "the other one must also have unsigned integer format. " << "Source format is " << string_VkFormat(src_format) << " Destination format is " << string_VkFormat(dst_format); skip |= log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, VALIDATION_ERROR_184001cc, "IMAGE", "%s. %s", + HandleToUint64(cb_state->commandBuffer), __LINE__, VALIDATION_ERROR_184001cc, "IMAGE", "%s. %s", ss.str().c_str(), validation_error_map[VALIDATION_ERROR_184001cc]); } @@ -2388,7 +2389,7 @@ bool PreCallValidateCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_nod << "the other one must also have signed integer format. " << "Source format is " << string_VkFormat(src_format) << " Destination format is " << string_VkFormat(dst_format); skip |= log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, VALIDATION_ERROR_184001ca, "IMAGE", "%s. %s", + HandleToUint64(cb_state->commandBuffer), __LINE__, VALIDATION_ERROR_184001ca, "IMAGE", "%s. %s", ss.str().c_str(), validation_error_map[VALIDATION_ERROR_184001ca]); } @@ -2398,7 +2399,7 @@ bool PreCallValidateCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_nod ss << "vkCmdBlitImage: If the format of srcImage is a depth, stencil, or depth stencil " << "then filter must be VK_FILTER_NEAREST."; skip |= log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, VALIDATION_ERROR_184001d0, "IMAGE", "%s. %s", + HandleToUint64(cb_state->commandBuffer), __LINE__, VALIDATION_ERROR_184001d0, "IMAGE", "%s. %s", ss.str().c_str(), validation_error_map[VALIDATION_ERROR_184001d0]); } @@ -2411,7 +2412,7 @@ bool PreCallValidateCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_nod << "Source format is " << string_VkFormat(src_format) << " Destination format is " << string_VkFormat(dst_format); skip |= log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, VALIDATION_ERROR_184001ce, "IMAGE", "%s. %s", + HandleToUint64(cb_state->commandBuffer), __LINE__, VALIDATION_ERROR_184001ce, "IMAGE", "%s. %s", ss.str().c_str(), validation_error_map[VALIDATION_ERROR_184001ce]); } @@ -2426,7 +2427,7 @@ bool PreCallValidateCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_nod "VK_IMAGE_ASPECT_DEPTH_BIT " << "and VK_IMAGE_ASPECT_STENCIL_BIT set in srcImage and dstImage"; skip |= log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, DRAWSTATE_INVALID_IMAGE_ASPECT, "IMAGE", + HandleToUint64(cb_state->commandBuffer), __LINE__, DRAWSTATE_INVALID_IMAGE_ASPECT, "IMAGE", "%s", ss.str().c_str()); } } @@ -2436,7 +2437,7 @@ bool PreCallValidateCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_nod ss << "vkCmdBlitImage: Stencil-only image formats must have only the VK_IMAGE_ASPECT_STENCIL_BIT " << "set in both the srcImage and dstImage"; skip |= log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, DRAWSTATE_INVALID_IMAGE_ASPECT, "IMAGE", + HandleToUint64(cb_state->commandBuffer), __LINE__, DRAWSTATE_INVALID_IMAGE_ASPECT, "IMAGE", "%s", ss.str().c_str()); } } @@ -2446,7 +2447,7 @@ bool PreCallValidateCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_nod ss << "vkCmdBlitImage: Depth-only image formats must have only the VK_IMAGE_ASPECT_DEPTH " << "set in both the srcImage and dstImage"; skip |= log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, DRAWSTATE_INVALID_IMAGE_ASPECT, "IMAGE", + HandleToUint64(cb_state->commandBuffer), __LINE__, DRAWSTATE_INVALID_IMAGE_ASPECT, "IMAGE", "%s", ss.str().c_str()); } } @@ -2464,7 +2465,7 @@ bool PreCallValidateCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_nod std::stringstream ss; ss << "vkCmdBlitImage: pRegions[" << i << "].srcOffsets specify a zero-volume area."; skip |= log_msg(report_data, VK_DEBUG_REPORT_WARNING_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, DRAWSTATE_INVALID_EXTENTS, "IMAGE", "%s", + HandleToUint64(cb_state->commandBuffer), __LINE__, DRAWSTATE_INVALID_EXTENTS, "IMAGE", "%s", ss.str().c_str()); } if ((rgn.dstOffsets[0].x == rgn.dstOffsets[1].x) || (rgn.dstOffsets[0].y == rgn.dstOffsets[1].y) || @@ -2472,39 +2473,39 @@ bool PreCallValidateCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_nod std::stringstream ss; ss << "vkCmdBlitImage: pRegions[" << i << "].dstOffsets specify a zero-volume area."; skip |= log_msg(report_data, VK_DEBUG_REPORT_WARNING_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, DRAWSTATE_INVALID_EXTENTS, "IMAGE", "%s", + HandleToUint64(cb_state->commandBuffer), __LINE__, DRAWSTATE_INVALID_EXTENTS, "IMAGE", "%s", ss.str().c_str()); } if (rgn.srcSubresource.layerCount == 0) { char const str[] = "vkCmdBlitImage: number of layers in source subresource is zero"; skip |= log_msg(report_data, VK_DEBUG_REPORT_WARNING_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, DRAWSTATE_MISMATCHED_IMAGE_ASPECT, "IMAGE", str); + HandleToUint64(cb_state->commandBuffer), __LINE__, DRAWSTATE_MISMATCHED_IMAGE_ASPECT, "IMAGE", str); } if (rgn.dstSubresource.layerCount == 0) { char const str[] = "vkCmdBlitImage: number of layers in destination subresource is zero"; skip |= log_msg(report_data, VK_DEBUG_REPORT_WARNING_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, DRAWSTATE_MISMATCHED_IMAGE_ASPECT, "IMAGE", str); + HandleToUint64(cb_state->commandBuffer), __LINE__, DRAWSTATE_MISMATCHED_IMAGE_ASPECT, "IMAGE", str); } // Check that src/dst layercounts match if (rgn.srcSubresource.layerCount != rgn.dstSubresource.layerCount) { skip |= log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, VALIDATION_ERROR_09a001de, "IMAGE", + HandleToUint64(cb_state->commandBuffer), __LINE__, VALIDATION_ERROR_09a001de, "IMAGE", "vkCmdBlitImage: layerCount in source and destination subresource of pRegions[%d] does not match. %s", i, validation_error_map[VALIDATION_ERROR_09a001de]); } if (rgn.srcSubresource.aspectMask != rgn.dstSubresource.aspectMask) { skip |= log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, VALIDATION_ERROR_09a001dc, "IMAGE", + HandleToUint64(cb_state->commandBuffer), __LINE__, VALIDATION_ERROR_09a001dc, "IMAGE", "vkCmdBlitImage: aspectMask members for pRegion[%d] do not match. %s", i, validation_error_map[VALIDATION_ERROR_09a001dc]); } if (!VerifyAspectsPresent(rgn.srcSubresource.aspectMask, src_format)) { skip |= log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, VALIDATION_ERROR_09a001e2, "IMAGE", + HandleToUint64(cb_state->commandBuffer), __LINE__, VALIDATION_ERROR_09a001e2, "IMAGE", "vkCmdBlitImage: region [%d] source aspectMask (0x%x) specifies aspects not present in source " "image format %s. %s", i, rgn.srcSubresource.aspectMask, string_VkFormat(src_format), @@ -2514,7 +2515,7 @@ bool PreCallValidateCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_nod if (!VerifyAspectsPresent(rgn.dstSubresource.aspectMask, dst_format)) { skip |= log_msg( report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, VALIDATION_ERROR_09a001e4, "IMAGE", + HandleToUint64(cb_state->commandBuffer), __LINE__, VALIDATION_ERROR_09a001e4, "IMAGE", "vkCmdBlitImage: region [%d] dest aspectMask (0x%x) specifies aspects not present in dest image format %s. %s", i, rgn.dstSubresource.aspectMask, string_VkFormat(dst_format), validation_error_map[VALIDATION_ERROR_09a001e4]); } @@ -2524,7 +2525,7 @@ bool PreCallValidateCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_nod if (VK_IMAGE_TYPE_1D == src_type) { if ((0 != rgn.srcOffsets[0].y) || (1 != rgn.srcOffsets[1].y)) { skip |= log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, VALIDATION_ERROR_09a001ea, "IMAGE", + HandleToUint64(cb_state->commandBuffer), __LINE__, VALIDATION_ERROR_09a001ea, "IMAGE", "vkCmdBlitImage: region [%d], source image of type VK_IMAGE_TYPE_1D with srcOffset[].y values " "of (%1d, %1d). These must be (0, 1). %s", i, rgn.srcOffsets[0].y, rgn.srcOffsets[1].y, validation_error_map[VALIDATION_ERROR_09a001ea]); @@ -2534,7 +2535,7 @@ bool PreCallValidateCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_nod if ((VK_IMAGE_TYPE_1D == src_type) || (VK_IMAGE_TYPE_2D == src_type)) { if ((0 != rgn.srcOffsets[0].z) || (1 != rgn.srcOffsets[1].z)) { skip |= log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, VALIDATION_ERROR_09a001ee, "IMAGE", + HandleToUint64(cb_state->commandBuffer), __LINE__, VALIDATION_ERROR_09a001ee, "IMAGE", "vkCmdBlitImage: region [%d], source image of type VK_IMAGE_TYPE_1D or VK_IMAGE_TYPE_2D with " "srcOffset[].z values of (%1d, %1d). These must be (0, 1). %s", i, rgn.srcOffsets[0].z, rgn.srcOffsets[1].z, validation_error_map[VALIDATION_ERROR_09a001ee]); @@ -2547,7 +2548,7 @@ bool PreCallValidateCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_nod oob = true; skip |= log_msg( report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, VALIDATION_ERROR_09a001e6, "IMAGE", + HandleToUint64(cb_state->commandBuffer), __LINE__, VALIDATION_ERROR_09a001e6, "IMAGE", "vkCmdBlitImage: region [%d] srcOffset[].x values (%1d, %1d) exceed srcSubresource width extent (%1d). %s", i, rgn.srcOffsets[0].x, rgn.srcOffsets[1].x, src_extent.width, validation_error_map[VALIDATION_ERROR_09a001e6]); } @@ -2556,7 +2557,7 @@ bool PreCallValidateCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_nod oob = true; skip |= log_msg( report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, VALIDATION_ERROR_09a001e8, "IMAGE", + HandleToUint64(cb_state->commandBuffer), __LINE__, VALIDATION_ERROR_09a001e8, "IMAGE", "vkCmdBlitImage: region [%d] srcOffset[].y values (%1d, %1d) exceed srcSubresource height extent (%1d). %s", i, rgn.srcOffsets[0].y, rgn.srcOffsets[1].y, src_extent.height, validation_error_map[VALIDATION_ERROR_09a001e8]); } @@ -2565,18 +2566,18 @@ bool PreCallValidateCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_nod oob = true; skip |= log_msg( report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, VALIDATION_ERROR_09a001ec, "IMAGE", + HandleToUint64(cb_state->commandBuffer), __LINE__, VALIDATION_ERROR_09a001ec, "IMAGE", "vkCmdBlitImage: region [%d] srcOffset[].z values (%1d, %1d) exceed srcSubresource depth extent (%1d). %s", i, rgn.srcOffsets[0].z, rgn.srcOffsets[1].z, src_extent.depth, validation_error_map[VALIDATION_ERROR_09a001ec]); } if (rgn.srcSubresource.mipLevel >= src_image_state->createInfo.mipLevels) { skip |= log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, VALIDATION_ERROR_184001ae, "IMAGE", + HandleToUint64(cb_state->commandBuffer), __LINE__, VALIDATION_ERROR_184001ae, "IMAGE", "vkCmdBlitImage: region [%d] source image, attempt to access a non-existant mip level %1d. %s", i, rgn.srcSubresource.mipLevel, validation_error_map[VALIDATION_ERROR_184001ae]); } else if (oob) { skip |= log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, VALIDATION_ERROR_184001ae, "IMAGE", + HandleToUint64(cb_state->commandBuffer), __LINE__, VALIDATION_ERROR_184001ae, "IMAGE", "vkCmdBlitImage: region [%d] source image blit region exceeds image dimensions. %s", i, validation_error_map[VALIDATION_ERROR_184001ae]); } @@ -2586,7 +2587,7 @@ bool PreCallValidateCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_nod if (VK_IMAGE_TYPE_1D == dst_type) { if ((0 != rgn.dstOffsets[0].y) || (1 != rgn.dstOffsets[1].y)) { skip |= log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, VALIDATION_ERROR_09a001f4, "IMAGE", + HandleToUint64(cb_state->commandBuffer), __LINE__, VALIDATION_ERROR_09a001f4, "IMAGE", "vkCmdBlitImage: region [%d], dest image of type VK_IMAGE_TYPE_1D with dstOffset[].y values of " "(%1d, %1d). These must be (0, 1). %s", i, rgn.dstOffsets[0].y, rgn.dstOffsets[1].y, validation_error_map[VALIDATION_ERROR_09a001f4]); @@ -2596,7 +2597,7 @@ bool PreCallValidateCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_nod if ((VK_IMAGE_TYPE_1D == dst_type) || (VK_IMAGE_TYPE_2D == dst_type)) { if ((0 != rgn.dstOffsets[0].z) || (1 != rgn.dstOffsets[1].z)) { skip |= log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, VALIDATION_ERROR_09a001f8, "IMAGE", + HandleToUint64(cb_state->commandBuffer), __LINE__, VALIDATION_ERROR_09a001f8, "IMAGE", "vkCmdBlitImage: region [%d], dest image of type VK_IMAGE_TYPE_1D or VK_IMAGE_TYPE_2D with " "dstOffset[].z values of (%1d, %1d). These must be (0, 1). %s", i, rgn.dstOffsets[0].z, rgn.dstOffsets[1].z, validation_error_map[VALIDATION_ERROR_09a001f8]); @@ -2609,7 +2610,7 @@ bool PreCallValidateCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_nod oob = true; skip |= log_msg( report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, VALIDATION_ERROR_09a001f0, "IMAGE", + HandleToUint64(cb_state->commandBuffer), __LINE__, VALIDATION_ERROR_09a001f0, "IMAGE", "vkCmdBlitImage: region [%d] dstOffset[].x values (%1d, %1d) exceed dstSubresource width extent (%1d). %s", i, rgn.dstOffsets[0].x, rgn.dstOffsets[1].x, dst_extent.width, validation_error_map[VALIDATION_ERROR_09a001f0]); } @@ -2618,7 +2619,7 @@ bool PreCallValidateCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_nod oob = true; skip |= log_msg( report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, VALIDATION_ERROR_09a001f2, "IMAGE", + HandleToUint64(cb_state->commandBuffer), __LINE__, VALIDATION_ERROR_09a001f2, "IMAGE", "vkCmdBlitImage: region [%d] dstOffset[].y values (%1d, %1d) exceed dstSubresource height extent (%1d). %s", i, rgn.dstOffsets[0].y, rgn.dstOffsets[1].y, dst_extent.height, validation_error_map[VALIDATION_ERROR_09a001f2]); } @@ -2627,18 +2628,18 @@ bool PreCallValidateCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_nod oob = true; skip |= log_msg( report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, VALIDATION_ERROR_09a001f6, "IMAGE", + HandleToUint64(cb_state->commandBuffer), __LINE__, VALIDATION_ERROR_09a001f6, "IMAGE", "vkCmdBlitImage: region [%d] dstOffset[].z values (%1d, %1d) exceed dstSubresource depth extent (%1d). %s", i, rgn.dstOffsets[0].z, rgn.dstOffsets[1].z, dst_extent.depth, validation_error_map[VALIDATION_ERROR_09a001f6]); } if (rgn.dstSubresource.mipLevel >= dst_image_state->createInfo.mipLevels) { skip |= log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, VALIDATION_ERROR_184001b0, "IMAGE", + HandleToUint64(cb_state->commandBuffer), __LINE__, VALIDATION_ERROR_184001b0, "IMAGE", "vkCmdBlitImage: region [%d] destination image, attempt to access a non-existant mip level %1d. %s", i, rgn.dstSubresource.mipLevel, validation_error_map[VALIDATION_ERROR_184001b0]); } else if (oob) { skip |= log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, VALIDATION_ERROR_184001b0, "IMAGE", + HandleToUint64(cb_state->commandBuffer), __LINE__, VALIDATION_ERROR_184001b0, "IMAGE", "vkCmdBlitImage: region [%d] destination image blit region exceeds image dimensions. %s", i, validation_error_map[VALIDATION_ERROR_184001b0]); } @@ -2647,32 +2648,38 @@ bool PreCallValidateCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_nod if ((0 != rgn.srcSubresource.baseArrayLayer) || (1 != rgn.srcSubresource.layerCount) || (0 != rgn.dstSubresource.baseArrayLayer) || (1 != rgn.dstSubresource.layerCount)) { skip |= log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, VALIDATION_ERROR_09a001e0, "IMAGE", + HandleToUint64(cb_state->commandBuffer), __LINE__, VALIDATION_ERROR_09a001e0, "IMAGE", "vkCmdBlitImage: region [%d] blit to/from a 3D image type with a non-zero baseArrayLayer, or a " "layerCount other than 1. %s", i, validation_error_map[VALIDATION_ERROR_09a001e0]); } } } // per-region checks + // Add memory access for this blit + // TODO: Currently treating all image accesses as imprecise & covering the whole image area + AddReadMemoryAccess(CMD_BLITIMAGE, mem_accesses, src_image_state->binding, false); + AddWriteMemoryAccess(CMD_BLITIMAGE, mem_accesses, dst_image_state->binding, false); + skip |= ValidateMemoryAccesses(report_data, cb_state, mem_accesses, "vkCmdBlitImage()"); } else { assert(0); } return skip; } -void PreCallRecordCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_node, IMAGE_STATE *src_image_state, - IMAGE_STATE *dst_image_state) { +void PreCallRecordCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_state, IMAGE_STATE *src_image_state, + IMAGE_STATE *dst_image_state, std::vector *mem_accesses) { // Update bindings between images and cmd buffer - AddCommandBufferBindingImage(device_data, cb_node, src_image_state); - AddCommandBufferBindingImage(device_data, cb_node, dst_image_state); + AddCommandBufferBindingImage(device_data, cb_state, src_image_state); + AddCommandBufferBindingImage(device_data, cb_state, dst_image_state); std::function function = [=]() { return ValidateImageMemoryIsValid(device_data, src_image_state, "vkCmdBlitImage()"); }; - cb_node->queue_submit_functions.push_back(function); + cb_state->queue_submit_functions.push_back(function); function = [=]() { SetImageMemoryValid(device_data, dst_image_state, true); return false; }; - cb_node->queue_submit_functions.push_back(function); + cb_state->queue_submit_functions.push_back(function); + AddCommandBufferCommandMemoryAccesses(cb_state, CMD_BLITIMAGE, mem_accesses); } // This validates that the initial layout specified in the command buffer for diff --git a/layers/buffer_validation.h b/layers/buffer_validation.h index 6cc0902d18..3c8c375e95 100644 --- a/layers/buffer_validation.h +++ b/layers/buffer_validation.h @@ -162,10 +162,11 @@ void PreCallRecordCmdResolveImage(layer_data *device_data, GLOBAL_CB_NODE *cb_no IMAGE_STATE *dst_image_state); bool PreCallValidateCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_node, IMAGE_STATE *src_image_state, - IMAGE_STATE *dst_image_state, uint32_t regionCount, const VkImageBlit *pRegions, VkFilter filter); + IMAGE_STATE *dst_image_state, uint32_t regionCount, const VkImageBlit *pRegions, VkFilter filter, + std::vector *mem_accesses); void PreCallRecordCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_node, IMAGE_STATE *src_image_state, - IMAGE_STATE *dst_image_state); + IMAGE_STATE *dst_image_state, std::vector *mem_accesses); bool ValidateCmdBufImageLayouts(layer_data *device_data, GLOBAL_CB_NODE *pCB, std::unordered_map const &globalImageLayoutMap, diff --git a/layers/core_validation.cpp b/layers/core_validation.cpp index 439d58d58f..fea6e18544 100644 --- a/layers/core_validation.cpp +++ b/layers/core_validation.cpp @@ -5942,16 +5942,18 @@ VKAPI_ATTR void VKAPI_CALL CmdBlitImage(VkCommandBuffer commandBuffer, VkImage s VkImage dstImage, VkImageLayout dstImageLayout, uint32_t regionCount, const VkImageBlit *pRegions, VkFilter filter) { layer_data *dev_data = GetLayerDataPtr(get_dispatch_key(commandBuffer), layer_data_map); + std::vector mem_accesses; unique_lock_t lock(global_lock); auto cb_node = GetCBNode(dev_data, commandBuffer); auto src_image_state = GetImageState(dev_data, srcImage); auto dst_image_state = GetImageState(dev_data, dstImage); - bool skip = PreCallValidateCmdBlitImage(dev_data, cb_node, src_image_state, dst_image_state, regionCount, pRegions, filter); + bool skip = PreCallValidateCmdBlitImage(dev_data, cb_node, src_image_state, dst_image_state, regionCount, pRegions, filter, + &mem_accesses); if (!skip) { - PreCallRecordCmdBlitImage(dev_data, cb_node, src_image_state, dst_image_state); + PreCallRecordCmdBlitImage(dev_data, cb_node, src_image_state, dst_image_state, &mem_accesses); lock.unlock(); dev_data->dispatch_table.CmdBlitImage(commandBuffer, srcImage, srcImageLayout, dstImage, dstImageLayout, regionCount, pRegions, filter); From ecce83b6ef82f48c916bdbedac48a5b0e85a0647 Mon Sep 17 00:00:00 2001 From: Tobin Ehlis Date: Wed, 9 Aug 2017 16:55:27 -0600 Subject: [PATCH 09/28] layers:Add CmdFillBuffer memory access tracking Add tracking for the memory access in CmdFillBuffer. This is a single write to a buffer for which we have precise info. --- layers/buffer_validation.cpp | 19 ++++++++++++------- layers/buffer_validation.h | 6 ++++-- layers/core_validation.cpp | 5 +++-- 3 files changed, 19 insertions(+), 11 deletions(-) diff --git a/layers/buffer_validation.cpp b/layers/buffer_validation.cpp index 5fe4d95693..556d87def0 100644 --- a/layers/buffer_validation.cpp +++ b/layers/buffer_validation.cpp @@ -3691,27 +3691,32 @@ void PostCallRecordDestroyBufferView(layer_data *device_data, VkBufferView buffe GetBufferViewMap(device_data)->erase(buffer_view); } -bool PreCallValidateCmdFillBuffer(layer_data *device_data, GLOBAL_CB_NODE *cb_node, BUFFER_STATE *buffer_state) { +bool PreCallValidateCmdFillBuffer(layer_data *device_data, GLOBAL_CB_NODE *cb_state, BUFFER_STATE *buffer_state, VkDeviceSize offset, + VkDeviceSize size, std::vector *mem_accesses) { bool skip = false; skip |= ValidateMemoryIsBoundToBuffer(device_data, buffer_state, "vkCmdFillBuffer()", VALIDATION_ERROR_1b40003e); - skip |= ValidateCmdQueueFlags(device_data, cb_node, "vkCmdFillBuffer()", + skip |= ValidateCmdQueueFlags(device_data, cb_state, "vkCmdFillBuffer()", VK_QUEUE_TRANSFER_BIT | VK_QUEUE_GRAPHICS_BIT | VK_QUEUE_COMPUTE_BIT, VALIDATION_ERROR_1b402415); - skip |= ValidateCmd(device_data, cb_node, CMD_FILLBUFFER, "vkCmdFillBuffer()"); + skip |= ValidateCmd(device_data, cb_state, CMD_FILLBUFFER, "vkCmdFillBuffer()"); // Validate that DST buffer has correct usage flags set skip |= ValidateBufferUsageFlags(device_data, buffer_state, VK_BUFFER_USAGE_TRANSFER_DST_BIT, true, VALIDATION_ERROR_1b40003a, "vkCmdFillBuffer()", "VK_BUFFER_USAGE_TRANSFER_DST_BIT"); - skip |= insideRenderPass(device_data, cb_node, "vkCmdFillBuffer()", VALIDATION_ERROR_1b400017); + skip |= insideRenderPass(device_data, cb_state, "vkCmdFillBuffer()", VALIDATION_ERROR_1b400017); + AddWriteMemoryAccess(CMD_FILLBUFFER, mem_accesses, {buffer_state->binding.mem, offset, size}, true); + skip |= ValidateMemoryAccesses(core_validation::GetReportData(device_data), cb_state, mem_accesses, "vkCmdFillBuffer()"); return skip; } -void PreCallRecordCmdFillBuffer(layer_data *device_data, GLOBAL_CB_NODE *cb_node, BUFFER_STATE *buffer_state) { +void PreCallRecordCmdFillBuffer(layer_data *device_data, GLOBAL_CB_NODE *cb_state, BUFFER_STATE *buffer_state, + std::vector *mem_accesses) { std::function function = [=]() { SetBufferMemoryValid(device_data, buffer_state, true); return false; }; - cb_node->queue_submit_functions.push_back(function); + cb_state->queue_submit_functions.push_back(function); // Update bindings between buffer and cmd buffer - AddCommandBufferBindingBuffer(device_data, cb_node, buffer_state); + AddCommandBufferBindingBuffer(device_data, cb_state, buffer_state); + AddCommandBufferCommandMemoryAccesses(cb_state, CMD_FILLBUFFER, mem_accesses); } bool ValidateBufferImageCopyData(const debug_report_data *report_data, uint32_t regionCount, const VkBufferImageCopy *pRegions, diff --git a/layers/buffer_validation.h b/layers/buffer_validation.h index 3c8c375e95..f5cf20bbbd 100644 --- a/layers/buffer_validation.h +++ b/layers/buffer_validation.h @@ -251,9 +251,11 @@ bool PreCallValidateDestroyBufferView(layer_data *device_data, VkBufferView buff void PostCallRecordDestroyBufferView(layer_data *device_data, VkBufferView buffer_view, BUFFER_VIEW_STATE *buffer_view_state, VK_OBJECT obj_struct); -bool PreCallValidateCmdFillBuffer(layer_data *device_data, GLOBAL_CB_NODE *cb_node, BUFFER_STATE *buffer_state); +bool PreCallValidateCmdFillBuffer(layer_data *device_data, GLOBAL_CB_NODE *cb_node, BUFFER_STATE *buffer_state, VkDeviceSize offset, + VkDeviceSize size, std::vector *mem_accesses); -void PreCallRecordCmdFillBuffer(layer_data *device_data, GLOBAL_CB_NODE *cb_node, BUFFER_STATE *buffer_state); +void PreCallRecordCmdFillBuffer(layer_data *device_data, GLOBAL_CB_NODE *cb_node, BUFFER_STATE *buffer_state, + std::vector *mem_accesses); bool PreCallValidateCmdCopyImageToBuffer(layer_data *device_data, VkImageLayout srcImageLayout, GLOBAL_CB_NODE *cb_node, IMAGE_STATE *src_image_state, BUFFER_STATE *dst_buff_state, uint32_t regionCount, diff --git a/layers/core_validation.cpp b/layers/core_validation.cpp index fea6e18544..6ccc1345fd 100644 --- a/layers/core_validation.cpp +++ b/layers/core_validation.cpp @@ -6066,14 +6066,15 @@ VKAPI_ATTR void VKAPI_CALL CmdUpdateBuffer(VkCommandBuffer commandBuffer, VkBuff VKAPI_ATTR void VKAPI_CALL CmdFillBuffer(VkCommandBuffer commandBuffer, VkBuffer dstBuffer, VkDeviceSize dstOffset, VkDeviceSize size, uint32_t data) { layer_data *device_data = GetLayerDataPtr(get_dispatch_key(commandBuffer), layer_data_map); + std::vector mem_accesses; unique_lock_t lock(global_lock); auto cb_node = GetCBNode(device_data, commandBuffer); auto buffer_state = GetBufferState(device_data, dstBuffer); if (cb_node && buffer_state) { - bool skip = PreCallValidateCmdFillBuffer(device_data, cb_node, buffer_state); + bool skip = PreCallValidateCmdFillBuffer(device_data, cb_node, buffer_state, dstOffset, size, &mem_accesses); if (!skip) { - PreCallRecordCmdFillBuffer(device_data, cb_node, buffer_state); + PreCallRecordCmdFillBuffer(device_data, cb_node, buffer_state, &mem_accesses); lock.unlock(); device_data->dispatch_table.CmdFillBuffer(commandBuffer, dstBuffer, dstOffset, size, data); } From 7b757d52f0394a3ae433272a5ddebb5199d65a3f Mon Sep 17 00:00:00 2001 From: Tobin Ehlis Date: Wed, 9 Aug 2017 17:17:06 -0600 Subject: [PATCH 10/28] layers:Add CmdClear*Image memory access tracking Add tracking for the memory accesses in CmdClear[DS|Color]Image. Initially just adding imprecise write access covering the whole image binding area. --- layers/buffer_validation.cpp | 51 +++++++++++++++++++++--------------- layers/buffer_validation.h | 8 +++--- layers/core_validation.cpp | 13 ++++++--- 3 files changed, 44 insertions(+), 28 deletions(-) diff --git a/layers/buffer_validation.cpp b/layers/buffer_validation.cpp index 556d87def0..8bc7bc6068 100644 --- a/layers/buffer_validation.cpp +++ b/layers/buffer_validation.cpp @@ -1090,65 +1090,71 @@ void RecordClearImageLayout(layer_data *device_data, GLOBAL_CB_NODE *cb_node, Vk } bool PreCallValidateCmdClearColorImage(layer_data *dev_data, VkCommandBuffer commandBuffer, VkImage image, - VkImageLayout imageLayout, uint32_t rangeCount, const VkImageSubresourceRange *pRanges) { + VkImageLayout imageLayout, uint32_t rangeCount, const VkImageSubresourceRange *pRanges, + std::vector *mem_accesses) { bool skip = false; // TODO : Verify memory is in VK_IMAGE_STATE_CLEAR state - auto cb_node = GetCBNode(dev_data, commandBuffer); + auto cb_state = GetCBNode(dev_data, commandBuffer); auto image_state = GetImageState(dev_data, image); - if (cb_node && image_state) { + if (cb_state && image_state) { skip |= ValidateMemoryIsBoundToImage(dev_data, image_state, "vkCmdClearColorImage()", VALIDATION_ERROR_18800006); - skip |= ValidateCmdQueueFlags(dev_data, cb_node, "vkCmdClearColorImage()", VK_QUEUE_GRAPHICS_BIT | VK_QUEUE_COMPUTE_BIT, + skip |= ValidateCmdQueueFlags(dev_data, cb_state, "vkCmdClearColorImage()", VK_QUEUE_GRAPHICS_BIT | VK_QUEUE_COMPUTE_BIT, VALIDATION_ERROR_18802415); - skip |= ValidateCmd(dev_data, cb_node, CMD_CLEARCOLORIMAGE, "vkCmdClearColorImage()"); - skip |= insideRenderPass(dev_data, cb_node, "vkCmdClearColorImage()", VALIDATION_ERROR_18800017); + skip |= ValidateCmd(dev_data, cb_state, CMD_CLEARCOLORIMAGE, "vkCmdClearColorImage()"); + skip |= insideRenderPass(dev_data, cb_state, "vkCmdClearColorImage()", VALIDATION_ERROR_18800017); for (uint32_t i = 0; i < rangeCount; ++i) { std::string param_name = "pRanges[" + std::to_string(i) + "]"; skip |= ValidateCmdClearColorSubresourceRange(dev_data, image_state, pRanges[i], param_name.c_str()); skip |= ValidateImageAttributes(dev_data, image_state, pRanges[i]); - skip |= VerifyClearImageLayout(dev_data, cb_node, image_state, pRanges[i], imageLayout, "vkCmdClearColorImage()"); + skip |= VerifyClearImageLayout(dev_data, cb_state, image_state, pRanges[i], imageLayout, "vkCmdClearColorImage()"); } } + // TODO: Currently treating all image accesses as imprecise & covering the whole image area, this should be per-range + AddWriteMemoryAccess(CMD_CLEARCOLORIMAGE, mem_accesses, image_state->binding, false); + skip |= ValidateMemoryAccesses(core_validation::GetReportData(dev_data), cb_state, mem_accesses, "vkCmdClearColorImage()"); return skip; } // This state recording routine is shared between ClearColorImage and ClearDepthStencilImage void PreCallRecordCmdClearImage(layer_data *dev_data, VkCommandBuffer commandBuffer, VkImage image, VkImageLayout imageLayout, - uint32_t rangeCount, const VkImageSubresourceRange *pRanges) { - auto cb_node = GetCBNode(dev_data, commandBuffer); + uint32_t rangeCount, const VkImageSubresourceRange *pRanges, CMD_TYPE cmd, + std::vector *mem_accesses) { + auto cb_state = GetCBNode(dev_data, commandBuffer); auto image_state = GetImageState(dev_data, image); - if (cb_node && image_state) { - AddCommandBufferBindingImage(dev_data, cb_node, image_state); + if (cb_state && image_state) { + AddCommandBufferBindingImage(dev_data, cb_state, image_state); std::function function = [=]() { SetImageMemoryValid(dev_data, image_state, true); return false; }; - cb_node->queue_submit_functions.push_back(function); + cb_state->queue_submit_functions.push_back(function); for (uint32_t i = 0; i < rangeCount; ++i) { - RecordClearImageLayout(dev_data, cb_node, image, pRanges[i], imageLayout); + RecordClearImageLayout(dev_data, cb_state, image, pRanges[i], imageLayout); } } + AddCommandBufferCommandMemoryAccesses(cb_state, cmd, mem_accesses); } bool PreCallValidateCmdClearDepthStencilImage(layer_data *device_data, VkCommandBuffer commandBuffer, VkImage image, VkImageLayout imageLayout, uint32_t rangeCount, - const VkImageSubresourceRange *pRanges) { + const VkImageSubresourceRange *pRanges, std::vector *mem_accesses) { bool skip = false; const debug_report_data *report_data = core_validation::GetReportData(device_data); // TODO : Verify memory is in VK_IMAGE_STATE_CLEAR state - auto cb_node = GetCBNode(device_data, commandBuffer); + auto cb_state = GetCBNode(device_data, commandBuffer); auto image_state = GetImageState(device_data, image); - if (cb_node && image_state) { + if (cb_state && image_state) { skip |= ValidateMemoryIsBoundToImage(device_data, image_state, "vkCmdClearDepthStencilImage()", VALIDATION_ERROR_18a00014); - skip |= ValidateCmdQueueFlags(device_data, cb_node, "vkCmdClearDepthStencilImage()", VK_QUEUE_GRAPHICS_BIT, + skip |= ValidateCmdQueueFlags(device_data, cb_state, "vkCmdClearDepthStencilImage()", VK_QUEUE_GRAPHICS_BIT, VALIDATION_ERROR_18a02415); - skip |= ValidateCmd(device_data, cb_node, CMD_CLEARDEPTHSTENCILIMAGE, "vkCmdClearDepthStencilImage()"); - skip |= insideRenderPass(device_data, cb_node, "vkCmdClearDepthStencilImage()", VALIDATION_ERROR_18a00017); + skip |= ValidateCmd(device_data, cb_state, CMD_CLEARDEPTHSTENCILIMAGE, "vkCmdClearDepthStencilImage()"); + skip |= insideRenderPass(device_data, cb_state, "vkCmdClearDepthStencilImage()", VALIDATION_ERROR_18a00017); for (uint32_t i = 0; i < rangeCount; ++i) { std::string param_name = "pRanges[" + std::to_string(i) + "]"; skip |= ValidateCmdClearDepthSubresourceRange(device_data, image_state, pRanges[i], param_name.c_str()); - skip |= - VerifyClearImageLayout(device_data, cb_node, image_state, pRanges[i], imageLayout, "vkCmdClearDepthStencilImage()"); + skip |= VerifyClearImageLayout(device_data, cb_state, image_state, pRanges[i], imageLayout, + "vkCmdClearDepthStencilImage()"); // Image aspect must be depth or stencil or both if (((pRanges[i].aspectMask & VK_IMAGE_ASPECT_DEPTH_BIT) != VK_IMAGE_ASPECT_DEPTH_BIT) && ((pRanges[i].aspectMask & VK_IMAGE_ASPECT_STENCIL_BIT) != VK_IMAGE_ASPECT_STENCIL_BIT)) { @@ -1166,6 +1172,9 @@ bool PreCallValidateCmdClearDepthStencilImage(layer_data *device_data, VkCommand validation_error_map[VALIDATION_ERROR_18a0001c]); } } + // TODO: Currently treating all image accesses as imprecise & covering the whole image area, this should be per-range + AddWriteMemoryAccess(CMD_CLEARDEPTHSTENCILIMAGE, mem_accesses, image_state->binding, false); + skip |= ValidateMemoryAccesses(report_data, cb_state, mem_accesses, "vkCmdClearDepthStencilImage()"); return skip; } diff --git a/layers/buffer_validation.h b/layers/buffer_validation.h index f5cf20bbbd..9f905028db 100644 --- a/layers/buffer_validation.h +++ b/layers/buffer_validation.h @@ -59,14 +59,16 @@ void RecordClearImageLayout(layer_data *dev_data, GLOBAL_CB_NODE *cb_node, VkIma VkImageLayout dest_image_layout); bool PreCallValidateCmdClearColorImage(layer_data *dev_data, VkCommandBuffer commandBuffer, VkImage image, - VkImageLayout imageLayout, uint32_t rangeCount, const VkImageSubresourceRange *pRanges); + VkImageLayout imageLayout, uint32_t rangeCount, const VkImageSubresourceRange *pRanges, + std::vector *mem_accesses); void PreCallRecordCmdClearImage(layer_data *dev_data, VkCommandBuffer commandBuffer, VkImage image, VkImageLayout imageLayout, - uint32_t rangeCount, const VkImageSubresourceRange *pRanges); + uint32_t rangeCount, const VkImageSubresourceRange *pRanges, CMD_TYPE cmd, + std::vector *mem_accesses); bool PreCallValidateCmdClearDepthStencilImage(layer_data *dev_data, VkCommandBuffer commandBuffer, VkImage image, VkImageLayout imageLayout, uint32_t rangeCount, - const VkImageSubresourceRange *pRanges); + const VkImageSubresourceRange *pRanges, std::vector *mem_accesses); bool FindLayoutVerifyNode(layer_data const *device_data, GLOBAL_CB_NODE const *pCB, ImageSubresourcePair imgpair, IMAGE_CMD_BUF_LAYOUT_NODE &node, const VkImageAspectFlags aspectMask); diff --git a/layers/core_validation.cpp b/layers/core_validation.cpp index 6ccc1345fd..9407727c63 100644 --- a/layers/core_validation.cpp +++ b/layers/core_validation.cpp @@ -6100,11 +6100,13 @@ VKAPI_ATTR void VKAPI_CALL CmdClearColorImage(VkCommandBuffer commandBuffer, VkI const VkClearColorValue *pColor, uint32_t rangeCount, const VkImageSubresourceRange *pRanges) { layer_data *dev_data = GetLayerDataPtr(get_dispatch_key(commandBuffer), layer_data_map); + std::vector mem_accesses; unique_lock_t lock(global_lock); - bool skip = PreCallValidateCmdClearColorImage(dev_data, commandBuffer, image, imageLayout, rangeCount, pRanges); + bool skip = PreCallValidateCmdClearColorImage(dev_data, commandBuffer, image, imageLayout, rangeCount, pRanges, &mem_accesses); if (!skip) { - PreCallRecordCmdClearImage(dev_data, commandBuffer, image, imageLayout, rangeCount, pRanges); + PreCallRecordCmdClearImage(dev_data, commandBuffer, image, imageLayout, rangeCount, pRanges, CMD_CLEARCOLORIMAGE, + &mem_accesses); lock.unlock(); dev_data->dispatch_table.CmdClearColorImage(commandBuffer, image, imageLayout, pColor, rangeCount, pRanges); } @@ -6114,11 +6116,14 @@ VKAPI_ATTR void VKAPI_CALL CmdClearDepthStencilImage(VkCommandBuffer commandBuff const VkClearDepthStencilValue *pDepthStencil, uint32_t rangeCount, const VkImageSubresourceRange *pRanges) { layer_data *dev_data = GetLayerDataPtr(get_dispatch_key(commandBuffer), layer_data_map); + std::vector mem_accesses; unique_lock_t lock(global_lock); - bool skip = PreCallValidateCmdClearDepthStencilImage(dev_data, commandBuffer, image, imageLayout, rangeCount, pRanges); + bool skip = + PreCallValidateCmdClearDepthStencilImage(dev_data, commandBuffer, image, imageLayout, rangeCount, pRanges, &mem_accesses); if (!skip) { - PreCallRecordCmdClearImage(dev_data, commandBuffer, image, imageLayout, rangeCount, pRanges); + PreCallRecordCmdClearImage(dev_data, commandBuffer, image, imageLayout, rangeCount, pRanges, CMD_CLEARDEPTHSTENCILIMAGE, + &mem_accesses); lock.unlock(); dev_data->dispatch_table.CmdClearDepthStencilImage(commandBuffer, image, imageLayout, pDepthStencil, rangeCount, pRanges); } From aec7b2fe79b0b7b942e06553532b7372ca448e95 Mon Sep 17 00:00:00 2001 From: Tobin Ehlis Date: Wed, 9 Aug 2017 17:44:26 -0600 Subject: [PATCH 11/28] layers:Add mem access for CmdClearAttachments Update CmdClearAttachments to record a write memory access per image attachment that's cleared. This is an imprecise access across the entire image binding for now. --- layers/buffer_validation.cpp | 37 ++++++++++++++++++++---------------- layers/buffer_validation.h | 3 ++- layers/core_validation.cpp | 8 ++++++-- 3 files changed, 29 insertions(+), 19 deletions(-) diff --git a/layers/buffer_validation.cpp b/layers/buffer_validation.cpp index 8bc7bc6068..96981f4c41 100644 --- a/layers/buffer_validation.cpp +++ b/layers/buffer_validation.cpp @@ -2093,18 +2093,19 @@ static inline bool ContainsRect(VkRect2D rect, VkRect2D sub_rect) { } bool PreCallValidateCmdClearAttachments(layer_data *device_data, VkCommandBuffer commandBuffer, uint32_t attachmentCount, - const VkClearAttachment *pAttachments, uint32_t rectCount, const VkClearRect *pRects) { - GLOBAL_CB_NODE *cb_node = GetCBNode(device_data, commandBuffer); + const VkClearAttachment *pAttachments, uint32_t rectCount, const VkClearRect *pRects, + std::vector *mem_accesses) { + GLOBAL_CB_NODE *cb_state = GetCBNode(device_data, commandBuffer); const debug_report_data *report_data = core_validation::GetReportData(device_data); bool skip = false; - if (cb_node) { - skip |= ValidateCmdQueueFlags(device_data, cb_node, "vkCmdClearAttachments()", VK_QUEUE_GRAPHICS_BIT, + if (cb_state) { + skip |= ValidateCmdQueueFlags(device_data, cb_state, "vkCmdClearAttachments()", VK_QUEUE_GRAPHICS_BIT, VALIDATION_ERROR_18602415); - skip |= ValidateCmd(device_data, cb_node, CMD_CLEARATTACHMENTS, "vkCmdClearAttachments()"); + skip |= ValidateCmd(device_data, cb_state, CMD_CLEARATTACHMENTS, "vkCmdClearAttachments()"); // Warn if this is issued prior to Draw Cmd and clearing the entire attachment - if (!cb_node->hasDrawCmd && (cb_node->activeRenderPassBeginInfo.renderArea.extent.width == pRects[0].rect.extent.width) && - (cb_node->activeRenderPassBeginInfo.renderArea.extent.height == pRects[0].rect.extent.height)) { + if (!cb_state->hasDrawCmd && (cb_state->activeRenderPassBeginInfo.renderArea.extent.width == pRects[0].rect.extent.width) && + (cb_state->activeRenderPassBeginInfo.renderArea.extent.height == pRects[0].rect.extent.height)) { // There are times where app needs to use ClearAttachments (generally when reusing a buffer inside of a render pass) // This warning should be made more specific. It'd be best to avoid triggering this test if it's a use that must call // CmdClearAttachments. @@ -2115,14 +2116,14 @@ bool PreCallValidateCmdClearAttachments(layer_data *device_data, VkCommandBuffer " It is recommended you use RenderPass LOAD_OP_CLEAR on Attachments prior to any Draw.", commandBuffer); } - skip |= outsideRenderPass(device_data, cb_node, "vkCmdClearAttachments()", VALIDATION_ERROR_18600017); + skip |= outsideRenderPass(device_data, cb_state, "vkCmdClearAttachments()", VALIDATION_ERROR_18600017); } // Validate that attachment is in reference list of active subpass - if (cb_node->activeRenderPass) { - const VkRenderPassCreateInfo *renderpass_create_info = cb_node->activeRenderPass->createInfo.ptr(); - const VkSubpassDescription *subpass_desc = &renderpass_create_info->pSubpasses[cb_node->activeSubpass]; - auto framebuffer = GetFramebufferState(device_data, cb_node->activeFramebuffer); + if (cb_state->activeRenderPass) { + const VkRenderPassCreateInfo *renderpass_create_info = cb_state->activeRenderPass->createInfo.ptr(); + const VkSubpassDescription *subpass_desc = &renderpass_create_info->pSubpasses[cb_state->activeSubpass]; + auto framebuffer = GetFramebufferState(device_data, cb_state->activeFramebuffer); for (uint32_t i = 0; i < attachmentCount; i++) { auto clear_desc = &pAttachments[i]; @@ -2141,7 +2142,7 @@ bool PreCallValidateCmdClearAttachments(layer_data *device_data, VkCommandBuffer skip |= log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, HandleToUint64(commandBuffer), __LINE__, VALIDATION_ERROR_1860001e, "DS", "vkCmdClearAttachments() color attachment index %d out of range for active subpass %d. %s", - clear_desc->colorAttachment, cb_node->activeSubpass, + clear_desc->colorAttachment, cb_state->activeSubpass, validation_error_map[VALIDATION_ERROR_1860001e]); } else if (subpass_desc->pColorAttachments[clear_desc->colorAttachment].attachment == VK_ATTACHMENT_UNUSED) { skip |= log_msg(report_data, VK_DEBUG_REPORT_PERFORMANCE_WARNING_BIT_EXT, @@ -2180,13 +2181,13 @@ bool PreCallValidateCmdClearAttachments(layer_data *device_data, VkCommandBuffer } } if (image_view) { - auto image_view_state = GetImageViewState(device_data, image_view); + const auto image_view_state = GetImageViewState(device_data, image_view); for (uint32_t j = 0; j < rectCount; j++) { // The rectangular region specified by a given element of pRects must be contained within the render area of // the current render pass instance // TODO: This check should be moved to CmdExecuteCommands or QueueSubmit to cover secondary CB cases - if ((cb_node->createInfo.level == VK_COMMAND_BUFFER_LEVEL_PRIMARY) && - (false == ContainsRect(cb_node->activeRenderPassBeginInfo.renderArea, pRects[j].rect))) { + if ((cb_state->createInfo.level == VK_COMMAND_BUFFER_LEVEL_PRIMARY) && + (false == ContainsRect(cb_state->activeRenderPassBeginInfo.renderArea, pRects[j].rect))) { skip |= log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, HandleToUint64(commandBuffer), __LINE__, VALIDATION_ERROR_18600020, "DS", "vkCmdClearAttachments(): The area defined by pRects[%d] is not contained in the area of " @@ -2206,6 +2207,10 @@ bool PreCallValidateCmdClearAttachments(layer_data *device_data, VkCommandBuffer j, i, validation_error_map[VALIDATION_ERROR_18600022]); } } + // TODO: Just marking memory accesses as imprecise per-image binding basis for now, should be per-rect above + const auto image_state = GetImageState(device_data, image_view_state->create_info.image); + AddWriteMemoryAccess(CMD_CLEARATTACHMENTS, mem_accesses, image_state->binding, false); + skip |= ValidateMemoryAccesses(report_data, cb_state, mem_accesses, "vkCmdClearAttachments()"); } } } diff --git a/layers/buffer_validation.h b/layers/buffer_validation.h index 9f905028db..5f827bf516 100644 --- a/layers/buffer_validation.h +++ b/layers/buffer_validation.h @@ -155,7 +155,8 @@ bool PreCallValidateCmdCopyImage(layer_data *device_data, GLOBAL_CB_NODE *cb_nod std::vector *mem_accesses); bool PreCallValidateCmdClearAttachments(layer_data *device_data, VkCommandBuffer commandBuffer, uint32_t attachmentCount, - const VkClearAttachment *pAttachments, uint32_t rectCount, const VkClearRect *pRects); + const VkClearAttachment *pAttachments, uint32_t rectCount, const VkClearRect *pRects, + std::vector *mem_accesses); bool PreCallValidateCmdResolveImage(layer_data *device_data, GLOBAL_CB_NODE *cb_node, IMAGE_STATE *src_image_state, IMAGE_STATE *dst_image_state, uint32_t regionCount, const VkImageResolve *pRegions); diff --git a/layers/core_validation.cpp b/layers/core_validation.cpp index 9407727c63..7325c6abc6 100644 --- a/layers/core_validation.cpp +++ b/layers/core_validation.cpp @@ -6089,11 +6089,15 @@ VKAPI_ATTR void VKAPI_CALL CmdClearAttachments(VkCommandBuffer commandBuffer, ui const VkClearRect *pRects) { bool skip = false; layer_data *dev_data = GetLayerDataPtr(get_dispatch_key(commandBuffer), layer_data_map); + std::vector mem_accesses; { lock_guard_t lock(global_lock); - skip = PreCallValidateCmdClearAttachments(dev_data, commandBuffer, attachmentCount, pAttachments, rectCount, pRects); + skip = PreCallValidateCmdClearAttachments(dev_data, commandBuffer, attachmentCount, pAttachments, rectCount, pRects, + &mem_accesses); + } + if (!skip) { + dev_data->dispatch_table.CmdClearAttachments(commandBuffer, attachmentCount, pAttachments, rectCount, pRects); } - if (!skip) dev_data->dispatch_table.CmdClearAttachments(commandBuffer, attachmentCount, pAttachments, rectCount, pRects); } VKAPI_ATTR void VKAPI_CALL CmdClearColorImage(VkCommandBuffer commandBuffer, VkImage image, VkImageLayout imageLayout, From d3f695f3748ea10cec96e40fb4ebcba3646c53e9 Mon Sep 17 00:00:00 2001 From: Tobin Ehlis Date: Wed, 9 Aug 2017 17:58:36 -0600 Subject: [PATCH 12/28] layers:Add state update for CmdClearAttachments Update global mem access state when CmdClearAttachments is called. --- layers/buffer_validation.cpp | 36 +++++++++++++++++++----------------- layers/buffer_validation.h | 5 ++++- layers/core_validation.cpp | 20 +++++++++++++------- 3 files changed, 36 insertions(+), 25 deletions(-) diff --git a/layers/buffer_validation.cpp b/layers/buffer_validation.cpp index 96981f4c41..1eb897f0c1 100644 --- a/layers/buffer_validation.cpp +++ b/layers/buffer_validation.cpp @@ -2092,32 +2092,29 @@ static inline bool ContainsRect(VkRect2D rect, VkRect2D sub_rect) { return true; } -bool PreCallValidateCmdClearAttachments(layer_data *device_data, VkCommandBuffer commandBuffer, uint32_t attachmentCount, +bool PreCallValidateCmdClearAttachments(layer_data *device_data, GLOBAL_CB_NODE *cb_state, uint32_t attachmentCount, const VkClearAttachment *pAttachments, uint32_t rectCount, const VkClearRect *pRects, std::vector *mem_accesses) { - GLOBAL_CB_NODE *cb_state = GetCBNode(device_data, commandBuffer); const debug_report_data *report_data = core_validation::GetReportData(device_data); - + const auto commandBuffer = cb_state->commandBuffer; bool skip = false; - if (cb_state) { - skip |= ValidateCmdQueueFlags(device_data, cb_state, "vkCmdClearAttachments()", VK_QUEUE_GRAPHICS_BIT, - VALIDATION_ERROR_18602415); - skip |= ValidateCmd(device_data, cb_state, CMD_CLEARATTACHMENTS, "vkCmdClearAttachments()"); - // Warn if this is issued prior to Draw Cmd and clearing the entire attachment - if (!cb_state->hasDrawCmd && (cb_state->activeRenderPassBeginInfo.renderArea.extent.width == pRects[0].rect.extent.width) && - (cb_state->activeRenderPassBeginInfo.renderArea.extent.height == pRects[0].rect.extent.height)) { - // There are times where app needs to use ClearAttachments (generally when reusing a buffer inside of a render pass) - // This warning should be made more specific. It'd be best to avoid triggering this test if it's a use that must call - // CmdClearAttachments. - skip |= - log_msg(report_data, VK_DEBUG_REPORT_PERFORMANCE_WARNING_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, + + skip |= + ValidateCmdQueueFlags(device_data, cb_state, "vkCmdClearAttachments()", VK_QUEUE_GRAPHICS_BIT, VALIDATION_ERROR_18602415); + skip |= ValidateCmd(device_data, cb_state, CMD_CLEARATTACHMENTS, "vkCmdClearAttachments()"); + // Warn if this is issued prior to Draw Cmd and clearing the entire attachment + if (!cb_state->hasDrawCmd && (cb_state->activeRenderPassBeginInfo.renderArea.extent.width == pRects[0].rect.extent.width) && + (cb_state->activeRenderPassBeginInfo.renderArea.extent.height == pRects[0].rect.extent.height)) { + // There are times where app needs to use ClearAttachments (generally when reusing a buffer inside of a render pass) + // This warning should be made more specific. It'd be best to avoid triggering this test if it's a use that must call + // CmdClearAttachments. + skip |= log_msg(report_data, VK_DEBUG_REPORT_PERFORMANCE_WARNING_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, HandleToUint64(commandBuffer), 0, DRAWSTATE_CLEAR_CMD_BEFORE_DRAW, "DS", "vkCmdClearAttachments() issued on command buffer object 0x%p prior to any Draw Cmds." " It is recommended you use RenderPass LOAD_OP_CLEAR on Attachments prior to any Draw.", commandBuffer); - } - skip |= outsideRenderPass(device_data, cb_state, "vkCmdClearAttachments()", VALIDATION_ERROR_18600017); } + skip |= outsideRenderPass(device_data, cb_state, "vkCmdClearAttachments()", VALIDATION_ERROR_18600017); // Validate that attachment is in reference list of active subpass if (cb_state->activeRenderPass) { @@ -2217,6 +2214,11 @@ bool PreCallValidateCmdClearAttachments(layer_data *device_data, VkCommandBuffer return skip; } +void PreCallRecordCmdClearAttachments(debug_report_data const *report_data, GLOBAL_CB_NODE *cb_state, + std::vector *mem_accesses) { + AddCommandBufferCommandMemoryAccesses(cb_state, CMD_CLEARATTACHMENTS, mem_accesses); +} + bool PreCallValidateCmdResolveImage(layer_data *device_data, GLOBAL_CB_NODE *cb_node, IMAGE_STATE *src_image_state, IMAGE_STATE *dst_image_state, uint32_t regionCount, const VkImageResolve *pRegions) { const debug_report_data *report_data = core_validation::GetReportData(device_data); diff --git a/layers/buffer_validation.h b/layers/buffer_validation.h index 5f827bf516..b303682c6d 100644 --- a/layers/buffer_validation.h +++ b/layers/buffer_validation.h @@ -154,10 +154,13 @@ bool PreCallValidateCmdCopyImage(layer_data *device_data, GLOBAL_CB_NODE *cb_nod VkImageLayout src_image_layout, VkImageLayout dst_image_layout, std::vector *mem_accesses); -bool PreCallValidateCmdClearAttachments(layer_data *device_data, VkCommandBuffer commandBuffer, uint32_t attachmentCount, +bool PreCallValidateCmdClearAttachments(layer_data *device_data, GLOBAL_CB_NODE *cb_state, uint32_t attachmentCount, const VkClearAttachment *pAttachments, uint32_t rectCount, const VkClearRect *pRects, std::vector *mem_accesses); +void PreCallRecordCmdClearAttachments(debug_report_data const *report_data, GLOBAL_CB_NODE *cb_state, + std::vector *mem_accesses); + bool PreCallValidateCmdResolveImage(layer_data *device_data, GLOBAL_CB_NODE *cb_node, IMAGE_STATE *src_image_state, IMAGE_STATE *dst_image_state, uint32_t regionCount, const VkImageResolve *pRegions); diff --git a/layers/core_validation.cpp b/layers/core_validation.cpp index 7325c6abc6..f0eea440e5 100644 --- a/layers/core_validation.cpp +++ b/layers/core_validation.cpp @@ -6090,13 +6090,19 @@ VKAPI_ATTR void VKAPI_CALL CmdClearAttachments(VkCommandBuffer commandBuffer, ui bool skip = false; layer_data *dev_data = GetLayerDataPtr(get_dispatch_key(commandBuffer), layer_data_map); std::vector mem_accesses; - { - lock_guard_t lock(global_lock); - skip = PreCallValidateCmdClearAttachments(dev_data, commandBuffer, attachmentCount, pAttachments, rectCount, pRects, - &mem_accesses); - } - if (!skip) { - dev_data->dispatch_table.CmdClearAttachments(commandBuffer, attachmentCount, pAttachments, rectCount, pRects); + unique_lock_t lock(global_lock); + auto cb_state = GetCBNode(dev_data, commandBuffer); + if (cb_state) { + skip = + PreCallValidateCmdClearAttachments(dev_data, cb_state, attachmentCount, pAttachments, rectCount, pRects, &mem_accesses); + if (!skip) { + PreCallRecordCmdClearAttachments(dev_data->report_data, cb_state, &mem_accesses); + lock.unlock(); + dev_data->dispatch_table.CmdClearAttachments(commandBuffer, attachmentCount, pAttachments, rectCount, pRects); + } + } else { + lock.unlock(); + assert(0); } } From 69524654d7a37fea93dfaf14d2c7e1451a35bbdc Mon Sep 17 00:00:00 2001 From: Tobin Ehlis Date: Thu, 10 Aug 2017 09:43:27 -0600 Subject: [PATCH 13/28] layers:Add CmdResolveImage memory access tracking Add tracking for the memory accesses in CmdResolveImage. Initially just adding a read & write access for the src & dest image memory that conservatively references whole binding and is recorded as imprecise so that it only will cause warnings. --- layers/buffer_validation.cpp | 49 ++++++++++++++++++++---------------- layers/buffer_validation.h | 5 ++-- layers/core_validation.cpp | 6 +++-- 3 files changed, 35 insertions(+), 25 deletions(-) diff --git a/layers/buffer_validation.cpp b/layers/buffer_validation.cpp index 1eb897f0c1..0d0c61512e 100644 --- a/layers/buffer_validation.cpp +++ b/layers/buffer_validation.cpp @@ -2219,17 +2219,18 @@ void PreCallRecordCmdClearAttachments(debug_report_data const *report_data, GLOB AddCommandBufferCommandMemoryAccesses(cb_state, CMD_CLEARATTACHMENTS, mem_accesses); } -bool PreCallValidateCmdResolveImage(layer_data *device_data, GLOBAL_CB_NODE *cb_node, IMAGE_STATE *src_image_state, - IMAGE_STATE *dst_image_state, uint32_t regionCount, const VkImageResolve *pRegions) { +bool PreCallValidateCmdResolveImage(layer_data *device_data, GLOBAL_CB_NODE *cb_state, IMAGE_STATE *src_image_state, + IMAGE_STATE *dst_image_state, uint32_t regionCount, const VkImageResolve *pRegions, + std::vector *mem_accesses) { const debug_report_data *report_data = core_validation::GetReportData(device_data); bool skip = false; - if (cb_node && src_image_state && dst_image_state) { + if (cb_state && src_image_state && dst_image_state) { skip |= ValidateMemoryIsBoundToImage(device_data, src_image_state, "vkCmdResolveImage()", VALIDATION_ERROR_1c800200); skip |= ValidateMemoryIsBoundToImage(device_data, dst_image_state, "vkCmdResolveImage()", VALIDATION_ERROR_1c800204); skip |= - ValidateCmdQueueFlags(device_data, cb_node, "vkCmdResolveImage()", VK_QUEUE_GRAPHICS_BIT, VALIDATION_ERROR_1c802415); - skip |= ValidateCmd(device_data, cb_node, CMD_RESOLVEIMAGE, "vkCmdResolveImage()"); - skip |= insideRenderPass(device_data, cb_node, "vkCmdResolveImage()", VALIDATION_ERROR_1c800017); + ValidateCmdQueueFlags(device_data, cb_state, "vkCmdResolveImage()", VK_QUEUE_GRAPHICS_BIT, VALIDATION_ERROR_1c802415); + skip |= ValidateCmd(device_data, cb_state, CMD_RESOLVEIMAGE, "vkCmdResolveImage()"); + skip |= insideRenderPass(device_data, cb_state, "vkCmdResolveImage()", VALIDATION_ERROR_1c800017); // For each region, the number of layers in the image subresource should not be zero // For each region, src and dest image aspect must be color only @@ -2237,17 +2238,17 @@ bool PreCallValidateCmdResolveImage(layer_data *device_data, GLOBAL_CB_NODE *cb_ if (pRegions[i].srcSubresource.layerCount == 0) { char const str[] = "vkCmdResolveImage: number of layers in source subresource is zero"; skip |= log_msg(report_data, VK_DEBUG_REPORT_WARNING_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, DRAWSTATE_MISMATCHED_IMAGE_ASPECT, "IMAGE", str); + HandleToUint64(cb_state->commandBuffer), __LINE__, DRAWSTATE_MISMATCHED_IMAGE_ASPECT, "IMAGE", str); } if (pRegions[i].dstSubresource.layerCount == 0) { char const str[] = "vkCmdResolveImage: number of layers in destination subresource is zero"; skip |= log_msg(report_data, VK_DEBUG_REPORT_WARNING_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, DRAWSTATE_MISMATCHED_IMAGE_ASPECT, "IMAGE", str); + HandleToUint64(cb_state->commandBuffer), __LINE__, DRAWSTATE_MISMATCHED_IMAGE_ASPECT, "IMAGE", str); } if (pRegions[i].srcSubresource.layerCount != pRegions[i].dstSubresource.layerCount) { skip |= log_msg( report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, VALIDATION_ERROR_0a200216, "IMAGE", + HandleToUint64(cb_state->commandBuffer), __LINE__, VALIDATION_ERROR_0a200216, "IMAGE", "vkCmdResolveImage: layerCount in source and destination subresource of pRegions[%d] does not match. %s", i, validation_error_map[VALIDATION_ERROR_0a200216]); } @@ -2256,31 +2257,36 @@ bool PreCallValidateCmdResolveImage(layer_data *device_data, GLOBAL_CB_NODE *cb_ char const str[] = "vkCmdResolveImage: src and dest aspectMasks for each region must specify only VK_IMAGE_ASPECT_COLOR_BIT"; skip |= log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, VALIDATION_ERROR_0a200214, "IMAGE", "%s. %s", str, - validation_error_map[VALIDATION_ERROR_0a200214]); + HandleToUint64(cb_state->commandBuffer), __LINE__, VALIDATION_ERROR_0a200214, "IMAGE", "%s. %s", + str, validation_error_map[VALIDATION_ERROR_0a200214]); } } + // Add memory access for this image resolve + // TODO: Currently treating all image accesses as imprecise & covering the whole image area + AddReadMemoryAccess(CMD_RESOLVEIMAGE, mem_accesses, src_image_state->binding, false); + AddWriteMemoryAccess(CMD_RESOLVEIMAGE, mem_accesses, dst_image_state->binding, false); + skip |= ValidateMemoryAccesses(report_data, cb_state, mem_accesses, "vkCmdResolveImage()"); if (src_image_state->createInfo.format != dst_image_state->createInfo.format) { char const str[] = "vkCmdResolveImage called with unmatched source and dest formats."; skip |= log_msg(report_data, VK_DEBUG_REPORT_WARNING_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, DRAWSTATE_MISMATCHED_IMAGE_FORMAT, "IMAGE", str); + HandleToUint64(cb_state->commandBuffer), __LINE__, DRAWSTATE_MISMATCHED_IMAGE_FORMAT, "IMAGE", str); } if (src_image_state->createInfo.imageType != dst_image_state->createInfo.imageType) { char const str[] = "vkCmdResolveImage called with unmatched source and dest image types."; skip |= log_msg(report_data, VK_DEBUG_REPORT_WARNING_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, DRAWSTATE_MISMATCHED_IMAGE_TYPE, "IMAGE", str); + HandleToUint64(cb_state->commandBuffer), __LINE__, DRAWSTATE_MISMATCHED_IMAGE_TYPE, "IMAGE", str); } if (src_image_state->createInfo.samples == VK_SAMPLE_COUNT_1_BIT) { char const str[] = "vkCmdResolveImage called with source sample count less than 2."; skip |= log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, VALIDATION_ERROR_1c800202, "IMAGE", "%s. %s", str, + HandleToUint64(cb_state->commandBuffer), __LINE__, VALIDATION_ERROR_1c800202, "IMAGE", "%s. %s", str, validation_error_map[VALIDATION_ERROR_1c800202]); } if (dst_image_state->createInfo.samples != VK_SAMPLE_COUNT_1_BIT) { char const str[] = "vkCmdResolveImage called with dest sample count greater than 1."; skip |= log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, - HandleToUint64(cb_node->commandBuffer), __LINE__, VALIDATION_ERROR_1c800206, "IMAGE", "%s. %s", str, + HandleToUint64(cb_state->commandBuffer), __LINE__, VALIDATION_ERROR_1c800206, "IMAGE", "%s. %s", str, validation_error_map[VALIDATION_ERROR_1c800206]); } // TODO: Need to validate image layouts, which will include layout validation for shared presentable images @@ -2290,21 +2296,22 @@ bool PreCallValidateCmdResolveImage(layer_data *device_data, GLOBAL_CB_NODE *cb_ return skip; } -void PreCallRecordCmdResolveImage(layer_data *device_data, GLOBAL_CB_NODE *cb_node, IMAGE_STATE *src_image_state, - IMAGE_STATE *dst_image_state) { +void PreCallRecordCmdResolveImage(layer_data *device_data, GLOBAL_CB_NODE *cb_state, IMAGE_STATE *src_image_state, + IMAGE_STATE *dst_image_state, std::vector *mem_accesses) { // Update bindings between images and cmd buffer - AddCommandBufferBindingImage(device_data, cb_node, src_image_state); - AddCommandBufferBindingImage(device_data, cb_node, dst_image_state); + AddCommandBufferBindingImage(device_data, cb_state, src_image_state); + AddCommandBufferBindingImage(device_data, cb_state, dst_image_state); std::function function = [=]() { return ValidateImageMemoryIsValid(device_data, src_image_state, "vkCmdResolveImage()"); }; - cb_node->queue_submit_functions.push_back(function); + cb_state->queue_submit_functions.push_back(function); function = [=]() { SetImageMemoryValid(device_data, dst_image_state, true); return false; }; - cb_node->queue_submit_functions.push_back(function); + cb_state->queue_submit_functions.push_back(function); + AddCommandBufferCommandMemoryAccesses(cb_state, CMD_RESOLVEIMAGE, mem_accesses); } bool PreCallValidateCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_state, IMAGE_STATE *src_image_state, diff --git a/layers/buffer_validation.h b/layers/buffer_validation.h index b303682c6d..19bcad2ed2 100644 --- a/layers/buffer_validation.h +++ b/layers/buffer_validation.h @@ -162,10 +162,11 @@ void PreCallRecordCmdClearAttachments(debug_report_data const *report_data, GLOB std::vector *mem_accesses); bool PreCallValidateCmdResolveImage(layer_data *device_data, GLOBAL_CB_NODE *cb_node, IMAGE_STATE *src_image_state, - IMAGE_STATE *dst_image_state, uint32_t regionCount, const VkImageResolve *pRegions); + IMAGE_STATE *dst_image_state, uint32_t regionCount, const VkImageResolve *pRegions, + std::vector *mem_accesses); void PreCallRecordCmdResolveImage(layer_data *device_data, GLOBAL_CB_NODE *cb_node, IMAGE_STATE *src_image_state, - IMAGE_STATE *dst_image_state); + IMAGE_STATE *dst_image_state, std::vector *mem_accesses); bool PreCallValidateCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_node, IMAGE_STATE *src_image_state, IMAGE_STATE *dst_image_state, uint32_t regionCount, const VkImageBlit *pRegions, VkFilter filter, diff --git a/layers/core_validation.cpp b/layers/core_validation.cpp index f0eea440e5..970e033f86 100644 --- a/layers/core_validation.cpp +++ b/layers/core_validation.cpp @@ -6143,16 +6143,18 @@ VKAPI_ATTR void VKAPI_CALL CmdResolveImage(VkCommandBuffer commandBuffer, VkImag VkImage dstImage, VkImageLayout dstImageLayout, uint32_t regionCount, const VkImageResolve *pRegions) { layer_data *dev_data = GetLayerDataPtr(get_dispatch_key(commandBuffer), layer_data_map); + std::vector mem_accesses; unique_lock_t lock(global_lock); auto cb_node = GetCBNode(dev_data, commandBuffer); auto src_image_state = GetImageState(dev_data, srcImage); auto dst_image_state = GetImageState(dev_data, dstImage); - bool skip = PreCallValidateCmdResolveImage(dev_data, cb_node, src_image_state, dst_image_state, regionCount, pRegions); + bool skip = + PreCallValidateCmdResolveImage(dev_data, cb_node, src_image_state, dst_image_state, regionCount, pRegions, &mem_accesses); if (!skip) { - PreCallRecordCmdResolveImage(dev_data, cb_node, src_image_state, dst_image_state); + PreCallRecordCmdResolveImage(dev_data, cb_node, src_image_state, dst_image_state, &mem_accesses); lock.unlock(); dev_data->dispatch_table.CmdResolveImage(commandBuffer, srcImage, srcImageLayout, dstImage, dstImageLayout, regionCount, pRegions); From e9d7e6975ae46ccfbe350b62f86e7d73adddb07c Mon Sep 17 00:00:00 2001 From: Tobin Ehlis Date: Thu, 10 Aug 2017 10:34:23 -0600 Subject: [PATCH 14/28] layers:Refactor CmdCopyQueryPoolResults Pre/Post Just moving code to prep for adding mem access support to CmdCopyQueryPoolResults. Refactor it to use the common PreValidate* & PostRecord* pattern. --- layers/core_validation.cpp | 71 +++++++++++++++++++++----------------- 1 file changed, 40 insertions(+), 31 deletions(-) diff --git a/layers/core_validation.cpp b/layers/core_validation.cpp index 970e033f86..f7fc421352 100644 --- a/layers/core_validation.cpp +++ b/layers/core_validation.cpp @@ -7141,6 +7141,33 @@ static bool validateQuery(VkQueue queue, GLOBAL_CB_NODE *pCB, VkQueryPool queryP return skip; } +static bool PreCallValidateCmdCopyQueryPoolResults(layer_data *device_data, GLOBAL_CB_NODE *cb_state, VkQueryPool queryPool, + uint32_t firstQuery, uint32_t queryCount, BUFFER_STATE *dst_buffer_state, + VkDeviceSize dstOffset, VkDeviceSize stride) { + bool skip = false; + skip |= ValidateMemoryIsBoundToBuffer(device_data, dst_buffer_state, "vkCmdCopyQueryPoolResults()", VALIDATION_ERROR_19400674); + // Validate that DST buffer has correct usage flags set + skip |= ValidateBufferUsageFlags(device_data, dst_buffer_state, VK_BUFFER_USAGE_TRANSFER_DST_BIT, true, + VALIDATION_ERROR_19400672, "vkCmdCopyQueryPoolResults()", "VK_BUFFER_USAGE_TRANSFER_DST_BIT"); + skip |= ValidateCmdQueueFlags(device_data, cb_state, "vkCmdCopyQueryPoolResults()", + VK_QUEUE_GRAPHICS_BIT | VK_QUEUE_COMPUTE_BIT, VALIDATION_ERROR_19402415); + skip |= ValidateCmd(device_data, cb_state, CMD_COPYQUERYPOOLRESULTS, "vkCmdCopyQueryPoolResults()"); + skip |= insideRenderPass(device_data, cb_state, "vkCmdCopyQueryPoolResults()", VALIDATION_ERROR_19400017); + return skip; +} + +static void PreCallRecordCmdCopyQueryPoolResults(layer_data *device_data, GLOBAL_CB_NODE *cb_state, VkQueryPool queryPool, + uint32_t firstQuery, uint32_t queryCount, BUFFER_STATE *dst_buffer_state) { + AddCommandBufferBindingBuffer(device_data, cb_state, dst_buffer_state); + cb_state->queue_submit_functions.emplace_back([=]() { + SetBufferMemoryValid(device_data, dst_buffer_state, true); + return false; + }); + cb_state->queryUpdates.emplace_back([=](VkQueue q) { return validateQuery(q, cb_state, queryPool, firstQuery, queryCount); }); + addCommandBufferBinding(&GetQueryPoolNode(device_data, queryPool)->cb_bindings, + {HandleToUint64(queryPool), kVulkanObjectTypeQueryPool}, cb_state); +} + VKAPI_ATTR void VKAPI_CALL CmdCopyQueryPoolResults(VkCommandBuffer commandBuffer, VkQueryPool queryPool, uint32_t firstQuery, uint32_t queryCount, VkBuffer dstBuffer, VkDeviceSize dstOffset, VkDeviceSize stride, VkQueryResultFlags flags) { @@ -7148,38 +7175,20 @@ VKAPI_ATTR void VKAPI_CALL CmdCopyQueryPoolResults(VkCommandBuffer commandBuffer layer_data *dev_data = GetLayerDataPtr(get_dispatch_key(commandBuffer), layer_data_map); unique_lock_t lock(global_lock); - auto cb_node = GetCBNode(dev_data, commandBuffer); + auto cb_state = GetCBNode(dev_data, commandBuffer); auto dst_buff_state = GetBufferState(dev_data, dstBuffer); - if (cb_node && dst_buff_state) { - skip |= ValidateMemoryIsBoundToBuffer(dev_data, dst_buff_state, "vkCmdCopyQueryPoolResults()", VALIDATION_ERROR_19400674); - // Validate that DST buffer has correct usage flags set - skip |= - ValidateBufferUsageFlags(dev_data, dst_buff_state, VK_BUFFER_USAGE_TRANSFER_DST_BIT, true, VALIDATION_ERROR_19400672, - "vkCmdCopyQueryPoolResults()", "VK_BUFFER_USAGE_TRANSFER_DST_BIT"); - skip |= ValidateCmdQueueFlags(dev_data, cb_node, "vkCmdCopyQueryPoolResults()", - VK_QUEUE_GRAPHICS_BIT | VK_QUEUE_COMPUTE_BIT, VALIDATION_ERROR_19402415); - skip |= ValidateCmd(dev_data, cb_node, CMD_COPYQUERYPOOLRESULTS, "vkCmdCopyQueryPoolResults()"); - skip |= insideRenderPass(dev_data, cb_node, "vkCmdCopyQueryPoolResults()", VALIDATION_ERROR_19400017); - } - lock.unlock(); - - if (skip) return; - - dev_data->dispatch_table.CmdCopyQueryPoolResults(commandBuffer, queryPool, firstQuery, queryCount, dstBuffer, dstOffset, - stride, flags); - - lock.lock(); - if (cb_node && dst_buff_state) { - AddCommandBufferBindingBuffer(dev_data, cb_node, dst_buff_state); - cb_node->queue_submit_functions.emplace_back([=]() { - SetBufferMemoryValid(dev_data, dst_buff_state, true); - return false; - }); - cb_node->queryUpdates.emplace_back([=](VkQueue q) { - return validateQuery(q, cb_node, queryPool, firstQuery, queryCount); - }); - addCommandBufferBinding(&GetQueryPoolNode(dev_data, queryPool)->cb_bindings, - {HandleToUint64(queryPool), kVulkanObjectTypeQueryPool}, cb_node); + if (cb_state && dst_buff_state) { + skip |= PreCallValidateCmdCopyQueryPoolResults(dev_data, cb_state, queryPool, firstQuery, queryCount, dst_buff_state, + dstOffset, stride); + if (!skip) { + PreCallRecordCmdCopyQueryPoolResults(dev_data, cb_state, queryPool, firstQuery, queryCount, dst_buff_state); + lock.unlock(); + dev_data->dispatch_table.CmdCopyQueryPoolResults(commandBuffer, queryPool, firstQuery, queryCount, dstBuffer, dstOffset, + stride, flags); + } + } else { + lock.unlock(); + assert(0); } } From e8f3e93116172c469188ce5df027f3e538cd275e Mon Sep 17 00:00:00 2001 From: Tobin Ehlis Date: Thu, 10 Aug 2017 13:45:23 -0600 Subject: [PATCH 15/28] layers:Add mem access for CmdCopyQueryPoolResults Add the buffer write access for CmdCopyQueryPoolResults. Updated query pool data struct to store number of elements in the query based on query type and then use that number when calculating total data size. --- layers/core_validation.cpp | 32 +++++++++++++++++++++++++++----- layers/core_validation.h | 1 + 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/layers/core_validation.cpp b/layers/core_validation.cpp index f7fc421352..4e345ef2aa 100644 --- a/layers/core_validation.cpp +++ b/layers/core_validation.cpp @@ -4004,6 +4004,13 @@ VKAPI_ATTR VkResult VKAPI_CALL CreateQueryPool(VkDevice device, const VkQueryPoo lock_guard_t lock(global_lock); QUERY_POOL_NODE *qp_node = &dev_data->queryPoolMap[*pQueryPool]; qp_node->createInfo = *pCreateInfo; + // Count data elements per query for later size calculation + size_t data_count = 1; + if (VK_QUERY_TYPE_PIPELINE_STATISTICS == pCreateInfo->queryType) { + std::bitset<32> stat_bits(pCreateInfo->pipelineStatistics); + data_count = stat_bits.count(); + } + qp_node->data_count = data_count; } return result; } @@ -7142,8 +7149,9 @@ static bool validateQuery(VkQueue queue, GLOBAL_CB_NODE *pCB, VkQueryPool queryP } static bool PreCallValidateCmdCopyQueryPoolResults(layer_data *device_data, GLOBAL_CB_NODE *cb_state, VkQueryPool queryPool, - uint32_t firstQuery, uint32_t queryCount, BUFFER_STATE *dst_buffer_state, - VkDeviceSize dstOffset, VkDeviceSize stride) { + uint32_t firstQuery, uint32_t query_count, BUFFER_STATE *dst_buffer_state, + VkDeviceSize offset, VkDeviceSize stride, VkQueryResultFlags flags, + std::vector *mem_accesses) { bool skip = false; skip |= ValidateMemoryIsBoundToBuffer(device_data, dst_buffer_state, "vkCmdCopyQueryPoolResults()", VALIDATION_ERROR_19400674); // Validate that DST buffer has correct usage flags set @@ -7153,11 +7161,22 @@ static bool PreCallValidateCmdCopyQueryPoolResults(layer_data *device_data, GLOB VK_QUEUE_GRAPHICS_BIT | VK_QUEUE_COMPUTE_BIT, VALIDATION_ERROR_19402415); skip |= ValidateCmd(device_data, cb_state, CMD_COPYQUERYPOOLRESULTS, "vkCmdCopyQueryPoolResults()"); skip |= insideRenderPass(device_data, cb_state, "vkCmdCopyQueryPoolResults()", VALIDATION_ERROR_19400017); + // Add mem write to buffer. + // TODO: Initially if data is not tightly packed, just mark as imprecise and cover whole as written + // we could make this case precise by splitting into a number of small accesses + auto const &qp_state = GetQueryPoolNode(device_data, queryPool); + auto num_bytes = (VK_QUERY_RESULT_64_BIT & flags) ? 8 : 4; + auto element_size = qp_state->data_count * num_bytes; + auto precise = (element_size == stride) ? true : false; + auto size = stride * (query_count - 1) + num_bytes; + AddWriteMemoryAccess(CMD_COPYQUERYPOOLRESULTS, mem_accesses, {dst_buffer_state->binding.mem, offset, size}, precise); + skip |= ValidateMemoryAccesses(device_data->report_data, cb_state, mem_accesses, "vkCmdCopyQueryPoolResults()"); return skip; } static void PreCallRecordCmdCopyQueryPoolResults(layer_data *device_data, GLOBAL_CB_NODE *cb_state, VkQueryPool queryPool, - uint32_t firstQuery, uint32_t queryCount, BUFFER_STATE *dst_buffer_state) { + uint32_t firstQuery, uint32_t queryCount, BUFFER_STATE *dst_buffer_state, + std::vector *mem_accesses) { AddCommandBufferBindingBuffer(device_data, cb_state, dst_buffer_state); cb_state->queue_submit_functions.emplace_back([=]() { SetBufferMemoryValid(device_data, dst_buffer_state, true); @@ -7166,6 +7185,7 @@ static void PreCallRecordCmdCopyQueryPoolResults(layer_data *device_data, GLOBAL cb_state->queryUpdates.emplace_back([=](VkQueue q) { return validateQuery(q, cb_state, queryPool, firstQuery, queryCount); }); addCommandBufferBinding(&GetQueryPoolNode(device_data, queryPool)->cb_bindings, {HandleToUint64(queryPool), kVulkanObjectTypeQueryPool}, cb_state); + AddCommandBufferCommandMemoryAccesses(cb_state, CMD_COPYQUERYPOOLRESULTS, mem_accesses); } VKAPI_ATTR void VKAPI_CALL CmdCopyQueryPoolResults(VkCommandBuffer commandBuffer, VkQueryPool queryPool, uint32_t firstQuery, @@ -7173,15 +7193,17 @@ VKAPI_ATTR void VKAPI_CALL CmdCopyQueryPoolResults(VkCommandBuffer commandBuffer VkDeviceSize stride, VkQueryResultFlags flags) { bool skip = false; layer_data *dev_data = GetLayerDataPtr(get_dispatch_key(commandBuffer), layer_data_map); + std::vector mem_accesses; unique_lock_t lock(global_lock); auto cb_state = GetCBNode(dev_data, commandBuffer); auto dst_buff_state = GetBufferState(dev_data, dstBuffer); if (cb_state && dst_buff_state) { skip |= PreCallValidateCmdCopyQueryPoolResults(dev_data, cb_state, queryPool, firstQuery, queryCount, dst_buff_state, - dstOffset, stride); + dstOffset, stride, flags, &mem_accesses); if (!skip) { - PreCallRecordCmdCopyQueryPoolResults(dev_data, cb_state, queryPool, firstQuery, queryCount, dst_buff_state); + PreCallRecordCmdCopyQueryPoolResults(dev_data, cb_state, queryPool, firstQuery, queryCount, dst_buff_state, + &mem_accesses); lock.unlock(); dev_data->dispatch_table.CmdCopyQueryPoolResults(commandBuffer, queryPool, firstQuery, queryCount, dstBuffer, dstOffset, stride, flags); diff --git a/layers/core_validation.h b/layers/core_validation.h index 460dba3736..aba7a8d118 100644 --- a/layers/core_validation.h +++ b/layers/core_validation.h @@ -138,6 +138,7 @@ class QUEUE_STATE { class QUERY_POOL_NODE : public BASE_NODE { public: VkQueryPoolCreateInfo createInfo; + size_t data_count; // Number of data items in this query }; struct PHYSICAL_DEVICE_STATE { From a780e83468c6e7500e774193f4162fae66e4d0c9 Mon Sep 17 00:00:00 2001 From: Tobin Ehlis Date: Fri, 11 Aug 2017 08:25:33 -0600 Subject: [PATCH 16/28] layers:Track draw/dispatch mem access Update the Draw/Dispatch validate/update framework to account for all memory accesses through descriptors. At validate time grab the complete set of active read and write memory bindings from descriptors. Then verify the memory accesses to flag any conflicts. During state update, record all of the accesses into the cmd buffer state. All of these accesses are imprecise and will only result in warnings if there is a potential synch issue. --- layers/buffer_validation.cpp | 7 +- layers/core_validation.cpp | 225 +++++++++++++++++++++------------ layers/core_validation_types.h | 24 +++- layers/descriptor_sets.cpp | 37 +++++- layers/descriptor_sets.h | 6 +- 5 files changed, 199 insertions(+), 100 deletions(-) diff --git a/layers/buffer_validation.cpp b/layers/buffer_validation.cpp index 0d0c61512e..6b03e39887 100644 --- a/layers/buffer_validation.cpp +++ b/layers/buffer_validation.cpp @@ -641,7 +641,6 @@ void AddMemoryAccess(CMD_TYPE cmd, std::vector *mem_accesses, Memo mem_access->write = write; mem_access->src_stage_flags = CommandToFlags[cmd][rw_index].stage_flags; mem_access->src_access_flags = CommandToFlags[cmd][rw_index].access_flags; - // TODO: If dynamic true lookup dynamic flag status mem_accesses->emplace_back(*mem_access); } @@ -695,8 +694,8 @@ bool ValidateMemoryAccesses(debug_report_data const *report_data, GLOBAL_CB_NODE bool skip = false; for (const auto &mem_access : *mem_accesses) { auto mem_obj = mem_access.location.mem; - auto mem_access_pair = cb_state->memory_accesses.find(mem_obj); - if (mem_access_pair != cb_state->memory_accesses.end()) { + auto mem_access_pair = cb_state->mem_accesses.access_map.find(mem_obj); + if (mem_access_pair != cb_state->mem_accesses.access_map.end()) { for (const auto &earlier_access : mem_access_pair->second) { if (MemoryConflict(&earlier_access, &mem_access)) { const char *access1 = earlier_access.write ? "write" : "read"; @@ -724,7 +723,7 @@ void AddCommandBufferCommandMemoryAccesses(GLOBAL_CB_NODE *cb_state, CMD_TYPE cm for (auto &mem_access : *mem_accesses) { mem_access.cmd = cmd_ptr; cmd_ptr->AddMemoryAccess(mem_access); - cb_state->memory_accesses[mem_access.location.mem].push_back(mem_access); + cb_state->mem_accesses.access_map[mem_access.location.mem].push_back(mem_access); } } diff --git a/layers/core_validation.cpp b/layers/core_validation.cpp index 4e345ef2aa..b09beddf8c 100644 --- a/layers/core_validation.cpp +++ b/layers/core_validation.cpp @@ -1145,8 +1145,6 @@ static void UpdateDrawState(layer_data *dev_data, GLOBAL_CB_NODE *cb_state, cons cvdescriptorset::DescriptorSet *descriptor_set = state.boundDescriptorSets[setIndex]; // Bind this set and its active descriptor resources to the command buffer descriptor_set->BindCommandBuffer(cb_state, set_binding_pair.second); - // For given active slots record updated images & buffers - descriptor_set->GetStorageUpdates(set_binding_pair.second, &cb_state->updateBuffers, &cb_state->updateImages); } } if (pPipe->vertexBindingDescriptions.size() > 0) { @@ -1727,7 +1725,7 @@ static void resetCB(layer_data *dev_data, const VkCommandBuffer cb) { pCB->broken_bindings.clear(); pCB->commands.clear(); pCB->synch_commands.clear(); - pCB->memory_accesses.clear(); + pCB->mem_accesses.reset(); pCB->waitedEvents.clear(); pCB->events.clear(); pCB->writeEventsBeforeWait.clear(); @@ -1753,8 +1751,6 @@ static void resetCB(layer_data *dev_data, const VkCommandBuffer cb) { pSubCB->linkedCommandBuffers.erase(pCB); } pCB->linkedCommandBuffers.clear(); - pCB->updateImages.clear(); - pCB->updateBuffers.clear(); clear_cmd_buf_and_mem_references(dev_data, pCB); pCB->queue_submit_functions.clear(); pCB->cmd_execute_commands_functions.clear(); @@ -5632,8 +5628,10 @@ VKAPI_ATTR void VKAPI_CALL CmdBindVertexBuffers(VkCommandBuffer commandBuffer, u } // Expects global_lock to be held by caller -static void MarkStoreImagesAndBuffersAsWritten(layer_data *dev_data, GLOBAL_CB_NODE *pCB) { - for (auto imageView : pCB->updateImages) { +// Mark write buffers & images as valid & record all of the memory accesses into access_map for CB +static void UpdateDrawMemoryAccessState(layer_data *dev_data, GLOBAL_CB_NODE *cb_state, CMD_TYPE cmd, + std::vector *mem_accesses) { + for (auto imageView : cb_state->mem_accesses.write_images) { auto view_state = GetImageViewState(dev_data, imageView); if (!view_state) continue; @@ -5643,24 +5641,62 @@ static void MarkStoreImagesAndBuffersAsWritten(layer_data *dev_data, GLOBAL_CB_N SetImageMemoryValid(dev_data, image_state, true); return false; }; - pCB->queue_submit_functions.push_back(function); + cb_state->queue_submit_functions.push_back(function); } - for (auto buffer : pCB->updateBuffers) { + for (auto buffer : cb_state->mem_accesses.write_buffers) { auto buffer_state = GetBufferState(dev_data, buffer); assert(buffer_state); std::function function = [=]() { SetBufferMemoryValid(dev_data, buffer_state, true); return false; }; - pCB->queue_submit_functions.push_back(function); + cb_state->queue_submit_functions.push_back(function); + } + AddCommandBufferCommandMemoryAccesses(cb_state, cmd, mem_accesses); +} + +// Update mem_access_vector from r/w buffer/image sets in given DrawDispatchAccesses struct +static void UpdateMemoryAccessVector(layer_data *device_data, CMD_TYPE cmd, std::vector *mem_access_vector, + const DrawDispatchAccesses &dd_mem_accesses) { + // Parse through r/w sets and conservatively add MemAccesses across whole bindings + // TODO: Would like to limit these to actual size of updates where possible + for (const auto buffer : dd_mem_accesses.read_buffers) { + const auto &buff_state = GetBufferState(device_data, buffer); + if (buff_state) { + AddReadMemoryAccess(cmd, mem_access_vector, buff_state->binding, false); + } + } + for (const auto iv : dd_mem_accesses.read_images) { + const auto &iv_state = GetImageViewState(device_data, iv); + if (iv_state) { + const auto &img_state = GetImageState(device_data, iv_state->create_info.image); + if (img_state) { + AddReadMemoryAccess(cmd, mem_access_vector, img_state->binding, false); + } + } + } + for (const auto buffer : dd_mem_accesses.write_buffers) { + const auto &buff_state = GetBufferState(device_data, buffer); + if (buff_state) { + AddWriteMemoryAccess(cmd, mem_access_vector, buff_state->binding, false); + } + } + for (const auto iv : dd_mem_accesses.write_images) { + const auto &iv_state = GetImageViewState(device_data, iv); + if (iv_state) { + const auto &img_state = GetImageState(device_data, iv_state->create_info.image); + if (img_state) { + AddWriteMemoryAccess(cmd, mem_access_vector, img_state->binding, false); + } + } } } // Generic function to handle validation for all CmdDraw* type functions static bool ValidateCmdDrawType(layer_data *dev_data, VkCommandBuffer cmd_buffer, bool indexed, VkPipelineBindPoint bind_point, - CMD_TYPE cmd_type, GLOBAL_CB_NODE **cb_state, const char *caller, VkQueueFlags queue_flags, - UNIQUE_VALIDATION_ERROR_CODE queue_flag_code, UNIQUE_VALIDATION_ERROR_CODE msg_code, - UNIQUE_VALIDATION_ERROR_CODE const dynamic_state_msg_code) { + CMD_TYPE cmd_type, GLOBAL_CB_NODE **cb_state, std::vector *mem_accesses, + const char *caller, VkQueueFlags queue_flags, UNIQUE_VALIDATION_ERROR_CODE queue_flag_code, + UNIQUE_VALIDATION_ERROR_CODE msg_code, UNIQUE_VALIDATION_ERROR_CODE const dynamic_state_msg_code) { bool skip = false; *cb_state = GetCBNode(dev_data, cmd_buffer); if (*cb_state) { @@ -5669,71 +5705,96 @@ static bool ValidateCmdDrawType(layer_data *dev_data, VkCommandBuffer cmd_buffer skip |= ValidateDrawState(dev_data, *cb_state, indexed, bind_point, caller, dynamic_state_msg_code); skip |= (VK_PIPELINE_BIND_POINT_GRAPHICS == bind_point) ? outsideRenderPass(dev_data, *cb_state, caller, msg_code) : insideRenderPass(dev_data, *cb_state, caller, msg_code); + // Grab mem accesses for this draw & check for missing synchs + auto const &state = (*cb_state)->lastBound[bind_point]; + PIPELINE_STATE *pPipe = state.pipeline_state; + if (VK_NULL_HANDLE != state.pipeline_layout.layout) { + DrawDispatchAccesses dd_mem_accesses; + for (const auto &set_binding_pair : pPipe->active_slots) { + uint32_t setIndex = set_binding_pair.first; + // Pull the set node + cvdescriptorset::DescriptorSet *descriptor_set = state.boundDescriptorSets[setIndex]; + if (descriptor_set) { + // For given active slots record updated images & buffers + descriptor_set->GetReadWriteBuffersAndImages(set_binding_pair.second, &dd_mem_accesses.read_buffers, + &dd_mem_accesses.read_images, &dd_mem_accesses.write_buffers, + &dd_mem_accesses.write_images); + UpdateMemoryAccessVector(dev_data, cmd_type, mem_accesses, dd_mem_accesses); + } + } + skip |= ValidateMemoryAccesses(dev_data->report_data, *cb_state, mem_accesses, caller); + } } return skip; } // Generic function to handle state update for all CmdDraw* and CmdDispatch* type functions -static void UpdateStateCmdDrawDispatchType(layer_data *dev_data, GLOBAL_CB_NODE *cb_state, VkPipelineBindPoint bind_point) { +static void UpdateStateCmdDrawDispatchType(layer_data *dev_data, GLOBAL_CB_NODE *cb_state, CMD_TYPE cmd, + VkPipelineBindPoint bind_point, std::vector *mem_accesses) { UpdateDrawState(dev_data, cb_state, bind_point); - MarkStoreImagesAndBuffersAsWritten(dev_data, cb_state); + UpdateDrawMemoryAccessState(dev_data, cb_state, cmd, mem_accesses); } // Generic function to handle state update for all CmdDraw* type functions -static void UpdateStateCmdDrawType(layer_data *dev_data, GLOBAL_CB_NODE *cb_state, VkPipelineBindPoint bind_point) { - UpdateStateCmdDrawDispatchType(dev_data, cb_state, bind_point); +static void UpdateStateCmdDrawType(layer_data *dev_data, GLOBAL_CB_NODE *cb_state, CMD_TYPE cmd, VkPipelineBindPoint bind_point, + std::vector *mem_accesses) { + UpdateStateCmdDrawDispatchType(dev_data, cb_state, cmd, bind_point, mem_accesses); updateResourceTrackingOnDraw(cb_state); cb_state->hasDrawCmd = true; } static bool PreCallValidateCmdDraw(layer_data *dev_data, VkCommandBuffer cmd_buffer, bool indexed, VkPipelineBindPoint bind_point, - GLOBAL_CB_NODE **cb_state, const char *caller) { - return ValidateCmdDrawType(dev_data, cmd_buffer, indexed, bind_point, CMD_DRAW, cb_state, caller, VK_QUEUE_GRAPHICS_BIT, - VALIDATION_ERROR_1a202415, VALIDATION_ERROR_1a200017, VALIDATION_ERROR_1a200376); + GLOBAL_CB_NODE **cb_state, std::vector *mem_accesses, const char *caller) { + return ValidateCmdDrawType(dev_data, cmd_buffer, indexed, bind_point, CMD_DRAW, cb_state, mem_accesses, caller, + VK_QUEUE_GRAPHICS_BIT, VALIDATION_ERROR_1a202415, VALIDATION_ERROR_1a200017, + VALIDATION_ERROR_1a200376); } -static void PostCallRecordCmdDraw(layer_data *dev_data, GLOBAL_CB_NODE *cb_state, VkPipelineBindPoint bind_point) { - UpdateStateCmdDrawType(dev_data, cb_state, bind_point); +static void PreCallRecordCmdDraw(layer_data *dev_data, GLOBAL_CB_NODE *cb_state, CMD_TYPE cmd, VkPipelineBindPoint bind_point, + std::vector *mem_accesses) { + UpdateStateCmdDrawType(dev_data, cb_state, cmd, bind_point, mem_accesses); } VKAPI_ATTR void VKAPI_CALL CmdDraw(VkCommandBuffer commandBuffer, uint32_t vertexCount, uint32_t instanceCount, uint32_t firstVertex, uint32_t firstInstance) { layer_data *dev_data = GetLayerDataPtr(get_dispatch_key(commandBuffer), layer_data_map); GLOBAL_CB_NODE *cb_state = nullptr; + std::vector mem_accesses; unique_lock_t lock(global_lock); - bool skip = PreCallValidateCmdDraw(dev_data, commandBuffer, false, VK_PIPELINE_BIND_POINT_GRAPHICS, &cb_state, "vkCmdDraw()"); - lock.unlock(); + bool skip = PreCallValidateCmdDraw(dev_data, commandBuffer, false, VK_PIPELINE_BIND_POINT_GRAPHICS, &cb_state, &mem_accesses, + "vkCmdDraw()"); if (!skip) { - dev_data->dispatch_table.CmdDraw(commandBuffer, vertexCount, instanceCount, firstVertex, firstInstance); - lock.lock(); - PostCallRecordCmdDraw(dev_data, cb_state, VK_PIPELINE_BIND_POINT_GRAPHICS); + PreCallRecordCmdDraw(dev_data, cb_state, CMD_DRAW, VK_PIPELINE_BIND_POINT_GRAPHICS, &mem_accesses); lock.unlock(); + dev_data->dispatch_table.CmdDraw(commandBuffer, vertexCount, instanceCount, firstVertex, firstInstance); } } static bool PreCallValidateCmdDrawIndexed(layer_data *dev_data, VkCommandBuffer cmd_buffer, bool indexed, - VkPipelineBindPoint bind_point, GLOBAL_CB_NODE **cb_state, const char *caller) { - return ValidateCmdDrawType(dev_data, cmd_buffer, indexed, bind_point, CMD_DRAWINDEXED, cb_state, caller, VK_QUEUE_GRAPHICS_BIT, - VALIDATION_ERROR_1a402415, VALIDATION_ERROR_1a400017, VALIDATION_ERROR_1a40039c); + VkPipelineBindPoint bind_point, GLOBAL_CB_NODE **cb_state, + std::vector *mem_accesses, const char *caller) { + return ValidateCmdDrawType(dev_data, cmd_buffer, indexed, bind_point, CMD_DRAWINDEXED, cb_state, mem_accesses, caller, + VK_QUEUE_GRAPHICS_BIT, VALIDATION_ERROR_1a402415, VALIDATION_ERROR_1a400017, + VALIDATION_ERROR_1a40039c); } -static void PostCallRecordCmdDrawIndexed(layer_data *dev_data, GLOBAL_CB_NODE *cb_state, VkPipelineBindPoint bind_point) { - UpdateStateCmdDrawType(dev_data, cb_state, bind_point); +static void PreCallRecordCmdDrawIndexed(layer_data *dev_data, GLOBAL_CB_NODE *cb_state, CMD_TYPE cmd, + VkPipelineBindPoint bind_point, std::vector *mem_accesses) { + UpdateStateCmdDrawType(dev_data, cb_state, cmd, bind_point, mem_accesses); } VKAPI_ATTR void VKAPI_CALL CmdDrawIndexed(VkCommandBuffer commandBuffer, uint32_t indexCount, uint32_t instanceCount, uint32_t firstIndex, int32_t vertexOffset, uint32_t firstInstance) { layer_data *dev_data = GetLayerDataPtr(get_dispatch_key(commandBuffer), layer_data_map); GLOBAL_CB_NODE *cb_state = nullptr; + std::vector mem_accesses; unique_lock_t lock(global_lock); bool skip = PreCallValidateCmdDrawIndexed(dev_data, commandBuffer, true, VK_PIPELINE_BIND_POINT_GRAPHICS, &cb_state, - "vkCmdDrawIndexed()"); - lock.unlock(); + &mem_accesses, "vkCmdDrawIndexed()"); if (!skip) { - dev_data->dispatch_table.CmdDrawIndexed(commandBuffer, indexCount, instanceCount, firstIndex, vertexOffset, firstInstance); - lock.lock(); - PostCallRecordCmdDrawIndexed(dev_data, cb_state, VK_PIPELINE_BIND_POINT_GRAPHICS); + PreCallRecordCmdDrawIndexed(dev_data, cb_state, CMD_DRAWINDEXED, VK_PIPELINE_BIND_POINT_GRAPHICS, &mem_accesses); lock.unlock(); + dev_data->dispatch_table.CmdDrawIndexed(commandBuffer, indexCount, instanceCount, firstIndex, vertexOffset, firstInstance); } } @@ -5742,8 +5803,8 @@ static bool PreCallValidateCmdDrawIndirect(layer_data *dev_data, VkCommandBuffer std::vector *mem_accesses, VkDeviceSize offset, uint32_t count, uint32_t stride, const char *caller) { bool skip = - ValidateCmdDrawType(dev_data, cmd_buffer, indexed, bind_point, CMD_DRAWINDIRECT, cb_state, caller, VK_QUEUE_GRAPHICS_BIT, - VALIDATION_ERROR_1aa02415, VALIDATION_ERROR_1aa00017, VALIDATION_ERROR_1aa003cc); + ValidateCmdDrawType(dev_data, cmd_buffer, indexed, bind_point, CMD_DRAWINDIRECT, cb_state, mem_accesses, caller, + VK_QUEUE_GRAPHICS_BIT, VALIDATION_ERROR_1aa02415, VALIDATION_ERROR_1aa00017, VALIDATION_ERROR_1aa003cc); *buffer_state = GetBufferState(dev_data, buffer); skip |= ValidateMemoryIsBoundToBuffer(dev_data, *buffer_state, caller, VALIDATION_ERROR_1aa003b4); // TODO: This is temp code to test specific case that needs to be generalized for memory dependency checks @@ -5759,12 +5820,10 @@ static bool PreCallValidateCmdDrawIndirect(layer_data *dev_data, VkCommandBuffer return skip; } -static void PostCallRecordCmdDrawIndirect(layer_data *dev_data, GLOBAL_CB_NODE *cb_state, VkPipelineBindPoint bind_point, - BUFFER_STATE *buffer_state, std::vector *mem_accesses) { - UpdateStateCmdDrawType(dev_data, cb_state, bind_point); +static void PreCallRecordCmdDrawIndirect(layer_data *dev_data, GLOBAL_CB_NODE *cb_state, VkPipelineBindPoint bind_point, + BUFFER_STATE *buffer_state, std::vector *mem_accesses) { + UpdateStateCmdDrawType(dev_data, cb_state, CMD_DRAWINDIRECT, bind_point, mem_accesses); AddCommandBufferBindingBuffer(dev_data, cb_state, buffer_state); - // TODO: MemoryAccess can be merged with memory binding tracking - AddCommandBufferCommandMemoryAccesses(cb_state, CMD_DRAWINDIRECT, mem_accesses); } VKAPI_ATTR void VKAPI_CALL CmdDrawIndirect(VkCommandBuffer commandBuffer, VkBuffer buffer, VkDeviceSize offset, uint32_t count, @@ -5776,20 +5835,19 @@ VKAPI_ATTR void VKAPI_CALL CmdDrawIndirect(VkCommandBuffer commandBuffer, VkBuff unique_lock_t lock(global_lock); bool skip = PreCallValidateCmdDrawIndirect(dev_data, commandBuffer, buffer, false, VK_PIPELINE_BIND_POINT_GRAPHICS, &cb_state, &buffer_state, &mem_accesses, offset, count, stride, "vkCmdDrawIndirect()"); - lock.unlock(); if (!skip) { - dev_data->dispatch_table.CmdDrawIndirect(commandBuffer, buffer, offset, count, stride); - lock.lock(); - PostCallRecordCmdDrawIndirect(dev_data, cb_state, VK_PIPELINE_BIND_POINT_GRAPHICS, buffer_state, &mem_accesses); + PreCallRecordCmdDrawIndirect(dev_data, cb_state, VK_PIPELINE_BIND_POINT_GRAPHICS, buffer_state, &mem_accesses); lock.unlock(); + dev_data->dispatch_table.CmdDrawIndirect(commandBuffer, buffer, offset, count, stride); } } static bool PreCallValidateCmdDrawIndexedIndirect(layer_data *dev_data, VkCommandBuffer cmd_buffer, VkBuffer buffer, bool indexed, VkPipelineBindPoint bind_point, GLOBAL_CB_NODE **cb_state, - BUFFER_STATE **buffer_state, const char *caller) { + std::vector *mem_accesses, BUFFER_STATE **buffer_state, + const char *caller) { bool skip = - ValidateCmdDrawType(dev_data, cmd_buffer, indexed, bind_point, CMD_DRAWINDEXEDINDIRECT, cb_state, caller, + ValidateCmdDrawType(dev_data, cmd_buffer, indexed, bind_point, CMD_DRAWINDEXEDINDIRECT, cb_state, mem_accesses, caller, VK_QUEUE_GRAPHICS_BIT, VALIDATION_ERROR_1a602415, VALIDATION_ERROR_1a600017, VALIDATION_ERROR_1a600434); *buffer_state = GetBufferState(dev_data, buffer); skip |= ValidateMemoryIsBoundToBuffer(dev_data, *buffer_state, caller, VALIDATION_ERROR_1a60041c); @@ -5799,9 +5857,9 @@ static bool PreCallValidateCmdDrawIndexedIndirect(layer_data *dev_data, VkComman return skip; } -static void PostCallRecordCmdDrawIndexedIndirect(layer_data *dev_data, GLOBAL_CB_NODE *cb_state, VkPipelineBindPoint bind_point, - BUFFER_STATE *buffer_state) { - UpdateStateCmdDrawType(dev_data, cb_state, bind_point); +static void PreCallRecordCmdDrawIndexedIndirect(layer_data *dev_data, GLOBAL_CB_NODE *cb_state, VkPipelineBindPoint bind_point, + BUFFER_STATE *buffer_state, std::vector *mem_accesses) { + UpdateStateCmdDrawType(dev_data, cb_state, CMD_DRAWINDEXEDINDIRECT, bind_point, mem_accesses); AddCommandBufferBindingBuffer(dev_data, cb_state, buffer_state); } @@ -5810,57 +5868,59 @@ VKAPI_ATTR void VKAPI_CALL CmdDrawIndexedIndirect(VkCommandBuffer commandBuffer, layer_data *dev_data = GetLayerDataPtr(get_dispatch_key(commandBuffer), layer_data_map); GLOBAL_CB_NODE *cb_state = nullptr; BUFFER_STATE *buffer_state = nullptr; + std::vector mem_accesses; unique_lock_t lock(global_lock); bool skip = PreCallValidateCmdDrawIndexedIndirect(dev_data, commandBuffer, buffer, true, VK_PIPELINE_BIND_POINT_GRAPHICS, - &cb_state, &buffer_state, "vkCmdDrawIndexedIndirect()"); - lock.unlock(); + &cb_state, &mem_accesses, &buffer_state, "vkCmdDrawIndexedIndirect()"); if (!skip) { - dev_data->dispatch_table.CmdDrawIndexedIndirect(commandBuffer, buffer, offset, count, stride); - lock.lock(); - PostCallRecordCmdDrawIndexedIndirect(dev_data, cb_state, VK_PIPELINE_BIND_POINT_GRAPHICS, buffer_state); + PreCallRecordCmdDrawIndexedIndirect(dev_data, cb_state, VK_PIPELINE_BIND_POINT_GRAPHICS, buffer_state, &mem_accesses); lock.unlock(); + dev_data->dispatch_table.CmdDrawIndexedIndirect(commandBuffer, buffer, offset, count, stride); } } static bool PreCallValidateCmdDispatch(layer_data *dev_data, VkCommandBuffer cmd_buffer, bool indexed, - VkPipelineBindPoint bind_point, GLOBAL_CB_NODE **cb_state, const char *caller) { - return ValidateCmdDrawType(dev_data, cmd_buffer, indexed, bind_point, CMD_DISPATCH, cb_state, caller, VK_QUEUE_COMPUTE_BIT, - VALIDATION_ERROR_19c02415, VALIDATION_ERROR_19c00017, VALIDATION_ERROR_UNDEFINED); + VkPipelineBindPoint bind_point, GLOBAL_CB_NODE **cb_state, + std::vector *mem_accesses, const char *caller) { + return ValidateCmdDrawType(dev_data, cmd_buffer, indexed, bind_point, CMD_DISPATCH, cb_state, mem_accesses, caller, + VK_QUEUE_COMPUTE_BIT, VALIDATION_ERROR_19c02415, VALIDATION_ERROR_19c00017, + VALIDATION_ERROR_UNDEFINED); } -static void PostCallRecordCmdDispatch(layer_data *dev_data, GLOBAL_CB_NODE *cb_state, VkPipelineBindPoint bind_point) { - UpdateStateCmdDrawDispatchType(dev_data, cb_state, bind_point); +static void PreCallRecordCmdDispatch(layer_data *dev_data, GLOBAL_CB_NODE *cb_state, VkPipelineBindPoint bind_point, + std::vector *mem_accesses) { + UpdateStateCmdDrawDispatchType(dev_data, cb_state, CMD_DISPATCH, bind_point, mem_accesses); } VKAPI_ATTR void VKAPI_CALL CmdDispatch(VkCommandBuffer commandBuffer, uint32_t x, uint32_t y, uint32_t z) { layer_data *dev_data = GetLayerDataPtr(get_dispatch_key(commandBuffer), layer_data_map); GLOBAL_CB_NODE *cb_state = nullptr; + std::vector mem_accesses; unique_lock_t lock(global_lock); - bool skip = - PreCallValidateCmdDispatch(dev_data, commandBuffer, false, VK_PIPELINE_BIND_POINT_COMPUTE, &cb_state, "vkCmdDispatch()"); - lock.unlock(); + bool skip = PreCallValidateCmdDispatch(dev_data, commandBuffer, false, VK_PIPELINE_BIND_POINT_COMPUTE, &cb_state, &mem_accesses, + "vkCmdDispatch()"); if (!skip) { - dev_data->dispatch_table.CmdDispatch(commandBuffer, x, y, z); - lock.lock(); - PostCallRecordCmdDispatch(dev_data, cb_state, VK_PIPELINE_BIND_POINT_COMPUTE); + PreCallRecordCmdDispatch(dev_data, cb_state, VK_PIPELINE_BIND_POINT_COMPUTE, &mem_accesses); lock.unlock(); + dev_data->dispatch_table.CmdDispatch(commandBuffer, x, y, z); } } static bool PreCallValidateCmdDispatchIndirect(layer_data *dev_data, VkCommandBuffer cmd_buffer, VkBuffer buffer, bool indexed, VkPipelineBindPoint bind_point, GLOBAL_CB_NODE **cb_state, - BUFFER_STATE **buffer_state, const char *caller) { + BUFFER_STATE **buffer_state, std::vector *mem_accesses, + const char *caller) { bool skip = - ValidateCmdDrawType(dev_data, cmd_buffer, indexed, bind_point, CMD_DISPATCHINDIRECT, cb_state, caller, VK_QUEUE_COMPUTE_BIT, - VALIDATION_ERROR_1a002415, VALIDATION_ERROR_1a000017, VALIDATION_ERROR_UNDEFINED); + ValidateCmdDrawType(dev_data, cmd_buffer, indexed, bind_point, CMD_DISPATCHINDIRECT, cb_state, mem_accesses, caller, + VK_QUEUE_COMPUTE_BIT, VALIDATION_ERROR_1a002415, VALIDATION_ERROR_1a000017, VALIDATION_ERROR_UNDEFINED); *buffer_state = GetBufferState(dev_data, buffer); skip |= ValidateMemoryIsBoundToBuffer(dev_data, *buffer_state, caller, VALIDATION_ERROR_1a000322); return skip; } -static void PostCallRecordCmdDispatchIndirect(layer_data *dev_data, GLOBAL_CB_NODE *cb_state, VkPipelineBindPoint bind_point, - BUFFER_STATE *buffer_state) { - UpdateStateCmdDrawDispatchType(dev_data, cb_state, bind_point); +static void PreCallRecordCmdDispatchIndirect(layer_data *dev_data, GLOBAL_CB_NODE *cb_state, VkPipelineBindPoint bind_point, + BUFFER_STATE *buffer_state, std::vector *mem_accesses) { + UpdateStateCmdDrawDispatchType(dev_data, cb_state, CMD_DISPATCHINDIRECT, bind_point, mem_accesses); AddCommandBufferBindingBuffer(dev_data, cb_state, buffer_state); } @@ -5868,15 +5928,14 @@ VKAPI_ATTR void VKAPI_CALL CmdDispatchIndirect(VkCommandBuffer commandBuffer, Vk layer_data *dev_data = GetLayerDataPtr(get_dispatch_key(commandBuffer), layer_data_map); GLOBAL_CB_NODE *cb_state = nullptr; BUFFER_STATE *buffer_state = nullptr; + std::vector mem_accesses; unique_lock_t lock(global_lock); bool skip = PreCallValidateCmdDispatchIndirect(dev_data, commandBuffer, buffer, false, VK_PIPELINE_BIND_POINT_COMPUTE, - &cb_state, &buffer_state, "vkCmdDispatchIndirect()"); - lock.unlock(); + &cb_state, &buffer_state, &mem_accesses, "vkCmdDispatchIndirect()"); if (!skip) { - dev_data->dispatch_table.CmdDispatchIndirect(commandBuffer, buffer, offset); - lock.lock(); - PostCallRecordCmdDispatchIndirect(dev_data, cb_state, VK_PIPELINE_BIND_POINT_COMPUTE, buffer_state); + PreCallRecordCmdDispatchIndirect(dev_data, cb_state, VK_PIPELINE_BIND_POINT_COMPUTE, buffer_state, &mem_accesses); lock.unlock(); + dev_data->dispatch_table.CmdDispatchIndirect(commandBuffer, buffer, offset); } } @@ -6940,7 +6999,7 @@ static void PreCallRecordCmdPipelineBarrier(layer_data *device_data, GLOBAL_CB_N // Now that we've merged previous synch commands, append this cmd to existing synch commands cb_state->synch_commands.emplace_back(synch_cmd_ptr); // TODO: Make this barrier/access parsing smarter - for (auto &mem_access_pair : cb_state->memory_accesses) { + for (auto &mem_access_pair : cb_state->mem_accesses.access_map) { for (auto &mem_access : mem_access_pair.second) { if (mem_access.src_stage_flags & src_stage_mask) { // Record any execution barrier overlaps @@ -6972,8 +7031,8 @@ static void PreCallRecordCmdPipelineBarrier(layer_data *device_data, GLOBAL_CB_N barrier_access.location.size = buff_barrier.size; barrier_access.mem_barrier = false; barrier_access.pipe_barrier = false; - const auto &mem_access_pair = cb_state->memory_accesses.find(mem_obj); - if (mem_access_pair != cb_state->memory_accesses.end()) { + const auto &mem_access_pair = cb_state->mem_accesses.access_map.find(mem_obj); + if (mem_access_pair != cb_state->mem_accesses.access_map.end()) { for (auto &mem_access : mem_access_pair->second) { // If the pipe & access masks overlap, then barrier applies if (((mem_access.src_access_flags & buff_barrier.srcAccessMask) != 0) && diff --git a/layers/core_validation_types.h b/layers/core_validation_types.h index 91da1bab21..6f805c429e 100644 --- a/layers/core_validation_types.h +++ b/layers/core_validation_types.h @@ -850,6 +850,23 @@ struct LAST_BOUND_STATE { dynamicOffsets.clear(); } }; + +// Struct container to group sets of r/w buffers & images +struct DrawDispatchAccesses { + std::unordered_set read_images; + std::unordered_set read_buffers; + std::unordered_set write_images; + std::unordered_set write_buffers; + std::unordered_map> access_map; + void reset() { + read_images.clear(); + read_buffers.clear(); + write_images.clear(); + write_buffers.clear(); + access_map.clear(); + } +}; + // Cmd Buffer Wrapper Struct - TODO : This desperately needs its own class struct GLOBAL_CB_NODE : public BASE_NODE { VkCommandBuffer commandBuffer; @@ -884,8 +901,8 @@ struct GLOBAL_CB_NODE : public BASE_NODE { // std::vector> commands; // Commands in this command buffer std::vector synch_commands; // Synch Commands in this command buffer - // Store all memory accesses per memory object made in this command buffer - std::unordered_map> memory_accesses; + // Track all mem_accesses by this CB at the point of a draw + DrawDispatchAccesses mem_accesses; std::unordered_set waitedEvents; std::vector writeEventsBeforeWait; @@ -900,9 +917,6 @@ struct GLOBAL_CB_NODE : public BASE_NODE { DRAW_DATA currentDrawData; bool vertex_buffer_used; // Track for perf warning to make sure any bound vtx buffer used VkCommandBuffer primaryCommandBuffer; - // Track images and buffers that are updated by this CB at the point of a draw - std::unordered_set updateImages; - std::unordered_set updateBuffers; // If primary, the secondary command buffers we will call. // If secondary, the primary command buffers we will be called by. std::unordered_set linkedCommandBuffers; diff --git a/layers/descriptor_sets.cpp b/layers/descriptor_sets.cpp index 693b3ae4e3..ad1971f219 100644 --- a/layers/descriptor_sets.cpp +++ b/layers/descriptor_sets.cpp @@ -552,9 +552,11 @@ bool cvdescriptorset::DescriptorSet::ValidateDrawState(const std::map &bindings, - std::unordered_set *buffer_set, - std::unordered_set *image_set) const { +uint32_t cvdescriptorset::DescriptorSet::GetReadWriteBuffersAndImages(const std::map &bindings, + std::unordered_set *read_buffer_set, + std::unordered_set *read_image_set, + std::unordered_set *write_buffer_set, + std::unordered_set *write_image_set) const { auto num_updates = 0; for (auto binding_pair : bindings) { auto binding = binding_pair.first; @@ -563,15 +565,32 @@ uint32_t cvdescriptorset::DescriptorSet::GetStorageUpdates(const std::mapGetGlobalStartIndexFromBinding(binding); + auto &buffer_set = read_buffer_set; + auto &image_set = read_image_set; if (descriptors_[start_idx]->IsStorage()) { - if (Image == descriptors_[start_idx]->descriptor_class) { + buffer_set = write_buffer_set; + image_set = write_image_set; + } + switch (descriptors_[start_idx]->descriptor_class) { + case (Image): { for (uint32_t i = 0; i < p_layout_->GetDescriptorCountFromBinding(binding); ++i) { if (descriptors_[start_idx + i]->updated) { image_set->insert(static_cast(descriptors_[start_idx + i].get())->GetImageView()); num_updates++; } } - } else if (TexelBuffer == descriptors_[start_idx]->descriptor_class) { + break; + } + case (ImageSampler): { + for (uint32_t i = 0; i < p_layout_->GetDescriptorCountFromBinding(binding); ++i) { + if (descriptors_[start_idx + i]->updated) { + image_set->insert(static_cast(descriptors_[start_idx + i].get())->GetImageView()); + num_updates++; + } + } + break; + } + case (TexelBuffer): { for (uint32_t i = 0; i < p_layout_->GetDescriptorCountFromBinding(binding); ++i) { if (descriptors_[start_idx + i]->updated) { auto bufferview = static_cast(descriptors_[start_idx + i].get())->GetBufferView(); @@ -582,14 +601,20 @@ uint32_t cvdescriptorset::DescriptorSet::GetStorageUpdates(const std::mapdescriptor_class) { + break; + } + case (GeneralBuffer): { for (uint32_t i = 0; i < p_layout_->GetDescriptorCountFromBinding(binding); ++i) { if (descriptors_[start_idx + i]->updated) { buffer_set->insert(static_cast(descriptors_[start_idx + i].get())->GetBuffer()); num_updates++; } } + break; } + default: + // Don't need to do anything for Sampler case + break; } } return num_updates; diff --git a/layers/descriptor_sets.h b/layers/descriptor_sets.h index bfe75e7c00..8384981dbc 100644 --- a/layers/descriptor_sets.h +++ b/layers/descriptor_sets.h @@ -348,8 +348,10 @@ class DescriptorSet : public BASE_NODE { const char *caller, std::string *) const; // For given set of bindings, add any buffers and images that will be updated to their respective unordered_sets & return number // of objects inserted - uint32_t GetStorageUpdates(const std::map &, std::unordered_set *, - std::unordered_set *) const; + uint32_t GetReadWriteBuffersAndImages(const std::map &, std::unordered_set *read_buffer_set, + std::unordered_set *read_image_set, + std::unordered_set *write_buffer_set, + std::unordered_set *write_image_set) const; // Descriptor Update functions. These functions validate state and perform update separately // Validate contents of a WriteUpdate From 970225e23421318703bff4a098b4e230a3c93693 Mon Sep 17 00:00:00 2001 From: Tobin Ehlis Date: Mon, 14 Aug 2017 14:48:36 -0600 Subject: [PATCH 17/28] layers:Only record barriers outside of renderPass A PipelineBarrier command that occurs within a renderPass is only for handling a subpass self-dependency. That case is not currently included in the synch model so updating code to only record barriers that are outside of a renderPass for now. --- layers/core_validation.cpp | 138 +++++++++++++++++++------------------ 1 file changed, 72 insertions(+), 66 deletions(-) diff --git a/layers/core_validation.cpp b/layers/core_validation.cpp index b09beddf8c..c2b046445b 100644 --- a/layers/core_validation.cpp +++ b/layers/core_validation.cpp @@ -6974,76 +6974,78 @@ static void PreCallRecordCmdPipelineBarrier(layer_data *device_data, GLOBAL_CB_N Command *cmd_ptr = cb_state->commands.back().get(); SynchCommand *synch_cmd_ptr = static_cast(cmd_ptr); - // First thing to do is parse through any previous barriers. If this barriers srcMasks match with - // previous barriers destination masks, then incorporate the previous barrier srcMasks into our srcMask - // and merge in all of the previous barriers. This will account for dependency chains. - for (const auto &synch_cmd : cb_state->synch_commands) { - if (synch_cmd->dst_stage_flags & src_stage_mask) { - // scopes overlap so merge prev barrier masks into current masks - src_stage_mask |= synch_cmd->src_stage_flags; - synch_cmd_ptr->src_stage_flags = src_stage_mask; - dst_stage_mask |= synch_cmd->dst_stage_flags; - synch_cmd_ptr->dst_stage_flags = dst_stage_mask; - // Pull all previous overlapping barriers into this barrier - for (const auto &global_barrier : synch_cmd->global_barriers) { - synch_cmd_ptr->global_barriers.emplace_back(global_barrier); - } - for (const auto &buff_barrier : synch_cmd->buffer_barriers) { - synch_cmd_ptr->buffer_barriers.emplace_back(buff_barrier); - } - for (const auto &img_barrier : synch_cmd->image_barriers) { - synch_cmd_ptr->image_barriers.emplace_back(img_barrier); - } - } - } - // Now that we've merged previous synch commands, append this cmd to existing synch commands - cb_state->synch_commands.emplace_back(synch_cmd_ptr); - // TODO: Make this barrier/access parsing smarter - for (auto &mem_access_pair : cb_state->mem_accesses.access_map) { - for (auto &mem_access : mem_access_pair.second) { - if (mem_access.src_stage_flags & src_stage_mask) { - // Record any execution barrier overlaps - mem_access.pipe_barrier = true; - mem_access.dst_stage_flags |= dst_stage_mask; - // For every global barrier that matches access mask, record the barrier - for (uint32_t i = 0; i < mem_barrier_count; ++i) { - const auto &mem_barrier = mem_barriers[i]; - if ((mem_barrier.srcAccessMask & mem_access.src_access_flags) != 0) { - // This memory barrier applies to earlier access so record details - mem_access.mem_barrier = true; - mem_access.dst_access_flags |= mem_barrier.dstAccessMask; - mem_access.synch_commands.push_back(cmd_ptr); + if (!cb_state->activeRenderPass) { // Barriers in a renderpass are only for subpass self-dep + // First thing to do is parse through any previous barriers. If this barriers srcMasks match with + // previous barriers destination masks, then incorporate the previous barrier srcMasks into our srcMask + // and merge in all of the previous barriers. This will account for dependency chains. + for (const auto &synch_cmd : cb_state->synch_commands) { + if (synch_cmd->dst_stage_flags & src_stage_mask) { + // scopes overlap so merge prev barrier masks into current masks + src_stage_mask |= synch_cmd->src_stage_flags; + synch_cmd_ptr->src_stage_flags = src_stage_mask; + dst_stage_mask |= synch_cmd->dst_stage_flags; + synch_cmd_ptr->dst_stage_flags = dst_stage_mask; + // Pull all previous overlapping barriers into this barrier + for (const auto &global_barrier : synch_cmd->global_barriers) { + synch_cmd_ptr->global_barriers.emplace_back(global_barrier); + } + for (const auto &buff_barrier : synch_cmd->buffer_barriers) { + synch_cmd_ptr->buffer_barriers.emplace_back(buff_barrier); + } + for (const auto &img_barrier : synch_cmd->image_barriers) { + synch_cmd_ptr->image_barriers.emplace_back(img_barrier); + } + } + } + // Now that we've merged previous synch commands, append this cmd to existing synch commands + cb_state->synch_commands.emplace_back(synch_cmd_ptr); + // TODO: Make this barrier/access parsing smarter + for (auto &mem_access_pair : cb_state->mem_accesses.access_map) { + for (auto &mem_access : mem_access_pair.second) { + if (mem_access.src_stage_flags & src_stage_mask) { + // Record any execution barrier overlaps + mem_access.pipe_barrier = true; + mem_access.dst_stage_flags |= dst_stage_mask; + // For every global barrier that matches access mask, record the barrier + for (uint32_t i = 0; i < mem_barrier_count; ++i) { + const auto &mem_barrier = mem_barriers[i]; + if ((mem_barrier.srcAccessMask & mem_access.src_access_flags) != 0) { + // This memory barrier applies to earlier access so record details + mem_access.mem_barrier = true; + mem_access.dst_access_flags |= mem_barrier.dstAccessMask; + mem_access.synch_commands.push_back(cmd_ptr); + } } } } } - } - for (uint32_t i = 0; i < buffer_mem_barrier_count; ++i) { - const auto &buff_barrier = buffer_mem_barriers[i]; - const auto &buff_state = GetBufferState(device_data, buff_barrier.buffer); - const auto mem_obj = buff_state->binding.mem; - // Make an access struct with barrier range details to check for overlap - // TODO: This is hacky, creating tmp access struct from barrier to check conflict - // need to rework original function so this makes sense or write alternate function - MemoryAccess barrier_access = {}; - barrier_access.location.mem = mem_obj; - barrier_access.location.offset = buff_barrier.offset; - barrier_access.location.size = buff_barrier.size; - barrier_access.mem_barrier = false; - barrier_access.pipe_barrier = false; - const auto &mem_access_pair = cb_state->mem_accesses.access_map.find(mem_obj); - if (mem_access_pair != cb_state->mem_accesses.access_map.end()) { - for (auto &mem_access : mem_access_pair->second) { - // If the pipe & access masks overlap, then barrier applies - if (((mem_access.src_access_flags & buff_barrier.srcAccessMask) != 0) && - ((mem_access.src_stage_flags & src_stage_mask) != 0)) { - // Set buff access write opposite of actual access so conflict is flagged on overlap - barrier_access.write = !mem_access.write; - if (MemoryConflict(&barrier_access, &mem_access)) { - mem_access.mem_barrier = true; - mem_access.dst_stage_flags |= dst_stage_mask; - mem_access.dst_access_flags |= buff_barrier.dstAccessMask; - mem_access.synch_commands.push_back(cmd_ptr); + for (uint32_t i = 0; i < buffer_mem_barrier_count; ++i) { + const auto &buff_barrier = buffer_mem_barriers[i]; + const auto &buff_state = GetBufferState(device_data, buff_barrier.buffer); + const auto mem_obj = buff_state->binding.mem; + // Make an access struct with barrier range details to check for overlap + // TODO: This is hacky, creating tmp access struct from barrier to check conflict + // need to rework original function so this makes sense or write alternate function + MemoryAccess barrier_access = {}; + barrier_access.location.mem = mem_obj; + barrier_access.location.offset = buff_barrier.offset; + barrier_access.location.size = buff_barrier.size; + barrier_access.mem_barrier = false; + barrier_access.pipe_barrier = false; + const auto &mem_access_pair = cb_state->mem_accesses.access_map.find(mem_obj); + if (mem_access_pair != cb_state->mem_accesses.access_map.end()) { + for (auto &mem_access : mem_access_pair->second) { + // If the pipe & access masks overlap, then barrier applies + if (((mem_access.src_access_flags & buff_barrier.srcAccessMask) != 0) && + ((mem_access.src_stage_flags & src_stage_mask) != 0)) { + // Set buff access write opposite of actual access so conflict is flagged on overlap + barrier_access.write = !mem_access.write; + if (MemoryConflict(&barrier_access, &mem_access)) { + mem_access.mem_barrier = true; + mem_access.dst_stage_flags |= dst_stage_mask; + mem_access.dst_access_flags |= buff_barrier.dstAccessMask; + mem_access.synch_commands.push_back(cmd_ptr); + } } } } @@ -8163,6 +8165,10 @@ VKAPI_ATTR void VKAPI_CALL CmdBeginRenderPass(VkCommandBuffer commandBuffer, con cb_node->queue_submit_functions.push_back(function); } } + const auto &subpass_desc = render_pass_state->createInfo.pSubpasses[0]; + for (uint32_t i = 0; i < subpass_desc.inputAttachmentCount; ++i) { + // Mark input attachments as read + } if (clear_op_size > pRenderPassBegin->clearValueCount) { skip |= log_msg( dev_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_RENDER_PASS_EXT, From 1f919d595bc92c8044b70ee2607e23bb2233e9b4 Mon Sep 17 00:00:00 2001 From: Tobin Ehlis Date: Mon, 14 Aug 2017 16:21:35 -0600 Subject: [PATCH 18/28] layers:Move barrier recording into a function Moved existing code into RecordBarrierMemoryAccess() function. This generalizes to code so it can more easily be multi-purposed in order to record barriers for CmdWaitEvents(). --- layers/core_validation.cpp | 171 ++++++++++++++++++--------------- layers/core_validation_types.h | 4 +- 2 files changed, 94 insertions(+), 81 deletions(-) diff --git a/layers/core_validation.cpp b/layers/core_validation.cpp index c2b046445b..a3f7c837e7 100644 --- a/layers/core_validation.cpp +++ b/layers/core_validation.cpp @@ -6886,6 +6886,95 @@ bool ValidateStageMasksAgainstQueueCapabilities(layer_data *dev_data, GLOBAL_CB_ return skip; } +// Record given set of barriers vs. outstanding memory accesses for this cmd buffer +// 1. First merge this barrier within any previous barriers in this CB +// 2. For each barrier, compare given existing mem accesses and record any applicable barriers +static void RecordBarrierMemoryAccess(layer_data *device_data, CMD_TYPE cmd, GLOBAL_CB_NODE *cb_state, + VkCommandBuffer command_buffer, VkPipelineStageFlags src_stage_mask, + VkPipelineStageFlags dst_stage_mask, uint32_t mem_barrier_count, + const VkMemoryBarrier *mem_barriers, uint32_t buffer_mem_barrier_count, + const VkBufferMemoryBarrier *buffer_mem_barriers, uint32_t image_memory_barrier_count, + const VkImageMemoryBarrier *image_memory_barriers) { + cb_state->commands.emplace_back(unique_ptr(new SynchCommand(cmd, src_stage_mask, dst_stage_mask))); + Command *cmd_ptr = cb_state->commands.back().get(); + SynchCommand *synch_cmd_ptr = static_cast(cmd_ptr); + // First thing to do is parse through any previous barriers. If this barriers srcMasks match with + // previous barriers destination masks, then incorporate the previous barrier srcMasks into our srcMask + // and merge in all of the previous barriers. This will account for dependency chains. + for (const auto &synch_cmd : cb_state->synch_commands) { + if (synch_cmd->dst_stage_flags & src_stage_mask) { + // scopes overlap so merge prev barrier masks into current masks + src_stage_mask |= synch_cmd->src_stage_flags; + synch_cmd_ptr->src_stage_flags = src_stage_mask; + dst_stage_mask |= synch_cmd->dst_stage_flags; + synch_cmd_ptr->dst_stage_flags = dst_stage_mask; + // Pull all previous overlapping barriers into this barrier + for (const auto &global_barrier : synch_cmd->global_barriers) { + synch_cmd_ptr->global_barriers.emplace_back(global_barrier); + } + for (const auto &buff_barrier : synch_cmd->buffer_barriers) { + synch_cmd_ptr->buffer_barriers.emplace_back(buff_barrier); + } + for (const auto &img_barrier : synch_cmd->image_barriers) { + synch_cmd_ptr->image_barriers.emplace_back(img_barrier); + } + } + } + // Now that we've merged previous synch commands, append this cmd to existing synch commands + cb_state->synch_commands.emplace_back(synch_cmd_ptr); + // TODO: Make this barrier/access parsing smarter + for (auto &mem_access_pair : cb_state->mem_accesses.access_map) { + for (auto &mem_access : mem_access_pair.second) { + if (mem_access.src_stage_flags & src_stage_mask) { + // Record any execution barrier overlaps + mem_access.pipe_barrier = true; + mem_access.dst_stage_flags |= dst_stage_mask; + // For every global barrier that matches access mask, record the barrier + for (uint32_t i = 0; i < mem_barrier_count; ++i) { + const auto &mem_barrier = mem_barriers[i]; + if ((mem_barrier.srcAccessMask & mem_access.src_access_flags) != 0) { + // This memory barrier applies to earlier access so record details + mem_access.mem_barrier = true; + mem_access.dst_access_flags |= mem_barrier.dstAccessMask; + mem_access.synch_commands.push_back(cmd_ptr); + } + } + } + } + } + for (uint32_t i = 0; i < buffer_mem_barrier_count; ++i) { + const auto &buff_barrier = buffer_mem_barriers[i]; + const auto &buff_state = GetBufferState(device_data, buff_barrier.buffer); + const auto mem_obj = buff_state->binding.mem; + // Make an access struct with barrier range details to check for overlap + // TODO: This is hacky, creating tmp access struct from barrier to check conflict + // need to rework original function so this makes sense or write alternate function + MemoryAccess barrier_access = {}; + barrier_access.location.mem = mem_obj; + barrier_access.location.offset = buff_barrier.offset; + barrier_access.location.size = buff_barrier.size; + barrier_access.mem_barrier = false; + barrier_access.pipe_barrier = false; + const auto &mem_access_pair = cb_state->mem_accesses.access_map.find(mem_obj); + if (mem_access_pair != cb_state->mem_accesses.access_map.end()) { + for (auto &mem_access : mem_access_pair->second) { + // If the pipe & access masks overlap, then barrier applies + if (((mem_access.src_access_flags & buff_barrier.srcAccessMask) != 0) && + ((mem_access.src_stage_flags & src_stage_mask) != 0)) { + // Set buff access write opposite of actual access so conflict is flagged on overlap + barrier_access.write = !mem_access.write; + if (MemoryConflict(&barrier_access, &mem_access)) { + mem_access.mem_barrier = true; + mem_access.dst_stage_flags |= dst_stage_mask; + mem_access.dst_access_flags |= buff_barrier.dstAccessMask; + mem_access.synch_commands.push_back(cmd_ptr); + } + } + } + } + } +} + VKAPI_ATTR void VKAPI_CALL CmdWaitEvents(VkCommandBuffer commandBuffer, uint32_t eventCount, const VkEvent *pEvents, VkPipelineStageFlags sourceStageMask, VkPipelineStageFlags dstStageMask, uint32_t memoryBarrierCount, const VkMemoryBarrier *pMemoryBarriers, @@ -6969,87 +7058,11 @@ static void PreCallRecordCmdPipelineBarrier(layer_data *device_data, GLOBAL_CB_N const VkBufferMemoryBarrier *buffer_mem_barriers, uint32_t imageMemoryBarrierCount, const VkImageMemoryBarrier *pImageMemoryBarriers) { TransitionImageLayouts(device_data, commandBuffer, imageMemoryBarrierCount, pImageMemoryBarriers); - cb_state->commands.emplace_back( - unique_ptr(new SynchCommand(CMD_PIPELINEBARRIER, src_stage_mask, dst_stage_mask))); - Command *cmd_ptr = cb_state->commands.back().get(); - SynchCommand *synch_cmd_ptr = static_cast(cmd_ptr); if (!cb_state->activeRenderPass) { // Barriers in a renderpass are only for subpass self-dep - // First thing to do is parse through any previous barriers. If this barriers srcMasks match with - // previous barriers destination masks, then incorporate the previous barrier srcMasks into our srcMask - // and merge in all of the previous barriers. This will account for dependency chains. - for (const auto &synch_cmd : cb_state->synch_commands) { - if (synch_cmd->dst_stage_flags & src_stage_mask) { - // scopes overlap so merge prev barrier masks into current masks - src_stage_mask |= synch_cmd->src_stage_flags; - synch_cmd_ptr->src_stage_flags = src_stage_mask; - dst_stage_mask |= synch_cmd->dst_stage_flags; - synch_cmd_ptr->dst_stage_flags = dst_stage_mask; - // Pull all previous overlapping barriers into this barrier - for (const auto &global_barrier : synch_cmd->global_barriers) { - synch_cmd_ptr->global_barriers.emplace_back(global_barrier); - } - for (const auto &buff_barrier : synch_cmd->buffer_barriers) { - synch_cmd_ptr->buffer_barriers.emplace_back(buff_barrier); - } - for (const auto &img_barrier : synch_cmd->image_barriers) { - synch_cmd_ptr->image_barriers.emplace_back(img_barrier); - } - } - } - // Now that we've merged previous synch commands, append this cmd to existing synch commands - cb_state->synch_commands.emplace_back(synch_cmd_ptr); - // TODO: Make this barrier/access parsing smarter - for (auto &mem_access_pair : cb_state->mem_accesses.access_map) { - for (auto &mem_access : mem_access_pair.second) { - if (mem_access.src_stage_flags & src_stage_mask) { - // Record any execution barrier overlaps - mem_access.pipe_barrier = true; - mem_access.dst_stage_flags |= dst_stage_mask; - // For every global barrier that matches access mask, record the barrier - for (uint32_t i = 0; i < mem_barrier_count; ++i) { - const auto &mem_barrier = mem_barriers[i]; - if ((mem_barrier.srcAccessMask & mem_access.src_access_flags) != 0) { - // This memory barrier applies to earlier access so record details - mem_access.mem_barrier = true; - mem_access.dst_access_flags |= mem_barrier.dstAccessMask; - mem_access.synch_commands.push_back(cmd_ptr); - } - } - } - } - } - for (uint32_t i = 0; i < buffer_mem_barrier_count; ++i) { - const auto &buff_barrier = buffer_mem_barriers[i]; - const auto &buff_state = GetBufferState(device_data, buff_barrier.buffer); - const auto mem_obj = buff_state->binding.mem; - // Make an access struct with barrier range details to check for overlap - // TODO: This is hacky, creating tmp access struct from barrier to check conflict - // need to rework original function so this makes sense or write alternate function - MemoryAccess barrier_access = {}; - barrier_access.location.mem = mem_obj; - barrier_access.location.offset = buff_barrier.offset; - barrier_access.location.size = buff_barrier.size; - barrier_access.mem_barrier = false; - barrier_access.pipe_barrier = false; - const auto &mem_access_pair = cb_state->mem_accesses.access_map.find(mem_obj); - if (mem_access_pair != cb_state->mem_accesses.access_map.end()) { - for (auto &mem_access : mem_access_pair->second) { - // If the pipe & access masks overlap, then barrier applies - if (((mem_access.src_access_flags & buff_barrier.srcAccessMask) != 0) && - ((mem_access.src_stage_flags & src_stage_mask) != 0)) { - // Set buff access write opposite of actual access so conflict is flagged on overlap - barrier_access.write = !mem_access.write; - if (MemoryConflict(&barrier_access, &mem_access)) { - mem_access.mem_barrier = true; - mem_access.dst_stage_flags |= dst_stage_mask; - mem_access.dst_access_flags |= buff_barrier.dstAccessMask; - mem_access.synch_commands.push_back(cmd_ptr); - } - } - } - } - } + RecordBarrierMemoryAccess(device_data, CMD_PIPELINEBARRIER, cb_state, commandBuffer, src_stage_mask, dst_stage_mask, + mem_barrier_count, mem_barriers, buffer_mem_barrier_count, buffer_mem_barriers, + imageMemoryBarrierCount, pImageMemoryBarriers); } } diff --git a/layers/core_validation_types.h b/layers/core_validation_types.h index 6f805c429e..94674071a0 100644 --- a/layers/core_validation_types.h +++ b/layers/core_validation_types.h @@ -582,9 +582,9 @@ static const CmdFlags CommandToFlags[CMD_COUNT][2] = { // CMD_BEGINRENDERPASS, {{VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_ALL_GRAPHICS_BIT, 0}, {0, 0}}, // CMD_NEXTSUBPASS, - {{0, 0}, {0, 0}}, + {{VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_ALL_GRAPHICS_BIT, 0}, {0, 0}}, // CMD_ENDRENDERPASS, - {{0, 0}, {0, 0}}, + {{VK_PIPELINE_STAGE_ALL_COMMANDS_BIT | VK_PIPELINE_STAGE_ALL_GRAPHICS_BIT, 0}, {0, 0}}, // CMD_EXECUTECOMMANDS, {{0, 0}, {0, 0}}, // CMD_END, // Should be last command in any RECORDED cmd buffer From 8e3d99409658a27ffeb6028e072911d6422b4156 Mon Sep 17 00:00:00 2001 From: Tobin Ehlis Date: Tue, 15 Aug 2017 14:49:50 -0600 Subject: [PATCH 19/28] tests:Add DrawWithBufferWaRandRaWConflicts This is a race condition test to check both a RaW and WaR conflict with buffers used in a Draw. A src buffer is copied to a dst buffer, then, without a barrier, the src is used as a storage buffer in a draw and the dst is used as a uniform buffer in the same draw. This results in a RaW conflict for the Dst/Uniform buffer and WaR conflict for the Src/Storage buffer. --- tests/layer_validation_tests.cpp | 133 +++++++++++++++++++++++++++++++ 1 file changed, 133 insertions(+) diff --git a/tests/layer_validation_tests.cpp b/tests/layer_validation_tests.cpp index 77700bf9d6..646c271487 100644 --- a/tests/layer_validation_tests.cpp +++ b/tests/layer_validation_tests.cpp @@ -10201,6 +10201,139 @@ TEST_F(VkLayerTest, UpdateBufferWithinRenderPass) { m_errorMonitor->VerifyFound(); } +TEST_F(VkLayerTest, DrawWithBufferWaRandRaWConflicts) { + TEST_DESCRIPTION( + "Attempt a draw that reads from a buffer that was written to and writes" + "to a buffer that was read from, both without any barrier present."); + + ASSERT_NO_FATAL_FAILURE(Init()); + ASSERT_NO_FATAL_FAILURE(InitViewport()); + ASSERT_NO_FATAL_FAILURE(InitRenderTarget()); + + m_commandBuffer->begin(); + + // Don't try this at home. Writing data into buffer that will be used as storage buffer + // for the draw. Before the Draw, though, copying data from the storage buffer into the + // uniform buffer, which will then be read during the draw. This will create a RaW issue + // for the uniform buffer and a WaR issue for the storage buffer. + VkDeviceSize buff_size = 1024; + uint32_t qfi = 0; + VkBufferCreateInfo bci = {}; + bci.sType = VK_STRUCTURE_TYPE_BUFFER_CREATE_INFO; + bci.usage = VK_BUFFER_USAGE_STORAGE_BUFFER_BIT | VK_BUFFER_USAGE_TRANSFER_SRC_BIT | VK_BUFFER_USAGE_TRANSFER_DST_BIT; + bci.size = buff_size; + bci.queueFamilyIndexCount = 1; + bci.pQueueFamilyIndices = &qfi; + VkMemoryPropertyFlags reqs = VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT; + vk_testing::Buffer storage_buffer; + storage_buffer.init(*m_device, bci, reqs); + bci.usage = VK_BUFFER_USAGE_UNIFORM_BUFFER_BIT | VK_BUFFER_USAGE_TRANSFER_DST_BIT; + vk_testing::Buffer uniform_buffer; + uniform_buffer.init(*m_device, bci, reqs); + + VkDeviceSize offset = 0; + uint32_t num_elements = (uint32_t)buff_size / sizeof(uint32_t); // Number of 32bit elements + std::vector Data(num_elements, 0); + // Fill in data buffer + for (uint32_t i = 0; i < num_elements; ++i) { + Data[i] = i; + } + VkDeviceSize data_size = Data.size() * sizeof(uint32_t); + // Write data into storage buffer + vkCmdUpdateBuffer(m_commandBuffer->handle(), storage_buffer.handle(), offset, data_size, Data.data()); + // Global Barrier to make sure buffer update completed (so we don't get RaW on copy) + VkMemoryBarrier mem_barrier = {}; + mem_barrier.sType = VK_STRUCTURE_TYPE_MEMORY_BARRIER; + mem_barrier.srcAccessMask = VK_ACCESS_TRANSFER_WRITE_BIT; + mem_barrier.dstAccessMask = VK_ACCESS_TRANSFER_READ_BIT; + vkCmdPipelineBarrier(m_commandBuffer->handle(), VK_PIPELINE_STAGE_TRANSFER_BIT, VK_PIPELINE_STAGE_TRANSFER_BIT, 0, 1, + &mem_barrier, 0, nullptr, 0, nullptr); + // Copy storage buffer contents to uniform buffer + VkBufferCopy buff_copy = {0, // srcOffset + 0, // dstOffset + buff_size}; + vkCmdCopyBuffer(m_commandBuffer->handle(), storage_buffer.handle(), uniform_buffer.handle(), 1, &buff_copy); + // + OneOffDescriptorSet ds(m_device->device(), { + {0, VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER, 1, VK_SHADER_STAGE_ALL, nullptr}, + {1, VK_DESCRIPTOR_TYPE_STORAGE_BUFFER, 1, VK_SHADER_STAGE_ALL, nullptr}, + }); + + VkPipelineLayoutCreateInfo pipeline_layout_ci = {}; + pipeline_layout_ci.sType = VK_STRUCTURE_TYPE_PIPELINE_LAYOUT_CREATE_INFO; + pipeline_layout_ci.setLayoutCount = 1; + pipeline_layout_ci.pSetLayouts = &ds.layout_; + + VkPipelineLayout pipeline_layout; + VkResult err = vkCreatePipelineLayout(m_device->device(), &pipeline_layout_ci, NULL, &pipeline_layout); + ASSERT_VK_SUCCESS(err); + + VkDescriptorBufferInfo buff_info = {}; + buff_info.buffer = uniform_buffer.handle(); + buff_info.range = buff_size; + VkWriteDescriptorSet descriptor_write = {}; + descriptor_write.sType = VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET; + descriptor_write.dstBinding = 0; + descriptor_write.descriptorCount = 1; + descriptor_write.pTexelBufferView = nullptr; + descriptor_write.pBufferInfo = &buff_info; + descriptor_write.pImageInfo = nullptr; + descriptor_write.descriptorType = VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER; + descriptor_write.dstSet = ds.set_; + vkUpdateDescriptorSets(m_device->device(), 1, &descriptor_write, 0, NULL); + // + buff_info.buffer = storage_buffer.handle(); + descriptor_write.dstBinding = 1; + descriptor_write.descriptorType = VK_DESCRIPTOR_TYPE_STORAGE_BUFFER; + vkUpdateDescriptorSets(m_device->device(), 1, &descriptor_write, 0, NULL); + + // Create PSO that uses the uniform buffers + char const *vsSource = + "#version 450\n" + "\n" + "void main(){\n" + " gl_Position = vec4(1);\n" + "}\n"; + char const *fsSource = + "#version 450\n" + "\n" + "layout(location=0) out vec4 color;\n" + "layout(set=0, binding=0) uniform block { vec4 read; };\n" + "layout(set=0, binding=1) buffer block { vec4 write; };\n" + "void main(){\n" + " write = read;\n" + " color = vec4(1);" + "}\n"; + VkShaderObj vs(m_device, vsSource, VK_SHADER_STAGE_VERTEX_BIT, this); + VkShaderObj fs(m_device, fsSource, VK_SHADER_STAGE_FRAGMENT_BIT, this); + + VkPipelineObj pipe(m_device); + pipe.AddShader(&vs); + pipe.AddShader(&fs); + pipe.AddColorAttachment(); + + err = pipe.CreateVKPipeline(pipeline_layout, renderPass()); + ASSERT_VK_SUCCESS(err); + m_commandBuffer->BeginRenderPass(m_renderPassBeginInfo); + + vkCmdBindPipeline(m_commandBuffer->handle(), VK_PIPELINE_BIND_POINT_GRAPHICS, pipe.handle()); + vkCmdBindDescriptorSets(m_commandBuffer->handle(), VK_PIPELINE_BIND_POINT_GRAPHICS, pipeline_layout, 0, 1, &ds.set_, 0, + nullptr); + + VkViewport viewport = {0, 0, 16, 16, 0, 1}; + vkCmdSetViewport(m_commandBuffer->handle(), 0, 1, &viewport); + VkRect2D scissor = {{0, 0}, {16, 16}}; + vkCmdSetScissor(m_commandBuffer->handle(), 0, 1, &scissor); + // Should now trigger RaW and WaR errors at Draw time + m_errorMonitor->SetDesiredFailureMsg(VK_DEBUG_REPORT_WARNING_BIT_EXT, " causes a read after write conflict with "); + m_errorMonitor->SetDesiredFailureMsg(VK_DEBUG_REPORT_WARNING_BIT_EXT, " causes a write after read conflict with "); + vkCmdDraw(m_commandBuffer->handle(), 3, 1, 0, 0); + m_errorMonitor->VerifyFound(); + vkCmdEndRenderPass(m_commandBuffer->handle()); + m_commandBuffer->end(); + vkDestroyPipelineLayout(m_device->device(), pipeline_layout, nullptr); +} + TEST_F(VkPositiveLayerTest, UpdateBufferRaWDependencyWithBarrier) { TEST_DESCRIPTION("Update buffer used in CmdDrawIndirect w/ barrier before use"); From c75e7a1a5272beea1be960fe911db7ee28383237 Mon Sep 17 00:00:00 2001 From: Tobin Ehlis Date: Tue, 15 Aug 2017 17:04:17 -0600 Subject: [PATCH 20/28] layers:Make all MemAccess conflicts warnings Make any memory access race condition conflict messages warning-level for initial release. There are still some holes in modeling the synch primitives in validation so setting callback to warning allows for a soft release where people can ignore or post bugs on incorrect warnings while the synch model is updated in validation. The current warning message is meant to deter developers from sinking too much time into debugging these warnings, and promotes feedback for known-bad warning cases. --- layers/buffer_validation.cpp | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/layers/buffer_validation.cpp b/layers/buffer_validation.cpp index 6b03e39887..430428391f 100644 --- a/layers/buffer_validation.cpp +++ b/layers/buffer_validation.cpp @@ -702,11 +702,18 @@ bool ValidateMemoryAccesses(debug_report_data const *report_data, GLOBAL_CB_NODE const char *access2 = mem_access.write ? "write" : "read"; // If either access is imprecise can only warn, otherwise can error auto level = VK_DEBUG_REPORT_WARNING_BIT_EXT; - if (earlier_access.precise && mem_access.precise) level = VK_DEBUG_REPORT_ERROR_BIT_EXT; + // TODO: Un-comment the line below when we're confident in coverage of memory access checks + // so that precise/precise conflicts can be flagged as errors. Keeping everything a warning + // for early release and so that incorrect warnings can be flagged/fixed without causing errors. + // if (earlier_access.precise && mem_access.precise) level = VK_DEBUG_REPORT_ERROR_BIT_EXT; skip |= log_msg(report_data, level, VK_DEBUG_REPORT_OBJECT_TYPE_DEVICE_MEMORY_EXT, HandleToUint64(mem_access.location.mem), 0, MEMTRACK_SYNCHRONIZATION_ERROR, "DS", "%s called on VkCommandBuffer 0x%" PRIxLEAST64 - " causes a %s after %s conflict with memory object 0x%" PRIxLEAST64 ".", + " causes a %s after %s conflict with memory object 0x%" PRIxLEAST64 ". NOTE: This" + " race condition warning is a new feature in validation that still has some holes" + " in tracking Read/Write accesses as well as modeling synchronization objects. Don't" + " spend too much time investigating this warning, and feel free to file GitHub bugs " + " on any warning cases that you known are incorrect.", caller, HandleToUint64(cb_state->commandBuffer), access2, access1, HandleToUint64(mem_access.location.mem)); } From dff900f61e5107c004d2caedeac0872f9af9db0d Mon Sep 17 00:00:00 2001 From: Tobin Ehlis Date: Wed, 16 Aug 2017 14:28:10 -0600 Subject: [PATCH 21/28] layers:Record barriers in CmdWaitEvents Record barriers in CmdWaitEvents using same method used for barriers from CmdPipelineBarriers. This involves merging any previous barriers to handle dependency chains and then recording the barriers into any previous memory accesses that fall into the source synch scope. --- layers/core_validation.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/layers/core_validation.cpp b/layers/core_validation.cpp index a3f7c837e7..46a4c41abc 100644 --- a/layers/core_validation.cpp +++ b/layers/core_validation.cpp @@ -6898,7 +6898,7 @@ static void RecordBarrierMemoryAccess(layer_data *device_data, CMD_TYPE cmd, GLO cb_state->commands.emplace_back(unique_ptr(new SynchCommand(cmd, src_stage_mask, dst_stage_mask))); Command *cmd_ptr = cb_state->commands.back().get(); SynchCommand *synch_cmd_ptr = static_cast(cmd_ptr); - // First thing to do is parse through any previous barriers. If this barriers srcMasks match with + // First thing to do is parse through any previous barriers. If this barriers' srcMasks match with // previous barriers destination masks, then incorporate the previous barrier srcMasks into our srcMask // and merge in all of the previous barriers. This will account for dependency chains. for (const auto &synch_cmd : cb_state->synch_commands) { @@ -7013,6 +7013,9 @@ VKAPI_ATTR void VKAPI_CALL CmdWaitEvents(VkCommandBuffer commandBuffer, uint32_t return validateEventStageMask(q, cb_state, eventCount, first_event_index, sourceStageMask); }); TransitionImageLayouts(dev_data, commandBuffer, imageMemoryBarrierCount, pImageMemoryBarriers); + RecordBarrierMemoryAccess(dev_data, CMD_WAITEVENTS, cb_state, commandBuffer, sourceStageMask, dstStageMask, + memoryBarrierCount, pMemoryBarriers, bufferMemoryBarrierCount, pBufferMemoryBarriers, + imageMemoryBarrierCount, pImageMemoryBarriers); } } lock.unlock(); From 11799b97eb1fc2e9b8108ced33e303c4e5621073 Mon Sep 17 00:00:00 2001 From: Tobin Ehlis Date: Thu, 17 Aug 2017 13:59:32 -0600 Subject: [PATCH 22/28] layers:Refactor ValidateMemoryAccesses() Modify ValidateMemoryAccesses() so that it takes map of prev accesses directly. This is preparation to share the function with command buffer submission validation where we'll deal with sets of initial accesses that may cross many command buffers. --- layers/buffer_validation.cpp | 45 ++++++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 17 deletions(-) diff --git a/layers/buffer_validation.cpp b/layers/buffer_validation.cpp index 430428391f..e197eefc54 100644 --- a/layers/buffer_validation.cpp +++ b/layers/buffer_validation.cpp @@ -688,14 +688,17 @@ bool MemoryConflict(MemoryAccess const *initial_access, MemoryAccess const *seco return true; } -// Check given set of mem_accesses against memory accesses in cmd buffer up to this point and flag any conflicts -bool ValidateMemoryAccesses(debug_report_data const *report_data, GLOBAL_CB_NODE const *cb_state, +// Check given vector of new_mem_accesses against prev_mem_accesses map, which has live R/W access up to the point. +// The "live" R/W accesses in prev_mem_access_map means the accesses have no barrier has been identified. +// For any new accesses that conflict with prev accesses, flag an error +bool ValidateMemoryAccesses(debug_report_data const *report_data, VkCommandBuffer command_buffer, + const std::unordered_map> &prev_mem_access_map, std::vector *mem_accesses, const char *caller) { bool skip = false; for (const auto &mem_access : *mem_accesses) { auto mem_obj = mem_access.location.mem; - auto mem_access_pair = cb_state->mem_accesses.access_map.find(mem_obj); - if (mem_access_pair != cb_state->mem_accesses.access_map.end()) { + auto mem_access_pair = prev_mem_access_map.find(mem_obj); + if (mem_access_pair != prev_mem_access_map.end()) { for (const auto &earlier_access : mem_access_pair->second) { if (MemoryConflict(&earlier_access, &mem_access)) { const char *access1 = earlier_access.write ? "write" : "read"; @@ -706,16 +709,16 @@ bool ValidateMemoryAccesses(debug_report_data const *report_data, GLOBAL_CB_NODE // so that precise/precise conflicts can be flagged as errors. Keeping everything a warning // for early release and so that incorrect warnings can be flagged/fixed without causing errors. // if (earlier_access.precise && mem_access.precise) level = VK_DEBUG_REPORT_ERROR_BIT_EXT; - skip |= log_msg(report_data, level, VK_DEBUG_REPORT_OBJECT_TYPE_DEVICE_MEMORY_EXT, - HandleToUint64(mem_access.location.mem), 0, MEMTRACK_SYNCHRONIZATION_ERROR, "DS", + skip |= log_msg(report_data, level, VK_DEBUG_REPORT_OBJECT_TYPE_DEVICE_MEMORY_EXT, HandleToUint64(mem_obj), 0, + MEMTRACK_SYNCHRONIZATION_ERROR, "DS", "%s called on VkCommandBuffer 0x%" PRIxLEAST64 - " causes a %s after %s conflict with memory object 0x%" PRIxLEAST64 ". NOTE: This" + " causes a %s after %s conflict with memory object 0x%" PRIxLEAST64 + ". NOTE: This" " race condition warning is a new feature in validation that still has some holes" " in tracking Read/Write accesses as well as modeling synchronization objects. Don't" " spend too much time investigating this warning, and feel free to file GitHub bugs " " on any warning cases that you known are incorrect.", - caller, HandleToUint64(cb_state->commandBuffer), access2, access1, - HandleToUint64(mem_access.location.mem)); + caller, HandleToUint64(command_buffer), access2, access1, HandleToUint64(mem_obj)); } } } @@ -1117,7 +1120,8 @@ bool PreCallValidateCmdClearColorImage(layer_data *dev_data, VkCommandBuffer com } // TODO: Currently treating all image accesses as imprecise & covering the whole image area, this should be per-range AddWriteMemoryAccess(CMD_CLEARCOLORIMAGE, mem_accesses, image_state->binding, false); - skip |= ValidateMemoryAccesses(core_validation::GetReportData(dev_data), cb_state, mem_accesses, "vkCmdClearColorImage()"); + skip |= ValidateMemoryAccesses(core_validation::GetReportData(dev_data), commandBuffer, cb_state->mem_accesses.access_map, + mem_accesses, "vkCmdClearColorImage()"); return skip; } @@ -1180,7 +1184,8 @@ bool PreCallValidateCmdClearDepthStencilImage(layer_data *device_data, VkCommand } // TODO: Currently treating all image accesses as imprecise & covering the whole image area, this should be per-range AddWriteMemoryAccess(CMD_CLEARDEPTHSTENCILIMAGE, mem_accesses, image_state->binding, false); - skip |= ValidateMemoryAccesses(report_data, cb_state, mem_accesses, "vkCmdClearDepthStencilImage()"); + skip |= ValidateMemoryAccesses(report_data, commandBuffer, cb_state->mem_accesses.access_map, mem_accesses, + "vkCmdClearDepthStencilImage()"); return skip; } @@ -2013,7 +2018,8 @@ bool PreCallValidateCmdCopyImage(layer_data *device_data, GLOBAL_CB_NODE *cb_sta // TODO: Currently treating all image accesses as imprecise & covering the whole image area AddReadMemoryAccess(CMD_COPYIMAGE, mem_accesses, src_image_state->binding, false); AddWriteMemoryAccess(CMD_COPYIMAGE, mem_accesses, dst_image_state->binding, false); - skip |= ValidateMemoryAccesses(report_data, cb_state, mem_accesses, "vkCmdCopyImage()"); + skip |= ValidateMemoryAccesses(report_data, cb_state->commandBuffer, cb_state->mem_accesses.access_map, mem_accesses, + "vkCmdCopyImage()"); } // The formats of src_image and dst_image must be compatible. Formats are considered compatible if their texel size in bytes @@ -2213,7 +2219,8 @@ bool PreCallValidateCmdClearAttachments(layer_data *device_data, GLOBAL_CB_NODE // TODO: Just marking memory accesses as imprecise per-image binding basis for now, should be per-rect above const auto image_state = GetImageState(device_data, image_view_state->create_info.image); AddWriteMemoryAccess(CMD_CLEARATTACHMENTS, mem_accesses, image_state->binding, false); - skip |= ValidateMemoryAccesses(report_data, cb_state, mem_accesses, "vkCmdClearAttachments()"); + skip |= ValidateMemoryAccesses(report_data, commandBuffer, cb_state->mem_accesses.access_map, mem_accesses, + "vkCmdClearAttachments()"); } } } @@ -2271,7 +2278,8 @@ bool PreCallValidateCmdResolveImage(layer_data *device_data, GLOBAL_CB_NODE *cb_ // TODO: Currently treating all image accesses as imprecise & covering the whole image area AddReadMemoryAccess(CMD_RESOLVEIMAGE, mem_accesses, src_image_state->binding, false); AddWriteMemoryAccess(CMD_RESOLVEIMAGE, mem_accesses, dst_image_state->binding, false); - skip |= ValidateMemoryAccesses(report_data, cb_state, mem_accesses, "vkCmdResolveImage()"); + skip |= ValidateMemoryAccesses(report_data, cb_state->commandBuffer, cb_state->mem_accesses.access_map, mem_accesses, + "vkCmdResolveImage()"); if (src_image_state->createInfo.format != dst_image_state->createInfo.format) { char const str[] = "vkCmdResolveImage called with unmatched source and dest formats."; @@ -2688,7 +2696,8 @@ bool PreCallValidateCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_sta // TODO: Currently treating all image accesses as imprecise & covering the whole image area AddReadMemoryAccess(CMD_BLITIMAGE, mem_accesses, src_image_state->binding, false); AddWriteMemoryAccess(CMD_BLITIMAGE, mem_accesses, dst_image_state->binding, false); - skip |= ValidateMemoryAccesses(report_data, cb_state, mem_accesses, "vkCmdBlitImage()"); + skip |= ValidateMemoryAccesses(report_data, cb_state->commandBuffer, cb_state->mem_accesses.access_map, mem_accesses, + "vkCmdBlitImage()"); } else { assert(0); } @@ -3618,7 +3627,8 @@ bool PreCallValidateCmdCopyBuffer(layer_data *device_data, GLOBAL_CB_NODE *cb_st AddReadMemoryAccess(CMD_COPYBUFFER, mem_accesses, {src_mem_obj, reg.srcOffset, reg.size}, true); AddWriteMemoryAccess(CMD_COPYBUFFER, mem_accesses, {dst_mem_obj, reg.dstOffset, reg.size}, true); } - skip |= ValidateMemoryAccesses(core_validation::GetReportData(device_data), cb_state, mem_accesses, "vkCmdCopyBuffer()"); + skip |= ValidateMemoryAccesses(core_validation::GetReportData(device_data), cb_state->commandBuffer, + cb_state->mem_accesses.access_map, mem_accesses, "vkCmdCopyBuffer()"); return skip; } @@ -3732,7 +3742,8 @@ bool PreCallValidateCmdFillBuffer(layer_data *device_data, GLOBAL_CB_NODE *cb_st "vkCmdFillBuffer()", "VK_BUFFER_USAGE_TRANSFER_DST_BIT"); skip |= insideRenderPass(device_data, cb_state, "vkCmdFillBuffer()", VALIDATION_ERROR_1b400017); AddWriteMemoryAccess(CMD_FILLBUFFER, mem_accesses, {buffer_state->binding.mem, offset, size}, true); - skip |= ValidateMemoryAccesses(core_validation::GetReportData(device_data), cb_state, mem_accesses, "vkCmdFillBuffer()"); + skip |= ValidateMemoryAccesses(core_validation::GetReportData(device_data), cb_state->commandBuffer, + cb_state->mem_accesses.access_map, mem_accesses, "vkCmdFillBuffer()"); return skip; } From 5fde2f9905788646bfd108411e468bb9e76c591b Mon Sep 17 00:00:00 2001 From: Tobin Ehlis Date: Tue, 22 Aug 2017 12:52:39 -0600 Subject: [PATCH 23/28] layers:Validate inter-CB memory conflicts At QueueSubmit time, check for any memory conflicts between command buffers in a single submit. Here's the high-level algorithm: 1. While submitting CBs, store vector of CB state for each CB 2. For each CB, create vector of mem accesses that are "live" meaning that no barrier within the CB made them visible 3. Check live vector against all other live mem accesses up to this point in submit. 3a. If there are no potential conflicts, save live access vector into submit access map and continue with next submit 3b. If there are potential conflicts, we need to replay commands up to this point so continue to step 4. 4. Gather all cmds submitted up to this point in a single vector 5. Note any synch commands that occur between the early and late mem access conflict commands 6. Replay mem access commands, including synch commands between the points of interest 7. Following replay, re-check for conflicts and if they still occur, flag an error Also update the race condition warning message between two separate command buffers with additional information about second cmd buffer. --- layers/buffer_validation.cpp | 51 +++++-- layers/buffer_validation.h | 6 +- layers/core_validation.cpp | 272 +++++++++++++++++++++++++++++---- layers/core_validation_types.h | 20 ++- 4 files changed, 302 insertions(+), 47 deletions(-) diff --git a/layers/buffer_validation.cpp b/layers/buffer_validation.cpp index e197eefc54..32319ed404 100644 --- a/layers/buffer_validation.cpp +++ b/layers/buffer_validation.cpp @@ -691,18 +691,37 @@ bool MemoryConflict(MemoryAccess const *initial_access, MemoryAccess const *seco // Check given vector of new_mem_accesses against prev_mem_accesses map, which has live R/W access up to the point. // The "live" R/W accesses in prev_mem_access_map means the accesses have no barrier has been identified. // For any new accesses that conflict with prev accesses, flag an error +// If pre_check is true no callback will be issued, instead on any conflict set early & late conflict ptrs and return "true" bool ValidateMemoryAccesses(debug_report_data const *report_data, VkCommandBuffer command_buffer, - const std::unordered_map> &prev_mem_access_map, - std::vector *mem_accesses, const char *caller) { + std::unordered_map> &prev_mem_access_map, + std::vector *mem_accesses, const char *caller, bool pre_check, + MemoryAccess **early_conflict, MemoryAccess **late_conflict) { bool skip = false; - for (const auto &mem_access : *mem_accesses) { + // early out + if (prev_mem_access_map.empty()) return skip; + // Avoid warnings on return params w/ default values + (void)early_conflict; + (void)late_conflict; + + for (auto &mem_access : *mem_accesses) { auto mem_obj = mem_access.location.mem; auto mem_access_pair = prev_mem_access_map.find(mem_obj); if (mem_access_pair != prev_mem_access_map.end()) { - for (const auto &earlier_access : mem_access_pair->second) { + for (auto &earlier_access : mem_access_pair->second) { if (MemoryConflict(&earlier_access, &mem_access)) { + if (pre_check) { + *early_conflict = &earlier_access; + *late_conflict = &mem_access; + return true; + } const char *access1 = earlier_access.write ? "write" : "read"; const char *access2 = mem_access.write ? "write" : "read"; + std::string inter_cb_info = ""; + if (mem_access.cmd && earlier_access.cmd && (mem_access.cmd->cb_state != earlier_access.cmd->cb_state)) { + // This is inter-CB conflict so give a bit more error detail + inter_cb_info = " where the initial access occurred in cmd buffer " + + std::to_string(HandleToUint64(earlier_access.cmd->cb_state->commandBuffer)); + } // If either access is imprecise can only warn, otherwise can error auto level = VK_DEBUG_REPORT_WARNING_BIT_EXT; // TODO: Un-comment the line below when we're confident in coverage of memory access checks @@ -713,12 +732,14 @@ bool ValidateMemoryAccesses(debug_report_data const *report_data, VkCommandBuffe MEMTRACK_SYNCHRONIZATION_ERROR, "DS", "%s called on VkCommandBuffer 0x%" PRIxLEAST64 " causes a %s after %s conflict with memory object 0x%" PRIxLEAST64 - ". NOTE: This" + "%s." + " NOTE: This" " race condition warning is a new feature in validation that still has some holes" " in tracking Read/Write accesses as well as modeling synchronization objects. Don't" " spend too much time investigating this warning, and feel free to file GitHub bugs " " on any warning cases that you known are incorrect.", - caller, HandleToUint64(command_buffer), access2, access1, HandleToUint64(mem_obj)); + caller, HandleToUint64(command_buffer), access2, access1, HandleToUint64(mem_obj), + inter_cb_info.c_str()); } } } @@ -728,7 +749,7 @@ bool ValidateMemoryAccesses(debug_report_data const *report_data, VkCommandBuffe // Add the given cmd and its mem_accesses to the command buffer void AddCommandBufferCommandMemoryAccesses(GLOBAL_CB_NODE *cb_state, CMD_TYPE cmd, std::vector *mem_accesses) { - cb_state->commands.emplace_back(std::unique_ptr(new Command(cmd))); + cb_state->commands.emplace_back(std::unique_ptr(new Command(cmd, cb_state->commands.size(), cb_state))); Command *cmd_ptr = cb_state->commands.back().get(); for (auto &mem_access : *mem_accesses) { mem_access.cmd = cmd_ptr; @@ -1121,7 +1142,7 @@ bool PreCallValidateCmdClearColorImage(layer_data *dev_data, VkCommandBuffer com // TODO: Currently treating all image accesses as imprecise & covering the whole image area, this should be per-range AddWriteMemoryAccess(CMD_CLEARCOLORIMAGE, mem_accesses, image_state->binding, false); skip |= ValidateMemoryAccesses(core_validation::GetReportData(dev_data), commandBuffer, cb_state->mem_accesses.access_map, - mem_accesses, "vkCmdClearColorImage()"); + mem_accesses, "vkCmdClearColorImage()", false, nullptr, nullptr); return skip; } @@ -1185,7 +1206,7 @@ bool PreCallValidateCmdClearDepthStencilImage(layer_data *device_data, VkCommand // TODO: Currently treating all image accesses as imprecise & covering the whole image area, this should be per-range AddWriteMemoryAccess(CMD_CLEARDEPTHSTENCILIMAGE, mem_accesses, image_state->binding, false); skip |= ValidateMemoryAccesses(report_data, commandBuffer, cb_state->mem_accesses.access_map, mem_accesses, - "vkCmdClearDepthStencilImage()"); + "vkCmdClearDepthStencilImage()", false, nullptr, nullptr); return skip; } @@ -2019,7 +2040,7 @@ bool PreCallValidateCmdCopyImage(layer_data *device_data, GLOBAL_CB_NODE *cb_sta AddReadMemoryAccess(CMD_COPYIMAGE, mem_accesses, src_image_state->binding, false); AddWriteMemoryAccess(CMD_COPYIMAGE, mem_accesses, dst_image_state->binding, false); skip |= ValidateMemoryAccesses(report_data, cb_state->commandBuffer, cb_state->mem_accesses.access_map, mem_accesses, - "vkCmdCopyImage()"); + "vkCmdCopyImage()", false, nullptr, nullptr); } // The formats of src_image and dst_image must be compatible. Formats are considered compatible if their texel size in bytes @@ -2220,7 +2241,7 @@ bool PreCallValidateCmdClearAttachments(layer_data *device_data, GLOBAL_CB_NODE const auto image_state = GetImageState(device_data, image_view_state->create_info.image); AddWriteMemoryAccess(CMD_CLEARATTACHMENTS, mem_accesses, image_state->binding, false); skip |= ValidateMemoryAccesses(report_data, commandBuffer, cb_state->mem_accesses.access_map, mem_accesses, - "vkCmdClearAttachments()"); + "vkCmdClearAttachments()", false, nullptr, nullptr); } } } @@ -2279,7 +2300,7 @@ bool PreCallValidateCmdResolveImage(layer_data *device_data, GLOBAL_CB_NODE *cb_ AddReadMemoryAccess(CMD_RESOLVEIMAGE, mem_accesses, src_image_state->binding, false); AddWriteMemoryAccess(CMD_RESOLVEIMAGE, mem_accesses, dst_image_state->binding, false); skip |= ValidateMemoryAccesses(report_data, cb_state->commandBuffer, cb_state->mem_accesses.access_map, mem_accesses, - "vkCmdResolveImage()"); + "vkCmdResolveImage()", false, nullptr, nullptr); if (src_image_state->createInfo.format != dst_image_state->createInfo.format) { char const str[] = "vkCmdResolveImage called with unmatched source and dest formats."; @@ -2697,7 +2718,7 @@ bool PreCallValidateCmdBlitImage(layer_data *device_data, GLOBAL_CB_NODE *cb_sta AddReadMemoryAccess(CMD_BLITIMAGE, mem_accesses, src_image_state->binding, false); AddWriteMemoryAccess(CMD_BLITIMAGE, mem_accesses, dst_image_state->binding, false); skip |= ValidateMemoryAccesses(report_data, cb_state->commandBuffer, cb_state->mem_accesses.access_map, mem_accesses, - "vkCmdBlitImage()"); + "vkCmdBlitImage()", false, nullptr, nullptr); } else { assert(0); } @@ -3628,7 +3649,7 @@ bool PreCallValidateCmdCopyBuffer(layer_data *device_data, GLOBAL_CB_NODE *cb_st AddWriteMemoryAccess(CMD_COPYBUFFER, mem_accesses, {dst_mem_obj, reg.dstOffset, reg.size}, true); } skip |= ValidateMemoryAccesses(core_validation::GetReportData(device_data), cb_state->commandBuffer, - cb_state->mem_accesses.access_map, mem_accesses, "vkCmdCopyBuffer()"); + cb_state->mem_accesses.access_map, mem_accesses, "vkCmdCopyBuffer()", false, nullptr, nullptr); return skip; } @@ -3743,7 +3764,7 @@ bool PreCallValidateCmdFillBuffer(layer_data *device_data, GLOBAL_CB_NODE *cb_st skip |= insideRenderPass(device_data, cb_state, "vkCmdFillBuffer()", VALIDATION_ERROR_1b400017); AddWriteMemoryAccess(CMD_FILLBUFFER, mem_accesses, {buffer_state->binding.mem, offset, size}, true); skip |= ValidateMemoryAccesses(core_validation::GetReportData(device_data), cb_state->commandBuffer, - cb_state->mem_accesses.access_map, mem_accesses, "vkCmdFillBuffer()"); + cb_state->mem_accesses.access_map, mem_accesses, "vkCmdFillBuffer()", false, nullptr, nullptr); return skip; } diff --git a/layers/buffer_validation.h b/layers/buffer_validation.h index 19bcad2ed2..a54ffc58be 100644 --- a/layers/buffer_validation.h +++ b/layers/buffer_validation.h @@ -144,8 +144,10 @@ void AddWriteMemoryAccess(CMD_TYPE cmd, std::vector *mem_accesses, bool MemoryConflict(MemoryAccess const *initial_access, MemoryAccess const *second_access); -bool ValidateMemoryAccesses(debug_report_data const *report_data, GLOBAL_CB_NODE const *cb_state, std::vector *mem_accesses, - const char *caller); +bool ValidateMemoryAccesses(debug_report_data const *report_data, VkCommandBuffer command_buffer, + std::unordered_map> &prev_mem_access_map, + std::vector *mem_accesses, const char *caller, bool pre_check, + MemoryAccess **early_conflict, MemoryAccess **late_conflict); void AddCommandBufferCommandMemoryAccesses(GLOBAL_CB_NODE *cb_state, CMD_TYPE cmd, std::vector *mem_accesses); diff --git a/layers/core_validation.cpp b/layers/core_validation.cpp index 46a4c41abc..c6576c6bcd 100644 --- a/layers/core_validation.cpp +++ b/layers/core_validation.cpp @@ -2623,6 +2623,10 @@ static void PostCallRecordQueueSubmit(layer_data *dev_data, VkQueue queue, uint3 } } +// Prototype +void ReplayMemoryAccessCommands(layer_data *, std::vector> *, uint32_t, uint32_t, + std::unordered_map> *, std::vector *); + static bool PreCallValidateQueueSubmit(layer_data *dev_data, VkQueue queue, uint32_t submitCount, const VkSubmitInfo *pSubmits, VkFence fence) { auto pFence = GetFenceNode(dev_data, fence); @@ -2636,6 +2640,14 @@ static bool PreCallValidateQueueSubmit(layer_data *dev_data, VkQueue queue, uint vector current_cmds; unordered_map localImageLayoutMap; // Now verify each individual submit + // We'll store a map of submit's memory accesses as we replace cmd buffers to find cross-cmd-buffer memory conflicts + std::unordered_map> submit_mem_access_map; + // If we hit a potential cross-CB synch conflict, we'll replay CBs up to point of conflict to verify if conflict is real + // We have to copy the commands into local vector b/c we can't modify in the CBs themselves + std::vector> submit_cmds; + std::vector replay_command_buffers; + uint32_t start_replay_index = 0, end_replay_index = 0; // Indices that set bounds for replaying synch cmds + std::vector cb_live_accesses; // Store up mem accesses per CB that are still live (not visible) for (uint32_t submit_idx = 0; submit_idx < submitCount; submit_idx++) { const VkSubmitInfo *submit = &pSubmits[submit_idx]; for (uint32_t i = 0; i < submit->waitSemaphoreCount; ++i) { @@ -2677,6 +2689,7 @@ static bool PreCallValidateQueueSubmit(layer_data *dev_data, VkQueue queue, uint if (cb_node) { skip |= ValidateCmdBufImageLayouts(dev_data, cb_node, dev_data->imageLayoutMap, localImageLayoutMap); current_cmds.push_back(submit->pCommandBuffers[i]); + replay_command_buffers.push_back(cb_node); skip |= validatePrimaryCommandBufferState( dev_data, cb_node, (int)std::count(current_cmds.begin(), current_cmds.end(), submit->pCommandBuffers[i])); skip |= validateQueueFamilyIndices(dev_data, cb_node, queue); @@ -2696,6 +2709,67 @@ static bool PreCallValidateQueueSubmit(layer_data *dev_data, VkQueue queue, uint for (auto &function : cb_node->queryUpdates) { skip |= function(queue); } + // TODO : Move this block of memory access check code to its own function, adding in-line to start + // Gather all outstanding accesses for this cmd buffer into a vector (Should already have this data struct + // pre-built) + cb_live_accesses.clear(); + for (const auto &mem_access_pair : cb_node->mem_accesses.access_map) { + for (const auto &mem_access : mem_access_pair.second) { + if (!mem_access.Visible()) { + // No applicable barrier so access is live + cb_live_accesses.push_back(mem_access); + } + } + } + // Run a pre-check and if there's no conflicts, just record updated accesses + MemoryAccess *early_conflict = nullptr, *late_conflict = nullptr; + if (!ValidateMemoryAccesses(dev_data->report_data, cb_node->commandBuffer, submit_mem_access_map, &cb_live_accesses, + "vkQueueSubmit()", true, &early_conflict, &late_conflict)) { + // Record accesses for this CB into Submit access going fwd + for (const auto &mem_access : cb_live_accesses) { + submit_mem_access_map[mem_access.location.mem].push_back(mem_access); + } + } else { // We have a pre-check conflict so have to replay Cmds to verify if conflict is real or cleared by a + // inter-CB synch + // This is the slow path + // We have to replay so copy cmd sequence up to this point into local vector + // Then mark seq replay start & seq replay end indices which are seq cmds between conflicting mem accesses that + // must be replayed + bool set_start_index = false, have_end_index = false; + for (const auto &cbstate : replay_command_buffers) { + for (const auto &cmd : cbstate->commands) { + if (cmd.get() == early_conflict->cmd) { + set_start_index = true; // We'll grab next synch cmd index + } else if (cmd.get() == late_conflict->cmd) { + have_end_index = true; + } + if (cmd->synch) { + if (set_start_index) { + start_replay_index = submit_cmds.size(); + } + if (!have_end_index) { // We'll just keep grabbing synch commands until we find late conflict + // command above + end_replay_index = submit_cmds.size(); + } + SynchCommand *synch_cmd = static_cast(cmd.get()); + submit_cmds.emplace_back(unique_ptr(new SynchCommand(*synch_cmd))); + } else { + submit_cmds.emplace_back(unique_ptr(new Command(*cmd))); + } + } + } + if (start_replay_index && end_replay_index) { + // We have synch commands between the conflict to replay. Clear current access map as well as live accesses + // which will both be filled on replay. + submit_mem_access_map.clear(); + cb_live_accesses.clear(); + ReplayMemoryAccessCommands(dev_data, &submit_cmds, start_replay_index, end_replay_index, + &submit_mem_access_map, &cb_live_accesses); + } + // Now that any synch replays are done, validate the mem accesses for real + skip |= ValidateMemoryAccesses(dev_data->report_data, cb_node->commandBuffer, submit_mem_access_map, + &cb_live_accesses, "vkQueueSubmit()", false, nullptr, nullptr); + } } } } @@ -5722,7 +5796,8 @@ static bool ValidateCmdDrawType(layer_data *dev_data, VkCommandBuffer cmd_buffer UpdateMemoryAccessVector(dev_data, cmd_type, mem_accesses, dd_mem_accesses); } } - skip |= ValidateMemoryAccesses(dev_data->report_data, *cb_state, mem_accesses, caller); + skip |= ValidateMemoryAccesses(dev_data->report_data, cmd_buffer, (*cb_state)->mem_accesses.access_map, mem_accesses, + caller, false, nullptr, nullptr); } } return skip; @@ -5814,7 +5889,8 @@ static bool PreCallValidateCmdDrawIndirect(layer_data *dev_data, VkCommandBuffer AddReadMemoryAccess(CMD_DRAWINDIRECT, mem_accesses, {(*buffer_state)->binding.mem, offset, size}, false); // TODO: Need a special draw mem access call here (or in existing shared function) that analyzes state and adds related R/W // accesses - skip |= ValidateMemoryAccesses(dev_data->report_data, *cb_state, mem_accesses, caller); + skip |= ValidateMemoryAccesses(dev_data->report_data, cmd_buffer, (*cb_state)->mem_accesses.access_map, mem_accesses, caller, + false, nullptr, nullptr); // TODO: If the drawIndirectFirstInstance feature is not enabled, all the firstInstance members of the // VkDrawIndirectCommand structures accessed by this command must be 0, which will require access to the contents of 'buffer'. return skip; @@ -6094,7 +6170,8 @@ static bool PreCallCmdUpdateBuffer(layer_data *device_data, VkCommandBuffer comm skip |= insideRenderPass(device_data, *cb_state, "vkCmdUpdateBuffer()", VALIDATION_ERROR_1e400017); // Add mem access for writing buffer AddWriteMemoryAccess(CMD_UPDATEBUFFER, mem_accesses, (*dst_buffer_state)->binding, true); - skip |= ValidateMemoryAccesses(device_data->report_data, *cb_state, mem_accesses, "vkCmdUpdateBuffer()"); + skip |= ValidateMemoryAccesses(device_data->report_data, commandBuffer, (*cb_state)->mem_accesses.access_map, mem_accesses, + "vkCmdUpdateBuffer()", false, nullptr, nullptr); return skip; } @@ -6886,6 +6963,153 @@ bool ValidateStageMasksAgainstQueueCapabilities(layer_data *dev_data, GLOBAL_CB_ return skip; } +// Merge the given synch command with any previous synch cmds from given vector of synch commands +// This resolves dependency chains by pulling overlapping earlier barriers into current barrier +// If current barriers' srcMasks match with previous barriers destination masks, then incorporate the +// previous barrier src & dst Masks into our src/dst Masks & merge in all of the previous barriers. +// Return "true" if any merge occurs, "false" otherwise +static bool MergeSynchCommands(const std::vector &prev_synch_commands, SynchCommand *synch_command) { + bool merge = false; + for (const auto &psc : prev_synch_commands) { + if (psc->dst_stage_flags & synch_command->src_stage_flags) { + merge = true; + // scopes overlap so merge prev barrier mem masks into current masks + synch_command->src_stage_flags |= psc->src_stage_flags; + synch_command->dst_stage_flags |= psc->dst_stage_flags; + // Pull previous barriers into this barrier + for (const auto &gb : psc->global_barriers) { + synch_command->global_barriers.emplace_back(gb); + } + for (const auto &bb : psc->buffer_barriers) { + synch_command->buffer_barriers.emplace_back(bb); + } + for (const auto &ib : psc->image_barriers) { + synch_command->image_barriers.emplace_back(ib); + } + } + } + return merge; +} + +// Record given synch cmd vs. outstanding memory accesses up to the point of the synch command, +// for each barrier, compare given existing mem accesses and record any applicable barriers +static void ReplaySynchCommand(layer_data *device_data, SynchCommand *synch_cmd, + std::unordered_map> *access_map) { + // cb_state->commands.emplace_back( + // unique_ptr(new SynchCommand(cmd, cb_state->commands.size(), cb_state, src_stage_mask, dst_stage_mask))); + // Command *cmd_ptr = cb_state->commands.back().get(); + // SynchCommand *synch_cmd_ptr = static_cast(cmd_ptr); + // // First merge any overlapping previous barriers. + // MergeSynchCommands(cb_state->synch_commands, synch_cmd_ptr); + // // Now that we've merged previous synch commands, append this cmd to existing synch commands + // cb_state->synch_commands.emplace_back(synch_cmd_ptr); + auto src_stage_mask = synch_cmd->src_stage_flags; + // TODO: Make this barrier/access parsing smarter + for (auto &mem_access_pair : *access_map) { + for (auto &mem_access : mem_access_pair.second) { + if (mem_access.src_stage_flags & src_stage_mask) { + // Record any execution barrier overlaps + mem_access.pipe_barrier = true; + mem_access.dst_stage_flags |= synch_cmd->dst_stage_flags; + // For every global barrier that matches access mask, record the barrier + for (const auto &global_barrier : synch_cmd->global_barriers) { + if (0 != (global_barrier.srcAccessMask & mem_access.src_access_flags)) { + // This memory barrier applies to mem_access so record details + mem_access.mem_barrier = true; + mem_access.dst_access_flags |= global_barrier.dstAccessMask; + mem_access.synch_commands.push_back(synch_cmd); + } + } + } + } + } + for (const auto &buff_barrier : synch_cmd->buffer_barriers) { + const auto &buff_state = GetBufferState(device_data, buff_barrier.buffer); + const auto mem_obj = buff_state->binding.mem; + // Make an access struct with barrier range details to check for overlap + // TODO: This is hacky, creating tmp access struct from barrier to check conflict + // need to rework original function so this makes sense or write alternate function + MemoryAccess barrier_access = {}; + barrier_access.location.mem = mem_obj; + barrier_access.location.offset = buff_barrier.offset; + barrier_access.location.size = buff_barrier.size; + barrier_access.mem_barrier = false; + barrier_access.pipe_barrier = false; + const auto &mem_access_pair = access_map->find(mem_obj); + if (mem_access_pair != access_map->end()) { + for (auto &mem_access : mem_access_pair->second) { + // If the pipe & access masks overlap, then barrier applies + if (((mem_access.src_access_flags & buff_barrier.srcAccessMask) != 0) && + ((mem_access.src_stage_flags & src_stage_mask) != 0)) { + // Set buff access write opposite of actual access so conflict is flagged on overlap + barrier_access.write = !mem_access.write; + if (MemoryConflict(&barrier_access, &mem_access)) { + mem_access.mem_barrier = true; + mem_access.dst_stage_flags |= synch_cmd->dst_stage_flags; + mem_access.dst_access_flags |= buff_barrier.dstAccessMask; + mem_access.synch_commands.push_back(synch_cmd); + } + } + } + } + } + // TODO: Handle image barriers +} + +// For the given vector of commands, replay memory access commands, only replaying synch commands that occur between +// start/end_synch_index For memory commands that are hit, put them into an access map For Synch commands between the indices that +// are hit merge them with any previous synch commands then: +// 1. Record them against any outstanding accesses +// 2. Add them into a synch cmd vector for future merges +// When the synch command at end_synch_index is replayed, we then store any remaining mem access commands into remaining_accesses +// vector. +// This will include the previously conflicting late access, so the returned vector of live accesses can be checked against updated +// access_map. +void ReplayMemoryAccessCommands(layer_data *device_data, std::vector> *commands, + uint32_t start_synch_index, uint32_t end_synch_index, + std::unordered_map> *access_map, + std::vector *remaining_accesses) { + if (commands->empty()) { + return; // early out + } + auto index = 0; + // Store running list of synch commands for merge purposes + std::vector prev_synch_commands; + Command *cmd_ptr = nullptr; + while (index <= end_synch_index) { + cmd_ptr = (*commands)[index++].get(); + // 1. Merge synch cmd and build up synch vector + // Note that we only replay synch commands between the mem access points of interest where the potential conflict occurs + if (cmd_ptr->synch && (index >= start_synch_index && index <= end_synch_index)) { + auto synch_cmd_ptr = static_cast(cmd_ptr); + MergeSynchCommands(prev_synch_commands, synch_cmd_ptr); + prev_synch_commands.push_back(synch_cmd_ptr); + // Now check synch command against outstanding memory accesses + ReplaySynchCommand(device_data, synch_cmd_ptr, access_map); + } else if (!cmd_ptr->synch) { + // Currently all non-synch commands should be memory access commands, if this assert fails, this + // code must be updated to handle different command types + assert(!cmd_ptr->mem_accesses.empty()); + // Record memory accesses into access map going fwd + for (const auto &mem_access : cmd_ptr->mem_accesses) { + (*access_map)[mem_access.location.mem].push_back(mem_access); + } + } + // 2. If we do merge synch commands, we'll need to re-check accesses + } + // Any remaining mem access commands (after last synch) are added into live access vector + while (index < commands->size()) { + cmd_ptr = (*commands)[index++].get(); + // If there are no mem_accesses in the cmd (such as a later synch) this loop does nothing + for (const auto &mem_access : cmd_ptr->mem_accesses) { + if (!mem_access.Visible()) { + // We only want mem accesses that are still live + remaining_accesses->push_back(mem_access); + } + } + } +} + // Record given set of barriers vs. outstanding memory accesses for this cmd buffer // 1. First merge this barrier within any previous barriers in this CB // 2. For each barrier, compare given existing mem accesses and record any applicable barriers @@ -6895,31 +7119,12 @@ static void RecordBarrierMemoryAccess(layer_data *device_data, CMD_TYPE cmd, GLO const VkMemoryBarrier *mem_barriers, uint32_t buffer_mem_barrier_count, const VkBufferMemoryBarrier *buffer_mem_barriers, uint32_t image_memory_barrier_count, const VkImageMemoryBarrier *image_memory_barriers) { - cb_state->commands.emplace_back(unique_ptr(new SynchCommand(cmd, src_stage_mask, dst_stage_mask))); + cb_state->commands.emplace_back( + unique_ptr(new SynchCommand(cmd, cb_state->commands.size(), cb_state, src_stage_mask, dst_stage_mask))); Command *cmd_ptr = cb_state->commands.back().get(); SynchCommand *synch_cmd_ptr = static_cast(cmd_ptr); - // First thing to do is parse through any previous barriers. If this barriers' srcMasks match with - // previous barriers destination masks, then incorporate the previous barrier srcMasks into our srcMask - // and merge in all of the previous barriers. This will account for dependency chains. - for (const auto &synch_cmd : cb_state->synch_commands) { - if (synch_cmd->dst_stage_flags & src_stage_mask) { - // scopes overlap so merge prev barrier masks into current masks - src_stage_mask |= synch_cmd->src_stage_flags; - synch_cmd_ptr->src_stage_flags = src_stage_mask; - dst_stage_mask |= synch_cmd->dst_stage_flags; - synch_cmd_ptr->dst_stage_flags = dst_stage_mask; - // Pull all previous overlapping barriers into this barrier - for (const auto &global_barrier : synch_cmd->global_barriers) { - synch_cmd_ptr->global_barriers.emplace_back(global_barrier); - } - for (const auto &buff_barrier : synch_cmd->buffer_barriers) { - synch_cmd_ptr->buffer_barriers.emplace_back(buff_barrier); - } - for (const auto &img_barrier : synch_cmd->image_barriers) { - synch_cmd_ptr->image_barriers.emplace_back(img_barrier); - } - } - } + // First merge any overlapping previous barriers. + MergeSynchCommands(cb_state->synch_commands, synch_cmd_ptr); // Now that we've merged previous synch commands, append this cmd to existing synch commands cb_state->synch_commands.emplace_back(synch_cmd_ptr); // TODO: Make this barrier/access parsing smarter @@ -6973,6 +7178,18 @@ static void RecordBarrierMemoryAccess(layer_data *device_data, CMD_TYPE cmd, GLO } } } + // TODO: Handle image barriers + + // Now need to record these barriers into the cmd for any future merges + for (uint32_t i = 0; i < mem_barrier_count; ++i) { + synch_cmd_ptr->global_barriers.emplace_back(mem_barriers[i]); + } + for (uint32_t i = 0; i < buffer_mem_barrier_count; ++i) { + synch_cmd_ptr->buffer_barriers.emplace_back(buffer_mem_barriers[i]); + } + for (uint32_t i = 0; i < image_memory_barrier_count; ++i) { + synch_cmd_ptr->image_barriers.emplace_back(image_memory_barriers[i]); + } } VKAPI_ATTR void VKAPI_CALL CmdWaitEvents(VkCommandBuffer commandBuffer, uint32_t eventCount, const VkEvent *pEvents, @@ -7247,7 +7464,8 @@ static bool PreCallValidateCmdCopyQueryPoolResults(layer_data *device_data, GLOB auto precise = (element_size == stride) ? true : false; auto size = stride * (query_count - 1) + num_bytes; AddWriteMemoryAccess(CMD_COPYQUERYPOOLRESULTS, mem_accesses, {dst_buffer_state->binding.mem, offset, size}, precise); - skip |= ValidateMemoryAccesses(device_data->report_data, cb_state, mem_accesses, "vkCmdCopyQueryPoolResults()"); + skip |= ValidateMemoryAccesses(device_data->report_data, cb_state->commandBuffer, cb_state->mem_accesses.access_map, + mem_accesses, "vkCmdCopyQueryPoolResults()", false, nullptr, nullptr); return skip; } diff --git a/layers/core_validation_types.h b/layers/core_validation_types.h index 94674071a0..438bafe5ee 100644 --- a/layers/core_validation_types.h +++ b/layers/core_validation_types.h @@ -619,6 +619,12 @@ struct MemoryAccess { mem_barrier(false), pipe_barrier(false), synch_commands{} {}; + + // Once an appropriate barrier has been identified for an access, it will be Visible + bool Visible() const { + auto visible = (mem_barrier || (pipe_barrier && write)); + return visible; + }; bool precise; // True if exact bounds of mem access are known, false if bounds are conservative bool write; // False for read access Command *cmd; // Ptr to cmd that makes this access @@ -640,17 +646,25 @@ struct MemoryAccess { class Command { public: - Command(CMD_TYPE type) : type(type){}; + Command(CMD_TYPE type, uint32_t seq, GLOBAL_CB_NODE *gcb) : type(type), seq(seq), cb_state(gcb), replay(false), synch(false){}; + Command(CMD_TYPE type, uint32_t seq, GLOBAL_CB_NODE *gcb, bool synch) + : type(type), seq(seq), cb_state(gcb), replay(false), synch(synch){}; virtual ~Command() {} void AddMemoryAccess(MemoryAccess access) { mem_accesses.push_back(access); }; + void SetSeq(uint32_t seq_num) { seq = seq_num; }; CMD_TYPE type; + uint32_t seq; // seq # of cmd in this cmd buffer + GLOBAL_CB_NODE *cb_state; + bool replay; // Track if cmd has been replayed during QueueSubmit + bool synch; std::vector mem_accesses; // vector of all mem accesses by this cmd }; class SynchCommand : public Command { public: - SynchCommand(CMD_TYPE type, VkPipelineStageFlags src_stage_flags, VkPipelineStageFlags dst_stage_flags) - : Command(type), src_stage_flags(src_stage_flags), dst_stage_flags(dst_stage_flags){}; + SynchCommand(CMD_TYPE type, uint32_t seq, GLOBAL_CB_NODE *gcb, VkPipelineStageFlags src_stage_flags, + VkPipelineStageFlags dst_stage_flags) + : Command(type, seq, gcb, true), src_stage_flags(src_stage_flags), dst_stage_flags(dst_stage_flags){}; VkPipelineStageFlags src_stage_flags; VkPipelineStageFlags dst_stage_flags; std::vector global_barriers; From 98eeca4ecf29e7568766a693b9f41a6a2909836d Mon Sep 17 00:00:00 2001 From: Tobin Ehlis Date: Thu, 24 Aug 2017 13:05:24 -0600 Subject: [PATCH 24/28] tests:Add inter-CB memory access conflict test Added TwoCommandBufferUpdateBufferRaWDependencyMissingBarrier test that is an alternate version of UpdateBufferRaWDependencyMissingBarrier test that separates the CmdUpdateBuffer and CmdDrawIndirect commands across two separate command buffers and verifies that the RaW race condition is flagged. --- tests/layer_validation_tests.cpp | 92 ++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/tests/layer_validation_tests.cpp b/tests/layer_validation_tests.cpp index 646c271487..4197ac17d4 100644 --- a/tests/layer_validation_tests.cpp +++ b/tests/layer_validation_tests.cpp @@ -10481,6 +10481,98 @@ TEST_F(VkLayerTest, UpdateBufferRaWDependencyMissingBarrier) { vkDestroyPipelineLayout(m_device->device(), pipeline_layout, nullptr); } +TEST_F(VkLayerTest, TwoCommandBufferUpdateBufferRaWDependencyMissingBarrier) { + TEST_DESCRIPTION( + "Update buffer in first command buffer then use buffer in CmdDrawIndirect in second command buffer with no barrier before " + "use"); + + ASSERT_NO_FATAL_FAILURE(Init()); + ASSERT_NO_FATAL_FAILURE(InitRenderTarget()); + + VkCommandBuffer cmd_bufs[2]; + VkCommandBufferAllocateInfo alloc_info; + alloc_info.sType = VK_STRUCTURE_TYPE_COMMAND_BUFFER_ALLOCATE_INFO; + alloc_info.pNext = nullptr; + alloc_info.commandBufferCount = 2; + alloc_info.commandPool = m_commandPool->handle(); + alloc_info.level = VK_COMMAND_BUFFER_LEVEL_PRIMARY; + vkAllocateCommandBuffers(m_device->device(), &alloc_info, cmd_bufs); + + VkCommandBufferBeginInfo cbbi; + cbbi.pNext = nullptr; + cbbi.sType = VK_STRUCTURE_TYPE_COMMAND_BUFFER_BEGIN_INFO; + cbbi.pInheritanceInfo = VK_NULL_HANDLE; + cbbi.flags = 0; + vkBeginCommandBuffer(cmd_bufs[0], &cbbi); + + VkMemoryPropertyFlags reqs = VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT; + vk_testing::Buffer dst_buffer; + dst_buffer.init_as_dst(*m_device, (VkDeviceSize)1024, reqs); + + VkDeviceSize dstOffset = 0; + uint32_t Data[] = {1, 2, 3, 4, 5, 6, 7, 8}; + VkDeviceSize dataSize = sizeof(Data) / sizeof(uint32_t); + vkCmdUpdateBuffer(cmd_bufs[0], dst_buffer.handle(), dstOffset, dataSize, &Data); + // No barrier between here and use of buffer in follow-on CB should cause RaW issue + vkEndCommandBuffer(cmd_bufs[0]); + + vkBeginCommandBuffer(cmd_bufs[1], &cbbi); + // Start renderpass for DrawIndirect + vkCmdBeginRenderPass(cmd_bufs[1], &m_renderPassBeginInfo, VK_SUBPASS_CONTENTS_INLINE); + // Bind gfx PSO to prevent unexpected errors + // Create PSO to be used for draw-time errors below + VkShaderObj vs(m_device, bindStateVertShaderText, VK_SHADER_STAGE_VERTEX_BIT, this); + VkShaderObj fs(m_device, bindStateFragShaderText, VK_SHADER_STAGE_FRAGMENT_BIT, this); + VkPipelineObj pipe(m_device); + pipe.AddShader(&vs); + pipe.AddShader(&fs); + VkViewport view_port = {}; + m_viewports.push_back(view_port); + pipe.SetViewport(m_viewports); + VkRect2D rect = {}; + m_scissors.push_back(rect); + pipe.SetScissor(m_scissors); + pipe.AddColorAttachment(); + VkPipelineLayoutCreateInfo pipeline_layout_ci = {}; + pipeline_layout_ci.sType = VK_STRUCTURE_TYPE_PIPELINE_LAYOUT_CREATE_INFO; + pipeline_layout_ci.setLayoutCount = 0; + pipeline_layout_ci.pSetLayouts = NULL; + + VkPipelineLayout pipeline_layout; + VkResult err = vkCreatePipelineLayout(m_device->device(), &pipeline_layout_ci, NULL, &pipeline_layout); + ASSERT_VK_SUCCESS(err); + pipe.CreateVKPipeline(pipeline_layout, renderPass()); + + vkCmdBindPipeline(cmd_bufs[1], VK_PIPELINE_BIND_POINT_GRAPHICS, pipe.handle()); + // Now issue indirect draw using buffer written in 1st cmd buffer + vkCmdDrawIndirect(cmd_bufs[1], dst_buffer.handle(), 0, 1, 0); + vkCmdEndRenderPass(cmd_bufs[1]); + vkEndCommandBuffer(cmd_bufs[1]); + // Now submit the CBs back-to-back and verify that error occurs + VkSubmitInfo submit_info[2] = {}; + submit_info[0].sType = VK_STRUCTURE_TYPE_SUBMIT_INFO; + submit_info[0].commandBufferCount = 2; + submit_info[0].pCommandBuffers = cmd_bufs; + m_errorMonitor->SetDesiredFailureMsg(VK_DEBUG_REPORT_WARNING_BIT_EXT, " causes a read after write conflict with "); + vkQueueSubmit(m_device->m_queue, 1, submit_info, VK_NULL_HANDLE); + m_errorMonitor->VerifyFound(); + vkQueueWaitIdle(m_device->m_queue); + + // Submit the same cmd buffers in two separate submits + submit_info[0].sType = VK_STRUCTURE_TYPE_SUBMIT_INFO; + submit_info[0].commandBufferCount = 1; + submit_info[0].pCommandBuffers = &cmd_bufs[0]; + submit_info[1].sType = VK_STRUCTURE_TYPE_SUBMIT_INFO; + submit_info[1].commandBufferCount = 1; + submit_info[1].pCommandBuffers = &cmd_bufs[1]; + m_errorMonitor->SetDesiredFailureMsg(VK_DEBUG_REPORT_WARNING_BIT_EXT, " causes a read after write conflict with "); + vkQueueSubmit(m_device->m_queue, 2, submit_info, VK_NULL_HANDLE); + m_errorMonitor->VerifyFound(); + vkQueueWaitIdle(m_device->m_queue); + + vkDestroyPipelineLayout(m_device->device(), pipeline_layout, nullptr); +} + TEST_F(VkLayerTest, ClearColorImageWithBadRange) { TEST_DESCRIPTION("Record clear color with an invalid VkImageSubresourceRange"); From 172b8868cea72966af041d5638f14edceaefad36 Mon Sep 17 00:00:00 2001 From: Tobin Ehlis Date: Thu, 24 Aug 2017 16:55:24 -0600 Subject: [PATCH 25/28] layers:Add comment for potential bug Merging synch barriers may be a bit naive. Note potential bug for future fix. --- layers/core_validation.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/layers/core_validation.cpp b/layers/core_validation.cpp index c6576c6bcd..576b7b4aa7 100644 --- a/layers/core_validation.cpp +++ b/layers/core_validation.cpp @@ -6968,6 +6968,10 @@ bool ValidateStageMasksAgainstQueueCapabilities(layer_data *dev_data, GLOBAL_CB_ // If current barriers' srcMasks match with previous barriers destination masks, then incorporate the // previous barrier src & dst Masks into our src/dst Masks & merge in all of the previous barriers. // Return "true" if any merge occurs, "false" otherwise +// TODO: Need to analyze this more and verify that it's correct. There may be a bug here where previous +// barriers get hoisted to a current barrier, but can then be applied to mem accesses that follow the +// hoisted barriers. I think that's wrong and if so could potentially be fixed by tracking seq# (or ptr +// to cmd of origin) for barriers and check sequencing when clearing mem accesses. static bool MergeSynchCommands(const std::vector &prev_synch_commands, SynchCommand *synch_command) { bool merge = false; for (const auto &psc : prev_synch_commands) { From e7c0cee1588069af5693bdd25782a85a12925f38 Mon Sep 17 00:00:00 2001 From: Tobin Ehlis Date: Thu, 24 Aug 2017 17:24:11 -0600 Subject: [PATCH 26/28] tests: Add positive inter-CB synch test Add MultiCBDrawWithBufferWaRandRaWSynchChain test. This features three command buffers with mem access dependencies and synch commands that cross CB boundaries. This is a positive test to verify that no errors occur. --- tests/layer_validation_tests.cpp | 172 +++++++++++++++++++++++++++++++ 1 file changed, 172 insertions(+) diff --git a/tests/layer_validation_tests.cpp b/tests/layer_validation_tests.cpp index 4197ac17d4..10c97db25c 100644 --- a/tests/layer_validation_tests.cpp +++ b/tests/layer_validation_tests.cpp @@ -10334,6 +10334,178 @@ TEST_F(VkLayerTest, DrawWithBufferWaRandRaWConflicts) { vkDestroyPipelineLayout(m_device->device(), pipeline_layout, nullptr); } +// This is a positive test to verify inter-CB memory access code is working correctly +// CB0 writes a SBuff +// CB1 has a synch for the write, copies SBuff to UBuff, and has a synch that doesn't affect copy +// CB2 has a synch that chains w/ last synch from prev CB and a Draw that reads UBuff & writes SBuff +TEST_F(VkPositiveLayerTest, MultiCBDrawWithBufferWaRandRaWSynchChain) { + TEST_DESCRIPTION( + "Create a sequence of 3 CBs that contain memory dependencies that are" + "cleared by synchs across CBs."); + + m_errorMonitor->ExpectSuccess(); + ASSERT_NO_FATAL_FAILURE(Init()); + ASSERT_NO_FATAL_FAILURE(InitViewport()); + ASSERT_NO_FATAL_FAILURE(InitRenderTarget()); + + VkCommandBuffer cmd_bufs[3]; + VkCommandBufferAllocateInfo alloc_info; + alloc_info.sType = VK_STRUCTURE_TYPE_COMMAND_BUFFER_ALLOCATE_INFO; + alloc_info.pNext = nullptr; + alloc_info.commandBufferCount = 3; + alloc_info.commandPool = m_commandPool->handle(); + alloc_info.level = VK_COMMAND_BUFFER_LEVEL_PRIMARY; + vkAllocateCommandBuffers(m_device->device(), &alloc_info, cmd_bufs); + + // First command buffer will write into storage buffer + VkDeviceSize buff_size = 1024; + uint32_t qfi = 0; + VkBufferCreateInfo bci = {}; + bci.sType = VK_STRUCTURE_TYPE_BUFFER_CREATE_INFO; + bci.usage = VK_BUFFER_USAGE_STORAGE_BUFFER_BIT | VK_BUFFER_USAGE_TRANSFER_SRC_BIT | VK_BUFFER_USAGE_TRANSFER_DST_BIT; + bci.size = buff_size; + bci.queueFamilyIndexCount = 1; + bci.pQueueFamilyIndices = &qfi; + VkMemoryPropertyFlags reqs = VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT; + vk_testing::Buffer storage_buffer; + storage_buffer.init(*m_device, bci, reqs); + bci.usage = VK_BUFFER_USAGE_UNIFORM_BUFFER_BIT | VK_BUFFER_USAGE_TRANSFER_DST_BIT; + vk_testing::Buffer uniform_buffer; + uniform_buffer.init(*m_device, bci, reqs); + + VkCommandBufferBeginInfo cbbi; + cbbi.pNext = nullptr; + cbbi.sType = VK_STRUCTURE_TYPE_COMMAND_BUFFER_BEGIN_INFO; + cbbi.pInheritanceInfo = VK_NULL_HANDLE; + cbbi.flags = 0; + vkBeginCommandBuffer(cmd_bufs[0], &cbbi); + + VkDeviceSize offset = 0; + uint32_t num_elements = (uint32_t)buff_size / sizeof(uint32_t); // Number of 32bit elements + std::vector Data(num_elements, 0); + // Fill in data buffer + for (uint32_t i = 0; i < num_elements; ++i) { + Data[i] = i; + } + VkDeviceSize data_size = Data.size() * sizeof(uint32_t); + // Write data into storage buffer + vkCmdUpdateBuffer(cmd_bufs[0], storage_buffer.handle(), offset, data_size, Data.data()); + vkEndCommandBuffer(cmd_bufs[0]); + + // Second command buffer starts with global barrier to clear buffer update + vkBeginCommandBuffer(cmd_bufs[1], &cbbi); + VkMemoryBarrier mem_barrier = {}; + mem_barrier.sType = VK_STRUCTURE_TYPE_MEMORY_BARRIER; + mem_barrier.srcAccessMask = VK_ACCESS_TRANSFER_WRITE_BIT; + mem_barrier.dstAccessMask = VK_ACCESS_TRANSFER_READ_BIT; + vkCmdPipelineBarrier(cmd_bufs[1], VK_PIPELINE_STAGE_TRANSFER_BIT, VK_PIPELINE_STAGE_TRANSFER_BIT, 0, 1, &mem_barrier, 0, + nullptr, 0, nullptr); + // Copy storage buffer contents to uniform buffer + VkBufferCopy buff_copy = {0, // srcOffset + 0, // dstOffset + buff_size}; + vkCmdCopyBuffer(cmd_bufs[1], storage_buffer.handle(), uniform_buffer.handle(), 1, &buff_copy); + // Now add a synch that doesn't affect outstanding R&W, but will chain with synch from next cmd buffer + // TODO: Would like to create synch chain here to verify synch merge code. Initial attempts cause other errors + // mem_barrier.srcAccessMask = VK_ACCESS_TRANSFER_WRITE_BIT | VK_ACCESS_TRANSFER_READ_BIT; + // vkCmdPipelineBarrier(cmd_bufs[1], VK_PIPELINE_STAGE_TRANSFER_BIT, VK_PIPELINE_STAGE_COMPUTE_SHADER_BIT, 0, 1, + // &mem_barrier, 0, nullptr, 0, nullptr); + vkEndCommandBuffer(cmd_bufs[1]); + + // Last Cmd buffer has synch that should chain with previous synch to make buffer accesses visible + vkBeginCommandBuffer(cmd_bufs[2], &cbbi); + mem_barrier.srcAccessMask = VK_ACCESS_TRANSFER_WRITE_BIT | VK_ACCESS_TRANSFER_READ_BIT; + mem_barrier.dstAccessMask = VK_ACCESS_TRANSFER_WRITE_BIT | VK_ACCESS_TRANSFER_READ_BIT; + vkCmdPipelineBarrier(cmd_bufs[2], VK_PIPELINE_STAGE_TRANSFER_BIT, VK_PIPELINE_STAGE_TRANSFER_BIT, 0, 1, &mem_barrier, 0, + nullptr, 0, nullptr); + OneOffDescriptorSet ds(m_device->device(), { + {0, VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER, 1, VK_SHADER_STAGE_ALL, nullptr}, + {1, VK_DESCRIPTOR_TYPE_STORAGE_BUFFER, 1, VK_SHADER_STAGE_ALL, nullptr}, + }); + + VkPipelineLayoutCreateInfo pipeline_layout_ci = {}; + pipeline_layout_ci.sType = VK_STRUCTURE_TYPE_PIPELINE_LAYOUT_CREATE_INFO; + pipeline_layout_ci.setLayoutCount = 1; + pipeline_layout_ci.pSetLayouts = &ds.layout_; + + VkPipelineLayout pipeline_layout; + VkResult err = vkCreatePipelineLayout(m_device->device(), &pipeline_layout_ci, NULL, &pipeline_layout); + ASSERT_VK_SUCCESS(err); + + VkDescriptorBufferInfo buff_info = {}; + buff_info.buffer = uniform_buffer.handle(); + buff_info.range = buff_size; + VkWriteDescriptorSet descriptor_write = {}; + descriptor_write.sType = VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET; + descriptor_write.dstBinding = 0; + descriptor_write.descriptorCount = 1; + descriptor_write.pTexelBufferView = nullptr; + descriptor_write.pBufferInfo = &buff_info; + descriptor_write.pImageInfo = nullptr; + descriptor_write.descriptorType = VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER; + descriptor_write.dstSet = ds.set_; + vkUpdateDescriptorSets(m_device->device(), 1, &descriptor_write, 0, NULL); + // + buff_info.buffer = storage_buffer.handle(); + descriptor_write.dstBinding = 1; + descriptor_write.descriptorType = VK_DESCRIPTOR_TYPE_STORAGE_BUFFER; + vkUpdateDescriptorSets(m_device->device(), 1, &descriptor_write, 0, NULL); + + // Create PSO that uses the uniform buffers + char const *vsSource = + "#version 450\n" + "\n" + "void main(){\n" + " gl_Position = vec4(1);\n" + "}\n"; + char const *fsSource = + "#version 450\n" + "\n" + "layout(location=0) out vec4 color;\n" + "layout(set=0, binding=0) uniform block { vec4 read; };\n" + "layout(set=0, binding=1) buffer block { vec4 write; };\n" + "void main(){\n" + " write = read;\n" + " color = vec4(1);" + "}\n"; + VkShaderObj vs(m_device, vsSource, VK_SHADER_STAGE_VERTEX_BIT, this); + VkShaderObj fs(m_device, fsSource, VK_SHADER_STAGE_FRAGMENT_BIT, this); + + VkPipelineObj pipe(m_device); + pipe.AddShader(&vs); + pipe.AddShader(&fs); + pipe.AddColorAttachment(); + + err = pipe.CreateVKPipeline(pipeline_layout, renderPass()); + ASSERT_VK_SUCCESS(err); + vkCmdBeginRenderPass(cmd_bufs[2], &m_renderPassBeginInfo, VK_SUBPASS_CONTENTS_INLINE); + + vkCmdBindPipeline(cmd_bufs[2], VK_PIPELINE_BIND_POINT_GRAPHICS, pipe.handle()); + vkCmdBindDescriptorSets(cmd_bufs[2], VK_PIPELINE_BIND_POINT_GRAPHICS, pipeline_layout, 0, 1, &ds.set_, 0, nullptr); + + VkViewport viewport = {0, 0, 16, 16, 0, 1}; + vkCmdSetViewport(cmd_bufs[2], 0, 1, &viewport); + VkRect2D scissor = {{0, 0}, {16, 16}}; + vkCmdSetScissor(cmd_bufs[2], 0, 1, &scissor); + // Should now trigger RaW and WaR errors at Draw time + // m_errorMonitor->SetDesiredFailureMsg(VK_DEBUG_REPORT_WARNING_BIT_EXT, " causes a read after write conflict with "); + // m_errorMonitor->SetDesiredFailureMsg(VK_DEBUG_REPORT_WARNING_BIT_EXT, " causes a write after read conflict with "); + vkCmdDraw(cmd_bufs[2], 3, 1, 0, 0); + // m_errorMonitor->VerifyFound(); + vkCmdEndRenderPass(cmd_bufs[2]); + vkEndCommandBuffer(cmd_bufs[2]); + + // Submit command buffers and verify no error + VkSubmitInfo submit_info = {}; + submit_info.sType = VK_STRUCTURE_TYPE_SUBMIT_INFO; + submit_info.commandBufferCount = 3; + submit_info.pCommandBuffers = cmd_bufs; + vkQueueSubmit(m_device->m_queue, 1, &submit_info, VK_NULL_HANDLE); + vkQueueWaitIdle(m_device->m_queue); + m_errorMonitor->VerifyNotFound(); + vkDestroyPipelineLayout(m_device->device(), pipeline_layout, nullptr); +} + TEST_F(VkPositiveLayerTest, UpdateBufferRaWDependencyWithBarrier) { TEST_DESCRIPTION("Update buffer used in CmdDrawIndirect w/ barrier before use"); From bf652c6d2d71176632427e501599c47cd32b9721 Mon Sep 17 00:00:00 2001 From: Tobin Ehlis Date: Fri, 25 Aug 2017 16:30:38 -0600 Subject: [PATCH 27/28] layers:Fix some win build warnings Mis-match between size_t and uint32_t was causing some Windows compiler warnings. Use size_t uniformly where vector size() function involved. --- layers/core_validation.cpp | 11 +++++------ layers/core_validation_types.h | 10 +++++----- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/layers/core_validation.cpp b/layers/core_validation.cpp index 576b7b4aa7..81a4915974 100644 --- a/layers/core_validation.cpp +++ b/layers/core_validation.cpp @@ -2624,7 +2624,7 @@ static void PostCallRecordQueueSubmit(layer_data *dev_data, VkQueue queue, uint3 } // Prototype -void ReplayMemoryAccessCommands(layer_data *, std::vector> *, uint32_t, uint32_t, +void ReplayMemoryAccessCommands(layer_data *, std::vector> *, size_t, size_t, std::unordered_map> *, std::vector *); static bool PreCallValidateQueueSubmit(layer_data *dev_data, VkQueue queue, uint32_t submitCount, const VkSubmitInfo *pSubmits, @@ -2646,7 +2646,7 @@ static bool PreCallValidateQueueSubmit(layer_data *dev_data, VkQueue queue, uint // We have to copy the commands into local vector b/c we can't modify in the CBs themselves std::vector> submit_cmds; std::vector replay_command_buffers; - uint32_t start_replay_index = 0, end_replay_index = 0; // Indices that set bounds for replaying synch cmds + size_t start_replay_index = 0, end_replay_index = 0; // Indices that set bounds for replaying synch cmds std::vector cb_live_accesses; // Store up mem accesses per CB that are still live (not visible) for (uint32_t submit_idx = 0; submit_idx < submitCount; submit_idx++) { const VkSubmitInfo *submit = &pSubmits[submit_idx]; @@ -7069,14 +7069,13 @@ static void ReplaySynchCommand(layer_data *device_data, SynchCommand *synch_cmd, // vector. // This will include the previously conflicting late access, so the returned vector of live accesses can be checked against updated // access_map. -void ReplayMemoryAccessCommands(layer_data *device_data, std::vector> *commands, - uint32_t start_synch_index, uint32_t end_synch_index, - std::unordered_map> *access_map, +void ReplayMemoryAccessCommands(layer_data *device_data, std::vector> *commands, size_t start_synch_index, + size_t end_synch_index, std::unordered_map> *access_map, std::vector *remaining_accesses) { if (commands->empty()) { return; // early out } - auto index = 0; + size_t index = 0; // Store running list of synch commands for merge purposes std::vector prev_synch_commands; Command *cmd_ptr = nullptr; diff --git a/layers/core_validation_types.h b/layers/core_validation_types.h index 438bafe5ee..c532a87d39 100644 --- a/layers/core_validation_types.h +++ b/layers/core_validation_types.h @@ -646,14 +646,14 @@ struct MemoryAccess { class Command { public: - Command(CMD_TYPE type, uint32_t seq, GLOBAL_CB_NODE *gcb) : type(type), seq(seq), cb_state(gcb), replay(false), synch(false){}; - Command(CMD_TYPE type, uint32_t seq, GLOBAL_CB_NODE *gcb, bool synch) + Command(CMD_TYPE type, size_t seq, GLOBAL_CB_NODE *gcb) : type(type), seq(seq), cb_state(gcb), replay(false), synch(false){}; + Command(CMD_TYPE type, size_t seq, GLOBAL_CB_NODE *gcb, bool synch) : type(type), seq(seq), cb_state(gcb), replay(false), synch(synch){}; virtual ~Command() {} void AddMemoryAccess(MemoryAccess access) { mem_accesses.push_back(access); }; - void SetSeq(uint32_t seq_num) { seq = seq_num; }; + void SetSeq(size_t seq_num) { seq = seq_num; }; CMD_TYPE type; - uint32_t seq; // seq # of cmd in this cmd buffer + size_t seq; // seq # of cmd in this cmd buffer GLOBAL_CB_NODE *cb_state; bool replay; // Track if cmd has been replayed during QueueSubmit bool synch; @@ -662,7 +662,7 @@ class Command { class SynchCommand : public Command { public: - SynchCommand(CMD_TYPE type, uint32_t seq, GLOBAL_CB_NODE *gcb, VkPipelineStageFlags src_stage_flags, + SynchCommand(CMD_TYPE type, size_t seq, GLOBAL_CB_NODE *gcb, VkPipelineStageFlags src_stage_flags, VkPipelineStageFlags dst_stage_flags) : Command(type, seq, gcb, true), src_stage_flags(src_stage_flags), dst_stage_flags(dst_stage_flags){}; VkPipelineStageFlags src_stage_flags; From f847f3152ffdbb6d4c8a636ecc7e0d7a2908e26c Mon Sep 17 00:00:00 2001 From: Tobin Ehlis Date: Mon, 28 Aug 2017 15:50:34 -0600 Subject: [PATCH 28/28] layers:Clear cmd vector on every replay Anytime we need to replay cmds during queue submit, we should start with an empty replay cmd vector, since replay always starts from the beginning of a submission. --- layers/core_validation.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/layers/core_validation.cpp b/layers/core_validation.cpp index 81a4915974..de7be1acda 100644 --- a/layers/core_validation.cpp +++ b/layers/core_validation.cpp @@ -2731,6 +2731,7 @@ static bool PreCallValidateQueueSubmit(layer_data *dev_data, VkQueue queue, uint } } else { // We have a pre-check conflict so have to replay Cmds to verify if conflict is real or cleared by a // inter-CB synch + submit_cmds.clear(); // Should always start replay with an empty cmd vector // This is the slow path // We have to replay so copy cmd sequence up to this point into local vector // Then mark seq replay start & seq replay end indices which are seq cmds between conflicting mem accesses that