Skip to content

fix(agc): retire native depth before binding an overlapping layout - #10375

Open
Ondra97 wants to merge 1 commit into
boykopovar:mainfrom
Ondra97:codex/fix-native-depth-attachment-alias
Open

Ondra97 wants to merge 1 commit into
boykopovar:mainfrom
Ondra97:codex/fix-native-depth-attachment-alias

Conversation

@Ondra97

@Ondra97 Ondra97 commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

What

Binding a new depth attachment whose plane or HTILE overlapped an active native multisample depth owner was rejected as a layout/metadata change. Even an unwritten owner could not be replaced by another valid layout over the same guest memory.

Use the existing native-depth guest-access transition before constructing the new attachment: publish supported written contents, retire intersecting owners, then validate/create the new layout. Keep the existing attachment unchanged when its identity matches. Release the surface-cache lock before publication; the GPU lock continues to serialize the transition. Unsupported publication paths still throw.

Tested

Windows 11 Pro build 26300, WinLibs GCC 15.2.0, Release, RTX 4070 Ti SUPER, NVIDIA 617.42.

Overlap review: 28 shared-file PR diffs inspected. #6808 adds overlap diagnostics, #7256 changes creation-lock placement, #9533/#9717/#9742 add separate stencil handling, and #9950 adds written D32 publication. Their nearby edits need normal merge coordination; they do not provide this attachment-alias transition.
Independent validation of the published commit: Published head e393705239a3104846c1a91548e7c1fc9a2f9a38: full Windows Release build and native multisample depth, depth-storage dispatch, native-depth BDA clear/retirement/clear/refusal checks and patched-library validation pass on a clean tracked tree without overlays (7/7, zero skips). Vulkan is required, shader caches are disabled, and loader logs confirm actual Khronos instance/device validation layers; no VUIDs. Hardware and toolchain are the Windows/NVIDIA configuration listed above. This independently validates the published commit in addition to the earlier integration runs. The complete CTest suite was not run.
Review baseline comparison (2026-10-24): I ran the same seven focused CTest names on current main 7966e590ef7cb1054500eef513f99c59e05547b0 and compared its log with the clean, previously run published head e393705239a3104846c1a91548e7c1fc9a2f9a38. Both runs used this Windows 11 / RTX 4070 Ti SUPER host, Release / WinLibs GCC 15.2.0, ANYPS5_REQUIRE_VULKAN=1, ANYPS5_NO_SHADER_CACHE=1, ANYPS5_GPU=RTX 4070 Ti SUPER, VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation, the same Khronos layer path and VK_LOADER_DEBUG=error,warn,layer. The main-side build was limited to the four targets required by the selected tests; the PR head also passed the full Release build reported above.

ctest --test-dir <build> -R '^(agc_driver_native_multisample_depth|agc_driver_depth_storage_dispatch|agc_driver_native_depth_(bda_clear|retirement|clears|clear_refusals)|patched_libraries)$' -j1 -V

Log check Current main Published PR head
Same seven CTest names 7 passed, 0 failed, 0 skipped 7 passed, 0 failed, 0 skipped
Vulkan loader Inserted VK_LAYER_KHRONOS_validation at instance and device level Inserted VK_LAYER_KHRONOS_validation at instance and device level
VUID- identifiers / validation warnings or errors / missing requested layer 0 / 0 / 0 0 / 0 / 0
New attachment-alias fixture line in agc_driver_native_multisample_depth absent, since the case is introduced by this PR present: native depth attachment alias changes layout while retaining held GPU views

Both logs report Selected GPU name=NVIDIA GeForce RTX 4070 Ti SUPER. The matched JUnit test names and results, loader insertion lines and VUID scans were compared directly. The earlier red control on main with the new fixture overlaid is reported above; this baseline comparison uses each revision's own clean source.

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: NVIDIA GeForce RTX 4070 Ti SUPER, NVIDIA 617.42, Windows 11 Pro build 26300
  • AI-assisted: yes

@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.

