Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190630203614.5290-1-robdclark@gmail.com> <20190630203614.5290-2-robdclark@gmail.com>
 <CAL_JsqKMULJJ9CERRBpqd7Y2dtovEJ6jcDKy6J4yR6rAdjibUg@mail.gmail.com> <CAF6AEGsAgUsKJjLQkRDdjZtSvX267Cz_m_P7_m6VeJ2u=Ozc1g@mail.gmail.com>
In-Reply-To: <CAF6AEGsAgUsKJjLQkRDdjZtSvX267Cz_m_P7_m6VeJ2u=Ozc1g@mail.gmail.com>
From:   Rob Clark <robdclark@gmail.com>
Date:   Sat, 30 Nov 2019 10:39:10 -0800
Message-ID: <CAF6AEGtJg3vGSqrpR4bVN8SZFCiaSK8GpAr0Om5BQuFQ1OvLjw@mail.gmail.com>
Subject: Re: [PATCH 1/4] dt-bindings: chosen: document panel-id binding
To:     Rob Herring <robh+dt@kernel.org>
Cc:     dri-devel <dri-devel@lists.freedesktop.org>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>,
        freedreno <freedreno@lists.freedesktop.org>,
        aarch64-laptops@lists.linaro.org,
        Rob Clark <robdclark@chromium.org>,
        Mark Rutland <mark.rutland@arm.com>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Vasily Khoruzhick <anarsoul@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sat, Nov 30, 2019 at 10:37 AM Rob Clark <robdclark@gmail.com> wrote:
>
> On Mon, Jul 1, 2019 at 7:03 AM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Sun, Jun 30, 2019 at 2:36 PM Rob Clark <robdclark@gmail.com> wrote:
> > >
> > > From: Rob Clark <robdclark@chromium.org>
> > >
> > > The panel-id property in chosen can be used to communicate which panel,
> > > of multiple possibilities, is installed.
> > >
> > > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > > ---
> > >  Documentation/devicetree/bindings/chosen.txt | 69 ++++++++++++++++++++
> > >  1 file changed, 69 insertions(+)
> >
> > I need to update this file to say it's moved to the schema repository...
> >
> > But I don't think that will matter...
> >
> > > diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
> > > index 45e79172a646..d502e6489b8b 100644
> > > --- a/Documentation/devicetree/bindings/chosen.txt
> > > +++ b/Documentation/devicetree/bindings/chosen.txt
> > > @@ -68,6 +68,75 @@ on PowerPC "stdout" if "stdout-path" is not found.  However, the
> > >  "linux,stdout-path" and "stdout" properties are deprecated. New platforms
> > >  should only use the "stdout-path" property.
> > >
> > > +panel-id
> > > +--------
> > > +
> > > +For devices that have multiple possible display panels (multi-sourcing the
> > > +display panels is common on laptops, phones, tablets), this allows the
> > > +bootloader to communicate which panel is installed, e.g.
> >
> > How does the bootloader figure out which panel? Why can't the kernel
> > do the same thing?
> >
> > > +
> > > +/ {
> > > +       chosen {
> > > +               panel-id = <0xc4>;
> > > +       };
> > > +
> > > +       ivo_panel {
> > > +               compatible = "ivo,m133nwf4-r0";
> > > +               power-supply = <&vlcm_3v3>;
> > > +               no-hpd;
> > > +
> > > +               ports {
> > > +                       port {
> > > +                               ivo_panel_in_edp: endpoint {
> > > +                                       remote-endpoint = <&sn65dsi86_out_ivo>;
> > > +                               };
> > > +                       };
> > > +               };
> > > +       };
> > > +
> > > +       boe_panel {
> > > +               compatible = "boe,nv133fhm-n61";
> >
> > Both panels are going to probe. So the bootloader needs to disable the
> > not populated panel setting 'status' (or delete the node). If you do
> > that, do you even need 'panel-id'?
> >
>
> So, I'm finally having some time to revisit this proposal..  I have a
> few updates:
>
> + Updated DtbLoader.efi to read UEFIDisplayInfo and get the panel-id
>   so I can drop the efi/libstub patch[1]
> + I can drop drm_of_find_panel_id() and fold the logic to look at
>   /chosen/panel-id into drm_of_find_panel_or_bridge() so that each
>   driver or bridge doesn't need an update
>
> This doesn't realy solve the issue that both panels will probe.  On
> the other hand, I really don't want to make the DtbLoader know enough
> about the dt structure of each laptop to patch dt, since that is not
> going to be scalable at all.  (Likewise, there is some interest for
> devices that use u-boot to take the panel-id from a uboot env var and
> patch dt, but again knowing enough to intelligently patch the dt is
> not going to be feasible.)
>
> But, an alternate solution could be, instead of adding a 'panel-id'
> node in chosen, I could add it as an optional property in the panel
> node.  So taking my original example of the yoga c630 laptops, with
> the two possible panel id's 0xc4 and 0xc5:
>
>     ivo_panel {
>         compatible = "ivo,m133nwf4-r0";
>         panel-id = <0xc4>;

correction, the ivo panel should have panel-id = <0xc5>

>         status = "disabled";
>
>         ports {
>             port {
>                 ivo_panel_in_edp: endpoint {
>                     remote-endpoint = <&sn65dsi86_out_ivo>;
>                 };
>             };
>         };
>     };
>
>     boe_panel {
>         compatible = "boe,nv133fhm-n61";
>         panel-id = <0xc4>;
>         status = "disabled";
>
>         ports {
>             port {
>                 boe_panel_in_edp: endpoint {
>                     remote-endpoint = <&sn65dsi86_out_boe>;
>                 };
>             };
>         };
>     };
>
>     sn65dsi86: bridge@2c {
>         compatible = "ti,sn65dsi86";
>
>         ports {
>             #address-cells = <1>;
>             #size-cells = <0>;
>
>             port@0 {
>                 reg = <0>;
>                 sn65dsi86_in_a: endpoint {
>                     remote-endpoint = <&dsi0_out>;
>                 };
>             };
>
>             port@1 {
>                 reg = <1>;
>
>                 sn65dsi86_out_boe: endpoint@c4 {
>                     remote-endpoint = <&boe_panel_in_edp>;
>                 };
>
>                 sn65dsi86_out_ivo: endpoint@c5 {
>                     remote-endpoint = <&ivo_panel_in_edp>;
>                 };
>             };
>         };
>     };
>
> With this, the "firmware" (DtbLoader, u-boot, etc) could crawl the
> entire dt looking for a node with a panel-id that matches the one it's
> looking for, and change that node's status to enabled.
>
> The advantage would be that the other panel(s) that is not installed
> will not be probed.  The downsides are that (1) the drm drivers would
> have to loop over all the endpoints to find the active panel (some
> drivers do this already, most do not), and (2) the property name
> "panel-id" (or whatever we pick) will now be somehow special, you
> couldn't re-use that name elsewhere without potential to confuse the
> firmware.  And it is more complexity in the firmware (although at
> least it can be done generically) compared to just adding a property
> in chosen.
>
> Not sure if this is better, I thought my initial proposal was more
> elegant.  I am open to other suggestions, anything other than teaching
> DtbLoader/u-boot about the specific dt of each different device that
> would use this.
>
> BR,
> -R
>
> [1] https://github.com/robclark/edk2/commits/dtbloader