Message-ID: <20220426131944.b26rqqmk7gpcplbg@houat>
Date:   Tue, 26 Apr 2022 15:19:44 +0200
From:   Maxime Ripard <maxime@...no.tech>
To:     Paul Kocialkowski <paul.kocialkowski@...tlin.com>
Cc:     Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Jagan Teki <jagan@...rulasolutions.com>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Thierry Reding <thierry.reding@...il.com>,
        Rob Clark <robdclark@...il.com>, linux-kernel@...r.kernel.org,
        dri-devel@...ts.freedesktop.org,
        Linus Walleij <linus.walleij@...aro.org>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Robert Foss <robert.foss@...aro.org>
Subject: Re: [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or
 bridge"

On Tue, Apr 26, 2022 at 03:04:17PM +0200, Paul Kocialkowski wrote:
> On Tue 26 Apr 22, 14:55, Maxime Ripard wrote:
> > On Tue, Apr 26, 2022 at 02:54:01PM +0200, Maxime Ripard wrote:
> > > On Tue, Apr 26, 2022 at 02:41:44PM +0200, Paul Kocialkowski wrote:
> > > > On Tue 26 Apr 22, 14:33, Laurent Pinchart wrote:
> > > > > On Tue, Apr 26, 2022 at 09:54:36AM +0200, Paul Kocialkowski wrote:
> > > > > > On Thu 21 Apr 22, 10:59, Paul Kocialkowski wrote:
> > > > > > > On Thu 21 Apr 22, 10:23, Maxime Ripard wrote:
> > > > > > > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote:
> > > > > > > > > + Linus
> > > > > > > > > + Marek
> > > > > > > > > + Laurent
> > > > > > > > > + Robert
> > > > > > > > > 
> > > > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson wrote:
> > > > > > > > > >
> > > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or
> > > > > > > > > > bridge")' attempted to simplify the case of expressing a simple panel
> > > > > > > > > > under a DSI controller, by assuming that the first non-graph child node
> > > > > > > > > > was a panel or bridge.
> > > > > > > > > >
> > > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a
> > > > > > > > > > panel or bridge.  Examples of this can be a aux-bus in the case of
> > > > > > > > > > DisplayPort, or an opp-table represented before the panel node.
> > > > > > > > > >
> > > > > > > > > > In these cases the reverted commit prevents the caller from ever finding
> > > > > > > > > > a reference to the panel.
> > > > > > > > > >
> > > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has
> > > > > > > > > > panel or bridge")', in favor of using an explicit graph reference to the
> > > > > > > > > > panel in the trivial case as well.
> > > > > > > > > 
> > > > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge
> > > > > > > > > switched drivers.  Do you have any suggestions on how to proceed to
> > > > > > > > > succeed in those use cases as well?
> > > > > > > > 
> > > > > > > > I guess we could create a new helper for those, like
> > > > > > > > devm_drm_of_get_bridge_with_panel, or something.
> > > > > > > 
> > > > > > > Oh wow I feel stupid for not thinking about that.
> > > > > > > 
> > > > > > > Yeah I agree that it seems like the best option.
> > > > > > 
> > > > > > Should I prepare a patch with such a new helper?
> > > > > > 
> > > > > > The idea would be to keep drm_of_find_panel_or_bridge only for the of graph
> > > > > > case and add one for the child node case, maybe:
> > > > > > drm_of_find_child_panel_or_bridge.
> > > > > > 
> > > > > > I really don't have a clear idea of which driver would need to be switched
> > > > > > over though. Could someone (Jagan?) let me know where it would be needed?
> > > > > > 
> > > > > > Are there cases where we could both expect of graph and child node?
> > > > > > (i.e. does the new helper also need to try via of graph?)
> > > > > 
> > > > > I still think we should use OF graph uncondtionally, even in the DSI
> > > > > case. We need to ensure backward-compatibility, but I'd like new
> > > > > bindings (and thus new drivers) to always use OF graph.
> > > > 
> > > > I just went over the thread on "drm: of: Improve error handling in bridge/panel
> > > > detection" again and I'm no longer sure there's actually still an issue that
> > > > stands, with the fix that allows returning -ENODEV when possible.
> > > > 
> > > > The remaining issue that was brought up was with a connector node, but it should
> > > > be up to the driver to detect that and avoid calling drm_of_find_panel_or_bridge
> > > > in such situations.
> > > > 
> > > > So with that in mind it feels like the child node approach can be viable
> > > > (and integrated in the same helper).
> > > > 
> > > > We might still want to favor an explicit OF graph approach, but note that
> > > > dsi-controller.yaml also specifies extra properties that are specific to
> > > > MIPI DSI and I'm not sure there are equivalent definitions for the OF graph
> > > > approach.
> > > > 
> > > > What do you think?
> > > 
> > > I don't think Laurent's point was to move the child node away from its
> > > DSI controller, that part doesn't make much sense. The panel or bridge
> > > is still accessed through the DSI bus, so it very much belongs there.
> > > 
> > > What he meant I think was that we mandate the OF graph for all panels,
> > > so for panels/bridges controlled through DCS, you would still list the
> > > output through the graph.
> > 
> > Also, we're already in a bit of a mess right now. I don't think rushing
> > that kind of patches in a (late) rc is making much sense, but as I said,
> > if you want to start working on this, then I'll take a revert for the
> > next rc, and then we can work calmly on this.
> 
> As I understand it we either have some broken stuff because of the revert of:
> - drm: of: Lookup if child node has panel or bridge
> - drm: of: Properly try all possible cases for bridge/panel detection
> 
> because the child node is already used in places, or we can have broken stuff
> because with the patches because with these two patches -ENODEV is no longer
> returned.
> 
> Now with the extra patch that I sent:
> - drm: of: Improve error handling in bridge/panel detection
> 
> we get -ENODEV back, except for the connector case but this one should be
> handled in drivers directly and drm_of_find_panel_or_bridge should not be
> called in that situation.
> 
> So all in all it seems that all the pieces are there, unless I'm missing
> something.
> 
> What do you think?

If Bjorn and Thierry can confirm that it indeeds work in their case,
I'll be happy to apply those patches as well.

Maxime

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

Powered by blists - more mailing lists