From f5ca23b6110a4f4ec50197e79f34854aa56a325b Mon Sep 17 00:00:00 2001 From: Jonathan Goyvaerts Date: Thu, 29 Oct 2026 00:55:53 -0300 Subject: [PATCH] drm/asahi: g17: stop withholding compute doorbells A compute publication with two or more unretired commands of its queue ahead of it withholds its doorbell. Only the compute scan of a later event pass rings it, once at most one command ahead lacks a completion record. Nothing guarantees that such a pass comes: if the last event pass for the commands ahead runs before that condition holds, the firmware sits idle on a command it was never told about, and every waiter on that command stalls until an unrelated firmware message runs the scan. Under a Vulkan game in a muvm guest (DXVK on Honeykrisp, which runs buffer copies as compute) this shows up as GPU-wide stalls of 330 to 400 ms, about ten a minute under steady input. Probes on the GPU mailbox show the firmware sending nothing for the whole stall. Each one opens right after a compute publication and ends when the compute scan rings a withheld compute doorbell, after which the queued work runs at once. The compositor's own frames stall with it. The follow-up commits closed some of the ways into this state (job timeouts, recovery, a record waiting for its stamp) but not the lost wakeup itself. Remove the withholding and everything built on it, so that every compute publication rings its doorbell again. This gives back the few microseconds per hop the withholding saved in a cross-queue semaphore ping-pong. This reverts: 3ff8c49c5f12 ("drm/asahi: withhold G17 compute doorbells behind two unretired commands") b1f5dc6df0ba ("drm/asahi: drop G17 compute doorbells that another doorbell announced") d38cce2d8be5 ("drm/asahi: announce withheld G17 compute doorbells whenever retirement stalls") 7012a59ed129 ("drm/asahi: announce withheld G17 compute doorbells after a recovery") 5de99fc6f682 ("drm/asahi: keep withholding G17 compute doorbells while a stamp is awaited") 83ff3a9040e6 ("drm/asahi: restart a withheld G17 compute doorbell another doorbell covered") Fixes: 3ff8c49c5f12 ("drm/asahi: withhold G17 compute doorbells behind two unretired commands") Signed-off-by: Jonathan Goyvaerts Assisted-by: LLM --- drivers/gpu/drm/asahi/g17/port.rs | 32 ------ drivers/gpu/drm/asahi/g17/queue/compute.rs | 101 +----------------- drivers/gpu/drm/asahi/g17/recovery.rs | 5 - drivers/gpu/drm/asahi/g17/runtime.rs | 30 +----- .../drm/asahi/g17/runtime/compute_control.rs | 14 +-- drivers/gpu/drm/asahi/g17/runtime/events.rs | 20 ---- 6 files changed, 8 insertions(+), 194 deletions(-) diff --git a/drivers/gpu/drm/asahi/g17/port.rs b/drivers/gpu/drm/asahi/g17/port.rs index 8c405c8cfb90f..6d0e87c14b947 100644 --- a/drivers/gpu/drm/asahi/g17/port.rs +++ b/drivers/gpu/drm/asahi/g17/port.rs @@ -113,11 +113,6 @@ pub(super) struct Port { generation: AtomicU64, /// Device publication order shared by compute and render commands. publications: AtomicU64, - /// Compute work slots written per priority class. - compute_written: [AtomicU64; PRIORITIES], - /// `compute_written` as read before the latest compute doorbell of each class: every slot - /// counted there was in its ring when the firmware handled that doorbell. - compute_announced: [AtomicU64; PRIORITIES], /// Full firmware effort is applied; cleared before an idle clear rechecks `submissions`. effort_full: AtomicBool, demand: super::power::Demand, @@ -175,8 +170,6 @@ impl Port { render <- new_mutex!((), "g17::Port::render"), generation: AtomicU64::new(0), publications: AtomicU64::new(0), - compute_written: [const { AtomicU64::new(0) }; PRIORITIES], - compute_announced: [const { AtomicU64::new(0) }; PRIORITIES], effort_full: AtomicBool::new(false), demand, last_submission_ns: AtomicU64::new(0), @@ -258,35 +251,10 @@ impl Port { unsafe { (*ring.producer).store(next, Ordering::Relaxed) }; fence(Ordering::SeqCst); reserved[claim.priority] = reserved[claim.priority].saturating_sub(1); - self.compute_written[claim.priority].fetch_add(1, Ordering::AcqRel); claim.armed = false; Ok(next) } - /// Compute work slots written at `priority` so far. - pub(super) fn compute_written(&self, priority: u8) -> u64 { - self.compute_written - .get(usize::from(priority)) - .map_or(u64::MAX, |written| written.load(Ordering::Acquire)) - } - - /// Compute work slots at `priority` that a doorbell has announced. - pub(super) fn compute_announced(&self, priority: u8) -> u64 { - self.compute_announced - .get(usize::from(priority)) - .map_or(0, |announced| announced.load(Ordering::Acquire)) - } - - /// Rings the compute doorbell `message` of `priority`, after noting which slots it - /// announces. - pub(super) fn notify_compute(&self, priority: u8, message: u64) -> Result { - let index = usize::from(priority); - let written = self.compute_written.get(index).ok_or(EINVAL)?.load(Ordering::Acquire); - self.notify(message)?; - self.compute_announced[index].fetch_max(written, Ordering::AcqRel); - Ok(()) - } - /// Taken around every render publication; see `render`. pub(super) fn render_lock(&self) -> MutexGuard<'_, ()> { self.render.lock() diff --git a/drivers/gpu/drm/asahi/g17/queue/compute.rs b/drivers/gpu/drm/asahi/g17/queue/compute.rs index e1bda516ee102..0ec5b1797c011 100644 --- a/drivers/gpu/drm/asahi/g17/queue/compute.rs +++ b/drivers/gpu/drm/asahi/g17/queue/compute.rs @@ -369,10 +369,6 @@ use crate::g17::{ }; use retirement::{Batch, Observation, Recordless, Ticket, IN_FLIGHT}; -/// Position in its queue's active ring from which a publication withholds its doorbell: -/// with two unretired commands ahead, it cannot be the next to start. -const WITHHOLD_DEPTH: usize = 3; - struct Fifo { entries: [Option; IN_FLIGHT], head: usize, @@ -599,65 +595,9 @@ pub(crate) struct Queue { retirement_proved: bool, source_readers: Arc, source_hint: crate::g17::sync_input::ReaderHint, - /// The doorbell withheld from publications that had [`WITHHOLD_DEPTH`] - 1 or more - /// unretired commands of this queue ahead of them, the publication of the first of them - /// that no doorbell has announced, and the compute work slots written at its priority once - /// the last one's slot was. - withheld_doorbell: Option<(u64, u64, u64)>, } impl Queue { - /// Returns the withheld doorbell once at most one command of this queue that precedes the - /// first publication it covers has no completion record yet. A doorbell of the same class - /// rung after the last withheld work slot was written announced every one of them: nothing - /// is left to ring. `announced` gives the slots announced per class. - /// - /// Retirement waiting for a stamp does not hold the doorbell back, and any departure from - /// ordinary retirement (timeout, kill, quarantine, replay, recovery or exit) rings it at - /// once: the firmware then sees every published command, as without withholding. - pub(crate) fn due_doorbell(&mut self, announced: impl Fn(u8) -> u64) -> Option { - let (doorbell, first, written) = self.withheld_doorbell?; - if announced(((doorbell >> 2) & 3) as u8) >= written { - self.withheld_doorbell = None; - return None; - } - let settling = self.settling(); - let incomplete = self - .active - .iter() - .filter(|active| active.published < first && active.observed.is_none()) - .count(); - if !settling && incomplete > 1 { - return None; - } - self.withheld_doorbell = None; - Some(doorbell) - } - - /// The queue has left ordinary retirement: a timeout, kill, quarantine, replay, recovery - /// witness or exit is in progress, or retirement stopped at a command the firmware - /// finished without a completion record. A front whose record has arrived and that only - /// waits for its stamp is ordinary retirement: [`Self::due_doorbell`] counts it as - /// finished, and ringing at once there rang most withheld doorbells early. - fn settling(&self) -> bool { - self.quarantined - || self.released - || self.exit.is_some() - || self.replays.len != 0 - || self.replay_retirement - || self.awaiting_witness - || (self.retire_pending - && self.active.front().is_none_or(|active| active.observed.is_none())) - || self.retired_by_teardown - || self.spared_quarantine - || self.failure_status_pending - || self.terminal.is_some() - } - - /// Takes the withheld doorbell regardless of its predecessors. - pub(crate) fn take_withheld_doorbell(&mut self) -> Option { - self.withheld_doorbell.take().map(|(doorbell, _, _)| doorbell) - } /// The caller reserves a QID first, cancels it on construction failure, and retains every /// successfully installed queue until processor stop. Shared VM apertures precede this call. pub(crate) fn new( @@ -724,7 +664,6 @@ impl Queue { retirement_proved: false, source_readers: Arc::new(core::sync::atomic::AtomicU32::new(0), GFP_KERNEL)?, source_hint: crate::g17::sync_input::ReaderHint::new(), - withheld_doorbell: None, }) } /// The firmware failed the command whose completion stamp is `stamp`. It writes a @@ -1711,7 +1650,6 @@ impl Queue { ring: DescriptorRing, ) -> Result<(u16, u64)> { if !self.can_rearm() { return Err(EAGAIN); } - self.withheld_doorbell = None; self.graph.rearm_ring(ring)?; self.pool.rearm(pages)?; self.pool_phase = PoolPhase::Populated; @@ -2006,10 +1944,7 @@ pub(crate) trait Host { fn publish_qid(&mut self, id: kick::Id) -> Result; fn publish_outer(&mut self, priority: u8, slot: &crate::g17::fw::channels::WorkSlot) -> Result; - fn notify_compute(&mut self, priority: u8, message: u64) -> Result; - fn compute_written(&self, priority: u8) -> u64; - /// Compute work slots at `priority` that a doorbell has announced. - fn compute_announced(&self, priority: u8) -> u64; + fn notify(&mut self, message: u64) -> Result; fn note_submission(&mut self) -> Result; } @@ -2342,42 +2277,12 @@ impl Queue { previous.publication = Some((packet.order.sequence, epoch)); } } - // A publication rings its own pipe and priority class, as render + // Every publication rings its own pipe and priority class, as render // publication does: pipe 2 (compute) in bits 1:0, priority class in // bits 3:2. The generic work doorbell left the firmware to find the // ring itself and delayed dispatch. - // - // A doorbell makes the firmware read the whole work ring of its class, - // and each one it handles while finishing a command delays that - // command's completion and the release of its waiters. A command with - // two or more unretired commands of its queue ahead cannot start soon: - // its doorbell is withheld until at most one precedes it (see - // `due_doorbell`), and any doorbell rung meanwhile on the same ring - // announces its work slot earlier. fence(Ordering::SeqCst); - let doorbell = (0x83 << 48) | 0x02 | u64::from(policy.priority()) << 2; - if self.active.len >= WITHHOLD_DEPTH - && !self.settling() - && self.withheld_doorbell.is_none_or(|(withheld, _, _)| withheld == doorbell) - { - let written = host.compute_written(policy.priority()); - // Once another doorbell announced every slot the withheld one covered, it - // starts over from this publication: it then waits for this command's own - // predecessors rather than for those of slots already announced. - if self - .withheld_doorbell - .is_some_and(|(_, _, last)| host.compute_announced(policy.priority()) >= last) - { - self.withheld_doorbell = None; - } - let entry = self.withheld_doorbell.get_or_insert((doorbell, published, written)); - entry.2 = written; - } else { - host.notify_compute(policy.priority(), doorbell)?; - if self.withheld_doorbell.is_some_and(|(withheld, _, _)| withheld == doorbell) { - self.withheld_doorbell = None; - } - } + host.notify((0x83 << 48) | 0x02 | u64::from(policy.priority()) << 2)?; crate::g17::trace::publish(crate::g17::trace::COMPUTE, qid, ordinal, &packet); host.note_submission() })(); diff --git a/drivers/gpu/drm/asahi/g17/recovery.rs b/drivers/gpu/drm/asahi/g17/recovery.rs index dcd8d59e4c412..ca8da8b03545f 100644 --- a/drivers/gpu/drm/asahi/g17/recovery.rs +++ b/drivers/gpu/drm/asahi/g17/recovery.rs @@ -109,9 +109,6 @@ pub(super) trait Queues { fn settle_cancelled_renders(&mut self, qids: u128); fn rescan_compute(&mut self) -> Result; fn release_retired_compute(&mut self); - /// Ring every compute doorbell still withheld: a restart is a departure from ordinary - /// retirement, after which the firmware must see every published command. - fn announce_withheld(&mut self); } /// Live shared-memory operations. The handshake word uses aligned 32-bit accesses; transitions @@ -310,7 +307,6 @@ pub(super) fn service( } let scan_error = host.rescan_compute().err(); host.release_retired_compute(); - host.announce_withheld(); Ok(ServiceResult { scan_error, target_error }) } @@ -474,7 +470,6 @@ mod tests { fn settle_cancelled_renders(&mut self, _qids: u128) {} fn rescan_compute(&mut self) -> Result { Ok(()) } fn release_retired_compute(&mut self) {} - fn announce_withheld(&mut self) {} } impl Host for Halted { diff --git a/drivers/gpu/drm/asahi/g17/runtime.rs b/drivers/gpu/drm/asahi/g17/runtime.rs index 108c9fe19a462..a3c840337d8f1 100644 --- a/drivers/gpu/drm/asahi/g17/runtime.rs +++ b/drivers/gpu/drm/asahi/g17/runtime.rs @@ -1393,14 +1393,8 @@ impl compute::Host for ComputeHost<'_> { fn publish_outer(&mut self, _priority: u8, slot: &super::fw::channels::WorkSlot) -> Result { self.port.publish_compute(self.outer.take().ok_or(EIO)?, slot).map(|_| ()) } - fn notify_compute(&mut self, priority: u8, message: u64) -> Result { - self.port.notify_compute(priority, message) - } - fn compute_written(&self, priority: u8) -> u64 { - self.port.compute_written(priority) - } - fn compute_announced(&self, priority: u8) -> u64 { - self.port.compute_announced(priority) + fn notify(&mut self, message: u64) -> Result { + self.port.notify(message) } fn note_submission(&mut self) -> Result { self.effort.submit(); @@ -1443,14 +1437,8 @@ impl compute::Host for DirectHost<'_> { fn publish_outer(&mut self, _priority: u8, slot: &super::fw::channels::WorkSlot) -> Result { self.port.publish_compute(self.outer.take().ok_or(EIO)?, slot).map(|_| ()) } - fn notify_compute(&mut self, priority: u8, message: u64) -> Result { - self.port.notify_compute(priority, message) - } - fn compute_written(&self, priority: u8) -> u64 { - self.port.compute_written(priority) - } - fn compute_announced(&self, priority: u8) -> u64 { - self.port.compute_announced(priority) + fn notify(&mut self, message: u64) -> Result { + self.port.notify(message) } fn note_submission(&mut self) -> Result { Ok(()) @@ -1675,7 +1663,6 @@ impl super::Firmware { let port = &self.port; let retired = deferred.pending(); let mut freed = false; - let mut doorbells = [0u64; 4]; for mut queue in registry .compute .iter_mut() @@ -1692,15 +1679,6 @@ impl super::Firmware { &mut |event| deferred.push(event), )?; freed |= busy && queue.channel_free(); - // Withheld doorbells are rung once their command is next but one; one per - // priority class announces every queue's work slots on that ring. - if let Some(doorbell) = queue.due_doorbell(|class| port.compute_announced(class)) { - let class = ((doorbell >> 2) & 3) as usize; - doorbells[class] = doorbell; - } - } - for doorbell in doorbells.into_iter().filter(|doorbell| *doorbell != 0) { - self.port.notify_compute(((doorbell >> 2) & 3) as u8, doorbell)?; } // All compute observations in this batch are latched before a VM // failure can settle its render peers. Exact successes win first. diff --git a/drivers/gpu/drm/asahi/g17/runtime/compute_control.rs b/drivers/gpu/drm/asahi/g17/runtime/compute_control.rs index 42282aea62023..4f519743aed53 100644 --- a/drivers/gpu/drm/asahi/g17/runtime/compute_control.rs +++ b/drivers/gpu/drm/asahi/g17/runtime/compute_control.rs @@ -69,14 +69,7 @@ impl Backend { if !queue.owns_packet(packet) { return Ok(None); } - let status = queue.timeout(&mut |event| deferred.push(event)); - // A timed-out queue leaves ordinary retirement: announce what it withheld. - let withheld = queue.take_withheld_doorbell(); - drop(queue); - if let Some(doorbell) = withheld { - firmware.port.notify_compute(((doorbell >> 2) & 3) as u8, doorbell)?; - } - status + queue.timeout(&mut |event| deferred.push(event)) })(); if result.as_ref().is_ok_and(|status| status.is_some()) { // VM status and peer settlement precede signalling this timeout. The backend @@ -99,11 +92,6 @@ impl Backend { firmware.scan_compute(None, &mut deferred)?; if let Some(mut queue) = firmware.queues.compute_mut(self.owner) { queue.cancel(packet, &mut |event| deferred.push(event))?; - let withheld = queue.take_withheld_doorbell(); - drop(queue); - if let Some(doorbell) = withheld { - firmware.port.notify_compute(((doorbell >> 2) & 3) as u8, doorbell)?; - } } Ok(()) })(); diff --git a/drivers/gpu/drm/asahi/g17/runtime/events.rs b/drivers/gpu/drm/asahi/g17/runtime/events.rs index 74eb6b7228c72..a59f496ab4f3b 100644 --- a/drivers/gpu/drm/asahi/g17/runtime/events.rs +++ b/drivers/gpu/drm/asahi/g17/runtime/events.rs @@ -251,26 +251,6 @@ impl recovery::Queues for Service<'_> { let result = self.firmware.release_compute_witnesses(self.deferred); self.settlement_error = self.settlement_error.or(result.err()); } - fn announce_withheld(&mut self) { - let mut doorbells = [0u64; 4]; - for mut queue in self - .firmware - .queues - .compute - .iter_mut() - .flatten() - .filter_map(|entry| entry.queue.as_deref_mut()) - { - if let Some(doorbell) = queue.take_withheld_doorbell() { - doorbells[((doorbell >> 2) & 3) as usize] = doorbell; - } - } - // One doorbell per priority class announces every queue's work slots on that ring. - for doorbell in doorbells.into_iter().filter(|doorbell| *doorbell != 0) { - let result = self.firmware.port.notify_compute(((doorbell >> 2) & 3) as u8, doorbell); - self.settlement_error = self.settlement_error.or(result.err()); - } - } } impl Firmware {