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...
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