Repository navigation
Conversation
|
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.
|
a349da2 to
2ddfffc
Compare
A draw whose vertex shader exports the render target array index into a color view of several slices of a 2D array target was refused. The view now renders layers lastSlice - slice + 1 through a 2D array attachment view of the array's resident image starting at its first slice, and the draw marks every rendered slice dirty. A view of several slices of a surface that is not a 2D array is still refused, as is a layered draw without the array's resident image. AI-assisted: yes
…of its view's slices A layered draw into a color view of slices 0 to 5 of a 2D surface whose CB_COLOR_ATTRIB3 declares one slice was refused. The CB clamps the render target array index to the view's SLICE_MAX, not to the surface's slice count, so each layer is written one slice pitch further on. PPSA01317 renders a 512x512 cube map this way. Such a view now renders as a 2D array of the view's last slice + 1 slices. AI-assisted: yes
…the volume layer guard The resident image of a 2D array color view only compared the view's own slice offset with the image's layer pitch, which is trivially equal for slice 0. Every slice of the array image is now checked against the slice size DecodeColorBuffer offsets the view by. AttachmentView again requires layer 0 for a 3D target, as it did before layered 2D array views. AI-assisted: yes
A vertex shader exporting the render target index draws one value per layer into a CB_COLOR_VIEW of slices 1 to 3 of a four-slice 2D array and into slices 0 to 2 of a surface of one slice. After the write-back every slice of guest memory must hold its layer's value, and the slices outside the view (slice 0, and slice 3 of the one-slice surface) their initial bytes. AI-assisted: yes
A layered draw (the vertex shader exports the render target array index) with a depth target was refused. PPSA01317 renders a 512x512 cube map this way: a six-slice color view with a DB_DEPTH_VIEW of slices 0 to 5. The depth view now renders as many layers as the color views, or its own slices in a depth-only layered draw. The depth surfaces of those slices share one single-sample array image, one layer each, and the draw attaches a 2D array view of it. Each slice keeps its own Z, stencil and HTILE address, plane ownership, HTILE clears and copies, and a later draw into one slice binds that slice's layer. A depth view of another slice count than the color views, a multisampled layered depth target and one without a depth plane throw. AI-assisted: yes
2ddfffc to
5589752
Compare
|
Rebased onto main 5f8d2a2 via the rebased #7254 (force-push, head 5589752). One conflict, in DepthSurface.cpp DepthSurfaceView: #7131 changed the inline bind code to use NoteHtileClear() instead of OR-ing pendingClear, and this PR had moved that code into bindSurface(). I kept bindSurface() and applied the NoteHtileClear() calls inside it, so the HTILE clears drop a pending plane fill for both single and per-slice binds. Each slice keeps its own depthFill/stencilFill, and RecordClear clears only its own layer. Rebuilt; all 666 agc_* tests pass, including agc_driver_layered_depth_target, agc_driver_depth_, agc_driver_stencil_, agc_driver_native_multisample_depth, agc_driver_sample_mask_depth_stencil, agc_driver_layered_array_color_draw and the new agc_driver_depth_plane_fill. check_conventions.py and check_test_registration.py are clean. AI-assisted: yes |
Adapt gjonn shared depth ownership from upstream PR boykopovar#7256, commit 5589752, while retaining the framebuffer layer-count API. Record repeated draws directly into the shared array and preserve per-slice sampled and storage views. Test overlapping views, HTILE clears, sampled planes, and repeated draw recipes. Co-authored-by: Gjon Gojcevic <gjonn@hotmail.com>
Adapt gjonn shared depth ownership from upstream PR boykopovar#7256, commit 5589752, while retaining the framebuffer layer-count API. Record repeated draws directly into the shared array and preserve per-slice sampled and storage views. Test overlapping views, HTILE clears, sampled planes, and repeated draw recipes. Co-authored-by: Gjon Gojcevic <gjonn@hotmail.com>
Adapt gjonn shared depth ownership from upstream PR boykopovar#7256, commit 5589752, while retaining the framebuffer layer-count API. Record repeated draws directly into the shared array and preserve per-slice sampled and storage views. Test overlapping views, HTILE clears, sampled planes, and repeated draw recipes. Co-authored-by: Gjon Gojcevic <gjonn@hotmail.com>
Adapt gjonn shared depth ownership from upstream PR boykopovar#7256, commit 5589752, while retaining the framebuffer layer-count API. Record repeated draws directly into the shared array and preserve per-slice sampled and storage views. Test overlapping views, HTILE clears, sampled planes, and repeated draw recipes. Co-authored-by: Gjon Gojcevic <gjonn@hotmail.com>
Integrate the remaining regression coverage from Gjonn PRs boykopovar#7238, boykopovar#7243 and boykopovar#7256 while retaining the larger storage heap, packed float blending and shared depth arrays. The offset folding from boykopovar#7240 is already covered. Co-authored-by: Gjon Gojcevic <gjonn@hotmail.com>
Adapt gjonn shared depth ownership from upstream PR boykopovar#7256, commit 5589752, while retaining the framebuffer layer-count API. Record repeated draws directly into the shared array and preserve per-slice sampled and storage views. Test overlapping views, HTILE clears, sampled planes, and repeated draw recipes. Co-authored-by: Gjon Gojcevic <gjonn@hotmail.com>
Integrate the remaining regression coverage from Gjonn PRs boykopovar#7238, boykopovar#7243 and boykopovar#7256 while retaining the larger storage heap, packed float blending and shared depth arrays. The offset folding from boykopovar#7240 is already covered. Co-authored-by: Gjon Gojcevic <gjonn@hotmail.com>
Adapt gjonn shared depth ownership from upstream PR boykopovar#7256, commit 5589752, while retaining the framebuffer layer-count API. Record repeated draws directly into the shared array and preserve per-slice sampled and storage views. Test overlapping views, HTILE clears, sampled planes, and repeated draw recipes. Co-authored-by: Gjon Gojcevic <gjonn@hotmail.com>
Integrate the remaining regression coverage from Gjonn PRs boykopovar#7238, boykopovar#7243 and boykopovar#7256 while retaining the larger storage heap, packed float blending and shared depth arrays. The offset folding from boykopovar#7240 is already covered. Co-authored-by: Gjon Gojcevic <gjonn@hotmail.com>
|
RTX 4090 data point on PPSA01317, on top of my #7254 run (same stack, same box). Short version: with this PR the cube map pass gets through its depth side as well, 3 of 3 runs, and the next stop is the sample of the cube it just rendered: Setup. Gran Turismo 7 PPSA01317 01.000.001 (base package), Windows 11 (26200), RTX 4090 (NVIDIA 616.92), AMD Ryzen 7 5800X, MinGW-w64 GCC 15.2.0 r7 (UCRT), Release Result. All three runs get past That message is #5958's
The chain on this box so far, all on a stderr of run 3 (warm, box idle) from the second 10 s summary to the abort, the 392 per-shader
|
Adapt gjonn shared depth ownership from upstream PR boykopovar#7256, commit 5589752, while retaining the framebuffer layer-count API. Record repeated draws directly into the shared array and preserve per-slice sampled and storage views. Test overlapping views, HTILE clears, sampled planes, and repeated draw recipes. Co-authored-by: Gjon Gojcevic <gjonn@hotmail.com>
Adapt gjonn shared depth ownership from upstream PR boykopovar#7256, commit 5589752, while retaining the framebuffer layer-count API. Record repeated draws directly into the shared array and preserve per-slice sampled and storage views. Test overlapping views, HTILE clears, sampled planes, and repeated draw recipes. Co-authored-by: Gjon Gojcevic <gjonn@hotmail.com>
Integrate the remaining regression coverage from Gjonn PRs boykopovar#7238, boykopovar#7243 and boykopovar#7256 while retaining the larger storage heap, packed float blending and shared depth arrays. The offset folding from boykopovar#7240 is already covered. Co-authored-by: Gjon Gojcevic <gjonn@hotmail.com>
Adapt gjonn shared depth ownership from upstream PR boykopovar#7256, commit 5589752, while retaining the framebuffer layer-count API. Record repeated draws directly into the shared array and preserve per-slice sampled and storage views. Test overlapping views, HTILE clears, sampled planes, and repeated draw recipes. Co-authored-by: Gjon Gojcevic <gjonn@hotmail.com>
Integrate the remaining regression coverage from Gjonn PRs boykopovar#7238, boykopovar#7243 and boykopovar#7256 while retaining the larger storage heap, packed float blending and shared depth arrays. The offset folding from boykopovar#7240 is already covered. Co-authored-by: Gjon Gojcevic <gjonn@hotmail.com>
Adapt gjonn shared depth ownership from upstream PR boykopovar#7256, commit 5589752, while retaining the framebuffer layer-count API. Record repeated draws directly into the shared array and preserve per-slice sampled and storage views. Test overlapping views, HTILE clears, sampled planes, and repeated draw recipes. Co-authored-by: Gjon Gojcevic <gjonn@hotmail.com>
Integrate the remaining regression coverage from Gjonn PRs boykopovar#7238, boykopovar#7243 and boykopovar#7256 while retaining the larger storage heap, packed float blending and shared depth arrays. The offset folding from boykopovar#7240 is already covered. Co-authored-by: Gjon Gojcevic <gjonn@hotmail.com>
|
@gjonn This PR now conflicts with main in |
XairRrR
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
The layered depth path builds and its tests pass at the PR head. But the branch no longer merges into current main, and a rebase has to rework the code it touches, so the change needs a rebase and a fresh test run.
What I ran
- PR head:
5589752147fe73b1487bce01b985b83e70f42b15 main:52c09d567d2fa453494dd720fcf53a98149468e4- PR merge base:
5f8d2a252437a8dc607f0f52239946bb3f9b22a2
All commands ran in a clean worktree on Arch Linux 7.2.11-zen, GCC 16.2.1, Intel Core i9-14900KF, NVIDIA GeForce RTX 4070 Ti SUPER (driver 615.78.08), with a Release, Ninja, BUILD_TESTING=ON build.
git fetch origin main pull/7256/head:refs/review/7256
git checkout --detach origin/main
git merge --no-ff --no-edit refs/review/7256 # conflicts, aborted
git checkout --detach refs/review/7256 # submodules synced to the PR's base
git diff --check origin/main...refs/review/7256
python3 tools/check_conventions.py --base origin/main
cmake --build build -j10 --target agc_driver_layered_depth_target_tests agc_driver_graphics_tests \
agc_driver_layered_array_color_draw_tests agc_driver_array_layer_target_tests agc_driver_depth_*_tests \
agc_driver_layered_volume_*_tests agc_driver_native_multisample_depth_tests \
agc_driver_sample_mask_depth_stencil_tests agc_driver_stencil_*_tests libs
VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 \
VK_LOADER_DEBUG=error,warn,layer ctest --test-dir build -j4 -V --output-on-failure \
-R '^(agc_driver_graphics|agc_driver_array_layer_target|agc_driver_depth_.*|agc_driver_layered_.*|agc_driver_native_multisample_depth|agc_driver_sample_mask_depth_stencil|agc_driver_stencil_.*)$'
# same build and ctest at the merge base 5f8d2a252 (without the two layered tests it lacks)
# negative control: see belowResults
| Check | Result |
|---|---|
Merge into main 52c09d5 |
Conflicts in 6 files, 20 hunks (see Findings) |
git diff --check |
clean |
tools/check_conventions.py --base origin/main |
All conventions checks passed. |
Build at PR head (test targets above and libs) |
OK, no new warnings in the changed files |
| 27 related tests at PR head, with validation | 26 passed, agc_driver_stencil_ref_export skipped (this device does not support it) |
agc_driver_layered_depth_target |
Passed, 0.35 s |
| Same 25 tests at merge base 5f8d2a2 | 24 passed, the same 1 skipped |
VUID- lines, PR head vs merge base |
40 vs 40, all VUID-vkBeginCommandBuffer-commandBuffer-00050 from agc_driver_depth_storage and agc_driver_native_multisample_depth; none come from the changed tests, and the PR adds none |
was not found but was requested by env var VK_INSTANCE_LAYERS |
absent from both logs; the loader inserted VK_LAYER_KHRONOS_validation in every test |
I compared validation against the PR's merge base, not against main. The merged tree does not exist, and 11,535 commits separate the base from main, so a main run would not isolate this change.
Merge conflicts
CONFLICT (content): core/libs/prx/libSceAgcDriver/Graphics/include/Texture.hpp (2 hunks)
CONFLICT (content): core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp (9 hunks)
CONFLICT (content): core/libs/prx/libSceAgcDriver/Graphics/src/Draw.cpp (5 hunks)
CONFLICT (content): core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp (1 hunk)
CONFLICT (content): core/libs/prx/libSceAgcDriver/tests/Graphics.cpp (2 hunks)
CONFLICT (content): docs/dev/debt/libSceAgcDriver.md (1 hunk)
ctest at PR head (excerpt)
525: layered depth target tests passed
16/27 Test #525: agc_driver_layered_depth_target ........... Passed 0.35 sec
319: Graphics validation tests passed
524: layered array color draw tests passed
100% tests passed out of 27
654 - agc_driver_stencil_ref_export (Skipped)
Validation messages (same in both runs)
364: Validation Error: [ VUID-vkBeginCommandBuffer-commandBuffer-00050 ] | MessageID = 0xb24f00f5
364: vkBeginCommandBuffer(): VkCommandBuffer ... was recorded and now in the executable state. You must call vkResetCommandBuffer() before calling vkBeginCommandBuffer() again.
20 lines from agc_driver_depth_storage and 20 from agc_driver_native_multisample_depth, at both the PR head and the merge base.
Negative control
At the PR head I restored DepthSurface.cpp, Draw.cpp, State.cpp, Texture.cpp and Texture.hpp from ebe289bdc7, the PR's last commit before its own change. I kept State.hpp (the new DepthTarget::layers field) and the tests, then rebuilt. Both tests fail:
agc_driver_graphics: AGC graphics: layered rendering with a depth target is unsupported
agc_driver_layered_depth_target: AGC graphics: a layered draw after a draw into depth slice 2 alone: slice 1 texel (0, 0) is 0.250000, expected 0.750000 (the depth test of a ramp from 0 to 1 over 32 texels passes on the first 32)
After the run I restored the PR's files.
Findings
Blocking
-
The branch conflicts with
main. This confirms the conflict list in the earlier comment from oneandonlydean: the same six files, 20 hunks. The conflicts touch the code this PR changes. A rebase has to reapply the per-layer logic to code thatmainhas since rewritten:- In the depth plane fill on
main, the direct image copy and theUNDEFINED -> TRANSFER_DST_OPTIMALdiscard barriers hard-code array layer 0 (DepthSurface.cpp#L735-L780 on main). That was correct for a one-layer image. On the shared array image this PR introduces, a fill of slice k would discard and overwrite layer 0, slice 0's contents. All four depth-image subresources there needlayer:depthRegion, the destination ofcopy, and bothdiscard.subresourceRange. The new test should also cover a direct-copy fill of a non-zero slice. - The surface constructor on
mainallocates throughAllocateImageMemory(L316). Thearray != nullptrbranch has to keep that path. - The depth
Textureconstructor onmaingained aVkImageUsageFlags usageparameter beforelayerCount. This PR'sSampled()call passes..., layer, owned, 1u. Rebased as is, that1uwould becomeusagerather thanlayerCount. Sampled()onmainnow goes through the multisample sample shadow (around L481). Thelayer,1uarguments must be carried into that call.
Because the resolved code will differ from what was tested, the GPU results here and in the description need rerunning after the rebase.
- In the depth plane fill on
Non-blocking
- The debt file keeps a contradicting entry. The "HTILE address of a depth array slice" entry (debt line 42 at the PR head, line 76 on main) says
decodeDepthdoes not add the slice offset tohtileAddress, so all slices share slice 0's HTILE. ButdecodeDepthalready adds it (State.cpp:310 at the PR head, since 8fec47a). The new "Layered depth targets" entry (line 144) relies on that per-slice HTILE address. The stale entry predates this PR, but this PR now documents the opposite behaviour. Please remove or rewrite it while resolving the debt-file conflict. - The description's dependency is out of date. #7254 was merged on 2026-10-19 from a later head,
deb0ba305. This branch still carries its four pre-merge commits. After rebasing, those commits should go,Depends onshould readnone, and the description should drop "the first four commits of this branch are #7254's".
I found no correctness problem in the PR's own logic at its base. layerTarget steps Z, stencil and HTILE by one slice from the decoded first slice, which matches decodeDepth. The layered image's view is torn down before the surfaces that share its OwnedImage. The unimplemented cases (multisampled, stencil-only, mismatched slice counts) throw.
Not verified
- A build or tests of the merged tree, because it does not merge.
- The PPSA01317 claim and the RTX 4090 report in the comments. I did not launch any title.
- Windows/MinGW builds.
agc_driver_stencil_ref_export, which also skips on this device.
To get this to a good state
- Rebase onto current
main, dropping the pre-merge #7254 commits. Then carrylayerthroughmain's direct depth copy, discard barriers,AllocateImageMemorypath and sample-shadowTextureconstruction. - Add a check to
agc_driver_layered_depth_targetthat fills a non-zero slice through the direct depth copy and then reads slice 0 back unchanged. - Remove or rewrite the stale "HTILE address of a depth array slice" debt entry.
- Rerun the layered, depth, stencil and graphics tests on the rebased branch, with the validation layer, on
mainand on the change. UpdateDepends ontonone.
|
@gjonn A focused current-main correction from the Sonic investigation is now #14084 (issue #14083): a newly created Your published layered |
|
Thank you for your work on this. Main has moved, and the PR now conflicts in:
Could you merge current Thank you! |
CnRJay
left a comment
There was a problem hiding this comment.
Points on 7256:
-
Comments in code.
Draw.cpp("DecodeColorBuffer offsets slice s by s times this size; every slice the image stores...") andtests/execution/LayeredArrayColorDraw.cpp(a four-line header comment) add comments. The rule is no comments except#endifand namespace ends. -
Graphics test protocol. This is an AGC change, so the same focused tests need to be run on main and on the change with
ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation, same machine and GPU, with a statement that no newVUID-lines appear. Please fillGPUs testedwith that result. -
Negative control. Does the new layered-array test fail on main for case (a) (slices 1..3 of a four-slice array) and case (b) (slices 0..2 of a one-slice surface)? Please paste the failing main output.
|
@gjonn another title stops on the refusal this PR removes: My First Gran Turismo (PPSA24157) ends with I wanted to run this PR on it, but the head ( AI-assisted (Claude Code); the stop, time and frame count are from the run's |
Co-authored-by: gjonn <7807870+gjonn@users.noreply.github.com>
✅ NVIDIA GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (9 IDs in 593 tests)Skipped (55)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
What
Depends on: #7254. The first four commits of this branch are #7254's (its two changes, the review follow-up fix and its
agc_driver_layered_array_color_drawtest); this PR's own change is the last commit,feat(agc): render layered draws into a depth view of several slices.With #7254, PPSA01317 gets through the color side of a 512x512 cube map pass and then aborts with
layered rendering with a depth target is unsupported. The draw exports the render target array index into a six-slice color view, with aDB_DEPTH_VIEWof slices 0 to 5.DepthSliceBytesand oneHtileMetadataBytesfurther per slice). Each slice also keeps its own plane ownership, HTILE clears and plane copies. A later draw into one slice binds that slice's layer. Another layered view over some of the same slices (another first slice or slice count) is a separate array image that takes each slice by a whole-slice copy when it is bound.Behaviour change on an existing path: a depth-only layered draw into a multisampled or stencil-only depth view of several slices used to render silently into the first slice. It now throws.
Open debt:
SLICE_MAX. That clamp is not modelled; such a layer is undefined in Vulkan.#7194 changes the same HTILE slice-clear function in
DepthSurface.cpp(taking a later slice's clear when every word has ZMask 0). It works per slice and composes with the per-slice HTILE addresses here; the two branches merge without conflicts.Tested
Windows 11, NVIDIA GeForce RTX 5090 Laptop GPU, driver 616.92. Ninja/MinGW Release with
BUILD_TESTING=ON, on top of #7254 rebased onmain922dc89.agc_driver_layered_depth_targetcovers a six-layer depth-tested draw:agc_driver_graphics_tests: new decode cases for depth-only layered views, color plus depth views of matching, shifted and mismatched slice counts, and views without the layer export staying single-layer.agc_driver_stencil_ref_exportis skipped because the device does not support it. The set is the layered depth target, graphics and decode state tests, every depth/HTILE/stencil execution test (agc_driver_depth_*,agc_driver_native_multisample_depth,agc_driver_sample_mask_depth_stencil,agc_driver_stencil_*), the array layer target test and the layered volume and vertex layer export tests.tools/check_conventions.py --base origin/mainandtools/check_test_registration.pypass.mainwith feat(agc): render layered draws into 2D array color views #7254 (checked in boots).Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)