On 05/03/25 - 17:59, Lyude Paul wrote:
+
+// SAFETY: DRM connectors are refcounted mode objects
+unsafe impl<T: DriverConnector> RcModeObject for Connector<T> {}
+
+// SAFETY:
+// * Via our type variants our data layout starts with `drm_connector`
+// * Since we don't expose `Connector` to users before it has been
initialized, this and our data
+// layout ensure that `as_raw()` always returns a valid pointer to a
`drm_connector`.
+unsafe impl<T: DriverConnector> AsRawConnector for Connector<T> {
+ fn as_raw(&self) -> *mut bindings::drm_connector {
+ self.connector.get()
+ }
+
+ unsafe fn from_raw<'a>(ptr: *mut bindings::drm_connector) -> &'a Self {
+ // SAFETY: Our data layout starts with `bindings::drm_connector`
+ unsafe { &*ptr.cast() }
I think you should use container_of macro here. It is functionnaly the
same thing, but it may avoid issue if for whatever reason the ->base is
not at the exact same address. This will make this function "symetrical"
with as_raw.
Ditto for the next patch
+ }
+}
+
+// SAFETY: We only expose this object to users directly after
KmsDriver::create_objects has been
+// called.
+unsafe impl<T: DriverConnector> ModesettableConnector for Connector<T> {
+ type State = ConnectorState<T::State>;
+}
+
+/// A [`Connector`] that has not yet been registered with userspace.
+///
+/// KMS registration is single-threaded, so this object is not thread-safe.
+///
+/// # Invariants
+///
+/// - This object can only exist before its respective KMS device has been
registered.
+/// - Otherwise, it inherits all invariants of [`Connector`] and has an
identical data layout.
To garantee a data layout, don't you need to add #[repr(transparent)]?
This will automatically break the compilation if one day NotThreadSafe
is not ZST.
+pub struct UnregisteredConnector<T: DriverConnector>(Connector<T>,
NotThreadSafe);
+
+// SAFETY: We share the invariants of `Connector`
+unsafe impl<T: DriverConnector> AsRawConnector for UnregisteredConnector<T> {
+ fn as_raw(&self) -> *mut bindings::drm_connector {
+ self.0.as_raw()
+ }
+
+ unsafe fn from_raw<'a>(ptr: *mut bindings::drm_connector) -> &'a Self {
+ // SAFETY: This is another from_raw() call, so this function shares
the same safety contract
+ let connector = unsafe { Connector::<T>::from_raw(ptr) };
+
+ // SAFETY: Our data layout is identical via our type invariants.
+ unsafe { mem::transmute(connector) }
IIRC, to be able to transmute, you need to add some #[repr] on the
types, so rust is forced to use a "fixed" layout. See above, I think you
need to add at least repr(transparent) for UnregisteredConnector
+ }
+}
+
+impl<T: DriverConnector> Deref for UnregisteredConnector<T> {
+ type Target = T;
+
+ fn deref(&self) -> &Self::Target {
+ &self.0.inner
+ }
+}
+
+impl<T: DriverConnector> UnregisteredConnector<T> {
+ /// Construct a new [`UnregisteredConnector`].
+ ///
+ /// A driver may use this to create new [`UnregisteredConnector`] objects.
+ ///
+ /// [`KmsDriver::create_objects`]:
kernel::drm::kms::KmsDriver::create_objects
+ pub fn new<'a>(
+ dev: &'a UnregisteredKmsDevice<'a, T::Driver>,
+ type_: Type,
+ args: T::Args,
+ ) -> Result<&'a Self> {
+ let new: Pin<KBox<Connector<T>>> = KBox::try_pin_init(
+ try_pin_init!(Connector::<T> {
+ connector: Opaque::new(bindings::drm_connector {
+ helper_private: &T::OPS.helper_funcs,
+ ..Default::default()
+ }),
+ inner <- T::new(dev, args),
+ _p: PhantomPinned
+ }),
+ GFP_KERNEL,
+ )?;
+
+ // SAFETY:
+ // - `dev` will hold a reference to the new connector, and thus
outlives us.
+ // - We just allocated `new` above
+ // - `new` starts with `drm_connector` via its type invariants.
Why do you need to add the third requirement here? This is not part of
the drm_connector_init requirement. It only requires to have a valid
pointer.
+ to_result(unsafe {
+ bindings::drm_connector_init(dev.as_raw(), new.as_raw(),
&T::OPS.funcs, type_ as i32)
+ })?;
+
+ // SAFETY: We don't move anything
+ let this = unsafe { Pin::into_inner_unchecked(new) };
+
+ // We'll re-assemble the box in connector_destroy_callback()
+ let this = KBox::into_raw(this);
+
+ // UnregisteredConnector has an equivalent data layout
+ let this: *mut Self = this.cast();
+
+ // SAFETY: We just allocated the connector above, so this pointer must
be valid
+ Ok(unsafe { &*this })
+ }
+}
+
+unsafe extern "C" fn connector_destroy_callback<T: DriverConnector>(
+ connector: *mut bindings::drm_connector,
+) {
+ // SAFETY: DRM guarantees that `connector` points to a valid initialized
`drm_connector`.
+ unsafe {
+ bindings::drm_connector_unregister(connector);
+ bindings::drm_connector_cleanup(connector);
+ };
+
+ // SAFETY:
+ // - We originally created the connector in a `Box`
+ // - We are guaranteed to hold the last remaining reference to this
connector
+ // - This cast is safe via `DriverConnector`s type invariants.
+ drop(unsafe { KBox::from_raw(connector as *mut Connector<T>) });
+}
+
+// SAFETY: DRM expects this struct to be zero-initialized
+unsafe impl Zeroable for bindings::drm_connector_state {}
+
+/// A trait implemented by any type which can produce a reference to a
+/// [`struct drm_connector_state`].
+///
+/// This is implemented internally by DRM.
+///
+/// [`struct drm_connector_state`]: srctree/include/drm/drm_connector.h
+pub trait AsRawConnectorState: private::AsRawConnectorState {
+ /// The type that represents this connector state's DRM connector.
+ type Connector: AsRawConnector;
+}
+
+pub(super) mod private {
+ use super::*;
+
+ /// Trait for retrieving references to the base connector state contained
within any connector
+ /// state compatible type
+ #[allow(unreachable_pub)]
+ pub trait AsRawConnectorState {
+ /// Return an immutable reference to the raw connector state.
+ fn as_raw(&self) -> &bindings::drm_connector_state;
+
+ /// Get a mutable reference to the raw [`struct drm_connector_state`]
contained within this
+ /// type.
+ ///
+ ///
+ /// # Safety
+ ///
+ /// The caller promises this mutable reference will not be used to
modify any contents of
+ /// [`struct drm_connector_state`] which DRM would consider to be
static - like the
+ /// backpointer to the DRM connector that owns this state. This also
means the mutable
+ /// reference should never be exposed outside of this crate.
+ ///
+ /// [`struct drm_connector_state`]: srctree/include/drm/drm_connector.h
+ unsafe fn as_raw_mut(&mut self) -> &mut bindings::drm_connector_state;
+ }
+}
+
+pub(super) use private::AsRawConnectorState as AsRawConnectorStatePrivate;
+
+/// A trait implemented for any type which can be constructed directly from a
+/// [`struct drm_connector_state`] pointer.
+///
+/// This is implemented internally by DRM.
+///
+/// [`struct drm_connector_state`]: srctree/include/drm/drm_connector.h
+pub trait FromRawConnectorState: AsRawConnectorState {
+ /// Get an immutable reference to this type from the given raw [`struct
drm_connector_state`]
+ /// pointer.
+ ///
+ /// # Safety
+ ///
+ /// - The caller guarantees `ptr` is contained within a valid instance of
`Self`.
+ /// - The caller guarantees that `ptr` cannot not be modified for the
lifetime of `'a`.
+ ///
+ /// [`struct drm_connector_state`]: srctree/include/drm/drm_connector.h
+ unsafe fn from_raw<'a>(ptr: *const bindings::drm_connector_state) -> &'a
Self;
+
+ /// Get a mutable reference to this type from the given raw [`struct
drm_connector_state`]
+ /// pointer.
+ ///
+ /// # Safety
+ ///
+ /// - The caller guarantees that `ptr` is contained within a valid
instance of `Self`.
+ /// - The caller guarantees that `ptr` cannot have any other references
taken out for the
+ /// lifetime of `'a`.
+ ///
+ /// [`struct drm_connector_state`]: srctree/include/drm/drm_connector.h
+ unsafe fn from_raw_mut<'a>(ptr: *mut bindings::drm_connector_state) -> &'a
mut Self;
+}
+
+/// The main interface for a [`struct drm_connector_state`].
+///
+/// This type is the main interface for dealing with the atomic state of DRM
connectors. In
+/// addition, it allows access to whatever private data is contained within an
implementor's
+/// [`DriverConnectorState`] type.
+///
+/// # Invariants
+///
+/// - The DRM C API and our interface guarantees that only the user has
mutable access to `state`,
+/// up until [`drm_atomic_helper_commit_hw_done`] is called. Therefore,
`connector` follows rust's
+/// data aliasing rules and does not need to be behind an [`Opaque`] type.
+/// - `state` and `inner` initialized for as long as this object is exposed to
users.
+/// - The data layout of this structure begins with [`struct
drm_connector_state`].
+/// - The connector for this atomic state can always be assumed to be of type
+/// [`Connector<T::Connector>`].
+///
+/// [`struct drm_connector_state`]: srctree/include/drm/drm_connector.h
+/// [`drm_atomic_helper_commit_hw_done`]:
srctree/include/drm/drm_atomic_helper.h
+#[derive(Default)]
+#[repr(C)]
+pub struct ConnectorState<T: DriverConnectorState> {
+ state: bindings::drm_connector_state,
+ inner: T,
+}
+
+/// The main trait for implementing the [`struct drm_connector_state`] API for
a [`Connector`].
+///
+/// A driver may store driver-private data within the implementor's type,
which will be available
+/// when using a full typed [`ConnectorState`] object.
+///
+/// # Invariants
+///
+/// - Any C FFI callbacks generated using this trait are guaranteed that
passed-in
+/// [`struct drm_connector`] pointers are contained within a
[`Connector<Self::Connector>`].
+/// - Any C FFI callbacks generated using this trait are guaranteed that
passed-in
+/// [`struct drm_connector_state`] pointers are contained within a
[`ConnectorState<Self>`].
+///
+/// [`struct drm_connector`]: srctree/include/drm_connector.h
+/// [`struct drm_connector_state`]: srctree/include/drm_connector.h
+pub trait DriverConnectorState: Clone + Default + Sized {
+ /// The parent [`DriverConnector`].
+ type Connector: DriverConnector;
+}
+
+impl<T: DriverConnectorState> Sealed for ConnectorState<T> {}
+
+impl<T: DriverConnectorState> AsRawConnectorState for ConnectorState<T> {
+ type Connector = Connector<T::Connector>;
+}
+
+impl<T: DriverConnectorState> private::AsRawConnectorState for
ConnectorState<T> {
+ fn as_raw(&self) -> &bindings::drm_connector_state {
+ &self.state
+ }
+
+ unsafe fn as_raw_mut(&mut self) -> &mut bindings::drm_connector_state {
+ &mut self.state
+ }
+}
+
+impl<T: DriverConnectorState> FromRawConnectorState for ConnectorState<T> {
+ unsafe fn from_raw<'a>(ptr: *const bindings::drm_connector_state) -> &'a
Self {
+ // Our data layout starts with `bindings::drm_connector_state`.
+ let ptr: *const Self = ptr.cast();
As for the connector, I think this is a bit safer to use container_of.
And what is the rule about unsafe in unsafe function? I think this
casting is unsafe, but you did not add the unsafe block around it.
+
+ // SAFETY:
+ // - Our safety contract requires that `ptr` be contained within
`Self`.
+ // - Our safety contract requires the caller ensure that it is safe
for us to take an
+ // immutable reference.
+ unsafe { &*ptr }
+ }
+
+ unsafe fn from_raw_mut<'a>(ptr: *mut bindings::drm_connector_state) -> &'a
mut Self {
+ // Our data layout starts with `bindings::drm_connector_state`.
+ let ptr: *mut Self = ptr.cast();
+
+ // SAFETY:
+ // - Our safety contract requires that `ptr` be contained within
`Self`.
+ // - Our safety contract requires the caller ensure it is safe for us
to take a mutable
+ // reference.
+ unsafe { &mut *ptr }
+ }
+}
+
+unsafe extern "C" fn atomic_duplicate_state_callback<T: DriverConnectorState>(
+ connector: *mut bindings::drm_connector,
+) -> *mut bindings::drm_connector_state {
+ // SAFETY: DRM guarantees that `connector` points to a valid initialized
`drm_connector`.
+ let state = unsafe { (*connector).state };
+ if state.is_null() {
+ return null_mut();
+ }
+
+ // SAFETY:
+ // - We just verified that `state` is non-null
+ // - This cast is guaranteed to be safe via our type invariants.
+ let state = unsafe { ConnectorState::<T>::from_raw(state) };
+
+ let new = Box::try_init(
+ try_init!(ConnectorState::<T> {
+ state: bindings::drm_connector_state {
+ ..Default::default()
+ },
+ inner: state.inner.clone()
+ }),
+ GFP_KERNEL,
+ );
+
+ if let Ok(mut new) = new {
+ // SAFETY:
+ // - `new` provides a valid pointer to a newly allocated
`drm_plane_state` via type
s/plane/connector/
+ // invariants
+ // - This initializes `new` via memcpy()
+ unsafe {
+ bindings::__drm_atomic_helper_connector_duplicate_state(connector,
new.as_raw_mut())
+ };
+
+ KBox::into_raw(new).cast()
+ } else {
+ null_mut()
+ }
+}
+
+unsafe extern "C" fn atomic_destroy_state_callback<T: DriverConnectorState>(
+ _connector: *mut bindings::drm_connector,
+ connector_state: *mut bindings::drm_connector_state,
+) {
+ // SAFETY: DRM guarantees that `state` points to a valid instance of
`drm_connector_state`
+ unsafe {
bindings::__drm_atomic_helper_connector_destroy_state(connector_state) };
+
+ // SAFETY:
+ // - DRM guarantees we are the only one with access to this
`drm_connector_state`
+ // - This cast is safe via our type invariants.
+ drop(unsafe { KBox::from_raw(connector_state.cast::<ConnectorState<T>>())
});
+}
+
+unsafe extern "C" fn connector_reset_callback<T: DriverConnectorState>(
+ connector: *mut bindings::drm_connector,
+) {
+ // SAFETY: DRM guarantees that `state` points to a valid instance of
`drm_connector_state`
+ let state = unsafe { (*connector).state };
+ if !state.is_null() {
+ // SAFETY:
+ // - We're guaranteed `connector` is `Connector<T>` via type invariants
+ // - We're guaranteed `state` is `ConnectorState<T>` via type
invariants.
+ unsafe { atomic_destroy_state_callback::<T>(connector, state) }
+
+ // SAFETY: No special requirements here, DRM expects this to be NULL
+ unsafe { (*connector).state = null_mut() };
+ }
+
+ // Unfortunately, this is the best we can do at the moment as this FFI
callback was mistakenly
+ // presumed to be infallible :(
+ let new = KBox::new(ConnectorState::<T>::default(), GFP_KERNEL).expect("Blame
the API, sorry!");
+
+ // DRM takes ownership of the state from here, resets it, and then assigns
it to the connector
+ // SAFETY:
+ // - DRM guarantees that `connector` points to a valid instance of
`drm_connector`.
+ // - The cast to `drm_connector_state` is safe via `ConnectorState`s type
invariants.
+ unsafe { bindings::__drm_atomic_helper_connector_reset(connector,
Box::into_raw(new).cast()) };
+}
--
2.48.1
--
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
