Repository navigation
fix(agc): reject unmeasured single-sample dithered alpha-to-coverage - #10581
adriadam10 merged 1 commit into
Conversation
github-actions
Bot
added
unblocks title
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
The two-sample dither path is consistently carried through decode state, draw keys, request serialization, and the shader table; the focused graphics tests and Bugsnax report provide useful evidence. One checklist item needs correction before I can approve: Unblocks: PPSA01503 #9900 points to no issue or pull request in this repository (the GitHub API returned 404 for both endpoints when checked). Please replace it with the correct bring-up discussion or none. Hosted conventions, intake, and labels pass; the hosted product build jobs were not present in this PR check set. I did not run the graphics/GPU tests locally.
|
@MarcZX10 Thank you for the review!
|
|
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.
|
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
The checklist now links the correct Bugsnax bring-up discussion (#9900), which I verified exists and matches PPSA01503. The implementation review remains unchanged: the two-sample alpha-to-coverage path is carried consistently through decode state, draw keys, request serialization, and shader sample-mask selection. The author added a same-machine Vulkan validation comparison against main and reports 0 VUID errors on both runs. Hosted conventions, intake, and labels pass; this PR check set does not include hosted product builds. I did not run the GPU test locally.
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
The implementation review remains positive: the single-sample bypass is threaded through the draw state and serialization, and the PR reports a same-GPU validation comparison against its stated base. A merge-blocking branch issue has appeared since that review: GitHub currently reports this PR as CONFLICTING / DIRTY against main. Its head is still based on the older #10515 head f179568345cd…, while #10515 now points to bb2e64983cf9…. Please rebase this stack on the current #10515 head (or current main after #10515 merges), resolve the conflict, and rerun the focused graphics validation comparison on the resulting head. The existing validation results are for base fa515def85 and the prior head; they do not establish results for a rebased tree. Product CI jobs are absent from the current check set.
|
Fresh follow-up to the existing rebase request: I fetched the current #10515 head (bb2e649) and the current #10581 head (5cfbcec). A |
|
I checked the one-sample premise before integrating this into my #10515 testing. The Vulkan claim in the description needs correction: Multisample Coverage describes alpha modifying the coverage mask when enabled; it does not provide the claimed one-sample exception. I measured this with an independent native Vulkan test on RX 6650 XT / RADV Mesa26.2.4 / Linux7.2.9. It uses a one-sample RGBA8 attachment and a fullscreen triangle with a constant fragment alpha, no blending, no shader discard/SampleMask output, and no AnyPS5 shader emulation. The only pipeline difference is
All three independent runs match, with actual Khronos validation insertion, zero validation errors and zero warnings. Source, shaders, reproduction commands, complete pixel readback and logs (9,687 bytes; SHA256 AMD PAL's register setup also configures The game progress is useful, but please revise the rationale and supply the missing register-level/hardware evidence before relying on the unconditional single-sample bypass. The Vulkan single-sample behavior above cannot justify disabling guest alpha coverage. I have not tested your title path or changed your branch; this is evidence for reviewing the proposed semantics. |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 5cfbcec53b70aa7b6d27cfcc21c40cc33d3e27f2 with the new native Vulkan result posted in the discussion. That test uses a one-sample attachment and changes only alphaToCoverageEnable; on RX 6650 XT/RADV, alpha-to-coverage changes the written-pixel count for alpha 0.25, 0.5 and 0.75. The current description says Vulkan Section 28.8.2 strictly disables this operation at one sample, but the cited rule does not state that, and the measured result contradicts it.
The game reproduction and register capture establish that this title reaches the state and that the current rejection blocks it; they do not yet establish what the PS5 does with stale DB_ALPHA_TO_MASK when MSAA_ENABLE is clear. Please remove the unsupported Vulkan-spec claim and provide PS5/hardware-oracle evidence for the disabled behavior, or keep the alpha-to-coverage state modeled and address the title path using measured console semantics. The current branch also remains DIRTY against its #10515 base, so the focused validation needs to be rerun after the required rebase. I did not run GPU tests locally.
5cfbcec to
31871f7
Compare
ashwin280802
changed the title
|
Rebased cleanly onto the current #10515 head ( 1. Hardware-Accurate Coverage Modeling & Rationale Correction
2. Technical Debt Documentation
3. Focused Vulkan Validation Layer Comparison
4. Physical Title Verification (Bugsnax PPSA01503)
5. Conventions & Cleanliness
|
31871f7 to
43ddcb6
Compare
adriadam10
left a comment
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the rebase and for dropping the Vulkan one-sample claim. One blocker remains:
State.cpp:1530,State.cpp:2388-2404:0xe701/0x18701on a one-sample target now decodes as the undithered model, and the dither-specific checks (sample count, locations, FP16/32 export) are skipped. The new debt entry indocs/dev/debt/libSceAgcDriver.mdsays the hardware does dither one-sample coverage across 2x2 quads, so the draw is accepted with a coverage that differs from the console's for every alpha strictly between 0 and 1. That is a plausible-but-wrong result wheremainthrew. Either measure the one-sample dithered mask (a RADV default-dither render on a one-sample target gives the per-quad pattern, as was done for the eight- and two-sample tables) and model it, or keep throwing for this case.tests/Graphics.cpp:4992-5005: the new test only checks the decoded flags and thatDrawRejectionis empty. Nothing pins the coverage a one-sample0xe701draw produces, so it cannot catch the difference above. Add an execution case in the style ofAlphaToCoverage.cppwith alphas such as 0.25, 0.5, 0.75 per quad position, with expected values from the measurement.- The Bugsnax result is from Windows/RTX 5070 only and the PR has no Build run on this head; please make sure the full Build is green once the above is settled.
|
Thanks for the specific test suggestion. I checked the earlier measurement comment and it appears to already contain the requested native one-sample RADV experiment: comment #6390289949, with the source, shaders, reproduction commands, full pixel readback, and three-run logs. It used an RX 6650 XT / RADV, a one-sample RGBA8 target, alpha values 0, 0.25, 0.5, 0.75, and 1, and recorded the complete 4×4 readback; all three runs matched. For example, alpha 0.25 wrote the upper-left 2×2 pixels, alpha 0.5 wrote the upper-left and lower-right 2×2 blocks, and alpha 0.75 wrote all but the lower-left 2×2 block. I understand that this is native Vulkan alpha-to-coverage evidence, not proof that the PS5’s Can you confirm that the linked RX 6650 XT measurement is the RADV evidence you intended, and that the remaining required work is the in-tree execution regression? I’ll keep the PR’s PS5-equivalence caveat explicit. |
|
Testing this on Alan Wake Remastered (PPSA01924), Linux, RTX 4080 SUPER, GCC 16.2.1, Release. The dump has no
It looks like the single-sample path also needs the UNORM8 MRT0 export (format 1) and the |
|
Follow-up with instrumentation on the next draw:
I have the format-1 change locally ( |
43ddcb6 to
244d287
Compare
ashwin280802
changed the title
|
Updated the branch with the requested single-sample dithered alpha-to-coverage model and in-tree execution regression:
@edubox: Thank you for identifying the follow-up format 1 (UNORM8) and 4-sample |
adriadam10
left a comment
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the table and the execution test. The model still doesn't match the measurement it is based on:
SpirvModuleEmitter.cpp:510-512,tests/execution/AlphaToCoverage.cpp:527-540:oneSampleMasksandOneSampleDitherCasesgive one covered pixel per 2x2 quad at alpha 0.25 ({1,0,0,0}) and a diagonal pair at 0.5. The RADV readback in your tarball (run-1.jsonl, 4x4 target) shows something else: alpha 0.25 writes only the upper-left 2x2 block, 0.5 writes the upper-left and lower-right blocks, 0.75 all but the lower-left. That is a quad-granular pattern (all four pixels of a quad share the result), not a per-pixel Bayer pattern inside each quad. As written, the draw is accepted with coverage that differs from the measurement for every alpha strictly between 0 and 1.- A 4x4 target can't show the period of that pattern either. Re-measure on a larger one-sample target (e.g. 16x16 or 32x32, alphas in steps of 1/8 and the FP16 export too) and derive the table and the test rows from that readback.
- The comment in the thread and the debt entry say the table is "measured on gfx1032"; the only one-sample measurement is Vulkan
alphaToCoverageEnableon RADV (RX 6650 XT). Please say that in the debt entry and the PR, and keep the caveat that it is notDB_ALPHA_TO_MASK0xe701 / 0x18701 withMSAA_ENABLEclear on PS5. docs/dev/debt/libSceAgcDriver.md:47: also states the "8-level per-quad Bayer" model; update it with whatever the larger readback shows.- The branch conflicts with
main; please rebase and make sure the full Build is green on the new head (the Bugsnax run was Windows/RTX 5070 only).
The four-sample 0x1e701 draw @edubox reports is a separate follow-up, agreed.
244d287 to
05082c0
Compare
|
Thanks for catching the quad-granular pattern in Here is the current status:
@JJenkx: Since you have the Arch Linux / RX 6650 XT RADV test bench that produced the measurements for #10515, would you be able to run a 16x16 or 32x32 target readback (with 1/8 alpha increments across FP16 and FP32 exports) to help us establish the spatial period for the single-sample pattern? Alternatively, I'm happy to push the rebased branch so anyone with AMD RDNA hardware can run the fixture. |
adriadam10
left a comment
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the rebase and the debt entry wording. The main point is still open:
SpirvModuleEmitter.cpp:521-523andtests/execution/AlphaToCoverage.cpp:585-598:oneSampleMasks/OneSampleDitherCasesare unchanged, so alpha 0.25 still covers one pixel per 2x2 quad ({1,0,0,0}) and 0.5 a diagonal pair. The RADV readback inrun-1.jsonlis quad-granular (0.25 writes only the upper-left 2x2 block, 0.5 the upper-left and lower-right blocks), so the draw is accepted with coverage that differs from the measurement for every alpha strictly between 0 and 1, and the test pins that same model.- The table and test rows need to come from a larger one-sample readback (16x16 or 32x32, alpha in steps of 1/8, FP16 and 32-bit exports); a 4x4 target can't show the period. Until someone with RADV runs it, the one-sample dithered path should keep throwing instead of using a table that contradicts the only data we have. Either land the measurement-derived table here, or drop the table and keep the rejection, with the debt entry describing what is missing.
- Once the table follows the readback, update
docs/dev/debt/libSceAgcDriver.md:48(which still describes the 0xe701/0x18701 paths without the one-sample case) and the new entry at:49with the period found. - The full Build on the new head needs to be green (CI is still pending).
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewing exact head 05082c049563be230b150b6a714824d55ed5da0b.
What I ran
- Inspected the state decode, recompiler table, execution regression, technical-debt changes, review discussion, and current hosted checks.
- Hosted Windows, conventions, labels, eligibility, and report checks pass. Linux and specialized product jobs are skipped.
- I did not run a local build or GPU test. The PR's test and validation results are author-reported.
Results
| Check | Result | Evidence |
|---|---|---|
| Single-sample dithered coverage implementation | Reviewed | Exact PR diff and posted readback discussion |
| Focused execution tests | Pass, author-reported | The new 4x4 regression and neighboring alpha-to-coverage tests pass |
| Vulkan validation comparison | Pass, author-reported | Main and PR logs report no missing-layer warning or VUID |
| Hosted Windows | Pass | GitHub Actions |
| Hosted Linux/product and specialized jobs | Skipped | GitHub Actions |
| Pattern equivalence to the posted readback | Not established | Existing 4x4 expected table conflicts with the reported spatial pattern |
Findings
Blocking
-
The current
oneSampleMaskstable andOneSampleDitherCasestest encode one covered pixel per 2x2 quad at alpha 0.25 and a diagonal pair at 0.5. The posted RADV readback describes quad-granular coverage instead: alpha 0.25 writes the upper-left 2x2 block, and 0.5 writes the upper-left and lower-right blocks. The current table therefore conflicts with the available measurement, and the 4x4 test only confirms the implementation's own assumed pattern. The separate Vulkan validation run checks API validity, not this rendering behavior.Please either derive the pattern from a larger 16x16 or 32x32 readback (with 1/8 alpha steps and both FP16 and 32-bit exports) and update the table and tests, or keep the single-sample dithered path rejected until the behavior is measured. The current README/debt entry should continue to distinguish modeled behavior from PS5 silicon behavior.
Non-blocking
- The new state path and serializer use the one-sample mode consistently, and the regression covers both FP16 and FP32 exports. Those checks do not resolve the pattern mismatch above.
- The separate format-1 and four-sample findings were explicitly scoped outside this change; I am not asking to add those unrelated paths here.
Sources
Not verified
- I did not run local CTest, a GPU readback, or a PS5 console comparison.
- The current test and validation results are author-reported.
To get this to a good state
Post a larger measurement-derived table and matching regression, or remove the unsupported single-sample table and retain the explicit rejection; then request a re-review.
05082c0 to
4bb5aa9
Compare
ashwin280802
changed the title
|
@adriadam10 @MarcZX10 Following your reviews, I have updated the branch to adopt Option B and rebased onto latest
|
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — new head, looks good
The current diff adds regression coverage for the existing rejection of unmeasured single-sample dithered alpha-to-coverage and documents the remaining measurement debt.
What I ran
Reviewed exact head 4bb5aa9f0e39858a6295c31cef199f8f164e25ea against base bb04a4f1ea0175e9e4f149f0359a764d62ff3296 (main). GitHub reports the PR mergeable with a clean merge state; I did not create a local merge preview. Environment: Debian 12, GCC 12.2.0, host Intel Xeon E5-1620 v2; the focused test binary ran under QEMU user mode with a Haswell CPU model.
cmake --build /dev/shm/AnyPS5-build-12915-full --target agc_driver_graphics_tests -j2
ANYPS5_NO_SHADER_CACHE=1 qemu-x86_64 -cpu Haswell /dev/shm/AnyPS5-build-12915-full/tests/agc_driver_graphics_tests --alpha-to-coverage-state
ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation VK_ICD_FILENAMES=/usr/share/vulkan/icd.d/lvp_icd.x86_64.json ctest --test-dir /dev/shm/AnyPS5-build-12915-full -R '^agc_driver_graphics$' -V
ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation VK_ICD_FILENAMES=/usr/share/vulkan/icd.d/lvp_icd.x86_64.json qemu-x86_64 -cpu Haswell /dev/shm/AnyPS5-build-12915-full/tests/agc_driver_graphics_tests
git diff --check bb04a4f1ea0175e9e4f149f0359a764d62ff3296 HEADI ran the focused state command and build on both the PR head and base. The Vulkan CTest command above was run on the PR head; the full binary was also attempted under QEMU on both revisions. GitHub checks were read for the exact PR head, and the Windows validation result below is author-reported.
Results
| Check | Result |
|---|---|
agc_driver_graphics_tests build |
Pass on base and PR head |
| Focused alpha-to-coverage state/refusal tests | Pass on base and PR head under QEMU Haswell |
git diff --check |
Pass |
| Full local CTest on PR head | Aborted before the test due to host CPU features missing LZCNT, BMI1, BMI2, MOVBE, AVX2 and FMA |
| Full graphics binary under QEMU, base and PR | Did not complete: QEMU user-mode memory access failed before the full suite could finish; this is an environment limitation, not evidence of a PR regression |
| Hosted exact-head checks | Windows, conventions, labels, eligibility and report pass; Linux and specialized jobs skipped |
| Vulkan validation on RTX 5070 / Windows 11 | Pass, author-reported: validation layer loaded and no new VUID- errors |
Unedited command output
Focused alpha-to-coverage command, PR head:
[src/HostCpu.cpp:193 operator()] host CPU Intel Core Processor (Haswell): has SSE, SSE2, SSE3, SSSE3, SSE4.1, SSE4.2, POPCNT, LZCNT, BMI1, BMI2, MOVBE, CMPXCHG16B, LAHF/SAHF, AVX, AVX2, FMA, F16C, AES, PCLMULQDQ, RDRAND, XSAVEOPT, OS AVX state (XCR0); lacks RDSEED, ADX, SHA, CLFLUSHOPT, CLWB, RDPID, XSAVEC, SSE4A, PREFETCHW, CLZERO
Alpha-to-coverage state and refusal tests passed
Full CTest command, PR head:
961: [src/HostCpu.cpp:193 operator()] host CPU Intel(R) Xeon(R) CPU E5-1620 v2 @ 3.70GHz: has SSE, SSE2, SSE3, SSSE3, SSE4.1, SSE4.2, POPCNT, CMPXCHG16B, LAHF/SAHF, AVX, F16C, AES, PCLMULQDQ, RDRAND, XSAVEOPT, OS AVX state (XCR0); lacks LZCNT, BMI1, BMI2, MOVBE, AVX2, FMA, RDSEED, ADX, SHA, CLFLUSHOPT, CLWB, RDPID, XSAVEC, SSE4A, PREFETCHW, CLZERO
961: [libc] unhandled exception 'St13runtime_error' thrown from 0x7fc9fdba5e87
961: terminate called after throwing an instance of 'std::runtime_error'
961: what(): The host CPU (Intel(R) Xeon(R) CPU E5-1620 v2 @ 3.70GHz) lacks LZCNT, BMI1, BMI2, MOVBE, AVX2, FMA. The PS5's Zen 2 CPU has these extensions and titles run their instructions natively, so this CPU cannot run them.
1/1 Test #961: agc_driver_graphics ..............Subprocess aborted***Exception: 0.01 sec
0% tests passed, 1 tests failed out of 1
Full graphics QEMU run, PR head:
Unused stencil reference sample equivalence passed: 22 accepted states, 12 dependent countermodels
Stencil logic operations matched AND, OR, XOR and XNOR under the write mask for 310 register states
All 256 MSAA CMASK byte classifications passed
AGC graphics: direct registered range must be covered by host import
Full graphics QEMU run, base:
Unused stencil reference sample equivalence passed: 22 accepted states, 12 dependent countermodels
Stencil logic operations matched AND, OR, XOR and XNOR under the write mask for 310 register states
AGC driver: guest memory is not readable at 0x7ff3a6ad0000 (range 0x7ff3a6ad0000+0x10000)
Hosted check output:
conventions\tpass\t18s\thttps://github.com/boykopovar/AnyPS5/actions/runs/40900013426/job/123539407094
labels\tpass\t8s\thttps://github.com/boykopovar/AnyPS5/actions/runs/40900013718/job/123541715960
windows\tpass\t4m35s\thttps://github.com/boykopovar/AnyPS5/actions/runs/40898998428/job/123541863430
linux\tskipping\t0\thttps://github.com/boykopovar/AnyPS5/actions/runs/40898998428/job/123541866210
register-context\tskipping\t0\thttps://github.com/boykopovar/AnyPS5/actions/runs/40898998428/job/123541866449
relinker\tskipping\t0\thttps://github.com/boykopovar/AnyPS5/actions/runs/40898998428/job/123541866549
relinker-asan\tskipping\t0\thttps://github.com/boykopovar/AnyPS5/actions/runs/40898998428/job/123541866202
write-watch\tskipping\t0\thttps://github.com/boykopovar/AnyPS5/actions/runs/40898998428/job/123541866177
eligibility\tpass\t8s\thttps://github.com/boykopovar/AnyPS5/actions/runs/40898998428/job/123536183958
report\tpass\t49s\thttps://github.com/boykopovar/AnyPS5/actions/runs/40898998394/job/123536185625
Negative control
| Assertion | Mutation to code under test | Did the new test fail? |
|---|---|---|
0xe701 and 0x18701 on a single-sample target are rejected by both DecodeState and DrawRejection |
No acceptance mutation was applied | Not exercised; the rejection tests pass on the unmodified implementation |
Findings
Blocking
None found in the current diff.
Non-blocking
The exact-head Windows Vulkan result is author-reported, and the local full graphics run could not complete under this host/QEMU setup. The PR keeps the unmeasured hardware behavior explicitly unsupported and recorded as debt.
Sources
- Rejection checks in
State.cpp, pinned to this head - Added regression cases, pinned to this head
- Updated technical-debt record, pinned to this head
- Author's current-head Windows Vulkan validation report
Not verified
- Physical PS5 alpha-to-coverage behavior; the PR intentionally leaves it unsupported pending measurement.
- A complete local Vulkan CTest pass on either base or PR, or a local same-GPU
VUID-comparison. The host CPU and QEMU user-mode memory limitations prevented that. The current-head Windows run is author-reported.
To get this to a good state
No blocking follow-up is needed for this change. Keep the debt entry until the spatial repetition and console behavior are measured; if the head changes, I will rerun the focused tests and relevant validation checks on the new head.
|
Static review plus an independent Linux/AMD run (CachyOS, kernel 7.2.10, GCC 16.2.1, Ninja 1.13.2, Release, What I checked The diff is test-only (
Focused test run (per repo AGENTS GC-change protocol)
Both logs: 0 occurrences of The validation layer binary is not part of the base image here; I unpacked Arch's Not covered The debt note's Vulkan |
|
Checked that both rejection strings the new cases assert exist unchanged in |
What
Reject unmeasured single-sample dithered alpha-to-coverage in the AGC Driver (
DecodeStateandDrawRejection) forDB_ALPHA_TO_MASK0xe701 and 0x18701, verify rejection in tests, and document technical debt.DecodeStateandDrawRejection(requires eight matched raster/DB_EQAA samplesfor0x18701andrequires two matched raster/DB_EQAA samplesfor0xe701), rejecting single-sample targets until the spatial repetition period and dither behavior are measured on PS5 silicon.core/libs/prx/libSceAgcDriver/tests/Graphics.cpp, add tests verifying that single-sample draws with0xe701and0x18701are explicitly rejected by bothDecodeStateandDrawRejection.docs/dev/debt/libSceAgcDriver.mdexplaining why single-sample dithered coverage remains unsupported/rejected and recording the preliminary quad-granular RADV observations.Tested
ctest --test-dir build-full -R "^agc_driver_graphics$" -Vpassed 1/1 (100%).agc_driver_graphicstest on Windows 11 with NVIDIA GeForce RTX 5070 withANYPS5_REQUIRE_VULKAN=1,ANYPS5_NO_SHADER_CACHE=1, andVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation.VK_LAYER_KHRONOS_validationloaded cleanly (no missing-layer warning); verified 0VUID-validation errors introduced.python tools/check_conventions.py --base origin/mainpassed cleanly with 0 violations.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)