sync: Add QueryCopyCommand

Author: artem-lunarg

layers/sync/sync_command.cpp

@@ -272,6 +272,10 @@ bool ReplayCommands(SyncEnvironment& env, AccessContext& destination_access_cont
                 command.Apply(env, base_tag + replay_tag, access_context);
                 continue;
             }
+            case CommandType::kQueryCopy: {
+                replay_common(command_data.query_copy_commands[index], access_context, replay_tag);
+                continue;
+            }
         }
         assert(false);
     }
@@ -304,6 +308,7 @@ void CommandData::Reset() {
     acceleration_structure_copy_commands.clear();
     video_commands.clear();
     clear_attachments_commands.clear();
+    query_copy_commands.clear();
 
     buffers.clear();
     buffer_lookup.clear();
@@ -2227,4 +2232,37 @@ void ClearAttachmentsCommand::Apply(SyncEnvironment& env, ResourceUsageTag tag,
     }
 }
 
+QueryCopyCommand QueryCopyCommand::Storage::MakeCommand(const CommandData& command_data) const {
+    return {*dst_buffer, range, query_pool, handle_index};
+}
+
+QueryCopyCommand::Storage QueryCopyCommand::MakeStorage(CommandData& command_data) const {
+    command_data.AddBuffer(dst_buffer);
+    return {&dst_buffer, range, query_pool, handle_index};
+}
+
+bool QueryCopyCommand::Validate(const CommandBufferContext& cb_context, const Location& loc) const {
+    return Validate(cb_context.GetSyncEnvironment(), cb_context.GetCbAccessContext(), cb_context, kInvalidTag, loc);
+}
+
+bool QueryCopyCommand::Validate(const SyncEnvironment& env, const AccessContext& access_context,
+                                const CommandBufferContext& cb_context, ResourceUsageTag replay_tag, const Location& loc) const {
+    const auto hazard = access_context.DetectHazard(dst_buffer, SYNC_COPY_TRANSFER_WRITE, range);
+    if (!hazard.IsHazard()) {
+        return false;
+    }
+    const SyncValidator& validator = env.validator;
+    LogObjectList objlist = BaseObjectList(env, cb_context, VulkanTypedHandle(query_pool, kVulkanObjectTypeQueryPool));
+    objlist.add(dst_buffer.Handle());
+    const std::string resource_description = "dstBuffer " + validator.FormatHandle(dst_buffer.Handle());
+    const std::string error =
+        validator.error_messages_.BufferError(env, hazard, cb_context, replay_tag, loc, resource_description, range);
+    return validator.SyncError(hazard.Hazard(), objlist, loc, error);
+}
+
+void QueryCopyCommand::Apply(SyncEnvironment& env, ResourceUsageTag tag, AccessContext& access_context) const {
+    access_context.UpdateAccessState(dst_buffer, SYNC_COPY_TRANSFER_WRITE, range, ResourceUsageTagEx{tag, handle_index}, 0,
+                                     env.queue_id);
+}
+
 }  // namespace syncval

layers/sync/sync_command.h

