[PATCH v2 01/10] media: v4l2-core: Introduce state management for video devices

Jai Luthra posted 10 patches 1 week, 5 days ago
[PATCH v2 01/10] media: v4l2-core: Introduce state management for video devices
Posted by Jai Luthra 1 week, 5 days ago
Similar to V4L2 subdev states, introduce state support for video devices
to provide a centralized location for storing device state information.
This includes the current (active) pixelformat used by the device and
the temporary (try) pixelformat used during format negotiation. In the
future, this may be extended or subclassed by device drivers to store
their internal state variables.

Also introduce a flag for drivers that wish to use this state
management. When set, the framework automatically allocates the state
during device registration and stores a pointer to it within the
video_device structure.

This change aligns video devices with V4L2 subdevices by storing
hardware state in a common framework-allocated structure. This is the
first step towards enabling the multiplexing of the underlying hardware
by using different software "contexts", each represented by the combined
state of all video devices and V4L2 subdevices in a complex media graph.

Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
--
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Hans Verkuil <hverkuil@kernel.org>
Cc: Ricardo Ribalda <ribalda@chromium.org>
Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Ma Ke <make24@iscas.ac.cn>
Cc: Jai Luthra <jai.luthra@ideasonboard.com>
Cc: linux-media@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/media/v4l2-core/v4l2-dev.c | 27 +++++++++++++++++++++++++
 include/media/v4l2-dev.h           | 40 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index 10a126e50c1ca25b1bd0e9872571261acfc26b39..997255709448510fcd17b6de798a3df99cd7ea09 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -163,6 +163,27 @@ void video_device_release_empty(struct video_device *vdev)
 }
 EXPORT_SYMBOL(video_device_release_empty);
 
+struct video_device_state *
+__video_device_state_alloc(struct video_device *vdev)
+{
+	struct video_device_state *state =
+		kzalloc(sizeof(struct video_device_state), GFP_KERNEL);
+
+	if (!state)
+		return ERR_PTR(-ENOMEM);
+
+	state->vdev = vdev;
+
+	return state;
+}
+EXPORT_SYMBOL_GPL(__video_device_state_alloc);
+
+void __video_device_state_free(struct video_device_state *state)
+{
+	kfree(state);
+}
+EXPORT_SYMBOL_GPL(__video_device_state_free);
+
 static inline void video_get(struct video_device *vdev)
 {
 	get_device(&vdev->dev);
@@ -939,6 +960,10 @@ int __video_register_device(struct video_device *vdev,
 	spin_lock_init(&vdev->fh_lock);
 	INIT_LIST_HEAD(&vdev->fh_list);
 
+	/* state support */
+	if (test_bit(V4L2_FL_USES_STATE, &vdev->flags))
+		vdev->state = __video_device_state_alloc(vdev);
+
 	/* Part 1: check device type */
 	switch (type) {
 	case VFL_TYPE_VIDEO:
@@ -1127,6 +1152,8 @@ void video_unregister_device(struct video_device *vdev)
 	clear_bit(V4L2_FL_REGISTERED, &vdev->flags);
 	mutex_unlock(&videodev_lock);
 	v4l2_event_wake_all(vdev);
+	if (test_bit(V4L2_FL_USES_STATE, &vdev->flags))
+		__video_device_state_free(vdev->state);
 	device_unregister(&vdev->dev);
 }
 EXPORT_SYMBOL(video_unregister_device);
diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
index a213c3398dcf60be8c531df87bf40c56b4ad772d..57e4691ef467aa2b0782dd4b8357bd0670643293 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,17 @@ struct v4l2_file_operations {
 	int (*release) (struct file *);
 };
 
+/**
+ * struct video_device_state - Used for storing video device state information.
+ *
+ * @fmt: Format of the capture stream
+ * @vdev: Pointer to video device
+ */
+struct video_device_state {
+	struct v4l2_format fmt;
+	struct video_device *vdev;
+};
+
 /*
  * Newer version of video_device, handled by videodev2.c
  *	This version moves redundant code from video device code to
@@ -238,6 +255,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 +301,7 @@ struct video_device {
 	struct vb2_queue *queue;
 
 	struct v4l2_prio_state *prio;
+	struct video_device_state *state;
 
 	/* device info */
 	char name[64];
@@ -546,6 +565,27 @@ static inline int video_is_registered(struct video_device *vdev)
 	return test_bit(V4L2_FL_REGISTERED, &vdev->flags);
 }
 
+/** __video_device_state_alloc - allocate video device state structure
+ *
+ * @vdev: pointer to struct video_device
+ *
+ * .. note::
+ *
+ *	This function is meant to be used only inside the V4L2 core.
+ */
+struct video_device_state *
+__video_device_state_alloc(struct video_device *vdev);
+
+/** __video_device_state_free - free video device state structure
+ *
+ * @state: pointer to the state to be freed
+ *
+ * .. note::
+ *
+ *	This function is meant to be used only inside the V4L2 core.
+ */
+void __video_device_state_free(struct video_device_state *state);
+
 /**
  * v4l2_debugfs_root - returns the dentry of the top-level "v4l2" debugfs dir
  *

-- 
2.51.0
Re: [PATCH v2 01/10] media: v4l2-core: Introduce state management for video devices
Posted by Mauro Carvalho Chehab 1 week ago
Em Fri, 19 Sep 2025 15:25:53 +0530
Jai Luthra <jai.luthra@ideasonboard.com> escreveu:

> Similar to V4L2 subdev states, introduce state support for video devices
> to provide a centralized location for storing device state information.
> This includes the current (active) pixelformat used by the device and
> the temporary (try) pixelformat used during format negotiation.

I didn't look at the patch series yet, so I may be just jumping too
fast here, but storing "try" attempts doesn't seem the right thing to
do.

Btw, IMHO, the first patch on this series should be against documentation,
where you would be describing not only the new feature with the supported 
states, but also the state machine transitions that are supported,
preferably with some graphs.

So, before actually looking on any code changes, I'd like to see a clear
description of this new feature, what it is proposing to address, how
and what impacts (if any) this would bring to userspace.

The current diffstat:

 include/media/v4l2-ctrls.h                         |   5 +-
 include/media/v4l2-dev.h                           |  84 +++++
 include/media/v4l2-fh.h                            |   2 +
 include/media/v4l2-ioctl.h                         | 238 ++++++-------
 include/media/v4l2-mem2mem.h                       |  48 ++-
 include/media/videobuf2-v4l2.h                     |  33 +-

implies that this affects for now only Documentation/driver-api/media/...

> In the
> future, this may be extended or subclassed by device drivers to store
> their internal state variables.
> 
> Also introduce a flag for drivers that wish to use this state
> management. When set, the framework automatically allocates the state
> during device registration and stores a pointer to it within the
> video_device structure.
> 
> This change aligns video devices with V4L2 subdevices by storing
> hardware state in a common framework-allocated structure. This is the
> first step towards enabling the multiplexing of the underlying hardware
> by using different software "contexts", each represented by the combined
> state of all video devices and V4L2 subdevices in a complex media graph.

... but when you mention "contexts", I'm assuming that you're aiming
at either userspace API changes and/or behavoral changes that will
affect uAPI as well.

Thanks,
Mauro
Re: [PATCH v2 01/10] media: v4l2-core: Introduce state management for video devices
Posted by Jai Luthra 2 days, 14 hours ago
Hi Mauro,

Quoting Mauro Carvalho Chehab (2025-09-24 15:45:45)
> Em Fri, 19 Sep 2025 15:25:53 +0530
> Jai Luthra <jai.luthra@ideasonboard.com> escreveu:
> 
> > Similar to V4L2 subdev states, introduce state support for video devices
> > to provide a centralized location for storing device state information.
> > This includes the current (active) pixelformat used by the device and
> > the temporary (try) pixelformat used during format negotiation.
> 
> I didn't look at the patch series yet, so I may be just jumping too
> fast here, but storing "try" attempts doesn't seem the right thing to
> do.
> 
> Btw, IMHO, the first patch on this series should be against documentation,
> where you would be describing not only the new feature with the supported 
> states, but also the state machine transitions that are supported,
> preferably with some graphs.
> 
> So, before actually looking on any code changes, I'd like to see a clear
> description of this new feature, what it is proposing to address, how
> and what impacts (if any) this would bring to userspace.
> 
> The current diffstat:
> 
>  include/media/v4l2-ctrls.h                         |   5 +-
>  include/media/v4l2-dev.h                           |  84 +++++
>  include/media/v4l2-fh.h                            |   2 +
>  include/media/v4l2-ioctl.h                         | 238 ++++++-------
>  include/media/v4l2-mem2mem.h                       |  48 ++-
>  include/media/videobuf2-v4l2.h                     |  33 +-
> 
> implies that this affects for now only Documentation/driver-api/media/...

There shouldn't be any change to userspace with this series. The state
structure introduced here is only for internal use by the drivers, which
currently store the applied formats in driver-specific structures.

In the next revision, I will add documentation in v4l2-dev.rst similar to
how the subdev state is described in v4l2-subdev.rst.

> 
> > In the
> > future, this may be extended or subclassed by device drivers to store
> > their internal state variables.
> > 
> > Also introduce a flag for drivers that wish to use this state
> > management. When set, the framework automatically allocates the state
> > during device registration and stores a pointer to it within the
> > video_device structure.
> > 
> > This change aligns video devices with V4L2 subdevices by storing
> > hardware state in a common framework-allocated structure. This is the
> > first step towards enabling the multiplexing of the underlying hardware
> > by using different software "contexts", each represented by the combined
> > state of all video devices and V4L2 subdevices in a complex media graph.
> 
> ... but when you mention "contexts", I'm assuming that you're aiming
> at either userspace API changes and/or behavoral changes that will
> affect uAPI as well.

Yes, the userspace side changes are documented in Jacopo's series adding
context support for media devices, for example:

https://lore.kernel.org/linux-media/20250724-multicontext-mainline-2025-v2-7-c9b316773486@ideasonboard.com/

> 
> Thanks,
> Mauro

Thanks,
Jai
Re: [PATCH v2 01/10] media: v4l2-core: Introduce state management for video devices
Posted by Hans Verkuil 1 week, 3 days ago
Hi Jai,

Apologies that I had no time to review v1, but I'll review v2 today.

On 19/09/2025 11:55, Jai Luthra wrote:
> Similar to V4L2 subdev states, introduce state support for video devices
> to provide a centralized location for storing device state information.
> This includes the current (active) pixelformat used by the device and
> the temporary (try) pixelformat used during format negotiation. In the
> future, this may be extended or subclassed by device drivers to store
> their internal state variables.
> 
> Also introduce a flag for drivers that wish to use this state
> management. When set, the framework automatically allocates the state
> during device registration and stores a pointer to it within the
> video_device structure.
> 
> This change aligns video devices with V4L2 subdevices by storing
> hardware state in a common framework-allocated structure. This is the
> first step towards enabling the multiplexing of the underlying hardware
> by using different software "contexts", each represented by the combined
> state of all video devices and V4L2 subdevices in a complex media graph.
> 
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> --
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: Hans Verkuil <hverkuil@kernel.org>
> Cc: Ricardo Ribalda <ribalda@chromium.org>
> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Ma Ke <make24@iscas.ac.cn>
> Cc: Jai Luthra <jai.luthra@ideasonboard.com>
> Cc: linux-media@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/media/v4l2-core/v4l2-dev.c | 27 +++++++++++++++++++++++++
>  include/media/v4l2-dev.h           | 40 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 67 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index 10a126e50c1ca25b1bd0e9872571261acfc26b39..997255709448510fcd17b6de798a3df99cd7ea09 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -163,6 +163,27 @@ void video_device_release_empty(struct video_device *vdev)
>  }
>  EXPORT_SYMBOL(video_device_release_empty);
>  
> +struct video_device_state *
> +__video_device_state_alloc(struct video_device *vdev)
> +{
> +	struct video_device_state *state =
> +		kzalloc(sizeof(struct video_device_state), GFP_KERNEL);
> +
> +	if (!state)
> +		return ERR_PTR(-ENOMEM);
> +
> +	state->vdev = vdev;
> +
> +	return state;
> +}
> +EXPORT_SYMBOL_GPL(__video_device_state_alloc);
> +
> +void __video_device_state_free(struct video_device_state *state)
> +{
> +	kfree(state);
> +}
> +EXPORT_SYMBOL_GPL(__video_device_state_free);
> +
>  static inline void video_get(struct video_device *vdev)
>  {
>  	get_device(&vdev->dev);
> @@ -939,6 +960,10 @@ int __video_register_device(struct video_device *vdev,
>  	spin_lock_init(&vdev->fh_lock);
>  	INIT_LIST_HEAD(&vdev->fh_list);
>  
> +	/* state support */
> +	if (test_bit(V4L2_FL_USES_STATE, &vdev->flags))
> +		vdev->state = __video_device_state_alloc(vdev);
> +
>  	/* Part 1: check device type */
>  	switch (type) {
>  	case VFL_TYPE_VIDEO:
> @@ -1127,6 +1152,8 @@ void video_unregister_device(struct video_device *vdev)
>  	clear_bit(V4L2_FL_REGISTERED, &vdev->flags);
>  	mutex_unlock(&videodev_lock);
>  	v4l2_event_wake_all(vdev);
> +	if (test_bit(V4L2_FL_USES_STATE, &vdev->flags))
> +		__video_device_state_free(vdev->state);
>  	device_unregister(&vdev->dev);
>  }
>  EXPORT_SYMBOL(video_unregister_device);
> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> index a213c3398dcf60be8c531df87bf40c56b4ad772d..57e4691ef467aa2b0782dd4b8357bd0670643293 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,17 @@ struct v4l2_file_operations {
>  	int (*release) (struct file *);
>  };
>  
> +/**
> + * struct video_device_state - Used for storing video device state information.
> + *
> + * @fmt: Format of the capture stream
> + * @vdev: Pointer to video device
> + */
> +struct video_device_state {
> +	struct v4l2_format fmt;

While typically a video_device supports only a single video format type, that is
not always the case. There are the following exceptions:

1) M2M devices have both a capture and output video format. However, for M2M devices
   the state is per-filehandle, so it shouldn't be stored in a video_device_state
   struct anyway.
2) VBI devices can have both a raw and sliced VBI format (either capture or output)
3) AFAIK non-M2M video devices can have both a video and meta format. That may have
   changed, I'm not 100% certain about this.
