Re: [PATCH v3 1/2] media: v4l2-subdev.h: allow V4L2_FRMIVAL_TYPE_CONTINUOUS & _STEPWISE

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Philippe,

Thank you for the patch.

On Tuesday, 11 September 2018 20:06:32 EEST Philippe De Muyter wrote:
> add V4L2_FRMIVAL_TYPE_CONTINUOUS and V4L2_FRMIVAL_TYPE_STEPWISE for
> subdev's frame intervals in addition to implicit existing
> V4L2_FRMIVAL_TYPE_DISCRETE type.  This needs three new fields in the
> v4l2_subdev_frame_interval_enum struct :
> - type
> - max_interval
> - step_interval
> 
> A helper function 'v4l2_fill_frmivalenum_from_subdev' is also added.
> 
> Subdevs must fill the 'type' field.  If they do not, the default
> value (0) is used which is equal to V4L2_SUBDEV_FRMIVAL_TYPE_DISCRETE.
> 
> if type is set to V4L2_SUBDEV_FRMIVAL_TYPE_DISCRETE, or left untouched,
> only the 'interval' field must be filled, just as before.
> 
> If type is set to V4L2_FRMIVAL_TYPE_CONTINUOUS, 'interval' must be set
> to the minimum frame interval (highest framerate), and 'max_interval'
> to the maximum frame interval.
> 
> If type is set to V4L2_FRMIVAL_TYPE_STEPWISE, 'step_interval' must be
> set to the step between available intervals, in addition to 'interval'
> and 'max_interval' which must be set as for V4L2_FRMIVAL_TYPE_CONTINUOUS

Continuous is a special case of stepwise with the step set to 1. Should we 
merge the two types ?

I'm curious, as there's nothing in this series, using the new types, what are 
your use cases ?

> Old users which do not check the 'type' field will get the minimum frame
> interval (highest framrate) just like before.
> 
> Callers who intend to check the 'type' field should zero it themselves,
> in case an old subdev driver does not do zero it.
> 
> When filled correctly by the sensor driver, the new fields must be
> used as follows by the caller :
> 
> 	 struct v4l2_frmivalenum * fival;
> 	 struct v4l2_subdev_frame_interval_enum fie;
> 
> 	 if (fie.type == V4L2_SUBDEV_FRMIVAL_TYPE_DISCRETE) {
> 		 fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
> 		 fival->discrete = fie.interval;
> 	 } else if (fie.type == V4L2_SUBDEV_FRMIVAL_TYPE_CONTINUOUS) {
> 		 fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
> 		 fival->stepwise.min = fie.interval;
> 		 fival->stepwise.max = fie.max_interval;
> 	 } else {
> 		 fival->type = V4L2_FRMIVAL_TYPE_STEPWISE;
> 		 fival->stepwise.min = fie.interval;
> 		 fival->stepwise.max = fie.max_interval;
> 		 fival->stepwise.step = fie.step_interval;
> 	 }
> 
> Kernel users should use the new 'v4l2_fill_frmivalenum_from_subdev'
> helper function.
> 
> Signed-off-by: Philippe De Muyter <phdm@macqel.be>
> ---
>  .../uapi/v4l/vidioc-subdev-enum-frame-interval.rst | 69 ++++++++++++++++++-
>  drivers/media/v4l2-core/v4l2-common.c              | 33 +++++++++++
>  include/media/v4l2-common.h                        | 12 ++++
>  include/uapi/linux/v4l2-subdev.h                   | 22 ++++++-
>  4 files changed, 133 insertions(+), 3 deletions(-)
> 
> diff --git
> a/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-interval.rst
> b/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-interval.rst index
> 1bfe386..e14fa14 100644
> --- a/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-interval.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-interval.rst
> @@ -51,6 +51,41 @@ EINVAL error code if one of the input fields is invalid.
> All frame intervals are enumerable by beginning at index zero and
> incrementing by one until ``EINVAL`` is returned.
> 
> +If the sub-device can work only with a fixed set of frame intervals, then
> +the driver must enumerate them with increasing indexes, by setting the
> +``type`` field to ``V4L2_SUBDEV_FRMIVAL_TYPE_DISCRETE`` and only filling
> +the ``interval`` field .  If the sub-device can work with a continuous
> +range of frame intervals, then the driver must only return success for
> +index 0, set the ``type`` field to ``V4L2_SUBDEV_FRMIVAL_TYPE_CONTINUOUS``,
> +fill ``interval`` with the minimum interval and ``max_interval`` with
> +the maximum interval.  If it is worth mentioning the step in the
> +continuous interval, the driver must set the ``type`` field to
> +``V4L2_SUBDEV_FRMIVAL_TYPE_STEPWISE`` and fill also the ``step_interval``
> +field with the step between the possible intervals.
> +
> +Callers are expected to use the returned information as follows:
> +
> +.. code-block:: c
> +
> +        struct v4l2_frmivalenum *fival;
> +        struct v4l2_subdev_frame_interval_enum fie;
> +
> +        if (fie.type == V4L2_SUBDEV_FRMIVAL_TYPE_DISCRETE) {
> +                fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
> +                fival->discrete = fie.interval;
> +        } else if (fie.type == V4L2_SUBDEV_FRMIVAL_TYPE_CONTINUOUS) {
> +                fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
> +                fival->stepwise.min = fie.interval;
> +                fival->stepwise.max = fie.max_interval;
> +        } else {
> +                fival->type = V4L2_FRMIVAL_TYPE_STEPWISE;
> +                fival->stepwise.min = fie.interval;
> +                fival->stepwise.max = fie.max_interval;
> +                fival->stepwise.step = fie.step_interval;
> +        }
> +
> +.. code-block:: c
> +
>  Available frame intervals may depend on the current 'try' formats at
>  other pads of the sub-device, as well as on the current active links.
>  See :ref:`VIDIOC_SUBDEV_G_FMT` for more
> @@ -93,11 +128,43 @@ multiple pads of the same sub-device is not defined.
>        - Frame intervals to be enumerated, from enum
> 
>  	:ref:`v4l2_subdev_format_whence <v4l2-subdev-format-whence>`.
> 
>      * - __u32
> -      - ``reserved``\ [8]
> +      - ``type``
> +      - Type of enumerated interval
> +	:ref:`v4l2_subdev_frmival_type <v4l2-subdev-frmival-type>`.
> +    * - struct :c:type:`v4l2_fract`
> +      - ``max_interval``
> +      - Maximum period, in seconds, between consecutive video frames, or 0.
> +    * - struct :c:type:`v4l2_fract`
> +      - ``step_interval``
> +      - Frame interval step size, in seconds, or 0.

