diff --git a/BUILD.gn b/BUILD.gn index 80774517667..f53e6c8f966 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -144,8 +144,6 @@ vvl_sources = [ "layers/core_checks/cc_spirv.cpp", "layers/core_checks/cc_state_tracker.cpp", "layers/core_checks/cc_state_tracker.h", - "layers/core_checks/cc_submit.cpp", - "layers/core_checks/cc_submit.h", "layers/core_checks/cc_sync_vuid_maps.h", "layers/core_checks/cc_sync_vuid_maps.cpp", "layers/core_checks/cc_synchronization.cpp", diff --git a/layers/CMakeLists.txt b/layers/CMakeLists.txt index d183cc046e8..b635cea3d4f 100644 --- a/layers/CMakeLists.txt +++ b/layers/CMakeLists.txt @@ -244,8 +244,6 @@ target_sources(vvl PRIVATE core_checks/cc_shader_object.cpp core_checks/cc_state_tracker.h core_checks/cc_state_tracker.cpp - core_checks/cc_submit.h - core_checks/cc_submit.cpp core_checks/cc_sync_vuid_maps.h core_checks/cc_sync_vuid_maps.cpp core_checks/cc_synchronization.h diff --git a/layers/core_checks/cc_queue.cpp b/layers/core_checks/cc_queue.cpp index c5321aae530..3422c2faa8e 100644 --- a/layers/core_checks/cc_queue.cpp +++ b/layers/core_checks/cc_queue.cpp @@ -290,9 +290,11 @@ bool CoreChecks::PreCallValidateQueueSubmit(VkQueue queue, uint32_t submitCount, chained_device_group_struct->commandBufferCount, submit.commandBufferCount); } } - // Perform submit time validation at the end. - // If submit API is used incorrectly, we want those errors to be reported first - skip |= submit_time_tracker.ProcessSubmitInfo(submit, queue, submit_loc); + // Run submit time validation only if skip is not set. This is mostly for vvl testing, + // some tests expect that submit state is not updated if regular validation fails + if (!skip) { + skip |= submit_time_tracker.ProcessSubmitInfo(submit, queue, submit_loc); + } } return skip; @@ -452,9 +454,11 @@ bool CoreChecks::ValidateQueueSubmit2(VkQueue queue, uint32_t submitCount, const skip |= LogError("VUID-VkSubmitInfo2-commandBuffer-06010", queue, submit_loc, "has a suspended render pass instance that was not resumed."); } - // Perform submit time validation at the end. - // If submit API is used incorrectly, we want those errors to be reported first - skip |= submit_time_tracker.ProcessSubmitInfo(submit, queue, submit_loc); + // Run submit time validation only if skip is not set. This is mostly for vvl testing, + // some tests expect that submit state is not updated if regular validation fails + if (!skip) { + skip |= submit_time_tracker.ProcessSubmitInfo(submit, queue, submit_loc); + } } return skip; @@ -783,8 +787,21 @@ bool CoreChecks::PreCallValidateQueueBindSparse(VkQueue queue, uint32_t bindInfo return skip; } -bool CoreChecks::ProcessSubmissionBatch(const std::vector>& command_buffers, - const Location& submit_loc) { +static Location GetSignaledSemaphoreLocation(const Location& submit_loc, uint32_t index) { + vvl::Field field = vvl::Field::Empty; + if (submit_loc.function == vvl::Func::vkQueueSubmit || submit_loc.function == vvl::Func::vkQueueBindSparse) { + field = vvl::Field::pSignalSemaphores; + } else if (submit_loc.function == vvl::Func::vkQueueSubmit2 || submit_loc.function == vvl::Func::vkQueueSubmit2KHR) { + field = vvl::Field::pSignalSemaphoreInfos; + } else { + assert(false && "Unhandled signaling function"); + } + return submit_loc.dot(field, index); +} + +bool CoreChecks::ProcessSubmissionBatch(const vvl::SubmitTimeTracker& tracker, + const std::vector>& command_buffers, + vvl::span signal_semaphores, const Location& submit_loc) { bool skip = false; // Validate image layouts on the command buffer boundaries { @@ -796,6 +813,27 @@ bool CoreChecks::ProcessSubmissionBatch(const std::vector current_value = tracker.GetTimelineValue(semaphore); + if (!current_value.has_value()) { + continue; + } + // Validate the case where the signal value is less than the current payload. + // Equality, which is also invalid, is handled earlier by SemaphoreSubmitState::ValidateTimelineSignal. + // Equality can be checked early because it is invalid regardless submit order. + // For not equal values, the comparison depends on submit ordering and is handled here + const uint64_t signal_value = signal_semaphores[i].value; + const bool invlid_signal_value = signal_value < *current_value; + if (invlid_signal_value) { + const Location signal_semaphore_loc = GetSignaledSemaphoreLocation(submit_loc, i); + const auto& vuid = GetQueueSubmitVUID(signal_semaphore_loc, vvl::SubmitError::kTimelineSemSmallValue); + LogError(vuid, semaphore, signal_semaphore_loc, + "(%s) signaled with value %" PRIu64 " which is smaller than the current value %" PRIu64, + FormatHandle(semaphore).c_str(), signal_value, *current_value); + } + } if (!skip) { for (const auto& cb : command_buffers) { if (cb) { diff --git a/layers/core_checks/cc_state_tracker.cpp b/layers/core_checks/cc_state_tracker.cpp index cf0e9367857..3efb983f16a 100644 --- a/layers/core_checks/cc_state_tracker.cpp +++ b/layers/core_checks/cc_state_tracker.cpp @@ -41,7 +41,7 @@ void CoreChecks::Created(vvl::CommandBuffer& cb) { } void CoreChecks::Created(vvl::Queue& queue) { - queue.SetSubState(container_type, std::make_unique(*this, queue)); + queue.SetSubState(container_type, std::make_unique(queue)); } namespace core { @@ -1214,9 +1214,6 @@ void CommandBufferSubState::SubmitTimeValidate() { } } -QueueSubState::QueueSubState(CoreChecks& core_checks, vvl::Queue& q) - : vvl::QueueSubState(q), queue_submission_validator_(core_checks) {} - void QueueSubState::PreSubmit(std::vector& submissions) { for (const auto& submission : submissions) { for (auto& cb : submission.cb_submissions) { @@ -1228,8 +1225,6 @@ void QueueSubState::PreSubmit(std::vector& submissions) { } void QueueSubState::Retire(vvl::QueueSubmission& submission) { - queue_submission_validator_.Validate(submission); - auto is_query_updated_after = [this](const QueryObject& query_object) { auto guard = base.Lock(); bool first_queue_submission = true; diff --git a/layers/core_checks/cc_state_tracker.h b/layers/core_checks/cc_state_tracker.h index 6c451671aa9..5beffb57546 100644 --- a/layers/core_checks/cc_state_tracker.h +++ b/layers/core_checks/cc_state_tracker.h @@ -18,7 +18,6 @@ * limitations under the License. */ #pragma once -#include "cc_submit.h" #include "state_tracker/state_tracker.h" #include "state_tracker/cmd_buffer_state.h" #include "state_tracker/queue_state.h" @@ -245,15 +244,12 @@ static inline const CommandBufferSubState &SubState(const vvl::CommandBuffer &cb class QueueSubState : public vvl::QueueSubState { public: - QueueSubState(CoreChecks &core_checks, vvl::Queue& q); + QueueSubState(vvl::Queue& q) : vvl::QueueSubState(q) {} void PreSubmit(std::vector &submissions) override; // Override Retire to validate submissions in the order defined by synchronization void Retire(vvl::QueueSubmission&) override; - - private: - QueueSubmissionValidator queue_submission_validator_; }; } // namespace core diff --git a/layers/core_checks/cc_submit.cpp b/layers/core_checks/cc_submit.cpp deleted file mode 100644 index 1f4a76dd8d9..00000000000 --- a/layers/core_checks/cc_submit.cpp +++ /dev/null @@ -1,59 +0,0 @@ -/* Copyright (c) 2026 The Khronos Group Inc. - * Copyright (c) 2026 Valve Corporation - * Copyright (c) 2026 LunarG, Inc. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#include "cc_submit.h" -#include "core_checks/cc_sync_vuid_maps.h" -#include "core_checks/core_validation.h" -#include "state_tracker/cmd_buffer_state.h" -#include "state_tracker/image_state.h" -#include "state_tracker/queue_state.h" - -static Location GetSignaledSemaphoreLocation(const Location& submit_loc, uint32_t index) { - vvl::Field field = vvl::Field::Empty; - if (submit_loc.function == vvl::Func::vkQueueSubmit || submit_loc.function == vvl::Func::vkQueueBindSparse) { - field = vvl::Field::pSignalSemaphores; - } else if (submit_loc.function == vvl::Func::vkQueueSubmit2 || submit_loc.function == vvl::Func::vkQueueSubmit2KHR) { - field = vvl::Field::pSignalSemaphoreInfos; - } else { - assert(false && "Unhandled signaling function"); - } - return submit_loc.dot(field, index); -} - -void QueueSubmissionValidator::Validate(const vvl::QueueSubmission& submission) const { - // Ensure that timeline signals are monotonically increasing values - for (uint32_t i = 0; i < (uint32_t)submission.signal_semaphores.size(); ++i) { - const auto& signal = submission.signal_semaphores[i]; - const uint64_t current_payload = signal.semaphore->CurrentPayload(); - - // Check only the case where the signal value is less than the current payload. - // Equality (also invalid) is handled during QueueSubmit. We can do such an early - // equality check because execution reordering of submits cannot change the result - // of comparison of equal values. On the other hand, for not equal values the - // result of comparison depends on the ordering of submits, which is only known at - // execution time (here). - const bool invlid_signal_value = signal.payload < current_payload; - - if (invlid_signal_value) { - const Location signal_semaphore_loc = GetSignaledSemaphoreLocation(submission.loc.Get(), i); - const auto& vuid = GetQueueSubmitVUID(signal_semaphore_loc, vvl::SubmitError::kTimelineSemSmallValue); - core_checks.LogError(vuid, signal.semaphore->Handle(), signal_semaphore_loc, - "(%s) signaled with value %" PRIu64 " which is smaller than the current value %" PRIu64, - core_checks.FormatHandle(signal.semaphore->VkHandle()).c_str(), signal.payload, current_payload); - } - } -} diff --git a/layers/core_checks/cc_submit.h b/layers/core_checks/cc_submit.h deleted file mode 100644 index dede4145cae..00000000000 --- a/layers/core_checks/cc_submit.h +++ /dev/null @@ -1,39 +0,0 @@ -/* Copyright (c) 2026 The Khronos Group Inc. - * Copyright (c) 2026 Valve Corporation - * Copyright (c) 2026 LunarG, Inc. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#pragma once - -// About submit time validation module: -// In some cases validation of the submitted work cannot be done directly during QueueSubmit call. -// Timeline semaphore's wait-before-signal can reorder batches comparing to the submission order. -// That's why validation that depends on the final batch order cannot be done immediately in -// QueueSubmit (it will validate in submission order). This file provides support for submit validation -// where ordering is important. - -namespace vvl { -struct QueueSubmission; -} // namespace vvl - -class CoreChecks; - -// Performs validationn when QueueSubmision is ready to retire. -struct QueueSubmissionValidator { - CoreChecks &core_checks; - - QueueSubmissionValidator(CoreChecks &core_checks) : core_checks(core_checks) {} - void Validate(const vvl::QueueSubmission &submission) const; -}; diff --git a/layers/core_checks/cc_synchronization.cpp b/layers/core_checks/cc_synchronization.cpp index 2d4ea5cc19b..063ecaeba88 100644 --- a/layers/core_checks/cc_synchronization.cpp +++ b/layers/core_checks/cc_synchronization.cpp @@ -1648,9 +1648,11 @@ bool CoreChecks::PreCallValidateSignalSemaphore(VkDevice device, const VkSemapho FormatHandle(*semaphore_state).c_str(), payload_type, *far_away_payload); } - // Perform submit time validation at the end. - // If signal semaphore API is used incorrectly, we want those errors to be reported first - skip |= submit_time_tracker.ProcessSignalSemaphore(*pSignalInfo); + // Run submit time validation only if skip is not set. This is mostly for vvl testing, + // some tests expect that submit state is not updated if regular validation fails + if (!skip) { + skip |= submit_time_tracker.ProcessSignalSemaphore(*pSignalInfo); + } return skip; } diff --git a/layers/core_checks/cc_wsi.cpp b/layers/core_checks/cc_wsi.cpp index a3be49887ac..fa8d2bc880f 100644 --- a/layers/core_checks/cc_wsi.cpp +++ b/layers/core_checks/cc_wsi.cpp @@ -1263,9 +1263,11 @@ bool CoreChecks::PreCallValidateQueuePresentKHR(VkQueue queue, const VkPresentIn skip |= ValidateSwapchainPresentFenceInfo(queue, *pPresentInfo, *swapchain_present_fence_info, present_info_loc); } - // Perform submit time validation at the end. - // If present API is used incorrectly, we want those errors to be reported first - skip |= submit_time_tracker.ProcessPresent(*pPresentInfo, present_info_loc); + // Run submit time validation only if skip is not set. This is mostly for vvl testing, + // some tests expect that submit state is not updated if regular validation fails + if (!skip) { + skip |= submit_time_tracker.ProcessPresent(*pPresentInfo, present_info_loc); + } return skip; } diff --git a/layers/core_checks/core_validation.h b/layers/core_checks/core_validation.h index bffa4a00ca0..2446911422c 100644 --- a/layers/core_checks/core_validation.h +++ b/layers/core_checks/core_validation.h @@ -1587,8 +1587,9 @@ class CoreChecks : public vvl::DeviceProxy { const RecordObject& record_obj) override; void PostCallRecordQueueSubmit2(VkQueue queue, uint32_t submitCount, const VkSubmitInfo2* pSubmits, VkFence fence, const RecordObject& record_obj) override; - bool ProcessSubmissionBatch(const std::vector>& command_buffers, - const Location& submit_loc) override; + bool ProcessSubmissionBatch(const vvl::SubmitTimeTracker& tracker, + const std::vector>& command_buffers, + vvl::span signal_semaphores, const Location& submit_loc) override; bool ProcessPresentBatch(const vvl::Image& swapchain_image, const Location& present_info_loc) override; bool IgnoreAllocationSize(const VkMemoryAllocateInfo& allocate_info) const; bool HasExternalMemoryImportSupport(const vvl::Buffer& buffer, VkExternalMemoryHandleTypeFlagBits handle_type) const; diff --git a/layers/state_tracker/queue_state.h b/layers/state_tracker/queue_state.h index ed5618e5300..36b6bb8317f 100644 --- a/layers/state_tracker/queue_state.h +++ b/layers/state_tracker/queue_state.h @@ -20,22 +20,20 @@ #include "state_tracker/state_object.h" #include "state_tracker/fence_state.h" #include "state_tracker/semaphore_state.h" +#include "error_message/error_location.h" +#include "chassis/dispatch_object.h" +#include "vk_layer_config.h" #include #include #include +#include #include #include -#include -#include "error_message/error_location.h" -#include "chassis/dispatch_object.h" -#include "vk_layer_config.h" namespace vvl { class CommandBuffer; class DeviceState; -class Image; -class Queue; class QueueSubState; struct CommandBufferSubmission { @@ -72,7 +70,6 @@ struct QueueSubmission { // Swapchain is not null if this submission represents QueuePresent request std::shared_ptr swapchain; - std::shared_ptr swapchain_image; LocationCapture loc; uint64_t seq{0}; diff --git a/layers/state_tracker/semaphore_state.cpp b/layers/state_tracker/semaphore_state.cpp index 9d5fb39a062..8ca0ab2ad57 100644 --- a/layers/state_tracker/semaphore_state.cpp +++ b/layers/state_tracker/semaphore_state.cpp @@ -407,7 +407,7 @@ bool vvl::Semaphore::CanRetireTimelineWait(const vvl::Queue* current_queue, uint continue; } // If the next signal is on the waiting (current) queue, it can't be a resolving signal (blocked by wait). - // QueueSubmissionValidator will also report an error about non-increasing signal values + // Submit time validation will also report an error about non-increasing signal values if (t.signal_submit->queue != nullptr && t.signal_submit->queue == current_queue) { continue; } diff --git a/layers/state_tracker/state_tracker.cpp b/layers/state_tracker/state_tracker.cpp index d38a04ea5c7..b092c1d548c 100644 --- a/layers/state_tracker/state_tracker.cpp +++ b/layers/state_tracker/state_tracker.cpp @@ -4398,9 +4398,7 @@ void DeviceState::PostCallRecordQueuePresentKHR(VkQueue queue, const VkPresentIn if (present_fence_info) { present_submission.AddFence(Get(present_fence_info->pFences[i])); } - auto swapchain = Get(pPresentInfo->pSwapchains[i]); - present_submission.swapchain = swapchain; - present_submission.swapchain_image = swapchain->GetSwapChainImageShared(pPresentInfo->pImageIndices[i]); + present_submission.swapchain = Get(pPresentInfo->pSwapchains[i]); } vvl::Semaphore::SwapchainWaitInfo semaphore_swapchain_info; diff --git a/layers/state_tracker/state_tracker.h b/layers/state_tracker/state_tracker.h index 673aff7ed3b..fe98cf09f2e 100644 --- a/layers/state_tracker/state_tracker.h +++ b/layers/state_tracker/state_tracker.h @@ -83,6 +83,7 @@ struct ShaderObject; class VideoSession; class VideoSessionParameters; class DataGraphPipelineSession; +class SubmitTimeTracker; } // namespace vvl namespace chassis { @@ -2358,8 +2359,9 @@ class DeviceProxy : public vvl::BaseDevice { // especially when a timeline signal resolves pending work on another queue. // This became even more important after the spec allowed internally synchronized queues, // which means the same queue can be used from multiple threads - virtual bool ProcessSubmissionBatch(const std::vector>& command_buffers, - const Location& submit_loc) { + virtual bool ProcessSubmissionBatch(const SubmitTimeTracker& tracker, + const std::vector>& command_buffers, + vvl::span signal_semaphores, const Location& submit_loc) { return false; } diff --git a/layers/state_tracker/submit_time_tracker.cpp b/layers/state_tracker/submit_time_tracker.cpp index fbf4f4296fb..7c8dcf4b68b 100644 --- a/layers/state_tracker/submit_time_tracker.cpp +++ b/layers/state_tracker/submit_time_tracker.cpp @@ -100,7 +100,7 @@ bool SubmitTimeTracker::ProcessBatch(std::vector> return skip; } - skip |= validator_.ProcessSubmissionBatch(command_buffers, submit_loc); + skip |= validator_.ProcessSubmissionBatch(*this, command_buffers, signal_semaphores, submit_loc); const bool new_timeline_signals = RegisterTimelineSignals(signal_semaphores); if (new_timeline_signals) { @@ -159,7 +159,8 @@ bool SubmitTimeTracker::PropagateTimelineSignals() { if (!CanBeResolved(batch)) { break; } - skip |= validator_.ProcessSubmissionBatch(batch.command_buffers, batch.submit_loc_capture.Get()); + const auto signals = vvl::span(batch.signals.data(), batch.signals.size()); + skip |= validator_.ProcessSubmissionBatch(*this, batch.command_buffers, signals, batch.submit_loc_capture.Get()); new_timeline_signals |= RegisterTimelineSignals(batch.signals); batches.erase(batches.begin()); } diff --git a/layers/state_tracker/submit_time_tracker.h b/layers/state_tracker/submit_time_tracker.h index 3cd50a4479b..bbd9658b8e7 100644 --- a/layers/state_tracker/submit_time_tracker.h +++ b/layers/state_tracker/submit_time_tracker.h @@ -64,6 +64,8 @@ class SubmitTimeTracker { bool ProcessSignalSemaphore(const VkSemaphoreSignalInfo& signal_info) const; bool ProcessPresent(const VkPresentInfoKHR& present_info, const Location& present_info_loc) const; + std::optional GetTimelineValue(VkSemaphore timeline) const; + private: bool ProcessBatch(std::vector>&& command_buffers, vvl::span wait_semaphores, @@ -74,7 +76,6 @@ class SubmitTimeTracker { bool RegisterTimelineSignals(vvl::span signal_semaphores); bool PropagateTimelineSignals(); bool CanBeResolved(const UnresolvedBatch& batch) const; - std::optional GetTimelineValue(VkSemaphore timeline) const; bool UpdateTimelineValue(VkSemaphore timeline, uint64_t signal_value); private: diff --git a/tests/unit/wsi.cpp b/tests/unit/wsi.cpp index 1097a2aadca..2167f5c64bd 100644 --- a/tests/unit/wsi.cpp +++ b/tests/unit/wsi.cpp @@ -1153,7 +1153,7 @@ TEST_F(NegativeWsi, SwapchainPresentShared) { uint32_t image_index = 0; // Try to Present without Acquire... - m_errorMonitor->SetDesiredError("VUID-VkPresentInfoKHR-pImageIndices-01430", 2); // no acquire + wrong image layout + m_errorMonitor->SetDesiredError("VUID-VkPresentInfoKHR-pImageIndices-01430"); // not acquired m_default_queue->Present(m_swapchain, image_index, vkt::no_semaphore); m_errorMonitor->VerifyFound(); } @@ -1962,10 +1962,7 @@ TEST_F(NegativeWsi, GetSwapchainImagesCountButNotImages) { // This test initiates image count query, but don't need resulting value m_swapchain.GetImageCount(); - // VUID is signaled two times: - // 1) Image was not acquired - // 2) Image was not transition to present layout - m_errorMonitor->SetDesiredError("VUID-VkPresentInfoKHR-pImageIndices-01430", 2); + m_errorMonitor->SetDesiredError("VUID-VkPresentInfoKHR-pImageIndices-01430"); // not acquired m_default_queue->Present(m_swapchain, 0, vkt::no_semaphore); m_errorMonitor->VerifyFound(); @@ -4829,7 +4826,7 @@ TEST_F(NegativeWsi, SwapchainUseAfterDestroy) { m_errorMonitor->VerifyFound(); present.pSwapchains = &swapchain2.handle(); - m_errorMonitor->SetDesiredError("VUID-VkPresentInfoKHR-pImageIndices-01430", 2); // not acquired + wrong layout + m_errorMonitor->SetDesiredError("VUID-VkPresentInfoKHR-pImageIndices-01430"); // not acquired vk::QueuePresentKHR(*m_default_queue, &present); m_errorMonitor->VerifyFound();