linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/7] rust: add initial v4l2 support
@ 2025-08-18  5:49 Daniel Almeida
  2025-08-18  5:49 ` [PATCH 1/7] rust: media: add the media module Daniel Almeida
                   ` (7 more replies)
  0 siblings, 8 replies; 17+ messages in thread
From: Daniel Almeida @ 2025-08-18  5:49 UTC (permalink / raw)
  To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, Alexandre Courbot
  Cc: linux-kernel, rust-for-linux, kernel, linux-media, Daniel Almeida

Hi,

This topic has been discussed in the last two iterations of the Media
Summit and it has been dormant since. In my humble opinion, and owing
to all the progress that Rust in the kernel has seen since my last
attempts ([0], [1]), it is time to try again.

This series reduces the scope of the original attempt considerably. It
adds APIs to register v4l2 and video devices, and just enough to process
a single ioctl (VIDIOC_QUERYCAP), including basic support for v4l2_fh.
It was tested with v4l2-ctl.

It builds upon the platform work from Danilo and others in order to
offer a concise, platform-based driver sample that is currently the only
user of the abstractions. It draws from all the work done for DRM
devices and others and uses patterns that are known to work for other
subsystems.

I hope that we can all agree that there is little that can go wrong
here.

I've also added a separate MAINTAINERS entry, as the topic of
maintaining the Rust abstractions has been a major point of contention
so far. Hopefully this settles this issue in a satisfactory way for all
involved. In other words, no one is forced to contribute or alter their
workflow in any way, while those that want to contribute are invited to
do so. This approach has worked rather well so far.

I've decided to work on this once more after being told by a few people
that they would likely try to play with Rust v4l2 drivers if only they
did not have to write all of the infrastructure themselves. This work
(and subsequent patches) will pave the way for them.

Note: this is v1 and I'm aware that there are a few checkpatch and doc
issues which I will fix later.

[0]: https://lore.kernel.org/rust-for-linux/20230406215615.122099-1-daniel.almeida@collabora.com/
[1]: https://lore.kernel.org/rust-for-linux/20240227215146.46487-1-daniel.almeida@collabora.com/

---
Daniel Almeida (7):
      rust: media: add the media module
      rust: v4l2: add support for v4l2_device
      rust: v4l2: add support for video device nodes
      rust: v4l2: add support for v4l2 file handles
      rust: v4l2: add device capabilities
      rust: v4l2: add basic ioctl support
      rust: samples: add the v4l2 sample driver

 MAINTAINERS                      |   8 ++
 rust/bindings/bindings_helper.h  |   2 +
 rust/helpers/helpers.c           |   1 +
 rust/helpers/v4l2-device.c       |  30 +++++
 rust/kernel/lib.rs               |   2 +
 rust/kernel/media/mod.rs         |   9 ++
 rust/kernel/media/v4l2/caps.rs   | 193 ++++++++++++++++++++++++++++
 rust/kernel/media/v4l2/device.rs | 176 +++++++++++++++++++++++++
 rust/kernel/media/v4l2/file.rs   | 164 ++++++++++++++++++++++++
 rust/kernel/media/v4l2/ioctl.rs  |  92 +++++++++++++
 rust/kernel/media/v4l2/mod.rs    |  20 +++
 rust/kernel/media/v4l2/video.rs  | 269 +++++++++++++++++++++++++++++++++++++++
 samples/rust/Kconfig             |  11 ++
 samples/rust/Makefile            |   1 +
 samples/rust/rust_driver_v4l2.rs | 145 +++++++++++++++++++++
 15 files changed, 1123 insertions(+)
---
base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585
change-id: 20250815-v4l2-f7a80388cf70

Best regards,
-- 
Daniel Almeida <daniel.almeida@collabora.com>


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 1/7] rust: media: add the media module
  2025-08-18  5:49 [PATCH 0/7] rust: add initial v4l2 support Daniel Almeida
@ 2025-08-18  5:49 ` Daniel Almeida
  2025-08-18  8:56   ` Miguel Ojeda
  2025-08-18 10:28   ` Janne Grunau
  2025-08-18  5:49 ` [PATCH 2/7] rust: v4l2: add support for v4l2_device Daniel Almeida
                   ` (6 subsequent siblings)
  7 siblings, 2 replies; 17+ messages in thread
From: Daniel Almeida @ 2025-08-18  5:49 UTC (permalink / raw)
  To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, Alexandre Courbot
  Cc: linux-kernel, rust-for-linux, kernel, linux-media, Daniel Almeida

In preparation for future commits that add support for Rust abstractions
like v4l2_device, video_device, v4l2_fh and others, add the media module
in lib.rs and a corresponding MAINTAINERS entry.

Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
 MAINTAINERS              | 7 +++++++
 rust/kernel/lib.rs       | 2 ++
 rust/kernel/media/mod.rs | 6 ++++++
 3 files changed, 15 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index fe168477caa45799dfe07de2f54de6d6a1ce0615..6fc5d57950e474d73d5d65271a0394efc5a8960b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15434,6 +15434,13 @@ F:	include/uapi/linux/uvcvideo.h
 F:	include/uapi/linux/v4l2-*
 F:	include/uapi/linux/videodev2.h
 
+MEDIA RUST INFRASTRUCTURE
+M:	Daniel Almeida <daniel.almeida@collabora.com>
+L:	linux-media@vger.kernel.org
+L:	rust-for-linux@vger.kernel.org
+S:	Supported
+F:	rust/media
+
 MEDIATEK BLUETOOTH DRIVER
 M:	Sean Wang <sean.wang@mediatek.com>
 L:	linux-bluetooth@vger.kernel.org
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index ed53169e795c0badf548025a57f946fa18bc73e3..34b9e1497b2a7f70c957bff31855aeac6039cf2b 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -96,6 +96,8 @@
 #[cfg(CONFIG_KUNIT)]
 pub mod kunit;
 pub mod list;
+#[cfg(CONFIG_MEDIA_SUPPORT)]
+pub mod media;
 pub mod miscdevice;
 pub mod mm;
 #[cfg(CONFIG_NET)]
diff --git a/rust/kernel/media/mod.rs b/rust/kernel/media/mod.rs
new file mode 100644
index 0000000000000000000000000000000000000000..e4a28be7b484888a02965d0e8b5fd5d3c969840a
--- /dev/null
+++ b/rust/kernel/media/mod.rs
@@ -0,0 +1,6 @@
+// SPDX-License-Identifier: GPL-2.0
+// SPDX-copyrightText: Copyright (C) 2025 Collabora Ltd.
+
+//! Media infrastructure support.
+//!
+//! Reference: <https://www.kernel.org/doc/html/latest/driver-api/media/index.html>
\ No newline at end of file

-- 
2.50.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 2/7] rust: v4l2: add support for v4l2_device
  2025-08-18  5:49 [PATCH 0/7] rust: add initial v4l2 support Daniel Almeida
  2025-08-18  5:49 ` [PATCH 1/7] rust: media: add the media module Daniel Almeida
@ 2025-08-18  5:49 ` Daniel Almeida
  2025-08-18  9:14   ` Danilo Krummrich
  2025-08-18  5:49 ` [PATCH 3/7] rust: v4l2: add support for video device nodes Daniel Almeida
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 17+ messages in thread
From: Daniel Almeida @ 2025-08-18  5:49 UTC (permalink / raw)
  To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, Alexandre Courbot
  Cc: linux-kernel, rust-for-linux, kernel, linux-media, Daniel Almeida

struct v4l2_device is the entry-point for video4linux2 drivers. This
struct contains the device state and is the root for v4l2 subdevices
(i.e.: struct v4l2_subdev), which play an important part on how modern
video devices are modeled.

For now, add the bare-minimum support to allocate a v4l2::Device an
register it with the v4l2 framework. Subsequent patches will add support
for video devices and more.

This is one of the steps needed to get a sample v4l2 driver to probe.

Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
 rust/bindings/bindings_helper.h  |   1 +
 rust/helpers/helpers.c           |   1 +
 rust/helpers/v4l2-device.c       |   8 ++
 rust/kernel/media/mod.rs         |   5 +-
 rust/kernel/media/v4l2/device.rs | 177 +++++++++++++++++++++++++++++++++++++++
 rust/kernel/media/v4l2/mod.rs    |   9 ++
 6 files changed, 200 insertions(+), 1 deletion(-)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 84d60635e8a9baef1f1a1b2752dc0fa044f8542f..95651c4bc9e561d9f4949111961f41e65d8c1585 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -75,6 +75,7 @@
 #include <linux/wait.h>
 #include <linux/workqueue.h>
 #include <linux/xarray.h>
+#include <media/v4l2-device.h>
 #include <trace/events/rust_sample.h>
 
 #if defined(CONFIG_DRM_PANIC_SCREEN_QR_CODE)
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 7cf7fe95e41dd51717050648d6160bebebdf4b26..83d7e76294207a804f2ad95097a1e4da53fe66f1 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -47,6 +47,7 @@
 #include "task.c"
 #include "time.c"
 #include "uaccess.c"
+#include "v4l2-device.c"
 #include "vmalloc.c"
 #include "wait.c"
 #include "workqueue.c"
diff --git a/rust/helpers/v4l2-device.c b/rust/helpers/v4l2-device.c
new file mode 100644
index 0000000000000000000000000000000000000000..d19b46e8283ce762b4259e3df5ecf8bb18e863e9
--- /dev/null
+++ b/rust/helpers/v4l2-device.c
@@ -0,0 +1,8 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <media/v4l2-device.h>
+
+void rust_helper_v4l2_device_get(struct v4l2_device *v4l2_dev)
+{
+    v4l2_device_get(v4l2_dev);
+}
diff --git a/rust/kernel/media/mod.rs b/rust/kernel/media/mod.rs
index e4a28be7b484888a02965d0e8b5fd5d3c969840a..476ea673867121fb68fd4695c2cddc5380e86421 100644
--- a/rust/kernel/media/mod.rs
+++ b/rust/kernel/media/mod.rs
@@ -3,4 +3,7 @@
 
 //! Media infrastructure support.
 //!
-//! Reference: <https://www.kernel.org/doc/html/latest/driver-api/media/index.html>
\ No newline at end of file
+//! Reference: <https://www.kernel.org/doc/html/latest/driver-api/media/index.html>
+
+#[cfg(CONFIG_VIDEO_DEV = "y")]
+pub mod v4l2;
diff --git a/rust/kernel/media/v4l2/device.rs b/rust/kernel/media/v4l2/device.rs
new file mode 100644
index 0000000000000000000000000000000000000000..26096672e6f6d35711ff9bdabf4d7b20f697a4ab
--- /dev/null
+++ b/rust/kernel/media/v4l2/device.rs
@@ -0,0 +1,177 @@
+// SPDX-License-Identifier: GPL-2.0
+// SPDX-copyrightText: Copyright (C) 2025 Collabora Ltd.
+
+//! Video for Linux 2 (V4L2) device support.
+//!
+//! A v4l2 [`Device] is the entry-point for video4linux2 drivers. It acts as a
+//! logical device that may back multiple video nodes.
+//!
+//! This struct contains the device state and is the root for v4l2 subdevices
+//! (i.e.: struct v4l2_subdev), which play an important part on how modern video
+//! devices are modelled.
+//!
+//! C headers: [`include/media/v4l2-dev.h`](srctree/include/media/v4l2-dev.h).
+
+use core::{mem::MaybeUninit, ptr::NonNull};
+
+use crate::{
+    alloc::{self, KBox},
+    device,
+    error::to_result,
+    init::InPlaceInit,
+    prelude::*,
+    types::{ARef, AlwaysRefCounted, Opaque},
+};
+
+/// A logical V4L2 device handle.
+///
+/// # Invariants
+///
+/// - `inner` points to a valid `v4l2_device` that has been registered.
+#[pin_data]
+#[repr(C)]
+pub struct Device<T: Driver> {
+    #[pin]
+    inner: Opaque<bindings::v4l2_device>,
+    #[pin]
+    data: T::Data,
+}
+
+impl<T: Driver> Device<T> {
+    /// Converts a raw pointer to a `Device<T>` reference.
+    ///
+    /// # Safety
+    ///
+    /// - `ptr` must be a valid pointer to a `struct v4l2_device` that must
+    ///   remain valid for the lifetime 'a.
+    #[expect(dead_code)]
+    pub(super) unsafe fn from_raw<'a>(ptr: *mut bindings::v4l2_device) -> &'a Device<T> {
+        // SAFETY: `ptr` is a valid pointer to a `struct v4l2_device` as per the
+        // safety requirements of this function.
+        unsafe { &*(ptr.cast::<Device<T>>()) }
+    }
+
+    /// Returns the raw pointer to the `struct v4l2_device`.
+    pub(crate) fn as_raw(&self) -> *mut bindings::v4l2_device {
+        self.inner.get()
+    }
+
+    /// # Safety
+    ///
+    /// This function must be called as the release callback of `struct v4l2_device`.
+    unsafe extern "C" fn release_callback(dev: *mut bindings::v4l2_device) {
+        // SAFETY: `dev` was set by calling `KBox::into_raw` on a
+        // `Pin<KBox<Device<T>>` in `Registration::new`. Now that the refcount
+        // reached zero, we are reassembling the KBox so it can be dropped.
+        let v4l2_dev: Pin<KBox<Device<T>>> =
+            unsafe { Pin::new_unchecked(Box::from_raw(dev.cast())) };
+
+        drop(v4l2_dev)
+    }
+}
+
+impl<T: Driver> AsRef<device::Device> for Device<T> {
+    fn as_ref(&self) -> &device::Device {
+        // SAFETY: the invariants of `Device` state that the device is
+        // registered, and the `dev` field is set at registration time, so the
+        // returned reference is valid for the lifetime of self.
+        unsafe { device::Device::from_raw((*self.as_raw()).dev) }
+    }
+}
+
+// SAFETY: V4L2 devices are always reference counted and the get/put functions
+// satisfy the requirements.
+unsafe impl<T: Driver> AlwaysRefCounted for Device<T> {
+    fn inc_ref(&self) {
+        // SAFETY: Safe as per the invariants of `Device`.
+        unsafe { bindings::v4l2_device_get(self.as_raw()) }
+    }
+
+    unsafe fn dec_ref(obj: core::ptr::NonNull<Self>) {
+        // SAFETY: Safe as per the invariants of `Device`.
+        unsafe { bindings::v4l2_device_put(obj.cast().as_ptr()) };
+    }
+}
+
+/// SAFETY: It is safe to send `Device<T>` to another thread. In particular,
+/// `Device<T>` instances can be dropped from any thread.
+unsafe impl<T: Driver> Send for Device<T> {}
+
+// SAFETY: it is safe to share references to `Device<T>` between threads as it
+// is not possible to mutate the underlying `struct v4l2_device` through a
+// shared reference.
+unsafe impl<T: Driver> Sync for Device<T> {}
+
+/// The interface that must be implemented by structs that would otherwise embed
+/// a C [`struct v4l2_device`](srctree/include/media/v4l2-device.h).
+pub trait Driver {
+    /// The type of the data associated with the device.
+    type Data: Sync + Send;
+}
+
+/// Represents the registration of a [`Device`].
+///
+/// # Invariants
+///
+/// - The underlying device was registered via [`bindings::v4l2_device_register`].
+pub struct Registration<T: Driver>(ARef<Device<T>>);
+
+impl<T: Driver> Registration<T> {
+    /// Creates and registers a [`Device`] given a [`kernel::device::Device`] reference and
+    /// the associated data.
+    pub fn new(
+        dev: &device::Device,
+        data: impl PinInit<T::Data, Error>,
+        flags: alloc::Flags,
+    ) -> Result<Self> {
+        let v4l2_dev = try_pin_init!(Device {
+            inner <- Opaque::try_ffi_init(move |slot: *mut bindings::v4l2_device| {
+                let v4l2_dev = bindings::v4l2_device {
+                    release: Some(Device::<T>::release_callback),
+                    // SAFETY: All zeros is valid for this C type.
+                    ..unsafe { MaybeUninit::zeroed().assume_init() }
+                };
+
+                // SAFETY: The initializer can write to the slot.
+                unsafe { slot.write(v4l2_dev) };
+
+                // SAFETY: It is OK to call this function on a zeroed
+                // v4l2_device and a valid `device::Device` reference.
+                to_result(unsafe { bindings::v4l2_device_register(dev.as_raw(), slot) })
+            }),
+            data <- data,
+        });
+
+        let v4l2_dev = KBox::pin_init(v4l2_dev, flags)?;
+
+        // SAFETY: We will be passing the ownership of the increment 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(v4l2_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 [`Device`].
+    pub fn device(&self) -> &Device<T> {
+        &self.0
+    }
+}
+
+impl<T: Driver> Drop for Registration<T> {
+    fn drop(&mut self) {
+        // SAFETY: Safe as per the invariants of [`Registration`].
+        unsafe { bindings::v4l2_device_unregister(self.0.as_raw()) }
+    }
+}
diff --git a/rust/kernel/media/v4l2/mod.rs b/rust/kernel/media/v4l2/mod.rs
new file mode 100644
index 0000000000000000000000000000000000000000..63394d0322fa1f646f3b23a5fadf2ac34a9f666e
--- /dev/null
+++ b/rust/kernel/media/v4l2/mod.rs
@@ -0,0 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! V4L2 support.
+//!
+//! See the [structure of the V4L2 framework] for more information.
+//!
+//! [structure of the V4L2 framework]: https://www.kernel.org/doc/html/latest/driver-api/media/v4l2-intro.html#structure-of-the-v4l2-framework
+/// Support for Video for Linux 2 (V4L2) devices.
+pub mod device;

-- 
2.50.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 3/7] rust: v4l2: add support for video device nodes
  2025-08-18  5:49 [PATCH 0/7] rust: add initial v4l2 support Daniel Almeida
  2025-08-18  5:49 ` [PATCH 1/7] rust: media: add the media module Daniel Almeida
  2025-08-18  5:49 ` [PATCH 2/7] rust: v4l2: add support for v4l2_device Daniel Almeida
