* [PATCH 0/4] ov5640: small fixes for compatibility
@ 2018-10-09 6:47 Sam Bobrowicz
2018-10-09 6:47 ` [PATCH 1/4] media: ov5640: fix resolution update Sam Bobrowicz
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: Sam Bobrowicz @ 2018-10-09 6:47 UTC (permalink / raw)
To: linux-media; +Cc: Sam Bobrowicz
These patches include fixes for some minor annoyances and also help
increase compatibility with some CSI2 drivers.
Based on latest media_tree commit: "media: ov5640: fix framerate update"
Sam Bobrowicz (4):
media: ov5640: fix resolution update
media: ov5640: fix get_light_freq on auto
media: ov5640: Don't access ctrl regs when off
media: ov5640: Add additional media bus formats
drivers/media/i2c/ov5640.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH 1/4] media: ov5640: fix resolution update 2018-10-09 6:47 [PATCH 0/4] ov5640: small fixes for compatibility Sam Bobrowicz @ 2018-10-09 6:47 ` Sam Bobrowicz 2018-10-10 10:58 ` jacopo mondi 2018-10-09 6:48 ` [PATCH 2/4] media: ov5640: fix get_light_freq on auto Sam Bobrowicz ` (2 subsequent siblings) 3 siblings, 1 reply; 15+ messages in thread From: Sam Bobrowicz @ 2018-10-09 6:47 UTC (permalink / raw) To: linux-media; +Cc: Sam Bobrowicz 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@elite-embedded.com> --- 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; } out: -- 2.7.4 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] media: ov5640: fix resolution update 2018-10-09 6:47 ` [PATCH 1/4] media: ov5640: fix resolution update Sam Bobrowicz @ 2018-10-10 10:58 ` jacopo mondi 2018-10-10 12:41 ` Laurent Pinchart 0 siblings, 1 reply; 15+ messages in thread From: jacopo mondi @ 2018-10-10 10:58 UTC (permalink / raw) To: Sam Bobrowicz Cc: linux-media, Laurent Pinchart, Hugues Fruchet, loic.poulain, slongerbeam, daniel, maxime.ripard [-- Attachment #1: Type: text/plain, Size: 4223 bytes --] 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@elite-embedded.com> > --- > 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? Thanks j 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 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] media: ov5640: fix resolution update 2018-10-10 10:58 ` jacopo mondi @ 2018-10-10 12:41 ` Laurent Pinchart 2018-10-15 15:13 ` Hugues FRUCHET 0 siblings, 1 reply; 15+ messages in thread From: Laurent Pinchart @ 2018-10-10 12:41 UTC (permalink / raw) To: jacopo mondi Cc: Sam Bobrowicz, linux-media, Hugues Fruchet, loic.poulain, slongerbeam, daniel, maxime.ripard 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@elite-embedded.com> > > --- > > > > 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] media: ov5640: fix resolution update 2018-10-10 12:41 ` Laurent Pinchart @ 2018-10-15 15:13 ` Hugues FRUCHET 2018-10-15 15:24 ` jacopo mondi 2018-10-16 12:15 ` Laurent Pinchart 0 siblings, 2 replies; 15+ messages in thread From: Hugues FRUCHET @ 2018-10-15 15:13 UTC (permalink / raw) To: Laurent Pinchart, jacopo mondi, Sam Bobrowicz, slongerbeam Cc: linux-media, loic.poulain, daniel, maxime.ripard Hi Laurent, Jacopo, Sam, I'm also OK to change to a simpler alternative; - drop the "restore" step - send the whole init register sequence + mode changes + format changes at streamon is this what you have in mind Laurent ? On 10/10/2018 02:41 PM, Laurent Pinchart wrote: > 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@elite-embedded.com> >>> --- >>> >>> 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 > > BR, Hugues. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] media: ov5640: fix resolution update 2018-10-15 15:13 ` Hugues FRUCHET @ 2018-10-15 15:24 ` jacopo mondi 2018-10-16 8:49 ` Hugues FRUCHET 2018-10-16 12:15 ` Laurent Pinchart 1 sibling, 1 reply; 15+ messages in thread From: jacopo mondi @ 2018-10-15 15:24 UTC (permalink / raw) To: Hugues FRUCHET Cc: Laurent Pinchart, Sam Bobrowicz, slongerbeam, linux-media, loic.poulain, daniel, maxime.ripard [-- Attachment #1: Type: text/plain, Size: 7309 bytes --] Hi Hugues, On Mon, Oct 15, 2018 at 03:13:12PM +0000, Hugues FRUCHET wrote: > Hi Laurent, Jacopo, Sam, > > I'm also OK to change to a simpler alternative; > - drop the "restore" step Do you mean the restore step at the end of 'ov5640_restore_mode()' ? I agree, I've been carrying this one [1] in my tree for some time now. I just didn't send it because of the too many issues in flight on this driver. > - send the whole init register sequence + mode changes + format changes > at streamon > This might be a first step in my opinion too, yes. > is this what you have in mind Laurent ? I know Laurent does not fully agree with me on this, but I would like to have Maxime's series on clock tree handling merged and tested on CSI-2 first before adding more patches to the pile of pending items on ov5640. I hope to have time to test them on CSI-2 this week before ELC-E. Steve, you're the driver maintainer do you have preferences here? Also, if this might be useful, I would like to help co-maintaining the driver (with possibily other people, possibly with the sensor wired in DVP mode), and help establishing priorities, as this driver needs some love, but one item at the time to avoid getting lost in too many pending changes as it happened recently :) Thanks j [1] media: ov5640: Do not restore format at power up Do not force restoring the last applied capture format during sensor power up as it will be re-set at s_stream time. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c index b226946..17ee55b 100644 --- a/drivers/media/i2c/ov5640.c +++ b/drivers/media/i2c/ov5640.c @@ -1737,12 +1737,10 @@ static int ov5640_restore_mode(struct ov5640_dev *sensor) if (ret) return ret; - /* now restore the last capture mode */ - ret = ov5640_set_mode(sensor, &ov5640_mode_init_data); - if (ret < 0) - return ret; + sensor->pending_mode_change = true; + sensor->pending_fmt_change = true; - return ov5640_set_framefmt(sensor, &sensor->fmt); + return 0; } static void ov5640_power(struct ov5640_dev *sensor, bool enable) > > On 10/10/2018 02:41 PM, Laurent Pinchart wrote: > > 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@elite-embedded.com> > >>> --- > >>> > >>> 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 > > > > > > BR, > Hugues. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] media: ov5640: fix resolution update 2018-10-15 15:24 ` jacopo mondi @ 2018-10-16 8:49 ` Hugues FRUCHET 0 siblings, 0 replies; 15+ messages in thread From: Hugues FRUCHET @ 2018-10-16 8:49 UTC (permalink / raw) To: jacopo mondi Cc: Laurent Pinchart, Sam Bobrowicz, slongerbeam, linux-media, loic.poulain, daniel, maxime.ripard Hi Jacopo, On 10/15/2018 05:24 PM, jacopo mondi wrote: > Hi Hugues, > > On Mon, Oct 15, 2018 at 03:13:12PM +0000, Hugues FRUCHET wrote: >> Hi Laurent, Jacopo, Sam, >> >> I'm also OK to change to a simpler alternative; >> - drop the "restore" step > > Do you mean the restore step at the end of 'ov5640_restore_mode()' ? yes > I agree, I've been carrying this one [1] in my tree for some time now. > I just didn't send it because of the too many issues in flight on this > driver. > >> - send the whole init register sequence + mode changes + format changes >> at streamon >> > > This might be a first step in my opinion too, yes. > >> is this what you have in mind Laurent ? > > I know Laurent does not fully agree with me on this, but I would like > to have Maxime's series on clock tree handling merged and tested on > CSI-2 first before adding more patches to the pile of pending items on > ov5640. I hope to have time to test them on CSI-2 this week before > ELC-E. > > Steve, you're the driver maintainer do you have preferences here? > > Also, if this might be useful, I would like to help co-maintaining the > driver (with possibily other people, possibly with the sensor wired in > DVP mode), and help establishing priorities, as this driver needs some > love, but one item at the time to avoid getting lost in too many > pending changes as it happened recently :) It's a good problem having too many contributions ;) This means that there is some interest on it... For DVP, me and Maxime have two different setup so we can cover the DVP tests. > > Thanks > j > > [1] > media: ov5640: Do not restore format at power up > > Do not force restoring the last applied capture format during sensor power up > as it will be re-set at s_stream time. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index b226946..17ee55b 100644 > --- a/drivers/media/i2c/ov5640.c > +++ b/drivers/media/i2c/ov5640.c > @@ -1737,12 +1737,10 @@ static int ov5640_restore_mode(struct ov5640_dev *sensor) > if (ret) > return ret; > > - /* now restore the last capture mode */ > - ret = ov5640_set_mode(sensor, &ov5640_mode_init_data); > - if (ret < 0) > - return ret; > + sensor->pending_mode_change = true; > + sensor->pending_fmt_change = true; > > - return ov5640_set_framefmt(sensor, &sensor->fmt); > + return 0; > } > > static void ov5640_power(struct ov5640_dev *sensor, bool enable) > >> >> On 10/10/2018 02:41 PM, Laurent Pinchart wrote: >>> 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@elite-embedded.com> >>>>> --- >>>>> >>>>> 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 >>> >>> >> >> BR, >> Hugues. BR, Hugues. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] media: ov5640: fix resolution update 2018-10-15 15:13 ` Hugues FRUCHET 2018-10-15 15:24 ` jacopo mondi @ 2018-10-16 12:15 ` Laurent Pinchart 2018-10-16 18:14 ` Samuel Bobrowicz 1 sibling, 1 reply; 15+ messages in thread From: Laurent Pinchart @ 2018-10-16 12:15 UTC (permalink / raw) To: Hugues FRUCHET Cc: jacopo mondi, Sam Bobrowicz, slongerbeam, linux-media, loic.poulain, daniel, maxime.ripard Hi Hugues, On Monday, 15 October 2018 18:13:12 EEST Hugues FRUCHET wrote: > Hi Laurent, Jacopo, Sam, > > I'm also OK to change to a simpler alternative; > - drop the "restore" step > - send the whole init register sequence + mode changes + format changes > at streamon > > is this what you have in mind Laurent ? Yes, that's pretty much the idea. The init sequence could be sent when powering the sensor on to save time at streamon. Everything else can be programmed at streamon time to simplify the implementation. > On 10/10/2018 02:41 PM, Laurent Pinchart wrote: > > 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@elite-embedded.com> > >>> --- > >>> > >>> 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: -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] media: ov5640: fix resolution update 2018-10-16 12:15 ` Laurent Pinchart @ 2018-10-16 18:14 ` Samuel Bobrowicz 0 siblings, 0 replies; 15+ messages in thread From: Samuel Bobrowicz @ 2018-10-16 18:14 UTC (permalink / raw) To: Laurent Pinchart Cc: Hugues FRUCHET, jacopo mondi, slongerbeam, linux-media, loic.poulain, daniel, maxime.ripard Glad this is spurring a lot of conversation, and I’m happy to see this many contributors too. I think we have all solved many of these problems (and the many others) offline, and now it’s the hard part to try to glue them all together. I decided to jump back in the mix with these patches because they seemed that they would be the easiest to rebase and were obvious bug fixes. I was hoping we could get this one applied and have a better starting place to work from before we start what is a very necessary restructure of the mode setting, but if everybody is ready to go now, we can just do a larger overhaul and I can withdraw this patch from this series. I expect the larger overhaul to warrant its own patch series. I’m not going to comment yet on some of the proposed changes below, because I think it is better if we just move the discussion to its own series. Another note, I also agree that prior to changing the rest of mode setting, the next step is to introduce maximes critical pll setting patch. It is required for my platform to work at all (yes my platform has never worked with main line) because my CSI2 receiver needs the PCLK PERIOD register to be set properly. I actually have tweaked Maximes original patch so that it doesn’t introduce a regression for MIPI platforms, but it will need to be tested on DVP platforms. I’ll send it out RFC tonight, so hopefully that will get us closer to a single patch that doesn’t break things in either mode. IMO I would like to see this patch broken out of Maximes larger series, because I think it is getting bogged down by the many other changes there. With this many interested parties, someone would really be helping if they put out a organized to do list that we could schedule out :) > On Oct 16, 2018, at 5:15 AM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Hugues, > >> On Monday, 15 October 2018 18:13:12 EEST Hugues FRUCHET wrote: >> Hi Laurent, Jacopo, Sam, >> >> I'm also OK to change to a simpler alternative; >> - drop the "restore" step >> - send the whole init register sequence + mode changes + format changes >> at streamon >> >> is this what you have in mind Laurent ? > > Yes, that's pretty much the idea. The init sequence could be sent when > powering the sensor on to save time at streamon. Everything else can be > programmed at streamon time to simplify the implementation. > >>> On 10/10/2018 02:41 PM, Laurent Pinchart wrote: >>>> 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@elite-embedded.com> >>>>> --- >>>>> >>>>> 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: > > -- > Regards, > > Laurent Pinchart > > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/4] media: ov5640: fix get_light_freq on auto 2018-10-09 6:47 [PATCH 0/4] ov5640: small fixes for compatibility Sam Bobrowicz 2018-10-09 6:47 ` [PATCH 1/4] media: ov5640: fix resolution update Sam Bobrowicz @ 2018-10-09 6:48 ` Sam Bobrowicz 2018-10-10 8:49 ` jacopo mondi 2018-10-09 6:48 ` [PATCH 3/4] media: ov5640: Don't access ctrl regs when off Sam Bobrowicz 2018-10-09 6:48 ` [PATCH 4/4] media: ov5640: Add additional media bus formats Sam Bobrowicz 3 siblings, 1 reply; 15+ messages in thread From: Sam Bobrowicz @ 2018-10-09 6:48 UTC (permalink / raw) To: linux-media; +Cc: Sam Bobrowicz Light frequency was not properly returned when in auto mode and the detected frequency was 60Hz. Fix this. Signed-off-by: Sam Bobrowicz <sam@elite-embedded.com> --- drivers/media/i2c/ov5640.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c index 5031aab..f183222 100644 --- a/drivers/media/i2c/ov5640.c +++ b/drivers/media/i2c/ov5640.c @@ -1295,6 +1295,7 @@ static int ov5640_get_light_freq(struct ov5640_dev *sensor) light_freq = 50; } else { /* 60Hz */ + light_freq = 60; } } -- 2.7.4 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/4] media: ov5640: fix get_light_freq on auto 2018-10-09 6:48 ` [PATCH 2/4] media: ov5640: fix get_light_freq on auto Sam Bobrowicz @ 2018-10-10 8:49 ` jacopo mondi 0 siblings, 0 replies; 15+ messages in thread From: jacopo mondi @ 2018-10-10 8:49 UTC (permalink / raw) To: Sam Bobrowicz; +Cc: linux-media [-- Attachment #1: Type: text/plain, Size: 764 bytes --] Hi Sam, On Mon, Oct 08, 2018 at 11:48:00PM -0700, Sam Bobrowicz wrote: > Light frequency was not properly returned when in auto > mode and the detected frequency was 60Hz. Fix this. > > Signed-off-by: Sam Bobrowicz <sam@elite-embedded.com> This is indeed a bugfix Acked-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > --- > drivers/media/i2c/ov5640.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index 5031aab..f183222 100644 > --- a/drivers/media/i2c/ov5640.c > +++ b/drivers/media/i2c/ov5640.c > @@ -1295,6 +1295,7 @@ static int ov5640_get_light_freq(struct ov5640_dev *sensor) > light_freq = 50; > } else { > /* 60Hz */ > + light_freq = 60; > } > } > > -- > 2.7.4 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/4] media: ov5640: Don't access ctrl regs when off 2018-10-09 6:47 [PATCH 0/4] ov5640: small fixes for compatibility Sam Bobrowicz 2018-10-09 6:47 ` [PATCH 1/4] media: ov5640: fix resolution update Sam Bobrowicz 2018-10-09 6:48 ` [PATCH 2/4] media: ov5640: fix get_light_freq on auto Sam Bobrowicz @ 2018-10-09 6:48 ` Sam Bobrowicz 2018-10-10 8:48 ` jacopo mondi 2018-10-09 6:48 ` [PATCH 4/4] media: ov5640: Add additional media bus formats Sam Bobrowicz 3 siblings, 1 reply; 15+ messages in thread From: Sam Bobrowicz @ 2018-10-09 6:48 UTC (permalink / raw) To: linux-media; +Cc: Sam Bobrowicz Add a check to g_volatile_ctrl to prevent trying to read registers when the sensor is not powered. Signed-off-by: Sam Bobrowicz <sam@elite-embedded.com> --- drivers/media/i2c/ov5640.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c index f183222..a50d884 100644 --- a/drivers/media/i2c/ov5640.c +++ b/drivers/media/i2c/ov5640.c @@ -2336,6 +2336,13 @@ static int ov5640_g_volatile_ctrl(struct v4l2_ctrl *ctrl) /* v4l2_ctrl_lock() locks our own mutex */ + /* + * If the sensor is not powered up by the host driver, do + * not try to access it to update the volatile controls. + */ + if (sensor->power_count == 0) + return 0; + switch (ctrl->id) { case V4L2_CID_AUTOGAIN: val = ov5640_get_gain(sensor); -- 2.7.4 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 3/4] media: ov5640: Don't access ctrl regs when off 2018-10-09 6:48 ` [PATCH 3/4] media: ov5640: Don't access ctrl regs when off Sam Bobrowicz @ 2018-10-10 8:48 ` jacopo mondi 0 siblings, 0 replies; 15+ messages in thread From: jacopo mondi @ 2018-10-10 8:48 UTC (permalink / raw) To: Sam Bobrowicz; +Cc: linux-media [-- Attachment #1: Type: text/plain, Size: 1238 bytes --] Hi Sam, thanks for the patch. On Mon, Oct 08, 2018 at 11:48:01PM -0700, Sam Bobrowicz wrote: > Add a check to g_volatile_ctrl to prevent trying to read > registers when the sensor is not powered. > > Signed-off-by: Sam Bobrowicz <sam@elite-embedded.com> I've been carrying a similar patch in my tree. I found it is required when the sensor control handler is add to the receiver driver control handler, and thus g_voltaile_ctrl can be called when the sensor is powered off. Please add my: Acked-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > --- > drivers/media/i2c/ov5640.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index f183222..a50d884 100644 > --- a/drivers/media/i2c/ov5640.c > +++ b/drivers/media/i2c/ov5640.c > @@ -2336,6 +2336,13 @@ static int ov5640_g_volatile_ctrl(struct v4l2_ctrl *ctrl) > > /* v4l2_ctrl_lock() locks our own mutex */ > > + /* > + * If the sensor is not powered up by the host driver, do > + * not try to access it to update the volatile controls. > + */ > + if (sensor->power_count == 0) > + return 0; > + > switch (ctrl->id) { > case V4L2_CID_AUTOGAIN: > val = ov5640_get_gain(sensor); > -- > 2.7.4 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 4/4] media: ov5640: Add additional media bus formats 2018-10-09 6:47 [PATCH 0/4] ov5640: small fixes for compatibility Sam Bobrowicz ` (2 preceding siblings ...) 2018-10-09 6:48 ` [PATCH 3/4] media: ov5640: Don't access ctrl regs when off Sam Bobrowicz @ 2018-10-09 6:48 ` Sam Bobrowicz 2018-11-16 13:18 ` Sakari Ailus 3 siblings, 1 reply; 15+ messages in thread From: Sam Bobrowicz @ 2018-10-09 6:48 UTC (permalink / raw) To: linux-media; +Cc: Sam Bobrowicz Add support for 1X16 yuv media bus formats (v4l2_mbus_framefmt). These formats are equivalent to the 2X8 formats that are already supported, both of which accurately describe the data present on the CSI2 interface. This change will increase compatibility with CSI2 RX drivers that only advertise support for the 1X16 formats. Signed-off-by: Sam Bobrowicz <sam@elite-embedded.com> --- drivers/media/i2c/ov5640.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c index a50d884..ca9de56 100644 --- a/drivers/media/i2c/ov5640.c +++ b/drivers/media/i2c/ov5640.c @@ -125,6 +125,8 @@ static const struct ov5640_pixfmt ov5640_formats[] = { { MEDIA_BUS_FMT_JPEG_1X8, V4L2_COLORSPACE_JPEG, }, { MEDIA_BUS_FMT_UYVY8_2X8, V4L2_COLORSPACE_SRGB, }, { MEDIA_BUS_FMT_YUYV8_2X8, V4L2_COLORSPACE_SRGB, }, + { MEDIA_BUS_FMT_UYVY8_1X16, V4L2_COLORSPACE_SRGB, }, + { MEDIA_BUS_FMT_YUYV8_1X16, V4L2_COLORSPACE_SRGB, }, { MEDIA_BUS_FMT_RGB565_2X8_LE, V4L2_COLORSPACE_SRGB, }, { MEDIA_BUS_FMT_RGB565_2X8_BE, V4L2_COLORSPACE_SRGB, }, }; @@ -2069,10 +2071,12 @@ static int ov5640_set_framefmt(struct ov5640_dev *sensor, switch (format->code) { case MEDIA_BUS_FMT_UYVY8_2X8: + case MEDIA_BUS_FMT_UYVY8_1X16: /* YUV422, UYVY */ val = 0x3f; break; case MEDIA_BUS_FMT_YUYV8_2X8: + case MEDIA_BUS_FMT_YUYV8_1X16: /* YUV422, YUYV */ val = 0x30; break; -- 2.7.4 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 4/4] media: ov5640: Add additional media bus formats 2018-10-09 6:48 ` [PATCH 4/4] media: ov5640: Add additional media bus formats Sam Bobrowicz @ 2018-11-16 13:18 ` Sakari Ailus 0 siblings, 0 replies; 15+ messages in thread From: Sakari Ailus @ 2018-11-16 13:18 UTC (permalink / raw) To: Sam Bobrowicz; +Cc: linux-media Hi Sam, On Mon, Oct 08, 2018 at 11:48:02PM -0700, Sam Bobrowicz wrote: > Add support for 1X16 yuv media bus formats (v4l2_mbus_framefmt). > These formats are equivalent to the 2X8 formats that are already > supported, both of which accurately describe the data present on > the CSI2 interface. This change will increase compatibility with > CSI2 RX drivers that only advertise support for the 1X16 formats. > > Signed-off-by: Sam Bobrowicz <sam@elite-embedded.com> > --- > drivers/media/i2c/ov5640.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index a50d884..ca9de56 100644 > --- a/drivers/media/i2c/ov5640.c > +++ b/drivers/media/i2c/ov5640.c > @@ -125,6 +125,8 @@ static const struct ov5640_pixfmt ov5640_formats[] = { > { MEDIA_BUS_FMT_JPEG_1X8, V4L2_COLORSPACE_JPEG, }, > { MEDIA_BUS_FMT_UYVY8_2X8, V4L2_COLORSPACE_SRGB, }, > { MEDIA_BUS_FMT_YUYV8_2X8, V4L2_COLORSPACE_SRGB, }, > + { MEDIA_BUS_FMT_UYVY8_1X16, V4L2_COLORSPACE_SRGB, }, > + { MEDIA_BUS_FMT_YUYV8_1X16, V4L2_COLORSPACE_SRGB, }, > { MEDIA_BUS_FMT_RGB565_2X8_LE, V4L2_COLORSPACE_SRGB, }, > { MEDIA_BUS_FMT_RGB565_2X8_BE, V4L2_COLORSPACE_SRGB, }, > }; Can you make these selectable only for the CSI-2 bus? I understand the driver also supports the parallel interface. > @@ -2069,10 +2071,12 @@ static int ov5640_set_framefmt(struct ov5640_dev *sensor, > > switch (format->code) { > case MEDIA_BUS_FMT_UYVY8_2X8: > + case MEDIA_BUS_FMT_UYVY8_1X16: > /* YUV422, UYVY */ > val = 0x3f; > break; > case MEDIA_BUS_FMT_YUYV8_2X8: > + case MEDIA_BUS_FMT_YUYV8_1X16: > /* YUV422, YUYV */ > val = 0x30; > break; -- Regards, Sakari Ailus e-mail: sakari.ailus@iki.fi ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2018-11-16 23:30 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-09 6:47 [PATCH 0/4] ov5640: small fixes for compatibility Sam Bobrowicz 2018-10-09 6:47 ` [PATCH 1/4] media: ov5640: fix resolution update Sam Bobrowicz 2018-10-10 10:58 ` jacopo mondi 2018-10-10 12:41 ` Laurent Pinchart 2018-10-15 15:13 ` Hugues FRUCHET 2018-10-15 15:24 ` jacopo mondi 2018-10-16 8:49 ` Hugues FRUCHET 2018-10-16 12:15 ` Laurent Pinchart 2018-10-16 18:14 ` Samuel Bobrowicz 2018-10-09 6:48 ` [PATCH 2/4] media: ov5640: fix get_light_freq on auto Sam Bobrowicz 2018-10-10 8:49 ` jacopo mondi 2018-10-09 6:48 ` [PATCH 3/4] media: ov5640: Don't access ctrl regs when off Sam Bobrowicz 2018-10-10 8:48 ` jacopo mondi 2018-10-09 6:48 ` [PATCH 4/4] media: ov5640: Add additional media bus formats Sam Bobrowicz 2018-11-16 13:18 ` Sakari Ailus
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.