Repository navigation
fix(agc): apply a LOD bias beyond the device limit in the shader - #14621
Merged
oneandonlydean merged 8 commits intoNov 5, 2026
Merged
Conversation
The S# field reaches -32 to +31.99, but Vulkan refuses a mipLodBias beyond maxSamplerLodBias, so the sampler threw on a bias the guest may set. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Collaborator
|
Please account for sampler/shader-bias cancellation in the new LOD-bias debt entry and check a focused mixed-bias sampling case. Vulkan clamps the sum of sampler and shader bias (https://docs.vulkan.org/spec/latest/chapters/limits.html); clamping the sampler first is different. With limit 15, sampler bias +16 (S# field 0x1000) and shader bias -14 give a combined +2, while the new clamp plus the unchanged shader operand gives +1. At implicit LOD 2 on a suitable multilevel view, that can select mip 4 versus mip 3 without extreme magnification or minification. |
Contributor
Author
|
Confirmed, the wider claim was wrong. On an RTX 4080, S# +16 with shader bias -14 reads level 3 where the sum selects 4, and S# +31.67 with -29 reads level 0 where the sum selects 5. The debt entry now records the cancellation and these results, and the PR description has the full table for both GPUs. I kept the narrower clamp and did not change the lowering. 🤖 Addressed by Claude Code |
Vulkan clamps the sum of sampler and shader bias, so a shader bias of the opposite sign cancels against the clamped S# bias, not the guest one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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.
|
CnRJay
reviewed
Nov 1, 2026
CnRJay
left a comment
CnRJay
left a comment
Contributor
There was a problem hiding this comment.
The diagnosis and the measured table are good, and oneandonlydean's cancellation point is recorded. The remaining problem is the approach itself: this ships a result that is known to be wrong, and the description says so.
It is an approximation, and the repository rule is exact-or-throw. AGENTS.md: "if the general behaviour is unknown, throw and record it in docs/dev/debt". Here the behaviour is known, and the PR substitutes a different one. The table shows +16 with shader bias -14 selecting level 4 on the guest arithmetic and level 3 here, and +31.67 with -29 selecting 5 and reading 0. That is a wrong mip, not a rounding difference, in exactly the case (a large sampler bias cancelled by a shader bias) that makes a title set a bias of that size. Recording it as debt does not change what the sampler returns.
An exact lowering exists, and it only costs for the samplers that need it. Vulkan clamps the sum of sampler and shader bias to maxSamplerLodBias, so passing the excess through the shader operand cannot work either, as the description says. What does work is a variant for samplers whose decoded bias exceeds the limit:
- create the host sampler with
mipLodBias = 0; - in the shader, compute
lod = OpImageQueryLod(...).y + samplerBias + shaderBiasin full float, apply the S# min/max LOD clamp, and sample withOpImageSampleExplicitLodand theLodoperand.
OpImageQueryLod returns the implicit LOD from the quad's derivatives, so the result is the guest sum, uncapped. Only draws that bind a sampler over the limit get the variant (key it on a bit in the sampler state), so the common sampler path is unchanged and no per-draw cost is added elsewhere.
If that is too large for this PR, the convention is to keep the throw (the Neptunia stop stays recorded) rather than land the clamp. If the clamp is kept, I'd ask for it to be limited to the case where it is provably exact: a shader bias that is a compile-time zero.
Smaller: #12904 pins rejecting a bias beyond the limit. Say in the description which one is expected to land, so the suite is not left asserting both.
Contributor
✅ AMD GPU test: passedPR head
Unstable on main (4, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (9, not counted against this PR)Validation IDs already present on main (5 IDs in 484 tests)Skipped (55)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
Contributor
✅ NVIDIA GPU test: passedPR head
Unstable on main (2, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (4, not counted against this PR)Validation IDs already present on main (5 IDs in 484 tests)Skipped (46)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
reviewed
Nov 1, 2026
MarcZX10
left a comment
MarcZX10
left a comment
Contributor
There was a problem hiding this comment.
Reviewed — no blocking findings
A guest LOD bias beyond the device limit is now clamped to the limit when the sampler is created, where the base refused it. The change matches what the agc debt entry describes. The device-limit values and the console behaviour in that entry are not verified here. The hosted Windows run passed agc_driver_graphics, the binary that runs samplerLodBiasTests.
What I ran
- Head
6834476a85042d915ec184e5e2ecf763901c48fa; basef2d874c1fdccf781c0fa5d86619bb5dd8894b958. 3 files, +26/-5:core/libs/prx/libSceAgcDriver/Graphics/src/Sampler.cpp(+ the clamp), the test inGraphics.cpp(newsamplerLodBiasTests, call at line 13228), anddocs/dev/debt/libSceAgcDriver.md. - Read the full diff, the base
Sampler::Validatecheck (Sampler.cppline 12), and the new test's expectations:1.0,-1.0,15.0at the limit,15.0for an over-limit positive value, and-15.0for an under-limit negative value, with no leaked objects. - Debt entry: says the S# LOD bias reaches -32 to +31.99, that Vulkan refuses a larger
mipLodBias(15 on an RTX 4080, 15.99 on Intel UHD 770), and that the shader bias is left unchanged. The entry says the effect on sampled levels is not measured on a console. - Precheck: diff check, conventions and PR template, and the live stack check passed. The ABI audit is not applicable (no changed source or public header under
core/libs/prxdeclares a new export; the test is internal). - Hosted: the windows CTest run passed 39 of 39 cases, including
agc_driver_graphics(Test #1509). - No MarcZX10 review at this head.
Results
| Check | Result | Note |
|---|---|---|
| Diff check | passed | |
| Conventions and PR template | passed | precheck and hosted conventions passed |
| Mergeability and dependencies | passed | depends on none; the live stack check passed |
Clamp in Sampler::Validate path |
passed | read; std::clamp to the device limit |
| Test expectations (in-range, at limit, over and under) | passed | read; they match the clamp |
agc_driver_graphics (hosted) |
passed | 13.87 s; the binary calls samplerLodBiasTests (Graphics.cpp line 13228) |
| Device limits in the debt entry | not verified | 15 and 15.99 are device-specific claims, not checked here |
| Console behaviour of the sampled level | not verified | the debt entry says it is not measured on a console |
| Build and runtime tests (Linux) | not run | the hosted run covers the binary; no local build |
| Negative controls | not run | no local build |
| Hosted CI | passed | windows passed (39/39); linux and relinkers skipped |
Commands and unedited precheck output
Commands:
- Precheck:
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 14621 --output-dir <scratch>/precheck/14621p(report.md in that directory). - Head fetched by URL (objects only):
git fetch https://github.com/boykopovar/AnyPS5.git pull/14621/head. - Hosted checks:
gh api repos/boykopovar/AnyPS5/commits/6834476a85042d915ec184e5e2ecf763901c48fa/check-runs; Windows CTest:100% tests passed, 0 tests failed out of 39.
Precheck table (unedited):
| Check | Status | Command / reason |
|---|---|---|
| build_and_compile | not_applicable | The precheck deliberately does not configure, build, or compile AnyPS5. |
| diff_check | passed | git diff --check f2d874c1fdccf781c0fa5d86619bb5dd8894b958...6834476a85042d915ec184e5e2ecf763901c48fa |
| conventions_and_pr_template | passed | /marc/DeusMachina_0x1914/.venv/bin/python3 tools/check_conventions.py --base f2d874c1fdccf781c0fa5d86619bb5dd8894b958 --head 6834476a85042d915ec184e5e2ecf763901c48fa --title fix(agc): clamp a guest LOD bias to the device limit --body /dev/shm/reviewer-run/precheck/14621p/pr-body.md |
| changed_export_abi_audit | failed | /marc/DeusMachina_0x1914/.venv/bin/python3 tools/pr_abi_audit.py --base f2d874c1fdccf781c0fa5d86619bb5dd8894b958 --head 6834476a85042d915ec184e5e2ecf763901c48fa --json |
| python_syntax | not_applicable | No changed Python implementation files. |
| related_python_unit_tests | not_applicable | No changed tools/ module has an existing sibling tools/tests/test_<module>.py suite. |
| python_test_registration | not_applicable | No changed test_*.py files. |
| live_dependency_and_mergeability_check | passed | /marc/DeusMachina_0x1914/.venv/bin/python3 /marc/AnyPS5/tools/pr_stack_check.py 14621 --repo boykopovar/AnyPS5 --output-dir /dev/shm/reviewer-run/precheck/14621p/stack |
| build_dependent_cpp_tests | not_applicable | Not run: C++ tests need exact-head binaries, which require a build; the precheck does not build. |
| test_quality_heuristics | failed | /marc/DeusMachina_0x1914/.venv/bin/python3 /dev/shm/reviewer-run/precheck/14621p/head-6834476a8504-c67f1670/tools/pr_test_quality.py --root /dev/shm/reviewer-run/precheck/14621p/head-6834476a8504-c67f1670 --json core/libs/prx/libSceAgcDriver/Graphics/src/Sampler.cpp core/libs/prx/libSceAgcDriver/tests/Graphics.cpp docs/dev/debt/libSceAgcDriver.md |
| negative_controls | not_applicable | No changed Python test module in tools/ can be run as a no-build base-source control. |
Negative control
Not run here. The test needs a build.
Findings
Blocking
None.
Non-blocking
- The debt entry's device limits (15 on an RTX 4080, 15.99 on an Intel UHD 770) and the sampled-level examples are specific claims that this review did not check. Please link the source of each measurement, or say they come from the Vulkan driver documentation.
Sources
- Clamp and base check: https://github.com/boykopovar/AnyPS5/blob/6834476a85042d915ec184e5e2ecf763901c48fa/core/libs/prx/libSceAgcDriver/Graphics/src/Sampler.cpp
- Test:
- Debt entry: https://github.com/boykopovar/AnyPS5/blob/6834476a85042d915ec184e5e2ecf763901c48fa/docs/dev/debt/libSceAgcDriver.md
Not verified
- The device limits and the sampled-level examples in the debt entry were not checked.
- The console behaviour was not measured, as the debt entry says.
To get this to a good state
- Nothing blocking. The source of the device-limit claims would help the reader.
Clamping the S# bias changed the LOD when a shader bias of the opposite sign cancelled it. An S# bias beyond 15 now leaves the host sampler at 0 and reaches the shader through a specialization constant: implicit and gradient samples scale their derivatives by two to the summed bias, and explicit LOD samples add it to the LOD. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
danielhorngt
changed the title
Contributor
Author
|
Done: the clamp is gone and S# +16/-14, -16/+14 and +31.67/-29 now read the summed level (4, 0, 5) on an RTX 4080 and a UHD 770, in a new committed test, A bias beyond 15 leaves the host sampler at 0 and reaches the shader through a per-sampler specialization constant. I took your approach with one change: implicit and gradient samples scale their derivatives by two to the summed bias, instead of Cases I could not make exact still throw and are in the debt entry: a sampler table entry beyond 15, a cube image, and an LOD query or adjusted sample with such a bias. The explicit LOD and gradient paths beyond the limit have no GPU test yet. The description now says this is expected to land instead of the bias check in #12904. 🤖 Addressed by Claude Code |
Contributor
✅ 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 (4, not counted against this PR)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (5 IDs in 483 tests)Skipped (46)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
reviewed
Nov 2, 2026
MarcZX10
left a comment
MarcZX10
left a comment
Contributor
There was a problem hiding this comment.
Reviewed — no blocking findings
This is a re-review at the current head, which replaces the approach I reviewed at 6834476a85. Earlier, guest LOD biases beyond the device limit were clamped in the host sampler. Now biases beyond 15 keep the host sampler bias at 0 and reach the shader through a per-sampler specialization constant. Explicit LOD adds the bias before the minimum-LOD clamp. Implicit and gradient samples scale their derivatives by 2^bias, which shifts the LOD by the bias and leaves the anisotropy ratio unchanged. The checks read in this change are consistent. Two things are not verified here: the GPU execution test was Skipped in the hosted Windows run, and the argument order of the dref path was not traced.
What I ran
- Head
7b03f92ac928475407ff2483bc3dfb68468a4b4b; basef2d874c1fdccf781c0fa5d86619bb5dd8894b958. 10 files, +302/-10: the sampler (Sampler.cpp,Sampler.hpp),ShaderResources.cpp,Resources.hpp(AppliesLodBiasInShader),DescriptorBindingBuilder.cpp,PipelineSpecialization.hpp(base 57600 and threshold 15.0),SpirvImageEmitter.cpp(+56),tests/Graphics.cpp(+34), the new execution testtests/execution/ImageSampleLodBiasSum.cpp(+196) and the debt line. - Read the sampler and binding changes: the bias field is 14 bits with 8 fractional bits (range -32 to +31.996), biases with |bias| > 15 are handled in the shader, and shader-side failures are explicit for table entries, cube images, LOD queries and adjusted samples.
- Read the emitter's
emitBiasedSample: the explicit path adds the bias to the LOD and then applies the clamp; the implicit and gradient paths insert derivatives when none are given, scale them bypow(2, total), and emit an explicit-LOD sample. The specialization-constant branch is uniform across invocations, so the derivative operations sit in uniform control flow. - Checked the specialization base: 57600 does not collide with the other bases in
PipelineSpecialization.hpp. - Registration: the execution test is registered by the glob in
libSceAgcDriver/CMakeLists.txt(line 556) asagc_driver_image_sample_lod_bias_sum. - Read-only checks from the trusted main checkout (precheck not run: no PR-head code is run here): diff check and conventions passed; the test-quality heuristic flagged the new execution test, a false positive because the glob registers it.
- Hosted: the windows CTest run passed 2590 of 2590 cases, including
agc_driver_graphics(Passed). The execution testagc_driver_image_sample_lod_bias_sumwas Skipped on the Windows runner (no Vulkan device there). - No MarcZX10 review at this head.
Results
| Check | Result | Note |
|---|---|---|
| Diff check | passed | |
| Conventions and PR template | passed | conventions check from the main checkout |
| Mergeability and dependencies | passed | depends on none; the stack check passed |
| Bias field decoding and range | passed | read in DescriptorBindingBuilder.cpp |
| Shader-side failures are explicit | passed | table entries, cube images, LOD queries and adjusted samples fail |
| Explicit-LOD path (bias, then clamp) | passed | read in emitBiasedSample |
| Implicit and gradient paths (derivative scaling) | passed | read; the uniform branch keeps the derivative operations valid |
| Specialization base 57600 | passed | no collision with the other bases |
| Dref path argument order | not verified | the operand layout in emitSample was not traced |
agc_driver_image_sample_lod_bias_sum (hosted) |
not run | Skipped on the Windows runner (no GPU) |
agc_driver_graphics (hosted) |
passed | the mock sampler tests |
| ABI audit | not applicable | no exported symbol changed |
| Build and runtime tests | not run | no local build |
| Negative controls | not run | no local build |
| Hosted CI | passed | windows passed (2590/2590) with the execution test Skipped; linux and relinkers skipped |
Read-only check output
Precheck was not run: no PR-head code is run here. Conventions: passed. Test-quality heuristic: flagged the new execution test, which the glob registers (false positive). Hosted: gh api repos/boykopovar/AnyPS5/commits/7b03f92ac928475407ff2483bc3dfb68468a4b4b/check-runs.
Negative control
Not run here. The execution test needs a Vulkan device, and code from this head is not run here.
Findings
Blocking
None.
Non-blocking
- The shader path's only execution test was Skipped on the hosted runner. The PR's evidence for the shader behaviour is therefore the author's own GPU run. Please attach that output for this head (the device and driver), or the reviewer cannot see it.
- The shader threshold is the constant 15.0, while the host limit is device-dependent (15 on one device, 15.99 on another). The two sides use the same constant, so they agree; a device whose limit is below 15 would throw for biases in between. Please note that limitation in the debt entry.
- The dref path with a gradient or implicit bias inserts the derivatives at the front of
argumentsbefore scaling. Please confirm in the PR that the dref operand is not among the first two arguments for that opcode.
Sources
- Sampler and bias decoding: https://github.com/boykopovar/AnyPS5/blob/7b03f92ac928475407ff2483bc3dfb68468a4b4b/core/libs/prx/libSceAgcDriver/Graphics/src/Sampler.cpp
- Binding builder: https://github.com/boykopovar/AnyPS5/blob/7b03f92ac928475407ff2483bc3dfb68468a4b4b/core/shader/recompiler/Optimization/src/DescriptorBindingBuilder.cpp
- Emitter: https://github.com/boykopovar/AnyPS5/blob/7b03f92ac928475407ff2483bc3dfb68468a4b4b/core/shader/recompiler/SpirvBackend/src/SpirvImageEmitter.cpp
- Execution test: https://github.com/boykopovar/AnyPS5/blob/7b03f92ac928475407ff2483bc3dfb68468a4b4b/core/libs/prx/libSceAgcDriver/tests/execution/ImageSampleLodBiasSum.cpp
Not verified
- The GPU execution test was not run (Skipped hosted).
- The dref operand order in
emitSamplewas not traced. - The hardware measurements the PR body cites were not checked.
To get this to a good state
- Nothing blocking. Attach the GPU run output for this head, and trace or note the dref operand order.
…er-lod-bias # Conflicts: # core/shader/recompiler/IntermediateRepresentation/include/IntermediateRepresentation/IrMetadata/Resources.hpp # core/shader/recompiler/Optimization/src/DescriptorBindingBuilder.cpp # core/shader/recompiler/PipelineSpecialization.hpp # core/shader/recompiler/SpirvBackend/src/SpirvImageEmitter.cpp # docs/dev/debt/libSceAgcDriver.md
Contributor
Author
|
Thanks. On your three non-blocking points:
🤖 Addressed by Claude Code |
BunnyFl
approved these changes
Nov 2, 2026
BunnyFl
left a comment
BunnyFl
left a comment
Contributor
There was a problem hiding this comment.
Reads right, and this fills the two gaps in MarcZX10's re-review: the GPU test on a real device, and the Dref operand order.
Dref order. emitDirectSample (core/shader/recompiler/SpirvBackend/src/SpirvImageEmitter.cpp) pushes drefValue itself, right after the coordinate and before the image-operand mask. So arguments never holds the depth reference, and emitBiasedSample's biased.front() is the Lod or Bias operand. The rewritten operands stay in mask-bit order, as the SPIR-V Image Operands rules require: Lod is first when present, Bias is removed from the front, and the added dPdx/dPdy go to the front, since Grad is then the lowest remaining bit, ahead of any MinLod. An implicit compare sample becomes OpImageSampleDrefExplicitLod with Grad, which that opcode allows. The derivatives sit inside the EmitValueIfElse on the per-sampler specialization constant, a uniform condition.
Tested on Windows 11, WinLibs GCC 15.2.0, with this head merged onto current main, on this machine's GPU: agc_driver_image_sample_lod_bias_sum and agc_driver_graphics pass, nothing skipped. The execution test is the one the hosted Windows runner skipped.
CnRJay's objection was to the earlier host-sampler clamp. This head replaces it with the derivative scaling MarcZX10 describes, which gives the guest's summed bias rather than a clamped one, but CnRJay hasn't re-reviewed it yet.
XairRrR
suggested changes
Nov 2, 2026
XairRrR
left a comment
XairRrR
left a comment
Contributor
There was a problem hiding this comment.
Reviewed — changes requested
This pull request does not merge cleanly into main (5cb015c): head 9b752d9 conflicts in
core/shader/recompiler/PipelineSpecialization.hppcore/shader/recompiler/SpirvBackend/src/SpirvImageEmitter.cpp
Please rebase it onto current main. I will review the change once it merges cleanly.
…er-lod-bias Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
✅ AMD 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 (10, not counted against this PR)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 510 tests)Skipped (53)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
XairRrR
suggested changes
Nov 2, 2026
XairRrR
left a comment
XairRrR
left a comment
Contributor
There was a problem hiding this comment.
Re-checked — changes still requested
Since my last review the branch was merged with main twice (9b752d9bf486 and 76d28cbd7e97). The merged tree builds and passes the mock tests and the convention checks. The new GPU execution test agc_driver_image_sample_lod_bias_sum fails on lavapipe, the device Linux CI runs Vulkan tests on. It fails at its first case, which has no S# or shader bias, and it fails the same way with the PR's production files reverted to main.
What changed since my last review
- New commits: two merges of
upstream/main(conflicts resolved inResources.hpp,DescriptorBindingBuilder.cpp,PipelineSpecialization.hpp,SpirvImageEmitter.cppand the debt file).SamplerLodBiasBaseis now 57664, becausemaintook 57600 forMinLodBase.EmitSampleOpnow shares itsclampLodhelper withmain's new T# MIN_LOD clamp. - Author replies: GPU output for the RTX 4080 and UHD 770 at
9b752d9bf4, and a reading of the Dref operand order, both in reply to MarcZX10. BunnyFl approved at9b752d9bf486. - The text of my previous review is not available to me, so this is a full review of the change.
What I ran
Head 76d28cbd7e97edc44f2bc257671ed76c3f6a0ebe, main 5cb015caaaaf83f8ee431414ee6a457b5b307d49, merged tree 43e456e9a97bffb9d208f759e7a6074d99b55609. Debian 13 VM, GCC 14.2.0, lavapipe (Mesa 26.1.6, LLVM 19).
git diff --check 5cb015caaaaf83f8ee431414ee6a457b5b307d49...76d28cbd7e97edc44f2bc257671ed76c3f6a0ebe
python3 tools/check_conventions.py --base 5cb015caaaaf83f8ee431414ee6a457b5b307d49
ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 ctest --test-dir build -R "^(agc_driver_image_sample_lod_bias_sum|agc_driver_image_sample_lod_clamp_shader_lane|agc_driver_graphics)$" --output-on-failure
ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 build/tests/agc_driver_image_sample_lod_bias_sum_tests # also without GALLIUM_OVERRIDE_CPU_CAPS
# control: the 7 production files restored from main
git show 5cb015caaa:<path> > <path>
ninja -C build tests/agc_driver_image_sample_lod_bias_sum_tests tests/agc_driver_image_sample_lod_clamp_tests
ctest --test-dir build -R "^(agc_driver_image_sample_lod_bias_sum|agc_driver_image_sample_lod_clamp.*)$" --output-on-failure
# control: Sampler.hpp, Sampler.cpp, ShaderResources.cpp restored from main
ninja -C build tests/agc_driver_graphics_tests
build/tests/agc_driver_graphics_tests
build/tests/agc_driver_graphics_tests --sampler-table-lod-bias-beyond-threshold
# merged files restored, rebuilt, git status --porcelain clean
| Check | Result |
|---|---|
git diff --check |
passed |
check_conventions.py |
passed |
| Build (script) | passed |
agc_driver_graphics (mock, includes samplerLodBiasTests) |
passed, 38.2 s on this VM |
agc_driver_image_sample_lod_bias_sum |
failed: S# field 0 with shader bias 0.000000 read 120, expected level 2 |
same, with main's production files |
failed, same message |
agc_driver_image_sample_lod_clamp_shader_lane |
failed, and fails the same way with main's production files, so it comes from main (the script's baseline 096c45570f predates #12980), not this PR |
unwrap_self_..._destination_directory_group |
failed with os.chown EINVAL in the sandbox. This tools test is untouched by the PR, so I am not counting it |
| Vulkan validation (script, 760 tests, layer loaded in 759) | 0 new VUID- lines compared with main |
lavapipe output of the new test
Selected GPU name=llvmpipe (LLVM 19.1.7, 256 bits) vendor=0x10005 device=0x0 subgroup=8
AGC graphics: image sample LOD bias sum: S# field 0 with shader bias 0.000000 read 120, expected level 2
Negative control
- Mock: with
Sampler.hpp,Sampler.cppandShaderResources.cpprestored frommain,agc_driver_graphics_testsfails withguest sampler descriptor requests a LOD bias beyond the device limit. SosamplerLodBiasTestsdetects the change. --sampler-table-lod-bias-beyond-thresholdpasses onmain's code as well (see the non-blocking finding).- GPU test: no control is possible on lavapipe, because the test fails before reaching a biased case.
Previous findings
My previous review's text is not available, so I cannot list its findings one by one. This re-check covers the whole change.
New findings
Blocking
- The new execution test fails on lavapipe at its unbiased case (
core/libs/prx/libSceAgcDriver/tests/execution/ImageSampleLodBiasSum.cpp:174). Case{0u, 0.0f, 2u}has no S# bias and no shader bias, and lavapipe reads level 6 (red 120) in place of level 2. The result is the same with every PR production file reverted tomain, so the PR's lowering is not at fault. Either the test's LOD setup depends on the device, or lavapipe computes this shader's implicit LOD differently. CONTRIBUTING says Linux CI runs Vulkan tests on lavapipe, so this test would fail on every full Linux run. The AMD/NVIDIA runs and the author's Windows runs pass it, so it is specific to lavapipe. Suggested fix: find out why the baseline case selects level 6 on lavapipe. Candidates are derivatives across the recompiled wave64 pixel shader's quads, or the texture's mip layout. Then make the test agree with lavapipe, or, if this is a lavapipe limitation, have the test detect it and report a skip thattools/check_skips.pyaccepts.
Non-blocking
core/libs/prx/libSceAgcDriver/tests/Graphics.cpp:7276: the new sampler-table entry0x1000(+16) is rejected onmainalready, because the mock'smaxSamplerLodBiasis 0.5. The case passes withmain'sSampler.cpp/ShaderResources.cpp, so it does not test the new table check inShaderResources.cpp. RaisingmaxSamplerLodBiasabove 16 for this case would make it fail without the newRequire.core/shader/recompiler/Optimization/src/DescriptorBindingBuilder.cpp:407adds a specialization constant for every non-table sampler with LOD uses, including in-range ones (value 0). This is correct, but it adds one more specialization entry per sampler on the common path. That cost is small and probably acceptable.
Not verified
- The explicit-LOD, gradient and Dref paths beyond the limit: no GPU test covers them, as the description says. I read the code, and I agree with BunnyFl's reading of the Dref operand order.
- The RTX 4080, UHD 770, AMD and NVIDIA results are author- and bot-reported. I had no hardware GPU.
- Console behaviour of the S# bias on explicit LOD samples. It is recorded as unmeasured in the debt entry.
To get this to a good state
- Make
agc_driver_image_sample_lod_bias_sumpass on lavapipe, or have it report a device-limitation skip there, and say what lavapipe does differently. - Optionally, make the sampler-table case fail without the new check.
BunnyFl
approved these changes
Nov 3, 2026
BunnyFl
left a comment
BunnyFl
left a comment
Contributor
There was a problem hiding this comment.
Re-review of 76d28cbd7e (main merged in after my approval at 9b752d9bf4). I checked the conflict resolution:
SamplerLodBiasBaseis now 57664 and main'sMinLodBaseis 57600 (core/shader/recompiler/PipelineSpecialization.hpp:52-53). The 64-wide range 57664-57727 overlaps no other base incore/shader/recompiler/PipelineSpecialization.hppbelowImageComponentBase(65536).- The biased-LOD path now goes through the shared
clampLod(core/shader/recompiler/SpirvBackend/src/SpirvImageEmitter.cpp:1674, used at:1687and:1766), which applies both the sampler clamp and main's view min-LOD clamp, the same as the explicit-LOD operand.
Windows 11, AMD RX 9070 XT, ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1, merged onto current main:
- PR:
agc_driver_image_sample_lod_bias_sumandagc_driver_graphicspass. - Main's seven production files restored: both fail with "guest sampler descriptor requests a LOD bias beyond the device limit".
Contributor
✅ 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)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 525 tests)Skipped (49)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. |
12 tasks done
…er-lod-bias Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
XairRrR
previously requested changes
Nov 3, 2026
XairRrR
left a comment
XairRrR
left a comment
Contributor
There was a problem hiding this comment.
Re-checked — changes still requested
The only new commit is d52b3c318487, a merge of main. In the PR's own change it alters one line: SamplerLodBiasBase moves from 57664 to 57728, because main now uses 57664 for VolumeGatherBase. The new GPU test agc_driver_image_sample_lod_bias_sum still fails on lavapipe at its first, unbiased case, so my blocking finding is still open.
What changed since my last review
- New commit
d52b3c318487mergesupstream/main. I compared the PR's added and removed lines at76d28cbd7e97(against itsmain) with those atd52b3c318487(against the currentmain). The only difference isSamplerLodBiasBase = 57728uincore/shader/recompiler/PipelineSpecialization.hpp:54. The range diff of the three non-merge commits shows no change. - Its 64-wide range, 57728–57791, starts right after
VolumeGatherBase(57664–57727) and ends belowImageComponentBase(65536). OnlyDescriptorBindingBuilder.cpp:410andSpirvImageEmitter.cpp:1933use it. I found no collision. - No author comments or edits to the description since my last review. There is no reply about the lavapipe failure.
What I ran
Head d52b3c31848750ae10c504f65e86b1924d591cea, main 086c0686c8adff3ee648fdbe34e0b0b647e1d511, merged tree 744f5ec39f06e1248f53ea8057f5cb639149986a. Debian 13 VM, GCC 14.2.0, lavapipe (Mesa 26.1.6, LLVM 19).
git diff --check 086c0686c8adff3ee648fdbe34e0b0b647e1d511...d52b3c31848750ae10c504f65e86b1924d591cea
python3 tools/check_conventions.py --base 086c0686c8adff3ee648fdbe34e0b0b647e1d511
# PR's own +/- lines, previous head vs current head
diff <(git diff 5cb015caaa...76d28cbd7e97 | grep '^[+-]') <(git diff 086c0686c8...d52b3c3184 | grep '^[+-]')
ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 ctest --test-dir build -R "^(agc_driver_image_sample_lod_bias_sum|agc_driver_graphics)$" --output-on-failure
# control: the 7 production files restored from main
git show 086c0686c8:<path> > <path>
ninja -C build tests/agc_driver_image_sample_lod_bias_sum_tests tests/agc_driver_graphics_tests
build/tests/agc_driver_image_sample_lod_bias_sum_tests
build/tests/agc_driver_graphics_tests
# merged files restored from 744f5ec39f, rebuilt; git status --porcelain clean
| Check | Result |
|---|---|
git diff --check |
passed |
check_conventions.py |
passed |
| Build (script) | passed |
agc_driver_graphics (mock) |
passed, 37.1 s on this VM |
agc_driver_image_sample_lod_bias_sum |
failed: S# field 0 with shader bias 0.000000 read 120, expected level 2 |
Same test with main's production files |
failed, same message |
agc_driver_graphics with main's production files |
failed: guest sampler descriptor requests a LOD bias beyond the device limit (so the mock test detects the change) |
| Vulkan validation (script, 784 tests, layer loaded in 783) | No new VUID- lines in any test that also ran on main. agc_driver_image_gather_volume reports VUID-VkSamplerCreateInfo-addressModeU-01079 / -parameter, but it is marked "test not run on main" and came in with main's volume-gather change, so I am not counting it against this PR |
| Other failures (script) | Only the new test above. The 7 known failures also fail on main |
Previous findings
- Blocking — the new execution test fails on lavapipe at its unbiased case (
core/libs/prx/libSceAgcDriver/tests/execution/ImageSampleLodBiasSum.cpp:174). Still open. It fails the same way at this head (case{0u, 0.0f, 2u}reads red 120, which is level 6, where level 2 is expected), and again with all PR production files restored frommain. The PR's lowering is not the cause. The test's assumption about implicit LOD does not hold on lavapipe, which is the device Linux CI runs Vulkan tests on. - Non-blocking — the sampler-table case
0x1000does not test the new table check (core/libs/prx/libSceAgcDriver/tests/Graphics.cpp:7434). Still open. The test code has not changed, and the mock'smaxSamplerLodBiasis still 0.5 (Graphics.cpp:7449), soSampler::Validatealready rejects the entry onmain. - Non-blocking — a specialization constant for every non-table sampler with LOD uses (
core/shader/recompiler/Optimization/src/DescriptorBindingBuilder.cpp:410). Still open. Unchanged, and acceptable.
New findings
Blocking
None.
Non-blocking
None.
Not verified
- No GPU test covers the explicit-LOD, gradient or Dref paths beyond the limit. This is unchanged.
- The results on RTX 4080, UHD 770, AMD and NVIDIA are reported by the author or by bots. The AMD/NVIDIA bot runs at
76d28cbd7e97predate this merge. I had no hardware GPU. - How the console applies the S# bias to explicit-LOD samples is not measured; the debt entry already records this.
To get this to a good state
- Make
agc_driver_image_sample_lod_bias_sumpass on lavapipe, and say what lavapipe does differently for the baseline case. If that is a device limitation, have the test detect it and report a skip thattools/check_skips.pyaccepts. - Optional: raise the mock's
maxSamplerLodBiasabove 16 for the0x1000sampler-table case, so it fails without the newRequireinShaderResources.cpp.
Contributor
✅ NVIDIA GPU test: passedPR head
Unstable on main (2, 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 (8 IDs in 527 tests)Skipped (50)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. |
12 tasks done
Contributor
|
@shahulmubeen-a @danielhorngt a heads-up: this PR and #14621 now take the same specialization constant base.
On |
…er-lod-bias Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
✅ NVIDIA GPU test: passedPR head
Unstable on main (2, 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 557 tests)Skipped (54)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. |
…as sum test On llvmpipe about 9 percent of pixels, in the same 2x2 quads for every case, get a wrong implicit derivative and read level 0 or 6 on the unbiased case too, so the exact all-pixel check failed before any bias was applied. Other drivers keep the exact per-pixel check; on llvmpipe the expected level must hold for at least three quarters of the pixels (90 percent measured) and no case is relaxed otherwise.
Contributor
✅ AMD 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 (8, not counted against this PR)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 557 tests)Skipped (66)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
Contributor
✅ NVIDIA GPU test: passedPR head
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 557 tests)Skipped (54)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. |
Contributor
✅ AMD 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 (8, not counted against this PR)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 557 tests)Skipped (66)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
oneandonlydean
dismissed
XairRrR’s stale review
November 5, 2026 09:07
Met: 169f433 makes agc_driver_image_sample_lod_bias_sum tolerate lavapipe's noisy implicit-LOD quads, on llvmpipe only. At least 3/4 of the 1024 pixels must still read the expected level, which a wrong bias cannot meet. Other devices keep the exact per-pixel check. It passes on lavapipe and an RTX 4090, and a negative control with a wrong expected level fails on both. The NVIDIA and AMD bots pass at 169f433.
github-actions
Bot
added
the
review queue
BunnyFl
approved these changes
Nov 5, 2026
BunnyFl
left a comment
BunnyFl
left a comment
Contributor
There was a problem hiding this comment.
@danielhorngt re-checked at 169f433e6d. git range-diff from 76d28cbd7e, the head I approved, shows your three commits unchanged. The new commit only relaxes the per-pixel check of agc_driver_image_sample_lod_bias_sum on llvmpipe, where the device name contains llvmpipe, to the expected level on at least three quarters of the pixels. Every other driver keeps the exact check, and no case's expected level changed.
On 169f433e6d merged onto main 78e4248fea (Windows 11, WinLibs GCC 15.2.0, Release, AMD Radeon RX 9070 XT, driver 32.0.32015.12012; the exact check applies there), ctest -V with ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation: agc_driver_image_sample_lod_bias_sum passes, with no missing-layer line and no VUID- message.
check_conventions.py passes. pr_overlap.py --branch finds no shared exports; #10489, #12790, #14963 and #14964 share lines.
One coordination point, not a blocker for this PR: SamplerLodBiasBase = 57728u (core/shader/recompiler/PipelineSpecialization.hpp:54 at 169f433e6d) is free on main, where the last base is VolumeGatherBase = 57664u. Two other open PRs claim the same base, though: #14964 adds HalfBorderUnnormalizedBase = 57728u at the same line, and #10489 moves FmaskFormatBase to 57728u (and VolumeGatherBase to 57856u, which #12790's GatherLodBase also takes). The edits touch neighbouring lines, and I expect the second to land to get a conflict there. main has no static_assert that the 64-wide ranges don't overlap (core/shader/recompiler/PipelineSpecialization.hpp:48-53 on main): a resolution that keeps both lines would build, with two features on the same specialization IDs. I'm flagging it on #14964 as well.
Approving again at 169f433e6d.
AI-assisted: yes (Claude Code)
12 tasks done
github-actions
Bot
removed
the
review queue
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
An S# LOD bias field reaches -32 to +31.99, but Vulkan refuses a
mipLodBiasbeyondmaxSamplerLodBias(15 on an RTX 4080) and clamps the sum of sampler and shader bias to the same limit.Sampler::Validatetherefore threw on a value the guest may legally set. Neptunia ReVerse binds +31.67 and stops there, 271-285 s in, onguest sampler descriptor requests a LOD bias beyond the device limit.An S# bias beyond 15 now leaves the host sampler at 0 and reaches the shader through a per-sampler specialization constant. Implicit and gradient samples scale their derivatives by two to the summed bias, which gives the guest sum uncapped and keeps anisotropy. Explicit LOD samples add it to the LOD. A bias of 15 or less takes the same path as before, so the common case only gains a specialized-out branch.
Still throws, recorded in
docs/dev/debt/libSceAgcDriver.md: a sampler table entry beyond 15, a cube image, an LOD query or an adjusted sample with such a bias, and a bias of 15 or less above the device limit.mixyt mentioned a prepared follow-up for this on #10556; no pull request for it exists, and this one can be dropped if that one arrives. #12904 (open) rejects a bias beyond the limit. This PR is expected to land instead of that one check, so #12904 needs to drop it or its suite will assert the opposite.
Tested
Windows 11, i9-12900K, GCC 15.2.0, Release, at 470356c on main f2d874c. The title itself was not run: I don't have it, and it also needs #10556 to reach this point.
Exact lowering, at 7b03f92 unless noted,
ANYPS5_REQUIRE_VULKAN=1,ANYPS5_NO_SHADER_CACHE=1:agc_driver_image_sample_lod_bias_sum: a pixel-shaderimage_sample_bdraw on a 64x64 7-level view at implicit LOD near 2.1. Every pixel of all 9 cases reads the summed level on both GPUs, including S# +16/-14 (4), S# -16/+14 (0), S# +31.67/-29 (5), S# -32/+32 (2), S# +31.67/0 (6) and S# -32/0 (0).VK_LAYER_KHRONOS_validation1.4.363 on the RTX 4080 and UHD 770,ctest --test-dir build -R '^(agc_driver_image_sample_lod_bias_sum|agc_driver_image_sample_shared_sampler|agc_driver_image_sample_vcc_sampler|agc_driver_emulated_color_compare.*)$' -V: 5 of 5 pass, the loader inserts the layer in all 5, 0VUID-lines.ctest -R 'agc|shader|spirv|recompiler|image|sampler'on the RTX 4080 at 9b752d9, merged with main a4eca2a: 1,274 passed, 11 skipped, 1 failed, 25 not built because their paths exceed the Windows limit here, includingagc_driver_image_plane_bias_on_single_level_volume, so those rely on the AMD and NVIDIA runs. The failure,agc_driver_direct_write_completion, also fails on this machine with this PR's sources reverted to main.agc_driver_graphics_tests(mock Vulkan): passes, withsamplerLodBiasTestscovering the host bias and the table rejections.The remaining measurements below are from the earlier clamp at 470356c and show the behaviour this replaces.
agc_driver_graphics_tests(mock Vulkan): passes. NewsamplerLodBiasTestsat that commit checked decoded S# fields within, at and beyond the limit, including the title's +31.67 (field 0x1fac) and -32.guest sampler descriptor requests a LOD bias beyond the device limit.ANYPS5_REQUIRE_VULKAN=1,ANYPS5_NO_SHADER_CACHE=1,agc_driver_image_sample_shared_sampler_testswith a temporary, uncommitted edit forcing every decoded S# to +31.67: this change passes, the old code stops with the title's error. Unedited, the test passes.ANYPS5_REQUIRE_VULKAN=1,ANYPS5_NO_SHADER_CACHE=1,VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation(1.4.363),ctest --test-dir build -R '^(agc_driver_image_sample_shared_sampler|agc_driver_image_sample_vcc_sampler|agc_driver_emulated_color_compare.*)$' -V. Each log shows the loader inserting the layer in all 4 tests and nowas not found but was requested by env var VK_INSTANCE_LAYERS:VUID-linesVUID-VkSamplerCreateInfo-mipLodBias-01069The last row shows the layer catches this bias, and that
ctestpasses regardless.ANYPS5_NO_SHADER_CACHE=1: a temporary, uncommitted pixel-shaderimage_sample_bdraw on a 64x64 7-level view with a point mip filter and an implicit LOD near 2.1. All 1024 pixels of each case read the level shown. "Sum" is the guest arithmetic, not a console measurement.Those bold cases are what this PR fixes. Whether Neptunia ReVerse pairs its +31.67 with a shader bias is not known.
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