@ 2025-08-18  5:49 ` Daniel Almeida
  2025-08-18  9:26   ` Danilo Krummrich
  2025-08-18  5:49 ` [PATCH 4/7] rust: v4l2: add support for v4l2 file handles Daniel Almeida
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 17+ messages in thread
From: Daniel Almeida @ 2025-08-18  5:49 UTC (permalink / raw)
  To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, Alexandre Courbot
  Cc: linux-kernel, rust-for-linux, kernel, linux-media, Daniel Almeida

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@collabora.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);
+}
+
+void rust_helper_video_put(struct video_device *vdev)
+{
+    put_device(&vdev->dev);
+}
+
+int rust_helper_video_register_device(struct video_device *vdev,
+                                      enum vfl_devnode_type type, int nr)
+{
+    return video_register_device(vdev, type, nr);
+}
diff --git a/rust/kernel/media/v4l2/device.rs b/rust/kernel/media/v4l2/device.rs
index 26096672e6f6d35711ff9bdabf4d7b20f697a4ab..cbbf07ab63b7725cafecb89eb93c497e749287e7 100644
--- a/rust/kernel/media/v4l2/device.rs
+++ b/rust/kernel/media/v4l2/device.rs
@@ -44,7 +44,6 @@ impl<T: Driver> Device<T> {
     ///
     /// - `ptr` must be a valid pointer to a `struct v4l2_device` that must
     ///   remain valid for the lifetime 'a.
-    #[expect(dead_code)]
     pub(super) unsafe fn from_raw<'a>(ptr: *mut bindings::v4l2_device) -> &'a Device<T> {
         // SAFETY: `ptr` is a valid pointer to a `struct v4l2_device` as per the
         // safety requirements of this function.
diff --git a/rust/kernel/media/v4l2/mod.rs b/rust/kernel/media/v4l2/mod.rs
index 63394d0322fa1f646f3b23a5fadf2ac34a9f666e..ba1d4b7da8d8887b1604031497c300d7e0609cd2 100644
--- a/rust/kernel/media/v4l2/mod.rs
+++ b/rust/kernel/media/v4l2/mod.rs
@@ -7,3 +7,6 @@
 //! [structure of the V4L2 framework]: https://www.kernel.org/doc/html/latest/driver-api/media/v4l2-intro.html#structure-of-the-v4l2-framework
 /// Support for Video for Linux 2 (V4L2) devices.
 pub mod device;
+
+/// Support for Video for Linux 2 (V4L2) video devices.
+pub mod video;
diff --git a/rust/kernel/media/v4l2/video.rs b/rust/kernel/media/v4l2/video.rs
new file mode 100644
index 0000000000000000000000000000000000000000..e6954d3e6ac65201bd40a0215babb354ae10cd12
--- /dev/null
+++ b/rust/kernel/media/v4l2/video.rs
@@ -0,0 +1,251 @@
+// SPDX-License-Identifier: GPL-2.0
+// SPDX-copyrightText: Copyright (C) 2025 Collabora Ltd.
+
+//! Video for Linux 2 (V4L2) video device support.
+//!
+//! Video device nodes back the actual /dev/videoX files and provide an
+//! interface for userspace to interact with the device, usually via the `ioctl`
+//! syscall.
+//!
+//! They expose a rich ioctl interface and allow for modelling complex hardware
+//! through a topology of nodes, each modelling a particular hardware function
+//! or component.
+//!
+//! C headers: [`include/media/v4l2-dev.h`](srctree/include/media/v4l2-dev.h).
+
+use core::{marker::PhantomData, mem::MaybeUninit, ops::Deref, ptr::NonNull};
+
+use pin_init::PinInit;
+
+use crate::{
+    alloc,
+    error::to_result,
+    media::v4l2::{self, video},
+    prelude::*,
+    types::{ARef, AlwaysRefCounted, Opaque},
+};
+
+/// The type of node that will be exposed to userspace.
+#[repr(u32)]
+pub enum NodeType {
+    /// For video input/output devices.
+    Video = bindings::vfl_devnode_type_VFL_TYPE_VIDEO,
+}
+
+/// Identifies if the video device corresponds to a receiver, a transmitter or a
+/// mem-to-mem device.
+#[repr(u32)]
+pub enum Direction {
+    /// The device is a receiver.
+    Rx,
+    /// The device is a transmitter.
+    Tx,
+    // TODO: m2m. We do not support this at the moment, so it is not possible to
+    // specify it here as well.
+}
+
+/// A V4L2 device node.
+///
+/// V4L2 devices nodes are backed by a [`v4l2::Device`]. Each instance is
+/// associated with a specific video device in the filesystem. A logical device
+/// can be represented by multiple instances of this struct.
+///
+/// # Invariants
+///
+/// - `inner` is a valid pointer to a `struct video_device`.
+#[pin_data]
+#[repr(C)]
+pub struct Device<T: Driver> {
+    #[pin]
+    inner: Opaque<bindings::video_device>,
+    #[pin]
+    data: <T as Driver>::Data,
+}
+
+impl<T: Driver> Device<T> {
+    pub(super) fn as_raw(&self) -> *mut bindings::video_device {
+        self.inner.get()
+    }
+
+    /// Converts a raw pointer to a `Device<T>` reference.
+    ///
+    /// # Safety
+    ///
+    /// - `ptr` must be a valid pointer to a `struct video_device` that must
+    ///   remain valid for the lifetime 'a.
+    #[expect(dead_code)]
+    pub(super) unsafe fn from_raw<'a>(ptr: *mut bindings::video_device) -> &'a Device<T> {
+        // SAFETY: `ptr` is a valid pointer to a `struct video_device` as per the
+        // safety requirements of this function.
+        unsafe { &*(ptr.cast::<Device<T>>()) }
+    }
+
+    /// Returns the video device number, i.e.: /dev/videoX.
+    pub fn num(&self) -> u16 {
+        // SAFETY: Safe as per the invariants of `Device`.
+        unsafe { (*self.as_raw()).num }
+    }
+
+    /// # Safety
+    ///
+    /// This function must be called as the release callback of `struct
+    /// video_device`.
+    unsafe extern "C" fn release_callback(dev: *mut bindings::video_device) {
+        // SAFETY: `dev` was set by calling `KBox::into_raw` on a
+        // `Pin<KBox<Device<T>>` in `Registration::new`. Now that the refcount
+        // reached zero, we are reassembling the KBox so it can be dropped.
+        let v4l2_dev: Pin<KBox<Device<T>>> =
+            unsafe { Pin::new_unchecked(Box::from_raw(dev.cast())) };
+
+        drop(v4l2_dev)
+    }
+}
+
+impl<T: Driver> AsRef<v4l2::device::Device<T>> for Device<T> {
+    fn as_ref(&self) -> &v4l2::device::Device<T> {
+        // SAFETY: `self.as_raw()` is a valid pointer to a `struct video_device`
+        // as per the invariants of `Device<T>`.
+        unsafe { v4l2::device::Device::from_raw((*self.as_raw()).v4l2_dev) }
+    }
+}
+
+impl<T: Driver> Deref for Device<T> {
+    type Target = <T as Driver>::Data;
+
+    fn deref(&self) -> &Self::Target {
+        &self.data
+    }
+}
+
+/// SAFETY: V4L2 device nodes are always reference counted and the get/put
+/// functions satisfy the requirements.
+unsafe impl<T: Driver> AlwaysRefCounted for Device<T> {
+    fn inc_ref(&self) {
+        // SAFETY: it is safe to call `bindings::video_get` because
+        // `self.inner` is a valid pointer to a `struct video_device` as per
+        // the invariants of `Device<T>`.
+        unsafe { bindings::video_get(self.inner.get()) }
+    }
+
+    unsafe fn dec_ref(obj: NonNull<Self>) {
+        // SAFETY: The safety requirements guarantee that the refcount is
+        // non-zero.
+        unsafe { bindings::video_put(obj.cast().as_ptr()) };
+    }
+}
+
+// SAFETY: it is safe to send a [`Device`] to another thread. In particular, a
+// [`Device`] can be dropped by any thread.
+unsafe impl<T: Driver> Send for Device<T> {}
+
+// SAFETY: It is safe to send a &Device<T> to another thread, as we do not allow
+// mutation through a shared reference.
+unsafe impl<T: Driver> Sync for Device<T> {}
+
+/// The interface that must be implemented by structs that would otherwise embed
+/// a C [`struct video_device`](srctree/include/media/v4l2-dev.h).
+pub trait Driver: v4l2::device::Driver {
+    /// The type of the driver's private data.
+    type Data;
+
+    /// The [`NodeType`] to use when registering the device node.
+    const NODE_TYPE: NodeType;
+
+    /// The [`Direction`] to use when registering the device node.
+    const DIRECTION: Direction;
+
+    /// The name to use when registering the device node.
+    const NAME: &'static CStr;
+}
+
+struct DeviceOptions<'a, T: Driver> {
+    dev: &'a v4l2::device::Device<T>,
+    _phantom: PhantomData<T>,
+}
+
+impl<'a, T: Driver> DeviceOptions<'a, T> {
+    /// Creates a `video_device` ready for registration.
+    fn into_raw(self) -> bindings::video_device {
+        bindings::video_device {
+            v4l2_dev: self.dev.as_raw(),
+            name: {
+                let mut name: [c_char; 64] = [0; 64];
+                let src = T::NAME.as_bytes();
+                let len = core::cmp::min(src.len(), name.len());
+                name[..len].copy_from_slice(&src[..len]);
+                name
+            },
+            vfl_dir: T::DIRECTION as c_uint,
+            release: Some(Device::<T>::release_callback),
+            // SAFETY: All zeros is valid for the rest of the fields in this C
+            // type.
+            ..unsafe { MaybeUninit::zeroed().assume_init() }
+        }
+    }
+}
+
+/// 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> {
+        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


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 4/7] rust: v4l2: add support for v4l2 file handles
  2025-08-18  5:49 [PATCH 0/7] rust: add initial v4l2 support Daniel Almeida
                   ` (2 preceding siblings ...)
  2025-08-18  5:49 ` [PATCH 3/7] rust: v4l2: add support for video device nodes Daniel Almeida
@ 2025-08-18  5:49 ` Daniel Almeida
  2025-08-18  5:49 ` [PATCH 5/7] rust: v4l2: add device capabilities Daniel Almeida
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 17+ messages in thread
From: Daniel Almeida @ 2025-08-18  5:49 UTC (permalink / raw)
  To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, Alexandre Courbot
  Cc: linux-kernel, rust-for-linux, kernel, linux-media, Daniel Almeida

