Re: [RFC v3 09/33] rust: drm/kms: Add DriverConnector::get_mode callback

[Maxime Ripard <mripard@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 9921 bytes --]

On Wed, Mar 05, 2025 at 05:59:25PM -0500, Lyude Paul wrote:
> Next up is filling out some of the basic connector hotplugging callbacks -
> which we'll need for setting up the fbdev helpers for KMS devices. Note
> that connector hotplugging in DRM follows a BFL scheme: pretty much all
> probing is protected under the mighty drm_device->mode_config.lock, which
> of course is a bit counter-intuitive to rust's locking schemes where data
> is always associated with its lock.
> 
> Since that lock is embedded in an FFI type and not a rust type, we need to
> introduce our own wrapper type that acts as a lock acquisition for this.
> This brings us to introducing a few new types:
> 
> * ModeConfigGuard - the most basic lock guard, as long as this object is
>   alive we are guaranteed to be holding drm_device->mode_config.lock. This
>   object doesn't do much else on its own currently.
> * ConnectorGuard - an object which corresponds to a specific typed DRM
>   connector. This can only be acquired with a ModeConfigGuard, and will be
>   used to allow calling methods that are only safe to call with
>   drm_device->mode_config.lock held. Since it implements
>   Deref<Target=Connector<T>> as well, it can also be used for any other
>   operations that would normally be available on a DRM connector.
> 
> And finally, we add the DriverConnector::get_modes() trait method which
> drivers can use to implement the drm_connector_helper_funcs.get_modes
> callback. Note that while we make this trait method mandatory, we only do
> so for the time being since VKMS doesn't do very much with DRM connectors -
> and as such we have no need yet to implement alternative connector probing
> schemes outside of get_modes().
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> 
> ---
> V3:
> * Document uses of ManuallyDrop
> * Use addr_of_mut!() instead of &mut
> * Add some missing invariant comments
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> ---
>  rust/bindings/bindings_helper.h  |  1 +
>  rust/kernel/drm/kms.rs           | 90 +++++++++++++++++++++++++++++++-
>  rust/kernel/drm/kms/connector.rs | 62 ++++++++++++++++++++--
>  3 files changed, 147 insertions(+), 6 deletions(-)
> 
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index a6735f6fba947..27828dd36d4f2 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -21,6 +21,7 @@
>  #include <drm/drm_gem_framebuffer_helper.h>
>  #include <drm/drm_gem_shmem_helper.h>
>  #include <drm/drm_plane.h>
> +#include <drm/drm_probe_helper.h>
>  #include <drm/drm_ioctl.h>
>  #include <kunit/test.h>
>  #include <linux/blk-mq.h>
> diff --git a/rust/kernel/drm/kms.rs b/rust/kernel/drm/kms.rs
> index f0044d396e1eb..7935e935f9975 100644
> --- a/rust/kernel/drm/kms.rs
> +++ b/rust/kernel/drm/kms.rs
> @@ -8,15 +8,20 @@
>  pub mod plane;
>  
>  use crate::{
> -    device,
> +    container_of, device,
>      drm::{device::Device, drv::Driver},
>      error::to_result,
>      prelude::*,
>      private::Sealed,
> +    sync::{Mutex, MutexGuard},
>      types::*,
>  };
>  use bindings;
> -use core::{marker::PhantomData, ops::Deref, ptr::NonNull};
> +use core::{
> +    marker::PhantomData,
> +    ops::Deref,
> +    ptr::{self, addr_of_mut, NonNull},
> +};
>  
>  /// The C vtable for a [`Device`].
>  ///
> @@ -191,6 +196,23 @@ pub struct ModeConfigInfo {
>      pub preferred_fourcc: Option<u32>,
>  }
>  
> +impl<T: KmsDriver> Device<T> {
> +    /// Retrieve a pointer to the mode_config mutex
> +    #[inline]
> +    pub(crate) fn mode_config_mutex(&self) -> &Mutex<()> {
> +        // SAFETY: This lock is initialized for as long as `Device<T>` is exposed to users
> +        unsafe { Mutex::from_raw(addr_of_mut!((*self.as_raw()).mode_config.mutex)) }
> +    }
> +
> +    /// Acquire the [`mode_config.mutex`] for this [`Device`].
> +    #[inline]
> +    pub fn mode_config_lock(&self) -> ModeConfigGuard<'_, T> {
> +        // INVARIANT: We're locking mode_config.mutex, fulfilling our invariant that this lock is
> +        // held throughout ModeConfigGuard's lifetime.
> +        ModeConfigGuard(self.mode_config_mutex().lock(), PhantomData)
> +    }
> +}
> +

Again, I think the introduction of ModeConfigGuard, the new API to get
the mutex and guard from the DRM device, etc, while obviously called
for, would be better in separate patches.

