[PATCH RFC 0/3] Add support for video device state for capture devices

[PATCH RFC 0/3] Add support for video device state for capture devices

[Jai Luthra]

Hi,

This RFC series adds "state" support for video devices, along similar
lines to the existing support for try and active states for subdevs [1].
The main motivation for this change is to achieve the long-term goal of
"atomic" state changes across the media graph (all subdevices and video
devices), which ties together with Jacopo's in-progress work for
multi-context support [2].

Along with PATCH 1/3, which adds video_device_state to the framework,
PATCH 2-3/3 converts two capture drivers (J721E and RKISP) as examples
of using this newly introduced state mechanism to simplify their format
handling.

In future, this may be extended to other video device types, such as
metadata or video output devices, or M2M devices, but it is omitted from
this iteration to ease review and get early feedback.

[1]: https://lore.kernel.org/linux-media/20210610145606.3468235-1-tomi.valkeinen@ideasonboard.com/
[2]: https://lore.kernel.org/linux-media/20240913214657.1502838-1-jacopo.mondi@ideasonboard.com/

Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
Jai Luthra (3):
      media: v4l2-core: Add support for video device state
      media: ti: j721e-csi2rx: Use video_device_state
      media: rkisp1: Use video_device_state

 .../platform/rockchip/rkisp1/rkisp1-capture.c      | 113 +++++++++------------
 .../media/platform/rockchip/rkisp1/rkisp1-common.h |   4 -
 .../media/platform/ti/j721e-csi2rx/j721e-csi2rx.c  |  57 ++++-------
 drivers/media/v4l2-core/v4l2-dev.c                 |  32 ++++++
 drivers/media/v4l2-core/v4l2-fh.c                  |   1 +
 drivers/media/v4l2-core/v4l2-ioctl.c               |  44 ++++++--
 include/media/v4l2-dev.h                           |  52 ++++++++++
 include/media/v4l2-fh.h                            |   5 +-
 8 files changed, 192 insertions(+), 116 deletions(-)
---
base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
change-id: 20250703-vdev-state-0743baa0ad4b

Best regards,
-- 
Jai Luthra <jai.luthra@ideasonboard.com>

[PATCH RFC 1/3] media: v4l2-core: Add support for video device state

[Jai Luthra]

Simplify video capture device drivers by maintaining active and try
states to track the v4l2 formats (for video and metadata capture) of the
device.

A lot of boilerplate in the drivers can be reduced by combining the
implementation of s_fmt and try_fmt hooks, and using a framework helper
for the g_fmt hook.

To achieve this, we pass the newly introduced state structure to the
hooks through the already existing private void pointer. For S_FMT, we
pass the pointer to the active state and enforce that the vb2 queue is
not busy before calling the driver hook. For TRY_FMT, we pass the
pointer to the temporary state stored in the file handle. Finally, we
introduce a framework helper for the g_fmt hook that the drivers can
use.

The private void pointer argument already had some rare uses, so we
switch away from using it in the v4l_*ctrl functions to access
file->private_data, instead doing that access directly. Some drivers'
hooks might still expect it to point to file->private_data, so we
replace it with the state pointer only if a driver selects the
V4L2_FL_USES_STATE flag while registering the device.

State support may be extended in the future to other device types, such
as video/metadata output or M2M devices.

Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 drivers/media/v4l2-core/v4l2-dev.c   | 32 ++++++++++++++++++++++
 drivers/media/v4l2-core/v4l2-fh.c    |  1 +
 drivers/media/v4l2-core/v4l2-ioctl.c | 44 ++++++++++++++++++++++++------
 include/media/v4l2-dev.h             | 52 ++++++++++++++++++++++++++++++++++++
 include/media/v4l2-fh.h              |  5 +++-
 5 files changed, 125 insertions(+), 9 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index c369235113d98ae26c30a1aa386e7d60d541a66e..b8227d5508dc5bd775706264739e5db2d577f7fd 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -27,6 +27,7 @@
 #include <linux/uaccess.h>
 
 #include <media/v4l2-common.h>
+#include <media/v4l2-dev.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-ioctl.h>
 #include <media/v4l2-event.h>
@@ -163,6 +164,34 @@ void video_device_release_empty(struct video_device *vdev)
 }
 EXPORT_SYMBOL(video_device_release_empty);
 
