Message-Id: <4D4D46ED-62C0-4B6A-A560-68D5F546AE21@collabora.com>
Date: Tue, 26 Nov 2024 17:34:50 -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>,
 Danilo Krummrich <dakr@...hat.com>,
 Mika Westerberg <mika.westerberg@...ux.intel.com>,
 open list <linux-kernel@...r.kernel.org>
Subject: Re: [WIP RFC v2 04/35] rust: drm/kms: Introduce the main
 ModeConfigObject traits

Hi Lyude,

> On 30 Sep 2024, at 20:09, Lyude Paul <lyude@...hat.com> wrote:
> 
> The KMS API has a very consistent idea of a "mode config object", which
> includes any object with a drm_mode_object struct embedded in it. These
> objects have their own object IDs which DRM exposes to userspace, and we
> introduce the ModeConfigObject trait to represent any object matching these
> characteristics.
> 
> One slightly less consistent trait of these objects however: some mode
> objects have a reference count, while others don't. Since rust requires
> that we are able to define the lifetime of an object up-front, we introduce
> two other super-traits of ModeConfigObject for this:
> 
> * StaticModeObject - this trait represents any mode object which does not
>  have a reference count of its own. Such objects can be considered to
>  share the lifetime of their parent KMS device
> * RcModeObject - this trait represents any mode object which does have its
>  own reference count. Objects implementing this trait get a free blanket
>  implementation of AlwaysRefCounted, and as such can be used with the ARef
>  container without us having to implement AlwaysRefCounted for each
>  individual mode object.
> 
> This will be able to handle most lifetimes we'll need with one exception:
> it's entirely possible a driver may want to hold a "owned" reference to a
> static mode object. We allow for this by introducing the KmsRef container,
> which grabs an owned refcount to the parent KMS device of a
> StaticModeObject and holds a pointer to said object - essentially allowing
> it to act identically to an owned refcount by preventing the device's
> lifetime from ending until the KmsRef is dropped. I choose not to use
> AlwaysRefCounted for this as holding a refcount to the device has its own
> set of implications since if you forget to drop the KmsRef the device will
> never be destroyed.
> 
> Signed-off-by: Lyude Paul <lyude@...hat.com>
> ---
> rust/bindings/bindings_helper.h |   1 +
> rust/kernel/drm/kms.rs          | 107 ++++++++++++++++++++++++++++++++
> 2 files changed, 108 insertions(+)
> 
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 9803e0ecac7c1..ba1871b05b7fa 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -17,6 +17,7 @@
> #include <drm/drm_gem.h>
> #include <drm/drm_gem_framebuffer_helper.h>
> #include <drm/drm_gem_shmem_helper.h>
> +#include <drm/drm_mode_object.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 d3558a5eccc54..f1a8ba4b7e296 100644
> --- a/rust/kernel/drm/kms.rs
> +++ b/rust/kernel/drm/kms.rs
> @@ -228,3 +228,110 @@ impl<T, K> KmsDriver for T
> where
>     T: Driver<Kms = K>,
>     K: Kms<Driver = T> {}
> +
> +/// A modesetting object in DRM.
> +///
> +/// This is any type of object where the underlying C object contains a [`struct drm_mode_object`].
> +///
> +/// [`struct drm_mode_object`]: srctree/include/drm/drm_mode_object.h
> +pub trait ModeObject: Sealed + Send + Sync {

Can you briefly document why these bounds are needed?

> +    /// The parent driver for this [`ModeObject`].
> +    type Driver: KmsDriver;
> +
> +    /// Return the [`Device`] for this [`ModeObject`].
> +    fn drm_dev(&self) -> &Device<Self::Driver>;
> +
> +    /// Return a pointer to the [`struct drm_mode_object`] for this [`ModeObject`].
> +    ///
> +    /// [`struct drm_mode_object`]: (srctree/include/drm/drm_mode_object.h)
> +    fn raw_mode_obj(&self) -> *mut bindings::drm_mode_object;
> +}
> +
> +/// A trait for modesetting objects which don't come with their own reference-counting.
> +///
> +/// Some [`ModeObject`] types in DRM do not have a reference count. These types are considered
> +/// "static" and share the lifetime of their parent [`Device`]. To retrieve an owned reference to
> +/// such types, see [`KmsRef`].
> +///
> +/// # Safety
> +///
> +/// This trait must only be implemented for modesetting objects which do not have a refcount within
> +/// their [`struct drm_mode_object`], otherwise [`KmsRef`] can't guarantee the object will stay
> +/// alive.
> +///
> +/// [`struct drm_mode_object`]: (srctree/include/drm/drm_mode_object.h)
> +pub unsafe trait StaticModeObject: ModeObject {}
> +
> +/// An owned reference to a [`StaticModeObject`].
> +///
> +/// Note that since [`StaticModeObject`] types share the lifetime of their parent [`Device`], the
> +/// parent [`Device`] will stay alive as long as this type exists. Thus, users should be aware that
> +/// storing a [`KmsRef`] within a [`ModeObject`] is a circular reference.
> +///
> +/// # Invariants
> +///
> +/// `self.0` points to a valid instance of `T` throughout the lifetime of this type.
> +pub struct KmsRef<T: StaticModeObject>(NonNull<T>);
> +
> +// SAFETY: Owned references to DRM device are thread-safe.
> +unsafe impl<T: StaticModeObject> Send for KmsRef<T> {}
> +unsafe impl<T: StaticModeObject> Sync for KmsRef<T> {}
> +
> +impl<T: StaticModeObject> From<&T> for KmsRef<T> {
> +    fn from(value: &T) -> Self {
> +        // We will drop the reference we leak here in Drop
> +        value.drm_dev().inc_ref();
> +
> +        Self(value.into())
> +    }
> +}
> +
> +impl<T: StaticModeObject> Drop for KmsRef<T> {
> +    fn drop(&mut self) {
> +        // SAFETY: We're reclaiming the reference we leaked in From<&T>
> +        drop(unsafe { ARef::from_raw(self.drm_dev().into()) })
> +    }
> +}
> +
> +impl<T: StaticModeObject> Deref for KmsRef<T> {
> +    type Target = T;
> +
> +    fn deref(&self) -> &Self::Target {
> +        // SAFETY: We're guaranteed object will point to a valid object as long as we hold dev
> +        unsafe { self.0.as_ref() }
> +    }
> +}
> +
> +impl<T: StaticModeObject> Clone for KmsRef<T> {
> +    fn clone(&self) -> Self {
> +        self.drm_dev().inc_ref();
> +
> +        Self(self.0)
> +    }
> +}
> +
> +/// A trait for [`ModeObject`] which is reference counted.
> +///
> +/// This trait is implemented by DRM for any [`ModeObject`] which has a reference count provided by
> +/// [`struct drm_mode_object`]. It provides a common implementation of [`AlwaysRefCounted`], since
> +/// all [`RcModeObject`] types use the same functions for refcounting.
> +///
> +/// # Safety
> +///
> +/// The [`ModeObject`] must initialize the refcount in its [`struct drm_mode_object`] field.
> +///
> +/// [`struct drm_mode_object`]: (srctree/include/drm/drm_mode_object.h)
> +pub unsafe trait RcModeObject: ModeObject {}
> +
> +unsafe impl<T: RcModeObject> AlwaysRefCounted for T {
> +    fn inc_ref(&self) {
> +        // SAFETY: FFI call with no special requirements
> +        unsafe { bindings::drm_mode_object_get(self.raw_mode_obj()) }

Well, at least the pointer has to be valid. I assume that passing core::ptr::null_mut() here will crash,
for example. Also, do we have to worry about races? T is Sync, so I assume you mean to have
this call reachable from multiple threads.

The kref docs seem to indicate this is not a problem:

```
This way, it doesn’t matter what order the two threads handle the data, the kref_put() handles knowing when the data is not referenced any more and releasing it. The kref_get() does not require a lock, since we already have a valid pointer that we own a refcount for. The put needs no lock because nothing tries to get the data without already holding a pointer.
```

Regardless, IMHO it’s good to document it here as well.
 
> +    }
> +
> +    unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
> +        // SAFETY: We never expose modesetting objects in our interfaces to users before they're
> +        // initialized
> +        unsafe { bindings::drm_mode_object_put(obj.as_ref().raw_mode_obj()) }

Same here, pointer must be valid.

> +    }
> +}
> -- 
> 2.46.1
> 
> 

— Daniel

Powered by blists - more mailing lists