| Message ID | 20250325235522.3992-4-dakr@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | DRM Rust abstractions | expand |
On Wed, Mar 26, 2025 at 12:54:30AM +0100, Danilo Krummrich wrote: > +/// Information data for a DRM Driver. > +pub struct DriverInfo { > + /// Driver major version. > + pub major: i32, > + /// Driver minor version. > + pub minor: i32, > + /// Driver patchlevel version. > + pub patchlevel: i32, > + /// Driver name. > + pub name: &'static CStr, > + /// Driver description. > + pub desc: &'static CStr, > + /// Driver date. > + pub date: &'static CStr, > +} The date was removed in 6.14 Once fixed, Reviewed-by: Maxime Ripard <mripard@kernel.org> Maxime
On Wed, 2025-03-26 at 00:54 +0100, Danilo Krummrich wrote: > diff --git a/rust/kernel/drm/driver.rs b/rust/kernel/drm/driver.rs > new file mode 100644 > index 000000000000..1ac770482ae0 > --- /dev/null > +++ b/rust/kernel/drm/driver.rs > @@ -0,0 +1,143 @@ > +// SPDX-License-Identifier: GPL-2.0 OR MIT > + > +//! DRM driver core. > +//! > +//! C header: [`include/linux/drm/drm_drv.h`](srctree/include/linux/drm/drm_drv.h) > + > +use crate::{bindings, drm, str::CStr}; > +use macros::vtable; > + > +/// Driver use the GEM memory manager. This should be set for all modern drivers. > +pub const FEAT_GEM: u32 = bindings::drm_driver_feature_DRIVER_GEM; > +/// Driver supports mode setting interfaces (KMS). > +pub const FEAT_MODESET: u32 = bindings::drm_driver_feature_DRIVER_MODESET; > +/// Driver supports dedicated render nodes. > +pub const FEAT_RENDER: u32 = bindings::drm_driver_feature_DRIVER_RENDER; > +/// Driver supports the full atomic modesetting userspace API. > +/// > +/// Drivers which only use atomic internally, but do not support the full userspace API (e.g. not > +/// all properties converted to atomic, or multi-plane updates are not guaranteed to be tear-free) > +/// should not set this flag. > +pub const FEAT_ATOMIC: u32 = bindings::drm_driver_feature_DRIVER_ATOMIC; > +/// Driver supports DRM sync objects for explicit synchronization of command submission. > +pub const FEAT_SYNCOBJ: u32 = bindings::drm_driver_feature_DRIVER_SYNCOBJ; > +/// Driver supports the timeline flavor of DRM sync objects for explicit synchronization of command > +/// submission. > +pub const FEAT_SYNCOBJ_TIMELINE: u32 = bindings::drm_driver_feature_DRIVER_SYNCOBJ_TIMELINE; > +/// Driver supports compute acceleration devices. This flag is mutually exclusive with `FEAT_RENDER` > +/// and `FEAT_MODESET`. Devices that support both graphics and compute acceleration should be > +/// handled by two drivers that are connected using auxiliary bus. > +pub const FEAT_COMPUTE_ACCEL: u32 = bindings::drm_driver_feature_DRIVER_COMPUTE_ACCEL; > +/// Driver supports user defined GPU VA bindings for GEM objects. > +pub const FEAT_GEM_GPUVA: u32 = bindings::drm_driver_feature_DRIVER_GEM_GPUVA; > +/// Driver supports and requires cursor hotspot information in the cursor plane (e.g. cursor plane > +/// has to actually track the mouse cursor and the clients are required to set hotspot in order for > +/// the cursor planes to work correctly). > +pub const FEAT_CURSOR_HOTSPOT: u32 = bindings::drm_driver_feature_DRIVER_CURSOR_HOTSPOT; IMHO I don't think that we should be exposing any of these constants, building the feature flag mask should honestly be abstracted away into the rust bindings as not doing so means it's very easy to break assumptions that could lead to unsoundness. As an example using the KMS bindings, assume we're handling passing the flags manually. If I were to have a type implement drm::drv::Driver and drm::kms::KmsDriver, we now have access to various Kms method calls on the drm::device::Device. But all of those method calls assume that we actually set up Kms in the first place, since checking this at runtime would lead to most of the Kms API being fallible in places that don't make sense. So if we tried calling a Kms specific method on a device that didn't set FEAT_MODESET | FEAT_ATOMIC, we'd hit undefined behavior.
On Fri, Mar 28, 2025 at 06:00:11PM -0400, Lyude Paul wrote: > On Wed, 2025-03-26 at 00:54 +0100, Danilo Krummrich wrote: > > diff --git a/rust/kernel/drm/driver.rs b/rust/kernel/drm/driver.rs > > new file mode 100644 > > index 000000000000..1ac770482ae0 > > --- /dev/null > > +++ b/rust/kernel/drm/driver.rs > > @@ -0,0 +1,143 @@ > > +// SPDX-License-Identifier: GPL-2.0 OR MIT > > + > > +//! DRM driver core. > > +//! > > +//! C header: [`include/linux/drm/drm_drv.h`](srctree/include/linux/drm/drm_drv.h) > > + > > +use crate::{bindings, drm, str::CStr}; > > +use macros::vtable; > > + > > +/// Driver use the GEM memory manager. This should be set for all modern drivers. > > +pub const FEAT_GEM: u32 = bindings::drm_driver_feature_DRIVER_GEM; > > +/// Driver supports mode setting interfaces (KMS). > > +pub const FEAT_MODESET: u32 = bindings::drm_driver_feature_DRIVER_MODESET; > > +/// Driver supports dedicated render nodes. > > +pub const FEAT_RENDER: u32 = bindings::drm_driver_feature_DRIVER_RENDER; > > +/// Driver supports the full atomic modesetting userspace API. > > +/// > > +/// Drivers which only use atomic internally, but do not support the full userspace API (e.g. not > > +/// all properties converted to atomic, or multi-plane updates are not guaranteed to be tear-free) > > +/// should not set this flag. > > +pub const FEAT_ATOMIC: u32 = bindings::drm_driver_feature_DRIVER_ATOMIC; > > +/// Driver supports DRM sync objects for explicit synchronization of command submission. > > +pub const FEAT_SYNCOBJ: u32 = bindings::drm_driver_feature_DRIVER_SYNCOBJ; > > +/// Driver supports the timeline flavor of DRM sync objects for explicit synchronization of command > > +/// submission. > > +pub const FEAT_SYNCOBJ_TIMELINE: u32 = bindings::drm_driver_feature_DRIVER_SYNCOBJ_TIMELINE; > > +/// Driver supports compute acceleration devices. This flag is mutually exclusive with `FEAT_RENDER` > > +/// and `FEAT_MODESET`. Devices that support both graphics and compute acceleration should be > > +/// handled by two drivers that are connected using auxiliary bus. > > +pub const FEAT_COMPUTE_ACCEL: u32 = bindings::drm_driver_feature_DRIVER_COMPUTE_ACCEL; > > +/// Driver supports user defined GPU VA bindings for GEM objects. > > +pub const FEAT_GEM_GPUVA: u32 = bindings::drm_driver_feature_DRIVER_GEM_GPUVA; > > +/// Driver supports and requires cursor hotspot information in the cursor plane (e.g. cursor plane > > +/// has to actually track the mouse cursor and the clients are required to set hotspot in order for > > +/// the cursor planes to work correctly). > > +pub const FEAT_CURSOR_HOTSPOT: u32 = bindings::drm_driver_feature_DRIVER_CURSOR_HOTSPOT; > > IMHO I don't think that we should be exposing any of these constants, building > the feature flag mask should honestly be abstracted away into the rust > bindings as not doing so means it's very easy to break assumptions that could > lead to unsoundness. Yep, that was the plan, just forgot to make them crate private; on the other hand I should probably drop them entirely for now. - Danilo
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h index b25ea9fe1663..a6be6dc80249 100644 --- a/rust/bindings/bindings_helper.h +++ b/rust/bindings/bindings_helper.h @@ -6,6 +6,7 @@ * Sorted alphabetically. */ +#include <drm/drm_drv.h> #include <drm/drm_ioctl.h> #include <kunit/test.h> #include <linux/auxiliary_bus.h> diff --git a/rust/kernel/drm/driver.rs b/rust/kernel/drm/driver.rs new file mode 100644 index 000000000000..1ac770482ae0 --- /dev/null +++ b/rust/kernel/drm/driver.rs @@ -0,0 +1,143 @@ +// SPDX-License-Identifier: GPL-2.0 OR MIT + +//! DRM driver core. +//! +//! C header: [`include/linux/drm/drm_drv.h`](srctree/include/linux/drm/drm_drv.h) + +use crate::{bindings, drm, str::CStr}; +use macros::vtable; + +/// Driver use the GEM memory manager. This should be set for all modern drivers. +pub const FEAT_GEM: u32 = bindings::drm_driver_feature_DRIVER_GEM; +/// Driver supports mode setting interfaces (KMS). +pub const FEAT_MODESET: u32 = bindings::drm_driver_feature_DRIVER_MODESET; +/// Driver supports dedicated render nodes. +pub const FEAT_RENDER: u32 = bindings::drm_driver_feature_DRIVER_RENDER; +/// Driver supports the full atomic modesetting userspace API. +/// +/// Drivers which only use atomic internally, but do not support the full userspace API (e.g. not +/// all properties converted to atomic, or multi-plane updates are not guaranteed to be tear-free) +/// should not set this flag. +pub const FEAT_ATOMIC: u32 = bindings::drm_driver_feature_DRIVER_ATOMIC; +/// Driver supports DRM sync objects for explicit synchronization of command submission. +pub const FEAT_SYNCOBJ: u32 = bindings::drm_driver_feature_DRIVER_SYNCOBJ; +/// Driver supports the timeline flavor of DRM sync objects for explicit synchronization of command +/// submission. +pub const FEAT_SYNCOBJ_TIMELINE: u32 = bindings::drm_driver_feature_DRIVER_SYNCOBJ_TIMELINE; +/// Driver supports compute acceleration devices. This flag is mutually exclusive with `FEAT_RENDER` +/// and `FEAT_MODESET`. Devices that support both graphics and compute acceleration should be +/// handled by two drivers that are connected using auxiliary bus. +pub const FEAT_COMPUTE_ACCEL: u32 = bindings::drm_driver_feature_DRIVER_COMPUTE_ACCEL; +/// Driver supports user defined GPU VA bindings for GEM objects. +pub const FEAT_GEM_GPUVA: u32 = bindings::drm_driver_feature_DRIVER_GEM_GPUVA; +/// Driver supports and requires cursor hotspot information in the cursor plane (e.g. cursor plane +/// has to actually track the mouse cursor and the clients are required to set hotspot in order for +/// the cursor planes to work correctly). +pub const FEAT_CURSOR_HOTSPOT: u32 = bindings::drm_driver_feature_DRIVER_CURSOR_HOTSPOT; + +/// Information data for a DRM Driver. +pub struct DriverInfo { + /// Driver major version. + pub major: i32, + /// Driver minor version. + pub minor: i32, + /// Driver patchlevel version. + pub patchlevel: i32, + /// Driver name. + pub name: &'static CStr, + /// Driver description. + pub desc: &'static CStr, + /// Driver date. + pub date: &'static CStr, +} + +/// Internal memory management operation set, normally created by memory managers (e.g. GEM). +/// +/// See `kernel::drm::gem` and `kernel::drm::gem::shmem`. +pub struct AllocOps { + #[expect(unused)] + pub(crate) gem_create_object: Option< + unsafe extern "C" fn( + dev: *mut bindings::drm_device, + size: usize, + ) -> *mut bindings::drm_gem_object, + >, + #[expect(unused)] + pub(crate) prime_handle_to_fd: Option< + unsafe extern "C" fn( + dev: *mut bindings::drm_device, + file_priv: *mut bindings::drm_file, + handle: u32, + flags: u32, + prime_fd: *mut core::ffi::c_int, + ) -> core::ffi::c_int, + >, + #[expect(unused)] + pub(crate) prime_fd_to_handle: Option< + unsafe extern "C" fn( + dev: *mut bindings::drm_device, + file_priv: *mut bindings::drm_file, + prime_fd: core::ffi::c_int, + handle: *mut u32, + ) -> core::ffi::c_int, + >, + #[expect(unused)] + pub(crate) gem_prime_import: Option< + unsafe extern "C" fn( + dev: *mut bindings::drm_device, + dma_buf: *mut bindings::dma_buf, + ) -> *mut bindings::drm_gem_object, + >, + #[expect(unused)] + pub(crate) gem_prime_import_sg_table: Option< + unsafe extern "C" fn( + dev: *mut bindings::drm_device, + attach: *mut bindings::dma_buf_attachment, + sgt: *mut bindings::sg_table, + ) -> *mut bindings::drm_gem_object, + >, + #[expect(unused)] + pub(crate) dumb_create: Option< + unsafe extern "C" fn( + file_priv: *mut bindings::drm_file, + dev: *mut bindings::drm_device, + args: *mut bindings::drm_mode_create_dumb, + ) -> core::ffi::c_int, + >, + #[expect(unused)] + pub(crate) dumb_map_offset: Option< + unsafe extern "C" fn( + file_priv: *mut bindings::drm_file, + dev: *mut bindings::drm_device, + handle: u32, + offset: *mut u64, + ) -> core::ffi::c_int, + >, +} + +/// Trait for memory manager implementations. Implemented internally. +pub trait AllocImpl: super::private::Sealed { + /// The C callback operations for this memory manager. + const ALLOC_OPS: AllocOps; +} + +/// The DRM `Driver` trait. +/// +/// 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 { + /// Context data associated with the DRM driver + type Data: Sync + Send; + + /// The type used to manage memory for this driver. + /// + /// Should be either `drm::gem::Object<T>` or `drm::gem::shmem::Object<T>`. + type Object: AllocImpl; + + /// Driver metadata + const INFO: DriverInfo; + + /// IOCTL list. See `kernel::drm::ioctl::declare_drm_ioctls!{}`. + const IOCTLS: &'static [drm::ioctl::DrmIoctlDescriptor]; +} diff --git a/rust/kernel/drm/mod.rs b/rust/kernel/drm/mod.rs index 9ec6d7cbcaf3..2e3f9a8a9353 100644 --- a/rust/kernel/drm/mod.rs +++ b/rust/kernel/drm/mod.rs @@ -2,4 +2,12 @@ //! DRM subsystem abstractions. +pub mod driver; pub mod ioctl; + +pub use self::driver::Driver; +pub use self::driver::DriverInfo; + +pub(crate) mod private { + pub trait Sealed {} +}