Hi Daniel,
Thanks for the patchset.
I'm also cc'ing Laurent who I think could be interested in this one.
On Tue, Apr 14, 2020 at 10:01:49PM +0200, Daniel Gomez wrote:
> Add min and max structures to the v4l2-subdev callback in order to allow
> the subdev to return a range of valid frame intervals.
>
> This would operate similar to the struct v4l2_subdev_frame_size_enum and
> its max and min values for the width and the height. In this case, the
> possibility to return a frame interval range is added to the v4l2-subdev level
> whenever the v4l2 device operates in step-wise or continuous mode.
The current API only allows providing a list of enumerated values. That is
limiting indeed, especially on register list based sensor drivers where
vertical blanking is configurable.
I guess this could be extended to cover what V4L2, more or less. If we tell
it's a range, is it assumed to be contiguous? We don't have try operation
for the frame interval, but I guess set is good enough. The fraction is
probably best for TV standards but it's not what camera sensors natively
use. (But for a register list based driver, the established practice
remains to use frame interval.)
I'm also wondering the effect on existing user space; if a driver gives a
range, how will the existing programs work with such a driver?
I'd add an anonymous union with the interval field, the other field being
min_interval. Then the current applications would get the minimum interval
and still continue to function. I guess compilers are modern enough these
days we can have an anonymous union in the uAPI?
>
> Signed-off-by: Daniel Gomez <daniel@xxxxxxxx>
> ---
> include/uapi/linux/v4l2-subdev.h | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
> index 03970ce30741..ee15393c58cd 100644
> --- a/include/uapi/linux/v4l2-subdev.h
> +++ b/include/uapi/linux/v4l2-subdev.h
> @@ -117,6 +117,8 @@ struct v4l2_subdev_frame_interval {
> * @code: format code (MEDIA_BUS_FMT_ definitions)
> * @width: frame width in pixels
> * @height: frame height in pixels
> + * @min_interval: min frame interval in seconds
> + * @max_interval: max frame interval in seconds
> * @interval: frame interval in seconds
> * @which: format type (from enum v4l2_subdev_format_whence)
> */
> @@ -126,9 +128,11 @@ struct v4l2_subdev_frame_interval_enum {
> __u32 code;
> __u32 width;
> __u32 height;
> + struct v4l2_fract min_interval;
> + struct v4l2_fract max_interval;
This changes the memory layout of the struct and breaks the ABI. Any new
fields in the struct may only replace reserved fields while the rest must
stay unchanged.
> struct v4l2_fract interval;
> __u32 which;
> - __u32 reserved[8];
> + __u32 reserved[4];
> };
>
> /**
--
Regards,
Sakari Ailus
