Message-Id: <653F8E8D-E21A-4448-91F4-81F02DB28746@collabora.com>
Date: Wed, 27 Nov 2024 17:54:38 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Lyude Paul <lyude@...hat.com>
Cc: dri-devel@...ts.freedesktop.org,
 rust-for-linux@...r.kernel.org,
 Asahi Lina <lina@...hilina.net>,
 Danilo Krummrich <dakr@...nel.org>,
 mcanal@...lia.com,
 airlied@...hat.com,
 zhiw@...dia.com,
 cjia@...dia.com,
 jhubbard@...dia.com,
 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>,
 Benno Lossin <benno.lossin@...ton.me>,
 Andreas Hindborg <a.hindborg@...sung.com>,
 Alice Ryhl <aliceryhl@...gle.com>,
 Trevor Gross <tmgross@...ch.edu>,
 open list <linux-kernel@...r.kernel.org>
Subject: Re: [WIP RFC v2 20/35] WIP: rust: drm/kms: Add drm_atomic_state
 bindings

Hi Lyude

> On 30 Sep 2024, at 20:10, Lyude Paul <lyude@...hat.com> 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

nit: typo here s/then/than

>  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).

nit: same typo here

> 
> 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
> BorrowedConnectorState, BorrowedCrtcState, and BorrowedPlaneState.

Are these `composers` as well? 

> 
> Each one of these types can be used in the same manner as a Ref - no

You mean core::cell::Ref, right? Maybe worth spelling out the full type.

> 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
> BorrowedCrtcState<'a, CrtcState<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>
> 
> ---
> 
> TODO:
> * Finish adding state iterators
>  We only have one iterator for planes right now, but the plan is to have
>  iterators for all types and have two different kind of iterators:

I assume you mean iterators for Connectors and Crtcs, right?

