Re: [PATCH] media: i2c: imx219: add support for enum frame interval

[Dave Stevenson]

On Fri, 28 Feb 2020 at 14:05, <Eugen.Hristev@xxxxxxxxxxxxx> wrote:
>
> On 28.02.2020 15:44, Dave Stevenson wrote:
> > Hi Eugen.
> >
> > On Thu, 27 Feb 2020 at 15:19, Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx> 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
:-/

> Thanks for replying,
>
> Eugen
>
> >
> >    Dave
> >
> >> Signed-off-by: Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx>
> >> ---
> >>
> >> 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
> >>
>