Re: [WIP RFC v2 06/35] rust: drm/kms: Add drm_plane bindings

[Louis Chauvet]

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