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@xxxxxxxxxx>
>
> ---
>
> 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@xxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: PGP signature
