[PATCH v3 0/8] drm/bridge: Make panel and bridge probe order consistent

[PATCH v3 0/8] 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.

The best practice to avoid those issues is to register its functions only after
all its dependencies are live. We also shouldn't wait any longer than we should
to play nice with the other components that are waiting for us, so in our case
that would mean moving the DSI device registration to the bridge probe.

If the general approach is agreed upon, other bridge drivers will obviously be
converted.

Let me know what you think,
Maxime

---

Changes from v2:
  - Changed the approach as suggested by Andrzej, and aligned the bridge on the
    panel this time.
  - Fixed some typos

Changes from v1:
  - Change the name of drm_of_get_next function to drm_of_get_bridge
  - Mention the revert of 87154ff86bf6 and squash the two patches that were
    reverting that commit
  - Add some documentation
  - Make drm_panel_attach and _detach succeed when no callback is there

Maxime Ripard (8):
  drm/bridge: Add documentation sections
  drm/bridge: Document the probe issue with MIPI-DSI bridges
  drm/mipi-dsi: Create devm device registration
  drm/mipi-dsi: Create devm device attachment
  drm/bridge: ps8640: Switch to devm MIPI-DSI helpers
  drm/bridge: ps8640: Register and attach our DSI device at probe
  drm/bridge: sn65dsi83: Switch to devm MIPI-DSI helpers
  drm/bridge: sn65dsi83: Register and attach our DSI device at probe

 Documentation/gpu/drm-kms-helpers.rst  | 12 ++++
 drivers/gpu/drm/bridge/parade-ps8640.c | 97 ++++++++++++++------------
 drivers/gpu/drm/bridge/ti-sn65dsi83.c  | 82 +++++++++++-----------
 drivers/gpu/drm/drm_bridge.c           | 70 +++++++++++++++++--
 drivers/gpu/drm/drm_mipi_dsi.c         | 81 +++++++++++++++++++++
 include/drm/drm_mipi_dsi.h             |  4 ++
 6 files changed, 256 insertions(+), 90 deletions(-)

-- 
2.31.1

[PATCH v3 1/8] 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 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 a8ed66751c2d..baff74ea4a33 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -49,6 +49,15 @@
  * Chaining multiple bridges to the output of a bridge, or the same bridge to
  * the output of different bridges, is not supported.
  *
+ * &drm_bridge, like &drm_panel, aren't &drm_mode_object entities like planes,
+ * CRTCs, encoders or connectors and hence are not visible to userspace. They
+ * just provide additional hooks to get the desired output at the end of the
+ * encoder chain.
+ */
+
+/**
+ * DOC:	display driver integration
+ *
  * Display drivers are responsible for linking encoders with the first bridge
  * in the chains. This is done by acquiring the appropriate bridge with
  * of_drm_find_bridge() or drm_of_find_panel_or_bridge(), or creating it for a
@@ -85,11 +94,6 @@
  * helper to create the &drm_connector, or implement it manually on top of the
  * connector-related operations exposed by the bridge (see the overview
  * documentation of bridge operations for more details).
- *
- * &drm_bridge, like &drm_panel, aren't &drm_mode_object entities like planes,
- * CRTCs, encoders or connectors and hence are not visible to userspace. They
- * just provide additional hooks to get the desired output at the end of the
- * encoder chain.
  */
 
 static DEFINE_MUTEX(bridge_lock);
-- 
2.31.1

[PATCH v3 2/8] 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          | 58 +++++++++++++++++++++++++++
 2 files changed, 64 insertions(+)

diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
index 10f8df7aecc0..ec2f65b31930 100644
--- a/Documentation/gpu/drm-kms-helpers.rst
+++ b/Documentation/gpu/drm-kms-helpers.rst
@@ -157,6 +157,12 @@ Display Driver Integration
 .. kernel-doc:: drivers/gpu/drm/drm_bridge.c
    :doc: display driver integration
 
+Special Care with MIPI-DSI bridges
+----------------------------------
+
+.. kernel-doc:: drivers/gpu/drm/drm_bridge.c
+   :doc: special care dsi
+
 Bridge Operations
 -----------------
 
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index baff74ea4a33..794654233cf5 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -96,6 +96,64 @@
  * 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 try to find its MIPI-DSI
