[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Zv5WGMxZHAeRkgZI@louis-chauvet-laptop>
Date: Thu, 3 Oct 2024 10:30:16 +0200
From: Louis Chauvet <louis.chauvet@...tlin.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 06/35] rust: drm/kms: Add drm_plane bindings
Hi Lyude
Thank you for all this amazing work!
[...]
> +impl<T: DriverPlane> Plane<T> {
> + /// Construct a new [`Plane`].
> + ///
> + /// A driver may use this from their [`Kms::create_objects`] callback in order to construct new
> + /// [`Plane`] objects.
> + ///
> + /// [`Kms::create_objects`]: kernel::drm::kms::Kms::create_objects
> + pub fn new<'a, 'b: 'a, const FMT_COUNT: usize, const MOD_COUNT: usize>(
> + dev: &'a UnregisteredKmsDevice<'a, T::Driver>,
> + possible_crtcs: u32,
> + formats: &'static FormatList<FMT_COUNT>,
> + format_modifiers: Option<&'static ModifierList<MOD_COUNT>>,
> + type_: PlaneType,
> + name: Option<&CStr>,
> + args: T::Args,
> + ) -> Result<&'b Self> {
Here I have a little comment about this API, I really like the fact that
FormatList and ModifierList have a type fixed length, but I fear it will
be limiting for the drivers. The same apply for the &'static lifetime,
does it really need to be static?
For example, with the introduction of ConfigFS interface in VKMS (I did
not send this part), I need to be able to create a plane with any number
of formats/modifier dynamically according to the userspace configuration:
so a dynamically allocated array, which is not 'static and not
fixed-length.
I think here you can easly remove the &'static requirement as the
format list and format modifiers are copied by drm core [1]. Do you think
it is also feasable to use a slice instead of a custom *List type?
[1]:https://elixir.bootlin.com/linux/v6.11.1/source/drivers/gpu/drm/drm_plane.c#L442
> + let this: Pin<Box<Self>> = Box::try_pin_init(
> + try_pin_init!(Self {
> + plane: Opaque::new(bindings::drm_plane {
> + helper_private: &T::OPS.helper_funcs,
> + ..Default::default()
> + }),
> + inner <- T::new(dev, args),
> + _p: PhantomPinned
> + }),
> + GFP_KERNEL
> + )?;
> +
> + // SAFETY: FFI call with no special requirements
I don't know what should be the granularity of safety comments, but I
think drm_universal_plane_init requires some pointers to be valid (at
least dev, this, formats, funcs)
> + to_result(unsafe {
> + bindings::drm_universal_plane_init(
> + dev.as_raw(),
> + this.as_raw(),
> + possible_crtcs,
> + &T::OPS.funcs,
> + formats.as_ptr(),
> + formats.raw_len() as _,
> + format_modifiers.map_or(null(), |f| f.as_ptr()),
> + type_ as _,
> + name.map_or(null(), |n| n.as_char_ptr())
> + )
> + })?;
> +
> + // Convert the box into a raw pointer, we'll re-assemble it in plane_destroy_callback()
> + // SAFETY: We don't move anything
> + Ok(unsafe { &*Box::into_raw(Pin::into_inner_unchecked(this)) })
> + }
> +}
[...]
--
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Powered by blists - more mailing lists