Repository navigation
fix(agc): compare host imports when device writes reach the write watch - #1046
aryanrawatispro-ship-it wants to merge 5 commits into
Conversation
github-actions
Bot
added
build
agc
|
Tested the PR head (7857d68) on Windows with real GPUs. Windows 11, MinGW-w64 GCC 15.2.0 (WinLibs r7),
No suite reported itself as not run on either driver. Tested with AI assistance (Claude Code). |
|
Tested the PR head (7857d68) on AMD and Intel, since the description notes it was not run on AMD hardware. Ubuntu 24.04.5, GCC 13.3, Release, Mesa 25.2.8; i3-12100 with RX 7600 (RADV) and UHD 730 (ANV).
#1210 fixes both in the test file. With it on top of this PR, the suite passes 3/3 on all three devices. |
|
I resolved this onto current 1. foreach(agcTest IN ITEMS ... agc_driver_graphics_tests agc_driver_bda_device_tests agc_driver_recorder_tests ${agcExecutionTests})2. 3.
state.unwatchImports = probe.writtenAfterSubmit != 0 || probe.writtenAfterFill != 0;
4. Verification on Lavapipe, CI environment ( That is the line the description predicts, and it is the failure from #1429 turning green. Real GPU (RTX 3080 Ti, NVIDIA 615.71.09) — no behaviour change: So the added term is inert on a device where imports stay watched, which is what you want from a probe. One thing worth knowing before this is registered. On the same GPU, on Verified against Written with AI assistance. |
|
The conflict list for this branch is stale — it now conflicts in 4 files / 6 hunks on current Refreshed onto That Apply it without redoing the resolution:
|
|
While reviewing #1532 against the current #1046 head, I found the same range mismatch in I made a local test-only patch that branches on the actual range's Since #1046 already changes and registers this same test, I have not opened a duplicate PR. Please consider folding this correction into #1046, or let me know if the |
|
I read the test at the current head (
The question stands for @aryanrawatispro-ship-it: if |
|
Third arm, measured on hardware — @MarcZX100's reading holds, and here is what it costs. Setup: a local branch = this PR's four files applied to current - const bool importWrites = probe.writtenAtImport != 0;
...
- if (importWrites) {
- Require(!UnchangedSince(address, surfaceBytes, cached), "(w) a cache over the pages the import reported sees no change");
+ if (!UnchangedSince(address, surfaceBytes, cached)) {
Three things this settles:
The negative control does still bite, elsewhere: with One more thing for the reviewer: this PR also registers No PR from me — the change belongs in this one. Written with AI assistance. |
|
RADV/ANV datapoint for the question at the end of @Dextroid17's last comment. Setup: this PR's head
So the import-window branch can't be reached on AMD (or Intel): a GPU read through a host import already dirties the pages, imports are compared, and the case returns before either gate is evaluated. Forcing |
7857d68 to
41f262b
Compare
|
Rebased onto
On the gate question from @MarcZX100 and @Dextroid17: Local results on the rebased head (WSL2, GCC 13.2, Release, lavapipe from Mesa 25.2.8):
I have no NVIDIA, AMD or Intel hardware here, so the NVIDIA result for the new gate is Dextroid17's, not mine. Written with AI assistance (Claude Code). |
41f262b to
0893905
Compare
|
Verified your head ( Both green, so the registration and the gating both hold on the CI device. One gotcha worth knowing, because I hit it myself: if a newly registered test fails in 0.00 s with no output at all, its binary was not built — I have not re-derived your Written with AI assistance. |
|
I checked the current PR head against upstream |
adriadam10
left a comment
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the work and for keeping this current. It conflicts with main in core/libs/prx/libSceAgcDriver/CMakeLists.txt and tools/check_test_registration.py; please rebase.
I rebased it locally onto bfef67f36 (the two conflicts are mechanical: keep main's list plus agc_driver_recorder_tests, and drop it from KNOWN_UNRUN). After that agc_driver_recorder fails, with CI's setup (lavapipe from Mesa 25.2.8, ANYPS5_REQUIRE_VULKAN=1, GALLIUM_OVERRIDE_CPU_CAPS=avx) and on RADV:
AGC graphics: the refresh's store of the results did not reach guest memory
It comes from core/libs/prx/libSceAgcDriver/tests/Recorder.cpp:1764 (memoryHolds). main has changed Recorder.cpp and Graphics/ since f2124645 (#2176, #1441, #2299, #856), so please check which of them this case now depends on, fix it, and rerun agc_driver_recorder on the new head. Registering the test in ctest means the Linux job goes red until it passes.
The import probe only checked whether a GPU read of an imported range reports its pages as written. On a device that runs its commands on the CPU (lavapipe), the device's own writes into an import go through the page tables and are collected as CPU stores. A unit shadow publish then marks its unit as written, and the next partial publish drops the slab's newer bytes as stale: agc_driver_recorder_tests stops at "the partial publish did not copy exactly the partly covered unit". The probe now also fills the scratch import on the GPU and collects again. When the fill reports pages as written, imported ranges are compared, as they are when a read does. lavapipe reports 0 of 4 pages after the read and 4 of 4 after the fill. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
agc_driver_recorder_tests was built and run by hand only. It is now registered with the other driver tests and skips with 77 when there is no usable Vulkan device, unless ANYPS5_REQUIRE_VULKAN is set. The draw snapshot reuse test assumed watched imports. With imports compared (amdgpu, lavapipe, or APS5_WRITE_WATCH_IMPORTS=unwatch), a draw copies its input every time, as intended, so the test reports itself as not run there, like the unit shadow test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
importWindowTests took its write checks when the import probe reported pages written at import time. The probe runs on a separate scratch mapping, so it does not show that this test's range changed. On NVIDIA the probe reports 4 of 4 pages while the range reads unchanged, and the case failed at "a cache over the pages the import reported sees no change" (boykopovar#1532). The case now branches on the range's own UnchangedSince after the collect. When the range changed, it checks the changed blocks and that the window does not read as a CPU store. Otherwise it checks only the pending results, as it did before when the probe reported no pages. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…mabuf importWatchTests required the dma-buf import probe to succeed whenever the device supports dma-buf import. The probe also needs /dev/udmabuf. GitHub's Linux runner has no such node, and many distributions create it as root:kvm 0660 (boykopovar#1688). The probe then fails at "open /dev/udmabuf", and the suite stopped there in CI. The driver already handles this case: such ranges are copied instead of imported. When the probe fails at that step, the case now reports the dma-buf probe as not tested, as it does when the device has no dma-buf import. Any other probe failure still fails the case. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… memory 0e1d25a (boykopovar#2096) compares an untracked import's bytes with the image's snapshot. When they still match, a refresh keeps the image's own results pending instead of storing them, as it already does over watched memory. storageRefreshTests (unwatched case) and staleGenerationTests read the guest bytes directly, so they expected the refresh itself to store the results, and stopped at "the refresh's store of the results did not reach guest memory" and "(s) results pending when their memory left the watch did not reach it". Both now read the memory through GuestMemory::Read, as the watched storage refresh case already does; its flush hook stores the pending results first. Results that were dropped still fail both checks. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
f212464 to
9579e62
Compare
|
@adriadam10 Rebased onto The failure comes from #2096 ( New commit On the new head, with CI's environment (lavapipe from Mesa 25.2.8,
I have no RADV device. The two changed checks take the same unwatched path on every device, but I have not run the rest of the suite on RADV on this head. Written with AI assistance (Claude Code). |
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
|
The import probe only checked whether a GPU read of an imported range reports its pages as written. lavapipe runs command buffers on CPU threads, so its writes into an import go through the page tables and are collected as CPU stores. In agc_unit_shadow the whole publish's copy into the import is then collected as a CPU store over unit 3, and the next partial publish drops the slab's newer bytes as stale: "the partial publish did not copy exactly the partly covered unit". The probe now also fills the scratch range on the GPU and collects again. When the read or the fill reports pages as written, host and dma-buf imports are compared. lavapipe reports 0 of 4 pages after the read and 4 of 4 after the fill; NVIDIA reports 0 after the fill. The probe change is the first commit of #1046.
7 participants
What
agc_driver_recorder_testsis built but never run in CI. On the Linux job's device (lavapipe) it fails onmainon every run, and the process then segfaults at exit:Cause. lavapipe runs command buffers on CPU threads, so its writes into a host import go through the page tables, and the write watch collects them as CPU stores. The probe that decides whether imports stay watched only checks a GPU read, which reports 0 of 4 pages on lavapipe, so imports stay watched. In
unitShadowTests, the whole publish's copy into the import is then collected as a CPU store over unit 3. After the unit is retiled again, the partial publish finds it written since the retile and drops the slab's newer bytes as stale:PublishShadowreturns 0 and the unit is no longer shadowed. Any write the driver records into a watched import on such a device reads the same way.Fixes #1429.
Five commits:
fix(agc): compare host imports when device writes reach the write watch:ProbeImportWriteProtectionalso fills the scratch import on the GPU and collects again (writtenAfterFill). When the read or the fill reports pages as written, imported ranges are compared, as on amdgpu. On lavapipe the startup line now reads:test(agc): run the recorder tests in ctest: registersagc_driver_recorderwith the other driver tests. It skips with 77 without a usable Vulkan device (the Windows job), unlessANYPS5_REQUIRE_VULKANis set (the Linux job). The draw snapshot reuse test (69a9420) assumed watched imports. With imports compared, a draw copies its input every time, as that commit intends, so the test now reports itself as not run there, like the unit shadow test. It also failed onmainwithAPS5_WRITE_WATCH_IMPORTS=unwatch, the amdgpu path.test(agc): gate the import window checks on the range's own writes:importWindowTeststook its write checks when the scratch import probe reported pages written at import time. That probe runs on a separate mapping. On NVIDIA it reports 4 of 4 pages while the test's range is unchanged, so the case failed (agc_driver_recorder_tests: on Linux NVIDIA a reported import write does not invalidate a cache, and the test has never run in CI #1532). The case now branches on the range's ownUnchangedSinceafter the collect. Suggested by @MarcZX100 and measured by @Dextroid17 (RTX 3080 Ti: exit 1 before, 0 after).test(agc): report the dma-buf import probe as not run without /dev/udmabuf:importWatchTestsrequired the dma-buf import probe (feat(agc): import shared direct memory through udmabuf on Linux #943) to succeed whenever the device supports dma-buf import. GitHub's Linux runner has no/dev/udmabuf, and many distributions create it asroot:kvm 0660(fix(agc): name the reason when /dev/udmabuf cannot be opened #1688), so the suite stopped in CI at(u) the dma-buf import probe failed at open /dev/udmabuf. When the probe fails at that step, the case now reports the dma-buf probe as not tested, as it does without dma-buf import. Any other probe failure still fails.test(agc): read storage results through the flush hook over unwatched memory: since fix(win): windows host import texture reuploads #2096 (0e1d25a), a refresh over an untracked import whose bytes still match the image's snapshot keeps the image's own results pending instead of storing them, as it already did over watched memory.storageRefreshTests(the unwatched case) andstaleGenerationTestsread the guest bytes directly and expected the refresh itself to store, so on currentmainthey stopped atthe refresh's store of the results did not reach guest memory(lavapipe and RADV, reported by @adriadam10) and then at(s) results pending when their memory left the watch did not reach it. Both now read the memory throughGuestMemory::Read, as the watched storage refresh case already did. Its flush hook stores the pending results first.No title needs this. It puts the suite that covers storage write-back, DCC keys, labels and unit shadows into CI, so every pull request that changes those paths runs it.
Tested
BUILD_TESTING=ON, lavapipe (Mesa 25.2.8).main, instrumented: the partial publish returns 0 and unit 3 is no longer shadowed. Right after the whole publish,UnchangedSinceCollected(unit3, unit, pre)is false: the publish's own copy was collected as a CPU write.5cb747c1, with CI's environment (VK_ICD_FILENAMESset to lavapipe,ANYPS5_REQUIRE_VULKAN=1,GALLIUM_OVERRIDE_CPU_CAPS=avx):agc_driver_recorderpasses (0.36 s), also underctest -R '^agc_driver_recorder$' --repeat until-fail:3and in 3 direct runs. WSL has no/dev/udmabuf, so the dma-buf probe reports itself as not tested, as on the CI runner. WithAPS5_WRITE_WATCH_IMPORTS=watch(the old lavapipe decision) it fails atthe partial publish did not copy exactly the partly covered unit. WithVK_ICD_FILENAMESpointing at no driver it skips (77); withANYPS5_REQUIRE_VULKAN=1as well it fails.cteston that head: 558 tests. The failures areguest_sce_netandnid_patcher_dynamic_strings(the resolver test; the local toolchain adds its own RUNPATH), as on earlier bases, andguest_video_out_resolution_status, whose window gets no Vulkan from this machine's SDL offscreen driver; the video-out flip tests skip here for the same reason.tools/check_test_registration.py buildpasses withKNOWN_UNRUNempty, andtools/check_conventions.pypasses.dc4b7104without the fifth commit. It is in the firststorageRefreshTestscall, whose block comes fromaligned_allocand is never watched, so this PR's import decision does not reach it; RADV compares imports with or without this PR. With only fix(win): windows host import texture reuploads #2096's comparison switched off (compareUntrackedreturning false), the suite passes there without the fifth commit.the refresh's store of the results did not reach guest memory; with the earlier cases that catch it skipped, it fails at(s) results pending when their memory left the watch did not reach it. When results dead under new 0001 keys are kept, it fails atnew 0001 clear keys did not clear the image.APS5_WRITE_WATCH_IMPORTS=watch(the old lavapipe decision) differs only in the recorder suite. There, draw snapshot reuse, unit shadows and the watched storage refresh report themselves as not run on lavapipe.mainwith test(agc): run the recorder tests on AMD and Intel GPUs #1210): RX 7600 (RADV), UHD 730 (ANV) and llvmpipe pass.writtenAfterFillis 0). It passes with the third commit.agc_driver_*rule.main. That path is not touched here.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)🤖 Generated with Claude Code