* [PATCH] drm: of: Improve error handling in bridge/panel detection
@ 2022-04-07 9:34 ` Paul Kocialkowski
0 siblings, 0 replies; 10+ messages in thread
From: Paul Kocialkowski @ 2022-04-07 9:34 UTC (permalink / raw)
To: dri-devel, linux-kernel
Cc: Thomas Zimmermann, David Airlie, Thomas Petazzoni,
Bjorn Andersson, Paul Kocialkowski, Thierry Reding, Jagan Teki
With the previous rework of drm_of_find_panel_or_bridge only
-EPROBE_DEFER is returned while previous behavior allowed -ENODEV
to be returned when the port/endpoint is either missing or unavailable.
Make the default return code of the function -ENODEV to handle this and
only return -EPROBE_DEFER in find_panel_or_bridge when the of device is
available but not yet registered. Also return the error code whenever
the remote node exists to avoid checking for child nodes.
Checking child nodes could result in -EPROBE_DEFER returned by
find_panel_or_bridge with an unrelated child node that would overwrite
a legitimate -ENODEV from find_panel_or_bridge if the remote node from
the of graph is unavailable. This happens because find_panel_or_bridge
has no way to distinguish between a legitimate panel/bridge node that
isn't yet registered and an unrelated node.
Add comments around to clarify this behavior.
Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Fixes: 67bae5f28c89 ("drm: of: Properly try all possible cases for bridge/panel detection")
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
---
drivers/gpu/drm/drm_of.c | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index 8716da6369a6..97ea9d2016ff 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -223,6 +223,9 @@ static int find_panel_or_bridge(struct device_node *node,
struct drm_panel **panel,
struct drm_bridge **bridge)
{
+ if (!of_device_is_available(node))
+ return -ENODEV;
+
if (panel) {
*panel = of_drm_find_panel(node);
if (!IS_ERR(*panel))
@@ -265,7 +268,7 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
struct drm_bridge **bridge)
{
struct device_node *node;
- int ret;
+ int ret = -ENODEV;
if (!panel && !bridge)
return -EINVAL;
@@ -282,8 +285,12 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
ret = find_panel_or_bridge(node, panel, bridge);
of_node_put(node);
- if (!ret)
- return 0;
+ /*
+ * If the graph/remote node is present we consider it
+ * to be the legitimate candidate here and return
+ * whatever code we got from find_panel_or_bridge.
+ */
+ return ret;
}
}
@@ -296,12 +303,18 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
ret = find_panel_or_bridge(node, panel, bridge);
of_node_put(node);
- /* Stop at the first found occurrence. */
+ /*
+ * Note that an unrelated (available) child node will cause
+ * find_panel_or_bridge to return -EPROBE_DEFER because there
+ * is no way to distinguish the node from a legitimate
+ * panel/bridge that didn't register yet. Keep iterating nodes
+ * and only return on the first found occurrence.
+ */
if (!ret)
return 0;
}
- return -EPROBE_DEFER;
+ return ret;
}
EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
--
2.35.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] drm: of: Improve error handling in bridge/panel detection
2022-04-07 9:34 ` Paul Kocialkowski
@ 2022-04-09 3:09 ` Bjorn Andersson
-1 siblings, 0 replies; 10+ messages in thread
From: Bjorn Andersson @ 2022-04-09 3:09 UTC (permalink / raw)
To: Paul Kocialkowski
Cc: dri-devel, linux-kernel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Daniel Vetter, Jagan Teki,
Thomas Petazzoni, Thierry Reding, Linus Walleij
On Thu 07 Apr 04:34 CDT 2022, Paul Kocialkowski wrote:
> With the previous rework of drm_of_find_panel_or_bridge only
> -EPROBE_DEFER is returned while previous behavior allowed -ENODEV
> to be returned when the port/endpoint is either missing or unavailable.
>
> Make the default return code of the function -ENODEV to handle this and
> only return -EPROBE_DEFER in find_panel_or_bridge when the of device is
> available but not yet registered. Also return the error code whenever
> the remote node exists to avoid checking for child nodes.
>
> Checking child nodes could result in -EPROBE_DEFER returned by
> find_panel_or_bridge with an unrelated child node that would overwrite
> a legitimate -ENODEV from find_panel_or_bridge if the remote node from
> the of graph is unavailable. This happens because find_panel_or_bridge
> has no way to distinguish between a legitimate panel/bridge node that
> isn't yet registered and an unrelated node.
>
> Add comments around to clarify this behavior.
>
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> Fixes: 67bae5f28c89 ("drm: of: Properly try all possible cases for bridge/panel detection")
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
>
Thanks for your patch, this does seem to solve the first problem I
reported, where I have a DisplayPort bridge with the following content:
sn65dsi86: bridge@2c {
compatible = "ti,sn65dsi86";
...;
ports {
port@0 {
reg = <0>;
sn65dsi86_in_a: endpoint {
remote-endpoint = <&dsi0_out>;
};
};
port@1 {
reg = <1>;
sn65dsi86_out: endpoint {
remote-endpoint = <&panel_in_edp>;
};
};
};
aux-bus {
panel: panel {
compatible = "boe,nv133fhm-n61";
backlight = <&backlight>;
port {
panel_in_edp: endpoint {
remote-endpoint = <&sn65dsi86_out>;
};
};
};
};
};
The code now finds a match on of_graph_get_remote_node() and returns 0
or -EPROBE_DEFER from find_panel_or_bridge(?, 1, ?). And we return this,
before failing to resolve the "aux-bus" as a panel.
But my other case still doesn't work:
mdss_dp: displayport-controller@ae90000 {
compatible = "qcom,sm8350-dp";
...;
operating-points-v2 = <&dp_opp_table>;
ports {
port@0 {
reg = <0>;
dp_in: endpoint {
remote-endpoint = <&dpu_intf0_out>;
};
};
};
dp_opp_table: opp-table {
...
};
};
port@1 may be a reference to a DisplayPort panel, but in this particular
case the output is a USB Type-c connector (compatible
"usb-c-connector"). So I'm not able to specify this link, given that it
will not be a bridge or panel...ever...
So this does not find a match on of_graph_get_remote_node(np, 1, ?), so
we move ahead and look at all children not named "port" or "ports". We
find the opp-table and concludes that this not a panel. At this point
ret is overwritten and we end up returning -EPROBE_DEFER.
I think it's worth reverting back to the explicit of_graph link to the
panel, even in the case that it's an immediate child node. It avoids the
problem of specifying that all future display nodes must only have
children of type ports, port or panel. We might be able to come up with
something that works for my case, but it seems fragile and not very
future proof. The explicit port is a little bit clunky, but it doesn't
have this problem.
Regards,
Bjorn
> ---
> drivers/gpu/drm/drm_of.c | 23 ++++++++++++++++++-----
> 1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index 8716da6369a6..97ea9d2016ff 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -223,6 +223,9 @@ static int find_panel_or_bridge(struct device_node *node,
> struct drm_panel **panel,
> struct drm_bridge **bridge)
> {
> + if (!of_device_is_available(node))
> + return -ENODEV;
> +
> if (panel) {
> *panel = of_drm_find_panel(node);
> if (!IS_ERR(*panel))
> @@ -265,7 +268,7 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> struct drm_bridge **bridge)
> {
> struct device_node *node;
> - int ret;
> + int ret = -ENODEV;
>
> if (!panel && !bridge)
> return -EINVAL;
> @@ -282,8 +285,12 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> ret = find_panel_or_bridge(node, panel, bridge);
> of_node_put(node);
>
> - if (!ret)
> - return 0;
> + /*
> + * If the graph/remote node is present we consider it
> + * to be the legitimate candidate here and return
> + * whatever code we got from find_panel_or_bridge.
> + */
> + return ret;
> }
> }
>
> @@ -296,12 +303,18 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> ret = find_panel_or_bridge(node, panel, bridge);
> of_node_put(node);
>
> - /* Stop at the first found occurrence. */
> + /*
> + * Note that an unrelated (available) child node will cause
> + * find_panel_or_bridge to return -EPROBE_DEFER because there
> + * is no way to distinguish the node from a legitimate
> + * panel/bridge that didn't register yet. Keep iterating nodes
> + * and only return on the first found occurrence.
> + */
> if (!ret)
> return 0;
> }
>
> - return -EPROBE_DEFER;
> + return ret;
> }
> EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
>
> --
> 2.35.1
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] drm: of: Improve error handling in bridge/panel detection
@ 2022-04-09 3:09 ` Bjorn Andersson
0 siblings, 0 replies; 10+ messages in thread
From: Bjorn Andersson @ 2022-04-09 3:09 UTC (permalink / raw)
To: Paul Kocialkowski
Cc: Thomas Petazzoni, David Airlie, linux-kernel, Thierry Reding,
dri-devel, Thomas Zimmermann, Jagan Teki
On Thu 07 Apr 04:34 CDT 2022, Paul Kocialkowski wrote:
> With the previous rework of drm_of_find_panel_or_bridge only
> -EPROBE_DEFER is returned while previous behavior allowed -ENODEV
> to be returned when the port/endpoint is either missing or unavailable.
>
> Make the default return code of the function -ENODEV to handle this and
> only return -EPROBE_DEFER in find_panel_or_bridge when the of device is
> available but not yet registered. Also return the error code whenever
> the remote node exists to avoid checking for child nodes.
>
> Checking child nodes could result in -EPROBE_DEFER returned by
> find_panel_or_bridge with an unrelated child node that would overwrite
> a legitimate -ENODEV from find_panel_or_bridge if the remote node from
> the of graph is unavailable. This happens because find_panel_or_bridge
> has no way to distinguish between a legitimate panel/bridge node that
> isn't yet registered and an unrelated node.
>
> Add comments around to clarify this behavior.
>
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> Fixes: 67bae5f28c89 ("drm: of: Properly try all possible cases for bridge/panel detection")
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
>
Thanks for your patch, this does seem to solve the first problem I
reported, where I have a DisplayPort bridge with the following content:
sn65dsi86: bridge@2c {
compatible = "ti,sn65dsi86";
...;
ports {
port@0 {
reg = <0>;
sn65dsi86_in_a: endpoint {
remote-endpoint = <&dsi0_out>;
};
};
port@1 {
reg = <1>;
sn65dsi86_out: endpoint {
remote-endpoint = <&panel_in_edp>;
};
};
};
aux-bus {
panel: panel {
compatible = "boe,nv133fhm-n61";
backlight = <&backlight>;
port {
panel_in_edp: endpoint {
remote-endpoint = <&sn65dsi86_out>;
};
};
};
};
};
The code now finds a match on of_graph_get_remote_node() and returns 0
or -EPROBE_DEFER from find_panel_or_bridge(?, 1, ?). And we return this,
before failing to resolve the "aux-bus" as a panel.
But my other case still doesn't work:
mdss_dp: displayport-controller@ae90000 {
compatible = "qcom,sm8350-dp";
...;
operating-points-v2 = <&dp_opp_table>;
ports {
port@0 {
reg = <0>;
dp_in: endpoint {
remote-endpoint = <&dpu_intf0_out>;
};
};
};
dp_opp_table: opp-table {
...
};
};
port@1 may be a reference to a DisplayPort panel, but in this particular
case the output is a USB Type-c connector (compatible
"usb-c-connector"). So I'm not able to specify this link, given that it
will not be a bridge or panel...ever...
So this does not find a match on of_graph_get_remote_node(np, 1, ?), so
we move ahead and look at all children not named "port" or "ports". We
find the opp-table and concludes that this not a panel. At this point
ret is overwritten and we end up returning -EPROBE_DEFER.
I think it's worth reverting back to the explicit of_graph link to the
panel, even in the case that it's an immediate child node. It avoids the
problem of specifying that all future display nodes must only have
children of type ports, port or panel. We might be able to come up with
something that works for my case, but it seems fragile and not very
future proof. The explicit port is a little bit clunky, but it doesn't
have this problem.
Regards,
Bjorn
> ---
> drivers/gpu/drm/drm_of.c | 23 ++++++++++++++++++-----
> 1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index 8716da6369a6..97ea9d2016ff 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -223,6 +223,9 @@ static int find_panel_or_bridge(struct device_node *node,
> struct drm_panel **panel,
> struct drm_bridge **bridge)
> {
> + if (!of_device_is_available(node))
> + return -ENODEV;
> +
> if (panel) {
> *panel = of_drm_find_panel(node);
> if (!IS_ERR(*panel))
> @@ -265,7 +268,7 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> struct drm_bridge **bridge)
> {
> struct device_node *node;
> - int ret;
> + int ret = -ENODEV;
>
> if (!panel && !bridge)
> return -EINVAL;
> @@ -282,8 +285,12 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> ret = find_panel_or_bridge(node, panel, bridge);
> of_node_put(node);
>
> - if (!ret)
> - return 0;
> + /*
> + * If the graph/remote node is present we consider it
> + * to be the legitimate candidate here and return
> + * whatever code we got from find_panel_or_bridge.
> + */
> + return ret;
> }
> }
>
> @@ -296,12 +303,18 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> ret = find_panel_or_bridge(node, panel, bridge);
> of_node_put(node);
>
> - /* Stop at the first found occurrence. */
> + /*
> + * Note that an unrelated (available) child node will cause
> + * find_panel_or_bridge to return -EPROBE_DEFER because there
> + * is no way to distinguish the node from a legitimate
> + * panel/bridge that didn't register yet. Keep iterating nodes
> + * and only return on the first found occurrence.
> + */
> if (!ret)
> return 0;
> }
>
> - return -EPROBE_DEFER;
> + return ret;
> }
> EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
>
> --
> 2.35.1
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] drm: of: Improve error handling in bridge/panel detection
2022-04-09 3:09 ` Bjorn Andersson
@ 2022-04-12 8:23 ` Paul Kocialkowski
-1 siblings, 0 replies; 10+ messages in thread
From: Paul Kocialkowski @ 2022-04-12 8:23 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Thomas Petazzoni, David Airlie, linux-kernel, Thierry Reding,
dri-devel, Thomas Zimmermann, Jagan Teki
[-- Attachment #1: Type: text/plain, Size: 7494 bytes --]
Hi Bjorn,
with a question for Jagan at the end.
On Fri 08 Apr 22, 22:09, Bjorn Andersson wrote:
> On Thu 07 Apr 04:34 CDT 2022, Paul Kocialkowski wrote:
>
> > With the previous rework of drm_of_find_panel_or_bridge only
> > -EPROBE_DEFER is returned while previous behavior allowed -ENODEV
> > to be returned when the port/endpoint is either missing or unavailable.
> >
> > Make the default return code of the function -ENODEV to handle this and
> > only return -EPROBE_DEFER in find_panel_or_bridge when the of device is
> > available but not yet registered. Also return the error code whenever
> > the remote node exists to avoid checking for child nodes.
> >
> > Checking child nodes could result in -EPROBE_DEFER returned by
> > find_panel_or_bridge with an unrelated child node that would overwrite
> > a legitimate -ENODEV from find_panel_or_bridge if the remote node from
> > the of graph is unavailable. This happens because find_panel_or_bridge
> > has no way to distinguish between a legitimate panel/bridge node that
> > isn't yet registered and an unrelated node.
> >
> > Add comments around to clarify this behavior.
> >
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > Fixes: 67bae5f28c89 ("drm: of: Properly try all possible cases for bridge/panel detection")
> > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Cc: Thierry Reding <thierry.reding@gmail.com>
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> >
>
> Thanks for your patch, this does seem to solve the first problem I
> reported, where I have a DisplayPort bridge with the following content:
>
> sn65dsi86: bridge@2c {
> compatible = "ti,sn65dsi86";
> ...;
>
> ports {
> port@0 {
> reg = <0>;
> sn65dsi86_in_a: endpoint {
> remote-endpoint = <&dsi0_out>;
> };
> };
>
> port@1 {
> reg = <1>;
> sn65dsi86_out: endpoint {
> remote-endpoint = <&panel_in_edp>;
> };
> };
> };
>
> aux-bus {
> panel: panel {
> compatible = "boe,nv133fhm-n61";
> backlight = <&backlight>;
>
> port {
> panel_in_edp: endpoint {
> remote-endpoint = <&sn65dsi86_out>;
> };
> };
> };
> };
> };
>
> The code now finds a match on of_graph_get_remote_node() and returns 0
> or -EPROBE_DEFER from find_panel_or_bridge(?, 1, ?). And we return this,
> before failing to resolve the "aux-bus" as a panel.
Sounds good!
> But my other case still doesn't work:
>
> mdss_dp: displayport-controller@ae90000 {
> compatible = "qcom,sm8350-dp";
> ...;
> operating-points-v2 = <&dp_opp_table>;
>
> ports {
> port@0 {
> reg = <0>;
> dp_in: endpoint {
> remote-endpoint = <&dpu_intf0_out>;
> };
> };
> };
>
> dp_opp_table: opp-table {
> ...
> };
> };
>
> port@1 may be a reference to a DisplayPort panel, but in this particular
> case the output is a USB Type-c connector (compatible
> "usb-c-connector"). So I'm not able to specify this link, given that it
> will not be a bridge or panel...ever...
So in this case you have one port that can go either to a panel or a connector.
Using drm_of_find_panel_or_bridge will always return -EPROBE_DEFER for the
connector (I suspect this was also the previous behavior) so I think you should
first check (in the driver) if the remote is a type-c connector
(of_graph_get_remote_node + of_device_is_compatible) and use
drm_of_find_panel_or_bridge if not.
> So this does not find a match on of_graph_get_remote_node(np, 1, ?), so
> we move ahead and look at all children not named "port" or "ports". We
> find the opp-table and concludes that this not a panel. At this point
> ret is overwritten and we end up returning -EPROBE_DEFER.
So if I understand correctly, port@1 is not defined when the issue happens
and it returns -EPROBE_DEFER like you described.
We could step things up a notch and return -ENODEV if of_graph_is_present
but of_graph_get_remote_node returns NULL. The idea would be that if the of
graph is present, the child node mechanism cannot be used.
> I think it's worth reverting back to the explicit of_graph link to the
> panel, even in the case that it's an immediate child node. It avoids the
> problem of specifying that all future display nodes must only have
> children of type ports, port or panel. We might be able to come up with
> something that works for my case, but it seems fragile and not very
> future proof. The explicit port is a little bit clunky, but it doesn't
> have this problem.
I think in any case we really don't want to specify that display nodes cannot
have any other child than ports, port or a direct panel/bridge. I know that
a number of bindings already specify different types of child nodes.
I am absolutly in favor of reverting the child node mechanism, but it might
be too late already if a binding was already defined to work this way.
Of course we should actively discourage anyone to use it in a new binding.
Jagan, can you prove some insight on why this mechanism was needed in the first
place and if we can get rid of it without breaking any active binding?
Cheers,
Paul
> Regards,
> Bjorn
>
> > ---
> > drivers/gpu/drm/drm_of.c | 23 ++++++++++++++++++-----
> > 1 file changed, 18 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> > index 8716da6369a6..97ea9d2016ff 100644
> > --- a/drivers/gpu/drm/drm_of.c
> > +++ b/drivers/gpu/drm/drm_of.c
> > @@ -223,6 +223,9 @@ static int find_panel_or_bridge(struct device_node *node,
> > struct drm_panel **panel,
> > struct drm_bridge **bridge)
> > {
> > + if (!of_device_is_available(node))
> > + return -ENODEV;
> > +
> > if (panel) {
> > *panel = of_drm_find_panel(node);
> > if (!IS_ERR(*panel))
> > @@ -265,7 +268,7 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> > struct drm_bridge **bridge)
> > {
> > struct device_node *node;
> > - int ret;
> > + int ret = -ENODEV;
> >
> > if (!panel && !bridge)
> > return -EINVAL;
> > @@ -282,8 +285,12 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> > ret = find_panel_or_bridge(node, panel, bridge);
> > of_node_put(node);
> >
> > - if (!ret)
> > - return 0;
> > + /*
> > + * If the graph/remote node is present we consider it
> > + * to be the legitimate candidate here and return
> > + * whatever code we got from find_panel_or_bridge.
> > + */
> > + return ret;
> > }
> > }
> >
> > @@ -296,12 +303,18 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> > ret = find_panel_or_bridge(node, panel, bridge);
> > of_node_put(node);
> >
> > - /* Stop at the first found occurrence. */
> > + /*
> > + * Note that an unrelated (available) child node will cause
> > + * find_panel_or_bridge to return -EPROBE_DEFER because there
> > + * is no way to distinguish the node from a legitimate
> > + * panel/bridge that didn't register yet. Keep iterating nodes
> > + * and only return on the first found occurrence.
> > + */
> > if (!ret)
> > return 0;
> > }
> >
> > - return -EPROBE_DEFER;
> > + return ret;
> > }
> > 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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] drm: of: Improve error handling in bridge/panel detection
@ 2022-04-12 8:23 ` Paul Kocialkowski
0 siblings, 0 replies; 10+ messages in thread
From: Paul Kocialkowski @ 2022-04-12 8:23 UTC (permalink / raw)
To: Bjorn Andersson
Cc: dri-devel, linux-kernel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Daniel Vetter, Jagan Teki,
Thomas Petazzoni, Thierry Reding, Linus Walleij
[-- Attachment #1: Type: text/plain, Size: 7494 bytes --]
Hi Bjorn,
with a question for Jagan at the end.
On Fri 08 Apr 22, 22:09, Bjorn Andersson wrote:
> On Thu 07 Apr 04:34 CDT 2022, Paul Kocialkowski wrote:
>
> > With the previous rework of drm_of_find_panel_or_bridge only
> > -EPROBE_DEFER is returned while previous behavior allowed -ENODEV
> > to be returned when the port/endpoint is either missing or unavailable.
> >
> > Make the default return code of the function -ENODEV to handle this and
> > only return -EPROBE_DEFER in find_panel_or_bridge when the of device is
> > available but not yet registered. Also return the error code whenever
> > the remote node exists to avoid checking for child nodes.
> >
> > Checking child nodes could result in -EPROBE_DEFER returned by
> > find_panel_or_bridge with an unrelated child node that would overwrite
> > a legitimate -ENODEV from find_panel_or_bridge if the remote node from
> > the of graph is unavailable. This happens because find_panel_or_bridge
> > has no way to distinguish between a legitimate panel/bridge node that
> > isn't yet registered and an unrelated node.
> >
> > Add comments around to clarify this behavior.
> >
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > Fixes: 67bae5f28c89 ("drm: of: Properly try all possible cases for bridge/panel detection")
> > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Cc: Thierry Reding <thierry.reding@gmail.com>
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> >
>
> Thanks for your patch, this does seem to solve the first problem I
> reported, where I have a DisplayPort bridge with the following content:
>
> sn65dsi86: bridge@2c {
> compatible = "ti,sn65dsi86";
> ...;
>
> ports {
> port@0 {
> reg = <0>;
> sn65dsi86_in_a: endpoint {
> remote-endpoint = <&dsi0_out>;
> };
> };
>
> port@1 {
> reg = <1>;
> sn65dsi86_out: endpoint {
> remote-endpoint = <&panel_in_edp>;
> };
> };
> };
>
> aux-bus {
> panel: panel {
> compatible = "boe,nv133fhm-n61";
> backlight = <&backlight>;
>
> port {
> panel_in_edp: endpoint {
> remote-endpoint = <&sn65dsi86_out>;
> };
> };
> };
> };
> };
>
> The code now finds a match on of_graph_get_remote_node() and returns 0
> or -EPROBE_DEFER from find_panel_or_bridge(?, 1, ?). And we return this,
> before failing to resolve the "aux-bus" as a panel.
Sounds good!
> But my other case still doesn't work:
>
> mdss_dp: displayport-controller@ae90000 {
> compatible = "qcom,sm8350-dp";
> ...;
> operating-points-v2 = <&dp_opp_table>;
>
> ports {
> port@0 {
> reg = <0>;
> dp_in: endpoint {
> remote-endpoint = <&dpu_intf0_out>;
> };
> };
> };
>
> dp_opp_table: opp-table {
> ...
> };
> };
>
> port@1 may be a reference to a DisplayPort panel, but in this particular
> case the output is a USB Type-c connector (compatible
> "usb-c-connector"). So I'm not able to specify this link, given that it
> will not be a bridge or panel...ever...
So in this case you have one port that can go either to a panel or a connector.
Using drm_of_find_panel_or_bridge will always return -EPROBE_DEFER for the
connector (I suspect this was also the previous behavior) so I think you should
first check (in the driver) if the remote is a type-c connector
(of_graph_get_remote_node + of_device_is_compatible) and use
drm_of_find_panel_or_bridge if not.
> So this does not find a match on of_graph_get_remote_node(np, 1, ?), so
> we move ahead and look at all children not named "port" or "ports". We
> find the opp-table and concludes that this not a panel. At this point
> ret is overwritten and we end up returning -EPROBE_DEFER.
So if I understand correctly, port@1 is not defined when the issue happens
and it returns -EPROBE_DEFER like you described.
We could step things up a notch and return -ENODEV if of_graph_is_present
but of_graph_get_remote_node returns NULL. The idea would be that if the of
graph is present, the child node mechanism cannot be used.
> I think it's worth reverting back to the explicit of_graph link to the
> panel, even in the case that it's an immediate child node. It avoids the
> problem of specifying that all future display nodes must only have
> children of type ports, port or panel. We might be able to come up with
> something that works for my case, but it seems fragile and not very
> future proof. The explicit port is a little bit clunky, but it doesn't
> have this problem.
I think in any case we really don't want to specify that display nodes cannot
have any other child than ports, port or a direct panel/bridge. I know that
a number of bindings already specify different types of child nodes.
I am absolutly in favor of reverting the child node mechanism, but it might
be too late already if a binding was already defined to work this way.
Of course we should actively discourage anyone to use it in a new binding.
Jagan, can you prove some insight on why this mechanism was needed in the first
place and if we can get rid of it without breaking any active binding?
Cheers,
Paul
> Regards,
> Bjorn
>
> > ---
> > drivers/gpu/drm/drm_of.c | 23 ++++++++++++++++++-----
> > 1 file changed, 18 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> > index 8716da6369a6..97ea9d2016ff 100644
> > --- a/drivers/gpu/drm/drm_of.c
> > +++ b/drivers/gpu/drm/drm_of.c
> > @@ -223,6 +223,9 @@ static int find_panel_or_bridge(struct device_node *node,
> > struct drm_panel **panel,
> > struct drm_bridge **bridge)
> > {
> > + if (!of_device_is_available(node))
> > + return -ENODEV;
> > +
> > if (panel) {
> > *panel = of_drm_find_panel(node);
> > if (!IS_ERR(*panel))
> > @@ -265,7 +268,7 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> > struct drm_bridge **bridge)
> > {
> > struct device_node *node;
> > - int ret;
> > + int ret = -ENODEV;
> >
> > if (!panel && !bridge)
> > return -EINVAL;
> > @@ -282,8 +285,12 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> > ret = find_panel_or_bridge(node, panel, bridge);
> > of_node_put(node);
> >
> > - if (!ret)
> > - return 0;
> > + /*
> > + * If the graph/remote node is present we consider it
> > + * to be the legitimate candidate here and return
> > + * whatever code we got from find_panel_or_bridge.
> > + */
> > + return ret;
> > }
> > }
> >
> > @@ -296,12 +303,18 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> > ret = find_panel_or_bridge(node, panel, bridge);
> > of_node_put(node);
> >
> > - /* Stop at the first found occurrence. */
> > + /*
> > + * Note that an unrelated (available) child node will cause
> > + * find_panel_or_bridge to return -EPROBE_DEFER because there
> > + * is no way to distinguish the node from a legitimate
> > + * panel/bridge that didn't register yet. Keep iterating nodes
> > + * and only return on the first found occurrence.
> > + */
> > if (!ret)
> > return 0;
> > }
> >
> > - return -EPROBE_DEFER;
> > + return ret;
> > }
> > 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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: drm: of: Improve error handling in bridge/panel detection
2022-04-07 9:34 ` Paul Kocialkowski
@ 2022-04-19 16:54 ` Paul Cercueil
-1 siblings, 0 replies; 10+ messages in thread
From: Paul Cercueil @ 2022-04-19 16:54 UTC (permalink / raw)
To: Paul Kocialkowski
Cc: Thomas Petazzoni, David Airlie, linux-kernel, dri-devel,
Bjorn Andersson, Thierry Reding, Jagan Teki, Thomas Zimmermann
Hi Paul,
Le jeu., avril 7 2022 at 11:34:08 +0200, Paul Kocialkowski
<paul.kocialkowski@bootlin.com> a écrit :
> With the previous rework of drm_of_find_panel_or_bridge only
> -EPROBE_DEFER is returned while previous behavior allowed -ENODEV
> to be returned when the port/endpoint is either missing or
> unavailable.
>
> Make the default return code of the function -ENODEV to handle this
> and
> only return -EPROBE_DEFER in find_panel_or_bridge when the of device
> is
> available but not yet registered. Also return the error code whenever
> the remote node exists to avoid checking for child nodes.
>
> Checking child nodes could result in -EPROBE_DEFER returned by
> find_panel_or_bridge with an unrelated child node that would overwrite
> a legitimate -ENODEV from find_panel_or_bridge if the remote node from
> the of graph is unavailable. This happens because find_panel_or_bridge
> has no way to distinguish between a legitimate panel/bridge node that
> isn't yet registered and an unrelated node.
>
> Add comments around to clarify this behavior.
>
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> Fixes: 67bae5f28c89 ("drm: of: Properly try all possible cases for
> bridge/panel detection")
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
This fixes the ingenic-drm driver, which was broken by the commit this
patch addresses.
So:
Tested-by: Paul Cercueil <paul@crapouillou.net>
Cheers,
-Paul
> ---
> drivers/gpu/drm/drm_of.c | 23 ++++++++++++++++++-----
> 1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index 8716da6369a6..97ea9d2016ff 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -223,6 +223,9 @@ static int find_panel_or_bridge(struct
> device_node *node,
> struct drm_panel **panel,
> struct drm_bridge **bridge)
> {
> + if (!of_device_is_available(node))
> + return -ENODEV;
> +
> if (panel) {
> *panel = of_drm_find_panel(node);
> if (!IS_ERR(*panel))
> @@ -265,7 +268,7 @@ int drm_of_find_panel_or_bridge(const struct
> device_node *np,
> struct drm_bridge **bridge)
> {
> struct device_node *node;
> - int ret;
> + int ret = -ENODEV;
>
> if (!panel && !bridge)
> return -EINVAL;
> @@ -282,8 +285,12 @@ int drm_of_find_panel_or_bridge(const struct
> device_node *np,
> ret = find_panel_or_bridge(node, panel, bridge);
> of_node_put(node);
>
> - if (!ret)
> - return 0;
> + /*
> + * If the graph/remote node is present we consider it
> + * to be the legitimate candidate here and return
> + * whatever code we got from find_panel_or_bridge.
> + */
> + return ret;
> }
> }
>
> @@ -296,12 +303,18 @@ int drm_of_find_panel_or_bridge(const struct
> device_node *np,
> ret = find_panel_or_bridge(node, panel, bridge);
> of_node_put(node);
>
> - /* Stop at the first found occurrence. */
> + /*
> + * Note that an unrelated (available) child node will cause
> + * find_panel_or_bridge to return -EPROBE_DEFER because there
> + * is no way to distinguish the node from a legitimate
> + * panel/bridge that didn't register yet. Keep iterating nodes
> + * and only return on the first found occurrence.
> + */
> if (!ret)
> return 0;
> }
>
> - return -EPROBE_DEFER;
> + return ret;
> }
> EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: drm: of: Improve error handling in bridge/panel detection
@ 2022-04-19 16:54 ` Paul Cercueil
0 siblings, 0 replies; 10+ messages in thread
From: Paul Cercueil @ 2022-04-19 16:54 UTC (permalink / raw)
To: Paul Kocialkowski
Cc: dri-devel, linux-kernel, Thomas Zimmermann, David Airlie,
Thomas Petazzoni, Bjorn Andersson, Thierry Reding, Jagan Teki
Hi Paul,
Le jeu., avril 7 2022 at 11:34:08 +0200, Paul Kocialkowski
<paul.kocialkowski@bootlin.com> a écrit :
> With the previous rework of drm_of_find_panel_or_bridge only
> -EPROBE_DEFER is returned while previous behavior allowed -ENODEV
> to be returned when the port/endpoint is either missing or
> unavailable.
>
> Make the default return code of the function -ENODEV to handle this
> and
> only return -EPROBE_DEFER in find_panel_or_bridge when the of device
> is
> available but not yet registered. Also return the error code whenever
> the remote node exists to avoid checking for child nodes.
>
> Checking child nodes could result in -EPROBE_DEFER returned by
> find_panel_or_bridge with an unrelated child node that would overwrite
> a legitimate -ENODEV from find_panel_or_bridge if the remote node from
> the of graph is unavailable. This happens because find_panel_or_bridge
> has no way to distinguish between a legitimate panel/bridge node that
> isn't yet registered and an unrelated node.
>
> Add comments around to clarify this behavior.
>
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> Fixes: 67bae5f28c89 ("drm: of: Properly try all possible cases for
> bridge/panel detection")
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
This fixes the ingenic-drm driver, which was broken by the commit this
patch addresses.
So:
Tested-by: Paul Cercueil <paul@crapouillou.net>
Cheers,
-Paul
> ---
> drivers/gpu/drm/drm_of.c | 23 ++++++++++++++++++-----
> 1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index 8716da6369a6..97ea9d2016ff 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -223,6 +223,9 @@ static int find_panel_or_bridge(struct
> device_node *node,
> struct drm_panel **panel,
> struct drm_bridge **bridge)
> {
> + if (!of_device_is_available(node))
> + return -ENODEV;
> +
> if (panel) {
> *panel = of_drm_find_panel(node);
> if (!IS_ERR(*panel))
> @@ -265,7 +268,7 @@ int drm_of_find_panel_or_bridge(const struct
> device_node *np,
> struct drm_bridge **bridge)
> {
> struct device_node *node;
> - int ret;
> + int ret = -ENODEV;
>
> if (!panel && !bridge)
> return -EINVAL;
> @@ -282,8 +285,12 @@ int drm_of_find_panel_or_bridge(const struct
> device_node *np,
> ret = find_panel_or_bridge(node, panel, bridge);
> of_node_put(node);
>
> - if (!ret)
> - return 0;
> + /*
> + * If the graph/remote node is present we consider it
> + * to be the legitimate candidate here and return
> + * whatever code we got from find_panel_or_bridge.
> + */
> + return ret;
> }
> }
>
> @@ -296,12 +303,18 @@ int drm_of_find_panel_or_bridge(const struct
> device_node *np,
> ret = find_panel_or_bridge(node, panel, bridge);
> of_node_put(node);
>
> - /* Stop at the first found occurrence. */
> + /*
> + * Note that an unrelated (available) child node will cause
> + * find_panel_or_bridge to return -EPROBE_DEFER because there
> + * is no way to distinguish the node from a legitimate
> + * panel/bridge that didn't register yet. Keep iterating nodes
> + * and only return on the first found occurrence.
> + */
> if (!ret)
> return 0;
> }
>
> - return -EPROBE_DEFER;
> + return ret;
> }
> EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm: of: Improve error handling in bridge/panel detection
2022-04-07 9:34 ` Paul Kocialkowski
@ 2022-04-20 15:25 ` Thierry Reding
-1 siblings, 0 replies; 10+ messages in thread
From: Thierry Reding @ 2022-04-20 15:25 UTC (permalink / raw)
To: Paul Kocialkowski
Cc: dri-devel, linux-kernel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Daniel Vetter, Jagan Teki,
Thomas Petazzoni, Bjorn Andersson, Linus Walleij
[-- Attachment #1: Type: text/plain, Size: 1579 bytes --]
On Thu, Apr 07, 2022 at 11:34:08AM +0200, Paul Kocialkowski wrote:
> With the previous rework of drm_of_find_panel_or_bridge only
> -EPROBE_DEFER is returned while previous behavior allowed -ENODEV
> to be returned when the port/endpoint is either missing or unavailable.
>
> Make the default return code of the function -ENODEV to handle this and
> only return -EPROBE_DEFER in find_panel_or_bridge when the of device is
> available but not yet registered. Also return the error code whenever
> the remote node exists to avoid checking for child nodes.
>
> Checking child nodes could result in -EPROBE_DEFER returned by
> find_panel_or_bridge with an unrelated child node that would overwrite
> a legitimate -ENODEV from find_panel_or_bridge if the remote node from
> the of graph is unavailable. This happens because find_panel_or_bridge
> has no way to distinguish between a legitimate panel/bridge node that
> isn't yet registered and an unrelated node.
>
> Add comments around to clarify this behavior.
>
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> Fixes: 67bae5f28c89 ("drm: of: Properly try all possible cases for bridge/panel detection")
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
>
> ---
> drivers/gpu/drm/drm_of.c | 23 ++++++++++++++++++-----
> 1 file changed, 18 insertions(+), 5 deletions(-)
This also fixes the regression that I was seeing on Tegra.
Tested-by: Thierry Reding <treding@nvidia.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] drm: of: Improve error handling in bridge/panel detection
@ 2022-04-20 15:25 ` Thierry Reding
0 siblings, 0 replies; 10+ messages in thread
From: Thierry Reding @ 2022-04-20 15:25 UTC (permalink / raw)
To: Paul Kocialkowski
Cc: Thomas Petazzoni, David Airlie, linux-kernel, Bjorn Andersson,
dri-devel, Thomas Zimmermann, Jagan Teki
[-- Attachment #1: Type: text/plain, Size: 1579 bytes --]
On Thu, Apr 07, 2022 at 11:34:08AM +0200, Paul Kocialkowski wrote:
> With the previous rework of drm_of_find_panel_or_bridge only
> -EPROBE_DEFER is returned while previous behavior allowed -ENODEV
> to be returned when the port/endpoint is either missing or unavailable.
>
> Make the default return code of the function -ENODEV to handle this and
> only return -EPROBE_DEFER in find_panel_or_bridge when the of device is
> available but not yet registered. Also return the error code whenever
> the remote node exists to avoid checking for child nodes.
>
> Checking child nodes could result in -EPROBE_DEFER returned by
> find_panel_or_bridge with an unrelated child node that would overwrite
> a legitimate -ENODEV from find_panel_or_bridge if the remote node from
> the of graph is unavailable. This happens because find_panel_or_bridge
> has no way to distinguish between a legitimate panel/bridge node that
> isn't yet registered and an unrelated node.
>
> Add comments around to clarify this behavior.
>
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> Fixes: 67bae5f28c89 ("drm: of: Properly try all possible cases for bridge/panel detection")
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
>
> ---
> drivers/gpu/drm/drm_of.c | 23 ++++++++++++++++++-----
> 1 file changed, 18 insertions(+), 5 deletions(-)
This also fixes the regression that I was seeing on Tegra.
Tested-by: Thierry Reding <treding@nvidia.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread