[PATCH v3] drm: of: Properly try all possible cases for bridge/panel detection

[PATCH v3] drm: of: Properly try all possible cases for bridge/panel detection

[Paul Kocialkowski]

While bridge/panel detection was initially relying on the usual
port/ports-based of graph detection, it was recently changed to
perform the lookup on any child node that is not port/ports
instead when such a node is available, with no fallback on the
usual way.

This results in breaking detection when a child node is present
but does not contain any panel or bridge node, even when the
usual port/ports-based of graph is there.

In order to support both situations properly, this commit reworks
the logic to try both options and not just one of the two: it will
only return -EPROBE_DEFER when both have failed.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Fixes: 80253168dbfd ("drm: of: Lookup if child node has panel or bridge")
---

Changes since v2:
- Removed unnecessary else statement and added a comment about
  clearing the panel pointer on error.

Changes since v1:
- Renamed remote to node;
- Renamed helper to find_panel_or_bridge;
- Cleared bridge pointer early;
- Returned early to make the code more concise;

---
 drivers/gpu/drm/drm_of.c | 99 ++++++++++++++++++++--------------------
 1 file changed, 50 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index 9d90cd75c457..8716da6369a6 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -219,6 +219,29 @@ int drm_of_encoder_active_endpoint(struct device_node *node,
 }
 EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint);
 
+static int find_panel_or_bridge(struct device_node *node,
+				struct drm_panel **panel,
+				struct drm_bridge **bridge)
+{
+	if (panel) {
+		*panel = of_drm_find_panel(node);
+		if (!IS_ERR(*panel))
+			return 0;
+
+		/* Clear the panel pointer in case of error. */
+		*panel = NULL;
+	}
+
+	/* No panel found yet, check for a bridge next. */
+	if (bridge) {
+		*bridge = of_drm_find_bridge(node);
+		if (*bridge)
+			return 0;
+	}
+
+	return -EPROBE_DEFER;
+}
+
 /**
  * drm_of_find_panel_or_bridge - return connected panel or bridge device
  * @np: device tree node containing encoder output ports
@@ -241,66 +264,44 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
 				struct drm_panel **panel,
 				struct drm_bridge **bridge)
 {
-	int ret = -EPROBE_DEFER;
-	struct device_node *remote;
+	struct device_node *node;
+	int ret;
 
 	if (!panel && !bridge)
 		return -EINVAL;
+
 	if (panel)
 		*panel = NULL;
-
-	/**
-	 * Devices can also be child nodes when we also control that device
-	 * through the upstream device (ie, MIPI-DCS for a MIPI-DSI device).
-	 *
-	 * Lookup for a child node of the given parent that isn't either port
-	 * or ports.
-	 */
-	for_each_available_child_of_node(np, remote) {
-		if (of_node_name_eq(remote, "port") ||
-		    of_node_name_eq(remote, "ports"))
-			continue;
-
-		goto of_find_panel_or_bridge;
+	if (bridge)
+		*bridge = NULL;
+
+	/* Check for a graph on the device node first. */
+	if (of_graph_is_present(np)) {
+		node = of_graph_get_remote_node(np, port, endpoint);
+		if (node) {
+			ret = find_panel_or_bridge(node, panel, bridge);
+			of_node_put(node);
+
+			if (!ret)
+				return 0;
+		}
 	}
 
-	/*
-	 * 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 -ENODEV;
-
-	remote = of_graph_get_remote_node(np, port, endpoint);
-
-of_find_panel_or_bridge:
-	if (!remote)
-		return -ENODEV;
+	/* Otherwise check for any child node other than port/ports. */
+	for_each_available_child_of_node(np, node) {
+		if (of_node_name_eq(node, "port") ||
+		    of_node_name_eq(node, "ports"))
+			continue;
 
-	if (panel) {
-		*panel = of_drm_find_panel(remote);
-		if (!IS_ERR(*panel))
-			ret = 0;
-		else
-			*panel = NULL;
-	}
-
-	/* No panel found yet, check for a bridge next. */
-	if (bridge) {
-		if (ret) {
-			*bridge = of_drm_find_bridge(remote);
-			if (*bridge)
-				ret = 0;
-		} else {
-			*bridge = NULL;
-		}
+		ret = find_panel_or_bridge(node, panel, bridge);
+		of_node_put(node);
 
+		/* Stop at the first found occurrence. */
+		if (!ret)
+			return 0;
 	}
 
-	of_node_put(remote);
-	return ret;
+	return -EPROBE_DEFER;
 }
 EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
 
-- 
2.35.1

[PATCH v3] drm: of: Properly try all possible cases for bridge/panel detection

[Paul Kocialkowski]

While bridge/panel detection was initially relying on the usual
port/ports-based of graph detection, it was recently changed to
perform the lookup on any child node that is not port/ports
instead when such a node is available, with no fallback on the
usual way.

This results in breaking detection when a child node is present
but does not contain any panel or bridge node, even when the
usual port/ports-based of graph is there.

In order to support both situations properly, this commit reworks
the logic to try both options and not just one of the two: it will
only return -EPROBE_DEFER when both have failed.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Fixes: 80253168dbfd ("drm: of: Lookup if child node has panel or bridge")
---

Changes since v2:
- Removed unnecessary else statement and added a comment about
  clearing the panel pointer on error.

Changes since v1:
- Renamed remote to node;
- Renamed helper to find_panel_or_bridge;
- Cleared bridge pointer early;
- Returned early to make the code more concise;

---
 drivers/gpu/drm/drm_of.c | 99 ++++++++++++++++++++--------------------
 1 file changed, 50 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index 9d90cd75c457..8716da6369a6 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -219,6 +219,29 @@ int drm_of_encoder_active_endpoint(struct device_node *node,
 }
 EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint);
 
+static int find_panel_or_bridge(struct device_node *node,
+				struct drm_panel **panel,
+				struct drm_bridge **bridge)
+{
+	if (panel) {
+		*panel = of_drm_find_panel(node);
+		if (!IS_ERR(*panel))
+			return 0;
+
+		/* Clear the panel pointer in case of error. */
+		*panel = NULL;
+	}
+
+	/* No panel found yet, check for a bridge next. */
+	if (bridge) {
+		*bridge = of_drm_find_bridge(node);
+		if (*bridge)
+			return 0;
+	}
+
+	return -EPROBE_DEFER;
+}
+
 /**
  * drm_of_find_panel_or_bridge - return connected panel or bridge device
  * @np: device tree node containing encoder output ports
@@ -241,66 +264,44 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
 				struct drm_panel **panel,
 				struct drm_bridge **bridge)
 {
-	int ret = -EPROBE_DEFER;
-	struct device_node *remote;
+	struct device_node *node;
+	int ret;
 
 	if (!panel && !bridge)
 		return -EINVAL;
+
 	if (panel)
 		*panel = NULL;
-
-	/**
-	 * Devices can also be child nodes when we also control that device
-	 * through the upstream device (ie, MIPI-DCS for a MIPI-DSI device).
-	 *
-	 * Lookup for a child node of the given parent that isn't either port
-	 * or ports.
-	 */
-	for_each_available_child_of_node(np, remote) {
-		if (of_node_name_eq(remote, "port") ||
-		    of_node_name_eq(remote, "ports"))
-			continue;
-
-		goto of_find_panel_or_bridge;
+	if (bridge)
+		*bridge = NULL;
+
+	/* Check for a graph on the device node first. */
+	if (of_graph_is_present(np)) {
+		node = of_graph_get_remote_node(np, port, endpoint);
+		if (node) {
+			ret = find_panel_or_bridge(node, panel, bridge);
+			of_node_put(node);
+
+			if (!ret)
+				return 0;
+		}
 	}
 
-	/*
-	 * 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 -ENODEV;
-
-	remote = of_graph_get_remote_node(np, port, endpoint);
-
-of_find_panel_or_bridge:
-	if (!remote)
-		return -ENODEV;
+	/* Otherwise check for any child node other than port/ports. */
+	for_each_available_child_of_node(np, node) {
+		if (of_node_name_eq(node, "port") ||
+		    of_node_name_eq(node, "ports"))
+			continue;
 
-	if (panel) {
-		*panel = of_drm_find_panel(remote);
-		if (!IS_ERR(*panel))
-			ret = 0;
-		else
-			*panel = NULL;
-	}
-
-	/* No panel found yet, check for a bridge next. */
-	if (bridge) {
-		if (ret) {
-			*bridge = of_drm_find_bridge(remote);
-			if (*bridge)
-				ret = 0;
-		} else {
-			*bridge = NULL;
-		}
+		ret = find_panel_or_bridge(node, panel, bridge);
+		of_node_put(node);
 
+		/* Stop at the first found occurrence. */
+		if (!ret)
+			return 0;
 	}
 
-	of_node_put(remote);
-	return ret;
+	return -EPROBE_DEFER;
 }
 EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
 
-- 
2.35.1

Re: (subset) [PATCH v3] drm: of: Properly try all possible cases for bridge/panel detection

[Maxime Ripard]

On Tue, 29 Mar 2022 15:27:32 +0200, Paul Kocialkowski wrote:
> While bridge/panel detection was initially relying on the usual
> port/ports-based of graph detection, it was recently changed to
> perform the lookup on any child node that is not port/ports
> instead when such a node is available, with no fallback on the
> usual way.
> 
> This results in breaking detection when a child node is present
> but does not contain any panel or bridge node, even when the
> usual port/ports-based of graph is there.
> 
> [...]

Applied to drm/drm-misc (drm-misc-next-fixes).

Thanks!
Maxime

Re: (subset) [PATCH v3] drm: of: Properly try all possible cases for bridge/panel detection

[Maxime Ripard]

On Tue, 29 Mar 2022 15:27:32 +0200, Paul Kocialkowski wrote:
> While bridge/panel detection was initially relying on the usual
> port/ports-based of graph detection, it was recently changed to
> perform the lookup on any child node that is not port/ports
> instead when such a node is available, with no fallback on the
> usual way.
> 
> This results in breaking detection when a child node is present
> but does not contain any panel or bridge node, even when the
> usual port/ports-based of graph is there.
> 
> [...]

Applied to drm/drm-misc (drm-misc-next-fixes).

Thanks!
Maxime

Re: [PATCH v3] drm: of: Properly try all possible cases for bridge/panel detection

[Bjorn Andersson]

On Tue 29 Mar 06:27 PDT 2022, Paul Kocialkowski wrote:

> While bridge/panel detection was initially relying on the usual
> port/ports-based of graph detection, it was recently changed to
> perform the lookup on any child node that is not port/ports
> instead when such a node is available, with no fallback on the
> usual way.
> 
> This results in breaking detection when a child node is present
> but does not contain any panel or bridge node, even when the
> usual port/ports-based of graph is there.
> 
> In order to support both situations properly, this commit reworks
> the logic to try both options and not just one of the two: it will
> only return -EPROBE_DEFER when both have failed.
> 

Thanks for your patch Paul, it fixed a regression on a device where I
have a eDP bridge with an of_graph and a aux-bus defined.

But unfortunately it does not resolve the regression I have for the
USB based DisplayPort setup described below.


In the Qualcomm DisplayPort driver We're calling:

	devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);

and with the following DT snippet the behavior changed:

displayport-controller@ae90000 {
	compatible = "qcom,sc8180x-dp";
	...

	operating-points-v2 = <&dp0_opp_table>;

	ports {
		#address-cells = <1>;
		#size-cells = <0>;

		port@0 {
			reg = <0>;
			dp0_in: endpoint {
				remote-endpoint = <&display_driver>;
			};
		};
	};

	dp0_opp_table: opp-table {
		...;
	};
};

