Repository navigation
fix(agc): refuse native MSAA index buffer reads - #6590
3 commits merged into
Conversation
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
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.
|
jotitadev
left a comment
There was a problem hiding this comment.
A run on hardware, since the description says no local GPU run was possible.
Machine: NVIDIA GeForce RTX 3070, driver 595.99.02, Vulkan 1.4.329, Ubuntu 26.04 (kernel 7.0.0-38), i7-8700. GCC 15.2, Release, BUILD_TESTING=ON, ANYPS5_GPU="NVIDIA GeForce RTX 3070". Head 881facc4f merged onto main 5d9dcc42d, no conflicts.
GPUs tested: NVIDIA GeForce RTX 3070 (595.99.02), Linux
agc_driver_sample_multisample_targetpasses 3/3. The fixture takes the host-import path on this driver (shared-import=1), and all four native-index cases (16/32-bit, base and edge) are refused withan index buffer read of ... overlaps multisampled color target ... whose samples are only on the host. The neighbouring index range draws the triangle.- Test only (the PR's fixture with
main'sDraw.cpp): fails 3/3 withAGC graphics: native indices missed 4 resident-image refusals(refused=0for all four cases). So the new test catches the missing guard on NVIDIA/Linux too, not only on the Windows RTX 5080 and AMD iGPU from #6579. ctest -R "^agc" -E "agc_display_buffer_dcc|agc_driver_flip|agc_driver_lds_atomics64" -j1 --timeout 60: 591/593 pass, the same result as unchangedmainon this machine (agc_driver_stencil_ref_exportskipped,agc_driver_store_over_untrackedfails as onmain, #6385).
Written with AI assistance (Claude Code). I ran the tests above on my machine.
Amoenus
left a comment
There was a problem hiding this comment.
Windows source review and Vulkan validation of PR head 881facc4fa50a49d4148b4f385b280b62886d144 merged onto main 5d9dcc42d2ec5401aebd089028b8d4d7410a77b4, plus only #6452's independent Windows test-alignment correction: composition b3e7aa184. The submitted Draw.cpp and multisample fixture are unchanged in that composition. This is not a bare submitted-head run.
- Full Windows application/libs build, conventions and registration pass; all execution fixtures are built.
- Five fresh focused processes per physical GPU pass all four index refusals, the shared-import neighbor triangle and all 1024 native sample checks.
- 19 related groups x 5 repetitions per GPU selector pass, with physical selection verified in 17 groups per vendor; no failures/skips/timeouts. Slowest matrix test 5.66s.
- Removing only the new guard produces all four missing refusals in every one of five fresh processes per GPU. Other controls remain correct.
Hardware: Windows 11 Enterprise 26200, WinLibs GCC 15.2r7 Release; NVIDIA RTX 5080 (10de:2c02, 32.0.16.1062) and AMD Radeon integrated graphics (1002:13c0, 32.0.21036.18). Shader cache disabled; actual vendor/device recorded in every focused process.
The production guard behaves as intended. I confirmed one fixture-oracle gap, detailed inline: omitting the 32-bit neighbor draw still passes 5/5 per GPU because the destination retains the 16-bit triangle. Six-line proposed fixture correction clears/materializes and verifies the destination before each width. It passes 5/5 per GPU, including a restored full build/check run; the same omitted-draw control with that correction now fails 5/5 per GPU at the expected zero-sample coverage assertion. Cppcheck 2.20.0 and clang-tidy 23.1.3 on Draw.cpp/the proposed fixture report no new selected diagnostic compared with the PR composition: three Cppcheck findings and one clang-tidy warning match in message/source context and remain unresolved. No parser/configuration errors or suppression. This is advisory comparison of the fixture proposal, not a clean-analyzer claim for main.
No fresh title replay, Linux runtime or PS5 console-equivalence claim. The issue's earlier Windows tests were mine; these are new tests of this PR composition. AI-assisted review/validation and proposed test correction: Codex.
|
Manual overlap check for the bot warning: #6571 changes the |
|
Tested on Linux (Arch), This PR and #5881 break That is AI-assisted: yes |
|
Thanks for testing both changes together and reporting the standalone controls. I checked the current #5881 and #6590 heads: they merge without a textual conflict, so this is a runtime interaction, not a rebase issue. The failure is after the refused index attempts, when the test reloads the original target's samples; I won't treat the independent green runs as covering this composition. Could you share the exact |
|
Sure. All runs used these pins (fetched before the runs, unchanged since):
Composition: git checkout -B test 5d9dcc42d2ec5401aebd089028b8d4d7410a77b4
git fetch upstream pull/5881/head:pr-5881 pull/6590/head:pr-6590
git merge --no-edit pr-5881
git merge --no-edit pr-6590
git submodule update --init
# existing Release build dir, BUILD_TESTING=ON
cmake . && ninja tests/agc_driver_sample_multisample_target_tests
VK_DRIVER_FILES=<lavapipe>/lvp.json ctest -R '^agc_driver_sample_multisample_target$' --output-on-failureLavapipe is Mesa 26.2.2 (llvmpipe, LLVM 22.1.8), with an Xwayland display for the device. The pair failed in all 4 runs on these pins, each time with the line from my report. One correction to my earlier comment: each PR alone passed in one run, not two; AI-assisted: yes |
|
I reconstructed your exact composition locally; the merged tree matches |
|
Tested the fixture-order explanation on Linux with current heads, since no one had run it yet. Pins:
With AI-assisted: yes |
|
Thanks for running the combined-tree check. The 3/3 failure on the merged tree and 3/3 passes after either restoring |
|
I also checked Amoenus's inline fixture finding against the current review thread. It is independent of the |
|
Updated in
Validation: |
|
Reran the combined test on 45fcdba as asked. Same setup as my last run, with
One problem with where the refill sits. It runs after the alias attempts and right before the check that the source was not written by them, so that check can no longer see a write. To test this I added one line that zeroes the source's metadata just after the alias loop:
Suggested change: move the AI-assisted: yes |
|
Fixed in |
|
Thanks for running the mutation check. You are right: the |
|
Correction after checking the published head: |
|
You're right: 1271998 already has the refill above Setup:
AI-assisted: yes |
|
Thanks for rerunning the mutation on the published |
|
Thanks for the fixture-oracle correction. The current head |
|
@MarcZX100 Extending the same component coverage exposed a separate fixed-function vertex-input gap, reported in #7015. I've opened #7016 as a dependent follow-up to your exact #6590 head The own production change is one guard call for planned vertex ranges. The index-guard parent still reproduces the vertex gap; the focused fix passes on NVIDIA, AMD and llvmpipe. Exact source pins, 25 current-head passes, 13 baseline controls and the four-file own diff are maintained in #7016. AI-assisted: Codex; author responsible. |
|
First GPU runtime for this PR (the authoring host could not reach Vulkan). RTX 3070, Ampere, driver 617.42, Windows 11 26200, WinLibs GCC 15.2.0, Release, ANYPS5_NO_SHADER_CACHE=1. Head 1271998, ctest serial fresh processes:
7/7 pass on the PR head. The one-line Draw.cpp guard plus fixture holds on real NVIDIA hardware. |
c9e6c45
What
Adds the live multisampled-target guard to fixed-function index-buffer reads after validating the guest range. The existing render-target alias check remains in place. Extends the multisample execution fixture to cover 16-bit and 32-bit index reads from a native target, the last in-range index words, and a valid neighboring index range; the target samples must remain unchanged. Updates the driver debt entry to list index reads among guarded accesses.
Fixes #6579. The issue report supplied the detailed Windows reproduction and an initial focused test/fix experiment; this PR adapts the guard and fixture to current
main.Related to #6498: that PR covers shader buffer descriptors; this change covers fixed-function index input. They share the existing multisample fixture and debt entry, but neither depends on the other.
Tested
agc_driver_sample_multisample_target_testssuccessfully. Building required a temporary localtemplateparser fix for the unrelated GCC 12 error inSrtDescriptorEvaluation.cpp; that edit is not in this PR.python3 tools/check_conventions.py --base upstream/main: passed.git diff --check: passed.Checklist
main; no open PR implements fixed-function index-buffer guard checks.mdfiles or images added to the repository