| Message ID | 20190625203945.28081-1-ezequiel@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: imx: mipi csi-2: Don't fail if initial state times-out | expand |
Thanks, there was earlier talk of relaxing those CSI-2 bus startup requirements, but somehow it fell through the cracks. Acked-by: Steve Longerbeam <slongerbeam@gmail.com> On 6/25/19 1:39 PM, Ezequiel Garcia wrote: > Not all sensors will be able to guarantee a proper initial state. > This may be either because the driver is not properly written, > or (probably unlikely) because the hardware won't support it. > > While the right solution in the former case is to fix the sensor > driver, the real world not always allows right solutions, due to lack > of available documentation and support on these sensors. > > Let's relax this requirement, and allow the driver to support stream start, > even if the sensor initial sequence wasn't the expected. > A warning is still emitted, so users should be hinted that something is off. > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > --- > drivers/staging/media/imx/imx6-mipi-csi2.c | 33 ++++++---------------- > 1 file changed, 9 insertions(+), 24 deletions(-) > > diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c > index f29e28df36ed..10342434e797 100644 > --- a/drivers/staging/media/imx/imx6-mipi-csi2.c > +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c > @@ -243,7 +243,7 @@ static int __maybe_unused csi2_dphy_wait_ulp(struct csi2_dev *csi2) > } > > /* Waits for low-power LP-11 state on data and clock lanes. */ > -static int csi2_dphy_wait_stopstate(struct csi2_dev *csi2) > +static void csi2_dphy_wait_stopstate(struct csi2_dev *csi2) > { > u32 mask, reg; > int ret; > @@ -253,29 +253,21 @@ static int csi2_dphy_wait_stopstate(struct csi2_dev *csi2) > > ret = readl_poll_timeout(csi2->base + CSI2_PHY_STATE, reg, > (reg & mask) == mask, 0, 500000); > - if (ret) { > - v4l2_err(&csi2->sd, "LP-11 timeout, phy_state = 0x%08x\n", reg); > - return ret; > - } > - > - return 0; > + if (ret) > + v4l2_warn(&csi2->sd, "LP-11 timeout, phy_state = 0x%08x\n", reg); > } > > /* Wait for active clock on the clock lane. */ > -static int csi2_dphy_wait_clock_lane(struct csi2_dev *csi2) > +static void csi2_dphy_wait_clock_lane(struct csi2_dev *csi2) > { > u32 reg; > int ret; > > ret = readl_poll_timeout(csi2->base + CSI2_PHY_STATE, reg, > (reg & PHY_RXCLKACTIVEHS), 0, 500000); > - if (ret) { > - v4l2_err(&csi2->sd, "clock lane timeout, phy_state = 0x%08x\n", > - reg); > - return ret; > - } > - > - return 0; > + if (ret) > + v4l2_warn(&csi2->sd, "clock lane timeout, phy_state = 0x%08x\n", > + reg); > } > > /* Setup the i.MX CSI2IPU Gasket */ > @@ -316,9 +308,7 @@ static int csi2_start(struct csi2_dev *csi2) > csi2_enable(csi2, true); > > /* Step 5 */ > - ret = csi2_dphy_wait_stopstate(csi2); > - if (ret) > - goto err_assert_reset; > + csi2_dphy_wait_stopstate(csi2); > > /* Step 6 */ > ret = v4l2_subdev_call(csi2->src_sd, video, s_stream, 1); > @@ -327,14 +317,9 @@ static int csi2_start(struct csi2_dev *csi2) > goto err_assert_reset; > > /* Step 7 */ > - ret = csi2_dphy_wait_clock_lane(csi2); > - if (ret) > - goto err_stop_upstream; > - > + csi2_dphy_wait_clock_lane(csi2); > return 0; > > -err_stop_upstream: > - v4l2_subdev_call(csi2->src_sd, video, s_stream, 0); > err_assert_reset: > csi2_enable(csi2, false); > err_disable_clk:
On Tue, 2019-06-25 at 17:39 -0300, Ezequiel Garcia wrote: > Not all sensors will be able to guarantee a proper initial state. > This may be either because the driver is not properly written, > or (probably unlikely) because the hardware won't support it. > > While the right solution in the former case is to fix the sensor > driver, the real world not always allows right solutions, due to lack > of available documentation and support on these sensors. Do you have a real world example for this? > Let's relax this requirement, and allow the driver to support stream start, > even if the sensor initial sequence wasn't the expected. > A warning is still emitted, so users should be hinted that something is off. I think the messages could use a note that they may be due to a sensor or sensor driver bug, and that there might be image artifacts or outright failure to capture as a consequence. > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> regards Philipp > --- > drivers/staging/media/imx/imx6-mipi-csi2.c | 33 ++++++---------------- > 1 file changed, 9 insertions(+), 24 deletions(-) > > diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c > index f29e28df36ed..10342434e797 100644 > --- a/drivers/staging/media/imx/imx6-mipi-csi2.c > +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c > @@ -243,7 +243,7 @@ static int __maybe_unused csi2_dphy_wait_ulp(struct csi2_dev *csi2) > } > > /* Waits for low-power LP-11 state on data and clock lanes. */ > -static int csi2_dphy_wait_stopstate(struct csi2_dev *csi2) > +static void csi2_dphy_wait_stopstate(struct csi2_dev *csi2) > { > u32 mask, reg; > int ret; > @@ -253,29 +253,21 @@ static int csi2_dphy_wait_stopstate(struct csi2_dev *csi2) > > ret = readl_poll_timeout(csi2->base + CSI2_PHY_STATE, reg, > (reg & mask) == mask, 0, 500000); > - if (ret) { > - v4l2_err(&csi2->sd, "LP-11 timeout, phy_state = 0x%08x\n", reg); > - return ret; > - } > - > - return 0; > + if (ret) > + v4l2_warn(&csi2->sd, "LP-11 timeout, phy_state = 0x%08x\n", reg); > } > > /* Wait for active clock on the clock lane. */ > -static int csi2_dphy_wait_clock_lane(struct csi2_dev *csi2) > +static void csi2_dphy_wait_clock_lane(struct csi2_dev *csi2) > { > u32 reg; > int ret; > > ret = readl_poll_timeout(csi2->base + CSI2_PHY_STATE, reg, > (reg & PHY_RXCLKACTIVEHS), 0, 500000); > - if (ret) { > - v4l2_err(&csi2->sd, "clock lane timeout, phy_state = 0x%08x\n", > - reg); > - return ret; > - } > - > - return 0; > + if (ret) > + v4l2_warn(&csi2->sd, "clock lane timeout, phy_state = 0x%08x\n", > + reg); > } > > /* Setup the i.MX CSI2IPU Gasket */ > @@ -316,9 +308,7 @@ static int csi2_start(struct csi2_dev *csi2) > csi2_enable(csi2, true); > > /* Step 5 */ > - ret = csi2_dphy_wait_stopstate(csi2); > - if (ret) > - goto err_assert_reset; > + csi2_dphy_wait_stopstate(csi2); > > /* Step 6 */ > ret = v4l2_subdev_call(csi2->src_sd, video, s_stream, 1); > @@ -327,14 +317,9 @@ static int csi2_start(struct csi2_dev *csi2) > goto err_assert_reset; > > /* Step 7 */ > - ret = csi2_dphy_wait_clock_lane(csi2); > - if (ret) > - goto err_stop_upstream; > - > + csi2_dphy_wait_clock_lane(csi2); > return 0; > > -err_stop_upstream: > - v4l2_subdev_call(csi2->src_sd, video, s_stream, 0); > err_assert_reset: > csi2_enable(csi2, false); > err_disable_clk:
Hi Ezequiel, Thank you for the patch. On Tue, Jun 25, 2019 at 05:39:45PM -0300, Ezequiel Garcia wrote: > Not all sensors will be able to guarantee a proper initial state. > This may be either because the driver is not properly written, > or (probably unlikely) because the hardware won't support it. > > While the right solution in the former case is to fix the sensor > driver, the real world not always allows right solutions, due to lack > of available documentation and support on these sensors. > > Let's relax this requirement, and allow the driver to support stream start, > even if the sensor initial sequence wasn't the expected. > A warning is still emitted, so users should be hinted that something is off. I'm not sure this is a very good idea. Failure to detect the LP-11 state may mean that the sensor is completely powered off, but it may also mean that it is already streaming data. I don't know how the CSI-2 receiver state machine will operate in the first case, but in the second case it will not be able to synchronise to the incoming stream, so it won't work anyway. I think you should instead fix the problem in the sensor driver, as you hinted. Relaxing the requirement here will only make it more confusing, it's a hack, and isn't portable across CSI-2 receivers. The same buggy sensor driver won't work with other CSI-2 receivers whose internal state machine require starting in the LP-11 state. Which sensor are you using ? > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > --- > drivers/staging/media/imx/imx6-mipi-csi2.c | 33 ++++++---------------- > 1 file changed, 9 insertions(+), 24 deletions(-) > > diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c > index f29e28df36ed..10342434e797 100644 > --- a/drivers/staging/media/imx/imx6-mipi-csi2.c > +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c > @@ -243,7 +243,7 @@ static int __maybe_unused csi2_dphy_wait_ulp(struct csi2_dev *csi2) > } > > /* Waits for low-power LP-11 state on data and clock lanes. */ > -static int csi2_dphy_wait_stopstate(struct csi2_dev *csi2) > +static void csi2_dphy_wait_stopstate(struct csi2_dev *csi2) > { > u32 mask, reg; > int ret; > @@ -253,29 +253,21 @@ static int csi2_dphy_wait_stopstate(struct csi2_dev *csi2) > > ret = readl_poll_timeout(csi2->base + CSI2_PHY_STATE, reg, > (reg & mask) == mask, 0, 500000); > - if (ret) { > - v4l2_err(&csi2->sd, "LP-11 timeout, phy_state = 0x%08x\n", reg); > - return ret; > - } > - > - return 0; > + if (ret) > + v4l2_warn(&csi2->sd, "LP-11 timeout, phy_state = 0x%08x\n", reg); > } > > /* Wait for active clock on the clock lane. */ > -static int csi2_dphy_wait_clock_lane(struct csi2_dev *csi2) > +static void csi2_dphy_wait_clock_lane(struct csi2_dev *csi2) > { > u32 reg; > int ret; > > ret = readl_poll_timeout(csi2->base + CSI2_PHY_STATE, reg, > (reg & PHY_RXCLKACTIVEHS), 0, 500000); > - if (ret) { > - v4l2_err(&csi2->sd, "clock lane timeout, phy_state = 0x%08x\n", > - reg); > - return ret; > - } > - > - return 0; > + if (ret) > + v4l2_warn(&csi2->sd, "clock lane timeout, phy_state = 0x%08x\n", > + reg); > } > > /* Setup the i.MX CSI2IPU Gasket */ > @@ -316,9 +308,7 @@ static int csi2_start(struct csi2_dev *csi2) > csi2_enable(csi2, true); > > /* Step 5 */ > - ret = csi2_dphy_wait_stopstate(csi2); > - if (ret) > - goto err_assert_reset; > + csi2_dphy_wait_stopstate(csi2); > > /* Step 6 */ > ret = v4l2_subdev_call(csi2->src_sd, video, s_stream, 1); > @@ -327,14 +317,9 @@ static int csi2_start(struct csi2_dev *csi2) > goto err_assert_reset; > > /* Step 7 */ > - ret = csi2_dphy_wait_clock_lane(csi2); > - if (ret) > - goto err_stop_upstream; > - > + csi2_dphy_wait_clock_lane(csi2); > return 0; > > -err_stop_upstream: > - v4l2_subdev_call(csi2->src_sd, video, s_stream, 0); > err_assert_reset: > csi2_enable(csi2, false); > err_disable_clk:
Hi Laurent, Philipp, Thank you for the prompt review! I was pretty sure this would raise your wise eyebrows :-) On Wed, 2019-06-26 at 11:00 +0300, Laurent Pinchart wrote: > Hi Ezequiel, > > Thank you for the patch. > > On Tue, Jun 25, 2019 at 05:39:45PM -0300, Ezequiel Garcia wrote: > > Not all sensors will be able to guarantee a proper initial state. > > This may be either because the driver is not properly written, > > or (probably unlikely) because the hardware won't support it. > > > > While the right solution in the former case is to fix the sensor > > driver, the real world not always allows right solutions, due to lack > > of available documentation and support on these sensors. > > > > Let's relax this requirement, and allow the driver to support stream start, > > even if the sensor initial sequence wasn't the expected. > > A warning is still emitted, so users should be hinted that something is off. > > I'm not sure this is a very good idea. Failure to detect the LP-11 state > may mean that the sensor is completely powered off, but it may also mean > that it is already streaming data. I don't know how the CSI-2 receiver > state machine will operate in the first case, but in the second case it > will not be able to synchronise to the incoming stream, so it won't work > anyway. > > I think you should instead fix the problem in the sensor driver, as you > hinted. Sure, we all agree that the sensor fix is the right solution. > Relaxing the requirement here will only make it more confusing, > it's a hack, and isn't portable across CSI-2 receivers. We can emit a warning as suggested by Philipp, stating that the sensor is buggy and needs fixing. > The same buggy > sensor driver won't work with other CSI-2 receivers whose internal state > machine require starting in the LP-11 state. > Right. But the same buggy sensor driver probaly does work already with other CSI-2 receivers, hence why it hasn't been detected when it was submitted. > Which sensor are you using ? > I noticed this problem on OV5645 on a Wandoboard. Looks to be basically the same issue as the one Jacopo fixed here: Author: Jacopo Mondi <jacopo@jmondi.org> Date: Fri Jul 6 05:51:52 2018 -0400 media: ov5640: Re-work MIPI startup sequence I fixed it for OV5645 (and will submit soon), but that is not the point. The point is that buggy drivers exist, and while I'd love to fix them all, it won't be always possible (due to lack of datasheets, and due to some of these sensors having no active maintainer). Without this commit, such a buggy sensor will not work for sure; while with the commit, there is a chance it will work. Why would we prevent the latter from happening? Thanks, Eze
Hi Laurent, On 6/26/19 1:00 AM, Laurent Pinchart wrote: > Hi Ezequiel, > > Thank you for the patch. > > On Tue, Jun 25, 2019 at 05:39:45PM -0300, Ezequiel Garcia wrote: >> Not all sensors will be able to guarantee a proper initial state. >> This may be either because the driver is not properly written, >> or (probably unlikely) because the hardware won't support it. >> >> While the right solution in the former case is to fix the sensor >> driver, the real world not always allows right solutions, due to lack >> of available documentation and support on these sensors. >> >> Let's relax this requirement, and allow the driver to support stream start, >> even if the sensor initial sequence wasn't the expected. >> A warning is still emitted, so users should be hinted that something is off. > I'm not sure this is a very good idea. Failure to detect the LP-11 state > may mean that the sensor is completely powered off, but it may also mean > that it is already streaming data. I don't know how the CSI-2 receiver > state machine will operate in the first case, but in the second case it > will not be able to synchronise to the incoming stream, so it won't work > anyway. From my experience, at least with the OV5640 and the DS90Ux940, it can be difficult to coax some CSI-2 transmitters into a quiescent LP-11 bus state after power on, and yet the CSI-2 receiver state machine, at least in the imx6, is able to move forward to stream on anyway. So I think it makes sense to at least relax the LP-11 requirement at stream on for the imx6 receiver driver, and print a warning. But on second thought I agree the active clock lane requirement before stream on needs to remain. In the second case you point out above (sensor is already actively streaming at stream on), why do you think that the receiver will not be able to synchronize? > > I think you should instead fix the problem in the sensor driver, as you > hinted. Relaxing the requirement here will only make it more confusing, > it's a hack, and isn't portable across CSI-2 receivers. The same buggy > sensor driver won't work with other CSI-2 receivers whose internal state > machine require starting in the LP-11 state. Agreed, if it's possible the sensor driver should be fixed to enter LP-11 at power on. Steve > Which sensor are you using ? > >> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> >> --- >> drivers/staging/media/imx/imx6-mipi-csi2.c | 33 ++++++---------------- >> 1 file changed, 9 insertions(+), 24 deletions(-) >> >> diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c >> index f29e28df36ed..10342434e797 100644 >> --- a/drivers/staging/media/imx/imx6-mipi-csi2.c >> +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c >> @@ -243,7 +243,7 @@ static int __maybe_unused csi2_dphy_wait_ulp(struct csi2_dev *csi2) >> } >> >> /* Waits for low-power LP-11 state on data and clock lanes. */ >> -static int csi2_dphy_wait_stopstate(struct csi2_dev *csi2) >> +static void csi2_dphy_wait_stopstate(struct csi2_dev *csi2) >> { >> u32 mask, reg; >> int ret; >> @@ -253,29 +253,21 @@ static int csi2_dphy_wait_stopstate(struct csi2_dev *csi2) >> >> ret = readl_poll_timeout(csi2->base + CSI2_PHY_STATE, reg, >> (reg & mask) == mask, 0, 500000); >> - if (ret) { >> - v4l2_err(&csi2->sd, "LP-11 timeout, phy_state = 0x%08x\n", reg); >> - return ret; >> - } >> - >> - return 0; >> + if (ret) >> + v4l2_warn(&csi2->sd, "LP-11 timeout, phy_state = 0x%08x\n", reg); >> } >> >> /* Wait for active clock on the clock lane. */ >> -static int csi2_dphy_wait_clock_lane(struct csi2_dev *csi2) >> +static void csi2_dphy_wait_clock_lane(struct csi2_dev *csi2) >> { >> u32 reg; >> int ret; >> >> ret = readl_poll_timeout(csi2->base + CSI2_PHY_STATE, reg, >> (reg & PHY_RXCLKACTIVEHS), 0, 500000); >> - if (ret) { >> - v4l2_err(&csi2->sd, "clock lane timeout, phy_state = 0x%08x\n", >> - reg); >> - return ret; >> - } >> - >> - return 0; >> + if (ret) >> + v4l2_warn(&csi2->sd, "clock lane timeout, phy_state = 0x%08x\n", >> + reg); >> } >> >> /* Setup the i.MX CSI2IPU Gasket */ >> @@ -316,9 +308,7 @@ static int csi2_start(struct csi2_dev *csi2) >> csi2_enable(csi2, true); >> >> /* Step 5 */ >> - ret = csi2_dphy_wait_stopstate(csi2); >> - if (ret) >> - goto err_assert_reset; >> + csi2_dphy_wait_stopstate(csi2); >> >> /* Step 6 */ >> ret = v4l2_subdev_call(csi2->src_sd, video, s_stream, 1); >> @@ -327,14 +317,9 @@ static int csi2_start(struct csi2_dev *csi2) >> goto err_assert_reset; >> >> /* Step 7 */ >> - ret = csi2_dphy_wait_clock_lane(csi2); >> - if (ret) >> - goto err_stop_upstream; >> - >> + csi2_dphy_wait_clock_lane(csi2); >> return 0; >> >> -err_stop_upstream: >> - v4l2_subdev_call(csi2->src_sd, video, s_stream, 0); >> err_assert_reset: >> csi2_enable(csi2, false); >> err_disable_clk:
Hi Philipp, On Wed, Jun 26, 2019 at 4:45 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > I think the messages could use a note that they may be due to a sensor > or sensor driver bug, and that there might be image artifacts or > outright failure to capture as a consequence. Yes, this a good idea. With this patch I could successfully test camera capture on a imx6dl-wandboard connected to a OV5645. Without this patch the Gsteamer pipeline fails. Tested-by: Fabio Estevam <festevam@gmail.com>
On 6/26/19 12:53 PM, Fabio Estevam wrote: > Hi Philipp, > > On Wed, Jun 26, 2019 at 4:45 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > >> I think the messages could use a note that they may be due to a sensor >> or sensor driver bug, and that there might be image artifacts or >> outright failure to capture as a consequence. > Yes, this a good idea. > > With this patch I could successfully test camera capture on a > imx6dl-wandboard connected to a OV5645. > > Without this patch the Gsteamer pipeline fails. Did you only get the LP-11 timeout warning message with this patch on the OV5645, or both the LP-11 timeout and clock lane timeout warnings? As I said it's OK to relax the LP-11 timeout to be warning only, but clock lane timeout should remain an error, since without an active clock on the clock lane it's guaranteed that no data will be received from the sensor. Steve > > Tested-by: Fabio Estevam <festevam@gmail.com>
Hi Steve, On Wed, Jun 26, 2019 at 6:19 PM Steve Longerbeam <slongerbeam@gmail.com> wrote: > Did you only get the LP-11 timeout warning message with this patch on > the OV5645, or both the LP-11 timeout and clock lane timeout warnings? With this patch applied I get only the LP-11 timeout warnings, not clock lane timeouts. Thanks
Hi Fabio, On 6/26/19 4:22 PM, Fabio Estevam wrote: > Hi Steve, > > On Wed, Jun 26, 2019 at 6:19 PM Steve Longerbeam <slongerbeam@gmail.com> wrote: > >> Did you only get the LP-11 timeout warning message with this patch on >> the OV5645, or both the LP-11 timeout and clock lane timeout warnings? > With this patch applied I get only the LP-11 timeout warnings, not > clock lane timeouts. Ok thanks for the confirmation that the imx6 CSI-2 receiver is able to successfully move to stream on without seeing the LP-11 state in this case. So in my opinion the next version of this patch should make LP-11 timeout a warning only, but keep the error return on clock lane timeouts. Steve
Hi Ezequiel, On Tue, Jun 25, 2019 at 05:39:45PM -0300, Ezequiel Garcia wrote: > Not all sensors will be able to guarantee a proper initial state. > This may be either because the driver is not properly written, > or (probably unlikely) because the hardware won't support it. > > While the right solution in the former case is to fix the sensor > driver, the real world not always allows right solutions, due to lack > of available documentation and support on these sensors. > We hit this in the past with the ov5640 sensor, whose driver was not properly initializing its data lanes in LP-11 state, so yes, I'm not surprised other sensors might fail in the same way. Do you get frames after you hit the error? I recall it was not possible with ov5640, even with something similar to what you've done here in the CSI-2 receiver driver. > Let's relax this requirement, and allow the driver to support stream start, > even if the sensor initial sequence wasn't the expected. > A warning is still emitted, so users should be hinted that something is off. > It seems you're also silencing errors related to clock lane detection. I would mention that. > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > --- > drivers/staging/media/imx/imx6-mipi-csi2.c | 33 ++++++---------------- > 1 file changed, 9 insertions(+), 24 deletions(-) > > diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c > index f29e28df36ed..10342434e797 100644 > --- a/drivers/staging/media/imx/imx6-mipi-csi2.c > +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c > @@ -243,7 +243,7 @@ static int __maybe_unused csi2_dphy_wait_ulp(struct csi2_dev *csi2) > } > > /* Waits for low-power LP-11 state on data and clock lanes. */ > -static int csi2_dphy_wait_stopstate(struct csi2_dev *csi2) > +static void csi2_dphy_wait_stopstate(struct csi2_dev *csi2) > { > u32 mask, reg; > int ret; > @@ -253,29 +253,21 @@ static int csi2_dphy_wait_stopstate(struct csi2_dev *csi2) > > ret = readl_poll_timeout(csi2->base + CSI2_PHY_STATE, reg, > (reg & mask) == mask, 0, 500000); > - if (ret) { > - v4l2_err(&csi2->sd, "LP-11 timeout, phy_state = 0x%08x\n", reg); > - return ret; > - } > - > - return 0; > + if (ret) > + v4l2_warn(&csi2->sd, "LP-11 timeout, phy_state = 0x%08x\n", reg); > } > > /* Wait for active clock on the clock lane. */ > -static int csi2_dphy_wait_clock_lane(struct csi2_dev *csi2) > +static void csi2_dphy_wait_clock_lane(struct csi2_dev *csi2) > { > u32 reg; > int ret; > > ret = readl_poll_timeout(csi2->base + CSI2_PHY_STATE, reg, > (reg & PHY_RXCLKACTIVEHS), 0, 500000); > - if (ret) { > - v4l2_err(&csi2->sd, "clock lane timeout, phy_state = 0x%08x\n", > - reg); > - return ret; > - } > - > - return 0; > + if (ret) > + v4l2_warn(&csi2->sd, "clock lane timeout, phy_state = 0x%08x\n", > + reg); > } > > /* Setup the i.MX CSI2IPU Gasket */ > @@ -316,9 +308,7 @@ static int csi2_start(struct csi2_dev *csi2) > csi2_enable(csi2, true); > > /* Step 5 */ > - ret = csi2_dphy_wait_stopstate(csi2); > - if (ret) > - goto err_assert_reset; > + csi2_dphy_wait_stopstate(csi2); > > /* Step 6 */ > ret = v4l2_subdev_call(csi2->src_sd, video, s_stream, 1); > @@ -327,14 +317,9 @@ static int csi2_start(struct csi2_dev *csi2) > goto err_assert_reset; > > /* Step 7 */ > - ret = csi2_dphy_wait_clock_lane(csi2); > - if (ret) > - goto err_stop_upstream; > - > + csi2_dphy_wait_clock_lane(csi2); > return 0; > > -err_stop_upstream: > - v4l2_subdev_call(csi2->src_sd, video, s_stream, 0); > err_assert_reset: > csi2_enable(csi2, false); > err_disable_clk: > -- > 2.20.1 >
On Wed, 2019-06-26 at 16:29 -0700, Steve Longerbeam wrote: > Hi Fabio, > > On 6/26/19 4:22 PM, Fabio Estevam wrote: > > Hi Steve, > > > > On Wed, Jun 26, 2019 at 6:19 PM Steve Longerbeam <slongerbeam@gmail.com> wrote: > > > > > Did you only get the LP-11 timeout warning message with this patch on > > > the OV5645, or both the LP-11 timeout and clock lane timeout warnings? > > > > With this patch applied I get only the LP-11 timeout warnings, not > > clock lane timeouts. > > Ok thanks for the confirmation that the imx6 CSI-2 receiver is able to > successfully move to stream on without seeing the LP-11 state in this > case. Are there any visual artifacts in the first frame(s) in this case? > So in my opinion the next version of this patch should make LP-11 > timeout a warning only, but keep the error return on clock lane timeouts. I agree. regards Philipp
Hi Philipp, On Thu, Jun 27, 2019 at 5:43 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > Are there any visual artifacts in the first frame(s) in this case? I do not observe visual artifacts when running gst-launch-1.0 v4l2src ! kmssink > > So in my opinion the next version of this patch should make LP-11 > > timeout a warning only, but keep the error return on clock lane timeouts. > > I agree. Here is a reworked version of Ezequiel's patch as per the suggestions: http://code.bulix.org/g5qap5-780475 Does this one look good? Thanks
On Thu, 2019-06-27 at 10:43 +0200, Philipp Zabel wrote: > On Wed, 2019-06-26 at 16:29 -0700, Steve Longerbeam wrote: > > Hi Fabio, > > > > On 6/26/19 4:22 PM, Fabio Estevam wrote: > > > Hi Steve, > > > > > > On Wed, Jun 26, 2019 at 6:19 PM Steve Longerbeam <slongerbeam@gmail.com> wrote: > > > > > > > Did you only get the LP-11 timeout warning message with this patch on > > > > the OV5645, or both the LP-11 timeout and clock lane timeout warnings? > > > > > > With this patch applied I get only the LP-11 timeout warnings, not > > > clock lane timeouts. > > > > Ok thanks for the confirmation that the imx6 CSI-2 receiver is able to > > successfully move to stream on without seeing the LP-11 state in this > > case. > > Are there any visual artifacts in the first frame(s) in this case? > I'll check. > > So in my opinion the next version of this patch should make LP-11 > > timeout a warning only, but keep the error return on clock lane timeouts. > > I agree. > Right. I only saw the LP-11 timeout, and wasn't really sure about the clock lane timeout. I'll drop that, improve the warning message and submit a v2. Thanks, Ezequiel
Hi Fabio, On Thu, 2019-06-27 at 09:38 -0300, Fabio Estevam wrote: > Hi Philipp, > > On Thu, Jun 27, 2019 at 5:43 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > > Are there any visual artifacts in the first frame(s) in this case? > > I do not observe visual artifacts when running gst-launch-1.0 v4l2src ! kmssink > > > > So in my opinion the next version of this patch should make LP-11 > > > timeout a warning only, but keep the error return on clock lane timeouts. > > > > I agree. > > Here is a reworked version of Ezequiel's patch as per the suggestions: > http://code.bulix.org/g5qap5-780475 > > Does this one look good? Limiting the change to wait_stopstate is fine, the actual message makes assumptions that could be misleading. How about: "Timeout waiting for LP-11 state on all active lanes. This is most likely caused by a bug in the sensor driver. Capture might fail or contain visual artifacts." I'd like to keep the phy_state register output though, if only as dev_dbg(). It contains useful output for debugging, for example if only some of the lanes are in stop state, which could indicate an issue with connections or lane configuration. regards Philipp
On Thu, 2019-06-27 at 14:56 +0200, Philipp Zabel wrote: > Hi Fabio, > > On Thu, 2019-06-27 at 09:38 -0300, Fabio Estevam wrote: > > Hi Philipp, > > > > On Thu, Jun 27, 2019 at 5:43 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > > > > Are there any visual artifacts in the first frame(s) in this case? > > > > I do not observe visual artifacts when running gst-launch-1.0 v4l2src ! kmssink > > > > > > So in my opinion the next version of this patch should make LP-11 > > > > timeout a warning only, but keep the error return on clock lane timeouts. > > > > > > I agree. > > > > Here is a reworked version of Ezequiel's patch as per the suggestions: > > http://code.bulix.org/g5qap5-780475 > > > > Does this one look good? > > Limiting the change to wait_stopstate is fine, the actual message > makes assumptions that could be misleading. How about: > > "Timeout waiting for LP-11 state on all active lanes. > This is most likely caused by a bug in the sensor driver. > Capture might fail or contain visual artifacts." > > I'd like to keep the phy_state register output though, if only as > dev_dbg(). It contains useful output for debugging, for example if only > some of the lanes are in stop state, which could indicate an issue with > connections or lane configuration. > > I think Philipp's suggestions looks very good, both the text and keeping the phy state. I think both should be kept in the warning. Fabio: feel free to submit a v2, or let me know so I'll add it to my TODO. Thanks, Eze
On 6/27/19 5:56 AM, Philipp Zabel wrote: > Hi Fabio, > > On Thu, 2019-06-27 at 09:38 -0300, Fabio Estevam wrote: >> Hi Philipp, >> >> On Thu, Jun 27, 2019 at 5:43 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: >> >>> Are there any visual artifacts in the first frame(s) in this case? >> I do not observe visual artifacts when running gst-launch-1.0 v4l2src ! kmssink >> >>>> So in my opinion the next version of this patch should make LP-11 >>>> timeout a warning only, but keep the error return on clock lane timeouts. >>> I agree. >> Here is a reworked version of Ezequiel's patch as per the suggestions: >> http://code.bulix.org/g5qap5-780475 >> >> Does this one look good? > Limiting the change to wait_stopstate is fine, the actual message > makes assumptions that could be misleading. How about: > > "Timeout waiting for LP-11 state on all active lanes. > This is most likely caused by a bug in the sensor driver. > Capture might fail or contain visual artifacts." Yes I agree that is more descriptive, if a bit wordy for a kernel error message. I think it could be reduced, something like: "LP-11 wait timeout on all lanes, likely a sensor driver bug, expect capture failures." > > I'd like to keep the phy_state register output though, if only as > dev_dbg(). It contains useful output for debugging, for example if only > some of the lanes are in stop state, which could indicate an issue with > connections or lane configuration. Agreed! Steve
On Thu, Jun 27, 2019 at 3:45 PM Ezequiel Garcia <ezequiel@collabora.com> wrote: > I think Philipp's suggestions looks very good, both the text and keeping > the phy state. I think both should be kept in the warning. > > Fabio: feel free to submit a v2, or let me know so I'll add it to my TODO. I have just sent a v2 with Philipp's suggestions. Thanks
On Thu, 2019-06-27 at 15:12 -0700, Steve Longerbeam wrote: > > On 6/27/19 5:56 AM, Philipp Zabel wrote: > > Hi Fabio, > > > > On Thu, 2019-06-27 at 09:38 -0300, Fabio Estevam wrote: > > > Hi Philipp, > > > > > > On Thu, Jun 27, 2019 at 5:43 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > > > > > > Are there any visual artifacts in the first frame(s) in this case? > > > > > > I do not observe visual artifacts when running gst-launch-1.0 v4l2src ! kmssink > > > > > > > > So in my opinion the next version of this patch should make LP-11 > > > > > timeout a warning only, but keep the error return on clock lane timeouts. > > > > > > > > I agree. > > > > > > Here is a reworked version of Ezequiel's patch as per the suggestions: > > > http://code.bulix.org/g5qap5-780475 > > > > > > Does this one look good? > > > > Limiting the change to wait_stopstate is fine, the actual message > > makes assumptions that could be misleading. How about: > > > > "Timeout waiting for LP-11 state on all active lanes. > > This is most likely caused by a bug in the sensor driver. > > Capture might fail or contain visual artifacts." > > Yes I agree that is more descriptive, if a bit wordy for a kernel error > message. Haha, yes, I remember that thought crossing my mind. > I think it could be reduced, something like: > > "LP-11 wait timeout on all lanes, likely a sensor driver bug, expect > capture failures." Much better, thanks. regards Philipp
diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c index f29e28df36ed..10342434e797 100644 --- a/drivers/staging/media/imx/imx6-mipi-csi2.c +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c @@ -243,7 +243,7 @@ static int __maybe_unused csi2_dphy_wait_ulp(struct csi2_dev *csi2) } /* Waits for low-power LP-11 state on data and clock lanes. */ -static int csi2_dphy_wait_stopstate(struct csi2_dev *csi2) +static void csi2_dphy_wait_stopstate(struct csi2_dev *csi2) { u32 mask, reg; int ret; @@ -253,29 +253,21 @@ static int csi2_dphy_wait_stopstate(struct csi2_dev *csi2) ret = readl_poll_timeout(csi2->base + CSI2_PHY_STATE, reg, (reg & mask) == mask, 0, 500000); - if (ret) { - v4l2_err(&csi2->sd, "LP-11 timeout, phy_state = 0x%08x\n", reg); - return ret; - } - - return 0; + if (ret) + v4l2_warn(&csi2->sd, "LP-11 timeout, phy_state = 0x%08x\n", reg); } /* Wait for active clock on the clock lane. */ -static int csi2_dphy_wait_clock_lane(struct csi2_dev *csi2) +static void csi2_dphy_wait_clock_lane(struct csi2_dev *csi2) { u32 reg; int ret; ret = readl_poll_timeout(csi2->base + CSI2_PHY_STATE, reg, (reg & PHY_RXCLKACTIVEHS), 0, 500000); - if (ret) { - v4l2_err(&csi2->sd, "clock lane timeout, phy_state = 0x%08x\n", - reg); - return ret; - } - - return 0; + if (ret) + v4l2_warn(&csi2->sd, "clock lane timeout, phy_state = 0x%08x\n", + reg); } /* Setup the i.MX CSI2IPU Gasket */ @@ -316,9 +308,7 @@ static int csi2_start(struct csi2_dev *csi2) csi2_enable(csi2, true); /* Step 5 */ - ret = csi2_dphy_wait_stopstate(csi2); - if (ret) - goto err_assert_reset; + csi2_dphy_wait_stopstate(csi2); /* Step 6 */ ret = v4l2_subdev_call(csi2->src_sd, video, s_stream, 1); @@ -327,14 +317,9 @@ static int csi2_start(struct csi2_dev *csi2) goto err_assert_reset; /* Step 7 */ - ret = csi2_dphy_wait_clock_lane(csi2); - if (ret) - goto err_stop_upstream; - + csi2_dphy_wait_clock_lane(csi2); return 0; -err_stop_upstream: - v4l2_subdev_call(csi2->src_sd, video, s_stream, 0); err_assert_reset: csi2_enable(csi2, false); err_disable_clk:
Not all sensors will be able to guarantee a proper initial state. This may be either because the driver is not properly written, or (probably unlikely) because the hardware won't support it. While the right solution in the former case is to fix the sensor driver, the real world not always allows right solutions, due to lack of available documentation and support on these sensors. Let's relax this requirement, and allow the driver to support stream start, even if the sensor initial sequence wasn't the expected. A warning is still emitted, so users should be hinted that something is off. Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> --- drivers/staging/media/imx/imx6-mipi-csi2.c | 33 ++++++---------------- 1 file changed, 9 insertions(+), 24 deletions(-)