[PATCH 00/10] drm/bridge: Make panel and bridge probe order consistent

[PATCH 00/10] drm/bridge: Make panel and bridge probe order consistent

[Maxime Ripard]

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

Maxime Ripard (10):
  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/bridge: panel: Call attach and detach for the panel
  drm/vc4: dsi: Switch to drm_of_get_next
  drm/panel: raspberrypi-touchscreen: Prevent double-free
  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                |   4 +
 drivers/gpu/drm/drm_bridge.c                  | 134 ++++++++++++++-
 drivers/gpu/drm/drm_of.c                      |   3 +
 drivers/gpu/drm/drm_panel.c                   |  20 +++
 .../drm/panel/panel-raspberrypi-touchscreen.c | 159 +++++++++---------
 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                       |   6 +
 10 files changed, 273 insertions(+), 122 deletions(-)

-- 
2.31.1

[PATCH 01/10] Revert "drm/vc4: dsi: Only register our component once a DSI device is attached"

[Maxime Ripard]

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;
 }
 
-- 
2.31.1

[PATCH 02/10] drm/bridge: Add a function to abstract away panels

[Maxime Ripard]

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 | 62 +++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/drm_of.c     |  3 ++
 include/drm/drm_bridge.h     |  2 ++
 3 files changed, 63 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 044acd07c153..aef8c9f4fb9f 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -24,10 +24,12 @@
 #include <linux/err.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/of_graph.h>
 
 #include <drm/drm_atomic_state_helper.h>
 #include <drm/drm_bridge.h>
 #include <drm/drm_encoder.h>
+#include <drm/drm_panel.h>
 
 #include "drm_crtc_internal.h"
 
@@ -50,10 +52,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().
+ * drm_of_get_next(). 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 +1223,60 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np)
 	return NULL;
 }
 EXPORT_SYMBOL(of_drm_find_bridge);
+
+/**
+ * devm_drm_of_get_next - 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_next(struct device *dev,
+					struct device_node *np,
+					unsigned int port,
+					unsigned int endpoint)
+{
+	struct device_node *remote;
+	struct drm_bridge *bridge;
+	struct drm_panel *panel;
+
+	/*
+	 * of_graph_get_remote_node() produces a noisy error message if port
+	 * node isn't found and the absence of the port is a legit case here,
+	 * so at first we silently check whether graph presents in the
+	 * device-tree node.
+	 */
+	if (!of_graph_is_present(np))
+		return ERR_PTR(-ENODEV);
+
+	remote = of_graph_get_remote_node(np, port, endpoint);
+	if (!remote)
+		return ERR_PTR(-ENODEV);
+
+	bridge = of_drm_find_bridge(remote);
+	if (bridge) {
+		of_node_put(remote);
+		return bridge;
+	}
+
+	panel = of_drm_find_panel(remote);
+	if (IS_ERR(panel)) {
+		of_node_put(remote);
+		return ERR_CAST(panel);
+	}
+
+	of_node_put(remote);
+
+	return devm_drm_panel_bridge_add(dev, panel);
+}
+EXPORT_SYMBOL(devm_drm_of_get_next);
 #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..bbbdc4d17ac9 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
+ * drm_of_get_next() 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..e16fafc6f37d 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_next(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
 
-- 
2.31.1

[PATCH 03/10] drm/bridge: Add documentation sections

[Maxime Ripard]

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 separare each bits.

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 aef8c9f4fb9f..c9a950bfdfe5 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -50,6 +50,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
  * drm_of_get_next(). Once acquired, the bridge shall be attached to the
@@ -84,11 +93,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);
-- 
2.31.1

[PATCH 04/10] drm/bridge: Document the probe issue with MIPI-DSI bridges

[Maxime Ripard]

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 c9a950bfdfe5..81f8dac12367 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -95,6 +95,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);
 
-- 
2.31.1

[PATCH 05/10] drm/panel: Create attach and detach callbacks

