Hi Jacopo,
On Wednesday, 10 October 2018 13:58:04 EEST jacopo mondi wrote:
> Hi Sam,
> thanks for the patch, I see the same issue you reported, but I
> think this patch can be improved.
>
> (expanding the Cc list to all people involved in recent ov5640
> developemts, not just for this patch, but for the whole series to look
> at. Copying names from another series cover letter, hope it is
> complete.)
>
> On Mon, Oct 08, 2018 at 11:47:59PM -0700, Sam Bobrowicz wrote:
> > set_fmt was not properly triggering a mode change when
> > a new mode was set that happened to have the same format
> > as the previous mode (for example, when only changing the
> > frame dimensions). Fix this.
> >
> > Signed-off-by: Sam Bobrowicz <sam@xxxxxxxxxxxxxxxxxx>
> > ---
> >
> > drivers/media/i2c/ov5640.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index eaefdb5..5031aab 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -2045,12 +2045,12 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
> >
> > goto out;
> >
> > }
> >
> > - if (new_mode != sensor->current_mode) {
> > +
> > + if (new_mode != sensor->current_mode ||
> > + mbus_fmt->code != sensor->fmt.code) {
> > + sensor->fmt = *mbus_fmt;
> >
> > sensor->current_mode = new_mode;
> > sensor->pending_mode_change = true;
> >
> > - }
> > - if (mbus_fmt->code != sensor->fmt.code) {
> > - sensor->fmt = *mbus_fmt;
> >
> > sensor->pending_fmt_change = true;
> >
> > }
>
> How I did reproduce the issue:
>
> # Set 1024x768 on ov5640 without changing the image format
> # (default image size at startup is 640x480)
> $ media-ctl --set-v4l2 "'ov5640 2-003c':0[fmt:UYVY2X8/1024x768 field:none]"
> sensor->pending_mode_change = true; //verified this flag gets set
>
> # Start streaming, after having configured the whole pipeline to work
> # with 1024x768
> $ yavta -c10 -n4 -f UYVY -s 1024x768 /dev/video4
> Unable to start streaming: Broken pipe (32).
>
> # Inspect which part of pipeline validation went wrong
> # Turns out the sensor->fmt field is not updated, and when get_fmt()
> # is called, the old one is returned.
> $ media-ctl -e "ov5640 2-003c" -p
> ...
> [fmt:UYVY8_2X8/640x480@1/30 field:none colorspace:srgb xfer:srgb ycbcr:601
> quantization:full-range] ^^^ ^^^
>
> So yes, sensor->fmt is not udapted as it should be when only image
> resolution is changed.
>
> Although I still see value in having two separate flags for the
> 'mode_change' (which in ov5640 lingo is resolution) and 'fmt_change' (which
> in ov5640 lingo is the image format), and write their configuration to
> registers only when they get actually changed.
>
> For this reasons I would like to propse the following patch which I
> have tested by:
> 1) changing resolution only
> 2) changing format only
> 3) change both
>
> What do you and others think?
I think that the format setting code should be completely rewritten, it's
pretty much unmaintainable as-is.
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index eaefdb5..e392b9d 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -2020,6 +2020,7 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
> struct ov5640_dev *sensor = to_ov5640_dev(sd);
> const struct ov5640_mode_info *new_mode;
> struct v4l2_mbus_framefmt *mbus_fmt = &format->format;
> + struct v4l2_mbus_framefmt *fmt;
> int ret;
>
> if (format->pad != 0)
> @@ -2037,22 +2038,19 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
> if (ret)
> goto out;
>
> - if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> - struct v4l2_mbus_framefmt *fmt =
> - v4l2_subdev_get_try_format(sd, cfg, 0);
> + if (format->which == V4L2_SUBDEV_FORMAT_TRY)
> + fmt = v4l2_subdev_get_try_format(sd, cfg, 0);
> + else
> + fmt = &sensor->fmt;
>
> - *fmt = *mbus_fmt;
> - goto out;
> - }
> + *fmt = *mbus_fmt;
>
> if (new_mode != sensor->current_mode) {
> sensor->current_mode = new_mode;
> sensor->pending_mode_change = true;
> }
> - if (mbus_fmt->code != sensor->fmt.code) {
> - sensor->fmt = *mbus_fmt;
> + if (mbus_fmt->code != sensor->fmt.code)
> sensor->pending_fmt_change = true;
> - }
> out:
> mutex_unlock(&sensor->lock);
> return ret;
>
> > out:
> > --
> > 2.7.4
--
Regards,
Laurent Pinchart
