Message-ID: <Ykx1yQGROqyiu/yJ@orome>
Date:   Tue, 5 Apr 2022 19:00:57 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     Paul Kocialkowski <paul.kocialkowski@...tlin.com>
Cc:     Bjorn Andersson <bjorn.andersson@...aro.org>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        David Airlie <airlied@...ux.ie>, linux-kernel@...r.kernel.org,
        Kuogee Hsieh <quic_khsieh@...cinc.com>,
        dri-devel@...ts.freedesktop.org,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Jagan Teki <jagan@...rulasolutions.com>
Subject: Re: [PATCH v3] drm: of: Properly try all possible cases for
 bridge/panel detection

On Fri, Apr 01, 2022 at 09:44:46AM +0200, Paul Kocialkowski wrote:
> Hi Bjorn,
> 
> On Thu 31 Mar 22, 20:16, Bjorn Andersson wrote:
> > On Tue 29 Mar 06:27 PDT 2022, 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.
> > > 
> > 
> > Thanks for your patch Paul, it fixed a regression on a device where I
> > have a eDP bridge with an of_graph and a aux-bus defined.
> > 
> > But unfortunately it does not resolve the regression I have for the
> > USB based DisplayPort setup described below.
> > 
> > 
> > In the Qualcomm DisplayPort driver We're calling:
> > 
> > 	devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
> > 
> > and with the following DT snippet the behavior changed:
> > 
> > displayport-controller@...0000 {
> > 	compatible = "qcom,sc8180x-dp";
> > 	...
> > 
> > 	operating-points-v2 = <&dp0_opp_table>;
> > 
> > 	ports {
> > 		#address-cells = <1>;
> > 		#size-cells = <0>;
> > 
> > 		port@0 {
> > 			reg = <0>;
> > 			dp0_in: endpoint {
> > 				remote-endpoint = <&display_driver>;
> > 			};
> > 		};
> > 	};
> > 
> > 	dp0_opp_table: opp-table {
> > 		...;
> > 	};
> > };
> > 
> > Prior to the introduction of 80253168dbfd ("drm: of: Lookup if child
> > node has panel or bridge") this would return -ENODEV, so we could
> > differentiate the case when we have a statically defined eDP panel from
> > that of a dynamically attached (over USB) DP panel.
> > 
> > Prior to your change, above case without the opp-table node would have
> > still returned -ENODEV.
> > 
> > But now this will just return -EPROBE_DEFER in both cases.
> 
> Oh that's right, the -ENODEV case was just completely removed by my change.
> Initially this would happen if !of_graph_is_present or if the remote node
> doesn't exist.
> 
> Now that we are also checking for child nodes, we can't just return -ENODEV
> when the graph or remote node is missing: we must also check that there is no
> child node that is a panel/bridge.
> 
> For the graph remote case, we can reliabily return -EPROBE_DEFER when
> of_graph_is_present and the remote exists and of_device_is_available.
> Otherwise we can go for -ENODEV. I think getting -EPROBE_DEFER at this point
> should stop the drm_of_find_panel_or_bridge process.
> 
> On the other hand for the child panel/bridge node case, I don't see how we
> can reliably distinguish between -EPROBE_DEFER and -ENODEV, because
> of_drm_find_panel and of_drm_find_bridge will behave the same if the child
> node is a not-yet-probed panel/bridge or a totally unrelated node.
> So I think we should always return -EPROBE_DEFER in that case.
> 
> As a result you can't get -ENODEV if using the of graph while having any
> (unrelated) child node there, so your issue remains.
> 
> Do you see any way we could make this work?
> 
> > I thought the appropriate method of referencing the dsi panel was to
> > actually reference that using the of_graph, even though it's a child of
> > the dsi controller - that's at least how we've done it in e.g. [1].
> > I find this to be much nicer than to just blindly define that all
> > children of any sort of display controller must be a bridge or a panel.
> 
> Yes I totally agree. Given that using the child node directly apparently
> can't allow us to distinguish between -EPROBE_DEFER/-ENODEV I would be in
> favor of dropping this mechanism and going with explicit of graph in any case
> (even if it's a child node). I don't see any downside to this approach.
> 
> What do yout think?

This patch has recently starting causing failures on various Tegra
devices that use drm_of_find_panel_or_bridge() for the case where the
output might be connected to an eDP or LVDS panel. However, that same
output could also be connected to an HDMI or DP monitor, in which case
we obviously don't want a DT representation because it's all
discoverable.

If I understand correctly, that's similar to what Bjorn described. In my
case I was able to fix the regression by returning -ENODEV at the very
end of the function (i.e. no matching ports were found and no graph is
present).

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists