Message-ID: <20250314-unselfish-mauve-anaconda-2991af@houat>
Date: Fri, 14 Mar 2025 11:05:52 +0100
From: Maxime Ripard <mripard@...nel.org>
To: Lyude Paul <lyude@...hat.com>
Cc: dri-devel@...ts.freedesktop.org, rust-for-linux@...r.kernel.org, 
	Danilo Krummrich <dakr@...nel.org>, mcanal@...lia.com, Alice Ryhl <aliceryhl@...gle.com>, 
	Simona Vetter <sima@...ll.ch>, Daniel Almeida <daniel.almeida@...labora.com>, 
	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 <benno.lossin@...ton.me>, 
	Andreas Hindborg <a.hindborg@...nel.org>, Trevor Gross <tmgross@...ch.edu>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Asahi Lina <lina@...hilina.net>, 
	Wedson Almeida Filho <wedsonaf@...il.com>, open list <linux-kernel@...r.kernel.org>
Subject: Re: [RFC v3 02/33] rust: drm: Add traits for registering KMS devices

Hi Lyude,

First off, thanks for keeping up with this series.

I'm quite familiar with Rust in userspace, but not so much in the
kernel, so I might have stupid questions or points, sorry I advance :)

On Wed, Mar 05, 2025 at 05:59:18PM -0500, Lyude Paul wrote:
> This commit adds some traits for registering DRM devices with KMS support,
> implemented through the kernel::drm::kms::KmsDriver trait. Devices which
> don't have KMS support can simply use PhantomData<Self>.
> 
> Signed-off-by: Lyude Paul <lyude@...hat.com>
> 
> ---
> 
> V3:
> * Get rid of Kms, long live KmsDriver
>   After Daniel pointed out that we should just make KmsDriver a supertrait
>   of Driver, it immediately occurred to me that there's no actual need for
>   Kms to be a separate trait at all. So, drop Kms entirely and move its
>   requirements over to KmsDriver.
> * Drop fbdev module entirely and move fbdev related setup into AllocImpl
>   (Daniel)
> * Rebase to use drm_client_setup()
> 
> TODO:
> * Generate feature flags automatically, these shouldn't need to be
>   specified by the user
> 
> Signed-off-by: Lyude Paul <lyude@...hat.com>
> ---
>  rust/bindings/bindings_helper.h |   6 ++
>  rust/kernel/drm/device.rs       |  10 +-
>  rust/kernel/drm/drv.rs          |  56 ++++++++--
>  rust/kernel/drm/gem/mod.rs      |   4 +
>  rust/kernel/drm/gem/shmem.rs    |   4 +
>  rust/kernel/drm/kms.rs          | 186 ++++++++++++++++++++++++++++++++
>  rust/kernel/drm/mod.rs          |   1 +
>  7 files changed, 258 insertions(+), 9 deletions(-)
>  create mode 100644 rust/kernel/drm/kms.rs
> 
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index ca857fb00b1a5..e1ed4f40c8e89 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -6,10 +6,16 @@
>   * Sorted alphabetically.
>   */
>  
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/clients/drm_client_setup.h>
>  #include <drm/drm_device.h>
>  #include <drm/drm_drv.h>
>  #include <drm/drm_file.h>
> +#include <drm/drm_fbdev_dma.h>
> +#include <drm/drm_fbdev_shmem.h>
>  #include <drm/drm_gem.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
>  #include <drm/drm_gem_shmem_helper.h>
>  #include <drm/drm_ioctl.h>
>  #include <kunit/test.h>
> diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
> index 5b4db2dfe87f5..cf063de387329 100644
> --- a/rust/kernel/drm/device.rs
> +++ b/rust/kernel/drm/device.rs
> @@ -5,8 +5,8 @@
>  //! C header: [`include/linux/drm/drm_device.h`](srctree/include/linux/drm/drm_device.h)
>  
>  use crate::{
> -    bindings, device, drm,
> -    drm::drv::AllocImpl,
> +    bindings, device,
> +    drm::{self, drv::AllocImpl, kms::private::KmsImpl as KmsImplPrivate},
>      error::code::*,
>      error::from_err_ptr,
>      error::Result,
> @@ -73,7 +73,7 @@ impl<T: drm::drv::Driver> Device<T> {
>          dumb_create: T::Object::ALLOC_OPS.dumb_create,
>          dumb_map_offset: T::Object::ALLOC_OPS.dumb_map_offset,
>          show_fdinfo: None,
> -        fbdev_probe: None,
> +        fbdev_probe: T::Object::ALLOC_OPS.fbdev_probe,
>  
>          major: T::INFO.major,
>          minor: T::INFO.minor,
> @@ -153,6 +153,10 @@ pub fn data(&self) -> <T::Data as ForeignOwnable>::Borrowed<'_> {
>          // SAFETY: `Self::data` is always converted and set on device creation.
>          unsafe { <T::Data as ForeignOwnable>::from_foreign(drm.raw_data()) };
>      }
> +
> +    pub(crate) const fn has_kms() -> bool {
> +        <T::Kms as KmsImplPrivate>::MODE_CONFIG_OPS.is_some()
> +    }
>  }
>  
>  // SAFETY: DRM device objects are always reference counted and the get/put functions
> diff --git a/rust/kernel/drm/drv.rs b/rust/kernel/drm/drv.rs
> index e42e266bdd0da..3e09e130730f6 100644
> --- a/rust/kernel/drm/drv.rs
> +++ b/rust/kernel/drm/drv.rs
> @@ -6,14 +6,15 @@
>  
>  use crate::{
>      alloc::flags::*,
> -    bindings,
> +    bindings, device,
>      devres::Devres,
> -    drm,
> +    drm::{self, kms::private::KmsImpl as KmsImplPrivate},
>      error::{Error, Result},
>      private::Sealed,
>      str::CStr,
>      types::{ARef, ForeignOwnable},
>  };
> +use core::ptr::null;
>  use macros::vtable;
>  
>  /// Driver use the GEM memory manager. This should be set for all modern drivers.
> @@ -115,6 +116,12 @@ pub struct AllocOps {
>              offset: *mut u64,
>          ) -> core::ffi::c_int,
>      >,
> +    pub(crate) fbdev_probe: Option<
> +        unsafe extern "C" fn(
> +            fbdev_helper: *mut bindings::drm_fb_helper,
> +            sizes: *mut bindings::drm_fb_helper_surface_size,
> +        ) -> core::ffi::c_int,
> +    >,
>  }
>  
>  /// Trait for memory manager implementations. Implemented internally.
> @@ -142,6 +149,14 @@ pub trait Driver {
>      /// The type used to represent a DRM File (client)
>      type File: drm::file::DriverFile;
>  
> +    /// The KMS implementation for this driver.
> +    ///
> +    /// Drivers that wish to support KMS should pass their implementation of `drm::kms::KmsDriver`
> +    /// here. Drivers which do not have KMS support can simply pass `drm::kms::NoKms` here.
> +    type Kms: drm::kms::KmsImpl<Driver = Self>
> +    where
> +        Self: Sized;
> +
>      /// Driver metadata
>      const INFO: DriverInfo;
>  
> @@ -159,21 +174,44 @@ pub trait Driver {
>  
>  impl<T: Driver> Registration<T> {
>      /// Creates a new [`Registration`] and registers it.
> -    pub fn new(drm: ARef<drm::device::Device<T>>, flags: usize) -> Result<Self> {
> +    pub fn new(dev: &device::Device, data: T::Data, flags: usize) -> Result<Self> {
> +        let drm = drm::device::Device::<T>::new(dev, data)?;
> +        let has_kms = drm::device::Device::<T>::has_kms();
> +
> +        let mode_config_info = if has_kms {
> +            // SAFETY: We have yet to register this device
> +            Some(unsafe { T::Kms::setup_kms(&drm)? })
> +        } else {
> +            None
> +        };
> +
>          // SAFETY: Safe by the invariants of `drm::device::Device`.
>          let ret = unsafe { bindings::drm_dev_register(drm.as_raw(), flags) };
>          if ret < 0 {
>              return Err(Error::from_errno(ret));
>          }
>  
> +        #[cfg(CONFIG_DRM_CLIENT = "y")]
> +        if has_kms {
> +            if let Some(ref info) = mode_config_info {
> +                if let Some(fourcc) = info.preferred_fourcc {
> +                    // SAFETY: We just registered `drm` above, fulfilling the C API requirements
> +                    unsafe { bindings::drm_client_setup_with_fourcc(drm.as_raw(), fourcc) }
> +                } else {
> +                    // SAFETY: We just registered `drm` above, fulfilling the C API requirements
> +                    unsafe { bindings::drm_client_setup(drm.as_raw(), null()) }
> +                }
> +            }
> +        }
> +
>          Ok(Self(drm))
>      }
>  
>      /// Same as [`Registration::new`}, but transfers ownership of the [`Registration`] to `Devres`.
> -    pub fn new_foreign_owned(drm: ARef<drm::device::Device<T>>, flags: usize) -> Result {
> -        let reg = Registration::<T>::new(drm.clone(), flags)?;
> +    pub fn new_foreign_owned(dev: &device::Device, data: T::Data, flags: usize) -> Result {
> +        let reg = Registration::<T>::new(dev, data, flags)?;
>  
> -        Devres::new_foreign_owned(drm.as_ref(), reg, GFP_KERNEL)
> +        Devres::new_foreign_owned(dev, reg, GFP_KERNEL)

I appreciate that it's a quite large series, but I think this patch (and
others, from a quick glance) could be broken down some more. For
example, the introduction of the new data parameter to
Registration::new() is a prerequisite but otherwise pretty orthogonal to
the patch subject.

>      }
>  
>      /// Returns a reference to the `Device` instance for this registration.
> @@ -195,5 +233,11 @@ fn drop(&mut self) {
>          // SAFETY: Safe by the invariant of `ARef<drm::device::Device<T>>`. The existance of this
>          // `Registration` also guarantees the this `drm::device::Device` is actually registered.
>          unsafe { bindings::drm_dev_unregister(self.0.as_raw()) };
> +
> +        if drm::device::Device::<T>::has_kms() {
> +            // SAFETY: We just checked above that KMS was setup for this device, so this is safe to
> +            // call
> +            unsafe { bindings::drm_atomic_helper_shutdown(self.0.as_raw()) }
> +        }

And similarly, calling drm_atomic_helper_shutdown() (even though it's
probably a good idea imo), should be a follow-up. I guess it's more of a
policy thing but drivers have different opinions about it and I guess we
should discuss that topic in isolation.

Breaking down the patches into smaller chunks will also make it easier
to review, and I'd really appreciate it :)

>      }
>  }
> diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs
> index 3fcab497cc2a5..605b0a22ac08b 100644
> --- a/rust/kernel/drm/gem/mod.rs
> +++ b/rust/kernel/drm/gem/mod.rs
> @@ -300,6 +300,10 @@ impl<T: DriverObject> drv::AllocImpl for Object<T> {
>          gem_prime_import_sg_table: None,
>          dumb_create: None,
>          dumb_map_offset: None,
> +        #[cfg(CONFIG_DRM_FBDEV_EMULATION = "y")]
> +        fbdev_probe: Some(bindings::drm_fbdev_dma_driver_fbdev_probe),
> +        #[cfg(CONFIG_DRM_FBDEV_EMULATION = "n")]
> +        fbdev_probe: None,
>      };
>  }
>  
> diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
> index 92da0d7d59912..9c0162b268aa8 100644
> --- a/rust/kernel/drm/gem/shmem.rs
> +++ b/rust/kernel/drm/gem/shmem.rs
> @@ -279,6 +279,10 @@ impl<T: DriverObject> drv::AllocImpl for Object<T> {
>          gem_prime_import_sg_table: Some(bindings::drm_gem_shmem_prime_import_sg_table),
>          dumb_create: Some(bindings::drm_gem_shmem_dumb_create),
>          dumb_map_offset: None,
> +        #[cfg(CONFIG_DRM_FBDEV_EMULATION = "y")]
> +        fbdev_probe: Some(bindings::drm_fbdev_shmem_driver_fbdev_probe),
> +        #[cfg(CONFIG_DRM_FBDEV_EMULATION = "n")]
> +        fbdev_probe: None,
>      };
>  }
>  
> diff --git a/rust/kernel/drm/kms.rs b/rust/kernel/drm/kms.rs
> new file mode 100644
> index 0000000000000..78970c69f4cda
> --- /dev/null
> +++ b/rust/kernel/drm/kms.rs
> @@ -0,0 +1,186 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +
> +//! KMS driver abstractions for rust.
> +
> +use crate::{
> +    device,
> +    drm::{device::Device, drv::Driver},
> +    error::to_result,
> +    prelude::*,
> +    types::*,
> +};
> +use bindings;
> +use core::{marker::PhantomData, ops::Deref};
> +
> +/// The C vtable for a [`Device`].
> +///
> +/// This is created internally by DRM.
> +pub struct ModeConfigOps {
> +    pub(crate) kms_vtable: bindings::drm_mode_config_funcs,
> +    pub(crate) kms_helper_vtable: bindings::drm_mode_config_helper_funcs,
> +}
> +
> +/// A trait representing a type that can be used for setting up KMS, or a stub.
> +///
> +/// For drivers which don't have KMS support, the methods provided by this trait may be stubs. It is
> +/// implemented internally by DRM.
> +pub trait KmsImpl: private::KmsImpl {}
> +
> +pub(crate) mod private {
> +    use super::*;
> +
> +    /// Private callback implemented internally by DRM for setting up KMS on a device, or stubbing
> +    /// the KMS setup for devices which don't have KMS support.
> +    #[allow(unreachable_pub)]
> +    pub trait KmsImpl {
> +        /// The parent driver for this KMS implementation
> +        type Driver: Driver;
> +
> +        /// The optional KMS callback operations for this driver.
> +        const MODE_CONFIG_OPS: Option<ModeConfigOps>;
> +
> +        /// The callback for setting up KMS on a device
> +        ///
> +        /// # Safety
> +        ///
> +        /// `drm` must be unregistered.
> +        unsafe fn setup_kms(_drm: &Device<Self::Driver>) -> Result<ModeConfigInfo> {
> +            build_error::build_error("This should never be reachable")
> +        }
> +    }
> +}
> +
> +/// A [`Device`] with KMS initialized that has not been registered with userspace.
> +///
> +/// This type is identical to [`Device`], except that it is able to create new static KMS resources.
> +/// It represents a KMS device that is not yet visible to userspace, and also contains miscellaneous
> +/// state required during the initialization process of a [`Device`].
> +pub struct UnregisteredKmsDevice<'a, T: Driver> {
> +    drm: &'a Device<T>,
> +}
> +
> +impl<'a, T: Driver> Deref for UnregisteredKmsDevice<'a, T> {
> +    type Target = Device<T>;
> +
> +    fn deref(&self) -> &Self::Target {
> +        self.drm
> +    }
> +}
> +
> +impl<'a, T: Driver> UnregisteredKmsDevice<'a, T> {
> +    /// Construct a new [`UnregisteredKmsDevice`].
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller promises that `drm` is an unregistered [`Device`].
> +    pub(crate) unsafe fn new(drm: &'a Device<T>) -> Self {
> +        Self { drm }
> +    }
> +}

