Repository navigation
fix(libSceAgcDriver): mask WAIT_REG_MEM addresses and elide non-overlapping pipeline drains - #6660
Conversation
|
Could you add focused regression cases for the two behaviors changed here? I don't see test files in this PR's diff. In particular, a PM4 case for misaligned 32-bit and 64-bit WAIT_REG_MEM addresses would verify the correct low bits are masked consistently through validation, decoding, and execution. A queue test could also verify that an already-satisfied wait on a non-overlapping range avoids the pipeline drain, while an overlapping pending write still takes the drain/wait path. The Overcooked run is useful evidence for the reported 32-bit abort, but does not cover the 64-bit form or the drain-elision branch. |
jotitadev
left a comment
jotitadev
left a comment
There was a problem hiding this comment.
The new masking uses two different alignments for one packet shape: WAIT_REG_MEM_64 (0x93) with a zero high mask dword (packet[7] == 0).
WaitMemory.cppand thePacketExecution.cppgate mask withawaitedBytes == 8 ? ~7 : ~3.WaitAwaitedBytesreturns 4 for that packet, so they keep the address 4-aligned.Pm4.cppWaitSatisfied,WaitSatisfiedUncheckedandDecodeWaitmask withwide ? ~7 : ~3, so they use 8 bytes for the same packet.
When the address is 4 mod 8, waitMemory checks the range, looks up labels and reports timeouts at A. Its satisfaction checks (WaitSatisfiedUnchecked, lines 137 and 334 on the head) read the qword at A - 4. With the high mask dword 0, that compares the dword at A - 4, not the dword at A. Before this PR, Validate rejected that address, so this combination could not happen.
I compiled a standalone copy of the head's expressions (WaitAwaitedBytes, waitCompares, WaitSatisfiedUnchecked and the WaitMemory.cpp mask) with Apple clang, -std=c++20, on arm64. Packet: 0x93, compare ==, reference 1, mask 0x00000000ffffffff, address at offset 4 of an 8-aligned buffer.
addr%8=4 awaitedBytes=4 WaitMemory.awaited=+0 Pm4.source=+-4
label written at packet address -> WaitSatisfiedUnchecked=0
after writing the dword 4 bytes below -> 1
So a title that writes the label at the packet's address keeps waiting, and a write to the neighbouring dword releases the wait. One mask in both places would remove the mismatch: either wide in WaitMemory.cpp and PacketExecution.cpp, or WaitAwaitedBytes(packet) == 8 in Pm4.cpp. A PM4 case for this exact shape would also cover the "masked consistently" test requested above. Separately, LabelHistory.cpp:53 still records the unmasked address for WAIT_REG_MEM. That only affects diagnostics.
Static review against main 5d9dcc4, head 97e5318. No full build and no GPU run.
Written with AI assistance (Claude Code). I ran the checks above on my machine.
|
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.
|
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
The follow-up review found a concrete mismatch in the 64-bit WAIT_REG_MEM path: with a zero high mask dword, the execution/validation path treats the wait as 4-byte aligned while the PM4 satisfaction/decode path aligns it as 8-byte. For an address that is 4 mod 8, those paths inspect different dwords, so a write at the packet address may not release the wait. Could you reconcile the alignment rule across these three files and add a focused PM4 regression for opcode 0x93 with packet[7] == 0 and an address 4 mod 8? The separate queue drain-elision coverage I asked about earlier remains useful too.
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
I found one concrete mismatch in the 64-bit WAIT_REG_MEM path: with a zero high mask dword, execution/validation treats the wait as 4-byte aligned while PM4 satisfaction/decode aligns it as 8-byte. For an address that is 4 mod 8, those paths inspect different dwords, so a write at the packet address may not release the wait. Please reconcile the alignment rule across these files and add a focused regression for opcode 0x93 with packet[7] == 0 and an address 4 mod 8. The queue drain-elision coverage from my earlier comment remains useful too.
97e5318 to
6ab4e09
Compare
|
Added focused regression test coverage in
All checks passed in |
|
Thanks for adding the focused cases. I checked the updated diff: the 64-bit opcode now uses 8-byte address masking consistently in validation/execution, decoding and satisfaction checks, including the zero-high-mask case; the queue test also exercises both non-overlap elision and overlap draining. This addresses the gaps I raised. I have not run the full PR build. |
jotitadev
left a comment
jotitadev
left a comment
There was a problem hiding this comment.
Re-checked at head 6ab4e09fd against my earlier note on 97e531811. The mask is now wide ? ~7 : ~3 in all three places, so the mismatch I reported is gone. I also ran the tests, and the new zero-high-mask case does not catch that mismatch.
- Machine: Ubuntu 26.04, x86_64 (i7-8700), GCC 15.2.0, Release. Base
main5d9dcc42d. agc_driver_pm4_testsdoes not need a GPU. I ran it with no Vulkan driver available (VK_DRIVER_FILESpointing to a missing file). Understraceit opens nolibvulkan,/dev/nvidia*or/dev/drifile, and it passes.
Results:
- Head
6ab4e09fd:agc_driver_pm4,agc_driver_pm4_failureandagc_driver_pm4_failure_inlinepass. mainwith only the newtests/Pm4.cpp: fails withmisaligned WAIT_REG_MEM address, as expected.- Old head
97e531811with the newtests/Pm4.cpp: passes.testWaitRegMemMasking, zero-high-mask case included, only callsPm4::Validate,DecodeWait,WaitSatisfiedandWaitSatisfiedUnchecked. Those helpers already masked withwide ? ~7 : ~3at97e531811. The mismatch was inWaitMemory.cppand thePacketExecution.cppelision gate, and no new case submits a 0x93 packet withpacket[7] == 0at an address that is 4 mod 8. So the test would not catch a regression back toawaitedBytes == 8 ? ~7 : ~3there.
This submission-level case does catch it. It uses the same pattern as testWaitRegMemPipelineDrain: a pending pipeline item covers the dword the wait compares, and the wait must not skip the drain.
void testWaitRegMem64ZeroHighMaskDrain() {
alignas(8) static std::uint32_t qword[2] = {42, 0};
const auto base = reinterpret_cast<std::uintptr_t>(&qword[0]);
const auto packetAddr = base + 4;
std::atomic<bool> hold{true};
std::atomic<bool> started{false};
AgcDriver::DriverDetail::DrawPipeline::Queue0().Enqueue([&] {
started.store(true);
while (hold.load()) std::this_thread::sleep_for(std::chrono::milliseconds(1));
}, { {base, base + 4} });
while (!started.load()) std::this_thread::sleep_for(std::chrono::milliseconds(1));
auto words = makePacket(0x93, {0x13, static_cast<std::uint32_t>(packetAddr), static_cast<std::uint32_t>(packetAddr >> 32u), 42, 0, 0xffffffffu, 0, 0});
submitWords(words, 0);
std::this_thread::sleep_for(std::chrono::milliseconds(50));
const auto outstanding = AgcDriver::DriverDetail::DrawPipeline::Queue0().Outstanding();
hold.store(false);
AgcDriverWaitIdle_nid_postfix();
check(outstanding == 1, "WAIT_REG_MEM_64 with zero high mask at 4 mod 8 skipped the drain of a pending write to the dword it compares");
}I called it after testWaitRegMemPipelineDrain() and ran it 3 times on each head:
| Head | Result |
|---|---|
6ab4e09fd |
3/3 pass |
97e531811 |
3/3 fail: the gate checks [A, A+4) for overlap, finds none, and then opens on the qword at A-4, whose low dword is the one with the pending write |
Feel free to take it as is if it's useful.
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
github-actions
Bot
added
agc
6ab4e09 to
ffeae68
Compare
|
Besiege (PPSA20313), Windows, RTX 4070 SUPER, Ancient Grounds sandbox (about 1000 draws per frame), head ffeae68 on top of current main plus open perf PRs: 22.3 -> 24.4 FPS with this PR, pipeline drains per 10 s 11,000 -> 1,700. A 10 minute run in the level had no errors and the frame looks the same. |
|
Correction to my earlier numbers here: stacked with #6082 this change shows stale frames in Besiege (PPSA20313, Windows, RTX 4070 SUPER). In the cube-selection menu with the window unfocused the full-screen background is occasionally drawn with a diagonal split (one triangle stale). The cause is the new branch in Driver::execute that skips a wait satisfied at entry ( |
|
Two titles hit the 64-bit form of this, PS5NES (PPSA99204, v1.0.0.9) and PS5N64 (PPSA99206, v.1.0.0.7). Both are converted with On main. PS5NES aborts in 3 of 3 runs (17–21 s) and PS5N64 in 2 of 3 with That is a 64-bit equal wait on With this PR ( PS5NES then ends with Experiment, not proposed. Accepting a 4-byte-aligned 64-bit address and reading at the exact address let 3 of 3 runs of each title reach the 70 s limit, with no timeouts and no rejected packets. AMD PAL's AI-assisted: yes (Claude Code); results above were run on this machine. |
ffeae68 to
cdedb1e
Compare
|
Updated with fixes for both reported cases and rebased onto current main:
Verified with ctest -R agc_driver_pm4 passing 100% and ools/check_conventions.py passing cleanly. |
|
Re-tested head
Not checked on Linux or on a console. These runs show the new handling matches what the titles expect. They don't measure what the CP does with a 4-byte-aligned 64-bit wait whose high mask is 0; PAL marks address bits [2:0] reserved. AI-assisted: yes (Claude Code); results above were run on this machine. |
|
Thanks for the exact-base title comparison. I verified the PR is still at the tested head |
cdedb1e to
13fb5a2
Compare
|
Added the submission-level regression test |
|
That regression assertion covers the specific opcode/mask/alignment combination from the title report, and the focused PM4 test plus conventions pass on the updated head. Thanks for adding it. The earlier 3/3 PS5NES and PS5N64 comparison remains useful title evidence; both reports are now recorded on this PR. |
…apping pipeline drains
…at elided the drain
13fb5a2 to
d0bf343
Compare
|
The full Build failed on |
6 participants
What
This pull request resolves a critical abort and removes heavy pipeline drains during WAIT_REG_MEM command processing in libSceAgcDriver:
Tested
Checklist