| 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
| ||
|
Message-ID: <CAPY8ntDmQuzqWfkq_4+Jvc8BFzQh+XnVMt0ZyNBbzmk4GEPnfg@mail.gmail.com>
Date: Fri, 28 Feb 2020 14:55:25 +0000
From: Dave Stevenson <dave.stevenson@...pberrypi.com>
To: Eugen.Hristev@...rochip.com
Cc: Andrey Konovalov <andrey.konovalov@...aro.org>,
Sakari Ailus <sakari.ailus@....fi>,
Linux Media Mailing List <linux-media@...r.kernel.org>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] media: i2c: imx219: add support for enum frame interval
On Fri, 28 Feb 2020 at 14:34, <Eugen.Hristev@...rochip.com> wrote:
>
> On 28.02.2020 16:16, Dave Stevenson wrote:
> > On Fri, 28 Feb 2020 at 14:05, <Eugen.Hristev@...rochip.com> wrote:
> >>
> >> On 28.02.2020 15:44, Dave Stevenson wrote:
> >>> Hi Eugen.
> >>>
> >>> On Thu, 27 Feb 2020 at 15:19, Eugen Hristev <eugen.hristev@...rochip.com> wrote:
> >>>>
> >>>> Add support for enum frame intervals IOCTL.
> >>>> The current supported framerates are only available as comments inside
> >>>> the code.
> >>>> Add support for VIDIOC_ENUM_FRAMEINTERVALS as the enum_frame_interval
> >>>> callback as pad ops.
> >>>>
> >>>> # v4l2-ctl --list-frameintervals width=1920,height=1080,pixelformat=RG10
> >>>> ioctl: VIDIOC_ENUM_FRAMEINTERVALS
> >>>> Interval: Discrete 0.067s (15.000 fps)
> >>>> Interval: Discrete 0.033s (30.000 fps)
> >>>> Interval: Discrete 0.033s (30.000 fps)
> >>>
> >>> But the frame rates are not discrete. You have frame rate control via
> >>> V4L2_CID_VBLANK, which can be used in conjunction with V4L2_CID_HBLANK
> >>> and V4L2_CID_PIXEL_RATE to determine actual frame period.
> >>>
> >>> See https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/ext-ctrls-image-source.html?highlight=v4l2_cid_vblank
> >>> I believe this is the preferred route to doing frame rate control on
> >>> image sensors. I assume someone will correct me if I'm wrong on that.
> >>
> >>
> >> Okay... , I was guided towards this by the comments in the code, saying
> >> that the three supported modes are at a constant frame per second...
> >>
> >> Those comments are wrong then ?
> >
> > Yes, the comments for each of the modes (eg "/* 8MPix 15fps mode */")
> > probably shouldn't have the frame rate in them. I don't see any other
> > references. Those frame rates are the defaults only, as set via eg
> > IMX219_VTS_15FPS.
> >
> > I originally wrote the driver without frame rate control, and the
> > comments obviously didn't get updated when VTS/HTS support was added
> > :-/
>
> So in my understanding, actually, to get actual frame rate, we should
> use another control, and the enum frame intervals is actually impossible
> to use, since the sensor supports a multitude of frame rates, and it
> would be egregious to enumerate them all.
>
> Is this the case ?
>
> My idea was that v4l2-ctl --list-formats-ext will not show anything
> related to frame rate. So using this command, will only get the
> resolution, but don't know what to expect in terms of frame rate.
> Wouldn't be useful to implement this 'default' frame rate in this IOCTL ?
I'll defer to Sakari or others on the original reasoning behind it. I
was basing imx219 on other recently merged Bayer sensor drivers.
Having individual control of HTS and VTS allows slightly finer control
of exactly how the sensor frames. It also matches with exposure time
being set in lines, therefore you need to know the time per horizontal
line to convert from units of time.
If the driver were implementing ENUM_FRAME_INTERVALS / [S|G]_PARM then
you'd still need to support HTS/VTS for exposure, so why duplicate
settings?
> Thanks for explanations
> >
> >> Thanks for replying,
> >>
> >> Eugen
> >>
> >>>
> >>> Dave
> >>>
> >>>> Signed-off-by: Eugen Hristev <eugen.hristev@...rochip.com>
> >>>> ---
> >>>>
> >>>> Hello,
> >>>>
> >>>> This is on top of Sakari's tree in linuxtv.org
> >>>>
> >>>> Thanks
> >>>> Eugen
> >>>>
> >>>> drivers/media/i2c/imx219.c | 27 +++++++++++++++++++++++++++
> >>>> 1 file changed, 27 insertions(+)
> >>>>
> >>>> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> >>>> index f1effb5a5f66..17fcedd4edb6 100644
> >>>> --- a/drivers/media/i2c/imx219.c
> >>>> +++ b/drivers/media/i2c/imx219.c
> >>>> @@ -127,6 +127,8 @@ struct imx219_mode {
> >>>> unsigned int width;
> >>>> /* Frame height */
> >>>> unsigned int height;
> >>>> + /* Frame rate */
> >>>> + u8 fps;
> >>>>
> >>>> /* V-timing */
> >>>> unsigned int vts_def;
> >>>> @@ -381,6 +383,7 @@ static const struct imx219_mode supported_modes[] = {
> >>>> /* 8MPix 15fps mode */
> >>>> .width = 3280,
> >>>> .height = 2464,
> >>>> + .fps = 15,
> >>>> .vts_def = IMX219_VTS_15FPS,
> >>>> .reg_list = {
> >>>> .num_of_regs = ARRAY_SIZE(mode_3280x2464_regs),
> >>>> @@ -391,6 +394,7 @@ static const struct imx219_mode supported_modes[] = {
> >>>> /* 1080P 30fps cropped */
> >>>> .width = 1920,
> >>>> .height = 1080,
> >>>> + .fps = 30,
> >>>> .vts_def = IMX219_VTS_30FPS_1080P,
> >>>> .reg_list = {
> >>>> .num_of_regs = ARRAY_SIZE(mode_1920_1080_regs),
> >>>> @@ -401,6 +405,7 @@ static const struct imx219_mode supported_modes[] = {
> >>>> /* 2x2 binned 30fps mode */
> >>>> .width = 1640,
> >>>> .height = 1232,
> >>>> + .fps = 30,
> >>>> .vts_def = IMX219_VTS_30FPS_BINNED,
> >>>> .reg_list = {
> >>>> .num_of_regs = ARRAY_SIZE(mode_1640_1232_regs),
> >>>> @@ -680,6 +685,27 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
> >>>> return 0;
> >>>> }
> >>>>
> >>>> +static int imx219_enum_frame_interval(struct v4l2_subdev *sd,
> >>>> + struct v4l2_subdev_pad_config *cfg,
> >>>> + struct v4l2_subdev_frame_interval_enum *fie)
> >>>> +{
> >>>> + struct imx219 *imx219 = to_imx219(sd);
> >>>> +
> >>>> + if (fie->index >= ARRAY_SIZE(supported_modes))
> >>>> + return -EINVAL;
> >>>> +
> >>>> + if (fie->code != imx219_get_format_code(imx219))
> >>>> + return -EINVAL;
> >>>> +
> >>>> + if (fie->pad)
> >>>> + return -EINVAL;
> >>>> +
> >>>> + fie->interval.numerator = 1;
> >>>> + fie->interval.denominator = supported_modes[fie->index].fps;
> >>>> +
> >>>> + return 0;
> >>>> +}
> >>>> +
> >>>> static void imx219_reset_colorspace(struct v4l2_mbus_framefmt *fmt)
> >>>> {
> >>>> fmt->colorspace = V4L2_COLORSPACE_SRGB;
> >>>> @@ -1004,6 +1030,7 @@ static const struct v4l2_subdev_pad_ops imx219_pad_ops = {
> >>>> .get_fmt = imx219_get_pad_format,
> >>>> .set_fmt = imx219_set_pad_format,
> >>>> .enum_frame_size = imx219_enum_frame_size,
> >>>> + .enum_frame_interval = imx219_enum_frame_interval,
> >>>> };
> >>>>
> >>>> static const struct v4l2_subdev_ops imx219_subdev_ops = {
> >>>> --
> >>>> 2.20.1
> >>>>
> >>
>
Powered by blists - more mailing lists