| Message ID | 20220309143200.111292-1-paul.kocialkowski@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: of: Properly try all possible cases for bridge/panel detection | expand |
Hi Paul, On Wed, Mar 09, 2022 at 03:32:00PM +0100, 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. > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > Fixes: 80253168dbfd ("drm: of: Lookup if child node has panel or bridge") Thanks, it's in pretty good shape now, but I have a few bike sheds to paint :) > --- > drivers/gpu/drm/drm_of.c | 93 +++++++++++++++++++++------------------- > 1 file changed, 49 insertions(+), 44 deletions(-) > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c > index 9d90cd75c457..67f1b7dfc892 100644 > --- a/drivers/gpu/drm/drm_of.c > +++ b/drivers/gpu/drm/drm_of.c > @@ -219,6 +219,35 @@ int drm_of_encoder_active_endpoint(struct device_node *node, > } > EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint); > > +static int drm_of_find_remote_panel_or_bridge(struct device_node *remote, > + struct drm_panel **panel, > + struct drm_bridge **bridge) This function performs its look up directly on the struct device_node passed as argument, so I don't think the "remote" in the name is great. Since it's static, we can just call it find_panel_or_bridge, what do you think? > +{ > + int ret = -EPROBE_DEFER; > + > + if (panel) { > + *panel = of_drm_find_panel(remote); > + if (!IS_ERR(*panel)) > + ret = 0; return 0? > + else > + *panel = NULL; > + > + } > + > + /* No panel found yet, check for a bridge next. */ > + if (bridge) { > + if (ret) { And the return above allows to remove that test > + *bridge = of_drm_find_bridge(remote); > + if (*bridge) > + ret = 0; return 0? > + } else { > + *bridge = NULL; > + } > + > + } > + > + return ret; And here we can just return -EPROBE_DEFER > +} > + > /** > * drm_of_find_panel_or_bridge - return connected panel or bridge device > * @np: device tree node containing encoder output ports > @@ -249,57 +278,33 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, > 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; > + /* Check for a graph on the device node first. */ > + if (of_graph_is_present(np)) { > + remote = of_graph_get_remote_node(np, port, endpoint); > + if (remote) { > + ret = drm_of_find_remote_panel_or_bridge(remote, panel, > + bridge); > + of_node_put(remote); > + } > } > > - /* > - * 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. */ > + if (ret) { > + for_each_available_child_of_node(np, remote) { > + if (of_node_name_eq(remote, "port") || > + of_node_name_eq(remote, "ports")) > + continue; > > - if (panel) { > - *panel = of_drm_find_panel(remote); > - if (!IS_ERR(*panel)) > - ret = 0; > - else > - *panel = NULL; > - } > + ret = drm_of_find_remote_panel_or_bridge(remote, panel, > + bridge); > + of_node_put(remote); > > - /* 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; > + /* Stop at the first found occurrence. */ > + if (!ret) > + break; > } > - > } > > - of_node_put(remote); > return ret; > } So the diff is fairly hard to read, but it ends up as: > int ret = -EPROBE_DEFER; > struct device_node *remote; > > if (!panel && !bridge) > return -EINVAL; > if (panel) > *panel = NULL; > > /* Check for a graph on the device node first. */ > if (of_graph_is_present(np)) { > remote = of_graph_get_remote_node(np, port, endpoint); > if (remote) { > ret = drm_of_find_remote_panel_or_bridge(remote, panel, > bridge); > of_node_put(remote); I think we can simplify this by doing if (!ret) return ret; > } > } > > /* Otherwise check for any child node other than port/ports. */ > if (ret) { And thus we won't have to check for ret here > for_each_available_child_of_node(np, remote) { I'm a bit reluctant with variables that we reuse from one loop to another, especially since it's a bit misleading here. What about using a (loop local) remote variable in the of_graph path, and a loop-local variable node or child here? > if (of_node_name_eq(remote, "port") || > of_node_name_eq(remote, "ports")) > continue; > > ret = drm_of_find_remote_panel_or_bridge(remote, panel, > bridge); > of_node_put(remote); > > /* Stop at the first found occurrence. */ > if (!ret) > break; Ditto, let's just return here > } > } > > return ret; And then we can just return EPROBE_DEFER here (and get rid of ret entirely) Maxime
Hi Maxime, Thanks for the review! On Thu 10 Mar 22, 15:54, Maxime Ripard wrote: > Hi Paul, > > On Wed, Mar 09, 2022 at 03:32:00PM +0100, 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. > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > Fixes: 80253168dbfd ("drm: of: Lookup if child node has panel or bridge") > > Thanks, it's in pretty good shape now, but I have a few bike sheds to paint :) > > > --- > > drivers/gpu/drm/drm_of.c | 93 +++++++++++++++++++++------------------- > > 1 file changed, 49 insertions(+), 44 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c > > index 9d90cd75c457..67f1b7dfc892 100644 > > --- a/drivers/gpu/drm/drm_of.c > > +++ b/drivers/gpu/drm/drm_of.c > > @@ -219,6 +219,35 @@ int drm_of_encoder_active_endpoint(struct device_node *node, > > } > > EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint); > > > > +static int drm_of_find_remote_panel_or_bridge(struct device_node *remote, > > + struct drm_panel **panel, > > + struct drm_bridge **bridge) > > This function performs its look up directly on the struct device_node > passed as argument, so I don't think the "remote" in the name is great. > Since it's static, we can just call it find_panel_or_bridge, what do you > think? From a quick look at other DRM code I got the impression that static functions also usually carry the drm prefix but I might be wrong. > > +{ > > + int ret = -EPROBE_DEFER; > > + > > + if (panel) { > > + *panel = of_drm_find_panel(remote); > > + if (!IS_ERR(*panel)) > > + ret = 0; > > return 0? The idea was to still go through the "*bridge = NULL;" path if a bridge pointer is provided, to preserve the original behavior of the function. There may or may not not be any hard expectation on that, in any case I feel like it would be good to avoid out-of-scope functional changes here. > > + else > > + *panel = NULL; > > + > > + } > > + > > + /* No panel found yet, check for a bridge next. */ > > + if (bridge) { > > + if (ret) { > > And the return above allows to remove that test > > > + *bridge = of_drm_find_bridge(remote); > > + if (*bridge) > > + ret = 0; > > return 0? > > > + } else { > > + *bridge = NULL; > > + } > > + > > + } > > + > > + return ret; > > And here we can just return -EPROBE_DEFER > > > +} > > + > > > /** > > * drm_of_find_panel_or_bridge - return connected panel or bridge device > > * @np: device tree node containing encoder output ports > > @@ -249,57 +278,33 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, > > 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; > > + /* Check for a graph on the device node first. */ > > + if (of_graph_is_present(np)) { > > + remote = of_graph_get_remote_node(np, port, endpoint); > > + if (remote) { > > + ret = drm_of_find_remote_panel_or_bridge(remote, panel, > > + bridge); > > + of_node_put(remote); > > + } > > } > > > > - /* > > - * 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. */ > > + if (ret) { > > + for_each_available_child_of_node(np, remote) { > > + if (of_node_name_eq(remote, "port") || > > + of_node_name_eq(remote, "ports")) > > + continue; > > > > - if (panel) { > > - *panel = of_drm_find_panel(remote); > > - if (!IS_ERR(*panel)) > > - ret = 0; > > - else > > - *panel = NULL; > > - } > > + ret = drm_of_find_remote_panel_or_bridge(remote, panel, > > + bridge); > > + of_node_put(remote); > > > > - /* 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; > > + /* Stop at the first found occurrence. */ > > + if (!ret) > > + break; > > } > > - > > } > > > > - of_node_put(remote); > > return ret; > > } > > So the diff is fairly hard to read, but it ends up as: Yeah I agree, not sure what I can do about that. > > int ret = -EPROBE_DEFER; > > struct device_node *remote; > > > > if (!panel && !bridge) > > return -EINVAL; > > if (panel) > > *panel = NULL; > > > > /* Check for a graph on the device node first. */ > > if (of_graph_is_present(np)) { > > remote = of_graph_get_remote_node(np, port, endpoint); > > if (remote) { > > ret = drm_of_find_remote_panel_or_bridge(remote, panel, > > bridge); > > of_node_put(remote); > > I think we can simplify this by doing > > if (!ret) > return ret; > > > } > > } > > > > /* Otherwise check for any child node other than port/ports. */ > > if (ret) { > > And thus we won't have to check for ret here Yes I agree this one makes things more readable. > > for_each_available_child_of_node(np, remote) { > > I'm a bit reluctant with variables that we reuse from one loop to > another, especially since it's a bit misleading here. What about using a > (loop local) remote variable in the of_graph path, and a loop-local > variable node or child here? I feel like reusing variables across loops is quite a common thing and not really an issue on its own, but I agree that calling this one remote is confusing and "child" would make things clearer here. > > if (of_node_name_eq(remote, "port") || > > of_node_name_eq(remote, "ports")) > > continue; > > > > ret = drm_of_find_remote_panel_or_bridge(remote, panel, > > bridge); > > of_node_put(remote); > > > > /* Stop at the first found occurrence. */ > > if (!ret) > > break; > > Ditto, let's just return here Sure, fair enough! > > } > > } > > > > return ret; > > And then we can just return EPROBE_DEFER here (and get rid of ret entirely) Sounds good to me, thanks! Paul
On Wed, Mar 16, 2022 at 04:40:49PM +0100, Paul Kocialkowski wrote: > Hi Maxime, > > Thanks for the review! > > On Thu 10 Mar 22, 15:54, Maxime Ripard wrote: > > Hi Paul, > > > > On Wed, Mar 09, 2022 at 03:32:00PM +0100, 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. > > > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > > Fixes: 80253168dbfd ("drm: of: Lookup if child node has panel or bridge") > > > > Thanks, it's in pretty good shape now, but I have a few bike sheds to paint :) > > > > > --- > > > drivers/gpu/drm/drm_of.c | 93 +++++++++++++++++++++------------------- > > > 1 file changed, 49 insertions(+), 44 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c > > > index 9d90cd75c457..67f1b7dfc892 100644 > > > --- a/drivers/gpu/drm/drm_of.c > > > +++ b/drivers/gpu/drm/drm_of.c > > > @@ -219,6 +219,35 @@ int drm_of_encoder_active_endpoint(struct device_node *node, > > > } > > > EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint); > > > > > > +static int drm_of_find_remote_panel_or_bridge(struct device_node *remote, > > > + struct drm_panel **panel, > > > + struct drm_bridge **bridge) > > > > This function performs its look up directly on the struct device_node > > passed as argument, so I don't think the "remote" in the name is great. > > Since it's static, we can just call it find_panel_or_bridge, what do you > > think? > > From a quick look at other DRM code I got the impression that static functions > also usually carry the drm prefix but I might be wrong. Not necessarily, see handle_conflicting_encoders, commit_tail, commit_work, convert_clip_rect_to_rect, edid_load, etc. Most functions do, but it's not a rule or a convention. > > > +{ > > > + int ret = -EPROBE_DEFER; > > > + > > > + if (panel) { > > > + *panel = of_drm_find_panel(remote); > > > + if (!IS_ERR(*panel)) > > > + ret = 0; > > > > return 0? > > The idea was to still go through the "*bridge = NULL;" path if a bridge > pointer is provided, to preserve the original behavior of the function. > There may or may not not be any hard expectation on that, in any case > I feel like it would be good to avoid out-of-scope functional changes here. Then we could just clear it just like we clear the panel pointer in drm_of_find_panel_or_bridge. It would be more consistent. > > > + else > > > + *panel = NULL; > > > + > > > + } > > > + > > > + /* No panel found yet, check for a bridge next. */ > > > + if (bridge) { > > > + if (ret) { > > > > And the return above allows to remove that test > > > > > + *bridge = of_drm_find_bridge(remote); > > > + if (*bridge) > > > + ret = 0; > > > > return 0? > > > > > + } else { > > > + *bridge = NULL; > > > + } > > > + > > > + } > > > + > > > + return ret; > > > > And here we can just return -EPROBE_DEFER > > > > > +} > > > + > > > > > /** > > > * drm_of_find_panel_or_bridge - return connected panel or bridge device > > > * @np: device tree node containing encoder output ports > > > @@ -249,57 +278,33 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, > > > 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; > > > + /* Check for a graph on the device node first. */ > > > + if (of_graph_is_present(np)) { > > > + remote = of_graph_get_remote_node(np, port, endpoint); > > > + if (remote) { > > > + ret = drm_of_find_remote_panel_or_bridge(remote, panel, > > > + bridge); > > > + of_node_put(remote); > > > + } > > > } > > > > > > - /* > > > - * 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. */ > > > + if (ret) { > > > + for_each_available_child_of_node(np, remote) { > > > + if (of_node_name_eq(remote, "port") || > > > + of_node_name_eq(remote, "ports")) > > > + continue; > > > > > > - if (panel) { > > > - *panel = of_drm_find_panel(remote); > > > - if (!IS_ERR(*panel)) > > > - ret = 0; > > > - else > > > - *panel = NULL; > > > - } > > > + ret = drm_of_find_remote_panel_or_bridge(remote, panel, > > > + bridge); > > > + of_node_put(remote); > > > > > > - /* 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; > > > + /* Stop at the first found occurrence. */ > > > + if (!ret) > > > + break; > > > } > > > - > > > } > > > > > > - of_node_put(remote); > > > return ret; > > > } > > > > So the diff is fairly hard to read, but it ends up as: > > Yeah I agree, not sure what I can do about that. Nothing, really. I don't expect any change there, it just happens sometimes :) Maxime
Hi Maxime, On Fri 18 Mar 22, 16:14, Maxime Ripard wrote: > On Wed, Mar 16, 2022 at 04:40:49PM +0100, Paul Kocialkowski wrote: > > Hi Maxime, > > > > Thanks for the review! > > > > On Thu 10 Mar 22, 15:54, Maxime Ripard wrote: > > > Hi Paul, > > > > > > On Wed, Mar 09, 2022 at 03:32:00PM +0100, 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. > > > > > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > > > Fixes: 80253168dbfd ("drm: of: Lookup if child node has panel or bridge") > > > > > > Thanks, it's in pretty good shape now, but I have a few bike sheds to paint :) > > > > > > > --- > > > > drivers/gpu/drm/drm_of.c | 93 +++++++++++++++++++++------------------- > > > > 1 file changed, 49 insertions(+), 44 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c > > > > index 9d90cd75c457..67f1b7dfc892 100644 > > > > --- a/drivers/gpu/drm/drm_of.c > > > > +++ b/drivers/gpu/drm/drm_of.c > > > > @@ -219,6 +219,35 @@ int drm_of_encoder_active_endpoint(struct device_node *node, > > > > } > > > > EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint); > > > > > > > > +static int drm_of_find_remote_panel_or_bridge(struct device_node *remote, > > > > + struct drm_panel **panel, > > > > + struct drm_bridge **bridge) > > > > > > This function performs its look up directly on the struct device_node > > > passed as argument, so I don't think the "remote" in the name is great. > > > Since it's static, we can just call it find_panel_or_bridge, what do you > > > think? > > > > From a quick look at other DRM code I got the impression that static functions > > also usually carry the drm prefix but I might be wrong. > > Not necessarily, see handle_conflicting_encoders, commit_tail, commit_work, > convert_clip_rect_to_rect, edid_load, etc. > > Most functions do, but it's not a rule or a convention. Okay then, I'm fine with find_panel_or_bridge. > > > > +{ > > > > + int ret = -EPROBE_DEFER; > > > > + > > > > + if (panel) { > > > > + *panel = of_drm_find_panel(remote); > > > > + if (!IS_ERR(*panel)) > > > > + ret = 0; > > > > > > return 0? > > > > The idea was to still go through the "*bridge = NULL;" path if a bridge > > pointer is provided, to preserve the original behavior of the function. > > There may or may not not be any hard expectation on that, in any case > > I feel like it would be good to avoid out-of-scope functional changes here. > > Then we could just clear it just like we clear the panel pointer in > drm_of_find_panel_or_bridge. It would be more consistent. Oh absolutely, I agree that would be best. > > > > + else > > > > + *panel = NULL; > > > > + > > > > + } > > > > + > > > > + /* No panel found yet, check for a bridge next. */ > > > > + if (bridge) { > > > > + if (ret) { > > > > > > And the return above allows to remove that test > > > > > > > + *bridge = of_drm_find_bridge(remote); > > > > + if (*bridge) > > > > + ret = 0; > > > > > > return 0? > > > > > > > + } else { > > > > + *bridge = NULL; > > > > + } > > > > + > > > > + } > > > > + > > > > + return ret; > > > > > > And here we can just return -EPROBE_DEFER > > > > > > > +} > > > > + > > > > > > > /** > > > > * drm_of_find_panel_or_bridge - return connected panel or bridge device > > > > * @np: device tree node containing encoder output ports > > > > @@ -249,57 +278,33 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, > > > > 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; > > > > + /* Check for a graph on the device node first. */ > > > > + if (of_graph_is_present(np)) { > > > > + remote = of_graph_get_remote_node(np, port, endpoint); > > > > + if (remote) { > > > > + ret = drm_of_find_remote_panel_or_bridge(remote, panel, > > > > + bridge); > > > > + of_node_put(remote); > > > > + } > > > > } > > > > > > > > - /* > > > > - * 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. */ > > > > + if (ret) { > > > > + for_each_available_child_of_node(np, remote) { > > > > + if (of_node_name_eq(remote, "port") || > > > > + of_node_name_eq(remote, "ports")) > > > > + continue; > > > > > > > > - if (panel) { > > > > - *panel = of_drm_find_panel(remote); > > > > - if (!IS_ERR(*panel)) > > > > - ret = 0; > > > > - else > > > > - *panel = NULL; > > > > - } > > > > + ret = drm_of_find_remote_panel_or_bridge(remote, panel, > > > > + bridge); > > > > + of_node_put(remote); > > > > > > > > - /* 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; > > > > + /* Stop at the first found occurrence. */ > > > > + if (!ret) > > > > + break; > > > > } > > > > - > > > > } > > > > > > > > - of_node_put(remote); > > > > return ret; > > > > } > > > > > > So the diff is fairly hard to read, but it ends up as: > > > > Yeah I agree, not sure what I can do about that. > > Nothing, really. I don't expect any change there, it just happens sometimes :) All right then :) I'll send another iteration soon. Cheers, Paul
Hi Paul, On Wed, Mar 9, 2022 at 8:02 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. Can you add that pipeline example on the commit message, it gives more information on specific use cases why the existing code breaks. Thanks, Jagan.
Hi Jagan, On Fri 18 Mar 22, 21:35, Jagan Teki wrote: > Hi Paul, > > On Wed, Mar 9, 2022 at 8:02 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. > > Can you add that pipeline example on the commit message, it gives more > information on specific use cases why the existing code breaks. Ah I just sent v2 before reading your message. Well I think the description says it all: the problem shows as soon as there's a child node to the node passed to drm_of_find_panel_or_bridge and it's really independent from the of graph setup in the end. I think Maxime put some examples on the original thread (v4 of your patch). Paul
diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c index 9d90cd75c457..67f1b7dfc892 100644 --- a/drivers/gpu/drm/drm_of.c +++ b/drivers/gpu/drm/drm_of.c @@ -219,6 +219,35 @@ int drm_of_encoder_active_endpoint(struct device_node *node, } EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint); +static int drm_of_find_remote_panel_or_bridge(struct device_node *remote, + struct drm_panel **panel, + struct drm_bridge **bridge) +{ + int ret = -EPROBE_DEFER; + + 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; + } + + } + + return ret; +} + /** * drm_of_find_panel_or_bridge - return connected panel or bridge device * @np: device tree node containing encoder output ports @@ -249,57 +278,33 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, 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; + /* Check for a graph on the device node first. */ + if (of_graph_is_present(np)) { + remote = of_graph_get_remote_node(np, port, endpoint); + if (remote) { + ret = drm_of_find_remote_panel_or_bridge(remote, panel, + bridge); + of_node_put(remote); + } } - /* - * 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. */ + if (ret) { + for_each_available_child_of_node(np, remote) { + if (of_node_name_eq(remote, "port") || + of_node_name_eq(remote, "ports")) + continue; - if (panel) { - *panel = of_drm_find_panel(remote); - if (!IS_ERR(*panel)) - ret = 0; - else - *panel = NULL; - } + ret = drm_of_find_remote_panel_or_bridge(remote, panel, + bridge); + of_node_put(remote); - /* 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; + /* Stop at the first found occurrence. */ + if (!ret) + break; } - } - of_node_put(remote); return ret; } 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") --- drivers/gpu/drm/drm_of.c | 93 +++++++++++++++++++++------------------- 1 file changed, 49 insertions(+), 44 deletions(-)