feat(render): surface unavailable GPU timestamp diagnostics#128
feat(render): surface unavailable GPU timestamp diagnostics#128
Conversation
- Emit deterministic Tier 1 diagnostics when GPU timestamps are unavailable - Preserve GPU timing and debug-label evidence when timestamps are supported - Keep frame recording and profiling flows safe when timestamps are disabled - Add profiling tests for unavailable, available, and non-blocking readback paths - Archive the profiling OpenSpec proposal, design, tasks, and verification
Review Summary by QodoSurface unavailable GPU timestamp diagnostics with deterministic testability seam
WalkthroughsDescription• Add deterministic GPU timestamp unavailability testability seam via DiagnosticPolicy override • Introduce explicit GpuTimestampPool::create_unavailable() factory for forced-unavailable testing • Extract debug-label dispatch handling into ScopedDebugLabel RAII helper for safer profiling • Expand profiling test coverage with deterministic unavailable-path and GPU-timing evidence assertions • Archive profiling OpenSpec proposal, design, tasks, and verification artifacts Diagramflowchart LR
DP["DiagnosticPolicy<br/>gpu_timestamp_availability"]
CRT["ChainRuntime<br/>sync_gpu_timestamp_pool"]
GTP["GpuTimestampPool<br/>create_unavailable"]
DLS["ScopedDebugLabel<br/>RAII wrapper"]
CE["ChainExecutor<br/>record"]
Tests["Profiling Tests<br/>deterministic coverage"]
DP -- "force_unavailable" --> CRT
CRT -- "selects path" --> GTP
GTP -- "no-op pool" --> CE
DLS -- "wraps labels" --> CE
CRT -- "emits event" --> Tests
GTP -- "unit tests" --> Tests
DLS -- "dispatch tests" --> Tests
File Changes1. src/util/diagnostics/diagnostic_policy.hpp
|
Code Review by Qodo
1.
|
📝 WalkthroughWalkthroughAdds a test-only seam to deterministically force GPU timestamp unavailability: new Changes
Sequence DiagramsequenceDiagram
participant Test as Test
participant ChainRuntime as ChainRuntime
participant Policy as DiagnosticPolicy
participant Pool as GpuTimestampPool
Test->>ChainRuntime: sync_gpu_timestamp_pool()
ChainRuntime->>Policy: query gpu_timestamp_availability
alt force_unavailable
Policy-->>ChainRuntime: force_unavailable
ChainRuntime->>Pool: create_unavailable()
Pool-->>ChainRuntime: unavailable pool
ChainRuntime->>ChainRuntime: mark timestamps_inactive, emit unavailable event
else auto_detect
Policy-->>ChainRuntime: auto_detect
ChainRuntime->>Pool: create(create_info)
Pool-->>ChainRuntime: available / unavailable
alt available
ChainRuntime->>ChainRuntime: proceed with timestamp recording
else unavailable
ChainRuntime->>ChainRuntime: emit unavailable event, use no-op behavior
end
end
ChainRuntime-->>Test: pool ready (available or unavailable)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/render/chain/chain_runtime.cpp (1)
417-436:⚠️ Potential issue | 🟡 MinorDifferentiate policy-disabled timestamps from device unavailability.
When
gpu_timestamp_availability == force_unavailable, Lines 434-436 still emit"GPU timestamps are unavailable on this device". That makes the deterministic test/policy path report a hardware limitation, which muddies the diagnostic evidence.Suggested fix
std::unique_ptr<diagnostics::GpuTimestampPool> pool; + const bool forced_unavailable = + m_diagnostic_session->policy().gpu_timestamp_availability == + diagnostics::GpuTimestampAvailabilityMode::force_unavailable; - if (m_diagnostic_session->policy().gpu_timestamp_availability == - diagnostics::GpuTimestampAvailabilityMode::force_unavailable) { + if (forced_unavailable) { pool = diagnostics::GpuTimestampPool::create_unavailable(); } else { auto pool_result = diagnostics::GpuTimestampPool::create( m_resources->m_vk_ctx.device, m_resources->m_vk_ctx.physical_device, static_cast<uint32_t>(std::max<size_t>(m_resources->pass_count(), 1U)), m_resources->m_num_sync_indices); if (!pool_result) { emit_timestamp_event(m_diagnostic_session.get(), diagnostics::Severity::warning, "Failed to enable GPU timestamps: " + pool_result.error().message); return; } pool = std::move(*pool_result); } if (!pool->is_available()) { - emit_timestamp_event(m_diagnostic_session.get(), diagnostics::Severity::info, - "GPU timestamps are unavailable on this device"); + emit_timestamp_event( + m_diagnostic_session.get(), diagnostics::Severity::info, + forced_unavailable + ? "GPU timestamps are disabled by diagnostic policy" + : "GPU timestamps are unavailable on this device"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/render/chain/chain_runtime.cpp` around lines 417 - 436, The code currently treats policy-forced unavailability the same as hardware unavailability and emits "GPU timestamps are unavailable on this device"; change the logic so that when m_diagnostic_session->policy().gpu_timestamp_availability == diagnostics::GpuTimestampAvailabilityMode::force_unavailable (after creating diagnostics::GpuTimestampPool::create_unavailable()), you either emit a distinct diagnostic (e.g., "GPU timestamps disabled by policy") or suppress the hardware-unavailable message; update the check around pool->is_available() to inspect the policy (or a local flag set when calling GpuTimestampPool::create_unavailable()) and call emit_timestamp_event accordingly (reference m_diagnostic_session, policy(), gpu_timestamp_availability, GpuTimestampAvailabilityMode::force_unavailable, diagnostics::GpuTimestampPool::create_unavailable, emit_timestamp_event, and pool->is_available()).
🧹 Nitpick comments (2)
openspec/changes/archive/2026-03-11-profiling-gpu-timestamp-test-coverage/proposal.md (1)
36-42: Update the affected-areas table to include the extracted debug-label helper.Lines 36-42 no longer match the implemented file set: this change also ships
src/render/chain/debug_label_scope.hpp. Leaving it out makes the archived proposal under-report the production delta and drift from the rest of the archive set.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/changes/archive/2026-03-11-profiling-gpu-timestamp-test-coverage/proposal.md` around lines 36 - 42, The affected-areas table omission: add an entry for the newly shipped header by updating the table to include `src/render/chain/debug_label_scope.hpp` with a brief note like "Modified | Add extracted debug-label helper used by profiling/debug paths" so the archived proposal accurately reflects the production delta; reference the table rows that list other files (`gpu_timestamp_pool.hpp`, `gpu_timestamp_pool.cpp`, `chain_executor.cpp`, tests/*`) and insert the new row for `debug_label_scope.hpp` adjacent to the other `src/render/chain` entries to keep ordering consistent.tests/render/test_gpu_timestamp_pool.cpp (1)
12-124: Consider extracting the shared Vulkan test harness.This fixture plus the image/command helper block is now largely duplicated from
tests/render/test_runtime_diagnostics.cpp. Pulling it into a small render-test utility would keep future Vulkan setup fixes from drifting between the two profiling suites.Also applies to: 126-302
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/render/test_gpu_timestamp_pool.cpp` around lines 12 - 124, The VulkanRuntimeFixture and the duplicated image/command helper should be extracted into a shared test utility: create a new header (e.g., render_test_utils.hpp) that defines VulkanRuntimeFixture (including its constructor, destructor, available(), timestamp_period()) and the image/command helper functions used in both tests, move the duplicated setup/teardown and helper code there, and update tests test_gpu_timestamp_pool.cpp and test_runtime_diagnostics.cpp to include that header and use the shared VulkanRuntimeFixture and helpers instead of their local copies; ensure symbol names (VulkanRuntimeFixture, available(), timestamp_period(), and the image/command helper functions) remain unchanged so callers need only switch the include.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/render/chain/debug_label_scope.hpp`:
- Around line 20-31: The constructor of ScopedDebugLabel passes name.data() to
VkDebugUtilsLabelEXT::pLabelName but std::string_view may not be
null-terminated; fix by making a null-terminated std::string copy of the view
(e.g. local or a member like m_label_name) and pass m_label_name.c_str() to
pLabelName so the string remains valid for the duration of
dispatch.begin/dispatch.end calls; update the ScopedDebugLabel constructor and
any places using pLabelName to use this c_str() instead of name.data().
In `@tests/render/test_gpu_timestamp_pool.cpp`:
- Around line 108-115: The timestamp_period() implementation is using
properties.limits.timestampPeriod as a support check which is incorrect; update
VulkanRuntimeFixture::timestamp_period() to detect timestamp support by querying
VkQueueFamilyProperties for the selected queue family and checking
timestampValidBits (>0) and/or falling back to
VkPhysicalDeviceProperties.limits.timestampComputeAndGraphics (true) instead of
using timestampPeriod; then change the test skip gates (the checks at lines
referenced in tests/render/test_gpu_timestamp_pool.cpp and
tests/render/test_runtime_diagnostics.cpp) and GpuTimestampPool::create() to use
the same supported-flag logic (timestampValidBits on the chosen queue family or
timestampComputeAndGraphics) to decide availability, while still using
timestampPeriod only for the nanoseconds-per-tick conversion when timestamps are
supported.
---
Outside diff comments:
In `@src/render/chain/chain_runtime.cpp`:
- Around line 417-436: The code currently treats policy-forced unavailability
the same as hardware unavailability and emits "GPU timestamps are unavailable on
this device"; change the logic so that when
m_diagnostic_session->policy().gpu_timestamp_availability ==
diagnostics::GpuTimestampAvailabilityMode::force_unavailable (after creating
diagnostics::GpuTimestampPool::create_unavailable()), you either emit a distinct
diagnostic (e.g., "GPU timestamps disabled by policy") or suppress the
hardware-unavailable message; update the check around pool->is_available() to
inspect the policy (or a local flag set when calling
GpuTimestampPool::create_unavailable()) and call emit_timestamp_event
accordingly (reference m_diagnostic_session, policy(),
gpu_timestamp_availability, GpuTimestampAvailabilityMode::force_unavailable,
diagnostics::GpuTimestampPool::create_unavailable, emit_timestamp_event, and
pool->is_available()).
---
Nitpick comments:
In
`@openspec/changes/archive/2026-03-11-profiling-gpu-timestamp-test-coverage/proposal.md`:
- Around line 36-42: The affected-areas table omission: add an entry for the
newly shipped header by updating the table to include
`src/render/chain/debug_label_scope.hpp` with a brief note like "Modified | Add
extracted debug-label helper used by profiling/debug paths" so the archived
proposal accurately reflects the production delta; reference the table rows that
list other files (`gpu_timestamp_pool.hpp`, `gpu_timestamp_pool.cpp`,
`chain_executor.cpp`, tests/*`) and insert the new row for
`debug_label_scope.hpp` adjacent to the other `src/render/chain` entries to keep
ordering consistent.
In `@tests/render/test_gpu_timestamp_pool.cpp`:
- Around line 12-124: The VulkanRuntimeFixture and the duplicated image/command
helper should be extracted into a shared test utility: create a new header
(e.g., render_test_utils.hpp) that defines VulkanRuntimeFixture (including its
constructor, destructor, available(), timestamp_period()) and the image/command
helper functions used in both tests, move the duplicated setup/teardown and
helper code there, and update tests test_gpu_timestamp_pool.cpp and
test_runtime_diagnostics.cpp to include that header and use the shared
VulkanRuntimeFixture and helpers instead of their local copies; ensure symbol
names (VulkanRuntimeFixture, available(), timestamp_period(), and the
image/command helper functions) remain unchanged so callers need only switch the
include.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d9156350-a817-417a-afb9-ef3810d9d880
📒 Files selected for processing (21)
openspec/changes/archive/2026-03-11-profiling-gpu-timestamp-test-coverage/design.mdopenspec/changes/archive/2026-03-11-profiling-gpu-timestamp-test-coverage/proposal.mdopenspec/changes/archive/2026-03-11-profiling-gpu-timestamp-test-coverage/state.yamlopenspec/changes/archive/2026-03-11-profiling-gpu-timestamp-test-coverage/tasks.mdopenspec/changes/archive/2026-03-11-profiling-gpu-timestamp-test-coverage/verify-report.mdsrc/render/chain/chain_executor.cppsrc/render/chain/chain_runtime.cppsrc/render/chain/debug_label_scope.hppsrc/util/diagnostics/diagnostic_policy.hppsrc/util/diagnostics/gpu_timestamp_pool.cppsrc/util/diagnostics/gpu_timestamp_pool.hpptests/CMakeLists.txttests/render/test_diagnostic_event_model.cpptests/render/test_diagnostic_reporting.cpptests/render/test_gpu_timestamp_pool.cpptests/render/test_runtime_diagnostics.cpptests/visual/runtime_capture_main.cpptests/visual/test_image_compare.cpptests/visual/test_intermediate_golden.cpptests/visual/test_semantic_probes.cpptests/visual/test_temporal_golden.cpp
| ScopedDebugLabel(vk::CommandBuffer cmd, std::string_view name, | ||
| const std::array<float, 4>& color, DebugLabelDispatch dispatch = {}) | ||
| : m_command_buffer(cmd) { | ||
| if (!dispatch.is_available()) { | ||
| return; | ||
| } | ||
|
|
||
| VkDebugUtilsLabelEXT label{}; | ||
| label.sType = VK_STRUCTURE_TYPE_DEBUG_UTILS_LABEL_EXT; | ||
| label.pLabelName = name.data(); | ||
| std::copy(color.begin(), color.end(), label.color); | ||
| dispatch.begin(static_cast<VkCommandBuffer>(cmd), &label); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What does the Vulkan specification require for VkDebugUtilsLabelEXT::pLabelNameinvkCmdBeginDebugUtilsLabelEXT? Does it need to point to a null-terminated UTF-8 string, and is std::string_view::data() sufficient for that contract?
💡 Result:
The Vulkan spec (via the reference pages / registry metadata) requires VkDebugUtilsLabelEXT::pLabelName to point to a null-terminated UTF-8 string. In particular, the VkDebugUtilsLabelEXT description states that pLabelName is “a pointer to a null-terminated UTF-8 string containing the name of the label” [1], and the Vulkan registry XML marks pLabelName as len="null-terminated" [2]. vkCmdBeginDebugUtilsLabelEXT in turn requires pLabelInfo to point to a valid VkDebugUtilsLabelEXT [3], so that null-termination requirement applies when recording the command.
std::string_view::data() is not sufficient by itself for that contract, because string_view does not guarantee a trailing '\0' at data()[size()]. It’s only safe if the view is over a buffer that you know is null-terminated at exactly the end of the view (e.g., a view of a whole std::string, and you ensure view.size() == strlen(view.data())). Otherwise, use something that guarantees null-termination such as std::string::c_str() (or materialize a std::string/null-terminated buffer before the call).
🏁 Script executed:
cat -n src/render/chain/debug_label_scope.hppRepository: goggles-dev/Goggles
Length of output: 2118
Don't pass std::string_view::data() directly to Vulkan here.
Line 29 hands name.data() to VkDebugUtilsLabelEXT::pLabelName, but std::string_view does not guarantee a trailing '\0'. The Vulkan specification requires pLabelName to point to a null-terminated UTF-8 string. For sliced or non-literal views, passing a non-null-terminated pointer can cause invalid label recording or out-of-bounds reads in the driver.
Suggested fix
+#include <string>
+
ScopedDebugLabel(vk::CommandBuffer cmd, std::string_view name,
const std::array<float, 4>& color, DebugLabelDispatch dispatch = {})
: m_command_buffer(cmd) {
if (!dispatch.is_available()) {
return;
}
+ const std::string label_name{name};
VkDebugUtilsLabelEXT label{};
label.sType = VK_STRUCTURE_TYPE_DEBUG_UTILS_LABEL_EXT;
- label.pLabelName = name.data();
+ label.pLabelName = label_name.c_str();
std::copy(color.begin(), color.end(), label.color);
dispatch.begin(static_cast<VkCommandBuffer>(cmd), &label);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ScopedDebugLabel(vk::CommandBuffer cmd, std::string_view name, | |
| const std::array<float, 4>& color, DebugLabelDispatch dispatch = {}) | |
| : m_command_buffer(cmd) { | |
| if (!dispatch.is_available()) { | |
| return; | |
| } | |
| VkDebugUtilsLabelEXT label{}; | |
| label.sType = VK_STRUCTURE_TYPE_DEBUG_UTILS_LABEL_EXT; | |
| label.pLabelName = name.data(); | |
| std::copy(color.begin(), color.end(), label.color); | |
| dispatch.begin(static_cast<VkCommandBuffer>(cmd), &label); | |
| `#include` <string> | |
| ScopedDebugLabel(vk::CommandBuffer cmd, std::string_view name, | |
| const std::array<float, 4>& color, DebugLabelDispatch dispatch = {}) | |
| : m_command_buffer(cmd) { | |
| if (!dispatch.is_available()) { | |
| return; | |
| } | |
| const std::string label_name{name}; | |
| VkDebugUtilsLabelEXT label{}; | |
| label.sType = VK_STRUCTURE_TYPE_DEBUG_UTILS_LABEL_EXT; | |
| label.pLabelName = label_name.c_str(); | |
| std::copy(color.begin(), color.end(), label.color); | |
| dispatch.begin(static_cast<VkCommandBuffer>(cmd), &label); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/render/chain/debug_label_scope.hpp` around lines 20 - 31, The constructor
of ScopedDebugLabel passes name.data() to VkDebugUtilsLabelEXT::pLabelName but
std::string_view may not be null-terminated; fix by making a null-terminated
std::string copy of the view (e.g. local or a member like m_label_name) and pass
m_label_name.c_str() to pLabelName so the string remains valid for the duration
of dispatch.begin/dispatch.end calls; update the ScopedDebugLabel constructor
and any places using pLabelName to use this c_str() instead of name.data().
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/render/test_runtime_diagnostics.cpp (1)
1050-1053: Assert the disabled-dispatch path, not just the available one.This block only proves
record_frame()doesn't crash with the dispatcher nulled. It doesn't fail if profiling labels are still emitted through some other path, so the fallback behavior remains unverified.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/render/test_runtime_diagnostics.cpp` around lines 1050 - 1053, The test currently only calls record_frame() with DebugLabelDispatcherDisable but doesn't assert the disabled-dispatch behavior; modify the block that uses DebugLabelDispatcherDisable to also verify that no profiling labels were emitted after record_frame() (e.g., assert the recorded-labels container/count is zero or that the label-dispatch callback was not invoked). Locate the block around DebugLabelDispatcherDisable and record_frame() and add an assertion (using the test's existing label inspection helpers or profiler API) that confirms the disabled path produced no labels.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/render/test_gpu_timestamp_pool.cpp`:
- Around line 67-76: The test fixture currently forces
VkPhysicalDeviceVulkan13Features.dynamicRendering = VK_TRUE and attaches
features13 to device_info.pNext before calling vkCreateDevice (symbols:
VkPhysicalDeviceVulkan13Features, features13, device_info, vkCreateDevice,
physical_device, device), which can fail on hardware without that feature;
remove the unconditional enable by deleting the features13.dynamicRendering =
VK_TRUE assignment (or only set it after querying the physical device for
support) and avoid unconditionally chaining features13 into device_info.pNext so
device creation succeeds on timestamp-only capable devices.
- Around line 441-490: read_results(0u) is being checked before first_frame is
submitted, so the test never exercises the non-blocking/pending-read path;
submit first_frame to the queue (e.g. call submit_and_wait or an equivalent
submit that ensures it reaches the queue) after vkEndCommandBuffer(first_frame)
and before calling pool->read_results(0u), then proceed with the rest of the
test (keep using pool->reset_frame/read/write_* and the second_frame flow
unchanged) so that read_results(0u) observes a pending/in-progress result rather
than the unsubmitted empty case.
---
Nitpick comments:
In `@tests/render/test_runtime_diagnostics.cpp`:
- Around line 1050-1053: The test currently only calls record_frame() with
DebugLabelDispatcherDisable but doesn't assert the disabled-dispatch behavior;
modify the block that uses DebugLabelDispatcherDisable to also verify that no
profiling labels were emitted after record_frame() (e.g., assert the
recorded-labels container/count is zero or that the label-dispatch callback was
not invoked). Locate the block around DebugLabelDispatcherDisable and
record_frame() and add an assertion (using the test's existing label inspection
helpers or profiler API) that confirms the disabled path produced no labels.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b9802688-d2f7-416a-aa90-aa6e6ae6b590
📒 Files selected for processing (10)
src/render/backend/vulkan_context.cppsrc/render/chain/api/c/goggles_filter_chain.cppsrc/render/chain/chain_runtime.cppsrc/render/chain/debug_label_scope.hppsrc/render/chain/vulkan_context.hppsrc/util/diagnostics/gpu_timestamp_pool.cppsrc/util/diagnostics/gpu_timestamp_pool.hpptests/render/test_gpu_timestamp_pool.cpptests/render/test_runtime_diagnostics.cpptests/visual/runtime_capture.cpp
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/render/test_gpu_timestamp_pool.cpp`:
- Around line 40-55: The current selection picks the first graphics queue family
found and may miss families/devices that support timestamps; change the loop
that iterates `devices` and queue families so that for each candidate graphics
family you call
`goggles::diagnostics::GpuTimestampPool::supports_timestamps(...)` and prefer
the first family that returns true, setting `physical_device` and
`queue_family_index` to that family; only if no graphics family across all
devices supports timestamps, fall back to the original behavior of choosing the
first graphics-capable family found (using the existing
`vkGetPhysicalDeviceQueueFamilyProperties` logic and variables `devices`,
`physical_device`, `queue_family_index`).
- Around line 455-469: The test reads pending results immediately after
vkQueueSubmit but on fast hardware the GPU may already have finished; modify the
test around vkQueueSubmit/first_frame_fence and pool->read_results(0u) to guard
the empty assertion by checking vkGetFenceStatus(first_frame_fence) ==
VK_NOT_READY before calling CHECK(pending_results->empty()), or alternatively
block until the fence is signaled (vkWaitForFences) and then assert non-empty as
appropriate; locate the vkQueueSubmit call, the VkFence first_frame_fence, and
the pool->read_results(0u) usage to implement the chosen fence-status check or
explicit wait to avoid the race.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5e380677-0126-45fa-9e1e-829e364a1fd8
📒 Files selected for processing (1)
tests/render/test_gpu_timestamp_pool.cpp
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/render/test_gpu_timestamp_pool.cpp`:
- Around line 252-267: The helper submit_and_wait currently uses an infinite
wait via vkWaitForFences(..., UINT64_MAX) which can hang CI; change it to use a
bounded timeout (e.g. a constant like kFenceTimeoutNs = 5'000'000'000ULL for 5s)
and pass that instead of UINT64_MAX to vkWaitForFences, treat a timeout or
non-success return as a test failure (return false), and keep the existing
vkDestroyFence cleanup; update any test expectations accordingly to rely on the
limited wait.
- Around line 201-245: The test currently hard-codes VK_FORMAT_B8G8R8A8_UNORM in
image_info and view_info which may not support the requested usage/tiling on all
GPUs; before creating the image (before using image_info and view_info and
calling vkCreateImage/vkCreateImageView), query format support using
vkGetPhysicalDeviceImageFormatProperties (or
vkGetPhysicalDeviceFormatProperties) for the desired VkImageTiling
(VK_IMAGE_TILING_OPTIMAL) and usage bits (VK_IMAGE_USAGE_SAMPLED_BIT |
VK_IMAGE_USAGE_TRANSFER_SRC_BIT | VK_IMAGE_USAGE_TRANSFER_DST_BIT |
VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT), choose a format that returns supported
properties, and assign that format to image_info.format and view_info.format
(with a sensible fallback or early skip/return if no format is supported). Also
ensure any memory/type selection (find_memory_type) and subsequent operations
use the selected format consistently.
- Around line 14-24: The test requests Vulkan 1.3 which can cause
vkCreateInstance to fail on older drivers; update
VkApplicationInfo.app_info.apiVersion to VK_API_VERSION_1_0 and change the
vkCreateInstance failure handling in the test (the block around
VkApplicationInfo app_info and vkCreateInstance) to report/propagate the error
instead of silently returning so test failures aren’t masked as skips.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0a151f03-99a1-4e00-af75-38df585ce172
📒 Files selected for processing (1)
tests/render/test_gpu_timestamp_pool.cpp
| VkApplicationInfo app_info{}; | ||
| app_info.sType = VK_STRUCTURE_TYPE_APPLICATION_INFO; | ||
| app_info.pApplicationName = "goggles_gpu_timestamp_pool_tests"; | ||
| app_info.applicationVersion = VK_MAKE_VERSION(1, 0, 0); | ||
| app_info.apiVersion = VK_API_VERSION_1_3; | ||
|
|
||
| VkInstanceCreateInfo instance_info{}; | ||
| instance_info.sType = VK_STRUCTURE_TYPE_INSTANCE_CREATE_INFO; | ||
| instance_info.pApplicationInfo = &app_info; | ||
| if (vkCreateInstance(&instance_info, nullptr, &instance) != VK_SUCCESS) { | ||
| return; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the test file to understand all APIs being used
fd -t f "test_gpu_timestamp_pool.cpp" && wc -l tests/render/test_gpu_timestamp_pool.cppRepository: goggles-dev/Goggles
Length of output: 148
🏁 Script executed:
# Read the full test file to identify all Vulkan API calls
cat -n tests/render/test_gpu_timestamp_pool.cppRepository: goggles-dev/Goggles
Length of output: 27184
🏁 Script executed:
# Search for all vk* function calls in the file to understand API usage
rg "vk[A-Z]" tests/render/test_gpu_timestamp_pool.cpp -oRepository: goggles-dev/Goggles
Length of output: 238
🌐 Web query:
Vulkan VkApplicationInfo apiVersion behavior failure vkCreateInstance older driver compatibility
💡 Result:
VkApplicationInfo::apiVersionis the highest Vulkan core API version your app is designed to use. If you set it to0(or omitpApplicationInfo), that is treated as Vulkan 1.0.0. [1]- On older Vulkan 1.0 implementations/drivers,
vkCreateInstancecan fail withVK_ERROR_INCOMPATIBLE_DRIVERif the driver does not support theapiVersionyou requested. The Vulkan docs explicitly warn about this and recommend checking the available version before callingvkCreateInstance. [1][4] - The intended compatibility pattern is:
- Call
vkEnumerateInstanceVersionto get the maximum instance-level Vulkan version supported by the loader/runtime. [2][3] - Set
VkApplicationInfo::apiVersion = min(appDesiredVersion, reportedVersion)(and ifvkEnumerateInstanceVersionis unavailable, assume only 1.0). [3][1]
- Call
- Loader/ICD nuance (multi-driver / very old ICDs):
- If multiple drivers are present and some are Vulkan-1.0-only, a conforming loader should downgrade
apiVersionto 1.0 when talking to those 1.0-only drivers. [5] - With newer loader/driver interface negotiation, drivers using newer interfaces are instructed not to return
VK_ERROR_INCOMPATIBLE_DRIVERmerely because the app passed a higherapiVersionthan the driver supports (the loader/negotiation is expected to handle this more gracefully). [6]
- If multiple drivers are present and some are Vulkan-1.0-only, a conforming loader should downgrade
Practical takeaway: if you want “old driver compatibility”, don’t hardcode VK_API_VERSION_1_3/1_4 in VkApplicationInfo unconditionally—query with vkEnumerateInstanceVersion and clamp, otherwise you can trigger VK_ERROR_INCOMPATIBLE_DRIVER on older stacks. [1][2][4]
Sources:
[1] VkApplicationInfo (Khronos) (registry.khronos.org)
[2] vkEnumerateInstanceVersion (Khronos) (registry.khronos.org)
[3] LunarG tutorial: Vulkan 1.1 changes / version query (vulkan.lunarg.com)
[4] vkCreateInstance error (VK_ERROR_INCOMPATIBLE_DRIVER) (registry.khronos.org)
[5] LunarG Loader/Driver Interface: loader adjusts apiVersion for 1.0-only drivers (vulkan.lunarg.com)
[6] LunarG Loader/Driver Interface: driver should not return incompatible-driver just due to apiVersion mismatch (vulkan.lunarg.com)
Citations:
- 1: https://registry.khronos.org/vulkan/specs/latest/man/html/VkApplicationInfo.html?utm_source=openai
- 2: https://registry.khronos.org/vulkan/specs/latest/man/html/vkEnumerateInstanceVersion.html?utm_source=openai
- 3: https://vulkan.lunarg.com/doc/view/latest/windows/tutorial/html/16-vulkan_1_1_changes.html?utm_source=openai
- 4: https://registry.khronos.org/vulkan/specs/latest/man/html/vkCreateInstance.html?utm_source=openai
- 5: https://vulkan.lunarg.com/doc/view/1.4.328.1/windows/LoaderDriverInterface.html?utm_source=openai
- 6: https://vulkan.lunarg.com/doc/view/1.4.328.1/windows/LoaderDriverInterface.html?utm_source=openai
Set apiVersion to VK_API_VERSION_1_0.
This test uses only Vulkan 1.0 core APIs (instance/device creation, queues, memory, command buffers, fences, and barriers). Requesting VK_API_VERSION_1_3 can cause vkCreateInstance to fail with VK_ERROR_INCOMPATIBLE_DRIVER on older Vulkan 1.0/1.1/1.2–only drivers. The current code silently returns on failure, creating false test skips on hardware that actually supports all the APIs being used.
Fix
- app_info.apiVersion = VK_API_VERSION_1_3;
+ app_info.apiVersion = VK_API_VERSION_1_0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/render/test_gpu_timestamp_pool.cpp` around lines 14 - 24, The test
requests Vulkan 1.3 which can cause vkCreateInstance to fail on older drivers;
update VkApplicationInfo.app_info.apiVersion to VK_API_VERSION_1_0 and change
the vkCreateInstance failure handling in the test (the block around
VkApplicationInfo app_info and vkCreateInstance) to report/propagate the error
instead of silently returning so test failures aren’t masked as skips.
| VkImageCreateInfo image_info{}; | ||
| image_info.sType = VK_STRUCTURE_TYPE_IMAGE_CREATE_INFO; | ||
| image_info.imageType = VK_IMAGE_TYPE_2D; | ||
| image_info.format = VK_FORMAT_B8G8R8A8_UNORM; | ||
| image_info.extent = {.width = extent.width, .height = extent.height, .depth = 1u}; | ||
| image_info.mipLevels = 1u; | ||
| image_info.arrayLayers = 1u; | ||
| image_info.samples = VK_SAMPLE_COUNT_1_BIT; | ||
| image_info.tiling = VK_IMAGE_TILING_OPTIMAL; | ||
| image_info.usage = VK_IMAGE_USAGE_SAMPLED_BIT | VK_IMAGE_USAGE_TRANSFER_SRC_BIT | | ||
| VK_IMAGE_USAGE_TRANSFER_DST_BIT | VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT; | ||
| image_info.sharingMode = VK_SHARING_MODE_EXCLUSIVE; | ||
| image_info.initialLayout = VK_IMAGE_LAYOUT_UNDEFINED; | ||
| if (vkCreateImage(device, &image_info, nullptr, &guard.image) != VK_SUCCESS) { | ||
| return std::nullopt; | ||
| } | ||
|
|
||
| VkMemoryRequirements requirements{}; | ||
| vkGetImageMemoryRequirements(device, guard.image, &requirements); | ||
| const auto memory_type = find_memory_type(physical_device, requirements.memoryTypeBits, | ||
| VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT); | ||
| if (!memory_type.has_value()) { | ||
| return std::nullopt; | ||
| } | ||
|
|
||
| VkMemoryAllocateInfo alloc_info{}; | ||
| alloc_info.sType = VK_STRUCTURE_TYPE_MEMORY_ALLOCATE_INFO; | ||
| alloc_info.allocationSize = requirements.size; | ||
| alloc_info.memoryTypeIndex = *memory_type; | ||
| if (vkAllocateMemory(device, &alloc_info, nullptr, &guard.memory) != VK_SUCCESS) { | ||
| return std::nullopt; | ||
| } | ||
| if (vkBindImageMemory(device, guard.image, guard.memory, 0u) != VK_SUCCESS) { | ||
| return std::nullopt; | ||
| } | ||
|
|
||
| VkImageViewCreateInfo view_info{}; | ||
| view_info.sType = VK_STRUCTURE_TYPE_IMAGE_VIEW_CREATE_INFO; | ||
| view_info.image = guard.image; | ||
| view_info.viewType = VK_IMAGE_VIEW_TYPE_2D; | ||
| view_info.format = VK_FORMAT_B8G8R8A8_UNORM; | ||
| view_info.subresourceRange.aspectMask = VK_IMAGE_ASPECT_COLOR_BIT; | ||
| view_info.subresourceRange.levelCount = 1u; | ||
| view_info.subresourceRange.layerCount = 1u; | ||
| if (vkCreateImageView(device, &view_info, nullptr, &guard.view) != VK_SUCCESS) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests/render/test_gpu_timestamp_pool.cpp | head -260 | tail -80Repository: goggles-dev/Goggles
Length of output: 3955
🏁 Script executed:
rg "vkGetPhysicalDeviceFormatProperties" --type cpp -B 2 -A 5Repository: goggles-dev/Goggles
Length of output: 45
🏁 Script executed:
rg "VK_FORMAT_" tests/render/ --type cpp | head -20Repository: goggles-dev/Goggles
Length of output: 1242
🏁 Script executed:
rg "formatProperties\|FormatProperties" --type cpp -B 2 -A 3Repository: goggles-dev/Goggles
Length of output: 45
🌐 Web query:
Vulkan VK_FORMAT_B8G8R8A8_UNORM support guarantee requirements
💡 Result:
-
As a regular image format (
VkFormatProperties):VK_FORMAT_B8G8R8A8_UNORMis part of Vulkan’s mandatory format support set (“Mandatory Format Support: 4 Byte-Sized Components”). That table specifies the minimumVkFormatFeatureFlagBitsan implementation must advertise for the format (notably inoptimalTilingFeatures, i.e.,VK_IMAGE_TILING_OPTIMAL). In other words, you can rely on it being supported with at least the required baseline feature bits listed in that table, and you query the exact bitmask viavkGetPhysicalDeviceFormatProperties. [1] -
As a presentable / swapchain format (
VkSurfaceFormatKHR): Vulkan does not guarantee that a surface will supportVK_FORMAT_B8G8R8A8_UNORM. The only hard guarantee is thatvkGetPhysicalDeviceSurfaceFormatsKHRreturns at least 1(format, colorSpace)pair, and none of them may haveformat = VK_FORMAT_UNDEFINED. You must enumerate and pick from what the surface reports. [2]
Sources: [1] Vulkan Spec – Formats chapter (Required/Mandatory format support tables) https://github.khronos.org/Vulkan-Site/spec/latest/chapters/formats.html, [2] Vulkan Refpage – vkGetPhysicalDeviceSurfaceFormatsKHR https://docs.vulkan.org/refpages/latest/refpages/source/vkGetPhysicalDeviceSurfaceFormatsKHR.html
🏁 Script executed:
rg "create_image" tests/render/test_gpu_timestamp_pool.cpp -B 2 -A 5Repository: goggles-dev/Goggles
Length of output: 1269
Probe format support before hard-coding VK_FORMAT_B8G8R8A8_UNORM.
While VK_FORMAT_B8G8R8A8_UNORM is a mandatory Vulkan format, the Vulkan spec only guarantees minimum feature support—not that all usage combinations (sampled, transfer source/destination, color attachment with optimal tiling) are supported on every device. Tests will fail on otherwise valid devices that don't support this exact format/usage combination, masking actual test failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/render/test_gpu_timestamp_pool.cpp` around lines 201 - 245, The test
currently hard-codes VK_FORMAT_B8G8R8A8_UNORM in image_info and view_info which
may not support the requested usage/tiling on all GPUs; before creating the
image (before using image_info and view_info and calling
vkCreateImage/vkCreateImageView), query format support using
vkGetPhysicalDeviceImageFormatProperties (or
vkGetPhysicalDeviceFormatProperties) for the desired VkImageTiling
(VK_IMAGE_TILING_OPTIMAL) and usage bits (VK_IMAGE_USAGE_SAMPLED_BIT |
VK_IMAGE_USAGE_TRANSFER_SRC_BIT | VK_IMAGE_USAGE_TRANSFER_DST_BIT |
VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT), choose a format that returns supported
properties, and assign that format to image_info.format and view_info.format
(with a sensible fallback or early skip/return if no format is supported). Also
ensure any memory/type selection (find_memory_type) and subsequent operations
use the selected format consistently.
| auto submit_and_wait(VkDevice device, VkQueue queue, VkCommandBuffer command_buffer) -> bool { | ||
| VkFenceCreateInfo fence_info{}; | ||
| fence_info.sType = VK_STRUCTURE_TYPE_FENCE_CREATE_INFO; | ||
| VkFence fence = VK_NULL_HANDLE; | ||
| if (vkCreateFence(device, &fence_info, nullptr, &fence) != VK_SUCCESS) { | ||
| return false; | ||
| } | ||
|
|
||
| VkSubmitInfo submit_info{}; | ||
| submit_info.sType = VK_STRUCTURE_TYPE_SUBMIT_INFO; | ||
| submit_info.commandBufferCount = 1u; | ||
| submit_info.pCommandBuffers = &command_buffer; | ||
| const bool ok = vkQueueSubmit(queue, 1u, &submit_info, fence) == VK_SUCCESS && | ||
| vkWaitForFences(device, 1u, &fence, VK_TRUE, UINT64_MAX) == VK_SUCCESS; | ||
| vkDestroyFence(device, fence, nullptr); | ||
| return ok; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the test file and examine the specific lines
find . -name "test_gpu_timestamp_pool.cpp" -type fRepository: goggles-dev/Goggles
Length of output: 105
🏁 Script executed:
#!/bin/bash
# Read the specific lines mentioned in the review (252-267)
sed -n '252,267p' ./tests/render/test_gpu_timestamp_pool.cppRepository: goggles-dev/Goggles
Length of output: 808
🏁 Script executed:
#!/bin/bash
# Check for other vkWaitForFences usage in the codebase to see patterns
rg "vkWaitForFences" --type cpp -B 2 -A 2Repository: goggles-dev/Goggles
Length of output: 1707
🏁 Script executed:
#!/bin/bash
# Check if there are any existing timeout constants or patterns in the test files
rg "timeout|Timeout|TIMEOUT|30\|seconds|Duration" tests/render/test_gpu_timestamp_pool.cpp -iRepository: goggles-dev/Goggles
Length of output: 148
🏁 Script executed:
#!/bin/bash
# Look for timeout constants or patterns in test files generally
rg "const.*=.*[0-9].*000" tests/ --type cpp | head -20Repository: goggles-dev/Goggles
Length of output: 497
🏁 Script executed:
#!/bin/bash
# Get more context around the submit_and_wait function to understand its usage
sed -n '240,275p' ./tests/render/test_gpu_timestamp_pool.cppRepository: goggles-dev/Goggles
Length of output: 1405
🏁 Script executed:
#!/bin/bash
# Find where submit_and_wait is called
rg "submit_and_wait" tests/render/test_gpu_timestamp_pool.cpp -B 2 -A 2Repository: goggles-dev/Goggles
Length of output: 736
Use a bounded fence wait in the test helper.
vkWaitForFences(..., UINT64_MAX) can hang the entire test job indefinitely if the GPU or driver wedges. A finite timeout makes this fail as a test error instead of stalling CI.
Suggested fix
+constexpr uint64_t kFenceWaitTimeoutNs = 30'000'000'000ULL;
+
auto submit_and_wait(VkDevice device, VkQueue queue, VkCommandBuffer command_buffer) -> bool {
VkFenceCreateInfo fence_info{};
fence_info.sType = VK_STRUCTURE_TYPE_FENCE_CREATE_INFO;
VkFence fence = VK_NULL_HANDLE;
if (vkCreateFence(device, &fence_info, nullptr, &fence) != VK_SUCCESS) {
return false;
}
VkSubmitInfo submit_info{};
submit_info.sType = VK_STRUCTURE_TYPE_SUBMIT_INFO;
submit_info.commandBufferCount = 1u;
submit_info.pCommandBuffers = &command_buffer;
- const bool ok = vkQueueSubmit(queue, 1u, &submit_info, fence) == VK_SUCCESS &&
- vkWaitForFences(device, 1u, &fence, VK_TRUE, UINT64_MAX) == VK_SUCCESS;
+ if (vkQueueSubmit(queue, 1u, &submit_info, fence) != VK_SUCCESS) {
+ vkDestroyFence(device, fence, nullptr);
+ return false;
+ }
+ const bool ok =
+ vkWaitForFences(device, 1u, &fence, VK_TRUE, kFenceWaitTimeoutNs) == VK_SUCCESS;
vkDestroyFence(device, fence, nullptr);
return ok;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/render/test_gpu_timestamp_pool.cpp` around lines 252 - 267, The helper
submit_and_wait currently uses an infinite wait via vkWaitForFences(...,
UINT64_MAX) which can hang CI; change it to use a bounded timeout (e.g. a
constant like kFenceTimeoutNs = 5'000'000'000ULL for 5s) and pass that instead
of UINT64_MAX to vkWaitForFences, treat a timeout or non-success return as a
test failure (return false), and keep the existing vkDestroyFence cleanup;
update any test expectations accordingly to rely on the limited wait.
1 participant
Summary by CodeRabbit
New Features
Tests
Refactor
Documentation