From 8da0d32769f16ee5151ee45772e4887e73416ca6 Mon Sep 17 00:00:00 2001 From: Tobin Ehlis Date: Thu, 14 Jan 2016 12:47:19 -0700 Subject: [PATCH] layers: MR137, Flag error if set updated or freed while in use It is illegal to free or update a descriptorSet that is in use. Updated SET_NODE to inherit from BASE_NODE for in_use tracking. At the time that Draws are recorded into the cmdBuffer, capture any active sets for that cmdBuffer into std::set activeSets. At the time vkCmdBindDescriptorSets is recoreded in cmdBuffer, flag descriptor sets as in use. At the time a set is freed, flag an error if set is in use. --- layers/draw_state.cpp | 69 +++++++++++++++++++++++++++++++---- layers/draw_state.h | 15 ++++++-- layers/vk_validation_layer_details.md | 3 +- 3 files changed, 74 insertions(+), 13 deletions(-) diff --git a/layers/draw_state.cpp b/layers/draw_state.cpp index 41cf1b4..416995d 100644 --- a/layers/draw_state.cpp +++ b/layers/draw_state.cpp @@ -1478,6 +1478,8 @@ static VkBool32 validate_draw_state(layer_data* my_data, GLOBAL_CB_NODE* pCB, Vk "VkDescriptorSet (%#" PRIxLEAST64 ") bound as set #%u is not compatible with overlapping VkPipelineLayout %#" PRIxLEAST64 " due to: %s", (uint64_t)setHandle, setIndex, (uint64_t)pPipe->graphicsPipelineCI.layout, errorString.c_str()); } else { // Valid set is bound and layout compatible, validate that it's updated and verify any dynamic offsets + // Add this set as a valid set for this CB + pCB->activeSets.insert(pCB->boundDescriptorSets[setIndex]); // Pull the set node SET_NODE* pSet = getSetNode(my_data, pCB->boundDescriptorSets[setIndex]); // Make sure set has been updated @@ -2190,7 +2192,24 @@ static VkBool32 validateUpdateContents(const layer_data* my_data, const VkWriteD } return skipCall; } - +// Validate that given set is valid and that it's not being used by an in-flight CmdBuffer +// func_str is the name of the calling function +// Return VK_FALSE if no errors occur +// Return VK_TRUE if validation error occurs and callback returns VK_TRUE (to skip upcoming API call down the chain) +VkBool32 validateIdleDescriptorSet(const layer_data* my_data, VkDescriptorSet set, std::string func_str) { + VkBool32 skip_call = VK_FALSE; + auto set_node = my_data->setMap.find(set); + if (set_node == my_data->setMap.end()) { + skip_call |= log_msg(my_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_DESCRIPTOR_SET_EXT, reinterpret_cast(set), __LINE__, DRAWSTATE_DOUBLE_DESTROY, "DS", + "Cannot call %s() on descriptor set %" PRIxLEAST64 " that has not been allocated.", func_str.c_str(), reinterpret_cast(set)); + } else { + if (set_node->second->in_use.load()) { + skip_call |= log_msg(my_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_DESCRIPTOR_SET_EXT, reinterpret_cast(set), __LINE__, DRAWSTATE_OBJECT_INUSE, "DS", + "Cannot call %s() on descriptor set %" PRIxLEAST64 " that is in use by a command buffer.", func_str.c_str(), reinterpret_cast(set)); + } + } + return skip_call; +} // update DS mappings based on write and copy update arrays static VkBool32 dsUpdate(layer_data* my_data, VkDevice device, uint32_t descriptorWriteCount, const VkWriteDescriptorSet* pWDS, uint32_t descriptorCopyCount, const VkCopyDescriptorSet* pCDS) { @@ -2204,6 +2223,9 @@ static VkBool32 dsUpdate(layer_data* my_data, VkDevice device, uint32_t descript for (i=0; i < descriptorWriteCount; i++) { VkDescriptorSet ds = pWDS[i].dstSet; SET_NODE* pSet = my_data->setMap[ds]; + // Set being updated cannot be in-flight + if ((skipCall = validateIdleDescriptorSet(my_data, ds, "VkUpdateDescriptorSets")) == VK_TRUE) + return skipCall; GENERIC_HEADER* pUpdate = (GENERIC_HEADER*) &pWDS[i]; pLayout = pSet->pLayout; // First verify valid update struct @@ -2214,7 +2236,7 @@ static VkBool32 dsUpdate(layer_data* my_data, VkDevice device, uint32_t descript binding = pWDS[i].dstBinding; // Make sure that layout being updated has the binding being updated if (pLayout->bindings.find(binding) == pLayout->bindings.end()) { - skipCall |= log_msg(my_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_DESCRIPTOR_SET_EXT, (uint64_t) ds, __LINE__, DRAWSTATE_INVALID_UPDATE_INDEX, "DS", + skipCall |= log_msg(my_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_DESCRIPTOR_SET_EXT, reinterpret_cast(ds), __LINE__, DRAWSTATE_INVALID_UPDATE_INDEX, "DS", "Descriptor Set %" PRIu64 " does not have binding to match update binding %u for update type %s!", reinterpret_cast(ds), binding, string_VkStructureType(pUpdate->sType)); } else { // Next verify that update falls within size of given binding @@ -2261,6 +2283,9 @@ static VkBool32 dsUpdate(layer_data* my_data, VkDevice device, uint32_t descript // For each copy make sure that update falls within given layout and that types match pSrcSet = my_data->setMap[pCDS[i].srcSet]; pDstSet = my_data->setMap[pCDS[i].dstSet]; + // Set being updated cannot be in-flight + if ((skipCall = validateIdleDescriptorSet(my_data, pDstSet->set, "VkUpdateDescriptorSets")) == VK_TRUE) + return skipCall; pSrcLayout = pSrcSet->pLayout; pDstLayout = pDstSet->pLayout; // Validate that src binding is valid for src set layout @@ -2571,6 +2596,7 @@ static void resetCB(layer_data* my_data, const VkCommandBuffer cb) pCB->lastBoundPipelineLayout = 0; pCB->activeRenderPass = 0; pCB->activeSubpass = 0; + pCB->activeSets.clear(); pCB->framebuffer = 0; pCB->boundDescriptorSets.clear(); pCB->drawData.clear(); @@ -3061,6 +3087,24 @@ VkBool32 ValidateCmdBufImageLayouts(VkCommandBuffer cmdBuffer) { } return skip_call; } +// Descriptor sets are unique vs. other resources in that they may be consumed at bind time: +// From spec section on vkCmdBindDescriptorSets - The descriptor set contents bound by a call +// to vkCmdBindDescriptorSets may be consumed during host execution of the command, or during +// shader execution of the resulting draws, or any time in between. +VkBool32 validateAndIncrementDescriptorSets(layer_data* my_data, GLOBAL_CB_NODE* pCB) { + VkBool32 skip_call = VK_FALSE; + // Verify that any active sets for this CB are valid and mark them in use + for (auto set : pCB->activeSets) { + auto setNode = my_data->setMap.find(set); + if (setNode == my_data->setMap.end()) { + skip_call |= log_msg(my_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_DESCRIPTOR_SET_EXT, reinterpret_cast(set), __LINE__, DRAWSTATE_INVALID_DESCRIPTOR_SET, "DS", + "Cannot submit cmd buffer using deleted descriptor set %" PRIu64 ".", reinterpret_cast(set)); + } else { + setNode->second->in_use.fetch_add(1); + } + } + return skip_call; +} VkBool32 validateAndIncrementResources(layer_data* my_data, GLOBAL_CB_NODE* pCB) { VkBool32 skip_call = VK_FALSE; @@ -3068,7 +3112,7 @@ VkBool32 validateAndIncrementResources(layer_data* my_data, GLOBAL_CB_NODE* pCB) for (auto buffer : drawDataElement.buffers) { auto buffer_data = my_data->bufferMap.find(buffer); if (buffer_data == my_data->bufferMap.end()) { - skip_call |= log_msg(my_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, 0, 0, DRAWSTATE_INVALID_BUFFER, "DS", + skip_call |= log_msg(my_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_BUFFER_EXT, reinterpret_cast(buffer), __LINE__, DRAWSTATE_INVALID_BUFFER, "DS", "Cannot submit cmd buffer using deleted buffer %" PRIu64 ".", reinterpret_cast(buffer)); } else { buffer_data->second.in_use.fetch_add(1); @@ -3088,6 +3132,12 @@ void decrementResources(layer_data* my_data, VkCommandBuffer cmdBuffer) { } } } + for (auto set : pCB->activeSets) { + auto setNode = my_data->setMap.find(set); + if (setNode != my_data->setMap.end()) { + setNode->second->in_use.fetch_sub(1); + } + } for (auto queryStatePair : pCB->queryToStateMap) { my_data->queryToStateMap[queryStatePair.first] = queryStatePair.second; } @@ -3380,13 +3430,12 @@ VkBool32 validateIdleBuffer(const layer_data* my_data, VkBuffer buffer) { VkBool32 skip_call = VK_FALSE; auto buffer_data = my_data->bufferMap.find(buffer); if (buffer_data == my_data->bufferMap.end()) { - skip_call |= log_msg(my_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, 0, __LINE__, DRAWSTATE_DOUBLE_DESTROY, "DS", + skip_call |= log_msg(my_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_BUFFER_EXT, reinterpret_cast(buffer), __LINE__, DRAWSTATE_DOUBLE_DESTROY, "DS", "Cannot free buffer %" PRIxLEAST64 " that has not been allocated.", reinterpret_cast(buffer)); } else { if (buffer_data->second.in_use.load()) { - skip_call |= log_msg(my_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, 0, __LINE__, DRAWSTATE_OBJECT_INUSE, "DS", + skip_call |= log_msg(my_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_BUFFER_EXT, reinterpret_cast(buffer), __LINE__, DRAWSTATE_OBJECT_INUSE, "DS", "Cannot free buffer %" PRIxLEAST64 " that is in use by a command buffer.", reinterpret_cast(buffer)); - } } return skip_call; @@ -3492,7 +3541,7 @@ VK_LAYER_EXPORT VKAPI_ATTR VkResult VKAPI_CALL vkCreateCommandPool(VkDevice devi } return result; } - +// Destroy commandPool along with all of the commandBuffers allocated from that pool VK_LAYER_EXPORT VKAPI_ATTR void VKAPI_CALL vkDestroyCommandPool(VkDevice device, VkCommandPool commandPool, const VkAllocationCallbacks* pAllocator) { layer_data* dev_data = get_my_data_ptr(get_dispatch_key(device), layer_data_map); @@ -3907,12 +3956,12 @@ VK_LAYER_EXPORT VKAPI_ATTR VkResult VKAPI_CALL vkAllocateDescriptorSets(VkDevice "Out of memory while attempting to allocate SET_NODE in vkAllocateDescriptorSets()")) return VK_ERROR_VALIDATION_FAILED_EXT; } else { - memset(pNewNode, 0, sizeof(SET_NODE)); // TODO : Pool should store a total count of each type of Descriptor available // When descriptors are allocated, decrement the count and validate here // that the count doesn't go below 0. One reset/free need to bump count back up. // Insert set at head of Set LL for this pool pNewNode->pNext = pPoolNode->pSets; + pNewNode->in_use.store(0); pPoolNode->pSets = pNewNode; LAYOUT_NODE* pLayout = getLayoutNode(dev_data, pAllocateInfo->pSetLayouts[i]); if (NULL == pLayout) { @@ -3941,6 +3990,9 @@ VK_LAYER_EXPORT VKAPI_ATTR VkResult VKAPI_CALL vkFreeDescriptorSets(VkDevice dev { VkBool32 skipCall = VK_FALSE; layer_data* dev_data = get_my_data_ptr(get_dispatch_key(device), layer_data_map); + // Make sure that no sets being destroyed are in-flight + for (uint32_t i=0; icreateInfo.flags)) { // Can't Free from a NON_FREE pool @@ -4427,6 +4479,7 @@ VK_LAYER_EXPORT VKAPI_ATTR void VKAPI_CALL vkCmdBindDescriptorSets(VkCommandBuff skipCall |= log_msg(dev_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, (uint64_t) commandBuffer, __LINE__, DRAWSTATE_INVALID_DYNAMIC_OFFSET_COUNT, "DS", "Attempting to bind %u descriptorSets with %u dynamic descriptors, but dynamicOffsetCount is %u. It should exactly match the number of dynamic descriptors.", setCount, totalDynamicDescriptors, dynamicOffsetCount); } + validateAndIncrementDescriptorSets(dev_data, pCB); } } else { skipCall |= report_error_no_cb_begin(dev_data, commandBuffer, "vkCmdBindDescriptorSets()"); diff --git a/layers/draw_state.h b/layers/draw_state.h index cbd9a08..1ab4228 100755 --- a/layers/draw_state.h +++ b/layers/draw_state.h @@ -54,6 +54,7 @@ typedef enum _DRAW_STATE_ERROR DRAWSTATE_VTX_INDEX_ALIGNMENT_ERROR, // binding offset in vkCmdBindIndexBuffer() out of alignment based on indexType //DRAWSTATE_MISSING_DOT_PROGRAM, // No "dot" program in order to generate png image DRAWSTATE_OUT_OF_MEMORY, // malloc failed + DRAWSTATE_INVALID_DESCRIPTOR_SET, // Descriptor Set handle is unknown DRAWSTATE_DESCRIPTOR_TYPE_MISMATCH, // Type in layout vs. update are not the same DRAWSTATE_DESCRIPTOR_STAGEFLAGS_MISMATCH, // StageFlags in layout are not the same throughout a single VkWriteDescriptorSet update DRAWSTATE_DESCRIPTOR_UPDATE_OUT_OF_BOUNDS, // Descriptors set for update out of bounds for corresponding layout section @@ -97,7 +98,7 @@ typedef enum _DRAW_STATE_ERROR DRAWSTATE_BUFFERINFO_DESCRIPTOR_ERROR, // A Descriptor of *_[UNIFORM|STORAGE]_BUFFER_[DYNAMIC] type is being updated with an invalid or bad BufferView DRAWSTATE_DYNAMIC_OFFSET_OVERFLOW, // At draw time the dynamic offset combined with buffer offset and range oversteps size of buffer DRAWSTATE_DOUBLE_DESTROY, // Destroying an object twice - DRAWSTATE_OBJECT_INUSE, // Destroying an object in use by a command buffer + DRAWSTATE_OBJECT_INUSE, // Destroying or modifying an object in use by a command buffer DRAWSTATE_QUEUE_FORWARD_PROGRESS, // Queue cannot guarantee forward progress DRAWSTATE_INVALID_UNIFORM_BUFFER_OFFSET, // Dynamic Uniform Buffer Offsets violate device limit DRAWSTATE_INVALID_STORAGE_BUFFER_OFFSET, // Dynamic Storage Buffer Offsets violate device limit @@ -303,7 +304,9 @@ struct PIPELINE_LAYOUT_NODE { vector pushConstantRanges; }; -typedef struct _SET_NODE { +class SET_NODE : public BASE_NODE { + public: + using BASE_NODE::in_use; VkDescriptorSet set; VkDescriptorPool pool; // Head of LL of all Update structs for this set @@ -312,9 +315,10 @@ typedef struct _SET_NODE { uint32_t descriptorCount; GENERIC_HEADER** ppDescriptors; // Array where each index points to update node for its slot LAYOUT_NODE* pLayout; // Layout for this set - struct _SET_NODE* pNext; + struct SET_NODE* pNext; vector dynamicOffsets; // one dynamic offset per dynamic descriptor -} SET_NODE; + SET_NODE() : pUpdateStructs(NULL), ppDescriptors(NULL), pLayout(NULL), pNext(NULL) {}; +}; typedef struct _DESCRIPTOR_POOL_NODE { VkDescriptorPool pool; @@ -510,6 +514,9 @@ typedef struct _GLOBAL_CB_NODE { VkSubpassContents activeSubpassContents; uint32_t activeSubpass; VkFramebuffer framebuffer; + // Capture which sets are actually used by the shaders of this CB. This is union of all sets used by each Draw in CB + std::set activeSets; + // Keep running track of which sets are bound to which set# at any given time vector boundDescriptorSets; // Index is set# that given set is bound to vector waitedEvents; unordered_map > waitedEventsBeforeQueryReset; diff --git a/layers/vk_validation_layer_details.md b/layers/vk_validation_layer_details.md index 7cde37d..d2dcbd2 100644 --- a/layers/vk_validation_layer_details.md +++ b/layers/vk_validation_layer_details.md @@ -30,6 +30,7 @@ The VK_LAYER_LUNARG_draw_state layer tracks state leading into Draw cmds. This i | Cmd Buffer Begin | Check that BeginCommandBuffer was called for this command buffer when binding commands or calling end | NO_BEGIN_COMMAND_BUFFER | vkEndCommandBuffer vkCmdBindPipeline vkCmdSetViewport vkCmdSetLineWidth vkCmdSetDepthBias vkCmdSetBlendConstants vkCmdSetDepthBounds vkCmdSetStencilCompareMask vkCmdSetStencilWriteMask vkCmdSetStencilReference vkCmdBindDescriptorSets vkCmdBindIndexBuffer vkCmdBindVertexBuffers vkCmdDraw vkCmdDrawIndexed vkCmdDrawIndirect vkCmdDrawIndexedIndirect vkCmdDispatch vkCmdDispatchIndirect vkCmdCopyBuffer vkCmdCopyImage vkCmdBlitImage vkCmdCopyBufferToImage vkCmdCopyImageToBuffer vkCmdUpdateBuffer vkCmdFillBuffer vkCmdClearAttachments vkCmdClearColorImage vkCmdClearDepthStencilImage vkCmdResolveImage vkCmdSetEvent vkCmdResetEvent vkCmdWaitEvents vkCmdPipelineBarrier vkCmdBeginQuery vkCmdEndQuery vkCmdResetQueryPool vkCmdWriteTimestamp | NoBeginCommandBuffer | NA | | Cmd Buffer Submit Count | Verify that ONE_TIME submit cmdbuffer is not submitted multiple times | COMMAND_BUFFER_SINGLE_SUBMIT_VIOLATION | vkBeginCommandBuffer, vkQueueSubmit | CommandBufferTwoSubmits | NA | | Valid Secondary CommandBuffer | Validates that no primary command buffers are sent to vkCmdExecuteCommands() are | INVALID_SECONDARY_COMMAND_BUFFER | vkCmdExecuteCommands | ExecuteCommandsPrimaryCB | NA | +| Invalid Descriptor Set | Invalid Descriptor Set used. Either never created or already destroyed. | INVALID_DESCRIPTOR_SET | vkQueueSubmit | TODO | Create Test | | Descriptor Type | Verify Descriptor type in bound descriptor set layout matches descriptor type specified in update. This also includes mismatches in the TYPES of copied descriptors. | DESCRIPTOR_TYPE_MISMATCH | vkUpdateDescriptorSets | DSTypeMismatch CopyDescriptorUpdateErrors | NA | | Descriptor StageFlags | Verify all descriptors within a single write update have the same stageFlags | DESCRIPTOR_STAGEFLAGS_MISMATCH | vkUpdateDescriptorSets | NONE | Test this case | | DS Update Size | DS update out of bounds for given layout section. | DESCRIPTOR_UPDATE_OUT_OF_BOUNDS | vkUpdateDescriptorSets | DSUpdateOutOfBounds CopyDescriptorUpdateErrors | NA | @@ -68,7 +69,7 @@ The VK_LAYER_LUNARG_draw_state layer tracks state leading into Draw cmds. This i | Verify Memory Access Flags/Memory Barriers | Validate correct access flags for memory barriers | INVALID_BARRIER | vkCmdWaitEvents vkCmdPipelineBarrier | TBD | None | | Verify Memory Buffer Not Deleted | Validate Command Buffer not submitted with deleted memory buffer | INVALID_BUFFER | vkQueueSubmit | TBD | None | | Verify Memory Buffer Destroy | Validate memory buffers are not destroyed more than once | DOUBLE_DESTROY | vkDestroyBuffer | TBD | None | -| Verify Memory Buffer Not In Use | Validate that memory buffer being freed is not in use | OBJECT_INUSE | vkDestroyBuffer | TBD | None | +| Verify Object Not In Use | Validate that object being freed or modified is not in use | OBJECT_INUSE | vkDestroyBuffer vkFreeDescriptorSets vkUpdateDescriptorSets | TBD | None | | Verify Get Queries| Validate that that queries are properly initialized and synchronized | INVALID_QUERY | vkGetFenceStatus vkQueueWaitIdle vkWaitForFences vkDeviceWaitIdle | TBD | None | | Live Semaphore | When waiting on a semaphore, need to make sure that the semaphore is live and therefore can be signalled, otherwise queue is stalled and cannot make forward progress. | QUEUE_FORWARD_PROGRESS | vkQueueSubmit vkQueueBindSparse vkQueuePresentKHR vkAcquireNextImageKHR | TODO | Create test | | Storage Buffer Alignment | Storage Buffer offsets in BindDescriptorSets must agree with offset alignment device limit | INVALID_STORAGE_BUFFER_OFFSET | vkCmdBindDescriptorSets | TODO | Create test | -- 2.7.4