V4L2 allow multiple opens for a variety of use-cases, where the actual
meaning of opening a node more than once is defined by the driver.

Each open call is associated with its own v4l2_fh instance, and these
instances are collected at the struct video_device level, making up a
private context that can be retrieved at later calls, like ioctl()s
performed on the node's file descriptor.

Add basic support for v4l2_fh and a corresponding DriverFile trait to
help model it. This will be needed in future patches in order to add
v4l2 ioctl support, among other things.

Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/v4l2-device.c      |   6 ++
 rust/kernel/media/v4l2/file.rs  | 165 ++++++++++++++++++++++++++++++++++++++++
 rust/kernel/media/v4l2/mod.rs   |   3 +
 rust/kernel/media/v4l2/video.rs |   7 +-
 5 files changed, 180 insertions(+), 2 deletions(-)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 95651c4bc9e561d9f4949111961f41e65d8c1585..11ff2c018515e99c8af9ad91ab09f8f15ae224c1 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -76,6 +76,7 @@
 #include <linux/workqueue.h>
 #include <linux/xarray.h>
 #include <media/v4l2-device.h>
+#include <media/v4l2-ioctl.h>
 #include <trace/events/rust_sample.h>
 
 #if defined(CONFIG_DRM_PANIC_SCREEN_QR_CODE)
diff --git a/rust/helpers/v4l2-device.c b/rust/helpers/v4l2-device.c
index 0ead52b9a1ccc0fbc4d7df63578b334b17c05b70..24fa41fe570d1263313e70325c4b39f819cb4177 100644
--- a/rust/helpers/v4l2-device.c
+++ b/rust/helpers/v4l2-device.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 
+#include <media/v4l2-dev.h>
 #include <media/v4l2-device.h>
 
 void rust_helper_v4l2_device_get(struct v4l2_device *v4l2_dev)
@@ -22,3 +23,8 @@ int rust_helper_video_register_device(struct video_device *vdev,
 {
     return video_register_device(vdev, type, nr);
 }
+
+struct video_device *rust_helper_video_devdata(struct file *filp)
+{
+    return video_devdata(filp);
+}
diff --git a/rust/kernel/media/v4l2/file.rs b/rust/kernel/media/v4l2/file.rs
new file mode 100644
index 0000000000000000000000000000000000000000..37b34f8e6f251fafde5f7e6b4bd654519d8247a5
--- /dev/null
+++ b/rust/kernel/media/v4l2/file.rs
@@ -0,0 +1,165 @@
+// SPDX-License-Identifier: GPL-2.0
+// SPDX-copyrightText: Copyright (C) 2025 Collabora Ltd.
+
+//! V4L2 file handle support.
+//!
+//! V4L2 allow multiple opens for a variety of use-cases, where the actual
+//! meaning of opening a node more than once is defined by the driver.
+//!
+//! Each open call is associated with its own v4l2_fh instance, and these
+//! instances are collected at the struct video_device level, making up a
+//! private context that can be retrieved at later calls, like ioctl()s
+//! performed on the node's file descriptor.
+
+use core::{marker::PhantomData, mem::MaybeUninit, ops::Deref};
+
+use pin_init::PinInit;
+
+use crate::{alloc::KBox, init::InPlaceInit, media::v4l2::video, prelude::*, types::Opaque};
+
+/// Trait that must be implemented by V4L2 drivers to represent a V4L2 file.
+pub trait DriverFile {
+    /// The parent `Driver` implementation for this `DriverFile`.
+    type Driver: video::Driver;
+
+    /// A reference to the module that this driver belongs to.
+    const MODULE: &'static ThisModule;
+
+    /// Called when a client opens a V4L2 node.
+    fn open(vdev: &video::Device<Self::Driver>) -> impl PinInit<Self, Error>;
+}
+
+/// An open V4L2 file handle.
+///
+/// # Invariants
+///
+/// `inner` is a valid instance of a `struct v4l2_fh`.
+#[repr(C)]
+#[pin_data]
+pub struct File<T: DriverFile> {
+    #[pin]
+    inner: Opaque<bindings::v4l2_fh>,
+    #[pin]
+    driver_file: T,
+}
+
+impl<T: DriverFile> File<T> {
+    /// Converts a raw pointer to a `File<T>` reference.
+    ///
+    /// # Safety
+    ///
+    /// - `ptr` must be a valid pointer to a `struct v4l2_file`.
+    /// - `ptr` must be valid for 'a.
+    #[expect(dead_code)]
+    pub(super) unsafe fn from_raw<'a>(ptr: *mut bindings::v4l2_fh) -> &'a File<T> {
+        // SAFETY: `ptr` is a valid pointer to a `struct v4l2_file` as per the
+        // safety requirements of this function.
+        unsafe { &*(ptr.cast::<File<T>>()) }
+    }
+
+    /// Returns the raw pointer to the `struct v4l2_fh`.
+    pub(super) fn as_raw(&self) -> *mut bindings::v4l2_fh {
+        self.inner.get()
+    }
+
+    /// The open callback of a `struct video_device`
+    ///
+    /// # Safety
+    ///
+    /// - this function must be called as the open callback of a `struct video_device`.
+    /// - `raw_file` must be a valid pointer to a `struct file`.
+    pub(super) unsafe extern "C" fn open_callback(raw_file: *mut bindings::file) -> c_int {
+        // SAFETY: `raw_file` is a valid pointer to a `struct file` and this is
+        // the open callback for a `struct video device` as per the safety
+        // requirements. This means that the `struct video_device` can be
+        // retrieved by this FFI call.
+        let vdev = unsafe { bindings::video_devdata(raw_file) };
+
+        let file = try_pin_init!(File::<T> {
+            inner <- Opaque::ffi_init(move |slot: *mut bindings::v4l2_fh| {
+                // SAFETY:
+                // - it is safe for the initializer to write to the slot,
+                // - zeroed() is a valid value for `struct v4l2_fh`.
+                unsafe { *slot = MaybeUninit::zeroed().assume_init() };
+
+                // SAFETY: `slot` was zero-initialized and `vdev` is a valid
+                // video device that was retrieved by `video_devdata`.
+                unsafe { bindings::v4l2_fh_init(slot, vdev) }
+            }),
+            driver_file <- {
+                // SAFETY: `vdev` is a valid pointer to a `struct video_device` that was
+                // retrieved using `video_devdata`. The underlying `struct video_device`
+                // remains valid for the lifetime 'a, i.e.: for the entire scope of this
+                // function.
+                let vdev = unsafe { video::Device::from_raw(vdev) };
+                T::open(vdev)
+            }
+        });
+
+        let file = match KBox::pin_init(file, GFP_KERNEL) {
+            Ok(file) => file,
+            Err(e) => return e.to_errno(),
+        };
+
+        let v4l2_fh = file.as_ref().as_raw();
+
+        // SAFETY: We are passing ownership of the `File<T>` to the kernel,
+        // which treats the underlying memory as pinned throughout its lifetime.
+        let ptr = KBox::into_raw(unsafe { Pin::into_inner_unchecked(file) });
+
+        // SAFETY: `raw_file` points to a valid `struct file` as per the safety
+        // requirements, so it is safe to dereference it.
+        unsafe { (*raw_file).private_data = ptr.cast() };
+
+        // SAFETY: `v4l2_fh` was initialized above in `v4l2_fh_init`.
+        unsafe { bindings::v4l2_fh_add(v4l2_fh) };
+
+        0
+    }
+
+    /// The release callback of a `struct v4l2_fh`.
+    ///
+    /// # Safety
+    ///
+    /// - this function must be called as the release callback of a `struct v4l2_fh`.
+    /// - `raw_file` must be a valid pointer to a `struct file`.
+    pub(super) unsafe extern "C" fn release_callback(raw_file: *mut bindings::file) -> c_int {
+        // SAFETY: `raw_file::private_data` was set to `Pin<KBox<File<T>` in
+        // `open_callback`.
+        let file: Pin<KBox<File<T>>> =
+            unsafe { Pin::new_unchecked(Box::from_raw((*raw_file).private_data.cast())) };
+
+        // SAFETY: `file.inner` is a valid pointer to a `struct v4l2_fh`.
+        unsafe { bindings::v4l2_fh_del(file.as_raw()) };
+        // SAFETY: `file.inner` is a valid pointer to a `struct v4l2_fh`.
+        unsafe { bindings::v4l2_fh_exit(file.as_raw()) };
+
+        0
+    }
+}
+
+impl<T: DriverFile> Deref for File<T> {
+    type Target = T;
+
+    fn deref(&self) -> &Self::Target {
+        &self.driver_file
+    }
+}
+
+pub(super) struct FileVtable<T: DriverFile>(PhantomData<T>);
+
+impl<T: DriverFile> FileVtable<T> {
+    const VTABLE: bindings::v4l2_file_operations = bindings::v4l2_file_operations {
+        open: Some(File::<T>::open_callback),
+        release: Some(File::<T>::release_callback),
+        owner: T::MODULE.as_ptr(),
+        unlocked_ioctl: Some(bindings::video_ioctl2),
+        // SAFETY: All zeros is valid for the rest of the fields in this C
+        // type.
+        ..unsafe { MaybeUninit::zeroed().assume_init() }
+    };
+
+    pub(super) const fn build() -> &'static bindings::v4l2_file_operations {
+        &Self::VTABLE
+    }
+}
diff --git a/rust/kernel/media/v4l2/mod.rs b/rust/kernel/media/v4l2/mod.rs
index ba1d4b7da8d8887b1604031497c300d7e0609cd2..1195c18f1336891c4b9b194d4e7e5cd40989ace9 100644
--- a/rust/kernel/media/v4l2/mod.rs
+++ b/rust/kernel/media/v4l2/mod.rs
@@ -10,3 +10,6 @@
 
 /// Support for Video for Linux 2 (V4L2) video devices.
 pub mod video;
+
+/// Support for Video for Linux 2 (V4L2) file handles.
+pub mod file;
diff --git a/rust/kernel/media/v4l2/video.rs b/rust/kernel/media/v4l2/video.rs
index e6954d3e6ac65201bd40a0215babb354ae10cd12..7ef2111c32ca55a2bced8325cd883b28204dc3ee 100644
--- a/rust/kernel/media/v4l2/video.rs
+++ b/rust/kernel/media/v4l2/video.rs
@@ -20,7 +20,7 @@
 use crate::{
     alloc,
     error::to_result,
-    media::v4l2::{self, video},
+    media::v4l2::{self, file::DriverFile, video},
     prelude::*,
     types::{ARef, AlwaysRefCounted, Opaque},
 };
