| Message ID | 20210809225845.916430-9-djrscally@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Extensions to ov8865 driver | expand |
On Tue, Aug 10, 2021 at 1:59 AM Daniel Scally <djrscally@gmail.com> wrote: > > Add a V4L2_CID_HBLANK control to the ov8865 driver. This is read only > with timing control intended to be done via vblanking alone. ... > + hblank = mode->hts < mode->output_size_x ? 0 : mode->hts - mode->output_size_x; In both cases can be hblank = max(mode->output_size_x, mode->hts) - mode->output_size_x; or max_t() in case types are different (but in this case the code might be uglier). > + ctrls->hblank = v4l2_ctrl_new_std(handler, ops, V4L2_CID_HBLANK, hblank, > + hblank, 1, hblank); > + Redundant blank line (pun is not intended :). > + if (ctrls->hblank) > + ctrls->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; ... > + hblank = mode->hts < mode->output_size_x ? 0 : mode->hts - mode->output_size_x; See above. -- With Best Regards, Andy Shevchenko
Hi Daniel, On Mon, Aug 09, 2021 at 11:58:41PM +0100, Daniel Scally wrote: > @@ -2542,6 +2544,13 @@ static int ov8865_ctrls_init(struct ov8865_sensor *sensor) > 0, 0, ov8865_test_pattern_menu); > > /* Blanking */ > + hblank = mode->hts < mode->output_size_x ? 0 : mode->hts - mode->output_size_x; Is the result in relation with the analogue crop size? Based on the above it wouldn't seem like that. > + ctrls->hblank = v4l2_ctrl_new_std(handler, ops, V4L2_CID_HBLANK, hblank, > + hblank, 1, hblank); > + > + if (ctrls->hblank) > + ctrls->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; > + > vblank_max = OV8865_TIMING_MAX_VTS - mode->output_size_y; > vblank_def = mode->vts - mode->output_size_y; > ctrls->vblank = v4l2_ctrl_new_std(handler, ops, V4L2_CID_VBLANK, -- Sakari Ailus
Hi Sakari On 10/08/2021 15:29, Sakari Ailus wrote: > Hi Daniel, > > On Mon, Aug 09, 2021 at 11:58:41PM +0100, Daniel Scally wrote: >> @@ -2542,6 +2544,13 @@ static int ov8865_ctrls_init(struct ov8865_sensor *sensor) >> 0, 0, ov8865_test_pattern_menu); >> >> /* Blanking */ >> + hblank = mode->hts < mode->output_size_x ? 0 : mode->hts - mode->output_size_x; > Is the result in relation with the analogue crop size? Based on the above > it wouldn't seem like that. This was a weird one actually. My understanding was that HTS should always be >= the horizontal crop plus hblank...but that doesn't hold true for some of this driver's modes and nor does it hold true when running the camera in Windows (I checked the registers whilst running it). So I went with setting hblank to 0 if the mode's HTS exceeded the horizontal crop as the only way I could see to reconcile that. > >> + ctrls->hblank = v4l2_ctrl_new_std(handler, ops, V4L2_CID_HBLANK, hblank, >> + hblank, 1, hblank); >> + >> + if (ctrls->hblank) >> + ctrls->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; >> + >> vblank_max = OV8865_TIMING_MAX_VTS - mode->output_size_y; >> vblank_def = mode->vts - mode->output_size_y; >> ctrls->vblank = v4l2_ctrl_new_std(handler, ops, V4L2_CID_VBLANK,
Hi Daniel, On Tue, Aug 10, 2021 at 11:07:22PM +0100, Daniel Scally wrote: > On 10/08/2021 15:29, Sakari Ailus wrote: > > On Mon, Aug 09, 2021 at 11:58:41PM +0100, Daniel Scally wrote: > >> @@ -2542,6 +2544,13 @@ static int ov8865_ctrls_init(struct ov8865_sensor *sensor) > >> 0, 0, ov8865_test_pattern_menu); > >> > >> /* Blanking */ > >> + hblank = mode->hts < mode->output_size_x ? 0 : mode->hts - mode->output_size_x; > > > > Is the result in relation with the analogue crop size? Based on the above > > it wouldn't seem like that. > > This was a weird one actually. My understanding was that HTS should > always be >= the horizontal crop plus hblank...but that doesn't hold > true for some of this driver's modes and nor does it hold true when > running the camera in Windows (I checked the registers whilst running > it). So I went with setting hblank to 0 if the mode's HTS exceeded the > horizontal crop as the only way I could see to reconcile that. There's something very fishy here, HTS is, by definition, equal to the analog crop width plus the horizontal blanking. I suspect that the values in ov8865_modes are wrong. > >> + ctrls->hblank = v4l2_ctrl_new_std(handler, ops, V4L2_CID_HBLANK, hblank, > >> + hblank, 1, hblank); > >> + > >> + if (ctrls->hblank) > >> + ctrls->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; > >> + > >> vblank_max = OV8865_TIMING_MAX_VTS - mode->output_size_y; > >> vblank_def = mode->vts - mode->output_size_y; > >> ctrls->vblank = v4l2_ctrl_new_std(handler, ops, V4L2_CID_VBLANK, -- Regards, Laurent Pinchart
Hi Laurent On 13/08/2021 04:05, Laurent Pinchart wrote: > Hi Daniel, > > On Tue, Aug 10, 2021 at 11:07:22PM +0100, Daniel Scally wrote: >> On 10/08/2021 15:29, Sakari Ailus wrote: >>> On Mon, Aug 09, 2021 at 11:58:41PM +0100, Daniel Scally wrote: >>>> @@ -2542,6 +2544,13 @@ static int ov8865_ctrls_init(struct ov8865_sensor *sensor) >>>> 0, 0, ov8865_test_pattern_menu); >>>> >>>> /* Blanking */ >>>> + hblank = mode->hts < mode->output_size_x ? 0 : mode->hts - mode->output_size_x; >>> Is the result in relation with the analogue crop size? Based on the above >>> it wouldn't seem like that. >> This was a weird one actually. My understanding was that HTS should >> always be >= the horizontal crop plus hblank...but that doesn't hold >> true for some of this driver's modes and nor does it hold true when >> running the camera in Windows (I checked the registers whilst running >> it). So I went with setting hblank to 0 if the mode's HTS exceeded the >> horizontal crop as the only way I could see to reconcile that. > There's something very fishy here, HTS is, by definition, equal to the > analog crop width plus the horizontal blanking. I suspect that the > values in ov8865_modes are wrong. I thought that initially too but confirming that the same thing happened running windows switched me into "you're probably wrong" mode. If we're confident that the HTS is likely wrong though I can add an extra patch to bring those into lining with that definition. > >>>> + ctrls->hblank = v4l2_ctrl_new_std(handler, ops, V4L2_CID_HBLANK, hblank, >>>> + hblank, 1, hblank); >>>> + >>>> + if (ctrls->hblank) >>>> + ctrls->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; >>>> + >>>> vblank_max = OV8865_TIMING_MAX_VTS - mode->output_size_y; >>>> vblank_def = mode->vts - mode->output_size_y; >>>> ctrls->vblank = v4l2_ctrl_new_std(handler, ops, V4L2_CID_VBLANK,
Hi Daniel, On Fri, Aug 13, 2021 at 10:45:48AM +0100, Daniel Scally wrote: > On 13/08/2021 04:05, Laurent Pinchart wrote: > > On Tue, Aug 10, 2021 at 11:07:22PM +0100, Daniel Scally wrote: > >> On 10/08/2021 15:29, Sakari Ailus wrote: > >>> On Mon, Aug 09, 2021 at 11:58:41PM +0100, Daniel Scally wrote: > >>>> @@ -2542,6 +2544,13 @@ static int ov8865_ctrls_init(struct ov8865_sensor *sensor) > >>>> 0, 0, ov8865_test_pattern_menu); > >>>> > >>>> /* Blanking */ > >>>> + hblank = mode->hts < mode->output_size_x ? 0 : mode->hts - mode->output_size_x; > >>> > >>> Is the result in relation with the analogue crop size? Based on the above > >>> it wouldn't seem like that. > >> > >> This was a weird one actually. My understanding was that HTS should > >> always be >= the horizontal crop plus hblank...but that doesn't hold > >> true for some of this driver's modes and nor does it hold true when > >> running the camera in Windows (I checked the registers whilst running > >> it). So I went with setting hblank to 0 if the mode's HTS exceeded the > >> horizontal crop as the only way I could see to reconcile that. > > > > There's something very fishy here, HTS is, by definition, equal to the > > analog crop width plus the horizontal blanking. I suspect that the > > values in ov8865_modes are wrong. > > I thought that initially too but confirming that the same thing happened > running windows switched me into "you're probably wrong" mode. If we're > confident that the HTS is likely wrong though I can add an extra patch > to bring those into lining with that definition. I think it's worth investigating this. The hblank computed here is clearly incorrect, and would thus be useless for all practical purposes. As usual with OmniVision, the datasheet is also quite useless. Paul, do you have any information about this ? > >>>> + ctrls->hblank = v4l2_ctrl_new_std(handler, ops, V4L2_CID_HBLANK, hblank, > >>>> + hblank, 1, hblank); > >>>> + > >>>> + if (ctrls->hblank) > >>>> + ctrls->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; > >>>> + > >>>> vblank_max = OV8865_TIMING_MAX_VTS - mode->output_size_y; > >>>> vblank_def = mode->vts - mode->output_size_y; > >>>> ctrls->vblank = v4l2_ctrl_new_std(handler, ops, V4L2_CID_VBLANK, -- Regards, Laurent Pinchart
Hi Paul On 14/08/2021 21:56, Laurent Pinchart wrote: > Hi Daniel, > > On Fri, Aug 13, 2021 at 10:45:48AM +0100, Daniel Scally wrote: >> On 13/08/2021 04:05, Laurent Pinchart wrote: >>> On Tue, Aug 10, 2021 at 11:07:22PM +0100, Daniel Scally wrote: >>>> On 10/08/2021 15:29, Sakari Ailus wrote: >>>>> On Mon, Aug 09, 2021 at 11:58:41PM +0100, Daniel Scally wrote: >>>>>> @@ -2542,6 +2544,13 @@ static int ov8865_ctrls_init(struct ov8865_sensor *sensor) >>>>>> 0, 0, ov8865_test_pattern_menu); >>>>>> >>>>>> /* Blanking */ >>>>>> + hblank = mode->hts < mode->output_size_x ? 0 : mode->hts - mode->output_size_x; >>>>> Is the result in relation with the analogue crop size? Based on the above >>>>> it wouldn't seem like that. >>>> This was a weird one actually. My understanding was that HTS should >>>> always be >= the horizontal crop plus hblank...but that doesn't hold >>>> true for some of this driver's modes and nor does it hold true when >>>> running the camera in Windows (I checked the registers whilst running >>>> it). So I went with setting hblank to 0 if the mode's HTS exceeded the >>>> horizontal crop as the only way I could see to reconcile that. >>> There's something very fishy here, HTS is, by definition, equal to the >>> analog crop width plus the horizontal blanking. I suspect that the >>> values in ov8865_modes are wrong. >> I thought that initially too but confirming that the same thing happened >> running windows switched me into "you're probably wrong" mode. If we're >> confident that the HTS is likely wrong though I can add an extra patch >> to bring those into lining with that definition. > I think it's worth investigating this. The hblank computed here is > clearly incorrect, and would thus be useless for all practical purposes. > As usual with OmniVision, the datasheet is also quite useless. > > Paul, do you have any information about this ? A gentle ping on this...I played around setting HTS / VTS values whilst the camera was running windows; and it behaves as you'd expect it to (raising/lowering the frame rate), so as far as I can tell the sensor itself isn't doing anything unusual... >>>>>> + ctrls->hblank = v4l2_ctrl_new_std(handler, ops, V4L2_CID_HBLANK, hblank, >>>>>> + hblank, 1, hblank); >>>>>> + >>>>>> + if (ctrls->hblank) >>>>>> + ctrls->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; >>>>>> + >>>>>> vblank_max = OV8865_TIMING_MAX_VTS - mode->output_size_y; >>>>>> vblank_def = mode->vts - mode->output_size_y; >>>>>> ctrls->vblank = v4l2_ctrl_new_std(handler, ops, V4L2_CID_VBLANK,
Hello all On 09/09/2021 23:36, Daniel Scally wrote: > Hi Paul > > On 14/08/2021 21:56, Laurent Pinchart wrote: >> Hi Daniel, >> >> On Fri, Aug 13, 2021 at 10:45:48AM +0100, Daniel Scally wrote: >>> On 13/08/2021 04:05, Laurent Pinchart wrote: >>>> On Tue, Aug 10, 2021 at 11:07:22PM +0100, Daniel Scally wrote: >>>>> On 10/08/2021 15:29, Sakari Ailus wrote: >>>>>> On Mon, Aug 09, 2021 at 11:58:41PM +0100, Daniel Scally wrote: >>>>>>> @@ -2542,6 +2544,13 @@ static int ov8865_ctrls_init(struct ov8865_sensor *sensor) >>>>>>> 0, 0, ov8865_test_pattern_menu); >>>>>>> >>>>>>> /* Blanking */ >>>>>>> + hblank = mode->hts < mode->output_size_x ? 0 : mode->hts - mode->output_size_x; >>>>>> Is the result in relation with the analogue crop size? Based on the above >>>>>> it wouldn't seem like that. >>>>> This was a weird one actually. My understanding was that HTS should >>>>> always be >= the horizontal crop plus hblank...but that doesn't hold >>>>> true for some of this driver's modes and nor does it hold true when >>>>> running the camera in Windows (I checked the registers whilst running >>>>> it). So I went with setting hblank to 0 if the mode's HTS exceeded the >>>>> horizontal crop as the only way I could see to reconcile that. >>>> There's something very fishy here, HTS is, by definition, equal to the >>>> analog crop width plus the horizontal blanking. I suspect that the >>>> values in ov8865_modes are wrong. >>> I thought that initially too but confirming that the same thing happened >>> running windows switched me into "you're probably wrong" mode. If we're >>> confident that the HTS is likely wrong though I can add an extra patch >>> to bring those into lining with that definition. >> I think it's worth investigating this. The hblank computed here is >> clearly incorrect, and would thus be useless for all practical purposes. >> As usual with OmniVision, the datasheet is also quite useless. >> >> Paul, do you have any information about this ? > > A gentle ping on this...I played around setting HTS / VTS values whilst > the camera was running windows; and it behaves as you'd expect it to > (raising/lowering the frame rate), so as far as I can tell the sensor > itself isn't doing anything unusual... So, looking at this again. The mode in question has: .output_size_x = 3264 .hts = 1944 .output_size_y = 2448 .vts = 2470 .frame_interval = { 1, 30 } And the driver sets a link frequency of 360MHz. That makes the pixel rate, depending on whether we're looking at 2 or 4 data lanes, either 144MHz or 288MHz. I think the HTS there is calculated so that the 2 lane configuration can make 30 FPS. Perhaps it would be better to default in the mode to the "ideal" 4-lane 30fps setting (by upping the .hts to 3888), but rather than hardcoding the frame interval there, calculate it for .g_frame_interval() based on the number of data lanes found in the bus (accepting that if we only have 2 it's going to be 15fps rather than 30, which doesn't seem unreasonable for that resolution)
diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c index 810047c247b4..db84294b7a03 100644 --- a/drivers/media/i2c/ov8865.c +++ b/drivers/media/i2c/ov8865.c @@ -673,6 +673,7 @@ struct ov8865_state { struct ov8865_ctrls { struct v4l2_ctrl *link_freq; struct v4l2_ctrl *pixel_rate; + struct v4l2_ctrl *hblank; struct v4l2_ctrl *vblank; struct v4l2_ctrl_handler handler; @@ -2506,6 +2507,7 @@ static int ov8865_ctrls_init(struct ov8865_sensor *sensor) const struct v4l2_ctrl_ops *ops = &ov8865_ctrl_ops; const struct ov8865_mode *mode = sensor->state.mode; unsigned int vblank_max, vblank_def; + unsigned int hblank; int ret; v4l2_ctrl_handler_init(handler, 32); @@ -2542,6 +2544,13 @@ static int ov8865_ctrls_init(struct ov8865_sensor *sensor) 0, 0, ov8865_test_pattern_menu); /* Blanking */ + hblank = mode->hts < mode->output_size_x ? 0 : mode->hts - mode->output_size_x; + ctrls->hblank = v4l2_ctrl_new_std(handler, ops, V4L2_CID_HBLANK, hblank, + hblank, 1, hblank); + + if (ctrls->hblank) + ctrls->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; + vblank_max = OV8865_TIMING_MAX_VTS - mode->output_size_y; vblank_def = mode->vts - mode->output_size_y; ctrls->vblank = v4l2_ctrl_new_std(handler, ops, V4L2_CID_VBLANK, @@ -2688,6 +2697,7 @@ static int ov8865_set_fmt(struct v4l2_subdev *subdev, struct v4l2_mbus_framefmt *mbus_format = &format->format; const struct ov8865_mode *mode; u32 mbus_code = 0; + unsigned int hblank; unsigned int index; int ret = 0; @@ -2732,6 +2742,10 @@ static int ov8865_set_fmt(struct v4l2_subdev *subdev, OV8865_TIMING_MAX_VTS - mode->output_size_y, 1, mode->vts - mode->output_size_y); + hblank = mode->hts < mode->output_size_x ? 0 : mode->hts - mode->output_size_x; + __v4l2_ctrl_modify_range(sensor->ctrls.hblank, hblank, hblank, 1, + hblank); + complete: mutex_unlock(&sensor->mutex);
Add a V4L2_CID_HBLANK control to the ov8865 driver. This is read only with timing control intended to be done via vblanking alone. Signed-off-by: Daniel Scally <djrscally@gmail.com> --- Changes in v2: - None drivers/media/i2c/ov8865.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)