>  * State object iterators
>    Basically, these just iterate through all of the mode objects of a
>    specific type present in an atomic state. Currently this is what our
>    plane iterator does.
>  * State mutator iterators
>    With the existence of AtomicStateMutator and friends, it makes sense to
>    have a type of iterator that:
>    * Only iterates through unborrowed atomic states, removing the need to
>      deal with the Option<> that get_new_*_state() functions return
>    * Returns each (object, old_state, new_state) triplet as a dedicated
>      type (PlaneUpdate, CrtcUpdate, ConnectorUpdate) that can be upcasted
>      from an Opaque type using a single call. This is desirable, as it
>      would make iterating through objects with a specific Driver*
>      implementation as easy as just adding a .filter_map() call to the
>      iterator.
>  * Upcast functions for the Borrowed* types
> 
> 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           |   9 +
> rust/kernel/drm/kms/atomic.rs    | 419 +++++++++++++++++++++++++++++++
> rust/kernel/drm/kms/connector.rs |  75 ++++++
> rust/kernel/drm/kms/crtc.rs      |  75 ++++++
> rust/kernel/drm/kms/plane.rs     |  77 ++++++
> 7 files changed, 690 insertions(+)
> 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 d74267c78864f..4ab039d67352e 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;
> @@ -248,6 +249,14 @@ pub(crate) fn mode_config_mutex(&self) -> &Mutex<()> {
>     pub fn mode_config_lock(&self) -> ModeConfigGuard<'_, T> {
>         ModeConfigGuard(self.mode_config_mutex().lock(), PhantomData)
>     }
> +
> +    /// Return the number of registered [`Plane`](plane::Plane) objects on this [`Device`].
> +    #[inline]
> +    pub fn num_plane(&self) -> i32 {
> +        // SAFETY: The only context which this could change is before registration, which must be
> +        // single-threaded anyway - so it's safe to just read this value

I think you can limit the scope of the unsafe block here, IIUC the pointer dereference is the only unsafe part?

Also, you should probably mention that `as_raw()` returns a valid pointer.

> +        unsafe { (*self.as_raw()).mode_config.num_total_plane }
> +    }
> }
> 
> /// A modesetting object in DRM.
> diff --git a/rust/kernel/drm/kms/atomic.rs b/rust/kernel/drm/kms/atomic.rs
> new file mode 100644
> index 0000000000000..a4354b89b07cc
> --- /dev/null
> +++ b/rust/kernel/drm/kms/atomic.rs
> @@ -0,0 +1,419 @@
> +// 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 alloc::boxed::Box;
> +use crate::{
> +    bindings,
> +    drm::{device::Device, drv::Driver},
> +    types::*,
> +    error::{from_err_ptr, to_result, from_result},
> +    init::Zeroable,
> +    prelude::*,
> +    private::Sealed
> +};
> +use core::{
> +    marker::*,
> +    ptr::NonNull,
> +    cell::Cell,
> +    ffi::*,
> +    slice,
> +    ops::*,
> +    mem::ManuallyDrop,
> +    iter::Iterator,
> +};
> +use super::{
> +    crtc::*,
> +    connector::*,
> +    plane::*,
> +    Kms,
> +    KmsDriver,
> +    ModeObject
> +};
> +
> +/// 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
> +    pub(super) unsafe fn from_raw<'a>(ptr: *const bindings::drm_atomic_state) -> &'a Self {
> +        // SAFETY: Our data layout is identical
> +        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: AsRawCrtc<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| unsafe { 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: AsRawPlane<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| unsafe { 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: AsRawConnector<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| unsafe { 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: FFI call with no special requirements
> +        unsafe { bindings::drm_atomic_state_get(self.state.get()) }
> +    }
> +
> +    unsafe fn dec_ref(obj: NonNull<Self>) {
> +        // SAFETY: FFI calls with no special requirements
> +        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>>>,

The last part was a bit difficult to understand. What do you mean by `will call the drop implementation here` ?

> +
> +    /// 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`
> +    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
> +            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: AsRawCrtc<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: AsRawConnector<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: AsRawPlane<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 a composer still exists for this state.
> +    pub fn get_new_crtc_state<C>(&self, crtc: &C) -> Option<BorrowedCrtcState<'_, C::State>>
> +    where
> +        C: AsRawCrtc<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())
> +        };
> +
> +        BorrowedCrtcState::<C::State>::new(self, NonNull::new(state)?)
> +    }

You say that this function returns a composer, but..

> +
> +    /// 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 a composer still exists for this state.
> +    pub fn get_new_plane_state<P>(&self, plane: &P) -> Option<BorrowedPlaneState<'_, P::State>>
> +    where
> +        P: AsRawPlane<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())
> +        };
> +
> +        BorrowedPlaneState::<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 a composer still exists for this state.
> +    pub fn get_new_connector_state<C>(
> +        &self,
> +        connector: &C
> +    ) -> Option<BorrowedConnectorState<'_, C::State>>
> +    where
> +        C: AsRawConnector<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())
> +        };
> +
> +        BorrowedConnectorState::<C::State>::new(self, NonNull::new(state)?)
> +    }
> +
> +    /// Iterate through each of the planes (regardless of type) currently in this atomic state.
> +    pub fn iter_planes(&self) -> AtomicStatePlaneIter<'_, T> {
> +        AtomicStatePlaneIter::new(&self.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>);

…the composer seems to be its own type? Maybe to ease the confusion a bit, you can
say that `get_new_crtc_state` returns a composer in its borrowed form?

> +
> +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) }

Ok, so the ManuallyDrop is actually dropped here, which is probably what you meant in the comment
in `AtomicStateMutator.

You should probably reference `AtomicStateComposer::drop()` directly instead of saying `here`, specially
as it’s another type entirely.

> +    }
> +}
> +
> +impl<T: KmsDriver> AtomicStateComposer<T> {
> +    /// # Safety
> +    ///
> +    /// The caller guarantees that `ptr` points to a valid instance of `drm_atomic_state`.
> +    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.


`get_new_crtc_state` returns `Option<BorrowedCrtcState<'_, C::State>>`, which you labeled a `composer`.

Now, the same type is being returned, but it’s being referenced to as a `mutator` ?

I am sure these things make perfect sense for experienced KMS developers, but it’s a bit confusing to other
reviewers at first.

