Repository navigation
fix(agc): destroy settle aliases before their imports are freed - #7629
Open
EagleBearerYS wants to merge 3 commits into
Open
EagleBearerYS wants to merge 3 commits into
EagleBearerYS wants to merge 3 commits into
Conversation
On Windows a host import under 1 MiB is settled by a batched 4-byte read. When the settle queue is in another queue family than the import's, as on AMD's Windows driver with its single graphics queue, the batch reads through an alias buffer bound to the import's memory. Reaping the batch destroys those aliases before it drops the imports retired while the batch held them. An unmap that needs such an import (releaseSettleRetired) waited for the batch and then released the import, destroying its buffer and freeing its memory while the alias was still bound to that memory; the alias went at the next reap. Destroy the batch's aliases first, as reapSettleBatches and drainSettles do.
forgetSettle handed back an import's settle batch only while the batch was still running. A batch the GPU had finished but that had not been reaped yet still held its aliases, so retireImport released the import at once, freeing its memory before the alias bound to it was destroyed. Hand back any busy batch. The import then stays with its batch until the batch is reaped or an unmap releases it, and both destroy the aliases first. A finished batch now keeps its imports' Vulkan objects until it is reaped (boykopovar#7601).
agc_driver_host_import_lifetime takes the settle alias path only on a device whose graphics family has a single queue, such as AMD's Windows driver; elsewhere the settles share the graphics family and make no alias. agc_driver_host_import_lifetime_other_family runs the same tests with the settles forced onto another queue family, as agc_import_settle_deferred_other_family does, so a device with spare graphics queues covers that path too. It is registered on Windows and skips where the device has no other queue family.
|
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.
|
EagleBearerYS
marked this pull request as ready for review
October 18, 2026 18:17
MarcZX10
approved these changes
Oct 18, 2026
MarcZX10
left a comment
MarcZX10
left a comment
Contributor
There was a problem hiding this comment.
Reviewed the lifetime paths and the diff. Keeping any busy settle batch with its retired imports covers the completed-but-not-yet-reaped case, and releasing the aliases before dropping overlapping retired imports covers partial unmap. The other-family Windows test exercises the alias path on devices that otherwise settle on the graphics family. I did not run the Windows GPU tests locally; the reported 860M and RTX 3070 results provide the hardware validation for this review.
Contributor
|
Rechecked the PR head The 11 AI-assisted |
This was referenced Oct 18, 2026
github-actions
Bot
added
build
agc
This branch has not been deployed
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
3 participants
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
A regression from our #6678. On Windows,
agc_driver_host_import_lifetime(from #7339) fails on unchangedmainon an AMD Radeon 860M: "host import Vulkan objects were destroyed out of order or more than once", at its first partial unmap. @LeandroLP23 reported it in #7601 and traced it there: the buffer the test tracks isn't the import's own but an alias bound to the import's memory, and two routes free that memory before the alias is destroyed. The second commit is LeandroLP23's own fix for one of them, as they posted it in #7601, with their authorship.How it happens (
main889cee5,GuestBufferMemory.cpp, Windows only):VulkanDevice.cpp~1473-1499), as on AMD's Windows driver. A device with spare graphics queues settles in the same family and makes no alias.retiredlist, and the reap destroys the aliases before it drops that list. Two routes skip that order:forgetSettle(~256) hands the batch back only while the GPU is still running it. For a batch that has finished but hasn't been reaped yet,retireImportreleases the import at once: its buffer is destroyed and its memory freed, and the alias goes at the next reap. LeandroLP23 found this route.releaseSettleRetired, ~376-395) waits for the batch, then releases the import with the alias still bound.vkBindBufferMemoryhook keeps the last buffer bound to an import's memory, which is the alias. SopartialUnmapsees the memory freed before that buffer is destroyed: AGC: agc_driver_host_import_lifetime fails on Windows (Radeon 860M) in partialUnmap: the import's memory is freed before its buffer is seen destroyed #7601'sdestroyed=0 outOfOrder=1 frees=1.As @MarcZX10 points out in #7601, Vulkan permits freeing memory while resources are still bound to it, once the commands that use them have completed. Both routes run after the batch has finished and nothing uses the alias afterwards, so this isn't a GPU hazard. LeandroLP23's trace there shows the sequence: the import's own buffer is destroyed, the memory is freed while the fixture tracks the alias (a new buffer reusing an earlier handle value), and the alias is destroyed right after, at the reap.
A fixture that tracked only the import's own buffer would pass as well, but it would no longer check the order for the alias. This change keeps the test as it is and makes both routes keep the order the reap and drain paths already keep. Which of the two to take is the maintainers' call.
The change, in three commits:
releaseSettleRetireddestroys a finished batch's aliases before it releases the imports the batch held.forgetSettlehands back any busy batch, not only a running one. An import retired after its read finished then stays with its batch until the batch is reaped or an unmap releases it, and both destroy the aliases first.agc_driver_host_import_lifetime_other_familyruns the lifetime tests with the settles forced onto another queue family (settleOnOtherFamily, asagc_import_settle_deferred_other_familydoes), so a GPU with spare graphics queues takes the alias path too. It's registered on Windows and skips where the device has no other queue family.Side effects:
[buffers]host imports line (APS5_PROFILE_DRAW), "retired during their read" now also counts imports retired after their read finished but before its batch was reaped. "unmaps waited for it" still counts only reads that were running.What stays as it is: devices that settle in the import's own family, which make no alias, and Linux, which has no settle batches.
Fixes #7601
Tested
main6efb726 with this branch's tree: thehost_importtests pass 5 runs of 5. With only the first commit, both lifetime tests fail 3 of 3; with only the second,agc_driver_host_import_lifetimefails 3 of 3. Their fullagc_*run passes 715 of 717; the 2 failures fail on unchangedmainthere too (fix(shader): kill demoted quad-mask pixels when the pixel program returns #6255, agc_* tests: 3 of 483 fail on AMD Radeon RX 6400, pass on NVIDIA RTX 4080 SUPER #5271).agc_driver_host_import_lifetimeandagc_driver_host_import_lifetime_other_familypass 3 of 3 each. The forced-family test ran on the device every time, with nothing skipped.main'sGuestBufferMemory.cppand the same tests, the forced-family test fails on its first run with AGC: agc_driver_host_import_lifetime fails on Windows (Radeon 860M) in partialUnmap: the import's memory is freed before its buffer is seen destroyed #7601's message. The plain test passes there, since this device settles in the import's own family.agc_driver_host_import_lifetimepasses 3 of 3.main'sGuestBufferMemory.cppit fails with AGC: agc_driver_host_import_lifetime fails on Windows (Radeon 860M) in partialUnmap: the import's memory is freed before its buffer is seen destroyed #7601's message, as on the 860M.main. libc'sprecise_waitfails on Linux withmain's code too.Checklist
main889cee5, and merges cleanly into currentmain; 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)