>  /// A modesetting object in DRM.
>  ///
>  /// This is any type of object where the underlying C object contains a [`struct drm_mode_object`].
> @@ -314,3 +336,67 @@ unsafe fn dec_ref(obj: NonNull<Self>) {
>          unsafe { bindings::drm_mode_object_put(obj.as_ref().raw_mode_obj()) }
>      }
>  }
> +
> +/// A mode config guard.
> +///
> +/// This is an exclusive primitive that represents when [`drm_device.mode_config.mutex`] is held - as
> +/// some modesetting operations (particularly ones related to [`connectors`](connector)) are still
> +/// protected under this single lock. The lock will be dropped once this object is dropped.
> +///
> +/// # Invariants
> +///
> +/// - `self.0` is contained within a [`struct drm_mode_config`], which is contained within a
> +///   [`struct drm_device`].
> +/// - The [`KmsDriver`] implementation of that [`struct drm_device`] is always `T`.
> +/// - This type proves that [`drm_device.mode_config.mutex`] is acquired.
> +///
> +/// [`struct drm_mode_config`]: (srctree/include/drm/drm_device.h)
> +/// [`drm_device.mode_config.mutex`]: (srctree/include/drm/drm_device.h)
> +/// [`struct drm_device`]: (srctree/include/drm/drm_device.h)
> +pub struct ModeConfigGuard<'a, T: KmsDriver>(MutexGuard<'a, ()>, PhantomData<T>);
> +
> +impl<'a, T: KmsDriver> ModeConfigGuard<'a, T> {
> +    /// Construct a new [`ModeConfigGuard`].
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must ensure that [`drm_device.mode_config.mutex`] is acquired.
> +    ///
> +    /// [`drm_device.mode_config.mutex`]: (srctree/include/drm/drm_device.h)
> +    pub(crate) unsafe fn new(drm: &'a Device<T>) -> Self {
> +        // SAFETY: Our safety contract fulfills the requirements of `MutexGuard::new()`
> +        // INVARIANT: And our safety contract ensures that this type proves that
> +        // `drm_device.mode_config.mutex` is acquired.
> +        Self(
> +            unsafe { MutexGuard::new(drm.mode_config_mutex(), ()) },
> +            PhantomData,
> +        )
> +    }
> +
> +    /// Return the [`Device`] that this [`ModeConfigGuard`] belongs to.
> +    pub fn drm_dev(&self) -> &'a Device<T> {
> +        // SAFETY:
> +        // - `self` is embedded within a `drm_mode_config` via our type invariants
> +        // - `self.0.lock` has an equivalent data type to `mutex` via its type invariants.
> +        let mode_config = unsafe { container_of!(self.0.lock, bindings::drm_mode_config, mutex) };
> +
> +        // SAFETY: And that `drm_mode_config` lives in a `drm_device` via type invariants.
> +        unsafe {
> +            Device::borrow(container_of!(
> +                mode_config,
> +                bindings::drm_device,
> +                mode_config
> +            ))
> +        }
> +    }
> +
> +    /// Assert that the given device is the owner of this mode config guard.
> +    ///
> +    /// # Panics
> +    ///
> +    /// Panics if `dev` is different from the owning device for this mode config guard.
> +    #[inline]
> +    pub(crate) fn assert_owner(&self, dev: &Device<T>) {
> +        assert!(ptr::eq(self.drm_dev(), dev));
> +    }
> +}
> diff --git a/rust/kernel/drm/kms/connector.rs b/rust/kernel/drm/kms/connector.rs
> index 6fe0a7517bd55..14de3b0529f89 100644
> --- a/rust/kernel/drm/kms/connector.rs
> +++ b/rust/kernel/drm/kms/connector.rs
> @@ -4,7 +4,7 @@
>  //!
>  //! C header: [`include/drm/drm_connector.h`](srctree/include/drm/drm_connector.h)
>  
> -use super::{encoder::*, KmsDriver, ModeObject, RcModeObject};
> +use super::{encoder::*, KmsDriver, ModeConfigGuard, ModeObject, RcModeObject};
>  use crate::{
>      alloc::KBox,
>      bindings,
> @@ -17,7 +17,7 @@
>  };
>  use core::{
>      marker::*,
> -    mem,
> +    mem::{self, ManuallyDrop},
>      ops::*,
>      ptr::{addr_of_mut, null_mut},
>      stringify,
> @@ -106,7 +106,7 @@ pub trait DriverConnector: Send + Sync + Sized {
>              destroy: Some(connector_destroy_callback::<Self>),
>              force: None,
>              detect: None,
> -            fill_modes: None,
> +            fill_modes: Some(bindings::drm_helper_probe_single_connector_modes),

It's kind of what I wanted to express in my earlier statements I guess,
but I'm not really sure we should force down helpers on drivers. The
larger approach KMS has taken over the years was to provide hooks and
default implementations, with the drivers allowed to use different
implementations if they wanted to.

That approach largely worked for us I think, so I'm a bit worried about
changing that.

>              debugfs_init: None,
>              oob_hotplug_event: None,
>              atomic_duplicate_state: Some(atomic_duplicate_state_callback::<Self::State>),
> @@ -114,7 +114,7 @@ pub trait DriverConnector: Send + Sync + Sized {
>          helper_funcs: bindings::drm_connector_helper_funcs {
>              mode_valid: None,
>              atomic_check: None,
> -            get_modes: None,
> +            get_modes: Some(get_modes_callback::<Self>),
>              detect_ctx: None,

Since you pass (the equivalent of) the locking context to get_modes, I'd
rather keep the convention you have with detect here and use the _ctx
suffix, or drop the one from detect_ctx, and pass the context
everywhere. But we should be consistent there at least.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]