> +    ///
> +    /// 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<BorrowedCrtcState<'_, C::State>>
> +    where
> +        C: AsRawCrtc<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))
> +        }?;
> +
> +        BorrowedCrtcState::<C::State>::new(self, state).ok_or(EBUSY)
> +    }
> +
> +    /// Attempt to add the state for `plane` 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 `plane`, this function returns `Error(EBUSY)`. If
> +    /// attempting to add the state fails, another error code will be returned.
> +    pub fn add_plane_state<P>(&self, plane: &P) -> Result<BorrowedPlaneState<'_, P::State>>
> +    where
> +        P: AsRawPlane<Driver = T>,
> +    {
> +        // SAFETY: DRM will only return a valid pointer to a [`drm_plane_state`] - or an error.
> +        let state = unsafe {
> +            from_err_ptr(
> +                bindings::drm_atomic_get_plane_state(self.as_raw(), plane.as_raw())
> +            ).map(|p| NonNull::new_unchecked(p))
> +        }?;
> +
> +        BorrowedPlaneState::<P::State>::new(self, state).ok_or(EBUSY)
> +    }
> +
> +    /// Attempt to add the state for `connector` 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 `connector`, this function returns `Error(EBUSY)`. If
> +    /// attempting to add the state fails, another error code will be returned.
> +    pub fn add_connector_state<C>(
> +        &self,
> +        connector: &C
> +    ) -> Result<BorrowedConnectorState<'_, C::State>>
> +    where
> +        C: AsRawConnector<Driver = T>,
> +    {
> +        // SAFETY: DRM will only return a valid pointer to a [`drm_plane_state`] - or an error.
> +        let state = unsafe {
> +            from_err_ptr(
> +                bindings::drm_atomic_get_connector_state(self.as_raw(), connector.as_raw())
> +            ).map(|c| NonNull::new_unchecked(c))
> +        }?;
> +
> +        BorrowedConnectorState::<C::State>::new(self, state).ok_or(EBUSY)
> +    }
> +
> +    /// Attempt to add any planes affected by changes on `crtc` to this [`AtomicStateComposer`].
> +    ///
> +    /// Will return an [`Error`] if this fails.
> +    pub fn add_affected_planes(&self, crtc: &impl AsRawCrtc<Driver = T>) -> Result {
> +        // SAFETY: FFI call with no special safety requirements
> +        to_result(unsafe {
> +            bindings::drm_atomic_add_affected_planes(self.as_raw(), crtc.as_raw())
> +        })
> +    }
> +}
> +
> +/// An iterator which goes through each DRM plane currently in an atomic state.
> +///
> +/// Note that this iterator will return [`OpaquePlane`]s, because it's entirely possible for a
> +/// driver to have multiple implementations of [`DriverPlane`] - so we don't know what the fully
> +/// qualified type of each plane is.
> +pub struct AtomicStatePlaneIter<'a, T: KmsDriver> {
> +    state: &'a AtomicState<T>,
> +    current_idx: u8,
> +}
> +
> +impl<'a, T: KmsDriver> Iterator for AtomicStatePlaneIter<'a, T> {
> +    type Item = &'a OpaquePlane<T>;
> +
> +    fn next(&mut self) -> Option<Self::Item> {
> +        let ptr = self.state.state.get();
> +
> +        // SAFETY: `planes` is initialized by the time we expose AtomicState<T> through any form to
> +        // users. And because we don't allow state mutation outside of mutators, which are single
> +        // threaded, the contents of this struct are at least guaranteed not to change through the
> +        // duration of this borrow.
> +        let planes: &[bindings::__drm_planes_state] = unsafe {
> +            slice::from_raw_parts((*ptr).planes.cast_const(), self.state.drm_dev().num_plane() as _)
> +        };
> +
> +        for plane_states in &planes[self.current_idx as _..] {
> +            self.current_idx += 1;
> +            if !plane_states.ptr.is_null() {
> +                // SAFETY: OpaquePlane has an identical data layout, and its only possible values
> +                // are NULL or pointing at a valid drm_plane
> +                return Some(unsafe { OpaquePlane::from_raw(plane_states.ptr) });
> +            }
> +        }
> +
> +        None
> +    }
> +}
> +
> +impl<'a, T: KmsDriver> AtomicStatePlaneIter<'a, T> {
> +    fn new(state: &'a AtomicState<T>) -> AtomicStatePlaneIter<'a, T> {
> +        Self {
> +            current_idx: 0,
> +            state
> +        }
> +    }
> +}
> diff --git a/rust/kernel/drm/kms/connector.rs b/rust/kernel/drm/kms/connector.rs
> index f62740d7f6469..6fcfce8b48c64 100644
> --- a/rust/kernel/drm/kms/connector.rs
> +++ b/rust/kernel/drm/kms/connector.rs
> @@ -31,6 +31,7 @@
>     ModeConfigGuard,
>     encoder::*,
>     KmsDriver,
> +    atomic::*,
> };
> use macros::pin_data;
> 
> @@ -679,6 +680,80 @@ unsafe fn from_raw_mut<'a>(ptr: *mut bindings::drm_connector_state) -> &'a mut S
>     }
> }
> 
> +/// An interface for mutating a [`Connector`]s atomic state.
> +///
> +/// This type is typically returned by an [`AtomicStateMutator`] within contexts where it is
> +/// possible to safely mutate a connector's state. In order to uphold rust's data-aliasing rules,
> +/// only [`BorrowedConnectorState`] may exist at a time.