4) video devices can also support an OVERLAY or OUTPUT_OVERLAY format (rare)

V4L2_CAP_VIDEO_OVERLAY is currently only used in
drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c, so once that driver
disappears we can drop video overlay support for capture devices.

2-4 are all quite rare, but 1 is very common. But for such devices the state
wouldn't be in video_device anyway.

But it would be nice if the same struct can be used in both m2m devices and non-m2m
devices. It's just stored either in struct v4l2_fh or struct video_device. It would
give a lot of opportunities for creating helper functions to make the life for
driver developers easier.

Regards,

	Hans

> +	struct video_device *vdev;
> +};
> +
>  /*
>   * Newer version of video_device, handled by videodev2.c
>   *	This version moves redundant code from video device code to
> @@ -238,6 +255,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 +301,7 @@ struct video_device {
>  	struct vb2_queue *queue;
>  
>  	struct v4l2_prio_state *prio;
> +	struct video_device_state *state;
>  
>  	/* device info */
>  	char name[64];
> @@ -546,6 +565,27 @@ static inline int video_is_registered(struct video_device *vdev)
>  	return test_bit(V4L2_FL_REGISTERED, &vdev->flags);
>  }
>  
> +/** __video_device_state_alloc - allocate video device state structure
> + *
> + * @vdev: pointer to struct video_device
> + *
> + * .. note::
> + *
> + *	This function is meant to be used only inside the V4L2 core.
> + */
> +struct video_device_state *
> +__video_device_state_alloc(struct video_device *vdev);
> +
> +/** __video_device_state_free - free video device state structure
> + *
> + * @state: pointer to the state to be freed
> + *
> + * .. note::
> + *
> + *	This function is meant to be used only inside the V4L2 core.
> + */
> +void __video_device_state_free(struct video_device_state *state);
> +
>  /**
>   * v4l2_debugfs_root - returns the dentry of the top-level "v4l2" debugfs dir
>   *
>
Re: [PATCH v2 01/10] media: v4l2-core: Introduce state management for video devices
Posted by Hans Verkuil 1 week, 1 day ago
On 22/09/2025 09:44, Hans Verkuil wrote:
> Hi Jai,
> 
> Apologies that I had no time to review v1, but I'll review v2 today.
> 
> On 19/09/2025 11:55, Jai Luthra wrote:
>> Similar to V4L2 subdev states, introduce state support for video devices
>> to provide a centralized location for storing device state information.
>> This includes the current (active) pixelformat used by the device and
>> the temporary (try) pixelformat used during format negotiation. In the
>> future, this may be extended or subclassed by device drivers to store
>> their internal state variables.
>>
>> Also introduce a flag for drivers that wish to use this state
>> management. When set, the framework automatically allocates the state
>> during device registration and stores a pointer to it within the
>> video_device structure.
>>
>> This change aligns video devices with V4L2 subdevices by storing
>> hardware state in a common framework-allocated structure. This is the
>> first step towards enabling the multiplexing of the underlying hardware
>> by using different software "contexts", each represented by the combined
>> state of all video devices and V4L2 subdevices in a complex media graph.
>>
>> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
>> --
>> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
>> Cc: Hans Verkuil <hverkuil@kernel.org>
>> Cc: Ricardo Ribalda <ribalda@chromium.org>
>> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>> Cc: Al Viro <viro@zeniv.linux.org.uk>
>> Cc: Ma Ke <make24@iscas.ac.cn>
>> Cc: Jai Luthra <jai.luthra@ideasonboard.com>
>> Cc: linux-media@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> ---
>>  drivers/media/v4l2-core/v4l2-dev.c | 27 +++++++++++++++++++++++++
>>  include/media/v4l2-dev.h           | 40 ++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 67 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>> index 10a126e50c1ca25b1bd0e9872571261acfc26b39..997255709448510fcd17b6de798a3df99cd7ea09 100644
>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>> @@ -163,6 +163,27 @@ void video_device_release_empty(struct video_device *vdev)
>>  }
>>  EXPORT_SYMBOL(video_device_release_empty);
>>  
>> +struct video_device_state *
>> +__video_device_state_alloc(struct video_device *vdev)
>> +{
>> +	struct video_device_state *state =
>> +		kzalloc(sizeof(struct video_device_state), GFP_KERNEL);
>> +
>> +	if (!state)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	state->vdev = vdev;
>> +
>> +	return state;
>> +}
>> +EXPORT_SYMBOL_GPL(__video_device_state_alloc);
>> +
>> +void __video_device_state_free(struct video_device_state *state)
>> +{
>> +	kfree(state);
>> +}
>> +EXPORT_SYMBOL_GPL(__video_device_state_free);
>> +
>>  static inline void video_get(struct video_device *vdev)
>>  {
>>  	get_device(&vdev->dev);
>> @@ -939,6 +960,10 @@ int __video_register_device(struct video_device *vdev,
>>  	spin_lock_init(&vdev->fh_lock);
>>  	INIT_LIST_HEAD(&vdev->fh_list);
>>  
>> +	/* state support */
>> +	if (test_bit(V4L2_FL_USES_STATE, &vdev->flags))
>> +		vdev->state = __video_device_state_alloc(vdev);
>> +
>>  	/* Part 1: check device type */
>>  	switch (type) {
>>  	case VFL_TYPE_VIDEO:
>> @@ -1127,6 +1152,8 @@ void video_unregister_device(struct video_device *vdev)
>>  	clear_bit(V4L2_FL_REGISTERED, &vdev->flags);
>>  	mutex_unlock(&videodev_lock);
>>  	v4l2_event_wake_all(vdev);
>> +	if (test_bit(V4L2_FL_USES_STATE, &vdev->flags))
>> +		__video_device_state_free(vdev->state);
>>  	device_unregister(&vdev->dev);
>>  }
>>  EXPORT_SYMBOL(video_unregister_device);
>> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
>> index a213c3398dcf60be8c531df87bf40c56b4ad772d..57e4691ef467aa2b0782dd4b8357bd0670643293 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,17 @@ struct v4l2_file_operations {
>>  	int (*release) (struct file *);
>>  };
>>  
>> +/**
>> + * struct video_device_state - Used for storing video device state information.
>> + *
>> + * @fmt: Format of the capture stream
>> + * @vdev: Pointer to video device
>> + */
>> +struct video_device_state {
>> +	struct v4l2_format fmt;
> 
> While typically a video_device supports only a single video format type, that is
> not always the case. There are the following exceptions:
> 
> 1) M2M devices have both a capture and output video format. However, for M2M devices
>    the state is per-filehandle, so it shouldn't be stored in a video_device_state
>    struct anyway.
> 2) VBI devices can have both a raw and sliced VBI format (either capture or output)

This is actually wrong. VBI devices are either in raw or in sliced VBI mode, you can't
have both at the same time. So a single v4l2_format in the state is fine for this.

Regards,

	Hans

