On 27/11/2023 12:13, 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>
> ---
> Changes since v1:
>
> - Fix .[gs]et_frame_interval() operation names in commit message
> - Fix typo in commit message
> ---
> .../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/thp7312.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 ++++++++--
> 36 files changed, 198 insertions(+), 17 deletions(-)
>
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-client-cap.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-client-cap.rst
> index 20f12a1cc0f7..f168140ebd59 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-client-cap.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-client-cap.rst
> @@ -71,6 +71,11 @@ is unknown to the kernel.
> of 'stream' fields (referring to the stream number) with various
> ioctls. If this is not set (which is the default), the 'stream' fields
> will be forced to 0 by the kernel.
> + * - ``V4L2_SUBDEV_CLIENT_CAP_WHICH_INTERVAL``
> + - The client is aware of the :c:type:`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.
>
> Return Value
> ============
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.rst
> index 842f962d2aea..41e0e2c8ecc3 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.rst
> @@ -58,8 +58,9 @@ struct
> contains the current frame interval as would be returned by a
> ``VIDIOC_SUBDEV_G_FRAME_INTERVAL`` call.
>
> -Calling ``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` on a subdev device node that has been
> -registered in read-only mode is not allowed. An error is returned and the errno
> +If the subdev device node has been registered in read-only mode, calls to
> +``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` are only valid if the ``which`` field is set
> +to ``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno
> variable is set to ``-EPERM``.
>
> Drivers must not return an error solely because the requested interval
> @@ -93,7 +94,11 @@ the same sub-device is not defined.
> - ``stream``
> - Stream identifier.
> * - __u32
> - - ``reserved``\ [8]
> + - ``which``
> + - Active or try frame interval, from enum
> + :ref:`v4l2_subdev_format_whence <v4l2-subdev-format-whence>`.
> + * - __u32
> + - ``reserved``\ [7]
> - Reserved for future extensions. Applications and drivers must set
> the array to zero.
>
> @@ -114,9 +119,9 @@ EBUSY
> EINVAL
> The struct
> :c:type:`v4l2_subdev_frame_interval`
> - ``pad`` references a non-existing pad, or the pad doesn't support
> - frame intervals.
> + ``pad`` references a non-existing pad, the ``which`` field references a
> + non-existing frame interval, or the pad doesn't support frame intervals.
"the ``which`` field references a non-existing frame interval": that's a rather
vague sentence. I noticed it was probably copy-and-pasted (VIDIOC_SUBDEV_G_FMT has
a similar phrase), but it is not clear in that documentation either.
I expect EINVAL if 'which' is set to something other than TRY or ACTIVE, or the
driver does not support TRY. Is that what you meant with "references a non-existing
frame interval"?
The 'references a non-existing' phrase works for pads since pad is an index
into a pad array, but that's not the case for 'which', which is effectively an
enum, so there is no obvious indexing going on.
I think a separate patch clarifying this EINVAL description for the relevant subdev
ioctls might be useful.
In any case, since this just copies existing text it isn't a blocker.
>
> EPERM
> The ``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` ioctl has been called on a read-only
> - subdevice.
> + subdevice and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.
> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> index 7ed86030fb5c..e1eec9f86539 100644
> --- a/drivers/media/i2c/adv7180.c
> +++ b/drivers/media/i2c/adv7180.c
> @@ -469,6 +469,9 @@ static int adv7180_get_frame_interval(struct v4l2_subdev *sd,
> {
> struct adv7180_state *state = to_state(sd);
>
> + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> if (state->curr_norm & V4L2_STD_525_60) {
> fi->interval.numerator = 1001;
> fi->interval.denominator = 30000;
> diff --git a/drivers/media/i2c/et8ek8/et8ek8_driver.c b/drivers/media/i2c/et8ek8/et8ek8_driver.c
> index 71fb5aebd3df..359ed943533c 100644
> --- a/drivers/media/i2c/et8ek8/et8ek8_driver.c
> +++ b/drivers/media/i2c/et8ek8/et8ek8_driver.c
> @@ -1051,6 +1051,9 @@ static int et8ek8_get_frame_interval(struct v4l2_subdev *subdev,
> {
> struct et8ek8_sensor *sensor = to_et8ek8_sensor(subdev);
>
> + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> memset(fi, 0, sizeof(*fi));
> fi->interval = sensor->current_reglist->mode.timeperframe;
>
> @@ -1064,6 +1067,9 @@ static int et8ek8_set_frame_interval(struct v4l2_subdev *subdev,
> struct et8ek8_sensor *sensor = to_et8ek8_sensor(subdev);
> struct et8ek8_reglist *reglist;
>
> + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> reglist = et8ek8_reglist_find_mode_ival(&meta_reglist,
> sensor->current_reglist,
> &fi->interval);
> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> index 624efc8834f3..80d14bc6f1ca 100644
> --- a/drivers/media/i2c/imx214.c
> +++ b/drivers/media/i2c/imx214.c
> @@ -799,6 +799,9 @@ static int imx214_get_frame_interval(struct v4l2_subdev *subdev,
> struct v4l2_subdev_state *sd_state,
> struct v4l2_subdev_frame_interval *fival)
> {
> + if (fival->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> fival->interval.numerator = 1;
> fival->interval.denominator = IMX214_FPS;
>
> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
> index 4040c642a36f..9f9fb3c488e2 100644
> --- a/drivers/media/i2c/imx274.c
> +++ b/drivers/media/i2c/imx274.c
> @@ -1333,6 +1333,9 @@ static int imx274_get_frame_interval(struct v4l2_subdev *sd,
> {
> struct stimx274 *imx274 = to_imx274(sd);
>
> + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> fi->interval = imx274->frame_interval;
> dev_dbg(&imx274->client->dev, "%s frame rate = %d / %d\n",
> __func__, imx274->frame_interval.numerator,
> @@ -1350,6 +1353,9 @@ static int imx274_set_frame_interval(struct v4l2_subdev *sd,
> int min, max, def;
> int ret;
>
> + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> ret = pm_runtime_resume_and_get(&imx274->client->dev);
> if (ret < 0)
> return ret;
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 7e8cb53d31c3..16f81479d411 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -874,6 +874,9 @@ static int max9286_get_frame_interval(struct v4l2_subdev *sd,
> {
> struct max9286_priv *priv = sd_to_max9286(sd);
>
> + if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> if (interval->pad != MAX9286_SRC_PAD)
> return -EINVAL;
>
> @@ -888,6 +891,9 @@ static int max9286_set_frame_interval(struct v4l2_subdev *sd,
> {
> struct max9286_priv *priv = sd_to_max9286(sd);
>
> + if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> if (interval->pad != MAX9286_SRC_PAD)
> return -EINVAL;
>
> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> index 602954650f2e..a30c17594b8e 100644
> --- a/drivers/media/i2c/mt9m111.c
> +++ b/drivers/media/i2c/mt9m111.c
> @@ -1051,6 +1051,9 @@ static int mt9m111_get_frame_interval(struct v4l2_subdev *sd,
> {
> struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev);
>
> + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> fi->interval = mt9m111->frame_interval;
>
> return 0;
> @@ -1068,6 +1071,9 @@ static int mt9m111_set_frame_interval(struct v4l2_subdev *sd,
> if (mt9m111->is_streaming)
> return -EBUSY;
>
> + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> if (fi->pad != 0)
> return -EINVAL;
>
> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
> index dcd94299787c..5e0d85b3d158 100644
> --- a/drivers/media/i2c/mt9m114.c
> +++ b/drivers/media/i2c/mt9m114.c
> @@ -1592,6 +1592,9 @@ static int mt9m114_ifp_get_frame_interval(struct v4l2_subdev *sd,
> struct v4l2_fract *ival = &interval->interval;
> struct mt9m114 *sensor = ifp_to_mt9m114(sd);
>
> + if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> mutex_lock(sensor->ifp.hdl.lock);
>
> ival->numerator = 1;
> @@ -1610,6 +1613,9 @@ static int mt9m114_ifp_set_frame_interval(struct v4l2_subdev *sd,
> struct mt9m114 *sensor = ifp_to_mt9m114(sd);
> int ret = 0;
>
> + if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> mutex_lock(sensor->ifp.hdl.lock);
>
> if (ival->numerator != 0 && ival->denominator != 0)
> diff --git a/drivers/media/i2c/mt9v011.c b/drivers/media/i2c/mt9v011.c
> index 3485761428ba..bc8c0591e4ae 100644
> --- a/drivers/media/i2c/mt9v011.c
> +++ b/drivers/media/i2c/mt9v011.c
> @@ -366,6 +366,9 @@ static int mt9v011_get_frame_interval(struct v4l2_subdev *sd,
> struct v4l2_subdev_state *sd_state,
> struct v4l2_subdev_frame_interval *ival)
> {
> + if (ival->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> calc_fps(sd,
> &ival->interval.numerator,
> &ival->interval.denominator);
> @@ -380,6 +383,9 @@ static int mt9v011_set_frame_interval(struct v4l2_subdev *sd,
> struct v4l2_fract *tpf = &ival->interval;
> u16 speed;
>
> + if (ival->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> speed = calc_speed(sd, tpf->numerator, tpf->denominator);
>
> mt9v011_write(sd, R0A_MT9V011_CLK_SPEED, speed);
> diff --git a/drivers/media/i2c/mt9v111.c b/drivers/media/i2c/mt9v111.c
> index 496be67c971b..b62624771e7b 100644
> --- a/drivers/media/i2c/mt9v111.c
> +++ b/drivers/media/i2c/mt9v111.c
> @@ -730,6 +730,9 @@ static int mt9v111_set_frame_interval(struct v4l2_subdev *sd,
> tpf->denominator;
> unsigned int max_fps;
>
> + if (ival->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> if (!tpf->numerator)
> tpf->numerator = 1;
>
> @@ -779,6 +782,9 @@ static int mt9v111_get_frame_interval(struct v4l2_subdev *sd,
> struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd);
> struct v4l2_fract *tpf = &ival->interval;
>
> + if (ival->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> mutex_lock(&mt9v111->stream_mutex);
>
> tpf->numerator = 1;
> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
> index e3ff64a9e6ca..5455e7afd1b3 100644
> --- a/drivers/media/i2c/ov2680.c
> +++ b/drivers/media/i2c/ov2680.c
> @@ -558,6 +558,9 @@ static int ov2680_get_frame_interval(struct v4l2_subdev *sd,
> {
> struct ov2680_dev *sensor = to_ov2680_dev(sd);
>
> + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> mutex_lock(&sensor->lock);
> fi->interval = sensor->mode.frame_interval;
> mutex_unlock(&sensor->lock);
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 336bfd1ffd32..2d75a67a3aff 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -3610,6 +3610,9 @@ static int ov5640_get_frame_interval(struct v4l2_subdev *sd,
> {
> struct ov5640_dev *sensor = to_ov5640_dev(sd);
>
> + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> mutex_lock(&sensor->lock);
> fi->interval = sensor->frame_interval;
> mutex_unlock(&sensor->lock);
> @@ -3625,6 +3628,9 @@ static int ov5640_set_frame_interval(struct v4l2_subdev *sd,
> const struct ov5640_mode_info *mode;
> int frame_rate, ret = 0;
>
> + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> if (fi->pad != 0)
> return -EINVAL;
>
> diff --git a/drivers/media/i2c/ov5648.c b/drivers/media/i2c/ov5648.c
> index d0d7e9968f48..f02a7e263aee 100644
> --- a/drivers/media/i2c/ov5648.c
> +++ b/drivers/media/i2c/ov5648.c
> @@ -2276,6 +2276,9 @@ static int ov5648_get_frame_interval(struct v4l2_subdev *subdev,
> const struct ov5648_mode *mode;
> int ret = 0;
>
> + if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> mutex_lock(&sensor->mutex);
>
> mode = sensor->state.mode;
> diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
> index a65645811fbc..ce49176560d4 100644
> --- a/drivers/media/i2c/ov5693.c
> +++ b/drivers/media/i2c/ov5693.c
> @@ -1013,6 +1013,9 @@ static int ov5693_get_frame_interval(struct v4l2_subdev *sd,
> ov5693->ctrls.vblank->val);
> unsigned int fps = DIV_ROUND_CLOSEST(OV5693_PIXEL_RATE, framesize);
>
> + if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> interval->interval.numerator = 1;
> interval->interval.denominator = fps;
>
> diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
> index a4dc45bdf3d7..4ef2b7db315e 100644
> --- a/drivers/media/i2c/ov6650.c
> +++ b/drivers/media/i2c/ov6650.c
> @@ -806,6 +806,9 @@ static int ov6650_get_frame_interval(struct v4l2_subdev *sd,
> struct i2c_client *client = v4l2_get_subdevdata(sd);
> struct ov6650 *priv = to_ov6650(client);
>
> + if (ival->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> ival->interval = priv->tpf;
>
> dev_dbg(&client->dev, "Frame interval: %u/%u s\n",
> @@ -823,6 +826,9 @@ static int ov6650_set_frame_interval(struct v4l2_subdev *sd,
> struct v4l2_fract *tpf = &ival->interval;
> int div, ret;
>
> + if (ival->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> if (tpf->numerator == 0 || tpf->denominator == 0)
> div = 1; /* Reset to full rate */
> else
> diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
> index 10d6b5deed83..08f5f2d0538d 100644
> --- a/drivers/media/i2c/ov7251.c
> +++ b/drivers/media/i2c/ov7251.c
> @@ -1391,6 +1391,9 @@ static int ov7251_get_frame_interval(struct v4l2_subdev *subdev,
> {
> struct ov7251 *ov7251 = to_ov7251(subdev);
>
> + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> mutex_lock(&ov7251->lock);
> fi->interval = ov7251->current_mode->timeperframe;
> mutex_unlock(&ov7251->lock);
> @@ -1406,6 +1409,9 @@ static int ov7251_set_frame_interval(struct v4l2_subdev *subdev,
> const struct ov7251_mode_info *new_mode;
> int ret = 0;
>
> + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> mutex_lock(&ov7251->lock);
> new_mode = ov7251_find_mode_by_ival(ov7251, &fi->interval);
>
> diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
> index 463f20ece36e..7874a8dd7cf0 100644
> --- a/drivers/media/i2c/ov7670.c
> +++ b/drivers/media/i2c/ov7670.c
> @@ -1160,6 +1160,8 @@ static int ov7670_get_frame_interval(struct v4l2_subdev *sd,
> {
> struct ov7670_info *info = to_state(sd);
>
> + if (ival->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
>
> info->devtype->get_framerate(sd, &ival->interval);
>
> @@ -1173,6 +1175,8 @@ static int ov7670_set_frame_interval(struct v4l2_subdev *sd,
> struct v4l2_fract *tpf = &ival->interval;
> struct ov7670_info *info = to_state(sd);
>
> + if (ival->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
>
> return info->devtype->set_framerate(sd, tpf);
> }
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index a14a25946c5b..d9a73871f7a3 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -724,6 +724,9 @@ static int ov772x_get_frame_interval(struct v4l2_subdev *sd,
> struct ov772x_priv *priv = to_ov772x(sd);
> struct v4l2_fract *tpf = &ival->interval;
>
> + if (ival->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> tpf->numerator = 1;
> tpf->denominator = priv->fps;
>
> @@ -739,6 +742,9 @@ static int ov772x_set_frame_interval(struct v4l2_subdev *sd,
> unsigned int fps;
> int ret = 0;
>
> + if (ival->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> mutex_lock(&priv->lock);
>
> if (priv->streaming) {
> diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
> index 02a595281c49..7a25dcd900f2 100644
> --- a/drivers/media/i2c/ov8865.c
> +++ b/drivers/media/i2c/ov8865.c
> @@ -2846,6 +2846,9 @@ static int ov8865_get_frame_interval(struct v4l2_subdev *subdev,
> unsigned int framesize;
> unsigned int fps;
>
> + if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> mutex_lock(&sensor->mutex);
>
> mode = sensor->state.mode;
> diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c
> index f528892c893f..08be6c4fc6d5 100644
> --- a/drivers/media/i2c/ov9650.c
> +++ b/drivers/media/i2c/ov9650.c
> @@ -1107,6 +1107,9 @@ static int ov965x_get_frame_interval(struct v4l2_subdev *sd,
> {
> struct ov965x *ov965x = to_ov965x(sd);
>
> + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> mutex_lock(&ov965x->lock);
> fi->interval = ov965x->fiv->interval;
> mutex_unlock(&ov965x->lock);
> @@ -1156,6 +1159,9 @@ static int ov965x_set_frame_interval(struct v4l2_subdev *sd,
> struct ov965x *ov965x = to_ov965x(sd);
> int ret;
>
> + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> v4l2_dbg(1, debug, sd, "Setting %d/%d frame interval\n",
> fi->interval.numerator, fi->interval.denominator);
>
> diff --git a/drivers/media/i2c/s5c73m3/s5c73m3-core.c b/drivers/media/i2c/s5c73m3/s5c73m3-core.c
> index 73ca50f49812..71a51794ced4 100644
> --- a/drivers/media/i2c/s5c73m3/s5c73m3-core.c
> +++ b/drivers/media/i2c/s5c73m3/s5c73m3-core.c
> @@ -872,6 +872,9 @@ static int s5c73m3_oif_get_frame_interval(struct v4l2_subdev *sd,
> {
> struct s5c73m3 *state = oif_sd_to_s5c73m3(sd);
>
> + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> if (fi->pad != OIF_SOURCE_PAD)
> return -EINVAL;
>
> @@ -923,6 +926,9 @@ static int s5c73m3_oif_set_frame_interval(struct v4l2_subdev *sd,
> struct s5c73m3 *state = oif_sd_to_s5c73m3(sd);
> int ret;
>
> + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> if (fi->pad != OIF_SOURCE_PAD)
> return -EINVAL;
>
> diff --git a/drivers/media/i2c/s5k5baf.c b/drivers/media/i2c/s5k5baf.c
> index 2fd1ecfeb086..6b1a2c4946a9 100644
> --- a/drivers/media/i2c/s5k5baf.c
> +++ b/drivers/media/i2c/s5k5baf.c
> @@ -1124,6 +1124,9 @@ static int s5k5baf_get_frame_interval(struct v4l2_subdev *sd,
> {
> struct s5k5baf *state = to_s5k5baf(sd);
>
> + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> mutex_lock(&state->lock);
> fi->interval.numerator = state->fiv;
> fi->interval.denominator = 10000;
> @@ -1162,6 +1165,9 @@ static int s5k5baf_set_frame_interval(struct v4l2_subdev *sd,
> {
> struct s5k5baf *state = to_s5k5baf(sd);
>
> + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> mutex_lock(&state->lock);
> __s5k5baf_set_frame_interval(state, fi);
> mutex_unlock(&state->lock);
> diff --git a/drivers/media/i2c/thp7312.c b/drivers/media/i2c/thp7312.c
> index 1698f3faa7cd..c8f42a588002 100644
> --- a/drivers/media/i2c/thp7312.c
> +++ b/drivers/media/i2c/thp7312.c
> @@ -740,6 +740,9 @@ static int thp7312_get_frame_interval(struct v4l2_subdev *sd,
> {
> struct thp7312_device *thp7312 = to_thp7312_dev(sd);
>
> + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> fi->interval.numerator = 1;
> fi->interval.denominator = thp7312->current_rate->fps;
>
> @@ -757,6 +760,9 @@ static int thp7312_set_frame_interval(struct v4l2_subdev *sd,
> unsigned int best_delta = UINT_MAX;
> unsigned int fps;
>
> + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> /* Avoid divisions by 0, pick the highest frame if the interval is 0. */
> fps = fi->interval.numerator
> ? DIV_ROUND_CLOSEST(fi->interval.denominator, fi->interval.numerator)
> diff --git a/drivers/media/i2c/tvp514x.c b/drivers/media/i2c/tvp514x.c
> index dee0cf992379..ae073a532eda 100644
> --- a/drivers/media/i2c/tvp514x.c
> +++ b/drivers/media/i2c/tvp514x.c
> @@ -746,6 +746,8 @@ tvp514x_get_frame_interval(struct v4l2_subdev *sd,
> struct tvp514x_decoder *decoder = to_decoder(sd);
> enum tvp514x_std current_std;
>
> + if (ival->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
>
> /* get the current standard */
> current_std = decoder->current_std;
> @@ -765,6 +767,8 @@ tvp514x_set_frame_interval(struct v4l2_subdev *sd,
> struct v4l2_fract *timeperframe;
> enum tvp514x_std current_std;
>
> + if (ival->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
>
> timeperframe = &ival->interval;
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 08c908988f7d..4cbe4024ff67 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -291,9 +291,8 @@ static inline int check_frame_interval(struct v4l2_subdev *sd,
> if (!fi)
> return -EINVAL;
>
> - return check_pad(sd, fi->pad) ? :
> - check_state(sd, state, V4L2_SUBDEV_FORMAT_ACTIVE, fi->pad,
> - fi->stream);
> + return check_which(fi->which) ? : check_pad(sd, fi->pad) ? :
> + check_state(sd, state, fi->which, fi->pad, fi->stream);
> }
>
> static int call_get_frame_interval(struct v4l2_subdev *sd,
> @@ -537,9 +536,16 @@ subdev_ioctl_get_state(struct v4l2_subdev *sd, struct v4l2_subdev_fh *subdev_fh,
> which = ((struct v4l2_subdev_selection *)arg)->which;
> break;
> case VIDIOC_SUBDEV_G_FRAME_INTERVAL:
> - case VIDIOC_SUBDEV_S_FRAME_INTERVAL:
> - which = V4L2_SUBDEV_FORMAT_ACTIVE;
> + case VIDIOC_SUBDEV_S_FRAME_INTERVAL: {
> + struct v4l2_subdev_frame_interval *fi = arg;
> +
> + if (!(subdev_fh->client_caps &
> + V4L2_SUBDEV_CLIENT_CAP_WHICH_INTERVAL))
> + fi->which = V4L2_SUBDEV_FORMAT_ACTIVE;
> +
> + which = fi->which;
> break;
> + }
> case VIDIOC_SUBDEV_G_ROUTING:
> case VIDIOC_SUBDEV_S_ROUTING:
> which = ((struct v4l2_subdev_routing *)arg)->which;
> @@ -796,12 +802,12 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
> case VIDIOC_SUBDEV_S_FRAME_INTERVAL: {
> struct v4l2_subdev_frame_interval *fi = arg;
>
> - if (ro_subdev)
> - return -EPERM;
> -
> if (!client_supports_streams)
> fi->stream = 0;
>
> + if (fi->which != V4L2_SUBDEV_FORMAT_TRY && ro_subdev)
> + return -EPERM;
> +
> memset(fi->reserved, 0, sizeof(fi->reserved));
> return v4l2_subdev_call(sd, pad, set_frame_interval, state, fi);
> }
> @@ -998,7 +1004,8 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
> client_cap->capabilities &= ~V4L2_SUBDEV_CLIENT_CAP_STREAMS;
>
> /* Filter out unsupported capabilities */
> - client_cap->capabilities &= V4L2_SUBDEV_CLIENT_CAP_STREAMS;
> + client_cap->capabilities &= (V4L2_SUBDEV_CLIENT_CAP_STREAMS |
> + V4L2_SUBDEV_CLIENT_CAP_WHICH_INTERVAL);
>
> subdev_fh->client_caps = client_cap->capabilities;
>
> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> index 006e8adac47b..3a032e1a06f8 100644
> --- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> +++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> @@ -500,6 +500,9 @@ static int gc0310_get_frame_interval(struct v4l2_subdev *sd,
> struct v4l2_subdev_state *sd_state,
> struct v4l2_subdev_frame_interval *interval)
> {
> + if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> interval->interval.numerator = 1;
> interval->interval.denominator = GC0310_FPS;
>
> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c b/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c
> index aa257322a700..a2bbe2864049 100644
> --- a/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c
> +++ b/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c
> @@ -704,6 +704,9 @@ static int gc2235_get_frame_interval(struct v4l2_subdev *sd,
> {
> struct gc2235_device *dev = to_gc2235_sensor(sd);
>
> + if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> interval->interval.numerator = 1;
> interval->interval.denominator = dev->res->fps;
>
> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c b/drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c
> index 459c5b8233ce..b4be6d3120b1 100644
> --- a/drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c
> +++ b/drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c
> @@ -1394,6 +1394,9 @@ static int mt9m114_get_frame_interval(struct v4l2_subdev *sd,
> {
> struct mt9m114_device *dev = to_mt9m114_sensor(sd);
>
> + if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> interval->interval.numerator = 1;
> interval->interval.denominator = mt9m114_res[dev->res].fps;
>
> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
> index b3ef04d7ccca..64e1addfc1c5 100644
> --- a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
> +++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
> @@ -851,6 +851,9 @@ static int ov2722_get_frame_interval(struct v4l2_subdev *sd,
> {
> struct ov2722_device *dev = to_ov2722_sensor(sd);
>
> + if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> interval->interval.numerator = 1;
> interval->interval.denominator = dev->res->fps;
>
> diff --git a/drivers/staging/media/imx/imx-ic-prp.c b/drivers/staging/media/imx/imx-ic-prp.c
> index fb96f87e664e..26c758b67bf2 100644
> --- a/drivers/staging/media/imx/imx-ic-prp.c
> +++ b/drivers/staging/media/imx/imx-ic-prp.c
> @@ -399,6 +399,9 @@ static int prp_get_frame_interval(struct v4l2_subdev *sd,
> {
> struct prp_priv *priv = sd_to_priv(sd);
>
> + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> if (fi->pad >= PRP_NUM_PADS)
> return -EINVAL;
>
> @@ -415,6 +418,9 @@ static int prp_set_frame_interval(struct v4l2_subdev *sd,
> {
> struct prp_priv *priv = sd_to_priv(sd);
>
> + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> if (fi->pad >= PRP_NUM_PADS)
> return -EINVAL;
>
> diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
> index 7bfe433cd322..94a8ace3fa7f 100644
> --- a/drivers/staging/media/imx/imx-ic-prpencvf.c
> +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
> @@ -1209,6 +1209,9 @@ static int prp_get_frame_interval(struct v4l2_subdev *sd,
> {
> struct prp_priv *priv = sd_to_priv(sd);
>
> + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> if (fi->pad >= PRPENCVF_NUM_PADS)
> return -EINVAL;
>
> @@ -1225,6 +1228,9 @@ static int prp_set_frame_interval(struct v4l2_subdev *sd,
> {
> struct prp_priv *priv = sd_to_priv(sd);
>
> + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> if (fi->pad >= PRPENCVF_NUM_PADS)
> return -EINVAL;
>
> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> index 4308fdc9b58e..9af5a0d5ace4 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -908,6 +908,9 @@ static int csi_get_frame_interval(struct v4l2_subdev *sd,
> {
> struct csi_priv *priv = v4l2_get_subdevdata(sd);
>
> + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> if (fi->pad >= CSI_NUM_PADS)
> return -EINVAL;
>
> @@ -928,6 +931,9 @@ static int csi_set_frame_interval(struct v4l2_subdev *sd,
> struct v4l2_fract *input_fi;
> int ret = 0;
>
> + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> mutex_lock(&priv->lock);
>
> input_fi = &priv->frame_interval[CSI_SINK_PAD];
> diff --git a/drivers/staging/media/imx/imx-media-vdic.c b/drivers/staging/media/imx/imx-media-vdic.c
> index a51b37679239..d34e11d925a1 100644
> --- a/drivers/staging/media/imx/imx-media-vdic.c
> +++ b/drivers/staging/media/imx/imx-media-vdic.c
> @@ -786,6 +786,9 @@ static int vdic_get_frame_interval(struct v4l2_subdev *sd,
> {
> struct vdic_priv *priv = v4l2_get_subdevdata(sd);
>
> + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> if (fi->pad >= VDIC_NUM_PADS)
> return -EINVAL;
>
> @@ -806,6 +809,9 @@ static int vdic_set_frame_interval(struct v4l2_subdev *sd,
> struct v4l2_fract *input_fi, *output_fi;
> int ret = 0;
>
> + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> mutex_lock(&priv->lock);
>
> input_fi = &priv->frame_interval[priv->active_input_pad];
> diff --git a/drivers/staging/media/tegra-video/csi.c b/drivers/staging/media/tegra-video/csi.c
> index b1b666179be5..a2ce8d025eaf 100644
> --- a/drivers/staging/media/tegra-video/csi.c
> +++ b/drivers/staging/media/tegra-video/csi.c
> @@ -231,6 +231,9 @@ static int tegra_csi_get_frame_interval(struct v4l2_subdev *subdev,
> if (!IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG))
> return -ENOIOCTLCMD;
>
> + if (vfi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> + return -EINVAL;
> +
> vfi->interval.numerator = 1;
> vfi->interval.denominator = csi_chan->framerate;
>
> diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
> index 4a195b68f28f..995b4f442a0d 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 (1U << 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)
> +
> /**
> * struct v4l2_subdev_client_capability - Capabilities of the client accessing
> * the subdev
Reviewed-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
Regards,
Hans