Maybe you mean that only one [`BorrowedConnectorState`] may exist at a time? In which case
the word `one` is missing.

> +pub struct BorrowedConnectorState<'a, T: FromRawConnectorState> {
> +    state: &'a mut T,
> +    mask: &'a Cell<u32>
> +}
> +
> +impl<'a, T: FromRawConnectorState> BorrowedConnectorState<'a, T> {
> +    pub(super) fn new<D: KmsDriver>(
> +        mutator: &'a AtomicStateMutator<D>,
> +        state: NonNull<bindings::drm_connector_state>
> +    ) -> Option<Self> {
> +        // SAFETY: `connector` is invariant throughout the lifetime of the atomic state, is

Maybe that’s only me, but every time you say `invariant`, my mind jumps to variance as in [0].


> +        // initialized by this point, and we're guaranteed it is of type `OpaqueConnector<T>` by
> +        // type invariance
> +        let connector = unsafe { T::Connector::from_raw((*state.as_ptr()).connector) };
> +        let conn_mask = connector.mask();
> +        let borrowed_mask = mutator.borrowed_connectors.get();
> +
> +        if borrowed_mask & conn_mask == 0 {
> +            mutator.borrowed_connectors.set(borrowed_mask | conn_mask);
> +            Some(Self {
> +                mask: &mutator.borrowed_connectors,
> +                // SAFETY: We're guaranteed `state` is of `ConnectorState<T>` by type invariance,
> +                // and we just confirmed by checking `borrowed_connectors` that no other mutable
> +                // borrows have been taken out for `state`
> +                state: unsafe { T::from_raw_mut(state.as_ptr()) },
> +            })
> +        } else {
> +            // TODO: Print a kernel warning here, this is a user error
> +            None
> +        }
> +    }
> +}
> +
> +impl<'a, T: DriverConnectorState> Deref for BorrowedConnectorState<'a, ConnectorState<T>> {
> +    type Target = T;
> +
> +    fn deref(&self) -> &Self::Target {
> +        &self.state.inner
> +    }
> +}
> +
> +impl<'a, T: DriverConnectorState> DerefMut for BorrowedConnectorState<'a, ConnectorState<T>> {
> +    fn deref_mut(&mut self) -> &mut Self::Target {
> +        &mut self.state.inner
> +    }
> +}
> +
> +impl<'a, T: FromRawConnectorState> Drop for BorrowedConnectorState<'a, T> {
> +    fn drop(&mut self) {
> +        let mask = self.state.connector().mask();
> +        self.mask.set(self.mask.get() & !mask);
> +    }
> +}
> +
> +impl<'a, T: FromRawConnectorState> AsRawConnectorState for BorrowedConnectorState<'a, T> {
> +    type Connector = T::Connector;
> +}
> +
> +impl<'a, T: FromRawConnectorState> private::AsRawConnectorState for BorrowedConnectorState<'a, T> {
> +    fn as_raw(&self) -> &bindings::drm_connector_state {
> +        self.state.as_raw()
> +    }
> +
> +    unsafe fn as_raw_mut(&mut self) -> &mut bindings::drm_connector_state {
> +        // SAFETY: We're bound by the same safety contract as this function
> +        unsafe { self.state.as_raw_mut() }
> +    }
> +}
> +
> unsafe extern "C" fn atomic_duplicate_state_callback<T: DriverConnectorState>(
>     connector: *mut bindings::drm_connector
> ) -> *mut bindings::drm_connector_state
> diff --git a/rust/kernel/drm/kms/crtc.rs b/rust/kernel/drm/kms/crtc.rs
> index 246d15a15c14d..7864540705f76 100644
> --- a/rust/kernel/drm/kms/crtc.rs
> +++ b/rust/kernel/drm/kms/crtc.rs
> @@ -3,6 +3,7 @@
> //! KMS driver abstractions for rust.
> 
> use super::{
> +    atomic::*,
>     plane::*,
>     ModeObject,
>     StaticModeObject,
> @@ -552,6 +553,80 @@ unsafe fn from_raw<'a>(ptr: *const bindings::drm_crtc_state) -> &'a Self {
>         unsafe { &*(ptr.cast()) }
>     }
> }
> +
> +/// An interface for mutating a [`Crtc`]s atomic state.
> +///
> +/// This type is typically returned by an [`AtomicStateMutator`] within contexts where it is
> +/// possible to safely mutate a plane's state. In order to uphold rust's data-aliasing rules, only
> +/// [`BorrowedCrtcState`] may exist at a time.

