layers: Unify invalid command buffer handling
Created a single vector to track any cmd buffer dependencies that are destroyed
and thereby cause the cmd buffer to become invalid. Currently just tracking
the previous object types of descriptor sets and framebuffers, but will update
the tracking to include all cmd buffer dependencies in future commits.
diff --git a/layers/core_validation.cpp b/layers/core_validation.cpp
index 6100e00..e8d8e74 100644
--- a/layers/core_validation.cpp
+++ b/layers/core_validation.cpp
@@ -607,6 +607,10 @@
return "buffer";
case VK_DEBUG_REPORT_OBJECT_TYPE_SWAPCHAIN_KHR_EXT:
return "swapchain";
+ case VK_DEBUG_REPORT_OBJECT_TYPE_DESCRIPTOR_SET_EXT:
+ return "descriptor set";
+ case VK_DEBUG_REPORT_OBJECT_TYPE_FRAMEBUFFER_EXT:
+ return "buffer";
default:
return "unknown";
}
@@ -3746,9 +3750,7 @@
pCB->activeRenderPass = nullptr;
pCB->activeSubpassContents = VK_SUBPASS_CONTENTS_INLINE;
pCB->activeSubpass = 0;
- pCB->destroyedSets.clear();
- pCB->updatedSets.clear();
- pCB->destroyedFramebuffers.clear();
+ pCB->broken_bindings.clear();
pCB->waitedEvents.clear();
pCB->events.clear();
pCB->writeEventsBeforeWait.clear();
@@ -4438,58 +4440,18 @@
if (CB_RECORDED != pCB->state) {
if (CB_INVALID == pCB->state) {
// Inform app of reason CB invalid
- bool causeReported = false;
- if (!pCB->destroyedSets.empty()) {
- std::stringstream set_string;
- for (auto set : pCB->destroyedSets)
- set_string << " " << set;
-
- skipCall |=
- log_msg(dev_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT,
- (uint64_t)(pCB->commandBuffer), __LINE__, DRAWSTATE_INVALID_COMMAND_BUFFER, "DS",
- "You are submitting command buffer 0x%" PRIxLEAST64
- " that is invalid because it had the following bound descriptor set(s) destroyed: %s",
- (uint64_t)(pCB->commandBuffer), set_string.str().c_str());
- causeReported = true;
- }
- if (!pCB->updatedSets.empty()) {
- std::stringstream set_string;
- for (auto set : pCB->updatedSets)
- set_string << " " << set;
-
- skipCall |=
- log_msg(dev_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT,
- (uint64_t)(pCB->commandBuffer), __LINE__, DRAWSTATE_INVALID_COMMAND_BUFFER, "DS",
- "You are submitting command buffer 0x%" PRIxLEAST64
- " that is invalid because it had the following bound descriptor set(s) updated: %s",
- (uint64_t)(pCB->commandBuffer), set_string.str().c_str());
- causeReported = true;
- }
- if (!pCB->destroyedFramebuffers.empty()) {
- std::stringstream fb_string;
- for (auto fb : pCB->destroyedFramebuffers)
- fb_string << " " << fb;
+ for (auto obj : pCB->broken_bindings) {
+ const char *type_str = object_type_to_string(obj.type);
+ // Descriptor sets are a special case that can be either destroyed or updated to invalidated a CB
+ const char *cause_str =
+ (obj.type == VK_DEBUG_REPORT_OBJECT_TYPE_DESCRIPTOR_SET_EXT) ? "destroyed or updated" : "destroyed";
skipCall |=
log_msg(dev_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT,
reinterpret_cast<uint64_t &>(pCB->commandBuffer), __LINE__, DRAWSTATE_INVALID_COMMAND_BUFFER, "DS",
- "You are submitting command buffer 0x%" PRIxLEAST64 " that is invalid because it had the following "
- "referenced framebuffers destroyed: %s",
- reinterpret_cast<uint64_t &>(pCB->commandBuffer), fb_string.str().c_str());
- causeReported = true;
- }
- // TODO : This is defensive programming to make sure an error is
- // flagged if we hit this INVALID cmd buffer case and none of the
- // above cases are hit. As the number of INVALID cases grows, this
- // code should be updated to seemlessly handle all the cases.
- if (!causeReported) {
- skipCall |= log_msg(
- dev_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT,
- reinterpret_cast<uint64_t &>(pCB->commandBuffer), __LINE__, DRAWSTATE_INVALID_COMMAND_BUFFER, "DS",
- "You are submitting command buffer 0x%" PRIxLEAST64 " that is invalid due to an unknown cause. Validation "
- "should "
- "be improved to report the exact cause.",
- reinterpret_cast<uint64_t &>(pCB->commandBuffer));
+ "You are submitting command buffer 0x%" PRIxLEAST64 " that is invalid because bound %s 0x%" PRIxLEAST64
+ " was %s.",
+ reinterpret_cast<uint64_t &>(pCB->commandBuffer), type_str, obj.handle, cause_str);
}
} else { // Flag error for using CB w/o vkEndCommandBuffer() called
skipCall |=
@@ -5523,17 +5485,22 @@
return result;
}
+// For given cb_nodes, invalidate them and track object causing invalidation
+void invalidateCommandBuffers(std::unordered_set<GLOBAL_CB_NODE *> cb_nodes, VK_OBJECT obj) {
+ for (auto cb_node : cb_nodes) {
+ cb_node->state = CB_INVALID;
+ cb_node->broken_bindings.push_back(obj);
+ }
+}
+
VKAPI_ATTR void VKAPI_CALL
DestroyFramebuffer(VkDevice device, VkFramebuffer framebuffer, const VkAllocationCallbacks *pAllocator) {
layer_data *dev_data = get_my_data_ptr(get_dispatch_key(device), layer_data_map);
std::unique_lock<std::mutex> lock(global_lock);
auto fb_node = getFramebuffer(dev_data, framebuffer);
if (fb_node) {
- for (auto cb_node : fb_node->cb_bindings) {
- // Set CB as invalid and record destroyed framebuffer
- cb_node->state = CB_INVALID;
- cb_node->destroyedFramebuffers.insert(framebuffer);
- }
+ invalidateCommandBuffers(fb_node->cb_bindings,
+ {reinterpret_cast<uint64_t &>(fb_node->framebuffer), VK_DEBUG_REPORT_OBJECT_TYPE_FRAMEBUFFER_EXT});
dev_data->frameBufferMap.erase(fb_node->framebuffer);
}
lock.unlock();
diff --git a/layers/core_validation_types.h b/layers/core_validation_types.h
index 18c8675..036ed1f 100644
--- a/layers/core_validation_types.h
+++ b/layers/core_validation_types.h
@@ -71,6 +71,12 @@
std::unordered_set<GLOBAL_CB_NODE *> cb_bindings;
};
+// Generic wrapper for vulkan objects
+struct VK_OBJECT {
+ uint64_t handle;
+ VkDebugReportObjectTypeEXT type;
+};
+
struct DESCRIPTOR_POOL_NODE {
VkDescriptorPool pool;
uint32_t maxSets; // Max descriptor sets allowed in this pool
@@ -462,13 +468,9 @@
uint32_t activeSubpass;
VkFramebuffer activeFramebuffer;
std::unordered_set<VkFramebuffer> framebuffers;
- // Track descriptor sets that are destroyed or updated while bound to CB
- // TODO : These data structures relate to tracking resources that invalidate
- // a cmd buffer that references them. Need to unify how we handle these
- // cases so we don't have different tracking data for each type.
- std::unordered_set<cvdescriptorset::DescriptorSet *> destroyedSets;
- std::unordered_set<cvdescriptorset::DescriptorSet *> updatedSets;
- std::unordered_set<VkFramebuffer> destroyedFramebuffers;
+ // Unified data struct to track dependencies that have been broken
+ // These are either destroyed objects, or updated descriptor sets
+ std::vector<VK_OBJECT> broken_bindings;
std::unordered_set<VkEvent> waitedEvents;
std::vector<VkEvent> writeEventsBeforeWait;
std::vector<VkEvent> events;
@@ -519,6 +521,7 @@
VkImageViewCreateInfo *getImageViewData(const layer_data *, VkImageView);
VkSwapchainKHR getSwapchainFromImage(const layer_data *, VkImage);
SWAPCHAIN_NODE *getSwapchainNode(const layer_data *, VkSwapchainKHR);
+void invalidateCommandBuffers(std::unordered_set<GLOBAL_CB_NODE *>, VK_OBJECT);
}
#endif // CORE_VALIDATION_TYPES_H_
diff --git a/layers/descriptor_sets.cpp b/layers/descriptor_sets.cpp
index 1f72d44..3767f78 100644
--- a/layers/descriptor_sets.cpp
+++ b/layers/descriptor_sets.cpp
@@ -321,7 +321,7 @@
cvdescriptorset::DescriptorSet::~DescriptorSet() {
InvalidateBoundCmdBuffers();
// Remove link to any cmd buffers
- for (auto cb : bound_cmd_buffers_) {
+ for (auto cb : cb_bindings) {
for (uint32_t i=0; i<VK_PIPELINE_BIND_POINT_RANGE_SIZE; ++i) {
cb->lastBound[i].uniqueBoundSets.erase(this);
}
@@ -460,9 +460,8 @@
}
// Set is being deleted or updates so invalidate all bound cmd buffers
void cvdescriptorset::DescriptorSet::InvalidateBoundCmdBuffers() {
- for (auto cb_node : bound_cmd_buffers_) {
- cb_node->state = CB_INVALID;
- }
+ core_validation::invalidateCommandBuffers(cb_bindings,
+ {reinterpret_cast<uint64_t &>(set_), VK_DEBUG_REPORT_OBJECT_TYPE_DESCRIPTOR_SET_EXT});
}
// Perform write update in given update struct
void cvdescriptorset::DescriptorSet::PerformWriteUpdate(const VkWriteDescriptorSet *update) {
diff --git a/layers/descriptor_sets.h b/layers/descriptor_sets.h
index 9282459..609f5d0 100644
--- a/layers/descriptor_sets.h
+++ b/layers/descriptor_sets.h
@@ -286,6 +286,7 @@
class DescriptorSet : public BASE_NODE {
public:
using BASE_NODE::in_use;
+ using BASE_NODE::cb_bindings;
DescriptorSet(const VkDescriptorSet, const DescriptorSetLayout *, const core_validation::layer_data *);
~DescriptorSet();
// A number of common Get* functions that return data based on layout from which this set was created
@@ -331,11 +332,11 @@
const DescriptorSetLayout *GetLayout() const { return p_layout_; };
VkDescriptorSet GetSet() const { return set_; };
// Return unordered_set of all command buffers that this set is bound to
- std::unordered_set<GLOBAL_CB_NODE *> GetBoundCmdBuffers() const { return bound_cmd_buffers_; }
+ std::unordered_set<GLOBAL_CB_NODE *> GetBoundCmdBuffers() const { return cb_bindings; }
// Bind given cmd_buffer to this descriptor set
- void BindCommandBuffer(GLOBAL_CB_NODE *cb_node) { bound_cmd_buffers_.insert(cb_node); }
- // If given cmd_buffer is in the bound_cmd_buffers_ set, remove it
- void RemoveBoundCommandBuffer(GLOBAL_CB_NODE *cb_node) { bound_cmd_buffers_.erase(cb_node); }
+ void BindCommandBuffer(GLOBAL_CB_NODE *cb_node) { cb_bindings.insert(cb_node); }
+ // If given cmd_buffer is in the cb_bindings set, remove it
+ void RemoveBoundCommandBuffer(GLOBAL_CB_NODE *cb_node) { cb_bindings.erase(cb_node); }
VkSampler const *GetImmutableSamplerPtrFromBinding(const uint32_t index) const {
return p_layout_->GetImmutableSamplerPtrFromBinding(index);
};
@@ -359,7 +360,6 @@
bool some_update_; // has any part of the set ever been updated?
VkDescriptorSet set_;
const DescriptorSetLayout *p_layout_;
- std::unordered_set<GLOBAL_CB_NODE *> bound_cmd_buffers_;
std::vector<std::unique_ptr<Descriptor>> descriptors_;
// Ptr to device data used for various data look-ups
const core_validation::layer_data *device_data_;