| Message ID | 20231127111359.30315-1-laurent.pinchart@ideasonboard.com |
|---|---|
| Headers | show |
| Series | media: v4l2-subdev: Improve frame interval handling | expand |
On 27/11/2023 12:13, Laurent Pinchart wrote: > Hello, > > This patch series improves frame interval handling in the V4L2 subdev > in-kernel and userspace APIs. > > Frame interval are exposed to userspace on pads and streams, but the > frame interval handling is currently implemented through a v4l2_subdev > video operation, without involving the subdev state. This makes frame > intervals a second class citizen compared to formats and selection > rectangles. > > Patch 1/4 starts by addressing the first issue, namely the frame > interval operations being video ops. This requires touching all the > drivers using frame intervals. > > Patch 2/4 then adds a 'which' field to the subdev frame interval > userspace API, allowing frame intervals to be tried the same way formats > and selection rectangles can. Again, the same drivers need to be touched > to preserve their current behaviour. > > Patch 3/4 adds support for storing the frame interval in the subdev > state, alongside the formats and selection rectangles, with similar > accessors and helper functions. > > Finally, patch 4/4 demonstrates how this is used in drivers, with the > thp7312 driver serving as an example. > > The series is based on Sakari's latest master branch ([1]). > > Given the large number of drivers that this series touches, I would like > to get it merged in v6.8 without too much delay to avoid rebasing. > > [1] https://git.linuxtv.org/sailus/media_tree.git/log/ > > Laurent Pinchart (4): > media: v4l2-subdev: Turn .[gs]_frame_interval into pad operations > media: v4l2-subdev: Add which field to struct > v4l2_subdev_frame_interval > media: v4l2-subdev: Store frame interval in subdev state > media: i2c: thp7312: Store frame interval in subdev state Wouldn't it be possible to first add the get/set_frame_interval() op to v4l2-subdev.h (so keep the old one), then add the which field, and only after that convert the subdev drivers. At the end there is a final patch removing the old ops. Main reason is that the core changes are easier to review, and it is easier to deal with cases where a subdev patch no longer applies, you can merge the remainder and fix that subdev in a follow-up patch. Only when all subdevs are converted is the final patch applied. I might well have missed something that prevents doing this, but if possible I think this would be a better approach. Regards, Hans > > .../media/v4l/vidioc-subdev-g-client-cap.rst | 5 + > .../v4l/vidioc-subdev-g-frame-interval.rst | 17 +- > drivers/media/i2c/adv7180.c | 10 +- > drivers/media/i2c/et8ek8/et8ek8_driver.c | 12 +- > drivers/media/i2c/imx214.c | 12 +- > drivers/media/i2c/imx274.c | 54 +++--- > drivers/media/i2c/max9286.c | 20 ++- > drivers/media/i2c/mt9m111.c | 20 ++- > drivers/media/i2c/mt9m114.c | 20 ++- > drivers/media/i2c/mt9v011.c | 24 +-- > drivers/media/i2c/mt9v111.c | 22 ++- > drivers/media/i2c/ov2680.c | 10 +- > drivers/media/i2c/ov5640.c | 22 ++- > drivers/media/i2c/ov5648.c | 62 +++---- > drivers/media/i2c/ov5693.c | 10 +- > drivers/media/i2c/ov6650.c | 22 ++- > drivers/media/i2c/ov7251.c | 12 +- > drivers/media/i2c/ov7670.c | 22 +-- > drivers/media/i2c/ov772x.c | 20 ++- > drivers/media/i2c/ov7740.c | 40 ++--- > drivers/media/i2c/ov8865.c | 54 +++--- > drivers/media/i2c/ov9650.c | 20 ++- > drivers/media/i2c/s5c73m3/s5c73m3-core.c | 20 ++- > drivers/media/i2c/s5k5baf.c | 26 ++- > drivers/media/i2c/thp7312.c | 160 ++++++++++-------- > drivers/media/i2c/tvp514x.c | 33 ++-- > drivers/media/usb/em28xx/em28xx-video.c | 6 +- > drivers/media/v4l2-core/v4l2-common.c | 8 +- > drivers/media/v4l2-core/v4l2-subdev.c | 128 ++++++++++---- > .../media/atomisp/i2c/atomisp-gc0310.c | 10 +- > .../media/atomisp/i2c/atomisp-gc2235.c | 10 +- > .../media/atomisp/i2c/atomisp-mt9m114.c | 10 +- > .../media/atomisp/i2c/atomisp-ov2722.c | 10 +- > .../staging/media/atomisp/pci/atomisp_cmd.c | 4 +- > .../staging/media/atomisp/pci/atomisp_ioctl.c | 4 +- > drivers/staging/media/imx/imx-ic-prp.c | 20 ++- > drivers/staging/media/imx/imx-ic-prpencvf.c | 20 ++- > drivers/staging/media/imx/imx-media-capture.c | 6 +- > drivers/staging/media/imx/imx-media-csi.c | 20 ++- > drivers/staging/media/imx/imx-media-vdic.c | 20 ++- > drivers/staging/media/tegra-video/csi.c | 12 +- > include/media/v4l2-common.h | 4 +- > include/media/v4l2-subdev.h | 65 +++++-- > include/uapi/linux/v4l2-subdev.h | 13 +- > 44 files changed, 706 insertions(+), 413 deletions(-) > > > base-commit: 543efaddeac0c7769c39d7aaa886e8b001acac76
Hi Hans, On Mon, Nov 27, 2023 at 02:28:09PM +0100, Hans Verkuil wrote: > On 27/11/2023 12:13, Laurent Pinchart wrote: > > Hello, > > > > This patch series improves frame interval handling in the V4L2 subdev > > in-kernel and userspace APIs. > > > > Frame interval are exposed to userspace on pads and streams, but the > > frame interval handling is currently implemented through a v4l2_subdev > > video operation, without involving the subdev state. This makes frame > > intervals a second class citizen compared to formats and selection > > rectangles. > > > > Patch 1/4 starts by addressing the first issue, namely the frame > > interval operations being video ops. This requires touching all the > > drivers using frame intervals. > > > > Patch 2/4 then adds a 'which' field to the subdev frame interval > > userspace API, allowing frame intervals to be tried the same way formats > > and selection rectangles can. Again, the same drivers need to be touched > > to preserve their current behaviour. > > > > Patch 3/4 adds support for storing the frame interval in the subdev > > state, alongside the formats and selection rectangles, with similar > > accessors and helper functions. > > > > Finally, patch 4/4 demonstrates how this is used in drivers, with the > > thp7312 driver serving as an example. > > > > The series is based on Sakari's latest master branch ([1]). > > > > Given the large number of drivers that this series touches, I would like > > to get it merged in v6.8 without too much delay to avoid rebasing. > > > > [1] https://git.linuxtv.org/sailus/media_tree.git/log/ > > > > Laurent Pinchart (4): > > media: v4l2-subdev: Turn .[gs]_frame_interval into pad operations > > media: v4l2-subdev: Add which field to struct > > v4l2_subdev_frame_interval > > media: v4l2-subdev: Store frame interval in subdev state > > media: i2c: thp7312: Store frame interval in subdev state > > Wouldn't it be possible to first add the get/set_frame_interval() op > to v4l2-subdev.h (so keep the old one), then add the which field, > and only after that convert the subdev drivers. > > At the end there is a final patch removing the old ops. > > Main reason is that the core changes are easier to review, and it is To review the core changes you can just skip the driver part in patches 1/4 and 2/4. I think turning the old operation into a new operation better shows the impact on the subsytem, compared to adding a new one and dropping the old one, so it's easier to review in the sense that a diff is easier to review than a copy+modify followed by a remove. I grant you that the patches that change the API also come with lots of driver changes, so that part makes it a bit more annoying to review. I would rather not refactor the series unless it really helps, as it will quite a bit of work to refactor the patches, for the exact same end result. Splitting the driver changes in one patch per driver would also improve my kernel development stats, but that would be gaming the system :-) > easier to deal with cases where a subdev patch no longer applies, you > can merge the remainder and fix that subdev in a follow-up patch. > > Only when all subdevs are converted is the final patch applied. If I had to carry the series over multiple kernel releases, I would agree with that. I however hope to get it in v6.8 :-) I'm fine handling the pain of rebase operations until then. If, for some reason, this change turns out to be controversial and needs to be carried forward over a longer period of time, I could restructure the series. > I might well have missed something that prevents doing this, but if > possible I think this would be a better approach. I don't think it would be impossible to restructure the patches in that way, but as I explained I'm not sure to really see the added value. I may also be missing something. If you find it particularly difficult to review 1/4 and 2/4, please let me know. > > > > .../media/v4l/vidioc-subdev-g-client-cap.rst | 5 + > > .../v4l/vidioc-subdev-g-frame-interval.rst | 17 +- > > drivers/media/i2c/adv7180.c | 10 +- > > drivers/media/i2c/et8ek8/et8ek8_driver.c | 12 +- > > drivers/media/i2c/imx214.c | 12 +- > > drivers/media/i2c/imx274.c | 54 +++--- > > drivers/media/i2c/max9286.c | 20 ++- > > drivers/media/i2c/mt9m111.c | 20 ++- > > drivers/media/i2c/mt9m114.c | 20 ++- > > drivers/media/i2c/mt9v011.c | 24 +-- > > drivers/media/i2c/mt9v111.c | 22 ++- > > drivers/media/i2c/ov2680.c | 10 +- > > drivers/media/i2c/ov5640.c | 22 ++- > > drivers/media/i2c/ov5648.c | 62 +++---- > > drivers/media/i2c/ov5693.c | 10 +- > > drivers/media/i2c/ov6650.c | 22 ++- > > drivers/media/i2c/ov7251.c | 12 +- > > drivers/media/i2c/ov7670.c | 22 +-- > > drivers/media/i2c/ov772x.c | 20 ++- > > drivers/media/i2c/ov7740.c | 40 ++--- > > drivers/media/i2c/ov8865.c | 54 +++--- > > drivers/media/i2c/ov9650.c | 20 ++- > > drivers/media/i2c/s5c73m3/s5c73m3-core.c | 20 ++- > > drivers/media/i2c/s5k5baf.c | 26 ++- > > drivers/media/i2c/thp7312.c | 160 ++++++++++-------- > > drivers/media/i2c/tvp514x.c | 33 ++-- > > drivers/media/usb/em28xx/em28xx-video.c | 6 +- > > drivers/media/v4l2-core/v4l2-common.c | 8 +- > > drivers/media/v4l2-core/v4l2-subdev.c | 128 ++++++++++---- > > .../media/atomisp/i2c/atomisp-gc0310.c | 10 +- > > .../media/atomisp/i2c/atomisp-gc2235.c | 10 +- > > .../media/atomisp/i2c/atomisp-mt9m114.c | 10 +- > > .../media/atomisp/i2c/atomisp-ov2722.c | 10 +- > > .../staging/media/atomisp/pci/atomisp_cmd.c | 4 +- > > .../staging/media/atomisp/pci/atomisp_ioctl.c | 4 +- > > drivers/staging/media/imx/imx-ic-prp.c | 20 ++- > > drivers/staging/media/imx/imx-ic-prpencvf.c | 20 ++- > > drivers/staging/media/imx/imx-media-capture.c | 6 +- > > drivers/staging/media/imx/imx-media-csi.c | 20 ++- > > drivers/staging/media/imx/imx-media-vdic.c | 20 ++- > > drivers/staging/media/tegra-video/csi.c | 12 +- > > include/media/v4l2-common.h | 4 +- > > include/media/v4l2-subdev.h | 65 +++++-- > > include/uapi/linux/v4l2-subdev.h | 13 +- > > 44 files changed, 706 insertions(+), 413 deletions(-) > > > > > > base-commit: 543efaddeac0c7769c39d7aaa886e8b001acac76
On 27/11/2023 12:13, Laurent Pinchart wrote: > Due to a historical mishap, the v4l2_subdev_frame_interval structure > is the only part of the V4L2 subdev userspace API that doesn't contain a > 'which' field. This prevents trying frame intervals using the subdev > 'TRY' state mechanism. > > Adding a 'which' field is simple as the structure has 8 reserved fields. > This would however break userspace as the field is currently set to 0, > corresponding to V4L2_SUBDEV_FORMAT_TRY, while the corresponding ioctls > currently operate on the 'ACTIVE' state. We thus need to add a new > subdev client cap, V4L2_SUBDEV_CLIENT_CAP_WHICH_INTERVAL, to indicate > that userspace is aware of this new field. > > All drivers that implement the subdev .get_frame_interval() and > .set_frame_interval() operations are updated to return -EINVAL when > operating on the TRY state, preserving the current behaviour. > > While at it, fix a bad copy&paste in the documentation of the struct > v4l2_subdev_frame_interval_enum 'which' field. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Changes since v1: > > - Fix .[gs]et_frame_interval() operation names in commit message > - Fix typo in commit message > --- > .../media/v4l/vidioc-subdev-g-client-cap.rst | 5 ++++ > .../v4l/vidioc-subdev-g-frame-interval.rst | 17 ++++++++----- > drivers/media/i2c/adv7180.c | 3 +++ > drivers/media/i2c/et8ek8/et8ek8_driver.c | 6 +++++ > drivers/media/i2c/imx214.c | 3 +++ > drivers/media/i2c/imx274.c | 6 +++++ > drivers/media/i2c/max9286.c | 6 +++++ > drivers/media/i2c/mt9m111.c | 6 +++++ > drivers/media/i2c/mt9m114.c | 6 +++++ > drivers/media/i2c/mt9v011.c | 6 +++++ > drivers/media/i2c/mt9v111.c | 6 +++++ > drivers/media/i2c/ov2680.c | 3 +++ > drivers/media/i2c/ov5640.c | 6 +++++ > drivers/media/i2c/ov5648.c | 3 +++ > drivers/media/i2c/ov5693.c | 3 +++ > drivers/media/i2c/ov6650.c | 6 +++++ > drivers/media/i2c/ov7251.c | 6 +++++ > drivers/media/i2c/ov7670.c | 4 +++ > drivers/media/i2c/ov772x.c | 6 +++++ > drivers/media/i2c/ov8865.c | 3 +++ > drivers/media/i2c/ov9650.c | 6 +++++ > drivers/media/i2c/s5c73m3/s5c73m3-core.c | 6 +++++ > drivers/media/i2c/s5k5baf.c | 6 +++++ > drivers/media/i2c/thp7312.c | 6 +++++ > drivers/media/i2c/tvp514x.c | 4 +++ > drivers/media/v4l2-core/v4l2-subdev.c | 25 ++++++++++++------- > .../media/atomisp/i2c/atomisp-gc0310.c | 3 +++ > .../media/atomisp/i2c/atomisp-gc2235.c | 3 +++ > .../media/atomisp/i2c/atomisp-mt9m114.c | 3 +++ > .../media/atomisp/i2c/atomisp-ov2722.c | 3 +++ > drivers/staging/media/imx/imx-ic-prp.c | 6 +++++ > drivers/staging/media/imx/imx-ic-prpencvf.c | 6 +++++ > drivers/staging/media/imx/imx-media-csi.c | 6 +++++ > drivers/staging/media/imx/imx-media-vdic.c | 6 +++++ > drivers/staging/media/tegra-video/csi.c | 3 +++ > include/uapi/linux/v4l2-subdev.h | 13 ++++++++-- > 36 files changed, 198 insertions(+), 17 deletions(-) > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-client-cap.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-client-cap.rst > index 20f12a1cc0f7..f168140ebd59 100644 > --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-client-cap.rst > +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-client-cap.rst > @@ -71,6 +71,11 @@ is unknown to the kernel. > of 'stream' fields (referring to the stream number) with various > ioctls. If this is not set (which is the default), the 'stream' fields > will be forced to 0 by the kernel. > + * - ``V4L2_SUBDEV_CLIENT_CAP_WHICH_INTERVAL`` > + - The client is aware of the :c:type:`v4l2_subdev_frame_interval` > + ``which`` field. If this is not set (which is the default), the > + ``which`` field is forced to ``V4L2_SUBDEV_FORMAT_ACTIVE`` by the > + kernel. > > Return Value > ============ > diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.rst > index 842f962d2aea..41e0e2c8ecc3 100644 > --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.rst > +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.rst > @@ -58,8 +58,9 @@ struct > contains the current frame interval as would be returned by a > ``VIDIOC_SUBDEV_G_FRAME_INTERVAL`` call. > > -Calling ``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` on a subdev device node that has been > -registered in read-only mode is not allowed. An error is returned and the errno > +If the subdev device node has been registered in read-only mode, calls to > +``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` are only valid if the ``which`` field is set > +to ``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno > variable is set to ``-EPERM``. > > Drivers must not return an error solely because the requested interval > @@ -93,7 +94,11 @@ the same sub-device is not defined. > - ``stream`` > - Stream identifier. > * - __u32 > - - ``reserved``\ [8] > + - ``which`` > + - Active or try frame interval, from enum > + :ref:`v4l2_subdev_format_whence <v4l2-subdev-format-whence>`. > + * - __u32 > + - ``reserved``\ [7] > - Reserved for future extensions. Applications and drivers must set > the array to zero. > > @@ -114,9 +119,9 @@ EBUSY > EINVAL > The struct > :c:type:`v4l2_subdev_frame_interval` > - ``pad`` references a non-existing pad, or the pad doesn't support > - frame intervals. > + ``pad`` references a non-existing pad, the ``which`` field references a > + non-existing frame interval, or the pad doesn't support frame intervals. "the ``which`` field references a non-existing frame interval": that's a rather vague sentence. I noticed it was probably copy-and-pasted (VIDIOC_SUBDEV_G_FMT has a similar phrase), but it is not clear in that documentation either. I expect EINVAL if 'which' is set to something other than TRY or ACTIVE, or the driver does not support TRY. Is that what you meant with "references a non-existing frame interval"? The 'references a non-existing' phrase works for pads since pad is an index into a pad array, but that's not the case for 'which', which is effectively an enum, so there is no obvious indexing going on. I think a separate patch clarifying this EINVAL description for the relevant subdev ioctls might be useful. In any case, since this just copies existing text it isn't a blocker. > > EPERM > The ``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` ioctl has been called on a read-only > - subdevice. > + subdevice and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``. > diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c > index 7ed86030fb5c..e1eec9f86539 100644 > --- a/drivers/media/i2c/adv7180.c > +++ b/drivers/media/i2c/adv7180.c > @@ -469,6 +469,9 @@ static int adv7180_get_frame_interval(struct v4l2_subdev *sd, > { > struct adv7180_state *state = to_state(sd); > > + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > if (state->curr_norm & V4L2_STD_525_60) { > fi->interval.numerator = 1001; > fi->interval.denominator = 30000; > diff --git a/drivers/media/i2c/et8ek8/et8ek8_driver.c b/drivers/media/i2c/et8ek8/et8ek8_driver.c > index 71fb5aebd3df..359ed943533c 100644 > --- a/drivers/media/i2c/et8ek8/et8ek8_driver.c > +++ b/drivers/media/i2c/et8ek8/et8ek8_driver.c > @@ -1051,6 +1051,9 @@ static int et8ek8_get_frame_interval(struct v4l2_subdev *subdev, > { > struct et8ek8_sensor *sensor = to_et8ek8_sensor(subdev); > > + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > memset(fi, 0, sizeof(*fi)); > fi->interval = sensor->current_reglist->mode.timeperframe; > > @@ -1064,6 +1067,9 @@ static int et8ek8_set_frame_interval(struct v4l2_subdev *subdev, > struct et8ek8_sensor *sensor = to_et8ek8_sensor(subdev); > struct et8ek8_reglist *reglist; > > + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > reglist = et8ek8_reglist_find_mode_ival(&meta_reglist, > sensor->current_reglist, > &fi->interval); > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c > index 624efc8834f3..80d14bc6f1ca 100644 > --- a/drivers/media/i2c/imx214.c > +++ b/drivers/media/i2c/imx214.c > @@ -799,6 +799,9 @@ static int imx214_get_frame_interval(struct v4l2_subdev *subdev, > struct v4l2_subdev_state *sd_state, > struct v4l2_subdev_frame_interval *fival) > { > + if (fival->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > fival->interval.numerator = 1; > fival->interval.denominator = IMX214_FPS; > > diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c > index 4040c642a36f..9f9fb3c488e2 100644 > --- a/drivers/media/i2c/imx274.c > +++ b/drivers/media/i2c/imx274.c > @@ -1333,6 +1333,9 @@ static int imx274_get_frame_interval(struct v4l2_subdev *sd, > { > struct stimx274 *imx274 = to_imx274(sd); > > + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > fi->interval = imx274->frame_interval; > dev_dbg(&imx274->client->dev, "%s frame rate = %d / %d\n", > __func__, imx274->frame_interval.numerator, > @@ -1350,6 +1353,9 @@ static int imx274_set_frame_interval(struct v4l2_subdev *sd, > int min, max, def; > int ret; > > + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > ret = pm_runtime_resume_and_get(&imx274->client->dev); > if (ret < 0) > return ret; > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > index 7e8cb53d31c3..16f81479d411 100644 > --- a/drivers/media/i2c/max9286.c > +++ b/drivers/media/i2c/max9286.c > @@ -874,6 +874,9 @@ static int max9286_get_frame_interval(struct v4l2_subdev *sd, > { > struct max9286_priv *priv = sd_to_max9286(sd); > > + if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > if (interval->pad != MAX9286_SRC_PAD) > return -EINVAL; > > @@ -888,6 +891,9 @@ static int max9286_set_frame_interval(struct v4l2_subdev *sd, > { > struct max9286_priv *priv = sd_to_max9286(sd); > > + if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > if (interval->pad != MAX9286_SRC_PAD) > return -EINVAL; > > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c > index 602954650f2e..a30c17594b8e 100644 > --- a/drivers/media/i2c/mt9m111.c > +++ b/drivers/media/i2c/mt9m111.c > @@ -1051,6 +1051,9 @@ static int mt9m111_get_frame_interval(struct v4l2_subdev *sd, > { > struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev); > > + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > fi->interval = mt9m111->frame_interval; > > return 0; > @@ -1068,6 +1071,9 @@ static int mt9m111_set_frame_interval(struct v4l2_subdev *sd, > if (mt9m111->is_streaming) > return -EBUSY; > > + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > if (fi->pad != 0) > return -EINVAL; > > diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c > index dcd94299787c..5e0d85b3d158 100644 > --- a/drivers/media/i2c/mt9m114.c > +++ b/drivers/media/i2c/mt9m114.c > @@ -1592,6 +1592,9 @@ static int mt9m114_ifp_get_frame_interval(struct v4l2_subdev *sd, > struct v4l2_fract *ival = &interval->interval; > struct mt9m114 *sensor = ifp_to_mt9m114(sd); > > + if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > mutex_lock(sensor->ifp.hdl.lock); > > ival->numerator = 1; > @@ -1610,6 +1613,9 @@ static int mt9m114_ifp_set_frame_interval(struct v4l2_subdev *sd, > struct mt9m114 *sensor = ifp_to_mt9m114(sd); > int ret = 0; > > + if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > mutex_lock(sensor->ifp.hdl.lock); > > if (ival->numerator != 0 && ival->denominator != 0) > diff --git a/drivers/media/i2c/mt9v011.c b/drivers/media/i2c/mt9v011.c > index 3485761428ba..bc8c0591e4ae 100644 > --- a/drivers/media/i2c/mt9v011.c > +++ b/drivers/media/i2c/mt9v011.c > @@ -366,6 +366,9 @@ static int mt9v011_get_frame_interval(struct v4l2_subdev *sd, > struct v4l2_subdev_state *sd_state, > struct v4l2_subdev_frame_interval *ival) > { > + if (ival->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > calc_fps(sd, > &ival->interval.numerator, > &ival->interval.denominator); > @@ -380,6 +383,9 @@ static int mt9v011_set_frame_interval(struct v4l2_subdev *sd, > struct v4l2_fract *tpf = &ival->interval; > u16 speed; > > + if (ival->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > speed = calc_speed(sd, tpf->numerator, tpf->denominator); > > mt9v011_write(sd, R0A_MT9V011_CLK_SPEED, speed); > diff --git a/drivers/media/i2c/mt9v111.c b/drivers/media/i2c/mt9v111.c > index 496be67c971b..b62624771e7b 100644 > --- a/drivers/media/i2c/mt9v111.c > +++ b/drivers/media/i2c/mt9v111.c > @@ -730,6 +730,9 @@ static int mt9v111_set_frame_interval(struct v4l2_subdev *sd, > tpf->denominator; > unsigned int max_fps; > > + if (ival->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > if (!tpf->numerator) > tpf->numerator = 1; > > @@ -779,6 +782,9 @@ static int mt9v111_get_frame_interval(struct v4l2_subdev *sd, > struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); > struct v4l2_fract *tpf = &ival->interval; > > + if (ival->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > mutex_lock(&mt9v111->stream_mutex); > > tpf->numerator = 1; > diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c > index e3ff64a9e6ca..5455e7afd1b3 100644 > --- a/drivers/media/i2c/ov2680.c > +++ b/drivers/media/i2c/ov2680.c > @@ -558,6 +558,9 @@ static int ov2680_get_frame_interval(struct v4l2_subdev *sd, > { > struct ov2680_dev *sensor = to_ov2680_dev(sd); > > + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > mutex_lock(&sensor->lock); > fi->interval = sensor->mode.frame_interval; > mutex_unlock(&sensor->lock); > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index 336bfd1ffd32..2d75a67a3aff 100644 > --- a/drivers/media/i2c/ov5640.c > +++ b/drivers/media/i2c/ov5640.c > @@ -3610,6 +3610,9 @@ static int ov5640_get_frame_interval(struct v4l2_subdev *sd, > { > struct ov5640_dev *sensor = to_ov5640_dev(sd); > > + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > mutex_lock(&sensor->lock); > fi->interval = sensor->frame_interval; > mutex_unlock(&sensor->lock); > @@ -3625,6 +3628,9 @@ static int ov5640_set_frame_interval(struct v4l2_subdev *sd, > const struct ov5640_mode_info *mode; > int frame_rate, ret = 0; > > + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > if (fi->pad != 0) > return -EINVAL; > > diff --git a/drivers/media/i2c/ov5648.c b/drivers/media/i2c/ov5648.c > index d0d7e9968f48..f02a7e263aee 100644 > --- a/drivers/media/i2c/ov5648.c > +++ b/drivers/media/i2c/ov5648.c > @@ -2276,6 +2276,9 @@ static int ov5648_get_frame_interval(struct v4l2_subdev *subdev, > const struct ov5648_mode *mode; > int ret = 0; > > + if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > mutex_lock(&sensor->mutex); > > mode = sensor->state.mode; > diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c > index a65645811fbc..ce49176560d4 100644 > --- a/drivers/media/i2c/ov5693.c > +++ b/drivers/media/i2c/ov5693.c > @@ -1013,6 +1013,9 @@ static int ov5693_get_frame_interval(struct v4l2_subdev *sd, > ov5693->ctrls.vblank->val); > unsigned int fps = DIV_ROUND_CLOSEST(OV5693_PIXEL_RATE, framesize); > > + if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > interval->interval.numerator = 1; > interval->interval.denominator = fps; > > diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c > index a4dc45bdf3d7..4ef2b7db315e 100644 > --- a/drivers/media/i2c/ov6650.c > +++ b/drivers/media/i2c/ov6650.c > @@ -806,6 +806,9 @@ static int ov6650_get_frame_interval(struct v4l2_subdev *sd, > struct i2c_client *client = v4l2_get_subdevdata(sd); > struct ov6650 *priv = to_ov6650(client); > > + if (ival->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > ival->interval = priv->tpf; > > dev_dbg(&client->dev, "Frame interval: %u/%u s\n", > @@ -823,6 +826,9 @@ static int ov6650_set_frame_interval(struct v4l2_subdev *sd, > struct v4l2_fract *tpf = &ival->interval; > int div, ret; > > + if (ival->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > if (tpf->numerator == 0 || tpf->denominator == 0) > div = 1; /* Reset to full rate */ > else > diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c > index 10d6b5deed83..08f5f2d0538d 100644 > --- a/drivers/media/i2c/ov7251.c > +++ b/drivers/media/i2c/ov7251.c > @@ -1391,6 +1391,9 @@ static int ov7251_get_frame_interval(struct v4l2_subdev *subdev, > { > struct ov7251 *ov7251 = to_ov7251(subdev); > > + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > mutex_lock(&ov7251->lock); > fi->interval = ov7251->current_mode->timeperframe; > mutex_unlock(&ov7251->lock); > @@ -1406,6 +1409,9 @@ static int ov7251_set_frame_interval(struct v4l2_subdev *subdev, > const struct ov7251_mode_info *new_mode; > int ret = 0; > > + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > mutex_lock(&ov7251->lock); > new_mode = ov7251_find_mode_by_ival(ov7251, &fi->interval); > > diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c > index 463f20ece36e..7874a8dd7cf0 100644 > --- a/drivers/media/i2c/ov7670.c > +++ b/drivers/media/i2c/ov7670.c > @@ -1160,6 +1160,8 @@ static int ov7670_get_frame_interval(struct v4l2_subdev *sd, > { > struct ov7670_info *info = to_state(sd); > > + if (ival->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > > info->devtype->get_framerate(sd, &ival->interval); > > @@ -1173,6 +1175,8 @@ static int ov7670_set_frame_interval(struct v4l2_subdev *sd, > struct v4l2_fract *tpf = &ival->interval; > struct ov7670_info *info = to_state(sd); > > + if (ival->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > > return info->devtype->set_framerate(sd, tpf); > } > diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c > index a14a25946c5b..d9a73871f7a3 100644 > --- a/drivers/media/i2c/ov772x.c > +++ b/drivers/media/i2c/ov772x.c > @@ -724,6 +724,9 @@ static int ov772x_get_frame_interval(struct v4l2_subdev *sd, > struct ov772x_priv *priv = to_ov772x(sd); > struct v4l2_fract *tpf = &ival->interval; > > + if (ival->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > tpf->numerator = 1; > tpf->denominator = priv->fps; > > @@ -739,6 +742,9 @@ static int ov772x_set_frame_interval(struct v4l2_subdev *sd, > unsigned int fps; > int ret = 0; > > + if (ival->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > mutex_lock(&priv->lock); > > if (priv->streaming) { > diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c > index 02a595281c49..7a25dcd900f2 100644 > --- a/drivers/media/i2c/ov8865.c > +++ b/drivers/media/i2c/ov8865.c > @@ -2846,6 +2846,9 @@ static int ov8865_get_frame_interval(struct v4l2_subdev *subdev, > unsigned int framesize; > unsigned int fps; > > + if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > mutex_lock(&sensor->mutex); > > mode = sensor->state.mode; > diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c > index f528892c893f..08be6c4fc6d5 100644 > --- a/drivers/media/i2c/ov9650.c > +++ b/drivers/media/i2c/ov9650.c > @@ -1107,6 +1107,9 @@ static int ov965x_get_frame_interval(struct v4l2_subdev *sd, > { > struct ov965x *ov965x = to_ov965x(sd); > > + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > mutex_lock(&ov965x->lock); > fi->interval = ov965x->fiv->interval; > mutex_unlock(&ov965x->lock); > @@ -1156,6 +1159,9 @@ static int ov965x_set_frame_interval(struct v4l2_subdev *sd, > struct ov965x *ov965x = to_ov965x(sd); > int ret; > > + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > v4l2_dbg(1, debug, sd, "Setting %d/%d frame interval\n", > fi->interval.numerator, fi->interval.denominator); > > diff --git a/drivers/media/i2c/s5c73m3/s5c73m3-core.c b/drivers/media/i2c/s5c73m3/s5c73m3-core.c > index 73ca50f49812..71a51794ced4 100644 > --- a/drivers/media/i2c/s5c73m3/s5c73m3-core.c > +++ b/drivers/media/i2c/s5c73m3/s5c73m3-core.c > @@ -872,6 +872,9 @@ static int s5c73m3_oif_get_frame_interval(struct v4l2_subdev *sd, > { > struct s5c73m3 *state = oif_sd_to_s5c73m3(sd); > > + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > if (fi->pad != OIF_SOURCE_PAD) > return -EINVAL; > > @@ -923,6 +926,9 @@ static int s5c73m3_oif_set_frame_interval(struct v4l2_subdev *sd, > struct s5c73m3 *state = oif_sd_to_s5c73m3(sd); > int ret; > > + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > if (fi->pad != OIF_SOURCE_PAD) > return -EINVAL; > > diff --git a/drivers/media/i2c/s5k5baf.c b/drivers/media/i2c/s5k5baf.c > index 2fd1ecfeb086..6b1a2c4946a9 100644 > --- a/drivers/media/i2c/s5k5baf.c > +++ b/drivers/media/i2c/s5k5baf.c > @@ -1124,6 +1124,9 @@ static int s5k5baf_get_frame_interval(struct v4l2_subdev *sd, > { > struct s5k5baf *state = to_s5k5baf(sd); > > + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > mutex_lock(&state->lock); > fi->interval.numerator = state->fiv; > fi->interval.denominator = 10000; > @@ -1162,6 +1165,9 @@ static int s5k5baf_set_frame_interval(struct v4l2_subdev *sd, > { > struct s5k5baf *state = to_s5k5baf(sd); > > + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > mutex_lock(&state->lock); > __s5k5baf_set_frame_interval(state, fi); > mutex_unlock(&state->lock); > diff --git a/drivers/media/i2c/thp7312.c b/drivers/media/i2c/thp7312.c > index 1698f3faa7cd..c8f42a588002 100644 > --- a/drivers/media/i2c/thp7312.c > +++ b/drivers/media/i2c/thp7312.c > @@ -740,6 +740,9 @@ static int thp7312_get_frame_interval(struct v4l2_subdev *sd, > { > struct thp7312_device *thp7312 = to_thp7312_dev(sd); > > + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > fi->interval.numerator = 1; > fi->interval.denominator = thp7312->current_rate->fps; > > @@ -757,6 +760,9 @@ static int thp7312_set_frame_interval(struct v4l2_subdev *sd, > unsigned int best_delta = UINT_MAX; > unsigned int fps; > > + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > /* Avoid divisions by 0, pick the highest frame if the interval is 0. */ > fps = fi->interval.numerator > ? DIV_ROUND_CLOSEST(fi->interval.denominator, fi->interval.numerator) > diff --git a/drivers/media/i2c/tvp514x.c b/drivers/media/i2c/tvp514x.c > index dee0cf992379..ae073a532eda 100644 > --- a/drivers/media/i2c/tvp514x.c > +++ b/drivers/media/i2c/tvp514x.c > @@ -746,6 +746,8 @@ tvp514x_get_frame_interval(struct v4l2_subdev *sd, > struct tvp514x_decoder *decoder = to_decoder(sd); > enum tvp514x_std current_std; > > + if (ival->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > > /* get the current standard */ > current_std = decoder->current_std; > @@ -765,6 +767,8 @@ tvp514x_set_frame_interval(struct v4l2_subdev *sd, > struct v4l2_fract *timeperframe; > enum tvp514x_std current_std; > > + if (ival->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > > timeperframe = &ival->interval; > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index 08c908988f7d..4cbe4024ff67 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -291,9 +291,8 @@ static inline int check_frame_interval(struct v4l2_subdev *sd, > if (!fi) > return -EINVAL; > > - return check_pad(sd, fi->pad) ? : > - check_state(sd, state, V4L2_SUBDEV_FORMAT_ACTIVE, fi->pad, > - fi->stream); > + return check_which(fi->which) ? : check_pad(sd, fi->pad) ? : > + check_state(sd, state, fi->which, fi->pad, fi->stream); > } > > static int call_get_frame_interval(struct v4l2_subdev *sd, > @@ -537,9 +536,16 @@ subdev_ioctl_get_state(struct v4l2_subdev *sd, struct v4l2_subdev_fh *subdev_fh, > which = ((struct v4l2_subdev_selection *)arg)->which; > break; > case VIDIOC_SUBDEV_G_FRAME_INTERVAL: > - case VIDIOC_SUBDEV_S_FRAME_INTERVAL: > - which = V4L2_SUBDEV_FORMAT_ACTIVE; > + case VIDIOC_SUBDEV_S_FRAME_INTERVAL: { > + struct v4l2_subdev_frame_interval *fi = arg; > + > + if (!(subdev_fh->client_caps & > + V4L2_SUBDEV_CLIENT_CAP_WHICH_INTERVAL)) > + fi->which = V4L2_SUBDEV_FORMAT_ACTIVE; > + > + which = fi->which; > break; > + } > case VIDIOC_SUBDEV_G_ROUTING: > case VIDIOC_SUBDEV_S_ROUTING: > which = ((struct v4l2_subdev_routing *)arg)->which; > @@ -796,12 +802,12 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, > case VIDIOC_SUBDEV_S_FRAME_INTERVAL: { > struct v4l2_subdev_frame_interval *fi = arg; > > - if (ro_subdev) > - return -EPERM; > - > if (!client_supports_streams) > fi->stream = 0; > > + if (fi->which != V4L2_SUBDEV_FORMAT_TRY && ro_subdev) > + return -EPERM; > + > memset(fi->reserved, 0, sizeof(fi->reserved)); > return v4l2_subdev_call(sd, pad, set_frame_interval, state, fi); > } > @@ -998,7 +1004,8 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, > client_cap->capabilities &= ~V4L2_SUBDEV_CLIENT_CAP_STREAMS; > > /* Filter out unsupported capabilities */ > - client_cap->capabilities &= V4L2_SUBDEV_CLIENT_CAP_STREAMS; > + client_cap->capabilities &= (V4L2_SUBDEV_CLIENT_CAP_STREAMS | > + V4L2_SUBDEV_CLIENT_CAP_WHICH_INTERVAL); > > subdev_fh->client_caps = client_cap->capabilities; > > diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c > index 006e8adac47b..3a032e1a06f8 100644 > --- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c > +++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c > @@ -500,6 +500,9 @@ static int gc0310_get_frame_interval(struct v4l2_subdev *sd, > struct v4l2_subdev_state *sd_state, > struct v4l2_subdev_frame_interval *interval) > { > + if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > interval->interval.numerator = 1; > interval->interval.denominator = GC0310_FPS; > > diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c b/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c > index aa257322a700..a2bbe2864049 100644 > --- a/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c > +++ b/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c > @@ -704,6 +704,9 @@ static int gc2235_get_frame_interval(struct v4l2_subdev *sd, > { > struct gc2235_device *dev = to_gc2235_sensor(sd); > > + if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > interval->interval.numerator = 1; > interval->interval.denominator = dev->res->fps; > > diff --git a/drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c b/drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c > index 459c5b8233ce..b4be6d3120b1 100644 > --- a/drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c > +++ b/drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c > @@ -1394,6 +1394,9 @@ static int mt9m114_get_frame_interval(struct v4l2_subdev *sd, > { > struct mt9m114_device *dev = to_mt9m114_sensor(sd); > > + if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > interval->interval.numerator = 1; > interval->interval.denominator = mt9m114_res[dev->res].fps; > > diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c > index b3ef04d7ccca..64e1addfc1c5 100644 > --- a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c > +++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c > @@ -851,6 +851,9 @@ static int ov2722_get_frame_interval(struct v4l2_subdev *sd, > { > struct ov2722_device *dev = to_ov2722_sensor(sd); > > + if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > interval->interval.numerator = 1; > interval->interval.denominator = dev->res->fps; > > diff --git a/drivers/staging/media/imx/imx-ic-prp.c b/drivers/staging/media/imx/imx-ic-prp.c > index fb96f87e664e..26c758b67bf2 100644 > --- a/drivers/staging/media/imx/imx-ic-prp.c > +++ b/drivers/staging/media/imx/imx-ic-prp.c > @@ -399,6 +399,9 @@ static int prp_get_frame_interval(struct v4l2_subdev *sd, > { > struct prp_priv *priv = sd_to_priv(sd); > > + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > if (fi->pad >= PRP_NUM_PADS) > return -EINVAL; > > @@ -415,6 +418,9 @@ static int prp_set_frame_interval(struct v4l2_subdev *sd, > { > struct prp_priv *priv = sd_to_priv(sd); > > + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > if (fi->pad >= PRP_NUM_PADS) > return -EINVAL; > > diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c > index 7bfe433cd322..94a8ace3fa7f 100644 > --- a/drivers/staging/media/imx/imx-ic-prpencvf.c > +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c > @@ -1209,6 +1209,9 @@ static int prp_get_frame_interval(struct v4l2_subdev *sd, > { > struct prp_priv *priv = sd_to_priv(sd); > > + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > if (fi->pad >= PRPENCVF_NUM_PADS) > return -EINVAL; > > @@ -1225,6 +1228,9 @@ static int prp_set_frame_interval(struct v4l2_subdev *sd, > { > struct prp_priv *priv = sd_to_priv(sd); > > + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > if (fi->pad >= PRPENCVF_NUM_PADS) > return -EINVAL; > > diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c > index 4308fdc9b58e..9af5a0d5ace4 100644 > --- a/drivers/staging/media/imx/imx-media-csi.c > +++ b/drivers/staging/media/imx/imx-media-csi.c > @@ -908,6 +908,9 @@ static int csi_get_frame_interval(struct v4l2_subdev *sd, > { > struct csi_priv *priv = v4l2_get_subdevdata(sd); > > + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > if (fi->pad >= CSI_NUM_PADS) > return -EINVAL; > > @@ -928,6 +931,9 @@ static int csi_set_frame_interval(struct v4l2_subdev *sd, > struct v4l2_fract *input_fi; > int ret = 0; > > + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > mutex_lock(&priv->lock); > > input_fi = &priv->frame_interval[CSI_SINK_PAD]; > diff --git a/drivers/staging/media/imx/imx-media-vdic.c b/drivers/staging/media/imx/imx-media-vdic.c > index a51b37679239..d34e11d925a1 100644 > --- a/drivers/staging/media/imx/imx-media-vdic.c > +++ b/drivers/staging/media/imx/imx-media-vdic.c > @@ -786,6 +786,9 @@ static int vdic_get_frame_interval(struct v4l2_subdev *sd, > { > struct vdic_priv *priv = v4l2_get_subdevdata(sd); > > + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > if (fi->pad >= VDIC_NUM_PADS) > return -EINVAL; > > @@ -806,6 +809,9 @@ static int vdic_set_frame_interval(struct v4l2_subdev *sd, > struct v4l2_fract *input_fi, *output_fi; > int ret = 0; > > + if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > mutex_lock(&priv->lock); > > input_fi = &priv->frame_interval[priv->active_input_pad]; > diff --git a/drivers/staging/media/tegra-video/csi.c b/drivers/staging/media/tegra-video/csi.c > index b1b666179be5..a2ce8d025eaf 100644 > --- a/drivers/staging/media/tegra-video/csi.c > +++ b/drivers/staging/media/tegra-video/csi.c > @@ -231,6 +231,9 @@ static int tegra_csi_get_frame_interval(struct v4l2_subdev *subdev, > if (!IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG)) > return -ENOIOCTLCMD; > > + if (vfi->which != V4L2_SUBDEV_FORMAT_ACTIVE) > + return -EINVAL; > + > vfi->interval.numerator = 1; > vfi->interval.denominator = csi_chan->framerate; > > diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h > index 4a195b68f28f..995b4f442a0d 100644 > --- a/include/uapi/linux/v4l2-subdev.h > +++ b/include/uapi/linux/v4l2-subdev.h > @@ -116,13 +116,15 @@ struct v4l2_subdev_frame_size_enum { > * @pad: pad number, as reported by the media API > * @interval: frame interval in seconds > * @stream: stream number, defined in subdev routing > + * @which: interval type (from enum v4l2_subdev_format_whence) > * @reserved: drivers and applications must zero this array > */ > struct v4l2_subdev_frame_interval { > __u32 pad; > struct v4l2_fract interval; > __u32 stream; > - __u32 reserved[8]; > + __u32 which; > + __u32 reserved[7]; > }; > > /** > @@ -133,7 +135,7 @@ struct v4l2_subdev_frame_interval { > * @width: frame width in pixels > * @height: frame height in pixels > * @interval: frame interval in seconds > - * @which: format type (from enum v4l2_subdev_format_whence) > + * @which: interval type (from enum v4l2_subdev_format_whence) > * @stream: stream number, defined in subdev routing > * @reserved: drivers and applications must zero this array > */ > @@ -241,6 +243,13 @@ struct v4l2_subdev_routing { > */ > #define V4L2_SUBDEV_CLIENT_CAP_STREAMS (1U << 0) > > +/* > + * The client is aware of the struct v4l2_subdev_frame_interval which field. If > + * this is not set (which is the default), the which field is forced to > + * V4L2_SUBDEV_FORMAT_ACTIVE by the kernel. > + */ > +#define V4L2_SUBDEV_CLIENT_CAP_WHICH_INTERVAL (1U << 1) > + > /** > * struct v4l2_subdev_client_capability - Capabilities of the client accessing > * the subdev Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Regards, Hans
On 27/11/2023 12:17, Laurent Pinchart wrote: > Use the newly added storage for frame interval in the subdev state to > simplify the driver. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Regards, Hans > --- > drivers/media/i2c/thp7312.c | 150 +++++++++++++++++++----------------- > 1 file changed, 81 insertions(+), 69 deletions(-) > > diff --git a/drivers/media/i2c/thp7312.c b/drivers/media/i2c/thp7312.c > index c8f42a588002..bc9d8306aef0 100644 > --- a/drivers/media/i2c/thp7312.c > +++ b/drivers/media/i2c/thp7312.c > @@ -266,9 +266,6 @@ struct thp7312_device { > struct v4l2_ctrl_handler ctrl_handler; > bool ctrls_applied; > > - /* These are protected by v4l2 active state */ > - const struct thp7312_mode_info *current_mode; > - const struct thp7312_frame_rate *current_rate; > s64 link_freq; > > struct { > @@ -310,6 +307,47 @@ static inline struct thp7312_device *to_thp7312_dev(struct v4l2_subdev *sd) > return container_of(sd, struct thp7312_device, sd); > } > > +static const struct thp7312_mode_info * > +thp7312_find_mode(unsigned int width, unsigned int height, bool nearest) > +{ > + const struct thp7312_mode_info *mode; > + > + mode = v4l2_find_nearest_size(thp7312_mode_info_data, > + ARRAY_SIZE(thp7312_mode_info_data), > + width, height, width, height); > + > + if (!nearest && (mode->width != width || mode->height != height)) > + return NULL; > + > + return mode; > +} > + > +static const struct thp7312_frame_rate * > +thp7312_find_rate(const struct thp7312_mode_info *mode, unsigned int fps, > + bool nearest) > +{ > + const struct thp7312_frame_rate *best_rate = NULL; > + const struct thp7312_frame_rate *rate; > + unsigned int best_delta = UINT_MAX; > + > + if (!mode) > + return NULL; > + > + for (rate = mode->rates; rate->fps && best_delta; ++rate) { > + unsigned int delta = abs(rate->fps - fps); > + > + if (delta <= best_delta) { > + best_delta = delta; > + best_rate = rate; > + } > + } > + > + if (!nearest && best_delta) > + return NULL; > + > + return best_rate; > +} > + > /* ----------------------------------------------------------------------------- > * Device Access & Configuration > */ > @@ -442,17 +480,30 @@ static int thp7312_set_framefmt(struct thp7312_device *thp7312, > static int thp7312_init_mode(struct thp7312_device *thp7312, > struct v4l2_subdev_state *sd_state) > { > + const struct thp7312_mode_info *mode; > + const struct thp7312_frame_rate *rate; > struct v4l2_mbus_framefmt *fmt; > + struct v4l2_fract *interval; > int ret; > > + /* > + * TODO: The mode and rate should be cached in the subdev state, once > + * support for extending states will be available. > + */ > fmt = v4l2_subdev_state_get_format(sd_state, 0); > + interval = v4l2_subdev_state_get_interval(sd_state, 0); > + > + mode = thp7312_find_mode(fmt->width, fmt->height, false); > + rate = thp7312_find_rate(mode, interval->denominator, false); > + > + if (WARN_ON(!mode || !rate)) > + return -EINVAL; > > ret = thp7312_set_framefmt(thp7312, fmt); > if (ret) > return ret; > > - return thp7312_change_mode(thp7312, thp7312->current_mode, > - thp7312->current_rate); > + return thp7312_change_mode(thp7312, mode, rate); > } > > static int thp7312_stream_enable(struct thp7312_device *thp7312, bool enable) > @@ -621,28 +672,6 @@ static bool thp7312_find_bus_code(u32 code) > return false; > } > > -static const struct thp7312_mode_info * > -thp7312_find_mode(unsigned int width, unsigned int height, bool nearest) > -{ > - const struct thp7312_mode_info *mode; > - > - mode = v4l2_find_nearest_size(thp7312_mode_info_data, > - ARRAY_SIZE(thp7312_mode_info_data), > - width, height, width, height); > - > - if (!nearest && (mode->width != width || mode->height != height)) > - return NULL; > - > - return mode; > -} > - > -static void thp7312_set_frame_rate(struct thp7312_device *thp7312, > - const struct thp7312_frame_rate *rate) > -{ > - thp7312->link_freq = rate->link_freq; > - thp7312->current_rate = rate; > -} > - > static int thp7312_enum_mbus_code(struct v4l2_subdev *sd, > struct v4l2_subdev_state *sd_state, > struct v4l2_subdev_mbus_code_enum *code) > @@ -707,6 +736,7 @@ static int thp7312_set_fmt(struct v4l2_subdev *sd, > struct thp7312_device *thp7312 = to_thp7312_dev(sd); > struct v4l2_mbus_framefmt *mbus_fmt = &format->format; > struct v4l2_mbus_framefmt *fmt; > + struct v4l2_fract *interval; > const struct thp7312_mode_info *mode; > > if (!thp7312_find_bus_code(mbus_fmt->code)) > @@ -726,25 +756,12 @@ static int thp7312_set_fmt(struct v4l2_subdev *sd, > > *mbus_fmt = *fmt; > > - if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) { > - thp7312->current_mode = mode; > - thp7312_set_frame_rate(thp7312, &mode->rates[0]); > - } > + interval = v4l2_subdev_state_get_interval(sd_state, 0); > + interval->numerator = 1; > + interval->denominator = mode->rates[0].fps; > > - return 0; > -} > - > -static int thp7312_get_frame_interval(struct v4l2_subdev *sd, > - struct v4l2_subdev_state *sd_state, > - struct v4l2_subdev_frame_interval *fi) > -{ > - 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; > + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) > + thp7312->link_freq = mode->rates[0].link_freq; > > return 0; > } > @@ -755,34 +772,28 @@ static int thp7312_set_frame_interval(struct v4l2_subdev *sd, > { > struct thp7312_device *thp7312 = to_thp7312_dev(sd); > const struct thp7312_mode_info *mode; > - const struct thp7312_frame_rate *best_rate = NULL; > const struct thp7312_frame_rate *rate; > - unsigned int best_delta = UINT_MAX; > + const struct v4l2_mbus_framefmt *fmt; > + struct v4l2_fract *interval; > 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) > : UINT_MAX; > > - mode = thp7312->current_mode; > + fmt = v4l2_subdev_state_get_format(sd_state, 0); > + mode = thp7312_find_mode(fmt->width, fmt->height, false); > + rate = thp7312_find_rate(mode, fps, true); > > - for (rate = mode->rates; rate->fps && best_delta; ++rate) { > - unsigned int delta = abs(rate->fps - fps); > + interval = v4l2_subdev_state_get_interval(sd_state, 0); > + interval->numerator = 1; > + interval->denominator = rate->fps; > > - if (delta <= best_delta) { > - best_delta = delta; > - best_rate = rate; > - } > - } > + if (fi->which == V4L2_SUBDEV_FORMAT_ACTIVE) > + thp7312->link_freq = rate->link_freq; > > - thp7312_set_frame_rate(thp7312, best_rate); > - > - fi->interval.numerator = 1; > - fi->interval.denominator = best_rate->fps; > + fi->interval = *interval; > > return 0; > } > @@ -842,8 +853,10 @@ static int thp7312_init_state(struct v4l2_subdev *sd, > { > const struct thp7312_mode_info *default_mode = &thp7312_mode_info_data[0]; > struct v4l2_mbus_framefmt *fmt; > + struct v4l2_fract *interval; > > fmt = v4l2_subdev_state_get_format(sd_state, 0); > + interval = v4l2_subdev_state_get_interval(sd_state, 0); > > /* > * default init sequence initialize thp7312 to > @@ -858,6 +871,9 @@ static int thp7312_init_state(struct v4l2_subdev *sd, > fmt->height = default_mode->height; > fmt->field = V4L2_FIELD_NONE; > > + interval->numerator = 1; > + interval->denominator = default_mode->rates[0].fps; > + > return 0; > } > > @@ -875,7 +891,7 @@ static const struct v4l2_subdev_pad_ops thp7312_pad_ops = { > .enum_mbus_code = thp7312_enum_mbus_code, > .get_fmt = v4l2_subdev_get_fmt, > .set_fmt = thp7312_set_fmt, > - .get_frame_interval = thp7312_get_frame_interval, > + .get_frame_interval = v4l2_subdev_get_frame_interval, > .set_frame_interval = thp7312_set_frame_interval, > .enum_frame_size = thp7312_enum_frame_size, > .enum_frame_interval = thp7312_enum_frame_interval, > @@ -1303,6 +1319,8 @@ static int thp7312_init_controls(struct thp7312_device *thp7312) > V4L2_CID_POWER_LINE_FREQUENCY_60HZ, 0, > V4L2_CID_POWER_LINE_FREQUENCY_50HZ); > > + thp7312->link_freq = thp7312_mode_info_data[0].rates[0].link_freq; > + > link_freq = v4l2_ctrl_new_int_menu(hdl, &thp7312_ctrl_ops, > V4L2_CID_LINK_FREQ, 0, 0, > &thp7312->link_freq); > @@ -2069,7 +2087,6 @@ static int thp7312_parse_dt(struct thp7312_device *thp7312) > static int thp7312_probe(struct i2c_client *client) > { > struct device *dev = &client->dev; > - struct v4l2_subdev_state *sd_state; > struct thp7312_device *thp7312; > int ret; > > @@ -2145,11 +2162,6 @@ static int thp7312_probe(struct i2c_client *client) > goto err_free_ctrls; > } > > - sd_state = v4l2_subdev_lock_and_get_active_state(&thp7312->sd); > - thp7312->current_mode = &thp7312_mode_info_data[0]; > - thp7312_set_frame_rate(thp7312, &thp7312->current_mode->rates[0]); > - v4l2_subdev_unlock_state(sd_state); > - > /* > * Enable runtime PM with autosuspend. As the device has been powered > * manually, mark it as active, and increase the usage count without
On Mo, 2023-11-27 at 13:13 +0200, Laurent Pinchart wrote: > Due to a historical mishap, the v4l2_subdev_frame_interval structure > is the only part of the V4L2 subdev userspace API that doesn't contain a > 'which' field. This prevents trying frame intervals using the subdev > 'TRY' state mechanism. > > Adding a 'which' field is simple as the structure has 8 reserved fields. > This would however break userspace as the field is currently set to 0, > corresponding to V4L2_SUBDEV_FORMAT_TRY, while the corresponding ioctls > currently operate on the 'ACTIVE' state. We thus need to add a new > subdev client cap, V4L2_SUBDEV_CLIENT_CAP_WHICH_INTERVAL, to indicate > that userspace is aware of this new field. > > All drivers that implement the subdev .get_frame_interval() and > .set_frame_interval() operations are updated to return -EINVAL when > operating on the TRY state, preserving the current behaviour. > > While at it, fix a bad copy&paste in the documentation of the struct > v4l2_subdev_frame_interval_enum 'which' field. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> # for imx-media regards Philipp
Hi Hans, On Tue, Nov 28, 2023 at 10:32:28AM +0100, Hans Verkuil wrote: > On 27/11/2023 12:13, Laurent Pinchart wrote: > > Due to a historical mishap, the v4l2_subdev_frame_interval structure > > is the only part of the V4L2 subdev userspace API that doesn't contain a > > 'which' field. This prevents trying frame intervals using the subdev > > 'TRY' state mechanism. > > > > Adding a 'which' field is simple as the structure has 8 reserved fields. > > This would however break userspace as the field is currently set to 0, > > corresponding to V4L2_SUBDEV_FORMAT_TRY, while the corresponding ioctls > > currently operate on the 'ACTIVE' state. We thus need to add a new > > subdev client cap, V4L2_SUBDEV_CLIENT_CAP_WHICH_INTERVAL, to indicate > > that userspace is aware of this new field. > > > > All drivers that implement the subdev .get_frame_interval() and > > .set_frame_interval() operations are updated to return -EINVAL when > > operating on the TRY state, preserving the current behaviour. > > > > While at it, fix a bad copy&paste in the documentation of the struct > > v4l2_subdev_frame_interval_enum 'which' field. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > Changes since v1: > > > > - Fix .[gs]et_frame_interval() operation names in commit message > > - Fix typo in commit message > > --- > > .../media/v4l/vidioc-subdev-g-client-cap.rst | 5 ++++ > > .../v4l/vidioc-subdev-g-frame-interval.rst | 17 ++++++++----- > > drivers/media/i2c/adv7180.c | 3 +++ > > drivers/media/i2c/et8ek8/et8ek8_driver.c | 6 +++++ > > drivers/media/i2c/imx214.c | 3 +++ > > drivers/media/i2c/imx274.c | 6 +++++ > > drivers/media/i2c/max9286.c | 6 +++++ > > drivers/media/i2c/mt9m111.c | 6 +++++ > > drivers/media/i2c/mt9m114.c | 6 +++++ > > drivers/media/i2c/mt9v011.c | 6 +++++ > > drivers/media/i2c/mt9v111.c | 6 +++++ > > drivers/media/i2c/ov2680.c | 3 +++ > > drivers/media/i2c/ov5640.c | 6 +++++ > > drivers/media/i2c/ov5648.c | 3 +++ > > drivers/media/i2c/ov5693.c | 3 +++ > > drivers/media/i2c/ov6650.c | 6 +++++ > > drivers/media/i2c/ov7251.c | 6 +++++ > > drivers/media/i2c/ov7670.c | 4 +++ > > drivers/media/i2c/ov772x.c | 6 +++++ > > drivers/media/i2c/ov8865.c | 3 +++ > > drivers/media/i2c/ov9650.c | 6 +++++ > > drivers/media/i2c/s5c73m3/s5c73m3-core.c | 6 +++++ > > drivers/media/i2c/s5k5baf.c | 6 +++++ > > drivers/media/i2c/thp7312.c | 6 +++++ > > drivers/media/i2c/tvp514x.c | 4 +++ > > drivers/media/v4l2-core/v4l2-subdev.c | 25 ++++++++++++------- > > .../media/atomisp/i2c/atomisp-gc0310.c | 3 +++ > > .../media/atomisp/i2c/atomisp-gc2235.c | 3 +++ > > .../media/atomisp/i2c/atomisp-mt9m114.c | 3 +++ > > .../media/atomisp/i2c/atomisp-ov2722.c | 3 +++ > > drivers/staging/media/imx/imx-ic-prp.c | 6 +++++ > > drivers/staging/media/imx/imx-ic-prpencvf.c | 6 +++++ > > drivers/staging/media/imx/imx-media-csi.c | 6 +++++ > > drivers/staging/media/imx/imx-media-vdic.c | 6 +++++ > > drivers/staging/media/tegra-video/csi.c | 3 +++ > > include/uapi/linux/v4l2-subdev.h | 13 ++++++++-- > > 36 files changed, 198 insertions(+), 17 deletions(-) > > > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-client-cap.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-client-cap.rst > > index 20f12a1cc0f7..f168140ebd59 100644 > > --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-client-cap.rst > > +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-client-cap.rst > > @@ -71,6 +71,11 @@ is unknown to the kernel. > > of 'stream' fields (referring to the stream number) with various > > ioctls. If this is not set (which is the default), the 'stream' fields > > will be forced to 0 by the kernel. > > + * - ``V4L2_SUBDEV_CLIENT_CAP_WHICH_INTERVAL`` > > + - The client is aware of the :c:type:`v4l2_subdev_frame_interval` > > + ``which`` field. If this is not set (which is the default), the > > + ``which`` field is forced to ``V4L2_SUBDEV_FORMAT_ACTIVE`` by the > > + kernel. > > > > Return Value > > ============ > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.rst > > index 842f962d2aea..41e0e2c8ecc3 100644 > > --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.rst > > +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.rst > > @@ -58,8 +58,9 @@ struct > > contains the current frame interval as would be returned by a > > ``VIDIOC_SUBDEV_G_FRAME_INTERVAL`` call. > > > > -Calling ``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` on a subdev device node that has been > > -registered in read-only mode is not allowed. An error is returned and the errno > > +If the subdev device node has been registered in read-only mode, calls to > > +``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` are only valid if the ``which`` field is set > > +to ``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno > > variable is set to ``-EPERM``. > > > > Drivers must not return an error solely because the requested interval > > @@ -93,7 +94,11 @@ the same sub-device is not defined. > > - ``stream`` > > - Stream identifier. > > * - __u32 > > - - ``reserved``\ [8] > > + - ``which`` > > + - Active or try frame interval, from enum > > + :ref:`v4l2_subdev_format_whence <v4l2-subdev-format-whence>`. > > + * - __u32 > > + - ``reserved``\ [7] > > - Reserved for future extensions. Applications and drivers must set > > the array to zero. > > > > @@ -114,9 +119,9 @@ EBUSY > > EINVAL > > The struct > > :c:type:`v4l2_subdev_frame_interval` > > - ``pad`` references a non-existing pad, or the pad doesn't support > > - frame intervals. > > + ``pad`` references a non-existing pad, the ``which`` field references a > > + non-existing frame interval, or the pad doesn't support frame intervals. > > "the ``which`` field references a non-existing frame interval": that's a rather > vague sentence. I'm not sure I would call it vague, but it's certainly not very understandable. > I noticed it was probably copy-and-pasted (VIDIOC_SUBDEV_G_FMT has > a similar phrase), but it is not clear in that documentation either. Yes, that's where it came from. > I expect EINVAL if 'which' is set to something other than TRY or ACTIVE, or the > driver does not support TRY. Is that what you meant with "references a non-existing > frame interval"? That's what it means for all the subdev ioctls that have a 'which' field, yes. > The 'references a non-existing' phrase works for pads since pad is an index > into a pad array, but that's not the case for 'which', which is effectively an > enum, so there is no obvious indexing going on. > > I think a separate patch clarifying this EINVAL description for the relevant subdev > ioctls might be useful. I agree. I'll include a patch in the next version of the series. > In any case, since this just copies existing text it isn't a blocker. > > > EPERM > > The ``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` ioctl has been called on a read-only > > - subdevice. > > + subdevice and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``. [snip]
Hello, This patch series improves frame interval handling in the V4L2 subdev in-kernel and userspace APIs. Frame interval are exposed to userspace on pads and streams, but the frame interval handling is currently implemented through a v4l2_subdev video operation, without involving the subdev state. This makes frame intervals a second class citizen compared to formats and selection rectangles. Patch 1/4 starts by addressing the first issue, namely the frame interval operations being video ops. This requires touching all the drivers using frame intervals. Patch 2/4 then adds a 'which' field to the subdev frame interval userspace API, allowing frame intervals to be tried the same way formats and selection rectangles can. Again, the same drivers need to be touched to preserve their current behaviour. Patch 3/4 adds support for storing the frame interval in the subdev state, alongside the formats and selection rectangles, with similar accessors and helper functions. Finally, patch 4/4 demonstrates how this is used in drivers, with the thp7312 driver serving as an example. The series is based on Sakari's latest master branch ([1]). Given the large number of drivers that this series touches, I would like to get it merged in v6.8 without too much delay to avoid rebasing. [1] https://git.linuxtv.org/sailus/media_tree.git/log/ Laurent Pinchart (4): media: v4l2-subdev: Turn .[gs]_frame_interval into pad operations media: v4l2-subdev: Add which field to struct v4l2_subdev_frame_interval media: v4l2-subdev: Store frame interval in subdev state media: i2c: thp7312: Store frame interval in subdev state .../media/v4l/vidioc-subdev-g-client-cap.rst | 5 + .../v4l/vidioc-subdev-g-frame-interval.rst | 17 +- drivers/media/i2c/adv7180.c | 10 +- drivers/media/i2c/et8ek8/et8ek8_driver.c | 12 +- drivers/media/i2c/imx214.c | 12 +- drivers/media/i2c/imx274.c | 54 +++--- drivers/media/i2c/max9286.c | 20 ++- drivers/media/i2c/mt9m111.c | 20 ++- drivers/media/i2c/mt9m114.c | 20 ++- drivers/media/i2c/mt9v011.c | 24 +-- drivers/media/i2c/mt9v111.c | 22 ++- drivers/media/i2c/ov2680.c | 10 +- drivers/media/i2c/ov5640.c | 22 ++- drivers/media/i2c/ov5648.c | 62 +++---- drivers/media/i2c/ov5693.c | 10 +- drivers/media/i2c/ov6650.c | 22 ++- drivers/media/i2c/ov7251.c | 12 +- drivers/media/i2c/ov7670.c | 22 +-- drivers/media/i2c/ov772x.c | 20 ++- drivers/media/i2c/ov7740.c | 40 ++--- drivers/media/i2c/ov8865.c | 54 +++--- drivers/media/i2c/ov9650.c | 20 ++- drivers/media/i2c/s5c73m3/s5c73m3-core.c | 20 ++- drivers/media/i2c/s5k5baf.c | 26 ++- drivers/media/i2c/thp7312.c | 160 ++++++++++-------- drivers/media/i2c/tvp514x.c | 33 ++-- drivers/media/usb/em28xx/em28xx-video.c | 6 +- drivers/media/v4l2-core/v4l2-common.c | 8 +- drivers/media/v4l2-core/v4l2-subdev.c | 128 ++++++++++---- .../media/atomisp/i2c/atomisp-gc0310.c | 10 +- .../media/atomisp/i2c/atomisp-gc2235.c | 10 +- .../media/atomisp/i2c/atomisp-mt9m114.c | 10 +- .../media/atomisp/i2c/atomisp-ov2722.c | 10 +- .../staging/media/atomisp/pci/atomisp_cmd.c | 4 +- .../staging/media/atomisp/pci/atomisp_ioctl.c | 4 +- drivers/staging/media/imx/imx-ic-prp.c | 20 ++- drivers/staging/media/imx/imx-ic-prpencvf.c | 20 ++- drivers/staging/media/imx/imx-media-capture.c | 6 +- drivers/staging/media/imx/imx-media-csi.c | 20 ++- drivers/staging/media/imx/imx-media-vdic.c | 20 ++- drivers/staging/media/tegra-video/csi.c | 12 +- include/media/v4l2-common.h | 4 +- include/media/v4l2-subdev.h | 65 +++++-- include/uapi/linux/v4l2-subdev.h | 13 +- 44 files changed, 706 insertions(+), 413 deletions(-) base-commit: 543efaddeac0c7769c39d7aaa886e8b001acac76