Repository navigation
feat(agc): trace shader failures through draw submission batches - #5431
sktharunbalaji13-cmd wants to merge 1 commit into
Conversation
|
I inspected the failing conventions run. It reports 39 errors: inline comments in I also compared this with #5238. Both PRs change |
|
Thank you for the review. The conventions findings are addressed in commit On composition with #5238: they are intended to be complementary. PR #5238 gives per-draw shader address information (stage, role, code/entry address, queue, submission context), which helps identify the selected program. This PR assigns a correlation token to each draw attempt and tracks whether the draw reaches selection, actual command recording, submission attempt, and successful queue submission; it also associates recorded draws with a deduplicated set of tokens per recorder batch. In practice, #5238 helps answer "which shader was selected?" while this PR helps answer "did that draw attempt survive setup and get recorded/submitted?" Both PRs touch the same No runtime/game validation is claimed yet, and the platform build jobs remain skipped for this PR. |
|
Thanks, that addresses both points. The distinction between identifying the selected shader and tracking a draw attempt through recording/submission is clear, and your note about reconciling the shared Draw hooks if #5238 lands first gives a workable integration plan. I verified the new conventions check passes; the platform build jobs remain skipped for this path. No runtime validation is claimed. |
CnRJay
left a comment
There was a problem hiding this comment.
Checked the cost with the trace off: the token is 0 and NoteDrawTraceToken returns at once, so the disabled path stays cheap. Two points about the enabled path and the scope:
- The dedup in
NoteDrawTraceTokenis quadratic per batch. It runsstd::findoveropen->drawTraceTokensfor every recorded draw. A batch holding a few thousand draws (common in a busy frame) then does millions of comparisons on the queue worker, under the recorder's open-batch state, while the trace measures that same worker. That distorts the timings the correlation is meant to explain. The tokens come from a counter, so a draw's commands are recorded contiguously and the token can only repeat back to back. Comparing againstback()gives the same dedup in O(1). If tokens can interleave, anunordered_setbeside the vector also works. - The draw-path refactor would be better split out. Moving per-stage materialization into
Driver::materializeDrawStage(27 parameters, several of them out-parameters) changes the hottest function in the driver as part of a diagnostics PR. It may well be behaviour-neutral, but reviewers have to verify that across ~200 lines, and it conflicts with every other open PR that touchesDriver::draw(#5238 already). Keeping the token as one extra argument through the existing code, and leaving the restructure to its ownrefactor(agc)PR, would make both easier to review.
Minor: the catch (...) {} in NoteDrawTraceToken hides allocation failures. In a diagnostics path, losing a token silently makes the trace look complete when it isn't. Letting it throw, or counting the drops in the trace, is safer.
|
Thank you for the detailed review. I’ve addressed the three points in commit A. Token deduplication
B. Scope of the draw-path changeThe PR mainly adds parameters rather than introducing a broad new draw refactor. The existing C. Silent allocation failuresThe previous empty catch now emits Validation
#5238 overlapI compared the overlap with PR #5238. It remains complementary: #5238 exposes per-draw shader address/context details, while this PR adds token lifecycle and submission-batch correlation. If #5238 lands first, this PR should adapt to its address formatting rather than duplicating it. No changes were made to #5238's branch. No claim of GPU/game-runtime validation is made. |
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the quick follow-ups on the dedup and the empty catch. Blocking points:
- Conflicts with
mainin code, not just adjacency:Execution/src/Driver/Draw/Draw.cppandDraw/DrawCapture.cpp(main's 73a808e changed how draw stages use their shared compiled result).Driver::materializeDrawStagehas to be redone on top of that code, and the draw tests rerun after the rebase. - Split the refactor out (as @CnRJay asked): moving per-stage materialization into
Driver::materializeDrawStage(27 parameters, several out-parameters) rewrites the hottest function in the driver, and it is the part that conflicts with #5238, #5364, #5441, #5609 and #5656. Send that as its ownrefactor(agc)PR that is behaviour-neutral and has its own tests; then keep this PR to the token passed as one extra argument through the existing code. - Evidence for the claim: "Unblocks PPSA02225 #4461" is not backed by anything in the PR. Show a trace from that title that reveals a failure that is not visible today, or drop the claim.
Welcome back once it is rebased and the refactor is separated.
2c3502d to
1dcebc2
Compare
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the rebase. Re-checked on the head merged onto current main; the conflict and the materializeDrawStage split points are resolved (main already has that function). Blocking points left:
- The PR's own tests fail (build + ctest of
libSceAgcDriveron the merged tree):agc_driver_draw_shader_failure("the capture trace received no draw-shader-request-failed record", log showsunsupported SOP1 opcode 2),agc_driver_draw_shader_failure_successful, andagc_driver_draw_shader_failure_request_correlation_disabled("driver shutdown failed unexpectedly"). Theagc_capture_trace_*tests pass. Please run them against a rebase on currentmainand fix whatever changed under them. - Evidence for the claim: "Unblocks PPSA02225 #4461" is still not backed by anything in the PR. Show a trace from that title that reveals a failure that is not visible today, or drop the claim.
Welcome back once the tests pass on the rebased head.
1dcebc2 to
8b73f50
Compare
|
Rebased cleanly on current \main\ (\80249ea54) and resolved all conflicts. All 14 unit tests in \libSceAgcDriver\ now pass 100%:
Test fixture adjustments:
|
adriadam10
left a comment
There was a problem hiding this comment.
Re-checked the new head against our last comment: the tests point and the PPSA02225 claim are both resolved (claim dropped), and conventions pass. One blocking point left:
DrawPipeline.cpp:78:if (stopped) return;inEnqueueis unrelated to tracing and hides a fatal failure. Once a commit fails fatally the worker setsstopped = trueand storesfailure; before this change the nextEnqueuecalledrethrowFailure()and surfaced it to the submitting packet. With the early return, draws are dropped silently andfailureis never rethrown. Please remove it. If a test needs a clean shutdown after a fatal failure, make the test handle that (or send it as a separate PR with a test that fails without it).- Minor:
Lifecycle.cpp:10adds an include ofDrawPipeline.hppthatLifecycle.cpp:5already has; drop it.
Welcome back once the early return is gone and the tests still pass.
8b73f50 to
2251c2e
Compare
2251c2e to
a5cdfa9
Compare
adriadam10
left a comment
There was a problem hiding this comment.
The merge queue's Linux Build run on the current head (run 39157472408, PR merged onto main) fails four of this PR's own tests, deterministically:
agc_driver_draw_shader_failure_successful,agc_driver_draw_shader_failure_correlation_disabled,agc_driver_draw_shader_failure_materialization,agc_driver_draw_shader_failure_materialization_correlation_disabled.- The log shows
[gpu] driver failure: AGC driver: guest memory has no write permission(the colour-target writability check atGraphics/src/Draw.cpp:2566) andAGC graphics: the submission failure lost the original shader-request error. The first test and therequest_correlation_disabledone pass, so it looks tied to submitting further draws into the sameColorMemoryafter an earlier one in the process.
Please rebase on current main, run ctest -R agc_driver_draw_shader_failure (Linux/lavapipe if you can, or check the Windows run too) and make the tests independent of each other, e.g. a fresh writable colour buffer per case, or find why the buffer loses write permission. (agc_driver_depth_bias_per_face also fails in that run but is not caused by this PR.)
a5cdfa9 to
7c2f861
Compare
|
Rebased on current \main\ (\676d423968) and resolved the test failures:
|
What
Add opt-in diagnostic correlation across draw attempts, shader selection and preparation failures, actual draw-command recording, and recorder batch submission in the AGC driver.
Each draw attempt receives a correlation token. Relevant failure and recording events preserve that identity. Recorder batches collect a deduplicated set of the tokens associated with commands recorded in the batch, allowing one batch to represent multiple draws. Submission-attempt diagnostics are distinguished from confirmation records emitted only after successful queue submission and finalized required recorder bookkeeping. Diagnostic formatting and logging are bounded and best-effort to prevent trace failures from masking original exceptions or blocking otherwise valid submission.
Tested
Clean PR branch \eat/shader-draw-submission-correlation-5166-fresh, rebased on current \main\ (\80249ea54).
Checklist