The RTX 4070 Ti SUPER run is detailed and the new alias regression is meaningful. Before approval, the AGC validation rule still needs the same focused tests on both current main and this head, using ANYPS5_REQUIRE_VULKAN=1, ANYPS5_NO_SHADER_CACHE=1, and VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation, with logs compared for newly introduced VUID- errors. The description reports the red main behavior and the PR-side 51 checks, confirms the layer loaded and no final VUIDs, but does not show the equivalent main-side focused run/log or establish that the comparison used the same test set and environment. Please add those baseline results (including confirmation the layer was found and the VUID comparison), or explain the exact equivalent baseline procedure. I could not run this locally because the required GPU is unavailable here.

@github-actions

github-actions Bot commented Oct 23, 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
PR 8105 core/libs/prx/libSceAgcDriver/tests/execution/NativeMultisampleDepth.cpp L665-666
PR 9950 core/libs/prx/libSceAgcDriver/tests/execution/NativeMultisampleDepth.cpp L1297

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 1368 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
PR 1395 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
PR 6146 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
PR 6300 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
core/libs/prx/libSceAgcDriver/tests/execution/NativeMultisampleDepth.cpp
PR 6808 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
core/libs/prx/libSceAgcDriver/tests/execution/NativeMultisampleDepth.cpp
PR 6977 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
PR 7201 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
PR 7256 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
PR 7449 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
PR 8223 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
PR 9431 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
PR 9432 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
PR 9533 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
core/libs/prx/libSceAgcDriver/tests/execution/NativeMultisampleDepth.cpp
PR 9672 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
PR 9673 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
PR 9674 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
PR 9675 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
PR 9676 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
PR 9717 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
core/libs/prx/libSceAgcDriver/tests/execution/NativeMultisampleDepth.cpp
PR 9742 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
core/libs/prx/libSceAgcDriver/tests/execution/NativeMultisampleDepth.cpp
PR 9842 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
PR 9891 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
PR 9892 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
PR 9893 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
PR 9895 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp

@github-actions github-actions Bot added the agc libSceAgc* label Oct 23, 2026
@BKlaasWerkman

Copy link
Copy Markdown

Intel run, a second vendor next to the RTX 4070 Ti SUPER.

Setup: Intel Arc Graphics (Meteor Lake, 0x7d55, Windows driver 101.9150), Windows 11 Pro 10.0.26300, WinLibs GCC 15.2.0 r7, Release. Head e393705 was merged into main ec5b89a (clean merge). Environment: ANYPS5_REQUIRE_VULKAN=1, ANYPS5_NO_SHADER_CACHE=1 and VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation (1.4.363). The run uses ctest -R 'native_multisample_depth|native_depth|depth_surface' -j1 -V.

  • Results. This PR passes 7 of 7 and main passes 7 of 7. Neither log has a VUID- line or was not found but was requested.
  • The new case runs on this device. agc_driver_native_multisample_depth prints exactly one line more than on main: native depth attachment alias changes layout while retaining held GPU views. Its other 39 lines are the same on both builds.

AI-assisted: yes (Claude Code)

@Ondra97

Ondra97 commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

@MarcZX10 I added the requested baseline comparison to the Tested section. On this RTX 4070 Ti SUPER Windows host, current main 7966e590ef7cb1054500eef513f99c59e05547b0 and the unchanged published PR head e393705239a3104846c1a91548e7c1fc9a2f9a38 each pass the same seven named focused CTests (7/7, no skips). The PR-head result is the previously recorded clean-head run; I built and ran the current-main side today with the same Vulkan environment and test selection. Both verbose logs explicitly show VK_LAYER_KHRONOS_validation inserted at instance and device level. Comparison of the complete logs finds no VUID- identifiers, validation warnings/errors or missing-layer reports on either side. The new alias fixture line appears only on the PR head, as expected; the earlier red control with that fixture on main remains documented above. Could you recheck the requested validation evidence?

@adriadam10
adriadam10 dismissed MarcZX10’s stale review October 24, 2026 10:20

Dismissing a stale review: the requested changes were addressed after it (new head and/or updated description with the asked-for runs) and the merge queue blocks on it. Please request changes again if anything is still open.

This branch has not been deployed

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

Labels

agc libSceAgc*

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants