drm/asahi: g17: #413 review follow-ups (no replay after blame, no Spare behind failed work)

drivers/gpu/drm/asahi/g17/completion.rs

@@ -264,6 +264,9 @@ pub(crate) struct Completion {
     timeout_error: AtomicI32,
     replays: AtomicU8,
     replay_timeout: AtomicBool,
+    /// A recovery blamed this command. Unlike `class`, which a later
+    /// classification can change, this never clears.
+    blamed: AtomicBool,
     firmware_consumer: AtomicBool,
     class: AtomicU8,
 }
@@ -318,6 +321,7 @@ impl Completion {
                 timeout_error: AtomicI32::new(0),
                 replays: AtomicU8::new(0),
                 replay_timeout: AtomicBool::new(false),
+                blamed: AtomicBool::new(false),
                 firmware_consumer: AtomicBool::new(false),
                 class: AtomicU8::new(0),
             }),
@@ -471,6 +475,16 @@ impl Completion {
         self.class.load(Ordering::Acquire) == 2
     }
 
+    /// A recovery blamed this command, whatever class it keeps: it may have caused the fault,
+    /// so it is never published again.
+    pub(crate) fn note_blamed(&self) {
+        self.blamed.store(true, Ordering::Release);
+    }
+
+    pub(crate) fn blamed(&self) -> bool {
+        self.blamed.load(Ordering::Acquire)
+    }
+
     /// Installed before the first firmware-visible prefix. Cancellation takes
     /// the same lock after claiming the signal, so it cannot miss a late lease.
     pub(crate) fn attach_render_lease(

drivers/gpu/drm/asahi/g17/queue/compute.rs

@@ -618,9 +618,10 @@ impl Queue {
             return None;
         }
         self.active.front().map(|active| crate::g17::timeout::Head {
-            vm: Arc::as_ptr(active.packet.completion.status()) as usize,
+            vm: active.packet.context.vm().id(),
             published: active.published,
             finished: active.observed.is_some(),
+            failed: self.quarantined,
         })
     }
 

drivers/gpu/drm/asahi/g17/queue/render.rs

@@ -966,18 +966,19 @@ impl Pair {
                 .all(|qid| masks[usize::from(qid) / 64] & (1u64 << (qid % 64)) != 0)
         })
     }
-    /// The oldest published group, for cross-VM timeout attribution (crate::g17::timeout).
-    /// The oldest published group still running. A quarantined pair's work holds the GPU until
-    /// the firmware drains it, so it stays visible to other VMs' watchdogs, as compute does.
+    /// The oldest published group still running, for cross-VM timeout attribution
+    /// (crate::g17::timeout). A quarantined pair's work holds the GPU until the firmware drains
+    /// it, so it stays visible to other VMs' watchdogs, as compute does, marked as failed.
     pub(crate) fn timeout_head(&self) -> Option<crate::g17::timeout::Head> {
         self.active
             .iter()
             .filter(|active| active.outer_published)
             .find(|active| active.drained_result.is_none())
             .map(|active| crate::g17::timeout::Head {
-                vm: Arc::as_ptr(active.packet.completion.status()) as usize,
+                vm: active.packet.context.vm().id(),
                 published: active.published,
                 finished: false,
+                failed: self.quarantined,
             })
     }
 
