Re: [PATCH] media: ov5640: add missing output pixel format setting

[Akinobu Mita]

2018-03-13 1:18 GMT+09:00 Hugues FRUCHET :
> Hi Akinobu,
>
> Thanks for the patch, could you describe the test you made to reproduce
> the issue that I can test on my side ?
>
> I'm using usually yavta or Gstreamer, but I don't know how to trig the
> power on/off independently of streamon/off.

Capturing a single image by yavta and gstreamer can reproduce this issue
in my environment.  I use Xilinx Video IP driver for video device with
the following change in order to support pipeline power management.

https://patchwork.linuxtv.org/patch/46343/

With this change, when opening the video device, s_power() is called with
on=1 for subdevice.

I observed the following steps when capturing a single image by
'yavta -n1 -c1 -Ftest.raw /dev/video1'. (The output pixel format is
already set up by media-ctl before this run)

1. open /dev/video1
2. ov5640_s_power() is called with on=1
   (ov5640_s_power -> ov5640_set_power -> ov5640_restore_mode
-> ov5640_set_mode, and pending_mode_change is cleared)
3. ov5640_s_stream() is called with on=1
   (But ov5640_set_framefmt() is not called because pending_mode_change
has already been cleared  in step 2.)

As ov5640_set_framefmt() is not called, output pixel format cannot be
restored (OV5640_REG_FORMAT_CONTROL00 and OV5640_REG_ISP_FORMAT_MUX_CTRL).

Re: [PATCH] media: ov5640: add missing output pixel format setting

[Hugues FRUCHET]

Hi Akinobu,

Thanks for the patch, could you describe the test you made to reproduce 
the issue that I can test on my side ?

I'm using usually yavta or Gstreamer, but I don't know how to trig the 
power on/off independently of streamon/off.

Best regards,
Hugues.

On 03/11/2018 04:34 PM, Akinobu Mita wrote:
> The output pixel format changed by set_fmt() pad operation is not
> correctly applied.  It is intended to be restored by calling
> ov5640_set_framefmt() when the video stream is started.
> 
> However, when the device is powered on by s_power subdev operation before
> the video stream is started, the current output mode setting is restored
> by ov5640_restore_mode() that also clears pending_mode_change flag in
> ov5640_set_mode().  So ov5640_set_framefmt() isn't called as intended and
> the output pixel format is not restored.
> 
> This change adds the missing output pixel format setting in the
> ov5640_restore_mode() that is called when the device is powered on.
> 
> Cc: Steve Longerbeam 
> Cc: Hugues Fruchet 
> Cc: Sakari Ailus 
> Cc: Mauro Carvalho Chehab 
> Signed-off-by: Akinobu Mita 
> ---
>   drivers/media/i2c/ov5640.c | 9 -
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index e2dd352..4eecc91 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -1633,6 +1633,9 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
>   return 0;
>   }
>   
> +static int ov5640_set_framefmt(struct ov5640_dev *sensor,
> +struct v4l2_mbus_framefmt *format);
> +
>   /* restore the last set video mode after chip power-on */
>   static int ov5640_restore_mode(struct ov5640_dev *sensor)
>   {
> @@ -1644,7 +1647,11 @@ static int ov5640_restore_mode(struct ov5640_dev 
> *sensor)
>   return ret;
>   
>   /* now restore the last capture mode */
> - return ov5640_set_mode(sensor, _mode_init_data);
> + ret = ov5640_set_mode(sensor, _mode_init_data);
> + if (ret < 0)
> + return ret;
> +
> + return ov5640_set_framefmt(sensor, >fmt);
>   }
>   
>   static void ov5640_power(struct ov5640_dev *sensor, bool enable)
> 

[PATCH] media: ov5640: add missing output pixel format setting

[Akinobu Mita]

The output pixel format changed by set_fmt() pad operation is not
correctly applied.  It is intended to be restored by calling
ov5640_set_framefmt() when the video stream is started.

However, when the device is powered on by s_power subdev operation before
the video stream is started, the current output mode setting is restored
by ov5640_restore_mode() that also clears pending_mode_change flag in
ov5640_set_mode().  So ov5640_set_framefmt() isn't called as intended and
the output pixel format is not restored.

This change adds the missing output pixel format setting in the
ov5640_restore_mode() that is called when the device is powered on.

Cc: Steve Longerbeam 
Cc: Hugues Fruchet 
Cc: Sakari Ailus 
Cc: Mauro Carvalho Chehab 
Signed-off-by: Akinobu Mita 
---
 drivers/media/i2c/ov5640.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index e2dd352..4eecc91 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -1633,6 +1633,9 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
return 0;
 }
 
+static int ov5640_set_framefmt(struct ov5640_dev *sensor,
+  struct v4l2_mbus_framefmt *format);
+
 /* restore the last set video mode after chip power-on */
 static int ov5640_restore_mode(struct ov5640_dev *sensor)
 {
@@ -1644,7 +1647,11 @@ static int ov5640_restore_mode(struct ov5640_dev *sensor)
return ret;
 
/* now restore the last capture mode */
-   return ov5640_set_mode(sensor, _mode_init_data);
+   ret = ov5640_set_mode(sensor, _mode_init_data);
+   if (ret < 0)
+   return ret;
+
+   return ov5640_set_framefmt(sensor, >fmt);
 }
 
 static void ov5640_power(struct ov5640_dev *sensor, bool enable)
-- 
2.7.4