Hi,
We've encountered an issue with the RaspberryPi DSI panel that prevented the whole display driver from probing.
The issue is described in detail in the commit 7213246a803f ("drm/vc4: dsi: Only register our component once a DSI device is attached"), but the basic idea is that since the panel is probed through i2c, there's no synchronization between its probe and the registration of the MIPI-DSI host it's attached to.
We initially moved the component framework registration to the MIPI-DSI Host attach hook to make sure we register our component only when we have a DSI device attached to our MIPI-DSI host, and then use lookup our DSI device in our bind hook.
However, all the DSI bridges controlled through i2c are only registering their associated DSI device in their bridge attach hook, meaning with our change above, we never got that far, and therefore ended up in the same situation than the one we were trying to fix for panels.
The best practice to avoid those issues is to register its functions only after all its dependencies are live. We also shouldn't wait any longer than we should to play nice with the other components that are waiting for us, so in our case that would mean moving the DSI device registration to the bridge probe.
I also had a look at all the DSI hosts, and it seems that exynos, kirin and msm would be affected by this and wouldn't probe anymore after those changes. Exynos and kirin seems to be simple enough for a mechanical change (that still requires to be tested), but the changes in msm seemed to be far more important and I wasn't confortable doing them.
Let me know what you think, Maxime
---
Changes from v3: - Converted exynos and kirin - Converted all the affected bridge drivers - Reworded the documentation a bit
Changes from v2: - Changed the approach as suggested by Andrzej, and aligned the bridge on the panel this time. - Fixed some typos
Changes from v1: - Change the name of drm_of_get_next function to drm_of_get_bridge - Mention the revert of 87154ff86bf6 and squash the two patches that were reverting that commit - Add some documentation - Make drm_panel_attach and _detach succeed when no callback is there
Maxime Ripard (24): drm/bridge: Add documentation sections drm/bridge: Document the probe issue with MIPI-DSI bridges drm/mipi-dsi: Create devm device registration drm/mipi-dsi: Create devm device attachment drm/bridge: adv7533: Switch to devm MIPI-DSI helpers drm/bridge: adv7511: Register and attach our DSI device at probe drm/bridge: anx7625: Switch to devm MIPI-DSI helpers drm/bridge: anx7625: Register and attach our DSI device at probe drm/bridge: lt8912b: Switch to devm MIPI-DSI helpers drm/bridge: lt8912b: Register and attach our DSI device at probe drm/bridge: lt9611: Switch to devm MIPI-DSI helpers drm/bridge: lt9611: Register and attach our DSI device at probe drm/bridge: lt9611uxc: Switch to devm MIPI-DSI helpers drm/bridge: lt9611uxc: Register and attach our DSI device at probe drm/bridge: ps8640: Switch to devm MIPI-DSI helpers drm/bridge: ps8640: Register and attach our DSI device at probe drm/bridge: sn65dsi83: Switch to devm MIPI-DSI helpers drm/bridge: sn65dsi83: Register and attach our DSI device at probe drm/bridge: sn65dsi86: Switch to devm MIPI-DSI helpers drm/bridge: sn65dsi86: Register and attach our DSI device at probe drm/bridge: tc358775: Switch to devm MIPI-DSI helpers drm/bridge: tc358775: Register and attach our DSI device at probe drm/kirin: dsi: Adjust probe order drm/exynos: dsi: Adjust probe order
Documentation/gpu/drm-kms-helpers.rst | 12 +++ drivers/gpu/drm/bridge/adv7511/adv7511.h | 1 - drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 15 ++- drivers/gpu/drm/bridge/adv7511/adv7533.c | 20 +--- drivers/gpu/drm/bridge/analogix/anx7625.c | 40 ++++---- drivers/gpu/drm/bridge/lontium-lt8912b.c | 31 ++---- drivers/gpu/drm/bridge/lontium-lt9611.c | 62 +++++------- drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 65 +++++------- drivers/gpu/drm/bridge/parade-ps8640.c | 101 ++++++++++--------- drivers/gpu/drm/bridge/tc358775.c | 50 +++++---- drivers/gpu/drm/bridge/ti-sn65dsi83.c | 86 ++++++++-------- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 94 ++++++++--------- drivers/gpu/drm/drm_bridge.c | 69 ++++++++++++- drivers/gpu/drm/drm_mipi_dsi.c | 81 +++++++++++++++ drivers/gpu/drm/exynos/exynos_drm_dsi.c | 19 ++-- drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c | 27 +++-- include/drm/drm_mipi_dsi.h | 4 + 17 files changed, 460 insertions(+), 317 deletions(-)
The bridge documentation overview is quite packed already, and we'll add some more documentation that isn't part of an overview at all.
Let's add some sections to the documentation to separate each bits.
Reviewed-by: Andrzej Hajda a.hajda@samsung.com Reviewed-by: Sam Ravnborg sam@ravnborg.org Signed-off-by: Maxime Ripard maxime@cerno.tech --- Documentation/gpu/drm-kms-helpers.rst | 6 ++++++ drivers/gpu/drm/drm_bridge.c | 14 +++++++++----- 2 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst index 389892f36185..10f8df7aecc0 100644 --- a/Documentation/gpu/drm-kms-helpers.rst +++ b/Documentation/gpu/drm-kms-helpers.rst @@ -151,6 +151,12 @@ Overview .. kernel-doc:: drivers/gpu/drm/drm_bridge.c :doc: overview
+Display Driver Integration +-------------------------- + +.. kernel-doc:: drivers/gpu/drm/drm_bridge.c + :doc: display driver integration + Bridge Operations -----------------
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index a8ed66751c2d..baff74ea4a33 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -49,6 +49,15 @@ * Chaining multiple bridges to the output of a bridge, or the same bridge to * the output of different bridges, is not supported. * + * &drm_bridge, like &drm_panel, aren't &drm_mode_object entities like planes, + * CRTCs, encoders or connectors and hence are not visible to userspace. They + * just provide additional hooks to get the desired output at the end of the + * encoder chain. + */ + +/** + * DOC: display driver integration + * * Display drivers are responsible for linking encoders with the first bridge * in the chains. This is done by acquiring the appropriate bridge with * of_drm_find_bridge() or drm_of_find_panel_or_bridge(), or creating it for a @@ -85,11 +94,6 @@ * helper to create the &drm_connector, or implement it manually on top of the * connector-related operations exposed by the bridge (see the overview * documentation of bridge operations for more details). - * - * &drm_bridge, like &drm_panel, aren't &drm_mode_object entities like planes, - * CRTCs, encoders or connectors and hence are not visible to userspace. They - * just provide additional hooks to get the desired output at the end of the - * encoder chain. */
static DEFINE_MUTEX(bridge_lock);
On Fri, 10 Sep 2021 12:11:55 +0200, Maxime Ripard wrote:
The bridge documentation overview is quite packed already, and we'll add some more documentation that isn't part of an overview at all.
Let's add some sections to the documentation to separate each bits.
Applied to drm/drm-misc (drm-misc-next).
Thanks! Maxime
Interactions between bridges, panels, MIPI-DSI host and the component framework are not trivial and can lead to probing issues when implementing a display driver. Let's document the various cases we need too consider, and the solution to support all the cases.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- Documentation/gpu/drm-kms-helpers.rst | 6 +++ drivers/gpu/drm/drm_bridge.c | 57 +++++++++++++++++++++++++++ 2 files changed, 63 insertions(+)
diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst index 10f8df7aecc0..ec2f65b31930 100644 --- a/Documentation/gpu/drm-kms-helpers.rst +++ b/Documentation/gpu/drm-kms-helpers.rst @@ -157,6 +157,12 @@ Display Driver Integration .. kernel-doc:: drivers/gpu/drm/drm_bridge.c :doc: display driver integration
+Special Care with MIPI-DSI bridges +---------------------------------- + +.. kernel-doc:: drivers/gpu/drm/drm_bridge.c + :doc: special care dsi + Bridge Operations -----------------
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index baff74ea4a33..7cc2d2f94ae3 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -96,6 +96,63 @@ * documentation of bridge operations for more details). */
+/** + * DOC: special care dsi + * + * The interaction between the bridges and other frameworks involved in + * the probing of the upstream driver and the bridge driver can be + * challenging. Indeed, there's multiple cases that needs to be + * considered: + * + * - The upstream driver doesn't use the component framework and isn't a + * MIPI-DSI host. In this case, the bridge driver will probe at some + * point and the upstream driver should try to probe again by returning + * EPROBE_DEFER as long as the bridge driver hasn't probed. + * + * - The upstream driver doesn't use the component framework, but is a + * MIPI-DSI host. The bridge device uses the MIPI-DCS commands to be + * controlled. In this case, the bridge device is a child of the + * display device and when it will probe it's assured that the display + * device (and MIPI-DSI host) is present. The upstream driver will be + * assured that the bridge driver is connected between the + * &mipi_dsi_host_ops.attach and &mipi_dsi_host_ops.detach operations. + * Therefore, it must run mipi_dsi_host_register() in its probe + * function, and then run drm_bridge_attach() in its + * &mipi_dsi_host_ops.attach hook. + * + * - The upstream driver uses the component framework and is a MIPI-DSI + * host. The bridge device uses the MIPI-DCS commands to be + * controlled. This is the same situation than above, and can run + * mipi_dsi_host_register() in either its probe or bind hooks. + * + * - The upstream driver uses the component framework and is a MIPI-DSI + * host. The bridge device uses a separate bus (such as I2C) to be + * controlled. In this case, there's no correlation between the probe + * of the bridge and upstream drivers, so care must be taken to avoid + * an endless EPROBE_DEFER loop, with each driver waiting for the + * other to probe. + * + * The ideal pattern to cover the last item (and all the others in the + * MIPI-DSI host driver case) is to split the operations like this: + * + * - The MIPI-DSI host driver must run mipi_dsi_host_register() in its + * probe hook. It will make sure that the MIPI-DSI host sticks around, + * and that the driver's bind can be called. + * + * - In its probe hook, the bridge driver must try to find its MIPI-DSI + * host, register as a MIPI-DSI device and attach the MIPI-DSI device + * to its host. The bridge driver is now functional. + * + * - In its &struct mipi_dsi_host_ops.attach hook, the MIPI-DSI host can + * now add its component. Its bind hook will now be called and since + * the bridge driver is attached and registered, we can now look for + * and attach it. + * + * At this point, we're now certain that both the upstream driver and + * the bridge driver are functional and we can't have a deadlock-like + * situation when probing. + */ + static DEFINE_MUTEX(bridge_lock); static LIST_HEAD(bridge_list);
W dniu 10.09.2021 o 12:11, Maxime Ripard pisze:
Interactions between bridges, panels, MIPI-DSI host and the component framework are not trivial and can lead to probing issues when implementing a display driver. Let's document the various cases we need too consider, and the solution to support all the cases.
Signed-off-by: Maxime Ripard maxime@cerno.tech
Documentation/gpu/drm-kms-helpers.rst | 6 +++ drivers/gpu/drm/drm_bridge.c | 57 +++++++++++++++++++++++++++ 2 files changed, 63 insertions(+)
diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst index 10f8df7aecc0..ec2f65b31930 100644 --- a/Documentation/gpu/drm-kms-helpers.rst +++ b/Documentation/gpu/drm-kms-helpers.rst @@ -157,6 +157,12 @@ Display Driver Integration .. kernel-doc:: drivers/gpu/drm/drm_bridge.c :doc: display driver integration
+Special Care with MIPI-DSI bridges +----------------------------------
+.. kernel-doc:: drivers/gpu/drm/drm_bridge.c
- :doc: special care dsi
Bridge Operations
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index baff74ea4a33..7cc2d2f94ae3 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -96,6 +96,63 @@
- documentation of bridge operations for more details).
*/
+/**
- DOC: special care dsi
- The interaction between the bridges and other frameworks involved in
- the probing of the upstream driver and the bridge driver can be
- challenging. Indeed, there's multiple cases that needs to be
- considered:
- The upstream driver doesn't use the component framework and isn't a
- MIPI-DSI host. In this case, the bridge driver will probe at some
- point and the upstream driver should try to probe again by returning
- EPROBE_DEFER as long as the bridge driver hasn't probed.
- The upstream driver doesn't use the component framework, but is a
- MIPI-DSI host. The bridge device uses the MIPI-DCS commands to be
- controlled. In this case, the bridge device is a child of the
- display device and when it will probe it's assured that the display
- device (and MIPI-DSI host) is present. The upstream driver will be
- assured that the bridge driver is connected between the
- &mipi_dsi_host_ops.attach and &mipi_dsi_host_ops.detach operations.
- Therefore, it must run mipi_dsi_host_register() in its probe
- function, and then run drm_bridge_attach() in its
- &mipi_dsi_host_ops.attach hook.
- The upstream driver uses the component framework and is a MIPI-DSI
- host. The bridge device uses the MIPI-DCS commands to be
- controlled. This is the same situation than above, and can run
- mipi_dsi_host_register() in either its probe or bind hooks.
- The upstream driver uses the component framework and is a MIPI-DSI
- host. The bridge device uses a separate bus (such as I2C) to be
- controlled. In this case, there's no correlation between the probe
- of the bridge and upstream drivers, so care must be taken to avoid
- an endless EPROBE_DEFER loop, with each driver waiting for the
- other to probe.
- The ideal pattern to cover the last item (and all the others in the
- MIPI-DSI host driver case) is to split the operations like this:
- The MIPI-DSI host driver must run mipi_dsi_host_register() in its
- probe hook. It will make sure that the MIPI-DSI host sticks around,
- and that the driver's bind can be called.
- In its probe hook, the bridge driver must try to find its MIPI-DSI
- host, register as a MIPI-DSI device and attach the MIPI-DSI device
- to its host. The bridge driver is now functional.
- In its &struct mipi_dsi_host_ops.attach hook, the MIPI-DSI host can
- now add its component. Its bind hook will now be called and since
- the bridge driver is attached and registered, we can now look for
- and attach it.
- At this point, we're now certain that both the upstream driver and
- the bridge driver are functional and we can't have a deadlock-like
- situation when probing.
- */
- static DEFINE_MUTEX(bridge_lock); static LIST_HEAD(bridge_list);
Nice work with documenting this initialization dance. It clearly shows that bridge API lacks better mechanism - usage of mipi dsi callbacks to get notifications about bridge appearance is ugly. It remains me my resource tracking patches which I have posted long time ago [1] - they would solve the issue in much more elegant way, described here [2]. Apparently I was not stubborn enough in promoting this solution.
Anyway:
Reviewed-by: Andrzej Hajda a.hajda@samsung.com
[1]: https://lkml.org/lkml/2014/12/10/342
[2]: https://events19.linuxfoundation.org/wp-content/uploads/2017/12/Deferred-Pro...
Regards Andrzej
Hi,
On Mon, Sep 13, 2021 at 08:29:37AM +0200, Andrzej Hajda wrote:
W dniu 10.09.2021 o 12:11, Maxime Ripard pisze:
Interactions between bridges, panels, MIPI-DSI host and the component framework are not trivial and can lead to probing issues when implementing a display driver. Let's document the various cases we need too consider, and the solution to support all the cases.
Signed-off-by: Maxime Ripard maxime@cerno.tech
Documentation/gpu/drm-kms-helpers.rst | 6 +++ drivers/gpu/drm/drm_bridge.c | 57 +++++++++++++++++++++++++++ 2 files changed, 63 insertions(+)
diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst index 10f8df7aecc0..ec2f65b31930 100644 --- a/Documentation/gpu/drm-kms-helpers.rst +++ b/Documentation/gpu/drm-kms-helpers.rst @@ -157,6 +157,12 @@ Display Driver Integration .. kernel-doc:: drivers/gpu/drm/drm_bridge.c :doc: display driver integration
+Special Care with MIPI-DSI bridges +----------------------------------
+.. kernel-doc:: drivers/gpu/drm/drm_bridge.c
- :doc: special care dsi
Bridge Operations
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index baff74ea4a33..7cc2d2f94ae3 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -96,6 +96,63 @@
- documentation of bridge operations for more details).
*/
+/**
- DOC: special care dsi
- The interaction between the bridges and other frameworks involved in
- the probing of the upstream driver and the bridge driver can be
- challenging. Indeed, there's multiple cases that needs to be
- considered:
- The upstream driver doesn't use the component framework and isn't a
- MIPI-DSI host. In this case, the bridge driver will probe at some
- point and the upstream driver should try to probe again by returning
- EPROBE_DEFER as long as the bridge driver hasn't probed.
- The upstream driver doesn't use the component framework, but is a
- MIPI-DSI host. The bridge device uses the MIPI-DCS commands to be
- controlled. In this case, the bridge device is a child of the
- display device and when it will probe it's assured that the display
- device (and MIPI-DSI host) is present. The upstream driver will be
- assured that the bridge driver is connected between the
- &mipi_dsi_host_ops.attach and &mipi_dsi_host_ops.detach operations.
- Therefore, it must run mipi_dsi_host_register() in its probe
- function, and then run drm_bridge_attach() in its
- &mipi_dsi_host_ops.attach hook.
- The upstream driver uses the component framework and is a MIPI-DSI
- host. The bridge device uses the MIPI-DCS commands to be
- controlled. This is the same situation than above, and can run
- mipi_dsi_host_register() in either its probe or bind hooks.
- The upstream driver uses the component framework and is a MIPI-DSI
- host. The bridge device uses a separate bus (such as I2C) to be
- controlled. In this case, there's no correlation between the probe
- of the bridge and upstream drivers, so care must be taken to avoid
- an endless EPROBE_DEFER loop, with each driver waiting for the
- other to probe.
- The ideal pattern to cover the last item (and all the others in the
- MIPI-DSI host driver case) is to split the operations like this:
- The MIPI-DSI host driver must run mipi_dsi_host_register() in its
- probe hook. It will make sure that the MIPI-DSI host sticks around,
- and that the driver's bind can be called.
- In its probe hook, the bridge driver must try to find its MIPI-DSI
- host, register as a MIPI-DSI device and attach the MIPI-DSI device
- to its host. The bridge driver is now functional.
- In its &struct mipi_dsi_host_ops.attach hook, the MIPI-DSI host can
- now add its component. Its bind hook will now be called and since
- the bridge driver is attached and registered, we can now look for
- and attach it.
- At this point, we're now certain that both the upstream driver and
- the bridge driver are functional and we can't have a deadlock-like
- situation when probing.
- */
- static DEFINE_MUTEX(bridge_lock); static LIST_HEAD(bridge_list);
Nice work with documenting this initialization dance. It clearly shows that bridge API lacks better mechanism - usage of mipi dsi callbacks to get notifications about bridge appearance is ugly.
Yeah, there's so many moving parts it's definitely not great.
It remains me my resource tracking patches which I have posted long time ago [1] - they would solve the issue in much more elegant way, described here [2]. Apparently I was not stubborn enough in promoting this solution.
Wow, that sounds like a massive change indeed :/
Anyway:
Reviewed-by: Andrzej Hajda a.hajda@samsung.com
I assume you'll want me to hold off that patch before someone reviews the rest?
Thanks! Maxime
W dniu 14.09.2021 o 16:35, Maxime Ripard pisze:
Hi,
On Mon, Sep 13, 2021 at 08:29:37AM +0200, Andrzej Hajda wrote:
W dniu 10.09.2021 o 12:11, Maxime Ripard pisze:
Interactions between bridges, panels, MIPI-DSI host and the component framework are not trivial and can lead to probing issues when implementing a display driver. Let's document the various cases we need too consider, and the solution to support all the cases.
Signed-off-by: Maxime Ripard maxime@cerno.tech
Documentation/gpu/drm-kms-helpers.rst | 6 +++ drivers/gpu/drm/drm_bridge.c | 57 +++++++++++++++++++++++++++ 2 files changed, 63 insertions(+)
diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst index 10f8df7aecc0..ec2f65b31930 100644 --- a/Documentation/gpu/drm-kms-helpers.rst +++ b/Documentation/gpu/drm-kms-helpers.rst @@ -157,6 +157,12 @@ Display Driver Integration .. kernel-doc:: drivers/gpu/drm/drm_bridge.c :doc: display driver integration
+Special Care with MIPI-DSI bridges +----------------------------------
+.. kernel-doc:: drivers/gpu/drm/drm_bridge.c
- :doc: special care dsi
Bridge Operations
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index baff74ea4a33..7cc2d2f94ae3 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -96,6 +96,63 @@ * documentation of bridge operations for more details). */
+/**
- DOC: special care dsi
- The interaction between the bridges and other frameworks involved in
- the probing of the upstream driver and the bridge driver can be
- challenging. Indeed, there's multiple cases that needs to be
- considered:
- The upstream driver doesn't use the component framework and isn't a
- MIPI-DSI host. In this case, the bridge driver will probe at some
- point and the upstream driver should try to probe again by returning
- EPROBE_DEFER as long as the bridge driver hasn't probed.
- The upstream driver doesn't use the component framework, but is a
- MIPI-DSI host. The bridge device uses the MIPI-DCS commands to be
- controlled. In this case, the bridge device is a child of the
- display device and when it will probe it's assured that the display
- device (and MIPI-DSI host) is present. The upstream driver will be
- assured that the bridge driver is connected between the
- &mipi_dsi_host_ops.attach and &mipi_dsi_host_ops.detach operations.
- Therefore, it must run mipi_dsi_host_register() in its probe
- function, and then run drm_bridge_attach() in its
- &mipi_dsi_host_ops.attach hook.
- The upstream driver uses the component framework and is a MIPI-DSI
- host. The bridge device uses the MIPI-DCS commands to be
- controlled. This is the same situation than above, and can run
- mipi_dsi_host_register() in either its probe or bind hooks.
- The upstream driver uses the component framework and is a MIPI-DSI
- host. The bridge device uses a separate bus (such as I2C) to be
- controlled. In this case, there's no correlation between the probe
- of the bridge and upstream drivers, so care must be taken to avoid
- an endless EPROBE_DEFER loop, with each driver waiting for the
- other to probe.
- The ideal pattern to cover the last item (and all the others in the
- MIPI-DSI host driver case) is to split the operations like this:
- The MIPI-DSI host driver must run mipi_dsi_host_register() in its
- probe hook. It will make sure that the MIPI-DSI host sticks around,
- and that the driver's bind can be called.
- In its probe hook, the bridge driver must try to find its MIPI-DSI
- host, register as a MIPI-DSI device and attach the MIPI-DSI device
- to its host. The bridge driver is now functional.
- In its &struct mipi_dsi_host_ops.attach hook, the MIPI-DSI host can
- now add its component. Its bind hook will now be called and since
- the bridge driver is attached and registered, we can now look for
- and attach it.
- At this point, we're now certain that both the upstream driver and
- the bridge driver are functional and we can't have a deadlock-like
- situation when probing.
- */
- static DEFINE_MUTEX(bridge_lock); static LIST_HEAD(bridge_list);
Nice work with documenting this initialization dance. It clearly shows that bridge API lacks better mechanism - usage of mipi dsi callbacks to get notifications about bridge appearance is ugly.
Yeah, there's so many moving parts it's definitely not great.
It remains me my resource tracking patches which I have posted long time ago [1] - they would solve the issue in much more elegant way, described here [2]. Apparently I was not stubborn enough in promoting this solution.
Wow, that sounds like a massive change indeed :/
Anyway:
Reviewed-by: Andrzej Hajda a.hajda@samsung.com
I assume you'll want me to hold off that patch before someone reviews the rest?
The last exynos patch should be dropped, kirin patch should be tested/reviewed/acked by kirin maintaner. I am not sure about bridge patches, which ones have been tested by you, and which one have other users.
If yes it would be good to test them as well - changes in initialization flow can beat sometimes :)
I think patches 1-4 can be merged earlier, if you like, as they are on the list for long time.
Regards
Andrzej
Thanks! Maxime
Hi,
On Tue, Sep 14, 2021 at 09:00:28PM +0200, Andrzej Hajda wrote:
W dniu 14.09.2021 o 16:35, Maxime Ripard pisze:
Hi,
On Mon, Sep 13, 2021 at 08:29:37AM +0200, Andrzej Hajda wrote:
W dniu 10.09.2021 o 12:11, Maxime Ripard pisze:
Interactions between bridges, panels, MIPI-DSI host and the component framework are not trivial and can lead to probing issues when implementing a display driver. Let's document the various cases we need too consider, and the solution to support all the cases.
Signed-off-by: Maxime Ripard maxime@cerno.tech
Documentation/gpu/drm-kms-helpers.rst | 6 +++ drivers/gpu/drm/drm_bridge.c | 57 +++++++++++++++++++++++++++ 2 files changed, 63 insertions(+)
diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst index 10f8df7aecc0..ec2f65b31930 100644 --- a/Documentation/gpu/drm-kms-helpers.rst +++ b/Documentation/gpu/drm-kms-helpers.rst @@ -157,6 +157,12 @@ Display Driver Integration .. kernel-doc:: drivers/gpu/drm/drm_bridge.c :doc: display driver integration
+Special Care with MIPI-DSI bridges +----------------------------------
+.. kernel-doc:: drivers/gpu/drm/drm_bridge.c
- :doc: special care dsi
Bridge Operations
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index baff74ea4a33..7cc2d2f94ae3 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -96,6 +96,63 @@ * documentation of bridge operations for more details). */
+/**
- DOC: special care dsi
- The interaction between the bridges and other frameworks involved in
- the probing of the upstream driver and the bridge driver can be
- challenging. Indeed, there's multiple cases that needs to be
- considered:
- The upstream driver doesn't use the component framework and isn't a
- MIPI-DSI host. In this case, the bridge driver will probe at some
- point and the upstream driver should try to probe again by returning
- EPROBE_DEFER as long as the bridge driver hasn't probed.
- The upstream driver doesn't use the component framework, but is a
- MIPI-DSI host. The bridge device uses the MIPI-DCS commands to be
- controlled. In this case, the bridge device is a child of the
- display device and when it will probe it's assured that the display
- device (and MIPI-DSI host) is present. The upstream driver will be
- assured that the bridge driver is connected between the
- &mipi_dsi_host_ops.attach and &mipi_dsi_host_ops.detach operations.
- Therefore, it must run mipi_dsi_host_register() in its probe
- function, and then run drm_bridge_attach() in its
- &mipi_dsi_host_ops.attach hook.
- The upstream driver uses the component framework and is a MIPI-DSI
- host. The bridge device uses the MIPI-DCS commands to be
- controlled. This is the same situation than above, and can run
- mipi_dsi_host_register() in either its probe or bind hooks.
- The upstream driver uses the component framework and is a MIPI-DSI
- host. The bridge device uses a separate bus (such as I2C) to be
- controlled. In this case, there's no correlation between the probe
- of the bridge and upstream drivers, so care must be taken to avoid
- an endless EPROBE_DEFER loop, with each driver waiting for the
- other to probe.
- The ideal pattern to cover the last item (and all the others in the
- MIPI-DSI host driver case) is to split the operations like this:
- The MIPI-DSI host driver must run mipi_dsi_host_register() in its
- probe hook. It will make sure that the MIPI-DSI host sticks around,
- and that the driver's bind can be called.
- In its probe hook, the bridge driver must try to find its MIPI-DSI
- host, register as a MIPI-DSI device and attach the MIPI-DSI device
- to its host. The bridge driver is now functional.
- In its &struct mipi_dsi_host_ops.attach hook, the MIPI-DSI host can
- now add its component. Its bind hook will now be called and since
- the bridge driver is attached and registered, we can now look for
- and attach it.
- At this point, we're now certain that both the upstream driver and
- the bridge driver are functional and we can't have a deadlock-like
- situation when probing.
- */
- static DEFINE_MUTEX(bridge_lock); static LIST_HEAD(bridge_list);
Nice work with documenting this initialization dance. It clearly shows that bridge API lacks better mechanism - usage of mipi dsi callbacks to get notifications about bridge appearance is ugly.
Yeah, there's so many moving parts it's definitely not great.
It remains me my resource tracking patches which I have posted long time ago [1] - they would solve the issue in much more elegant way, described here [2]. Apparently I was not stubborn enough in promoting this solution.
Wow, that sounds like a massive change indeed :/
Anyway:
Reviewed-by: Andrzej Hajda a.hajda@samsung.com
I assume you'll want me to hold off that patch before someone reviews the rest?
The last exynos patch should be dropped,
Done
kirin patch should be tested/reviewed/acked by kirin maintaner. I am not sure about bridge patches, which ones have been tested by you, and which one have other users.
Rob was nice enough to give it a try last week for msm and do the needed changes. He tested it with the sn65dsi86 bridge. John was also saying it was on their todo list (for kirin I assume?). So hopefully it can be fairly smooth for everyone.
I tested sn65dsi83 and ps8640 with the vc4 driver. I don't have the hardware so it was just making sure that everything was probing properly, but it's what we're interested in anyway.
If yes it would be good to test them as well - changes in initialization flow can beat sometimes :)
I think patches 1-4 can be merged earlier, if you like, as they are on the list for long time.
Ack, I'll merge them, thanks! Maxime
On Fri, 10 Sep 2021 12:11:56 +0200, Maxime Ripard wrote:
Interactions between bridges, panels, MIPI-DSI host and the component framework are not trivial and can lead to probing issues when implementing a display driver. Let's document the various cases we need too consider, and the solution to support all the cases.
Applied to drm/drm-misc (drm-misc-next).
Thanks! Maxime
Devices that take their data through the MIPI-DSI bus but are controlled through a secondary bus like I2C have to register a secondary device on the MIPI-DSI bus through the mipi_dsi_device_register_full() function.
At removal or when an error occurs, that device needs to be removed through a call to mipi_dsi_device_unregister().
Let's create a device-managed variant of the registration function that will automatically unregister the device at unbind.
Reviewed-by: Andrzej Hajda a.hajda@samsung.com Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/drm_mipi_dsi.c | 46 ++++++++++++++++++++++++++++++++++ include/drm/drm_mipi_dsi.h | 3 +++ 2 files changed, 49 insertions(+)
diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c index 5dd475e82995..ddf67463eaa1 100644 --- a/drivers/gpu/drm/drm_mipi_dsi.c +++ b/drivers/gpu/drm/drm_mipi_dsi.c @@ -246,6 +246,52 @@ void mipi_dsi_device_unregister(struct mipi_dsi_device *dsi) } EXPORT_SYMBOL(mipi_dsi_device_unregister);
+static void devm_mipi_dsi_device_unregister(void *arg) +{ + struct mipi_dsi_device *dsi = arg; + + mipi_dsi_device_unregister(dsi); +} + +/** + * devm_mipi_dsi_device_register_full - create a managed MIPI DSI device + * @dev: device to tie the MIPI-DSI device lifetime to + * @host: DSI host to which this device is connected + * @info: pointer to template containing DSI device information + * + * Create a MIPI DSI device by using the device information provided by + * mipi_dsi_device_info template + * + * This is the managed version of mipi_dsi_device_register_full() which + * automatically calls mipi_dsi_device_unregister() when @dev is + * unbound. + * + * Returns: + * A pointer to the newly created MIPI DSI device, or, a pointer encoded + * with an error + */ +struct mipi_dsi_device * +devm_mipi_dsi_device_register_full(struct device *dev, + struct mipi_dsi_host *host, + const struct mipi_dsi_device_info *info) +{ + struct mipi_dsi_device *dsi; + int ret; + + dsi = mipi_dsi_device_register_full(host, info); + if (IS_ERR(dsi)) + return dsi; + + ret = devm_add_action_or_reset(dev, + devm_mipi_dsi_device_unregister, + dsi); + if (ret) + return ERR_PTR(ret); + + return dsi; +} +EXPORT_SYMBOL_GPL(devm_mipi_dsi_device_register_full); + static DEFINE_MUTEX(host_lock); static LIST_HEAD(host_list);
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h index af7ba8071eb0..d0032e435e08 100644 --- a/include/drm/drm_mipi_dsi.h +++ b/include/drm/drm_mipi_dsi.h @@ -227,6 +227,9 @@ struct mipi_dsi_device * mipi_dsi_device_register_full(struct mipi_dsi_host *host, const struct mipi_dsi_device_info *info); void mipi_dsi_device_unregister(struct mipi_dsi_device *dsi); +struct mipi_dsi_device * +devm_mipi_dsi_device_register_full(struct device *dev, struct mipi_dsi_host *host, + const struct mipi_dsi_device_info *info); struct mipi_dsi_device *of_find_mipi_dsi_device_by_node(struct device_node *np); int mipi_dsi_attach(struct mipi_dsi_device *dsi); int mipi_dsi_detach(struct mipi_dsi_device *dsi);
On Fri, 10 Sep 2021 12:11:57 +0200, Maxime Ripard wrote:
Devices that take their data through the MIPI-DSI bus but are controlled through a secondary bus like I2C have to register a secondary device on the MIPI-DSI bus through the mipi_dsi_device_register_full() function.
At removal or when an error occurs, that device needs to be removed through a call to mipi_dsi_device_unregister().
[...]
Applied to drm/drm-misc (drm-misc-next).
Thanks! Maxime
MIPI-DSI devices need to call mipi_dsi_attach() when their probe is done to attach against their host.
However, at removal or when an error occurs, that attachment needs to be undone through a call to mipi_dsi_detach().
Let's create a device-managed variant of the attachment function that will automatically detach the device at unbind.
Reviewed-by: Andrzej Hajda a.hajda@samsung.com Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/drm_mipi_dsi.c | 35 ++++++++++++++++++++++++++++++++++ include/drm/drm_mipi_dsi.h | 1 + 2 files changed, 36 insertions(+)
diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c index ddf67463eaa1..18cef04df2f2 100644 --- a/drivers/gpu/drm/drm_mipi_dsi.c +++ b/drivers/gpu/drm/drm_mipi_dsi.c @@ -391,6 +391,41 @@ int mipi_dsi_detach(struct mipi_dsi_device *dsi) } EXPORT_SYMBOL(mipi_dsi_detach);
+static void devm_mipi_dsi_detach(void *arg) +{ + struct mipi_dsi_device *dsi = arg; + + mipi_dsi_detach(dsi); +} + +/** + * devm_mipi_dsi_attach - Attach a MIPI-DSI device to its DSI Host + * @dev: device to tie the MIPI-DSI device attachment lifetime to + * @dsi: DSI peripheral + * + * This is the managed version of mipi_dsi_attach() which automatically + * calls mipi_dsi_detach() when @dev is unbound. + * + * Returns: + * 0 on success, a negative error code on failure. + */ +int devm_mipi_dsi_attach(struct device *dev, + struct mipi_dsi_device *dsi) +{ + int ret; + + ret = mipi_dsi_attach(dsi); + if (ret) + return ret; + + ret = devm_add_action_or_reset(dev, devm_mipi_dsi_detach, dsi); + if (ret) + return ret; + + return 0; +} +EXPORT_SYMBOL_GPL(devm_mipi_dsi_attach); + static ssize_t mipi_dsi_device_transfer(struct mipi_dsi_device *dsi, struct mipi_dsi_msg *msg) { diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h index d0032e435e08..147e51b6d241 100644 --- a/include/drm/drm_mipi_dsi.h +++ b/include/drm/drm_mipi_dsi.h @@ -233,6 +233,7 @@ devm_mipi_dsi_device_register_full(struct device *dev, struct mipi_dsi_host *hos struct mipi_dsi_device *of_find_mipi_dsi_device_by_node(struct device_node *np); int mipi_dsi_attach(struct mipi_dsi_device *dsi); int mipi_dsi_detach(struct mipi_dsi_device *dsi); +int devm_mipi_dsi_attach(struct device *dev, struct mipi_dsi_device *dsi); int mipi_dsi_shutdown_peripheral(struct mipi_dsi_device *dsi); int mipi_dsi_turn_on_peripheral(struct mipi_dsi_device *dsi); int mipi_dsi_set_maximum_return_packet_size(struct mipi_dsi_device *dsi,
On Fri, 10 Sep 2021 12:11:58 +0200, Maxime Ripard wrote:
MIPI-DSI devices need to call mipi_dsi_attach() when their probe is done to attach against their host.
However, at removal or when an error occurs, that attachment needs to be undone through a call to mipi_dsi_detach().
Let's create a device-managed variant of the attachment function that will automatically detach the device at unbind.
[...]
Applied to drm/drm-misc (drm-misc-next).
Thanks! Maxime
Let's switch to the new devm MIPI-DSI function to register and attach our secondary device. This also avoids leaking the device when we detach the bridge.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/bridge/adv7511/adv7511.h | 1 - drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 2 -- drivers/gpu/drm/bridge/adv7511/adv7533.c | 20 ++++---------------- 3 files changed, 4 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h index 05e3abb5a0c9..592ecfcf00ca 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h @@ -401,7 +401,6 @@ void adv7533_mode_set(struct adv7511 *adv, const struct drm_display_mode *mode); int adv7533_patch_registers(struct adv7511 *adv); int adv7533_patch_cec_registers(struct adv7511 *adv); int adv7533_attach_dsi(struct adv7511 *adv); -void adv7533_detach_dsi(struct adv7511 *adv); int adv7533_parse_dt(struct device_node *np, struct adv7511 *adv);
#ifdef CONFIG_DRM_I2C_ADV7511_AUDIO diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 76555ae64e9c..9e3585f23cf1 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -1307,8 +1307,6 @@ static int adv7511_remove(struct i2c_client *i2c) { struct adv7511 *adv7511 = i2c_get_clientdata(i2c);
- if (adv7511->type == ADV7533 || adv7511->type == ADV7535) - adv7533_detach_dsi(adv7511); i2c_unregister_device(adv7511->i2c_cec); clk_disable_unprepare(adv7511->cec_clk);
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7533.c b/drivers/gpu/drm/bridge/adv7511/adv7533.c index 59d718bde8c4..eb7579dec40a 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7533.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7533.c @@ -153,11 +153,10 @@ int adv7533_attach_dsi(struct adv7511 *adv) return -EPROBE_DEFER; }
- dsi = mipi_dsi_device_register_full(host, &info); + dsi = devm_mipi_dsi_device_register_full(dev, host, &info); if (IS_ERR(dsi)) { dev_err(dev, "failed to create dsi device\n"); - ret = PTR_ERR(dsi); - goto err_dsi_device; + return PTR_ERR(dsi); }
adv->dsi = dsi; @@ -167,24 +166,13 @@ int adv7533_attach_dsi(struct adv7511 *adv) dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE | MIPI_DSI_MODE_NO_EOT_PACKET | MIPI_DSI_MODE_VIDEO_HSE;
- ret = mipi_dsi_attach(dsi); + ret = devm_mipi_dsi_attach(dev, dsi); if (ret < 0) { dev_err(dev, "failed to attach dsi to host\n"); - goto err_dsi_attach; + return ret; }
return 0; - -err_dsi_attach: - mipi_dsi_device_unregister(dsi); -err_dsi_device: - return ret; -} - -void adv7533_detach_dsi(struct adv7511 *adv) -{ - mipi_dsi_detach(adv->dsi); - mipi_dsi_device_unregister(adv->dsi); }
int adv7533_parse_dt(struct device_node *np, struct adv7511 *adv)
In order to avoid any probe ordering issue, the best practice is to move the secondary MIPI-DSI device registration and attachment to the MIPI-DSI host at probe time. Let's do this.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 9e3585f23cf1..f8e5da148599 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -910,9 +910,6 @@ static int adv7511_bridge_attach(struct drm_bridge *bridge, return ret; }
- if (adv->type == ADV7533 || adv->type == ADV7535) - ret = adv7533_attach_dsi(adv); - if (adv->i2c_main->irq) regmap_write(adv->regmap, ADV7511_REG_INT_ENABLE(0), ADV7511_INT0_HPD); @@ -1288,8 +1285,18 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) drm_bridge_add(&adv7511->bridge);
adv7511_audio_init(dev, adv7511); + + if (adv7511->type == ADV7533 || adv7511->type == ADV7535) { + ret = adv7533_attach_dsi(adv7511); + if (ret) + goto err_unregister_audio; + } + return 0;
+err_unregister_audio: + adv7511_audio_exit(adv7511); + drm_bridge_remove(&adv7511->bridge); err_unregister_cec: i2c_unregister_device(adv7511->i2c_cec); clk_disable_unprepare(adv7511->cec_clk);
Let's switch to the new devm MIPI-DSI function to register and attach our secondary device.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/bridge/analogix/anx7625.c | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c index 1a871f6b6822..4adeb2bad03a 100644 --- a/drivers/gpu/drm/bridge/analogix/anx7625.c +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c @@ -1316,6 +1316,7 @@ static int anx7625_attach_dsi(struct anx7625_data *ctx) .channel = 0, .node = NULL, }; + int ret;
DRM_DEV_DEBUG_DRIVER(dev, "attach dsi\n");
@@ -1325,7 +1326,7 @@ static int anx7625_attach_dsi(struct anx7625_data *ctx) return -EINVAL; }
- dsi = mipi_dsi_device_register_full(host, &info); + dsi = devm_mipi_dsi_device_register_full(dev, host, &info); if (IS_ERR(dsi)) { DRM_DEV_ERROR(dev, "fail to create dsi device.\n"); return -EINVAL; @@ -1337,10 +1338,10 @@ static int anx7625_attach_dsi(struct anx7625_data *ctx) MIPI_DSI_MODE_VIDEO_SYNC_PULSE | MIPI_DSI_MODE_VIDEO_HSE;
- if (mipi_dsi_attach(dsi) < 0) { + ret = devm_mipi_dsi_attach(dev, dsi); + if (ret) { DRM_DEV_ERROR(dev, "fail to attach dsi to host.\n"); - mipi_dsi_device_unregister(dsi); - return -EINVAL; + return ret; }
ctx->dsi = dsi; @@ -1350,16 +1351,6 @@ static int anx7625_attach_dsi(struct anx7625_data *ctx) return 0; }
-static void anx7625_bridge_detach(struct drm_bridge *bridge) -{ - struct anx7625_data *ctx = bridge_to_anx7625(bridge); - - if (ctx->dsi) { - mipi_dsi_detach(ctx->dsi); - mipi_dsi_device_unregister(ctx->dsi); - } -} - static int anx7625_bridge_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags) { @@ -1624,7 +1615,6 @@ static struct edid *anx7625_bridge_get_edid(struct drm_bridge *bridge,
static const struct drm_bridge_funcs anx7625_bridge_funcs = { .attach = anx7625_bridge_attach, - .detach = anx7625_bridge_detach, .disable = anx7625_bridge_disable, .mode_valid = anx7625_bridge_mode_valid, .mode_set = anx7625_bridge_mode_set,
In order to avoid any probe ordering issue, the best practice is to move the secondary MIPI-DSI device registration and attachment to the MIPI-DSI host at probe time. Let's do this.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/bridge/analogix/anx7625.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c index 4adeb2bad03a..d0317651cd75 100644 --- a/drivers/gpu/drm/bridge/analogix/anx7625.c +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c @@ -1367,12 +1367,6 @@ static int anx7625_bridge_attach(struct drm_bridge *bridge, return -ENODEV; }
- err = anx7625_attach_dsi(ctx); - if (err) { - DRM_DEV_ERROR(dev, "Fail to attach to dsi : %d\n", err); - return err; - } - if (ctx->pdata.panel_bridge) { err = drm_bridge_attach(bridge->encoder, ctx->pdata.panel_bridge, @@ -1845,10 +1839,24 @@ static int anx7625_i2c_probe(struct i2c_client *client, platform->bridge.type = DRM_MODE_CONNECTOR_eDP; drm_bridge_add(&platform->bridge);
+ ret = anx7625_attach_dsi(platform); + if (ret) { + DRM_DEV_ERROR(dev, "Fail to attach to dsi : %d\n", ret); + goto unregister_bridge; + } + DRM_DEV_DEBUG_DRIVER(dev, "probe done\n");
return 0;
+unregister_bridge: + drm_bridge_remove(&platform->bridge); + + if (!platform->pdata.low_power_mode) + pm_runtime_put_sync_suspend(&client->dev); + + anx7625_unregister_i2c_dummy_clients(platform); + free_wq: if (platform->workqueue) destroy_workqueue(platform->workqueue);
Let's switch to the new devm MIPI-DSI function to register and attach our secondary device.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/bridge/lontium-lt8912b.c | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/bridge/lontium-lt8912b.c b/drivers/gpu/drm/bridge/lontium-lt8912b.c index 1b0c7eaf6c84..cc968d65936b 100644 --- a/drivers/gpu/drm/bridge/lontium-lt8912b.c +++ b/drivers/gpu/drm/bridge/lontium-lt8912b.c @@ -472,11 +472,11 @@ static int lt8912_attach_dsi(struct lt8912 *lt) return -EPROBE_DEFER; }
- dsi = mipi_dsi_device_register_full(host, &info); + dsi = devm_mipi_dsi_device_register_full(dev, host, &info); if (IS_ERR(dsi)) { ret = PTR_ERR(dsi); dev_err(dev, "failed to create dsi device (%d)\n", ret); - goto err_dsi_device; + return ret; }
lt->dsi = dsi; @@ -489,24 +489,13 @@ static int lt8912_attach_dsi(struct lt8912 *lt) MIPI_DSI_MODE_LPM | MIPI_DSI_MODE_NO_EOT_PACKET;
- ret = mipi_dsi_attach(dsi); + ret = devm_mipi_dsi_attach(dev, dsi); if (ret < 0) { dev_err(dev, "failed to attach dsi to host\n"); - goto err_dsi_attach; + return ret; }
return 0; - -err_dsi_attach: - mipi_dsi_device_unregister(dsi); -err_dsi_device: - return ret; -} - -static void lt8912_detach_dsi(struct lt8912 *lt) -{ - mipi_dsi_detach(lt->dsi); - mipi_dsi_device_unregister(lt->dsi); }
static int lt8912_bridge_connector_init(struct drm_bridge *bridge) @@ -573,7 +562,6 @@ static void lt8912_bridge_detach(struct drm_bridge *bridge) struct lt8912 *lt = bridge_to_lt8912(bridge);
if (lt->is_attached) { - lt8912_detach_dsi(lt); lt8912_hard_power_off(lt); drm_connector_unregister(<->connector); drm_connector_cleanup(<->connector);
In order to avoid any probe ordering issue, the best practice is to move the secondary MIPI-DSI device registration and attachment to the MIPI-DSI host at probe time. Let's do this.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/bridge/lontium-lt8912b.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/bridge/lontium-lt8912b.c b/drivers/gpu/drm/bridge/lontium-lt8912b.c index cc968d65936b..c642d1e02b2f 100644 --- a/drivers/gpu/drm/bridge/lontium-lt8912b.c +++ b/drivers/gpu/drm/bridge/lontium-lt8912b.c @@ -544,10 +544,6 @@ static int lt8912_bridge_attach(struct drm_bridge *bridge, if (ret) goto error;
- ret = lt8912_attach_dsi(lt); - if (ret) - goto error; - lt->is_attached = true;
return 0; @@ -706,8 +702,15 @@ static int lt8912_probe(struct i2c_client *client,
drm_bridge_add(<->bridge);
+ ret = lt8912_attach_dsi(lt); + if (ret) + goto err_attach; + return 0;
+err_attach: + drm_bridge_remove(<->bridge); + lt8912_free_i2c(lt); err_i2c: lt8912_put_dt(lt); err_dt_parse:
Let's switch to the new devm MIPI-DSI function to register and attach our secondary device.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/bridge/lontium-lt9611.c | 24 ++++-------------------- 1 file changed, 4 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/bridge/lontium-lt9611.c b/drivers/gpu/drm/bridge/lontium-lt9611.c index 29b1ce2140ab..654131aca5ed 100644 --- a/drivers/gpu/drm/bridge/lontium-lt9611.c +++ b/drivers/gpu/drm/bridge/lontium-lt9611.c @@ -760,6 +760,7 @@ static struct mipi_dsi_device *lt9611_attach_dsi(struct lt9611 *lt9611, const struct mipi_dsi_device_info info = { "lt9611", 0, NULL }; struct mipi_dsi_device *dsi; struct mipi_dsi_host *host; + struct device *dev = lt9611->dev; int ret;
host = of_find_mipi_dsi_host_by_node(dsi_node); @@ -768,7 +769,7 @@ static struct mipi_dsi_device *lt9611_attach_dsi(struct lt9611 *lt9611, return ERR_PTR(-EPROBE_DEFER); }
- dsi = mipi_dsi_device_register_full(host, &info); + dsi = devm_mipi_dsi_device_register_full(dev, host, &info); if (IS_ERR(dsi)) { dev_err(lt9611->dev, "failed to create dsi device\n"); return dsi; @@ -779,29 +780,15 @@ static struct mipi_dsi_device *lt9611_attach_dsi(struct lt9611 *lt9611, dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE | MIPI_DSI_MODE_VIDEO_HSE;
- ret = mipi_dsi_attach(dsi); + ret = devm_mipi_dsi_attach(dev, dsi); if (ret < 0) { - dev_err(lt9611->dev, "failed to attach dsi to host\n"); - mipi_dsi_device_unregister(dsi); + dev_err(dev, "failed to attach dsi to host\n"); return ERR_PTR(ret); }
return dsi; }
-static void lt9611_bridge_detach(struct drm_bridge *bridge) -{ - struct lt9611 *lt9611 = bridge_to_lt9611(bridge); - - if (lt9611->dsi1) { - mipi_dsi_detach(lt9611->dsi1); - mipi_dsi_device_unregister(lt9611->dsi1); - } - - mipi_dsi_detach(lt9611->dsi0); - mipi_dsi_device_unregister(lt9611->dsi0); -} - static int lt9611_connector_init(struct drm_bridge *bridge, struct lt9611 *lt9611) { int ret; @@ -855,9 +842,7 @@ static int lt9611_bridge_attach(struct drm_bridge *bridge, return 0;
err_unregister_dsi0: - lt9611_bridge_detach(bridge); drm_connector_cleanup(<9611->connector); - mipi_dsi_device_unregister(lt9611->dsi0);
return ret; } @@ -952,7 +937,6 @@ static void lt9611_bridge_hpd_enable(struct drm_bridge *bridge)
static const struct drm_bridge_funcs lt9611_bridge_funcs = { .attach = lt9611_bridge_attach, - .detach = lt9611_bridge_detach, .mode_valid = lt9611_bridge_mode_valid, .enable = lt9611_bridge_enable, .disable = lt9611_bridge_disable,
In order to avoid any probe ordering issue, the best practice is to move the secondary MIPI-DSI device registration and attachment to the MIPI-DSI host at probe time. Let's do this.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/bridge/lontium-lt9611.c | 38 ++++++++++++------------- 1 file changed, 19 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/bridge/lontium-lt9611.c b/drivers/gpu/drm/bridge/lontium-lt9611.c index 654131aca5ed..d2f45a0f79c8 100644 --- a/drivers/gpu/drm/bridge/lontium-lt9611.c +++ b/drivers/gpu/drm/bridge/lontium-lt9611.c @@ -825,26 +825,7 @@ static int lt9611_bridge_attach(struct drm_bridge *bridge, return ret; }
- /* Attach primary DSI */ - lt9611->dsi0 = lt9611_attach_dsi(lt9611, lt9611->dsi0_node); - if (IS_ERR(lt9611->dsi0)) - return PTR_ERR(lt9611->dsi0); - - /* Attach secondary DSI, if specified */ - if (lt9611->dsi1_node) { - lt9611->dsi1 = lt9611_attach_dsi(lt9611, lt9611->dsi1_node); - if (IS_ERR(lt9611->dsi1)) { - ret = PTR_ERR(lt9611->dsi1); - goto err_unregister_dsi0; - } - } - return 0; - -err_unregister_dsi0: - drm_connector_cleanup(<9611->connector); - - return ret; }
static enum drm_mode_status lt9611_bridge_mode_valid(struct drm_bridge *bridge, @@ -1165,10 +1146,29 @@ static int lt9611_probe(struct i2c_client *client,
drm_bridge_add(<9611->bridge);
+ /* Attach primary DSI */ + lt9611->dsi0 = lt9611_attach_dsi(lt9611, lt9611->dsi0_node); + if (IS_ERR(lt9611->dsi0)) { + ret = PTR_ERR(lt9611->dsi0); + goto err_remove_bridge; + } + + /* Attach secondary DSI, if specified */ + if (lt9611->dsi1_node) { + lt9611->dsi1 = lt9611_attach_dsi(lt9611, lt9611->dsi1_node); + if (IS_ERR(lt9611->dsi1)) { + ret = PTR_ERR(lt9611->dsi1); + goto err_remove_bridge; + } + } + lt9611_enable_hpd_interrupts(lt9611);
return lt9611_audio_init(dev, lt9611);
+err_remove_bridge: + drm_bridge_remove(<9611->bridge); + err_disable_regulators: regulator_bulk_disable(ARRAY_SIZE(lt9611->supplies), lt9611->supplies);
Let's switch to the new devm MIPI-DSI function to register and attach our secondary device.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 38 +++++----------------- 1 file changed, 8 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c index 3cac16db970f..e5083bdf4c89 100644 --- a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c +++ b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c @@ -257,17 +257,18 @@ static struct mipi_dsi_device *lt9611uxc_attach_dsi(struct lt9611uxc *lt9611uxc, const struct mipi_dsi_device_info info = { "lt9611uxc", 0, NULL }; struct mipi_dsi_device *dsi; struct mipi_dsi_host *host; + struct device *dev = lt9611uxc->dev; int ret;
host = of_find_mipi_dsi_host_by_node(dsi_node); if (!host) { - dev_err(lt9611uxc->dev, "failed to find dsi host\n"); + dev_err(dev, "failed to find dsi host\n"); return ERR_PTR(-EPROBE_DEFER); }
- dsi = mipi_dsi_device_register_full(host, &info); + dsi = devm_mipi_dsi_device_register_full(dev, host, &info); if (IS_ERR(dsi)) { - dev_err(lt9611uxc->dev, "failed to create dsi device\n"); + dev_err(dev, "failed to create dsi device\n"); return dsi; }
@@ -276,10 +277,9 @@ static struct mipi_dsi_device *lt9611uxc_attach_dsi(struct lt9611uxc *lt9611uxc, dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE | MIPI_DSI_MODE_VIDEO_HSE;
- ret = mipi_dsi_attach(dsi); + ret = devm_mipi_dsi_attach(dev, dsi); if (ret < 0) { - dev_err(lt9611uxc->dev, "failed to attach dsi to host\n"); - mipi_dsi_device_unregister(dsi); + dev_err(dev, "failed to attach dsi to host\n"); return ERR_PTR(ret); }
@@ -352,19 +352,6 @@ static int lt9611uxc_connector_init(struct drm_bridge *bridge, struct lt9611uxc return drm_connector_attach_encoder(<9611uxc->connector, bridge->encoder); }
-static void lt9611uxc_bridge_detach(struct drm_bridge *bridge) -{ - struct lt9611uxc *lt9611uxc = bridge_to_lt9611uxc(bridge); - - if (lt9611uxc->dsi1) { - mipi_dsi_detach(lt9611uxc->dsi1); - mipi_dsi_device_unregister(lt9611uxc->dsi1); - } - - mipi_dsi_detach(lt9611uxc->dsi0); - mipi_dsi_device_unregister(lt9611uxc->dsi0); -} - static int lt9611uxc_bridge_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags) { @@ -385,19 +372,11 @@ static int lt9611uxc_bridge_attach(struct drm_bridge *bridge, /* Attach secondary DSI, if specified */ if (lt9611uxc->dsi1_node) { lt9611uxc->dsi1 = lt9611uxc_attach_dsi(lt9611uxc, lt9611uxc->dsi1_node); - if (IS_ERR(lt9611uxc->dsi1)) { - ret = PTR_ERR(lt9611uxc->dsi1); - goto err_unregister_dsi0; - } + if (IS_ERR(lt9611uxc->dsi1)) + return PTR_ERR(lt9611uxc->dsi1); }
return 0; - -err_unregister_dsi0: - mipi_dsi_detach(lt9611uxc->dsi0); - mipi_dsi_device_unregister(lt9611uxc->dsi0); - - return ret; }
static enum drm_mode_status @@ -541,7 +520,6 @@ static struct edid *lt9611uxc_bridge_get_edid(struct drm_bridge *bridge,
static const struct drm_bridge_funcs lt9611uxc_bridge_funcs = { .attach = lt9611uxc_bridge_attach, - .detach = lt9611uxc_bridge_detach, .mode_valid = lt9611uxc_bridge_mode_valid, .mode_set = lt9611uxc_bridge_mode_set, .detect = lt9611uxc_bridge_detect,
In order to avoid any probe ordering issue, the best practice is to move the secondary MIPI-DSI device registration and attachment to the MIPI-DSI host at probe time. Let's do this.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 31 +++++++++++++--------- 1 file changed, 19 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c index e5083bdf4c89..78c4175e0a12 100644 --- a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c +++ b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c @@ -364,18 +364,6 @@ static int lt9611uxc_bridge_attach(struct drm_bridge *bridge, return ret; }
- /* Attach primary DSI */ - lt9611uxc->dsi0 = lt9611uxc_attach_dsi(lt9611uxc, lt9611uxc->dsi0_node); - if (IS_ERR(lt9611uxc->dsi0)) - return PTR_ERR(lt9611uxc->dsi0); - - /* Attach secondary DSI, if specified */ - if (lt9611uxc->dsi1_node) { - lt9611uxc->dsi1 = lt9611uxc_attach_dsi(lt9611uxc, lt9611uxc->dsi1_node); - if (IS_ERR(lt9611uxc->dsi1)) - return PTR_ERR(lt9611uxc->dsi1); - } - return 0; }
@@ -955,8 +943,27 @@ static int lt9611uxc_probe(struct i2c_client *client,
drm_bridge_add(<9611uxc->bridge);
+ /* Attach primary DSI */ + lt9611uxc->dsi0 = lt9611uxc_attach_dsi(lt9611uxc, lt9611uxc->dsi0_node); + if (IS_ERR(lt9611uxc->dsi0)) { + ret = PTR_ERR(lt9611uxc->dsi0); + goto err_remove_bridge; + } + + /* Attach secondary DSI, if specified */ + if (lt9611uxc->dsi1_node) { + lt9611uxc->dsi1 = lt9611uxc_attach_dsi(lt9611uxc, lt9611uxc->dsi1_node); + if (IS_ERR(lt9611uxc->dsi1)) { + ret = PTR_ERR(lt9611uxc->dsi1); + goto err_remove_bridge; + } + } + return lt9611uxc_audio_init(dev, lt9611uxc);
+err_remove_bridge: + drm_bridge_remove(<9611uxc->bridge); + err_disable_regulators: regulator_bulk_disable(ARRAY_SIZE(lt9611uxc->supplies), lt9611uxc->supplies);
Let's switch to the new devm MIPI-DSI function to register and attach our secondary device. This also avoids leaking the device on removal.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/bridge/parade-ps8640.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c index 685e9c38b2db..c943045f3370 100644 --- a/drivers/gpu/drm/bridge/parade-ps8640.c +++ b/drivers/gpu/drm/bridge/parade-ps8640.c @@ -243,7 +243,7 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge, if (!host) return -ENODEV;
- dsi = mipi_dsi_device_register_full(host, &info); + dsi = devm_mipi_dsi_device_register_full(dev, host, &info); if (IS_ERR(dsi)) { dev_err(dev, "failed to create dsi device\n"); ret = PTR_ERR(dsi); @@ -257,17 +257,13 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge, MIPI_DSI_MODE_VIDEO_SYNC_PULSE; dsi->format = MIPI_DSI_FMT_RGB888; dsi->lanes = NUM_MIPI_LANES; - ret = mipi_dsi_attach(dsi); + ret = devm_mipi_dsi_attach(dev, dsi); if (ret) - goto err_dsi_attach; + return ret;
/* Attach the panel-bridge to the dsi bridge */ return drm_bridge_attach(bridge->encoder, ps_bridge->panel_bridge, &ps_bridge->bridge, flags); - -err_dsi_attach: - mipi_dsi_device_unregister(dsi); - return ret; }
static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
In order to avoid any probe ordering issue, the best practice is to move the secondary MIPI-DSI device registration and attachment to the MIPI-DSI host at probe time. Let's do this.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/bridge/parade-ps8640.c | 97 +++++++++++++++----------- 1 file changed, 55 insertions(+), 42 deletions(-)
diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c index c943045f3370..8d161b6cdbb2 100644 --- a/drivers/gpu/drm/bridge/parade-ps8640.c +++ b/drivers/gpu/drm/bridge/parade-ps8640.c @@ -215,52 +215,10 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags) { struct ps8640 *ps_bridge = bridge_to_ps8640(bridge); - struct device *dev = &ps_bridge->page[0]->dev; - struct device_node *in_ep, *dsi_node; - struct mipi_dsi_device *dsi; - struct mipi_dsi_host *host; - int ret; - const struct mipi_dsi_device_info info = { .type = "ps8640", - .channel = 0, - .node = NULL, - };
if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) return -EINVAL;
- /* port@0 is ps8640 dsi input port */ - in_ep = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1); - if (!in_ep) - return -ENODEV; - - dsi_node = of_graph_get_remote_port_parent(in_ep); - of_node_put(in_ep); - if (!dsi_node) - return -ENODEV; - - host = of_find_mipi_dsi_host_by_node(dsi_node); - of_node_put(dsi_node); - if (!host) - return -ENODEV; - - dsi = devm_mipi_dsi_device_register_full(dev, host, &info); - if (IS_ERR(dsi)) { - dev_err(dev, "failed to create dsi device\n"); - ret = PTR_ERR(dsi); - return ret; - } - - ps_bridge->dsi = dsi; - - dsi->host = host; - dsi->mode_flags = MIPI_DSI_MODE_VIDEO | - MIPI_DSI_MODE_VIDEO_SYNC_PULSE; - dsi->format = MIPI_DSI_FMT_RGB888; - dsi->lanes = NUM_MIPI_LANES; - ret = devm_mipi_dsi_attach(dev, dsi); - if (ret) - return ret; - /* Attach the panel-bridge to the dsi bridge */ return drm_bridge_attach(bridge->encoder, ps_bridge->panel_bridge, &ps_bridge->bridge, flags); @@ -307,6 +265,53 @@ static const struct drm_bridge_funcs ps8640_bridge_funcs = { .pre_enable = ps8640_pre_enable, };
+static int ps8640_bridge_host_attach(struct device *dev, struct ps8640 *ps_bridge) +{ + struct device_node *in_ep, *dsi_node; + struct mipi_dsi_device *dsi; + struct mipi_dsi_host *host; + int ret; + const struct mipi_dsi_device_info info = { .type = "ps8640", + .channel = 0, + .node = NULL, + }; + + /* port@0 is ps8640 dsi input port */ + in_ep = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1); + if (!in_ep) + return -ENODEV; + + dsi_node = of_graph_get_remote_port_parent(in_ep); + of_node_put(in_ep); + if (!dsi_node) + return -ENODEV; + + host = of_find_mipi_dsi_host_by_node(dsi_node); + of_node_put(dsi_node); + if (!host) + return -EPROBE_DEFER; + + dsi = devm_mipi_dsi_device_register_full(dev, host, &info); + if (IS_ERR(dsi)) { + dev_err(dev, "failed to create dsi device\n"); + return PTR_ERR(dsi); + } + + ps_bridge->dsi = dsi; + + dsi->host = host; + dsi->mode_flags = MIPI_DSI_MODE_VIDEO | + MIPI_DSI_MODE_VIDEO_SYNC_PULSE; + dsi->format = MIPI_DSI_FMT_RGB888; + dsi->lanes = NUM_MIPI_LANES; + + ret = devm_mipi_dsi_attach(dev, dsi); + if (ret) + return ret; + + return 0; +} + static int ps8640_probe(struct i2c_client *client) { struct device *dev = &client->dev; @@ -373,7 +378,15 @@ static int ps8640_probe(struct i2c_client *client)
drm_bridge_add(&ps_bridge->bridge);
+ ret = ps8640_bridge_host_attach(dev, ps_bridge); + if (ret) + goto err_bridge_remove; + return 0; + +err_bridge_remove: + drm_bridge_remove(&ps_bridge->bridge); + return ret; }
static int ps8640_remove(struct i2c_client *client)
Let's switch to the new devm MIPI-DSI function to register and attach our secondary device. This also avoids leaking the device when we detach the bridge but don't remove its driver.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/bridge/ti-sn65dsi83.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c index a32f70bc68ea..db4d39082705 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c @@ -262,7 +262,7 @@ static int sn65dsi83_attach(struct drm_bridge *bridge, return -EPROBE_DEFER; }
- dsi = mipi_dsi_device_register_full(host, &info); + dsi = devm_mipi_dsi_device_register_full(dev, host, &info); if (IS_ERR(dsi)) { return dev_err_probe(dev, PTR_ERR(dsi), "failed to create dsi device\n"); @@ -274,18 +274,14 @@ static int sn65dsi83_attach(struct drm_bridge *bridge, dsi->format = MIPI_DSI_FMT_RGB888; dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST;
- ret = mipi_dsi_attach(dsi); + ret = devm_mipi_dsi_attach(dev, dsi); if (ret < 0) { dev_err(dev, "failed to attach dsi to host\n"); - goto err_dsi_attach; + return ret; }
return drm_bridge_attach(bridge->encoder, ctx->panel_bridge, &ctx->bridge, flags); - -err_dsi_attach: - mipi_dsi_device_unregister(dsi); - return ret; }
static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge, @@ -697,8 +693,6 @@ static int sn65dsi83_remove(struct i2c_client *client) { struct sn65dsi83 *ctx = i2c_get_clientdata(client);
- mipi_dsi_detach(ctx->dsi); - mipi_dsi_device_unregister(ctx->dsi); drm_bridge_remove(&ctx->bridge); of_node_put(ctx->host_node);
In order to avoid any probe ordering issue, the best practice is to move the secondary MIPI-DSI device registration and attachment to the MIPI-DSI host at probe time. Let's do this.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/bridge/ti-sn65dsi83.c | 80 +++++++++++++++------------ 1 file changed, 46 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c index db4d39082705..f951eb19767b 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c @@ -245,40 +245,6 @@ static int sn65dsi83_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags) { struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge); - struct device *dev = ctx->dev; - struct mipi_dsi_device *dsi; - struct mipi_dsi_host *host; - int ret = 0; - - const struct mipi_dsi_device_info info = { - .type = "sn65dsi83", - .channel = 0, - .node = NULL, - }; - - host = of_find_mipi_dsi_host_by_node(ctx->host_node); - if (!host) { - dev_err(dev, "failed to find dsi host\n"); - return -EPROBE_DEFER; - } - - dsi = devm_mipi_dsi_device_register_full(dev, host, &info); - if (IS_ERR(dsi)) { - return dev_err_probe(dev, PTR_ERR(dsi), - "failed to create dsi device\n"); - } - - ctx->dsi = dsi; - - dsi->lanes = ctx->dsi_lanes; - dsi->format = MIPI_DSI_FMT_RGB888; - dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST; - - ret = devm_mipi_dsi_attach(dev, dsi); - if (ret < 0) { - dev_err(dev, "failed to attach dsi to host\n"); - return ret; - }
return drm_bridge_attach(bridge->encoder, ctx->panel_bridge, &ctx->bridge, flags); @@ -646,6 +612,44 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model) return 0; }
+static int sn65dsi83_host_attach(struct sn65dsi83 *ctx) +{ + struct device *dev = ctx->dev; + struct mipi_dsi_device *dsi; + struct mipi_dsi_host *host; + const struct mipi_dsi_device_info info = { + .type = "sn65dsi83", + .channel = 0, + .node = NULL, + }; + int ret; + + host = of_find_mipi_dsi_host_by_node(ctx->host_node); + if (!host) { + dev_err(dev, "failed to find dsi host\n"); + return -EPROBE_DEFER; + } + + dsi = devm_mipi_dsi_device_register_full(dev, host, &info); + if (IS_ERR(dsi)) + return dev_err_probe(dev, PTR_ERR(dsi), + "failed to create dsi device\n"); + + ctx->dsi = dsi; + + dsi->lanes = ctx->dsi_lanes; + dsi->format = MIPI_DSI_FMT_RGB888; + dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST; + + ret = devm_mipi_dsi_attach(dev, dsi); + if (ret < 0) { + dev_err(dev, "failed to attach dsi to host: %d\n", ret); + return ret; + } + + return 0; +} + static int sn65dsi83_probe(struct i2c_client *client, const struct i2c_device_id *id) { @@ -686,7 +690,15 @@ static int sn65dsi83_probe(struct i2c_client *client, ctx->bridge.of_node = dev->of_node; drm_bridge_add(&ctx->bridge);
+ ret = sn65dsi83_host_attach(ctx); + if (ret) + goto err_remove_bridge; + return 0; + +err_remove_bridge: + drm_bridge_remove(&ctx->bridge); + return ret; }
static int sn65dsi83_remove(struct i2c_client *client)
Let's switch to the new devm MIPI-DSI function to register and attach our secondary device. This also avoids leaking the device when we detach the bridge.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 41d48a393e7f..b5662269ff95 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -674,6 +674,7 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge, struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge); struct mipi_dsi_host *host; struct mipi_dsi_device *dsi; + struct device *dev = pdata->dev; const struct mipi_dsi_device_info info = { .type = "ti_sn_bridge", .channel = 0, .node = NULL, @@ -713,7 +714,7 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge, goto err_dsi_host; }
- dsi = mipi_dsi_device_register_full(host, &info); + dsi = devm_mipi_dsi_device_register_full(dev, host, &info); if (IS_ERR(dsi)) { DRM_ERROR("failed to create dsi device\n"); ret = PTR_ERR(dsi); @@ -726,16 +727,16 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge, dsi->mode_flags = MIPI_DSI_MODE_VIDEO;
/* check if continuous dsi clock is required or not */ - pm_runtime_get_sync(pdata->dev); + pm_runtime_get_sync(dev); regmap_read(pdata->regmap, SN_DPPLL_SRC_REG, &val); - pm_runtime_put_autosuspend(pdata->dev); + pm_runtime_put_autosuspend(dev); if (!(val & DPPLL_CLK_SRC_DSICLK)) dsi->mode_flags |= MIPI_DSI_CLOCK_NON_CONTINUOUS;
- ret = mipi_dsi_attach(dsi); + ret = devm_mipi_dsi_attach(dev, dsi); if (ret < 0) { DRM_ERROR("failed to attach dsi to host\n"); - goto err_dsi_attach; + goto err_dsi_host; } pdata->dsi = dsi;
@@ -746,14 +747,10 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge, ret = drm_bridge_attach(bridge->encoder, pdata->next_bridge, &pdata->bridge, flags); if (ret < 0) - goto err_dsi_detach; + goto err_dsi_host;
return 0;
-err_dsi_detach: - mipi_dsi_detach(dsi); -err_dsi_attach: - mipi_dsi_device_unregister(dsi); err_dsi_host: drm_connector_cleanup(&pdata->connector); err_conn_init: @@ -1236,11 +1233,6 @@ static void ti_sn_bridge_remove(struct auxiliary_device *adev) if (!pdata) return;
- if (pdata->dsi) { - mipi_dsi_detach(pdata->dsi); - mipi_dsi_device_unregister(pdata->dsi); - } - drm_bridge_remove(&pdata->bridge);
of_node_put(pdata->host_node);
In order to avoid any probe ordering issue, the best practice is to move the secondary MIPI-DSI device registration and attachment to the MIPI-DSI host at probe time. Let's do this.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 74 ++++++++++++++------------- 1 file changed, 38 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index b5662269ff95..7f71329536a2 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -667,58 +667,27 @@ static struct ti_sn65dsi86 *bridge_to_ti_sn65dsi86(struct drm_bridge *bridge) return container_of(bridge, struct ti_sn65dsi86, bridge); }
-static int ti_sn_bridge_attach(struct drm_bridge *bridge, - enum drm_bridge_attach_flags flags) +static int ti_sn_attach_host(struct ti_sn65dsi86 *pdata) { int ret, val; - struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge); struct mipi_dsi_host *host; struct mipi_dsi_device *dsi; struct device *dev = pdata->dev; const struct mipi_dsi_device_info info = { .type = "ti_sn_bridge", .channel = 0, .node = NULL, - }; + };
- if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) { - DRM_ERROR("Fix bridge driver to make connector optional!"); - return -EINVAL; - } - - pdata->aux.drm_dev = bridge->dev; - ret = drm_dp_aux_register(&pdata->aux); - if (ret < 0) { - drm_err(bridge->dev, "Failed to register DP AUX channel: %d\n", ret); - return ret; - } - - ret = ti_sn_bridge_connector_init(pdata); - if (ret < 0) - goto err_conn_init; - - /* - * TODO: ideally finding host resource and dsi dev registration needs - * to be done in bridge probe. But some existing DSI host drivers will - * wait for any of the drm_bridge/drm_panel to get added to the global - * bridge/panel list, before completing their probe. So if we do the - * dsi dev registration part in bridge probe, before populating in - * the global bridge list, then it will cause deadlock as dsi host probe - * will never complete, neither our bridge probe. So keeping it here - * will satisfy most of the existing host drivers. Once the host driver - * is fixed we can move the below code to bridge probe safely. - */ host = of_find_mipi_dsi_host_by_node(pdata->host_node); if (!host) { DRM_ERROR("failed to find dsi host\n"); - ret = -ENODEV; - goto err_dsi_host; + return -ENODEV; }
dsi = devm_mipi_dsi_device_register_full(dev, host, &info); if (IS_ERR(dsi)) { DRM_ERROR("failed to create dsi device\n"); - ret = PTR_ERR(dsi); - goto err_dsi_host; + return PTR_ERR(dsi); }
/* TODO: setting to 4 MIPI lanes always for now */ @@ -736,10 +705,35 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge, ret = devm_mipi_dsi_attach(dev, dsi); if (ret < 0) { DRM_ERROR("failed to attach dsi to host\n"); - goto err_dsi_host; + return ret; } pdata->dsi = dsi;
+ return 0; +} + +static int ti_sn_bridge_attach(struct drm_bridge *bridge, + enum drm_bridge_attach_flags flags) +{ + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge); + int ret; + + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) { + DRM_ERROR("Fix bridge driver to make connector optional!"); + return -EINVAL; + } + + pdata->aux.drm_dev = bridge->dev; + ret = drm_dp_aux_register(&pdata->aux); + if (ret < 0) { + drm_err(bridge->dev, "Failed to register DP AUX channel: %d\n", ret); + return ret; + } + + ret = ti_sn_bridge_connector_init(pdata); + if (ret < 0) + goto err_conn_init; + /* We never want the next bridge to *also* create a connector: */ flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR;
@@ -1223,7 +1217,15 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
drm_bridge_add(&pdata->bridge);
+ ret = ti_sn_attach_host(pdata); + if (ret) + goto err_remove_bridge; + return 0; + +err_remove_bridge: + drm_bridge_remove(&pdata->bridge); + return ret; }
static void ti_sn_bridge_remove(struct auxiliary_device *adev)
Let's switch to the new devm MIPI-DSI function to register and attach our secondary device. This also avoids leaking the device when we detach the bridge.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/bridge/tc358775.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c index 2272adcc5b4a..35e66d1b6456 100644 --- a/drivers/gpu/drm/bridge/tc358775.c +++ b/drivers/gpu/drm/bridge/tc358775.c @@ -610,11 +610,10 @@ static int tc_bridge_attach(struct drm_bridge *bridge, return -EPROBE_DEFER; }
- dsi = mipi_dsi_device_register_full(host, &info); + dsi = devm_mipi_dsi_device_register_full(dev, host, &info); if (IS_ERR(dsi)) { dev_err(dev, "failed to create dsi device\n"); - ret = PTR_ERR(dsi); - goto err_dsi_device; + return PTR_ERR(dsi); }
tc->dsi = dsi; @@ -623,19 +622,15 @@ static int tc_bridge_attach(struct drm_bridge *bridge, dsi->format = MIPI_DSI_FMT_RGB888; dsi->mode_flags = MIPI_DSI_MODE_VIDEO;
- ret = mipi_dsi_attach(dsi); + ret = devm_mipi_dsi_attach(dev, dsi); if (ret < 0) { dev_err(dev, "failed to attach dsi to host\n"); - goto err_dsi_attach; + return ret; }
/* Attach the panel-bridge to the dsi bridge */ return drm_bridge_attach(bridge->encoder, tc->panel_bridge, &tc->bridge, flags); -err_dsi_attach: - mipi_dsi_device_unregister(dsi); -err_dsi_device: - return ret; }
static const struct drm_bridge_funcs tc_bridge_funcs = {
In order to avoid any probe ordering issue, the best practice is to move the secondary MIPI-DSI device registration and attachment to the MIPI-DSI host at probe time. Let's do this.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/bridge/tc358775.c | 37 +++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c index 35e66d1b6456..2c76331b251d 100644 --- a/drivers/gpu/drm/bridge/tc358775.c +++ b/drivers/gpu/drm/bridge/tc358775.c @@ -594,11 +594,26 @@ static int tc_bridge_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags) { struct tc_data *tc = bridge_to_tc(bridge); + + /* Attach the panel-bridge to the dsi bridge */ + return drm_bridge_attach(bridge->encoder, tc->panel_bridge, + &tc->bridge, flags); +} + +static const struct drm_bridge_funcs tc_bridge_funcs = { + .attach = tc_bridge_attach, + .pre_enable = tc_bridge_pre_enable, + .enable = tc_bridge_enable, + .mode_valid = tc_mode_valid, + .post_disable = tc_bridge_post_disable, +}; + +static int tc_attach_host(struct tc_data *tc) +{ struct device *dev = &tc->i2c->dev; struct mipi_dsi_host *host; struct mipi_dsi_device *dsi; int ret; - const struct mipi_dsi_device_info info = { .type = "tc358775", .channel = 0, .node = NULL, @@ -628,19 +643,9 @@ static int tc_bridge_attach(struct drm_bridge *bridge, return ret; }
- /* Attach the panel-bridge to the dsi bridge */ - return drm_bridge_attach(bridge->encoder, tc->panel_bridge, - &tc->bridge, flags); + return 0; }
-static const struct drm_bridge_funcs tc_bridge_funcs = { - .attach = tc_bridge_attach, - .pre_enable = tc_bridge_pre_enable, - .enable = tc_bridge_enable, - .mode_valid = tc_mode_valid, - .post_disable = tc_bridge_post_disable, -}; - static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) { struct device *dev = &client->dev; @@ -704,7 +709,15 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
i2c_set_clientdata(client, tc);
+ ret = tc_attach_host(tc); + if (ret) + goto err_bridge_remove; + return 0; + +err_bridge_remove: + drm_bridge_remove(&tc->bridge); + return ret; }
static int tc_remove(struct i2c_client *client)
Without proper care and an agreement between how DSI hosts and devices drivers register their MIPI-DSI entities and potential components, we can end up in a situation where the drivers can never probe.
Most drivers were taking evasive maneuvers to try to workaround this, but not all of them were following the same conventions, resulting in various incompatibilities between DSI hosts and devices.
Now that we have a sequence agreed upon and documented, let's convert kirin to it.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c | 27 +++++++++++++++----- 1 file changed, 20 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c index 952cfdb1961d..be20c2ffe798 100644 --- a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c +++ b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c @@ -720,10 +720,13 @@ static int dw_drm_encoder_init(struct device *dev, return 0; }
+static const struct component_ops dsi_ops; static int dsi_host_attach(struct mipi_dsi_host *host, struct mipi_dsi_device *mdsi) { struct dw_dsi *dsi = host_to_dsi(host); + struct device *dev = host->dev; + int ret;
if (mdsi->lanes < 1 || mdsi->lanes > 4) { DRM_ERROR("dsi device params invalid\n"); @@ -734,13 +737,20 @@ static int dsi_host_attach(struct mipi_dsi_host *host, dsi->format = mdsi->format; dsi->mode_flags = mdsi->mode_flags;
+ ret = component_add(dev, &dsi_ops); + if (ret) + return ret; + return 0; }
static int dsi_host_detach(struct mipi_dsi_host *host, struct mipi_dsi_device *mdsi) { - /* do nothing */ + struct device *dev = host->dev; + + component_del(dev, &dsi_ops); + return 0; }
@@ -785,10 +795,6 @@ static int dsi_bind(struct device *dev, struct device *master, void *data) if (ret) return ret;
- ret = dsi_host_init(dev, dsi); - if (ret) - return ret; - ret = dsi_bridge_init(drm_dev, dsi); if (ret) return ret; @@ -859,12 +865,19 @@ static int dsi_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, data);
- return component_add(&pdev->dev, &dsi_ops); + ret = dsi_host_init(&pdev->dev, dsi); + if (ret) + return ret; + + return 0; }
static int dsi_remove(struct platform_device *pdev) { - component_del(&pdev->dev, &dsi_ops); + struct dsi_data *data = platform_get_drvdata(pdev); + struct dw_dsi *dsi = &data->dsi; + + mipi_dsi_host_unregister(&dsi->host);
return 0; }
Without proper care and an agreement between how DSI hosts and devices drivers register their MIPI-DSI entities and potential components, we can end up in a situation where the drivers can never probe.
Most drivers were taking evasive maneuvers to try to workaround this, but not all of them were following the same conventions, resulting in various incompatibilities between DSI hosts and devices.
Now that we have a sequence agreed upon and documented, let's convert exynos to it.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/exynos/exynos_drm_dsi.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index e39fac889edc..dfda2b259c44 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -1529,6 +1529,7 @@ static const struct drm_encoder_helper_funcs exynos_dsi_encoder_helper_funcs = {
MODULE_DEVICE_TABLE(of, exynos_dsi_of_match);
+static const struct component_ops exynos_dsi_component_ops; static int exynos_dsi_host_attach(struct mipi_dsi_host *host, struct mipi_dsi_device *device) { @@ -1536,6 +1537,7 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host, struct drm_encoder *encoder = &dsi->encoder; struct drm_device *drm = encoder->dev; struct drm_bridge *out_bridge; + struct device *dev = host->dev;
out_bridge = of_drm_find_bridge(device->dev.of_node); if (out_bridge) { @@ -1585,7 +1587,7 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host, if (drm->mode_config.poll_enabled) drm_kms_helper_hotplug_event(drm);
- return 0; + return component_add(dev, &exynos_dsi_component_ops); }
static int exynos_dsi_host_detach(struct mipi_dsi_host *host, @@ -1593,6 +1595,9 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host, { struct exynos_dsi *dsi = host_to_dsi(host); struct drm_device *drm = dsi->encoder.dev; + struct device *dev = host->dev; + + component_del(dev, &exynos_dsi_component_ops);
if (dsi->panel) { mutex_lock(&drm->mode_config.mutex); @@ -1716,7 +1721,7 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, of_node_put(in_bridge_node); }
- return mipi_dsi_host_register(&dsi->dsi_host); + return 0; }
static void exynos_dsi_unbind(struct device *dev, struct device *master, @@ -1726,8 +1731,6 @@ static void exynos_dsi_unbind(struct device *dev, struct device *master, struct drm_encoder *encoder = &dsi->encoder;
exynos_dsi_disable(encoder); - - mipi_dsi_host_unregister(&dsi->dsi_host); }
static const struct component_ops exynos_dsi_component_ops = { @@ -1821,7 +1824,7 @@ static int exynos_dsi_probe(struct platform_device *pdev)
pm_runtime_enable(dev);
- ret = component_add(dev, &exynos_dsi_component_ops); + ret = mipi_dsi_host_register(&dsi->dsi_host); if (ret) goto err_disable_runtime;
@@ -1835,10 +1838,12 @@ static int exynos_dsi_probe(struct platform_device *pdev)
static int exynos_dsi_remove(struct platform_device *pdev) { + struct exynos_dsi *dsi = platform_get_drvdata(pdev); + + mipi_dsi_host_unregister(&dsi->dsi_host); + pm_runtime_disable(&pdev->dev);
- component_del(&pdev->dev, &exynos_dsi_component_ops); - return 0; }
W dniu 10.09.2021 o 12:12, Maxime Ripard pisze:
Without proper care and an agreement between how DSI hosts and devices drivers register their MIPI-DSI entities and potential components, we can end up in a situation where the drivers can never probe.
Most drivers were taking evasive maneuvers to try to workaround this, but not all of them were following the same conventions, resulting in various incompatibilities between DSI hosts and devices.
Now that we have a sequence agreed upon and documented, let's convert exynos to it.
Signed-off-by: Maxime Ripard maxime@cerno.tech
This patch should be dropped, as it will probably break the driver.
Exynos is already compatible with the pattern register-bus-then-get-sink, but it adds/removes panel/bridge dynamically, so it creates drm_device without waiting for downstream sink.
Regards
Andrzej
drivers/gpu/drm/exynos/exynos_drm_dsi.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index e39fac889edc..dfda2b259c44 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -1529,6 +1529,7 @@ static const struct drm_encoder_helper_funcs exynos_dsi_encoder_helper_funcs = {
MODULE_DEVICE_TABLE(of, exynos_dsi_of_match);
+static const struct component_ops exynos_dsi_component_ops; static int exynos_dsi_host_attach(struct mipi_dsi_host *host, struct mipi_dsi_device *device) { @@ -1536,6 +1537,7 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host, struct drm_encoder *encoder = &dsi->encoder; struct drm_device *drm = encoder->dev; struct drm_bridge *out_bridge;
struct device *dev = host->dev;
out_bridge = of_drm_find_bridge(device->dev.of_node); if (out_bridge) {
@@ -1585,7 +1587,7 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host, if (drm->mode_config.poll_enabled) drm_kms_helper_hotplug_event(drm);
- return 0;
return component_add(dev, &exynos_dsi_component_ops); }
static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
@@ -1593,6 +1595,9 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host, { struct exynos_dsi *dsi = host_to_dsi(host); struct drm_device *drm = dsi->encoder.dev;
struct device *dev = host->dev;
component_del(dev, &exynos_dsi_component_ops);
if (dsi->panel) { mutex_lock(&drm->mode_config.mutex);
@@ -1716,7 +1721,7 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, of_node_put(in_bridge_node); }
- return mipi_dsi_host_register(&dsi->dsi_host);
return 0; }
static void exynos_dsi_unbind(struct device *dev, struct device *master,
@@ -1726,8 +1731,6 @@ static void exynos_dsi_unbind(struct device *dev, struct device *master, struct drm_encoder *encoder = &dsi->encoder;
exynos_dsi_disable(encoder);
mipi_dsi_host_unregister(&dsi->dsi_host); }
static const struct component_ops exynos_dsi_component_ops = {
@@ -1821,7 +1824,7 @@ static int exynos_dsi_probe(struct platform_device *pdev)
pm_runtime_enable(dev);
- ret = component_add(dev, &exynos_dsi_component_ops);
- ret = mipi_dsi_host_register(&dsi->dsi_host); if (ret) goto err_disable_runtime;
@@ -1835,10 +1838,12 @@ static int exynos_dsi_probe(struct platform_device *pdev)
static int exynos_dsi_remove(struct platform_device *pdev) {
- struct exynos_dsi *dsi = platform_get_drvdata(pdev);
- mipi_dsi_host_unregister(&dsi->dsi_host);
- pm_runtime_disable(&pdev->dev);
- component_del(&pdev->dev, &exynos_dsi_component_ops);
- return 0; }
Hi,
On 13.09.2021 12:30, Andrzej Hajda wrote:
W dniu 10.09.2021 o 12:12, Maxime Ripard pisze:
Without proper care and an agreement between how DSI hosts and devices drivers register their MIPI-DSI entities and potential components, we can end up in a situation where the drivers can never probe.
Most drivers were taking evasive maneuvers to try to workaround this, but not all of them were following the same conventions, resulting in various incompatibilities between DSI hosts and devices.
Now that we have a sequence agreed upon and documented, let's convert exynos to it.
Signed-off-by: Maxime Ripard maxime@cerno.tech
This patch should be dropped, as it will probably break the driver.
Exynos is already compatible with the pattern register-bus-then-get-sink, but it adds/removes panel/bridge dynamically, so it creates drm_device without waiting for downstream sink.
Right, this patch breaks Exynos DSI driver operation. Without it, the whole series works fine on all Exynos based test boards.
Best regards
Hi Marek,
On Fri, Sep 17, 2021 at 02:35:05PM +0200, Marek Szyprowski wrote:
Hi,
On 13.09.2021 12:30, Andrzej Hajda wrote:
W dniu 10.09.2021 o 12:12, Maxime Ripard pisze:
Without proper care and an agreement between how DSI hosts and devices drivers register their MIPI-DSI entities and potential components, we can end up in a situation where the drivers can never probe.
Most drivers were taking evasive maneuvers to try to workaround this, but not all of them were following the same conventions, resulting in various incompatibilities between DSI hosts and devices.
Now that we have a sequence agreed upon and documented, let's convert exynos to it.
Signed-off-by: Maxime Ripard maxime@cerno.tech
This patch should be dropped, as it will probably break the driver.
Exynos is already compatible with the pattern register-bus-then-get-sink, but it adds/removes panel/bridge dynamically, so it creates drm_device without waiting for downstream sink.
Right, this patch breaks Exynos DSI driver operation. Without it, the whole series works fine on all Exynos based test boards.
Thanks for testing. Did you have any board using one of those bridges in your test sample?
Thanks! Maxime
Hi Maxime,
On 22.09.2021 10:53, Maxime Ripard wrote:
On Fri, Sep 17, 2021 at 02:35:05PM +0200, Marek Szyprowski wrote:
On 13.09.2021 12:30, Andrzej Hajda wrote:
W dniu 10.09.2021 o 12:12, Maxime Ripard pisze:
Without proper care and an agreement between how DSI hosts and devices drivers register their MIPI-DSI entities and potential components, we can end up in a situation where the drivers can never probe.
Most drivers were taking evasive maneuvers to try to workaround this, but not all of them were following the same conventions, resulting in various incompatibilities between DSI hosts and devices.
Now that we have a sequence agreed upon and documented, let's convert exynos to it.
Signed-off-by: Maxime Ripard maxime@cerno.tech
This patch should be dropped, as it will probably break the driver.
Exynos is already compatible with the pattern register-bus-then-get-sink, but it adds/removes panel/bridge dynamically, so it creates drm_device without waiting for downstream sink.
Right, this patch breaks Exynos DSI driver operation. Without it, the whole series works fine on all Exynos based test boards.
Thanks for testing. Did you have any board using one of those bridges in your test sample?
Nope, the only bridges I've tested are tc358764 and exynos-mic. However, both are used in a bit special way. The rest of my test boards just have a dsi panel.
Best regards
On Fri, Sep 10, 2021 at 3:12 AM Maxime Ripard maxime@cerno.tech wrote:
We've encountered an issue with the RaspberryPi DSI panel that prevented the whole display driver from probing.
The issue is described in detail in the commit 7213246a803f ("drm/vc4: dsi: Only register our component once a DSI device is attached"), but the basic idea is that since the panel is probed through i2c, there's no synchronization between its probe and the registration of the MIPI-DSI host it's attached to.
We initially moved the component framework registration to the MIPI-DSI Host attach hook to make sure we register our component only when we have a DSI device attached to our MIPI-DSI host, and then use lookup our DSI device in our bind hook.
However, all the DSI bridges controlled through i2c are only registering their associated DSI device in their bridge attach hook, meaning with our change above, we never got that far, and therefore ended up in the same situation than the one we were trying to fix for panels.
The best practice to avoid those issues is to register its functions only after all its dependencies are live. We also shouldn't wait any longer than we should to play nice with the other components that are waiting for us, so in our case that would mean moving the DSI device registration to the bridge probe.
I also had a look at all the DSI hosts, and it seems that exynos, kirin and msm would be affected by this and wouldn't probe anymore after those changes. Exynos and kirin seems to be simple enough for a mechanical change (that still requires to be tested), but the changes in msm seemed to be far more important and I wasn't confortable doing them.
Hey Maxime, Sorry for taking so long to get to this, but now that plumbers is over I've had a chance to check it out on kirin
Rob Clark pointed me to his branch with some fixups here: https://gitlab.freedesktop.org/robclark/msm/-/commits/for-mripard/bridge-rew...
But trying to boot hikey with that, I see the following loop indefinitely: [ 4.632132] adv7511 2-0039: supply avdd not found, using dummy regulator [ 4.638961] adv7511 2-0039: supply dvdd not found, using dummy regulator [ 4.645741] adv7511 2-0039: supply pvdd not found, using dummy regulator [ 4.652483] adv7511 2-0039: supply a2vdd not found, using dummy regulator [ 4.659342] adv7511 2-0039: supply v3p3 not found, using dummy regulator [ 4.666086] adv7511 2-0039: supply v1p2 not found, using dummy regulator [ 4.681898] adv7511 2-0039: failed to find dsi host [ 4.688836] adv7511 2-0039: supply avdd not found, using dummy regulator [ 4.695724] adv7511 2-0039: supply dvdd not found, using dummy regulator [ 4.702583] adv7511 2-0039: supply pvdd not found, using dummy regulator [ 4.709369] adv7511 2-0039: supply a2vdd not found, using dummy regulator [ 4.716232] adv7511 2-0039: supply v3p3 not found, using dummy regulator [ 4.722972] adv7511 2-0039: supply v1p2 not found, using dummy regulator [ 4.738720] adv7511 2-0039: failed to find dsi host
I'll have to dig a bit to figure out what's going wrong, but wanted to give you the heads up that there seems to be a problem
thanks -john
On Wed, Sep 29, 2021 at 2:27 PM John Stultz john.stultz@linaro.org wrote:
On Fri, Sep 10, 2021 at 3:12 AM Maxime Ripard maxime@cerno.tech wrote:
We've encountered an issue with the RaspberryPi DSI panel that prevented the whole display driver from probing.
The issue is described in detail in the commit 7213246a803f ("drm/vc4: dsi: Only register our component once a DSI device is attached"), but the basic idea is that since the panel is probed through i2c, there's no synchronization between its probe and the registration of the MIPI-DSI host it's attached to.
We initially moved the component framework registration to the MIPI-DSI Host attach hook to make sure we register our component only when we have a DSI device attached to our MIPI-DSI host, and then use lookup our DSI device in our bind hook.
However, all the DSI bridges controlled through i2c are only registering their associated DSI device in their bridge attach hook, meaning with our change above, we never got that far, and therefore ended up in the same situation than the one we were trying to fix for panels.
The best practice to avoid those issues is to register its functions only after all its dependencies are live. We also shouldn't wait any longer than we should to play nice with the other components that are waiting for us, so in our case that would mean moving the DSI device registration to the bridge probe.
I also had a look at all the DSI hosts, and it seems that exynos, kirin and msm would be affected by this and wouldn't probe anymore after those changes. Exynos and kirin seems to be simple enough for a mechanical change (that still requires to be tested), but the changes in msm seemed to be far more important and I wasn't confortable doing them.
Hey Maxime, Sorry for taking so long to get to this, but now that plumbers is over I've had a chance to check it out on kirin
Rob Clark pointed me to his branch with some fixups here: https://gitlab.freedesktop.org/robclark/msm/-/commits/for-mripard/bridge-rew...
But trying to boot hikey with that, I see the following loop indefinitely: [ 4.632132] adv7511 2-0039: supply avdd not found, using dummy regulator [ 4.638961] adv7511 2-0039: supply dvdd not found, using dummy regulator [ 4.645741] adv7511 2-0039: supply pvdd not found, using dummy regulator [ 4.652483] adv7511 2-0039: supply a2vdd not found, using dummy regulator [ 4.659342] adv7511 2-0039: supply v3p3 not found, using dummy regulator [ 4.666086] adv7511 2-0039: supply v1p2 not found, using dummy regulator [ 4.681898] adv7511 2-0039: failed to find dsi host
I just realized Rob's tree is missing the kirin patch. My apologies! I'll retest and let you know.
thanks -john
On Wed, Sep 29, 2021 at 2:32 PM John Stultz john.stultz@linaro.org wrote:
On Wed, Sep 29, 2021 at 2:27 PM John Stultz john.stultz@linaro.org wrote:
On Fri, Sep 10, 2021 at 3:12 AM Maxime Ripard maxime@cerno.tech wrote:
The best practice to avoid those issues is to register its functions only after all its dependencies are live. We also shouldn't wait any longer than we should to play nice with the other components that are waiting for us, so in our case that would mean moving the DSI device registration to the bridge probe.
I also had a look at all the DSI hosts, and it seems that exynos, kirin and msm would be affected by this and wouldn't probe anymore after those changes. Exynos and kirin seems to be simple enough for a mechanical change (that still requires to be tested), but the changes in msm seemed to be far more important and I wasn't confortable doing them.
Hey Maxime, Sorry for taking so long to get to this, but now that plumbers is over I've had a chance to check it out on kirin
Rob Clark pointed me to his branch with some fixups here: https://gitlab.freedesktop.org/robclark/msm/-/commits/for-mripard/bridge-rew...
But trying to boot hikey with that, I see the following loop indefinitely: [ 4.632132] adv7511 2-0039: supply avdd not found, using dummy regulator [ 4.638961] adv7511 2-0039: supply dvdd not found, using dummy regulator [ 4.645741] adv7511 2-0039: supply pvdd not found, using dummy regulator [ 4.652483] adv7511 2-0039: supply a2vdd not found, using dummy regulator [ 4.659342] adv7511 2-0039: supply v3p3 not found, using dummy regulator [ 4.666086] adv7511 2-0039: supply v1p2 not found, using dummy regulator [ 4.681898] adv7511 2-0039: failed to find dsi host
I just realized Rob's tree is missing the kirin patch. My apologies! I'll retest and let you know.
Ok, just retested including the kirin patch and unfortunately I'm still seeing the same thing. :(
Will dig a bit and let you know when I find more.
thanks -john
On Wed, Sep 29, 2021 at 2:51 PM John Stultz john.stultz@linaro.org wrote:
On Wed, Sep 29, 2021 at 2:32 PM John Stultz john.stultz@linaro.org wrote:
On Wed, Sep 29, 2021 at 2:27 PM John Stultz john.stultz@linaro.org wrote:
On Fri, Sep 10, 2021 at 3:12 AM Maxime Ripard maxime@cerno.tech wrote:
The best practice to avoid those issues is to register its functions only after all its dependencies are live. We also shouldn't wait any longer than we should to play nice with the other components that are waiting for us, so in our case that would mean moving the DSI device registration to the bridge probe.
I also had a look at all the DSI hosts, and it seems that exynos, kirin and msm would be affected by this and wouldn't probe anymore after those changes. Exynos and kirin seems to be simple enough for a mechanical change (that still requires to be tested), but the changes in msm seemed to be far more important and I wasn't confortable doing them.
Hey Maxime, Sorry for taking so long to get to this, but now that plumbers is over I've had a chance to check it out on kirin
Rob Clark pointed me to his branch with some fixups here: https://gitlab.freedesktop.org/robclark/msm/-/commits/for-mripard/bridge-rew...
But trying to boot hikey with that, I see the following loop indefinitely: [ 4.632132] adv7511 2-0039: supply avdd not found, using dummy regulator [ 4.638961] adv7511 2-0039: supply dvdd not found, using dummy regulator [ 4.645741] adv7511 2-0039: supply pvdd not found, using dummy regulator [ 4.652483] adv7511 2-0039: supply a2vdd not found, using dummy regulator [ 4.659342] adv7511 2-0039: supply v3p3 not found, using dummy regulator [ 4.666086] adv7511 2-0039: supply v1p2 not found, using dummy regulator [ 4.681898] adv7511 2-0039: failed to find dsi host
I just realized Rob's tree is missing the kirin patch. My apologies! I'll retest and let you know.
Ok, just retested including the kirin patch and unfortunately I'm still seeing the same thing. :(
Will dig a bit and let you know when I find more.
Did you have a chance to test it on anything using drm/msm with DSI panels? That would at least confirm that I didn't miss anything in the drm/msm patch to swap the dsi-host vs bridge ordering..
BR, -R
On Wed, Sep 29, 2021 at 4:20 PM Rob Clark robdclark@gmail.com wrote:
On Wed, Sep 29, 2021 at 2:51 PM John Stultz john.stultz@linaro.org wrote:
On Wed, Sep 29, 2021 at 2:32 PM John Stultz john.stultz@linaro.org wrote:
On Wed, Sep 29, 2021 at 2:27 PM John Stultz john.stultz@linaro.org wrote:
On Fri, Sep 10, 2021 at 3:12 AM Maxime Ripard maxime@cerno.tech wrote:
The best practice to avoid those issues is to register its functions only after all its dependencies are live. We also shouldn't wait any longer than we should to play nice with the other components that are waiting for us, so in our case that would mean moving the DSI device registration to the bridge probe.
I also had a look at all the DSI hosts, and it seems that exynos, kirin and msm would be affected by this and wouldn't probe anymore after those changes. Exynos and kirin seems to be simple enough for a mechanical change (that still requires to be tested), but the changes in msm seemed to be far more important and I wasn't confortable doing them.
Hey Maxime, Sorry for taking so long to get to this, but now that plumbers is over I've had a chance to check it out on kirin
Rob Clark pointed me to his branch with some fixups here: https://gitlab.freedesktop.org/robclark/msm/-/commits/for-mripard/bridge-rew...
But trying to boot hikey with that, I see the following loop indefinitely: [ 4.632132] adv7511 2-0039: supply avdd not found, using dummy regulator [ 4.638961] adv7511 2-0039: supply dvdd not found, using dummy regulator [ 4.645741] adv7511 2-0039: supply pvdd not found, using dummy regulator [ 4.652483] adv7511 2-0039: supply a2vdd not found, using dummy regulator [ 4.659342] adv7511 2-0039: supply v3p3 not found, using dummy regulator [ 4.666086] adv7511 2-0039: supply v1p2 not found, using dummy regulator [ 4.681898] adv7511 2-0039: failed to find dsi host
I just realized Rob's tree is missing the kirin patch. My apologies! I'll retest and let you know.
Ok, just retested including the kirin patch and unfortunately I'm still seeing the same thing. :(
Will dig a bit and let you know when I find more.
Did you have a chance to test it on anything using drm/msm with DSI panels? That would at least confirm that I didn't miss anything in the drm/msm patch to swap the dsi-host vs bridge ordering..
I believe Amit(cc'ed) tried to give it a run on his pocof1, but had some troubles getting it working against a kernel that wasn't suffering other regressions at the moment.
Amit/Caleb: Any chance one of you could try again w/ these merged to 5.15-rc3?
thanks -john
On Wed, Sep 29, 2021 at 4:20 PM Rob Clark robdclark@gmail.com wrote:
On Wed, Sep 29, 2021 at 2:51 PM John Stultz john.stultz@linaro.org wrote:
On Wed, Sep 29, 2021 at 2:32 PM John Stultz john.stultz@linaro.org wrote:
On Wed, Sep 29, 2021 at 2:27 PM John Stultz john.stultz@linaro.org wrote:
On Fri, Sep 10, 2021 at 3:12 AM Maxime Ripard maxime@cerno.tech wrote:
The best practice to avoid those issues is to register its functions only after all its dependencies are live. We also shouldn't wait any longer than we should to play nice with the other components that are waiting for us, so in our case that would mean moving the DSI device registration to the bridge probe.
I also had a look at all the DSI hosts, and it seems that exynos, kirin and msm would be affected by this and wouldn't probe anymore after those changes. Exynos and kirin seems to be simple enough for a mechanical change (that still requires to be tested), but the changes in msm seemed to be far more important and I wasn't confortable doing them.
Hey Maxime, Sorry for taking so long to get to this, but now that plumbers is over I've had a chance to check it out on kirin
Rob Clark pointed me to his branch with some fixups here: https://gitlab.freedesktop.org/robclark/msm/-/commits/for-mripard/bridge-rew...
But trying to boot hikey with that, I see the following loop indefinitely: [ 4.632132] adv7511 2-0039: supply avdd not found, using dummy regulator [ 4.638961] adv7511 2-0039: supply dvdd not found, using dummy regulator [ 4.645741] adv7511 2-0039: supply pvdd not found, using dummy regulator [ 4.652483] adv7511 2-0039: supply a2vdd not found, using dummy regulator [ 4.659342] adv7511 2-0039: supply v3p3 not found, using dummy regulator [ 4.666086] adv7511 2-0039: supply v1p2 not found, using dummy regulator [ 4.681898] adv7511 2-0039: failed to find dsi host
I just realized Rob's tree is missing the kirin patch. My apologies! I'll retest and let you know.
Ok, just retested including the kirin patch and unfortunately I'm still seeing the same thing. :(
Will dig a bit and let you know when I find more.
Did you have a chance to test it on anything using drm/msm with DSI panels? That would at least confirm that I didn't miss anything in the drm/msm patch to swap the dsi-host vs bridge ordering..
Not a dsi panel, but for what it's worth, I did just test the series with the db845c w/ HDMI using the lt9611 bridge. So at least that is looking ok.
thanks -john
On Thu, 30 Sept 2021 at 04:50, Rob Clark robdclark@gmail.com wrote:
On Wed, Sep 29, 2021 at 2:51 PM John Stultz john.stultz@linaro.org wrote:
On Wed, Sep 29, 2021 at 2:32 PM John Stultz john.stultz@linaro.org wrote:
On Wed, Sep 29, 2021 at 2:27 PM John Stultz john.stultz@linaro.org wrote:
On Fri, Sep 10, 2021 at 3:12 AM Maxime Ripard maxime@cerno.tech wrote:
The best practice to avoid those issues is to register its functions only after all its dependencies are live. We also shouldn't wait any longer than we should to play nice with the other components that are waiting for us, so in our case that would mean moving the DSI device registration to the bridge probe.
I also had a look at all the DSI hosts, and it seems that exynos, kirin and msm would be affected by this and wouldn't probe anymore after those changes. Exynos and kirin seems to be simple enough for a mechanical change (that still requires to be tested), but the changes in msm seemed to be far more important and I wasn't confortable doing them.
Hey Maxime, Sorry for taking so long to get to this, but now that plumbers is over I've had a chance to check it out on kirin
Rob Clark pointed me to his branch with some fixups here: https://gitlab.freedesktop.org/robclark/msm/-/commits/for-mripard/bridge-rew...
But trying to boot hikey with that, I see the following loop indefinitely: [ 4.632132] adv7511 2-0039: supply avdd not found, using dummy regulator [ 4.638961] adv7511 2-0039: supply dvdd not found, using dummy regulator [ 4.645741] adv7511 2-0039: supply pvdd not found, using dummy regulator [ 4.652483] adv7511 2-0039: supply a2vdd not found, using dummy regulator [ 4.659342] adv7511 2-0039: supply v3p3 not found, using dummy regulator [ 4.666086] adv7511 2-0039: supply v1p2 not found, using dummy regulator [ 4.681898] adv7511 2-0039: failed to find dsi host
I just realized Rob's tree is missing the kirin patch. My apologies! I'll retest and let you know.
Ok, just retested including the kirin patch and unfortunately I'm still seeing the same thing. :(
Will dig a bit and let you know when I find more.
Did you have a chance to test it on anything using drm/msm with DSI panels? That would at least confirm that I didn't miss anything in the drm/msm patch to swap the dsi-host vs bridge ordering..
Hi, smoke tested https://gitlab.freedesktop.org/robclark/msm/-/commits/for-mripard/bridge-rew... on Pocophone F1 (sdm845 / A630) with v5.15-rc3. I see no obvious regressions in my limited testing so far including video (youtube) playback.
BR, -R
Hi,
On 30/09/2021 20:49, Amit Pundir wrote:
On Thu, 30 Sept 2021 at 04:50, Rob Clark robdclark@gmail.com wrote:
On Wed, Sep 29, 2021 at 2:51 PM John Stultz john.stultz@linaro.org wrote:
On Wed, Sep 29, 2021 at 2:32 PM John Stultz john.stultz@linaro.org wrote:
On Wed, Sep 29, 2021 at 2:27 PM John Stultz john.stultz@linaro.org wrote:
On Fri, Sep 10, 2021 at 3:12 AM Maxime Ripard maxime@cerno.tech wrote:
The best practice to avoid those issues is to register its functions only after all its dependencies are live. We also shouldn't wait any longer than we should to play nice with the other components that are waiting for us, so in our case that would mean moving the DSI device registration to the bridge probe.
I also had a look at all the DSI hosts, and it seems that exynos, kirin and msm would be affected by this and wouldn't probe anymore after those changes. Exynos and kirin seems to be simple enough for a mechanical change (that still requires to be tested), but the changes in msm seemed to be far more important and I wasn't confortable doing them.
Hey Maxime, Sorry for taking so long to get to this, but now that plumbers is over I've had a chance to check it out on kirin
Rob Clark pointed me to his branch with some fixups here: https://gitlab.freedesktop.org/robclark/msm/-/commits/for-mripard/bridge-rew...
But trying to boot hikey with that, I see the following loop indefinitely: [ 4.632132] adv7511 2-0039: supply avdd not found, using dummy regulator [ 4.638961] adv7511 2-0039: supply dvdd not found, using dummy regulator [ 4.645741] adv7511 2-0039: supply pvdd not found, using dummy regulator [ 4.652483] adv7511 2-0039: supply a2vdd not found, using dummy regulator [ 4.659342] adv7511 2-0039: supply v3p3 not found, using dummy regulator [ 4.666086] adv7511 2-0039: supply v1p2 not found, using dummy regulator [ 4.681898] adv7511 2-0039: failed to find dsi host
I just realized Rob's tree is missing the kirin patch. My apologies! I'll retest and let you know.
Ok, just retested including the kirin patch and unfortunately I'm still seeing the same thing. :(
Will dig a bit and let you know when I find more.
Did you have a chance to test it on anything using drm/msm with DSI panels? That would at least confirm that I didn't miss anything in the drm/msm patch to swap the dsi-host vs bridge ordering..
Hi, smoke tested https://gitlab.freedesktop.org/robclark/msm/-/commits/for-mripard/bridge-rew... on Pocophone F1 (sdm845 / A630) with v5.15-rc3. I see no obvious regressions in my limited testing so far including video (youtube) playback.
Tested on the OnePlus 6 too booting AOSP, works fine. This *fixes* FBDEV_EMULATION (so we can get a working framebuffer console) which was otherwise broken on 5.15.
However it spits out some warnings during boot: https://p.calebs.dev/gucysowyna.yaml
BR, -R
Hi Caleb,
On Thu, Sep 30, 2021 at 09:20:52PM +0100, Caleb Connolly wrote:
Hi,
On 30/09/2021 20:49, Amit Pundir wrote:
On Thu, 30 Sept 2021 at 04:50, Rob Clark robdclark@gmail.com wrote:
On Wed, Sep 29, 2021 at 2:51 PM John Stultz john.stultz@linaro.org wrote:
On Wed, Sep 29, 2021 at 2:32 PM John Stultz john.stultz@linaro.org wrote:
On Wed, Sep 29, 2021 at 2:27 PM John Stultz john.stultz@linaro.org wrote:
On Fri, Sep 10, 2021 at 3:12 AM Maxime Ripard maxime@cerno.tech wrote: > The best practice to avoid those issues is to register its functions only after > all its dependencies are live. We also shouldn't wait any longer than we should > to play nice with the other components that are waiting for us, so in our case > that would mean moving the DSI device registration to the bridge probe. > > I also had a look at all the DSI hosts, and it seems that exynos, kirin and msm > would be affected by this and wouldn't probe anymore after those changes. > Exynos and kirin seems to be simple enough for a mechanical change (that still > requires to be tested), but the changes in msm seemed to be far more important > and I wasn't confortable doing them.
Hey Maxime, Sorry for taking so long to get to this, but now that plumbers is over I've had a chance to check it out on kirin
Rob Clark pointed me to his branch with some fixups here: https://gitlab.freedesktop.org/robclark/msm/-/commits/for-mripard/bridge-rew...
But trying to boot hikey with that, I see the following loop indefinitely: [ 4.632132] adv7511 2-0039: supply avdd not found, using dummy regulator [ 4.638961] adv7511 2-0039: supply dvdd not found, using dummy regulator [ 4.645741] adv7511 2-0039: supply pvdd not found, using dummy regulator [ 4.652483] adv7511 2-0039: supply a2vdd not found, using dummy regulator [ 4.659342] adv7511 2-0039: supply v3p3 not found, using dummy regulator [ 4.666086] adv7511 2-0039: supply v1p2 not found, using dummy regulator [ 4.681898] adv7511 2-0039: failed to find dsi host
I just realized Rob's tree is missing the kirin patch. My apologies! I'll retest and let you know.
Ok, just retested including the kirin patch and unfortunately I'm still seeing the same thing. :(
Will dig a bit and let you know when I find more.
Did you have a chance to test it on anything using drm/msm with DSI panels? That would at least confirm that I didn't miss anything in the drm/msm patch to swap the dsi-host vs bridge ordering..
Hi, smoke tested https://gitlab.freedesktop.org/robclark/msm/-/commits/for-mripard/bridge-rew... on Pocophone F1 (sdm845 / A630) with v5.15-rc3. I see no obvious regressions in my limited testing so far including video (youtube) playback.
Tested on the OnePlus 6 too booting AOSP, works fine. This *fixes* FBDEV_EMULATION (so we can get a working framebuffer console) which was otherwise broken on 5.15.
However it spits out some warnings during boot: https://p.calebs.dev/gucysowyna.yaml
Thanks for testing. It looks like the runtime_pm ordering between the msm devices changed a bit with the conversion Rob did.
Rob, do you know what could be going on?
Thanks! Maxime
On Wed, Oct 13, 2021 at 7:16 AM Maxime Ripard maxime@cerno.tech wrote:
Hi Caleb,
On Thu, Sep 30, 2021 at 09:20:52PM +0100, Caleb Connolly wrote:
Hi,
On 30/09/2021 20:49, Amit Pundir wrote:
On Thu, 30 Sept 2021 at 04:50, Rob Clark robdclark@gmail.com wrote:
On Wed, Sep 29, 2021 at 2:51 PM John Stultz john.stultz@linaro.org wrote:
On Wed, Sep 29, 2021 at 2:32 PM John Stultz john.stultz@linaro.org wrote:
On Wed, Sep 29, 2021 at 2:27 PM John Stultz john.stultz@linaro.org wrote: > On Fri, Sep 10, 2021 at 3:12 AM Maxime Ripard maxime@cerno.tech wrote: > > The best practice to avoid those issues is to register its functions only after > > all its dependencies are live. We also shouldn't wait any longer than we should > > to play nice with the other components that are waiting for us, so in our case > > that would mean moving the DSI device registration to the bridge probe. > > > > I also had a look at all the DSI hosts, and it seems that exynos, kirin and msm > > would be affected by this and wouldn't probe anymore after those changes. > > Exynos and kirin seems to be simple enough for a mechanical change (that still > > requires to be tested), but the changes in msm seemed to be far more important > > and I wasn't confortable doing them. > > > Hey Maxime, > Sorry for taking so long to get to this, but now that plumbers is > over I've had a chance to check it out on kirin > > Rob Clark pointed me to his branch with some fixups here: > https://gitlab.freedesktop.org/robclark/msm/-/commits/for-mripard/bridge-rew... > > But trying to boot hikey with that, I see the following loop indefinitely: > [ 4.632132] adv7511 2-0039: supply avdd not found, using dummy regulator > [ 4.638961] adv7511 2-0039: supply dvdd not found, using dummy regulator > [ 4.645741] adv7511 2-0039: supply pvdd not found, using dummy regulator > [ 4.652483] adv7511 2-0039: supply a2vdd not found, using dummy regulator > [ 4.659342] adv7511 2-0039: supply v3p3 not found, using dummy regulator > [ 4.666086] adv7511 2-0039: supply v1p2 not found, using dummy regulator > [ 4.681898] adv7511 2-0039: failed to find dsi host
I just realized Rob's tree is missing the kirin patch. My apologies! I'll retest and let you know.
Ok, just retested including the kirin patch and unfortunately I'm still seeing the same thing. :(
Will dig a bit and let you know when I find more.
Did you have a chance to test it on anything using drm/msm with DSI panels? That would at least confirm that I didn't miss anything in the drm/msm patch to swap the dsi-host vs bridge ordering..
Hi, smoke tested https://gitlab.freedesktop.org/robclark/msm/-/commits/for-mripard/bridge-rew... on Pocophone F1 (sdm845 / A630) with v5.15-rc3. I see no obvious regressions in my limited testing so far including video (youtube) playback.
Tested on the OnePlus 6 too booting AOSP, works fine. This *fixes* FBDEV_EMULATION (so we can get a working framebuffer console) which was otherwise broken on 5.15.
However it spits out some warnings during boot: https://p.calebs.dev/gucysowyna.yaml
Thanks for testing. It looks like the runtime_pm ordering between the msm devices changed a bit with the conversion Rob did.
Rob, do you know what could be going on?
Not entirely sure.. I didn't see that first splat, but maybe I was missing some debug config? (The 2nd one is kind of "normal", I think related to bootloader leaving the display on)
BR, -R
Hi Rob,
On Wed, Oct 13, 2021 at 05:16:58PM -0700, Rob Clark wrote:
On Wed, Oct 13, 2021 at 7:16 AM Maxime Ripard maxime@cerno.tech wrote:
Hi Caleb,
On Thu, Sep 30, 2021 at 09:20:52PM +0100, Caleb Connolly wrote:
Hi,
On 30/09/2021 20:49, Amit Pundir wrote:
On Thu, 30 Sept 2021 at 04:50, Rob Clark robdclark@gmail.com wrote:
On Wed, Sep 29, 2021 at 2:51 PM John Stultz john.stultz@linaro.org wrote:
On Wed, Sep 29, 2021 at 2:32 PM John Stultz john.stultz@linaro.org wrote: > On Wed, Sep 29, 2021 at 2:27 PM John Stultz john.stultz@linaro.org wrote: > > On Fri, Sep 10, 2021 at 3:12 AM Maxime Ripard maxime@cerno.tech wrote: > > > The best practice to avoid those issues is to register its functions only after > > > all its dependencies are live. We also shouldn't wait any longer than we should > > > to play nice with the other components that are waiting for us, so in our case > > > that would mean moving the DSI device registration to the bridge probe. > > > > > > I also had a look at all the DSI hosts, and it seems that exynos, kirin and msm > > > would be affected by this and wouldn't probe anymore after those changes. > > > Exynos and kirin seems to be simple enough for a mechanical change (that still > > > requires to be tested), but the changes in msm seemed to be far more important > > > and I wasn't confortable doing them. > > > > > > Hey Maxime, > > Sorry for taking so long to get to this, but now that plumbers is > > over I've had a chance to check it out on kirin > > > > Rob Clark pointed me to his branch with some fixups here: > > https://gitlab.freedesktop.org/robclark/msm/-/commits/for-mripard/bridge-rew... > > > > But trying to boot hikey with that, I see the following loop indefinitely: > > [ 4.632132] adv7511 2-0039: supply avdd not found, using dummy regulator > > [ 4.638961] adv7511 2-0039: supply dvdd not found, using dummy regulator > > [ 4.645741] adv7511 2-0039: supply pvdd not found, using dummy regulator > > [ 4.652483] adv7511 2-0039: supply a2vdd not found, using dummy regulator > > [ 4.659342] adv7511 2-0039: supply v3p3 not found, using dummy regulator > > [ 4.666086] adv7511 2-0039: supply v1p2 not found, using dummy regulator > > [ 4.681898] adv7511 2-0039: failed to find dsi host > > I just realized Rob's tree is missing the kirin patch. My apologies! > I'll retest and let you know.
Ok, just retested including the kirin patch and unfortunately I'm still seeing the same thing. :(
Will dig a bit and let you know when I find more.
Did you have a chance to test it on anything using drm/msm with DSI panels? That would at least confirm that I didn't miss anything in the drm/msm patch to swap the dsi-host vs bridge ordering..
Hi, smoke tested https://gitlab.freedesktop.org/robclark/msm/-/commits/for-mripard/bridge-rew... on Pocophone F1 (sdm845 / A630) with v5.15-rc3. I see no obvious regressions in my limited testing so far including video (youtube) playback.
Tested on the OnePlus 6 too booting AOSP, works fine. This *fixes* FBDEV_EMULATION (so we can get a working framebuffer console) which was otherwise broken on 5.15.
However it spits out some warnings during boot: https://p.calebs.dev/gucysowyna.yaml
Thanks for testing. It looks like the runtime_pm ordering between the msm devices changed a bit with the conversion Rob did.
Rob, do you know what could be going on?
Not entirely sure.. I didn't see that first splat, but maybe I was missing some debug config? (The 2nd one is kind of "normal", I think related to bootloader leaving the display on)
So do you feel like this is a blocker or do you expect it to be fixed sometime down the road?
Maxime
On Mon, Oct 18, 2021 at 5:34 AM Maxime Ripard maxime@cerno.tech wrote:
Hi Rob,
On Wed, Oct 13, 2021 at 05:16:58PM -0700, Rob Clark wrote:
On Wed, Oct 13, 2021 at 7:16 AM Maxime Ripard maxime@cerno.tech wrote:
Hi Caleb,
On Thu, Sep 30, 2021 at 09:20:52PM +0100, Caleb Connolly wrote:
Hi,
On 30/09/2021 20:49, Amit Pundir wrote:
On Thu, 30 Sept 2021 at 04:50, Rob Clark robdclark@gmail.com wrote:
On Wed, Sep 29, 2021 at 2:51 PM John Stultz john.stultz@linaro.org wrote: > > On Wed, Sep 29, 2021 at 2:32 PM John Stultz john.stultz@linaro.org wrote: > > On Wed, Sep 29, 2021 at 2:27 PM John Stultz john.stultz@linaro.org wrote: > > > On Fri, Sep 10, 2021 at 3:12 AM Maxime Ripard maxime@cerno.tech wrote: > > > > The best practice to avoid those issues is to register its functions only after > > > > all its dependencies are live. We also shouldn't wait any longer than we should > > > > to play nice with the other components that are waiting for us, so in our case > > > > that would mean moving the DSI device registration to the bridge probe. > > > > > > > > I also had a look at all the DSI hosts, and it seems that exynos, kirin and msm > > > > would be affected by this and wouldn't probe anymore after those changes. > > > > Exynos and kirin seems to be simple enough for a mechanical change (that still > > > > requires to be tested), but the changes in msm seemed to be far more important > > > > and I wasn't confortable doing them. > > > > > > > > > Hey Maxime, > > > Sorry for taking so long to get to this, but now that plumbers is > > > over I've had a chance to check it out on kirin > > > > > > Rob Clark pointed me to his branch with some fixups here: > > > https://gitlab.freedesktop.org/robclark/msm/-/commits/for-mripard/bridge-rew... > > > > > > But trying to boot hikey with that, I see the following loop indefinitely: > > > [ 4.632132] adv7511 2-0039: supply avdd not found, using dummy regulator > > > [ 4.638961] adv7511 2-0039: supply dvdd not found, using dummy regulator > > > [ 4.645741] adv7511 2-0039: supply pvdd not found, using dummy regulator > > > [ 4.652483] adv7511 2-0039: supply a2vdd not found, using dummy regulator > > > [ 4.659342] adv7511 2-0039: supply v3p3 not found, using dummy regulator > > > [ 4.666086] adv7511 2-0039: supply v1p2 not found, using dummy regulator > > > [ 4.681898] adv7511 2-0039: failed to find dsi host > > > > I just realized Rob's tree is missing the kirin patch. My apologies! > > I'll retest and let you know. > > Ok, just retested including the kirin patch and unfortunately I'm > still seeing the same thing. :( > > Will dig a bit and let you know when I find more.
Did you have a chance to test it on anything using drm/msm with DSI panels? That would at least confirm that I didn't miss anything in the drm/msm patch to swap the dsi-host vs bridge ordering..
Hi, smoke tested https://gitlab.freedesktop.org/robclark/msm/-/commits/for-mripard/bridge-rew... on Pocophone F1 (sdm845 / A630) with v5.15-rc3. I see no obvious regressions in my limited testing so far including video (youtube) playback.
Tested on the OnePlus 6 too booting AOSP, works fine. This *fixes* FBDEV_EMULATION (so we can get a working framebuffer console) which was otherwise broken on 5.15.
However it spits out some warnings during boot: https://p.calebs.dev/gucysowyna.yaml
Thanks for testing. It looks like the runtime_pm ordering between the msm devices changed a bit with the conversion Rob did.
Rob, do you know what could be going on?
Not entirely sure.. I didn't see that first splat, but maybe I was missing some debug config? (The 2nd one is kind of "normal", I think related to bootloader leaving the display on)
So do you feel like this is a blocker or do you expect it to be fixed sometime down the road?
No
I can try and take a look at the 2nd splat, but shouldn't block your series
BR, -R
On Wed, Sep 29, 2021 at 2:51 PM John Stultz john.stultz@linaro.org wrote:
On Wed, Sep 29, 2021 at 2:32 PM John Stultz john.stultz@linaro.org wrote:
On Wed, Sep 29, 2021 at 2:27 PM John Stultz john.stultz@linaro.org wrote:
On Fri, Sep 10, 2021 at 3:12 AM Maxime Ripard maxime@cerno.tech wrote:
The best practice to avoid those issues is to register its functions only after all its dependencies are live. We also shouldn't wait any longer than we should to play nice with the other components that are waiting for us, so in our case that would mean moving the DSI device registration to the bridge probe.
I also had a look at all the DSI hosts, and it seems that exynos, kirin and msm would be affected by this and wouldn't probe anymore after those changes. Exynos and kirin seems to be simple enough for a mechanical change (that still requires to be tested), but the changes in msm seemed to be far more important and I wasn't confortable doing them.
Hey Maxime, Sorry for taking so long to get to this, but now that plumbers is over I've had a chance to check it out on kirin
Rob Clark pointed me to his branch with some fixups here: https://gitlab.freedesktop.org/robclark/msm/-/commits/for-mripard/bridge-rew...
But trying to boot hikey with that, I see the following loop indefinitely: [ 4.632132] adv7511 2-0039: supply avdd not found, using dummy regulator [ 4.638961] adv7511 2-0039: supply dvdd not found, using dummy regulator [ 4.645741] adv7511 2-0039: supply pvdd not found, using dummy regulator [ 4.652483] adv7511 2-0039: supply a2vdd not found, using dummy regulator [ 4.659342] adv7511 2-0039: supply v3p3 not found, using dummy regulator [ 4.666086] adv7511 2-0039: supply v1p2 not found, using dummy regulator [ 4.681898] adv7511 2-0039: failed to find dsi host
I just realized Rob's tree is missing the kirin patch. My apologies! I'll retest and let you know.
Ok, just retested including the kirin patch and unfortunately I'm still seeing the same thing. :(
Will dig a bit and let you know when I find more.
Hey Maxime! I chased down the issue. The dsi probe code was still calling drm_of_find_panel_or_bridge() in order to succeed.
I've moved the logic that looks for the bridge into the bridge_init and with that it seems to work.
Feel free (assuming it looks ok) to fold this change into your kirin patch: https://git.linaro.org/people/john.stultz/android-dev.git/commit/?id=4a35ccc...
thanks -john
Hi John,
On Wed, Sep 29, 2021 at 04:29:42PM -0700, John Stultz wrote:
On Wed, Sep 29, 2021 at 2:51 PM John Stultz john.stultz@linaro.org wrote:
On Wed, Sep 29, 2021 at 2:32 PM John Stultz john.stultz@linaro.org wrote:
On Wed, Sep 29, 2021 at 2:27 PM John Stultz john.stultz@linaro.org wrote:
On Fri, Sep 10, 2021 at 3:12 AM Maxime Ripard maxime@cerno.tech wrote:
The best practice to avoid those issues is to register its functions only after all its dependencies are live. We also shouldn't wait any longer than we should to play nice with the other components that are waiting for us, so in our case that would mean moving the DSI device registration to the bridge probe.
I also had a look at all the DSI hosts, and it seems that exynos, kirin and msm would be affected by this and wouldn't probe anymore after those changes. Exynos and kirin seems to be simple enough for a mechanical change (that still requires to be tested), but the changes in msm seemed to be far more important and I wasn't confortable doing them.
Hey Maxime, Sorry for taking so long to get to this, but now that plumbers is over I've had a chance to check it out on kirin
Rob Clark pointed me to his branch with some fixups here: https://gitlab.freedesktop.org/robclark/msm/-/commits/for-mripard/bridge-rew...
But trying to boot hikey with that, I see the following loop indefinitely: [ 4.632132] adv7511 2-0039: supply avdd not found, using dummy regulator [ 4.638961] adv7511 2-0039: supply dvdd not found, using dummy regulator [ 4.645741] adv7511 2-0039: supply pvdd not found, using dummy regulator [ 4.652483] adv7511 2-0039: supply a2vdd not found, using dummy regulator [ 4.659342] adv7511 2-0039: supply v3p3 not found, using dummy regulator [ 4.666086] adv7511 2-0039: supply v1p2 not found, using dummy regulator [ 4.681898] adv7511 2-0039: failed to find dsi host
I just realized Rob's tree is missing the kirin patch. My apologies! I'll retest and let you know.
Ok, just retested including the kirin patch and unfortunately I'm still seeing the same thing. :(
Will dig a bit and let you know when I find more.
Hey Maxime! I chased down the issue. The dsi probe code was still calling drm_of_find_panel_or_bridge() in order to succeed.
I've moved the logic that looks for the bridge into the bridge_init and with that it seems to work.
Feel free (assuming it looks ok) to fold this change into your kirin patch: https://git.linaro.org/people/john.stultz/android-dev.git/commit/?id=4a35ccc...
Thanks for testing, I've picked and squashed your fixup
Maxime
Hi Maxime,
(CC'ing Kieran)
On Fri, Sep 10, 2021 at 12:11:54PM +0200, Maxime Ripard wrote:
Hi,
We've encountered an issue with the RaspberryPi DSI panel that prevented the whole display driver from probing.
The issue is described in detail in the commit 7213246a803f ("drm/vc4: dsi: Only register our component once a DSI device is attached"), but the basic idea is that since the panel is probed through i2c, there's no synchronization between its probe and the registration of the MIPI-DSI host it's attached to.
We initially moved the component framework registration to the MIPI-DSI Host attach hook to make sure we register our component only when we have a DSI device attached to our MIPI-DSI host, and then use lookup our DSI device in our bind hook.
However, all the DSI bridges controlled through i2c are only registering their associated DSI device in their bridge attach hook, meaning with our change above, we never got that far, and therefore ended up in the same situation than the one we were trying to fix for panels.
The best practice to avoid those issues is to register its functions only after all its dependencies are live. We also shouldn't wait any longer than we should to play nice with the other components that are waiting for us, so in our case that would mean moving the DSI device registration to the bridge probe.
I also had a look at all the DSI hosts, and it seems that exynos, kirin and msm would be affected by this and wouldn't probe anymore after those changes. Exynos and kirin seems to be simple enough for a mechanical change (that still requires to be tested), but the changes in msm seemed to be far more important and I wasn't confortable doing them.
Let me know what you think,
I've tested this series on my RPi CM4-based board, and there's a clear improvement: the sn65dsi83 now probes successfully !
The downside is that I can now look at a panel that desperately refuses to display anything. That's a separate issue, but it prevents me from telling whether this series introduces regressions :-S I'll try to debug that separately.
Also, Kieran, would you be able to test this with the SN65DSI86 ?
Changes from v3:
- Converted exynos and kirin
- Converted all the affected bridge drivers
- Reworded the documentation a bit
Changes from v2:
- Changed the approach as suggested by Andrzej, and aligned the bridge on the panel this time.
- Fixed some typos
Changes from v1:
- Change the name of drm_of_get_next function to drm_of_get_bridge
- Mention the revert of 87154ff86bf6 and squash the two patches that were reverting that commit
- Add some documentation
- Make drm_panel_attach and _detach succeed when no callback is there
Maxime Ripard (24): drm/bridge: Add documentation sections drm/bridge: Document the probe issue with MIPI-DSI bridges drm/mipi-dsi: Create devm device registration drm/mipi-dsi: Create devm device attachment drm/bridge: adv7533: Switch to devm MIPI-DSI helpers drm/bridge: adv7511: Register and attach our DSI device at probe drm/bridge: anx7625: Switch to devm MIPI-DSI helpers drm/bridge: anx7625: Register and attach our DSI device at probe drm/bridge: lt8912b: Switch to devm MIPI-DSI helpers drm/bridge: lt8912b: Register and attach our DSI device at probe drm/bridge: lt9611: Switch to devm MIPI-DSI helpers drm/bridge: lt9611: Register and attach our DSI device at probe drm/bridge: lt9611uxc: Switch to devm MIPI-DSI helpers drm/bridge: lt9611uxc: Register and attach our DSI device at probe drm/bridge: ps8640: Switch to devm MIPI-DSI helpers drm/bridge: ps8640: Register and attach our DSI device at probe drm/bridge: sn65dsi83: Switch to devm MIPI-DSI helpers drm/bridge: sn65dsi83: Register and attach our DSI device at probe drm/bridge: sn65dsi86: Switch to devm MIPI-DSI helpers drm/bridge: sn65dsi86: Register and attach our DSI device at probe drm/bridge: tc358775: Switch to devm MIPI-DSI helpers drm/bridge: tc358775: Register and attach our DSI device at probe drm/kirin: dsi: Adjust probe order drm/exynos: dsi: Adjust probe order
Documentation/gpu/drm-kms-helpers.rst | 12 +++ drivers/gpu/drm/bridge/adv7511/adv7511.h | 1 - drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 15 ++- drivers/gpu/drm/bridge/adv7511/adv7533.c | 20 +--- drivers/gpu/drm/bridge/analogix/anx7625.c | 40 ++++---- drivers/gpu/drm/bridge/lontium-lt8912b.c | 31 ++---- drivers/gpu/drm/bridge/lontium-lt9611.c | 62 +++++------- drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 65 +++++------- drivers/gpu/drm/bridge/parade-ps8640.c | 101 ++++++++++--------- drivers/gpu/drm/bridge/tc358775.c | 50 +++++---- drivers/gpu/drm/bridge/ti-sn65dsi83.c | 86 ++++++++-------- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 94 ++++++++--------- drivers/gpu/drm/drm_bridge.c | 69 ++++++++++++- drivers/gpu/drm/drm_mipi_dsi.c | 81 +++++++++++++++ drivers/gpu/drm/exynos/exynos_drm_dsi.c | 19 ++-- drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c | 27 +++-- include/drm/drm_mipi_dsi.h | 4 + 17 files changed, 460 insertions(+), 317 deletions(-)
Hi Maxime,
On Thu, Sep 30, 2021 at 12:56:02AM +0300, Laurent Pinchart wrote:
On Fri, Sep 10, 2021 at 12:11:54PM +0200, Maxime Ripard wrote:
Hi,
We've encountered an issue with the RaspberryPi DSI panel that prevented the whole display driver from probing.
The issue is described in detail in the commit 7213246a803f ("drm/vc4: dsi: Only register our component once a DSI device is attached"), but the basic idea is that since the panel is probed through i2c, there's no synchronization between its probe and the registration of the MIPI-DSI host it's attached to.
We initially moved the component framework registration to the MIPI-DSI Host attach hook to make sure we register our component only when we have a DSI device attached to our MIPI-DSI host, and then use lookup our DSI device in our bind hook.
However, all the DSI bridges controlled through i2c are only registering their associated DSI device in their bridge attach hook, meaning with our change above, we never got that far, and therefore ended up in the same situation than the one we were trying to fix for panels.
The best practice to avoid those issues is to register its functions only after all its dependencies are live. We also shouldn't wait any longer than we should to play nice with the other components that are waiting for us, so in our case that would mean moving the DSI device registration to the bridge probe.
I also had a look at all the DSI hosts, and it seems that exynos, kirin and msm would be affected by this and wouldn't probe anymore after those changes. Exynos and kirin seems to be simple enough for a mechanical change (that still requires to be tested), but the changes in msm seemed to be far more important and I wasn't confortable doing them.
Let me know what you think,
I've tested this series on my RPi CM4-based board, and there's a clear improvement: the sn65dsi83 now probes successfully !
The downside is that I can now look at a panel that desperately refuses to display anything. That's a separate issue, but it prevents me from telling whether this series introduces regressions :-S I'll try to debug that separately.
I managed to (partly) fix that issue with a few backports from the RPi kernel, making me confident enough to say
Tested-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
for
drivers/gpu/drm/bridge/ti-sn65dsi83.c drivers/gpu/drm/drm_bridge.c drivers/gpu/drm/drm_mipi_dsi.c include/drm/drm_mipi_dsi.h
Also, Kieran, would you be able to test this with the SN65DSI86 ?
Changes from v3:
- Converted exynos and kirin
- Converted all the affected bridge drivers
- Reworded the documentation a bit
Changes from v2:
- Changed the approach as suggested by Andrzej, and aligned the bridge on the panel this time.
- Fixed some typos
Changes from v1:
- Change the name of drm_of_get_next function to drm_of_get_bridge
- Mention the revert of 87154ff86bf6 and squash the two patches that were reverting that commit
- Add some documentation
- Make drm_panel_attach and _detach succeed when no callback is there
Maxime Ripard (24): drm/bridge: Add documentation sections drm/bridge: Document the probe issue with MIPI-DSI bridges drm/mipi-dsi: Create devm device registration drm/mipi-dsi: Create devm device attachment drm/bridge: adv7533: Switch to devm MIPI-DSI helpers drm/bridge: adv7511: Register and attach our DSI device at probe drm/bridge: anx7625: Switch to devm MIPI-DSI helpers drm/bridge: anx7625: Register and attach our DSI device at probe drm/bridge: lt8912b: Switch to devm MIPI-DSI helpers drm/bridge: lt8912b: Register and attach our DSI device at probe drm/bridge: lt9611: Switch to devm MIPI-DSI helpers drm/bridge: lt9611: Register and attach our DSI device at probe drm/bridge: lt9611uxc: Switch to devm MIPI-DSI helpers drm/bridge: lt9611uxc: Register and attach our DSI device at probe drm/bridge: ps8640: Switch to devm MIPI-DSI helpers drm/bridge: ps8640: Register and attach our DSI device at probe drm/bridge: sn65dsi83: Switch to devm MIPI-DSI helpers drm/bridge: sn65dsi83: Register and attach our DSI device at probe drm/bridge: sn65dsi86: Switch to devm MIPI-DSI helpers drm/bridge: sn65dsi86: Register and attach our DSI device at probe drm/bridge: tc358775: Switch to devm MIPI-DSI helpers drm/bridge: tc358775: Register and attach our DSI device at probe drm/kirin: dsi: Adjust probe order drm/exynos: dsi: Adjust probe order
Documentation/gpu/drm-kms-helpers.rst | 12 +++ drivers/gpu/drm/bridge/adv7511/adv7511.h | 1 - drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 15 ++- drivers/gpu/drm/bridge/adv7511/adv7533.c | 20 +--- drivers/gpu/drm/bridge/analogix/anx7625.c | 40 ++++---- drivers/gpu/drm/bridge/lontium-lt8912b.c | 31 ++---- drivers/gpu/drm/bridge/lontium-lt9611.c | 62 +++++------- drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 65 +++++------- drivers/gpu/drm/bridge/parade-ps8640.c | 101 ++++++++++--------- drivers/gpu/drm/bridge/tc358775.c | 50 +++++---- drivers/gpu/drm/bridge/ti-sn65dsi83.c | 86 ++++++++-------- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 94 ++++++++--------- drivers/gpu/drm/drm_bridge.c | 69 ++++++++++++- drivers/gpu/drm/drm_mipi_dsi.c | 81 +++++++++++++++ drivers/gpu/drm/exynos/exynos_drm_dsi.c | 19 ++-- drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c | 27 +++-- include/drm/drm_mipi_dsi.h | 4 + 17 files changed, 460 insertions(+), 317 deletions(-)
Hi Maxime,
drm/bridge: adv7533: Switch to devm MIPI-DSI helpers drm/bridge: adv7511: Register and attach our DSI device at probe drm/bridge: anx7625: Switch to devm MIPI-DSI helpers drm/bridge: anx7625: Register and attach our DSI device at probe drm/bridge: lt8912b: Switch to devm MIPI-DSI helpers drm/bridge: lt8912b: Register and attach our DSI device at probe drm/bridge: lt9611: Switch to devm MIPI-DSI helpers drm/bridge: lt9611: Register and attach our DSI device at probe drm/bridge: lt9611uxc: Switch to devm MIPI-DSI helpers drm/bridge: lt9611uxc: Register and attach our DSI device at probe drm/bridge: ps8640: Switch to devm MIPI-DSI helpers drm/bridge: ps8640: Register and attach our DSI device at probe
All the above are: Acked-by: Sam Ravnborg sam@ravnborg.org
Will try to look at sn65dsi83 and tc358775 later today.
I assume kirin and exynos is already covered.
Sam
Hi Maxime,
drm/bridge: sn65dsi83: Switch to devm MIPI-DSI helpers drm/bridge: sn65dsi83: Register and attach our DSI device at probe drm/bridge: sn65dsi86: Switch to devm MIPI-DSI helpers drm/bridge: sn65dsi86: Register and attach our DSI device at probe drm/bridge: tc358775: Switch to devm MIPI-DSI helpers drm/bridge: tc358775: Register and attach our DSI device at probe
The above are: Acked-by: Sam Ravnborg sam@ravnborg.org
I hope you can land this series soon.
Sam
dri-devel@lists.freedesktop.org
-
Amit Pundir -
Andrzej Hajda -
Caleb Connolly -
John Stultz -
Laurent Pinchart -
Marek Szyprowski -
Maxime Ripard -
Rob Clark -
Sam Ravnborg