@@ -73,7 +73,6 @@ pub(super) fn as_raw(&self) -> *mut bindings::video_device {
     ///
     /// - `ptr` must be a valid pointer to a `struct video_device` that must
     ///   remain valid for the lifetime 'a.
-    #[expect(dead_code)]
     pub(super) unsafe fn from_raw<'a>(ptr: *mut bindings::video_device) -> &'a Device<T> {
         // SAFETY: `ptr` is a valid pointer to a `struct video_device` as per the
         // safety requirements of this function.
@@ -148,6 +147,9 @@ pub trait Driver: v4l2::device::Driver {
     /// The type of the driver's private data.
     type Data;
 
+    /// The driver's file type.
+    type File: DriverFile;
+
     /// The [`NodeType`] to use when registering the device node.
     const NODE_TYPE: NodeType;
 
@@ -177,6 +179,7 @@ fn into_raw(self) -> bindings::video_device {
             },
             vfl_dir: T::DIRECTION as c_uint,
             release: Some(Device::<T>::release_callback),
+            fops: super::file::FileVtable::<T::File>::build(),
             // SAFETY: All zeros is valid for the rest of the fields in this C
             // type.
             ..unsafe { MaybeUninit::zeroed().assume_init() }

-- 
2.50.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 5/7] rust: v4l2: add device capabilities
  2025-08-18  5:49 [PATCH 0/7] rust: add initial v4l2 support Daniel Almeida
                   ` (3 preceding siblings ...)
  2025-08-18  5:49 ` [PATCH 4/7] rust: v4l2: add support for v4l2 file handles Daniel Almeida
@ 2025-08-18  5:49 ` Daniel Almeida
  2025-08-20  4:14   ` Elle Rhumsaa
  2025-08-18  5:49 ` [PATCH 6/7] rust: v4l2: add basic ioctl support Daniel Almeida
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 17+ messages in thread
From: Daniel Almeida @ 2025-08-18  5:49 UTC (permalink / raw)
  To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, Alexandre Courbot
  Cc: linux-kernel, rust-for-linux, kernel, linux-media, Daniel Almeida

All v4l2 devices must expose a given set of capabilities to the v4l2
core and to userspace. Add support for that in v4l2::caps. This will be
used by the next patch in order to add support for VIDIOC_QUERYCAP.

Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
 rust/kernel/media/v4l2/caps.rs  | 193 ++++++++++++++++++++++++++++++++++++++++
 rust/kernel/media/v4l2/mod.rs   |   2 +
 rust/kernel/media/v4l2/video.rs |   6 +-
 3 files changed, 200 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/media/v4l2/caps.rs b/rust/kernel/media/v4l2/caps.rs
new file mode 100644
index 0000000000000000000000000000000000000000..4b0164c58d13e83e728091228fae025dbce59bc8
--- /dev/null
+++ b/rust/kernel/media/v4l2/caps.rs
@@ -0,0 +1,193 @@
+// SPDX-License-Identifier: GPL-2.0
+// SPDX-copyrightText: Copyright (C) 2025 Collabora Ltd.
+
+use crate::{prelude::*, str::CStr, types::Opaque};
+use core::cmp::min;
+
+/// A wrapper over `struct v4l2_capability`.
+///
+/// # Invariants
+///
+/// - `self.0` is a valid instance of `struct v4l2_capability`.
+/// - All strings in `struct v4l2_capability` are valid C strings.
+///
+/// TODO: This type would benefit from an #[derive(accessor)] macro to automate
+/// the boilerplate below.
+#[repr(transparent)]
+pub struct Capabilities(Opaque<bindings::v4l2_capability>);
+
+impl Capabilities {
+    /// Returns the raw pointer to the `struct v4l2_capability`.
+    pub fn as_raw(&self) -> *const bindings::v4l2_capability {
+        self.0.get()
+    }
+
+    /// Converts a raw pointer to a `Capabilities` reference.
+    ///
+    /// # Safety
+    ///
+    /// - `ptr` must be a valid pointer to a `struct v4l2_capability` that must
+    ///   remain valid for the lifetime 'a.
+    /// - the returned reference must obey Rust's reference rules.
+    pub unsafe fn from_raw<'a>(ptr: *mut bindings::v4l2_capability) -> &'a mut Self {
+        // SAFETY: `ptr` is a valid pointer to a `struct v4l2_capability` as per the
+        // safety requirements of this function.
+        unsafe { &mut *(ptr.cast::<Self>()) }
+    }
+
+    fn inner(&self) -> &bindings::v4l2_capability {
+        // SAFETY: safe as per the invariants of `Capabilities`
+        unsafe { &*self.0.get() }
+    }
+
+    fn inner_mut(&mut self) -> &mut bindings::v4l2_capability {
+        // SAFETY: safe as per the invariants of `Capabilities`
+        unsafe { &mut *self.0.get() }
+    }
+
+    /// Returns the `driver` field.
+    pub fn driver(&self) -> &CStr {
+        // SAFETY: safe as per the invariants of `Capabilities`
+        unsafe { CStr::from_bytes_with_nul_unchecked(&self.inner().driver) }
+    }
+
+    /// Sets the `driver` field.
+    pub fn set_driver(&mut self, name: &CStr) -> Result {
+        if name.len_with_nul() > self.inner().driver.len() {
+            return Err(EINVAL);
+        }
+
+        let cap = self.inner_mut();
+        let src = name.to_bytes_with_nul();
+        let n = min(src.len(), cap.driver.len());
+        cap.driver[..n].copy_from_slice(&src[..n]);
+
+        Ok(())
+    }
+
+    /// Returns the `card` field.
+    pub fn card(&self) -> &CStr {
+        // SAFETY: safe as per the invariants of `Capabilities`
+        unsafe { CStr::from_bytes_with_nul_unchecked(&self.inner().card) }
+    }
+
+    /// Sets the `card` field.
+    pub fn set_card(&mut self, card: &CStr) -> Result {
+        if card.len_with_nul() > self.inner().card.len() {
+            return Err(EINVAL);
+        }
+
+        let cap = self.inner_mut();
+        let src = card.to_bytes_with_nul();
+        let n = min(src.len(), cap.card.len());
+        cap.card[..n].copy_from_slice(&src[..n]);
+
+        Ok(())
+    }
+
+    /// Returns the `bus_info` field.
+    pub fn bus_info(&self) -> &CStr {
+        // SAFETY: safe as per the invariants of `Capabilities`
+        unsafe { CStr::from_bytes_with_nul_unchecked(&self.inner().bus_info) }
+    }
+
+    /// Sets the `bus_info` field.
+    pub fn set_bus_info(&mut self, info: &CStr) -> Result {
+        if info.len_with_nul() > self.inner().bus_info.len() {
+            return Err(EINVAL);
+        }
+
+        let cap = self.inner_mut();
+        let src = info.to_bytes_with_nul();
+        let n = min(src.len(), cap.bus_info.len());
+        cap.bus_info[..n].copy_from_slice(&src[..n]);
+
+        Ok(())
+    }
+
+    /// Returns the `version` field.
+    pub fn version(&self) -> u32 {
+        self.inner().version
+    }
+
+    /// Sets the `version` field.
+    pub fn set_version(&mut self, v: u32) {
+        self.inner_mut().version = v;
+    }
+
+    /// Returns the `capabilities` field.
+    pub fn capabilities(&self) -> u32 {
+        self.inner().capabilities
+    }
+
+    /// Sets the `capabilities` field.
+    pub fn set_capabilities(&mut self, caps: u32) {
+        self.inner_mut().capabilities = caps;
+    }
+
+    /// Returns the `device_caps` field.
+    pub fn device_caps(&self) -> Option<DeviceCaps> {
+        if self.inner().device_caps == 0 {
+            None
+        } else {
+            Some(DeviceCaps(self.inner().device_caps))
+        }
+    }
+
+    /// Sets the `device_caps` field.
+    pub fn set_device_caps(&mut self, caps: DeviceCaps) {
+        self.inner_mut().device_caps = caps.as_raw();
+    }
+}
+
+/// Device capabilities.
+///
+/// They can be combined with the operators `|`, `&`, and `!`.
+///
+/// Values can be used from the [`device_caps`] module.
+#[derive(Clone, Copy, PartialEq)]
+pub struct DeviceCaps(u32);
+
+impl DeviceCaps {
+    /// Get the raw representation of the device capabilties.
+    pub(crate) fn as_raw(self) -> u32 {
+        self.0
+    }
+
+    /// Check whether `cap` is contained in `self`.
+    pub fn contains(self, cap: DeviceCaps) -> bool {
+        (self & cap) == cap
+    }
+}
+
+impl core::ops::BitOr for DeviceCaps {
+    type Output = Self;
+    fn bitor(self, rhs: Self) -> Self::Output {
+        Self(self.0 | rhs.0)
+    }
+}
+
+impl core::ops::BitAnd for DeviceCaps {
+    type Output = Self;
+    fn bitand(self, rhs: Self) -> Self::Output {
+        Self(self.0 & rhs.0)
+    }
+}
+
+impl core::ops::Not for DeviceCaps {
+    type Output = Self;
+    fn not(self) -> Self::Output {
+        Self(!self.0)
+    }
+}
+
+/// Device capabilities.
+pub mod device_caps {
+    use super::DeviceCaps;
+
+    /// The device is a video capture device.
+    pub const VIDEO_CAPTURE: DeviceCaps = DeviceCaps(bindings::V4L2_CAP_VIDEO_CAPTURE);
+
+    /// The device is a video output device.
+    pub const VIDEO_OUTPUT: DeviceCaps = DeviceCaps(bindings::V4L2_CAP_VIDEO_OUTPUT);
+}
diff --git a/rust/kernel/media/v4l2/mod.rs b/rust/kernel/media/v4l2/mod.rs
index 1195c18f1336891c4b9b194d4e7e5cd40989ace9..1d8241f8a2230954371965bb91b20e726f144dce 100644
--- a/rust/kernel/media/v4l2/mod.rs
+++ b/rust/kernel/media/v4l2/mod.rs
@@ -11,5 +11,7 @@
 /// Support for Video for Linux 2 (V4L2) video devices.
 pub mod video;
 
+/// Support for Video for Linux 2 device capabilities.
+pub mod caps;
 /// Support for Video for Linux 2 (V4L2) file handles.
 pub mod file;
diff --git a/rust/kernel/media/v4l2/video.rs b/rust/kernel/media/v4l2/video.rs
index 7ef2111c32ca55a2bced8325cd883b28204dc3ee..c0ac99a8234d2f7a8effd4701b9f7440236540c8 100644
--- a/rust/kernel/media/v4l2/video.rs
+++ b/rust/kernel/media/v4l2/video.rs
@@ -20,7 +20,7 @@
 use crate::{
     alloc,
     error::to_result,
-    media::v4l2::{self, file::DriverFile, video},
+    media::v4l2::{self, caps::DeviceCaps, file::DriverFile, video},
     prelude::*,
     types::{ARef, AlwaysRefCounted, Opaque},
 };
@@ -158,6 +158,9 @@ pub trait Driver: v4l2::device::Driver {
 
     /// The name to use when registering the device node.
     const NAME: &'static CStr;
+
+    /// The capabilities offered by this device node.
+    const CAPS: DeviceCaps;
 }
 
 struct DeviceOptions<'a, T: Driver> {
@@ -180,6 +183,7 @@ fn into_raw(self) -> bindings::video_device {
             vfl_dir: T::DIRECTION as c_uint,
             release: Some(Device::<T>::release_callback),
             fops: super::file::FileVtable::<T::File>::build(),
+            device_caps: T::CAPS.as_raw(),
             // SAFETY: All zeros is valid for the rest of the fields in this C
             // type.
             ..unsafe { MaybeUninit::zeroed().assume_init() }

-- 
2.50.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 6/7] rust: v4l2: add basic ioctl support
  2025-08-18  5:49 [PATCH 0/7] rust: add initial v4l2 support Daniel Almeida
                   ` (4 preceding siblings ...)
  2025-08-18  5:49 ` [PATCH 5/7] rust: v4l2: add device capabilities Daniel Almeida
@ 2025-08-18  5:49 ` Daniel Almeida
  2025-08-20  4:22   ` Elle Rhumsaa
  2025-08-18  5:49 ` [PATCH 7/7] rust: samples: add the v4l2 sample driver Daniel Almeida
  2025-08-18  8:45 ` [PATCH 0/7] rust: add initial v4l2 support Miguel Ojeda
  7 siblings, 1 reply; 17+ messages in thread
From: Daniel Almeida @ 2025-08-18  5:49 UTC (permalink / raw)
  To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, Alexandre Courbot
  Cc: linux-kernel, rust-for-linux, kernel, linux-media, Daniel Almeida

Most of the v4l2 API is implemented through ioctl(), so adding support
for them is essential in order to be able to write v4l2 drivers.

Hook up ioctl support by filling up an instance of v4l2_ioctl_ops. For
now, we only support the most basic v4l2 ioctl: VIDIOC_QUERYCAPS. This
is used by userspace to retrieve information about the device, and is
considered enough to implement a simple v4l2 sample Rust driver.

Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
 rust/kernel/media/v4l2/file.rs  |  1 -
 rust/kernel/media/v4l2/ioctl.rs | 92 +++++++++++++++++++++++++++++++++++++++++
 rust/kernel/media/v4l2/mod.rs   |  3 ++
 rust/kernel/media/v4l2/video.rs | 13 +++++-
 4 files changed, 107 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/media/v4l2/file.rs b/rust/kernel/media/v4l2/file.rs
index 37b34f8e6f251fafde5f7e6b4bd654519d8247a5..8817051268323866f41fd56a0c7e8fa4b7537858 100644
--- a/rust/kernel/media/v4l2/file.rs
+++ b/rust/kernel/media/v4l2/file.rs
@@ -50,7 +50,6 @@ impl<T: DriverFile> File<T> {
     ///
     /// - `ptr` must be a valid pointer to a `struct v4l2_file`.
     /// - `ptr` must be valid for 'a.
-    #[expect(dead_code)]
     pub(super) unsafe fn from_raw<'a>(ptr: *mut bindings::v4l2_fh) -> &'a File<T> {
         // SAFETY: `ptr` is a valid pointer to a `struct v4l2_file` as per the
         // safety requirements of this function.
diff --git a/rust/kernel/media/v4l2/ioctl.rs b/rust/kernel/media/v4l2/ioctl.rs
new file mode 100644
index 0000000000000000000000000000000000000000..e8d20d4cb70f5722c0109ea5bad36041355fc7a1
--- /dev/null
+++ b/rust/kernel/media/v4l2/ioctl.rs
@@ -0,0 +1,92 @@
+// SPDX-License-Identifier: GPL-2.0
+// SPDX-copyrightText: Copyright (C) 2025 Collabora Ltd.
+
+//! V4L2 device node ioctl support.
+//!
+//! Most of the V4L2 API is implemented through the ioctl system call. This
+//! module provides support for ioctl operations on V4L2 device nodes.
+
+use core::{marker::PhantomData, mem::MaybeUninit};
+
+use crate::{
+    media::v4l2::{
+        self,
+        video::{self, Driver},
+    },
+    prelude::*,
+};
+
+/// The vtable for the ioctls of a registered Rust [`video::Device`].
+///
+/// # Invariants
+///
+/// - All the callbacks in [`IoctlVtable`] are called after the underlying
+///   [`video::Device`] has been registered.
+pub(super) struct IoctlVtable<T: Driver>(PhantomData<T>);
+
+impl<T: Driver> IoctlVtable<T> {
+    /// # Safety
+    ///
+    /// This should only be called from the ioctl callbacks and the returned
+    /// reference should not outlive the callback itself.
+    unsafe fn data<'a>(file: *mut bindings::file) -> &'a <T as Driver>::Data
+    where
+        T: 'a,
+    {
+        // SAFETY: This was set during the video device registration process.
+        let vdev = unsafe { bindings::video_devdata(file) };
+
+        // SAFETY: `video_device` is a valid pointer to a `struct video_device`
+        // returned by `bindings::video_devdata` and it is valid while the
+        // reference is alive.
+        unsafe { video::Device::<T>::from_raw(vdev) }
+    }
+
+    /// # Safety
+    ///
+    /// This should only be used as the `videoc_querycap` callback.
+    unsafe extern "C" fn vidioc_querycap_callback(
+        file: *mut bindings::file,
+        _fh: *mut c_void,
+        cap: *mut bindings::v4l2_capability,
+    ) -> core::ffi::c_int {
+        // SAFETY: this is being called from an ioctl callback and the returned
+        // reference does not outlive it.
+        let data = unsafe { Self::data(file) };
+
+        // SAFETY: the fact that this is being called from an ioctl callback means that:
+        //
+        // - the video device has been registered.
+        // - `open()` has been called (as you cannot call ioctl() on a file that
+        // has not been previously opened).
+        // - as a result from the statement above, a valid `v4l2_fh` was
+        // installed in `bindings::file::private_data`, which we then convert
+        // into `File<T>` here.
+        // - `ptr` is valid for 'a, i.e.: for the scope of this function.
+        let file = unsafe { v4l2::file::File::<T::File>::from_raw((*file).private_data.cast()) };
+
+        // SAFETY: the safety requirements ensure that `cap` is a valid pointer
+        // to a `struct v4l2_capability`, which fulfills the requirements of the
+        // `Capabilities::from_raw`.
+        let cap = unsafe { v4l2::caps::Capabilities::from_raw(cap) };
+
+        match T::querycap(file, data, cap) {
+            Ok(()) => 0,
+            Err(err) => err.to_errno(),
+        }
+    }
+
+    const VTABLE: bindings::v4l2_ioctl_ops = bindings::v4l2_ioctl_ops {
+        vidioc_querycap: if T::HAS_QUERYCAP {
+            Some(Self::vidioc_querycap_callback)
+        } else {
+            None
+        },
+        // SAFETY: All zeros is a valid value for `bindings::v4l2_ioctl_ops`.
+        ..unsafe { MaybeUninit::zeroed().assume_init() }
+    };
+
+    pub(super) const fn build() -> &'static bindings::v4l2_ioctl_ops {
+        &Self::VTABLE
+    }
+}
diff --git a/rust/kernel/media/v4l2/mod.rs b/rust/kernel/media/v4l2/mod.rs
index 1d8241f8a2230954371965bb91b20e726f144dce..4caa9fdc1ff3ed2bd6145cc653b49a84873ecac2 100644
--- a/rust/kernel/media/v4l2/mod.rs
+++ b/rust/kernel/media/v4l2/mod.rs
@@ -15,3 +15,6 @@
 pub mod caps;
 /// Support for Video for Linux 2 (V4L2) file handles.
 pub mod file;
+
+/// Support for Video for Linux 2 (V4L2) ioctls.
+pub mod ioctl;
diff --git a/rust/kernel/media/v4l2/video.rs b/rust/kernel/media/v4l2/video.rs
index c0ac99a8234d2f7a8effd4701b9f7440236540c8..2390a93a39925dbff3f809bc65adfac1d881309c 100644
--- a/rust/kernel/media/v4l2/video.rs
+++ b/rust/kernel/media/v4l2/video.rs
@@ -19,7 +19,7 @@
 
 use crate::{
     alloc,
-    error::to_result,
+    error::{to_result, VTABLE_DEFAULT_ERROR},
     media::v4l2::{self, caps::DeviceCaps, file::DriverFile, video},
     prelude::*,
     types::{ARef, AlwaysRefCounted, Opaque},
@@ -143,6 +143,7 @@ unsafe impl<T: Driver> Sync for Device<T> {}
 
 /// The interface that must be implemented by structs that would otherwise embed
 /// a C [`struct video_device`](srctree/include/media/v4l2-dev.h).
+#[vtable]
 pub trait Driver: v4l2::device::Driver {
     /// The type of the driver's private data.
     type Data;
@@ -161,6 +162,15 @@ pub trait Driver: v4l2::device::Driver {
 
     /// The capabilities offered by this device node.
     const CAPS: DeviceCaps;
+
+    /// Driver implementation for the `querycap` ioctl.
+    fn querycap(
+        _file: &v4l2::file::File<<Self as Driver>::File>,
+        _data: &<Self as Driver>::Data,
+        _cap: &mut v4l2::caps::Capabilities,
+    ) -> Result {
+        build_error!(VTABLE_DEFAULT_ERROR)
+    }
 }
 
 struct DeviceOptions<'a, T: Driver> {
@@ -184,6 +194,7 @@ fn into_raw(self) -> bindings::video_device {
             release: Some(Device::<T>::release_callback),
             fops: super::file::FileVtable::<T::File>::build(),
             device_caps: T::CAPS.as_raw(),
+            ioctl_ops: super::ioctl::IoctlVtable::<T>::build(),
             // SAFETY: All zeros is valid for the rest of the fields in this C
             // type.
             ..unsafe { MaybeUninit::zeroed().assume_init() }

-- 
2.50.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 7/7] rust: samples: add the v4l2 sample driver
  2025-08-18  5:49 [PATCH 0/7] rust: add initial v4l2 support Daniel Almeida
                   ` (5 preceding siblings ...)
  2025-08-18  5:49 ` [PATCH 6/7] rust: v4l2: add basic ioctl support Daniel Almeida
@ 2025-08-18  5:49 ` Daniel Almeida
  2025-08-20  4:24   ` Elle Rhumsaa
  2025-08-20 12:39   ` Danilo Krummrich
  2025-08-18  8:45 ` [PATCH 0/7] rust: add initial v4l2 support Miguel Ojeda
  7 siblings, 2 replies; 17+ messages in thread
From: Daniel Almeida @ 2025-08-18  5:49 UTC (permalink / raw)
  To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, Alexandre Courbot
  Cc: linux-kernel, rust-for-linux, kernel, linux-media, Daniel Almeida

This driver is the only user of the v4l2 abstractions at the moment. It
serves as a means to validate the abstractions by proving that the
device is actually registered as /dev/videoX, and it can be opened and
queried by v4l2-ctl, while also serving as a display of the current v4l2
support in Rust, as well as a blueprint for more elaborate Rust v4l2
drivers in the future.

Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
 MAINTAINERS                      |   1 +
 samples/rust/Kconfig             |  11 +++
 samples/rust/Makefile            |   1 +
 samples/rust/rust_driver_v4l2.rs | 145 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 158 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 6fc5d57950e474d73d5d65271a0394efc5a8960b..14521bc0585503992da582f2cee361666985e39f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15440,6 +15440,7 @@ L:	linux-media@vger.kernel.org
 L:	rust-for-linux@vger.kernel.org
 S:	Supported
 F:	rust/media
+F:	sample/rust/rust_driver_v4l2.rs
 
 MEDIATEK BLUETOOTH DRIVER
 M:	Sean Wang <sean.wang@mediatek.com>
diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
index 7f7371a004ee0a8f67dca99c836596709a70c4fa..64422acf1e9da9d05f904e14fd423b3b4aef173a 100644
--- a/samples/rust/Kconfig
+++ b/samples/rust/Kconfig
@@ -105,6 +105,17 @@ config SAMPLE_RUST_DRIVER_AUXILIARY
 
 	  If unsure, say N.
 
+config SAMPLE_RUST_DRIVER_V4L2
+	tristate "Video4Linux2 driver"
+	depends on MEDIA_SUPPORT && VIDEO_DEV
+	help
+	  This option builds the Rust V4L2 driver sample.
+
+	  To compile this as a module, choose M here:
+	  the module will be called rust_driver_v4l2.
+
+	  If unsure, say N.
+
 config SAMPLE_RUST_HOSTPROGS
 	bool "Host programs"
 	help
diff --git a/samples/rust/Makefile b/samples/rust/Makefile
index bd2faad63b4f3befe7d1ed5139fe25c7a8b6d7f6..57e21f0373938bb70b4cb400ea550010895b4c94 100644
--- a/samples/rust/Makefile
+++ b/samples/rust/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_SAMPLE_RUST_DRIVER_PCI)		+= rust_driver_pci.o
 obj-$(CONFIG_SAMPLE_RUST_DRIVER_PLATFORM)	+= rust_driver_platform.o
 obj-$(CONFIG_SAMPLE_RUST_DRIVER_FAUX)		+= rust_driver_faux.o
 obj-$(CONFIG_SAMPLE_RUST_DRIVER_AUXILIARY)	+= rust_driver_auxiliary.o
+obj-$(CONFIG_SAMPLE_RUST_DRIVER_V4L2)		+= rust_driver_v4l2.o
 obj-$(CONFIG_SAMPLE_RUST_CONFIGFS)		+= rust_configfs.o
 
 rust_print-y := rust_print_main.o rust_print_events.o
diff --git a/samples/rust/rust_driver_v4l2.rs b/samples/rust/rust_driver_v4l2.rs
new file mode 100644
index 0000000000000000000000000000000000000000..a3ef98a613f2fed9e8589f0761ce7e43029c02b6
--- /dev/null
+++ b/samples/rust/rust_driver_v4l2.rs
@@ -0,0 +1,145 @@
+// SPDX-License-Identifier: GPL-2.0
+// SPDX-copyrightText: Copyright (C) 2025 Collabora Ltd.
+
+//! Rust V4L2 sample driver.
+//!
+//! This sample demonstrates how to:
+//! - Register a V4L2 device (struct v4l2_device),
+//! - Register a video node (struct video_device) using the Rust V4L2
+//!   abstractions,
+//! - Implement support for a V4L2 ioctl in a driver.
+//!
+//! It implements only `VIDIOC_QUERYCAP` and minimal open/close handling.
+
+use kernel::{
+    c_str,
+    device::Core,
+    media::v4l2::{
+        self,
+        caps::{self, DeviceCaps},
+        video,
+    },
+    of, platform,
+    prelude::*,
+    types::ARef,
+    ThisModule,
+};
+
+/// The private data associated with the V4L2 device.
+#[pin_data]
+struct Data {}
+
+/// The private data associated with a V4L2 device node, i.e. `struct
+/// video_device`.
+#[pin_data]
+struct VideoData {}
+
+/// The private data associated with a V4L2 file, i.e. `struct v4l2_fh`.
+#[pin_data]
+struct File {}
+
+impl v4l2::file::DriverFile for File {
+    type Driver = SampleDriver;
+
+    const MODULE: &'static ThisModule = &THIS_MODULE;
+
+    fn open(_vdev: &v4l2::video::Device<Self::Driver>) -> impl PinInit<Self, Error> {
+        try_pin_init!(Self {})
+    }
+}
+
+struct SampleDriver {
+    _pdev: ARef<platform::Device>,
+    _v4l2_reg: v4l2::device::Registration<Self>,
+    video_reg: video::Registration<Self>,
+}
+
+impl v4l2::device::Driver for SampleDriver {
+    type Data = Data;
+}
+
+#[vtable]
+impl video::Driver for SampleDriver {
+    type Data = VideoData;
+    type File = File;
+
+    const NODE_TYPE: video::NodeType = video::NodeType::Video;
+    const DIRECTION: video::Direction = video::Direction::Rx;
+    const NAME: &'static CStr = c_str!("rv4l2");
+    const CAPS: DeviceCaps = caps::device_caps::VIDEO_CAPTURE;
+
+    fn querycap(
+        _file: &v4l2::file::File<Self::File>,
+        _data: &<Self as video::Driver>::Data,
+        cap: &mut caps::Capabilities,
+    ) -> Result {
+        cap.set_driver(c_str!("rv4l2"))?;
+        cap.set_card(c_str!("rv4l2"))?;
+        cap.set_bus_info(c_str!("platform:rv4l2"))?;
+
+        cap.set_device_caps(Self::CAPS);
+        Ok(())
+    }
+}
+
+kernel::of_device_table!(
+    OF_TABLE,
+    MODULE_OF_TABLE,
+    <SampleDriver as platform::Driver>::IdInfo,
+    [(of::DeviceId::new(c_str!("test, rust-v4l2")), ())]
+);
+
+impl platform::Driver for SampleDriver {
+    type IdInfo = ();
+    const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
+
+    fn probe(
+        pdev: &platform::Device<Core>,
+        _info: Option<&Self::IdInfo>,
+    ) -> Result<Pin<KBox<Self>>> {
+        let dev = pdev.as_ref();
+
+        let v4l2_reg =
+            v4l2::device::Registration::<Self>::new(dev, try_pin_init!(Data {}), GFP_KERNEL)?;
+
+        let video_reg = video::Registration::<Self>::new(
+            v4l2_reg.device(),
+            try_pin_init!(VideoData {}),
+            GFP_KERNEL,
+        )?;
+
+        let this = KBox::new(
+            Self {
+                _pdev: pdev.into(),
+                _v4l2_reg: v4l2_reg,
+                video_reg,
+            },
+            GFP_KERNEL,
+        )?;
+
+        dev_info!(
+            dev,
+            "Registered /dev/video{}\n",
+            this.video_reg.device().num()
+        );
+        Ok(this.into())
+    }
+
+    fn unbind(pdev: &platform::Device<Core>, _this: Pin<&Self>) {
+        dev_info!(pdev.as_ref(), "Unbinding Rust V4L2 sample driver\n");
+    }
+}
+
+impl Drop for SampleDriver {
+    fn drop(&mut self) {
+        dev_dbg!(self._pdev.as_ref(), "Rust V4L2 sample driver removed\n");
+    }
+}
+
+kernel::module_platform_driver! {
+    type: SampleDriver,
+    name: "rust_driver_v4l2",
+    authors: ["Daniel Almeida"],
+    description: "Rust V4L2 sample video driver",
+    license: "GPL v2",
+}

-- 
2.50.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/7] rust: add initial v4l2 support
  2025-08-18  5:49 [PATCH 0/7] rust: add initial v4l2 support Daniel Almeida
                   ` (6 preceding siblings ...)
  2025-08-18  5:49 ` [PATCH 7/7] rust: samples: add the v4l2 sample driver Daniel Almeida
@ 2025-08-18  8:45 ` Miguel Ojeda
  7 siblings, 0 replies; 17+ messages in thread
From: Miguel Ojeda @ 2025-08-18  8:45 UTC (permalink / raw)
  To: Daniel Almeida
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, Alexandre Courbot, linux-kernel,
	rust-for-linux, kernel, linux-media

On Mon, Aug 18, 2025 at 7:51 AM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> I've decided to work on this once more after being told by a few people
> that they would likely try to play with Rust v4l2 drivers if only they
> did not have to write all of the infrastructure themselves. This work
> (and subsequent patches) will pave the way for them.

Glad to hear there is interest and to see you reviving this effort!

(Mentioning who is interested, if it is public, would be nice)

I see the media list Cc'd, but this should likely be sent to the
maintainers too.

Thanks!

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/7] rust: media: add the media module
  2025-08-18  5:49 ` [PATCH 1/7] rust: media: add the media module Daniel Almeida
@ 2025-08-18  8:56   ` Miguel Ojeda
  2025-08-18 10:28   ` Janne Grunau
  1 sibling, 0 replies; 17+ messages in thread
From: Miguel Ojeda @ 2025-08-18  8:56 UTC (permalink / raw)
  To: Daniel Almeida
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, Alexandre Courbot, linux-kernel,
	rust-for-linux, kernel, linux-media

On Mon, Aug 18, 2025 at 7:51 AM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
>  MAINTAINERS              | 7 +++++++

I would suggest splitting this into its own patch, "MAINTAINERS: "
prefix, so that it is more visible. The other bits can be moved to the
first patch that adds the file -- it is what we usually do.

> diff --git a/MAINTAINERS b/MAINTAINERS
> index fe168477caa45799dfe07de2f54de6d6a1ce0615..6fc5d57950e474d73d5d65271a0394efc5a8960b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15434,6 +15434,13 @@ F:     include/uapi/linux/uvcvideo.h
>  F:     include/uapi/linux/v4l2-*
>  F:     include/uapi/linux/videodev2.h
>
> +MEDIA RUST INFRASTRUCTURE

It may be good to match the parent entry/subsystem, e.g. with "
[RUST]" at the end or whatever is the convention for the subsystem (if
there is no convention, then " [RUST]" is what we have been normally
doing).

