Repository navigation
feat(agc): reuse a native multisample colour target and honour guest HTILE clears - #6808
KingDarkHN wants to merge 5 commits into
Conversation
…, and Z ones whose HTILE is cleared
…HTILE fills A 2, 4 or 8 sample D32_SFLOAT_S8_UINT target with its stencil in HTILE is imported natively when every HTILE word proves both planes cleared: the depth range as before, and SMem 0 for the stencil. The stencil plane joins the watched, guarded and generation-checked ranges. A depth-only HTILE, an unaligned stencil plane, or a word failing either proof is refused with its own reason. A fill covering the whole HTILE of a sampled native target is a clear transition, proven at the next sample like the HTILE mask clear: complete tracked stores since the fill, no CPU or queued writes, and every word proving the depth and, with a stencil plane, the stencil clear value. A partial fill and an HTILE mask clear of a stencil target stay refused.
…attern A whole HTILE fill recorded on the GPU is still pending when the title binds or samples the depth in the same frame, so the HTILE words in guest memory are not the fill's yet and the queued write was refused as an external GPU write. The fill's write note is kept with its pattern: while it is still the newest queued write over the HTILE, the clear is proven from the pattern, and the completed tracked store check is not needed for a GPU fill. A later queued write, a CPU write or a pattern that does not prove the clear is refused as before. AI-assisted: yes
28a9399 to
ca565ed
Compare
|
I compared this head with open #6727 because both replace |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
The updated head still has this correctness gap at RequireNoResidentDepthRead_nid_no_patch: once an overlapping guard is inactive as an attachment, it retires the owning surface without checking whether the host image contains writes that have not been published to guest memory. retire() only marks the owner inactive and erases the guards; it does not copy the host depth/HTILE contents back. An inactive owner can therefore still have newer host data, so a guest read after reuse can observe stale bytes. The related open #6727 tracks hostWritten and refuses this transition for a written owner. Please preserve the same refusal (or provide a path that publishes the written data first), and add a regression that writes the depth target on the host, makes it inactive, then attempts the guest reuse read.
|
You're right about the underlying gap: I added the same So the strict refusal can't ship on top of this head without sample-preserving publication, which is not implemented for native multisample depth (the sampling debt entry already says so). I also tried letting the retirement through only once the guest has overwritten the owner's memory, the What this PR does instead is bounded and explicit: it retires only when the surface is not the current attachment and the read is a reuse read (context-register, texture or vertex-buffer). It is a compatibility workaround for SONIC SUPERSTARS (PPSA06888), not a correct general fix, so I have recorded the gap as a debt entry in On coordination: #6727 and this head both rewrite Would you rather I gate the retirement further in the meantime, or land the debt-documented workaround and follow up with publication? |
|
Thanks for adding the host-write tracking and documenting the limitation. I would not merge the current compatibility retirement as a general fix: it can discard newer host depth/stencil/HTILE data, and #6727’s contract is explicitly to refuse that state until sample-preserving publication exists. The measured title failure makes this a real follow-up, but it does not make the guest bytes safe to expose. Please rebase the read-site classification, whole-fill HTILE note and exact-plane overlap work onto #6727 after it lands, and keep written-owner publication as a separate change with its own regression. If you want to pursue the bounded workaround independently, please first provide evidence that the guest read is a reuse/alias event whose contents are intentionally replacing the old surface; the current “not attached” condition alone does not establish that. I’ll keep the changes-requested review in place until the unsafe retirement is removed or a publication path is added. |
|
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.
|
…HTILE clears Guest titles (e.g. Sonic Superstars) render into a native multisampled colour target and then reuse that guest memory for something else while the driver still guards it as a resident target; every such access aborted and closed the title as soon as a level began. Drop a reused multisampled colour guard instead of throwing (issue boykopovar#5933). Supporting fixes that fall out of the same depth paths: - Honour a guest HTILE fast clear the driver has no producer note for when the whole HTILE proves the surface's clear value and neither the depth nor the stencil plane changed, instead of aborting. - Retire a depth surface only over the bytes its plane actually stores. The tiled footprint rounds the plane up to whole 128x128 blocks, so a neighbouring allocation could land in the padding and wrongly retire a surface it never reused. - Accept a stencil plane whose clear state is in the raw plane when HTILE stencil metadata is disabled, and accept a clear-proven HTILE without ITERATE_256 or pipe alignment. A guest read over a resident native multisample depth guard is not retired: it keeps refusing ("sample-preserving guest publication of native multisample depth is not implemented"), matching the contract of boykopovar#6727 for an unwritten surface, until sample-preserving publication of the depth, stencil and HTILE planes to guest memory exists. The read-site classification, whole-fill HTILE note and exact-plane overlap work are intended to be rebased onto boykopovar#6727 once it lands. Tests: agc_driver_native_multisample_depth, agc_driver_depth_fast_clear and agc_driver_depth_surface_last_writer exercise the new paths; the full agc_driver suite stays at its pre-existing pass rate.
A guest read over the guard of a resident native multisample depth surface refuses instead of retiring the owner, so a title that reuses written depth memory sees stale bytes or aborts. SONIC SUPERSTARS (PPSA06888) aborts on a register read over a written owner (measured on an RTX 5060 Ti, driver 610.57.04); publishing the samples to guest memory is the follow-up, with its own regression. Record the gap next to the sampling debt entry.
1e8b273 to
734edb5
Compare
KingDarkHN
changed the title
|
Thanks for the careful review — I agree, and I've followed it. Exposing guest bytes for a written I've removed the unsafe retirement. A guest read over the guard of a resident native multisample What the branch keeps:
I updated the debt entry ( Plan, as you suggest:
Branch updated (head |
|
A Windows data point for this branch: SONIC SUPERSTARS (PPSA06888) on It now stops a few seconds after start, during the logo screens, at the depth refusal you describe: So it matches your Linux run on the RTX 5060 Ti. The I can rerun it on this machine when the depth publication follow-up is up. AI-assisted (Claude Code). |
Amoenus
left a comment
Amoenus
left a comment
There was a problem hiding this comment.
Source review of exact head 734edb5 while checking overlap with native color publication work. The previous depth correction is acknowledged; the remaining color retirement has the two findings below. I did not build or run this PR or its title. AI-assisted: Codex; author responsible.
| for (const auto& range : guarded()) { | ||
| if (address >= range.address + range.bytes || range.address >= address + bytes) continue; | ||
| lock.unlock(); | ||
| retireGuardForAddress(address, bytes); |
There was a problem hiding this comment.
[P1] Preserve written color data before retiring its guard
This executes for every overlapping color guard. retireGuardForAddress marks the resident inactive and erases its guard without checking whether the host image has unpublished writes or copying its samples back. RequireNoMultisampleTarget then returns, allowing a guest reader to consume the old bytes. A read classified as reuse does not prove those bytes replace the rendered content; this function does not even classify the access. The same ownership concern that prompted the depth correction still applies to color. The drawn-color/guest-buffer counterexample in #6480 documents the stale-byte hazard, although I have not run that fixture against this PR. Please preserve the refusal until replacement is established or samples are published, and add a written-color/read regression.
| char text[256]; | ||
| std::snprintf(text, sizeof(text), "AGC graphics: %s of 0x%llx+0x%zx overlaps multisampled color target 0x%llx+0x%llx (%u samples), whose samples are only on the host: guest access to it is not implemented", access, static_cast<unsigned long long>(address), bytes, static_cast<unsigned long long>(range.address), static_cast<unsigned long long>(range.bytes), range.samples); | ||
| throw std::runtime_error(text); | ||
| std::snprintf(text, sizeof(text), "[gpu] %s of 0x%llx+0x%zx retired guard for reused multisampled color target 0x%llx+0x%llx (%u samples)", access, static_cast<unsigned long long>(address), bytes, static_cast<unsigned long long>(range.address), static_cast<unsigned long long>(range.bytes), range.samples); |
There was a problem hiding this comment.
[P2] Copy the guard before unlocking and erasing it
range is a reference to an element of guarded(). After the shared lock is released, another writer can invalidate it; retireGuardForAddress then calls std::erase_if on that same vector and removes the overlapping entry. Reading range.address, range.bytes and range.samples afterward therefore uses an invalidated reference. Copy the needed fields while the shared lock is still held and use that copy for the diagnostic. This remains necessary if the retirement semantics are corrected separately.
|
@KingDarkHN Updated in place: the qualified sample-preserving stack is #6973 → #6987 → #6989 → #6995 → #7002 #7002 has 99 focused passes (87 actual NVIDIA/AMD GPU, 12 CPU), full Windows builds, and the same nine AMD DCC assertion failures as literal main This is our adapted composition, not validation of unchanged #6808 or SONIC SUPERSTARS. For PPSA06888 reuse in #5933, format, tile/sample count, DCC/CMASK state and the next consumer still matter; I have not tested that title. #7260 retains Sonic's ordinary-run boundary and historical gameplay; no new title/FPS claim. AI-assisted: Codex; author responsible. |
|
I checked the linked #5933 report for the format/reuse details. It records a 4x, 64 KiB native target overlapping a 512x512 texture view (format 173, tile mode 2, 10 mips, size 0x56000); the view starts 0x51000 before the target. The report also has separate storage-image and CPU-read overlaps. It does not record DCC/CMASK state or establish whether the aliased texture is meant to consume the old rendered samples versus data written after reuse, so I cannot say the #7002 buffer-publication path applies. Its stated scope is read-only shader-buffer consumers; this #5933 example is a texture read and needs its own publication evidence/coverage. |
|
Another title that stops on this refusal: Assassin's Creed Mirage (PPSA13960, 01.007.000), on With a local change equivalent to this PR's |
github-actions
Bot
added
the
unblocks title
|
Thanks. This branch is far behind |
|
@KingDarkHN a follow-up to my rebase ask: we're going to take the native multisample colour work forward through Amoenus's stack (#6973 → #6987 → #6989 → #6995 → #7002), which adapts this approach and is kept up to date with Two questions, so nothing in this PR gets lost:
Thanks for the original work. The stack builds on it. |
This branch has not been deployed
7 participants
What
Sonic Superstars (PPSA06888) reaches the menu, but as soon as a level starts the emulator aborts
because it renders into a native multisampled colour target and then reuses that guest memory
for something else while the driver still guards it as a resident target (issue #5933). This branch
builds on #6297 and #6300 (its first three commits) and adds:
RequireNoMultisampleTargetinstead of throwing, viathe new
retireGuardForAddress(issue AGC: multisampled targets whose memory a title reuses throw on the next access (SONIC SUPERSTARS PPSA06888) #5933).the surface's clear value and neither the depth nor the stencil plane changed.
the plane up to whole 128x128 blocks, so a neighbouring allocation could land in the padding and
wrongly retire a surface it never reused.
disabled (
TILE_STENCIL_DISABLE), and accept a clear-proven HTILE without ITERATE_256 or pipealignment.
A guest read over a resident native multisample depth guard is not retired: it keeps
refusing (
sample-preserving guest publication of native multisample depth is not implemented),matching the contract of #6727 for an unwritten surface, until sample-preserving publication of the
depth, stencil and HTILE planes to guest memory exists. The read-site classification, whole-fill
HTILE note and exact-plane overlap work are intended to be rebased onto #6727 once it lands;
written-owner publication will be a separate change with its own regression. With this branch Sonic
Superstars aborts on a register read over a written depth owner — the documented follow-up.
Tested
agc_driver_native_multisample_depth(a metadata-only HTILE write with no producer notemust be served as a fast clear; a reuse read over a resident depth guard must refuse and leave the
owner resident),
agc_driver_depth_fast_clear,agc_driver_depth_fast_clear_submit,agc_driver_depth_surface_last_writerandagc_driver_depth_storage_reusepass. Fullctest -R agc_driveris at 98% with the same pre-existing failures as the base commit; none areintroduced (
worker_failure_logalso fails identically with this branch stashed).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)