| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <9beb643b-603d-46e8-9c1d-cd8060548507@kernel.org>
Date: Mon, 22 Sep 2025 10:00:05 +0200
From: Hans Verkuil <hverkuil+cisco@...nel.org>
To: Jai Luthra <jai.luthra@...asonboard.com>,
Hans Verkuil <hverkuil@...nel.org>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Sakari Ailus <sakari.ailus@...ux.intel.com>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Tomi Valkeinen <tomi.valkeinen@...asonboard.com>,
Jacopo Mondi <jacopo.mondi@...asonboard.com>, linux-media@...r.kernel.org
Cc: Ricardo Ribalda <ribalda@...omium.org>,
Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
Al Viro <viro@...iv.linux.org.uk>, Ma Ke <make24@...as.ac.cn>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 01/10] media: v4l2-core: Introduce state management for
video devices
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@...asonboard.com>
>> --
>> Cc: Mauro Carvalho Chehab <mchehab@...nel.org>
>> Cc: Hans Verkuil <hverkuil@...nel.org>
>> Cc: Ricardo Ribalda <ribalda@...omium.org>
>> Cc: Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>
>> Cc: Al Viro <viro@...iv.linux.org.uk>
>> Cc: Ma Ke <make24@...as.ac.cn>
>> Cc: Jai Luthra <jai.luthra@...asonboard.com>
>> Cc: linux-media@...r.kernel.org
>> Cc: linux-kernel@...r.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
>> *
>>
>
>
Powered by blists - more mailing lists