Message-Id: <F713E667-3414-4BBF-9820-FA21C7F1A4CE@collabora.com>
Date: Mon, 22 Dec 2025 10:11:51 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Lyude Paul <lyude@...hat.com>
Cc: linux-kernel@...r.kernel.org,
 rust-for-linux@...r.kernel.org,
 dri-devel@...ts.freedesktop.org,
 Danilo Krummrich <dakr@...nel.org>,
 Alice Ryhl <aliceryhl@...gle.com>,
 David Airlie <airlied@...il.com>,
 Simona Vetter <simona@...ll.ch>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>,
 Thomas Zimmermann <tzimmermann@...e.de>,
 Miguel Ojeda <ojeda@...nel.org>,
 Alex Gaynor <alex.gaynor@...il.com>,
 Boqun Feng <boqun.feng@...il.com>,
 Gary Guo <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>,
 Benno Lossin <lossin@...nel.org>,
 Andreas Hindborg <a.hindborg@...nel.org>,
 Trevor Gross <tmgross@...ch.edu>,
 Asahi Lina <lina+kernel@...hilina.net>,
 Shankari Anand <shankari.ak0208@...il.com>,
 "open list:DRM DRIVER FOR NVIDIA GPUS [RUST]" <nouveau@...ts.freedesktop.org>
Subject: Re: [PATCH 2/2] rust/drm/gem: Use DeviceCtx with GEM objects



