Skip to content

test(libSceAgc): check the packet sceAgcDcbSetNumInstances writes - #12377

Closed
milescoviello wants to merge 1 commit into
boykopovar:mainfrom
milescoviello:test/agc-num-instances
Closed

milescoviello wants to merge 1 commit into
boykopovar:mainfrom
milescoviello:test/agc-num-instances

Conversation

@milescoviello

Copy link
Copy Markdown
Contributor

What

sceAgcDcbSetNumInstances (libSceAgc/DcbDraw/src/Instancing.cpp) writes the PM4 NUM_INSTANCES packet that sets the instance count for the draws after it. No test covers it, and nine of the ten titles here call it: Borderlands 4 (PPSA01494, 7 call sites), Ghost of Tsushima (PPSA02225, 18), Forspoken (PPSA03984, 15), and Metal Gear Solid 4 (PPSA18194), Crimson Desert (PPSA25286), MXGP 26 (PPSA30839), Super Meat Boy 3D (PPSA31162), Call of Duty: Black Ops II (PPSA34502) and Steins;Gate Re:Boot (PPSA36591) with one each. Crimson Desert, Black Ops II and Steins;Gate also call sceAgcDcbSetNumInstancesGetSize. In the three titles with the most call sites, 23 of their 40 calls pass the constant 1, read from the instructions before each call. I didn't trace the other 17.

How

New test agc_num_instances (tests/NumInstances.cpp), registered in libSceAgc/CMakeLists.txt after agc_draw_index_multi_instanced, the same way. No code change. It checks:

  • sceAgcDcbSetNumInstancesGetSize returns 8.
  • Counts 1, 2 and 1000, written one after another into the same buffer, each come out as a 2-dword packet at the cursor: header 0xc0002f00 (type 3, one payload dword, opcode 0x2F) and the count. Each call returns the packet's start and moves the cursor by two dwords, and nothing is written after the third packet.

These are the words the driver's PM4 parser takes: it expects 0x2F to be 2 dwords long (Pm4.cpp:314) and reads the instance count from the second (Pm4.cpp:1065).

Tested

Linux (Ubuntu 26.04.1, GCC 15.2), on main 2227f13661:

  • agc_num_instances passes 3 of 3 at this head (b7220cf006).
  • With Instancing.cpp writing opcode 0x2E instead (local mutant a89f04e956), it fails with instance count 1 wrote 0xc0002e00 0x00000001, not NUM_INSTANCES with the count.
  • Windows 11 VM (WinLibs GCC 15.2, Release): agc_num_instances passes 3 of 3 at this head.

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Fixes: none
  • Unblocks: none
  • GPUs tested: n/a
  • AI-assisted: yes

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the packet layout against the implementation and parser references, then built and ran the new test with the exact-head and ; it passed. The local harness stubbed only (the driver notification), while retaining the real packet allocation and encoding path. Diff check and repository conventions also pass. No correctness issue found in this focused test addition.

@MarcZX10

Copy link
Copy Markdown
Contributor

Correction to my approval summary: I compiled and ran the exact-head Instancing.cpp and Packet.cpp with the PR test; it passed. The local harness stubbed only AgcDriverPrepareCommandWrite_nid_no_patch (the driver notification), while keeping the real packet allocation and encoding path. The omitted identifiers in the review body were a quoting issue; the test result and review are unchanged.

@adriadam10 adriadam10 self-assigned this Oct 28, 2026
@milescoviello

Copy link
Copy Markdown
Contributor Author

Closing this as a duplicate of my own #11678, which already checks sceAgcDcbSetNumInstances and sceAgcDcbSetNumInstancesGetSize in tests/Commands.cpp. My duplicate search went by PR text, and #11678's title calls it the instance count packet, so I missed it. @MarcZX10, thanks for the review, and sorry for the extra work.

AI-assisted: yes (Claude Code)

@milescoviello
milescoviello deleted the test/agc-num-instances branch October 28, 2026 12:37
@github-actions

Copy link
Copy Markdown

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.

Pull request Shared files not compared
PR 5021 core/libs/prx/libSceAgc/CMakeLists.txt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants