Repository navigation
test(agc): fix two stale expectations in the full recorder run - #3103
Conversation
|
Thank you for your interest in the project! We are sorry, but we cannot accept more PRs while we work through the existing backlog. This PR has been automatically closed and labeled "intake paused". This is not a judgment of the quality of your changes. |
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the follow-up. I ran the full agc_driver_recorder_tests (no arguments, RADV) with this PR merged onto current main: it exits 1 on (s) the storage image imported its memory before a generation was taken, before reaching either step this PR changes. That matches your note on #1532 (it started with #2699), so on main the PR's two fixes can't be shown to work.
- The
(s)stop after #2699 needs to be settled first, either by taking the generation before constructing the image instaleGenerationTests, or by deciding that the early import is a problem for what(s)guards. It can go in this PR or a separate one. - Then please show the full run exiting 0 on current
mainwith the PR, and update theTestedtable. Fixes #1532should stay only if the full run is green onmain; the issue also covers the test never running in CI.
The two changes themselves (explicit flush in (s), cached taken before MarkDirty() in (w)) look reasonable.
|
Additional platform validation of standalone PR head Windows: passed — Windows 11, WinLibs GCC 15.2.0 UCRT/POSIX/SEH r7; CMake 4.3.1 / Ninja; Release, 1 checks, 0 failures, 0 CTest skips. Built the selected targets and dependencies; this is focused validation, not a full build or full-suite run. Checks: Command after building: This scope checks CPU-side behavior; no GPU execution result is claimed. passed full recorder executable No title/gameplay validation. These results cover the standalone head, not a merge with current main. |
|
Windows run, Windows 11 (10.0.26200), WinLibs GCC 15.2.0 r7, Release, with
|
1c7ff0b to
433302b
Compare
|
Done in 433302b, and the branch is rebased onto The early import is the fast-clear path of The description has the new table and two driver mutations that turn each step red. I changed |
adriadam10
left a comment
There was a problem hiding this comment.
Checked the full agc_driver_recorder_tests run (no arguments, ANYPS5_REQUIRE_VULKAN=1, RADV) with this PR merged onto current main: exits 0. The same run with main's Recorder.cpp stops at (s) the storage image imported its memory before a generation was taken. The test-only diff fits the StorageTexture::upload fast-clear import from #2699; merging once CI is green.
What
Test-only changes in
tests/Recorder.cppso that a fullagc_driver_recorder_testsrun (no arguments, which ctest never does) gets to the end again on currentmain.staleGenerationTests, step (s): after theRefresh()over untracked memory the image is still pending, so the check now flushes it explicitly first, the way the unwatched branch ofstorageRefreshTestsalready does.importWindowTests, step (w):image->MarkDirty()already creates the import, socachedwas taken after the pages were reported andUnchangedSincestayed true.Both steps, since #2699:
StorageTexture::uploadnow callsHostImportForon the fast-clear path (Texture.cpp:1940), so constructing the image imports the surface and takes the range off the watch. The image still collects its generation before that import (Texture.cpp:1915), which is what the steps guard, so only their precondition was stale. (s) now requires the block to be watched before the image is constructed and the image to hold a generation afterwards. (w) takescachedand checks!HostImportCoversbefore the constructor, and checks that the image's generation predates the import window.Refs #1532 (the full run is still not registered with ctest, so the issue stays open with this).
Tested
Arch Linux, GCC 16.2.1, Release, RTX 5080 (NVIDIA 615.78.08) and Radeon 610M (RADV),
ANYPS5_REQUIRE_VULKAN=1, full run with no arguments. The branch is rebased ontomain65a4e8e.main2bb78d2 + the first commit only(s) the storage image imported its memory before a generation was taken(w) the storage image imported its memory before a generation was takenmain65a4e8e)Recorder read tracking and label tests passedMutations on the driver, each run on both devices: counting an image with a generation as tracked in
Texture.cpp:1564turns (s) red (the image is not pending after the refresh); forcingwritten = trueinGuestMemory.cpp:945turns (w) red (the pages the import reported read as written by the CPU). Removing the import atTexture.cpp:1940leaves the run green. The[gpu] lease wait failed ... Vulkan result -4line in the old logs came from the image being destroyed with its clear recorded when theRequirethrew; it is in no passing log.Not shown: no mutation targets the "results pending over the import window" drop path of (w), whose window now falls inside the constructor. Not tested on Windows by me (both steps are skipped there).
Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt.mdfiles or images added to the repository (attach them to this PR)