[Maxime Ripard]

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.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/drm_panel.c | 20 ++++++++++++++++++++
 include/drm/drm_panel.h     |  6 ++++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index f634371c717a..23bca798a2f3 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -223,6 +223,26 @@ int drm_panel_get_modes(struct drm_panel *panel,
 }
 EXPORT_SYMBOL(drm_panel_get_modes);
 
+int drm_panel_attach(struct drm_panel *panel)
+{
+	if (!panel)
+		return -EINVAL;
+
+	if (panel->funcs && panel->funcs->attach)
+		return panel->funcs->attach(panel);
+
+	return -EOPNOTSUPP;
+}
+
+void drm_panel_detach(struct drm_panel *panel)
+{
+	if (!panel)
+		return;
+
+	if (panel->funcs && panel->funcs->detach)
+		panel->funcs->detach(panel);
+}
+
 #ifdef CONFIG_OF
 /**
  * of_drm_find_panel - look up a panel using a device tree node
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
index 4602f833eb51..b9201d520754 100644
--- a/include/drm/drm_panel.h
+++ b/include/drm/drm_panel.h
@@ -68,6 +68,9 @@ enum drm_panel_orientation;
  * does not need to implement the functionality to enable/disable backlight.
  */
 struct drm_panel_funcs {
+	int (*attach)(struct drm_panel *panel);
+	void (*detach)(struct drm_panel *panel);
+
 	/**
 	 * @prepare:
 	 *
@@ -180,6 +183,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);
+void drm_panel_detach(struct drm_panel *panel);
+
 int drm_panel_prepare(struct drm_panel *panel);
 int drm_panel_unprepare(struct drm_panel *panel);
 
-- 
2.31.1

[PATCH 06/10] drm/bridge: panel: Call attach and detach for the panel

[Maxime Ripard]

Now that we have additional attach and detach hooks for panels, make
sure that the panel bridge driver calls them when relevant.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/bridge/panel.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index c916f4b8907e..c2249f3fd357 100644
--- a/drivers/gpu/drm/bridge/panel.c
+++ b/drivers/gpu/drm/bridge/panel.c
@@ -60,6 +60,8 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
 	struct drm_connector *connector = &panel_bridge->connector;
 	int ret;
 
+	drm_panel_attach(panel_bridge->panel);
+
 	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
 		return 0;
 
@@ -90,6 +92,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.
 	 *
-- 
2.31.1

[PATCH 07/10] drm/vc4: dsi: Switch to drm_of_get_next

[Maxime Ripard]

The new drm_of_get_next removes most of the boilerplate we have to deal
with. Let's switch to it.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 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..f51ce8db0f4e 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_next(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
-- 
2.31.1

[PATCH 08/10] drm/panel: raspberrypi-touchscreen: Prevent double-free

[Maxime Ripard]

The mipi_dsi_device allocated by mipi_dsi_device_register_full() is
already free'd on release.

Fixes: 2f733d6194bd ("drm/panel: Add support for the Raspberry Pi 7" Touchscreen.")
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
index 6f2ce3b81f47..462faae0f446 100644
--- a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
+++ b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
@@ -447,7 +447,6 @@ static int rpi_touchscreen_remove(struct i2c_client *i2c)
 	drm_panel_remove(&ts->base);
 
 	mipi_dsi_device_unregister(ts->dsi);
-	kfree(ts->dsi);
 
 	return 0;
 }
-- 
2.31.1

[PATCH 09/10] drm/panel: raspberrypi-touchscreen: Use the attach hook

[Maxime Ripard]

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 = {
-- 
2.31.1

[PATCH 10/10] drm/panel: raspberrypi-touchscreen: Remove MIPI-DSI driver

[Maxime Ripard]

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");
-- 
2.31.1

Re: [PATCH 04/10] drm/bridge: Document the probe issue with MIPI-DSI bridges

[Daniel Vetter]

On Tue, Jul 20, 2021 at 03:45:19PM +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.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

I still have this dream that eventually we resurrect a patch to add
device_link to bridges/panels (ideally automatically), to help with some
of the suspend/resume issues around here.

Will this make things worse?

I think it'd be really good to figure that out with some coding, since if
we have incompatible solution to handle probe issues vs suspend/resume
issues, we're screwed.

Atm the duct-tape is to carefully move things around between suspend and
suspend_early hooks (and resume and resume_late) and hope it all works ...
-Daniel

> ---
>  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 c9a950bfdfe5..81f8dac12367 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -95,6 +95,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);
>  
> -- 
> 2.31.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 04/10] drm/bridge: Document the probe issue with MIPI-DSI bridges

[Daniel Vetter]

On Wed, Jul 21, 2021 at 02:05:01PM +0200, Daniel Vetter wrote:
> On Tue, Jul 20, 2021 at 03:45:19PM +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.
> > 
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> 
> I still have this dream that eventually we resurrect a patch to add
> device_link to bridges/panels (ideally automatically), to help with some
> of the suspend/resume issues around here.
> 
> Will this make things worse?
> 
> I think it'd be really good to figure that out with some coding, since if
> we have incompatible solution to handle probe issues vs suspend/resume
> issues, we're screwed.
> 
> Atm the duct-tape is to carefully move things around between suspend and
> suspend_early hooks (and resume and resume_late) and hope it all works ...

Just remembered: The other reason for device links was module ordering
fun. Especially when you have parts managed as a component. Currently if
you unload a bridge driver then in some cases the drm_device doesn't
rebind.
-Daniel

> 
> > ---
> >  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 c9a950bfdfe5..81f8dac12367 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -95,6 +95,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);
> >  
> > -- 
> > 2.31.1
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 02/10] drm/bridge: Add a function to abstract away panels

[Jagan Teki]

Hi Maxime,

On Tue, Jul 20, 2021 at 7:15 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> 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.

Correct, but drm_of_find_panel_or_bridge is still required in some DSI
drivers where the panel pointer required separately.

>
> 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 | 62 +++++++++++++++++++++++++++++++++---
>  drivers/gpu/drm/drm_of.c     |  3 ++
>  include/drm/drm_bridge.h     |  2 ++
>  3 files changed, 63 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 044acd07c153..aef8c9f4fb9f 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -24,10 +24,12 @@
>  #include <linux/err.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> +#include <linux/of_graph.h>
>
>  #include <drm/drm_atomic_state_helper.h>
>  #include <drm/drm_bridge.h>
>  #include <drm/drm_encoder.h>
> +#include <drm/drm_panel.h>
>
>  #include "drm_crtc_internal.h"
>
> @@ -50,10 +52,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().
> + * drm_of_get_next(). 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 +1223,60 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np)
>         return NULL;
>  }
>  EXPORT_SYMBOL(of_drm_find_bridge);
> +
> +/**
> + * devm_drm_of_get_next - 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_next(struct device *dev,
> +                                       struct device_node *np,
> +                                       unsigned int port,
> +                                       unsigned int endpoint)
> +{
> +       struct device_node *remote;
> +       struct drm_bridge *bridge;
> +       struct drm_panel *panel;
> +
> +       /*
> +        * of_graph_get_remote_node() produces a noisy error message if port
> +        * node isn't found and the absence of the port is a legit case here,
> +        * so at first we silently check whether graph presents in the
> +        * device-tree node.
> +        */
> +       if (!of_graph_is_present(np))
> +               return ERR_PTR(-ENODEV);
> +
> +       remote = of_graph_get_remote_node(np, port, endpoint);
> +       if (!remote)
> +               return ERR_PTR(-ENODEV);
> +
> +       bridge = of_drm_find_bridge(remote);
> +       if (bridge) {
> +               of_node_put(remote);
> +               return bridge;
> +       }
> +
> +       panel = of_drm_find_panel(remote);
> +       if (IS_ERR(panel)) {
> +               of_node_put(remote);
> +               return ERR_CAST(panel);
> +       }
> +
> +       of_node_put(remote);
> +
> +       return devm_drm_panel_bridge_add(dev, panel);
> +}

