| Message ID | 1520782481-13558-1-git-send-email-akinobu.mita@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <slongerbeam@gmail.com> > Cc: Hugues Fruchet <hugues.fruchet@st.com> > Cc: Sakari Ailus <sakari.ailus@linux.intel.com> > Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> > --- > 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, &ov5640_mode_init_data); > + ret = ov5640_set_mode(sensor, &ov5640_mode_init_data); > + if (ret < 0) > + return ret; > + > + return ov5640_set_framefmt(sensor, &sensor->fmt); > } > > static void ov5640_power(struct ov5640_dev *sensor, bool enable) >
2018-03-13 1:18 GMT+09:00 Hugues FRUCHET <hugues.fruchet@st.com>: > 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).
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, &ov5640_mode_init_data); + ret = ov5640_set_mode(sensor, &ov5640_mode_init_data); + if (ret < 0) + return ret; + + return ov5640_set_framefmt(sensor, &sensor->fmt); } static void ov5640_power(struct ov5640_dev *sensor, bool enable)
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 <slongerbeam@gmail.com> Cc: Hugues Fruchet <hugues.fruchet@st.com> Cc: Sakari Ailus <sakari.ailus@linux.intel.com> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> --- drivers/media/i2c/ov5640.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)