| Message ID | 20190630203614.5290-2-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | drm+dt+efi: support devices with multiple possible panels | expand |
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'? > + power-supply = <&vlcm_3v3>; > + no-hpd; > + > + ports { > + port { > + boe_panel_in_edp: endpoint { > + remote-endpoint = <&sn65dsi86_out_boe>; > + }; > + }; > + }; > + }; > + > + display_or_bridge_device { > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + ... > + > + port@0 { > + #address-cells = <1>; > + #size-cells = <0>; > + reg = <0>; > + > + endpoint@c4 { > + reg = <0xc4>; What does this number represent? It is supposed to be defined by the display_or_bridge_device, not a specific panel. We also need to consider how the DSI case with panels as children of the DSI controller would work and how this would work with multiple displays each having multiple panel options. Rob
On 7/1/2019 8:03 AM, Rob Herring 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? Its platform specific. In the devices that Rob Clark seems interested in, there are multiple mechanisms in place - read a gpio, enable the DSI and send a specific command to the panel controller asking for its panel id, or read some efuses. The efuses may not be accessible by Linux. The DSI solution is problematic because it causes a chicken and egg situation where linux needs the DT to probe the DSI driver to query the panel, in order to edit the DT to probe DSI/panel. In the systems Rob Clark is interested in, the FW already provides a specific EFI variable with the panel id encoded in it for HLOS to use (although this is broken on some of the devices), but this is a specific vendor's solution. The FW/bootloader has probably already figured out the panel details and brought up the display for a boot splash, bios menu, etc. I'm not sure it makes a lot of sense to define N mechanisms for linux to figure out the same across every platform/vendor. > >> + >> +/ { >> + 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'? > >> + power-supply = <&vlcm_3v3>; >> + no-hpd; >> + >> + ports { >> + port { >> + boe_panel_in_edp: endpoint { >> + remote-endpoint = <&sn65dsi86_out_boe>; >> + }; >> + }; >> + }; >> + }; >> + >> + display_or_bridge_device { >> + >> + ports { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + ... >> + >> + port@0 { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + reg = <0>; >> + >> + endpoint@c4 { >> + reg = <0xc4>; > > What does this number represent? It is supposed to be defined by the > display_or_bridge_device, not a specific panel. Its the specific FW/bootloader defined panel id, that matches the above defined panel-id property. > > We also need to consider how the DSI case with panels as children of > the DSI controller would work and how this would work with multiple > displays each having multiple panel options. > > Rob >
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? see jhugo's response, he has I guess more access to the bootloader than I > > + > > +/ { > > + 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'? I don't think we can rely on bootloader knowing a thing about DT on these devices.. OTOH I don't really think it is a big problem for both panels to probe. But I suppose it might be possible to have something somewhere in the kernel that realizes and disables the unused panels. > > + power-supply = <&vlcm_3v3>; > > + no-hpd; > > + > > + ports { > > + port { > > + boe_panel_in_edp: endpoint { > > + remote-endpoint = <&sn65dsi86_out_boe>; > > + }; > > + }; > > + }; > > + }; > > + > > + display_or_bridge_device { > > + > > + ports { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + ... > > + > > + port@0 { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + reg = <0>; > > + > > + endpoint@c4 { > > + reg = <0xc4>; > > What does this number represent? It is supposed to be defined by the > display_or_bridge_device, not a specific panel. it matches /chosen/panel-id.. in this case I'm not sure how the panel-id's are assigned, but for our purposes all that matters is that they are assigned. > We also need to consider how the DSI case with panels as children of > the DSI controller would work and how this would work with multiple > displays each having multiple panel options. In the non-bridge case (panel hooked directly to dsi controller), the dsi controller could use the same ports {} mechanism. For multiple displays, we could extend, I suppose, /chosen/panel-id to be an array of id's indexed by display. I think this is the type of extension we could do later when the use-case comes up. Just having this solved upstream for single display would already be a huge advancement. (You don't want to look at how this is solved downstream for android phones.) Btw, if you are curious how this works on windows/ACPI, the ACPI tables have entries for each of the panels. The kernel is expected to take the panel-id from that EFI variable that jhugo mentioned, and pass it to a _ROM method which returns the appropriate panel table. (Not entirely sure how the orchestrate reading the EFI variable early, since it does not appear to be available after ExitBootServices) BR, -R
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
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
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. + +/ { + 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>; + }; + }; + }; + }; + + display_or_bridge_device { + + ports { + #address-cells = <1>; + #size-cells = <0>; + + ... + + port@0 { + #address-cells = <1>; + #size-cells = <0>; + reg = <0>; + + endpoint@c4 { + reg = <0xc4>; + remote-endpoint = <&boe_panel_in_edp>; + }; + + endpoint@c5 { + reg = <0xc5>; + remote-endpoint = <&ivo_panel_in_edp>; + }; + }; + }; + } +}; + +Note that panel-id values can be sparse (ie. not just integers 0..n). + linux,booted-from-kexec -----------------------