Message-Id: <DC5G2LJGBN3D.8JSJY9U25IAW@kernel.org>
Date: Mon, 18 Aug 2025 11:26:32 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Daniel Almeida" <daniel.almeida@...labora.com>
Cc: "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" <lossin@...nel.org>, "Andreas
 Hindborg" <a.hindborg@...nel.org>, "Alice Ryhl" <aliceryhl@...gle.com>,
 "Trevor Gross" <tmgross@...ch.edu>, "Alexandre Courbot"
 <acourbot@...dia.com>, <linux-kernel@...r.kernel.org>,
 <rust-for-linux@...r.kernel.org>, <kernel@...labora.com>,
 <linux-media@...r.kernel.org>
Subject: Re: [PATCH 3/7] rust: v4l2: add support for video device nodes

On Mon Aug 18, 2025 at 7:49 AM CEST, Daniel Almeida wrote:
> Video device nodes back the actual /dev/videoX files. They expose a rich
> ioctl interface for which we will soon add support for and allow for
> modelling complex hardware through a topology of nodes, each modelling a
> particular hardware function or component.
>
> V4l2 drivers rely on video device nodes pretty extensively, so add a
> minimal Rust abstraction for them. The abstraction currently does the
> bare-minimum to let users register a V4L2 device node. It also
> introduces the video::Driver trait that will be implemented by Rust v4l2
> drivers. This trait will then be refined in future patches.
>
> Signed-off-by: Daniel Almeida <daniel.almeida@...labora.com>
> ---
>  rust/helpers/v4l2-device.c       |  16 +++
>  rust/kernel/media/v4l2/device.rs |   1 -
>  rust/kernel/media/v4l2/mod.rs    |   3 +
>  rust/kernel/media/v4l2/video.rs  | 251 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 270 insertions(+), 1 deletion(-)
>
> diff --git a/rust/helpers/v4l2-device.c b/rust/helpers/v4l2-device.c
> index d19b46e8283ce762b4259e3df5ecf8bb18e863e9..0ead52b9a1ccc0fbc4d7df63578b334b17c05b70 100644
> --- a/rust/helpers/v4l2-device.c
> +++ b/rust/helpers/v4l2-device.c
> @@ -6,3 +6,19 @@ void rust_helper_v4l2_device_get(struct v4l2_device *v4l2_dev)
>  {
>      v4l2_device_get(v4l2_dev);
>  }
> +
> +void rust_helper_video_get(struct video_device *vdev)
> +{
> +    get_device(&vdev->dev);

Rust helpers shouldn't encode semantics. I think you want to use video_get()
instead.

> +}
> +
> +void rust_helper_video_put(struct video_device *vdev)
> +{
> +    put_device(&vdev->dev);

video_put()

> +/// Represents the registration of a V4L2 device node.
> +pub struct Registration<T: Driver>(ARef<Device<T>>);
> +
> +impl<T: Driver> Registration<T> {
> +    /// Returns a new `Registration` for the given device, which guarantees that
> +    /// the underlying device node is properly initialized and registered, which
> +    /// means that it can be safely used.
> +    pub fn new(
> +        dev: &v4l2::device::Device<T>,
> +        data: impl PinInit<<T as Driver>::Data, Error>,
> +        flags: alloc::Flags,
> +    ) -> Result<Self> {

Same comment regarding Device having its own constructor as for
v4l::device::Registration. I don't see a reason why Registration::new() should
serve as constructor for Device.

Additionally, I think we should use devres::register() for the Registration, such
that you can provide &Device<Bound> cookies in the video callbacks / ioctls.

As far as I can see, video devices are synchronized when unregistered [1]
-- let's take advantage of that.

We do the same thing in the PWM abstractions [2], which is a great optimization.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/v4l2-core/v4l2-dev.c?h=v6.16#n1105
[2] https://lore.kernel.org/linux-pwm/20250806-rust-next-pwm-working-fan-for-sending-v13-3-690b669295b6@samsung.com/

> +        let video_dev = try_pin_init!(Device {
> +            inner <- Opaque::try_ffi_init(move |slot: *mut bindings::video_device| {
> +                let opts: DeviceOptions<'_, T> = DeviceOptions {
> +                    dev,
> +                    _phantom: PhantomData
> +                };
> +
> +                // SAFETY: `DeviceOptions::into_raw` produces a valid
> +                // `bindings::video_device` that is ready for registration.
> +                unsafe { slot.write(opts.into_raw()) };
> +
> +
> +                // SAFETY: It is OK to call this function on a zeroed
> +                // `video_device` and a valid `v4l2::Device` reference.
> +                to_result(unsafe { bindings::video_register_device(slot, T::NODE_TYPE as c_uint, -1) })
> +            }),
> +            data <- data,
> +        });
> +
> +        let video_dev = KBox::pin_init(video_dev, flags)?;
> +
> +        // SAFETY: We will be passing the ownership to ARef<T>, which treats the
> +        // underlying memory as pinned throughout its lifetime.
> +        //
> +        // This is true because:
> +        //
> +        // - ARef<T> does not expose a &mut T, so there is no way to move the T
> +        // (e.g.: via a `core::mem::swap` or similar).
> +        // - ARef<T>'s member functions do not move the T either.
> +        let ptr = KBox::into_raw(unsafe { Pin::into_inner_unchecked(video_dev) });
> +
> +        // SAFETY:
> +        //
> +        // - the refcount is one, and we are transfering the ownership of that
> +        // increment to the ARef.
> +        // - `ptr` is non-null as it came from `KBox::into_raw`, so it is safe
> +        // to call `NonNulll::new_unchecked`.
> +        Ok(Self(unsafe { ARef::from_raw(NonNull::new_unchecked(ptr)) }))
> +    }
> +
> +    /// Returns a reference to the underlying video device.
> +    pub fn device(&self) -> &video::Device<T> {
> +        &self.0
> +    }
> +}
> +
> +impl<T: Driver> Drop for Registration<T> {
> +    fn drop(&mut self) {
> +        // SAFETY: `self.0` is a valid `video_device` that was registered in
> +        // [`Registration::new`].
> +        unsafe { bindings::video_unregister_device(self.0.as_raw()) };
> +    }
> +}
>
> -- 
> 2.50.1

Powered by blists - more mailing lists