[PATCH v4] drm: of: Lookup if child node has panel or bridge

[Paul Kocialkowski]

Hi Maxime,

On Fri 04 Mar 22, 12:38, Maxime Ripard wrote:
> On Fri, Mar 04, 2022 at 12:05:14PM +0100, Paul Kocialkowski wrote:
> > On Fri 04 Mar 22, 12:00, Paul Kocialkowski wrote:
> > > Hi Maxime,
> > > 
> > > On Fri 04 Mar 22, 09:54, Maxime Ripard wrote:
> > > > Hi Paul,
> > > > 
> > > > On Thu, Mar 03, 2022 at 09:26:30PM +0100, Paul Kocialkowski wrote:
> > > > > On Wed 02 Feb 22, 21:34, Jagan Teki wrote:
> > > > > > Devices can also be child nodes when we also control that device
> > > > > > through the upstream device (ie, MIPI-DCS for a MIPI-DSI device).
> > > > > > 
> > > > > > drm_of_find_panel_or_bridge can lookup panel or bridge for a given
> > > > > > device has port and endpoint and it fails to lookup if the device
> > > > > > has a child nodes.
> > > > > 
> > > > > This patch breaks the logicvc drm driver that I'm currently developping.
> > > > > The symptom is that drm_of_find_panel_or_bridge now always returns
> > > > > -EPROBE_DEFER even after the panel has probed and is running well.
> > > > > It seems that the function can no longer find the panel.
> > > > > 
> > > > > I haven't figured out the details, but reverting your patch makes
> > > > > it work again. I suspect other drivers might be affected as well, so
> > > > > it would probably be a good idea to revert the patch until the root
> > > > > cause is clearly understood and the patch can be adapted accordingly.
> > > > > 
> > > > > Here is what the device-tree looks like:
> > > > > 
> > > > > / {
> > > > > 	panel: panel-lvds {
> > > > > 		compatible = "panel-lvds";
> > > > > 
> > > > > 		[...]
> > > > > 
> > > > > 		port {
> > > > > 			#address-cells = <1>;
> > > > > 			#size-cells = <0>;
> > > > > 
> > > > > 			panel_input: endpoint at 0 {
> > > > > 				reg = <0>;
> > > > > 				remote-endpoint = <&logicvc_output>;
> > > > > 			};
> > > > > 		};
> > > > > 	};
> > > > > };
> > > > > 
> > > > > &amba {
> > > > > 	logicvc: logicvc at 43c00000 {
> > > > > 		compatible = "xylon,logicvc-3.02.a", "syscon", "simple-mfd";
> > > > > 		reg = <0x43c00000 0x6000>;
> > > > > 
> > > > > 		#address-cells = <1>;
> > > > > 		#size-cells = <1>;
> > > > > 
> > > > > 		[...]
> > > > > 
> > > > > 		logicvc_display: display-engine at 0 {
> > > > > 			compatible = "xylon,logicvc-4.01.a-display";
> > > > > 
> > > > > 			[...]
> > > > 
> > > > I think the issue lies in what you left out here: you have another node
> > > > aside from the port one, called layers. I *think* the issue is that the
> > > > code will now pick up the layers node, and try to use it as a panel,
> > > > which will never probe.
> > > > 
> > > > I've had a look at all the other bindings though, it seems like this
> > > > driver is the only one that can be affected: the anx7625 seems to be the
> > > > only other driver that has a child node that isn't either a port or a
> > > > panel (aux-bus) but it doesn't use drm_of_find_panel_or_bridge either.
> > > 
> > > Thanks a lot for looking into this so quickly!
> > > 
> > > After some testing it clearly appears that you're right and the layers
> > > node is the one conflicting with the patch. Removing it brings the
> > > behavior back to normal. I'll try to dig-in a bit more to understand
> > > why this is happening since it's really not obvious when just looking
> > > at the patch.
> > 
> > Ah wait I do understand it actually. The patch will take the *first* node
> > that doesn't have ports/port in it and use that as remote instead of
> > of_graph_get_remote_node.
> > 
> > So maybe the fix would be to first look via of_graph_get_remote_node and
> > if nothing is returned then it should try to use the first node as remote.
> > tl;dr just inverting the order of the logic.
> > 
> > Do you think that would work?
> 
> We can have multiple strategies here. The one you have in mind does work
> indeed, but relying on the node order is still fairly fragile.
> 
> I think it would work fine then if:
> 
>   - We first lookup any endpoint, and see if we have a panel or bridge.
>     If so, we return it.
> 
>   - Then, we look at any available child node, and see if we have a
>     panel or bridge attached. If so, we return it.
> 
>   - we return -EPROBE_DEFER
> 
> That way, even if we have something like:
> 
> node {
>      totally-not-a-panel {
>      }
> 
>      panel {
>      }
> }
> 
> It would work fine, without relying on the node name (well, except for
> port(s)?)
> 
> What do you think?

Yes it would definitely be better to try all possible cases, including when
one of them fails instead of just selecting one to check and failing if
the panel/bridge nodes aren't there.

I'll have a try at this and send a fixup patch soon!

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20220309/89faf91f/attachment.sig>