mirror of
https://github.com/ARMSX2/ARMSX2.git
synced 2026-08-24 16:50:16 -07:00
cache-dxstg-followups
100
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Brian Degenhardt
|
bb9df22fc9 |
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
|
85a88d75da |
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. |
||
|
Brian Degenhardt
|
5f50eab28e |
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. |
||
|
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.
|
||
|
bmdhacks
|
c5a6c7cb0a |
Record the FPU's measured timings; leave the cycle table alone
The EE's divide unit was timed on silicon to bound what its recurrence can be -- eleven rounds of modelling its rounding had never constrained the hypothesis space, and cycles per quotient bit constrains it hard. DIV.S and SQRT.S each occupy the FPU for 7 cycles and RSQRT.S for 13, against this table's 6, 6 and 8. MUL.S is latency 4 / issue interval 1, which is the one entry the table already has right. Measured with COP0 Count around loops of k copies of one instruction so the loop overhead falls out as the intercept; four runs, three byte-identical. The interesting part is not the numbers but the mix. Interleaved div.s/sqrt.s costs the sum while div.s/add.s costs the divide alone, so div and sqrt are one shared non-pipelined unit rather than two that happen to take equally long, and RSQRT.S at 7+7-1 is two passes through it -- which is what FPU.cpp's RSQRT_S already assumed from values alone. The latency does not move on any operand class, including denormals and divide by zero, so there is no early-out. The table is not touched. It drives game timing, its own comment already calls itself a hack, and nothing here measured what moving it would do to compatibility; that is a separate change with a separate burden of proof. The comment exists so the console run does not have to be taken a third time. Idea by pstef. |
||
|
bmdhacks
|
9eb45cf590 |
Fix: the recompiler-test GIF Path 1 sink missed a wrapped XGKICK's head
`mVU_XGKICK_` splits a packet that runs past the top of VU1 memory in two: the pre-wrap head goes to `Gif_Path::CopyGSPacketData`, and only the post-wrap tail goes to `Gif_Unit::TransferGSPacketData`. The PCSX2_RECOMPILER_TESTS sink hooked only the second one, so a wrapped kick was captured as its tail alone — no GIFtag at all. Measured while landing the console XGKICK cases: 64 captured bytes from the recompiler against 112 from the interpreter, which loops through TransferGSPacketData and never calls CopyGSPacketData. That reads exactly like a serious microVU miscompile and is entirely our instrumentation; both engines emit the same correct stream. CopyGSPacketData now feeds the same sink and skips the ring, since nothing drains it while the sink is installed. The gif_test_hooks declaration moves above Gif_Path so the member function can see it. Consequence, which is larger than the bug: before this, no test could observe a wrapped XGKICK's GIFtag. Every XGKICK test used packets that fit inside VU1 memory, so the blind spot never showed. Test-build only — the whole block is inside #ifdef PCSX2_RECOMPILER_TESTS. vu1_xgkick_drain_tests.cpp moves with it. That file is ours and postdates the branch this came from, and its wrap case was written against the blind spot: it asserted the JIT capture was the 32-byte tail, with a header comment explaining that the head was unreachable. Both halves now arrive, so it asserts the whole 48 bytes — head carrying the GIFtag, tail resuming at offset 0 — which is the stronger property and the one that was never testable before. The XgKickHack wrap case already asserted that shape and only loses a stale "unlike the non-hack path" aside. Idea by pstef. |
||
|
bmdhacks
|
63e7904e55 |
Fix: VU replay inherited the interpreter's stale sticky STATUS flags
vu_capture::CapturedState carries microVU's four-deep micro_statusflags[]
shadows but not the interpreter's live scalar accumulators
VU->statusflag/macflag/clipflag, and RestoreState left them alone. So the
interpreter pass of a replay started from whatever the previously executed
VU program had left in VURegs.
Most of that is harmless because it gets recomputed, but one field is not:
_vuFMACAdd snapshots VU->statusflag into fmac[i].statusflag
_vuFMACflush ORs (fmac[i].statusflag & 0xFC0) into VI[REG_STATUS_FLAG]
0xFC0 is the STICKY field (ZS/SS/US/OS/IS/DS). Every op recomputes the 0xF
cause nibble, but nothing clears the sticky bits except FSSET -- so a stale
one rides straight through into the architectural result. The JIT derives its
status entirely from the restored micro_statusflags[] and never grows the
phantom bit, and the replay reports a divergence that belongs to neither
engine.
Seeded from VI[] rather than zeroed, because that is the exact inverse of the
flush above (VI[REG_MAC_FLAG] = fmac[i].macflag; STATUS takes the sticky field
plus the cause nibble). A capture taken with sticky flags already raised now
replays with them instead of silently losing them. No format bump -- VI[] is
already carried in full.
Found as an order-dependent failure of VuReplay.ReplayVu0Vadd... under
--gtest_shuffle. Diagnosed by execution, not by reading: with the triggering
predecessor in place VU0.statusflag was measured at 0x82 on entry to
ReplayCapture, and injecting that value directly reproduced the exact diff
`vi16: JIT=0x0 INTERP=0x80`. Injecting 0x02 (cause-only) did not -- only the
sticky half survives, which is the field the interpreter never recomputes.
Tests: the new ReplayDoesNotInheritStaleInterpreterStickyFlags seeds the
accumulator directly, so the fault is pinned without depending on a shuffle
seed; verified live (fails with the identical vi16 diff on the unpatched
RestoreState). ReplayVu0Vadd... also gained the diff_lines printout its VU1
twin already had -- a bare EXPECT_FALSE on `diverged` names no register, which
is most of why this took as long to triage as it did.
1538 pass, 0 fail. VuReplay no longer fails under any shuffle seed tried
(17, 4242, 99, 31337, 8, 2); EeRecCarbonSelfLoop.PinnedValueLoopCarriedBaseByteFill
still does and is unrelated.
Idea by pstef.
|
||
|
bmdhacks
|
5bd3e7deb8 |
Fix: arm64 EATAN paired four coefficients with the wrong power of x
mVU_Globals (microVU_Misc.h) stores the EFU atan series in ascending power order but names the entries T1, T5, T2, T3, T4, T6, T7, T8 -- that is, T5 is the x^3 coefficient and T2 the x^5 one. mVU_EATAN_arm called the helpers in NAME order, so c5 landed on x^3, c7 on x^5, c9 on x^7 and c3 on x^9. Only the first term and the last three were on the right power. Against unknownbrackets/ps2autotests tests/vu/lower/efu.expected that is worth up to 919642 ULP, worst on the largest reduced arguments since the misplacement is in the low-order terms: case before after console EATAN CVF_3PI_OVER2 3fbc5441 3fae4be7 3fae4be7 919642 -> 0 ULP EATAN CVF_PI 3fa99875 3fa19dc5 3fa19dc4 522929 -> 1 EATAN CVF_INCREASING 3fa72d09 3f9fe0ba 3f9fe0ba 478287 -> 0 EATANxz CVF_DECREASING 3ee3f44a 3eed633d 3eed6339 -618223 -> 4 EATAN CVF_MAX_MANTISSA 3f9012c8 3f8db70b 3f8db70b 154557 -> 0 EATAN CVF_PI_OVER2 3f813895 3f807f4c 3f807f4c 47433 -> 0 Every EATAN-family row moved toward silicon or stood still; none moved away. Clears bad_jit on eight cases (kEfuBadJit 126 -> 118) and drops EATAN CVF_PI_OVER2 and EATAN CVF_PI from kEatanEngineDivergences, where the JIT now agrees with the interpreter exactly. On the rows that still differ the JIT is the side nearer the capture: it is exact where the interpreter is 1-2 ULP out, because it evaluates in single precision throughout while _vuCalculateEATAN goes through double-precision pow(). Free: the eight constants live within one struct at offsets 96..224, all encoding as the same single scaled-immediate LDR, so the emitted sequence is a permutation of identical instruction pairs -- same count, same bytes. Upstream x86 mVU_EATAN_ has the identical defect and is left alone; it is never executed on this host and is not updated upstream. DISABLED_DumpEatanFamily is the measurement that produced the table, kept so the reading can be re-made from data rather than from emitter source. It was cross-checked against a standalone single-precision evaluation of both orderings outside the emulator, which reproduces the same magnitudes to within 3 ULP. Idea by pstef. |
||
|
bmdhacks
|
fc5ca4aca4 |
Fix: interp EATAN's x^7 coefficient was T3 with a digit dropped
_vuCalculateEATAN's eatanconst[] is a hand-transcription of the atan
coefficients microVU keeps in mVU_Globals (microVU_Misc.h). Eight of the
nine round-trip to those globals bit for bit, and so do all five of the
sinconsts[] added alongside them. One did not:
T3 printed: -0.139085337519646
in tree: -0.13085337519646 <- the 9 after "0.13" is gone
Origin:
|
||
|
bmdhacks
|
1d16981a29 |
Fix: interp EATAN family was missing the range-reduction argument transform
Deficient: interp. Correct: arm64 JIT + x86 JIT (both emit the same shape).
_vuCalculateEATAN evaluates the EFU's atan series and then adds
eatanconst[8] = 0.785398185 = pi/4. That constant is only correct as the
second half of
atan(x) = pi/4 + atan((x - 1) / (x + 1))
so the polynomial has to be fed the REDUCED argument. Both recompilers do
that -- arm64 mVU_EATAN computes (Fs-1)/(Fs+1) before mVU_EATAN_arm, x86
mVU_EATAN the identical SUBSS/ADDSS/DIVSS -- and the interpreter passed the
raw argument, adding a pi/4 offset nothing had earned. The xy/xz forms want
the same identity for atan(y/x), i.e. (y-x)/(y+x), which is again what both
recompilers emit.
Re-derived arithmetically rather than read off the source: for Fs = 1.0 the
unreduced expression evaluates to 0x3FCA1D99, bit-for-bit what the
interpreter returned, and the reduced one to 0x3F490FDB, bit-for-bit what
both recompilers return (console 0x3F490FDA). A second witness at Fs = 3.0
agrees: unreduced gives 0xC6F69D96, i.e. -3e4 for an arctangent.
Dropping the `if (x != 0)` guard in the xy/xz forms is part of the same fix:
with the reduced argument the divisor is (y+x), not x, and the guard was
returning +0 where both recompilers and the console return a NaN pattern.
HOW THIS WAS INVISIBLE, which is the part worth keeping. Every test in
vu1_efu_console_conformance_tests.cpp scores each engine against silicon
SEPARATELY and records what it cannot reproduce per engine. That is
deliberate and the file says why: a pure JIT-vs-interp differential is blind
to anything both engines get wrong together. But it left the mirror-image
blind spot -- nothing asserted the two engines agree with EACH OTHER, and
CaseMatches() reduces each run to a bool and discards the value, so two
engines returning DIFFERENT wrong answers are flagged twice as "known bad"
and look settled. Measured: 111 of the 208 EFU cases had the engines
disagreeing. This fix brings 15 into agreement, leaving 96.
New tests, since that assertion class did not exist:
- EatanFamilyEnginesAgreeExceptWhereListed asserts engine agreement across
all 48 EATAN-family cases, with the 33 still-diverging labels listed
explicitly so movement in either direction fails loudly.
- EatanAppliesTheRangeReductionBeforeThePolynomial pins the defect as
arithmetic (EATAN(1.0) is the bare pi/4 constant), so it stays meaningful
even if both engines are later changed together.
The 33 remaining EATAN-family divergences are two further classes, both
documented in the test file and neither this commit's subject: the
recompilers hand raw exponent-255 patterns to the polynomial and get NaNs
where the interpreter clamps operands through vuDouble first, and the
interpreter evaluates the series in double-precision pow() against the
recompilers' single-precision Horner chain. On both, the interpreter is the
side nearer the console.
Bidirectional: with VUops.cpp reverted, both new tests fail (15 red
assertions naming jit=3f490fdb interp=3fca1d99). Suite 1530 pass / 0 fail /
22 disabled, plus core 86, common 31, gs_vertex 21, mvu_progcache 13.
Idea by pstef.
|
||
|
bmdhacks
|
48016d4ead |
Fix: an empty COP2 dest mask must still retire the MAC flag
cop2EmitFlagUpdate returned early on xyzw == 0, so a masked-to-nothing FMAC left the PREVIOUS FMAC's MAC standing. It is not a silent op: every lane takes VU_MACx_CLEAR, so MAC reads back 0 and the STATUS cause nibble empties while the stickies stand. Both references already agree. The interpreter runs the clear plus the STAT update through applyBinaryMACOp -- _getDst returns &RDzero for fd == 0, it does not skip the op -- and x86's REC_COP2_mVU0 has no such early-out at all, reaching mVUupdateFlags with AND_XYZW == 0. Console case VUSTICKY_EMPTY_DEST_MASK_SILENT; pinned by VuStickyConsoleConformance.Arm64Cop2MacroEmptyDestMaskRetiresTheMacFlag. This was previously bundled into the COP2 U/O flag work, which has been reverted; the fix is independent of it and stands on its own. Idea by pstef. |
||
|
bmdhacks
|
63f12f4009 |
Correct the COP2 FPCR comment: FZ is set in a default game
The comment 882c0d40ea left behind says "FPCR.FZ is NOT set while these
run". That was read off the test harness, which sets FPCR to 0 --
RecompilerTestEnvironment mirrors CPUThreadInitialize and stops before
the VM applies FPUFPCR. A real boot says otherwise. Printing FPCR from
recExecute before entering recompiled code, God of War II:
FPCRPROBE live=0x1c00000 FPUFPCR=0x1c00000 VU0FPCR=0x1c00000 FZ=1
0x1c00000 is FZ plus RMode=ChopZero, from EmuConfig.Cpu.FPUFPCR, whose
default is DAZ+FTZ+ChopZero. On aarch64 DAZ and FTZ are the same bit, so
that flushes denormal operands and results alike.
No code change: the software flush stays, and the reason it stays is now
the accurate one. FZ is a per-unit user setting (EmuCore/CPU:
FPU/VU0/VU1.DenormalsAreZero), so it cannot be assumed either way; and it
could not supply the flag half regardless, since it erases the mantissa
that U is defined on. What the flush actually buys is that the two
engines agree in both environments -- redundant when FZ is on, load
bearing when it is not.
Idea by pstef.
|
||
|
bmdhacks
|
3d15e04c6c |
Fix: VU sticky bits accumulate where the flags are produced, not at flush
VU_STAT_UPDATE assigned the bare ZSUO cause nibble to statusflag, throwing the sticky field away, and _vuFMACflush re-derived the stickies from that cause nibble on every pipeline entry it retired. That is idempotent right up until an FSSET clears the sticky field: the stale cause then regenerates the bit FSSET just cleared. Traced on VUSTICKY_MICRO_FSSET_ASSIGNS_NOT_ORS. The FSSET merge itself is correct -- it produces VI=801 -- and the bit comes back one entry later: [FSSET] imm=800 sf 001 -> 801 [FLUSH] pipe[2] flagreg=10000 entry.sf=801 VI=041 -> VI=801 (FSSET arm) [ADD] pipe[3] lower VIwrite=8 snap.sf=801 [FLUSH] pipe[3] flagreg=8 entry.sf=801 VI=801 -> VI=841 (FMAC arm) pipe[3] is not an FMAC at all; flagreg=8 is a lower op writing integer register VI[3]. It took the non-FSSET arm only because every retired entry rewrites STATUS, and (sf & 0xF) << 6 turned the long-dead Z cause back into sticky Z. So the sticky OR moves to VU_STAT_UPDATE, where the flags are produced, and both flush sites take the sticky field from the snapshot instead of re-deriving it. statusflag is seeded from the whole STATUS register in vu0ExecMicro, so carrying the sticky field in it is the documented intent -- VU_STAT_UPDATE's original comment claimed exactly this preservation while the code did the opposite. Both halves are load-bearing: reverting VU_STAT_UPDATE alone fails the Vu0AluUpper suite, reverting the flush arms alone leaves the FSSET rows diverging. Graduates VUSTICKY_MICRO_FSSET_CLEARS and VUSTICKY_MICRO_FSSET_ASSIGNS_NOT_ORS from kMicroDivergences, which no longer holds any interp-only row. Idea by pstef. |
||
|
bmdhacks
|
4a7284ea4a |
Fix: ESQRT/ERSQRT take the operand's magnitude
The EFU square root roots a negative operand as if it were positive; the interpreter's `p >= 0` guard returned the operand unchanged instead, so ESQRT and ERSQRT of -1.0 gave -1.0 where the console gives 1.0, and ESQRT of -0.0 gave -0.0 where the console gives +0.0. Both recompilers already AND the raw bits with absclip before FSQRT (mVU_ESQRT, mVU_ERSQRT), so masking the sign off before vuDouble puts the interpreter on the same order of operations rather than on a new one. Clears bad_interp on ERSQRT CVF_NEGONE, ESQRT CVF_NEGZERO and ESQRT CVF_NEGONE; kEfuBadInterp 119 -> 116. The remaining negative-operand rows (CVF_MIN, CVF_GARBAGE2, ...) still fail on BOTH engines and are untouched here: they are Inf/NaN inputs that vuDouble only clamps to fMax under CHECK_VU_OVERFLOW, which ships off. That is a separate, all-engines-wrong defect. Idea by pstef. |
||
|
bmdhacks
|
bcf55312d4 |
Fix: VU0 macro flag merge keeps the D/I cause and accumulates sticky D/I
Two mask defects in the COP2 macro flag merge, both against the console and against the arm64 recompiler: SYNCMSFLAGS preserved only 0xFC0, so every macro FMAC cleared the D/I cause pair (0x30) that belongs to the div unit. VU_STAT_UPDATE assigns statusflag outright, so those bits have to be carried from VI -- arm64 does the equivalent, keeping its denormalized bits 18-19 across cop2EmitFlagUpdate, which EeVu0Cop2MacroLazyStatus.DivCurrentDIBitsSurviveFmac already pinned. SYNCFDIV preserved only 0x3CF, clearing sticky IS/DS and re-deriving them from the current cause, so sticky D/I could never accumulate across two div ops nor outlive a clean one. Graduates eight interp rows from kMacroStatusDivergences: VUSTICKY_DIV_DI_ACCUMULATE, VUSTICKY_FMAC_KEEPS_DI, VUSTICKY_DI_ACCUMULATE_SQRT_DIV and VUSTICKY_DI_CAUSE_REPLACED_STICKY_KEPT, slots 2 and 3 of each. VUSTICKY_CLEAN_DIV_KEEPS_STICKY_DI stays recorded for both engines: its slot-1 VRSQRT of -0 raises only D where hardware raises D and I, and slots 2-3 are that missing bit carried forward -- a different defect that happened to share the old reason text, now corrected. Also drops VU_STAT_UPDATE's comment claiming it saves the sticky flags and D/I settings; it assigns and saves neither, which is precisely why the merge masks have to. Idea by pstef. |
||
|
bmdhacks
|
1ea5e98e4b |
Fix: VU div-unit ops raise the sticky D/I bits, not just the cause bits
_vuDIV/_vuSQRT/_vuRSQRT set only the cause bits (STATUS 5:4) when they raise D or I. VU->statusflag therefore never carried a sticky bit, so the snapshot _vuFDIVAdd hands to the pipeline had nothing sticky in it and _vuFDIVflush's `& 0xC30` contributed only the cause half -- the interpreter's micro path set NO sticky D or I at any point. microVU accumulates them and matches the console. Sticky is derived from the cause bits the op just raised, which is the same `<< 6` relation SYNCFDIV and the FMAC flush already use. The macro path is unaffected: SYNCFDIV computes the sticky field from the cause bits itself and masks these bits out. Graduates VUSTICKY_MICRO_DIV_DI_ACCUMULATE (the always-on MicroStatusMatchesConsole tripwire caught it, as intended). Two neighbouring rows were re-diagnosed while confirming this and are left recorded with corrected reasons -- the previous text named causes the code does not have: VUSTICKY_MICRO_FSSET_CLEARS / _ASSIGNS_NOT_ORS are NOT an assign-vs-OR bug. _vuFSSET assigns the field, _vuRegsFSSET declares the REG_STATUS_FLAG write, and _vuFMACflush's REG_STATUS_FLAG arm assigns rather than ORs. Interp reads back 840 (the FSSET's sticky D plus a stale sticky Z) where the console gives 800. The real cause is recorded as an explicitly unverified hypothesis. VUSTICKY_MICRO_CLEAN_DIV_KEEPS_STICKY_DI is not the accumulation gap either; both engines read C00 as 800 because vrsqrt of -0 raises only D where hardware raises D and I. DISABLED_AllMicroStatusMatchesConsole stays disabled: 9 -> 8 failures. Idea by pstef. |
||
|
bmdhacks
|
2114367187 |
Fix: interpreter CTC2 narrows the integer VIs and masks STATUS
CTC2's default arm stored all 32 bits of the source GPR, so two cases diverged from the console capture on the interpreter only: vi01-vi15 are 16-bit integer registers and read back as 0000FFFF after a write of FFFFFFFF; the interpreter kept the full word. vi16 STATUS has a writable sticky field of 0xFC0. The 0x3F current-flag field belongs to the FMAC pipeline and survives a CTC2, so a write of FFFFFFFF reads back as 00000FC0 with the live cause nibble intact. Both recompilers already did this (microVU_Macro.inl recCTC2 emits a 16-bit store and the 0xFC0/0x3F merge; iCOP2-arm64.cpp recCOP2_CTC2 emits Strh and the same merge). The recompilers additionally broadcast the denormalized STATUS into micro_statusflags; the interpreter does not need to, because vu0ExecMicro re-copies VI[REG_STATUS_FLAG] into the micro instances at program start -- COP2 cannot execute while a microprogram is running, which is the comment's stated reason for doing the copy there. CLIP, I and Q still land in the default arm and stay 32-bit. vi18/vi27/vi31 are left recorded in kCtc2Divergences: CLIP is 24-bit and CMSAR0/CMSAR1 are 16-bit on silicon, and REG_CMSAR1 kicks VU1 without storing at all. Those are wrong on BOTH engines, so DISABLED_AllCtc2WriteMasksMatchConsole stays disabled -- its failure count drops from 8 to 6. The STATUS mask also graduates three interpreter rows in the sticky suite, which is what "clears the sticky field and leaves the cause nibble standing" means in practice. Idea by pstef. |
||
|
bmdhacks
|
eff1a3b4b9 |
Fix: interpreter CFC1 implements the FCR alias and mask model
The interpreter's CFC1 was wrong three ways against the console capture: it hardcoded 0x2E00 for FCR0 instead of reading fprc[0] (which holds 0x00002E30), it returned 0 for every index that is not 0 or 31, and it handed back the raw FCR31 word with no fixed-bit model. Hardware decodes only bit 4 of the register field: 0-15 alias FCR0 and 16-31 alias FCR31, with FCR31 reading back as (fprc[31] & 0x0083C078) | 0x01000001. Both recompilers already did exactly this (iFPU.cpp recCFC1, iFPU-arm64.cpp recCFC1), so this is the shared interpreter catching up rather than a new model. CTC1 is left as a raw store: both recompilers also store raw and apply the model at read. All 14 failures in EeFpuFcrConsoleConformance.DISABLED_BothEnginesMatchConsoleFcrModel were [interp]; the tripwire now passes and is enabled, and the divergence table it fed is gone. The arm64 recCFC1 comment claiming the interpreter returns the raw word is updated -- that is no longer true. Idea by pstef. |
||
|
bmdhacks
|
45b4197bee |
Fix: PMADDW/PMSUBW accumulate into one 64-bit value, not two 32-bit halves
Both the interpreter and the arm64 recompiler modelled each PMADDW/PMSUBW lane as two independent 32-bit accumulations. That loses the carry between LO and HI, needs a truncating `/ 0xFFFFFFFF` where an arithmetic shift belongs, grew a +0x70000000 lane-0 addend to compensate, and leaves LO sign-extended from the wrapped intermediate rather than from the architectural result. The x86 recompiler has always done it correctly (recPMADDW: PMULDQ + PADDQ + PMOVSXDQ against the packed accumulator), so the three engines disagreed; this brings the other two into line with it. Scored against the 64 ps2autotests PMADDW/PMSUBW captures, which preset HI/LO to nonzero values and so make the width of the accumulate observable: two 32-bit halves + errata 54/64 packed 64-bit accumulate 64/64 The errata components are not separable. Reintroducing any one of them alone into the corrected model scores far worse than leaving all three in: packed + voodoo addend 52/64 packed + truncating divide 3/64 packed + carry dropped from HI 33/64 The truncating divide's off-by-one very nearly cancels the missing carry on these operands, which is why the shipping code only lost 10 cases. A partial fix here would have been much worse than none. On arm64 the correct form is also the smaller one: Smull plus a 64-bit add against the spliced accumulator replaces the lane splitting, the SDIV by 0xFFFFFFFF and the whole conditional-addend block, and Rd.UD[dd] turns out to be the raw result. Verified bidirectionally: reverting either engine alone leaves 7 tests failing, since h.Run() auto-diffs the JIT against the interpreter. Tests: the six EeRecMmi vectors that pinned the errata are rewritten to pin the carry, the borrow, the per-half sign extension and the absence of the lane-0 addend; all six fail on the unpatched baseline. The PMADDW/PMSUBW allowance in EeMmiConsoleConformance is deleted, so all 64 cases now assert normally. Also corrects a comment in iMMI-arm64.cpp claiming MMI2_RECOMPILE is never defined -- Config.h:1650 defines it unconditionally, which makes that part of pcsx2/x86/iMMI.cpp the shipping x86 implementation rather than dead reference code. Idea by pstef. |
||
|
bmdhacks
|
e13cfa6311 |
Fix: interpreter LD must not write the zero register
LD wrote cpuRegs.GPR.r[_Rt_].UD[0] unconditionally, so `ld $0, ...` followed by a read of $0 returned the loaded doubleword instead of zero. All eleven of its neighbours already guard: LB/LBU/LH/LHU/LW/LWU/LWL/LWR/ LDL/LDR early-out on !_Rt_, and LQ routes through gpr_GetWritePtr, whose reason for existing is the dummy zero slot. Both recompilers were already correct. Shape matches LW/LDL/LDR: perform the read first so its side effects still happen, then drop the result if the destination is $0. EeLsuConsoleConformance.DISABLED_AllLoadsMatchConsole recorded this as the one divergence from the console capture; the case now passes on both engines, so the divergence table is gone and the always-on LoadsMatchConsole scores every case against silicon directly. Idea by pstef. |
||
|
Brian Degenhardt
|
af4079edea |
Merge pull request #519 from ARMSX2/aethersx2-savestates
Load AetherSX2/NetherSX2 save states |
||
|
Brian Degenhardt
|
173d10bf0c |
GS: never wait for the back queue to empty from the EE thread
The back queue's empty-wait admits a single waiter. The worker posts m_empty_sema exactly once on its transition to idle, clears the waiting flag, and sleeps; a second waiter that armed the same flag blocks forever, because nothing will post again. WaitForEmpty has a pxAssertMsg saying so, but it is compiled out in Release, so on a handheld build this is a silent hang rather than a message. ReadLocalMemoryUnsync was the second waiter. It runs on the EE thread, and it drained the back queue on entry. Under the HW download modes that read local memory from the EE thread — Unsynchronized and Asynchronous — every readback arms both waiters by construction: the EE thread drains here while the MTGS thread is already waiting, either in the lockstep tail of PushRecord or in InitReadFIFO servicing the very AsyncReadFIFO packet this function queues. A readback-heavy scene therefore wedges within seconds, and resuming a savestate into one wedges immediately. Drop the drain. Nothing depended on it. The Asynchronous path reads the shadow copy under m_async_readback_mutex, which is the actual synchronization point and is published by the GS thread — a drain adds nothing there, and stalls the EE thread on the GS thread, which is what the mode exists to avoid. The Unsynchronized path races the renderer's local memory writes by definition, and a drain narrows that window without closing it, since MTGS can queue another record the instant it returns. Staleness is already bounded the real way, in OpenGSRenderer: the pipelined front-object split is refused outright whenever HW downloads are read from the EE thread. The queue now also records which thread claimed the empty-wait, and drains from any other thread assert. WaitForEmpty's own guard only trips when two waits happen to overlap; this one trips on the first off-thread drain whatever the timing, so the next one to be added fails loudly in Devel instead of hanging in Release. Only back-thread modes Lockstep and Pipelined ever started a consumer, so only those two combined with the two EE-thread download modes were affected. With the back thread off — the default — the removed call was already a no-op. Verified with pcsx2-gsrunner replaying an OutRun 2006 water-scene dump: the repro aborted on the assert within five loops before, and now runs twenty loops and 312 readbacks clean. All twelve back-thread x download mode combinations pass, and frame dumps are byte-identical between back-thread-off and pipelined-with-async. |
||
|
Brian Degenhardt
|
c2ea1c1f24 |
SPU2: restore the full core state from AetherSX2-era savestates
The first cut reset the cores and brought back only RAM, registers and the mixer scalars, and mapped the era's volume levels across raw. Both halves were wrong. Volume levels are 31-bit in these eras and 15-bit today, so a full-volume legacy level overflows the mixer's s32 volume multiply and turns the whole mix into broadband static — loud enough to amplify even a paused game's silence into noise, which is what made the defect look independent of the SPU2 payload. And the dropped voice, streaming and DMA state belongs to a transfer the restored IOP driver still believes is in flight: left at reset, the driver waits forever for a completion interrupt no counter will deliver. Map everything instead: voices (envelope levels scaled 31->15 bits, the era's Releasing flag folded into the phase exactly as its Calculate() folded it, decode restarting at the current block), the ADMA/DMA counters (host pointers left null for the engine's own savestate heal, except the read-path pointer, which the heal misses — rebuilt from the channel TADR register), and the SPDIF/ring-position/playmode tail. Now that this is a field-by-field mapping rather than a memory copy, it moves out of spu2freeze.cpp into its own translation unit, leaving that file byte-identical to upstream's. The era layout tables are ours to carry and upstream keeps editing that file, so the two are better apart. Verified headless against graded audio captures: a mid-gameplay 0x9A2C resume state (Dragon Quest VIII) comes back with its music grading music-like (spectral flatness 0.007) where the partial restore graded noise (0.38+), and pause-menu saves come back silent because their sound driver parks every voice at zero pitch and volume until unpaused. 92 core and 1714 recompiler tests green. |
||
|
Brian Degenhardt
|
21d1bfcf97 |
SaveState: don't reject a legacy state when the VM has no ELF CRC yet
Loading a state at startup resets the VM first, and the reset clears the ELF CRC, so by the time the legacy reader checks identity the running game's CRC is 0. Comparing against it rejected every state booted with -statefile: This AetherSX2 save state is for CRC 45FE0CC4, but the running game is CRC 00000000. An unknown CRC is not a mismatch. Only compare when the VM actually has one, which is the same rule the rest of the emulator uses (ReportGameChangeToHost reports a CRC only once HasBootedELF()). The disc serial, which survives the reset because it comes from the disc rather than the ELF, still hard-fails on a wrong game — that gate is unchanged. Ratchet & Clank: Up Your Arsenal now resumes from both legacy formats: the NetherSX2 0x9A34 state and the AetherSX2-era 0x9A2C one, the latter exercising all three of that era's deviations. Both consume the blob exactly, and the EE, IOP and GS all run on from the restored state. |