+int video_device_g_fmt_vid(struct file *file, void *priv,
+			   struct v4l2_format *fmt)
+{
+	struct video_device_state *state = priv;
+
+	if (WARN_ON_ONCE(!state))
+		return -EINVAL;
+
+	*fmt = state->vid_fmt;
+
+	return 0;
+}
+EXPORT_SYMBOL(video_device_g_fmt_vid);
+
+int video_device_g_fmt_meta(struct file *file, void *priv,
+			    struct v4l2_format *fmt)
+{
+	struct video_device_state *state = priv;
+
+	if (WARN_ON_ONCE(!state))
+		return -EINVAL;
+
+	*fmt = state->meta_fmt;
+
+	return 0;
+}
+EXPORT_SYMBOL(video_device_g_fmt_meta);
+
 static inline void video_get(struct video_device *vdev)
 {
 	get_device(&vdev->dev);
@@ -927,6 +956,9 @@ int __video_register_device(struct video_device *vdev,
 	spin_lock_init(&vdev->fh_lock);
 	INIT_LIST_HEAD(&vdev->fh_list);
 
+	/* video_device_state support */
+	vdev->state.which = VIDEO_DEVICE_FORMAT_ACTIVE;
+
 	/* Part 1: check device type */
 	switch (type) {
 	case VFL_TYPE_VIDEO:
diff --git a/drivers/media/v4l2-core/v4l2-fh.c b/drivers/media/v4l2-core/v4l2-fh.c
index 90eec79ee995a2d214590beeacc91b9f8f33236d..d246e05f8ef1244e212412caa5c9c6788a5c948a 100644
--- a/drivers/media/v4l2-core/v4l2-fh.c
+++ b/drivers/media/v4l2-core/v4l2-fh.c
@@ -37,6 +37,7 @@ void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev)
 	INIT_LIST_HEAD(&fh->available);
 	INIT_LIST_HEAD(&fh->subscribed);
 	fh->sequence = -1;
+	fh->state.which = VIDEO_DEVICE_FORMAT_TRY;
 	mutex_init(&fh->subscribe_lock);
 }
 EXPORT_SYMBOL_GPL(v4l2_fh_init);
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 650dc1956f73d2f1943b56c42140c7b8d757259f..78a0db364725ec6641be37d0c4804edb222a9154 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -21,6 +21,7 @@
 
 #include <media/media-device.h> /* for media_set_bus_info() */
 #include <media/v4l2-common.h>
+#include <media/v4l2-dev.h>
 #include <media/v4l2-ioctl.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-fh.h>
@@ -1745,6 +1746,15 @@ static int v4l_s_fmt(const struct v4l2_ioctl_ops *ops,
 	if (ret)
 		return ret;
 
+	/*
+	 * Make sure queue isn't busy for devices that use state, as they have a
+	 * single implementation for .s_fmt and .try_fmt, and rely on us to make
+	 * sure the queue is not busy when calling for the .s_fmt case
+	 */
+	if (test_bit(V4L2_FL_USES_STATE, &vfd->flags) && vfd->queue &&
+	    vb2_is_busy(vfd->queue))
+		return -EBUSY;
+
 	ret = v4l_enable_media_source(vfd);
 	if (ret)
 		return ret;
@@ -2293,7 +2303,7 @@ static int v4l_queryctrl(const struct v4l2_ioctl_ops *ops,
 	struct v4l2_query_ext_ctrl qec = {};
 	struct v4l2_queryctrl *p = arg;
 	struct v4l2_fh *vfh =
-		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
+		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
 	int ret;
 
 	if (vfh && vfh->ctrl_handler)
@@ -2318,7 +2328,7 @@ static int v4l_query_ext_ctrl(const struct v4l2_ioctl_ops *ops,
 	struct video_device *vfd = video_devdata(file);
 	struct v4l2_query_ext_ctrl *p = arg;
 	struct v4l2_fh *vfh =
-		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
+		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
 
 	if (vfh && vfh->ctrl_handler)
 		return v4l2_query_ext_ctrl(vfh->ctrl_handler, p);
@@ -2335,7 +2345,7 @@ static int v4l_querymenu(const struct v4l2_ioctl_ops *ops,
 	struct video_device *vfd = video_devdata(file);
 	struct v4l2_querymenu *p = arg;
 	struct v4l2_fh *vfh =
-		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
+		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
 
 	if (vfh && vfh->ctrl_handler)
 		return v4l2_querymenu(vfh->ctrl_handler, p);
@@ -2352,7 +2362,7 @@ static int v4l_g_ctrl(const struct v4l2_ioctl_ops *ops,
 	struct video_device *vfd = video_devdata(file);
 	struct v4l2_control *p = arg;
 	struct v4l2_fh *vfh =
-		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
+		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
 	struct v4l2_ext_controls ctrls;
 	struct v4l2_ext_control ctrl;
 
@@ -2384,7 +2394,7 @@ static int v4l_s_ctrl(const struct v4l2_ioctl_ops *ops,
 	struct video_device *vfd = video_devdata(file);
 	struct v4l2_control *p = arg;
 	struct v4l2_fh *vfh =
-		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
+		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
 	struct v4l2_ext_controls ctrls;
 	struct v4l2_ext_control ctrl;
 	int ret;
@@ -2414,7 +2424,7 @@ static int v4l_g_ext_ctrls(const struct v4l2_ioctl_ops *ops,
 	struct video_device *vfd = video_devdata(file);
 	struct v4l2_ext_controls *p = arg;
 	struct v4l2_fh *vfh =
-		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
+		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
 
 	p->error_idx = p->count;
 	if (vfh && vfh->ctrl_handler)
@@ -2435,7 +2445,7 @@ static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops,
 	struct video_device *vfd = video_devdata(file);
 	struct v4l2_ext_controls *p = arg;
 	struct v4l2_fh *vfh =
-		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
+		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
 
 	p->error_idx = p->count;
 	if (vfh && vfh->ctrl_handler)
@@ -2456,7 +2466,7 @@ static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops,
 	struct video_device *vfd = video_devdata(file);
 	struct v4l2_ext_controls *p = arg;
 	struct v4l2_fh *vfh =
-		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
+		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
 
 	p->error_idx = p->count;
 	if (vfh && vfh->ctrl_handler)
@@ -3057,6 +3067,21 @@ void v4l_printk_ioctl(const char *prefix, unsigned int cmd)
 }
 EXPORT_SYMBOL(v4l_printk_ioctl);
 
+static struct video_device_state *
+video_device_get_state(struct video_device *vfd, struct v4l2_fh *vfh,
+		       unsigned int cmd, void *arg)
+{
+	switch (cmd) {
+	default:
+		return NULL;
+	case VIDIOC_G_FMT:
+	case VIDIOC_S_FMT:
+		return &vfd->state;
+	case VIDIOC_TRY_FMT:
+		return &vfh->state;
+	}
+}
+
 static long __video_do_ioctl(struct file *file,
 		unsigned int cmd, void *arg)
 {
@@ -3081,6 +3106,9 @@ static long __video_do_ioctl(struct file *file,
 	if (test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags))
 		vfh = file->private_data;
 
+	if (vfh && test_bit(V4L2_FL_USES_STATE, &vfd->flags))
+		fh = video_device_get_state(vfd, vfh, cmd, arg);
+
 	/*
 	 * We need to serialize streamon/off with queueing new requests.
 	 * These ioctls may trigger the cancellation of a streaming
diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
index 1b6222fab24eda96cbe459b435431c01f7259366..8e6e7799212cd07ae4ad3dfc85912c21a9bcab2d 100644
--- a/include/media/v4l2-dev.h
+++ b/include/media/v4l2-dev.h
@@ -89,12 +89,18 @@ struct dentry;
  *	set by the core when the sub-devices device nodes are registered with
  *	v4l2_device_register_ro_subdev_nodes() and used by the sub-device ioctl
  *	handler to restrict access to some ioctl calls.
+ * @V4L2_FL_USES_STATE:
+ *	indicates that the &struct video_device has state support.
+ *	The active video and metadata formats are stored in video_device.state,
+ *	and the try video and metadata formats are stored in v4l2_fh.state.
+ *	All new drivers should use it.
  */
 enum v4l2_video_device_flags {
 	V4L2_FL_REGISTERED		= 0,
 	V4L2_FL_USES_V4L2_FH		= 1,
 	V4L2_FL_QUIRK_INVERTED_CROP	= 2,
 	V4L2_FL_SUBDEV_RO_DEVNODE	= 3,
+	V4L2_FL_USES_STATE		= 4,
 };
 
 /* Priority helper functions */
@@ -214,6 +220,30 @@ struct v4l2_file_operations {
 	int (*release) (struct file *);
 };
 
+/**
+ * enum video_device_format_whence - Video device format type
+ *
+ * @V4L2_DEVICE_FORMAT_TRY: from VIDIOC_TRY_FMT, for negotiation only
+ * @V4L2_DEVICE_FORMAT_ACTIVE: from VIDIOC_S_FMT, applied to the device
+ */
+enum video_device_format_whence {
+	VIDEO_DEVICE_FORMAT_TRY = 0,
+	VIDEO_DEVICE_FORMAT_ACTIVE = 1,
+};
+
+/**
+ * struct video_device_state - Used for storing video device state information.
+ *
+ * @vid_fmt: Format of the video capture stream
+ * @meta_fmt: Format of the metadata capture stream
+ * @which: is this a TRY or ACTIVE format?
+ */
+struct video_device_state {
+	struct v4l2_format vid_fmt;
+	struct v4l2_format meta_fmt;
+	enum video_device_format_whence which;
+};
+
 /*
  * Newer version of video_device, handled by videodev2.c
  *	This version moves redundant code from video device code to
@@ -238,6 +268,7 @@ struct v4l2_file_operations {
  * @queue: &struct vb2_queue associated with this device node. May be NULL.
  * @prio: pointer to &struct v4l2_prio_state with device's Priority state.
  *	 If NULL, then v4l2_dev->prio will be used.
+ * @state: &struct video_device_state, holds the active state for the device.
  * @name: video device name
  * @vfl_type: V4L device type, as defined by &enum vfl_devnode_type
  * @vfl_dir: V4L receiver, transmitter or m2m
@@ -283,6 +314,7 @@ struct video_device {
 	struct vb2_queue *queue;
 
 	struct v4l2_prio_state *prio;
+	struct video_device_state state;
 
 	/* device info */
 	char name[64];
@@ -540,6 +572,26 @@ static inline int video_is_registered(struct video_device *vdev)
 	return test_bit(V4L2_FL_REGISTERED, &vdev->flags);
 }
 
+/**
+ * video_device_g_fmt_vid() - fill video v4l2_format from the state.
+ *
+ * @file: pointer to struct file
+ * @state: pointer to video device state
+ * @format: pointer to &struct v4l2_format
+ */
+int video_device_g_fmt_vid(struct file *file, void *state,
+			   struct v4l2_format *format);
+
+/**
+ * video_device_g_fmt_meta() - fill metadata v4l2_format from the state.
+ *
+ * @file: pointer to struct file
+ * @state: pointer to video device state
+ * @format: pointer to &struct v4l2_format
+ */
+int video_device_g_fmt_meta(struct file *file, void *state,
+			    struct v4l2_format *format);
+
 /**
  * v4l2_debugfs_root - returns the dentry of the top-level "v4l2" debugfs dir
  *
diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
index b5b3e00c8e6a0b082d9cd8a0c972a5094adcb6f2..02579f87ba99d0c849a0865f8cc4295446c39f94 100644
--- a/include/media/v4l2-fh.h
+++ b/include/media/v4l2-fh.h
@@ -18,7 +18,8 @@
 #include <linux/list.h>
 #include <linux/videodev2.h>
 
-struct video_device;
+#include <media/v4l2-dev.h>
+
 struct v4l2_ctrl_handler;
 
 /**
@@ -28,6 +29,7 @@ struct v4l2_ctrl_handler;
  * @vdev: pointer to &struct video_device
  * @ctrl_handler: pointer to &struct v4l2_ctrl_handler
  * @prio: priority of the file handler, as defined by &enum v4l2_priority
+ * @state: try state used for format negotiation on the video device
  *
  * @wait: event' s wait queue
  * @subscribe_lock: serialise changes to the subscribed list; guarantee that
@@ -44,6 +46,7 @@ struct v4l2_fh {
 	struct video_device	*vdev;
 	struct v4l2_ctrl_handler *ctrl_handler;
 	enum v4l2_priority	prio;
+	struct video_device_state state;
 
 	/* Events */
 	wait_queue_head_t	wait;

-- 
2.49.0

Re: [PATCH RFC 1/3] media: v4l2-core: Add support for video device state

[Sakari Ailus]

Hi Jai,

Thanks for the patchset.

On Thu, Jul 03, 2025 at 06:02:08PM -0700, Jai Luthra wrote:
> Simplify video capture device drivers by maintaining active and try
> states to track the v4l2 formats (for video and metadata capture) of the
> device.
> 
> A lot of boilerplate in the drivers can be reduced by combining the
> implementation of s_fmt and try_fmt hooks, and using a framework helper
> for the g_fmt hook.
> 
> To achieve this, we pass the newly introduced state structure to the
> hooks through the already existing private void pointer. For S_FMT, we
> pass the pointer to the active state and enforce that the vb2 queue is
> not busy before calling the driver hook. For TRY_FMT, we pass the
> pointer to the temporary state stored in the file handle. Finally, we
> introduce a framework helper for the g_fmt hook that the drivers can
> use.
> 
> The private void pointer argument already had some rare uses, so we
> switch away from using it in the v4l_*ctrl functions to access
> file->private_data, instead doing that access directly. Some drivers'
> hooks might still expect it to point to file->private_data, so we
> replace it with the state pointer only if a driver selects the
> V4L2_FL_USES_STATE flag while registering the device.
> 
> State support may be extended in the future to other device types, such
> as video/metadata output or M2M devices.
> 
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> ---
>  drivers/media/v4l2-core/v4l2-dev.c   | 32 ++++++++++++++++++++++
>  drivers/media/v4l2-core/v4l2-fh.c    |  1 +
>  drivers/media/v4l2-core/v4l2-ioctl.c | 44 ++++++++++++++++++++++++------
>  include/media/v4l2-dev.h             | 52 ++++++++++++++++++++++++++++++++++++
>  include/media/v4l2-fh.h              |  5 +++-
>  5 files changed, 125 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index c369235113d98ae26c30a1aa386e7d60d541a66e..b8227d5508dc5bd775706264739e5db2d577f7fd 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -27,6 +27,7 @@
>  #include <linux/uaccess.h>
>  
>  #include <media/v4l2-common.h>
> +#include <media/v4l2-dev.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-ioctl.h>
>  #include <media/v4l2-event.h>
> @@ -163,6 +164,34 @@ void video_device_release_empty(struct video_device *vdev)
>  }
>  EXPORT_SYMBOL(video_device_release_empty);
>  
> +int video_device_g_fmt_vid(struct file *file, void *priv,
> +			   struct v4l2_format *fmt)
> +{
> +	struct video_device_state *state = priv;
> +
> +	if (WARN_ON_ONCE(!state))
> +		return -EINVAL;
> +
> +	*fmt = state->vid_fmt;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(video_device_g_fmt_vid);

EXPORT_SYMBOL_GPL(), please.

> +
> +int video_device_g_fmt_meta(struct file *file, void *priv,
> +			    struct v4l2_format *fmt)
> +{
> +	struct video_device_state *state = priv;
> +
> +	if (WARN_ON_ONCE(!state))
> +		return -EINVAL;
> +
> +	*fmt = state->meta_fmt;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(video_device_g_fmt_meta);
> +
>  static inline void video_get(struct video_device *vdev)
>  {
>  	get_device(&vdev->dev);
> @@ -927,6 +956,9 @@ int __video_register_device(struct video_device *vdev,
>  	spin_lock_init(&vdev->fh_lock);
>  	INIT_LIST_HEAD(&vdev->fh_list);
>  
> +	/* video_device_state support */
> +	vdev->state.which = VIDEO_DEVICE_FORMAT_ACTIVE;
> +
>  	/* Part 1: check device type */
>  	switch (type) {
>  	case VFL_TYPE_VIDEO:
> diff --git a/drivers/media/v4l2-core/v4l2-fh.c b/drivers/media/v4l2-core/v4l2-fh.c
> index 90eec79ee995a2d214590beeacc91b9f8f33236d..d246e05f8ef1244e212412caa5c9c6788a5c948a 100644
> --- a/drivers/media/v4l2-core/v4l2-fh.c
> +++ b/drivers/media/v4l2-core/v4l2-fh.c
> @@ -37,6 +37,7 @@ void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev)
>  	INIT_LIST_HEAD(&fh->available);
>  	INIT_LIST_HEAD(&fh->subscribed);
>  	fh->sequence = -1;
> +	fh->state.which = VIDEO_DEVICE_FORMAT_TRY;
>  	mutex_init(&fh->subscribe_lock);
>  }
>  EXPORT_SYMBOL_GPL(v4l2_fh_init);
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 650dc1956f73d2f1943b56c42140c7b8d757259f..78a0db364725ec6641be37d0c4804edb222a9154 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -21,6 +21,7 @@
>  
>  #include <media/media-device.h> /* for media_set_bus_info() */
>  #include <media/v4l2-common.h>
> +#include <media/v4l2-dev.h>
>  #include <media/v4l2-ioctl.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-fh.h>
> @@ -1745,6 +1746,15 @@ static int v4l_s_fmt(const struct v4l2_ioctl_ops *ops,
>  	if (ret)
>  		return ret;
>  
> +	/*
> +	 * Make sure queue isn't busy for devices that use state, as they have a
> +	 * single implementation for .s_fmt and .try_fmt, and rely on us to make
> +	 * sure the queue is not busy when calling for the .s_fmt case
> +	 */
> +	if (test_bit(V4L2_FL_USES_STATE, &vfd->flags) && vfd->queue &&
> +	    vb2_is_busy(vfd->queue))
> +		return -EBUSY;
> +
>  	ret = v4l_enable_media_source(vfd);
>  	if (ret)
>  		return ret;
> @@ -2293,7 +2303,7 @@ static int v4l_queryctrl(const struct v4l2_ioctl_ops *ops,
>  	struct v4l2_query_ext_ctrl qec = {};
>  	struct v4l2_queryctrl *p = arg;
>  	struct v4l2_fh *vfh =
> -		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> +		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
>  	int ret;
>  
>  	if (vfh && vfh->ctrl_handler)
> @@ -2318,7 +2328,7 @@ static int v4l_query_ext_ctrl(const struct v4l2_ioctl_ops *ops,
>  	struct video_device *vfd = video_devdata(file);
>  	struct v4l2_query_ext_ctrl *p = arg;
>  	struct v4l2_fh *vfh =
> -		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> +		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
>  
>  	if (vfh && vfh->ctrl_handler)
>  		return v4l2_query_ext_ctrl(vfh->ctrl_handler, p);
> @@ -2335,7 +2345,7 @@ static int v4l_querymenu(const struct v4l2_ioctl_ops *ops,
>  	struct video_device *vfd = video_devdata(file);
>  	struct v4l2_querymenu *p = arg;
>  	struct v4l2_fh *vfh =
> -		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> +		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
>  
>  	if (vfh && vfh->ctrl_handler)
>  		return v4l2_querymenu(vfh->ctrl_handler, p);
> @@ -2352,7 +2362,7 @@ static int v4l_g_ctrl(const struct v4l2_ioctl_ops *ops,
>  	struct video_device *vfd = video_devdata(file);
>  	struct v4l2_control *p = arg;
>  	struct v4l2_fh *vfh =
> -		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> +		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
>  	struct v4l2_ext_controls ctrls;
>  	struct v4l2_ext_control ctrl;
>  
> @@ -2384,7 +2394,7 @@ static int v4l_s_ctrl(const struct v4l2_ioctl_ops *ops,
>  	struct video_device *vfd = video_devdata(file);
>  	struct v4l2_control *p = arg;
>  	struct v4l2_fh *vfh =
> -		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> +		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
>  	struct v4l2_ext_controls ctrls;
>  	struct v4l2_ext_control ctrl;
>  	int ret;
> @@ -2414,7 +2424,7 @@ static int v4l_g_ext_ctrls(const struct v4l2_ioctl_ops *ops,
>  	struct video_device *vfd = video_devdata(file);
>  	struct v4l2_ext_controls *p = arg;
>  	struct v4l2_fh *vfh =
> -		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> +		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
>  
>  	p->error_idx = p->count;
>  	if (vfh && vfh->ctrl_handler)
> @@ -2435,7 +2445,7 @@ static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops,
>  	struct video_device *vfd = video_devdata(file);
>  	struct v4l2_ext_controls *p = arg;
>  	struct v4l2_fh *vfh =
> -		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> +		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
>  
>  	p->error_idx = p->count;
>  	if (vfh && vfh->ctrl_handler)
> @@ -2456,7 +2466,7 @@ static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops,
>  	struct video_device *vfd = video_devdata(file);
>  	struct v4l2_ext_controls *p = arg;
>  	struct v4l2_fh *vfh =
> -		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> +		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
>  
>  	p->error_idx = p->count;
>  	if (vfh && vfh->ctrl_handler)
> @@ -3057,6 +3067,21 @@ void v4l_printk_ioctl(const char *prefix, unsigned int cmd)
>  }
>  EXPORT_SYMBOL(v4l_printk_ioctl);
>  
> +static struct video_device_state *
> +video_device_get_state(struct video_device *vfd, struct v4l2_fh *vfh,
> +		       unsigned int cmd, void *arg)
> +{
> +	switch (cmd) {
> +	default:
> +		return NULL;
> +	case VIDIOC_G_FMT:
> +	case VIDIOC_S_FMT:
> +		return &vfd->state;
> +	case VIDIOC_TRY_FMT:
> +		return &vfh->state;
> +	}
> +}
> +
>  static long __video_do_ioctl(struct file *file,
>  		unsigned int cmd, void *arg)
>  {
> @@ -3081,6 +3106,9 @@ static long __video_do_ioctl(struct file *file,
>  	if (test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags))
>  		vfh = file->private_data;
>  
> +	if (vfh && test_bit(V4L2_FL_USES_STATE, &vfd->flags))
> +		fh = video_device_get_state(vfd, vfh, cmd, arg);
> +
>  	/*
>  	 * We need to serialize streamon/off with queueing new requests.
>  	 * These ioctls may trigger the cancellation of a streaming
> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> index 1b6222fab24eda96cbe459b435431c01f7259366..8e6e7799212cd07ae4ad3dfc85912c21a9bcab2d 100644
> --- a/include/media/v4l2-dev.h
> +++ b/include/media/v4l2-dev.h
> @@ -89,12 +89,18 @@ struct dentry;
>   *	set by the core when the sub-devices device nodes are registered with
>   *	v4l2_device_register_ro_subdev_nodes() and used by the sub-device ioctl
>   *	handler to restrict access to some ioctl calls.
> + * @V4L2_FL_USES_STATE:
> + *	indicates that the &struct video_device has state support.
> + *	The active video and metadata formats are stored in video_device.state,
> + *	and the try video and metadata formats are stored in v4l2_fh.state.
> + *	All new drivers should use it.
>   */
>  enum v4l2_video_device_flags {
>  	V4L2_FL_REGISTERED		= 0,
>  	V4L2_FL_USES_V4L2_FH		= 1,
>  	V4L2_FL_QUIRK_INVERTED_CROP	= 2,
>  	V4L2_FL_SUBDEV_RO_DEVNODE	= 3,
> +	V4L2_FL_USES_STATE		= 4,
>  };
>  
>  /* Priority helper functions */
> @@ -214,6 +220,30 @@ struct v4l2_file_operations {
>  	int (*release) (struct file *);
>  };
>  
> +/**
> + * enum video_device_format_whence - Video device format type

What about selection rectangles?

Should this be called video_device_state_whence?

> + *
> + * @V4L2_DEVICE_FORMAT_TRY: from VIDIOC_TRY_FMT, for negotiation only
> + * @V4L2_DEVICE_FORMAT_ACTIVE: from VIDIOC_S_FMT, applied to the device
> + */
> +enum video_device_format_whence {
> +	VIDEO_DEVICE_FORMAT_TRY = 0,
> +	VIDEO_DEVICE_FORMAT_ACTIVE = 1,
> +};
> +
> +/**
> + * struct video_device_state - Used for storing video device state information.
> + *
> + * @vid_fmt: Format of the video capture stream
> + * @meta_fmt: Format of the metadata capture stream
> + * @which: is this a TRY or ACTIVE format?
> + */
> +struct video_device_state {
> +	struct v4l2_format vid_fmt;
> +	struct v4l2_format meta_fmt;
> +	enum video_device_format_whence which;
> +};
> +
>  /*
>   * Newer version of video_device, handled by videodev2.c
>   *	This version moves redundant code from video device code to
> @@ -238,6 +268,7 @@ struct v4l2_file_operations {
>   * @queue: &struct vb2_queue associated with this device node. May be NULL.
>   * @prio: pointer to &struct v4l2_prio_state with device's Priority state.
>   *	 If NULL, then v4l2_dev->prio will be used.
> + * @state: &struct video_device_state, holds the active state for the device.
>   * @name: video device name
>   * @vfl_type: V4L device type, as defined by &enum vfl_devnode_type
>   * @vfl_dir: V4L receiver, transmitter or m2m
> @@ -283,6 +314,7 @@ struct video_device {
>  	struct vb2_queue *queue;
>  
>  	struct v4l2_prio_state *prio;
> +	struct video_device_state state;
>  
>  	/* device info */
>  	char name[64];
> @@ -540,6 +572,26 @@ static inline int video_is_registered(struct video_device *vdev)
>  	return test_bit(V4L2_FL_REGISTERED, &vdev->flags);
>  }
>  
> +/**
> + * video_device_g_fmt_vid() - fill video v4l2_format from the state.
> + *
> + * @file: pointer to struct file
> + * @state: pointer to video device state
> + * @format: pointer to &struct v4l2_format
> + */
> +int video_device_g_fmt_vid(struct file *file, void *state,
> +			   struct v4l2_format *format);
> +
> +/**
> + * video_device_g_fmt_meta() - fill metadata v4l2_format from the state.
> + *
> + * @file: pointer to struct file
> + * @state: pointer to video device state
> + * @format: pointer to &struct v4l2_format
> + */
> +int video_device_g_fmt_meta(struct file *file, void *state,
> +			    struct v4l2_format *format);
> +
>  /**
>   * v4l2_debugfs_root - returns the dentry of the top-level "v4l2" debugfs dir
>   *
> diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
> index b5b3e00c8e6a0b082d9cd8a0c972a5094adcb6f2..02579f87ba99d0c849a0865f8cc4295446c39f94 100644
> --- a/include/media/v4l2-fh.h
> +++ b/include/media/v4l2-fh.h
> @@ -18,7 +18,8 @@
>  #include <linux/list.h>
>  #include <linux/videodev2.h>
>  
> -struct video_device;
> +#include <media/v4l2-dev.h>
> +
>  struct v4l2_ctrl_handler;
>  
>  /**
> @@ -28,6 +29,7 @@ struct v4l2_ctrl_handler;
>   * @vdev: pointer to &struct video_device
>   * @ctrl_handler: pointer to &struct v4l2_ctrl_handler
>   * @prio: priority of the file handler, as defined by &enum v4l2_priority
> + * @state: try state used for format negotiation on the video device
>   *
>   * @wait: event' s wait queue
>   * @subscribe_lock: serialise changes to the subscribed list; guarantee that
> @@ -44,6 +46,7 @@ struct v4l2_fh {
>  	struct video_device	*vdev;
>  	struct v4l2_ctrl_handler *ctrl_handler;
>  	enum v4l2_priority	prio;
> +	struct video_device_state state;
>  
>  	/* Events */
>  	wait_queue_head_t	wait;
> 

It'd be also nice to have a callback to initialise the state. That could
well go to a separate patch though.

-- 
Regards,

Sakari Ailus

Re: [PATCH RFC 1/3] media: v4l2-core: Add support for video device state

[Jacopo Mondi]

Hi Jai

On Thu, Jul 03, 2025 at 06:02:08PM -0700, Jai Luthra wrote:
> Simplify video capture device drivers by maintaining active and try
> states to track the v4l2 formats (for video and metadata capture) of the
> device.
>
> A lot of boilerplate in the drivers can be reduced by combining the
> implementation of s_fmt and try_fmt hooks, and using a framework helper
> for the g_fmt hook.
>
> To achieve this, we pass the newly introduced state structure to the
> hooks through the already existing private void pointer. For S_FMT, we
> pass the pointer to the active state and enforce that the vb2 queue is
> not busy before calling the driver hook. For TRY_FMT, we pass the
> pointer to the temporary state stored in the file handle. Finally, we
> introduce a framework helper for the g_fmt hook that the drivers can
> use.
>
> The private void pointer argument already had some rare uses, so we
> switch away from using it in the v4l_*ctrl functions to access
> file->private_data, instead doing that access directly. Some drivers'
> hooks might still expect it to point to file->private_data, so we
> replace it with the state pointer only if a driver selects the
> V4L2_FL_USES_STATE flag while registering the device.
>
> State support may be extended in the future to other device types, such
> as video/metadata output or M2M devices.
>
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> ---
>  drivers/media/v4l2-core/v4l2-dev.c   | 32 ++++++++++++++++++++++
>  drivers/media/v4l2-core/v4l2-fh.c    |  1 +
>  drivers/media/v4l2-core/v4l2-ioctl.c | 44 ++++++++++++++++++++++++------
>  include/media/v4l2-dev.h             | 52 ++++++++++++++++++++++++++++++++++++
>  include/media/v4l2-fh.h              |  5 +++-
>  5 files changed, 125 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index c369235113d98ae26c30a1aa386e7d60d541a66e..b8227d5508dc5bd775706264739e5db2d577f7fd 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -27,6 +27,7 @@
>  #include <linux/uaccess.h>
>
>  #include <media/v4l2-common.h>
> +#include <media/v4l2-dev.h>

v4l2-common includes this already


>  #include <media/v4l2-device.h>
>  #include <media/v4l2-ioctl.h>
>  #include <media/v4l2-event.h>
> @@ -163,6 +164,34 @@ void video_device_release_empty(struct video_device *vdev)
>  }
>  EXPORT_SYMBOL(video_device_release_empty);
>
> +int video_device_g_fmt_vid(struct file *file, void *priv,

The function prototype (and documentation) names the second parameter
state.

> +			   struct v4l2_format *fmt)
> +{
> +	struct video_device_state *state = priv;
> +
> +	if (WARN_ON_ONCE(!state))
> +		return -EINVAL;
> +
> +	*fmt = state->vid_fmt;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(video_device_g_fmt_vid);
> +
> +int video_device_g_fmt_meta(struct file *file, void *priv,
> +			    struct v4l2_format *fmt)
> +{
> +	struct video_device_state *state = priv;
> +
> +	if (WARN_ON_ONCE(!state))
> +		return -EINVAL;
> +
> +	*fmt = state->meta_fmt;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(video_device_g_fmt_meta);
> +

These two helpers, and the presence of two struct v4l2_format
'vid_fmt' and 'meta_fmt' are a bit puzzling to me.

A video device will handle one buffer type and struct v4l2_format
accomodates all type of buffers in the 'fmt' union member.

Why do you store two of them in the video device state ?

>  static inline void video_get(struct video_device *vdev)
>  {
>  	get_device(&vdev->dev);
> @@ -927,6 +956,9 @@ int __video_register_device(struct video_device *vdev,
>  	spin_lock_init(&vdev->fh_lock);
>  	INIT_LIST_HEAD(&vdev->fh_list);
>
> +	/* video_device_state support */
> +	vdev->state.which = VIDEO_DEVICE_FORMAT_ACTIVE;
> +
>  	/* Part 1: check device type */
>  	switch (type) {
>  	case VFL_TYPE_VIDEO:
> diff --git a/drivers/media/v4l2-core/v4l2-fh.c b/drivers/media/v4l2-core/v4l2-fh.c
> index 90eec79ee995a2d214590beeacc91b9f8f33236d..d246e05f8ef1244e212412caa5c9c6788a5c948a 100644
> --- a/drivers/media/v4l2-core/v4l2-fh.c
> +++ b/drivers/media/v4l2-core/v4l2-fh.c
> @@ -37,6 +37,7 @@ void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev)
>  	INIT_LIST_HEAD(&fh->available);
>  	INIT_LIST_HEAD(&fh->subscribed);
>  	fh->sequence = -1;
> +	fh->state.which = VIDEO_DEVICE_FORMAT_TRY;
>  	mutex_init(&fh->subscribe_lock);
>  }
>  EXPORT_SYMBOL_GPL(v4l2_fh_init);
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 650dc1956f73d2f1943b56c42140c7b8d757259f..78a0db364725ec6641be37d0c4804edb222a9154 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -21,6 +21,7 @@
>
>  #include <media/media-device.h> /* for media_set_bus_info() */
>  #include <media/v4l2-common.h>
> +#include <media/v4l2-dev.h>
>  #include <media/v4l2-ioctl.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-fh.h>
> @@ -1745,6 +1746,15 @@ static int v4l_s_fmt(const struct v4l2_ioctl_ops *ops,
>  	if (ret)
>  		return ret;
>
> +	/*
> +	 * Make sure queue isn't busy for devices that use state, as they have a
> +	 * single implementation for .s_fmt and .try_fmt, and rely on us to make
> +	 * sure the queue is not busy when calling for the .s_fmt case
> +	 */
> +	if (test_bit(V4L2_FL_USES_STATE, &vfd->flags) && vfd->queue &&
> +	    vb2_is_busy(vfd->queue))
> +		return -EBUSY;
> +
>  	ret = v4l_enable_media_source(vfd);
>  	if (ret)
>  		return ret;
> @@ -2293,7 +2303,7 @@ static int v4l_queryctrl(const struct v4l2_ioctl_ops *ops,
>  	struct v4l2_query_ext_ctrl qec = {};
>  	struct v4l2_queryctrl *p = arg;
>  	struct v4l2_fh *vfh =
> -		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> +		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
>  	int ret;
>
>  	if (vfh && vfh->ctrl_handler)
> @@ -2318,7 +2328,7 @@ static int v4l_query_ext_ctrl(const struct v4l2_ioctl_ops *ops,
>  	struct video_device *vfd = video_devdata(file);
>  	struct v4l2_query_ext_ctrl *p = arg;
>  	struct v4l2_fh *vfh =
> -		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> +		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
>
>  	if (vfh && vfh->ctrl_handler)
>  		return v4l2_query_ext_ctrl(vfh->ctrl_handler, p);
> @@ -2335,7 +2345,7 @@ static int v4l_querymenu(const struct v4l2_ioctl_ops *ops,
>  	struct video_device *vfd = video_devdata(file);
>  	struct v4l2_querymenu *p = arg;
>  	struct v4l2_fh *vfh =
> -		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> +		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
>
>  	if (vfh && vfh->ctrl_handler)
>  		return v4l2_querymenu(vfh->ctrl_handler, p);
> @@ -2352,7 +2362,7 @@ static int v4l_g_ctrl(const struct v4l2_ioctl_ops *ops,
>  	struct video_device *vfd = video_devdata(file);
>  	struct v4l2_control *p = arg;
>  	struct v4l2_fh *vfh =
> -		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> +		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
>  	struct v4l2_ext_controls ctrls;
>  	struct v4l2_ext_control ctrl;
>
> @@ -2384,7 +2394,7 @@ static int v4l_s_ctrl(const struct v4l2_ioctl_ops *ops,
>  	struct video_device *vfd = video_devdata(file);
>  	struct v4l2_control *p = arg;
>  	struct v4l2_fh *vfh =
> -		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> +		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
>  	struct v4l2_ext_controls ctrls;
>  	struct v4l2_ext_control ctrl;
>  	int ret;
> @@ -2414,7 +2424,7 @@ static int v4l_g_ext_ctrls(const struct v4l2_ioctl_ops *ops,
>  	struct video_device *vfd = video_devdata(file);
>  	struct v4l2_ext_controls *p = arg;
>  	struct v4l2_fh *vfh =
> -		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> +		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
>
>  	p->error_idx = p->count;
>  	if (vfh && vfh->ctrl_handler)
> @@ -2435,7 +2445,7 @@ static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops,
>  	struct video_device *vfd = video_devdata(file);
>  	struct v4l2_ext_controls *p = arg;
>  	struct v4l2_fh *vfh =
> -		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> +		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
>
>  	p->error_idx = p->count;
>  	if (vfh && vfh->ctrl_handler)
> @@ -2456,7 +2466,7 @@ static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops,
>  	struct video_device *vfd = video_devdata(file);
>  	struct v4l2_ext_controls *p = arg;
>  	struct v4l2_fh *vfh =
> -		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> +		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
>
>  	p->error_idx = p->count;
>  	if (vfh && vfh->ctrl_handler)
> @@ -3057,6 +3067,21 @@ void v4l_printk_ioctl(const char *prefix, unsigned int cmd)
>  }
>  EXPORT_SYMBOL(v4l_printk_ioctl);
>
> +static struct video_device_state *
> +video_device_get_state(struct video_device *vfd, struct v4l2_fh *vfh,
> +		       unsigned int cmd, void *arg)
> +{
> +	switch (cmd) {
> +	default:
> +		return NULL;
> +	case VIDIOC_G_FMT:
> +	case VIDIOC_S_FMT:
> +		return &vfd->state;
> +	case VIDIOC_TRY_FMT:
> +		return &vfh->state;
> +	}
> +}
> +
>  static long __video_do_ioctl(struct file *file,
>  		unsigned int cmd, void *arg)
>  {
> @@ -3081,6 +3106,9 @@ static long __video_do_ioctl(struct file *file,
>  	if (test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags))
>  		vfh = file->private_data;
>
> +	if (vfh && test_bit(V4L2_FL_USES_STATE, &vfd->flags))
> +		fh = video_device_get_state(vfd, vfh, cmd, arg);
> +
>  	/*
>  	 * We need to serialize streamon/off with queueing new requests.
>  	 * These ioctls may trigger the cancellation of a streaming
> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> index 1b6222fab24eda96cbe459b435431c01f7259366..8e6e7799212cd07ae4ad3dfc85912c21a9bcab2d 100644
> --- a/include/media/v4l2-dev.h
> +++ b/include/media/v4l2-dev.h
> @@ -89,12 +89,18 @@ struct dentry;
>   *	set by the core when the sub-devices device nodes are registered with
>   *	v4l2_device_register_ro_subdev_nodes() and used by the sub-device ioctl
>   *	handler to restrict access to some ioctl calls.
> + * @V4L2_FL_USES_STATE:
> + *	indicates that the &struct video_device has state support.
> + *	The active video and metadata formats are stored in video_device.state,
> + *	and the try video and metadata formats are stored in v4l2_fh.state.
> + *	All new drivers should use it.
>   */
>  enum v4l2_video_device_flags {
>  	V4L2_FL_REGISTERED		= 0,
>  	V4L2_FL_USES_V4L2_FH		= 1,
>  	V4L2_FL_QUIRK_INVERTED_CROP	= 2,
>  	V4L2_FL_SUBDEV_RO_DEVNODE	= 3,
> +	V4L2_FL_USES_STATE		= 4,
>  };
>
>  /* Priority helper functions */
> @@ -214,6 +220,30 @@ struct v4l2_file_operations {
>  	int (*release) (struct file *);
>  };
>
> +/**
> + * enum video_device_format_whence - Video device format type
> + *
> + * @V4L2_DEVICE_FORMAT_TRY: from VIDIOC_TRY_FMT, for negotiation only
> + * @V4L2_DEVICE_FORMAT_ACTIVE: from VIDIOC_S_FMT, applied to the device
> + */
> +enum video_device_format_whence {
> +	VIDEO_DEVICE_FORMAT_TRY = 0,
> +	VIDEO_DEVICE_FORMAT_ACTIVE = 1,
> +};

I'm not sure we need these. More on this on the drivers
implementation in the next patches.

> +
> +/**
> + * struct video_device_state - Used for storing video device state information.
> + *
> + * @vid_fmt: Format of the video capture stream
> + * @meta_fmt: Format of the metadata capture stream
> + * @which: is this a TRY or ACTIVE format?
> + */
> +struct video_device_state {
> +	struct v4l2_format vid_fmt;
> +	struct v4l2_format meta_fmt;
> +	enum video_device_format_whence which;
> +};
> +
>  /*
>   * Newer version of video_device, handled by videodev2.c
>   *	This version moves redundant code from video device code to
> @@ -238,6 +268,7 @@ struct v4l2_file_operations {
>   * @queue: &struct vb2_queue associated with this device node. May be NULL.
>   * @prio: pointer to &struct v4l2_prio_state with device's Priority state.
>   *	 If NULL, then v4l2_dev->prio will be used.
> + * @state: &struct video_device_state, holds the active state for the device.
>   * @name: video device name
>   * @vfl_type: V4L device type, as defined by &enum vfl_devnode_type
>   * @vfl_dir: V4L receiver, transmitter or m2m
> @@ -283,6 +314,7 @@ struct video_device {
>  	struct vb2_queue *queue;
>
>  	struct v4l2_prio_state *prio;
> +	struct video_device_state state;

One of the key design requirement it's the ability for drivers to
sub-class video_device_state. One possibile way to obtain this is to
dynamically allocate the state either by deferring to the driver's the
allocation (so that they can allocate a bigger structure) or by
passing to the framework the size it has to allocate.

In any case, I'm afraid the state should be allocated dynamically,
either in the drivers' init_state() (or similar) callback or by the
framework with a size hint from the driver.

What do you think ?

>
>  	/* device info */
>  	char name[64];
> @@ -540,6 +572,26 @@ static inline int video_is_registered(struct video_device *vdev)
>  	return test_bit(V4L2_FL_REGISTERED, &vdev->flags);
>  }
>
> +/**
> + * video_device_g_fmt_vid() - fill video v4l2_format from the state.
> + *
> + * @file: pointer to struct file
> + * @state: pointer to video device state
> + * @format: pointer to &struct v4l2_format
> + */
> +int video_device_g_fmt_vid(struct file *file, void *state,
> +			   struct v4l2_format *format);
> +
> +/**
> + * video_device_g_fmt_meta() - fill metadata v4l2_format from the state.
> + *
> + * @file: pointer to struct file
> + * @state: pointer to video device state
> + * @format: pointer to &struct v4l2_format
> + */
> +int video_device_g_fmt_meta(struct file *file, void *state,
> +			    struct v4l2_format *format);
> +
>  /**
>   * v4l2_debugfs_root - returns the dentry of the top-level "v4l2" debugfs dir
>   *
> diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
> index b5b3e00c8e6a0b082d9cd8a0c972a5094adcb6f2..02579f87ba99d0c849a0865f8cc4295446c39f94 100644
> --- a/include/media/v4l2-fh.h
> +++ b/include/media/v4l2-fh.h
> @@ -18,7 +18,8 @@
>  #include <linux/list.h>
>  #include <linux/videodev2.h>
>
> -struct video_device;
> +#include <media/v4l2-dev.h>
> +
>  struct v4l2_ctrl_handler;
>
>  /**
> @@ -28,6 +29,7 @@ struct v4l2_ctrl_handler;
>   * @vdev: pointer to &struct video_device
>   * @ctrl_handler: pointer to &struct v4l2_ctrl_handler
>   * @prio: priority of the file handler, as defined by &enum v4l2_priority
> + * @state: try state used for format negotiation on the video device
>   *
>   * @wait: event' s wait queue
>   * @subscribe_lock: serialise changes to the subscribed list; guarantee that
> @@ -44,6 +46,7 @@ struct v4l2_fh {
>  	struct video_device	*vdev;
>  	struct v4l2_ctrl_handler *ctrl_handler;
>  	enum v4l2_priority	prio;
> +	struct video_device_state state;
>
>  	/* Events */
>  	wait_queue_head_t	wait;
>
> --
> 2.49.0
>
>

Re: [PATCH RFC 1/3] media: v4l2-core: Add support for video device state

[Jai Luthra]

Hi Jacopo,

Thanks for the review.

Quoting Jacopo Mondi (2025-07-08 09:26:29)
> Hi Jai
> 
> On Thu, Jul 03, 2025 at 06:02:08PM -0700, Jai Luthra wrote:
> > Simplify video capture device drivers by maintaining active and try
> > states to track the v4l2 formats (for video and metadata capture) of the
> > device.
> >
> > A lot of boilerplate in the drivers can be reduced by combining the
> > implementation of s_fmt and try_fmt hooks, and using a framework helper
> > for the g_fmt hook.
> >
> > To achieve this, we pass the newly introduced state structure to the
> > hooks through the already existing private void pointer. For S_FMT, we
> > pass the pointer to the active state and enforce that the vb2 queue is
> > not busy before calling the driver hook. For TRY_FMT, we pass the
> > pointer to the temporary state stored in the file handle. Finally, we
> > introduce a framework helper for the g_fmt hook that the drivers can
> > use.
> >
> > The private void pointer argument already had some rare uses, so we
> > switch away from using it in the v4l_*ctrl functions to access
> > file->private_data, instead doing that access directly. Some drivers'
> > hooks might still expect it to point to file->private_data, so we
> > replace it with the state pointer only if a driver selects the
> > V4L2_FL_USES_STATE flag while registering the device.
> >
> > State support may be extended in the future to other device types, such
> > as video/metadata output or M2M devices.
> >
> > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > ---
> >  drivers/media/v4l2-core/v4l2-dev.c   | 32 ++++++++++++++++++++++
> >  drivers/media/v4l2-core/v4l2-fh.c    |  1 +
> >  drivers/media/v4l2-core/v4l2-ioctl.c | 44 ++++++++++++++++++++++++------
> >  include/media/v4l2-dev.h             | 52 ++++++++++++++++++++++++++++++++++++
> >  include/media/v4l2-fh.h              |  5 +++-
> >  5 files changed, 125 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> > index c369235113d98ae26c30a1aa386e7d60d541a66e..b8227d5508dc5bd775706264739e5db2d577f7fd 100644
> > --- a/drivers/media/v4l2-core/v4l2-dev.c
> > +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > @@ -27,6 +27,7 @@
> >  #include <linux/uaccess.h>
> >
> >  #include <media/v4l2-common.h>
> > +#include <media/v4l2-dev.h>
> 
> v4l2-common includes this already
> 

Ah okay, somehow clangd wasn't happy with it, will drop this in next revision.

> 
> >  #include <media/v4l2-device.h>
> >  #include <media/v4l2-ioctl.h>
> >  #include <media/v4l2-event.h>
> > @@ -163,6 +164,34 @@ void video_device_release_empty(struct video_device *vdev)
> >  }
> >  EXPORT_SYMBOL(video_device_release_empty);
> >
> > +int video_device_g_fmt_vid(struct file *file, void *priv,
> 
> The function prototype (and documentation) names the second parameter
> state.
> 

Will fix.

> > +                        struct v4l2_format *fmt)
> > +{
> > +     struct video_device_state *state = priv;
> > +
> > +     if (WARN_ON_ONCE(!state))
> > +             return -EINVAL;
> > +
> > +     *fmt = state->vid_fmt;
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL(video_device_g_fmt_vid);
> > +
> > +int video_device_g_fmt_meta(struct file *file, void *priv,
> > +                         struct v4l2_format *fmt)
> > +{
> > +     struct video_device_state *state = priv;
> > +
> > +     if (WARN_ON_ONCE(!state))
> > +             return -EINVAL;
> > +
> > +     *fmt = state->meta_fmt;
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL(video_device_g_fmt_meta);
> > +
> 
> These two helpers, and the presence of two struct v4l2_format
> 'vid_fmt' and 'meta_fmt' are a bit puzzling to me.
> 
> A video device will handle one buffer type and struct v4l2_format
> accomodates all type of buffers in the 'fmt' union member.
> 
> Why do you store two of them in the video device state ?
> 

I stored both as I was also looking at RPi CFE video nodes, which supports
both video and metadata capture in a single video device.

These may explode even more in future for some M2M device with video +
metdata. We will have to store and provide helpers for 4 combinations then,
of capture & output formats for both video & metadata usecases.

> >  static inline void video_get(struct video_device *vdev)
> >  {
> >       get_device(&vdev->dev);
> > @@ -927,6 +956,9 @@ int __video_register_device(struct video_device *vdev,
> >       spin_lock_init(&vdev->fh_lock);
> >       INIT_LIST_HEAD(&vdev->fh_list);
> >
> > +     /* video_device_state support */
> > +     vdev->state.which = VIDEO_DEVICE_FORMAT_ACTIVE;
> > +
> >       /* Part 1: check device type */
> >       switch (type) {
> >       case VFL_TYPE_VIDEO:
> > diff --git a/drivers/media/v4l2-core/v4l2-fh.c b/drivers/media/v4l2-core/v4l2-fh.c
> > index 90eec79ee995a2d214590beeacc91b9f8f33236d..d246e05f8ef1244e212412caa5c9c6788a5c948a 100644
> > --- a/drivers/media/v4l2-core/v4l2-fh.c
> > +++ b/drivers/media/v4l2-core/v4l2-fh.c
> > @@ -37,6 +37,7 @@ void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev)
> >       INIT_LIST_HEAD(&fh->available);
> >       INIT_LIST_HEAD(&fh->subscribed);
> >       fh->sequence = -1;
> > +     fh->state.which = VIDEO_DEVICE_FORMAT_TRY;
> >       mutex_init(&fh->subscribe_lock);
> >  }
> >  EXPORT_SYMBOL_GPL(v4l2_fh_init);
> > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> > index 650dc1956f73d2f1943b56c42140c7b8d757259f..78a0db364725ec6641be37d0c4804edb222a9154 100644
> > --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> > @@ -21,6 +21,7 @@
> >
> >  #include <media/media-device.h> /* for media_set_bus_info() */
> >  #include <media/v4l2-common.h>
> > +#include <media/v4l2-dev.h>
> >  #include <media/v4l2-ioctl.h>
> >  #include <media/v4l2-ctrls.h>
> >  #include <media/v4l2-fh.h>
> > @@ -1745,6 +1746,15 @@ static int v4l_s_fmt(const struct v4l2_ioctl_ops *ops,
> >       if (ret)
> >               return ret;
> >
> > +     /*
> > +      * Make sure queue isn't busy for devices that use state, as they have a
> > +      * single implementation for .s_fmt and .try_fmt, and rely on us to make
> > +      * sure the queue is not busy when calling for the .s_fmt case
> > +      */
> > +     if (test_bit(V4L2_FL_USES_STATE, &vfd->flags) && vfd->queue &&
> > +         vb2_is_busy(vfd->queue))
> > +             return -EBUSY;
> > +
> >       ret = v4l_enable_media_source(vfd);
> >       if (ret)
> >               return ret;
> > @@ -2293,7 +2303,7 @@ static int v4l_queryctrl(const struct v4l2_ioctl_ops *ops,
> >       struct v4l2_query_ext_ctrl qec = {};
> >       struct v4l2_queryctrl *p = arg;
> >       struct v4l2_fh *vfh =
> > -             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> > +             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
> >       int ret;
> >
> >       if (vfh && vfh->ctrl_handler)
> > @@ -2318,7 +2328,7 @@ static int v4l_query_ext_ctrl(const struct v4l2_ioctl_ops *ops,
> >       struct video_device *vfd = video_devdata(file);
> >       struct v4l2_query_ext_ctrl *p = arg;
> >       struct v4l2_fh *vfh =
> > -             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> > +             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
> >
> >       if (vfh && vfh->ctrl_handler)
> >               return v4l2_query_ext_ctrl(vfh->ctrl_handler, p);
> > @@ -2335,7 +2345,7 @@ static int v4l_querymenu(const struct v4l2_ioctl_ops *ops,
> >       struct video_device *vfd = video_devdata(file);
> >       struct v4l2_querymenu *p = arg;
> >       struct v4l2_fh *vfh =
> > -             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> > +             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
> >
> >       if (vfh && vfh->ctrl_handler)
> >               return v4l2_querymenu(vfh->ctrl_handler, p);
> > @@ -2352,7 +2362,7 @@ static int v4l_g_ctrl(const struct v4l2_ioctl_ops *ops,
> >       struct video_device *vfd = video_devdata(file);
> >       struct v4l2_control *p = arg;
> >       struct v4l2_fh *vfh =
> > -             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> > +             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
> >       struct v4l2_ext_controls ctrls;
> >       struct v4l2_ext_control ctrl;
> >
> > @@ -2384,7 +2394,7 @@ static int v4l_s_ctrl(const struct v4l2_ioctl_ops *ops,
> >       struct video_device *vfd = video_devdata(file);
> >       struct v4l2_control *p = arg;
> >       struct v4l2_fh *vfh =
> > -             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> > +             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
> >       struct v4l2_ext_controls ctrls;
> >       struct v4l2_ext_control ctrl;
> >       int ret;
> > @@ -2414,7 +2424,7 @@ static int v4l_g_ext_ctrls(const struct v4l2_ioctl_ops *ops,
> >       struct video_device *vfd = video_devdata(file);
> >       struct v4l2_ext_controls *p = arg;
> >       struct v4l2_fh *vfh =
> > -             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> > +             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
> >
> >       p->error_idx = p->count;
> >       if (vfh && vfh->ctrl_handler)
> > @@ -2435,7 +2445,7 @@ static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops,
> >       struct video_device *vfd = video_devdata(file);
> >       struct v4l2_ext_controls *p = arg;
> >       struct v4l2_fh *vfh =
> > -             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> > +             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
> >
> >       p->error_idx = p->count;
> >       if (vfh && vfh->ctrl_handler)
> > @@ -2456,7 +2466,7 @@ static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops,
> >       struct video_device *vfd = video_devdata(file);
> >       struct v4l2_ext_controls *p = arg;
> >       struct v4l2_fh *vfh =
> > -             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> > +             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
> >
> >       p->error_idx = p->count;
> >       if (vfh && vfh->ctrl_handler)
> > @@ -3057,6 +3067,21 @@ void v4l_printk_ioctl(const char *prefix, unsigned int cmd)
> >  }
> >  EXPORT_SYMBOL(v4l_printk_ioctl);
> >
> > +static struct video_device_state *
> > +video_device_get_state(struct video_device *vfd, struct v4l2_fh *vfh,
> > +                    unsigned int cmd, void *arg)
> > +{
> > +     switch (cmd) {
> > +     default:
> > +             return NULL;
> > +     case VIDIOC_G_FMT:
> > +     case VIDIOC_S_FMT:
> > +             return &vfd->state;
> > +     case VIDIOC_TRY_FMT:
> > +             return &vfh->state;
> > +     }
> > +}
> > +
> >  static long __video_do_ioctl(struct file *file,
> >               unsigned int cmd, void *arg)
> >  {
> > @@ -3081,6 +3106,9 @@ static long __video_do_ioctl(struct file *file,
> >       if (test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags))
> >               vfh = file->private_data;
> >
> > +     if (vfh && test_bit(V4L2_FL_USES_STATE, &vfd->flags))
> > +             fh = video_device_get_state(vfd, vfh, cmd, arg);
> > +
> >       /*
> >        * We need to serialize streamon/off with queueing new requests.
> >        * These ioctls may trigger the cancellation of a streaming
> > diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> > index 1b6222fab24eda96cbe459b435431c01f7259366..8e6e7799212cd07ae4ad3dfc85912c21a9bcab2d 100644
> > --- a/include/media/v4l2-dev.h
> > +++ b/include/media/v4l2-dev.h
> > @@ -89,12 +89,18 @@ struct dentry;
> >   *   set by the core when the sub-devices device nodes are registered with
> >   *   v4l2_device_register_ro_subdev_nodes() and used by the sub-device ioctl
> >   *   handler to restrict access to some ioctl calls.
> > + * @V4L2_FL_USES_STATE:
> > + *   indicates that the &struct video_device has state support.
> > + *   The active video and metadata formats are stored in video_device.state,
> > + *   and the try video and metadata formats are stored in v4l2_fh.state.
> > + *   All new drivers should use it.
> >   */
> >  enum v4l2_video_device_flags {
> >       V4L2_FL_REGISTERED              = 0,
> >       V4L2_FL_USES_V4L2_FH            = 1,
> >       V4L2_FL_QUIRK_INVERTED_CROP     = 2,
> >       V4L2_FL_SUBDEV_RO_DEVNODE       = 3,
> > +     V4L2_FL_USES_STATE              = 4,
> >  };
> >
> >  /* Priority helper functions */
> > @@ -214,6 +220,30 @@ struct v4l2_file_operations {
> >       int (*release) (struct file *);
> >  };
> >
> > +/**
> > + * enum video_device_format_whence - Video device format type
> > + *
> > + * @V4L2_DEVICE_FORMAT_TRY: from VIDIOC_TRY_FMT, for negotiation only
> > + * @V4L2_DEVICE_FORMAT_ACTIVE: from VIDIOC_S_FMT, applied to the device
> > + */
> > +enum video_device_format_whence {
> > +     VIDEO_DEVICE_FORMAT_TRY = 0,
> > +     VIDEO_DEVICE_FORMAT_ACTIVE = 1,
> > +};
> 
> I'm not sure we need these. More on this on the drivers
> implementation in the next patches.
> 
> > +
> > +/**
> > + * struct video_device_state - Used for storing video device state information.
> > + *
> > + * @vid_fmt: Format of the video capture stream
> > + * @meta_fmt: Format of the metadata capture stream
> > + * @which: is this a TRY or ACTIVE format?
> > + */
> > +struct video_device_state {
> > +     struct v4l2_format vid_fmt;
> > +     struct v4l2_format meta_fmt;
> > +     enum video_device_format_whence which;
> > +};
> > +
> >  /*
> >   * Newer version of video_device, handled by videodev2.c
> >   *   This version moves redundant code from video device code to
> > @@ -238,6 +268,7 @@ struct v4l2_file_operations {
> >   * @queue: &struct vb2_queue associated with this device node. May be NULL.
> >   * @prio: pointer to &struct v4l2_prio_state with device's Priority state.
> >   *    If NULL, then v4l2_dev->prio will be used.
> > + * @state: &struct video_device_state, holds the active state for the device.
> >   * @name: video device name
> >   * @vfl_type: V4L device type, as defined by &enum vfl_devnode_type
> >   * @vfl_dir: V4L receiver, transmitter or m2m
> > @@ -283,6 +314,7 @@ struct video_device {
> >       struct vb2_queue *queue;
> >
> >       struct v4l2_prio_state *prio;
> > +     struct video_device_state state;
> 
> One of the key design requirement it's the ability for drivers to
> sub-class video_device_state. One possibile way to obtain this is to
> dynamically allocate the state either by deferring to the driver's the
> allocation (so that they can allocate a bigger structure) or by
> passing to the framework the size it has to allocate.
> 
> In any case, I'm afraid the state should be allocated dynamically,
> either in the drivers' init_state() (or similar) callback or by the
> framework with a size hint from the driver.
> 
> What do you think ?
> 

Ah okay, I missed that. Should be possible to make this dynamically
allocatable by the driver. It will also tie into Sakari's suggestion of
creating a helper for initializing the state.

> >
> >       /* device info */
> >       char name[64];
> > @@ -540,6 +572,26 @@ static inline int video_is_registered(struct video_device *vdev)
> >       return test_bit(V4L2_FL_REGISTERED, &vdev->flags);
> >  }
> >
> > +/**
> > + * video_device_g_fmt_vid() - fill video v4l2_format from the state.
> > + *
> > + * @file: pointer to struct file
> > + * @state: pointer to video device state
> > + * @format: pointer to &struct v4l2_format
> > + */
> > +int video_device_g_fmt_vid(struct file *file, void *state,
> > +                        struct v4l2_format *format);
> > +
> > +/**
> > + * video_device_g_fmt_meta() - fill metadata v4l2_format from the state.
> > + *
> > + * @file: pointer to struct file
> > + * @state: pointer to video device state
> > + * @format: pointer to &struct v4l2_format
> > + */
> > +int video_device_g_fmt_meta(struct file *file, void *state,
> > +                         struct v4l2_format *format);
> > +
> >  /**
> >   * v4l2_debugfs_root - returns the dentry of the top-level "v4l2" debugfs dir
> >   *
> > diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
> > index b5b3e00c8e6a0b082d9cd8a0c972a5094adcb6f2..02579f87ba99d0c849a0865f8cc4295446c39f94 100644
> > --- a/include/media/v4l2-fh.h
> > +++ b/include/media/v4l2-fh.h
> > @@ -18,7 +18,8 @@
> >  #include <linux/list.h>
> >  #include <linux/videodev2.h>
> >
> > -struct video_device;
> > +#include <media/v4l2-dev.h>
> > +
> >  struct v4l2_ctrl_handler;
> >
> >  /**
> > @@ -28,6 +29,7 @@ struct v4l2_ctrl_handler;
> >   * @vdev: pointer to &struct video_device
> >   * @ctrl_handler: pointer to &struct v4l2_ctrl_handler
> >   * @prio: priority of the file handler, as defined by &enum v4l2_priority
> > + * @state: try state used for format negotiation on the video device
> >   *
> >   * @wait: event' s wait queue
> >   * @subscribe_lock: serialise changes to the subscribed list; guarantee that
> > @@ -44,6 +46,7 @@ struct v4l2_fh {
> >       struct video_device     *vdev;
> >       struct v4l2_ctrl_handler *ctrl_handler;
> >       enum v4l2_priority      prio;
> > +     struct video_device_state state;
> >
> >       /* Events */
> >       wait_queue_head_t       wait;
> >
> > --
> > 2.49.0
> >
> >

Thanks,
Jai

Re: [PATCH RFC 1/3] media: v4l2-core: Add support for video device state

[Laurent Pinchart]

On Fri, Jul 11, 2025 at 04:54:08PM -0700, Jai Luthra wrote:
> Quoting Jacopo Mondi (2025-07-08 09:26:29)
> > On Thu, Jul 03, 2025 at 06:02:08PM -0700, Jai Luthra wrote:
> > > Simplify video capture device drivers by maintaining active and try
> > > states to track the v4l2 formats (for video and metadata capture) of the
> > > device.

I think you should split this patch in two, one introducing the active
state, and another one introducing the try state.

> > >
> > > A lot of boilerplate in the drivers can be reduced by combining the
> > > implementation of s_fmt and try_fmt hooks, and using a framework helper
> > > for the g_fmt hook.
> > >
> > > To achieve this, we pass the newly introduced state structure to the
> > > hooks through the already existing private void pointer. For S_FMT, we
> > > pass the pointer to the active state and enforce that the vb2 queue is
> > > not busy before calling the driver hook.

Is that the right thing to do ? The framework doesn't enforce that
constraint at the moment and lets drivers implement it. If there's no
good reason to do so, then the constraint should be enforced by the core
unconditionally, for all drivers (and that should be split to a patch of
its own). Otherwise, if there are use cases where a driver could
meaningfully handle VIDIOC_S_FMT while streaming, we should let drivers
enforce the constraint.

> > > For TRY_FMT, we pass the
> > > pointer to the temporary state stored in the file handle. Finally, we
> > > introduce a framework helper for the g_fmt hook that the drivers can
> > > use.
> > >
> > > The private void pointer argument already had some rare uses, so we
> > > switch away from using it in the v4l_*ctrl functions to access
> > > file->private_data, instead doing that access directly. Some drivers'
> > > hooks might still expect it to point to file->private_data, so we
> > > replace it with the state pointer only if a driver selects the
> > > V4L2_FL_USES_STATE flag while registering the device.
> > >
> > > State support may be extended in the future to other device types, such
> > > as video/metadata output or M2M devices.
> > >
> > > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > > ---
> > >  drivers/media/v4l2-core/v4l2-dev.c   | 32 ++++++++++++++++++++++
> > >  drivers/media/v4l2-core/v4l2-fh.c    |  1 +
> > >  drivers/media/v4l2-core/v4l2-ioctl.c | 44 ++++++++++++++++++++++++------
> > >  include/media/v4l2-dev.h             | 52 ++++++++++++++++++++++++++++++++++++
> > >  include/media/v4l2-fh.h              |  5 +++-
> > >  5 files changed, 125 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> > > index c369235113d98ae26c30a1aa386e7d60d541a66e..b8227d5508dc5bd775706264739e5db2d577f7fd 100644
> > > --- a/drivers/media/v4l2-core/v4l2-dev.c
> > > +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > > @@ -27,6 +27,7 @@
> > >  #include <linux/uaccess.h>
> > >
> > >  #include <media/v4l2-common.h>
> > > +#include <media/v4l2-dev.h>
> > 
> > v4l2-common includes this already
> 
> Ah okay, somehow clangd wasn't happy with it, will drop this in next revision.
> 
> > 
> > >  #include <media/v4l2-device.h>
> > >  #include <media/v4l2-ioctl.h>
> > >  #include <media/v4l2-event.h>
> > > @@ -163,6 +164,34 @@ void video_device_release_empty(struct video_device *vdev)
> > >  }
> > >  EXPORT_SYMBOL(video_device_release_empty);
> > >
> > > +int video_device_g_fmt_vid(struct file *file, void *priv,
> > 
> > The function prototype (and documentation) names the second parameter
> > state.
> > 
> 
> Will fix.
> 
> > > +                        struct v4l2_format *fmt)
> > > +{
> > > +     struct video_device_state *state = priv;
> > > +
> > > +     if (WARN_ON_ONCE(!state))
> > > +             return -EINVAL;
> > > +
> > > +     *fmt = state->vid_fmt;
> > > +
> > > +     return 0;
> > > +}
> > > +EXPORT_SYMBOL(video_device_g_fmt_vid);
> > > +
> > > +int video_device_g_fmt_meta(struct file *file, void *priv,
> > > +                         struct v4l2_format *fmt)
> > > +{
> > > +     struct video_device_state *state = priv;
> > > +
> > > +     if (WARN_ON_ONCE(!state))
> > > +             return -EINVAL;
> > > +
> > > +     *fmt = state->meta_fmt;
> > > +
> > > +     return 0;
> > > +}
> > > +EXPORT_SYMBOL(video_device_g_fmt_meta);
> > > +
> > 
> > These two helpers, and the presence of two struct v4l2_format
> > 'vid_fmt' and 'meta_fmt' are a bit puzzling to me.
> > 
> > A video device will handle one buffer type and struct v4l2_format
> > accomodates all type of buffers in the 'fmt' union member.
> > 
> > Why do you store two of them in the video device state ?
> 
> I stored both as I was also looking at RPi CFE video nodes, which supports
> both video and metadata capture in a single video device.

That's correct, but not at the same time. The driver should be
refactored to use a single v4l2_format, and we should have a single
video_device_g_fmt() helper.

> These may explode even more in future for some M2M device with video +
> metdata. We will have to store and provide helpers for 4 combinations then,
> of capture & output formats for both video & metadata usecases.

Note the rp1-cfe driver already supports both metadata capture and
metadata output.

> > >  static inline void video_get(struct video_device *vdev)
> > >  {
> > >       get_device(&vdev->dev);
> > > @@ -927,6 +956,9 @@ int __video_register_device(struct video_device *vdev,
> > >       spin_lock_init(&vdev->fh_lock);
> > >       INIT_LIST_HEAD(&vdev->fh_list);
> > >
> > > +     /* video_device_state support */
> > > +     vdev->state.which = VIDEO_DEVICE_FORMAT_ACTIVE;
> > > +
> > >       /* Part 1: check device type */
> > >       switch (type) {
> > >       case VFL_TYPE_VIDEO:
> > > diff --git a/drivers/media/v4l2-core/v4l2-fh.c b/drivers/media/v4l2-core/v4l2-fh.c
> > > index 90eec79ee995a2d214590beeacc91b9f8f33236d..d246e05f8ef1244e212412caa5c9c6788a5c948a 100644
> > > --- a/drivers/media/v4l2-core/v4l2-fh.c
> > > +++ b/drivers/media/v4l2-core/v4l2-fh.c
> > > @@ -37,6 +37,7 @@ void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev)
> > >       INIT_LIST_HEAD(&fh->available);
> > >       INIT_LIST_HEAD(&fh->subscribed);
> > >       fh->sequence = -1;
> > > +     fh->state.which = VIDEO_DEVICE_FORMAT_TRY;
> > >       mutex_init(&fh->subscribe_lock);
> > >  }
> > >  EXPORT_SYMBOL_GPL(v4l2_fh_init);
> > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> > > index 650dc1956f73d2f1943b56c42140c7b8d757259f..78a0db364725ec6641be37d0c4804edb222a9154 100644
> > > --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> > > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> > > @@ -21,6 +21,7 @@
> > >
> > >  #include <media/media-device.h> /* for media_set_bus_info() */
> > >  #include <media/v4l2-common.h>
> > > +#include <media/v4l2-dev.h>
> > >  #include <media/v4l2-ioctl.h>
> > >  #include <media/v4l2-ctrls.h>
> > >  #include <media/v4l2-fh.h>
> > > @@ -1745,6 +1746,15 @@ static int v4l_s_fmt(const struct v4l2_ioctl_ops *ops,
> > >       if (ret)
> > >               return ret;
> > >
> > > +     /*
> > > +      * Make sure queue isn't busy for devices that use state, as they have a
> > > +      * single implementation for .s_fmt and .try_fmt, and rely on us to make
> > > +      * sure the queue is not busy when calling for the .s_fmt case
> > > +      */
> > > +     if (test_bit(V4L2_FL_USES_STATE, &vfd->flags) && vfd->queue &&
> > > +         vb2_is_busy(vfd->queue))
> > > +             return -EBUSY;
> > > +
> > >       ret = v4l_enable_media_source(vfd);
> > >       if (ret)
> > >               return ret;
> > > @@ -2293,7 +2303,7 @@ static int v4l_queryctrl(const struct v4l2_ioctl_ops *ops,
> > >       struct v4l2_query_ext_ctrl qec = {};
> > >       struct v4l2_queryctrl *p = arg;
> > >       struct v4l2_fh *vfh =
> > > -             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> > > +             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;

This should be split to a separate patch with a clear commit message.

> > >       int ret;
> > >
> > >       if (vfh && vfh->ctrl_handler)
> > > @@ -2318,7 +2328,7 @@ static int v4l_query_ext_ctrl(const struct v4l2_ioctl_ops *ops,
> > >       struct video_device *vfd = video_devdata(file);
> > >       struct v4l2_query_ext_ctrl *p = arg;
> > >       struct v4l2_fh *vfh =
> > > -             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> > > +             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
> > >
> > >       if (vfh && vfh->ctrl_handler)
> > >               return v4l2_query_ext_ctrl(vfh->ctrl_handler, p);
> > > @@ -2335,7 +2345,7 @@ static int v4l_querymenu(const struct v4l2_ioctl_ops *ops,
> > >       struct video_device *vfd = video_devdata(file);
> > >       struct v4l2_querymenu *p = arg;
u> > >       struct v4l2_fh *vfh =
> > > -             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> > > +             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
> > >
> > >       if (vfh && vfh->ctrl_handler)
> > >               return v4l2_querymenu(vfh->ctrl_handler, p);
> > > @@ -2352,7 +2362,7 @@ static int v4l_g_ctrl(const struct v4l2_ioctl_ops *ops,
> > >       struct video_device *vfd = video_devdata(file);
> > >       struct v4l2_control *p = arg;
> > >       struct v4l2_fh *vfh =
> > > -             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> > > +             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
> > >       struct v4l2_ext_controls ctrls;
> > >       struct v4l2_ext_control ctrl;
> > >
> > > @@ -2384,7 +2394,7 @@ static int v4l_s_ctrl(const struct v4l2_ioctl_ops *ops,
> > >       struct video_device *vfd = video_devdata(file);
> > >       struct v4l2_control *p = arg;
> > >       struct v4l2_fh *vfh =
> > > -             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> > > +             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
> > >       struct v4l2_ext_controls ctrls;
> > >       struct v4l2_ext_control ctrl;
> > >       int ret;
> > > @@ -2414,7 +2424,7 @@ static int v4l_g_ext_ctrls(const struct v4l2_ioctl_ops *ops,
> > >       struct video_device *vfd = video_devdata(file);
> > >       struct v4l2_ext_controls *p = arg;
> > >       struct v4l2_fh *vfh =
> > > -             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> > > +             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
> > >
> > >       p->error_idx = p->count;
> > >       if (vfh && vfh->ctrl_handler)
> > > @@ -2435,7 +2445,7 @@ static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops,
> > >       struct video_device *vfd = video_devdata(file);
> > >       struct v4l2_ext_controls *p = arg;
> > >       struct v4l2_fh *vfh =
> > > -             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> > > +             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
> > >
> > >       p->error_idx = p->count;
> > >       if (vfh && vfh->ctrl_handler)
> > > @@ -2456,7 +2466,7 @@ static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops,
> > >       struct video_device *vfd = video_devdata(file);
> > >       struct v4l2_ext_controls *p = arg;
> > >       struct v4l2_fh *vfh =
> > > -             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> > > +             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
> > >
> > >       p->error_idx = p->count;
> > >       if (vfh && vfh->ctrl_handler)
> > > @@ -3057,6 +3067,21 @@ void v4l_printk_ioctl(const char *prefix, unsigned int cmd)
> > >  }
> > >  EXPORT_SYMBOL(v4l_printk_ioctl);
> > >
> > > +static struct video_device_state *
> > > +video_device_get_state(struct video_device *vfd, struct v4l2_fh *vfh,
> > > +                    unsigned int cmd, void *arg)
> > > +{
> > > +     switch (cmd) {
> > > +     default:
> > > +             return NULL;
> > > +     case VIDIOC_G_FMT:
> > > +     case VIDIOC_S_FMT:
> > > +             return &vfd->state;
> > > +     case VIDIOC_TRY_FMT:
> > > +             return &vfh->state;
> > > +     }
> > > +}
> > > +
> > >  static long __video_do_ioctl(struct file *file,
> > >               unsigned int cmd, void *arg)
> > >  {
> > > @@ -3081,6 +3106,9 @@ static long __video_do_ioctl(struct file *file,
> > >       if (test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags))
> > >               vfh = file->private_data;
> > >
> > > +     if (vfh && test_bit(V4L2_FL_USES_STATE, &vfd->flags))
> > > +             fh = video_device_get_state(vfd, vfh, cmd, arg);
> > > +

That's very dangerous, using the same void pointer for two different
purposes depending on the USES_STATE flag. I think you should start with
some refactoring to move away from using a void pointer.

The first step is to remove direct setting of file->private_data in
drivers. Quite a few do so because they need to allocate the structure
containing the v4l2_fh. They therefore can't call v4l2_fh_open(), which
sets file->private_data, but call v4l2_fh_init(), set the private_data
field, and then call v4l2_fh_add(). In many cases it seems
v4l2_fh_init() could set file->private_data. You will need to check all
drivers that set the field directly, see if any of them set
file->private_data to a value different than the v4l2_fh pointer, and if
that could be fixed.

(Note that some drivers set the private_data field of a debugfs file, or
an ALSA device. Don't mistakenly consider those as direct usage of
private_data conflicting with V4L2_FL_USES_V4L2_FH.)

If we're lucky, we'll be able to remove manual usage of private_data in
drivers. In that case, we could then either

- Patch ioctl handlers in drivers to use file->private_data to access
  the v4l2_fh and stop using the void *fh argument, and then replace the
  void *fh argument with a video_device_state *state.

- Add a video_device_state *state argument to the ioctl handlers, if we
  decide to keep the fh pointer. In that case we should replace void *fh
  with v4l2_fh *fh.

If there are drivers left that can't easily stop setting private_data
manually, let's discuss them.

> > >       /*
> > >        * We need to serialize streamon/off with queueing new requests.
> > >        * These ioctls may trigger the cancellation of a streaming
> > > diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> > > index 1b6222fab24eda96cbe459b435431c01f7259366..8e6e7799212cd07ae4ad3dfc85912c21a9bcab2d 100644
> > > --- a/include/media/v4l2-dev.h
> > > +++ b/include/media/v4l2-dev.h
> > > @@ -89,12 +89,18 @@ struct dentry;
> > >   *   set by the core when the sub-devices device nodes are registered with
> > >   *   v4l2_device_register_ro_subdev_nodes() and used by the sub-device ioctl
> > >   *   handler to restrict access to some ioctl calls.
> > > + * @V4L2_FL_USES_STATE:
> > > + *   indicates that the &struct video_device has state support.
> > > + *   The active video and metadata formats are stored in video_device.state,
> > > + *   and the try video and metadata formats are stored in v4l2_fh.state.
> > > + *   All new drivers should use it.
> > >   */
> > >  enum v4l2_video_device_flags {
> > >       V4L2_FL_REGISTERED              = 0,
> > >       V4L2_FL_USES_V4L2_FH            = 1,
> > >       V4L2_FL_QUIRK_INVERTED_CROP     = 2,
> > >       V4L2_FL_SUBDEV_RO_DEVNODE       = 3,
> > > +     V4L2_FL_USES_STATE              = 4,
> > >  };
> > >
> > >  /* Priority helper functions */
> > > @@ -214,6 +220,30 @@ struct v4l2_file_operations {
> > >       int (*release) (struct file *);
> > >  };
> > >
> > > +/**
> > > + * enum video_device_format_whence - Video device format type
> > > + *
> > > + * @V4L2_DEVICE_FORMAT_TRY: from VIDIOC_TRY_FMT, for negotiation only
> > > + * @V4L2_DEVICE_FORMAT_ACTIVE: from VIDIOC_S_FMT, applied to the device
> > > + */
> > > +enum video_device_format_whence {
> > > +     VIDEO_DEVICE_FORMAT_TRY = 0,
> > > +     VIDEO_DEVICE_FORMAT_ACTIVE = 1,
> > > +};
> > 
> > I'm not sure we need these. More on this on the drivers
> > implementation in the next patches.

I agree, this should not be needed at this point. The whole point of
states is that drivers should not care whether they're operating on a
TRY or ACTIVE state. There are exceptions with subdevs for historical
reasons, but we shouldn't repeat that here.

It may make sense to later add TRY/ACTIVE identifiers for the UAPI, but
within drivers they should not be used. How about repurposing the
.try_fmt() and .s_fmt() ioctl handlers for drivers support states, by
using .try_fmt() first to adjust the format and store it in the state
(ACTIVE or TRY, that shouldn't matter to drivers), and then using
.s_fmt() to apply the state to the device ? The V4L2 core should call
.try_fmt() first followed by .s_fmt() when V4L2_FL_USES_STATE is set.
The vast majority of state-aware drivers will configure the device when
starting streaming, so they won't need to implement .s_fmt().

Now that I wrote this, the plan may conflict with my comment above
regarding leaving the busy check in drivers. Let's figure that one
first, and see if we need to let drivers known on what state they're
operating. I think repurposing .try_fmt() and .s_fmt() should be done
regardless.

> > > +
> > > +/**
> > > + * struct video_device_state - Used for storing video device state information.
> > > + *
> > > + * @vid_fmt: Format of the video capture stream
> > > + * @meta_fmt: Format of the metadata capture stream
> > > + * @which: is this a TRY or ACTIVE format?
> > > + */
> > > +struct video_device_state {
> > > +     struct v4l2_format vid_fmt;
> > > +     struct v4l2_format meta_fmt;
> > > +     enum video_device_format_whence which;
> > > +};
> > > +
> > >  /*
> > >   * Newer version of video_device, handled by videodev2.c
> > >   *   This version moves redundant code from video device code to
> > > @@ -238,6 +268,7 @@ struct v4l2_file_operations {
> > >   * @queue: &struct vb2_queue associated with this device node. May be NULL.
> > >   * @prio: pointer to &struct v4l2_prio_state with device's Priority state.
> > >   *    If NULL, then v4l2_dev->prio will be used.
> > > + * @state: &struct video_device_state, holds the active state for the device.
> > >   * @name: video device name
> > >   * @vfl_type: V4L device type, as defined by &enum vfl_devnode_type
> > >   * @vfl_dir: V4L receiver, transmitter or m2m
> > > @@ -283,6 +314,7 @@ struct video_device {
> > >       struct vb2_queue *queue;
> > >
> > >       struct v4l2_prio_state *prio;
> > > +     struct video_device_state state;
> > 
> > One of the key design requirement it's the ability for drivers to
> > sub-class video_device_state. One possibile way to obtain this is to
> > dynamically allocate the state either by deferring to the driver's the
> > allocation (so that they can allocate a bigger structure) or by
> > passing to the framework the size it has to allocate.
> > 
> > In any case, I'm afraid the state should be allocated dynamically,
> > either in the drivers' init_state() (or similar) callback or by the
> > framework with a size hint from the driver.
> > 
> > What do you think ?
> 
> Ah okay, I missed that. Should be possible to make this dynamically
> allocatable by the driver. It will also tie into Sakari's suggestion of
> creating a helper for initializing the state.

Yes, I agree with Jacopo and Sakari here. The state should be
dynamically allocated, and you should add an operation to initialize it.

> > >
> > >       /* device info */
> > >       char name[64];
> > > @@ -540,6 +572,26 @@ static inline int video_is_registered(struct video_device *vdev)
> > >       return test_bit(V4L2_FL_REGISTERED, &vdev->flags);
> > >  }
> > >
> > > +/**
> > > + * video_device_g_fmt_vid() - fill video v4l2_format from the state.
> > > + *
> > > + * @file: pointer to struct file
> > > + * @state: pointer to video device state
> > > + * @format: pointer to &struct v4l2_format
> > > + */
> > > +int video_device_g_fmt_vid(struct file *file, void *state,
> > > +                        struct v4l2_format *format);
> > > +
> > > +/**
> > > + * video_device_g_fmt_meta() - fill metadata v4l2_format from the state.
> > > + *
> > > + * @file: pointer to struct file
> > > + * @state: pointer to video device state
> > > + * @format: pointer to &struct v4l2_format
> > > + */
> > > +int video_device_g_fmt_meta(struct file *file, void *state,
> > > +                         struct v4l2_format *format);
> > > +
> > >  /**
> > >   * v4l2_debugfs_root - returns the dentry of the top-level "v4l2" debugfs dir
> > >   *
> > > diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
> > > index b5b3e00c8e6a0b082d9cd8a0c972a5094adcb6f2..02579f87ba99d0c849a0865f8cc4295446c39f94 100644
> > > --- a/include/media/v4l2-fh.h
> > > +++ b/include/media/v4l2-fh.h
> > > @@ -18,7 +18,8 @@
> > >  #include <linux/list.h>
> > >  #include <linux/videodev2.h>
> > >
> > > -struct video_device;
> > > +#include <media/v4l2-dev.h>
> > > +

You will be able to go back to forward declarations once you replace the
state field below with a pointer.

> > >  struct v4l2_ctrl_handler;
> > >
> > >  /**
> > > @@ -28,6 +29,7 @@ struct v4l2_ctrl_handler;
> > >   * @vdev: pointer to &struct video_device
> > >   * @ctrl_handler: pointer to &struct v4l2_ctrl_handler
> > >   * @prio: priority of the file handler, as defined by &enum v4l2_priority
> > > + * @state: try state used for format negotiation on the video device
> > >   *
> > >   * @wait: event' s wait queue
> > >   * @subscribe_lock: serialise changes to the subscribed list; guarantee that
> > > @@ -44,6 +46,7 @@ struct v4l2_fh {
> > >       struct video_device     *vdev;
> > >       struct v4l2_ctrl_handler *ctrl_handler;
> > >       enum v4l2_priority      prio;
> > > +     struct video_device_state state;
> > >
> > >       /* Events */
> > >       wait_queue_head_t       wait;
> > >

-- 
Regards,

Laurent Pinchart

Re: [PATCH RFC 1/3] media: v4l2-core: Add support for video device state

[Jacopo Mondi]

Hi Laurent

On Mon, Jul 14, 2025 at 08:16:32PM +0300, Laurent Pinchart wrote:
> On Fri, Jul 11, 2025 at 04:54:08PM -0700, Jai Luthra wrote:
> > Quoting Jacopo Mondi (2025-07-08 09:26:29)

[snip]

> > > >  static long __video_do_ioctl(struct file *file,
> > > >               unsigned int cmd, void *arg)
> > > >  {
> > > > @@ -3081,6 +3106,9 @@ static long __video_do_ioctl(struct file *file,
> > > >       if (test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags))
> > > >               vfh = file->private_data;
> > > >
> > > > +     if (vfh && test_bit(V4L2_FL_USES_STATE, &vfd->flags))
> > > > +             fh = video_device_get_state(vfd, vfh, cmd, arg);
> > > > +
>
> That's very dangerous, using the same void pointer for two different
> purposes depending on the USES_STATE flag. I think you should start with
> some refactoring to move away from using a void pointer.
>
> The first step is to remove direct setting of file->private_data in
> drivers. Quite a few do so because they need to allocate the structure
> containing the v4l2_fh. They therefore can't call v4l2_fh_open(), which
> sets file->private_data, but call v4l2_fh_init(), set the private_data
> field, and then call v4l2_fh_add(). In many cases it seems
> v4l2_fh_init() could set file->private_data. You will need to check all
> drivers that set the field directly, see if any of them set
> file->private_data to a value different than the v4l2_fh pointer, and if
> that could be fixed.

Can I reverse it ?

drivers that sets file->private_date = vfh; should be checked to make
sure they do not retrieve 'vfh' from the 'void *' in their ioctl
handlers but rather access file->private_data directly.

Once all of them are checked, we can replace the 'void *' argument
with the video_device_state pointer. And yes, if we can have
v4l2_fh_init() set file->private_data to vfh that's certainly better
than having drivers doing that by hand.

But ...
>
> (Note that some drivers set the private_data field of a debugfs file, or

... some drivers assume something different is set in
file->private_data.

Again however, if in their ioctl handlers they retrieve it from
file->private_data and do not hard-cast the void * to whatever they
need, again the void * argument can safely be replaced by a pointer to
video_device_state.

> an ALSA device. Don't mistakenly consider those as direct usage of
> private_data conflicting with V4L2_FL_USES_V4L2_FH.)

I noticed, in example  drivers/media/platform/amphion/vpu_dbg.c

However I don't see it setting file->private_data but rather access
it in their debugfs handlers:

	struct seq_file *s = file->private_data;

Is this what you mean with

"Don't mistakenly consider those as direct usage of
 private_data conflicting with V4L2_FL_USES_V4L2_FH."

?

>
> If we're lucky, we'll be able to remove manual usage of private_data in
> drivers. In that case, we could then either
>
> - Patch ioctl handlers in drivers to use file->private_data to access
>   the v4l2_fh and stop using the void *fh argument, and then replace the
>   void *fh argument with a video_device_state *state.
>

This would be preferable imho

Thanks
  j


> - Add a video_device_state *state argument to the ioctl handlers, if we
>   decide to keep the fh pointer. In that case we should replace void *fh
>   with v4l2_fh *fh.
>
> If there are drivers left that can't easily stop setting private_data
> manually, let's discuss them.
>
> > > >       /*
> > > >        * We need to serialize streamon/off with queueing new requests.
> > > >        * These ioctls may trigger the cancellation of a streaming
> > > > diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> > > > index 1b6222fab24eda96cbe459b435431c01f7259366..8e6e7799212cd07ae4ad3dfc85912c21a9bcab2d 100644
> > > > --- a/include/media/v4l2-dev.h
> > > > +++ b/include/media/v4l2-dev.h
> > > > @@ -89,12 +89,18 @@ struct dentry;
> > > >   *   set by the core when the sub-devices device nodes are registered with
> > > >   *   v4l2_device_register_ro_subdev_nodes() and used by the sub-device ioctl
> > > >   *   handler to restrict access to some ioctl calls.
> > > > + * @V4L2_FL_USES_STATE:
> > > > + *   indicates that the &struct video_device has state support.
> > > > + *   The active video and metadata formats are stored in video_device.state,
> > > > + *   and the try video and metadata formats are stored in v4l2_fh.state.
> > > > + *   All new drivers should use it.
> > > >   */
> > > >  enum v4l2_video_device_flags {
> > > >       V4L2_FL_REGISTERED              = 0,
> > > >       V4L2_FL_USES_V4L2_FH            = 1,
> > > >       V4L2_FL_QUIRK_INVERTED_CROP     = 2,
> > > >       V4L2_FL_SUBDEV_RO_DEVNODE       = 3,
> > > > +     V4L2_FL_USES_STATE              = 4,
> > > >  };
> > > >
> > > >  /* Priority helper functions */
> > > > @@ -214,6 +220,30 @@ struct v4l2_file_operations {
> > > >       int (*release) (struct file *);
> > > >  };
> > > >
> > > > +/**
> > > > + * enum video_device_format_whence - Video device format type
> > > > + *
> > > > + * @V4L2_DEVICE_FORMAT_TRY: from VIDIOC_TRY_FMT, for negotiation only
> > > > + * @V4L2_DEVICE_FORMAT_ACTIVE: from VIDIOC_S_FMT, applied to the device
> > > > + */
> > > > +enum video_device_format_whence {
> > > > +     VIDEO_DEVICE_FORMAT_TRY = 0,
> > > > +     VIDEO_DEVICE_FORMAT_ACTIVE = 1,
> > > > +};
> > >
> > > I'm not sure we need these. More on this on the drivers
> > > implementation in the next patches.
>
> I agree, this should not be needed at this point. The whole point of
> states is that drivers should not care whether they're operating on a
> TRY or ACTIVE state. There are exceptions with subdevs for historical
> reasons, but we shouldn't repeat that here.
>
> It may make sense to later add TRY/ACTIVE identifiers for the UAPI, but
> within drivers they should not be used. How about repurposing the
> .try_fmt() and .s_fmt() ioctl handlers for drivers support states, by
> using .try_fmt() first to adjust the format and store it in the state
> (ACTIVE or TRY, that shouldn't matter to drivers), and then using
> .s_fmt() to apply the state to the device ? The V4L2 core should call
> .try_fmt() first followed by .s_fmt() when V4L2_FL_USES_STATE is set.
> The vast majority of state-aware drivers will configure the device when
> starting streaming, so they won't need to implement .s_fmt().
>
> Now that I wrote this, the plan may conflict with my comment above
> regarding leaving the busy check in drivers. Let's figure that one
> first, and see if we need to let drivers known on what state they're
> operating. I think repurposing .try_fmt() and .s_fmt() should be done
> regardless.
>
> > > > +
> > > > +/**
> > > > + * struct video_device_state - Used for storing video device state information.
> > > > + *
> > > > + * @vid_fmt: Format of the video capture stream
> > > > + * @meta_fmt: Format of the metadata capture stream
> > > > + * @which: is this a TRY or ACTIVE format?
> > > > + */
> > > > +struct video_device_state {
> > > > +     struct v4l2_format vid_fmt;
> > > > +     struct v4l2_format meta_fmt;
> > > > +     enum video_device_format_whence which;
> > > > +};
> > > > +
> > > >  /*
> > > >   * Newer version of video_device, handled by videodev2.c
> > > >   *   This version moves redundant code from video device code to
> > > > @@ -238,6 +268,7 @@ struct v4l2_file_operations {
> > > >   * @queue: &struct vb2_queue associated with this device node. May be NULL.
> > > >   * @prio: pointer to &struct v4l2_prio_state with device's Priority state.
> > > >   *    If NULL, then v4l2_dev->prio will be used.
> > > > + * @state: &struct video_device_state, holds the active state for the device.
> > > >   * @name: video device name
> > > >   * @vfl_type: V4L device type, as defined by &enum vfl_devnode_type
> > > >   * @vfl_dir: V4L receiver, transmitter or m2m
> > > > @@ -283,6 +314,7 @@ struct video_device {
> > > >       struct vb2_queue *queue;
> > > >
> > > >       struct v4l2_prio_state *prio;
> > > > +     struct video_device_state state;
> > >
> > > One of the key design requirement it's the ability for drivers to
> > > sub-class video_device_state. One possibile way to obtain this is to
> > > dynamically allocate the state either by deferring to the driver's the
> > > allocation (so that they can allocate a bigger structure) or by
> > > passing to the framework the size it has to allocate.
> > >
> > > In any case, I'm afraid the state should be allocated dynamically,
> > > either in the drivers' init_state() (or similar) callback or by the
> > > framework with a size hint from the driver.
> > >
> > > What do you think ?
> >
> > Ah okay, I missed that. Should be possible to make this dynamically
> > allocatable by the driver. It will also tie into Sakari's suggestion of
> > creating a helper for initializing the state.
>
> Yes, I agree with Jacopo and Sakari here. The state should be
> dynamically allocated, and you should add an operation to initialize it.
>
> > > >
> > > >       /* device info */
> > > >       char name[64];
> > > > @@ -540,6 +572,26 @@ static inline int video_is_registered(struct video_device *vdev)
> > > >       return test_bit(V4L2_FL_REGISTERED, &vdev->flags);
> > > >  }
> > > >
> > > > +/**
> > > > + * video_device_g_fmt_vid() - fill video v4l2_format from the state.
> > > > + *
> > > > + * @file: pointer to struct file
> > > > + * @state: pointer to video device state
> > > > + * @format: pointer to &struct v4l2_format
> > > > + */
> > > > +int video_device_g_fmt_vid(struct file *file, void *state,
> > > > +                        struct v4l2_format *format);
> > > > +
> > > > +/**
> > > > + * video_device_g_fmt_meta() - fill metadata v4l2_format from the state.
> > > > + *
> > > > + * @file: pointer to struct file
> > > > + * @state: pointer to video device state
> > > > + * @format: pointer to &struct v4l2_format
> > > > + */
> > > > +int video_device_g_fmt_meta(struct file *file, void *state,
> > > > +                         struct v4l2_format *format);
> > > > +
> > > >  /**
> > > >   * v4l2_debugfs_root - returns the dentry of the top-level "v4l2" debugfs dir
> > > >   *
> > > > diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
> > > > index b5b3e00c8e6a0b082d9cd8a0c972a5094adcb6f2..02579f87ba99d0c849a0865f8cc4295446c39f94 100644
> > > > --- a/include/media/v4l2-fh.h
> > > > +++ b/include/media/v4l2-fh.h
> > > > @@ -18,7 +18,8 @@
> > > >  #include <linux/list.h>
> > > >  #include <linux/videodev2.h>
> > > >
> > > > -struct video_device;
> > > > +#include <media/v4l2-dev.h>
> > > > +
>
> You will be able to go back to forward declarations once you replace the
> state field below with a pointer.
>
> > > >  struct v4l2_ctrl_handler;
> > > >
> > > >  /**
> > > > @@ -28,6 +29,7 @@ struct v4l2_ctrl_handler;
> > > >   * @vdev: pointer to &struct video_device
> > > >   * @ctrl_handler: pointer to &struct v4l2_ctrl_handler
> > > >   * @prio: priority of the file handler, as defined by &enum v4l2_priority
> > > > + * @state: try state used for format negotiation on the video device
> > > >   *
> > > >   * @wait: event' s wait queue
> > > >   * @subscribe_lock: serialise changes to the subscribed list; guarantee that
> > > > @@ -44,6 +46,7 @@ struct v4l2_fh {
> > > >       struct video_device     *vdev;
> > > >       struct v4l2_ctrl_handler *ctrl_handler;
> > > >       enum v4l2_priority      prio;
> > > > +     struct video_device_state state;
> > > >
> > > >       /* Events */
> > > >       wait_queue_head_t       wait;
> > > >
>
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH RFC 1/3] media: v4l2-core: Add support for video device state

[Laurent Pinchart]

On Tue, Jul 29, 2025 at 03:43:17PM +0200, Jacopo Mondi wrote:
> Hi Laurent
> 
> On Mon, Jul 14, 2025 at 08:16:32PM +0300, Laurent Pinchart wrote:
> > On Fri, Jul 11, 2025 at 04:54:08PM -0700, Jai Luthra wrote:
> > > Quoting Jacopo Mondi (2025-07-08 09:26:29)
> 
> [snip]
> 
> > > > >  static long __video_do_ioctl(struct file *file,
> > > > >               unsigned int cmd, void *arg)
> > > > >  {
> > > > > @@ -3081,6 +3106,9 @@ static long __video_do_ioctl(struct file *file,
> > > > >       if (test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags))
> > > > >               vfh = file->private_data;
> > > > >
> > > > > +     if (vfh && test_bit(V4L2_FL_USES_STATE, &vfd->flags))
> > > > > +             fh = video_device_get_state(vfd, vfh, cmd, arg);
> > > > > +
> >
> > That's very dangerous, using the same void pointer for two different
> > purposes depending on the USES_STATE flag. I think you should start with
> > some refactoring to move away from using a void pointer.
> >
> > The first step is to remove direct setting of file->private_data in
> > drivers. Quite a few do so because they need to allocate the structure
> > containing the v4l2_fh. They therefore can't call v4l2_fh_open(), which
> > sets file->private_data, but call v4l2_fh_init(), set the private_data
> > field, and then call v4l2_fh_add(). In many cases it seems
> > v4l2_fh_init() could set file->private_data. You will need to check all
> > drivers that set the field directly, see if any of them set
> > file->private_data to a value different than the v4l2_fh pointer, and if
> > that could be fixed.
> 
> Can I reverse it ?
> 
> drivers that sets file->private_date = vfh; should be checked to make
> sure they do not retrieve 'vfh' from the 'void *' in their ioctl
> handlers but rather access file->private_data directly.
> 
> Once all of them are checked, we can replace the 'void *' argument
> with the video_device_state pointer. And yes, if we can have
> v4l2_fh_init() set file->private_data to vfh that's certainly better
> than having drivers doing that by hand.
> 
> But ...
> 
> > (Note that some drivers set the private_data field of a debugfs file, or
> 
> ... some drivers assume something different is set in
> file->private_data.

I'm not sure about that. My comment was related to the fact that more
than a quick grep is needed. There are drivers that set the private_date
field of a debugfs file or an ALSA device. Grepping for private_data
will show those, but they're not a problem. What we can is only the
private_data field of the struct file associated with a video_device.

Can we start with an assessment of what existing drivers do ?

> Again however, if in their ioctl handlers they retrieve it from
> file->private_data and do not hard-cast the void * to whatever they
> need, again the void * argument can safely be replaced by a pointer to
> video_device_state.
> 
> > an ALSA device. Don't mistakenly consider those as direct usage of
> > private_data conflicting with V4L2_FL_USES_V4L2_FH.)
> 
> I noticed, in example  drivers/media/platform/amphion/vpu_dbg.c
> 
> However I don't see it setting file->private_data but rather access
> it in their debugfs handlers:
> 
> 	struct seq_file *s = file->private_data;
> 
> Is this what you mean with
> 
> "Don't mistakenly consider those as direct usage of
>  private_data conflicting with V4L2_FL_USES_V4L2_FH."
> 
> ?

Yes, this isn't a problem, as that struct file isn't related to the
video_device.

> > If we're lucky, we'll be able to remove manual usage of private_data in
> > drivers. In that case, we could then either
> >
> > - Patch ioctl handlers in drivers to use file->private_data to access
> >   the v4l2_fh and stop using the void *fh argument, and then replace the
> >   void *fh argument with a video_device_state *state.
> 
> This would be preferable imho
> 
> > - Add a video_device_state *state argument to the ioctl handlers, if we
> >   decide to keep the fh pointer. In that case we should replace void *fh
> >   with v4l2_fh *fh.
> >
> > If there are drivers left that can't easily stop setting private_data
> > manually, let's discuss them.
> >
> > > > >       /*
> > > > >        * We need to serialize streamon/off with queueing new requests.
> > > > >        * These ioctls may trigger the cancellation of a streaming
> > > > > diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> > > > > index 1b6222fab24eda96cbe459b435431c01f7259366..8e6e7799212cd07ae4ad3dfc85912c21a9bcab2d 100644
> > > > > --- a/include/media/v4l2-dev.h
> > > > > +++ b/include/media/v4l2-dev.h
> > > > > @@ -89,12 +89,18 @@ struct dentry;
> > > > >   *   set by the core when the sub-devices device nodes are registered with
> > > > >   *   v4l2_device_register_ro_subdev_nodes() and used by the sub-device ioctl
> > > > >   *   handler to restrict access to some ioctl calls.
> > > > > + * @V4L2_FL_USES_STATE:
> > > > > + *   indicates that the &struct video_device has state support.
> > > > > + *   The active video and metadata formats are stored in video_device.state,
> > > > > + *   and the try video and metadata formats are stored in v4l2_fh.state.
> > > > > + *   All new drivers should use it.
> > > > >   */
> > > > >  enum v4l2_video_device_flags {
> > > > >       V4L2_FL_REGISTERED              = 0,
> > > > >       V4L2_FL_USES_V4L2_FH            = 1,
> > > > >       V4L2_FL_QUIRK_INVERTED_CROP     = 2,
> > > > >       V4L2_FL_SUBDEV_RO_DEVNODE       = 3,
> > > > > +     V4L2_FL_USES_STATE              = 4,
> > > > >  };
> > > > >
> > > > >  /* Priority helper functions */
> > > > > @@ -214,6 +220,30 @@ struct v4l2_file_operations {
> > > > >       int (*release) (struct file *);
> > > > >  };
> > > > >
> > > > > +/**
> > > > > + * enum video_device_format_whence - Video device format type
> > > > > + *
> > > > > + * @V4L2_DEVICE_FORMAT_TRY: from VIDIOC_TRY_FMT, for negotiation only
> > > > > + * @V4L2_DEVICE_FORMAT_ACTIVE: from VIDIOC_S_FMT, applied to the device
> > > > > + */
> > > > > +enum video_device_format_whence {
> > > > > +     VIDEO_DEVICE_FORMAT_TRY = 0,
> > > > > +     VIDEO_DEVICE_FORMAT_ACTIVE = 1,
> > > > > +};
> > > >
> > > > I'm not sure we need these. More on this on the drivers
> > > > implementation in the next patches.
> >
> > I agree, this should not be needed at this point. The whole point of
> > states is that drivers should not care whether they're operating on a
> > TRY or ACTIVE state. There are exceptions with subdevs for historical
> > reasons, but we shouldn't repeat that here.
> >
> > It may make sense to later add TRY/ACTIVE identifiers for the UAPI, but
> > within drivers they should not be used. How about repurposing the
> > .try_fmt() and .s_fmt() ioctl handlers for drivers support states, by
> > using .try_fmt() first to adjust the format and store it in the state
> > (ACTIVE or TRY, that shouldn't matter to drivers), and then using
> > .s_fmt() to apply the state to the device ? The V4L2 core should call
> > .try_fmt() first followed by .s_fmt() when V4L2_FL_USES_STATE is set.
> > The vast majority of state-aware drivers will configure the device when
> > starting streaming, so they won't need to implement .s_fmt().
> >
> > Now that I wrote this, the plan may conflict with my comment above
> > regarding leaving the busy check in drivers. Let's figure that one
> > first, and see if we need to let drivers known on what state they're
> > operating. I think repurposing .try_fmt() and .s_fmt() should be done
> > regardless.
> >
> > > > > +
> > > > > +/**
> > > > > + * struct video_device_state - Used for storing video device state information.
> > > > > + *
> > > > > + * @vid_fmt: Format of the video capture stream
> > > > > + * @meta_fmt: Format of the metadata capture stream
> > > > > + * @which: is this a TRY or ACTIVE format?
> > > > > + */
> > > > > +struct video_device_state {
> > > > > +     struct v4l2_format vid_fmt;
> > > > > +     struct v4l2_format meta_fmt;
> > > > > +     enum video_device_format_whence which;
> > > > > +};
> > > > > +
> > > > >  /*
> > > > >   * Newer version of video_device, handled by videodev2.c
> > > > >   *   This version moves redundant code from video device code to
> > > > > @@ -238,6 +268,7 @@ struct v4l2_file_operations {
> > > > >   * @queue: &struct vb2_queue associated with this device node. May be NULL.
> > > > >   * @prio: pointer to &struct v4l2_prio_state with device's Priority state.
> > > > >   *    If NULL, then v4l2_dev->prio will be used.
> > > > > + * @state: &struct video_device_state, holds the active state for the device.
> > > > >   * @name: video device name
> > > > >   * @vfl_type: V4L device type, as defined by &enum vfl_devnode_type
> > > > >   * @vfl_dir: V4L receiver, transmitter or m2m
> > > > > @@ -283,6 +314,7 @@ struct video_device {
> > > > >       struct vb2_queue *queue;
> > > > >
> > > > >       struct v4l2_prio_state *prio;
> > > > > +     struct video_device_state state;
> > > >
> > > > One of the key design requirement it's the ability for drivers to
> > > > sub-class video_device_state. One possibile way to obtain this is to
> > > > dynamically allocate the state either by deferring to the driver's the
> > > > allocation (so that they can allocate a bigger structure) or by
> > > > passing to the framework the size it has to allocate.
> > > >
> > > > In any case, I'm afraid the state should be allocated dynamically,
> > > > either in the drivers' init_state() (or similar) callback or by the
> > > > framework with a size hint from the driver.
> > > >
> > > > What do you think ?
> > >
> > > Ah okay, I missed that. Should be possible to make this dynamically
> > > allocatable by the driver. It will also tie into Sakari's suggestion of
> > > creating a helper for initializing the state.
> >
> > Yes, I agree with Jacopo and Sakari here. The state should be
> > dynamically allocated, and you should add an operation to initialize it.
> >
> > > > >
> > > > >       /* device info */
> > > > >       char name[64];
> > > > > @@ -540,6 +572,26 @@ static inline int video_is_registered(struct video_device *vdev)
> > > > >       return test_bit(V4L2_FL_REGISTERED, &vdev->flags);
> > > > >  }
> > > > >
> > > > > +/**
> > > > > + * video_device_g_fmt_vid() - fill video v4l2_format from the state.
> > > > > + *
> > > > > + * @file: pointer to struct file
> > > > > + * @state: pointer to video device state
> > > > > + * @format: pointer to &struct v4l2_format
> > > > > + */
> > > > > +int video_device_g_fmt_vid(struct file *file, void *state,
> > > > > +                        struct v4l2_format *format);
> > > > > +
> > > > > +/**
> > > > > + * video_device_g_fmt_meta() - fill metadata v4l2_format from the state.
> > > > > + *
> > > > > + * @file: pointer to struct file
> > > > > + * @state: pointer to video device state
> > > > > + * @format: pointer to &struct v4l2_format
> > > > > + */
> > > > > +int video_device_g_fmt_meta(struct file *file, void *state,
> > > > > +                         struct v4l2_format *format);
> > > > > +
> > > > >  /**
> > > > >   * v4l2_debugfs_root - returns the dentry of the top-level "v4l2" debugfs dir
> > > > >   *
> > > > > diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
> > > > > index b5b3e00c8e6a0b082d9cd8a0c972a5094adcb6f2..02579f87ba99d0c849a0865f8cc4295446c39f94 100644
> > > > > --- a/include/media/v4l2-fh.h
> > > > > +++ b/include/media/v4l2-fh.h
> > > > > @@ -18,7 +18,8 @@
> > > > >  #include <linux/list.h>
> > > > >  #include <linux/videodev2.h>
> > > > >
> > > > > -struct video_device;
> > > > > +#include <media/v4l2-dev.h>
> > > > > +
> >
> > You will be able to go back to forward declarations once you replace the
> > state field below with a pointer.
> >
> > > > >  struct v4l2_ctrl_handler;
> > > > >
> > > > >  /**
> > > > > @@ -28,6 +29,7 @@ struct v4l2_ctrl_handler;
> > > > >   * @vdev: pointer to &struct video_device
> > > > >   * @ctrl_handler: pointer to &struct v4l2_ctrl_handler
> > > > >   * @prio: priority of the file handler, as defined by &enum v4l2_priority
> > > > > + * @state: try state used for format negotiation on the video device
> > > > >   *
> > > > >   * @wait: event' s wait queue
> > > > >   * @subscribe_lock: serialise changes to the subscribed list; guarantee that
> > > > > @@ -44,6 +46,7 @@ struct v4l2_fh {
> > > > >       struct video_device     *vdev;
> > > > >       struct v4l2_ctrl_handler *ctrl_handler;
> > > > >       enum v4l2_priority      prio;
> > > > > +     struct video_device_state state;
> > > > >
> > > > >       /* Events */
> > > > >       wait_queue_head_t       wait;
> > > > >

-- 
Regards,

Laurent Pinchart

Re: [PATCH RFC 1/3] media: v4l2-core: Add support for video device state

[Jai Luthra]

Hi Laurent,

Thanks for the review.

Quoting Laurent Pinchart (2025-07-14 22:46:32)
> On Fri, Jul 11, 2025 at 04:54:08PM -0700, Jai Luthra wrote:
> > Quoting Jacopo Mondi (2025-07-08 09:26:29)
> > > On Thu, Jul 03, 2025 at 06:02:08PM -0700, Jai Luthra wrote:
> > > > Simplify video capture device drivers by maintaining active and try
> > > > states to track the v4l2 formats (for video and metadata capture) of the
> > > > device.
> 
> I think you should split this patch in two, one introducing the active
> state, and another one introducing the try state.
> 

Ack.

> > > >
> > > > A lot of boilerplate in the drivers can be reduced by combining the
> > > > implementation of s_fmt and try_fmt hooks, and using a framework helper
> > > > for the g_fmt hook.
> > > >
> > > > To achieve this, we pass the newly introduced state structure to the
> > > > hooks through the already existing private void pointer. For S_FMT, we
> > > > pass the pointer to the active state and enforce that the vb2 queue is
> > > > not busy before calling the driver hook.
> 
> Is that the right thing to do ? The framework doesn't enforce that
> constraint at the moment and lets drivers implement it. If there's no
> good reason to do so, then the constraint should be enforced by the core
> unconditionally, for all drivers (and that should be split to a patch of
> its own). Otherwise, if there are use cases where a driver could
> meaningfully handle VIDIOC_S_FMT while streaming, we should let drivers
> enforce the constraint.
> 

I ran a crude script across drivers/media to check this, and about 40% of
the drivers don't have a vb2_is_busy() or vb2_is_streaming() check in their
s_fmt hooks, even though most of them use a vb2 queue:

https://paste.debian.net/1390097/

Out of those missing, quite a few are false negatives, as either their
hooks call a second function for the actual s_fmt operation, which does
have a vb2_is_(busy|streaming) check, or they don't allow setting formats
and reuse the g_fmt function.

The final list of drivers missing any kind of vb2 check is relatively
small:

bttv_s_fmt_vbi_cap (drivers/media/pci/bt8xx/bttv-vbi.c:261) # Has internal checks for EBUSY

cx18_s_fmt_vid_cap (drivers/media/pci/cx18/cx18-ioctl.c:121) # Has internal checks for EBUSY
cx18_s_fmt_vbi_cap (drivers/media/pci/cx18/cx18-ioctl.c:330) # Has internal checks for EBUSY
cx18_s_fmt_sliced_vbi_cap (drivers/media/pci/cx18/cx18-ioctl.c:360) # Has internal checks for EBUSY

cio2_v4l2_s_fmt (drivers/media/pci/intel/ipu3/ipu3-cio2.c:1126)

ivtv_s_fmt_vid_cap (drivers/media/pci/ivtv/ivtv-ioctl.c:567) # Has internal checks for EBUSY
ivtv_s_fmt_vbi_cap (drivers/media/pci/ivtv/ivtv-ioctl.c:598) # Has internal checks for EBUSY
ivtv_s_fmt_sliced_vbi_cap (drivers/media/pci/ivtv/ivtv-ioctl.c:610) # Has internal checks for EBUSY

ivtv_s_fmt_vid_out (drivers/media/pci/ivtv/ivtv-ioctl.c:629)
ivtv_s_fmt_vid_out_overlay (drivers/media/pci/ivtv/ivtv-ioctl.c:674)

empress_s_fmt_vid_cap (drivers/media/pci/saa7134/saa7134-empress.c:116)
saa7134_s_fmt_vid_cap (drivers/media/pci/saa7134/saa7134-video.c:1107)

tw68_s_fmt_vid_cap (drivers/media/pci/tw68/tw68-video.c:644)

zoran_s_fmt_vid_cap (drivers/media/pci/zoran/zoran_driver.c:510)

allegro_s_fmt_vid_out (drivers/media/platform/allegro-dvt/allegro-core.c:3395)

c3_isp_cap_s_fmt_mplane (drivers/media/platform/amlogic/c3/isp/c3-isp-capture.c:435)

wave5_vpu_dec_s_fmt_cap (drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c:548)
wave5_vpu_dec_s_fmt_out (drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c:679)
wave5_vpu_enc_s_fmt_cap (drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c:412)
wave5_vpu_enc_s_fmt_out (drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c:516)

isp_video_s_fmt_mplane (drivers/media/platform/samsung/exynos4-is/fimc-isp-video.c:414)

sun4i_csi_s_fmt_vid_cap (drivers/media/platform/sunxi/sun4i-csi/sun4i_v4l2.c:141)

vidioc_s_fmt_vid_overlay (drivers/media/platform/ti/omap/omap_vout.c:719)

isp_video_set_format (drivers/media/platform/ti/omap3isp/ispvideo.c:674)
isp_video_set_format (drivers/media/platform/ti/omap3isp/ispvideo.c:674)

vidioc_s_fmt_out_mplane (drivers/media/platform/verisilicon/hantro_v4l2.c:649) # Separate checks for is_streaming/is_busy depending upon encoder vs decoder
vidioc_s_fmt_cap_mplane (drivers/media/platform/verisilicon/hantro_v4l2.c:655) # Only encoder checks is_busy

viacam_s_fmt_vid_cap (drivers/media/platform/via/via-camera.c:872)

visl_s_fmt_vid_cap (drivers/media/test-drivers/visl/visl-video.c:492)

vivid_s_fmt_cap (drivers/media/test-drivers/vivid/vivid-core.c:545)
vivid_s_fmt_cap_mplane (drivers/media/test-drivers/vivid/vivid-core.c:575)
vidioc_s_fmt_vid_out (drivers/media/test-drivers/vim2m.c:1047)
vidioc_s_fmt_vid_out_mplane (drivers/media/test-drivers/vim2m.c:1071)
vidioc_s_fmt_vbi_cap (drivers/media/usb/cx231xx/cx231xx-video.c:1455)
vidioc_s_fmt_sliced_vbi_cap (drivers/media/test-drivers/vivid/vivid-vbi-cap.h:21)
vidioc_s_fmt_vbi_out (drivers/media/test-drivers/vivid/vivid-vbi-out.h:14)
vidioc_s_fmt_sliced_vbi_out (drivers/media/test-drivers/vivid/vivid-vbi-out.h:18)
vidioc_s_fmt_sdr_cap (drivers/media/test-drivers/vivid/vivid-sdr-cap.h:18)
vidioc_s_fmt_vid_out_overlay (drivers/media/test-drivers/vivid/vivid-vid-out.h:30)

vidioc_s_fmt_vbi_cap (drivers/media/usb/cx231xx/cx231xx-video.c:1455)

pvr2_s_fmt_vid_cap (drivers/media/usb/pvrusb2/pvrusb2-v4l2.c:453)


Quite a few of those are VBI or test drivers. But some relevant cases are
encoders/decoders or ISP drivers that may allow changing resolution or
format mid stream.

So I'm not sure if we can add a framework-level check for this. Let me know
what you think?

> > > > For TRY_FMT, we pass the
> > > > pointer to the temporary state stored in the file handle. Finally, we
> > > > introduce a framework helper for the g_fmt hook that the drivers can
> > > > use.
> > > >
> > > > The private void pointer argument already had some rare uses, so we
> > > > switch away from using it in the v4l_*ctrl functions to access
> > > > file->private_data, instead doing that access directly. Some drivers'
> > > > hooks might still expect it to point to file->private_data, so we
> > > > replace it with the state pointer only if a driver selects the
> > > > V4L2_FL_USES_STATE flag while registering the device.
> > > >
> > > > State support may be extended in the future to other device types, such
> > > > as video/metadata output or M2M devices.
> > > >
> > > > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > > > ---
> > > >  drivers/media/v4l2-core/v4l2-dev.c   | 32 ++++++++++++++++++++++
> > > >  drivers/media/v4l2-core/v4l2-fh.c    |  1 +
> > > >  drivers/media/v4l2-core/v4l2-ioctl.c | 44 ++++++++++++++++++++++++------
> > > >  include/media/v4l2-dev.h             | 52 ++++++++++++++++++++++++++++++++++++
> > > >  include/media/v4l2-fh.h              |  5 +++-
> > > >  5 files changed, 125 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> > > > index c369235113d98ae26c30a1aa386e7d60d541a66e..b8227d5508dc5bd775706264739e5db2d577f7fd 100644
> > > > --- a/drivers/media/v4l2-core/v4l2-dev.c
> > > > +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > > > @@ -27,6 +27,7 @@
> > > >  #include <linux/uaccess.h>
> > > >
> > > >  #include <media/v4l2-common.h>
> > > > +#include <media/v4l2-dev.h>
> > > 
> > > v4l2-common includes this already
> > 
> > Ah okay, somehow clangd wasn't happy with it, will drop this in next revision.
> > 
> > > 
> > > >  #include <media/v4l2-device.h>
> > > >  #include <media/v4l2-ioctl.h>
> > > >  #include <media/v4l2-event.h>
> > > > @@ -163,6 +164,34 @@ void video_device_release_empty(struct video_device *vdev)
> > > >  }
> > > >  EXPORT_SYMBOL(video_device_release_empty);
> > > >
> > > > +int video_device_g_fmt_vid(struct file *file, void *priv,
> > > 
> > > The function prototype (and documentation) names the second parameter
> > > state.
> > > 
> > 
> > Will fix.
> > 
> > > > +                        struct v4l2_format *fmt)
> > > > +{
> > > > +     struct video_device_state *state = priv;
> > > > +
> > > > +     if (WARN_ON_ONCE(!state))
> > > > +             return -EINVAL;
> > > > +
> > > > +     *fmt = state->vid_fmt;
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +EXPORT_SYMBOL(video_device_g_fmt_vid);
> > > > +
> > > > +int video_device_g_fmt_meta(struct file *file, void *priv,
> > > > +                         struct v4l2_format *fmt)
> > > > +{
> > > > +     struct video_device_state *state = priv;
> > > > +
> > > > +     if (WARN_ON_ONCE(!state))
> > > > +             return -EINVAL;
> > > > +
> > > > +     *fmt = state->meta_fmt;
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +EXPORT_SYMBOL(video_device_g_fmt_meta);
> > > > +
> > > 
> > > These two helpers, and the presence of two struct v4l2_format
> > > 'vid_fmt' and 'meta_fmt' are a bit puzzling to me.
> > > 
> > > A video device will handle one buffer type and struct v4l2_format
> > > accomodates all type of buffers in the 'fmt' union member.
> > > 
> > > Why do you store two of them in the video device state ?
> > 
> > I stored both as I was also looking at RPi CFE video nodes, which supports
> > both video and metadata capture in a single video device.
> 
> That's correct, but not at the same time. The driver should be
> refactored to use a single v4l2_format, and we should have a single
> video_device_g_fmt() helper.
> 
> > These may explode even more in future for some M2M device with video +
> > metdata. We will have to store and provide helpers for 4 combinations then,
> > of capture & output formats for both video & metadata usecases.
> 
> Note the rp1-cfe driver already supports both metadata capture and
> metadata output.
> 
> > > >  static inline void video_get(struct video_device *vdev)
> > > >  {
> > > >       get_device(&vdev->dev);
> > > > @@ -927,6 +956,9 @@ int __video_register_device(struct video_device *vdev,
> > > >       spin_lock_init(&vdev->fh_lock);
> > > >       INIT_LIST_HEAD(&vdev->fh_list);
> > > >
> > > > +     /* video_device_state support */
> > > > +     vdev->state.which = VIDEO_DEVICE_FORMAT_ACTIVE;
> > > > +
> > > >       /* Part 1: check device type */
> > > >       switch (type) {
> > > >       case VFL_TYPE_VIDEO:
> > > > diff --git a/drivers/media/v4l2-core/v4l2-fh.c b/drivers/media/v4l2-core/v4l2-fh.c
> > > > index 90eec79ee995a2d214590beeacc91b9f8f33236d..d246e05f8ef1244e212412caa5c9c6788a5c948a 100644
> > > > --- a/drivers/media/v4l2-core/v4l2-fh.c
> > > > +++ b/drivers/media/v4l2-core/v4l2-fh.c
> > > > @@ -37,6 +37,7 @@ void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev)
> > > >       INIT_LIST_HEAD(&fh->available);
> > > >       INIT_LIST_HEAD(&fh->subscribed);
> > > >       fh->sequence = -1;
> > > > +     fh->state.which = VIDEO_DEVICE_FORMAT_TRY;
> > > >       mutex_init(&fh->subscribe_lock);
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(v4l2_fh_init);
> > > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> > > > index 650dc1956f73d2f1943b56c42140c7b8d757259f..78a0db364725ec6641be37d0c4804edb222a9154 100644
> > > > --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> > > > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> > > > @@ -21,6 +21,7 @@
> > > >
> > > >  #include <media/media-device.h> /* for media_set_bus_info() */
> > > >  #include <media/v4l2-common.h>
> > > > +#include <media/v4l2-dev.h>
> > > >  #include <media/v4l2-ioctl.h>
> > > >  #include <media/v4l2-ctrls.h>
> > > >  #include <media/v4l2-fh.h>
> > > > @@ -1745,6 +1746,15 @@ static int v4l_s_fmt(const struct v4l2_ioctl_ops *ops,
> > > >       if (ret)
> > > >               return ret;
> > > >
> > > > +     /*
> > > > +      * Make sure queue isn't busy for devices that use state, as they have a
> > > > +      * single implementation for .s_fmt and .try_fmt, and rely on us to make
> > > > +      * sure the queue is not busy when calling for the .s_fmt case
> > > > +      */
> > > > +     if (test_bit(V4L2_FL_USES_STATE, &vfd->flags) && vfd->queue &&
> > > > +         vb2_is_busy(vfd->queue))
> > > > +             return -EBUSY;
> > > > +
> > > >       ret = v4l_enable_media_source(vfd);
> > > >       if (ret)
> > > >               return ret;
> > > > @@ -2293,7 +2303,7 @@ static int v4l_queryctrl(const struct v4l2_ioctl_ops *ops,
> > > >       struct v4l2_query_ext_ctrl qec = {};
> > > >       struct v4l2_queryctrl *p = arg;
> > > >       struct v4l2_fh *vfh =
> > > > -             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> > > > +             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
> 
> This should be split to a separate patch with a clear commit message.
> 
> > > >       int ret;
> > > >
> > > >       if (vfh && vfh->ctrl_handler)
> > > > @@ -2318,7 +2328,7 @@ static int v4l_query_ext_ctrl(const struct v4l2_ioctl_ops *ops,
> > > >       struct video_device *vfd = video_devdata(file);
> > > >       struct v4l2_query_ext_ctrl *p = arg;
> > > >       struct v4l2_fh *vfh =
> > > > -             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> > > > +             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
> > > >
> > > >       if (vfh && vfh->ctrl_handler)
> > > >               return v4l2_query_ext_ctrl(vfh->ctrl_handler, p);
> > > > @@ -2335,7 +2345,7 @@ static int v4l_querymenu(const struct v4l2_ioctl_ops *ops,
> > > >       struct video_device *vfd = video_devdata(file);
> > > >       struct v4l2_querymenu *p = arg;
> u> > >       struct v4l2_fh *vfh =
> > > > -             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> > > > +             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
> > > >
> > > >       if (vfh && vfh->ctrl_handler)
> > > >               return v4l2_querymenu(vfh->ctrl_handler, p);
> > > > @@ -2352,7 +2362,7 @@ static int v4l_g_ctrl(const struct v4l2_ioctl_ops *ops,
> > > >       struct video_device *vfd = video_devdata(file);
> > > >       struct v4l2_control *p = arg;
> > > >       struct v4l2_fh *vfh =
> > > > -             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> > > > +             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
> > > >       struct v4l2_ext_controls ctrls;
> > > >       struct v4l2_ext_control ctrl;
> > > >
> > > > @@ -2384,7 +2394,7 @@ static int v4l_s_ctrl(const struct v4l2_ioctl_ops *ops,
> > > >       struct video_device *vfd = video_devdata(file);
> > > >       struct v4l2_control *p = arg;
> > > >       struct v4l2_fh *vfh =
> > > > -             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> > > > +             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
> > > >       struct v4l2_ext_controls ctrls;
> > > >       struct v4l2_ext_control ctrl;
> > > >       int ret;
> > > > @@ -2414,7 +2424,7 @@ static int v4l_g_ext_ctrls(const struct v4l2_ioctl_ops *ops,
> > > >       struct video_device *vfd = video_devdata(file);
> > > >       struct v4l2_ext_controls *p = arg;
> > > >       struct v4l2_fh *vfh =
> > > > -             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> > > > +             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
> > > >
> > > >       p->error_idx = p->count;
> > > >       if (vfh && vfh->ctrl_handler)
> > > > @@ -2435,7 +2445,7 @@ static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops,
> > > >       struct video_device *vfd = video_devdata(file);
> > > >       struct v4l2_ext_controls *p = arg;
> > > >       struct v4l2_fh *vfh =
> > > > -             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> > > > +             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
> > > >
> > > >       p->error_idx = p->count;
> > > >       if (vfh && vfh->ctrl_handler)
> > > > @@ -2456,7 +2466,7 @@ static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops,
> > > >       struct video_device *vfd = video_devdata(file);
> > > >       struct v4l2_ext_controls *p = arg;
> > > >       struct v4l2_fh *vfh =
> > > > -             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> > > > +             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? file->private_data : NULL;
> > > >
> > > >       p->error_idx = p->count;
> > > >       if (vfh && vfh->ctrl_handler)
> > > > @@ -3057,6 +3067,21 @@ void v4l_printk_ioctl(const char *prefix, unsigned int cmd)
> > > >  }
> > > >  EXPORT_SYMBOL(v4l_printk_ioctl);
> > > >
> > > > +static struct video_device_state *
> > > > +video_device_get_state(struct video_device *vfd, struct v4l2_fh *vfh,
> > > > +                    unsigned int cmd, void *arg)
> > > > +{
> > > > +     switch (cmd) {
> > > > +     default:
> > > > +             return NULL;
> > > > +     case VIDIOC_G_FMT:
> > > > +     case VIDIOC_S_FMT:
> > > > +             return &vfd->state;
> > > > +     case VIDIOC_TRY_FMT:
> > > > +             return &vfh->state;
> > > > +     }
> > > > +}
> > > > +
> > > >  static long __video_do_ioctl(struct file *file,
> > > >               unsigned int cmd, void *arg)
> > > >  {
> > > > @@ -3081,6 +3106,9 @@ static long __video_do_ioctl(struct file *file,
> > > >       if (test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags))
> > > >               vfh = file->private_data;
> > > >
> > > > +     if (vfh && test_bit(V4L2_FL_USES_STATE, &vfd->flags))
> > > > +             fh = video_device_get_state(vfd, vfh, cmd, arg);
> > > > +
> 
> That's very dangerous, using the same void pointer for two different
> purposes depending on the USES_STATE flag. I think you should start with
> some refactoring to move away from using a void pointer.
> 
> The first step is to remove direct setting of file->private_data in
> drivers. Quite a few do so because they need to allocate the structure
> containing the v4l2_fh. They therefore can't call v4l2_fh_open(), which
> sets file->private_data, but call v4l2_fh_init(), set the private_data
> field, and then call v4l2_fh_add(). In many cases it seems
> v4l2_fh_init() could set file->private_data. You will need to check all
> drivers that set the field directly, see if any of them set
> file->private_data to a value different than the v4l2_fh pointer, and if
> that could be fixed.
> 
> (Note that some drivers set the private_data field of a debugfs file, or
> an ALSA device. Don't mistakenly consider those as direct usage of
> private_data conflicting with V4L2_FL_USES_V4L2_FH.)
> 
> If we're lucky, we'll be able to remove manual usage of private_data in
> drivers. In that case, we could then either
> 
> - Patch ioctl handlers in drivers to use file->private_data to access
>   the v4l2_fh and stop using the void *fh argument, and then replace the
>   void *fh argument with a video_device_state *state.
> 
> - Add a video_device_state *state argument to the ioctl handlers, if we
>   decide to keep the fh pointer. In that case we should replace void *fh
>   with v4l2_fh *fh.
> 
> If there are drivers left that can't easily stop setting private_data
> manually, let's discuss them.
> 
> > > >       /*
> > > >        * We need to serialize streamon/off with queueing new requests.
> > > >        * These ioctls may trigger the cancellation of a streaming
> > > > diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> > > > index 1b6222fab24eda96cbe459b435431c01f7259366..8e6e7799212cd07ae4ad3dfc85912c21a9bcab2d 100644
> > > > --- a/include/media/v4l2-dev.h
> > > > +++ b/include/media/v4l2-dev.h
> > > > @@ -89,12 +89,18 @@ struct dentry;
> > > >   *   set by the core when the sub-devices device nodes are registered with
> > > >   *   v4l2_device_register_ro_subdev_nodes() and used by the sub-device ioctl
> > > >   *   handler to restrict access to some ioctl calls.
> > > > + * @V4L2_FL_USES_STATE:
> > > > + *   indicates that the &struct video_device has state support.
> > > > + *   The active video and metadata formats are stored in video_device.state,
> > > > + *   and the try video and metadata formats are stored in v4l2_fh.state.
> > > > + *   All new drivers should use it.
> > > >   */
> > > >  enum v4l2_video_device_flags {
> > > >       V4L2_FL_REGISTERED              = 0,
> > > >       V4L2_FL_USES_V4L2_FH            = 1,
> > > >       V4L2_FL_QUIRK_INVERTED_CROP     = 2,
> > > >       V4L2_FL_SUBDEV_RO_DEVNODE       = 3,
> > > > +     V4L2_FL_USES_STATE              = 4,
> > > >  };
> > > >
> > > >  /* Priority helper functions */
> > > > @@ -214,6 +220,30 @@ struct v4l2_file_operations {
> > > >       int (*release) (struct file *);
> > > >  };
> > > >
> > > > +/**
> > > > + * enum video_device_format_whence - Video device format type
> > > > + *
> > > > + * @V4L2_DEVICE_FORMAT_TRY: from VIDIOC_TRY_FMT, for negotiation only
> > > > + * @V4L2_DEVICE_FORMAT_ACTIVE: from VIDIOC_S_FMT, applied to the device
> > > > + */
> > > > +enum video_device_format_whence {
> > > > +     VIDEO_DEVICE_FORMAT_TRY = 0,
> > > > +     VIDEO_DEVICE_FORMAT_ACTIVE = 1,
> > > > +};
> > > 
> > > I'm not sure we need these. More on this on the drivers
> > > implementation in the next patches.
> 
> I agree, this should not be needed at this point. The whole point of
> states is that drivers should not care whether they're operating on a
> TRY or ACTIVE state. There are exceptions with subdevs for historical
> reasons, but we shouldn't repeat that here.
> 
> It may make sense to later add TRY/ACTIVE identifiers for the UAPI, but
> within drivers they should not be used. How about repurposing the
> .try_fmt() and .s_fmt() ioctl handlers for drivers support states, by
> using .try_fmt() first to adjust the format and store it in the state
> (ACTIVE or TRY, that shouldn't matter to drivers), and then using
> .s_fmt() to apply the state to the device ? The V4L2 core should call
> .try_fmt() first followed by .s_fmt() when V4L2_FL_USES_STATE is set.
> The vast majority of state-aware drivers will configure the device when
> starting streaming, so they won't need to implement .s_fmt().
> 
> Now that I wrote this, the plan may conflict with my comment above
> regarding leaving the busy check in drivers. Let's figure that one
> first, and see if we need to let drivers known on what state they're
> operating. I think repurposing .try_fmt() and .s_fmt() should be done
> regardless.

I'm not sure splitting makes a lot of sense if we are leaving the
vb2_is_busy checks inside the driver. We might have a scenario where the
active state is updated by .try_fmt() while device is streaming, but it
doesn't get applied till the next STREAMOFF and STREAMON.. which would lead
to wrong format being returned by .g_fmt() during this time.

> 
> > > > +
> > > > +/**
> > > > + * struct video_device_state - Used for storing video device state information.
> > > > + *
> > > > + * @vid_fmt: Format of the video capture stream
> > > > + * @meta_fmt: Format of the metadata capture stream
> > > > + * @which: is this a TRY or ACTIVE format?
> > > > + */
> > > > +struct video_device_state {
> > > > +     struct v4l2_format vid_fmt;
> > > > +     struct v4l2_format meta_fmt;
> > > > +     enum video_device_format_whence which;
> > > > +};
> > > > +
> > > >  /*
> > > >   * Newer version of video_device, handled by videodev2.c
> > > >   *   This version moves redundant code from video device code to
> > > > @@ -238,6 +268,7 @@ struct v4l2_file_operations {
> > > >   * @queue: &struct vb2_queue associated with this device node. May be NULL.
> > > >   * @prio: pointer to &struct v4l2_prio_state with device's Priority state.
> > > >   *    If NULL, then v4l2_dev->prio will be used.
> > > > + * @state: &struct video_device_state, holds the active state for the device.
> > > >   * @name: video device name
> > > >   * @vfl_type: V4L device type, as defined by &enum vfl_devnode_type
> > > >   * @vfl_dir: V4L receiver, transmitter or m2m
> > > > @@ -283,6 +314,7 @@ struct video_device {
> > > >       struct vb2_queue *queue;
> > > >
> > > >       struct v4l2_prio_state *prio;
> > > > +     struct video_device_state state;
> > > 
> > > One of the key design requirement it's the ability for drivers to
> > > sub-class video_device_state. One possibile way to obtain this is to
> > > dynamically allocate the state either by deferring to the driver's the
> > > allocation (so that they can allocate a bigger structure) or by
> > > passing to the framework the size it has to allocate.
> > > 
> > > In any case, I'm afraid the state should be allocated dynamically,
> > > either in the drivers' init_state() (or similar) callback or by the
> > > framework with a size hint from the driver.
> > > 
> > > What do you think ?
> > 
> > Ah okay, I missed that. Should be possible to make this dynamically
> > allocatable by the driver. It will also tie into Sakari's suggestion of
> > creating a helper for initializing the state.
> 
> Yes, I agree with Jacopo and Sakari here. The state should be
> dynamically allocated, and you should add an operation to initialize it.
> 
> > > >
> > > >       /* device info */
> > > >       char name[64];
> > > > @@ -540,6 +572,26 @@ static inline int video_is_registered(struct video_device *vdev)
> > > >       return test_bit(V4L2_FL_REGISTERED, &vdev->flags);
> > > >  }
> > > >
> > > > +/**
> > > > + * video_device_g_fmt_vid() - fill video v4l2_format from the state.
> > > > + *
> > > > + * @file: pointer to struct file
> > > > + * @state: pointer to video device state
> > > > + * @format: pointer to &struct v4l2_format
> > > > + */
> > > > +int video_device_g_fmt_vid(struct file *file, void *state,
> > > > +                        struct v4l2_format *format);
> > > > +
> > > > +/**
> > > > + * video_device_g_fmt_meta() - fill metadata v4l2_format from the state.
> > > > + *
> > > > + * @file: pointer to struct file
> > > > + * @state: pointer to video device state
> > > > + * @format: pointer to &struct v4l2_format
> > > > + */
> > > > +int video_device_g_fmt_meta(struct file *file, void *state,
> > > > +                         struct v4l2_format *format);
> > > > +
> > > >  /**
> > > >   * v4l2_debugfs_root - returns the dentry of the top-level "v4l2" debugfs dir
> > > >   *
> > > > diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
> > > > index b5b3e00c8e6a0b082d9cd8a0c972a5094adcb6f2..02579f87ba99d0c849a0865f8cc4295446c39f94 100644
> > > > --- a/include/media/v4l2-fh.h
> > > > +++ b/include/media/v4l2-fh.h
> > > > @@ -18,7 +18,8 @@
> > > >  #include <linux/list.h>
> > > >  #include <linux/videodev2.h>
> > > >
> > > > -struct video_device;
> > > > +#include <media/v4l2-dev.h>
> > > > +
> 
> You will be able to go back to forward declarations once you replace the
> state field below with a pointer.
> 
> > > >  struct v4l2_ctrl_handler;
> > > >
> > > >  /**
> > > > @@ -28,6 +29,7 @@ struct v4l2_ctrl_handler;
> > > >   * @vdev: pointer to &struct video_device
> > > >   * @ctrl_handler: pointer to &struct v4l2_ctrl_handler
> > > >   * @prio: priority of the file handler, as defined by &enum v4l2_priority
> > > > + * @state: try state used for format negotiation on the video device
> > > >   *
> > > >   * @wait: event' s wait queue
> > > >   * @subscribe_lock: serialise changes to the subscribed list; guarantee that
> > > > @@ -44,6 +46,7 @@ struct v4l2_fh {
> > > >       struct video_device     *vdev;
> > > >       struct v4l2_ctrl_handler *ctrl_handler;
> > > >       enum v4l2_priority      prio;
> > > > +     struct video_device_state state;
> > > >
> > > >       /* Events */
> > > >       wait_queue_head_t       wait;
> > > >
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Will fix other comments, and rebase on top of Jacopo's series before
posting v2.

Thanks,
Jai

Re: [PATCH RFC 1/3] media: v4l2-core: Add support for video device state

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 783 bytes --]

Hi Jai,

Le jeudi 07 août 2025 à 12:26 +0530, Jai Luthra a écrit :
> Quite a few of those are VBI or test drivers. But some relevant cases are
> encoders/decoders or ISP drivers that may allow changing resolution or
> format mid stream.

I checked up a subset of the reported codec, and they pretty much all seem false
positive. They simply use a different state to fixate the format at a certain
point in time. Sateful decoder do that once the stream header have been
received, and most of them don't allow any change of capture queues.

That being said, we don't want to return EBUSY while streaming on stateless
decoders. Hantro supports changing resolution without flushing the reference
frames (heteronegeous reference frame, used in VP9 and AV1).

Nicolas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH RFC 2/3] media: ti: j721e-csi2rx: Use video_device_state

[Jai Luthra]

Use the newly introduced video_device_state to store the active v4l2
format for the video device.

With this change, we can use a single function for the .s_fmt and
.try_fmt hooks, and the framework helper for the .g_fmt hook.

Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 .../media/platform/ti/j721e-csi2rx/j721e-csi2rx.c  | 57 +++++++---------------
 1 file changed, 17 insertions(+), 40 deletions(-)

diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
index 6412a00be8eab89548950dd21b3b3ec02dafa5b4..5aed3c005c8788f77690104c1c1d63509fd59e09 100644
--- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
+++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
@@ -15,6 +15,7 @@
 #include <linux/platform_device.h>
 
 #include <media/mipi-csi2.h>
+#include <media/v4l2-dev.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-ioctl.h>
 #include <media/v4l2-mc.h>
@@ -104,7 +105,6 @@ struct ti_csi2rx_dev {
 	struct v4l2_subdev		*source;
 	struct vb2_queue		vidq;
 	struct mutex			mutex; /* To serialize ioctls. */
-	struct v4l2_format		v_fmt;
 	struct ti_csi2rx_dma		dma;
 	u32				sequence;
 };
@@ -299,19 +299,10 @@ static int ti_csi2rx_enum_fmt_vid_cap(struct file *file, void *priv,
 	return 0;
 }
 
-static int ti_csi2rx_g_fmt_vid_cap(struct file *file, void *prov,
-				   struct v4l2_format *f)
-{
-	struct ti_csi2rx_dev *csi = video_drvdata(file);
-
-	*f = csi->v_fmt;
-
-	return 0;
-}
-
-static int ti_csi2rx_try_fmt_vid_cap(struct file *file, void *priv,
+static int ti_csi2rx_adj_fmt_vid_cap(struct file *file, void *priv,
 				     struct v4l2_format *f)
 {
+	struct video_device_state *state = priv;
 	const struct ti_csi2rx_fmt *fmt;
 
 	/*
@@ -327,24 +318,7 @@ static int ti_csi2rx_try_fmt_vid_cap(struct file *file, void *priv,
 
 	ti_csi2rx_fill_fmt(fmt, f);
 
-	return 0;
-}
-
-static int ti_csi2rx_s_fmt_vid_cap(struct file *file, void *priv,
-				   struct v4l2_format *f)
-{
-	struct ti_csi2rx_dev *csi = video_drvdata(file);
-	struct vb2_queue *q = &csi->vidq;
-	int ret;
-
-	if (vb2_is_busy(q))
-		return -EBUSY;
-
-	ret = ti_csi2rx_try_fmt_vid_cap(file, priv, f);
-	if (ret < 0)
-		return ret;
-
-	csi->v_fmt = *f;
+	state->vid_fmt = *f;
 
 	return 0;
 }
@@ -380,9 +354,9 @@ static int ti_csi2rx_enum_framesizes(struct file *file, void *fh,
 static const struct v4l2_ioctl_ops csi_ioctl_ops = {
 	.vidioc_querycap      = ti_csi2rx_querycap,
 	.vidioc_enum_fmt_vid_cap = ti_csi2rx_enum_fmt_vid_cap,
-	.vidioc_try_fmt_vid_cap = ti_csi2rx_try_fmt_vid_cap,
-	.vidioc_g_fmt_vid_cap = ti_csi2rx_g_fmt_vid_cap,
-	.vidioc_s_fmt_vid_cap = ti_csi2rx_s_fmt_vid_cap,
+	.vidioc_try_fmt_vid_cap = ti_csi2rx_adj_fmt_vid_cap,
+	.vidioc_g_fmt_vid_cap = video_device_g_fmt_vid,
+	.vidioc_s_fmt_vid_cap = ti_csi2rx_adj_fmt_vid_cap,
 	.vidioc_enum_framesizes = ti_csi2rx_enum_framesizes,
 	.vidioc_reqbufs       = vb2_ioctl_reqbufs,
 	.vidioc_create_bufs   = vb2_ioctl_create_bufs,
@@ -488,7 +462,7 @@ static void ti_csi2rx_setup_shim(struct ti_csi2rx_dev *csi)
 	const struct ti_csi2rx_fmt *fmt;
 	unsigned int reg;
 
-	fmt = find_format_by_fourcc(csi->v_fmt.fmt.pix.pixelformat);
+	fmt = find_format_by_fourcc(csi->vdev.state.vid_fmt.fmt.pix.pixelformat);
 
 	/* De-assert the pixel interface reset. */
 	reg = SHIM_CNTL_PIX_RST;
@@ -636,7 +610,7 @@ static int ti_csi2rx_start_dma(struct ti_csi2rx_dev *csi,
 {
 	unsigned long addr;
 	struct dma_async_tx_descriptor *desc;
-	size_t len = csi->v_fmt.fmt.pix.sizeimage;
+	size_t len = csi->vdev.state.vid_fmt.fmt.pix.sizeimage;
 	dma_cookie_t cookie;
 	int ret = 0;
 
@@ -714,7 +688,7 @@ static int ti_csi2rx_queue_setup(struct vb2_queue *q, unsigned int *nbuffers,
 				 struct device *alloc_devs[])
 {
 	struct ti_csi2rx_dev *csi = vb2_get_drv_priv(q);
-	unsigned int size = csi->v_fmt.fmt.pix.sizeimage;
+	unsigned int size = csi->vdev.state.vid_fmt.fmt.pix.sizeimage;
 
 	if (*nplanes) {
 		if (sizes[0] < size)
@@ -731,7 +705,7 @@ static int ti_csi2rx_queue_setup(struct vb2_queue *q, unsigned int *nbuffers,
 static int ti_csi2rx_buffer_prepare(struct vb2_buffer *vb)
 {
 	struct ti_csi2rx_dev *csi = vb2_get_drv_priv(vb->vb2_queue);
-	unsigned long size = csi->v_fmt.fmt.pix.sizeimage;
+	unsigned long size = csi->vdev.state.vid_fmt.fmt.pix.sizeimage;
 
 	if (vb2_plane_size(vb, 0) < size) {
 		dev_err(csi->dev, "Data will not fit into plane\n");
@@ -910,7 +884,7 @@ static int ti_csi2rx_link_validate(struct media_link *link)
 	struct media_entity *entity = link->sink->entity;
 	struct video_device *vdev = media_entity_to_video_device(entity);
 	struct ti_csi2rx_dev *csi = container_of(vdev, struct ti_csi2rx_dev, vdev);
-	struct v4l2_pix_format *csi_fmt = &csi->v_fmt.fmt.pix;
+	struct v4l2_pix_format *csi_fmt = &vdev->state.vid_fmt.fmt.pix;
 	struct v4l2_subdev_format source_fmt = {
 		.which	= V4L2_SUBDEV_FORMAT_ACTIVE,
 		.pad	= link->source->index,
@@ -1001,13 +975,15 @@ static int ti_csi2rx_v4l2_init(struct ti_csi2rx_dev *csi)
 	struct media_device *mdev = &csi->mdev;
 	struct video_device *vdev = &csi->vdev;
 	const struct ti_csi2rx_fmt *fmt;
-	struct v4l2_pix_format *pix_fmt = &csi->v_fmt.fmt.pix;
+	struct v4l2_pix_format *pix_fmt;
 	int ret;
 
 	fmt = find_format_by_fourcc(V4L2_PIX_FMT_UYVY);
 	if (!fmt)
 		return -EINVAL;
 
+	vdev->state.vid_fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+	pix_fmt = &vdev->state.vid_fmt.fmt.pix;
 	pix_fmt->width = 640;
 	pix_fmt->height = 480;
 	pix_fmt->field = V4L2_FIELD_NONE;
@@ -1016,7 +992,7 @@ static int ti_csi2rx_v4l2_init(struct ti_csi2rx_dev *csi)
 	pix_fmt->quantization = V4L2_QUANTIZATION_LIM_RANGE;
 	pix_fmt->xfer_func = V4L2_XFER_FUNC_SRGB;
 
-	ti_csi2rx_fill_fmt(fmt, &csi->v_fmt);
+	ti_csi2rx_fill_fmt(fmt, &vdev->state.vid_fmt);
 
 	mdev->dev = csi->dev;
 	mdev->hw_revision = 1;
@@ -1033,6 +1009,7 @@ static int ti_csi2rx_v4l2_init(struct ti_csi2rx_dev *csi)
 	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING |
 			    V4L2_CAP_IO_MC;
 	vdev->lock = &csi->mutex;
+	set_bit(V4L2_FL_USES_STATE, &vdev->flags);
 	video_set_drvdata(vdev, csi);
 
 	csi->pad.flags = MEDIA_PAD_FL_SINK;

-- 
2.49.0

[PATCH RFC 3/3] media: rkisp1: Use video_device_state

[Jai Luthra]

Use the newly introduced video_device_state to store the active v4l2
format for the video device.

Additionally, perform the stride calculation when required instead of
storing it in the driver context structure.

Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 .../platform/rockchip/rkisp1/rkisp1-capture.c      | 113 +++++++++------------
 .../media/platform/rockchip/rkisp1/rkisp1-common.h |   4 -
 2 files changed, 50 insertions(+), 67 deletions(-)

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
index 6dcefd144d5abe358323e37ac6133c6134ac636e..f3f2a7c3c11319470f6619cb83a87d39ee21ba61 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
@@ -11,6 +11,7 @@
 #include <linux/delay.h>
 #include <linux/pm_runtime.h>
 #include <media/v4l2-common.h>
+#include <media/v4l2-dev.h>
 #include <media/v4l2-event.h>
 #include <media/v4l2-fh.h>
 #include <media/v4l2-ioctl.h>
@@ -482,7 +483,9 @@ static void rkisp1_irq_frame_end_enable(struct rkisp1_capture *cap)
 
 static void rkisp1_mp_config(struct rkisp1_capture *cap)
 {
-	const struct v4l2_pix_format_mplane *pixm = &cap->pix.fmt;
+	const struct v4l2_pix_format_mplane *pixm =
+		&cap->vnode.vdev.state.vid_fmt.fmt.pix_mp;
+	u32 stride = pixm->plane_fmt[0].bytesperline / cap->pix.info->bpp[0];
 	struct rkisp1_device *rkisp1 = cap->rkisp1;
 	u32 reg;
 
@@ -494,11 +497,11 @@ static void rkisp1_mp_config(struct rkisp1_capture *cap)
 		     rkisp1_pixfmt_comp_size(pixm, RKISP1_PLANE_CR));
 
 	if (rkisp1_has_feature(rkisp1, MAIN_STRIDE)) {
-		rkisp1_write(rkisp1, RKISP1_CIF_MI_MP_Y_LLENGTH, cap->stride);
+		rkisp1_write(rkisp1, RKISP1_CIF_MI_MP_Y_LLENGTH, stride);
 		rkisp1_write(rkisp1, RKISP1_CIF_MI_MP_Y_PIC_WIDTH, pixm->width);
 		rkisp1_write(rkisp1, RKISP1_CIF_MI_MP_Y_PIC_HEIGHT, pixm->height);
 		rkisp1_write(rkisp1, RKISP1_CIF_MI_MP_Y_PIC_SIZE,
-			     cap->stride * pixm->height);
+			     stride * pixm->height);
 	}
 
 	rkisp1_irq_frame_end_enable(cap);
@@ -546,7 +549,9 @@ static void rkisp1_mp_config(struct rkisp1_capture *cap)
 
 static void rkisp1_sp_config(struct rkisp1_capture *cap)
 {
-	const struct v4l2_pix_format_mplane *pixm = &cap->pix.fmt;
+	const struct v4l2_pix_format_mplane *pixm =
+		&cap->vnode.vdev.state.vid_fmt.fmt.pix_mp;
+	u32 stride = pixm->plane_fmt[0].bytesperline / cap->pix.info->bpp[0];
 	struct rkisp1_device *rkisp1 = cap->rkisp1;
 	u32 mi_ctrl, reg;
 
@@ -557,11 +562,11 @@ static void rkisp1_sp_config(struct rkisp1_capture *cap)
 	rkisp1_write(rkisp1, cap->config->mi.cr_size_init,
 		     rkisp1_pixfmt_comp_size(pixm, RKISP1_PLANE_CR));
 
-	rkisp1_write(rkisp1, RKISP1_CIF_MI_SP_Y_LLENGTH, cap->stride);
+	rkisp1_write(rkisp1, RKISP1_CIF_MI_SP_Y_LLENGTH, stride);
 	rkisp1_write(rkisp1, RKISP1_CIF_MI_SP_Y_PIC_WIDTH, pixm->width);
 	rkisp1_write(rkisp1, RKISP1_CIF_MI_SP_Y_PIC_HEIGHT, pixm->height);
 	rkisp1_write(rkisp1, RKISP1_CIF_MI_SP_Y_PIC_SIZE,
-		     cap->stride * pixm->height);
+		     stride * pixm->height);
 
 	rkisp1_irq_frame_end_enable(cap);
 
@@ -704,7 +709,8 @@ static const struct rkisp1_capture_ops rkisp1_capture_ops_sp = {
 
 static int rkisp1_dummy_buf_create(struct rkisp1_capture *cap)
 {
-	const struct v4l2_pix_format_mplane *pixm = &cap->pix.fmt;
+	const struct v4l2_pix_format_mplane *pixm =
+		&cap->vnode.vdev.state.vid_fmt.fmt.pix_mp;
 	struct rkisp1_dummy_buffer *dummy_buf = &cap->buf.dummy;
 
 	dummy_buf->size = max3(rkisp1_pixfmt_comp_size(pixm, RKISP1_PLANE_Y),
@@ -869,7 +875,8 @@ static int rkisp1_vb2_queue_setup(struct vb2_queue *queue,
 				  struct device *alloc_devs[])
 {
 	struct rkisp1_capture *cap = queue->drv_priv;
-	const struct v4l2_pix_format_mplane *pixm = &cap->pix.fmt;
+	const struct v4l2_pix_format_mplane *pixm =
+		&cap->vnode.vdev.state.vid_fmt.fmt.pix_mp;
 	unsigned int i;
 
 	if (*num_planes) {
@@ -894,7 +901,8 @@ static int rkisp1_vb2_buf_init(struct vb2_buffer *vb)
 	struct rkisp1_buffer *ispbuf =
 		container_of(vbuf, struct rkisp1_buffer, vb);
 	struct rkisp1_capture *cap = vb->vb2_queue->drv_priv;
-	const struct v4l2_pix_format_mplane *pixm = &cap->pix.fmt;
+	const struct v4l2_pix_format_mplane *pixm =
+		&cap->vnode.vdev.state.vid_fmt.fmt.pix_mp;
 	unsigned int i;
 
 	memset(ispbuf->buff_addr, 0, sizeof(ispbuf->buff_addr));
@@ -936,10 +944,12 @@ static void rkisp1_vb2_buf_queue(struct vb2_buffer *vb)
 static int rkisp1_vb2_buf_prepare(struct vb2_buffer *vb)
 {
 	struct rkisp1_capture *cap = vb->vb2_queue->drv_priv;
+	const struct v4l2_pix_format_mplane *pixm =
+		&cap->vnode.vdev.state.vid_fmt.fmt.pix_mp;
 	unsigned int i;
 
-	for (i = 0; i < cap->pix.fmt.num_planes; i++) {
-		unsigned long size = cap->pix.fmt.plane_fmt[i].sizeimage;
+	for (i = 0; i < pixm->num_planes; i++) {
+		unsigned long size = pixm->plane_fmt[i].sizeimage;
 
 		if (vb2_plane_size(vb, i) < size) {
 			dev_err(cap->rkisp1->dev,
@@ -1278,7 +1288,7 @@ rkisp1_find_fmt_cfg(const struct rkisp1_capture *cap, const u32 pixelfmt)
 	return NULL;
 }
 
-static void rkisp1_try_fmt(const struct rkisp1_capture *cap,
+static void rkisp1_adj_fmt(const struct rkisp1_capture *cap,
 			   struct v4l2_pix_format_mplane *pixm,
 			   const struct rkisp1_capture_fmt_cfg **fmt_cfg,
 			   const struct v4l2_format_info **fmt_info)
@@ -1317,22 +1327,20 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap,
 		*fmt_info = info;
 }
 
-static void rkisp1_set_fmt(struct rkisp1_capture *cap,
-			   struct v4l2_pix_format_mplane *pixm)
-{
-	rkisp1_try_fmt(cap, pixm, &cap->pix.cfg, &cap->pix.info);
-
-	cap->pix.fmt = *pixm;
-	cap->stride = pixm->plane_fmt[0].bytesperline / cap->pix.info->bpp[0];
-}
-
-static int rkisp1_try_fmt_vid_cap_mplane(struct file *file, void *fh,
+static int rkisp1_adj_fmt_vid_cap_mplane(struct file *file, void *priv,
 					 struct v4l2_format *f)
 {
 	struct rkisp1_capture *cap = video_drvdata(file);
+	struct video_device_state *state = priv;
 
-	rkisp1_try_fmt(cap, &f->fmt.pix_mp, NULL, NULL);
+	if (state->which == VIDEO_DEVICE_FORMAT_ACTIVE) {
+		rkisp1_adj_fmt(cap, &f->fmt.pix_mp, &cap->pix.cfg,
+			       &cap->pix.info);
+	} else {
+		rkisp1_adj_fmt(cap, &f->fmt.pix_mp, NULL, NULL);
+	}
 
+	state->vid_fmt = *f;
 	return 0;
 }
 
@@ -1399,31 +1407,6 @@ static int rkisp1_enum_framesizes(struct file *file, void *fh,
 	return 0;
 }
 
-static int rkisp1_s_fmt_vid_cap_mplane(struct file *file,
-				       void *priv, struct v4l2_format *f)
-{
-	struct rkisp1_capture *cap = video_drvdata(file);
-	struct rkisp1_vdev_node *node =
-				rkisp1_vdev_to_node(&cap->vnode.vdev);
-
-	if (vb2_is_busy(&node->buf_queue))
-		return -EBUSY;
-
-	rkisp1_set_fmt(cap, &f->fmt.pix_mp);
-
-	return 0;
-}
-
-static int rkisp1_g_fmt_vid_cap_mplane(struct file *file, void *fh,
-				       struct v4l2_format *f)
-{
-	struct rkisp1_capture *cap = video_drvdata(file);
-
-	f->fmt.pix_mp = cap->pix.fmt;
-
-	return 0;
-}
-
 static int
 rkisp1_querycap(struct file *file, void *priv, struct v4l2_capability *cap)
 {
@@ -1444,9 +1427,9 @@ static const struct v4l2_ioctl_ops rkisp1_v4l2_ioctl_ops = {
 	.vidioc_prepare_buf = vb2_ioctl_prepare_buf,
 	.vidioc_streamon = vb2_ioctl_streamon,
 	.vidioc_streamoff = vb2_ioctl_streamoff,
-	.vidioc_try_fmt_vid_cap_mplane = rkisp1_try_fmt_vid_cap_mplane,
-	.vidioc_s_fmt_vid_cap_mplane = rkisp1_s_fmt_vid_cap_mplane,
-	.vidioc_g_fmt_vid_cap_mplane = rkisp1_g_fmt_vid_cap_mplane,
+	.vidioc_try_fmt_vid_cap_mplane = rkisp1_adj_fmt_vid_cap_mplane,
+	.vidioc_s_fmt_vid_cap_mplane = rkisp1_adj_fmt_vid_cap_mplane,
+	.vidioc_g_fmt_vid_cap_mplane = video_device_g_fmt_vid,
 	.vidioc_enum_fmt_vid_cap = rkisp1_enum_fmt_vid_cap_mplane,
 	.vidioc_enum_framesizes = rkisp1_enum_framesizes,
 	.vidioc_querycap = rkisp1_querycap,
@@ -1461,8 +1444,10 @@ static int rkisp1_capture_link_validate(struct media_link *link)
 	struct v4l2_subdev *sd =
 		media_entity_to_v4l2_subdev(link->source->entity);
 	struct rkisp1_capture *cap = video_get_drvdata(vdev);
+	const struct v4l2_pix_format_mplane *pixm =
+		&vdev->state.vid_fmt.fmt.pix_mp;
 	const struct rkisp1_capture_fmt_cfg *fmt =
-		rkisp1_find_fmt_cfg(cap, cap->pix.fmt.pixelformat);
+		rkisp1_find_fmt_cfg(cap, pixm->pixelformat);
 	struct v4l2_subdev_format sd_fmt = {
 		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
 		.pad = link->source->index,
@@ -1473,16 +1458,16 @@ static int rkisp1_capture_link_validate(struct media_link *link)
 	if (ret)
 		return ret;
 
-	if (sd_fmt.format.height != cap->pix.fmt.height ||
-	    sd_fmt.format.width != cap->pix.fmt.width ||
+	if (sd_fmt.format.height != pixm->height ||
+	    sd_fmt.format.width != pixm->width ||
 	    sd_fmt.format.code != fmt->mbus) {
 		dev_dbg(cap->rkisp1->dev,
 			"link '%s':%u -> '%s':%u not valid: 0x%04x/%ux%u != 0x%04x/%ux%u\n",
 			link->source->entity->name, link->source->index,
 			link->sink->entity->name, link->sink->index,
 			sd_fmt.format.code, sd_fmt.format.width,
-			sd_fmt.format.height, fmt->mbus, cap->pix.fmt.width,
-			cap->pix.fmt.height);
+			sd_fmt.format.height, fmt->mbus, pixm->width,
+			pixm->height);
 		return -EPIPE;
 	}
 
@@ -1531,6 +1516,7 @@ static int rkisp1_register_capture(struct rkisp1_capture *cap)
 	};
 	struct v4l2_device *v4l2_dev = &cap->rkisp1->v4l2_dev;
 	struct video_device *vdev = &cap->vnode.vdev;
+	struct v4l2_pix_format_mplane *pixm;
 	struct rkisp1_vdev_node *node;
 	struct vb2_queue *q;
 	int ret;
@@ -1548,6 +1534,7 @@ static int rkisp1_register_capture(struct rkisp1_capture *cap)
 	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE_MPLANE |
 			    V4L2_CAP_STREAMING | V4L2_CAP_IO_MC;
 	vdev->entity.ops = &rkisp1_media_ops;
+	set_bit(V4L2_FL_USES_STATE, &vdev->flags);
 	video_set_drvdata(vdev, cap);
 	vdev->vfl_dir = VFL_DIR_RX;
 	node->pad.flags = MEDIA_PAD_FL_SINK;
@@ -1572,6 +1559,13 @@ static int rkisp1_register_capture(struct rkisp1_capture *cap)
 
 	vdev->queue = q;
 
+	vdev->state.vid_fmt.type = q->type;
+	pixm = &vdev->state.vid_fmt.fmt.pix_mp;
+	pixm->pixelformat = V4L2_PIX_FMT_YUYV;
+	pixm->width = RKISP1_DEFAULT_WIDTH;
+	pixm->height = RKISP1_DEFAULT_HEIGHT;
+	rkisp1_adj_fmt(cap, pixm, &cap->pix.cfg, &cap->pix.info);
+
 	ret = media_entity_pads_init(&vdev->entity, 1, &node->pad);
 	if (ret)
 		goto error;
@@ -1598,7 +1592,6 @@ static void
 rkisp1_capture_init(struct rkisp1_device *rkisp1, enum rkisp1_stream_id id)
 {
 	struct rkisp1_capture *cap = &rkisp1->capture_devs[id];
-	struct v4l2_pix_format_mplane pixm;
 
 	memset(cap, 0, sizeof(*cap));
 	cap->id = id;
@@ -1616,12 +1609,6 @@ rkisp1_capture_init(struct rkisp1_device *rkisp1, enum rkisp1_stream_id id)
 	}
 
 	cap->is_streaming = false;
-
-	memset(&pixm, 0, sizeof(pixm));
-	pixm.pixelformat = V4L2_PIX_FMT_YUYV;
-	pixm.width = RKISP1_DEFAULT_WIDTH;
-	pixm.height = RKISP1_DEFAULT_HEIGHT;
-	rkisp1_set_fmt(cap, &pixm);
 }
 
 int rkisp1_capture_devs_register(struct rkisp1_device *rkisp1)
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
index ca952fd0829ba7d923ad42fec92840ccd422b6e5..7c1556bc5980f937ff2c503282bb5623283bda1a 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
@@ -304,7 +304,6 @@ struct rkisp1_device;
  *		  handler to stop the streaming by waiting on the 'done' wait queue.
  *		  If the irq handler is not called, the stream is stopped by the callback
  *		  after timeout.
- * @stride:       the line stride for the first plane, in pixel units
  * @buf.lock:	  lock to protect buf.queue
  * @buf.queue:	  queued buffer list
  * @buf.dummy:	  dummy space to store dropped data
@@ -314,7 +313,6 @@ struct rkisp1_device;
  * @buf.next:	  the buffer used for next frame
  * @pix.cfg:	  pixel configuration
  * @pix.info:	  a pointer to the v4l2_format_info of the pixel format
- * @pix.fmt:	  buffer format
  */
 struct rkisp1_capture {
 	struct rkisp1_vdev_node vnode;
@@ -325,7 +323,6 @@ struct rkisp1_capture {
 	bool is_streaming;
 	bool is_stopping;
 	wait_queue_head_t done;
-	unsigned int stride;
 	struct {
 		/* protects queue, curr and next */
 		spinlock_t lock;
@@ -337,7 +334,6 @@ struct rkisp1_capture {
 	struct {
 		const struct rkisp1_capture_fmt_cfg *cfg;
 		const struct v4l2_format_info *info;
-		struct v4l2_pix_format_mplane fmt;
 	} pix;
 };
 

-- 
2.49.0

Re: [PATCH RFC 3/3] media: rkisp1: Use video_device_state

[Jacopo Mondi]

Hi Jai

On Thu, Jul 03, 2025 at 06:02:10PM -0700, Jai Luthra wrote:
> Use the newly introduced video_device_state to store the active v4l2
> format for the video device.
>
> Additionally, perform the stride calculation when required instead of
> storing it in the driver context structure.
>
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> ---
>  .../platform/rockchip/rkisp1/rkisp1-capture.c      | 113 +++++++++------------
>  .../media/platform/rockchip/rkisp1/rkisp1-common.h |   4 -
>  2 files changed, 50 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> index 6dcefd144d5abe358323e37ac6133c6134ac636e..f3f2a7c3c11319470f6619cb83a87d39ee21ba61 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> @@ -11,6 +11,7 @@
>  #include <linux/delay.h>
>  #include <linux/pm_runtime.h>
>  #include <media/v4l2-common.h>
> +#include <media/v4l2-dev.h>
>  #include <media/v4l2-event.h>
>  #include <media/v4l2-fh.h>
>  #include <media/v4l2-ioctl.h>
> @@ -482,7 +483,9 @@ static void rkisp1_irq_frame_end_enable(struct rkisp1_capture *cap)
>
>  static void rkisp1_mp_config(struct rkisp1_capture *cap)
>  {
> -	const struct v4l2_pix_format_mplane *pixm = &cap->pix.fmt;
> +	const struct v4l2_pix_format_mplane *pixm =
> +		&cap->vnode.vdev.state.vid_fmt.fmt.pix_mp;

We'll need helpers to get the correct format given a video device

In example:
struct v4l2_pix_format_mplane *video_device_state_get_mplane_fmt(vdev)

> +	u32 stride = pixm->plane_fmt[0].bytesperline / cap->pix.info->bpp[0];
>  	struct rkisp1_device *rkisp1 = cap->rkisp1;
>  	u32 reg;
>
> @@ -494,11 +497,11 @@ static void rkisp1_mp_config(struct rkisp1_capture *cap)
>  		     rkisp1_pixfmt_comp_size(pixm, RKISP1_PLANE_CR));
>
>  	if (rkisp1_has_feature(rkisp1, MAIN_STRIDE)) {
> -		rkisp1_write(rkisp1, RKISP1_CIF_MI_MP_Y_LLENGTH, cap->stride);
> +		rkisp1_write(rkisp1, RKISP1_CIF_MI_MP_Y_LLENGTH, stride);
>  		rkisp1_write(rkisp1, RKISP1_CIF_MI_MP_Y_PIC_WIDTH, pixm->width);
>  		rkisp1_write(rkisp1, RKISP1_CIF_MI_MP_Y_PIC_HEIGHT, pixm->height);
>  		rkisp1_write(rkisp1, RKISP1_CIF_MI_MP_Y_PIC_SIZE,
> -			     cap->stride * pixm->height);
> +			     stride * pixm->height);
>  	}
>
>  	rkisp1_irq_frame_end_enable(cap);
> @@ -546,7 +549,9 @@ static void rkisp1_mp_config(struct rkisp1_capture *cap)
>
>  static void rkisp1_sp_config(struct rkisp1_capture *cap)
>  {
> -	const struct v4l2_pix_format_mplane *pixm = &cap->pix.fmt;
> +	const struct v4l2_pix_format_mplane *pixm =
> +		&cap->vnode.vdev.state.vid_fmt.fmt.pix_mp;
> +	u32 stride = pixm->plane_fmt[0].bytesperline / cap->pix.info->bpp[0];
>  	struct rkisp1_device *rkisp1 = cap->rkisp1;
>  	u32 mi_ctrl, reg;
>
> @@ -557,11 +562,11 @@ static void rkisp1_sp_config(struct rkisp1_capture *cap)
>  	rkisp1_write(rkisp1, cap->config->mi.cr_size_init,
>  		     rkisp1_pixfmt_comp_size(pixm, RKISP1_PLANE_CR));
>
> -	rkisp1_write(rkisp1, RKISP1_CIF_MI_SP_Y_LLENGTH, cap->stride);
> +	rkisp1_write(rkisp1, RKISP1_CIF_MI_SP_Y_LLENGTH, stride);
>  	rkisp1_write(rkisp1, RKISP1_CIF_MI_SP_Y_PIC_WIDTH, pixm->width);
>  	rkisp1_write(rkisp1, RKISP1_CIF_MI_SP_Y_PIC_HEIGHT, pixm->height);
>  	rkisp1_write(rkisp1, RKISP1_CIF_MI_SP_Y_PIC_SIZE,
> -		     cap->stride * pixm->height);
> +		     stride * pixm->height);
>
>  	rkisp1_irq_frame_end_enable(cap);
>
> @@ -704,7 +709,8 @@ static const struct rkisp1_capture_ops rkisp1_capture_ops_sp = {
>
>  static int rkisp1_dummy_buf_create(struct rkisp1_capture *cap)
>  {
> -	const struct v4l2_pix_format_mplane *pixm = &cap->pix.fmt;
> +	const struct v4l2_pix_format_mplane *pixm =
> +		&cap->vnode.vdev.state.vid_fmt.fmt.pix_mp;
>  	struct rkisp1_dummy_buffer *dummy_buf = &cap->buf.dummy;
>
>  	dummy_buf->size = max3(rkisp1_pixfmt_comp_size(pixm, RKISP1_PLANE_Y),
> @@ -869,7 +875,8 @@ static int rkisp1_vb2_queue_setup(struct vb2_queue *queue,
>  				  struct device *alloc_devs[])
>  {
>  	struct rkisp1_capture *cap = queue->drv_priv;
> -	const struct v4l2_pix_format_mplane *pixm = &cap->pix.fmt;
> +	const struct v4l2_pix_format_mplane *pixm =
> +		&cap->vnode.vdev.state.vid_fmt.fmt.pix_mp;
>  	unsigned int i;
>
>  	if (*num_planes) {
> @@ -894,7 +901,8 @@ static int rkisp1_vb2_buf_init(struct vb2_buffer *vb)
>  	struct rkisp1_buffer *ispbuf =
>  		container_of(vbuf, struct rkisp1_buffer, vb);
>  	struct rkisp1_capture *cap = vb->vb2_queue->drv_priv;
> -	const struct v4l2_pix_format_mplane *pixm = &cap->pix.fmt;
> +	const struct v4l2_pix_format_mplane *pixm =
> +		&cap->vnode.vdev.state.vid_fmt.fmt.pix_mp;
>  	unsigned int i;
>
>  	memset(ispbuf->buff_addr, 0, sizeof(ispbuf->buff_addr));
> @@ -936,10 +944,12 @@ static void rkisp1_vb2_buf_queue(struct vb2_buffer *vb)
>  static int rkisp1_vb2_buf_prepare(struct vb2_buffer *vb)
>  {
>  	struct rkisp1_capture *cap = vb->vb2_queue->drv_priv;
> +	const struct v4l2_pix_format_mplane *pixm =
> +		&cap->vnode.vdev.state.vid_fmt.fmt.pix_mp;
>  	unsigned int i;
>
> -	for (i = 0; i < cap->pix.fmt.num_planes; i++) {
> -		unsigned long size = cap->pix.fmt.plane_fmt[i].sizeimage;
> +	for (i = 0; i < pixm->num_planes; i++) {
> +		unsigned long size = pixm->plane_fmt[i].sizeimage;
>
>  		if (vb2_plane_size(vb, i) < size) {
>  			dev_err(cap->rkisp1->dev,
> @@ -1278,7 +1288,7 @@ rkisp1_find_fmt_cfg(const struct rkisp1_capture *cap, const u32 pixelfmt)
>  	return NULL;
>  }
>
> -static void rkisp1_try_fmt(const struct rkisp1_capture *cap,
> +static void rkisp1_adj_fmt(const struct rkisp1_capture *cap,
>  			   struct v4l2_pix_format_mplane *pixm,
>  			   const struct rkisp1_capture_fmt_cfg **fmt_cfg,
>  			   const struct v4l2_format_info **fmt_info)
> @@ -1317,22 +1327,20 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap,
>  		*fmt_info = info;
>  }
>
> -static void rkisp1_set_fmt(struct rkisp1_capture *cap,
> -			   struct v4l2_pix_format_mplane *pixm)
> -{
> -	rkisp1_try_fmt(cap, pixm, &cap->pix.cfg, &cap->pix.info);
> -
> -	cap->pix.fmt = *pixm;
> -	cap->stride = pixm->plane_fmt[0].bytesperline / cap->pix.info->bpp[0];
> -}
> -
> -static int rkisp1_try_fmt_vid_cap_mplane(struct file *file, void *fh,
> +static int rkisp1_adj_fmt_vid_cap_mplane(struct file *file, void *priv,
>  					 struct v4l2_format *f)
>  {
>  	struct rkisp1_capture *cap = video_drvdata(file);
> +	struct video_device_state *state = priv;
>
> -	rkisp1_try_fmt(cap, &f->fmt.pix_mp, NULL, NULL);
> +	if (state->which == VIDEO_DEVICE_FORMAT_ACTIVE) {
> +		rkisp1_adj_fmt(cap, &f->fmt.pix_mp, &cap->pix.cfg,
> +			       &cap->pix.info);
> +	} else {
> +		rkisp1_adj_fmt(cap, &f->fmt.pix_mp, NULL, NULL);
> +	}

This I'm not sure is desirable.

Applying the format on a video device happens when the streaming is
started. IOW if you set fmt while the device is streaming you get back
ebusy (and I think your first patch implement that already).

So do you need to update cap->pix here ? Or can you operate on a the
v4l2_format stored in the state regardless of the
FORMAT_ACTIVE/FORMAT_TRY and when streaming is started update the
driver-specific info with the format info stored in the 'active'
state ? This means that we'll need helpers that given a vdev return
the 'active' format, ie the one stored in the state embedded in the
video device.

>
> +	state->vid_fmt = *f;
>  	return 0;
>  }
>
> @@ -1399,31 +1407,6 @@ static int rkisp1_enum_framesizes(struct file *file, void *fh,
>  	return 0;
>  }
>
> -static int rkisp1_s_fmt_vid_cap_mplane(struct file *file,
> -				       void *priv, struct v4l2_format *f)
> -{
> -	struct rkisp1_capture *cap = video_drvdata(file);
> -	struct rkisp1_vdev_node *node =
> -				rkisp1_vdev_to_node(&cap->vnode.vdev);
> -
> -	if (vb2_is_busy(&node->buf_queue))
> -		return -EBUSY;
> -
> -	rkisp1_set_fmt(cap, &f->fmt.pix_mp);
> -
> -	return 0;
> -}
> -
> -static int rkisp1_g_fmt_vid_cap_mplane(struct file *file, void *fh,
> -				       struct v4l2_format *f)
> -{
> -	struct rkisp1_capture *cap = video_drvdata(file);
> -
> -	f->fmt.pix_mp = cap->pix.fmt;
> -
> -	return 0;
> -}
> -
>  static int
>  rkisp1_querycap(struct file *file, void *priv, struct v4l2_capability *cap)
>  {
> @@ -1444,9 +1427,9 @@ static const struct v4l2_ioctl_ops rkisp1_v4l2_ioctl_ops = {
>  	.vidioc_prepare_buf = vb2_ioctl_prepare_buf,
>  	.vidioc_streamon = vb2_ioctl_streamon,
>  	.vidioc_streamoff = vb2_ioctl_streamoff,
> -	.vidioc_try_fmt_vid_cap_mplane = rkisp1_try_fmt_vid_cap_mplane,
> -	.vidioc_s_fmt_vid_cap_mplane = rkisp1_s_fmt_vid_cap_mplane,
> -	.vidioc_g_fmt_vid_cap_mplane = rkisp1_g_fmt_vid_cap_mplane,
> +	.vidioc_try_fmt_vid_cap_mplane = rkisp1_adj_fmt_vid_cap_mplane,
> +	.vidioc_s_fmt_vid_cap_mplane = rkisp1_adj_fmt_vid_cap_mplane,
> +	.vidioc_g_fmt_vid_cap_mplane = video_device_g_fmt_vid,
>  	.vidioc_enum_fmt_vid_cap = rkisp1_enum_fmt_vid_cap_mplane,
>  	.vidioc_enum_framesizes = rkisp1_enum_framesizes,
>  	.vidioc_querycap = rkisp1_querycap,
> @@ -1461,8 +1444,10 @@ static int rkisp1_capture_link_validate(struct media_link *link)
>  	struct v4l2_subdev *sd =
>  		media_entity_to_v4l2_subdev(link->source->entity);
>  	struct rkisp1_capture *cap = video_get_drvdata(vdev);
> +	const struct v4l2_pix_format_mplane *pixm =
> +		&vdev->state.vid_fmt.fmt.pix_mp;
>  	const struct rkisp1_capture_fmt_cfg *fmt =
> -		rkisp1_find_fmt_cfg(cap, cap->pix.fmt.pixelformat);
> +		rkisp1_find_fmt_cfg(cap, pixm->pixelformat);
>  	struct v4l2_subdev_format sd_fmt = {
>  		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
>  		.pad = link->source->index,
> @@ -1473,16 +1458,16 @@ static int rkisp1_capture_link_validate(struct media_link *link)
>  	if (ret)
>  		return ret;
>
> -	if (sd_fmt.format.height != cap->pix.fmt.height ||
> -	    sd_fmt.format.width != cap->pix.fmt.width ||
> +	if (sd_fmt.format.height != pixm->height ||
> +	    sd_fmt.format.width != pixm->width ||
>  	    sd_fmt.format.code != fmt->mbus) {
>  		dev_dbg(cap->rkisp1->dev,
>  			"link '%s':%u -> '%s':%u not valid: 0x%04x/%ux%u != 0x%04x/%ux%u\n",
>  			link->source->entity->name, link->source->index,
>  			link->sink->entity->name, link->sink->index,
>  			sd_fmt.format.code, sd_fmt.format.width,
> -			sd_fmt.format.height, fmt->mbus, cap->pix.fmt.width,
> -			cap->pix.fmt.height);
> +			sd_fmt.format.height, fmt->mbus, pixm->width,
> +			pixm->height);
>  		return -EPIPE;
>  	}
>
> @@ -1531,6 +1516,7 @@ static int rkisp1_register_capture(struct rkisp1_capture *cap)
>  	};
>  	struct v4l2_device *v4l2_dev = &cap->rkisp1->v4l2_dev;
>  	struct video_device *vdev = &cap->vnode.vdev;
> +	struct v4l2_pix_format_mplane *pixm;
>  	struct rkisp1_vdev_node *node;
>  	struct vb2_queue *q;
>  	int ret;
> @@ -1548,6 +1534,7 @@ static int rkisp1_register_capture(struct rkisp1_capture *cap)
>  	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE_MPLANE |
>  			    V4L2_CAP_STREAMING | V4L2_CAP_IO_MC;
>  	vdev->entity.ops = &rkisp1_media_ops;
> +	set_bit(V4L2_FL_USES_STATE, &vdev->flags);

I think we should introduce an helper in the core that allows drivers
to init the device state.

As suggested in the review of patch 1 this could look like

        video_device_state_alloc(size)

Where the framework allocates the state for the driver (allocating the
requested size) and then calls a driver's callback to initialize the
state.

I'm not 100% sure where the callback (analogue to subdev's
.init_state()) should live though.

struct video_device has
- const struct v4l2_file_operations *fops;
- const struct v4l2_ioctl_ops *ioctl_ops

and in none of these an .init_state() operation would fit well.

It also has
	/* callbacks */
	void (*release)(struct video_device *vdev);

we could add one
        int (*init_state)(struct video_device_state *state);

but it seems pretty rough done this way.

Another option, but I don't think it's right, it's
media_entity_operations.

What do you think ?

>  	video_set_drvdata(vdev, cap);
>  	vdev->vfl_dir = VFL_DIR_RX;
>  	node->pad.flags = MEDIA_PAD_FL_SINK;
> @@ -1572,6 +1559,13 @@ static int rkisp1_register_capture(struct rkisp1_capture *cap)
>
>  	vdev->queue = q;
>
> +	vdev->state.vid_fmt.type = q->type;
> +	pixm = &vdev->state.vid_fmt.fmt.pix_mp;
> +	pixm->pixelformat = V4L2_PIX_FMT_YUYV;
> +	pixm->width = RKISP1_DEFAULT_WIDTH;
> +	pixm->height = RKISP1_DEFAULT_HEIGHT;
> +	rkisp1_adj_fmt(cap, pixm, &cap->pix.cfg, &cap->pix.info);
> +
>  	ret = media_entity_pads_init(&vdev->entity, 1, &node->pad);
>  	if (ret)
>  		goto error;
> @@ -1598,7 +1592,6 @@ static void
>  rkisp1_capture_init(struct rkisp1_device *rkisp1, enum rkisp1_stream_id id)
>  {
>  	struct rkisp1_capture *cap = &rkisp1->capture_devs[id];
> -	struct v4l2_pix_format_mplane pixm;
>
>  	memset(cap, 0, sizeof(*cap));
>  	cap->id = id;
> @@ -1616,12 +1609,6 @@ rkisp1_capture_init(struct rkisp1_device *rkisp1, enum rkisp1_stream_id id)
>  	}
>
>  	cap->is_streaming = false;
> -
> -	memset(&pixm, 0, sizeof(pixm));
> -	pixm.pixelformat = V4L2_PIX_FMT_YUYV;
> -	pixm.width = RKISP1_DEFAULT_WIDTH;
> -	pixm.height = RKISP1_DEFAULT_HEIGHT;
> -	rkisp1_set_fmt(cap, &pixm);
>  }
>
>  int rkisp1_capture_devs_register(struct rkisp1_device *rkisp1)
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> index ca952fd0829ba7d923ad42fec92840ccd422b6e5..7c1556bc5980f937ff2c503282bb5623283bda1a 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> @@ -304,7 +304,6 @@ struct rkisp1_device;
>   *		  handler to stop the streaming by waiting on the 'done' wait queue.
>   *		  If the irq handler is not called, the stream is stopped by the callback
>   *		  after timeout.
> - * @stride:       the line stride for the first plane, in pixel units
>   * @buf.lock:	  lock to protect buf.queue
>   * @buf.queue:	  queued buffer list
>   * @buf.dummy:	  dummy space to store dropped data
> @@ -314,7 +313,6 @@ struct rkisp1_device;
>   * @buf.next:	  the buffer used for next frame
>   * @pix.cfg:	  pixel configuration
>   * @pix.info:	  a pointer to the v4l2_format_info of the pixel format
> - * @pix.fmt:	  buffer format
>   */
>  struct rkisp1_capture {
>  	struct rkisp1_vdev_node vnode;
> @@ -325,7 +323,6 @@ struct rkisp1_capture {
>  	bool is_streaming;
>  	bool is_stopping;
>  	wait_queue_head_t done;
> -	unsigned int stride;
>  	struct {
>  		/* protects queue, curr and next */
>  		spinlock_t lock;
> @@ -337,7 +334,6 @@ struct rkisp1_capture {
>  	struct {
>  		const struct rkisp1_capture_fmt_cfg *cfg;
>  		const struct v4l2_format_info *info;
> -		struct v4l2_pix_format_mplane fmt;
>  	} pix;
>  };
>
>
> --
> 2.49.0
>
>

Re: [PATCH RFC 3/3] media: rkisp1: Use video_device_state

[Jai Luthra]

Quoting Jacopo Mondi (2025-07-08 09:42:26)
> Hi Jai
> 
> On Thu, Jul 03, 2025 at 06:02:10PM -0700, Jai Luthra wrote:
> > Use the newly introduced video_device_state to store the active v4l2
> > format for the video device.
> >
> > Additionally, perform the stride calculation when required instead of
> > storing it in the driver context structure.
> >
> > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > ---
> >  .../platform/rockchip/rkisp1/rkisp1-capture.c      | 113 +++++++++------------
> >  .../media/platform/rockchip/rkisp1/rkisp1-common.h |   4 -
> >  2 files changed, 50 insertions(+), 67 deletions(-)
> >
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > index 6dcefd144d5abe358323e37ac6133c6134ac636e..f3f2a7c3c11319470f6619cb83a87d39ee21ba61 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/delay.h>
> >  #include <linux/pm_runtime.h>
> >  #include <media/v4l2-common.h>
> > +#include <media/v4l2-dev.h>
> >  #include <media/v4l2-event.h>
> >  #include <media/v4l2-fh.h>
> >  #include <media/v4l2-ioctl.h>
> > @@ -482,7 +483,9 @@ static void rkisp1_irq_frame_end_enable(struct rkisp1_capture *cap)
> >
> >  static void rkisp1_mp_config(struct rkisp1_capture *cap)
> >  {
> > -     const struct v4l2_pix_format_mplane *pixm = &cap->pix.fmt;
> > +     const struct v4l2_pix_format_mplane *pixm =
> > +             &cap->vnode.vdev.state.vid_fmt.fmt.pix_mp;
> 
> We'll need helpers to get the correct format given a video device
> 
> In example:
> struct v4l2_pix_format_mplane *video_device_state_get_mplane_fmt(vdev)
> 
> > +     u32 stride = pixm->plane_fmt[0].bytesperline / cap->pix.info->bpp[0];
> >       struct rkisp1_device *rkisp1 = cap->rkisp1;
> >       u32 reg;
> >
> > @@ -494,11 +497,11 @@ static void rkisp1_mp_config(struct rkisp1_capture *cap)
> >                    rkisp1_pixfmt_comp_size(pixm, RKISP1_PLANE_CR));
> >
> >       if (rkisp1_has_feature(rkisp1, MAIN_STRIDE)) {
> > -             rkisp1_write(rkisp1, RKISP1_CIF_MI_MP_Y_LLENGTH, cap->stride);
> > +             rkisp1_write(rkisp1, RKISP1_CIF_MI_MP_Y_LLENGTH, stride);
> >               rkisp1_write(rkisp1, RKISP1_CIF_MI_MP_Y_PIC_WIDTH, pixm->width);
> >               rkisp1_write(rkisp1, RKISP1_CIF_MI_MP_Y_PIC_HEIGHT, pixm->height);
> >               rkisp1_write(rkisp1, RKISP1_CIF_MI_MP_Y_PIC_SIZE,
> > -                          cap->stride * pixm->height);
> > +                          stride * pixm->height);
> >       }
> >
> >       rkisp1_irq_frame_end_enable(cap);
> > @@ -546,7 +549,9 @@ static void rkisp1_mp_config(struct rkisp1_capture *cap)
> >
> >  static void rkisp1_sp_config(struct rkisp1_capture *cap)
> >  {
> > -     const struct v4l2_pix_format_mplane *pixm = &cap->pix.fmt;
> > +     const struct v4l2_pix_format_mplane *pixm =
> > +             &cap->vnode.vdev.state.vid_fmt.fmt.pix_mp;
> > +     u32 stride = pixm->plane_fmt[0].bytesperline / cap->pix.info->bpp[0];
> >       struct rkisp1_device *rkisp1 = cap->rkisp1;
> >       u32 mi_ctrl, reg;
> >
> > @@ -557,11 +562,11 @@ static void rkisp1_sp_config(struct rkisp1_capture *cap)
> >       rkisp1_write(rkisp1, cap->config->mi.cr_size_init,
> >                    rkisp1_pixfmt_comp_size(pixm, RKISP1_PLANE_CR));
> >
> > -     rkisp1_write(rkisp1, RKISP1_CIF_MI_SP_Y_LLENGTH, cap->stride);
> > +     rkisp1_write(rkisp1, RKISP1_CIF_MI_SP_Y_LLENGTH, stride);
> >       rkisp1_write(rkisp1, RKISP1_CIF_MI_SP_Y_PIC_WIDTH, pixm->width);
> >       rkisp1_write(rkisp1, RKISP1_CIF_MI_SP_Y_PIC_HEIGHT, pixm->height);
> >       rkisp1_write(rkisp1, RKISP1_CIF_MI_SP_Y_PIC_SIZE,
> > -                  cap->stride * pixm->height);
> > +                  stride * pixm->height);
> >
> >       rkisp1_irq_frame_end_enable(cap);
> >
> > @@ -704,7 +709,8 @@ static const struct rkisp1_capture_ops rkisp1_capture_ops_sp = {
> >
> >  static int rkisp1_dummy_buf_create(struct rkisp1_capture *cap)
> >  {
> > -     const struct v4l2_pix_format_mplane *pixm = &cap->pix.fmt;
> > +     const struct v4l2_pix_format_mplane *pixm =
> > +             &cap->vnode.vdev.state.vid_fmt.fmt.pix_mp;
> >       struct rkisp1_dummy_buffer *dummy_buf = &cap->buf.dummy;
> >
> >       dummy_buf->size = max3(rkisp1_pixfmt_comp_size(pixm, RKISP1_PLANE_Y),
> > @@ -869,7 +875,8 @@ static int rkisp1_vb2_queue_setup(struct vb2_queue *queue,
> >                                 struct device *alloc_devs[])
> >  {
> >       struct rkisp1_capture *cap = queue->drv_priv;
> > -     const struct v4l2_pix_format_mplane *pixm = &cap->pix.fmt;
> > +     const struct v4l2_pix_format_mplane *pixm =
> > +             &cap->vnode.vdev.state.vid_fmt.fmt.pix_mp;
> >       unsigned int i;
> >
> >       if (*num_planes) {
> > @@ -894,7 +901,8 @@ static int rkisp1_vb2_buf_init(struct vb2_buffer *vb)
> >       struct rkisp1_buffer *ispbuf =
> >               container_of(vbuf, struct rkisp1_buffer, vb);
> >       struct rkisp1_capture *cap = vb->vb2_queue->drv_priv;
> > -     const struct v4l2_pix_format_mplane *pixm = &cap->pix.fmt;
> > +     const struct v4l2_pix_format_mplane *pixm =
> > +             &cap->vnode.vdev.state.vid_fmt.fmt.pix_mp;
> >       unsigned int i;
> >
> >       memset(ispbuf->buff_addr, 0, sizeof(ispbuf->buff_addr));
> > @@ -936,10 +944,12 @@ static void rkisp1_vb2_buf_queue(struct vb2_buffer *vb)
> >  static int rkisp1_vb2_buf_prepare(struct vb2_buffer *vb)
> >  {
> >       struct rkisp1_capture *cap = vb->vb2_queue->drv_priv;
> > +     const struct v4l2_pix_format_mplane *pixm =
> > +             &cap->vnode.vdev.state.vid_fmt.fmt.pix_mp;
> >       unsigned int i;
> >
> > -     for (i = 0; i < cap->pix.fmt.num_planes; i++) {
> > -             unsigned long size = cap->pix.fmt.plane_fmt[i].sizeimage;
> > +     for (i = 0; i < pixm->num_planes; i++) {
> > +             unsigned long size = pixm->plane_fmt[i].sizeimage;
> >
> >               if (vb2_plane_size(vb, i) < size) {
> >                       dev_err(cap->rkisp1->dev,
> > @@ -1278,7 +1288,7 @@ rkisp1_find_fmt_cfg(const struct rkisp1_capture *cap, const u32 pixelfmt)
> >       return NULL;
> >  }
> >
> > -static void rkisp1_try_fmt(const struct rkisp1_capture *cap,
> > +static void rkisp1_adj_fmt(const struct rkisp1_capture *cap,
> >                          struct v4l2_pix_format_mplane *pixm,
> >                          const struct rkisp1_capture_fmt_cfg **fmt_cfg,
> >                          const struct v4l2_format_info **fmt_info)
> > @@ -1317,22 +1327,20 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap,
> >               *fmt_info = info;
> >  }
> >
> > -static void rkisp1_set_fmt(struct rkisp1_capture *cap,
> > -                        struct v4l2_pix_format_mplane *pixm)
> > -{
> > -     rkisp1_try_fmt(cap, pixm, &cap->pix.cfg, &cap->pix.info);
> > -
> > -     cap->pix.fmt = *pixm;
> > -     cap->stride = pixm->plane_fmt[0].bytesperline / cap->pix.info->bpp[0];
> > -}
> > -
> > -static int rkisp1_try_fmt_vid_cap_mplane(struct file *file, void *fh,
> > +static int rkisp1_adj_fmt_vid_cap_mplane(struct file *file, void *priv,
> >                                        struct v4l2_format *f)
> >  {
> >       struct rkisp1_capture *cap = video_drvdata(file);
> > +     struct video_device_state *state = priv;
> >
> > -     rkisp1_try_fmt(cap, &f->fmt.pix_mp, NULL, NULL);
> > +     if (state->which == VIDEO_DEVICE_FORMAT_ACTIVE) {
> > +             rkisp1_adj_fmt(cap, &f->fmt.pix_mp, &cap->pix.cfg,
> > +                            &cap->pix.info);
> > +     } else {
> > +             rkisp1_adj_fmt(cap, &f->fmt.pix_mp, NULL, NULL);
> > +     }
> 
> This I'm not sure is desirable.
> 
> Applying the format on a video device happens when the streaming is
> started. IOW if you set fmt while the device is streaming you get back
> ebusy (and I think your first patch implement that already).
> 
> So do you need to update cap->pix here ? Or can you operate on a the
> v4l2_format stored in the state regardless of the
> FORMAT_ACTIVE/FORMAT_TRY and when streaming is started update the
> driver-specific info with the format info stored in the 'active'
> state ? This means that we'll need helpers that given a vdev return
> the 'active' format, ie the one stored in the state embedded in the
> video device.
> 

Ah yes, I didn't want to change too much in the driver so kept the function
as similar as possible.. but I think it would be feasible to fetch the
driver-specific config and v4l2_format_info when needed, instead of storing
it in the driver structure.

The majority of the uses I see for both are fairly contained, during buffer
init, and in the mp/sp_config functions.

> >
> > +     state->vid_fmt = *f;
> >       return 0;
> >  }
> >
> > @@ -1399,31 +1407,6 @@ static int rkisp1_enum_framesizes(struct file *file, void *fh,
> >       return 0;
> >  }
> >
> > -static int rkisp1_s_fmt_vid_cap_mplane(struct file *file,
> > -                                    void *priv, struct v4l2_format *f)
> > -{
> > -     struct rkisp1_capture *cap = video_drvdata(file);
> > -     struct rkisp1_vdev_node *node =
> > -                             rkisp1_vdev_to_node(&cap->vnode.vdev);
> > -
> > -     if (vb2_is_busy(&node->buf_queue))
> > -             return -EBUSY;
> > -
> > -     rkisp1_set_fmt(cap, &f->fmt.pix_mp);
> > -
> > -     return 0;
> > -}
> > -
> > -static int rkisp1_g_fmt_vid_cap_mplane(struct file *file, void *fh,
> > -                                    struct v4l2_format *f)
> > -{
> > -     struct rkisp1_capture *cap = video_drvdata(file);
> > -
> > -     f->fmt.pix_mp = cap->pix.fmt;
> > -
> > -     return 0;
> > -}
> > -
> >  static int
> >  rkisp1_querycap(struct file *file, void *priv, struct v4l2_capability *cap)
> >  {
> > @@ -1444,9 +1427,9 @@ static const struct v4l2_ioctl_ops rkisp1_v4l2_ioctl_ops = {
> >       .vidioc_prepare_buf = vb2_ioctl_prepare_buf,
> >       .vidioc_streamon = vb2_ioctl_streamon,
> >       .vidioc_streamoff = vb2_ioctl_streamoff,
> > -     .vidioc_try_fmt_vid_cap_mplane = rkisp1_try_fmt_vid_cap_mplane,
> > -     .vidioc_s_fmt_vid_cap_mplane = rkisp1_s_fmt_vid_cap_mplane,
> > -     .vidioc_g_fmt_vid_cap_mplane = rkisp1_g_fmt_vid_cap_mplane,
> > +     .vidioc_try_fmt_vid_cap_mplane = rkisp1_adj_fmt_vid_cap_mplane,
> > +     .vidioc_s_fmt_vid_cap_mplane = rkisp1_adj_fmt_vid_cap_mplane,
> > +     .vidioc_g_fmt_vid_cap_mplane = video_device_g_fmt_vid,
> >       .vidioc_enum_fmt_vid_cap = rkisp1_enum_fmt_vid_cap_mplane,
> >       .vidioc_enum_framesizes = rkisp1_enum_framesizes,
> >       .vidioc_querycap = rkisp1_querycap,
> > @@ -1461,8 +1444,10 @@ static int rkisp1_capture_link_validate(struct media_link *link)
> >       struct v4l2_subdev *sd =
> >               media_entity_to_v4l2_subdev(link->source->entity);
> >       struct rkisp1_capture *cap = video_get_drvdata(vdev);
> > +     const struct v4l2_pix_format_mplane *pixm =
> > +             &vdev->state.vid_fmt.fmt.pix_mp;
> >       const struct rkisp1_capture_fmt_cfg *fmt =
> > -             rkisp1_find_fmt_cfg(cap, cap->pix.fmt.pixelformat);
> > +             rkisp1_find_fmt_cfg(cap, pixm->pixelformat);
> >       struct v4l2_subdev_format sd_fmt = {
> >               .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> >               .pad = link->source->index,
> > @@ -1473,16 +1458,16 @@ static int rkisp1_capture_link_validate(struct media_link *link)
> >       if (ret)
> >               return ret;
> >
> > -     if (sd_fmt.format.height != cap->pix.fmt.height ||
> > -         sd_fmt.format.width != cap->pix.fmt.width ||
> > +     if (sd_fmt.format.height != pixm->height ||
> > +         sd_fmt.format.width != pixm->width ||
> >           sd_fmt.format.code != fmt->mbus) {
> >               dev_dbg(cap->rkisp1->dev,
> >                       "link '%s':%u -> '%s':%u not valid: 0x%04x/%ux%u != 0x%04x/%ux%u\n",
> >                       link->source->entity->name, link->source->index,
> >                       link->sink->entity->name, link->sink->index,
> >                       sd_fmt.format.code, sd_fmt.format.width,
> > -                     sd_fmt.format.height, fmt->mbus, cap->pix.fmt.width,
> > -                     cap->pix.fmt.height);
> > +                     sd_fmt.format.height, fmt->mbus, pixm->width,
> > +                     pixm->height);
> >               return -EPIPE;
> >       }
> >
> > @@ -1531,6 +1516,7 @@ static int rkisp1_register_capture(struct rkisp1_capture *cap)
> >       };
> >       struct v4l2_device *v4l2_dev = &cap->rkisp1->v4l2_dev;
> >       struct video_device *vdev = &cap->vnode.vdev;
> > +     struct v4l2_pix_format_mplane *pixm;
> >       struct rkisp1_vdev_node *node;
> >       struct vb2_queue *q;
> >       int ret;
> > @@ -1548,6 +1534,7 @@ static int rkisp1_register_capture(struct rkisp1_capture *cap)
> >       vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE_MPLANE |
> >                           V4L2_CAP_STREAMING | V4L2_CAP_IO_MC;
> >       vdev->entity.ops = &rkisp1_media_ops;
> > +     set_bit(V4L2_FL_USES_STATE, &vdev->flags);
> 
> I think we should introduce an helper in the core that allows drivers
> to init the device state.
> 
> As suggested in the review of patch 1 this could look like
> 
>         video_device_state_alloc(size)
> 
> Where the framework allocates the state for the driver (allocating the
> requested size) and then calls a driver's callback to initialize the
> state.

Makes sense.

> 
> I'm not 100% sure where the callback (analogue to subdev's
> .init_state()) should live though.
> 
> struct video_device has
> - const struct v4l2_file_operations *fops;
> - const struct v4l2_ioctl_ops *ioctl_ops
> 
> and in none of these an .init_state() operation would fit well.
> 
> It also has
>         /* callbacks */
>         void (*release)(struct video_device *vdev);
> 
> we could add one
>         int (*init_state)(struct video_device_state *state);
> 
> but it seems pretty rough done this way.
> 
> Another option, but I don't think it's right, it's
> media_entity_operations.
> 
> What do you think ?
> 

Hmm out of all of those options, a dedicated init_state callback in
video_device seems the best one to me... all the other ones seem out of
place.

> >       video_set_drvdata(vdev, cap);
> >       vdev->vfl_dir = VFL_DIR_RX;
> >       node->pad.flags = MEDIA_PAD_FL_SINK;
> > @@ -1572,6 +1559,13 @@ static int rkisp1_register_capture(struct rkisp1_capture *cap)
> >
> >       vdev->queue = q;
> >
> > +     vdev->state.vid_fmt.type = q->type;
> > +     pixm = &vdev->state.vid_fmt.fmt.pix_mp;
> > +     pixm->pixelformat = V4L2_PIX_FMT_YUYV;
> > +     pixm->width = RKISP1_DEFAULT_WIDTH;
> > +     pixm->height = RKISP1_DEFAULT_HEIGHT;
> > +     rkisp1_adj_fmt(cap, pixm, &cap->pix.cfg, &cap->pix.info);
> > +
> >       ret = media_entity_pads_init(&vdev->entity, 1, &node->pad);
> >       if (ret)
> >               goto error;
> > @@ -1598,7 +1592,6 @@ static void
> >  rkisp1_capture_init(struct rkisp1_device *rkisp1, enum rkisp1_stream_id id)
> >  {
> >       struct rkisp1_capture *cap = &rkisp1->capture_devs[id];
> > -     struct v4l2_pix_format_mplane pixm;
> >
> >       memset(cap, 0, sizeof(*cap));
> >       cap->id = id;
> > @@ -1616,12 +1609,6 @@ rkisp1_capture_init(struct rkisp1_device *rkisp1, enum rkisp1_stream_id id)
> >       }
> >
> >       cap->is_streaming = false;
> > -
> > -     memset(&pixm, 0, sizeof(pixm));
> > -     pixm.pixelformat = V4L2_PIX_FMT_YUYV;
> > -     pixm.width = RKISP1_DEFAULT_WIDTH;
> > -     pixm.height = RKISP1_DEFAULT_HEIGHT;
> > -     rkisp1_set_fmt(cap, &pixm);
> >  }
> >
> >  int rkisp1_capture_devs_register(struct rkisp1_device *rkisp1)
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > index ca952fd0829ba7d923ad42fec92840ccd422b6e5..7c1556bc5980f937ff2c503282bb5623283bda1a 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > @@ -304,7 +304,6 @@ struct rkisp1_device;
> >   *             handler to stop the streaming by waiting on the 'done' wait queue.
> >   *             If the irq handler is not called, the stream is stopped by the callback
> >   *             after timeout.
> > - * @stride:       the line stride for the first plane, in pixel units
> >   * @buf.lock:          lock to protect buf.queue
> >   * @buf.queue:         queued buffer list
> >   * @buf.dummy:         dummy space to store dropped data
> > @@ -314,7 +313,6 @@ struct rkisp1_device;
> >   * @buf.next:          the buffer used for next frame
> >   * @pix.cfg:   pixel configuration
> >   * @pix.info:          a pointer to the v4l2_format_info of the pixel format
> > - * @pix.fmt:   buffer format
> >   */
> >  struct rkisp1_capture {
> >       struct rkisp1_vdev_node vnode;
> > @@ -325,7 +323,6 @@ struct rkisp1_capture {
> >       bool is_streaming;
> >       bool is_stopping;
> >       wait_queue_head_t done;
> > -     unsigned int stride;
> >       struct {
> >               /* protects queue, curr and next */
> >               spinlock_t lock;
> > @@ -337,7 +334,6 @@ struct rkisp1_capture {
> >       struct {
> >               const struct rkisp1_capture_fmt_cfg *cfg;
> >               const struct v4l2_format_info *info;
> > -             struct v4l2_pix_format_mplane fmt;
> >       } pix;
> >  };
> >
> >
> > --
> > 2.49.0
> >
> >

Thanks,
Jai

Re: [PATCH RFC 3/3] media: rkisp1: Use video_device_state

[Laurent Pinchart]

On Fri, Jul 11, 2025 at 05:26:35PM -0700, Jai Luthra wrote:
> Quoting Jacopo Mondi (2025-07-08 09:42:26)
> > On Thu, Jul 03, 2025 at 06:02:10PM -0700, Jai Luthra wrote:
> > > Use the newly introduced video_device_state to store the active v4l2
> > > format for the video device.
> > >
> > > Additionally, perform the stride calculation when required instead of
> > > storing it in the driver context structure.
> > >
> > > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > > ---
> > >  .../platform/rockchip/rkisp1/rkisp1-capture.c      | 113 +++++++++------------
> > >  .../media/platform/rockchip/rkisp1/rkisp1-common.h |   4 -
> > >  2 files changed, 50 insertions(+), 67 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > > index 6dcefd144d5abe358323e37ac6133c6134ac636e..f3f2a7c3c11319470f6619cb83a87d39ee21ba61 100644
> > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > > @@ -11,6 +11,7 @@
> > >  #include <linux/delay.h>
> > >  #include <linux/pm_runtime.h>
> > >  #include <media/v4l2-common.h>
> > > +#include <media/v4l2-dev.h>
> > >  #include <media/v4l2-event.h>
> > >  #include <media/v4l2-fh.h>
> > >  #include <media/v4l2-ioctl.h>
> > > @@ -482,7 +483,9 @@ static void rkisp1_irq_frame_end_enable(struct rkisp1_capture *cap)
> > >
> > >  static void rkisp1_mp_config(struct rkisp1_capture *cap)
> > >  {
> > > -     const struct v4l2_pix_format_mplane *pixm = &cap->pix.fmt;
> > > +     const struct v4l2_pix_format_mplane *pixm =
> > > +             &cap->vnode.vdev.state.vid_fmt.fmt.pix_mp;
> > 
> > We'll need helpers to get the correct format given a video device
> > 
> > In example:
> > struct v4l2_pix_format_mplane *video_device_state_get_mplane_fmt(vdev)

As there should be a single format in the state, I'd add a helper to
access the v4l2_format, and let drivers extract fmt.pix_fmt themselves.
Also, the state helpers should operate on a state pointer.

Eventually the state should be passed all the way from the top-level to
the leaf functions such as rkisp1_mp_config(), but for the time being a
helper to get the active state can be introduced too. No driver should
*ever* access vdev->state or fh->state manually.

> > > +     u32 stride = pixm->plane_fmt[0].bytesperline / cap->pix.info->bpp[0];
> > >       struct rkisp1_device *rkisp1 = cap->rkisp1;
> > >       u32 reg;
> > >
> > > @@ -494,11 +497,11 @@ static void rkisp1_mp_config(struct rkisp1_capture *cap)
> > >                    rkisp1_pixfmt_comp_size(pixm, RKISP1_PLANE_CR));
> > >
> > >       if (rkisp1_has_feature(rkisp1, MAIN_STRIDE)) {
> > > -             rkisp1_write(rkisp1, RKISP1_CIF_MI_MP_Y_LLENGTH, cap->stride);
> > > +             rkisp1_write(rkisp1, RKISP1_CIF_MI_MP_Y_LLENGTH, stride);
> > >               rkisp1_write(rkisp1, RKISP1_CIF_MI_MP_Y_PIC_WIDTH, pixm->width);
> > >               rkisp1_write(rkisp1, RKISP1_CIF_MI_MP_Y_PIC_HEIGHT, pixm->height);
> > >               rkisp1_write(rkisp1, RKISP1_CIF_MI_MP_Y_PIC_SIZE,
> > > -                          cap->stride * pixm->height);
> > > +                          stride * pixm->height);
> > >       }
> > >
> > >       rkisp1_irq_frame_end_enable(cap);
> > > @@ -546,7 +549,9 @@ static void rkisp1_mp_config(struct rkisp1_capture *cap)
> > >
> > >  static void rkisp1_sp_config(struct rkisp1_capture *cap)
> > >  {
> > > -     const struct v4l2_pix_format_mplane *pixm = &cap->pix.fmt;
> > > +     const struct v4l2_pix_format_mplane *pixm =
> > > +             &cap->vnode.vdev.state.vid_fmt.fmt.pix_mp;
> > > +     u32 stride = pixm->plane_fmt[0].bytesperline / cap->pix.info->bpp[0];
> > >       struct rkisp1_device *rkisp1 = cap->rkisp1;
> > >       u32 mi_ctrl, reg;
> > >
> > > @@ -557,11 +562,11 @@ static void rkisp1_sp_config(struct rkisp1_capture *cap)
> > >       rkisp1_write(rkisp1, cap->config->mi.cr_size_init,
> > >                    rkisp1_pixfmt_comp_size(pixm, RKISP1_PLANE_CR));
> > >
> > > -     rkisp1_write(rkisp1, RKISP1_CIF_MI_SP_Y_LLENGTH, cap->stride);
> > > +     rkisp1_write(rkisp1, RKISP1_CIF_MI_SP_Y_LLENGTH, stride);
> > >       rkisp1_write(rkisp1, RKISP1_CIF_MI_SP_Y_PIC_WIDTH, pixm->width);
> > >       rkisp1_write(rkisp1, RKISP1_CIF_MI_SP_Y_PIC_HEIGHT, pixm->height);
> > >       rkisp1_write(rkisp1, RKISP1_CIF_MI_SP_Y_PIC_SIZE,
> > > -                  cap->stride * pixm->height);
> > > +                  stride * pixm->height);
> > >
> > >       rkisp1_irq_frame_end_enable(cap);
> > >
> > > @@ -704,7 +709,8 @@ static const struct rkisp1_capture_ops rkisp1_capture_ops_sp = {
> > >
> > >  static int rkisp1_dummy_buf_create(struct rkisp1_capture *cap)
> > >  {
> > > -     const struct v4l2_pix_format_mplane *pixm = &cap->pix.fmt;
> > > +     const struct v4l2_pix_format_mplane *pixm =
> > > +             &cap->vnode.vdev.state.vid_fmt.fmt.pix_mp;
> > >       struct rkisp1_dummy_buffer *dummy_buf = &cap->buf.dummy;
> > >
> > >       dummy_buf->size = max3(rkisp1_pixfmt_comp_size(pixm, RKISP1_PLANE_Y),
> > > @@ -869,7 +875,8 @@ static int rkisp1_vb2_queue_setup(struct vb2_queue *queue,
> > >                                 struct device *alloc_devs[])
> > >  {
> > >       struct rkisp1_capture *cap = queue->drv_priv;
> > > -     const struct v4l2_pix_format_mplane *pixm = &cap->pix.fmt;
> > > +     const struct v4l2_pix_format_mplane *pixm =
> > > +             &cap->vnode.vdev.state.vid_fmt.fmt.pix_mp;
> > >       unsigned int i;
> > >
> > >       if (*num_planes) {
> > > @@ -894,7 +901,8 @@ static int rkisp1_vb2_buf_init(struct vb2_buffer *vb)
> > >       struct rkisp1_buffer *ispbuf =
> > >               container_of(vbuf, struct rkisp1_buffer, vb);
> > >       struct rkisp1_capture *cap = vb->vb2_queue->drv_priv;
> > > -     const struct v4l2_pix_format_mplane *pixm = &cap->pix.fmt;
> > > +     const struct v4l2_pix_format_mplane *pixm =
> > > +             &cap->vnode.vdev.state.vid_fmt.fmt.pix_mp;
> > >       unsigned int i;
> > >
> > >       memset(ispbuf->buff_addr, 0, sizeof(ispbuf->buff_addr));
> > > @@ -936,10 +944,12 @@ static void rkisp1_vb2_buf_queue(struct vb2_buffer *vb)
> > >  static int rkisp1_vb2_buf_prepare(struct vb2_buffer *vb)
> > >  {
> > >       struct rkisp1_capture *cap = vb->vb2_queue->drv_priv;
> > > +     const struct v4l2_pix_format_mplane *pixm =
> > > +             &cap->vnode.vdev.state.vid_fmt.fmt.pix_mp;
> > >       unsigned int i;
> > >
> > > -     for (i = 0; i < cap->pix.fmt.num_planes; i++) {
> > > -             unsigned long size = cap->pix.fmt.plane_fmt[i].sizeimage;
> > > +     for (i = 0; i < pixm->num_planes; i++) {
> > > +             unsigned long size = pixm->plane_fmt[i].sizeimage;
> > >
> > >               if (vb2_plane_size(vb, i) < size) {
> > >                       dev_err(cap->rkisp1->dev,
> > > @@ -1278,7 +1288,7 @@ rkisp1_find_fmt_cfg(const struct rkisp1_capture *cap, const u32 pixelfmt)
> > >       return NULL;
> > >  }
> > >
> > > -static void rkisp1_try_fmt(const struct rkisp1_capture *cap,
> > > +static void rkisp1_adj_fmt(const struct rkisp1_capture *cap,
> > >                          struct v4l2_pix_format_mplane *pixm,
> > >                          const struct rkisp1_capture_fmt_cfg **fmt_cfg,
> > >                          const struct v4l2_format_info **fmt_info)
> > > @@ -1317,22 +1327,20 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap,
> > >               *fmt_info = info;
> > >  }
> > >
> > > -static void rkisp1_set_fmt(struct rkisp1_capture *cap,
> > > -                        struct v4l2_pix_format_mplane *pixm)
> > > -{
> > > -     rkisp1_try_fmt(cap, pixm, &cap->pix.cfg, &cap->pix.info);
> > > -
> > > -     cap->pix.fmt = *pixm;
> > > -     cap->stride = pixm->plane_fmt[0].bytesperline / cap->pix.info->bpp[0];
> > > -}
> > > -
> > > -static int rkisp1_try_fmt_vid_cap_mplane(struct file *file, void *fh,
> > > +static int rkisp1_adj_fmt_vid_cap_mplane(struct file *file, void *priv,
> > >                                        struct v4l2_format *f)
> > >  {
> > >       struct rkisp1_capture *cap = video_drvdata(file);
> > > +     struct video_device_state *state = priv;
> > >
> > > -     rkisp1_try_fmt(cap, &f->fmt.pix_mp, NULL, NULL);
> > > +     if (state->which == VIDEO_DEVICE_FORMAT_ACTIVE) {
> > > +             rkisp1_adj_fmt(cap, &f->fmt.pix_mp, &cap->pix.cfg,
> > > +                            &cap->pix.info);
> > > +     } else {
> > > +             rkisp1_adj_fmt(cap, &f->fmt.pix_mp, NULL, NULL);
> > > +     }
> > 
> > This I'm not sure is desirable.
> > 
> > Applying the format on a video device happens when the streaming is
> > started. IOW if you set fmt while the device is streaming you get back
> > ebusy (and I think your first patch implement that already).
> > 
> > So do you need to update cap->pix here ? Or can you operate on a the
> > v4l2_format stored in the state regardless of the
> > FORMAT_ACTIVE/FORMAT_TRY and when streaming is started update the
> > driver-specific info with the format info stored in the 'active'
> > state ? This means that we'll need helpers that given a vdev return
> > the 'active' format, ie the one stored in the state embedded in the
> > video device.
> 
> Ah yes, I didn't want to change too much in the driver so kept the function
> as similar as possible.. but I think it would be feasible to fetch the
> driver-specific config and v4l2_format_info when needed, instead of storing
> it in the driver structure.
> 
> The majority of the uses I see for both are fairly contained, during buffer
> init, and in the mp/sp_config functions.

They are used in the following functions:

- rkisp1_mp_config()
- rkisp1_sp_config()
- rkisp1_mp_enable()
- rkisp1_vb2_buf_init()
- rkisp1_set_next_buf()

The first three functions are called from rkisp1_cap_stream_enable(), so
the info and cfg could be looked up there, and passed to the functions.

rkisp1_vb2_buf_init() could look up info and cfg manually, but it could
be more efficient to move the swap to rkisp1_set_next_buf() (not
swapping in the buffer itself, just when writing to the cb_base_ad_init
and cr_base_ad_init registers) as that function will need to access
pix.cfg anyway.

As for rkisp1_set_next_buf(), as the function is called in interrupt
context, it would be best to avoid the cost of looking up the data based
on the format. I'd recommend moving the lookups and setting pix.cfg and
pix.fmt to stream start time, so that rkisp1_set_next_buf() will be able
to access them.

Or you could bite the bullet and implement support for subclassing the
state, in which case the driver could add the cfg and info fields to its
custom state. Accessing the active state from rkisp1_set_next_buf() will
however be possibly problematic due to locking, so I think keeping the
pix.cfg and pix.info storage and moving the lookup to stream start would
be the best solution.

> > >
> > > +     state->vid_fmt = *f;
> > >       return 0;
> > >  }
> > >
> > > @@ -1399,31 +1407,6 @@ static int rkisp1_enum_framesizes(struct file *file, void *fh,
> > >       return 0;
> > >  }
> > >
> > > -static int rkisp1_s_fmt_vid_cap_mplane(struct file *file,
> > > -                                    void *priv, struct v4l2_format *f)
> > > -{
> > > -     struct rkisp1_capture *cap = video_drvdata(file);
> > > -     struct rkisp1_vdev_node *node =
> > > -                             rkisp1_vdev_to_node(&cap->vnode.vdev);
> > > -
> > > -     if (vb2_is_busy(&node->buf_queue))
> > > -             return -EBUSY;
> > > -
> > > -     rkisp1_set_fmt(cap, &f->fmt.pix_mp);
> > > -
> > > -     return 0;
> > > -}
> > > -
> > > -static int rkisp1_g_fmt_vid_cap_mplane(struct file *file, void *fh,
> > > -                                    struct v4l2_format *f)
> > > -{
> > > -     struct rkisp1_capture *cap = video_drvdata(file);
> > > -
> > > -     f->fmt.pix_mp = cap->pix.fmt;
> > > -
> > > -     return 0;
> > > -}
> > > -
> > >  static int
> > >  rkisp1_querycap(struct file *file, void *priv, struct v4l2_capability *cap)
> > >  {
> > > @@ -1444,9 +1427,9 @@ static const struct v4l2_ioctl_ops rkisp1_v4l2_ioctl_ops = {
> > >       .vidioc_prepare_buf = vb2_ioctl_prepare_buf,
> > >       .vidioc_streamon = vb2_ioctl_streamon,
> > >       .vidioc_streamoff = vb2_ioctl_streamoff,
> > > -     .vidioc_try_fmt_vid_cap_mplane = rkisp1_try_fmt_vid_cap_mplane,
> > > -     .vidioc_s_fmt_vid_cap_mplane = rkisp1_s_fmt_vid_cap_mplane,
> > > -     .vidioc_g_fmt_vid_cap_mplane = rkisp1_g_fmt_vid_cap_mplane,
> > > +     .vidioc_try_fmt_vid_cap_mplane = rkisp1_adj_fmt_vid_cap_mplane,
> > > +     .vidioc_s_fmt_vid_cap_mplane = rkisp1_adj_fmt_vid_cap_mplane,
> > > +     .vidioc_g_fmt_vid_cap_mplane = video_device_g_fmt_vid,
> > >       .vidioc_enum_fmt_vid_cap = rkisp1_enum_fmt_vid_cap_mplane,
> > >       .vidioc_enum_framesizes = rkisp1_enum_framesizes,
> > >       .vidioc_querycap = rkisp1_querycap,
> > > @@ -1461,8 +1444,10 @@ static int rkisp1_capture_link_validate(struct media_link *link)
> > >       struct v4l2_subdev *sd =
> > >               media_entity_to_v4l2_subdev(link->source->entity);
> > >       struct rkisp1_capture *cap = video_get_drvdata(vdev);
> > > +     const struct v4l2_pix_format_mplane *pixm =
> > > +             &vdev->state.vid_fmt.fmt.pix_mp;
> > >       const struct rkisp1_capture_fmt_cfg *fmt =
> > > -             rkisp1_find_fmt_cfg(cap, cap->pix.fmt.pixelformat);
> > > +             rkisp1_find_fmt_cfg(cap, pixm->pixelformat);
> > >       struct v4l2_subdev_format sd_fmt = {
> > >               .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > >               .pad = link->source->index,
> > > @@ -1473,16 +1458,16 @@ static int rkisp1_capture_link_validate(struct media_link *link)
> > >       if (ret)
> > >               return ret;
> > >
> > > -     if (sd_fmt.format.height != cap->pix.fmt.height ||
> > > -         sd_fmt.format.width != cap->pix.fmt.width ||
> > > +     if (sd_fmt.format.height != pixm->height ||
> > > +         sd_fmt.format.width != pixm->width ||
> > >           sd_fmt.format.code != fmt->mbus) {
> > >               dev_dbg(cap->rkisp1->dev,
> > >                       "link '%s':%u -> '%s':%u not valid: 0x%04x/%ux%u != 0x%04x/%ux%u\n",
> > >                       link->source->entity->name, link->source->index,
> > >                       link->sink->entity->name, link->sink->index,
> > >                       sd_fmt.format.code, sd_fmt.format.width,
> > > -                     sd_fmt.format.height, fmt->mbus, cap->pix.fmt.width,
> > > -                     cap->pix.fmt.height);
> > > +                     sd_fmt.format.height, fmt->mbus, pixm->width,
> > > +                     pixm->height);
> > >               return -EPIPE;
> > >       }
> > >
> > > @@ -1531,6 +1516,7 @@ static int rkisp1_register_capture(struct rkisp1_capture *cap)
> > >       };
> > >       struct v4l2_device *v4l2_dev = &cap->rkisp1->v4l2_dev;
> > >       struct video_device *vdev = &cap->vnode.vdev;
> > > +     struct v4l2_pix_format_mplane *pixm;
> > >       struct rkisp1_vdev_node *node;
> > >       struct vb2_queue *q;
> > >       int ret;
> > > @@ -1548,6 +1534,7 @@ static int rkisp1_register_capture(struct rkisp1_capture *cap)
> > >       vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE_MPLANE |
> > >                           V4L2_CAP_STREAMING | V4L2_CAP_IO_MC;
> > >       vdev->entity.ops = &rkisp1_media_ops;
> > > +     set_bit(V4L2_FL_USES_STATE, &vdev->flags);
> > 
> > I think we should introduce an helper in the core that allows drivers
> > to init the device state.
> > 
> > As suggested in the review of patch 1 this could look like
> > 
> >         video_device_state_alloc(size)
> > 
> > Where the framework allocates the state for the driver (allocating the
> > requested size) and then calls a driver's callback to initialize the
> > state.
> 
> Makes sense.

I don't think you should call video_device_state_alloc() from drivers,
as you'll need to allocate try states the same way, and that will be
handled by the V4L2 core. A V4L2_FL_USES_STATE flag seems fine for now.
When we'll implement support for subclassing states, we will add
.alloc_state() and .destroy_state() operations (and possibly
.duplicate_state()), similar to DRM state handling.

> > I'm not 100% sure where the callback (analogue to subdev's
> > .init_state()) should live though.
> > 
> > struct video_device has
> > - const struct v4l2_file_operations *fops;
> > - const struct v4l2_ioctl_ops *ioctl_ops
> > 
> > and in none of these an .init_state() operation would fit well.
> > 
> > It also has
> >         /* callbacks */
> >         void (*release)(struct video_device *vdev);
> > 
> > we could add one
> >         int (*init_state)(struct video_device_state *state);
> > 
> > but it seems pretty rough done this way.
> > 
> > Another option, but I don't think it's right, it's
> > media_entity_operations.
> > 
> > What do you think ?
> 
> Hmm out of all of those options, a dedicated init_state callback in
> video_device seems the best one to me... all the other ones seem out of
> place.

I'd add a video_device_internal_ops structure for that, and store a
pointer to it in video_device.

> > >       video_set_drvdata(vdev, cap);
> > >       vdev->vfl_dir = VFL_DIR_RX;
> > >       node->pad.flags = MEDIA_PAD_FL_SINK;
> > > @@ -1572,6 +1559,13 @@ static int rkisp1_register_capture(struct rkisp1_capture *cap)
> > >
> > >       vdev->queue = q;
> > >
> > > +     vdev->state.vid_fmt.type = q->type;
> > > +     pixm = &vdev->state.vid_fmt.fmt.pix_mp;
> > > +     pixm->pixelformat = V4L2_PIX_FMT_YUYV;
> > > +     pixm->width = RKISP1_DEFAULT_WIDTH;
> > > +     pixm->height = RKISP1_DEFAULT_HEIGHT;
> > > +     rkisp1_adj_fmt(cap, pixm, &cap->pix.cfg, &cap->pix.info);
> > > +
> > >       ret = media_entity_pads_init(&vdev->entity, 1, &node->pad);
> > >       if (ret)
> > >               goto error;
> > > @@ -1598,7 +1592,6 @@ static void
> > >  rkisp1_capture_init(struct rkisp1_device *rkisp1, enum rkisp1_stream_id id)
> > >  {
> > >       struct rkisp1_capture *cap = &rkisp1->capture_devs[id];
> > > -     struct v4l2_pix_format_mplane pixm;
> > >
> > >       memset(cap, 0, sizeof(*cap));
> > >       cap->id = id;
> > > @@ -1616,12 +1609,6 @@ rkisp1_capture_init(struct rkisp1_device *rkisp1, enum rkisp1_stream_id id)
> > >       }
> > >
> > >       cap->is_streaming = false;
> > > -
> > > -     memset(&pixm, 0, sizeof(pixm));
> > > -     pixm.pixelformat = V4L2_PIX_FMT_YUYV;
> > > -     pixm.width = RKISP1_DEFAULT_WIDTH;
> > > -     pixm.height = RKISP1_DEFAULT_HEIGHT;
> > > -     rkisp1_set_fmt(cap, &pixm);
> > >  }
> > >
> > >  int rkisp1_capture_devs_register(struct rkisp1_device *rkisp1)
> > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > > index ca952fd0829ba7d923ad42fec92840ccd422b6e5..7c1556bc5980f937ff2c503282bb5623283bda1a 100644
> > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > > @@ -304,7 +304,6 @@ struct rkisp1_device;
> > >   *             handler to stop the streaming by waiting on the 'done' wait queue.
> > >   *             If the irq handler is not called, the stream is stopped by the callback
> > >   *             after timeout.
> > > - * @stride:       the line stride for the first plane, in pixel units
> > >   * @buf.lock:          lock to protect buf.queue
> > >   * @buf.queue:         queued buffer list
> > >   * @buf.dummy:         dummy space to store dropped data
> > > @@ -314,7 +313,6 @@ struct rkisp1_device;
> > >   * @buf.next:          the buffer used for next frame
> > >   * @pix.cfg:   pixel configuration
> > >   * @pix.info:          a pointer to the v4l2_format_info of the pixel format
> > > - * @pix.fmt:   buffer format
> > >   */
> > >  struct rkisp1_capture {
> > >       struct rkisp1_vdev_node vnode;
> > > @@ -325,7 +323,6 @@ struct rkisp1_capture {
> > >       bool is_streaming;
> > >       bool is_stopping;
> > >       wait_queue_head_t done;
> > > -     unsigned int stride;
> > >       struct {
> > >               /* protects queue, curr and next */
> > >               spinlock_t lock;
> > > @@ -337,7 +334,6 @@ struct rkisp1_capture {
> > >       struct {
> > >               const struct rkisp1_capture_fmt_cfg *cfg;
> > >               const struct v4l2_format_info *info;
> > > -             struct v4l2_pix_format_mplane fmt;
> > >       } pix;
> > >  };
> > >

-- 
Regards,

Laurent Pinchart