Re: [RFC v3 04/33] rust: drm/kms: Add drm_connector bindings

[Louis Chauvet <louis.chauvet@bootlin.com>]

On 05/03/25 - 17:59, Lyude Paul wrote:

> +
> +// SAFETY: DRM connectors are refcounted mode objects
> +unsafe impl<T: DriverConnector> RcModeObject for Connector<T> {}
> +
> +// SAFETY:
> +// * Via our type variants our data layout starts with `drm_connector`
> +// * Since we don't expose `Connector` to users before it has been initialized, this and our data
> +//   layout ensure that `as_raw()` always returns a valid pointer to a `drm_connector`.
> +unsafe impl<T: DriverConnector> AsRawConnector for Connector<T> {
> +    fn as_raw(&self) -> *mut bindings::drm_connector {
> +        self.connector.get()
> +    }
> +
> +    unsafe fn from_raw<'a>(ptr: *mut bindings::drm_connector) -> &'a Self {
> +        // SAFETY: Our data layout starts with `bindings::drm_connector`
> +        unsafe { &*ptr.cast() }

I think you should use container_of macro here. It is functionnaly the 
same thing, but it may avoid issue if for whatever reason the ->base is 
not at the exact same address. This will make this function "symetrical" 
with as_raw.

Ditto for the next patch

> +    }
> +}
> +
> +// SAFETY: We only expose this object to users directly after KmsDriver::create_objects has been
> +// called.
> +unsafe impl<T: DriverConnector> ModesettableConnector for Connector<T> {
> +    type State = ConnectorState<T::State>;
> +}
> +
> +/// A [`Connector`] that has not yet been registered with userspace.
> +///
> +/// KMS registration is single-threaded, so this object is not thread-safe.
> +///
> +/// # Invariants
> +///
> +/// - This object can only exist before its respective KMS device has been registered.
> +/// - Otherwise, it inherits all invariants of [`Connector`] and has an identical data layout.

To garantee a data layout, don't you need to add #[repr(transparent)]? 
This will automatically break the compilation if one day NotThreadSafe 
is not ZST.

> +pub struct UnregisteredConnector<T: DriverConnector>(Connector<T>, NotThreadSafe);
> +
> +// SAFETY: We share the invariants of `Connector`
> +unsafe impl<T: DriverConnector> AsRawConnector for UnregisteredConnector<T> {
> +    fn as_raw(&self) -> *mut bindings::drm_connector {
> +        self.0.as_raw()
> +    }
> +
> +    unsafe fn from_raw<'a>(ptr: *mut bindings::drm_connector) -> &'a Self {
> +        // SAFETY: This is another from_raw() call, so this function shares the same safety contract
> +        let connector = unsafe { Connector::<T>::from_raw(ptr) };
> +
> +        // SAFETY: Our data layout is identical via our type invariants.
> +        unsafe { mem::transmute(connector) }

IIRC, to be able to transmute, you need to add some #[repr] on the 
types, so rust is forced to use a "fixed" layout. See above, I think you 
need to add at least repr(transparent) for UnregisteredConnector

> +    }
> +}
> +
> +impl<T: DriverConnector> Deref for UnregisteredConnector<T> {
> +    type Target = T;
> +
> +    fn deref(&self) -> &Self::Target {
> +        &self.0.inner
> +    }
> +}
> +
> +impl<T: DriverConnector> UnregisteredConnector<T> {
> +    /// Construct a new [`UnregisteredConnector`].
> +    ///
> +    /// A driver may use this to create new [`UnregisteredConnector`] objects.
> +    ///
> +    /// [`KmsDriver::create_objects`]: kernel::drm::kms::KmsDriver::create_objects
> +    pub fn new<'a>(
> +        dev: &'a UnregisteredKmsDevice<'a, T::Driver>,
> +        type_: Type,
> +        args: T::Args,
> +    ) -> Result<&'a Self> {
> +        let new: Pin<KBox<Connector<T>>> = KBox::try_pin_init(
> +            try_pin_init!(Connector::<T> {
> +                connector: Opaque::new(bindings::drm_connector {
> +                    helper_private: &T::OPS.helper_funcs,
> +                    ..Default::default()
> +                }),
> +                inner <- T::new(dev, args),
> +                _p: PhantomPinned
> +            }),
> +            GFP_KERNEL,
> +        )?;
> +
> +        // SAFETY:
> +        // - `dev` will hold a reference to the new connector, and thus outlives us.
> +        // - We just allocated `new` above
> +        // - `new` starts with `drm_connector` via its type invariants.

