Repository navigation
fix(agc): check whole storage views for native surface overlap - #10368
oneandonlydean merged 1 commit into
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed current head 87bdf54. Resolving viewedBytes before RequireNoMultisampleTarget closes the first-byte-only gap for callers that have not precomputed a surface size, and the check now precedes both native-depth storage handling and cached lookup. The regression exercises an overlapping cached range and an adjacent disjoint range ending exactly at the native depth boundary, then checks owner retirement and byte preservation. The author reports 47 focused Windows GPU checks with Vulkan required, shader cache disabled and validation layers confirmed, no new VUIDs; I did not run the GPU test locally. Hosted eligibility, conventions, intake, labels and report pass; product jobs are skipped by path policy. git diff --check upstream/main...upstream/pr/10368 passes. I found no blocking issue.
|
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.
|
|
Intel run, a second vendor next to the RTX 4070 Ti SUPER. Setup: Intel Arc Graphics (Meteor Lake,
AI-assisted: yes (Claude Code) |
87bdf54 to
d1dc3a5
Compare
|
Linux run of this head
Edited: the first version of this comment reported 23 failures and said they also fail on AI-assisted: yes (Claude Code) |
What
A storage-image lookup without a precomputed surface size checked only its first byte against native multisample owners. A native depth surface inside a larger cached view was missed, so the cache could return stale guest bytes and keep the overlapping native owner active.
Resolve the viewed surface's complete size before the overlap check and use that range before either the native-depth storage path or a cached storage lookup. The regression seeds a cached linear view, creates an unwritten native multisample depth surface inside it, checks a disjoint view ending at the depth boundary, then requires the complete view to retire the intersecting owner without changing guest bytes.
Tested
Current-main refresh: upstream
maine05ba6feebd000cfc316c1d1f00e37e030aa3d8b, submitted headd1dc3a5920b5017e1b958151eca93479187fb75f. Windows 11 Pro 10.0.26300, AMD Ryzen 9 7900X, 31.7 GiB RAM, WinLibs MinGW-w64 GCC 15.2.0 r7, Ninja, Release,BUILD_TESTING=ON,ANYPS5_ENABLE_SPIRV_TOOLS=ON, pinned bundled dependencies. NVIDIA GeForce RTX 4070 Ti SUPER, driver 617.42 / 32.0.16.1742; Khronos validation 1.4.363.cmake --build <build> --target relinker libs patched_libraries_tests agc_driver_graphics_tests agc_driver_native_multisample_depth_tests agc_driver_sample_multisample_target_tests agc_driver_depth_storage_dispatch_tests agc_driver_native_depth_bda_clear_tests --parallel 8).ANYPS5_REQUIRE_VULKAN=1,ANYPS5_NO_SHADER_CACHE=1,VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation,ctest -j 1 -V, and confirmed actual instance/device layer insertion. Neither log contains a missing requested validation layer,VUID-, or validation error/warning. Candidate selection:^(patched_libraries|agc_driver_(graphics|native_multisample_depth|sample_multisample_target|depth_storage_dispatch|native_depth_(bda_clear|retirement|clears|clear_refusals)))$. Main selection:^(patched_libraries|agc_driver_(graphics|native_multisample_depth|sample_multisample_target|depth_storage_dispatch|native_depth_(bda_clear|retirement|clears|clear_refusals)))$.The one original feature patch remains limited to two files. The conflict placed two independent fixtures at the same insertion point; main's
CheckDepthAttachmentAliasand this PR's byte-identicalCheckStorageAliasRetirementare now separate functions, and both run in the default native-depth case. The production patch still only resolvesviewedBytesbefore the existing native-MSAA guard and cached/native-depth lookup. No prerequisite was added and shader instruction semantics are unchanged.The new regression executes on the selected GPU: an adjacent view ending exactly at the native-depth boundary retains that owner, while a larger cached storage span retires only the intersecting unwritten owner and preserves the guest bytes. Depth-storage dispatch and native-depth BDA clear checks are included in the paired selection alongside the shared native-depth/library/graphics cases. Earlier 47-check mixed-integration evidence below remains historical; it is not claimed for the rebased head.
Earlier submitted-revision evidence (historical)
Windows 11 Pro build 26300, WinLibs GCC 15.2.0, Release, NVIDIA GeForce RTX 4070 Ti SUPER, NVIDIA 617.42.
a cached storage view kept a native depth owner inside its range.Overlap review: 31 shared-file PR diffs checked. #10049 adds overwrite coverage while retaining the one-byte fallback; this fix uses the complete span and needs a small textual merge with that call-site change. #10064 changes committed 3D slabs. The native-depth fixture additions in #6808/#9950 cover other behavior. None implements this full-span check.
Independent validation of the published commit: Published head
87bdf5445d747ddc76f0cedcb31579e9900aa9c1: full Windows Release build and native multisample depth, depth-storage dispatch, native-depth BDA clear/retirement/clear/refusal checks and patched-library validation pass on a clean tracked tree without overlays (7/7, zero skips). Vulkan is required, shader caches are disabled, and loader logs confirm actual Khronos instance/device validation layers; no VUIDs. Hardware and toolchain are the Windows/NVIDIA configuration listed above. This independently validates the published commit in addition to the earlier integration runs. The complete CTest suite was not run.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)