> 3) AFAIK non-M2M video devices can have both a video and meta format. That may have
>    changed, I'm not 100% certain about this.
> 4) video devices can also support an OVERLAY or OUTPUT_OVERLAY format (rare)
> 
> V4L2_CAP_VIDEO_OVERLAY is currently only used in
> drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c, so once that driver
> disappears we can drop video overlay support for capture devices.
> 
> 2-4 are all quite rare, but 1 is very common. But for such devices the state
> wouldn't be in video_device anyway.
> 
> But it would be nice if the same struct can be used in both m2m devices and non-m2m
> devices. It's just stored either in struct v4l2_fh or struct video_device. It would
> give a lot of opportunities for creating helper functions to make the life for
> driver developers easier.
> 
> Regards,
> 
> 	Hans
> 
>> +	struct video_device *vdev;
>> +};
>> +
>>  /*
>>   * Newer version of video_device, handled by videodev2.c
>>   *	This version moves redundant code from video device code to
>> @@ -238,6 +255,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 +301,7 @@ struct video_device {
>>  	struct vb2_queue *queue;
>>  
>>  	struct v4l2_prio_state *prio;
>> +	struct video_device_state *state;
>>  
>>  	/* device info */
>>  	char name[64];
>> @@ -546,6 +565,27 @@ static inline int video_is_registered(struct video_device *vdev)
>>  	return test_bit(V4L2_FL_REGISTERED, &vdev->flags);
>>  }
>>  
>> +/** __video_device_state_alloc - allocate video device state structure
>> + *
>> + * @vdev: pointer to struct video_device
>> + *
>> + * .. note::
>> + *
>> + *	This function is meant to be used only inside the V4L2 core.
>> + */
>> +struct video_device_state *
>> +__video_device_state_alloc(struct video_device *vdev);
>> +
>> +/** __video_device_state_free - free video device state structure
>> + *
>> + * @state: pointer to the state to be freed
>> + *
>> + * .. note::
>> + *
>> + *	This function is meant to be used only inside the V4L2 core.
>> + */
>> +void __video_device_state_free(struct video_device_state *state);
>> +
>>  /**
>>   * v4l2_debugfs_root - returns the dentry of the top-level "v4l2" debugfs dir
>>   *
>>
> 
>
Re: [PATCH v2 01/10] media: v4l2-core: Introduce state management for video devices
Posted by Hans Verkuil 1 week, 2 days ago
On 22/09/2025 09:44, Hans Verkuil wrote:
> Hi Jai,
> 
> Apologies that I had no time to review v1, but I'll review v2 today.
> 
> On 19/09/2025 11:55, Jai Luthra wrote:
>> Similar to V4L2 subdev states, introduce state support for video devices
>> to provide a centralized location for storing device state information.
>> This includes the current (active) pixelformat used by the device and
>> the temporary (try) pixelformat used during format negotiation. In the
>> future, this may be extended or subclassed by device drivers to store
>> their internal state variables.
>>
>> Also introduce a flag for drivers that wish to use this state
>> management. When set, the framework automatically allocates the state
>> during device registration and stores a pointer to it within the
>> video_device structure.
>>
>> This change aligns video devices with V4L2 subdevices by storing
>> hardware state in a common framework-allocated structure. This is the
>> first step towards enabling the multiplexing of the underlying hardware
>> by using different software "contexts", each represented by the combined
>> state of all video devices and V4L2 subdevices in a complex media graph.
>>
>> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
>> --
>> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
>> Cc: Hans Verkuil <hverkuil@kernel.org>
>> Cc: Ricardo Ribalda <ribalda@chromium.org>
>> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>> Cc: Al Viro <viro@zeniv.linux.org.uk>
>> Cc: Ma Ke <make24@iscas.ac.cn>
>> Cc: Jai Luthra <jai.luthra@ideasonboard.com>
>> Cc: linux-media@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> ---
>>  drivers/media/v4l2-core/v4l2-dev.c | 27 +++++++++++++++++++++++++
>>  include/media/v4l2-dev.h           | 40 ++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 67 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>> index 10a126e50c1ca25b1bd0e9872571261acfc26b39..997255709448510fcd17b6de798a3df99cd7ea09 100644
>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>> @@ -163,6 +163,27 @@ void video_device_release_empty(struct video_device *vdev)
>>  }
>>  EXPORT_SYMBOL(video_device_release_empty);
>>  
>> +struct video_device_state *
>> +__video_device_state_alloc(struct video_device *vdev)
>> +{
>> +	struct video_device_state *state =
>> +		kzalloc(sizeof(struct video_device_state), GFP_KERNEL);
>> +
>> +	if (!state)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	state->vdev = vdev;
>> +
>> +	return state;
>> +}
>> +EXPORT_SYMBOL_GPL(__video_device_state_alloc);
>> +
>> +void __video_device_state_free(struct video_device_state *state)
>> +{
>> +	kfree(state);
>> +}
>> +EXPORT_SYMBOL_GPL(__video_device_state_free);
>> +
>>  static inline void video_get(struct video_device *vdev)
>>  {
>>  	get_device(&vdev->dev);
>> @@ -939,6 +960,10 @@ int __video_register_device(struct video_device *vdev,
>>  	spin_lock_init(&vdev->fh_lock);
>>  	INIT_LIST_HEAD(&vdev->fh_list);
>>  
>> +	/* state support */
>> +	if (test_bit(V4L2_FL_USES_STATE, &vdev->flags))
>> +		vdev->state = __video_device_state_alloc(vdev);
>> +
>>  	/* Part 1: check device type */
>>  	switch (type) {
>>  	case VFL_TYPE_VIDEO:
>> @@ -1127,6 +1152,8 @@ void video_unregister_device(struct video_device *vdev)
>>  	clear_bit(V4L2_FL_REGISTERED, &vdev->flags);
>>  	mutex_unlock(&videodev_lock);
>>  	v4l2_event_wake_all(vdev);
>> +	if (test_bit(V4L2_FL_USES_STATE, &vdev->flags))
>> +		__video_device_state_free(vdev->state);
>>  	device_unregister(&vdev->dev);
>>  }
>>  EXPORT_SYMBOL(video_unregister_device);
>> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
>> index a213c3398dcf60be8c531df87bf40c56b4ad772d..57e4691ef467aa2b0782dd4b8357bd0670643293 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,17 @@ struct v4l2_file_operations {
>>  	int (*release) (struct file *);
>>  };
>>  
>> +/**
>> + * struct video_device_state - Used for storing video device state information.
>> + *
>> + * @fmt: Format of the capture stream
>> + * @vdev: Pointer to video device
>> + */
>> +struct video_device_state {
>> +	struct v4l2_format fmt;
> 
> While typically a video_device supports only a single video format type, that is
> not always the case. There are the following exceptions:
> 
> 1) M2M devices have both a capture and output video format. However, for M2M devices
>    the state is per-filehandle, so it shouldn't be stored in a video_device_state
>    struct anyway.
> 2) VBI devices can have both a raw and sliced VBI format (either capture or output)
> 3) AFAIK non-M2M video devices can have both a video and meta format. That may have
>    changed, I'm not 100% certain about this.
> 4) video devices can also support an OVERLAY or OUTPUT_OVERLAY format (rare)
> 
> V4L2_CAP_VIDEO_OVERLAY is currently only used in
> drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c, so once that driver
> disappears we can drop video overlay support for capture devices.
> 
> 2-4 are all quite rare, but 1 is very common. But for such devices the state
> wouldn't be in video_device anyway.
> 
> But it would be nice if the same struct can be used in both m2m devices and non-m2m
> devices. It's just stored either in struct v4l2_fh or struct video_device. It would
> give a lot of opportunities for creating helper functions to make the life for
> driver developers easier.

Follow-up: assuming we want to support M2M devices as well (I think we should), then
consider renaming video_device_state since it isn't video_device specific, i.e. it
can either live in video_device or in v4l2_fh, and in the latter case you'd have
two instances: capture and output state.

Regards,

	Hans

