Hi Steve, On Tue, Aug 13, 2019 at 04:27:06PM -0700, Steve Longerbeam wrote: > Hi Sakari, > > On 8/13/19 1:28 AM, Sakari Ailus wrote: > > Hi Steve, > > > > On Thu, Aug 08, 2019 at 11:02:29AM -0700, Steve Longerbeam wrote: > > > > > > On 8/8/19 1:53 AM, Philipp Zabel wrote: > > > > Hi Sakari, > > > > > > > > On Thu, 2019-08-08 at 11:26 +0300, Sakari Ailus wrote: > > > > [...] > > > > > > > Have you checked how it works if you simply leave out this test? > > > > Whether this works or not depends on the sensor used, and for some > > > > sensor/drivers may depend on timing (or random factors influencing it). > > > > See below. > > > > > > > > [...] > > > > > Some devices can be commanded to enter LP-11 state but some will just > > > > > briefly visit that state. The LP-11 state is mandatory but software should > > > > > not be involved in detecting it if at all possible. > > > > This is a good point. Devices that can be set to LP-11 state > > > > immediately, but that don't stay there long enough (either because they > > > > wait for less than the required by spec 100µs, or because system load > > > > causes this check to be executed too late) may end up working reliably > > > > even though the warning fires. > > > > > So if the hardware does not require further initialisation to be done in > > > > > LP-11 state, you should remove the check. > > > > > > > > > > > We had to fix at least OV5645 and OV5640 recently because of this, > > > > > > and I can imagine more drivers will have the same issue. > > > > > This is actually an issue in the IMX driver (or hardware), not in the > > > > > sensor driver. It may be that sometimes it's easier to work around it in > > > > > the sensor driver. > > > > > > > > > > So, I'd like to know whether the check itself is a driver bug, or something > > > > > that the hardware requires. The fact that you're sending this patch > > > > > suggests the former. > > > > This is something that the hardware requires, according to the reference > > > > manual. See the comment in drivers/staging/media/imx/imx6-mipi-csi2.c, > > > > especially step 5.: > > > > > > > > /* > > > > * The required sequence of MIPI CSI-2 startup as specified in the i.MX6 > > > > * reference manual is as follows: > > > > * > > > > * 1. Deassert presetn signal (global reset). > > > > * It's not clear what this "global reset" signal is (maybe APB > > > > * global reset), but in any case this step would be probably > > > > * be carried out during driver load in csi2_probe(). > > > > * > > > > * 2. Configure MIPI Camera Sensor to put all Tx lanes in LP-11 state. > > > > * This must be carried out by the MIPI sensor's s_power(ON) subdev > > > > * op. > > > > * > > > > * 3. D-PHY initialization. > > > > * 4. CSI2 Controller programming (Set N_LANES, deassert PHY_SHUTDOWNZ, > > > > * deassert PHY_RSTZ, deassert CSI2_RESETN). > > > > * 5. Read the PHY status register (PHY_STATE) to confirm that all data and > > > > * clock lanes of the D-PHY are in LP-11 state. > > > > * 6. Configure the MIPI Camera Sensor to start transmitting a clock on the > > > > * D-PHY clock lane. > > > > * 7. CSI2 Controller programming - Read the PHY status register (PHY_STATE) > > > > * to confirm that the D-PHY is receiving a clock on the D-PHY clock lane. > > > > */ > > > > > > > > I read this as the hardware needing to see the LP-11 -> HS transition > > > > after the DPHY reset has been released, and before the CSI2 RX > > > > controller is programmed. > > > I think that's a fair assumption, and there's another paragraph at the end > > > of that comment above that adds more evidence to that: > > > > > > ... > > > * All steps 3 through 7 are carried out by csi2_s_stream(ON) here. Step > > > * 6 is accomplished by calling the source subdev's s_stream(ON) between > > > * steps 5 and 7. > > > */ > > > > > > > > > So the driver is expecting that the LP-11 state persists until step 6, at > > > which the LP-11 -> HS transition occurs when streaming is started at the > > > transmitter. > > > > > > But if the transmitter only stays in LP-11 state for the minimum 100 usec > > > after it is powered on, and then _automatically_ transitions to HS, it's > > > quite possible the LP-11 -> HS transition will happen long before the DPHY > > > is taken out of reset. That's because the transmitter's s_power(ON) is > > > called when the capture device is opened (via v4l2_pipeline_pm_use()), but > > > the steps above are carried out when streaming starts, so userland would > > > have to issue VIDIOC_STREAMON well within the 100 usec expires after > > > open()'ing the device, for the receiver hardware to see the transition. > > After powering on the sensor it sensor should stay in (software) standby > > mode until streaming is started. It should still have its lanes in LP-11 > > mode if the sensor supports it (as documented) until the sensor switches to > > the streaming mode. > > Yes that's my understanding too. > > > > > > Perhaps that would be an argument for moving steps 1 - 5 into the driver's > > > s_power(ON) call, which would first call s_power(ON) to the transmitter and > > > then immediately go through steps 1 - 5. Steps 6,7 would then remain in > > > s_stream(ON). > > Not all sensor drivers power on the sensor before starting streaming. > > > > Perhaps we could add a prepare_streaming() callback (in absence of a better > > proposal) to address that? We'd also need a corresponding > > unprepare_streaming() callback as well --- to power off the sensor. I think > > this only should be done if the sensor can be switched to LP-11 explicitly; > > many simply don't support that. > > So you are saying a sensor can power on and place its lanes in LP-11 in this > new prepare_streaming() callback, the reason being, this can be called > closer in time, and just before, the receiver DPHY is brought out of reset, > in order for the receiver DPHY to be able to detect a possible early > transition to HS mode? > > While I don't have any objection to adding a prepare_streaming() callback > (it could be useful for other reasons), I don't see why the sensor could not > simply make use of existing s_power to power on and place the lanes in > LP-11. That is what it's intended for anyway. The vast majority of new drivers no longer have s_power callbacks --- they use runtime PM instead. > > There's really no drawback to the patch I posted that moves receiver DPHY > init and reset-release to s_power time in the receiver driver. If given a > choice, an isolated fix to the imx mipi csi-2 receiver driver should be > preferable to implementing a new callback. > > But perhaps one advantage to the prepare_streaming() callback idea, is that > there is more of a guarantee of minimum time between the > sensor-power-on-and-enter-LP-11 event and receiver-DPHY-reset-release event, > since s_power is normally called from v4l2_pipeline_pm_use() which could add > extra overhead between the time s_power is called at the transmitter and at > the receiver (but not much - the transmitter is connected directly to > receiver in the graph). -- Regards, Sakari Ailus