Why do you need to add the third requirement here? This is not part of 
the drm_connector_init requirement. It only requires to have a valid 
pointer.
> +        to_result(unsafe {
> +            bindings::drm_connector_init(dev.as_raw(), new.as_raw(), &T::OPS.funcs, type_ as i32)
> +        })?;
> +
> +        // SAFETY: We don't move anything
> +        let this = unsafe { Pin::into_inner_unchecked(new) };
> +
> +        // We'll re-assemble the box in connector_destroy_callback()
> +        let this = KBox::into_raw(this);
> +
> +        // UnregisteredConnector has an equivalent data layout
> +        let this: *mut Self = this.cast();
> +
> +        // SAFETY: We just allocated the connector above, so this pointer must be valid
> +        Ok(unsafe { &*this })
> +    }
> +}
> +
> +unsafe extern "C" fn connector_destroy_callback<T: DriverConnector>(
> +    connector: *mut bindings::drm_connector,
> +) {
> +    // SAFETY: DRM guarantees that `connector` points to a valid initialized `drm_connector`.
> +    unsafe {
> +        bindings::drm_connector_unregister(connector);
> +        bindings::drm_connector_cleanup(connector);
> +    };
> +
> +    // SAFETY:
> +    // - We originally created the connector in a `Box`
> +    // - We are guaranteed to hold the last remaining reference to this connector
> +    // - This cast is safe via `DriverConnector`s type invariants.
> +    drop(unsafe { KBox::from_raw(connector as *mut Connector<T>) });
> +}
> +
> +// SAFETY: DRM expects this struct to be zero-initialized
> +unsafe impl Zeroable for bindings::drm_connector_state {}
> +
> +/// A trait implemented by any type which can produce a reference to a
> +/// [`struct drm_connector_state`].
> +///
> +/// This is implemented internally by DRM.
> +///
> +/// [`struct drm_connector_state`]: srctree/include/drm/drm_connector.h
> +pub trait AsRawConnectorState: private::AsRawConnectorState {
> +    /// The type that represents this connector state's DRM connector.
> +    type Connector: AsRawConnector;
> +}
> +
> +pub(super) mod private {
> +    use super::*;
> +
> +    /// Trait for retrieving references to the base connector state contained within any connector
> +    /// state compatible type
> +    #[allow(unreachable_pub)]
> +    pub trait AsRawConnectorState {
> +        /// Return an immutable reference to the raw connector state.
> +        fn as_raw(&self) -> &bindings::drm_connector_state;
> +
> +        /// Get a mutable reference to the raw [`struct drm_connector_state`] contained within this
> +        /// type.
> +        ///
> +        ///
> +        /// # Safety
> +        ///
> +        /// The caller promises this mutable reference will not be used to modify any contents of
> +        /// [`struct drm_connector_state`] which DRM would consider to be static - like the
> +        /// backpointer to the DRM connector that owns this state. This also means the mutable
> +        /// reference should never be exposed outside of this crate.
> +        ///
> +        /// [`struct drm_connector_state`]: srctree/include/drm/drm_connector.h
> +        unsafe fn as_raw_mut(&mut self) -> &mut bindings::drm_connector_state;
> +    }
> +}
> +
> +pub(super) use private::AsRawConnectorState as AsRawConnectorStatePrivate;
> +
> +/// A trait implemented for any type which can be constructed directly from a
> +/// [`struct drm_connector_state`] pointer.
> +///
> +/// This is implemented internally by DRM.
> +///
> +/// [`struct drm_connector_state`]: srctree/include/drm/drm_connector.h
> +pub trait FromRawConnectorState: AsRawConnectorState {
> +    /// Get an immutable reference to this type from the given raw [`struct drm_connector_state`]
> +    /// pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// - The caller guarantees `ptr` is contained within a valid instance of `Self`.
> +    /// - The caller guarantees that `ptr` cannot not be modified for the lifetime of `'a`.
> +    ///
> +    /// [`struct drm_connector_state`]: srctree/include/drm/drm_connector.h
> +    unsafe fn from_raw<'a>(ptr: *const bindings::drm_connector_state) -> &'a Self;
> +
> +    /// Get a mutable reference to this type from the given raw [`struct drm_connector_state`]
> +    /// pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// - The caller guarantees that `ptr` is contained within a valid instance of `Self`.
> +    /// - The caller guarantees that `ptr` cannot have any other references taken out for the
> +    ///   lifetime of `'a`.
> +    ///
> +    /// [`struct drm_connector_state`]: srctree/include/drm/drm_connector.h
> +    unsafe fn from_raw_mut<'a>(ptr: *mut bindings::drm_connector_state) -> &'a mut Self;
> +}
> +
> +/// The main interface for a [`struct drm_connector_state`].
> +///
> +/// This type is the main interface for dealing with the atomic state of DRM connectors. In
> +/// addition, it allows access to whatever private data is contained within an implementor's
> +/// [`DriverConnectorState`] type.
> +///
> +/// # Invariants
> +///
> +/// - The DRM C API and our interface guarantees that only the user has mutable access to `state`,
> +///   up until [`drm_atomic_helper_commit_hw_done`] is called. Therefore, `connector` follows rust's
> +///   data aliasing rules and does not need to be behind an [`Opaque`] type.
> +/// - `state` and `inner` initialized for as long as this object is exposed to users.
> +/// - The data layout of this structure begins with [`struct drm_connector_state`].
> +/// - The connector for this atomic state can always be assumed to be of type
> +///   [`Connector<T::Connector>`].
> +///
> +/// [`struct drm_connector_state`]: srctree/include/drm/drm_connector.h
> +/// [`drm_atomic_helper_commit_hw_done`]: srctree/include/drm/drm_atomic_helper.h
> +#[derive(Default)]
> +#[repr(C)]
> +pub struct ConnectorState<T: DriverConnectorState> {
> +    state: bindings::drm_connector_state,
> +    inner: T,
> +}
> +
> +/// The main trait for implementing the [`struct drm_connector_state`] API for a [`Connector`].
> +///
> +/// A driver may store driver-private data within the implementor's type, which will be available
> +/// when using a full typed [`ConnectorState`] object.
> +///
> +/// # Invariants
> +///
> +/// - Any C FFI callbacks generated using this trait are guaranteed that passed-in
> +///   [`struct drm_connector`] pointers are contained within a [`Connector<Self::Connector>`].
> +/// - Any C FFI callbacks generated using this trait are guaranteed that passed-in
> +///   [`struct drm_connector_state`] pointers are contained within a [`ConnectorState<Self>`].
> +///
> +/// [`struct drm_connector`]: srctree/include/drm_connector.h
> +/// [`struct drm_connector_state`]: srctree/include/drm_connector.h
> +pub trait DriverConnectorState: Clone + Default + Sized {
> +    /// The parent [`DriverConnector`].
> +    type Connector: DriverConnector;
> +}
> +
> +impl<T: DriverConnectorState> Sealed for ConnectorState<T> {}
> +
> +impl<T: DriverConnectorState> AsRawConnectorState for ConnectorState<T> {
> +    type Connector = Connector<T::Connector>;
> +}
> +
> +impl<T: DriverConnectorState> private::AsRawConnectorState for ConnectorState<T> {
> +    fn as_raw(&self) -> &bindings::drm_connector_state {
> +        &self.state
> +    }
> +
> +    unsafe fn as_raw_mut(&mut self) -> &mut bindings::drm_connector_state {
> +        &mut self.state
> +    }
> +}
> +
> +impl<T: DriverConnectorState> FromRawConnectorState for ConnectorState<T> {
> +    unsafe fn from_raw<'a>(ptr: *const bindings::drm_connector_state) -> &'a Self {
> +        // Our data layout starts with `bindings::drm_connector_state`.
> +        let ptr: *const Self = ptr.cast();

As for the connector, I think this is a bit safer to use container_of.
And what is the rule about unsafe in unsafe function? I think this 
casting is unsafe, but you did not add the unsafe block around it.

> +
> +        // SAFETY:
> +        // - Our safety contract requires that `ptr` be contained within `Self`.
> +        // - Our safety contract requires the caller ensure that it is safe for us to take an
> +        //   immutable reference.
> +        unsafe { &*ptr }
> +    }
> +
> +    unsafe fn from_raw_mut<'a>(ptr: *mut bindings::drm_connector_state) -> &'a mut Self {
> +        // Our data layout starts with `bindings::drm_connector_state`.
> +        let ptr: *mut Self = ptr.cast();
> +
> +        // SAFETY:
> +        // - Our safety contract requires that `ptr` be contained within `Self`.
> +        // - Our safety contract requires the caller ensure it is safe for us to take a mutable
> +        //   reference.
> +        unsafe { &mut *ptr }
> +    }
> +}
> +
> +unsafe extern "C" fn atomic_duplicate_state_callback<T: DriverConnectorState>(
> +    connector: *mut bindings::drm_connector,
> +) -> *mut bindings::drm_connector_state {
> +    // SAFETY: DRM guarantees that `connector` points to a valid initialized `drm_connector`.
> +    let state = unsafe { (*connector).state };
> +    if state.is_null() {
> +        return null_mut();
> +    }
> +
> +    // SAFETY:
> +    // - We just verified that `state` is non-null
> +    // - This cast is guaranteed to be safe via our type invariants.
> +    let state = unsafe { ConnectorState::<T>::from_raw(state) };
> +
> +    let new = Box::try_init(
> +        try_init!(ConnectorState::<T> {
> +            state: bindings::drm_connector_state {
> +                ..Default::default()
> +            },
> +            inner: state.inner.clone()
> +        }),
> +        GFP_KERNEL,
> +    );
> +
> +    if let Ok(mut new) = new {
> +        // SAFETY:
> +        // - `new` provides a valid pointer to a newly allocated `drm_plane_state` via type

s/plane/connector/

> +        //   invariants
> +        // - This initializes `new` via memcpy()
> +        unsafe {
> +            bindings::__drm_atomic_helper_connector_duplicate_state(connector, new.as_raw_mut())
> +        };
> +
> +        KBox::into_raw(new).cast()
> +    } else {
> +        null_mut()
> +    }
> +}
> +
> +unsafe extern "C" fn atomic_destroy_state_callback<T: DriverConnectorState>(
> +    _connector: *mut bindings::drm_connector,
> +    connector_state: *mut bindings::drm_connector_state,
> +) {
> +    // SAFETY: DRM guarantees that `state` points to a valid instance of `drm_connector_state`
> +    unsafe { bindings::__drm_atomic_helper_connector_destroy_state(connector_state) };
> +
> +    // SAFETY:
> +    // - DRM guarantees we are the only one with access to this `drm_connector_state`
> +    // - This cast is safe via our type invariants.
> +    drop(unsafe { KBox::from_raw(connector_state.cast::<ConnectorState<T>>()) });
> +}
> +
> +unsafe extern "C" fn connector_reset_callback<T: DriverConnectorState>(
> +    connector: *mut bindings::drm_connector,
> +) {
> +    // SAFETY: DRM guarantees that `state` points to a valid instance of `drm_connector_state`
> +    let state = unsafe { (*connector).state };
> +    if !state.is_null() {
> +        // SAFETY:
> +        // - We're guaranteed `connector` is `Connector<T>` via type invariants
> +        // - We're guaranteed `state` is `ConnectorState<T>` via type invariants.
> +        unsafe { atomic_destroy_state_callback::<T>(connector, state) }
> +
> +        // SAFETY: No special requirements here, DRM expects this to be NULL
> +        unsafe { (*connector).state = null_mut() };
> +    }
> +
> +    // Unfortunately, this is the best we can do at the moment as this FFI callback was mistakenly
> +    // presumed to be infallible :(
> +    let new = KBox::new(ConnectorState::<T>::default(), GFP_KERNEL).expect("Blame the API, sorry!");
> +
> +    // DRM takes ownership of the state from here, resets it, and then assigns it to the connector
> +    // SAFETY:
> +    // - DRM guarantees that `connector` points to a valid instance of `drm_connector`.
> +    // - The cast to `drm_connector_state` is safe via `ConnectorState`s type invariants.
> +    unsafe { bindings::__drm_atomic_helper_connector_reset(connector, Box::into_raw(new).cast()) };
> +}
> -- 
> 2.48.1
> 
> 

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com