Message-Id: <C75763C3-A2A4-410F-934D-582B44A3B550@collabora.com>
Date: Wed, 27 Nov 2024 12:51:08 -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 13/35] WIP: rust: drm/kms: Add OpaqueConnector and
 OpaqueConnectorState

Hi Lyude,

> On 30 Sep 2024, at 20:09, Lyude Paul <lyude@...hat.com> wrote:
> 
> Since we allow drivers to have multiple implementations of DriverConnector
> and DriverConnectorState (in C, the equivalent of this is having multiple
> structs which embed drm_connector) - there are some situations we will run
> into where it's not possible for us to know the corresponding
> DriverConnector or DriverConnectorState for a given connector. The most
> obvious one is iterating through all connectors on a KMS device.
> 
> So, take advantage of the various connector traits we added to introduce
> OpaqueConnector<> and OpaqueConnectorState<> which both can be used as a
> DRM connector and connector state respectively without needing to know the
> corresponding traits.
> 
> Signed-off-by: Lyude Paul <lyude@...hat.com>
> 
> ---
> 
> TODO:
> * Add upcast functions for these types
> 
> Signed-off-by: Lyude Paul <lyude@...hat.com>
> ---
> rust/kernel/drm/kms/connector.rs | 108 +++++++++++++++++++++++++++++++
> 1 file changed, 108 insertions(+)
> 
> diff --git a/rust/kernel/drm/kms/connector.rs b/rust/kernel/drm/kms/connector.rs
> index ec842ebc111ae..98ac7fb781d4e 100644
> --- a/rust/kernel/drm/kms/connector.rs
> +++ b/rust/kernel/drm/kms/connector.rs
> @@ -359,6 +359,64 @@ unsafe fn from_raw<'a>(ptr: *mut bindings::drm_connector) -> &'a Self {
>     T::get_modes(connector.guard(&guard), &guard)
> }
> 
> +/// A [`struct drm_connector`] without a known [`DriverConnector`] implementation.
> +///
> +/// This is mainly for situations where our bindings can't infer the [`DriverConnector`]
> +/// implementation for a [`struct drm_connector`] automatically. It is identical to [`Connector`],
> +/// except that it does not provide access to the driver's private data.
> +///
> +/// TODO: Add upcast methods for this

You mean a way to go from OpaqueConnector to Connector?

> +///
> +/// # Invariants
> +///
> +/// - `connector` is initialized for as long as this object is exposed to users.
> +/// - The data layout of this type is equivalent to [`struct drm_connector`].
> +///
> +/// [`struct drm_connector`]: srctree/include/drm/drm_connector.h
> +#[repr(transparent)]
> +pub struct OpaqueConnector<T: KmsDriver> {
> +    connector: Opaque<bindings::drm_connector>,
> +    _p: PhantomData<T>
> +}
> +
> +impl<T: KmsDriver> Sealed for OpaqueConnector<T> {}
> +
> +impl<T: KmsDriver> AsRawConnector for OpaqueConnector<T> {
> +    type Driver = T;
> +    type State = OpaqueConnectorState<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 is identical to `bindings::drm_connector`
> +        unsafe { &*ptr.cast() }
> +    }
> +}
> +
> +impl<T: KmsDriver> ModeObject for OpaqueConnector<T> {
> +    type Driver = T;
> +
> +    fn drm_dev(&self) -> &Device<Self::Driver> {
> +        // SAFETY: The parent device for a DRM connector will never outlive the connector, and this
> +        // pointer is invariant through the lifetime of the connector
> +        unsafe { Device::borrow((*self.as_raw()).dev) }
> +    }
> +
> +    fn raw_mode_obj(&self) -> *mut bindings::drm_mode_object {
> +        // SAFETY: We don't expose DRM connectors to users before `base` is initialized
> +        unsafe { &mut (*self.as_raw()).base }
> +    }
> +}
> +
> +// SAFETY: Connectors are reference counted mode objects
> +unsafe impl<T: KmsDriver> RcModeObject for OpaqueConnector<T> {}
> +
> +// SAFETY: Our connector interfaces are guaranteed to be thread-safe
> +unsafe impl<T: KmsDriver> Send for OpaqueConnector<T> {}
> +unsafe impl<T: KmsDriver> Sync for OpaqueConnector<T> {}
> +
> /// A privileged [`Connector`] obtained while holding a [`ModeConfigGuard`].
> ///
> /// This provides access to various methods for [`Connector`] that must happen under lock, such as
> @@ -537,6 +595,56 @@ unsafe fn from_raw_mut<'a>(ptr: *mut bindings::drm_connector_state) -> &'a mut S
>     }
> }
> 
> +/// A [`struct drm_connector_state`] without a known [`DriverConnectorState`] implementation.
> +///
> +/// This is mainly for situations where our bindings can't infer the [`DriverConnectorState`]
> +/// implementation for a [`struct drm_connector_state`] automatically. It is identical to
> +/// [`Connector`], except that it does not provide access to the driver's private data.
> +///
> +/// TODO: Add upcast functions
> +///
> +/// # Invariants
> +///
> +/// - `state` is initialized for as long as this object is exposed to users.
> +/// - The data layout of this type is identical to [`struct drm_connector_state`].
> +/// - 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.

By the way, as you did in a previous commit, I wonder whether it would be better to have the invariants
in a single place, since I’ve noticed that most of them are quite similar.

Something like “The invariants for this type are the same as the ones for Foo”

This way, if you need to update your design, these will not get out of sync that easily.

> +///
> +/// [`struct drm_connector_state`]: srctree/include/drm/drm_connector.h
> +/// [`drm_atomic_helper_commit_hw_done`]: srctree/include/drm/drm_atomic_helper.h
> +#[repr(transparent)]
> +pub struct OpaqueConnectorState<T: KmsDriver> {
> +    state: bindings::drm_connector_state,
> +    _p: PhantomData<T>
> +}
> +
> +impl<T: KmsDriver> AsRawConnectorState for OpaqueConnectorState<T> {
> +    type Connector = OpaqueConnector<T>;
> +}
> +
> +impl<T: KmsDriver> private::AsRawConnectorState for OpaqueConnectorState<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: KmsDriver> FromRawConnectorState for OpaqueConnectorState<T> {
> +    unsafe fn from_raw<'a>(ptr: *const bindings::drm_connector_state) -> &'a Self {
> +        // SAFETY: Our data layout is identical to `bindings::drm_connector_state`
> +        unsafe { &*ptr.cast() }
> +    }
> +
> +    unsafe fn from_raw_mut<'a>(ptr: *mut bindings::drm_connector_state) -> &'a mut Self {
> +        // SAFETY: Our data layout is identical to `bindings::drm_connector_state`
> +        unsafe { &mut *ptr.cast() }
> +    }
> +}
> +
> unsafe extern "C" fn atomic_duplicate_state_callback<T: DriverConnectorState>(
>     connector: *mut bindings::drm_connector
> ) -> *mut bindings::drm_connector_state
> -- 
> 2.46.1
> 

This LGTM overall.

— Daniel

Powered by blists - more mailing lists