Repository navigation
fix(agc): wait for an image another thread is storing before a host read - #7769
Merged
oneandonlydean merged 2 commits intoOct 19, 2026
Merged
oneandonlydean merged 2 commits into
oneandonlydean merged 2 commits into
Conversation
FlushPending moves the overlapping images from the pending list to the flushing list and stores them under the GPU mutex. A FlushPending or a flush hook on another thread for the same range found nothing pending and returned while the store was still running, so the host read saw guest memory before the store; the queue-idle store hits this with any host read. When an overlapping image is being stored and this thread does not hold the GPU mutex, take it and check again until no overlapping image is left being stored, and report the range as stored so the hook syncs the recorded store. Fixes boykopovar#7744
|
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.
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.
|
Contributor
|
A Windows run, since the description lists Windows as not tested: RX 6700 XT (gfx1031, RDNA 2), AMD driver 32.0.21045.5002, Windows 11 26200, WinLibs GCC 15.2.0 r7, Release.
So the race reproduces on Windows with AMD's driver, and the PR closes it. The runs are also shorter with the PR: about 1.1 to 1.6 s each, against 4.9 to 6.7 s on With the PR, and every related test executable rebuilt against it, the 33 tests matching |
This was referenced Oct 19, 2026
12 tasks done
Add the concurrent flush case from boykopovar#7757 to agc_driver_storage_file_write: while one thread's FlushPending stores the surface under a held GPU mutex, a second thread calls FlushPending and then GuestMemory::Read on the same range, and the read must see the stored bytes. Only the test from boykopovar#7757 is taken here; agc_driver_storage_read_during_idle_store keeps covering the flush hook path. Co-authored-by: MarcZX10 <67475268+MarcZX10@users.noreply.github.com>
Contributor
Author
|
Thanks @oneandonlydean for reproducing it on the 4090. I added the concurrent Head
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine. |
Contributor
|
The new head
|
12 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fixes the race reported in #7744.
StorageTexture::FlushPendingmoves the overlapping images frompending.texturestopending.flushing, then stores them under the GPU mutex. A host read of the same range on another thread finds nothing intexturesand returns without waiting, so it reads guest memory before the store lands. The queue-idle store (StoreIdleResults: GPU mutex, thenFlushAllPending("idle")) is the usual other thread.StorageTexture::WaitForFlushing(address, bytes, except). When an image influshingoverlaps the range and this thread does not hold the GPU mutex, it takes the GPU mutex, releases it, and checks again until no overlapping image is left influshing. It returns whether it waited.Recorder::SyncThrough. It also releases the mutex before it erases its images fromflushing.FlushPendingcalls it when it lists no image and returnstrueif it waited. That return is what tells the flush hook to sync the recorded store. Without it, the read still saw zeros after the wait.FlushForAccess) calls it on its C4 path, which skipsFlushPendingwhenAccessKeptByCpulists no pending image. That is the path a file write's host read takes by default. A fix inFlushPendingalone left the repro at 200 of 200.flushingis empty almost always. An atomicflushingCount, written under the registry mutex next to each change offlushing, keeps the hot path to one atomic load.Lock order:
WaitForFlushingnever takes the GPU mutex while it holds the registry mutex.StoreIdleResults,FlushAllPendingand theUnregisterofFlushPendingtake the GPU mutex first and the registry mutex second, so the order stays the same. A thread that already holds the GPU mutex does not wait: the other writer may be blocked on that mutex. Readers on the hook path already take the GPU mutex (AccessKeptByCpu, the recorder sync), so the wait adds no new lock order.agc_driver_storage_read_during_idle_storeis a new test that reproduces #7744. A thread takes the GPU mutex and runsFlushAllPending. The main thread waits untilFindPendingreturns null for the surface, so the image is influshing, and then callsGuestMemory::Read. This runs for 20 rounds, and the test fails if any read sees the bytes from before the store. It covers the flush hook path, which a plainGuestMemory::Readtakes.agc_driver_storage_file_writealso gets the concurrent flush case from #7757, by @MarcZX10 (his test only, with co-authorship in the commit). One thread'sFlushPendingstores the surface while another thread holds the GPU mutex, and a second thread callsFlushPendingand thenGuestMemory::Readon the same range. That covers a reader that callsFlushPendingitself. Folded in at @oneandonlydean's suggestion in #7757.Tested
Current head
ba72cb78fmerged ontomain118266aec(clean), same machine and drivers as below,ANYPS5_NO_SHADER_CACHE=1:main+ only the two test filesagc_driver_storage_file_write, lavapipeconcurrent guest read returned before an overlapping image was storedagc_driver_storage_file_write, RTX 3070agc_driver_storage_read_during_idle_store, lavapipeagc_driver_storage_read_during_idle_store, RTX 3070ctest -R "^agc"(-j4) onmain118266aec: lavapipe 787 of 787, RTX 3070 782 of 782 (withoutagc_display_buffer_dcc,agc_driver_flipandagc_driver_lds_atomics64). With this change: 788 of 788 and 783 of 783, the same skips.agc_driver_bda_devicenow passes onmainhere too.check_conventions.py --base origin/mainandcheck_test_registration.pypass.Earlier runs, before the #7757 test was added:
Intel i7-8700, Ubuntu 26.04, GCC 15.2, Release,
BUILD_TESTING=ON, this branch onmain63e4a6380. Earlier runs onmain9f5d4bd92gave the same results. lavapipe is Mesa 26.0.3 withGALLIUM_OVERRIDE_CPU_CAPS=avx.mainagc_driver_storage_file_write"wrote only zeros", lavapipe,SIGSTOP70 ms every 20 ms, 100 runsAPS5_HOOK_FLUSH_CPU_BLOCKS=0, where the hook goes throughFlushPending.agc_driver_storage_file_writefailures underSIGSTOPstill happen. They come from the test's assumption that the images stay pending (for examplean unrelated pending GPU image was flushed), which test(agc): keep the queue-idle store out of the file write cases #7745 addressed (merged).ctest -R "^agc"(-j4) has the same results asmainapart from the new test:agc_display_buffer_dcc,agc_driver_flipandagc_driver_lds_atomics64. The one failure isagc_driver_bda_device, which fails onmaintoo (agc_driver_bda_device times out in vkWaitForFences since the BDA anchor lookup (#5859, d422c760c) #7584).python3 tools/check_conventions.py --base origin/mainandtools/check_test_registration.pypass.pr_overlap.pyreports no line conflicts.Not tested: Windows. The change uses only
GpuMutex,std::atomicandstd::this_thread::yield.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)🤖 Generated with Claude Code