Repository navigation
Conversation
github-actions
Bot
added
the
unblocks title
55c2906 to
1a57150
Compare
|
Head |
|
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 exact-head C++20 syntax check found a blocking compile error in the new execution test. UintColorTarget.cpp opens an anonymous namespace at line 35, but it never closes it before defining main(); the file ends at line 281 still inside that namespace. Compiling the exact PR file with the repository headers fails with expected '}' at end of input (matching the namespace opened at line 35). Please close the helper namespace before main(), then rerun the focused target/build.
The hosted eligibility, conventions, labels and report checks pass, but the product build/test jobs are skipped, so they do not catch this compile failure. I ran only a syntax-only compile locally; no runtime test ran here.
After the fix, please consider adding a 32-bit raw export into an R8_UINT target to the execution test as well. The issue's ccleste case uses R8_UINT; the current execution test covers tiled and linear R32_UINT, while the 8-bit path is only covered by state-decode assertions and a title run that proceeds to a later failure.
1a57150 to
81d6718
Compare
|
Head |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
The state decode and output specialization changes are consistent with the intended raw UINT path, and the new decode checks cover unsupported packed exports plus blend suppression. Current-main check_conventions.py and git diff --check pass. I did not build/run the Vulkan execution test locally. The fork verification workflow for the fix/agc-uint-color-export ref is still in progress at the time of this review; I have not counted its results as passing. The upstream PR checks themselves show eligibility/conventions/labels, while full product builds are skipped.
Two follow-ups before merge:
- This head is eight commits behind live
main(8499ea52e571b0edfc1feecfd3f6b9944012b359). CONTRIBUTING.md requires branches to be based on and kept up to date withmain; please sync and rerun the checks on the final head. - The execution test bit-checks
32_Routput intoR32_UINTin tiled and linear layouts. The implementation also accepts32_GR,32_ARand32_ABGR, while the decode unit tests currently check R32 and R8. Please add focused multi-channel execution coverage for those accepted export formats, or narrow the supported-format claim to the formats covered by the evidence.
The issue #9783 is open and correctly linked with Fixes #9783; I found no duplicate fix in its comments.
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
The latest head 81d67188b66510d2465ef10ba8dd15743e90b586 fixes the previously reported test compile issues according to the author’s follow-up and macOS workflow link. Current-main conventions and git diff --check pass, and the merge tree is clean. The PR reports valuable MoltenVK/title validation and the upstream hosted product jobs are skipped. This VM has no Vulkan device for the required paired validation-layer run, so those results remain contributor-reported. The branch is still 21 commits behind current main (11f976b27c16ed91633f00039dba5dfd97dd7cc1); please rebase and rerun the execution test and main/change validation comparison on the current base before merge.
|
The hosted Linux run on |
|
I reviewed the changed decode, specialization, serialization, and execution-test paths. The One coordination point before merge: the PR currently targets base |
adriadam10
left a comment
adriadam10
left a comment
There was a problem hiding this comment.
The new execution test fails on your own hosted run, and the cause is in the shader, which this PR doesn't touch:
- Your verify run 39811668668 on
81d67188b6is green only because its ctest step ends in|| true. Itsctest.logartifact hasagc_driver_uint_color_targetfailing:a 32_R export of 0x00000001 into a tiled 32-bit UINT target: pixel (0, 0) inside the scissor holds 0x00000000, expected 0x00000001. The same log also hasagc_driver_graphicsfailing (alternate color blend constants were not swizzled with the export), along with three other agc tests. Please check whether those also fail on the base you ran on. - Cause:
SelectExportPacking(core/shader/recompiler/SpirvBackend/src/SpirvModuleEmitter.cpp:427-441) treats any packing other thanNoneas a packed word. WithUintit takes theFormat11_11_10UNormpath, writesPackExport(...)into component 0 and zeroes the rest. The specialization then bitcasts that word touint4, so a UINT target gets the 10_11_11 UNORM packing of the float, not its raw bits. Word 0 survives that and word 1 (a denormal) becomes 0, which matches the failure above. So the Doom 3 BFG pipeline compiles on MoltenVK, but the target doesn't get the values the console's CB stores. - Suggested fix: in
SelectExportPacking, setpackedonly forUnorm10_11_11andFloat11_11_10, so thatUintpasses the color through and the existing retype does the bitcast. Then make sureagc_driver_uint_color_targetpasses on a run whose ctest failures fail the job. Please also update the "Tested" section, which currently says the hosted run passes.
ca9a64a to
cdfc5fe
Compare
|
@adriadam10 you're right on both counts, and thanks for reading the log. My fork workflow ends its ctest step in Head |
cdfc5fe to
2297b5a
Compare
adriadam10
left a comment
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the fix: SelectExportPacking now passes the color through for Uint, and agc_driver_uint_color_target passes on your run of cdfc5fe689. Two things remain:
core/libs/prx/libSceAgcDriver/Graphics/src/State.cpp:2025: the CB resolve guard issource.packing != ColorExportPacking::None. Every UINT target now haspacking == Uint, so a resolve of a UINT target throws "CB resolves of unsigned integer color targets are unsupported", where on main it was accepted (the format/extent checks above are the only ones it had). UseWordPacked(source.packing)there, as you did for the DCC check at line 1856, and add a case toGraphics.cppthat decodes a resolve with a UINT source and expects it to pass.- Your run 39846282162 on
cdfc5fe689still failsagc_driver_graphics(ctest.loghas it failing after 0.10 s, with no message). Your earlier comment lists it among the failures from81d67188b6, but not among those that also fail onmain. Please show the failing assertion and whethermainfails it too on the same job; if it is the PR (e.g. thestatic_cast<ColorExportPacking>(4)change infloat11_11_10TargetTestsor the newuintTargetTests), it needs fixing.
Also please correct the "Tested" section so it states which tests pass and which fail on a run whose ctest failures fail the job.
2297b5a to
d7b372d
Compare
adriadam10
left a comment
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the update. The first point from the last review is still open:
core/libs/prx/libSceAgcDriver/Graphics/src/State.cpp:2027: the CB resolve guard is stillsource.packing != ColorExportPacking::None.DecodeColorFormat(line 1748) now gives every UINT targetpacking == Uint, so a resolve of a UINT target throws "CB resolves of unsigned integer color targets are unsupported", wheremainaccepted it (the format/extent checks above were its only ones). UseWordPacked(source.packing)there, as at line 1858, and add a case next to the resolve checks inGraphics.cpp(around line 3790; the loop there only covers the 10_11_11 and 11_11_10 targets) that decodes a resolve with a UINT source and expects it to pass.
The change to uintTargetTests (zeroing context[0x105..0x108]) is not explained: if it fixes the agc_driver_graphics failure from your fork run, please post the failing assertion it addressed and a run on this head whose ctest failures fail the job, so the "Tested" section can say which tests pass and which fail (and whether main fails them too).
d7b372d to
fced1e1
Compare
adriadam10
left a comment
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the update. The head only adds words.context[0x1e0] = 0 to the test; the main point is still open:
core/libs/prx/libSceAgcDriver/Graphics/src/State.cpp:2027: the CB resolve guard is stillsource.packing != ColorExportPacking::None. Every UINT target now haspacking == Uint, so resolving a UINT target throws "CB resolves of unsigned integer color targets are unsupported", wheremainaccepted it. UseWordPacked(source.packing)there, as at line 1858, and add a case next to the resolve checks inGraphics.cpp(around line 3790, where the loop only covers 10_11_11 and 11_11_10) that decodes a resolve with a UINT source and expects it to pass.uintTargetTests: please say which assertion thecontext[0x1e0] = 0line fixes, and show a run on this head so the "Tested" section lists what passes and what fails (and whethermainfails the same tests).
|
I checked the completed fork verification run |
|
I rechecked the current head The requested exact-head test evidence also remains incomplete: the last downloaded fork run failed four tests, including |
1 similar comment
|
I rechecked the current head The requested exact-head test evidence also remains incomplete: the last downloaded fork run failed four tests, including |
|
@adriadam10 @MarcZX10 head What changed since
The graphics decode test also passes in both modes in a Linux container (Ubuntu 24.04, GCC 13.3) with this diff on |
fced1e1 to
f9b349a
Compare
|
@adriadam10 @MarcZX10 the resolve guard is fixed in head On the test evidence: the GitHub has disabled Actions on my fork since then, so the current head is covered by a container run instead: Ubuntu 24.04, GCC 13.3, Release, llvmpipe, this head's diff on |
|
Independent Windows/NVIDIA validation of head Host: Windows 11 x86-64 26300.9711, Core i7-13650HX, NVIDIA GeForce RTX 4060 Laptop GPU, driver 610.62, WinLibs GCC 15.2.0 r7, Release, Focused testsBuilt actual
ccleste A/BRelease 1.00, PPSA59322, content version 01.000.000. ZIP SHA-256 verified against the published asset digest: The executable, guest module and all other system libraries are hash-identical between the two runtimes: only Both use the settings above plus
All four reach the first level; captured display readbacks show the player in different positions after the scripted movement/jump inputs. Options hold exits normally before the 40 s cap. Main renders on this NVIDIA driver but emits: The PR removes those warnings in both runs, with no Validation Warning/Error or VUID messages. No fatal or driver failure in any of the four runs. This is a Vulkan-correctness/portability result, not a newly playable title or performance improvement: main already reaches this first level on NVIDIA. Two earlier main runs are excluded from that four-run comparison: a 25 s no-input run, and a 35 s run whose timeline was incorrectly queued before startup and therefore ignored as stale. Both were harness-terminated, not natural exits. The harness was corrected before the table's runs; no runtime code was changed for input. Audible sound, physical controller, motion/rumble, longer gameplay and console semantics were not tested. Audio uses the dummy backend. Logs/readbacks are retained locally; this report is measured data plus frame inspection, not a claim to have reproduced the MoltenVK failure on NVIDIA. AI-assisted execution, log/frame inspection and report: yes. |
f9b349a to
05a8481
Compare
|
@valenciana-nc thank you, that is the NVIDIA data this PR lacked and it answers the "GPUs tested" line: the four tests passing 3 of 3 under the validation layer with no message, and ccleste on Rebased onto current |
05a8481 to
f23f7dc
Compare
|
Rebased onto current |
adriadam10
left a comment
adriadam10
left a comment
There was a problem hiding this comment.
Re-checked on the current head: the code stands as reviewed (CB resolve guard, UINT-source resolve case, serializer and specialization bounds, debt entry), and valenciana-nc's Windows/NVIDIA run covers the draw path. The merge queue refuses it only because of @MarcZX10's open change request (2026-10-22 16:30), which his later comment does not clear and our approval cannot override. The point still open:
core/libs/prx/libSceAgcDriver/tests/execution/UintColorTarget.cpp:225(TargetTests): the execution test draws only a32_Rexport (Word32Export) into tiled and linearR32_UINT. The description and the debt entry say32_GR,32_ARand32_ABGRexports are stored as raw bits too, and the ccleste case is anR8_UINTtarget; both are covered only by decode assertions. Either add execution cases for those exports (bit-exact read-back of each exported channel, plus a 32-bit export into anR8_UINTtarget with the expected channel conversion) or narrow the claim in the description and the debt entry to what the test pins.
Once that is pushed, please ping @MarcZX10 for a re-review so the request is cleared; the rest is ready to merge.
What
A pixel shader that exports raw 32-bit values (
32_R,32_GR,32_AR,32_ABGR) into a color target whose number type is UINT reached Vulkan as afloat4output on an integer attachment:State.cpppicked the UINT attachment format from the target, but only aUINT16_ABGRexport (uintExport) made the fragment outputuint4. NVIDIA and AMD drivers write the float's bits, which is what the console's CB stores for 32-bit channels, but Metal refuses the pipeline, so on macOS two catalog titles stop at their first draw (#9783):(PPSA99313 Doom 3 BFG,
COLOR_32_32_32_32UINT; PPSA59322 ccleste the same withCOLOR_8UINT andMTLPixelFormatR8Uint.)The fragment-output specialization already retypes a
float4output touint4with a bitcast for the two word-packed targets (Unorm10_11_11,Float11_11_10). This adds a third kind,Uint, that does only that retype.DecodeColorFormatsets it for every UINT number type whose decode has no packing of its own, the UINT16 export keeps its existinguintExportpath (the output is alreadyuint4there, so the packing is cleared), and the other 16-bit export formats into a UINT target throw as the packed targets do. The rules that only make sense for packed targets, no partial writes, no blending, no DCC and no CB resolve, stay on the packed kinds (Unorm10_11_11,Float11_11_10and, since #8563,Unorm4_4_4_4) throughWordPacked;Uintis the last enumerator, after #8563's.Blending on a UINT target is dropped at decode: the Vulkan framebuffer chapter says
Blending is only defined for floating-point, UNORM, SNORM, and sRGB formats, Metal refuses it, and Doom 3 BFG has it enabled on that same target. A CB resolve whose source is a UINT target keeps working: the resolve guard that refuses the two word-packed sources now testsWordPackedinstead of any packing, with a decode case for a UINT-source resolve. The debt entry records what is not measured on a console: the CB's conversion of a raw export into 8- and 16-bit UINT channels and whether it blends into integer targets.Tested
ci/macos-series-2integration tree at53db0badbwith fix(tests): compare the Bessel exports with fixed reference values #7524, the code of feat(libkernel): JIT shared memory on macOS hosts #7937 and the libSceSsl change of feat: macOS on Apple Silicon host support (Rosetta 2 and MoltenVK) #7332: the two titles: PPSA99313 Doom 3 BFG on that tree without this change dies at its first rect-list draw with the lines above; with it the pipeline compiles and the title runs through to its own data check and exits 0 (Sys_Error: Could not load default font, Doom 3 BFG (PPSA99313) #9787). PPSA59322 ccleste moves from the same compile error to the MoltenVK barycentric gap noted on macOS support #1 (ccleste (PPSA59322) #9788). MoltenVK still printsBlending is enabled for attachment with format VK_FORMAT_R8G8B8A8_UINT, which does not support itfor these targets: itsMVKPipeline.mmprints that for every attachment whose format cannot blend, whateverblendEnablesays, and a log line I added beforevkCreateGraphicsPipelinesconfirmed no integer attachment reached it with blending on.agc_driver_packed_float_color_targetandagc_driver_packed_unorm_color_targetpass 3 of 3 on MoltenVK with the correctedSelectExportPacking, and both title results above were re-run on that code with the same outcome; the integration tree is older thanmainand cannot build the new execution test or runagc_driver_graphics_teststo the new checks (it stops earlier on a check ofmain's test file against the tree's olderState.cpp), so those two are covered by the hosted run below.fced1e175d, the head before the resolve-guard change, fails the job on any ctest failure and passesagc_driver_graphics(withuintTargetTests),agc_driver_graphics_maps_fallback,agc_driver_uint_color_target(the new execution test),agc_driver_packed_float_color_target,agc_driver_packed_unorm_color_target,agc_driver_packed_unorm4444_color_targetand theshader_recompilertests. Its only failure,agc_driver_sgpr_pattern_fill_keys, fails the same way onmainon that runner (the base side of https://github.com/rooklz/AnyPS5/actions/runs/39845397298). The same run on the current headf23f7dcd90, rebased over feat(agc): render 4_4_4_4 UNORM color targets through packed R16_UINT exports #8563, is https://github.com/rooklz/AnyPS5/actions/runs/40010114899 (and on05a84816f6before it, https://github.com/rooklz/AnyPS5/actions/runs/39982371741): every targeted test passes, and its two failures,agc_driver_queued_index_read_orderandagc_driver_sgpr_pattern_fill_keys, aremain's on that runner. An earlier version of this section called a run on81d67188b6passing when its log had the new execution test failing:SelectExportPackingin the SPIR-V backend packed every non-Nonekind as a word, so a UINT target received the 10_11_11 UNORM packing of the float instead of its bits; @adriadam10 found it in that log.maina6dcea9279:agc_driver_graphics_testspasses plain and with--maps-fallback(both reach the newuintTargetTestsand the UINT-source resolve check), andagc_driver_uint_color_target_tests,agc_driver_packed_float_color_target_testsandagc_driver_packed_unorm_color_target_testspass, each binary run directly with its exit status checked.agc_driver_graphics_testsgainsuintTargetTests: aCOLOR_32UINT target decodes toR32_UINTwith theUintpacking and its blend dropped,COLOR_8UINT toR8_UINTthe same, aUINT16_ABGRexport keepsuintExportwith no packing, anFP16_ABGRexport into it is refused, and a 32-bit float target keeps no packing. The newagc_driver_uint_color_target_testsdraws eight 32-bit words (0, 1, the bits of 1.0f, a quiet NaN, negative infinity, negative zero,0xdeadbeef,0xffffffff) through a32_Rexport into tiled and linearR32_UINTtargets and reads each back bit for bit, the wayagc_driver_packed_float_color_target_testsdoes for the 11_11_10 target.f9b349a056merged ontomainc383e3772b:agc_driver_graphics,agc_driver_packed_float_color_target,agc_driver_packed_unorm_color_targetandagc_driver_uint_color_targetpass 3 of 3 each with no validation message;mainpasses the first three. PPSA59322 ccleste reaches its first level onmainand with this change (two runs each, A/B/B/A, natural exit 0);mainlogs sixUndefined-Value-ShaderFragmentOutputMismatch-DynamicRenderingwarnings for the float output intoR8_UINT, and this change logs none (details on ccleste (PPSA59322) #9788).check_conventions.pypasses.pr_overlap.pyreports feat(agc): render 4_4_4_4 UNORM color targets through packed R16_UINT exports #8563 and feat(agc): merge partial writes into 4_4_4_4 UNORM color targets #8564 (4_4_4_4 UNORM packings) inDecodeColorFormatand feat: macOS host support (Apple Silicon via Rosetta 2, Vulkan on MoltenVK) #8658 next to theuintExportline; none implements this, and whichever lands second rebases a few lines.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)