Skip to content

fix(agc): store D16 depth with stencil as D24 or D32 where the device lacks it - #3846

Merged
adriadam10 merged 4 commits into
boykopovar:mainfrom
tv7:fix/d16s8-depth-fallback
Oct 12, 2026
Merged

adriadam10 merged 4 commits into
boykopovar:mainfrom
tv7:fix/d16s8-depth-fallback

Conversation

@tv7

@tv7 tv7 commented Oct 11, 2026

Copy link
Copy Markdown
Contributor

What

A 16-bit depth target with stencil decodes to VK_FORMAT_D16_UNORM_S8_UINT (State.cpp, zFormat == 1 with stencil). NVIDIA devices do not offer that format as a depth/stencil attachment, so DepthSurface threw depth/stencil format 128 cannot be an attachment on this device.

The depth surface lives only on the host: it is cleared at creation and sampled through views, never copied from or to guest memory. So the host image can use a wider format:

  • New HostDepthFormat(context, format): for D16_UNORM_S8_UINT it returns the first of D16_UNORM_S8_UINT, D24_UNORM_S8_UINT, D32_SFLOAT_S8_UINT that the device can attach and sample (cached per physical device). Every other format is returned unchanged.
  • DepthSurface creates its image, view and sampled textures with it, and the pipeline uses it for the render pass attachment, the dynamic rendering formats and the Begin depth format.
  • Guest-side decisions still use the guest format: 16-bit sampled views (R16_UNORM or 16 depth bits) of the plane, the depth-bias units check in decodeDepthBias, and the pipeline keys.
  • Depth bias: the constant is in units of the format's resolution, so HostDepthBiasConstant scales it by 256 for D24 storage. For D32 float storage the unit depends on the depth value, so a non-zero constant there throws (not implemented). The slope factor needs no change.

Tested

Windows 11, RTX 3070 (no D16_UNORM_S8_UINT attachment support), GCC 15.2.0 UCRT, Release, on main a11c8f7f7.

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
  • No notes, investigation .md files or images added to the repository
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • AI-assisted: yes, Claude Code

🤖 Generated with Claude Code

@adriadam10

Copy link
Copy Markdown
Collaborator

Checked: every use of the depth format (image, view, sampled textures, render pass, dynamic rendering, Begin, depth bias in all three paths); guest-side decisions (16-bit views, bias units check, pipeline keys) correctly keep the guest format. The bias scale (2^-16 → 2^-24 = x256) is right, and the D32 constant bias throws instead of guessing. agc_driver_graphics passes with the new case, and the case does not build without the change.

Added one commit: a TechnicalDebt line under Functional for the D24/D32 precision difference and the D32 bias throw (checklist: unverified choices get a line). Merging once CI is green.

@tv7
tv7 force-pushed the fix/d16s8-depth-fallback branch from b738cd4 to 7f5cd94 Compare October 11, 2026 22:23
@github-actions github-actions Bot added docs and removed needs rebase Conflicts with main, rebase needed labels Oct 11, 2026
@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 12, 2026
@oneandonlydean

Copy link
Copy Markdown
Collaborator

Conflicts with main in DepthSurface.cpp since #2169: it added SampleLocationsCompatibleDepth(context, target.format, target.samples) just above info.format in the DepthSurface constructor. Please rebase; that call checks the image being created, so it should take hostFormat too.

@blawesom

Copy link
Copy Markdown
Contributor

Rebase handoff — rebased onto current main, authorship unchanged (both commits).

Two conflicts, both resolved: (1) DepthSurface.cpp — main added SampleLocationsCompatibleDepth where this PR adds HostDepthFormat/HostDepthBiasConstant; kept both, independent functions. (2) TechnicalDebt.md — union, both entries kept.

Verified on Linux (CachyOS, kernel 7.2.9, GCC 16.2.1, Ninja, Release, BUILD_TESTING=ON, AMD RX 9070 XT / RADV — a device that has native D16_UNORM_S8_UINT, so this PR's fallback path is not exercised here, only that nothing regresses):

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

To pick it up:

git fetch https://github.com/blawesom/AnyPS5 rebase/3846
git checkout <your branch>
git merge --ff-only FETCH_HEAD

Head is 042ff1dff.

@tv7
tv7 force-pushed the fix/d16s8-depth-fallback branch from 7f5cd94 to c2160f0 Compare October 12, 2026 17:20
@tv7

tv7 commented Oct 12, 2026

Copy link
Copy Markdown
Contributor Author

Took @blawesom's rebase (thanks!) and rebased it on main 2a5eca1. Per @oneandonlydean: SampleLocationsCompatibleDepth now gets the host format, both in the DepthSurface constructor (hostFormat) and in ValidateSampleLocations (HostDepthFormat(context, state.depth->format)), so the pipeline and the image ask about the same format (c2160f0). Windows/MinGW: 973/973.

@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 12, 2026
@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 12, 2026
@tv7
tv7 force-pushed the fix/d16s8-depth-fallback branch from c2160f0 to 574d7ec Compare October 12, 2026 19:14
@tv7

tv7 commented Oct 12, 2026

Copy link
Copy Markdown
Contributor Author

Rebased on main d539606 (574d7ec). The only conflict was docs/dev/TechnicalDebt.md after 342a8c4: the D16 stencil entry now sits in docs/dev/debt/libSceAgcDriver.md under Functional (link depth adjusted). This also undoes a join from the earlier union, where the entry had run into the next line. Windows/MinGW: 995/995.

@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 12, 2026
@github-actions

github-actions Bot commented Oct 12, 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
#4500 core/libs/prx/libSceAgcDriver/Graphics/include/DepthSurface.hpp L28
#4683 core/libs/prx/libSceAgcDriver/tests/DepthSurfaceReuse.cpp L8-9, L33-35

tv7 and others added 4 commits October 12, 2026 22:13
… lacks it

A 16-bit depth target with stencil maps to VK_FORMAT_D16_UNORM_S8_UINT,
which NVIDIA devices do not support as an attachment, so creating it threw.
The surface, its views and the pipelines now use the first of D16_UNORM_S8_UINT,
D24_UNORM_S8_UINT and D32_SFLOAT_S8_UINT the device can attach and sample.
Guest-side choices (16-bit sampled views, bias units) still follow the guest
format. A constant depth bias is scaled by 256 for D24 storage; on D32 float
storage it throws, as its unit depends on the depth value.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
The depth image is created with the host format, so the sample locations
check for the image and for the pipeline asks about that format too.

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

Copy link
Copy Markdown
Collaborator

Checked the new head: SampleLocationsCompatibleDepth now gets the host depth format in the DepthSurface constructor and in ValidateSampleLocations, as @oneandonlydean asked, so the pipeline and the image query the same format. The debt entry in docs/dev/debt/libSceAgcDriver.md is correct after the move. agc_driver_graphics passes with the D16/D24/D32 cases; libSceAgcDriver ctest 452/452.

Added one commit: dropped the duplicated DepthSurface.hpp include in Pipeline.cpp. Merging once CI is green.

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

Labels

agc libSceAgc* docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants