Message-Id: <DFUG2U4PVVTN.1DVPXGJK735F8@garyguo.net>
Date: Wed, 21 Jan 2026 17:31:25 +0000
From: "Gary Guo" <gary@...yguo.net>
To: "Daniel Almeida" <daniel.almeida@...labora.com>, "Danilo Krummrich"
 <dakr@...nel.org>, "Alice Ryhl" <aliceryhl@...gle.com>, "David Airlie"
 <airlied@...il.com>, "Simona Vetter" <simona@...ll.ch>, "Miguel Ojeda"
 <ojeda@...nel.org>, "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>,
 "Boris Brezillon" <boris.brezillon@...labora.com>
Cc: <nouveau@...ts.freedesktop.org>, <dri-devel@...ts.freedesktop.org>,
 <linux-kernel@...r.kernel.org>, <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH 1/2] rust: drm: add support for driver features

On Mon Jan 19, 2026 at 11:34 PM GMT, Daniel Almeida wrote:
> Add initial support for drm driver features via the DriverFeatures trait.
> This trait is unsafe, requiring the implementer to comply with the safety
> requirements of each feature individually if the feature is enabled.

I think such unsafe requirement is quite vague and also very non-local.

Maybe we can use a single trait (the `ModesetOps` that you described) to do
this:

Something like:

    pub unsafe trait ModesetOps<D> { ... }

    // Maybe the never type in the future...
    pub enum NoFeature {}

    impl<D> ModesetOps<D> for NoFeature {
        fn foo(&self, ...) { unimplemented!() }
    }

    impl Driver {
        /// Reference the modeset implementation (typically Self),
        /// or `NoFeature` to indicate that the feature is not implemented.
        type Modeset: ModesetOps<Self>;
    }

When building, you can use `TypeId` to check if it's actually implemented, and
set bits in the feature flags automatically.

Best,
Gary