+ *   host, register as a MIPI-DSI device and attach the MIPI-DSI device
+ *   to its host. The bridge driver is now functional.
+ *
+ * - In its &struct mipi_dsi_host_ops.attach hook, the display driver
+ *   can now add its component. Its bind hook will now be called and
+ *   since the bridge driver is attached and registered, we can now look
+ *   for and attach it.
+ *
+ * At this point, we're now certain that both the 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 v3 3/8] drm/mipi-dsi: Create devm device registration

[Maxime Ripard]

Devices that take their data through the MIPI-DSI bus but are controlled
through a secondary bus like I2C have to register a secondary device on
the MIPI-DSI bus through the mipi_dsi_device_register_full() function.

At removal or when an error occurs, that device needs to be removed
through a call to mipi_dsi_device_unregister().

Let's create a device-managed variant of the registration function that
will automatically unregister the device at unbind.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/drm_mipi_dsi.c | 46 ++++++++++++++++++++++++++++++++++
 include/drm/drm_mipi_dsi.h     |  3 +++
 2 files changed, 49 insertions(+)

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index 5dd475e82995..ddf67463eaa1 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -246,6 +246,52 @@ void mipi_dsi_device_unregister(struct mipi_dsi_device *dsi)
 }
 EXPORT_SYMBOL(mipi_dsi_device_unregister);
 
+static void devm_mipi_dsi_device_unregister(void *arg)
+{
+	struct mipi_dsi_device *dsi = arg;
+
+	mipi_dsi_device_unregister(dsi);
+}
+
+/**
+ * devm_mipi_dsi_device_register_full - create a managed MIPI DSI device
+ * @dev: device to tie the MIPI-DSI device lifetime to
+ * @host: DSI host to which this device is connected
+ * @info: pointer to template containing DSI device information
+ *
+ * Create a MIPI DSI device by using the device information provided by
+ * mipi_dsi_device_info template
+ *
+ * This is the managed version of mipi_dsi_device_register_full() which
+ * automatically calls mipi_dsi_device_unregister() when @dev is
+ * unbound.
+ *
+ * Returns:
+ * A pointer to the newly created MIPI DSI device, or, a pointer encoded
+ * with an error
+ */
+struct mipi_dsi_device *
+devm_mipi_dsi_device_register_full(struct device *dev,
+				   struct mipi_dsi_host *host,
+				   const struct mipi_dsi_device_info *info)
+{
+	struct mipi_dsi_device *dsi;
+	int ret;
+
+	dsi = mipi_dsi_device_register_full(host, info);
+	if (IS_ERR(dsi))
+		return dsi;
+
+	ret = devm_add_action_or_reset(dev,
+				       devm_mipi_dsi_device_unregister,
+				       dsi);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return dsi;
+}
+EXPORT_SYMBOL_GPL(devm_mipi_dsi_device_register_full);
+
 static DEFINE_MUTEX(host_lock);
 static LIST_HEAD(host_list);
 
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index af7ba8071eb0..d0032e435e08 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -227,6 +227,9 @@ struct mipi_dsi_device *
 mipi_dsi_device_register_full(struct mipi_dsi_host *host,
 			      const struct mipi_dsi_device_info *info);
 void mipi_dsi_device_unregister(struct mipi_dsi_device *dsi);
+struct mipi_dsi_device *
+devm_mipi_dsi_device_register_full(struct device *dev, struct mipi_dsi_host *host,
+				   const struct mipi_dsi_device_info *info);
 struct mipi_dsi_device *of_find_mipi_dsi_device_by_node(struct device_node *np);
 int mipi_dsi_attach(struct mipi_dsi_device *dsi);
 int mipi_dsi_detach(struct mipi_dsi_device *dsi);
-- 
2.31.1

[PATCH v3 4/8] drm/mipi-dsi: Create devm device attachment

[Maxime Ripard]

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/drm_mipi_dsi.c | 35 ++++++++++++++++++++++++++++++++++
 include/drm/drm_mipi_dsi.h     |  1 +
 2 files changed, 36 insertions(+)

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index ddf67463eaa1..18cef04df2f2 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -391,6 +391,41 @@ int mipi_dsi_detach(struct mipi_dsi_device *dsi)
 }
 EXPORT_SYMBOL(mipi_dsi_detach);
 
