On Sun, Dec 8, 2019 at 3:24 PM Rob Clark <robdclark@xxxxxxxxx> wrote:
>
> On Sun, Dec 8, 2019 at 10:28 AM Laurent Pinchart
> <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
> >
> > Hi Rob,
> >
> > On Sun, Dec 08, 2019 at 08:50:32AM -0800, Rob Clark wrote:
> > > On Sun, Dec 8, 2019 at 6:45 AM Laurent Pinchart wrote:
> > > > On Sat, Dec 07, 2019 at 12:35:50PM -0800, Rob Clark wrote:
> > > > > From: Rob Clark <robdclark@xxxxxxxxxxxx>
> > > > >
> > > > > For devices that have one of several possible panels installed, the
> > > > > panel-id property gives firmware a generic way to locate and enable the
> > > > > panel node corresponding to the installed panel. Example of how to use
> > > > > this property:
> > > > >
> > > > > ivo_panel {
> > > > > compatible = "ivo,m133nwf4-r0";
> > > > > 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>;
> > > > > };
> > > > > };
> > > > > };
> > > > > };
> > > > >
> > > > > Signed-off-by: Rob Clark <robdclark@xxxxxxxxxxxx>
> > > > > ---
> > > > > .../bindings/display/panel/panel-common.yaml | 26 +++++++++++++++++++
> > > > > 1 file changed, 26 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/display/panel/panel-common.yaml b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > > > > index ef8d8cdfcede..6113319b91dd 100644
> > > > > --- a/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > > > > +++ b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > > > > @@ -75,6 +75,32 @@ properties:
> > > > > in the device graph bindings defined in
> > > > > Documentation/devicetree/bindings/graph.txt.
> > > > >
> > > > > + panel-id:
> > > > > + description:
> > > > > + To support the case where one of several different panels can be installed
> > > > > + on a device, the panel-id property can be used by the firmware to identify
> > > > > + which panel should have it's status changed to "ok". This property is not
> > > > > + used by the HLOS itself.
> > > >
> > > > If your firmware can modify the status property of a panel, it can also
> > > > add DT nodes. As discussed before, I don't think this belongs to DT.
> > > > Even if panel-id isn't used by the operating system, you have Linux
> > > > kernel patches in this series that show that this isn't transparent.
> > >
> > > I've already explained several times why this is not feasible. It
> > > would require DtbLoader to be familiar with each individual device,
> > > and be rev'd every time a new device appears. That is not practical
> > > at all.
> > >
> > > (And fwiw, the ACPI tables describe each panel.. with an ACPI method
> > > that is passed the the panel-id and returns the appropriate table..
> > > since DT doesn't have methods, this is the solution.)
> > >
> > > I stand by this patch, we can't keep running away from this problem
> > > and wave the magic firmware wand.
> >
> > I believe in firmware solutions more than firmware magic wands :-)
> >
>
> and with that in mind, I think I've come up with a firmware solution,
> in the form of dtb overlays :-)
>
> I've managed to get DtbLoader to find and load a panel overlay based
> on the panel-id it reads, which drops all patches in the patchset
> except the last one, which now has this delta:
This looks good to me. The only slight concern I have with it is
making the overlay filename an ABI. I don't have a better suggestion
though. How would this work for other vendors or the same panel ID
(for different panels) used on different platforms? For different
vendors at least, I guess dtbloader gets the base dtb path somehow and
the overlay's are relative to that?
> ---------
> diff --git a/arch/arm64/boot/dts/qcom/Makefile
> b/arch/arm64/boot/dts/qcom/Makefile
> index 6498a1ec893f..1a61e8da2521 100644
> --- a/arch/arm64/boot/dts/qcom/Makefile
> +++ b/arch/arm64/boot/dts/qcom/Makefile
> @@ -1,4 +1,5 @@
> # SPDX-License-Identifier: GPL-2.0
> +subdir-y += panels
> dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc.dtb
> dtb-$(CONFIG_ARCH_QCOM) += apq8096-db820c.dtb
> dtb-$(CONFIG_ARCH_QCOM) += ipq8074-hk01.dtb
> diff --git a/arch/arm64/boot/dts/qcom/panels/Makefile
> b/arch/arm64/boot/dts/qcom/panels/Makefile
> new file mode 100644
> index 000000000000..dbf55f423555
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/panels/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +dtb-$(CONFIG_ARCH_QCOM) += panel-c4.dtb
> +dtb-$(CONFIG_ARCH_QCOM) += panel-c5.dtb
> diff --git a/arch/arm64/boot/dts/qcom/panels/panel-c4.dts
> b/arch/arm64/boot/dts/qcom/panels/panel-c4.dts
> new file mode 100644
> index 000000000000..ebcf65419dad
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/panels/panel-c4.dts
> @@ -0,0 +1,17 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +/*
> + * Panel overlay for panel-id 0xc4
> + *
> + * Copyright (c) 2019, Linaro Ltd.
> + */
> +
> +/dts-v1/;
> +/plugin/;
> +/ {
> + fragment@0 {
> + target-path = "/panel";
> + __overlay__ {
> + compatible = "boe,nv133fhm-n61";
> + };
> + };
> +};
> diff --git a/arch/arm64/boot/dts/qcom/panels/panel-c5.dts
> b/arch/arm64/boot/dts/qcom/panels/panel-c5.dts
> new file mode 100644
> index 000000000000..0ad5bb6003e3
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/panels/panel-c5.dts
> @@ -0,0 +1,17 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +/*
> + * Panel overlay for panel-id 0xc5
> + *
> + * Copyright (c) 2019, Linaro Ltd.
> + */
> +
> +/dts-v1/;
> +/plugin/;
> +/ {
> + fragment@0 {
> + target-path = "/panel";
> + __overlay__ {
> + compatible = "ivo,m133nwf4-r0";
> + };
> + };
> +};
> diff --git a/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> index c35d8099d8eb..92c76afb721c 100644
> --- a/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> @@ -22,11 +22,13 @@
> hsuart0 = &uart6;
> };
>
> + /*
> + * stub node which defines how panel is connected to bridge, which
> + * will be updated by panel specific overlay
> + */
> panel {
> - compatible = "ivo,m133nwf4-r0";
> power-supply = <&vlcm_3v3>;
> no-hpd;
> -
> ports {
> port {
> panel_in_edp: endpoint {
> ---------
>
> Side note, try as I might, I couldn't get the 'target = <&phandle>'
> approach to work in the overlays, so I ended up going with target-path
> instead. From digging thru the fdt_overlay code, I *think* it is
> because I end up w/ an overlay dtb without symbols. In the end, I
> guess target-path works just as well.
It's the base dtb that needs the symbols I think.
BTW, to answer the question on #dri-devel, if you wanted to put the
full panel into an overlay, the way to solve the problem of having
bridge specific knowledge is defining a connector node. That should
provide enough abstraction. Presumably the connector is actually the
same across panels in this situation, so that should match up with the
actual h/w. It could be possible to have a different physical
connector populated for each possible panel, but hopefully that's not
the common case.
Rob
