* [PATCH] media: i2c: imx219: add support for enum frame interval
@ 2020-02-27 15:17 Eugen Hristev
2020-02-28 13:44 ` Dave Stevenson
0 siblings, 1 reply; 6+ messages in thread
From: Eugen Hristev @ 2020-02-27 15:17 UTC (permalink / raw)
To: dave.stevenson, andrey.konovalov, sakari.ailus, linux-media,
linux-kernel
Cc: Eugen Hristev
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)
Signed-off-by: Eugen Hristev <eugen.hristev@microchip.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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] media: i2c: imx219: add support for enum frame interval
2020-02-27 15:17 [PATCH] media: i2c: imx219: add support for enum frame interval Eugen Hristev
@ 2020-02-28 13:44 ` Dave Stevenson
2020-02-28 14:05 ` Eugen.Hristev
0 siblings, 1 reply; 6+ messages in thread
From: Dave Stevenson @ 2020-02-28 13:44 UTC (permalink / raw)
To: Eugen Hristev
Cc: Andrey Konovalov, Sakari Ailus, Linux Media Mailing List, linux-kernel
Hi Eugen.
On Thu, 27 Feb 2020 at 15:19, Eugen Hristev <eugen.hristev@microchip.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.
Dave
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: i2c: imx219: add support for enum frame interval
2020-02-28 13:44 ` Dave Stevenson
@ 2020-02-28 14:05 ` Eugen.Hristev
2020-02-28 14:16 ` Dave Stevenson
0 siblings, 1 reply; 6+ messages in thread
From: Eugen.Hristev @ 2020-02-28 14:05 UTC (permalink / raw)
To: dave.stevenson; +Cc: andrey.konovalov, sakari.ailus, linux-media, linux-kernel
On 28.02.2020 15:44, Dave Stevenson wrote:
> Hi Eugen.
>
> On Thu, 27 Feb 2020 at 15:19, Eugen Hristev <eugen.hristev@microchip.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 ?
Thanks for replying,
Eugen
>
> Dave
>
>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.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
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: i2c: imx219: add support for enum frame interval
2020-02-28 14:05 ` Eugen.Hristev
@ 2020-02-28 14:16 ` Dave Stevenson
2020-02-28 14:34 ` Eugen.Hristev
0 siblings, 1 reply; 6+ messages in thread
From: Dave Stevenson @ 2020-02-28 14:16 UTC (permalink / raw)
To: Eugen.Hristev
Cc: Andrey Konovalov, Sakari Ailus, Linux Media Mailing List, linux-kernel
On Fri, 28 Feb 2020 at 14:05, <Eugen.Hristev@microchip.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@microchip.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
:-/
> Thanks for replying,
>
> Eugen
>
> >
> > Dave
> >
> >> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.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
> >>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: i2c: imx219: add support for enum frame interval
2020-02-28 14:16 ` Dave Stevenson
@ 2020-02-28 14:34 ` Eugen.Hristev
2020-02-28 14:55 ` Dave Stevenson
0 siblings, 1 reply; 6+ messages in thread
From: Eugen.Hristev @ 2020-02-28 14:34 UTC (permalink / raw)
To: dave.stevenson; +Cc: andrey.konovalov, sakari.ailus, linux-media, linux-kernel
On 28.02.2020 16:16, Dave Stevenson wrote:
> On Fri, 28 Feb 2020 at 14:05, <Eugen.Hristev@microchip.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@microchip.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 ?
Thanks for explanations
>
>> Thanks for replying,
>>
>> Eugen
>>
>>>
>>> Dave
>>>
>>>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.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
>>>>
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: i2c: imx219: add support for enum frame interval
2020-02-28 14:34 ` Eugen.Hristev
@ 2020-02-28 14:55 ` Dave Stevenson
0 siblings, 0 replies; 6+ messages in thread
From: Dave Stevenson @ 2020-02-28 14:55 UTC (permalink / raw)
To: Eugen.Hristev
Cc: Andrey Konovalov, Sakari Ailus, Linux Media Mailing List, linux-kernel
On Fri, 28 Feb 2020 at 14:34, <Eugen.Hristev@microchip.com> wrote:
>
> On 28.02.2020 16:16, Dave Stevenson wrote:
> > On Fri, 28 Feb 2020 at 14:05, <Eugen.Hristev@microchip.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@microchip.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@microchip.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
> >>>>
> >>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-02-28 14:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-27 15:17 [PATCH] media: i2c: imx219: add support for enum frame interval Eugen Hristev
2020-02-28 13:44 ` Dave Stevenson
2020-02-28 14:05 ` Eugen.Hristev
2020-02-28 14:16 ` Dave Stevenson
2020-02-28 14:34 ` Eugen.Hristev
2020-02-28 14:55 ` Dave Stevenson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).