Skip to content

fix(shader): number an instanced draw's hull patches across its instances - #4450

Merged
adriadam10 merged 2 commits into
boykopovar:mainfrom
Pipodi:feat/tessellation-instanced-hull
Oct 14, 2026
Merged

adriadam10 merged 2 commits into
boykopovar:mainfrom
Pipodi:feat/tessellation-instanced-hull

Conversation

@Pipodi

@Pipodi Pipodi commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

What

When GE_CNTL.BREAK_WAVE_AT_EOI is clear, the instances of a tessellated draw share hull threadgroups. The relative patch index then counts on through the instances, while the host's PrimitiveID restarts with each one. #4201 rebuilds the index from PrimitiveID alone, which is only right when every instance starts a threadgroup.

  • Instance numbering. The local stage writes its draw-relative instance (InstanceIndex - BaseInstance) to one more interface slot. The hull reads it from control point 0, and groupedPatch numbers the patches instance * patchesPerInstance + PrimitiveID before taking (mod PRIM_GRP_SIZE) mod NUM_PATCHES.
  • Restart cases. The per-instance PrimitiveID is kept when BREAK_WAVE_AT_EOI is set, or when the patches per instance are a multiple of the group. In both cases every instance starts a threadgroup.
  • GE_CNTL decode. feat(agc): rebuild the hull's relative patch index when it is used as data #4201 now decodes GE_CNTL.BREAK_WAVE_AT_EOI into TessellationConfiguration::breakAtInstance (request serialization, draw and pipeline keys) and refuses a hull or domain that uses its relative index as data while the bit is clear. This PR builds on that decode and replaces the refusal with the numbering above; it no longer decodes or serializes the bit itself.
  • Cache keys. The patches per instance are derived from the draw (TessellationInstancePatches) and go into the hull and domain cache keys, in place of feat(agc): rebuild the hull's relative patch index when it is used as data #4201's breakAtInstance key: the bit changes the compiled program only through the patches per instance.
  • Indirect draws. The hull and domain are recompiled for each indirect record's instance count. An instanced tessellated draw neither reuses nor stores a draw entry.
  • Device feature. The device enables shaderDrawParameters when it has it, and ShaderDeviceProfile and shader validation accept BaseInstance and DrawParameters in vertex stages. Without the feature, a hull that uses its relative index as data in such a draw is refused.
  • Domain. A domain that uses its relative index as data in such a draw is refused. Passing the instance on to the domain is not implemented.

Tested

