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@xxxxxxxxxx>
> > ---
> > 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
