Repository navigation
fix(agc): keep guest stores over freed storage-image memory - #6960
romainveurrier wants to merge 1 commit into
Conversation
22aa5c7 to
36df663
Compare
|
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.
|
Overlap check:
This PR is a draft until the #5934 reporter tests it in-game, so the others can go first; I'll rebase. Depends on: none. |
CnRJay
left a comment
CnRJay
left a comment
There was a problem hiding this comment.
The bug is real and the two tests show it (main loses 4-21 of 96 stores, this PR 0). The cost is the problem, and the PR already measures it.
"Results in compared memory that were never written back before are now stored at every label": 2.7 to 18.6 ms per frame (RTX 2050, import path) and 2.6 to 32.3 ms (CPU path) for 4 MiB images, 4.6 to 21.4 / 4.5 to 33.9 ms on the Iris Xe, 0.22 to 1.29 ms for 256 KiB images. That is a GPU-to-CPU round trip of every pending storage image at every label, on by default, for every title on every Windows host import. A title that writes a few large GPU-only render targets and has a label per frame goes from 60 fps to about 30 for this fix alone. "No title measured" does not make that acceptable, and storing at every label is the wrong approach: the SONIC SUPERSTARS case is a 32x32 RGBA8 image in a 64 KiB block, and the large images never needed this.
What I would do instead:
- Bound the label store to the surfaces the existing small-store rule covers (
StorageTexture::SmallStoreBytes, 64 KiB), which is the class that shares a block with something a title allocates over. Large render targets stay on the current path. - Or gate the new store on the image's results having been read back before (the image is known to be CPU-visible), so GPU-only targets are never touched.
- Whichever is chosen, the per-frame cost on a title with several large GPU-only targets and a label per frame belongs in the test, as a bound, not in the description as a known limit.
Please also post the cost on the title this was found on (SONIC SUPERSTARS), with the summary lines of main and this PR from the same scene, no APS5_PROFILE_*. The report asks the reporter to test the branch; until they have, the PR can't show that the crash is gone or what it costs.
#6948 stores linear storage images after every dispatch for a similar reason and touches the same write-back path; the two should be reconciled so that a label does not store the same image twice.
|
AMD / Windows run of this PR at 36df663: Radeon 780M (RDNA 3, 0x15bf), AMD driver 25.10.20.01 (32.0.21020.1007), Windows 11 Home 10.0.26100, WinLibs GCC 15.2.0 r7, Release.
AI-assisted: yes (runs made on the device above). |
|
Data point from Linux, AMD Radeon RX 9070 XT, RADV (Mesa 26.2.5): EA SPORTS FC 25 (PPSA20049),
Few runs, so this is a hint, not proof. |
github-actions
Bot
added
the
unblocks title
|
I am investigating issue #8152, where a CPU write to the right half of a resident linear target shares 64 KiB blocks with a recorded draw to the left. I added a local regression variant that seeds the left half, draws only there, writes Does #6960's GPU-direct imported write-back path cover that partial-draw case? The current-main block selection can retain a whole touched block, preserving the CPU bytes while dropping GPU output elsewhere in that block. I want to coordinate this with your write-back changes before opening a related PR. |
This branch has not been deployed
5 participants
What
SONIC SUPERSTARS crashes in its allocator after respawns: the free-list links at the start of a freed 64 KiB block hold
0x00ffffff00ffffff, the texels of a 32x32 RGBA8 R64KBX storage image at that address (#5934). The image's results stay on the GPU past the label after its dispatch and are stored later, when the title may have freed the block. Two cases lose the title's stores:decideImportWatch), a CPU store is found only by comparing with the bytes saved at upload. A free that writes the links the block already held reads as no store, and the results are stored over it. Deterministic.writeBackLayerschooses the 64 KiB blocks the CPU owns, then stores the others after a GPU round trip (CPU path) or when the recorded copy runs (import path). A CPU store landing in between is overwritten. On the CPU path the bytes the store compares against were also read after the decision.Changes:
GuestMemory::WriteChangedOverOriginal[Committed]: each changed 8-byte word is stored by compare-exchange against the bytes the decision used, now read before the decision. A refused range leaves its layers stale, so they re-upload.StorageTexture::StoreBeforeGuestCompletion, called fromWriteLabelOnGpuand for end-of-pipe interrupts without a label: pending results over compared memory are stored in queue order before the label.APS5_DEFER_COMPARED_RESULTS=1andAPS5_RECORDED_WRITEBACK=1restore the previous behaviour of each part, for comparison.New tests, through
AgcDriver::Submit(PM4 dispatch, then a RELEASE_MEM label the test waits for, then CPU stores; no hooks):StorageWriteBackWindowR64Kbx: links stored at random times around a driver read that triggers the write-back, in compared and in watched memory.StorageSameBytesReuseR64Kbx: the block freed with the links it held at upload.Not addressed:
unchangedSinceBaseline(dfe147f) stores over a stamped block equal to its baseline, a rule for stamps on unchanged bytes (PPSA12544).APS5_TEST_WATCHED_SAME_BYTES=1runs that case; it fails before and after this change.main-> this PR: RTX 2050 2.7 -> 18.6 (import) and 2.6 -> 32.3 (CPU path), Iris Xe 4.6 -> 21.4 (import) and 4.5 -> 33.9 (CPU path); for 256 KiB images RTX 2050 0.22 -> 1.29 (import). Watched memory is unaffected. No title measured.Tested
Fedora 44, Release build of
mainat 4986aca with and without this change, tests only (no title run).StorageWriteBackWindowR64Kbx, links lost of 96 attempts per memory kind:main4-21 per GPU, path and memory kind; this PR 0 in every case. In watched memory 16–29 attempts per run had the compare-exchange keep a store made after the decision.StorageSameBytesReuseR64Kbx:main8/8 lost; this PR 0/8.APS5_NO_HOST_IMPORT=1(CPU path) and with the host import.main129 passed per GPU, this PR 131. The two new tests account for the difference. The one remaining failure fails onmaintoo:agc_driver_store_over_untrackedon the RTX 2050 ("the result did not reach a block the CPU left alone") andagc_driver_sample_multisample_targeton the Iris Xe.Investigation report and logs attached.
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)