Repository navigation
perf(libSceAgcDriver): order in-place vertex inputs behind pending GPU writes with a barrier - #4649
Conversation
|
Windows test (MinGW GCC 15.2, Release, Windows 10, RTX 4080 SUPER). Built on current main
Not tested: no game run. The batch was built once, not per PR; nothing failed, so no bisect was needed. AI-assisted (Claude Code). |
adriadam10
left a comment
adriadam10
left a comment
There was a problem hiding this comment.
Built merged onto current main and ran the libSceAgcDriver ctests:
agc_driver_vertex_target_range_importedfails with this PR ("the vertex regression used the wrong copied/imported path",tests/Recorder.cpp:5643) and passes on main (same build dir, PR files reverted). That test expects a vertex range under a pending GPU write to follow the existing flush/import behaviour; with the newbehindGpuWritepath the pending read it checks (DescribePendingRead) no longer matches. Please make the code and that test agree, and say in the PR which behaviour is intended.agc_driver_tessellationsegfaults here too; please check whether it does on main for you.- Conventions is red: the template needs
Depends onfilled with#Nornone(#4824 also touchesInPlaceDrawInputinDraw.cpp, so check for overlap).
The new cases in drawInputInPlaceTests do fail to compile without the change, so they exercise it.
CnRJay
left a comment
CnRJay
left a comment
There was a problem hiding this comment.
The idea is right. A GPU-produced vertex buffer should be ordered with a barrier, not with a CPU wait. The driver submits everything to one VkQueue, so a barrier also covers writes in batches already submitted. Two parts of the current change are too broad, though, and they probably explain the agc_driver_vertex_target_range_imported failure adriadam10 reported.
1. Skipping FlushGpuWrites skips more than the recorder wait. The flush hook also writes back storage-image results that are still pending (StorageTexture layers marked by markLayersPending, written to guest memory on flush). Those results live only in the VkImage until written back, so the host import doesn't hold them yet and no buffer barrier can make them visible. With behindGpuWrite set, the hook no longer runs and only PendingWriteOverlaps is consulted. A vertex range under a render target or storage image with pending results is then bound in place over stale bytes. That is the vertex-over-target case the failing test covers. The new path should apply only when the overlapping writes are buffer writes into the import itself. It should still refuse, or fall back to the flush, when an image has pending results over the range (StorageTexture::FlushPending or its query form).
2. The source access mask misses attachment writes. VK_ACCESS_SHADER_WRITE_BIT | VK_ACCESS_TRANSFER_WRITE_BIT doesn't cover COLOR_ATTACHMENT_WRITE or DEPTH_STENCIL_ATTACHMENT_WRITE. Even after point 1, any path where a draw's attachment aliases the import (directly or through a proxy) needs those in srcAccessMask. ALL_COMMANDS as the source stage is fine.
Minor:
vertexInputBehindGpuWriteis set before the label and copied-writer checks that can still returnnullptr. The draw then emits the barrier for an input that was copied rather than bound. That is harmless but adds a barrier, and it would be clearer to set the flag only on success.- The
perf(/Unblocksevidence (42 → 60 fps on the Minecraft home screen) would be stronger as theaps5-summary.jsonllines ofmainand of the change, withoutAPS5_PROFILE_DRAW, per the Performance report form. The[hooksync]counts are good supporting detail.
|
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.
|
b1e0675 to
92d7697
Compare
|
Pushed 92d7697 on current
|
…U writes with a barrier An in-place draw input flushed pending GPU writes over its range on the CPU before the bind, a submit-and-wait of the open batch when a dispatch in it wrote the buffer. The CPU never reads a vertex buffer's bytes, so a vertex input under a pending write is bound in place behind a barrier from shader and transfer writes to vertex attribute reads, recorded before the draw's rendering begins; index inputs keep the flush, since the highest index is read on the CPU. Minecraft generates a vertex buffer with a compute dispatch every frame and drew from it one CPU wait later. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…cover attachment writes
92d7697 to
4339eee
Compare
adriadam10
left a comment
adriadam10
left a comment
There was a problem hiding this comment.
Re-checked 92d7697: agc_driver_vertex_target_range_imported passes now, and the FlushPending for storage images covers CnRJay's first point. Their two remaining points are addressed in 4339eee, pushed on top:
- the vertex-input barrier's
srcAccessMasknow includesCOLOR_ATTACHMENT_WRITEandDEPTH_STENCIL_ATTACHMENT_WRITE; behindGpuWriteis only set when the input is actually bound in place, so a copied input no longer emits the barrier.
Built and ran the libSceAgcDriver tests (only agc_driver_tessellation fails, same as on main); the new cases fail to compile without the change. Merging once CI is green. The Minecraft numbers are not verified on our side.
What
A draw's vertex buffers bound in place from a host import no longer wait on the CPU for pending GPU writes over them.
InPlaceDrawInputflushed GPU writes before every in-place input; for a vertex buffer the CPU never reads those bytes, so when a write recorded in the open or in-flight batches overlaps the range, the draw now records aVK_PIPELINE_STAGE_ALL_COMMANDS_BIT -> VK_PIPELINE_STAGE_VERTEX_INPUT_BITbarrier (shader and transfer writes to vertex attribute reads) before its rendering begins, and binds the import. Index buffers keep the flush:InPlaceIndexRangescans them on the CPU for the highest index. Labels, queued stores and copied writers over the range still take the copy path.Minecraft (PPSA17221) generates a 48-byte vertex buffer with a compute dispatch every frame and draws from it next; the flush turned that into one submit-and-wait of the open batch per frame (
[hooksync]underAPS5_PROFILE_DRAW=1: about 430 pending-write syncs per 10 s, 2.2 s waited, every one reading unchanged bytes afterwards). With the barrier the home screen goes from 42 to 60 fps on Intel Arc Meteor Lake and the report drops to 30 to 50 syncs per 10 s, none on an open batch.Intended behaviour: a vertex range under a pending GPU write is written back by the GPU (pending storage images are flushed first, as the old CPU flush did) and the draw is ordered behind it by the barrier, instead of waiting for the batch on the CPU. The write-back's own storage-upload read therefore stays pending next to the draw's input read, so
agc_driver_vertex_target_range_importedaccepts either kind for the first read it finds and keeps the copied case (no read at all) strict; the draw consuming the GPU-produced offsets (check) is the proof the barrier orders it.Tested
Fedora 44 (kernel 7.2.8, GCC 16.2.1, Ninja, Release,
BUILD_TESTING=ON, Intel Arc Meteor Lake, Mesa 26.2.3), worktree onmain7663bc6:agc_driver_vertex_target_range_imported,agc_driver_vertex_target_range_copied,agc_driver_tessellation(it passes on this base),agc_recorder_kept_bytes,agc_recorder_read_list,agc_pipeline_cache,agc_pipeline_libraryandagc_pipeline_link_optimizationpass with the new cases (a vertex input free of pending writes is not marked, one under a pending write is bound in place and marked, one under a queued label store still refuses);tools/check_conventions.py --base origin/mainandgit diff --checkpass. Minecraft home screen (a performance change, so the trace form applies, which I have not captured yet; the figures below come from the recorder's[hooksync]report withAPS5_PROFILE_DRAW=1, not fromAPS5_TRACE_FRAMES): 42 to 60 fps on this device, with the recorder report dropping from about 430 pending-write syncs per 10 s to 30 to 50, none on an open batch. Not tested on Windows or NVIDIA.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)InPlaceDrawInput, is closed)🤖 Generated with Claude Code