>
> New features can be described by adding separate ZSTs to encode them. The
> current design assumes two types, for example: FeatureFooSupported and
> NoFoo. As said above, this will require implementors to observe more safety
> requirements in their DriverFeatures trait implementation.
>
> A subsequent commit will build on this one to add support for FEAT_RENDER.
> This is required by Tyr and other drivers.
>
> Additionally, features can also require additional traits to be implemented
> when enabled. These traits can add their own safety requirements.
>
> This is roughly described below, with some boilerplate omitted:
>
> pub struct ModesetSupported;
> pub struct NoModeset;
>
> pub unsafe trait ModesetOps: Driver {
>     fn set_mode(&self, ...);
> }
>
> pub trait ModesetRequirement<F: ModesetFeature> {}
> impl<T: ModesetOps> ModesetRequirement<ModesetSupported> for T {}
> impl<T> ModesetRequirement<NoModeset> for T {}
>
> pub trait Driver:
>     DriverFeatures
>     + ModesetRequirement<Self::Modeset>
> {
>     // ...
> }
>
> // `driver::compute_features` is augmented to include the feature flag.
> const fn compute_features() -> u32 {
>   if T::Modeset::ENABLED {
>     features |= FEAT_MODESET;
>   }
>
>   features
> }
>
> // In driver code, `DriverFeatures` can enable the feature via
> // `ModesetSupported`.
> unsafe impl DriverFeatures for MyDriver {
>   type Modeset = ModesetSupported;
> }
>
> // Required because `Modeset = ModesetSupported`.
> unsafe impl ModesetOps for MyDriver {...}
>
> Feature support will soon be required by upcoming DRM drivers. This
> extensible model lets us describe them in terms of either additional safety
> requirements and/or traits.
>
> Signed-off-by: Daniel Almeida <daniel.almeida@...labora.com>
> ---
>  drivers/gpu/drm/nova/driver.rs |  5 +++++
>  drivers/gpu/drm/tyr/driver.rs  |  5 +++++
>  rust/kernel/drm/device.rs      |  6 +++++-
>  rust/kernel/drm/driver.rs      | 17 ++++++++++++++++-
>  4 files changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/nova/driver.rs b/drivers/gpu/drm/nova/driver.rs
> index 2246d8e104e0..247a05f7b3a7 100644
> --- a/drivers/gpu/drm/nova/driver.rs
> +++ b/drivers/gpu/drm/nova/driver.rs
> @@ -69,3 +69,8 @@ impl drm::Driver for NovaDriver {
>          (NOVA_GEM_INFO, drm_nova_gem_info, ioctl::AUTH | ioctl::RENDER_ALLOW, File::gem_info),
>      }
>  }
> +
> +// SAFETY: This trait requires implementers to observe the safety requirements
> +// of each enabled feature. There are no features enabled, so this is safe by
> +// definition.
> +unsafe impl drm::driver::DriverFeatures for NovaDriver {}
> diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
> index 0389c558c036..ec2aa30515a1 100644
> --- a/drivers/gpu/drm/tyr/driver.rs
> +++ b/drivers/gpu/drm/tyr/driver.rs
> @@ -191,6 +191,11 @@ impl drm::Driver for TyrDriver {
>      }
>  }
>  
> +// SAFETY: This trait requires implementers to observe the safety requirements
> +// of each enabled feature. There are no features enabled, so this is safe by
> +// definition.
> +unsafe impl drm::driver::DriverFeatures for TyrDriver {}
> +
>  #[pin_data]
>  struct Clocks {
>      core: Clk,
> diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
> index 3ce8f62a0056..cfc2f34e8cc2 100644
> --- a/rust/kernel/drm/device.rs
> +++ b/rust/kernel/drm/device.rs
> @@ -61,6 +61,10 @@ pub struct Device<T: drm::Driver> {
>  }
>  
>  impl<T: drm::Driver> Device<T> {
> +    const fn compute_features() -> u32 {
> +        drm::driver::FEAT_GEM
> +    }
> +
>      const VTABLE: bindings::drm_driver = drm_legacy_fields! {
>          load: None,
>          open: Some(drm::File::<T::File>::open_callback),
> @@ -86,7 +90,7 @@ impl<T: drm::Driver> Device<T> {
>          name: crate::str::as_char_ptr_in_const_context(T::INFO.name).cast_mut(),
>          desc: crate::str::as_char_ptr_in_const_context(T::INFO.desc).cast_mut(),
>  
> -        driver_features: drm::driver::FEAT_GEM,
> +        driver_features: Self::compute_features(),
>          ioctls: T::IOCTLS.as_ptr(),
>          num_ioctls: T::IOCTLS.len() as i32,
>          fops: &Self::GEM_FOPS,
> diff --git a/rust/kernel/drm/driver.rs b/rust/kernel/drm/driver.rs
> index f30ee4c6245c..fdfd083ba2b6 100644
> --- a/rust/kernel/drm/driver.rs
> +++ b/rust/kernel/drm/driver.rs
> @@ -98,7 +98,7 @@ pub trait AllocImpl: super::private::Sealed + drm::gem::IntoGEMObject {
>  /// This trait must be implemented by drivers in order to create a `struct drm_device` and `struct
>  /// drm_driver` to be registered in the DRM subsystem.
>  #[vtable]
> -pub trait Driver {
> +pub trait Driver: DriverFeatures {
>      /// Context data associated with the DRM driver
>      type Data: Sync + Send;
>  
> @@ -168,3 +168,18 @@ fn drop(&mut self) {
>          unsafe { bindings::drm_dev_unregister(self.0.as_raw()) };
>      }
>  }
> +
> +/// Marker trait for drivers supporting specific features.
> +///
> +/// This trait is unsafe, and each feature might add its own safety
> +/// requirements. The safety requirements for this trait requires the caller to
> +/// comply with the safety requirements of each supported feature.
> +///
> +/// Features might also require additional trait implementations to be present.
> +/// These additional traits may also be unsafe.
> +///
> +/// # Safety
> +///
> +/// Drivers implementing this trait must ensure they comply with the safety
> +/// requirements of each supported feature.
> +pub unsafe trait DriverFeatures {}

Powered by blists - more mailing lists