This is ideally similar to drm_of_find_panel_or_bridge followed by
panel_bridge_add. Can we reuse drm_of_find_panel_or_bridge for now
till it deprecated?

Thanks,
Jagan.

Re: [PATCH 05/10] drm/panel: Create attach and detach callbacks

[Jagan Teki]

Hi Maxime,

On Tue, Jul 20, 2021 at 7:15 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> 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.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/gpu/drm/drm_panel.c | 20 ++++++++++++++++++++
>  include/drm/drm_panel.h     |  6 ++++++
>  2 files changed, 26 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> index f634371c717a..23bca798a2f3 100644
> --- a/drivers/gpu/drm/drm_panel.c
> +++ b/drivers/gpu/drm/drm_panel.c
> @@ -223,6 +223,26 @@ int drm_panel_get_modes(struct drm_panel *panel,
>  }
>  EXPORT_SYMBOL(drm_panel_get_modes);
>
> +int drm_panel_attach(struct drm_panel *panel)
> +{
> +       if (!panel)
> +               return -EINVAL;
> +
> +       if (panel->funcs && panel->funcs->attach)
> +               return panel->funcs->attach(panel);
> +
> +       return -EOPNOTSUPP;

Most of the panel drivers won't require panel attach/detach API's so
we need to handle those cases as well if we didn't already.

Thanks,
Jagan.

Re: [PATCH 08/10] drm/panel: raspberrypi-touchscreen: Prevent double-free

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 830 bytes --]

