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

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

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