Skip to content

fix(agc): transition both aspects of combined depth/stencil images an… - #9807

Closed
Moeed2 wants to merge 1 commit into
boykopovar:mainfrom
Moeed2:fix/agc-depth-sample-order-validation
Closed

Moeed2 wants to merge 1 commit into
boykopovar:mainfrom
Moeed2:fix/agc-depth-sample-order-validation

Conversation

@Moeed2

@Moeed2 Moeed2 commented Oct 22, 2026 •

Copy link
Copy Markdown

What

Fixes two Vulkan validation errors in the multisampled depth sample-order paths. On an AMD RX 6700 XT (Windows) they made agc_driver_sample_order and agc_driver_sample_order_recipe_depth_locations fail. On lavapipe those tests pass, but the same errors are logged.

  • VUID-VkImageMemoryBarrier-image-03320 (driver)
    • Two layout transitions named only the depth aspect of a combined depth/stencil image, because they followed the guest target's stencil address instead of the host format. This happens when a depth-only target is hosted as VK_FORMAT_D32_SFLOAT_S8_UINT.
    • separateDepthStencilLayouts is not enabled, so the transitions now include the stencil aspect whenever the host format has one:
      • the UNDEFINED→GENERAL transition of a new DepthSurface
      • the sample-order depth copy image in SampleOrder.cpp
    • The clear and copy ranges are unchanged.
  • VUID-VkWriteDescriptorSet-descriptorType-00337 (test)
    • The sample-order tests read depth by binding DepthSurfaceOrderTarget(...).view as a sampled image. That is the surface's attachment view. When the driver samples through its sample shadow, the image is created without VK_IMAGE_USAGE_SAMPLED_BIT.
    • The tests now copy the depth aspect into a test-owned image with SAMPLED | TRANSFER_DST usage, at the same sample count, and sample that.
    • On the RX 6700 XT, sampling the attachment view returned garbage depth (1.0759e33).

Tested

  • Linux, lavapipe (Mesa, LLVM 20.1.2)
    • Ran ctest -R sample_order -V with ANYPS5_REQUIRE_VULKAN=1, ANYPS5_NO_SHADER_CACHE=1 and VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation.
    • main logs 03320 and 00337. The change logs neither, and all tests pass or skip as on main.
  • Windows 10 22H2, AMD RX 6700 XT, driver 26.8.1
    • Same environment, on main (b7d1d47) and on this change.
    • Ran ctest -V on the focused set sample_order|native_multisample_depth|depth (35 tests): 32 passed on main, 34 pass with the change.
      • agc_driver_sample_order: failed on main, passes with the change.
      • agc_driver_sample_order_recipe_depth_locations: failed on main, passes with the change.
      • agc_driver_sample_order_grid_reference still fails as on main: [device-fault] read invalid at 0x200000000, then VK_ERROR_DEVICE_LOST, with one VUID-VkSubmitInfo-pSignalSemaphores-03242. That is a separate fault and is left for a follow-up.
    • Validation errors, main → change: 03320 6 → 0 and 00337 7 → 0. No new VUID appears.
    • Full agc|shader|video set (931 tests): 5 failures on main, 3 with the change (the grid-reference test above, plus agc_driver_image_array_window_past_layer_limit and agc_driver_tessellation, both unrelated and unchanged).
  • The branch was rebased onto main at 093149c. None of the three new commits touch these files. After the rebase, the lavapipe run was repeated with the same result.
  • python3 tools/check_conventions.py --base origin/main passes.

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: AMD Radeon RX 6700 XT, AMD driver 26.8.1 (Vulkan 2.0.353), Windows 10 22H2 19045; llvmpipe (Mesa, LLVM 20.1.2), Ubuntu 24.04
  • AI-assisted: yes

🤖 Generated with Claude Code

https://claude.ai/code/session_015C6HgEib8H1FTuXHk5SjKa

…d sample a SAMPLED copy in sample order tests

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015C6HgEib8H1FTuXHk5SjKa

@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 two Vulkan fixes are well supported by the reported before/after validation-layer runs, and my exact-head conventions check plus git diff --check passed. The branch still needs overlap coordination before approval:

  1. Open #9057 changes Graphics/src/SampleOrder.cpp to transition the depth copy with both aspects when its format has stencil. This PR makes the same transition change in that file and additionally fixes the initial DepthSurface.cpp transition.
  2. Open #9056 changes tests/execution/SampleOrder.cpp to read depth through a test-owned sampled copy, addressing the same descriptor-usage VUID that this PR’s ReadDepth helper addresses.

Please coordinate with both authors, agree which PR lands first or whether one change supersedes part of the other, and record the resulting order/dependency on the checklist. The PR currently says Depends on: none; that does not reflect these shared files and overlapping fixes. I did not run the AGC runtime suite locally; the validation-layer and GPU runs in the description are contributor-reported. Hosted product tests are not present in this PR’s checks.

@github-actions

