Hi Lyude,
> On 30 Sep 2024, at 20:09, Lyude Paul <lyude@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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
