Skip to content

fix(agc): give a depth stencil copy's layout barrier both aspects - #9057

Merged
adriadam10 merged 1 commit into
boykopovar:mainfrom
milescoviello:fix/sample-order-depth-copy-aspects
Oct 23, 2026
Merged

adriadam10 merged 1 commit into
boykopovar:mainfrom
milescoviello:fix/sample-order-depth-copy-aspects

Conversation

@milescoviello

@milescoviello milescoviello commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

What

The depth sample reorder (SampleOrder.cpp, Depth) copies the depth plane into an image of the surface's own format before redrawing it in the guest's order. For a D32_SFLOAT_S8_UINT surface that only reorders depth (SampleOrderMethod::Depth), the copy image is still D32S8. But its first transition to GENERAL named the depth aspect alone. Without separateDepthStencilLayouts, Vulkan requires a depth/stencil format's barrier to name both aspects. The Khronos layer reports this as VUID-VkImageMemoryBarrier-image-03320, 6 times over the sample order tests, from the 2x and 4x "depth format 130 through a depth pass" cases.

The barrier now takes its aspects from the format (HasStencil(target.format)), as transitionLocated in the same file already does. The copy region still moves depth only when only depth is reordered.

Tested

Ubuntu 24.04, GCC 13.3, NVIDIA RTX 4070 Ti, driver 595.99.02, on main 275e896.

Not tested: Intel.

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, driver 595.99.02, Ubuntu 24.04, and driver 615.71.09, Ubuntu 26.04.1; lavapipe (llvmpipe), Mesa 26.0.8, LLVM 21.1.8, Ubuntu 26.04.1; lavapipe (llvmpipe, LLVM 20.1.2, Vulkan 1.4.318), Ubuntu 24.04, Mesa version not recorded; AMD Radeon 680M, driver 32.0.21030.2001, Windows 11 (@DavidSamir); NVIDIA GeForce RTX 4090, driver 617.42, Windows 11 (@MaxLikesCode); lavapipe (llvmpipe), WSL2 Ubuntu 24.04, layer 1.3.275 (@xXPBSXx; no Mesa version given); NVIDIA GeForce RTX 5070 Ti, driver 617.42, and AMD Radeon iGPU 1002:13c0 (gfx1036), driver 32.0.21042.62, Windows 11 (@wachi35676); GitHub runners (lavapipe) (@EagleBearerYS); AMD Radeon RX 9070 XT (RDNA 4), Adrenalin 26.8.1, Windows 11 (@SoRadGaming)
  • AI-assisted: yes (Claude Code)

🤖 Generated with Claude Code

@DavidSamir

DavidSamir commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

AMD Radeon 680M (AMD 32.0.21030.2001, Windows) result for this PR together with #9056, under the validation layer: #9056 (comment). The VUID-VkImageMemoryBarrier-image-03320 errors (6 on main 27fabb6b7) are gone with both PRs.

AI-assisted: yes

@EagleBearerYS

Copy link
Copy Markdown
Contributor

Read against SampleOrder.cpp on main: this is right, and it fixes a bug in our #6683's code (57ceb9e). Thanks for finding it.

  • A D32S8 surface whose stencil isn't reordered (SampleOrderMethod::Depth, as when it has no stencil address) still gets a D32S8 copy from copyFor. That copy's first UNDEFINED to GENERAL barrier named the depth aspect alone. Nothing in the driver enables separateDepthStencilLayouts, so VUID-VkImageMemoryBarrier-image-03320 applies and the barrier must name both.
  • layoutAspects follows the copy's format, as transitionLocated does for the surface. The copy region and the descriptor stay depth-only, so only the layout barrier changes.
  • It was the only barrier in SampleOrder.cpp that named fewer aspects than its image's format has. The tests' own ToGeneral calls already take both.
  • Checked here, with this PR and test(agc): read the sample order tests' depth through a sampled copy #9056 merged on main 275e896 (run):
    • On GitHub's Linux runner (lavapipe), agc_driver_sample_order, agc_driver_sample_order_planning, agc_driver_pixel_sample_iteration and agc_driver_sample_location_grid_fallback pass 3 runs in a row, and the grid-reference and recipe tests skip.
    • On Windows, without a GPU, 4 of those 6 skip and 2 pass.
    • The runners have no validation layer, so the 03320 count isn't reproduced here.

