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

[Hans Verkuil <hverkuil+cisco@kernel.org>]

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
>   *
>