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

[Maxime Ripard]

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?
Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20220304/0cb031c5/attachment.sig>