sync: Fix regression due to old refactor

Questionable refactor:
#8702

Author: artem-lunarg

layers/sync/sync_validation.cpp

@@ -2560,7 +2560,7 @@ bool SyncValidator::PropagateTimelineSignals(SignalsUpdate& signals_update, cons
     // Each iteration uses registered timeline signals to resolve existing unresolved batches.
     // Each resolved batch can generate new timeline signals which can resolve more unresolved batches on the next iteration.
     // This finishes when all unresolved batches are resolved or when iteration does not generate new timeline signals.
-    while (PropagateTimelineSignalsIteration(queues, signals_update, skip, error_obj)) {
+    while (ProcessUnresolvedBatches(queues, signals_update, skip, error_obj)) {
         ;
     }
 
@@ -2573,21 +2573,55 @@ bool SyncValidator::PropagateTimelineSignals(SignalsUpdate& signals_update, cons
     return skip;
 }
 
-bool SyncValidator::PropagateTimelineSignalsIteration(std::vector<UnresolvedQueue>& queues, SignalsUpdate& signals_update,
-                                                      bool& skip, const ErrorObject& error_obj) const {
+bool SyncValidator::ProcessUnresolvedBatches(std::vector<UnresolvedQueue>& queues, SignalsUpdate& signals_update, bool& skip,
+                                             const ErrorObject& error_obj) const {
     bool has_new_timeline_signals = false;
     for (auto& queue : queues) {
         if (queue.unresolved_batches.empty()) {
             continue;  // all batches for this queue were resolved by previous iterations
         }
+        // Resolve waits that have matching signal
+        for (UnresolvedBatch& unresolved_batch : queue.unresolved_batches) {
+            auto it = unresolved_batch.unresolved_waits.begin();
+            while (it != unresolved_batch.unresolved_waits.end()) {
+                const VkSemaphoreSubmitInfo& wait_info = *it;
+                auto resolving_signal = signals_update.OnTimelineWait(wait_info.semaphore, wait_info.value);
+                if (!resolving_signal.has_value()) {
+                    ++it;
+                    continue;  // resolving signal not found, the wait stays unresolved
+                }
+                if (resolving_signal->batch) {  // null for host signals
+                    unresolved_batch.batch->ResolveSubmitSemaphoreWait(*resolving_signal, wait_info.stageMask);
+                    unresolved_batch.batch->ImportTags(*resolving_signal->batch);
+                    unresolved_batch.resolved_dependencies.emplace_back(resolving_signal->batch);
+                }
+                it = unresolved_batch.unresolved_waits.erase(it);
+            }
+        }
 
         BatchContextPtr last_batch = queue.queue_state->LastBatch();
         const BatchContextPtr initial_last_batch = last_batch;
 
-        while (!queue.unresolved_batches.empty()) {
-            auto& unresolved_batch = queue.unresolved_batches.front();
+        // Process batches that do not have unresolved waits anymore.
+        // Stop when find a batch with unresolved waits or when all the batches are processed.
+        while (!queue.unresolved_batches.empty() && queue.unresolved_batches.front().unresolved_waits.empty()) {
+            UnresolvedBatch& ready_batch = queue.unresolved_batches.front();
+
+            // Import the previous batch information
+            if (last_batch && !vvl::Contains(ready_batch.resolved_dependencies, last_batch)) {
+                ready_batch.batch->ResolveLastBatch(last_batch);
+                ready_batch.resolved_dependencies.emplace_back(std::move(last_batch));
+            }
+            last_batch = ready_batch.batch;
+
+            const auto async_batches = ready_batch.batch->RegisterAsyncContexts(ready_batch.resolved_dependencies);
+
+            skip |= ready_batch.batch->ValidateSubmit(ready_batch.command_buffers, ready_batch.submit_index,
+                                                      ready_batch.batch_index, ready_batch.label_stack, error_obj);
 
-            has_new_timeline_signals |= ProcessUnresolvedBatch(unresolved_batch, signals_update, last_batch, skip, error_obj);
+            // Process signals. New timeline signals can resolve more batches on the next iteration
+            const auto submit_signals = vvl::make_span(ready_batch.signals.data(), ready_batch.signals.size());
+            has_new_timeline_signals |= signals_update.RegisterSignals(ready_batch.batch, submit_signals);
 
             // Remove processed batch from the (local) unresolved list
             queue.unresolved_batches.erase(queue.unresolved_batches.begin());
@@ -2597,54 +2631,14 @@ bool SyncValidator::PropagateTimelineSignalsIteration(std::vector<UnresolvedQueu
 
             stats.RemoveUnresolvedBatch();
         }
+
         if (last_batch != initial_last_batch) {
             queue.queue_state->SetLastBatch(std::move(last_batch));
         }
     }
     return has_new_timeline_signals;
 }
 
-bool SyncValidator::ProcessUnresolvedBatch(UnresolvedBatch& unresolved_batch, SignalsUpdate& signals_update,
-                                           BatchContextPtr& last_batch, bool& skip, const ErrorObject& error_obj) const {
-    // Resolve waits that have matching signal
-    auto it = unresolved_batch.unresolved_waits.begin();
-    while (it != unresolved_batch.unresolved_waits.end()) {
-        const VkSemaphoreSubmitInfo& wait_info = *it;
-        auto resolving_signal = signals_update.OnTimelineWait(wait_info.semaphore, wait_info.value);
-        if (!resolving_signal.has_value()) {
-            ++it;
-            continue;  // resolving signal not found, the wait stays unresolved
-        }
-        if (resolving_signal->batch) {  // null for host signals
-            unresolved_batch.batch->ResolveSubmitSemaphoreWait(*resolving_signal, wait_info.stageMask);
-            unresolved_batch.batch->ImportTags(*resolving_signal->batch);
-            unresolved_batch.resolved_dependencies.emplace_back(resolving_signal->batch);
-        }
-        it = unresolved_batch.unresolved_waits.erase(it);
-    }
-
-    // This batch still has unresolved waits
-    if (!unresolved_batch.unresolved_waits.empty()) {
-        return false;  // no new timeline signals were registered
-    }
-
-    // Process fully resolved batch
-    UnresolvedBatch& ready_batch = unresolved_batch;
-    if (last_batch && !vvl::Contains(ready_batch.resolved_dependencies, last_batch)) {
-        ready_batch.batch->ResolveLastBatch(last_batch);
-        ready_batch.resolved_dependencies.emplace_back(std::move(last_batch));
-    }
-    last_batch = ready_batch.batch;
-
-    const auto async_batches = ready_batch.batch->RegisterAsyncContexts(ready_batch.resolved_dependencies);
-
-    skip |= ready_batch.batch->ValidateSubmit(ready_batch.command_buffers, ready_batch.submit_index, ready_batch.batch_index,
-                                              ready_batch.label_stack, error_obj);
-
-    const auto submit_signals = vvl::make_span(ready_batch.signals.data(), ready_batch.signals.size());
-    return signals_update.RegisterSignals(ready_batch.batch, submit_signals);
-}
-
 void SyncValidator::PostCallRecordGetFenceStatus(VkDevice device, VkFence fence, const RecordObject& record_obj) {
     if (!syncval_settings.submit_time_validation) return;
     if (record_obj.result == VK_SUCCESS) {

layers/sync/sync_validation.h

@@ -97,10 +97,8 @@ class SyncValidator : public vvl::DeviceProxy {
     bool PropagateTimelineSignals(SignalsUpdate& signals_update, const ErrorObject& error_obj);
 
     // Return true if new timeline signals were generated by resolved batches
-    bool PropagateTimelineSignalsIteration(std::vector<UnresolvedQueue> &queues, SignalsUpdate &signals_update, bool &skip,
-                                           const ErrorObject &error_ob) const;
-    bool ProcessUnresolvedBatch(UnresolvedBatch &unresolved_batch, SignalsUpdate &signals_update, BatchContextPtr &last_batch,
-                                bool &skip, const ErrorObject &error_obj) const;
+    bool ProcessUnresolvedBatches(std::vector<UnresolvedQueue>& queues, SignalsUpdate& signals_update, bool& skip,
+                                  const ErrorObject& error_ob) const;
 
     void ApplyTaggedWait(QueueId queue_id, ResourceUsageTag tag, const LastSynchronizedPresent &last_synchronized_present,
                          const std::vector<ResourceUsageTag> &queue_sync_tags);

tests/framework/sync_val_tests.h

@@ -44,4 +44,6 @@ class VkSyncValTest : public VkLayerTest {
     std::unique_ptr<vkt::rt::Pipeline> GetTraceRaysPipeline(VkAccelerationStructureKHR as);
     std::unique_ptr<vkt::as::BuildGeometryInfoKHR> BuildBLAS();
     std::unique_ptr<vkt::as::BuildGeometryInfoKHR> BuildTLAS(const vkt::as::AccelerationStructureKHR &blas);
+
+    static std::pair<vkt::Queue*, vkt::Queue*> GetTwoQueuesFromSameFamily(const std::vector<vkt ::Queue*>& queues);
 };

tests/unit/sync_val.cpp

@@ -4762,17 +4762,6 @@ TEST_F(NegativeSyncVal, QSBufferCopyVsFence) {
     m_default_queue->Wait();
 }
 
-static std::pair<vkt::Queue*, vkt::Queue*> GetTwoQueuesFromSameFamily(const std::vector<vkt ::Queue*>& queues) {
-    for (size_t i = 0; i < queues.size(); i++) {
-        for (size_t k = i + 1; k < queues.size(); k++) {
-            if (queues[i]->family_index == queues[k]->family_index) {
-                return {queues[i], queues[k]};
-            }
-        }
-    }
-    return {};
-}
-
 TEST_F(NegativeSyncVal, QSBufferCopyQSORules) {
     all_queue_count_ = true;
     RETURN_IF_SKIP(InitSyncVal());

tests/unit/sync_val_positive.cpp

@@ -97,6 +97,17 @@ void VkSyncValTest::InitRayTracing() {
     RETURN_IF_SKIP(InitSyncVal());
 }
 
+std::pair<vkt::Queue*, vkt::Queue*> VkSyncValTest::GetTwoQueuesFromSameFamily(const std::vector<vkt ::Queue*>& queues) {
+    for (size_t i = 0; i < queues.size(); i++) {
+        for (size_t k = i + 1; k < queues.size(); k++) {
+            if (queues[i]->family_index == queues[k]->family_index) {
+                return {queues[i], queues[k]};
+            }
+        }
+    }
+    return {};
+}
+
 TEST_F(PositiveSyncVal, BufferCopyNonOverlappedRegions) {
     TEST_DESCRIPTION("Copy to non-overlapped regions of the same buffer");
     RETURN_IF_SKIP(InitSyncVal());

tests/unit/sync_val_semaphore_positive.cpp

@@ -795,3 +795,41 @@ TEST_F(PositiveSyncValTimelineSemaphore, ConcurrentHostSignalAndSubmit) {
     }
     thread.join();
 }
+
+TEST_F(PositiveSyncValTimelineSemaphore, WaitBeforeSinglaBlocksAnotherSubmit) {
+    TEST_DESCRIPTION("Test that a batch blocked by a wait-before-signal batch is not validated");
+    all_queue_count_ = true;
+    RETURN_IF_SKIP(InitTimelineSemaphore());
+
+    auto [queue0, queue1] = GetTwoQueuesFromSameFamily(m_device->QueuesWithTransferCapability());
+    if (!queue0) {
+        GTEST_SKIP() << "Test requires two queues with transfer capabilities from the same queue family";
+    }
+
+    vkt::Buffer buffer_a(*m_device, 1024, VK_BUFFER_USAGE_TRANSFER_SRC_BIT);
+    vkt::Buffer buffer_b(*m_device, 1024, VK_BUFFER_USAGE_TRANSFER_DST_BIT);
+
+    vkt::CommandPool command_pool(*m_device, queue0->family_index);
+    vkt::CommandBuffer command_buffer(*m_device, command_pool);
+    command_buffer.Begin(VK_COMMAND_BUFFER_USAGE_SIMULTANEOUS_USE_BIT);
+    command_buffer.Copy(buffer_a, buffer_b);
+    command_buffer.End();
+
+    vkt::Semaphore timeline(*m_device, VK_SEMAPHORE_TYPE_TIMELINE);
+
+    queue0->Submit(vkt::no_cmd, vkt::TimelineWait(timeline, 2));
+    queue0->Submit(command_buffer);  // This will be block until the above wait is resolved
+
+    // This seemingly useless submit tests that syncval stops processing pending batches when
+    // it encounters the first batch blocked by wait-before-signal.
+    //
+    // Signal=1 does not resolve wait=2 in the first submit above, so the following submit on
+    // the same queue should not be processed, even though it does not wait on the semaphore directly.
+    //
+    // We had a bug where syncval iterated over all pending batches instead of stopping at the
+    // first blocked one. This introduced false positives.
+    queue1->Submit(command_buffer, vkt::TimelineSignal(timeline, 1));
+
+    queue1->Submit(vkt::no_cmd, vkt::TimelineSignal(timeline, 2));
+    m_device->Wait();
+}