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.
Since the RaspberryPi panel is the only driver in that situation, whereas it seems like there's a consensus in bridge drivers, it makes more sense to try to mimic the bridge pattern in the panel driver.
However, panels don't have an attach hook, and adding more panel hooks would lead to more path to maintain in each and every driver, while the general push is towards bridges. We also have to make sure that each and every DSI host and device driver behaves the same in order to have expectations to rely on.
The solution I'm proposing is thus done in several steps:
- We get rid of the initial patch to make sure we support the bridge case, and not the odd-panel one.
- Add a function that returns a bridge from a DT node, reducing the amount of churn in each and every driver and making it a real incentive to not care about panels in display drivers but only bridges.
- Add an attach and detach hook into the panel operations, and make it called automatically by the DRM panel bridge.
- Convert the VC4 DSI host to this new bridge function, and the RaspberryPi Panel to the new attach and detach hooks.
If the general approach is agreed upon, other drivers will obviously be converted to drm_of_get_next.
Let me know what you think, Maxime
---
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 (8): Revert "drm/vc4: dsi: Only register our component once a DSI device is attached" drm/bridge: Add a function to abstract away panels drm/bridge: Add documentation sections drm/bridge: Document the probe issue with MIPI-DSI bridges drm/panel: Create attach and detach callbacks drm/vc4: dsi: Switch to drm_of_get_bridge drm/panel: raspberrypi-touchscreen: Use the attach hook drm/panel: raspberrypi-touchscreen: Remove MIPI-DSI driver
Documentation/gpu/drm-kms-helpers.rst | 12 ++ drivers/gpu/drm/bridge/panel.c | 6 + drivers/gpu/drm/drm_bridge.c | 114 ++++++++++++- drivers/gpu/drm/drm_of.c | 3 + drivers/gpu/drm/drm_panel.c | 47 ++++++ .../drm/panel/panel-raspberrypi-touchscreen.c | 158 +++++++++--------- drivers/gpu/drm/vc4/vc4_drv.c | 2 + drivers/gpu/drm/vc4/vc4_dsi.c | 53 +++--- include/drm/drm_bridge.h | 2 + include/drm/drm_panel.h | 27 +++ 10 files changed, 303 insertions(+), 121 deletions(-)
This reverts commit 7213246a803f9b8da0677adb9ae06a3d8b806d02.
The commit 7213246a803f ("drm/vc4: dsi: Only register our component once a DSI device is attached") aimed at preventing an endless probe loop between the DSI host driver and its panel/bridge where both would wait for each other to probe.
The solution implemented in that commit however relies on the fact that MIPI-DSI device will either be a MIPI-DSI device, or would call mipi_dsi_device_register_full() at probe time.
This assumption isn't true for bridges though where most drivers will do so in the bridge attach hook. However, the drm_bridge_attach is usually called in the DSI host bind hook, and thus we never get this far, resulting in a DSI bridge that will never have its attach run, and the DSI host that will never be bound, effectively creating the same situation we were trying to avoid for panels.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_dsi.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c index a55256ed0955..6dfcbd9e234e 100644 --- a/drivers/gpu/drm/vc4/vc4_dsi.c +++ b/drivers/gpu/drm/vc4/vc4_dsi.c @@ -1257,12 +1257,10 @@ static ssize_t vc4_dsi_host_transfer(struct mipi_dsi_host *host, return ret; }
-static const struct component_ops vc4_dsi_ops; static int vc4_dsi_host_attach(struct mipi_dsi_host *host, struct mipi_dsi_device *device) { struct vc4_dsi *dsi = host_to_dsi(host); - int ret;
dsi->lanes = device->lanes; dsi->channel = device->channel; @@ -1297,12 +1295,6 @@ static int vc4_dsi_host_attach(struct mipi_dsi_host *host, return 0; }
- ret = component_add(&dsi->pdev->dev, &vc4_dsi_ops); - if (ret) { - mipi_dsi_host_unregister(&dsi->dsi_host); - return ret; - } - return 0; }
@@ -1689,6 +1681,7 @@ static int vc4_dsi_dev_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; struct vc4_dsi *dsi; + int ret;
dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL); if (!dsi) @@ -1696,10 +1689,26 @@ static int vc4_dsi_dev_probe(struct platform_device *pdev) dev_set_drvdata(dev, dsi);
dsi->pdev = pdev; + + /* Note, the initialization sequence for DSI and panels is + * tricky. The component bind above won't get past its + * -EPROBE_DEFER until the panel/bridge probes. The + * panel/bridge will return -EPROBE_DEFER until it has a + * mipi_dsi_host to register its device to. So, we register + * the host during pdev probe time, so vc4 as a whole can then + * -EPROBE_DEFER its component bind process until the panel + * successfully attaches. + */ dsi->dsi_host.ops = &vc4_dsi_host_ops; dsi->dsi_host.dev = dev; mipi_dsi_host_register(&dsi->dsi_host);
+ ret = component_add(&pdev->dev, &vc4_dsi_ops); + if (ret) { + mipi_dsi_host_unregister(&dsi->dsi_host); + return ret; + } + return 0; }
Display drivers so far need to have a lot of boilerplate to first retrieve either the panel or bridge that they are connected to using drm_of_find_panel_or_bridge(), and then either deal with each with ad-hoc functions or create a drm panel bridge through drm_panel_bridge_add.
In order to reduce the boilerplate and hopefully create a path of least resistance towards using the DRM panel bridge layer, let's create the function devm_drm_of_get_next to reduce that boilerplate.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/drm_bridge.c | 42 ++++++++++++++++++++++++++++++++---- drivers/gpu/drm/drm_of.c | 3 +++ include/drm/drm_bridge.h | 2 ++ 3 files changed, 43 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 044acd07c153..426ce7442c86 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -28,6 +28,7 @@ #include <drm/drm_atomic_state_helper.h> #include <drm/drm_bridge.h> #include <drm/drm_encoder.h> +#include <drm/drm_of.h>
#include "drm_crtc_internal.h"
@@ -50,10 +51,8 @@ * * 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 - * panel with drm_panel_bridge_add_typed() (or the managed version - * devm_drm_panel_bridge_add_typed()). Once acquired, the bridge shall be - * attached to the encoder with a call to drm_bridge_attach(). + * devm_drm_of_get_bridge(). Once acquired, the bridge shall be attached to the + * encoder with a call to drm_bridge_attach(). * * Bridges are responsible for linking themselves with the next bridge in the * chain, if any. This is done the same way as for encoders, with the call to @@ -1223,6 +1222,41 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np) return NULL; } EXPORT_SYMBOL(of_drm_find_bridge); + +/** + * devm_drm_of_get_bridge - Return next bridge in the chain + * @dev: device to tie the bridge lifetime to + * @np: device tree node containing encoder output ports + * @port: port in the device tree node + * @endpoint: endpoint in the device tree node + * + * Given a DT node's port and endpoint number, finds the connected node + * and returns the associated bridge if any, or creates and returns a + * drm panel bridge instance if a panel is connected. + * + * Returns a pointer to the bridge if successful, or an error pointer + * otherwise. + */ +struct drm_bridge *devm_drm_of_get_bridge(struct device *dev, + struct device_node *np, + unsigned int port, + unsigned int endpoint) +{ + struct drm_bridge *bridge; + struct drm_panel *panel; + int ret; + + ret = drm_of_find_panel_or_bridge(np, port, endpoint, + &panel, &bridge); + if (ret) + return ERR_PTR(ret); + + if (panel) + bridge = devm_drm_panel_bridge_add(dev, panel); + + return bridge; +} +EXPORT_SYMBOL(devm_drm_of_get_bridge); #endif
MODULE_AUTHOR("Ajay Kumar ajaykumar.rs@samsung.com"); diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c index 997b8827fed2..37c34146eea8 100644 --- a/drivers/gpu/drm/drm_of.c +++ b/drivers/gpu/drm/drm_of.c @@ -231,6 +231,9 @@ EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint); * return either the associated struct drm_panel or drm_bridge device. Either * @panel or @bridge must not be NULL. * + * This function is deprecated and should not be used in new drivers. Use + * devm_drm_of_get_bridge() instead. + * * Returns zero if successful, or one of the standard error codes if it fails. */ int drm_of_find_panel_or_bridge(const struct device_node *np, diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 46bdfa48c413..f70c88ca96ef 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -911,6 +911,8 @@ struct drm_bridge *devm_drm_panel_bridge_add(struct device *dev, struct drm_bridge *devm_drm_panel_bridge_add_typed(struct device *dev, struct drm_panel *panel, u32 connector_type); +struct drm_bridge *devm_drm_of_get_bridge(struct device *dev, struct device_node *node, + unsigned int port, unsigned int endpoint); struct drm_connector *drm_panel_bridge_connector(struct drm_bridge *bridge); #endif
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: 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 426ce7442c86..71d3370ce209 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 * devm_drm_of_get_bridge(). Once acquired, the bridge shall be attached to the @@ -83,11 +92,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);
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 | 60 +++++++++++++++++++++++++++ 2 files changed, 66 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 71d3370ce209..fd01f1ce7976 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -94,6 +94,66 @@ * 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 display driver and the bridge driver can be + * challenging. Indeed, there's multiple cases that needs to be + * considered: + * + * - The display 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 display driver should try to probe again by returning + * EPROBE_DEFER as long as the bridge driver hasn't probed. + * + * - The display 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 display 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 display 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 display 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 display 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 + * display driver case) is to split the operations like this: + * + * - In the display driver must run mipi_dsi_host_register() and + * component_add 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 not try to find its + * MIPI-DSI host or register as a MIPI-DSI device. As far as the + * framework is concerned, it must only call drm_bridge_add(). + * + * - In its bind hook, the display driver must try to find the bridge + * and return -EPROBE_DEFER if it doesn't find it. If it's there, it + * must call drm_bridge_attach(). The MIPI-DSI host is now functional. + * + * - In its &drm_bridge_funcs.attach hook, the bridge driver can now try + * to find its MIPI-DSI host and can register as a MIPI-DSI device. + * + * At this point, we're now certain that both the display 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);
This is a partial revert of 87154ff86bf6 ("drm: Remove unnecessary drm_panel_attach and drm_panel_detach").
In order to make the probe order expectation more consistent between bridges, let's create attach and detach hooks for the panels as well to match what is there for bridges.
Most drivers have changed significantly since the reverted commit, so we only brought back the calls in the panel bridge to lessen the risk of regressions, and since panel bridge is what we're converging to these days, it's not a big deal anyway.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/bridge/panel.c | 6 +++++ drivers/gpu/drm/drm_panel.c | 47 ++++++++++++++++++++++++++++++++++ include/drm/drm_panel.h | 27 +++++++++++++++++++ 3 files changed, 80 insertions(+)
diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index c916f4b8907e..0b06e0dbad00 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -82,6 +82,10 @@ static int panel_bridge_attach(struct drm_bridge *bridge, drm_connector_attach_encoder(&panel_bridge->connector, bridge->encoder);
+ ret = drm_panel_attach(panel_bridge->panel, &panel_bridge->connector); + if (ret < 0) + return ret; + return 0; }
@@ -90,6 +94,8 @@ static void panel_bridge_detach(struct drm_bridge *bridge) struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); struct drm_connector *connector = &panel_bridge->connector;
+ drm_panel_detach(panel_bridge->panel); + /* * Cleanup the connector if we know it was initialized. * diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c index f634371c717a..10716b740685 100644 --- a/drivers/gpu/drm/drm_panel.c +++ b/drivers/gpu/drm/drm_panel.c @@ -93,6 +93,53 @@ void drm_panel_remove(struct drm_panel *panel) } EXPORT_SYMBOL(drm_panel_remove);
+/** + * drm_panel_attach - attach a panel to a connector + * @panel: DRM panel + * @connector: DRM connector + * + * After obtaining a pointer to a DRM panel a display driver calls this + * function to attach a panel to a connector. + * + * An error is returned if the panel is already attached to another connector. + * + * When unloading, the driver should detach from the panel by calling + * drm_panel_detach(). + * + * Return: 0 on success or a negative error code on failure. + */ +int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector) +{ + if (!panel) + return -EINVAL; + + if (panel->funcs && panel->funcs->attach) + return panel->funcs->attach(panel); + + return 0; +} +EXPORT_SYMBOL(drm_panel_attach); + +/** + * drm_panel_detach - detach a panel from a connector + * @panel: DRM panel + * + * Detaches a panel from the connector it is attached to. If a panel is not + * attached to any connector this is effectively a no-op. + * + * This function should not be called by the panel device itself. It + * is only for the drm device that called drm_panel_attach(). + */ +void drm_panel_detach(struct drm_panel *panel) +{ + if (!panel) + return; + + if (panel->funcs && panel->funcs->detach) + panel->funcs->detach(panel); +} +EXPORT_SYMBOL(drm_panel_detach); + /** * drm_panel_prepare - power on a panel * @panel: DRM panel diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 4602f833eb51..33d139e98b19 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -68,6 +68,30 @@ enum drm_panel_orientation; * does not need to implement the functionality to enable/disable backlight. */ struct drm_panel_funcs { + /** + * @attach: + * + * This callback is invoked whenever our panel is being attached + * to its DRM connector. + * + * The @attach callback is optional. + * + * RETURNS: + * + * Zero on success, error code on failure. + */ + int (*attach)(struct drm_panel *panel); + + /** + * @detach: + * + * This callback is invoked whenever our panel is being detached + * from its DRM connector. + * + * The @detach callback is optional. + */ + void (*detach)(struct drm_panel *panel); + /** * @prepare: * @@ -180,6 +204,9 @@ void drm_panel_init(struct drm_panel *panel, struct device *dev, void drm_panel_add(struct drm_panel *panel); void drm_panel_remove(struct drm_panel *panel);
+int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector); +void drm_panel_detach(struct drm_panel *panel); + int drm_panel_prepare(struct drm_panel *panel); int drm_panel_unprepare(struct drm_panel *panel);
The new drm_of_get_bridge removes most of the boilerplate we have to deal with. Let's switch to it.
Signed-off-by: Maxime Ripard maxime@cerno.tech
fixup! drm/vc4: dsi: Switch to drm_of_get_bridge --- drivers/gpu/drm/vc4/vc4_drv.c | 2 ++ drivers/gpu/drm/vc4/vc4_dsi.c | 28 ++++------------------------ 2 files changed, 6 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c index 73335feb712f..ff056ee8bc4b 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.c +++ b/drivers/gpu/drm/vc4/vc4_drv.c @@ -25,7 +25,9 @@ #include <linux/device.h> #include <linux/dma-mapping.h> #include <linux/io.h> +#include <linux/i2c.h> #include <linux/module.h> +#include <linux/of_graph.h> #include <linux/of_platform.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c index 6dfcbd9e234e..3db03c95707f 100644 --- a/drivers/gpu/drm/vc4/vc4_dsi.c +++ b/drivers/gpu/drm/vc4/vc4_dsi.c @@ -1489,7 +1489,6 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data) struct drm_device *drm = dev_get_drvdata(master); struct vc4_dsi *dsi = dev_get_drvdata(dev); struct vc4_dsi_encoder *vc4_dsi_encoder; - struct drm_panel *panel; const struct of_device_id *match; dma_cap_mask_t dma_mask; int ret; @@ -1601,27 +1600,9 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data) return ret; }
- ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, - &panel, &dsi->bridge); - if (ret) { - /* If the bridge or panel pointed by dev->of_node is not - * enabled, just return 0 here so that we don't prevent the DRM - * dev from being registered. Of course that means the DSI - * encoder won't be exposed, but that's not a problem since - * nothing is connected to it. - */ - if (ret == -ENODEV) - return 0; - - return ret; - } - - if (panel) { - dsi->bridge = devm_drm_panel_bridge_add_typed(dev, panel, - DRM_MODE_CONNECTOR_DSI); - if (IS_ERR(dsi->bridge)) - return PTR_ERR(dsi->bridge); - } + dsi->bridge = devm_drm_of_get_bridge(dev, dev->of_node, 0, 0); + if (IS_ERR(dsi->bridge)) + return PTR_ERR(dsi->bridge);
/* The esc clock rate is supposed to always be 100Mhz. */ ret = clk_set_rate(dsi->escape_clock, 100 * 1000000); @@ -1661,8 +1642,7 @@ static void vc4_dsi_unbind(struct device *dev, struct device *master, { struct vc4_dsi *dsi = dev_get_drvdata(dev);
- if (dsi->bridge) - pm_runtime_disable(dev); + pm_runtime_disable(dev);
/* * Restore the bridge_chain so the bridge detach procedure can happen
Now that we have an attach hook available for panels as well, let's use it for the RaspberryPi 7" DSI panel.
This now mimics what all the other bridges in a similar situation are doing, and we avoid our probe order issue entirely.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- .../drm/panel/panel-raspberrypi-touchscreen.c | 135 ++++++++++-------- 1 file changed, 77 insertions(+), 58 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c index 462faae0f446..995c5cafb970 100644 --- a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c +++ b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c @@ -346,7 +346,83 @@ static int rpi_touchscreen_get_modes(struct drm_panel *panel, return num; }
+static int rpi_touchscreen_attach(struct drm_panel *panel) +{ + struct rpi_touchscreen *ts = panel_to_ts(panel); + struct device *dev = &ts->i2c->dev; + struct device_node *endpoint, *dsi_host_node; + struct mipi_dsi_device *dsi; + struct mipi_dsi_host *host; + int ret; + + struct mipi_dsi_device_info info = { + .type = RPI_DSI_DRIVER_NAME, + .channel = 0, + .node = NULL, + }; + + /* Look up the DSI host. It needs to probe before we do. */ + endpoint = of_graph_get_next_endpoint(dev->of_node, NULL); + if (!endpoint) + return -ENODEV; + + dsi_host_node = of_graph_get_remote_port_parent(endpoint); + if (!dsi_host_node) { + of_node_put(endpoint); + return -ENODEV; + } + + host = of_find_mipi_dsi_host_by_node(dsi_host_node); + of_node_put(dsi_host_node); + if (!host) { + of_node_put(endpoint); + return -EPROBE_DEFER; + } + + info.node = of_graph_get_remote_port(endpoint); + if (!info.node) { + of_node_put(endpoint); + return -ENODEV; + } + + of_node_put(endpoint); + + dsi = mipi_dsi_device_register_full(host, &info); + if (IS_ERR(dsi)) { + dev_err(dev, "DSI device registration failed: %ld\n", + PTR_ERR(dsi)); + return PTR_ERR(dsi); + } + + ts->dsi = dsi; + + dsi->mode_flags = (MIPI_DSI_MODE_VIDEO | + MIPI_DSI_MODE_VIDEO_SYNC_PULSE | + MIPI_DSI_MODE_LPM); + dsi->format = MIPI_DSI_FMT_RGB888; + dsi->lanes = 1; + + ret = mipi_dsi_attach(dsi); + if (ret) { + dev_err(&dsi->dev, "failed to attach dsi to host: %d\n", ret); + return ret; + } + + return 0; +} + +static void rpi_touchscreen_detach(struct drm_panel *panel) +{ + struct rpi_touchscreen *ts = panel_to_ts(panel); + + mipi_dsi_detach(ts->dsi); + mipi_dsi_device_unregister(ts->dsi); +} + static const struct drm_panel_funcs rpi_touchscreen_funcs = { + .attach = rpi_touchscreen_attach, + .detach = rpi_touchscreen_detach, + .disable = rpi_touchscreen_disable, .unprepare = rpi_touchscreen_noop, .prepare = rpi_touchscreen_noop, @@ -359,14 +435,7 @@ static int rpi_touchscreen_probe(struct i2c_client *i2c, { struct device *dev = &i2c->dev; struct rpi_touchscreen *ts; - struct device_node *endpoint, *dsi_host_node; - struct mipi_dsi_host *host; int ver; - struct mipi_dsi_device_info info = { - .type = RPI_DSI_DRIVER_NAME, - .channel = 0, - .node = NULL, - };
ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL); if (!ts) @@ -394,35 +463,6 @@ static int rpi_touchscreen_probe(struct i2c_client *i2c, /* /* Turn off at boot, so we can cleanly sequence powering on. */ */ /* rpi_touchscreen_i2c_write(ts, REG_POWERON, 0); */
- /* Look up the DSI host. It needs to probe before we do. */ - endpoint = of_graph_get_next_endpoint(dev->of_node, NULL); - if (!endpoint) - return -ENODEV; - - dsi_host_node = of_graph_get_remote_port_parent(endpoint); - if (!dsi_host_node) - goto error; - - host = of_find_mipi_dsi_host_by_node(dsi_host_node); - of_node_put(dsi_host_node); - if (!host) { - of_node_put(endpoint); - return -EPROBE_DEFER; - } - - info.node = of_graph_get_remote_port(endpoint); - if (!info.node) - goto error; - - of_node_put(endpoint); - - ts->dsi = mipi_dsi_device_register_full(host, &info); - if (IS_ERR(ts->dsi)) { - dev_err(dev, "DSI device registration failed: %ld\n", - PTR_ERR(ts->dsi)); - return PTR_ERR(ts->dsi); - } - drm_panel_init(&ts->base, dev, &rpi_touchscreen_funcs, DRM_MODE_CONNECTOR_DSI);
@@ -432,41 +472,20 @@ static int rpi_touchscreen_probe(struct i2c_client *i2c, drm_panel_add(&ts->base);
return 0; - -error: - of_node_put(endpoint); - return -ENODEV; }
static int rpi_touchscreen_remove(struct i2c_client *i2c) { struct rpi_touchscreen *ts = i2c_get_clientdata(i2c);
- mipi_dsi_detach(ts->dsi); - drm_panel_remove(&ts->base);
- mipi_dsi_device_unregister(ts->dsi); - return 0; }
static int rpi_touchscreen_dsi_probe(struct mipi_dsi_device *dsi) { - int ret; - - dsi->mode_flags = (MIPI_DSI_MODE_VIDEO | - MIPI_DSI_MODE_VIDEO_SYNC_PULSE | - MIPI_DSI_MODE_LPM); - dsi->format = MIPI_DSI_FMT_RGB888; - dsi->lanes = 1; - - ret = mipi_dsi_attach(dsi); - - if (ret) - dev_err(&dsi->dev, "failed to attach dsi to host: %d\n", ret); - - return ret; + return 0; }
static struct mipi_dsi_driver rpi_touchscreen_dsi_driver = {
The driver was using a two-steps initialisation when probing with the i2c probe first registering the MIPI-DSI device, and then when that device was probed the driver would attach the device to its host. This resulted in a fairly non-standard probe logic.
The previous commit changed that logic entirely though, resulting in a completely empty MIPI-DSI device probe. Let's simplify the driver by removing it entirely and just behave as a normal i2c driver.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- .../drm/panel/panel-raspberrypi-touchscreen.c | 25 +------------------ 1 file changed, 1 insertion(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c index 995c5cafb970..09937aa26c6a 100644 --- a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c +++ b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c @@ -483,16 +483,6 @@ static int rpi_touchscreen_remove(struct i2c_client *i2c) return 0; }
-static int rpi_touchscreen_dsi_probe(struct mipi_dsi_device *dsi) -{ - return 0; -} - -static struct mipi_dsi_driver rpi_touchscreen_dsi_driver = { - .driver.name = RPI_DSI_DRIVER_NAME, - .probe = rpi_touchscreen_dsi_probe, -}; - static const struct of_device_id rpi_touchscreen_of_ids[] = { { .compatible = "raspberrypi,7inch-touchscreen-panel" }, { } /* sentinel */ @@ -507,20 +497,7 @@ static struct i2c_driver rpi_touchscreen_driver = { .probe = rpi_touchscreen_probe, .remove = rpi_touchscreen_remove, }; - -static int __init rpi_touchscreen_init(void) -{ - mipi_dsi_driver_register(&rpi_touchscreen_dsi_driver); - return i2c_add_driver(&rpi_touchscreen_driver); -} -module_init(rpi_touchscreen_init); - -static void __exit rpi_touchscreen_exit(void) -{ - i2c_del_driver(&rpi_touchscreen_driver); - mipi_dsi_driver_unregister(&rpi_touchscreen_dsi_driver); -} -module_exit(rpi_touchscreen_exit); +module_i2c_driver(rpi_touchscreen_driver);
MODULE_AUTHOR("Eric Anholt eric@anholt.net"); MODULE_DESCRIPTION("Raspberry Pi 7-inch touchscreen driver");
Hi Maxime,
I have been busy with other tasks, and I did not follow the list last time, so sorry for my late response.
On 28.07.2021 15:32, 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
I guess this is incorrect. I have promoted several times the pattern that device driver shouldn't expose its interfaces (for example component_add, drm_panel_add, drm_bridge_add) until it gathers all required dependencies. In this particular case bridges should defer probe until DSI bus becomes available. I guess this way the patch you reverts would work.
I advised few times this pattern in case of DSI hosts, apparently I didn't notice the similar issue can appear in case of bridges. Or there is something I have missed???
Anyway there are already eleven(?) bridge drivers using this pattern. I wonder if fixing it would be difficult, or if it expose other issues??? The patches should be quite straightforward - move of_find_mipi_dsi_host_by_node and mipi_dsi_device_register_full to probe time.
Finally I think that if we will not fix these bridge drivers we will encounter another set of issues with new platforms connecting "DSI host drivers assuming this pattern" and "i2c/dsi device drivers assuming pattern already present in the bridges".
Regards Andrzej
Hi Andrzej,
On Wed, Aug 4, 2021 at 7:48 PM a.hajda a.hajda@samsung.com wrote:
Hi Maxime,
I have been busy with other tasks, and I did not follow the list last time, so sorry for my late response.
On 28.07.2021 15:32, 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
I guess this is incorrect. I have promoted several times the pattern that device driver shouldn't expose its interfaces (for example component_add, drm_panel_add, drm_bridge_add) until it gathers all required dependencies. In this particular case bridges should defer probe until DSI bus becomes available. I guess this way the patch you reverts would work.
I advised few times this pattern in case of DSI hosts, apparently I didn't notice the similar issue can appear in case of bridges. Or there is something I have missed???
Look like Maxime is correct. I2C based DSI bridge will get probe during bridge_attach which usually called from bridge driver bridge_attach call. Non-I2C bridges and DSI panels will get probe during host.attach.
We do get similar situation for dw-mipi-dsi bridges, where icn6211 bridge is not I2C-based bridge and it gets probed in host_attach and sn65dsi83 is I2C based bridge and it gets probed in bridge_attach.
Here is the simple call trace we have observed with dw-mipi-dsi bridge when all possible DSI device are trying to probe.
1. DSI panels and bridges will invoke the host attach from probe in order to find the panel_or_bridge.
chipone_probe() dw_mipi_dsi_host_attach().start dw_mipi_dsi_panel_or_bridge() ...found the panel_or_bridge...
ltdc_encoder_init().start dw_mipi_dsi_bridge_attach().start dw_mipi_dsi_host_attach().start chipone_attach(). start
chipone_attach(). done dw_mipi_dsi_host_attach().done dw_mipi_dsi_bridge_attach(). done ltdc_encoder_init().done
2. I2C based DSI bridge will invoke the drm_bridge_attach from bridge attach in order to find the panel_or_bridge.
ltdc_encoder_init().start dw_mipi_dsi_bridge_attach().start dw_mipi_dsi_panel_or_bridge() ...found the panel_or_bridge... dw_mipi_dsi_host_attach().start sn65dsi83_attach(). start
sn65dsi83_attach(). done dw_mipi_dsi_host_attach().done dw_mipi_dsi_bridge_attach(). done ltdc_encoder_init().done
It is correct that the I2C-based bridges will attach the host via mipi_dsi_attach in driver bridge API where as it done in probe for Non-I2C bridges and DSI panels.
Anyway there are already eleven(?) bridge drivers using this pattern. I wonder if fixing it would be difficult, or if it expose other issues??? The patches should be quite straightforward - move of_find_mipi_dsi_host_by_node and mipi_dsi_device_register_full to probe time.
Finally I think that if we will not fix these bridge drivers we will encounter another set of issues with new platforms connecting "DSI host drivers assuming this pattern" and "i2c/dsi device drivers assuming pattern already present in the bridges".
Agreed, I'm trying to understand the several ways to fix this. Right now I'm trying this on sun6i_mipi_dsi and exynos_drm_dsi. Will let you know for any update and suggestions on the same.
Thanks, Jagan.
Hi Jagan,
W dniu 09.08.2021 o 10:00, Jagan Teki pisze:
Hi Andrzej,
On Wed, Aug 4, 2021 at 7:48 PM a.hajda a.hajda@samsung.com wrote:
Hi Maxime,
I have been busy with other tasks, and I did not follow the list last time, so sorry for my late response.
On 28.07.2021 15:32, 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
I guess this is incorrect. I have promoted several times the pattern that device driver shouldn't expose its interfaces (for example component_add, drm_panel_add, drm_bridge_add) until it gathers all required dependencies. In this particular case bridges should defer probe until DSI bus becomes available. I guess this way the patch you reverts would work.
I advised few times this pattern in case of DSI hosts, apparently I didn't notice the similar issue can appear in case of bridges. Or there is something I have missed???
Look like Maxime is correct. I2C based DSI bridge will get probe during bridge_attach which usually called from bridge driver bridge_attach call. Non-I2C bridges and DSI panels will get probe during host.attach. We do get similar situation for dw-mipi-dsi bridges, where icn6211 bridge is not I2C-based bridge and it gets probed in host_attach and sn65dsi83 is I2C based bridge and it gets probed in bridge_attach.
Here is the simple call trace we have observed with dw-mipi-dsi bridge when all possible DSI device are trying to probe.
DSI panels and bridges will invoke the host attach from probe in order to find the panel_or_bridge.
chipone_probe() dw_mipi_dsi_host_attach().start dw_mipi_dsi_panel_or_bridge() ...found the panel_or_bridge...
ltdc_encoder_init().start dw_mipi_dsi_bridge_attach().start dw_mipi_dsi_host_attach().start chipone_attach(). start
chipone_attach(). done dw_mipi_dsi_host_attach().done dw_mipi_dsi_bridge_attach(). doneltdc_encoder_init().done
I2C based DSI bridge will invoke the drm_bridge_attach from bridge attach in order to find the panel_or_bridge.
ltdc_encoder_init().start dw_mipi_dsi_bridge_attach().start dw_mipi_dsi_panel_or_bridge() ...found the panel_or_bridge... dw_mipi_dsi_host_attach().start sn65dsi83_attach(). start
sn65dsi83_attach(). done dw_mipi_dsi_host_attach().done dw_mipi_dsi_bridge_attach(). doneltdc_encoder_init().done
It is correct that the I2C-based bridges will attach the host via mipi_dsi_attach in driver bridge API where as it done in probe for Non-I2C bridges and DSI panels.
The call order depends on the registration time of DSI host. In case of dw-mipi-dsi it is called from .component_bind callback (dsi_bind-> dsi_host_init -> mipi_dsi_host_register). And this is "the original sin" :)
dw-mipi-dsi calls component_add without prior acquiring its dependency - drm_bridge and before DSI host registration. In such situation bridge author should follow this pattern and perform similar initialization: first drm_bridge_add, then mipi_dsi_attach.
And now authors of bridges are in dead end in case they want their bridge/panel drivers cooperate with dw-mipi-dsi host (with pattern look for sink - bridge/panel, then register DSI bus) and with other DSI hosts (most of then use pattern - register DSI bus then look for the sink - panel or bridge).
Quick look at the DSI hosts suggests the pattern get-sink-then-register-bus are used only by kirin/dw_drm_dsi.c and msm/dsi.
All other DSI hosts uses apparently register-bus-then-get-sink pattern - as I said it was not profound analysis - just few greps of some keywords.
Anyway there are already eleven(?) bridge drivers using this pattern. I wonder if fixing it would be difficult, or if it expose other issues??? The patches should be quite straightforward - move of_find_mipi_dsi_host_by_node and mipi_dsi_device_register_full to probe time.
Finally I think that if we will not fix these bridge drivers we will encounter another set of issues with new platforms connecting "DSI host drivers assuming this pattern" and "i2c/dsi device drivers assuming pattern already present in the bridges".
Agreed, I'm trying to understand the several ways to fix this. Right now I'm trying this on sun6i_mipi_dsi and exynos_drm_dsi. Will let you know for any update and suggestions on the same.
Quick look at sun6i suggests it uses register-bus-then-get-sink pattern (incompatible with kirin), only issue is that currently it support only panel sink.
Exynos uses also register-bus-then-get-sink pattern, but it slightly different as it supports dynamic attach/detach of sinks.
Regards
Andrzej
Thanks, Jagan.
Hi Andrzej,
On Wed, Aug 04, 2021 at 04:09:38PM +0200, a.hajda wrote:
Hi Maxime,
I have been busy with other tasks, and I did not follow the list last time, so sorry for my late response.
On 28.07.2021 15:32, 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
I guess this is incorrect. I have promoted several times the pattern that device driver shouldn't expose its interfaces (for example component_add, drm_panel_add, drm_bridge_add) until it gathers all required dependencies. In this particular case bridges should defer probe until DSI bus becomes available. I guess this way the patch you reverts would work.
I advised few times this pattern in case of DSI hosts, apparently I didn't notice the similar issue can appear in case of bridges. Or there is something I have missed???
Anyway there are already eleven(?) bridge drivers using this pattern. I wonder if fixing it would be difficult, or if it expose other issues??? The patches should be quite straightforward - move of_find_mipi_dsi_host_by_node and mipi_dsi_device_register_full to probe time.
I gave this a try today, went back to the current upstream code and found that indeed it works. I converted two bridges that works now. I'll send a new version some time next week and will convert all the others if we agree on the approach.
Thanks for the suggestion!
Finally I think that if we will not fix these bridge drivers we will encounter another set of issues with new platforms connecting "DSI host drivers assuming this pattern" and "i2c/dsi device drivers assuming pattern already present in the bridges".
Yeah, this is exactly the situation I'm in :)
Maxime
dri-devel@lists.freedesktop.org
-
a.hajda -
Andrzej Hajda
-
Jagan Teki -
Maxime Ripard