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@xxxxxxxxxxxxxxxx> > > --- > > Changes since v1: > > > > - Fix .[gs]et_frame_interval() operation names in commit message > > - Fix typo in commit message > > --- > > .../media/v4l/vidioc-subdev-g-client-cap.rst | 5 ++++ > > .../v4l/vidioc-subdev-g-frame-interval.rst | 17 ++++++++----- > > drivers/media/i2c/adv7180.c | 3 +++ > > drivers/media/i2c/et8ek8/et8ek8_driver.c | 6 +++++ > > drivers/media/i2c/imx214.c | 3 +++ > > drivers/media/i2c/imx274.c | 6 +++++ > > drivers/media/i2c/max9286.c | 6 +++++ > > drivers/media/i2c/mt9m111.c | 6 +++++ > > drivers/media/i2c/mt9m114.c | 6 +++++ > > drivers/media/i2c/mt9v011.c | 6 +++++ > > drivers/media/i2c/mt9v111.c | 6 +++++ > > drivers/media/i2c/ov2680.c | 3 +++ > > drivers/media/i2c/ov5640.c | 6 +++++ > > drivers/media/i2c/ov5648.c | 3 +++ > > drivers/media/i2c/ov5693.c | 3 +++ > > drivers/media/i2c/ov6650.c | 6 +++++ > > drivers/media/i2c/ov7251.c | 6 +++++ > > drivers/media/i2c/ov7670.c | 4 +++ > > drivers/media/i2c/ov772x.c | 6 +++++ > > drivers/media/i2c/ov8865.c | 3 +++ > > drivers/media/i2c/ov9650.c | 6 +++++ > > drivers/media/i2c/s5c73m3/s5c73m3-core.c | 6 +++++ > > drivers/media/i2c/s5k5baf.c | 6 +++++ > > drivers/media/i2c/thp7312.c | 6 +++++ > > drivers/media/i2c/tvp514x.c | 4 +++ > > drivers/media/v4l2-core/v4l2-subdev.c | 25 ++++++++++++------- > > .../media/atomisp/i2c/atomisp-gc0310.c | 3 +++ > > .../media/atomisp/i2c/atomisp-gc2235.c | 3 +++ > > .../media/atomisp/i2c/atomisp-mt9m114.c | 3 +++ > > .../media/atomisp/i2c/atomisp-ov2722.c | 3 +++ > > drivers/staging/media/imx/imx-ic-prp.c | 6 +++++ > > drivers/staging/media/imx/imx-ic-prpencvf.c | 6 +++++ > > drivers/staging/media/imx/imx-media-csi.c | 6 +++++ > > drivers/staging/media/imx/imx-media-vdic.c | 6 +++++ > > drivers/staging/media/tegra-video/csi.c | 3 +++ > > include/uapi/linux/v4l2-subdev.h | 13 ++++++++-- > > 36 files changed, 198 insertions(+), 17 deletions(-) > > > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-client-cap.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-client-cap.rst > > index 20f12a1cc0f7..f168140ebd59 100644 > > --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-client-cap.rst > > +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-client-cap.rst > > @@ -71,6 +71,11 @@ is unknown to the kernel. > > of 'stream' fields (referring to the stream number) with various > > ioctls. If this is not set (which is the default), the 'stream' fields > > will be forced to 0 by the kernel. > > + * - ``V4L2_SUBDEV_CLIENT_CAP_WHICH_INTERVAL`` > > + - The client is aware of the :c:type:`v4l2_subdev_frame_interval` > > + ``which`` field. If this is not set (which is the default), the > > + ``which`` field is forced to ``V4L2_SUBDEV_FORMAT_ACTIVE`` by the > > + kernel. > > > > Return Value > > ============ > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.rst > > index 842f962d2aea..41e0e2c8ecc3 100644 > > --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.rst > > +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.rst > > @@ -58,8 +58,9 @@ struct > > contains the current frame interval as would be returned by a > > ``VIDIOC_SUBDEV_G_FRAME_INTERVAL`` call. > > > > -Calling ``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` on a subdev device node that has been > > -registered in read-only mode is not allowed. An error is returned and the errno > > +If the subdev device node has been registered in read-only mode, calls to > > +``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` are only valid if the ``which`` field is set > > +to ``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno > > variable is set to ``-EPERM``. > > > > Drivers must not return an error solely because the requested interval > > @@ -93,7 +94,11 @@ the same sub-device is not defined. > > - ``stream`` > > - Stream identifier. > > * - __u32 > > - - ``reserved``\ [8] > > + - ``which`` > > + - Active or try frame interval, from enum > > + :ref:`v4l2_subdev_format_whence <v4l2-subdev-format-whence>`. > > + * - __u32 > > + - ``reserved``\ [7] > > - Reserved for future extensions. Applications and drivers must set > > the array to zero. > > > > @@ -114,9 +119,9 @@ EBUSY > > EINVAL > > The struct > > :c:type:`v4l2_subdev_frame_interval` > > - ``pad`` references a non-existing pad, or the pad doesn't support > > - frame intervals. > > + ``pad`` references a non-existing pad, the ``which`` field references a > > + non-existing frame interval, or the pad doesn't support frame intervals. > > "the ``which`` field references a non-existing frame interval": that's a rather > vague sentence. I'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] -- Regards, Laurent Pinchart