See e.g. "PCI SUBSYSTEM [RUST]".

> +S:     Supported

Very nice to see Collabora wants to support this!

> +F:     rust/media

rust/media/

i.e. a trailing slash here is mandatory to include all files and subdirectories.

> +// SPDX-copyrightText: Copyright (C) 2025 Collabora Ltd.

SPDX-FileCopyrightText?

> +//! Reference: <https://www.kernel.org/doc/html/latest/driver-api/media/index.html>

docs.kernel.org are nicer links :)

    https://docs.kernel.org/driver-api/media/index.html

> \ No newline at end of file

Normally there would be one.

Thanks!

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/7] rust: v4l2: add support for v4l2_device
  2025-08-18  5:49 ` [PATCH 2/7] rust: v4l2: add support for v4l2_device Daniel Almeida
@ 2025-08-18  9:14   ` Danilo Krummrich
  0 siblings, 0 replies; 17+ messages in thread
From: Danilo Krummrich @ 2025-08-18  9:14 UTC (permalink / raw)
  To: Daniel Almeida
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Alexandre Courbot, linux-kernel, rust-for-linux,
	kernel, linux-media

On Mon Aug 18, 2025 at 7:49 AM CEST, Daniel Almeida wrote:
> +/// A logical V4L2 device handle.
> +///
> +/// # Invariants
> +///
> +/// - `inner` points to a valid `v4l2_device` that has been registered.
> +#[pin_data]
> +#[repr(C)]
> +pub struct Device<T: Driver> {
> +    #[pin]
> +    inner: Opaque<bindings::v4l2_device>,
> +    #[pin]
> +    data: T::Data,
> +}
> +
> +impl<T: Driver> Device<T> {
> +    /// Converts a raw pointer to a `Device<T>` reference.
> +    ///
> +    /// # Safety
> +    ///
> +    /// - `ptr` must be a valid pointer to a `struct v4l2_device` that must
> +    ///   remain valid for the lifetime 'a.

"valid pointer to a `struct v4l2_device`" is not sufficient for casting it to
Device<T>.

> +    #[expect(dead_code)]
> +    pub(super) unsafe fn from_raw<'a>(ptr: *mut bindings::v4l2_device) -> &'a Device<T> {
> +        // SAFETY: `ptr` is a valid pointer to a `struct v4l2_device` as per the
> +        // safety requirements of this function.
> +        unsafe { &*(ptr.cast::<Device<T>>()) }
> +    }

<snip>

> +/// Represents the registration of a [`Device`].
> +///
> +/// # Invariants
> +///
> +/// - The underlying device was registered via [`bindings::v4l2_device_register`].
> +pub struct Registration<T: Driver>(ARef<Device<T>>);
> +
> +impl<T: Driver> Registration<T> {
> +    /// Creates and registers a [`Device`] given a [`kernel::device::Device`] reference and
> +    /// the associated data.
> +    pub fn new(
> +        dev: &device::Device,
> +        data: impl PinInit<T::Data, Error>,
> +        flags: alloc::Flags,
> +    ) -> Result<Self> {

Why does Registration::new() create the Device<T> instance?

I think Device<T> should have its own constructor. It is confusing that the
Device<T> is silently created by the Registration and to get a reference one has
to call `reg.device().into()` afterwards.

> +        let v4l2_dev = try_pin_init!(Device {
> +            inner <- Opaque::try_ffi_init(move |slot: *mut bindings::v4l2_device| {
> +                let v4l2_dev = bindings::v4l2_device {
> +                    release: Some(Device::<T>::release_callback),
> +                    // SAFETY: All zeros is valid for this C type.
> +                    ..unsafe { MaybeUninit::zeroed().assume_init() }
> +                };
> +
> +                // SAFETY: The initializer can write to the slot.
> +                unsafe { slot.write(v4l2_dev) };
> +
> +                // SAFETY: It is OK to call this function on a zeroed
> +                // v4l2_device and a valid `device::Device` reference.
> +                to_result(unsafe { bindings::v4l2_device_register(dev.as_raw(), slot) })
> +            }),
> +            data <- data,
> +        });
> +
> +        let v4l2_dev = KBox::pin_init(v4l2_dev, flags)?;
> +
> +        // SAFETY: We will be passing the ownership of the increment 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(v4l2_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 [`Device`].
> +    pub fn device(&self) -> &Device<T> {
> +        &self.0
> +    }
> +}
> +
> +impl<T: Driver> Drop for Registration<T> {
> +    fn drop(&mut self) {
> +        // SAFETY: Safe as per the invariants of [`Registration`].
> +        unsafe { bindings::v4l2_device_unregister(self.0.as_raw()) }
> +    }
> +}

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/7] rust: v4l2: add support for video device nodes
  2025-08-18  5:49 ` [PATCH 3/7] rust: v4l2: add support for video device nodes Daniel Almeida
@ 2025-08-18  9:26   ` Danilo Krummrich
  0 siblings, 0 replies; 17+ messages in thread
From: Danilo Krummrich @ 2025-08-18  9:26 UTC (permalink / raw)
  To: Daniel Almeida
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Alexandre Courbot, linux-kernel, rust-for-linux,
	kernel, linux-media

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@collabora.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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/7] rust: media: add the media module
  2025-08-18  5:49 ` [PATCH 1/7] rust: media: add the media module Daniel Almeida
  2025-08-18  8:56   ` Miguel Ojeda
@ 2025-08-18 10:28   ` Janne Grunau
  1 sibling, 0 replies; 17+ messages in thread
From: Janne Grunau @ 2025-08-18 10:28 UTC (permalink / raw)
  To: Daniel Almeida
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, Alexandre Courbot, linux-kernel,
	rust-for-linux, kernel, linux-media

On Mon, Aug 18, 2025 at 02:49:47AM -0300, Daniel Almeida wrote:
> In preparation for future commits that add support for Rust abstractions
> like v4l2_device, video_device, v4l2_fh and others, add the media module
> in lib.rs and a corresponding MAINTAINERS entry.
> 
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> ---
>  MAINTAINERS              | 7 +++++++
>  rust/kernel/lib.rs       | 2 ++
>  rust/kernel/media/mod.rs | 6 ++++++
>  3 files changed, 15 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index fe168477caa45799dfe07de2f54de6d6a1ce0615..6fc5d57950e474d73d5d65271a0394efc5a8960b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15434,6 +15434,13 @@ F:	include/uapi/linux/uvcvideo.h
>  F:	include/uapi/linux/v4l2-*
>  F:	include/uapi/linux/videodev2.h
>  
> +MEDIA RUST INFRASTRUCTURE
> +M:	Daniel Almeida <daniel.almeida@collabora.com>
> +L:	linux-media@vger.kernel.org
> +L:	rust-for-linux@vger.kernel.org
> +S:	Supported
> +F:	rust/media

You probably mean rust/kernel/media/

Janne

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 5/7] rust: v4l2: add device capabilities
  2025-08-18  5:49 ` [PATCH 5/7] rust: v4l2: add device capabilities Daniel Almeida
@ 2025-08-20  4:14   ` Elle Rhumsaa
  0 siblings, 0 replies; 17+ messages in thread
From: Elle Rhumsaa @ 2025-08-20  4:14 UTC (permalink / raw)
  To: Daniel Almeida
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, Alexandre Courbot, linux-kernel,
	rust-for-linux, kernel, linux-media

On Mon, Aug 18, 2025 at 02:49:51AM -0300, Daniel Almeida wrote:
> All v4l2 devices must expose a given set of capabilities to the v4l2
> core and to userspace. Add support for that in v4l2::caps. This will be
> used by the next patch in order to add support for VIDIOC_QUERYCAP.
> 
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> ---
>  rust/kernel/media/v4l2/caps.rs  | 193 ++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/media/v4l2/mod.rs   |   2 +
>  rust/kernel/media/v4l2/video.rs |   6 +-
>  3 files changed, 200 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/kernel/media/v4l2/caps.rs b/rust/kernel/media/v4l2/caps.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..4b0164c58d13e83e728091228fae025dbce59bc8
> --- /dev/null
> +++ b/rust/kernel/media/v4l2/caps.rs
> @@ -0,0 +1,193 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// SPDX-copyrightText: Copyright (C) 2025 Collabora Ltd.
> +
> +use crate::{prelude::*, str::CStr, types::Opaque};
> +use core::cmp::min;
> +
> +/// A wrapper over `struct v4l2_capability`.
> +///
> +/// # Invariants
> +///
> +/// - `self.0` is a valid instance of `struct v4l2_capability`.
> +/// - All strings in `struct v4l2_capability` are valid C strings.
> +///
> +/// TODO: This type would benefit from an #[derive(accessor)] macro to automate
> +/// the boilerplate below.
> +#[repr(transparent)]
> +pub struct Capabilities(Opaque<bindings::v4l2_capability>);
> +
> +impl Capabilities {
> +    /// Returns the raw pointer to the `struct v4l2_capability`.
> +    pub fn as_raw(&self) -> *const bindings::v4l2_capability {
> +        self.0.get()
> +    }
> +
> +    /// Converts a raw pointer to a `Capabilities` reference.
> +    ///
> +    /// # Safety
> +    ///
> +    /// - `ptr` must be a valid pointer to a `struct v4l2_capability` that must
> +    ///   remain valid for the lifetime 'a.
> +    /// - the returned reference must obey Rust's reference rules.
> +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::v4l2_capability) -> &'a mut Self {
> +        // SAFETY: `ptr` is a valid pointer to a `struct v4l2_capability` as per the
> +        // safety requirements of this function.
> +        unsafe { &mut *(ptr.cast::<Self>()) }
> +    }
> +
> +    fn inner(&self) -> &bindings::v4l2_capability {
> +        // SAFETY: safe as per the invariants of `Capabilities`
> +        unsafe { &*self.0.get() }
> +    }
> +
> +    fn inner_mut(&mut self) -> &mut bindings::v4l2_capability {
> +        // SAFETY: safe as per the invariants of `Capabilities`
> +        unsafe { &mut *self.0.get() }
> +    }
> +
> +    /// Returns the `driver` field.
> +    pub fn driver(&self) -> &CStr {
> +        // SAFETY: safe as per the invariants of `Capabilities`
> +        unsafe { CStr::from_bytes_with_nul_unchecked(&self.inner().driver) }
> +    }
> +
> +    /// Sets the `driver` field.
> +    pub fn set_driver(&mut self, name: &CStr) -> Result {
> +        if name.len_with_nul() > self.inner().driver.len() {
> +            return Err(EINVAL);
> +        }
> +
> +        let cap = self.inner_mut();
> +        let src = name.to_bytes_with_nul();
> +        let n = min(src.len(), cap.driver.len());
> +        cap.driver[..n].copy_from_slice(&src[..n]);
> +
> +        Ok(())
> +    }
> +
> +    /// Returns the `card` field.
> +    pub fn card(&self) -> &CStr {
> +        // SAFETY: safe as per the invariants of `Capabilities`
> +        unsafe { CStr::from_bytes_with_nul_unchecked(&self.inner().card) }
> +    }
> +
> +    /// Sets the `card` field.
> +    pub fn set_card(&mut self, card: &CStr) -> Result {
> +        if card.len_with_nul() > self.inner().card.len() {
> +            return Err(EINVAL);
> +        }
> +
> +        let cap = self.inner_mut();
> +        let src = card.to_bytes_with_nul();
> +        let n = min(src.len(), cap.card.len());
> +        cap.card[..n].copy_from_slice(&src[..n]);
> +
> +        Ok(())
> +    }
> +
> +    /// Returns the `bus_info` field.
> +    pub fn bus_info(&self) -> &CStr {
> +        // SAFETY: safe as per the invariants of `Capabilities`
> +        unsafe { CStr::from_bytes_with_nul_unchecked(&self.inner().bus_info) }
> +    }
> +
> +    /// Sets the `bus_info` field.
> +    pub fn set_bus_info(&mut self, info: &CStr) -> Result {
> +        if info.len_with_nul() > self.inner().bus_info.len() {
> +            return Err(EINVAL);
> +        }
> +
> +        let cap = self.inner_mut();
> +        let src = info.to_bytes_with_nul();
> +        let n = min(src.len(), cap.bus_info.len());
> +        cap.bus_info[..n].copy_from_slice(&src[..n]);
> +
> +        Ok(())
> +    }
> +
> +    /// Returns the `version` field.
> +    pub fn version(&self) -> u32 {
> +        self.inner().version
> +    }
> +
> +    /// Sets the `version` field.
> +    pub fn set_version(&mut self, v: u32) {
> +        self.inner_mut().version = v;
> +    }
> +
> +    /// Returns the `capabilities` field.
> +    pub fn capabilities(&self) -> u32 {
> +        self.inner().capabilities
> +    }
> +
> +    /// Sets the `capabilities` field.
> +    pub fn set_capabilities(&mut self, caps: u32) {
> +        self.inner_mut().capabilities = caps;
> +    }
> +
> +    /// Returns the `device_caps` field.
> +    pub fn device_caps(&self) -> Option<DeviceCaps> {
> +        if self.inner().device_caps == 0 {
> +            None
> +        } else {
> +            Some(DeviceCaps(self.inner().device_caps))
> +        }
> +    }
> +
> +    /// Sets the `device_caps` field.
> +    pub fn set_device_caps(&mut self, caps: DeviceCaps) {
> +        self.inner_mut().device_caps = caps.as_raw();
> +    }
> +}
> +
> +/// Device capabilities.
> +///
> +/// They can be combined with the operators `|`, `&`, and `!`.
> +///
> +/// Values can be used from the [`device_caps`] module.
> +#[derive(Clone, Copy, PartialEq)]
> +pub struct DeviceCaps(u32);
> +
> +impl DeviceCaps {
> +    /// Get the raw representation of the device capabilties.
> +    pub(crate) fn as_raw(self) -> u32 {
> +        self.0
> +    }
> +
> +    /// Check whether `cap` is contained in `self`.
> +    pub fn contains(self, cap: DeviceCaps) -> bool {
> +        (self & cap) == cap
> +    }
> +}
> +
> +impl core::ops::BitOr for DeviceCaps {
> +    type Output = Self;
> +    fn bitor(self, rhs: Self) -> Self::Output {
> +        Self(self.0 | rhs.0)
> +    }
> +}
> +
> +impl core::ops::BitAnd for DeviceCaps {
> +    type Output = Self;
> +    fn bitand(self, rhs: Self) -> Self::Output {
> +        Self(self.0 & rhs.0)
> +    }
> +}
> +
> +impl core::ops::Not for DeviceCaps {
> +    type Output = Self;
> +    fn not(self) -> Self::Output {
> +        Self(!self.0)
> +    }
> +}
> +
> +/// Device capabilities.
> +pub mod device_caps {
> +    use super::DeviceCaps;
> +
> +    /// The device is a video capture device.
> +    pub const VIDEO_CAPTURE: DeviceCaps = DeviceCaps(bindings::V4L2_CAP_VIDEO_CAPTURE);
> +
> +    /// The device is a video output device.
> +    pub const VIDEO_OUTPUT: DeviceCaps = DeviceCaps(bindings::V4L2_CAP_VIDEO_OUTPUT);
> +}

These can probably be implemented as associated constants:

```rust
impl DeviceCaps {
    pub const VIDEO_CAPTURE: Self = Self(bindings::V4L2_CAP_VIDEO_CAPTURE);
    pub const VIDEO_OUTPUT: Self = Self(bindings::V4L2_CAP_VIDEO_OUTPUT);
```

That would allow for a slightly more ergonomic usage:

```rust
let _ = DeviceCaps::VIDEO_CAPTURE;
```

> diff --git a/rust/kernel/media/v4l2/mod.rs b/rust/kernel/media/v4l2/mod.rs
> index 1195c18f1336891c4b9b194d4e7e5cd40989ace9..1d8241f8a2230954371965bb91b20e726f144dce 100644
> --- a/rust/kernel/media/v4l2/mod.rs
> +++ b/rust/kernel/media/v4l2/mod.rs
> @@ -11,5 +11,7 @@
>  /// Support for Video for Linux 2 (V4L2) video devices.
>  pub mod video;
>  
> +/// Support for Video for Linux 2 device capabilities.
> +pub mod caps;
>  /// Support for Video for Linux 2 (V4L2) file handles.
>  pub mod file;
> diff --git a/rust/kernel/media/v4l2/video.rs b/rust/kernel/media/v4l2/video.rs
> index 7ef2111c32ca55a2bced8325cd883b28204dc3ee..c0ac99a8234d2f7a8effd4701b9f7440236540c8 100644
> --- a/rust/kernel/media/v4l2/video.rs
> +++ b/rust/kernel/media/v4l2/video.rs
> @@ -20,7 +20,7 @@
>  use crate::{
>      alloc,
>      error::to_result,
> -    media::v4l2::{self, file::DriverFile, video},
> +    media::v4l2::{self, caps::DeviceCaps, file::DriverFile, video},
>      prelude::*,
>      types::{ARef, AlwaysRefCounted, Opaque},
>  };
> @@ -158,6 +158,9 @@ pub trait Driver: v4l2::device::Driver {
>  
>      /// The name to use when registering the device node.
>      const NAME: &'static CStr;
> +
> +    /// The capabilities offered by this device node.
> +    const CAPS: DeviceCaps;
>  }
>  
>  struct DeviceOptions<'a, T: Driver> {
> @@ -180,6 +183,7 @@ fn into_raw(self) -> bindings::video_device {
>              vfl_dir: T::DIRECTION as c_uint,
>              release: Some(Device::<T>::release_callback),
>              fops: super::file::FileVtable::<T::File>::build(),
> +            device_caps: T::CAPS.as_raw(),
>              // SAFETY: All zeros is valid for the rest of the fields in this C
>              // type.
>              ..unsafe { MaybeUninit::zeroed().assume_init() }
> 
> -- 
> 2.50.1

Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 6/7] rust: v4l2: add basic ioctl support
  2025-08-18  5:49 ` [PATCH 6/7] rust: v4l2: add basic ioctl support Daniel Almeida
@ 2025-08-20  4:22   ` Elle Rhumsaa
  0 siblings, 0 replies; 17+ messages in thread
From: Elle Rhumsaa @ 2025-08-20  4:22 UTC (permalink / raw)
  To: Daniel Almeida
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, Alexandre Courbot, linux-kernel,
	rust-for-linux, kernel, linux-media

On Mon, Aug 18, 2025 at 02:49:52AM -0300, Daniel Almeida wrote:
> Most of the v4l2 API is implemented through ioctl(), so adding support
> for them is essential in order to be able to write v4l2 drivers.
> 
> Hook up ioctl support by filling up an instance of v4l2_ioctl_ops. For
> now, we only support the most basic v4l2 ioctl: VIDIOC_QUERYCAPS. This
> is used by userspace to retrieve information about the device, and is
> considered enough to implement a simple v4l2 sample Rust driver.
> 
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> ---
>  rust/kernel/media/v4l2/file.rs  |  1 -
>  rust/kernel/media/v4l2/ioctl.rs | 92 +++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/media/v4l2/mod.rs   |  3 ++
>  rust/kernel/media/v4l2/video.rs | 13 +++++-
>  4 files changed, 107 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/kernel/media/v4l2/file.rs b/rust/kernel/media/v4l2/file.rs
> index 37b34f8e6f251fafde5f7e6b4bd654519d8247a5..8817051268323866f41fd56a0c7e8fa4b7537858 100644
> --- a/rust/kernel/media/v4l2/file.rs
> +++ b/rust/kernel/media/v4l2/file.rs
> @@ -50,7 +50,6 @@ impl<T: DriverFile> File<T> {
>      ///
>      /// - `ptr` must be a valid pointer to a `struct v4l2_file`.
>      /// - `ptr` must be valid for 'a.
> -    #[expect(dead_code)]
>      pub(super) unsafe fn from_raw<'a>(ptr: *mut bindings::v4l2_fh) -> &'a File<T> {
>          // SAFETY: `ptr` is a valid pointer to a `struct v4l2_file` as per the
>          // safety requirements of this function.
> diff --git a/rust/kernel/media/v4l2/ioctl.rs b/rust/kernel/media/v4l2/ioctl.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..e8d20d4cb70f5722c0109ea5bad36041355fc7a1
> --- /dev/null
> +++ b/rust/kernel/media/v4l2/ioctl.rs
> @@ -0,0 +1,92 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// SPDX-copyrightText: Copyright (C) 2025 Collabora Ltd.
> +
> +//! V4L2 device node ioctl support.
> +//!
> +//! Most of the V4L2 API is implemented through the ioctl system call. This
> +//! module provides support for ioctl operations on V4L2 device nodes.
> +
> +use core::{marker::PhantomData, mem::MaybeUninit};
> +
> +use crate::{
> +    media::v4l2::{
> +        self,
> +        video::{self, Driver},
> +    },
> +    prelude::*,
> +};
> +
> +/// The vtable for the ioctls of a registered Rust [`video::Device`].
> +///
> +/// # Invariants
> +///
> +/// - All the callbacks in [`IoctlVtable`] are called after the underlying
> +///   [`video::Device`] has been registered.
> +pub(super) struct IoctlVtable<T: Driver>(PhantomData<T>);
> +
> +impl<T: Driver> IoctlVtable<T> {
> +    /// # Safety
> +    ///
> +    /// This should only be called from the ioctl callbacks and the returned
> +    /// reference should not outlive the callback itself.
> +    unsafe fn data<'a>(file: *mut bindings::file) -> &'a <T as Driver>::Data
> +    where
> +        T: 'a,
> +    {
> +        // SAFETY: This was set during the video device registration process.
> +        let vdev = unsafe { bindings::video_devdata(file) };
> +
> +        // SAFETY: `video_device` is a valid pointer to a `struct video_device`
> +        // returned by `bindings::video_devdata` and it is valid while the
> +        // reference is alive.
> +        unsafe { video::Device::<T>::from_raw(vdev) }
> +    }
> +
> +    /// # Safety
> +    ///
> +    /// This should only be used as the `videoc_querycap` callback.
> +    unsafe extern "C" fn vidioc_querycap_callback(
> +        file: *mut bindings::file,
> +        _fh: *mut c_void,
> +        cap: *mut bindings::v4l2_capability,
> +    ) -> core::ffi::c_int {
> +        // SAFETY: this is being called from an ioctl callback and the returned
> +        // reference does not outlive it.
> +        let data = unsafe { Self::data(file) };
> +
> +        // SAFETY: the fact that this is being called from an ioctl callback means that:
> +        //
> +        // - the video device has been registered.
> +        // - `open()` has been called (as you cannot call ioctl() on a file that
> +        // has not been previously opened).
> +        // - as a result from the statement above, a valid `v4l2_fh` was
> +        // installed in `bindings::file::private_data`, which we then convert
> +        // into `File<T>` here.
> +        // - `ptr` is valid for 'a, i.e.: for the scope of this function.
> +        let file = unsafe { v4l2::file::File::<T::File>::from_raw((*file).private_data.cast()) };
> +
> +        // SAFETY: the safety requirements ensure that `cap` is a valid pointer
> +        // to a `struct v4l2_capability`, which fulfills the requirements of the
> +        // `Capabilities::from_raw`.
> +        let cap = unsafe { v4l2::caps::Capabilities::from_raw(cap) };
> +
> +        match T::querycap(file, data, cap) {
> +            Ok(()) => 0,
> +            Err(err) => err.to_errno(),
> +        }
> +    }
> +
> +    const VTABLE: bindings::v4l2_ioctl_ops = bindings::v4l2_ioctl_ops {
> +        vidioc_querycap: if T::HAS_QUERYCAP {
> +            Some(Self::vidioc_querycap_callback)
> +        } else {
> +            None
> +        },
> +        // SAFETY: All zeros is a valid value for `bindings::v4l2_ioctl_ops`.
> +        ..unsafe { MaybeUninit::zeroed().assume_init() }
> +    };
> +
> +    pub(super) const fn build() -> &'static bindings::v4l2_ioctl_ops {
> +        &Self::VTABLE
> +    }
> +}
> diff --git a/rust/kernel/media/v4l2/mod.rs b/rust/kernel/media/v4l2/mod.rs
> index 1d8241f8a2230954371965bb91b20e726f144dce..4caa9fdc1ff3ed2bd6145cc653b49a84873ecac2 100644
> --- a/rust/kernel/media/v4l2/mod.rs
> +++ b/rust/kernel/media/v4l2/mod.rs
> @@ -15,3 +15,6 @@
>  pub mod caps;
>  /// Support for Video for Linux 2 (V4L2) file handles.
>  pub mod file;
> +
> +/// Support for Video for Linux 2 (V4L2) ioctls.
> +pub mod ioctl;
> diff --git a/rust/kernel/media/v4l2/video.rs b/rust/kernel/media/v4l2/video.rs
> index c0ac99a8234d2f7a8effd4701b9f7440236540c8..2390a93a39925dbff3f809bc65adfac1d881309c 100644
> --- a/rust/kernel/media/v4l2/video.rs
> +++ b/rust/kernel/media/v4l2/video.rs
> @@ -19,7 +19,7 @@
>  
>  use crate::{
>      alloc,
> -    error::to_result,
> +    error::{to_result, VTABLE_DEFAULT_ERROR},
>      media::v4l2::{self, caps::DeviceCaps, file::DriverFile, video},
>      prelude::*,
>      types::{ARef, AlwaysRefCounted, Opaque},
> @@ -143,6 +143,7 @@ unsafe impl<T: Driver> Sync for Device<T> {}
>  
>  /// The interface that must be implemented by structs that would otherwise embed
>  /// a C [`struct video_device`](srctree/include/media/v4l2-dev.h).
> +#[vtable]
>  pub trait Driver: v4l2::device::Driver {
>      /// The type of the driver's private data.
>      type Data;
> @@ -161,6 +162,15 @@ pub trait Driver: v4l2::device::Driver {
>  
>      /// The capabilities offered by this device node.
>      const CAPS: DeviceCaps;
> +
> +    /// Driver implementation for the `querycap` ioctl.
> +    fn querycap(
> +        _file: &v4l2::file::File<<Self as Driver>::File>,
> +        _data: &<Self as Driver>::Data,
> +        _cap: &mut v4l2::caps::Capabilities,
> +    ) -> Result {
> +        build_error!(VTABLE_DEFAULT_ERROR)
> +    }
>  }
>  
>  struct DeviceOptions<'a, T: Driver> {
> @@ -184,6 +194,7 @@ fn into_raw(self) -> bindings::video_device {
>              release: Some(Device::<T>::release_callback),
>              fops: super::file::FileVtable::<T::File>::build(),
>              device_caps: T::CAPS.as_raw(),
> +            ioctl_ops: super::ioctl::IoctlVtable::<T>::build(),
>              // SAFETY: All zeros is valid for the rest of the fields in this C
>              // type.
>              ..unsafe { MaybeUninit::zeroed().assume_init() }
> 
> -- 
> 2.50.1

Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 7/7] rust: samples: add the v4l2 sample driver
  2025-08-18  5:49 ` [PATCH 7/7] rust: samples: add the v4l2 sample driver Daniel Almeida
@ 2025-08-20  4:24   ` Elle Rhumsaa
  2025-08-20 12:39   ` Danilo Krummrich
  1 sibling, 0 replies; 17+ messages in thread
From: Elle Rhumsaa @ 2025-08-20  4:24 UTC (permalink / raw)
  To: Daniel Almeida
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, Alexandre Courbot, linux-kernel,
	rust-for-linux, kernel, linux-media

On Mon, Aug 18, 2025 at 02:49:53AM -0300, Daniel Almeida wrote:
> This driver is the only user of the v4l2 abstractions at the moment. It
> serves as a means to validate the abstractions by proving that the
> device is actually registered as /dev/videoX, and it can be opened and
> queried by v4l2-ctl, while also serving as a display of the current v4l2
> support in Rust, as well as a blueprint for more elaborate Rust v4l2
> drivers in the future.
> 
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> ---
>  MAINTAINERS                      |   1 +
>  samples/rust/Kconfig             |  11 +++
>  samples/rust/Makefile            |   1 +
>  samples/rust/rust_driver_v4l2.rs | 145 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 158 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6fc5d57950e474d73d5d65271a0394efc5a8960b..14521bc0585503992da582f2cee361666985e39f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15440,6 +15440,7 @@ L:	linux-media@vger.kernel.org
>  L:	rust-for-linux@vger.kernel.org
>  S:	Supported
>  F:	rust/media
> +F:	sample/rust/rust_driver_v4l2.rs
>  
>  MEDIATEK BLUETOOTH DRIVER
>  M:	Sean Wang <sean.wang@mediatek.com>
> diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
> index 7f7371a004ee0a8f67dca99c836596709a70c4fa..64422acf1e9da9d05f904e14fd423b3b4aef173a 100644
> --- a/samples/rust/Kconfig
> +++ b/samples/rust/Kconfig
> @@ -105,6 +105,17 @@ config SAMPLE_RUST_DRIVER_AUXILIARY
>  
>  	  If unsure, say N.
>  
> +config SAMPLE_RUST_DRIVER_V4L2
> +	tristate "Video4Linux2 driver"
> +	depends on MEDIA_SUPPORT && VIDEO_DEV
> +	help
> +	  This option builds the Rust V4L2 driver sample.
> +
> +	  To compile this as a module, choose M here:
> +	  the module will be called rust_driver_v4l2.
> +
> +	  If unsure, say N.
> +
>  config SAMPLE_RUST_HOSTPROGS
>  	bool "Host programs"
>  	help
> diff --git a/samples/rust/Makefile b/samples/rust/Makefile
> index bd2faad63b4f3befe7d1ed5139fe25c7a8b6d7f6..57e21f0373938bb70b4cb400ea550010895b4c94 100644
> --- a/samples/rust/Makefile
> +++ b/samples/rust/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_SAMPLE_RUST_DRIVER_PCI)		+= rust_driver_pci.o
>  obj-$(CONFIG_SAMPLE_RUST_DRIVER_PLATFORM)	+= rust_driver_platform.o
>  obj-$(CONFIG_SAMPLE_RUST_DRIVER_FAUX)		+= rust_driver_faux.o
>  obj-$(CONFIG_SAMPLE_RUST_DRIVER_AUXILIARY)	+= rust_driver_auxiliary.o
> +obj-$(CONFIG_SAMPLE_RUST_DRIVER_V4L2)		+= rust_driver_v4l2.o
>  obj-$(CONFIG_SAMPLE_RUST_CONFIGFS)		+= rust_configfs.o
>  
>  rust_print-y := rust_print_main.o rust_print_events.o
> diff --git a/samples/rust/rust_driver_v4l2.rs b/samples/rust/rust_driver_v4l2.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..a3ef98a613f2fed9e8589f0761ce7e43029c02b6
> --- /dev/null
> +++ b/samples/rust/rust_driver_v4l2.rs
> @@ -0,0 +1,145 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// SPDX-copyrightText: Copyright (C) 2025 Collabora Ltd.
> +
> +//! Rust V4L2 sample driver.
> +//!
> +//! This sample demonstrates how to:
> +//! - Register a V4L2 device (struct v4l2_device),
> +//! - Register a video node (struct video_device) using the Rust V4L2
> +//!   abstractions,
> +//! - Implement support for a V4L2 ioctl in a driver.
> +//!
> +//! It implements only `VIDIOC_QUERYCAP` and minimal open/close handling.
> +
> +use kernel::{
> +    c_str,
> +    device::Core,
> +    media::v4l2::{
> +        self,
> +        caps::{self, DeviceCaps},
> +        video,
> +    },
> +    of, platform,
> +    prelude::*,
> +    types::ARef,
> +    ThisModule,
> +};
> +
> +/// The private data associated with the V4L2 device.
> +#[pin_data]
> +struct Data {}
> +
> +/// The private data associated with a V4L2 device node, i.e. `struct
> +/// video_device`.
> +#[pin_data]
> +struct VideoData {}
> +
> +/// The private data associated with a V4L2 file, i.e. `struct v4l2_fh`.
> +#[pin_data]
> +struct File {}
> +
> +impl v4l2::file::DriverFile for File {
> +    type Driver = SampleDriver;
> +
> +    const MODULE: &'static ThisModule = &THIS_MODULE;
> +
> +    fn open(_vdev: &v4l2::video::Device<Self::Driver>) -> impl PinInit<Self, Error> {
> +        try_pin_init!(Self {})
> +    }
> +}
> +
> +struct SampleDriver {
> +    _pdev: ARef<platform::Device>,
> +    _v4l2_reg: v4l2::device::Registration<Self>,
> +    video_reg: video::Registration<Self>,
> +}
> +
> +impl v4l2::device::Driver for SampleDriver {
> +    type Data = Data;
> +}
> +
> +#[vtable]
> +impl video::Driver for SampleDriver {
> +    type Data = VideoData;
> +    type File = File;
> +
> +    const NODE_TYPE: video::NodeType = video::NodeType::Video;
> +    const DIRECTION: video::Direction = video::Direction::Rx;
> +    const NAME: &'static CStr = c_str!("rv4l2");
> +    const CAPS: DeviceCaps = caps::device_caps::VIDEO_CAPTURE;
> +
> +    fn querycap(
> +        _file: &v4l2::file::File<Self::File>,
> +        _data: &<Self as video::Driver>::Data,
> +        cap: &mut caps::Capabilities,
> +    ) -> Result {
> +        cap.set_driver(c_str!("rv4l2"))?;
> +        cap.set_card(c_str!("rv4l2"))?;
> +        cap.set_bus_info(c_str!("platform:rv4l2"))?;
> +
> +        cap.set_device_caps(Self::CAPS);
> +        Ok(())
> +    }
> +}
> +
> +kernel::of_device_table!(
> +    OF_TABLE,
> +    MODULE_OF_TABLE,
> +    <SampleDriver as platform::Driver>::IdInfo,
> +    [(of::DeviceId::new(c_str!("test, rust-v4l2")), ())]
> +);
> +
> +impl platform::Driver for SampleDriver {
> +    type IdInfo = ();
> +    const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
> +
> +    fn probe(
> +        pdev: &platform::Device<Core>,
> +        _info: Option<&Self::IdInfo>,
> +    ) -> Result<Pin<KBox<Self>>> {
> +        let dev = pdev.as_ref();
> +
> +        let v4l2_reg =
> +            v4l2::device::Registration::<Self>::new(dev, try_pin_init!(Data {}), GFP_KERNEL)?;
> +
> +        let video_reg = video::Registration::<Self>::new(
> +            v4l2_reg.device(),
> +            try_pin_init!(VideoData {}),
> +            GFP_KERNEL,
> +        )?;
> +
> +        let this = KBox::new(
> +            Self {
> +                _pdev: pdev.into(),
> +                _v4l2_reg: v4l2_reg,
> +                video_reg,
> +            },
> +            GFP_KERNEL,
> +        )?;
> +
> +        dev_info!(
> +            dev,
> +            "Registered /dev/video{}\n",
> +            this.video_reg.device().num()
> +        );
> +        Ok(this.into())
> +    }
> +
> +    fn unbind(pdev: &platform::Device<Core>, _this: Pin<&Self>) {
> +        dev_info!(pdev.as_ref(), "Unbinding Rust V4L2 sample driver\n");
> +    }
> +}
> +
> +impl Drop for SampleDriver {
> +    fn drop(&mut self) {
> +        dev_dbg!(self._pdev.as_ref(), "Rust V4L2 sample driver removed\n");
> +    }
> +}
> +
> +kernel::module_platform_driver! {
> +    type: SampleDriver,
> +    name: "rust_driver_v4l2",
> +    authors: ["Daniel Almeida"],
> +    description: "Rust V4L2 sample video driver",
> +    license: "GPL v2",
> +}
> 
> -- 
> 2.50.1

Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 7/7] rust: samples: add the v4l2 sample driver
  2025-08-18  5:49 ` [PATCH 7/7] rust: samples: add the v4l2 sample driver Daniel Almeida
  2025-08-20  4:24   ` Elle Rhumsaa
