| Message ID | 20190630203614.5290-3-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | drm+dt+efi: support devices with multiple possible panels | expand |
On Sun, 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 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: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 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?
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