Repository navigation
feat(agc): honour the DEPTH row pitch of linear 2D textures as RDNA2 does - #11280
Conversation
|
Reviewed the exact diff and the reported hardware probe/test setup. The |
|
Thanks for the review. No order is needed and none of them is a prerequisite, so Each one conflicts with this PR in three places. All three are additions next to each other, resolved by keeping both sides:
The two paths don't interact. In those PRs, I merged #9891's head into this branch with that resolution and ran |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
I reviewed the updated exact head bf4b9e17d6433887e05143f09a93cd3834a565f1. The revised overlap explanation addresses the earlier question: it identifies the three adjacent insertions and describes why the PS4 GCN path does not consume linearPitch.
One required validation result is still missing. CONTRIBUTING.md requires the same focused AGC tests on main and the change, with validation enabled, on each reported GPU. The report shows agc_driver_graphics as n/a on main; only agc_driver_linear_texture_pitch is shown there. Please run the baseline agc_driver_graphics test (using the baseline test source if the new assertions depend on linearPitch) on both listed GPUs with the same validation settings, and add its outcome alongside the PR result. The hosted Linux/Windows product checks are skipped for this change, so CI does not supply that baseline evidence. I have not independently run these hardware tests.
|
Ran the baseline. I built
The validation layer was inserted in all four runs. No log has |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
The exact-head report now includes the missing baseline run on both listed GPUs, plus the same focused tests on the change, with validation-layer insertion confirmed and no new output. The prior baseline-evidence blocker is resolved. The current conventions check passes; hosted product tests remain path-skipped, and the GPU runs are author-reported rather than independently reproduced by me.
|
Clarification to my approval: the previously missing test was |
|
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.
|
|
The reported baseline/change GPU matrix addresses the original evidence gap: on both listed GPUs, |
|
Linux run of this head
AI-assisted: yes (Claude Code) |
bf4b9e1 to
1a12086
Compare
|
Rebased onto current I used the same settings as before on both GPUs:
On |
|
@Michael-KD one test change before this can merge: please drop the new |
|
@oneandonlydean Addressed in |
jotitadev
left a comment
jotitadev
left a comment
There was a problem hiding this comment.
NVIDIA run on Linux for head 3773b4960.
Linux run: Ubuntu 26.04, GCC 15.2, Release. RTX 3070 (NVIDIA 595.99.02) and lavapipe (Mesa 26.0.3), one at a time via VK_DRIVER_FILES. Khronos validation layer 1.4.341 with ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1; VK_LOADER_DEBUG=layer shows the layer inserted, no "was not found but was requested", 0 VUID- lines.
Base main 808a557ac; the merge is clean.
- Red:
main+ onlyLinearTexturePitch.cpp:agc_driver_linear_texture_pitchfails on both devices withguest 2D linear texture descriptor sets a custom pitch other than its default pitch, which is not implemented (format 20, 16x8x16 ...). - Green:
agc_driver_linear_texture_pitch,agc_driver_graphicsandagc_driver_linear_target_*(14 tests) pass 3/3 on each device. On lavapipelinear_target_direct_write_completionskips, as onmain. 0VUID-. ^agcsuite (no flip/display/lds64,-j1): 3070 1053/1053, lavapipe 1052/1053. The one failure isagc_driver_multisample_retirement, which also fails onmain.- Builds on Linux/GCC;
public_headerspasses.
GPUs tested: NVIDIA RTX 3070 (Linux), lavapipe.
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
✅ 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 (5, not counted against this PR)Validation IDs already present on main (4 IDs in 407 tests)Skipped (39)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. |
…does A linear 2D T# whose DEPTH field named a row pitch other than its default threw. On RDNA2 (gfx1036, hardware oracle, 135 image_load kernels over 8, 16, 32 and 64-bit texels) the row pitch is DEPTH + 1 elements, raised to the width and rounded up to 256 bytes. A single-level uncompressed linear 2D texture now records that pitch and lays its rows out with it; the texture cache, storage-to-sampled copies and surface shape checks keep textures of different pitches apart. Block-compressed, mipmapped and base-array surfaces with a pitch still throw. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…uests The DEPTH row pitch was measured on RDNA2 only. A PS4 (GCN) guest's linear 2D T# keeps the rule main had: only the default pitch decodes, any other pitch throws. Its layout comes from its own descriptor fields, so the RDNA pitch does not apply to it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3773b49 to
2baa6d8
Compare
✅ NVIDIA GPU test: passedPR head
Unstable on main (3, 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)Validation IDs already present on main (5 IDs in 484 tests)Skipped (45)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. |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — new head, looks good
The rebased head carries the measured RDNA2 linear 2D row pitch through decoding, surface sizing, and texture compatibility checks, and the reported exact-head GPU tests pass.
What I ran
Reviewed PR head 2baa6d8d407e8b79192f3e2a272cfe17c12ca9d3 against base bb04a4f1ea0175e9e4f149f0359a764d62ff3296; refreshed current main is f46f453a56f91d77e29ce0cbd7fe5bfb2fb69834. The merge preview with current main is clean (tree a434269dc23e81758c5b2eca46693f30da0e3ba0). Environment: Linux 6.1.0-54-amd64 x86_64; no local compiler, build, or tests run.
git diff --check upstream/main...refs/review/pr-11280-current
python3 tools/check_conventions.py --base upstream/main
git merge-tree --write-tree upstream/main refs/review/pr-11280-current
gh pr checks 11280 --repo boykopovar/AnyPS5Results
| Check | Result |
|---|---|
| Diff whitespace check | Pass; exit 0, no output. |
| Repository conventions | Pass. |
Merge preview with current main |
Clean. |
| Hosted Windows job | Pass on this exact head. |
| Other hosted jobs | Conventions, eligibility, labels, and report pass; Linux and specialized jobs skipped. |
| Focused GPU tests | The author reports baseline/current-head tests on an NVIDIA RTX 5070 Ti and an AMD Radeon iGPU, including the new row-pitch test, graphics and GCN-detile tests with validation enabled and no new VUIDs. These are author-run results, not local runs. |
Unedited output from local and hosted checks
$ git diff --check upstream/main...refs/review/pr-11280-current
(exit 0; no output)
$ python3 tools/check_conventions.py --base upstream/main
All conventions checks passed.
$ git merge-tree --write-tree upstream/main refs/review/pr-11280-current
a434269dc23e81758c5b2eca46693f30da0e3ba0
$ gh pr checks 11280 --repo boykopovar/AnyPS5
conventions pass 17s https://github.com/boykopovar/AnyPS5/actions/runs/40928209377/job/123639355169
labels pass 9s https://github.com/boykopovar/AnyPS5/actions/runs/40928209684/job/123639334925
windows pass 13m9s https://github.com/boykopovar/AnyPS5/actions/runs/40928172880/job/123639266398
linux skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/40928172880/job/123639268895
register-context skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/40928172880/job/123639269091
relinker skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/40928172880/job/123639269270
relinker-asan skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/40928172880/job/123639269526
write-watch skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/40928172880/job/123639269795
eligibility pass 5s https://github.com/boykopovar/AnyPS5/actions/runs/40928172880/job/123639199466
report pass 49s https://github.com/boykopovar/AnyPS5/actions/runs/40928272820/job/123639197837
The hosted results above are the recorded output of gh pr checks; the hardware runs are author-reported in the PR description.
Negative control
| Assertion | Change to code under test | Did the new test fail? |
|---|---|---|
A non-default DEPTH pitch is decoded and used for row addressing |
The author reports running the new test against base bb04a4f, before this change. |
Yes; it failed at the first case with the expected unsupported-custom-pitch error. |
Findings
Blocking
None found in the reviewed head. The custom pitch is accepted only for uncompressed, single-level linear 2D surfaces without a base array; it is rounded using the element size and propagated into layout and compatibility checks. PS4 GCN descriptors retain their separate pitch handling.
Non-blocking
The measured hardware rule is reported for RDNA2; the PR leaves PS5 gfx1013 and unsupported compressed, mipmapped, and base-array cases documented as unverified or unsupported.
Sources
- Pitch decoding and validation at this head
- Linear row layout at this head
- Repository hardware-oracle procedure at this head
Not verified
I did not build or run tests locally, independently reproduce the two GPU reports, or verify the behavior on PS5 hardware. Linux and specialized hosted jobs were skipped.
Correction — hosted-check job linksTwo job URLs in my October 29 review transcript were transcribed incorrectly; the exact What I rangh pr checks 11280 --repo boykopovar/AnyPS5ResultsThe recorded output is reproduced exactly: Unedited hosted-check output |
There was a problem hiding this comment.
Withdrawn. The validation comparison this review asked for is already in the PR's Tested section: main vs this head on both GPUs with ANYPS5_REQUIRE_VULKAN=1, ANYPS5_NO_SHADER_CACHE=1, VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation, no VUID-. The PR also has no title to name; Unblocks: none is correct.
|
@CnRJay Thanks for looking at this. Title: I have no title ID or log line to add. I have no PS5 or dumps, and this PR doesn't come from a title. It implements a field that Validation comparison: this is already in Tested. It has the same focused tests on Separately, the NVIDIA bot's run of this head above found no new failures and no new validation IDs against |
What
A linear 2D T# whose
DEPTHfield (word 4) named a row pitch other than its default threwguest 2D linear texture descriptor sets a custom pitch other than its default pitch, which is not implemented. RDNA2 hardware uses that field as the row pitch, so such textures now decode and read with the measured layout instead of stopping the guest.The rule, measured with the hardware oracle: the row pitch is
DEPTH+ 1 elements, raised to the width and rounded up to 256 bytes. ADEPTHthat rounds to the default pitch, including any below the width, now decodes as the default pitch; before, onlyDEPTH= default pitch − 1 was accepted.GuestTextureResource: a newlinearPitchfield, in elements.DecodeTextureResourcesets it for a single-level, uncompressed linear 2D T# with no base array, and only when the pitch differs from the default, so every descriptor accepted before decodes as before. Block-compressed, mipmapped and base-array surfaces with aDEPTHpitch still throw (not measured).EncodeTextureResourcerefuses a resource with a custom pitch rather than dropping it.ComputeLinearMipLayoutandComputeMipLayouttake the pitch;DescribeSurfaceand the texture cache pass it, sopitchBytes,blocksPerRowand the guest size follow it.Texture::CanCopyFromandStorageTexture::SameSurfaceShapecompare the pitch, so a storage or sampled texture of one pitch does not serve another. The texture cache keys on the descriptor words, which holdDEPTH.main's rule: a linear 2D T# decodes only its default pitch and throws otherwise, since the pitch rule is measured on RDNA2 only.docs/dev/debt/libSceAgcDriver.md: a new "Linear 2D texture pitch" entry for what stays open (gfx1013; compressed, mipmapped and base-array surfaces; PS4 guests).Overlap:
tools/pr_overlap.pylists #9891, #9892, #9893 and #9895 (PS4 support; heads375f5630ba,545e2b5b3f,781ffafe9f,3e32049166). No order is needed and none is a prerequisite. Each conflicts with this PR in three adjacent additions that keep both sides: the#include <array>they add next toRdnaDecoder/RdnaInstructionDecoder.hppinGuestTextureResource.cpp, theirif (gcn) { result.gcnPitch ... }block besideresult.linearPitchthere, and theirRequire(!IsGcnTiled(tileMode), ...)line beside this PR'sComputeLinearMipLayout(..., 0u)call inComputeElementMipLayout. TheirDescribeSurfacereturns throughdescribeGcnSurfacefor a PS4 surface before the RDNA layout that readslinearPitch, and a PS4 guest's linear 2D T# keepsmain's default-pitch rule here (GcnGuestShaders()), so this PR changes nothing for PS4 guests either way. #9891's head merged into this branch with that resolution passesagc_driver_graphics,agc_driver_gcn_detileandagc_driver_linear_texture_pitchon both GPUs below with the validation layer and noVUID-lines. No PR it could not compare changes the custom-pitch check.Measurement:
image_load(DIM 2D, data mask 1) through a linear 2D T# (TYPE9, height 4, one level) over a buffer whose element at offsetoencodeso(for 8-bit texels, two runs with the low and the high byte of the offset). Each lane loads(x, y)for 16 x values and y 0 to 3, and the element it reads gives the row pitch the hardware used.yaty × pitch, with pitch =max(DEPTH + 1, width)rounded up to256 / bytes per element. For example, width 16 32_UINT:DEPTH0 to 63 give 64, 64 to 127 give 128, 128 gives 192, 1023 gives 1024. No lane read anything the rule doesn't explain.Tested
Windows 11 Pro 26200, WinLibs GCC 15.2.0 r7, Release.
agc_driver_linear_texture_pitch(new): 128 threadsimage_loada 16×8 32_UINT linear 2D texture, whose elementiholds0xa0000000 | i, withDEPTH15, 63, 64, 127, 128 and 255 in turn over the same memory, and check every texel against the measured pitch (64, 64, 128, 128, 192, 256). Onmainit fails at its first case,DEPTH15, a pitch below the width that the hardware treats as the default:guest 2D linear texture descriptor sets a custom pitch other than its default pitch, which is not implemented (format 20, 16x8x16, type 9, tile 0, mips 0..0 of 1). With this change it passes.agc_driver_graphics(GuestTextureResource.cppcases updated): the decoded pitch for 32-bit and 8-bit formats, rounding, a pitch below the width, the surface layout, encoding with the default pitch, and the mipmapped refusal.ctest --test-dir build -R '^agc_driver_(graphics|linear_texture_pitch|gcn_detile)$' -VwithANYPS5_REQUIRE_VULKAN=1,ANYPS5_NO_SHADER_CACHE=1,VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation(Vulkan SDK 1.4.363.0, insertion confirmed withVK_LOADER_DEBUG=layer) andANYPS5_GPUset to each device, on this PR's head2baa6d8d40and on its basemainbb04a4f1ea.agc_driver_gcn_detileis included because the rebase onto the PS4 (GCN) texture path in0bec339021touched the same decode and layout code; PS4 descriptors carry their pitch in word 7 and never setlinearPitch. Themainarm builds every file this PR changes frommain, includingtests/GuestTextureResource.cpp, soagc_driver_graphicsrunsmain's own test source; it keeps only the newLinearTexturePitch.cpp, so the new test runs on both sides:main, RTX 5070 Timain, Radeon iGPUagc_driver_linear_texture_pitchagc_driver_graphicsagc_driver_gcn_detileNo log contains
was not found but was requested by env var VK_INSTANCE_LAYERS, no log has aVUID-line, and each log names the selected GPU.Full
ctest -j8with this change, before the rebase ontobb04a4f1ea(onmain48c160c58b): 2394 of 2396 pass. The two failures,agc_host_import_cut_lifetime(#10746) andagc_driver_depth_plane_partial_store(#10664), also fail with the change reverted.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)🤖 Generated with Claude Code