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

[Marek Vasut]

On 9/7/21 9:31 AM, Andrzej Hajda wrote:
> On 07.09.2021 04:39, Marek Vasut wrote:
>> In rare cases, the bridge may not start up correctly, which usually
>> leads to no display output. In case this happens, warn about it in
>> the kernel log.
>>
>> Signed-off-by: Marek Vasut <marex at denx.de>
>> Cc: Jagan Teki <jagan at amarulasolutions.com>
>> Cc: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>> Cc: Linus Walleij <linus.walleij at linaro.org>
>> Cc: Robert Foss <robert.foss at linaro.org>
>> Cc: Sam Ravnborg <sam at ravnborg.org>
>> Cc: dri-devel at lists.freedesktop.org
>> ---
>> NOTE: See the following:
>> https://e2e.ti.com/support/interface-group/interface/f/interface-forum/942005/sn65dsi83-dsi83-lvds-bridge---sporadic-behavior---no-video
>> https://community.nxp.com/t5/i-MX-Processors/i-MX8M-MIPI-DSI-Interface-LVDS-Bridge-Initialization/td-p/1156533
>> ---
>>    drivers/gpu/drm/bridge/ti-sn65dsi83.c | 5 +++++
>>    1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>> index a32f70bc68ea4..4ea71d7f0bfbc 100644
>> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>> @@ -520,6 +520,11 @@ static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
>>    	/* Clear all errors that got asserted during initialization. */
>>    	regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
>>    	regmap_write(ctx->regmap, REG_IRQ_STAT, pval);
> 
> 
> It does not look as correct error handling, maybe it would be good to
> analyze and optionally report 'unexpected' errors here as well.

The above is correct -- it clears the status register because the setup 
might've set random bits in that register. Then we wait a bit, let the 
link run, and read them again to get the real link status in this new 
piece of code below, hence the usleep_range there. And then if the link 
indicates a problem, we know it is a problem.

>> +
>> +	usleep_range(10000, 12000);
>> +	regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
>> +	if (pval)
>> +		dev_err(ctx->dev, "Unexpected link status 0x%02x\n", pval);
> 
> 
> I am not sure what is the case here but it looks like 'we do not know
> what is going on, so let's add some diagnostic messages to gather info
> and figure it out later'.

That's pretty much the case, see the two links above in the NOTE 
section. If something goes wrong, we print the value for the user 
(usually developer) so they can fix their problems. We cannot do much 
better in the attach callback.

The issue I ran into (and where this would be helpful information to me 
during debugging, since the issue happened real seldom, see also the 
NOTE links above) is that the DSI controller driver started streaming 
video on the data lanes before the DSI83 had a chance to initialize. 
This worked most of the time, except for a few exceptions here and 
there, where the video didn't start. This does set link status bits 
consistently. In the meantime, I fixed the controller driver (so far 
downstream, due to ongoing discussion).

> Whole driver lacks IRQ handler which IMO could perform better diagnosis,
> and I guess it could also help in recovery, but this is just my guess.
> So if this patch is enough for now you can add:

No, IRQ won't help you here, because by the time you get the IRQ, the 
DSI host already started streaming video on data lanes and you won't be 
able to correctly reinit the DSI83 unless you communicate to the DSI 
host that it should switch the data lanes back to LP11.

And for that, there is a bigger chunk missing really. What needs to be 
added is a way for the DSI bridge / panel to communicate its needs to 
the DSI host -- things like "I need DSI clock lane frequency f MHz, I 
need clock lane in HS mode and data lanes in LP11 mode". If you look at 
the way DSI hosts and bridges/panels work out the DSI link parameters, 
you will notice they basically do it each on their own, there is no such 
API or communication channel.

With the above in place, you could have an interrupt which would use it, 
notify the host to configure the DSI bus in required way, then you would 
be able to reinit the DSI83, and then again notify the DSI host to start 
streaming video.

> Reviewed-by: Andrzej Hajda <a.hajda at samsung.com>
> 
> Regards
> Andrzej
> 
> 
>>    }
>>    
>>    static void sn65dsi83_atomic_disable(struct drm_bridge *bridge,
>>
> 


-- 
Best regards,
Marek Vasut