Prior to the introduction of 80253168dbfd ("drm: of: Lookup if child
node has panel or bridge") this would return -ENODEV, so we could
differentiate the case when we have a statically defined eDP panel from
that of a dynamically attached (over USB) DP panel.

Prior to your change, above case without the opp-table node would have
still returned -ENODEV.

But now this will just return -EPROBE_DEFER in both cases.


I thought the appropriate method of referencing the dsi panel was to
actually reference that using the of_graph, even though it's a child of
the dsi controller - that's at least how we've done it in e.g. [1].
I find this to be much nicer than to just blindly define that all
children of any sort of display controller must be a bridge or a panel.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sdm845-mtp.dts#n436

Regards,
Bjorn

> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> Fixes: 80253168dbfd ("drm: of: Lookup if child node has panel or bridge")
> ---
> 
> Changes since v2:
> - Removed unnecessary else statement and added a comment about
>   clearing the panel pointer on error.
> 
> Changes since v1:
> - Renamed remote to node;
> - Renamed helper to find_panel_or_bridge;
> - Cleared bridge pointer early;
> - Returned early to make the code more concise;
> 
> ---
>  drivers/gpu/drm/drm_of.c | 99 ++++++++++++++++++++--------------------
>  1 file changed, 50 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index 9d90cd75c457..8716da6369a6 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -219,6 +219,29 @@ int drm_of_encoder_active_endpoint(struct device_node *node,
>  }
>  EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint);
>  
> +static int find_panel_or_bridge(struct device_node *node,
> +				struct drm_panel **panel,
> +				struct drm_bridge **bridge)
> +{
> +	if (panel) {
> +		*panel = of_drm_find_panel(node);
> +		if (!IS_ERR(*panel))
> +			return 0;
> +
> +		/* Clear the panel pointer in case of error. */
> +		*panel = NULL;
> +	}
> +
> +	/* No panel found yet, check for a bridge next. */
> +	if (bridge) {
> +		*bridge = of_drm_find_bridge(node);
> +		if (*bridge)
> +			return 0;
> +	}
> +
> +	return -EPROBE_DEFER;
> +}
> +
>  /**
>   * drm_of_find_panel_or_bridge - return connected panel or bridge device
>   * @np: device tree node containing encoder output ports
> @@ -241,66 +264,44 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
>  				struct drm_panel **panel,
>  				struct drm_bridge **bridge)
>  {
> -	int ret = -EPROBE_DEFER;
> -	struct device_node *remote;
> +	struct device_node *node;
> +	int ret;
>  
>  	if (!panel && !bridge)
>  		return -EINVAL;
> +
>  	if (panel)
>  		*panel = NULL;
> -
> -	/**
> -	 * Devices can also be child nodes when we also control that device
> -	 * through the upstream device (ie, MIPI-DCS for a MIPI-DSI device).
> -	 *
> -	 * Lookup for a child node of the given parent that isn't either port
> -	 * or ports.
> -	 */
> -	for_each_available_child_of_node(np, remote) {
> -		if (of_node_name_eq(remote, "port") ||
> -		    of_node_name_eq(remote, "ports"))
> -			continue;
> -
> -		goto of_find_panel_or_bridge;
> +	if (bridge)
> +		*bridge = NULL;
> +
> +	/* Check for a graph on the device node first. */
> +	if (of_graph_is_present(np)) {
> +		node = of_graph_get_remote_node(np, port, endpoint);
> +		if (node) {
> +			ret = find_panel_or_bridge(node, panel, bridge);
> +			of_node_put(node);
> +
> +			if (!ret)
> +				return 0;
> +		}
>  	}
>  
> -	/*
> -	 * 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 -ENODEV;
> -
> -	remote = of_graph_get_remote_node(np, port, endpoint);
> -
> -of_find_panel_or_bridge:
> -	if (!remote)
> -		return -ENODEV;
> +	/* Otherwise check for any child node other than port/ports. */
> +	for_each_available_child_of_node(np, node) {
> +		if (of_node_name_eq(node, "port") ||
> +		    of_node_name_eq(node, "ports"))
> +			continue;
>  
> -	if (panel) {
> -		*panel = of_drm_find_panel(remote);
> -		if (!IS_ERR(*panel))
> -			ret = 0;
> -		else
> -			*panel = NULL;
> -	}
> -
> -	/* No panel found yet, check for a bridge next. */
> -	if (bridge) {
> -		if (ret) {
> -			*bridge = of_drm_find_bridge(remote);
> -			if (*bridge)
> -				ret = 0;
> -		} else {
> -			*bridge = NULL;
> -		}
> +		ret = find_panel_or_bridge(node, panel, bridge);
> +		of_node_put(node);
>  
> +		/* Stop at the first found occurrence. */
> +		if (!ret)
> +			return 0;
>  	}
>  
> -	of_node_put(remote);
> -	return ret;
> +	return -EPROBE_DEFER;
>  }
>  EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
>  
> -- 
> 2.35.1
> 

Re: [PATCH v3] drm: of: Properly try all possible cases for bridge/panel detection

[Bjorn Andersson]

On Tue 29 Mar 06:27 PDT 2022, Paul Kocialkowski wrote:

> While bridge/panel detection was initially relying on the usual
> port/ports-based of graph detection, it was recently changed to
> perform the lookup on any child node that is not port/ports
> instead when such a node is available, with no fallback on the
> usual way.
> 
> This results in breaking detection when a child node is present
> but does not contain any panel or bridge node, even when the
> usual port/ports-based of graph is there.
> 
> In order to support both situations properly, this commit reworks
> the logic to try both options and not just one of the two: it will
> only return -EPROBE_DEFER when both have failed.
> 

Thanks for your patch Paul, it fixed a regression on a device where I
have a eDP bridge with an of_graph and a aux-bus defined.

But unfortunately it does not resolve the regression I have for the
USB based DisplayPort setup described below.


In the Qualcomm DisplayPort driver We're calling:

	devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);

and with the following DT snippet the behavior changed:

displayport-controller@ae90000 {
	compatible = "qcom,sc8180x-dp";
	...

	operating-points-v2 = <&dp0_opp_table>;

	ports {
		#address-cells = <1>;
		#size-cells = <0>;

		port@0 {
			reg = <0>;
			dp0_in: endpoint {
				remote-endpoint = <&display_driver>;
			};
		};
	};

	dp0_opp_table: opp-table {
		...;
	};
};

