From d5ee0bd28d7e75edfb2acceef113138c457e5c76 Mon Sep 17 00:00:00 2001 From: Ryan Murray Date: Sun, 25 Oct 2026 04:55:18 +1100 Subject: [PATCH] drm/asahi: keep work-queue firmware structures until the context is invalidated When a queue is destroyed while it still has jobs on the GPU, drm_sched_fini() cancels the scheduler jobs, but the firmware keeps running the commands it already holds. Each work queue then stays alive only through its event slot and is dropped from the completion path as soon as its last stamp is seen. Dropping it frees its queue info (ring state, ring and notifier list), and the last command frees the queue's notifier and threshold. Those come from the shared and private kernel allocators, so the memory can be reused right away. The GPU context that the firmware still associates those structures with is only invalidated at the next alloc(), so until then the firmware can still refer to a work queue whose memory has already been handed out. The context data already keeps the tile vertex buffer alive until it is invalidated. Do the same for the queue structures. A GpuContext now keeps a list of retained objects. A work queue moves its queue info there when it is dropped, and the queue adds its notifier when it is created. The list travels with the context data to the garbage list and is freed once DestroyContext has completed. The queue info points to the context weakly now, since the context owns it from then on. This closes the lifetime window for those two objects. On its own it does not stop the sustained-teardown firmware crash on the M1 Pro (G13X): that crash still reproduces with this change, so at least one other kernel object on the teardown path is read by the firmware after it is freed and reused. Investigation continues. Assisted-by: LLM Signed-off-by: Ryan Murray --- drivers/gpu/drm/asahi/fw/workqueue.rs | 7 +- drivers/gpu/drm/asahi/gpu.rs | 14 +- drivers/gpu/drm/asahi/queue/mod.rs | 15 +- drivers/gpu/drm/asahi/workqueue.rs | 202 +++++++++++++++++--------- 4 files changed, 160 insertions(+), 78 deletions(-) diff --git a/drivers/gpu/drm/asahi/fw/workqueue.rs b/drivers/gpu/drm/asahi/fw/workqueue.rs index b86bd3ff3757a..78ac6c676a6eb 100644 --- a/drivers/gpu/drm/asahi/fw/workqueue.rs +++ b/drivers/gpu/drm/asahi/fw/workqueue.rs @@ -152,7 +152,7 @@ pub(crate) mod raw { pub(crate) unk_94: u32, pub(crate) pending: AtomicU32, pub(crate) unk_9c: u32, - pub(crate) gpu_context: GpuPointer<'a, super::GpuContextData>, + pub(crate) gpu_context: GpuWeakPointer, pub(crate) unk_a8: U64, #[ver(V >= V13_2 && G < G14X)] pub(crate) unk_b0: u32, @@ -171,6 +171,10 @@ impl GpuStruct for GpuContextData { type Raw<'a> = raw::GpuContextData; } +/// The firmware structures of a work queue. +/// +/// The raw structure points to the queue's GPU context weakly. The `GpuContext` keeps these +/// structures until it has been invalidated, and only then frees its own data. #[versions(AGX)] #[derive(Debug)] pub(crate) struct QueueInfo { @@ -178,7 +182,6 @@ pub(crate) struct QueueInfo { pub(crate) ring: GpuArray, pub(crate) gpu_buf: GpuArray, pub(crate) notifier_list: Arc>, - pub(crate) gpu_context: Arc, } #[versions(AGX)] diff --git a/drivers/gpu/drm/asahi/gpu.rs b/drivers/gpu/drm/asahi/gpu.rs index b68381ce60b30..62c049ff7dddc 100644 --- a/drivers/gpu/drm/asahi/gpu.rs +++ b/drivers/gpu/drm/asahi/gpu.rs @@ -260,9 +260,8 @@ pub(crate) struct GpuManager { stats: Arc, buffer_mgr: buffer::BufferManager::ver, ids: SequenceIDs, - #[allow(clippy::vec_box)] #[pin] - garbage_contexts: Mutex>>>, + garbage_contexts: Mutex>, } /// Operations the DRM driver performs on a GPU, independent of its firmware interface. @@ -401,8 +400,8 @@ pub(crate) trait GpuManager: Send + Sync { fn get_cfg(&self) -> &'static hw::HwConfig; /// Get the dynamic GPU configuration for this SoC. fn get_dyncfg(&self) -> &hw::DynConfig; - /// Register an unused context as garbage - fn free_context(&self, data: KBox>); + /// Register an unused context, with the objects it retains, for invalidation. + fn free_context(&self, ctx: workqueue::RetiredContext); } /// Private generic trait for functions that don't need to escape this module. @@ -1456,7 +1455,7 @@ impl GpuManager for GpuManager::ver { core::mem::swap(&mut *self.garbage_contexts.lock(), &mut garbage_ctx); for ctx in garbage_ctx { - if self.invalidate_context(&ctx).is_err() { + if self.invalidate_context(&ctx.data).is_err() { dev_err!( self.dev.as_ref(), "GpuContext: Failed to invalidate GPU context!\n" @@ -1465,6 +1464,9 @@ impl GpuManager for GpuManager::ver { panic!("GPU firmware timed out"); } } + // The firmware no longer knows the context's queues, so their structures may go now, + // ahead of the context data they point to. + core::mem::drop(ctx.retained); } let mut guard = self.alloc.lock(); @@ -1733,7 +1735,7 @@ impl GpuManager for GpuManager::ver { &self.dyncfg } - fn free_context(&self, ctx: KBox>) { + fn free_context(&self, ctx: workqueue::RetiredContext) { let mut garbage = self.garbage_contexts.lock(); if garbage.push(ctx, GFP_KERNEL).is_err() { diff --git a/drivers/gpu/drm/asahi/queue/mod.rs b/drivers/gpu/drm/asahi/queue/mod.rs index 2b32b97b5e169..2d9052c2edb51 100644 --- a/drivers/gpu/drm/asahi/queue/mod.rs +++ b/drivers/gpu/drm/asahi/queue/mod.rs @@ -525,6 +525,15 @@ impl Queue::ver { let buffer = buffer::Buffer::ver::new(gpu, alloc, ualloc.clone(), ualloc_priv, mgr)?; + let gpu_context = Arc::new( + workqueue::GpuContext::new(dev.clone(), alloc, buffer.any_ref())?, + GFP_KERNEL, + )?; + // Every command of this queue points the firmware at the notifier, and the notifier links + // into the notifier list of the work queues. Keep it as long as they are kept, until the + // context is invalidated, instead of freeing it with the last command. + gpu_context.retain(KBox::new(notifier.clone(), GFP_KERNEL)?)?; + let mut ret = Queue::ver { dev: dev.clone(), _sched: sched, @@ -538,11 +547,7 @@ impl Queue::ver { inner: QueueInner::ver { dev: dev.clone(), ualloc, - gpu_context: Arc::new( - workqueue::GpuContext::new(dev.clone(), alloc, buffer.any_ref())?, - GFP_KERNEL, - )?, - + gpu_context, buffer, notifier_list: Arc::new(notifier_list, GFP_KERNEL)?, notifier, diff --git a/drivers/gpu/drm/asahi/workqueue.rs b/drivers/gpu/drm/asahi/workqueue.rs index 6281ef684e251..d6695a1b3ee96 100644 --- a/drivers/gpu/drm/asahi/workqueue.rs +++ b/drivers/gpu/drm/asahi/workqueue.rs @@ -95,10 +95,31 @@ impl From for kernel::error::Error { } } +/// An object the firmware may still reference while its GPU context is live. +pub(crate) type RetainedObject = KBox; + +/// Objects retained by every context: the notifier and the vertex, fragment and compute work +/// queues of the owning queue. +const CONTEXT_RETAINED_OBJECTS: usize = 4; + +/// A dropped GPU context waiting for the firmware to invalidate it. +pub(crate) struct RetiredContext { + /// The context data named in the invalidation request. + pub(crate) data: KBox>, + /// Firmware structures of the context's queues. They must not be freed, and so must not be + /// reused, until the invalidation has completed. + pub(crate) retained: KVec, +} + /// A GPU context tracking structure, which must be explicitly invalidated when dropped. +/// +/// The firmware learns about the work queues of a context as they are run, and only forgets them +/// when the context is invalidated. Their firmware structures are therefore handed to the context +/// when their owner drops them, and freed after the invalidation. pub(crate) struct GpuContext { dev: driver::AsahiDevice, data: Option>>, + retained: Pin>>>, } no_debug!(GpuContext); @@ -118,27 +139,42 @@ impl GpuContext { )?, GFP_KERNEL, )?), + retained: KBox::pin_init( + new_mutex!( + KVec::with_capacity(CONTEXT_RETAINED_OBJECTS, GFP_KERNEL)?, + "GpuContext::retained" + ), + GFP_KERNEL, + )?, }) } - /// Returns the GPU pointer to the inner GPU context data structure. - pub(crate) fn gpu_pointer(&self) -> GpuPointer<'_, fw::workqueue::GpuContextData> { - self.data.as_ref().unwrap().gpu_pointer() + /// Returns a weak GPU pointer to the inner GPU context data structure. + pub(crate) fn weak_pointer(&self) -> GpuWeakPointer { + self.data.as_ref().unwrap().weak_pointer() + } + + /// Keep `object` allocated until the firmware has invalidated this context. + pub(crate) fn retain(&self, object: RetainedObject) -> Result { + self.retained.lock().push(object, GFP_KERNEL)?; + Ok(()) } } impl Drop for GpuContext { fn drop(&mut self) { + let data = self.data.take().unwrap(); + let retained = core::mem::take(&mut *self.retained.lock()); + let Some(guard) = self.dev.registration_guard() else { return; }; guard.registration_data_with(|reg_data| { - let data = self.data.take().unwrap(); // A context only exists for the manager whose allocators created it, so the manager is // always present. if let Some(gpu) = reg_data.gpu.manager() { - gpu.free_context(data); + gpu.free_context(RetiredContext { data, retained }); } }) } @@ -237,7 +273,10 @@ impl) + Send + Sync> GenSubm struct WorkQueueInner { dev: driver::AsahiDevice, event_manager: Arc, - info: GpuObject, + /// The firmware structures of this queue. Always `Some` until the queue is dropped, when they + /// move to `gpu_context`. + info: Option>>, + gpu_context: Arc, new: bool, pipe_type: PipeType, size: u32, @@ -265,10 +304,22 @@ pub(crate) struct WorkQueue { #[versions(AGX)] impl WorkQueueInner::ver { + fn info(&self) -> &GpuObject { + self.info + .as_deref() + .expect("WorkQueue: lost its queue info") + } + + fn info_mut(&mut self) -> &mut GpuObject { + self.info + .as_deref_mut() + .expect("WorkQueue: lost its queue info") + } + /// Return the GPU done pointer, representing how many work items have been completed by the /// GPU. fn doneptr(&self) -> u32 { - self.info + self.info() .state .with(|raw, _inner| raw.gpu_doneptr.load(Ordering::Acquire)) } @@ -485,7 +536,7 @@ impl Job::ver { let next_wptr = (wptr + 1) % inner.size; assert!(inner.doneptr() != next_wptr); - inner.info.ring[wptr as usize] = command.inner.gpu_va().get(); + inner.info_mut().ring[wptr as usize] = command.inner.gpu_va().get(); wptr = next_wptr; // Cannot fail, since we did a reserve(1) above @@ -515,7 +566,7 @@ impl<'a> JobSubmission::ver<'a> { core::mem::forget(self); inner - .info + .info() .state .with(|raw, _inner| raw.cpu_wptr.store(wptr, Ordering::Release)); @@ -527,7 +578,7 @@ impl<'a> JobSubmission::ver<'a> { let msg = fw::channels::RunWorkQueueMsg::ver { pipe_type: inner.pipe_type, - work_queue: Some(inner.info.weak_pointer()), + work_queue: Some(inner.info().weak_pointer()), wptr: inner.wptr, event_slot, is_new: inner.new, @@ -617,6 +668,23 @@ impl<'a> Drop for JobSubmission::ver<'a> { } } +#[versions(AGX)] +impl Drop for WorkQueueInner::ver { + fn drop(&mut self) { + // The firmware keeps referring to this queue until its context is invalidated. When the + // queue's owner went away with work in flight, the queue is dropped as soon as that work + // completes, which can be well before the invalidation. + if let Some(info) = self.info.take() { + if self.gpu_context.retain(info).is_err() { + pr_err!( + "WorkQueue({:?}): Failed to keep the queue until its context is invalidated\n", + self.pipe_type + ); + } + } + } +} + #[versions(AGX)] impl WorkQueueInner::ver { /// Return the number of free entries in the workqueue @@ -680,61 +748,65 @@ impl WorkQueue::ver { prio.5 = 1; } + let gpu_context_pointer = gpu_context.weak_pointer(); + + // Use shared (coherent) state with verbose faults so we can dump state correctly + let info = if debug_enabled(DebugFlags::VerboseFaults) { + &mut alloc.shared + } else { + &mut alloc.private + } + .new_init( + try_init!(QueueInfo::ver { + state: { + state.with_mut(|raw, _inner| { + raw.rb_size = size; + }); + state + }, + ring, + gpu_buf, + notifier_list: notifier_list, + }), + |inner, _p| { + try_init!(raw::QueueInfo::ver { + state: inner.state.gpu_pointer(), + ring: inner.ring.gpu_pointer(), + notifier_list: inner.notifier_list.gpu_pointer(), + gpu_buf: inner.gpu_buf.gpu_pointer(), + gpu_rptr1: Default::default(), + gpu_rptr2: Default::default(), + gpu_rptr3: Default::default(), + event_id: AtomicI32::new(-1), + priority: prio, + unk_4c: -1, + uuid: id as u32, + unk_54: -1, + unk_58: Default::default(), + busy: Default::default(), + __pad: Default::default(), + #[ver(V >= V13_2 && G < G14X)] + unk_84_0: 0, + unk_84_state: Default::default(), + error_count: Default::default(), + unk_8c: 0, + unk_90: 0, + unk_94: 0, + pending: Default::default(), + unk_9c: 0, + gpu_context: gpu_context_pointer, + unk_a8: Default::default(), + #[ver(V >= V13_2 && G < G14X)] + unk_b0: 0, + }) + }, + )?; + let inner = WorkQueueInner::ver { dev: dev.into(), event_manager, - // Use shared (coherent) state with verbose faults so we can dump state correctly - info: if debug_enabled(DebugFlags::VerboseFaults) { - &mut alloc.shared - } else { - &mut alloc.private - } - .new_init( - try_init!(QueueInfo::ver { - state: { - state.with_mut(|raw, _inner| { - raw.rb_size = size; - }); - state - }, - ring, - gpu_buf, - notifier_list: notifier_list, - gpu_context: gpu_context, - }), - |inner, _p| { - try_init!(raw::QueueInfo::ver { - state: inner.state.gpu_pointer(), - ring: inner.ring.gpu_pointer(), - notifier_list: inner.notifier_list.gpu_pointer(), - gpu_buf: inner.gpu_buf.gpu_pointer(), - gpu_rptr1: Default::default(), - gpu_rptr2: Default::default(), - gpu_rptr3: Default::default(), - event_id: AtomicI32::new(-1), - priority: prio, - unk_4c: -1, - uuid: id as u32, - unk_54: -1, - unk_58: Default::default(), - busy: Default::default(), - __pad: Default::default(), - #[ver(V >= V13_2 && G < G14X)] - unk_84_0: 0, - unk_84_state: Default::default(), - error_count: Default::default(), - unk_8c: 0, - unk_90: 0, - unk_94: 0, - pending: Default::default(), - unk_9c: 0, - gpu_context: inner.gpu_context.gpu_pointer(), - unk_a8: Default::default(), - #[ver(V >= V13_2 && G < G14X)] - unk_b0: 0, - }) - }, - )?, + info: Some(KBox::new(info, GFP_KERNEL)?), + gpu_context, new: true, pipe_type, size, @@ -751,7 +823,7 @@ impl WorkQueue::ver { last_submitted: None, }; - let info_pointer = inner.info.weak_pointer(); + let info_pointer = inner.info().weak_pointer(); Arc::pin_init( pin_init!(Self { @@ -835,7 +907,7 @@ impl WorkQueue::ver { pub(crate) fn dump_info(&self) { pr_info!("WorkQueue @ {:?}:", self.info_pointer); - self.inner.lock().info.with(|raw, _inner| { + self.inner.lock().info().with(|raw, _inner| { pr_info!(" GPU rptr1: {:#x}", raw.gpu_rptr1.load(Ordering::Relaxed)); pr_info!(" GPU rptr1: {:#x}", raw.gpu_rptr2.load(Ordering::Relaxed)); pr_info!(" GPU rptr1: {:#x}", raw.gpu_rptr3.load(Ordering::Relaxed)); @@ -953,7 +1025,7 @@ impl WorkQueue for WorkQueue::ver { ); inner - .info + .info() .state .with(|raw, _inner| raw.cpu_freeptr.store(last_wptr, Ordering::Release));