Repository navigation
Conversation
…addrlib does The reference address of a PS4 PRT surface reduced x and y modulo the macro tile for all six PRT modes, as the production equation does. In AMD's addrlib the reduction applies only to the PRT modes flagged as having no rotation, PRT_TILED_THIN1 and PRT_TILED_THICK; in this code that is GB_TILE_MODE 11 and 16. The 2D PRT modes 6, 7, 12 and 17 keep the whole coordinates. On main the tiling test stops at the first mismatch, GB_TILE_MODE 6 at one byte per element. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The bank bits of a PS4 PRT surface ignored the macro tile index for all six PRT modes. AMD's addrlib and fpPS4 do that only for GB_TILE_MODE 11 and 16; for the 2D PRT modes (6, 7, 12, 17) the bank comes from the whole coordinates. The pipe bits lie inside the macro tile in every mode, so only the bank changes. shadPS4 reduces all six and disagrees. The detiler uses the same equation, so the GPU path follows. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…rdinates The entry now matches GcnTiling after the change: addrlib and fpPS4 do the same, shadPS4 still reduces all six PRT modes. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
The source change and independent test reference look internally consistent, but the PR does not yet show the repository-required AGC validation run or an author-agreed order with #9674.
What I ran
Reviewed head 9934f03567a9dd5bc4e5bc3e8ace9caf65b2bff8 against base 4a58bc29ad656ad7a54c1733b3b683e68b7ccf85. The no-build precheck reports clean independent merge previews against current main 8c91f25b3924a8419096aa368ce44043a5894558. No build or local C++/GPU test was run.
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 14248
git diff --name-status 01d65a198c0a73944c8043555337aac7731b1ba0 8c91f25b3924a8419096aa368ce44043a5894558 -- core/libs/prx/libSceAgcDriver/Graphics/src/TextureTiling.cpp core/libs/prx/libSceAgcDriver/tests/TextureTiling.cpp docs/dev/debt/libSceAgcDriver.md
gh pr view 9674 -R boykopovar/AnyPS5 --json state,headRefOid,baseRefName,updatedAt,mergeable
gh api repos/boykopovar/AnyPS5/issues/comments/6553080159 --jq '{user:.user.login,created_at,body}'Results
| Check | Result |
|---|---|
| Diff check and conventions/template | Passed in precheck. |
| Current-base merge preview | Clean against current main 8c91f25b3924a8419096aa368ce44043a5894558; declared order-only dependency #9674 is present. |
| Source/test inspection | The test reference reduces coordinates only for PRT modes 11 and 16; production does the same for bank bits. I found no source-level mismatch in the inspected lines. |
| Changes since reported test base | No changes to the three touched paths between the PR's reported 01d65a... current-main check and this base. This does not supply the missing validation-layer/VUID evidence. |
| Author-reported tests | agc_driver_graphics_tests reportedly fails with the main implementation and passes with this change; agc_driver_gcn_detile_tests reportedly passes on RTX 4080 SUPER and RX 6400. I did not run these tests, and the PR does not provide the required validation-layer marker/VUID comparison or reproducible command lines. |
| Hosted CI | At refresh, checks were queued; the earlier conventions run was cancelled. No completed product test result was available. |
| Helper limitations | ABI and test-quality helper subprocesses could not run because the isolated precheck checkout lacks the workspace-local helper scripts. This is not a PR failure. |
| Local build/tests | Not run. |
Raw precheck output
All conventions checks passed.
Head unchanged at completion: True
Base unchanged at completion: True
Conclusion: checks failed
The precheck's failure is from unavailable workspace-local ABI/test-quality helper scripts; its diff, conventions/template, and live mergeability checks passed.
Negative control
| Assertion | Change in control | Result |
|---|---|---|
| Bank bits for the four 2D PRT modes use whole coordinates | The author reports running the changed test with the main TextureTiling.cpp |
It reportedly fails first at GB_TILE_MODE 6; I did not independently run the control. |
Findings
Blocking: The repository requires the same focused AGC tests on main and the change with ANYPS5_REQUIRE_VULKAN=1, ANYPS5_NO_SHADER_CACHE=1, and VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation, then checking both logs for the requested-layer warning and comparing VUIDs. The PR describes test outcomes but supplies neither the commands/raw logs nor evidence that this required validation setup was active. Please attach the paired focused outputs and confirm there is no new VUID on the change.
Blocking: The PR identifies an adjacent debt-file conflict with open #9674 and proposes #9674 first. #14248 declares that order, but the linked comment says it is a proposal and asks #9674's author to confirm; no agreement is recorded yet. The repository requires authors of overlapping PRs to agree on order and declare it. Please record the agreement (or the agreed alternate order) before treating the overlap as resolved.
Non-blocking: No implementation defect was established in this source pass. I did not approve because the required validation evidence and overlap agreement are missing.
Sources
- AGC validation requirement at the PR base
- Overlap-order requirement at the PR base
- Changed bank-bit calculation at the PR head
- Independent test reference at the PR head
- Order proposal on #9674
Not verified
I did not build, run ctest, run Vulkan validation, inspect raw author test logs, or test on console hardware. The author-reported GPU tests are not independent results from this review.
To get this to a good state
Attach the required paired focused AGC test logs with validation-layer and VUID comparison, and record #9674's author's agreement on the declared landing order. I will rerun applicable checks on the updated head.
|
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.
|
✅ NVIDIA GPU test: passedPR head
Unstable on main (2, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validation IDs already present on main (5 IDs in 480 tests)Skipped (44)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — changes requested
The paired NVIDIA validation report now covers the required GPU evidence, but the landing order with #9674 is still only proposed, not confirmed by both PR authors.
What I ran
Re-checked head 9934f03567a9dd5bc4e5bc3e8ace9caf65b2bff8 against PR base 4a58bc29ad656ad7a54c1733b3b683e68b7ccf85; current main was 6fe120d11ba3b92c56ade1bb783e211d5dc0949e. The current-base merge preview is clean, with the declared order-only dependency #9674 → #14248. Environment: Linux x86_64, Python 3.11.2, Git 2.39.5.
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 14248 --output-dir /tmp/anyps5-pr-precheck-14248-recheck
python3 tools/pr_stack_check.py 14248 --repo boykopovar/AnyPS5 --output-dir /tmp/anyps5-pr-stack-14248-finalResults
| Check | Result |
|---|---|
| Diff check and conventions/template | Passed. |
| Current-main merge previews | #9674 and #14248 each preview cleanly against current main; their declared order-only dependency is not yet included in #14248's head. |
| Hosted GPU validation | The exact PR head was built and tested with an NVIDIA RTX 2000 Ada; the report says no new validation IDs versus its main baseline. |
| Hosted Windows / conventions / intake / labels / portability / eligibility / report | Passed on this head. Linux and specialized jobs were skipped. |
| Local build and C++ tests | Not run. |
| ABI/test-quality helper scripts | Not executable in the detached precheck checkout because those workspace-only scripts are absent there; this is not a reported PR failure. |
Unedited output excerpts
All conventions checks passed.
PR #9674: clean against current base; head 8f4a2856a25736615ffd051e9859d8ebf275d79d
PR #14248: clean against current base; head 9934f03567a9dd5bc4e5bc3e8ace9caf65b2bff8
Target branch tip: 6fe120d11ba3b92c56ade1bb783e211d5dc0949e
PR head `9934f03` merged onto main `b6bd6e7`
Build: passed (Release, -DANYPS5_ENABLE_SPIRV_TOOLS=ON)
Tests: 2,421 passed · 5 failed (0 new) · 44 skipped
Test setup: GPU tests ... with VK_LAYER_KHRONOS_validation and ANYPS5_NO_SHADER_CACHE=1; main ran the same way
Vulkan validation: no new validation IDs vs main
Findings
Blocking
The PR now declares Depends on: #9674 and proposes #9674 first, but the linked discussion still describes this as a proposal and asks #9674's author to confirm; no confirmation is recorded. CONTRIBUTING.md says authors of overlapping PRs should agree on order and declare it. Please record that agreement, or the agreed alternate order, before merge.
Non-blocking
None found in this re-check.
Sources
- Repository overlap-order rule at the reviewed base
- #14248 order proposal on #9674
- #14248 dependency checklist at the reviewed head
- Exact-head NVIDIA validation report
Not verified
I did not run the C++ tests locally or test on a console. Linux and specialized hosted jobs were skipped. The GPU report's logs are linked in the cited comment; I did not independently reproduce that run.
To get this to a good state
Please obtain or record #9674's author's agreement on the order already declared by #14248 (or update both PRs to the agreed alternative). I will re-check the live heads, overlap preview, and applicable checks after that update.
Supplemental RADV run: paired Vulkan validation and all affected PRT modesThis supplies a paired validation-layer run for the current head, plus an extended local GPU probe covering all four affected 2D PRT modes. No upstream code was added or changed for this report, apart from temporary test-only transplants for the controls. Pins: main
The negative-control message is: Here 24 is Additional GPU coverage: the existing Important interpretation: the GPU detile test prepares its input using production Validation: serial execution, Hardware/toolchain: CachyOS Linux Environment caveat: both sides equally emit loader errors about the pre-existing implicit Reproduction, source identity, and extended probeUse sibling folders cmake -S AnyPS5 -B AnyPS5/build-main -G Ninja -DCMAKE_BUILD_TYPE=Release -DBUILD_TESTING=ON -DANYPS5_ENABLE_SPIRV_TOOLS=OFF
cmake --build AnyPS5/build-main --parallel 4 --target agc_driver_graphics_tests agc_driver_gcn_detile_tests
python3 tester/build_gcn_probe.py
export ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1
export VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation VK_LOADER_DEBUG=layer
for repeat in 1 2 3; do
ctest --test-dir AnyPS5/build-main -R '^(agc_driver_graphics|agc_driver_gcn_detile)$' --parallel 1 --no-tests=error -V > "ctest-${repeat}.log" 2>&1
./tester/gcn_prt_probe > "probe-${repeat}.log" 2>&1
doneSet Copy --- GcnDetile.cpp
+++ gcn_prt_probe.cpp
@@ -79,14 +79,14 @@
const auto context = AgcDriver::VulkanDeviceTestAccess::ContextOf(*device);
TextureDetiler detiler(context);
using enum TextureTileMode;
- for (const auto mode : {kGcnThin1D, kGcnDisplay1D, kGcnDisplay2D, kGcnThin2D, kGcnDepth2D64, kGcnDepth2D256, kGcnDepth2D1K, kGcnDisplayPrt, kGcnThin2DPrt, kGcnDepthPrt256}) {
+ for (const auto mode : {kGcnThin1D, kGcnDisplay1D, kGcnDisplay2D, kGcnThin2D, kGcnDepth2D64, kGcnDepth2D256, kGcnDepth2D1K, kGcnDisplayPrt, kGcnThin2DPrt, kGcnDepthPrt256, kGcnDepthPrt1K, kGcnDisplay2DPrt, kGcnThinPrt}) {
for (std::uint32_t bytes = 1u; bytes <= 16u; bytes <<= 1u) {
for (const auto layer : {0u, 1u, 2u, 3u, 5u}) detile(context, detiler, mode, bytes, layer);
}
}
}
device.reset();
- std::puts("PS4 detile tests passed");
+ std::puts("PS4 detile tests passed: 13 modes x 5 element sizes x 5 array layers = 325 cases");
return 0;
} catch (const std::exception& error) {
std::cerr << error.what() << '\n';Build helper import shlex
import subprocess
from pathlib import Path
def main():
root = Path(__file__).resolve().parent.parent
build = root / "AnyPS5/build-main"
commands = subprocess.check_output(
["ninja", "-C", str(build), "-t", "commands", "agc_driver_gcn_detile_tests"], text=True).splitlines()
template = next(shlex.split(command) for command in commands
if " -c " in command and command.endswith("/tests/execution/GcnDetile.cpp"))
options = []
arguments = iter(template[1:])
for argument in arguments:
if argument in {"-I", "-isystem", "-iquote", "-D"}:
options.extend([argument, next(arguments)])
elif argument.startswith(("-I", "-D")):
options.append(argument)
if not any(argument.startswith("-I") for argument in options):
raise RuntimeError("production test include paths were not found")
libraries = build / "core/libs/libs/unpatched"
command = [
"g++", "-std=c++20", "-O2", *options,
"-I" + str(root / "AnyPS5/core/libs/prx/libSceAgcDriver/tests/execution"),
str(root / "tester/gcn_prt_probe.cpp"), "-o", str(root / "tester/gcn_prt_probe"),
f"-L{libraries}", f"-Wl,-rpath,{libraries}", f"-Wl,-rpath-link,{libraries}",
"-l:libSceAgcDriver.prx",
]
subprocess.run(command, cwd=build, check=True)
print("Extended GCN probe rebuilt against the current production library.")
if __name__ == "__main__":
main()SHA-256, identical across all three phases: This is supplemental execution evidence, not approval. It does not resolve the separate #9674 landing-order discussion, establish console equivalence, or make game/performance claims. AI-assisted hardware test run and report: yes (Codex). |
|
Thanks for the re-check. The validation evidence for the current head is the hosted NVIDIA report and the RADV report in this thread: no new validation IDs versus main, and a paired validation-layer run that covers all four affected 2D PRT modes. The local runs behind the description are from 2026-10-31 between 19:41 UTC and 19:44 UTC (the CPU test fails on The landing order with #9674 is still only proposed: the comment there (#9674 (comment)) was posted on 2026-11-01 at 03:52 UTC and has no answer yet. The two pull requests only touch adjacent lines of the debt file; the source and test files merge cleanly. If #9674's author wants another order, this pull request changes its From this pull request's side the order is confirmed: #9674 lands first, #14248 declares AI-assisted (Claude Code). |
|
The landing order with #9674 is now answered by that pull request's author: either order works (2026-11-01, 17:04 UTC). The overlap is only the two adjacent entries of AI-assisted (Claude Code). |
What
The PS4 (GCN) tiling reduced x and y modulo the macro tile for all six PRT modes before it built the bank bits, so the bank never saw the macro tile index. AMD's addrlib does that only for the PRT modes without rotation:
egbaddrlib.cppL1775-L1780 reduces them underIsPrtNoRotationTileMode, and its mode flags mark PRT_TILED_THIN1 and PRT_TILED_THICK (not the 2D and 3D PRT modes). fpPS4 does the same for its two non-2D PRT array modes (ps4_tiling.pasL2812-L2816) and documents TiledThinPrt and TiledThickPrt (L79, L83) as able to alias several virtual pages to one physical page and the 2D and 3D PRT modes (L80, L84, L85) as not. In this code GB_TILE_MODE 11 and 16 are the thin PRT mode without rotation (kGcnDisplayPrt,kGcnThinPrt); the four modes 6, 7, 12 and 17 (kGcnDepthPrt256,kGcnDepthPrt1K,kGcnDisplay2DPrt,kGcnThin2DPrt; shadPS4 calls them Depth2DThinPrt256, Depth2DThinPrt1K, Display2DThinPrt and Thin2DThinPrt) are the 2D thin PRT mode and now take the bank from the whole coordinates. The pipe bits lie inside the macro tile in every mode, so only the bank changes. The detiler shader uses the same equation, so the GPU path follows.Dissenting source: shadPS4 reduces all PRT modes (
tiling.compL392-L396), as this code did. Two sources against one; I follow addrlib and fpPS4 because addrlib is AMD's own address library and fpPS4 documents why the 2D modes do not alias. Which titles use modes 6, 7, 12 and 17 is not checked, and nothing was measured on a console. PS4 only (surfaces withgcnPitch); the call chain is the GB_TILE_MODE index ->GuestTextureResource.cppmapping ->GcnTiling(TextureTiling.cpp) ->TextureDetilerand the tests.Commits: (1) the tiling test's reference now reduces the coordinates only for GB_TILE_MODE 11 and 16, following the addrlib text above and not the production equation; on main it stops at the first mismatch, GB_TILE_MODE 6 at one byte per element (see Tested). (2)
GcnTilingtakes the macro-tile limit of the bank bits only for those two modes. (3) The debt file: of the two entries merged in #13691, "PS4 (GCN) tiling" stays true (it says the PRT pipe, and for modes 11 and 16 the bank, come from the coordinates inside the macro tile) and "PS4 (GCN) 2D PRT tiling" said the opposite of this change, so it now reads:The entry is 488 characters.
Overlap
Open PRs that change
core/libs/prx/libSceAgcDriver/Graphics/src/TextureTiling.cpp,core/libs/prx/libSceAgcDriver/tests/TextureTiling.cppordocs/dev/debt/libSceAgcDriver.md(file sets fromgit diff --name-onlyagainst each PR head's merge base, 1694 open pull requests as of 2026-11-01 00:37 UTC, maincc26ab4438): 149 touch at least one. One gives a new conflict with this change: #9674 rewrites the thick and layered surfaces entry next to the 2D PRT entry, so the conflict is in the debt file only (lines 107-113). #9674 also changesTextureTiling.cpp(it addskGcnThick1Dto the block-layout and mip-tail switches, toThickBlockExtent, and a thick branch inGcnTiling), but not the PRT lines this change touches (prt,splitRotation,tx,ty; checked in its diff against its merge base); the source and test files merge cleanly, and whichever lands second needs a small rebase of the debt file. The other pull requests that touch the tiling files and already conflict withmain(for example #9432 and #9891) give the same result with and without this change. Checked once more on 2026-11-01 at 03:50 UTC againstmain01d65a198c: of the 22 pull requests opened since 00:30 UTC, #14243 and #14220 touch these files and merge the same with and without this change, and #9674 (head8f4a2856a2) still conflicts with it in the debt file.Tested
agc_driver_graphics_testsfails without the fix and passes with it. The failing run uses the test commit withTextureTiling.cppfrommainand stops at the first mismatch: "PS4 tile mode 24 at 1 bytes: element 256,0 is at 65536, addrlib places it at 73728", where 24 is the value ofTextureTileMode::kGcnDepthPrt256, GB_TILE_MODE 6; the test throws there, so it did not show modes 7, 12 and 17 separately.agc_driver_gcn_detile_tests, which compares the detiler shader with the CPU reference, passes on both GPUs of the test machine: NVIDIA GeForce RTX 4080 SUPER and AMD Radeon RX 6400 (GPU taken from the "Selected GPU" line of each log). Windows 10, MinGW Release build,ANYPS5_NO_SHADER_CACHE=1,APS5_NO_HOST_IMPORT=1, all runs on 2026-10-31 between 19:41 UTC and 19:44 UTC on the same tree as the three commits.tools/check_conventions.py --base f14ae07e29passes. Based onmainf14ae07e29, and it merges withmain01d65a198cwithout a conflict (checked 2026-11-01 03:50 UTC); merged together with #9674 (head8f4a2856a2, updated 2026-11-01 02:12 UTC) it conflicts in the debt file only.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)AI-assisted (Claude Code).
🤖 Generated with Claude Code