Repository navigation
Conversation
|
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.
|
Collaborator
|
@MarcZX10 Reviewed at SDL2 also documents that a message box without a parent should run on the main thread and blocks its calling thread: https://wiki.libsdl.org/SDL2/SDL_ShowMessageBox. The current worker-thread call violates that rule, rather than merely lacking a macOS measurement. Please add a regression through an actual queue-order wait that keeps a prompt pending while another submission or presentation operation can acquire the driver lock, then verifies Wait and Quit handling. Keep the unset/zero and unavailable-display paths covered. Existing injected-presenter tests never open the real SDL box. |
Contributor
Author
Re-checked — new head 455321a, lock regression passes and fails without the fixThe stall prompt now runs without Review item 1: move the message box to the host main thread, answer back without submission or presentation locks.
Review item 2: a regression through a real queue-order wait, prompt pending, lock taken, then Wait and Quit.
Review item 3: unset/zero and unavailable-display paths.
Ran at |
Contributor
✅ NVIDIA GPU test: passedPR head
Unstable on main (2, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (2, not counted against this PR)Validated but not compared (10, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 425 tests)Skipped (42)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
MarcZX10
force-pushed
the
feat/stall-watchdog-prompt
branch
from
October 31, 2026 23:28
455321a to
fe79f2d
Compare
Contributor
✅ NVIDIA GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Passed on retry (1, failed in the parallel run, passed when re-run with a 360 s timeout; not counted against this PR)Validated but not compared (10, not in the main baseline's validation pass)Validation IDs already present on main (5 IDs in 484 tests)Skipped (45)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
12 tasks done
The stall watchdog takes its time from a settable clock. The default is steady_clock as before, so the title's behaviour does not change. A test injects a fake clock and pins one dump per wait at the watchdog period.
ANYPS5_STALL_PROMPT=1, with APS5_STALL_WATCHDOG_SECS set, shows a host message box when the watchdog expires for a wait: Wait keeps the title running, Quit ends it with exit code 124 after the submission rings are printed. The default, 0, keeps the single ring dump. Any other value stops the title at its first submission. When no message box can be shown, the rings are printed and the title keeps running.
…ne prompt per wait The prompt tests inject the clock and the presenter. They check that an invalid ANYPS5_STALL_PROMPT stops the first submission, that the prompt stays off by default, that an expired wait asks once and dumps the rings once, that an unavailable message box keeps the title running, and that Quit ends the process with exit code 124. The Quit case runs through a CMake script that checks the exit status itself.
…r wait After Wait, the stall watchdog waits one more period and asks again, at most three times for one wait. The submission rings are still dumped once per wait. The fake-clock test that pinned one prompt per wait is now stall-prompt-rearm and pins the new sequence and the limit.
A stalled WAIT_REG_MEM on the real clock, with APS5_STALL_WATCHDOG_SECS=1 and ANYPS5_STALL_PROMPT=1, is asked three times in about three seconds and not a fourth time, and its rings are dumped once.
The real-path stall test now records when each prompt is asked and checks that the second and third come at least one period after the answer to the previous one. Without the re-arm, the prompts came back to back, and the test only counted them.
When several waits expire together, one of them holds the message box and the others only dump their rings. A wait that finds the box taken is asked again on its next check, once the open prompt has been answered. A prompt that is already open cannot be opened again from inside itself.
A prompt is opened while another is answered: the wait that finds the message box taken still dumps its rings, does not ask the host, and is asked once the open prompt has been answered. The test runs the nested check from inside the presenter, so a re-entrant prompt would show up as a second call.
The section says what the prompt shows, what Wait and Quit do, when the box comes back, and what happens when no box can be shown or the value is invalid. It also states that the watchdog without the prompt never ends a title.
The stall prompt opens its message box from the thread whose wait expired. Whether SDL's macOS box works from a thread other than the main one has not been measured.
…t as unmeasured The stall prompt box is modal while the stalled wait holds its thread. The window system's handling of focus, the title window and presentation during that wait has not been measured on any desktop platform, and the tests inject the presenter instead of opening a window.
…behaviour No console behaviour is measured for a wait that stalls past the watchdog period, so the Wait and Quit choice has nothing on the console to match. The prompt is a diagnostic of AnyPS5.
… open The queue-order waits in QueueWorker.cpp called StallWatch::Check with Driver::mutex held, so a presenter that blocked on a message box also blocked every submission and video output registration that needed the driver lock. Check now takes the held lock, releases it around the presenter call, and takes it again before it returns Wait or Unavailable. Quit ends the title without retaking the lock.
…ock is taken Two real queue-order waits (queue 0 held at its rendering wait, queue 0x20 held behind it) reach an expired watchdog with ANYPS5_STALL_PROMPT=1 and a presenter that blocks until the test answers. While the prompt is pending, a submission to queue 0x21 and a video output registration must both complete, which they cannot while the stalled wait holds Driver::mutex. The Wait answer keeps the rings to one dump and asks again one period later; the Quit answer ends the title with exit code 124. The Unavailable answer prints the rings once, asks no further prompt for that wait, and the stalled submission still finishes once the rendering wait is released.
SDL_ShowMessageBox without a parent window should run on the main thread (SDL_messagebox.h). The watchdog asked the presenter from whichever queue worker or REWIND wait had expired. On macOS the presenter now runs through Aps5RunOnMainThread_nid_no_patch, the same main-queue entry point the video output uses, and the answer is returned to the waiting thread, which holds no driver lock while it waits. Other platforms have no main-thread queue, so the presenter still runs in place there. The debt entry for the message box records which platforms run it where.
The documented setting is unset or 0 for no box. Only the unset case was pinned, so a regression that treated 0 as an invalid value or as a prompt would pass. The zero mode runs the same test as the unset mode with the setting explicitly 0.
A compute submission without a WAIT_REG_MEM is waitFree, so it waits behind queue 0 and starts its own stall watch. The probe submitted during the prompt would have printed a second rings dump, breaking the one-dump check. The probe now carries a WAIT_REG_MEM whose condition already holds, so it is not order-held and still takes the driver lock in Submit.
MarcZX10
force-pushed
the
feat/stall-watchdog-prompt
branch
from
November 4, 2026 12:18
fe79f2d to
4749594
Compare
Contributor
✅ NVIDIA GPU test: passedPR head
Unstable on main (2, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validation IDs already present on main (13 IDs in 557 tests)Skipped (49)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
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
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
Today the stall watchdog (
APS5_STALL_WATCHDOG_SECS) never quits a title. When a watched wait has gone the set number of seconds without progress, it dumps the submission rings once for that wait and the title keeps running. It only dumps once; it never asks, and it never ends the title.This PR adds an opt-in host prompt for that moment,
ANYPS5_STALL_PROMPT=1, which needsAPS5_STALL_WATCHDOG_SECSto be set:0(the default) shows no box, so the current behaviour does not change. Any other value stops the title at its first submission with an error that namesANYPS5_STALL_PROMPT.The box is requested without
Driver::mutexheld. The waiting thread releases the driver lock before it calls the presenter and takes it back before it returns Wait or Unavailable, so a submission or a video output registration can complete while the box is open. On macOS the presenter runs on the main thread throughAps5RunOnMainThread_nid_no_patch, the main-queue entry point the video output already uses, whenever the main queue is being served, because SDL documents the main thread for a message box without a parent window. Linux and Windows have no main-thread queue, so the box opens on the thread whose wait expired there; the technical debt file records that as unmeasured.Commits, in order:
refactor: the stall clock is read through a settable seam, withsteady_clockas the default. Teststall-clockpins one dump per wait on an injected clock.feat: the setting, the SDL message box presenter, Quit, the unavailable fallback and theENVIRONMENT.mdrow. Tests for the invalid value, off, one prompt, Quit and the exit code.featandtest: re-arm after Wait and the three-prompt limit, with a real-path test on the real clock.featandtest: one box at a time, with a re-entry test.docs: theUSAGE.mdsection, and three technical debt entries: the macOS message box thread, the window system behaviour and the lack of a measured console behaviour.upstream/mainat bff0289 (d90188fc2a). None of the files this PR changes were changed onmainbetween the branch's base and bff0289, so the merge was clean.fix: the stall prompt releases the driver lock while the box is open (84b4242). Teststall-prompt-lock(804e75c) holds the box over a real queue-order wait while a submission and a video output registration take the driver lock, then checks Wait and Quit. Teststall-prompt-lock-unavailablechecks the unavailable answer the same way. The follow-upcd2ee7fmakes the probe submission carry a WAIT_REG_MEM that already holds, so the probe is not held behind queue 0 and does not print a second dump.fix: on macOS the box is requested throughAps5RunOnMainThread_nid_no_patch(bca46b2), and the debt entry for the message box says which platforms run it on which thread.test:ANYPS5_STALL_PROMPT=0is pinned as no box, teststall-prompt-zero(b3c5898).upstream/mainat 828ac0b (455321a2e4), a plain merge with no conflicts. Two files changed on both sides,core/libs/prx/libSceAgcDriver/CMakeLists.txtanddocs/user/USAGE.md; both sides' edits are present in the merged files. This PR is now based on currentmain.Overlaps: re-checked on
828ac0b2aewithgh pr list --state open --searchforANYPS5_STALL_PROMPT,StallWatch,SubmissionRingandStallPrompt: only this PR matches.tools/pr_overlap.py --branchcould not confirm 93 open pull requests (merge simulation) and so failed closed. Checked by hand: the file list of each PR it reported, and a search of open PR titles and bodies. No open PR touchesSubmissionRing.*orStallPrompt.cpp(no hit in the file probe). Searches of open PR titles and bodies forANYPS5_STALL_PROMPTandStallWatchfound nothing. Not checked line by line: the 93 unconfirmed PRs. Textual overlaps only, with no shared code: #12863 and #12895 (ENVIRONMENT.md,USAGE.md), #13701 (ENVIRONMENT.md,USAGE.md, PSN dialog debt docs), #12500 (ENVIRONMENT.md), #12764 (one debt line), #13474 and #6549 (tests/Pm4.cpp, debt), and #6185 and #6186 (CMakeLists.txt,tests/Pm4.cpp, debt). #12895 also opens an SDL message box in its own file (HostUrlConfirmation.cpp); this PR shares no code with it.Tested
Environment: Debian 12, Linux 6.1.0-54-amd64, GCC 12.2.0, ninja 1.11.1, Release,
BUILD_TESTING=ON. The scratch build copy relaxes the host CPU feature gate incore/libs/prx/libkernel/System/src/HostCpu.cpp, because this host lacks CPU extensions that the stock gate requires. The branch does not change that file.Built:
agc_driver_pm4_testsat the merged head455321a2e4, incremental in the scratch build tree after refreshing its sources fromgit archiveof the merged head (463 steps,NINJA-EXIT 0). The replay targets (agc_driver_pm4_replay_tests,agc_pm4_replay) were not rebuilt for this head. They linklibSceAgcDriver, which changed, so their rows below are the earlier run atd90188fc2a, not a run on this head. Ran:ctest -R agc_driver_pm4 -E replay --timeout 120at455321a2e4: 24 of 24 passed. The upstream column is the earlier run atbff0289787, the PR's previous base.upstream/mainhas since moved to828ac0b2ae; it was not rerun, because that needs another build tree.The 16 existing tests passed on
bff0289787and ond90188fc2a; on the merged head 14 of them were rerun and passed, and the two replay tests were not rebuilt. So the defaults are unchanged in the tested paths. WithANYPS5_STALL_PROMPTunset, the watchdog takes the same path as before: one dump per wait, no box, no exit.Lock control, on the merged head (
455321a2e4): in a scratch copy of the build tree, the two order-wait calls inQueueWorker.cppwere changed to pass no held lock, so the presenter ran withDriver::mutexheld.agc_driver_pm4_tests stall-prompt-lockthen failed witha submission waited for the driver lock while the stall prompt was openand exit code 1. The clone'sQueueWorker.cppwas restored and rebuilt, andstall_prompt_lock,stall_prompt_lock_unavailable,stall_prompt_quit,stall_prompt_off,stall_prompt_zeroandstall_prompt_rearmpass again on the restored build.Negative controls: each was a mutation in a scratch copy of the build tree, one per assertion. Every mutation failed the intended test and was then reverted. The controls ran on the code of their commits, not all on the head: the setting rows and the Quit rows on the test commit for those tests; the re-arm rows (R1 to R7) and the injected-clock rows (C1, C2) on the re-arm commit, before the try-lock commit; R4 and R5 on the real-path test before its timestamp assertions were added, and R1 and R2 after; the try-lock rows on the try-lock commit. Re-run on the head: the unavailable-as-Quit row and the never-prompt row. The lines each mutation changes are the same on the head, except that the try-lock commit added the open-prompt check before the presenter call.
QueueWorker.cpppasses no held lock)Not controlled: the Unavailable answer's relock on the queue-order path (covered by
stall_prompt_lock_unavailable, not mutated), and the__APPLE__branch ofaskOnMainThread, which is compiled only on macOS. Thetarget == 0check inexpectSettingRejectedis shared with tests that already existed. The[stall prompt] no message boxline is not asserted by any test; the tests assert the dump and prompt counts instead.Not run: Windows and macOS builds and tests. On macOS the
Aps5RunOnMainThread_nid_no_patchbranch ofaskOnMainThreadwas not compiled, so the main-queue hop is not verified here. The replay targets were not rebuilt for455321a2e4. The GPU comparison that AGENTS.md asks for AGC changes was not run; these tests run on the CPU side with an injected presenter. The hosted checks passed ond90188fc2a(their scope was not inspected); they were not inspected for455321a2e4. #13701 and the 93 PRs that pr_overlap could not confirm were not compared line by line. No test opens a real message box or a window: every test injects the presenter. No console measurement exists for this behaviour (see the debt entry). Other test binaries oflibSceAgcDriverand the rest of AnyPS5 were not built or run. No GPU was used; the watchdog tests run on the CPU side.Checks at the merged head
455321a2e4:tools/check_conventions.py --base upstream/mainpasses, and so doesgit diff --checkagainstupstream/main.tools/env_reference.py --checkfails only onAPS5_PAD_SPEAKER_FALLBACK_DB, the same single line as onupstream/main.tools/pr_abi_audit.py --base 828ac0b2ae --head 455321a2e4reports not applicable (no export changed).tools/pr_test_quality.pyflagstests/Pm4.cppas an unregistered test, which is a false positive: the file is registered asagc_driver_pm4_testsincore/libs/prx/libSceAgcDriver/CMakeLists.txt.tools/pr_precheck.py --base 828ac0b2ae --head 455321a2e4, run from a copy inside the clone, passes the diff check and the conventions and PR-template item. Its ABI and test-quality items fail only because those two tools are not in the candidate tree; they were run by hand as above.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)