Repository navigation
drm/asahi: g17: #413 review follow-ups (no replay after blame, no Spare behind failed work) - #418
Merged
Conversation
settle_blamed_render() keeps the firmware-blamed queue's oldest group spared when an earlier recovery spared it, so that its failure stays non-sticky. fail_render_packet() then tries replay_abandoned(), which counted the group as innocent because it was not classified guilty. A group aborted by a recovery carries the same abandonment witness as an innocent group the firmware dropped, so the blamed work was published again, up to MAX_REPLAYS times, and each replay could set off another recovery. The class cannot carry the blame on its own: the earlier recovery's spare has to stand, and quarantine() can later extend a spared class to the rest of the pair. Record the blame on the completion instead, apart from the class and never cleared, set it in settle_blamed_render() whatever class the group keeps, and make the replay decision refuse any set that holds a blamed group. The error class is unchanged, so blamed work an earlier recovery spared still fails without a sticky device loss. KUnit covers a blamed group at the front, behind an innocent group and at the replay limit, where an innocent front alone would be replayed. Assisted-by: LLM Signed-off-by: Ryan Murray <ryan@aurorasilicon.org>
Pair::timeout_head() had two first lines of documentation, an older one and its replacement; keep one. replay_abandoned() said the firmware drops an innocent pair's queued groups unexecuted after another VM's recovery on 25G83. The same was seen on the A18 Pro's 26A428 firmware when the replay was first added (b9fd334, "drm/asahi: g17: replay render groups firmware retired without executing"), so name both. Also note what the single witness sample of the timeout and settlement paths can cost: a sample can straddle a firmware update of the fragment end record, and a group that did execute then runs a second time. The behaviour is unchanged. Assisted-by: LLM Signed-off-by: Ryan Murray <ryan@aurorasilicon.org>
classify() assumes the culprit is the oldest unfinished work. That does not hold when an innocent frame F was published before the culprit C and stalls behind it: F times out first and, being oldest, fails as its own fault. F's quarantined pair keeps its timeout head until the firmware drains it, so C then defers MAX_RENEWALS times, about 16 s, and is classified Spared: its failure is not sticky and its client is not torn down. Before render timeouts had a grace, C failed after about 2 s. Every compositor has a render head, so on the A18 Pro this covers most render and compute hangs, and a quarantined pair that never drains does the same to every later timeout. Mark a head whose queue has already failed (a quarantined render pair, or a quarantined compute queue still awaiting its witness), and split classify() into blocker(), which prefers a live blocker to a failed one, and decide(). A timeout behind another VM's older work still defers for the whole grace, so that a failed culprit's teardown can free the GPU. Once the grace runs out it is spared only behind a live blocker, which its own watchdog still settles; behind failed work only it is its own. This also applies to compute, which weighs render heads and its own failed queues the same way: compute work still blocked after the grace behind queues that already failed now fails as its own instead of being spared, as a render did before the grace existed. KUnit covers a failed older blocker (defer, then own, at any renewal count past the grace), a pair that never drains, a live blocker next to failed ones, and the blocker filter. Assisted-by: LLM Signed-off-by: Ryan Murray <ryan@aurorasilicon.org>
The render and compute watchdog lines said a timeout was "behind earlier work of another VM" without naming that work, and the render line also lacked the publication= field the compute line has. Neither showed whether a decision rested on a live blocker or on one that had already failed, which is what the decision now depends on. Return the blocker with the decision (timeout::attribute()) and print the VM and publication of both the timed-out work and its blocker, and whether the blocker had already failed. The line is printed at the first deferral, as before, and once the grace has run out, now also when that ends in Own behind failed work. VMs were compared by the address of their status, which cannot be logged. Compare them by mmu::Vm::id() instead, which a G17 build now compiles too: the status is created with its VM, a context takes it from its VM and a completion from its context, so the two identify the same VMs. The new line reads: Render watchdog owner=12 vm=5 publication=345 renewals=0 decision=Defer behind earlier work of another VM: blocker_vm=7 blocker_publication=340 blocker_failed=0 KUnit checks which verdicts name a blocker. Assisted-by: LLM Signed-off-by: Ryan Murray <ryan@aurorasilicon.org>
… no proof When fail_after_observation() cannot replay a pair's dropped groups it falls back to retiring an abandoned spared front, with its consecutive abandoned spared successors. That loop took retirement::abandoned() with "?", so a snapshot whose cursors straddled a firmware update returned its error through timeout_render() to stop_after_error() and lose_device_quietly(), and every client lost the device. replay_abandoned() already reads that error as no proof, and the ordinary retirement poll retries it. Do the same here, for the front and its successors: the loop stops, groups already retired stay retired, and if none was the pair is quarantined as on any other timeout. Errors from reading the snapshot, the stamps or the recovery state still propagate, as they do in replay_abandoned(). Assisted-by: LLM Signed-off-by: Ryan Murray <ryan@aurorasilicon.org>
…play When the front of a pair's dropped groups has already been republished MAX_REPLAYS times, replay_abandoned() failed that front with EIO and returned, leaving its successors active. They were then replayed on their own, after the failed front, and consumed output it never wrote, while the commit that added the multi-group replay says that a failed replay fails the parked successors with it. Fail them with the front instead. replay::decide() already allows the limit only on the front and every group carries the abandonment witness, so each successor is retired with the same accounting and error as the front and none is parked. Log the limit as such rather than as a replay. Assisted-by: LLM Signed-off-by: Ryan Murray <ryan@aurorasilicon.org>
…imeout The compute watchdog runs scan_compute() before it classifies a timeout, so a head that finished but whose completion the event worker has not processed yet no longer counts as a blocker. The render watchdog classified without such a pass, and render heads never report themselves finished, so a finished but unprocessed group of another VM could defer a render timeout, and, if its completion stayed unprocessed for the whole grace, spare it. Add scan_renders(), which retires the finished front groups of every pair the registry holds, with the in-lock retirement, collection and terminal settlement that the timeout and VM settlement paths already use, and run it before the render timeout is attributed. Pairs lent out for preparation, growth or observation, and pairs holding a deferred timeout or error, are left to the event worker, which settles them in its own order; nothing is retired while a recovery is pending. A packet the pass retires has its retirement queued, which both timeout paths already treat as settled. The collected callbacks run after the device mutex is released, as in compute, and if no collector can be allocated the attribution goes ahead without the pass, as before. Assisted-by: LLM Signed-off-by: Ryan Murray <ryan@aurorasilicon.org>
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.
Follow-ups to #413 (jgoyvaerts, keep innocent clients alive across another client's GPU stall or recovery, merged as 2a868cf), from our review of it.
settle_blamed_render()and refused by the replay decision, whatever error class the packet keeps.Sparebehind work that already failed. A head now records whether its queue is quarantined.classify()is split intoblocker(), which prefers a live blocker, anddecide(). Behind only failed work, a timeout defers for the grace and then getsOwn. This covers renders and compute. Trade-off: an innocent client stuck behind a culprit that has already failed and never drains fails asOwnafter the grace, as before drm/asahi: g17: keep innocent clients alive across another client's GPU stall or recovery #413.vm=andpublication=for the timed-out work andblocker_vm=,blocker_publication=,blocker_failed=for the blocker. VMs are compared bymmu::Vm::id()instead of a pointer.Testing:
🤖 Generated with Claude Code