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>; }; }; }; } };
Note that the panel-id is potentially a sparse-int. The values I've seen so far on aarch64 laptops are:
* 0xc2 * 0xc3 * 0xc4 * 0xc5 * 0x8011 * 0x8012 * 0x8055 * 0x8056
At least on snapdragon aarch64 laptops, they can be any u32 value.
However, on these laptops, the bootloader/firmware is not populating the chosen node, but instead providing an "UEFIDisplayInfo" variable, which contains the panel id. Unfortunately EFI variables are only available before ExitBootServices, so the second patch checks for this variable before EBS and populates the /chosen/panel-id variable.
[1] https://patchwork.freedesktop.org/series/63001/
Rob Clark (4): dt-bindings: chosen: document panel-id binding efi/libstub: detect panel-id drm: add helper to lookup panel-id drm/bridge: ti-sn65dsi86: use helper to lookup panel-id
Documentation/devicetree/bindings/chosen.txt | 69 ++++++++++++++++++++ drivers/firmware/efi/libstub/arm-stub.c | 49 ++++++++++++++ drivers/firmware/efi/libstub/efistub.h | 2 + drivers/firmware/efi/libstub/fdt.c | 9 +++ drivers/gpu/drm/bridge/ti-sn65dsi86.c | 5 +- drivers/gpu/drm/drm_of.c | 21 ++++++ include/drm/drm_of.h | 7 ++ 7 files changed, 160 insertions(+), 2 deletions(-)
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(+)
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 -----------------------
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
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
From: Rob Clark robdclark@chromium.org
On snapdragon aarch64 laptops, a 'UEFIDisplayInfo' variable is provided to communicate some information about the display. Crutially it has the panel-id, so the appropriate panel driver can be selected. Read this out and stash in /chosen/panel-id so that display driver can use it to pick the appropriate panel.
Signed-off-by: Rob Clark robdclark@chromium.org --- drivers/firmware/efi/libstub/arm-stub.c | 49 +++++++++++++++++++++++++ drivers/firmware/efi/libstub/efistub.h | 2 + drivers/firmware/efi/libstub/fdt.c | 9 +++++ 3 files changed, 60 insertions(+)
diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c index 04e6ecd72cd9..999813252e0d 100644 --- a/drivers/firmware/efi/libstub/arm-stub.c +++ b/drivers/firmware/efi/libstub/arm-stub.c @@ -69,6 +69,53 @@ static struct screen_info *setup_graphics(efi_system_table_t *sys_table_arg) return si; }
+/* + * We (at least currently) don't care about most of the fields, just + * panel_id: + */ +struct mdp_disp_info { + u32 version_info; + u32 pad0[9]; + u32 panel_id; + u32 pad1[17]; +}; + +#define MDP_DISP_INFO_VERSION_MAGIC 0xaa + +static void get_panel_id(efi_system_table_t *sys_table_arg, + unsigned long fdt_addr) +{ + efi_guid_t gop_proto = EFI_GRAPHICS_OUTPUT_PROTOCOL_GUID; + efi_status_t status; + struct mdp_disp_info *disp_info; + unsigned long size = 0; + + status = efi_call_runtime(get_variable, L"UEFIDisplayInfo", + &gop_proto, NULL, &size, NULL); + if (status == EFI_NOT_FOUND) + return; + + status = efi_call_early(allocate_pool, EFI_LOADER_DATA, size, + (void **)&disp_info); + if (status != EFI_SUCCESS) + return; + + status = efi_call_runtime(get_variable, L"UEFIDisplayInfo", + &gop_proto, NULL, &size, disp_info); + if (status != EFI_SUCCESS) + goto cleanup; + + if ((disp_info->version_info >> 16) != MDP_DISP_INFO_VERSION_MAGIC) + goto cleanup; + + efi_printk(sys_table_arg, "found a panel-id!\n"); + + set_chosen_panel_id(fdt_addr, disp_info->panel_id); + +cleanup: + efi_call_early(free_pool, disp_info); +} + void install_memreserve_table(efi_system_table_t *sys_table_arg) { struct linux_efi_memreserve *rsv; @@ -229,6 +276,8 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table, if (!fdt_addr) pr_efi(sys_table, "Generating empty DTB\n");
+ get_panel_id(sys_table, fdt_addr); + status = handle_cmdline_files(sys_table, image, cmdline_ptr, "initrd=", efi_get_max_initrd_addr(dram_base, *image_addr), diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h index 1b1dfcaa6fb9..8832cb9a7a40 100644 --- a/drivers/firmware/efi/libstub/efistub.h +++ b/drivers/firmware/efi/libstub/efistub.h @@ -39,6 +39,8 @@ void efi_char16_printk(efi_system_table_t *, efi_char16_t *);
unsigned long get_dram_base(efi_system_table_t *sys_table_arg);
+void set_chosen_panel_id(unsigned long fdt_addr, unsigned panel_id); + efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table, void *handle, unsigned long *new_fdt_addr, diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c index 5440ba17a1c5..cb6ea160a40a 100644 --- a/drivers/firmware/efi/libstub/fdt.c +++ b/drivers/firmware/efi/libstub/fdt.c @@ -200,6 +200,15 @@ static efi_status_t update_fdt_memmap(void *fdt, struct efi_boot_memmap *map) return EFI_SUCCESS; }
+void set_chosen_panel_id(unsigned long fdt_addr, unsigned panel_id) +{ + void *fdt = (void *)fdt_addr; + int node = fdt_subnode_offset(fdt, 0, "chosen"); + u32 fdt_val32 = cpu_to_fdt32(panel_id); + + fdt_setprop_var(fdt, node, "panel-id", fdt_val32); +} + #ifndef EFI_FDT_ALIGN # define EFI_FDT_ALIGN EFI_PAGE_SIZE #endif
On Sun, 30 Jun 2019 at 22:36, Rob Clark robdclark@gmail.com wrote:
From: Rob Clark robdclark@chromium.org
On snapdragon aarch64 laptops, a 'UEFIDisplayInfo' variable is provided to communicate some information about the display. Crutially it has the panel-id, so the appropriate panel driver can be selected. Read this out and stash in /chosen/panel-id so that display driver can use it to pick the appropriate panel.
Signed-off-by: Rob Clark robdclark@chromium.org
Hi Rob,
I understand why you are doing this, but this *really* belongs elsewhere.
So we are dealing with a platform that violates the UEFI spec, since it does not bother to implement variable services at runtime (because MS let the vendor get away with this).
First of all, to pass data between the EFI stub and the OS proper, we should use a configuration table rather than a DT property, since the former is ACPI/DT agnostic. Also, I'd like the consumer of the data to actually interpret it, i.e., just dump the whole opaque thing into a config table in the stub, and do the parsing in the OS proper.
However, I am not thrilled at adding code to the stub that unconditionally looks for some variable with some magic name on all ARM/arm64 EFI systems, so this will need to live under a Kconfig option that depends on ARM64 (and does not default to y)
drivers/firmware/efi/libstub/arm-stub.c | 49 +++++++++++++++++++++++++ drivers/firmware/efi/libstub/efistub.h | 2 + drivers/firmware/efi/libstub/fdt.c | 9 +++++ 3 files changed, 60 insertions(+)
diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c index 04e6ecd72cd9..999813252e0d 100644 --- a/drivers/firmware/efi/libstub/arm-stub.c +++ b/drivers/firmware/efi/libstub/arm-stub.c @@ -69,6 +69,53 @@ static struct screen_info *setup_graphics(efi_system_table_t *sys_table_arg) return si; }
+/*
- We (at least currently) don't care about most of the fields, just
- panel_id:
- */
+struct mdp_disp_info {
u32 version_info;u32 pad0[9];u32 panel_id;u32 pad1[17];+};
+#define MDP_DISP_INFO_VERSION_MAGIC 0xaa
+static void get_panel_id(efi_system_table_t *sys_table_arg,
unsigned long fdt_addr)+{
efi_guid_t gop_proto = EFI_GRAPHICS_OUTPUT_PROTOCOL_GUID;efi_status_t status;struct mdp_disp_info *disp_info;unsigned long size = 0;status = efi_call_runtime(get_variable, L"UEFIDisplayInfo",&gop_proto, NULL, &size, NULL);if (status == EFI_NOT_FOUND)return;status = efi_call_early(allocate_pool, EFI_LOADER_DATA, size,(void **)&disp_info);if (status != EFI_SUCCESS)return;status = efi_call_runtime(get_variable, L"UEFIDisplayInfo",&gop_proto, NULL, &size, disp_info);if (status != EFI_SUCCESS)goto cleanup;if ((disp_info->version_info >> 16) != MDP_DISP_INFO_VERSION_MAGIC)goto cleanup;efi_printk(sys_table_arg, "found a panel-id!\n");set_chosen_panel_id(fdt_addr, disp_info->panel_id);+cleanup:
efi_call_early(free_pool, disp_info);+}
void install_memreserve_table(efi_system_table_t *sys_table_arg) { struct linux_efi_memreserve *rsv; @@ -229,6 +276,8 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table, if (!fdt_addr) pr_efi(sys_table, "Generating empty DTB\n");
get_panel_id(sys_table, fdt_addr);status = handle_cmdline_files(sys_table, image, cmdline_ptr, "initrd=", efi_get_max_initrd_addr(dram_base, *image_addr),diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h index 1b1dfcaa6fb9..8832cb9a7a40 100644 --- a/drivers/firmware/efi/libstub/efistub.h +++ b/drivers/firmware/efi/libstub/efistub.h @@ -39,6 +39,8 @@ void efi_char16_printk(efi_system_table_t *, efi_char16_t *);
unsigned long get_dram_base(efi_system_table_t *sys_table_arg);
+void set_chosen_panel_id(unsigned long fdt_addr, unsigned panel_id);
efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table, void *handle, unsigned long *new_fdt_addr, diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c index 5440ba17a1c5..cb6ea160a40a 100644 --- a/drivers/firmware/efi/libstub/fdt.c +++ b/drivers/firmware/efi/libstub/fdt.c @@ -200,6 +200,15 @@ static efi_status_t update_fdt_memmap(void *fdt, struct efi_boot_memmap *map) return EFI_SUCCESS; }
+void set_chosen_panel_id(unsigned long fdt_addr, unsigned panel_id) +{
void *fdt = (void *)fdt_addr;int node = fdt_subnode_offset(fdt, 0, "chosen");u32 fdt_val32 = cpu_to_fdt32(panel_id);fdt_setprop_var(fdt, node, "panel-id", fdt_val32);+}
#ifndef EFI_FDT_ALIGN # define EFI_FDT_ALIGN EFI_PAGE_SIZE
#endif
2.20.1
On Tue, 2 Jul 2019 at 22:26, Ard Biesheuvel ard.biesheuvel@linaro.org wrote:
On Sun, 30 Jun 2019 at 22:36, Rob Clark robdclark@gmail.com wrote:
From: Rob Clark robdclark@chromium.org
On snapdragon aarch64 laptops, a 'UEFIDisplayInfo' variable is provided to communicate some information about the display. Crutially it has the panel-id, so the appropriate panel driver can be selected. Read this out and stash in /chosen/panel-id so that display driver can use it to pick the appropriate panel.
Signed-off-by: Rob Clark robdclark@chromium.org
Hi Rob,
I understand why you are doing this, but this *really* belongs elsewhere.
So we are dealing with a platform that violates the UEFI spec, since it does not bother to implement variable services at runtime (because MS let the vendor get away with this).
To clarify, the above remark applies to populating the DT from the OS rather than from the firmware.
First of all, to pass data between the EFI stub and the OS proper, we should use a configuration table rather than a DT property, since the former is ACPI/DT agnostic. Also, I'd like the consumer of the data to actually interpret it, i.e., just dump the whole opaque thing into a config table in the stub, and do the parsing in the OS proper.
However, I am not thrilled at adding code to the stub that unconditionally looks for some variable with some magic name on all ARM/arm64 EFI systems, so this will need to live under a Kconfig option that depends on ARM64 (and does not default to y)
... but saving variables at boot time for consumption at runtime is something that we will likely see more of in the future.
On Tue, Jul 2, 2019 at 1:35 PM Ard Biesheuvel ard.biesheuvel@linaro.org wrote:
On Tue, 2 Jul 2019 at 22:26, Ard Biesheuvel ard.biesheuvel@linaro.org wrote:
On Sun, 30 Jun 2019 at 22:36, Rob Clark robdclark@gmail.com wrote:
From: Rob Clark robdclark@chromium.org
On snapdragon aarch64 laptops, a 'UEFIDisplayInfo' variable is provided to communicate some information about the display. Crutially it has the panel-id, so the appropriate panel driver can be selected. Read this out and stash in /chosen/panel-id so that display driver can use it to pick the appropriate panel.
Signed-off-by: Rob Clark robdclark@chromium.org
Hi Rob,
I understand why you are doing this, but this *really* belongs elsewhere.
So we are dealing with a platform that violates the UEFI spec, since it does not bother to implement variable services at runtime (because MS let the vendor get away with this).
To clarify, the above remark applies to populating the DT from the OS rather than from the firmware.
yeah, it isn't pretty, but there *are* some other similar cases where efi-stub is populating DT.. (like update_fdt_memmap() and kaslr-seed)..
it would be kinda nice to have an early-quirks mechanism where this could fit, but I thought that might be equally unpopular ;-)
First of all, to pass data between the EFI stub and the OS proper, we should use a configuration table rather than a DT property, since the former is ACPI/DT agnostic. Also, I'd like the consumer of the data to actually interpret it, i.e., just dump the whole opaque thing into a config table in the stub, and do the parsing in the OS proper.
However, I am not thrilled at adding code to the stub that unconditionally looks for some variable with some magic name on all ARM/arm64 EFI systems, so this will need to live under a Kconfig option that depends on ARM64 (and does not default to y)
I defn can add this under kconfig.. is it ok if that option is select'd by ARCH_QCOM?
(Just trying to minimize the things that can go wrong and the "I get a blank screen at boot" bug reports I get ;-))
... but saving variables at boot time for consumption at runtime is something that we will likely see more of in the future.
I think this will be nice, but it also doesn't address the need for a quirk to get this into /chosen.. I guess we *could* use a shim or something that runs before the kernel to do this. But that just seems like a logistical/support nightmare. There is one kernel, and there are N distro's, so debugging a users "I don't get a screen at boot" problem because their distro missed some shim patch really just doesn't seem like a headache I want to have.
BR, -R
On Tue, 2 Jul 2019 at 23:02, Rob Clark robdclark@gmail.com wrote:
On Tue, Jul 2, 2019 at 1:35 PM Ard Biesheuvel ard.biesheuvel@linaro.org wrote:
On Tue, 2 Jul 2019 at 22:26, Ard Biesheuvel ard.biesheuvel@linaro.org wrote:
On Sun, 30 Jun 2019 at 22:36, Rob Clark robdclark@gmail.com wrote:
From: Rob Clark robdclark@chromium.org
On snapdragon aarch64 laptops, a 'UEFIDisplayInfo' variable is provided to communicate some information about the display. Crutially it has the panel-id, so the appropriate panel driver can be selected. Read this out and stash in /chosen/panel-id so that display driver can use it to pick the appropriate panel.
Signed-off-by: Rob Clark robdclark@chromium.org
Hi Rob,
I understand why you are doing this, but this *really* belongs elsewhere.
So we are dealing with a platform that violates the UEFI spec, since it does not bother to implement variable services at runtime (because MS let the vendor get away with this).
To clarify, the above remark applies to populating the DT from the OS rather than from the firmware.
yeah, it isn't pretty, but there *are* some other similar cases where efi-stub is populating DT.. (like update_fdt_memmap() and kaslr-seed)..
True, but those don't describe the hardware.
it would be kinda nice to have an early-quirks mechanism where this could fit, but I thought that might be equally unpopular ;-)
Very :-)
First of all, to pass data between the EFI stub and the OS proper, we should use a configuration table rather than a DT property, since the former is ACPI/DT agnostic. Also, I'd like the consumer of the data to actually interpret it, i.e., just dump the whole opaque thing into a config table in the stub, and do the parsing in the OS proper.
However, I am not thrilled at adding code to the stub that unconditionally looks for some variable with some magic name on all ARM/arm64 EFI systems, so this will need to live under a Kconfig option that depends on ARM64 (and does not default to y)
I defn can add this under kconfig.. is it ok if that option is select'd by ARCH_QCOM?
I guess some mobile SOC/snapdragon symbol would be more appropriate, but given that qcom left the server business, I guess it hardly matters, so default y if ARM64 && ARCH_QCOM is fine with me
(Just trying to minimize the things that can go wrong and the "I get a blank screen at boot" bug reports I get ;-))
Sure
... but saving variables at boot time for consumption at runtime is something that we will likely see more of in the future.
I think this will be nice, but it also doesn't address the need for a quirk to get this into /chosen.. I guess we *could* use a shim or something that runs before the kernel to do this. But that just seems like a logistical/support nightmare. There is one kernel, and there are N distro's, so debugging a users "I don't get a screen at boot" problem because their distro missed some shim patch really just doesn't seem like a headache I want to have.
I'd argue that this does not belong in /chosen in the first place, i.e., it doesn't belong in the DT at all if the OS can access the config table (and therefore the variable) directly.
On Tue, Jul 2, 2019 at 2:53 PM Ard Biesheuvel ard.biesheuvel@linaro.org wrote:
On Tue, 2 Jul 2019 at 23:02, Rob Clark robdclark@gmail.com wrote:
On Tue, Jul 2, 2019 at 1:35 PM Ard Biesheuvel ard.biesheuvel@linaro.org wrote:
On Tue, 2 Jul 2019 at 22:26, Ard Biesheuvel ard.biesheuvel@linaro.org wrote:
On Sun, 30 Jun 2019 at 22:36, Rob Clark robdclark@gmail.com wrote:
From: Rob Clark robdclark@chromium.org
On snapdragon aarch64 laptops, a 'UEFIDisplayInfo' variable is provided to communicate some information about the display. Crutially it has the panel-id, so the appropriate panel driver can be selected. Read this out and stash in /chosen/panel-id so that display driver can use it to pick the appropriate panel.
Signed-off-by: Rob Clark robdclark@chromium.org
Hi Rob,
I understand why you are doing this, but this *really* belongs elsewhere.
So we are dealing with a platform that violates the UEFI spec, since it does not bother to implement variable services at runtime (because MS let the vendor get away with this).
To clarify, the above remark applies to populating the DT from the OS rather than from the firmware.
yeah, it isn't pretty, but there *are* some other similar cases where efi-stub is populating DT.. (like update_fdt_memmap() and kaslr-seed)..
True, but those don't describe the hardware.
it would be kinda nice to have an early-quirks mechanism where this could fit, but I thought that might be equally unpopular ;-)
Very :-)
First of all, to pass data between the EFI stub and the OS proper, we should use a configuration table rather than a DT property, since the former is ACPI/DT agnostic. Also, I'd like the consumer of the data to actually interpret it, i.e., just dump the whole opaque thing into a config table in the stub, and do the parsing in the OS proper.
However, I am not thrilled at adding code to the stub that unconditionally looks for some variable with some magic name on all ARM/arm64 EFI systems, so this will need to live under a Kconfig option that depends on ARM64 (and does not default to y)
I defn can add this under kconfig.. is it ok if that option is select'd by ARCH_QCOM?
I guess some mobile SOC/snapdragon symbol would be more appropriate, but given that qcom left the server business, I guess it hardly matters, so default y if ARM64 && ARCH_QCOM is fine with me
(Just trying to minimize the things that can go wrong and the "I get a blank screen at boot" bug reports I get ;-))
Sure
... but saving variables at boot time for consumption at runtime is something that we will likely see more of in the future.
I think this will be nice, but it also doesn't address the need for a quirk to get this into /chosen.. I guess we *could* use a shim or something that runs before the kernel to do this. But that just seems like a logistical/support nightmare. There is one kernel, and there are N distro's, so debugging a users "I don't get a screen at boot" problem because their distro missed some shim patch really just doesn't seem like a headache I want to have.
I'd argue that this does not belong in /chosen in the first place, i.e., it doesn't belong in the DT at all if the OS can access the config table (and therefore the variable) directly.
admittedly I'm trying to solve two different problems here.. we also have the problem of knowing which panel is installed for the "pure DT" case.. where /chosen is (IMO) the right thing (alternatives involve a shim that knows far too much about the device)..
I guess for ACPI boot case, we do anyways want some sort of configuration table. I suppose the drm code could look for both /chosen/panel-id and configuration table and use either. Although it would be nice if somehow the config table info ended up in /chosen when we are not using ACPI, since then at least code paths are more similar to how we want future android devices to solve this without a pile of downstream hacks..
BR, -R
On Tue, Jul 02, 2019 at 02:01:49PM -0700, Rob Clark wrote:
So we are dealing with a platform that violates the UEFI spec, since it does not bother to implement variable services at runtime (because MS let the vendor get away with this).
To clarify, the above remark applies to populating the DT from the OS rather than from the firmware.
yeah, it isn't pretty, but there *are* some other similar cases where efi-stub is populating DT.. (like update_fdt_memmap() and kaslr-seed)..
The problem isn't with the stub updating the DT, the problem is what it updates it with.
update_fdt_memmap() is the stub filling in the information it communicates to the main kernel.
kaslr-seed sets a standard property using a standard interface if that interface is available to it at the point of execution.
Since what we're doing here is dressing up an ACPI platform to make it look like it was a DT platform, and since we have the ability to tweak the DT before ever passing it to the kernel, let's just do that.
Yes, I know I said I'd rather not, but it's way nicer than sticking platform-specific hacks into the EFI stub.
(If adding it as a DT property is indeed the thing to do.)
... but saving variables at boot time for consumption at runtime is something that we will likely see more of in the future.
I think this will be nice, but it also doesn't address the need for a quirk to get this into /chosen.. I guess we *could* use a shim or something that runs before the kernel to do this. But that just seems like a logistical/support nightmare.
There is one kernel, and there are N distro's, so debugging a users "I don't get a screen at boot" problem because their distro missed some shim patch really just doesn't seem like a headache I want to have.
The distros should not need to be aware *at all* of the hacks required to disguise these platforms as DT platforms.
If they do, they're already device-specific installers and have already accepted the logistical/support nightmare.
/ Leif
On Tue, Jul 2, 2019 at 2:59 PM Leif Lindholm leif.lindholm@linaro.org wrote:
On Tue, Jul 02, 2019 at 02:01:49PM -0700, Rob Clark wrote:
So we are dealing with a platform that violates the UEFI spec, since it does not bother to implement variable services at runtime (because MS let the vendor get away with this).
To clarify, the above remark applies to populating the DT from the OS rather than from the firmware.
yeah, it isn't pretty, but there *are* some other similar cases where efi-stub is populating DT.. (like update_fdt_memmap() and kaslr-seed)..
The problem isn't with the stub updating the DT, the problem is what it updates it with.
update_fdt_memmap() is the stub filling in the information it communicates to the main kernel.
kaslr-seed sets a standard property using a standard interface if that interface is available to it at the point of execution.
Since what we're doing here is dressing up an ACPI platform to make it look like it was a DT platform, and since we have the ability to tweak the DT before ever passing it to the kernel, let's just do that.
Yes, I know I said I'd rather not, but it's way nicer than sticking platform-specific hacks into the EFI stub.
(If adding it as a DT property is indeed the thing to do.)
... but saving variables at boot time for consumption at runtime is something that we will likely see more of in the future.
I think this will be nice, but it also doesn't address the need for a quirk to get this into /chosen.. I guess we *could* use a shim or something that runs before the kernel to do this. But that just seems like a logistical/support nightmare.
There is one kernel, and there are N distro's, so debugging a users "I don't get a screen at boot" problem because their distro missed some shim patch really just doesn't seem like a headache I want to have.
The distros should not need to be aware *at all* of the hacks required to disguise these platforms as DT platforms.
If they do, they're already device-specific installers and have already accepted the logistical/support nightmare.
I guess I'm not *against* a DT loader shim populating the panel-id over into /chosen.. I had it in mind as a backup plan. Ofc still need to get dt folks to buy into /chosen/panel-id but for DT boot I think that is the best option. (At least the /chosen/panel-id approach doesn't require the shim to be aware of how the panel is wired up to dsi controller and whether their is a bridge in between, and that short of thing, so the panel-id approach seems more maintainable that other options.)
I am a bit fearful of problems arising from different distros and users using different versions of shim, and how to manage that. I guess if somehow "shim thing" was part of the kernel, there would by one less moving part... I'd know if user had kernel vX.Y.Z they'd be good to go vs not. But *also* depending on a new-enough version of a shim, where the version # is probably not easily apparent to the end user, sounds a bit scary from the "all the things that can go wrong" point of view. Maybe I'm paranoid, but I'm a bit worried about how to manage that.
BR, -R
On Tue, Jul 02, 2019 at 03:48:48PM -0700, Rob Clark wrote:
There is one kernel, and there are N distro's, so debugging a users "I don't get a screen at boot" problem because their distro missed some shim patch really just doesn't seem like a headache I want to have.
The distros should not need to be aware *at all* of the hacks required to disguise these platforms as DT platforms.
If they do, they're already device-specific installers and have already accepted the logistical/support nightmare.
I guess I'm not *against* a DT loader shim populating the panel-id over into /chosen.. I had it in mind as a backup plan. Ofc still need to get dt folks to buy into /chosen/panel-id but for DT boot I think that is the best option. (At least the /chosen/panel-id approach doesn't require the shim to be aware of how the panel is wired up to dsi controller and whether their is a bridge in between, and that short of thing, so the panel-id approach seems more maintainable that other options.)
I am leaning like Ard towards preferring a configuration table though.
That removes the question of no runtime services (needing to manually cache things, at least until EBBR 1.2 (?) is out and in use), and means we don't have to use different paths for DT and ACPI. Now we have UEFI in U-Boot, do we really need to worry about the non-UEFI case?
I am a bit fearful of problems arising from different distros and users using different versions of shim, and how to manage that. I guess if somehow "shim thing" was part of the kernel, there would by one less moving part...
Sure, but that's insurance against bindings changing non-backwards-compatibly - which there are ways to prevent, and which streamlining the design for really isn't the way to discourage...
Distros have no need to worry about the DT loader - the whole point of it is to remove the need for the distro to worry about anything other than getting the required drivers in.
I'd know if user had kernel vX.Y.Z they'd be good to go vs not. But *also* depending on a new-enough version of a shim, where the version # is probably not easily apparent to the end user, sounds a bit scary from the "all the things that can go wrong" point of view. Maybe I'm paranoid, but I'm a bit worried about how to manage that.
Until the hardware abstractions provided by the system firmware (ACPI) is supported, these platforms are not going to be appropriate for end users anyway. No matter how many not-quite-upstream hacks distros include, they won't be able to support the next minor spin that comes off the production line and is no longer compatible with existing DTs.
/ Leif
On Wed, Jul 3, 2019 at 9:33 AM Leif Lindholm leif.lindholm@linaro.org wrote:
On Tue, Jul 02, 2019 at 03:48:48PM -0700, Rob Clark wrote:
There is one kernel, and there are N distro's, so debugging a users "I don't get a screen at boot" problem because their distro missed some shim patch really just doesn't seem like a headache I want to have.
The distros should not need to be aware *at all* of the hacks required to disguise these platforms as DT platforms.
If they do, they're already device-specific installers and have already accepted the logistical/support nightmare.
I guess I'm not *against* a DT loader shim populating the panel-id over into /chosen.. I had it in mind as a backup plan. Ofc still need to get dt folks to buy into /chosen/panel-id but for DT boot I think that is the best option. (At least the /chosen/panel-id approach doesn't require the shim to be aware of how the panel is wired up to dsi controller and whether their is a bridge in between, and that short of thing, so the panel-id approach seems more maintainable that other options.)
I am leaning like Ard towards preferring a configuration table though.
Ok, if you want the DT loader to propagate UEFIDisplayInfo to a config table, I can update the drm parts of my patchset to look for that in addition to /chosen/panel-id
That removes the question of no runtime services (needing to manually cache things, at least until EBBR 1.2 (?) is out and in use), and means we don't have to use different paths for DT and ACPI. Now we have UEFI in U-Boot, do we really need to worry about the non-UEFI case?
I've mixed feelings about requiring UEFI.. I definitely want to give qcom an incentive to turn on GOP and full UEFI boot for future android devices. OTOH there are quite a few devices out there that aren't UEFI boot. But I guess if drm falls back to /chosen/panel-id we are covered.
I am a bit fearful of problems arising from different distros and users using different versions of shim, and how to manage that. I guess if somehow "shim thing" was part of the kernel, there would by one less moving part...
Sure, but that's insurance against bindings changing non-backwards-compatibly - which there are ways to prevent, and which streamlining the design for really isn't the way to discourage...
Distros have no need to worry about the DT loader - the whole point of it is to remove the need for the distro to worry about anything other than getting the required drivers in.
I'm a bit more concerned about DT loader getting into the business of DT fixup.. I guess if we don't do that, it is less of a concern. But if we relied on it to fixup DT for installed panel, we could probably make it work semi-generically on existing devices that have bridge and panel wired up same way. But seems like some of the 835 laptops have bridge hooked up as child of dsi bus instead. And someday we could see devices using dsi directly, etc.
(It would be really nice to see DT loader able to pick the correct .dtb based on smbios tables tho ;-).. but maybe different topic)
I'd know if user had kernel vX.Y.Z they'd be good to go vs not. But *also* depending on a new-enough version of a shim, where the version # is probably not easily apparent to the end user, sounds a bit scary from the "all the things that can go wrong" point of view. Maybe I'm paranoid, but I'm a bit worried about how to manage that.
Until the hardware abstractions provided by the system firmware (ACPI) is supported, these platforms are not going to be appropriate for end users anyway. No matter how many not-quite-upstream hacks distros include, they won't be able to support the next minor spin that comes off the production line and is no longer compatible with existing DTs.
yeah, that will be a problem.. and also switching to older kernel after upgrading when in-flight dt bindings evolve. Having one less moving part would be nice.
Maybe if adding a config table for UEFIDisplayInfo, you could also add one for DT loader version, so (at least if user is able to get far enough to get dmesg) we could see that more easily?
BR, -R
On Wed, 3 Jul 2019 at 19:41, Rob Clark robdclark@gmail.com wrote:
On Wed, Jul 3, 2019 at 9:33 AM Leif Lindholm leif.lindholm@linaro.org wrote:
On Tue, Jul 02, 2019 at 03:48:48PM -0700, Rob Clark wrote:
There is one kernel, and there are N distro's, so debugging a users "I don't get a screen at boot" problem because their distro missed some shim patch really just doesn't seem like a headache I want to have.
The distros should not need to be aware *at all* of the hacks required to disguise these platforms as DT platforms.
If they do, they're already device-specific installers and have already accepted the logistical/support nightmare.
I guess I'm not *against* a DT loader shim populating the panel-id over into /chosen.. I had it in mind as a backup plan. Ofc still need to get dt folks to buy into /chosen/panel-id but for DT boot I think that is the best option. (At least the /chosen/panel-id approach doesn't require the shim to be aware of how the panel is wired up to dsi controller and whether their is a bridge in between, and that short of thing, so the panel-id approach seems more maintainable that other options.)
I am leaning like Ard towards preferring a configuration table though.
Ok, if you want the DT loader to propagate UEFIDisplayInfo to a config table, I can update the drm parts of my patchset to look for that in addition to /chosen/panel-id
That removes the question of no runtime services (needing to manually cache things, at least until EBBR 1.2 (?) is out and in use), and means we don't have to use different paths for DT and ACPI. Now we have UEFI in U-Boot, do we really need to worry about the non-UEFI case?
I've mixed feelings about requiring UEFI.. I definitely want to give qcom an incentive to turn on GOP and full UEFI boot for future android devices. OTOH there are quite a few devices out there that aren't UEFI boot. But I guess if drm falls back to /chosen/panel-id we are covered.
I am a bit fearful of problems arising from different distros and users using different versions of shim, and how to manage that. I guess if somehow "shim thing" was part of the kernel, there would by one less moving part...
Sure, but that's insurance against bindings changing non-backwards-compatibly - which there are ways to prevent, and which streamlining the design for really isn't the way to discourage...
Distros have no need to worry about the DT loader - the whole point of it is to remove the need for the distro to worry about anything other than getting the required drivers in.
I'm a bit more concerned about DT loader getting into the business of DT fixup.. I guess if we don't do that, it is less of a concern. But if we relied on it to fixup DT for installed panel, we could probably make it work semi-generically on existing devices that have bridge and panel wired up same way. But seems like some of the 835 laptops have bridge hooked up as child of dsi bus instead. And someday we could see devices using dsi directly, etc.
(It would be really nice to see DT loader able to pick the correct .dtb based on smbios tables tho ;-).. but maybe different topic)
I think this is the only sane way of doing things: the DT loader, which is tied much more closely to the platform, does whatever it needs to do to infer from UEFI variables and/or ACPI or SMBIOS tables which bundled DT it installs, and whether/how it needs to fix things up. This indeed constitutes a moving part separate from the OS, but this is the only way that scales. Getting DTs for these devices into distros is *not* what we should be doing.
I'd know if user had kernel vX.Y.Z they'd be good to go vs not. But *also* depending on a new-enough version of a shim, where the version # is probably not easily apparent to the end user, sounds a bit scary from the "all the things that can go wrong" point of view. Maybe I'm paranoid, but I'm a bit worried about how to manage that.
Until the hardware abstractions provided by the system firmware (ACPI) is supported, these platforms are not going to be appropriate for end users anyway. No matter how many not-quite-upstream hacks distros include, they won't be able to support the next minor spin that comes off the production line and is no longer compatible with existing DTs.
yeah, that will be a problem.. and also switching to older kernel after upgrading when in-flight dt bindings evolve. Having one less moving part would be nice.
The whole point of the discussion we have been having for years is that for production use cases, we should not be dealing with evolving in-flight DT binding in the first place. If we ship a DT loader with a certain version of the binding and there is a need to change it, we can only do so if we retain support for the old binding as well.
Maybe if adding a config table for UEFIDisplayInfo, you could also add one for DT loader version, so (at least if user is able to get far enough to get dmesg) we could see that more easily?
I'd prefer it if the DT loader were in charge of creating the UEFIDisplayInfo config tables, but given that we'll need to deal with platforms that don't implement runtime variable services, it is something I would be able to live with in the stub.
In summary, working around platform limitations that prevent us from delivering an accurate DT to the OS should preferably be addressed in a platform specific pre-OS component. I haven't had the chance to look at leif's code yet, but from what I understand, we have pretty much what we need with the exception of the panel/gop variable handling, no?
From: Rob Clark robdclark@chromium.org
Finds the panel-id from chosen, so drivers can use this to pick the correct endpoint when looking up panel.
Signed-off-by: Rob Clark robdclark@chromium.org --- drivers/gpu/drm/drm_of.c | 21 +++++++++++++++++++++ include/drm/drm_of.h | 7 +++++++ 2 files changed, 28 insertions(+)
diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c index 43d89dd59c6b..3ba65750048b 100644 --- a/drivers/gpu/drm/drm_of.c +++ b/drivers/gpu/drm/drm_of.c @@ -279,3 +279,24 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, return ret; } EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge); + +/** + * drm_of_find_panel_id - return id of panel from chosen + * + * Returns the panel id, or zero if none specified + */ +int drm_of_find_panel_id(void) +{ + struct device_node *np = NULL; + u32 panel_id; + + np = of_find_node_by_path("/chosen"); + if (!np) + return 0; + + if (of_property_read_u32(np, "panel-id", &panel_id)) + return 0; + + return panel_id; +} +EXPORT_SYMBOL_GPL(drm_of_find_panel_id); diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h index ead34ab5ca4e..6cd2d59cb1db 100644 --- a/include/drm/drm_of.h +++ b/include/drm/drm_of.h @@ -35,6 +35,8 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, int port, int endpoint, struct drm_panel **panel, struct drm_bridge **bridge); +int drm_of_find_panel_id(void); + #else static inline uint32_t drm_of_crtc_port_mask(struct drm_device *dev, struct device_node *port) @@ -77,6 +79,11 @@ static inline int drm_of_find_panel_or_bridge(const struct device_node *np, { return -EINVAL; } + +static inline int drm_of_find_panel_id(void) +{ + return 0; +} #endif
/*
From: Rob Clark robdclark@chromium.org
Use the drm_of_find_panel_id() helper to decide which endpoint to use when looking up panel. This way we can support devices that have multiple possible panels, such as the aarch64 laptops.
Signed-off-by: Rob Clark robdclark@chromium.org --- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 2719d9c0864b..56c66a43f1a6 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -790,7 +790,7 @@ static int ti_sn_bridge_probe(struct i2c_client *client, const struct i2c_device_id *id) { struct ti_sn_bridge *pdata; - int ret; + int ret, panel_id;
if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { DRM_ERROR("device doesn't support I2C\n"); @@ -811,7 +811,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
pdata->dev = &client->dev;
- ret = drm_of_find_panel_or_bridge(pdata->dev->of_node, 1, 0, + panel_id = drm_of_find_panel_id(); + ret = drm_of_find_panel_or_bridge(pdata->dev->of_node, 1, panel_id, &pdata->panel, NULL); if (ret) { DRM_ERROR("could not find any panel node\n");
Hi Rob,
Thank you for the patch.
On Sun, Jun 30, 2019 at 01:36:08PM -0700, Rob Clark wrote:
From: Rob Clark robdclark@chromium.org
Use the drm_of_find_panel_id() helper to decide which endpoint to use when looking up panel. This way we can support devices that have multiple possible panels, such as the aarch64 laptops.
Signed-off-by: Rob Clark robdclark@chromium.org
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 2719d9c0864b..56c66a43f1a6 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -790,7 +790,7 @@ static int ti_sn_bridge_probe(struct i2c_client *client, const struct i2c_device_id *id) { struct ti_sn_bridge *pdata;
- int ret;
- int ret, panel_id;
if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { DRM_ERROR("device doesn't support I2C\n"); @@ -811,7 +811,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client, pdata->dev = &client->dev;
- ret = drm_of_find_panel_or_bridge(pdata->dev->of_node, 1, 0,
- panel_id = drm_of_find_panel_id();
- ret = drm_of_find_panel_or_bridge(pdata->dev->of_node, 1, panel_id, &pdata->panel, NULL); if (ret) { DRM_ERROR("could not find any panel node\n");
No, I'm sorry, but that's a no-go. We can't patch every single bridge driver to support this hack. We need a solution implemented at another level that will not spread throughout the whole subsystem.
On Sun, Jun 30, 2019 at 2:17 PM Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Rob,
Thank you for the patch.
On Sun, Jun 30, 2019 at 01:36:08PM -0700, Rob Clark wrote:
From: Rob Clark robdclark@chromium.org
Use the drm_of_find_panel_id() helper to decide which endpoint to use when looking up panel. This way we can support devices that have multiple possible panels, such as the aarch64 laptops.
Signed-off-by: Rob Clark robdclark@chromium.org
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 2719d9c0864b..56c66a43f1a6 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -790,7 +790,7 @@ static int ti_sn_bridge_probe(struct i2c_client *client, const struct i2c_device_id *id) { struct ti_sn_bridge *pdata;
int ret;
int ret, panel_id; if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { DRM_ERROR("device doesn't support I2C\n");@@ -811,7 +811,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
pdata->dev = &client->dev;
ret = drm_of_find_panel_or_bridge(pdata->dev->of_node, 1, 0,
panel_id = drm_of_find_panel_id();ret = drm_of_find_panel_or_bridge(pdata->dev->of_node, 1, panel_id, &pdata->panel, NULL); if (ret) { DRM_ERROR("could not find any panel node\n");No, I'm sorry, but that's a no-go. We can't patch every single bridge driver to support this hack. We need a solution implemented at another level that will not spread throughout the whole subsystem.
it could be possible to make a better helper.. but really there aren't *that* many bridge drivers
suggestions ofc welcome, but I think one way or another we are going to need to patch bridges by the time we get to adding ACPI support, so really trivial couple line patches to the handful of bridges we have isn't really something that worries me
BR, -R
Hi Rob,
On Sun, Jun 30, 2019 at 02:50:59PM -0700, Rob Clark wrote:
On Sun, Jun 30, 2019 at 2:17 PM Laurent Pinchart wrote:
On Sun, Jun 30, 2019 at 01:36:08PM -0700, Rob Clark wrote:
From: Rob Clark robdclark@chromium.org
Use the drm_of_find_panel_id() helper to decide which endpoint to use when looking up panel. This way we can support devices that have multiple possible panels, such as the aarch64 laptops.
Signed-off-by: Rob Clark robdclark@chromium.org
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 2719d9c0864b..56c66a43f1a6 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -790,7 +790,7 @@ static int ti_sn_bridge_probe(struct i2c_client *client, const struct i2c_device_id *id) { struct ti_sn_bridge *pdata;
int ret;
int ret, panel_id; if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { DRM_ERROR("device doesn't support I2C\n");@@ -811,7 +811,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
pdata->dev = &client->dev;
ret = drm_of_find_panel_or_bridge(pdata->dev->of_node, 1, 0,
panel_id = drm_of_find_panel_id();ret = drm_of_find_panel_or_bridge(pdata->dev->of_node, 1, panel_id, &pdata->panel, NULL); if (ret) { DRM_ERROR("could not find any panel node\n");No, I'm sorry, but that's a no-go. We can't patch every single bridge driver to support this hack. We need a solution implemented at another level that will not spread throughout the whole subsystem.
it could be possible to make a better helper.. but really there aren't *that* many bridge drivers
suggestions ofc welcome, but I think one way or another we are going to need to patch bridges by the time we get to adding ACPI support, so really trivial couple line patches to the handful of bridges we have isn't really something that worries me
It's only one right now as that's the only one you care about, but before we'll have time to blink, it will be another one, and another one, ... Sorry, that's a no-go for me.
On Sun, Jun 30, 2019 at 2:58 PM Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Rob,
On Sun, Jun 30, 2019 at 02:50:59PM -0700, Rob Clark wrote:
On Sun, Jun 30, 2019 at 2:17 PM Laurent Pinchart wrote:
On Sun, Jun 30, 2019 at 01:36:08PM -0700, Rob Clark wrote:
From: Rob Clark robdclark@chromium.org
Use the drm_of_find_panel_id() helper to decide which endpoint to use when looking up panel. This way we can support devices that have multiple possible panels, such as the aarch64 laptops.
Signed-off-by: Rob Clark robdclark@chromium.org
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 2719d9c0864b..56c66a43f1a6 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -790,7 +790,7 @@ static int ti_sn_bridge_probe(struct i2c_client *client, const struct i2c_device_id *id) { struct ti_sn_bridge *pdata;
int ret;
int ret, panel_id; if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { DRM_ERROR("device doesn't support I2C\n");@@ -811,7 +811,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
pdata->dev = &client->dev;
ret = drm_of_find_panel_or_bridge(pdata->dev->of_node, 1, 0,
panel_id = drm_of_find_panel_id();ret = drm_of_find_panel_or_bridge(pdata->dev->of_node, 1, panel_id, &pdata->panel, NULL); if (ret) { DRM_ERROR("could not find any panel node\n");No, I'm sorry, but that's a no-go. We can't patch every single bridge driver to support this hack. We need a solution implemented at another level that will not spread throughout the whole subsystem.
it could be possible to make a better helper.. but really there aren't *that* many bridge drivers
suggestions ofc welcome, but I think one way or another we are going to need to patch bridges by the time we get to adding ACPI support, so really trivial couple line patches to the handful of bridges we have isn't really something that worries me
It's only one right now as that's the only one you care about, but before we'll have time to blink, it will be another one, and another one, ... Sorry, that's a no-go for me.
I could ofc add helper call to all the existing bridges.. that seemed a bit overkill for v1 patchset
BR, -R
Hi Rob,
Thank you for the patch.
On Sun, Jun 30, 2019 at 01:36:04PM -0700, Rob Clark 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.)
I have to ask the obvious question: why doesn't the boot loader just pass a correct DT to Linux ? There's no point in passing a list of panels that are not there, this seems quite a big hack to me. A proper boot loader should construct the DT based on hardware detection.
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>; }; }; };} };
Note that the panel-id is potentially a sparse-int. The values I've seen so far on aarch64 laptops are:
- 0xc2
- 0xc3
- 0xc4
- 0xc5
- 0x8011
- 0x8012
- 0x8055
- 0x8056
At least on snapdragon aarch64 laptops, they can be any u32 value.
However, on these laptops, the bootloader/firmware is not populating the chosen node, but instead providing an "UEFIDisplayInfo" variable, which contains the panel id. Unfortunately EFI variables are only available before ExitBootServices, so the second patch checks for this variable before EBS and populates the /chosen/panel-id variable.
[1] https://patchwork.freedesktop.org/series/63001/
Rob Clark (4): dt-bindings: chosen: document panel-id binding efi/libstub: detect panel-id drm: add helper to lookup panel-id drm/bridge: ti-sn65dsi86: use helper to lookup panel-id
Documentation/devicetree/bindings/chosen.txt | 69 ++++++++++++++++++++ drivers/firmware/efi/libstub/arm-stub.c | 49 ++++++++++++++ drivers/firmware/efi/libstub/efistub.h | 2 + drivers/firmware/efi/libstub/fdt.c | 9 +++ drivers/gpu/drm/bridge/ti-sn65dsi86.c | 5 +- drivers/gpu/drm/drm_of.c | 21 ++++++ include/drm/drm_of.h | 7 ++ 7 files changed, 160 insertions(+), 2 deletions(-)
On Sun, Jun 30, 2019 at 1:47 PM Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Rob,
Thank you for the patch.
On Sun, Jun 30, 2019 at 01:36:04PM -0700, Rob Clark 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.)
I have to ask the obvious question: why doesn't the boot loader just pass a correct DT to Linux ? There's no point in passing a list of panels that are not there, this seems quite a big hack to me. A proper boot loader should construct the DT based on hardware detection.
Hi Laurent,
Actually the bootloader on these devices is passing *no* dt (they boot ACPI, we are loading dtb from grub currently)
I think normally a device built w/ dt in mind would populate /chosen/panel-id directly (rather than the way it is currently populated based on reading an efi variable prior to ExitBootServices). But that is considerably easier ask than having it re-write of_graph bindings. Either way, we aren't in control of the bootloader on these devices, so it is a matter of coming up with something that works on actual hw that we don't like rather than idealized hw that we don't have ;-)
BR, -R
Hi Rob,
On Sun, Jun 30, 2019 at 02:05:21PM -0700, Rob Clark wrote:
On Sun, Jun 30, 2019 at 1:47 PM Laurent Pinchart wrote:
On Sun, Jun 30, 2019 at 01:36:04PM -0700, Rob Clark 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.)
I have to ask the obvious question: why doesn't the boot loader just pass a correct DT to Linux ? There's no point in passing a list of panels that are not there, this seems quite a big hack to me. A proper boot loader should construct the DT based on hardware detection.
Hi Laurent,
Actually the bootloader on these devices is passing *no* dt (they boot ACPI, we are loading dtb from grub currently)
Ah, the broken promises of ACPI on ARM64. I wonder how long it will take before a public acknowledgement that it was a bad idea. Bad ideas happen and can be forgiven, but stubborness in claiming it was the right decision is another story.
(Not that you can be blamed for this of course :-))
I think normally a device built w/ dt in mind would populate /chosen/panel-id directly (rather than the way it is currently populated based on reading an efi variable prior to ExitBootServices). But that is considerably easier ask than having it re-write of_graph bindings. Either way, we aren't in control of the bootloader on these devices,
If you can't control the initial boot loader, then I see two options, none of which you will like I'm afraid.
- As you pass the DT to Linux from grub, there's your intermediate boot loader where you can construct a valid DT.
- If the ACPI cult needs to be venerated, then drivers should be converted to support ACPI without the need for DT.
A possible a middleground could be a platform driver (in drivers/firmware/efi/ ? in drivers/platform/ ?) that will patch the DT to instantiate the right panel based on the information retrieved from the boot loader. We will need something similar for the Intel IPU3 camera driver, as Intel decided to come up with two different ACPI "bindings", one for Windows and one for Chrome OS, leaving Windows machine impossible to handle from a kernel driver due to required information being hardcoded in Windows drivers shipped by Intel. This is thus an option that may (unfortunately) need to become more widespread for ACPI-based systems.
so it is a matter of coming up with something that works on actual hw that we don't like rather than idealized hw that we don't have ;-)
That doesn't however justify not going for the best solution we can achieve. What do you like best in the above ? :-)
On Sun, Jun 30, 2019 at 2:15 PM Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Rob,
On Sun, Jun 30, 2019 at 02:05:21PM -0700, Rob Clark wrote:
On Sun, Jun 30, 2019 at 1:47 PM Laurent Pinchart wrote:
On Sun, Jun 30, 2019 at 01:36:04PM -0700, Rob Clark 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.)
I have to ask the obvious question: why doesn't the boot loader just pass a correct DT to Linux ? There's no point in passing a list of panels that are not there, this seems quite a big hack to me. A proper boot loader should construct the DT based on hardware detection.
Hi Laurent,
Actually the bootloader on these devices is passing *no* dt (they boot ACPI, we are loading dtb from grub currently)
Ah, the broken promises of ACPI on ARM64. I wonder how long it will take before a public acknowledgement that it was a bad idea. Bad ideas happen and can be forgiven, but stubborness in claiming it was the right decision is another story.
(Not that you can be blamed for this of course :-))
To be fair, I think the only blame here is that MS let qcom get away with some things in their ACPI and UEFI implementation.. I think we'll need to shift to ACPI eventually for these laptops, in order to keep up. DT isn't a thing that would scale with the volume of x86 laptops that exist, and if aarch64 laptops get there too, we'll need ACPI. Lets face it, the # of different dt devices supported upstream is a drop in the bucket compared to number of *actually physically different* x86 devices supported by upstream. (And I don't mean individual models of laptops, but different production runs where they picked a different panel or trackpad or whatever.)
But we have a lot of upstream work to get there to support how ACPI works on these things:
* The new Platform Extension Plugin (PEP) model for device power control * untangling drm bridge hookup from DT * untangling drm panel hook from DT * figuring out how to deal with mis-matches between dt device model and ACPI device model
There is some early work for ACPI support for these devices, but realistically I think it is going to take a better part of a year to get there. Until then we rely on DT.
That isn't to say my proposal doesn't make a ton of sense. We also need to solve this problem for DT based devices, and I think /chosen/panel-id makes a *ton* of sense for those devices.
I think normally a device built w/ dt in mind would populate /chosen/panel-id directly (rather than the way it is currently populated based on reading an efi variable prior to ExitBootServices). But that is considerably easier ask than having it re-write of_graph bindings. Either way, we aren't in control of the bootloader on these devices,
If you can't control the initial boot loader, then I see two options, none of which you will like I'm afraid.
- As you pass the DT to Linux from grub, there's your intermediate boot loader where you can construct a valid DT.
not really a solution that is going to scale
- If the ACPI cult needs to be venerated, then drivers should be converted to support ACPI without the need for DT.
we're working on it
A possible a middleground could be a platform driver (in drivers/firmware/efi/ ? in drivers/platform/ ?) that will patch the DT to instantiate the right panel based on the information retrieved from the boot loader. We will need something similar for the Intel IPU3 camera driver, as Intel decided to come up with two different ACPI "bindings", one for Windows and one for Chrome OS, leaving Windows machine impossible to handle from a kernel driver due to required information being hardcoded in Windows drivers shipped by Intel. This is thus an option that may (unfortunately) need to become more widespread for ACPI-based systems.
again, a kernel (or bootloader) side massively intrusive re-write the dt approach isn't going to scale. If you keep it simple, ie. /chosen/panel-id I can see a possibility to move my patch from drivers/firmware/efi into an earlier stage. But if it has to re-write graph, that falls apart as soon as a new device comes along with a different bridge, or perhaps some vendor decides to use dsi directly and forego the bridge.
usually (from what I've seen so far) there are a few gpios to probe to decide which panel you have. So after a few lines of gpio banging you can either ask fw engineers to set appropriate node in chosen.. or re-write of_graph bindings. I think the former has a chance of gaining traction on android devices.. latter not so much. You are really making too big of an ask for fw engineers ;-)
so it is a matter of coming up with something that works on actual hw that we don't like rather than idealized hw that we don't have ;-)
That doesn't however justify not going for the best solution we can achieve. What do you like best in the above ? :-)
I want a solution that is achievable ;-)
BR, -R
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
aarch64-laptops@lists.linaro.org
-
Ard Biesheuvel -
Jeffrey Hugo -
Laurent Pinchart -
Leif Lindholm -
Rob Clark -
Rob Herring