I guess it's more of a question here than a review, but what's the
advantage of that pattern over Into<UnregisteredKmsDevice> for Device<T> ?

> +/// A trait which must be implemented by drivers that wish to support KMS
> +///
> +/// It should be implemented for the same type that implements [`Driver`]. Drivers which don't
> +/// support KMS should use [`PhantomData<Self>`].
> +///
> +/// [`PhantomData<Self>`]: PhantomData
> +#[vtable]
> +pub trait KmsDriver: Driver {
> +    /// Return a [`ModeConfigInfo`] structure for this [`device::Device`].
> +    fn mode_config_info(
> +        dev: &device::Device,
> +        drm_data: <Self::Data as ForeignOwnable>::Borrowed<'_>,
> +    ) -> Result<ModeConfigInfo>;
> +
> +    /// Create mode objects like [`crtc::Crtc`], [`plane::Plane`], etc. for this device
> +    fn create_objects(drm: &UnregisteredKmsDevice<'_, Self>) -> Result
> +    where
> +        Self: Sized;
> +}
> +
> +impl<T: KmsDriver> private::KmsImpl for T {
> +    type Driver = Self;
> +
> +    const MODE_CONFIG_OPS: Option<ModeConfigOps> = Some(ModeConfigOps {
> +        kms_vtable: bindings::drm_mode_config_funcs {
> +            atomic_check: Some(bindings::drm_atomic_helper_check),
> +            fb_create: Some(bindings::drm_gem_fb_create),
> +            mode_valid: None,
> +            atomic_commit: Some(bindings::drm_atomic_helper_commit),
> +            get_format_info: None,
> +            atomic_state_free: None,
> +            atomic_state_alloc: None,
> +            atomic_state_clear: None,
> +        },
> +
> +        kms_helper_vtable: bindings::drm_mode_config_helper_funcs {
> +            atomic_commit_setup: None,
> +            atomic_commit_tail: None,
> +        },
> +    });

I think here we venture into what we want from the bindings exactly. If
we want to model the API truthfully, then the
drm_mode_config_helper_funcs should be optional. We could also take a
stand and say that any modern driver should use the helpers anyway, and
thus it's mandatory.

Both are fine imo, but we should make it clearer what we want our
bindings to be: the same API, or a better one.

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (274 bytes)

Powered by blists - more mailing lists