lkml.org 
[lkml]   [2023]   [Mar]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH RFC 01/18] rust: drm: ioctl: Add DRM ioctl abstraction
    On Tue, Mar 7, 2023 at 3:27 PM Asahi Lina <lina@asahilina.net> wrote:
    >
    > DRM drivers need to be able to declare which driver-specific ioctls they
    > support. This abstraction adds 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>
    > ---
    > drivers/gpu/drm/Kconfig | 7 ++
    > rust/bindings/bindings_helper.h | 2 +
    > rust/kernel/drm/ioctl.rs | 147 ++++++++++++++++++++++++++++++++++++++++
    > rust/kernel/drm/mod.rs | 5 ++
    > rust/kernel/lib.rs | 2 +
    > 5 files changed, 163 insertions(+)
    >
    > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
    > index dc0f94f02a82..dab8f0f9aa96 100644
    > --- a/drivers/gpu/drm/Kconfig
    > +++ b/drivers/gpu/drm/Kconfig
    > @@ -27,6 +27,13 @@ menuconfig DRM
    > details. You should also select and configure AGP
    > (/dev/agpgart) support if it is available for your platform.
    >
    > +# Rust abstractions cannot be built as modules currently, so force them as
    > +# bool by using these intermediate symbols. In the future these could be
    > +# tristate once abstractions themselves can be built as modules.
    > +config RUST_DRM
    > + bool "Rust support for the DRM subsystem"
    > + depends on DRM=y
    > +
    > config DRM_MIPI_DBI
    > tristate
    > depends on DRM
    > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
    > index 91bb7906ca5a..2687bef1676f 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 <linux/delay.h>
    > #include <linux/device.h>
    > #include <linux/dma-mapping.h>
    > @@ -23,6 +24,7 @@
    > #include <linux/sysctl.h>
    > #include <linux/timekeeping.h>
    > #include <linux/xarray.h>
    > +#include <uapi/drm/drm.h>
    >

    might make more sense to add this chunk to the patch actually needing it

    > /* `bindgen` gets confused at certain things. */
    > const gfp_t BINDINGS_GFP_KERNEL = GFP_KERNEL;
    > diff --git a/rust/kernel/drm/ioctl.rs b/rust/kernel/drm/ioctl.rs
    > new file mode 100644
    > index 000000000000..10304efbd5f1
    > --- /dev/null
    > +++ b/rust/kernel/drm/ioctl.rs
    > @@ -0,0 +1,147 @@
    > +// 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 = bindings::DRM_IOCTL_BASE as u32;
    > +
    > +/// Construct a DRM ioctl number with no argument.
    > +pub const fn IO(nr: u32) -> u32 {
    > + ioctl::_IO(BASE, nr)
    > +}
    > +
    > +/// Construct a DRM ioctl number with a read-only argument.
    > +pub const fn IOR<T>(nr: u32) -> u32 {
    > + ioctl::_IOR::<T>(BASE, nr)
    > +}
    > +
    > +/// Construct a DRM ioctl number with a write-only argument.
    > +pub const fn IOW<T>(nr: u32) -> u32 {
    > + ioctl::_IOW::<T>(BASE, nr)
    > +}
    > +
    > +/// Construct a DRM ioctl number with a read-write argument.
    > +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;
    > +
    > +/// Whether drm_ioctl_desc.func should be called with the DRM BKL held or not. Enforced as the
    > +/// default for all modern drivers, hence there should never be a need to set this flag.
    > +///
    > +/// Do not use anywhere else than for the VBLANK_WAIT IOCTL, which is the only legacy IOCTL which
    > +/// needs this.
    > +pub const UNLOCKED: u32 = bindings::drm_ioctl_flags_DRM_UNLOCKED;
    > +
    > +/// 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;
    > +
    > +/// 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),
    > +/// }

    I am wondering.. couldn't we make it a proc_macro and just tag all the
    functions instead? Though I also see the point of having a central
    list of all ioctls... Maybe we should have some higher level
    discussions around on _how_ we want things to look like.

    > +/// ```
    > +///
    > +#[macro_export]
    > +macro_rules! declare_drm_ioctls {
    > + ( $(($cmd:ident, $struct:ident, $flags:expr, $func:expr)),* $(,)? ) => {
    > + const IOCTLS: &'static [$crate::drm::ioctl::DrmIoctlDescriptor] = {
    > + const _:() = {
    > + let i: u32 = $crate::bindings::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!($crate::bindings::DRM_IOCTL_, $cmd);
    > + ::core::assert!(i == $crate::ioctl::_IOC_NR(cmd));
    > + ::core::assert!(core::mem::size_of::<$crate::bindings::$struct>() == $crate::ioctl::_IOC_SIZE(cmd));

    ::core::mem::size_of

    > + let i: u32 = i + 1;
    > + )*
    > + };
    > +
    > + let ioctls = &[$(
    > + $crate::bindings::drm_ioctl_desc {
    > + cmd: $crate::macros::concat_idents!($crate::bindings::DRM_IOCTL_, $cmd) as u32,
    > + func: {
    > + #[allow(non_snake_case)]
    > + unsafe extern "C" fn $cmd(
    > + raw_dev: *mut $crate::bindings::drm_device,
    > + raw_data: *mut ::core::ffi::c_void,
    > + raw_file_priv: *mut $crate::bindings::drm_file,
    > + ) -> core::ffi::c_int {

    ::core

    > + // SAFETY: We never drop this, and 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 = ::core::mem::ManuallyDrop::new(unsafe {
    > + $crate::drm::device::Device::from_raw(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::bindings::$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_kernel_errno(),
    > + Ok(i) => i.try_into().unwrap_or(ERANGE.to_kernel_errno()),

    need to specify the namespace on ERANGE, no?

    > + }
    > + }
    > + 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 7903490816bf..cb23d24c6718 100644
    > --- a/rust/kernel/lib.rs
    > +++ b/rust/kernel/lib.rs
    > @@ -37,6 +37,8 @@ mod build_assert;
    > pub mod delay;
    > pub mod device;
    > pub mod driver;
    > +#[cfg(CONFIG_RUST_DRM)]
    > +pub mod drm;
    > pub mod error;
    > pub mod io_buffer;
    > pub mod io_mem;
    >
    > --
    > 2.35.1
    >

    \
     
     \ /
      Last update: 2023-03-27 00:47    [W:4.336 / U:0.052 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site