From 1cc4da803d4935d618108539ec425e7975ea99bf Mon Sep 17 00:00:00 2001 From: Jonathan Goyvaerts Date: Sat, 24 Oct 2026 17:09:48 -0300 Subject: [PATCH 1/2] drm/asahi: g17: keep innocent clients alive across another client's recovery When one client hangs the GPU and the firmware recovers, other clients lost their device too. On a J616s (25G83) three reason-0 recoveries of one dEQP client took down Hyprland (frozen desktop), Chromium, Electron and a muvm GPU worker; a hang in headless vkmark took down T3 Code the same way. Each failed before publication, and fail_unpublished() reported the error, which Honeykrisp treats as a lost device: - Innocent work waited only briefly for a recovery to settle: render admission behind a spared, quarantined pair for 1 s, the render publication for 2 s, render and compute for 200 polls of 1 ms while a recovery was pending, a spared compute drain for 200 polls, and the compute admission, render capacity and busy waits for 2 s (the last with ETIMEDOUT, which also marks the VM lost). A spared pair drains only when the watchdog settles the dropped work, after one or more 2 s polls: EBUSY. - A command whose input fence, same-VM predecessor or compute producer had failed was cancelled (inherited ECANCELED), so the consumer of innocent work that the firmware dropped in the recovery failed too, and so did a replay of such a command; an ordered output publication inherited its predecessor's error and reported it. Wait instead, while a recovery is pending or the owner's pair is spared and draining, for up to RECOVERY_WAIT_MS from the start of the wait: the longest the watchdog defers work behind another VM before it settles it, ray work included (26 s). The compute admission deadline becomes extendable for this; each render wait keeps its own clock, so its usual bound applies afresh once the recovery ends instead of having been spent during it. A guilty pair's publication still fails after a second, and a guilty VM's sticky status fails its next submission at once. Do not fail a command because an input, a predecessor or a producer failed. As in drm_sched, only its own state decides (cancellation and its VM status); a signalled producer, failed or not, orders nothing. The producer's client sees its own error. The queue's per-engine failed-ordinal record, which existed only to cancel later commands, goes. Do not report ECANCELED from fail_unpublished(), the ordered output publication, or fail_while_owned() for spared work, as complete() already does: a cancellation is not this client's device loss, and a guilty VM carries its own sticky error. The UAPI text for PER_COMMAND_INPUT_SYNCS and VM_STATUS says so. Trade-off: a command no longer waits for a failed same-VM predecessor that is still retained on a quarantined pair; if that predecessor is replayed later, the two run out of order (wrong contents for a frame, not a lost device). Signed-off-by: Jonathan Goyvaerts Assisted-by: LLM --- drivers/gpu/drm/asahi/g17/completion.rs | 8 +- .../drm/asahi/g17/completion/publication.rs | 71 +++++------- drivers/gpu/drm/asahi/g17/dependency.rs | 32 ++++-- drivers/gpu/drm/asahi/g17/job.rs | 21 +--- drivers/gpu/drm/asahi/g17/preparation.rs | 35 +++++- drivers/gpu/drm/asahi/g17/queue/admission.rs | 38 +++++-- drivers/gpu/drm/asahi/g17/queue/compute.rs | 11 +- drivers/gpu/drm/asahi/g17/queue/mod.rs | 54 +++------ drivers/gpu/drm/asahi/g17/recovery/safety.rs | 4 +- drivers/gpu/drm/asahi/g17/runtime.rs | 35 +++--- .../drm/asahi/g17/runtime/render/submit.rs | 104 +++++++++++++----- drivers/gpu/drm/asahi/g17/timeout.rs | 7 ++ include/uapi/drm/asahi_drm.h | 16 ++- 13 files changed, 244 insertions(+), 192 deletions(-) diff --git a/drivers/gpu/drm/asahi/g17/completion.rs b/drivers/gpu/drm/asahi/g17/completion.rs index 4f5b87bc853dc..276bdc8e81a96 100644 --- a/drivers/gpu/drm/asahi/g17/completion.rs +++ b/drivers/gpu/drm/asahi/g17/completion.rs @@ -570,7 +570,9 @@ impl Completion { return; } self.status.record_if_device_loss(error); - if self.accepted.load(Ordering::Acquire) { + // As in complete(): a cancellation is not this client's device loss. A guilty VM + // already carries its own sticky error. + if self.accepted.load(Ordering::Acquire) && error != ECANCELED { self.status.report_failure(error); } self.release_healthy(); @@ -589,7 +591,9 @@ impl Completion { guard.quarantine(); } if self.spared() { - self.status.report_failure(error); + if error != ECANCELED { + self.status.report_failure(error); + } } else if error != ECANCELED { self.status.record(error); } diff --git a/drivers/gpu/drm/asahi/g17/completion/publication.rs b/drivers/gpu/drm/asahi/g17/completion/publication.rs index a1e05a9408552..2ef21401e355e 100644 --- a/drivers/gpu/drm/asahi/g17/completion/publication.rs +++ b/drivers/gpu/drm/asahi/g17/completion/publication.rs @@ -8,14 +8,13 @@ //! nor waits for another command; the last prerequisite publishes a ready result. use super::Output; -use crate::g17::{dependency::inherited_error, status::VmStatus}; +use crate::g17::status::VmStatus; use kernel::{new_mutex, prelude::*, sync::{Arc, Mutex}}; type Links = Option>; struct Link { target: Arc, - result: Result, next: Links, // Reuse an already-consumed link as a traversal frame instead of recursing // or allocating when publication unlocks a long chain of ready successors. @@ -25,9 +24,8 @@ struct Link { struct State { remaining: usize, result: Option>, - predecessor_error: Option, claimed: bool, - published: Option, + published: bool, successors: Links, } @@ -38,10 +36,9 @@ impl State { } let result = self.result.take()?; self.claimed = true; - Some(match (result, self.predecessor_error) { - (Ok(_), Some(error)) => Err(error), - (result, _) => result, - }) + // A predecessor's failure orders this publication but does not fail it: the + // command ran (Packet::check_dependencies), and a guilty VM's status is sticky. + Some(result) } } @@ -63,9 +60,8 @@ impl Publication { // Acceptance installs all links before releasing this sentinel. remaining: 1, result: None, - predecessor_error: None, claimed: false, - published: None, + published: false, successors: None, }, "G17 host timestamp publication"), }), @@ -94,36 +90,23 @@ impl Publication { fn publish(&self, result: Result<[u64; 4]>) -> Links { if let Err(error) = result { - // Inherited failure is visible to the client without inventing a - // permanent admission failure in this command's VM. - self.status.report_failure(error); + // As in Completion::complete(): a cancellation is not this client's device loss. + if error != ECANCELED { + self.status.report_failure(error); + } } self.output.publish(result); - let result = result.map(|_| ()); - let mut links = { - let mut state = self.state.lock(); - // A late link is acknowledged only after timestamp writes, the public - // fence and aggregate membership have all been published off this lock. - state.published = Some(result); - state.successors.take() - }; - let mut next = links.as_mut(); - while let Some(link) = next { - link.result = result; - next = link.next.as_mut(); - } - links + let mut state = self.state.lock(); + // A late link is acknowledged only after timestamp writes, the public + // fence and aggregate membership have all been published off this lock. + state.published = true; + state.successors.take() } - fn predecessor_done(&self, result: Result) -> Links { + fn predecessor_done(&self) -> Links { let ready = { let mut state = self.state.lock(); debug_assert!(state.remaining != 0); - if let Err(error) = result { - if state.predecessor_error.is_none() { - state.predecessor_error = Some(inherited_error(error)); - } - } state.remaining -= 1; state.claim() }; @@ -131,18 +114,15 @@ impl Publication { } fn add_successor(&self, mut link: KBox) { - let published = { + { let mut state = self.state.lock(); - match state.published { - Some(result) => result, - None => { - link.next = state.successors.take(); - state.successors = Some(link); - return; - } + if !state.published { + link.next = state.successors.take(); + state.successors = Some(link); + return; } - }; - Self::drain(link.target.predecessor_done(published)); + } + Self::drain(link.target.predecessor_done()); } /// Iterative depth-first notification, O(nodes + links), with no stack growth. @@ -151,7 +131,7 @@ impl Publication { loop { if let Some(mut link) = pending.take() { pending = link.next.take(); - let children = link.target.predecessor_done(link.result); + let children = link.target.predecessor_done(); if children.is_some() { link.resume = pending.take(); link.next = frames.take(); @@ -184,7 +164,6 @@ impl Prepared { for predecessor in predecessors { let link = KBox::new(Link { target: target.clone(), - result: Ok(()), next: None, resume: None, }, GFP_KERNEL)?; @@ -204,6 +183,6 @@ impl Prepared { for (predecessor, link) in self.links { predecessor.add_successor(link); } - Publication::drain(self.target.predecessor_done(Ok(()))); + Publication::drain(self.target.predecessor_done()); } } diff --git a/drivers/gpu/drm/asahi/g17/dependency.rs b/drivers/gpu/drm/asahi/g17/dependency.rs index 0bc1d5024566e..ce982e3d8918c 100644 --- a/drivers/gpu/drm/asahi/g17/dependency.rs +++ b/drivers/gpu/drm/asahi/g17/dependency.rs @@ -18,11 +18,11 @@ pub(crate) fn inherited_error(error: Error) -> Error { } /// A replay's original host inputs must still be signalled. Treat an unmet -/// input as a failed replay, never as a wait under the device mutex. +/// input as a failed replay, never as a wait under the device mutex. A failed +/// input does not fail the replay, as it did not fail the first publication +/// (`Packet::check_dependencies`). pub(crate) fn replay_status(status: i32) -> Result { - if status == 0 { Err(EBUSY) } - else if status < 0 { Err(inherited_error(Error::from_errno(status))) } - else { Ok(()) } + if status == 0 { Err(EBUSY) } else { Ok(()) } } pub(crate) struct Frontier { @@ -67,8 +67,8 @@ impl Frontier { Ok(()) } - /// The caller has matched the logical owner and VM. Check every covered - /// predecessor for failure, including one a newer kick would subsume. + /// The caller has matched the logical owner and VM. Order this command after + /// every covered predecessor still pending on the firmware. pub(crate) fn include( &mut self, prefix: Option, @@ -79,13 +79,25 @@ impl Frontier { if !Order::contains(prefix, sequence) { return Ok(()); } - if status < 0 { - return Err(ECANCELED); - } - if status > 0 { + // A failed predecessor will never run: there is nothing to wait for. Like a failed + // input (Packet::check_dependencies), it does not cancel this command. + if status != 0 { return Ok(()); } let dependency = KickDependency::from_word(word).ok_or(EIO)?; self.insert(sequence, dependency) } } + +#[kunit_tests(asahi_g17_dependency)] +mod tests { + use super::*; + + #[test] + fn a_failed_predecessor_is_no_dependency() { + let mut frontier = Frontier::new(); + assert_eq!(frontier.include(Some(5), 5, ECANCELED.to_errno(), 0), Ok(())); + assert_eq!(frontier.include(Some(5), 4, EIO.to_errno(), 0), Ok(())); + assert_eq!(frontier.count, 0); + } +} diff --git a/drivers/gpu/drm/asahi/g17/job.rs b/drivers/gpu/drm/asahi/g17/job.rs index 3b47d361c2ef9..98599b2d816fb 100644 --- a/drivers/gpu/drm/asahi/g17/job.rs +++ b/drivers/gpu/drm/asahi/g17/job.rs @@ -149,12 +149,14 @@ impl Packet { for fence in &self.inputs { super::dependency::replay_status(fence_status(fence))?; } - for fence in &self.firmware { check_status(fence)?; } Ok(()) } - /// Scheduler dependency readiness includes failed fences. Consume the actual - /// status before publication, and retain the references through that check. + /// Wait until every input has signalled, retaining the references through the wait. + /// A failed input does not fail this command: as in drm_sched, only its own state + /// decides (`cancel_requested`, its VM status). The producer's client already sees its + /// own error, and an innocent producer whose work another VM's recovery cancelled must + /// not take its consumers (a compositor, say) down with it. pub(crate) fn check_dependencies(&self) -> Result { let polls = if self.command.long_job() { 5 } else { 1 }; let deadline = super::now_ns().saturating_add(u64::from(TIMEOUT_MS) * polls * 1_000_000); @@ -178,10 +180,6 @@ impl Packet { } if waited > 0 { break; } } - check_status(fence)?; - } - for fence in &self.firmware { - check_status(fence)?; } Ok(()) } @@ -249,15 +247,6 @@ pub(crate) fn fence_status(fence: &Fence) -> i32 { unsafe { bindings::dma_fence_get_status(fence.raw()) } } -fn check_status(fence: &Fence) -> Result { - let status = fence_status(fence); - if status < 0 { - Err(super::dependency::inherited_error(Error::from_errno(status))) - } else { - Ok(()) - } -} - /// The backend retains visible packets through exact retirement or processor stop. /// It performs all completion signalling after releasing the device mutex. pub(crate) trait Backend: Send + Sync { diff --git a/drivers/gpu/drm/asahi/g17/preparation.rs b/drivers/gpu/drm/asahi/g17/preparation.rs index 0297c619b4f93..bb726ef2cf68a 100644 --- a/drivers/gpu/drm/asahi/g17/preparation.rs +++ b/drivers/gpu/drm/asahi/g17/preparation.rs @@ -6,6 +6,7 @@ //! the device mutex, then drops that mutex before waiting for builders. A builder's lease is //! stored after its tentative mappings so those mappings are destroyed before the join completes. +use core::cell::Cell; use core::sync::atomic::{AtomicU64, Ordering}; use kernel::{ new_condvar, new_mutex, @@ -24,25 +25,37 @@ const EPOCH: u64 = CLOSED << 3; /// packet cancellation, logical close, VM status and device loss even when no /// capacity/recovery event arrives. Wakes and nested constructors never renew it. pub(super) struct Admission<'a> { - deadline: u64, + deadline: Cell, + started: u64, check: &'a dyn Fn() -> Result, } impl<'a> Admission<'a> { pub(super) fn new(deadline: u64, check: &'a dyn Fn() -> Result) -> Self { - Self { deadline, check } + Self { deadline: Cell::new(deadline), started: super::now_ns(), check } + } + + /// A recovery is pending or this owner's spared work drains: wait as long as the + /// watchdog may take to settle it (RECOVERY_WAIT_MS from the start) instead of failing. + pub(super) fn extend_for_recovery(&self) { + let limit = self + .started + .saturating_add(u64::from(super::timeout::RECOVERY_WAIT_MS) * 1_000_000); + if self.deadline.get() < limit { + self.deadline.set(limit); + } } pub(super) fn check(&self) -> Result { (self.check)()?; - if super::now_ns() >= self.deadline { return Err(EBUSY); } + if super::now_ns() >= self.deadline.get() { return Err(EBUSY); } Ok(()) } /// Ignore condvar remaining-time reports: only the monotonic deadline counts. pub(super) fn slice_ms(&self) -> Result { self.check()?; - let remaining = self.deadline.saturating_sub(super::now_ns()); + let remaining = self.deadline.get().saturating_sub(super::now_ns()); if remaining == 0 { return Err(EBUSY); } Ok(remaining.div_ceil(1_000_000).min(10) as u32) } @@ -268,6 +281,20 @@ mod growth_gate_tests { mod compute_deadline_tests { use super::*; + #[test] + fn a_recovery_extends_the_deadline_once_from_the_start() { + let check = || Ok(()); + let admission = Admission::new(super::super::now_ns(), &check); + assert_eq!(admission.check(), Err(EBUSY)); + admission.extend_for_recovery(); + assert_eq!(admission.check(), Ok(())); + let limit = admission.started + + u64::from(super::super::timeout::RECOVERY_WAIT_MS) * 1_000_000; + assert_eq!(admission.deadline.get(), limit); + admission.extend_for_recovery(); + assert_eq!(admission.deadline.get(), limit); + } + #[test] fn spent_budget_refuses_even_an_open_gate() { let gate = Gate::new().unwrap(); diff --git a/drivers/gpu/drm/asahi/g17/queue/admission.rs b/drivers/gpu/drm/asahi/g17/queue/admission.rs index ebdc79d2b2dd3..c9421419f9327 100644 --- a/drivers/gpu/drm/asahi/g17/queue/admission.rs +++ b/drivers/gpu/drm/asahi/g17/queue/admission.rs @@ -403,6 +403,16 @@ impl Pool { } } } + /// The owner's pair is quarantined behind another VM's recovery (spared) and drains. + pub(crate) fn spared_quarantined(&self, owner: u64) -> bool { + let state = self.state.lock(); + state + .owners + .iter() + .position(|id| *id == owner) + .is_some_and(|slot| state.quarantined & state.spared & (1u64 << slot) != 0) + } + pub(crate) fn acquire_render( self: &Arc, owner: u64, @@ -434,17 +444,22 @@ impl Pool { .position(|id| *id == owner) .is_some_and(|slot| state.quarantined & (1u64 << slot) != 0) { - if now >= quarantine_deadline { - // Behind an innocent pair's quarantine (another VM wedged the GPU) only this - // publication fails, not the VM: it recovers once the pair drains. - let spared = state - .owners - .iter() - .position(|id| *id == owner) - .is_some_and(|slot| state.spared & (1u64 << slot) != 0); - return Err(if spared { EBUSY } else { EIO }); + // Behind an innocent pair's quarantine (another VM wedged the GPU) wait for the + // pair to drain until the caller's deadline, which the caller extends while it + // drains (RECOVERY_WAIT_MS); a guilty pair's publication fails after a second. + let spared = state + .owners + .iter() + .position(|id| *id == owner) + .is_some_and(|slot| state.spared & (1u64 << slot) != 0); + if now >= quarantine_deadline && !spared { + return Err(EIO); } - let slice = remaining_ms.min(wait_slice_ms(quarantine_deadline, now)?); + let slice = if spared { + remaining_ms + } else { + remaining_ms.min(wait_slice_ms(quarantine_deadline, now)?) + }; if matches!(self.released.wait_interruptible_timeout( &mut state, kernel::time::msecs_to_jiffies(slice) ), CondVarTimeoutResult::Signal { .. }) { @@ -785,10 +800,13 @@ mod fault_capacity_tests { let queue = pool.reserve_queue().unwrap(); let lease = pool.acquire_render(1, None, u64::MAX, || Ok(())).unwrap(); let bit = 1u64 << lease.slot(); + assert!(!pool.spared_quarantined(1)); lease.quarantine(true); assert_ne!(pool.state.lock().spared & bit, 0); + assert!(pool.spared_quarantined(1)); lease.quarantine(false); assert_eq!(pool.state.lock().spared & bit, 0); + assert!(!pool.spared_quarantined(1)); // Once guilty, a later spared failure does not make it spared again. lease.quarantine(true); assert_eq!(pool.state.lock().spared & bit, 0); diff --git a/drivers/gpu/drm/asahi/g17/queue/compute.rs b/drivers/gpu/drm/asahi/g17/queue/compute.rs index 713809907dd4b..f641220f56fc9 100644 --- a/drivers/gpu/drm/asahi/g17/queue/compute.rs +++ b/drivers/gpu/drm/asahi/g17/queue/compute.rs @@ -1663,8 +1663,10 @@ impl Queue { else { return Ok(None); }; + // Spared work dropped by another VM's recovery, like a failed producer below, is + // nothing to wait for and does not cancel its consumer (Packet::check_dependencies). if self.quarantined && self.spared_quarantine { - return Err(ECANCELED); + return Ok(None); } if !Arc::ptr_eq(active.packet.completion.status(), status) || self.quarantined @@ -1674,12 +1676,7 @@ impl Queue { } let ticket = active.ticket.ok_or(EIO)?; let state = crate::g17::job::fence_status(&active.packet.completion.fence()); - if state < 0 { - return Err(crate::g17::dependency::inherited_error(Error::from_errno( - state, - ))); - } - if state > 0 { + if state != 0 { return Ok(None); } active.packet.completion.note_firmware_consumer(); diff --git a/drivers/gpu/drm/asahi/g17/queue/mod.rs b/drivers/gpu/drm/asahi/g17/queue/mod.rs index 6a9b15789339f..cc3b5799446ef 100644 --- a/drivers/gpu/drm/asahi/g17/queue/mod.rs +++ b/drivers/gpu/drm/asahi/g17/queue/mod.rs @@ -96,7 +96,6 @@ pub(crate) struct Queue { proof: Proof, sequence: [u64; 2], frontiers: [KVec<(u64, Fences)>; 2], - failures: [Option<(u64, Fence)>; 2], // Closing the userspace queue releases admission even if firmware still // retains one of its execution contexts or installed physical graphs. _slot: admission::QueueSlot, @@ -134,7 +133,6 @@ impl Queue { proof: Proof::new(), sequence: [0; 2], frontiers: core::array::from_fn(|_| KVec::new()), - failures: [None, None], _slot: slot, }) } @@ -153,20 +151,10 @@ impl Queue { Ok(()) } + /// Drop every producer that has signalled. A failed one does not fail later + /// commands (Packet::check_dependencies), so nothing of it is retained. fn prune_frontier(&mut self, index: usize) { - let failure = &mut self.failures[index]; - self.frontiers[index].retain(|(sequence, fences)| { - let status = job::fence_status(&fences.completed); - if status == 0 { - return true; - } - // One earliest failed ordinal covers every later prefix without - // retaining its packet, mappings, or every failed fence. - if status < 0 && failure.as_ref().is_none_or(|(first, _)| *sequence < *first) { - *failure = Some((*sequence, fences.completed.clone())); - } - false - }); + self.frontiers[index].retain(|(_, fences)| job::fence_status(&fences.completed) == 0); } #[expect( @@ -249,21 +237,10 @@ impl Queue { .iter() .filter(|(_, fences)| fences.ready.raw() == fences.completed.raw()) .count(); - let failures: [Option<&Fence>; 2] = core::array::from_fn(|index| { - self.failures[index].as_ref().and_then(|(sequence, fence)| { - Order::contains(order.wait_through[index], *sequence).then_some(fence) - }) - }); - let failure_count = failures.iter().flatten().count(); let mut dependencies = - KVec::with_capacity(inputs.len() + count + timestamp_count + failure_count, GFP_KERNEL)?; - let mut checked_inputs = - KVec::with_capacity(inputs.len() + timestamp_count + failure_count, GFP_KERNEL)?; + KVec::with_capacity(inputs.len() + count + timestamp_count, GFP_KERNEL)?; + let mut checked_inputs = KVec::with_capacity(inputs.len() + timestamp_count, GFP_KERNEL)?; let mut firmware = KVec::with_capacity(count, GFP_KERNEL)?; - for fence in failures.into_iter().flatten() { - checked_inputs.push(fence.clone(), GFP_KERNEL)?; - dependencies.push(fence.clone(), GFP_KERNEL)?; - } // Same-engine parents order GPU execution, not the host's timestamp // writes. Only timestamp publishers need these completion waits and // status checks; ordinary compute keeps its existing queue ordering. @@ -274,24 +251,23 @@ impl Queue { } } for (_, fences) in frontier { - // Successful completion includes host timestamp publication. Its - // ready fence adds no ordering and would prevent shallow inline - // publication. Failed and pending producers still need checking. - if job::fence_status(&fences.completed) > 0 { + // Completion includes host timestamp publication. A signalled producer's + // ready fence adds no ordering and would prevent shallow inline publication; + // a failed one does not fail this command. Pending producers order it. + if job::fence_status(&fences.completed) != 0 { continue; } dependencies.push(fences.ready.clone(), GFP_KERNEL)?; firmware.push(fences.completed.clone(), GFP_KERNEL)?; } - // Each command owns every input fence; independent engines may publish in - // either order and must each consume an input's error status before running. + // Each command owns every input fence it still has to wait for. for sync in inputs { if let Some(fence) = &sync.fence { - // An input that already signalled without error orders nothing and has no status - // left to consume (like a completed frontier producer above). Keeping it would - // send the render through the scheduler thread instead of inline publication: - // most compositor and client renders carry such already-signalled buffer fences. - if job::fence_status(fence) > 0 { + // An input that already signalled orders nothing, and a failed one does not fail + // this command (like a signalled frontier producer above). Keeping it would send + // the render through the scheduler thread instead of inline publication: most + // compositor and client renders carry such already-signalled buffer fences. + if job::fence_status(fence) != 0 { continue; } dependencies.push(fence.clone(), GFP_KERNEL)?; diff --git a/drivers/gpu/drm/asahi/g17/recovery/safety.rs b/drivers/gpu/drm/asahi/g17/recovery/safety.rs index c30e45e22579a..0c84c4612dd29 100644 --- a/drivers/gpu/drm/asahi/g17/recovery/safety.rs +++ b/drivers/gpu/drm/asahi/g17/recovery/safety.rs @@ -78,7 +78,7 @@ mod tests { use crate::g17::dependency::replay_status; assert_eq!(replay_status(0), Err(EBUSY)); assert_eq!(replay_status(1), Ok(())); - assert_eq!(replay_status(EIO.to_errno()), Err(ECANCELED)); - assert_eq!(replay_status(ETIMEDOUT.to_errno()), Err(ECANCELED)); + assert_eq!(replay_status(EIO.to_errno()), Ok(())); + assert_eq!(replay_status(ETIMEDOUT.to_errno()), Ok(())); } } diff --git a/drivers/gpu/drm/asahi/g17/runtime.rs b/drivers/gpu/drm/asahi/g17/runtime.rs index 6f4a5355d6eb3..515f81cf8a5a5 100644 --- a/drivers/gpu/drm/asahi/g17/runtime.rs +++ b/drivers/gpu/drm/asahi/g17/runtime.rs @@ -31,7 +31,6 @@ use kernel::{prelude::*, sync::Arc}; const QIDS: usize = 128; const FIRST_COMPUTE_QID: u8 = 4; -const RECOVERY_SETTLE_POLLS: usize = 200; const WORK_RING_WAIT_NS: u64 = super::job::TIMEOUT_MS as u64 * 1_000_000; /// A reservation remains identifiable while its backing is built without the @@ -1287,9 +1286,8 @@ impl Backend { admission.check()?; let required = super::fw::compute_pool::required_blocks(self.shared.active_cores, scs_pages)?; use kernel::time::{delay::fsleep, Delta}; - const SPARED_WAIT_MS: usize = 200; let mut deferred = DeferredBatch::publication()?; - for _ in 0..SPARED_WAIT_MS { + loop { admission.check()?; let pending = (|| { let mut state = self.shared.state.lock(); @@ -1303,18 +1301,20 @@ impl Backend { let pending = pending?; if !pending { match self.build_compute(required, admission) { - Err(error) if error == EAGAIN => continue, + Err(error) if error == EAGAIN => { + fsleep(Delta::from_millis(1)); + continue; + } result => return result, } } - // Other owners continue submitting while this owner gives its - // queue-local drain witness time to arrive. + // Other owners continue submitting while this owner gives its queue-local drain + // witness time to arrive. No new command has been accepted; if the admission + // deadline passes, the spared queue and its retained work stay intact until a later + // admission observes its witness. + admission.extend_for_recovery(); fsleep(Delta::from_millis(1)); } - // No new command has been accepted. Keep the spared queue and its - // retained work intact until a later admission observes its witness. - admission.check()?; - Err(EBUSY) } /// Called after taking this packet's publication claim. All waits and work @@ -1326,7 +1326,6 @@ impl Backend { ) -> Result { packet.check_dependencies()?; let super::command::Validated::Compute { scratch, .. } = &packet.command else { return Err(EINVAL); }; - let mut recovery_polls = 0; let mut state_delay = 20i64; let deadline = now_ns().saturating_add(WORK_RING_WAIT_NS); let check = || { @@ -1376,10 +1375,7 @@ impl Backend { } let recovery_state = super::recovery::Memory::recovery_state(&firmware.init)?; if firmware.recovery.pending() || recovery_state != 0 { - if recovery_polls == RECOVERY_SETTLE_POLLS { - return Err(EBUSY); - } - recovery_polls += 1; + admission.extend_for_recovery(); drop(state); drop(lease); self.shared.queue_events(); @@ -1405,10 +1401,7 @@ impl Backend { && (firmware.recovery.pending() || super::recovery::Memory::recovery_state(&firmware.init)? != 0) { - if recovery_polls == RECOVERY_SETTLE_POLLS { - return Err(EBUSY); - } - recovery_polls += 1; + admission.extend_for_recovery(); drop(state); drop(lease); self.shared.queue_events(); @@ -1609,9 +1602,9 @@ impl Registry { } impl Backend { - pub(super) fn wait_capacity(&self, started: u64) -> Result { + pub(super) fn wait_capacity(&self, deadline_ns: u64) -> Result { let check = || self.admission_open(); - let admission = Admission::new(started.saturating_add(WORK_RING_WAIT_NS), &check); + let admission = Admission::new(deadline_ns, &check); self.wait_compute_capacity(&admission) } diff --git a/drivers/gpu/drm/asahi/g17/runtime/render/submit.rs b/drivers/gpu/drm/asahi/g17/runtime/render/submit.rs index b77fc13b7ed2f..c4dde941c280f 100644 --- a/drivers/gpu/drm/asahi/g17/runtime/render/submit.rs +++ b/drivers/gpu/drm/asahi/g17/runtime/render/submit.rs @@ -59,10 +59,18 @@ impl Backend { (*state).as_deref().ok_or(ENODEV)?.queues.admission.clone() }; let started = super::super::now_ns(); - let deadline_ns = started.saturating_add(u64::from(crate::g17::job::TIMEOUT_MS) * 1_000_000); + let mut deadline_ns = started.saturating_add(u64::from(crate::g17::job::TIMEOUT_MS) * 1_000_000); + let recovery_deadline_ns = started.saturating_add(recovery_wait_ns()); + let mut capacity = WaitClock::default(); loop { self.admission_open()?; - if super::super::now_ns() >= deadline_ns { return Err(EBUSY); } + if super::super::now_ns() >= deadline_ns { + if deadline_ns < recovery_deadline_ns && self.waiting_for_recovery()? { + deadline_ns = recovery_deadline_ns; + } else { + return Err(EBUSY); + } + } if packet.cancel_requested() { return Err(ECANCELED); } @@ -97,11 +105,13 @@ impl Backend { .reclaim_unused_render_spare(Some(self.owner), false)? }; if reclaimed.is_none() { - self.wait_render_capacity(started, sleep)?; + self.wait_render_capacity(&mut capacity, sleep)?; } drop(reclaimed); continue; } + // The deadline check above decides whether a recovery extends the wait. + Err(error) if error == EBUSY => continue, Err(error) => return Err(error), } }; @@ -131,7 +141,7 @@ impl Backend { let ready = firmware.queues.has_render_ids(self.owner); drop(state); if reclaimed.is_none() && !ready { - self.wait_render_capacity(started, sleep)?; + self.wait_render_capacity(&mut capacity, sleep)?; } drop(reclaimed); } @@ -140,31 +150,62 @@ impl Backend { } } + /// A recovery is pending, or this owner's pair is spared and draining after one: an + /// innocent publication waits up to RECOVERY_WAIT_MS for it instead of failing. + fn waiting_for_recovery(&self) -> Result { + let state = self.shared.state.lock(); + let firmware = (*state).as_deref().ok_or(ENODEV)?; + Ok(firmware.recovery.pending() + || crate::g17::recovery::Memory::recovery_state(&firmware.init)? != 0 + || firmware.queues.admission.spared_quarantined(self.owner)) + } + + /// Whether the wait `clock` measures gives up: after `bound_ns` counted from the later of + /// its first poll and the last poll that saw a recovery pending or this owner's spared pair + /// draining (so the usual bound applies afresh once a recovery ends), and after + /// RECOVERY_WAIT_MS from its first poll in any case. + fn wait_expired(&self, clock: &mut WaitClock, bound_ns: u64) -> Result { + let now = super::super::now_ns(); + let first = *clock.first.get_or_insert(now); + clock.calm = clock.calm.max(first); + if self.waiting_for_recovery()? { + clock.calm = now; + } + Ok(now.saturating_sub(first) >= recovery_wait_ns() + || now.saturating_sub(clock.calm) >= bound_ns) + } + /// A pair is borrowed, still settling, or waiting for the release of the /// scheduler it last ran for to be consumed and the pair parked. The event /// worker observes both, so kick it on every poll; time out only after the /// same bound as a capacity wait. - fn wait_render_busy(&self, since: &mut Option, sleep: bool) -> Result { + fn wait_render_busy(&self, clock: &mut WaitClock, sleep: bool) -> Result { if !sleep { return Err(FALLBACK); } - let now = super::super::now_ns(); - if now.saturating_sub(*since.get_or_insert(now)) >= super::super::WORK_RING_WAIT_NS { + if self.wait_expired(clock, super::super::WORK_RING_WAIT_NS)? { return Err(ETIMEDOUT); } self.shared.queue_events(); - fsleep(Delta::from_micros(200)); + // Past the usual bound this waits for a recovery: poll gently. + let first = clock.first.unwrap_or(0); + let long = super::super::now_ns().saturating_sub(first) >= super::super::WORK_RING_WAIT_NS; + fsleep(if long { Delta::from_millis(1) } else { Delta::from_micros(200) }); Ok(()) } /// A closing owner may still hold its pair until exact retirement. Wait /// outside both the device mutex and preparation gate so the event worker /// and recovery can return capacity. No packet is firmware-visible here. - fn wait_render_capacity(&self, started: u64, sleep: bool) -> Result { + fn wait_render_capacity(&self, clock: &mut WaitClock, sleep: bool) -> Result { if !sleep { return Err(FALLBACK); } - self.wait_capacity(started) + if self.wait_expired(clock, super::super::WORK_RING_WAIT_NS)? { + return Err(EBUSY); + } + // One reclaim-and-sleep pass; the clock above bounds the wait. + self.wait_capacity(super::super::now_ns().saturating_add(super::super::WORK_RING_WAIT_NS)) } fn render_with_lease( @@ -188,11 +229,11 @@ impl Backend { let admission = self.acquire_render_with_ids(packet, early, sleep)?; let slot = admission.slot(); packet.completion.attach_render_lease(admission)?; - let mut state_polls = 0; + let mut state_clock = WaitClock::default(); let mut state_delay = 20i64; - let mut busy_since = None; - let mut recovery_polls = 0; - let capacity_started = super::super::now_ns(); + let mut busy = WaitClock::default(); + let mut recovery_since = None; + let mut capacity = WaitClock::default(); loop { if packet.cancel_requested() { return Err(ECANCELED); @@ -223,10 +264,10 @@ impl Backend { if !sleep { return Err(FALLBACK); } - if state_polls == 1000 { + // Work states free as earlier, possibly spared, work retires. + if self.wait_expired(&mut state_clock, 1_000_000_000)? { return Err(ETIMEDOUT); } - state_polls += 1; fsleep(Delta::from_micros(state_delay)); state_delay = (state_delay * 2).min(1000); continue; @@ -237,40 +278,34 @@ impl Backend { } match self.ensure_render(packet, slot, sleep) { Err(error) if error == ENOSPC => { - self.wait_render_capacity(capacity_started, sleep)?; + self.wait_render_capacity(&mut capacity, sleep)?; continue; } Err(error) if error == EAGAIN => continue, Err(error) if error == EBUSY => { - self.wait_render_busy(&mut busy_since, sleep)?; + self.wait_render_busy(&mut busy, sleep)?; continue; } result => result?, } match self.prepare_render(slot, packet, deferred, sleep) { Err(error) if !packet.is_published() && error == ENOSPC => { - self.wait_render_capacity(capacity_started, sleep)?; + self.wait_render_capacity(&mut capacity, sleep)?; continue; } Err(error) if !packet.is_published() && [EAGAIN, EBUSY].contains(&error) => { - let recovering = { - let state = self.shared.state.lock(); - let firmware = (*state).as_deref().ok_or(ENODEV)?; - firmware.recovery.pending() - || crate::g17::recovery::Memory::recovery_state(&firmware.init)? != 0 - }; - if recovering { + if self.waiting_for_recovery()? { if !sleep { return Err(FALLBACK); } - if recovery_polls == super::super::RECOVERY_SETTLE_POLLS { + let since = *recovery_since.get_or_insert(super::super::now_ns()); + if super::super::now_ns().saturating_sub(since) >= recovery_wait_ns() { return Err(ETIMEDOUT); } - recovery_polls += 1; self.shared.queue_events(); fsleep(Delta::from_millis(1)); } else if error == EBUSY { - self.wait_render_busy(&mut busy_since, sleep)?; + self.wait_render_busy(&mut busy, sleep)?; } else if !sleep { return Err(FALLBACK); } @@ -505,3 +540,14 @@ impl crate::g17::Firmware { result } } + +fn recovery_wait_ns() -> u64 { + u64::from(crate::g17::timeout::RECOVERY_WAIT_MS) * 1_000_000 +} + +/// One bounded wait (Backend::wait_expired). +#[derive(Default)] +struct WaitClock { + first: Option, + calm: u64, +} diff --git a/drivers/gpu/drm/asahi/g17/timeout.rs b/drivers/gpu/drm/asahi/g17/timeout.rs index d951bd164d6e9..8215be8c83009 100644 --- a/drivers/gpu/drm/asahi/g17/timeout.rs +++ b/drivers/gpu/drm/asahi/g17/timeout.rs @@ -18,6 +18,13 @@ pub(crate) fn transaction_grace(renewals: u8) -> bool { renewals < MAX_RENEWALS pub(crate) const POLL_MS: u32 = 2000; const EXECUTION_RENEWALS: u8 = 4; +/// How long an innocent client's publication waits for a recovery to settle and its +/// spared work to drain, instead of failing with EBUSY: the watchdog defers work behind +/// another VM at most this long before it settles it (`classify`; ray work gets the +/// execution renewals on top). +pub(crate) const RECOVERY_WAIT_MS: u32 = + POLL_MS * (1 + EXECUTION_RENEWALS as u32 + MAX_RENEWALS as u32); + /// A background control/replay poll has no scheduler fence to expire it. /// Its timestamp belongs to the exact transaction and is never renewed by work. pub(crate) fn background_stalled(since: Option, now: u64) -> bool { diff --git a/include/uapi/drm/asahi_drm.h b/include/uapi/drm/asahi_drm.h index 66396f89cc333..4d15d6524d602 100644 --- a/include/uapi/drm/asahi_drm.h +++ b/include/uapi/drm/asahi_drm.h @@ -318,11 +318,12 @@ enum drm_asahi_feature { /** * @DRM_ASAHI_FEATURE_PER_COMMAND_INPUT_SYNCS: Every hardware command of - * a submission waits for all of the submission's input syncs, including - * their errors, so independent commands need no extra dependency on the - * first command. Userspace that splits a batch across several - * submissions and relies on this must pass its input syncs to each of - * them. + * a submission waits for all of the submission's input syncs, so + * independent commands need no extra dependency on the first command. + * An input that signalled with an error is waited for, not inherited: as + * with drm_sched, the command still runs, and its inputs' contents are + * undefined. Userspace that splits a batch across several submissions + * and relies on this must pass its input syncs to each of them. */ DRM_ASAHI_FEATURE_PER_COMMAND_INPUT_SYNCS = (1UL) << 10, @@ -391,7 +392,10 @@ enum drm_asahi_feature { * The driver publishes an accepted job's failure before signaling its fence. * Thus userspace must query after a successful DRM syncobj wait/poll before * treating that fence as successful execution. Syncobj signal alone is not - * an execution-success indication. + * 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. * 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 986ec63833586a1451575a4fb05b3d2b95679a53 Mon Sep 17 00:00:00 2001 From: Ryan Murray Date: Sun, 25 Oct 2026 11:05:56 +1100 Subject: [PATCH 2/2] drm/asahi: g17: keep dependency failures ordered across recovery A failed fence is not proof that its firmware command has retired. Quarantined render and compute work can still own memory, and an abandoned render can replay. Dropping that dependency lets its consumer execute before the retained producer, then lets the producer overwrite the consumer's result after replay. Keep the longer recovery waits and avoid reporting ECANCELED as device loss, but retain cancellation of commands with failed inputs or internal predecessors. Restore error propagation through ordered output publication and replay admission as well. The cancellation reaches the fence without poisoning an innocent VM, while no successful consumer can bypass a retained failed producer. Keep the existing input-sync UAPI contract. Add a regression case for failed predecessor admission. Independent work can still proceed; making failed-input consumers execute needs a separate retirement guarantee first. Assisted-by: LLM Signed-off-by: Ryan Murray --- .../drm/asahi/g17/completion/publication.rs | 67 +++++++++++++------ drivers/gpu/drm/asahi/g17/dependency.rs | 28 ++++---- drivers/gpu/drm/asahi/g17/job.rs | 21 ++++-- drivers/gpu/drm/asahi/g17/queue/compute.rs | 11 +-- drivers/gpu/drm/asahi/g17/queue/mod.rs | 57 +++++++++++----- drivers/gpu/drm/asahi/g17/recovery/safety.rs | 4 +- .../drm/asahi/g17/runtime/render/submit.rs | 3 +- include/uapi/drm/asahi_drm.h | 11 ++- 8 files changed, 134 insertions(+), 68 deletions(-) diff --git a/drivers/gpu/drm/asahi/g17/completion/publication.rs b/drivers/gpu/drm/asahi/g17/completion/publication.rs index 2ef21401e355e..fc4b4f8739015 100644 --- a/drivers/gpu/drm/asahi/g17/completion/publication.rs +++ b/drivers/gpu/drm/asahi/g17/completion/publication.rs @@ -8,13 +8,14 @@ //! nor waits for another command; the last prerequisite publishes a ready result. use super::Output; -use crate::g17::status::VmStatus; +use crate::g17::{dependency::inherited_error, status::VmStatus}; use kernel::{new_mutex, prelude::*, sync::{Arc, Mutex}}; type Links = Option>; struct Link { target: Arc, + result: Result, next: Links, // Reuse an already-consumed link as a traversal frame instead of recursing // or allocating when publication unlocks a long chain of ready successors. @@ -24,8 +25,9 @@ struct Link { struct State { remaining: usize, result: Option>, + predecessor_error: Option, claimed: bool, - published: bool, + published: Option, successors: Links, } @@ -36,9 +38,10 @@ impl State { } let result = self.result.take()?; self.claimed = true; - // A predecessor's failure orders this publication but does not fail it: the - // command ran (Packet::check_dependencies), and a guilty VM's status is sticky. - Some(result) + Some(match (result, self.predecessor_error) { + (Ok(_), Some(error)) => Err(error), + (result, _) => result, + }) } } @@ -60,8 +63,9 @@ impl Publication { // Acceptance installs all links before releasing this sentinel. remaining: 1, result: None, + predecessor_error: None, claimed: false, - published: false, + published: None, successors: None, }, "G17 host timestamp publication"), }), @@ -90,23 +94,38 @@ impl Publication { fn publish(&self, result: Result<[u64; 4]>) -> Links { if let Err(error) = result { - // As in Completion::complete(): a cancellation is not this client's device loss. + // Inherited failure is visible to the client without inventing a + // permanent admission failure in this command's VM. if error != ECANCELED { self.status.report_failure(error); } } self.output.publish(result); - let mut state = self.state.lock(); - // A late link is acknowledged only after timestamp writes, the public - // fence and aggregate membership have all been published off this lock. - state.published = true; - state.successors.take() + let result = result.map(|_| ()); + let mut links = { + let mut state = self.state.lock(); + // A late link is acknowledged only after timestamp writes, the public + // fence and aggregate membership have all been published off this lock. + state.published = Some(result); + state.successors.take() + }; + let mut next = links.as_mut(); + while let Some(link) = next { + link.result = result; + next = link.next.as_mut(); + } + links } - fn predecessor_done(&self) -> Links { + fn predecessor_done(&self, result: Result) -> Links { let ready = { let mut state = self.state.lock(); debug_assert!(state.remaining != 0); + if let Err(error) = result { + if state.predecessor_error.is_none() { + state.predecessor_error = Some(inherited_error(error)); + } + } state.remaining -= 1; state.claim() }; @@ -114,15 +133,18 @@ impl Publication { } fn add_successor(&self, mut link: KBox) { - { + let published = { let mut state = self.state.lock(); - if !state.published { - link.next = state.successors.take(); - state.successors = Some(link); - return; + match state.published { + Some(result) => result, + None => { + link.next = state.successors.take(); + state.successors = Some(link); + return; + } } - } - Self::drain(link.target.predecessor_done()); + }; + Self::drain(link.target.predecessor_done(published)); } /// Iterative depth-first notification, O(nodes + links), with no stack growth. @@ -131,7 +153,7 @@ impl Publication { loop { if let Some(mut link) = pending.take() { pending = link.next.take(); - let children = link.target.predecessor_done(); + let children = link.target.predecessor_done(link.result); if children.is_some() { link.resume = pending.take(); link.next = frames.take(); @@ -164,6 +186,7 @@ impl Prepared { for predecessor in predecessors { let link = KBox::new(Link { target: target.clone(), + result: Ok(()), next: None, resume: None, }, GFP_KERNEL)?; @@ -183,6 +206,6 @@ impl Prepared { for (predecessor, link) in self.links { predecessor.add_successor(link); } - Publication::drain(self.target.predecessor_done()); + Publication::drain(self.target.predecessor_done(Ok(()))); } } diff --git a/drivers/gpu/drm/asahi/g17/dependency.rs b/drivers/gpu/drm/asahi/g17/dependency.rs index ce982e3d8918c..43b0bbfdbbd94 100644 --- a/drivers/gpu/drm/asahi/g17/dependency.rs +++ b/drivers/gpu/drm/asahi/g17/dependency.rs @@ -18,11 +18,11 @@ pub(crate) fn inherited_error(error: Error) -> Error { } /// A replay's original host inputs must still be signalled. Treat an unmet -/// input as a failed replay, never as a wait under the device mutex. A failed -/// input does not fail the replay, as it did not fail the first publication -/// (`Packet::check_dependencies`). +/// input as a failed replay, never as a wait under the device mutex. pub(crate) fn replay_status(status: i32) -> Result { - if status == 0 { Err(EBUSY) } else { Ok(()) } + if status == 0 { Err(EBUSY) } + else if status < 0 { Err(inherited_error(Error::from_errno(status))) } + else { Ok(()) } } pub(crate) struct Frontier { @@ -67,8 +67,8 @@ impl Frontier { Ok(()) } - /// The caller has matched the logical owner and VM. Order this command after - /// every covered predecessor still pending on the firmware. + /// The caller has matched the logical owner and VM. Check every covered + /// predecessor for failure, including one a newer kick would subsume. pub(crate) fn include( &mut self, prefix: Option, @@ -79,9 +79,10 @@ impl Frontier { if !Order::contains(prefix, sequence) { return Ok(()); } - // A failed predecessor will never run: there is nothing to wait for. Like a failed - // input (Packet::check_dependencies), it does not cancel this command. - if status != 0 { + if status < 0 { + return Err(ECANCELED); + } + if status > 0 { return Ok(()); } let dependency = KickDependency::from_word(word).ok_or(EIO)?; @@ -94,10 +95,13 @@ mod tests { use super::*; #[test] - fn a_failed_predecessor_is_no_dependency() { + fn failed_predecessor_cannot_be_bypassed() { let mut frontier = Frontier::new(); - assert_eq!(frontier.include(Some(5), 5, ECANCELED.to_errno(), 0), Ok(())); - assert_eq!(frontier.include(Some(5), 4, EIO.to_errno(), 0), Ok(())); + for error in [ECANCELED, EIO, ETIMEDOUT] { + assert_eq!(frontier.include(Some(5), 5, error.to_errno(), 0), Err(ECANCELED)); + } + assert_eq!(frontier.include(Some(5), 5, 1, 0), Ok(())); + assert_eq!(frontier.include(Some(5), 6, EIO.to_errno(), 0), Ok(())); assert_eq!(frontier.count, 0); } } diff --git a/drivers/gpu/drm/asahi/g17/job.rs b/drivers/gpu/drm/asahi/g17/job.rs index 98599b2d816fb..3b47d361c2ef9 100644 --- a/drivers/gpu/drm/asahi/g17/job.rs +++ b/drivers/gpu/drm/asahi/g17/job.rs @@ -149,14 +149,12 @@ impl Packet { for fence in &self.inputs { super::dependency::replay_status(fence_status(fence))?; } + for fence in &self.firmware { check_status(fence)?; } Ok(()) } - /// Wait until every input has signalled, retaining the references through the wait. - /// A failed input does not fail this command: as in drm_sched, only its own state - /// decides (`cancel_requested`, its VM status). The producer's client already sees its - /// own error, and an innocent producer whose work another VM's recovery cancelled must - /// not take its consumers (a compositor, say) down with it. + /// Scheduler dependency readiness includes failed fences. Consume the actual + /// status before publication, and retain the references through that check. pub(crate) fn check_dependencies(&self) -> Result { let polls = if self.command.long_job() { 5 } else { 1 }; let deadline = super::now_ns().saturating_add(u64::from(TIMEOUT_MS) * polls * 1_000_000); @@ -180,6 +178,10 @@ impl Packet { } if waited > 0 { break; } } + check_status(fence)?; + } + for fence in &self.firmware { + check_status(fence)?; } Ok(()) } @@ -247,6 +249,15 @@ pub(crate) fn fence_status(fence: &Fence) -> i32 { unsafe { bindings::dma_fence_get_status(fence.raw()) } } +fn check_status(fence: &Fence) -> Result { + let status = fence_status(fence); + if status < 0 { + Err(super::dependency::inherited_error(Error::from_errno(status))) + } else { + Ok(()) + } +} + /// The backend retains visible packets through exact retirement or processor stop. /// It performs all completion signalling after releasing the device mutex. pub(crate) trait Backend: Send + Sync { diff --git a/drivers/gpu/drm/asahi/g17/queue/compute.rs b/drivers/gpu/drm/asahi/g17/queue/compute.rs index f641220f56fc9..713809907dd4b 100644 --- a/drivers/gpu/drm/asahi/g17/queue/compute.rs +++ b/drivers/gpu/drm/asahi/g17/queue/compute.rs @@ -1663,10 +1663,8 @@ impl Queue { else { return Ok(None); }; - // Spared work dropped by another VM's recovery, like a failed producer below, is - // nothing to wait for and does not cancel its consumer (Packet::check_dependencies). if self.quarantined && self.spared_quarantine { - return Ok(None); + return Err(ECANCELED); } if !Arc::ptr_eq(active.packet.completion.status(), status) || self.quarantined @@ -1676,7 +1674,12 @@ impl Queue { } let ticket = active.ticket.ok_or(EIO)?; let state = crate::g17::job::fence_status(&active.packet.completion.fence()); - if state != 0 { + if state < 0 { + return Err(crate::g17::dependency::inherited_error(Error::from_errno( + state, + ))); + } + if state > 0 { return Ok(None); } active.packet.completion.note_firmware_consumer(); diff --git a/drivers/gpu/drm/asahi/g17/queue/mod.rs b/drivers/gpu/drm/asahi/g17/queue/mod.rs index cc3b5799446ef..dfd6fa30c74bd 100644 --- a/drivers/gpu/drm/asahi/g17/queue/mod.rs +++ b/drivers/gpu/drm/asahi/g17/queue/mod.rs @@ -96,6 +96,7 @@ pub(crate) struct Queue { proof: Proof, sequence: [u64; 2], frontiers: [KVec<(u64, Fences)>; 2], + failures: [Option<(u64, Fence)>; 2], // Closing the userspace queue releases admission even if firmware still // retains one of its execution contexts or installed physical graphs. _slot: admission::QueueSlot, @@ -133,6 +134,7 @@ impl Queue { proof: Proof::new(), sequence: [0; 2], frontiers: core::array::from_fn(|_| KVec::new()), + failures: [None, None], _slot: slot, }) } @@ -151,10 +153,20 @@ impl Queue { Ok(()) } - /// Drop every producer that has signalled. A failed one does not fail later - /// commands (Packet::check_dependencies), so nothing of it is retained. fn prune_frontier(&mut self, index: usize) { - self.frontiers[index].retain(|(_, fences)| job::fence_status(&fences.completed) == 0); + let failure = &mut self.failures[index]; + self.frontiers[index].retain(|(sequence, fences)| { + let status = job::fence_status(&fences.completed); + if status == 0 { + return true; + } + // One earliest failed ordinal covers every later prefix without + // retaining its packet, mappings, or every failed fence. + if status < 0 && failure.as_ref().is_none_or(|(first, _)| *sequence < *first) { + *failure = Some((*sequence, fences.completed.clone())); + } + false + }); } #[expect( @@ -237,10 +249,22 @@ impl Queue { .iter() .filter(|(_, fences)| fences.ready.raw() == fences.completed.raw()) .count(); - let mut dependencies = - KVec::with_capacity(inputs.len() + count + timestamp_count, GFP_KERNEL)?; - let mut checked_inputs = KVec::with_capacity(inputs.len() + timestamp_count, GFP_KERNEL)?; + let failures: [Option<&Fence>; 2] = core::array::from_fn(|index| { + self.failures[index].as_ref().and_then(|(sequence, fence)| { + Order::contains(order.wait_through[index], *sequence).then_some(fence) + }) + }); + let failure_count = failures.iter().flatten().count(); + let mut dependencies = KVec::with_capacity( + inputs.len() + count + timestamp_count + failure_count, GFP_KERNEL, + )?; + let mut checked_inputs = + KVec::with_capacity(inputs.len() + timestamp_count + failure_count, GFP_KERNEL)?; let mut firmware = KVec::with_capacity(count, GFP_KERNEL)?; + for fence in failures.into_iter().flatten() { + checked_inputs.push(fence.clone(), GFP_KERNEL)?; + dependencies.push(fence.clone(), GFP_KERNEL)?; + } // Same-engine parents order GPU execution, not the host's timestamp // writes. Only timestamp publishers need these completion waits and // status checks; ordinary compute keeps its existing queue ordering. @@ -251,23 +275,24 @@ impl Queue { } } for (_, fences) in frontier { - // Completion includes host timestamp publication. A signalled producer's - // ready fence adds no ordering and would prevent shallow inline publication; - // a failed one does not fail this command. Pending producers order it. - if job::fence_status(&fences.completed) != 0 { + // Successful completion includes host timestamp publication. Its + // ready fence adds no ordering and would prevent shallow inline + // publication. Failed and pending producers still need checking. + if job::fence_status(&fences.completed) > 0 { continue; } dependencies.push(fences.ready.clone(), GFP_KERNEL)?; firmware.push(fences.completed.clone(), GFP_KERNEL)?; } - // Each command owns every input fence it still has to wait for. + // Each command owns every input fence; independent engines may publish in + // either order and must each consume an input's error status before running. for sync in inputs { if let Some(fence) = &sync.fence { - // An input that already signalled orders nothing, and a failed one does not fail - // this command (like a signalled frontier producer above). Keeping it would send - // the render through the scheduler thread instead of inline publication: most - // compositor and client renders carry such already-signalled buffer fences. - if job::fence_status(fence) != 0 { + // An input that already signalled without error orders nothing and has no status + // left to consume (like a completed frontier producer above). Keeping it would + // send the render through the scheduler thread instead of inline publication: + // most compositor and client renders carry such already-signalled buffer fences. + if job::fence_status(fence) > 0 { continue; } dependencies.push(fence.clone(), GFP_KERNEL)?; diff --git a/drivers/gpu/drm/asahi/g17/recovery/safety.rs b/drivers/gpu/drm/asahi/g17/recovery/safety.rs index 0c84c4612dd29..c30e45e22579a 100644 --- a/drivers/gpu/drm/asahi/g17/recovery/safety.rs +++ b/drivers/gpu/drm/asahi/g17/recovery/safety.rs @@ -78,7 +78,7 @@ mod tests { use crate::g17::dependency::replay_status; assert_eq!(replay_status(0), Err(EBUSY)); assert_eq!(replay_status(1), Ok(())); - assert_eq!(replay_status(EIO.to_errno()), Ok(())); - assert_eq!(replay_status(ETIMEDOUT.to_errno()), Ok(())); + assert_eq!(replay_status(EIO.to_errno()), Err(ECANCELED)); + assert_eq!(replay_status(ETIMEDOUT.to_errno()), Err(ECANCELED)); } } diff --git a/drivers/gpu/drm/asahi/g17/runtime/render/submit.rs b/drivers/gpu/drm/asahi/g17/runtime/render/submit.rs index c4dde941c280f..035d484ed42f3 100644 --- a/drivers/gpu/drm/asahi/g17/runtime/render/submit.rs +++ b/drivers/gpu/drm/asahi/g17/runtime/render/submit.rs @@ -59,7 +59,8 @@ impl Backend { (*state).as_deref().ok_or(ENODEV)?.queues.admission.clone() }; let started = super::super::now_ns(); - let mut deadline_ns = started.saturating_add(u64::from(crate::g17::job::TIMEOUT_MS) * 1_000_000); + let mut deadline_ns = + started.saturating_add(u64::from(crate::g17::job::TIMEOUT_MS) * 1_000_000); let recovery_deadline_ns = started.saturating_add(recovery_wait_ns()); let mut capacity = WaitClock::default(); loop { diff --git a/include/uapi/drm/asahi_drm.h b/include/uapi/drm/asahi_drm.h index 4d15d6524d602..e4db0063eb937 100644 --- a/include/uapi/drm/asahi_drm.h +++ b/include/uapi/drm/asahi_drm.h @@ -318,12 +318,11 @@ enum drm_asahi_feature { /** * @DRM_ASAHI_FEATURE_PER_COMMAND_INPUT_SYNCS: Every hardware command of - * a submission waits for all of the submission's input syncs, so - * independent commands need no extra dependency on the first command. - * An input that signalled with an error is waited for, not inherited: as - * with drm_sched, the command still runs, and its inputs' contents are - * undefined. Userspace that splits a batch across several submissions - * and relies on this must pass its input syncs to each of them. + * a submission waits for all of the submission's input syncs, including + * their errors, so independent commands need no extra dependency on the + * first command. Userspace that splits a batch across several + * submissions and relies on this must pass its input syncs to each of + * them. */ DRM_ASAHI_FEATURE_PER_COMMAND_INPUT_SYNCS = (1UL) << 10,