| Message ID | 1534155586-26974-6-git-send-email-hugues.fruchet@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix OV5640 exposure & gain | expand |
Hi Hugues,
thanks for the patch
On Mon, Aug 13, 2018 at 12:19:46PM +0200, Hugues Fruchet wrote:
> Mode setting depends on last mode set, in particular
> because of exposure calculation when downscale mode
> change between subsampling and scaling.
> At stream on the last mode was wrongly set to current mode,
> so no change was detected and exposure calculation
> was not made, fix this.
I actually see a different issue here...
The issue I see here depends on the format programmed through
set_fmt() never being applied when using the sensor with a media
controller equipped device (in this case an i.MX6 board) through
capture sessions, and the not properly calculated exposure you see may
be a consequence of this.
I'll try to write down what I see, with the help of some debug output.
- At probe time mode 640x460@30 is programmed:
[ 1.651216] ov5640_probe: Initial mode with id: 2
- I set the format on the sensor's pad and it gets not applied but
marked as pending as the sensor is powered off:
#media-ctl --set-v4l2 "'ov5640 2-003c':0[fmt:UYVY2X8/320x240 field:none]"
[ 65.611983] ov5640_set_fmt: NEW mode with id: 1 - PENDING
- I start streaming with yavta, and the sensor receives a power on;
this causes the 'initial' format to be re-programmed and the pending
change to be ignored:
#yavta -c10 -n4 -f YUYV -s $320x240 -F"../frame-#.yuv" /dev/video4
[ 69.395018] ov5640_set_power:1805 - on
[ 69.431342] ov5640_restore_mode:1711
[ 69.996882] ov5640_set_mode: Apply mode with id: 0
The 'ov5640_set_mode()' call from 'ov5640_restore_mode()' clears the
sensor->pending flag, discarding the newly requested format, for
this reason, at s_stream() time, the pending flag is not set
anymore.
Are you using a media-controller system? I suspect in non-mc cases,
the set_fmt is applied through a single power_on/power_off session, not
causing the 'restore_mode()' issue. Is this the case for you or your
issue is differnt?
Edit:
Mita-san tried to address the issue of the output pixel format not
being restored when the image format was restored in
19ad26f9e6e1 ("media: ov5640: add missing output pixel format setting")
I understand the issue he tried to fix, but shouldn't the pending
format (if any) be applied instead of the initial one unconditionally?
Thanks
j
>
> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> ---
> drivers/media/i2c/ov5640.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index c110a6a..923cc30 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -225,6 +225,7 @@ struct ov5640_dev {
> struct v4l2_mbus_framefmt fmt;
>
> const struct ov5640_mode_info *current_mode;
> + const struct ov5640_mode_info *last_mode;
> enum ov5640_frame_rate current_fr;
> struct v4l2_fract frame_interval;
>
> @@ -1628,6 +1629,9 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
> bool auto_exp = sensor->ctrls.auto_exp->val == V4L2_EXPOSURE_AUTO;
> int ret;
>
> + if (!orig_mode)
> + orig_mode = mode;
> +
> dn_mode = mode->dn_mode;
> orig_dn_mode = orig_mode->dn_mode;
>
> @@ -1688,6 +1692,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
> return ret;
>
> sensor->pending_mode_change = false;
> + sensor->last_mode = mode;
>
> return 0;
>
> @@ -2551,7 +2556,8 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
>
> if (sensor->streaming == !enable) {
> if (enable && sensor->pending_mode_change) {
> - ret = ov5640_set_mode(sensor, sensor->current_mode);
> + ret = ov5640_set_mode(sensor, sensor->last_mode);
> +
> if (ret)
> goto out;
>
> --
> 2.7.4
>
Hi Jacopo, On 08/16/2018 12:10 PM, jacopo mondi wrote: > Hi Hugues, > thanks for the patch > > On Mon, Aug 13, 2018 at 12:19:46PM +0200, Hugues Fruchet wrote: >> Mode setting depends on last mode set, in particular >> because of exposure calculation when downscale mode >> change between subsampling and scaling. >> At stream on the last mode was wrongly set to current mode, >> so no change was detected and exposure calculation >> was not made, fix this. > > I actually see a different issue here... Which problem do you have exactly, you got a VGA JPEG instead of a QVGA YUYV ? > > The issue I see here depends on the format programmed through > set_fmt() never being applied when using the sensor with a media > controller equipped device (in this case an i.MX6 board) through > capture sessions, and the not properly calculated exposure you see may > be a consequence of this. > > I'll try to write down what I see, with the help of some debug output. > > - At probe time mode 640x460@30 is programmed: > [ 1.651216] ov5640_probe: Initial mode with id: 2 > > - I set the format on the sensor's pad and it gets not applied but > marked as pending as the sensor is powered off: > > #media-ctl --set-v4l2 "'ov5640 2-003c':0[fmt:UYVY2X8/320x240 field:none]" > [ 65.611983] ov5640_set_fmt: NEW mode with id: 1 - PENDING So here sensor->current_mode is set to <1>;//QVGA and sensor->pending_mode_change is set to true; > > - I start streaming with yavta, and the sensor receives a power on; > this causes the 'initial' format to be re-programmed and the pending > change to be ignored: > > #yavta -c10 -n4 -f YUYV -s $320x240 -F"../frame-#.yuv" /dev/video4 > [ 69.395018] ov5640_set_power:1805 - on > [ 69.431342] ov5640_restore_mode:1711 > [ 69.996882] ov5640_set_mode: Apply mode with id: 0 > > The 'ov5640_set_mode()' call from 'ov5640_restore_mode()' clears the > sensor->pending flag, discarding the newly requested format, for > this reason, at s_stream() time, the pending flag is not set > anymore. OK but before clearing sensor->pending_mode_change, set_mode() is loading registers corresponding to sensor->current_mode: static int ov5640_set_mode(struct ov5640_dev *sensor, const struct ov5640_mode_info *orig_mode) { ==> const struct ov5640_mode_info *mode = sensor->current_mode; ... ret = ov5640_set_mode_direct(sensor, mode, exposure); => so mode <1> is expected to be set now, so I don't understand your trace: "> [ 69.996882] ov5640_set_mode: Apply mode with id: 0" Which variable do you trace that shows "0" ? > > Are you using a media-controller system? I suspect in non-mc cases, > the set_fmt is applied through a single power_on/power_off session, not > causing the 'restore_mode()' issue. Is this the case for you or your > issue is differnt? > > Edit: > Mita-san tried to address the issue of the output pixel format not > being restored when the image format was restored in > 19ad26f9e6e1 ("media: ov5640: add missing output pixel format setting") > > I understand the issue he tried to fix, but shouldn't the pending > format (if any) be applied instead of the initial one unconditionally? This is what does the ov5640_restore_mode(), set the current mode (sensor->current_mode), that is done through this line: /* now restore the last capture mode */ ret = ov5640_set_mode(sensor, &ov5640_mode_init_data); => note that the comment above is weird, in fact it is the "current" mode that is set. => note also that the 2nd parameter is not the mode to be set but the previously applied mode ! (ie loaded in ov5640 registers). This is used to decide if we have to go to the "set_mode_exposure_calc" or "set_mode_direct". the ov5640_restore_mode() also set the current pixel format (sensor->fmt), that is done through this line: return ov5640_set_framefmt(sensor, &sensor->fmt); ==> This is what have fixed Mita-san, this line was missing previously, leading to "mode registers" being loaded but not the "pixel format registers". PS: There are two other "set mode" related changes that are related to this: 1) 6949d864776e ("media: ov5640: do not change mode if format or frame interval is unchanged") => this is merged in media master, unfortunately I've introduced a regression on "pixel format" side that I've fixed in this patchset : 2) https://www.mail-archive.com/linux-media@vger.kernel.org/msg134413.html Symptom was a noisy image when capturing QVGA YUV (in fact captured as JPEG data). Best regards, Hugues. > > Thanks > j > >> >> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> >> --- >> drivers/media/i2c/ov5640.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c >> index c110a6a..923cc30 100644 >> --- a/drivers/media/i2c/ov5640.c >> +++ b/drivers/media/i2c/ov5640.c >> @@ -225,6 +225,7 @@ struct ov5640_dev { >> struct v4l2_mbus_framefmt fmt; >> >> const struct ov5640_mode_info *current_mode; >> + const struct ov5640_mode_info *last_mode; >> enum ov5640_frame_rate current_fr; >> struct v4l2_fract frame_interval; >> >> @@ -1628,6 +1629,9 @@ static int ov5640_set_mode(struct ov5640_dev *sensor, >> bool auto_exp = sensor->ctrls.auto_exp->val == V4L2_EXPOSURE_AUTO; >> int ret; >> >> + if (!orig_mode) >> + orig_mode = mode; >> + >> dn_mode = mode->dn_mode; >> orig_dn_mode = orig_mode->dn_mode; >> >> @@ -1688,6 +1692,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor, >> return ret; >> >> sensor->pending_mode_change = false; >> + sensor->last_mode = mode; >> >> return 0; >> >> @@ -2551,7 +2556,8 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable) >> >> if (sensor->streaming == !enable) { >> if (enable && sensor->pending_mode_change) { >> - ret = ov5640_set_mode(sensor, sensor->current_mode); >> + ret = ov5640_set_mode(sensor, sensor->last_mode); >> + >> if (ret) >> goto out; >> >> -- >> 2.7.4 >>
Hi Hugues, On Thu, Aug 16, 2018 at 03:07:54PM +0000, Hugues FRUCHET wrote: > Hi Jacopo, > > On 08/16/2018 12:10 PM, jacopo mondi wrote: > > Hi Hugues, > > thanks for the patch > > > > On Mon, Aug 13, 2018 at 12:19:46PM +0200, Hugues Fruchet wrote: > >> Mode setting depends on last mode set, in particular > >> because of exposure calculation when downscale mode > >> change between subsampling and scaling. > >> At stream on the last mode was wrongly set to current mode, > >> so no change was detected and exposure calculation > >> was not made, fix this. > > > > I actually see a different issue here... > > Which problem do you have exactly, you got a VGA JPEG instead of a QVGA > YUYV ? > Not a matter of image format but sizes. I printed out the format applied and it seems to me it was always the initial one. On a second thought, a pipeline with a mis-configuration would not have ever started streaming, so I should have investigated better. > > > > The issue I see here depends on the format programmed through > > set_fmt() never being applied when using the sensor with a media > > controller equipped device (in this case an i.MX6 board) through > > capture sessions, and the not properly calculated exposure you see may > > be a consequence of this. > > > > I'll try to write down what I see, with the help of some debug output. > > > > - At probe time mode 640x460@30 is programmed: > > [ 1.651216] ov5640_probe: Initial mode with id: 2 > > > > - I set the format on the sensor's pad and it gets not applied but > > marked as pending as the sensor is powered off: > > > > #media-ctl --set-v4l2 "'ov5640 2-003c':0[fmt:UYVY2X8/320x240 field:none]" > > [ 65.611983] ov5640_set_fmt: NEW mode with id: 1 - PENDING > > So here sensor->current_mode is set to <1>;//QVGA > and sensor->pending_mode_change is set to true; > > > > > - I start streaming with yavta, and the sensor receives a power on; > > this causes the 'initial' format to be re-programmed and the pending > > change to be ignored: > > > > #yavta -c10 -n4 -f YUYV -s $320x240 -F"../frame-#.yuv" /dev/video4 > > [ 69.395018] ov5640_set_power:1805 - on > > [ 69.431342] ov5640_restore_mode:1711 > > [ 69.996882] ov5640_set_mode: Apply mode with id: 0 > > > > The 'ov5640_set_mode()' call from 'ov5640_restore_mode()' clears the > > sensor->pending flag, discarding the newly requested format, for > > this reason, at s_stream() time, the pending flag is not set > > anymore. > > OK but before clearing sensor->pending_mode_change, set_mode() is > loading registers corresponding to sensor->current_mode: > static int ov5640_set_mode(struct ov5640_dev *sensor, > const struct ov5640_mode_info *orig_mode) > { > ==> const struct ov5640_mode_info *mode = sensor->current_mode; > ... > ret = ov5640_set_mode_direct(sensor, mode, exposure); > > => so mode <1> is expected to be set now, so I don't understand your trace: > "> [ 69.996882] ov5640_set_mode: Apply mode with id: 0" > Which variable do you trace that shows "0" ? > > > > > > Are you using a media-controller system? I suspect in non-mc cases, > > the set_fmt is applied through a single power_on/power_off session, not > > causing the 'restore_mode()' issue. Is this the case for you or your > > issue is differnt? > > > > Edit: > > Mita-san tried to address the issue of the output pixel format not > > being restored when the image format was restored in > > 19ad26f9e6e1 ("media: ov5640: add missing output pixel format setting") > > > > I understand the issue he tried to fix, but shouldn't the pending > > format (if any) be applied instead of the initial one unconditionally? > > This is what does the ov5640_restore_mode(), set the current mode > (sensor->current_mode), that is done through this line: > /* now restore the last capture mode */ > ret = ov5640_set_mode(sensor, &ov5640_mode_init_data); > => note that the comment above is weird, in fact it is the "current" > mode that is set. > => note also that the 2nd parameter is not the mode to be set but the > previously applied mode ! (ie loaded in ov5640 registers). This is used > to decide if we have to go to the "set_mode_exposure_calc" or > "set_mode_direct". This is where I got lost... Sorry for the sloppy comment, now I get what your patch was for :) > > the ov5640_restore_mode() also set the current pixel format > (sensor->fmt), that is done through this line: > return ov5640_set_framefmt(sensor, &sensor->fmt); > ==> This is what have fixed Mita-san, this line was missing previously, > leading to "mode registers" being loaded but not the "pixel format > registers". > > > PS: There are two other "set mode" related changes that are related to this: > 1) 6949d864776e ("media: ov5640: do not change mode if format or frame > interval is unchanged") > => this is merged in media master, unfortunately I've introduced a > regression on "pixel format" side that I've fixed in this patchset : > 2) https://www.mail-archive.com/linux-media@vger.kernel.org/msg134413.html > Symptom was a noisy image when capturing QVGA YUV (in fact captured as > JPEG data). I see, thanks for the detailed explanation! Thanks j > > > Best regards, > Hugues. > > > > > Thanks > > j > > > >> > >> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> > >> --- > >> drivers/media/i2c/ov5640.c | 8 +++++++- > >> 1 file changed, 7 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > >> index c110a6a..923cc30 100644 > >> --- a/drivers/media/i2c/ov5640.c > >> +++ b/drivers/media/i2c/ov5640.c > >> @@ -225,6 +225,7 @@ struct ov5640_dev { > >> struct v4l2_mbus_framefmt fmt; > >> > >> const struct ov5640_mode_info *current_mode; > >> + const struct ov5640_mode_info *last_mode; > >> enum ov5640_frame_rate current_fr; > >> struct v4l2_fract frame_interval; > >> > >> @@ -1628,6 +1629,9 @@ static int ov5640_set_mode(struct ov5640_dev *sensor, > >> bool auto_exp = sensor->ctrls.auto_exp->val == V4L2_EXPOSURE_AUTO; > >> int ret; > >> > >> + if (!orig_mode) > >> + orig_mode = mode; > >> + > >> dn_mode = mode->dn_mode; > >> orig_dn_mode = orig_mode->dn_mode; > >> > >> @@ -1688,6 +1692,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor, > >> return ret; > >> > >> sensor->pending_mode_change = false; > >> + sensor->last_mode = mode; > >> > >> return 0; > >> > >> @@ -2551,7 +2556,8 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable) > >> > >> if (sensor->streaming == !enable) { > >> if (enable && sensor->pending_mode_change) { > >> - ret = ov5640_set_mode(sensor, sensor->current_mode); > >> + ret = ov5640_set_mode(sensor, sensor->last_mode); > >> + > >> if (ret) > >> goto out; > >> > >> -- > >> 2.7.4 > >>
Hi Hugues, one more comment on this patch.. On Mon, Aug 13, 2018 at 12:19:46PM +0200, Hugues Fruchet wrote: > Mode setting depends on last mode set, in particular > because of exposure calculation when downscale mode > change between subsampling and scaling. > At stream on the last mode was wrongly set to current mode, > so no change was detected and exposure calculation > was not made, fix this. > > Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> > --- > drivers/media/i2c/ov5640.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index c110a6a..923cc30 100644 > --- a/drivers/media/i2c/ov5640.c > +++ b/drivers/media/i2c/ov5640.c > @@ -225,6 +225,7 @@ struct ov5640_dev { > struct v4l2_mbus_framefmt fmt; > > const struct ov5640_mode_info *current_mode; > + const struct ov5640_mode_info *last_mode; > enum ov5640_frame_rate current_fr; > struct v4l2_fract frame_interval; > > @@ -1628,6 +1629,9 @@ static int ov5640_set_mode(struct ov5640_dev *sensor, > bool auto_exp = sensor->ctrls.auto_exp->val == V4L2_EXPOSURE_AUTO; > int ret; > > + if (!orig_mode) > + orig_mode = mode; > + Am I wrong or with the introduction of last_mode we could drop the 'orig_mode' parameter (which has confused me already :/ ) from the set_mode() function? Just set here 'orig_mode = sensor->last_mode' and make sure last_mode is intialized properly at probe time... Or is there some other value in keeping the orig_mode parameter here? Thanks j > dn_mode = mode->dn_mode; > orig_dn_mode = orig_mode->dn_mode; > > @@ -1688,6 +1692,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor, > return ret; > > sensor->pending_mode_change = false; > + sensor->last_mode = mode; > > return 0; > > @@ -2551,7 +2556,8 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable) > > if (sensor->streaming == !enable) { > if (enable && sensor->pending_mode_change) { > - ret = ov5640_set_mode(sensor, sensor->current_mode); > + ret = ov5640_set_mode(sensor, sensor->last_mode); > + > if (ret) > goto out; > > -- > 2.7.4 >
Hello Hugues, On Thursday, 16 August 2018 18:07:54 EEST Hugues FRUCHET wrote: > On 08/16/2018 12:10 PM, jacopo mondi wrote: > > On Mon, Aug 13, 2018 at 12:19:46PM +0200, Hugues Fruchet wrote: > > > >> Mode setting depends on last mode set, in particular > >> because of exposure calculation when downscale mode > >> change between subsampling and scaling. > >> At stream on the last mode was wrongly set to current mode, > >> so no change was detected and exposure calculation > >> was not made, fix this. > > > > I actually see a different issue here... > > Which problem do you have exactly, you got a VGA JPEG instead of a QVGA > YUYV ? > > > The issue I see here depends on the format programmed through > > set_fmt() never being applied when using the sensor with a media > > controller equipped device (in this case an i.MX6 board) through > > capture sessions, and the not properly calculated exposure you see may > > be a consequence of this. > > > > I'll try to write down what I see, with the help of some debug output. > > > > - At probe time mode 640x460@30 is programmed: > > > > [ 1.651216] ov5640_probe: Initial mode with id: 2 > > > > - I set the format on the sensor's pad and it gets not applied but > > marked as pending as the sensor is powered off: > > > > #media-ctl --set-v4l2 "'ov5640 2-003c':0[fmt:UYVY2X8/320x240 > > field:none]" > > [ 65.611983] ov5640_set_fmt: NEW mode with id: 1 - PENDING > > So here sensor->current_mode is set to <1>;//QVGA > and sensor->pending_mode_change is set to true; > > > - I start streaming with yavta, and the sensor receives a power on; > > this causes the 'initial' format to be re-programmed and the pending > > change to be ignored: > > > > #yavta -c10 -n4 -f YUYV -s $320x240 -F"../frame-#.yuv" /dev/video4 > > > > [ 69.395018] ov5640_set_power:1805 - on > > [ 69.431342] ov5640_restore_mode:1711 > > [ 69.996882] ov5640_set_mode: Apply mode with id: 0 > > > > The 'ov5640_set_mode()' call from 'ov5640_restore_mode()' clears the > > sensor->pending flag, discarding the newly requested format, for > > this reason, at s_stream() time, the pending flag is not set > > anymore. > > OK but before clearing sensor->pending_mode_change, set_mode() is > loading registers corresponding to sensor->current_mode: > static int ov5640_set_mode(struct ov5640_dev *sensor, > const struct ov5640_mode_info *orig_mode) > { > ==> const struct ov5640_mode_info *mode = sensor->current_mode; > ... > ret = ov5640_set_mode_direct(sensor, mode, exposure); > > => so mode <1> is expected to be set now, so I don't understand your trace: > "> [ 69.996882] ov5640_set_mode: Apply mode with id: 0" > Which variable do you trace that shows "0" ? > > > Are you using a media-controller system? I suspect in non-mc cases, > > the set_fmt is applied through a single power_on/power_off session, not > > causing the 'restore_mode()' issue. Is this the case for you or your > > issue is differnt? > > > > Edit: > > Mita-san tried to address the issue of the output pixel format not > > being restored when the image format was restored in > > 19ad26f9e6e1 ("media: ov5640: add missing output pixel format setting") > > > > I understand the issue he tried to fix, but shouldn't the pending > > format (if any) be applied instead of the initial one unconditionally? > > This is what does the ov5640_restore_mode(), set the current mode > (sensor->current_mode), that is done through this line: > /* now restore the last capture mode */ > ret = ov5640_set_mode(sensor, &ov5640_mode_init_data); > => note that the comment above is weird, in fact it is the "current" > mode that is set. > => note also that the 2nd parameter is not the mode to be set but the > previously applied mode ! (ie loaded in ov5640 registers). This is used > to decide if we have to go to the "set_mode_exposure_calc" or > "set_mode_direct". > > the ov5640_restore_mode() also set the current pixel format > (sensor->fmt), that is done through this line: > return ov5640_set_framefmt(sensor, &sensor->fmt); > ==> This is what have fixed Mita-san, this line was missing previously, > leading to "mode registers" being loaded but not the "pixel format > registers". This seems overly complicated to me. Why do we have to set the mode at power on time at all, why can't we do it at stream on time only, and simplify all this logic ? > PS: There are two other "set mode" related changes that are related to > this: > 1) 6949d864776e ("media: ov5640: do not change mode if format or > frame interval is unchanged") > => this is merged in media master, unfortunately I've introduced a > regression on "pixel format" side that I've fixed in this patchset : > 2) https://www.mail-archive.com/linux-media@vger.kernel.org/msg134413.html > Symptom was a noisy image when capturing QVGA YUV (in fact captured as > JPEG data). [snip]
Hi Laurent, Steve, On 09/07/2018 04:18 PM, Laurent Pinchart wrote: > Hello Hugues, > > On Thursday, 16 August 2018 18:07:54 EEST Hugues FRUCHET wrote: >> On 08/16/2018 12:10 PM, jacopo mondi wrote: >>> On Mon, Aug 13, 2018 at 12:19:46PM +0200, Hugues Fruchet wrote: >>> >>>> Mode setting depends on last mode set, in particular >>>> because of exposure calculation when downscale mode >>>> change between subsampling and scaling. >>>> At stream on the last mode was wrongly set to current mode, >>>> so no change was detected and exposure calculation >>>> was not made, fix this. >>> >>> I actually see a different issue here... >> >> Which problem do you have exactly, you got a VGA JPEG instead of a QVGA >> YUYV ? >> >>> The issue I see here depends on the format programmed through >>> set_fmt() never being applied when using the sensor with a media >>> controller equipped device (in this case an i.MX6 board) through >>> capture sessions, and the not properly calculated exposure you see may >>> be a consequence of this. >>> >>> I'll try to write down what I see, with the help of some debug output. >>> >>> - At probe time mode 640x460@30 is programmed: >>> >>> [ 1.651216] ov5640_probe: Initial mode with id: 2 >>> >>> - I set the format on the sensor's pad and it gets not applied but >>> marked as pending as the sensor is powered off: >>> >>> #media-ctl --set-v4l2 "'ov5640 2-003c':0[fmt:UYVY2X8/320x240 >>> field:none]" >>> [ 65.611983] ov5640_set_fmt: NEW mode with id: 1 - PENDING >> >> So here sensor->current_mode is set to <1>;//QVGA >> and sensor->pending_mode_change is set to true; >> >>> - I start streaming with yavta, and the sensor receives a power on; >>> this causes the 'initial' format to be re-programmed and the pending >>> change to be ignored: >>> >>> #yavta -c10 -n4 -f YUYV -s $320x240 -F"../frame-#.yuv" /dev/video4 >>> >>> [ 69.395018] ov5640_set_power:1805 - on >>> [ 69.431342] ov5640_restore_mode:1711 >>> [ 69.996882] ov5640_set_mode: Apply mode with id: 0 >>> >>> The 'ov5640_set_mode()' call from 'ov5640_restore_mode()' clears the >>> sensor->pending flag, discarding the newly requested format, for >>> this reason, at s_stream() time, the pending flag is not set >>> anymore. >> >> OK but before clearing sensor->pending_mode_change, set_mode() is >> loading registers corresponding to sensor->current_mode: >> static int ov5640_set_mode(struct ov5640_dev *sensor, >> const struct ov5640_mode_info *orig_mode) >> { >> ==> const struct ov5640_mode_info *mode = sensor->current_mode; >> ... >> ret = ov5640_set_mode_direct(sensor, mode, exposure); >> >> => so mode <1> is expected to be set now, so I don't understand your trace: >> "> [ 69.996882] ov5640_set_mode: Apply mode with id: 0" >> Which variable do you trace that shows "0" ? >> >>> Are you using a media-controller system? I suspect in non-mc cases, >>> the set_fmt is applied through a single power_on/power_off session, not >>> causing the 'restore_mode()' issue. Is this the case for you or your >>> issue is differnt? >>> >>> Edit: >>> Mita-san tried to address the issue of the output pixel format not >>> being restored when the image format was restored in >>> 19ad26f9e6e1 ("media: ov5640: add missing output pixel format setting") >>> >>> I understand the issue he tried to fix, but shouldn't the pending >>> format (if any) be applied instead of the initial one unconditionally? >> >> This is what does the ov5640_restore_mode(), set the current mode >> (sensor->current_mode), that is done through this line: >> /* now restore the last capture mode */ >> ret = ov5640_set_mode(sensor, &ov5640_mode_init_data); >> => note that the comment above is weird, in fact it is the "current" >> mode that is set. >> => note also that the 2nd parameter is not the mode to be set but the >> previously applied mode ! (ie loaded in ov5640 registers). This is used >> to decide if we have to go to the "set_mode_exposure_calc" or >> "set_mode_direct". >> >> the ov5640_restore_mode() also set the current pixel format >> (sensor->fmt), that is done through this line: >> return ov5640_set_framefmt(sensor, &sensor->fmt); >> ==> This is what have fixed Mita-san, this line was missing previously, >> leading to "mode registers" being loaded but not the "pixel format >> registers". > > This seems overly complicated to me. Why do we have to set the mode at power > on time at all, why can't we do it at stream on time only, and simplify all > this logic ? > I'm not the author of this driver, Steve do you know the origin and the gain to do so ? Anyway, I would prefer that we stabilize currently existing code before going to larger changes. >> PS: There are two other "set mode" related changes that are related to >> this: >> 1) 6949d864776e ("media: ov5640: do not change mode if format or >> frame interval is unchanged") >> => this is merged in media master, unfortunately I've introduced a >> regression on "pixel format" side that I've fixed in this patchset : >> 2) https://www.mail-archive.com/linux-media@vger.kernel.org/msg134413.html >> Symptom was a noisy image when capturing QVGA YUV (in fact captured as >> JPEG data). > > [snip] > BR Hugues.
Hi Hugues, On Monday, 10 September 2018 18:14:45 EEST Hugues FRUCHET wrote: > On 09/07/2018 04:18 PM, Laurent Pinchart wrote: > > On Thursday, 16 August 2018 18:07:54 EEST Hugues FRUCHET wrote: > >> On 08/16/2018 12:10 PM, jacopo mondi wrote: > >>> On Mon, Aug 13, 2018 at 12:19:46PM +0200, Hugues Fruchet wrote: > >>> > >>>> Mode setting depends on last mode set, in particular > >>>> because of exposure calculation when downscale mode > >>>> change between subsampling and scaling. > >>>> At stream on the last mode was wrongly set to current mode, > >>>> so no change was detected and exposure calculation > >>>> was not made, fix this. > >>> > >>> I actually see a different issue here... > >> > >> Which problem do you have exactly, you got a VGA JPEG instead of a QVGA > >> YUYV ? > >> > >>> The issue I see here depends on the format programmed through > >>> set_fmt() never being applied when using the sensor with a media > >>> controller equipped device (in this case an i.MX6 board) through > >>> capture sessions, and the not properly calculated exposure you see may > >>> be a consequence of this. > >>> > >>> I'll try to write down what I see, with the help of some debug output. > >>> > >>> - At probe time mode 640x460@30 is programmed: > >>> > >>> [ 1.651216] ov5640_probe: Initial mode with id: 2 > >>> > >>> - I set the format on the sensor's pad and it gets not applied but > >>> marked as pending as the sensor is powered off: >>> > >>> #media-ctl --set-v4l2 "'ov5640 2-003c':0[fmt:UYVY2X8/320x240 > >>> field:none]" > >>> [ 65.611983] ov5640_set_fmt: NEW mode with id: 1 - PENDING > >> > >> So here sensor->current_mode is set to <1>;//QVGA > >> and sensor->pending_mode_change is set to true; > >> > >>> - I start streaming with yavta, and the sensor receives a power on; > >>> this causes the 'initial' format to be re-programmed and the > >>> pending change to be ignored: > >>> > >>> #yavta -c10 -n4 -f YUYV -s $320x240 -F"../frame-#.yuv" /dev/video4 > >>> > >>> [ 69.395018] ov5640_set_power:1805 - on > >>> [ 69.431342] ov5640_restore_mode:1711 > >>> [ 69.996882] ov5640_set_mode: Apply mode with id: 0 > >>> > >>> The 'ov5640_set_mode()' call from 'ov5640_restore_mode()' clears > >>> the sensor->pending flag, discarding the newly requested format, for > >>> this reason, at s_stream() time, the pending flag is not set > >>> anymore. > >> > >> OK but before clearing sensor->pending_mode_change, set_mode() is > >> loading registers corresponding to sensor->current_mode: > >> > >> static int ov5640_set_mode(struct ov5640_dev *sensor, > >> const struct ov5640_mode_info *orig_mode) > >> { > >> ==> const struct ov5640_mode_info *mode = sensor->current_mode; > >> ... > >> ret = ov5640_set_mode_direct(sensor, mode, exposure); > >> > >> => so mode <1> is expected to be set now, so I don't understand your > >> trace: > >> "> [ 69.996882] ov5640_set_mode: Apply mode with id: 0" > >> Which variable do you trace that shows "0" ? > >> > >>> Are you using a media-controller system? I suspect in non-mc cases, > >>> the set_fmt is applied through a single power_on/power_off session, not > >>> causing the 'restore_mode()' issue. Is this the case for you or your > >>> issue is differnt? > >>> > >>> Edit: > >>> Mita-san tried to address the issue of the output pixel format not > >>> being restored when the image format was restored in > >>> 19ad26f9e6e1 ("media: ov5640: add missing output pixel format setting") > >>> > >>> I understand the issue he tried to fix, but shouldn't the pending > >>> format (if any) be applied instead of the initial one unconditionally? > >> > >> This is what does the ov5640_restore_mode(), set the current mode > >> (sensor->current_mode), that is done through this line: > >> > >> /* now restore the last capture mode */ > >> ret = ov5640_set_mode(sensor, &ov5640_mode_init_data); > >> > >> => note that the comment above is weird, in fact it is the "current" > >> mode that is set. > >> => note also that the 2nd parameter is not the mode to be set but the > >> previously applied mode ! (ie loaded in ov5640 registers). This is used > >> to decide if we have to go to the "set_mode_exposure_calc" or > >> "set_mode_direct". > >> > >> the ov5640_restore_mode() also set the current pixel format > >> (sensor->fmt), that is done through this line: > >> > >> return ov5640_set_framefmt(sensor, &sensor->fmt); > >> > >> ==> This is what have fixed Mita-san, this line was missing previously, > >> leading to "mode registers" being loaded but not the "pixel format > >> registers". > > > > This seems overly complicated to me. Why do we have to set the mode at > > power on time at all, why can't we do it at stream on time only, and > > simplify all this logic ? > > I'm not the author of this driver, Steve do you know the origin and the > gain to do so ? Anyway, I would prefer that we stabilize currently existing > code before going to larger changes. I'm not opposed to that, but it's then pretty hard to review the patches, when they replace hard to read code with other hard to read code :-) Eventually we should really clean this up. > >> PS: There are two other "set mode" related changes that are related to > >> this: > >> 1) 6949d864776e ("media: ov5640: do not change mode if format or > >> frame interval is unchanged") > >> => this is merged in media master, unfortunately I've introduced a > >> regression on "pixel format" side that I've fixed in this patchset : > >> 2) > >> https://www.mail-archive.com/linux-media@vger.kernel.org/msg134413.html > >> Symptom was a noisy image when capturing QVGA YUV (in fact captured as > >> JPEG data). > > > > [snip]
Hi Jacopo, On 08/25/2018 04:53 PM, jacopo mondi wrote: > Hi Hugues, > one more comment on this patch.. > > On Mon, Aug 13, 2018 at 12:19:46PM +0200, Hugues Fruchet wrote: >> Mode setting depends on last mode set, in particular >> because of exposure calculation when downscale mode >> change between subsampling and scaling. >> At stream on the last mode was wrongly set to current mode, >> so no change was detected and exposure calculation >> was not made, fix this. >> >> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> >> --- >> drivers/media/i2c/ov5640.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c >> index c110a6a..923cc30 100644 >> --- a/drivers/media/i2c/ov5640.c >> +++ b/drivers/media/i2c/ov5640.c >> @@ -225,6 +225,7 @@ struct ov5640_dev { >> struct v4l2_mbus_framefmt fmt; >> >> const struct ov5640_mode_info *current_mode; >> + const struct ov5640_mode_info *last_mode; >> enum ov5640_frame_rate current_fr; >> struct v4l2_fract frame_interval; >> >> @@ -1628,6 +1629,9 @@ static int ov5640_set_mode(struct ov5640_dev *sensor, >> bool auto_exp = sensor->ctrls.auto_exp->val == V4L2_EXPOSURE_AUTO; >> int ret; >> >> + if (!orig_mode) >> + orig_mode = mode; >> + > > Am I wrong or with the introduction of last_mode we could drop the > 'orig_mode' parameter (which has confused me already :/ ) from the > set_mode() function? > > Just set here 'orig_mode = sensor->last_mode' and make sure last_mode > is intialized properly at probe time... > > Or is there some other value in keeping the orig_mode parameter here? > > Thanks > j I'm fine with that change, will push it in v3. > >> dn_mode = mode->dn_mode; >> orig_dn_mode = orig_mode->dn_mode; >> >> @@ -1688,6 +1692,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor, >> return ret; >> >> sensor->pending_mode_change = false; >> + sensor->last_mode = mode; >> >> return 0; >> >> @@ -2551,7 +2556,8 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable) >> >> if (sensor->streaming == !enable) { >> if (enable && sensor->pending_mode_change) { >> - ret = ov5640_set_mode(sensor, sensor->current_mode); >> + ret = ov5640_set_mode(sensor, sensor->last_mode); >> + >> if (ret) >> goto out; >> >> -- >> 2.7.4 >> BR Hugues.
Hi Laurent, On 09/10/2018 10:56 PM, Laurent Pinchart wrote: > Hi Hugues, > > On Monday, 10 September 2018 18:14:45 EEST Hugues FRUCHET wrote: >> On 09/07/2018 04:18 PM, Laurent Pinchart wrote: >>> On Thursday, 16 August 2018 18:07:54 EEST Hugues FRUCHET wrote: >>>> On 08/16/2018 12:10 PM, jacopo mondi wrote: >>>>> On Mon, Aug 13, 2018 at 12:19:46PM +0200, Hugues Fruchet wrote: >>>>> >>>>>> Mode setting depends on last mode set, in particular >>>>>> because of exposure calculation when downscale mode >>>>>> change between subsampling and scaling. >>>>>> At stream on the last mode was wrongly set to current mode, >>>>>> so no change was detected and exposure calculation >>>>>> was not made, fix this. >>>>> >>>>> I actually see a different issue here... >>>> >>>> Which problem do you have exactly, you got a VGA JPEG instead of a QVGA >>>> YUYV ? >>>> >>>>> The issue I see here depends on the format programmed through >>>>> set_fmt() never being applied when using the sensor with a media >>>>> controller equipped device (in this case an i.MX6 board) through >>>>> capture sessions, and the not properly calculated exposure you see may >>>>> be a consequence of this. >>>>> >>>>> I'll try to write down what I see, with the help of some debug output. >>>>> >>>>> - At probe time mode 640x460@30 is programmed: >>>>> >>>>> [ 1.651216] ov5640_probe: Initial mode with id: 2 >>>>> >>>>> - I set the format on the sensor's pad and it gets not applied but >>>>> marked as pending as the sensor is powered off: > >>> >>>>> #media-ctl --set-v4l2 "'ov5640 2-003c':0[fmt:UYVY2X8/320x240 >>>>> field:none]" >>>>> [ 65.611983] ov5640_set_fmt: NEW mode with id: 1 - PENDING >>>> >>>> So here sensor->current_mode is set to <1>;//QVGA >>>> and sensor->pending_mode_change is set to true; >>>> >>>>> - I start streaming with yavta, and the sensor receives a power on; >>>>> this causes the 'initial' format to be re-programmed and the >>>>> pending change to be ignored: >>>>> >>>>> #yavta -c10 -n4 -f YUYV -s $320x240 -F"../frame-#.yuv" /dev/video4 >>>>> >>>>> [ 69.395018] ov5640_set_power:1805 - on >>>>> [ 69.431342] ov5640_restore_mode:1711 >>>>> [ 69.996882] ov5640_set_mode: Apply mode with id: 0 >>>>> >>>>> The 'ov5640_set_mode()' call from 'ov5640_restore_mode()' clears >>>>> the sensor->pending flag, discarding the newly requested format, for >>>>> this reason, at s_stream() time, the pending flag is not set >>>>> anymore. >>>> >>>> OK but before clearing sensor->pending_mode_change, set_mode() is >>>> loading registers corresponding to sensor->current_mode: >>>> >>>> static int ov5640_set_mode(struct ov5640_dev *sensor, >>>> const struct ov5640_mode_info *orig_mode) >>>> { >>>> ==> const struct ov5640_mode_info *mode = sensor->current_mode; >>>> ... >>>> ret = ov5640_set_mode_direct(sensor, mode, exposure); >>>> >>>> => so mode <1> is expected to be set now, so I don't understand your >>>> trace: >>>> "> [ 69.996882] ov5640_set_mode: Apply mode with id: 0" >>>> Which variable do you trace that shows "0" ? >>>> >>>>> Are you using a media-controller system? I suspect in non-mc cases, >>>>> the set_fmt is applied through a single power_on/power_off session, not >>>>> causing the 'restore_mode()' issue. Is this the case for you or your >>>>> issue is differnt? >>>>> >>>>> Edit: >>>>> Mita-san tried to address the issue of the output pixel format not >>>>> being restored when the image format was restored in >>>>> 19ad26f9e6e1 ("media: ov5640: add missing output pixel format setting") >>>>> >>>>> I understand the issue he tried to fix, but shouldn't the pending >>>>> format (if any) be applied instead of the initial one unconditionally? >>>> >>>> This is what does the ov5640_restore_mode(), set the current mode >>>> (sensor->current_mode), that is done through this line: >>>> >>>> /* now restore the last capture mode */ >>>> ret = ov5640_set_mode(sensor, &ov5640_mode_init_data); >>>> >>>> => note that the comment above is weird, in fact it is the "current" >>>> mode that is set. >>>> => note also that the 2nd parameter is not the mode to be set but the >>>> previously applied mode ! (ie loaded in ov5640 registers). This is used >>>> to decide if we have to go to the "set_mode_exposure_calc" or >>>> "set_mode_direct". >>>> >>>> the ov5640_restore_mode() also set the current pixel format >>>> (sensor->fmt), that is done through this line: >>>> >>>> return ov5640_set_framefmt(sensor, &sensor->fmt); >>>> >>>> ==> This is what have fixed Mita-san, this line was missing previously, >>>> leading to "mode registers" being loaded but not the "pixel format >>>> registers". >>> >>> This seems overly complicated to me. Why do we have to set the mode at >>> power on time at all, why can't we do it at stream on time only, and >>> simplify all this logic ? >> >> I'm not the author of this driver, Steve do you know the origin and the >> gain to do so ? Anyway, I would prefer that we stabilize currently existing >> code before going to larger changes. > > I'm not opposed to that, but it's then pretty hard to review the patches, when > they replace hard to read code with other hard to read code :-) > > Eventually we should really clean this up. No problem to cleanup that code, I will be really happy to simplify that stuff, but there are lot of stakeholders now so better to isolate that exact change in a new serie and ask for a non-regression campaign. > >>>> PS: There are two other "set mode" related changes that are related to >>>> this: >>>> 1) 6949d864776e ("media: ov5640: do not change mode if format or >>>> frame interval is unchanged") >>>> => this is merged in media master, unfortunately I've introduced a >>>> regression on "pixel format" side that I've fixed in this patchset : >>>> 2) >>>> https://www.mail-archive.com/linux-media@vger.kernel.org/msg134413.html >>>> Symptom was a noisy image when capturing QVGA YUV (in fact captured as >>>> JPEG data). >>> >>> [snip] > BR Hugues.
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c index c110a6a..923cc30 100644 --- a/drivers/media/i2c/ov5640.c +++ b/drivers/media/i2c/ov5640.c @@ -225,6 +225,7 @@ struct ov5640_dev { struct v4l2_mbus_framefmt fmt; const struct ov5640_mode_info *current_mode; + const struct ov5640_mode_info *last_mode; enum ov5640_frame_rate current_fr; struct v4l2_fract frame_interval; @@ -1628,6 +1629,9 @@ static int ov5640_set_mode(struct ov5640_dev *sensor, bool auto_exp = sensor->ctrls.auto_exp->val == V4L2_EXPOSURE_AUTO; int ret; + if (!orig_mode) + orig_mode = mode; + dn_mode = mode->dn_mode; orig_dn_mode = orig_mode->dn_mode; @@ -1688,6 +1692,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor, return ret; sensor->pending_mode_change = false; + sensor->last_mode = mode; return 0; @@ -2551,7 +2556,8 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable) if (sensor->streaming == !enable) { if (enable && sensor->pending_mode_change) { - ret = ov5640_set_mode(sensor, sensor->current_mode); + ret = ov5640_set_mode(sensor, sensor->last_mode); + if (ret) goto out;
Mode setting depends on last mode set, in particular because of exposure calculation when downscale mode change between subsampling and scaling. At stream on the last mode was wrongly set to current mode, so no change was detected and exposure calculation was not made, fix this. Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> --- drivers/media/i2c/ov5640.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)