> 
> Regards,
> 
> 	Hans
> 
>> +	struct video_device *vdev;
>> +};
>> +
>>  /*
>>   * Newer version of video_device, handled by videodev2.c
>>   *	This version moves redundant code from video device code to
>> @@ -238,6 +255,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 +301,7 @@ struct video_device {
>>  	struct vb2_queue *queue;
>>  
>>  	struct v4l2_prio_state *prio;
>> +	struct video_device_state *state;
>>  
>>  	/* device info */
>>  	char name[64];
>> @@ -546,6 +565,27 @@ static inline int video_is_registered(struct video_device *vdev)
>>  	return test_bit(V4L2_FL_REGISTERED, &vdev->flags);
>>  }
>>  
>> +/** __video_device_state_alloc - allocate video device state structure
>> + *
>> + * @vdev: pointer to struct video_device
>> + *
>> + * .. note::
>> + *
>> + *	This function is meant to be used only inside the V4L2 core.
>> + */
>> +struct video_device_state *
>> +__video_device_state_alloc(struct video_device *vdev);
>> +
>> +/** __video_device_state_free - free video device state structure
>> + *
>> + * @state: pointer to the state to be freed
>> + *
>> + * .. note::
>> + *
>> + *	This function is meant to be used only inside the V4L2 core.
>> + */
>> +void __video_device_state_free(struct video_device_state *state);
>> +
>>  /**
>>   * v4l2_debugfs_root - returns the dentry of the top-level "v4l2" debugfs dir
>>   *
>>
> 
>
Re: [PATCH v2 01/10] media: v4l2-core: Introduce state management for video devices
Posted by Jai Luthra 2 days, 16 hours ago
Hi Hans,

Thanks for the review.

Quoting Hans Verkuil (2025-09-22 13:30:05)
> On 22/09/2025 09:44, Hans Verkuil wrote:
> > Hi Jai,
> > 
> > Apologies that I had no time to review v1, but I'll review v2 today.
> > 
> > On 19/09/2025 11:55, Jai Luthra wrote:
> >> Similar to V4L2 subdev states, introduce state support for video devices
> >> to provide a centralized location for storing device state information.
> >> This includes the current (active) pixelformat used by the device and
> >> the temporary (try) pixelformat used during format negotiation. In the
> >> future, this may be extended or subclassed by device drivers to store
> >> their internal state variables.
> >>
> >> Also introduce a flag for drivers that wish to use this state
> >> management. When set, the framework automatically allocates the state
> >> during device registration and stores a pointer to it within the
> >> video_device structure.
> >>
> >> This change aligns video devices with V4L2 subdevices by storing
> >> hardware state in a common framework-allocated structure. This is the
> >> first step towards enabling the multiplexing of the underlying hardware
> >> by using different software "contexts", each represented by the combined
> >> state of all video devices and V4L2 subdevices in a complex media graph.
> >>
> >> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> >> --
> >> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> >> Cc: Hans Verkuil <hverkuil@kernel.org>
> >> Cc: Ricardo Ribalda <ribalda@chromium.org>
> >> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >> Cc: Al Viro <viro@zeniv.linux.org.uk>
> >> Cc: Ma Ke <make24@iscas.ac.cn>
> >> Cc: Jai Luthra <jai.luthra@ideasonboard.com>
> >> Cc: linux-media@vger.kernel.org
> >> Cc: linux-kernel@vger.kernel.org
> >> ---
> >>  drivers/media/v4l2-core/v4l2-dev.c | 27 +++++++++++++++++++++++++
> >>  include/media/v4l2-dev.h           | 40 ++++++++++++++++++++++++++++++++++++++
> >>  2 files changed, 67 insertions(+)
> >>
> >> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> >> index 10a126e50c1ca25b1bd0e9872571261acfc26b39..997255709448510fcd17b6de798a3df99cd7ea09 100644
> >> --- a/drivers/media/v4l2-core/v4l2-dev.c
> >> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> >> @@ -163,6 +163,27 @@ void video_device_release_empty(struct video_device *vdev)
> >>  }
> >>  EXPORT_SYMBOL(video_device_release_empty);
> >>  
> >> +struct video_device_state *
> >> +__video_device_state_alloc(struct video_device *vdev)
> >> +{
> >> +    struct video_device_state *state =
> >> +            kzalloc(sizeof(struct video_device_state), GFP_KERNEL);
> >> +
> >> +    if (!state)
> >> +            return ERR_PTR(-ENOMEM);
> >> +
> >> +    state->vdev = vdev;
> >> +
> >> +    return state;
> >> +}
> >> +EXPORT_SYMBOL_GPL(__video_device_state_alloc);
> >> +
> >> +void __video_device_state_free(struct video_device_state *state)
> >> +{
> >> +    kfree(state);
> >> +}
> >> +EXPORT_SYMBOL_GPL(__video_device_state_free);
> >> +
> >>  static inline void video_get(struct video_device *vdev)
> >>  {
> >>      get_device(&vdev->dev);
> >> @@ -939,6 +960,10 @@ int __video_register_device(struct video_device *vdev,
> >>      spin_lock_init(&vdev->fh_lock);
> >>      INIT_LIST_HEAD(&vdev->fh_list);
> >>  
> >> +    /* state support */
> >> +    if (test_bit(V4L2_FL_USES_STATE, &vdev->flags))
> >> +            vdev->state = __video_device_state_alloc(vdev);
> >> +
> >>      /* Part 1: check device type */
> >>      switch (type) {
> >>      case VFL_TYPE_VIDEO:
> >> @@ -1127,6 +1152,8 @@ void video_unregister_device(struct video_device *vdev)
> >>      clear_bit(V4L2_FL_REGISTERED, &vdev->flags);
> >>      mutex_unlock(&videodev_lock);
> >>      v4l2_event_wake_all(vdev);
> >> +    if (test_bit(V4L2_FL_USES_STATE, &vdev->flags))
> >> +            __video_device_state_free(vdev->state);
> >>      device_unregister(&vdev->dev);
> >>  }
> >>  EXPORT_SYMBOL(video_unregister_device);
> >> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> >> index a213c3398dcf60be8c531df87bf40c56b4ad772d..57e4691ef467aa2b0782dd4b8357bd0670643293 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,17 @@ struct v4l2_file_operations {
> >>      int (*release) (struct file *);
> >>  };
> >>  
> >> +/**
> >> + * struct video_device_state - Used for storing video device state information.
> >> + *
> >> + * @fmt: Format of the capture stream
> >> + * @vdev: Pointer to video device
> >> + */
> >> +struct video_device_state {
> >> +    struct v4l2_format fmt;
> > 
> > While typically a video_device supports only a single video format type, that is
> > not always the case. There are the following exceptions:
> > 
> > 1) M2M devices have both a capture and output video format. However, for M2M devices
> >    the state is per-filehandle, so it shouldn't be stored in a video_device_state
> >    struct anyway.

Ah I see, so for M2M devices the formats are stored per-context, where the
context is tied to the filehandle. In that case, I agree that storing the
format state inside struct video_device would not work.

> > 2) VBI devices can have both a raw and sliced VBI format (either capture or output)
> > 3) AFAIK non-M2M video devices can have both a video and meta format. That may have
> >    changed, I'm not 100% certain about this.

RPi CFE driver is one such case, where a single driver structure stores
both metadata and video format. But if I understand correctly, it creates
separate video device nodes for metadata and video capture, so it can be
managed through a single v4l2_format.fmt union for each video device.

Are there any non-M2M drivers which allow more than one type of formats to
be set on the same device node?

> > 4) video devices can also support an OVERLAY or OUTPUT_OVERLAY format (rare)
> > 
> > V4L2_CAP_VIDEO_OVERLAY is currently only used in
> > drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c, so once that driver
> > disappears we can drop video overlay support for capture devices.

Yes, bcm2835-camera should be dropped hopefully in a couple more revisions of
https://lore.kernel.org/all/20250907-vchiq-destage-v2-4-6884505dca78@ideasonboard.com/

> > 
> > 2-4 are all quite rare, but 1 is very common. But for such devices the state
> > wouldn't be in video_device anyway.
> > 
> > But it would be nice if the same struct can be used in both m2m devices and non-m2m
> > devices. It's just stored either in struct v4l2_fh or struct video_device. It would
> > give a lot of opportunities for creating helper functions to make the life for
> > driver developers easier.

Sure, I think we can modify the existing state struct to store both capture
and output formats, and keep it inside struct v4l2_fh for M2M devices.

This will definitely be confusing for driver developers, as currently the
two example patches in this series access the state directly. So I will add
framework helpers to access the correct state and format type, and document
properly that it should never be accessed manually by drivers.

> 
> Follow-up: assuming we want to support M2M devices as well (I think we should), then
> consider renaming video_device_state since it isn't video_device specific, i.e. it
> can either live in video_device or in v4l2_fh, and in the latter case you'd have
> two instances: capture and output state.

Argh, naming is the hardest problem :-)
Do you have any suggestions?

I personally don't think video_device_state is a bad name, even if it is
stored somewhere else for m2m devices, given it is still the "state" of the
video device, even if it is not persistent across multiple file opens.

I was trying to avoid names with "context" in then, so it does not clash
with Jacopo's work.

> 
> Regards,
> 
>         Hans
> 
> > 
> > Regards,
> > 
> >       Hans
> > 
> >> +    struct video_device *vdev;
> >> +};
> >> +
> >>  /*
> >>   * Newer version of video_device, handled by videodev2.c
> >>   *  This version moves redundant code from video device code to
> >> @@ -238,6 +255,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 +301,7 @@ struct video_device {
> >>      struct vb2_queue *queue;
> >>  
> >>      struct v4l2_prio_state *prio;
> >> +    struct video_device_state *state;
> >>  
> >>      /* device info */
> >>      char name[64];
> >> @@ -546,6 +565,27 @@ static inline int video_is_registered(struct video_device *vdev)
> >>      return test_bit(V4L2_FL_REGISTERED, &vdev->flags);
> >>  }
> >>  
> >> +/** __video_device_state_alloc - allocate video device state structure
> >> + *
> >> + * @vdev: pointer to struct video_device
> >> + *
> >> + * .. note::
> >> + *
> >> + *  This function is meant to be used only inside the V4L2 core.
> >> + */
> >> +struct video_device_state *
> >> +__video_device_state_alloc(struct video_device *vdev);
> >> +
> >> +/** __video_device_state_free - free video device state structure
> >> + *
> >> + * @state: pointer to the state to be freed
> >> + *
> >> + * .. note::
> >> + *
> >> + *  This function is meant to be used only inside the V4L2 core.
> >> + */
> >> +void __video_device_state_free(struct video_device_state *state);
> >> +
> >>  /**
> >>   * v4l2_debugfs_root - returns the dentry of the top-level "v4l2" debugfs dir
> >>   *
> >>
> > 
> > 
> 

Thanks,
Jai
Re: [PATCH v2 01/10] media: v4l2-core: Introduce state management for video devices
Posted by Hans Verkuil 2 days ago
On 29/09/2025 17:30, Jai Luthra wrote:
> Hi Hans,
> 
> Thanks for the review.
> 
> Quoting Hans Verkuil (2025-09-22 13:30:05)
>> On 22/09/2025 09:44, Hans Verkuil wrote:
>>> Hi Jai,
>>>
>>> Apologies that I had no time to review v1, but I'll review v2 today.
>>>
>>> On 19/09/2025 11:55, Jai Luthra wrote:
>>>> Similar to V4L2 subdev states, introduce state support for video devices
>>>> to provide a centralized location for storing device state information.
>>>> This includes the current (active) pixelformat used by the device and
>>>> the temporary (try) pixelformat used during format negotiation. In the
>>>> future, this may be extended or subclassed by device drivers to store
>>>> their internal state variables.
>>>>
>>>> Also introduce a flag for drivers that wish to use this state
>>>> management. When set, the framework automatically allocates the state
>>>> during device registration and stores a pointer to it within the
>>>> video_device structure.
>>>>
>>>> This change aligns video devices with V4L2 subdevices by storing
>>>> hardware state in a common framework-allocated structure. This is the
>>>> first step towards enabling the multiplexing of the underlying hardware
>>>> by using different software "contexts", each represented by the combined
>>>> state of all video devices and V4L2 subdevices in a complex media graph.
>>>>
>>>> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
>>>> --
>>>> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
>>>> Cc: Hans Verkuil <hverkuil@kernel.org>
>>>> Cc: Ricardo Ribalda <ribalda@chromium.org>
>>>> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>>>> Cc: Al Viro <viro@zeniv.linux.org.uk>
>>>> Cc: Ma Ke <make24@iscas.ac.cn>
>>>> Cc: Jai Luthra <jai.luthra@ideasonboard.com>
>>>> Cc: linux-media@vger.kernel.org
>>>> Cc: linux-kernel@vger.kernel.org
>>>> ---
>>>>  drivers/media/v4l2-core/v4l2-dev.c | 27 +++++++++++++++++++++++++
>>>>  include/media/v4l2-dev.h           | 40 ++++++++++++++++++++++++++++++++++++++
>>>>  2 files changed, 67 insertions(+)
>>>>
>>>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>>>> index 10a126e50c1ca25b1bd0e9872571261acfc26b39..997255709448510fcd17b6de798a3df99cd7ea09 100644
>>>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>>>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>>>> @@ -163,6 +163,27 @@ void video_device_release_empty(struct video_device *vdev)
>>>>  }
>>>>  EXPORT_SYMBOL(video_device_release_empty);
>>>>  
>>>> +struct video_device_state *
>>>> +__video_device_state_alloc(struct video_device *vdev)
>>>> +{
>>>> +    struct video_device_state *state =
>>>> +            kzalloc(sizeof(struct video_device_state), GFP_KERNEL);
>>>> +
>>>> +    if (!state)
>>>> +            return ERR_PTR(-ENOMEM);
>>>> +
>>>> +    state->vdev = vdev;
>>>> +
>>>> +    return state;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(__video_device_state_alloc);
>>>> +
>>>> +void __video_device_state_free(struct video_device_state *state)
>>>> +{
>>>> +    kfree(state);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(__video_device_state_free);
>>>> +
>>>>  static inline void video_get(struct video_device *vdev)
>>>>  {
>>>>      get_device(&vdev->dev);
>>>> @@ -939,6 +960,10 @@ int __video_register_device(struct video_device *vdev,
>>>>      spin_lock_init(&vdev->fh_lock);
>>>>      INIT_LIST_HEAD(&vdev->fh_list);
>>>>  
>>>> +    /* state support */
>>>> +    if (test_bit(V4L2_FL_USES_STATE, &vdev->flags))
>>>> +            vdev->state = __video_device_state_alloc(vdev);
>>>> +
>>>>      /* Part 1: check device type */
>>>>      switch (type) {
>>>>      case VFL_TYPE_VIDEO:
>>>> @@ -1127,6 +1152,8 @@ void video_unregister_device(struct video_device *vdev)
>>>>      clear_bit(V4L2_FL_REGISTERED, &vdev->flags);
>>>>      mutex_unlock(&videodev_lock);
>>>>      v4l2_event_wake_all(vdev);
>>>> +    if (test_bit(V4L2_FL_USES_STATE, &vdev->flags))
>>>> +            __video_device_state_free(vdev->state);
>>>>      device_unregister(&vdev->dev);
>>>>  }
>>>>  EXPORT_SYMBOL(video_unregister_device);
>>>> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
>>>> index a213c3398dcf60be8c531df87bf40c56b4ad772d..57e4691ef467aa2b0782dd4b8357bd0670643293 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,17 @@ struct v4l2_file_operations {
>>>>      int (*release) (struct file *);
>>>>  };
>>>>  
>>>> +/**
>>>> + * struct video_device_state - Used for storing video device state information.
>>>> + *
>>>> + * @fmt: Format of the capture stream
>>>> + * @vdev: Pointer to video device
>>>> + */
>>>> +struct video_device_state {
>>>> +    struct v4l2_format fmt;
>>>
>>> While typically a video_device supports only a single video format type, that is
>>> not always the case. There are the following exceptions:
>>>
>>> 1) M2M devices have both a capture and output video format. However, for M2M devices
>>>    the state is per-filehandle, so it shouldn't be stored in a video_device_state
>>>    struct anyway.
> 
> Ah I see, so for M2M devices the formats are stored per-context, where the
> context is tied to the filehandle. In that case, I agree that storing the
> format state inside struct video_device would not work.
> 
>>> 2) VBI devices can have both a raw and sliced VBI format (either capture or output)
>>> 3) AFAIK non-M2M video devices can have both a video and meta format. That may have
>>>    changed, I'm not 100% certain about this.
> 
> RPi CFE driver is one such case, where a single driver structure stores
> both metadata and video format. But if I understand correctly, it creates
> separate video device nodes for metadata and video capture, so it can be
> managed through a single v4l2_format.fmt union for each video device.
> 
> Are there any non-M2M drivers which allow more than one type of formats to
> be set on the same device node?

I think this one does that:

drivers/media/pci/intel/ipu6/ipu6-isys-video.c

I think it can set both video and metadata formats, but it can only stream one
type at a time. I'm not 100% certain of that, but that's what it looks like
reading the code.

> 
>>> 4) video devices can also support an OVERLAY or OUTPUT_OVERLAY format (rare)
>>>
>>> V4L2_CAP_VIDEO_OVERLAY is currently only used in
>>> drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c, so once that driver
>>> disappears we can drop video overlay support for capture devices.
> 
> Yes, bcm2835-camera should be dropped hopefully in a couple more revisions of
> https://lore.kernel.org/all/20250907-vchiq-destage-v2-4-6884505dca78@ideasonboard.com/
> 
>>>
>>> 2-4 are all quite rare, but 1 is very common. But for such devices the state
>>> wouldn't be in video_device anyway.
>>>
>>> But it would be nice if the same struct can be used in both m2m devices and non-m2m
>>> devices. It's just stored either in struct v4l2_fh or struct video_device. It would
>>> give a lot of opportunities for creating helper functions to make the life for
>>> driver developers easier.
> 
> Sure, I think we can modify the existing state struct to store both capture
> and output formats, and keep it inside struct v4l2_fh for M2M devices.
> 
> This will definitely be confusing for driver developers, as currently the
> two example patches in this series access the state directly. So I will add
> framework helpers to access the correct state and format type, and document
> properly that it should never be accessed manually by drivers.
> 
>>
>> Follow-up: assuming we want to support M2M devices as well (I think we should), then
>> consider renaming video_device_state since it isn't video_device specific, i.e. it
>> can either live in video_device or in v4l2_fh, and in the latter case you'd have
>> two instances: capture and output state.
> 
> Argh, naming is the hardest problem :-)
> Do you have any suggestions?
> 
> I personally don't think video_device_state is a bad name, even if it is
> stored somewhere else for m2m devices, given it is still the "state" of the
> video device, even if it is not persistent across multiple file opens.

Perhaps just v4l2_state?

video_device_state refers to struct video_device, so that's not a good name.

Regards,

	Hans

> 
> I was trying to avoid names with "context" in then, so it does not clash
> with Jacopo's work.
> 
>>
>> Regards,
>>
>>         Hans
>>
>>>
>>> Regards,
>>>
>>>       Hans
>>>
>>>> +    struct video_device *vdev;
>>>> +};
>>>> +
>>>>  /*
>>>>   * Newer version of video_device, handled by videodev2.c
>>>>   *  This version moves redundant code from video device code to
>>>> @@ -238,6 +255,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 +301,7 @@ struct video_device {
>>>>      struct vb2_queue *queue;
>>>>  
>>>>      struct v4l2_prio_state *prio;
>>>> +    struct video_device_state *state;
>>>>  
>>>>      /* device info */
>>>>      char name[64];
>>>> @@ -546,6 +565,27 @@ static inline int video_is_registered(struct video_device *vdev)
>>>>      return test_bit(V4L2_FL_REGISTERED, &vdev->flags);
>>>>  }
>>>>  
>>>> +/** __video_device_state_alloc - allocate video device state structure
>>>> + *
>>>> + * @vdev: pointer to struct video_device
>>>> + *
>>>> + * .. note::
>>>> + *
>>>> + *  This function is meant to be used only inside the V4L2 core.
>>>> + */
>>>> +struct video_device_state *
>>>> +__video_device_state_alloc(struct video_device *vdev);
>>>> +
>>>> +/** __video_device_state_free - free video device state structure
>>>> + *
>>>> + * @state: pointer to the state to be freed
>>>> + *
>>>> + * .. note::
>>>> + *
>>>> + *  This function is meant to be used only inside the V4L2 core.
>>>> + */
>>>> +void __video_device_state_free(struct video_device_state *state);
>>>> +
>>>>  /**
>>>>   * v4l2_debugfs_root - returns the dentry of the top-level "v4l2" debugfs dir
>>>>   *
>>>>
>>>
>>>
>>
> 
> Thanks,
> Jai
Re: [PATCH v2 01/10] media: v4l2-core: Introduce state management for video devices
Posted by Jacopo Mondi 2 days ago
Hi Jai, Hans

On Mon, Sep 29, 2025 at 09:00:51PM +0530, Jai Luthra wrote:
> Hi Hans,
>
> Thanks for the review.
>
> Quoting Hans Verkuil (2025-09-22 13:30:05)
> > On 22/09/2025 09:44, Hans Verkuil wrote:
> > > Hi Jai,
> > >
> > > Apologies that I had no time to review v1, but I'll review v2 today.
> > >
> > > On 19/09/2025 11:55, Jai Luthra wrote:
> > >> Similar to V4L2 subdev states, introduce state support for video devices
> > >> to provide a centralized location for storing device state information.
> > >> This includes the current (active) pixelformat used by the device and
> > >> the temporary (try) pixelformat used during format negotiation. In the
> > >> future, this may be extended or subclassed by device drivers to store
> > >> their internal state variables.
> > >>
> > >> Also introduce a flag for drivers that wish to use this state
> > >> management. When set, the framework automatically allocates the state
> > >> during device registration and stores a pointer to it within the
> > >> video_device structure.
> > >>
> > >> This change aligns video devices with V4L2 subdevices by storing
> > >> hardware state in a common framework-allocated structure. This is the
> > >> first step towards enabling the multiplexing of the underlying hardware
> > >> by using different software "contexts", each represented by the combined
> > >> state of all video devices and V4L2 subdevices in a complex media graph.
> > >>
> > >> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > >> --
> > >> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> > >> Cc: Hans Verkuil <hverkuil@kernel.org>
> > >> Cc: Ricardo Ribalda <ribalda@chromium.org>
> > >> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > >> Cc: Al Viro <viro@zeniv.linux.org.uk>
> > >> Cc: Ma Ke <make24@iscas.ac.cn>
> > >> Cc: Jai Luthra <jai.luthra@ideasonboard.com>
> > >> Cc: linux-media@vger.kernel.org
> > >> Cc: linux-kernel@vger.kernel.org
> > >> ---
> > >>  drivers/media/v4l2-core/v4l2-dev.c | 27 +++++++++++++++++++++++++
> > >>  include/media/v4l2-dev.h           | 40 ++++++++++++++++++++++++++++++++++++++
> > >>  2 files changed, 67 insertions(+)
> > >>
> > >> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> > >> index 10a126e50c1ca25b1bd0e9872571261acfc26b39..997255709448510fcd17b6de798a3df99cd7ea09 100644
> > >> --- a/drivers/media/v4l2-core/v4l2-dev.c
> > >> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > >> @@ -163,6 +163,27 @@ void video_device_release_empty(struct video_device *vdev)
> > >>  }
> > >>  EXPORT_SYMBOL(video_device_release_empty);
> > >>
> > >> +struct video_device_state *
> > >> +__video_device_state_alloc(struct video_device *vdev)
> > >> +{
> > >> +    struct video_device_state *state =
> > >> +            kzalloc(sizeof(struct video_device_state), GFP_KERNEL);
> > >> +
> > >> +    if (!state)
> > >> +            return ERR_PTR(-ENOMEM);
> > >> +
> > >> +    state->vdev = vdev;
> > >> +
> > >> +    return state;
> > >> +}
> > >> +EXPORT_SYMBOL_GPL(__video_device_state_alloc);
> > >> +
> > >> +void __video_device_state_free(struct video_device_state *state)
> > >> +{
> > >> +    kfree(state);
> > >> +}
> > >> +EXPORT_SYMBOL_GPL(__video_device_state_free);
> > >> +
> > >>  static inline void video_get(struct video_device *vdev)
> > >>  {
> > >>      get_device(&vdev->dev);
> > >> @@ -939,6 +960,10 @@ int __video_register_device(struct video_device *vdev,
> > >>      spin_lock_init(&vdev->fh_lock);
> > >>      INIT_LIST_HEAD(&vdev->fh_list);
> > >>
> > >> +    /* state support */
> > >> +    if (test_bit(V4L2_FL_USES_STATE, &vdev->flags))
> > >> +            vdev->state = __video_device_state_alloc(vdev);
> > >> +
> > >>      /* Part 1: check device type */
> > >>      switch (type) {
> > >>      case VFL_TYPE_VIDEO:
> > >> @@ -1127,6 +1152,8 @@ void video_unregister_device(struct video_device *vdev)
> > >>      clear_bit(V4L2_FL_REGISTERED, &vdev->flags);
> > >>      mutex_unlock(&videodev_lock);
> > >>      v4l2_event_wake_all(vdev);
> > >> +    if (test_bit(V4L2_FL_USES_STATE, &vdev->flags))
> > >> +            __video_device_state_free(vdev->state);
> > >>      device_unregister(&vdev->dev);
> > >>  }
> > >>  EXPORT_SYMBOL(video_unregister_device);
> > >> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> > >> index a213c3398dcf60be8c531df87bf40c56b4ad772d..57e4691ef467aa2b0782dd4b8357bd0670643293 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,17 @@ struct v4l2_file_operations {
> > >>      int (*release) (struct file *);
> > >>  };
> > >>
> > >> +/**
> > >> + * struct video_device_state - Used for storing video device state information.
> > >> + *
> > >> + * @fmt: Format of the capture stream
> > >> + * @vdev: Pointer to video device
> > >> + */
> > >> +struct video_device_state {
> > >> +    struct v4l2_format fmt;
> > >
> > > While typically a video_device supports only a single video format type, that is
> > > not always the case. There are the following exceptions:
> > >
> > > 1) M2M devices have both a capture and output video format. However, for M2M devices
> > >    the state is per-filehandle, so it shouldn't be stored in a video_device_state
> > >    struct anyway.
>
> Ah I see, so for M2M devices the formats are stored per-context, where the
> context is tied to the filehandle. In that case, I agree that storing the
> format state inside struct video_device would not work.
>

The m2m frameworks stores in 'struct v4l2_m2m_ctx' two queue contexts,
one for the output queue and one for capture queue. A v4l2_m2m_ctx is
created every time the video device is open and the two queues
initialized. Userspace opens the video device two (or more) times and
one of the two contexts is used at a time depending on the buffer type
userspace uses to initialize the device.

The right place where to store the 'state' seems to me to be
'struct v4l2_m2m_queue_ctx' and a set of helpers for m2m should be
provided for drivers to be able to get the 'capture' or 'output'
format, provided a v4l2_m2m_ctx to operate with and the buffer type in
use.

I'm looking at the m2m driver I know the better (imx8-isi-m2m.c) and
the conversion seems trivial there. The format information stored as
ctx->queues.out.format/ctx->queues.cap.format could be moved to the
framework and accessed with helpers by the driver. As soon as we allow
drivers to sub-class the video device state (something I think we
should do from the very beginning in this series) the whole 'struct
mxc_isi_m2m_ctx_queue_data' type can be replaced and become 'struct
mxc_isi_m2m_ctx_state'.

M2M drivers will not set V4L2_FL_USES_STATE so there won't be a state
stored in the file handle or the video device for them, but we can
indeed require m2m drivers to provide an 'init_state' operation like
we do for regular video devices, where the state will be initialised.
Only new drivers that provide such operation will have a centralized
state.

Now, what do we gain here ? The current video_device_state stores a
v4l2_format and a video_device *, and only the v4l2_format is
relevant for the m2m framework [*]. However centralizing storage of the
format in the framwork and allowing drivers to sub-class it would save
some boilerplate indeed. I would be happy to try do this on
imx8-isi-m2m.c to see how it will look like.

[*] I would be tempted to introduce a 'class' hierarchy such as:

        struct v4l2_device_state {
                struct v4l2_format fmt;
        }

        struct video_device_state {
                struct v4l2_device_state state;
                struct video_device *vdev;
        }

        struct m2m_context_state {
                struct v4l2_device_state state;
                struct v4l2_m2m_ctx *ctx;
        };

Helpers could shorten access to the state, and drivers can
sub-class struct v4l2_device_state by making it a pointer here and
delegating allocation and initialization of the state to drivers.


> > > 2) VBI devices can have both a raw and sliced VBI format (either capture or output)
> > > 3) AFAIK non-M2M video devices can have both a video and meta format. That may have
> > >    changed, I'm not 100% certain about this.
>
> RPi CFE driver is one such case, where a single driver structure stores
> both metadata and video format. But if I understand correctly, it creates
> separate video device nodes for metadata and video capture, so it can be
> managed through a single v4l2_format.fmt union for each video device.
>
> Are there any non-M2M drivers which allow more than one type of formats to
> be set on the same device node?
>

For non-m2m device I would consider the above a non-issue. As Jai
said the rp1-cfe driver uses two instances of 'struct v4l2_format' to
store the meta and capture formats, but they're not used at the same
time. IOW the format information can be easily move to be
per-video-device state.

> > > 4) video devices can also support an OVERLAY or OUTPUT_OVERLAY format (rare)
> > >
> > > V4L2_CAP_VIDEO_OVERLAY is currently only used in
> > > drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c, so once that driver
> > > disappears we can drop video overlay support for capture devices.
>
> Yes, bcm2835-camera should be dropped hopefully in a couple more revisions of
> https://lore.kernel.org/all/20250907-vchiq-destage-v2-4-6884505dca78@ideasonboard.com/
>
> > >
> > > 2-4 are all quite rare, but 1 is very common. But for such devices the state
> > > wouldn't be in video_device anyway.
> > >
> > > But it would be nice if the same struct can be used in both m2m devices and non-m2m
> > > devices. It's just stored either in struct v4l2_fh or struct video_device. It would
> > > give a lot of opportunities for creating helper functions to make the life for
> > > driver developers easier.
>
> Sure, I think we can modify the existing state struct to store both capture
> and output formats, and keep it inside struct v4l2_fh for M2M devices.

I don't think we want to store multiple v4l2_formats (or members of
the fmt union) in the state. You would need one for each member of the
fmt union as you don't know which one the driver will use. I would
keep trying to have a single v4l2_format per state and place the state
in the right place so that only a single format is needed (see above
for m2m).

>
> This will definitely be confusing for driver developers, as currently the
> two example patches in this series access the state directly. So I will add

Argh, they shouldn't :)

> framework helpers to access the correct state and format type, and document
> properly that it should never be accessed manually by drivers.

Thanks!

>
> >
> > Follow-up: assuming we want to support M2M devices as well (I think we should), then
> > consider renaming video_device_state since it isn't video_device specific, i.e. it
> > can either live in video_device or in v4l2_fh, and in the latter case you'd have
> > two instances: capture and output state.
>
> Argh, naming is the hardest problem :-)
> Do you have any suggestions?

What do you think about the above idea of subclassing a generic
v4l2_device_state with video_device_state and m2m_context_state ?

>
> I personally don't think video_device_state is a bad name, even if it is
> stored somewhere else for m2m devices, given it is still the "state" of the
> video device, even if it is not persistent across multiple file opens.

I think the problem is mostly due to the fact a video_device_state
stores a video_device * which is not relevant for m2m..

>
> I was trying to avoid names with "context" in then, so it does not clash
> with Jacopo's work.
>

Thanks ;)

> >
> > Regards,
> >
> >         Hans
> >
> > >
> > > Regards,
> > >
> > >       Hans
> > >
> > >> +    struct video_device *vdev;
> > >> +};
> > >> +
> > >>  /*
> > >>   * Newer version of video_device, handled by videodev2.c
> > >>   *  This version moves redundant code from video device code to
> > >> @@ -238,6 +255,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 +301,7 @@ struct video_device {
> > >>      struct vb2_queue *queue;
> > >>
> > >>      struct v4l2_prio_state *prio;
> > >> +    struct video_device_state *state;
> > >>
> > >>      /* device info */
> > >>      char name[64];
> > >> @@ -546,6 +565,27 @@ static inline int video_is_registered(struct video_device *vdev)
> > >>      return test_bit(V4L2_FL_REGISTERED, &vdev->flags);
> > >>  }
> > >>
> > >> +/** __video_device_state_alloc - allocate video device state structure
> > >> + *
> > >> + * @vdev: pointer to struct video_device
> > >> + *
> > >> + * .. note::
> > >> + *
> > >> + *  This function is meant to be used only inside the V4L2 core.
> > >> + */
> > >> +struct video_device_state *
> > >> +__video_device_state_alloc(struct video_device *vdev);
> > >> +
> > >> +/** __video_device_state_free - free video device state structure
> > >> + *
> > >> + * @state: pointer to the state to be freed
> > >> + *
> > >> + * .. note::
> > >> + *
> > >> + *  This function is meant to be used only inside the V4L2 core.
> > >> + */
> > >> +void __video_device_state_free(struct video_device_state *state);
> > >> +
> > >>  /**
> > >>   * v4l2_debugfs_root - returns the dentry of the top-level "v4l2" debugfs dir
> > >>   *
> > >>
> > >
> > >
> >
>
> Thanks,
> Jai
Re: [PATCH v2 01/10] media: v4l2-core: Introduce state management for video devices
Posted by Michael Riesch 2 days, 11 hours ago
Hi Jai,

Thanks for your efforts! Could you include me in Cc: in the next
iteration, please?

On 9/29/25 17:30, Jai Luthra wrote:
> Hi Hans,
> 
> Thanks for the review.
> 
> Quoting Hans Verkuil (2025-09-22 13:30:05)
>> On 22/09/2025 09:44, Hans Verkuil wrote:
>>> Hi Jai,
>>>
>>> Apologies that I had no time to review v1, but I'll review v2 today.
>>>
>>> On 19/09/2025 11:55, Jai Luthra wrote:
>>>> Similar to V4L2 subdev states, introduce state support for video devices
>>>> to provide a centralized location for storing device state information.
>>>> This includes the current (active) pixelformat used by the device and
>>>> the temporary (try) pixelformat used during format negotiation. In the
>>>> future, this may be extended or subclassed by device drivers to store
>>>> their internal state variables.
>>>>
>>>> Also introduce a flag for drivers that wish to use this state
>>>> management. When set, the framework automatically allocates the state
>>>> during device registration and stores a pointer to it within the
>>>> video_device structure.
>>>>
>>>> This change aligns video devices with V4L2 subdevices by storing
>>>> hardware state in a common framework-allocated structure. This is the
>>>> first step towards enabling the multiplexing of the underlying hardware
>>>> by using different software "contexts", each represented by the combined
>>>> state of all video devices and V4L2 subdevices in a complex media graph.

Could you elaborate a bit on how (sub)device states and the future
contexts will be related? Based on the description above, I imagine that
at some point there will be a context data structure that contains all
(sub) device states, and when a certain context becomes active, the
states are passed and applied to the (sub)devices. But this is only my
imagination here. It would be great to know the overall concept here.

>>>>
>>>> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
>>>> --
>>>> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
>>>> Cc: Hans Verkuil <hverkuil@kernel.org>
>>>> Cc: Ricardo Ribalda <ribalda@chromium.org>
>>>> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>>>> Cc: Al Viro <viro@zeniv.linux.org.uk>
>>>> Cc: Ma Ke <make24@iscas.ac.cn>
>>>> Cc: Jai Luthra <jai.luthra@ideasonboard.com>
>>>> Cc: linux-media@vger.kernel.org
>>>> Cc: linux-kernel@vger.kernel.org
>>>> ---
>>>>  drivers/media/v4l2-core/v4l2-dev.c | 27 +++++++++++++++++++++++++
>>>>  include/media/v4l2-dev.h           | 40 ++++++++++++++++++++++++++++++++++++++
>>>>  2 files changed, 67 insertions(+)
>>>>
>>>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>>>> index 10a126e50c1ca25b1bd0e9872571261acfc26b39..997255709448510fcd17b6de798a3df99cd7ea09 100644
>>>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>>>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>>>> @@ -163,6 +163,27 @@ void video_device_release_empty(struct video_device *vdev)
>>>>  }
>>>>  EXPORT_SYMBOL(video_device_release_empty);
>>>>  
>>>> +struct video_device_state *
>>>> +__video_device_state_alloc(struct video_device *vdev)
>>>> +{
>>>> +    struct video_device_state *state =
>>>> +            kzalloc(sizeof(struct video_device_state), GFP_KERNEL);
>>>> +
>>>> +    if (!state)
>>>> +            return ERR_PTR(-ENOMEM);
>>>> +
>>>> +    state->vdev = vdev;
>>>> +
>>>> +    return state;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(__video_device_state_alloc);
>>>> +
>>>> +void __video_device_state_free(struct video_device_state *state)
>>>> +{
>>>> +    kfree(state);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(__video_device_state_free);
>>>> +
>>>>  static inline void video_get(struct video_device *vdev)
>>>>  {
>>>>      get_device(&vdev->dev);
>>>> @@ -939,6 +960,10 @@ int __video_register_device(struct video_device *vdev,
>>>>      spin_lock_init(&vdev->fh_lock);
>>>>      INIT_LIST_HEAD(&vdev->fh_list);
>>>>  
>>>> +    /* state support */
>>>> +    if (test_bit(V4L2_FL_USES_STATE, &vdev->flags))
>>>> +            vdev->state = __video_device_state_alloc(vdev);
>>>> +
>>>>      /* Part 1: check device type */
>>>>      switch (type) {
>>>>      case VFL_TYPE_VIDEO:
>>>> @@ -1127,6 +1152,8 @@ void video_unregister_device(struct video_device *vdev)
>>>>      clear_bit(V4L2_FL_REGISTERED, &vdev->flags);
>>>>      mutex_unlock(&videodev_lock);
>>>>      v4l2_event_wake_all(vdev);
>>>> +    if (test_bit(V4L2_FL_USES_STATE, &vdev->flags))
>>>> +            __video_device_state_free(vdev->state);
>>>>      device_unregister(&vdev->dev);
>>>>  }
>>>>  EXPORT_SYMBOL(video_unregister_device);
>>>> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
>>>> index a213c3398dcf60be8c531df87bf40c56b4ad772d..57e4691ef467aa2b0782dd4b8357bd0670643293 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,17 @@ struct v4l2_file_operations {
>>>>      int (*release) (struct file *);
>>>>  };
>>>>  
>>>> +/**
>>>> + * struct video_device_state - Used for storing video device state information.
>>>> + *
>>>> + * @fmt: Format of the capture stream
>>>> + * @vdev: Pointer to video device

What else do you envisage to be included in the state?

>>>> + */
>>>> +struct video_device_state {
>>>> +    struct v4l2_format fmt;
>>>
>>> While typically a video_device supports only a single video format type, that is
>>> not always the case. There are the following exceptions:
>>>
>>> 1) M2M devices have both a capture and output video format. However, for M2M devices
>>>    the state is per-filehandle, so it shouldn't be stored in a video_device_state
>>>    struct anyway.
> 
> Ah I see, so for M2M devices the formats are stored per-context, where the
> context is tied to the filehandle. In that case, I agree that storing the
> format state inside struct video_device would not work.
> 
>>> 2) VBI devices can have both a raw and sliced VBI format (either capture or output)
>>> 3) AFAIK non-M2M video devices can have both a video and meta format. That may have
>>>    changed, I'm not 100% certain about this.
> 
> RPi CFE driver is one such case, where a single driver structure stores
> both metadata and video format. But if I understand correctly, it creates
> separate video device nodes for metadata and video capture, so it can be
> managed through a single v4l2_format.fmt union for each video device.
> 
> Are there any non-M2M drivers which allow more than one type of formats to
> be set on the same device node?
> 
>>> 4) video devices can also support an OVERLAY or OUTPUT_OVERLAY format (rare)
>>>
>>> V4L2_CAP_VIDEO_OVERLAY is currently only used in
>>> drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c, so once that driver
>>> disappears we can drop video overlay support for capture devices.
> 
> Yes, bcm2835-camera should be dropped hopefully in a couple more revisions of
> https://lore.kernel.org/all/20250907-vchiq-destage-v2-4-6884505dca78@ideasonboard.com/
> 
>>>
>>> 2-4 are all quite rare, but 1 is very common. But for such devices the state
>>> wouldn't be in video_device anyway.
>>>
>>> But it would be nice if the same struct can be used in both m2m devices and non-m2m
>>> devices. It's just stored either in struct v4l2_fh or struct video_device. It would
>>> give a lot of opportunities for creating helper functions to make the life for
>>> driver developers easier.
> 
> Sure, I think we can modify the existing state struct to store both capture
> and output formats, and keep it inside struct v4l2_fh for M2M devices.
> 
> This will definitely be confusing for driver developers, as currently the
> two example patches in this series access the state directly. So I will add
> framework helpers to access the correct state and format type, and document
> properly that it should never be accessed manually by drivers.

Would that be a similar approach as in v4l2_subdev_state?

>> Follow-up: assuming we want to support M2M devices as well (I think we should), then
>> consider renaming video_device_state since it isn't video_device specific, i.e. it
>> can either live in video_device or in v4l2_fh, and in the latter case you'd have
>> two instances: capture and output state.
> 
> Argh, naming is the hardest problem :-)
> Do you have any suggestions?
> 
> I personally don't think video_device_state is a bad name, even if it is
> stored somewhere else for m2m devices, given it is still the "state" of the
> video device, even if it is not persistent across multiple file opens.
> 
> I was trying to avoid names with "context" in then, so it does not clash
> with Jacopo's work.

