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.
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.
