Re: [PATCH v1 RESEND 4/4] drm/tyr: add GPU reset handling

["Onur Özkan" <work@onurozkan.dev>]

On Sat, 11 Apr 2026 08:52:15 +0300
Onur Özkan <work@onurozkan.dev> wrote:

> On Fri, 10 Apr 2026 07:56:38 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
> 
> > On Thu, Apr 09, 2026 at 10:44:24AM -0300, Daniel Almeida wrote:
> > > Hi Onur,
> > > 
> > > > On 9 Apr 2026, at 08:41, Onur Özkan <work@onurozkan.dev> wrote:
> > > > 
> > > > On Fri, 03 Apr 2026 12:01:09 -0300
> > > > Daniel Almeida <daniel.almeida@collabora.com> wrote:
> > > > 
> > > >> 
> > > >> 
> > > >>> On 19 Mar 2026, at 08:08, Boris Brezillon <boris.brezillon@collabora.com> wrote:
> > > >>> 
> > > >>> On Fri, 13 Mar 2026 12:16:44 +0300
> > > >>> Onur Özkan <work@onurozkan.dev> wrote:
> > > >>> 
> > > >>>> +impl Controller {
> > > >>>> +    /// Creates a [`Controller`] instance.
> > > >>>> +    fn new(pdev: ARef<platform::Device>, iomem: Arc<Devres<IoMem>>) -> Result<Arc<Self>> {
> > > >>>> +        let wq = workqueue::OrderedQueue::new(c"tyr-reset-wq", 0)?;
> > > >>>> +
> > > >>>> +        Arc::pin_init(
> > > >>>> +            try_pin_init!(Self {
> > > >>>> +                pdev,
> > > >>>> +                iomem,
> > > >>>> +                pending: Atomic::new(false),
> > > >>>> +                wq,
> > > >>>> +                work <- kernel::new_work!("tyr::reset"),
> > > >>>> +            }),
> > > >>>> +            GFP_KERNEL,
> > > >>>> +        )
> > > >>>> +    }
> > > >>>> +
> > > >>>> +    /// Processes one scheduled reset request.
> > > >>>> +    ///
> > > >>>> +    /// Panthor reference:
> > > >>>> +    /// - drivers/gpu/drm/panthor/panthor_device.c::panthor_device_reset_work()
> > > >>>> +    fn reset_work(self: &Arc<Self>) {
> > > >>>> +        dev_info!(self.pdev.as_ref(), "GPU reset work is started.\n");
> > > >>>> +
> > > >>>> +        // SAFETY: `Controller` is part of driver-private data and only exists
> > > >>>> +        // while the platform device is bound.
> > > >>>> +        let pdev = unsafe { self.pdev.as_ref().as_bound() };
> > > >>>> +        if let Err(e) = run_reset(pdev, &self.iomem) {
> > > >>>> +            dev_err!(self.pdev.as_ref(), "GPU reset failed: {:?}\n", e);
> > > >>>> +        } else {
> > > >>>> +            dev_info!(self.pdev.as_ref(), "GPU reset work is done.\n");
> > > >>>> +        }
> > > >>> 
> > > >>> Unfortunately, the reset operation is not as simple as instructing the
> > > >>> GPU to reset, it's a complex synchronization process where you need to
> > > >>> try to gracefully put various components on hold before you reset, and
> > > >>> then resume those after the reset is effective. Of course, with what we
> > > >>> currently have in-tree, there's not much to suspend/resume, but I think
> > > >>> I'd prefer to design the thing so we can progressively add more
> > > >>> components without changing the reset logic too much.
> > > >>> 
> > > >>> I would probably start with a Resettable trait that has the
> > > >>> {pre,post}_reset() methods that exist in Panthor.
> > > >>> 
> > > >>> The other thing we need is a way for those components to know when a
> > > >>> reset is about to happen so they can postpone some actions they were
> > > >>> planning in order to not further delay the reset, or end up with
> > > >>> actions that fail because the HW is already unusable. Not too sure how
> > > >>> we want to handle that though. Panthor is currently sprinkled with
> > > >>> panthor_device_reset_is_pending() calls in key places, but that's still
> > > >>> very manual, maybe we can automate that a bit more in Tyr, dunno.
> > > >>> 
> > > >> 
> > > >> 
> > > >> We could have an enum where one of the variants is Resetting, and the other one
> > > >> gives access to whatever state is not accessible while resets are in progress.
> > > >> 
> > > >> Something like
> > > >> 
> > > >> pub enum TyrData {
> > > >>  Active(ActiveTyrData),
> > > >>  ResetInProgress(ActiveTyrData)
> > > >> }
> > > >> 
> > > >> fn access() -> Option<&ActiveTyrData> {
> > > >>  … // if the “ResetInProgress” variant is active, return None
> > > >> }
> > > >> 
> > > > 
> > > > That's an interesting approach, but if it's all about `fn access` function, we
> > > > can already do that with a simple atomic state e.g.,:
> > > > 
> > > > // The state flag in reset::Controller
> > > > state: Atomic<ResetState>
> > > > 
> > > > fn access(&self) -> Option<&Arc<Devres<IoMem>>> {
> > > > match self.state.load(Relaxed) {
> > > > ResetState::Idle => Some(&self.iomem),
> > > > _ => None,
> > > > }
> > > > }
> > > > 
> > > > What do you think? Would this be sufficient?
> > > > 
> > > > Btw, a sample code snippet from the caller side would be very helpful for
> > > > designing this further. That part is kind a blurry for me.
> > > > 
> > > > Thanks,
> > > > Onur
> > > > 
> > > >> 
> > > >>>> +
> > > >>>> +        self.pending.store(false, Release);
> > > >>>> +    }
> > > >>>> +}
> > > 
> > > I think that there are two things we're trying to implement correctly:
> > > 
> > > 1) Deny access to a subset of the state while a reset is in progress
> > > 2) Wait for anyone accessing 1) to finish before starting a reset
> > > 
> > > IIUC, using Atomic<T> can solve 1 by bailing if the "reset in progress"
> > > flag/variant is set, but I don't think it implements 2? One would have to
> > > implement more logic to block until the state is not being actively used.
> > > 
> > > Now, there are probably easier ways to solve this, but I propose that we do the
> > > extra legwork to make this explicit and enforceable by the type system.
> > > 
> > > How about introducing a r/w semaphore abstraction? It seems to correctly encode
> > > the logic we want:
> > > 
> > > a) multiple users can access the state if no reset is pending ("read" side)
> > > b) the reset code can block until the state is no longer being accessed (the "write" side)
> > > 
> > > In Tyr, this would roughly map to something like:
> > > 
> > > struct TyrData {
> > >   reset_gate: RwSem<ActiveHwState>
> > >   // other, always accessible members
> > > }
> > > 
> > > impl TyrData {
> > >   fn try_access(&self) -> Option<ReadGuard<'_, ActiveHwState>> {...}
> > > }
> > > 
> > > Where ActiveHwState contains the fw/mmu/sched blocks (these are not upstream
> > > yet, Deborah has a series that will introduce the fw block that should land
> > > soon) and perhaps more.
> > > 
> > > Now, the reset logic would be roughly:
> > > 
> > > fn reset_work(tdev: Arc<TyrDevice>) {
> > >   // Block until nobody else is accessing the hw, prevent others from
> > >   // initiating new accesses too..
> > >   let _guard = tdev.reset_gate.write();
> > >    
> > >   // pre_reset() all Resettable implementors
> > > 
> > >   ... reset
> > > 
> > >   // post_Reset all Resettable implementors
> > > }
> > > 
> > > Now, for every block that might touch a resource that would be unavailable
> > > during a reset, we enforce a try_access() via the type system, and ensure that
> > > the reset cannot start while the guard is alive. In particular, ioctls would
> > > look like:
> > > 
> > > fn ioctl_foo(...) {
> > >   let hw = tdev.reset_gate.try_access()?;
> > >   // resets are blocked while the guard is alive, no other way to access that state otherwise
> > > }
> > > 
> > > The code will not compile otherwise, so long as we keep the state in ActiveHwState, i.e.: 
> > > protected by the r/w sem.
> > > 
> > > This looks like an improvement over Panthor, since Panthor relies on manually
> > > canceling work that access hw state via cancel_work_sync(), and gating new work
> > > submissions on the "reset_in_progress" flag, i.e.:
> > > 
> > > /**
> > >  * sched_queue_work() - Queue a scheduler work.
> > >  * @sched: Scheduler object.
> > >  * @wname: Work name.
> > >  *
> > >  * Conditionally queues a scheduler work if no reset is pending/in-progress.
> > >  */
> > > #define sched_queue_work(sched, wname) \
> > > do { \
> > > if (!atomic_read(&(sched)->reset.in_progress) && \
> > >     !panthor_device_reset_is_pending((sched)->ptdev)) \
> > > queue_work((sched)->wq, &(sched)->wname ## _work); \
> > > } while (0)
> > > 
> > > 
> > > Thoughts?
> > 
> > I don't think a semaphore is the right answer. I think SRCU plus a
> > counter makes more sense. (With a sentinal value reserved for "currently
> > resetting".)
> > 
> > When you begin using the hardware, you start an srcu critical region and
> > read the counter. If the counter has the sentinel value, you know the
> > hardware is resetting and you fail. Otherwise you record the couter and
> > proceed.
> > 
> > If at any point you release the srcu critical region and want to
> > re-acquire it to continue the same ongoing work, then you must ensure
> > that the counter still has the same value. This ensures that if the GPU
> > is reset, then even if the reset has finished by the time you come back,
> > you still fail because the counter has changed.
> > 
> > 
> > Another option could be to rely on the existing Device unbind logic.
> > Entirely remove the class device and run the full unbind procedure,
> > cleaning up all devm work etc. Then once that has finished, probe the
> > device again and start from scratch. After all, GPU reset does not have
> > to be a cheap operation.
> > 
> > Alice
> 
> Anybody working on the SRCU support or do we need to write one?
> 
> Ignoring the sync primitive for a moment (since there is no clear consensus
> yet), I was trying to figure out the HW access API around a closure.
> 
> I wonder if something like this (roughly, haven't implemented or compiled it
> yet) would make sense:
> 
> 	struct HwGate {
> 		// srcu, I guess?
> 	}
> 
> 	struct HwGuard<'a> {
> 		gate: &'a HwGate,
> 		hw: &'a ActiveHw,
> 	}
> 
> 	struct ActiveHw {
> 		// mmu, fw, ...
> 	}
> 
> 	struct Controller {
> 		...
> 		hw: ActiveHw,
> 	}
> 
> 	impl Controller {
> 		fn with_hw<R>(&self, f: impl FnOnce(&HwGuard<'_>) -> Result<R>) -> Result<R> {
> 			let guard = self.gate.lock(&self.hw)?;
> 			f(&guard)
> 		}
> 	}
> 
> and TyrDrmDeviceData would hava a helper wrapper:
> 
> 	impl TyrDrmDeviceData {
> 		fn with_hw<R>(&self, f: impl FnOnce(&ActiveHwState) -> Result<R>) -> Result<R> {
> 			self.reset.with_hw(f) // maybe find a better name for "reset"
> 		}
> 	}
> 
> Then the user can use it like this:
> 
> 	let driver: TyrDrmDeviceData = ...;
> 
> 	driver.with_hw(|hw| {
> 		let mmu = hw.mmu;
> 		...
> 	})
> 
> Any thoughts?
> 
> - Onur

I forgot to mention that with the closure approach, we can easily inject
additional operations into hardware access like preventing the access if a
reset work has been queued but not started yet, which was one of the key
considerations bringed up by Boris during the Thursday call.

- Onur