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

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

[Philippe De Muyter]

add max_interval and step_interval to struct
v4l2_subdev_frame_interval_enum.

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;
        }

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];
 };
 
 /**
-- 
1.8.4

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

[Hans Verkuil]

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];
>  };
>  
>  /**
> 

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

[Philippe De Muyter]

Hi Hans,

On Tue, Aug 28, 2018 at 12:03:25PM +0200, Hans Verkuil wrote:
> 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

Like that ?

	struct v4l2_subdev_frame_interval_enum {
		__u32 index;
		__u32 pad;
		__u32 code;
		__u32 width;
		__u32 height;
		struct v4l2_fract interval;
		__u32 which;
		__u32 type;
		struct v4l2_fract max_interval;
		struct v4l2_fract step_interval;
		__u32 reserved[3];
	};


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

That's actually what also happens with the above implementation, because
max_interval.numerator and step_interval.numerator were previously
reserved and thus 0, and if this code is moved to a helper function,
that does not matter if it's a little bit magical :).

Both implementations are equal to me, but the proposed one uses less
space from the 'reserved' field.

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

I'll look at that if I find enough interest and test opportunity for it,
but those things are unrelated except that they are missing :)

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

OK

> 
> 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];
> >  };
> >  
> >  /**
> > 

-- 
Philippe De Muyter +32 2 6101532 Macq SA rue de l'Aeronef 2 B-1140 Bruxelles

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

[Hans Verkuil]

On 28/08/18 12:26, Philippe De Muyter wrote:
> Hi Hans,
> 
> On Tue, Aug 28, 2018 at 12:03:25PM +0200, Hans Verkuil wrote:
>> 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
> 
> Like that ?
> 
> 	struct v4l2_subdev_frame_interval_enum {
> 		__u32 index;
> 		__u32 pad;
> 		__u32 code;
> 		__u32 width;
> 		__u32 height;
> 		struct v4l2_fract interval;
> 		__u32 which;
> 		__u32 type;
> 		struct v4l2_fract max_interval;
> 		struct v4l2_fract step_interval;
> 		__u32 reserved[3];
> 	};

Yes.

> 
> 
>>
>> 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.
> 
> That's actually what also happens with the above implementation, because
> max_interval.numerator and step_interval.numerator were previously
> reserved and thus 0, and if this code is moved to a helper function,
> that does not matter if it's a little bit magical :).

It's not just the kernel, but userspace can also enumerate directly from
a v4l-subdevX device node, and they can't use any kernel helper functions.

Let's keep this API clean.

> Both implementations are equal to me, but the proposed one uses less
> space from the 'reserved' field.

That's OK.

Regards,

	Hans


> 
>>
>> 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.
> 
> I'll look at that if I find enough interest and test opportunity for it,
> but those things are unrelated except that they are missing :)
> 
>>
>> Add some helper functions to translate between v4l2_subdev_frame_size/interval_enum
>> and v4l2_frmsize/ivalenum and this becomes much cleaner.
> 
> OK
> 
>>
>> 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];
>>>  };
>>>  
>>>  /**
>>>
> 

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

[Philippe De Muyter]

Hi Hans,

On Tue, Aug 28, 2018 at 12:29:21PM +0200, Hans Verkuil wrote:
> On 28/08/18 12:26, Philippe De Muyter wrote:
> > Hi Hans,
> > 
> > On Tue, Aug 28, 2018 at 12:03:25PM +0200, Hans Verkuil wrote:
> >> 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

Should I put that in an enum like 'enum v4l2_subdev_format_whence'
or use simple define's ?

Philippe

-- 
Philippe De Muyter +32 2 6101532 Macq SA rue de l'Aeronef 2 B-1140 Bruxelles

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

[Hans Verkuil]

On 28/08/18 12:40, Philippe De Muyter wrote:
> Hi Hans,
> 
> On Tue, Aug 28, 2018 at 12:29:21PM +0200, Hans Verkuil wrote:
>> On 28/08/18 12:26, Philippe De Muyter wrote:
>>> Hi Hans,
>>>
>>> On Tue, Aug 28, 2018 at 12:03:25PM +0200, Hans Verkuil wrote:
>>>> 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
> 
> Should I put that in an enum like 'enum v4l2_subdev_format_whence'
> or use simple define's ?

Use an enum for consistency with the existing framesize/ival APIs.

Regards,

	Hans

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

[Philippe De Muyter]

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

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>
---
v2:
	Add a 'type' field and a helper function, as asked by Hans

 .../uapi/v4l/vidioc-subdev-enum-frame-interval.rst | 46 +++++++++++++++++++++-
 drivers/media/v4l2-core/v4l2-common.c              | 32 +++++++++++++++
 include/media/v4l2-common.h                        | 12 ++++++
 include/uapi/linux/v4l2-subdev.h                   | 22 ++++++++++-
 4 files changed, 109 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..d3144b7 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,44 @@ 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 a fixed set of frame intervals,
+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, 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 mentionning 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
+
+Kernel users may use the ``v4l2_fill_frmivalenum_from_subdev`` helper
+function instead.
+
 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 +130,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/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index b062111..d652dd3 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -445,3 +445,35 @@ 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.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];
 };
 
 /**
-- 
1.8.4

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

[Hans Verkuil]

On 09/11/18 10:36, 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
> 
> 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>
> ---
> v2:
> 	Add a 'type' field and a helper function, as asked by Hans
> 
>  .../uapi/v4l/vidioc-subdev-enum-frame-interval.rst | 46 +++++++++++++++++++++-
>  drivers/media/v4l2-core/v4l2-common.c              | 32 +++++++++++++++
>  include/media/v4l2-common.h                        | 12 ++++++
>  include/uapi/linux/v4l2-subdev.h                   | 22 ++++++++++-
>  4 files changed, 109 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..d3144b7 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,44 @@ 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 a fixed set of frame intervals,

at -> with

> +driver must enumerate them with increasing indexes, by setting the

driver -> then the driver

> +``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, 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 mentionning the step in the

mentionning -> mentioning

> +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 :

No space before :

> +
> +.. code-block:: c
> +
> +        struct v4l2_frmivalenum * fival;

No space after *

> +        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
> +
> +Kernel users may use the ``v4l2_fill_frmivalenum_from_subdev`` helper
> +function instead.

Drop this line. This file documents the userspace API, so this is irrelevant.

> +
>  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 +130,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>`.

I'm missing the documentation for the new 'type' field here.

> +    * - 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]

4 -> 3

>        - Reserved for future extensions. Applications and drivers must set
>  	the array to zero.
>  
> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index b062111..d652dd3 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -445,3 +445,35 @@ 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.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);
> +

It would be very nice if you can use this helper in a driver as well.
We prefer to see helpers being used somewhere.

>  #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];
>  };
>  
>  /**
> 

This is shaping up nicely!

Regards,

	Hans