Re: [PATCH 1/4] dt-bindings: chosen: document panel-id binding

[Rob Clark <robdclark@gmail.com>]

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>;
        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