[Linux-stm32] [PATCH 0/1] [Kontron] [URGENT]: drm/bridge/synopsys/dsi: fix activation sequence

[Philippe CORNU]

Hi Antonio,

And thank you for the patch.

If it helps a customer, I have nothing more to say except that it's a great patch!

You're pointing to a real complex problem. You may have seen that a first tentative to solve it has been done by Matt Redfearn on the bridge side (here the adv7511/7533 RGB to DSI bridge, controlled by I2C, https://cgit.freedesktop.org/drm/drm-misc/commit/drivers/gpu/drm/bridge/adv7511?id=83f35bc3a852f1c3892c7474998c5cec707c7ba3) and 2 months after, Rob Clarke revert this patch, explaining how it will be complex to find a proper fix due to the legacy and the complexity (https://cgit.freedesktop.org/drm/drm-misc/commit/drivers/gpu/drm/bridge/adv7511?id=2472518e44eee3a45bd436a4162046790b74767a).

Note that in our internal baseline, we are still using the Matt Redfearn patch for the adv7533, this is of course not perfect, but easier for the moment to enable the adv7533 use case.

DSI probing dynamic is very complex: panels & bridges dynamics are different, but also "direct" probing (stm) vs "bind/unbind" (hisilicon) strategies add more complexity...

Your patch looks good and if you have done enough tests on both MP15x eval and disco boards, then I think you can share it with the customer (of course please warn the customer that this fix will probably not be the final one in future kernel versions). 

Nevertheless, I wonder if a patch in the SN65DSI84 bridge driver like the one we use for the adv7533 may also help to enable the use case.

Somehow, your patch could be used to precisely start these complex discussions with the drm community and the users of the dsi designware bridge.

Best regards,
Philippe :-)

-----Original Message-----
From: Antonio BORNEO <antonio.borneo at st.com> 
Sent: Monday, June 22, 2020 14:37
To: Yannick FERTRE <yannick.fertre at st.com>; Philippe CORNU <philippe.cornu at st.com>
Cc: Bernard PUEL <bernard.puel at st.com>; Valere DELONG <valere.delong at st.com>; Antonio BORNEO <antonio.borneo at st.com>; linux-stm32 at st-md-mailman.stormreply.com
Subject: [PATCH 0/1] [Kontron] [URGENT]: drm/bridge/synopsys/dsi: fix activation sequence

Customer is waiting for this patch, but I don't want to send it out without the approval of the experts.

Background:
Kontron is connecting a dsi-to-lvds bridge TI sn65dsi83 to MP15x.
The driver for the bridge is not mainline, but taken from
https://github.com/varigit/linux-imx/tree/imx_4.14.98_2.0.0_ga_var01/drivers/gpu/drm/bridge/sn65dsi83
and rebased by customer.

Note: I do not have the bridge above, so I have hacked the driver to skip all the I2C transfer. My target is to get it loaded and to get the DSI signal out.

Kontron reports:
| we want to connect a LVDS display on the DSI port of the MP157.
| The DSI2LVDS bridge chip is an SN65DSI84 chip on i2c bus. I've 
| configured the connectors in the following way:
|
| ltdc at ep1 -> dsi at ep0, dsi at ep1 -> lvds at ep0
|
| The documentation mentions that a panel or bridge node must be 
| available in the DSI part. So for a panel this works. But what must be 
| the entry for a bridge?
| My bridge must be located on the i2c bus because of the configuration 
| interface. The display parameters are configured in the bridge node.
|
| When the DSI node doesn't contain a panel node, DRM/modetest doesn't 
| even recognize the DSI-1 connector. When the node contains a panel, 
| the DSI-1 is created, but no framebuffer can be created. Seems this is 
| not the right way, else i have to> configure the display twice (in the 
| dsi node and in the bridge).

Verified on my side. If there is no panel as subnode of DSI, the DSI does not start and also LTDC fails the probe.

This case would be common for each bridge that is not initialized through DSI but through e.g. I2C.

The bug is schematized in the activation sequence of current code:

LTDC driver:
 .probe
	stm_drm_platform_probe()
		drv_load()
			ltdc_load()
				drm_of_find_panel_or_bridge()

The call drm_of_find_panel_or_bridge() returns -EPROBE_DEFER if the driver of DSI has not registered the bridge (rgb to dsi) by calling drm_bridge_add().

DSI driver:
 .probe
	dw_mipi_dsi_stm_probe()
		dw_mipi_dsi_probe()
			mipi_dsi_host_register()

As you can see, the probe of DSI does not call drm_bridge_add() directly.
If as subnode of DSI there is a "panel", mipi_dsi_host_register() will cause the probe() of the panel.
At least for the panels we use today with MP15x, their probe() terminates by attaching the panel to the DSI by calling
mipi_dsi_attach() that, in turn, calls the .attach() method of the DSI driver, reported below:

 .attach
	dw_mipi_dsi_host_attach()
		drm_bridge_add()

If the DSI has no subnodes, the .attach method will never get called, the DSI does not declare itself as a DRM bridge, the ltdc will defer forever its own probe.

This patch moves the relevant part of the attach method in probe.
Of course, the corresponding detach part goes in remove.

The binding document in v5.4 reports that the DSI subnode is mandatory. But from v5.5 the yaml document drops the mandatory requirement.

I have tested that on EV1 we do not have regressions.
Test on DK2 on-going.
Do you approve sending this out to customer?

By the way, I highlighted with FIXME another bug in the code.
I will check it later.

Antonio Borneo (1):
  drm/bridge/synopsys: fix activation sequence

 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 72 ++++++++++---------
 1 file changed, 37 insertions(+), 35 deletions(-)

--
2.17.1