Am 05.04.22 um 23:36 schrieb Marek Vasut: > On 4/5/22 17:24, Dave Stevenson wrote: > > Hi, > >>>>>> If we can initialise the DSI host before the bridge for the >>>>>> pre_enable, then all the configuration moves to the atomic_pre_enable >>>>>> and there should be no need to have the delay. >>>>>> >>>>>> I can't 100% guarantee that, but one of the folks on the Pi forums is >>>>>> using [1] which does that, and is reporting it working well. (He's >>>>>> also using the DSI85 to take 2 DSI links and drive 2 LVDS single link >>>>>> panels) >>>>> >>>>> It seems to me that checking whether the bridge got correctly >>>>> initialized is orthogonal to the aforementioned patchset though ? >>>> >>>> It's the delay that is ugly. >>> >>> You do need to wait a little after the initialization and before >>> checking the status, so that delay is not going away no matter how you >>> frob with the DSI bridge. >>> >>>> Put the check in atomic_enable which will be slightly later than >>>> configuration in pre_enable? Check that sufficient jiffies have passed >>>> if you needed. >>>> Or wire up the IRQ line from the SN65DSI83 rather than polling the IRQ >>>> Status register. Delayed workqueue if the IRQ isn't wired up. >>> >>> Are you able to do such deferred non-atomic operations in atomic_enable >>> callback ? >> >> atomic_enable returns void so you can't fail the atomic_enable. >> All you're doing is reading a register and potentially logging an >> error - surely that can be done from any context. >> >>>> If I read it right your log message is triggered by any bit being set >>>> in REG_IRQ_STAT. So an inconveniently timed correctable DSI error will >>>> set bit 4 and log the error even though it's been corrected. Likewise >>>> bit 7 / CHA_SYNCH_ERR could get triggered by an H or V sync packet >>>> being received in that 10-12ms window (we're in atomic_enable, so >>>> video is already running). >>> >>> There should be no bits set in the IRQ_STAT register if everything works >>> as it should. >> >> On a perfect link, yes. > > If your hardware is broken, then you really do want to know about it. > >> Looking at the top 4 bits. >> >> Bit 4 >> CHA_COR_ECC_ERR >> When the DSI channel A packet processor detects a correctable ECC >> error, this bit is set. >> >> The error is corrected, so why do we care? The display is still working. > > If you get a lot of those correctable errors, your display might not > work at all. So we do care. > >> Bit 5 >> CHA_UNC_ECC_ERR >> When the DSI channel A packet processor detects an uncorrectable ECC >> error, this bit is set; >> Bit 6 >> CHA_CRC_ERR >> When the DSI channel A packet processor detects a data stream CRC error, >> this bit is set >> >> It doesn't say what behaviour the DSI83 will take under these >> circumstances, but shouldn't be fatal to the display. > > See above, it is an error, hardware is broken, you want to know about > this and fix the hardware. > >> Bit 7 >> CHA_SYNCH_ERR >> When the DSI channel A packet processor detects an HS or VS >> synchronization error, that is, an unexpected sync packet; this bit is >> set. >> >> It's happened, but shouldn't be fatal, so why do we care? The display >> should pick up correctly at the start of the next frame. > > Or maybe it won't because of noise on the DSI link, fix the hardware. > > Sorry, I am not happy about hiding errors and trying to somehow justify > they are OK. They are not, the hardware is likely broken and it should > be fixed, that is the right way to handle these errors. > >> As I've already said, we're in atomic_enable and video is therefore >> running, this is highly plausibly going to happen. We've delayed for >> 4-5ms in sn65dsi83_atomic_enable, so we're a third of the way through >> a frame at 60fps. The chances of seeing a VS or HS packet at an >> unexpected time is therefore high. >> >> Bits 2 & 3 look equally inconsequential. >> >> Bit 0 as PLL unlock would cause grief. >> >>>> If it's the PLL being unlocked that is the issue then it should only >>>> be checking bit 0. Or possibly reading the actual PLL lock status from >>>> REG_RC_LVDS_PLL_PLL_EN_STAT. Although as we've already checked that >>>> the PLL is locked via REG_RC_LVDS_PLL_PLL_EN_STAT earlier in the >>>> atomic_enable, I'm now a little confused as to the condition you are >>>> actually wanting to detect. >>> >>> Any outstanding errors which are currently hidden and only show up >>> sporadically at the worst possible moment. >> >> If you were constantly looking at the status then that would be >> reasonable. >> To be looking during one specific 10-12ms period of time for some >> fairly generic status values seems a little redundant, and a waste of >> time in delaying the atomic_enable. > > Sorry, I disagree. I think 10ms extra time in atomic_enable() is a good > trade-off for knowing about possible hardware problems sooner rather > than later. Isn't the delay at this point even required (regardless of the debugging information), as the init sequence in the datasheet mentions a 10 ms delay after issuing a soft reset and before the DSI stream is enabled? Or is this handled elsewhere? > >> It'll be interesting to see if these events just go away when the >> initialisation sequence specified in the datasheet is being followed. >> Let's leave the debate until then, as it's currently fairly arbitrary. > > No, your patch series is orthogonal to this patch. > >
