mirror of
https://github.com/ARMSX2/ARMSX2.git
synced 2026-08-24 16:50:16 -07:00
memcard-rollback
100
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Brian Degenhardt
|
112bc73c4c |
Android: take a card snapshot at launch, and offer the restore
Wires MemoryCardBackup into the app. The snapshot is taken immediately before the emulation thread starts, in MainActivityRuntime.start() and startBios(). At that instant the card file is not open, so the copy cannot catch a half-finished write and there is no thread timing to reason about. It also means the copy holds the card as it stood when the player last finished successfully -- if this session is the one that breaks things, the snapshot is clean by construction. Restoring then loses the current session's saves, which is the trade a save-state slot already makes. The BIOS boot gets one too: its memory card manager can format a card or delete saves off it, so that session is worth a copy for the same reason a game is. Launch also checks the cards it is about to mount. If one will not read AND a verified backup exists, the boot is HELD and the prompt offers to put it back before the game starts. That ordering is not cosmetic: once the console has mounted a card it caches its own picture of the directory in guest memory, and a restore underneath would be written straight over. "Start anyway" stays available -- some people will want to format fresh -- and is remembered only for the launch it was answered for. The memory card screen gets a per-card Backups panel: the snapshots with their date, size and the game that was running, a verified-or-suspect badge, restore, back up now, and the automatic-backups switch. That manual path is the one that actually matters, because the automatic offer cannot fire for the failure players hit most -- a card that verifies perfectly while the save inside it is damaged. Recognising that would mean understanding each game's save format. Restore is refused while a game is running, for the cache reason above, and says so rather than failing quietly. A suspect snapshot is listed rather than hidden: the pre-restore copy of a broken card is exactly what someone may need back. |
||
|
Brian Degenhardt
|
8e07613231 |
Android: rolling per-card memory card snapshots
Nothing today keeps a previous version of a memory card. A file card is written
straight through to disk as the game plays, so the card on storage is always the
only card there is; anything that interrupts a write leaves the real card
damaged, and the player finds out when they load.
MemoryCardBackup keeps a small rotation of snapshots per card so that is
recoverable. This commit is the engine only -- nothing calls it yet.
Two rules do the real work:
- A card that fails verify() is never snapshotted. There is nothing worth
saving and a rotation slot to lose. That same check is what detects the
user's problem, so it is reported rather than swallowed.
- prune() never deletes the newest snapshot that passed verification. Without
that, the rotation is a shredder: the card breaks, the player relaunches five
times trying to work out why, and every good copy has been overwritten with a
copy of the broken card.
Retention is the newest 3, plus the newest from each of the preceding 4 distinct
days, plus always the newest verified copy however old. Three copies from one
afternoon only protect against that afternoon.
verify() is the same test the core trusts (FileMcd_IsMemoryCardFormatted): the
PS2 format signature at the head of the card, or the folder card's superblock
marker. Deliberately a size FLOOR rather than a table of exact sizes -- strict
about the signature, permissive about the size, so it refuses damaged cards
without refusing unusual ones that work.
Snapshots are content-hashed, so relaunching without saving costs nothing. They
are written to a .part file and renamed into place, so a process killed mid-write
leaves no truncated archive that would list as valid -- the same class of bug
this feature exists to undo. Restore stages and swaps through a sidelined copy,
outside the cards folder: a folder card is a directory, and the card list shows
every directory under memcards/ as a card, so staging there would flash a phantom
card mid-restore and leave one for good if the process died.
Restore takes a snapshot of the live card first, unconditionally, including a
broken one marked as such. Restoring the wrong copy must not be the act that
destroys the evidence.
Written against the Java file APIs rather than the core on purpose. The storage
memory cards live on is FUSE-emulated on some Android configurations, where
libc's file-creation call is denied outright -- a problem this tree has hit twice
and worked around both times from Java. A snapshot writer in the core would fail
silently on exactly the devices that need it.
The rotation joins the whole-app export: a card restored onto a new phone from
that archive is just as likely to be the broken one, so the history that can undo
it has to travel with it.
|
||
|
Brian Degenhardt
|
cd20f6454d |
Android: flush memory card writes when the app is backgrounded
A memory card write does not necessarily reach the file system when it happens,
and on Android that is data loss rather than a detail.
A FOLDER card holds writes in an in-memory page cache and flushes two frames
after the last one, counted down by the per-frame tick that runs off vsync. So
pausing does not delay that flush, it stops it ever being reached -- the counter
does not advance at all while the VM is paused.
A FILE card writes through stdio with no flush anywhere in the path. Seeking on
an update stream pushes the previous write out, so a run of writes mostly
self-corrects, but the last write of a save sequence sits in the buffer until the
next card access or fclose.
Either way the pending write is lost if Android reclaims the process while it is
backgrounded, which it may do with no further callback. Save in-game, switch
apps, get reclaimed, and the save was never on disk.
The pause path already handles exactly this shape for the BIOS NVRAM
(cdvdSaveNVRAM, added because "the process is frequently killed while paused"),
so the card flush goes next to it, in both the Running and already-Paused
branches. It runs on the CPU thread, queued after SetPaused, so the console is
stopped and nothing can be written behind it, and it is fire-and-forget -- onPause
is on a deadline and blocking it risks an ANR.
- FileMcd_Flush() / FileMemoryCard::Flush() / FolderMemoryCardAggregator::Flush()
write out what is buffered without closing anything, so the console keeps
playing afterwards.
- FileMemoryCard::Flush deliberately does NOT stamp the running checksum the way
Close() does. That value is a change-detector a savestate load compares to
decide whether the card moved under the console, not an integrity check, and
m_chkaddr is card data rather than a header field we own. A stale value costs
one auto-eject on the next savestate load, which is the safe direction, so
writing to the card on a path upstream never writes on buys nothing.
- FolderMemoryCard::FlushNow clears the frame countdown so a resumed VM does not
repeat the work. Flush() is already a no-op when nothing is cached, so calling
this on a quiet card costs nothing.
- Save() flushes each sector as it is written.
Also bounds the emulation-thread join in onDestroy. NativeApp.shutdown() already
gives up waiting after 5 s and returns anyway, so an unbounded join inherited a
wedged CPU thread and hung the destroy path until Android force-closed us.
|
||
|
Brian Degenhardt
|
df17121cc1 |
SIO/Memcard: refuse a card write whose read-back failed
FileMemoryCard::Save is a read-modify-write: it reads the sector being written into a scratch buffer, ANDs the new data into it (memory card bits only ever go 1->0 without an erase), and writes the result back. When the read failed it reported the error and then carried straight on into the merge. m_currentdata is only ever grown, never cleared, so the merge ran against whatever an earlier -- and possibly completely unrelated -- write had left in the buffer, and that was what got written to the card. A transient read failure therefore did not lose a write, it corrupted a sector the console never asked to change. Refuse the write instead. The sector keeps its previous contents, which the console can retry. The return value is discarded by the only caller (Sio.h), so this reads as "don't write" rather than as an error report -- the same shape as the two Seek failures already in this function. Desktop hosts rarely see a read fail on a card file. Android does: the storage memory cards live on is FUSE-emulated, where this tree already documents libc calls being denied outright. |
||
|
Brian Degenhardt
|
dfef534426 |
UI: the Android internal-resolution set in the Qt and Big Picture pickers
Both desktop resolution pickers ran Native, 2x, 3x ... in whole steps. That is the wrong shape for a handheld at both ends: below native is unreachable even though rendering there is a large win and the GS accepts it -- only the top end is clamped, in GSClampUpscaleMultiplier -- and above native the jump from 1x to 2x is four times the pixels with nothing in between. Both pickers now carry the set the Android UI has offered since issue #207: 0.25x 0.5x 0.75x Native 1.25x 1.5x 1.75x 2x 2.25x 2.5x 2.75x 3x 3.5x 4x 5x 6x 7x 8x Everything above 8x moves behind Extended Upscaling Multipliers, which used to unlock 13x and up. That checkbox was dead on ordinary hardware: the GPU cap is max texture size / 1280, so a 16K-texture part reports exactly 12x and the "supports extended" test wanted more than 12x. It now unlocks anything past 8x, which is the first time it does something on a normal GPU, and 9x-25x still appear only as far as the GPU can actually go. Big Picture indexed two parallel arrays as "slot i means multiplier i+1", which a non-uniform list breaks. Replaced with one table carrying the label, the INI string and the multiplier, filtered against the cap. The quarter steps are all exact in binary, so matching a saved multiplier by equality is safe. Two things fixed in passing, both of which the new list would otherwise have broken. The Qt global tab keyed "nothing found" off index 0 being Native, and index 0 is now 0.25x. And Big Picture passed "1.000000" as its default while the INI is written by StringUtil::ToChars, which produces "1" -- so with no key set the picker read back a value matching no entry and displayed Unknown. |
||
|
Brian Degenhardt
|
7f0ae7a6c6 |
GameDB: internal FPS by DISPFB blit for NASCAR Thunder 2002 through NASCAR 06
The register-write detector reads these engines as producing a new image every vblank, so duplicate-frame skipping never fires and every repeat pays for a full present. Counting blits into the displayed framebuffer restores it. Reported-by: yuasasa |
||
|
Brian Degenhardt
|
da25cb84cc |
Android: call eeClampMode 4 Exact, the name the other frontends use
The desktop and Big Picture pickers landed the same tier as Exact, and GameIndex.md documents it under that name. Android was the only frontend calling it something else, which made the same setting look like two different ones depending on which screen the user was on. The translation key moves with the label rather than keeping the old name for a value it no longer matches, so the map stays alphabetical and there is nothing left to mislead the next reader. |
||
|
Brian Degenhardt
|
541b1abfaf |
Android: offer eeClampMode 4 as Ludicrous in the clamping pickers
The tier existed in the core but nowhere in the UI, on any frontend, so
reaching it meant hand-editing the settings file. Both Android pickers —
the Performance tab and the in-game pause menu — now carry a fifth
option, and the settings layer packs it.
The packing is the part that matters. emucore validates the four clamp
booleans as a cascade and silently resets an inconsistent set to the
defaults rather than rejecting it, so writing fpuExactMode without its
three predecessors would not fail loudly, it would quietly land the user
back on Normal. applyTo therefore writes all four cumulatively, and
readFromIni reads them back highest-first.
readFromIni treats a missing fpuExactMode as an older core rather than as
mode 3: a build without the key never wrote it, and inferring 3 there
would demote a Ludicrous setting every time the settings were reloaded
under a mixed pair of builds.
The chip row already scrolls horizontally, so a fifth option needs no
layout change.
⚠️ Not addressed here, and worth a decision: the GameDB overwrites the
whole tier for any title carrying an eeClampMode entry, and an entry
below 4 clears the exact bit outright. On those ~115 titles the new
option is inert unless game fixes are off — which is most of the titles
whose users would want it. The setting description says so; whether the
core should let a user's choice raise the database's is a separate call.
|
||
|
Brian Degenhardt
|
99299e556c |
Merge pull request #590 from pstef/mode-3-4
Widen FPRreg to host double |
||
|
Brian Degenhardt
|
aeabe3e0e5 |
EE cache: fix the DXSTG tag lookup's 29-bit fold, and the tests around it (#568)
* EE: the D-cache store-tag lookup dropped the top three bits of the tag DXSTG takes a guest physical page from TagLo and has to turn it into the host pointer our tags carry. It did that by routing the page through its KSEG0 alias, which meant masking the tag to 29 bits first -- and KSEG0 is only 512 MB wide, so the mask was not a formality. Every physical page at or above 0x20000000 folded into the low half of the map and resolved to whatever happened to live at the folded address. The consequence that matters is that a page past the end of the physical map folded onto real memory: 0x60129000 resolved to 0x00129000, and the eviction wrote 64 bytes of cache line into guest RAM the tag never named. Use vtlb_GetPhyPtr instead, which is what the debugger and PSM already use to ask this question. It covers the whole 1 GB physical map and answers null both for a handler page and for an address off the end of the map, so the unbacked case is now decided by the same lookup that produces the pointer rather than by a truncation. Where a tag naming one of our main-RAM mirrors resolves changes as a side effect of that, and is deliberately left unpinned. Those mirrors are our physical map's, not a console's: an SCPH-30001 has no RAM at those physical addresses, and an eviction steered at one reached nothing at all. There is no hardware answer to hold us to, so nothing asserts one. * Tests: point the DXSTG unresolvable-page check at a page that is unresolvable The check named 0x1FFFF000, described as "BIOS/unmapped territory at the top of the physical map". That page is the last one of the 4 MB BIOS ROM mapped at 0x1FC00000, so it is real backing memory: the test took the backed branch every time, wrote 64 bytes into the loaded BIOS image, and asserted only that nothing faulted. The branch it was named for -- the one carrying the safety property -- had no coverage at all. Name 0x60129000 instead. It is past the end of the physical map, and it is the page with teeth, because the old 29-bit fold sent it to 0x00129000 in main RAM. A witness there turns "we did not fault" into "we did not write somewhere the guest never named", which is the property worth holding. An SCPH-30001 agrees with that much: an eviction steered above the end of RAM puts nothing into RAM. Nothing beyond it is asserted -- where a tag naming one of our main-RAM mirrors resolves is emulator-specific, so it stays unpinned, with a comment saying so and why. * Tests: stop the DXSTG write-back check skipping on 16K-page hosts MapAt's candidate addresses are 4K-aligned and none is 16K-aligned, so on a 16K-page kernel -- Asahi, Apple Silicon, some Android, and one of our own CI jobs -- the kernel rejects every one of them and the mapping fails. The write-back check treated that as a precondition and skipped outright, which took its guest-side assertions with it: the ones that actually pin where a DXSTG-steered eviction lands, none of which need anything from the host. The mapping is only the negative control, there to show the write-back did not ALSO reach the host page carrying the same number. Make it optional. The guest-side half now runs everywhere and only the control drops out. DxstgDirtyStaysInsideGuestMemory still skips, and should: it is entirely about the host page. That leaves one skip here on a 16K-page host instead of two, and none at all on a 4K one. |
||
|
Brian Degenhardt
|
bbda693a7f |
Android: let analog triggers be bound, and read the left one everywhere (#584)
Reported on an Xbox One controller: every control on the pad binds except the triggers, which do nothing at the "press a button" prompt. The binding model is keyed on Android keycodes, and most pads — an Xbox controller among them — report their triggers ONLY as analog axes, never as KEYCODE_BUTTON_L2/R2. The capture path already bridges motion to key for the HAT and for stick deflection; triggers were simply never added, and since that path consumes the motion event the press vanished without a trace. Pads whose triggers do send key events were unaffected, which is why this only surfaced now. A pulled trigger now stands in for the keycode a key-emitting pad would send, so it is an ordinary button to everything downstream: bindable to any PS2 control, stealable by another row, assignable as a hotkey or a macro, usable as a combo member. Gameplay resolves that same keycode back through the binding table, so what the capture records is what gets honoured — the two now share one axis resolver rather than each knowing its own list. That makes trigger-bound hotkeys and macros reachable from the Hotkeys and Pad tabs, so the gameplay side has to be able to fire them, or binding one would be a dead end. Both act on the press and the release, which lets a trigger drive the hold-type hotkeys (fast-forward, pressure modifier, gyro hold) that a stick edge cannot. Second fix, same area: the right trigger has a per-device fallback axis for pads that report it on AXIS_RZ, and the left had none. A pad Android has no vendor key layout for passes raw HID through, putting the triggers on plain Z and RZ — so on those devices the right trigger worked and the LEFT ONE WAS READ BY NOTHING, dead in gameplay rather than merely unbindable. Both sides now take the fallback, gated on a 0..1 range so a stick axis (-1..1) can never be mistaken for a trigger. |
||
|
Brian Degenhardt
|
c8b51438cc |
IPU: dither a whole row per deinterleaving load
ipu_dither has had an SSE2 path and a scalar reference since forever, and
arm64 took the reference. The compiler closes half of that gap on its own —
with dithering off the loop is simple enough that clang vectorises it, and
measured here the scalar and NEON versions come out cycle-identical. With
dithering on it closes none of it: the clamp is written as std::max/std::min
around a table lookup, the destination is a 5/5/5/1 bitfield, and between
them the vectoriser gives up entirely. That arm ran at about 36 instructions
per pixel.
The NEON version is not a transliteration of the SSE2 one. x86 needs six
unpacks to split a row into channels because it has no deinterleaving load;
NEON has VLD4, so a whole 16-pixel row arrives already split one register per
channel and the shuffle chain simply does not exist. The dither tables are
the reference's coefficients with the sign folded into the choice of
operation, which lets saturating byte arithmetic supply the clamp for free —
the same trick the SSE2 path uses, and the reason both agree with the
reference bit for bit.
Measured on an M2 Max P-core, 2M macroblocks, two runs each:
dither on reference 18.76G instructions / 3.372G cycles
NEON 1.29G instructions / 0.293G cycles (11.5x)
dither off reference 1.08G instructions / 0.247G cycles
NEON 1.13G instructions / 0.247G cycles (even)
Function size drops from 476 to 208 bytes.
The tests are the point of the commit as much as the code is. Three
implementations of one function existed and nothing had ever compared them,
which is a bad shape here: a wrong result does not crash, it tints an FMV,
and nobody reports that. The transform depends on nothing but a pixel's four
bytes and its position modulo four in each axis, so the suite sweeps every
byte value through every one of the sixteen dither cells rather than
sampling. It holds whichever path the host selected to the reference, so it
gates the SSE2 arm on x86 exactly as it gates NEON here.
Proven to discriminate by mutation: transposing the r and b channels fails
three of four cases (correctly not the sweep that holds the channels equal),
perturbing one dither cell by one fails two, and dropping saturation fails
all four.
ipu_dither_reference loses its __ri so that a symbol survives into Release
for the tests to call.
|
||
|
Brian Degenhardt
|
a3bf73bf7a |
GS: AArch64 has no slow unaligned load to compile around
FAST_UNALIGNED was defined only inside the ARCH_X86 arm, where it records that AVX-and-later cores stopped punishing unaligned vector loads. On ARM64 the macro was therefore undefined, which the preprocessor reads as zero, so every arm64 build compiled the texture-upload path as though the punishment existed. It never did. LDR Q and LD1 take any address, and GSVector4i's load template ignores its own `aligned` parameter and emits the same instruction either way. So the callers were paying for a distinction with no machine behind it: WriteImage tests the source address and the pitch on every call to choose between three template instantiations of WriteImageBlock and WriteImageColumn that, for 8- and 4-bit columns, compile to identical code. For 32- and 16-bit columns the unaligned arm is not identical, but it is the worse one — eight combining 64-bit loads instead of four 128-bit loads and a swizzle. Defining it collapses all of that. GSLocalMemoryMultiISA.cpp.o goes from 80,368 to 62,184 bytes of .text and from 58 emitted functions to 32, which is what an I-cache on a handheld cares about. Only GSBlock.h and GSLocalMemoryMultiISA.cpp read the macro, so nothing else moves. The retained load strategy is not new code: whenever an upload happened to land 32-byte aligned, arm64 already ran exactly this sequence. What goes away is the arm that only ever ran when it did not. |
||
|
Brian Degenhardt
|
6aa2fd79d9 |
Android: a Game-scope save must still write the process-wide fields
Toggling PINE from the in-game menu wrote it nowhere, so the setting was gone at the next launch while the switch still read as enabled -- saveSettings had already updated the in-memory Settings, and only a process restart exposed that the store never agreed. PINE is one server for the whole process, so "this game runs with PINE on" is not a thing that can be true. Settings.merge therefore pins pineEnabled and pineSlot to the global value, and Settings.diff never emits either key, so a per-game file can never acquire them. Both are deliberate and both are right. What was missing is the other half: a Game-scope save writes ONLY the override file. So for these two fields the write had no destination at all -- the override file structurally refuses them, and global was never touched. Every other field is fine, because every other field is one the override file accepts. The in-game menu saves in Game scope whenever a game is running, which is exactly when someone reaches for PINE, so the toggle looked simply broken. So promote those fields to global on a Game-scope save. Copied onto the loaded global rather than saving `updated` wholesale: `updated` is the game's RESOLVED settings, so writing all of it to global would push every per-game value into the global layer. The diff below is unaffected -- it reads the pre-promotion `global`, and the keys involved are precisely the ones it never emits. Pairs with the core fix that makes a commit act on the value; without this the value never survived to be acted on a second time. |
||
|
Brian Degenhardt
|
58b6dd983c |
Core: apply a PINE toggle when it is toggled, not at the next game change
ReloadPINE() had two callers -- CPUThreadInitialize() and UpdateDiscDetails().
Neither is on the settings path, so turning PINE on did nothing until the next
app start or the next game boot. The switch stays on in the UI, because that
part persists correctly; it is only the server that never appears. Reported on
Android, where it is worst -- a handheld has no second window to restart into,
so there is nothing to reveal that the setting did land and simply was not acted
on -- but nothing about the gap is Android-specific. The desktop Big Picture
toggle in FullscreenUI_Settings has the same two-call-site problem behind it.
Discord presence is the same shape of feature: an optional external service
whose whole lifecycle is one bool, toggled from the same settings pages. It is
handled in CheckForMiscConfigChanges, three lines from where PINE was missing.
So put PINE beside it, which is also what the Android settings layer already
documents as the contract ("a commit is enough -- no game restart").
Called unconditionally rather than gated on old_config, because ReloadPINE()
already compares the request against the LIVE server -- whether one is
initialized and on which slot -- and that is strictly stronger than a config
diff. It early-returns when the two agree, so the common commit costs one
comparison, and it recovers a server whose bind failed earlier rather than
trusting a config value that never changed. The port sitting in TIME_WAIT after
a fast restart is the usual way a bind is lost, and it is exactly the case a
config diff cannot see.
CheckForMiscConfigChanges runs from ApplySettings/ApplyCoreSettings, which
assert the CPU thread -- the same thread CPUThreadInitialize already reloads
PINE from, so this adds no new threading contract. The iOS emulation-only path
is unaffected: ReleaseNonEssentialRuntimeResources runs after
CheckForConfigChanges and calls PINEServer::Deinitialize() itself, so a release
still ends with the server down.
|
||
|
Brian Degenhardt
|
0b9e9cdfcb |
GS/SW: the C++ rasteriser packs a colour gradient like the generators do
The per-lane colour offsets were packed with the signed saturating pack while both code generators used the unsigned one. The mask above the pack has already put every lane in 0..65535, which makes the unsigned pack the identity and makes the signed pack flatten everything from 32768 up to 32767. A descending gouraud gradient is how a lane gets there: its offset is negative, the mask turns it into a large positive, and the pack saturates it. Every pixel of the group then carries that instead of its own colour, for the whole scanline. The mask and the unsigned pack were introduced together to fix exactly this, in "GS/SW: Mask color gradients to prevent incorrect clamping"; a later refactor that rewrote the same lines to change how the shift table is loaded retyped the tail back to the signed pack. The generators were not part of that refactor, which is why only the C++ path regressed and why nothing noticed. Where the path is reachable, measured rather than argued: with the rasteriser JIT on, a probe at the top of the C++ setup never fires across corpus replays that generate tens of kilobytes of scanline code apiece. It is entered only when there is no code memory to compile into at all, and that same condition turns off the EE, IOP and VU recompilers, so it is not a configuration anyone plays in. What it is, is the path a measurement runs under -- the only way to ask what the renderer computes without a JIT in the way, and so the arbiter of a generated-code question. It was about to arbitrate one, and would have lied: the gs-shade console capture re-run under it differed from the generated arm in 42,240 bytes, concentrated in exactly the gouraud colour it was to be asked about. It is now byte-identical, and the generated arm is byte-identical to before the change, so nothing a shipping build renders moves. The new suite runs both paths over the same spans and compares the setup state and the stored pixels, so the next divergence anywhere in the scanline fails loudly instead of waiting for a capture to find it. |
||
|
Brian Degenhardt
|
02e93048d4 |
IOP: let an immediate jump to zero reach the handler we already wrote
The recompiler already has a policy for arriving at address zero. A fetch at PC=0 raises an Address Error and the BIOS handler takes over (AX-11), because PS1 mode drives the IOP there through a register jump often enough to be worth modelling rather than asserting on. The immediate form of the same event never got there. Emitting a jump whose target is zero asserted instead, so the two ways of reaching the same address behaved differently: through a register it is emulated and the guest carries on, through `j 0` it aborts a Devel build one instruction before the handler would have seen it. Dropping the assert routes the immediate form into the existing path — the tail stores pc, links the block at zero, and the dispatcher hands it to psxRecompile, which raises the Address Error. Unlike the EE, nothing here compiles a jump the guest does not take: the IOP scanner ends every block at the first branch, so an unresolved weak symbol's guarded `jal 0` is never emitted. Reaching this needs the guest to genuinely jump to zero — an unguarded weak call, a branch target that computes to zero in low RAM, or a corrupted code word. The test runs the JIT arm alone, which is what the new harness mode is for: the interpreter has no PC=0 model at all, so the arms are meant to disagree here and the differential harness has nothing to say. |
||
|
Brian Degenhardt
|
cb56a72b26 |
EE: a jump to address zero is a target, not an impossibility
A call to an unresolved weak symbol links as `jal 0`, guarded by a null test on the symbol's address that always skips it. PS2SDK's libc glue ships four such sites, so every homebrew ELF built against it carries the shape, and the recompiler asserted the moment it met one. It meets one because SL-03 continuation compiles the skipped path: the guard branch becomes a continuation site, the scan runs on through the dead call, and the emitter is handed a zero target for code that never executes. The assert (inherited from the x86 recompiler, which aborts on the same ELF) then takes down any Devel build before the program starts. Nothing needs to happen at that target. If something did jump there, address zero resolves like every other address — a block in RAM page 0, or the unmapped-page handler — so the three tails just emit it. The shape only reaches the emitter when the guard cannot be resolved at compile time; a constant address folds the branch and the dead call is never emitted, which is why an ELF carrying it can run clean until one block boundary lands between the address materialization and the test. The tests pin the reachable half. |
||
|
Brian Degenhardt
|
85adcfe6df | Merge branch 'upstream-sync-2026-08' | ||
|
Brian Degenhardt
|
2d73c39f03 |
GS: lift the r44p1 GL fetch blocklist -- the field chose the fast path
Delete gl-arm-r44p1-attachment-self-read from the driver-bug database, so
r44p1 Mali takes GL_ARM_shader_framebuffer_fetch again on GLES and -- because
GSUtil::AndroidAutoPrefersVulkan asks the same table -- Auto resolves back to
OpenGL on those devices.
The rule was correct about the defect and wrong about the trade. Through
2.6.6.4 the gate it formalised was inert: the Mali profile block re-enabled
the ARM backend moments after the gate disabled it, so every r44p1 device
shipped on GL + fetch. 2.6.6.5 made the gate actually engage, and on GLES --
where fetch and the texture barrier are one capability -- every
self-referential draw became an RT copy plus a tile flush. Shadow of the
Colossus fell 30 -> 7 fps on the Anbernic RG 477V and users mass-downgraded
to 2.6.6.4. Offline replay of that scene under the device's feature shape
shows why no smaller fix could win the speed back: 890 render-target copies
and 938 render-pass breaks a frame against 1664 draws -- and a 2.6.6.4
replay under the same shape produces the same ledger (901/948/1664), so the
old build's speed WAS the in-tile read, not better GS decisions.
The known cost is unchanged from 2.6.6.4: r44p1's fetch corrupts some
content (MGS3 observed; most likely the driver grants the tile-read slot per
attachment format and silently degrades denied reads to memory fetches
inside a live feedback loop). Vulkan stays available as the
correct-rendering choice for those games, and its own r44p1 rule is
untouched -- there the in-tile read is a device loss, and the RT copy is an
ordinary image copy rather than a tile flush.
Unlike 2.6.6.4, the restored path is ordering-correct:
|
||
|
Brian Degenhardt
|
b2e22efc45 |
Android: send Auto to Vulkan where GL cannot read the target in-tile
The Auto renderer resolution picked Vulkan on Adreno and OpenGL everywhere else, on the reasoning that Mali runs GL_ARM_shader_framebuffer_fetch and so has the in-tile fast path on GL. That holds for a healthy Mali. It does not hold for a driver on the fetch blocklist, and the two decisions were made in different places, so nothing noticed when they disagreed. On GLES framebuffer fetch and the texture barrier are one capability -- there is no ARB or NV barrier extension -- so a blocklisted driver loses both. That is not a mild fallback on a tiler: it is not only accurate blending that starts reading the render target from a copy, it is every self-referential draw, and each copy forces the tile to flush and resolve to main memory. Measured on an Anbernic RG 477V (Mali-G615, r44p1) with Shadow of the Colossus: 7 fps on OpenGL against ~30 on Vulkan, same device, same settings. Vulkan reaches the same copy-based concept with an ordinary image copy and no tile flush. So Auto now also prefers Vulkan when the device's OpenGL driver profile carries UseRenderTargetCopyForFeedback. Both halves of the question are asked of the driver database rather than of substrings, which also retires the case-sensitive search for "Adreno" in GL_RENDERER in favour of the resolved runtime profile. The decision has to be native, because the database is: rules match a PARSED driver revision, which is what lets one say "exactly r44p1". The app cannot do that, so it now hands over the GL strings it already probes -- GL_VERSION is where the driver revision lives, and the probe was reading GL_RENDERER and throwing the rest away -- and GSUtil::AndroidAutoPrefersVulkan answers. setPreferVulkan(boolean) is replaced by setAutoRendererGpuStrings(3 strings) rather than kept alongside it; there was one call site. An explicit Vulkan/OpenGL/SW pick still wins, as before. The only devices this moves are the ones whose GL is degraded: currently r44p1 Mali and nothing else. |
||
|
Brian Degenhardt
|
db41082150 |
GS/OpenGL: ARM framebuffer fetch does order overlapping primitives
|
||
|
Brian Degenhardt
|
3e56da7f86 |
Merge upstream PCSX2 (2026-07-15 .. 2026-08-10)
71 commits from |
||
|
Brian Degenhardt
|
e509b17e7a |
Merge pull request #565 from pstef/tests
Assorted improvements |
||
|
Brian Degenhardt
|
e9f8f83669 |
GS/HW: carry the blend-mix factor in the output alpha without dual-source blend
A blend mix hands the blend unit exactly one number - the alpha factor, on the PS2's 0..2 scale where 128 is opaque. A second fragment output is the usual way to carry a value on that scale, but it is not the only one: fixed-function SRC_ALPHA reads the first output's alpha, and the shader can put the factor there instead. Two cases make that free. When the target holds its alpha double-scaled, the alpha the shader would write IS the factor - tfx computes both as C.a/128 under RTA correction - so scaling the target is the whole change. Otherwise the substitution is free whenever the pass writes no alpha at all, because the output alpha is discarded on the way to the target: a draw whose alpha is masked outright, or one whose alpha write has moved into a second pass under SPLIT_RGB_ONLY. Only the plain mix1 shape qualifies. The other mix cases rewrite the second output's RGB independently of its alpha, so there the two outputs really do carry different values and no substitution exists. Without this, a GPU with no dual-source blend emulates the equation in the shader, which needs a fresh destination read per primitive. With neither a texture barrier nor a multidraw framebuffer copy available, all it gets is one snapshot taken before the draw, so every primitive after the first composites against stale pixels. That is what hollowed out God of War II's menu glyphs on Mali r44p1, where the whole text is a single draw whose drop-shadow and bright quads overlap each other 200 times. Measured against a dual-source GPU rendering the same dump: over the text the mean per-pixel error falls from 3.351 to 0.109 and the worst pixel from 163 to 25, with the lit-pixel count landing on 3896 against the reference's 3898. Frame-wide it removes 25k of the 49k differing pixels and introduces 22. It needs no barriers and no target copies at all, where matching this by refreshing the snapshot per primitive group cost ~1000 render-pass breaks a frame and two thirds of the frame rate on device. No effect where dual-source blending exists: 33 frames across 11 dumps are byte identical. |
||
|
Brian Degenhardt
|
89e51d93a1 |
GS/HW: split RGB_ONLY alpha test by channel without dual-source blend
AFAIL=RGB_ONLY means every fragment writes RGB and only the ones passing the alpha test write A and Z. The accurate single-pass form of that carries the pass/fail decision in the second blend source, so it needs a hardware dual-source blend unit. Mali Vulkan stacks routinely report dualSrcBlend=false, and there the draw fell back to pass/fail: one pass for the passing fragments, another for the failing ones. Pass/fail splits the draw by *fragment*, which puts RGB in both passes. Where the primitives overlap each other, the two passes then composite out of order - every failing fragment of the whole draw lands after every passing one, rather than each primitive completing before the next begins. Splitting by *channel* instead is exact and costs the same two passes: run one pass with the alpha test off writing RGB, then one with the test on writing A and Z. Both passes see the primitives in order, so overlap stops mattering. Forced on over a dual-source GPU it reproduces the single-pass path byte for byte - 33 frames across 11 dumps, no differing pixels. Against that reference on a no-dual-source configuration it takes God of War II's pause menu from 2.064 to 0.893 mean per-pixel error. |
||
|
Brian Degenhardt
|
9d7f8c2376 |
Translations: restore the pt-BR plural forms for the save-state delete count
The Brazilian Portuguese update flattened "%n save states deleted." into a single string, but the message is declared numerus="yes", so its translation may only hold <numerusform> children — one per plural form of the language. Bare text there is a hard lrelease error, which stopped ninja and took down every Qt desktop build (Linux 4k/16k, macOS, Windows); Android and iOS pass only because they never run lrelease. Give the message back its singular and plural forms. All translation files now release clean. |
||
|
Brian Degenhardt
|
ebc4ee75f3 |
Merge pull request #563 from johnpetersa19/master
Complete Brazilian Portuguese graphics translations |
||
|
Brian Degenhardt
|
0daaf5a6f7 |
GameDB overlay: stop erasing upstream fixes the overlay never meant to drop
The mobile overlay layers onto bin/resources/GameIndex.yaml, and the loader clears-then-replaces each map rather than merging: an entry that lists one gsHWFix erases every other fix upstream sets for that serial. The file header states the invariant - each entry must carry the complete block - but nothing enforces it and nothing warns when it is broken. 115 serials were silently dropping at least one upstream fix. The bulk of it is one generation defect, not sync drift. Android used to carry a forked copy of the GameDB; |
||
|
Brian Degenhardt
|
ce3eac044e |
GS: stop taking a voluntary RT feedback read where it costs a render pass
An Ad blend with alpha writes masked can be substituted (Ad -> As) and run in
hardware if the draw reads the render target. The draw did not otherwise need
that read, so the substitution is only worth taking where reading is free.
The gate for "free" was !texture_barrier, written to mean D3D11, where the
fallback is a plain copy on an API with no render passes. It is equally true of
every driver carrying UseRenderTargetCopyForFeedback, where the fallback is a
per-draw copy bracketed by a render-pass break - the most expensive feedback
read we have. Widening that workaround to all of Adreno therefore handed those
drivers the whole optimization in its worst form, on thousands of draws that
never needed to read anything. This is the same regression fixed for the
framebuffer-fetch path in
|
||
|
Brian Degenhardt
|
b5415c8105 |
GS: stop a screenshot ending a GS dump that is already recording
A snapshot request and a running recording shared one frame counter. The screenshot hotkey asks for zero dump frames, so pressing it mid-recording zeroed the budget of the dump in progress and the next VSync closed it as though the user had asked it to stop. A single-frame dump request did the same thing one frame later. Both were silent; the file simply ended early. Two fields now, so a request cannot reach into a recording at all: one for what the queued request asked for, one for what the open dump still owes, written only when that dump is created. The two branches were also alternatives rather than independent, so the frame a screenshot landed on never reached the dump and two guest frames merged into one on replay. A recording now takes every frame it is open for -- except the one it was opened on, whose state went into the dump's header and whose replay therefore starts from the frame after. A dump request arriving while one records still cannot open a second dump, but it says so on the OSD instead of quietly writing only the screenshot. The decision is extracted to a header-only policy with the usual static_asserts, pinned by eight cases riding the GS test target. The truncation is reachable only from the hotkeys and the Big Picture button -- PINE's dump opcode was written to refuse rather than trip over it -- so the policy suite is the regression gate. Its refusal comment is updated: it now rests on not handing back a path for a file that will never appear, which was always the better half of the argument. |
||
|
Brian Degenhardt
|
727ffd7c7d |
Android: add a PINE toggle to Advanced settings
EnablePINE and PINESlot were already INI-backed and VMManager::ReloadPINE already starts and stops the server when they change, but the Android frontend never surfaced them, so there was no way to switch PINE on from the device. It sits beside the recompiler switches because it is the same class of control: a developer tool a player has no reason to find, next to the other things you turn on to diagnose rather than to play. The row states the address and, once enabled, the adb forward line -- the listener is on loopback, so it does nothing until a workstation bridges the port, and a port nobody tells you about cannot be bridged. The port itself gets no editing widget. The only reason to move it is running two emulators on one machine, which does not happen on a handheld, and a free-entry port field is a support burden for a knob nobody turns; it stays readable from the INI. It is still carried in the settings model so the row can state the real port rather than assume the default. Note the per-game merge is a full constructor, so a field omitted there resets to its default instead of inheriting: PINE is a process-wide server and cannot be per-game, so it is absent from the diff (no game file ever acquires the key) but explicitly carried through the merge. |
||
|
Brian Degenhardt
|
912b1d8f95 |
PINE: listen on loopback TCP on Android
PINE has never worked on Android. Every non-Windows platform binds an AF_UNIX socket under XDG_RUNTIME_DIR, falling back to /tmp; Android sets neither and has no /tmp, so Initialize() failed at bind() and the server simply never started. Nor is there anywhere better to put the socket. The writable directories on Android are app-private, and every client that would want to connect -- adb, a shell, another process -- runs under a different uid, so a socket placed there binds successfully and then admits nobody. Loopback TCP is the transport Android does support reaching into: adb forward bridges a device port to a workstation. PINE already speaks TCP because Windows has always needed it, and the wire format is identical, so this is a matter of selecting the existing branch rather than writing a new one -- hence PINE_TCP_TRANSPORT, which separates "which socket family" from the two Windows API questions (the SOCKET handle type, winsock startup) that _WIN32 still owns. SO_REUSEADDR comes along on the POSIX side: relaunching the app is the normal debugging loop on a handheld, and without it a killed process leaves the port in TIME_WAIT and PINE looks broken for a minute with nothing explaining why. |
||
|
Brian Degenhardt
|
7af3929992 |
GS: move the Mali r44p1 self-read gates into the driver-bug database
The r44p1 blob cannot survive reading the render target in-tile, in any
spelling: on Vulkan it loses the device outright, on GL the same silicon
corrupts the frame instead. Three hand-rolled substring searches encoded that
one fact -- one in the GL backend testing GL_VERSION, two in the Vulkan backend
testing driverInfo -- while the database that exists precisely for this already
modelled it as UseRenderTargetCopyForFeedback, described in its own definition
as being for "drivers where no form of attachment self-read works".
So express it as two rules, one per API, and read them:
- GL takes the workaround bit in place of its GL_VERSION search.
- The Vulkan texture_barrier gate is deleted outright. It was setting
m_features.texture_barrier = false sixteen lines below a table-driven block
that now sets exactly the same thing for the same driver -- pure duplication
once the rule exists.
One deliberate behaviour difference: the table-driven path respects
OverrideTextureBarriers, which the hand-rolled test ignored. The comment above
it documents forcing barriers on as the way back to the in-tile path for A/B
work, so honouring that is the intent rather than a regression.
Rules match a PARSED driver revision, which is what lets them say "exactly
r44p1" instead of "contains r44p1" and what makes the next bad blob a table row.
It is also the risk: a rule that matches nothing looks perfectly healthy and
puts the device back on the faulting path with no diagnostic. Hence the new
tests, which drive the resolver with the exact strings the RG 477V reports and
assert the outcome -- plus the neighbouring revisions r44p0, r44p2, r45p1, r38
and r52, which must keep the fast in-tile read.
Desktop GL is unaffected (the profile only resolves on Android, so the
workaround bit is never set there); verified through gsrunner that framebuffer
fetch is still selected. 53/53 GS tests.
Two r44p1 gates are deliberately left alone for now: both run during device
creation, before the Vulkan profile is resolved in CheckFeatures, so they need
that resolution moved earlier first.
|
||
|
Brian Degenhardt
|
4db909d0ad |
GS: say what died when the host GPU device is lost
A lost device is almost always the driver refusing something we asked it to do, and the ask lives in the feature set rather than in the crash. The Mali r44p1 blob is the worked example: it loses the Vulkan device under attachment-feedback-loop and mishandles in-tile framebuffer fetch on GL, both of which are the accurate-blending destination read. Neither is deducible from "host GPU lost", and nothing else in the log restates which blend path the device picked -- that is decided from driver strings at startup and never mentioned again. The second-loss-within-15s guard makes this worse than it looks. Recovery rebuilds the identical device, so a configuration the driver cannot survive reaches the guard deterministically: the second loss follows the first within a frame or two. The guard aborts before the OSD warning is raised, so from the user's side it is an unexplained crash, and the abort message named neither the GPU nor the driver version nor anything about the blend path. So log the driver identity, the renderer, the destination-read path in words, the features behind it, and the settings that steer them -- once at the loss, and again in both abort messages. Captured before the recovery path destroys the device, which is the last point at which any of it can still be read. No behaviour change: recovery still rebuilds the same device. Automatic demotion was considered and rejected -- falling back silently is what stops the bug report reaching us, and these reports are the only signal we get from hardware we do not own. |
||
|
Brian Degenhardt
|
393cb544e0 |
GS/OpenGL: fall back per draw, not per primitive, when GLES has no barrier
A GLES device has no ARB or NV texture barrier, so CheckFeatures sets multidraw_fb_copy and the backend substitutes a render-target copy taken once per primitive group inside a full-barrier draw. That is the right substitute on an immediate-mode GPU, where a blit is a blit and the per-primitive copy buys real blend ordering. On a tiler it is not a copy at all: reading the render target back forces the tile to flush and resolve to main memory, so a draw with a few hundred primitive groups pays a few hundred full-screen flushes. Nothing noticed because the flag is inert while there is a barrier, and on GLES framebuffer fetch supplies one. Where fetch is off it becomes the whole blend path -- and fetch is off on exactly the devices least able to afford it: the Mali r44p1 blocklist, a user who disabled fetch, or a GLES stack without the extension. Metal Gear Solid 3 on an Anbernic RG 477V (Mali-G615 MC6, r44p1) ran at 0.33 fps. The same game on the same device runs at ~30 fps on Vulkan, which reaches the identical copy-based concept only without this flag -- Vulkan, D3D12 and Metal all clear it unconditionally. So clear it on GLES too when the barrier does not materialise. GSRendererHW then sees no feedback loop, drops require_full_barrier, and the backend takes one render-target copy per draw. Measured on device: 0.33 fps to 23 fps, and the frame is clean. The accuracy cost is real and worth stating. Against the software rasteriser on a 640x480 MGS3 frame, the fetch path is 0.245% of pixels off by >=8 and the per-draw copy is 2.399%. Losing fetch itself accounts for none of that (0.247% with fetch off and barriers intact) -- it is entirely the dropped per-primitive ordering. That is the same trade every barrier-less backend already ships, and it is not really a trade against 0.33 fps. Only the auto path decides this; both OverrideTextureBarriers branches keep clearing the flag themselves, so Force Disabled still means no copies rather than a different kind of copy. Desktop GL is untouched -- verified unchanged at 753/759 differing pixels either way. gs_vertex_tests 48/48, including four new cases pinning the fallback shape. |
||
|
Brian Degenhardt
|
84f7c33822 |
GS: derive fetch-orders-overlap where it cannot go stale
The Vulkan backend derived framebuffer_fetch_orders_overlap from framebuffer_fetch immediately after the first assignment, but three later statements still write framebuffer_fetch -- the RT-copy workaround's texture_barrier mask among them. On Adreno that mask clears fetch, and the derived bit kept the value it had beforehand, so the device came out advertising no framebuffer fetch and "fetch orders overlapping primitives" at the same time. Nothing reads the stale value today: DetermineBarriers is the only consumer and it sits inside an `if (features.framebuffer_fetch)` gate, so the contradiction is unreachable. That is a property of the current single call site, not of the bit, and it is the kind of guarantee a second reader removes without noticing. Derive it after the last write instead. Record what the Turnip source says about the contract while the bit is being explained, because the file already carries a measurement that reads like a counter-example and is not one. Turnip does request the ordering when tiled (SINGLE_PRIM_MODE = FLUSH_PER_OVERLAP under rasterization-order access, which the a6xx docs define as waiting for overlapping primitives); what it only sets untiled is the stronger mode that additionally keeps UCHE and CCU in sync when fetching the current pixel's previous value. So the Adreno failure recorded above is read visibility while tiled, not primitive ordering, and it does not generalise to a tiler whose fetch is a genuine tile-local read. OpenGL never assigned the bit at all and took false from the FeatureSupport memset, which is the value it wants -- GL fetch does not order overlapping primitives, which is why the flag exists. Say so explicitly: Vulkan and Metal both assign it, and the one backend that stays silent reads as an omission rather than as an answer. No behaviour change on any backend. gs_vertex_tests 48/48. |
||
|
Brian Degenhardt
|
7668167aa1 |
Counters: mark the savestate poison-repair blocks DELETEME after 2026-12-01
The load-time repairs in rcntFreeze/psxRcntFreeze exist only to heal .p2s
files saved by builds that predate the trigger fix (
|
||
|
Brian Degenhardt
|
c77ed879a3 |
eerunner: add EERUNNER_EXITSTORM and EE cycle-hack knobs to liverun
EERUNNER_EXITSTORM=<period_us> spawns a thread that fires Cpu->ExitExecution() at randomized intervals during a liverun, mimicking the Android JNI pause/suspend churn (native-lib calls it cross-thread against a running EE). This is what reproduced the God of War II poisoned-timer trigger on a desktop within 1500 frames, and what verified the fix clean over a denser 3000-frame storm. EERUNNER_EECYCLESKIP / EERUNNER_EECYCLERATE apply the EE cycle speedhacks so a handheld's clock shape (the Android Low-End preset ships cycle skip 1) is reproducible on the desk. |
||
|
Brian Degenhardt
|
abe076fb2c |
Counters: warn loudly when a counter baseline sits ahead of the clock
The baseline-ahead guards added in
|
||
|
Brian Degenhardt
|
9f6288531d |
EE/arm64: make recSafeExitExecution safe to call cross-thread
The Android pause/stop JNI calls Cpu->ExitExecution() from the UI thread
against a running EE. recSafeExitExecution carried two accelerants inherited
from the x86 recompiler alongside its exit flag: it zeroed
cpuRegs.nextEventCycle when the EE was outside the event test, and folded
psxRegs.iopCycleEE into iopBreak when inside it.
Both are data races from a foreign thread, and the first one is how God of
War II savestates got their poisoned timers. The arm64 JIT pins the cycle
counter as a delta (RECCYCLE = cycle - nextEventCycle) for the whole life of
a block chain, reconstructing the absolute clock as delta + nextEventCycle
at C-call seams. A cross-thread zero landing mid-chain makes the next flush
reconstruct cycle = delta + 0, warping the EE clock back to near VM birth —
observed as a 142-billion-cycle rollback in a live repro. Counter baselines
are then "ahead" of the clock, which the old u32 rcntSyncCounter arithmetic
turned into the +2^32 startCycle scar and blown count that rode along in
every savestate taken afterwards (see the Counters fix in
|
||
|
Brian Degenhardt
|
8e4aa15918 |
Android: run autosave save and load state on the CPU thread
|
||
|
Brian Degenhardt
|
4c57fbf49d |
eerunner: add --statereport, a field-level savestate timebase decoder
Loads a savestate through the emulator's own thaw path and prints every serialized timebase and transfer-engine state by name: EE cycle and the COP0 Count/lastCOP0Cycle pair, the four rcnts with baselines and derived game-visible counts, vsync/hsync phase, EE<->IOP skew, IOP counters, CDVD RTC, GIF path buffers, VIF, DMA channel registers, and the MTVU frozen atomics. Stuck-timer bugs live in the relationship between clocks that normally advance in lockstep; diffing two reports makes a broken pair legible where a byte-level diff of the .p2s cannot. First use found the GoW II poisoned-state scar on its first run: every field identical between a poisoned and a clean state except EE timer 0, whose baseline sat exactly one 2^32 epoch in the future. |
||
|
Brian Degenhardt
|
4e34e65b84 |
Counters: fix u32 blowup when a counter baseline sits ahead of cycle
rcntSyncCounter computed (cpuRegs.cycle - startCycle) / rate into a u32. With 64-bit cycle counts, a baseline even one cycle AHEAD of now (a transient state around savestate thaw and vsync-retime seams) underflows the subtraction, and the truncated quotient becomes change=0xFFFFFFFF: count += 0xFFFFFFFF and startCycle += 2^32 - rate, zero-extended into the u64. The counter is then dead until cycle crosses the bogus baseline (14.6s at EE clock), and the blown-out count drains at one overflow lap per pass for minutes afterwards - and the scar rides along in every savestate taken meanwhile. psxRcntSync had the identical pattern, where one epoch is 116.5s at IOP clock. This is the God of War II poisoned-savestate bug: the area-title banner stays stuck and gorgon-eye chest pickups freeze for ~6 minutes after loading an affected state, on every host that loads it. A poisoned state carries EXACTLY startCycle = (cycle & ~(rate-1)) + 2^32 on EE timer 0, byte-for-byte the arithmetic above. A/B from that state: 1200 frames on the old code still shows the stuck banner; with this change it clears. Guard the negative case (skip the sync; the counter resumes within one tick), widen change to u64, and repair poisoned baselines/counts when thawing a savestate so existing affected saves heal on load. |
||
|
Brian Degenhardt
|
3cda8a2e60 |
PINE: stop savestate slot from clobbering the gsctl socket slot
The loadstate/savestate positional was named "slot", which is also the global option selecting the PINE socket. argparse shares one namespace, so the positional overwrote it and `gsctl.py loadstate 1` dialled pcsx2.sock.1 instead of the emulator's socket, failing to connect. Give the positional its own dest and keep "slot" as the metavar, so the command line is unchanged. |
||
|
Brian Degenhardt
|
fd71bdf4ae |
Merge branch 'android-pad-modals'
Makes every modal in the Android UI reachable from a gamepad. Android windows
take focus and consume key events before dispatchKeyEvent, so every AlertDialog,
ModalBottomSheet and DropdownMenu was a dead end on a handheld: both exit
confirms, the hardcore confirms, BIOS and memory-card delete, the manager
error dialogs, the stick-target and macro pickers, the per-game sheet, both
overflow menus, and the memory-card and network text entry. 24 window-modal
call sites across 14 files, now zero, with a preBuild check to keep it that way.
Also carries the pause menu drawing its highlight from the nav registry it
actually moves, tab navigation along the axis the strip is drawn on (every
sub-700dp device walked it across the short axis), nav-layer containment so a
selection cannot step through a scrim onto the row behind it, and the raw NUL
in SettingsSearchOverlay.kt escaped so the file stops being invisible to grep.
This branch was PR #526, closed unmerged. An audit of all 19 commits against
master found none of them present, in whole or in part -- the window-modal call
site count on master was still identical to the merge base. Merged now with
jpolo's agreement.
Its save/load CPU-thread marshal was cherry-picked ahead of this merge as
|
||
|
Brian Degenhardt
|
435f8bd9fd |
Android: run save and load state on the CPU thread
Saving a state from the pause menu aborted every assert-enabled build. The screenshot the save embeds goes through MTGS::RunOnGSThread, which asserts it is on the CPU thread, and the JNI entry point ran the whole save inline on whatever thread the picker dispatched it from. Parking the VM first, which is what these two entry points did, is not the same guarantee. It stops the EE, but the MTGS ring's write position is single-producer and owned by the CPU thread, and the CPU thread does not stop producing when the VM is paused: its idle loop keeps draining Host::PumpMessagesOnCPUThread() every 16 ms, so any GS-settings apply or window resize queued from the UI pushes to the same ring the save is pushing to. Two producers claiming one slot drops a packet, and a dropped data-packet header leaves the GS thread parsing payload qwords as command tags. So marshal both entry points with a blocking Host::RunOnCPUThread, matching what commitSettings and changeDisc in the same file already do. The park stays: it stops the EE for the inline zip and holds the audio pause the picker is built around. Thread identity is what makes the ring pushes legal. The load path is fixed alongside it. It has the identical violation — Freeze on the way in, plus a recompiler cache reset — and goes unreported only because MTGS::Freeze pushes its packet directly rather than through RunOnGSThread. Its follow-up present moves into the same task, which also stops it racing the resume in the pause guard's destructor. |
||
|
Brian Degenhardt
|
d8e2741234 |
GS: keep the full barrier under framebuffer fetch when primitives overlap
DetermineBarriers dropped both barrier flags whenever framebuffer fetch was
available, on the reasoning that fetch makes them unnecessary. It does not.
Fetch replaces the destination *read*; whether it also orders overlapping
primitives *within* one draw is a per-backend property, and the blending path
depends on that ordering: it switches an overlapping draw to software blending
precisely because fetch is available ("on fbfetch, one barrier is like full
barrier") and requests a full barrier to get per-primitive ordering. Clearing
the flag here on the same reasoning removed the mechanism that supplied it, so
a primitive blended against a destination its predecessor had not written yet.
The two decisions were reading one feature bit as the answer to two different
questions, and only the first is what GL fetch guarantees.
Measured on the GL arm (Mesa 25.3.6, Apple M2 Max, GL_EXT_shader_framebuffer_
fetch), replaying dumps through gsrunner and scoring every pixel against the
software rasteriser, which is an exact GS:
MGS3 76872 px wrong by >=8 -> 753 (RT-copy path: 759)
Katamari 8255, 2415 px >=64 -> 806, 30 px >=64 (copy: 8255)
FlatOut 2 -> pixel-identical to the copy path
Dirge of Cerberus 235 -> 234
It was also nondeterministic, which is what pointed at ordering rather than
arithmetic: 18% of the MGS3 frame changed between identical replays, 56464
pixels varying run to run, while the copy path was byte-identical across every
run. That is now zero.
Vulkan's framebuffer fetch *is* VK_EXT_rasterization_order_attachment_access
and Metal's is programmable blending; both order overlapping fragments by
contract, so they keep the barrier-free path and their render passes intact --
making them pay for a barrier would reintroduce the pass breaks that path
exists to remove. Only GL changes. Where a draw's own primitives do not
overlap the question is moot, since a live in-tile read and a pre-draw snapshot
are the same value, so the barrier is still dropped there on every backend:
76 of the 84 affected draws in that MGS3 frame.
The decision moves into GSFramebufferFetchPolicy.h beside the fetch decision
itself, for the same reason that one was extracted -- it is a rule about what a
capability does and does not imply, and it belongs somewhere a reader and a
test can see it whole.
|
||
|
Brian Degenhardt
|
03cb89a957 |
GS/OpenGL: pin the framebuffer-fetch decision against re-override
Nine cases over DecideGLFramebufferFetch, riding gs_vertex_tests -- the policy is header-only constexpr, so it needs no extra linkage, the same arrangement gs_interlace_policy_tests.cpp uses. The named cases cover the two vetoes surviving the Mali profile, the profile demotion, and the backend selection against what tfx_fs.glsl actually compiles. The two that matter most are the sweeps at the bottom, because they state the bug generically rather than by input: over all 64 combinations, "enabled" must imply that no veto applied, and the profile demotion must be identical across every value of the two veto inputs. A future block that re-decides fetch fails those whichever knob it reaches for -- which is the failure mode here, not any particular condition being wrong. Red-checked by reinstating the historical resurrection in the policy: 4 of the 9 fail, including both sweeps. Green with it removed; full suite 39/39. |
||
|
Brian Degenhardt
|
030d8a1ff0 |
GS/OpenGL: decide framebuffer fetch once, in a policy function
CheckFeatures decided m_features.framebuffer_fetch three times across roughly a hundred lines. The last of them -- the Mali profile block -- tested the raw GL_ARM_shader_framebuffer_fetch extension instead of the decision the earlier two had already made, and set the flag unconditionally back to true. So both the r44p1 driver guard and the user's DisableFramebufferFetch setting were undone a tenth of a millisecond after they ran, and there was no way to turn framebuffer fetch off on Mali GL from settings at all. The device log stated the contradiction in plain language -- "Mali r44p1: disabling framebuffer fetch" followed by "Active framebuffer fetch backend (Mali profile): ARM" -- which is why this is about where the decision lives, not about the condition itself. Move it to DecideGLFramebufferFetch in GSFramebufferFetchPolicy.h: one pure constexpr function, all inputs explicit, no GL types. CheckFeatures assigns m_features.framebuffer_fetch once from its result and nothing downstream writes that flag again. Two behavioural points fall out of separating them: - Turning fetch off no longer drags a Mali device to the PowerVR profile. The demotion is what it always was, a property of the extension set (a Mali profile that cannot reach the ARM shader path is on the wrong profile), but a driver blocklist or a user setting is a blend-path choice and must not swap in another vendor's tuning as a side effect. - With fetch off on Mali GLES, texture_barrier already resolves to false at the Auto branch above (ARB/NV texture barrier do not exist on GLES), so the non-fetch copy blend path the r44p1 comment intends is what actually runs. The block's own texture_barrier assignment was redundant in every reachable case and is now only a log line. The backend selection now mirrors tfx_fs.glsl exactly, including its `#elif HAS_ARM_SHADER_FRAMEBUFFER_FETCH` fallback for non-Mali profiles, and the reason fetch is off rides on the same line as the verdict. Verified on an M2 Max under Mesa (GL, EXT fetch): the setting-off arm now logs "backend (Generic profile): None (disabled in settings)" and the setting-on arm "backend (Generic profile): EXT/PLS". The Mali arm needs an Android build. |
||
|
Brian Degenhardt
|
75d78d8d5f |
Merge pull request #558 from caribbeanwebdev/gs-single-present-throttle-query
GS: query ShouldSkipPresentingFrame() only once per VSync |
||
|
Brian Degenhardt
|
9c0b679567 |
GS: delete the stale-frame diagnostic instead of caching around it
The present-throttle check is not a query. Answering "present" books this frame as the one that was displayed, so a second call inside the same throttle period answers "skip". The stale-frame diagnostic asked first, which is why the real present decision was always told it had just presented, and why the picture froze for as long as the throttle stayed armed. Caching the answer fixes this instance and leaves the trap armed for the next heuristic added to VSync(). Delete the diagnostic instead, because the column that needed the stateful call could never have carried a signal. It was written against a Retroid Pocket 6, where Vulkan advertises mailbox, so the throttle check exits early and that counter always reads zero. Where it could read non-zero - Metal, a driver without mailbox, or the mailbox-disable setting - asking is what freezes the picture. The other two columns read locals that are already to hand. That takes the Console include and the deliberate-skip flag, which had no other reader, with it. jpolo, who wrote the diagnostic, agreed to its removal. Warn at the declaration so the next caller does not have to rediscover any of this. |
||
|
Brian Degenhardt
|
6445e762cb |
Merge pull request #551 from sunshineinabox/tarball-version-fallback
cmake: allow setting version as fallback |
||
|
Brian Degenhardt
|
eca78074c9 |
GitHub: soften the AI-assistance question in the PR template
The template was inherited from PCSX2, where the AI section is a yes/no question a contributor must answer and a link to upstream's LLM usage policy. That policy is not ours to enforce, and the framing treats AI-assisted work as something to be declared rather than reviewed. Replace it with an optional note. The requirements that actually matter apply to every PR regardless of how it was written: the author understands the change, can explain why it is correct, and has built and tested it. |
||
|
Brian Degenhardt
|
9d202e5ffc |
GS/Vulkan: document why the feedback clone cannot be reused across draws
A cross-draw snapshot cache for the one-barrier RT clone was fully built (validity tied to the render pass staying open, per-draw written-area tracking) and verified byte-exact on ten GS dumps -- and reused the clone zero times in ~4,800 feedback draws. PS2 feedback chains read the bytes the previous feedback draw just wrote, so the snapshot is stale by construction; the same byte dependency rules out batching several copies into one pass break. Record the negative result next to the pass-break one so the bracket-per-feedback-draw shape is not re-attempted. |
||
|
Brian Degenhardt
|
37b9d0f681 |
GS/Vulkan: document why the Turnip RT copy cannot become a pass break
The copy-per-feedback-draw workaround looked replaceable by ending the render pass before each reading draw and sampling the live attachment: on this driver an in-pass read provably returns render-pass-start content, which after a fresh break is exactly the pre-draw snapshot the copy exists to provide. The replacement was built and it is byte-exact. It is also 2-5x slower, and the reason closes the question for good: this workload's speed lives in Turnip's untiled sysmem NO_FLUSH mode, which the bandwidth autotuner picks for most of these small passes and which the copy path itself depends on. A live self-read in that mode is a data race, and every mechanism that fixes it - declaring the feedback loop, the rasterization-order pipeline flag, pinning tiled rendering - abandons NO_FLUSH and lands at the same 2x-5x cost. The copy is the unique shape that is correct, deterministic and compatible with the fast mode, so its measured per-draw cost is not recoverable. Every cell of that map was measured on the SD865 (Adreno 650, Mesa 26.1.2), including run-to-run determinism; the numbers are in the comment. |
||
|
bmdhacks
|
8f67945c9e |
Fix: RSQRT.S's zero-divisor sign comes from the dividend
Two sign rules, and they are not the same rule. DIV.S takes the xor of both operands. RSQRT.S takes the DIVIDEND's sign alone -- it divides by sqrt(|Ft|), so the divisor has no sign left to contribute by the time the division happens. Both of our engines took Ft's sign, and the arm64 emitter was alone among recompilers in it: x86 recRSQRThelper1 (iFPU.cpp) has always taken Fs's. The console rows that separate the rules: rsqrt(+0, -0) is positive and rsqrt(-0, -0) is negative on silicon; an xor rule, or Ft's sign, flips both. Fixing the sign moves the arm64 emitter's agreement with the console and with the x86 JIT, and keeps the two local engines in exact agreement on the whole zero path. The MAGNITUDE stays at the fast tier's +/-fMax saturation. Silicon returns 0x7FFFFFFF there -- the EE's real maximum, one binade up -- but that is the top-binade compromise shared by every fast-path op, not the sign rule, and it moves as a class or not at all. Pinned by EeRecFpuRsqrt.ZeroDivisorSignComesFromTheDividend, six rows across both zero-sign combinations and nonzero dividends, both engines diffed. DenormalDivisorTreatedAsZero's expectation flips to the new rule. Idea by pstef. |
||
|
bmdhacks
|
665533c738 |
Fix: MAX.S/MIN.S select a word on silicon, the fast path computed one
The EE does not take a maximum, it picks one: the two raw words are
ordered by (sign, magnitude) and the winner's 32 bits are written
through untouched. That is fp_max/fp_min in FPU.cpp, and it is what the
interpreter and the DOUBLE tier have always done.
The arm64 fast path clamped both operands to +/-fMax and then used
Fmaxnm/Fminnm, which loses two whole operand classes:
denormals Fmaxnm/Fminnm are arithmetic ops, so FPCR.FZ flushed the
operand first and the winner's word was destroyed:
max(0x00000001, 0x00000000) read back 0x00000000 where
the console says 0x00000001.
exponent 255 the clamp folded the entire top binade onto 0x7F7FFFFF:
max(0x7F7FFFFF, 0x7FFFFFFF) read back 0x7F7FFFFF where
the console says 0x7FFFFFFF.
Both classes are exactly what ABS.S/NEG.S carried until cbf04acba1, for
exactly the same two reasons.
Replaced with an integer ordering key, k(x) = x ^ ((x >>s 31) >>u 1),
compared signed and resolved with a Csel between the untouched
originals -- nine instructions, and no arithmetic for FZ to act on. The
scratch registers stay inside fpuEmitGuardedAddSub's contract
(w0/w1/w8/w9), so a resident FCR31 in the x2-x7/x14/x15 pool is safe.
Measured on the 1147-case SCPH-90000 capture, corpus v3:
MAX 28/66 -> 66/66, MIN 50/66 -> 66/66
whole corpus, result axis: 755 -> 809 of 1147
54 cases changed, 54 onto the console value, 0 away, 0 outside MAX/MIN
FCR31 axis unmoved at 978
and 66/66 on both ops in all five regimes measured: eeClampMode 0/1/2,
fpuFullMode, and DenormalsAreZero off. The interpreter column is
untouched at 1067 and remains the control.
CHECK_FPU_OVERFLOW now gates no arm64 emitter path at all -- SQRT.S gave
up its operand clamp in 1a09344ba6, ABS.S in cbf04acba1, and MAX/MIN
here. The knob is still live on x86 and still set from the GameDB, but
eeClampMode 0 and 1 emit identical code on this port. The liveness
witness that rode on it is retired in place, with that stated, rather
than replaced by one that cannot fail.
Four tests in ee_rec_fpu_tests.cpp asserted the clamp and are inverted
here; their premise was that the x86 JIT is the FPU-clamp oracle, which
the capture refutes (upstream's fast tier is wrong on 41 MAX and 22 MIN
of the same cases). New file ee_fpu_minmax_console_tests.cpp carries the
54 distinct console triples, the aliased register forms, the O|U clear
against capture rows 734/735, and the FCR31-residency hazard. It fails
on 3 of 6 tests against the unpatched emitter and passes on all 6 here.
Idea by pstef.
|
||
|
bmdhacks
|
5dbb14458d |
Fix: the fast path never cleared the O and U cause flags
The EE clears the O and U CAUSE bits (the sticky SO/SU survive) on every op that can raise them, whether or not it does: ADD, SUB, MUL, the four A-forms, the four multiply-accumulates, and MAX/MIN/ABS/NEG, which clear the pair and do nothing else. DIV, SQRT, RSQRT and MOV leave both alone. Measured on FCR31-seeded capture rows: ABS, NEG, ADD, ADDA, MADD, MSUB, MUL, MULA, MAX and MIN all read back 0x0183C079 where the console gives 0x01830079; SUB, SUBA, MADDA and MSUBA have no seeded row and follow on the interpreter's authority (checkOverflow/checkUnderflow/clearFPUFlags clear the pair on all fourteen). The arm64 fast path cleared the pair only on ABS/NEG, so an O or U raised by an earlier instruction stayed visible to every later cfc1 in the block. The interpreter has always cleared them, which made this a live JIT-vs-interp FCR31 divergence as well as a console one. x86 iFPU.cpp has the identical defect -- the clear is commented out at 13 sites. The clear goes FIRST in each emitter, before the op writes anything: the fast path raises neither flag today so the order is not yet observable, but an emitter that later learns to raise O must not have its flag wiped by a clear placed after it. One Bic on the block-resident FCR31 per op. RAISING O and U is a separate, harder obligation -- a correct raise needs the exact magnitude of the result, which a saturating single cannot carry -- and stays with the FULL tier and the DISABLED tripwires in the FCR conformance file. Pinned by EeFpuFcrConsoleConformance.EnginesAgreeOnTheOverflowFlagClear: fourteen clearing ops plus the four leave-alone controls, both engines, seeded with the capture's word. Idea by pstef. |
||
|
bmdhacks
|
ac88a41f95 |
Fix: ABS.S/NEG.S clamped operands the console passes through
The arm64 fast path clamped both results to +/-fMax. The EE does neither:
ABS.S is `& 0x7fffffff` and NEG.S is `^ 0x80000000`, which is what the
interpreter has always done, what the FULL path (DOUBLE::recABS_S_xmm) has
always emitted, and what silicon does. The clamp corrupted 22 of the 54
ABS/NEG operands in the first-party capture, in two distinct ways:
* exponent-255 in, +/-fMax out (16 rows). Those are ordinary large PS2
floats, not infinities -- abs(7F800000) is 7F800000, not 7F7FFFFF.
* denormal in, ZERO out (6 rows), on ABS only. Its clamp was an Fminnm,
an ARITHMETIC op, so FPCR.FZ flushed the operand before the compare
happened. NEG's clamp was an integer Smin/Umin and never did this,
which is exactly why the defect showed on one op and not the other --
and why an operand pool built only from exponent-255 patterns missed
it entirely.
Fabs and Fneg alone are correct and total: non-arithmetic bit operations,
no exceptions, no flush, payloads through with only the sign changed.
Found while removing SQRT.S's operand clamp (1a09344ba6) -- same finding,
one op over. Note the upstream x86 JIT is wrong on the same 22 rows; both
interpreters are right on all 54. This aligns our JIT with our interpreter
and with the console, and diverges it from upstream-x86, which is not a
cost when upstream-x86 is not the reference.
Second, independent defect in the same two emitters, fixed here because it
lives on the lines being rewritten: the fast path never cleared the O and U
cause flags. Interp ABS_S/NEG_S call clearFPUFlags(FPUflagO | FPUflagU) and
the FULL path emits ClearOUFlags; only the fast path skipped it, so an
overflow raised by an earlier op survived an ABS.S. Capture rows 729/730
seed FCR31 with flags set and confirm it against silicon: FCR31 goes
0183C079 -> 01830079, which is hardware's value.
Verified over the full 1147-case corpus, both engines, stock regime, on top
of the SQRT fix: 34 engine-cases moved, all 34 onto the silicon value, 0
away, 0 outside the two expected classes, 2260 identical. The 22 ABS/NEG
moves are arm64-JIT-only -- the interpreter did not move, which is the
control that its console rows were not quietly re-fitted.
EeFpuAbsNegClamp.DISABLED_JitMatchesConsoleInEveryClampMode is graduated.
Its console table gains 8 rows from the first-party capture covering the
denormal and signalling-NaN shapes ps2autotests does not reach, tagged by
source; the interpreter leg passes on those rows both before and after this
change, which is what validates the transcription independently of the fix.
EeRecFpu.NegSPreservesSignOnPoisonedNan pinned the second of three answers
this op has had (clamp losing the sign -> clamp keeping it -> no clamp). It
is rewritten to pin the console's answer and now runs the engine diff,
since its premise that no rec matches the interpreter no longer holds.
Idea by pstef.
|
||
|
bmdhacks
|
ef6d720e39 |
Fix: SQRT.S of an exponent-255 operand, by scaling instead of clamping
Exponent 255 is an ordinary binade on the EE -- no Inf, no NaN, and the
representable max is 0x7FFFFFFF rather than FLT_MAX -- so an exponent-255
operand never needed saturating. Both engines clamped it to +/-FLT_MAX
anyway (the interpreter inside fpuDouble, arm64 with an integer Umin gated
on CHECK_FPU_OVERFLOW, mirroring x86's xMIN.SS) and landed two binades
below the console:
sqrt.s 7F800000 -> 5F7FFFFF, silicon 5F800000
sqrt.s 7FFFFFFF -> 5F7FFFFF, silicon 5FB504F3
sqrt.s 7FC00000 -> 5F7FFFFF, silicon 5F9CC471
All three engines agreed with each other and none agreed with the console.
Agreement is a weaker property than accuracy and it was bought at the cost
of accuracy.
Both engines now compute sqrt(|Ft|/4)*2. sqrt halves exponents, so the
scaled operand (exponent field 253) and the doubled result are both
ordinary singles: this needs no wider format and so leaves the fast path
single-precision, which is what the fast path is for. 4 is an even power of
two, so its own square root is exact and the identity contributes no
rounding -- the sqrt remains the only rounding step. It is the same
power-of-two prescale ToDouble() already uses to carry these operands into
FULL mode, with the factor picked to suit sqrt.
Ungated, because there was no mode in which the old code was right: with
CHECK_FPU_OVERFLOW off the same operands came back as 0x7F7FFFFF instead,
wrong a different way. Nothing with exponent field <= 254 is affected --
the old Umin was already a no-op on those, the new branch is not taken.
The JIT lands on the silicon value on every exponent-255 shape, both
signs, plus the exponent-254 control -- expected values computed
independently by exact integer arithmetic (no host float), validated
against silicon on the six witnessed operands. The interpreter moves two
binades onto the same values except where the sqrt is inexact in single
precision: there it narrows under the ambient ChopZero rather than the
divide unit's round-to-nearest and sits one ULP below silicon. That gap
predates this change, is documented as CLASS 3 in the conformance file's
divergence list, and closes when the interpreter models the div-unit
rounding law.
RSQRT.S deliberately unchanged: its two clamped operands currently cancel
on rsqrt(2^128, 2^128), so unclamping only the sqrt breaks a row that is
right today. It is all-or-nothing and is a separate change.
The two conformance tests that pinned the clamp are rewritten to pin the
console value instead, keeping their anti-vacuity clauses and gaining an
exponent-254 negative control. EeFpuAbsNegClamp's liveness witness for
DisableFpuOverflow() rode on SQRT's gate; it moves to MAX.S, now the only
remaining CHECK_FPU_OVERFLOW-gated emitter path.
Idea by pstef.
|
||
|
bmdhacks
|
318dd102dc |
Tests: ABS.S/NEG.S against hardware — the EE never clamps them
The console says both are pure sign-bit operations. From ps2autotests
tests/cpu/ee_fpu/arithmetic.expected:
abs 7fffffff: 7fffffff neg 7fffffff: ffffffff
abs ffffffff: 7fffffff neg ffffffff: 7fffffff
abs 7f800000: 7f800000 neg 7f800000: ff800000
An exponent-255 operand comes back exactly, sign bit aside. The
interpreter reproduces every console row; the arm64 recompiler does not.
recABS_S_xmm and recNEG_S_xmm call fpuClampResultPositive/fpuClampResult
with no CHECK_FPU_* gate, so exp-255 operands all collapse to
±0x7F7FFFFF and eeClampMode has no effect whatsoever — x86 at least gates
ABS on CHECK_FPU_OVERFLOW, arm64 gates neither op on anything.
That defect is pre-existing (present at the merge-base), so the JIT leg
lands as a DISABLED tripwire, not a failing test. It fails on 30 of its
60 assertions today — the 5 exponent-255 rows × 2 ops × 3 clamp modes —
so it is live, not vacuous, and it should pass unchanged once the clamp
is removed.
Also promotes what was a printf-only measurement probe into assertions:
- InterpMatchesConsoleInEveryClampMode: enabled must-not-regress control
on the side that matches silicon.
- JitIgnoresEeClampModeForAbsAndNeg: pins the inertness itself, so wiring
the gate up fails here and points at the tripwire instead of going
unnoticed.
- DisableFpuOverflowReachesTheEmitter: liveness witness for the new
harness knob. DisableFpuOverflow() is observationally a no-op on
ABS.S/NEG.S precisely because they ignore the mode, so without this the
switch would ship with nothing proving it reaches the emitter. SQRT.S
is the discriminator — its operand clamp is gated on
CHECK_FPU_OVERFLOW, giving 0x5F7FFFFF clamped vs 0x7F7FFFFF not.
1551 pass / 0 fail / 27 disabled.
Idea by pstef.
|
||
|
bmdhacks
|
525355bc58 |
Fix: SQRT.S raises invalid on -0 and negative denormals
Both this branch's engines gated SQRT.S's I|SI on `exp != 0 && sign`. The
console gates on the sign bit alone, so -0 and the negative denormals -- which
flush to -0 and produce a perfectly ordinary +0 -- raise invalid-operation
there too. That gate is why exactly those two operand classes lost the flag and
nothing else did.
From the first-party capture that records FCR31 alongside the result,
cases 227 and 236:
sqrt 80000000 : console 00000000/01020041 both engines 00000000/01000001
sqrt 80000001 : console 00000000/01020041 both engines 00000000/01000001
Read across all 38 SQRT.S rows the rule holds without exception: every
sign-set operand raises 01020041, every sign-clear one leaves 01000001,
whatever the exponent. Case 248, a POSITIVE qNaN, raises nothing -- it is the
sign bit and not "is this operand strange".
x86's recSQRT_S_xmm has always tested MOVMSKPS's sign bit alone (iFPU.cpp:1767),
which is why upstream-x86-jit is the one column in the capture that answers
both rows correctly, and the arm64 FULL-mode DOUBLE::recSQRT_S_xmm already
tested the sign alone too and was unaffected. So the fix is a deletion on both
sides -- the Tst(0x7F800000)/B.eq pair in the arm64 fast path, and the hoisting
of the flag set out of FPU.cpp's negative-normal arm. Neither touches the value
path or the |Ft| clamp the exponent-255 rows depend on.
Commit 6e1c28f fixed the value half of case 227 (the interpreter returned
_FtValUl_ & 0x80000000 and so answered -0 where the console answers +0). This
is the flag half of the same two rows, and case 236 is a second witness that
commit did not know about.
Verified bidirectionally. EeRecFpu.SqrtSInvalidFlagFollowsTheSignBitAlone is
the ten-row sign x exponent matrix from the capture, each engine scored on the
full FCR31 word: before the patch the -0 and -MIN_DENORM rows fail on both
engines with 01000001 against 01020041 and the other eight pass; after, all ten
pass.
The six positive rows are controls, and because the fix is a deletion they were
checked live rather than assumed: with the sign test deleted as well, all six
fail on both engines and the four negative rows still pass. Without them, a
deletion that went one step too far would raise I on every SQRT.S and nothing
in the suite would notice.
EeRecFpu.SqrtSOfNegativeZeroIsPositiveZero had asserted the opposite -- "the
zero path is not the negative path: no I|SI" -- on nothing but the two engines
agreeing with each other. ps2autotests' sqrt.expected prints results only,
never FCR31, so it never supported that claim. Its value assertion stands; the
flag rule moves to the new test, and it keeps the one flag statement that is
still true, that SQRT.S never raises D.
recompiler_tests 1582 pass / 0 fail / 46 disabled, and the other four ctest
binaries rebuilt against the changed libpcsx2 and rerun: core_test 86,
common_test 31, gs_vertex_tests 21, mvu_progcache_versioning_tests 13.
Noticed in the same sweep and NOT addressed here, recorded as leads in the
capture's handoff: the arm64 JIT reports 01000001 on every RSQRT.S row in the
capture including ordinary-negative operands, where interp, x86-jit and
hardware all say 01020041 -- recRSQRT_S_xmm does contain the I|SI set, so that
looks like a lost flag write rather than a missing one. And hardware raises
I|SI, not D|SD, on RSQRT's 0/0 rows.
Idea by pstef.
|
||
|
bmdhacks
|
b3dfef14e3 |
Fix: interp SQRT.S of -0.0 returns +0.0, not -0.0
IEEE-754 says sqrt(-0) is -0, and the interpreter said so too:
_FdValUl_ = _FtValUl_ & 0x80000000;
The EE does not. ps2autotests tests/cpu/ee_fpu/sqrt.expected, captured on
hardware:
sqrt 80000000/-0.00: 00000000/+0.00
sqrt CF_NEGZERO: 00000000/+0.00
Both recompilers already agreed with the console by construction --
recSQRT_S_xmm takes |Ft| before the Fsqrt, so the sign is gone before the
zero case is reached -- which makes this an interp-vs-JIT divergence with
the interpreter on the deficient side.
Found by a randomized SQRT.S differential over signed zeros, +/-fMax and
full-range normals. It went unnoticed for as long as it did because every
hand-written SQRT.S case in the suite uses +/-4.0; the operand pool that
found it is going in with the next commit.
Bidirectional per the repo's evidence rule: the new test fails on the
unpatched tree with
fpr[2]: JIT=0x0 INTERP=0x80000000
and passes with the patch. Full suite 1518 pass / 0 fail / 22 disabled.
Idea by pstef.
|
||
|
bmdhacks
|
d80101329e |
Fix: SQRT.S clamps its operand in the arm64 fast path
recSQRT_S_xmm was the one emitter in iFPU-arm64.cpp that never clamped its
source. fpuClampInput has twelve call sites covering ADD/SUB/MUL/DIV/RSQRT and
the six accumulator forms; SQRT called it zero times. An exponent-255 Ft is an
ordinary large PS2 float, but it reaches the host as Inf, so Fsqrt returned Inf
and fpuClampResult flattened it to 0x7F7FFFFF -- two binades from the
interpreter's sqrt(fpuDouble(Ft)).
Found by the hardware capture landed in 47d910efa6, rows 44/45:
sqrt +EEMAX : console 5fb504f3 interp 5f7fffff jit 7f7fffff
sqrt 2^128 : console 5f800000 interp 5f7fffff jit 7f7fffff
Unlike the six operand-clamp rows beside them, these did not close under
CHECK_FPU_EXTRA_OVERFLOW -- there was no gate to turn on. That is what made it a
defect rather than the clamp-mode axis.
The gate is CHECK_FPU_OVERFLOW (eeClampMode >= 1, ON by default), not the
arithmetic family's CHECK_FPU_EXTRA_OVERFLOW. x86 recSQRT_S_xmm clamps at that
same lower threshold (iFPU.cpp:1777), and SQRT is alone in it: x86 gates RSQRT's
operand clamp on CHECK_FPU_EXTRA_OVERFLOW (recRSQRThelper1/2, iFPU.cpp:1835/1853),
which recRSQRT_S_xmm already matched, and every other x86 clamp reaches the FPU
through fpuFloat/fpuFloat2 under the same higher gate. Matching x86 rather than
DIV.S is what aligns all three engines in the mode games actually run in.
Direction per the standing rule: the interpreter was the side nearer the console,
so the recompiler moved. At eeClampMode 0 nothing is emitted, exactly as before.
One-sided, since Fabs has already made the operand non-negative -- the same
positive-only shape as x86's xMIN.SS. It is NOT Fminnm, which is what the first
cut of this used, and that was wrong: FPMinNum only prefers the number when the
other operand is a QUIET NaN, so a signalling operand goes down FPProcessNaNs
and comes back merely quieted, surviving the clamp. x86's MINSS returns src2 for
ANY NaN, and half of the EE's exponent-255 mantissa space is signalling, so
Fminnm covered only half the class the comment claims ("any Ft whose exponent
field is 255"). Measured exhaustively on this host over all 2^31 non-negative
operands, against a model of MINSS(x, +FLT_MAX):
UMIN mismatches vs MINSS: 0
FMINNM mismatches vs MINSS: 4194303 (first at 7f800001)
End to end, sqrt(0x7F800001) came back 0x7F7FFFFF where the interpreter -- whose
fpuDouble switches on the exponent FIELD alone, mantissa irrelevant -- gives
0x5F7FFFFF. Same for 0xFF800001 and 0x7FBFFFFF. The capture's three SQRT rows
are a qNaN, an Inf and a finite number, so nothing in it could reach the
signalling half.
So the clamp is done in the integer domain instead: the operand is post-Fabs, so
bit 31 is clear, and over non-negative floats the IEEE ordering IS the unsigned
integer ordering. Umin against 0x7F7FFFFF clamps Inf, sNaN and qNaN alike and
passes every representable finite value -- exact MINSS agreement on every input,
at one instruction. Umin has no scalar form so it is a 2S vector op; only lane 0
carries the operand and the scalar Fsqrt that follows zeroes the rest.
Kept as a SQRT-local helper rather than folded into fpuClampResultPositive,
whose other caller recABS_S_xmm emits its clamp with no CHECK_FPU_* gate at all,
where the interpreter and the console both leave exponent-255 operands alone.
That is a separate pre-existing defect whose fix is to delete the clamp, not to
change which wrong answer it produces; editing the shared helper would have
moved ABS.S's output for an unfixed case. Verified unchanged: ABS.S/NEG.S still
diverge on the same 30 of 48 rows (EeFpuAbsNegClamp.DISABLED_DumpAllLegs).
FULL mode was checked and does not share the gap: DOUBLE::recSQRT_S_xmm widens
through ToDouble, which carries exponent 255 across exactly, and
EeRecFpuFull.SqrtPseudoInfExact already pins the true sqrt(2^128) = 0x5f800000.
Its inline note about the fast path was describing behavior the fast path did
not yet have; it does now, so the note is updated with the measured value.
Verified bidirectionally. On the unpatched emitter with these tests present,
SqrtClampsItsOperandLikeTheRestOfTheFamily fails on rows 44 and 45 in both clamp
modes (interp 5f7fffff vs jit 7f7fffff) and passes on row 46, and
EnginesAgreeExceptOnTheDocumentedRows fails because the two rows no longer
belong on the allowance list. SqrtClampCoversSignallingOperandsToo sweeps every
exponent-255 shape in both signs and fails on exactly the three signalling rows
under Fminnm. With the patch all enabled tests in the file pass, and the console
tally is unchanged at 20 match / 19 value-only / 3 flag-only / 15 both -- the JIT
moved onto the interpreter's answer without changing what the file says about
the hardware.
The tripwire is promoted to an enabled regression test that asserts the value as
well as the agreement -- agreement alone could be reached by degrading the
interpreter, which is the side nearer the console here.
Idea by pstef.
|
||
|
bmdhacks
|
2951dd5fb0 |
Tests: run the EE harness in the FP environment a game runs in
The recompiler suite ran at FPCR 0 -- round-to-nearest, denormals live
-- while a real game runs 0x1c00000, FZ plus ChopZero, from
EmuConfig.Cpu.FPUFPCR. So the suite was answering questions about an FP
environment no player has.
Production's model is already consistent and every engine implements its
half: ambient is FPUFPCR, the EE FPU DIV/SQRT emitters swap to
FPUDivFPCR and back, the microVU dispatcher loads VU0FPCR/VU1FPCR and
restores FPUFPCR, and the VU micro interpreters scope-guard to the same.
Only the harness never established the baseline the rest of that model
assumes -- EeRecTestHarness's own comment said as much, and chose to
contain each JIT block's FPCR mutation instead. VuTestHarness had
already worked around the consequence by pinning both of its passes to
the VU FPCR; ScopedEeFpcr is the EE-side equivalent, and it establishes
rather than contains.
mVU's skip-the-FPCR-load-when-equal gate is the sharpest illustration:
it compares FPUFPCR against VU0FPCR and skips when they match, which is
only sound if ambient really is FPUFPCR. It was not, so the VU micro JIT
ran at the host default while the VU micro interp applied VU0FPCR.
15 tests then failed, none of them from an engine disagreeing with the
other in the environment they were written for. One root cause covers
most: round-toward-zero saturates an overflow to +/-FLT_MAX, so nothing
is ever Inf, and every path that infers overflow from Inf is inert --
the VU O flag, the EE FPU "unclamped intermediate product" cases, and
FCR31's overflow bit alike. FZ accounts for the rest by erasing the
mantissa the VU U bit is defined over.
Rather than disable them, the environment becomes an explicit per-test
axis: ScopedFpEnv, which rewrites EmuConfig's four FPCRs for its scope
so the whole stack agrees -- poking only the host register would leave
the baked FPUFPCR immediate and mVU's sentinel disagreeing with it. Two
kinds, both states a user can actually configure: IeeeNearest for the VU
tests, which need denormals to exist, and FlushNearest -- bit-for-bit
the default FPUDivFPCR -- for the EE FPU tests, which need Inf but are
built around FZ and diverge between engines without it.
No coverage is lost. One test is repaired instead of tagged:
EmptyDestMaskRetiresTheMacFlag now raises S off a plain -1.0, because
its subject holds in every FP environment and an underflow witness tied
it to one. One is added: ProductionFpEnvironmentErasesUnderflowAndOverflow
pins what a game gets -- the engines agreeing, on a value the console
contradicts -- so nobody re-derives the U/O work from a green suite and
concludes it is reachable in play.
Two findings the old environment was hiding get DISABLED tripwires,
both confirmed to fail when force-enabled:
- RSQRT_S is half-fixed. It rounds the sqrt to single but still
divides in double, and double-rounding a quotient is benign often
enough to vanish at nearest. Truncation is not so forgiving: at
ChopZero the interpreter lands one ULP above the JIT again, the
identical 0x3F5105EC/0x3F5105EB pair the original defect produced.
- FCR31 misses overflow on BOTH engines in production, reading
0x1000001 where the console says 0x1008011.
The second leaves a real question for hardware rather than for us: the
EE FPU truncates, so does silicon raise O from the magnitude of the
exact result, independently of rounding? A capture of FCR31 after an
overflowing ADD.S would settle it -- and the same answer decides the VU
O flag.
recompiler_tests 1517 pass / 0 fail / 22 disabled; core 86, common 31,
mvu_progcache 13, gs_vertex 21.
Idea by pstef.
|
||
|
bmdhacks
|
0d7e6df7fe |
Tests: FCR31 O and SO against hardware, as tripwires
pcsx2/FPU.cpp runs every EE FPU arithmetic op through checkOverflow(result, FPUflagO|FPUflagSO): an infinite result saturates to +/-fMax, raises O and the sticky SO, and returns early (so U keeps whatever it had); a finite one clears O and then clears U. ABS/NEG/MAX/MIN clearFPUFlags(O|U). DIV/SQRT/RSQRT pass 0 and must leave both alone. The recompiler's fast path models none of it, so an overflowing MUL.S leaves FCR31 reading a bare 0x01000001 where the interpreter -- and the console capture in ps2autotests tests/cpu/ee_fpu/fcr.expected -- say 0x01008011. Measured as a 27-row sweep over the whole class rather than the two rows the capture happens to cover: 24 of 27 diverge between the engines, and the interpreter matches the checkOverflow model on all 27. The three that already agree are the DIV/SQRT/RSQRT negative controls, and they are live ones -- the other 24 rows in the same table prove the probe can see an FCR31 change at all. The harness gains DisableFpuOverflow/EnableFpuExtraOverflow so the clamp-mode axis can be measured instead of assumed. An emitter that closed all 24 was written and measured, then reverted, and five tests go in DISABLED to record what it did not settle: DISABLED_EnginesAgreeExceptOnTheOverflowFlags DISABLED_EnginesAgreeOnOverflowFlagsAcrossTheArithmeticFamily DISABLED_OverflowFlagsComposeAcrossOneBlock DISABLED_ExceptionFlagsMatchConsole DISABLED_NanMathOverflowIsAnOperandClampModeDifference fpuEmitOverflowFlags detected overflow as `fabs(result) > FLT_MAX` -- that is, by sniffing for a HOST infinity, which makes an architectural flag a function of eeRoundMode. Measured on this host: under the shipping ChopZero default it never raises at all, and under eeRoundMode 1 or 2 it raises for ONE SIGN ONLY, because directed rounding produces 0x7f7fffff on one side and 0xff800000 on the other. It also fired on operations that are not overflows at all -- mul 2^128 by 1.0 or 0.5, add 2^128 + 0 -- contradicting the console on rows the JIT had got right. And it cost +8 host instructions per arithmetic op (MUL.S 3 -> 11) for a flag the x86 recompiler does not maintain at all -- every O/U write in pcsx2/x86/iFPU.cpp is commented out. The redesign should port iFPUd-arm64.cpp's ToPS2FPU_Full magnitude thresholds, which are round-mode and FZ independent, rather than test for a host Inf. Direction is unchanged: one correct engine against two, so the recompiler is the side that moves -- making the interpreter stop raising O/SO would align all three cheaply by destroying the only correct reference in the tree. Two things deliberately left, both recorded rather than papered over: - x86. pcsx2/x86/iFPU.cpp is in a separate CMake source list and is not built on this host, so the mirror could not even be compiled, let alone diffed against the interpreter. Its commented-out xAND lines are not the fix on their own either: they sit before the op and clear O|U unconditionally, which is only half of checkOverflow. - The underflow half. checkUnderflow can only SET U from a denormal result, and every FP environment PCSX2 runs the EE under has FZ set, so the host flushes one to signed zero before either engine looks. With FZ off the engines also disagree on the VALUE, which is the denormal work item; DISABLED_UnderflowFlagsNeedFzOff pins it. The fifth tripwire is a different question wearing the same clothes. "NAN math" feeds ADD.S two raw exp-255 words, so the engines compute different things before any flag logic runs -- interp clamps operands through fpuDouble and gets Inf, the fast path gets a host NaN. Turn on CHECK_FPU_EXTRA_OVERFLOW and the row aligns exactly, which is what DISABLED_NanMathOverflowIsAnOperandClampModeDifference measures: it attributes the row to the operand-clamp mode axis, a deliberate x86-JIT-parity compromise, instead of leaving it as an unexplained entry in a known-divergence list. It asserts FCR31 as part of that alignment, so it rides on the O/SO revert and is disabled with the rest; the row itself stays in kFcrEngineDivergences either way. Idea by pstef. |
||
|
bmdhacks
|
bf4e1089a0 |
Tests: EE FPU overflow against hardware — the max is 0x7FFFFFFF, not FLT_MAX
ps2autotests' fpu/fcr.cpp has run MUL.S(0x7F7FFFFF, 0x7F7FFFFF) on hardware all along, but prints the result with %f, so the only thing it ever recorded was the string "NaN". This captures the bits: 57 EE FPU rows and 8 VU0 macro-mode rows from a real PS2 over ps2link, every value a raw word. One rule accounts for every row: The EE FPU's representable maximum is 0x7FFFFFFF == (2 - 2^-23) * 2^128. Exponent 255 is an ordinary exponent -- there is no Inf and no NaN. Overflow means exceeding THAT, it saturates there, and only then are O and SO raised. So +FLT_MAX + +FLT_MAX is not an overflow on this machine: the exact sum is representable and the console returns it with FCR31 untouched. 2^127 * 2 is likewise fine; 2^127 * 4 is not. The generator asserts both halves of that in exact rational arithmetic across all 47 arithmetic rows, plus an underflow law (denormal operands flush to signed zero first, U follows from the flushed result), and rejects a capture that fails either rather than reshaping it. Both laws were confirmed live by corrupting the input. div 1.0/+0 is carried as a known-answer control, and the run is byte-identical across two resets. That max is one binade above what IEEE single can hold, which is why the fast path cannot match the console here however the flag test is written -- the host cannot represent the EE's top octave, so a result the console returns exactly necessarily arrives as a host overflow. The FULL double path can, and does. Also settles the VU half named in the same work item: VU0 saturates to 0x7FFFFFFF too and raises MAC O, and a row that overflows x, y and z while leaving w in range confirms the x=8 y=4 z=2 w=1 nibble layout. Nothing is "fixed" here. All three console divergences are shared by both engines and deliberate -- 19 rows are the +/-FLT_MAX saturation compromise, 3 are underflow U|SU needing FZ off, 15 are the overflow pair -- so they are recorded and left to the hardware-alignment stage. What is not deliberate, and is what the capture surfaced: SQRT.S is the only op in iFPU-arm64.cpp whose emitter never clamps its operand. fpuClampInput has twelve call sites covering ADD/SUB/MUL/DIV/RSQRT and the six accumulator forms; recSQRT_S_xmm calls it zero times, so an exponent-255 Ft reaches Fsqrt as a host +Inf and comes back 0x7F7FFFFF where the interpreter lands two binades away. Unlike the six operand-clamp rows beside it, this does not close under CHECK_FPU_EXTRA_OVERFLOW, because there is no gate to turn on. The interpreter is nearer the console on both rows, so the direction is to give SQRT the clamp the rest of the family has. Recorded as a divergence with a DISABLED tripwire, not fixed in this commit. The engine-agreement test asserts the listed rows still diverge as well as that the unlisted ones agree, so the allowance list cannot go stale silently. kEngineDivergences does not list rows 3, 11 and 16 (mul 2^128 by 2.0, by 1.0, add 2^128 + 0) even though they sit in the middle of the operand-clamp block they look like they belong to. They are not divergences: both engines return the same result word on all three, and the only thing that ever differed there was FCR31, which is the O/SO question deferred to the redesign -- see the DISABLED tripwires in ee_fpu_fcr_console_conformance_tests.cpp. The file says so in place, so the omission cannot be read as an oversight. Idea by pstef. |
||
|
bmdhacks
|
3dba206e8a |
Fix: FULL-mode RSQRT returned -0.0 for the largest magnitude
ToPS2FPU_Full has an arm for values the EE's top binade can hold but a
host single cannot: halve the double, narrow, add 0x00800000 back to the
single. Its guard was |x| >= 2^129, inherited from x86 iFPUd.cpp's
dbl_ps2_overflow. But the largest number this FPU has is 0x7FFFFFFF ==
(2 - 2^-23) * 2^128, a whole binade below 2^129, so everything in the band
(kEeFpuMax, 2^129) was routed into the halving arm when it should have
saturated.
Halved, such a value sits just under 2^128. Under the divide unit's
round-to-NEAREST FPCR the narrow rounds it up to a host infinity and the
+0x00800000 carries out of the exponent field into the sign bit:
0x7f800000 + 0x00800000 == 0x80000000
so the largest magnitude the FPU can produce came back as negative zero --
sign flipped and exponent field 0 rather than 255, which the NFS Carbon
corner (DivZeroOverZeroKeepsPseudoInfExponent) already established is
game-visible through guest softfloat classifiers.
Under the arithmetic FPCR the narrow chops to 0x7f7fffff and the arm is
correct, which is why only the ops that swap to FPUDivFPCR could reach it.
The interpreter's eeRoundToSingle is immune by construction -- it scales by
2^-4, and its comment says why: "the +4 lands on 255 exactly -- it can
never carry into the sign."
ONLY RSQRT REACHES THE BAND, which is why this survived. A DIV quotient
cannot: for 24-bit significands with a < b, a/b <= 1 - 2^-24 strictly, and
the band's relative width is exactly 2^-24. A sweep of the four reachable
exponent differences found 0 hits, and the first probe written for this --
DIV.S(0x7FFFFFFF, 0x3F7FFFFF) -- lands on 2^129 *exactly* and came back
correct, which is what sent me looking for the algebra. SQRT halves
exponents and cannot get near. RSQRT divides by a 53-bit sqrt result, so
the significand argument does not apply; a coarse sweep found 2.5M hits.
The fix is the bound, not the arm: compare against kEeFpuMax's double bit
pattern, with `hi` rather than `hs` because kEeFpuMax itself is
representable and the halving arm handles it exactly (halved it is
+FLT_MAX, and 0x7f7fffff + 0x00800000 == 0x7fffffff). Costs 2 extra
instructions to materialise the constant, on the cold toComplex arm; the
in-range path is untouched.
Verified bidirectionally: the new test's 5 pairs fail on the unpatched
source (jit 80000000, interp 7fffffff on all five) and pass after. The
liveness companion, DivKeepsTopBinadeResultsBelowTheEeMaximum, is green
both ways -- it holds 1.5*2^128 in the halving arm, so over-tightening the
guard down to 2^128 turns it red rather than letting the first test go
green for the wrong reason.
All six ctest binaries green on exit code: recompiler_tests 1640,
core_test 86, mvu_progcache_versioning_tests 13, gs_vertex_tests 21,
common_test 31, demangler_test. The 53 console-conformance and FULL-mode
tests (EeFpuOverflowConsole, EeFpuZeroDivisorConsole, EeRecFpuFull) pass
unchanged, so no capture row moved.
x86 iFPUd.cpp carries the identical constant (s_const DOUBLE(0, 1152, 0)
at :115, consumed at :185) and so has the same defect. Not touched here:
this is an aarch64 host, that column is never executed, and an unverifiable
port is not a fix.
Idea by pstef.
|
||
|
Brian Degenhardt
|
25ecdc704d |
GS: log primitive overlap and the RT-read predicate per draw
Two columns the drawlog was missing whenever the question was "which draws read the render target, and could their own primitives be feeding each other". fb_loop_rt is the predicate the Vulkan backend actually branches on when it decides to copy the target, so it is the only honest way to compare draw populations between configurations. The barrier column is not a substitute: framebuffer fetch clears the barrier flags outright, so a config with in-tile reads logs zero barriers while reading the target in more draws than the config that logs one barrier each. prim_overlap is the renderer's own answer, recorded as it stands - which is UNKNOWN for every triangle-class draw, since the exact test only runs for sprites or when a drawlist is being built. That is worth seeing rather than inferring. Diagnostic only; nothing reads either column. |
||
|
Brian Degenhardt
|
9f73c77d59 |
Android: run save and load state on the CPU thread
Saving a state from the pause menu aborted every assert-enabled build. The screenshot the save embeds goes through MTGS::RunOnGSThread, which asserts it is on the CPU thread, and the JNI entry point ran the whole save inline on whatever thread the picker dispatched it from. Parking the VM first, which is what these two entry points did, is not the same guarantee. It stops the EE, but the MTGS ring's write position is single-producer and owned by the CPU thread, and the CPU thread does not stop producing when the VM is paused: its idle loop keeps draining Host::PumpMessagesOnCPUThread() every 16 ms, so any GS-settings apply or window resize queued from the UI pushes to the same ring the save is pushing to. Two producers claiming one slot drops a packet, and a dropped data-packet header leaves the GS thread parsing payload qwords as command tags. So marshal both entry points with a blocking Host::RunOnCPUThread, matching what commitSettings and changeDisc in the same file already do. The park stays: it stops the EE for the inline zip and holds the audio pause the picker is built around. Thread identity is what makes the ring pushes legal. The load path is fixed alongside it. It has the identical violation — Freeze on the way in, plus a recompiler cache reset — and goes unreported only because MTGS::Freeze pushes its packet directly rather than through RunOnGSThread. Its follow-up present moves into the same task, which also stops it racing the resume in the pause guard's destructor. |
||
|
Brian Degenhardt
|
112838e5fc |
Android: walk the pause-menu tabs along the axis they are drawn on
The in-game menu's input controller navigated a layout the screen had stopped drawing: a vertical tab column on the left, content pane to its right. There are now two layouts and it is neither of them. Under 700dp — which is every handheld — the tabs are a horizontally-scrolling row above the content; wider than that they are a rail to the content's right. So the pad walked the strip across its short axis. Up and Down cycled tabs that run left to right, and Right stepped "into" a pane that sits below them. The same constants are wrong the other way round on the wide layout, where entering the content means moving Left, off the rail. Make the axis a property of the layout rather than a constant. The one place that decides `compact` now publishes it, and the mover walks the strip along its own axis and enters the content in the direction the content actually lies. Leaving the pane mirrors that, which frees the other axis to adjust values the way it does on every other registry-driven pane. |
||
|
Brian Degenhardt
|
075be92b5a |
Android: delete the dead nav API and guard against window modals
Two kinds of cleanup, both consequences of the branch. The registry sheds what no longer has callers: the scope begin/end pair and its field, the item count, the has-items test, and the 1D stepper. The stepper is the one worth naming. Every router path is spatial now, and an index step is the wrong model for a 2D surface -- it walks registration order, which matches visual order only by accident. Its two callers both meant "highlight the first control", so they say that instead. A doc comment describing how the memory-card dialog consumed keys is gone too; that dialog no longer exists. And a Gradle check, wired into preBuild, that fails on any use or import of AlertDialog, ModalBottomSheet, DropdownMenu or the window Dialog. A check rather than a test because CI runs an assemble and never runs tests, so a test would need a workflow change and would still be skippable locally. The failure message carries the reason -- a focused window eats gamepad keys before the dispatcher runs -- because the next person to hit it will be adding a perfectly reasonable dialog and needs to know why it is refused, not just that it is. There is a commented allowlist for a genuine exception, currently empty. It earned itself immediately: a stale AlertDialog import in the settings hub with no call site behind it, which every human pass over this branch had missed. Two rules it cannot check live in the primitive's docs instead: no lazy lists inside modal content, and never AnimatedVisibility around it. |
||
|
Brian Degenhardt
|
3b1652fd10 |
Android: route address and card-name entry to the on-screen keyboard
The last three window dialogs, all text entry. Two of them did not need a modal at all. The network address rows and the HDD filename row now open LibraryKeyboard directly, which is what their sibling LocalLinkRow in the same file has been doing all along, with a comment explaining exactly why. Those rows needed the change twice over: the dialog swallowed the pad, and the row underneath carried no registry registration either, so it could not be reached to open the dialog in the first place. Seven address rows become navigable and two dialogs disappear. The HDD row takes D-pad Left to reset, matching how every other row uses left/right on the focused control. The memory-card create form does become a real modal, and it is the clearest instance of the trap this branch keeps finding: every control in it already carried a controllerFocusable id. They registered from inside a dialog window, so the ids joined the registry and reported positions the pad behind could land on, while the form answered nothing. Not one id changes here -- they simply start working. Its name field takes the keyboard on live update rather than treating the keyboard closing as the done signal, because the panel outlives the keyboard and the draft has to stay visible on the row behind it. No AlertDialog, Dialog, DropdownMenu or ModalBottomSheet remains anywhere under com/armsx2. |
||
|
Brian Degenhardt
|
0ce5d36218 |
Android: replace the two overflow menus with anchored panels
The library's and the BIOS manager's menus were DropdownMenus, so every row in both was pad-dead. The library one is the worse loss: it holds sort order, cover style, custom and English titles, show-hidden, the background picker and Exit -- most of the library's settings, none of them reachable without a touchscreen. Both keep their position. A menu that belongs to one button has to look like that button's menu, not a prompt about the whole screen, so the primitive gains an anchor: a root-space point the panel pins its top-left to, clamped so one opened near an edge stays on screen. The trigger reports its own bottom-left through onGloballyPositioned, the same mechanism the focusable modifier already uses to track rows. Their scrim is lighter than a prompt's for the same reason. Rows derive their nav id from their label, which is unique within each menu -- that registers all twelve library rows without threading an id argument through twelve call sites. That is the last DropdownMenu and the last ModalBottomSheet in the app. Three AlertDialogs remain, all of them text entry, and they are the next commit. |
||
|
Brian Degenhardt
|
48b4298cf6 |
Android: replace the per-game sheet with a bottom panel, and bind X to it
The per-game menu was a ModalBottomSheet -- its own focused Android window, so none of its six rows could be reached with a pad. It keeps its look: still rises from the bottom edge, full width, rounded at the top, grab-handle silhouette intact. Swipe-to-dismiss is the one thing genuinely lost; B and a tap on the scrim both close it. X on a highlighted cover now opens that menu instead of jumping straight to the game's settings. The shortcut was not wrong so much as narrow: settings is one of the menu's six rows, and while the menu was a sheet the other five -- play, per-game BIOS, pin to launcher, hide, drop from Recents -- had no controller route at all. Anyone without a touchscreen simply could not reach them. Settings is still one press away as the second row, so the shortcut costs one A to keep. The menu's visibility is HomeScreen's own composable state, so the input controller takes a callback for it rather than trying to hold it, the same shape as its existing drawer hook. |
||
|
Brian Degenhardt
|
8c50d9de6b |
Android: make the stick-target and macro pickers pad-navigable
Both were AlertDialogs, and both are lists the pad has to walk, so they were the worst of the set: not merely a button you could not press, but a whole list of choices with no way to reach any of them. Each becomes an inline panel with every row registered, so the selection walks the list and A picks. Deliberately a plain Column with verticalScroll rather than a LazyColumn: the nav registry only knows about rows that are actually composed, so a lazy list would hide everything past the viewport from the pad while looking correct on touch. Both lists are bounded -- a fixed button set plus the hotkey enum, and the macro target set -- so composing all of it costs nothing. The macro rows also take Left/Right to clear and set, which is what every other toggle in the app does. A row should not behave differently because it happens to be inside a panel. Test both paths here especially. These are the only converted sites where the list is long enough for held-direction repeat to matter, and the two ladders repeat through different mechanisms -- a timer on the motion path, repeat-count on the key path. |
||
|
Brian Degenhardt
|
b27ba2e209 |
Android: escape the NUL delimiter in the settings-search index
SettingsSearchOverlay.kt held a raw NUL byte inside a string literal -- the delimiter distinctBy uses to join a label to its category. Perfectly valid Kotlin, and identical after compilation to the unicode escape it becomes here. The cost was entirely on the tooling side, and it was not small. Git classifies the file as binary, so `git grep` skips it in silence and `git diff` renders every change to it as "Bin 8025 -> 7920 bytes". A search for a call site came back empty during this branch's work for that reason alone, and the previous commit's one-line change was unreviewable in the diff. A file no search can see is worse than a file with an awkward delimiter. |
||
|
Brian Degenhardt
|
c6887a6bb0 |
Android: drop the duplicate on-screen keyboard host
The settings-search overlay hosted the controller keyboard as well as the shared host did, so while search was open it was composed twice — two identical keyboards at the same position, drawn on top of each other. Invisible by construction, which is why it survived. Found while hoisting the hosts in the previous commit: the comment on the shared one asserted "exactly one, here", and it was not true. Now it is, and the assertion lives at the Compose root where it can be checked by looking in one place. Note for anyone grepping this file and finding nothing: it contains a raw NUL byte in a string literal (a delimiter in distinctBy), so git treats it as binary and `git grep` skips it silently. Predates this branch. `grep -a` sees it. |
||
|
Brian Degenhardt
|
07dae0d23f |
Android: make the setup-wizard error pad-navigable, and hoist the hosts
The wizard's error prompt was an AlertDialog like the rest, but it could not be fixed the way the rest were: the shared modal host lived inside WindowImpl.Window, and Window and the wizard are the two arms of one `if`. Nothing hosted inside Window exists during setup, so a modal authored by the wizard had nothing to render it. So both overlay hosts move up to the Compose root, where every arm of that branch can reach them. The on-screen keyboard goes with the modal host rather than being left behind, because it has exactly the same defect for exactly the same reason — and it is the second time: it once lived inside HomeScreen and vanished whenever the user navigated to Settings, a sibling destination that unmounted HomeScreen and its host together. That was fixed by moving it one level up from where it broke. One level up from the last breakage is not a rule. The Compose root is. Ordering is preserved deliberately: keyboard after the modal host, so a modal handing text entry over cannot draw on top of it, and both wrapped in ScaledUi since they no longer sit inside Window's copy of it. An overlay ignoring the UI Size setting while every screen behind it honoured it would read as a rendering bug. There was already a precedent for hosting at the root, three lines below where these landed: the friend-online banner, hoisted there because in a game the library is not composed at all. Same reasoning, same place. This is the one commit on this branch with real structural blast radius, which is why it is alone. It is also the only one whose failure mode is loud rather than silent — if the hoist is wrong, overlays do not appear at all, on any screen. |
||
|
Brian Degenhardt
|
187821938f |
Android: make the remaining confirmations pad-navigable
Five two-button prompts, all AlertDialogs and so all pad-dead: both exit confirmations (library toolbar and nav drawer), the BIOS and memory-card delete confirmations, and the achievements screen's hardcore toggle — the twin of the pause menu's, which is the bug this branch opened on. The library's exit confirm is the interesting one. It is authored deep inside the overflow menu's anchor Box, and the plan for this branch called for hoisting it out, because an inline scrim drawn there would clip to its container. That hoist turns out to be unnecessary: a modal is authored at its call site and drawn at the top of the window, so nesting depth stops being a constraint on where a prompt may be raised. Left in place, and it is the clearest demonstration of what the portal buys. The memory-card delete confirm shares the trap the previous commit found in its sibling: it registered controllerFocusable ids for both buttons from inside a dialog window. They joined the registry and reported positions the pad behind the dialog could land on, while the dialog itself answered nothing. Deleting them removes a hazard, not a feature. Nothing left that is only a confirmation. What remains is shaped differently — the setup wizard, the pickers, the sheet, the two overflow menus, and the text-entry prompts. |
||
|
Brian Degenhardt
|
6d6ef31a98 |
Android: make the manager acknowledge dialogs pad-navigable
Seven sites across six manager screens, all the same shape: something went wrong (or finished), here is the text, press OK. Every one was an AlertDialog, so every one was its own focused Android window and killed the pad for as long as it was up — on the screens where a pad user is most likely to be stuck, since these are what an import failure or a bad path actually surfaces. They collapse to one call each against a new NotifyOverlay: the same card as the confirmation, one button instead of two, and a scrolling height-capped body so a long error can be read to the end. The window dialogs simply clipped it. The settings info panel folds onto the same thing, deleting the copy the previous commit had to make of the card. A setting description and an error notice are the same object; keeping them as one is what stops the next fix landing on only one of them, which is the exact failure that commit had to repair between the two info-hint copies. Worth reading carefully in the memory-card diff: its message dialog registered controllerFocusable ids for its OK button. That looks like working controller code and never was — it registered from inside a dialog window, so the ids landed in the registry, reported positions the pad behind could navigate onto, and answered nothing. Removing them is the fix, not a loss of function. These are only reachable by causing the error they report, so most are verified by inspection rather than exercised. |
||
|
Brian Degenhardt
|
dfbc490c2c |
Android: make the settings info hint an inline panel
The "i" bubble on a settings row opened an AlertDialog, so it was its own focused Android window and the pad went dead while it was up. It also existed TWICE — an independent copy in SettingControls.kt — which is how the scroll-and-cap fix for long descriptions came to be applied to only half the app's settings rows. Both copies looked identical closed, so nothing pointed at the divergence. Delete the copy, export the survivor, point the other file at it. One change now fixes the bubble on every settings row in the app, and the rows that were still clipping long text (the updater switches, among others) inherit the fix for free. The panel carries one thing the primitive did not have yet. A modal whose only focusable is its Close button leaves a pad with nothing to move to, so a description longer than the panel was unreadable past the fold — the very defect the AlertDialog had, reproduced faithfully. So a modal can now declare a scrollable body, and the shared move function scrolls it when the selection has nowhere left to go. Directions are already swallowed at that point, so this costs nothing when a modal has no scrollable body. Opening the bubble still needs a touch: the "i" is not a nav stop, and making it one would add a phantom stop beside every settings row. Worth solving, but as its own decision rather than smuggled in here. |
||
|
Brian Degenhardt
|
b74dab7493 |
Android: let the hardcore confirm take the pad
The pause menu's own confirmation was an AlertDialog, so it was its own focused Android window and consumed gamepad keys before the Activity dispatcher — where every D-pad route in this app lives — ever ran. The prompt sat on top of a surface that IS pad-navigable, so it read as the controller dying the instant the confirmation appeared: the menu behind still moved, the prompt answered nothing, and the only way out was the touchscreen. This is the reported bug, and the first site to prove the whole mechanism end to end: the primitive, the layer stack, the layer discipline, and both router rungs. Worth checking by hand with the prompt up: press L1/R1. The tabs behind must not cycle. That is the high placement of the key rung earning its keep — the tab flick is handled in the dispatcher, well below it. |
||
|
Brian Degenhardt
|
d80571fdaf |
Android: draw the pause-menu highlight from the registry
The pause menu painted one selection and moved another. Its action rows took their tint from selectedAction, an index in the view model that reset on tab change and advanced only when a row was ACTIVATED, while the D-pad moved the nav registry, which drew its own focus ring. Two selections, two highlights, and the row you were pointing at was not the one lit up — which is the symptom this branch was opened for. The registry was already the real one: every grid row has registered a controllerFocusable id all along. So the fix is to read the highlight from it and delete the other model outright — the state field, both places that reset it, moveSelection, selectAction, activateSelection, and the hardcoded per-tab count table. None of them had callers left; only the highlight had survived, which is exactly why the two could disagree without anything failing loudly. Deleting that count table is worth it on its own. It duplicated each pane's row count as a literal, and its own comment records the last time they drifted: it read 4 against a list of 5, so the pad could not reach Close at all. Nothing derives a count now. One intended visible change: no row is tinted while focus is on the tab column. Previously row 0 was always tinted regardless of where the pad actually was, which is the same lie in a quieter form. |
||
|
Brian Degenhardt
|
74b361e014 |
Android: route the pad to the topmost modal layer
One rung per input ladder, serving every modal there will ever be. Not one branch per site: the two ladders are the reason modals break in the first place, and a per-site branch is exactly how a prompt ends up alive on a D-pad and dead on a stick. The ladders are not interchangeable. Key events carry the face buttons and shoulders — A and B arrive only there, on every device — while many of the handhelds we target report the D-pad as a HAT axis, so their directions arrive on the motion path instead. A modal wired into one ladder is half-navigable on hardware the author didn't have. Both rungs call modalNavMove, so they cannot drift: horizontal adjusts the focused control first and only moves when it has none, vertical always moves. Same semantics the registry already uses on base screens, so a widget behaves identically inside a modal and outside one. The key rung sits HIGH — above the L1/R1 settings-tab flick and the hold-BACK-to-exit block, and far above the library cover grid. That is the point of the placement: those are exactly what must not act behind a scrim, and precedence settles it structurally instead of scattering a modal term across three blocks that then have to stay correct as each is edited separately. It also means the cover grid can no longer steal the first press, with no edit to the home input controller at all. The two rungs are asymmetric about the on-screen keyboard, deliberately. A modal with a text field hands entry over to the keyboard and must not take it back until it closes. On the key path the keyboard's block sits BELOW the modal rung, so the exception is spelled out; the motion path's `when` is ordered and terminal with the keyboard branch above, so it gets the same precedence for free. The key rung swallows every key it does not handle, not just the ones it does. Face buttons exist only on that path, so it is the sole place they can be absorbed. Both visibility predicates gain the modal state. Without it a modal raised over a running game with nothing else up would never enter the motion ladder at all (its caller gates on controllerDrivesFrontend), gameplay hotkeys would keep firing behind the scrim, and the game SurfaceView would hold the focus the modal needs. Needs testing on BOTH paths — walk a modal with the D-pad, then again with the left stick — and step-counting on single taps: the key/axis double-fire guard elsewhere in this file is inert, so a device that emits both a key and an axis for one press will double-step this rung. |
||
|
Brian Degenhardt
|
ba19bcec22 |
Android: extract the pad-navigable modal primitive
A Compose Dialog / AlertDialog / DropdownMenu / ModalBottomSheet is its
own focused Android window, so it consumes gamepad keys before they can
reach the Activity's dispatchKeyEvent — which is where every D-pad route
in this app lives. Anything built on one is unreachable by pad, and it
fails silently: perfect on touch, completely dead on a handheld. The
confirmation overlay was written to dodge that trap, but only for itself.
Generalise it into PadModal, so the next two dozen conversions have one
thing to reuse instead of a pattern to re-derive.
A modal is now a portal. PadModal is composed at the call site, beside
the state that opens it and with the call site's values in scope, but it
renders nothing there: it publishes its content into a global stack that
PadModalHost, mounted once above every surface, draws. The split is
forced by where these prompts are raised — a row inside a settings tab,
a card inside the library grid — because a scrim drawn there clips to its
container and scrolls away with it.
Three properties are worth stating, because each replaces something that
was previously a rule somebody had to remember:
- Claiming the pad is a consequence of being composed, never something
a call site opts into. PadModal pushes its nav layer on enter and
pops it on dispose, and the host republishes that layer to its
content as the ambient LocalNavLayer. controllerFocusable now reads
that instead of taking a layer argument, so an unmodified ToggleRow
or slider dropped inside a modal layers correctly with no plumbing.
- Initial focus is a same-frame guarantee, not a retry. Rows register
from a SideEffect, and side effects run at the end of a pass in
recording order, so a claim recorded after the content is guaranteed
to see every row it just composed. The old bounded ten-frame loop
could quietly end with nothing selected and hand the first press to
the screen behind the scrim.
- Content republishes on every recomposition, so a captured closure
can never go stale — the same lesson the nav registry already
records for its rows.
The layer slot becomes a stack. Modals nest (the library's exit confirm
opens from inside the overflow panel), and the save-the-previous-value
idiom a single slot forces is wrong whenever two sibling subtrees
dispose in the order Compose happens to pick: the survivor keeps a layer
that is no longer active, so it looks fine and answers nothing. Entries
are removed by key for the same reason. Popping also restores the
selection the layer interrupted, so closing a row's panel returns you to
that row instead of to the top of the pane.
Pure extraction — no call site changes behaviour. The three existing
confirmation sites now route through the host, and the two rules the
compiler cannot check (no lazy lists, never AnimatedVisibility around
modal content) are documented on the primitive.
|
||
|
Brian Degenhardt
|
d4a71dab8d |
Android: remove the unreachable memory-card input routing
MemoryCardManager.visible was never set true anywhere in the tree — its
sole assignment is the `= false` in the router branch that reads it. So
the object was permanently false and everything gated on it was dead:
- a whole rung of dispatchKeyEvent (B/A/D-pad, plus a debug log that
fired on every key press while it was live),
- the matching fireNavMove branch,
- handleMemcardControllerMotion and its two axis-latch fields,
- the USB-keyboard forwarding guard,
- a term in controllerDrivesFrontend() and one in frontendCovers.
The real memory-card UI is a route (AppRoute.MemoryCardManager ->
MemoryCardScreen), which is unrelated to this object and untouched. The
dialog this rung was written for no longer exists in that shape.
Deleting it now, before the modal-routing surgery, keeps that surgery
diffing against a smaller dispatcher and makes the two input ladders read
visibly parallel — which is the property the modal rung has to preserve.
No behaviour change: every removed branch was unreachable.
|
||
|
Brian Degenhardt
|
cee6839a59 |
Android: honour the nav layer in every registry mover and reader
The controller registry has an exclusive-layer mechanism so that a modal can own the D-pad outright, but only the ordering pass and the 1D stepper ever consulted it. Spatial movement scanned the raw position map, while value adjust, activation and the focus-ring test all keyed off bare registration. With a layer active the selection could therefore step out through a scrim onto a row behind it, fire that row's handler, and light its ring. The router drives all four directions through the spatial mover, so the layer was dead on the one path that actually runs. Spatial movement now takes its candidates from the layer-filtered ordered id list. That also makes the scan deterministic: the position map is a HashMap, so a tie between two equally good candidates used to resolve differently from run to run. The selected-item lookup and the ring test now test layer membership rather than mere registration. The layer predicate needed hardening first. The map lookup yields null both for "not registered" and for "registered at the base layer", so the old form answered true for an unknown id whenever no layer was active. That was safe for its two previous callers, which only ever passed ids straight out of the registry, and wrong for the ones added here. Nothing observable changes yet except at the confirmation overlay, the only thing in the tree that sets a layer. The MCNAV debug logging went with the rewrite; it named a dialog whose input path is unreachable, and the rest of that path comes out next. |
||
|
Brian Degenhardt
|
957b3130c9 |
Fix: the unsigned MMIO load's zero-extend aborts every assert-enabled build
Both MMIO load paths — the const-paddr shortcut and the backpatch stub — widen an unsigned sub-64-bit handler return with Uxtw(x0, w0), mirroring the Sxtb/Sxth/Sxtw right above them. But the sign-extending three lower to sbfm, which genuinely accepts a narrow source, and Uxtw lowers to ubfm, which does not: UBFM has no W-source/X-destination form at all. Its 64-bit encoding is UBFM Xd,Xn,#0,#31 and the width is carried by the destination alone. vixl asserts the two operands match, and that assert is live in every Debug build and compiled out of Release. The emitted word is the same either way — the operand field holds only the register number and sf comes from Rd — so this assembles to exactly the UBFM X0,X0,#0,#31 that was meant. Release has always been correct, which is why no nightly ever showed it. An assert-enabled build instead aborts on the CPU thread the first time a game takes an unsigned load through an MMIO/handler page, which is seconds into a boot: it makes Debug unrunnable rather than wrong. Pass x0 on both sides. |
||
|
Brian Degenhardt
|
25fda735da |
GS/Vulkan: record what the Turnip in-pass read actually corrupts
The note above the RT-copy workaround said OutRun's sea failed while FlatOut's several hundred feedback draws did not, and that nobody had isolated the difference. Both halves are wrong. Scored against the software renderer, FlatOut 2 is the worst case measured - 31.3% of the frame wrong by more than 16 levels, against OutRun's 4.6% - and NFS Underground, the title the original texture-pack gate rested on, is corrupt across roughly 40% of the frame at an amplitude no eye catches. Replace the open question with the isolation result: two distinct failure mechanisms, one an ordering failure between draws in a pass and one a hazard inside a single draw that no pass boundary can separate; an explicit in-pass barrier that the driver ignores byte-for-byte; and the fact that most corrupted draws never sample the target at all, reading it for the destination-alpha test and the write mask instead. Comment only. |
||
|
Brian Degenhardt
|
143b9839db |
GS: land a carried-forward clear on the copy's destination rect
When DoCopyRect is handed a source that still owes a clear and a render target for a destination, it skips the copy and clears the destination instead. That shortcut was wrong in three ways, all of which only show up when the copy is partial and lands at a nonzero offset -- a full-target copy takes the early-out in ProcessClearsBeforeCopy and never gets here. Vulkan filled both members of VkClearValue, which is a union: the depth and the stencil landed on top of the colour's red and green. Every colour clear taken down this path arrived with those two channels replaced by the source's clear colour reinterpreted as a depth (clamped away to zero) and by the stencil's zero. Only write the aspect being cleared. Both backends then put the clear in the wrong place. VkClearRect is in framebuffer coordinates and the framebuffer is the whole destination, so dropping the destination offset put the clear in the target's top-left corner. D3D12 passed no rects at all, which clears the entire view. Give each the copy's destination rect. D3D12 also has to commit any clear the destination still owes before clearing part of it -- previously the whole-view clear stood in for that, and a partial one would have discarded it. Never observed in the dump library: the path is not taken once across ten titles at native and 3x. Verified against a synthetic copy from a cleared source into an offset region of a larger target, colour and depth, which reads back the two clear values swapped before the fix and correct after. |
||
|
Brian Degenhardt
|
8ee55f7039 |
GS: say so when native resolution turns an upscaling fix back off
The GameDB apply announces every fix it sets with "Enabled GS Hardware Fix", and at native resolution MaskUpscalingHacks then turns a subset of them straight back off again — silently. Nothing in the log ever contradicted the Enabled line, so a log read at face value overstated what was actually in force. On Rogue Galaxy at 1x it claimed halfPixelOffset, roundSprite and nativeScaling were on when all three were off, which is exactly what it looked like during the Rogue Galaxy work. The apply line is not wrong when it is printed; the second event was just never reported. So report it where it happens, which is the only place it is true by construction. Only fixes that were genuinely on get named. That keeps the line honest, and it also keeps repeat calls quiet: after the first pass there is nothing left to clear, so a settings re-apply adds no noise — measured as one line across a 40-iteration replay, and none at all above 1x, where the fixes legitimately stay on. Names match the GameDB ones so the two lines read together. |
||
|
Brian Degenhardt
|
9e34dc20a3 |
PINE: report what a setting is actually running as, not what the INI says
A settings query answered the wrong question. GameDB hardware fixes are applied to the live config after the settings load and are never written back to the file, so on any game carrying them the persisted value and the running value disagree — and the query only ever knew about the first. On Rogue Galaxy it reported autoflush off and preload off while the renderer was running autoflush at 2 and preload on. That cost real time during the Rogue Galaxy work. The confusion is the smaller half. The real damage is to measurement: a settings A/B that writes a key to some value measures the GameDB value in BOTH arms, because GameDB re-applies it after every settings load, while the two arms report two different settings. That is a wrong answer with no symptom, on exactly the titles worth investigating. So add an opcode that reports both values side by side. Effective values come from serialising the live config back out through the same wrapper that writes the INI, which means they land under the identical section/key names a caller already uses and every setting is covered without a key map — a hand-written map would need extending by every future setting, and the one that got missed would be the one somebody trusted. It also fixes a smaller lie: keys absent from the INI came back as empty strings, reading as "unset" rather than as their default. The reply says the two strings differ; it does not say why, because from here that is not knowable. A GameDB fix, safe-mode masking and a settings layer this query does not read are indistinguishable at the point of comparison, and naming one of them would be inventing the reason. The existing read is left alone, so anything speaking the old opcode keeps working. gsctl's `get` now reports the running value, prints the discrepancy to stderr where a human cannot miss it and a pipeline does not have to care, and keeps the on-disk value available behind --persisted. |
||
|
Brian Degenhardt
|
8b31dbce6c |
GS: let the pipelined split run with asynchronous HW downloads
The front-object split was refused whenever the EE thread services the readback
itself, which covers both Unsynchronized and Asynchronous. That groups the modes
by which thread reads, when the question is what it reads.
Unsynchronized takes GS local memory directly, no lock and no drain, so a queued
back thread leaves it arbitrarily far behind what the EE expects. Asynchronous
does not read local memory at all: it takes the CPU shadow under
m_async_readback_mutex, and the mutex is the synchronization point. The shadow
moves only when the GS thread publishes a completed GPU download, never when a
record is queued or executed, so queue depth cannot change what the EE sees.
Every shadow accessor already routes through m_mem_target, so a front object
reaches the back's authoritative copy - the plumbing was in place, only the gate
was wrong. The refusal was Unsynchronized-only when the split landed; the
asynchronous readback import widened it to the shared predicate.
Keep lockstep for the one case that does read live memory under Asynchronous: a
shadow that never came up sends ReadLocalMemoryUnsync down the fallback path.
The renderer is constructed before this decision, so ask it directly.
Skip the shadow allocation on the front object. The base constructor could not
tell it was building one - m_mem_target still points at itself there - so it
allocated and seeded a full GS-memory-sized copy that nothing can ever read once
the derived constructor repoints it. UpdateSettings runs on both halves and had
the same problem, re-seeding that dead copy on every settings change.
Measured on the SD865, which ships this exact configuration (HWDownloadMode 5,
GSBackThreadMode 3) and was therefore never pipelining at all. Both arms come
from one binary: the back object always constructs lockstep, so -backthread 2 is
precisely what -backthread 3 did before this change. Fan pinned, 3 runs per arm,
-loop 40, medians, ranges disjoint in both titles:
lockstep pipelined
OutRun 12.05 ms 7.75 ms -36% 83 -> 129 fps
Rogue Gal 18.97 ms 12.87 ms -32% 53 -> 78 fps
Rogue Galaxy is the title that just took Asynchronous by GameDB, and it crosses
60 fps on this device as a result.
Correctness: frames are byte-identical across back-thread modes Off,
InlineRecords, Lockstep and Pipelined under Asynchronous, on both the M2 and the
SD865, against a same-binary control run first to confirm the dumps reproduce.
40-loop runs of the previously deadlocking combination complete cleanly with
readbacks exercised. On the M2 frame time is flat at ~6.6 ms - that replay is not
GS-CPU-bound there - though the GS thread still drops 6.17/6.55 ms to 4.06/4.26.
Default configuration is untouched: the back thread is off by default, and only
Asynchronous plus Pipelined changes behaviour.
|
||
|
Brian Degenhardt
|
7fee32e49f |
GS: let a player claim preload frame data and partial invalidation
Both are GameDB hardware fixes, so the database sets them per game and the player's own value is discarded. The only way out was manual hack mode, which is all or nothing: switching one fix off throws away every automatic fix that game had. The pinning mechanism exists precisely for this, and already covers twelve other fixes; these two were simply never added to it. Append them to GSUserHackOverride after TextureOffsetY, so masks already written to an INI keep meaning what they meant, and map the two GameDB fix ids onto them. MaskUserHacks reset both unconditionally in the block below the keep() guards, so move them up: a pinned value has to survive the mask as well as the database, and only both together make the claim stick. MaskUserHacks(false) — the BIOS-boot call that strips hacks for safety rather than preference — still resets them, since the new guards take the same respect_claims parameter as the rest. Verified on a Rogue Galaxy replay, which carries seven fixes including disablePartialInvalidation. Pinning that one alone reports it skipped and still applies the other six; pinning preload frame data behaves the same against a temporary database row; pinning both skips both. The mask half shows up as silence — with a fix pinned and its value on, the database finds the config already agreeing and logs nothing, where the same run without the pin logs the fix being applied over the wiped value. No frontend exposes these yet. Pins are set by writing the override mask, and the only frontend doing that today lists upscaling fixes only, which neither of these is. AutoFlush and TextureInsideRt already sit in the enum with no frontend entry, so the mapping is useful on its own and surfacing them is a separate decision per frontend. |
||
|
Brian Degenhardt
|
37d56169db |
GS/Vulkan: take the RT copy on every draw where the self-read is broken
The workaround was gated on texture replacements being loaded. That read the
evidence backwards. Tales of the Abyss lost its text layer with a pack while NFS
Underground pushed 608 barrier draws per frame with no pack and looked fine, so
the failure was attributed to sampling a replacement. It is not: the in-pass
self-read is unreliable for ordinary blending too, it just fails subtly enough
there to pass inspection.
OutRun 2006 has no pack and renders its sea as high-contrast two-tone speckle.
Measured against the software renderer on Turnip/Adreno 650, in-tile differs from
the oracle over 4.0% of the frame by more than 16 levels; through the RT copy,
0.13%. Both in-pass shapes - the subpassLoad input attachment and the
feedback-loop-layout texelFetch sampler - are byte-identical wrong, which is what
identifies this as the driver rather than the draw.
Drop the LoadTextureReplacements term so every draw on an affected driver reads a
copy. Cost measured on device, RT copy vs in-tile, median frame time over two
runs each:
1x 3x 4x
FlatOut 2 +7.5% +10.5% +24.6% (copies/frame 23 -> 477, RPs 100 -> 538)
OutRun +20.5% +9.9% +0.5%
GoW II -3.2% +2.9%
RG lamps -1.5% +9.3%
The old note recorded +38%/+40% at 3x/4x on NFSU. Nothing here reproduces that
on the titles available now - FlatOut 2 makes a bigger structural change for a
third of the cost at 3x - so it is recorded as an upper bound on a build we can
no longer run rather than as a contradiction. OverrideTextureBarriers = 1 still
restores the in-tile path for anyone who would rather have the frames.
This depends on the preceding DATE change: turning texture barriers off also
turns framebuffer fetch off, and Adreno has no stencil buffer, which used to
leave DATE with no mechanism at all and washed the road blue.
LoadTextureReplacements leaves RestartOptionsAreEqual with it: the shader variant
now follows only OverrideTextureBarriers and the driver profile, so replacements
can be toggled in place again.
Verified on device: with default settings the water dump is now byte-identical to
the forced RT-copy run and within 0.13% of the software oracle. Non-Adreno is
untouched - 16 colour frames identical across OutRun, Katamari, MGS3, Dirge of
Cerberus and Shadow of the Colossus on Honeykrisp.
|
||
|
Brian Degenhardt
|
e180eea455 |
GS: give DATE a fallback when there is no stencil buffer
Every DATE selection branch that cannot use barriers or primitive-ID tracking falls back to read-only stencil, and EmulateDATEGetConfig's terminal else is gated on features.stencil_buffer. Upstream that gate always holds: stencil is only cleared alongside framebuffer fetch, which is picked at the top of the chain. Adreno breaks the pairing - depth is created as plain D32F because any stencil-bearing depth buffer trips the A6xx hangcheck - so with framebuffer fetch and texture barriers both off, no branch assigns destination_alpha at all. m_conf is a member and is not cleared per draw, so the draw silently inherits whatever mode the previous draw used. Sampling destination alpha in the shader answers the same question a read-only stencil pre-pass does: both test the target as it stood before the draw. So substitute DATE >= 5 with one barrier, which is like-for-like rather than a downgrade. One barrier is what publishes that snapshot - with texture barriers the backend inserts a real barrier, without them it copies the target and binds the copy. DATE >= 5 is already part of PSSelector::IsFeedbackLoopRT, so the copy happens with no backend change. Verified by forcing the Adreno feature shape (no stencil, no barriers) on Honeykrisp/M2 with a temporary hook and replaying an OutRun 2006 GS dump. Before, 47.5% of pixels differ from the reference and 44.4% differ by more than 16 levels - the road and sea washed out blue over the bottom half of the frame. After, 0.00-0.64% differ and nothing differs by more than 16 levels. Where a stencil buffer exists the change is inert: colour frames are identical across OutRun 2006, Katamari Damacy, MGS3, Dirge of Cerberus and Shadow of the Colossus. |
||
|
Brian Degenhardt
|
d08356d954 |
GameDB: give Rogue Galaxy the asynchronous GS download mode
Rogue Galaxy blocks the GS thread 7.5 ms every frame to read back sixty-four pixels. It is an 8x8 patch of the depth buffer at a fixed screen position, read once a frame -- a depth occlusion probe, the test a game does before deciding whether to draw a lens flare. The cost is entirely GPU-fence synchronisation, so it does not scale with the payload: the emulator submits, waits for the GPU to finish, and reads 256 bytes. The asynchronous download mode issues the same copy into a throwaway staging texture and retires it at a later vsync, so the GS thread never waits. Measured on the M2 by timing the readback directly rather than the frame, which keeps the number clear of the gsrunner -perf perturbation: 7.2-7.8 ms becomes 0.005-0.008 ms. On a 45 ms frame that stall was around a fifth of the time. It is not the blunt option. NoReadbacks and Unsynchronized, which this overlay already ships for other titles, either skip the readback or race it. Async does the real download and merely serves it a frame late, and it drops a late result outright when an EE upload or a local-to-local move has since claimed those pages, so stale data cannot overwrite newer contents. Output is unchanged on every frame we can compare: four captured scenes, thirty two colour frames, all pixel-identical, each scored against three baseline runs of the unmodified binary first so run-to-run dump nondeterminism could not be read as a result. What that evidence cannot cover is motion. A frame-old occlusion probe differs from a fresh one exactly when the probe's answer changes, which needs the camera or the light to move, and every capture we hold is near-static. The visible failure would be a lens flare blinking a frame late as it passes behind scenery. That is the thing to watch for in play, and reverting is a one-line change if anyone sees it. Seven serials, every Rogue Galaxy entry the overlay carries, including the three Japanese ones -- SCPS-15102, SCPS-17013 and SCPS-19254 are the same game under its Japanese title. OutRun 2006 is the only other title of the eight we profile that reads back at all, three times a frame for 3.3-4.2 ms, and it is deliberately not included here. Its readbacks are small colour buffers rather than depth, and there is an open unexplained brightness bug in that game; feeding frame-old data into what may be an adaptive-exposure loop is not something to do before understanding it. |
||
|
bmdhacks
|
fca3e9074b |
Correct the FpuMulHack comment: the constant is pi, not pi/2
0x40490fdb is 3.14159274, and 0.25 * that is pi/4 == 0x3f490fdb. The
comment called the multiplicand pi/2 and did not say what the patched
value 0x3f490fda is, which left it reading as an arbitrary magic number
lifted from x86.
It is not arbitrary: 0x3f490fda is pi/4 one ULP low, and one ULP low is
what the EE's multiplier returns here. Its Booth recoding drops one ULP
when ft's significand has an odd digit pair (ft & 0x2AA -- 0x490fdb & 0x2AA
== 0x28a, so it fires) and the exact product carries no tail below the
single ULP -- fs = 0.25 = 2^-2 has significand 2^23 exactly, so the product
is exact and the deficit reaches the result. The gamefix is a hardcoded
instance of a general defect, not a game-specific fudge.
Checked by executing the general widened model (cmtst/fmul/fcmeq/bic/add on
doubles under FZ|RZ) on both operand orders:
model(0.25, pi) = 0x3f490fda gamefix patches to 0x3f490fda
model(pi, 0.25) = 0x3f490fdb gamefix leaves alone (host value)
so the model agrees with the gamefix on the asymmetry too -- the predicate
reads ft's significand alone, and 0.25's is zero. That is the same
asymmetry the Cmp sequence below has, where s must be 0.25 and t must be pi.
Comment also records why the general model is not being pulled into this
fast path: here it costs ~9 instructions on every multiply in every game,
against 1 today. It belongs in iFPUd-arm64.cpp, where the operands are
already doubles and it costs 4 -- and where every eeClampMode:3 title gets
it, rather than the one title that needs the hack. Extending it to this
path needs its own measured case.
Comment-only change; no emitted code moves.
Idea by pstef.
|