Message-ID: <20250314-bizarre-pearl-herring-dfeb01@houat>
Date: Fri, 14 Mar 2025 13:18:07 +0100
From: Maxime Ripard <mripard@...nel.org>
To: Lyude Paul <lyude@...hat.com>
Cc: dri-devel@...ts.freedesktop.org, rust-for-linux@...r.kernel.org, 
	Danilo Krummrich <dakr@...nel.org>, mcanal@...lia.com, Alice Ryhl <aliceryhl@...gle.com>, 
	Simona Vetter <sima@...ll.ch>, Daniel Almeida <daniel.almeida@...labora.com>, 
	Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, 
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, Benno Lossin <benno.lossin@...ton.me>, 
	Andreas Hindborg <a.hindborg@...nel.org>, Trevor Gross <tmgross@...ch.edu>, 
	Asahi Lina <lina@...hilina.net>, open list <linux-kernel@...r.kernel.org>
Subject: Re: [RFC v3 18/33] rust: drm/kms: Add drm_atomic_state bindings

On Wed, Mar 05, 2025 at 05:59:34PM -0500, Lyude Paul wrote:
> Next up is introducing bindings that we can use to represent the global DRM
> atomic state, along with all of the various object states contained within.
> We do this by introducing a few new concepts: borrowed states, atomic state
> mutators, and atomic state composers.
> 
> To understand these, we need to quickly touch upon the general life of an
> atomic commit. Assuming a driver does its own internal atomic commit, the
> procedure looks something like this:
> 
> * Allocate a new atomic state
> * Duplicate the atomic state of each mode object we want to mutate, and add
>   the duplicated state to the new atomic state
> * Check (possibly more then once) the atomic state, possibly modifying it
>   along the way
> * Commit the atomic state to software (we'll call this commit time). At
>   this point no new objects can be added to the state
> * Finish committing the atomic state to hardware asynchronously
> 
> With this in mind, we introduce AtomicStateMutator and AtomicStateComposer
> (along with leaky variants intended for uses in FFI calls). An
> AtomicStateMutator allows mutating an atomic state but does not allow for
> adding new objects to the state. Subsequently, an AtomicStateComposer
> allows for both mutating an atomic state and adding new mode objects. We
> control when we expose each of these types in order to implement the
> limitations required by the aforementioned example.
> 
> Note as well that AtomicStateComposer is intended to eventually be usable
> directly by drivers. In this scenario, a driver will be able to create an
> AtomicStateComposer (the equivalent of allocating an atomic state in C) and
> then commit it by passing it to our DRM bindings by-value, insuring that
> once the commit process begins it is impossible to keep using the
> AtomicStateComposer.
> 
> The next part of this is allowing users to modify the atomic states of all
> of the objects contained within an atomic state. Since it's an extremely
> common usecase for objects to mutate the atomic state of multiple objects
> at once in an unpredictable order, we need a mechanism that will allow us
> to hand out &mut references to each state while ensuring at runtime that we
> do not break rust's data aliasing rules (which disallow us from ever having
> more then one &mut reference to the same piece of data).
> 
> We do this by introducing the concept of a "borrowed" state. This is a very
> similar concept to RefCell, where it is ensured during runtime that when a
> &mut reference is taken out another one cannot be created until the
> corresponding Ref object has been dropped. Our equivalent Ref types are
> ConnectorState, BorrowedCrtcState, and BorrowedPlaneStateMutator.
> 
> Each one of these types can be used in the same manner as a Ref - no
> additional borrows for an atomic state may be taken until the existing one
> has been dropped. Subsequently, all of these types implement their
> respective AsRaw* and FromRaw* counter-parts - and allow dereferencing to
> each driver-private data structure for fully qualified borrows (like
> CrtcState<'a, CrtcStateMutator<T>>. This allows a pretty clean way of
> mutating multiple states at once without ever breaking rust's mutability
> rules.
> 
> We'll use all of these types over the next few commits to begin introducing
> various atomic modeset callbacks to each mode object type.
> 
> Signed-off-by: Lyude Paul <lyude@...hat.com>
> 
> ---
> 
> V3:
> * Drop the TODO about printing a kernel error in ConnectorStateMutator
>   I thought this was something we'd want early on in designing this, but
>   since then I'm pretty sure we just want to return None - there are valid
>   cases where we'd get None while doing connector iteration through an
>   atomic state
> * Improve safety comments in ConnectorStateMutator::new()
> * Rename Borrowed*State to *StateMutator
>   I think this makes things a lot clearer, as well - cleanup the
>   documentation regarding this.
> * Drop plane state iterator for now. It's not that we don't need this, it's
>   just that I haven't actually finished writing these up for all types so
>   I'd rather focus on that later now that we've demonstrated it's a thing
>   that is possible. And it at least shouldn't be needed for getting these
>   bindings/rvkms upstream.
> * Drop num_plane() for the time being
>   Without the plane state iterator in this patch series there's no current
>   usecase for this, so just drop the function for the time being and we'll
>   reintroduce it when it's ready.
> 
> Signed-off-by: Lyude Paul <lyude@...hat.com>
> ---
>  rust/helpers/drm/atomic.c        |  32 +++
>  rust/helpers/drm/drm.c           |   3 +
>  rust/kernel/drm/kms.rs           |   1 +
>  rust/kernel/drm/kms/atomic.rs    | 359 +++++++++++++++++++++++++++++++
>  rust/kernel/drm/kms/connector.rs | 104 ++++++++-
>  rust/kernel/drm/kms/crtc.rs      | 103 ++++++++-
>  rust/kernel/drm/kms/plane.rs     | 105 ++++++++-
>  7 files changed, 700 insertions(+), 7 deletions(-)
>  create mode 100644 rust/helpers/drm/atomic.c
>  create mode 100644 rust/kernel/drm/kms/atomic.rs
> 
> diff --git a/rust/helpers/drm/atomic.c b/rust/helpers/drm/atomic.c
> new file mode 100644
> index 0000000000000..fff70053f6943
> --- /dev/null
> +++ b/rust/helpers/drm/atomic.c
> @@ -0,0 +1,32 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <drm/drm_atomic.h>
> +
> +void rust_helper_drm_atomic_state_get(struct drm_atomic_state *state)
> +{
> +	drm_atomic_state_get(state);
> +}
> +
> +void rust_helper_drm_atomic_state_put(struct drm_atomic_state *state)
> +{
> +	drm_atomic_state_put(state);
> +}
> +
> +// Macros for generating one repetitive atomic state accessors (like drm_atomic_get_new_plane_state)
> +#define STATE_FUNC(type, tense)                                                                     \
> +	struct drm_ ## type ## _state *rust_helper_drm_atomic_get_ ## tense ## _ ## type ## _state( \
> +		const struct drm_atomic_state *state,                                               \
> +		struct drm_ ## type *type                                                           \
> +	) {                                                                                         \
> +		return drm_atomic_get_## tense ## _ ## type ## _state(state, type);                 \
> +	}
> +#define STATE_FUNCS(type) \
> +	STATE_FUNC(type, new); \
> +	STATE_FUNC(type, old);
> +
> +STATE_FUNCS(plane);
> +STATE_FUNCS(crtc);
> +STATE_FUNCS(connector);
> +
> +#undef STATE_FUNCS
> +#undef STATE_FUNC
> diff --git a/rust/helpers/drm/drm.c b/rust/helpers/drm/drm.c
> index 028b8ab429572..365f6807774d4 100644
> --- a/rust/helpers/drm/drm.c
> +++ b/rust/helpers/drm/drm.c
> @@ -1,5 +1,8 @@
>  // SPDX-License-Identifier: GPL-2.0
>  
> +#ifdef CONFIG_DRM_KMS_HELPER
> +#include "atomic.c"
> +#endif
>  #include "gem.c"
>  #ifdef CONFIG_DRM_GEM_SHMEM_HELPER
>  #include "gem_shmem_helper.c"
> diff --git a/rust/kernel/drm/kms.rs b/rust/kernel/drm/kms.rs
> index 1d6ca9c92659a..978bb6712ffbe 100644
> --- a/rust/kernel/drm/kms.rs
> +++ b/rust/kernel/drm/kms.rs
> @@ -2,6 +2,7 @@
>  
>  //! KMS driver abstractions for rust.
>  
> +pub mod atomic;
>  pub mod connector;
>  pub mod crtc;
>  pub mod encoder;
> diff --git a/rust/kernel/drm/kms/atomic.rs b/rust/kernel/drm/kms/atomic.rs
> new file mode 100644
> index 0000000000000..3d5c70dbc4274
> --- /dev/null
> +++ b/rust/kernel/drm/kms/atomic.rs
> @@ -0,0 +1,359 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +
> +//! [`struct drm_atomic_state`] related bindings for rust.
> +//!
> +//! [`struct drm_atomic_state`]: srctree/include/drm/drm_atomic.h
> +use super::{connector::*, crtc::*, plane::*, KmsDriver, ModeObject};
> +use crate::{
> +    bindings,
> +    drm::device::Device,
> +    error::{from_err_ptr, to_result},
> +    prelude::*,
> +    types::*,
> +};
> +use core::{cell::Cell, marker::*, mem::ManuallyDrop, ops::*, ptr::NonNull};
> +
> +/// The main wrapper around [`struct drm_atomic_state`].
> +///
> +/// This type is usually embedded within another interface such as an [`AtomicStateMutator`].
> +///
> +/// # Invariants
> +///
> +/// - The data layout of this type is identical to [`struct drm_atomic_state`].
> +/// - `state` is initialized for as long as this type is exposed to users.
> +///
> +/// [`struct drm_atomic_state`]: srctree/include/drm/drm_atomic.h
> +#[repr(transparent)]
> +pub struct AtomicState<T: KmsDriver> {
> +    pub(super) state: Opaque<bindings::drm_atomic_state>,
> +    _p: PhantomData<T>,
> +}
> +
> +impl<T: KmsDriver> AtomicState<T> {
> +    /// Reconstruct an immutable reference to an atomic state from the given pointer
> +    ///
> +    /// # Safety
> +    ///
> +    /// `ptr` must point to a valid initialized instance of [`struct drm_atomic_state`].
> +    ///
> +    /// [`struct drm_atomic_state`]: srctree/include/drm/drm_atomic.h
> +    #[allow(dead_code)]
> +    pub(super) unsafe fn from_raw<'a>(ptr: *const bindings::drm_atomic_state) -> &'a Self {
> +        // SAFETY: Our data layout is identical
> +        // INVARIANT: Our safety contract upholds the guarantee that `state` is initialized for as
> +        // long as this type is exposed to users.
> +        unsafe { &*ptr.cast() }
> +    }
> +
> +    pub(crate) fn as_raw(&self) -> *mut bindings::drm_atomic_state {
> +        self.state.get()
> +    }
> +
> +    /// Return the [`Device`] associated with this [`AtomicState`].
> +    pub fn drm_dev(&self) -> &Device<T> {
> +        // SAFETY:
> +        // - `state` is initialized via our type invariants.
> +        // - `dev` is invariant throughout the lifetime of `AtomicState`
> +        unsafe { Device::borrow((*self.state.get()).dev) }
> +    }
> +
> +    /// Return the old atomic state for `crtc`, if it is present within this [`AtomicState`].
> +    pub fn get_old_crtc_state<C>(&self, crtc: &C) -> Option<&C::State>
> +    where
> +        C: ModesettableCrtc + ModeObject<Driver = T>,
> +    {
> +        // SAFETY: This function either returns NULL or a valid pointer to a `drm_crtc_state`
> +        unsafe {
> +            bindings::drm_atomic_get_old_crtc_state(self.as_raw(), crtc.as_raw())
> +                .as_ref()
> +                .map(|p| C::State::from_raw(p))
> +        }
> +    }
> +
> +    /// Return the old atomic state for `plane`, if it is present within this [`AtomicState`].
> +    pub fn get_old_plane_state<P>(&self, plane: &P) -> Option<&P::State>
> +    where
> +        P: ModesettablePlane + ModeObject<Driver = T>,
> +    {
> +        // SAFETY: This function either returns NULL or a valid pointer to a `drm_plane_state`
> +        unsafe {
> +            bindings::drm_atomic_get_old_plane_state(self.as_raw(), plane.as_raw())
> +                .as_ref()
> +                .map(|p| P::State::from_raw(p))
> +        }
> +    }
> +
> +    /// Return the old atomic state for `connector` if it is present within this [`AtomicState`].
> +    pub fn get_old_connector_state<C>(&self, connector: &C) -> Option<&C::State>
> +    where
> +        C: ModesettableConnector + ModeObject<Driver = T>,
> +    {
> +        // SAFETY: This function either returns NULL or a valid pointer to a `drm_connector_state`.
> +        unsafe {
> +            bindings::drm_atomic_get_old_connector_state(self.as_raw(), connector.as_raw())
> +                .as_ref()
> +                .map(|p| C::State::from_raw(p))
> +        }
> +    }
> +}
> +
> +// SAFETY: DRM atomic state objects are always reference counted and the get/put functions satisfy
> +// the requirements.
> +unsafe impl<T: KmsDriver> AlwaysRefCounted for AtomicState<T> {
> +    fn inc_ref(&self) {
> +        // SAFETY: `state` is initialized for as long as this type is exposed to users
> +        unsafe { bindings::drm_atomic_state_get(self.state.get()) }
> +    }
> +
> +    unsafe fn dec_ref(obj: NonNull<Self>) {
> +        // SAFETY: `obj` contains a valid non-null pointer to an initialized `Self`.
> +        unsafe { bindings::drm_atomic_state_put(obj.as_ptr().cast()) }
> +    }
> +}
> +
> +/// A smart-pointer for modifying the contents of an atomic state.
> +///
> +/// As it's not unreasonable for a modesetting driver to want to have references to the state of
> +/// multiple modesetting objects at once, along with mutating multiple states for unique modesetting
> +/// objects at once, this type provides a mechanism for safely doing both of these things.
> +///
> +/// To honor Rust's aliasing rules regarding mutable references, this structure ensures only one
> +/// mutable reference to a mode object's atomic state may exist at a time - and refuses to provide
> +/// another if one has already been taken out using runtime checks.
> +pub struct AtomicStateMutator<T: KmsDriver> {
> +    /// The state being mutated. Note that the use of `ManuallyDrop` here is because mutators are
> +    /// only constructed in FFI callbacks and thus borrow their references to the atomic state from
> +    /// DRM. Composers, which make use of mutators internally, can potentially be owned by rust code
> +    /// if a driver is performing an atomic commit internally - and thus will call the drop
> +    /// implementation here.
> +    state: ManuallyDrop<ARef<AtomicState<T>>>,
> +
> +    /// Bitmask of borrowed CRTC state objects
> +    pub(super) borrowed_crtcs: Cell<u32>,
> +    /// Bitmask of borrowed plane state objects
> +    pub(super) borrowed_planes: Cell<u32>,
> +    /// Bitmask of borrowed connector state objects
> +    pub(super) borrowed_connectors: Cell<u32>,
> +}
> +
> +impl<T: KmsDriver> AtomicStateMutator<T> {
> +    /// Construct a new [`AtomicStateMutator`]
> +    ///
> +    /// # Safety
> +    ///
> +    /// `ptr` must point to a valid `drm_atomic_state`
> +    #[allow(dead_code)]
> +    pub(super) unsafe fn new(ptr: NonNull<bindings::drm_atomic_state>) -> Self {
> +        Self {
> +            // SAFETY: The data layout of `AtomicState<T>` is identical to drm_atomic_state
> +            // We use `ManuallyDrop` because `AtomicStateMutator` is only ever provided to users in
> +            // the context of KMS callbacks. As such, skipping ref inc/dec for the atomic state is
> +            // convienent for our bindings.
> +            state: ManuallyDrop::new(unsafe { ARef::from_raw(ptr.cast()) }),
> +            borrowed_planes: Cell::default(),
> +            borrowed_crtcs: Cell::default(),
> +            borrowed_connectors: Cell::default(),
> +        }
> +    }
> +
> +    pub(crate) fn as_raw(&self) -> *mut bindings::drm_atomic_state {
> +        self.state.as_raw()
> +    }
> +
> +    /// Return the [`Device`] for this [`AtomicStateMutator`].
> +    pub fn drm_dev(&self) -> &Device<T> {
> +        self.state.drm_dev()
> +    }
> +
> +    /// Retrieve the last committed atomic state for `crtc` if `crtc` has already been added to the
> +    /// atomic state being composed.
> +    ///
> +    /// Returns `None` otherwise.
> +    pub fn get_old_crtc_state<C>(&self, crtc: &C) -> Option<&C::State>
> +    where
> +        C: ModesettableCrtc + ModeObject<Driver = T>,
> +    {
> +        self.state.get_old_crtc_state(crtc)
> +    }
> +
> +    /// Retrieve the last committed atomic state for `connector` if `connector` has already been
> +    /// added to the atomic state being composed.
> +    ///
> +    /// Returns `None` otherwise.
> +    pub fn get_old_connector_state<C>(&self, connector: &C) -> Option<&C::State>
> +    where
> +        C: ModesettableConnector + ModeObject<Driver = T>,
> +    {
> +        self.state.get_old_connector_state(connector)
> +    }
> +
> +    /// Retrieve the last committed atomic state for `plane` if `plane` has already been added to
> +    /// the atomic state being composed.
> +    ///
> +    /// Returns `None` otherwise.
> +    pub fn get_old_plane_state<P>(&self, plane: &P) -> Option<&P::State>
> +    where
> +        P: ModesettablePlane + ModeObject<Driver = T>,
> +    {
> +        self.state.get_old_plane_state(plane)
> +    }
> +
> +    /// Return a composer for `plane`s new atomic state if it was previously added to the atomic
> +    /// state being composed.
> +    ///
> +    /// Returns `None` otherwise, or if another mutator still exists for this state.
> +    pub fn get_new_crtc_state<C>(&self, crtc: &C) -> Option<CrtcStateMutator<'_, C::State>>
> +    where
> +        C: ModesettableCrtc + ModeObject<Driver = T>,
> +    {
> +        // SAFETY: DRM either returns NULL or a valid pointer to a `drm_crtc_state`
> +        let state =
> +            unsafe { bindings::drm_atomic_get_new_crtc_state(self.as_raw(), crtc.as_raw()) };
> +
> +        CrtcStateMutator::<C::State>::new(self, NonNull::new(state)?)
> +    }
> +
> +    /// Return a composer for `plane`s new atomic state if it was previously added to the atomic
> +    /// state being composed.
> +    ///
> +    /// Returns `None` otherwise, or if another mutator still exists for this state.
> +    pub fn get_new_plane_state<P>(&self, plane: &P) -> Option<PlaneStateMutator<'_, P::State>>
> +    where
> +        P: ModesettablePlane + ModeObject<Driver = T>,
> +    {
> +        // SAFETY: DRM either returns NULL or a valid pointer to a `drm_plane_state`.
> +        let state =
> +            unsafe { bindings::drm_atomic_get_new_plane_state(self.as_raw(), plane.as_raw()) };
> +
> +        PlaneStateMutator::<P::State>::new(self, NonNull::new(state)?)
> +    }
> +
> +    /// Return a composer for `crtc`s new atomic state if it was previously added to the atomic
> +    /// state being composed.
> +    ///
> +    /// Returns `None` otherwise, or if another mutator still exists for this state.
> +    pub fn get_new_connector_state<C>(
> +        &self,
> +        connector: &C,
> +    ) -> Option<ConnectorStateMutator<'_, C::State>>
> +    where
> +        C: ModesettableConnector + ModeObject<Driver = T>,
> +    {
> +        // SAFETY: DRM either returns NULL or a valid pointer to a `drm_connector_state`
> +        let state = unsafe {
> +            bindings::drm_atomic_get_new_connector_state(self.as_raw(), connector.as_raw())
> +        };
> +
> +        ConnectorStateMutator::<C::State>::new(self, NonNull::new(state)?)
> +    }
> +}
> +
> +/// An [`AtomicStateMutator`] wrapper which is not yet part of any commit operation.
> +///
> +/// Since it's not yet part of a commit operation, new mode objects may be added to the state. It
> +/// also holds a reference to the underlying [`AtomicState`] that will be released when this object
> +/// is dropped.
> +pub struct AtomicStateComposer<T: KmsDriver>(AtomicStateMutator<T>);
> +
> +impl<T: KmsDriver> Deref for AtomicStateComposer<T> {
> +    type Target = AtomicStateMutator<T>;
> +
> +    fn deref(&self) -> &Self::Target {
> +        &self.0
> +    }
> +}
> +
> +impl<T: KmsDriver> Drop for AtomicStateComposer<T> {
> +    fn drop(&mut self) {
> +        // SAFETY: We're in drop, so this is guaranteed to be the last possible reference
> +        unsafe { ManuallyDrop::drop(&mut self.0.state) }
> +    }
> +}
> +
> +impl<T: KmsDriver> AtomicStateComposer<T> {
> +    /// # Safety
> +    ///
> +    /// The caller guarantees that `ptr` points to a valid instance of `drm_atomic_state`.
> +    #[allow(dead_code)]
> +    pub(crate) unsafe fn new(ptr: NonNull<bindings::drm_atomic_state>) -> Self {
> +        // SAFETY: see `AtomicStateMutator::from_raw()`
> +        Self(unsafe { AtomicStateMutator::new(ptr) })
> +    }
> +
> +    /// Attempt to add the state for `crtc` to the atomic state for this composer if it hasn't
> +    /// already been added, and create a mutator for it.
> +    ///
> +    /// If a composer already exists for this `crtc`, this function returns `Error(EBUSY)`. If
> +    /// attempting to add the state fails, another error code will be returned.
> +    pub fn add_crtc_state<C>(&self, crtc: &C) -> Result<CrtcStateMutator<'_, C::State>>
> +    where
> +        C: ModesettableCrtc + ModeObject<Driver = T>,
> +    {
> +        // SAFETY: DRM will only return a valid pointer to a `drm_crtc_state` - or an error.
> +        let state = unsafe {
> +            from_err_ptr(bindings::drm_atomic_get_crtc_state(
> +                self.as_raw(),
> +                crtc.as_raw(),
> +            ))
> +            .map(|c| NonNull::new_unchecked(c))
> +        }?;
> +
> +        CrtcStateMutator::<C::State>::new(self, state).ok_or(EBUSY)
> +    }

I think it should be called get_crtc_state. First because it would be
consistent with the C part of KMS, but also because, to me, add would
take the state and return void. Here, you take no state and return it,
so it's more of a get operation than add.

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (274 bytes)

Powered by blists - more mailing lists