Repository navigation
fix(agc): read capture deltas and replay them while queues wait on the title - #12567
Open
chitters2004 wants to merge 16 commits into
Open
chitters2004 wants to merge 16 commits into
chitters2004 wants to merge 16 commits into
Conversation
…apshot A capture snapshot read every committed block through the coherent path, which refuses memory under a multisampled color target, its FMASK or native multisample depth whose samples exist only on the host, so the capture stopped there. The snapshot now writes those bytes as zeros, counts them as host_only_sample_bytes in capture.json and adds a warning, and the replay starts those targets without their earlier contents. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 28, 2026
12 tasks done
MarcZX10
approved these changes
Oct 28, 2026
MarcZX10
left a comment
MarcZX10
left a comment
Contributor
There was a problem hiding this comment.
Reviewed — last commit only
Per the PR description, this review covers commit 7431fc799665d5a853e54cbc2892f4dd81d0e1cf and its settled-driver hold. It does not independently approve the preceding stacked work in this PR or its prerequisites.
What I checked
- Reviewed the settled predicate, wait-state capture, worker admission gates, label-pending check, delta RAII release, replay pacing/breaker call sites, and the new hold and replay tests.
- Verified the predicate only accepts a completed queue or queues parked on still-unsatisfied title waits, and that worker resumption and new submissions are gated until the capture/replay releases the hold.
git diff --check upstream/main...refs/review/pr-12567-currentpassed.git merge-tree --write-tree upstream/main refs/review/pr-12567-currentwas clean (8f8f26546a275d3b913eb7c95cbd09457018ce2b).- Hosted Windows job passed. Eligibility, conventions, intake, labels and report checks passed; Linux product tests were skipped.
Findings
Blocking
None found in the reviewed commit.
Limits
I did not build or run the AGC/Vulkan tests locally or reproduce the paired Windows/Linux and GTA V reports. Those results are author-provided; the hosted Windows job passed. This review is scoped to the requested commit and makes no console-equivalence or pixel-fidelity claim.
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
Late-added ranges and write-watched blocks are read through the same host-only-aware read as the snapshot, so a CPU store over a guarded multisample target no longer aborts a capture. The read is a helper tested for ranges in the middle, at either edge, overlapping and outside a block, and against a native target and its neighbor. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ry group, and write REWIND control words last A title that parks a queue on a REWIND writes the tail's command buffers and only then sets the control word valid. agc_frame_replay released such a wait by copying the recorded control word alone, so the tail ran with its INDIRECT_BUFFER still unwritten, and applying a memory group restored chunks in address order, so a control word could become valid before the buffers it chains to. A parked REWIND is now released by applying the first later memory group whose recorded control word is valid, and every group publishes valid REWIND control words, and the control words of parked REWINDs, only after the rest of the group is written. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e title A memory delta was read only once every accepted submission completed, so a capture stopped after 10 s whenever a queue waited on a REWIND control word or a label a later call stores, as in the two-queue reproducer on boykopovar#11157 and the Ghost of Yotei capture on boykopovar#11197. The driver gains a settled hold: AgcDriverHoldSettled succeeds once every queue has finished or is parked on a REWIND or WAIT_REG_MEM value the title has not stored, with no queued draw, unrecorded label or pending label write, and keeps submissions and parked queues where they are until AgcDriverReleaseSettled. The recorder reads each delta under that hold through the coherent path. The replay restores each delta under the same hold. Its breaker, built on boykopovar#12318, releases a parked REWIND by applying the first later memory group that sets its control word, only under a settled probe, one wait per pass, and no longer writes a value from a later delta for any other wait, which applied a later CPU store early. Pacing counts a running queue as progress and gives up after --pace-timeout-ms only once no queue runs, or after the busy timeout. The drain also ends after 10 s without a completion once no queue runs, and the busy timeout, ten minutes, is shared by pacing, the settled hold and the drain. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
chitters2004
force-pushed
the
fix/agc-capture-settled-deltas
branch
from
October 28, 2026 22:00
7431fc7 to
db4ab05
Compare
Contributor
Author
|
Restacked onto #12566's review fix (fa2b143), so the series merges in order without conflicts. The last commit is now db4ab05. Its only change from 7431fc7 is that @MarcZX10, could you re-approve db4ab05? Your review covered the rest of it unchanged. |
12 tasks done
MarcZX10
approved these changes
Oct 29, 2026
MarcZX10
left a comment
MarcZX10
left a comment
Contributor
There was a problem hiding this comment.
Reviewed — looks good
This approval is scoped to commit db4ab05bc12abeadf6a58184d2b7901c958c01bc and its ReadAroundRanges extraction; it does not independently approve the preceding stacked PRs.
What I ran
- Compared the exact head with previously reviewed commit
7431fc799665d5a853e54cbc2892f4dd81d0e1cf. - Compiled the exact-head
FrameCapture.cppandtests/FrameCapture.cpptogether with the exact-head header using GCC, C++20, and system Zstd; ran the resulting executable. - Ran
git diff --checkon the follow-up.
Results
| Check | Result |
|---|---|
| Standalone frame-capture test | Raw outputframe capture tests passed |
| Whitespace check | Raw outputgit diff --check 7431fc799665d5a853e54cbc2892f4dd81d0e1cf...refs/review/pr-12567 — no output; exit status 0 |
| Hosted checks | Raw statuseswindows pass; conventions pass; labels pass; report pass; linux, register-context, relinker, relinker-asan, write-watch skipped |
| Vulkan validation | Author-reported exact-head resultRTX 3080, Windows 11 Pro 26220: 29 selected tests passed, 0VUID- lines, and no missing-layer warning. The author reports the same focused command on main and intermediate stack heads with 12 and 19 tests respectively. |
Negative control
The PR report includes main and intermediate-stack comparison runs, but no mutation control for this extraction. I did not run a Vulkan main-versus-change control locally.
Findings
Blocking
None found in this scoped follow-up.
Non-blocking
ReadAroundRanges walks skipped ranges in order. Its production producer, HostOnlySampleRanges, sorts the ranges before returning them; the new tests cover overlapping ranges and ranges outside the captured block. The standalone test passed.
Sources
- Reviewed commit
db4ab05bc12abeadf6a58184d2b7901c958c01bc. - Previous reviewed commit
7431fc799665d5a853e54cbc2892f4dd81d0e1cf. - Vulkan test report in the PR description at the reviewed head.
Not verified
I did not run the Vulkan suite or test a console. The Vulkan results above are author-reported; my independent run covered the CPU frame-capture target only.
To get this to a good state
No further change is requested for this commit. Keep the PR's declared dependency order so the other stacked commits are reviewed and merged separately.
12 tasks done
Collaborator
|
@chitters2004 thanks, the settle/hold logic reads correctly (RAII release, no worker self-wait, the hold costs one atomic load when capture is off), and the tests are registered. One request about
|
12 tasks done
Contributor
Author
|
@oneandonlydean, thanks for reading the settle/hold logic. On the two points:
AI-assisted: yes (Claude Code) |
12 tasks done
Collaborator
|
@chitters2004 Thanks, that explains where the new default output comes from. Please revert the "pacing gave up" warning to its old text here, so this PR adds nothing to the tool's default output itself. The |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
@oneandonlydean done: Paired run on Windows 11, RTX 3080 (NVIDIA 621.01), full builds, validation layers on, AI-assisted: yes (Claude Code) |
MarcZX10
approved these changes
Oct 29, 2026
MarcZX10
left a comment
MarcZX10
left a comment
Contributor
There was a problem hiding this comment.
Re-checked — new head, looks good
The latest commit restores the prior pacing-timeout warning text, addressing the requested default-output change; the author also reports the refreshed paired Windows run passing.
What I ran
- Exact PR head:
4ef54b694c2c5cd4900ff418eb1923bcf29f516f. - Current base inspected:
main420b6a064eadaa7c5e50b080c7fad59c41150b4b; merge preview is clean (5d547a7a98bb27bed8ad9161bc56fe4090314dce). - Environment: Linux 6.1.0-54-amd64 x86_64, Git 2.39.5, Python 3.11.2.
- Commands:
git merge-tree --write-tree upstream/main refs/remotes/review/pr-12567
git diff --check upstream/main...refs/remotes/review/pr-12567
python3 tools/check_conventions.py --base upstream/main --head refs/remotes/review/pr-12567
git diff --unified=4 refs/remotes/review/pr-12567^ refs/remotes/review/pr-12567 -- core/libs/prx/libSceAgcDriver/tools/FrameReplay.cpp
Results
| Check | Result |
|---|---|
| Current-main merge preview | Clean; tree 5d547a7a98bb27bed8ad9161bc56fe4090314dce. |
git diff --check |
Passed; no output. |
| Conventions | Passed. |
| Latest commit diff | Only restores the prior pacing gave up stderr format; the busy-timeout and blocked-queue suffixes are removed. |
| Author-reported paired run | On Windows 11 Pro, RTX 3080, driver 621.01: full builds; `frame_(replay |
| Hosted checks | Windows, eligibility, conventions, labels and report pass; Linux and specialized product jobs are skipped by workflow policy. |
Unedited local output
$ git merge-tree --write-tree upstream/main refs/remotes/review/pr-12567
5d547a7a98bb27bed8ad9161bc56fe4090314dce
$ python3 tools/check_conventions.py --base upstream/main --head refs/remotes/review/pr-12567
All conventions checks passed.
$ git diff --stat refs/remotes/review/pr-12567^ refs/remotes/review/pr-12567 -- core/libs/prx/libSceAgcDriver/tools/FrameReplay.cpp
core/libs/prx/libSceAgcDriver/tools/FrameReplay.cpp | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
Negative control
No mutation control was run for the stderr-format restoration. The author reports a paired main/PR run, but says no test specifically asserted the restored warning text.
Findings
Blocking
None found in the latest commit.
Non-blocking
The author-reported test run was against bb04a4f1ea, not the newer 420b6a064eadaa7c5e50b080c7fad59c41150b4b; my merge preview against current main is clean, but I did not rerun the product suite locally. This approval is scoped to the previously approved follow-up and the latest warning-text restoration; the PR still declares its dependency stack in the body.
Sources
Not verified
I did not build or run CTest locally, test on a console, or independently rerun the author-reported Windows/GPU matrix. No test specifically asserts the warning text.
To get this to a good state
No further change is requested for this scoped follow-up. Preserve the declared dependency order; I will recheck any changed head and rerun the relevant commands then.
Contributor
Correction — author test report linkThe Windows paired-run results cited above are in the author’s 19:46 UTC comment; the issue-comment link in the review’s Sources section was incorrect. |
Contributor
✅ AMD GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (11, not counted against this PR)Validated but not compared (8, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 516 tests)Skipped (54)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
Diagnostic follow-up: buffered image observations (no game-FPS claim)Sharing this here alongside our earlier replay findings so others investigating intermittent image differences do not spend time rediscovering the same observer interference. This is separate from the reader optimisation already discussed above and is not a request to expand this capture-fix PR. The improvement is to our experimental diagnostic logger, not a newly discovered fault in upstream's general logging. Upstream already has asynchronous logging from #6371. Our image observer used direct Observation and changeTwo nearby selected-pixel observer calls, with no explicit GPU work between them, were consistently about 24–26 ms apart in our preceding diagnostic runs. We added opt-in Driver and replay buffers are flushed separately after final image acquisition and existing shutdown work. The change adds no GPU command, wait or resource retention and does not change publication stores or ordering. Flush seals reservations, skips unreadied payloads with an explicit incomplete count, and never waits for writers. Our external harness rejects missing module summaries or any incomplete/overflow/truncated/late record. The diagnostic failure marker alone does not change replay's return code; consumers must enforce that completeness gate. Abnormal shutdown can lose the driver's unflushed records. Bounded qualification, not a workload benchmarkOne predeclared synchronous run followed by one buffered run, the same frozen executable and libraries with only the buffered flag changed. Windows, Ryzen 9 3900X, 64 GiB RAM, RTX 3080 Ti 12 GiB, NVIDIA 610.88, MinGW GCC 15.2 Release. Profiling off; hidden/muted;
Buffered output retained 50 driver + 84 replay records, all complete, with no overflow, truncation or late records. Both modes had identical stage counts and matching observed byte chains and conversion settings. These are three final buffer images after repeat3, not independent images of every temporal frame. The timestamp gap includes formatting/helpers and scheduling; it is not isolated fprintf CPU cost. One ordered pair establishes no confidence interval, average per-event cost, repeatable whole-replay improvement or live FPS gain. Do not multiply the gap by all records or treat it as a frame-budget saving. We have not shown that synchronous observation was suppressing the intermittent failure. Both runs had zero failed/stale registrations, unfinished submissions, skipped MiB, pacing timeouts, reservation errors, pending-image destruction warnings, contention flags or reported validation diagnostics. Existing warnings remain: packet-wait warnings 2/1, one prebuild snapshot disagreement and eight pending-image CPU-change warnings per run. No warning or correctness check was suppressed. Focused checks and limits
Source for reuseThe incremental patch below is against our local diagnostic observer layer, which is not present in upstream main. It ignores end-of-line whitespace changes and excludes the earlier conversion observer; it is not a complete reconstruction of the private investigation build and should not be applied directly to this PR. Complete recorder header and synthetic fixture source are also included so the implementation can be reviewed/reused without that local layer. Porting must place the flush sites after the relevant producers are quiescent and final images are acquired, preserve separate module buffers, enforce the completeness gate and add any needed include/test registration. No current-main qualification is claimed. A standalone synthetic fixture can be built with a C++20 MinGW toolchain after placing these two files at the shown paths: g++ -std=c++20 -O2 -I core/libs core/libs/prx/libSceAgcDriver/tests/ImageObservation.cpp -o image_observation_tests.exe
$env:APS5_IMAGE_OBSERVE='1000,17,19'
$env:APS5_IMAGE_OBSERVE_BUFFERED='1'
./image_observation_tests.exeThe address above belongs only to synthetic test data. No private capture, image, shader, guest-memory sample, game code, personal path or binary is included in this comment. AI-assisted: yes (Codex). Incremental source-only review patch against the local observer layerdiff --git a/core/libs/prx/libSceAgcDriver/CMakeLists.txt b/core/libs/prx/libSceAgcDriver/CMakeLists.txt
index 0633583..4e55f3e 100644
--- a/core/libs/prx/libSceAgcDriver/CMakeLists.txt
+++ b/core/libs/prx/libSceAgcDriver/CMakeLists.txt
@@ -505,6 +505,8 @@ if(BUILD_TESTING)
add_test(NAME agc_image_observation_disabled COMMAND agc_image_observation_tests)
add_test(NAME agc_image_observation_enabled COMMAND agc_image_observation_tests)
set_tests_properties(agc_image_observation_enabled PROPERTIES ENVIRONMENT "APS5_IMAGE_OBSERVE=1000,17,19")
+ add_test(NAME agc_image_observation_buffered COMMAND agc_image_observation_tests)
+ set_tests_properties(agc_image_observation_buffered PROPERTIES ENVIRONMENT "APS5_IMAGE_OBSERVE=1000,17,19;APS5_IMAGE_OBSERVE_BUFFERED=1")
add_test_executable(agc_frame_dump_io_tests tests/FrameDumpIo.cpp)
target_include_directories(agc_frame_dump_io_tests PRIVATE ${LIBS_INCLUDE_DIR})
diff --git a/core/libs/prx/libSceAgcDriver/Execution/include/ImageObservation.hpp b/core/libs/prx/libSceAgcDriver/Execution/include/ImageObservation.hpp
index 318dd06..591f796 100644
--- a/core/libs/prx/libSceAgcDriver/Execution/include/ImageObservation.hpp
+++ b/core/libs/prx/libSceAgcDriver/Execution/include/ImageObservation.hpp
@@ -58,9 +58,101 @@ inline bool Selects(std::uint64_t address, std::size_t bytes) {
return Selected().address != 0 && Contains(address, bytes, Selected().address);
}
+template<std::size_t TCapacity>
+class RecordBuffer {
+public:
+ struct Record {
+ std::atomic<bool> ready{false};
+ long long ns = 0;
+ std::size_t thread = 0;
+ std::array<char, 64> stage{};
+ std::array<char, 768> detail{};
+ bool truncated = false;
+ };
+
+ Record* Reserve() {
+ auto value = state.load(std::memory_order_relaxed);
+ for (;;) {
+ if ((value & sealed) != 0) {
+ late.fetch_add(1, std::memory_order_relaxed);
+ std::fprintf(stderr, "[image-observe-buffer] late record after flush FAILED\n");
+ return nullptr;
+ }
+ if (value == TCapacity) {
+ overflow.fetch_add(1, std::memory_order_relaxed);
+ return nullptr;
+ }
+ if (state.compare_exchange_weak(value, value + 1, std::memory_order_relaxed)) return &records[value];
+ }
+ }
+
+ template<typename... TArgs>
+ void Write(const char* stage, const char* format, TArgs... args) {
+ auto* record = Reserve();
+ if (record == nullptr) return;
+ record->ns = std::chrono::duration_cast<std::chrono::nanoseconds>(std::chrono::steady_clock::now().time_since_epoch()).count();
+ record->thread = std::hash<std::thread::id>{}(std::this_thread::get_id());
+ const auto stageSize = std::snprintf(record->stage.data(), record->stage.size(), "%s", stage);
+ const auto detailSize = std::snprintf(record->detail.data(), record->detail.size(), format, args...);
+ record->truncated = stageSize < 0 || static_cast<std::size_t>(stageSize) >= record->stage.size() || detailSize < 0 || static_cast<std::size_t>(detailSize) >= record->detail.size();
+ record->ready.store(true, std::memory_order_release);
+ }
+
+ bool Flush(std::FILE* stream, const char* module) {
+ const auto value = state.fetch_or(sealed, std::memory_order_acq_rel);
+ if ((value & sealed) != 0) return false;
+ std::size_t complete = 0;
+ std::size_t truncated = 0;
+ bool output = true;
+ for (std::size_t i = 0; i < value; ++i) {
+ const auto& record = records[i];
+ if (!record.ready.load(std::memory_order_acquire)) continue;
+ ++complete;
+ truncated += record.truncated;
+ output &= std::fprintf(stream, "[image-observe] ns=%lld thread=%zu stage=%s %s\n", record.ns, record.thread, record.stage.data(), record.detail.data()) >= 0;
+ }
+ const auto dropped = overflow.load(std::memory_order_relaxed);
+ const auto after = late.load(std::memory_order_relaxed);
+ output &= std::fprintf(stream, "[image-observe-buffer] module=%s reserved=%llu complete=%zu incomplete=%llu overflow=%llu truncated=%zu late=%llu\n", module, static_cast<unsigned long long>(value), complete, static_cast<unsigned long long>(value - complete), static_cast<unsigned long long>(dropped), truncated, static_cast<unsigned long long>(after)) >= 0;
+ output &= std::fflush(stream) == 0;
+ return output && complete == value && dropped == 0 && truncated == 0 && after == 0;
+ }
+
+private:
+ static_assert(TCapacity > 0 && TCapacity < (std::uint64_t{1} << 63));
+ static constexpr std::uint64_t sealed = std::uint64_t{1} << 63;
+ std::array<Record, TCapacity> records{};
+ std::atomic<std::uint64_t> state{0};
+ std::atomic<std::uint64_t> overflow{0};
+ std::atomic<std::uint64_t> late{0};
+};
+
+inline bool Buffered() {
+ static const bool enabled = [] {
+ const auto* value = std::getenv("APS5_IMAGE_OBSERVE_BUFFERED");
+ if (value == nullptr || std::string_view(value) == "0") return false;
+ if (std::string_view(value) != "1") throw std::runtime_error("image observation buffered mode requires 0 or 1");
+ return true;
+ }();
+ return enabled;
+}
+
+inline RecordBuffer<4096>& Buffer() {
+ static constinit RecordBuffer<4096> buffer;
+ return buffer;
+}
+
+inline void Flush(const char* module) {
+ if (Selected().address != 0 && Buffered() && !Buffer().Flush(stderr, module)) std::fprintf(stderr, "[image-observe-buffer] module=%s FAILED\n", module);
+}
+
template<typename... TArgs>
void Log(const char* stage, const char* format, TArgs... args) {
if (Selected().address == 0) return;
+ if (Buffered()) {
+ Buffer().Write(stage, format, args...);
+ return;
+ }
static std::atomic<unsigned> count{0};
const auto event = count.fetch_add(1, std::memory_order_relaxed);
if (event > 4096) return;
diff --git a/core/libs/prx/libSceAgcDriver/Execution/src/Driver/Lifecycle.cpp b/core/libs/prx/libSceAgcDriver/Execution/src/Driver/Lifecycle.cpp
index 3811e82..f503fac 100644
--- a/core/libs/prx/libSceAgcDriver/Execution/src/Driver/Lifecycle.cpp
+++ b/core/libs/prx/libSceAgcDriver/Execution/src/Driver/Lifecycle.cpp
@@ -190,6 +190,7 @@ void Driver::stop() {
FlushAsyncLogAtEnd_nid_no_patch();
StopProfileOutput_nid_no_patch();
stopped = true;
+ ImageObservation::Flush("driver");
}
Driver::Driver() {
diff --git a/core/libs/prx/libSceAgcDriver/tests/ImageObservation.cpp b/core/libs/prx/libSceAgcDriver/tests/ImageObservation.cpp
index d0129c3..e31a959 100644
--- a/core/libs/prx/libSceAgcDriver/tests/ImageObservation.cpp
+++ b/core/libs/prx/libSceAgcDriver/tests/ImageObservation.cpp
@@ -1,6 +1,8 @@
#include "prx/libSceAgcDriver/Execution/include/ImageObservation.hpp"
#include <array>
#include <limits>
+#include <memory>
+#include <string>
#include <thread>
int main() {
@@ -39,5 +41,51 @@ int main() {
firstThread.join();
secondThread.join();
require(bytes == saved);
+ const auto capture = [&](auto& buffer, const char* module, bool expected) {
+ const std::unique_ptr<std::FILE, decltype(&std::fclose)> file(std::tmpfile(), &std::fclose);
+ require(file != nullptr);
+ require(buffer.Flush(file.get(), module) == expected);
+ std::rewind(file.get());
+ std::string result;
+ std::array<char, 1024> chunk{};
+ while (const auto size = std::fread(chunk.data(), 1, chunk.size(), file.get())) result.append(chunk.data(), size);
+ require(!std::ferror(file.get()));
+ return result;
+ };
+ RecordBuffer<128> concurrent;
+ std::array<std::thread, 8> writers;
+ for (std::size_t i = 0; i < writers.size(); ++i) writers[i] = std::thread([&, i] {
+ for (unsigned j = 0; j < 16; ++j) concurrent.Write("concurrent", "writer=%zu item=%u!", i, j);
+ });
+ for (auto& writer : writers) writer.join();
+ const auto records = capture(concurrent, "concurrent", true);
+ require(records.find("reserved=128 complete=128 incomplete=0 overflow=0 truncated=0 late=0") != records.npos);
+ for (unsigned i = 0; i < 8; ++i) for (unsigned j = 0; j < 16; ++j) {
+ const auto key = "writer=" + std::to_string(i) + " item=" + std::to_string(j) + "!";
+ const auto index = records.find(key);
+ require(index != records.npos && records.find(key, index + 1) == records.npos);
+ }
+ require(concurrent.Reserve() == nullptr);
+ RecordBuffer<1> overflow;
+ overflow.Write("first", "%s", "retained");
+ overflow.Write("second", "%s", "not retained");
+ const auto overflowText = capture(overflow, "overflow", false);
+ require(overflowText.find("overflow=1") != overflowText.npos && overflowText.find("not retained") == overflowText.npos);
+ RecordBuffer<1> incomplete;
+ auto* reserved = incomplete.Reserve();
+ require(reserved != nullptr);
+ const auto incompleteText = capture(incomplete, "incomplete", false);
+ require(incompleteText.find("complete=0 incomplete=1") != incompleteText.npos);
+ reserved->ready.store(true, std::memory_order_release);
+ RecordBuffer<1> truncated;
+ const std::string longText(900, 'x');
+ truncated.Write(longText.c_str(), "%s", longText.c_str());
+ const auto truncatedText = capture(truncated, "truncated", false);
+ require(truncatedText.find("truncated=1") != truncatedText.npos);
+ RecordBuffer<1> late;
+ capture(late, "empty", true);
+ late.Write("late", "%s", "not retained");
+ capture(late, "duplicate", false);
+ Flush("fixture");
std::puts("image observation parser, boundaries, aliases and concurrent read-only sampling passed");
}
diff --git a/core/libs/prx/libSceAgcDriver/tools/FrameReplay.cpp b/core/libs/prx/libSceAgcDriver/tools/FrameReplay.cpp
index 8c25735..f991eb2 100644
--- a/core/libs/prx/libSceAgcDriver/tools/FrameReplay.cpp
+++ b/core/libs/prx/libSceAgcDriver/tools/FrameReplay.cpp
@@ -1351,12 +1351,14 @@ int main(int argc, char** argv) {
AgcDriver::ImageObservation::Log("replay-shutdown-begin", "%s", "");
AgcDriverShutdown_nid_postfix();
AgcDriver::ImageObservation::Log("replay-shutdown-end", "%s", "");
+ AgcDriver::ImageObservation::Flush("replay");
return 0;
}
} catch (const std::exception& error) {
std::fprintf(stderr, "agc_frame_replay: %s\n", error.what());
}
std::fflush(stdout);
+ AgcDriver::ImageObservation::Flush("replay-error");
std::fflush(stderr);
std::_Exit(code);
}
Complete recorder header: core/libs/prx/libSceAgcDriver/Execution/include/ImageObservation.hpp#pragma once
#include <algorithm>
#include <array>
#include <atomic>
#include <charconv>
#include <chrono>
#include <cstddef>
#include <cstdint>
#include <cstdio>
#include <cstdlib>
#include <functional>
#include <span>
#include <stdexcept>
#include <string_view>
#include <thread>
namespace AgcDriver::ImageObservation {
struct Selection {
std::uint64_t address = 0;
std::uint32_t x = 0;
std::uint32_t y = 0;
};
inline Selection Parse(std::string_view value) {
if (value.empty()) return {};
const auto first = value.find(',');
const auto second = first == value.npos ? value.npos : value.find(',', first + 1);
if (first == value.npos || second == value.npos) throw std::runtime_error("image observation requires hex-address,x,y");
auto address = value.substr(0, first);
if (address.starts_with("0x") || address.starts_with("0X")) address.remove_prefix(2);
const auto number = [](std::string_view text, auto& result, int base) {
const auto parsed = std::from_chars(text.data(), text.data() + text.size(), result, base);
if (text.empty() || parsed.ec != std::errc{} || parsed.ptr != text.data() + text.size()) throw std::runtime_error("invalid image observation number");
};
Selection selected;
number(address, selected.address, 16);
number(value.substr(first + 1, second - first - 1), selected.x, 10);
number(value.substr(second + 1), selected.y, 10);
if (selected.address == 0) throw std::runtime_error("image observation address is zero");
return selected;
}
inline const Selection& Selected() {
static const Selection selected = [] {
const auto* value = std::getenv("APS5_IMAGE_OBSERVE");
return Parse(value == nullptr ? std::string_view{} : std::string_view(value));
}();
return selected;
}
inline bool Contains(std::uint64_t address, std::size_t bytes, std::uint64_t selected) {
return selected >= address && selected - address < bytes;
}
inline bool Selects(std::uint64_t address, std::size_t bytes) {
return Selected().address != 0 && Contains(address, bytes, Selected().address);
}
template<std::size_t TCapacity>
class RecordBuffer {
public:
struct Record {
std::atomic<bool> ready{false};
long long ns = 0;
std::size_t thread = 0;
std::array<char, 64> stage{};
std::array<char, 768> detail{};
bool truncated = false;
};
Record* Reserve() {
auto value = state.load(std::memory_order_relaxed);
for (;;) {
if ((value & sealed) != 0) {
late.fetch_add(1, std::memory_order_relaxed);
std::fprintf(stderr, "[image-observe-buffer] late record after flush FAILED\n");
return nullptr;
}
if (value == TCapacity) {
overflow.fetch_add(1, std::memory_order_relaxed);
return nullptr;
}
if (state.compare_exchange_weak(value, value + 1, std::memory_order_relaxed)) return &records[value];
}
}
template<typename... TArgs>
void Write(const char* stage, const char* format, TArgs... args) {
auto* record = Reserve();
if (record == nullptr) return;
record->ns = std::chrono::duration_cast<std::chrono::nanoseconds>(std::chrono::steady_clock::now().time_since_epoch()).count();
record->thread = std::hash<std::thread::id>{}(std::this_thread::get_id());
const auto stageSize = std::snprintf(record->stage.data(), record->stage.size(), "%s", stage);
const auto detailSize = std::snprintf(record->detail.data(), record->detail.size(), format, args...);
record->truncated = stageSize < 0 || static_cast<std::size_t>(stageSize) >= record->stage.size() || detailSize < 0 || static_cast<std::size_t>(detailSize) >= record->detail.size();
record->ready.store(true, std::memory_order_release);
}
bool Flush(std::FILE* stream, const char* module) {
const auto value = state.fetch_or(sealed, std::memory_order_acq_rel);
if ((value & sealed) != 0) return false;
std::size_t complete = 0;
std::size_t truncated = 0;
bool output = true;
for (std::size_t i = 0; i < value; ++i) {
const auto& record = records[i];
if (!record.ready.load(std::memory_order_acquire)) continue;
++complete;
truncated += record.truncated;
output &= std::fprintf(stream, "[image-observe] ns=%lld thread=%zu stage=%s %s\n", record.ns, record.thread, record.stage.data(), record.detail.data()) >= 0;
}
const auto dropped = overflow.load(std::memory_order_relaxed);
const auto after = late.load(std::memory_order_relaxed);
output &= std::fprintf(stream, "[image-observe-buffer] module=%s reserved=%llu complete=%zu incomplete=%llu overflow=%llu truncated=%zu late=%llu\n", module, static_cast<unsigned long long>(value), complete, static_cast<unsigned long long>(value - complete), static_cast<unsigned long long>(dropped), truncated, static_cast<unsigned long long>(after)) >= 0;
output &= std::fflush(stream) == 0;
return output && complete == value && dropped == 0 && truncated == 0 && after == 0;
}
private:
static_assert(TCapacity > 0 && TCapacity < (std::uint64_t{1} << 63));
static constexpr std::uint64_t sealed = std::uint64_t{1} << 63;
std::array<Record, TCapacity> records{};
std::atomic<std::uint64_t> state{0};
std::atomic<std::uint64_t> overflow{0};
std::atomic<std::uint64_t> late{0};
};
inline bool Buffered() {
static const bool enabled = [] {
const auto* value = std::getenv("APS5_IMAGE_OBSERVE_BUFFERED");
if (value == nullptr || std::string_view(value) == "0") return false;
if (std::string_view(value) != "1") throw std::runtime_error("image observation buffered mode requires 0 or 1");
return true;
}();
return enabled;
}
inline RecordBuffer<4096>& Buffer() {
static constinit RecordBuffer<4096> buffer;
return buffer;
}
inline void Flush(const char* module) {
if (Selected().address != 0 && Buffered() && !Buffer().Flush(stderr, module)) std::fprintf(stderr, "[image-observe-buffer] module=%s FAILED\n", module);
}
template<typename... TArgs>
void Log(const char* stage, const char* format, TArgs... args) {
if (Selected().address == 0) return;
if (Buffered()) {
Buffer().Write(stage, format, args...);
return;
}
static std::atomic<unsigned> count{0};
const auto event = count.fetch_add(1, std::memory_order_relaxed);
if (event > 4096) return;
const auto now = std::chrono::duration_cast<std::chrono::nanoseconds>(std::chrono::steady_clock::now().time_since_epoch()).count();
if (event == 4096) {
std::fprintf(stderr, "[image-observe] ns=%lld stage=overflow\n", static_cast<long long>(now));
return;
}
std::array<char, 768> detail{};
std::snprintf(detail.data(), detail.size(), format, args...);
std::fprintf(stderr, "[image-observe] ns=%lld thread=%zu stage=%s %s\n", static_cast<long long>(now), std::hash<std::thread::id>{}(std::this_thread::get_id()), stage, detail.data());
}
struct Window {
std::size_t offset = 0;
std::size_t bytes = 0;
};
inline Window SelectWindow(std::uint64_t address, std::size_t bytes, std::uint64_t selected) {
if (!Contains(address, bytes, selected)) return {};
const auto offset = static_cast<std::size_t>(selected - address);
const auto first = offset > 16 ? offset - 16 : 0;
return {first, std::min(bytes - first, offset - first + 20)};
}
inline void Bytes(const char* stage, std::uint64_t address, std::span<const std::byte> source, const void* owner = nullptr, std::uint64_t generation = 0) {
if (!Selects(address, source.size())) return;
const auto window = SelectWindow(address, source.size(), Selected().address);
std::array<char, 81> hex{};
constexpr char digits[] = "0123456789abcdef";
for (std::size_t i = 0; i < window.bytes; ++i) {
const auto byte = std::to_integer<unsigned>(source[window.offset + i]);
hex[2 * i] = digits[byte >> 4];
hex[2 * i + 1] = digits[byte & 15];
}
Log(stage, "owner=%p generation=%llu range=%llx+%zu selected=%llx sample=%llx bytes=%zu hex=%s", owner, static_cast<unsigned long long>(generation), static_cast<unsigned long long>(address), source.size(), static_cast<unsigned long long>(Selected().address), static_cast<unsigned long long>(address + window.offset), window.bytes, hex.data());
}
}
Complete synthetic fixture: core/libs/prx/libSceAgcDriver/tests/ImageObservation.cpp#include "prx/libSceAgcDriver/Execution/include/ImageObservation.hpp"
#include <array>
#include <limits>
#include <memory>
#include <string>
#include <thread>
int main() {
using namespace AgcDriver::ImageObservation;
const auto require = [](bool condition) {
if (!condition) throw std::runtime_error("image observation regression failed");
};
const auto selected = Parse("0x1000,17,19");
require(selected.address == 4096 && selected.x == 17 && selected.y == 19);
require(Parse("").address == 0);
for (const auto* value : {"0,1,2", "1000", "1000,1", "1000,1,2,3", "1000,-1,2", "1000,1,4294967296", "10000000000000000,1,2", "g,1,2"}) {
bool rejected = false;
try { static_cast<void>(Parse(value)); } catch (const std::runtime_error&) { rejected = true; }
require(rejected);
}
require(!Contains(100, 0, 100) && !Contains(100, 4, 99) && !Contains(100, 4, 104));
require(Contains(100, 4, 100) && Contains(100, 4, 103));
constexpr auto maximum = std::numeric_limits<std::uint64_t>::max();
require(Contains(maximum - 3, 4, maximum));
const auto full = SelectWindow(100, 100, 150);
require(full.offset == 34 && full.bytes == 36);
const auto first = SelectWindow(100, 4, 100);
const auto last = SelectWindow(100, 100, 199);
require(first.offset == 0 && first.bytes == 4);
require(last.offset == 83 && last.bytes == 17);
require(SelectWindow(100, 4, 104).bytes == 0);
std::array<std::byte, 64> bytes{};
for (std::size_t i = 0; i < bytes.size(); ++i) bytes[i] = static_cast<std::byte>(i);
const auto saved = bytes;
const auto observe = [&] {
Bytes("test-whole", 4096, bytes);
Bytes("test-alias", 4096, std::span<const std::byte>(bytes).subspan(4, 8));
Bytes("test-partial", 4096, std::span<const std::byte>(bytes).first(1));
};
std::thread firstThread(observe), secondThread(observe);
firstThread.join();
secondThread.join();
require(bytes == saved);
const auto capture = [&](auto& buffer, const char* module, bool expected) {
const std::unique_ptr<std::FILE, decltype(&std::fclose)> file(std::tmpfile(), &std::fclose);
require(file != nullptr);
require(buffer.Flush(file.get(), module) == expected);
std::rewind(file.get());
std::string result;
std::array<char, 1024> chunk{};
while (const auto size = std::fread(chunk.data(), 1, chunk.size(), file.get())) result.append(chunk.data(), size);
require(!std::ferror(file.get()));
return result;
};
RecordBuffer<128> concurrent;
std::array<std::thread, 8> writers;
for (std::size_t i = 0; i < writers.size(); ++i) writers[i] = std::thread([&, i] {
for (unsigned j = 0; j < 16; ++j) concurrent.Write("concurrent", "writer=%zu item=%u!", i, j);
});
for (auto& writer : writers) writer.join();
const auto records = capture(concurrent, "concurrent", true);
require(records.find("reserved=128 complete=128 incomplete=0 overflow=0 truncated=0 late=0") != records.npos);
for (unsigned i = 0; i < 8; ++i) for (unsigned j = 0; j < 16; ++j) {
const auto key = "writer=" + std::to_string(i) + " item=" + std::to_string(j) + "!";
const auto index = records.find(key);
require(index != records.npos && records.find(key, index + 1) == records.npos);
}
require(concurrent.Reserve() == nullptr);
RecordBuffer<1> overflow;
overflow.Write("first", "%s", "retained");
overflow.Write("second", "%s", "not retained");
const auto overflowText = capture(overflow, "overflow", false);
require(overflowText.find("overflow=1") != overflowText.npos && overflowText.find("not retained") == overflowText.npos);
RecordBuffer<1> incomplete;
auto* reserved = incomplete.Reserve();
require(reserved != nullptr);
const auto incompleteText = capture(incomplete, "incomplete", false);
require(incompleteText.find("complete=0 incomplete=1") != incompleteText.npos);
reserved->ready.store(true, std::memory_order_release);
RecordBuffer<1> truncated;
const std::string longText(900, 'x');
truncated.Write(longText.c_str(), "%s", longText.c_str());
const auto truncatedText = capture(truncated, "truncated", false);
require(truncatedText.find("truncated=1") != truncatedText.npos);
RecordBuffer<1> late;
capture(late, "empty", true);
late.Write("late", "%s", "not retained");
capture(late, "duplicate", false);
Flush("fixture");
std::puts("image observation parser, boundaries, aliases and concurrent read-only sampling passed");
}
|
Contributor
|
@embersas On whether synchronous observation suppressed your intermittent failure: in our replays, a 22 µs busy-wait per delta chunk restore suppressed a restore-timing failure. That is one mechanism worth ruling out; we don't know it is yours. Our reader pools
Your patch keeps that call, so this may not apply. But the call acts only when pending recorded GPU work writes the range, and we have not identified the hazard (e.g. a pending GPU store landing after the copy, or a copy while GPU work still reads the range). What would tell: did the failing runs restore faster than the old reader, and did their tool make that call before each delta copy? If faster, interleave plain runs with runs that busy-wait each chunk restore up to the old reader's cost until a plain run fails; if those fail too, it is unlikely to be this. @chitters2004 For your port of embersas's reader, two additions from ours if useful: a zstd context per pooled stream (decoding those chunks: 769–813 ms, against 816–834 ms with a fresh context per chunk, in 6 bench runs) and a grow-only compressed buffer instead of an allocation per read. We can push a branch, or leave it if you'd rather port his as is. Windows 11 26200, Ryzen 5 5600X, RX 6700 XT (gfx1031), AMD driver 32.0.21045.16010, WinLibs GCC 15.2.0 r7, Release (reader bench: AI-assisted: yes (Claude Code) |
thuanquang
added a commit
to thuanquang/AnyPS5
that referenced
this pull request
Nov 3, 2026
…refuse calls format 1 FrameRecorder::readGuest reads range and written-block deltas through GuestMemory::ReadCommitted, which first lands the GPU writes the driver still has pending over the range, so such a delta holds the bytes after those writes. The replay restored a delta without landing them, so a write still pending over the range in the replay landed after the restore and replaced the delta's bytes. The replay now reads each delta chunk, calls AgcDriverPrepareCommandWrite_nid_no_patch over its range and then copies it, the order chitters2004's boykopovar#12567 uses in restoreGroup. A calls format 1 capture does not record whether GPU writes had landed before each memory delta was read: main's recorder read its deltas without landing them. calls.bin is now format 2 ("aps5-frame-calls 2"). FrameCaptureReader still reads format 1 and reports the format (Version()), and agc_frame_replay refuses a format 1 capture before it restores any memory, with a message that says to record it again. Co-authored-by: chitters2004 <55252868+chitters2004@users.noreply.github.com> Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
Author
|
@thuanquang Yes please, push the branch. A zstd context per pooled stream and a grow-only compressed buffer are what I'd want in the port anyway, and your bench numbers make the case. I'll build the port on your branch with embersas's reader once this stack has merged, credit you both in it, and take your bench on the PPSA05110 capture as the reference to reproduce before claiming anything. cc @embersas AI-assisted: yes (Claude Code) |
Contributor
|
@chitters2004 Pushed: The capture behind the bench numbers comes from my own copy of the game, so it stays local; the bench program is ours, and I can push it as well if it helps you reproduce on your own capture. AI-assisted: yes (Claude Code) |
|
Thanks @thuanquang. We checked our failing stack: We’ve now measured the original sequential reader on the same capture with unchanged graphics libraries, then run a calibrated comparison. Warm delta-restoration times were:
Per-group targets came from four warm original-reader measurements, totalling 17.565 seconds per pass. Every warm slowed group reached or exceeded its target. Both comparison modes used the same executable, We ran normal/slowed, slowed/normal, then normal/slowed. All six processes matched the three preserved final replay-output buffers exactly. An extra real restore excluded one calibration process and the final comparison pair from matched timing. No replacement runs. The table’s normal/slowed means use the two eligible pairs. Neither historical failure recurred, so we can’t demonstrate suppression or rule out your timing hypothesis. This approximates original group costs rather than recreating the exact sequential read/copy cadence. I’ve also prepared anyps5-restore-harness-v1.zip for review and reuse. It contains the measured archival patches, timing helper, portable Python runner, GPLv2 licence/attribution and synthetic tests. Recipients supply their own compatible capture and references. The portable runner passed synthetic checks; the measurements above used our original local launcher. The archival patches require porting to newer revisions. Windows, Ryzen 3900X, RTX 3080 Ti, NVIDIA 610.88; archived integrated stack, not current main or #14137. No live-FPS claim. AI-assisted: yes (Codex). |
Contributor
Author
|
@thuanquang Thanks. Yes please, push the bench program too: since the capture stays with you, I'll run your bench and the sequential reader on my own GTA V (PPSA04263) capture on this machine, and put both numbers in the port before claiming a gain. The port waits for this stack to merge, as planned. @embersas Thanks for the calibrated run and the harness. I read it the same way you do: the slowed reader brings back the sequential reader's per-group cost and the failure still doesn't recur, so the reader's speed isn't shown to be what exposed it. I'll keep that open in the port's notes rather than claim the faster reader fixes it. AI-assisted: yes (Claude Code) |
…s its REWIND control words Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
@thuanquang On the hazard you couldn't name: one candidate we haven't run, from reading the restore path on #14137's head
To check, wait for submitted work to finish before each copy instead of only flushing. Plain One ordering point on our side, in #12567 only: @embersas The same check applies to your stack, which keeps the flush call before each copy. The read-hazard check is not run here; we can't reproduce the failure on our captures. I'll answer your #12568 design question there separately. AI-assisted: yes (Claude Code) |
Contributor
✅ NVIDIA GPU test: passedPR head
Unstable on main (3, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validation IDs already present on main (8 IDs in 545 tests)Skipped (50)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
Contributor
✅ AMD GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (8, not counted against this PR)Validated but not compared (55, not in the main baseline's validation pass)Validation IDs already present on main (8 IDs in 532 tests)Skipped (57)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
Contributor
|
@chitters2004 Thanks. We ran a version of your check: with a bounded wait for the recorded GPU work before each delta group's restore, 6 of 6 replays matched the capture's 2 reference frames with the pre-copy flush, and 6 of 6 without it. Our only earlier replay without the flush (11-03, no wait) failed in the driver. Why not What we ran. The code from the second item of our 11-03 12:10Z comment: #14137's round 3 (the same code as
The 11-03 rows are our earlier runs of the same tool and reader without the wait (another build and day, not interleaved with these); we didn't repeat the no-flush failure, since such replays can leave the AMD driver hung until a reboot. Pacing give-ups count submit, flip and suspend calls; a Memory call's pacing timeout returns silently ( What this does and doesn't tell. The wait-only matches fit GPU work still running at the copy behind the 11-03 no-flush failure, but they don't place that work over the restored range: the wait covers every recorded batch, and it also delays each group's copy (a delay alone, the 22 µs busy-wait, hid a failure before). The wait lets reads of the range finish and also lands the stores those batches make straight into imported guest memory, so these runs don't separate your write-after-read case from such a store landing after the copy; a wait for only the batches that read the group's ranges would. It lands no deferred result (CPU write-backs made when a batch is reaped, storage-image results and shadows kept on the GPU until a flush): those land whenever a driver thread next reaps or flushes, so in the wait-only arm their order against the copy isn't fixed, while in the wait + flush arm the flush still lands them for chunks with a recorded write. 12 runs are too few to say whether the wait affects the pacing give-up, which Windows 11 26200, Ryzen 5 5600X, RX 6700 XT (gfx1031), AMD driver 32.0.21045.16010, WinLibs GCC 15.2.0 r7, Release; replays on a shared GPU (two of the 12 next to another GPU job); the lowest available physical memory in a run was 0.0-2.9 GB. Not tested: AI-assisted: yes (Claude Code) |
Contributor
Author
|
@thuanquang You're right about This doesn't change #12567. It keeps the flush before each copy, and that arm matched 6 of 6. As you say, a wait for only the batches that read a group's ranges would tell the write-after-read case apart from a late store. I'll leave that with you. AI-assisted: yes (Claude Code) |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
github-actions
Bot
added
needs rebase
Contributor
|
@chitters2004 Pushed the bench program:
The README has the build lines. For the comparison with Checked tonight: both programs build on this branch, and AI-assisted: yes (Claude Code) |
Contributor
✅ NVIDIA GPU test: passedPR head
Unstable on main (3, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validation IDs already present on main (13 IDs in 557 tests)Skipped (49)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
Contributor
Author
|
@thuanquang Thanks for the bench. We ran both programs on a GTA V capture (PPSA04263, 10 frames, 7.1 GB
The decode baselines agree, so the runs are comparable. Your reader costs about 1.2 s over a bare decode of the same bytes here, and main's about 17.5 s. So the gain holds, and is larger, on the large groups a GTA replay is mostly made of. RTX 3080 machine, Windows 11 26220, WinLibs GCC 15.2.0 r7, AI-assisted: yes (Claude Code) |
Contributor
✅ AMD GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (9, not counted against this PR)Passed on retry (1, failed in the parallel run, passed when re-run with a 360 s timeout; not counted against this PR)Validation IDs already present on main (4 IDs in 568 tests)Skipped (60)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
12 tasks done
…ed-deltas # Conflicts: # core/libs/prx/libSceAgcDriver/tests/FrameReplay.cpp # core/libs/prx/libSceAgcDriver/tests/cmake/agc_driver_frame_replay_tests.cmake # docs/dev/BUILD.md
Contributor
✅ NVIDIA GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validation IDs already present on main (9 IDs in 598 tests)Skipped (56)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
…ed-deltas # Conflicts: # core/libs/prx/libSceAgcDriver/tests/FrameReplay.cpp # core/libs/prx/libSceAgcDriver/tests/cmake/agc_driver_frame_replay_tests.cmake # core/libs/prx/libSceAgcDriver/tools/FrameReplay.cpp
github-actions
Bot
added
needs rebase
Contributor
✅ NVIDIA GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validation IDs already present on main (9 IDs in 602 tests)Skipped (55)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Review
db4ab05bc1(+574 −52) and @JJenkx's76ce637975(+10 −3). The others belong to PRs that merge first in the agreed order and drop out of this diff as they do: #11435 and #11436 (Sam-mon7) and #12318 (Tehshortbus). #12566 has merged, and79d7617af2mergesmain(ce9394b844) into this branch.76ce637975is JJenkx's fixture shutdown fix (his42ccdf0b79, first carried in #12568), kept as his commit. The replay fixture callsAgcDriverShutdownbefore static teardown; on Linux the driver's static destructor otherwise crashes the tests this PR adds after they pass (his GDB trace). It moves here because this PR adds those tests.A memory delta is read only once every accepted submission has completed, so a capture stops after 10 s whenever a queue waits on a REWIND control word or a label that a later call stores: @JJenkx's two-queue reproducer on #11157 (comment), and the Ghost of Yōtei capture on #11197 (comment).
Driver:
AgcDriverHoldSettled(ms)succeeds once every queue has finished or is parked on a REWIND orWAIT_REG_MEMvalue the title has not stored yet (parkedOnTitle: no uncommitted queue 0 work, queued draw, unrecorded label or pending label write, and the awaited value still fails its compare). On success it holds:SubmitandSuspendPointwait at admission, and a worker leaving a wait waits innoteWaitBlocked, untilAgcDriverReleaseSettled.AgcDriverSettledis the same check without the hold.Pm4::WaitComparesleavesPm4.cpp's anonymous namespace for that compare.Recorder:
writeDeltatakes the hold instead of waiting for idle and reads the delta through the coherent path (readGuest, from fix(agc): write host-only multisample memory as zeros in a capture snapshot #12566), so the bytes of work that ran before the hold are in it.Replay (built on #11436 and #12318, whose commits are included until they merge):
AgcDriverPrepareCommandWrite). It waits for the hold for up to the busy timeout, or not at all once pacing has already given up at the current serial.AgcDriverHoldSettled(0)). Any other wait gets the last earlier recorded value or a synthesized one; the breaker no longer writes a value from a later delta, which applied a later CPU store early.--pace-timeout-ms(new option, default 10000) only once the driver is settled and progress did not change, or after the busy timeout regardless, so a queue still executing (compiling pipelines on a cold cache) counts as progress. The busy timeout is feat(agc): replay full titles with repeated passes and the title's pipeline cache #11435's ten-minute drain limit, renamedbusyTimeoutand shared by pacing, the settled hold and the drain, replacing a separate limit of this PR. A memory delta waits for the hold instead of a submission depth.Overlap: this follows the order agreed on #11435 (comment): #11435, #11436, #12318, then this PR, which is stacked on all three and adds its test modes to their argument check in
tests/FrameReplay.cpp. #7978 changesQueueWorker.cppL72-74 next to the worker's wait hook, #11455 rewrites BUILD.md L257, and #10760, #11447, #12348 and #12452 edit the frame capture debt entries; whichever merges second rebases those lines.BUILD.md is updated. The frame capture debt entry loses its delta stop; two entries are added for what is still open: a capture still stops when a busy queue neither completes nor parks within 10 s, a submission another captured thread made before a delta can still reach the driver after it, and a queue that neither finishes nor parks holds the replay for the busy timeout.
Tested
Windows 11 Pro 26220, WinLibs GCC, Ninja Release, NVIDIA GeForce RTX 3080 (driver 621.01).
New tests. "Before", measured before this was restacked onto #11436, is this branch with
FrameRecorder.cppandtools/FrameReplay.cppat their base (#12318), keeping only the--pace-timeout-msparsing, and the admission wait removed fromSubmitandSuspendPoint:agc_driver_frame_capture_eopnow also takes a settled hold on an idle driver and checks that another thread'sSuspendPointis not admitted until the release. Before:the driver admitted a submission while a settled hold was taken on an idle driver.agc_driver_frame_replay_rewind_store,_rewind_copy,_rewind_compute,_rewind_releaseand_busy_queueread a delta while a queue is parked on a REWIND or a fence and check the capture and the replayed bytes (see BUILD.md). Before, each capture stops after 10 s:the GPU work submitted before a memory delta did not finish within 10 s.agc_driver_frame_replay_wait_labelis JJenkx's reproducer. Before:the producer submission waited 10016 ms for the capture.agc_driver_frame_replay_rewind_fill,_rewind_splitand_fence_reusepass before and after; they cover delta restoration and fence reuse under the hold.All pass with this PR.
_busy_queuealso covers the replay's pacing and breaker rules: it runs the replay with--pace-timeout-ms 1while a queue copies 64 MiB, and with the tool'sAgcDriverSettledchecks removed it fails 3 of 3 (pacing gave up at call 9 (flip): serials 0/2); with them it passes 3 of 3 in about 1 s. On GTA V (PPSA04263, North Yankton prologue, 3 frames, 13 deltas, all four PRs of this series applied), with an earlier revision of this tool that had neither the pacing rule nor the one-wait-per-pass limit, each of 6 replays gave up pacing once (pacing gave up at call 131479 (suspend): serials 0/3, the capture had 0 in flight, a queue still compiling) and released 2 to 3 waits with synthesized values. With this PR's tool before the restack, which changed only the busy limit (never reached in these runs) and the drain's reports: 1 cold-cache run with--no-synthesize(90 s) and 5 normal runs, all with 0 pace timeouts, 0 released waits, 0 synthesized and no unfinished submissions. With the restacked tool at5ed0a3f0bc, a cold run with--no-synthesizeand a warm run give the same. This is not a pixel-fidelity claim; replayed frames still differ from the title's.Paired run on
mainand this branch, same machine and GPU,ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ctest --test-dir build -R "frame_(capture|replay)|sample_multisample_target" -V:mainat1235db19dc: 12 tests, all passed, 0VUID-lines; #11436 at210bd86b58: 19 tests, all passed, 0VUID-lines; this branch atdb4ab05bc1: 29 tests, all passed, 0VUID-lines (the full set ran on the previous revision, which differed only in one check intests/FrameCapture.cpp;agc_frame_capturewas rerun at this head and passes). Neither log containswas not found but was requested by env var VK_INSTANCE_LAYERS.After
4ef54b694c(the pacing warning back to its text before this PR):ctest -R "frame_(replay|capture)" -Vwith the same variables, full builds with submodules at their pinned commits.mainbb04a4f1ea: 11 tests, all passed. This head merged into it (79e77c5da3): 28 tests, all passed, includingagc_driver_frame_replay_busy_queue. 0VUID-lines on both, and neither log has the missing-layer message.After
76ce637975, onmain4c8cff1916(after #12264), the same command and variables, full builds:main: 11 tests, all passed. This head merged into it (13240d9aca): 28 tests, all passed, including the nine replay modes the Linux CI saw crash. 0VUID-lines on both, and neither log has the missing-layer message. Windows 11 26220, RTX 3080, driver 621.01.After
79d7617af2(mainwith #12566 merged in), the same command, variables and full builds:maince9394b844: 11 tests, all passed. This head merged into it (ba86d7be1f): 28 tests, all passed. 0VUID-lines on both, and neither log has the missing-layer message.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)🤖 Generated with Claude Code