[v4,3/8] media: v4l2-subdev: Add which field to struct v4l2_subdev_frame_interval

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@ideasonboard.com>
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> # for imx-media
Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
Changes since v3:

- Rename V4L2_SUBDEV_CLIENT_CAP_WHICH_INTERVAL to
  V4L2_SUBDEV_CLIENT_CAP_INTERVAL_USES_WHICH
- Use ULL for bit constant
- Address thp7312 driver
---
 .../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              | 15 ++++++++---
 36 files changed, 199 insertions(+), 18 deletions(-)

Em Fri,  8 Dec 2023 20:16:43 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

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

The uAPI change looks ok to me. However, having to add an special
logic on every client using (get/set)_time_interval seems risky,
as it will require an extra check before accepting new subdev
drivers.

Please move such check to drivers/media/v4l2-core/v4l2-subdev.c:

	if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
		return -EINVAL;

and avoid adding backward-compatible uAPI code inside drivers.

Regards,
Mauro

> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> # for imx-media
> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
> Changes since v3:
> 
> - Rename V4L2_SUBDEV_CLIENT_CAP_WHICH_INTERVAL to
>   V4L2_SUBDEV_CLIENT_CAP_INTERVAL_USES_WHICH
> - Use ULL for bit constant
> - Address thp7312 driver
> ---
>  .../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              | 15 ++++++++---
>  36 files changed, 199 insertions(+), 18 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..810b6a859dc8 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_INTERVAL_USES_WHICH``
> +      - 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.
>  
>  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 d4975b180704..49eda07f1c96 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..b9ce3e41d52c 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_INTERVAL_USES_WHICH))
> +			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_INTERVAL_USES_WHICH);
>  
>  		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 f0fbb4a7c150..7048c51581c6 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
>   */
> @@ -239,7 +241,14 @@ struct v4l2_subdev_routing {
>   * set (which is the default), the 'stream' fields will be forced to 0 by the
>   * kernel.
>   */
> -#define V4L2_SUBDEV_CLIENT_CAP_STREAMS		(1ULL << 0)
> +#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_INTERVAL_USES_WHICH	(1ULL << 1)
>  
>  /**
>   * struct v4l2_subdev_client_capability - Capabilities of the client accessing



Thanks,
Mauro

On Sat, Dec 09, 2023 at 06:55:01AM +0100, Mauro Carvalho Chehab wrote:
> Em Fri,  8 Dec 2023 20:16:43 +0200
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
> 
> > 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.
> 
> The uAPI change looks ok to me. However, having to add an special
> logic on every client using (get/set)_time_interval seems risky,
> as it will require an extra check before accepting new subdev
> drivers.
> 
> Please move such check to drivers/media/v4l2-core/v4l2-subdev.c:
> 
> 	if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> 		return -EINVAL;

But then no new driver will be able to use this API. Look at patch 8/8
in this series, the whole point of this exercise is to support
V4L2_SUBDEV_FORMAT_TRY in drivers. The added checks in existing drivers
is because they don't support V4L2_SUBDEV_FORMAT_TRY. Moving forward,
the drivers that are still maintained should be converted eventually.
All new drivers should use the V4L2 subdev active state API and support
V4L2_SUBDEV_FORMAT_TRY right away.

> and avoid adding backward-compatible uAPI code inside drivers.
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> # for imx-media
> > Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > ---
> > Changes since v3:
> > 
> > - Rename V4L2_SUBDEV_CLIENT_CAP_WHICH_INTERVAL to
> >   V4L2_SUBDEV_CLIENT_CAP_INTERVAL_USES_WHICH
> > - Use ULL for bit constant
> > - Address thp7312 driver
> > ---
> >  .../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              | 15 ++++++++---
> >  36 files changed, 199 insertions(+), 18 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..810b6a859dc8 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_INTERVAL_USES_WHICH``
> > +      - 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.
> >  
> >  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 d4975b180704..49eda07f1c96 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..b9ce3e41d52c 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_INTERVAL_USES_WHICH))
> > +			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_INTERVAL_USES_WHICH);
> >  
> >  		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 f0fbb4a7c150..7048c51581c6 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
> >   */
> > @@ -239,7 +241,14 @@ struct v4l2_subdev_routing {
> >   * set (which is the default), the 'stream' fields will be forced to 0 by the
> >   * kernel.
> >   */
> > -#define V4L2_SUBDEV_CLIENT_CAP_STREAMS		(1ULL << 0)
> > +#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_INTERVAL_USES_WHICH	(1ULL << 1)
> >  
> >  /**
> >   * struct v4l2_subdev_client_capability - Capabilities of the client accessing

On 08/12/2023 19:16, 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

You forgot to update this capability to the new name :-)

I'm not sure if a v5 will be needed, if not then I hope I'll remember to
update this name.

Regards,

	Hans

> 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@ideasonboard.com>
> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> # for imx-media
> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
> Changes since v3:
> 
> - Rename V4L2_SUBDEV_CLIENT_CAP_WHICH_INTERVAL to
>   V4L2_SUBDEV_CLIENT_CAP_INTERVAL_USES_WHICH
> - Use ULL for bit constant
> - Address thp7312 driver
> ---
>  .../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              | 15 ++++++++---
>  36 files changed, 199 insertions(+), 18 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..810b6a859dc8 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_INTERVAL_USES_WHICH``
> +      - 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.
>  
>  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 d4975b180704..49eda07f1c96 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..b9ce3e41d52c 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_INTERVAL_USES_WHICH))
> +			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_INTERVAL_USES_WHICH);
>  
>  		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 f0fbb4a7c150..7048c51581c6 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
>   */
> @@ -239,7 +241,14 @@ struct v4l2_subdev_routing {
>   * set (which is the default), the 'stream' fields will be forced to 0 by the
>   * kernel.
>   */
> -#define V4L2_SUBDEV_CLIENT_CAP_STREAMS		(1ULL << 0)
> +#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_INTERVAL_USES_WHICH	(1ULL << 1)
>  
>  /**
>   * struct v4l2_subdev_client_capability - Capabilities of the client accessing

On 09/12/2023 12:11, Laurent Pinchart wrote:
> On Sat, Dec 09, 2023 at 06:55:01AM +0100, Mauro Carvalho Chehab wrote:
>> Em Fri,  8 Dec 2023 20:16:43 +0200
>> Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
>>
>>> 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.
>>
>> The uAPI change looks ok to me. However, having to add an special
>> logic on every client using (get/set)_time_interval seems risky,
>> as it will require an extra check before accepting new subdev
>> drivers.
>>
>> Please move such check to drivers/media/v4l2-core/v4l2-subdev.c:
>>
>> 	if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
>> 		return -EINVAL;
> 
> But then no new driver will be able to use this API. Look at patch 8/8
> in this series, the whole point of this exercise is to support
> V4L2_SUBDEV_FORMAT_TRY in drivers. The added checks in existing drivers
> is because they don't support V4L2_SUBDEV_FORMAT_TRY. Moving forward,
> the drivers that are still maintained should be converted eventually.
> All new drivers should use the V4L2 subdev active state API and support
> V4L2_SUBDEV_FORMAT_TRY right away.

I agree with Laurent here. Note that the logic isn't 'special', it is standard
handling of the 'which' field. It was never there for g/s_frame_interval
because that was the only one that didn't have a 'which' field, but now that
it does, the drivers need to be adapted to handle this new field.

It shouldn't be hidden in some core code, that would only confuse matters.

Regards,

	Hans

> 
>> and avoid adding backward-compatible uAPI code inside drivers.
>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> # for imx-media
>>> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>> ---
>>> Changes since v3:
>>>
>>> - Rename V4L2_SUBDEV_CLIENT_CAP_WHICH_INTERVAL to
>>>   V4L2_SUBDEV_CLIENT_CAP_INTERVAL_USES_WHICH
>>> - Use ULL for bit constant
>>> - Address thp7312 driver
>>> ---
>>>  .../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              | 15 ++++++++---
>>>  36 files changed, 199 insertions(+), 18 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..810b6a859dc8 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_INTERVAL_USES_WHICH``
>>> +      - 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.
>>>  
>>>  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 d4975b180704..49eda07f1c96 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..b9ce3e41d52c 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_INTERVAL_USES_WHICH))
>>> +			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_INTERVAL_USES_WHICH);
>>>  
>>>  		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 f0fbb4a7c150..7048c51581c6 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
>>>   */
>>> @@ -239,7 +241,14 @@ struct v4l2_subdev_routing {
>>>   * set (which is the default), the 'stream' fields will be forced to 0 by the
>>>   * kernel.
>>>   */
>>> -#define V4L2_SUBDEV_CLIENT_CAP_STREAMS		(1ULL << 0)
>>> +#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_INTERVAL_USES_WHICH	(1ULL << 1)
>>>  
>>>  /**
>>>   * struct v4l2_subdev_client_capability - Capabilities of the client accessing
>

Em Mon, 11 Dec 2023 09:59:25 +0100
Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:

> On 09/12/2023 12:11, Laurent Pinchart wrote:
> > On Sat, Dec 09, 2023 at 06:55:01AM +0100, Mauro Carvalho Chehab wrote:  
> >> Em Fri,  8 Dec 2023 20:16:43 +0200
> >> Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
> >>  
> >>> 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.  
> >>
> >> The uAPI change looks ok to me. However, having to add an special
> >> logic on every client using (get/set)_time_interval seems risky,
> >> as it will require an extra check before accepting new subdev
> >> drivers.
> >>
> >> Please move such check to drivers/media/v4l2-core/v4l2-subdev.c:
> >>
> >> 	if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> >> 		return -EINVAL;  
> > 
> > But then no new driver will be able to use this API. Look at patch 8/8
> > in this series, the whole point of this exercise is to support
> > V4L2_SUBDEV_FORMAT_TRY in drivers. The added checks in existing drivers
> > is because they don't support V4L2_SUBDEV_FORMAT_TRY. Moving forward,
> > the drivers that are still maintained should be converted eventually.

Then please add a FIXME note there warning that the logic there
exists just because the driver doesn't yet support V4L2_SUBDEV_FORMAT_TRY.

With that, feel free to add: 

Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>

> > All new drivers should use the V4L2 subdev active state API and support
> > V4L2_SUBDEV_FORMAT_TRY right away.  

OK.

> I agree with Laurent here. Note that the logic isn't 'special', it is standard
> handling of the 'which' field. It was never there for g/s_frame_interval
> because that was the only one that didn't have a 'which' field, but now that
> it does, the drivers need to be adapted to handle this new field.
> 
> It shouldn't be hidden in some core code, that would only confuse matters.

If the idea is to move forward without implementing support for such
features at the existing drivers, we should at least have something
annotated at the drivers that this is something that will require
further changes in the future to support the current behavior.

I also expect that the API compliance tool to produce warnings
on drivers using the old behavior.

Thanks,
Mauro

On 11/12/2023 12:53, Mauro Carvalho Chehab wrote:
> Em Mon, 11 Dec 2023 09:59:25 +0100
> Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:
> 
>> On 09/12/2023 12:11, Laurent Pinchart wrote:
>>> On Sat, Dec 09, 2023 at 06:55:01AM +0100, Mauro Carvalho Chehab wrote:  
>>>> Em Fri,  8 Dec 2023 20:16:43 +0200
>>>> Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
>>>>  
>>>>> 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.  
>>>>
>>>> The uAPI change looks ok to me. However, having to add an special
>>>> logic on every client using (get/set)_time_interval seems risky,
>>>> as it will require an extra check before accepting new subdev
>>>> drivers.
>>>>
>>>> Please move such check to drivers/media/v4l2-core/v4l2-subdev.c:
>>>>
>>>> 	if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
>>>> 		return -EINVAL;  
>>>
>>> But then no new driver will be able to use this API. Look at patch 8/8
>>> in this series, the whole point of this exercise is to support
>>> V4L2_SUBDEV_FORMAT_TRY in drivers. The added checks in existing drivers
>>> is because they don't support V4L2_SUBDEV_FORMAT_TRY. Moving forward,
>>> the drivers that are still maintained should be converted eventually.
> 
> Then please add a FIXME note there warning that the logic there
> exists just because the driver doesn't yet support V4L2_SUBDEV_FORMAT_TRY.

I think there are two types of cases here: the first case is when the subdev
does not support TRY at all for any of the pad ops, i.e. it can't be used by
MC-centric drivers.

In that case a top-level comment to that effect might be useful (that can be
done in a separate patch).

The second case is where it does support TRY for the other pad ops, just not
for g/s_frame_intervals. I checked the updated documentation in patch 3/8, but
it is not clear whether that would be considered a bug or not. That is something
that should be clarified.

Frankly, it is not clear to me if, for an MC-centric capable subdev, support
for FORMAT_TRY is required to be present for all pad ops, or not. And if not,
for which pad ops is it optional and what does that mean for applications.

> 
> With that, feel free to add: 
> 
> Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>
> 
>>> All new drivers should use the V4L2 subdev active state API and support
>>> V4L2_SUBDEV_FORMAT_TRY right away.  
> 
> OK.
> 
>> I agree with Laurent here. Note that the logic isn't 'special', it is standard
>> handling of the 'which' field. It was never there for g/s_frame_interval
>> because that was the only one that didn't have a 'which' field, but now that
>> it does, the drivers need to be adapted to handle this new field.
>>
>> It shouldn't be hidden in some core code, that would only confuse matters.
> 
> If the idea is to move forward without implementing support for such
> features at the existing drivers, we should at least have something
> annotated at the drivers that this is something that will require
> further changes in the future to support the current behavior.
> 
> I also expect that the API compliance tool to produce warnings
> on drivers using the old behavior.

Yeah, I think I should add this. If a /dev/v4l-subdevX exists, then that means
it is very likely created through an MC-centric driver and TRY support should
be there.

Regards,

	Hans

Em Mon, 11 Dec 2023 13:25:23 +0100
Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:

> On 11/12/2023 12:53, Mauro Carvalho Chehab wrote:
> > Em Mon, 11 Dec 2023 09:59:25 +0100
> > Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:
> >   
> >> On 09/12/2023 12:11, Laurent Pinchart wrote:  
> >>> On Sat, Dec 09, 2023 at 06:55:01AM +0100, Mauro Carvalho Chehab wrote:    
> >>>> Em Fri,  8 Dec 2023 20:16:43 +0200
> >>>> Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
> >>>>    
> >>>>> 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.    
> >>>>
> >>>> The uAPI change looks ok to me. However, having to add an special
> >>>> logic on every client using (get/set)_time_interval seems risky,
> >>>> as it will require an extra check before accepting new subdev
> >>>> drivers.
> >>>>
> >>>> Please move such check to drivers/media/v4l2-core/v4l2-subdev.c:
> >>>>
> >>>> 	if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> >>>> 		return -EINVAL;    
> >>>
> >>> But then no new driver will be able to use this API. Look at patch 8/8
> >>> in this series, the whole point of this exercise is to support
> >>> V4L2_SUBDEV_FORMAT_TRY in drivers. The added checks in existing drivers
> >>> is because they don't support V4L2_SUBDEV_FORMAT_TRY. Moving forward,
> >>> the drivers that are still maintained should be converted eventually.  
> > 
> > Then please add a FIXME note there warning that the logic there
> > exists just because the driver doesn't yet support V4L2_SUBDEV_FORMAT_TRY.  
> 
> I think there are two types of cases here: the first case is when the subdev
> does not support TRY at all for any of the pad ops, i.e. it can't be used by
> MC-centric drivers.
> 
> In that case a top-level comment to that effect might be useful (that can be
> done in a separate patch).

Agreed.

> The second case is where it does support TRY for the other pad ops, just not
> for g/s_frame_intervals. I checked the updated documentation in patch 3/8, but
> it is not clear whether that would be considered a bug or not. That is something
> that should be clarified.
>
> Frankly, it is not clear to me if, for an MC-centric capable subdev, support
> for FORMAT_TRY is required to be present for all pad ops, or not. And if not,
> for which pad ops is it optional and what does that mean for applications.

Agreed.

Ideally, the best would be to add support for this feature for all subdevs
at the same patch series, but I understand that this will require tests 
from the developers proposing the uAPI change and/or tested-by.
So, I can accept merging this series and enforcing for new drivers without
requiring a conversion of all existing ones at the same patch series, but
someone (the ones behind this change) should work to have existing drivers
with g/s_frame_interval subdev uAPI also supporting TRY_.


> > 
> > With that, feel free to add: 
> > 
> > Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>
> >   
> >>> All new drivers should use the V4L2 subdev active state API and support
> >>> V4L2_SUBDEV_FORMAT_TRY right away.    
> > 
> > OK.
> >   
> >> I agree with Laurent here. Note that the logic isn't 'special', it is standard
> >> handling of the 'which' field. It was never there for g/s_frame_interval
> >> because that was the only one that didn't have a 'which' field, but now that
> >> it does, the drivers need to be adapted to handle this new field.
> >>
> >> It shouldn't be hidden in some core code, that would only confuse matters.  
> > 
> > If the idea is to move forward without implementing support for such
> > features at the existing drivers, we should at least have something
> > annotated at the drivers that this is something that will require
> > further changes in the future to support the current behavior.
> > 
> > I also expect that the API compliance tool to produce warnings
> > on drivers using the old behavior.  
> 
> Yeah, I think I should add this. If a /dev/v4l-subdevX exists, then that means
> it is very likely created through an MC-centric driver and TRY support should
> be there.

Yes.

Thanks,
Mauro

Hello,

On Mon, Dec 11, 2023 at 02:21:19PM +0100, Mauro Carvalho Chehab wrote:
> Em Mon, 11 Dec 2023 13:25:23 +0100 Hans Verkuil escreveu:
> > On 11/12/2023 12:53, Mauro Carvalho Chehab wrote:
> > > Em Mon, 11 Dec 2023 09:59:25 +0100 Hans Verkuil escreveu:
> > >> On 09/12/2023 12:11, Laurent Pinchart wrote:  
> > >>> On Sat, Dec 09, 2023 at 06:55:01AM +0100, Mauro Carvalho Chehab wrote:    
> > >>>> Em Fri,  8 Dec 2023 20:16:43 +0200
> > >>>> Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
> > >>>>    
> > >>>>> 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.    
> > >>>>
> > >>>> The uAPI change looks ok to me. However, having to add an special
> > >>>> logic on every client using (get/set)_time_interval seems risky,
> > >>>> as it will require an extra check before accepting new subdev
> > >>>> drivers.
> > >>>>
> > >>>> Please move such check to drivers/media/v4l2-core/v4l2-subdev.c:
> > >>>>
> > >>>> 	if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> > >>>> 		return -EINVAL;    
> > >>>
> > >>> But then no new driver will be able to use this API. Look at patch 8/8
> > >>> in this series, the whole point of this exercise is to support
> > >>> V4L2_SUBDEV_FORMAT_TRY in drivers. The added checks in existing drivers
> > >>> is because they don't support V4L2_SUBDEV_FORMAT_TRY. Moving forward,
> > >>> the drivers that are still maintained should be converted eventually.  
> > > 
> > > Then please add a FIXME note there warning that the logic there
> > > exists just because the driver doesn't yet support V4L2_SUBDEV_FORMAT_TRY.  
> > 
> > I think there are two types of cases here: the first case is when the subdev
> > does not support TRY at all for any of the pad ops, i.e. it can't be used by
> > MC-centric drivers.
> > 
> > In that case a top-level comment to that effect might be useful (that can be
> > done in a separate patch).
> 
> Agreed.

Recording the need to convert those drivers in a TODO comment seems fine
to me. Whether or not someone will address those comments is another
question... I'd like that to be the case, but most (if not all) of those
drivers are for old devices, so the incentive will be very low, not even
mentioning the lack of hardware availability for testing.

We can add TODO comments in a separate patch on top. Or would it be
better to add a single TODO file in drivers/media/i2c/, like done in
staging ? Or a todo.rst in Documentation, like done in
Documentation/gpu/todo.rst ?

Update: I checked the drivers touched by this patch, and they all
implement TRY support for the pad ops, so there's no TODO to record :-)

> > The second case is where it does support TRY for the other pad ops, just not
> > for g/s_frame_intervals. I checked the updated documentation in patch 3/8, but
> > it is not clear whether that would be considered a bug or not. That is something
> > that should be clarified.
> >
> > Frankly, it is not clear to me if, for an MC-centric capable subdev, support
> > for FORMAT_TRY is required to be present for all pad ops, or not. And if not,
> > for which pad ops is it optional and what does that mean for applications.
> 
> Agreed.
> 
> Ideally, the best would be to add support for this feature for all subdevs
> at the same patch series, but I understand that this will require tests 
> from the developers proposing the uAPI change and/or tested-by.
>
> So, I can accept merging this series and enforcing for new drivers without
> requiring a conversion of all existing ones at the same patch series, but
> someone (the ones behind this change) should work to have existing drivers
> with g/s_frame_interval subdev uAPI also supporting TRY_.

Many of the drivers are unmaintained :-( Nobody has the hardware
anymore, or the time to test it. Some drivers can be converted, but not
all of them unfortunately.

This series addresses the thp7312 already. I can address the mt9m114
too. I think we can try to also address the other three drivers for
sensors supported by libcamera, namely the ov5640, ov5693 and ov8865.
The rest will need other volunteers.

> > > With that, feel free to add: 
> > > 
> > > Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>
> > >   
> > >>> All new drivers should use the V4L2 subdev active state API and support
> > >>> V4L2_SUBDEV_FORMAT_TRY right away.    
> > > 
> > > OK.
> > >   
> > >> I agree with Laurent here. Note that the logic isn't 'special', it is standard
> > >> handling of the 'which' field. It was never there for g/s_frame_interval
> > >> because that was the only one that didn't have a 'which' field, but now that
> > >> it does, the drivers need to be adapted to handle this new field.
> > >>
> > >> It shouldn't be hidden in some core code, that would only confuse matters.  
> > > 
> > > If the idea is to move forward without implementing support for such
> > > features at the existing drivers, we should at least have something
> > > annotated at the drivers that this is something that will require
> > > further changes in the future to support the current behavior.
> > > 
> > > I also expect that the API compliance tool to produce warnings
> > > on drivers using the old behavior.  
> > 
> > Yeah, I think I should add this. If a /dev/v4l-subdevX exists, then that means
> > it is very likely created through an MC-centric driver and TRY support should
> > be there.
> 
> Yes.

Hans, would you be able to handle v4l2-compliance ?

On 11/12/2023 16:07, Laurent Pinchart wrote:
> Hello,
> 
> On Mon, Dec 11, 2023 at 02:21:19PM +0100, Mauro Carvalho Chehab wrote:
>> Em Mon, 11 Dec 2023 13:25:23 +0100 Hans Verkuil escreveu:
>>> On 11/12/2023 12:53, Mauro Carvalho Chehab wrote:
>>>> Em Mon, 11 Dec 2023 09:59:25 +0100 Hans Verkuil escreveu:
>>>>> On 09/12/2023 12:11, Laurent Pinchart wrote:  
>>>>>> On Sat, Dec 09, 2023 at 06:55:01AM +0100, Mauro Carvalho Chehab wrote:    
>>>>>>> Em Fri,  8 Dec 2023 20:16:43 +0200
>>>>>>> Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
>>>>>>>    
>>>>>>>> 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.    
>>>>>>>
>>>>>>> The uAPI change looks ok to me. However, having to add an special
>>>>>>> logic on every client using (get/set)_time_interval seems risky,
>>>>>>> as it will require an extra check before accepting new subdev
>>>>>>> drivers.
>>>>>>>
>>>>>>> Please move such check to drivers/media/v4l2-core/v4l2-subdev.c:
>>>>>>>
>>>>>>> 	if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
>>>>>>> 		return -EINVAL;    
>>>>>>
>>>>>> But then no new driver will be able to use this API. Look at patch 8/8
>>>>>> in this series, the whole point of this exercise is to support
>>>>>> V4L2_SUBDEV_FORMAT_TRY in drivers. The added checks in existing drivers
>>>>>> is because they don't support V4L2_SUBDEV_FORMAT_TRY. Moving forward,
>>>>>> the drivers that are still maintained should be converted eventually.  
>>>>
>>>> Then please add a FIXME note there warning that the logic there
>>>> exists just because the driver doesn't yet support V4L2_SUBDEV_FORMAT_TRY.  
>>>
>>> I think there are two types of cases here: the first case is when the subdev
>>> does not support TRY at all for any of the pad ops, i.e. it can't be used by
>>> MC-centric drivers.
>>>
>>> In that case a top-level comment to that effect might be useful (that can be
>>> done in a separate patch).
>>
>> Agreed.
> 
> Recording the need to convert those drivers in a TODO comment seems fine
> to me. Whether or not someone will address those comments is another
> question... I'd like that to be the case, but most (if not all) of those
> drivers are for old devices, so the incentive will be very low, not even
> mentioning the lack of hardware availability for testing.
> 
> We can add TODO comments in a separate patch on top. Or would it be
> better to add a single TODO file in drivers/media/i2c/, like done in
> staging ? Or a todo.rst in Documentation, like done in
> Documentation/gpu/todo.rst ?

Definitely add it to the source code itself since nobody reads those TODO
or todo.rst files :-)

> 
> Update: I checked the drivers touched by this patch, and they all
> implement TRY support for the pad ops, so there's no TODO to record :-)

OK, good.

I think it would be useful to record in a comment which drivers need work
in order to be MC-centric capable. But it is a nice-to-have patch and can be
done separately from this series.

For the g/s_frame_interval drivers a FIXME is needed for those that do
not support TRY (all except thp7312).

> 
>>> The second case is where it does support TRY for the other pad ops, just not
>>> for g/s_frame_intervals. I checked the updated documentation in patch 3/8, but
>>> it is not clear whether that would be considered a bug or not. That is something
>>> that should be clarified.
>>>
>>> Frankly, it is not clear to me if, for an MC-centric capable subdev, support
>>> for FORMAT_TRY is required to be present for all pad ops, or not. And if not,
>>> for which pad ops is it optional and what does that mean for applications.
>>
>> Agreed.
>>
>> Ideally, the best would be to add support for this feature for all subdevs
>> at the same patch series, but I understand that this will require tests 
>> from the developers proposing the uAPI change and/or tested-by.
>>
>> So, I can accept merging this series and enforcing for new drivers without
>> requiring a conversion of all existing ones at the same patch series, but
>> someone (the ones behind this change) should work to have existing drivers
>> with g/s_frame_interval subdev uAPI also supporting TRY_.
> 
> Many of the drivers are unmaintained :-( Nobody has the hardware
> anymore, or the time to test it. Some drivers can be converted, but not
> all of them unfortunately.
> 
> This series addresses the thp7312 already. I can address the mt9m114
> too. I think we can try to also address the other three drivers for
> sensors supported by libcamera, namely the ov5640, ov5693 and ov8865.
> The rest will need other volunteers.

So the main question is: what is userspace (esp. libcamera) supposed to
do if a subdev supports TRY for all ops except g/s_frame_interval? Does that
mean it is not suitable for MC-centric use anymore? Or is userspace required
to work around that? At the very least that should be documented.

> 
>>>> With that, feel free to add: 
>>>>
>>>> Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>
>>>>   
>>>>>> All new drivers should use the V4L2 subdev active state API and support
>>>>>> V4L2_SUBDEV_FORMAT_TRY right away.    
>>>>
>>>> OK.
>>>>   
>>>>> I agree with Laurent here. Note that the logic isn't 'special', it is standard
>>>>> handling of the 'which' field. It was never there for g/s_frame_interval
>>>>> because that was the only one that didn't have a 'which' field, but now that
>>>>> it does, the drivers need to be adapted to handle this new field.
>>>>>
>>>>> It shouldn't be hidden in some core code, that would only confuse matters.  
>>>>
>>>> If the idea is to move forward without implementing support for such
>>>> features at the existing drivers, we should at least have something
>>>> annotated at the drivers that this is something that will require
>>>> further changes in the future to support the current behavior.
>>>>
>>>> I also expect that the API compliance tool to produce warnings
>>>> on drivers using the old behavior.  
>>>
>>> Yeah, I think I should add this. If a /dev/v4l-subdevX exists, then that means
>>> it is very likely created through an MC-centric driver and TRY support should
>>> be there.
>>
>> Yes.
> 
> Hans, would you be able to handle v4l2-compliance ?
> 

Sure. Although that likely will be in January.

Regards,

	Hans

On Mon, Dec 11, 2023 at 04:27:27PM +0100, Hans Verkuil wrote:
> On 11/12/2023 16:07, Laurent Pinchart wrote:
> > On Mon, Dec 11, 2023 at 02:21:19PM +0100, Mauro Carvalho Chehab wrote:
> >> Em Mon, 11 Dec 2023 13:25:23 +0100 Hans Verkuil escreveu:
> >>> On 11/12/2023 12:53, Mauro Carvalho Chehab wrote:
> >>>> Em Mon, 11 Dec 2023 09:59:25 +0100 Hans Verkuil escreveu:
> >>>>> On 09/12/2023 12:11, Laurent Pinchart wrote:  
> >>>>>> On Sat, Dec 09, 2023 at 06:55:01AM +0100, Mauro Carvalho Chehab wrote:    
> >>>>>>> Em Fri,  8 Dec 2023 20:16:43 +0200
> >>>>>>> Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
> >>>>>>>    
> >>>>>>>> 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.    
> >>>>>>>
> >>>>>>> The uAPI change looks ok to me. However, having to add an special
> >>>>>>> logic on every client using (get/set)_time_interval seems risky,
> >>>>>>> as it will require an extra check before accepting new subdev
> >>>>>>> drivers.
> >>>>>>>
> >>>>>>> Please move such check to drivers/media/v4l2-core/v4l2-subdev.c:
> >>>>>>>
> >>>>>>> 	if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> >>>>>>> 		return -EINVAL;    
> >>>>>>
> >>>>>> But then no new driver will be able to use this API. Look at patch 8/8
> >>>>>> in this series, the whole point of this exercise is to support
> >>>>>> V4L2_SUBDEV_FORMAT_TRY in drivers. The added checks in existing drivers
> >>>>>> is because they don't support V4L2_SUBDEV_FORMAT_TRY. Moving forward,
> >>>>>> the drivers that are still maintained should be converted eventually.  
> >>>>
> >>>> Then please add a FIXME note there warning that the logic there
> >>>> exists just because the driver doesn't yet support V4L2_SUBDEV_FORMAT_TRY.  
> >>>
> >>> I think there are two types of cases here: the first case is when the subdev
> >>> does not support TRY at all for any of the pad ops, i.e. it can't be used by
> >>> MC-centric drivers.
> >>>
> >>> In that case a top-level comment to that effect might be useful (that can be
> >>> done in a separate patch).
> >>
> >> Agreed.
> > 
> > Recording the need to convert those drivers in a TODO comment seems fine
> > to me. Whether or not someone will address those comments is another
> > question... I'd like that to be the case, but most (if not all) of those
> > drivers are for old devices, so the incentive will be very low, not even
> > mentioning the lack of hardware availability for testing.
> > 
> > We can add TODO comments in a separate patch on top. Or would it be
> > better to add a single TODO file in drivers/media/i2c/, like done in
> > staging ? Or a todo.rst in Documentation, like done in
> > Documentation/gpu/todo.rst ?
> 
> Definitely add it to the source code itself since nobody reads those TODO
> or todo.rst files :-)
> 
> > Update: I checked the drivers touched by this patch, and they all
> > implement TRY support for the pad ops, so there's no TODO to record :-)
> 
> OK, good.
> 
> I think it would be useful to record in a comment which drivers need work
> in order to be MC-centric capable. But it is a nice-to-have patch and can be
> done separately from this series.
> 
> For the g/s_frame_interval drivers a FIXME is needed for those that do
> not support TRY (all except thp7312).

OK, I can handle that easily. I will add

	/* FIXME: Implement support for V4L2_SUBDEV_FORMAT_TRY. */

above all the checks.

> >>> The second case is where it does support TRY for the other pad ops, just not
> >>> for g/s_frame_intervals. I checked the updated documentation in patch 3/8, but
> >>> it is not clear whether that would be considered a bug or not. That is something
> >>> that should be clarified.
> >>>
> >>> Frankly, it is not clear to me if, for an MC-centric capable subdev, support
> >>> for FORMAT_TRY is required to be present for all pad ops, or not. And if not,
> >>> for which pad ops is it optional and what does that mean for applications.
> >>
> >> Agreed.
> >>
> >> Ideally, the best would be to add support for this feature for all subdevs
> >> at the same patch series, but I understand that this will require tests 
> >> from the developers proposing the uAPI change and/or tested-by.
> >>
> >> So, I can accept merging this series and enforcing for new drivers without
> >> requiring a conversion of all existing ones at the same patch series, but
> >> someone (the ones behind this change) should work to have existing drivers
> >> with g/s_frame_interval subdev uAPI also supporting TRY_.
> > 
> > Many of the drivers are unmaintained :-( Nobody has the hardware
> > anymore, or the time to test it. Some drivers can be converted, but not
> > all of them unfortunately.
> > 
> > This series addresses the thp7312 already. I can address the mt9m114
> > too. I think we can try to also address the other three drivers for
> > sensors supported by libcamera, namely the ov5640, ov5693 and ov8865.
> > The rest will need other volunteers.
> 
> So the main question is: what is userspace (esp. libcamera) supposed to
> do if a subdev supports TRY for all ops except g/s_frame_interval? Does that
> mean it is not suitable for MC-centric use anymore? Or is userspace required
> to work around that? At the very least that should be documented.

Existing userspace isn't supposed to do anything, it will continue
working as it does today.

libcamera doesn't use those ioctls yet, so there will be no change in
behaviour there. Once we start using them, we will likely require
support for TRY, and consider the driver not supported if it doesn't
implement that feature. I would encourage other future userspace users
of the ioctls to do the same, but I don't think we need to enforce any
specific behaviour there.

> >>>> With that, feel free to add: 
> >>>>
> >>>> Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>
> >>>>   
> >>>>>> All new drivers should use the V4L2 subdev active state API and support
> >>>>>> V4L2_SUBDEV_FORMAT_TRY right away.    
> >>>>
> >>>> OK.
> >>>>   
> >>>>> I agree with Laurent here. Note that the logic isn't 'special', it is standard
> >>>>> handling of the 'which' field. It was never there for g/s_frame_interval
> >>>>> because that was the only one that didn't have a 'which' field, but now that
> >>>>> it does, the drivers need to be adapted to handle this new field.
> >>>>>
> >>>>> It shouldn't be hidden in some core code, that would only confuse matters.  
> >>>>
> >>>> If the idea is to move forward without implementing support for such
> >>>> features at the existing drivers, we should at least have something
> >>>> annotated at the drivers that this is something that will require
> >>>> further changes in the future to support the current behavior.
> >>>>
> >>>> I also expect that the API compliance tool to produce warnings
> >>>> on drivers using the old behavior.  
> >>>
> >>> Yeah, I think I should add this. If a /dev/v4l-subdevX exists, then that means
> >>> it is very likely created through an MC-centric driver and TRY support should
> >>> be there.
> >>
> >> Yes.
> > 
> > Hans, would you be able to handle v4l2-compliance ?
> 
> Sure. Although that likely will be in January.

Thank you :-)

On Mon, Dec 11, 2023 at 05:42:37PM +0200, Laurent Pinchart wrote:
> On Mon, Dec 11, 2023 at 04:27:27PM +0100, Hans Verkuil wrote:
> > On 11/12/2023 16:07, Laurent Pinchart wrote:
> > > On Mon, Dec 11, 2023 at 02:21:19PM +0100, Mauro Carvalho Chehab wrote:
> > >> Em Mon, 11 Dec 2023 13:25:23 +0100 Hans Verkuil escreveu:
> > >>> On 11/12/2023 12:53, Mauro Carvalho Chehab wrote:
> > >>>> Em Mon, 11 Dec 2023 09:59:25 +0100 Hans Verkuil escreveu:
> > >>>>> On 09/12/2023 12:11, Laurent Pinchart wrote:  
> > >>>>>> On Sat, Dec 09, 2023 at 06:55:01AM +0100, Mauro Carvalho Chehab wrote:    
> > >>>>>>> Em Fri,  8 Dec 2023 20:16:43 +0200
> > >>>>>>> Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
> > >>>>>>>    
> > >>>>>>>> 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.    
> > >>>>>>>
> > >>>>>>> The uAPI change looks ok to me. However, having to add an special
> > >>>>>>> logic on every client using (get/set)_time_interval seems risky,
> > >>>>>>> as it will require an extra check before accepting new subdev
> > >>>>>>> drivers.
> > >>>>>>>
> > >>>>>>> Please move such check to drivers/media/v4l2-core/v4l2-subdev.c:
> > >>>>>>>
> > >>>>>>> 	if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> > >>>>>>> 		return -EINVAL;    
> > >>>>>>
> > >>>>>> But then no new driver will be able to use this API. Look at patch 8/8
> > >>>>>> in this series, the whole point of this exercise is to support
> > >>>>>> V4L2_SUBDEV_FORMAT_TRY in drivers. The added checks in existing drivers
> > >>>>>> is because they don't support V4L2_SUBDEV_FORMAT_TRY. Moving forward,
> > >>>>>> the drivers that are still maintained should be converted eventually.  
> > >>>>
> > >>>> Then please add a FIXME note there warning that the logic there
> > >>>> exists just because the driver doesn't yet support V4L2_SUBDEV_FORMAT_TRY.  
> > >>>
> > >>> I think there are two types of cases here: the first case is when the subdev
> > >>> does not support TRY at all for any of the pad ops, i.e. it can't be used by
> > >>> MC-centric drivers.
> > >>>
> > >>> In that case a top-level comment to that effect might be useful (that can be
> > >>> done in a separate patch).
> > >>
> > >> Agreed.
> > > 
> > > Recording the need to convert those drivers in a TODO comment seems fine
> > > to me. Whether or not someone will address those comments is another
> > > question... I'd like that to be the case, but most (if not all) of those
> > > drivers are for old devices, so the incentive will be very low, not even
> > > mentioning the lack of hardware availability for testing.
> > > 
> > > We can add TODO comments in a separate patch on top. Or would it be
> > > better to add a single TODO file in drivers/media/i2c/, like done in
> > > staging ? Or a todo.rst in Documentation, like done in
> > > Documentation/gpu/todo.rst ?
> > 
> > Definitely add it to the source code itself since nobody reads those TODO
> > or todo.rst files :-)
> > 
> > > Update: I checked the drivers touched by this patch, and they all
> > > implement TRY support for the pad ops, so there's no TODO to record :-)
> > 
> > OK, good.
> > 
> > I think it would be useful to record in a comment which drivers need work
> > in order to be MC-centric capable. But it is a nice-to-have patch and can be
> > done separately from this series.
> > 
> > For the g/s_frame_interval drivers a FIXME is needed for those that do
> > not support TRY (all except thp7312).
> 
> OK, I can handle that easily. I will add
> 
> 	/* FIXME: Implement support for V4L2_SUBDEV_FORMAT_TRY. */

Actually I'll expand that to

 	/*
	 * FIXME: Implement support for V4L2_SUBDEV_FORMAT_TRY, using the V4L2
	 * subdev active state API.
	 */

> above all the checks.
> 
> > >>> The second case is where it does support TRY for the other pad ops, just not
> > >>> for g/s_frame_intervals. I checked the updated documentation in patch 3/8, but
> > >>> it is not clear whether that would be considered a bug or not. That is something
> > >>> that should be clarified.
> > >>>
> > >>> Frankly, it is not clear to me if, for an MC-centric capable subdev, support
> > >>> for FORMAT_TRY is required to be present for all pad ops, or not. And if not,
> > >>> for which pad ops is it optional and what does that mean for applications.
> > >>
> > >> Agreed.
> > >>
> > >> Ideally, the best would be to add support for this feature for all subdevs
> > >> at the same patch series, but I understand that this will require tests 
> > >> from the developers proposing the uAPI change and/or tested-by.
> > >>
> > >> So, I can accept merging this series and enforcing for new drivers without
> > >> requiring a conversion of all existing ones at the same patch series, but
> > >> someone (the ones behind this change) should work to have existing drivers
> > >> with g/s_frame_interval subdev uAPI also supporting TRY_.
> > > 
> > > Many of the drivers are unmaintained :-( Nobody has the hardware
> > > anymore, or the time to test it. Some drivers can be converted, but not
> > > all of them unfortunately.
> > > 
> > > This series addresses the thp7312 already. I can address the mt9m114
> > > too. I think we can try to also address the other three drivers for
> > > sensors supported by libcamera, namely the ov5640, ov5693 and ov8865.
> > > The rest will need other volunteers.
> > 
> > So the main question is: what is userspace (esp. libcamera) supposed to
> > do if a subdev supports TRY for all ops except g/s_frame_interval? Does that
> > mean it is not suitable for MC-centric use anymore? Or is userspace required
> > to work around that? At the very least that should be documented.
> 
> Existing userspace isn't supposed to do anything, it will continue
> working as it does today.
> 
> libcamera doesn't use those ioctls yet, so there will be no change in
> behaviour there. Once we start using them, we will likely require
> support for TRY, and consider the driver not supported if it doesn't
> implement that feature. I would encourage other future userspace users
> of the ioctls to do the same, but I don't think we need to enforce any
> specific behaviour there.
> 
> > >>>> With that, feel free to add: 
> > >>>>
> > >>>> Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>
> > >>>>   
> > >>>>>> All new drivers should use the V4L2 subdev active state API and support
> > >>>>>> V4L2_SUBDEV_FORMAT_TRY right away.    
> > >>>>
> > >>>> OK.
> > >>>>   
> > >>>>> I agree with Laurent here. Note that the logic isn't 'special', it is standard
> > >>>>> handling of the 'which' field. It was never there for g/s_frame_interval
> > >>>>> because that was the only one that didn't have a 'which' field, but now that
> > >>>>> it does, the drivers need to be adapted to handle this new field.
> > >>>>>
> > >>>>> It shouldn't be hidden in some core code, that would only confuse matters.  
> > >>>>
> > >>>> If the idea is to move forward without implementing support for such
> > >>>> features at the existing drivers, we should at least have something
> > >>>> annotated at the drivers that this is something that will require
> > >>>> further changes in the future to support the current behavior.
> > >>>>
> > >>>> I also expect that the API compliance tool to produce warnings
> > >>>> on drivers using the old behavior.  
> > >>>
> > >>> Yeah, I think I should add this. If a /dev/v4l-subdevX exists, then that means
> > >>> it is very likely created through an MC-centric driver and TRY support should
> > >>> be there.
> > >>
> > >> Yes.
> > > 
> > > Hans, would you be able to handle v4l2-compliance ?
> > 
> > Sure. Although that likely will be in January.
> 
> Thank you :-)

Hi Laurent,

On Fri,  8 Dec 2023 20:16:43 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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@ideasonboard.com>
> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> # for imx-media
> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

...

>  drivers/staging/media/tegra-video/csi.c       |  3 +++

For tegra-video:
Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>