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>
In-Reply-To: <20190630203614.5290-1-robdclark@gmail.com>
From:   Rob Clark <robdclark@gmail.com>
Date:   Tue, 2 Jul 2019 05:50:36 -0700
Message-ID: <CAF6AEGv8EJPmje_8bpK8LmLdLFkOSQVJOg_CTS7C_HwVB6i9eQ@mail.gmail.com>
Subject: Re: [PATCH 0/4] drm+dt+efi: support devices with multiple possible panels
To:     dri-devel <dri-devel@lists.freedesktop.org>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>
Cc:     freedreno <freedreno@lists.freedesktop.org>,
        aarch64-laptops@lists.linaro.org,
        Rob Clark <robdclark@chromium.org>,
        Ard Biesheuvel <ard.biesheuvel@linaro.org>,
        Catalin Marinas <catalin.marinas@arm.com>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>, Ingo Molnar <mingo@kernel.org>,
        Julien Thierry <julien.thierry@arm.com>,
        Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
        "open list:EXTENSIBLE FIRMWARE INTERFACE (EFI)" 
        <linux-efi@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>,
        Lukas Wunner <lukas@wunner.de>,
        Steve Capper <steve.capper@arm.com>,
        Will Deacon <will@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, Jun 30, 2019 at 1:36 PM Rob Clark <robdclark@gmail.com> wrote:
>
> From: Rob Clark <robdclark@chromium.org>
>
> Now that we can deal gracefully with bootloader (firmware) initialized
> display on aarch64 laptops[1], the next step is to deal with the fact
> that the same model of laptop can have one of multiple different panels.
> (For the yoga c630 that I have, I know of at least two possible panels,
> there might be a third.)
>
> This is actually a scenario that comes up frequently in phones and
> tablets as well, so it is useful to have an upstream solution for this.
>
> The basic idea is to add a 'panel-id' property in dt chosen node, and
> use that to pick the endpoint we look at when loading the panel driver,
> e.g.
>
> / {
>         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";
>                 power-supply = <&vlcm_3v3>;
>                 no-hpd;
>
>                 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@1 {
>                                 #address-cells = <1>;
>                                 #size-cells = <0>;
>                                 reg = <1>;
>
>                                 endpoint@c4 {
>                                         reg = <0xc4>;
>                                         remote-endpoint = <&boe_panel_in_edp>;
>                                 };
>
>                                 endpoint@c5 {
>                                         reg = <0xc5>;
>                                         remote-endpoint = <&ivo_panel_in_edp>;
>                                 };
>                         };
>                 };
>         }
> };
>

Just to put out an alternative idea for how to handle this in DT
(since Laurent seemed unhappy with the idea of using endpoints to
describe multiple connections between ports that *might* exist.

This approach would require of_drm_find_panel() to check the
device_node to see if it is a special "panel-select" thing.  (I think
we could use device_node::data to recover the actual selected panel.)

On the plus side, it would work for cases that aren't using of_graph
to connect display/bridge to panel, so it would be pretty much
transparent to drivers and bridges.

And I guess it could be extended to cases where gpio's are used to
detect which panel is attached..  not sure how far down that road I
want to go, as jhugo mentioned elsewhere on this patchset, in some
cases dsi is used to select (which could be problematic to do from
kernel if display is already active in video mode scanout), or efuses
which aren't accessible from kernel.


    chosen {
        panel-id = <0xc4>;
    };

    panel_select {
        compatible = "linux,panel-select";
        #address-cells = <1>;
        #size-cells = <0>;

        boe_panel {
            compatible = "boe,nv133fhm-n61";
            reg = <0xc4>;
            power-supply = <&vlcm_3v3>;
            no-hpd;
        };

        ivo_panel {
            compatible = "ivo,m133nwf4-r0";
            reg = <0xc5>;
            power-supply = <&vlcm_3v3>;
            no-hpd;
        };

        ports {
            port {
                panel_in_edp: endpoint {
                    remote-endpoint = <&sn65dsi86_out>;
                };
            };
        };
    };

    dsi_controller_or_bridge {
        ...
        ports {
            ...
            port@1 {
                reg = <1>;
                sn65dsi86_out: endpoint {
                    remote-endpoint = <&panel_in_edp>;
                };
            };
        };
    };

Personally I find my original proposal more natural (which is why I
went with it, this second idea is more similar to what I initially had
in mind before looking at of_graph).  And it seems to be a bit weird
to have a panel_select thing which isn't really hardware.

BR,
-R