It would be interesting how contexts (the new ones from Jacopo's series)
are applied to mem2mem devices. And again, how the state introduced here
is related to the (new) context.

Best regards,
Michael

> 
>>
>> Regards,
>>
>>         Hans
>>
>>>
>>> Regards,
>>>
>>>       Hans
>>>
>>>> +    struct video_device *vdev;
>>>> +};
>>>> +
>>>>  /*
>>>>   * Newer version of video_device, handled by videodev2.c
>>>>   *  This version moves redundant code from video device code to
>>>> @@ -238,6 +255,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 +301,7 @@ struct video_device {
>>>>      struct vb2_queue *queue;
>>>>  
>>>>      struct v4l2_prio_state *prio;
>>>> +    struct video_device_state *state;
>>>>  
>>>>      /* device info */
>>>>      char name[64];
>>>> @@ -546,6 +565,27 @@ static inline int video_is_registered(struct video_device *vdev)
>>>>      return test_bit(V4L2_FL_REGISTERED, &vdev->flags);
>>>>  }
>>>>  
>>>> +/** __video_device_state_alloc - allocate video device state structure
>>>> + *
>>>> + * @vdev: pointer to struct video_device
>>>> + *
>>>> + * .. note::
>>>> + *
>>>> + *  This function is meant to be used only inside the V4L2 core.
>>>> + */
>>>> +struct video_device_state *
>>>> +__video_device_state_alloc(struct video_device *vdev);
>>>> +
>>>> +/** __video_device_state_free - free video device state structure
>>>> + *
>>>> + * @state: pointer to the state to be freed
>>>> + *
>>>> + * .. note::
>>>> + *
>>>> + *  This function is meant to be used only inside the V4L2 core.
>>>> + */
>>>> +void __video_device_state_free(struct video_device_state *state);
>>>> +
>>>>  /**
>>>>   * v4l2_debugfs_root - returns the dentry of the top-level "v4l2" debugfs dir
>>>>   *
>>>>
>>>
>>>
>>
> 
> Thanks,
> Jai
>
Re: [PATCH v2 01/10] media: v4l2-core: Introduce state management for video devices
Posted by Jacopo Mondi 2 days ago
Hi Michael

On Mon, Sep 29, 2025 at 10:02:41PM +0200, Michael Riesch wrote:
> Hi Jai,
>
> Thanks for your efforts! Could you include me in Cc: in the next
> iteration, please?
>
> On 9/29/25 17:30, Jai Luthra wrote:
> > Hi Hans,
> >
> > Thanks for the review.
> >
> > Quoting Hans Verkuil (2025-09-22 13:30:05)
> >> On 22/09/2025 09:44, Hans Verkuil wrote:
> >>> Hi Jai,
> >>>
> >>> Apologies that I had no time to review v1, but I'll review v2 today.
> >>>
> >>> On 19/09/2025 11:55, Jai Luthra wrote:
> >>>> Similar to V4L2 subdev states, introduce state support for video devices
> >>>> to provide a centralized location for storing device state information.
> >>>> This includes the current (active) pixelformat used by the device and
> >>>> the temporary (try) pixelformat used during format negotiation. In the
> >>>> future, this may be extended or subclassed by device drivers to store
> >>>> their internal state variables.
> >>>>
> >>>> Also introduce a flag for drivers that wish to use this state
> >>>> management. When set, the framework automatically allocates the state
> >>>> during device registration and stores a pointer to it within the
> >>>> video_device structure.
> >>>>
> >>>> This change aligns video devices with V4L2 subdevices by storing
> >>>> hardware state in a common framework-allocated structure. This is the
> >>>> first step towards enabling the multiplexing of the underlying hardware
> >>>> by using different software "contexts", each represented by the combined
> >>>> state of all video devices and V4L2 subdevices in a complex media graph.
>
> Could you elaborate a bit on how (sub)device states and the future

Sure
https://patchwork.linuxtv.org/project/linux-media/patch/20250724-multicontext-mainline-2025-v2-12-c9b316773486@ideasonboard.com/

> contexts will be related? Based on the description above, I imagine that
> at some point there will be a context data structure that contains all
> (sub) device states, and when a certain context becomes active, the

Not all states, a single state.

There will be
1) One state per context
2) One state per open-file handle
3) One state per subdevice

If the driver supports contexts 1) will be used for active and 2) for
try.

If the driver doesn't support contexts 3) will be used for activer and
2) for try.

It's a bit more complex than this as we also have a default context in
the subdev to support drivers that are context-aware but are operated
by context-unaware userspace.

Happy to discuss this more on the actual context series.

> states are passed and applied to the (sub)devices. But this is only my
> imagination here. It would be great to know the overall concept here.
>
> >>>>
> >>>> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> >>>> --
> >>>> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> >>>> Cc: Hans Verkuil <hverkuil@kernel.org>
> >>>> Cc: Ricardo Ribalda <ribalda@chromium.org>
> >>>> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >>>> Cc: Al Viro <viro@zeniv.linux.org.uk>
> >>>> Cc: Ma Ke <make24@iscas.ac.cn>
> >>>> Cc: Jai Luthra <jai.luthra@ideasonboard.com>
> >>>> Cc: linux-media@vger.kernel.org
> >>>> Cc: linux-kernel@vger.kernel.org
> >>>> ---
> >>>>  drivers/media/v4l2-core/v4l2-dev.c | 27 +++++++++++++++++++++++++
> >>>>  include/media/v4l2-dev.h           | 40 ++++++++++++++++++++++++++++++++++++++
> >>>>  2 files changed, 67 insertions(+)
> >>>>
> >>>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> >>>> index 10a126e50c1ca25b1bd0e9872571261acfc26b39..997255709448510fcd17b6de798a3df99cd7ea09 100644
> >>>> --- a/drivers/media/v4l2-core/v4l2-dev.c
> >>>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> >>>> @@ -163,6 +163,27 @@ void video_device_release_empty(struct video_device *vdev)
> >>>>  }
> >>>>  EXPORT_SYMBOL(video_device_release_empty);
> >>>>
> >>>> +struct video_device_state *
> >>>> +__video_device_state_alloc(struct video_device *vdev)
> >>>> +{
> >>>> +    struct video_device_state *state =
> >>>> +            kzalloc(sizeof(struct video_device_state), GFP_KERNEL);
> >>>> +
> >>>> +    if (!state)
> >>>> +            return ERR_PTR(-ENOMEM);
> >>>> +
> >>>> +    state->vdev = vdev;
> >>>> +
> >>>> +    return state;
> >>>> +}
> >>>> +EXPORT_SYMBOL_GPL(__video_device_state_alloc);
> >>>> +
> >>>> +void __video_device_state_free(struct video_device_state *state)
> >>>> +{
> >>>> +    kfree(state);
> >>>> +}
> >>>> +EXPORT_SYMBOL_GPL(__video_device_state_free);
> >>>> +
> >>>>  static inline void video_get(struct video_device *vdev)
> >>>>  {
> >>>>      get_device(&vdev->dev);
> >>>> @@ -939,6 +960,10 @@ int __video_register_device(struct video_device *vdev,
> >>>>      spin_lock_init(&vdev->fh_lock);
> >>>>      INIT_LIST_HEAD(&vdev->fh_list);
> >>>>
> >>>> +    /* state support */
> >>>> +    if (test_bit(V4L2_FL_USES_STATE, &vdev->flags))
> >>>> +            vdev->state = __video_device_state_alloc(vdev);
> >>>> +
> >>>>      /* Part 1: check device type */
> >>>>      switch (type) {
> >>>>      case VFL_TYPE_VIDEO:
> >>>> @@ -1127,6 +1152,8 @@ void video_unregister_device(struct video_device *vdev)
> >>>>      clear_bit(V4L2_FL_REGISTERED, &vdev->flags);
> >>>>      mutex_unlock(&videodev_lock);
> >>>>      v4l2_event_wake_all(vdev);
> >>>> +    if (test_bit(V4L2_FL_USES_STATE, &vdev->flags))
> >>>> +            __video_device_state_free(vdev->state);
> >>>>      device_unregister(&vdev->dev);
> >>>>  }
> >>>>  EXPORT_SYMBOL(video_unregister_device);
> >>>> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> >>>> index a213c3398dcf60be8c531df87bf40c56b4ad772d..57e4691ef467aa2b0782dd4b8357bd0670643293 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,17 @@ struct v4l2_file_operations {
> >>>>      int (*release) (struct file *);
> >>>>  };
> >>>>
> >>>> +/**
> >>>> + * struct video_device_state - Used for storing video device state information.
> >>>> + *
> >>>> + * @fmt: Format of the capture stream
> >>>> + * @vdev: Pointer to video device
>
> What else do you envisage to be included in the state?
>
> >>>> + */
> >>>> +struct video_device_state {
> >>>> +    struct v4l2_format fmt;
> >>>
> >>> While typically a video_device supports only a single video format type, that is
> >>> not always the case. There are the following exceptions:
> >>>
> >>> 1) M2M devices have both a capture and output video format. However, for M2M devices
> >>>    the state is per-filehandle, so it shouldn't be stored in a video_device_state
> >>>    struct anyway.
> >
> > Ah I see, so for M2M devices the formats are stored per-context, where the
> > context is tied to the filehandle. In that case, I agree that storing the
> > format state inside struct video_device would not work.
> >
> >>> 2) VBI devices can have both a raw and sliced VBI format (either capture or output)
> >>> 3) AFAIK non-M2M video devices can have both a video and meta format. That may have
> >>>    changed, I'm not 100% certain about this.
> >
> > RPi CFE driver is one such case, where a single driver structure stores
> > both metadata and video format. But if I understand correctly, it creates
> > separate video device nodes for metadata and video capture, so it can be
> > managed through a single v4l2_format.fmt union for each video device.
> >
> > Are there any non-M2M drivers which allow more than one type of formats to
> > be set on the same device node?
> >
> >>> 4) video devices can also support an OVERLAY or OUTPUT_OVERLAY format (rare)
> >>>
> >>> V4L2_CAP_VIDEO_OVERLAY is currently only used in
> >>> drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c, so once that driver
> >>> disappears we can drop video overlay support for capture devices.
> >
> > Yes, bcm2835-camera should be dropped hopefully in a couple more revisions of
> > https://lore.kernel.org/all/20250907-vchiq-destage-v2-4-6884505dca78@ideasonboard.com/
> >
> >>>
> >>> 2-4 are all quite rare, but 1 is very common. But for such devices the state
> >>> wouldn't be in video_device anyway.
> >>>
> >>> But it would be nice if the same struct can be used in both m2m devices and non-m2m
> >>> devices. It's just stored either in struct v4l2_fh or struct video_device. It would
> >>> give a lot of opportunities for creating helper functions to make the life for
> >>> driver developers easier.
> >
> > Sure, I think we can modify the existing state struct to store both capture
> > and output formats, and keep it inside struct v4l2_fh for M2M devices.
> >
> > This will definitely be confusing for driver developers, as currently the
> > two example patches in this series access the state directly. So I will add
> > framework helpers to access the correct state and format type, and document
> > properly that it should never be accessed manually by drivers.
>
> Would that be a similar approach as in v4l2_subdev_state?
>
> >> Follow-up: assuming we want to support M2M devices as well (I think we should), then
> >> consider renaming video_device_state since it isn't video_device specific, i.e. it
> >> can either live in video_device or in v4l2_fh, and in the latter case you'd have
> >> two instances: capture and output state.
> >
> > Argh, naming is the hardest problem :-)
> > Do you have any suggestions?
> >
> > I personally don't think video_device_state is a bad name, even if it is
> > stored somewhere else for m2m devices, given it is still the "state" of the
> > video device, even if it is not persistent across multiple file opens.
> >
> > I was trying to avoid names with "context" in then, so it does not clash
> > with Jacopo's work.
>
> It would be interesting how contexts (the new ones from Jacopo's series)
> are applied to mem2mem devices. And again, how the state introduced here
> is related to the (new) context.
>
> Best regards,
> Michael
>
> >
> >>
> >> Regards,
> >>
> >>         Hans
> >>
> >>>
> >>> Regards,
> >>>
> >>>       Hans
> >>>
> >>>> +    struct video_device *vdev;
> >>>> +};
> >>>> +
> >>>>  /*
> >>>>   * Newer version of video_device, handled by videodev2.c
> >>>>   *  This version moves redundant code from video device code to
> >>>> @@ -238,6 +255,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 +301,7 @@ struct video_device {
> >>>>      struct vb2_queue *queue;
> >>>>
> >>>>      struct v4l2_prio_state *prio;
> >>>> +    struct video_device_state *state;
> >>>>
> >>>>      /* device info */
> >>>>      char name[64];
> >>>> @@ -546,6 +565,27 @@ static inline int video_is_registered(struct video_device *vdev)
> >>>>      return test_bit(V4L2_FL_REGISTERED, &vdev->flags);
> >>>>  }
> >>>>
> >>>> +/** __video_device_state_alloc - allocate video device state structure
> >>>> + *
> >>>> + * @vdev: pointer to struct video_device
> >>>> + *
> >>>> + * .. note::
> >>>> + *
> >>>> + *  This function is meant to be used only inside the V4L2 core.
> >>>> + */
> >>>> +struct video_device_state *
> >>>> +__video_device_state_alloc(struct video_device *vdev);
> >>>> +
> >>>> +/** __video_device_state_free - free video device state structure
> >>>> + *
> >>>> + * @state: pointer to the state to be freed
> >>>> + *
> >>>> + * .. note::
> >>>> + *
> >>>> + *  This function is meant to be used only inside the V4L2 core.
> >>>> + */
> >>>> +void __video_device_state_free(struct video_device_state *state);
> >>>> +
> >>>>  /**
> >>>>   * v4l2_debugfs_root - returns the dentry of the top-level "v4l2" debugfs dir
> >>>>   *
> >>>>
> >>>
> >>>
> >>
> >
> > Thanks,
> > Jai
> >
>