Message-ID: <9fd1fea40f5d053e371bd076d9cb095ba3d77d93.camel@redhat.com>
Date: Sun, 21 Apr 2024 21:47:14 -0400
From: Lyude Paul <lyude@...hat.com>
To: Benno Lossin <benno.lossin@...ton.me>, dri-devel@...ts.freedesktop.org
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, 
 Wedson Almeida Filho <wedsonaf@...il.com>, Boqun Feng
 <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>, Andreas
 Hindborg <a.hindborg@...sung.com>, Alice Ryhl <aliceryhl@...gle.com>, Asahi
 Lina <lina@...hilina.net>, Martin Rodriguez Reboredo <yakoyoku@...il.com>,
 FUJITA Tomonori <fujita.tomonori@...il.com>, Danilo Krummrich
 <dakr@...hat.com>, linux-kernel@...r.kernel.org,
 rust-for-linux@...r.kernel.org
Subject: Re: [PATCH 1/4] WIP: rust: Add basic KMS bindings

On Wed, 2024-03-27 at 20:50 +0000, Benno Lossin wrote:
> Hi,
> 
> I just took a quick look and commented on the things that stuck
> out to me. Some general things:
> - several `unsafe` blocks have missing SAFETY comments,
> - missing documentation and examples.

This is really early on - so I had wanted to post a WIP before I
actually wrote up everything to make sure I'm going in the right
direction (I'm certainly not planning on leaving things undocumented
when this is actually ready for submission :).

> 
> On 22.03.24 23:03, Lyude Paul wrote:
> > Signed-off-by: Lyude Paul <lyude@...hat.com>
> > ---
> >  rust/bindings/bindings_helper.h  |   4 +
> >  rust/helpers.c                   |  17 ++
> >  rust/kernel/drm/device.rs        |   2 +
> >  rust/kernel/drm/drv.rs           | 115 +++++++--
> >  rust/kernel/drm/kms.rs           | 146 +++++++++++
> >  rust/kernel/drm/kms/connector.rs | 404
> > +++++++++++++++++++++++++++++++
> >  rust/kernel/drm/kms/crtc.rs      | 300 +++++++++++++++++++++++
> >  rust/kernel/drm/kms/encoder.rs   | 175 +++++++++++++
> >  rust/kernel/drm/kms/plane.rs     | 300 +++++++++++++++++++++++
> >  rust/kernel/drm/mod.rs           |   1 +
> >  10 files changed, 1448 insertions(+), 16 deletions(-)
> 
> Please try to break this up into smaller patches. It makes review
> a lot easier!

I'll definitely try to do that next time!

> 
> [...]
> 
> > diff --git a/rust/kernel/drm/kms.rs b/rust/kernel/drm/kms.rs
> > new file mode 100644
> > index 0000000000000..b55d14415367a
> > --- /dev/null
> > +++ b/rust/kernel/drm/kms.rs
> > @@ -0,0 +1,146 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR MIT
> > +
> > +//! KMS driver abstractions for rust.
> > +
> > +pub mod connector;
> > +pub mod crtc;
> > +pub mod encoder;
> > +pub mod plane;
> > +
> > +use crate::{
> > +    drm::{drv, device::Device},
> > +    prelude::*,
> > +    types::ARef,
> > +    private::Sealed
> > +};
> > +use core::{
> > +    ops::Deref,
> > +    ptr,
> > +};
> > +use bindings;
> > +
> > +#[derive(Copy, Clone)]
> > +pub struct ModeConfigInfo {
> > +    /// The minimum (w, h) resolution this driver can support
> > +    pub min_resolution: (i32, i32),
> > +    /// The maximum (w, h) resolution this driver can support
> > +    pub max_resolution: (i32, i32),
> > +    /// The maximum (w, h) cursor size this driver can support
> > +    pub max_cursor: (u32, u32),
> > +    /// The preferred depth for dumb ioctls
> > +    pub preferred_depth: u32,
> > +}
> > +
> > +// TODO: I am not totally sure about this. Ideally, I'd like a
> > nice way of hiding KMS-specific
> > +// functions for DRM drivers which don't implement KMS - so that
> > we don't have to have a bunch of
> > +// random modesetting functions all over the DRM device trait.
> > But, unfortunately I don't know of
> > +// any nice way of doing that yet :(
> 
> I don't follow, can't you put the KMS specific functions into the
> KmsDriver trait?

I can, lol. I realized how that would work a little while after writing
this, so I'm not quite sure where my confusion was with this - so I'll
fix this on the next version I send out.

