Re: [PATCH] media: v4l2-subdev.h: allow V4L2_FRMIVAL_TYPE_CONTINUOUS & _STEPWISE

[Hans Verkuil <hverkuil@xs4all.nl>]

Hi Philippe,

On 28/08/18 09:55, Philippe De Muyter wrote:
> add max_interval and step_interval to struct
> v4l2_subdev_frame_interval_enum.

Yeah, I never understood why this wasn't supported when this API was designed.
Clearly an oversight.

> 
> When filled correctly by the sensor driver, those fields must be
> used as follows by the intermediate level :
> 
>         struct v4l2_frmivalenum *fival;
>         struct v4l2_subdev_frame_interval_enum fie;
> 
>         if (fie.max_interval.numerator == 0) {
>                 fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
>                 fival->discrete = fie.interval;
>         } else if (fie.step_interval.numerator == 0) {
>                 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;
>         }

This is a bit too magical for my tastes. I'd add a type field:

#define V4L2_SUBDEV_FRMIVAL_TYPE_DISCRETE 0
#define V4L2_SUBDEV_FRMIVAL_TYPE_CONTINUOUS 1
#define V4L2_SUBDEV_FRMIVAL_TYPE_STEPWISE 2

Older applications that do not know about the type field will just
see a single discrete interval containing the minimum interval.
I guess that's OK as they will keep working.

While at it: it would be really nice if you can also add stepwise
support to VIDIOC_SUBDEV_ENUM_FRAME_SIZE. I think the only thing
you need to do there is to add two new fields: step_width and step_height.
If 0, then that just means a step size of 1.

Add some helper functions to translate between v4l2_subdev_frame_size/interval_enum
and v4l2_frmsize/ivalenum and this becomes much cleaner.

Regards,

	Hans

> 
> Signed-off-by: Philippe De Muyter <phdm@macqel.be>
> ---
>  .../uapi/v4l/vidioc-subdev-enum-frame-interval.rst | 39 +++++++++++++++++++++-
>  include/uapi/linux/v4l2-subdev.h                   |  4 ++-
>  2 files changed, 41 insertions(+), 2 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..acc516e 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,37 @@ 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 at the fixed set of frame intervals,
> +driver must enumerate them with increasing indexes, by only filling
> +the ``interval`` field.  If the sub-device can work with a continuous
> +range of frame intervals, driver must only return success for index 0
> +and fill ``interval`` with the minimum interval, ``max_interval`` with
> +the maximum interval, and ``step_interval`` with 0 or 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.max_interval.numerator == 0) {
> +                fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
> +                fival->discrete = fie.interval;
> +        } else if (fie.step_interval.numerator == 0) {
> +                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
> @@ -92,8 +123,14 @@ multiple pads of the same sub-device is not defined.
>        - ``which``
>        - Frame intervals to be enumerated, from enum
>  	:ref:`v4l2_subdev_format_whence <v4l2-subdev-format-whence>`.
> +    * - 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.
>      * - __u32
> -      - ``reserved``\ [8]
> +      - ``reserved``\ [4]
>        - Reserved for future extensions. Applications and drivers must set
>  	the array to zero.
>  
> diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
> index 03970ce..c944644 100644
> --- a/include/uapi/linux/v4l2-subdev.h
> +++ b/include/uapi/linux/v4l2-subdev.h
> @@ -128,7 +128,9 @@ struct v4l2_subdev_frame_interval_enum {
>  	__u32 height;
>  	struct v4l2_fract interval;
>  	__u32 which;
> -	__u32 reserved[8];
> +	struct v4l2_fract max_interval;
> +	struct v4l2_fract step_interval;
> +	__u32 reserved[4];
>  };
>  
>  /**
>