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

[Eugen.Hristev]

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 ?

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