Repository navigation
feat(agc): enable VK_NV_device_diagnostics_config behind APS5_NV_DIAGNOSTICS - #9934
Conversation
|
I reviewed the feature-query and device-create chains, the extension enablement, and the helper tests. When the opt-in is absent, this leaves device creation unchanged; when requested, it enables the diagnostics feature/config only when supported and adds diagnostic checkpoints only when that extension is available (preserving the existing I ran |
|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
|
I reviewed exact head |
|
Additional exact-head test evidence: I compiled and ran the submitted |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Update after the later review at 2026-10-22 23:58:57 UTC: I refreshed the exact PR head and reran the current-main convention and whitespace checks. At current upstream main 0db47b263d1019043f7d28fad4e7ee0a1ea5ec6a, git diff --check upstream/main...refs/review/9934 and python3 -I tools/check_conventions.py --base upstream/main --head refs/review/9934 pass. The PR is still based at 1a745ae38ea6a6c38949144ffb09ff962a9c1f1d (74 commits behind), and 7920 remains open, sharing VulkanDevice.cpp and ENVIRONMENT.md; its current implementation adds AMD buffer-marker diagnostics. The substantial intervening VulkanDevice changes also remove/sample-gate several feature-chain members, so the final rebased chain needs another exact review. Your RTX 3080 and lavapipe A/B evidence is valuable, but after refreshing please rerun the focused validation-layer baseline/head tests on the final head and resolve the overlap/checklist (Depends on if there is an agreed ordering). Hosted policy checks pass; product jobs are skipped. I did not run the Vulkan tests locally in this session.
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Please refresh this AGC change onto current main and resolve its overlap with open PR #7920, which also edits VulkanDevice.cpp and ENVIRONMENT.md to add the AMD buffer-marker diagnostics path. The overlap detector could not compare many shared VulkanDevice versions, so the checklist claim that no open PR implements overlapping functions needs an explicit coordinated order or explanation. After refreshing, rerun the repository-required focused ctest -V A/B on the same GPU for main and the final head with VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation, ANYPS5_NO_SHADER_CACHE=1, and ANYPS5_REQUIRE_VULKAN=1; confirm the layer loaded and compare VUIDs. Existing RTX 3080/lavapipe test evidence is useful but predates current main. The current-main whitespace and conventions checks pass; this request is for the required refreshed shared-chain/overlap review and final-head GPU evidence.
…NOSTICS A device loss on NVIDIA reports only VK_ERROR_DEVICE_LOST, and VK_EXT_device_fault often gives no address. Nsight Aftermath can write a crash dump that names the faulting shader and the kind of fault, but only when the device was created with VK_NV_device_diagnostics_config. APS5_NV_DIAGNOSTICS (presence) now creates the device with that extension (shader debug info, resource tracking, automatic checkpoints, shader error reporting) and with VK_NV_device_diagnostic_checkpoints when the device supports them. Unset, or on a device without the extension, the device is created as before; the latter prints a notice. ChooseDeviceDiagnostics holds the choice so the test can check it. AI-assisted: yes
460d79e to
c77548f
Compare
|
@MarcZX10, I've refreshed this onto current I reran the validation A/B on main and the final head, RTX 3080 Laptop, driver 610.62, with On #7920 (@hugodearaujo's AMD buffer markers): the two merge cleanly in either order, and a local merge of both builds and passes |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact current-main head c77548f5f109a71894605d9d52f5cb624103b42d. The change is opt-in: absent APS5_NV_DIAGNOSTICS, device creation follows the existing chain; when set, it queries and enables VK_NV_device_diagnostics_config only when supported, adds the diagnostic checkpoint extension only when available, and preserves APS5_GPU_CHECKPOINTS behavior. The refreshed test matrix covers supported/unsupported combinations. The author reports repeated Windows unit/env tests and validation-layer A/B on NVIDIA, plus focused Linux checks on lavapipe; I did not run GPU tests locally. Local diff-check, conventions, and current-main sync pass (1 ahead/0 behind). The author reports #7920’s AMD checkpoint change combines and tests cleanly; this review found no remaining blocking overlap. Product build jobs are skipped by path policy. Approval is for this exact head.
What
On NVIDIA a GPU hang or fault usually surfaces only as
VK_ERROR_DEVICE_LOST. In our runsVK_EXT_device_fault(#5453) added no fault address. Nsight Aftermath can write a crash dump with the device state, the faulting engine and the shader. But it does that only for a Vulkan device created withVK_NV_device_diagnostics_config, and our driver never asked for it.APS5_NV_DIAGNOSTICS(presence) creates the device withVK_NV_device_diagnostics_configand turns on shader debug info, resource tracking, automatic checkpoints and shader error reporting. It also enablesVK_NV_device_diagnostic_checkpointswhere the device supports it;APS5_GPU_CHECKPOINTSstill decides whether markers are recorded.ChooseDeviceDiagnostics(inDeviceFault.hpp, next to the fault report) makes the choice, so a unit test can check it.ENVIRONMENT.mdhas a row for the variable.Overlap: #7920 adds
VK_AMD_buffer_markercheckpoints in the same device setup,DeviceFault.hpp, its test andENVIRONMENT.md. The two touch different lines and merge cleanly in either order; on the combinationagc_device_faultpasses.checkpointskeeps its meaning there (the NV extension in use), so neither needs the other first.Why we wanted it: in Ghost of Yōtei a stop that showed up only as a device loss (
result 0) looked like a GPU hang. With this on, Aftermath dumped it asError_DMA_PageFault, a transfer write to address 0, which sent us down a different path.Tested
-DBUILD_TESTING=ON, now based onmain0db47b2:agc_device_fault,env_reference,conventions_env_reference3/3 with--repeat until-fail:3. The full ctest ran on the earlier base 1a745ae: 1898/1904, the 6 failures (agc_driver_sample_order*,log_macros*,guest_kqueue) also onmain.Diagnosticscase inagc_device_fault: nothing chosen without the variable or device support, otherwise the four flags plus checkpoints when supported. It doesn't build onmain.APS5_NV_DIAGNOSTICS=1,VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation,ANYPS5_NO_SHADER_CACHE=1,ANYPS5_REQUIRE_VULKAN=1,ctest -V),agc_device_faultand the packed color target tests,main0db47b2 vs this head: 3/3 each, the loader inserted the layer in both runs, nowas not foundline, noVUID-or warning; the head logs the extension as enabled.main0db47b2: the 5 focused tests pass; the full job before the refresh: 1832/1852, the same 20 failing onmainthere.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)