2024-03-22 22:18:44

by Lyude Paul

[permalink] [raw]
Subject: [PATCH 3/4] rust/drm/kms: Extract PlaneState<T> into IntoPlaneState

DRM actually has a number of helpers that wrap drm_plane_state, one of
which is actually needed by VKMS - drm_shadow_plane_state.

So, let's start preparing to write bindings for this by first extracting
PlaneState<T> into the IntoPlaneState trait - which all DRM structs which
wrap drm_plane_state will implement. This is basically the same idea as the
GEM ops - but for plane states.

Signed-off-by: Lyude Paul <[email protected]>
---
drivers/gpu/drm/rvkms/plane.rs | 2 +-
rust/kernel/drm/kms/plane.rs | 165 ++++++++++++++++++++-------------
2 files changed, 103 insertions(+), 64 deletions(-)

diff --git a/drivers/gpu/drm/rvkms/plane.rs b/drivers/gpu/drm/rvkms/plane.rs
index 54c4bbda64d8e..d98a1f7bf79e2 100644
--- a/drivers/gpu/drm/rvkms/plane.rs
+++ b/drivers/gpu/drm/rvkms/plane.rs
@@ -20,7 +20,7 @@ pub(crate) struct DriverPlane {
impl plane::DriverPlane for DriverPlane {
type Initializer = impl PinInit<Self, Error>;

- type State = RvkmsPlaneState;
+ type State = PlaneState;

type Driver = RvkmsDriver;

diff --git a/rust/kernel/drm/kms/plane.rs b/rust/kernel/drm/kms/plane.rs
index 78c8e370b997c..73c285445be63 100644
--- a/rust/kernel/drm/kms/plane.rs
+++ b/rust/kernel/drm/kms/plane.rs
@@ -13,6 +13,7 @@
sync::{Arc, ArcBorrow},
init::InPlaceInit,
offset_of,
+ private::Sealed,
};
use core::{
cell::UnsafeCell,
@@ -62,14 +63,16 @@ pub trait DriverPlane: Send + Sync + Sized {
/// The parent driver implementation
type Driver: KmsDriver;

- /// The type for this driver's drm_plane_state implementation
- type State: DriverPlaneState;
+ /// The full type for this driver's drm_plane_state implementation. Drivers which don't need
+ /// special DRM helpers for their plane states may just use `PlaneState<T>` here, where `T` is
+ /// their private state struct which implements `DriverPlaneState`
+ type State: IntoPlaneState;

/// Create a new plane for this driver
fn new(device: &Device<Self::Driver>, args: Self::Args) -> Self::Initializer;
}

-impl<T: DriverPlane> crate::private::Sealed for Plane<T> {}
+impl<T: DriverPlane> Sealed for Plane<T> {}

impl<T: DriverPlane> ModeObject for Plane<T> {
type Driver = T::Driver;
@@ -177,6 +180,70 @@ pub(super) fn raw_mut_ptr(&self) -> *mut bindings::drm_plane {
unsafe { drop(Box::from_raw(plane as *mut Plane<T>)) };
}

+/// Operations implemented by any base atomic plane state. These are implemented by DRM to provide
+/// wrappers around the generic atomic plane state, along with various plane state helpers.
+///
+/// SAFETY: Incorrect implementation of these methods will result in UB, users should take care to
+/// read through the documentation of each method - especially the provided methods.
+pub unsafe trait IntoPlaneState: Default + Sealed {
+ /// Consume the box for this plane state without dropping its contents, and return a reference
+ /// to it's base plane state to hand off to DRM
+ ///
+ /// Implementors must override this if their data layout does not start with
+ /// `bindings::drm_plane_state`.
+ fn into_raw(self: Box<Self>) -> *mut bindings::drm_plane_state {
+ // Our data layout starts with drm_plane_state
+ Box::into_raw(self).cast()
+ }
+
+ /// Reconstruct the box for this plate state for deallocation
+ ///
+ /// Implementors must override this if their data layout does not start with
+ /// `bindings::drm_plane_state`.
+ unsafe fn from_raw(ptr: *mut bindings::drm_plane_state) -> Box<Self> {
+ // SAFETY: Users of this default promise their data layout starts with drm_plane_state
+ unsafe { Box::from_raw(ptr.cast()) }
+ }
+
+ /// Get a mutable reference to the raw `bindings::drm_plane_state` contained within this type
+ /// that we can pass to DRM
+ fn as_raw_mut(&mut self) -> &mut bindings::drm_plane_state {
+ // SAFETY: Users of this default promise their data layout starts with drm_plane_state
+ unsafe { mem::transmute(self) }
+ }
+
+ /// Get an immutable reference to this type from the given raw `bindings::drm_plane_state`
+ /// pointer
+ ///
+ /// Implementors must override this if their data layout does not start with
+ /// `bindings::drm_plane_state`.
+ ///
+ /// SAFETY: The caller guarantees `ptr` is contained within a valid instance of `Self`
+ unsafe fn ref_from_raw<'a>(ptr: *const bindings::drm_plane_state) -> &'a Self {
+ // SAFETY: Users of this default promise their data layout starts with drm_plane_state
+ unsafe { &*ptr.cast() }
+ }
+
+ /// Get a mutable reference to this type from the given raw `bindings::drm_plane_state` pointer
+ ///
+ /// SAFETY: The caller guarantees `ptr` is contained within a valid instance of `Self`
+ unsafe fn ref_from_raw_mut<'a>(ptr: *mut bindings::drm_plane_state) -> &'a mut Self {
+ // SAFETY: Users of this default promise their data layout starts with drm_plane_state
+ unsafe { &mut (*ptr.cast()) }
+ }
+
+ // Allocate a "duplicated" copy of this state, which usually involves calling DRM's helper
+ // function for this plane state type
+ fn __duplicate_state(&self, plane: *mut bindings::drm_plane) -> Result<Box<Self>>;
+
+ // Call DRM's destroy helper for this type of plane state. Note this only cleans up the base DRM
+ // state struct and does not de-allocate its `Box`.
+ fn __destroy_state(state: *mut bindings::drm_plane_state);
+
+ // Call DRM's reset helper for this type of plane state.
+ fn __reset_state(plane: *mut bindings::drm_plane, state: *mut bindings::drm_plane_state);
+}
+
#[derive(Default)]
#[repr(C)]
pub struct PlaneState<T: DriverPlaneState> {
@@ -190,46 +257,35 @@ pub trait DriverPlaneState: Clone + Default + Sized {
type Plane: DriverPlane;
}

-impl<T: DriverPlaneState> PlaneState<T> {
- /// Consume this struct without dropping it, and return a pointer to its base `drm_plane_state`
- /// which can be handed off to DRM.
- fn into_raw(self: Box<Self>) -> *mut bindings::drm_plane_state {
- let this = Box::into_raw(self);
+impl<T: DriverPlaneState> Sealed for PlaneState<T> {}

- unsafe { &mut (*this).state }
- }
+// SAFETY: Our data layout starts with drm_plane_state
+unsafe impl<T: DriverPlaneState> IntoPlaneState for PlaneState<T> {
+ fn __duplicate_state(&self, plane: *mut bindings::drm_plane) -> Result<Box<Self>> {
+ let mut new: Box<Self> = Box::try_init(try_init!(Self {
+ state: bindings::drm_plane_state { ..Default::default() },
+ inner: self.inner.clone()
+ }))?;

- /// Consume a raw pointer and recover the original `Box<PlaneState<T>>`
- ///
- /// SAFETY: Callers must ensure that ptr contains a valid instance of `PlaneState<T>`
- unsafe fn from_raw(ptr: *mut bindings::drm_plane_state) -> Box<Self> {
- unsafe { Box::from_raw(ptr as *mut _) }
- }
+ // SAFETY: FFI call with no special requirements
+ unsafe { bindings::__drm_atomic_helper_plane_duplicate_state(plane, new.as_raw_mut()) };

- /// Obtain a reference back to the `PlaneState<T>`
- ///
- /// SAFETY: Callers must ensure that ptr contains a valid instance of `PlaneState<T>`.
- unsafe fn as_ref<'a>(ptr: *const bindings::drm_plane_state) -> &'a Self {
- unsafe { &*(ptr as *const _) }
+ Ok(new)
}

- /// Obtain a mutable reference back to the PlaneState<T>
- ///
- /// SAFETY: Callers must ensure that ptr contains a valid instance of `PlaneState<T>`, and that
- /// no other references to this `PlaneState<T>` exist for the lifetime of this reference
- unsafe fn as_mut_ref<'a>(ptr: *mut bindings::drm_plane_state) -> &'a mut Self {
- unsafe { &mut *(ptr as *mut _) }
+ fn __destroy_state(state: *mut bindings::drm_plane_state) {
+ // SAFETY: This would not be called without a plane state to destroy
+ unsafe { bindings::__drm_atomic_helper_plane_destroy_state(state) };
}

- /// Obtain a mutable pointer to the base plane state, for use in FFI calls
- unsafe fn as_mut_ptr(&mut self) -> *mut bindings::drm_plane_state {
- &mut self.state
+ fn __reset_state(plane: *mut bindings::drm_plane, state: *mut bindings::drm_plane_state) {
+ unsafe { bindings::__drm_atomic_helper_plane_reset(plane, state) }
}
}

unsafe impl Zeroable for bindings::drm_plane_state { }

-unsafe extern "C" fn atomic_duplicate_state_callback<T: DriverPlaneState>(
+unsafe extern "C" fn atomic_duplicate_state_callback<T: IntoPlaneState>(
plane: *mut bindings::drm_plane
) -> *mut bindings::drm_plane_state
{
@@ -240,55 +296,38 @@ unsafe impl Zeroable for bindings::drm_plane_state { }
return null_mut();
}

- // SAFETY: We just verified that `state` is non-null, and we're guaranteed by our bindings that
- // `state` is of type `PlaneState<T>`.
- let state = unsafe { PlaneState::<T>::as_ref(state) };
-
- let mut new: Result<Box<PlaneState<T>>> = Box::try_init(try_init!(PlaneState::<T> {
- state: bindings::drm_plane_state { ..Default::default() },
- inner: state.inner.clone()
- }));
-
- if let Ok(mut new) = new {
- // SAFETY: Just a lil' FFI call, nothing special here
- unsafe { bindings::__drm_atomic_helper_plane_duplicate_state(plane, new.as_mut_ptr()) };
-
- new.into_raw()
- } else {
- null_mut()
- }
+ // SAFETY: We just checked that the state is non-null, and we're guaranteed this operation is
+ // safe via type invariance
+ unsafe { T::ref_from_raw(state) }.__duplicate_state(plane).map_or(null_mut(), T::into_raw)
}

-unsafe extern "C" fn atomic_destroy_state_callback<T: DriverPlaneState>(
+unsafe extern "C" fn atomic_destroy_state_callback<T: IntoPlaneState>(
_plane: *mut bindings::drm_plane,
- plane_state: *mut bindings::drm_plane_state
+ state: *mut bindings::drm_plane_state
) {
- // SAFETY: This callback wouldn't be called unless there a plane state to destroy
- unsafe { bindings::__drm_atomic_helper_plane_destroy_state(plane_state) };
+ T::__destroy_state(state);

- // SAFETY: We're guaranteed by type invariants that plane_state is of type PlaneState<T>, and
- // since this is the destructor callback for DRM - we're guaranteed to hold the only remaining
- // reference to this state
- unsafe { drop(PlaneState::<T>::from_raw(plane_state)) };
+ // SAFETY: We're guaranteed by type invariance that state of type T, and __destroy_state() does
+ // not perform any deallocations
+ drop(unsafe { T::from_raw(state) });
}

-unsafe extern "C" fn plane_reset_callback<T: DriverPlaneState>(
+unsafe extern "C" fn plane_reset_callback<T: IntoPlaneState>(
plane: *mut bindings::drm_plane,
) {
- // SAFETY: The only entrypoint to this function lives in `plane` so it must be valid, and
+ // SAFETY: The only entrypoint to this function lives in `plane` so it must be valid
let state = unsafe { (*plane).state };
if !state.is_null() {
- // SAFETY: We're guaranteed by type invariance that this plane's state is of type
- // DriverPlaneState<T>
+ // SAFETY: We're guaranteed by type invariance that this planes state is of type T
unsafe { atomic_destroy_state_callback::<T>(plane, state) }
}

// 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_new(PlaneState::<T>::default()).expect("Blame the API, sorry!");
+ let new = Box::try_new(T::default()).expect("Blame the API, sorry!");

- // SAFETY: DRM takes ownership of the state from here and assigns it to the plane
- unsafe { bindings::__drm_atomic_helper_plane_reset(plane, new.into_raw()) };
+ // DRM takes ownership of the state from here, resets it, and then assigns it to the plane
+ T::__reset_state(plane, new.into_raw());
}

#[derive(Copy, Clone, Debug, PartialEq, Eq)]
--
2.43.0