> 
> > +
> > +/// An atomic KMS driver implementation
> > +pub trait KmsDriver: drv::Driver { }
> > +
> > +impl<T: KmsDriver> Device<T> {
> > +    pub fn mode_config_reset(&self) {
> > +        // SAFETY: The previous build assertion ensures this can
> > only be called for devices with KMS
> > +        // support, which means mode_config is initialized
> > +        unsafe { bindings::drm_mode_config_reset(self.drm.get()) }
> > +    }
> > +}
> > +
> > +/// Main trait for a modesetting object in DRM
> > +pub trait ModeObject: Sealed + Send + Sync {
> > +    /// The parent driver for this ModeObject
> > +    type Driver: KmsDriver;
> > +
> > +    /// Return the `drv::Device` for this `ModeObject`
> > +    fn drm_dev(&self) -> &Device<Self::Driver>;
> > +}
> 
> [...]
> 
> > diff --git a/rust/kernel/drm/kms/connector.rs
> > b/rust/kernel/drm/kms/connector.rs
> > new file mode 100644
> > index 0000000000000..88dfa946d306b
> > --- /dev/null
> > +++ b/rust/kernel/drm/kms/connector.rs
> > @@ -0,0 +1,404 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR MIT
> > +
> > +//! Rust bindings for DRM connectors
> > +
> > +use crate::{
> > +    bindings,
> > +    sync::ArcBorrow,
> > +    drm::{
> > +        drv::{Driver, FEAT_MODESET},
> > +        device::Device,
> > +    },
> > +    types::{AlwaysRefCounted, Opaque, ARef},
> > +    prelude::*,
> > +    init::Zeroable,
> > +    error::{to_result, from_result},
> > +    build_error,
> > +};
> > +use core::{
> > +    marker::PhantomPinned,
> > +    ptr::null_mut,
> > +    mem,
> > +    ptr::{self, NonNull},
> > +    ffi::*,
> > +    ops::Deref,
> > +};
> > +use super::{
> > +    ModeObject,
> > +    ModeConfigGuard,
> > +    encoder::{Encoder, DriverEncoder},
> > +    KmsDriver,
> > +};
> > +use macros::pin_data;
> > +
> > +// XXX: This is :\, figure out a better way at some point?
> > +pub use bindings::{
> > +    DRM_MODE_CONNECTOR_Unknown,
> > +    DRM_MODE_CONNECTOR_VGA,
> > +    DRM_MODE_CONNECTOR_DVII,
> > +    DRM_MODE_CONNECTOR_DVID,
> > +    DRM_MODE_CONNECTOR_DVIA,
> > +    DRM_MODE_CONNECTOR_Composite,
> > +    DRM_MODE_CONNECTOR_SVIDEO,
> > +    DRM_MODE_CONNECTOR_LVDS,
> > +    DRM_MODE_CONNECTOR_Component,
> > +    DRM_MODE_CONNECTOR_9PinDIN,
> > +    DRM_MODE_CONNECTOR_DisplayPort,
> > +    DRM_MODE_CONNECTOR_HDMIA,
> > +    DRM_MODE_CONNECTOR_HDMIB,
> > +    DRM_MODE_CONNECTOR_TV,
> > +    DRM_MODE_CONNECTOR_eDP,
> > +    DRM_MODE_CONNECTOR_VIRTUAL,
> > +    DRM_MODE_CONNECTOR_DSI,
> > +    DRM_MODE_CONNECTOR_DPI,
> > +    DRM_MODE_CONNECTOR_WRITEBACK,
> > +    DRM_MODE_CONNECTOR_SPI,
> > +    DRM_MODE_CONNECTOR_USB,
> > +};
> > +
> > +/// A DRM connector implementation
> > +pub trait DriverConnector: Send + Sync + Sized {
> > +    /// The return type of the new() function. Should be `impl
> > PinInit<Self, Error>`.
> > +    /// TODO: Remove this when return_position_impl_trait_in_trait
> > is stable.
> > +    type Initializer: PinInit<Self, Error>;
> 
> This has been stabilized in 1.75.0, so now you should be able to
> write
> 
>      fn new(dev: &Device<Self::Driver>, args: Self::Args) -> impl
> PinInit<Self, Error>;

Ack for this and the below comment as well!

> 
> > +
> > +    /// The data type to use for passing incoming arguments for
> > new `Connector<T>` instances
> > +    /// Drivers which don't care about this can just use `()`
> > +    type Args;
> > +
> > +    /// The parent driver for this DRM connector implementation
> > +    type Driver: KmsDriver;
> > +
> > +    /// The atomic state implementation for this DRM connector
> > implementation
> > +    type State: DriverConnectorState;
> > +
> > +    /// Create a new instance of the private driver data struct
> > for this connector in-place
> > +    fn new(dev: &Device<Self::Driver>, args: Self::Args) ->
> > Self::Initializer;
> > +
> > +    /// Retrieve a list of available display modes for this
> > connector
> > +    fn get_modes<'a>(
> > +        connector: ConnectorGuard<'a, Self>,
> > +        guard: &ModeConfigGuard<'a, Self::Driver>
> > +    ) -> i32;
> > +}
> 
> [...]
> 
> > diff --git a/rust/kernel/drm/kms/crtc.rs
> > b/rust/kernel/drm/kms/crtc.rs
> > new file mode 100644
> > index 0000000000000..3d072028a4884
> > --- /dev/null
> > +++ b/rust/kernel/drm/kms/crtc.rs
> > @@ -0,0 +1,300 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR MIT
> > +
> > +//! KMS driver abstractions for rust.
> > +
> > +use super::{
> > +    plane::*,
> > +    ModeObject,
> > +    StaticModeObject,
> > +    KmsDriver
> > +};
> > +use crate::{
> > +    bindings,
> > +    drm::{drv::Driver, device::Device},
> > +    device,
> > +    prelude::*,
> > +    types::Opaque,
> > +    init::Zeroable,
> > +    sync::Arc,
> > +    error::to_result,
> > +};
> > +use core::{
> > +    cell::UnsafeCell,
> > +    marker::PhantomPinned,
> > +    ptr::{null, null_mut},
> > +    ops::Deref,
> > +};
> > +use macros::vtable;
> > +
> > +/// A typed KMS CRTC with a specific driver.
> > +#[repr(C)]
> > +#[pin_data]
> > +pub struct Crtc<T: DriverCrtc> {
> > +    // The FFI drm_crtc object
> > +    pub(super) crtc: Opaque<bindings::drm_crtc>,
> > +    /// The driver's private inner data
> > +    #[pin]
> > +    inner: T,
> > +    #[pin]
> > +    _p: PhantomPinned,
> 
> Instead of adding this field, you can mark the `crtc` field above as
> `#[pin]`. This is because of 0b4e3b6f6b79 ("rust: types: make
> `Opaque`
> be `!Unpin`").
> 

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

Powered by blists - more mailing lists