Message-ID: <Yk1Prpew7Sn//tZN@aptenodytes>
Date:   Wed, 6 Apr 2022 10:30:38 +0200
From:   Paul Kocialkowski <paul.kocialkowski@...tlin.com>
To:     Thierry Reding <thierry.reding@...il.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

Hi Thierry,

On Tue 05 Apr 22, 19:00, Thierry Reding wrote:
> 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).

Okay so I'll send a patch that returns -ENODEV when possible, but there
is still a case where we can't distinguish between -EPROBE_DEFER and -ENODEV,
which calls for removing the whole child node-based mechanism.

Paul

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

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

Powered by blists - more mailing lists