@ 2025-08-20 12:39   ` Danilo Krummrich
  1 sibling, 0 replies; 17+ messages in thread
From: Danilo Krummrich @ 2025-08-20 12:39 UTC (permalink / raw)
  To: Daniel Almeida
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Alexandre Courbot, linux-kernel, rust-for-linux,
	kernel, linux-media

On Mon Aug 18, 2025 at 7:49 AM CEST, Daniel Almeida wrote:
> +/// The private data associated with the V4L2 device.
> +#[pin_data]
> +struct Data {}

I think you don't need #[pin_data] on this and the other empty structs. There
should be a blanket implementation for PinInit that got you covered.

> +/// The private data associated with a V4L2 device node, i.e. `struct
> +/// video_device`.
> +#[pin_data]
> +struct VideoData {}
> +
> +/// The private data associated with a V4L2 file, i.e. `struct v4l2_fh`.
> +#[pin_data]
> +struct File {}
> +
> +impl v4l2::file::DriverFile for File {
> +    type Driver = SampleDriver;
> +
> +    const MODULE: &'static ThisModule = &THIS_MODULE;
> +
> +    fn open(_vdev: &v4l2::video::Device<Self::Driver>) -> impl PinInit<Self, Error> {
> +        try_pin_init!(Self {})
> +    }
> +}
> +
> +struct SampleDriver {
> +    _pdev: ARef<platform::Device>,
> +    _v4l2_reg: v4l2::device::Registration<Self>,
> +    video_reg: video::Registration<Self>,

Is the drop order of those registrations relevant? (If so, it shouldn't be on
the driver to get that right.)

> +}
> +
> +impl v4l2::device::Driver for SampleDriver {
> +    type Data = Data;
> +}
> +
> +#[vtable]
> +impl video::Driver for SampleDriver {
> +    type Data = VideoData;
> +    type File = File;
> +
> +    const NODE_TYPE: video::NodeType = video::NodeType::Video;
> +    const DIRECTION: video::Direction = video::Direction::Rx;
> +    const NAME: &'static CStr = c_str!("rv4l2");
> +    const CAPS: DeviceCaps = caps::device_caps::VIDEO_CAPTURE;
> +
> +    fn querycap(
> +        _file: &v4l2::file::File<Self::File>,
> +        _data: &<Self as video::Driver>::Data,
> +        cap: &mut caps::Capabilities,
> +    ) -> Result {
> +        cap.set_driver(c_str!("rv4l2"))?;
> +        cap.set_card(c_str!("rv4l2"))?;
> +        cap.set_bus_info(c_str!("platform:rv4l2"))?;
> +
> +        cap.set_device_caps(Self::CAPS);
> +        Ok(())
> +    }
> +}
> +
> +kernel::of_device_table!(
> +    OF_TABLE,
> +    MODULE_OF_TABLE,
> +    <SampleDriver as platform::Driver>::IdInfo,
> +    [(of::DeviceId::new(c_str!("test, rust-v4l2")), ())]
> +);
> +
> +impl platform::Driver for SampleDriver {
> +    type IdInfo = ();
> +    const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
> +
> +    fn probe(
> +        pdev: &platform::Device<Core>,
> +        _info: Option<&Self::IdInfo>,
> +    ) -> Result<Pin<KBox<Self>>> {
> +        let dev = pdev.as_ref();
> +
> +        let v4l2_reg =
> +            v4l2::device::Registration::<Self>::new(dev, try_pin_init!(Data {}), GFP_KERNEL)?;

As mentioned on the struct already, you shouldn't need try_pin_init!() here,
since there should be a blanket implementation for PinInit.

> +
> +        let video_reg = video::Registration::<Self>::new(
> +            v4l2_reg.device(),
> +            try_pin_init!(VideoData {}),
> +            GFP_KERNEL,
> +        )?;
> +
> +        let this = KBox::new(
> +            Self {
> +                _pdev: pdev.into(),
> +                _v4l2_reg: v4l2_reg,
> +                video_reg,
> +            },
> +            GFP_KERNEL,
> +        )?;
> +
> +        dev_info!(
> +            dev,
> +            "Registered /dev/video{}\n",
> +            this.video_reg.device().num()
> +        );
> +        Ok(this.into())
> +    }
> +
> +    fn unbind(pdev: &platform::Device<Core>, _this: Pin<&Self>) {
> +        dev_info!(pdev.as_ref(), "Unbinding Rust V4L2 sample driver\n");
> +    }
> +}
> +
> +impl Drop for SampleDriver {
> +    fn drop(&mut self) {
> +        dev_dbg!(self._pdev.as_ref(), "Rust V4L2 sample driver removed\n");

A driver being unbound / removed is the same thing.

unbind() is the correct callback for this. SampleDriver::drop() is for
additional cleanup needed for the driver's private data. It's just that this
cleanup also happens on driver unbind.

(I think I need to fix up the existing sample drivers that date from before we
had unbind() in place.)

I'd just remove this drop() implementation and the ARef<platform::Device> from
SampleDriver.

> +    }
> +}
> +
> +kernel::module_platform_driver! {
> +    type: SampleDriver,
> +    name: "rust_driver_v4l2",
> +    authors: ["Daniel Almeida"],
> +    description: "Rust V4L2 sample video driver",
> +    license: "GPL v2",
> +}
>
> -- 
> 2.50.1


^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2025-08-20 12:39 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-18  5:49 [PATCH 0/7] rust: add initial v4l2 support Daniel Almeida
2025-08-18  5:49 ` [PATCH 1/7] rust: media: add the media module Daniel Almeida
2025-08-18  8:56   ` Miguel Ojeda
2025-08-18 10:28   ` Janne Grunau
2025-08-18  5:49 ` [PATCH 2/7] rust: v4l2: add support for v4l2_device Daniel Almeida
2025-08-18  9:14   ` Danilo Krummrich
2025-08-18  5:49 ` [PATCH 3/7] rust: v4l2: add support for video device nodes Daniel Almeida
2025-08-18  9:26   ` Danilo Krummrich
2025-08-18  5:49 ` [PATCH 4/7] rust: v4l2: add support for v4l2 file handles Daniel Almeida
2025-08-18  5:49 ` [PATCH 5/7] rust: v4l2: add device capabilities Daniel Almeida
2025-08-20  4:14   ` Elle Rhumsaa
2025-08-18  5:49 ` [PATCH 6/7] rust: v4l2: add basic ioctl support Daniel Almeida
2025-08-20  4:22   ` Elle Rhumsaa
2025-08-18  5:49 ` [PATCH 7/7] rust: samples: add the v4l2 sample driver Daniel Almeida
2025-08-20  4:24   ` Elle Rhumsaa
2025-08-20 12:39   ` Danilo Krummrich
2025-08-18  8:45 ` [PATCH 0/7] rust: add initial v4l2 support Miguel Ojeda

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).