@@ -86,6 +86,7 @@ enum class CommandType : uint32_t {
     kAccelerationStructureCopy,
     kVideo,
     kClearAttachments,
+    kQueryCopy,
 };
 
 struct BufferCopyRegion {
@@ -879,6 +880,26 @@ struct ClearAttachmentsCommand {
     void Apply(SyncEnvironment& env, ResourceUsageTag tag, AccessContext& access_context) const;
 };
 
+struct QueryCopyCommand {
+    const vvl::Buffer& dst_buffer;
+    AccessRange range;
+    VkQueryPool query_pool;
+    uint32_t handle_index = vvl::kNoIndex32;
+
+    struct Storage {
+        const vvl::Buffer* dst_buffer;
+        AccessRange range;
+        VkQueryPool query_pool;
+        uint32_t handle_index;
+        QueryCopyCommand MakeCommand(const CommandData& command_data) const;
+    };
+    Storage MakeStorage(CommandData& command_data) const;
+    bool Validate(const CommandBufferContext& cb_context, const Location& loc) const;
+    bool Validate(const SyncEnvironment& env, const AccessContext& access_context, const CommandBufferContext& cb_context,
+                  ResourceUsageTag replay_tag, const Location& loc) const;
+    void Apply(SyncEnvironment& env, ResourceUsageTag tag, AccessContext& access_context) const;
+};
+
 struct CommandRef {
     CommandType type;
     uint32_t index;
@@ -915,6 +936,7 @@ struct CommandData {
     std::vector<AccelerationStructureCopyCommand::Storage> acceleration_structure_copy_commands;
     std::vector<VideoCommand::Storage> video_commands;
     std::vector<ClearAttachmentsCommand::Storage> clear_attachments_commands;
+    std::vector<QueryCopyCommand::Storage> query_copy_commands;
 
     //
     // Resources and additional data used by the commands
@@ -1051,6 +1073,9 @@ struct CommandData {
     CommandRef Store(const ClearAttachmentsCommand::Storage& storage) {
         return Store(CommandType::kClearAttachments, clear_attachments_commands, storage);
     }
+    CommandRef Store(const QueryCopyCommand::Storage& storage) {
+        return Store(CommandType::kQueryCopy, query_copy_commands, storage);
+    }
 
   private:
     template <typename Storage>

layers/sync/sync_command_buffer.cpp

@@ -945,6 +945,10 @@ void CommandBufferContext::RecordExecutedCommandBuffer(const CommandBufferContex
                     StoreCommand(tag, command, entry.tag_count);
                     continue;
                 }
+                case CommandType::kQueryCopy: {
+                    import_common(command_data.query_copy_commands[index], command_data, tag, entry.tag_count);
+                    continue;
+                }
             }
             assert(false);
         }
@@ -1545,15 +1549,20 @@ void CommandBufferSubState::RecordCopyQueryPoolResults(vvl::QueryPool& pool_stat
         return;
     }
     const auto tag = cb_context.NextCommandTag(loc.function);
-    AccessContext& context = cb_context.GetCbAccessContext();
 
     const uint32_t query_size = (flags & VK_QUERY_RESULT_64_BIT) ? 8 : 4;
     const VkDeviceSize range_size = (query_count - 1) * stride + query_size;
     const AccessRange range = MakeRange(dst_offset, range_size);
     const ResourceUsageTagEx tag_ex = cb_context.AddCommandHandle(tag, dst_buffer_state.Handle());
-    context.UpdateAccessState(dst_buffer_state, SYNC_COPY_TRANSFER_WRITE, range, tag_ex);
+    const QueryCopyCommand command{dst_buffer_state, range, pool_state.VkHandle(), tag_ex.handle_index};
 
-    // TODO:Track VkQueryPool
+    const auto& settings = cb_context.GetSyncState().syncval_settings;
+    if (settings.IsRecordTimeValidationEnabled()) {
+        command.Apply(cb_context.GetSyncEnvironment(), tag, cb_context.GetCbAccessContext());
+    }
+    if (settings.full_validation) {
+        cb_context.StoreCommand(tag, command);
+    }
 }
 
 void CommandBufferSubState::RecordBeginRenderPass(const VkRenderPassBeginInfo& render_pass_begin,

layers/sync/sync_error_messages.cpp

@@ -96,19 +96,6 @@ std::string ErrorMessages::Error(const SyncEnvironment& env, const HazardResult&
     return message;
 }
 
-std::string ErrorMessages::BufferError(const HazardResult& hazard, const CommandBufferContext& cb_context, vvl::Func command,
-                                       const std::string& resource_description, const AccessRange range,
-                                       AdditionalMessageInfo additional_info) const {
-    std::ostringstream ss;
-    ss << "\nBuffer access region: {\n";
-    ss << "  offset = " << range.begin << "\n";
-    ss << "  size = " << range.end - range.begin << "\n";
-    ss << "}\n";
-    additional_info.message_end_text += ss.str();
-
-    return Error(cb_context.GetSyncEnvironment(), hazard, command, resource_description, "BufferError", additional_info);
-}
-
 std::string ErrorMessages::BufferError(const SyncEnvironment& env, const HazardResult& hazard,
                                        const CommandBufferContext& cb_context, ResourceUsageTag replay_tag, const Location& loc,
                                        const std::string& resource_description, const AccessRange range) const {

layers/sync/sync_error_messages.h

@@ -47,11 +47,6 @@ class ErrorMessages {
                       const std::string& resource_description, const char* message_type,
                       const AdditionalMessageInfo& additional_info = {}) const;
 
-    // TODO: temp legacy version
-    std::string BufferError(const HazardResult& hazard, const CommandBufferContext& cb_context, vvl::Func command,
-                            const std::string& resource_description, const AccessRange range,
-                            AdditionalMessageInfo additional_info = {}) const;
-
     std::string BufferError(const SyncEnvironment& env, const HazardResult& hazard, const CommandBufferContext& cb_context,
                             ResourceUsageTag replay_tag, const Location& loc, const std::string& resource_description,
                             const AccessRange range) const;

layers/sync/sync_validation.cpp

@@ -1595,28 +1595,22 @@ bool SyncValidator::PreCallValidateCmdCopyQueryPoolResults(VkCommandBuffer comma
                                                            uint32_t firstQuery, uint32_t queryCount, VkBuffer dstBuffer,
                                                            VkDeviceSize dstOffset, VkDeviceSize stride, VkQueryResultFlags flags,
                                                            const ErrorObject& error_obj) const {
-    bool skip = false;
+    if (!syncval_settings.IsRecordTimeValidationEnabled() || queryCount == 0) {
+        return false;
+    }
+    const auto dst_buffer = Get<vvl::Buffer>(dstBuffer);
+    if (!dst_buffer) {
+        return false;
+    }
     const auto cb_state = Get<vvl::CommandBuffer>(commandBuffer);
     const CommandBufferContext& cb_context = GetCommandBufferContext(*cb_state);
-    const AccessContext& access_context = cb_context.GetCbAccessContext();
 
-    auto dst_buffer = Get<vvl::Buffer>(dstBuffer);
+    const uint32_t query_size = (flags & VK_QUERY_RESULT_64_BIT) ? 8 : 4;
+    const VkDeviceSize range_size = (queryCount - 1) * stride + query_size;
+    const AccessRange range = MakeRange(dstOffset, range_size);
 
-    if (dst_buffer && queryCount > 0) {
-        const uint32_t query_size = (flags & VK_QUERY_RESULT_64_BIT) ? 8 : 4;
-        const VkDeviceSize range_size = (queryCount - 1) * stride + query_size;
-        const AccessRange range = MakeRange(dstOffset, range_size);
-        auto hazard = access_context.DetectHazard(*dst_buffer, SYNC_COPY_TRANSFER_WRITE, range);
-        if (hazard.IsHazard()) {
-            const LogObjectList objlist(commandBuffer, queryPool, dstBuffer);
-            const std::string resource_description = "dstBuffer " + FormatHandle(dstBuffer);
-            const auto error =
-                error_messages_.BufferError(hazard, cb_context, error_obj.location.function, resource_description, range);
-            skip |= SyncError(hazard.Hazard(), objlist, error_obj.location, error);
-        }
-    }
-    // TODO:Track VkQueryPool
-    return skip;
+    const QueryCopyCommand command{*dst_buffer, range, queryPool};
+    return command.Validate(cb_context, error_obj.location);
 }
 
 bool SyncValidator::PreCallValidateCmdResolveImage(VkCommandBuffer commandBuffer, VkImage srcImage, VkImageLayout srcImageLayout,

tests/unit/sync_val.cpp

@@ -3374,6 +3374,69 @@ TEST_F(NegativeSyncVal, CmdQuery) {
     // TODO:CmdWriteTimestamp
 }
 
+TEST_F(NegativeSyncVal, ReadAfterCopyQueryPoolResults) {
+    TEST_DESCRIPTION("Read the last query result in a later command buffer without synchronization");
+    RETURN_IF_SKIP(InitSyncVal());
+    if (!m_device->Physical().limits_.timestampComputeAndGraphics) {
+        GTEST_SKIP() << "Timestamps not supported";
+    }
+
+    vkt::QueryPool query_pool(*m_device, VK_QUERY_TYPE_TIMESTAMP, 2);
+    vkt::Buffer results(*m_device, 64, VK_BUFFER_USAGE_TRANSFER_SRC_BIT | VK_BUFFER_USAGE_TRANSFER_DST_BIT);
+    vkt::Buffer dst(*m_device, 4, VK_BUFFER_USAGE_TRANSFER_DST_BIT);
+
+    m_command_buffer.Begin();
+    vk::CmdResetQueryPool(m_command_buffer, query_pool, 0, 2);
+    vk::CmdWriteTimestamp(m_command_buffer, VK_PIPELINE_STAGE_ALL_GRAPHICS_BIT, query_pool, 0);
+    vk::CmdWriteTimestamp(m_command_buffer, VK_PIPELINE_STAGE_ALL_GRAPHICS_BIT, query_pool, 1);
+    vk::CmdCopyQueryPoolResults(m_command_buffer, query_pool, 0, 2, results, 16 /* offset */, 16 /* stride */,
+                                VK_QUERY_RESULT_64_BIT | VK_QUERY_RESULT_WAIT_BIT);
+    m_command_buffer.End();
+
+    // The second result occupies [32, 40). Read its upper four bytes
+    VkBufferCopy region{};
+    region.srcOffset = 36;
+    region.dstOffset = 0;
+    region.size = 4;
+
+    vkt::CommandBuffer read_cb(*m_device, m_command_pool);
+    read_cb.Begin();
+    vk::CmdCopyBuffer(read_cb, results, dst, 1, &region);
+    read_cb.End();
+
+    m_errorMonitor->SetDesiredError("SYNC-HAZARD-READ-AFTER-WRITE");
+    m_default_queue->Submit({m_command_buffer, read_cb});
+    m_errorMonitor->VerifyFound();
+    m_default_queue->Wait();
+}
+
+TEST_F(NegativeSyncVal, CopyQueryPoolResultsAfterWrite) {
+    TEST_DESCRIPTION("Copy a query result over a buffer write from another command buffer");
+    RETURN_IF_SKIP(InitSyncVal());
+    if (!m_device->Physical().limits_.timestampComputeAndGraphics) {
+        GTEST_SKIP() << "Timestamps not supported";
+    }
+
+    vkt::QueryPool query_pool(*m_device, VK_QUERY_TYPE_TIMESTAMP, 1);
+    vkt::Buffer results(*m_device, 4, VK_BUFFER_USAGE_TRANSFER_DST_BIT);
+
+    m_command_buffer.Begin();
+    vk::CmdFillBuffer(m_command_buffer, results, 0, 4, 0);
+    m_command_buffer.End();
+
+    vkt::CommandBuffer query_cb(*m_device, m_command_pool);
+    query_cb.Begin();
+    vk::CmdResetQueryPool(query_cb, query_pool, 0, 1);
+    vk::CmdWriteTimestamp(query_cb, VK_PIPELINE_STAGE_ALL_GRAPHICS_BIT, query_pool, 0);
+    vk::CmdCopyQueryPoolResults(query_cb, query_pool, 0, 1, results, 0, 4, VK_QUERY_RESULT_WAIT_BIT);
+    query_cb.End();
+
+    m_errorMonitor->SetDesiredError("SYNC-HAZARD-WRITE-AFTER-WRITE");
+    m_default_queue->Submit({m_command_buffer, query_cb});
+    m_errorMonitor->VerifyFound();
+    m_default_queue->Wait();
+}
+
 TEST_F(NegativeSyncVal, CmdDrawDepthStencil) {
     RETURN_IF_SKIP(InitSyncValFramework());
     RETURN_IF_SKIP(InitState());