Same here, missing `one` perhaps?

> +///
> +/// # Invariants
> +///
> +/// `self.state` always points to a valid instance of a [`FromRawCrtcState`] object.
> +pub struct BorrowedCrtcState<'a, T: FromRawCrtcState> {
> +    state: NonNull<T>,
> +    mask: &'a Cell<u32>,
> +}
> +
> +impl<'a, T: FromRawCrtcState> BorrowedCrtcState<'a, T> {
> +    pub(super) fn new<D: KmsDriver>(
> +        mutator: &'a AtomicStateMutator<D>,
> +        state: NonNull<bindings::drm_crtc_state>
> +    ) -> Option<Self> {
> +        // SAFETY: `crtc` is invariant throughout the lifetime of the atomic state, and always
> +        // points to a valid `Crtc<T::Crtc>`
> +        let crtc = unsafe { T::Crtc::from_raw((*state.as_ptr()).crtc) };
> +        let crtc_mask = crtc.mask();
> +        let borrowed_mask = mutator.borrowed_crtcs.get();
> +
> +        if borrowed_mask & crtc_mask == 0 {
> +            mutator.borrowed_crtcs.set(borrowed_mask | crtc_mask);

Do you mean for this to be called by multiple threads? If so, I wonder if atomics would be a
better fit.

> +            Some(Self {
> +                mask: &mutator.borrowed_crtcs,
> +                state: state.cast()
> +            })
> +        } else {
> +            None
> +        }
> +    }
> +}
> +
> +impl<'a, T: FromRawCrtcState> Drop for BorrowedCrtcState<'a, T> {
> +    fn drop(&mut self) {
> +        // SAFETY: Our interface is proof that we are the only ones with a reference to this data
> +        let mask = unsafe { self.state.as_ref() }.crtc().mask();
> +        self.mask.set(self.mask.get() & !mask);
> +    }
> +}
> +
> +impl<'a, T: DriverCrtcState> Deref for BorrowedCrtcState<'a, CrtcState<T>> {
> +    type Target = T;
> +
> +    fn deref(&self) -> &Self::Target {
> +        // SAFETY: Our interface ensures that `self.state.inner` follows rust's data-aliasing rules,
> +        // so this is safe
> +        unsafe { &*(*self.state.as_ptr()).inner.get() }
> +    }
> +}
> +
> +impl<'a, T: DriverCrtcState> DerefMut for BorrowedCrtcState<'a, CrtcState<T>> {
> +    fn deref_mut(&mut self) -> &mut Self::Target {
> +        // SAFETY: Our interface ensures that `self.state.inner` follows rust's data-aliasing rules,
> +        // so this is safe
> +        unsafe { (*self.state.as_ptr()).inner.get_mut() }
> +    }
> +}
> +
> +impl<'a, T: FromRawCrtcState> AsRawCrtcState for BorrowedCrtcState<'a, T> {
> +    type Crtc = T::Crtc;
> +}
> +
> +impl<'a, T: FromRawCrtcState> private::AsRawCrtcState for BorrowedCrtcState<'a, T> {
> +    fn as_raw(&self) -> *mut bindings::drm_crtc_state {
> +        self.state.as_ptr().cast()
> +    }
> +}
> +
> unsafe extern "C" fn crtc_destroy_callback<T: DriverCrtc>(
>     crtc: *mut bindings::drm_crtc
> ) {
> diff --git a/rust/kernel/drm/kms/plane.rs b/rust/kernel/drm/kms/plane.rs
> index 1c151ae3b3dcc..d6e11a65cc101 100644
> --- a/rust/kernel/drm/kms/plane.rs
> +++ b/rust/kernel/drm/kms/plane.rs
> @@ -29,6 +29,7 @@
>     UnregisteredKmsDevice,
>     ModeObject,
>     StaticModeObject,
> +    atomic::*,
> };
> 
> /// The main trait for implementing the [`struct drm_plane`] API for [`Plane`]
> @@ -597,6 +598,82 @@ unsafe fn from_raw_mut<'a>(ptr: *mut bindings::drm_plane_state) -> &'a mut Self
>         unsafe { &mut *ptr.cast() }
>     }
> }
> +
> +/// An interface for mutating a [`Plane`]s atomic state.
> +///
> +/// This type is typically returned by an [`AtomicStateMutator`] within contexts where it is
> +/// possible to safely mutate a plane's state. In order to uphold rust's data-aliasing rules, only
> +/// [`BorrowedPlaneState`] may exist at a time.
> +pub struct BorrowedPlaneState<'a, T: FromRawPlaneState> {
> +    state: &'a mut T,
> +    mask: &'a Cell<u32>
> +}
> +
> +impl<'a, T: FromRawPlaneState> BorrowedPlaneState<'a, T> {
> +    pub(super) fn new<D: KmsDriver>(
> +        mutator: &'a AtomicStateMutator<D>,
> +        state: NonNull<bindings::drm_plane_state>
> +    ) -> Option<Self> {
> +        // SAFETY: `plane` is invariant throughout the lifetime of the atomic state, is
> +        // initialized by this point, and we're guaranteed it is of type `AsRawPlane` by type
> +        // invariance
> +        let plane = unsafe { T::Plane::from_raw((*state.as_ptr()).plane) };
> +        let plane_mask = plane.mask();
> +        let borrowed_mask = mutator.borrowed_planes.get();
> +
> +        if borrowed_mask & plane_mask == 0 {
> +            mutator.borrowed_planes.set(borrowed_mask | plane_mask);
> +            Some(Self {
> +                mask: &mutator.borrowed_planes,
> +                // SAFETY: We're guaranteed `state` is of `FromRawPlaneState` by type invariance,
> +                // and we just confirmed by checking `borrowed_planes` that no other mutable borrows
> +                // have been taken out for `state`
> +                state: unsafe { T::from_raw_mut(state.as_ptr()) },
> +            })
> +        } else {
> +            None
> +        }
> +    }
> +}
> +
> +impl<'a, T: FromRawPlaneState> Drop for BorrowedPlaneState<'a, T> {
> +    fn drop(&mut self) {
> +        let mask = self.state.plane().mask();
> +        self.mask.set(self.mask.get() & !mask);
> +    }
> +}
> +
> +impl<'a, T: FromRawPlaneState> AsRawPlaneState for BorrowedPlaneState<'a, T> {
> +    type Plane = T::Plane;
> +}
> +
> +impl<'a, T: FromRawPlaneState> private::AsRawPlaneState for BorrowedPlaneState<'a, T> {
> +    fn as_raw(&self) -> &bindings::drm_plane_state {
> +        self.state.as_raw()
> +    }
> +
> +    unsafe fn as_raw_mut(&mut self) -> &mut bindings::drm_plane_state {
> +        // SAFETY: This function is bound by the same safety contract as `self.inner.as_raw_mut()`
> +        unsafe { self.state.as_raw_mut() }
> +    }
> +}
> +
> +impl<'a, T: FromRawPlaneState> Sealed for BorrowedPlaneState<'a, T> {}
> +
> +impl<'a, T: DriverPlaneState> Deref for BorrowedPlaneState<'a, PlaneState<T>> {
> +    type Target = T;
> +
> +    fn deref(&self) -> &Self::Target {
> +        &self.state.inner
> +    }
> +}
> +
> +impl<'a, T: DriverPlaneState> DerefMut for BorrowedPlaneState<'a, PlaneState<T>> {
> +    fn deref_mut(&mut self) -> &mut Self::Target {
> +        &mut self.state.inner
> +    }
> +}
> +
> unsafe extern "C" fn plane_destroy_callback<T: DriverPlane>(
>     plane: *mut bindings::drm_plane
> ) {
> -- 
> 2.46.1
> 


By the way, you said:

> 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

Can you be more specific? I couldn’t pinpoint where exactly this happens.

———


This is what I have for now. I may come back to this patch at a later point since
there is quite a bit going on here.

— Daniel

[0] https://doc.rust-lang.org/reference/subtyping.html

Powered by blists - more mailing lists