github-actions Bot commented Oct 22, 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 9056 core/libs/prx/libSceAgcDriver/tests/execution/SampleOrder.cpp L1124, L1172, L1351
PR 9057 core/libs/prx/libSceAgcDriver/Graphics/src/SampleOrder.cpp L196
PR 9950 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp L7

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
PR 6808 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.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 7642 core/libs/prx/libSceAgcDriver/Graphics/src/SampleOrder.cpp
PR 7705 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
PR 8999 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 9508 core/libs/prx/libSceAgcDriver/Graphics/src/SampleOrder.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 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

@Lindamea1

Copy link
Copy Markdown
Contributor

NVIDIA run on Linux, where the sample-order tests fail on main: head 07a6cac602 merged onto main 8499ea52e (clean). RTX 5090, NVIDIA 595.99.02, Gentoo (kernel 7.2.8), GCC 16.2.1, Release. ctest -V -R 'sample_order|native_multisample_depth|depth' (35 tests, your focused set) with ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation (layer 1.4.350). No log has was not found but was requested by env var VK_INSTANCE_LAYERS.

To separate the two halves of the PR, I also ran the four sample_order tests with only one half applied:

Build agc_driver_sample_order _recipe_depth_locations _grid_reference Validation errors
main fails fails fails 03320 ×3, 00337 ×9
main + this PR passes passes skipped none
test change only (main's DepthSurface.cpp and Graphics/src/SampleOrder.cpp) passes passes skipped 03320 ×3
source change only (main's tests/execution/SampleOrder.cpp) fails fails fails 00337 ×9
  • Counts are Validation Error: [ VUID-… ] messages. Each message repeats the ID in its spec link, so counting VUID- strings gives 6 and 18, the numbers in fix(agc): give a depth stencil copy's layout barrier both aspects #9057's description.
  • On main, agc_driver_sample_order and _grid_reference fail with the slopes did not leave depth both at and below the clear value. _recipe_depth_locations fails with the clear between the recorded draw and its replay left depth other than the clear value. With the test change, _grid_reference gets past its failure and skips with its own message: skipped, the device does not keep depth drawn at custom sample locations at those locations, even after a transition that names them: the composite of uniform renders holds the guest's depth in 0 of 3005 sloped samples, so no path can be compared with it bit for bit.
  • So on this GPU the pass/fail change comes from the test's ReadDepth copy, the same approach as test(agc): read the sample order tests' depth through a sampled copy #9056's ReadDepth (copy the depth aspect into a sampled test image and read that). The source change alone removes the three 03320 messages and changes nothing else in these four tests.
  • agc_driver_sample_order_planning passes in all four builds. The other 31 tests of the focused set pass on main and with the PR.
  • ctest -V -R '^(agc|shader|request_serializer)' (874 tests) under the layer: with the PR, 872 pass and 2 skip (agc_driver_stencil_ref_export, as on main, and _grid_reference). The only errors left are the 13 that main also has outside the sample-order tests (00605, 02275, 07285). The validation warnings are the same as on main.
  • Full ctest without the layer: all 1834 pass or skip with the PR. On main the three tests above fail.

AI-assisted: yes (Claude Code). The runs are from this machine.

@EagleBearerYS

Copy link
Copy Markdown
Contributor

@Moeed2 a merge-order note from #9533, ours, which #9717 and #9742 stack on. Both PRs change the same vkCmdClearDepthStencilImage call when a DepthSurface is created in Graphics/src/DepthSurface.cpp:

They compose: the call takes #9533's clear value and this PR's range. We'd propose that this PR goes first, and #9533 rebases over it and lists it under Depends on. If you'd rather go second, say so.

@Moeed2

Moeed2 commented Oct 23, 2026

Copy link
Copy Markdown
Author

Thanks @MarcZX10, @Lindamea1 and @EagleBearerYS for the review and the runs. I hit these failures independently on an RX 6700 XT and missed that #9056 and #9057 were already open for the same issue. Those cover it, with the same results on my card (Windows 10, driver 26.8.1), so I'm closing this in favour of them. Sorry for the noise.

@EagleBearerYS, so #9533 isn't held up waiting on this: I don't think this branch widened the vkCmdClearDepthStencilImage range. Before, it took toGeneral.subresourceRange, which was a copy of viewInfo.subresourceRange. Now it takes viewInfo.subresourceRange directly, so it's the same range. Only the UNDEFINED→GENERAL barrier gained the stencil aspect. Please correct me if I've misread it.

@Moeed2 Moeed2 closed this Oct 23, 2026
@EagleBearerYS

Copy link
Copy Markdown
Contributor

@Moeed2 thank you for writing, and you're right. On main the clear already took toGeneral.subresourceRange, a copy of viewInfo.subresourceRange, so passing viewInfo.subresourceRange directly keeps the same range. Only the UNDEFINED→GENERAL barrier gained the stencil aspect. Our note said the range was widened, which was wrong. With this closed, #9533 no longer lists it, and #9056 and #9057 don't touch #9533's files.

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