+static void devm_mipi_dsi_detach(void *arg)
+{
+	struct mipi_dsi_device *dsi = arg;
+
+	mipi_dsi_detach(dsi);
+}
+
+/**
+ * devm_mipi_dsi_attach - Attach a MIPI-DSI device to its DSI Host
+ * @dev: device to tie the MIPI-DSI device attachment lifetime to
+ * @dsi: DSI peripheral
+ *
+ * This is the managed version of mipi_dsi_attach() which automatically
+ * calls mipi_dsi_detach() when @dev is unbound.
+ *
+ * Returns:
+ * 0 on success, a negative error code on failure.
+ */
+int devm_mipi_dsi_attach(struct device *dev,
+			 struct mipi_dsi_device *dsi)
+{
+	int ret;
+
+	ret = mipi_dsi_attach(dsi);
+	if (ret)
+		return ret;
+
+	ret = devm_add_action_or_reset(dev, devm_mipi_dsi_detach, dsi);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devm_mipi_dsi_attach);
+
 static ssize_t mipi_dsi_device_transfer(struct mipi_dsi_device *dsi,
 					struct mipi_dsi_msg *msg)
 {
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index d0032e435e08..147e51b6d241 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -233,6 +233,7 @@ devm_mipi_dsi_device_register_full(struct device *dev, struct mipi_dsi_host *hos
 struct mipi_dsi_device *of_find_mipi_dsi_device_by_node(struct device_node *np);
 int mipi_dsi_attach(struct mipi_dsi_device *dsi);
 int mipi_dsi_detach(struct mipi_dsi_device *dsi);
+int devm_mipi_dsi_attach(struct device *dev, struct mipi_dsi_device *dsi);
 int mipi_dsi_shutdown_peripheral(struct mipi_dsi_device *dsi);
 int mipi_dsi_turn_on_peripheral(struct mipi_dsi_device *dsi);
 int mipi_dsi_set_maximum_return_packet_size(struct mipi_dsi_device *dsi,
-- 
2.31.1

[PATCH v3 5/8] drm/bridge: ps8640: Switch to devm MIPI-DSI helpers

[Maxime Ripard]

Let's switch to the new devm MIPI-DSI function to register and attach
our secondary device. This also avoids leaking the device on removal.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/bridge/parade-ps8640.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index 7bd0affa057a..794c9516b05d 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -241,7 +241,7 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge,
 	if (!host)
 		return -ENODEV;
 
-	dsi = mipi_dsi_device_register_full(host, &info);
+	dsi = devm_mipi_dsi_device_register_full(dev, host, &info);
 	if (IS_ERR(dsi)) {
 		dev_err(dev, "failed to create dsi device\n");
 		ret = PTR_ERR(dsi);
@@ -255,17 +255,13 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge,
 			  MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
 	dsi->format = MIPI_DSI_FMT_RGB888;
 	dsi->lanes = DP_NUM_LANES;
-	ret = mipi_dsi_attach(dsi);
+	ret = devm_mipi_dsi_attach(dev, dsi);
 	if (ret)
-		goto err_dsi_attach;
+		return ret;
 
 	/* Attach the panel-bridge to the dsi bridge */
 	return drm_bridge_attach(bridge->encoder, ps_bridge->panel_bridge,
 				 &ps_bridge->bridge, flags);
-
-err_dsi_attach:
-	mipi_dsi_device_unregister(dsi);
-	return ret;
 }
 
 static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
-- 
2.31.1

[PATCH v3 6/8] drm/bridge: ps8640: Register and attach our DSI device at probe

[Maxime Ripard]

In order to avoid any probe ordering issue, the best practice is to move
the secondary MIPI-DSI device registration and attachment to the
MIPI-DSI host at probe time. Let's do this.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/bridge/parade-ps8640.c | 93 ++++++++++++++------------
 1 file changed, 51 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index 794c9516b05d..37f7d166a3c6 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -213,52 +213,10 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge,
 				enum drm_bridge_attach_flags flags)
 {
 	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
-	struct device *dev = &ps_bridge->page[0]->dev;
-	struct device_node *in_ep, *dsi_node;
-	struct mipi_dsi_device *dsi;
-	struct mipi_dsi_host *host;
-	int ret;
-	const struct mipi_dsi_device_info info = { .type = "ps8640",
-						   .channel = 0,
-						   .node = NULL,
-						 };
 
 	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
 		return -EINVAL;
 
-	/* port@0 is ps8640 dsi input port */
-	in_ep = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);
-	if (!in_ep)
-		return -ENODEV;
-
-	dsi_node = of_graph_get_remote_port_parent(in_ep);
-	of_node_put(in_ep);
-	if (!dsi_node)
-		return -ENODEV;
-
-	host = of_find_mipi_dsi_host_by_node(dsi_node);
-	of_node_put(dsi_node);
-	if (!host)
-		return -ENODEV;
-
-	dsi = devm_mipi_dsi_device_register_full(dev, host, &info);
-	if (IS_ERR(dsi)) {
-		dev_err(dev, "failed to create dsi device\n");
-		ret = PTR_ERR(dsi);
-		return ret;
-	}
-
-	ps_bridge->dsi = dsi;
-
-	dsi->host = host;
-	dsi->mode_flags = MIPI_DSI_MODE_VIDEO |
-			  MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
-	dsi->format = MIPI_DSI_FMT_RGB888;
-	dsi->lanes = DP_NUM_LANES;
-	ret = devm_mipi_dsi_attach(dev, dsi);
-	if (ret)
-		return ret;
-
 	/* Attach the panel-bridge to the dsi bridge */
 	return drm_bridge_attach(bridge->encoder, ps_bridge->panel_bridge,
 				 &ps_bridge->bridge, flags);
@@ -305,6 +263,53 @@ static const struct drm_bridge_funcs ps8640_bridge_funcs = {
 	.pre_enable = ps8640_pre_enable,
 };
 
+static int ps8640_bridge_host_attach(struct device *dev, struct ps8640 *ps_bridge)
+{
+	struct device_node *in_ep, *dsi_node;
+	struct mipi_dsi_device *dsi;
+	struct mipi_dsi_host *host;
+	int ret;
+	const struct mipi_dsi_device_info info = { .type = "ps8640",
+						   .channel = 0,
+						   .node = NULL,
+						 };
+
+	/* port@0 is ps8640 dsi input port */
+	in_ep = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);
+	if (!in_ep)
+		return -ENODEV;
+
+	dsi_node = of_graph_get_remote_port_parent(in_ep);
+	of_node_put(in_ep);
+	if (!dsi_node)
+		return -ENODEV;
+
+	host = of_find_mipi_dsi_host_by_node(dsi_node);
+	of_node_put(dsi_node);
+	if (!host)
+		return -EPROBE_DEFER;
+
+	dsi = devm_mipi_dsi_device_register_full(dev, host, &info);
+	if (IS_ERR(dsi)) {
+		dev_err(dev, "failed to create dsi device\n");
+		return PTR_ERR(dsi);
+	}
+
+	ps_bridge->dsi = dsi;
+
+	dsi->host = host;
+	dsi->mode_flags = MIPI_DSI_MODE_VIDEO |
+			  MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
+	dsi->format = MIPI_DSI_FMT_RGB888;
+	dsi->lanes = DP_NUM_LANES;
+
+	ret = devm_mipi_dsi_attach(dev, dsi);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static int ps8640_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
@@ -371,6 +376,10 @@ static int ps8640_probe(struct i2c_client *client)
 
 	drm_bridge_add(&ps_bridge->bridge);
 
+	ret = ps8640_bridge_host_attach(dev, ps_bridge);
+	if (ret)
+		return ret;
+
 	return 0;
 }
 
-- 
2.31.1

[PATCH v3 7/8] drm/bridge: sn65dsi83: Switch to devm MIPI-DSI helpers

[Maxime Ripard]

Let's switch to the new devm MIPI-DSI function to register and attach
our secondary device. This also avoids leaking the device when we detach
the bridge but don't remove its driver.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/bridge/ti-sn65dsi83.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index a32f70bc68ea..db4d39082705 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -262,7 +262,7 @@ static int sn65dsi83_attach(struct drm_bridge *bridge,
 		return -EPROBE_DEFER;
 	}
 