On Tue, Jul 20, 2021 at 07:19:40PM +0200, Sam Ravnborg wrote:
> Hi Maxime,
> On Tue, Jul 20, 2021 at 03:45:23PM +0200, Maxime Ripard wrote:
> > The mipi_dsi_device allocated by mipi_dsi_device_register_full() is
> > already free'd on release.
> > 
> > Fixes: 2f733d6194bd ("drm/panel: Add support for the Raspberry Pi 7" Touchscreen.")
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
>
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

Thanks, I applied it to drm-misc-fixes

> I did a quick audit (as using grep mostly) to see if other panels had
> the same bug, but did not find others.

Yeah, the RaspberryPi panel seems to be the only odd DSI panel not
controlled through DCS, and the other panels don't have to allocate the
mipi-dsi device anyway.

No bridge seems to have the issue though.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 04/10] drm/bridge: Document the probe issue with MIPI-DSI bridges

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1442 bytes --]

Hi Daniel,

On Wed, Jul 21, 2021 at 02:05:01PM +0200, Daniel Vetter wrote:
> On Tue, Jul 20, 2021 at 03:45:19PM +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.
> > 
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> 
> I still have this dream that eventually we resurrect a patch to add
> device_link to bridges/panels (ideally automatically), to help with some
> of the suspend/resume issues around here.
> 
> Will this make things worse?
> 
> I think it'd be really good to figure that out with some coding, since if
> we have incompatible solution to handle probe issues vs suspend/resume
> issues, we're screwed.
> 
> Atm the duct-tape is to carefully move things around between suspend and
> suspend_early hooks (and resume and resume_late) and hope it all works ...

My initial idea to fix this was indeed to use device links. I gave up
after a while since it doesn't look like there's a way to add a device
link before either the bridge or encoder probes.

Indeed the OF-Graph representation is device-specific, so it can't be
generic, and if you need to probe to add that link, well, it's already
too late for the probe ordering :)

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 04/10] drm/bridge: Document the probe issue with MIPI-DSI bridges

[Daniel Vetter]

