| Message ID | 20190630203614.5290-1-robdclark@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm+dt+efi: support devices with multiple possible panels | expand |
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
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(-)