| Message ID | 20211101001119.46056-10-djrscally@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Extensions to ov8865 driver | expand |
Hi Daniel, On Mon, Nov 01, 2021 at 12:11:12AM +0000, Daniel Scally wrote: > The HTS values for some of the modes in the ov8865 driver are a bit > unusual, coming in lower than the output_size_x is set to. It seems > like they might be calculated to fit the desired framerate into a > configuration with just two data lanes. To bring this more in line > with expected behaviour, raise the HTS values above the output_size_x. This isn't necessarily a problem as binning may be done in analogue domain. I don't know about this sensor though. Is the frame interval still as expected in binned modes after this patch? > > The corollary of that change is that the hardcoded frame intervals > against the modes no longer make sense, so remove those entirely. > Update the .g/s_frame_interval() callbacks to calculate the frame > interval based on the current mode and the vblank and hblank settings. > > The implementation of the .enum_frame_interval() callback is no longer > suitable since the possible frame rate is now a continuous range depending > on the vblank control setting, so remove that callback entirely. > > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > drivers/media/i2c/ov8865.c | 65 +++++++------------------------------- > 1 file changed, 11 insertions(+), 54 deletions(-) > > diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c > index 4b18cc80f985..1b8674152750 100644 > --- a/drivers/media/i2c/ov8865.c > +++ b/drivers/media/i2c/ov8865.c > @@ -659,8 +659,6 @@ struct ov8865_mode { > unsigned int blc_anchor_right_start; > unsigned int blc_anchor_right_end; > > - struct v4l2_fract frame_interval; > - > bool pll2_binning; > > const struct ov8865_register_value *register_values; > @@ -964,7 +962,7 @@ static const struct ov8865_mode ov8865_modes[] = { > { > /* Horizontal */ > .output_size_x = 3264, > - .hts = 1944, > + .hts = 3888, > > /* Vertical */ > .output_size_y = 2448, > @@ -1003,9 +1001,6 @@ static const struct ov8865_mode ov8865_modes[] = { > .blc_anchor_right_start = 1984, > .blc_anchor_right_end = 2239, > > - /* Frame Interval */ > - .frame_interval = { 1, 30 }, > - > /* PLL */ > .pll2_binning = false, > > @@ -1018,11 +1013,11 @@ static const struct ov8865_mode ov8865_modes[] = { > { > /* Horizontal */ > .output_size_x = 3264, > - .hts = 2582, > + .hts = 3888, > > /* Vertical */ > .output_size_y = 1836, > - .vts = 2002, > + .vts = 2470, > > .size_auto = true, > .size_auto_boundary_x = 8, > @@ -1057,9 +1052,6 @@ static const struct ov8865_mode ov8865_modes[] = { > .blc_anchor_right_start = 1984, > .blc_anchor_right_end = 2239, > > - /* Frame Interval */ > - .frame_interval = { 1, 30 }, > - > /* PLL */ > .pll2_binning = false, > > @@ -1115,9 +1107,6 @@ static const struct ov8865_mode ov8865_modes[] = { > .blc_anchor_right_start = 992, > .blc_anchor_right_end = 1119, > > - /* Frame Interval */ > - .frame_interval = { 1, 30 }, > - > /* PLL */ > .pll2_binning = true, > > @@ -1179,9 +1168,6 @@ static const struct ov8865_mode ov8865_modes[] = { > .blc_anchor_right_start = 992, > .blc_anchor_right_end = 1119, > > - /* Frame Interval */ > - .frame_interval = { 1, 90 }, > - > /* PLL */ > .pll2_binning = true, > > @@ -2628,11 +2614,18 @@ static int ov8865_g_frame_interval(struct v4l2_subdev *subdev, > { > struct ov8865_sensor *sensor = ov8865_subdev_sensor(subdev); > const struct ov8865_mode *mode; > + unsigned int framesize; > + unsigned int fps; > > mutex_lock(&sensor->mutex); > > mode = sensor->state.mode; > - interval->interval = mode->frame_interval; > + framesize = mode->hts * (mode->output_size_y + > + sensor->ctrls.vblank->val); > + fps = DIV_ROUND_CLOSEST(sensor->ctrls.pixel_rate->val, framesize); > + > + interval->interval.numerator = 1; > + interval->interval.denominator = fps; > > mutex_unlock(&sensor->mutex); > > @@ -2777,41 +2770,6 @@ static int ov8865_enum_frame_size(struct v4l2_subdev *subdev, > return 0; > } > > -static int ov8865_enum_frame_interval(struct v4l2_subdev *subdev, > - struct v4l2_subdev_state *sd_state, > - struct v4l2_subdev_frame_interval_enum *interval_enum) > -{ > - const struct ov8865_mode *mode = NULL; > - unsigned int mode_index; > - unsigned int interval_index; > - > - if (interval_enum->index > 0) > - return -EINVAL; > - /* > - * Multiple modes with the same dimensions may have different frame > - * intervals, so look up each relevant mode. > - */ > - for (mode_index = 0, interval_index = 0; > - mode_index < ARRAY_SIZE(ov8865_modes); mode_index++) { > - mode = &ov8865_modes[mode_index]; > - > - if (mode->output_size_x == interval_enum->width && > - mode->output_size_y == interval_enum->height) { > - if (interval_index == interval_enum->index) > - break; > - > - interval_index++; > - } > - } > - > - if (mode_index == ARRAY_SIZE(ov8865_modes)) > - return -EINVAL; > - > - interval_enum->interval = mode->frame_interval; > - > - return 0; > -} > - > static void > __ov8865_get_pad_crop(struct ov8865_sensor *sensor, > struct v4l2_subdev_state *state, unsigned int pad, > @@ -2870,7 +2828,6 @@ static const struct v4l2_subdev_pad_ops ov8865_subdev_pad_ops = { > .get_fmt = ov8865_get_fmt, > .set_fmt = ov8865_set_fmt, > .enum_frame_size = ov8865_enum_frame_size, > - .enum_frame_interval = ov8865_enum_frame_interval, > .get_selection = ov8865_get_selection, > .set_selection = ov8865_get_selection, > };
Hi Sakari On 01/11/2021 15:04, Sakari Ailus wrote: > Hi Daniel, > > On Mon, Nov 01, 2021 at 12:11:12AM +0000, Daniel Scally wrote: >> The HTS values for some of the modes in the ov8865 driver are a bit >> unusual, coming in lower than the output_size_x is set to. It seems >> like they might be calculated to fit the desired framerate into a >> configuration with just two data lanes. To bring this more in line >> with expected behaviour, raise the HTS values above the output_size_x. > This isn't necessarily a problem as binning may be done in analogue domain. > I don't know about this sensor though. > > Is the frame interval still as expected in binned modes after this patch? Actually none of the modes have binning in the X dimension set on. The ones with Y binning still have the framerates I expect, and the only modes who's HTS values I'm changing here don't use binning at all > >> The corollary of that change is that the hardcoded frame intervals >> against the modes no longer make sense, so remove those entirely. >> Update the .g/s_frame_interval() callbacks to calculate the frame >> interval based on the current mode and the vblank and hblank settings. >> >> The implementation of the .enum_frame_interval() callback is no longer >> suitable since the possible frame rate is now a continuous range depending >> on the vblank control setting, so remove that callback entirely. >> >> Signed-off-by: Daniel Scally <djrscally@gmail.com> >> --- >> drivers/media/i2c/ov8865.c | 65 +++++++------------------------------- >> 1 file changed, 11 insertions(+), 54 deletions(-) >> >> diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c >> index 4b18cc80f985..1b8674152750 100644 >> --- a/drivers/media/i2c/ov8865.c >> +++ b/drivers/media/i2c/ov8865.c >> @@ -659,8 +659,6 @@ struct ov8865_mode { >> unsigned int blc_anchor_right_start; >> unsigned int blc_anchor_right_end; >> >> - struct v4l2_fract frame_interval; >> - >> bool pll2_binning; >> >> const struct ov8865_register_value *register_values; >> @@ -964,7 +962,7 @@ static const struct ov8865_mode ov8865_modes[] = { >> { >> /* Horizontal */ >> .output_size_x = 3264, >> - .hts = 1944, >> + .hts = 3888, >> >> /* Vertical */ >> .output_size_y = 2448, >> @@ -1003,9 +1001,6 @@ static const struct ov8865_mode ov8865_modes[] = { >> .blc_anchor_right_start = 1984, >> .blc_anchor_right_end = 2239, >> >> - /* Frame Interval */ >> - .frame_interval = { 1, 30 }, >> - >> /* PLL */ >> .pll2_binning = false, >> >> @@ -1018,11 +1013,11 @@ static const struct ov8865_mode ov8865_modes[] = { >> { >> /* Horizontal */ >> .output_size_x = 3264, >> - .hts = 2582, >> + .hts = 3888, >> >> /* Vertical */ >> .output_size_y = 1836, >> - .vts = 2002, >> + .vts = 2470, >> >> .size_auto = true, >> .size_auto_boundary_x = 8, >> @@ -1057,9 +1052,6 @@ static const struct ov8865_mode ov8865_modes[] = { >> .blc_anchor_right_start = 1984, >> .blc_anchor_right_end = 2239, >> >> - /* Frame Interval */ >> - .frame_interval = { 1, 30 }, >> - >> /* PLL */ >> .pll2_binning = false, >> >> @@ -1115,9 +1107,6 @@ static const struct ov8865_mode ov8865_modes[] = { >> .blc_anchor_right_start = 992, >> .blc_anchor_right_end = 1119, >> >> - /* Frame Interval */ >> - .frame_interval = { 1, 30 }, >> - >> /* PLL */ >> .pll2_binning = true, >> >> @@ -1179,9 +1168,6 @@ static const struct ov8865_mode ov8865_modes[] = { >> .blc_anchor_right_start = 992, >> .blc_anchor_right_end = 1119, >> >> - /* Frame Interval */ >> - .frame_interval = { 1, 90 }, >> - >> /* PLL */ >> .pll2_binning = true, >> >> @@ -2628,11 +2614,18 @@ static int ov8865_g_frame_interval(struct v4l2_subdev *subdev, >> { >> struct ov8865_sensor *sensor = ov8865_subdev_sensor(subdev); >> const struct ov8865_mode *mode; >> + unsigned int framesize; >> + unsigned int fps; >> >> mutex_lock(&sensor->mutex); >> >> mode = sensor->state.mode; >> - interval->interval = mode->frame_interval; >> + framesize = mode->hts * (mode->output_size_y + >> + sensor->ctrls.vblank->val); >> + fps = DIV_ROUND_CLOSEST(sensor->ctrls.pixel_rate->val, framesize); >> + >> + interval->interval.numerator = 1; >> + interval->interval.denominator = fps; >> >> mutex_unlock(&sensor->mutex); >> >> @@ -2777,41 +2770,6 @@ static int ov8865_enum_frame_size(struct v4l2_subdev *subdev, >> return 0; >> } >> >> -static int ov8865_enum_frame_interval(struct v4l2_subdev *subdev, >> - struct v4l2_subdev_state *sd_state, >> - struct v4l2_subdev_frame_interval_enum *interval_enum) >> -{ >> - const struct ov8865_mode *mode = NULL; >> - unsigned int mode_index; >> - unsigned int interval_index; >> - >> - if (interval_enum->index > 0) >> - return -EINVAL; >> - /* >> - * Multiple modes with the same dimensions may have different frame >> - * intervals, so look up each relevant mode. >> - */ >> - for (mode_index = 0, interval_index = 0; >> - mode_index < ARRAY_SIZE(ov8865_modes); mode_index++) { >> - mode = &ov8865_modes[mode_index]; >> - >> - if (mode->output_size_x == interval_enum->width && >> - mode->output_size_y == interval_enum->height) { >> - if (interval_index == interval_enum->index) >> - break; >> - >> - interval_index++; >> - } >> - } >> - >> - if (mode_index == ARRAY_SIZE(ov8865_modes)) >> - return -EINVAL; >> - >> - interval_enum->interval = mode->frame_interval; >> - >> - return 0; >> -} >> - >> static void >> __ov8865_get_pad_crop(struct ov8865_sensor *sensor, >> struct v4l2_subdev_state *state, unsigned int pad, >> @@ -2870,7 +2828,6 @@ static const struct v4l2_subdev_pad_ops ov8865_subdev_pad_ops = { >> .get_fmt = ov8865_get_fmt, >> .set_fmt = ov8865_set_fmt, >> .enum_frame_size = ov8865_enum_frame_size, >> - .enum_frame_interval = ov8865_enum_frame_interval, >> .get_selection = ov8865_get_selection, >> .set_selection = ov8865_get_selection, >> };
diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c index 4b18cc80f985..1b8674152750 100644 --- a/drivers/media/i2c/ov8865.c +++ b/drivers/media/i2c/ov8865.c @@ -659,8 +659,6 @@ struct ov8865_mode { unsigned int blc_anchor_right_start; unsigned int blc_anchor_right_end; - struct v4l2_fract frame_interval; - bool pll2_binning; const struct ov8865_register_value *register_values; @@ -964,7 +962,7 @@ static const struct ov8865_mode ov8865_modes[] = { { /* Horizontal */ .output_size_x = 3264, - .hts = 1944, + .hts = 3888, /* Vertical */ .output_size_y = 2448, @@ -1003,9 +1001,6 @@ static const struct ov8865_mode ov8865_modes[] = { .blc_anchor_right_start = 1984, .blc_anchor_right_end = 2239, - /* Frame Interval */ - .frame_interval = { 1, 30 }, - /* PLL */ .pll2_binning = false, @@ -1018,11 +1013,11 @@ static const struct ov8865_mode ov8865_modes[] = { { /* Horizontal */ .output_size_x = 3264, - .hts = 2582, + .hts = 3888, /* Vertical */ .output_size_y = 1836, - .vts = 2002, + .vts = 2470, .size_auto = true, .size_auto_boundary_x = 8, @@ -1057,9 +1052,6 @@ static const struct ov8865_mode ov8865_modes[] = { .blc_anchor_right_start = 1984, .blc_anchor_right_end = 2239, - /* Frame Interval */ - .frame_interval = { 1, 30 }, - /* PLL */ .pll2_binning = false, @@ -1115,9 +1107,6 @@ static const struct ov8865_mode ov8865_modes[] = { .blc_anchor_right_start = 992, .blc_anchor_right_end = 1119, - /* Frame Interval */ - .frame_interval = { 1, 30 }, - /* PLL */ .pll2_binning = true, @@ -1179,9 +1168,6 @@ static const struct ov8865_mode ov8865_modes[] = { .blc_anchor_right_start = 992, .blc_anchor_right_end = 1119, - /* Frame Interval */ - .frame_interval = { 1, 90 }, - /* PLL */ .pll2_binning = true, @@ -2628,11 +2614,18 @@ static int ov8865_g_frame_interval(struct v4l2_subdev *subdev, { struct ov8865_sensor *sensor = ov8865_subdev_sensor(subdev); const struct ov8865_mode *mode; + unsigned int framesize; + unsigned int fps; mutex_lock(&sensor->mutex); mode = sensor->state.mode; - interval->interval = mode->frame_interval; + framesize = mode->hts * (mode->output_size_y + + sensor->ctrls.vblank->val); + fps = DIV_ROUND_CLOSEST(sensor->ctrls.pixel_rate->val, framesize); + + interval->interval.numerator = 1; + interval->interval.denominator = fps; mutex_unlock(&sensor->mutex); @@ -2777,41 +2770,6 @@ static int ov8865_enum_frame_size(struct v4l2_subdev *subdev, return 0; } -static int ov8865_enum_frame_interval(struct v4l2_subdev *subdev, - struct v4l2_subdev_state *sd_state, - struct v4l2_subdev_frame_interval_enum *interval_enum) -{ - const struct ov8865_mode *mode = NULL; - unsigned int mode_index; - unsigned int interval_index; - - if (interval_enum->index > 0) - return -EINVAL; - /* - * Multiple modes with the same dimensions may have different frame - * intervals, so look up each relevant mode. - */ - for (mode_index = 0, interval_index = 0; - mode_index < ARRAY_SIZE(ov8865_modes); mode_index++) { - mode = &ov8865_modes[mode_index]; - - if (mode->output_size_x == interval_enum->width && - mode->output_size_y == interval_enum->height) { - if (interval_index == interval_enum->index) - break; - - interval_index++; - } - } - - if (mode_index == ARRAY_SIZE(ov8865_modes)) - return -EINVAL; - - interval_enum->interval = mode->frame_interval; - - return 0; -} - static void __ov8865_get_pad_crop(struct ov8865_sensor *sensor, struct v4l2_subdev_state *state, unsigned int pad, @@ -2870,7 +2828,6 @@ static const struct v4l2_subdev_pad_ops ov8865_subdev_pad_ops = { .get_fmt = ov8865_get_fmt, .set_fmt = ov8865_set_fmt, .enum_frame_size = ov8865_enum_frame_size, - .enum_frame_interval = ov8865_enum_frame_interval, .get_selection = ov8865_get_selection, .set_selection = ov8865_get_selection, };
The HTS values for some of the modes in the ov8865 driver are a bit unusual, coming in lower than the output_size_x is set to. It seems like they might be calculated to fit the desired framerate into a configuration with just two data lanes. To bring this more in line with expected behaviour, raise the HTS values above the output_size_x. The corollary of that change is that the hardcoded frame intervals against the modes no longer make sense, so remove those entirely. Update the .g/s_frame_interval() callbacks to calculate the frame interval based on the current mode and the vblank and hblank settings. The implementation of the .enum_frame_interval() callback is no longer suitable since the possible frame rate is now a continuous range depending on the vblank control setting, so remove that callback entirely. Signed-off-by: Daniel Scally <djrscally@gmail.com> --- drivers/media/i2c/ov8865.c | 65 +++++++------------------------------- 1 file changed, 11 insertions(+), 54 deletions(-)