On Mon, Jul 26, 2021 at 05:16:57PM +0200, Maxime Ripard wrote:
> Hi Daniel,
> 
> On Wed, Jul 21, 2021 at 02:05:01PM +0200, Daniel Vetter wrote:
> > On Tue, Jul 20, 2021 at 03:45:19PM +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.
> > > 
> > > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > 
> > I still have this dream that eventually we resurrect a patch to add
> > device_link to bridges/panels (ideally automatically), to help with some
> > of the suspend/resume issues around here.
> > 
> > Will this make things worse?
> > 
> > I think it'd be really good to figure that out with some coding, since if
> > we have incompatible solution to handle probe issues vs suspend/resume
> > issues, we're screwed.
> > 
> > Atm the duct-tape is to carefully move things around between suspend and
> > suspend_early hooks (and resume and resume_late) and hope it all works ...
> 
> My initial idea to fix this was indeed to use device links. I gave up
> after a while since it doesn't look like there's a way to add a device
> link before either the bridge or encoder probes.
> 
> Indeed the OF-Graph representation is device-specific, so it can't be
> generic, and if you need to probe to add that link, well, it's already
> too late for the probe ordering :)

But don't we still need the device_link for suspend/resume and module
reload? All very annoying indeed anyway.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 04/10] drm/bridge: Document the probe issue with MIPI-DSI bridges

[Jagan Teki]

On Tue, Jul 20, 2021 at 7:15 PM Maxime Ripard <maxime@cerno.tech> 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.
>
> 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 c9a950bfdfe5..81f8dac12367 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -95,6 +95,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.

There is an another problem occur for this scenario in the case of kms
hotplug driver, sun6i_mipi_dsi.c. When host attach wait till drm
device pointer found and drm device pointer would found only when bind
done, and bind would complete only when &drm_bridge_funcs.attach hooks
are complete. But, If DSI driver is fully bridge driven then this
attach in bind will trigger panel_bridge hook attach and at this point
we cannot get panel_bridge at all which indeed second attach would
would failed.

This is one of the reason I'm trying to use drm_bridge_attach host
attach itself instead of component bind, not yet succeeded.

Thanks,
Jagan.

Re: [PATCH 04/10] drm/bridge: Document the probe issue with MIPI-DSI bridges

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2039 bytes --]

Hi,

On Tue, Jul 27, 2021 at 11:20:54AM +0200, Daniel Vetter wrote:
> On Mon, Jul 26, 2021 at 05:16:57PM +0200, Maxime Ripard wrote:
> > Hi Daniel,
> > 
> > On Wed, Jul 21, 2021 at 02:05:01PM +0200, Daniel Vetter wrote:
> > > On Tue, Jul 20, 2021 at 03:45:19PM +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.
> > > > 
> > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > > 
> > > I still have this dream that eventually we resurrect a patch to add
> > > device_link to bridges/panels (ideally automatically), to help with some
> > > of the suspend/resume issues around here.
> > > 
> > > Will this make things worse?
> > > 
> > > I think it'd be really good to figure that out with some coding, since if
> > > we have incompatible solution to handle probe issues vs suspend/resume
> > > issues, we're screwed.
> > > 
> > > Atm the duct-tape is to carefully move things around between suspend and
> > > suspend_early hooks (and resume and resume_late) and hope it all works ...
> > 
> > My initial idea to fix this was indeed to use device links. I gave up
> > after a while since it doesn't look like there's a way to add a device
> > link before either the bridge or encoder probes.
> > 
> > Indeed the OF-Graph representation is device-specific, so it can't be
> > generic, and if you need to probe to add that link, well, it's already
> > too late for the probe ordering :)
> 
> But don't we still need the device_link for suspend/resume and module
> reload? All very annoying indeed anyway.

I guess we would still need it for proper suspend and resume ordering
(but I never really worked on that part, so I'm not sure), but it's a
bit orthogonal to the issue here since those can be added after probe

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 04/10] drm/bridge: Document the probe issue with MIPI-DSI bridges

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 5571 bytes --]

Hi Jagan,

