| Message ID | 1529309219-27404-2-git-send-email-todor.tomov@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] media: ov5645: Supported external clock is 24MHz | expand |
Hi Todor, On Mon, Jun 18, 2018 at 11:06:59AM +0300, Todor Tomov wrote: > The OV5645 supports automatic exposure (AE) and automatic white > balance (AWB). When streaming is started it takes up to 5 frames > until the AE and AWB converge and output a frame with good quality. The frames aren't bad as such; it's just that the AE hasn't converged yet. I presume the number of the frames needed depends on the lighting conditions. The g_skip_frames is intended to tell the frames really are bad, i.e. distorted or broken somehow. I wouldn't discard them on the grounds of unconverged exposure. If we did, then on which other grounds should the frames be discarded as well? Does the white balance and focus need to be converged as well before considering the frames good, for instance? If you need this, I'd use a control instead to tell AE has converged. I wonder what others think. > > Implement g_skip_frames to report number of frames to be skipped > when streaming is started. > > Signed-off-by: Todor Tomov <todor.tomov@linaro.org> > --- > drivers/media/i2c/ov5645.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > index 1722cda..00bc3c0 100644 > --- a/drivers/media/i2c/ov5645.c > +++ b/drivers/media/i2c/ov5645.c > @@ -70,6 +70,9 @@ > #define OV5645_SDE_SAT_U 0x5583 > #define OV5645_SDE_SAT_V 0x5584 > > +/* Number of frames needed for AE and AWB to converge */ > +#define OV5645_NUM_OF_SKIP_FRAMES 5 > + > struct reg_value { > u16 reg; > u8 val; > @@ -1071,6 +1074,13 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable) > return 0; > } > > +static int ov5645_get_skip_frames(struct v4l2_subdev *sd, u32 *frames) > +{ > + *frames = OV5645_NUM_OF_SKIP_FRAMES; > + > + return 0; > +} > + > static const struct v4l2_subdev_core_ops ov5645_core_ops = { > .s_power = ov5645_s_power, > }; > @@ -1088,10 +1098,15 @@ static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = { > .get_selection = ov5645_get_selection, > }; > > +static const struct v4l2_subdev_sensor_ops ov5645_sensor_ops = { > + .g_skip_frames = ov5645_get_skip_frames, > +}; > + > static const struct v4l2_subdev_ops ov5645_subdev_ops = { > .core = &ov5645_core_ops, > .video = &ov5645_video_ops, > .pad = &ov5645_subdev_pad_ops, > + .sensor = &ov5645_sensor_ops, > }; > > static int ov5645_probe(struct i2c_client *client, > -- > 2.7.4 > -- Sakari Ailus sakari.ailus@linux.intel.com
Hi Sakari, Thank you for the reply and sorry for not following up on this for so long. On 19.06.2018 07:04, Sakari Ailus wrote: > Hi Todor, > > On Mon, Jun 18, 2018 at 11:06:59AM +0300, Todor Tomov wrote: >> The OV5645 supports automatic exposure (AE) and automatic white >> balance (AWB). When streaming is started it takes up to 5 frames >> until the AE and AWB converge and output a frame with good quality. > > The frames aren't bad as such; it's just that the AE hasn't converged yet. > I presume the number of the frames needed depends on the lighting > conditions. Yes, the number of frames (for AE and AWB converge) is different in different conditions. From testing I see usually 4 frames are needed, in more rare cases - 5 frames. The sensor doesn't provide any information about the state of the algorithms - are they currently converged or not. So the driver has no way to check that or tell for sure. > > The g_skip_frames is intended to tell the frames really are bad, i.e. > distorted or broken somehow. I was about to say that if the exposure is severely wrong then they are bad enough to not be used, which means that they are anyway bad, however... > > I wouldn't discard them on the grounds of unconverged exposure. If we did, > then on which other grounds should the frames be discarded as well? Does > the white balance and focus need to be converged as well before considering > the frames good, for instance? ...I agree that this measure for a bad vs not bad frame is quite subjective (when we have no feedback about the state of the algorithms). > > If you need this, I'd use a control instead to tell AE has converged. > > I wonder what others think. It would be nice to hear any ideas if anyone has any. I wonder whether the driver can do anything useful besides leaving it to the userspace. Best regards, Todor > >> >> Implement g_skip_frames to report number of frames to be skipped >> when streaming is started. >> >> Signed-off-by: Todor Tomov <todor.tomov@linaro.org> >> --- >> drivers/media/i2c/ov5645.c | 15 +++++++++++++++ >> 1 file changed, 15 insertions(+) >> >> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c >> index 1722cda..00bc3c0 100644 >> --- a/drivers/media/i2c/ov5645.c >> +++ b/drivers/media/i2c/ov5645.c >> @@ -70,6 +70,9 @@ >> #define OV5645_SDE_SAT_U 0x5583 >> #define OV5645_SDE_SAT_V 0x5584 >> >> +/* Number of frames needed for AE and AWB to converge */ >> +#define OV5645_NUM_OF_SKIP_FRAMES 5 >> + >> struct reg_value { >> u16 reg; >> u8 val; >> @@ -1071,6 +1074,13 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable) >> return 0; >> } >> >> +static int ov5645_get_skip_frames(struct v4l2_subdev *sd, u32 *frames) >> +{ >> + *frames = OV5645_NUM_OF_SKIP_FRAMES; >> + >> + return 0; >> +} >> + >> static const struct v4l2_subdev_core_ops ov5645_core_ops = { >> .s_power = ov5645_s_power, >> }; >> @@ -1088,10 +1098,15 @@ static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = { >> .get_selection = ov5645_get_selection, >> }; >> >> +static const struct v4l2_subdev_sensor_ops ov5645_sensor_ops = { >> + .g_skip_frames = ov5645_get_skip_frames, >> +}; >> + >> static const struct v4l2_subdev_ops ov5645_subdev_ops = { >> .core = &ov5645_core_ops, >> .video = &ov5645_video_ops, >> .pad = &ov5645_subdev_pad_ops, >> + .sensor = &ov5645_sensor_ops, >> }; >> >> static int ov5645_probe(struct i2c_client *client, >> -- >> 2.7.4 >> >
diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c index 1722cda..00bc3c0 100644 --- a/drivers/media/i2c/ov5645.c +++ b/drivers/media/i2c/ov5645.c @@ -70,6 +70,9 @@ #define OV5645_SDE_SAT_U 0x5583 #define OV5645_SDE_SAT_V 0x5584 +/* Number of frames needed for AE and AWB to converge */ +#define OV5645_NUM_OF_SKIP_FRAMES 5 + struct reg_value { u16 reg; u8 val; @@ -1071,6 +1074,13 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable) return 0; } +static int ov5645_get_skip_frames(struct v4l2_subdev *sd, u32 *frames) +{ + *frames = OV5645_NUM_OF_SKIP_FRAMES; + + return 0; +} + static const struct v4l2_subdev_core_ops ov5645_core_ops = { .s_power = ov5645_s_power, }; @@ -1088,10 +1098,15 @@ static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = { .get_selection = ov5645_get_selection, }; +static const struct v4l2_subdev_sensor_ops ov5645_sensor_ops = { + .g_skip_frames = ov5645_get_skip_frames, +}; + static const struct v4l2_subdev_ops ov5645_subdev_ops = { .core = &ov5645_core_ops, .video = &ov5645_video_ops, .pad = &ov5645_subdev_pad_ops, + .sensor = &ov5645_sensor_ops, }; static int ov5645_probe(struct i2c_client *client,
The OV5645 supports automatic exposure (AE) and automatic white balance (AWB). When streaming is started it takes up to 5 frames until the AE and AWB converge and output a frame with good quality. Implement g_skip_frames to report number of frames to be skipped when streaming is started. Signed-off-by: Todor Tomov <todor.tomov@linaro.org> --- drivers/media/i2c/ov5645.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) -- 2.7.4