> On 7 Nov 2025, at 16:23, Lyude Paul <lyude@...hat.com> wrote:
> 
> Now that we have the ability to represent the context in which a DRM device
> is in at compile-time, we can start carrying around this context with GEM
> object types in order to allow a driver to safely create GEM objects before
> a DRM device has registered with userspace.
> 
> Signed-off-by: Lyude Paul <lyude@...hat.com>
> ---
> drivers/gpu/drm/nova/driver.rs |  2 +-
> drivers/gpu/drm/nova/gem.rs    | 11 +++---
> drivers/gpu/drm/tyr/driver.rs  |  2 +-
> drivers/gpu/drm/tyr/gem.rs     |  3 +-
> rust/kernel/drm/device.rs      | 14 ++++----
> rust/kernel/drm/driver.rs      |  2 +-
> rust/kernel/drm/gem/mod.rs     | 66 ++++++++++++++++++++++------------
> 7 files changed, 63 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nova/driver.rs b/drivers/gpu/drm/nova/driver.rs
> index c78d69d5f045a..54245ee439042 100644
> --- a/drivers/gpu/drm/nova/driver.rs
> +++ b/drivers/gpu/drm/nova/driver.rs
> @@ -59,7 +59,7 @@ fn probe(adev: &auxiliary::Device<Core>, _info: &Self::IdInfo) -> Result<Pin<KBo
> impl drm::Driver for NovaDriver {
>     type Data = NovaData;
>     type File = File;
> -    type Object = gem::Object<NovaObject>;
> +    type Object<Ctx: drm::DeviceCtx> = gem::Object<NovaObject, Ctx>;
> 
>     const INFO: drm::DriverInfo = INFO;
> 
> diff --git a/drivers/gpu/drm/nova/gem.rs b/drivers/gpu/drm/nova/gem.rs
> index 2760ba4f3450b..14d275dc74ce7 100644
> --- a/drivers/gpu/drm/nova/gem.rs
> +++ b/drivers/gpu/drm/nova/gem.rs
> @@ -2,7 +2,7 @@
> 
> use kernel::{
>     drm,
> -    drm::{gem, gem::BaseObject},
> +    drm::{gem, gem::BaseObject, DeviceCtx},
>     prelude::*,
>     sync::aref::ARef,
> };
> @@ -19,21 +19,24 @@ pub(crate) struct NovaObject {}
> impl gem::DriverObject for NovaObject {
>     type Driver = NovaDriver;
> 
> -    fn new(_dev: &NovaDevice, _size: usize) -> impl PinInit<Self, Error> {
> +    fn new<Ctx: DeviceCtx>(_dev: &NovaDevice<Ctx>, _size: usize) -> impl PinInit<Self, Error> {
>         try_pin_init!(NovaObject {})
>     }
> }
> 
> impl NovaObject {
>     /// Create a new DRM GEM object.
> -    pub(crate) fn new(dev: &NovaDevice, size: usize) -> Result<ARef<gem::Object<Self>>> {
> +    pub(crate) fn new<Ctx: DeviceCtx>(
> +        dev: &NovaDevice<Ctx>,
> +        size: usize,
> +    ) -> Result<ARef<gem::Object<Self, Ctx>>> {
>         let aligned_size = size.next_multiple_of(1 << 12);
> 
>         if size == 0 || size > aligned_size {
>             return Err(EINVAL);
>         }
> 
> -        gem::Object::new(dev, aligned_size)
> +        gem::Object::<Self, Ctx>::new(dev, aligned_size)
>     }
> 
>     /// Look up a GEM object handle for a `File` and return an `ObjectRef` for it.
> diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
> index e3ea5ad85f49b..78fc08945e08e 100644
> --- a/drivers/gpu/drm/tyr/driver.rs
> +++ b/drivers/gpu/drm/tyr/driver.rs
> @@ -187,7 +187,7 @@ fn drop(self: Pin<&mut Self>) {
> impl drm::Driver for TyrDriver {
>     type Data = TyrData;
>     type File = File;
> -    type Object = drm::gem::Object<TyrObject>;
> +    type Object<R: drm::DeviceCtx> = drm::gem::Object<TyrObject, R>;
> 
>     const INFO: drm::DriverInfo = INFO;
> 
> diff --git a/drivers/gpu/drm/tyr/gem.rs b/drivers/gpu/drm/tyr/gem.rs
> index 1273bf89dbd5d..5e1403409f468 100644
> --- a/drivers/gpu/drm/tyr/gem.rs
> +++ b/drivers/gpu/drm/tyr/gem.rs
> @@ -3,6 +3,7 @@
> use crate::driver::TyrDevice;
> use crate::driver::TyrDriver;
> use kernel::drm::gem;
> +use kernel::drm::DeviceCtx;
> use kernel::prelude::*;
> 
> /// GEM Object inner driver data
> @@ -12,7 +13,7 @@ pub(crate) struct TyrObject {}
> impl gem::DriverObject for TyrObject {
>     type Driver = TyrDriver;
> 
> -    fn new(_dev: &TyrDevice, _size: usize) -> impl PinInit<Self, Error> {
> +    fn new<Ctx: DeviceCtx>(_dev: &TyrDevice<Ctx>, _size: usize) -> impl PinInit<Self, Error> {
>         try_pin_init!(TyrObject {})
>     }
> }
> diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
> index 00072984930a3..227df6bf88c31 100644
> --- a/rust/kernel/drm/device.rs
> +++ b/rust/kernel/drm/device.rs
> @@ -127,13 +127,13 @@ impl<T: drm::Driver> UnregisteredDevice<T> {
>         master_set: None,
>         master_drop: None,
>         debugfs_init: None,
> -        gem_create_object: T::Object::ALLOC_OPS.gem_create_object,
> -        prime_handle_to_fd: T::Object::ALLOC_OPS.prime_handle_to_fd,
> -        prime_fd_to_handle: T::Object::ALLOC_OPS.prime_fd_to_handle,
> -        gem_prime_import: T::Object::ALLOC_OPS.gem_prime_import,
> -        gem_prime_import_sg_table: T::Object::ALLOC_OPS.gem_prime_import_sg_table,
> -        dumb_create: T::Object::ALLOC_OPS.dumb_create,
> -        dumb_map_offset: T::Object::ALLOC_OPS.dumb_map_offset,
> +        gem_create_object: T::Object::<AnyCtx>::ALLOC_OPS.gem_create_object,
> +        prime_handle_to_fd: T::Object::<AnyCtx>::ALLOC_OPS.prime_handle_to_fd,
> +        prime_fd_to_handle: T::Object::<AnyCtx>::ALLOC_OPS.prime_fd_to_handle,
> +        gem_prime_import: T::Object::<AnyCtx>::ALLOC_OPS.gem_prime_import,
> +        gem_prime_import_sg_table: T::Object::<AnyCtx>::ALLOC_OPS.gem_prime_import_sg_table,
> +        dumb_create: T::Object::<AnyCtx>::ALLOC_OPS.dumb_create,
> +        dumb_map_offset: T::Object::<AnyCtx>::ALLOC_OPS.dumb_map_offset,
>         show_fdinfo: None,
>         fbdev_probe: None,
> 
> diff --git a/rust/kernel/drm/driver.rs b/rust/kernel/drm/driver.rs
> index fde0447566bef..4e8735e9018c4 100644
> --- a/rust/kernel/drm/driver.rs
> +++ b/rust/kernel/drm/driver.rs
> @@ -104,7 +104,7 @@ pub trait Driver {
>     type Data: Sync + Send;
> 
>     /// The type used to manage memory for this driver.
> -    type Object: AllocImpl;
> +    type Object<Ctx: drm::DeviceCtx>: AllocImpl;
> 
>     /// The type used to represent a DRM File (client)
>     type File: drm::file::DriverFile;
> diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs
> index eb5f3feac8907..62ca11126e1c0 100644
> --- a/rust/kernel/drm/gem/mod.rs
> +++ b/rust/kernel/drm/gem/mod.rs
> @@ -7,13 +7,16 @@
> use crate::{
>     alloc::flags::*,
>     bindings, drm,
> -    drm::driver::{AllocImpl, AllocOps},
> +    drm::{
> +        device::{DeviceCtx, Registered},
> +        driver::{AllocImpl, AllocOps},
> +    },
>     error::{to_result, Result},
>     prelude::*,
>     sync::aref::{ARef, AlwaysRefCounted},
>     types::Opaque,
> };
> -use core::{ops::Deref, ptr::NonNull};
> +use core::{marker::PhantomData, ops::Deref, ptr::NonNull};
> 
> /// A type alias for retrieving a [`Driver`]s [`DriverFile`] implementation from its
> /// [`DriverObject`] implementation.
> @@ -22,21 +25,30 @@
> /// [`DriverFile`]: drm::file::DriverFile
> pub type DriverFile<T> = drm::File<<<T as DriverObject>::Driver as drm::Driver>::File>;
> 
> +/// A type alias for retrieving the current [`AllocImpl`] for a given [`DriverObject`].
> +///
> +/// [`Driver`]: drm::Driver
> +pub type DriverAllocImpl<T, Ctx = Registered> =
> +    <<T as DriverObject>::Driver as drm::Driver>::Object<Ctx>;
> +
> /// GEM object functions, which must be implemented by drivers.
> pub trait DriverObject: Sync + Send + Sized {
>     /// Parent `Driver` for this object.
>     type Driver: drm::Driver;
> 
>     /// Create a new driver data object for a GEM object of a given size.
> -    fn new(dev: &drm::Device<Self::Driver>, size: usize) -> impl PinInit<Self, Error>;
> +    fn new<Ctx: DeviceCtx>(
> +        dev: &drm::Device<Self::Driver, Ctx>,
> +        size: usize,
> +    ) -> impl PinInit<Self, Error>;
> 
>     /// Open a new handle to an existing object, associated with a File.
> -    fn open(_obj: &<Self::Driver as drm::Driver>::Object, _file: &DriverFile<Self>) -> Result {
> +    fn open(_obj: &DriverAllocImpl<Self>, _file: &DriverFile<Self>) -> Result {
>         Ok(())
>     }
> 
>     /// Close a handle to an existing object, associated with a File.
> -    fn close(_obj: &<Self::Driver as drm::Driver>::Object, _file: &DriverFile<Self>) {}
> +    fn close(_obj: &DriverAllocImpl<Self>, _file: &DriverFile<Self>) {}
> }
> 
> /// Trait that represents a GEM object subtype
> @@ -62,9 +74,12 @@ extern "C" fn open_callback<T: DriverObject>(
>     // SAFETY: `open_callback` is only ever called with a valid pointer to a `struct drm_file`.
>     let file = unsafe { DriverFile::<T>::from_raw(raw_file) };
> 
> -    // SAFETY: `open_callback` is specified in the AllocOps structure for `DriverObject<T>`,
> -    // ensuring that `raw_obj` is contained within a `DriverObject<T>`
> -    let obj = unsafe { <<T::Driver as drm::Driver>::Object as IntoGEMObject>::from_raw(raw_obj) };
> +    // SAFETY:
> +    // * `open_callback` is specified in the AllocOps structure for `DriverObject`, ensuring that
> +    //   `raw_obj` is contained within a `DriverAllocImpl<T>`
> +    // * It is only possible for `open_callback` to be called after device registration, ensuring
> +    //   that the object's device is in the `Registered` state.
> +    let obj: &DriverAllocImpl<T> = unsafe { IntoGEMObject::from_raw(raw_obj) };
> 
>     match T::open(obj, file) {
>         Err(e) => e.to_errno(),
> @@ -81,12 +96,12 @@ extern "C" fn close_callback<T: DriverObject>(
> 
>     // SAFETY: `close_callback` is specified in the AllocOps structure for `Object<T>`, ensuring
>     // that `raw_obj` is indeed contained within a `Object<T>`.
> -    let obj = unsafe { <<T::Driver as drm::Driver>::Object as IntoGEMObject>::from_raw(raw_obj) };
> +    let obj: &DriverAllocImpl<T> = unsafe { IntoGEMObject::from_raw(raw_obj) };
> 
>     T::close(obj, file);
> }
> 
> -impl<T: DriverObject> IntoGEMObject for Object<T> {
> +impl<T: DriverObject, Ctx: DeviceCtx> IntoGEMObject for Object<T, Ctx> {
>     fn as_raw(&self) -> *mut bindings::drm_gem_object {
>         self.obj.get()
>     }
> @@ -94,7 +109,7 @@ fn as_raw(&self) -> *mut bindings::drm_gem_object {
>     unsafe fn from_raw<'a>(self_ptr: *mut bindings::drm_gem_object) -> &'a Self {
>         // SAFETY: `obj` is guaranteed to be in an `Object<T>` via the safety contract of this
>         // function
> -        unsafe { &*crate::container_of!(Opaque::cast_from(self_ptr), Object<T>, obj) }
> +        unsafe { &*crate::container_of!(Opaque::cast_from(self_ptr), Object<T, Ctx>, obj) }
>     }
> }
> 
> @@ -111,7 +126,7 @@ fn size(&self) -> usize {
>     fn create_handle<D, F>(&self, file: &drm::File<F>) -> Result<u32>
>     where
>         Self: AllocImpl<Driver = D>,
> -        D: drm::Driver<Object = Self, File = F>,
> +        D: drm::Driver<Object<Registered> = Self, File = F>,
>         F: drm::file::DriverFile<Driver = D>,
>     {
>         let mut handle: u32 = 0;
> @@ -126,7 +141,7 @@ fn create_handle<D, F>(&self, file: &drm::File<F>) -> Result<u32>
>     fn lookup_handle<D, F>(file: &drm::File<F>, handle: u32) -> Result<ARef<Self>>
>     where
>         Self: AllocImpl<Driver = D>,
> -        D: drm::Driver<Object = Self, File = F>,
> +        D: drm::Driver<Object<Registered> = Self, File = F>,
>         F: drm::file::DriverFile<Driver = D>,
>     {
>         // SAFETY: The arguments are all valid per the type invariants.
> @@ -166,16 +181,18 @@ impl<T: IntoGEMObject> BaseObject for T {}
> ///
> /// Invariants
> ///
> -/// - `self.obj` is a valid instance of a `struct drm_gem_object`.
> +/// * `self.obj` is a valid instance of a `struct drm_gem_object`.
> +/// * Any type invariants of `Ctx` apply to the parent DRM device for this GEM object.
> #[repr(C)]
> #[pin_data]
> -pub struct Object<T: DriverObject + Send + Sync> {
> +pub struct Object<T: DriverObject + Send + Sync, Ctx: DeviceCtx = Registered> {
>     obj: Opaque<bindings::drm_gem_object>,
>     #[pin]
>     data: T,
> +    _ctx: PhantomData<Ctx>,
> }
> 
> -impl<T: DriverObject> Object<T> {
> +impl<T: DriverObject, Ctx: DeviceCtx> Object<T, Ctx> {
>     const OBJECT_FUNCS: bindings::drm_gem_object_funcs = bindings::drm_gem_object_funcs {
>         free: Some(Self::free_callback),
>         open: Some(open_callback::<T>),
> @@ -195,11 +212,12 @@ impl<T: DriverObject> Object<T> {
>     };
> 
>     /// Create a new GEM object.
> -    pub fn new(dev: &drm::Device<T::Driver>, size: usize) -> Result<ARef<Self>> {
> +    pub fn new(dev: &drm::Device<T::Driver, Ctx>, size: usize) -> Result<ARef<Self>> {
>         let obj: Pin<KBox<Self>> = KBox::pin_init(
>             try_pin_init!(Self {
>                 obj: Opaque::new(bindings::drm_gem_object::default()),
>                 data <- T::new(dev, size),
> +                _ctx: PhantomData,
>             }),
>             GFP_KERNEL,
>         )?;
> @@ -208,6 +226,8 @@ pub fn new(dev: &drm::Device<T::Driver>, size: usize) -> Result<ARef<Self>> {
>         unsafe { (*obj.as_raw()).funcs = &Self::OBJECT_FUNCS };
> 
>         // SAFETY: The arguments are all valid per the type invariants.
> +        // INVARIANT: We use `dev` for creating the GEM object, which is known to be in state `Ctx` -
> +        // ensuring that the GEM object's pointer to the DRM device is always in the same state.
>         to_result(unsafe { bindings::drm_gem_object_init(dev.as_raw(), obj.obj.get(), size) })?;
> 
>         // SAFETY: We never move out of `Self`.
> @@ -221,13 +241,15 @@ pub fn new(dev: &drm::Device<T::Driver>, size: usize) -> Result<ARef<Self>> {
>     }
> 
>     /// Returns the `Device` that owns this GEM object.
> -    pub fn dev(&self) -> &drm::Device<T::Driver> {
> +    pub fn dev(&self) -> &drm::Device<T::Driver, Ctx> {
>         // SAFETY:
>         // - `struct drm_gem_object.dev` is initialized and valid for as long as the GEM
>         //   object lives.
>         // - The device we used for creating the gem object is passed as &drm::Device<T::Driver> to
>         //   Object::<T>::new(), so we know that `T::Driver` is the right generic parameter to use
>         //   here.
> +        // - Any type invariants of `Ctx` are upheld by using the same `Ctx` for the `Device` we
> +        //   return.
>         unsafe { drm::Device::from_raw((*self.as_raw()).dev) }
>     }
> 
> @@ -253,7 +275,7 @@ extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) {
> }
> 
> // SAFETY: Instances of `Object<T>` are always reference-counted.
> -unsafe impl<T: DriverObject> crate::types::AlwaysRefCounted for Object<T> {
> +unsafe impl<T: DriverObject, Ctx: DeviceCtx> crate::types::AlwaysRefCounted for Object<T, Ctx> {
>     fn inc_ref(&self) {
>         // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
>         unsafe { bindings::drm_gem_object_get(self.as_raw()) };
> @@ -268,9 +290,9 @@ unsafe fn dec_ref(obj: NonNull<Self>) {
>     }
> }
> 
> -impl<T: DriverObject> super::private::Sealed for Object<T> {}
> +impl<T: DriverObject, Ctx: DeviceCtx> super::private::Sealed for Object<T, Ctx> {}
> 
> -impl<T: DriverObject> Deref for Object<T> {
> +impl<T: DriverObject, Ctx: DeviceCtx> Deref for Object<T, Ctx> {
>     type Target = T;
> 
>     fn deref(&self) -> &Self::Target {
> @@ -278,7 +300,7 @@ fn deref(&self) -> &Self::Target {
>     }
> }
> 
> -impl<T: DriverObject> AllocImpl for Object<T> {
> +impl<T: DriverObject, Ctx: DeviceCtx> AllocImpl for Object<T, Ctx> {
>     type Driver = T::Driver;
> 
>     const ALLOC_OPS: AllocOps = AllocOps {
> -- 
> 2.51.1
> 
> 

Reviewed-by: Daniel Almeida <daniel.almeida@...labora.com>

(See comments on patch 1 before applying this) 

Powered by blists - more mailing lists