[PATCH][RESEND] drm/bridge: ti-sn65dsi83: Check link status register after enabling the bridge

[Frieder Schrempf]

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