* [RFC PATCH 1/8] rust: drm: ioctl: Add DRM ioctl abstraction
2024-05-20 17:20 [RFC PATCH 0/8] [RFC] DRM Rust abstractions and Nova Danilo Krummrich
@ 2024-05-20 17:20 ` Danilo Krummrich
2024-05-20 17:20 ` [RFC PATCH 2/8] rust: Add a Sealed trait Danilo Krummrich
` (8 subsequent siblings)
9 siblings, 0 replies; 53+ messages in thread
From: Danilo Krummrich @ 2024-05-20 17:20 UTC (permalink / raw)
To: maarten.lankhorst, mripard, tzimmermann, airlied, daniel, ojeda,
alex.gaynor, wedsonaf, boqun.feng, gary, bjorn3_gh, benno.lossin,
a.hindborg, aliceryhl, fujita.tomonori, lina, pstanner, ajanulgu,
lyude, gregkh
Cc: rust-for-linux, dri-devel, nouveau, Danilo Krummrich
From: Asahi Lina <lina@asahilina.net>
DRM drivers need to be able to declare which driver-specific ioctls they
support. Add an abstraction implementing the required types and a helper
macro to generate the ioctl definition inside the DRM driver.
Note that this macro is not usable until further bits of the abstraction
are in place (but it will not fail to compile on its own, if not called).
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/ioctl.rs | 153 ++++++++++++++++++++++++++++++++
rust/kernel/drm/mod.rs | 5 ++
rust/kernel/lib.rs | 2 +
rust/uapi/uapi_helper.h | 1 +
5 files changed, 162 insertions(+)
create mode 100644 rust/kernel/drm/ioctl.rs
create mode 100644 rust/kernel/drm/mod.rs
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 32221de16e57..14188034285a 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -6,6 +6,7 @@
* Sorted alphabetically.
*/
+#include <drm/drm_ioctl.h>
#include <kunit/test.h>
#include <linux/errname.h>
#include <linux/ethtool.h>
diff --git a/rust/kernel/drm/ioctl.rs b/rust/kernel/drm/ioctl.rs
new file mode 100644
index 000000000000..79d720e9d18e
--- /dev/null
+++ b/rust/kernel/drm/ioctl.rs
@@ -0,0 +1,153 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+#![allow(non_snake_case)]
+
+//! DRM IOCTL definitions.
+//!
+//! C header: [`include/linux/drm/drm_ioctl.h`](../../../../include/linux/drm/drm_ioctl.h)
+
+use crate::ioctl;
+
+const BASE: u32 = uapi::DRM_IOCTL_BASE as u32;
+
+/// Construct a DRM ioctl number with no argument.
+#[inline(always)]
+pub const fn IO(nr: u32) -> u32 {
+ ioctl::_IO(BASE, nr)
+}
+
+/// Construct a DRM ioctl number with a read-only argument.
+#[inline(always)]
+pub const fn IOR<T>(nr: u32) -> u32 {
+ ioctl::_IOR::<T>(BASE, nr)
+}
+
+/// Construct a DRM ioctl number with a write-only argument.
+#[inline(always)]
+pub const fn IOW<T>(nr: u32) -> u32 {
+ ioctl::_IOW::<T>(BASE, nr)
+}
+
+/// Construct a DRM ioctl number with a read-write argument.
+#[inline(always)]
+pub const fn IOWR<T>(nr: u32) -> u32 {
+ ioctl::_IOWR::<T>(BASE, nr)
+}
+
+/// Descriptor type for DRM ioctls. Use the `declare_drm_ioctls!{}` macro to construct them.
+pub type DrmIoctlDescriptor = bindings::drm_ioctl_desc;
+
+/// This is for ioctl which are used for rendering, and require that the file descriptor is either
+/// for a render node, or if it’s a legacy/primary node, then it must be authenticated.
+pub const AUTH: u32 = bindings::drm_ioctl_flags_DRM_AUTH;
+
+/// This must be set for any ioctl which can change the modeset or display state. Userspace must
+/// call the ioctl through a primary node, while it is the active master.
+///
+/// Note that read-only modeset ioctl can also be called by unauthenticated clients, or when a
+/// master is not the currently active one.
+pub const MASTER: u32 = bindings::drm_ioctl_flags_DRM_MASTER;
+
+/// Anything that could potentially wreak a master file descriptor needs to have this flag set.
+///
+/// Current that’s only for the SETMASTER and DROPMASTER ioctl, which e.g. logind can call to
+/// force a non-behaving master (display compositor) into compliance.
+///
+/// This is equivalent to callers with the SYSADMIN capability.
+pub const ROOT_ONLY: u32 = bindings::drm_ioctl_flags_DRM_ROOT_ONLY;
+
+/// This is used for all ioctl needed for rendering only, for drivers which support render nodes.
+/// This should be all new render drivers, and hence it should be always set for any ioctl with
+/// `AUTH` set. Note though that read-only query ioctl might have this set, but have not set
+/// DRM_AUTH because they do not require authentication.
+pub const RENDER_ALLOW: u32 = bindings::drm_ioctl_flags_DRM_RENDER_ALLOW;
+
+/// Internal structures used by the [`declare_drm_ioctls!{}`] macro. Do not use directly.
+#[doc(hidden)]
+pub mod internal {
+ pub use bindings::drm_device;
+ pub use bindings::drm_file;
+ pub use bindings::drm_ioctl_desc;
+}
+
+/// Declare the DRM ioctls for a driver.
+///
+/// Each entry in the list should have the form:
+///
+/// `(ioctl_number, argument_type, flags, user_callback),`
+///
+/// `argument_type` is the type name within the `bindings` crate.
+/// `user_callback` should have the following prototype:
+///
+/// ```
+/// fn foo(device: &kernel::drm::device::Device<Self>,
+/// data: &mut bindings::argument_type,
+/// file: &kernel::drm::file::File<Self::File>,
+/// )
+/// ```
+/// where `Self` is the drm::drv::Driver implementation these ioctls are being declared within.
+///
+/// # Examples
+///
+/// ```
+/// kernel::declare_drm_ioctls! {
+/// (FOO_GET_PARAM, drm_foo_get_param, ioctl::RENDER_ALLOW, my_get_param_handler),
+/// }
+/// ```
+///
+#[macro_export]
+macro_rules! declare_drm_ioctls {
+ ( $(($cmd:ident, $struct:ident, $flags:expr, $func:expr)),* $(,)? ) => {
+ const IOCTLS: &'static [$crate::drm::ioctl::DrmIoctlDescriptor] = {
+ use $crate::uapi::*;
+ const _:() = {
+ let i: u32 = $crate::uapi::DRM_COMMAND_BASE;
+ // Assert that all the IOCTLs are in the right order and there are no gaps,
+ // and that the sizeof of the specified type is correct.
+ $(
+ let cmd: u32 = $crate::macros::concat_idents!(DRM_IOCTL_, $cmd);
+ ::core::assert!(i == $crate::ioctl::_IOC_NR(cmd));
+ ::core::assert!(core::mem::size_of::<$crate::uapi::$struct>() ==
+ $crate::ioctl::_IOC_SIZE(cmd));
+ let i: u32 = i + 1;
+ )*
+ };
+
+ let ioctls = &[$(
+ $crate::drm::ioctl::internal::drm_ioctl_desc {
+ cmd: $crate::macros::concat_idents!(DRM_IOCTL_, $cmd) as u32,
+ func: {
+ #[allow(non_snake_case)]
+ unsafe extern "C" fn $cmd(
+ raw_dev: *mut $crate::drm::ioctl::internal::drm_device,
+ raw_data: *mut ::core::ffi::c_void,
+ raw_file_priv: *mut $crate::drm::ioctl::internal::drm_file,
+ ) -> core::ffi::c_int {
+ // SAFETY: The DRM core ensures the device lives while callbacks are
+ // being called.
+ //
+ // FIXME: Currently there is nothing enforcing that the types of the
+ // dev/file match the current driver these ioctls are being declared
+ // for, and it's not clear how to enforce this within the type system.
+ let dev = $crate::drm::device::Device::borrow(raw_dev);
+ // SAFETY: This is just the ioctl argument, which hopefully has the
+ // right type (we've done our best checking the size).
+ let data = unsafe { &mut *(raw_data as *mut $crate::uapi::$struct) };
+ // SAFETY: This is just the DRM file structure
+ let file = unsafe { $crate::drm::file::File::from_raw(raw_file_priv) };
+
+ match $func(dev, data, &file) {
+ Err(e) => e.to_errno(),
+ Ok(i) => i.try_into()
+ .unwrap_or($crate::error::code::ERANGE.to_errno()),
+ }
+ }
+ Some($cmd)
+ },
+ flags: $flags,
+ name: $crate::c_str!(::core::stringify!($cmd)).as_char_ptr(),
+ }
+ ),*];
+ ioctls
+ };
+ };
+}
diff --git a/rust/kernel/drm/mod.rs b/rust/kernel/drm/mod.rs
new file mode 100644
index 000000000000..9ec6d7cbcaf3
--- /dev/null
+++ b/rust/kernel/drm/mod.rs
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+
+//! DRM subsystem abstractions.
+
+pub mod ioctl;
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 15730deca822..cb0dd3d76729 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -32,6 +32,8 @@
pub mod device;
pub mod devres;
pub mod driver;
+#[cfg(CONFIG_DRM = "y")]
+pub mod drm;
pub mod error;
pub mod init;
pub mod ioctl;
diff --git a/rust/uapi/uapi_helper.h b/rust/uapi/uapi_helper.h
index 08f5e9334c9e..ed42a456da2e 100644
--- a/rust/uapi/uapi_helper.h
+++ b/rust/uapi/uapi_helper.h
@@ -7,5 +7,6 @@
*/
#include <uapi/asm-generic/ioctl.h>
+#include <uapi/drm/drm.h>
#include <uapi/linux/mii.h>
#include <uapi/linux/ethtool.h>
--
2.45.1
^ permalink raw reply related [flat|nested] 53+ messages in thread* [RFC PATCH 2/8] rust: Add a Sealed trait
2024-05-20 17:20 [RFC PATCH 0/8] [RFC] DRM Rust abstractions and Nova Danilo Krummrich
2024-05-20 17:20 ` [RFC PATCH 1/8] rust: drm: ioctl: Add DRM ioctl abstraction Danilo Krummrich
@ 2024-05-20 17:20 ` Danilo Krummrich
2024-05-20 17:20 ` [RFC PATCH 3/8] rust: drm: Add Device and Driver abstractions Danilo Krummrich
` (7 subsequent siblings)
9 siblings, 0 replies; 53+ messages in thread
From: Danilo Krummrich @ 2024-05-20 17:20 UTC (permalink / raw)
To: maarten.lankhorst, mripard, tzimmermann, airlied, daniel, ojeda,
alex.gaynor, wedsonaf, boqun.feng, gary, bjorn3_gh, benno.lossin,
a.hindborg, aliceryhl, fujita.tomonori, lina, pstanner, ajanulgu,
lyude, gregkh
Cc: rust-for-linux, dri-devel, nouveau, Danilo Krummrich
From: Asahi Lina <lina@asahilina.net>
Some traits exposed by the kernel crate may not be intended to be
implemented by downstream modules. Add a Sealed trait to allow avoiding
this using the sealed trait pattern.
Signed-off-by: Asahi Lina <lina@asahilina.net>
Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
rust/kernel/lib.rs | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index cb0dd3d76729..6415968ee3b8 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -65,6 +65,11 @@
#[doc(hidden)]
pub use build_error::build_error;
+pub(crate) mod private {
+ #[allow(unreachable_pub)]
+ pub trait Sealed {}
+}
+
/// Prefix to appear before log messages printed from within the `kernel` crate.
const __LOG_PREFIX: &[u8] = b"rust_kernel\0";
--
2.45.1
^ permalink raw reply related [flat|nested] 53+ messages in thread* [RFC PATCH 3/8] rust: drm: Add Device and Driver abstractions
2024-05-20 17:20 [RFC PATCH 0/8] [RFC] DRM Rust abstractions and Nova Danilo Krummrich
2024-05-20 17:20 ` [RFC PATCH 1/8] rust: drm: ioctl: Add DRM ioctl abstraction Danilo Krummrich
2024-05-20 17:20 ` [RFC PATCH 2/8] rust: Add a Sealed trait Danilo Krummrich
@ 2024-05-20 17:20 ` Danilo Krummrich
2024-05-21 21:23 ` Rob Herring
2024-05-20 17:20 ` [RFC PATCH 4/8] rust: drm: implement `AsRef` for DRM device Danilo Krummrich
` (6 subsequent siblings)
9 siblings, 1 reply; 53+ messages in thread
From: Danilo Krummrich @ 2024-05-20 17:20 UTC (permalink / raw)
To: maarten.lankhorst, mripard, tzimmermann, airlied, daniel, ojeda,
alex.gaynor, wedsonaf, boqun.feng, gary, bjorn3_gh, benno.lossin,
a.hindborg, aliceryhl, fujita.tomonori, lina, pstanner, ajanulgu,
lyude, gregkh
Cc: rust-for-linux, dri-devel, nouveau, Danilo Krummrich
From: Asahi Lina <lina@asahilina.net>
Add abstractions for DRM drivers and devices. These go together in one
commit since both are fairly tightly coupled types.
A few things have been stubbed out, to be implemented as further bits of
the DRM subsystem are introduced.
Signed-off-by: Asahi Lina <lina@asahilina.net>
Co-developed-by: Danilo Krummrich <dakr@redhat.com>
Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
rust/bindings/bindings_helper.h | 2 +
rust/kernel/drm/device.rs | 87 +++++++++
rust/kernel/drm/drv.rs | 318 ++++++++++++++++++++++++++++++++
rust/kernel/drm/mod.rs | 2 +
4 files changed, 409 insertions(+)
create mode 100644 rust/kernel/drm/device.rs
create mode 100644 rust/kernel/drm/drv.rs
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 14188034285a..831fbfe03a47 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -6,6 +6,8 @@
* Sorted alphabetically.
*/
+#include <drm/drm_device.h>
+#include <drm/drm_drv.h>
#include <drm/drm_ioctl.h>
#include <kunit/test.h>
#include <linux/errname.h>
diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
new file mode 100644
index 000000000000..f72bab8dd42d
--- /dev/null
+++ b/rust/kernel/drm/device.rs
@@ -0,0 +1,87 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+
+//! DRM device.
+//!
+//! C header: [`include/linux/drm/drm_device.h`](../../../../include/linux/drm/drm_device.h)
+
+use crate::{
+ bindings, device, drm,
+ error::code::*,
+ error::from_err_ptr,
+ error::Result,
+ types::{ARef, AlwaysRefCounted, ForeignOwnable, Opaque},
+};
+use alloc::boxed::Box;
+use core::{ffi::c_void, marker::PhantomData, pin::Pin, ptr::NonNull};
+
+/// A typed DRM device with a specific driver. The device is always reference-counted.
+#[repr(transparent)]
+pub struct Device<T: drm::drv::Driver>(Opaque<bindings::drm_device>, PhantomData<T>);
+
+impl<T: drm::drv::Driver> Device<T> {
+ pub(crate) fn new(
+ dev: &device::Device,
+ vtable: &Pin<Box<bindings::drm_driver>>,
+ ) -> Result<ARef<Self>> {
+ let raw_drm = unsafe { bindings::drm_dev_alloc(&**vtable, dev.as_raw()) };
+ let raw_drm = NonNull::new(from_err_ptr(raw_drm)? as *mut _).ok_or(ENOMEM)?;
+
+ // SAFETY: The reference count is one, and now we take ownership of that reference as a
+ // drm::device::Device.
+ Ok(unsafe { ARef::from_raw(raw_drm) })
+ }
+
+ pub(crate) fn as_raw(&self) -> *mut bindings::drm_device {
+ self.0.get()
+ }
+
+ // Not intended to be called externally, except via declare_drm_ioctls!()
+ #[doc(hidden)]
+ pub unsafe fn borrow<'a>(raw: *const bindings::drm_device) -> &'a Self {
+ unsafe { &*(raw as *const Self) }
+ }
+
+ pub(crate) fn raw_data(&self) -> *const c_void {
+ // SAFETY: `self` is guaranteed to hold a valid `bindings::drm_device` pointer.
+ unsafe { *self.as_raw() }.dev_private
+ }
+
+ // SAFETY: Must be called only once after device creation.
+ pub(crate) unsafe fn set_raw_data(&self, ptr: *const c_void) {
+ // SAFETY: Safe as by the safety precondition.
+ unsafe { &mut *self.as_raw() }.dev_private = ptr as _;
+ }
+
+ /// Returns a borrowed reference to the user data associated with this Device.
+ pub fn data(&self) -> Option<<T::Data as ForeignOwnable>::Borrowed<'_>> {
+ let dev_private = self.raw_data();
+
+ if dev_private.is_null() {
+ None
+ } else {
+ // SAFETY: `dev_private` is NULL before the DRM device is registered; after the DRM
+ // device has been registered dev_private is guaranteed to be valid.
+ Some(unsafe { T::Data::borrow(dev_private) })
+ }
+ }
+}
+
+// SAFETY: DRM device objects are always reference counted and the get/put functions
+// satisfy the requirements.
+unsafe impl<T: drm::drv::Driver> AlwaysRefCounted for Device<T> {
+ fn inc_ref(&self) {
+ unsafe { bindings::drm_dev_get(&self.as_raw() as *const _ as *mut _) };
+ }
+
+ unsafe fn dec_ref(obj: NonNull<Self>) {
+ // SAFETY: The Device<T> type has the same layout as drm_device, so we can just cast.
+ unsafe { bindings::drm_dev_put(obj.as_ptr() as *mut _) };
+ }
+}
+
+// SAFETY: `Device` only holds a pointer to a C device, which is safe to be used from any thread.
+unsafe impl<T: drm::drv::Driver> Send for Device<T> {}
+
+// SAFETY: `Device` only holds a pointer to a C device, references to which are safe to be used
+// from any thread.
+unsafe impl<T: drm::drv::Driver> Sync for Device<T> {}
diff --git a/rust/kernel/drm/drv.rs b/rust/kernel/drm/drv.rs
new file mode 100644
index 000000000000..5dd8f3f8df7c
--- /dev/null
+++ b/rust/kernel/drm/drv.rs
@@ -0,0 +1,318 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+
+//! DRM driver core.
+//!
+//! C header: [`include/linux/drm/drm_drv.h`](../../../../include/linux/drm/drm_drv.h)
+
+use crate::{
+ alloc::flags::*,
+ bindings, device, drm,
+ error::code::*,
+ error::{Error, Result},
+ prelude::*,
+ private::Sealed,
+ str::CStr,
+ types::{ARef, ForeignOwnable},
+ ThisModule,
+};
+use core::{
+ marker::{PhantomData, PhantomPinned},
+ pin::Pin,
+};
+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;
+
+/// 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;
+}
+
+/// A DRM driver implementation.
+#[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];
+}
+
+/// A registration of a DRM device
+///
+/// # Invariants:
+///
+/// drm is always a valid pointer to an allocated drm_device
+pub struct Registration<T: Driver> {
+ drm: ARef<drm::device::Device<T>>,
+ registered: bool,
+ fops: bindings::file_operations,
+ vtable: Pin<Box<bindings::drm_driver>>,
+ _p: PhantomData<T>,
+ _pin: PhantomPinned,
+}
+
+#[cfg(CONFIG_DRM_LEGACY)]
+macro_rules! drm_legacy_fields {
+ ( $($field:ident: $val:expr),* $(,)? ) => {
+ bindings::drm_driver {
+ $( $field: $val ),*,
+ firstopen: None,
+ preclose: None,
+ dma_ioctl: None,
+ dma_quiescent: None,
+ context_dtor: None,
+ irq_handler: None,
+ irq_preinstall: None,
+ irq_postinstall: None,
+ irq_uninstall: None,
+ get_vblank_counter: None,
+ enable_vblank: None,
+ disable_vblank: None,
+ dev_priv_size: 0,
+ }
+ }
+}
+
+#[cfg(not(CONFIG_DRM_LEGACY))]
+macro_rules! drm_legacy_fields {
+ ( $($field:ident: $val:expr),* $(,)? ) => {
+ bindings::drm_driver {
+ $( $field: $val ),*
+ }
+ }
+}
+
+/// Registers a DRM device with the rest of the kernel.
+///
+/// It automatically picks up THIS_MODULE.
+#[allow(clippy::crate_in_macro_def)]
+#[macro_export]
+macro_rules! drm_device_register {
+ ($reg:expr, $data:expr, $flags:expr $(,)?) => {{
+ $crate::drm::drv::Registration::register($reg, $data, $flags, &crate::THIS_MODULE)
+ }};
+}
+
+impl<T: Driver> Registration<T> {
+ const VTABLE: bindings::drm_driver = drm_legacy_fields! {
+ load: None,
+ open: None, // TODO: File abstraction
+ postclose: None, // TODO: File abstraction
+ lastclose: None,
+ unload: None,
+ release: None,
+ 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,
+ show_fdinfo: None,
+
+ major: T::INFO.major,
+ minor: T::INFO.minor,
+ patchlevel: T::INFO.patchlevel,
+ name: T::INFO.name.as_char_ptr() as *mut _,
+ desc: T::INFO.desc.as_char_ptr() as *mut _,
+ date: T::INFO.date.as_char_ptr() as *mut _,
+
+ driver_features: T::FEATURES,
+ ioctls: T::IOCTLS.as_ptr(),
+ num_ioctls: T::IOCTLS.len() as i32,
+ fops: core::ptr::null_mut(),
+ };
+
+ /// Creates a new [`Registration`] but does not register it yet.
+ ///
+ /// It is allowed to move.
+ pub fn new(parent: &device::Device) -> Result<Self> {
+ let vtable = Pin::new(Box::new(Self::VTABLE, GFP_KERNEL)?);
+
+ Ok(Self {
+ drm: drm::device::Device::new(parent, &vtable)?,
+ registered: false,
+ vtable,
+ fops: Default::default(), // TODO: GEM abstraction
+ _pin: PhantomPinned,
+ _p: PhantomData,
+ })
+ }
+
+ /// Registers a DRM device with the rest of the kernel.
+ ///
+ /// Users are encouraged to use the [`drm_device_register!()`] macro because it automatically
+ /// picks up the current module.
+ pub fn register(
+ self: Pin<&mut Self>,
+ data: T::Data,
+ flags: usize,
+ module: &'static ThisModule,
+ ) -> Result {
+ if self.registered {
+ // Already registered.
+ return Err(EINVAL);
+ }
+
+ // SAFETY: We never move out of `this`.
+ let this = unsafe { self.get_unchecked_mut() };
+ let data_pointer = <T::Data as ForeignOwnable>::into_foreign(data);
+
+ // SAFETY: This is the only code touching a device' private data pointer.
+ unsafe { this.drm.set_raw_data(data_pointer) };
+
+ this.fops.owner = module.0;
+ this.vtable.fops = &this.fops;
+
+ // SAFETY: The device is now initialized and ready to be registered.
+ let ret = unsafe { bindings::drm_dev_register(this.drm.as_raw(), flags as u64) };
+ if ret < 0 {
+ // SAFETY: `data_pointer` was returned by `into_foreign` above.
+ unsafe { T::Data::from_foreign(data_pointer) };
+ return Err(Error::from_errno(ret));
+ }
+
+ this.registered = true;
+ Ok(())
+ }
+
+ /// Returns a reference to the `Device` instance for this registration.
+ pub fn device(&self) -> &drm::device::Device<T> {
+ &self.drm
+ }
+}
+
+// SAFETY: `Registration` doesn't offer any methods or access to fields when shared between threads
+// or CPUs, so it is safe to share it.
+unsafe impl<T: Driver> Sync for Registration<T> {}
+
+// SAFETY: Registration with and unregistration from the drm subsystem can happen from any thread.
+// Additionally, `T::Data` (which is dropped during unregistration) is `Send`, so it is ok to move
+// `Registration` to different threads.
+#[allow(clippy::non_send_fields_in_send_ty)]
+unsafe impl<T: Driver> Send for Registration<T> {}
+
+impl<T: Driver> Drop for Registration<T> {
+ /// Removes the registration from the kernel if it has completed successfully before.
+ fn drop(&mut self) {
+ if self.registered {
+ // Get a pointer to the data stored in device before destroying it.
+ // SAFETY: `drm` is valid per the type invariant
+ let data_pointer = self.drm.raw_data();
+
+ // SAFETY: Since `registered` is true, `self.drm` is both valid and registered.
+ unsafe { bindings::drm_dev_unregister(self.drm.as_raw()) };
+
+ // Free data as well.
+ // SAFETY: `data_pointer` was returned by `into_foreign` during registration.
+ unsafe { <T::Data as ForeignOwnable>::from_foreign(data_pointer) };
+ }
+ }
+}
diff --git a/rust/kernel/drm/mod.rs b/rust/kernel/drm/mod.rs
index 9ec6d7cbcaf3..69376b3c6db9 100644
--- a/rust/kernel/drm/mod.rs
+++ b/rust/kernel/drm/mod.rs
@@ -2,4 +2,6 @@
//! DRM subsystem abstractions.
+pub mod device;
+pub mod drv;
pub mod ioctl;
--
2.45.1
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [RFC PATCH 3/8] rust: drm: Add Device and Driver abstractions
2024-05-20 17:20 ` [RFC PATCH 3/8] rust: drm: Add Device and Driver abstractions Danilo Krummrich
@ 2024-05-21 21:23 ` Rob Herring
2024-05-27 19:26 ` Danilo Krummrich
2024-06-09 5:15 ` Asahi Lina
0 siblings, 2 replies; 53+ messages in thread
From: Rob Herring @ 2024-05-21 21:23 UTC (permalink / raw)
To: Danilo Krummrich
Cc: maarten.lankhorst, mripard, tzimmermann, airlied, daniel, ojeda,
alex.gaynor, wedsonaf, boqun.feng, gary, bjorn3_gh, benno.lossin,
a.hindborg, aliceryhl, fujita.tomonori, lina, pstanner, ajanulgu,
lyude, gregkh, rust-for-linux, dri-devel, nouveau
On Mon, May 20, 2024 at 07:20:50PM +0200, Danilo Krummrich wrote:
> From: Asahi Lina <lina@asahilina.net>
>
> Add abstractions for DRM drivers and devices. These go together in one
> commit since both are fairly tightly coupled types.
>
> A few things have been stubbed out, to be implemented as further bits of
> the DRM subsystem are introduced.
>
> Signed-off-by: Asahi Lina <lina@asahilina.net>
> Co-developed-by: Danilo Krummrich <dakr@redhat.com>
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> ---
> rust/bindings/bindings_helper.h | 2 +
> rust/kernel/drm/device.rs | 87 +++++++++
> rust/kernel/drm/drv.rs | 318 ++++++++++++++++++++++++++++++++
> rust/kernel/drm/mod.rs | 2 +
> 4 files changed, 409 insertions(+)
> create mode 100644 rust/kernel/drm/device.rs
> create mode 100644 rust/kernel/drm/drv.rs
[...]
> diff --git a/rust/kernel/drm/drv.rs b/rust/kernel/drm/drv.rs
> new file mode 100644
> index 000000000000..5dd8f3f8df7c
> --- /dev/null
> +++ b/rust/kernel/drm/drv.rs
> @@ -0,0 +1,318 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +
> +//! DRM driver core.
> +//!
> +//! C header: [`include/linux/drm/drm_drv.h`](../../../../include/linux/drm/drm_drv.h)
> +
> +use crate::{
> + alloc::flags::*,
> + bindings, device, drm,
> + error::code::*,
> + error::{Error, Result},
> + prelude::*,
> + private::Sealed,
> + str::CStr,
> + types::{ARef, ForeignOwnable},
> + ThisModule,
> +};
> +use core::{
> + marker::{PhantomData, PhantomPinned},
> + pin::Pin,
> +};
> +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;
This is missing an entry for DRIVER_GEM_GPUVA. And some others perhaps.
I suppose some are legacy which won't be needed any time soon if ever.
Not sure if you intend for this to be complete, or you are just adding
what you are using? Only FEAT_GEM is used by nova ATM.
Rob
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [RFC PATCH 3/8] rust: drm: Add Device and Driver abstractions
2024-05-21 21:23 ` Rob Herring
@ 2024-05-27 19:26 ` Danilo Krummrich
2024-06-09 5:15 ` Asahi Lina
1 sibling, 0 replies; 53+ messages in thread
From: Danilo Krummrich @ 2024-05-27 19:26 UTC (permalink / raw)
To: Rob Herring
Cc: maarten.lankhorst, mripard, tzimmermann, airlied, daniel, ojeda,
alex.gaynor, wedsonaf, boqun.feng, gary, bjorn3_gh, benno.lossin,
a.hindborg, aliceryhl, fujita.tomonori, lina, pstanner, ajanulgu,
lyude, gregkh, rust-for-linux, dri-devel, nouveau
On Tue, May 21, 2024 at 04:23:33PM -0500, Rob Herring wrote:
> On Mon, May 20, 2024 at 07:20:50PM +0200, Danilo Krummrich wrote:
> > From: Asahi Lina <lina@asahilina.net>
> >
> > Add abstractions for DRM drivers and devices. These go together in one
> > commit since both are fairly tightly coupled types.
> >
> > A few things have been stubbed out, to be implemented as further bits of
> > the DRM subsystem are introduced.
> >
> > Signed-off-by: Asahi Lina <lina@asahilina.net>
> > Co-developed-by: Danilo Krummrich <dakr@redhat.com>
> > Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> > ---
> > rust/bindings/bindings_helper.h | 2 +
> > rust/kernel/drm/device.rs | 87 +++++++++
> > rust/kernel/drm/drv.rs | 318 ++++++++++++++++++++++++++++++++
> > rust/kernel/drm/mod.rs | 2 +
> > 4 files changed, 409 insertions(+)
> > create mode 100644 rust/kernel/drm/device.rs
> > create mode 100644 rust/kernel/drm/drv.rs
>
> [...]
>
> > diff --git a/rust/kernel/drm/drv.rs b/rust/kernel/drm/drv.rs
> > new file mode 100644
> > index 000000000000..5dd8f3f8df7c
> > --- /dev/null
> > +++ b/rust/kernel/drm/drv.rs
> > @@ -0,0 +1,318 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR MIT
> > +
> > +//! DRM driver core.
> > +//!
> > +//! C header: [`include/linux/drm/drm_drv.h`](../../../../include/linux/drm/drm_drv.h)
> > +
> > +use crate::{
> > + alloc::flags::*,
> > + bindings, device, drm,
> > + error::code::*,
> > + error::{Error, Result},
> > + prelude::*,
> > + private::Sealed,
> > + str::CStr,
> > + types::{ARef, ForeignOwnable},
> > + ThisModule,
> > +};
> > +use core::{
> > + marker::{PhantomData, PhantomPinned},
> > + pin::Pin,
> > +};
> > +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;
>
> This is missing an entry for DRIVER_GEM_GPUVA. And some others perhaps.
> I suppose some are legacy which won't be needed any time soon if ever.
> Not sure if you intend for this to be complete, or you are just adding
> what you are using? Only FEAT_GEM is used by nova ATM.
Good catch, I think we should add all of them, except the legacy ones. If no one
disagrees, I will fix this in v2.
>
> Rob
>
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [RFC PATCH 3/8] rust: drm: Add Device and Driver abstractions
2024-05-21 21:23 ` Rob Herring
2024-05-27 19:26 ` Danilo Krummrich
@ 2024-06-09 5:15 ` Asahi Lina
2024-06-09 14:18 ` Danilo Krummrich
2024-06-11 15:46 ` Rob Herring
1 sibling, 2 replies; 53+ messages in thread
From: Asahi Lina @ 2024-06-09 5:15 UTC (permalink / raw)
To: Rob Herring, Danilo Krummrich
Cc: maarten.lankhorst, mripard, tzimmermann, airlied, daniel, ojeda,
alex.gaynor, wedsonaf, boqun.feng, gary, bjorn3_gh, benno.lossin,
a.hindborg, aliceryhl, fujita.tomonori, pstanner, ajanulgu, lyude,
gregkh, rust-for-linux, dri-devel, nouveau
On 5/22/24 6:23 AM, Rob Herring wrote:
> On Mon, May 20, 2024 at 07:20:50PM +0200, Danilo Krummrich wrote:
>> From: Asahi Lina <lina@asahilina.net>
>>
>> Add abstractions for DRM drivers and devices. These go together in one
>> commit since both are fairly tightly coupled types.
>>
>> A few things have been stubbed out, to be implemented as further bits of
>> the DRM subsystem are introduced.
>>
>> Signed-off-by: Asahi Lina <lina@asahilina.net>
>> Co-developed-by: Danilo Krummrich <dakr@redhat.com>
>> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
>> ---
>> rust/bindings/bindings_helper.h | 2 +
>> rust/kernel/drm/device.rs | 87 +++++++++
>> rust/kernel/drm/drv.rs | 318 ++++++++++++++++++++++++++++++++
>> rust/kernel/drm/mod.rs | 2 +
>> 4 files changed, 409 insertions(+)
>> create mode 100644 rust/kernel/drm/device.rs
>> create mode 100644 rust/kernel/drm/drv.rs
>
> [...]
>
>> diff --git a/rust/kernel/drm/drv.rs b/rust/kernel/drm/drv.rs
>> new file mode 100644
>> index 000000000000..5dd8f3f8df7c
>> --- /dev/null
>> +++ b/rust/kernel/drm/drv.rs
>> @@ -0,0 +1,318 @@
>> +// SPDX-License-Identifier: GPL-2.0 OR MIT
>> +
>> +//! DRM driver core.
>> +//!
>> +//! C header: [`include/linux/drm/drm_drv.h`](../../../../include/linux/drm/drm_drv.h)
>> +
>> +use crate::{
>> + alloc::flags::*,
>> + bindings, device, drm,
>> + error::code::*,
>> + error::{Error, Result},
>> + prelude::*,
>> + private::Sealed,
>> + str::CStr,
>> + types::{ARef, ForeignOwnable},
>> + ThisModule,
>> +};
>> +use core::{
>> + marker::{PhantomData, PhantomPinned},
>> + pin::Pin,
>> +};
>> +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;
>
> This is missing an entry for DRIVER_GEM_GPUVA. And some others perhaps.
> I suppose some are legacy which won't be needed any time soon if ever.
> Not sure if you intend for this to be complete, or you are just adding
> what you are using? Only FEAT_GEM is used by nova ATM.
>
This was developed before DRIVER_GEM_GPUVA existed ^^
I have this in my branch since I'm using the GPUVA manager now. Danilo,
what tree are you using for this submission? It would be good to
coordinate this and try to keep the WIP branches from diverging too much...
That said, I don't think there's reason to support all features unless
we expect new drivers to actually use them. The goal of the abstractions
is to serve the drivers that will use them, and to evolve together with
them and any newer drivers, not to attempt to be feature-complete from
the get go (it's very difficult to evaluate an abstraction if it has no
users!). In general my approach when writing them was to abstract what I
need and add "obvious" extra trivial features that didn't require much
thought even if I wasn't using them, but otherwise not attempt to
comprehensively cover everything.
~~ Lina
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [RFC PATCH 3/8] rust: drm: Add Device and Driver abstractions
2024-06-09 5:15 ` Asahi Lina
@ 2024-06-09 14:18 ` Danilo Krummrich
2024-06-11 15:46 ` Rob Herring
1 sibling, 0 replies; 53+ messages in thread
From: Danilo Krummrich @ 2024-06-09 14:18 UTC (permalink / raw)
To: Asahi Lina
Cc: Rob Herring, maarten.lankhorst, mripard, tzimmermann, airlied,
daniel, ojeda, alex.gaynor, wedsonaf, boqun.feng, gary, bjorn3_gh,
benno.lossin, a.hindborg, aliceryhl, fujita.tomonori, pstanner,
ajanulgu, lyude, gregkh, rust-for-linux, dri-devel, nouveau
Hi Lina,
Welcome back!
On Sun, Jun 09, 2024 at 02:15:57PM +0900, Asahi Lina wrote:
>
>
> On 5/22/24 6:23 AM, Rob Herring wrote:
> > On Mon, May 20, 2024 at 07:20:50PM +0200, Danilo Krummrich wrote:
> >> From: Asahi Lina <lina@asahilina.net>
> > This is missing an entry for DRIVER_GEM_GPUVA. And some others perhaps.
> > I suppose some are legacy which won't be needed any time soon if ever.
> > Not sure if you intend for this to be complete, or you are just adding
> > what you are using? Only FEAT_GEM is used by nova ATM.
> >
>
> This was developed before DRIVER_GEM_GPUVA existed ^^
>
> I have this in my branch since I'm using the GPUVA manager now. Danilo,
> what tree are you using for this submission? It would be good to
> coordinate this and try to keep the WIP branches from diverging too much...
Trying to prevent things from diverging was one of my main objectives when I
started those efforts a couple of month ago (I also sent you a mail for that
purpose back then).
I am maintaining a couple of branches:
In the RfL repository [1]:
- staging/rust-device [2] (Device / driver, devres infrastructure)
- staging/dev [3] (common branch containing all the Rust infrastructure I'm
currently upstreaming, including PCI and firmware abstractions)
In the drm-misc repository [4]:
- topic/rust-drm [5] (all the DRM abstractions)
In the Nova repository [6]
- nova-next [7] (the Nova stub driver making use of everything)
I regularly rebase those branches and keep them up to date with improvements and
feedback from the mailing list.
The device / driver and PCI abstractions are getting most of my attention
currently, but there are some recent changes (besides some minor ones) on the
DRM abstractions I plan to send in a v2 as well:
- static const allocation of driver and fops structures
- rework of the `Registration` type to use `Devres` and combine the new() and
register() methods to register in new() already
There is also some more information regarding those branches in the cover
letters of the series and the links included there.
[1] https://github.com/Rust-for-Linux/linux
[2] https://github.com/Rust-for-Linux/linux/tree/staging/rust-device
[3] https://github.com/Rust-for-Linux/linux/tree/staging/dev
[4] https://gitlab.freedesktop.org/drm/misc/kernel
[5] https://gitlab.freedesktop.org/drm/misc/kernel/-/tree/topic/rust-drm
[6] https://gitlab.freedesktop.org/drm/nova
[7] https://gitlab.freedesktop.org/drm/nova/-/tree/nova-next
>
> That said, I don't think there's reason to support all features unless
> we expect new drivers to actually use them. The goal of the abstractions
> is to serve the drivers that will use them, and to evolve together with
> them and any newer drivers, not to attempt to be feature-complete from
> the get go (it's very difficult to evaluate an abstraction if it has no
> users!). In general my approach when writing them was to abstract what I
> need and add "obvious" extra trivial features that didn't require much
> thought even if I wasn't using them, but otherwise not attempt to
> comprehensively cover everything.
Fully agree, I think the point here only was whether we want to list all the
feature flags in the abstraction already. Which I think is something we can do.
However, I'm also find with only listing the ones we actually use and keep
adding further ones subsequently. It just shouldn't be random.
- Danilo
>
> ~~ Lina
>
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [RFC PATCH 3/8] rust: drm: Add Device and Driver abstractions
2024-06-09 5:15 ` Asahi Lina
2024-06-09 14:18 ` Danilo Krummrich
@ 2024-06-11 15:46 ` Rob Herring
1 sibling, 0 replies; 53+ messages in thread
From: Rob Herring @ 2024-06-11 15:46 UTC (permalink / raw)
To: Asahi Lina
Cc: Danilo Krummrich, maarten.lankhorst, mripard, tzimmermann,
airlied, daniel, ojeda, alex.gaynor, wedsonaf, boqun.feng, gary,
bjorn3_gh, benno.lossin, a.hindborg, aliceryhl, fujita.tomonori,
pstanner, ajanulgu, lyude, gregkh, rust-for-linux, dri-devel,
nouveau
On Sat, Jun 8, 2024 at 11:16 PM Asahi Lina <lina@asahilina.net> wrote:
>
>
>
> On 5/22/24 6:23 AM, Rob Herring wrote:
> > On Mon, May 20, 2024 at 07:20:50PM +0200, Danilo Krummrich wrote:
> >> From: Asahi Lina <lina@asahilina.net>
> >>
> >> Add abstractions for DRM drivers and devices. These go together in one
> >> commit since both are fairly tightly coupled types.
> >>
> >> A few things have been stubbed out, to be implemented as further bits of
> >> the DRM subsystem are introduced.
> >>
> >> Signed-off-by: Asahi Lina <lina@asahilina.net>
> >> Co-developed-by: Danilo Krummrich <dakr@redhat.com>
> >> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> >> ---
> >> rust/bindings/bindings_helper.h | 2 +
> >> rust/kernel/drm/device.rs | 87 +++++++++
> >> rust/kernel/drm/drv.rs | 318 ++++++++++++++++++++++++++++++++
> >> rust/kernel/drm/mod.rs | 2 +
> >> 4 files changed, 409 insertions(+)
> >> create mode 100644 rust/kernel/drm/device.rs
> >> create mode 100644 rust/kernel/drm/drv.rs
> >
> > [...]
> >
> >> diff --git a/rust/kernel/drm/drv.rs b/rust/kernel/drm/drv.rs
> >> new file mode 100644
> >> index 000000000000..5dd8f3f8df7c
> >> --- /dev/null
> >> +++ b/rust/kernel/drm/drv.rs
> >> @@ -0,0 +1,318 @@
> >> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> >> +
> >> +//! DRM driver core.
> >> +//!
> >> +//! C header: [`include/linux/drm/drm_drv.h`](../../../../include/linux/drm/drm_drv.h)
> >> +
> >> +use crate::{
> >> + alloc::flags::*,
> >> + bindings, device, drm,
> >> + error::code::*,
> >> + error::{Error, Result},
> >> + prelude::*,
> >> + private::Sealed,
> >> + str::CStr,
> >> + types::{ARef, ForeignOwnable},
> >> + ThisModule,
> >> +};
> >> +use core::{
> >> + marker::{PhantomData, PhantomPinned},
> >> + pin::Pin,
> >> +};
> >> +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;
> >
> > This is missing an entry for DRIVER_GEM_GPUVA. And some others perhaps.
> > I suppose some are legacy which won't be needed any time soon if ever.
> > Not sure if you intend for this to be complete, or you are just adding
> > what you are using? Only FEAT_GEM is used by nova ATM.
> >
>
> This was developed before DRIVER_GEM_GPUVA existed ^^
>
> I have this in my branch since I'm using the GPUVA manager now.
TBC, I'm using it as well, not just providing drive-by comments. I'm
starting to look at converting panthor to rust.
> Danilo,
> what tree are you using for this submission? It would be good to
> coordinate this and try to keep the WIP branches from diverging too much...
Yes, please!
Besides asahi of course, there's several people[1][2][3] using the
platform and DT abstractions and we're all rebasing those. I'd
volunteer to maintain, but I don't know rust well enough yet to even
be dangerous... :)
Rob
[1] https://lore.kernel.org/all/cover.1717750631.git.viresh.kumar@linaro.org/
[2] https://lore.kernel.org/all/20240322221305.1403600-1-lyude@redhat.com/
[3] https://lore.kernel.org/all/CAPFo5VLoYGq_OgC5dqcueTyymuSCsLpjasLZnKO1jpY6gV7s2g@mail.gmail.com/
^ permalink raw reply [flat|nested] 53+ messages in thread
* [RFC PATCH 4/8] rust: drm: implement `AsRef` for DRM device
2024-05-20 17:20 [RFC PATCH 0/8] [RFC] DRM Rust abstractions and Nova Danilo Krummrich
` (2 preceding siblings ...)
2024-05-20 17:20 ` [RFC PATCH 3/8] rust: drm: Add Device and Driver abstractions Danilo Krummrich
@ 2024-05-20 17:20 ` Danilo Krummrich
2024-05-20 17:24 ` Danilo Krummrich
` (5 subsequent siblings)
9 siblings, 0 replies; 53+ messages in thread
From: Danilo Krummrich @ 2024-05-20 17:20 UTC (permalink / raw)
To: maarten.lankhorst, mripard, tzimmermann, airlied, daniel, ojeda,
alex.gaynor, wedsonaf, boqun.feng, gary, bjorn3_gh, benno.lossin,
a.hindborg, aliceryhl, fujita.tomonori, lina, pstanner, ajanulgu,
lyude, gregkh
Cc: rust-for-linux, dri-devel, nouveau, Danilo Krummrich
Implement `AsRef<device::Device>` for `drm::device::Device` such that
`dev_*` print macros can be used conveniently.
Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
rust/kernel/drm/device.rs | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
index f72bab8dd42d..aef947893dab 100644
--- a/rust/kernel/drm/device.rs
+++ b/rust/kernel/drm/device.rs
@@ -79,6 +79,14 @@ unsafe fn dec_ref(obj: NonNull<Self>) {
}
}
+impl<T: drm::drv::Driver> AsRef<device::Device> for Device<T> {
+ fn as_ref(&self) -> &device::Device {
+ // SAFETY: `bindings::drm_device::dev` is valid as long as the DRM device itself is valid,
+ // which is guaranteed by the type invariant.
+ unsafe { device::Device::as_ref((*self.as_raw()).dev) }
+ }
+}
+
// SAFETY: `Device` only holds a pointer to a C device, which is safe to be used from any thread.
unsafe impl<T: drm::drv::Driver> Send for Device<T> {}
--
2.45.1
^ permalink raw reply related [flat|nested] 53+ messages in thread* [RFC PATCH 4/8] rust: drm: implement `AsRef` for DRM device
2024-05-20 17:20 [RFC PATCH 0/8] [RFC] DRM Rust abstractions and Nova Danilo Krummrich
` (3 preceding siblings ...)
2024-05-20 17:20 ` [RFC PATCH 4/8] rust: drm: implement `AsRef` for DRM device Danilo Krummrich
@ 2024-05-20 17:24 ` Danilo Krummrich
2024-05-20 17:24 ` [RFC PATCH 5/8] rust: drm: file: Add File abstraction Danilo Krummrich
` (4 subsequent siblings)
9 siblings, 0 replies; 53+ messages in thread
From: Danilo Krummrich @ 2024-05-20 17:24 UTC (permalink / raw)
To: maarten.lankhorst, mripard, tzimmermann, airlied, daniel, ojeda,
alex.gaynor, wedsonaf, boqun.feng, gary, bjorn3_gh, benno.lossin,
a.hindborg, aliceryhl, fujita.tomonori, lina, pstanner, ajanulgu,
lyude, gregkh
Cc: rust-for-linux, dri-devel, nouveau, Danilo Krummrich
Implement `AsRef<device::Device>` for `drm::device::Device` such that
`dev_*` print macros can be used conveniently.
Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
rust/kernel/drm/device.rs | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
index f72bab8dd42d..aef947893dab 100644
--- a/rust/kernel/drm/device.rs
+++ b/rust/kernel/drm/device.rs
@@ -79,6 +79,14 @@ unsafe fn dec_ref(obj: NonNull<Self>) {
}
}
+impl<T: drm::drv::Driver> AsRef<device::Device> for Device<T> {
+ fn as_ref(&self) -> &device::Device {
+ // SAFETY: `bindings::drm_device::dev` is valid as long as the DRM device itself is valid,
+ // which is guaranteed by the type invariant.
+ unsafe { device::Device::as_ref((*self.as_raw()).dev) }
+ }
+}
+
// SAFETY: `Device` only holds a pointer to a C device, which is safe to be used from any thread.
unsafe impl<T: drm::drv::Driver> Send for Device<T> {}
--
2.45.1
^ permalink raw reply related [flat|nested] 53+ messages in thread* [RFC PATCH 5/8] rust: drm: file: Add File abstraction
2024-05-20 17:20 [RFC PATCH 0/8] [RFC] DRM Rust abstractions and Nova Danilo Krummrich
` (4 preceding siblings ...)
2024-05-20 17:24 ` Danilo Krummrich
@ 2024-05-20 17:24 ` Danilo Krummrich
2024-05-20 17:24 ` [RFC PATCH 6/8] rust: drm: gem: Add GEM object abstraction Danilo Krummrich
` (3 subsequent siblings)
9 siblings, 0 replies; 53+ messages in thread
From: Danilo Krummrich @ 2024-05-20 17:24 UTC (permalink / raw)
To: maarten.lankhorst, mripard, tzimmermann, airlied, daniel, ojeda,
alex.gaynor, wedsonaf, boqun.feng, gary, bjorn3_gh, benno.lossin,
a.hindborg, aliceryhl, fujita.tomonori, lina, pstanner, ajanulgu,
lyude, gregkh
Cc: rust-for-linux, dri-devel, nouveau, Danilo Krummrich
From: Asahi Lina <lina@asahilina.net>
A DRM File is the DRM counterpart to a kernel file structure,
representing an open DRM file descriptor. Add a Rust abstraction to
allow drivers to implement their own File types that implement the
DriverFile trait.
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 | 7 +-
rust/kernel/drm/file.rs | 113 ++++++++++++++++++++++++++++++++
rust/kernel/drm/mod.rs | 1 +
4 files changed, 120 insertions(+), 2 deletions(-)
create mode 100644 rust/kernel/drm/file.rs
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 831fbfe03a47..c591811ccb67 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -8,6 +8,7 @@
#include <drm/drm_device.h>
#include <drm/drm_drv.h>
+#include <drm/drm_file.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
index 5dd8f3f8df7c..c5a63663ea21 100644
--- a/rust/kernel/drm/drv.rs
+++ b/rust/kernel/drm/drv.rs
@@ -131,6 +131,9 @@ pub trait Driver {
/// Should be either `drm::gem::Object<T>` or `drm::gem::shmem::Object<T>`.
type Object: AllocImpl;
+ /// The type used to represent a DRM File (client)
+ type File: drm::file::DriverFile;
+
/// Driver metadata
const INFO: DriverInfo;
@@ -200,8 +203,8 @@ macro_rules! drm_device_register {
impl<T: Driver> Registration<T> {
const VTABLE: bindings::drm_driver = drm_legacy_fields! {
load: None,
- open: None, // TODO: File abstraction
- postclose: None, // TODO: File abstraction
+ open: Some(drm::file::open_callback::<T::File>),
+ postclose: Some(drm::file::postclose_callback::<T::File>),
lastclose: None,
unload: None,
release: None,
diff --git a/rust/kernel/drm/file.rs b/rust/kernel/drm/file.rs
new file mode 100644
index 000000000000..a25b8c61f4ee
--- /dev/null
+++ b/rust/kernel/drm/file.rs
@@ -0,0 +1,113 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+
+//! DRM File objects.
+//!
+//! C header: [`include/linux/drm/drm_file.h`](../../../../include/linux/drm/drm_file.h)
+
+use crate::{bindings, drm, error::Result};
+use alloc::boxed::Box;
+use core::marker::PhantomData;
+use core::pin::Pin;
+
+/// Trait that must be implemented by DRM drivers to represent a DRM File (a client instance).
+pub trait DriverFile {
+ /// The parent `Driver` implementation for this `DriverFile`.
+ type Driver: drm::drv::Driver;
+
+ /// Open a new file (called when a client opens the DRM device).
+ fn open(device: &drm::device::Device<Self::Driver>) -> Result<Pin<Box<Self>>>;
+}
+
+/// An open DRM File.
+///
+/// # Invariants
+/// `raw` is a valid pointer to a `drm_file` struct.
+#[repr(transparent)]
+pub struct File<T: DriverFile> {
+ raw: *mut bindings::drm_file,
+ _p: PhantomData<T>,
+}
+
+pub(super) unsafe extern "C" fn open_callback<T: DriverFile>(
+ raw_dev: *mut bindings::drm_device,
+ raw_file: *mut bindings::drm_file,
+) -> core::ffi::c_int {
+ let drm = unsafe { drm::device::Device::borrow(raw_dev) };
+ // SAFETY: This reference won't escape this function
+ let file = unsafe { &mut *raw_file };
+
+ let inner = match T::open(drm) {
+ Err(e) => {
+ return e.to_errno();
+ }
+ Ok(i) => i,
+ };
+
+ // SAFETY: This pointer is treated as pinned, and the Drop guarantee is upheld below.
+ file.driver_priv = Box::into_raw(unsafe { Pin::into_inner_unchecked(inner) }) as *mut _;
+
+ 0
+}
+
+pub(super) unsafe extern "C" fn postclose_callback<T: DriverFile>(
+ _dev: *mut bindings::drm_device,
+ raw_file: *mut bindings::drm_file,
+) {
+ // SAFETY: This reference won't escape this function
+ let file = unsafe { &*raw_file };
+
+ // Drop the DriverFile
+ unsafe {
+ let _ = Box::from_raw(file.driver_priv as *mut T);
+ };
+}
+
+impl<T: DriverFile> File<T> {
+ // Not intended to be called externally, except via declare_drm_ioctls!()
+ #[doc(hidden)]
+ pub unsafe fn from_raw(raw_file: *mut bindings::drm_file) -> File<T> {
+ File {
+ raw: raw_file,
+ _p: PhantomData,
+ }
+ }
+
+ #[allow(dead_code)]
+ /// Return the raw pointer to the underlying `drm_file`.
+ pub(super) fn raw(&self) -> *const bindings::drm_file {
+ self.raw
+ }
+
+ /// Return an immutable reference to the raw `drm_file` structure.
+ pub(super) fn file(&self) -> &bindings::drm_file {
+ unsafe { &*self.raw }
+ }
+
+ /// Return a pinned reference to the driver file structure.
+ pub fn inner(&self) -> Pin<&T> {
+ unsafe { Pin::new_unchecked(&*(self.file().driver_priv as *const T)) }
+ }
+}
+
+impl<T: DriverFile> crate::private::Sealed for File<T> {}
+
+/// Generic trait to allow users that don't care about driver specifics to accept any File<T>.
+///
+/// # Safety
+/// Must only be implemented for File<T> and return the pointer, following the normal invariants
+/// of that type.
+pub unsafe trait GenericFile: crate::private::Sealed {
+ /// Returns the raw const pointer to the `struct drm_file`
+ fn raw(&self) -> *const bindings::drm_file;
+ /// Returns the raw mut pointer to the `struct drm_file`
+ fn raw_mut(&mut self) -> *mut bindings::drm_file;
+}
+
+unsafe impl<T: DriverFile> GenericFile for File<T> {
+ fn raw(&self) -> *const bindings::drm_file {
+ self.raw
+ }
+ fn raw_mut(&mut self) -> *mut bindings::drm_file {
+ self.raw
+ }
+}
diff --git a/rust/kernel/drm/mod.rs b/rust/kernel/drm/mod.rs
index 69376b3c6db9..a767942d0b52 100644
--- a/rust/kernel/drm/mod.rs
+++ b/rust/kernel/drm/mod.rs
@@ -4,4 +4,5 @@
pub mod device;
pub mod drv;
+pub mod file;
pub mod ioctl;
--
2.45.1
^ permalink raw reply related [flat|nested] 53+ messages in thread* [RFC PATCH 6/8] rust: drm: gem: Add GEM object abstraction
2024-05-20 17:20 [RFC PATCH 0/8] [RFC] DRM Rust abstractions and Nova Danilo Krummrich
` (5 preceding siblings ...)
2024-05-20 17:24 ` [RFC PATCH 5/8] rust: drm: file: Add File abstraction Danilo Krummrich
@ 2024-05-20 17:24 ` Danilo Krummrich
2024-06-06 15:26 ` Daniel Almeida
2024-05-20 17:24 ` [RFC PATCH 7/8] rust: add firmware abstractions Danilo Krummrich
` (2 subsequent siblings)
9 siblings, 1 reply; 53+ messages in thread
From: Danilo Krummrich @ 2024-05-20 17:24 UTC (permalink / raw)
To: maarten.lankhorst, mripard, tzimmermann, airlied, daniel, ojeda,
alex.gaynor, wedsonaf, boqun.feng, gary, bjorn3_gh, benno.lossin,
a.hindborg, aliceryhl, fujita.tomonori, lina, pstanner, ajanulgu,
lyude, gregkh
Cc: rust-for-linux, dri-devel, nouveau, Danilo Krummrich
From: Asahi Lina <lina@asahilina.net>
The DRM GEM subsystem is the DRM memory management subsystem used by
most modern drivers. Add a Rust abstraction to allow Rust DRM driver
implementations to use it.
Signed-off-by: Asahi Lina <lina@asahilina.net>
Co-developed-by: Danilo Krummrich <dakr@redhat.com>
Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
rust/bindings/bindings_helper.h | 1 +
rust/helpers.c | 23 ++
rust/kernel/drm/drv.rs | 4 +-
rust/kernel/drm/gem/mod.rs | 406 ++++++++++++++++++++++++++++++++
rust/kernel/drm/mod.rs | 1 +
5 files changed, 433 insertions(+), 2 deletions(-)
create mode 100644 rust/kernel/drm/gem/mod.rs
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index c591811ccb67..b245db8d5a87 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -9,6 +9,7 @@
#include <drm/drm_device.h>
#include <drm/drm_drv.h>
#include <drm/drm_file.h>
+#include <drm/drm_gem.h>
#include <drm/drm_ioctl.h>
#include <kunit/test.h>
#include <linux/errname.h>
diff --git a/rust/helpers.c b/rust/helpers.c
index dc2405772b1a..30e86bf00337 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -20,6 +20,7 @@
* Sorted alphabetically.
*/
+#include <drm/drm_gem.h>
#include <kunit/test-bug.h>
#include <linux/bug.h>
#include <linux/build_bug.h>
@@ -302,6 +303,28 @@ u64 rust_helper_pci_resource_len(struct pci_dev *pdev, int barnr)
return pci_resource_len(pdev, barnr);
}
+#ifdef CONFIG_DRM
+
+void rust_helper_drm_gem_object_get(struct drm_gem_object *obj)
+{
+ drm_gem_object_get(obj);
+}
+EXPORT_SYMBOL_GPL(rust_helper_drm_gem_object_get);
+
+void rust_helper_drm_gem_object_put(struct drm_gem_object *obj)
+{
+ drm_gem_object_put(obj);
+}
+EXPORT_SYMBOL_GPL(rust_helper_drm_gem_object_put);
+
+__u64 rust_helper_drm_vma_node_offset_addr(struct drm_vma_offset_node *node)
+{
+ return drm_vma_node_offset_addr(node);
+}
+EXPORT_SYMBOL_GPL(rust_helper_drm_vma_node_offset_addr);
+
+#endif
+
/*
* `bindgen` binds the C `size_t` type as the Rust `usize` type, so we can
* use it in contexts where Rust expects a `usize` like slice (array) indices.
diff --git a/rust/kernel/drm/drv.rs b/rust/kernel/drm/drv.rs
index c5a63663ea21..063b420f57e5 100644
--- a/rust/kernel/drm/drv.rs
+++ b/rust/kernel/drm/drv.rs
@@ -113,7 +113,7 @@ pub struct AllocOps {
}
/// Trait for memory manager implementations. Implemented internally.
-pub trait AllocImpl: Sealed {
+pub trait AllocImpl: Sealed + drm::gem::IntoGEMObject {
/// The C callback operations for this memory manager.
const ALLOC_OPS: AllocOps;
}
@@ -243,7 +243,7 @@ pub fn new(parent: &device::Device) -> Result<Self> {
drm: drm::device::Device::new(parent, &vtable)?,
registered: false,
vtable,
- fops: Default::default(), // TODO: GEM abstraction
+ fops: drm::gem::create_fops(),
_pin: PhantomPinned,
_p: PhantomData,
})
diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs
new file mode 100644
index 000000000000..4cd85d5f1df8
--- /dev/null
+++ b/rust/kernel/drm/gem/mod.rs
@@ -0,0 +1,406 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+
+//! DRM GEM API
+//!
+//! C header: [`include/linux/drm/drm_gem.h`](../../../../include/linux/drm/drm_gem.h)
+
+use alloc::boxed::Box;
+
+use crate::{
+ alloc::flags::*,
+ bindings,
+ drm::{device, drv, file},
+ error::{to_result, Result},
+ prelude::*,
+};
+use core::{marker::PhantomPinned, mem, ops::Deref, ops::DerefMut};
+
+/// GEM object functions, which must be implemented by drivers.
+pub trait BaseDriverObject<T: BaseObject>: Sync + Send + Sized {
+ /// Create a new driver data object for a GEM object of a given size.
+ fn new(dev: &device::Device<T::Driver>, size: usize) -> impl PinInit<Self, Error>;
+
+ /// Open a new handle to an existing object, associated with a File.
+ fn open(
+ _obj: &<<T as IntoGEMObject>::Driver as drv::Driver>::Object,
+ _file: &file::File<<<T as IntoGEMObject>::Driver as drv::Driver>::File>,
+ ) -> Result {
+ Ok(())
+ }
+
+ /// Close a handle to an existing object, associated with a File.
+ fn close(
+ _obj: &<<T as IntoGEMObject>::Driver as drv::Driver>::Object,
+ _file: &file::File<<<T as IntoGEMObject>::Driver as drv::Driver>::File>,
+ ) {
+ }
+}
+
+/// Trait that represents a GEM object subtype
+pub trait IntoGEMObject: Sized + crate::private::Sealed {
+ /// Owning driver for this type
+ type Driver: drv::Driver;
+
+ /// Returns a reference to the raw `drm_gem_object` structure, which must be valid as long as
+ /// this owning object is valid.
+ fn gem_obj(&self) -> &bindings::drm_gem_object;
+
+ /// Converts a pointer to a `drm_gem_object` into a pointer to this type.
+ fn from_gem_obj(obj: *mut bindings::drm_gem_object) -> *mut Self;
+}
+
+/// Trait which must be implemented by drivers using base GEM objects.
+pub trait DriverObject: BaseDriverObject<Object<Self>> {
+ /// Parent `Driver` for this object.
+ type Driver: drv::Driver;
+}
+
+unsafe extern "C" fn free_callback<T: DriverObject>(obj: *mut bindings::drm_gem_object) {
+ // SAFETY: All of our objects are Object<T>.
+ let this = unsafe { crate::container_of!(obj, Object<T>, obj) } as *mut Object<T>;
+
+ // SAFETY: The pointer we got has to be valid
+ unsafe { bindings::drm_gem_object_release(obj) };
+
+ // SAFETY: All of our objects are allocated via Box<>, and we're in the
+ // free callback which guarantees this object has zero remaining references,
+ // so we can drop it
+ unsafe {
+ let _ = Box::from_raw(this);
+ };
+}
+
+unsafe extern "C" fn open_callback<T: BaseDriverObject<U>, U: BaseObject>(
+ raw_obj: *mut bindings::drm_gem_object,
+ raw_file: *mut bindings::drm_file,
+) -> core::ffi::c_int {
+ // SAFETY: The pointer we got has to be valid.
+ let file = unsafe {
+ file::File::<<<U as IntoGEMObject>::Driver as drv::Driver>::File>::from_raw(raw_file)
+ };
+ let obj =
+ <<<U as IntoGEMObject>::Driver as drv::Driver>::Object as IntoGEMObject>::from_gem_obj(
+ raw_obj,
+ );
+
+ // SAFETY: from_gem_obj() returns a valid pointer as long as the type is
+ // correct and the raw_obj we got is valid.
+ match T::open(unsafe { &*obj }, &file) {
+ Err(e) => e.to_errno(),
+ Ok(()) => 0,
+ }
+}
+
+unsafe extern "C" fn close_callback<T: BaseDriverObject<U>, U: BaseObject>(
+ raw_obj: *mut bindings::drm_gem_object,
+ raw_file: *mut bindings::drm_file,
+) {
+ // SAFETY: The pointer we got has to be valid.
+ let file = unsafe {
+ file::File::<<<U as IntoGEMObject>::Driver as drv::Driver>::File>::from_raw(raw_file)
+ };
+ let obj =
+ <<<U as IntoGEMObject>::Driver as drv::Driver>::Object as IntoGEMObject>::from_gem_obj(
+ raw_obj,
+ );
+
+ // SAFETY: from_gem_obj() returns a valid pointer as long as the type is
+ // correct and the raw_obj we got is valid.
+ T::close(unsafe { &*obj }, &file);
+}
+
+impl<T: DriverObject> IntoGEMObject for Object<T> {
+ type Driver = T::Driver;
+
+ fn gem_obj(&self) -> &bindings::drm_gem_object {
+ &self.obj
+ }
+
+ fn from_gem_obj(obj: *mut bindings::drm_gem_object) -> *mut Object<T> {
+ unsafe { crate::container_of!(obj, Object<T>, obj) as *mut Object<T> }
+ }
+}
+
+/// Base operations shared by all GEM object classes
+pub trait BaseObject: IntoGEMObject {
+ /// Returns the size of the object in bytes.
+ fn size(&self) -> usize {
+ self.gem_obj().size
+ }
+
+ /// Creates a new reference to the object.
+ fn reference(&self) -> ObjectRef<Self> {
+ // SAFETY: Having a reference to an Object implies holding a GEM reference
+ unsafe {
+ bindings::drm_gem_object_get(self.gem_obj() as *const _ as *mut _);
+ }
+ ObjectRef {
+ ptr: self as *const _,
+ }
+ }
+
+ /// Creates a new handle for the object associated with a given `File`
+ /// (or returns an existing one).
+ fn create_handle(
+ &self,
+ file: &file::File<<<Self as IntoGEMObject>::Driver as drv::Driver>::File>,
+ ) -> Result<u32> {
+ let mut handle: u32 = 0;
+ // SAFETY: The arguments are all valid per the type invariants.
+ to_result(unsafe {
+ bindings::drm_gem_handle_create(
+ file.raw() as *mut _,
+ self.gem_obj() as *const _ as *mut _,
+ &mut handle,
+ )
+ })?;
+ Ok(handle)
+ }
+
+ /// Looks up an object by its handle for a given `File`.
+ fn lookup_handle(
+ file: &file::File<<<Self as IntoGEMObject>::Driver as drv::Driver>::File>,
+ handle: u32,
+ ) -> Result<ObjectRef<Self>> {
+ // SAFETY: The arguments are all valid per the type invariants.
+ let ptr = unsafe { bindings::drm_gem_object_lookup(file.raw() as *mut _, handle) };
+
+ if ptr.is_null() {
+ Err(ENOENT)
+ } else {
+ Ok(ObjectRef {
+ ptr: ptr as *const _,
+ })
+ }
+ }
+
+ /// Creates an mmap offset to map the object from userspace.
+ fn create_mmap_offset(&self) -> Result<u64> {
+ // SAFETY: The arguments are valid per the type invariant.
+ to_result(unsafe {
+ bindings::drm_gem_create_mmap_offset(self.gem_obj() as *const _ as *mut _)
+ })?;
+ Ok(unsafe {
+ bindings::drm_vma_node_offset_addr(&self.gem_obj().vma_node as *const _ as *mut _)
+ })
+ }
+}
+
+impl<T: IntoGEMObject> BaseObject for T {}
+
+/// A base GEM object.
+#[repr(C)]
+#[pin_data]
+pub struct Object<T: DriverObject> {
+ obj: bindings::drm_gem_object,
+ // The DRM core ensures the Device exists as long as its objects exist, so we don't need to
+ // manage the reference count here.
+ dev: *const bindings::drm_device,
+ #[pin]
+ inner: T,
+ #[pin]
+ _p: PhantomPinned,
+}
+
+// SAFETY: This struct is safe to zero-initialize
+unsafe impl init::Zeroable for bindings::drm_gem_object {}
+
+impl<T: DriverObject> Object<T> {
+ /// The size of this object's structure.
+ pub const SIZE: usize = mem::size_of::<Self>();
+
+ const OBJECT_FUNCS: bindings::drm_gem_object_funcs = bindings::drm_gem_object_funcs {
+ free: Some(free_callback::<T>),
+ open: Some(open_callback::<T, Object<T>>),
+ close: Some(close_callback::<T, Object<T>>),
+ print_info: None,
+ export: None,
+ pin: None,
+ unpin: None,
+ get_sg_table: None,
+ vmap: None,
+ vunmap: None,
+ mmap: None,
+ status: None,
+ vm_ops: core::ptr::null_mut(),
+ evict: None,
+ rss: None,
+ };
+
+ /// Create a new GEM object.
+ pub fn new(dev: &device::Device<T::Driver>, size: usize) -> Result<Pin<UniqueObjectRef<Self>>> {
+ let obj: Pin<Box<Self>> = Box::pin_init(
+ try_pin_init!(Self {
+ // SAFETY: This struct is expected to be zero-initialized
+ obj: bindings::drm_gem_object {
+ funcs: &Self::OBJECT_FUNCS,
+ ..Default::default()
+ },
+ inner <- T::new(dev, size),
+ // SAFETY: The drm subsystem guarantees that the drm_device will live as long as
+ // the GEM object lives, so we can conjure a reference out of thin air.
+ dev: dev.as_raw(),
+ _p: PhantomPinned
+ }),
+ GFP_KERNEL,
+ )?;
+
+ to_result(unsafe {
+ bindings::drm_gem_object_init(dev.as_raw(), &obj.obj as *const _ as *mut _, size)
+ })?;
+
+ // SAFETY: We never move out of self
+ let obj_ref = unsafe {
+ Pin::new_unchecked(UniqueObjectRef {
+ // SAFETY: We never move out of the Box
+ ptr: Box::leak(Pin::into_inner_unchecked(obj)),
+ _p: PhantomPinned,
+ })
+ };
+
+ Ok(obj_ref)
+ }
+
+ /// Returns the `Device` that owns this GEM object.
+ pub fn dev(&self) -> &device::Device<T::Driver> {
+ // SAFETY: The drm subsystem guarantees that the drm_device will live as long as
+ // the GEM object lives, so we can just borrow from the raw pointer.
+ unsafe { device::Device::borrow(self.dev) }
+ }
+}
+
+impl<T: DriverObject> crate::private::Sealed for Object<T> {}
+
+impl<T: DriverObject> Deref for Object<T> {
+ type Target = T;
+
+ fn deref(&self) -> &Self::Target {
+ &self.inner
+ }
+}
+
+impl<T: DriverObject> DerefMut for Object<T> {
+ fn deref_mut(&mut self) -> &mut Self::Target {
+ &mut self.inner
+ }
+}
+
+impl<T: DriverObject> drv::AllocImpl for Object<T> {
+ const ALLOC_OPS: drv::AllocOps = drv::AllocOps {
+ gem_create_object: None,
+ prime_handle_to_fd: None,
+ prime_fd_to_handle: None,
+ gem_prime_import: None,
+ gem_prime_import_sg_table: None,
+ dumb_create: None,
+ dumb_map_offset: None,
+ };
+}
+
+/// A reference-counted shared reference to a base GEM object.
+pub struct ObjectRef<T: IntoGEMObject> {
+ // Invariant: the pointer is valid and initialized, and this ObjectRef owns a reference to it.
+ ptr: *const T,
+}
+
+impl<T: IntoGEMObject> ObjectRef<T> {
+ /// Downgrade this reference to a shared reference.
+ pub fn from_pinned_unique(pin: Pin<UniqueObjectRef<T>>) -> Self {
+ // SAFETY: A (shared) `ObjectRef` doesn't need to be pinned, since it doesn't allow us to
+ // optain a mutable reference.
+ let uq = unsafe { Pin::into_inner_unchecked(pin) };
+
+ uq.into_ref()
+ }
+}
+
+/// SAFETY: GEM object references are safe to share between threads.
+unsafe impl<T: IntoGEMObject> Send for ObjectRef<T> {}
+unsafe impl<T: IntoGEMObject> Sync for ObjectRef<T> {}
+
+impl<T: IntoGEMObject> Clone for ObjectRef<T> {
+ fn clone(&self) -> Self {
+ self.reference()
+ }
+}
+
+impl<T: IntoGEMObject> Drop for ObjectRef<T> {
+ fn drop(&mut self) {
+ // SAFETY: Having an ObjectRef implies holding a GEM reference.
+ // The free callback will take care of deallocation.
+ unsafe {
+ bindings::drm_gem_object_put((*self.ptr).gem_obj() as *const _ as *mut _);
+ }
+ }
+}
+
+impl<T: IntoGEMObject> Deref for ObjectRef<T> {
+ type Target = T;
+
+ fn deref(&self) -> &Self::Target {
+ // SAFETY: The pointer is valid per the invariant
+ unsafe { &*self.ptr }
+ }
+}
+
+/// A unique reference to a base GEM object.
+pub struct UniqueObjectRef<T: IntoGEMObject> {
+ // Invariant: the pointer is valid and initialized, and this ObjectRef owns the only reference
+ // to it.
+ ptr: *mut T,
+ _p: PhantomPinned,
+}
+
+impl<T: IntoGEMObject> UniqueObjectRef<T> {
+ /// Downgrade this reference to a shared reference.
+ pub fn into_ref(self) -> ObjectRef<T> {
+ let ptr = self.ptr as *const _;
+ core::mem::forget(self);
+
+ ObjectRef { ptr }
+ }
+}
+
+impl<T: IntoGEMObject> Drop for UniqueObjectRef<T> {
+ fn drop(&mut self) {
+ // SAFETY: Having a UniqueObjectRef implies holding a GEM
+ // reference. The free callback will take care of deallocation.
+ unsafe {
+ bindings::drm_gem_object_put((*self.ptr).gem_obj() as *const _ as *mut _);
+ }
+ }
+}
+
+impl<T: IntoGEMObject> Deref for UniqueObjectRef<T> {
+ type Target = T;
+
+ fn deref(&self) -> &Self::Target {
+ // SAFETY: The pointer is valid per the invariant
+ unsafe { &*self.ptr }
+ }
+}
+
+impl<T: IntoGEMObject> DerefMut for UniqueObjectRef<T> {
+ fn deref_mut(&mut self) -> &mut Self::Target {
+ // SAFETY: The pointer is valid per the invariant
+ unsafe { &mut *self.ptr }
+ }
+}
+
+pub(super) fn create_fops() -> bindings::file_operations {
+ bindings::file_operations {
+ owner: core::ptr::null_mut(),
+ open: Some(bindings::drm_open),
+ release: Some(bindings::drm_release),
+ unlocked_ioctl: Some(bindings::drm_ioctl),
+ #[cfg(CONFIG_COMPAT)]
+ compat_ioctl: Some(bindings::drm_compat_ioctl),
+ #[cfg(not(CONFIG_COMPAT))]
+ compat_ioctl: None,
+ poll: Some(bindings::drm_poll),
+ read: Some(bindings::drm_read),
+ llseek: Some(bindings::noop_llseek),
+ mmap: Some(bindings::drm_gem_mmap),
+ ..Default::default()
+ }
+}
diff --git a/rust/kernel/drm/mod.rs b/rust/kernel/drm/mod.rs
index a767942d0b52..c44760a1332f 100644
--- a/rust/kernel/drm/mod.rs
+++ b/rust/kernel/drm/mod.rs
@@ -5,4 +5,5 @@
pub mod device;
pub mod drv;
pub mod file;
+pub mod gem;
pub mod ioctl;
--
2.45.1
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [RFC PATCH 6/8] rust: drm: gem: Add GEM object abstraction
2024-05-20 17:24 ` [RFC PATCH 6/8] rust: drm: gem: Add GEM object abstraction Danilo Krummrich
@ 2024-06-06 15:26 ` Daniel Almeida
0 siblings, 0 replies; 53+ messages in thread
From: Daniel Almeida @ 2024-06-06 15:26 UTC (permalink / raw)
To: Danilo Krummrich
Cc: maarten.lankhorst, mripard, tzimmermann, airlied, Daniel Vetter,
ojeda, alex.gaynor, Wedson Almeida Filho, Boqun Feng, gary,
bjorn3_gh, benno.lossin, a.hindborg, aliceryhl, fujita.tomonori,
lina, pstanner, ajanulgu, lyude, gregkh, rust-for-linux,
dri-devel, nouveau
Hi Danilo, Lina
> On 20 May 2024, at 14:24, Danilo Krummrich <dakr@redhat.com> wrote:
>
> From: Asahi Lina <lina@asahilina.net>
>
> The DRM GEM subsystem is the DRM memory management subsystem used by
> most modern drivers. Add a Rust abstraction to allow Rust DRM driver
> implementations to use it.
>
> Signed-off-by: Asahi Lina <lina@asahilina.net>
> Co-developed-by: Danilo Krummrich <dakr@redhat.com>
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> ---
> rust/bindings/bindings_helper.h | 1 +
> rust/helpers.c | 23 ++
> rust/kernel/drm/drv.rs | 4 +-
> rust/kernel/drm/gem/mod.rs | 406 ++++++++++++++++++++++++++++++++
> rust/kernel/drm/mod.rs | 1 +
> 5 files changed, 433 insertions(+), 2 deletions(-)
> create mode 100644 rust/kernel/drm/gem/mod.rs
>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index c591811ccb67..b245db8d5a87 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -9,6 +9,7 @@
> #include <drm/drm_device.h>
> #include <drm/drm_drv.h>
> #include <drm/drm_file.h>
> +#include <drm/drm_gem.h>
> #include <drm/drm_ioctl.h>
> #include <kunit/test.h>
> #include <linux/errname.h>
> diff --git a/rust/helpers.c b/rust/helpers.c
> index dc2405772b1a..30e86bf00337 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -20,6 +20,7 @@
> * Sorted alphabetically.
> */
>
> +#include <drm/drm_gem.h>
> #include <kunit/test-bug.h>
> #include <linux/bug.h>
> #include <linux/build_bug.h>
> @@ -302,6 +303,28 @@ u64 rust_helper_pci_resource_len(struct pci_dev *pdev, int barnr)
> return pci_resource_len(pdev, barnr);
> }
>
> +#ifdef CONFIG_DRM
> +
> +void rust_helper_drm_gem_object_get(struct drm_gem_object *obj)
> +{
> + drm_gem_object_get(obj);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_drm_gem_object_get);
> +
> +void rust_helper_drm_gem_object_put(struct drm_gem_object *obj)
> +{
> + drm_gem_object_put(obj);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_drm_gem_object_put);
> +
> +__u64 rust_helper_drm_vma_node_offset_addr(struct drm_vma_offset_node *node)
> +{
> + return drm_vma_node_offset_addr(node);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_drm_vma_node_offset_addr);
> +
> +#endif
> +
> /*
> * `bindgen` binds the C `size_t` type as the Rust `usize` type, so we can
> * use it in contexts where Rust expects a `usize` like slice (array) indices.
> diff --git a/rust/kernel/drm/drv.rs b/rust/kernel/drm/drv.rs
> index c5a63663ea21..063b420f57e5 100644
> --- a/rust/kernel/drm/drv.rs
> +++ b/rust/kernel/drm/drv.rs
> @@ -113,7 +113,7 @@ pub struct AllocOps {
> }
>
> /// Trait for memory manager implementations. Implemented internally.
> -pub trait AllocImpl: Sealed {
> +pub trait AllocImpl: Sealed + drm::gem::IntoGEMObject {
> /// The C callback operations for this memory manager.
> const ALLOC_OPS: AllocOps;
> }
> @@ -243,7 +243,7 @@ pub fn new(parent: &device::Device) -> Result<Self> {
> drm: drm::device::Device::new(parent, &vtable)?,
> registered: false,
> vtable,
> - fops: Default::default(), // TODO: GEM abstraction
> + fops: drm::gem::create_fops(),
> _pin: PhantomPinned,
> _p: PhantomData,
> })
> diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs
> new file mode 100644
> index 000000000000..4cd85d5f1df8
> --- /dev/null
> +++ b/rust/kernel/drm/gem/mod.rs
> @@ -0,0 +1,406 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +
> +//! DRM GEM API
> +//!
> +//! C header: [`include/linux/drm/drm_gem.h`](../../../../include/linux/drm/drm_gem.h)
> +
> +use alloc::boxed::Box;
> +
> +use crate::{
> + alloc::flags::*,
> + bindings,
> + drm::{device, drv, file},
> + error::{to_result, Result},
> + prelude::*,
> +};
> +use core::{marker::PhantomPinned, mem, ops::Deref, ops::DerefMut};
> +
> +/// GEM object functions, which must be implemented by drivers.
> +pub trait BaseDriverObject<T: BaseObject>: Sync + Send + Sized {
> + /// Create a new driver data object for a GEM object of a given size.
> + fn new(dev: &device::Device<T::Driver>, size: usize) -> impl PinInit<Self, Error>;
> +
> + /// Open a new handle to an existing object, associated with a File.
> + fn open(
> + _obj: &<<T as IntoGEMObject>::Driver as drv::Driver>::Object,
> + _file: &file::File<<<T as IntoGEMObject>::Driver as drv::Driver>::File>,
I wonder if these can be put into a type alias? As in,
type File<T: IntoGEMObject> = file::File<<<T as IntoGEMObject>::Driver as drv::Driver>::File>;
etc
That would clean things up a bit.
> + ) -> Result {
> + Ok(())
> + }
> +
> + /// Close a handle to an existing object, associated with a File.
> + fn close(
> + _obj: &<<T as IntoGEMObject>::Driver as drv::Driver>::Object,
> + _file: &file::File<<<T as IntoGEMObject>::Driver as drv::Driver>::File>,
> + ) {
> + }
> +}
> +
> +/// Trait that represents a GEM object subtype
> +pub trait IntoGEMObject: Sized + crate::private::Sealed {
> + /// Owning driver for this type
> + type Driver: drv::Driver;
> +
> + /// Returns a reference to the raw `drm_gem_object` structure, which must be valid as long as
> + /// this owning object is valid.
> + fn gem_obj(&self) -> &bindings::drm_gem_object;
> +
> + /// Converts a pointer to a `drm_gem_object` into a pointer to this type.
> + fn from_gem_obj(obj: *mut bindings::drm_gem_object) -> *mut Self;
> +}
> +
> +/// Trait which must be implemented by drivers using base GEM objects.
> +pub trait DriverObject: BaseDriverObject<Object<Self>> {
> + /// Parent `Driver` for this object.
> + type Driver: drv::Driver;
> +}
> +
> +unsafe extern "C" fn free_callback<T: DriverObject>(obj: *mut bindings::drm_gem_object) {
> + // SAFETY: All of our objects are Object<T>.
> + let this = unsafe { crate::container_of!(obj, Object<T>, obj) } as *mut Object<T>;
> +
> + // SAFETY: The pointer we got has to be valid
> + unsafe { bindings::drm_gem_object_release(obj) };
> +
> + // SAFETY: All of our objects are allocated via Box<>, and we're in the
> + // free callback which guarantees this object has zero remaining references,
> + // so we can drop it
> + unsafe {
> + let _ = Box::from_raw(this);
> + };
> +}
> +
> +unsafe extern "C" fn open_callback<T: BaseDriverObject<U>, U: BaseObject>(
> + raw_obj: *mut bindings::drm_gem_object,
> + raw_file: *mut bindings::drm_file,
> +) -> core::ffi::c_int {
> + // SAFETY: The pointer we got has to be valid.
> + let file = unsafe {
> + file::File::<<<U as IntoGEMObject>::Driver as drv::Driver>::File>::from_raw(raw_file)
> + };
> + let obj =
> + <<<U as IntoGEMObject>::Driver as drv::Driver>::Object as IntoGEMObject>::from_gem_obj(
> + raw_obj,
> + );
> +
> + // SAFETY: from_gem_obj() returns a valid pointer as long as the type is
> + // correct and the raw_obj we got is valid.
> + match T::open(unsafe { &*obj }, &file) {
> + Err(e) => e.to_errno(),
> + Ok(()) => 0,
> + }
> +}
> +
> +unsafe extern "C" fn close_callback<T: BaseDriverObject<U>, U: BaseObject>(
> + raw_obj: *mut bindings::drm_gem_object,
> + raw_file: *mut bindings::drm_file,
> +) {
> + // SAFETY: The pointer we got has to be valid.
> + let file = unsafe {
> + file::File::<<<U as IntoGEMObject>::Driver as drv::Driver>::File>::from_raw(raw_file)
> + };
> + let obj =
> + <<<U as IntoGEMObject>::Driver as drv::Driver>::Object as IntoGEMObject>::from_gem_obj(
> + raw_obj,
> + );
> +
> + // SAFETY: from_gem_obj() returns a valid pointer as long as the type is
> + // correct and the raw_obj we got is valid.
> + T::close(unsafe { &*obj }, &file);
> +}
> +
> +impl<T: DriverObject> IntoGEMObject for Object<T> {
> + type Driver = T::Driver;
> +
> + fn gem_obj(&self) -> &bindings::drm_gem_object {
> + &self.obj
> + }
> +
> + fn from_gem_obj(obj: *mut bindings::drm_gem_object) -> *mut Object<T> {
> + unsafe { crate::container_of!(obj, Object<T>, obj) as *mut Object<T> }
> + }
> +}
> +
> +/// Base operations shared by all GEM object classes
> +pub trait BaseObject: IntoGEMObject {
> + /// Returns the size of the object in bytes.
> + fn size(&self) -> usize {
> + self.gem_obj().size
> + }
> +
> + /// Creates a new reference to the object.
> + fn reference(&self) -> ObjectRef<Self> {
> + // SAFETY: Having a reference to an Object implies holding a GEM reference
> + unsafe {
> + bindings::drm_gem_object_get(self.gem_obj() as *const _ as *mut _);
> + }
> + ObjectRef {
> + ptr: self as *const _,
> + }
> + }
> +
> + /// Creates a new handle for the object associated with a given `File`
> + /// (or returns an existing one).
> + fn create_handle(
> + &self,
> + file: &file::File<<<Self as IntoGEMObject>::Driver as drv::Driver>::File>,
> + ) -> Result<u32> {
> + let mut handle: u32 = 0;
> + // SAFETY: The arguments are all valid per the type invariants.
> + to_result(unsafe {
> + bindings::drm_gem_handle_create(
> + file.raw() as *mut _,
> + self.gem_obj() as *const _ as *mut _,
> + &mut handle,
> + )
> + })?;
> + Ok(handle)
> + }
> +
> + /// Looks up an object by its handle for a given `File`.
> + fn lookup_handle(
> + file: &file::File<<<Self as IntoGEMObject>::Driver as drv::Driver>::File>,
> + handle: u32,
> + ) -> Result<ObjectRef<Self>> {
> + // SAFETY: The arguments are all valid per the type invariants.
> + let ptr = unsafe { bindings::drm_gem_object_lookup(file.raw() as *mut _, handle) };
> +
> + if ptr.is_null() {
> + Err(ENOENT)
> + } else {
> + Ok(ObjectRef {
> + ptr: ptr as *const _,
> + })
> + }
> + }
> +
> + /// Creates an mmap offset to map the object from userspace.
> + fn create_mmap_offset(&self) -> Result<u64> {
> + // SAFETY: The arguments are valid per the type invariant.
> + to_result(unsafe {
> + bindings::drm_gem_create_mmap_offset(self.gem_obj() as *const _ as *mut _)
> + })?;
> + Ok(unsafe {
> + bindings::drm_vma_node_offset_addr(&self.gem_obj().vma_node as *const _ as *mut _)
> + })
> + }
> +}
> +
> +impl<T: IntoGEMObject> BaseObject for T {}
> +
> +/// A base GEM object.
> +#[repr(C)]
> +#[pin_data]
> +pub struct Object<T: DriverObject> {
> + obj: bindings::drm_gem_object,
> + // The DRM core ensures the Device exists as long as its objects exist, so we don't need to
> + // manage the reference count here.
> + dev: *const bindings::drm_device,
> + #[pin]
> + inner: T,
> + #[pin]
> + _p: PhantomPinned,
> +}
A bit minor, but why is PhantomPinned structurally pinned here?
Maybe Benno can also chime in, but, at a first glance, I do not see any reason for a marker type to be pinned.
> +
> +// SAFETY: This struct is safe to zero-initialize
> +unsafe impl init::Zeroable for bindings::drm_gem_object {}
> +
> +impl<T: DriverObject> Object<T> {
> + /// The size of this object's structure.
> + pub const SIZE: usize = mem::size_of::<Self>();
> +
> + const OBJECT_FUNCS: bindings::drm_gem_object_funcs = bindings::drm_gem_object_funcs {
> + free: Some(free_callback::<T>),
> + open: Some(open_callback::<T, Object<T>>),
> + close: Some(close_callback::<T, Object<T>>),
> + print_info: None,
> + export: None,
> + pin: None,
> + unpin: None,
> + get_sg_table: None,
> + vmap: None,
> + vunmap: None,
> + mmap: None,
> + status: None,
> + vm_ops: core::ptr::null_mut(),
> + evict: None,
> + rss: None,
> + };
> +
> + /// Create a new GEM object.
> + pub fn new(dev: &device::Device<T::Driver>, size: usize) -> Result<Pin<UniqueObjectRef<Self>>> {
> + let obj: Pin<Box<Self>> = Box::pin_init(
> + try_pin_init!(Self {
> + // SAFETY: This struct is expected to be zero-initialized
> + obj: bindings::drm_gem_object {
> + funcs: &Self::OBJECT_FUNCS,
> + ..Default::default()
> + },
> + inner <- T::new(dev, size),
> + // SAFETY: The drm subsystem guarantees that the drm_device will live as long as
> + // the GEM object lives, so we can conjure a reference out of thin air.
> + dev: dev.as_raw(),
> + _p: PhantomPinned
> + }),
> + GFP_KERNEL,
> + )?;
> +
> + to_result(unsafe {
> + bindings::drm_gem_object_init(dev.as_raw(), &obj.obj as *const _ as *mut _, size)
> + })?;
> +
> + // SAFETY: We never move out of self
> + let obj_ref = unsafe {
> + Pin::new_unchecked(UniqueObjectRef {
> + // SAFETY: We never move out of the Box
> + ptr: Box::leak(Pin::into_inner_unchecked(obj)),
> + _p: PhantomPinned,
> + })
> + };
> +
> + Ok(obj_ref)
> + }
> +
> + /// Returns the `Device` that owns this GEM object.
> + pub fn dev(&self) -> &device::Device<T::Driver> {
> + // SAFETY: The drm subsystem guarantees that the drm_device will live as long as
> + // the GEM object lives, so we can just borrow from the raw pointer.
> + unsafe { device::Device::borrow(self.dev) }
> + }
> +}
> +
> +impl<T: DriverObject> crate::private::Sealed for Object<T> {}
> +
> +impl<T: DriverObject> Deref for Object<T> {
> + type Target = T;
> +
> + fn deref(&self) -> &Self::Target {
> + &self.inner
> + }
> +}
> +
> +impl<T: DriverObject> DerefMut for Object<T> {
> + fn deref_mut(&mut self) -> &mut Self::Target {
> + &mut self.inner
> + }
> +}
> +
> +impl<T: DriverObject> drv::AllocImpl for Object<T> {
> + const ALLOC_OPS: drv::AllocOps = drv::AllocOps {
> + gem_create_object: None,
> + prime_handle_to_fd: None,
> + prime_fd_to_handle: None,
> + gem_prime_import: None,
> + gem_prime_import_sg_table: None,
> + dumb_create: None,
> + dumb_map_offset: None,
> + };
> +}
> +
> +/// A reference-counted shared reference to a base GEM object.
> +pub struct ObjectRef<T: IntoGEMObject> {
> + // Invariant: the pointer is valid and initialized, and this ObjectRef owns a reference to it.
> + ptr: *const T,
> +}
> +
> +impl<T: IntoGEMObject> ObjectRef<T> {
> + /// Downgrade this reference to a shared reference.
> + pub fn from_pinned_unique(pin: Pin<UniqueObjectRef<T>>) -> Self {
> + // SAFETY: A (shared) `ObjectRef` doesn't need to be pinned, since it doesn't allow us to
> + // optain a mutable reference.
> + let uq = unsafe { Pin::into_inner_unchecked(pin) };
> +
> + uq.into_ref()
> + }
> +}
> +
> +/// SAFETY: GEM object references are safe to share between threads.
> +unsafe impl<T: IntoGEMObject> Send for ObjectRef<T> {}
> +unsafe impl<T: IntoGEMObject> Sync for ObjectRef<T> {}
> +
> +impl<T: IntoGEMObject> Clone for ObjectRef<T> {
> + fn clone(&self) -> Self {
> + self.reference()
> + }
> +}
> +
> +impl<T: IntoGEMObject> Drop for ObjectRef<T> {
> + fn drop(&mut self) {
> + // SAFETY: Having an ObjectRef implies holding a GEM reference.
> + // The free callback will take care of deallocation.
> + unsafe {
> + bindings::drm_gem_object_put((*self.ptr).gem_obj() as *const _ as *mut _);
> + }
> + }
> +}
> +
> +impl<T: IntoGEMObject> Deref for ObjectRef<T> {
> + type Target = T;
> +
> + fn deref(&self) -> &Self::Target {
> + // SAFETY: The pointer is valid per the invariant
> + unsafe { &*self.ptr }
> + }
> +}
> +
> +/// A unique reference to a base GEM object.
> +pub struct UniqueObjectRef<T: IntoGEMObject> {
> + // Invariant: the pointer is valid and initialized, and this ObjectRef owns the only reference
> + // to it.
> + ptr: *mut T,
> + _p: PhantomPinned,
> +}
> +
> +impl<T: IntoGEMObject> UniqueObjectRef<T> {
> + /// Downgrade this reference to a shared reference.
> + pub fn into_ref(self) -> ObjectRef<T> {
> + let ptr = self.ptr as *const _;
> + core::mem::forget(self);
> +
> + ObjectRef { ptr }
> + }
> +}
> +
> +impl<T: IntoGEMObject> Drop for UniqueObjectRef<T> {
> + fn drop(&mut self) {
> + // SAFETY: Having a UniqueObjectRef implies holding a GEM
> + // reference. The free callback will take care of deallocation.
> + unsafe {
> + bindings::drm_gem_object_put((*self.ptr).gem_obj() as *const _ as *mut _);
> + }
> + }
> +}
> +
> +impl<T: IntoGEMObject> Deref for UniqueObjectRef<T> {
> + type Target = T;
> +
> + fn deref(&self) -> &Self::Target {
> + // SAFETY: The pointer is valid per the invariant
> + unsafe { &*self.ptr }
> + }
> +}
> +
> +impl<T: IntoGEMObject> DerefMut for UniqueObjectRef<T> {
> + fn deref_mut(&mut self) -> &mut Self::Target {
> + // SAFETY: The pointer is valid per the invariant
> + unsafe { &mut *self.ptr }
> + }
> +}
> +
> +pub(super) fn create_fops() -> bindings::file_operations {
> + bindings::file_operations {
> + owner: core::ptr::null_mut(),
> + open: Some(bindings::drm_open),
> + release: Some(bindings::drm_release),
> + unlocked_ioctl: Some(bindings::drm_ioctl),
> + #[cfg(CONFIG_COMPAT)]
> + compat_ioctl: Some(bindings::drm_compat_ioctl),
> + #[cfg(not(CONFIG_COMPAT))]
> + compat_ioctl: None,
> + poll: Some(bindings::drm_poll),
> + read: Some(bindings::drm_read),
> + llseek: Some(bindings::noop_llseek),
> + mmap: Some(bindings::drm_gem_mmap),
> + ..Default::default()
> + }
> +}
> diff --git a/rust/kernel/drm/mod.rs b/rust/kernel/drm/mod.rs
> index a767942d0b52..c44760a1332f 100644
> --- a/rust/kernel/drm/mod.rs
> +++ b/rust/kernel/drm/mod.rs
> @@ -5,4 +5,5 @@
> pub mod device;
> pub mod drv;
> pub mod file;
> +pub mod gem;
> pub mod ioctl;
> --
> 2.45.1
This looks good to me,
Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
^ permalink raw reply [flat|nested] 53+ messages in thread
* [RFC PATCH 7/8] rust: add firmware abstractions
2024-05-20 17:20 [RFC PATCH 0/8] [RFC] DRM Rust abstractions and Nova Danilo Krummrich
` (6 preceding siblings ...)
2024-05-20 17:24 ` [RFC PATCH 6/8] rust: drm: gem: Add GEM object abstraction Danilo Krummrich
@ 2024-05-20 17:24 ` Danilo Krummrich
2024-05-21 5:32 ` Zhi Wang
` (2 more replies)
2024-05-20 17:24 ` [RFC PATCH 8/8] nova: add initial driver stub Danilo Krummrich
2024-05-20 17:30 ` Device / Driver and PCI Rust abstractions Danilo Krummrich
9 siblings, 3 replies; 53+ messages in thread
From: Danilo Krummrich @ 2024-05-20 17:24 UTC (permalink / raw)
To: maarten.lankhorst, mripard, tzimmermann, airlied, daniel, ojeda,
alex.gaynor, wedsonaf, boqun.feng, gary, bjorn3_gh, benno.lossin,
a.hindborg, aliceryhl, fujita.tomonori, lina, pstanner, ajanulgu,
lyude, gregkh
Cc: rust-for-linux, dri-devel, nouveau, Danilo Krummrich
Add an abstraction around the kernels firmware API to request firmware
images. The abstraction provides functions to access the firmware
buffer and / or copy it to a new buffer allocated with a given allocator
backend.
The firmware is released once the abstraction is dropped.
Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
rust/bindings/bindings_helper.h | 1 +
rust/kernel/firmware.rs | 74 +++++++++++++++++++++++++++++++++
rust/kernel/lib.rs | 1 +
3 files changed, 76 insertions(+)
create mode 100644 rust/kernel/firmware.rs
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index b245db8d5a87..e4ffc47da5ec 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -14,6 +14,7 @@
#include <kunit/test.h>
#include <linux/errname.h>
#include <linux/ethtool.h>
+#include <linux/firmware.h>
#include <linux/jiffies.h>
#include <linux/mdio.h>
#include <linux/pci.h>
diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
new file mode 100644
index 000000000000..700504fb3c9c
--- /dev/null
+++ b/rust/kernel/firmware.rs
@@ -0,0 +1,74 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Firmware abstraction
+//!
+//! C header: [`include/linux/firmware.h`](../../../../include/linux/firmware.h")
+
+use crate::{bindings, device::Device, error::Error, error::Result, str::CStr, types::Opaque};
+
+/// Abstraction around a C firmware struct.
+///
+/// This is a simple abstraction around the C firmware API. Just like with the C API, firmware can
+/// be requested. Once requested the abstraction provides direct access to the firmware buffer as
+/// `&[u8]`. Alternatively, the firmware can be copied to a new buffer using `Firmware::copy`. The
+/// firmware is released once [`Firmware`] is dropped.
+///
+/// # Examples
+///
+/// ```
+/// let fw = Firmware::request("path/to/firmware.bin", dev.as_ref())?;
+/// driver_load_firmware(fw.data());
+/// ```
+pub struct Firmware(Opaque<*const bindings::firmware>);
+
+impl Firmware {
+ /// Send a firmware request and wait for it. See also `bindings::request_firmware`.
+ pub fn request(name: &CStr, dev: &Device) -> Result<Self> {
+ let fw = Opaque::uninit();
+
+ let ret = unsafe { bindings::request_firmware(fw.get(), name.as_char_ptr(), dev.as_raw()) };
+ if ret != 0 {
+ return Err(Error::from_errno(ret));
+ }
+
+ Ok(Firmware(fw))
+ }
+
+ /// Send a request for an optional fw module. See also `bindings::request_firmware_nowarn`.
+ pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Self> {
+ let fw = Opaque::uninit();
+
+ let ret = unsafe {
+ bindings::firmware_request_nowarn(fw.get(), name.as_char_ptr(), dev.as_raw())
+ };
+ if ret != 0 {
+ return Err(Error::from_errno(ret));
+ }
+
+ Ok(Firmware(fw))
+ }
+
+ /// Returns the size of the requested firmware in bytes.
+ pub fn size(&self) -> usize {
+ unsafe { (*(*self.0.get())).size }
+ }
+
+ /// Returns the requested firmware as `&[u8]`.
+ pub fn data(&self) -> &[u8] {
+ unsafe { core::slice::from_raw_parts((*(*self.0.get())).data, self.size()) }
+ }
+}
+
+impl Drop for Firmware {
+ fn drop(&mut self) {
+ unsafe { bindings::release_firmware(*self.0.get()) };
+ }
+}
+
+// SAFETY: `Firmware` only holds a pointer to a C firmware struct, which is safe to be used from any
+// thread.
+unsafe impl Send for Firmware {}
+
+// SAFETY: `Firmware` only holds a pointer to a C firmware struct, references to which are safe to
+// be used from any thread.
+unsafe impl Sync for Firmware {}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 6415968ee3b8..ed97d131661a 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -35,6 +35,7 @@
#[cfg(CONFIG_DRM = "y")]
pub mod drm;
pub mod error;
+pub mod firmware;
pub mod init;
pub mod ioctl;
#[cfg(CONFIG_KUNIT)]
--
2.45.1
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-05-20 17:24 ` [RFC PATCH 7/8] rust: add firmware abstractions Danilo Krummrich
@ 2024-05-21 5:32 ` Zhi Wang
2024-05-27 19:18 ` Danilo Krummrich
2024-05-21 23:53 ` FUJITA Tomonori
2024-05-28 12:06 ` Danilo Krummrich
2 siblings, 1 reply; 53+ messages in thread
From: Zhi Wang @ 2024-05-21 5:32 UTC (permalink / raw)
To: Danilo Krummrich
Cc: maarten.lankhorst, mripard, tzimmermann, airlied, daniel, ojeda,
alex.gaynor, wedsonaf, boqun.feng, gary, bjorn3_gh, benno.lossin,
a.hindborg, aliceryhl, fujita.tomonori, lina, pstanner, ajanulgu,
lyude, gregkh, rust-for-linux, dri-devel, nouveau
On Mon, 20 May 2024 19:24:19 +0200
Danilo Krummrich <dakr@redhat.com> wrote:
> Add an abstraction around the kernels firmware API to request firmware
> images. The abstraction provides functions to access the firmware
> buffer and / or copy it to a new buffer allocated with a given
> allocator backend.
>
Was playing with firmware extractions based on this patch.
Unfortunately I ended up with a lot of pointer operations, unsafe
statements.
As we know many vendors have a C headers for the definitions of the
firwmare content, the driver extract the data by applying a struct
pointer on it.
But in rust, I feel it would nice that we can also have a common
firmware extractor for drivers, that can wrap the pointer operations,
take a list of the firmware struct members that converted from C headers
as the input, offer the driver some common ABI methods to query them.
Maybe that would ease the pain a lot.
Thanks,
Zhi.
> The firmware is released once the abstraction is dropped.
>
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> ---
> rust/bindings/bindings_helper.h | 1 +
> rust/kernel/firmware.rs | 74
> +++++++++++++++++++++++++++++++++ rust/kernel/lib.rs |
> 1 + 3 files changed, 76 insertions(+)
> create mode 100644 rust/kernel/firmware.rs
>
> diff --git a/rust/bindings/bindings_helper.h
> b/rust/bindings/bindings_helper.h index b245db8d5a87..e4ffc47da5ec
> 100644 --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -14,6 +14,7 @@
> #include <kunit/test.h>
> #include <linux/errname.h>
> #include <linux/ethtool.h>
> +#include <linux/firmware.h>
> #include <linux/jiffies.h>
> #include <linux/mdio.h>
> #include <linux/pci.h>
> diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> new file mode 100644
> index 000000000000..700504fb3c9c
> --- /dev/null
> +++ b/rust/kernel/firmware.rs
> @@ -0,0 +1,74 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Firmware abstraction
> +//!
> +//! C header:
> [`include/linux/firmware.h`](../../../../include/linux/firmware.h") +
> +use crate::{bindings, device::Device, error::Error, error::Result,
> str::CStr, types::Opaque}; +
> +/// Abstraction around a C firmware struct.
> +///
> +/// This is a simple abstraction around the C firmware API. Just
> like with the C API, firmware can +/// be requested. Once requested
> the abstraction provides direct access to the firmware buffer as +///
> `&[u8]`. Alternatively, the firmware can be copied to a new buffer
> using `Firmware::copy`. The +/// firmware is released once
> [`Firmware`] is dropped. +/// +/// # Examples
> +///
> +/// ```
> +/// let fw = Firmware::request("path/to/firmware.bin",
> dev.as_ref())?; +/// driver_load_firmware(fw.data());
> +/// ```
> +pub struct Firmware(Opaque<*const bindings::firmware>);
> +
> +impl Firmware {
> + /// Send a firmware request and wait for it. See also
> `bindings::request_firmware`.
> + pub fn request(name: &CStr, dev: &Device) -> Result<Self> {
> + let fw = Opaque::uninit();
> +
> + let ret = unsafe { bindings::request_firmware(fw.get(),
> name.as_char_ptr(), dev.as_raw()) };
> + if ret != 0 {
> + return Err(Error::from_errno(ret));
> + }
> +
> + Ok(Firmware(fw))
> + }
> +
> + /// Send a request for an optional fw module. See also
> `bindings::request_firmware_nowarn`.
> + pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Self>
> {
> + let fw = Opaque::uninit();
> +
> + let ret = unsafe {
> + bindings::firmware_request_nowarn(fw.get(),
> name.as_char_ptr(), dev.as_raw())
> + };
> + if ret != 0 {
> + return Err(Error::from_errno(ret));
> + }
> +
> + Ok(Firmware(fw))
> + }
> +
> + /// Returns the size of the requested firmware in bytes.
> + pub fn size(&self) -> usize {
> + unsafe { (*(*self.0.get())).size }
> + }
> +
> + /// Returns the requested firmware as `&[u8]`.
> + pub fn data(&self) -> &[u8] {
> + unsafe {
> core::slice::from_raw_parts((*(*self.0.get())).data, self.size()) }
> + }
> +}
> +
> +impl Drop for Firmware {
> + fn drop(&mut self) {
> + unsafe { bindings::release_firmware(*self.0.get()) };
> + }
> +}
> +
> +// SAFETY: `Firmware` only holds a pointer to a C firmware struct,
> which is safe to be used from any +// thread.
> +unsafe impl Send for Firmware {}
> +
> +// SAFETY: `Firmware` only holds a pointer to a C firmware struct,
> references to which are safe to +// be used from any thread.
> +unsafe impl Sync for Firmware {}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 6415968ee3b8..ed97d131661a 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -35,6 +35,7 @@
> #[cfg(CONFIG_DRM = "y")]
> pub mod drm;
> pub mod error;
> +pub mod firmware;
> pub mod init;
> pub mod ioctl;
> #[cfg(CONFIG_KUNIT)]
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-05-21 5:32 ` Zhi Wang
@ 2024-05-27 19:18 ` Danilo Krummrich
2024-05-28 8:40 ` Zhi Wang
0 siblings, 1 reply; 53+ messages in thread
From: Danilo Krummrich @ 2024-05-27 19:18 UTC (permalink / raw)
To: Zhi Wang
Cc: maarten.lankhorst, mripard, tzimmermann, airlied, daniel, ojeda,
alex.gaynor, wedsonaf, boqun.feng, gary, bjorn3_gh, benno.lossin,
a.hindborg, aliceryhl, fujita.tomonori, lina, pstanner, ajanulgu,
lyude, gregkh, rust-for-linux, dri-devel, nouveau
On Tue, May 21, 2024 at 08:32:31AM +0300, Zhi Wang wrote:
> On Mon, 20 May 2024 19:24:19 +0200
> Danilo Krummrich <dakr@redhat.com> wrote:
>
> > Add an abstraction around the kernels firmware API to request firmware
> > images. The abstraction provides functions to access the firmware
> > buffer and / or copy it to a new buffer allocated with a given
> > allocator backend.
> >
>
> Was playing with firmware extractions based on this patch.
> Unfortunately I ended up with a lot of pointer operations, unsafe
> statements.
>
> As we know many vendors have a C headers for the definitions of the
> firwmare content, the driver extract the data by applying a struct
> pointer on it.
>
> But in rust, I feel it would nice that we can also have a common
> firmware extractor for drivers, that can wrap the pointer operations,
> take a list of the firmware struct members that converted from C headers
> as the input, offer the driver some common ABI methods to query them.
> Maybe that would ease the pain a lot.
So, you mean some abstraction that takes a list of types, offsets in the
firmware and a reference to the firmware itself and provides references to the
corresponding objects?
I agree it might be helpful to have some common infrastructure for this, but the
operations on it would still be unsafe, since ultimately it involves
dereferencing pointers.
>
> Thanks,
> Zhi.
>
> > The firmware is released once the abstraction is dropped.
> >
> > Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> > ---
> > rust/bindings/bindings_helper.h | 1 +
> > rust/kernel/firmware.rs | 74
> > +++++++++++++++++++++++++++++++++ rust/kernel/lib.rs |
> > 1 + 3 files changed, 76 insertions(+)
> > create mode 100644 rust/kernel/firmware.rs
> >
> > diff --git a/rust/bindings/bindings_helper.h
> > b/rust/bindings/bindings_helper.h index b245db8d5a87..e4ffc47da5ec
> > 100644 --- a/rust/bindings/bindings_helper.h
> > +++ b/rust/bindings/bindings_helper.h
> > @@ -14,6 +14,7 @@
> > #include <kunit/test.h>
> > #include <linux/errname.h>
> > #include <linux/ethtool.h>
> > +#include <linux/firmware.h>
> > #include <linux/jiffies.h>
> > #include <linux/mdio.h>
> > #include <linux/pci.h>
> > diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> > new file mode 100644
> > index 000000000000..700504fb3c9c
> > --- /dev/null
> > +++ b/rust/kernel/firmware.rs
> > @@ -0,0 +1,74 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! Firmware abstraction
> > +//!
> > +//! C header:
> > [`include/linux/firmware.h`](../../../../include/linux/firmware.h") +
> > +use crate::{bindings, device::Device, error::Error, error::Result,
> > str::CStr, types::Opaque}; +
> > +/// Abstraction around a C firmware struct.
> > +///
> > +/// This is a simple abstraction around the C firmware API. Just
> > like with the C API, firmware can +/// be requested. Once requested
> > the abstraction provides direct access to the firmware buffer as +///
> > `&[u8]`. Alternatively, the firmware can be copied to a new buffer
> > using `Firmware::copy`. The +/// firmware is released once
> > [`Firmware`] is dropped. +/// +/// # Examples
> > +///
> > +/// ```
> > +/// let fw = Firmware::request("path/to/firmware.bin",
> > dev.as_ref())?; +/// driver_load_firmware(fw.data());
> > +/// ```
> > +pub struct Firmware(Opaque<*const bindings::firmware>);
> > +
> > +impl Firmware {
> > + /// Send a firmware request and wait for it. See also
> > `bindings::request_firmware`.
> > + pub fn request(name: &CStr, dev: &Device) -> Result<Self> {
> > + let fw = Opaque::uninit();
> > +
> > + let ret = unsafe { bindings::request_firmware(fw.get(),
> > name.as_char_ptr(), dev.as_raw()) };
> > + if ret != 0 {
> > + return Err(Error::from_errno(ret));
> > + }
> > +
> > + Ok(Firmware(fw))
> > + }
> > +
> > + /// Send a request for an optional fw module. See also
> > `bindings::request_firmware_nowarn`.
> > + pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Self>
> > {
> > + let fw = Opaque::uninit();
> > +
> > + let ret = unsafe {
> > + bindings::firmware_request_nowarn(fw.get(),
> > name.as_char_ptr(), dev.as_raw())
> > + };
> > + if ret != 0 {
> > + return Err(Error::from_errno(ret));
> > + }
> > +
> > + Ok(Firmware(fw))
> > + }
> > +
> > + /// Returns the size of the requested firmware in bytes.
> > + pub fn size(&self) -> usize {
> > + unsafe { (*(*self.0.get())).size }
> > + }
> > +
> > + /// Returns the requested firmware as `&[u8]`.
> > + pub fn data(&self) -> &[u8] {
> > + unsafe {
> > core::slice::from_raw_parts((*(*self.0.get())).data, self.size()) }
> > + }
> > +}
> > +
> > +impl Drop for Firmware {
> > + fn drop(&mut self) {
> > + unsafe { bindings::release_firmware(*self.0.get()) };
> > + }
> > +}
> > +
> > +// SAFETY: `Firmware` only holds a pointer to a C firmware struct,
> > which is safe to be used from any +// thread.
> > +unsafe impl Send for Firmware {}
> > +
> > +// SAFETY: `Firmware` only holds a pointer to a C firmware struct,
> > references to which are safe to +// be used from any thread.
> > +unsafe impl Sync for Firmware {}
> > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > index 6415968ee3b8..ed97d131661a 100644
> > --- a/rust/kernel/lib.rs
> > +++ b/rust/kernel/lib.rs
> > @@ -35,6 +35,7 @@
> > #[cfg(CONFIG_DRM = "y")]
> > pub mod drm;
> > pub mod error;
> > +pub mod firmware;
> > pub mod init;
> > pub mod ioctl;
> > #[cfg(CONFIG_KUNIT)]
>
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-05-27 19:18 ` Danilo Krummrich
@ 2024-05-28 8:40 ` Zhi Wang
2024-05-28 10:17 ` FUJITA Tomonori
2024-05-28 14:18 ` Danilo Krummrich
0 siblings, 2 replies; 53+ messages in thread
From: Zhi Wang @ 2024-05-28 8:40 UTC (permalink / raw)
To: Danilo Krummrich
Cc: maarten.lankhorst@linux.intel.com, mripard@kernel.org,
tzimmermann@suse.de, airlied@gmail.com, daniel@ffwll.ch,
ojeda@kernel.org, alex.gaynor@gmail.com, wedsonaf@gmail.com,
boqun.feng@gmail.com, gary@garyguo.net, bjorn3_gh@protonmail.com,
benno.lossin@proton.me, a.hindborg@samsung.com,
aliceryhl@google.com, fujita.tomonori@gmail.com,
lina@asahilina.net, pstanner@redhat.com, ajanulgu@redhat.com,
lyude@redhat.com, gregkh@linuxfoundation.org,
rust-for-linux@vger.kernel.org, dri-devel@lists.freedesktop.org,
nouveau@lists.freedesktop.org
On 27/05/2024 22.18, Danilo Krummrich wrote:
> External email: Use caution opening links or attachments
>
>
> On Tue, May 21, 2024 at 08:32:31AM +0300, Zhi Wang wrote:
>> On Mon, 20 May 2024 19:24:19 +0200
>> Danilo Krummrich <dakr@redhat.com> wrote:
>>
>>> Add an abstraction around the kernels firmware API to request firmware
>>> images. The abstraction provides functions to access the firmware
>>> buffer and / or copy it to a new buffer allocated with a given
>>> allocator backend.
>>>
>>
>> Was playing with firmware extractions based on this patch.
>> Unfortunately I ended up with a lot of pointer operations, unsafe
>> statements.
>>
>> As we know many vendors have a C headers for the definitions of the
>> firwmare content, the driver extract the data by applying a struct
>> pointer on it.
>>
>> But in rust, I feel it would nice that we can also have a common
>> firmware extractor for drivers, that can wrap the pointer operations,
>> take a list of the firmware struct members that converted from C headers
>> as the input, offer the driver some common ABI methods to query them.
>> Maybe that would ease the pain a lot.
>
> So, you mean some abstraction that takes a list of types, offsets in the
> firmware and a reference to the firmware itself and provides references to the
> corresponding objects?
>
> I agree it might be helpful to have some common infrastructure for this, but the
> operations on it would still be unsafe, since ultimately it involves
> dereferencing pointers.
>
I think the goal is to 1) concentrate the "unsafe" operations in a
abstraction so the driver doesn't have to write explanation of unsafe
operations here and there, improve the readability of the driver that
interacts with vendor-firmware buffer. 2) ease the driver maintenance
burden.
Some solutions I saw in different projects written in rust for
de-referencing a per-defined struct:
1) Take the vendor firmware buffer as a whole, invent methods like
read/write with offset for operating the buffer.
In this scheme, the driver is responsible for taking care of each data
member in a vendor firmware struct and also its valid offset. The
abstraction only covers the boundary of the whole firmware buffer.
The cons is the readability of the operation of the vendor firmware
buffer in the driver is not good comparing with C code.
Hate to think a lot of xxx = vendor_firmware_struct.read(offset); //
reading item A in the code.
2) Define the firmware struct in rust (might need to find a better way
to handle "union" in the definition of the vendor firmware struct). Use
macros to generate methods of accessing each data member for the vendor
firmware struct.
Then the code in the driver will be like xxx =
vendor_firmware_struct.item_a(); in the driver.
In this scheme, the abstraction and the generated methods covers the
boundary check. The "unsafe" statement can stay in the generated
struct-access methods.
This might even be implemented as a more generic rust feature in the kernel.
The cons is still the driver might need to sync between the C-version
definition and rust-version definition.
3) Also re-using C bindings generation in the kernel came to my mind
when thinking of this problem, since it allows the rust code to access
the C struct, but they don't have the boundary check. Still need another
layer/macros to wrap it.
>>
>> Thanks,
>> Zhi.
>>
>>> The firmware is released once the abstraction is dropped.
>>>
>>> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
>>> ---
>>> rust/bindings/bindings_helper.h | 1 +
>>> rust/kernel/firmware.rs | 74
>>> +++++++++++++++++++++++++++++++++ rust/kernel/lib.rs |
>>> 1 + 3 files changed, 76 insertions(+)
>>> create mode 100644 rust/kernel/firmware.rs
>>>
>>> diff --git a/rust/bindings/bindings_helper.h
>>> b/rust/bindings/bindings_helper.h index b245db8d5a87..e4ffc47da5ec
>>> 100644 --- a/rust/bindings/bindings_helper.h
>>> +++ b/rust/bindings/bindings_helper.h
>>> @@ -14,6 +14,7 @@
>>> #include <kunit/test.h>
>>> #include <linux/errname.h>
>>> #include <linux/ethtool.h>
>>> +#include <linux/firmware.h>
>>> #include <linux/jiffies.h>
>>> #include <linux/mdio.h>
>>> #include <linux/pci.h>
>>> diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
>>> new file mode 100644
>>> index 000000000000..700504fb3c9c
>>> --- /dev/null
>>> +++ b/rust/kernel/firmware.rs
>>> @@ -0,0 +1,74 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +
>>> +//! Firmware abstraction
>>> +//!
>>> +//! C header:
>>> [`include/linux/firmware.h`](../../../../include/linux/firmware.h") +
>>> +use crate::{bindings, device::Device, error::Error, error::Result,
>>> str::CStr, types::Opaque}; +
>>> +/// Abstraction around a C firmware struct.
>>> +///
>>> +/// This is a simple abstraction around the C firmware API. Just
>>> like with the C API, firmware can +/// be requested. Once requested
>>> the abstraction provides direct access to the firmware buffer as +///
>>> `&[u8]`. Alternatively, the firmware can be copied to a new buffer
>>> using `Firmware::copy`. The +/// firmware is released once
>>> [`Firmware`] is dropped. +/// +/// # Examples
>>> +///
>>> +/// ```
>>> +/// let fw = Firmware::request("path/to/firmware.bin",
>>> dev.as_ref())?; +/// driver_load_firmware(fw.data());
>>> +/// ```
>>> +pub struct Firmware(Opaque<*const bindings::firmware>);
>>> +
>>> +impl Firmware {
>>> + /// Send a firmware request and wait for it. See also
>>> `bindings::request_firmware`.
>>> + pub fn request(name: &CStr, dev: &Device) -> Result<Self> {
>>> + let fw = Opaque::uninit();
>>> +
>>> + let ret = unsafe { bindings::request_firmware(fw.get(),
>>> name.as_char_ptr(), dev.as_raw()) };
>>> + if ret != 0 {
>>> + return Err(Error::from_errno(ret));
>>> + }
>>> +
>>> + Ok(Firmware(fw))
>>> + }
>>> +
>>> + /// Send a request for an optional fw module. See also
>>> `bindings::request_firmware_nowarn`.
>>> + pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Self>
>>> {
>>> + let fw = Opaque::uninit();
>>> +
>>> + let ret = unsafe {
>>> + bindings::firmware_request_nowarn(fw.get(),
>>> name.as_char_ptr(), dev.as_raw())
>>> + };
>>> + if ret != 0 {
>>> + return Err(Error::from_errno(ret));
>>> + }
>>> +
>>> + Ok(Firmware(fw))
>>> + }
>>> +
>>> + /// Returns the size of the requested firmware in bytes.
>>> + pub fn size(&self) -> usize {
>>> + unsafe { (*(*self.0.get())).size }
>>> + }
>>> +
>>> + /// Returns the requested firmware as `&[u8]`.
>>> + pub fn data(&self) -> &[u8] {
>>> + unsafe {
>>> core::slice::from_raw_parts((*(*self.0.get())).data, self.size()) }
>>> + }
>>> +}
>>> +
>>> +impl Drop for Firmware {
>>> + fn drop(&mut self) {
>>> + unsafe { bindings::release_firmware(*self.0.get()) };
>>> + }
>>> +}
>>> +
>>> +// SAFETY: `Firmware` only holds a pointer to a C firmware struct,
>>> which is safe to be used from any +// thread.
>>> +unsafe impl Send for Firmware {}
>>> +
>>> +// SAFETY: `Firmware` only holds a pointer to a C firmware struct,
>>> references to which are safe to +// be used from any thread.
>>> +unsafe impl Sync for Firmware {}
>>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>>> index 6415968ee3b8..ed97d131661a 100644
>>> --- a/rust/kernel/lib.rs
>>> +++ b/rust/kernel/lib.rs
>>> @@ -35,6 +35,7 @@
>>> #[cfg(CONFIG_DRM = "y")]
>>> pub mod drm;
>>> pub mod error;
>>> +pub mod firmware;
>>> pub mod init;
>>> pub mod ioctl;
>>> #[cfg(CONFIG_KUNIT)]
>>
>
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-05-28 8:40 ` Zhi Wang
@ 2024-05-28 10:17 ` FUJITA Tomonori
2024-05-28 10:45 ` Zhi Wang
2024-05-28 14:18 ` Danilo Krummrich
1 sibling, 1 reply; 53+ messages in thread
From: FUJITA Tomonori @ 2024-05-28 10:17 UTC (permalink / raw)
To: zhiw
Cc: dakr, maarten.lankhorst, mripard, tzimmermann, airlied, daniel,
ojeda, alex.gaynor, wedsonaf, boqun.feng, gary, bjorn3_gh,
benno.lossin, a.hindborg, aliceryhl, fujita.tomonori, lina,
pstanner, ajanulgu, lyude, gregkh, rust-for-linux, dri-devel,
nouveau
Hi,
On Tue, 28 May 2024 08:40:20 +0000
Zhi Wang <zhiw@nvidia.com> wrote:
> On 27/05/2024 22.18, Danilo Krummrich wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On Tue, May 21, 2024 at 08:32:31AM +0300, Zhi Wang wrote:
>>> On Mon, 20 May 2024 19:24:19 +0200
>>> Danilo Krummrich <dakr@redhat.com> wrote:
>>>
>>>> Add an abstraction around the kernels firmware API to request firmware
>>>> images. The abstraction provides functions to access the firmware
>>>> buffer and / or copy it to a new buffer allocated with a given
>>>> allocator backend.
>>>>
>>>
>>> Was playing with firmware extractions based on this patch.
>>> Unfortunately I ended up with a lot of pointer operations, unsafe
>>> statements.
>>>
>>> As we know many vendors have a C headers for the definitions of the
>>> firwmare content, the driver extract the data by applying a struct
>>> pointer on it.
>>>
>>> But in rust, I feel it would nice that we can also have a common
>>> firmware extractor for drivers, that can wrap the pointer operations,
>>> take a list of the firmware struct members that converted from C headers
>>> as the input, offer the driver some common ABI methods to query them.
>>> Maybe that would ease the pain a lot.
>>
>> So, you mean some abstraction that takes a list of types, offsets in the
>> firmware and a reference to the firmware itself and provides references to the
>> corresponding objects?
>>
>> I agree it might be helpful to have some common infrastructure for this, but the
>> operations on it would still be unsafe, since ultimately it involves
>> dereferencing pointers.
>>
>
> I think the goal is to 1) concentrate the "unsafe" operations in a
> abstraction so the driver doesn't have to write explanation of unsafe
> operations here and there, improve the readability of the driver that
> interacts with vendor-firmware buffer. 2) ease the driver maintenance
> burden.
>
> Some solutions I saw in different projects written in rust for
> de-referencing a per-defined struct:
Aren't there other abstractions that require that functionality, not
just the firmware abstractions?
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-05-28 10:17 ` FUJITA Tomonori
@ 2024-05-28 10:45 ` Zhi Wang
0 siblings, 0 replies; 53+ messages in thread
From: Zhi Wang @ 2024-05-28 10:45 UTC (permalink / raw)
To: FUJITA Tomonori
Cc: dakr@redhat.com, maarten.lankhorst@linux.intel.com,
mripard@kernel.org, tzimmermann@suse.de, airlied@gmail.com,
daniel@ffwll.ch, ojeda@kernel.org, alex.gaynor@gmail.com,
wedsonaf@gmail.com, boqun.feng@gmail.com, gary@garyguo.net,
bjorn3_gh@protonmail.com, benno.lossin@proton.me,
a.hindborg@samsung.com, aliceryhl@google.com, lina@asahilina.net,
pstanner@redhat.com, ajanulgu@redhat.com, lyude@redhat.com,
gregkh@linuxfoundation.org, rust-for-linux@vger.kernel.org,
dri-devel@lists.freedesktop.org, nouveau@lists.freedesktop.org
On 28/05/2024 13.17, FUJITA Tomonori wrote:
> External email: Use caution opening links or attachments
>
>
> Hi,
>
> On Tue, 28 May 2024 08:40:20 +0000
> Zhi Wang <zhiw@nvidia.com> wrote:
>
>> On 27/05/2024 22.18, Danilo Krummrich wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On Tue, May 21, 2024 at 08:32:31AM +0300, Zhi Wang wrote:
>>>> On Mon, 20 May 2024 19:24:19 +0200
>>>> Danilo Krummrich <dakr@redhat.com> wrote:
>>>>
>>>>> Add an abstraction around the kernels firmware API to request firmware
>>>>> images. The abstraction provides functions to access the firmware
>>>>> buffer and / or copy it to a new buffer allocated with a given
>>>>> allocator backend.
>>>>>
>>>>
>>>> Was playing with firmware extractions based on this patch.
>>>> Unfortunately I ended up with a lot of pointer operations, unsafe
>>>> statements.
>>>>
>>>> As we know many vendors have a C headers for the definitions of the
>>>> firwmare content, the driver extract the data by applying a struct
>>>> pointer on it.
>>>>
>>>> But in rust, I feel it would nice that we can also have a common
>>>> firmware extractor for drivers, that can wrap the pointer operations,
>>>> take a list of the firmware struct members that converted from C headers
>>>> as the input, offer the driver some common ABI methods to query them.
>>>> Maybe that would ease the pain a lot.
>>>
>>> So, you mean some abstraction that takes a list of types, offsets in the
>>> firmware and a reference to the firmware itself and provides references to the
>>> corresponding objects?
>>>
>>> I agree it might be helpful to have some common infrastructure for this, but the
>>> operations on it would still be unsafe, since ultimately it involves
>>> dereferencing pointers.
>>>
>>
>> I think the goal is to 1) concentrate the "unsafe" operations in a
>> abstraction so the driver doesn't have to write explanation of unsafe
>> operations here and there, improve the readability of the driver that
>> interacts with vendor-firmware buffer. 2) ease the driver maintenance
>> burden.
>>
>> Some solutions I saw in different projects written in rust for
>> de-referencing a per-defined struct:
>
> Aren't there other abstractions that require that functionality, not
> just the firmware abstractions?
Sure, but they might implement it in a different way due to their
different scale and requirements of maintenance.
I am more interested in what is the better option for firmware
abstractions based on its scale and requirements. 1) how to improve the
readability of the driver, meanwhile keep the operations safe. 2) there
has been C-version vendor-firmware definitions, what would be the best
for the rust driver to leverage it based on the routines that the rust
kernel has already had. 3) how to avoid syncing the headers of vendor
firmware between C and rust version, as the definition can scale up due
to HW generations or ABI changes.
Thanks,
Zhi.
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-05-28 8:40 ` Zhi Wang
2024-05-28 10:17 ` FUJITA Tomonori
@ 2024-05-28 14:18 ` Danilo Krummrich
2024-05-28 21:20 ` Zhi Wang
1 sibling, 1 reply; 53+ messages in thread
From: Danilo Krummrich @ 2024-05-28 14:18 UTC (permalink / raw)
To: Zhi Wang
Cc: maarten.lankhorst@linux.intel.com, mripard@kernel.org,
tzimmermann@suse.de, airlied@gmail.com, daniel@ffwll.ch,
ojeda@kernel.org, alex.gaynor@gmail.com, wedsonaf@gmail.com,
boqun.feng@gmail.com, gary@garyguo.net, bjorn3_gh@protonmail.com,
benno.lossin@proton.me, a.hindborg@samsung.com,
aliceryhl@google.com, fujita.tomonori@gmail.com,
lina@asahilina.net, pstanner@redhat.com, ajanulgu@redhat.com,
lyude@redhat.com, gregkh@linuxfoundation.org,
rust-for-linux@vger.kernel.org, dri-devel@lists.freedesktop.org,
nouveau@lists.freedesktop.org
On Tue, May 28, 2024 at 08:40:20AM +0000, Zhi Wang wrote:
> On 27/05/2024 22.18, Danilo Krummrich wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > On Tue, May 21, 2024 at 08:32:31AM +0300, Zhi Wang wrote:
> >> On Mon, 20 May 2024 19:24:19 +0200
> >> Danilo Krummrich <dakr@redhat.com> wrote:
> >>
> >>> Add an abstraction around the kernels firmware API to request firmware
> >>> images. The abstraction provides functions to access the firmware
> >>> buffer and / or copy it to a new buffer allocated with a given
> >>> allocator backend.
> >>>
> >>
> >> Was playing with firmware extractions based on this patch.
> >> Unfortunately I ended up with a lot of pointer operations, unsafe
> >> statements.
> >>
> >> As we know many vendors have a C headers for the definitions of the
> >> firwmare content, the driver extract the data by applying a struct
> >> pointer on it.
> >>
> >> But in rust, I feel it would nice that we can also have a common
> >> firmware extractor for drivers, that can wrap the pointer operations,
> >> take a list of the firmware struct members that converted from C headers
> >> as the input, offer the driver some common ABI methods to query them.
> >> Maybe that would ease the pain a lot.
> >
> > So, you mean some abstraction that takes a list of types, offsets in the
> > firmware and a reference to the firmware itself and provides references to the
> > corresponding objects?
> >
> > I agree it might be helpful to have some common infrastructure for this, but the
> > operations on it would still be unsafe, since ultimately it involves
> > dereferencing pointers.
> >
>
> I think the goal is to 1) concentrate the "unsafe" operations in a
> abstraction so the driver doesn't have to write explanation of unsafe
> operations here and there, improve the readability of the driver that
> interacts with vendor-firmware buffer. 2) ease the driver maintenance
> burden.
With a generic abstraction, as in 1), at lest some of the abstraction's
functions would be unsafe themselves, since they have to rely on the layout
(or offset) passed by the driver being correct. What if I pass a wrong layout /
offset for a structure that contains a pointer? This might still result in an
invalid pointer dereference. Am I missing something?
>
> Some solutions I saw in different projects written in rust for
> de-referencing a per-defined struct:
>
> 1) Take the vendor firmware buffer as a whole, invent methods like
> read/write with offset for operating the buffer.
>
> In this scheme, the driver is responsible for taking care of each data
> member in a vendor firmware struct and also its valid offset. The
> abstraction only covers the boundary of the whole firmware buffer.
>
> The cons is the readability of the operation of the vendor firmware
> buffer in the driver is not good comparing with C code.
>
> Hate to think a lot of xxx = vendor_firmware_struct.read(offset); //
> reading item A in the code.
>
> 2) Define the firmware struct in rust (might need to find a better way
> to handle "union" in the definition of the vendor firmware struct). Use
> macros to generate methods of accessing each data member for the vendor
> firmware struct.
>
> Then the code in the driver will be like xxx =
> vendor_firmware_struct.item_a(); in the driver.
>
> In this scheme, the abstraction and the generated methods covers the
> boundary check. The "unsafe" statement can stay in the generated
> struct-access methods.
>
> This might even be implemented as a more generic rust feature in the kernel.
This sounds more like a driver specific abstraction to me, which, as you write,
we can probably come up with some macros that help implementing it.
But again, what if the offsets are within the boundary, but still at a wrong
offset? What if the data obtained from a wrong offset leads to other safety
implications when further processing them? I think no generic abstraction can
ever cover the safety parts of this (entirely). I think there are always semanic
parts to this the driver has to cover.
Generally, I think we should aim for some generalization, but I think we should
not expect it to cover all the safety aspects.
>
> The cons is still the driver might need to sync between the C-version
> definition and rust-version definition.
What do you mean with the driver needs to sync between a C and a Rust version of
structure definitions?
>
> 3) Also re-using C bindings generation in the kernel came to my mind
> when thinking of this problem, since it allows the rust code to access
> the C struct, but they don't have the boundary check. Still need another
> layer/macros to wrap it.
I think we should have the structure definitions in Rust directly.
- Danilo
>
>
> >>
> >> Thanks,
> >> Zhi.
> >>
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-05-28 14:18 ` Danilo Krummrich
@ 2024-05-28 21:20 ` Zhi Wang
0 siblings, 0 replies; 53+ messages in thread
From: Zhi Wang @ 2024-05-28 21:20 UTC (permalink / raw)
To: Danilo Krummrich
Cc: maarten.lankhorst@linux.intel.com, mripard@kernel.org,
tzimmermann@suse.de, airlied@gmail.com, daniel@ffwll.ch,
ojeda@kernel.org, alex.gaynor@gmail.com, wedsonaf@gmail.com,
boqun.feng@gmail.com, gary@garyguo.net, bjorn3_gh@protonmail.com,
benno.lossin@proton.me, a.hindborg@samsung.com,
aliceryhl@google.com, fujita.tomonori@gmail.com,
lina@asahilina.net, pstanner@redhat.com, ajanulgu@redhat.com,
lyude@redhat.com, gregkh@linuxfoundation.org,
rust-for-linux@vger.kernel.org, dri-devel@lists.freedesktop.org,
nouveau@lists.freedesktop.org
On 28/05/2024 17.18, Danilo Krummrich wrote:
> External email: Use caution opening links or attachments
>
>
> On Tue, May 28, 2024 at 08:40:20AM +0000, Zhi Wang wrote:
>> On 27/05/2024 22.18, Danilo Krummrich wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On Tue, May 21, 2024 at 08:32:31AM +0300, Zhi Wang wrote:
>>>> On Mon, 20 May 2024 19:24:19 +0200
>>>> Danilo Krummrich <dakr@redhat.com> wrote:
>>>>
>>>>> Add an abstraction around the kernels firmware API to request firmware
>>>>> images. The abstraction provides functions to access the firmware
>>>>> buffer and / or copy it to a new buffer allocated with a given
>>>>> allocator backend.
>>>>>
>>>>
>>>> Was playing with firmware extractions based on this patch.
>>>> Unfortunately I ended up with a lot of pointer operations, unsafe
>>>> statements.
>>>>
>>>> As we know many vendors have a C headers for the definitions of the
>>>> firwmare content, the driver extract the data by applying a struct
>>>> pointer on it.
>>>>
>>>> But in rust, I feel it would nice that we can also have a common
>>>> firmware extractor for drivers, that can wrap the pointer operations,
>>>> take a list of the firmware struct members that converted from C headers
>>>> as the input, offer the driver some common ABI methods to query them.
>>>> Maybe that would ease the pain a lot.
>>>
>>> So, you mean some abstraction that takes a list of types, offsets in the
>>> firmware and a reference to the firmware itself and provides references to the
>>> corresponding objects?
>>>
>>> I agree it might be helpful to have some common infrastructure for this, but the
>>> operations on it would still be unsafe, since ultimately it involves
>>> dereferencing pointers.
>>>
>>
>> I think the goal is to 1) concentrate the "unsafe" operations in a
>> abstraction so the driver doesn't have to write explanation of unsafe
>> operations here and there, improve the readability of the driver that
>> interacts with vendor-firmware buffer. 2) ease the driver maintenance
>> burden.
>
> With a generic abstraction, as in 1), at lest some of the abstraction's
> functions would be unsafe themselves, since they have to rely on the layout
> (or offset) passed by the driver being correct. What if I pass a wrong layout /
> offset for a structure that contains a pointer? This might still result in an
> invalid pointer dereference. Am I missing something?
>
>>
>> Some solutions I saw in different projects written in rust for
>> de-referencing a per-defined struct:
>>
>> 1) Take the vendor firmware buffer as a whole, invent methods like
>> read/write with offset for operating the buffer.
>>
>> In this scheme, the driver is responsible for taking care of each data
>> member in a vendor firmware struct and also its valid offset. The
>> abstraction only covers the boundary of the whole firmware buffer.
>>
>> The cons is the readability of the operation of the vendor firmware
>> buffer in the driver is not good comparing with C code.
>>
>> Hate to think a lot of xxx = vendor_firmware_struct.read(offset); //
>> reading item A in the code.
>>
>> 2) Define the firmware struct in rust (might need to find a better way
>> to handle "union" in the definition of the vendor firmware struct). Use
>> macros to generate methods of accessing each data member for the vendor
>> firmware struct.
>>
>> Then the code in the driver will be like xxx =
>> vendor_firmware_struct.item_a(); in the driver.
>>
>> In this scheme, the abstraction and the generated methods covers the
>> boundary check. The "unsafe" statement can stay in the generated
>> struct-access methods.
>>
>> This might even be implemented as a more generic rust feature in the kernel.
>
> This sounds more like a driver specific abstraction to me, which, as you write,
> we can probably come up with some macros that help implementing it.
>
> But again, what if the offsets are within the boundary, but still at a wrong
> offset? What if the data obtained from a wrong offset leads to other safety
> implications when further processing them? I think no generic abstraction can
> ever cover the safety parts of this (entirely). I think there are always semanic
> parts to this the driver has to cover.
>
I was thinking of a proc_macro that takes a vender-firmware struct
definition. As it can get the type and the name of each member, then it
can generate methods like xxx::member_a() that returns the value from
the "unsafe {*(type *)(pointer + offset)}". Thus, the unsafe stuff stays
in the generated methods.
For offset, I was hoping the macro can automatically calculate it based
on the member offset (the vendor-firmware definition) when generating
xxx::member_x(). (Maybe it can also take alignment into account)
As the return value has a rust type, rust should catch it if the caller
wants to do something crazy there.
> Generally, I think we should aim for some generalization, but I think we should
> not expect it to cover all the safety aspects.
>
I agree. I was mostly thinking how to ease the pain of driver and see
how the best we can do for it.
>>
>> The cons is still the driver might need to sync between the C-version
>> definition and rust-version definition.
>
> What do you mean with the driver needs to sync between a C and a Rust version of
> structure definitions?
>
Let's say a C driver maintains quite some headers and support some not
very new HWs. A new rust driver maintains some headers that written in
rust, it needs those headers as well. Now the firmware updates, both the
C headers and rust headers needs to be updated accordingly due to ABI
changes.
I was thinking if that process can be optimized, at least trying to
avoid the sync process, which might be painful if the amount of the
headers are huge.
>>
>> 3) Also re-using C bindings generation in the kernel came to my mind
>> when thinking of this problem, since it allows the rust code to access
>> the C struct, but they don't have the boundary check. Still need another
>> layer/macros to wrap it.
>
> I think we should have the structure definitions in Rust directly.
>
> - Danilo
>
>>
>>
>>>>
>>>> Thanks,
>>>> Zhi.
>>>>
>
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-05-20 17:24 ` [RFC PATCH 7/8] rust: add firmware abstractions Danilo Krummrich
2024-05-21 5:32 ` Zhi Wang
@ 2024-05-21 23:53 ` FUJITA Tomonori
2024-05-22 7:37 ` Philipp Stanner
2024-05-27 19:22 ` Danilo Krummrich
2024-05-28 12:06 ` Danilo Krummrich
2 siblings, 2 replies; 53+ messages in thread
From: FUJITA Tomonori @ 2024-05-21 23:53 UTC (permalink / raw)
To: dakr
Cc: maarten.lankhorst, mripard, tzimmermann, airlied, daniel, ojeda,
alex.gaynor, wedsonaf, boqun.feng, gary, bjorn3_gh, benno.lossin,
a.hindborg, aliceryhl, fujita.tomonori, lina, pstanner, ajanulgu,
lyude, gregkh, rust-for-linux, dri-devel, nouveau
Hi,
Thanks for working on the firmware API!
On Mon, 20 May 2024 19:24:19 +0200
Danilo Krummrich <dakr@redhat.com> wrote:
> Add an abstraction around the kernels firmware API to request firmware
> images. The abstraction provides functions to access the firmware
> buffer and / or copy it to a new buffer allocated with a given allocator
> backend.
>
> The firmware is released once the abstraction is dropped.
>
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> ---
> rust/bindings/bindings_helper.h | 1 +
> rust/kernel/firmware.rs | 74 +++++++++++++++++++++++++++++++++
> rust/kernel/lib.rs | 1 +
> 3 files changed, 76 insertions(+)
> create mode 100644 rust/kernel/firmware.rs
>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index b245db8d5a87..e4ffc47da5ec 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -14,6 +14,7 @@
> #include <kunit/test.h>
> #include <linux/errname.h>
> #include <linux/ethtool.h>
> +#include <linux/firmware.h>
> #include <linux/jiffies.h>
> #include <linux/mdio.h>
> #include <linux/pci.h>
> diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> new file mode 100644
> index 000000000000..700504fb3c9c
> --- /dev/null
> +++ b/rust/kernel/firmware.rs
> @@ -0,0 +1,74 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Firmware abstraction
> +//!
> +//! C header: [`include/linux/firmware.h`](../../../../include/linux/firmware.h")
> +
> +use crate::{bindings, device::Device, error::Error, error::Result, str::CStr, types::Opaque};
> +
> +/// Abstraction around a C firmware struct.
> +///
> +/// This is a simple abstraction around the C firmware API. Just like with the C API, firmware can
> +/// be requested. Once requested the abstraction provides direct access to the firmware buffer as
> +/// `&[u8]`. Alternatively, the firmware can be copied to a new buffer using `Firmware::copy`. The
> +/// firmware is released once [`Firmware`] is dropped.
> +///
> +/// # Examples
> +///
> +/// ```
> +/// let fw = Firmware::request("path/to/firmware.bin", dev.as_ref())?;
> +/// driver_load_firmware(fw.data());
> +/// ```
> +pub struct Firmware(Opaque<*const bindings::firmware>);
Wrapping a raw pointer is not the intended use of Qpaque type?
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-05-21 23:53 ` FUJITA Tomonori
@ 2024-05-22 7:37 ` Philipp Stanner
2024-05-22 23:15 ` FUJITA Tomonori
2024-05-27 19:22 ` Danilo Krummrich
1 sibling, 1 reply; 53+ messages in thread
From: Philipp Stanner @ 2024-05-22 7:37 UTC (permalink / raw)
To: FUJITA Tomonori, dakr
Cc: maarten.lankhorst, mripard, tzimmermann, airlied, daniel, ojeda,
alex.gaynor, wedsonaf, boqun.feng, gary, bjorn3_gh, benno.lossin,
a.hindborg, aliceryhl, lina, ajanulgu, lyude, gregkh,
rust-for-linux, dri-devel, nouveau
On Wed, 2024-05-22 at 08:53 +0900, FUJITA Tomonori wrote:
> Hi,
> Thanks for working on the firmware API!
>
> On Mon, 20 May 2024 19:24:19 +0200
> Danilo Krummrich <dakr@redhat.com> wrote:
>
> > Add an abstraction around the kernels firmware API to request
> > firmware
> > images. The abstraction provides functions to access the firmware
> > buffer and / or copy it to a new buffer allocated with a given
> > allocator
> > backend.
> >
> > The firmware is released once the abstraction is dropped.
> >
> > Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> > ---
> > rust/bindings/bindings_helper.h | 1 +
> > rust/kernel/firmware.rs | 74
> > +++++++++++++++++++++++++++++++++
> > rust/kernel/lib.rs | 1 +
> > 3 files changed, 76 insertions(+)
> > create mode 100644 rust/kernel/firmware.rs
> >
> > diff --git a/rust/bindings/bindings_helper.h
> > b/rust/bindings/bindings_helper.h
> > index b245db8d5a87..e4ffc47da5ec 100644
> > --- a/rust/bindings/bindings_helper.h
> > +++ b/rust/bindings/bindings_helper.h
> > @@ -14,6 +14,7 @@
> > #include <kunit/test.h>
> > #include <linux/errname.h>
> > #include <linux/ethtool.h>
> > +#include <linux/firmware.h>
> > #include <linux/jiffies.h>
> > #include <linux/mdio.h>
> > #include <linux/pci.h>
> > diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> > new file mode 100644
> > index 000000000000..700504fb3c9c
> > --- /dev/null
> > +++ b/rust/kernel/firmware.rs
> > @@ -0,0 +1,74 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! Firmware abstraction
> > +//!
> > +//! C header:
> > [`include/linux/firmware.h`](../../../../include/linux/firmware.h")
> > +
> > +use crate::{bindings, device::Device, error::Error, error::Result,
> > str::CStr, types::Opaque};
> > +
> > +/// Abstraction around a C firmware struct.
> > +///
> > +/// This is a simple abstraction around the C firmware API. Just
> > like with the C API, firmware can
> > +/// be requested. Once requested the abstraction provides direct
> > access to the firmware buffer as
> > +/// `&[u8]`. Alternatively, the firmware can be copied to a new
> > buffer using `Firmware::copy`. The
> > +/// firmware is released once [`Firmware`] is dropped.
> > +///
> > +/// # Examples
> > +///
> > +/// ```
> > +/// let fw = Firmware::request("path/to/firmware.bin",
> > dev.as_ref())?;
> > +/// driver_load_firmware(fw.data());
> > +/// ```
> > +pub struct Firmware(Opaque<*const bindings::firmware>);
>
> Wrapping a raw pointer is not the intended use of Qpaque type?
>
What is the intended use?
As I see it, all uses wrapp some binding::* – but a rawpointer to a
binding shouldn't be wrapped by it?
Maybe we can add something to the docu in kernel/types.rs:
/// Stores an opaque value.
///
/// This is meant to be used with FFI objects that are never interpreted by Rust code.
#[repr(transparent)]
pub struct Opaque<T> {
P.
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-05-22 7:37 ` Philipp Stanner
@ 2024-05-22 23:15 ` FUJITA Tomonori
2024-05-23 2:48 ` Boqun Feng
0 siblings, 1 reply; 53+ messages in thread
From: FUJITA Tomonori @ 2024-05-22 23:15 UTC (permalink / raw)
To: pstanner
Cc: fujita.tomonori, dakr, maarten.lankhorst, mripard, tzimmermann,
airlied, daniel, ojeda, alex.gaynor, wedsonaf, boqun.feng, gary,
bjorn3_gh, benno.lossin, a.hindborg, aliceryhl, lina, ajanulgu,
lyude, gregkh, rust-for-linux, dri-devel, nouveau
Hi,
On Wed, 22 May 2024 09:37:30 +0200
Philipp Stanner <pstanner@redhat.com> wrote:
>> > +/// Abstraction around a C firmware struct.
>> > +///
>> > +/// This is a simple abstraction around the C firmware API. Just
>> > like with the C API, firmware can
>> > +/// be requested. Once requested the abstraction provides direct
>> > access to the firmware buffer as
>> > +/// `&[u8]`. Alternatively, the firmware can be copied to a new
>> > buffer using `Firmware::copy`. The
>> > +/// firmware is released once [`Firmware`] is dropped.
>> > +///
>> > +/// # Examples
>> > +///
>> > +/// ```
>> > +/// let fw = Firmware::request("path/to/firmware.bin",
>> > dev.as_ref())?;
>> > +/// driver_load_firmware(fw.data());
>> > +/// ```
>> > +pub struct Firmware(Opaque<*const bindings::firmware>);
>>
>> Wrapping a raw pointer is not the intended use of Qpaque type?
>>
>
> What is the intended use?
> As I see it, all uses wrapp some binding::* – but a rawpointer to a
> binding shouldn't be wrapped by it?
If I understand correctly, it's for embedding C's struct in Rust's
struct (as all the usage in the tree do). It gives the tricks for
initialization and a pointer to the embedded object.
The C's firmware API gives a pointer to an initialized object so no
reason to use Opaque.
With such C API, Rust's struct simply uses raw pointers in old rust
branch. For example,
https://github.com/Rust-for-Linux/linux/blob/rust/rust/kernel/chrdev.rs#L28
struct Cdev(*mut bindings::cdev);
Another choice that I know is NonNull:
https://lore.kernel.org/lkml/20240415-alice-mm-v5-4-6f55e4d8ef51@google.com/
pub struct Page {
page: NonNull<bindings::page>,
}
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-05-22 23:15 ` FUJITA Tomonori
@ 2024-05-23 2:48 ` Boqun Feng
0 siblings, 0 replies; 53+ messages in thread
From: Boqun Feng @ 2024-05-23 2:48 UTC (permalink / raw)
To: FUJITA Tomonori
Cc: pstanner, dakr, maarten.lankhorst, mripard, tzimmermann, airlied,
daniel, ojeda, alex.gaynor, wedsonaf, gary, bjorn3_gh,
benno.lossin, a.hindborg, aliceryhl, lina, ajanulgu, lyude,
gregkh, rust-for-linux, dri-devel, nouveau
On Thu, May 23, 2024 at 08:15:13AM +0900, FUJITA Tomonori wrote:
> Hi,
>
> On Wed, 22 May 2024 09:37:30 +0200
> Philipp Stanner <pstanner@redhat.com> wrote:
>
> >> > +/// Abstraction around a C firmware struct.
> >> > +///
> >> > +/// This is a simple abstraction around the C firmware API. Just
> >> > like with the C API, firmware can
> >> > +/// be requested. Once requested the abstraction provides direct
> >> > access to the firmware buffer as
> >> > +/// `&[u8]`. Alternatively, the firmware can be copied to a new
> >> > buffer using `Firmware::copy`. The
> >> > +/// firmware is released once [`Firmware`] is dropped.
> >> > +///
> >> > +/// # Examples
> >> > +///
> >> > +/// ```
> >> > +/// let fw = Firmware::request("path/to/firmware.bin",
> >> > dev.as_ref())?;
> >> > +/// driver_load_firmware(fw.data());
> >> > +/// ```
> >> > +pub struct Firmware(Opaque<*const bindings::firmware>);
> >>
> >> Wrapping a raw pointer is not the intended use of Qpaque type?
> >>
> >
> > What is the intended use?
> > As I see it, all uses wrapp some binding::* – but a rawpointer to a
> > binding shouldn't be wrapped by it?
>
Thank you Tomo for calling this out!
And yes, using `Opaque` on a pointer is weird. A `Opaque<T>` is
`UnsafeCell<MayUninit<T>>`, `UnsafeCell` means the inner `T` may be
accessed by C code at anytime, and `MayUninit` means that the inner `T`
may not be properly initialized by the C code. So as the doc says:
This is meant to be used with FFI objects that are never
interpreted by Rust code.
that is, Rust code should never create a `&T` or `&mut T`, it should
only be accessed with `Opaque::get()` or its friends (i.e. accessing it
via a raw pointer), much like a black box to Rust code in some sense.
Hence putting `Opaque` on a raw pointer is not what you want to do.
> If I understand correctly, it's for embedding C's struct in Rust's
> struct (as all the usage in the tree do). It gives the tricks for
> initialization and a pointer to the embedded object.
>
> The C's firmware API gives a pointer to an initialized object so no
> reason to use Opaque.
>
> With such C API, Rust's struct simply uses raw pointers in old rust
> branch. For example,
>
> https://github.com/Rust-for-Linux/linux/blob/rust/rust/kernel/chrdev.rs#L28
>
> struct Cdev(*mut bindings::cdev);
>
>
> Another choice that I know is NonNull:
>
> https://lore.kernel.org/lkml/20240415-alice-mm-v5-4-6f55e4d8ef51@google.com/
>
> pub struct Page {
> page: NonNull<bindings::page>,
> }
Both are reasonable for temporary use, although, we could generify the
"wrapping on pointer which owns the underlying object" in the future.
Regards,
Boqun
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-05-21 23:53 ` FUJITA Tomonori
2024-05-22 7:37 ` Philipp Stanner
@ 2024-05-27 19:22 ` Danilo Krummrich
2024-05-28 11:01 ` FUJITA Tomonori
1 sibling, 1 reply; 53+ messages in thread
From: Danilo Krummrich @ 2024-05-27 19:22 UTC (permalink / raw)
To: FUJITA Tomonori
Cc: maarten.lankhorst, mripard, tzimmermann, airlied, daniel, ojeda,
alex.gaynor, wedsonaf, boqun.feng, gary, bjorn3_gh, benno.lossin,
a.hindborg, aliceryhl, lina, pstanner, ajanulgu, lyude, gregkh,
rust-for-linux, dri-devel, nouveau
On Wed, May 22, 2024 at 08:53:34AM +0900, FUJITA Tomonori wrote:
> Hi,
> Thanks for working on the firmware API!
>
> On Mon, 20 May 2024 19:24:19 +0200
> Danilo Krummrich <dakr@redhat.com> wrote:
>
> > Add an abstraction around the kernels firmware API to request firmware
> > images. The abstraction provides functions to access the firmware
> > buffer and / or copy it to a new buffer allocated with a given allocator
> > backend.
> >
> > The firmware is released once the abstraction is dropped.
> >
> > Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> > ---
> > rust/bindings/bindings_helper.h | 1 +
> > rust/kernel/firmware.rs | 74 +++++++++++++++++++++++++++++++++
> > rust/kernel/lib.rs | 1 +
> > 3 files changed, 76 insertions(+)
> > create mode 100644 rust/kernel/firmware.rs
> >
> > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> > index b245db8d5a87..e4ffc47da5ec 100644
> > --- a/rust/bindings/bindings_helper.h
> > +++ b/rust/bindings/bindings_helper.h
> > @@ -14,6 +14,7 @@
> > #include <kunit/test.h>
> > #include <linux/errname.h>
> > #include <linux/ethtool.h>
> > +#include <linux/firmware.h>
> > #include <linux/jiffies.h>
> > #include <linux/mdio.h>
> > #include <linux/pci.h>
> > diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> > new file mode 100644
> > index 000000000000..700504fb3c9c
> > --- /dev/null
> > +++ b/rust/kernel/firmware.rs
> > @@ -0,0 +1,74 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! Firmware abstraction
> > +//!
> > +//! C header: [`include/linux/firmware.h`](../../../../include/linux/firmware.h")
> > +
> > +use crate::{bindings, device::Device, error::Error, error::Result, str::CStr, types::Opaque};
> > +
> > +/// Abstraction around a C firmware struct.
> > +///
> > +/// This is a simple abstraction around the C firmware API. Just like with the C API, firmware can
> > +/// be requested. Once requested the abstraction provides direct access to the firmware buffer as
> > +/// `&[u8]`. Alternatively, the firmware can be copied to a new buffer using `Firmware::copy`. The
> > +/// firmware is released once [`Firmware`] is dropped.
> > +///
> > +/// # Examples
> > +///
> > +/// ```
> > +/// let fw = Firmware::request("path/to/firmware.bin", dev.as_ref())?;
> > +/// driver_load_firmware(fw.data());
> > +/// ```
> > +pub struct Firmware(Opaque<*const bindings::firmware>);
>
> Wrapping a raw pointer is not the intended use of Qpaque type?
>
Indeed, will fix this in v2 and use NonNull instead. I'll also offload most of
the boilerplate in the 'request' functions to some common 'request_internal' one.
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-05-27 19:22 ` Danilo Krummrich
@ 2024-05-28 11:01 ` FUJITA Tomonori
2024-05-28 12:19 ` Danilo Krummrich
0 siblings, 1 reply; 53+ messages in thread
From: FUJITA Tomonori @ 2024-05-28 11:01 UTC (permalink / raw)
To: dakr
Cc: fujita.tomonori, maarten.lankhorst, mripard, tzimmermann, airlied,
daniel, ojeda, alex.gaynor, wedsonaf, boqun.feng, gary, bjorn3_gh,
benno.lossin, a.hindborg, aliceryhl, lina, pstanner, ajanulgu,
lyude, gregkh, rust-for-linux, dri-devel, nouveau
On Mon, 27 May 2024 21:22:47 +0200
Danilo Krummrich <dakr@redhat.com> wrote:
>> > +/// Abstraction around a C firmware struct.
>> > +///
>> > +/// This is a simple abstraction around the C firmware API. Just like with the C API, firmware can
>> > +/// be requested. Once requested the abstraction provides direct access to the firmware buffer as
>> > +/// `&[u8]`. Alternatively, the firmware can be copied to a new buffer using `Firmware::copy`. The
>> > +/// firmware is released once [`Firmware`] is dropped.
>> > +///
>> > +/// # Examples
>> > +///
>> > +/// ```
>> > +/// let fw = Firmware::request("path/to/firmware.bin", dev.as_ref())?;
>> > +/// driver_load_firmware(fw.data());
>> > +/// ```
>> > +pub struct Firmware(Opaque<*const bindings::firmware>);
>>
>> Wrapping a raw pointer is not the intended use of Qpaque type?
>>
>
> Indeed, will fix this in v2 and use NonNull instead. I'll also offload most of
> the boilerplate in the 'request' functions to some common 'request_internal' one.
You might need to add 'Invariants' comment on Firmware struct.
BTW, what merge window are you aiming for? As I wrote before, I have a
driver that needs the firmware abstractions (the minimum device
abstractions is enough; Device::as_raw() and as_ref()). So the sooner,
the better for me.
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-05-28 11:01 ` FUJITA Tomonori
@ 2024-05-28 12:19 ` Danilo Krummrich
2024-05-28 12:45 ` Greg KH
2024-05-29 0:38 ` FUJITA Tomonori
0 siblings, 2 replies; 53+ messages in thread
From: Danilo Krummrich @ 2024-05-28 12:19 UTC (permalink / raw)
To: FUJITA Tomonori
Cc: maarten.lankhorst, mripard, tzimmermann, airlied, daniel, ojeda,
alex.gaynor, wedsonaf, boqun.feng, gary, bjorn3_gh, benno.lossin,
a.hindborg, aliceryhl, lina, pstanner, ajanulgu, lyude, gregkh,
rust-for-linux, dri-devel, nouveau, mcgrof, russ.weight
On Tue, May 28, 2024 at 08:01:26PM +0900, FUJITA Tomonori wrote:
> On Mon, 27 May 2024 21:22:47 +0200
> Danilo Krummrich <dakr@redhat.com> wrote:
>
> >> > +/// Abstraction around a C firmware struct.
> >> > +///
> >> > +/// This is a simple abstraction around the C firmware API. Just like with the C API, firmware can
> >> > +/// be requested. Once requested the abstraction provides direct access to the firmware buffer as
> >> > +/// `&[u8]`. Alternatively, the firmware can be copied to a new buffer using `Firmware::copy`. The
> >> > +/// firmware is released once [`Firmware`] is dropped.
> >> > +///
> >> > +/// # Examples
> >> > +///
> >> > +/// ```
> >> > +/// let fw = Firmware::request("path/to/firmware.bin", dev.as_ref())?;
> >> > +/// driver_load_firmware(fw.data());
> >> > +/// ```
> >> > +pub struct Firmware(Opaque<*const bindings::firmware>);
> >>
> >> Wrapping a raw pointer is not the intended use of Qpaque type?
> >>
> >
> > Indeed, will fix this in v2 and use NonNull instead. I'll also offload most of
> > the boilerplate in the 'request' functions to some common 'request_internal' one.
>
> You might need to add 'Invariants' comment on Firmware struct.
Which ones do you think should be documented?
>
> BTW, what merge window are you aiming for? As I wrote before, I have a
> driver that needs the firmware abstractions (the minimum device
> abstractions is enough; Device::as_raw() and as_ref()). So the sooner,
> the better for me.
I'm not aiming this on a specific merge window.
However, if you have a driver that needs the firmware abstractions, I would be
surprised if there were any hesitations to already merge the minimum device
abstractions [1] and this one (once reviewed) without the rest. At least there
aren't any from my side.
[1] https://lore.kernel.org/rust-for-linux/20240520172554.182094-2-dakr@redhat.com/
>
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-05-28 12:19 ` Danilo Krummrich
@ 2024-05-28 12:45 ` Greg KH
2024-05-28 13:17 ` Danilo Krummrich
2024-05-29 0:28 ` FUJITA Tomonori
2024-05-29 0:38 ` FUJITA Tomonori
1 sibling, 2 replies; 53+ messages in thread
From: Greg KH @ 2024-05-28 12:45 UTC (permalink / raw)
To: Danilo Krummrich
Cc: FUJITA Tomonori, maarten.lankhorst, mripard, tzimmermann, airlied,
daniel, ojeda, alex.gaynor, wedsonaf, boqun.feng, gary, bjorn3_gh,
benno.lossin, a.hindborg, aliceryhl, lina, pstanner, ajanulgu,
lyude, rust-for-linux, dri-devel, nouveau, mcgrof, russ.weight
On Tue, May 28, 2024 at 02:19:24PM +0200, Danilo Krummrich wrote:
> However, if you have a driver that needs the firmware abstractions, I would be
> surprised if there were any hesitations to already merge the minimum device
> abstractions [1] and this one (once reviewed) without the rest. At least there
> aren't any from my side.
>
> [1] https://lore.kernel.org/rust-for-linux/20240520172554.182094-2-dakr@redhat.com/
No, the device abstractions are NOT ready for merging just yet, sorry,
if for no other reason than a non-RFC has never been posted :)
greg k-h
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-05-28 12:45 ` Greg KH
@ 2024-05-28 13:17 ` Danilo Krummrich
2024-05-29 0:28 ` FUJITA Tomonori
1 sibling, 0 replies; 53+ messages in thread
From: Danilo Krummrich @ 2024-05-28 13:17 UTC (permalink / raw)
To: Greg KH
Cc: FUJITA Tomonori, maarten.lankhorst, mripard, tzimmermann, airlied,
daniel, ojeda, alex.gaynor, wedsonaf, boqun.feng, gary, bjorn3_gh,
benno.lossin, a.hindborg, aliceryhl, lina, pstanner, ajanulgu,
lyude, rust-for-linux, dri-devel, nouveau, mcgrof, russ.weight
On Tue, May 28, 2024 at 02:45:02PM +0200, Greg KH wrote:
> On Tue, May 28, 2024 at 02:19:24PM +0200, Danilo Krummrich wrote:
> > However, if you have a driver that needs the firmware abstractions, I would be
> > surprised if there were any hesitations to already merge the minimum device
> > abstractions [1] and this one (once reviewed) without the rest. At least there
> > aren't any from my side.
> >
> > [1] https://lore.kernel.org/rust-for-linux/20240520172554.182094-2-dakr@redhat.com/
>
> No, the device abstractions are NOT ready for merging just yet, sorry,
> if for no other reason than a non-RFC has never been posted :)
I did not say they are ready. I said that I'd be surprised if we couldn't merge
[1] once it is ready even without the rest being ready. :)
>
> greg k-h
>
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-05-28 12:45 ` Greg KH
2024-05-28 13:17 ` Danilo Krummrich
@ 2024-05-29 0:28 ` FUJITA Tomonori
2024-05-29 19:57 ` Greg KH
1 sibling, 1 reply; 53+ messages in thread
From: FUJITA Tomonori @ 2024-05-29 0:28 UTC (permalink / raw)
To: gregkh
Cc: dakr, fujita.tomonori, maarten.lankhorst, mripard, tzimmermann,
airlied, daniel, ojeda, alex.gaynor, wedsonaf, boqun.feng, gary,
bjorn3_gh, benno.lossin, a.hindborg, aliceryhl, lina, pstanner,
ajanulgu, lyude, rust-for-linux, dri-devel, nouveau, mcgrof,
russ.weight
Hi,
On Tue, 28 May 2024 14:45:02 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:
> On Tue, May 28, 2024 at 02:19:24PM +0200, Danilo Krummrich wrote:
>> However, if you have a driver that needs the firmware abstractions, I would be
>> surprised if there were any hesitations to already merge the minimum device
>> abstractions [1] and this one (once reviewed) without the rest. At least there
>> aren't any from my side.
>>
>> [1] https://lore.kernel.org/rust-for-linux/20240520172554.182094-2-dakr@redhat.com/
>
> No, the device abstractions are NOT ready for merging just yet, sorry,
> if for no other reason than a non-RFC has never been posted :)
Indeed, I understand that you aren't convinced.
We can start with much simpler device abstractions than the above
minimum for the firmware abstractions.
All the firmware abstractions need is wrapping C's pointer to a device
object with Rust struct only during a caller knows the pointer is
valid. No play with the reference count of a struct device.
For a Rust PHY driver, you know that you have a valid pointer to C's
device object of C's PHY device during the probe callback. The driver
creates a Rust device object to wrap the C pointer to the C's device
object and passes it to the firmware abstractions. The firmware
abstractions gets the C's pointer from the Rust object and calls C's
function to load firmware, returns the result.
You have concerns about the simple code like the following?
diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
new file mode 100644
index 000000000000..6144437984a9
--- /dev/null
+++ b/rust/kernel/device.rs
@@ -0,0 +1,30 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Generic devices that are part of the kernel's driver model.
+//!
+//! C header: [`include/linux/device.h`](srctree/include/linux/device.h)
+
+use crate::types::Opaque;
+
+#[repr(transparent)]
+pub struct Device(Opaque<bindings::device>);
+
+impl Device {
+ /// Creates a new [`Device`] instance from a raw pointer.
+ ///
+ /// # Safety
+ ///
+ /// For the duration of 'a, the pointer must point at a valid `device`.
+ pub unsafe fn from_raw<'a>(ptr: *mut bindings::device) -> &'a Self {
+ // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::device`.
+ let ptr = ptr.cast::<Self>();
+ // SAFETY: by the function requirements the pointer is valid and we have unique access for
+ // the duration of `'a`.
+ unsafe { &mut *ptr }
+ }
+
+ /// Returns the raw pointer to the device.
+ pub fn as_raw(&self) -> *mut bindings::device {
+ self.0.get()
+ }
+}
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-05-29 0:28 ` FUJITA Tomonori
@ 2024-05-29 19:57 ` Greg KH
2024-05-29 23:28 ` FUJITA Tomonori
0 siblings, 1 reply; 53+ messages in thread
From: Greg KH @ 2024-05-29 19:57 UTC (permalink / raw)
To: FUJITA Tomonori
Cc: dakr, maarten.lankhorst, mripard, tzimmermann, airlied, daniel,
ojeda, alex.gaynor, wedsonaf, boqun.feng, gary, bjorn3_gh,
benno.lossin, a.hindborg, aliceryhl, lina, pstanner, ajanulgu,
lyude, rust-for-linux, dri-devel, nouveau, mcgrof, russ.weight
On Wed, May 29, 2024 at 09:28:21AM +0900, FUJITA Tomonori wrote:
> Hi,
>
> On Tue, 28 May 2024 14:45:02 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
>
> > On Tue, May 28, 2024 at 02:19:24PM +0200, Danilo Krummrich wrote:
> >> However, if you have a driver that needs the firmware abstractions, I would be
> >> surprised if there were any hesitations to already merge the minimum device
> >> abstractions [1] and this one (once reviewed) without the rest. At least there
> >> aren't any from my side.
> >>
> >> [1] https://lore.kernel.org/rust-for-linux/20240520172554.182094-2-dakr@redhat.com/
> >
> > No, the device abstractions are NOT ready for merging just yet, sorry,
> > if for no other reason than a non-RFC has never been posted :)
>
> Indeed, I understand that you aren't convinced.
>
> We can start with much simpler device abstractions than the above
> minimum for the firmware abstractions.
>
> All the firmware abstractions need is wrapping C's pointer to a device
> object with Rust struct only during a caller knows the pointer is
> valid. No play with the reference count of a struct device.
Makes sense, but note that almost no one should be dealing with "raw"
struct device pointers :)
> For a Rust PHY driver, you know that you have a valid pointer to C's
> device object of C's PHY device during the probe callback. The driver
> creates a Rust device object to wrap the C pointer to the C's device
> object and passes it to the firmware abstractions. The firmware
> abstractions gets the C's pointer from the Rust object and calls C's
> function to load firmware, returns the result.
>
> You have concerns about the simple code like the following?
>
>
> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> new file mode 100644
> index 000000000000..6144437984a9
> --- /dev/null
> +++ b/rust/kernel/device.rs
> @@ -0,0 +1,30 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Generic devices that are part of the kernel's driver model.
> +//!
> +//! C header: [`include/linux/device.h`](srctree/include/linux/device.h)
> +
> +use crate::types::Opaque;
> +
> +#[repr(transparent)]
> +pub struct Device(Opaque<bindings::device>);
> +
> +impl Device {
> + /// Creates a new [`Device`] instance from a raw pointer.
> + ///
> + /// # Safety
> + ///
> + /// For the duration of 'a, the pointer must point at a valid `device`.
If the following rust code does what this comment says, then sure, I'm
ok with it for now if it helps you all out with stuff like the firmware
interface for the phy rust code.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-05-29 19:57 ` Greg KH
@ 2024-05-29 23:28 ` FUJITA Tomonori
2024-05-30 2:01 ` Danilo Krummrich
0 siblings, 1 reply; 53+ messages in thread
From: FUJITA Tomonori @ 2024-05-29 23:28 UTC (permalink / raw)
To: gregkh, dakr, wedsonaf
Cc: fujita.tomonori, maarten.lankhorst, mripard, tzimmermann, airlied,
daniel, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
benno.lossin, a.hindborg, aliceryhl, lina, pstanner, ajanulgu,
lyude, rust-for-linux, dri-devel, nouveau, mcgrof, russ.weight
Hi,
On Wed, 29 May 2024 21:57:03 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:
>> For a Rust PHY driver, you know that you have a valid pointer to C's
>> device object of C's PHY device during the probe callback. The driver
>> creates a Rust device object to wrap the C pointer to the C's device
>> object and passes it to the firmware abstractions. The firmware
>> abstractions gets the C's pointer from the Rust object and calls C's
>> function to load firmware, returns the result.
>>
>> You have concerns about the simple code like the following?
>>
>>
>> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
>> new file mode 100644
>> index 000000000000..6144437984a9
>> --- /dev/null
>> +++ b/rust/kernel/device.rs
>> @@ -0,0 +1,30 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Generic devices that are part of the kernel's driver model.
>> +//!
>> +//! C header: [`include/linux/device.h`](srctree/include/linux/device.h)
>> +
>> +use crate::types::Opaque;
>> +
>> +#[repr(transparent)]
>> +pub struct Device(Opaque<bindings::device>);
>> +
>> +impl Device {
>> + /// Creates a new [`Device`] instance from a raw pointer.
>> + ///
>> + /// # Safety
>> + ///
>> + /// For the duration of 'a, the pointer must point at a valid `device`.
>
> If the following rust code does what this comment says, then sure, I'm
> ok with it for now if it helps you all out with stuff like the firmware
> interface for the phy rust code.
Great, thanks a lot!
Danilo and Wedson, are there any concerns about pushing this patch [1]
for the firmware abstractions?
I you prefer to be the author of the patch, please let me know. Who
the author is doesn't matter to me. Otherwise, I'll add
Co-developed-by tag.
[1] https://lore.kernel.org/rust-for-linux/20240529.092821.1593412345609718860.fujita.tomonori@gmail.com/
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-05-29 23:28 ` FUJITA Tomonori
@ 2024-05-30 2:01 ` Danilo Krummrich
2024-05-30 4:24 ` FUJITA Tomonori
0 siblings, 1 reply; 53+ messages in thread
From: Danilo Krummrich @ 2024-05-30 2:01 UTC (permalink / raw)
To: FUJITA Tomonori
Cc: gregkh, wedsonaf, maarten.lankhorst, mripard, tzimmermann,
airlied, daniel, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
benno.lossin, a.hindborg, aliceryhl, lina, pstanner, ajanulgu,
lyude, rust-for-linux, dri-devel, nouveau, mcgrof, russ.weight
On Thu, May 30, 2024 at 08:28:24AM +0900, FUJITA Tomonori wrote:
> Hi,
>
> On Wed, 29 May 2024 21:57:03 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
>
> >> For a Rust PHY driver, you know that you have a valid pointer to C's
> >> device object of C's PHY device during the probe callback. The driver
> >> creates a Rust device object to wrap the C pointer to the C's device
> >> object and passes it to the firmware abstractions. The firmware
> >> abstractions gets the C's pointer from the Rust object and calls C's
> >> function to load firmware, returns the result.
> >>
> >> You have concerns about the simple code like the following?
> >>
> >>
> >> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> >> new file mode 100644
> >> index 000000000000..6144437984a9
> >> --- /dev/null
> >> +++ b/rust/kernel/device.rs
> >> @@ -0,0 +1,30 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +
> >> +//! Generic devices that are part of the kernel's driver model.
> >> +//!
> >> +//! C header: [`include/linux/device.h`](srctree/include/linux/device.h)
> >> +
> >> +use crate::types::Opaque;
> >> +
> >> +#[repr(transparent)]
> >> +pub struct Device(Opaque<bindings::device>);
> >> +
> >> +impl Device {
> >> + /// Creates a new [`Device`] instance from a raw pointer.
> >> + ///
> >> + /// # Safety
> >> + ///
> >> + /// For the duration of 'a, the pointer must point at a valid `device`.
> >
> > If the following rust code does what this comment says, then sure, I'm
> > ok with it for now if it helps you all out with stuff like the firmware
> > interface for the phy rust code.
>
> Great, thanks a lot!
>
> Danilo and Wedson, are there any concerns about pushing this patch [1]
> for the firmware abstractions?
Well, if everyone is fine with this one I don't see why we can't we go with [1]
directly? AFAICS, we'd only need the following fix:
-//! C header: [`include/linux/device.h`](../../../../include/linux/device.h)
+//! C header: [`include/linux/device.h`](srctree/include/linux/device.h)
[1] https://lore.kernel.org/rust-for-linux/20240520172554.182094-2-dakr@redhat.com/
>
> I you prefer to be the author of the patch, please let me know. Who
> the author is doesn't matter to me. Otherwise, I'll add
> Co-developed-by tag.
>
> [1] https://lore.kernel.org/rust-for-linux/20240529.092821.1593412345609718860.fujita.tomonori@gmail.com/
>
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-05-30 2:01 ` Danilo Krummrich
@ 2024-05-30 4:24 ` FUJITA Tomonori
2024-05-30 6:47 ` Danilo Krummrich
0 siblings, 1 reply; 53+ messages in thread
From: FUJITA Tomonori @ 2024-05-30 4:24 UTC (permalink / raw)
To: dakr, gregkh
Cc: fujita.tomonori, wedsonaf, maarten.lankhorst, mripard,
tzimmermann, airlied, daniel, ojeda, alex.gaynor, boqun.feng,
gary, bjorn3_gh, benno.lossin, a.hindborg, aliceryhl, lina,
pstanner, ajanulgu, lyude, rust-for-linux, dri-devel, nouveau,
mcgrof, russ.weight
Hi,
On Thu, 30 May 2024 04:01:39 +0200
Danilo Krummrich <dakr@redhat.com> wrote:
> On Thu, May 30, 2024 at 08:28:24AM +0900, FUJITA Tomonori wrote:
>> Hi,
>>
>> On Wed, 29 May 2024 21:57:03 +0200
>> Greg KH <gregkh@linuxfoundation.org> wrote:
>>
>> >> For a Rust PHY driver, you know that you have a valid pointer to C's
>> >> device object of C's PHY device during the probe callback. The driver
>> >> creates a Rust device object to wrap the C pointer to the C's device
>> >> object and passes it to the firmware abstractions. The firmware
>> >> abstractions gets the C's pointer from the Rust object and calls C's
>> >> function to load firmware, returns the result.
>> >>
>> >> You have concerns about the simple code like the following?
>> >>
>> >>
>> >> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
>> >> new file mode 100644
>> >> index 000000000000..6144437984a9
>> >> --- /dev/null
>> >> +++ b/rust/kernel/device.rs
>> >> @@ -0,0 +1,30 @@
>> >> +// SPDX-License-Identifier: GPL-2.0
>> >> +
>> >> +//! Generic devices that are part of the kernel's driver model.
>> >> +//!
>> >> +//! C header: [`include/linux/device.h`](srctree/include/linux/device.h)
>> >> +
>> >> +use crate::types::Opaque;
>> >> +
>> >> +#[repr(transparent)]
>> >> +pub struct Device(Opaque<bindings::device>);
>> >> +
>> >> +impl Device {
>> >> + /// Creates a new [`Device`] instance from a raw pointer.
>> >> + ///
>> >> + /// # Safety
>> >> + ///
>> >> + /// For the duration of 'a, the pointer must point at a valid `device`.
>> >
>> > If the following rust code does what this comment says, then sure, I'm
>> > ok with it for now if it helps you all out with stuff like the firmware
>> > interface for the phy rust code.
>>
>> Great, thanks a lot!
>>
>> Danilo and Wedson, are there any concerns about pushing this patch [1]
>> for the firmware abstractions?
>
> Well, if everyone is fine with this one I don't see why we can't we go with [1]
> directly? AFAICS, we'd only need the following fix:
>
> -//! C header: [`include/linux/device.h`](../../../../include/linux/device.h)
> +//! C header: [`include/linux/device.h`](srctree/include/linux/device.h)
>
> [1] https://lore.kernel.org/rust-for-linux/20240520172554.182094-2-dakr@redhat.com/
The difference is that your patch touches the reference count of a
struct device. My patch doesn't.
The following part in your patch allows the rust code to freely play
with the reference count of a struct device. Your Rust drm driver
interact with the reference count in a different way than C's drm
drivers if I understand correctly. I'm not sure that Greg will be
convinenced with that approach.
+// SAFETY: Instances of `Device` are always ref-counted.
+unsafe impl crate::types::AlwaysRefCounted for Device {
+ fn inc_ref(&self) {
+ // SAFETY: The existence of a shared reference guarantees that the refcount is nonzero.
+ unsafe { bindings::get_device(self.as_raw()) };
+ }
+
+ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
+ // SAFETY: The safety requirements guarantee that the refcount is nonzero.
+ unsafe { bindings::put_device(obj.cast().as_ptr()) }
+ }
+}
The following comments give the impression that Rust abstractions
wrongly interact with the reference count; callers check out the
reference counter. Nobody should do that.
+ /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count.
+ pub unsafe fn from_raw(ptr: *mut bindings::device) -> ARef<Self> {
+ /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count for
+ /// the entire duration when the returned reference exists.
+ pub unsafe fn as_ref<'a>(ptr: *mut bindings::device) -> &'a Self {
+ // SAFETY: Guaranteed by the safety requirements of the function.
+ unsafe { &*ptr.cast() }
+ }
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-05-30 4:24 ` FUJITA Tomonori
@ 2024-05-30 6:47 ` Danilo Krummrich
2024-05-31 7:50 ` FUJITA Tomonori
0 siblings, 1 reply; 53+ messages in thread
From: Danilo Krummrich @ 2024-05-30 6:47 UTC (permalink / raw)
To: FUJITA Tomonori
Cc: gregkh, wedsonaf, maarten.lankhorst, mripard, tzimmermann,
airlied, daniel, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
benno.lossin, a.hindborg, aliceryhl, lina, pstanner, ajanulgu,
lyude, rust-for-linux, dri-devel, nouveau, mcgrof, russ.weight
On Thu, May 30, 2024 at 01:24:33PM +0900, FUJITA Tomonori wrote:
> Hi,
>
> On Thu, 30 May 2024 04:01:39 +0200
> Danilo Krummrich <dakr@redhat.com> wrote:
>
> > On Thu, May 30, 2024 at 08:28:24AM +0900, FUJITA Tomonori wrote:
> >> Hi,
> >>
> >> On Wed, 29 May 2024 21:57:03 +0200
> >> Greg KH <gregkh@linuxfoundation.org> wrote:
> >>
> >> >> For a Rust PHY driver, you know that you have a valid pointer to C's
> >> >> device object of C's PHY device during the probe callback. The driver
> >> >> creates a Rust device object to wrap the C pointer to the C's device
> >> >> object and passes it to the firmware abstractions. The firmware
> >> >> abstractions gets the C's pointer from the Rust object and calls C's
> >> >> function to load firmware, returns the result.
> >> >>
> >> >> You have concerns about the simple code like the following?
> >> >>
> >> >>
> >> >> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> >> >> new file mode 100644
> >> >> index 000000000000..6144437984a9
> >> >> --- /dev/null
> >> >> +++ b/rust/kernel/device.rs
> >> >> @@ -0,0 +1,30 @@
> >> >> +// SPDX-License-Identifier: GPL-2.0
> >> >> +
> >> >> +//! Generic devices that are part of the kernel's driver model.
> >> >> +//!
> >> >> +//! C header: [`include/linux/device.h`](srctree/include/linux/device.h)
> >> >> +
> >> >> +use crate::types::Opaque;
> >> >> +
> >> >> +#[repr(transparent)]
> >> >> +pub struct Device(Opaque<bindings::device>);
> >> >> +
> >> >> +impl Device {
> >> >> + /// Creates a new [`Device`] instance from a raw pointer.
> >> >> + ///
> >> >> + /// # Safety
> >> >> + ///
> >> >> + /// For the duration of 'a, the pointer must point at a valid `device`.
> >> >
> >> > If the following rust code does what this comment says, then sure, I'm
> >> > ok with it for now if it helps you all out with stuff like the firmware
> >> > interface for the phy rust code.
> >>
> >> Great, thanks a lot!
> >>
> >> Danilo and Wedson, are there any concerns about pushing this patch [1]
> >> for the firmware abstractions?
> >
> > Well, if everyone is fine with this one I don't see why we can't we go with [1]
> > directly? AFAICS, we'd only need the following fix:
> >
> > -//! C header: [`include/linux/device.h`](../../../../include/linux/device.h)
> > +//! C header: [`include/linux/device.h`](srctree/include/linux/device.h)
> >
> > [1] https://lore.kernel.org/rust-for-linux/20240520172554.182094-2-dakr@redhat.com/
>
> The difference is that your patch touches the reference count of a
> struct device. My patch doesn't.
>
> The following part in your patch allows the rust code to freely play
> with the reference count of a struct device. Your Rust drm driver
> interact with the reference count in a different way than C's drm
> drivers if I understand correctly. I'm not sure that Greg will be
> convinenced with that approach.
I don't see how this is different than what we do in C. If you (for whatever
reason) want to keep a pointer to a struct device you should make sure to
increase the reference count of this device, such that it can't get freed for
the time being.
This is a 1:1 representation of that and conceptually identical.
>
> +// SAFETY: Instances of `Device` are always ref-counted.
> +unsafe impl crate::types::AlwaysRefCounted for Device {
> + fn inc_ref(&self) {
> + // SAFETY: The existence of a shared reference guarantees that the refcount is nonzero.
> + unsafe { bindings::get_device(self.as_raw()) };
> + }
> +
> + unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
> + // SAFETY: The safety requirements guarantee that the refcount is nonzero.
> + unsafe { bindings::put_device(obj.cast().as_ptr()) }
> + }
> +}
>
> The following comments give the impression that Rust abstractions
> wrongly interact with the reference count; callers check out the
> reference counter. Nobody should do that.
No, saying that the caller must ensure that the device "has a non-zero reference
count" is a perfectly valid requirement.
It doensn't mean that the calling code has to peek the actual reference count,
but it means that it must be ensured that while we try to increase the reference
count it can't drop to zero unexpectedly.
Your patch, as a subset of this one, has the same requirements as listed below.
>
> + /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count.
> + pub unsafe fn from_raw(ptr: *mut bindings::device) -> ARef<Self> {
>
> + /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count for
> + /// the entire duration when the returned reference exists.
> + pub unsafe fn as_ref<'a>(ptr: *mut bindings::device) -> &'a Self {
> + // SAFETY: Guaranteed by the safety requirements of the function.
> + unsafe { &*ptr.cast() }
> + }
>
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-05-30 6:47 ` Danilo Krummrich
@ 2024-05-31 7:50 ` FUJITA Tomonori
2024-05-31 9:59 ` Danilo Krummrich
0 siblings, 1 reply; 53+ messages in thread
From: FUJITA Tomonori @ 2024-05-31 7:50 UTC (permalink / raw)
To: dakr
Cc: fujita.tomonori, gregkh, wedsonaf, maarten.lankhorst, mripard,
tzimmermann, airlied, daniel, ojeda, alex.gaynor, boqun.feng,
gary, bjorn3_gh, benno.lossin, a.hindborg, aliceryhl, lina,
pstanner, ajanulgu, lyude, rust-for-linux, dri-devel, nouveau,
mcgrof, russ.weight
On Thu, 30 May 2024 08:47:25 +0200
Danilo Krummrich <dakr@redhat.com> wrote:
>> >> >> For a Rust PHY driver, you know that you have a valid pointer to C's
>> >> >> device object of C's PHY device during the probe callback. The driver
>> >> >> creates a Rust device object to wrap the C pointer to the C's device
>> >> >> object and passes it to the firmware abstractions. The firmware
>> >> >> abstractions gets the C's pointer from the Rust object and calls C's
>> >> >> function to load firmware, returns the result.
>> >> >>
>> >> >> You have concerns about the simple code like the following?
>> >> >>
>> >> >>
>> >> >> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
>> >> >> new file mode 100644
>> >> >> index 000000000000..6144437984a9
>> >> >> --- /dev/null
>> >> >> +++ b/rust/kernel/device.rs
>> >> >> @@ -0,0 +1,30 @@
>> >> >> +// SPDX-License-Identifier: GPL-2.0
>> >> >> +
>> >> >> +//! Generic devices that are part of the kernel's driver model.
>> >> >> +//!
>> >> >> +//! C header: [`include/linux/device.h`](srctree/include/linux/device.h)
>> >> >> +
>> >> >> +use crate::types::Opaque;
>> >> >> +
>> >> >> +#[repr(transparent)]
>> >> >> +pub struct Device(Opaque<bindings::device>);
>> >> >> +
>> >> >> +impl Device {
>> >> >> + /// Creates a new [`Device`] instance from a raw pointer.
>> >> >> + ///
>> >> >> + /// # Safety
>> >> >> + ///
>> >> >> + /// For the duration of 'a, the pointer must point at a valid `device`.
>> >> >
>> >> > If the following rust code does what this comment says, then sure, I'm
>> >> > ok with it for now if it helps you all out with stuff like the firmware
>> >> > interface for the phy rust code.
>> >>
>> >> Great, thanks a lot!
>> >>
>> >> Danilo and Wedson, are there any concerns about pushing this patch [1]
>> >> for the firmware abstractions?
>> >
>> > Well, if everyone is fine with this one I don't see why we can't we go with [1]
>> > directly? AFAICS, we'd only need the following fix:
>> >
>> > -//! C header: [`include/linux/device.h`](../../../../include/linux/device.h)
>> > +//! C header: [`include/linux/device.h`](srctree/include/linux/device.h)
>> >
>> > [1] https://lore.kernel.org/rust-for-linux/20240520172554.182094-2-dakr@redhat.com/
>>
>> The difference is that your patch touches the reference count of a
>> struct device. My patch doesn't.
>>
>> The following part in your patch allows the rust code to freely play
>> with the reference count of a struct device. Your Rust drm driver
>> interact with the reference count in a different way than C's drm
>> drivers if I understand correctly. I'm not sure that Greg will be
>> convinenced with that approach.
>
> I don't see how this is different than what we do in C. If you (for whatever
> reason) want to keep a pointer to a struct device you should make sure to
> increase the reference count of this device, such that it can't get freed for
> the time being.
>
> This is a 1:1 representation of that and conceptually identical.
A drm driver does such? If a drm driver allocates two types of
driver-specific data and keep a pointer to the pci device, then the
driver calls get_device() twice to increase the reference count of the
pci's device?
At the very least, network device drivers don't. a driver doesn't play
with the reference count. The network subsystem does. And, the network
subsystem doesn't care about how many pointers to a pci device a
driver keeps.
With the patch [1], a driver plays with the reference count of a
device and directly calls get/put_device. It's a different than C
drivers for me (not sure about drm subsystem though).
But I might be misunderstanding that Greg isn't convinced with this
reference count thing. We need to wait for his response.
>> +// SAFETY: Instances of `Device` are always ref-counted.
>> +unsafe impl crate::types::AlwaysRefCounted for Device {
>> + fn inc_ref(&self) {
>> + // SAFETY: The existence of a shared reference guarantees that the refcount is nonzero.
>> + unsafe { bindings::get_device(self.as_raw()) };
>> + }
>> +
>> + unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
>> + // SAFETY: The safety requirements guarantee that the refcount is nonzero.
>> + unsafe { bindings::put_device(obj.cast().as_ptr()) }
>> + }
>> +}
>>
>> The following comments give the impression that Rust abstractions
>> wrongly interact with the reference count; callers check out the
>> reference counter. Nobody should do that.
>
> No, saying that the caller must ensure that the device "has a non-zero reference
> count" is a perfectly valid requirement.
>
> It doensn't mean that the calling code has to peek the actual reference count,
> but it means that it must be ensured that while we try to increase the reference
> count it can't drop to zero unexpectedly.
Yeah, the same requirements but expressed differently.
But again, I might be misunderstanding. Greg might have a different reason.
> Your patch, as a subset of this one, has the same requirements as listed below.
>
>>
>> + /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count.
>> + pub unsafe fn from_raw(ptr: *mut bindings::device) -> ARef<Self> {
>>
>> + /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count for
>> + /// the entire duration when the returned reference exists.
>> + pub unsafe fn as_ref<'a>(ptr: *mut bindings::device) -> &'a Self {
>> + // SAFETY: Guaranteed by the safety requirements of the function.
>> + unsafe { &*ptr.cast() }
>> + }
>>
>
>
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-05-31 7:50 ` FUJITA Tomonori
@ 2024-05-31 9:59 ` Danilo Krummrich
2024-06-07 12:11 ` FUJITA Tomonori
0 siblings, 1 reply; 53+ messages in thread
From: Danilo Krummrich @ 2024-05-31 9:59 UTC (permalink / raw)
To: FUJITA Tomonori
Cc: gregkh, wedsonaf, maarten.lankhorst, mripard, tzimmermann,
airlied, daniel, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
benno.lossin, a.hindborg, aliceryhl, lina, pstanner, ajanulgu,
lyude, rust-for-linux, dri-devel, nouveau, mcgrof, russ.weight
On Fri, May 31, 2024 at 04:50:32PM +0900, FUJITA Tomonori wrote:
> On Thu, 30 May 2024 08:47:25 +0200
> Danilo Krummrich <dakr@redhat.com> wrote:
>
> >> >> >> For a Rust PHY driver, you know that you have a valid pointer to C's
> >> >> >> device object of C's PHY device during the probe callback. The driver
> >> >> >> creates a Rust device object to wrap the C pointer to the C's device
> >> >> >> object and passes it to the firmware abstractions. The firmware
> >> >> >> abstractions gets the C's pointer from the Rust object and calls C's
> >> >> >> function to load firmware, returns the result.
> >> >> >>
> >> >> >> You have concerns about the simple code like the following?
> >> >> >>
> >> >> >>
> >> >> >> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> >> >> >> new file mode 100644
> >> >> >> index 000000000000..6144437984a9
> >> >> >> --- /dev/null
> >> >> >> +++ b/rust/kernel/device.rs
> >> >> >> @@ -0,0 +1,30 @@
> >> >> >> +// SPDX-License-Identifier: GPL-2.0
> >> >> >> +
> >> >> >> +//! Generic devices that are part of the kernel's driver model.
> >> >> >> +//!
> >> >> >> +//! C header: [`include/linux/device.h`](srctree/include/linux/device.h)
> >> >> >> +
> >> >> >> +use crate::types::Opaque;
> >> >> >> +
> >> >> >> +#[repr(transparent)]
> >> >> >> +pub struct Device(Opaque<bindings::device>);
> >> >> >> +
> >> >> >> +impl Device {
> >> >> >> + /// Creates a new [`Device`] instance from a raw pointer.
> >> >> >> + ///
> >> >> >> + /// # Safety
> >> >> >> + ///
> >> >> >> + /// For the duration of 'a, the pointer must point at a valid `device`.
> >> >> >
> >> >> > If the following rust code does what this comment says, then sure, I'm
> >> >> > ok with it for now if it helps you all out with stuff like the firmware
> >> >> > interface for the phy rust code.
> >> >>
> >> >> Great, thanks a lot!
> >> >>
> >> >> Danilo and Wedson, are there any concerns about pushing this patch [1]
> >> >> for the firmware abstractions?
> >> >
> >> > Well, if everyone is fine with this one I don't see why we can't we go with [1]
> >> > directly? AFAICS, we'd only need the following fix:
> >> >
> >> > -//! C header: [`include/linux/device.h`](../../../../include/linux/device.h)
> >> > +//! C header: [`include/linux/device.h`](srctree/include/linux/device.h)
> >> >
> >> > [1] https://lore.kernel.org/rust-for-linux/20240520172554.182094-2-dakr@redhat.com/
> >>
> >> The difference is that your patch touches the reference count of a
> >> struct device. My patch doesn't.
> >>
> >> The following part in your patch allows the rust code to freely play
> >> with the reference count of a struct device. Your Rust drm driver
> >> interact with the reference count in a different way than C's drm
> >> drivers if I understand correctly. I'm not sure that Greg will be
> >> convinenced with that approach.
> >
> > I don't see how this is different than what we do in C. If you (for whatever
> > reason) want to keep a pointer to a struct device you should make sure to
> > increase the reference count of this device, such that it can't get freed for
> > the time being.
> >
> > This is a 1:1 representation of that and conceptually identical.
>
> A drm driver does such? If a drm driver allocates two types of
> driver-specific data and keep a pointer to the pci device, then the
> driver calls get_device() twice to increase the reference count of the
> pci's device?
Think about it more generically. If you store a pointer to a device in some
structure (driver private data, generic subsystem structure, etc.), can you
guarantee that without acquiring a reference that the device' reference count
doesn't drop to zero while your structure is alive?
There are cases where you can't, e.g. with Arc<device::Data>. How do you
guarantee (generically for every driver) the last reference of your device data
is dropped with the driver's remove callback? If we make it the driver's
responsibility to care about this the whole thing is unsafe as in C.
In some cases you can, mostly the ones where you free a structure in the
driver's remove callback. But again, then there is no advantage over what we do
in C. What if you change the lifetime of your structure later on, then you may
introduce a bug.
>
> At the very least, network device drivers don't. a driver doesn't play
> with the reference count. The network subsystem does. And, the network
A quick 'grep' shows 23 occurrences of get_device() in network drivers.
> subsystem doesn't care about how many pointers to a pci device a
> driver keeps.
If none of the drivers has structures storing a pointer to a pci device that out
live the driver's remove callback that's fine.
>
> With the patch [1], a driver plays with the reference count of a
> device and directly calls get/put_device. It's a different than C
> drivers for me (not sure about drm subsystem though).
It's not different. Again, when a C driver stores a device pointer in a
structure whose lifetime is not bound a special boundary, like the device remove
callback, it must increase the reference count.
It's just that most of the time we are moving within this boundary and in C we
just rely on that. In Rust we can only get this safe by taking a reference for
every pointer we store. And making it safe to use seems to be the goal.
Generally, the whole point of a reference count is that everyone who owns a
pointer to this shared structure increases / decreases the reference count
accordingly.
>
> But I might be misunderstanding that Greg isn't convinced with this
> reference count thing. We need to wait for his response.
>
>
> >> +// SAFETY: Instances of `Device` are always ref-counted.
> >> +unsafe impl crate::types::AlwaysRefCounted for Device {
> >> + fn inc_ref(&self) {
> >> + // SAFETY: The existence of a shared reference guarantees that the refcount is nonzero.
> >> + unsafe { bindings::get_device(self.as_raw()) };
> >> + }
> >> +
> >> + unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
> >> + // SAFETY: The safety requirements guarantee that the refcount is nonzero.
> >> + unsafe { bindings::put_device(obj.cast().as_ptr()) }
> >> + }
> >> +}
> >>
> >> The following comments give the impression that Rust abstractions
> >> wrongly interact with the reference count; callers check out the
> >> reference counter. Nobody should do that.
> >
> > No, saying that the caller must ensure that the device "has a non-zero reference
> > count" is a perfectly valid requirement.
> >
> > It doensn't mean that the calling code has to peek the actual reference count,
> > but it means that it must be ensured that while we try to increase the reference
> > count it can't drop to zero unexpectedly.
>
> Yeah, the same requirements but expressed differently.
Well, instead of
"ensure that `ptr` is valid, non-null, and has a non-zero reference count"
you propose
"the pointer must point at a valid `device`".
When I ask you to specify what "pointer to valid device" means, what would be
the answer?
Since we're still discussing this in the thread of the firmware abstraction, if
you have any further concerns, let's discuss them in the thread for the
corresponding patch.
Once we get to a conclusion I can send a series with only the device and firmare
abstractions such that we can get them in outside of the scope of the reset of
both series to get your driver going.
- Danilo
>
> But again, I might be misunderstanding. Greg might have a different reason.
>
> > Your patch, as a subset of this one, has the same requirements as listed below.
> >
> >>
> >> + /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count.
> >> + pub unsafe fn from_raw(ptr: *mut bindings::device) -> ARef<Self> {
> >>
> >> + /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count for
> >> + /// the entire duration when the returned reference exists.
> >> + pub unsafe fn as_ref<'a>(ptr: *mut bindings::device) -> &'a Self {
> >> + // SAFETY: Guaranteed by the safety requirements of the function.
> >> + unsafe { &*ptr.cast() }
> >> + }
> >>
> >
> >
>
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-05-31 9:59 ` Danilo Krummrich
@ 2024-06-07 12:11 ` FUJITA Tomonori
2024-06-07 12:36 ` Greg KH
2024-06-07 12:43 ` Danilo Krummrich
0 siblings, 2 replies; 53+ messages in thread
From: FUJITA Tomonori @ 2024-06-07 12:11 UTC (permalink / raw)
To: dakr
Cc: fujita.tomonori, gregkh, wedsonaf, maarten.lankhorst, mripard,
tzimmermann, airlied, daniel, ojeda, alex.gaynor, boqun.feng,
gary, bjorn3_gh, benno.lossin, a.hindborg, aliceryhl, lina,
pstanner, ajanulgu, lyude, rust-for-linux, dri-devel, nouveau,
mcgrof, russ.weight
Hi,
On Fri, 31 May 2024 11:59:47 +0200
Danilo Krummrich <dakr@redhat.com> wrote:
> Once we get to a conclusion I can send a series with only the device and firmare
> abstractions such that we can get them in outside of the scope of the reset of
> both series to get your driver going.
Since your discussion with Greg seems to continue for a while, let me
include the following patch that Greg approved with the next version
of the PHY driver patchset.
You have the new version of the firmware patch? The unused functions
will not be merged so only request_firmware() and release_firmware()
can be included. If not, I can include my firmware patch that I wrote
before.
=
From: Danilo Krummrich <dakr@redhat.com>
Date: Fri, 7 Jun 2024 20:14:59 +0900
Subject: [PATCH] add abstraction for struct device
Add abstraction for struct device. This implements only the minimum
necessary functions required for the abstractions of firmware API;
that is, wrapping C's pointer to a device object with Rust struct only
during a caller knows the pointer is valid (e.g., the probe callback).
Co-developed-by: Wedson Almeida Filho <wedsonaf@gmail.com>
Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
Signed-off-by: Danilo Krummrich <dakr@redhat.com>
Co-developed-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
rust/kernel/device.rs | 31 +++++++++++++++++++++++++++++++
rust/kernel/lib.rs | 1 +
2 files changed, 32 insertions(+)
create mode 100644 rust/kernel/device.rs
diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
new file mode 100644
index 000000000000..55ec4f364628
--- /dev/null
+++ b/rust/kernel/device.rs
@@ -0,0 +1,31 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Generic devices that are part of the kernel's driver model.
+//!
+//! C header: [`include/linux/device.h`](srctree/include/linux/device.h)
+
+use crate::types::Opaque;
+
+/// Wraps the kernel's `struct task_struct`.
+#[repr(transparent)]
+pub struct Device(Opaque<bindings::device>);
+
+impl Device {
+ /// Creates a new [`Device`] instance from a raw pointer.
+ ///
+ /// # Safety
+ ///
+ /// For the duration of 'a, the pointer must point at a valid `device`.
+ pub unsafe fn from_raw<'a>(ptr: *mut bindings::device) -> &'a Self {
+ // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::device`.
+ let ptr = ptr.cast::<Self>();
+ // SAFETY: by the function requirements the pointer is valid and we have unique access for
+ // the duration of `'a`.
+ unsafe { &mut *ptr }
+ }
+
+ /// Returns the raw pointer to the device.
+ pub(crate) fn as_raw(&self) -> *mut bindings::device {
+ self.0.get()
+ }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index fbd91a48ff8b..dd1207f1a873 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -28,6 +28,7 @@
pub mod alloc;
mod build_assert;
+pub mod device;
pub mod error;
pub mod init;
pub mod ioctl;
--
2.34.1
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-06-07 12:11 ` FUJITA Tomonori
@ 2024-06-07 12:36 ` Greg KH
2024-06-07 13:05 ` Danilo Krummrich
2024-06-07 13:33 ` Danilo Krummrich
2024-06-07 12:43 ` Danilo Krummrich
1 sibling, 2 replies; 53+ messages in thread
From: Greg KH @ 2024-06-07 12:36 UTC (permalink / raw)
To: FUJITA Tomonori
Cc: dakr, wedsonaf, maarten.lankhorst, mripard, tzimmermann, airlied,
daniel, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
benno.lossin, a.hindborg, aliceryhl, lina, pstanner, ajanulgu,
lyude, rust-for-linux, dri-devel, nouveau, mcgrof, russ.weight
On Fri, Jun 07, 2024 at 09:11:32PM +0900, FUJITA Tomonori wrote:
> Hi,
>
> On Fri, 31 May 2024 11:59:47 +0200
> Danilo Krummrich <dakr@redhat.com> wrote:
>
> > Once we get to a conclusion I can send a series with only the device and firmare
> > abstractions such that we can get them in outside of the scope of the reset of
> > both series to get your driver going.
>
> Since your discussion with Greg seems to continue for a while, let me
> include the following patch that Greg approved with the next version
> of the PHY driver patchset.
Yes, please take this one now. We can build on it from there.
I had a meeting yesterday with a lot of rust kernel and userspace people
at Microsoft and talked a bunch about this and how to move forward. I
think we need to take much smaller "steps" and even encourage most
drivers to start out as a mix of c and rust as there is no real
"requirement" that a driver be "pure" rust at all. This should both
make the interface logic simpler to start with, AND provide a base so
that people can just write the majority of their driver logic in rust,
which is where the language "safety" issues are most needed, not in the
lifecycle rules involving the internal driver model infrastructure.
Anyway, that's all hand-wavy right now, sorry, to get back to the point
here, again, let's take this, which will allow the firmware bindings to
be resubmitted and hopefully accepted, and we can move forward from
there to "real" things like a USB or PCI or even platform device and
driver binding stuff.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-06-07 12:36 ` Greg KH
@ 2024-06-07 13:05 ` Danilo Krummrich
2024-06-07 13:33 ` Danilo Krummrich
1 sibling, 0 replies; 53+ messages in thread
From: Danilo Krummrich @ 2024-06-07 13:05 UTC (permalink / raw)
To: Greg KH
Cc: FUJITA Tomonori, wedsonaf, maarten.lankhorst, mripard,
tzimmermann, airlied, daniel, ojeda, alex.gaynor, boqun.feng,
gary, bjorn3_gh, benno.lossin, a.hindborg, aliceryhl, lina,
pstanner, ajanulgu, lyude, rust-for-linux, dri-devel, nouveau,
mcgrof, russ.weight
On Fri, Jun 07, 2024 at 02:36:50PM +0200, Greg KH wrote:
> On Fri, Jun 07, 2024 at 09:11:32PM +0900, FUJITA Tomonori wrote:
> > Hi,
> >
> > On Fri, 31 May 2024 11:59:47 +0200
> > Danilo Krummrich <dakr@redhat.com> wrote:
> >
> > > Once we get to a conclusion I can send a series with only the device and firmare
> > > abstractions such that we can get them in outside of the scope of the reset of
> > > both series to get your driver going.
> >
> > Since your discussion with Greg seems to continue for a while, let me
> > include the following patch that Greg approved with the next version
> > of the PHY driver patchset.
>
> Yes, please take this one now. We can build on it from there.
This patch still contains the points *you* are discussing on the original one.
Why do I spend my time on this discussion if those points don't seem to matter
for you now?
Also, why would we want to have this patch (and the firmware one) in two
separate submissions? If we urgently want to land the firmware abstractions I
can send a separate series with just the device and firmware abstraction
patches.
>
> I had a meeting yesterday with a lot of rust kernel and userspace people
> at Microsoft and talked a bunch about this and how to move forward. I
> think we need to take much smaller "steps" and even encourage most
> drivers to start out as a mix of c and rust as there is no real
> "requirement" that a driver be "pure" rust at all. This should both
> make the interface logic simpler to start with, AND provide a base so
> that people can just write the majority of their driver logic in rust,
> which is where the language "safety" issues are most needed, not in the
> lifecycle rules involving the internal driver model infrastructure.
What do you mean by "drivers to start out as mix of C and Rust".
I don' think this is desireable. Writing abstractions for C core infrastructure
already is a lot of effort and sometimes rather difficult in certain aspects,
(e.g. align lifetimes).
An immediate concern I have is that this is an invitation for drivers to write
their own abstractions, as in interfacing with C core infrastructure in C and
and do a driver specific abstraction.
>
> Anyway, that's all hand-wavy right now, sorry, to get back to the point
> here, again, let's take this, which will allow the firmware bindings to
> be resubmitted and hopefully accepted, and we can move forward from
> there to "real" things like a USB or PCI or even platform device and
> driver binding stuff.
The abstractions for that are on the list and in the sense of what you say above
for "smaller steps, they are quite minimal. I don't see how we could break this
down even further.
>
> thanks,
>
> greg k-h
>
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-06-07 12:36 ` Greg KH
2024-06-07 13:05 ` Danilo Krummrich
@ 2024-06-07 13:33 ` Danilo Krummrich
2024-06-07 15:41 ` Greg KH
1 sibling, 1 reply; 53+ messages in thread
From: Danilo Krummrich @ 2024-06-07 13:33 UTC (permalink / raw)
To: Greg KH, FUJITA Tomonori
Cc: wedsonaf, maarten.lankhorst, mripard, tzimmermann, airlied,
daniel, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
benno.lossin, a.hindborg, aliceryhl, lina, pstanner, ajanulgu,
lyude, rust-for-linux, dri-devel, nouveau, mcgrof, russ.weight
On Fri, Jun 07, 2024 at 02:36:50PM +0200, Greg KH wrote:
> Anyway, that's all hand-wavy right now, sorry, to get back to the point
> here, again, let's take this, which will allow the firmware bindings to
> be resubmitted and hopefully accepted, and we can move forward from
> there to "real" things like a USB or PCI or even platform device and
> driver binding stuff.
In order to continue I propose to send out the following series:
1) minimal device and firmware abstractions only
2) v2 of all other device / driver, devres and PCI driver abstractions
3) v2 of basic DRM driver abstractions and Nova
The v2 series would contain static driver allocation (in case of DRM even const)
and quite a few more simplifications around `driver::Registration` and
`device::Data`.
Does that make sense?
- Danilo
>
> thanks,
>
> greg k-h
>
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-06-07 13:33 ` Danilo Krummrich
@ 2024-06-07 15:41 ` Greg KH
2024-06-07 17:55 ` Danilo Krummrich
0 siblings, 1 reply; 53+ messages in thread
From: Greg KH @ 2024-06-07 15:41 UTC (permalink / raw)
To: Danilo Krummrich
Cc: FUJITA Tomonori, wedsonaf, maarten.lankhorst, mripard,
tzimmermann, airlied, daniel, ojeda, alex.gaynor, boqun.feng,
gary, bjorn3_gh, benno.lossin, a.hindborg, aliceryhl, lina,
pstanner, ajanulgu, lyude, rust-for-linux, dri-devel, nouveau,
mcgrof, russ.weight
On Fri, Jun 07, 2024 at 03:33:39PM +0200, Danilo Krummrich wrote:
> On Fri, Jun 07, 2024 at 02:36:50PM +0200, Greg KH wrote:
> > Anyway, that's all hand-wavy right now, sorry, to get back to the point
> > here, again, let's take this, which will allow the firmware bindings to
> > be resubmitted and hopefully accepted, and we can move forward from
> > there to "real" things like a USB or PCI or even platform device and
> > driver binding stuff.
>
> In order to continue I propose to send out the following series:
>
> 1) minimal device and firmware abstractions only
Sounds good.
But after this, I don't want to take ANY driver core rust code that is
not able to live in the "normal" part of the Linux kernel tree. It's
just unsustainable to have it all in one directory sorry. If this
deadline makes that kbuild work actually happen faster, all the more
reason to have it. If that kbuild work is somehow stalled due to other
issues, please let me know what that is.
> 2) v2 of all other device / driver, devres and PCI driver abstractions
I will be glad to review this.
> 3) v2 of basic DRM driver abstractions and Nova
I love it how one of the most complex driver subsystems we have (drm) is
attempting to be the "first real" driver use for the rust apis. Bold
move :)
> The v2 series would contain static driver allocation (in case of DRM even const)
> and quite a few more simplifications around `driver::Registration` and
> `device::Data`.
>
> Does that make sense?
Yes, but note, I'm going to probably push back on the "driver::" stuff.
But will do so with more constructive criticism as I want this to work
very much and I agree that I think we are both talking past each other
in different ways. Mostly probably due to my total lack of Rust
experience, my apologies, thanks for your patience with me for this.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-06-07 15:41 ` Greg KH
@ 2024-06-07 17:55 ` Danilo Krummrich
2024-06-07 23:28 ` FUJITA Tomonori
0 siblings, 1 reply; 53+ messages in thread
From: Danilo Krummrich @ 2024-06-07 17:55 UTC (permalink / raw)
To: Greg KH
Cc: FUJITA Tomonori, wedsonaf, maarten.lankhorst, mripard,
tzimmermann, airlied, daniel, ojeda, alex.gaynor, boqun.feng,
gary, bjorn3_gh, benno.lossin, a.hindborg, aliceryhl, lina,
pstanner, ajanulgu, lyude, rust-for-linux, dri-devel, nouveau,
mcgrof, russ.weight
On Fri, Jun 07, 2024 at 05:41:11PM +0200, Greg KH wrote:
> On Fri, Jun 07, 2024 at 03:33:39PM +0200, Danilo Krummrich wrote:
> > On Fri, Jun 07, 2024 at 02:36:50PM +0200, Greg KH wrote:
> > > Anyway, that's all hand-wavy right now, sorry, to get back to the point
> > > here, again, let's take this, which will allow the firmware bindings to
> > > be resubmitted and hopefully accepted, and we can move forward from
> > > there to "real" things like a USB or PCI or even platform device and
> > > driver binding stuff.
> >
> > In order to continue I propose to send out the following series:
> >
> > 1) minimal device and firmware abstractions only
>
> Sounds good.
Just a heads-up, I'll probably send this one quite a bit earlier than the other
two to make sure to unblock Fujita on their PHY driver.
>
> But after this, I don't want to take ANY driver core rust code that is
> not able to live in the "normal" part of the Linux kernel tree. It's
> just unsustainable to have it all in one directory sorry. If this
> deadline makes that kbuild work actually happen faster, all the more
> reason to have it. If that kbuild work is somehow stalled due to other
> issues, please let me know what that is.
>
> > 2) v2 of all other device / driver, devres and PCI driver abstractions
>
> I will be glad to review this.
Glad to hear that!
>
> > 3) v2 of basic DRM driver abstractions and Nova
>
> I love it how one of the most complex driver subsystems we have (drm) is
> attempting to be the "first real" driver use for the rust apis. Bold
> move :)
I'd argue that as one of the most complex driver subsystems we have a lot of
need for the advantages Rust provides to us. :)
>
> > The v2 series would contain static driver allocation (in case of DRM even const)
> > and quite a few more simplifications around `driver::Registration` and
> > `device::Data`.
> >
> > Does that make sense?
>
> Yes, but note, I'm going to probably push back on the "driver::" stuff.
That's OK, I'm happy to convince you of its usefulness. :)
> But will do so with more constructive criticism as I want this to work
> very much and I agree that I think we are both talking past each other
> in different ways. Mostly probably due to my total lack of Rust
> experience, my apologies, thanks for your patience with me for this.
Very much appreciated! Please don't hesitate to ask for more explanation on
certain things if they're unclear. I'm happy trying my best to provide useful
answers.
- Danilo
>
> thanks,
>
> greg k-h
>
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-06-07 17:55 ` Danilo Krummrich
@ 2024-06-07 23:28 ` FUJITA Tomonori
2024-06-10 13:13 ` Danilo Krummrich
0 siblings, 1 reply; 53+ messages in thread
From: FUJITA Tomonori @ 2024-06-07 23:28 UTC (permalink / raw)
To: dakr
Cc: gregkh, fujita.tomonori, wedsonaf, maarten.lankhorst, mripard,
tzimmermann, airlied, daniel, ojeda, alex.gaynor, boqun.feng,
gary, bjorn3_gh, benno.lossin, a.hindborg, aliceryhl, lina,
pstanner, ajanulgu, lyude, rust-for-linux, dri-devel, nouveau,
mcgrof, russ.weight
On Fri, 7 Jun 2024 19:55:49 +0200
Danilo Krummrich <dakr@redhat.com> wrote:
> On Fri, Jun 07, 2024 at 05:41:11PM +0200, Greg KH wrote:
>> On Fri, Jun 07, 2024 at 03:33:39PM +0200, Danilo Krummrich wrote:
>> > On Fri, Jun 07, 2024 at 02:36:50PM +0200, Greg KH wrote:
>> > > Anyway, that's all hand-wavy right now, sorry, to get back to the point
>> > > here, again, let's take this, which will allow the firmware bindings to
>> > > be resubmitted and hopefully accepted, and we can move forward from
>> > > there to "real" things like a USB or PCI or even platform device and
>> > > driver binding stuff.
>> >
>> > In order to continue I propose to send out the following series:
>> >
>> > 1) minimal device and firmware abstractions only
>>
>> Sounds good.
>
> Just a heads-up, I'll probably send this one quite a bit earlier than the other
> two to make sure to unblock Fujita on their PHY driver.
Please. The sooner, the better. I need to send the PHY driver with
these patchse to netdev.
I'm not sure what the above "minimal device" means. If you send the
original patch again instead of the patch that Greg already approved
and the discussion continues, then I proceed with the approved patch.
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-06-07 23:28 ` FUJITA Tomonori
@ 2024-06-10 13:13 ` Danilo Krummrich
0 siblings, 0 replies; 53+ messages in thread
From: Danilo Krummrich @ 2024-06-10 13:13 UTC (permalink / raw)
To: FUJITA Tomonori
Cc: gregkh, wedsonaf, maarten.lankhorst, mripard, tzimmermann,
airlied, daniel, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
benno.lossin, a.hindborg, aliceryhl, lina, pstanner, ajanulgu,
lyude, rust-for-linux, dri-devel, nouveau, mcgrof, russ.weight
On Sat, Jun 08, 2024 at 08:28:06AM +0900, FUJITA Tomonori wrote:
> On Fri, 7 Jun 2024 19:55:49 +0200
> Danilo Krummrich <dakr@redhat.com> wrote:
>
> > On Fri, Jun 07, 2024 at 05:41:11PM +0200, Greg KH wrote:
> >> On Fri, Jun 07, 2024 at 03:33:39PM +0200, Danilo Krummrich wrote:
> >> > On Fri, Jun 07, 2024 at 02:36:50PM +0200, Greg KH wrote:
> >> > > Anyway, that's all hand-wavy right now, sorry, to get back to the point
> >> > > here, again, let's take this, which will allow the firmware bindings to
> >> > > be resubmitted and hopefully accepted, and we can move forward from
> >> > > there to "real" things like a USB or PCI or even platform device and
> >> > > driver binding stuff.
> >> >
> >> > In order to continue I propose to send out the following series:
> >> >
> >> > 1) minimal device and firmware abstractions only
> >>
> >> Sounds good.
> >
> > Just a heads-up, I'll probably send this one quite a bit earlier than the other
> > two to make sure to unblock Fujita on their PHY driver.
>
> Please. The sooner, the better. I need to send the PHY driver with
> these patchse to netdev.
Why do you want to send those patches to netdev?
I think nothing prevents you from sending your PHY driver to netdev. Just add a
note to your series that it depends on those two patches.
How and through which trees things are merged in the end can be figured out by
the corresponding maintainers in the end.
>
> I'm not sure what the above "minimal device" means. If you send the
> original patch again instead of the patch that Greg already approved
> and the discussion continues, then I proceed with the approved patch.
>
I'm honestly getting a bit tired of this...
1) I fundamentally disagree that it's a good thing to fork off patches that are
actively discussed and reviewed on the mailing list with the objective to
bypass the discussion and the review process. Especially without agreement of
all involved parties.
2) It's at least questionable to claim that your forked-off patch can be
considered to be "approved".
3) I really try to help and already confirmed to send out a separate series with
only the patches you need as well to accelerate things for you.
If you really want to help with that, you are very welcome to get involved in
the discussion and review process. If you don't want to, that is fine too. But
please stop adding confusion to those series by forking off patches.
Besides that, I also don't appreciate your attitude, trying to put some kind of
"ultimatum" on me.
- Danilo
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-06-07 12:11 ` FUJITA Tomonori
2024-06-07 12:36 ` Greg KH
@ 2024-06-07 12:43 ` Danilo Krummrich
1 sibling, 0 replies; 53+ messages in thread
From: Danilo Krummrich @ 2024-06-07 12:43 UTC (permalink / raw)
To: FUJITA Tomonori
Cc: gregkh, wedsonaf, maarten.lankhorst, mripard, tzimmermann,
airlied, daniel, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
benno.lossin, a.hindborg, aliceryhl, lina, pstanner, ajanulgu,
lyude, rust-for-linux, dri-devel, nouveau, mcgrof, russ.weight
On Fri, Jun 07, 2024 at 09:11:32PM +0900, FUJITA Tomonori wrote:
> Hi,
>
> On Fri, 31 May 2024 11:59:47 +0200
> Danilo Krummrich <dakr@redhat.com> wrote:
>
> > Once we get to a conclusion I can send a series with only the device and firmare
> > abstractions such that we can get them in outside of the scope of the reset of
> > both series to get your driver going.
>
> Since your discussion with Greg seems to continue for a while, let me
> include the following patch that Greg approved with the next version
> of the PHY driver patchset.
This all doesn't make a lot of sense. If that's the case, Greg approved
something the he keeps arguing about in the discussion of the original patch.
Please see the discussion about the NULL pointer check [1].
Besides that, I really don't think it's the correct approach to just (partially)
pick up a patch from the mailing list that is actively discussed and submit
versions that are slightly altered in the points that are actively discussed.
This really just complicates the situation and does not help getting to an
agreement.
>
> You have the new version of the firmware patch? The unused functions
> will not be merged so only request_firmware() and release_firmware()
> can be included. If not, I can include my firmware patch that I wrote
> before.
>
Please find the updated firmware patch in [2].
However, I propose to just send a new series with just the device abstraction
and this firmware patch and proceed from there.
[1] https://lore.kernel.org/rust-for-linux/Zl8_bXqK-T24y1kp@cassiopeiae/
[2] https://gitlab.freedesktop.org/drm/nova/-/commit/0683e186929c4922d565e5315525925aa2d8d686
NAK for the patch below, for the reasons above.
Please also see comments inline.
> =
> From: Danilo Krummrich <dakr@redhat.com>
> Date: Fri, 7 Jun 2024 20:14:59 +0900
> Subject: [PATCH] add abstraction for struct device
>
> Add abstraction for struct device. This implements only the minimum
> necessary functions required for the abstractions of firmware API;
> that is, wrapping C's pointer to a device object with Rust struct only
> during a caller knows the pointer is valid (e.g., the probe callback).
>
> Co-developed-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> Co-developed-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> rust/kernel/device.rs | 31 +++++++++++++++++++++++++++++++
> rust/kernel/lib.rs | 1 +
> 2 files changed, 32 insertions(+)
> create mode 100644 rust/kernel/device.rs
>
> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> new file mode 100644
> index 000000000000..55ec4f364628
> --- /dev/null
> +++ b/rust/kernel/device.rs
> @@ -0,0 +1,31 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Generic devices that are part of the kernel's driver model.
> +//!
> +//! C header: [`include/linux/device.h`](srctree/include/linux/device.h)
> +
> +use crate::types::Opaque;
> +
> +/// Wraps the kernel's `struct task_struct`.
> +#[repr(transparent)]
> +pub struct Device(Opaque<bindings::device>);
> +
> +impl Device {
> + /// Creates a new [`Device`] instance from a raw pointer.
> + ///
> + /// # Safety
> + ///
> + /// For the duration of 'a, the pointer must point at a valid `device`.
The original patch says: "Callers must ensure that `ptr` is valid, non-null, and
has a non-zero reference count for the entire duration when the returned
reference exists."
This is way more precise than just saying "For the duration of 'a, the pointer
must point at a valid `device`.", hence we should keep the original comment.
> + pub unsafe fn from_raw<'a>(ptr: *mut bindings::device) -> &'a Self {
> + // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::device`.
> + let ptr = ptr.cast::<Self>();
> + // SAFETY: by the function requirements the pointer is valid and we have unique access for
> + // the duration of `'a`.
> + unsafe { &mut *ptr }
Why not just
+ // SAFETY: Guaranteed by the safety requirements of the function.
+ unsafe { &*ptr.cast() }
as in the original commit?
> + }
> +
> + /// Returns the raw pointer to the device.
> + pub(crate) fn as_raw(&self) -> *mut bindings::device {
> + self.0.get()
> + }
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index fbd91a48ff8b..dd1207f1a873 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -28,6 +28,7 @@
>
> pub mod alloc;
> mod build_assert;
> +pub mod device;
> pub mod error;
> pub mod init;
> pub mod ioctl;
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-05-28 12:19 ` Danilo Krummrich
2024-05-28 12:45 ` Greg KH
@ 2024-05-29 0:38 ` FUJITA Tomonori
1 sibling, 0 replies; 53+ messages in thread
From: FUJITA Tomonori @ 2024-05-29 0:38 UTC (permalink / raw)
To: dakr
Cc: fujita.tomonori, maarten.lankhorst, mripard, tzimmermann, airlied,
daniel, ojeda, alex.gaynor, wedsonaf, boqun.feng, gary, bjorn3_gh,
benno.lossin, a.hindborg, aliceryhl, lina, pstanner, ajanulgu,
lyude, gregkh, rust-for-linux, dri-devel, nouveau, mcgrof,
russ.weight
On Tue, 28 May 2024 14:19:24 +0200
Danilo Krummrich <dakr@redhat.com> wrote:
> On Tue, May 28, 2024 at 08:01:26PM +0900, FUJITA Tomonori wrote:
>> On Mon, 27 May 2024 21:22:47 +0200
>> Danilo Krummrich <dakr@redhat.com> wrote:
>>
>> >> > +/// Abstraction around a C firmware struct.
>> >> > +///
>> >> > +/// This is a simple abstraction around the C firmware API. Just like with the C API, firmware can
>> >> > +/// be requested. Once requested the abstraction provides direct access to the firmware buffer as
>> >> > +/// `&[u8]`. Alternatively, the firmware can be copied to a new buffer using `Firmware::copy`. The
>> >> > +/// firmware is released once [`Firmware`] is dropped.
>> >> > +///
>> >> > +/// # Examples
>> >> > +///
>> >> > +/// ```
>> >> > +/// let fw = Firmware::request("path/to/firmware.bin", dev.as_ref())?;
>> >> > +/// driver_load_firmware(fw.data());
>> >> > +/// ```
>> >> > +pub struct Firmware(Opaque<*const bindings::firmware>);
>> >>
>> >> Wrapping a raw pointer is not the intended use of Qpaque type?
>> >>
>> >
>> > Indeed, will fix this in v2 and use NonNull instead. I'll also offload most of
>> > the boilerplate in the 'request' functions to some common 'request_internal' one.
>>
>> You might need to add 'Invariants' comment on Firmware struct.
>
> Which ones do you think should be documented?
Something like the comment for struct Page looks fine to me. But the
Rust reviewers might have a different opinion.
/// The pointer is valid, and has ownership over the page.
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC PATCH 7/8] rust: add firmware abstractions
2024-05-20 17:24 ` [RFC PATCH 7/8] rust: add firmware abstractions Danilo Krummrich
2024-05-21 5:32 ` Zhi Wang
2024-05-21 23:53 ` FUJITA Tomonori
@ 2024-05-28 12:06 ` Danilo Krummrich
2 siblings, 0 replies; 53+ messages in thread
From: Danilo Krummrich @ 2024-05-28 12:06 UTC (permalink / raw)
To: mcgrof, russ.weight
Cc: rust-for-linux, dri-devel, nouveau, maarten.lankhorst, mripard,
tzimmermann, airlied, daniel, ojeda, alex.gaynor, wedsonaf,
boqun.feng, gary, bjorn3_gh, benno.lossin, a.hindborg, aliceryhl,
fujita.tomonori, lina, pstanner, ajanulgu, lyude, gregkh
Hi Luis and Russ,
I just noted I forgot to add you to this patch, sorry for that.
Please find the full series and previous discussions in [1].
- Danilo
[1] https://lore.kernel.org/rust-for-linux/20240520172059.181256-1-dakr@redhat.com/
On Mon, May 20, 2024 at 07:24:19PM +0200, Danilo Krummrich wrote:
> Add an abstraction around the kernels firmware API to request firmware
> images. The abstraction provides functions to access the firmware
> buffer and / or copy it to a new buffer allocated with a given allocator
> backend.
>
> The firmware is released once the abstraction is dropped.
>
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> ---
> rust/bindings/bindings_helper.h | 1 +
> rust/kernel/firmware.rs | 74 +++++++++++++++++++++++++++++++++
> rust/kernel/lib.rs | 1 +
> 3 files changed, 76 insertions(+)
> create mode 100644 rust/kernel/firmware.rs
>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index b245db8d5a87..e4ffc47da5ec 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -14,6 +14,7 @@
> #include <kunit/test.h>
> #include <linux/errname.h>
> #include <linux/ethtool.h>
> +#include <linux/firmware.h>
> #include <linux/jiffies.h>
> #include <linux/mdio.h>
> #include <linux/pci.h>
> diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> new file mode 100644
> index 000000000000..700504fb3c9c
> --- /dev/null
> +++ b/rust/kernel/firmware.rs
> @@ -0,0 +1,74 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Firmware abstraction
> +//!
> +//! C header: [`include/linux/firmware.h`](../../../../include/linux/firmware.h")
> +
> +use crate::{bindings, device::Device, error::Error, error::Result, str::CStr, types::Opaque};
> +
> +/// Abstraction around a C firmware struct.
> +///
> +/// This is a simple abstraction around the C firmware API. Just like with the C API, firmware can
> +/// be requested. Once requested the abstraction provides direct access to the firmware buffer as
> +/// `&[u8]`. Alternatively, the firmware can be copied to a new buffer using `Firmware::copy`. The
> +/// firmware is released once [`Firmware`] is dropped.
> +///
> +/// # Examples
> +///
> +/// ```
> +/// let fw = Firmware::request("path/to/firmware.bin", dev.as_ref())?;
> +/// driver_load_firmware(fw.data());
> +/// ```
> +pub struct Firmware(Opaque<*const bindings::firmware>);
> +
> +impl Firmware {
> + /// Send a firmware request and wait for it. See also `bindings::request_firmware`.
> + pub fn request(name: &CStr, dev: &Device) -> Result<Self> {
> + let fw = Opaque::uninit();
> +
> + let ret = unsafe { bindings::request_firmware(fw.get(), name.as_char_ptr(), dev.as_raw()) };
> + if ret != 0 {
> + return Err(Error::from_errno(ret));
> + }
> +
> + Ok(Firmware(fw))
> + }
> +
> + /// Send a request for an optional fw module. See also `bindings::request_firmware_nowarn`.
> + pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Self> {
> + let fw = Opaque::uninit();
> +
> + let ret = unsafe {
> + bindings::firmware_request_nowarn(fw.get(), name.as_char_ptr(), dev.as_raw())
> + };
> + if ret != 0 {
> + return Err(Error::from_errno(ret));
> + }
> +
> + Ok(Firmware(fw))
> + }
> +
> + /// Returns the size of the requested firmware in bytes.
> + pub fn size(&self) -> usize {
> + unsafe { (*(*self.0.get())).size }
> + }
> +
> + /// Returns the requested firmware as `&[u8]`.
> + pub fn data(&self) -> &[u8] {
> + unsafe { core::slice::from_raw_parts((*(*self.0.get())).data, self.size()) }
> + }
> +}
> +
> +impl Drop for Firmware {
> + fn drop(&mut self) {
> + unsafe { bindings::release_firmware(*self.0.get()) };
> + }
> +}
> +
> +// SAFETY: `Firmware` only holds a pointer to a C firmware struct, which is safe to be used from any
> +// thread.
> +unsafe impl Send for Firmware {}
> +
> +// SAFETY: `Firmware` only holds a pointer to a C firmware struct, references to which are safe to
> +// be used from any thread.
> +unsafe impl Sync for Firmware {}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 6415968ee3b8..ed97d131661a 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -35,6 +35,7 @@
> #[cfg(CONFIG_DRM = "y")]
> pub mod drm;
> pub mod error;
> +pub mod firmware;
> pub mod init;
> pub mod ioctl;
> #[cfg(CONFIG_KUNIT)]
> --
> 2.45.1
>
^ permalink raw reply [flat|nested] 53+ messages in thread
* [RFC PATCH 8/8] nova: add initial driver stub
2024-05-20 17:20 [RFC PATCH 0/8] [RFC] DRM Rust abstractions and Nova Danilo Krummrich
` (7 preceding siblings ...)
2024-05-20 17:24 ` [RFC PATCH 7/8] rust: add firmware abstractions Danilo Krummrich
@ 2024-05-20 17:24 ` Danilo Krummrich
2024-05-20 17:30 ` Device / Driver and PCI Rust abstractions Danilo Krummrich
9 siblings, 0 replies; 53+ messages in thread
From: Danilo Krummrich @ 2024-05-20 17:24 UTC (permalink / raw)
To: maarten.lankhorst, mripard, tzimmermann, airlied, daniel, ojeda,
alex.gaynor, wedsonaf, boqun.feng, gary, bjorn3_gh, benno.lossin,
a.hindborg, aliceryhl, fujita.tomonori, lina, pstanner, ajanulgu,
lyude, gregkh
Cc: rust-for-linux, dri-devel, nouveau, Danilo Krummrich
Add the initial driver stub of Nova, a Rust-based GSP-only driver for
Nvidia GPUs. Nova, in the long term, is intended to serve as the
successor of Nouveau for GSP-firmware-based GPUs. [1]
As a stub driver Nova's focus is to make use of the most basic device /
driver infrastructure required to build a DRM driver on the PCI bus and
serve as demonstration example and justification for this
infrastructure.
In further consequence, the idea is to develop Nova continuously
upstream, using those increments to lift further Rust abstractions and
infrastructure upstream.
Link: https://lore.kernel.org/dri-devel/Zfsj0_tb-0-tNrJy@cassiopeiae/T/#u [1]
Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
MAINTAINERS | 10 ++
drivers/gpu/drm/Kconfig | 2 +
drivers/gpu/drm/Makefile | 1 +
drivers/gpu/drm/nova/Kconfig | 11 +++
drivers/gpu/drm/nova/Makefile | 3 +
drivers/gpu/drm/nova/driver.rs | 110 +++++++++++++++++++++
drivers/gpu/drm/nova/file.rs | 71 ++++++++++++++
drivers/gpu/drm/nova/gem.rs | 50 ++++++++++
drivers/gpu/drm/nova/gpu.rs | 172 +++++++++++++++++++++++++++++++++
drivers/gpu/drm/nova/nova.rs | 20 ++++
include/uapi/drm/nova_drm.h | 101 +++++++++++++++++++
rust/uapi/uapi_helper.h | 1 +
12 files changed, 552 insertions(+)
create mode 100644 drivers/gpu/drm/nova/Kconfig
create mode 100644 drivers/gpu/drm/nova/Makefile
create mode 100644 drivers/gpu/drm/nova/driver.rs
create mode 100644 drivers/gpu/drm/nova/file.rs
create mode 100644 drivers/gpu/drm/nova/gem.rs
create mode 100644 drivers/gpu/drm/nova/gpu.rs
create mode 100644 drivers/gpu/drm/nova/nova.rs
create mode 100644 include/uapi/drm/nova_drm.h
diff --git a/MAINTAINERS b/MAINTAINERS
index aa3b947fb080..1ca0ea445e3f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6863,6 +6863,16 @@ T: git https://gitlab.freedesktop.org/drm/nouveau.git
F: drivers/gpu/drm/nouveau/
F: include/uapi/drm/nouveau_drm.h
+DRM DRIVER (STUB) FOR NVIDIA GSP GPUS [RUST]
+M: Danilo Krummrich <dakr@redhat.com>
+L: dri-devel@lists.freedesktop.org
+L: nouveau@lists.freedesktop.org
+S: Supported
+C: irc://irc.oftc.net/nouveau
+T: git https://gitlab.freedesktop.org/drm/nova.git
+F: drivers/gpu/drm/nova/
+F: include/uapi/drm/nova_drm.h
+
DRM DRIVER FOR OLIMEX LCD-OLINUXINO PANELS
M: Stefan Mavrodiev <stefan@olimex.com>
S: Maintained
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 5a0c476361c3..0bb0442a252e 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -274,6 +274,8 @@ source "drivers/gpu/drm/amd/amdgpu/Kconfig"
source "drivers/gpu/drm/nouveau/Kconfig"
+source "drivers/gpu/drm/nova/Kconfig"
+
source "drivers/gpu/drm/i915/Kconfig"
source "drivers/gpu/drm/xe/Kconfig"
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 104b42df2e95..de6dc006cb7f 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -143,6 +143,7 @@ obj-$(CONFIG_DRM_VMWGFX)+= vmwgfx/
obj-$(CONFIG_DRM_VGEM) += vgem/
obj-$(CONFIG_DRM_VKMS) += vkms/
obj-$(CONFIG_DRM_NOUVEAU) +=nouveau/
+obj-$(CONFIG_DRM_NOVA_STUB) += nova/
obj-$(CONFIG_DRM_EXYNOS) +=exynos/
obj-$(CONFIG_DRM_ROCKCHIP) +=rockchip/
obj-$(CONFIG_DRM_GMA500) += gma500/
diff --git a/drivers/gpu/drm/nova/Kconfig b/drivers/gpu/drm/nova/Kconfig
new file mode 100644
index 000000000000..1ae2d1322a92
--- /dev/null
+++ b/drivers/gpu/drm/nova/Kconfig
@@ -0,0 +1,11 @@
+config DRM_NOVA_STUB
+ tristate "Nova GPU driver stub"
+ depends on DRM
+ depends on PCI
+ depends on RUST
+ default n
+ help
+ Choose this if you want to build the Nova stub driver for Nvidia
+ GSP-based GPUs.
+
+ If M is selected, the module will be called nova.
diff --git a/drivers/gpu/drm/nova/Makefile b/drivers/gpu/drm/nova/Makefile
new file mode 100644
index 000000000000..733ac5fb9f4f
--- /dev/null
+++ b/drivers/gpu/drm/nova/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_DRM_NOVA_STUB) += nova.o
diff --git a/drivers/gpu/drm/nova/driver.rs b/drivers/gpu/drm/nova/driver.rs
new file mode 100644
index 000000000000..47eaa2eddf16
--- /dev/null
+++ b/drivers/gpu/drm/nova/driver.rs
@@ -0,0 +1,110 @@
+// SPDX-License-Identifier: GPL-2.0
+
+use alloc::boxed::Box;
+use core::pin::Pin;
+use kernel::{
+ bindings, c_str, device, driver, drm,
+ drm::{drv, ioctl},
+ pci,
+ pci::define_pci_id_table,
+ prelude::*,
+ sync::Arc,
+};
+
+use crate::{file::File, gpu::Gpu};
+
+pub(crate) struct NovaDriver;
+
+/// Convienence type alias for the DRM device type for this driver
+pub(crate) type NovaDevice = drm::device::Device<NovaDriver>;
+
+#[allow(dead_code)]
+pub(crate) struct NovaData {
+ pub(crate) gpu: Arc<Gpu>,
+ pub(crate) pdev: pci::Device,
+}
+
+type DeviceData = device::Data<drm::drv::Registration<NovaDriver>, NovaData>;
+
+const INFO: drm::drv::DriverInfo = drm::drv::DriverInfo {
+ major: 0,
+ minor: 0,
+ patchlevel: 0,
+ name: c_str!("nova"),
+ desc: c_str!("Nvidia Graphics"),
+ date: c_str!("20240227"),
+};
+
+impl pci::Driver for NovaDriver {
+ type Data = Arc<DeviceData>;
+
+ define_pci_id_table! {
+ (),
+ [ (pci::DeviceId::new(bindings::PCI_VENDOR_ID_NVIDIA, bindings::PCI_ANY_ID as u32), None) ]
+ }
+
+ fn probe(pdev: &mut pci::Device, _id_info: Option<&Self::IdInfo>) -> Result<Arc<DeviceData>> {
+ dev_dbg!(pdev.as_ref(), "Probe Nova GPU driver.\n");
+
+ let reg = drm::drv::Registration::<NovaDriver>::new(pdev.as_ref())?;
+
+ pdev.enable_device_mem()?;
+ pdev.set_master();
+
+ let bar = pdev.iomap_region(0, c_str!("nova"))?;
+
+ let gpu = Gpu::new(pdev, bar)?;
+
+ let data = kernel::new_device_data!(
+ reg,
+ NovaData {
+ gpu,
+ pdev: pdev.clone(),
+ },
+ "NovaData"
+ )?;
+ let data: Arc<DeviceData> = data.into();
+
+ kernel::drm_device_register!(
+ data.registrations().ok_or(ENXIO)?.as_pinned_mut(),
+ data.clone(),
+ 0
+ )?;
+
+ Ok(data)
+ }
+
+ fn remove(data: &Self::Data) {
+ dev_dbg!(data.pdev.as_ref(), "Remove Nova GPU driver.\n");
+ }
+}
+
+#[vtable]
+impl drm::drv::Driver for NovaDriver {
+ type Data = Arc<DeviceData>;
+ type File = File;
+ type Object = crate::gem::Object;
+
+ const INFO: drm::drv::DriverInfo = INFO;
+ const FEATURES: u32 = drv::FEAT_GEM;
+
+ kernel::declare_drm_ioctls! {
+ (NOVA_GETPARAM, drm_nova_getparam, ioctl::RENDER_ALLOW, File::get_param),
+ (NOVA_GEM_CREATE, drm_nova_gem_create, ioctl::AUTH | ioctl::RENDER_ALLOW, File::gem_create),
+ (NOVA_GEM_INFO, drm_nova_gem_info, ioctl::AUTH | ioctl::RENDER_ALLOW, File::gem_info),
+ }
+}
+
+pub(crate) struct NovaModule {
+ _registration: Pin<Box<driver::Registration<pci::Adapter<NovaDriver>>>>,
+}
+
+impl kernel::Module for NovaModule {
+ fn init(_name: &'static CStr, module: &'static ThisModule) -> Result<Self> {
+ let registration = driver::Registration::new_pinned(c_str!("nova"), module)?;
+
+ Ok(Self {
+ _registration: registration,
+ })
+ }
+}
diff --git a/drivers/gpu/drm/nova/file.rs b/drivers/gpu/drm/nova/file.rs
new file mode 100644
index 000000000000..d649fa3973b5
--- /dev/null
+++ b/drivers/gpu/drm/nova/file.rs
@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: GPL-2.0
+
+use crate::driver::{NovaDevice, NovaDriver};
+use crate::gem;
+use kernel::{
+ alloc::flags::*,
+ drm::{self, device::Device as DrmDevice, gem::BaseObject},
+ prelude::*,
+ uapi,
+};
+
+pub(crate) struct File();
+
+/// Convenience type alias for our DRM `File` type
+pub(crate) type DrmFile = drm::file::File<File>;
+
+impl drm::file::DriverFile for File {
+ type Driver = NovaDriver;
+
+ fn open(dev: &DrmDevice<Self::Driver>) -> Result<Pin<Box<Self>>> {
+ dev_dbg!(dev.as_ref(), "drm::device::Device::open\n");
+
+ Ok(Box::into_pin(Box::new(Self(), GFP_KERNEL)?))
+ }
+}
+
+impl File {
+ /// IOCTL: get_param: Query GPU / driver metadata.
+ pub(crate) fn get_param(
+ dev: &NovaDevice,
+ getparam: &mut uapi::drm_nova_getparam,
+ _file: &DrmFile,
+ ) -> Result<u32> {
+ let data = dev.data().ok_or(ENODEV)?;
+ let pdev = &data.pdev;
+
+ getparam.value = match getparam.param as u32 {
+ uapi::NOVA_GETPARAM_VRAM_BAR_SIZE => pdev.resource_len(1)?,
+ _ => return Err(EINVAL),
+ };
+
+ Ok(0)
+ }
+
+ /// IOCTL: gem_create: Create a new DRM GEM object.
+ pub(crate) fn gem_create(
+ dev: &NovaDevice,
+ req: &mut uapi::drm_nova_gem_create,
+ file: &DrmFile,
+ ) -> Result<u32> {
+ let obj = gem::object_new(dev, req.size.try_into()?)?;
+
+ let handle = obj.create_handle(file)?;
+ req.handle = handle;
+
+ Ok(0)
+ }
+
+ /// IOCTL: gem_info: Query GEM metadata.
+ pub(crate) fn gem_info(
+ _dev: &NovaDevice,
+ req: &mut uapi::drm_nova_gem_info,
+ file: &DrmFile,
+ ) -> Result<u32> {
+ let bo = gem::lookup_handle(file, req.handle)?;
+
+ req.size = bo.size().try_into()?;
+
+ Ok(0)
+ }
+}
diff --git a/drivers/gpu/drm/nova/gem.rs b/drivers/gpu/drm/nova/gem.rs
new file mode 100644
index 000000000000..51bc30c226e2
--- /dev/null
+++ b/drivers/gpu/drm/nova/gem.rs
@@ -0,0 +1,50 @@
+// SPDX-License-Identifier: GPL-2.0
+
+use kernel::{
+ drm::{
+ gem,
+ gem::{BaseObject, ObjectRef},
+ },
+ prelude::*,
+};
+
+use crate::{
+ driver::{NovaDevice, NovaDriver},
+ file::DrmFile,
+};
+
+/// GEM Object inner driver data
+#[pin_data]
+pub(crate) struct DriverObject {}
+
+/// Type alias for the GEM object tyoe for this driver.
+pub(crate) type Object = gem::Object<DriverObject>;
+
+impl gem::BaseDriverObject<Object> for DriverObject {
+ fn new(dev: &NovaDevice, _size: usize) -> impl PinInit<Self, Error> {
+ dev_dbg!(dev.as_ref(), "DriverObject::new\n");
+ DriverObject {}
+ }
+}
+
+impl gem::DriverObject for DriverObject {
+ type Driver = NovaDriver;
+}
+
+/// Create a new DRM GEM object.
+pub(crate) fn object_new(dev: &NovaDevice, size: usize) -> Result<ObjectRef<Object>> {
+ let aligned_size = size.next_multiple_of(1 << 12);
+
+ if size == 0 || size > aligned_size {
+ return Err(EINVAL);
+ }
+
+ let gem = Object::new(dev, aligned_size)?;
+
+ Ok(ObjectRef::from_pinned_unique(gem))
+}
+
+/// Look up a GEM object handle for a `File` and return an `ObjectRef` for it.
+pub(crate) fn lookup_handle(file: &DrmFile, handle: u32) -> Result<ObjectRef<Object>> {
+ Object::lookup_handle(file, handle)
+}
diff --git a/drivers/gpu/drm/nova/gpu.rs b/drivers/gpu/drm/nova/gpu.rs
new file mode 100644
index 000000000000..b35d055e5322
--- /dev/null
+++ b/drivers/gpu/drm/nova/gpu.rs
@@ -0,0 +1,172 @@
+// SPDX-License-Identifier: GPL-2.0
+
+use kernel::{
+ device, devres::Devres, error::code::*, firmware, fmt, pci, prelude::*, str::CString, sync::Arc,
+};
+
+use core::fmt::Debug;
+
+/// Enum representing the GPU chipset.
+#[derive(Debug)]
+pub(crate) enum Chipset {
+ TU102 = 0x162,
+ TU104 = 0x164,
+ TU106 = 0x166,
+ TU117 = 0x167,
+ TU116 = 0x168,
+ GA102 = 0x172,
+ GA103 = 0x173,
+ GA104 = 0x174,
+ GA106 = 0x176,
+ GA107 = 0x177,
+ AD102 = 0x192,
+ AD103 = 0x193,
+ AD104 = 0x194,
+ AD106 = 0x196,
+ AD107 = 0x197,
+}
+
+/// Enum representing the GPU generation.
+#[derive(Debug)]
+pub(crate) enum CardType {
+ /// Turing
+ TU100 = 0x160,
+ /// Ampere
+ GA100 = 0x170,
+ /// Ada Lovelace
+ AD100 = 0x190,
+}
+
+/// Structure holding the metadata of the GPU.
+#[allow(dead_code)]
+pub(crate) struct GpuSpec {
+ /// Contents of the boot0 register.
+ boot0: u64,
+ card_type: CardType,
+ chipset: Chipset,
+ /// The revision of the chipset.
+ chiprev: u8,
+}
+
+/// Structure encapsulating the firmware blobs required for the GPU to operate.
+#[allow(dead_code)]
+pub(crate) struct Firmware {
+ booter_load: firmware::Firmware,
+ booter_unload: firmware::Firmware,
+ gsp: firmware::Firmware,
+}
+
+/// Structure holding the resources required to operate the GPU.
+#[allow(dead_code)]
+#[pin_data]
+pub(crate) struct Gpu {
+ spec: GpuSpec,
+ /// MMIO mapping of PCI BAR 0
+ bar: Devres<pci::Bar>,
+ fw: Firmware,
+}
+
+// TODO replace with something like derive(FromPrimitive)
+impl Chipset {
+ fn from_u32(value: u32) -> Option<Chipset> {
+ match value {
+ 0x162 => Some(Chipset::TU102),
+ 0x164 => Some(Chipset::TU104),
+ 0x166 => Some(Chipset::TU106),
+ 0x167 => Some(Chipset::TU117),
+ 0x168 => Some(Chipset::TU116),
+ 0x172 => Some(Chipset::GA102),
+ 0x173 => Some(Chipset::GA103),
+ 0x174 => Some(Chipset::GA104),
+ 0x176 => Some(Chipset::GA106),
+ 0x177 => Some(Chipset::GA107),
+ 0x192 => Some(Chipset::AD102),
+ 0x193 => Some(Chipset::AD103),
+ 0x194 => Some(Chipset::AD104),
+ 0x196 => Some(Chipset::AD106),
+ 0x197 => Some(Chipset::AD107),
+ _ => None,
+ }
+ }
+}
+
+// TODO replace with something like derive(FromPrimitive)
+impl CardType {
+ fn from_u32(value: u32) -> Option<CardType> {
+ match value {
+ 0x160 => Some(CardType::TU100),
+ 0x170 => Some(CardType::GA100),
+ 0x190 => Some(CardType::AD100),
+ _ => None,
+ }
+ }
+}
+
+impl GpuSpec {
+ fn new(bar: &Devres<pci::Bar>) -> Result<GpuSpec> {
+ let bar = bar.try_access().ok_or(ENXIO)?;
+ let boot0 = u64::from_le(bar.readq(0)?);
+ let chip = ((boot0 & 0x1ff00000) >> 20) as u32;
+
+ if boot0 & 0x1f000000 == 0 {
+ return Err(ENODEV);
+ }
+
+ let chipset = match Chipset::from_u32(chip) {
+ Some(x) => x,
+ None => return Err(ENODEV),
+ };
+
+ let card_type = match CardType::from_u32(chip & 0x1f0) {
+ Some(x) => x,
+ None => return Err(ENODEV),
+ };
+
+ Ok(Self {
+ boot0,
+ card_type,
+ chipset,
+ chiprev: (boot0 & 0xff) as u8,
+ })
+ }
+}
+
+impl Firmware {
+ fn new(dev: &device::Device, spec: &GpuSpec, ver: &str) -> Result<Firmware> {
+ let mut chip_name = CString::try_from_fmt(fmt!("{:?}", spec.chipset))?;
+ chip_name.make_ascii_lowercase();
+
+ let fw_booter_load_path =
+ CString::try_from_fmt(fmt!("nvidia/{}/gsp/booter_load-{}.bin", &*chip_name, ver,))?;
+ let fw_booter_unload_path =
+ CString::try_from_fmt(fmt!("nvidia/{}/gsp/booter_unload-{}.bin", &*chip_name, ver))?;
+ let fw_gsp_path =
+ CString::try_from_fmt(fmt!("nvidia/{}/gsp/gsp-{}.bin", &*chip_name, ver))?;
+
+ let booter_load = firmware::Firmware::request(&fw_booter_load_path, dev)?;
+ let booter_unload = firmware::Firmware::request(&fw_booter_unload_path, dev)?;
+ let gsp = firmware::Firmware::request(&fw_gsp_path, dev)?;
+
+ Ok(Firmware {
+ booter_load,
+ booter_unload,
+ gsp,
+ })
+ }
+}
+
+impl Gpu {
+ pub(crate) fn new(pdev: &pci::Device, bar: Devres<pci::Bar>) -> Result<Arc<Gpu>> {
+ let spec = GpuSpec::new(&bar)?;
+ let fw = Firmware::new(pdev.as_ref(), &spec, "535.113.01")?;
+
+ dev_info!(
+ pdev.as_ref(),
+ "NVIDIA {:?} ({:#x})",
+ spec.chipset,
+ spec.boot0
+ );
+
+ Arc::pin_init(try_pin_init!(Self { spec, bar, fw }), GFP_KERNEL)
+ }
+}
diff --git a/drivers/gpu/drm/nova/nova.rs b/drivers/gpu/drm/nova/nova.rs
new file mode 100644
index 000000000000..48420267bcc1
--- /dev/null
+++ b/drivers/gpu/drm/nova/nova.rs
@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Nova GPU Driver
+
+mod driver;
+mod file;
+mod gem;
+mod gpu;
+
+use kernel::prelude::module;
+
+use driver::NovaModule;
+
+module! {
+ type: NovaModule,
+ name: "Nova",
+ author: "Danilo Krummrich",
+ description: "Nova GPU driver",
+ license: "GPL v2",
+}
diff --git a/include/uapi/drm/nova_drm.h b/include/uapi/drm/nova_drm.h
new file mode 100644
index 000000000000..3ca90ed9d2bb
--- /dev/null
+++ b/include/uapi/drm/nova_drm.h
@@ -0,0 +1,101 @@
+/* SPDX-License-Identifier: MIT */
+
+#ifndef __NOVA_DRM_H__
+#define __NOVA_DRM_H__
+
+#include "drm.h"
+
+/* DISCLAIMER: Do not use, this is not a stable uAPI.
+ *
+ * This uAPI serves only testing purposes as long as this driver is still in
+ * development. It is required to implement and test infrastructure which is
+ * upstreamed in the context of this driver. See also [1].
+ *
+ * [1] https://lore.kernel.org/dri-devel/Zfsj0_tb-0-tNrJy@cassiopeiae/T/#u
+ */
+
+#if defined(__cplusplus)
+extern "C" {
+#endif
+
+/*
+ * NOVA_GETPARAM_VRAM_BAR_SIZE
+ *
+ * Query the VRAM BAR size in bytes.
+ */
+#define NOVA_GETPARAM_VRAM_BAR_SIZE 0x1
+
+/**
+ * struct drm_nova_getparam - query GPU and driver metadata
+ */
+struct drm_nova_getparam {
+ /**
+ * @param: The identifier of the parameter to query.
+ */
+ __u64 param;
+
+ /**
+ * @value: The value for the specified parameter.
+ */
+ __u64 value;
+};
+
+/**
+ * struct drm_nova_gem_create - create a new DRM GEM object
+ */
+struct drm_nova_gem_create {
+ /**
+ * @handle: The handle of the new DRM GEM object.
+ */
+ __u32 handle;
+
+ /**
+ * @pad: 32 bit padding, should be 0.
+ */
+ __u32 pad;
+
+ /**
+ * @size: The size of the new DRM GEM object.
+ */
+ __u64 size;
+};
+
+/**
+ * struct drm_nova_gem_info - query DRM GEM object metadata
+ */
+struct drm_nova_gem_info {
+ /**
+ * @handle: The handle of the DRM GEM object to query.
+ */
+ __u32 handle;
+
+ /**
+ * @pad: 32 bit padding, should be 0.
+ */
+ __u32 pad;
+
+ /**
+ * @size: The size of the DRM GEM obejct.
+ */
+ __u64 size;
+};
+
+#define DRM_NOVA_GETPARAM 0x00
+#define DRM_NOVA_GEM_CREATE 0x01
+#define DRM_NOVA_GEM_INFO 0x02
+
+/* Note: this is an enum so that it can be resolved by Rust bindgen. */
+enum {
+ DRM_IOCTL_NOVA_GETPARAM = DRM_IOWR(DRM_COMMAND_BASE + DRM_NOVA_GETPARAM,
+ struct drm_nova_getparam),
+ DRM_IOCTL_NOVA_GEM_CREATE = DRM_IOWR(DRM_COMMAND_BASE + DRM_NOVA_GEM_CREATE,
+ struct drm_nova_gem_create),
+ DRM_IOCTL_NOVA_GEM_INFO = DRM_IOWR(DRM_COMMAND_BASE + DRM_NOVA_GEM_INFO,
+ struct drm_nova_gem_info),
+};
+
+#if defined(__cplusplus)
+}
+#endif
+
+#endif /* __NOVA_DRM_H__ */
diff --git a/rust/uapi/uapi_helper.h b/rust/uapi/uapi_helper.h
index ed42a456da2e..b9ab3406b2ce 100644
--- a/rust/uapi/uapi_helper.h
+++ b/rust/uapi/uapi_helper.h
@@ -8,5 +8,6 @@
#include <uapi/asm-generic/ioctl.h>
#include <uapi/drm/drm.h>
+#include <uapi/drm/nova_drm.h>
#include <uapi/linux/mii.h>
#include <uapi/linux/ethtool.h>
--
2.45.1
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: Device / Driver and PCI Rust abstractions
2024-05-20 17:20 [RFC PATCH 0/8] [RFC] DRM Rust abstractions and Nova Danilo Krummrich
` (8 preceding siblings ...)
2024-05-20 17:24 ` [RFC PATCH 8/8] nova: add initial driver stub Danilo Krummrich
@ 2024-05-20 17:30 ` Danilo Krummrich
9 siblings, 0 replies; 53+ messages in thread
From: Danilo Krummrich @ 2024-05-20 17:30 UTC (permalink / raw)
To: maarten.lankhorst, mripard, tzimmermann, airlied, daniel, ojeda,
alex.gaynor, wedsonaf, boqun.feng, gary, bjorn3_gh, benno.lossin,
a.hindborg, aliceryhl, fujita.tomonori, lina, pstanner, ajanulgu,
lyude, gregkh
Cc: rust-for-linux, dri-devel, nouveau
https://lore.kernel.org/rust-for-linux/20240520172554.182094-1-dakr@redhat.com/
^ permalink raw reply [flat|nested] 53+ messages in thread