Re: [PATCH 3/6] rust: drm: Add RegistrationData to drm::Driver
From: lyude
Date: Thu May 28 2026 - 20:17:29 EST
Actually - thinking about this more now that I've been back at getting
rvkms ready and trying to make KMS initialization work in rust again: I
noticed I completely missed the fact that we now have a
`drm::Driver::Data` associated type and -separately- have a
`drm::Driver::RegistrationData` trait with this patch series. Which
means `RegistrationData` is probably a perfectly fine name for this and
you can disregard my previous comment.
FWIW: now that I've noticed this, that might simplify things w/r/t what
KMS needs here from DeviceContext and allow me to go from modeling
DeviceContext around 3 stages (Uninit, Initialized, Registered), to
just 2 stages (Initialized, Registered). The purpose of having a
separate Uninit and Initialized had been to workaround not having
access to `drm::Driver::Data` when a driver performs an atomic commit
before userspace registration, which would be a big ergonomic issue for
implementing a lot of atomic KMS callbacks.
But if that's not an issue with this patch series, then it probably
makes more sense for me to respin that DeviceContext patch series to
reflect that since that simplifies things a lot for the KMS side. For
your patch series, it just means you have to replace "Uninit" with
"Initialized" in the next version.
I will send out a respin with this tomorrow after working on respinning
the gem shmem patch series, since that shouldn't take me very long at
all.
On Wed, 2026-05-27 at 15:21 -0400, lyude@xxxxxxxxxx wrote:
> So I just realized while working on rebasing rvkms - I'm not sure
> RegistrationData is the right name for this. If you recall, I
> described
> 3 different DeviceContext types in the patches I sent for adding
> DeviceContext and explicitly mentioned one of them isn't used yet:
>
> * Uninit
> * Initialized (the unused one)
> * Registered
>
> The thing is we probably want the RegistrationData available starting
> from Initialized, not from Registered. The reason being - setting up
> a
> DRM device with KMS support can often require performing a modeset
> _before_ the device is registered.
>
> Furthermore, the C callbacks that are used for such modesets are
> exactly the same callbacks used for modesets after registration -
> which
> implies that the DeviceContext we'll be working with in nearly all of
> the modeset callbacks is going to be &Device<T, Initialized> - not
> &Device<T, Registered>. And as you might imagine, it would be pretty
> painful for a KMS driver not to be able to use RegistrationData from
> any of its modesetting callbacks.
>
> We don't specify a type for Initialized yet, but in preparation for
> that we probably should give this a name such as DeviceData or
> DriverData - not RegistrationData.
>
> On Thu, 2026-05-07 at 00:06 +0200, Danilo Krummrich wrote:
> > Add a RegistrationData associated type to drm::Driver. This is a
> > ForLt
> > type whose lifetime is tied to the parent bus device binding scope.
> >
> > Registration<T> takes ownership of the data via Pin<KBox<_>>,
> > erasing
> > the lifetime to 'static for storage. The pointer is written to
> > drm::Device before drm_dev_register() to ensure it is already in
> > place
> > when ioctls arrive.
> >
> > UnbindGuard::registration_data() provides access with the lifetime
> > shortened from 'static via ForLt::cast_ref. Since
> > Registration::drop()
> > calls drm_dev_unplug() -- which performs an SRCU barrier waiting
> > for
> > all
> > drm_dev_enter() critical sections to complete -- the data is
> > guaranteed
> > to remain valid for the duration of any UnbindGuard.
> >
> > Signed-off-by: Danilo Krummrich <dakr@xxxxxxxxxx>
> > ---
> > drivers/gpu/drm/nova/driver.rs | 6 ++-
> > drivers/gpu/drm/tyr/driver.rs | 6 ++-
> > rust/kernel/drm/device.rs | 40 +++++++++++++++
> > rust/kernel/drm/driver.rs | 89 +++++++++++++++++++++++++++---
> > --
> > --
> > rust/kernel/drm/mod.rs | 1 +
> > 5 files changed, 121 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/nova/driver.rs
> > b/drivers/gpu/drm/nova/driver.rs
> > index 9d4100f01ea7..54a3391371ba 100644
> > --- a/drivers/gpu/drm/nova/driver.rs
> > +++ b/drivers/gpu/drm/nova/driver.rs
> > @@ -12,7 +12,8 @@
> > ioctl, //
> > },
> > prelude::*,
> > - sync::aref::ARef, //
> > + sync::aref::ARef,
> > + types::ForLt, //
> > };
> >
> > use crate::file::File;
> > @@ -63,7 +64,7 @@ fn probe(
> > let data = try_pin_init!(NovaData { adev: adev.into() });
> >
> > let drm = drm::UnregisteredDevice::<Self>::new(adev,
> > data)?;
> > - let drm = drm::Registration::new_foreign_owned(drm,
> > adev.as_ref(), 0)?;
> > + let drm = drm::Registration::new_foreign_owned(drm,
> > adev.as_ref(), (), 0)?;
> >
> > Ok(Self { drm: drm.into() })
> > }
> > @@ -72,6 +73,7 @@ fn probe(
> > #[vtable]
> > impl drm::Driver for NovaDriver {
> > type Data = NovaData;
> > + type RegistrationData = ForLt!(());
> > type File = File;
> > type Object<Ctx: drm::DeviceContext> = gem::Object<NovaObject,
> > Ctx>;
> > type ParentDevice<Ctx: DeviceContext> =
> > auxiliary::Device<Ctx>;
> > diff --git a/drivers/gpu/drm/tyr/driver.rs
> > b/drivers/gpu/drm/tyr/driver.rs
> > index 747745d23f31..7ac3707823b6 100644
> > --- a/drivers/gpu/drm/tyr/driver.rs
> > +++ b/drivers/gpu/drm/tyr/driver.rs
> > @@ -25,7 +25,8 @@
> > aref::ARef,
> > Mutex, //
> > },
> > - time, //
> > + time,
> > + types::ForLt, //
> > };
> >
> > use crate::{
> > @@ -133,7 +134,7 @@ fn probe(
> > });
> >
> > let tdev =
> > drm::UnregisteredDevice::<TyrDrmDriver>::new(pdev, data)?;
> > - let tdev =
> > drm::driver::Registration::new_foreign_owned(tdev, pdev.as_ref(),
> > 0)?;
> > + let tdev =
> > drm::driver::Registration::new_foreign_owned(tdev, pdev.as_ref(),
> > (),
> > 0)?;
> >
> > let driver = TyrPlatformDriverData {
> > _device: tdev.into(),
> > @@ -175,6 +176,7 @@ fn drop(self: Pin<&mut Self>) {
> > #[vtable]
> > impl drm::Driver for TyrDrmDriver {
> > type Data = TyrDrmDeviceData;
> > + type RegistrationData = ForLt!(());
> > type File = TyrDrmFileData;
> > type Object<R: drm::DeviceContext> =
> > drm::gem::Object<TyrObject,
> > R>;
> > type ParentDevice<Ctx: DeviceContext> = platform::Device<Ctx>;
> > diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
> > index bb685165032d..11edbe6f9f42 100644
> > --- a/rust/kernel/drm/device.rs
> > +++ b/rust/kernel/drm/device.rs
> > @@ -23,6 +23,7 @@
> > AlwaysRefCounted, //
> > },
> > types::{
> > + ForLt,
> > NotThreadSafe,
> > Opaque, //
> > },
> > @@ -35,6 +36,7 @@
> > };
> > use core::{
> > alloc::Layout,
> > + cell::UnsafeCell,
> > marker::PhantomData,
> > mem,
> > ops::Deref,
> > @@ -239,6 +241,9 @@ pub fn new(
> > unsafe { bindings::drm_dev_put(drm_dev) };
> > })?;
> >
> > + // SAFETY: `raw_drm` is valid; no concurrent access before
> > registration.
> > + unsafe { (*raw_drm.as_ptr()).registration_data =
> > UnsafeCell::new(NonNull::dangling()) };
> > +
> > // SAFETY: The reference count is one, and now we take
> > ownership of that reference as a
> > // `drm::Device`.
> > // INVARIANT: We just created the device above, but have
> > yet
> > to call `drm_dev_register`.
> > @@ -270,6 +275,7 @@ pub fn new(
> > pub struct Device<T: drm::Driver, C: DeviceContext = Registered> {
> > dev: Opaque<bindings::drm_device>,
> > data: T::Data,
> > + pub(super) registration_data:
> > UnsafeCell<NonNull<<T::RegistrationData as ForLt>::Of<'static>>>,
> > _ctx: PhantomData<C>,
> > }
> >
> > @@ -278,6 +284,23 @@ pub(crate) fn as_raw(&self) -> *mut
> > bindings::drm_device {
> > self.dev.get()
> > }
> >
> > + /// Returns a reference to the registration data with lifetime
> > shortened
> > + /// from `'static`.
> > + ///
> > + /// # Safety
> > + ///
> > + /// The caller must ensure the parent bus device is bound.
> > This
> > is
> > + /// typically guaranteed by holding an active
> > `drm_dev_enter()`
> > critical
> > + /// section (e.g. via [`UnbindGuard`]).
> > + #[doc(hidden)]
> > + pub unsafe fn raw_registration_data(&self) ->
> > &<T::RegistrationData as ForLt>::Of<'_> {
> > + // SAFETY: Caller guarantees the parent bus device is
> > bound,
> > hence
> > + // the pointer is valid.
> > + let static_ref = unsafe {
> > (*self.registration_data.get()).as_ref() };
> > +
> > + T::RegistrationData::cast_ref(static_ref)
> > + }
> > +
> > /// # Safety
> > ///
> > /// `ptr` must be a valid pointer to a `struct device`
> > embedded
> > in `Self`.
> > @@ -391,6 +414,23 @@ pub struct UnbindGuard<'a, T: drm::Driver> {
> > idx: i32,
> > }
> >
> > +impl<T: drm::Driver> UnbindGuard<'_, T> {
> > + /// Returns a reference to the registration data with its
> > lifetime shortened from `'static`
> > + /// to the guard's borrow lifetime.
> > + ///
> > + /// The data is owned by
> > [`Registration`](drm::driver::Registration) and is guaranteed to
> > + /// remain valid for the duration of this guard, since
> > + /// [`Registration`](drm::driver::Registration)'s `drop` calls
> > + /// `drm_dev_unplug()` which waits for all `drm_dev_enter()`
> > critical sections to complete.
> > + pub fn registration_data(&self) -> &<T::RegistrationData as
> > ForLt>::Of<'_> {
> > + // SAFETY: The pointer was set in `Registration::new()`
> > before `drm_dev_register()`, and
> > + // is only invalidated after `drm_dev_unplug()` in
> > `Registration::drop()`. Since we hold
> > + // an active `drm_dev_enter()` critical section, the SRCU
> > barrier in `drm_dev_unplug()`
> > + // guarantees the pointer is still valid.
> > + unsafe { self.dev.raw_registration_data() }
> > + }
> > +}
> > +
> > impl<T: drm::Driver> Deref for UnbindGuard<'_, T> {
> > type Target = T::ParentDevice<device::Bound>;
> >
> > diff --git a/rust/kernel/drm/driver.rs b/rust/kernel/drm/driver.rs
> > index 751a68bb27e1..3a49ef324ada 100644
> > --- a/rust/kernel/drm/driver.rs
> > +++ b/rust/kernel/drm/driver.rs
> > @@ -11,7 +11,8 @@
> > drm,
> > error::to_result,
> > prelude::*,
> > - sync::aref::ARef, //
> > + sync::aref::ARef,
> > + types::ForLt, //
> > };
> > use core::{
> > mem,
> > @@ -108,6 +109,16 @@ pub trait Driver {
> > /// Context data associated with the DRM driver
> > type Data: Sync + Send;
> >
> > + /// Data owned by the [`Registration`] and accessible through
> > [`drm::device::UnbindGuard`].
> > + ///
> > + /// This is a [`ForLt`](trait@ForLt) type whose lifetime is
> > tied
> > to the parent bus
> > + /// device binding scope.
> > + /// The data is only accessible while the parent bus device is
> > bound (i.e. within a
> > + /// `drm_dev_enter/exit` critical section), and references
> > handed out by
> > + ///
> > [`UnbindGuard::registration_data()`](drm::device::UnbindGuard::regi
> > st
> > ration_data) have
> > + /// their lifetime shortened accordingly via
> > [`ForLt::cast_ref`].
> > + type RegistrationData: ForLt;
> > +
> > /// The type used to manage memory for this driver.
> > type Object<Ctx: drm::DeviceContext>: AllocImpl;
> >
> > @@ -127,12 +138,44 @@ pub trait Driver {
> > /// The registration type of a `drm::Device`.
> > ///
> > /// Once the `Registration` structure is dropped, the device is
> > unregistered.
> > -pub struct Registration<T: Driver>(ARef<drm::Device<T>>);
> > +pub struct Registration<T: Driver> {
> > + drm: ARef<drm::Device<T>>,
> > + #[allow(dead_code)]
> > + reg_data: Pin<KBox<<T::RegistrationData as
> > ForLt>::Of<'static>>>,
> > +}
> >
> > -impl<T: Driver> Registration<T> {
> > - fn new(drm: drm::UnregisteredDevice<T>, flags: usize) ->
> > Result<Self> {
> > - // SAFETY: `drm.as_raw()` is valid by the invariants of
> > `drm::Device`.
> > - to_result(unsafe {
> > bindings::drm_dev_register(drm.as_raw(),
> > flags) })?;
> > +impl<T: Driver> Registration<T>
> > +where
> > + for<'a> <T::RegistrationData as ForLt>::Of<'a>: Send,
> > +{
> > + fn new<'bound, E>(
> > + drm: drm::UnregisteredDevice<T>,
> > + reg_data: impl PinInit<<T::RegistrationData as
> > ForLt>::Of<'bound>, E>,
> > + flags: usize,
> > + ) -> Result<Self>
> > + where
> > + Error: From<E>,
> > + {
> > + let reg_data: Pin<KBox<<T::RegistrationData as
> > ForLt>::Of<'bound>>> =
> > + KBox::pin_init(reg_data, GFP_KERNEL)?;
> > +
> > + // SAFETY: `ForLt` guarantees covariance; lifetimes do not
> > affect layout.
> > + let reg_data: Pin<KBox<<T::RegistrationData as
> > ForLt>::Of<'static>>> =
> > + unsafe { mem::transmute(reg_data) };
> > +
> > + // Store the registration data pointer in the device
> > before
> > registration, so that it is
> > + // visible once ioctls can be called.
> > + //
> > + // SAFETY: No concurrent access; the device is not yet
> > registered.
> > + unsafe { *drm.registration_data.get() =
> > NonNull::from(Pin::get_ref(reg_data.as_ref())) }
> > +
> > + // SAFETY: `drm` is a valid, initialized but not yet
> > registered DRM device.
> > + let ret = unsafe {
> > bindings::drm_dev_register(drm.as_raw(),
> > flags) };
> > + if let Err(e) = to_result(ret) {
> > + // SAFETY: No concurrent access; registration failed.
> > + unsafe { *drm.registration_data.get() =
> > NonNull::dangling() };
> > + return Err(e);
> > + }
> >
> > // SAFETY: We just called `drm_dev_register` above
> > let new = NonNull::from(unsafe { drm.assume_ctx() });
> > @@ -144,46 +187,55 @@ fn new(drm: drm::UnregisteredDevice<T>,
> > flags:
> > usize) -> Result<Self> {
> > // one reference to the device - which we take ownership
> > over here.
> > let new = unsafe { ARef::from_raw(new) };
> >
> > - Ok(Self(new))
> > + Ok(Self { drm: new, reg_data })
> > }
> >
> > /// Registers a new
> > [`UnregisteredDevice`](drm::UnregisteredDevice) with userspace.
> > ///
> > /// Ownership of the [`Registration`] object is passed to
> > [`devres::register`].
> > - pub fn new_foreign_owned<'a>(
> > + pub fn new_foreign_owned<'bound, E>(
> > drm: drm::UnregisteredDevice<T>,
> > - dev: &'a device::Device<device::Bound>,
> > + dev: &'bound device::Device<device::Bound>,
> > + reg_data: impl PinInit<<T::RegistrationData as
> > ForLt>::Of<'bound>, E>,
> > flags: usize,
> > - ) -> Result<&'a drm::Device<T>>
> > + ) -> Result<&'bound drm::Device<T>>
> > where
> > T: 'static,
> > + Error: From<E>,
> > {
> > if drm.as_ref().as_raw() != dev.as_raw() {
> > return Err(EINVAL);
> > }
> >
> > - let reg = Registration::<T>::new(drm, flags)?;
> > + let reg = Registration::<T>::new(drm, reg_data, flags)?;
> > let drm = NonNull::from(reg.device());
> >
> > - devres::register(dev, reg, GFP_KERNEL)?;
> > + devres::register::<_, core::convert::Infallible>(dev, reg,
> > GFP_KERNEL)?;
> >
> > // SAFETY: Since `reg` was passed to devres::register(),
> > the
> > device now owns the lifetime
> > - // of the DRM registration - ensuring that this references
> > lives for at least as long as 'a.
> > + // of the DRM registration - ensuring that this reference
> > lives for
> > + // at least as long as 'bound.
> > Ok(unsafe { drm.as_ref() })
> > }
> >
> > /// Returns a reference to the `Device` instance for this
> > registration.
> > pub fn device(&self) -> &drm::Device<T> {
> > - &self.0
> > + &self.drm
> > }
> > }
> >
> > // SAFETY: `Registration` doesn't offer any methods or access to
> > fields when shared between
> > // threads, hence it's safe to share it.
> > -unsafe impl<T: Driver> Sync for Registration<T> {}
> > +unsafe impl<T: Driver> Sync for Registration<T> where
> > + for<'a> <T::RegistrationData as ForLt>::Of<'a>: Send
> > +{
> > +}
> >
> > // SAFETY: Registration with and unregistration from the DRM
> > subsystem can happen from any thread.
> > -unsafe impl<T: Driver> Send for Registration<T> {}
> > +unsafe impl<T: Driver> Send for Registration<T> where
> > + for<'a> <T::RegistrationData as ForLt>::Of<'a>: Send
> > +{
> > +}
> >
> > impl<T: Driver> Drop for Registration<T> {
> > fn drop(&mut self) {
> > @@ -195,6 +247,9 @@ fn drop(&mut self) {
> > //
> > // SAFETY: Safe by the invariant of
> > `ARef<drm::Device<T>>`.
> > The existence of this
> > // `Registration` also guarantees that this `drm::Device`
> > is
> > actually registered.
> > - unsafe { bindings::drm_dev_unplug(self.0.as_raw()) };
> > + unsafe { bindings::drm_dev_unplug(self.drm.as_raw()) };
> > + // After drm_dev_unplug(), the SRCU barrier guarantees
> > that
> > all UnbindGuard critical
> > + // sections have completed, so no one holds a reference to
> > reg_data anymore.
> > + // reg_data is dropped here automatically.
> > }
> > }
> > diff --git a/rust/kernel/drm/mod.rs b/rust/kernel/drm/mod.rs
> > index 64a43cb0fe57..6c0ba9c82b92 100644
> > --- a/rust/kernel/drm/mod.rs
> > +++ b/rust/kernel/drm/mod.rs
> > @@ -11,6 +11,7 @@
> > pub use self::device::Device;
> > pub use self::device::DeviceContext;
> > pub use self::device::Registered;
> > +pub use self::device::UnbindGuard;
> > pub use self::device::Uninit;
> > pub use self::device::UnregisteredDevice;
> > pub use self::driver::Driver;