@milescoviello

Copy link
Copy Markdown
Contributor Author

Thanks @EagleBearerYS for the review, and for checking that no other barrier in SampleOrder.cpp names fewer aspects than its image's format. Thanks @DavidSamir for the 680M run under the layer. Both are in the Tested section now.

AI-assisted: yes (Claude Code)

@xXPBSXx

xXPBSXx commented Oct 21, 2026

Copy link
Copy Markdown
Contributor

Validation-layer data point for this PR (data only).

Setup: Linux (WSL2 Ubuntu 24.04, GCC 13.3, Release), llvmpipe, vulkan-validationlayers 1.3.275.0-1 from the Ubuntu archive (older than the SDK a driver ships with), VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation. main 950388bfd; this PR's change (taken as git diff upstream/main...refs/pr/9057) applied on top of it.

Before (main): across all 483 built agc_driver_*_tests the layer reports VUID-VkImageMemoryBarrier-image-03320 once, from agc_driver_sample_order: barrier on a depth/stencil image (D32_SFLOAT_S8_UINT) with only the DEPTH aspect and no separateDepthStencilLayouts.

After (this PR): agc_driver_sample_order run again with the layer: 0 reports of that VUID, test passes. VUID-VkWriteDescriptorSet-descriptorType-00337 (a depth view without SAMPLED usage used as a sampled image) is still reported 3 times in that run's log; #9056 is about that one.

One run each, on llvmpipe only.

Not run: other GPUs for the layer run, other platforms, the other tests with the change under the layer.

AI-assisted (Claude Code).

@milescoviello

Copy link
Copy Markdown
Contributor Author

Thanks @xXPBSXx. So 03320 also comes only from agc_driver_sample_order across the 483 tests, and this PR clears it there. I added it to the Tested section.

AI-assisted: yes (Claude Code)

@github-actions

github-actions Bot commented Oct 22, 2026 •

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 7642 core/libs/prx/libSceAgcDriver/Graphics/src/SampleOrder.cpp
PR 9508 core/libs/prx/libSceAgcDriver/Graphics/src/SampleOrder.cpp
PR 10193 core/libs/prx/libSceAgcDriver/Graphics/src/SampleOrder.cpp
PR 10194 core/libs/prx/libSceAgcDriver/Graphics/src/SampleOrder.cpp

The depth sample reorder copies a D32S8 surface into an image of the
same format. When only depth is reordered, the copy's transition to
GENERAL named the depth aspect alone, which Vulkan does not allow for a
depth/stencil format without separateDepthStencilLayouts
(VUID-VkImageMemoryBarrier-image-03320). The barrier now names both
aspects of a format with stencil, as transitionLocated does; the copy
itself still moves depth only.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@milescoviello
milescoviello force-pushed the fix/sample-order-depth-copy-aspects branch from 78254be to 7333de6 Compare October 23, 2026 18:56
@milescoviello

Copy link
Copy Markdown
Contributor Author

Rebased onto main d2e42c3 as 7333de6aee, with the commit unchanged (git range-diff), together with #9056 (53949c0775). On the RTX 4070 Ti (615.71.09, Linux), the four sample_order tests with both PRs pass 4 of 4, 3 runs out of 3. This PR alone still fails three of them there, as the Tested section says: #9056 fixes those. The full table is on #9056.

AI-assisted: yes (Claude Code)

@adriadam10 adriadam10 self-assigned this Oct 23, 2026
@adriadam10
adriadam10 merged commit a23f8fc into boykopovar:main Oct 23, 2026
12 checks passed
@adriadam10

Copy link
Copy Markdown
Collaborator

Checked the barrier in SampleOrder.cpp: the depth-only copy is still a D32_SFLOAT_S8_UINT image, and without separateDepthStencilLayouts its barrier must name both aspects. It now follows HasStencil(target.format) like transitionLocated, while the copy region still moves depth only. Third-party runs under the validation layer (680M, RTX 4090, lavapipe) show VUID-VkImageMemoryBarrier-image-03320 gone; merged.

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.

5 participants