On 05/12/2023 15:08, Laurent Pinchart wrote:
> Due to a historical mishap, the v4l2_subdev_frame_interval structure
> is the only part of the V4L2 subdev userspace API that doesn't contain a
> 'which' field. This prevents trying frame intervals using the subdev
> 'TRY' state mechanism.
>
> Adding a 'which' field is simple as the structure has 8 reserved fields.
> This would however break userspace as the field is currently set to 0,
> corresponding to V4L2_SUBDEV_FORMAT_TRY, while the corresponding ioctls
> currently operate on the 'ACTIVE' state. We thus need to add a new
> subdev client cap, V4L2_SUBDEV_CLIENT_CAP_WHICH_INTERVAL, to indicate
> that userspace is aware of this new field.
>
> All drivers that implement the subdev .get_frame_interval() and
> .set_frame_interval() operations are updated to return -EINVAL when
> operating on the TRY state, preserving the current behaviour.
>
> While at it, fix a bad copy&paste in the documentation of the struct
> v4l2_subdev_frame_interval_enum 'which' field.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Reviewed-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> # for imx-media
> Reviewed-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
> ---
> .../media/v4l/vidioc-subdev-g-client-cap.rst | 5 ++++
> .../v4l/vidioc-subdev-g-frame-interval.rst | 17 ++++++++-----
> drivers/media/i2c/adv7180.c | 3 +++
> drivers/media/i2c/et8ek8/et8ek8_driver.c | 6 +++++
> drivers/media/i2c/imx214.c | 3 +++
> drivers/media/i2c/imx274.c | 6 +++++
> drivers/media/i2c/max9286.c | 6 +++++
> drivers/media/i2c/mt9m111.c | 6 +++++
> drivers/media/i2c/mt9m114.c | 6 +++++
> drivers/media/i2c/mt9v011.c | 6 +++++
> drivers/media/i2c/mt9v111.c | 6 +++++
> drivers/media/i2c/ov2680.c | 3 +++
> drivers/media/i2c/ov5640.c | 6 +++++
> drivers/media/i2c/ov5648.c | 3 +++
> drivers/media/i2c/ov5693.c | 3 +++
> drivers/media/i2c/ov6650.c | 6 +++++
> drivers/media/i2c/ov7251.c | 6 +++++
> drivers/media/i2c/ov7670.c | 4 +++
> drivers/media/i2c/ov772x.c | 6 +++++
> drivers/media/i2c/ov8865.c | 3 +++
> drivers/media/i2c/ov9650.c | 6 +++++
> drivers/media/i2c/s5c73m3/s5c73m3-core.c | 6 +++++
> drivers/media/i2c/s5k5baf.c | 6 +++++
> drivers/media/i2c/tvp514x.c | 4 +++
> drivers/media/v4l2-core/v4l2-subdev.c | 25 ++++++++++++-------
> .../media/atomisp/i2c/atomisp-gc0310.c | 3 +++
> .../media/atomisp/i2c/atomisp-gc2235.c | 3 +++
> .../media/atomisp/i2c/atomisp-mt9m114.c | 3 +++
> .../media/atomisp/i2c/atomisp-ov2722.c | 3 +++
> drivers/staging/media/imx/imx-ic-prp.c | 6 +++++
> drivers/staging/media/imx/imx-ic-prpencvf.c | 6 +++++
> drivers/staging/media/imx/imx-media-csi.c | 6 +++++
> drivers/staging/media/imx/imx-media-vdic.c | 6 +++++
> drivers/staging/media/tegra-video/csi.c | 3 +++
> include/uapi/linux/v4l2-subdev.h | 13 ++++++++--
> 35 files changed, 192 insertions(+), 17 deletions(-)
>
<snip>
> diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
> index f0fbb4a7c150..cc2717f734e7 100644
> --- a/include/uapi/linux/v4l2-subdev.h
> +++ b/include/uapi/linux/v4l2-subdev.h
> @@ -116,13 +116,15 @@ struct v4l2_subdev_frame_size_enum {
> * @pad: pad number, as reported by the media API
> * @interval: frame interval in seconds
> * @stream: stream number, defined in subdev routing
> + * @which: interval type (from enum v4l2_subdev_format_whence)
> * @reserved: drivers and applications must zero this array
> */
> struct v4l2_subdev_frame_interval {
> __u32 pad;
> struct v4l2_fract interval;
> __u32 stream;
> - __u32 reserved[8];
> + __u32 which;
> + __u32 reserved[7];
> };
>
> /**
> @@ -133,7 +135,7 @@ struct v4l2_subdev_frame_interval {
> * @width: frame width in pixels
> * @height: frame height in pixels
> * @interval: frame interval in seconds
> - * @which: format type (from enum v4l2_subdev_format_whence)
> + * @which: interval type (from enum v4l2_subdev_format_whence)
> * @stream: stream number, defined in subdev routing
> * @reserved: drivers and applications must zero this array
> */
> @@ -241,6 +243,13 @@ struct v4l2_subdev_routing {
> */
> #define V4L2_SUBDEV_CLIENT_CAP_STREAMS (1ULL << 0)
>
> +/*
> + * The client is aware of the struct v4l2_subdev_frame_interval which field. If
> + * this is not set (which is the default), the which field is forced to
> + * V4L2_SUBDEV_FORMAT_ACTIVE by the kernel.
> + */
> +#define V4L2_SUBDEV_CLIENT_CAP_WHICH_INTERVAL (1U << 1)
1U -> 1ULL
I also think that the define should be renamed to _CAP_INTERVAL_WHICH.
I keep reading "WHICH_INTERVAL" as "Which interval?" :-)
Also, you generally first put the structure before the field name,
so INTERVAL_WHICH feels much more logical to me.
Or possibly: INTERVAL_USES_WHICH, which is even more descriptive.
Regards,
Hans
> +
> /**
> * struct v4l2_subdev_client_capability - Capabilities of the client accessing
> * the subdev