Prior to the introduction of 80253168dbfd ("drm: of: Lookup if child
node has panel or bridge") this would return -ENODEV, so we could
differentiate the case when we have a statically defined eDP panel from
that of a dynamically attached (over USB) DP panel.

Prior to your change, above case without the opp-table node would have
still returned -ENODEV.

But now this will just return -EPROBE_DEFER in both cases.


I thought the appropriate method of referencing the dsi panel was to
actually reference that using the of_graph, even though it's a child of
the dsi controller - that's at least how we've done it in e.g. [1].
I find this to be much nicer than to just blindly define that all
children of any sort of display controller must be a bridge or a panel.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sdm845-mtp.dts#n436

Regards,
Bjorn

> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> Fixes: 80253168dbfd ("drm: of: Lookup if child node has panel or bridge")
> ---
> 
> Changes since v2:
> - Removed unnecessary else statement and added a comment about
>   clearing the panel pointer on error.
> 
> Changes since v1:
> - Renamed remote to node;
> - Renamed helper to find_panel_or_bridge;
> - Cleared bridge pointer early;
> - Returned early to make the code more concise;
> 
> ---
>  drivers/gpu/drm/drm_of.c | 99 ++++++++++++++++++++--------------------
>  1 file changed, 50 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index 9d90cd75c457..8716da6369a6 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -219,6 +219,29 @@ int drm_of_encoder_active_endpoint(struct device_node *node,
>  }
>  EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint);
>  
> +static int find_panel_or_bridge(struct device_node *node,
> +				struct drm_panel **panel,
> +				struct drm_bridge **bridge)
> +{
> +	if (panel) {
> +		*panel = of_drm_find_panel(node);
> +		if (!IS_ERR(*panel))
> +			return 0;
> +
> +		/* Clear the panel pointer in case of error. */
> +		*panel = NULL;
> +	}
> +
> +	/* No panel found yet, check for a bridge next. */
> +	if (bridge) {
> +		*bridge = of_drm_find_bridge(node);
> +		if (*bridge)
> +			return 0;
> +	}
> +
> +	return -EPROBE_DEFER;
> +}
> +
>  /**
>   * drm_of_find_panel_or_bridge - return connected panel or bridge device
>   * @np: device tree node containing encoder output ports
> @@ -241,66 +264,44 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
>  				struct drm_panel **panel,
>  				struct drm_bridge **bridge)
>  {
> -	int ret = -EPROBE_DEFER;
> -	struct device_node *remote;
> +	struct device_node *node;
> +	int ret;
>  
>  	if (!panel && !bridge)
>  		return -EINVAL;
> +
>  	if (panel)
>  		*panel = NULL;
> -
> -	/**
> -	 * Devices can also be child nodes when we also control that device
> -	 * through the upstream device (ie, MIPI-DCS for a MIPI-DSI device).
> -	 *
> -	 * Lookup for a child node of the given parent that isn't either port
> -	 * or ports.
> -	 */
> -	for_each_available_child_of_node(np, remote) {
> -		if (of_node_name_eq(remote, "port") ||
> -		    of_node_name_eq(remote, "ports"))
> -			continue;
> -
> -		goto of_find_panel_or_bridge;
> +	if (bridge)
> +		*bridge = NULL;
> +
> +	/* Check for a graph on the device node first. */
> +	if (of_graph_is_present(np)) {
> +		node = of_graph_get_remote_node(np, port, endpoint);
> +		if (node) {
> +			ret = find_panel_or_bridge(node, panel, bridge);
> +			of_node_put(node);
> +
> +			if (!ret)
> +				return 0;
> +		}
>  	}
>  
> -	/*
> -	 * 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 -ENODEV;
> -
> -	remote = of_graph_get_remote_node(np, port, endpoint);
> -
> -of_find_panel_or_bridge:
> -	if (!remote)
> -		return -ENODEV;
> +	/* Otherwise check for any child node other than port/ports. */
> +	for_each_available_child_of_node(np, node) {
> +		if (of_node_name_eq(node, "port") ||
> +		    of_node_name_eq(node, "ports"))
> +			continue;
>  
> -	if (panel) {
> -		*panel = of_drm_find_panel(remote);
> -		if (!IS_ERR(*panel))
> -			ret = 0;
> -		else
> -			*panel = NULL;
> -	}
> -
> -	/* No panel found yet, check for a bridge next. */
> -	if (bridge) {
> -		if (ret) {
> -			*bridge = of_drm_find_bridge(remote);
> -			if (*bridge)
> -				ret = 0;
> -		} else {
> -			*bridge = NULL;
> -		}
> +		ret = find_panel_or_bridge(node, panel, bridge);
> +		of_node_put(node);
>  
> +		/* Stop at the first found occurrence. */
> +		if (!ret)
> +			return 0;
>  	}
>  
> -	of_node_put(remote);
> -	return ret;
> +	return -EPROBE_DEFER;
>  }
>  EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
>  
> -- 
> 2.35.1
> 

Re: [PATCH v3] drm: of: Properly try all possible cases for bridge/panel detection

[Paul Kocialkowski]

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

Hi Bjorn,

On Thu 31 Mar 22, 20:16, Bjorn Andersson wrote:
> On Tue 29 Mar 06:27 PDT 2022, Paul Kocialkowski wrote:
> 
> > While bridge/panel detection was initially relying on the usual
> > port/ports-based of graph detection, it was recently changed to
> > perform the lookup on any child node that is not port/ports
> > instead when such a node is available, with no fallback on the
> > usual way.
> > 
> > This results in breaking detection when a child node is present
> > but does not contain any panel or bridge node, even when the
> > usual port/ports-based of graph is there.
> > 
> > In order to support both situations properly, this commit reworks
> > the logic to try both options and not just one of the two: it will
> > only return -EPROBE_DEFER when both have failed.
> > 
> 
> Thanks for your patch Paul, it fixed a regression on a device where I
> have a eDP bridge with an of_graph and a aux-bus defined.
> 
> But unfortunately it does not resolve the regression I have for the
> USB based DisplayPort setup described below.
> 
> 
> In the Qualcomm DisplayPort driver We're calling:
> 
> 	devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
> 
> and with the following DT snippet the behavior changed:
> 
> displayport-controller@ae90000 {
> 	compatible = "qcom,sc8180x-dp";
> 	...
> 
> 	operating-points-v2 = <&dp0_opp_table>;
> 
> 	ports {
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 
> 		port@0 {
> 			reg = <0>;
> 			dp0_in: endpoint {
> 				remote-endpoint = <&display_driver>;
> 			};
> 		};
> 	};
> 
> 	dp0_opp_table: opp-table {
> 		...;
> 	};
> };
> 
> Prior to the introduction of 80253168dbfd ("drm: of: Lookup if child
> node has panel or bridge") this would return -ENODEV, so we could
> differentiate the case when we have a statically defined eDP panel from
> that of a dynamically attached (over USB) DP panel.
> 
> Prior to your change, above case without the opp-table node would have
> still returned -ENODEV.
> 
> But now this will just return -EPROBE_DEFER in both cases.

Oh that's right, the -ENODEV case was just completely removed by my change.
Initially this would happen if !of_graph_is_present or if the remote node
doesn't exist.

Now that we are also checking for child nodes, we can't just return -ENODEV
when the graph or remote node is missing: we must also check that there is no
child node that is a panel/bridge.

For the graph remote case, we can reliabily return -EPROBE_DEFER when
of_graph_is_present and the remote exists and of_device_is_available.
Otherwise we can go for -ENODEV. I think getting -EPROBE_DEFER at this point
should stop the drm_of_find_panel_or_bridge process.

On the other hand for the child panel/bridge node case, I don't see how we
can reliably distinguish between -EPROBE_DEFER and -ENODEV, because
of_drm_find_panel and of_drm_find_bridge will behave the same if the child
node is a not-yet-probed panel/bridge or a totally unrelated node.
So I think we should always return -EPROBE_DEFER in that case.

As a result you can't get -ENODEV if using the of graph while having any
(unrelated) child node there, so your issue remains.

Do you see any way we could make this work?

> I thought the appropriate method of referencing the dsi panel was to
> actually reference that using the of_graph, even though it's a child of
> the dsi controller - that's at least how we've done it in e.g. [1].
> I find this to be much nicer than to just blindly define that all
> children of any sort of display controller must be a bridge or a panel.

Yes I totally agree. Given that using the child node directly apparently
can't allow us to distinguish between -EPROBE_DEFER/-ENODEV I would be in
favor of dropping this mechanism and going with explicit of graph in any case
(even if it's a child node). I don't see any downside to this approach.

What do yout think?

Paul

> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sdm845-mtp.dts#n436
> 
> Regards,
> Bjorn
> 
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > Fixes: 80253168dbfd ("drm: of: Lookup if child node has panel or bridge")
> > ---
> > 
> > Changes since v2:
> > - Removed unnecessary else statement and added a comment about
> >   clearing the panel pointer on error.
> > 
> > Changes since v1:
> > - Renamed remote to node;
> > - Renamed helper to find_panel_or_bridge;
> > - Cleared bridge pointer early;
> > - Returned early to make the code more concise;
> > 
> > ---
> >  drivers/gpu/drm/drm_of.c | 99 ++++++++++++++++++++--------------------
> >  1 file changed, 50 insertions(+), 49 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> > index 9d90cd75c457..8716da6369a6 100644
> > --- a/drivers/gpu/drm/drm_of.c
> > +++ b/drivers/gpu/drm/drm_of.c
> > @@ -219,6 +219,29 @@ int drm_of_encoder_active_endpoint(struct device_node *node,
> >  }
> >  EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint);
> >  
> > +static int find_panel_or_bridge(struct device_node *node,
> > +				struct drm_panel **panel,
> > +				struct drm_bridge **bridge)
> > +{
> > +	if (panel) {
> > +		*panel = of_drm_find_panel(node);
> > +		if (!IS_ERR(*panel))
> > +			return 0;
> > +
> > +		/* Clear the panel pointer in case of error. */
> > +		*panel = NULL;
> > +	}
> > +
> > +	/* No panel found yet, check for a bridge next. */
> > +	if (bridge) {
> > +		*bridge = of_drm_find_bridge(node);
> > +		if (*bridge)
> > +			return 0;
> > +	}
> > +
> > +	return -EPROBE_DEFER;
> > +}
> > +
> >  /**
> >   * drm_of_find_panel_or_bridge - return connected panel or bridge device
> >   * @np: device tree node containing encoder output ports
> > @@ -241,66 +264,44 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> >  				struct drm_panel **panel,
> >  				struct drm_bridge **bridge)
> >  {
> > -	int ret = -EPROBE_DEFER;
> > -	struct device_node *remote;
> > +	struct device_node *node;
> > +	int ret;
> >  
> >  	if (!panel && !bridge)
> >  		return -EINVAL;
> > +
> >  	if (panel)
> >  		*panel = NULL;
> > -
> > -	/**
> > -	 * Devices can also be child nodes when we also control that device
> > -	 * through the upstream device (ie, MIPI-DCS for a MIPI-DSI device).
> > -	 *
> > -	 * Lookup for a child node of the given parent that isn't either port
> > -	 * or ports.
> > -	 */
> > -	for_each_available_child_of_node(np, remote) {
> > -		if (of_node_name_eq(remote, "port") ||
> > -		    of_node_name_eq(remote, "ports"))
> > -			continue;
> > -
> > -		goto of_find_panel_or_bridge;
> > +	if (bridge)
> > +		*bridge = NULL;
> > +
> > +	/* Check for a graph on the device node first. */
> > +	if (of_graph_is_present(np)) {
> > +		node = of_graph_get_remote_node(np, port, endpoint);
> > +		if (node) {
> > +			ret = find_panel_or_bridge(node, panel, bridge);
> > +			of_node_put(node);
> > +
> > +			if (!ret)
> > +				return 0;
> > +		}
> >  	}
> >  
> > -	/*
> > -	 * 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 -ENODEV;
> > -
> > -	remote = of_graph_get_remote_node(np, port, endpoint);
> > -
> > -of_find_panel_or_bridge:
> > -	if (!remote)
> > -		return -ENODEV;
> > +	/* Otherwise check for any child node other than port/ports. */
> > +	for_each_available_child_of_node(np, node) {
> > +		if (of_node_name_eq(node, "port") ||
> > +		    of_node_name_eq(node, "ports"))
> > +			continue;
> >  
> > -	if (panel) {
> > -		*panel = of_drm_find_panel(remote);
> > -		if (!IS_ERR(*panel))
> > -			ret = 0;
> > -		else
> > -			*panel = NULL;
> > -	}
> > -
> > -	/* No panel found yet, check for a bridge next. */
> > -	if (bridge) {
> > -		if (ret) {
> > -			*bridge = of_drm_find_bridge(remote);
> > -			if (*bridge)
> > -				ret = 0;
> > -		} else {
> > -			*bridge = NULL;
> > -		}
> > +		ret = find_panel_or_bridge(node, panel, bridge);
> > +		of_node_put(node);
> >  
> > +		/* Stop at the first found occurrence. */
> > +		if (!ret)
> > +			return 0;
> >  	}
> >  
> > -	of_node_put(remote);
> > -	return ret;
> > +	return -EPROBE_DEFER;
> >  }
> >  EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
> >  
> > -- 
> > 2.35.1
> > 

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

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

Re: [PATCH v3] drm: of: Properly try all possible cases for bridge/panel detection

[Paul Kocialkowski]

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

Hi Bjorn,

On Thu 31 Mar 22, 20:16, Bjorn Andersson wrote:
> On Tue 29 Mar 06:27 PDT 2022, Paul Kocialkowski wrote:
> 
> > While bridge/panel detection was initially relying on the usual
> > port/ports-based of graph detection, it was recently changed to
> > perform the lookup on any child node that is not port/ports
> > instead when such a node is available, with no fallback on the
> > usual way.
> > 
> > This results in breaking detection when a child node is present
> > but does not contain any panel or bridge node, even when the
> > usual port/ports-based of graph is there.
> > 
> > In order to support both situations properly, this commit reworks
> > the logic to try both options and not just one of the two: it will
> > only return -EPROBE_DEFER when both have failed.
> > 
> 
> Thanks for your patch Paul, it fixed a regression on a device where I
> have a eDP bridge with an of_graph and a aux-bus defined.
> 
> But unfortunately it does not resolve the regression I have for the
> USB based DisplayPort setup described below.
> 
> 
> In the Qualcomm DisplayPort driver We're calling:
> 
> 	devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
> 
> and with the following DT snippet the behavior changed:
> 
> displayport-controller@ae90000 {
> 	compatible = "qcom,sc8180x-dp";
> 	...
> 
> 	operating-points-v2 = <&dp0_opp_table>;
> 
> 	ports {
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 
> 		port@0 {
> 			reg = <0>;
> 			dp0_in: endpoint {
> 				remote-endpoint = <&display_driver>;
> 			};
> 		};
> 	};
> 
> 	dp0_opp_table: opp-table {
> 		...;
> 	};
> };
> 
> Prior to the introduction of 80253168dbfd ("drm: of: Lookup if child
> node has panel or bridge") this would return -ENODEV, so we could
> differentiate the case when we have a statically defined eDP panel from
> that of a dynamically attached (over USB) DP panel.
> 
> Prior to your change, above case without the opp-table node would have
> still returned -ENODEV.
> 
> But now this will just return -EPROBE_DEFER in both cases.

Oh that's right, the -ENODEV case was just completely removed by my change.
Initially this would happen if !of_graph_is_present or if the remote node
doesn't exist.

Now that we are also checking for child nodes, we can't just return -ENODEV
when the graph or remote node is missing: we must also check that there is no
child node that is a panel/bridge.

For the graph remote case, we can reliabily return -EPROBE_DEFER when
of_graph_is_present and the remote exists and of_device_is_available.
Otherwise we can go for -ENODEV. I think getting -EPROBE_DEFER at this point
should stop the drm_of_find_panel_or_bridge process.

On the other hand for the child panel/bridge node case, I don't see how we
can reliably distinguish between -EPROBE_DEFER and -ENODEV, because
of_drm_find_panel and of_drm_find_bridge will behave the same if the child
node is a not-yet-probed panel/bridge or a totally unrelated node.
So I think we should always return -EPROBE_DEFER in that case.

As a result you can't get -ENODEV if using the of graph while having any
(unrelated) child node there, so your issue remains.

Do you see any way we could make this work?

> I thought the appropriate method of referencing the dsi panel was to
> actually reference that using the of_graph, even though it's a child of
> the dsi controller - that's at least how we've done it in e.g. [1].
> I find this to be much nicer than to just blindly define that all
> children of any sort of display controller must be a bridge or a panel.

Yes I totally agree. Given that using the child node directly apparently
can't allow us to distinguish between -EPROBE_DEFER/-ENODEV I would be in
favor of dropping this mechanism and going with explicit of graph in any case
(even if it's a child node). I don't see any downside to this approach.

What do yout think?

Paul

> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sdm845-mtp.dts#n436
> 
> Regards,
> Bjorn
> 
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > Fixes: 80253168dbfd ("drm: of: Lookup if child node has panel or bridge")
> > ---
> > 
> > Changes since v2:
> > - Removed unnecessary else statement and added a comment about
> >   clearing the panel pointer on error.
> > 
> > Changes since v1:
> > - Renamed remote to node;
> > - Renamed helper to find_panel_or_bridge;
> > - Cleared bridge pointer early;
> > - Returned early to make the code more concise;
> > 
> > ---
> >  drivers/gpu/drm/drm_of.c | 99 ++++++++++++++++++++--------------------
> >  1 file changed, 50 insertions(+), 49 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> > index 9d90cd75c457..8716da6369a6 100644
> > --- a/drivers/gpu/drm/drm_of.c
> > +++ b/drivers/gpu/drm/drm_of.c
> > @@ -219,6 +219,29 @@ int drm_of_encoder_active_endpoint(struct device_node *node,
> >  }
> >  EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint);
> >  
> > +static int find_panel_or_bridge(struct device_node *node,
> > +				struct drm_panel **panel,
> > +				struct drm_bridge **bridge)
> > +{
> > +	if (panel) {
> > +		*panel = of_drm_find_panel(node);
> > +		if (!IS_ERR(*panel))
> > +			return 0;
> > +
> > +		/* Clear the panel pointer in case of error. */
> > +		*panel = NULL;
> > +	}
> > +
> > +	/* No panel found yet, check for a bridge next. */
> > +	if (bridge) {
> > +		*bridge = of_drm_find_bridge(node);
> > +		if (*bridge)
> > +			return 0;
> > +	}
> > +
> > +	return -EPROBE_DEFER;
> > +}
> > +
> >  /**
> >   * drm_of_find_panel_or_bridge - return connected panel or bridge device
> >   * @np: device tree node containing encoder output ports
> > @@ -241,66 +264,44 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> >  				struct drm_panel **panel,
> >  				struct drm_bridge **bridge)
> >  {
> > -	int ret = -EPROBE_DEFER;
> > -	struct device_node *remote;
> > +	struct device_node *node;
> > +	int ret;
> >  
> >  	if (!panel && !bridge)
> >  		return -EINVAL;
> > +
> >  	if (panel)
> >  		*panel = NULL;
> > -
> > -	/**
> > -	 * Devices can also be child nodes when we also control that device
> > -	 * through the upstream device (ie, MIPI-DCS for a MIPI-DSI device).
> > -	 *
> > -	 * Lookup for a child node of the given parent that isn't either port
> > -	 * or ports.
> > -	 */
> > -	for_each_available_child_of_node(np, remote) {
> > -		if (of_node_name_eq(remote, "port") ||
> > -		    of_node_name_eq(remote, "ports"))
> > -			continue;
> > -
> > -		goto of_find_panel_or_bridge;
> > +	if (bridge)
> > +		*bridge = NULL;
> > +
> > +	/* Check for a graph on the device node first. */
> > +	if (of_graph_is_present(np)) {
> > +		node = of_graph_get_remote_node(np, port, endpoint);
> > +		if (node) {
> > +			ret = find_panel_or_bridge(node, panel, bridge);
> > +			of_node_put(node);
> > +
> > +			if (!ret)
> > +				return 0;
> > +		}
> >  	}
> >  
> > -	/*
> > -	 * 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 -ENODEV;
> > -
> > -	remote = of_graph_get_remote_node(np, port, endpoint);
> > -
> > -of_find_panel_or_bridge:
> > -	if (!remote)
> > -		return -ENODEV;
> > +	/* Otherwise check for any child node other than port/ports. */
> > +	for_each_available_child_of_node(np, node) {
> > +		if (of_node_name_eq(node, "port") ||
> > +		    of_node_name_eq(node, "ports"))
> > +			continue;
> >  
> > -	if (panel) {
> > -		*panel = of_drm_find_panel(remote);
> > -		if (!IS_ERR(*panel))
> > -			ret = 0;
> > -		else
> > -			*panel = NULL;
> > -	}
> > -
> > -	/* No panel found yet, check for a bridge next. */
> > -	if (bridge) {
> > -		if (ret) {
> > -			*bridge = of_drm_find_bridge(remote);
> > -			if (*bridge)
> > -				ret = 0;
> > -		} else {
> > -			*bridge = NULL;
> > -		}
> > +		ret = find_panel_or_bridge(node, panel, bridge);
> > +		of_node_put(node);
> >  
> > +		/* Stop at the first found occurrence. */
> > +		if (!ret)
> > +			return 0;
> >  	}
> >  
> > -	of_node_put(remote);
> > -	return ret;
> > +	return -EPROBE_DEFER;
> >  }
> >  EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
> >  
> > -- 
> > 2.35.1
> > 

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

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

Re: [PATCH v3] drm: of: Properly try all possible cases for bridge/panel detection

[Bjorn Andersson]

On Fri 01 Apr 00:44 PDT 2022, Paul Kocialkowski wrote:

> Hi Bjorn,
> 
> On Thu 31 Mar 22, 20:16, Bjorn Andersson wrote:
> > On Tue 29 Mar 06:27 PDT 2022, Paul Kocialkowski wrote:
> > 
> > > While bridge/panel detection was initially relying on the usual
> > > port/ports-based of graph detection, it was recently changed to
> > > perform the lookup on any child node that is not port/ports
> > > instead when such a node is available, with no fallback on the
> > > usual way.
> > > 
> > > This results in breaking detection when a child node is present
> > > but does not contain any panel or bridge node, even when the
> > > usual port/ports-based of graph is there.
> > > 
> > > In order to support both situations properly, this commit reworks
> > > the logic to try both options and not just one of the two: it will
> > > only return -EPROBE_DEFER when both have failed.
> > > 
> > 
> > Thanks for your patch Paul, it fixed a regression on a device where I
> > have a eDP bridge with an of_graph and a aux-bus defined.
> > 
> > But unfortunately it does not resolve the regression I have for the
> > USB based DisplayPort setup described below.
> > 
> > 
> > In the Qualcomm DisplayPort driver We're calling:
> > 
> > 	devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
> > 
> > and with the following DT snippet the behavior changed:
> > 
> > displayport-controller@ae90000 {
> > 	compatible = "qcom,sc8180x-dp";
> > 	...
> > 
> > 	operating-points-v2 = <&dp0_opp_table>;
> > 
> > 	ports {
> > 		#address-cells = <1>;
> > 		#size-cells = <0>;
> > 
> > 		port@0 {
> > 			reg = <0>;
> > 			dp0_in: endpoint {
> > 				remote-endpoint = <&display_driver>;
> > 			};
> > 		};
> > 	};
> > 
> > 	dp0_opp_table: opp-table {
> > 		...;
> > 	};
> > };
> > 
> > Prior to the introduction of 80253168dbfd ("drm: of: Lookup if child
> > node has panel or bridge") this would return -ENODEV, so we could
> > differentiate the case when we have a statically defined eDP panel from
> > that of a dynamically attached (over USB) DP panel.
> > 
> > Prior to your change, above case without the opp-table node would have
> > still returned -ENODEV.
> > 
> > But now this will just return -EPROBE_DEFER in both cases.
> 
> Oh that's right, the -ENODEV case was just completely removed by my change.
> Initially this would happen if !of_graph_is_present or if the remote node
> doesn't exist.
> 
> Now that we are also checking for child nodes, we can't just return -ENODEV
> when the graph or remote node is missing: we must also check that there is no
> child node that is a panel/bridge.
> 
> For the graph remote case, we can reliabily return -EPROBE_DEFER when
> of_graph_is_present and the remote exists and of_device_is_available.

Right, if I have a of_graph port@1 which references something that isn't
available we can reliably claim this is -EPROBE_DEFER.

> Otherwise we can go for -ENODEV.

Are you suggesting that if we find a "port" or "ports" we return -ENODEV
if we didn't find the requested port@N?

> I think getting -EPROBE_DEFER at this point
> should stop the drm_of_find_panel_or_bridge process.
> 

I think that makes sense, i.e. if we found an of_graph reference, but
it's not a panel yet.

> On the other hand for the child panel/bridge node case, I don't see how we
> can reliably distinguish between -EPROBE_DEFER and -ENODEV, because
> of_drm_find_panel and of_drm_find_bridge will behave the same if the child
> node is a not-yet-probed panel/bridge or a totally unrelated node.
> So I think we should always return -EPROBE_DEFER in that case.
> 
> As a result you can't get -ENODEV if using the of graph while having any
> (unrelated) child node there, so your issue remains.
> 
> Do you see any way we could make this work?
> 

I'm afraid I don't have any good suggestions on determining if that
child node is a panel/bridge or something else.

> > I thought the appropriate method of referencing the dsi panel was to
> > actually reference that using the of_graph, even though it's a child of
> > the dsi controller - that's at least how we've done it in e.g. [1].
> > I find this to be much nicer than to just blindly define that all
> > children of any sort of display controller must be a bridge or a panel.
> 
> Yes I totally agree. Given that using the child node directly apparently
> can't allow us to distinguish between -EPROBE_DEFER/-ENODEV I would be in
> favor of dropping this mechanism and going with explicit of graph in any case
> (even if it's a child node). I don't see any downside to this approach.
> 
> What do yout think?
> 

The explicit of_graph reference is a little bit clunky, but it's clear
and doesn't rely on "skipping" or only including names based on
particular names etc.

So I am in favour of reverting this back to the explicit reference.

Regards,
Bjorn

> Paul
> 
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sdm845-mtp.dts#n436
> > 
> > Regards,
> > Bjorn
> > 
> > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > Fixes: 80253168dbfd ("drm: of: Lookup if child node has panel or bridge")
> > > ---
> > > 
> > > Changes since v2:
> > > - Removed unnecessary else statement and added a comment about
> > >   clearing the panel pointer on error.
> > > 
> > > Changes since v1:
> > > - Renamed remote to node;
> > > - Renamed helper to find_panel_or_bridge;
> > > - Cleared bridge pointer early;
> > > - Returned early to make the code more concise;
> > > 
> > > ---
> > >  drivers/gpu/drm/drm_of.c | 99 ++++++++++++++++++++--------------------
> > >  1 file changed, 50 insertions(+), 49 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> > > index 9d90cd75c457..8716da6369a6 100644
> > > --- a/drivers/gpu/drm/drm_of.c
> > > +++ b/drivers/gpu/drm/drm_of.c
> > > @@ -219,6 +219,29 @@ int drm_of_encoder_active_endpoint(struct device_node *node,
> > >  }
> > >  EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint);
> > >  
> > > +static int find_panel_or_bridge(struct device_node *node,
> > > +				struct drm_panel **panel,
> > > +				struct drm_bridge **bridge)
> > > +{
> > > +	if (panel) {
> > > +		*panel = of_drm_find_panel(node);
> > > +		if (!IS_ERR(*panel))
> > > +			return 0;
> > > +
> > > +		/* Clear the panel pointer in case of error. */
> > > +		*panel = NULL;
> > > +	}
> > > +
> > > +	/* No panel found yet, check for a bridge next. */
> > > +	if (bridge) {
> > > +		*bridge = of_drm_find_bridge(node);
> > > +		if (*bridge)
> > > +			return 0;
> > > +	}
> > > +
> > > +	return -EPROBE_DEFER;
> > > +}
> > > +
> > >  /**
> > >   * drm_of_find_panel_or_bridge - return connected panel or bridge device
> > >   * @np: device tree node containing encoder output ports
> > > @@ -241,66 +264,44 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> > >  				struct drm_panel **panel,
> > >  				struct drm_bridge **bridge)
> > >  {
> > > -	int ret = -EPROBE_DEFER;
> > > -	struct device_node *remote;
> > > +	struct device_node *node;
> > > +	int ret;
> > >  
> > >  	if (!panel && !bridge)
> > >  		return -EINVAL;
> > > +
> > >  	if (panel)
> > >  		*panel = NULL;
> > > -
> > > -	/**
> > > -	 * Devices can also be child nodes when we also control that device
> > > -	 * through the upstream device (ie, MIPI-DCS for a MIPI-DSI device).
> > > -	 *
> > > -	 * Lookup for a child node of the given parent that isn't either port
> > > -	 * or ports.
> > > -	 */
> > > -	for_each_available_child_of_node(np, remote) {
> > > -		if (of_node_name_eq(remote, "port") ||
> > > -		    of_node_name_eq(remote, "ports"))
> > > -			continue;
> > > -
> > > -		goto of_find_panel_or_bridge;
> > > +	if (bridge)
> > > +		*bridge = NULL;
> > > +
> > > +	/* Check for a graph on the device node first. */
> > > +	if (of_graph_is_present(np)) {
> > > +		node = of_graph_get_remote_node(np, port, endpoint);
> > > +		if (node) {
> > > +			ret = find_panel_or_bridge(node, panel, bridge);
> > > +			of_node_put(node);
> > > +
> > > +			if (!ret)
> > > +				return 0;
> > > +		}
> > >  	}
> > >  
> > > -	/*
> > > -	 * 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 -ENODEV;
> > > -
> > > -	remote = of_graph_get_remote_node(np, port, endpoint);
> > > -
> > > -of_find_panel_or_bridge:
> > > -	if (!remote)
> > > -		return -ENODEV;
> > > +	/* Otherwise check for any child node other than port/ports. */
> > > +	for_each_available_child_of_node(np, node) {
> > > +		if (of_node_name_eq(node, "port") ||
> > > +		    of_node_name_eq(node, "ports"))
> > > +			continue;
> > >  
> > > -	if (panel) {
> > > -		*panel = of_drm_find_panel(remote);
> > > -		if (!IS_ERR(*panel))
> > > -			ret = 0;
> > > -		else
> > > -			*panel = NULL;
> > > -	}
> > > -
> > > -	/* No panel found yet, check for a bridge next. */
> > > -	if (bridge) {
> > > -		if (ret) {
> > > -			*bridge = of_drm_find_bridge(remote);
> > > -			if (*bridge)
> > > -				ret = 0;
> > > -		} else {
> > > -			*bridge = NULL;
> > > -		}
> > > +		ret = find_panel_or_bridge(node, panel, bridge);
> > > +		of_node_put(node);
> > >  
> > > +		/* Stop at the first found occurrence. */
> > > +		if (!ret)
> > > +			return 0;
> > >  	}
> > >  
> > > -	of_node_put(remote);
> > > -	return ret;
> > > +	return -EPROBE_DEFER;
> > >  }
> > >  EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
> > >  
> > > -- 
> > > 2.35.1
> > > 
> 
> -- 
> Paul Kocialkowski, Bootlin
> Embedded Linux and kernel engineering
> https://bootlin.com

Re: [PATCH v3] drm: of: Properly try all possible cases for bridge/panel detection

[Bjorn Andersson]

On Fri 01 Apr 00:44 PDT 2022, Paul Kocialkowski wrote:

> Hi Bjorn,
> 
> On Thu 31 Mar 22, 20:16, Bjorn Andersson wrote:
> > On Tue 29 Mar 06:27 PDT 2022, Paul Kocialkowski wrote:
> > 
> > > While bridge/panel detection was initially relying on the usual
> > > port/ports-based of graph detection, it was recently changed to
> > > perform the lookup on any child node that is not port/ports
> > > instead when such a node is available, with no fallback on the
> > > usual way.
> > > 
> > > This results in breaking detection when a child node is present
> > > but does not contain any panel or bridge node, even when the
> > > usual port/ports-based of graph is there.
> > > 
> > > In order to support both situations properly, this commit reworks
> > > the logic to try both options and not just one of the two: it will
> > > only return -EPROBE_DEFER when both have failed.
> > > 
> > 
> > Thanks for your patch Paul, it fixed a regression on a device where I
> > have a eDP bridge with an of_graph and a aux-bus defined.
> > 
> > But unfortunately it does not resolve the regression I have for the
> > USB based DisplayPort setup described below.
> > 
> > 
> > In the Qualcomm DisplayPort driver We're calling:
> > 
> > 	devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
> > 
> > and with the following DT snippet the behavior changed:
> > 
> > displayport-controller@ae90000 {
> > 	compatible = "qcom,sc8180x-dp";
> > 	...
> > 
> > 	operating-points-v2 = <&dp0_opp_table>;
> > 
> > 	ports {
> > 		#address-cells = <1>;
> > 		#size-cells = <0>;
> > 
> > 		port@0 {
> > 			reg = <0>;
> > 			dp0_in: endpoint {
> > 				remote-endpoint = <&display_driver>;
> > 			};
> > 		};
> > 	};
> > 
> > 	dp0_opp_table: opp-table {
> > 		...;
> > 	};
> > };
> > 
> > Prior to the introduction of 80253168dbfd ("drm: of: Lookup if child
> > node has panel or bridge") this would return -ENODEV, so we could
> > differentiate the case when we have a statically defined eDP panel from
> > that of a dynamically attached (over USB) DP panel.
> > 
> > Prior to your change, above case without the opp-table node would have
> > still returned -ENODEV.
> > 
> > But now this will just return -EPROBE_DEFER in both cases.
> 
> Oh that's right, the -ENODEV case was just completely removed by my change.
> Initially this would happen if !of_graph_is_present or if the remote node
> doesn't exist.
> 
> Now that we are also checking for child nodes, we can't just return -ENODEV
> when the graph or remote node is missing: we must also check that there is no
> child node that is a panel/bridge.
> 
> For the graph remote case, we can reliabily return -EPROBE_DEFER when
> of_graph_is_present and the remote exists and of_device_is_available.

Right, if I have a of_graph port@1 which references something that isn't
available we can reliably claim this is -EPROBE_DEFER.

> Otherwise we can go for -ENODEV.

Are you suggesting that if we find a "port" or "ports" we return -ENODEV
if we didn't find the requested port@N?

> I think getting -EPROBE_DEFER at this point
> should stop the drm_of_find_panel_or_bridge process.
> 

I think that makes sense, i.e. if we found an of_graph reference, but
it's not a panel yet.

> On the other hand for the child panel/bridge node case, I don't see how we
> can reliably distinguish between -EPROBE_DEFER and -ENODEV, because
> of_drm_find_panel and of_drm_find_bridge will behave the same if the child
> node is a not-yet-probed panel/bridge or a totally unrelated node.
> So I think we should always return -EPROBE_DEFER in that case.
> 
> As a result you can't get -ENODEV if using the of graph while having any
> (unrelated) child node there, so your issue remains.
> 
> Do you see any way we could make this work?
> 

I'm afraid I don't have any good suggestions on determining if that
child node is a panel/bridge or something else.

> > I thought the appropriate method of referencing the dsi panel was to
> > actually reference that using the of_graph, even though it's a child of
> > the dsi controller - that's at least how we've done it in e.g. [1].
> > I find this to be much nicer than to just blindly define that all
> > children of any sort of display controller must be a bridge or a panel.
> 
> Yes I totally agree. Given that using the child node directly apparently
> can't allow us to distinguish between -EPROBE_DEFER/-ENODEV I would be in
> favor of dropping this mechanism and going with explicit of graph in any case
> (even if it's a child node). I don't see any downside to this approach.
> 
> What do yout think?
> 

The explicit of_graph reference is a little bit clunky, but it's clear
and doesn't rely on "skipping" or only including names based on
particular names etc.

So I am in favour of reverting this back to the explicit reference.

Regards,
Bjorn

> Paul
> 
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sdm845-mtp.dts#n436
> > 
> > Regards,
> > Bjorn
> > 
> > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > Fixes: 80253168dbfd ("drm: of: Lookup if child node has panel or bridge")
> > > ---
> > > 
> > > Changes since v2:
> > > - Removed unnecessary else statement and added a comment about
> > >   clearing the panel pointer on error.
> > > 
> > > Changes since v1:
> > > - Renamed remote to node;
> > > - Renamed helper to find_panel_or_bridge;
> > > - Cleared bridge pointer early;
> > > - Returned early to make the code more concise;
> > > 
> > > ---
> > >  drivers/gpu/drm/drm_of.c | 99 ++++++++++++++++++++--------------------
> > >  1 file changed, 50 insertions(+), 49 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> > > index 9d90cd75c457..8716da6369a6 100644
> > > --- a/drivers/gpu/drm/drm_of.c
> > > +++ b/drivers/gpu/drm/drm_of.c
> > > @@ -219,6 +219,29 @@ int drm_of_encoder_active_endpoint(struct device_node *node,
> > >  }
> > >  EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint);
> > >  
> > > +static int find_panel_or_bridge(struct device_node *node,
> > > +				struct drm_panel **panel,
> > > +				struct drm_bridge **bridge)
> > > +{
> > > +	if (panel) {
> > > +		*panel = of_drm_find_panel(node);
> > > +		if (!IS_ERR(*panel))
> > > +			return 0;
> > > +
> > > +		/* Clear the panel pointer in case of error. */
> > > +		*panel = NULL;
> > > +	}
> > > +
> > > +	/* No panel found yet, check for a bridge next. */
> > > +	if (bridge) {
> > > +		*bridge = of_drm_find_bridge(node);
> > > +		if (*bridge)
> > > +			return 0;
> > > +	}
> > > +
> > > +	return -EPROBE_DEFER;
> > > +}
> > > +
> > >  /**
> > >   * drm_of_find_panel_or_bridge - return connected panel or bridge device
> > >   * @np: device tree node containing encoder output ports
> > > @@ -241,66 +264,44 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> > >  				struct drm_panel **panel,
> > >  				struct drm_bridge **bridge)
> > >  {
> > > -	int ret = -EPROBE_DEFER;
> > > -	struct device_node *remote;
> > > +	struct device_node *node;
> > > +	int ret;
> > >  
> > >  	if (!panel && !bridge)
> > >  		return -EINVAL;
> > > +
> > >  	if (panel)
> > >  		*panel = NULL;
> > > -
> > > -	/**
> > > -	 * Devices can also be child nodes when we also control that device
> > > -	 * through the upstream device (ie, MIPI-DCS for a MIPI-DSI device).
> > > -	 *
> > > -	 * Lookup for a child node of the given parent that isn't either port
> > > -	 * or ports.
> > > -	 */
> > > -	for_each_available_child_of_node(np, remote) {
> > > -		if (of_node_name_eq(remote, "port") ||
> > > -		    of_node_name_eq(remote, "ports"))
> > > -			continue;
> > > -
> > > -		goto of_find_panel_or_bridge;
> > > +	if (bridge)
> > > +		*bridge = NULL;
> > > +
> > > +	/* Check for a graph on the device node first. */
> > > +	if (of_graph_is_present(np)) {
> > > +		node = of_graph_get_remote_node(np, port, endpoint);
> > > +		if (node) {
> > > +			ret = find_panel_or_bridge(node, panel, bridge);
> > > +			of_node_put(node);
> > > +
> > > +			if (!ret)
> > > +				return 0;
> > > +		}
> > >  	}
> > >  
> > > -	/*
> > > -	 * 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 -ENODEV;
> > > -
> > > -	remote = of_graph_get_remote_node(np, port, endpoint);
> > > -
> > > -of_find_panel_or_bridge:
> > > -	if (!remote)
> > > -		return -ENODEV;
> > > +	/* Otherwise check for any child node other than port/ports. */
> > > +	for_each_available_child_of_node(np, node) {
> > > +		if (of_node_name_eq(node, "port") ||
> > > +		    of_node_name_eq(node, "ports"))
> > > +			continue;
> > >  
> > > -	if (panel) {
> > > -		*panel = of_drm_find_panel(remote);
> > > -		if (!IS_ERR(*panel))
> > > -			ret = 0;
> > > -		else
> > > -			*panel = NULL;
> > > -	}
> > > -
> > > -	/* No panel found yet, check for a bridge next. */
> > > -	if (bridge) {
> > > -		if (ret) {
> > > -			*bridge = of_drm_find_bridge(remote);
> > > -			if (*bridge)
> > > -				ret = 0;
> > > -		} else {
> > > -			*bridge = NULL;
> > > -		}
> > > +		ret = find_panel_or_bridge(node, panel, bridge);
> > > +		of_node_put(node);
> > >  
> > > +		/* Stop at the first found occurrence. */
> > > +		if (!ret)
> > > +			return 0;
> > >  	}
> > >  
> > > -	of_node_put(remote);
> > > -	return ret;
> > > +	return -EPROBE_DEFER;
> > >  }
> > >  EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
> > >  
> > > -- 
> > > 2.35.1
> > > 
> 
> -- 
> Paul Kocialkowski, Bootlin
> Embedded Linux and kernel engineering
> https://bootlin.com

Re: [PATCH v3] drm: of: Properly try all possible cases for bridge/panel detection

[Thierry Reding]

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

On Fri, Apr 01, 2022 at 09:44:46AM +0200, Paul Kocialkowski wrote:
> Hi Bjorn,
> 
> On Thu 31 Mar 22, 20:16, Bjorn Andersson wrote:
> > On Tue 29 Mar 06:27 PDT 2022, Paul Kocialkowski wrote:
> > 
> > > While bridge/panel detection was initially relying on the usual
> > > port/ports-based of graph detection, it was recently changed to
> > > perform the lookup on any child node that is not port/ports
> > > instead when such a node is available, with no fallback on the
> > > usual way.
> > > 
> > > This results in breaking detection when a child node is present
> > > but does not contain any panel or bridge node, even when the
> > > usual port/ports-based of graph is there.
> > > 
> > > In order to support both situations properly, this commit reworks
> > > the logic to try both options and not just one of the two: it will
> > > only return -EPROBE_DEFER when both have failed.
> > > 
> > 
> > Thanks for your patch Paul, it fixed a regression on a device where I
> > have a eDP bridge with an of_graph and a aux-bus defined.
> > 
> > But unfortunately it does not resolve the regression I have for the
> > USB based DisplayPort setup described below.
> > 
> > 
> > In the Qualcomm DisplayPort driver We're calling:
> > 
> > 	devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
> > 
> > and with the following DT snippet the behavior changed:
> > 
> > displayport-controller@ae90000 {
> > 	compatible = "qcom,sc8180x-dp";
> > 	...
> > 
> > 	operating-points-v2 = <&dp0_opp_table>;
> > 
> > 	ports {
> > 		#address-cells = <1>;
> > 		#size-cells = <0>;
> > 
> > 		port@0 {
> > 			reg = <0>;
> > 			dp0_in: endpoint {
> > 				remote-endpoint = <&display_driver>;
> > 			};
> > 		};
> > 	};
> > 
> > 	dp0_opp_table: opp-table {
> > 		...;
> > 	};
> > };
> > 
> > Prior to the introduction of 80253168dbfd ("drm: of: Lookup if child
> > node has panel or bridge") this would return -ENODEV, so we could
> > differentiate the case when we have a statically defined eDP panel from
> > that of a dynamically attached (over USB) DP panel.
> > 
> > Prior to your change, above case without the opp-table node would have
> > still returned -ENODEV.
> > 
> > But now this will just return -EPROBE_DEFER in both cases.
> 
> Oh that's right, the -ENODEV case was just completely removed by my change.
> Initially this would happen if !of_graph_is_present or if the remote node
> doesn't exist.
> 
> Now that we are also checking for child nodes, we can't just return -ENODEV
> when the graph or remote node is missing: we must also check that there is no
> child node that is a panel/bridge.
> 
> For the graph remote case, we can reliabily return -EPROBE_DEFER when
> of_graph_is_present and the remote exists and of_device_is_available.
> Otherwise we can go for -ENODEV. I think getting -EPROBE_DEFER at this point
> should stop the drm_of_find_panel_or_bridge process.
> 
> On the other hand for the child panel/bridge node case, I don't see how we
> can reliably distinguish between -EPROBE_DEFER and -ENODEV, because
> of_drm_find_panel and of_drm_find_bridge will behave the same if the child
> node is a not-yet-probed panel/bridge or a totally unrelated node.
> So I think we should always return -EPROBE_DEFER in that case.
> 
> As a result you can't get -ENODEV if using the of graph while having any
> (unrelated) child node there, so your issue remains.
> 
> Do you see any way we could make this work?
> 
> > I thought the appropriate method of referencing the dsi panel was to
> > actually reference that using the of_graph, even though it's a child of
> > the dsi controller - that's at least how we've done it in e.g. [1].
> > I find this to be much nicer than to just blindly define that all
> > children of any sort of display controller must be a bridge or a panel.
> 
> Yes I totally agree. Given that using the child node directly apparently
> can't allow us to distinguish between -EPROBE_DEFER/-ENODEV I would be in
> favor of dropping this mechanism and going with explicit of graph in any case
> (even if it's a child node). I don't see any downside to this approach.
> 
> What do yout think?

This patch has recently starting causing failures on various Tegra
devices that use drm_of_find_panel_or_bridge() for the case where the
output might be connected to an eDP or LVDS panel. However, that same
output could also be connected to an HDMI or DP monitor, in which case
we obviously don't want a DT representation because it's all
discoverable.

If I understand correctly, that's similar to what Bjorn described. In my
case I was able to fix the regression by returning -ENODEV at the very
end of the function (i.e. no matching ports were found and no graph is
present).

Thierry

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

Re: [PATCH v3] drm: of: Properly try all possible cases for bridge/panel detection

[Thierry Reding]

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

On Fri, Apr 01, 2022 at 09:44:46AM +0200, Paul Kocialkowski wrote:
> Hi Bjorn,
> 
> On Thu 31 Mar 22, 20:16, Bjorn Andersson wrote:
> > On Tue 29 Mar 06:27 PDT 2022, Paul Kocialkowski wrote:
> > 
> > > While bridge/panel detection was initially relying on the usual
> > > port/ports-based of graph detection, it was recently changed to
> > > perform the lookup on any child node that is not port/ports
> > > instead when such a node is available, with no fallback on the
> > > usual way.
> > > 
> > > This results in breaking detection when a child node is present
> > > but does not contain any panel or bridge node, even when the
> > > usual port/ports-based of graph is there.
> > > 
> > > In order to support both situations properly, this commit reworks
> > > the logic to try both options and not just one of the two: it will
> > > only return -EPROBE_DEFER when both have failed.
> > > 
> > 
> > Thanks for your patch Paul, it fixed a regression on a device where I
> > have a eDP bridge with an of_graph and a aux-bus defined.
> > 
> > But unfortunately it does not resolve the regression I have for the
> > USB based DisplayPort setup described below.
> > 
> > 
> > In the Qualcomm DisplayPort driver We're calling:
> > 
> > 	devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
> > 
> > and with the following DT snippet the behavior changed:
> > 
> > displayport-controller@ae90000 {
> > 	compatible = "qcom,sc8180x-dp";
> > 	...
> > 
> > 	operating-points-v2 = <&dp0_opp_table>;
> > 
> > 	ports {
> > 		#address-cells = <1>;
> > 		#size-cells = <0>;
> > 
> > 		port@0 {
> > 			reg = <0>;
> > 			dp0_in: endpoint {
> > 				remote-endpoint = <&display_driver>;
> > 			};
> > 		};
> > 	};
> > 
> > 	dp0_opp_table: opp-table {
> > 		...;
> > 	};
> > };
> > 
> > Prior to the introduction of 80253168dbfd ("drm: of: Lookup if child
> > node has panel or bridge") this would return -ENODEV, so we could
> > differentiate the case when we have a statically defined eDP panel from
> > that of a dynamically attached (over USB) DP panel.
> > 
> > Prior to your change, above case without the opp-table node would have
> > still returned -ENODEV.
> > 
> > But now this will just return -EPROBE_DEFER in both cases.
> 
> Oh that's right, the -ENODEV case was just completely removed by my change.
> Initially this would happen if !of_graph_is_present or if the remote node
> doesn't exist.
> 
> Now that we are also checking for child nodes, we can't just return -ENODEV
> when the graph or remote node is missing: we must also check that there is no
> child node that is a panel/bridge.
> 
> For the graph remote case, we can reliabily return -EPROBE_DEFER when
> of_graph_is_present and the remote exists and of_device_is_available.
> Otherwise we can go for -ENODEV. I think getting -EPROBE_DEFER at this point
> should stop the drm_of_find_panel_or_bridge process.
> 
> On the other hand for the child panel/bridge node case, I don't see how we
> can reliably distinguish between -EPROBE_DEFER and -ENODEV, because
> of_drm_find_panel and of_drm_find_bridge will behave the same if the child
> node is a not-yet-probed panel/bridge or a totally unrelated node.
> So I think we should always return -EPROBE_DEFER in that case.
> 
> As a result you can't get -ENODEV if using the of graph while having any
> (unrelated) child node there, so your issue remains.
> 
> Do you see any way we could make this work?
> 
> > I thought the appropriate method of referencing the dsi panel was to
> > actually reference that using the of_graph, even though it's a child of
> > the dsi controller - that's at least how we've done it in e.g. [1].
> > I find this to be much nicer than to just blindly define that all
> > children of any sort of display controller must be a bridge or a panel.
> 
> Yes I totally agree. Given that using the child node directly apparently
> can't allow us to distinguish between -EPROBE_DEFER/-ENODEV I would be in
> favor of dropping this mechanism and going with explicit of graph in any case
> (even if it's a child node). I don't see any downside to this approach.
> 
> What do yout think?

This patch has recently starting causing failures on various Tegra
devices that use drm_of_find_panel_or_bridge() for the case where the
output might be connected to an eDP or LVDS panel. However, that same
output could also be connected to an HDMI or DP monitor, in which case
we obviously don't want a DT representation because it's all
discoverable.

If I understand correctly, that's similar to what Bjorn described. In my
case I was able to fix the regression by returning -ENODEV at the very
end of the function (i.e. no matching ports were found and no graph is
present).

Thierry

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

Re: [PATCH v3] drm: of: Properly try all possible cases for bridge/panel detection

[Linus Walleij]

On Tue, Mar 29, 2022 at 3:27 PM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:

> While bridge/panel detection was initially relying on the usual
> port/ports-based of graph detection, it was recently changed to
> perform the lookup on any child node that is not port/ports
> instead when such a node is available, with no fallback on the
> usual way.
>
> This results in breaking detection when a child node is present
> but does not contain any panel or bridge node, even when the
> usual port/ports-based of graph is there.
>
> In order to support both situations properly, this commit reworks
> the logic to try both options and not just one of the two: it will
> only return -EPROBE_DEFER when both have failed.
>
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> Fixes: 80253168dbfd ("drm: of: Lookup if child node has panel or bridge")

This patch fixes the problems I have on the Ux500 MCDE with DPI
panels such as Janice, so:
Tested-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v3] drm: of: Properly try all possible cases for bridge/panel detection

[Linus Walleij]

On Tue, Mar 29, 2022 at 3:27 PM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:

> While bridge/panel detection was initially relying on the usual
> port/ports-based of graph detection, it was recently changed to
> perform the lookup on any child node that is not port/ports
> instead when such a node is available, with no fallback on the
> usual way.
>
> This results in breaking detection when a child node is present
> but does not contain any panel or bridge node, even when the
> usual port/ports-based of graph is there.
>
> In order to support both situations properly, this commit reworks
> the logic to try both options and not just one of the two: it will
> only return -EPROBE_DEFER when both have failed.
>
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> Fixes: 80253168dbfd ("drm: of: Lookup if child node has panel or bridge")

This patch fixes the problems I have on the Ux500 MCDE with DPI
panels such as Janice, so:
Tested-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v3] drm: of: Properly try all possible cases for bridge/panel detection

[Paul Kocialkowski]

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

Hi Thierry,

On Tue 05 Apr 22, 19:00, Thierry Reding wrote:
> On Fri, Apr 01, 2022 at 09:44:46AM +0200, Paul Kocialkowski wrote:
> > Hi Bjorn,
> > 
> > On Thu 31 Mar 22, 20:16, Bjorn Andersson wrote:
> > > On Tue 29 Mar 06:27 PDT 2022, Paul Kocialkowski wrote:
> > > 
> > > > While bridge/panel detection was initially relying on the usual
> > > > port/ports-based of graph detection, it was recently changed to
> > > > perform the lookup on any child node that is not port/ports
> > > > instead when such a node is available, with no fallback on the
> > > > usual way.
> > > > 
> > > > This results in breaking detection when a child node is present
> > > > but does not contain any panel or bridge node, even when the
> > > > usual port/ports-based of graph is there.
> > > > 
> > > > In order to support both situations properly, this commit reworks
> > > > the logic to try both options and not just one of the two: it will
> > > > only return -EPROBE_DEFER when both have failed.
> > > > 
> > > 
> > > Thanks for your patch Paul, it fixed a regression on a device where I
> > > have a eDP bridge with an of_graph and a aux-bus defined.
> > > 
> > > But unfortunately it does not resolve the regression I have for the
> > > USB based DisplayPort setup described below.
> > > 
> > > 
> > > In the Qualcomm DisplayPort driver We're calling:
> > > 
> > > 	devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
> > > 
> > > and with the following DT snippet the behavior changed:
> > > 
> > > displayport-controller@ae90000 {
> > > 	compatible = "qcom,sc8180x-dp";
> > > 	...
> > > 
> > > 	operating-points-v2 = <&dp0_opp_table>;
> > > 
> > > 	ports {
> > > 		#address-cells = <1>;
> > > 		#size-cells = <0>;
> > > 
> > > 		port@0 {
> > > 			reg = <0>;
> > > 			dp0_in: endpoint {
> > > 				remote-endpoint = <&display_driver>;
> > > 			};
> > > 		};
> > > 	};
> > > 
> > > 	dp0_opp_table: opp-table {
> > > 		...;
> > > 	};
> > > };
> > > 
> > > Prior to the introduction of 80253168dbfd ("drm: of: Lookup if child
> > > node has panel or bridge") this would return -ENODEV, so we could
> > > differentiate the case when we have a statically defined eDP panel from
> > > that of a dynamically attached (over USB) DP panel.
> > > 
> > > Prior to your change, above case without the opp-table node would have
> > > still returned -ENODEV.
> > > 
> > > But now this will just return -EPROBE_DEFER in both cases.
> > 
> > Oh that's right, the -ENODEV case was just completely removed by my change.
> > Initially this would happen if !of_graph_is_present or if the remote node
> > doesn't exist.
> > 
> > Now that we are also checking for child nodes, we can't just return -ENODEV
> > when the graph or remote node is missing: we must also check that there is no
> > child node that is a panel/bridge.
> > 
> > For the graph remote case, we can reliabily return -EPROBE_DEFER when
> > of_graph_is_present and the remote exists and of_device_is_available.
> > Otherwise we can go for -ENODEV. I think getting -EPROBE_DEFER at this point
> > should stop the drm_of_find_panel_or_bridge process.
> > 
> > On the other hand for the child panel/bridge node case, I don't see how we
> > can reliably distinguish between -EPROBE_DEFER and -ENODEV, because
> > of_drm_find_panel and of_drm_find_bridge will behave the same if the child
> > node is a not-yet-probed panel/bridge or a totally unrelated node.
> > So I think we should always return -EPROBE_DEFER in that case.
> > 
> > As a result you can't get -ENODEV if using the of graph while having any
> > (unrelated) child node there, so your issue remains.
> > 
> > Do you see any way we could make this work?
> > 
> > > I thought the appropriate method of referencing the dsi panel was to
> > > actually reference that using the of_graph, even though it's a child of
> > > the dsi controller - that's at least how we've done it in e.g. [1].
> > > I find this to be much nicer than to just blindly define that all
> > > children of any sort of display controller must be a bridge or a panel.
> > 
> > Yes I totally agree. Given that using the child node directly apparently
> > can't allow us to distinguish between -EPROBE_DEFER/-ENODEV I would be in
> > favor of dropping this mechanism and going with explicit of graph in any case
> > (even if it's a child node). I don't see any downside to this approach.
> > 
> > What do yout think?
> 
> This patch has recently starting causing failures on various Tegra
> devices that use drm_of_find_panel_or_bridge() for the case where the
> output might be connected to an eDP or LVDS panel. However, that same
> output could also be connected to an HDMI or DP monitor, in which case
> we obviously don't want a DT representation because it's all
> discoverable.
> 
> If I understand correctly, that's similar to what Bjorn described. In my
> case I was able to fix the regression by returning -ENODEV at the very
> end of the function (i.e. no matching ports were found and no graph is
> present).

Okay so I'll send a patch that returns -ENODEV when possible, but there
is still a case where we can't distinguish between -EPROBE_DEFER and -ENODEV,
which calls for removing the whole child node-based mechanism.

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

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

Re: [PATCH v3] drm: of: Properly try all possible cases for bridge/panel detection

[Paul Kocialkowski]

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

Hi Thierry,

On Tue 05 Apr 22, 19:00, Thierry Reding wrote:
> On Fri, Apr 01, 2022 at 09:44:46AM +0200, Paul Kocialkowski wrote:
> > Hi Bjorn,
> > 
> > On Thu 31 Mar 22, 20:16, Bjorn Andersson wrote:
> > > On Tue 29 Mar 06:27 PDT 2022, Paul Kocialkowski wrote:
> > > 
> > > > While bridge/panel detection was initially relying on the usual
> > > > port/ports-based of graph detection, it was recently changed to
> > > > perform the lookup on any child node that is not port/ports
> > > > instead when such a node is available, with no fallback on the
> > > > usual way.
> > > > 
> > > > This results in breaking detection when a child node is present
> > > > but does not contain any panel or bridge node, even when the
> > > > usual port/ports-based of graph is there.
> > > > 
> > > > In order to support both situations properly, this commit reworks
> > > > the logic to try both options and not just one of the two: it will
> > > > only return -EPROBE_DEFER when both have failed.
> > > > 
> > > 
> > > Thanks for your patch Paul, it fixed a regression on a device where I
> > > have a eDP bridge with an of_graph and a aux-bus defined.
> > > 
> > > But unfortunately it does not resolve the regression I have for the
> > > USB based DisplayPort setup described below.
> > > 
> > > 
> > > In the Qualcomm DisplayPort driver We're calling:
> > > 
> > > 	devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
> > > 
> > > and with the following DT snippet the behavior changed:
> > > 
> > > displayport-controller@ae90000 {
> > > 	compatible = "qcom,sc8180x-dp";
> > > 	...
> > > 
> > > 	operating-points-v2 = <&dp0_opp_table>;
> > > 
> > > 	ports {
> > > 		#address-cells = <1>;
> > > 		#size-cells = <0>;
> > > 
> > > 		port@0 {
> > > 			reg = <0>;
> > > 			dp0_in: endpoint {
> > > 				remote-endpoint = <&display_driver>;
> > > 			};
> > > 		};
> > > 	};
> > > 
> > > 	dp0_opp_table: opp-table {
> > > 		...;
> > > 	};
> > > };
> > > 
> > > Prior to the introduction of 80253168dbfd ("drm: of: Lookup if child
> > > node has panel or bridge") this would return -ENODEV, so we could
> > > differentiate the case when we have a statically defined eDP panel from
> > > that of a dynamically attached (over USB) DP panel.
> > > 
> > > Prior to your change, above case without the opp-table node would have
> > > still returned -ENODEV.
> > > 
> > > But now this will just return -EPROBE_DEFER in both cases.
> > 
> > Oh that's right, the -ENODEV case was just completely removed by my change.
> > Initially this would happen if !of_graph_is_present or if the remote node
> > doesn't exist.
> > 
> > Now that we are also checking for child nodes, we can't just return -ENODEV
> > when the graph or remote node is missing: we must also check that there is no
> > child node that is a panel/bridge.
> > 
> > For the graph remote case, we can reliabily return -EPROBE_DEFER when
> > of_graph_is_present and the remote exists and of_device_is_available.
> > Otherwise we can go for -ENODEV. I think getting -EPROBE_DEFER at this point
> > should stop the drm_of_find_panel_or_bridge process.
> > 
> > On the other hand for the child panel/bridge node case, I don't see how we
> > can reliably distinguish between -EPROBE_DEFER and -ENODEV, because
> > of_drm_find_panel and of_drm_find_bridge will behave the same if the child
> > node is a not-yet-probed panel/bridge or a totally unrelated node.
> > So I think we should always return -EPROBE_DEFER in that case.
> > 
> > As a result you can't get -ENODEV if using the of graph while having any
> > (unrelated) child node there, so your issue remains.
> > 
> > Do you see any way we could make this work?
> > 
> > > I thought the appropriate method of referencing the dsi panel was to
> > > actually reference that using the of_graph, even though it's a child of
> > > the dsi controller - that's at least how we've done it in e.g. [1].
> > > I find this to be much nicer than to just blindly define that all
> > > children of any sort of display controller must be a bridge or a panel.
> > 
> > Yes I totally agree. Given that using the child node directly apparently
> > can't allow us to distinguish between -EPROBE_DEFER/-ENODEV I would be in
> > favor of dropping this mechanism and going with explicit of graph in any case
> > (even if it's a child node). I don't see any downside to this approach.
> > 
> > What do yout think?
> 
> This patch has recently starting causing failures on various Tegra
> devices that use drm_of_find_panel_or_bridge() for the case where the
> output might be connected to an eDP or LVDS panel. However, that same
> output could also be connected to an HDMI or DP monitor, in which case
> we obviously don't want a DT representation because it's all
> discoverable.
> 
> If I understand correctly, that's similar to what Bjorn described. In my
> case I was able to fix the regression by returning -ENODEV at the very
> end of the function (i.e. no matching ports were found and no graph is
> present).

Okay so I'll send a patch that returns -ENODEV when possible, but there
is still a case where we can't distinguish between -EPROBE_DEFER and -ENODEV,
which calls for removing the whole child node-based mechanism.

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

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

Re: [PATCH v3] drm: of: Properly try all possible cases for bridge/panel detection

[Paul Cercueil]

Hi Paul,

This patch breaks the ingenic-drm driver.

It calls drm_of_find_panel_or_bridge(np, 0, i, ...) starting for i=0, until
-ENODEV is returned, which does not happen anymore.

The idea is to probe all the connected panels/bridges, should it be done
differently now?

Cheers,
-Paul

Re: [PATCH v3] drm: of: Properly try all possible cases for bridge/panel detection

[Paul Cercueil]

Hi Paul,

This patch breaks the ingenic-drm driver.

It calls drm_of_find_panel_or_bridge(np, 0, i, ...) starting for i=0, until
-ENODEV is returned, which does not happen anymore.

The idea is to probe all the connected panels/bridges, should it be done
differently now?

Cheers,
-Paul

Re: [PATCH v3] drm: of: Properly try all possible cases for bridge/panel detection

[Paul Kocialkowski]

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

Hi Paul,

On Sat 16 Apr 22, 23:21, Paul Cercueil wrote:
> Hi Paul,
> 
> This patch breaks the ingenic-drm driver.
> 
> It calls drm_of_find_panel_or_bridge(np, 0, i, ...) starting for i=0, until
> -ENODEV is returned, which does not happen anymore.
> 
> The idea is to probe all the connected panels/bridges, should it be done
> differently now?

I've sent out a different patch which restores -ENODEV at:
https://patchwork.freedesktop.org/patch/481135/

Feel free to try it and reply with tested-by/reviewed-by there.
I've also made a proposal in the thread to skip the "child node" mechanism
as soon as an of graph is present, which would allow covering more legit
cases with -ENODEV (the patch linked above doesn't cover all cases that
need -ENODEV).

Ideally we'd like to remove the child node mechanism entirely, but it may
already be part of a device-tree binding spec.

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

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

Re: [PATCH v3] drm: of: Properly try all possible cases for bridge/panel detection

[Paul Kocialkowski]

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

Hi Paul,

On Sat 16 Apr 22, 23:21, Paul Cercueil wrote:
> Hi Paul,
> 
> This patch breaks the ingenic-drm driver.
> 
> It calls drm_of_find_panel_or_bridge(np, 0, i, ...) starting for i=0, until
> -ENODEV is returned, which does not happen anymore.
> 
> The idea is to probe all the connected panels/bridges, should it be done
> differently now?

I've sent out a different patch which restores -ENODEV at:
https://patchwork.freedesktop.org/patch/481135/

Feel free to try it and reply with tested-by/reviewed-by there.
I've also made a proposal in the thread to skip the "child node" mechanism
as soon as an of graph is present, which would allow covering more legit
cases with -ENODEV (the patch linked above doesn't cover all cases that
need -ENODEV).

Ideally we'd like to remove the child node mechanism entirely, but it may
already be part of a device-tree binding spec.

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

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

Re: [PATCH v3] drm: of: Properly try all possible cases for bridge/panel detection

[Rob Clark]

On Tue, Apr 5, 2022 at 10:01 AM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Fri, Apr 01, 2022 at 09:44:46AM +0200, Paul Kocialkowski wrote:
> > Hi Bjorn,
> >
> > On Thu 31 Mar 22, 20:16, Bjorn Andersson wrote:
> > > On Tue 29 Mar 06:27 PDT 2022, Paul Kocialkowski wrote:
> > >
> > > > While bridge/panel detection was initially relying on the usual
> > > > port/ports-based of graph detection, it was recently changed to
> > > > perform the lookup on any child node that is not port/ports
> > > > instead when such a node is available, with no fallback on the
> > > > usual way.
> > > >
> > > > This results in breaking detection when a child node is present
> > > > but does not contain any panel or bridge node, even when the
> > > > usual port/ports-based of graph is there.
> > > >
> > > > In order to support both situations properly, this commit reworks
> > > > the logic to try both options and not just one of the two: it will
> > > > only return -EPROBE_DEFER when both have failed.
> > > >
> > >
> > > Thanks for your patch Paul, it fixed a regression on a device where I
> > > have a eDP bridge with an of_graph and a aux-bus defined.
> > >
> > > But unfortunately it does not resolve the regression I have for the
> > > USB based DisplayPort setup described below.
> > >
> > >
> > > In the Qualcomm DisplayPort driver We're calling:
> > >
> > >     devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
> > >
> > > and with the following DT snippet the behavior changed:
> > >
> > > displayport-controller@ae90000 {
> > >     compatible = "qcom,sc8180x-dp";
> > >     ...
> > >
> > >     operating-points-v2 = <&dp0_opp_table>;
> > >
> > >     ports {
> > >             #address-cells = <1>;
> > >             #size-cells = <0>;
> > >
> > >             port@0 {
> > >                     reg = <0>;
> > >                     dp0_in: endpoint {
> > >                             remote-endpoint = <&display_driver>;
> > >                     };
> > >             };
> > >     };
> > >
> > >     dp0_opp_table: opp-table {
> > >             ...;
> > >     };
> > > };
> > >
> > > Prior to the introduction of 80253168dbfd ("drm: of: Lookup if child
> > > node has panel or bridge") this would return -ENODEV, so we could
> > > differentiate the case when we have a statically defined eDP panel from
> > > that of a dynamically attached (over USB) DP panel.
> > >
> > > Prior to your change, above case without the opp-table node would have
> > > still returned -ENODEV.
> > >
> > > But now this will just return -EPROBE_DEFER in both cases.
> >
> > Oh that's right, the -ENODEV case was just completely removed by my change.
> > Initially this would happen if !of_graph_is_present or if the remote node
> > doesn't exist.
> >
> > Now that we are also checking for child nodes, we can't just return -ENODEV
> > when the graph or remote node is missing: we must also check that there is no
> > child node that is a panel/bridge.
> >
> > For the graph remote case, we can reliabily return -EPROBE_DEFER when
> > of_graph_is_present and the remote exists and of_device_is_available.
> > Otherwise we can go for -ENODEV. I think getting -EPROBE_DEFER at this point
> > should stop the drm_of_find_panel_or_bridge process.
> >
> > On the other hand for the child panel/bridge node case, I don't see how we
> > can reliably distinguish between -EPROBE_DEFER and -ENODEV, because
> > of_drm_find_panel and of_drm_find_bridge will behave the same if the child
> > node is a not-yet-probed panel/bridge or a totally unrelated node.
> > So I think we should always return -EPROBE_DEFER in that case.
> >
> > As a result you can't get -ENODEV if using the of graph while having any
> > (unrelated) child node there, so your issue remains.
> >
> > Do you see any way we could make this work?
> >
> > > I thought the appropriate method of referencing the dsi panel was to
> > > actually reference that using the of_graph, even though it's a child of
> > > the dsi controller - that's at least how we've done it in e.g. [1].
> > > I find this to be much nicer than to just blindly define that all
> > > children of any sort of display controller must be a bridge or a panel.
> >
> > Yes I totally agree. Given that using the child node directly apparently
> > can't allow us to distinguish between -EPROBE_DEFER/-ENODEV I would be in
> > favor of dropping this mechanism and going with explicit of graph in any case
> > (even if it's a child node). I don't see any downside to this approach.
> >
> > What do yout think?
>
> This patch has recently starting causing failures on various Tegra
> devices that use drm_of_find_panel_or_bridge() for the case where the
> output might be connected to an eDP or LVDS panel. However, that same
> output could also be connected to an HDMI or DP monitor, in which case
> we obviously don't want a DT representation because it's all
> discoverable.
>
> If I understand correctly, that's similar to what Bjorn described. In my
> case I was able to fix the regression by returning -ENODEV at the very
> end of the function (i.e. no matching ports were found and no graph is
> present).
>

All the qcom laptops/chromebooks with eDP bridge are also still broken
as of -rc3.. I need to revert the following:

67bae5f28c89 (drm: of: Properly try all possible cases for
bridge/panel detection, 2022-03-29)
80253168dbfd (drm: of: Lookup if child node has panel or bridge, 2022-02-02)

Since it is already -rc3 could we just go ahead and revert those two
and try again next time?

BR,
-R

Re: [PATCH v3] drm: of: Properly try all possible cases for bridge/panel detection

[Rob Clark]

On Tue, Apr 5, 2022 at 10:01 AM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Fri, Apr 01, 2022 at 09:44:46AM +0200, Paul Kocialkowski wrote:
> > Hi Bjorn,
> >
> > On Thu 31 Mar 22, 20:16, Bjorn Andersson wrote:
> > > On Tue 29 Mar 06:27 PDT 2022, Paul Kocialkowski wrote:
> > >
> > > > While bridge/panel detection was initially relying on the usual
> > > > port/ports-based of graph detection, it was recently changed to
> > > > perform the lookup on any child node that is not port/ports
> > > > instead when such a node is available, with no fallback on the
> > > > usual way.
> > > >
> > > > This results in breaking detection when a child node is present
> > > > but does not contain any panel or bridge node, even when the
> > > > usual port/ports-based of graph is there.
> > > >
> > > > In order to support both situations properly, this commit reworks
> > > > the logic to try both options and not just one of the two: it will
> > > > only return -EPROBE_DEFER when both have failed.
> > > >
> > >
> > > Thanks for your patch Paul, it fixed a regression on a device where I
> > > have a eDP bridge with an of_graph and a aux-bus defined.
> > >
> > > But unfortunately it does not resolve the regression I have for the
> > > USB based DisplayPort setup described below.
> > >
> > >
> > > In the Qualcomm DisplayPort driver We're calling:
> > >
> > >     devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
> > >
> > > and with the following DT snippet the behavior changed:
> > >
> > > displayport-controller@ae90000 {
> > >     compatible = "qcom,sc8180x-dp";
> > >     ...
> > >
> > >     operating-points-v2 = <&dp0_opp_table>;
> > >
> > >     ports {
> > >             #address-cells = <1>;
> > >             #size-cells = <0>;
> > >
> > >             port@0 {
> > >                     reg = <0>;
> > >                     dp0_in: endpoint {
> > >                             remote-endpoint = <&display_driver>;
> > >                     };
> > >             };
> > >     };
> > >
> > >     dp0_opp_table: opp-table {
> > >             ...;
> > >     };
> > > };
> > >
> > > Prior to the introduction of 80253168dbfd ("drm: of: Lookup if child
> > > node has panel or bridge") this would return -ENODEV, so we could
> > > differentiate the case when we have a statically defined eDP panel from
> > > that of a dynamically attached (over USB) DP panel.
> > >
> > > Prior to your change, above case without the opp-table node would have
> > > still returned -ENODEV.
> > >
> > > But now this will just return -EPROBE_DEFER in both cases.
> >
> > Oh that's right, the -ENODEV case was just completely removed by my change.
> > Initially this would happen if !of_graph_is_present or if the remote node
> > doesn't exist.
> >
> > Now that we are also checking for child nodes, we can't just return -ENODEV
> > when the graph or remote node is missing: we must also check that there is no
> > child node that is a panel/bridge.
> >
> > For the graph remote case, we can reliabily return -EPROBE_DEFER when
> > of_graph_is_present and the remote exists and of_device_is_available.
> > Otherwise we can go for -ENODEV. I think getting -EPROBE_DEFER at this point
> > should stop the drm_of_find_panel_or_bridge process.
> >
> > On the other hand for the child panel/bridge node case, I don't see how we
> > can reliably distinguish between -EPROBE_DEFER and -ENODEV, because
> > of_drm_find_panel and of_drm_find_bridge will behave the same if the child
> > node is a not-yet-probed panel/bridge or a totally unrelated node.
> > So I think we should always return -EPROBE_DEFER in that case.
> >
> > As a result you can't get -ENODEV if using the of graph while having any
> > (unrelated) child node there, so your issue remains.
> >
> > Do you see any way we could make this work?
> >
> > > I thought the appropriate method of referencing the dsi panel was to
> > > actually reference that using the of_graph, even though it's a child of
> > > the dsi controller - that's at least how we've done it in e.g. [1].
> > > I find this to be much nicer than to just blindly define that all
> > > children of any sort of display controller must be a bridge or a panel.
> >
> > Yes I totally agree. Given that using the child node directly apparently
> > can't allow us to distinguish between -EPROBE_DEFER/-ENODEV I would be in
> > favor of dropping this mechanism and going with explicit of graph in any case
> > (even if it's a child node). I don't see any downside to this approach.
> >
> > What do yout think?
>
> This patch has recently starting causing failures on various Tegra
> devices that use drm_of_find_panel_or_bridge() for the case where the
> output might be connected to an eDP or LVDS panel. However, that same
> output could also be connected to an HDMI or DP monitor, in which case
> we obviously don't want a DT representation because it's all
> discoverable.
>
> If I understand correctly, that's similar to what Bjorn described. In my
> case I was able to fix the regression by returning -ENODEV at the very
> end of the function (i.e. no matching ports were found and no graph is
> present).
>

All the qcom laptops/chromebooks with eDP bridge are also still broken
as of -rc3.. I need to revert the following:

67bae5f28c89 (drm: of: Properly try all possible cases for
bridge/panel detection, 2022-03-29)
80253168dbfd (drm: of: Lookup if child node has panel or bridge, 2022-02-02)

Since it is already -rc3 could we just go ahead and revert those two
and try again next time?

BR,
-R