| Message ID | 20240618233324.14217-4-dakr@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DRM Rust abstractions and Nova | expand |
On Wed, Jun 19, 2024 at 01:31:39AM +0200, Danilo Krummrich wrote: > Implement the DRM driver abstractions. > > The `Driver` trait provides the interface to the actual driver to fill > in the driver specific data, such as the `DriverInfo`, driver features > and IOCTLs. > > Co-developed-by: Asahi Lina <lina@asahilina.net> > Signed-off-by: Asahi Lina <lina@asahilina.net> > Signed-off-by: Danilo Krummrich <dakr@redhat.com> > --- > rust/bindings/bindings_helper.h | 1 + > rust/kernel/drm/drv.rs | 141 ++++++++++++++++++++++++++++++++ > rust/kernel/drm/mod.rs | 1 + > 3 files changed, 143 insertions(+) > create mode 100644 rust/kernel/drm/drv.rs > > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h > index ed25b686748e..dc19cb789536 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/errname.h> > diff --git a/rust/kernel/drm/drv.rs b/rust/kernel/drm/drv.rs > new file mode 100644 > index 000000000000..cd594a32f9e4 > --- /dev/null > +++ b/rust/kernel/drm/drv.rs > @@ -0,0 +1,141 @@ > +// 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, private::Sealed, str::CStr, types::ForeignOwnable}; > +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 { > + pub(crate) gem_create_object: Option< > + unsafe extern "C" fn( > + dev: *mut bindings::drm_device, > + size: usize, > + ) -> *mut bindings::drm_gem_object, > + >, > + 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, > + >, > + 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, > + >, > + 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, > + >, > + 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, > + >, > + 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, > + >, > + 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: 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 > + /// > + /// Determines the type of the context data passed to each of the methods of the trait. > + type Data: ForeignOwnable + 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; Bit similar comment to what I discussed at length with lyude, drivers might have a need for different implementations. But I think from the kms discussions we have solid solution for that, so I think we should be fine. > + > + /// Driver metadata > + const INFO: DriverInfo; > + > + /// Feature flags > + const FEATURES: u32; I think there's a type safety issue here with allowing drivers to muck with these directly. Example: - If you don't set FEAT_GEM but try to use gem C functions, stuff blows up because the core doesn't call drm_gem_init() in that case. - For modesetting it's more fun because there mandatory init functions are meant to be called by the driver, in the right sequence, interleaved with other driver setup code for all the right modeset objects. If you get it wrong you go boom. For the modeset side of things I've dumped a pile of comments on lyude's patches already: Essentially during registration I think we need a special drmKmsDriverInit object or phantom type or so, so that you can proof you're registering kms objects at the right time, with the rust abstraction calling all the other functions around that in the right order. For gem I think we should flat out not allow non-gem drivers in rust. At least until someone comes up with a need for a non-gem driver. For some of the values like hotspot cursor support we might need to change the rust abstraction to compute these at runtime driver init, and then set them somewhere in the runtime data structure instead of having them statically sepcified in drm_driver->features. In general these feature flag are midlayer design and that tends to be bad, rust is just the messenger here. Cheers, Sima > + > + /// 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..d987c56b3cec 100644 > --- a/rust/kernel/drm/mod.rs > +++ b/rust/kernel/drm/mod.rs > @@ -2,4 +2,5 @@ > > //! DRM subsystem abstractions. > > +pub mod drv; > pub mod ioctl; > -- > 2.45.1 >
On 9/3/24 1:29 AM, Daniel Vetter wrote: > On Wed, Jun 19, 2024 at 01:31:39AM +0200, Danilo Krummrich wrote: >> Implement the DRM driver abstractions. >> >> The `Driver` trait provides the interface to the actual driver to fill >> in the driver specific data, such as the `DriverInfo`, driver features >> and IOCTLs. >> >> Co-developed-by: Asahi Lina <lina@asahilina.net> >> Signed-off-by: Asahi Lina <lina@asahilina.net> >> Signed-off-by: Danilo Krummrich <dakr@redhat.com> >> --- >> rust/bindings/bindings_helper.h | 1 + >> rust/kernel/drm/drv.rs | 141 ++++++++++++++++++++++++++++++++ >> rust/kernel/drm/mod.rs | 1 + >> 3 files changed, 143 insertions(+) >> create mode 100644 rust/kernel/drm/drv.rs >> >> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h >> index ed25b686748e..dc19cb789536 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/errname.h> >> diff --git a/rust/kernel/drm/drv.rs b/rust/kernel/drm/drv.rs >> new file mode 100644 >> index 000000000000..cd594a32f9e4 >> --- /dev/null >> +++ b/rust/kernel/drm/drv.rs >> @@ -0,0 +1,141 @@ >> +// 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, private::Sealed, str::CStr, types::ForeignOwnable}; >> +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 { >> + pub(crate) gem_create_object: Option< >> + unsafe extern "C" fn( >> + dev: *mut bindings::drm_device, >> + size: usize, >> + ) -> *mut bindings::drm_gem_object, >> + >, >> + 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, >> + >, >> + 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, >> + >, >> + 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, >> + >, >> + 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, >> + >, >> + 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, >> + >, >> + 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: 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 >> + /// >> + /// Determines the type of the context data passed to each of the methods of the trait. >> + type Data: ForeignOwnable + 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; > > Bit similar comment to what I discussed at length with lyude, drivers > might have a need for different implementations. But I think from the kms > discussions we have solid solution for that, so I think we should be fine. > >> + >> + /// Driver metadata >> + const INFO: DriverInfo; >> + >> + /// Feature flags >> + const FEATURES: u32; > > I think there's a type safety issue here with allowing drivers to muck > with these directly. Example: > > - If you don't set FEAT_GEM but try to use gem C functions, stuff blows up > because the core doesn't call drm_gem_init() in that case. > > - For modesetting it's more fun because there mandatory init functions are > meant to be called by the driver, in the right sequence, interleaved > with other driver setup code for all the right modeset objects. If you > get it wrong you go boom. Same with GEM_GPUVA, I noticed that if you don't set the flag it blows up. But the only effect of the flag seems to be some trivial initialization in GEM objects (a single INIT_LIST_HEAD)... > > For the modeset side of things I've dumped a pile of comments on lyude's > patches already: Essentially during registration I think we need a special > drmKmsDriverInit object or phantom type or so, so that you can proof > you're registering kms objects at the right time, with the rust > abstraction calling all the other functions around that in the right > order. > > For gem I think we should flat out not allow non-gem drivers in rust. At > least until someone comes up with a need for a non-gem driver. ... so I think we can also just hard-code GPUVM on even if the driver doesn't actually make use of the functionality? I'm not even sure if that feature flag should exist at this point, it's probably faster not to check for the feature and just unconditionally initialize it even for drivers that don't need it. > > For some of the values like hotspot cursor support we might need to change > the rust abstraction to compute these at runtime driver init, and then set > them somewhere in the runtime data structure instead of having them > statically sepcified in drm_driver->features. > > In general these feature flag are midlayer design and that tends to be > bad, rust is just the messenger here. > > Cheers, Sima > > >> + >> + /// 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..d987c56b3cec 100644 >> --- a/rust/kernel/drm/mod.rs >> +++ b/rust/kernel/drm/mod.rs >> @@ -2,4 +2,5 @@ >> >> //! DRM subsystem abstractions. >> >> +pub mod drv; >> pub mod ioctl; >> -- >> 2.45.1 >> > ~~ Lina
On Mon, Sep 02, 2024 at 06:29:06PM +0200, Daniel Vetter wrote: > On Wed, Jun 19, 2024 at 01:31:39AM +0200, Danilo Krummrich wrote: > > Implement the DRM driver abstractions. > > > > The `Driver` trait provides the interface to the actual driver to fill > > in the driver specific data, such as the `DriverInfo`, driver features > > and IOCTLs. > > > > Co-developed-by: Asahi Lina <lina@asahilina.net> > > Signed-off-by: Asahi Lina <lina@asahilina.net> > > Signed-off-by: Danilo Krummrich <dakr@redhat.com> > > --- > > rust/bindings/bindings_helper.h | 1 + > > rust/kernel/drm/drv.rs | 141 ++++++++++++++++++++++++++++++++ > > rust/kernel/drm/mod.rs | 1 + > > 3 files changed, 143 insertions(+) > > create mode 100644 rust/kernel/drm/drv.rs > > > > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h > > index ed25b686748e..dc19cb789536 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/errname.h> > > diff --git a/rust/kernel/drm/drv.rs b/rust/kernel/drm/drv.rs > > new file mode 100644 > > index 000000000000..cd594a32f9e4 > > --- /dev/null > > +++ b/rust/kernel/drm/drv.rs > > @@ -0,0 +1,141 @@ > > +// 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, private::Sealed, str::CStr, types::ForeignOwnable}; > > +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 { > > + pub(crate) gem_create_object: Option< > > + unsafe extern "C" fn( > > + dev: *mut bindings::drm_device, > > + size: usize, > > + ) -> *mut bindings::drm_gem_object, > > + >, > > + 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, > > + >, > > + 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, > > + >, > > + 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, > > + >, > > + 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, > > + >, > > + 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, > > + >, > > + 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: 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 > > + /// > > + /// Determines the type of the context data passed to each of the methods of the trait. > > + type Data: ForeignOwnable + 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; > > Bit similar comment to what I discussed at length with lyude, drivers > might have a need for different implementations. But I think from the kms > discussions we have solid solution for that, so I think we should be fine. > > > + > > + /// Driver metadata > > + const INFO: DriverInfo; > > + > > + /// Feature flags > > + const FEATURES: u32; > > I think there's a type safety issue here with allowing drivers to muck > with these directly. Example: > > - If you don't set FEAT_GEM but try to use gem C functions, stuff blows up > because the core doesn't call drm_gem_init() in that case. Indeed. Ideally, we'd want the feature flags to be automatically set, when a corresponding implementation is provided by the driver. For now, as you say, GEM can just be mandatory I think. > > - For modesetting it's more fun because there mandatory init functions are > meant to be called by the driver, in the right sequence, interleaved > with other driver setup code for all the right modeset objects. If you > get it wrong you go boom. My proposal was to just have another associated `Kms` type for `Driver` that provides the corresponding callbacks for initialization. KMS initialization functions could then be exposed only through those callbacks, such that they can't be called after registration. > > For the modeset side of things I've dumped a pile of comments on lyude's > patches already: Essentially during registration I think we need a special > drmKmsDriverInit object or phantom type or so, so that you can proof > you're registering kms objects at the right time, with the rust > abstraction calling all the other functions around that in the right > order. > > For gem I think we should flat out not allow non-gem drivers in rust. At > least until someone comes up with a need for a non-gem driver. > > For some of the values like hotspot cursor support we might need to change > the rust abstraction to compute these at runtime driver init, and then set > them somewhere in the runtime data structure instead of having them > statically sepcified in drm_driver->features. > > In general these feature flag are midlayer design and that tends to be > bad, rust is just the messenger here. > > Cheers, Sima > > > > + > > + /// 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..d987c56b3cec 100644 > > --- a/rust/kernel/drm/mod.rs > > +++ b/rust/kernel/drm/mod.rs > > @@ -2,4 +2,5 @@ > > > > //! DRM subsystem abstractions. > > > > +pub mod drv; > > pub mod ioctl; > > -- > > 2.45.1 > > > > -- > Simona Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch >
On Tue, Sep 03, 2024 at 01:11:55PM +0200, Danilo Krummrich wrote: > On Mon, Sep 02, 2024 at 06:29:06PM +0200, Daniel Vetter wrote: > > On Wed, Jun 19, 2024 at 01:31:39AM +0200, Danilo Krummrich wrote: > > > Implement the DRM driver abstractions. > > > > > > The `Driver` trait provides the interface to the actual driver to fill > > > in the driver specific data, such as the `DriverInfo`, driver features > > > and IOCTLs. > > > > > > Co-developed-by: Asahi Lina <lina@asahilina.net> > > > Signed-off-by: Asahi Lina <lina@asahilina.net> > > > Signed-off-by: Danilo Krummrich <dakr@redhat.com> > > > --- > > > rust/bindings/bindings_helper.h | 1 + > > > rust/kernel/drm/drv.rs | 141 ++++++++++++++++++++++++++++++++ > > > rust/kernel/drm/mod.rs | 1 + > > > 3 files changed, 143 insertions(+) > > > create mode 100644 rust/kernel/drm/drv.rs > > > > > > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h > > > index ed25b686748e..dc19cb789536 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/errname.h> > > > diff --git a/rust/kernel/drm/drv.rs b/rust/kernel/drm/drv.rs > > > new file mode 100644 > > > index 000000000000..cd594a32f9e4 > > > --- /dev/null > > > +++ b/rust/kernel/drm/drv.rs > > > @@ -0,0 +1,141 @@ > > > +// 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, private::Sealed, str::CStr, types::ForeignOwnable}; > > > +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 { > > > + pub(crate) gem_create_object: Option< > > > + unsafe extern "C" fn( > > > + dev: *mut bindings::drm_device, > > > + size: usize, > > > + ) -> *mut bindings::drm_gem_object, > > > + >, > > > + 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, > > > + >, > > > + 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, > > > + >, > > > + 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, > > > + >, > > > + 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, > > > + >, > > > + 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, > > > + >, > > > + 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: 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 > > > + /// > > > + /// Determines the type of the context data passed to each of the methods of the trait. > > > + type Data: ForeignOwnable + 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; > > > > Bit similar comment to what I discussed at length with lyude, drivers > > might have a need for different implementations. But I think from the kms > > discussions we have solid solution for that, so I think we should be fine. > > > > > + > > > + /// Driver metadata > > > + const INFO: DriverInfo; > > > + > > > + /// Feature flags > > > + const FEATURES: u32; > > > > I think there's a type safety issue here with allowing drivers to muck > > with these directly. Example: > > > > - If you don't set FEAT_GEM but try to use gem C functions, stuff blows up > > because the core doesn't call drm_gem_init() in that case. > > Indeed. Ideally, we'd want the feature flags to be automatically set, when a > corresponding implementation is provided by the driver. > > For now, as you say, GEM can just be mandatory I think. > > > > > - For modesetting it's more fun because there mandatory init functions are > > meant to be called by the driver, in the right sequence, interleaved > > with other driver setup code for all the right modeset objects. If you > > get it wrong you go boom. > > My proposal was to just have another associated `Kms` type for `Driver` that > provides the corresponding callbacks for initialization. KMS initialization > functions could then be exposed only through those callbacks, such that they > can't be called after registration. Hm yeah I guess callbacks works too. No idea whether that's the rust way or not though, at least on the C side callbacks for init are kinda midlayer smell and tend to be in the way. -Sima > > > > > For the modeset side of things I've dumped a pile of comments on lyude's > > patches already: Essentially during registration I think we need a special > > drmKmsDriverInit object or phantom type or so, so that you can proof > > you're registering kms objects at the right time, with the rust > > abstraction calling all the other functions around that in the right > > order. > > > > For gem I think we should flat out not allow non-gem drivers in rust. At > > least until someone comes up with a need for a non-gem driver. > > > > For some of the values like hotspot cursor support we might need to change > > the rust abstraction to compute these at runtime driver init, and then set > > them somewhere in the runtime data structure instead of having them > > statically sepcified in drm_driver->features. > > > > In general these feature flag are midlayer design and that tends to be > > bad, rust is just the messenger here. > > > > Cheers, Sima > > > > > > > + > > > + /// 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..d987c56b3cec 100644 > > > --- a/rust/kernel/drm/mod.rs > > > +++ b/rust/kernel/drm/mod.rs > > > @@ -2,4 +2,5 @@ > > > > > > //! DRM subsystem abstractions. > > > > > > +pub mod drv; > > > pub mod ioctl; > > > -- > > > 2.45.1 > > > > > > > -- > > Simona Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > >
On Mon, 2024-09-02 at 18:29 +0200, Daniel Vetter wrote: > On Wed, Jun 19, 2024 at 01:31:39AM +0200, Danilo Krummrich wrote: > > Implement the DRM driver abstractions. > > > > The `Driver` trait provides the interface to the actual driver to fill > > in the driver specific data, such as the `DriverInfo`, driver features > > and IOCTLs. > > > > Co-developed-by: Asahi Lina <lina@asahilina.net> > > Signed-off-by: Asahi Lina <lina@asahilina.net> > > Signed-off-by: Danilo Krummrich <dakr@redhat.com> > > --- > > rust/bindings/bindings_helper.h | 1 + > > rust/kernel/drm/drv.rs | 141 ++++++++++++++++++++++++++++++++ > > rust/kernel/drm/mod.rs | 1 + > > 3 files changed, 143 insertions(+) > > create mode 100644 rust/kernel/drm/drv.rs > > > > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h > > index ed25b686748e..dc19cb789536 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/errname.h> > > diff --git a/rust/kernel/drm/drv.rs b/rust/kernel/drm/drv.rs > > new file mode 100644 > > index 000000000000..cd594a32f9e4 > > --- /dev/null > > +++ b/rust/kernel/drm/drv.rs > > @@ -0,0 +1,141 @@ > > +// 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, private::Sealed, str::CStr, types::ForeignOwnable}; > > +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 { > > + pub(crate) gem_create_object: Option< > > + unsafe extern "C" fn( > > + dev: *mut bindings::drm_device, > > + size: usize, > > + ) -> *mut bindings::drm_gem_object, > > + >, > > + 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, > > + >, > > + 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, > > + >, > > + 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, > > + >, > > + 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, > > + >, > > + 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, > > + >, > > + 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: 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 > > + /// > > + /// Determines the type of the context data passed to each of the methods of the trait. > > + type Data: ForeignOwnable + 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; > > Bit similar comment to what I discussed at length with lyude, drivers > might have a need for different implementations. But I think from the kms > discussions we have solid solution for that, so I think we should be fine. > > > + > > + /// Driver metadata > > + const INFO: DriverInfo; > > + > > + /// Feature flags > > + const FEATURES: u32; > > I think there's a type safety issue here with allowing drivers to muck > with these directly. Example: > > - If you don't set FEAT_GEM but try to use gem C functions, stuff blows up > because the core doesn't call drm_gem_init() in that case. > > - For modesetting it's more fun because there mandatory init functions are > meant to be called by the driver, in the right sequence, interleaved > with other driver setup code for all the right modeset objects. If you > get it wrong you go boom. > > For the modeset side of things I've dumped a pile of comments on lyude's > patches already: Essentially during registration I think we need a special > drmKmsDriverInit object or phantom type or so, so that you can proof > you're registering kms objects at the right time, with the rust > abstraction calling all the other functions around that in the right > order. Yes actually, and the next version of my patches that I'll be sending out actually has exactly this :). Note - I need to update this branch, but this should give you a pretty good idea of how this works currently: https://gitlab.freedesktop.org/lyudess/linux/-/blob/rvkms-wip/rust/kernel/drm/kms.rs?ref_type=heads#L109 Once a driver does that, it gets an automatic (and not-overridable) implementation of `KmsImpl` here: https://gitlab.freedesktop.org/lyudess/linux/-/blob/rvkms-wip/rust/kernel/drm/kms.rs?ref_type=heads#L137 Which allows it to pass whatever type (it can be done with any type, since we don't instantiate a `KmsImpl`) as an associated type to the driver here: https://gitlab.freedesktop.org/lyudess/linux/-/blob/rvkms-wip/rust/kernel/drm/drv.rs?ref_type=heads#L139 And then finally, we do compile-time gating of functionality that's dependent on KMS by using the `KmsDriver` trait which is also implemented for drivers implementing `Kms`: https://gitlab.freedesktop.org/lyudess/linux/-/blob/rvkms-wip/rust/kernel/drm/kms.rs?ref_type=heads#L240 (ignore the mode_config_reset bit there, I'm going to be dropping that function). For drivers that don't use KMS, we provide an stub implementation of `KmsImpl` for PhantomData<T>: https://gitlab.freedesktop.org/lyudess/linux/-/blob/rvkms-wip/rust/kernel/drm/kms.rs?ref_type=heads#L213 And those drivers can just use PhantomData<Self> for fulfilling the associated type on kms::drv::Driver It may even be worth mentioning, I've already at least handled vblank events - which is a pretty good example of the pattern I think will work for handling KMS-dependent optional driver functionality: https://gitlab.freedesktop.org/lyudess/linux/-/blob/rvkms-wip/rust/kernel/drm/kms/vblank.rs?ref_type=heads There's definitely a number of changes I need to make there, but it's more or less the same thing. > > For gem I think we should flat out not allow non-gem drivers in rust. At > least until someone comes up with a need for a non-gem driver. > > For some of the values like hotspot cursor support we might need to change > the rust abstraction to compute these at runtime driver init, and then set > them somewhere in the runtime data structure instead of having them > statically sepcified in drm_driver->features. Yeah - in the bindings that I wrote up, there is a hook dedicated for computing mode_config_info that has early access to a DRM device's private data which can be used for passing information needed for this. So runtime initialization should be totally possible > > In general these feature flag are midlayer design and that tends to be > bad, rust is just the messenger here. > > Cheers, Sima > > > > + > > + /// 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..d987c56b3cec 100644 > > --- a/rust/kernel/drm/mod.rs > > +++ b/rust/kernel/drm/mod.rs > > @@ -2,4 +2,5 @@ > > > > //! DRM subsystem abstractions. > > > > +pub mod drv; > > pub mod ioctl; > > -- > > 2.45.1 > > >
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h index ed25b686748e..dc19cb789536 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/errname.h> diff --git a/rust/kernel/drm/drv.rs b/rust/kernel/drm/drv.rs new file mode 100644 index 000000000000..cd594a32f9e4 --- /dev/null +++ b/rust/kernel/drm/drv.rs @@ -0,0 +1,141 @@ +// 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, private::Sealed, str::CStr, types::ForeignOwnable}; +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 { + pub(crate) gem_create_object: Option< + unsafe extern "C" fn( + dev: *mut bindings::drm_device, + size: usize, + ) -> *mut bindings::drm_gem_object, + >, + 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, + >, + 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, + >, + 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, + >, + 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, + >, + 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, + >, + 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: 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 + /// + /// Determines the type of the context data passed to each of the methods of the trait. + type Data: ForeignOwnable + 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; + + /// Feature flags + const FEATURES: u32; + + /// 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..d987c56b3cec 100644 --- a/rust/kernel/drm/mod.rs +++ b/rust/kernel/drm/mod.rs @@ -2,4 +2,5 @@ //! DRM subsystem abstractions. +pub mod drv; pub mod ioctl;