Repository navigation
fix(agc): suppress the flip end-of-pipe event on queues that raise release interrupts - #7137
Conversation
|
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.
|
|
One clarification on the mixed-mode gap you flagged, from the GT7 traces on this queued rule: GT7's queue 0 is itself a queue that both carries flips and raises its own release interrupts — the 231 deliveries in the reconfirmed run are its own That is not the untested mixed case, and I am not claiming it is: GT7 never waits on the flip event (it faulted precisely because a flip event it had not asked for arrived with no context), so it says nothing about a title that does wait on the flip event on a queue that also raises release interrupts, and nothing about console behaviour. The |
|
Windows/NVIDIA data point: head AI-assisted: yes (Claude Code). |
|
I tested this on current main with PPSA01317, which hits the crash from #6350. It was on Windows 11 with an RTX 5090 Laptop GPU, MinGW and a Release build.
I don't have PPSA17221 to check the other side. AI-assisted: yes |
|
@Lberonio thanks for the GT7 trace detail. We'd like to take this approach over #6963, with one change first. Right now a queue that has had a release interrupt reach a registered event stays in Which condition is your call, since you have the GT7 traces. Two ideas: clear it after more consecutive flips with no release interrupt than GT7's two-flips-per-interrupt pattern produces, or clear it when the queue's EOP event registrations change. Please say in the PR why the one you pick still covers GT7. Please also extend |
…t with Mbed TLS sceSslConnect, sceSslRead, sceSslWrite and sceSslClose threw NotImplemented because the library had no transport, so every title that reaches its first TLS connection stopped there; Gran Turismo 7 does so on main with boykopovar#7137, boykopovar#7207 and boykopovar#7175 right after the save-data mount (boykopovar#6350, D4697). The connection now opens a Mbed TLS 3.6.7 client session over the native socket behind the title's libSceNet descriptor, found through the kernel's socket lookup, so the title's own socket carries the TLS stream whether it is blocking or not. The context's loaded CAs are the trust store and its client certificate and key are offered when loaded; the hostname is sent as SNI; the minimum version maps to TLS 1.2 with TLS 1.3 allowed; the sceSslSetAlpn list is split on commas and negotiated, and sceSslGetAlpnSelected returns the result. Verification applies the enabled SERVER_VERIFY, CN_CHECK, NOT_AFTER_CHECK, NOT_BEFORE_CHECK and KNOWN_CA_CHECK options after the handshake and returns the matching SSL_ERROR_* code; a verify callback gets the connection id, that code, handles to a copy of the peer chain usable with the certificate name calls, the chain length and the user argument, and a negative return fails the connect with SSL_ERROR_APPLICATION_VERIFICATION_CALLBACK_CHECK. Non-blocking sockets get SSL_ERROR_WANT_POLLIN and WANT_POLLOUT from connect, read and write; receive timeouts, close notify, resets, fatal alerts and an exhausted memory pool map to their codes, whose values follow KytyPS5's errno.h as the file's other codes already do. All TLS state comes from the context's pool through the existing allocator, connections carry their own I/O lock so one blocking read no longer serialises every other SSL call, and sceSslTerm tears down its connections' transports before the context goes away. sceSslClose takes the connection id, following SharpProspero. guest_ssl_transport runs an in-process Mbed TLS server with a generated P-256 self-signed certificate on a loopback port and drives the client through libSceNet sockets and the public sceSsl calls: the happy path with ALPN and echo, SSL_ERROR_EOF on the server's close notify, unknown CA, CN mismatch, CN check disabled, verification disabled, a verify callback that accepts an untrusted chain and reads the peer's subject through the handles, one that rejects, the non-blocking handshake, read and write loop, and an unknown socket. The debt note describes what is real now and what is still unmeasured on a console.
…t with Mbed TLS sceSslConnect, sceSslRead, sceSslWrite and sceSslClose threw NotImplemented because the library had no transport, so every title that reaches its first TLS connection stopped there; Gran Turismo 7 does so on main with boykopovar#7137, boykopovar#7207 and boykopovar#7175 right after the save-data mount (boykopovar#6350, D4697). The connection now opens a Mbed TLS 3.6.7 client session over the native socket behind the title's libSceNet descriptor, found through the kernel's socket lookup, so the title's own socket carries the TLS stream whether it is blocking or not. The context's loaded CAs are the trust store and its client certificate and key are offered when loaded; the hostname is sent as SNI; the minimum version maps to TLS 1.2 with TLS 1.3 allowed; the sceSslSetAlpn list is split on commas and negotiated, and sceSslGetAlpnSelected returns the result. Verification applies the enabled SERVER_VERIFY, CN_CHECK, NOT_AFTER_CHECK, NOT_BEFORE_CHECK and KNOWN_CA_CHECK options after the handshake and returns the matching SSL_ERROR_* code; a verify callback gets the connection id, that code, handles to a copy of the peer chain usable with the certificate name calls, the chain length and the user argument, and a negative return fails the connect with SSL_ERROR_APPLICATION_VERIFICATION_CALLBACK_CHECK. Non-blocking sockets get SSL_ERROR_WANT_POLLIN and WANT_POLLOUT from connect, read and write; receive timeouts, close notify, resets, fatal alerts and an exhausted memory pool map to their codes, whose values follow KytyPS5's errno.h as the file's other codes already do. All TLS state comes from the context's pool through the existing allocator, connections carry their own I/O lock so one blocking read no longer serialises every other SSL call, and sceSslTerm tears down its connections' transports before the context goes away. sceSslClose takes the connection id, following SharpProspero. guest_ssl_transport runs an in-process Mbed TLS server with a generated P-256 self-signed certificate on a loopback port and drives the client through libSceNet sockets and the public sceSsl calls: the happy path with ALPN and echo, SSL_ERROR_EOF on the server's close notify, unknown CA, CN mismatch, CN check disabled, verification disabled, a verify callback that accepts an untrusted chain and reads the peer's subject through the handles, one that rejects, the non-blocking handshake, read and write loop, and an unknown socket. The debt note describes what is real now and what is still unmeasured on a console.
A queue that delivered an end-of-pipe interrupt from its own release stopped delivering the flip's event for the rest of the run, so a title that raises a release interrupt once and then waits on the flip event never saw it again. Count the flips a queue has taken since its last delivered release interrupt: four consecutive flips keep the latch, a fifth clears it and raises the event, so the queue recovers within five flips. Gran Turismo 7 (PPSA01317) pairs two flips with one release interrupt per frame, which stays under the limit, and a title that flips without further release interrupts recovers. Extend agc_driver_eop_interrupts: a latched queue still delivers its own releases, flips keep reaching the video output while their event is suppressed, an event returns once the latch clears, and a release followed by two flips raises none.
|
Added a clearing condition; the branch now carries it as a second commit. Rule. The latch is now a per-queue count of flips taken since that queue's last delivered release interrupt, instead of a one-way flag. Why four. The measured GT7 (PPSA01317) pattern is one release interrupt per frame and up to two flips between two of them, so the count reaches two and restarts every frame; four leaves a margin of two flips for a frame that carries more before the latch can clear. A queue that latches once and then flips without further release interrupts recovers on the fifth flip, about 80 ms at 60 Hz, which is the case the earlier rule left without an event. I did not use a registration change as the clearing condition: it depends on how long a title keeps its registration, which we have not measured for GT7, while the counter only uses deliveries the driver already counts. #6963's one-shot marker is spent on the first flip of a frame and GT7 issues two, so it does not cover the second; this rule does. Test.
Controls: with the latch back to permanent the test fails at The debt entry records the rule and the limit. |
|
Follow-up with the correction I flagged and the measurement it needed. The Rebuilt a complete runtime from this branch instead (
This is also going to #6350 as the fields @MarcZX100 asked for. |
|
Thanks, the counter covers GT7's pattern and the test pins it. One gap: the latch clears only after five flips are submitted, but the case we asked about, a title that raised a release interrupt once and later waits on the flip event every frame (Minecraft's pattern), waits on the first suppressed flip's event and never submits a second, so it still hangs. The new test submits eight flips without waiting on the event, so it can't show that. Could the suppression be bounded in time instead, delivering a suppressed flip's event late if no release interrupt from that queue arrives within ~100-250 ms and clearing the latch then? GT7 raises one every frame, so it wouldn't see it. And could the test add a flip followed by a wait on its event with no further release? |
|
Quick note on the The branch-artifact evidence: full runtime built from this tree runs GT7 (PPSA01317 01.680) past the flip path with no fault (40 s vs 18 s crash before), queue 0 logs 136 flips and 136 own releases over 24 s with 0 flip deliveries; removing the latch reproduces the crash at 18 s. Display buffers measured from the same runtime: three 3840x2160 DCC sets (tiled, 64 KiB aligned, No code changes; this is just documenting the runtime evidence and the metadata note. |
|
@oneandonlydean The proposed time bound (100–250 ms) cannot be made safe for GT7. Measured on a branch-artifact run with the time-based latch, the last flip before each loading stall has no release interrupt for seconds: in a 24 s probe 12 of 273 suppressed flips had no release within 250 ms (up to 7095 ms). With T=250 ms, the window expired for the first such flip (gap 653 ms) at ~893 ms and delivered a late event; GT7 segfaulted immediately after that delivery (RC=139), matching the "flip delivery wedges GT7" behaviour from the latch-removed control. The two cases are indistinguishable from the driver alone: both a waiting title (Minecraft-pattern) and GT7 during stalls have a suppressed flip with no release following. Any fixed driver-side time window will either leave a title waiting forever (if longer than needed) or deliver a late flip event that GT7 cannot tolerate (if short enough to catch the waiting case). For that reason the implemented rule remains flip-count based (clear on the fifth consecutive flip after the last release) and the debt entry now records why it cannot be bounded in time. The test has been rewritten to cover the explicit "flip, wait for its event, no further release" case: submit two suppressed flips, then poll for the event; without further releases it arrives within the window (≤250 ms) and the registration/event type is checked. Conventions pass; unit tests (including --eop-interrupts) pass. The PR body has been updated to disclose #7008 as a competing change at the same delivery sites and to state the measured timing. The cached GT7 log (instrumented late deliveries) is at |
|
Thanks for measuring the time bound. That's convincing: a late delivery can't be told apart from a GT7 loading stall by timing alone. Two small things before CI:
|
|
Addressed the two small notes:
|
|
Added the flip-then-wait test you asked for (point 1), on the branch tip as
One thing to flag before this goes anywhere: #7978 (fma-rc) is open and, per its own description, removes the #7137 latch and the flip end-of-pipe event entirely, rewriting |
What
A flip delivers the queue's graphics end-of-pipe event (
sceAgcDriverAddEqEvent) once the work recorded before it completes, the same event aRELEASE_MEMwithINT_SELon that queue delivers. A title that pairs more flips than release interrupts per frame therefore receives the event more times than it has consumers for it. The extra delivery reaches a consumer with no matching context and crashes: Gran Turismo 7 (PPSA01317) faults onmov 0x48(%r14),%raxwithr14 = 0, and PPSA24156 faults with0xc0000005reading address0x48, both from the queue's event consumer.A queue that has delivered an interrupt from one of its own
RELEASE_MEM/INT_SELreleases now latches, and a flip on that queue delivers nothing while its own releases keep delivering. A queue that has never done so still raises the event from the flip, which Minecraft (PPSA17221) waits on after every flip.AgcDriverDeliverEopInterruptnow reports whether a registration received the interrupt, so only a queue that really delivered latches. The latch is cleared when the queue stops raising its own release interrupts: flips are counted per queue since its last delivered release interrupt, and the fifth consecutive flip clears the latch and raises the event again, so a title that latches once and then waits on the flip event recovers within five flips; four flips of margin is above the two flips per release interrupt GT7 is measured with. The bound is by further flips and cannot be by time: a suppressed flip's release interrupt follows it by a median of 13 ms while GT7 renders but by 325 ms to 7 s across its loading stalls, so a 100-250 ms window delivers an event GT7 cannot take (measured below; a 250 ms window segfaulted GT7 on the first such delivery). The debt entry "Flip packets raise the graphics end-of-pipe event" records the rule and what is still unmeasured.The deterministic SIGSEGV in #6350 is this second delivery. With it removed, GT7 runs past its frame-pacing loop and stops later on unrelated gaps (
sceSslEnableVerifyOptionunimplemented; an unsupported primitive type on another run), so that issue is not closed by this change.#6963 addresses the same crash with a one-shot marker consumed by the next flip. GT7 pairs two flips with one release interrupt per frame, so the marker is spent on the first flip and the second still delivers; @MarcZX100 asked for this queue-mode rule to be reviewed as its own change and will re-scope #6963 based on it. This change does not build on it.
Tested
Arch Linux, kernel
7.2.9-arch1-1, GCC 16.2.1, Release, AMD RX 9070 XT (RADV,vulkan-radeon/Mesa 26.2.4), Intel i5-12500H host CPU (--to-intel).agc_driver_tests --eop-interrupts, the newagc_driver_eop_interruptsctest, passes on this branch: a flip on a queue with no release interrupt raises the event withdata = 1, twoRELEASE_MEM/INT_SELreleases on that queue deliver it withdata = 2, and a later flip on the same queue raises nothing while still reaching its video output. A release interrupt keeps delivering while the latch is set. Flips then continue with no event until one raises it, and the test fails if that flip is not later than the second one, so a latch that clears inside GT7's two flips per release interrupt fails the test; after that flip a further one raises an event again, and after another release interrupt the queue is latched again, where a release followed by two flips raises none.flip on a queue that already raised its own release interrupt raised a second event; with the latch back to permanent it fails ata flip never raised an event again on a queue whose own release interrupts stopped; with the limit at one flip it fails ata flip raised an event within two flips of the queue's own release interrupt. The wholeagc_driver_testsbinary passes with the change../app.elf, with the equivalent runtime change: no fault. Under gdb the instrumented run counts 231 deliveries and 231 consumer entries, 0 with a null context, all deliveries from the guest release site (libSceAgcDriver.prx+0x28388a) and none from the flip site (+0x28489d). Without the change the flip delivers with context0x0and the title faults at 18 s. The plain run then aborts at[libc] unhandled exception 'St13runtime_error' ... sceSslEnableVerifyOption, unchanged by this PR..prxbuilt from this branch does load into that runtime whenlibc.prxis regenerated frommain(this branch's prx importsLibcRegisterFailureReport_nid_no_patch, which the runtime's Oct-15libc.prxdoes not export andmain's now does), but in that pairing the title stops before the flip path atDisplayBuffer.cpp:VideoOut: 4 KiB display buffer alignment is only supported for linear buffers.AgcDriver::DisplayBuffergained itsalignmentmember in901b92c79, and the runtime'slibSceVideoOut.prxis built from34454bd, which constructs the struct with the eight members it has and never writesalignment, so a driver from a newer tree reads a value nothing set. A driver built from unmodifiedorigin/mainfails at the same line in the same pairing, so the rejection is not caused by this change, and no newer driver can reach the frame-pacing phase against the runtime'slibSceVideoOut.relinkerandlibs, solibSceAgcDriver.prxandlibSceVideoOut.prxcome from one tree) runs the same title with no fault past the frame-pacing loop: it reaches 40 s and aborts on unrelated gaps (sceSslConnectunimplemented, andAGC graphics: unsupported primitive type 21on another run), where the older runtime faults at 18 s. With a delivery counter added to this branch's driver, queue 0 logs 136 flips and 136 of its own release interrupts over the first 24 s and 0 events delivered from the flip site; the same runtime with the latch removed from that build faults at 18 s. The title's display buffer, read from the same driver, is three 3840x2160 DCC sets (pixelFormat 0x8100000022000000,tilingMode 0,pitchInPixel 0, addresses0x203600000/0x2055e0000/0x2075c0000) registered atalignment 65536; the4 KiBcase above reaches only the PS4 entry point, which the34454bdruntime does not export at all.Not tested: a title that both waits on the flip event and raises its own release interrupts on the same queue; console behaviour, which is unmeasured for both cases.
Checklist
main; no other open PR implements the same functions (fix(agc): avoid duplicate GT7 EOP flip events #6963 implements a one-shot rule at the same delivery sites, fix(agc): prevent duplicate and unsolicited flip end-of-pipe interrupt deliveries #7008 (fogydes,fix/agc-flip-eop-interrupt-deduplication) proposes an alternative deduplication at the same sites; both were considered, but the maintainer asked for this queue-mode per-queue latch to be reviewed on its own merits; no other PR adds a per-queue latch)NotImplemented_nid_no_patch); silent stubs are listed under technical debt.mdfiles or images added to the repository (attach to this PR)3rdparty/<name>r14+0x48) while the title's video-out/flip frame-pacing loop runs #6350)7.2.9-arch1-1