linux-kernel - Re: [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200430094233.GE867@valkosipuli.retiisi.org.uk>
Date:   Thu, 30 Apr 2020 12:42:33 +0300
From:   Sakari Ailus <sakari.ailus@....fi>
To:     Daniel Gomez <daniel@...c.com>
Cc:     mchehab@...nel.org, hverkuil-cisco@...all.nl,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>
Subject: Re: [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum

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@...c.com>
> ---
>  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