On Tue, Jul 27, 2021 at 03:12:09PM +0530, Jagan Teki wrote:
> On Tue, Jul 20, 2021 at 7:15 PM Maxime Ripard <maxime@cerno.tech> 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.
> >
> > 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 c9a950bfdfe5..81f8dac12367 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -95,6 +95,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.
> 
> There is an another problem occur for this scenario in the case of kms
> hotplug driver, sun6i_mipi_dsi.c. When host attach wait till drm
> device pointer found and drm device pointer would found only when bind
> done, and bind would complete only when &drm_bridge_funcs.attach hooks
> are complete. But, If DSI driver is fully bridge driven then this
> attach in bind will trigger panel_bridge hook attach and at this point
> we cannot get panel_bridge at all which indeed second attach would
> would failed.
> 
> This is one of the reason I'm trying to use drm_bridge_attach host
> attach itself instead of component bind, not yet succeeded.

I'm not really sure what you mean, but if you mention the code we have
in the DSI driver to make sure we can probe without our panel, then it's
not something that we really can support. Bridges cannot be hotplugged
in DRM and having some inconsistencies between drivers (since none of
them behave the same way there) and between what's plugged on the other
side of the DSI bus feels weird.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 04/10] drm/bridge: Document the probe issue with MIPI-DSI bridges

[Jagan Teki]

Hi Maxime,

On Wed, Jul 28, 2021 at 7:05 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi Jagan,
>
> On Tue, Jul 27, 2021 at 03:12:09PM +0530, Jagan Teki wrote:
> > On Tue, Jul 20, 2021 at 7:15 PM Maxime Ripard <maxime@cerno.tech> 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.
> > >
> > > 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 c9a950bfdfe5..81f8dac12367 100644
> > > --- a/drivers/gpu/drm/drm_bridge.c
> > > +++ b/drivers/gpu/drm/drm_bridge.c
> > > @@ -95,6 +95,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.
> >
> > There is an another problem occur for this scenario in the case of kms
> > hotplug driver, sun6i_mipi_dsi.c. When host attach wait till drm
> > device pointer found and drm device pointer would found only when bind
> > done, and bind would complete only when &drm_bridge_funcs.attach hooks
> > are complete. But, If DSI driver is fully bridge driven then this
> > attach in bind will trigger panel_bridge hook attach and at this point
> > we cannot get panel_bridge at all which indeed second attach would
> > would failed.
> >
> > This is one of the reason I'm trying to use drm_bridge_attach host
> > attach itself instead of component bind, not yet succeeded.
>
> I'm not really sure what you mean, but if you mention the code we have
> in the DSI driver to make sure we can probe without our panel, then it's
> not something that we really can support. Bridges cannot be hotplugged
> in DRM and having some inconsistencies between drivers (since none of
> them behave the same way there) and between what's plugged on the other
> side of the DSI bus feels weird.

Yes, but for associated bridges to attach on component base DSI
drivers the panel_bridge or bridge pointer look necessary.

Here is the pseudo code for sun6i_mipi_dsi which support the DSI probe
without panel, by waiting for drm pointer to found, but the same seems
not possible for bridge cases.

static int sun6i_dsi_bridge_attach(struct drm_bridge *bridge,
                                   enum drm_bridge_attach_flags flags)
{
        return drm_bridge_attach(bridge->encoder, dsi->bridge, bridge, flags);
}

static int sun6i_dsi_attach(struct mipi_dsi_host *host,
                            struct mipi_dsi_device *device)
{
   ...
   if (!dsi->drm || !dsi->drm->registered)
                return -EPROBE_DEFER;

   panel = of_drm_find_panel(device->dev.of_node);
        if (IS_ERR(panel)) {
                panel = NULL;

                bridge = of_drm_find_bridge(device->dev.of_node);
                if (IS_ERR(bridge)) {
                        dev_err(dsi->dev, "failed to find bridge\n");
                        return PTR_ERR(bridge);
                }
        } else {
                bridge = NULL;
    }

    dsi->panel = panel;
    dsi->bridge = bridge;
    ....
}

static int sun6i_dsi_bind(struct device *dev, struct device *master,
                         void *data)
{
   ...
   ret = drm_bridge_attach(&dsi->encoder, dsi->bridge, NULL, 0);
   ..
   dsi->drm = drm;
}

I believe some-sort bridge handling in hotpulg would necessary to keep
the hotplug probing happens for bridge pointers as well.

Thanks,
Jagan.