| Message ID | 20190124175801.28018-1-jagan@amarulasolutions.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ov5640: Fix set 15fps regression | expand |
On Thu, Jan 24, 2019 at 11:28:01PM +0530, Jagan Teki wrote: > The ov5640_try_frame_interval operation updates the FPS as per user > input based on default ov5640_frame_rate, OV5640_30_FPS which is failed > to update when user trigger 15fps. > > So, initialize the default ov5640_frame_rate to OV5640_15_FPS so-that > it can satisfy to update all fps. > > Fixes: 5a3ad937bc78 ("media: ov5640: Make the return rate type more explicit") > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> I'm pretty sure I tested this and it was working fine. You're mentionning a regression, but what regression is there exactly (ie, what was working before that commit that doesn't work anymore?). What tools/commands are you using to see this behaviour? It really isn't obvious from your patch and the patch you mention what could go wrong or be improved. Maxime
On Fri, Jan 25, 2019 at 9:10 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > On Thu, Jan 24, 2019 at 11:28:01PM +0530, Jagan Teki wrote: > > The ov5640_try_frame_interval operation updates the FPS as per user > > input based on default ov5640_frame_rate, OV5640_30_FPS which is failed > > to update when user trigger 15fps. > > > > So, initialize the default ov5640_frame_rate to OV5640_15_FPS so-that > > it can satisfy to update all fps. > > > > Fixes: 5a3ad937bc78 ("media: ov5640: Make the return rate type more explicit") > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > > I'm pretty sure I tested this and it was working fine. You're > mentionning a regression, but what regression is there exactly (ie, > what was working before that commit that doesn't work anymore?). What > tools/commands are you using to see this behaviour? In fact I have mentioned this on your v9 [1], may be you missed it. I have reproduced with media-ctl, below is the full log. let me know if I still miss anything. Before this change: # media-ctl -p Media controller API version 5.0.0 Media device information ------------------------ driver sun6i-csi model Allwinner Video Capture Device serial bus info hw revision 0x0 driver version 5.0.0 Device topology - entity 1: sun6i-csi (1 pad, 1 link) type Node subtype V4L flags 0 device node name /dev/video0 pad0: Sink <- "ov5640 1-003c":0 [ENABLED,IMMUTABLE] - entity 5: ov5640 1-003c (1 pad, 1 link) type V4L2 subdev subtype Sensor flags 0 device node name /dev/v4l-subdev0 pad0: Source [fmt:UYVY8_2X8/640x480@1/30 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:full-range] -> "sun6i-csi":0 [ENABLED,IMMUTABLE] # media-ctl --set-v4l2 "'ov5640 1-003c':0[fmt:UYVY8_2X8/640x480@1/15 field:none] " # media-ctl -p Media controller API version 5.0.0 Media device information ------------------------ driver sun6i-csi model Allwinner Video Capture Device serial bus info hw revision 0x0 driver version 5.0.0 Device topology - entity 1: sun6i-csi (1 pad, 1 link) type Node subtype V4L flags 0 device node name /dev/video0 pad0: Sink <- "ov5640 1-003c":0 [ENABLED,IMMUTABLE] - entity 5: ov5640 1-003c (1 pad, 1 link) type V4L2 subdev subtype Sensor flags 0 device node name /dev/v4l-subdev0 pad0: Source [fmt:UYVY8_2X8/640x480@1/30 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:full-range] -> "sun6i-csi":0 [ENABLED,IMMUTABLE] After this change: # media-ctl -p Media controller API version 5.0.0 Media device information ------------------------ driver sun6i-csi model Allwinner Video Capture Device serial bus info hw revision 0x0 driver version 5.0.0 Device topology - entity 1: sun6i-csi (1 pad, 1 link) type Node subtype V4L flags 0 device node name /dev/video0 pad0: Sink <- "ov5640 1-003c":0 [ENABLED,IMMUTABLE] - entity 5: ov5640 1-003c (1 pad, 1 link) type V4L2 subdev subtype Sensor flags 0 device node name /dev/v4l-subdev0 pad0: Source [fmt:UYVY8_2X8/640x480@1/30 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:full-range] -> "sun6i-csi":0 [ENABLED,IMMUTABLE] # media-ctl --set-v4l2 "'ov5640 1-003c':0[fmt:UYVY8_2X8/640x480@1/15 field:none] " # media-ctl -p Media controller API version 5.0.0 Media device information ------------------------ driver sun6i-csi model Allwinner Video Capture Device serial bus info hw revision 0x0 driver version 5.0.0 Device topology - entity 1: sun6i-csi (1 pad, 1 link) type Node subtype V4L flags 0 device node name /dev/video0 pad0: Sink <- "ov5640 1-003c":0 [ENABLED,IMMUTABLE] - entity 5: ov5640 1-003c (1 pad, 1 link) type V4L2 subdev subtype Sensor flags 0 device node name /dev/v4l-subdev0 pad0: Source [fmt:UYVY8_2X8/640x480@1/15 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:full-range] -> "sun6i-csi":0 [ENABLED,IMMUTABLE] [1] https://patchwork.kernel.org/patch/10708931/
Hi everyone, On Mon, Jan 28, 2019 at 01:20:37PM +0530, Jagan Teki wrote: > On Fri, Jan 25, 2019 at 9:10 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > On Thu, Jan 24, 2019 at 11:28:01PM +0530, Jagan Teki wrote: > > > The ov5640_try_frame_interval operation updates the FPS as per user > > > input based on default ov5640_frame_rate, OV5640_30_FPS which is failed > > > to update when user trigger 15fps. > > > > > > So, initialize the default ov5640_frame_rate to OV5640_15_FPS so-that > > > it can satisfy to update all fps. > > > > > > Fixes: 5a3ad937bc78 ("media: ov5640: Make the return rate type more explicit") > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > > > > I'm pretty sure I tested this and it was working fine. You're > > mentionning a regression, but what regression is there exactly (ie, > > what was working before that commit that doesn't work anymore?). What > > tools/commands are you using to see this behaviour? > I think Jagan's right, here. For the 15FPS use case, the below condition in the for loop of ov5640_try_frame_interval() never gets satisfied (0 < 0 ?) and 'rate' retains its initial value of 30FPS. if (abs(curr_fps - fps) < abs(best_fps - fps)) { best_fps = curr_fps; rate = i; } To make more clear what's happening, I would initialize 'rate' just before 'best_fps' before the for loop. Anyway, please add: Acked-by: Jacopo Mondi <jacopo@jmondi.org> Maxime, does this make sense to you? Thanks j > In fact I have mentioned this on your v9 [1], may be you missed it. > > I have reproduced with media-ctl, below is the full log. let me know > if I still miss anything. > > Before this change: > # media-ctl -p > Media controller API version 5.0.0 > > Media device information > ------------------------ > driver sun6i-csi > model Allwinner Video Capture Device > serial > bus info > hw revision 0x0 > driver version 5.0.0 > > Device topology > - entity 1: sun6i-csi (1 pad, 1 link) > type Node subtype V4L flags 0 > device node name /dev/video0 > pad0: Sink > <- "ov5640 1-003c":0 [ENABLED,IMMUTABLE] > > - entity 5: ov5640 1-003c (1 pad, 1 link) > type V4L2 subdev subtype Sensor flags 0 > device node name /dev/v4l-subdev0 > pad0: Source > [fmt:UYVY8_2X8/640x480@1/30 field:none colorspace:srgb > xfer:srgb ycbcr:601 quantization:full-range] > -> "sun6i-csi":0 [ENABLED,IMMUTABLE] > > # media-ctl --set-v4l2 "'ov5640 1-003c':0[fmt:UYVY8_2X8/640x480@1/15 field:none] > " > # media-ctl -p > Media controller API version 5.0.0 > > Media device information > ------------------------ > driver sun6i-csi > model Allwinner Video Capture Device > serial > bus info > hw revision 0x0 > driver version 5.0.0 > > Device topology > - entity 1: sun6i-csi (1 pad, 1 link) > type Node subtype V4L flags 0 > device node name /dev/video0 > pad0: Sink > <- "ov5640 1-003c":0 [ENABLED,IMMUTABLE] > > - entity 5: ov5640 1-003c (1 pad, 1 link) > type V4L2 subdev subtype Sensor flags 0 > device node name /dev/v4l-subdev0 > pad0: Source > [fmt:UYVY8_2X8/640x480@1/30 field:none colorspace:srgb > xfer:srgb ycbcr:601 quantization:full-range] > -> "sun6i-csi":0 [ENABLED,IMMUTABLE] > > > After this change: > # media-ctl -p > Media controller API version 5.0.0 > > Media device information > ------------------------ > driver sun6i-csi > model Allwinner Video Capture Device > serial > bus info > hw revision 0x0 > driver version 5.0.0 > > Device topology > - entity 1: sun6i-csi (1 pad, 1 link) > type Node subtype V4L flags 0 > device node name /dev/video0 > pad0: Sink > <- "ov5640 1-003c":0 [ENABLED,IMMUTABLE] > > - entity 5: ov5640 1-003c (1 pad, 1 link) > type V4L2 subdev subtype Sensor flags 0 > device node name /dev/v4l-subdev0 > pad0: Source > [fmt:UYVY8_2X8/640x480@1/30 field:none colorspace:srgb > xfer:srgb ycbcr:601 quantization:full-range] > -> "sun6i-csi":0 [ENABLED,IMMUTABLE] > > # media-ctl --set-v4l2 "'ov5640 1-003c':0[fmt:UYVY8_2X8/640x480@1/15 field:none] > " > # media-ctl -p > Media controller API version 5.0.0 > > Media device information > ------------------------ > driver sun6i-csi > model Allwinner Video Capture Device > serial > bus info > hw revision 0x0 > driver version 5.0.0 > > Device topology > - entity 1: sun6i-csi (1 pad, 1 link) > type Node subtype V4L flags 0 > device node name /dev/video0 > pad0: Sink > <- "ov5640 1-003c":0 [ENABLED,IMMUTABLE] > > - entity 5: ov5640 1-003c (1 pad, 1 link) > type V4L2 subdev subtype Sensor flags 0 > device node name /dev/v4l-subdev0 > pad0: Source > [fmt:UYVY8_2X8/640x480@1/15 field:none colorspace:srgb > xfer:srgb ycbcr:601 quantization:full-range] > -> "sun6i-csi":0 [ENABLED,IMMUTABLE] > > [1] https://patchwork.kernel.org/patch/10708931/
On Mon, Jan 28, 2019 at 09:33:04AM +0100, Jacopo Mondi wrote: > Hi everyone, > > On Mon, Jan 28, 2019 at 01:20:37PM +0530, Jagan Teki wrote: > > On Fri, Jan 25, 2019 at 9:10 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > > > On Thu, Jan 24, 2019 at 11:28:01PM +0530, Jagan Teki wrote: > > > > The ov5640_try_frame_interval operation updates the FPS as per user > > > > input based on default ov5640_frame_rate, OV5640_30_FPS which is failed > > > > to update when user trigger 15fps. > > > > > > > > So, initialize the default ov5640_frame_rate to OV5640_15_FPS so-that > > > > it can satisfy to update all fps. > > > > > > > > Fixes: 5a3ad937bc78 ("media: ov5640: Make the return rate type more explicit") > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > > > > > > I'm pretty sure I tested this and it was working fine. You're > > > mentionning a regression, but what regression is there exactly (ie, > > > what was working before that commit that doesn't work anymore?). What > > > tools/commands are you using to see this behaviour? > > > > I think Jagan's right, here. > > For the 15FPS use case, the below condition in the for loop of > ov5640_try_frame_interval() never gets satisfied (0 < 0 ?) and 'rate' retains > its initial value of 30FPS. > > if (abs(curr_fps - fps) < abs(best_fps - fps)) { > best_fps = curr_fps; > rate = i; > } > > To make more clear what's happening, I would initialize 'rate' just > before 'best_fps' before the for loop. Anyway, please add: > Acked-by: Jacopo Mondi <jacopo@jmondi.org> > > Maxime, does this make sense to you? With that explanation in the commit log, yes. Maxime
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c index 5a909abd0a2d..4081c29176c6 100644 --- a/drivers/media/i2c/ov5640.c +++ b/drivers/media/i2c/ov5640.c @@ -2072,7 +2072,7 @@ static int ov5640_try_frame_interval(struct ov5640_dev *sensor, u32 width, u32 height) { const struct ov5640_mode_info *mode; - enum ov5640_frame_rate rate = OV5640_30_FPS; + enum ov5640_frame_rate rate = OV5640_15_FPS; int minfps, maxfps, best_fps, fps; int i;
The ov5640_try_frame_interval operation updates the FPS as per user input based on default ov5640_frame_rate, OV5640_30_FPS which is failed to update when user trigger 15fps. So, initialize the default ov5640_frame_rate to OV5640_15_FPS so-that it can satisfy to update all fps. Fixes: 5a3ad937bc78 ("media: ov5640: Make the return rate type more explicit") Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> --- drivers/media/i2c/ov5640.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)