From ec38a64c6b2b8314f2366d88a8be7c702fd0105b Mon Sep 17 00:00:00 2001 From: Jonathan Goyvaerts Date: Fri, 23 Oct 2026 17:55:55 -0300 Subject: [PATCH 1/9] drm/asahi: name the client in failure reports "GPU work of a client failed" did not say which client: on a desktop with a browser, a game under Wine and a compositor, 31 such lines in one boot could not be attributed. Record the creating process (tgid and comm) with the VM status and print it. The message keeps its prefix, which tooling matches. Signed-off-by: Jonathan Goyvaerts Assisted-by: LLM --- drivers/gpu/drm/asahi/g17/status.rs | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/asahi/g17/status.rs b/drivers/gpu/drm/asahi/g17/status.rs index a065c5030b1b8..4d9cd28cc430a 100644 --- a/drivers/gpu/drm/asahi/g17/status.rs +++ b/drivers/gpu/drm/asahi/g17/status.rs @@ -5,7 +5,7 @@ use crate::debug::*; use crate::gem; use core::sync::atomic::{AtomicBool, AtomicI32, Ordering}; -use kernel::{new_mutex, prelude::*, sync::Mutex}; +use kernel::{new_mutex, prelude::*, str::BStr, sync::Mutex, task::Pid}; const DEBUG_CLASS: DebugFlags = DebugFlags::Errors; @@ -22,6 +22,9 @@ pub(crate) struct VmStatus { client_closed: AtomicBool, /// A failure of this VM has been logged as a warning. warned: AtomicBool, + /// The process that created the VM, named in the failure report. + tgid: Pid, + comm: [u8; 16], /// The client's mapped status word, retained with its object only while the /// client's VM handle exists; released by `release_mirror` at VM close. #[pin] @@ -38,12 +41,22 @@ struct Mirror { unsafe impl Send for Mirror {} impl VmStatus { + /// Called from the creating client's ioctl: the VM records who it belongs to. pub(crate) fn new() -> impl PinInit { + let task = kernel::current!(); + // SAFETY: `comm` is a fixed array inside the live current task. A concurrent rename by + // another thread of the process may tear the copy, as with C's unlocked `current->comm`. + let comm: [u8; 16] = unsafe { + core::ptr::read_volatile(core::ptr::addr_of!((*task.as_ptr()).comm).cast()) + }; + let tgid = task.tgid(); pin_init!(Self { error: AtomicI32::new(0), reported_error: AtomicI32::new(0), client_closed: AtomicBool::new(false), warned: AtomicBool::new(false), + tgid, + comm, mirror <- new_mutex!(None, "Asahi VM status mirror"), }) } @@ -98,11 +111,14 @@ impl VmStatus { ); } } else if !self.warned.swap(true, Ordering::AcqRel) { + let len = self.comm.iter().position(|&b| b == 0).unwrap_or(self.comm.len()); pr_warn!( - "GPU work of a client failed ({:?}, reported at {}:{}); the client sees a lost device\n", + "GPU work of a client failed ({:?}, reported at {}:{}, client {}[{}]); the client sees a lost device\n", error, file, - at.line() + at.line(), + BStr::from_bytes(&self.comm[..len]), + self.tgid ); } let error = match first { From 3ff3dc1e03aed9c2efb2545f2190da2b3d4f25af Mon Sep 17 00:00:00 2001 From: Jonathan Goyvaerts Date: Tue, 3 Nov 2026 12:32:20 -0300 Subject: [PATCH 2/9] drm/asahi: g17: blame the running VM, not a starved one, for a keyless timeout On the M4 Pro (25G83) every recovery slot key reads 0, so recovery attribution cannot check the firmware's blame and always keeps it. A fragment shader spinning on a lock then takes down unrelated clients: a restart drops only the guilty pair's started group, its next queued group spins again, and each following recovery 83 ms later blames whichever queue the spinner starved. The driver fails that queue's oldest render with EIO, which poisons its VM. On 11-03 that killed Hyprland, quickshell and T3 Code in two cascades of deqp-vk hangs. In five of the six innocent blames the blamed render had never started (tiler start and fragment end unwritten). When slot identity cannot judge a timeout (not a page fault), read the host's own pass records at the same halted instant: if the blamed pair's oldest published render has not started, and exactly one VM has a published render that started and did not finish, and no other VM holds compute work, blame that VM's queues instead. Groups an earlier recovery listed or blamed are skipped; the restart dropped them, so their records stay half-written. In every other case the firmware's blame stands, as before. A blamed compute queue keeps the firmware's blame: only render pairs have the pass records this reads. This does not stop the guilty pair's queued groups from running after the restart, so a client with many of them can still reach the restart storm limit. Signed-off-by: Jonathan Goyvaerts Assisted-by: LLM --- drivers/gpu/drm/asahi/g17/queue/render.rs | 24 ++++++++++++ drivers/gpu/drm/asahi/g17/recovery.rs | 10 +++++ .../gpu/drm/asahi/g17/recovery/attribution.rs | 39 +++++++++++++++++++ drivers/gpu/drm/asahi/g17/runtime/events.rs | 3 ++ .../drm/asahi/g17/runtime/render/recovery.rs | 30 +++++++++++++- 5 files changed, 104 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/asahi/g17/queue/render.rs b/drivers/gpu/drm/asahi/g17/queue/render.rs index 2436cfd33ed35..b210f55efa2b7 100644 --- a/drivers/gpu/drm/asahi/g17/queue/render.rs +++ b/drivers/gpu/drm/asahi/g17/queue/render.rs @@ -1297,6 +1297,30 @@ impl Pair { Some(observation::snapshot(&self.memory, ordinal).ok()?.pass[0] != 0) } + /// The oldest published group has neither its tiler start nor its fragment end written. + pub(crate) fn front_untouched(&self) -> Option { + let front = self.active.iter().find(|active| active.outer_published)?; + Some(observation::snapshot(&self.memory, front.ticket.ordinal).ok()?.untouched()) + } + + /// The owner of a published group whose tiler started and whose fragment end is unwritten. + /// Groups an earlier recovery listed or blamed are skipped: a restart drops started groups, + /// so their records stay half-written without anything running. + pub(crate) fn running_owner(&self) -> Option<&Arc> { + self.active + .iter() + .filter(|active| { + active.outer_published + && !active.packet.completion.interrupted() + && !active.packet.completion.blamed() + }) + .find(|active| { + observation::snapshot(&self.memory, active.ticket.ordinal) + .is_ok_and(|snapshot| snapshot.pass[0] != 0 && snapshot.pass[1] == 0) + }) + .map(|active| active.packet.completion.status()) + } + /// Firmware is halted and all graph borrowers have joined before this copy. pub(crate) fn publish_recovery_targets(&self) -> Result { self.memory.graph.publish_recovery_targets() diff --git a/drivers/gpu/drm/asahi/g17/recovery.rs b/drivers/gpu/drm/asahi/g17/recovery.rs index ca8da8b03545f..3f5b4c353e165 100644 --- a/drivers/gpu/drm/asahi/g17/recovery.rs +++ b/drivers/gpu/drm/asahi/g17/recovery.rs @@ -92,6 +92,11 @@ pub(super) enum Class { pub(super) trait Queues { fn render_scheduler_active(&self) -> bool; fn render_pass_started(&self, qid: u8, stamp: u64) -> Option; + /// Read while firmware is halted, like `render_pass_started`. When the oldest published + /// render of the pair holding `blamed` had not started, the QIDs of every pair of the one VM + /// with a render that started and did not finish. None if no such VM, several, or any other + /// VM holds compute work, which these records cannot see. + fn render_culprit(&self, _blamed: u8) -> Option { None } /// Copy every retained queue's live producer to its recovery target, then /// issue a full barrier. A failed graph must not skip subsequent queues. fn publish_recovery_targets(&self) -> Option; @@ -252,6 +257,11 @@ pub(super) fn service( // acknowledged; sampling them during classification can blame a bystander. let attribution = sources.attribute(request.blamed, |qid, stamp| host.render_pass_started(qid, stamp)); + let attribution = sources.host_fallback(attribution, + || host.render_culprit(request.blamed?)); + if let Attribution::Queues(qids) = attribution { + pr_warn!("G17: recovery gen {} blames queues {:#x}\n", request.generation, qids); + } let target_error = host.publish_recovery_targets(); host.transition(1, 2)?; let mut recovered = false; diff --git a/drivers/gpu/drm/asahi/g17/recovery/attribution.rs b/drivers/gpu/drm/asahi/g17/recovery/attribution.rs index 38ad8612a0b99..fdf0cd62cbd07 100644 --- a/drivers/gpu/drm/asahi/g17/recovery/attribution.rs +++ b/drivers/gpu/drm/asahi/g17/recovery/attribution.rs @@ -122,6 +122,21 @@ impl Sources { Attribution::Queues(guilty) } } + + /// When slot identity cannot check a timeout's blame (25G83 reads every key as 0, and a + /// single-slot mask has no peer), the host's own pass records still can: a blamed render + /// that never started was starved, not hung. `culprit` names the queues of the one VM whose + /// render started and did not finish; without one the firmware's blame stands. + pub(super) fn host_fallback( + &self, + attribution: Attribution, + culprit: impl FnOnce() -> Option, + ) -> Attribution { + if attribution != Attribution::Firmware || self.reason.is_none_or(|reason| reason == PAGE_FAULT) { + return attribution; + } + culprit().map_or(Attribution::Firmware, Attribution::Queues) + } } #[kernel::macros::kunit_tests(asahi_g17_recovery_sources)] @@ -173,4 +188,28 @@ mod tests { assert_eq!(sources.attribute(Some(38), |qid, _| Some(qid == 39)), Attribution::Firmware); } + + #[test] + fn host_records_move_a_keyless_timeout_blame_to_the_running_vm() { + let mut sources = Sources::sample(0b10, |_| Some((0, 2))); + sources.reason = Some(0); + let attribution = sources.attribute(Some(36), |_, _| panic!("unknown key")); + assert_eq!(attribution, Attribution::Firmware); + assert_eq!(sources.host_fallback(attribution, || Some(3 << 60)), Attribution::Queues(3 << 60)); + assert_eq!(sources.host_fallback(attribution, || None), Attribution::Firmware); + } + + #[test] + fn host_records_never_override_a_page_fault_or_slot_evidence() { + let mut sources = Sources::sample(0b10, |_| Some((0, 2))); + sources.reason = Some(PAGE_FAULT); + assert_eq!(sources.host_fallback(Attribution::Firmware, || panic!("page fault")), + Attribution::Firmware); + sources.reason = None; + assert_eq!(sources.host_fallback(Attribution::Firmware, || panic!("no reason")), + Attribution::Firmware); + sources.reason = Some(0); + assert_eq!(sources.host_fallback(Attribution::NoCulprit, || panic!("slot evidence")), + Attribution::NoCulprit); + } } diff --git a/drivers/gpu/drm/asahi/g17/runtime/events.rs b/drivers/gpu/drm/asahi/g17/runtime/events.rs index 3e779bba4fbbf..ba76bb3735062 100644 --- a/drivers/gpu/drm/asahi/g17/runtime/events.rs +++ b/drivers/gpu/drm/asahi/g17/runtime/events.rs @@ -158,6 +158,9 @@ impl recovery::Queues for Service<'_> { fn render_pass_started(&self, qid: u8, stamp: u64) -> Option { self.firmware.queues.render_pass_started(qid, stamp) } + fn render_culprit(&self, blamed: u8) -> Option { + self.firmware.queues.render_culprit(blamed) + } fn publish_recovery_targets(&self) -> Option { let mut failure = self.firmware.queues.publish_render_recovery_targets(); for queue in self diff --git a/drivers/gpu/drm/asahi/g17/runtime/render/recovery.rs b/drivers/gpu/drm/asahi/g17/runtime/render/recovery.rs index 7a6d987591c76..e597f63f15b65 100644 --- a/drivers/gpu/drm/asahi/g17/runtime/render/recovery.rs +++ b/drivers/gpu/drm/asahi/g17/runtime/render/recovery.rs @@ -2,8 +2,8 @@ //! Recovery targets for retained render graphs while firmware is halted. -use crate::g17::runtime::Registry; -use kernel::prelude::*; +use crate::g17::{runtime::Registry, status::VmStatus}; +use kernel::{prelude::*, sync::Arc}; impl Registry { pub(in crate::g17::runtime) fn report_render_recovery(&self, qid: u8, port: &crate::g17::port::Port) { @@ -29,6 +29,32 @@ impl Registry { .pass_started(stamp) } + /// See `recovery::Queues::render_culprit`. + pub(in crate::g17::runtime) fn render_culprit(&self, blamed: u8) -> Option { + let pairs = || self.render.entries.iter().flatten().filter_map(|entry| entry.pair.as_ref()); + if pairs().find(|pair| pair.qids().contains(&blamed))?.front_untouched() != Some(true) { + return None; + } + let mut culprit: Option<&Arc> = None; + let mut qids = 0u128; + for pair in pairs() { + let Some(owner) = pair.running_owner() else { + continue; + }; + if culprit.is_some_and(|known| !Arc::ptr_eq(known, owner)) { + return None; + } + culprit = Some(owner); + qids |= pair.qids().into_iter().fold(0, |mask, qid| mask | 1u128 << qid); + } + let culprit = culprit?; + let mut foreign = false; + for queue in self.compute.iter().flatten().filter_map(|entry| entry.queue.as_deref()) { + queue.visit_owners(&mut |owner| foreign |= !Arc::ptr_eq(owner, culprit)); + } + (!foreign).then_some(qids) + } + pub(in crate::g17::runtime) fn publish_render_recovery_targets(&self) -> Option { let mut failure = None; for pair in self.render.entries.iter().flatten() From b8cb1a94ded4fb4120b35c5bdfe601ed9c52eb8a Mon Sep 17 00:00:00 2001 From: Jonathan Goyvaerts Date: Tue, 3 Nov 2026 13:00:47 -0300 Subject: [PATCH 3/9] drm/asahi: g17: keep another VM's recovery out of an innocent VM's status A command a GPU restart interrupted fails with ECANCELED. Unless a recovery found its VM guilty, it does not poison the VM, but it was still passed to VmStatus::report_failure, which stores it in the VM status that GET_PARAMS VM_STATUS and the status mirror return. Honeykrisp's check_status treats any nonzero status as a lost device, so every Vulkan client with work in flight lost its device in another client's recovery, while GL clients, which never read the status that way, survived. The UAPI documents the opposite: a job cancelled because another VM's recovery discarded it is not a failure of this VM, and its error stays 0. Report a non-poisoning ECANCELED to nobody, as was already done for work cancelled by its owner. The fence still carries ECANCELED, the restart is still logged ("interrupted by a GPU recovery after it started"), and a guilty VM is still poisoned with it. Under Vulkan such a submission's output is undefined, as the UAPI says, instead of the device being lost. A restart that finds nobody guilty (no blamed queue and no guilty table entries, or owners it cannot resolve) now fails all interrupted work this way, the culprit's too, so a culprit that keeps hanging is stopped only by the restart storm limit. Signed-off-by: Jonathan Goyvaerts Assisted-by: LLM --- drivers/gpu/drm/asahi/g17/completion.rs | 11 ++++++----- drivers/gpu/drm/asahi/g17/queue/render.rs | 7 ++++--- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/asahi/g17/completion.rs b/drivers/gpu/drm/asahi/g17/completion.rs index 90f6d77780bff..9e285b81c2cf2 100644 --- a/drivers/gpu/drm/asahi/g17/completion.rs +++ b/drivers/gpu/drm/asahi/g17/completion.rs @@ -547,7 +547,7 @@ impl Completion { /// A firmware restart listed this command in its recovery table: it was in /// flight, so it may have run partly. Its stamps and completion record may /// still look like success. Unless a replay republishes it from the start, - /// it fails and the failure is always reported to the client. + /// it fails: its fence carries ECANCELED, and only a guilty VM is poisoned. pub(crate) fn note_interrupted(&self) { self.interrupted.store(true, Ordering::Release); } @@ -563,12 +563,13 @@ impl Completion { (error != ECANCELED || interrupted) && !self.spared() && !(interrupted && !self.guilty()) } - /// ECANCELED stays silent only for work that was never interrupted (closed - /// or cancelled by its owner); every other failure reaches the client. + /// A non-poisoning ECANCELED stays out of the VM status, as the UAPI documents for work + /// another VM's recovery discarded: Honeykrisp reads any status error as a lost device. + /// Every other failure reaches the client. fn report(&self, error: Error) { if self.poisons(error) { self.status.record(error); - } else if error != ECANCELED || self.interrupted() { + } else if error != ECANCELED { self.status.report_failure(error); } } @@ -715,7 +716,7 @@ impl Completion { guard.quarantine(); } if self.spared() { - if error != ECANCELED || self.interrupted() { + if error != ECANCELED { self.status.report_failure(error); } } else { diff --git a/drivers/gpu/drm/asahi/g17/queue/render.rs b/drivers/gpu/drm/asahi/g17/queue/render.rs index b210f55efa2b7..7d427aeef5c49 100644 --- a/drivers/gpu/drm/asahi/g17/queue/render.rs +++ b/drivers/gpu/drm/asahi/g17/queue/render.rs @@ -1696,9 +1696,10 @@ impl Pair { /// A front group a restart interrupted after its tiler started, which firmware /// then retired without writing its fragment end, may have run partly. It is - /// not replayed (blending or storage writes could apply twice) and fails now, - /// reported to its owner, instead of waiting out the render timeout. Returns - /// None when the front is not such a group. + /// not replayed (blending or storage writes could apply twice) and fails now + /// instead of waiting out the render timeout: its fence carries ECANCELED, and + /// only a guilty VM is poisoned. Returns None when the front is not such a + /// group. fn settle_interrupted_front( &mut self, host: &mut impl Host, From 109dac13643f33c22d1bdea88ad508e3ffa15a81 Mon Sep 17 00:00:00 2001 From: Jonathan Goyvaerts Date: Tue, 3 Nov 2026 13:00:47 -0300 Subject: [PATCH 4/9] drm/asahi: g17: do not count a failed VM's leftover hangs as a restart storm A firmware restart drops the groups that had started, but a hung client's other published groups run as usual afterwards. A client that queued the same spinning draw many times (deqp-vk on 11-03 had 17 renders retained on one pair) therefore hangs the GPU once per group, 83 ms apart. Once the blame lands on that client, its VM is poisoned and it can publish nothing new, so the cascade ends with its queued work. Yet the eighth restart within 30 s reaches the storm limit and stops the GPU for every client. When every VM a recovery finds guilty had already failed before it, and the recovery table matched one of their commands by its exact stamp (so the restart dropped it), take that restart back from the storm count. At most RENDER_DEPTH restarts are forgiven per window, so an unknown firmware loop still reaches the limit. A first failure always counts. The host cannot stop the queued groups themselves: 25G83 has no recovery target word, and a ContextKill is only sent for closed contexts. Releasing a killed live context's commands would depend on the kill acknowledgement waiting for a running spinner, which is unverified. Signed-off-by: Jonathan Goyvaerts Assisted-by: LLM --- drivers/gpu/drm/asahi/g17/recovery.rs | 78 ++++++++++++++++++-- drivers/gpu/drm/asahi/g17/recovery/safety.rs | 33 ++++++++- drivers/gpu/drm/asahi/g17/runtime/events.rs | 3 + 3 files changed, 105 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/asahi/g17/recovery.rs b/drivers/gpu/drm/asahi/g17/recovery.rs index 3f5b4c353e165..5e7f001eeb681 100644 --- a/drivers/gpu/drm/asahi/g17/recovery.rs +++ b/drivers/gpu/drm/asahi/g17/recovery.rs @@ -45,6 +45,8 @@ pub(super) struct Entries { /// Listed commands found by their exact stamp, and by the oldest-work fallback. pub(super) exact: u32, pub(super) fallback: u32, + /// QIDs with a listed command found by its exact stamp: that command was dropped. + exact_qids: u128, } impl Entries { @@ -149,6 +151,8 @@ pub(super) struct State { pub(super) struct ServiceResult { pub(super) scan_error: Option, pub(super) target_error: Option, + /// Every VM found guilty had failed before, and the restart dropped one of its commands. + pub(super) leftover: bool, } impl State { @@ -164,6 +168,9 @@ impl State { self.timeouts.note_unless_progress(now, safety::retired()) } pub(super) fn restart_limit(&mut self, now: u64) -> bool { self.restarts.note(now) } + /// A restart that only dropped leftover work of failed VMs (`ServiceResult::leftover`) + /// does not count toward the storm limit. Returns whether it was forgiven. + pub(super) fn forgive_restart(&mut self) -> bool { self.restarts.forgive() } pub(super) fn generation(&self) -> u64 { self.accepted } @@ -292,7 +299,7 @@ pub(super) fn service( host.recovery_mut().pending = None; let (listed, exact, fallback) = (entries.listed, entries.exact, entries.fallback); - let (cancelled, blame) = classify(host, entries, request.blamed, attribution); + let (cancelled, blame, leftover) = classify(host, entries, request.blamed, attribution); if listed != 0 { pr_warn!( "G17: GPU recovery: {} commands were in flight ({} matched by stamp, {} by queue); they fail instead of completing\n", @@ -317,7 +324,7 @@ pub(super) fn service( } let scan_error = host.rescan_compute().err(); host.release_retired_compute(); - Ok(ServiceResult { scan_error, target_error }) + Ok(ServiceResult { scan_error, target_error, leftover }) } /// Walks the packed recovery table up to its first invalid record and marks @@ -331,8 +338,12 @@ fn read_entries(host: &mut H) -> Entries { if !entries.include(flags) { break; } - match host.note_interrupted(((flags >> 4) & 0x7f) as u8, stamp) { - Interrupted::Exact => entries.exact += 1, + let qid = ((flags >> 4) & 0x7f) as u8; + match host.note_interrupted(qid, stamp) { + Interrupted::Exact => { + entries.exact += 1; + entries.exact_qids |= 1u128 << qid; + } Interrupted::Oldest => entries.fallback += 1, Interrupted::None => {} } @@ -346,7 +357,9 @@ fn read_entries(host: &mut H) -> Entries { /// Also returns whether the firmware's blamed QID is still guilty after attribution: a blamed /// render that had not started while others had (or with no culprit at all) is innocent, and its /// work is spared like that of every other innocent VM instead of failing with EIO. -fn classify(queues: &mut Q, entries: Entries, blamed: Option, attribution: Attribution) -> (u128, bool) { +/// Last, whether every guilty VM had failed before this restart and it dropped one of their +/// commands: a failed VM publishes nothing new, so such restarts end with its queued work. +fn classify(queues: &mut Q, entries: Entries, blamed: Option, attribution: Attribution) -> (u128, bool, bool) { let mut guilty_qids = entries.guilty | blamed.map_or(0, |qid| 1u128 << qid); let cancelled = entries.cancelled; match attribution { @@ -357,7 +370,7 @@ fn classify(queues: &mut Q, entries: Entries, blamed: Option, att let no_culprit = attribution == Attribution::NoCulprit; let blame = blamed.is_some_and(|qid| guilty_qids & (1u128 << qid) != 0); if guilty_qids == 0 && !no_culprit { - return (0, blame); + return (0, blame, false); } let mut guilty: [Option>; MAX_GUILTY_VMS] = Default::default(); let mut count = 0; @@ -393,8 +406,14 @@ fn classify(queues: &mut Q, entries: Entries, blamed: Option, att } } if unresolved || (count == 0 && collateral == 0 && !no_culprit) { - return (0, blame); + return (0, blame, false); + } + let known = |status: &Arc| guilty[..count].iter().flatten().any(|known| Arc::ptr_eq(known, status)); + let mut dropped = false; + for qid in (0..QIDS).filter(|qid| entries.exact_qids & (1u128 << qid) != 0) { + queues.visit_owners(qid, &mut |status| dropped |= known(status)); } + let leftover = count != 0 && dropped && guilty[..count].iter().flatten().all(|status| status.get() != 0); let spared = queues.classify_work(&|status| { guilty[..count] .iter() @@ -406,7 +425,7 @@ fn classify(queues: &mut Q, entries: Entries, blamed: Option, att }) .unwrap_or(Class::Spared) }); - (cancelled & !(guilty_qids & !collateral) & spared, blame) + (cancelled & !(guilty_qids & !collateral) & spared, blame, leftover) } @@ -497,6 +516,49 @@ mod tests { assert!(!host.blamed); } + /// Two VMs, each owning one render pair: `a` queues 40/41, `b` queues 60/61. + struct Owners([(u8, Arc); 4]); + + impl Queues for Owners { + fn render_scheduler_active(&self) -> bool { true } + fn render_pass_started(&self, _qid: u8, _stamp: u64) -> Option { None } + fn publish_recovery_targets(&self) -> Option { None } + fn oldest_spared(&self, qid: u8) -> Option { + self.0.iter().any(|(own, _)| *own == qid).then_some(false) + } + fn visit_owners(&self, qid: u8, visit: &mut dyn FnMut(&Arc)) { + self.0.iter().filter(|(own, _)| *own == qid).for_each(|(_, status)| visit(status)); + } + fn classify_work(&mut self, _class: &dyn Fn(&Arc) -> Class) -> u128 { 0 } + fn note_interrupted(&mut self, _qid: u8, _stamp: u32) -> Interrupted { Interrupted::None } + fn settle_blamed_render(&mut self, _qid: u8, _error: Error) {} + fn settle_cancelled_renders(&mut self, _qids: u128) {} + fn rescan_compute(&mut self) -> Result { Ok(()) } + fn release_retired_compute(&mut self) {} + } + + fn owners() -> Owners { + let a = Arc::pin_init(VmStatus::new(), GFP_KERNEL).unwrap(); + let b = Arc::pin_init(VmStatus::new(), GFP_KERNEL).unwrap(); + Owners([(40, a.clone()), (41, a), (60, b.clone()), (61, b)]) + } + + fn leftover(queues: &mut Owners, exact_qid: u8) -> bool { + let entries = Entries { exact_qids: 1u128 << exact_qid, ..Entries::default() }; + classify(queues, entries, Some(40), Attribution::Queues(3u128 << 60)).2 + } + + #[test] + fn only_dropped_work_of_an_already_failed_vm_is_leftover() { + let mut queues = owners(); + // A first failure counts toward the storm. + assert!(!leftover(&mut queues, 60)); + queues.0[2].1.record(EIO); + assert!(leftover(&mut queues, 60)); + // The restart dropped nothing of the failed VM. + assert!(!leftover(&mut queues, 40)); + } + #[test] fn unknown_25g83_keys_still_preserve_firmware_blame() { let mut host = Halted::new(false); diff --git a/drivers/gpu/drm/asahi/g17/recovery/safety.rs b/drivers/gpu/drm/asahi/g17/recovery/safety.rs index d71d3cc81553f..5c2ad346c9890 100644 --- a/drivers/gpu/drm/asahi/g17/recovery/safety.rs +++ b/drivers/gpu/drm/asahi/g17/recovery/safety.rs @@ -6,6 +6,9 @@ use core::sync::atomic::{AtomicU64, Ordering}; const WINDOW_NS: u64 = 30_000_000_000; const LIMIT: u8 = 8; +/// Restarts forgiven per window: one per group a render pair holds. Leftover work of a failed +/// VM ends on its own, but an unknown firmware loop must still reach the limit. +const FORGIVE_LIMIT: u8 = crate::hw::t8140::queues::RENDER_DEPTH; /// Commands retired successfully. A watchdog storm means the GPU stopped making progress; /// timeouts of overloaded clients while other work keeps retiring are not one. @@ -34,12 +37,13 @@ pub(crate) fn since_retired(now: u64) -> Option { pub(super) struct Storm { start: Option, count: u8, + forgiven: u8, stopped: bool, progress: u64, } impl Storm { pub(super) const fn new() -> Self { - Self { start: None, count: 0, stopped: false, progress: 0 } + Self { start: None, count: 0, forgiven: 0, stopped: false, progress: 0 } } /// Like [`Self::note`], but work retired since the previous event starts a new window. pub(super) fn note_unless_progress(&mut self, now: u64, progress: u64) -> bool { @@ -54,11 +58,21 @@ impl Storm { if self.start.is_none_or(|start| now.saturating_sub(start) >= WINDOW_NS) { self.start = Some(now); self.count = 0; + self.forgiven = 0; } self.count += 1; self.stopped = self.count >= LIMIT; self.stopped } + /// Takes back the latest event, at most [`FORGIVE_LIMIT`] times per window. A stop stays. + pub(super) fn forgive(&mut self) -> bool { + if self.stopped || self.count == 0 || self.forgiven >= FORGIVE_LIMIT { + return false; + } + self.count -= 1; + self.forgiven += 1; + true + } } #[kernel::macros::kunit_tests(asahi_g17_timeout_recovery_safety)] @@ -109,6 +123,23 @@ mod tests { for _ in 0..1024 { assert!(storm.note(u64::MAX)); } } #[test] + fn forgiven_restarts_are_bounded_per_window() { + let mut storm = Storm::new(); + for _ in 0..FORGIVE_LIMIT { + assert!(!storm.note(0)); + assert!(storm.forgive()); + } + assert!(!storm.note(0)); + assert!(!storm.forgive()); + for _ in 2..LIMIT { assert!(!storm.note(0)); } + assert!(storm.note(0)); + assert!(!storm.forgive()); + let mut storm = Storm::new(); + for _ in 0..FORGIVE_LIMIT { storm.note(0); storm.forgive(); } + assert!(!storm.note(WINDOW_NS)); + assert!(storm.forgive()); + } + #[test] fn recovery_table_ends_at_its_first_invalid_record() { use crate::g17::recovery::Entries; let mut entries = Entries::default(); diff --git a/drivers/gpu/drm/asahi/g17/runtime/events.rs b/drivers/gpu/drm/asahi/g17/runtime/events.rs index ba76bb3735062..d6d811b410e89 100644 --- a/drivers/gpu/drm/asahi/g17/runtime/events.rs +++ b/drivers/gpu/drm/asahi/g17/runtime/events.rs @@ -326,6 +326,9 @@ impl Firmware { .set_generation(service.firmware.recovery.generation()); service.firmware.rings.invalidate_statistics(); let result = result?; + if result.leftover && service.firmware.recovery.forgive_restart() { + pr_warn!("G17: recovery dropped queued work of an already failed VM; not counted as a restart storm\n"); + } if let Some(error) = result.target_error { dev_warn!( shared.dev.as_ref(), From 93bdf2867955e49e6765b69bdbb745f28248e165 Mon Sep 17 00:00:00 2001 From: Jonathan Goyvaerts Date: Tue, 3 Nov 2026 14:15:03 -0300 Subject: [PATCH 5/9] drm/asahi: g17: keep work a recovery dropped out of the VM status When a GPU restart drops an innocent pair's groups and they cannot run again in order, fail_dropped fails them with ENODATA without poisoning the VM. The failure was still passed to VmStatus::report_failure, which stores it in the status that GET_PARAMS VM_STATUS and the status mirror return, and Honeykrisp's check_status treats any nonzero status as a lost device. In the 16-group spin repro on m4.119, vkmark lost its device this way twice ("failing 2 groups ... ENODATA ... the client sees a lost device") although no recovery blamed it. Treat a non-poisoning ENODATA like a non-poisoning ECANCELED: the fence carries the error, the VM status stays 0. Every other ENODATA source (compute replay and spared quarantine) is likewise work a recovery dropped, and a poisoning ENODATA is still recorded. The VM_STATUS UAPI comment now names ENODATA beside ECANCELED. Signed-off-by: Jonathan Goyvaerts Assisted-by: LLM --- drivers/gpu/drm/asahi/g17/completion.rs | 15 ++++++++++----- drivers/gpu/drm/asahi/g17/queue/render.rs | 4 ++-- include/uapi/drm/asahi_drm.h | 5 +++-- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/asahi/g17/completion.rs b/drivers/gpu/drm/asahi/g17/completion.rs index 9e285b81c2cf2..4d3d5e5a7904d 100644 --- a/drivers/gpu/drm/asahi/g17/completion.rs +++ b/drivers/gpu/drm/asahi/g17/completion.rs @@ -563,13 +563,13 @@ impl Completion { (error != ECANCELED || interrupted) && !self.spared() && !(interrupted && !self.guilty()) } - /// A non-poisoning ECANCELED stays out of the VM status, as the UAPI documents for work - /// another VM's recovery discarded: Honeykrisp reads any status error as a lost device. - /// Every other failure reaches the client. + /// A non-poisoning ECANCELED or ENODATA (work a recovery discarded or dropped) stays out of + /// the VM status, as the UAPI documents for work another VM's recovery discarded: + /// Honeykrisp reads any status error as a lost device. Every other failure reaches the client. fn report(&self, error: Error) { if self.poisons(error) { self.status.record(error); - } else if error != ECANCELED { + } else if !discarded(error) { self.status.report_failure(error); } } @@ -716,7 +716,7 @@ impl Completion { guard.quarantine(); } if self.spared() { - if error != ECANCELED { + if !discarded(error) { self.status.report_failure(error); } } else { @@ -727,6 +727,11 @@ impl Completion { } } +/// The errors of work a recovery discarded (ECANCELED) or dropped before it ran (ENODATA). +fn discarded(error: Error) -> bool { + error == ECANCELED || error == ENODATA +} + /// Collected under the device mutex and executed only after releasing it. /// Runtime queues establish the ownership witness before constructing `Retired` /// or `Release`; `FailedOwned` leaves their physical and VM ownership intact. diff --git a/drivers/gpu/drm/asahi/g17/queue/render.rs b/drivers/gpu/drm/asahi/g17/queue/render.rs index 7d427aeef5c49..661d210998b9a 100644 --- a/drivers/gpu/drm/asahi/g17/queue/render.rs +++ b/drivers/gpu/drm/asahi/g17/queue/render.rs @@ -1658,8 +1658,8 @@ impl Pair { /// Firmware retired every group of the pair after a restart, but some were dropped /// unexecuted and cannot run again in order (see `replay::decide`). Every group fails now, /// the ones that ran too: they consumed output the dropped ones never wrote. The work is - /// innocent, so it fails without poisoning its VM, with ENODATA, which its owner sees, - /// instead of waiting for the render timeout to fail it with ETIMEDOUT. + /// innocent, so it fails without poisoning its VM: its fences carry ENODATA, which stays out + /// of the VM status, instead of waiting for the render timeout to fail it with ETIMEDOUT. fn fail_dropped( &mut self, host: &mut impl Host, diff --git a/include/uapi/drm/asahi_drm.h b/include/uapi/drm/asahi_drm.h index 418522205ee6f..5641384b51bfe 100644 --- a/include/uapi/drm/asahi_drm.h +++ b/include/uapi/drm/asahi_drm.h @@ -447,8 +447,9 @@ enum drm_asahi_feature { * treating that fence as successful execution. Syncobj signal alone is not * an execution-success indication. A job cancelled (ECANCELED) because * another VM's recovery discarded it, an input did not signal in time, or - * its queue was closed, is not a failure of this VM: its fence carries the - * error, error stays 0, and its output is undefined. + * its queue was closed, or dropped (ENODATA) because a recovery could not + * run it again, is not a failure of this VM: its fence carries the error, + * error stays 0, and its output is undefined. * The feature bit is the availability contract; older kernels leave it zero. * Kernel errors for invalid requests: EINVAL flags/pad/size, ENOENT foreign or * missing vm_id, EFAULT inaccessible pointer. No output on invalid request. From 616a0678a847097b578ef094653a11549bdb1c99 Mon Sep 17 00:00:00 2001 From: Jonathan Goyvaerts Date: Tue, 3 Nov 2026 14:15:04 -0300 Subject: [PATCH 6/9] drm/asahi: g17: fail exhausted replays without poisoning their VM A pair whose groups a restart drops untouched has them republished, up to MAX_REPLAYS times. In a cascade of restarts another client causes, the same innocent groups are dropped on every restart, and the fourth drop failed them with EIO recorded on their VM: T3 Code lost its device in the 16-group spin repro on m4.119 ("12 groups retired without execution; replay limit reached, failing them"). replay::decide() only replays groups that are innocent, not blamed and never ran, so none of them can have caused the hang. Fail them like fail_dropped does: spared, with ENODATA, which stays out of the VM status. The replay limit still bounds how often they are republished. Signed-off-by: Jonathan Goyvaerts Assisted-by: LLM --- drivers/gpu/drm/asahi/g17/queue/render.rs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/asahi/g17/queue/render.rs b/drivers/gpu/drm/asahi/g17/queue/render.rs index 661d210998b9a..b387d294cda5a 100644 --- a/drivers/gpu/drm/asahi/g17/queue/render.rs +++ b/drivers/gpu/drm/asahi/g17/queue/render.rs @@ -1635,13 +1635,10 @@ impl Pair { let packet = front.packet.clone(); failing = failing || !packet.completion.begin_replay(); if failing { - if !packet.completion.spared() { - packet.completion.status().record(EIO); - if self.terminal.is_none() { - self.terminal = Some((packet.completion.status().clone(), EIO)); - } - } - defer(Deferred::Retired(packet.completion.clone(), Err(EIO)))?; + // decide() replays only innocent groups that never ran: like `fail_dropped`, + // they fail without poisoning their VM. Restarts other VMs caused dropped them. + packet.completion.classify(recovery::Class::Spared); + defer(Deferred::Retired(packet.completion.clone(), Err(ENODATA)))?; self.memory.retire(ordinal)?; self.active.pop().ok_or(EIO)?; continue; From 92f6396a4476ca5fbd1943353cb52f2c6cf11ce6 Mon Sep 17 00:00:00 2001 From: Jonathan Goyvaerts Date: Tue, 3 Nov 2026 14:15:04 -0300 Subject: [PATCH 7/9] drm/asahi: g17: keep a VM a recovery found guilty out of later spared work Each recovery classifies every VM it does not find guilty as spared. A hung client's VM that an earlier recovery found guilty therefore turned spared as soon as one later recovery blamed someone else. The next recovery that blamed it again then found its oldest work spared, took the blame as collateral to that recovery, found no guilty VM, and so did not treat the restart as leftover work of a failed VM: it counted toward the restart storm limit. In the 16-group spin repro on m4.119, gens 9 and 10 blamed glmark2 and quickshell, and the four correct blames on the spinning client after them all counted; the next restart reached the limit and stopped the GPU for everyone. Remember on the VM status that a recovery found it guilty, and keep such a VM's work guilty in every later recovery. A VM that failed only for its own client is still spared. Signed-off-by: Jonathan Goyvaerts Assisted-by: LLM --- drivers/gpu/drm/asahi/g17/recovery.rs | 30 +++++++++++++++++++++++---- drivers/gpu/drm/asahi/g17/status.rs | 13 ++++++++++++ 2 files changed, 39 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/asahi/g17/recovery.rs b/drivers/gpu/drm/asahi/g17/recovery.rs index 5e7f001eeb681..d09a41a771e5b 100644 --- a/drivers/gpu/drm/asahi/g17/recovery.rs +++ b/drivers/gpu/drm/asahi/g17/recovery.rs @@ -414,6 +414,7 @@ fn classify(queues: &mut Q, entries: Entries, blamed: Option, att queues.visit_owners(qid, &mut |status| dropped |= known(status)); } let leftover = count != 0 && dropped && guilty[..count].iter().flatten().all(|status| status.get() != 0); + guilty[..count].iter().flatten().for_each(|status| status.note_recovery_guilty()); let spared = queues.classify_work(&|status| { guilty[..count] .iter() @@ -423,7 +424,9 @@ fn classify(queues: &mut Q, entries: Entries, blamed: Option, att .filter(|known| Arc::ptr_eq(known, status)) .map(|_| Class::Guilty) }) - .unwrap_or(Class::Spared) + // A VM an earlier recovery found guilty stays guilty. Spared, its leftover work would + // make the next blame on it collateral, and that restart would count as a storm. + .unwrap_or(if status.recovery_guilty() { Class::Guilty } else { Class::Spared }) }); (cancelled & !(guilty_qids & !collateral) & spared, blame, leftover) } @@ -517,7 +520,7 @@ mod tests { } /// Two VMs, each owning one render pair: `a` queues 40/41, `b` queues 60/61. - struct Owners([(u8, Arc); 4]); + struct Owners([(u8, Arc); 4], [Option; 4]); impl Queues for Owners { fn render_scheduler_active(&self) -> bool { true } @@ -529,7 +532,12 @@ mod tests { fn visit_owners(&self, qid: u8, visit: &mut dyn FnMut(&Arc)) { self.0.iter().filter(|(own, _)| *own == qid).for_each(|(_, status)| visit(status)); } - fn classify_work(&mut self, _class: &dyn Fn(&Arc) -> Class) -> u128 { 0 } + fn classify_work(&mut self, class: &dyn Fn(&Arc) -> Class) -> u128 { + for (index, (_, status)) in self.0.iter().enumerate() { + self.1[index] = Some(class(status)); + } + 0 + } fn note_interrupted(&mut self, _qid: u8, _stamp: u32) -> Interrupted { Interrupted::None } fn settle_blamed_render(&mut self, _qid: u8, _error: Error) {} fn settle_cancelled_renders(&mut self, _qids: u128) {} @@ -540,7 +548,7 @@ mod tests { fn owners() -> Owners { let a = Arc::pin_init(VmStatus::new(), GFP_KERNEL).unwrap(); let b = Arc::pin_init(VmStatus::new(), GFP_KERNEL).unwrap(); - Owners([(40, a.clone()), (41, a), (60, b.clone()), (61, b)]) + Owners([(40, a.clone()), (41, a), (60, b.clone()), (61, b)], [None; 4]) } fn leftover(queues: &mut Owners, exact_qid: u8) -> bool { @@ -559,6 +567,20 @@ mod tests { assert!(!leftover(&mut queues, 40)); } + #[test] + fn a_guilty_vm_is_not_spared_by_another_vms_recovery() { + let mut queues = owners(); + let entries = || Entries { exact_qids: 1u128 << 40, ..Entries::default() }; + // b failing for its own client does not make it guilty in a's recovery. + queues.0[2].1.record(EIO); + classify(&mut queues, entries(), Some(40), Attribution::Queues(3u128 << 40)); + assert_eq!(queues.1, [Some(Class::Guilty), Some(Class::Guilty), Some(Class::Spared), Some(Class::Spared)]); + // Once a recovery finds b guilty, a later recovery that blames a keeps b guilty. + classify(&mut queues, entries(), Some(60), Attribution::Queues(3u128 << 60)); + classify(&mut queues, entries(), Some(40), Attribution::Queues(3u128 << 40)); + assert_eq!(queues.1, [Some(Class::Guilty); 4]); + } + #[test] fn unknown_25g83_keys_still_preserve_firmware_blame() { let mut host = Halted::new(false); diff --git a/drivers/gpu/drm/asahi/g17/status.rs b/drivers/gpu/drm/asahi/g17/status.rs index 4d9cd28cc430a..75df8ad0480ae 100644 --- a/drivers/gpu/drm/asahi/g17/status.rs +++ b/drivers/gpu/drm/asahi/g17/status.rs @@ -22,6 +22,8 @@ pub(crate) struct VmStatus { client_closed: AtomicBool, /// A failure of this VM has been logged as a warning. warned: AtomicBool, + /// A GPU recovery found this VM's work guilty. + recovery_guilty: AtomicBool, /// The process that created the VM, named in the failure report. tgid: Pid, comm: [u8; 16], @@ -55,6 +57,7 @@ impl VmStatus { reported_error: AtomicI32::new(0), client_closed: AtomicBool::new(false), warned: AtomicBool::new(false), + recovery_guilty: AtomicBool::new(false), tgid, comm, mirror <- new_mutex!(None, "Asahi VM status mirror"), @@ -65,6 +68,16 @@ impl VmStatus { self.error.load(Ordering::Acquire) } + pub(crate) fn note_recovery_guilty(&self) { + self.recovery_guilty.store(true, Ordering::Release); + } + + /// A GPU recovery found this VM guilty: it hung or faulted the GPU, unlike a VM that + /// failed only for its own client. + pub(crate) fn recovery_guilty(&self) -> bool { + self.recovery_guilty.load(Ordering::Acquire) + } + /// Record permanent admission failure and report it to the client. #[track_caller] pub(crate) fn record(&self, error: Error) { From 8669bb339ac494978a493cd76e4ca67a08cf8437 Mon Sep 17 00:00:00 2001 From: Jonathan Goyvaerts Date: Tue, 3 Nov 2026 14:15:04 -0300 Subject: [PATCH 8/9] drm/asahi: g17: blame a guilty VM's running render before a starved one render_culprit moves a keyless timeout's blame off a render that never started only when exactly one VM has a render that started and did not finish, and no other VM holds compute work. Under load that rarely holds: bystanders' tilers start and their fragments then wait behind the spinning one. In the 16-group spin repro on m4.119, gens 9 and 10 blamed glmark2 and quickshell, whose blamed renders had neither pass written, and both lost their device. If a VM an earlier recovery found guilty still has such a render, blame it first: its groups queued before that recovery keep hanging the GPU one per restart, and it has already failed. Only otherwise does the one-running-VM rule apply. Signed-off-by: Jonathan Goyvaerts Assisted-by: LLM --- .../drm/asahi/g17/runtime/render/recovery.rs | 82 +++++++++++++++---- 1 file changed, 65 insertions(+), 17 deletions(-) diff --git a/drivers/gpu/drm/asahi/g17/runtime/render/recovery.rs b/drivers/gpu/drm/asahi/g17/runtime/render/recovery.rs index e597f63f15b65..3bfca0d1aafe1 100644 --- a/drivers/gpu/drm/asahi/g17/runtime/render/recovery.rs +++ b/drivers/gpu/drm/asahi/g17/runtime/render/recovery.rs @@ -35,24 +35,17 @@ impl Registry { if pairs().find(|pair| pair.qids().contains(&blamed))?.front_untouched() != Some(true) { return None; } - let mut culprit: Option<&Arc> = None; - let mut qids = 0u128; - for pair in pairs() { - let Some(owner) = pair.running_owner() else { - continue; - }; - if culprit.is_some_and(|known| !Arc::ptr_eq(known, owner)) { - return None; + let running = pairs().filter_map(|pair| { + let qids = pair.qids().into_iter().fold(0, |mask, qid| mask | 1u128 << qid); + Some((qids, pair.running_owner()?)) + }); + culprit(running, |culprit| { + let mut foreign = false; + for queue in self.compute.iter().flatten().filter_map(|entry| entry.queue.as_deref()) { + queue.visit_owners(&mut |owner| foreign |= !Arc::ptr_eq(owner, culprit)); } - culprit = Some(owner); - qids |= pair.qids().into_iter().fold(0, |mask, qid| mask | 1u128 << qid); - } - let culprit = culprit?; - let mut foreign = false; - for queue in self.compute.iter().flatten().filter_map(|entry| entry.queue.as_deref()) { - queue.visit_owners(&mut |owner| foreign |= !Arc::ptr_eq(owner, culprit)); - } - (!foreign).then_some(qids) + foreign + }) } pub(in crate::g17::runtime) fn publish_render_recovery_targets(&self) -> Option { @@ -67,3 +60,58 @@ impl Registry { failure } } + +/// Picks the QIDs to blame from each pair's running owner. A VM an earlier recovery found guilty +/// comes first: its groups queued before that keep hanging the GPU one per restart, while +/// bystanders' renders whose tiler started wait behind them, and it has already failed. +/// Otherwise exactly one VM may be running, and `foreign` must find no other VM's compute work. +fn culprit<'a>( + running: impl Iterator)>, + foreign: impl FnOnce(&Arc) -> bool, +) -> Option { + let (mut guilty, mut qids, mut several) = (0u128, 0u128, false); + let mut culprit: Option<&Arc> = None; + for (pair, owner) in running { + if owner.recovery_guilty() { + guilty |= pair; + } + several |= culprit.is_some_and(|known| !Arc::ptr_eq(known, owner)); + culprit = Some(owner); + qids |= pair; + } + if guilty != 0 { + return Some(guilty); + } + let culprit = culprit.filter(|_| !several)?; + (!foreign(culprit)).then_some(qids) +} + +#[kernel::macros::kunit_tests(asahi_g17_render_culprit)] +mod tests { + use super::*; + + fn vm() -> Arc { + Arc::pin_init(VmStatus::new(), GFP_KERNEL).unwrap() + } + + #[test] + fn a_guilty_vm_still_running_is_blamed_before_running_bystanders() { + let (spinner, bystander) = (vm(), vm()); + let running = || [(0b11, &bystander), (0b1100, &spinner)].into_iter(); + // Two VMs running and no earlier guilt: the firmware's blame stands, even when one of + // them failed only for its own client. + bystander.record(EIO); + assert_eq!(culprit(running(), |_| false), None); + spinner.note_recovery_guilty(); + assert_eq!(culprit(running(), |_| panic!("compute is not consulted")), Some(0b1100)); + } + + #[test] + fn one_running_vm_is_blamed_only_without_foreign_compute() { + let (a, b) = (vm(), vm()); + assert_eq!(culprit([(0b11, &a), (0b1100, &a)].into_iter(), |_| false), Some(0b1111)); + assert_eq!(culprit([(0b11, &a)].into_iter(), |_| true), None); + assert_eq!(culprit([(0b11, &a), (0b1100, &b)].into_iter(), |_| false), None); + assert_eq!(culprit(core::iter::empty(), |_| false), None); + } +} From 6295b0256c1c1de0d02f056548b62c290c93b3bd Mon Sep 17 00:00:00 2001 From: Jonathan Goyvaerts Date: Tue, 3 Nov 2026 15:10:23 -0300 Subject: [PATCH 9/9] drm/asahi: g17: blame a guilty VM's running render over a started one render_culprit moved a keyless timeout's blame only when the blamed pair's oldest render had not started. A bystander's render whose tiler started and whose fragment pass then waits behind the spinning one is just as starved. On m4.120, in the 16-group spin repro, 15 of the 16 recoveries landed on the spinning client, 3 of them through render_culprit. In the 16th, the firmware blamed T3 Code, whose render had its tiler start written and no fragment end, while the spinner's last group ran. T3 Code lost its device. When a VM an earlier recovery found guilty still has a render that started and did not finish, blame it whether or not the blamed render had started: that VM has already failed, and its queued groups end with the restarts they cause. The one-running-VM rule, which can blame a VM that has not failed, still needs a blamed render that never started. Known limit: if a second VM hangs while a guilty VM still has started renders, each restart blames the guilty VM until those renders are gone, and those restarts do not count toward the restart storm limit. The evidence here cannot tell that second hang from a starved bystander, which is the case this change is for. Signed-off-by: Jonathan Goyvaerts Assisted-by: LLM --- drivers/gpu/drm/asahi/g17/recovery.rs | 9 +++--- .../drm/asahi/g17/runtime/render/recovery.rs | 32 +++++++++++-------- 2 files changed, 23 insertions(+), 18 deletions(-) diff --git a/drivers/gpu/drm/asahi/g17/recovery.rs b/drivers/gpu/drm/asahi/g17/recovery.rs index d09a41a771e5b..d42c0e8f37efb 100644 --- a/drivers/gpu/drm/asahi/g17/recovery.rs +++ b/drivers/gpu/drm/asahi/g17/recovery.rs @@ -94,10 +94,11 @@ pub(super) enum Class { pub(super) trait Queues { fn render_scheduler_active(&self) -> bool; fn render_pass_started(&self, qid: u8, stamp: u64) -> Option; - /// Read while firmware is halted, like `render_pass_started`. When the oldest published - /// render of the pair holding `blamed` had not started, the QIDs of every pair of the one VM - /// with a render that started and did not finish. None if no such VM, several, or any other - /// VM holds compute work, which these records cannot see. + /// Read while firmware is halted, like `render_pass_started`. The QIDs of every pair of a VM + /// an earlier recovery found guilty with a render that started and did not finish. Without + /// one, and only when the oldest published render of the pair holding `blamed` had not + /// started, those of the one VM with such a render. None if no such VM, several, or any + /// other VM holds compute work, which these records cannot see. fn render_culprit(&self, _blamed: u8) -> Option { None } /// Copy every retained queue's live producer to its recovery target, then /// issue a full barrier. A failed graph must not skip subsequent queues. diff --git a/drivers/gpu/drm/asahi/g17/runtime/render/recovery.rs b/drivers/gpu/drm/asahi/g17/runtime/render/recovery.rs index 3bfca0d1aafe1..02ece8cf4025b 100644 --- a/drivers/gpu/drm/asahi/g17/runtime/render/recovery.rs +++ b/drivers/gpu/drm/asahi/g17/runtime/render/recovery.rs @@ -32,14 +32,12 @@ impl Registry { /// See `recovery::Queues::render_culprit`. pub(in crate::g17::runtime) fn render_culprit(&self, blamed: u8) -> Option { let pairs = || self.render.entries.iter().flatten().filter_map(|entry| entry.pair.as_ref()); - if pairs().find(|pair| pair.qids().contains(&blamed))?.front_untouched() != Some(true) { - return None; - } + let untouched = pairs().find(|pair| pair.qids().contains(&blamed))?.front_untouched() == Some(true); let running = pairs().filter_map(|pair| { let qids = pair.qids().into_iter().fold(0, |mask, qid| mask | 1u128 << qid); Some((qids, pair.running_owner()?)) }); - culprit(running, |culprit| { + culprit(running, untouched, |culprit| { let mut foreign = false; for queue in self.compute.iter().flatten().filter_map(|entry| entry.queue.as_deref()) { queue.visit_owners(&mut |owner| foreign |= !Arc::ptr_eq(owner, culprit)); @@ -62,11 +60,13 @@ impl Registry { } /// Picks the QIDs to blame from each pair's running owner. A VM an earlier recovery found guilty -/// comes first: its groups queued before that keep hanging the GPU one per restart, while -/// bystanders' renders whose tiler started wait behind them, and it has already failed. -/// Otherwise exactly one VM may be running, and `foreign` must find no other VM's compute work. +/// comes first, even when the blamed render had started: its groups queued before that keep +/// hanging the GPU one per restart, while bystanders' renders whose tiler started wait behind +/// them, and that recovery failed its interrupted work. Otherwise the blamed render must be +/// `untouched`, exactly one VM may be running, and `foreign` must find no other VM's compute work. fn culprit<'a>( running: impl Iterator)>, + untouched: bool, foreign: impl FnOnce(&Arc) -> bool, ) -> Option { let (mut guilty, mut qids, mut several) = (0u128, 0u128, false); @@ -82,7 +82,7 @@ fn culprit<'a>( if guilty != 0 { return Some(guilty); } - let culprit = culprit.filter(|_| !several)?; + let culprit = culprit.filter(|_| untouched && !several)?; (!foreign(culprit)).then_some(qids) } @@ -101,17 +101,21 @@ mod tests { // Two VMs running and no earlier guilt: the firmware's blame stands, even when one of // them failed only for its own client. bystander.record(EIO); - assert_eq!(culprit(running(), |_| false), None); + assert_eq!(culprit(running(), true, |_| false), None); spinner.note_recovery_guilty(); - assert_eq!(culprit(running(), |_| panic!("compute is not consulted")), Some(0b1100)); + assert_eq!(culprit(running(), true, |_| panic!("compute is not consulted")), Some(0b1100)); + // The bystander's blamed render whose tiler started is starved too. + assert_eq!(culprit(running(), false, |_| panic!("compute is not consulted")), Some(0b1100)); } #[test] fn one_running_vm_is_blamed_only_without_foreign_compute() { let (a, b) = (vm(), vm()); - assert_eq!(culprit([(0b11, &a), (0b1100, &a)].into_iter(), |_| false), Some(0b1111)); - assert_eq!(culprit([(0b11, &a)].into_iter(), |_| true), None); - assert_eq!(culprit([(0b11, &a), (0b1100, &b)].into_iter(), |_| false), None); - assert_eq!(culprit(core::iter::empty(), |_| false), None); + assert_eq!(culprit([(0b11, &a), (0b1100, &a)].into_iter(), true, |_| false), Some(0b1111)); + // A blamed render that had started may be the hang itself. + assert_eq!(culprit([(0b11, &a), (0b1100, &a)].into_iter(), false, |_| false), None); + assert_eq!(culprit([(0b11, &a)].into_iter(), true, |_| true), None); + assert_eq!(culprit([(0b11, &a), (0b1100, &b)].into_iter(), true, |_| false), None); + assert_eq!(culprit(core::iter::empty(), true, |_| false), None); } }