Having minimum and maximum values as fractions and a step as a fraction as 
well will make the maths pretty fun... I think it was a mistake to go with 
fractions in the first place, as it gives many ways to represent the same 
value. It lead to gems such as https://elixir.bootlin.com/linux/latest/source/
drivers/media/usb/uvc/uvc_driver.c#L263 (which I'm both proud and ashamed of).

Instead of repeating the mistake, is there a chance we could fix it and 
express intervals as an integer (in nanoseconds for instance, or possibly 10s 
or 100s of nanoseconds) for these new types ? Ideally I'd move to the same 
unit for the discrete type as well but we can't change the API there. We could 
deprecate it though (given that I expect very few users of that subdev 
userspace API) and add a new type for discrete intervals using integers. 
Subdev drivers would then implement the new API only, with the conversion 
performed in core code.

> +    * - __u32
> +      - ``reserved``\ [3]
>        - Reserved for future extensions. Applications and drivers must set
>  	the array to zero.
> 
> 
> +
> +.. tabularcolumns:: |p{6.6cm}|p{2.2cm}|p{8.7cm}|
> +
> +.. _v4l2-subdev-frmival-type:
> +
> +.. flat-table:: enum v4l2_subdev_format_whence
> +    :header-rows:  0
> +    :stub-columns: 0
> +    :widths:       3 1 4
> +
> +    * - V4L2_SUBDEV_FRMIVAL_TYPE_DISCRETE
> +      - 0
> +      - This frame interval is fixed
> +    * - V4L2_SUBDEV_FRMIVAL_TYPE_CONTINUOUS
> +      - 1
> +      - Any frame interval between min and max is available
> +    * - V4L2_SUBDEV_FRMIVAL_TYPE_STEPWISE
> +      - 2
> +      - Many frame intervals between min and max are available, with a
> +        significant and constant step between them.
> +
> +
>  Return Value
>  ============
> 
> diff --git a/drivers/media/v4l2-core/v4l2-common.c
> b/drivers/media/v4l2-core/v4l2-common.c index b062111..ec9b748 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -445,3 +445,36 @@ int v4l2_s_parm_cap(struct video_device *vdev,
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(v4l2_s_parm_cap);
> +
> +int v4l2_fill_frmivalenum_from_subdev(struct v4l2_subdev *sd, struct
> v4l2_frmivalenum *fival, int code) +{
> +	struct v4l2_subdev_frame_interval_enum fie;
> +	int ret;
> +
> +	fie.index = fival->index;
> +	fie.code = code;
> +	fie.width = fival->width;
> +	fie.height = fival->height;
> +	fie.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> +
> +	fie.type = V4L2_SUBDEV_FRMIVAL_TYPE_DISCRETE; /* for old subdev drivers */
> +	ret = v4l2_subdev_call(sd, pad, enum_frame_interval, NULL, &fie); +
> +	if (!ret) {
> +		if (fie.type == V4L2_SUBDEV_FRMIVAL_TYPE_DISCRETE) {
> +			fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
> +			fival->discrete = fie.interval;
> +		} else if (fie.type == V4L2_SUBDEV_FRMIVAL_TYPE_CONTINUOUS) {
> +			fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
> +			fival->stepwise.min = fie.interval;
> +			fival->stepwise.max = fie.max_interval;
> +		} else {
> +			fival->type = V4L2_FRMIVAL_TYPE_STEPWISE;
> +			fival->stepwise.min = fie.interval;
> +			fival->stepwise.max = fie.max_interval;
> +			fival->stepwise.step = fie.step_interval;
> +		}
> +	}
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_fill_frmivalenum_from_subdev);
> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> index cdc87ec..3c62403 100644
> --- a/include/media/v4l2-common.h
> +++ b/include/media/v4l2-common.h
> @@ -384,4 +384,16 @@ int v4l2_g_parm_cap(struct video_device *vdev,
>  int v4l2_s_parm_cap(struct video_device *vdev,
>  		    struct v4l2_subdev *sd, struct v4l2_streamparm *a);
> 
> +/**
> + * v4l2_fill_frmivalenum_from_subdev - helper for
> vidioc_enum_frameintervals + *      calling the enum_frame_interval op of
> the given subdev.
> + *
> + * @sd: the sub-device pointer.
> + * @fival: the VIDIOC_ENUM_FRAMEINTERVALS argument.
> + * @code: the MEDIA_BUS_FMT_ code (not fival->pixel_format !)
> + */
> +int v4l2_fill_frmivalenum_from_subdev(struct v4l2_subdev *sd,
> +	       			      struct v4l2_frmivalenum *fival,
> +				      int code);
> +
>  #endif /* V4L2_COMMON_H_ */
> diff --git a/include/uapi/linux/v4l2-subdev.h
> b/include/uapi/linux/v4l2-subdev.h index 03970ce..3faae35 100644
> --- a/include/uapi/linux/v4l2-subdev.h
> +++ b/include/uapi/linux/v4l2-subdev.h
> @@ -100,6 +100,16 @@ struct v4l2_subdev_frame_size_enum {
>  };
> 
>  /**
> + * enum v4l2_subdev_frmival_type - Frame interval type
> + */
> +enum v4l2_subdev_frmival_type {
> +	V4L2_SUBDEV_FRMIVAL_TYPE_DISCRETE = 0,
> +	V4L2_SUBDEV_FRMIVAL_TYPE_CONTINUOUS = 1,
> +	V4L2_SUBDEV_FRMIVAL_TYPE_STEPWISE = 2,
> +};
> +
> +
> +/**
>   * struct v4l2_subdev_frame_interval - Pad-level frame rate
>   * @pad: pad number, as reported by the media API
>   * @interval: frame interval in seconds
> @@ -117,8 +127,13 @@ struct v4l2_subdev_frame_interval {
>   * @code: format code (MEDIA_BUS_FMT_ definitions)
>   * @width: frame width in pixels
>   * @height: frame height in pixels
> - * @interval: frame interval in seconds
> + * @interval: minimum frame interval in seconds
>   * @which: format type (from enum v4l2_subdev_format_whence)
> + * @type: frame interval type (from enum v4l2_subdev_frmival_type)
> + * @max_interval: maximum frame interval in seconds,
> + *                if type != V4L2_SUBDEV_FRMIVAL_TYPE_DISCRETE
> + * @step_interval: step between frame intervals, in seconds,
> + *                 if type == V4L2_SUBDEV_FRMIVAL_TYPE_STEPWISE
>   */
>  struct v4l2_subdev_frame_interval_enum {
>  	__u32 index;
> @@ -128,7 +143,10 @@ struct v4l2_subdev_frame_interval_enum {
>  	__u32 height;
>  	struct v4l2_fract interval;
>  	__u32 which;
> -	__u32 reserved[8];
> +	__u32 type;
> +	struct v4l2_fract max_interval;
> +	struct v4l2_fract step_interval;
> +	__u32 reserved[3];
>  };
> 
>  /**

-- 
Regards,

Laurent Pinchart