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

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

[Paul Kocialkowski]

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

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

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

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Fixes: 80253168dbfd ("drm: of: Lookup if child node has panel or bridge")
---
 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)
+{
+	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);
-- 
2.35.1

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

[Maxime Ripard]

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

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

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

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

[Paul Kocialkowski]

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

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

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

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

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

[Maxime Ripard]

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

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

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

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

[Paul Kocialkowski]

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

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

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

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

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

[Jagan Teki]

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.

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

[Paul Kocialkowski]

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

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

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

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