Hi Lyude,
> On 30 Sep 2024, at 20:09, Lyude Paul <lyude@xxxxxxxxxx> wrote:
>
> This introduces basic bindings for DRM CRTCs which follow the same general
> pattern as connectors and planes (e.g. AsRawCrtc, AsRawCrtcState, etc.).
> There is one big difference though - drm_crtc_state appears to be the one
> atomic state that actually has data which can be mutated from outside of
> the atomic commit phase - which means we can't keep rust referencs to it,
Nit: typo in `references to it`
> and instead need to use the Opaque type and implement things through
> pointers instead.
>
> This should be the last mode object we're introducing for the time being
> with its own atomic state. Note that we've not added bindings for private
> modesetting objects yet, but I don't think those will be needed for rvkms -
> and the same general patterns we're using here should work for adding
> private modesetting objects.
>
> Signed-off-by: Lyude Paul <lyude@xxxxxxxxxx>
>
> ---
>
> TODO:
> * Add commit data in the future
>
> Signed-off-by: Lyude Paul <lyude@xxxxxxxxxx>
> ---
> rust/kernel/drm/kms.rs | 1 +
> rust/kernel/drm/kms/crtc.rs | 501 ++++++++++++++++++++++++++++++++++++
> 2 files changed, 502 insertions(+)
> create mode 100644 rust/kernel/drm/kms/crtc.rs
>
> diff --git a/rust/kernel/drm/kms.rs b/rust/kernel/drm/kms.rs
> index 5b075794a1155..4b54611fdba8b 100644
> --- a/rust/kernel/drm/kms.rs
> +++ b/rust/kernel/drm/kms.rs
> @@ -3,6 +3,7 @@
> //! KMS driver abstractions for rust.
>
> pub mod connector;
> +pub mod crtc;
> pub mod fbdev;
> pub mod plane;
>
> diff --git a/rust/kernel/drm/kms/crtc.rs b/rust/kernel/drm/kms/crtc.rs
> new file mode 100644
> index 0000000000000..d84db49948380
> --- /dev/null
> +++ b/rust/kernel/drm/kms/crtc.rs
> @@ -0,0 +1,501 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +
> +//! KMS driver abstractions for rust.
Maybe this should be a little more specific?
> +
> +use super::{
> + plane::*,
> + ModeObject,
> + StaticModeObject,
> + KmsDriver,
> + UnregisteredKmsDevice
> +};
> +use crate::{
> + bindings,
> + drm::device::Device,
> + device,
> + prelude::*,
> + private::Sealed,
> + error::from_result,
> + types::Opaque,
> + init::Zeroable,
> + sync::Arc,
> + error::to_result,
> +};
> +use core::{
> + cell::{Cell, UnsafeCell},
> + marker::*,
> + ptr::{NonNull, null, null_mut, addr_of_mut, self},
> + ops::{Deref, DerefMut},
> + mem,
> +};
> +use macros::vtable;
> +
> +/// The main trait for implementing the [`struct drm_crtc`] API for [`Crtc`].
> +///
> +/// Any KMS driver should have at least one implementation of this type, which allows them to create
> +/// [`Crtc`] objects. Additionally, a driver may store driver-private data within the type that
> +/// implements [`DriverCrtc`] - and it will be made available when using a fully typed [`Crtc`]
> +/// object.
> +///
> +/// # Invariants
> +///
> +/// - Any C FFI callbacks generated using this trait are guaranteed that passed-in
> +/// [`struct drm_crtc`] pointers are contained within a [`Crtc<Self>`].
> +/// - Any C FFI callbacks generated using this trait are guaranteed that passed-in
> +/// [`struct drm_crtc_state`] pointers are contained within a [`CrtcState<Self::State>`].
> +///
> +/// [`struct drm_crtc`]: srctree/include/drm/drm_crtc.h
> +/// [`struct drm_crtc_state`]: srctree/include/drm/drm_crtc.h
> +#[vtable]
> +pub trait DriverCrtc: Send + Sync + Sized {
> + /// The generated C vtable for this [`DriverCrtc`] implementation.
> + #[unique]
> + const OPS: &'static DriverCrtcOps = &DriverCrtcOps {
> + funcs: bindings::drm_crtc_funcs {
> + atomic_destroy_state: Some(atomic_destroy_state_callback::<Self::State>),
> + atomic_duplicate_state: Some(atomic_duplicate_state_callback::<Self::State>),
> + atomic_get_property: None,
> + atomic_print_state: None,
> + atomic_set_property: None,
> + cursor_move: None,
> + cursor_set2: None,
> + cursor_set: None,
> + destroy: Some(crtc_destroy_callback::<Self>),
> + disable_vblank: None,
> + early_unregister: None,
> + enable_vblank: None,
> + gamma_set: None, // TODO
> + get_crc_sources: None,
> + get_vblank_counter: None,
> + get_vblank_timestamp: None,
> + late_register: None,
> + page_flip: Some(bindings::drm_atomic_helper_page_flip),
> + page_flip_target: None,
> + reset: Some(crtc_reset_callback::<Self::State>),
> + set_config: Some(bindings::drm_atomic_helper_set_config),
> + set_crc_source: None,
> + set_property: None,
> + verify_crc_source: None,
> + },
> +
> + helper_funcs: bindings::drm_crtc_helper_funcs {
> + atomic_disable: None,
> + atomic_enable: None,
> + atomic_check: None,
> + dpms: None,
> + commit: None,
> + prepare: None,
> + disable: None,
> + mode_set: None,
> + mode_valid: None,
> + mode_fixup: None,
> + atomic_begin: None,
> + atomic_flush: None,
> + mode_set_nofb: None,
> + mode_set_base: None,
> + mode_set_base_atomic: None,
> + get_scanout_position: None,
> + },
> + };
> +
> + /// The type to pass to the `args` field of [`Crtc::new`].
> + ///
> + /// This type will be made available in in the `args` argument of [`Self::new`]. Drivers which
> + /// don't need this can simply pass [`()`] here.
> + type Args;
> +
> + /// The parent [`KmsDriver`] implementation.
> + type Driver: KmsDriver;
> +
> + /// The [`DriverCrtcState`] implementation for this [`DriverCrtc`].
> + ///
> + /// See [`DriverCrtcState`] for more info.
> + type State: DriverCrtcState;
> +
> + /// The constructor for creating a [`Crtc`] using this [`DriverCrtc`] implementation.
> + ///
> + /// Drivers may use this to instantiate their [`DriverCrtc`] object.
> + fn new(device: &Device<Self::Driver>, args: &Self::Args) -> impl PinInit<Self, Error>;
> +}
> +
> +/// The generated C vtable for a [`DriverCrtc`].
> +///
> +/// This type is created internally by DRM.
> +pub struct DriverCrtcOps {
> + funcs: bindings::drm_crtc_funcs,
> + helper_funcs: bindings::drm_crtc_helper_funcs,
> +}
> +
> +/// The main interface for a [`struct drm_crtc`].
> +///
> +/// This type is the main interface for dealing with DRM CRTCs. In addition, it also allows
> +/// immutable access to whatever private data is contained within an implementor's [`DriverCrtc`]
> +/// type.
> +///
> +/// # Invariants
> +///
> +/// - `crtc` and `inner` are initialized for as long as this object is made available to users.
> +/// - The data layout of this structure begins with [`struct drm_crtc`].
> +/// - The atomic state for this type can always be assumed to be of type [`CrtcState<T::State>`].
> +///
> +/// [`struct drm_crtc`]: srctree/include/drm/drm_crtc.h
> +#[repr(C)]
> +#[pin_data]
> +pub struct Crtc<T: DriverCrtc> {
> + // The FFI drm_crtc object
> + crtc: Opaque<bindings::drm_crtc>,
> + /// The driver's private inner data
> + #[pin]
> + inner: T,
> + #[pin]
> + _p: PhantomPinned,
> +}
> +
> +// SAFETY: DRM expects this struct to be zero-initialized
> +unsafe impl Zeroable for bindings::drm_crtc { }
> +
> +impl<T: DriverCrtc> Sealed for Crtc<T> {}
> +
> +// SAFETY: Our CRTC interfaces are guaranteed to be thread-safe
> +unsafe impl<T: DriverCrtc> Send for Crtc<T> { }
> +
> +// SAFETY: Our CRTC interfaces are guaranteed to be thread-safe
> +unsafe impl<T: DriverCrtc> Sync for Crtc<T> { }
> +
> +impl<T: DriverCrtc> Deref for Crtc<T> {
> + type Target = T;
> +
> + fn deref(&self) -> &Self::Target {
> + &self.inner
> + }
> +}
> +
> +impl<T: DriverCrtc> ModeObject for Crtc<T> {
> + type Driver = T::Driver;
> +
> + fn drm_dev(&self) -> &Device<Self::Driver> {
> + // SAFETY: DRM connectors exist for as long as the device does, so this pointer is always
> + // valid
> + unsafe { Device::borrow((*self.as_raw()).dev) }
> + }
> +
> + fn raw_mode_obj(&self) -> *mut bindings::drm_mode_object {
> + // SAFETY: We don't expose Crtc<T> to users before it's initialized, so `base` is always
> + // initialized
> + unsafe { addr_of_mut!((*self.as_raw()).base) }
> + }
> +}
> +
> +// SAFETY: CRTCs are non-refcounted modesetting objects
> +unsafe impl<T: DriverCrtc> StaticModeObject for Crtc<T> { }
> +
> +impl<T: DriverCrtc> Crtc<T> {
> + /// Construct a new [`Crtc`].
> + ///
> + /// A driver may use this from their [`Kms::create_objects`] callback in order to construct new
> + /// [`Crtc`] objects.
> + ///
> + /// [`Kms::create_objects`]: kernel::drm::kms::Kms::create_objects
> + pub fn new<'a, 'b: 'a, P, C>(
With two lifetimes and two generic types, this is getting a bit convoluted IMHO.
I wonder if more descriptive names for the generics would help here, like `PlaneData` instead of P.
> + dev: &'a UnregisteredKmsDevice<'a, T::Driver>,
> + primary: &'a Plane<P>,
> + cursor: Option<&'a Plane<C>>,
> + name: Option<&CStr>,
> + args: T::Args,
> + ) -> Result<&'b Self>
> + where
> + P: DriverPlane<Driver = T::Driver>,
> + C: DriverPlane<Driver = T::Driver>
> + {
> + let this = Box::try_pin_init(
> + try_pin_init!(Self {
> + crtc: Opaque::new(bindings::drm_crtc {
> + helper_private: &T::OPS.helper_funcs,
> + ..Default::default()
> + }),
> + inner <- T::new(dev, &args),
> + _p: PhantomPinned,
> + }),
> + GFP_KERNEL
> + )?;
> +
> + to_result(unsafe {
> + bindings::drm_crtc_init_with_planes(
> + dev.as_raw(),
> + this.as_raw(),
> + primary.as_raw(),
> + cursor.map_or(null_mut(), |c| c.as_raw()),
> + &T::OPS.funcs,
> + name.map_or(null(), |n| n.as_char_ptr())
> + )
> + })?;
> +
> + // Convert the box into a raw pointer, we'll re-assemble it in crtc_destroy_callback()
> + // SAFETY: We don't move anything
> + Ok(unsafe { &*Box::into_raw(Pin::into_inner_unchecked(this)) })
Maybe break this into multiple lines?
> + }
> +}
> +
> +/// A trait implemented by any type that acts as a [`struct drm_crtc`] interface.
> +///
> +/// This is implemented internally by DRM.
> +///
> +/// [`struct drm_crtc`]: srctree/include/drm/drm_crtc.h
> +pub trait AsRawCrtc: StaticModeObject {
> + /// The type that should be returned for a CRTC state acquired using this CRTC interface
> + type State: FromRawCrtcState;
> +
> + /// Return a raw pointer to the `bindings::drm_crtc` for this object
> + fn as_raw(&self) -> *mut bindings::drm_crtc;
> +
> + /// Convert a raw `bindings::drm_crtc` pointer into an object of this type.
> + ///
> + /// SAFETY: Callers promise that `ptr` points to a valid instance of this type
> + unsafe fn from_raw<'a>(ptr: *mut bindings::drm_crtc) -> &'a Self;
> +}
> +
> +impl<T: DriverCrtc> AsRawCrtc for Crtc<T> {
> + type State = CrtcState<T::State>;
> +
> + fn as_raw(&self) -> *mut bindings::drm_crtc {
> + self.crtc.get()
> + }
> +
> + unsafe fn from_raw<'a>(ptr: *mut bindings::drm_crtc) -> &'a Self {
> + // SAFETY: Our data layout starts with `bindings::drm_crtc`
> + unsafe { &*ptr.cast() }
> + }
> +}
> +
> +unsafe impl Zeroable for bindings::drm_crtc_state { }
> +
> +impl<T: DriverCrtcState> Sealed for CrtcState<T> {}
> +
> +/// The main trait for implementing the [`struct drm_crtc_state`] API for a [`Crtc`].
> +///
> +/// A driver may store driver-private data within the implementor's type, which will be available
> +/// when using a full typed [`CrtcState`] object.
> +///
> +/// # Invariants
> +///
> +/// - Any C FFI callbacks generated using this trait are guaranteed that passed-in
> +/// [`struct drm_crtc`] pointers are contained within a [`Crtc<Self::Crtc>`].
> +/// - Any C FFI callbacks generated using this trait are guaranteed that passed-in
> +/// [`struct drm_crtc_state`] pointers are contained within a [`CrtcState<Self>`].
> +///
> +/// [`struct drm_crtc`]: srctree/include/drm_crtc.h
> +/// [`struct drm_crtc_state`]: srctree/include/drm_crtc.h
> +pub trait DriverCrtcState: Clone + Default + Unpin {
> + /// The parent CRTC driver for this CRTC state
> + type Crtc: DriverCrtc<State = Self> where Self: Sized;
> +}
> +
> +/// The main interface for a [`struct drm_crtc_state`].
> +///
> +/// This type is the main interface for dealing with the atomic state of DRM crtcs. In addition, it
> +/// allows access to whatever private data is contained within an implementor's [`DriverCrtcState`]
> +/// type.
> +///
> +/// # Invariants
> +///
> +/// - `state` and `inner` initialized for as long as this object is exposed to users.
> +/// - The data layout of this structure begins with [`struct drm_crtc_state`].
> +/// - The CRTC for this type can always be assumed to be of type [`Crtc<T::Crtc>`].
> +///
> +/// [`struct drm_crtc_state`]: srctree/include/drm/drm_crtc.h
> +#[repr(C)]
> +pub struct CrtcState<T: DriverCrtcState> {
> + state: Opaque<bindings::drm_crtc_state>,
> + inner: UnsafeCell<T>,
I don’t think this is being passed to C, nor do I see UnsafeCell being used for its interior mutability
Here, so can’t this just be T?
> +}
> +
> +impl<T: DriverCrtcState> Deref for CrtcState<T> {
> + type Target = T;
> +
> + fn deref(&self) -> &Self::Target {
> + // SAFETY: Our interface ensures that `inner` will not be modified unless only a single
> + // mutable reference exists to `inner`, so this is safe
> + unsafe { &*self.inner.get() }
> + }
> +}
> +
> +impl<T: DriverCrtcState> DerefMut for CrtcState<T> {
> + fn deref_mut(&mut self) -> &mut Self::Target {
> + // SAFETY: Our interfaces ensures that we either have one mutable reference to the state
> + // (this one), or multiple immutable references
> + unsafe { self.inner.get_mut() }
> + }
> +}
> +
> +/// A trait implemented by any type which can produce a reference to a [`struct drm_crtc_state`].
> +///
> +/// This is implemented internally by DRM.
> +///
> +/// [`struct drm_crtc_state`]: srctree/include/drm/drm_crtc.h
> +pub trait AsRawCrtcState: private::AsRawCrtcState {
> + /// The type that this CRTC state interface returns to represent the parent CRTC
> + type Crtc: AsRawCrtc;
> +}
> +
> +pub(crate) mod private {
> + use super::*;
> +
> + #[doc(hidden)]
> + pub trait AsRawCrtcState {
> + /// Return a raw pointer to the DRM CRTC state
> + ///
> + /// Note that CRTC states are the only atomic state in KMS which don't nicely follow rust's
> + /// data aliasing rules already.
> + fn as_raw(&self) -> *mut bindings::drm_crtc_state;
> + }
> +}
> +
> +pub(super) use private::AsRawCrtcState as AsRawCrtcStatePrivate;
> +
> +/// A trait for providing common methods which can be used on any type that can be used as an atomic
> +/// CRTC state.
> +pub trait RawCrtcState: AsRawCrtcState {
> + /// Return the CRTC that owns this state.
> + fn crtc(&self) -> &Self::Crtc {
> + // SAFETY:
> + // * This type conversion is guaranteed by type invariance
> + // * Our interface ensures that this access follows rust's data-aliasing rules
> + // * `crtc` is guaranteed to never be NULL and is invariant throughout the lifetime of the
> + // state
> + unsafe { <Self::Crtc as AsRawCrtc>::from_raw((*self.as_raw()).crtc) }
> + }
> +}
> +impl<T: AsRawCrtcState> RawCrtcState for T {}
> +
> +/// A trait implemented for any type which can be constructed directly from a
> +/// [`struct drm_crtc_state`] pointer.
> +///
> +/// This is implemented internally by DRM.
> +///
> +/// [`struct drm_crtc_state`]: srctree/include/drm/drm_crtc.h
> +pub trait FromRawCrtcState: AsRawCrtcState {
> + /// Obtain a reference back to this type from a raw DRM crtc state pointer
> + ///
> + /// # Safety
> + ///
> + /// Callers must ensure that ptr contains a valid instance of this type.
> + unsafe fn from_raw<'a>(ptr: *const bindings::drm_crtc_state) -> &'a Self;
> +}
> +
> +impl<T: DriverCrtcState> private::AsRawCrtcState for CrtcState<T> {
> + #[inline]
> + fn as_raw(&self) -> *mut bindings::drm_crtc_state {
> + self.state.get()
> + }
> +}
> +
> +impl<T: DriverCrtcState> AsRawCrtcState for CrtcState<T> {
> + type Crtc = Crtc<T::Crtc>;
> +}
> +
> +impl<T: DriverCrtcState> FromRawCrtcState for CrtcState<T> {
> + unsafe fn from_raw<'a>(ptr: *const bindings::drm_crtc_state) -> &'a Self {
> + // SAFETY: Our data layout starts with `bindings::drm_crtc_state`
> + unsafe { &*(ptr.cast()) }
> + }
> +}
> +
> +unsafe extern "C" fn crtc_destroy_callback<T: DriverCrtc>(
> + crtc: *mut bindings::drm_crtc
> +) {
> + // SAFETY: DRM guarantees that `crtc` points to a valid initialized `drm_crtc`.
> + unsafe { bindings::drm_crtc_cleanup(crtc) };
> +
> + // SAFETY:
> + // - DRM guarantees we are now the only one with access to this [`drm_crtc`].
> + // - This cast is safe via `DriverCrtc`s type invariants.
> + // - We created this as a pinned type originally
> + drop(unsafe { Pin::new_unchecked(Box::from_raw(crtc as *mut Crtc<T>)) });
> +}
> +
> +unsafe extern "C" fn atomic_duplicate_state_callback<T: DriverCrtcState>(
> + crtc: *mut bindings::drm_crtc
> +) -> *mut bindings::drm_crtc_state {
> + // SAFETY: DRM guarantees that `crtc` points to a valid initialized `drm_crtc`.
> + let state = unsafe { (*crtc).state };
> + if state.is_null() {
> + return null_mut();
> + }
> +
> + // SAFETY: This cast is safe via `DriverCrtcState`s type invariants.
> + let crtc = unsafe { Crtc::<T::Crtc>::from_raw(crtc) };
> +
> + // SAFETY: This cast is safe via `DriverCrtcState`s type invariants.
> + let state = unsafe { CrtcState::<T>::from_raw(state) };
> +
> + let mut new = Box::try_init(
> + try_init!(CrtcState::<T> {
> + state: Opaque::new(Default::default()),
> + inner: UnsafeCell::new((*state).clone()),
> + }),
> + GFP_KERNEL
> + );
> +
> + if let Ok(mut new) = new {
> + let new = Box::into_raw(new).cast();
> +
> + // SAFETY: DRM simply copies the data from the previous base DRM state here and does not
> + // move the contents of `ptr`
> + unsafe { bindings::__drm_atomic_helper_crtc_duplicate_state(crtc.as_raw(), new) }
> +
> + new
> + } else {
> + null_mut()
> + }
> +}
> +
> +unsafe extern "C" fn atomic_destroy_state_callback<T: DriverCrtcState>(
> + _crtc: *mut bindings::drm_crtc,
> + crtc_state: *mut bindings::drm_crtc_state,
> +) {
> + // SAFETY: DRM guarantees that `state` points to a valid instance of `drm_crtc_state`
> + unsafe { bindings::__drm_atomic_helper_crtc_destroy_state(crtc_state) };
> +
> + // SAFETY:
> + // * DRM guarantees we are the only one with access to this `drm_crtc_state`
> + // * This cast is safe via our type invariants.
> + // * All data in `CrtcState` is either Unpin, or pinned
> + drop(unsafe { Box::from_raw(crtc_state as *mut CrtcState<T>) });
> +}
> +
> +unsafe extern "C" fn crtc_reset_callback<T: DriverCrtcState>(
> + crtc: *mut bindings::drm_crtc,
> +) {
> + // SAFETY: DRM guarantees that `state` points to a valid instance of `drm_crtc_state`
> + let state = unsafe { (*crtc).state };
> + if !state.is_null() {
> + // SAFETY:
> + // * We're guaranteed `crtc` is `Crtc<T>` via type invariants
> + // * We're guaranteed `state` is `CrtcState<T>` via type invariants.
> + unsafe { atomic_destroy_state_callback::<T>(crtc, state) }
> +
> + // SAFETY: No special requirements here, DRM expects this to be NULL
> + unsafe { (*crtc).state = null_mut(); }
> + }
> +
> + // SAFETY: `crtc` is guaranteed to be of type `Crtc<T::Crtc>` by type invariance
> + let crtc = unsafe { Crtc::<T::Crtc>::from_raw(crtc) };
> +
> + // Unfortunately, this is the best we can do at the moment as this FFI callback was mistakenly
> + // presumed to be infallible :(
> + let new = Box::try_init(
> + try_init!(CrtcState::<T> {
> + state: Opaque::new(Default::default()),
> + inner: UnsafeCell::new(Default::default()),
> + }),
> + GFP_KERNEL
> + ).expect("Unfortunately, this API was presumed infallible");
> +
> + // SAFETY: DRM takes ownership of the state from here, and will never move it
> + unsafe {
> + bindings::__drm_atomic_helper_crtc_reset(
> + crtc.as_raw(),
> + Box::into_raw(new).cast()
> + )
> + };
> +}
> --
> 2.46.1
>
>
Overall LGTM.
By the way, what is WIP about this?
— Daniel