Windows 11, RTX 4090.

  • agc_driver_tessellation. New RequireInstancedRelativePatches test: two instances of three patches, first instance 1, (patches per group, primitive group) = (4, 8). The test reads back the relative index that the second instance's patches 0 and 2 carry. The counted-through numbering, a per-instance restart and the absolute instance each give different values. Also checked: the (3, 3) and BREAK_WAVE_AT_EOI restart cases, the domain refusal, and the refusal without DrawParameters.
  • Mutations. In an earlier version of this test, ignoring the instance and dropping the BaseInstance subtraction each failed on the predicted patch.
  • agc_shader_memory. The patches per instance change only the hull and domain keys, and BREAK_WAVE_AT_EOI survives serialization and leaves the key as for one instance.
  • Rebased onto main e34c47921 (fix(shader): record BDA faults in hull programs instead of refusing them #4222 merged). The tessellation levels live in the draw state, so the instanced test draws pass their levels in PatchDraw. feat(agc): rebuild the hull's relative patch index when it is used as data #4201's two "BREAK_WAVE_AT_EOI is clear" refusal cases are dropped, as this PR replaces that refusal; the instanced refusals above cover the domain and the missing DrawParameters. Two lists on main gained entries next to this PR's: StageInputKind keeps LayerExported and adds BaseInstance, and the shader validation keeps main's mesh ClipDistance condition next to this PR's DrawParameters one.
  • Full suite (ctest, Windows, RTX 4090): of 1198 tests, 1183 pass, among them agc_driver_tessellation, and 14 hardware- or display-dependent tests skip. check_main_sync fails test_paths_with_spaces_and_unicode_are_reported_verbatim because this machine's console code page prints é as é; it does not touch this change.
  • Ghost of Yōtei (PPSA26344). It sets GE_CNTL 0x8040 (BREAK_WAVE_AT_EOI 0). Its terrain DRAW_INDIRECT records hold 4 vertices and 2 to 3,157 instances of one 4-point patch each. With this change the hull compiles for every record and the title renders past these draws to its post-load popover.

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 in TechnicalDebt (the unimplemented domain case is a refusal)
  • No notes, investigation .md files or images added to the repository
  • New third-party code is a submodule built from source (none)
  • Generic behaviour, not specific to one title
  • Depends on: none (fix(shader): record BDA faults in hull programs instead of refusing them #4222 is merged)
  • AI-assisted: yes

🤖 Generated with Claude Code

@github-actions github-actions Bot added needs rebase Conflicts with main, rebase needed shader core/shader docs agc libSceAgc* labels Oct 12, 2026
@blawesom

Copy link
Copy Markdown
Contributor

Rebase handoff — this PR is the 6th link of the hull/tessellation chain (it contains #4222 underneath), so I stacked the rebase on the already-rebased chain through rebase/4222. Authorship unchanged.

One conflict in ShaderValidation.cpp: both sides add conditions to the same capability whitelist — kept main's ClipDistance condition and this PR's DrawParameters condition together.

Verified on Linux (CachyOS, kernel 7.2.9, GCC 16.2.1, Ninja, Release, BUILD_TESTING=ON, AMD RX 9070 XT / RADV):

  • builds; tools/check_conventions.py --base origin/main passes
  • full suite: 949/949 passed

To pick it up (whole chain):

git fetch https://github.com/blawesom/AnyPS5 rebase/4114 rebase/4163 rebase/4172 rebase/4201 rebase/4222 rebase/4450

Head is 5d64d741a. Worth listing Depends on: #4114 → #4163 → #4172 → #4201 → #4222 so the chain merges in order.

@Pipodi

Pipodi commented Oct 13, 2026

Copy link
Copy Markdown
Contributor Author

@Celegans12 @blawesom thanks both for the rebase offers. I've rebased this onto #4222's current head (3f165b6db) the way Celegans12 described: the commit now builds on #4201's breakAtInstance decode and serialization instead of adding its own, the hull and domain keys hold TessellationInstancePatches instead of the bit, and #4201's refusal is replaced by the numbering. #4201's two "BREAK_WAVE_AT_EOI is clear" refusal cases are dropped from agc_driver_tessellation, and the instanced copy in agc_shader_memory clears breakAtInstance. New head 26ff6ad16. On Windows (RTX 4090), agc_driver_tessellation, agc_driver_graphics, agc_shader_memory and agc_shader_bindless pass. No need for either rebase branch now.

Tested and written by Claude (AI assistant) on a contributor's machine.

@Pipodi

Pipodi commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto main e34c47921 now that #4222 is merged. This PR is now its one commit, with head 10337ce9e.

Three spots conflicted only because main added entries next to this PR's, so both are kept:

  • StageInputKind keeps LayerExported and adds BaseInstance.
  • ShaderValidation keeps the mesh ClipDistance condition next to the DrawParameters one.
  • Draw.cpp keeps the dereferenced materializeDrawStage result with the hull and domain draw arguments.

On Windows (RTX 4090), the full suite gives 1183 pass and 14 skip, with agc_driver_tessellation among the passes. The one failure is check_main_sync's unicode-path case, from this machine's console code page. Conventions pass.

Tested and written by Claude (AI assistant) on a contributor's machine.

@adriadam10 adriadam10 self-assigned this Oct 14, 2026
@github-actions

github-actions Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

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.

Pull request Conflicting files (lines on main) Same exports Same new files
#5402 docs/dev/debt/shader.md L106-107
#5799 docs/dev/debt/shader.md L107-108
#5804 core/libs/prx/libSceAgcDriver/Execution/src/ShaderDeviceProfile.cpp L32
#5925 core/libs/prx/libSceAgcDriver/Execution/src/Driver/Draw/Draw.cpp L176

@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 14, 2026
Pipodi and others added 2 commits October 14, 2026 19:08
…nces

With GE_CNTL.BREAK_WAVE_AT_EOI clear, the instances of a tessellated draw share
hull threadgroups: the relative patch index counts on through the instances,
while the host's PrimitiveID restarts with each one. The local stage now passes
its draw-relative instance (InstanceIndex - BaseInstance) to the hull, which
numbers its patches instance * patches-per-instance + PrimitiveID before taking
(mod PRIM_GRP_SIZE) mod NUM_PATCHES. Instances that start a threadgroup anyway
(BREAK_WAVE_AT_EOI, or patches per instance a multiple of the group) keep the
per-instance PrimitiveID.

This replaces boykopovar#4201's refusal of a hull or domain that uses its relative index
as data while BREAK_WAVE_AT_EOI is clear, and builds on its decode of the bit
(breakAtInstance, request serialization and draw keys).

- Hull and domain cache keys hold the patches per instance instead of the bit;
  an indirect draw recompiles its hull and domain for each record's instance
  count.
- The device enables shaderDrawParameters when available. Without it, a hull
  using its relative index as data in such a draw is refused, as is a domain
  doing so (passing the instance on to the domain is not implemented).

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

Copy link
Copy Markdown
Collaborator

Checked on the PR merged onto main: build, agc_shader_memory, and agc_driver_tessellation on CI's lavapipe (it segfaults on local RADV on plain main too). Dropping the cross-instance numbering in groupedPatch makes the new RequireInstancedRelativePatches fail, so the test pins it.

Added one commit: the TechnicalDebt line for the relative patch index said it throws when BREAK_WAVE_AT_EOI is clear, which this PR makes false; it now describes the instance numbering and the remaining refusals (domain, no shaderDrawParameters). Merging once the full Build is green.

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

Labels

agc libSceAgc* docs shader core/shader

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants