Message-Id: <7CBB09E7-1B00-4F75-8EC5-D6139734654C@collabora.com>
Date: Mon, 12 May 2025 13:29:56 -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,
 Danilo Krummrich <dakr@...nel.org>,
 mcanal@...lia.com,
 Alice Ryhl <aliceryhl@...gle.com>,
 Maxime Ripard <mripard@...nel.org>,
 Simona Vetter <sima@...ll.ch>,
 Miguel Ojeda <ojeda@...nel.org>,
 Alex Gaynor <alex.gaynor@...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@...nel.org>,
 Trevor Gross <tmgross@...ch.edu>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Asahi Lina <lina@...hilina.net>,
 Wedson Almeida Filho <wedsonaf@...il.com>,
 open list <linux-kernel@...r.kernel.org>
Subject: Re: [RFC v3 05/33] rust: drm/kms: Add drm_plane bindings

Hi Lyude,

This patch is similar to the last one, I see that most of my
comments also apply here.

> On 5 Mar 2025, at 19:59, Lyude Paul <lyude@...hat.com> wrote:
> 
> The next step is adding a set of basic bindings to create a plane, which
> has to happen before we can create a CRTC (since we need to be able to at
> least specify a primary plane for a CRTC upon creation). This mostly
> follows the same general pattern as connectors (AsRawPlane,
> AsRawPlaneState, etc.).
> 
> There is one major difference with planes vs. other types of atomic mode
> objects: drm_plane_state isn't the only base plane struct used in DRM
> drivers, as some drivers will use helpers like drm_shadow_plane_state which
> have a drm_plane_state embedded within them.
> 
> Since we'll eventually be adding bindings for shadow planes, we introduce a
> PlaneStateHelper trait - which represents any data type which can be used
> as the main wrapping structure around a drm_plane_state - and we implement
> this trait for PlaneState<T>. This trait can be used in our C callbacks to
> allow for drivers to use different wrapping structures without needing to
> implement a separate set of FFI callbacks for each type. Currently planes
> are the only type I'm aware of which do this.
> 
> Signed-off-by: Lyude Paul <lyude@...hat.com>
> 
> ---
> 
> V2:
> * Start using Gerry Guo's updated #[vtable] function so that our driver
>  operations table has a static location in memory

nit: Gary’s name is misspelled.

> 
> V3:
> * Add safety comment for implementation of ModeObject
> * Make AsRawPlane unsafe, since we need a guarantee that `as_raw()` always
>  returns a valid pointer to an initialized drm_plane.
> * Add comments to __drm_atomic_helper_duplicate_state()
> * Switch `PlaneType` to camel casing
> * Improve safety comment in `Plane::<T>::new()`
> * Fix parameter types for `formats` and `format_modifiers`, as pointed out
>  by Louis Chauvet DRM will copy all of these into its own storage.
> * Improve safety comments in FromRawPlaneState
> * Introduce UnregisteredPlane type
> * Don't have AsRawPlane be a supertrait of StaticModeObject. We don't want
>  Unregistered mode object variants to be able to return a pointer to the
>  DRM device since that would break the UnregisteredKmsDevice pattern.
> * Change name of PlaneType to Type (for consistency with the other type IDs
>  we've adde)
> * Use addr_of_mut! in more places instead of &mut
> 
> Signed-off-by: Lyude Paul <lyude@...hat.com>
> ---
> rust/bindings/bindings_helper.h |   2 +
> rust/kernel/drm/fourcc.rs       |   1 -
> rust/kernel/drm/kms.rs          |   1 +
> rust/kernel/drm/kms/plane.rs    | 621 ++++++++++++++++++++++++++++++++
> 4 files changed, 624 insertions(+), 1 deletion(-)
> create mode 100644 rust/kernel/drm/kms/plane.rs
> 
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index c41a3309223b2..5b85f3faca525 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -10,6 +10,7 @@
> #include <drm/drm_atomic_helper.h>
> #include <drm/clients/drm_client_setup.h>
> #include <drm/drm_connector.h>
> +#include <drm/drm_plane.h>
> #include <drm/drm_device.h>
> #include <drm/drm_drv.h>
> #include <drm/drm_file.h>
> @@ -18,6 +19,7 @@
> #include <drm/drm_gem.h>
> #include <drm/drm_gem_framebuffer_helper.h>
> #include <drm/drm_gem_shmem_helper.h>
> +#include <drm/drm_plane.h>
> #include <drm/drm_ioctl.h>
> #include <kunit/test.h>
> #include <linux/blk-mq.h>
> diff --git a/rust/kernel/drm/fourcc.rs b/rust/kernel/drm/fourcc.rs
> index 62203478b5955..a30e40dbc037c 100644
> --- a/rust/kernel/drm/fourcc.rs
> +++ b/rust/kernel/drm/fourcc.rs
> @@ -11,7 +11,6 @@ const fn fourcc_code(a: u8, b: u8, c: u8, d: u8) -> u32 {
> 
> // TODO: We manually import this because we don't have a reasonable way of getting constants from
> // function-like macros in bindgen yet.
> -#[allow(dead_code)]
> pub(crate) const FORMAT_MOD_INVALID: u64 = 0xffffffffffffff;
> 
> // TODO: We need to automate importing all of these. For the time being, just add the single one
> diff --git a/rust/kernel/drm/kms.rs b/rust/kernel/drm/kms.rs
> index f10e9f83ccb78..6cc5bb53f3628 100644
> --- a/rust/kernel/drm/kms.rs
> +++ b/rust/kernel/drm/kms.rs
> @@ -3,6 +3,7 @@
> //! KMS driver abstractions for rust.
> 
> pub mod connector;
> +pub mod plane;
> 
> use crate::{
>     device,
> diff --git a/rust/kernel/drm/kms/plane.rs b/rust/kernel/drm/kms/plane.rs
> new file mode 100644
> index 0000000000000..9f262156eac6c
> --- /dev/null
> +++ b/rust/kernel/drm/kms/plane.rs
> @@ -0,0 +1,621 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +
> +//! DRM display planes.
> +//!
> +//! C header: [`include/drm/drm_plane.h`](srctree/include/drm/drm_plane.h)
> +
> +use super::{KmsDriver, ModeObject, StaticModeObject, UnregisteredKmsDevice};
> +use crate::{
> +    alloc::KBox,
> +    bindings,
> +    drm::{device::Device, fourcc::*},
> +    error::{to_result, Error},
> +    init::Zeroable,
> +    prelude::*,
> +    private::Sealed,
> +    types::{NotThreadSafe, Opaque},
> +};
> +use core::{
> +    marker::*,
> +    mem,
> +    ops::*,
> +    pin::Pin,
> +    ptr::{addr_of_mut, null, null_mut},
> +};
> +use macros::pin_data;
> +
> +/// The main trait for implementing the [`struct drm_plane`] API for [`Plane`].
> +///
> +/// Any KMS driver should have at least one implementation of this type, which allows them to create
> +/// [`Plane`] objects. Additionally, a driver may store driver-private data within the type that
> +/// implements [`DriverPlane`] - and it will be made available when using a fully typed [`Plane`]
> +/// object.
> +///
> +/// # Invariants
> +///
> +/// - Any C FFI callbacks generated using this trait are guaranteed that passed-in
> +///   [`struct drm_plane`] pointers are contained within a [`Plane<Self>`].
> +/// - Any C FFI callbacks generated using this trait are guaranteed that passed-in
> +///   [`struct drm_plane_state`] pointers are contained within a [`PlaneState<Self::State>`].
> +///
> +/// [`struct drm_plane`]: srctree/include/drm/drm_plane.h
> +/// [`struct drm_plane_state`]: srctree/include/drm/drm_plane.h
> +#[vtable]
> +pub trait DriverPlane: Send + Sync + Sized {
> +    /// The generated C vtable for this [`DriverPlane`] implementation.
> +    #[unique]
> +    const OPS: &'static DriverPlaneOps = &DriverPlaneOps {
> +        funcs: bindings::drm_plane_funcs {
> +            update_plane: Some(bindings::drm_atomic_helper_update_plane),
> +            disable_plane: Some(bindings::drm_atomic_helper_disable_plane),
> +            destroy: Some(plane_destroy_callback::<Self>),
> +            reset: Some(plane_reset_callback::<Self>),
> +            set_property: None,
> +            atomic_duplicate_state: Some(atomic_duplicate_state_callback::<Self::State>),
> +            atomic_destroy_state: Some(atomic_destroy_state_callback::<Self::State>),
> +            atomic_set_property: None,
> +            atomic_get_property: None,
> +            late_register: None,
> +            early_unregister: None,
> +            atomic_print_state: None,
> +            format_mod_supported: None,
> +        },
> +
> +        helper_funcs: bindings::drm_plane_helper_funcs {
> +            prepare_fb: None,
> +            cleanup_fb: None,
> +            begin_fb_access: None,
> +            end_fb_access: None,
> +            atomic_check: None,
> +            atomic_update: None,
> +            atomic_enable: None,
> +            atomic_disable: None,
> +            atomic_async_check: None,
> +            atomic_async_update: None,
> +            panic_flush: None,
> +            get_scanout_buffer: None,
> +        },
> +    };
> +
> +    /// The type to pass to the `args` field of [`UnregisteredPlane::new`].
> +    ///
> +    /// This type will be made available in in the `args` argument of [`Self::new`]. Drivers which
> +    /// don't need this can simply pass [`()`] here.
> +    type Args;
> +
> +    /// The parent [`KmsDriver`] implementation.
> +    type Driver: KmsDriver;
> +
> +    /// The [`DriverPlaneState`] implementation for this [`DriverPlane`].
> +    ///
> +    /// See [`DriverPlaneState`] for more info.
> +    type State: DriverPlaneState;
> +
> +    /// The constructor for creating a [`Plane`] using this [`DriverPlane`] implementation.
> +    ///
> +    /// Drivers may use this to instantiate their [`DriverPlane`] object.
> +    fn new(device: &Device<Self::Driver>, args: Self::Args) -> impl PinInit<Self, Error>;
> +}
> +
> +/// The generated C vtable for a [`DriverPlane`].
> +///
> +/// This type is created internally by DRM.
> +pub struct DriverPlaneOps {
> +    funcs: bindings::drm_plane_funcs,
> +    helper_funcs: bindings::drm_plane_helper_funcs,
> +}
> +
> +#[derive(Copy, Clone, Debug, PartialEq, Eq)]
> +#[repr(u32)]
> +/// An enumerator describing a type of [`Plane`].
> +///
> +/// This is mainly just relevant for DRM legacy drivers.
> +///
> +/// # Invariants
> +///
> +/// This type is identical to [`enum drm_plane_type`].
> +///
> +/// [`enum drm_plane_type`]: srctree/include/drm/drm_plane.h

nit: the order of attributes and docs is reversed here.

> +pub enum Type {
> +    /// Overlay planes represent all non-primary, non-cursor planes. Some drivers refer to these
> +    /// types of planes as "sprites" internally.
> +    Overlay = bindings::drm_plane_type_DRM_PLANE_TYPE_OVERLAY,
> +
> +    /// A primary plane attached to a CRTC that is the most likely to be able to light up the CRTC
> +    /// when no scaling/cropping is used, and the plane covers the whole CRTC.
> +    Primary = bindings::drm_plane_type_DRM_PLANE_TYPE_PRIMARY,
> +
> +    /// A cursor plane attached to a CRTC that is more likely to be enabled when no scaling/cropping
> +    /// is used, and the framebuffer has the size indicated by [`ModeConfigInfo::max_cursor`].
> +    ///
> +    /// [`ModeConfigInfo::max_cursor`]: crate::drm::kms::ModeConfigInfo
> +    Cursor = bindings::drm_plane_type_DRM_PLANE_TYPE_CURSOR,
> +}
> +
> 

— Daniel

Powered by blists - more mailing lists