Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WiP: FB_EFI (EFIFB kernel module's framebuffer on top of libgfxinit or GOP) next steps related cleanups #1522

Merged
merged 12 commits into from
Dec 12, 2023

Conversation

tlaurion
Copy link
Collaborator

@tlaurion tlaurion commented Nov 8, 2023

As it was discussed under #1403 #1250 #1461, this PR:

  • Changes KERNEL_ADD, KERNEL_REMOVE board statements which tampers with OS grub config options.
    • Removes intel_iommu=igfx_off statement (CONFIG_BOOT_KERNEL_REMOVE="intel_iommu=on intel_iommu=igfx_off) that would be pushed by QubesOS since now unneeded for intel_igfx (passed to efifb and no workaround are needed as of now. But keep in mind that console appears there when i915drm kicks in on dom0. It is unknown what is missing there so that dom0 kernel can handle efifb passed by kexec call, but console appears under 6 seconds under x230 so that's good enough)
  • removes coreboot to Heads kernel command line CONFIG_LINUX_COMMAND_LINE="intel_iommu=igfx_off" which is also not needed anymore
  • Turn on CONFIG_INTEL_IOMMU_DEFAULT_ON=y in linux kernel configs that didn't have it (x230)
  • Remove intel related stuff (me, txt, iommu) under kgpe-d16 since I witness they were there
  • Note: qemu still have them so someone wanting to test i440fx instead of q35 can from same compiled kernel (in theory)
  • Adapts nv41/ns50 from https://github.com/Nitrokey/heads/releases/tag/v2.3 fixing [nitropad-nx] igfx not properly working, reports llvm-pipe Nitrokey/heads#25
    ns50 v2.3 boots to blank screen Nitrokey/heads#28
    with proper branding (Heads here by default from Makefile and no board override)

Tested on x230-maximized:

  • Heads enables IOMMU
    signal-2023-11-08-131404_002
  • Qubes's Xen is happy
    signal-2023-11-08-131404_003
  • Qubes dom0 kernel says its suboptimal
    signal-2023-11-08-131404_004

@marmarek if you have any pointers or info on the last screenshot? Booting into installer from Heads after kexec call still takes a really long while though, like 20 seconds after kernel is started following plymouth first output on installer console.

@marmarek
Copy link
Contributor

marmarek commented Nov 8, 2023

@marmarek if you have any pointers or info on the last screenshot?

This is expected. Huge pages are not supported at all in Xen dom0, regardless of settings.

@tlaurion tlaurion mentioned this pull request Nov 8, 2023
12 tasks
@tlaurion tlaurion force-pushed the efifb_next_step_related_cleanups branch from 78e6b5a to 1c54d44 Compare November 9, 2023 16:16
@@ -1989,6 +2010,7 @@ CONFIG_FB_DEFERRED_IO=y
# CONFIG_FB_IMSTT is not set
# CONFIG_FB_VGA16 is not set
CONFIG_FB_VESA=y
# CONFIG_FB_EFI is not set
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something is wrong here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed under 3a648df

@@ -2793,6 +2815,7 @@ CONFIG_ARCH_WANT_HUGETLB_PAGE_OPTIMIZE_VMEMMAP=y
CONFIG_MEMFD_CREATE=y
CONFIG_ARCH_HAS_GIGANTIC_PAGE=y
# CONFIG_CONFIGFS_FS is not set
CONFIG_EFIVAR_FS=m
Copy link
Collaborator Author

@tlaurion tlaurion Nov 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not needed. Nothing compiled as modules is considered but on-demand kernel modules loaded by scripts.
Will pass configs to oldefconfig and advise @daringer @JonathonHall-Purism to compare with x230-maximized which turned off most of the modules, but for an older kernel version. Otherwise compile time for kernel is way longer then needed and for absolutely no outcome: kernel drivers compiled as modules are not packed under modules.cpio unless explicitly defined under modules/linux to be packed under initrd.

@tlaurion tlaurion changed the title WiP: FB_EFI next step related cleanups WiP: FB_EFI (EFIB) next steps related cleanups Nov 10, 2023
@tlaurion tlaurion changed the title WiP: FB_EFI (EFIB) next steps related cleanups WiP: FB_EFI (EFIFB kernel module's framebuffer on top of libgfxinit or GOP) next steps related cleanups Nov 10, 2023
@tlaurion
Copy link
Collaborator Author

tlaurion commented Nov 10, 2023

And here we go again folks with unbricking capabilities (external flashing recovery). Internal flashing as normal, but if needs be, you might have to flash back master top and bottom roms if needed externally.

I want to bring awareness that dGPU testing is badly needed, where again main screen should be iGPU on boot and should not be impacted by present changes, but who knows. Roms as usual can be downloaded per this PR checks and the following instructions apply here as on master. Just click:
2023-11-10-133114
and follow https://osresearch.net/Downloading


Testing needed for unticketed platforms below

@pcm720
Copy link

pcm720 commented Nov 10, 2023

Works perfectly, no issues with GUI/console scaling.

@d-wid
Copy link
Contributor

d-wid commented Nov 10, 2023 via email

@tlaurion
Copy link
Collaborator Author

tlaurion commented Nov 10, 2023

https://output.circle-artifacts.com/output/job/82dfe1cb-ad9d-413c-a841-6b41e48b2e69/artifacts/0/build/x86/z220-cmt-hotp-maximized/heads-z220-cmt-hotp-maximized-v0.2.0-1841-gd110a64.rom on the HP Z220 works. No progress bar when reflashing (successfully) back to the rom I'd been using though. @tlaurion

@d-wid this is odd. Last changes on flash.sh are from a long time ago. Can you post the version string you use on your current rom? Screenshot of system info would be good. Maybe output of "flashrom -p internal" as well?

@d-wid
Copy link
Contributor

d-wid commented Nov 10, 2023 via email

@tlaurion
Copy link
Collaborator Author

@d-wid attachement didn't make it through no. Can you get the last part of the rom name after -gxxxxxxxx that would be the commit of your current rom.

@d-wid
Copy link
Contributor

d-wid commented Nov 10, 2023

IMG_20231110
IMG_20231110

Can you get the last part of the rom name after -gxxxxxxxx

No can do. Probably because I still built this from a fork I made in order to make the #1399 PR. I guess I'll just try building the current version from master locally and see if I get issues trying to flash that instead.

@d-wid
Copy link
Contributor

d-wid commented Nov 11, 2023

https://output.circle-artifacts.com/output/job/82dfe1cb-ad9d-413c-a841-6b41e48b2e69/artifacts/0/build/x86/z220-cmt-hotp-maximized/heads-z220-cmt-hotp-maximized-v0.2.0-1841-gd110a64.rom on the HP Z220 works. No progress bar when reflashing (successfully) back to the rom I'd been using though. @tlaurion

unfortunately I've just noticed something:

  1. The image doesn't actually get scaled up to my display's size (the gap is close enough that I didn't notice without trying to use a new bootsplash I made for myself that should run up against (two of) the borders, but doesn't. Unfortunately this might've already been the case when the change from how libgfxinit was previously used was first tested a few months ago, since it would've been just as hard for me to notice if the image was too small.
  2. Images with non-greyscale colours look wonky on my machine. I think Red and blue got swapped, or something like that
    Untitled

I want to bring awareness that dGPU testing is badly needed, where again main screen should be iGPU on boot and should not be impacted by present changes

No issues as far as that is concerned, unless Heads has magically gained the ability to use an R9 Fury.

@tlaurion
Copy link
Collaborator Author

tlaurion commented Nov 11, 2023

https://output.circle-artifacts.com/output/job/82dfe1cb-ad9d-413c-a841-6b41e48b2e69/artifacts/0/build/x86/z220-cmt-hotp-maximized/heads-z220-cmt-hotp-maximized-v0.2.0-1841-gd110a64.rom on the HP Z220 works. No progress bar when reflashing (successfully) back to the rom I'd been using though. @tlaurion

unfortunately I've just noticed something:

  1. The image doesn't actually get scaled up to my display's size (the gap is close enough that I didn't notice without trying to use a new bootsplash I made for myself that should run up against (two of) the borders, but doesn't. Unfortunately this might've already been the case when the change from how libgfxinit was previously used was first tested a few months ago, since it would've been just as hard for me to notice if the image was too small.
  2. Images with non-greyscale colours look wonky on my machine. I think Red and blue got swapped, or something like that

Resize is not setuped in coreboot config. Could be though. For color inversion that is weird since swap is also not defined. You can play with those setting around

# CONFIG_BOOTSPLASH_CONVERT_COLORSWAP is not set

Untitled

I want to bring awareness that dGPU testing is badly needed, where again main screen should be iGPU on boot and should not be impacted by present changes

No issues as far as that is concerned, unless Heads has magically gained the ability to use an R9 Fury.

Sorry I didn't mean dGPU configs in general, I was talking about dGPU board configs :)
That is specific to boards having dgpu in their names, which are now limited to laptops.

@JonathonHall-Purism
Copy link
Collaborator

Reviewed change to config/linux-librem_common-6.1.8.config, only change is conversion to oldconfig, compared with master. LGTM 👍

@tlaurion
Copy link
Collaborator Author

tlaurion commented Nov 14, 2023

@d-wid following my previous comments, those patches under 4.19 patch directory have been reworked and merged upstream on newer versions of coreboot, where the pointed 4.19 coreboot configuration can be changed to resize, swap colors and/or center bootsplash. As of now, Heads points to a 1024x768 jpeg and is asked by config to center, not resize and keep 70% of its original quality doing so.

This is best I could do without having the hardware in question, but if tested esthetics is better with board specific tweaks, PR welcome!

@srgrint
Copy link
Contributor

srgrint commented Nov 14, 2023

Have tested heads-t440p-maximized-v0.2.0-1841-gd110a64.rom on my t440p. Seems to work fine.

@srgrint
Copy link
Contributor

srgrint commented Nov 18, 2023

Have tested heads-x220-maximized-v0.2.0-1841-gd110a64.rom. No obvious problems.

@gaspar-ilom
Copy link
Contributor

I tested the W541 hotp-maximized and did not encounter any obvious issues.

The only thing I noticed is that the hashes.txt contains no hash for the 12M rom but the the hash for the bottom rom shows up twice. The same happens also for other boards like the t440p. I did not analyze this, just something I found worth mentioning.

…iommu=igfx_off, add CONFIG_BOOT_KERNEL_REMOVE=intel_iommu=on intel_iommu=igfx_off, remove quiet removal from CONFIG_BOOT_KERNEL_REMOVE. TLDR: do not interfere with OS setting its own boot policies

Signed-off-by: Thierry Laurion <insurgo@riseup.net>
…s. TODO: check linux config to see if enabling automatically works as expected.

Signed-off-by: Thierry Laurion <insurgo@riseup.net>
…all intel boards touched in past commit

Touches c216, x230-flash, x230-legacy and x230-maximized.
TODO: Other boards, including AMD ones (qemu/kgpe) have this ON, including nv41/ns50 (which uses i915drm which most probably causes problems)
 Note that qemu boards use q35 in config, but were made to have both i440fx and q35, where q35 is tested, which explains why its on by default there.

Signed-off-by: Thierry Laurion <insurgo@riseup.net>
@srgrint
Copy link
Contributor

srgrint commented Nov 22, 2023

Tested t430-maximized/heads-t430-maximized-v0.2.0-1924-ga7fe284.rom on my intel graphics t430. Seems to work fine.

@tlaurion tlaurion marked this pull request as draft November 28, 2023 03:34
@tlaurion tlaurion marked this pull request as ready for review November 28, 2023 03:34
@tlaurion
Copy link
Collaborator Author

tlaurion commented Dec 4, 2023

So as of today:

Testing needed for unticketed platforms below

This means from current list of untested boards that

  • ASUS P8Z77 M PRO
  • t420
  • t530
  • z220
  • w530

needs to be moved from tested to untested until reported tested.

I try to move board to untested from PR associated with which PR a board started to be untested by currently associated board owners.

So @ThePlexus @3hhh@alexmaloteaux @natterangell (iGPU) @akfhasodh @doob85 @d-wid :

Last call to test this PR and report result prior of the boards to continue to produce tested roms for others to flash with confidence. This means those boards are not properly tested by daily owners of boards and should eventually be deprecated. We cannot wait forever for PR to be tested which stops merging for other boards.

Unless people are willing to donate boards or board owners are responsive, I cannot leave untested boards rom be thought tested while they aren't.

Nex commit in this PR is to prefix aforementioned boards with UNTESTED in board directory, alongside all other currently untested board configs.

@natterangell
Copy link
Contributor

I can test t420 this coming weekend, will report back.

@d-wid
Copy link
Contributor

d-wid commented Dec 8, 2023 via email

@tlaurion
Copy link
Collaborator Author

tlaurion commented Dec 8, 2023

@d-wid @natterangell are you aware of other board owners with external programmers?

I feel like I need to seperate boards and configs again so that each boards can be tested independently and PR like this don't stay open for months. Not sure how to handle this properly to reduce delay between PR creation and merge time.

@d-wid
Copy link
Contributor

d-wid commented Dec 9, 2023 via email

@tlaurion
Copy link
Collaborator Author

tlaurion commented Dec 11, 2023

@tlaurion I'm not, though I'd invite anyone reading this who's looking to get a Tower PC to consider one if there's a used example for cheap nearby😄 The newest build from this PR also works on my machine, but is this a repeat of #1522 (comment) or has something changed again that warranted testing?

On 8 December 2023 13:54:36 UTC, tlaurion @.> wrote: @d-wid are you aware of other board owners with external programmers? -- Reply to this email directly or view it on GitHub: #1522 (comment) You are receiving this because you were mentioned. Message ID: @.>

Sorry about that let me check real quick. So you said having tested
https://output.circle-artifacts.com/output/job/82dfe1cb-ad9d-413c-a841-6b41e48b2e69/artifacts/0/build/x86/z220-cmt-hotp-maximized/heads-z220-cmt-hotp-maximized-v0.2.0-1841-gd110a64.rom which was commit d110a64 where latest commit of this pr is a7fe284

Let's build a compare url:
d110a64...a7fe284

And see if z220-cmt linux/coreboot/board config changed:

  • board config changed removing export CONFIG_BOOT_KERNEL_ADD="intel_iommu=igfx_off" and export CONFIG_BOOT_KERNEL_REMOVE="quiet" and adding export CONFIG_BOOT_KERNEL_REMOVE="intel_iommu=on intel_iommu=igfx_off" which permits to QubesOS to deal properly with initel_iommu without hacks that were needed because double usage of i915 driver in both Heads and final kexec'ed OS. (It changes grub options before calling kexec)
  • coreboot's config linux command line changed to remove intel_iommu=igfx_off which was required to hack around i915+drm limitation when used twice under heads and final OS
  • linux config changed to enable intel iommu by default in kernel

@d-wid so I consider this board tested with a7fe284! Thank you!

@tlaurion
Copy link
Collaborator Author

tlaurion commented Dec 11, 2023

This means from current list of untested boards that

  • ASUS P8Z77 M PRO
  • t420
  • t530
  • w530

ASUS P8Z77 M PRO (Ivy bridge): @ThePlexus
t420 (xx20): @alexmaloteaux @natterangell (iGPU) @akfhasodh @doob85
t530 (xx30): @3hhh
w530 (xx30): @eganonoa @zifxify @weyounsix (dGPU: w530-k2000m) @jnscmns (dGPU K1000M) @computer-user123 (w530 / & w530 k2000 : prefers iGPU)

Note that w530 was already marked as UNTESTED. Please look for board config and click on blame to see what changed that needs to be tested prior of asking for that board to be moved back as other tested boards.

Next commit will move those boards to UNTESTED_boards. This needs to be merged and Heads to move on. Sorry folks.

tlaurion added a commit to tlaurion/heads that referenced this pull request Dec 11, 2023
…inuxboot#1522 (comment). Note that w530 was already marked as UNTESTED, look for commit having moved this board as untested.

Signed-off-by: Thierry Laurion <insurgo@riseup.net>
@tlaurion
Copy link
Collaborator Author

@JonathonHall-Purism Would need an approval for merge here once CI build passes :)

…inuxboot#1522 (comment). Note that w530 was already marked as UNTESTED, look for commit having moved this board as untested.

Signed-off-by: Thierry Laurion <insurgo@riseup.net>
@tlaurion tlaurion force-pushed the efifb_next_step_related_cleanups branch from eb9d7e2 to 0dbbae5 Compare December 11, 2023 21:07
Copy link
Collaborator

@JonathonHall-Purism JonathonHall-Purism left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me:

  • No Librem changes to review
  • Qemu changes look fine
  • coreboot patches for nitrokey tree look OK, appears to be the same patches we have on 4.19 IIRC
  • Skimmed the other boards and don't have any objections

tlaurion added a commit to tlaurion/heads that referenced this pull request Dec 12, 2023
…=igfx_off equally as for NS50 under 2fcef4a and tested for NS50 at linuxboot#1522 (comment)

Signed-off-by: Thierry Laurion <insurgo@riseup.net>
…=igfx_off equally as for NS50 under 2fcef4a and tested for NS50 at linuxboot#1522 (comment)

Signed-off-by: Thierry Laurion <insurgo@riseup.net>
@tlaurion tlaurion force-pushed the efifb_next_step_related_cleanups branch from 7aefa97 to e0fabb1 Compare December 12, 2023 16:34
@tlaurion tlaurion mentioned this pull request Dec 12, 2023
49 tasks
@tlaurion
Copy link
Collaborator Author

tlaurion commented Dec 12, 2023

@daringer @nestire @jans23

Should fix:

Should attenuate:

Reminder:

@natterangell
Copy link
Contributor

@tlaurion, I apologize for the late reply. Since this PR has been merged I just flashed the current master artifact for the (hotp-maximized) t420 (heads-UNTESTED_t420-hotp-maximized-v0.2.0-1952-g25d7b06.rom). And I can confirm it works fine!

So there is no need for the t420 to be labeled untested.

Merry Christmas!

@tlaurion
Copy link
Collaborator Author

tlaurion commented Dec 23, 2023

@tlaurion, I apologize for the late reply. Since this PR has been merged I just flashed the current master artifact for the (hotp-maximized) t420 (heads-UNTESTED_t420-hotp-maximized-v0.2.0-1952-g25d7b06.rom). And I can confirm it works fine!

So there is no need for the t420 to be labeled untested.

Merry Christmas!

Please open an issue @natterangell !

At this point, that platform as many other is not tested promptly enough, when needed, to be considered tested in a rolling release.

Not enough known willing testers with external programmers are engaged into testing needed changes that could impact the booting. This is now the issue to be resolved. Unless I have time to automatize switching board names and Circleci configs automatically, those boards are now a maintainership problem that withstands needed testing time-frame to consider those boards properly tested by their known testers.

I can put it back to normal (open an issue) but I now require board donations or more reactiveness from those boards flapping between testing and untested in the past. If you know other board owners with external programmers, now would be a good time to tag them in the issue you will open to ask for that board to be switched back to tested. Don't feel alone, the same process will apply to any current board currently marked as untested. For the protection of users who don't have an external programmer and to stop wasting my time doing this ping pong game for what is known to currently be one board owner, which is not enough for my current "maintainer" pay grade (on donations). I'm/was under grant for some pr approved new features/improvements.

But maintaining Heads is not free as in free beer.

Merry Christmas!

@tlaurion
Copy link
Collaborator Author

@natterangell happy new year #1567

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants