From 9697710e431517fd4cf6c5c84f7cd524e8af8140 Mon Sep 17 00:00:00 2001 From: Dipshet <264011288+Dipshet@users.noreply.github.com> Date: Wed, 5 Aug 2026 21:56:39 +0200 Subject: [PATCH] Fix the one-frame effects/clouds flicker under host CPU pressure The effect update and effect draw are two async worker-pool jobs entering through one vtable slot, told apart only by a shared mode word with no per-job copy; a host stall lets both jobs read "draw", so the effect and cloud passes are emitted twice for one frame. Snapshot the mode at submit and dispatch from the snapshot. Gated behind ac6_effect_mode_fix, default off. --- CMakeLists.txt | 1 + src/ac6_backend_fixes/ac6_effect_mode_fix.cpp | 263 ++++++++++++++++++ 2 files changed, 264 insertions(+) create mode 100644 src/ac6_backend_fixes/ac6_effect_mode_fix.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 399fa47b..2b802bd5 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -41,6 +41,7 @@ set(AC6RECOMP_SOURCES src/ac6_backend_fixes/ac6_backend_hooks.cpp src/ac6_backend_fixes/ac6_backend_pass_classifier.cpp src/ac6_backend_fixes/ac6_cutscene_resync.cpp + src/ac6_backend_fixes/ac6_effect_mode_fix.cpp src/ac6_backend_fixes/ac6_fps_physics_fix.cpp src/ac6_backend_fixes/ac6_kbm_input.cpp src/ac6_backend_fixes/ac6_widescreen.cpp diff --git a/src/ac6_backend_fixes/ac6_effect_mode_fix.cpp b/src/ac6_backend_fixes/ac6_effect_mode_fix.cpp new file mode 100644 index 00000000..6903971d --- /dev/null +++ b/src/ac6_backend_fixes/ac6_effect_mode_fix.cpp @@ -0,0 +1,263 @@ +// AC6 one-frame effects/clouds flicker - cure. +// +// SYMPTOM (reported in the wild since 2026-07-11, reproduces identically in +// stock Xenia): for a single frame the afterburner / light sprites render +// brighter and world-locked, and the clouds look doubled. Self-heals the next +// frame. Triggered by HOST-side CPU pressure only. +// +// MECHANISM (round 141, measured over six instrumented runs): +// +// The effect UPDATE and the effect DRAW are two asynchronous jobs on the +// game's second worker pool. Both enter through the SAME vtable slot - +// CX360EffectManager / CAce6EffectManager vtbl[+0x04] = rex_sub_822B3248 - +// and are told apart ONLY by a shared plain-store mode word at +// [mgr + 0xA1E84], with no per-job copy of it. Every dispatch arrives from +// lr = 0x821D4F80, inside the pool worker body rex_sub_821D4F20. +// +// mode 0 -> f1 = dt ; rex_sub_822B2E20 UPDATE +// mode 1 -> f1 = dt ; rex_sub_822B0848 +// mode 2 -> vtbl[+0x100] = rex_sub_820B25D0 DRAW +// mode >=3 -> return +// +// The submitter writes the mode, then submits; the worker reads it later. On +// the 360 the two jobs never overlapped - fixed core assignment, deterministic +// scheduling - so the shared word was safe. rexglue discards both guest +// priorities and guest affinities, so that ordering guarantee is gone, and a +// host stall lets the second submit's write land before the first job has read +// its own value. +// +// When that happens BOTH jobs read mode 2: the update job draws instead, the +// update never runs, and the effect + cloud passes are emitted twice - ~+137 +// draw requests into one draw-order bucket, +124 draws, every effect generator +// and observer doubling in lockstep and reverting the next frame. The two +// generator classes involved are CEffectGeneratorAfterBurner and +// CEffectGeneratorLightSprite, which are exactly the effects reported. +// +// THE FIX: snapshot the mode at submit time and dispatch from that snapshot, +// so a job runs with the parameter it was submitted with. +// +// Two things this deliberately does NOT do, both learned by measurement: +// +// 1. It does not write the mode word back and let the guest re-read it. That +// was tried: the write landed, read back correct, and the guest still saw +// the racing value ~6 instructions later. The concurrent writer owns that +// window and no store can hold it. Measured 0.76% residual vs 0.00% here. +// +// 2. It does not take over "only when the snapshot disagrees with memory". +// That sounds like a smaller blast radius and is actually the same bug in +// miniature: on the agreeing path the guest still re-reads, so a write +// landing after the check reintroduces exactly the hole in (1). Partial +// takeover always leaves one direction open - pass through on mode 2 and a +// racing write turns a draw into an update instead, which is the same +// artifact mirrored. Full takeover is what makes it airtight. +// +// VALIDATION (identical binary, one cvar, host CPU stress throughout): +// fix ON : doubled-frame rate 0.00% (0 / 3748), 0.0 detections per 1k +// fix OFF : doubled-frame rate 4.41%, 28.2 detections per 1k +// and cured rather than silenced - the effect manager still runs its normal +// two dispatches and exactly one draw per frame. + +#include +#include +#include + +#include +#include +#include +#include + +REXCVAR_DEFINE_BOOL( + ac6_effect_mode_fix, false, "AC6", + "Fix the one-frame effects/clouds flicker under host CPU pressure. The effect update " + "and the effect draw are two async worker jobs sharing one vtable slot, told apart only " + "by a plain-store mode word with no per-job copy; when a host stall lets the second " + "submit's write land first, both jobs draw and the effect + cloud passes are emitted " + "twice. This snapshots the mode at submit and dispatches from the snapshot. Off by " + "default."); + +// Dispatch targets, called directly. NOT via PPC_CALL_INDIRECT_FUNC: that macro +// degrades to __builtin_debugtrap() unless ppc_config.h was included first, +// which it is not in this translation unit - it would trap on the first +// corrected frame. +PPC_EXTERN_FUNC(rex_sub_820B25D0); // vtbl[+0x100] DRAW (mode 2) +PPC_EXTERN_FUNC(rex_sub_822B2E20); // UPDATE, f1 = dt (mode 0) +PPC_EXTERN_FUNC(rex_sub_822B0848); // f1 = dt (mode 1) + +PPC_EXTERN_FUNC(__imp__rex_sub_821D4A78); // worker-pool Submit(tag, thr, job, arg) +PPC_EXTERN_FUNC(__imp__rex_sub_822B3248); // effect manager vtbl[+0x04], the dispatcher + +namespace { + +constexpr uint32_t kEffModeOffset = 0xA1E84; +constexpr uint32_t kEffDispatcher = 0x822B3248; +constexpr uint32_t kEffDraw = 0x820B25D0; + +// Submissions are matched to dispatches by the exact (job, arg) pair rather +// than positionally. The pool has PER-THREAD queues, so dispatch order is not +// submit order; a positional match hands each job the other one's mode, which +// swaps them. (That was a real bug in an earlier revision - its fingerprint was +// corrections arriving as "2 -> 0" immediately followed by "0 -> 2".) The +// worker calls job->vtbl[1](job, arg) with the same arg the submitter passed, +// so the pair identifies a submission uniquely without assuming anything about +// ordering, threads or queue depth. +struct EffModeEntry { + uint32_t job = 0; + uint32_t arg = 0; + uint32_t mode = 0; + bool used = true; +}; + +struct EffModeTable { + std::mutex mu; + EffModeEntry e[16]; + int next = 0; + uint64_t corrections = 0; + uint64_t matched = 0; + uint64_t unmatched = 0; + uint64_t dropped = 0; + // (job, arg) is assumed to identify a submission uniquely. If two + // outstanding submissions ever share a key, the newest-first lookup at + // dispatch can hand a job the other one's mode - and that failure is SILENT, + // because `unmatched` only counts "no entry found", never "found the wrong + // entry". So measure the assumption instead of resting on it: + // key_collisions - same key outstanding twice, same mode (harmless) + // key_conflicts - same key outstanding twice, DIFFERENT modes (the + // dangerous case; if this is ever non-zero the key needs + // a third component, e.g. the target thread index) + uint64_t key_collisions = 0; + uint64_t key_conflicts = 0; +}; + +EffModeTable g_modefix; + +// A pool job is the effect manager iff its vtable slot +4 is the dispatcher. +// Both CX360EffectManager and CAce6EffectManager share that slot. +bool IsEffectManagerJob(uint8_t* base, uint32_t job) { + if (job < 0x82000000u || job >= 0xC0000000u) { + return false; + } + const uint32_t vt = rex::memory::load_and_swap(base + job); + if (vt < 0x82000000u || vt >= 0x82A00000u) { + return false; + } + return rex::memory::load_and_swap(base + vt + 4) == kEffDispatcher; +} + +} // namespace + +// Snapshot the mode word at the moment the job is queued - that is the +// parameter this particular job was submitted with. +PPC_FUNC_IMPL(rex_sub_821D4A78) { + PPC_FUNC_PROLOGUE(); + if (REXCVAR_GET(ac6_effect_mode_fix)) { + const uint32_t job = ctx.r5.u32; + if (IsEffectManagerJob(base, job)) { + const uint32_t mode = rex::memory::load_and_swap(base + job + kEffModeOffset); + const uint32_t arg = ctx.r6.u32; + std::lock_guard lk(g_modefix.mu); + // Is (job, arg) actually unique among outstanding submissions? See the + // counter declarations - a duplicate key with a differing mode is the one + // way this fix can go wrong without any counter noticing. + for (const EffModeEntry& other : g_modefix.e) { + if (other.used || other.job != job || other.arg != arg) { + continue; + } + ++g_modefix.key_collisions; + if (other.mode != mode) { + ++g_modefix.key_conflicts; + if (g_modefix.key_conflicts <= 8) { + REXLOG_WARN( + "[AC6-MODEFIX] key collision: job {:08X} arg {:08X} outstanding with " + "mode {} while submitting mode {} - (job, arg) is not a unique key here " + "and a dispatch could take the wrong one (conflicts {}, benign {})", + job, arg, other.mode, mode, g_modefix.key_conflicts, g_modefix.key_collisions); + } + } + } + // Overwriting a still-unused slot means a submission was never claimed by + // a dispatch. Count it rather than losing it silently. + EffModeEntry& slot = g_modefix.e[g_modefix.next]; + if (!slot.used) { + ++g_modefix.dropped; + } + slot.job = job; + slot.arg = arg; + slot.mode = mode; + slot.used = false; + g_modefix.next = (g_modefix.next + 1) & 15; + } + } + __imp__rex_sub_821D4A78(ctx, base); +} + +// Dispatch from the snapshot instead of re-reading the racy mode word. +// Anything unmodelled falls through to the original, untouched. +PPC_FUNC_IMPL(rex_sub_822B3248) { + PPC_FUNC_PROLOGUE(); + if (REXCVAR_GET(ac6_effect_mode_fix)) { + const uint32_t self = ctx.r3.u32; + const uint32_t self_arg = ctx.r4.u32; + uint32_t want = 0; + bool have = false; + { + std::lock_guard lk(g_modefix.mu); + // Newest first, so a repeated (job, arg) resolves to the latest submit. + for (int i = 0; i < 16 && !have; ++i) { + EffModeEntry& slot = g_modefix.e[(g_modefix.next - 1 - i) & 15]; + if (!slot.used && slot.job == self && slot.arg == self_arg) { + want = slot.mode; + slot.used = true; + have = true; + } + } + if (!have) { + ++g_modefix.unmatched; + } + } + if (have) { + const uint32_t now = rex::memory::load_and_swap(base + self + kEffModeOffset); + { + std::lock_guard lk(g_modefix.mu); + if (now != want) { + ++g_modefix.corrections; + if (g_modefix.corrections <= 8 || (g_modefix.corrections & 0xFF) == 0) { + REXLOG_INFO( + "[AC6-MODEFIX] mode word reads {} but this job was submitted with {} " + "(corrected {}, in-order {}, unmatched {}, dropped {}, key conflicts {})", + now, want, g_modefix.corrections, g_modefix.matched, g_modefix.unmatched, + g_modefix.dropped, g_modefix.key_conflicts); + } + } else { + ++g_modefix.matched; + } + } + + const uint32_t dt_bits = ctx.r4.u32; + if (want == 2) { + const uint32_t vt = rex::memory::load_and_swap(base + self); + // Only take the direct path when the slot really is the draw we + // decoded. A different vtable means a class we have not modelled, and + // guessing there would be worse than the bug. + if (vt >= 0x82000000u && vt < 0x82A00000u && + rex::memory::load_and_swap(base + vt + 0x100) == kEffDraw) { + rex_sub_820B25D0(ctx, base); // r3 = this, r4 = arg already in place + return; + } + } else if (want == 0 || want == 1) { + float dt = 0.0f; + std::memcpy(&dt, &dt_bits, sizeof(dt)); + ctx.fpscr.disableFlushMode(); + ctx.f1.f64 = static_cast(dt); + if (want == 0) { + rex_sub_822B2E20(ctx, base); + } else { + rex_sub_822B0848(ctx, base); + } + return; + } else if (want >= 3) { + return; // the original returns without dispatching + } + } + } + __imp__rex_sub_822B3248(ctx, base); +}