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@xxxxxxxxx>
>> ---
>> 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,
>> };