-	dsi = mipi_dsi_device_register_full(host, &info);
+	dsi = devm_mipi_dsi_device_register_full(dev, host, &info);
 	if (IS_ERR(dsi)) {
 		return dev_err_probe(dev, PTR_ERR(dsi),
 				     "failed to create dsi device\n");
@@ -274,18 +274,14 @@ static int sn65dsi83_attach(struct drm_bridge *bridge,
 	dsi->format = MIPI_DSI_FMT_RGB888;
 	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST;
 
-	ret = mipi_dsi_attach(dsi);
+	ret = devm_mipi_dsi_attach(dev, dsi);
 	if (ret < 0) {
 		dev_err(dev, "failed to attach dsi to host\n");
-		goto err_dsi_attach;
+		return ret;
 	}
 
 	return drm_bridge_attach(bridge->encoder, ctx->panel_bridge,
 				 &ctx->bridge, flags);
-
-err_dsi_attach:
-	mipi_dsi_device_unregister(dsi);
-	return ret;
 }
 
 static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
@@ -697,8 +693,6 @@ static int sn65dsi83_remove(struct i2c_client *client)
 {
 	struct sn65dsi83 *ctx = i2c_get_clientdata(client);
 
-	mipi_dsi_detach(ctx->dsi);
-	mipi_dsi_device_unregister(ctx->dsi);
 	drm_bridge_remove(&ctx->bridge);
 	of_node_put(ctx->host_node);
 
-- 
2.31.1

[PATCH v3 8/8] drm/bridge: sn65dsi83: Register and attach our DSI device at probe

[Maxime Ripard]

In order to avoid any probe ordering issue, the best practice is to move
the secondary MIPI-DSI device registration and attachment to the
MIPI-DSI host at probe time. Let's do this.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/bridge/ti-sn65dsi83.c | 76 +++++++++++++++------------
 1 file changed, 42 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index db4d39082705..3a75224b7348 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -245,40 +245,6 @@ static int sn65dsi83_attach(struct drm_bridge *bridge,
 			    enum drm_bridge_attach_flags flags)
 {
 	struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
-	struct device *dev = ctx->dev;
-	struct mipi_dsi_device *dsi;
-	struct mipi_dsi_host *host;
-	int ret = 0;
-
-	const struct mipi_dsi_device_info info = {
-		.type = "sn65dsi83",
-		.channel = 0,
-		.node = NULL,
-	};
-
-	host = of_find_mipi_dsi_host_by_node(ctx->host_node);
-	if (!host) {
-		dev_err(dev, "failed to find dsi host\n");
-		return -EPROBE_DEFER;
-	}
-
-	dsi = devm_mipi_dsi_device_register_full(dev, host, &info);
-	if (IS_ERR(dsi)) {
-		return dev_err_probe(dev, PTR_ERR(dsi),
-				     "failed to create dsi device\n");
-	}
-
-	ctx->dsi = dsi;
-
-	dsi->lanes = ctx->dsi_lanes;
-	dsi->format = MIPI_DSI_FMT_RGB888;
-	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST;
-
-	ret = devm_mipi_dsi_attach(dev, dsi);
-	if (ret < 0) {
-		dev_err(dev, "failed to attach dsi to host\n");
-		return ret;
-	}
 
 	return drm_bridge_attach(bridge->encoder, ctx->panel_bridge,
 				 &ctx->bridge, flags);
@@ -646,6 +612,44 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model)
 	return 0;
 }
 
+static int sn65dsi83_host_attach(struct sn65dsi83 *ctx)
+{
+	struct device *dev = ctx->dev;
+	struct mipi_dsi_device *dsi;
+	struct mipi_dsi_host *host;
+	const struct mipi_dsi_device_info info = {
+		.type = "sn65dsi83",
+		.channel = 0,
+		.node = NULL,
+	};
+	int ret;
+
+	host = of_find_mipi_dsi_host_by_node(ctx->host_node);
+	if (!host) {
+		dev_err(dev, "failed to find dsi host\n");
+		return -EPROBE_DEFER;
+	}
+
+	dsi = devm_mipi_dsi_device_register_full(dev, host, &info);
+	if (IS_ERR(dsi))
+		return dev_err_probe(dev, PTR_ERR(dsi),
+				     "failed to create dsi device\n");
+
+	ctx->dsi = dsi;
+
+	dsi->lanes = ctx->dsi_lanes;
+	dsi->format = MIPI_DSI_FMT_RGB888;
+	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST;
+
+	ret = devm_mipi_dsi_attach(dev, dsi);
+	if (ret < 0) {
+		dev_err(dev, "failed to attach dsi to host: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
 static int sn65dsi83_probe(struct i2c_client *client,
 			   const struct i2c_device_id *id)
 {
@@ -686,6 +690,10 @@ static int sn65dsi83_probe(struct i2c_client *client,
 	ctx->bridge.of_node = dev->of_node;
 	drm_bridge_add(&ctx->bridge);
 
+	ret = sn65dsi83_host_attach(ctx);
+	if (ret)
+		return ret;
+
 	return 0;
 }
 
-- 
2.31.1

Re: [PATCH v3 2/8] drm/bridge: Document the probe issue with MIPI-DSI bridges

[Andrzej Hajda]

Hi Maxime,

On 23.08.2021 10:47, 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>
> ---
>   Documentation/gpu/drm-kms-helpers.rst |  6 +++
>   drivers/gpu/drm/drm_bridge.c          | 58 +++++++++++++++++++++++++++
>   2 files changed, 64 insertions(+)
>
> diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
> index 10f8df7aecc0..ec2f65b31930 100644
> --- a/Documentation/gpu/drm-kms-helpers.rst
> +++ b/Documentation/gpu/drm-kms-helpers.rst
> @@ -157,6 +157,12 @@ Display Driver Integration
>   .. kernel-doc:: drivers/gpu/drm/drm_bridge.c
>      :doc: display driver integration
>   
> +Special Care with MIPI-DSI bridges
> +----------------------------------
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_bridge.c
> +   :doc: special care dsi
> +
>   Bridge Operations
>   -----------------
>   
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index baff74ea4a33..794654233cf5 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -96,6 +96,64 @@
>    * 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.


I guess component_add is leftover from previous iteration (as you wrote 
few lines below) component_add should be called from dsi host attach 
callback.


> + *
> + * - In its probe hook, the bridge driver must try to find its MIPI-DSI
> + *   host, register as a MIPI-DSI device and attach the MIPI-DSI device
> + *   to its host. The bridge driver is now functional.
> + *
> + * - In its &struct mipi_dsi_host_ops.attach hook, the display driver
> + *   can now add its component. Its bind hook will now be called and
> + *   since the bridge driver is attached and registered, we can now look
> + *   for and attach it.
> + *
> + * At this point, we're now certain that both the display driver and the
> + * bridge driver are functional and we can't have a deadlock-like
> + * situation when probing.
> + */
> +


Beside small mistake the whole patch looks OK for me. Maybe it would be 
worth to mention what is the real cause of this "special DSI case" - 
there is mutual dependency between two following entities in display chain:

1. display driver - it provides DSI bus, and requires drm_bridge or 
drm_panel provided by child device.

2. bridge or panel with DSI transport - it requires DSI bus provided by 
display driver, and provides drm_bridge or drm_panel interface required 
by display driver.

I guess similar issues can appear with other data/control bus-es, 
apparently DSI case is the most common.


And one more thing - you use "display driver" term but this is also case 
of any bridge providing DSI bus - there are already 3 such bridges in 
kernel - cdns, nwl, synopsys, tc358768, maybe "DSI host" would be better 
term.

And another thing - downstream device can be bridge or *panel*, it would 
be good to mention that panels also should follow this pattern.

Btw this is another place where word bridge can be 1:1 replaced by word 
panel - it clearly suggest that DRM subsystem waits for brave men who 
proposes patches unifying them, we would save lot of words, and lines of 
code if we could use drm_sink instead of "if (sink is bridge) do sth 
else do sth-similar-but-with-drm_panel-interface".


Regards

Andrzej


>   static DEFINE_MUTEX(bridge_lock);
>   static LIST_HEAD(bridge_list);
>   

Re: [PATCH v3 1/8] drm/bridge: Add documentation sections

[Andrzej Hajda]

W dniu 23.08.2021 o 10:47, Maxime Ripard pisze:
> 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>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

Regards
Andrzej

Re: [PATCH v3 3/8] drm/mipi-dsi: Create devm device registration

[Andrzej Hajda]

W dniu 23.08.2021 o 10:47, Maxime Ripard pisze:
> Devices that take their data through the MIPI-DSI bus but are controlled
> through a secondary bus like I2C have to register a secondary device on
> the MIPI-DSI bus through the mipi_dsi_device_register_full() function.
>
> At removal or when an error occurs, that device needs to be removed
> through a call to mipi_dsi_device_unregister().
>
> Let's create a device-managed variant of the registration function that
> will automatically unregister the device at unbind.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

Regards
Andrzej

Re: [PATCH v3 4/8] drm/mipi-dsi: Create devm device attachment

[Andrzej Hajda]

W dniu 23.08.2021 o 10:47, Maxime Ripard pisze:
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>


Missing description.

With this fixed:

Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

Regards
Andrzej

Re: [PATCH v3 6/8] drm/bridge: ps8640: Register and attach our DSI device at probe

[Andrzej Hajda]

W dniu 23.08.2021 o 10:47, Maxime Ripard pisze:
> In order to avoid any probe ordering issue, the best practice is to move
> the secondary MIPI-DSI device registration and attachment to the
> MIPI-DSI host at probe time. Let's do this.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>   drivers/gpu/drm/bridge/parade-ps8640.c | 93 ++++++++++++++------------
>   1 file changed, 51 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index 794c9516b05d..37f7d166a3c6 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -213,52 +213,10 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge,
>   				enum drm_bridge_attach_flags flags)
>   {
>   	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
> -	struct device *dev = &ps_bridge->page[0]->dev;
> -	struct device_node *in_ep, *dsi_node;
> -	struct mipi_dsi_device *dsi;
> -	struct mipi_dsi_host *host;
> -	int ret;
> -	const struct mipi_dsi_device_info info = { .type = "ps8640",
> -						   .channel = 0,
> -						   .node = NULL,
> -						 };
>   
>   	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
>   		return -EINVAL;
>   
> -	/* port@0 is ps8640 dsi input port */
> -	in_ep = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);
> -	if (!in_ep)
> -		return -ENODEV;
> -
> -	dsi_node = of_graph_get_remote_port_parent(in_ep);
> -	of_node_put(in_ep);
> -	if (!dsi_node)
> -		return -ENODEV;
> -
> -	host = of_find_mipi_dsi_host_by_node(dsi_node);
> -	of_node_put(dsi_node);
> -	if (!host)
> -		return -ENODEV;
> -
> -	dsi = devm_mipi_dsi_device_register_full(dev, host, &info);
> -	if (IS_ERR(dsi)) {
> -		dev_err(dev, "failed to create dsi device\n");
> -		ret = PTR_ERR(dsi);
> -		return ret;
> -	}
> -
> -	ps_bridge->dsi = dsi;
> -
> -	dsi->host = host;
> -	dsi->mode_flags = MIPI_DSI_MODE_VIDEO |
> -			  MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
> -	dsi->format = MIPI_DSI_FMT_RGB888;
> -	dsi->lanes = DP_NUM_LANES;
> -	ret = devm_mipi_dsi_attach(dev, dsi);
> -	if (ret)
> -		return ret;
> -
>   	/* Attach the panel-bridge to the dsi bridge */
>   	return drm_bridge_attach(bridge->encoder, ps_bridge->panel_bridge,
>   				 &ps_bridge->bridge, flags);
> @@ -305,6 +263,53 @@ static const struct drm_bridge_funcs ps8640_bridge_funcs = {
>   	.pre_enable = ps8640_pre_enable,
>   };
>   
> +static int ps8640_bridge_host_attach(struct device *dev, struct ps8640 *ps_bridge)
> +{
> +	struct device_node *in_ep, *dsi_node;
> +	struct mipi_dsi_device *dsi;
> +	struct mipi_dsi_host *host;
> +	int ret;
> +	const struct mipi_dsi_device_info info = { .type = "ps8640",
> +						   .channel = 0,
> +						   .node = NULL,
> +						 };
> +
> +	/* port@0 is ps8640 dsi input port */
> +	in_ep = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);
> +	if (!in_ep)
> +		return -ENODEV;
> +
> +	dsi_node = of_graph_get_remote_port_parent(in_ep);
> +	of_node_put(in_ep);
> +	if (!dsi_node)
> +		return -ENODEV;
> +
> +	host = of_find_mipi_dsi_host_by_node(dsi_node);
> +	of_node_put(dsi_node);
> +	if (!host)
> +		return -EPROBE_DEFER;
> +
> +	dsi = devm_mipi_dsi_device_register_full(dev, host, &info);
> +	if (IS_ERR(dsi)) {
> +		dev_err(dev, "failed to create dsi device\n");
> +		return PTR_ERR(dsi);
> +	}
> +
> +	ps_bridge->dsi = dsi;
> +
> +	dsi->host = host;
> +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO |
> +			  MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
> +	dsi->format = MIPI_DSI_FMT_RGB888;
> +	dsi->lanes = DP_NUM_LANES;
> +
> +	ret = devm_mipi_dsi_attach(dev, dsi);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
>   static int ps8640_probe(struct i2c_client *client)
>   {
>   	struct device *dev = &client->dev;
> @@ -371,6 +376,10 @@ static int ps8640_probe(struct i2c_client *client)
>   
>   	drm_bridge_add(&ps_bridge->bridge);
>   
> +	ret = ps8640_bridge_host_attach(dev, ps_bridge);
> +	if (ret)
> +		return ret;


I do not see drm_bridge_remove on error path, the same for sn65dsi83.


Regards

Andrzej


> +
>   	return 0;
>   }
>   

Re: [PATCH v3 2/8] drm/bridge: Document the probe issue with MIPI-DSI bridges

[Maxime Ripard]

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

Hi Andrzej,

On Mon, Aug 23, 2021 at 06:32:11PM +0200, Andrzej Hajda wrote:
> Hi Maxime,
> 
> On 23.08.2021 10:47, 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>
> > ---
> >   Documentation/gpu/drm-kms-helpers.rst |  6 +++
> >   drivers/gpu/drm/drm_bridge.c          | 58 +++++++++++++++++++++++++++
> >   2 files changed, 64 insertions(+)
> >
> > diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
> > index 10f8df7aecc0..ec2f65b31930 100644
> > --- a/Documentation/gpu/drm-kms-helpers.rst
> > +++ b/Documentation/gpu/drm-kms-helpers.rst
> > @@ -157,6 +157,12 @@ Display Driver Integration
> >   .. kernel-doc:: drivers/gpu/drm/drm_bridge.c
> >      :doc: display driver integration
> >   
> > +Special Care with MIPI-DSI bridges
> > +----------------------------------
> > +
> > +.. kernel-doc:: drivers/gpu/drm/drm_bridge.c
> > +   :doc: special care dsi
> > +
> >   Bridge Operations
> >   -----------------
> >   
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index baff74ea4a33..794654233cf5 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -96,6 +96,64 @@
> >    * 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.
> 
> I guess component_add is leftover from previous iteration (as you wrote 
> few lines below) component_add should be called from dsi host attach 
> callback.

Indeed, I'll remove it

> > + *
> > + * - In its probe hook, the bridge driver must try to find its MIPI-DSI
> > + *   host, register as a MIPI-DSI device and attach the MIPI-DSI device
> > + *   to its host. The bridge driver is now functional.
> > + *
> > + * - In its &struct mipi_dsi_host_ops.attach hook, the display driver
> > + *   can now add its component. Its bind hook will now be called and
> > + *   since the bridge driver is attached and registered, we can now look
> > + *   for and attach it.
> > + *
> > + * At this point, we're now certain that both the display driver and the
> > + * bridge driver are functional and we can't have a deadlock-like
> > + * situation when probing.
> > + */
> > +
> 
> Beside small mistake the whole patch looks OK for me. Maybe it would be 
> worth to mention what is the real cause of this "special DSI case" - 
> there is mutual dependency between two following entities in display chain:
> 
> 1. display driver - it provides DSI bus, and requires drm_bridge or 
> drm_panel provided by child device.
> 
> 2. bridge or panel with DSI transport - it requires DSI bus provided by 
> display driver, and provides drm_bridge or drm_panel interface required 
> by display driver.

I was trying to explain it in the first part of this patch. Is there
anything misleading there?

> I guess similar issues can appear with other data/control bus-es, 
> apparently DSI case is the most common.

The issue only presents itself when it's using a separate control bus
actually. If it's controlled through DCS, the panel / bridge will be a
children node of the DSI host and will only be probed when the host is
registered, so we don't have this issue.

> And one more thing - you use "display driver" term but this is also case 
> of any bridge providing DSI bus - there are already 3 such bridges in 
> kernel - cdns, nwl, synopsys, tc358768, maybe "DSI host" would be better 
> term.

Good point, I'll change it.

> And another thing - downstream device can be bridge or *panel*, it would 
> be good to mention that panels also should follow this pattern.

We're pretty much forced to do this with panels though. They don't have
an attach hook unlike bridges so we don't have much other options than
putting it in probe.

> Btw this is another place where word bridge can be 1:1 replaced by word 
> panel - it clearly suggest that DRM subsystem waits for brave men who 
> proposes patches unifying them, we would save lot of words, and lines of 
> code if we could use drm_sink instead of "if (sink is bridge) do sth 
> else do sth-similar-but-with-drm_panel-interface".

I agree. In the previous iteration I had this patch that was a step in
this direction:
https://lore.kernel.org/dri-devel/20210728133229.2247965-3-maxime@cerno.tech/

Even though it's not relevant to this series anymore, I still plan on
submitting it and converting as many users as possible (if my coccinelle
skills allows me to at least).

Maxime

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