@@ -1199,13 +1200,16 @@ impl Pair {
 
     /// Every retained group was retired and unlinked by firmware without
     /// executing (job list empty, stamps cover it, no recovery, fragment end
-    /// never written): after another VM's recovery the 25G83 firmware drops an
-    /// innocent pair's queued groups this way. Nothing of them reached their
-    /// attachments, so the same commands are published again, in order, instead
-    /// of failing their owners. Only the whole set is replayed; a replayed prefix
-    /// would run after a live successor. `grace` asks for the strict witness
-    /// twice ABANDON_GRACE_NS apart (worker polling); the render timeout, long
-    /// after publication, takes it once as its retirement does.
+    /// never written): after another VM's recovery the firmware drops an
+    /// innocent pair's queued groups this way (seen on 25G83 and 26A428).
+    /// Nothing of them reached their attachments, so the same commands are
+    /// published again, in order, instead of failing their owners. Only the
+    /// whole set is replayed; a replayed prefix would run after a live
+    /// successor. `grace` asks for the strict witness twice ABANDON_GRACE_NS
+    /// apart (worker polling); the render timeout, long after publication,
+    /// takes it once as its retirement does. A single sample can still
+    /// straddle a firmware update of the fragment end record; the cost is
+    /// that a group which did execute runs a second time (blending twice).
     fn replay_abandoned(
         &mut self,
         host: &mut impl Host,
@@ -1223,10 +1227,12 @@ impl Pair {
         let groups = self.active.iter().map(|active| {
             let innocent =
                 active.outer_published && !active.failed && !active.packet.completion.guilty();
+            let blamed = active.packet.completion.blamed();
             let exhausted =
                 active.packet.completion.replay_count() >= crate::g17::completion::MAX_REPLAYS;
             // decide() stops at the first group that is not abandoned, an error included.
             let abandoned = innocent
+                && !blamed
                 && (|| -> Result<bool> {
                     let mut observer = Observer {
                         memory,
@@ -1249,6 +1255,7 @@ impl Pair {
                 });
             Group {
                 innocent,
+                blamed,
                 abandoned,
                 exhausted,
             }
@@ -1276,7 +1283,18 @@ impl Pair {
                 return Ok(false);
             }
         }
-        if !grace || crate::debug::debug_enabled(crate::debug::DebugFlags::VerboseFaults) {
+        // decide() lets only the front be exhausted. An exhausted front fails, and its
+        // successors, which consume its output, fail with it rather than run without it.
+        let mut failing = self.active.front().is_some_and(|front| {
+            front.packet.completion.replay_count() >= crate::g17::completion::MAX_REPLAYS
+        });
+        if failing {
+            pr_warn!(
+                "G17: render slot {}: {} groups retired without execution; replay limit reached, failing them\n",
+                self.slot,
+                self.active.len()
+            );
+        } else if !grace || crate::debug::debug_enabled(crate::debug::DebugFlags::VerboseFaults) {
             pr_warn!(
                 "G17: render slot {}: {} groups retired without execution; replaying\n",
                 self.slot,
@@ -1301,7 +1319,8 @@ impl Pair {
             }
             let ordinal = front.ticket.ordinal;
             let packet = front.packet.clone();
-            if !packet.completion.begin_replay() {
+            failing = failing || !packet.completion.begin_replay();
+            if failing {
                 if !packet.completion.spared() {
                     packet.completion.status().record(EIO);
                     if self.terminal.is_none() {
@@ -1311,7 +1330,7 @@ impl Pair {
                 defer(Deferred::Retired(packet.completion.clone(), Err(EIO)))?;
                 self.memory.retire(ordinal)?;
                 self.active.pop().ok_or(EIO)?;
-                return Ok(true);
+                continue;
             }
             self.memory.retire(ordinal)?;
             self.active.pop().ok_or(EIO)?;
@@ -1484,8 +1503,14 @@ impl Pair {
                 let snapshot = observer.snapshot(active.ticket.ordinal)?;
                 let stamps = observer.stamps()?;
                 let (_, state) = observer.host.epoch()?;
+                // A torn sample is no proof, as in replay_abandoned(): the group stays retained
+                // (quarantined with the pair if nothing was retired) instead of the error
+                // stopping the runtime for every client.
                 if !active.outer_published
-                    || !retirement::abandoned(&active.ticket, snapshot, stamps, state, false)?
+                    || !matches!(
+                        retirement::abandoned(&active.ticket, snapshot, stamps, state, false),
+                        Ok(true)
+                    )
                 {
                     break;
                 }

drivers/gpu/drm/asahi/g17/replay.rs

@@ -7,8 +7,11 @@
 /// One retained group, front first.
 #[derive(Clone, Copy, Debug, PartialEq, Eq)]
 pub(crate) struct Group {
-    /// Published, not failed and not blamed (guilty) by a recovery.
+    /// Published, not failed and not classified guilty by a recovery.
     pub(crate) innocent: bool,
+    /// A recovery blamed it, whatever class it kept (an earlier recovery may have spared it):
+    /// it may have caused the fault.
+    pub(crate) blamed: bool,
     /// Carries the strict abandonment witness.
     pub(crate) abandoned: bool,
     /// Already republished the maximum number of times.
@@ -25,13 +28,13 @@ pub(crate) enum Decision {
 }
 
 /// Groups are read in order and the first one that decides stops the walk, so a caller can
-/// observe the abandonment lazily. An exhausted front is replayed: the replay fails it alone.
-/// An exhausted successor would break the order.
+/// observe the abandonment lazily. An exhausted front is replayed: the replay fails it, and its
+/// successors, which consume its output, with it. An exhausted successor would break the order.
 pub(crate) fn decide(groups: impl Iterator<Item = Group>) -> Decision {
     let mut front_exhausted = None;
     let mut successor_exhausted = false;
     for group in groups {
-        if !group.innocent {
+        if !group.innocent || group.blamed {
             return Decision::Refuse;
         }
         if !group.abandoned {
@@ -53,9 +56,10 @@ pub(crate) fn decide(groups: impl Iterator<Item = Group>) -> Decision {
 mod tests {
     use super::*;
     fn group(innocent: bool, abandoned: bool, exhausted: bool) -> Group {
-        Group { innocent, abandoned, exhausted }
+        Group { innocent, blamed: false, abandoned, exhausted }
     }
-    const OK: Group = Group { innocent: true, abandoned: true, exhausted: false };
+    const OK: Group = Group { innocent: true, blamed: false, abandoned: true, exhausted: false };
+    const BLAMED: Group = Group { innocent: true, blamed: true, abandoned: true, exhausted: false };
     #[test]
     fn every_group_abandoned_and_innocent_is_replayed() {
         assert_eq!(decide([OK].into_iter()), Decision::Replay);
@@ -71,6 +75,16 @@ mod tests {
         assert_eq!(decide([OK, group(false, true, false)].into_iter()), Decision::Refuse);
     }
     #[test]
+    fn blamed_work_is_never_replayed_even_when_spared() {
+        // An earlier recovery spared it, so it still counts as innocent; the blame refuses it.
+        assert_eq!(decide([BLAMED].into_iter()), Decision::Refuse);
+        assert_eq!(decide([BLAMED, OK].into_iter()), Decision::Refuse);
+        assert_eq!(decide([OK, BLAMED].into_iter()), Decision::Refuse);
+        // Also once the replay limit is reached, where an innocent front alone is replayed.
+        let exhausted = Group { exhausted: true, ..BLAMED };
+        assert_eq!(decide([exhausted].into_iter()), Decision::Refuse);
+    }
+    #[test]
     fn a_live_group_waits_and_stops_the_walk() {
         assert_eq!(decide([OK, group(true, false, false)].into_iter()), Decision::Wait);
         // Nothing after the live group is looked at, guilty or not.

drivers/gpu/drm/asahi/g17/runtime/backend.rs

@@ -231,29 +231,46 @@ impl job::Backend for Backend {
             return self.defer_compute_timeout(packet, renewals);
         }
         use crate::g17::timeout::{self, Decision};
-        let mut state = self.shared.state.lock();
-        let Some(firmware) = (*state).as_deref_mut() else {
-            return false;
-        };
-        // Borrowed-graph and queued-retirement deferrals share the renewal bound.
-        if timeout::transaction_grace(renewals)
-            && firmware.queues.defer_render_timeout(packet, ETIMEDOUT)
-        {
-            return true;
-        }
-        let decision = firmware.queues.render_timeout_decision(packet, renewals);
-        // As compute does: work still blocked behind another VM's older unfinished work after the
-        // whole grace is innocent, so its failure must stay non-sticky. The pair's quarantine
-        // extends the class to the pair's other groups.
-        if decision == Decision::Spare {
-            packet.completion.classify(crate::g17::recovery::Class::Spared);
-        }
-        if decision != Decision::Own && (renewals == 0 || decision == Decision::Spare) {
-            dev_warn!(self.shared.dev.as_ref(),
-                "Render watchdog owner={} renewals={} decision={:?} behind earlier work of another VM\n",
-                self.owner, renewals, decision);
+        // As for compute, a final retirement pass precedes the attribution, and all completion
+        // callbacks run after releasing the device mutex. Without a collector the attribution
+        // goes ahead without that pass, as it did before it existed.
+        let mut deferred = DeferredBatch::worker().ok();
+        let result = (|| -> Result<bool> {
+            let mut state = self.shared.state.lock();
+            let Some(firmware) = (*state).as_deref_mut() else {
+                return Ok(false);
+            };
+            if let Some(deferred) = deferred.as_mut() {
+                firmware.scan_renders(deferred)?;
+            }
+            // Borrowed-graph and queued-retirement deferrals share the renewal bound.
+            if timeout::transaction_grace(renewals)
+                && firmware.queues.defer_render_timeout(packet, ETIMEDOUT)
+            {
+                return Ok(true);
+            }
+            let verdict = firmware.queues.render_timeout_decision(packet, renewals);
+            let decision = verdict.map_or(Decision::Own, |verdict| verdict.decision);
+            // As compute does: work still blocked behind another VM's older unfinished work
+            // after the whole grace is innocent, so its failure must stay non-sticky. The pair's
+            // quarantine extends the class to the pair's other groups.
+            if decision == Decision::Spare {
+                packet.completion.classify(crate::g17::recovery::Class::Spared);
+            }
+            if let Some(verdict) = verdict {
+                if let Some(blocker) = verdict.reported_blocker(renewals) {
+                    dev_warn!(self.shared.dev.as_ref(),
+                        "Render watchdog owner={} vm={} publication={} renewals={} decision={:?} behind earlier work of another VM: blocker_vm={} blocker_publication={} blocker_failed={}\n",
+                        self.owner, verdict.own.vm, verdict.own.published, renewals, decision,
+                        blocker.vm, blocker.published, u8::from(blocker.failed));
+                }
+            }
+            Ok(decision == Decision::Defer)
+        })();
+        if let Some(deferred) = deferred.as_mut() {
+            deferred.finish();
         }
-        decision == Decision::Defer
+        matches!(result, Ok(true))
     }
 
     fn try_early(&self, packet: &Arc<Packet>) -> bool {

drivers/gpu/drm/asahi/g17/runtime/compute_control.rs

@@ -32,15 +32,17 @@ impl Backend {
                 .filter(|other| other.owner() != Some(self.owner))
                 .filter_map(|other| other.timeout_head());
             let others = Iterator::chain(computes, firmware.queues.render_timeout_heads());
-            let decision = timeout::classify(own, renewals, others);
+            let verdict = timeout::attribute(own, renewals, others);
+            let decision = verdict.decision;
             if decision == Decision::Spare {
                 firmware.queues.compute(self.owner).ok_or(EIO)?
                     .classify(&|_| Class::Spared);
             }
-            if decision != Decision::Own && (renewals == 0 || decision == Decision::Spare) {
+            if let Some(blocker) = verdict.reported_blocker(renewals) {
                 dev_warn!(self.shared.dev.as_ref(),
-                    "Compute watchdog owner={} publication={} renewals={} decision={:?} behind earlier work of another VM\n",
-                    self.owner, own.published, renewals, decision);
+                    "Compute watchdog owner={} vm={} publication={} renewals={} decision={:?} behind earlier work of another VM: blocker_vm={} blocker_publication={} blocker_failed={}\n",
+                    self.owner, own.vm, own.published, renewals, decision,
+                    blocker.vm, blocker.published, u8::from(blocker.failed));
             }
             Ok(decision)
         })();

drivers/gpu/drm/asahi/g17/runtime/render.rs

@@ -683,6 +683,44 @@ impl crate::g17::Firmware {
         Ok(())
     }
 
+    /// A final retirement pass before a render watchdog attributes its timeout, as compute runs
+    /// scan_compute() first: a head that finished but whose completion the event worker has not
+    /// processed yet must not defer another VM's timeout. Pairs lent out, or holding a deferred
+    /// timeout or error, stay with the worker, which observes and settles them in its order.
+    pub(in crate::g17::runtime) fn scan_renders(&mut self, deferred: &mut DeferredBatch) -> Result {
+        if self.recovery.pending()
+            || self
+                .primary
+                .state
+                .shared
+                .crashed
+                .load(core::sync::atomic::Ordering::Acquire)
+        {
+            return Ok(());
+        }
+        for slot in 1..self.queues.render.entries.len() {
+            if !self.queues.render.entries[slot]
+                .as_ref()
+                .is_some_and(|entry| {
+                    entry
+                        .pair
+                        .as_ref()
+                        .is_some_and(|pair| pair.published_in_flight())
+                        && entry.preparing.is_none()
+                        && !entry.growing
+                        && !entry.pending_timeout
+                        && entry.pending_error.is_none()
+                })
+            {
+                continue;
+            }
+            self.retire_render_slot(slot, None, deferred)?;
+            self.queues.render.collect_retired(slot)?;
+            self.settle_render_terminal(slot, deferred)?;
+        }
+        Ok(())
+    }
+
     pub(in crate::g17) fn settle_render_vm(
         &mut self,
         status: &Arc<crate::g17::status::VmStatus>,
@@ -978,7 +1016,7 @@ impl Registry {
         &self,
         packet: &Arc<Packet>,
         renewals: u8,
-    ) -> crate::g17::timeout::Decision {
+    ) -> Option<crate::g17::timeout::Verdict> {
         let own = self
             .render
             .entries
@@ -993,14 +1031,14 @@ impl Registry {
             .flatten()
             .filter_map(|entry| entry.queue.as_deref())
             .filter_map(|queue| queue.timeout_head());
-        match own {
-            Some(own) => crate::g17::timeout::classify(
+        // Without a head of its own the timeout is its own.
+        own.map(|own| {
+            crate::g17::timeout::attribute(
                 own,
                 renewals,
                 Iterator::chain(self.render_timeout_heads(), computes),
-            ),
-            None => crate::g17::timeout::Decision::Own,
-        }
+            )
+        })
     }
 }