| Message ID | 20220329132732.628474-1-paul.kocialkowski@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] drm: of: Properly try all possible cases for bridge/panel detection | expand |
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
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 >
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 > >
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
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
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
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
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
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
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
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);
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(-)