rust: drm: Higher-Ranked Lifetime private data
Subsystems (7)
Danilo Krummrich
maintainercover rust: drm: Higher-Ranked Lifetime private data
DRM ioctls run in process context without any guarantee that the parent bus device is still bound. This series solves the problem by introducing UnbindGuard -- a guard representing a drm_dev_enter/exit SRCU critical section that proves the parent bus device is bound for the lifetime of the guard.
On top of that, add RegistrationData as a ForLt associated type on drm::Driver, allowing drivers to store data whose lifetime is tied to the parent bus device binding scope. The data is allocated in Registration::new(), lifetime-erased to 'static for storage, and made accessible through UnbindGuard::registration_data() with the lifetime shortened back via ForLt::cast_ref.
Also update the ioctl dispatch macro to wrap every handler in an UnbindGuard, returning ENODEV if the device has been unplugged, and pass both the bound parent bus device and the registration data as arguments to handlers.
This series is based on [1], as well as Lyudes drm::DeviceContext work; a branch with all patches can be found in [2].
[1] https://lore.kernel.org/driver-core/[email protected]/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=drm-lifetime
Danilo Krummrich (6):
rust: drm: Add Driver::ParentDevice associated type
rust: drm: Add UnbindGuard for drm_dev_enter/exit critical sections
rust: drm: Add RegistrationData to drm::Driver
rust: drm: Wrap ioctl dispatch in UnbindGuard
rust: drm: Pass registration data to ioctl handlers
rust: drm: Pass bound parent device to ioctl handlers
drivers/gpu/drm/nova/driver.rs | 14 +++- drivers/gpu/drm/nova/file.rs | 8 ++ drivers/gpu/drm/tyr/driver.rs | 12 ++- drivers/gpu/drm/tyr/file.rs | 4 + rust/kernel/drm/device.rs | 134 ++++++++++++++++++++++++++++++++- rust/kernel/drm/driver.rs | 100 +++++++++++++++++++----- rust/kernel/drm/ioctl.rs | 17 ++++- rust/kernel/drm/mod.rs | 1 + 8 files changed, 258 insertions(+), 32 deletions(-)
Signature · 1 line
2.54.0
Danilo Krummrich
maintainerPATCH 1/6 rust: drm: Add Driver::ParentDevice associated type
Add a ParentDevice associated type to the Driver trait, allowing each DRM driver to declare its parent bus device type (e.g. auxiliary::Device, platform::Device).
Change UnregisteredDevice::new() to take &T::ParentDevice<Bound>, ensuring at the type level that the DRM device's parent matches the declared bus device type.
--- drivers/gpu/drm/nova/driver.rs | 8 ++++++-- drivers/gpu/drm/tyr/driver.rs | 6 ++++-- rust/kernel/drm/device.rs | 7 +++++-- rust/kernel/drm/driver.rs | 3 +++ 4 files changed, 18 insertions(+), 6 deletions(-)
drivers/gpu/drm/nova/driver.rs +6 -2
@@ -2,7 +2,10 @@use kernel::{ auxiliary, - device::Core , + device::{ +Core , +DeviceContext ,// + }, drm::{ self, gem,
@@ -59,7 +62,7 @@ fn probe( ) -> impl PinInit<Self, Error> + 'bound {let data =try_pin_init! (NovaData { adev: adev.into () }); -let drm = drm::UnregisteredDevice ::<Self >::new (adev.as_ref (), data)?; +let drm = drm::UnregisteredDevice ::<Self >::new (adev, data)?;let drm = drm::Registration ::new_foreign_owned (drm, adev.as_ref (),0 )?; Ok(Self { drm: drm.into() })
@@ -71,6 +74,7 @@ impl drm::Driver for NovaDriver {type Data =NovaData ;type File =File ;type Object <Ctx : drm::DeviceContext > = gem::Object <NovaObject ,Ctx >; +type ParentDevice <Ctx :DeviceContext > = auxiliary::Device <Ctx >;const INFO : drm::DriverInfo =INFO ;
drivers/gpu/drm/tyr/driver.rs +4 -2
@@ -8,7 +8,8 @@ device::{ Bound, Core, - Device, // + Device, + DeviceContext, // }, drm, drm::ioctl,
@@ -131,7 +132,7 @@ fn probe( gpu_info, }); -let tdev = drm::UnregisteredDevice ::<TyrDrmDriver >::new (pdev.as_ref (), data)?; +let tdev = drm::UnregisteredDevice ::<TyrDrmDriver >::new (pdev, data)?;let tdev = drm::driver::Registration ::new_foreign_owned (tdev, pdev.as_ref (),0 )?;let driver =TyrPlatformDriverData {
@@ -176,6 +177,7 @@ impl drm::Driver for TyrDrmDriver {type Data =TyrDrmDeviceData ;type File =TyrDrmFileData ;type Object <R : drm::DeviceContext > = drm::gem::Object <TyrObject ,R >; +type ParentDevice <Ctx :DeviceContext > = platform::Device <Ctx >;const INFO : drm::DriverInfo =INFO ;
rust/kernel/drm/device.rs +5 -2
@@ -197,7 +197,10 @@ impl<T: drm::Driver> UnregisteredDevice<T> {/// Create a new `UnregisteredDevice` for a `drm::Driver`. /// /// This can be used to create a [`Registration`](kernel::drm::Registration). -pub fn new (dev: &device::Device , data:impl PinInit <T ::Data ,Error >) ->Result <Self > { +pub fn new ( + dev: &T ::ParentDevice <device::Bound >, + data:impl PinInit <T ::Data ,Error >, + ) ->Result <Self > {// `__drm_dev_alloc` uses `kmalloc()` to allocate memory, hence ensure a `kmalloc()` // compatible `Layout`. let layout =Kmalloc ::aligned_layout (Layout ::new ::<Device <T ,Uninit >>());
@@ -207,7 +210,7 @@ pub fn new(dev: &device::Device, data: impl PinInit<T::Data, Error>) -> Result<S// - `dev` is valid by its type invarants, let raw_drm: *mut Device <T ,Uninit > =unsafe { bindings::__drm_dev_alloc ( - dev.as_raw (), + dev.as_ref ().as_raw (), &Self ::VTABLE , layout.size (), mem::offset_of!(Device<T, Uninit>, dev),
rust/kernel/drm/driver.rs +3 -0
@@ -114,6 +114,9 @@ pub trait Driver {/// The type used to represent a DRM File (client) type File : drm::file::DriverFile ; +/// The bus device type of the parent device that the DRM device is associated with. +type ParentDevice <Ctx : device::DeviceContext >: device::AsBusDevice <Ctx >; +/// Driver metadata const INFO :DriverInfo ;
Signature · 1 line
2.54.0
Re: [PATCH 1/6] rust: drm: Add Driver::ParentDevice associated type
Quoted 136 lines · Danilo Krummrich
Add a ParentDevice associated type to the Driver trait, allowing each DRM driver to declare its parent bus device type (e.g.
auxiliary::Device,platform::Device).Change
UnregisteredDevice::new()to take &T::ParentDevice<Bound>, ensuring at the type level that the DRM device's parent matches the declared bus device type.Signed-off-by Danilo Krummrich <[email protected]>---
drivers/gpu/drm/nova/driver.rs | 8 ++++++--
drivers/gpu/drm/tyr/driver.rs | 6 ++++--
rust/kernel/drm/device.rs | 7 +++++--
rust/kernel/drm/driver.rs | 3 +++4 files changed, 18 insertions(+), 6 deletions(-)drivers/gpu/drm/nova/driver.rs +0
-0@@ -2,7 +2,10 @@
use kernel::{
auxiliary,
-device::Core,
+ device::{
+ Core,
+ DeviceContext, //
+ },
drm::{
self,
gem,drivers/gpu/drm/nova/driver.rs +0
-0@@ -59,7 +62,7 @@ fn probe( ) -> impl PinInit<Self, Error> + 'bound { let data = try_pin_init!(NovaData { adev: adev.into() });
- let drm = drm::UnregisteredDevice::<Self>::new(adev.as_ref(), data)?; + let drm = drm::UnregisteredDevice::<Self>::new(adev, data)?; let drm =drm::Registration::new_foreign_owned(drm, adev.as_ref(), 0)?;
Ok(Self { drm:drm.into()})drivers/gpu/drm/nova/driver.rs +1
-0@@ -71,6 +74,7 @@ impl drm::Driver for NovaDriver { type Data = NovaData; type File = File; type Object<Ctx: drm::DeviceContext> = gem::Object<NovaObject,Ctx >; + type ParentDevice<Ctx: DeviceContext> = auxiliary::Device<Ctx>;
const INFO:drm::DriverInfo= INFO;drivers/gpu/drm/tyr/driver.rs +2
-1@@ -8,7 +8,8 @@ device::{ Bound, Core, - Device, // + Device, + DeviceContext, // }, drm, drm::ioctl,@@ -131,7 +132,7 @@ fn probe( gpu_info, });
- let tdev = drm::UnregisteredDevice::<TyrDrmDriver>::new(pdev.as_ref(), data)?; + let tdev = drm::UnregisteredDevice::<TyrDrmDriver>::new(pdev, data)?; let tdev =drm::driver::Registration::new_foreign_owned(tdev, pdev.as_ref(), 0)?;
let driver = TyrPlatformDriverData {drivers/gpu/drm/tyr/driver.rs +1
-0@@ -176,6 +177,7 @@ impl drm::Driver for TyrDrmDriver { type Data = TyrDrmDeviceData; type File = TyrDrmFileData; type Object<R: drm::DeviceContext> = drm::gem::Object<TyrObject,R >; + type ParentDevice<Ctx: DeviceContext> = platform::Device<Ctx>;
const INFO:drm::DriverInfo= INFO;rust/kernel/drm/device.rs +0
-0@@ -197,7 +197,10 @@ impl<T: drm::Driver> UnregisteredDevice<T> { /// Create a new `UnregisteredDevice` for a `drm::Driver`. /// /// This can be used to create a[
Registration](kernel::drm::Registration).
- pub fnnew(dev: &device::Device, data: impl PinInit<T::Data, Error>)-> Result<Self> {
+ pub fn new(
+ dev: &T::ParentDevice<device::Bound>,
+ data: impl PinInit<T::Data, Error>,
+ ) -> Result<Self> {
//__drm_dev_allocuseskmalloc()to allocate memory, hence ensure akmalloc()
// compatibleLayout.
let layout =Kmalloc::aligned_layout(Layout::new::<Device<T, Uninit>>());rust/kernel/drm/device.rs +0
-0@@ -207,7 +210,7 @@ pub fn new(dev: &device::Device, data: implPinInit<T::Data, Error>) -> Result<S
// -devis valid by its type invarants, letraw_drm: *mut Device<T, Uninit> = unsafe { bindings::__drm_dev_alloc(
-dev.as_raw(),
+ dev.as_ref().as_raw(),&Self::VTABLE,
layout.size(), mem::offset_of!(Device<T, Uninit>, dev),rust/kernel/drm/driver.rs +0
-0@@ -114,6 +114,9 @@ pub trait Driver { /// The type used to represent a DRM File (client) type File: drm::file::DriverFile;
+ /// The bus device type of the parent device that the DRM device is associated with.
+ type ParentDevice<Ctx:device::DeviceContext>:
device::AsBusDevice<Ctx>;
+
/// Driver metadata
const INFO: DriverInfo;
Danilo Krummrich
maintainerPATCH 2/6 rust: drm: Add UnbindGuard for drm_dev_enter/exit critical sections
In reply to Danilo KrummrichDRM ioctls do not guarantee that the parent bus device is still bound. However, since DRM device registration is managed through Devres, using drm_dev_unplug() on unregistration ensures that between drm_dev_enter() and drm_dev_exit() the parent device must be bound.
Add UnbindGuard, a guard object representing a drm_dev_enter/exit SRCU critical section that dereferences to &Device<Bound> of the parent bus device. The guard is only available on Device<T, Registered>, ensuring it cannot be used on unregistered devices.
Also add with_unbind_guard() as a convenience helper that executes a closure with the bound device reference.
Switch Registration::drop from drm_dev_unregister() to drm_dev_unplug() to provide the SRCU barrier that UnbindGuard's safety argument relies on.
--- rust/kernel/drm/device.rs | 80 ++++++++++++++++++++++++++++++++++++++- rust/kernel/drm/driver.rs | 10 ++++- 2 files changed, 87 insertions(+), 3 deletions(-)
rust/kernel/drm/device.rs +79 -1
@@ -6,7 +6,11 @@use crate ::{ alloc::allocator::Kmalloc , - bindings, device, + bindings, + device::{ + self, +AsBusDevice as _,// + }, drm::{ self, driver::AllocImpl ,
@@ -334,6 +338,80 @@ pub(crate) unsafe fn assume_ctx<NewCtx: DeviceContext>(&self) -> &Device<T, NewC } } +impl <T : drm::Driver >Device <T ,Registered > { +/// Guard against the parent bus device being unbound. +/// +/// Returns an [`UnbindGuard`] if the device has not been unplugged, [`None`] otherwise. +/// +/// The returned guard dereferences to the parent bus device in the [`device::Bound`] context +/// (see [`Driver::ParentDevice`](drm::Driver::ParentDevice)). +/// Between `drm_dev_enter()` and `drm_dev_exit()` the parent device is guaranteed to be bound. +#[must_use] +pub fn unbind_guard (&self) ->Option <UnbindGuard <'_,T >> { +let mut idx:i32 =0 ; +// SAFETY: `self.as_raw()` is a valid pointer to a `struct drm_device` by the type +// invariants of `Device<T, Registered>`. +if unsafe { bindings::drm_dev_enter (self.as_raw (), &mut idx) } { +Some (UnbindGuard { dev: self, idx }) + }else { +None + } + } + +/// Execute a closure while the parent bus device is guaranteed to be bound. +/// +/// Acquires the [`UnbindGuard`] and, if the device has not been unplugged, calls `f` with the +/// parent bus device. Returns `None` if the device has been unplugged. +pub fn with_unbind_guard <R >( + &self, + f:impl FnOnce (&T ::ParentDevice <device::Bound >) ->R , + ) ->Option <R > { +let guard = self.unbind_guard ()?; +Some (f (&guard)) + } +} + +/// A guard preventing the parent bus device from being unbound. +/// +/// The guard dereferences to [`Driver::ParentDevice<Bound>`](drm::Driver::ParentDevice), providing +/// access to the parent bus device with the guarantee that it is bound for the entire duration of +/// the critical section. +/// +/// Internally this is backed by a `drm_dev_enter()` / `drm_dev_exit()` SRCU critical section. +/// +/// See [`Device::unbind_guard`] for details on the safety argument. +/// +/// # Invariants +/// +/// - `idx` is the SRCU read lock index returned by a successful `drm_dev_enter()` call. +/// - The parent bus device of `dev` is bound for the lifetime of this guard. +#[must_use] +pub struct UnbindGuard <'a,T : drm::Driver > { + dev: &'aDevice <T ,Registered >, + idx:i32 , +} + +impl <T : drm::Driver >Deref for UnbindGuard <'_,T > { +type Target =T ::ParentDevice <device::Bound >; + +fn deref (&self) -> &Self ::Target { +// SAFETY: +// - The parent `struct device` is embedded in a `T::ParentDevice`, as guaranteed by +// `UnregisteredDevice::new` taking a `&T::ParentDevice<device::Bound>`. +// - By the type invariants of `UnbindGuard`, the parent device is bound for the lifetime +// of this guard. +unsafe {T ::ParentDevice ::from_device (self.dev.as_ref ().as_bound ()) } + } +} + +impl <T : drm::Driver >Drop for UnbindGuard <'_,T > { +fn drop (&mut self) { +// SAFETY: `self.idx` was returned by a successful `drm_dev_enter()` call, as guaranteed +// by the type invariants of `UnbindGuard`. +unsafe { bindings::drm_dev_exit (self.idx) }; + } +} +impl <T : drm::Driver ,C :DeviceContext >Deref for Device <T ,C > {type Target =T ::Data ;
rust/kernel/drm/driver.rs +8 -2
@@ -187,8 +187,14 @@ unsafe impl<T: Driver> Send for Registration<T> {}impl <T :Driver >Drop for Registration <T > {fn drop (&mut self) { +// Use `drm_dev_unplug` rather than `drm_dev_unregister` to ensure that existing +// `drm_dev_enter()` critical sections complete before unregistration proceeds. This +// is required for the safety of `UnbindGuard`, which relies on the SRCU barrier in +// `drm_dev_unplug()` to guarantee that the parent device is still bound within the +// critical section. +// // SAFETY: Safe by the invariant of `ARef<drm::Device<T>>`. The existence of this -// `Registration` also guarantees the this `drm::Device` is actually registered. -unsafe { bindings::drm_dev_unregister (self.0 .as_raw ()) }; +// `Registration` also guarantees that this `drm::Device` is actually registered. +unsafe { bindings::drm_dev_unplug (self.0 .as_raw ()) }; } }
Signature · 1 line
2.54.0
Re: [PATCH 2/6] rust: drm: Add UnbindGuard for drm_dev_enter/exit critical sections
On 2026-05-07 at 08:06 +1000, Danilo Krummrich <[email protected]> wrote...
Quoted 10 lines
DRM ioctls do not guarantee that the parent bus device is still bound. However, since DRM device registration is managed through Devres, using
drm_dev_unplug()on unregistration ensures that betweendrm_dev_enter()anddrm_dev_exit()the parent device must be bound.Add UnbindGuard, a guard object representing a drm_dev_enter/exit SRCU critical section that dereferences to &Device<Bound> of the parent bus device. The guard is only available on Device<T, Registered>, ensuring it cannot be used on unregistered devices.
Quoted 5 lines
Also add
with_unbind_guard()as a convenience helper that executes a closure with the bound device reference.Switch
Registration::dropfromdrm_dev_unregister()todrm_dev_unplug()to provide the SRCU barrier that UnbindGuard's safety argument relies on.
I don't (yet) claim to be a Rust-for-Linux expert but reading through this made sense to me and it's been useful for some Nova development so feel free to add:
for whatever that is worth :-)
Quoted 128 lines
Signed-off-by Danilo Krummrich <[email protected]>--- rust/kernel/drm/device.rs | 80 ++++++++++++++++++++++++++++++++++++++- rust/kernel/drm/driver.rs | 10 ++++- 2 files changed, 87 insertions(+), 3 deletions(-)rust/kernel/drm/device.rs +79
-1@@ -6,7 +6,11 @@use crate ::{ alloc::allocator::Kmalloc , - bindings, device, + bindings, + device::{ + self, +AsBusDevice as _,// + }, drm::{ self, driver::AllocImpl ,@@ -334,6 +338,80 @@ pub(crate) unsafe fn assume_ctx<NewCtx: DeviceContext>(&self) -> &Device<T, NewC } } +impl <T : drm::Driver >Device <T ,Registered > { +/// Guard against the parent bus device being unbound. +/// +/// Returns an [`UnbindGuard`] if the device has not been unplugged, [`None`] otherwise. +/// +/// The returned guard dereferences to the parent bus device in the [`device::Bound`] context +/// (see [`Driver::ParentDevice`](drm::Driver::ParentDevice)). +/// Between `drm_dev_enter()` and `drm_dev_exit()` the parent device is guaranteed to be bound. +#[must_use] +pub fn unbind_guard (&self) ->Option <UnbindGuard <'_,T >> { +let mut idx:i32 =0 ; +// SAFETY: `self.as_raw()` is a valid pointer to a `struct drm_device` by the type +// invariants of `Device<T, Registered>`. +if unsafe { bindings::drm_dev_enter (self.as_raw (), &mut idx) } { +Some (UnbindGuard { dev: self, idx }) + }else { +None + } + } + +/// Execute a closure while the parent bus device is guaranteed to be bound. +/// +/// Acquires the [`UnbindGuard`] and, if the device has not been unplugged, calls `f` with the +/// parent bus device. Returns `None` if the device has been unplugged. +pub fn with_unbind_guard <R >( + &self, + f:impl FnOnce (&T ::ParentDevice <device::Bound >) ->R , + ) ->Option <R > { +let guard = self.unbind_guard ()?; +Some (f (&guard)) + } +} + +/// A guard preventing the parent bus device from being unbound. +/// +/// The guard dereferences to [`Driver::ParentDevice<Bound>`](drm::Driver::ParentDevice), providing +/// access to the parent bus device with the guarantee that it is bound for the entire duration of +/// the critical section. +/// +/// Internally this is backed by a `drm_dev_enter()` / `drm_dev_exit()` SRCU critical section. +/// +/// See [`Device::unbind_guard`] for details on the safety argument. +/// +/// # Invariants +/// +/// - `idx` is the SRCU read lock index returned by a successful `drm_dev_enter()` call. +/// - The parent bus device of `dev` is bound for the lifetime of this guard. +#[must_use] +pub struct UnbindGuard <'a,T : drm::Driver > { + dev: &'aDevice <T ,Registered >, + idx:i32 , +} + +impl <T : drm::Driver >Deref for UnbindGuard <'_,T > { +type Target =T ::ParentDevice <device::Bound >; + +fn deref (&self) -> &Self ::Target { +// SAFETY: +// - The parent `struct device` is embedded in a `T::ParentDevice`, as guaranteed by +// `UnregisteredDevice::new` taking a `&T::ParentDevice<device::Bound>`. +// - By the type invariants of `UnbindGuard`, the parent device is bound for the lifetime +// of this guard. +unsafe {T ::ParentDevice ::from_device (self.dev.as_ref ().as_bound ()) } + } +} + +impl <T : drm::Driver >Drop for UnbindGuard <'_,T > { +fn drop (&mut self) { +// SAFETY: `self.idx` was returned by a successful `drm_dev_enter()` call, as guaranteed +// by the type invariants of `UnbindGuard`. +unsafe { bindings::drm_dev_exit (self.idx) }; + } +} +impl <T : drm::Driver ,C :DeviceContext >Deref for Device <T ,C > {type Target =T ::Data ;rust/kernel/drm/driver.rs +8
-2@@ -187,8 +187,14 @@ unsafe impl<T: Driver> Send for Registration<T> {}impl <T :Driver >Drop for Registration <T > {fn drop (&mut self) { +// Use `drm_dev_unplug` rather than `drm_dev_unregister` to ensure that existing +// `drm_dev_enter()` critical sections complete before unregistration proceeds. This +// is required for the safety of `UnbindGuard`, which relies on the SRCU barrier in +// `drm_dev_unplug()` to guarantee that the parent device is still bound within the +// critical section. +// // SAFETY: Safe by the invariant of `ARef<drm::Device<T>>`. The existence of this -// `Registration` also guarantees the this `drm::Device` is actually registered. -unsafe { bindings::drm_dev_unregister (self.0 .as_raw ()) }; +// `Registration` also guarantees that this `drm::Device` is actually registered. +unsafe { bindings::drm_dev_unplug (self.0 .as_raw ()) }; } }Signature · 1 line
2.54.0
Re: [PATCH 2/6] rust: drm: Add UnbindGuard for drm_dev_enter/exit critical sections
On 2026-05-26 at 16:17 +1000, Alistair Popple <[email protected]> wrote...
Quoted 17 lines
On 2026-05-07 at 08:06 +1000, Danilo Krummrich <[email protected]> wrote...
Quoted 10 lines
DRM ioctls do not guarantee that the parent bus device is still bound. However, since DRM device registration is managed through Devres, using
drm_dev_unplug()on unregistration ensures that betweendrm_dev_enter()anddrm_dev_exit()the parent device must be bound.Add UnbindGuard, a guard object representing a drm_dev_enter/exit SRCU critical section that dereferences to &Device<Bound> of the parent bus device. The guard is only available on Device<T, Registered>, ensuring it cannot be used on unregistered devices.
Quoted 5 lines
Also add
with_unbind_guard()as a convenience helper that executes a closure with the bound device reference.Switch
Registration::dropfromdrm_dev_unregister()todrm_dev_unplug()to provide the SRCU barrier that UnbindGuard's safety argument relies on.
Quoted 6 lines
I don't (yet) claim to be a Rust-for-Linux expert but reading through this made sense to me and it's been useful for some Nova development so feel free to add:
Reviewed-by Alistair Popple <[email protected]>for whatever that is worth :-)
Which is definitely not much :) I just realised this has been replaced by a more recent version of this series with 24 patches, so will use that instead.
- Alistair
Quoted 128 lines
Quoted 128 lines
Signed-off-by Danilo Krummrich <[email protected]>--- rust/kernel/drm/device.rs | 80 ++++++++++++++++++++++++++++++++++++++- rust/kernel/drm/driver.rs | 10 ++++- 2 files changed, 87 insertions(+), 3 deletions(-)rust/kernel/drm/device.rs +79
-1@@ -6,7 +6,11 @@use crate ::{ alloc::allocator::Kmalloc , - bindings, device, + bindings, + device::{ + self, +AsBusDevice as _,// + }, drm::{ self, driver::AllocImpl ,@@ -334,6 +338,80 @@ pub(crate) unsafe fn assume_ctx<NewCtx: DeviceContext>(&self) -> &Device<T, NewC } } +impl <T : drm::Driver >Device <T ,Registered > { +/// Guard against the parent bus device being unbound. +/// +/// Returns an [`UnbindGuard`] if the device has not been unplugged, [`None`] otherwise. +/// +/// The returned guard dereferences to the parent bus device in the [`device::Bound`] context +/// (see [`Driver::ParentDevice`](drm::Driver::ParentDevice)). +/// Between `drm_dev_enter()` and `drm_dev_exit()` the parent device is guaranteed to be bound. +#[must_use] +pub fn unbind_guard (&self) ->Option <UnbindGuard <'_,T >> { +let mut idx:i32 =0 ; +// SAFETY: `self.as_raw()` is a valid pointer to a `struct drm_device` by the type +// invariants of `Device<T, Registered>`. +if unsafe { bindings::drm_dev_enter (self.as_raw (), &mut idx) } { +Some (UnbindGuard { dev: self, idx }) + }else { +None + } + } + +/// Execute a closure while the parent bus device is guaranteed to be bound. +/// +/// Acquires the [`UnbindGuard`] and, if the device has not been unplugged, calls `f` with the +/// parent bus device. Returns `None` if the device has been unplugged. +pub fn with_unbind_guard <R >( + &self, + f:impl FnOnce (&T ::ParentDevice <device::Bound >) ->R , + ) ->Option <R > { +let guard = self.unbind_guard ()?; +Some (f (&guard)) + } +} + +/// A guard preventing the parent bus device from being unbound. +/// +/// The guard dereferences to [`Driver::ParentDevice<Bound>`](drm::Driver::ParentDevice), providing +/// access to the parent bus device with the guarantee that it is bound for the entire duration of +/// the critical section. +/// +/// Internally this is backed by a `drm_dev_enter()` / `drm_dev_exit()` SRCU critical section. +/// +/// See [`Device::unbind_guard`] for details on the safety argument. +/// +/// # Invariants +/// +/// - `idx` is the SRCU read lock index returned by a successful `drm_dev_enter()` call. +/// - The parent bus device of `dev` is bound for the lifetime of this guard. +#[must_use] +pub struct UnbindGuard <'a,T : drm::Driver > { + dev: &'aDevice <T ,Registered >, + idx:i32 , +} + +impl <T : drm::Driver >Deref for UnbindGuard <'_,T > { +type Target =T ::ParentDevice <device::Bound >; + +fn deref (&self) -> &Self ::Target { +// SAFETY: +// - The parent `struct device` is embedded in a `T::ParentDevice`, as guaranteed by +// `UnregisteredDevice::new` taking a `&T::ParentDevice<device::Bound>`. +// - By the type invariants of `UnbindGuard`, the parent device is bound for the lifetime +// of this guard. +unsafe {T ::ParentDevice ::from_device (self.dev.as_ref ().as_bound ()) } + } +} + +impl <T : drm::Driver >Drop for UnbindGuard <'_,T > { +fn drop (&mut self) { +// SAFETY: `self.idx` was returned by a successful `drm_dev_enter()` call, as guaranteed +// by the type invariants of `UnbindGuard`. +unsafe { bindings::drm_dev_exit (self.idx) }; + } +} +impl <T : drm::Driver ,C :DeviceContext >Deref for Device <T ,C > {type Target =T ::Data ;rust/kernel/drm/driver.rs +8
-2@@ -187,8 +187,14 @@ unsafe impl<T: Driver> Send for Registration<T> {}impl <T :Driver >Drop for Registration <T > {fn drop (&mut self) { +// Use `drm_dev_unplug` rather than `drm_dev_unregister` to ensure that existing +// `drm_dev_enter()` critical sections complete before unregistration proceeds. This +// is required for the safety of `UnbindGuard`, which relies on the SRCU barrier in +// `drm_dev_unplug()` to guarantee that the parent device is still bound within the +// critical section. +// // SAFETY: Safe by the invariant of `ARef<drm::Device<T>>`. The existence of this -// `Registration` also guarantees the this `drm::Device` is actually registered. -unsafe { bindings::drm_dev_unregister (self.0 .as_raw ()) }; +// `Registration` also guarantees that this `drm::Device` is actually registered. +unsafe { bindings::drm_dev_unplug (self.0 .as_raw ()) }; } }Signature · 1 line
2.54.0
Gary Guo
reviewerRe: [PATCH 2/6] rust: drm: Add UnbindGuard for drm_dev_enter/exit critical sections
Quoted 21 lines · Alistair Popple
On 2026-05-26 at 16:17 +1000, Alistair Popple <[email protected]> wrote...
Quoted 20 lines
On 2026-05-07 at 08:06 +1000, Danilo Krummrich <[email protected]> wrote...
Quoted 10 lines
DRM ioctls do not guarantee that the parent bus device is still bound. However, since DRM device registration is managed through Devres, using
drm_dev_unplug()on unregistration ensures that betweendrm_dev_enter()anddrm_dev_exit()the parent device must be bound.Add UnbindGuard, a guard object representing a drm_dev_enter/exit SRCU critical section that dereferences to &Device<Bound> of the parent bus device. The guard is only available on Device<T, Registered>, ensuring it cannot be used on unregistered devices.
Quoted 5 lines
Also add
with_unbind_guard()as a convenience helper that executes a closure with the bound device reference.Switch
Registration::dropfromdrm_dev_unregister()todrm_dev_unplug()to provide the SRCU barrier that UnbindGuard's safety argument relies on.I don't (yet) claim to be a Rust-for-Linux expert but reading through this made sense to me and it's been useful for some Nova development so feel free to add:
Quoted 6 lines
Quoted 3 lines
Reviewed-by Alistair Popple <[email protected]>for whatever that is worth :-)
Which is definitely not much :) I just realised this has been replaced by a more recent version of this series with 24 patches, so will use that instead.
Are you talking about
https://lore.kernel.org/rust-for-linux/[email protected]?
That's for driver-core, this is for DRM.
Best,
Gary
Gary Guo
reviewerRe: [PATCH 2/6] rust: drm: Add UnbindGuard for drm_dev_enter/exit critical sections
In reply to Danilo KrummrichQuoted 41 lines · Danilo Krummrich
DRM ioctls do not guarantee that the parent bus device is still bound. However, since DRM device registration is managed through Devres, using
drm_dev_unplug()on unregistration ensures that betweendrm_dev_enter()anddrm_dev_exit()the parent device must be bound.Add UnbindGuard, a guard object representing a drm_dev_enter/exit SRCU critical section that dereferences to &Device<Bound> of the parent bus device. The guard is only available on Device<T, Registered>, ensuring it cannot be used on unregistered devices.
Also add
with_unbind_guard()as a convenience helper that executes a closure with the bound device reference.Switch
Registration::dropfromdrm_dev_unregister()todrm_dev_unplug()to provide the SRCU barrier that UnbindGuard's safety argument relies on.Signed-off-by Danilo Krummrich <[email protected]>--- rust/kernel/drm/device.rs | 80 ++++++++++++++++++++++++++++++++++++++- rust/kernel/drm/driver.rs | 10 ++++- 2 files changed, 87 insertions(+), 3 deletions(-)rust/kernel/drm/device.rs +5
-1@@ -6,7 +6,11 @@use crate ::{ alloc::allocator::Kmalloc , - bindings, device, + bindings, + device::{ + self, +AsBusDevice as _,// + }, drm::{ self, driver::AllocImpl ,@@ -334,6 +338,80 @@ pub(crate) unsafe fn assume_ctx<NewCtx: DeviceContext>(&self) -> &Device<T, NewC }
rust/kernel/drm/device.rs pub(crate) unsafe fn assume_ctx<NewCtx: DeviceContext>(&self) -> &Device<T, NewC · line 348
Quoted 10 lines
rust/kernel/drm/device.rs +8
-0} +impl <T : drm::Driver >Device <T ,Registered > { +/// Guard against the parent bus device being unbound. +/// +/// Returns an [`UnbindGuard`] if the device has not been unplugged, [`None`] otherwise. +/// +/// The returned guard dereferences to the parent bus device in the [`device::Bound`] context +/// (see [`Driver::ParentDevice`](drm::Driver::ParentDevice)). +/// Between `drm_dev_enter()` and `drm_dev_exit()` the parent device is guaranteed to be bound.
This should propbably say "While UnbindGuard" is held, the parent device is guaranteed to be bound.
rust/kernel/drm/device.rs pub(crate) unsafe fn assume_ctx<NewCtx: DeviceContext>(&self) -> &Device<T, NewC · line 349
Quoted 1 line
rust/kernel/drm/device.rs +1
-0+#[must_use]
To aid search you can add #[doc(alias = "drm_dev_enter")] here.
rust/kernel/drm/device.rs pub(crate) unsafe fn assume_ctx<NewCtx: DeviceContext>(&self) -> &Device<T, NewC · line 350
Quoted 1 line
rust/kernel/drm/device.rs +1
-0+pub fn unbind_guard (&self) ->Option <UnbindGuard <'_,T >> {
Would it make sense to have a dedicated error type that converts to some fixed error code so people can use ??
Quoted 36 lines
rust/kernel/drm/device.rs +36
-0+let mut idx:i32 =0 ; +// SAFETY: `self.as_raw()` is a valid pointer to a `struct drm_device` by the type +// invariants of `Device<T, Registered>`. +if unsafe { bindings::drm_dev_enter (self.as_raw (), &mut idx) } { +Some (UnbindGuard { dev: self, idx }) + }else { +None + } + } + +/// Execute a closure while the parent bus device is guaranteed to be bound. +/// +/// Acquires the [`UnbindGuard`] and, if the device has not been unplugged, calls `f` with the +/// parent bus device. Returns `None` if the device has been unplugged. +pub fn with_unbind_guard <R >( + &self, + f:impl FnOnce (&T ::ParentDevice <device::Bound >) ->R , + ) ->Option <R > { +let guard = self.unbind_guard ()?; +Some (f (&guard)) + } +} + +/// A guard preventing the parent bus device from being unbound. +/// +/// The guard dereferences to [`Driver::ParentDevice<Bound>`](drm::Driver::ParentDevice), providing +/// access to the parent bus device with the guarantee that it is bound for the entire duration of +/// the critical section. +/// +/// Internally this is backed by a `drm_dev_enter()` / `drm_dev_exit()` SRCU critical section. +/// +/// See [`Device::unbind_guard`] for details on the safety argument. +/// +/// # Invariants +/// +/// - `idx` is the SRCU read lock index returned by a successful `drm_dev_enter()` call.
rust/kernel/drm/device.rs pub(crate) unsafe fn assume_ctx<NewCtx: DeviceContext>(&self) -> &Device<T, NewC · line 396
Quoted 10 lines
rust/kernel/drm/device.rs +10
-0+/// - The parent bus device of `dev` is bound for the lifetime of this guard. +#[must_use] +pub struct UnbindGuard <'a,T : drm::Driver > { + dev: &'aDevice <T ,Registered >, + idx:i32 , +} + +impl <T : drm::Driver >Deref for UnbindGuard <'_,T > { +type Target =T ::ParentDevice <device::Bound >; +
#[inline]
rust/kernel/drm/device.rs pub(crate) unsafe fn assume_ctx<NewCtx: DeviceContext>(&self) -> &Device<T, NewC · line 403
Quoted 7 lines
rust/kernel/drm/device.rs +7
-0+fn deref (&self) -> &Self ::Target { +// SAFETY: +// - The parent `struct device` is embedded in a `T::ParentDevice`, as guaranteed by +// `UnregisteredDevice::new` taking a `&T::ParentDevice<device::Bound>`. +// - By the type invariants of `UnbindGuard`, the parent device is bound for the lifetime +// of this guard. +unsafe {T ::ParentDevice ::from_device (self.dev.as_ref ().as_bound ()) }
I suppose you could also just store this whole thing in unbind guard.
rust/kernel/drm/device.rs pub(crate) unsafe fn assume_ctx<NewCtx: DeviceContext>(&self) -> &Device<T, NewC · line 407
Quoted 4 lines
rust/kernel/drm/device.rs +4
-0+ } +} + +impl <T : drm::Driver >Drop for UnbindGuard <'_,T > {
#[inline]
Quoted 31 lines
rust/kernel/drm/device.rs +7
-0+fn drop (&mut self) { +// SAFETY: `self.idx` was returned by a successful `drm_dev_enter()` call, as guaranteed +// by the type invariants of `UnbindGuard`. +unsafe { bindings::drm_dev_exit (self.idx) }; + } +} +impl <T : drm::Driver ,C :DeviceContext >Deref for Device <T ,C > {type Target =T ::Data ;rust/kernel/drm/driver.rs +8
-2@@ -187,8 +187,14 @@ unsafe impl<T: Driver> Send for Registration<T> {}impl <T :Driver >Drop for Registration <T > {fn drop (&mut self) { +// Use `drm_dev_unplug` rather than `drm_dev_unregister` to ensure that existing +// `drm_dev_enter()` critical sections complete before unregistration proceeds. This +// is required for the safety of `UnbindGuard`, which relies on the SRCU barrier in +// `drm_dev_unplug()` to guarantee that the parent device is still bound within the +// critical section. +// // SAFETY: Safe by the invariant of `ARef<drm::Device<T>>`. The existence of this -// `Registration` also guarantees the this `drm::Device` is actually registered. -unsafe { bindings::drm_dev_unregister (self.0 .as_raw ()) }; +// `Registration` also guarantees that this `drm::Device` is actually registered. +unsafe { bindings::drm_dev_unplug (self.0 .as_raw ()) }; } }
Danilo Krummrich
maintainerPATCH 3/6 rust: drm: Add RegistrationData to drm::Driver
In reply to Danilo KrummrichAdd 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.
--- 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(-)
drivers/gpu/drm/nova/driver.rs +4 -2
@@ -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 >;
drivers/gpu/drm/tyr/driver.rs +4 -2
@@ -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 >;
rust/kernel/drm/device.rs +40 -0
@@ -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 >;
rust/kernel/drm/driver.rs +72 -17
149 lines not on this page · raw message
rust/kernel/drm/mod.rs +1 -0
@@ -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 ;
Signature · 1 line
2.54.0
Re: [PATCH 3/6] rust: drm: Add RegistrationData to drm::Driver
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.
Quoted 416 lines · Danilo Krummrich
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::Devicebeforedrm_dev_register()to ensure it is already in place
when ioctls arrive.
UnbindGuard::registration_data()provides access with the lifetime shortened from 'static viaForLt::cast_ref. SinceRegistration::drop()
callsdrm_dev_unplug()-- which performs an SRCU barrier waiting for alldrm_dev_enter()critical sections to complete -- the data is guaranteed
to remain valid for the duration of any UnbindGuard.Signed-off-by Danilo Krummrich <[email protected]>---
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(-)drivers/gpu/drm/nova/driver.rs +0
-0@@ -12,7 +12,8 @@ioctl, //
},
prelude::*,
-sync::aref::ARef, //
+sync::aref::ARef,
+types::ForLt, //
};
usecrate::file::File;drivers/gpu/drm/nova/driver.rs +0
-0@@ -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()})
}drivers/gpu/drm/nova/driver.rs +1
-0@@ -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>;
drivers/gpu/drm/tyr/driver.rs +0
-0@@ -25,7 +25,8 @@ aref::ARef,Mutex, //
},
- time, //
+ time,
+types::ForLt, //
};
use crate::{drivers/gpu/drm/tyr/driver.rs +0
-0@@ -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(),drivers/gpu/drm/tyr/driver.rs +1
-0@@ -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>;
rust/kernel/drm/device.rs +0
-0@@ -23,6 +23,7 @@AlwaysRefCounted, //
},
types::{
+ ForLt,
NotThreadSafe,
Opaque, //
},rust/kernel/drm/device.rs +1
-0@@ -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_drmis 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 calldrm_dev_register.rust/kernel/drm/device.rs +1
-0@@ -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>, }rust/kernel/drm/device.rs +0
-0@@ -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 activedrm_dev_enter()critical
+ /// section (e.g. via [UnbindGuard]).
+ #[doc(hidden)]
+ pub unsafe fnraw_registration_data(&self)-> &<T::RegistrationData as ForLt>::Of<'_> {
+ // SAFETY: Caller guarantees the parent bus device is bound, hence
+ // the pointer is valid.
+ letstatic_ref= unsafe { (*self.registration_data.get()).as_ref() };rust/kernel/drm/device.rs +4
-0+ + T::RegistrationData::cast_ref(static_ref) + } +/// # Safety
///
///ptrmust be a valid pointer to astruct deviceembedded inSelf.rust/kernel/drm/device.rs +0
-0@@ -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)'sdropcalls + ///drm_dev_unplug()which waits for alldrm_dev_enter()critical sections to complete.
+ pub fnregistration_data(&self)-> &<T::RegistrationData as ForLt>::Of<'_> {
+ // SAFETY: The pointer was set inRegistration::new()beforedrm_dev_register(), and
+ // is only invalidated afterdrm_dev_unplug()inRegistration::drop(). Since we hold
+ // an activedrm_dev_enter()critical section, the SRCU barrier indrm_dev_unplug()
+ // guarantees the pointer is still valid.
+ unsafe {self.dev.raw_registration_data()}
+ }rust/kernel/drm/device.rs +2
-0+} +impl<T:
drm::Driver> Deref for UnbindGuard<'_, T> { type Target = T::ParentDevice<device::Bound>;rust/kernel/drm/driver.rs +2
-1@@ -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/exitcritical section), and references handed out by
+ /// [UnbindGuard::registration_data()](drm::device::UnbindGuard::registration_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;rust/kernel/drm/driver.rs +0
-0@@ -127,12 +138,44 @@ pub trait Driver { /// The registration type of a `drm::Device`. /// /// Once the `Registration` structure is dropped, the device isunregistered.
rust/kernel/drm/driver.rs +4
-1-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 asForLt>::Of<'static>>>,
+}
-impl<T: Driver> Registration<T> {
- fnnew(drm: drm::UnregisteredDevice<T>, flags: usize)-> Result<Self> {
- // SAFETY:drm.as_raw()is valid by the invariants ofdrm::Device.
-to_result(unsafe { bindings::drm_dev_register(drm.as_raw(), flags) })?;rust/kernel/drm/driver.rs +7
-0+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 asForLt>::Of<'bound>, E>,
+ flags: usize,
+ ) -> Result<Self>
+ where
+ Error: From<E>,
+ {
+ letreg_data: Pin<KBox<<T::RegistrationData as ForLt>::Of<'bound>>> =
+KBox::pin_init(reg_data, GFP_KERNEL)?;
+
+ // SAFETY:ForLtguarantees covariance; lifetimes do not affect layout.
+ letreg_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:drmis 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 calleddrm_dev_registerabove let new =NonNull::from(unsafe { drm.assume_ctx() });rust/kernel/drm/driver.rs +0
-0@@ -144,46 +187,55 @@ fn new(drm: drm::UnregisteredDevice<T>, flags: usize) -> Result<Self> { // one reference to the device - which we take ownershipover 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: Sinceregwas passed todevres::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 theDeviceinstance for this registration.
pub fndevice(&self)-> &drm::Device<T> {
- &self.0
+ &self.drm
}
}
// SAFETY:Registrationdoesn't offer any methods or access to fields when shared between
// threads, hence it's safe to share it.rust/kernel/drm/driver.rs +4
-1-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.rust/kernel/drm/driver.rs +4
-1-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> {
fndrop(&mut self){rust/kernel/drm/driver.rs +0
-0@@ -195,6 +247,9 @@ fn drop(&mut self) { // // SAFETY: Safe by the invariant of `ARef<drm::Device<T>>`.The existence of this
//Registrationalso guarantees that thisdrm::Deviceis actually registered.
- unsafe { bindings::drm_dev_unplug(self.0.as_raw()) }; + unsafe { bindings::drm_dev_unplug(self.drm.as_raw()) }; + // Afterdrm_dev_unplug(), the SRCU barrier guarantees that all UnbindGuard critical
+ // sections have completed, so no one holds a reference toreg_dataanymore.
+ //reg_datais dropped here automatically.
}
}rust/kernel/drm/mod.rs +1
-0@@ -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;
Re: [PATCH 3/6] rust: drm: Add RegistrationData to drm::Driver
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.
Quoted 468 lines · On Wed, 2026-05-27 at 15:21 -0400, [email protected] 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)
* RegisteredThe 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.
Quoted 438 lines · Danilo Krummrich
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 todrm::Devicebeforedrm_dev_register()to ensure it is already in place
when ioctls arrive.
UnbindGuard::registration_data()provides access with the lifetime shortened from 'static viaForLt::cast_ref. SinceRegistration::drop()
callsdrm_dev_unplug()-- which performs an SRCU barrier waiting for
alldrm_dev_enter()critical sections to complete -- the data is guaranteed
to remain valid for the duration of any UnbindGuard.Signed-off-by Danilo Krummrich <[email protected]>---
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(-)drivers/gpu/drm/nova/driver.rs +0
-0@@ -12,7 +12,8 @@ioctl, //
},
prelude::*,
-sync::aref::ARef, //
+sync::aref::ARef,
+types::ForLt, //
};
usecrate::file::File;drivers/gpu/drm/nova/driver.rs +0
-0@@ -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()})
}drivers/gpu/drm/nova/driver.rs +1
-0@@ -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>;drivers/gpu/drm/tyr/driver.rs +0
-0@@ -25,7 +25,8 @@ aref::ARef,Mutex, //
},
- time, //
+ time,
+types::ForLt, //
};
use crate::{drivers/gpu/drm/tyr/driver.rs +0
-0@@ -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(),drivers/gpu/drm/tyr/driver.rs +1
-0@@ -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>;rust/kernel/drm/device.rs +0
-0@@ -23,6 +23,7 @@AlwaysRefCounted, //
},
types::{
+ ForLt,
NotThreadSafe,
Opaque, //
},rust/kernel/drm/device.rs +1
-0@@ -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_drmis 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 calldrm_dev_register.rust/kernel/drm/device.rs +1
-0@@ -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>, }rust/kernel/drm/device.rs +0
-0@@ -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 activedrm_dev_enter()
critical
+ /// section (e.g. via [UnbindGuard]).
+ #[doc(hidden)]
+ pub unsafe fnraw_registration_data(&self)-> &<T::RegistrationData as ForLt>::Of<'_> {
+ // SAFETY: Caller guarantees the parent bus device is bound,
hence
+ // the pointer is valid.
+ letstatic_ref= unsafe { (*self.registration_data.get()).as_ref() };rust/kernel/drm/device.rs +4
-0+ + T::RegistrationData::cast_ref(static_ref) + } +/// # Safety
///
///ptrmust be a valid pointer to astruct deviceembedded
inSelf.rust/kernel/drm/device.rs +0
-0@@ -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)'sdropcalls + ///drm_dev_unplug()which waits for alldrm_dev_enter()critical sections to complete.
+ pub fnregistration_data(&self)-> &<T::RegistrationData as ForLt>::Of<'_> {
+ // SAFETY: The pointer was set inRegistration::new()beforedrm_dev_register(), and
+ // is only invalidated afterdrm_dev_unplug()inRegistration::drop(). Since we hold
+ // an activedrm_dev_enter()critical section, the SRCU barrier indrm_dev_unplug()
+ // guarantees the pointer is still valid.
+ unsafe {self.dev.raw_registration_data()}
+ }rust/kernel/drm/device.rs +2
-0+} +impl<T:
drm::Driver> Deref for UnbindGuard<'_, T> { type Target = T::ParentDevice<device::Bound>;rust/kernel/drm/driver.rs +2
-1@@ -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/exitcritical section), and references handed out by
+ /// [UnbindGuard::registration_data()](drm::device::UnbindGuard::regi stration_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;rust/kernel/drm/driver.rs +0
-0@@ -127,12 +138,44 @@ pub trait Driver { /// The registration type of a `drm::Device`. /// /// Once the `Registration` structure is dropped, the device isunregistered.
rust/kernel/drm/driver.rs +4
-1-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 asForLt>::Of<'static>>>,
+}
-impl<T: Driver> Registration<T> {
- fnnew(drm: drm::UnregisteredDevice<T>, flags: usize)-> Result<Self> {
- // SAFETY:drm.as_raw()is valid by the invariants ofdrm::Device.
- to_result(unsafe { bindings::drm_dev_register(drm.as_raw(),
flags) })?;rust/kernel/drm/driver.rs +7
-0+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 asForLt>::Of<'bound>, E>,
+ flags: usize,
+ ) -> Result<Self>
+ where
+ Error: From<E>,
+ {
+ letreg_data: Pin<KBox<<T::RegistrationData as ForLt>::Of<'bound>>> =
+KBox::pin_init(reg_data, GFP_KERNEL)?;
+
+ // SAFETY:ForLtguarantees covariance; lifetimes do not affect layout.
+ letreg_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:drmis 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 calleddrm_dev_registerabove let new =NonNull::from(unsafe { drm.assume_ctx() });rust/kernel/drm/driver.rs +0
-0@@ -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: Sinceregwas passed todevres::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 theDeviceinstance for this registration.
pub fndevice(&self)-> &drm::Device<T> {
- &self.0
+ &self.drm
}
}
// SAFETY:Registrationdoesn't offer any methods or access to fields when shared between
// threads, hence it's safe to share it.rust/kernel/drm/driver.rs +4
-1-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.rust/kernel/drm/driver.rs +4
-1-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> {
fndrop(&mut self){rust/kernel/drm/driver.rs +0
-0@@ -195,6 +247,9 @@ fn drop(&mut self) { // // SAFETY: Safe by the invariant of
ARef<drm::Device<T>>.
The existence of this
//Registrationalso guarantees that thisdrm::Deviceis
actually registered.
- unsafe { bindings::drm_dev_unplug(self.0.as_raw()) }; + unsafe { bindings::drm_dev_unplug(self.drm.as_raw()) }; + // Afterdrm_dev_unplug(), the SRCU barrier guarantees that
all UnbindGuard critical
+ // sections have completed, so no one holds a reference toreg_dataanymore.
+ //reg_datais dropped here automatically.
}
}rust/kernel/drm/mod.rs +1
-0@@ -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;
Re: [PATCH 3/6] rust: drm: Add RegistrationData to drm::Driver
Quoted 17 lines · On Thu, 2026-05-28 at 20:15 -0400, [email protected] wrote:
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 todrm::Driver::Datawhen 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.
Quoted 4 lines
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.
Actually - after working on it a little bit more, I think having the 3 states (Uninit, Initialized, Registered) is still going to make sense for KMS. So, I probably actually don't need to respin/change the DeviceContext series from how it is right now after all! Sorry for the noise :)
Danilo Krummrich
maintainerPATCH 4/6 rust: drm: Wrap ioctl dispatch in UnbindGuard
In reply to Danilo KrummrichRun every ioctl handler inside a drm_dev_enter/exit critical section via UnbindGuard. If the device has been unplugged, the ioctl returns ENODEV without calling the handler.
A free-function wrapper is added because the macro context prevents method resolution from inferring the driver type.
--- rust/kernel/drm/device.rs | 7 +++++++ rust/kernel/drm/ioctl.rs | 13 +++++++++++-- 2 files changed, 18 insertions(+), 2 deletions(-)
rust/kernel/drm/device.rs +7 -0
@@ -452,6 +452,13 @@ fn drop(&mut self) { } } +/// Free-function equivalent of [`Device::unbind_guard()`] for use in macro contexts where method +/// resolution cannot infer the driver type. +#[doc(hidden)] +pub fn unbind_guard <T : drm::Driver >(dev: &Device <T ,Registered >) ->Option <UnbindGuard <'_,T >> { + dev.unbind_guard () +} +impl <T : drm::Driver ,C :DeviceContext >Deref for Device <T ,C > {type Target =T ::Data ;
rust/kernel/drm/ioctl.rs +11 -2
@@ -87,7 +87,10 @@ pub mod internal {/// file: &kernel::drm::File<Self::File>, /// ) -> Result<u32> /// ``` -/// where `Self` is the drm::drv::Driver implementation these ioctls are being declared within. +/// where `Self` is the `drm::Driver` implementation these ioctls are being declared within. +/// +/// The ioctl runs inside a `drm_dev_enter/exit` critical section. If the device has been +/// unplugged, the ioctl returns `ENODEV` without calling the handler. /// /// # Examples ///
@@ -134,7 +137,13 @@ macro_rules! declare_drm_ioctls {// FIXME: Currently there is nothing enforcing that the types of the // dev/file match the current driver these ioctls are being declared // for, and it's not clear how to enforce this within the type system. -let dev = $crate::drm::device::Device ::from_raw (raw_dev); +let dev =unsafe { + $crate::drm::device::Device ::from_raw (raw_dev) + }; +let _guard =match $crate::drm::device::unbind_guard (dev) { +Some (g) => g, +None =>return $crate::error::code::ENODEV .to_errno (), + };// SAFETY: The ioctl argument has size `_IOC_SIZE(cmd)`, which we // asserted above matches the size of this type, and all bit patterns of // UAPI structs must be valid.
Signature · 1 line
2.54.0
Danilo Krummrich
maintainerPATCH 5/6 rust: drm: Pass registration data to ioctl handlers
In reply to Danilo KrummrichPass &<Self::RegistrationData as ForLt>::Of<'_> after the device argument to ioctl handlers. The lifetime is shortened from 'static via ForLt::cast_ref; the reference is valid for the duration of the drm_dev_enter/exit critical section held by UnbindGuard.
--- drivers/gpu/drm/nova/file.rs | 3 +++ drivers/gpu/drm/tyr/file.rs | 1 + rust/kernel/drm/ioctl.rs | 5 +++-- 3 files changed, 7 insertions(+), 2 deletions(-)
drivers/gpu/drm/nova/file.rs +3 -0
@@ -24,6 +24,7 @@ impl File {/// IOCTL: get_param: Query GPU / driver metadata. pub (crate )fn get_param ( dev: &NovaDevice , + _reg_data: &(), getparam: &mut uapi::drm_nova_getparam , _file: &drm::File <File >, ) ->Result <u32 > {
@@ -44,6 +45,7 @@ pub(crate) fn get_param(/// IOCTL: gem_create: Create a new DRM GEM object. pub (crate )fn gem_create ( dev: &NovaDevice , + _reg_data: &(), req: &mut uapi::drm_nova_gem_create , file: &drm::File <File >, ) ->Result <u32 > {
@@ -57,6 +59,7 @@ pub(crate) fn gem_create(/// IOCTL: gem_info: Query GEM metadata. pub (crate )fn gem_info ( _dev: &NovaDevice , + _reg_data: &(), req: &mut uapi::drm_nova_gem_info , file: &drm::File <File >, ) ->Result <u32 > {
drivers/gpu/drm/tyr/file.rs +1 -0
@@ -29,6 +29,7 @@ fn open(_dev: &drm::Device<Self::Driver>) -> Result<Pin<KBox<Self>>> {impl TyrDrmFileData {pub (crate )fn dev_query ( ddev: &TyrDrmDevice , + _reg_data: &(), devquery: &mut uapi::drm_panthor_dev_query , _file: &TyrDrmFile , ) ->Result <u32 > {
rust/kernel/drm/ioctl.rs +3 -2
@@ -83,6 +83,7 @@ pub mod internal {/// /// ```ignore /// fn foo(device: &kernel::drm::Device<Self>, +/// reg_data: &<Self::RegistrationData as kernel::types::ForLt>::Of<'_>, /// data: &mut uapi::argument_type, /// file: &kernel::drm::File<Self::File>, /// ) -> Result<u32>
@@ -140,7 +141,7 @@ macro_rules! declare_drm_ioctls {let dev =unsafe { $crate::drm::device::Device ::from_raw (raw_dev) }; -let _guard =match $crate::drm::device::unbind_guard (dev) { +let guard =match $crate::drm::device::unbind_guard (dev) {Some (g) => g,None =>return $crate::error::code::ENODEV .to_errno (), };
@@ -156,7 +157,7 @@ macro_rules! declare_drm_ioctls {// SAFETY: This is just the DRM file structure let file =unsafe { $crate::drm::File ::from_raw (raw_file) }; -match $func(dev, data, file) { +match $func(dev, guard.registration_data (), data, file) {Err (e) => e.to_errno (),Ok (i) => i.try_into () .unwrap_or ($crate::error::code::ERANGE .to_errno ()),
Signature · 1 line
2.54.0
Danilo Krummrich
maintainerPATCH 6/6 rust: drm: Pass bound parent device to ioctl handlers
In reply to Danilo KrummrichPass &Self::ParentDevice<Bound> as the second argument to ioctl handlers, obtained from the UnbindGuard via Deref. This gives ioctl handlers direct access to the bound parent bus device.
--- drivers/gpu/drm/nova/file.rs | 5 +++++ drivers/gpu/drm/tyr/file.rs | 3 +++ rust/kernel/drm/ioctl.rs | 3 ++- 3 files changed, 10 insertions(+), 1 deletion(-)
drivers/gpu/drm/nova/file.rs +5 -0
@@ -4,6 +4,8 @@use crate ::gem::NovaObject ;use kernel::{ alloc::flags::*, + auxiliary, + device::Bound , drm::{self, gem::BaseObject }, pci, prelude::*,
@@ -24,6 +26,7 @@ impl File {/// IOCTL: get_param: Query GPU / driver metadata. pub (crate )fn get_param ( dev: &NovaDevice , + _adev: &auxiliary::Device <Bound >, _reg_data: &(), getparam: &mut uapi::drm_nova_getparam , _file: &drm::File<File>,
@@ -45,6 +48,7 @@ pub(crate) fn get_param(/// IOCTL: gem_create: Create a new DRM GEM object. pub (crate )fn gem_create ( dev: &NovaDevice , + _adev: &auxiliary::Device <Bound >, _reg_data: &(), req: &mut uapi::drm_nova_gem_create , file: &drm::File<File>,
@@ -59,6 +63,7 @@ pub(crate) fn gem_create(/// IOCTL: gem_info: Query GEM metadata. pub (crate )fn gem_info ( _dev: &NovaDevice , + _adev: &auxiliary::Device <Bound >, _reg_data: &(), req: &mut uapi::drm_nova_gem_info , file: &drm::File<File>,
drivers/gpu/drm/tyr/file.rs +3 -0
@@ -1,7 +1,9 @@// SPDX-License-Identifier: GPL-2.0 or MIT use kernel::{ + device, drm, + platform, prelude::*, uaccess::UserSlice , uapi,//
@@ -29,6 +31,7 @@ fn open(_dev: &drm::Device<Self::Driver>) -> Result<Pin<KBox<Self>>> {impl TyrDrmFileData {pub (crate )fn dev_query ( ddev: &TyrDrmDevice , + _pdev: &platform::Device <device::Bound >, _reg_data: &(), devquery: &mut uapi::drm_panthor_dev_query , _file: &TyrDrmFile,
rust/kernel/drm/ioctl.rs +2 -1
@@ -83,6 +83,7 @@ pub mod internal {/// /// ```ignore /// fn foo(device: &kernel::drm::Device<Self>, +/// parent: &Self::ParentDevice<kernel::device::Bound>, /// reg_data: &<Self::RegistrationData as kernel::types::ForLt>::Of<'_>, /// data: &mut uapi::argument_type, /// file: &kernel::drm::File<Self::File>,
@@ -157,7 +158,7 @@ macro_rules! declare_drm_ioctls {// SAFETY: This is just the DRM file structure let file =unsafe { $crate::drm::File ::from_raw (raw_file) }; -match $func(dev, guard.registration_data (), data, file) { +match $func(dev, &*guard, guard.registration_data (), data, file) {Err (e) => e.to_errno (),Ok (i) => i.try_into () .unwrap_or ($crate::error::code::ERANGE .to_errno ()),
Signature · 1 line
2.54.0
Danilo Krummrich
maintainerQuoted 7 lines · Danilo Krummrich
Danilo Krummrich (6):
rust: drm: AddDriver::ParentDeviceassociated type
rust: drm: Add UnbindGuard for drm_dev_enter/exit critical sections
rust: drm: Add RegistrationData todrm::Driver
rust: drm: Wrap ioctl dispatch in UnbindGuard
rust: drm: Pass registration data to ioctl handlers
rust: drm: Pass bound parent device to ioctl handlers
Deborah, following up on your question in the other thread, this is roughly how you'd use this in Tyr.
drivers/gpu/drm/tyr/driver.rs +10 -4
@@ -37,13 +37,17 @@ regs, // }; -pub (crate )type IoMem = kernel::io::Mmio <SZ_2M >; +pub (crate )type IoMem <'bound> = kernel::io::mem::IoMem <'bound,SZ_2M >;pub (crate )struct TyrDrmDriver ;/// Convenience type alias for the DRM device type for this driver. pub (crate )type TyrDrmDevice <Ctx = drm::Registered > = drm::Device <TyrDrmDriver ,Ctx >; +pub (crate )struct RegData <'bound> { +pub (crate ) iomem:IoMem <'bound>, +} +#[pin_data( PinnedDrop )] pub (crate )struct TyrPlatformDriverData { _device:ARef <TyrDrmDevice >,
@@ -65,7 +69,7 @@ pub(crate) struct TyrDrmDeviceData { pub(crate) gpu_info: GpuInfo, } -fn issue_soft_reset (dev: &Device <Bound >, iomem: &IoMem ) ->Result { +fn issue_soft_reset (dev: &Device <Bound >, iomem: &IoMem <'_>) ->Result { regs::GPU_CMD .write (iomem, regs::GPU_CMD_SOFT_RESET ); poll::read_poll_timeout(
@@ -133,8 +137,10 @@ fn probe( gpu_info, }); +let reg_data =RegData { iomem }; +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 (), reg_data,0 )?;let driver =TyrPlatformDriverData { _device: tdev.into (),
@@ -176,7 +182,7 @@ fn drop(self: Pin<&mut Self>) {#[vtable] impl drm::Driver for TyrDrmDriver {type Data =TyrDrmDeviceData ; -type RegistrationData =ForLt! (()); +type RegistrationData =ForLt! (for <'a>RegData <'a>);type File =TyrDrmFileData ;type Object <R : drm::DeviceContext > = drm::gem::Object <TyrObject ,R >;type ParentDevice <Ctx :DeviceContext > = platform::Device <Ctx >;
drivers/gpu/drm/tyr/file.rs +4 -1
@@ -10,6 +10,7 @@ };use crate ::driver::{ +RegData ,TyrDrmDevice ,TyrDrmDriver ,// };
@@ -32,10 +33,12 @@ impl TyrDrmFileData {pub (crate )fn dev_query ( ddev: &TyrDrmDevice , _pdev: &platform::Device <device::Bound >, - _reg_data: &(), + reg_data: &RegData <'_>, devquery: &mut uapi::drm_panthor_dev_query , _file: &TyrDrmFile , ) ->Result <u32 > { +let _iomem = reg_data.iomem; +if devquery.pointer ==0 {match devquery.type_ { uapi::drm_panthor_dev_query_type_DRM_PANTHOR_DEV_QUERY_GPU_INFO => {
drivers/gpu/drm/tyr/gpu.rs +2 -2
@@ -34,7 +34,7 @@pub (crate )struct GpuInfo (pub (crate ) uapi::drm_panthor_gpu_info );impl GpuInfo { -pub (crate )fn new (iomem: &IoMem ) ->Self { +pub (crate )fn new (iomem: &IoMem <'_>) ->Self {let gpu_id = regs::GPU_ID .read (iomem);let csf_id = regs::GPU_CSF_ID .read (iomem);let gpu_rev = regs::GPU_REVID .read (iomem);
@@ -206,7 +206,7 @@ fn from(value: u32) -> Self { }/// Powers on the l2 block. -pub (crate )fn l2_power_on (dev: &Device <Bound >, iomem: &IoMem ) ->Result { +pub (crate )fn l2_power_on (dev: &Device <Bound >, iomem: &IoMem <'_>) ->Result { regs::L2_PWRON_LO .write (iomem,1 ); poll::read_poll_timeout(
drivers/gpu/drm/tyr/regs.rs +2 -2
@@ -20,12 +20,12 @@impl <const OFFSET :usize >Register <OFFSET > {#[inline] -pub (crate )fn read (&self, iomem: &IoMem ) ->u32 { +pub (crate )fn read (&self, iomem: &IoMem <'_>) ->u32 { iomem.read32 (OFFSET ) }#[inline] -pub (crate )fn write (&self, iomem: &IoMem , value:u32 ) { +pub (crate )fn write (&self, iomem: &IoMem <'_>, value:u32 ) { iomem.write32 (value,OFFSET ); } }
Quoted 11 lines · Danilo Krummrich
Quoted 7 lines · Danilo Krummrich
Danilo Krummrich (6):
rust: drm: AddDriver::ParentDeviceassociated type
rust: drm: Add UnbindGuard for drm_dev_enter/exit critical sections
rust: drm: Add RegistrationData todrm::Driver
rust: drm: Wrap ioctl dispatch in UnbindGuard
rust: drm: Pass registration data to ioctl handlers
rust: drm: Pass bound parent device to ioctl handlersDeborah, following up on your question in the other thread, this is roughly how you'd use this in Tyr.
Hi Danilo,
Thanks for this advice, I've been working on applying these changes to Tyr's MCU/firmware boot series. One problem i've had is that we used to be able to get an unregistered device without providing the drm::Driver type Data like this:
let unreg_dev = drm::UnregisteredDevice::<TyrDrmDriver>::new(pdev)?;
Then Tyr used that unregistered device to create a gem::shmem::Object and initialize a gpuvm tree which we used to map the shared memory to load the firmware.
Has the design changed so that the drm::Driver type Data must be provided now to get the unregistered devices? Like this?
let unreg_dev = drm::UnregisteredDevice::<TyrDrmDriver>::new(pdev, data)?;
Just checking that this is what we need to work with? If so, I was thinking of just simplifying the firmware boot to use a small coherent dma allocation instead like this:
https://gitlab.freedesktop.org/dbrouwer/linux/-/merge_requests/3/diffs?commit_id=39be9140e22c6252317082a1424a66c35dc004b3
Could you let me know if this aligns with your driver core changes?
Deborah
Quoted 121 lines
drivers/gpu/drm/tyr/driver.rs +10
-4@@ -37,13 +37,17 @@ regs, // }; -pub (crate )type IoMem = kernel::io::Mmio <SZ_2M >; +pub (crate )type IoMem <'bound> = kernel::io::mem::IoMem <'bound,SZ_2M >;pub (crate )struct TyrDrmDriver ;/// Convenience type alias for the DRM device type for this driver. pub (crate )type TyrDrmDevice <Ctx = drm::Registered > = drm::Device <TyrDrmDriver ,Ctx >; +pub (crate )struct RegData <'bound> { +pub (crate ) iomem:IoMem <'bound>, +} +#[pin_data( PinnedDrop )] pub (crate )struct TyrPlatformDriverData { _device:ARef <TyrDrmDevice >,@@ -65,7 +69,7 @@ pub(crate) struct TyrDrmDeviceData { pub(crate) gpu_info: GpuInfo, } -fn issue_soft_reset (dev: &Device <Bound >, iomem: &IoMem ) ->Result { +fn issue_soft_reset (dev: &Device <Bound >, iomem: &IoMem <'_>) ->Result { regs::GPU_CMD .write (iomem, regs::GPU_CMD_SOFT_RESET ); poll::read_poll_timeout(@@ -133,8 +137,10 @@ fn probe( gpu_info, }); +let reg_data =RegData { iomem }; +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 (), reg_data,0 )?;let driver =TyrPlatformDriverData { _device: tdev.into (),@@ -176,7 +182,7 @@ fn drop(self: Pin<&mut Self>) {#[vtable] impl drm::Driver for TyrDrmDriver {type Data =TyrDrmDeviceData ; -type RegistrationData =ForLt! (()); +type RegistrationData =ForLt! (for <'a>RegData <'a>);type File =TyrDrmFileData ;type Object <R : drm::DeviceContext > = drm::gem::Object <TyrObject ,R >;type ParentDevice <Ctx :DeviceContext > = platform::Device <Ctx >;drivers/gpu/drm/tyr/file.rs +4
-1@@ -10,6 +10,7 @@ };use crate ::driver::{ +RegData ,TyrDrmDevice ,TyrDrmDriver ,// };@@ -32,10 +33,12 @@ impl TyrDrmFileData {pub (crate )fn dev_query ( ddev: &TyrDrmDevice , _pdev: &platform::Device <device::Bound >, - _reg_data: &(), + reg_data: &RegData <'_>, devquery: &mut uapi::drm_panthor_dev_query , _file: &TyrDrmFile , ) ->Result <u32 > { +let _iomem = reg_data.iomem; +if devquery.pointer ==0 {match devquery.type_ { uapi::drm_panthor_dev_query_type_DRM_PANTHOR_DEV_QUERY_GPU_INFO => {drivers/gpu/drm/tyr/gpu.rs +2
-2@@ -34,7 +34,7 @@pub (crate )struct GpuInfo (pub (crate ) uapi::drm_panthor_gpu_info );impl GpuInfo { -pub (crate )fn new (iomem: &IoMem ) ->Self { +pub (crate )fn new (iomem: &IoMem <'_>) ->Self {let gpu_id = regs::GPU_ID .read (iomem);let csf_id = regs::GPU_CSF_ID .read (iomem);let gpu_rev = regs::GPU_REVID .read (iomem);@@ -206,7 +206,7 @@ fn from(value: u32) -> Self { }/// Powers on the l2 block. -pub (crate )fn l2_power_on (dev: &Device <Bound >, iomem: &IoMem ) ->Result { +pub (crate )fn l2_power_on (dev: &Device <Bound >, iomem: &IoMem <'_>) ->Result { regs::L2_PWRON_LO .write (iomem,1 ); poll::read_poll_timeout(drivers/gpu/drm/tyr/regs.rs +2
-2@@ -20,12 +20,12 @@impl <const OFFSET :usize >Register <OFFSET > {#[inline] -pub (crate )fn read (&self, iomem: &IoMem ) ->u32 { +pub (crate )fn read (&self, iomem: &IoMem <'_>) ->u32 { iomem.read32 (OFFSET ) }#[inline] -pub (crate )fn write (&self, iomem: &IoMem , value:u32 ) { +pub (crate )fn write (&self, iomem: &IoMem <'_>, value:u32 ) { iomem.write32 (value,OFFSET ); } }
Danilo Krummrich
maintainerQuoted 1 line · Deborah Brouwer
let
unreg_dev= drm::UnregisteredDevice::<TyrDrmDriver>::new(pdev, data)?;
You shouldn't need this anymore as the drm::Registration itself has private data now that is bound to the lifetime of the underlying bus device, which should be the correct lifetime for juggling the GPU page tables.
Quoted 6 lines · On Thu, 14 May 2026 21:07:11 +0200 "Danilo Krummrich" <[email protected]> wrote:
Quoted 1 line · Deborah Brouwer
let
unreg_dev= drm::UnregisteredDevice::<TyrDrmDriver>::new(pdev, data)?;You shouldn't need this anymore as the
drm::Registrationitself has private data now that is bound to the lifetime of the underlying bus device, which should be the correct lifetime for juggling the GPU page tables.
The problem we have is that, to initialize some of the sub-components of the driver, we need to be able to allocate GEM objects before the DRM device is exposed (registered), and because the data we want to attach to the final registration contains these sub-components, we need to defer the data assignment to the registration step (which was allowed by [1], but apparently this was dropped from the latest version of the series for some reason).
Quoted 23 lines · Boris Brezillon
Quoted 9 lines · On Thu, 14 May 2026 21:07:11 +0200 "Danilo Krummrich" <[email protected]> wrote:
Quoted 2 lines · Deborah Brouwer
let
unreg_dev= drm::UnregisteredDevice::<TyrDrmDriver>::new(pdev, data)?;You shouldn't need this anymore as the
drm::Registrationitself has private data
now that is bound to the lifetime of the underlying bus device, which should be
the correct lifetime for juggling the GPU page tables.The problem we have is that, to initialize some of the sub-components of the driver, we need to be able to allocate GEM objects before the DRM
device is exposed (registered), and because the data we want to attach
to the final registration contains these sub-components, we need to defer the data assignment to the registration step (which was allowed by [1], but apparently this was dropped from the latest version of the series for some reason).
Quoted 3 lines
[1]
https://lore.kernel.org/dri-devel/20260320233645.950190-4-lyude@redha
t.com/
I dropped it because when talking with Danilo I had the impression that we weren't going to need this (and I thought we explicitly had mentioned it when talking with eachother). But I'm always happy to add it back
Danilo Krummrich
maintainerQuoted 10 lines · Boris Brezillon
Quoted 6 lines · On Thu, 14 May 2026 21:07:11 +0200 "Danilo Krummrich" <[email protected]> wrote:
Quoted 1 line · Deborah Brouwer
let
unreg_dev= drm::UnregisteredDevice::<TyrDrmDriver>::new(pdev, data)?;You shouldn't need this anymore as the
drm::Registrationitself has private data now that is bound to the lifetime of the underlying bus device, which should be the correct lifetime for juggling the GPU page tables.
Quoted 7 lines
The problem we have is that, to initialize some of the sub-components of the driver, we need to be able to allocate GEM objects before the DRM device is exposed (registered), and because the data we want to attach to the final registration contains these sub-components, we need to defer the data assignment to the registration step (which was allowed by [1], but apparently this was dropped from the latest version of the series for some reason).
I'm perfectly aware, what I'm saying above is that you probably want to store those GEM objects (that you can create with the previously allocated drm::Device) in the drm::Registration data. This fulfills this requirement *and* ties the lifetime of those GEM objects to the lifetime of the underlying bus device being bound to the driver, which is exactly what you want for GEM objects that contain the device page tables.
Quoted 19 lines · On Tue, 19 May 2026 21:28:17 +0200 "Danilo Krummrich" <[email protected]> wrote:
Quoted 10 lines · Boris Brezillon
Quoted 6 lines · On Thu, 14 May 2026 21:07:11 +0200 "Danilo Krummrich" <[email protected]> wrote:
Quoted 1 line · Deborah Brouwer
let
unreg_dev= drm::UnregisteredDevice::<TyrDrmDriver>::new(pdev, data)?;You shouldn't need this anymore as the
drm::Registrationitself has private data now that is bound to the lifetime of the underlying bus device, which should be the correct lifetime for juggling the GPU page tables.Quoted 7 lines
The problem we have is that, to initialize some of the sub-components of the driver, we need to be able to allocate GEM objects before the DRM device is exposed (registered), and because the data we want to attach to the final registration contains these sub-components, we need to defer the data assignment to the registration step (which was allowed by [1], but apparently this was dropped from the latest version of the series for some reason).
Quoted 6 lines
I'm perfectly aware, what I'm saying above is that you probably want to store those GEM objects (that you can create with the previously allocated
drm::Device) in thedrm::Registrationdata. This fulfills this requirement *and* ties the lifetime of those GEM objects to the lifetime of the underlying bus device being bound to the driver, which is exactly what you want for GEM objects that contain the device page tables.
Yeah, I don't think the problem Deborah was mentioning was about not being able to allocate GEMs early on, or making sure those GEMs don't outlive the bus-device. Oh, and BTW, we don't use GEMs for the page tables themselves, those still go through regular page allocation and are entirely handled by the io-pgtable-arm.c logic. We need early GEM allocation to populate FW sections, and to map those to the VM attached to the FW we need GPUVM too (because the FW also lives behind the GPU MMU and has its own page table that's populated by the driver).
What I think Deborah was pointing out is the chicken-and-egg issue, where the registration data is only fully populated after all sub-components of the drivers have been initialized, and one of these sub-components (the FW) needs to allocate GEMs and map them to its VM. So, GEM allocation and GPUVM are required are required early on, and both want a drm::Device of some sort (Unregistered or Registered). So the question is, are you happy with having [1] applied before/after your patchset to address this chicken-and-egg issue I mentioned, or are you proposing something else (Option<Subcomponent> so that we start with all sub-components set to None and progressively populate those, which I remember Daniel was strongly opposed to)?
Danilo Krummrich
maintainerQuoted 4 lines · Boris Brezillon
So, GEM allocation and GPUVM are required are required early on, and both want a
drm::Deviceof some sort (Unregistered or Registered). So the question is, are you happy with having [1] applied before/after your patchset to address this chicken-and-egg issue I mentioned,
Yes, this is exactly what I had in mind for all of my replies, but I think I only implied my key point rather than making it explicit, so let me do that now.
What I'm saying is that we now have all DRM ioctls behind the SRCU guard, which means that the drm::Registration data is always available from DRM ioctls.
Now, with this precondition, the real question is whether there are any resources left that should be bound to the lifetime of the DRM device itself, which I don't think is the case.
After device unbind nothing is able to call back into the driver anymore, so there is no reason to keep the corresponding data alive within the driver's private data.
There may be resources that are kept alive through a separate reference count while userspace still has open file descriptors, but they are separate reference counts and should be independent from the driver's private data.
IWO, the fact that we guard the ioctls makes the whole class of problems go away that we inherit from having to keep the driver's private data alive until after remvoe() -- the DRM device private data becomes obsolete and we should probably evene remove it.
Instead, we should use the drm::Registration private data for everything.
With this, your chicken-and-egg issue is already solved; the DRM device is available, you can populate all the resources you need (GEM, GPUVM, etc.) and then pass it into drm::Registration::new().
In terms of making any of the resources you store in the drm::Registration data available in the bus device private data, it becomes the same pattern as for everything else, i.e. you need shared ownership.
So, let's say you need IoMem<'bound> in both the drm::Registration data and in the bus device private data, then you currently need Arc<IoMem<'bound>> in both of them, which is fine, but I think we can do better.
Eventually, once we have landed the series I currently have in flight and once Gary finished his self-referencial support in pin-init, I think we can even avoid the reference count. Here is an example.
struct TyrPlatformDriverData<'bound> {
_device: ARef<TyrDrmDevice>,
_reg: drm::Registration<'bound, TyrDrmData<'_>>,
iomem: IoMem<'bound>,
}
struct TyrDrmData<'bound> {
iomem: &'bound IoMem<'bound>,
}
fn probe() -> ... {
try_pin_init!(TyrPlatformDriverData {
_device: ...,
iomem <- ...,
_reg <- drm::Registration::new(..., TyrDrmData { iomem: &iomem }),
})
}I don't know how the pin-init self-referencial stuff will exactly turn out, but this is pretty much what I hope we can evolve this into.
(And sorry again, now that I spelled this all out, having that left implicit in my first reply was obviously unreasonable. :)
Quoted 3 lines
or are you proposing something else (Option<Subcomponent> so that we start with all sub-components set to None and progressively populate those, which I remember Daniel was strongly opposed to)?
No, I am strongly opposed to do that (as well).
Quoted 29 lines · On Wed, 20 May 2026 12:55:40 +0200 "Danilo Krummrich" <[email protected]> wrote:
Quoted 4 lines · Boris Brezillon
So, GEM allocation and GPUVM are required are required early on, and both want a
drm::Deviceof some sort (Unregistered or Registered). So the question is, are you happy with having [1] applied before/after your patchset to address this chicken-and-egg issue I mentioned,Yes, this is exactly what I had in mind for all of my replies, but I think I only implied my key point rather than making it explicit, so let me do that now.
What I'm saying is that we now have all DRM ioctls behind the SRCU guard, which means that the
drm::Registrationdata is always available from DRM ioctls.Now, with this precondition, the real question is whether there are any resources left that should be bound to the lifetime of the DRM device itself, which I don't think is the case.
After device unbind nothing is able to call back into the driver anymore, so there is no reason to keep the corresponding data alive within the driver's private data.
There may be resources that are kept alive through a separate reference count while userspace still has open file descriptors, but they are separate reference counts and should be independent from the driver's private data.
IWO, the fact that we guard the ioctls makes the whole class of problems go away that we inherit from having to keep the driver's private data alive until after
remvoe()-- the DRM device private data becomes obsolete and we should probably evene remove it.
Quoted 5 lines
Instead, we should use the
drm::Registrationprivate data for everything.With this, your chicken-and-egg issue is already solved; the DRM device is available, you can populate all the resources you need (GEM, GPUVM, etc.) and then pass it into
drm::Registration::new().
Okay, it starts to click. So ioctls get passed the drm::Registration, which has both a device and some driver specific data (I guess it's even hidden behind the File abstraction). During early probe, we'd still be able to pass an Unregistered variant of the device, and then our current TyrDrmDeviceData would be attached to the Registration object when Registration::new_foreign_owned() is called.
This probably imply changing the places where we currently access our TyrDrmDeviceData through the drm::Device, but hopefully there's not many of those cases.
Quoted 4 lines
In terms of making any of the resources you store in the
drm::Registrationdata available in the bus device private data, it becomes the same pattern as for everything else, i.e. you need shared ownership.
Yep, that we can solve.
Quoted 27 lines
So, let's say you need IoMem<'bound> in both the
drm::Registrationdata and in the bus device private data, then you currently need Arc<IoMem<'bound>> in both of them, which is fine, but I think we can do better.Eventually, once we have landed the series I currently have in flight and once Gary finished his self-referencial support in pin-init, I think we can even avoid the reference count. Here is an example.
struct TyrPlatformDriverData<'bound> { _device: ARef<TyrDrmDevice>, _reg: drm::Registration<'bound, TyrDrmData<'_>>, iomem: IoMem<'bound>, } struct TyrDrmData<'bound> { iomem: &'bound IoMem<'bound>, } fn probe() -> ... { try_pin_init!(TyrPlatformDriverData { _device: ..., iomem <- ..., _reg <- drm::Registration::new(..., TyrDrmData { iomem: &iomem }), }) }
Quoted 2 lines
I don't know how the pin-init self-referencial stuff will exactly turn out, but this is pretty much what I hope we can evolve this into.
Yep, makes sense.
Quoted 3 lines
(And sorry again, now that I spelled this all out, having that left implicit in my first reply was obviously unreasonable. :)
No worries, that's the usual back-and-forth happening during reviews, and it's all fine.
Quoted 6 lines
Quoted 4 lines
or are you proposing something else (Option<Subcomponent> so that we start with all sub-components set to None and progressively populate those, which I remember Daniel was strongly opposed to)?
No, I am strongly opposed to do that (as well).
Got it.