Change in ...coreboot[master]: lenovo/g505s/Kconfig: Set framebuffer graphics mode to VESA 118h mode
Hello Mike Banon,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/31325
to review the following change.
Change subject: lenovo/g505s/Kconfig: Set framebuffer graphics mode to VESA 118h mode ......................................................................
lenovo/g505s/Kconfig: Set framebuffer graphics mode to VESA 118h mode
Set VESA/native framebuffer mode (needed for bootsplash and graphical framebuffer console) to 118h VESA (1024x768 16.8M-color (8:8:8)) mode because it's the closest to this laptop's 1366x768 screen resolution.
This provides console output even if e.g. GRUB is the payload.
Signed-off-by: Mike Banon mikebdp2@gmail.com Change-Id: I0b4eacf61d905f1160531a988e192c3b626dca68 --- M src/mainboard/lenovo/g505s/Kconfig 1 file changed, 8 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/31325/1
diff --git a/src/mainboard/lenovo/g505s/Kconfig b/src/mainboard/lenovo/g505s/Kconfig index 883ef27..2d1eac7 100644 --- a/src/mainboard/lenovo/g505s/Kconfig +++ b/src/mainboard/lenovo/g505s/Kconfig @@ -55,4 +55,12 @@ string default "1002,990b"
+config FRAMEBUFFER_SET_VESA_MODE + bool + default y + +config FRAMEBUFFER_VESA_DEFAULT_118 + bool + default y + endif # BOARD_LENOVO_G505S
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31325 )
Change subject: lenovo/g505s/Kconfig: Set framebuffer graphics mode to VESA 118h mode ......................................................................
Patch Set 1:
Please note that this depends on CB:31324 change - "src/device/Kconfig: Add Kconfig options to choose a default VESA mode"
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31325 )
Change subject: lenovo/g505s/Kconfig: Set framebuffer graphics mode to VESA 118h mode ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31325/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31325/1//COMMIT_MSG@13 PS1, Line 13: This provides console output even if e.g. GRUB is the payload. Before nothing would be shown?
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31325 )
Change subject: lenovo/g505s/Kconfig: Set framebuffer graphics mode to VESA 118h mode ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31325/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31325/1//COMMIT_MSG@13 PS1, Line 13: This provides console output even if e.g. GRUB is the payload.
Before nothing would be shown?
Yes
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31325 )
Change subject: lenovo/g505s/Kconfig: Set framebuffer graphics mode to VESA 118h mode ......................................................................
Patch Set 2:
Superseded by setting 118h a global default in commit 749fe1e ?
Hello Kyösti Mälkki, Alexander Couzens, Patrick Rudolph, Mike Banon, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31325
to look at the new patch set (#3).
Change subject: lenovo/g505s/Kconfig: Set framebuffer graphics mode to VESA 118h mode ......................................................................
lenovo/g505s/Kconfig: Set framebuffer graphics mode to VESA 118h mode
Set VESA/native framebuffer mode (needed for bootsplash and graphical framebuffer console) to 118h VESA (1024x768 16.8M-color (8:8:8)) mode because it's the closest to this laptop's 1366x768 screen resolution.
This provides console output even if e.g. GRUB is the payload.
Signed-off-by: Mike Banon mikebdp2@gmail.com Change-Id: I0b4eacf61d905f1160531a988e192c3b626dca68 --- M src/mainboard/lenovo/g505s/Kconfig 1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/31325/3
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31325 )
Change subject: lenovo/g505s/Kconfig: Set framebuffer graphics mode to VESA 118h mode ......................................................................
Patch Set 3:
Patch Set 2:
Superseded by setting 118h a global default in commit 749fe1e ?
Thank you for a notice. The first part of this patch - enabling FRAMEBUFFER_SET_VESA_MODE - is still needed, but I have removed the second part.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31325 )
Change subject: lenovo/g505s/Kconfig: Set framebuffer graphics mode to VESA 118h mode ......................................................................
Patch Set 4:
This is user-visible option in menuconfig, nobody else seems to enable this for default config so this raises some eyebrows here. Your arguments sound as if this should be more globally enabled on default builds.
Not my area of expertise, this GFX/VBIOS stuff.
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31325 )
Change subject: lenovo/g505s/Kconfig: Set framebuffer graphics mode to VESA 118h mode ......................................................................
Patch Set 4:
Patch set 4:
nobody else seems to enable this for default config so this raises some eyebrows here.
Maybe that's because the default config is using SeaBIOS and not GRUB. Personally I have not observed any differences while using this framebuffer 118h VESA option together with SeaBIOS. But this change could benefit those G505S owners who would like to use GRUB as the payload, so could be useful for some.
Your arguments sound as if this should be more globally enabled on default builds.
I think it should be decided for each board individually (don't know if this option could be as good/not-harmful for them, needs to be tested by those who are interested)
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31325 )
Change subject: lenovo/g505s/Kconfig: Set framebuffer graphics mode to VESA 118h mode ......................................................................
Patch Set 4:
Please update the commit message and provide some reasoning for this. What happens when it's not set? AIUI, the mode is reset to text mode anyway before launching the payload, so this would only be useful to show a splashscreen in coreboot?
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31325 )
Change subject: lenovo/g505s/Kconfig: Set framebuffer graphics mode to VESA 118h mode ......................................................................
Patch Set 4:
Patch Set 4:
Please update the commit message and provide some reasoning for this.
I already mentioned there the only benefit I knew at the moment of submission: "This provides console output even if e.g. GRUB is the payload."
What happens when it's not set?
Personally I have not noticed any "enabled vs disabled" differences while using SeaBIOS, but maybe that's because I never used a splashscreen. Thank you very much for information, I'm going to test it soon - and if it turns out that this config is required for showing a splashscreen, I will update the commit message with this new info.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31325 )
Change subject: lenovo/g505s/Kconfig: Set framebuffer graphics mode to VESA 118h mode ......................................................................
Patch Set 4:
Please update the commit message and provide some reasoning for this.
I already mentioned there the only benefit I knew at the moment of submission: "This provides console output even if e.g. GRUB is the payload."
Are you sure you tested that this patch, in its current version, applied to upstream coreboot, with the resulting default config, makes a difference?
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31325 )
Change subject: lenovo/g505s/Kconfig: Set framebuffer graphics mode to VESA 118h mode ......................................................................
Patch Set 4:
Patch Set 4:
Please update the commit message and provide some reasoning for this.
I already mentioned there the only benefit I knew at the moment of submission: "This provides console output even if e.g. GRUB is the payload."
Are you sure you tested that this patch, in its current version, applied to upstream coreboot, with the resulting default config, makes a difference?
It's not me but HJK who discovered this effect, and he did it with coreboot 4.8 at November 2018. Here is his full message from the archives: https://mail.coreboot.org/hyperkitty/list/coreboot@coreboot.org/thread/LRKJP... . Sorry, I really should have said it at the beginning and also add "Signed-off-by: Hans Jürgen Kitter eforname@freemail.hu" to this patch as well.
To be honest, personally I'm a bit indifferent to this particular change because it does not change anything for me; but if turns out that it benefits some - e.g. at least those who are using a splash screen - then I'll be happy for doing a good thing for them. It would be much easier for me to test a splash screen than GRUB console output (and I never tried GRUB with coreboot yet), so I will be testing a splashscreen soon - and if there's a difference, perhaps the GRUB reasoning will be replaced with a splashscreen one.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31325 )
Change subject: lenovo/g505s/Kconfig: Set framebuffer graphics mode to VESA 118h mode ......................................................................
Patch Set 4:
It's not me but HJK who discovered this effect, and he did it with coreboot 4.8 at November 2018. Here is his full message from the archives: https://mail.coreboot.org/hyperkitty/list/coreboot@coreboot.org/thread/LRKJP... .
Ok, it seems he didn't mention the exact config and I'm rather sure that SET_VESA_MODE alone doesn't make a difference. So I guess we can close this.
Sorry, I really should have said it at the beginning and also add "Signed-off-by: Hans Jürgen Kitter eforname@freemail.hu" to this patch as well.
You cannot sign off for other people.
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31325 )
Change subject: lenovo/g505s/Kconfig: Set framebuffer graphics mode to VESA 118h mode ......................................................................
Patch Set 4:
Patch Set 4:
It's not me but HJK who discovered this effect, and he did it with coreboot 4.8 at November 2018. Here is his full message from the archives: https://mail.coreboot.org/hyperkitty/list/coreboot@coreboot.org/thread/LRKJP... .
Ok, it seems he didn't mention the exact config
He shared his 4.8 config with me privately; just uploaded it here - https://pastebin.com/eB09ytRS
I'm rather sure that SET_VESA_MODE alone doesn't make a difference.
Indeed, you are right! I just tested a BMP splashscreen (and also found out how to get it working after so many attempts - more information at https://mail.coreboot.org/hyperkitty/list/coreboot@coreboot.org/thread/4L3CW... ) and it worked for both builds, the only difference between which - was a FRAMEBUFFER_SET_VESA_MODE config enabled/disabled.
So, unless someone (probably not me) will test the latest coreboot/dGPU patches with GRUB to check its' console output, this CB:31325 patch above could be abandoned as "not bringing any real world benefit"; marking it as WIP meanwhile...
You cannot sign off for other people.
He entrusted me to work on his patches to get them merged (he already worked a lot to develop them, now it's my turn to do a fair share), and I promised to mention him at my submissions. I think I've seen it at some Linux kernel patches where a team of developers have been working on some patches, and all these developers have been mentioned this way to give them credit - while just one person has been sending them; so I thought it's a correct way to do it.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31325 )
Change subject: lenovo/g505s/Kconfig: Set framebuffer graphics mode to VESA 118h mode ......................................................................
Patch Set 4:
I'm rather sure that SET_VESA_MODE alone doesn't make a difference.
Indeed, you are right! I just tested a BMP splashscreen (and also found out how to get it working after so many attempts - more information at https://mail.coreboot.org/hyperkitty/list/coreboot@coreboot.org/thread/4L3CW... ) and it worked for both builds, the only difference between which - was a FRAMEBUFFER_SET_VESA_MODE config enabled/disabled.
That's about SeaBIOS' bootsplash feature not coreboot's ;) coreboot has it's own (probably not used in 10 years) version that works only with VBIOS and a VBE mode. So by default the VBE mode is only set for that and coreboot would return to text mode before booting the payload (unless configured otherwise, VBE_LINEAR_FRAMEBUFFER, IIRC).
If you ask me, we could just remove that feature from coreboot.
You cannot sign off for other people.
He entrusted me to work on his patches to get them merged (he already worked a lot to develop them, now it's my turn to do a fair share), and I promised to mention him at my submissions. I think I've seen it at some Linux kernel patches where a team of developers have been working on some patches, and all these developers have been mentioned this way to give them credit - while just one person has been sending them; so I thought it's a correct way to do it.
It's not about crediting, it's about legality issues: https://coreboot.org/Development_Guidelines#Sign-off_Procedure You can add a signed-off-by for other people only if they explicitly agreed to that.
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31325 )
Change subject: lenovo/g505s/Kconfig: Set framebuffer graphics mode to VESA 118h mode ......................................................................
Patch Set 4:
That's about SeaBIOS' bootsplash feature not coreboot's ;) coreboot has it's own (probably not used in 10 years) version that works only with VBIOS and a VBE mode. So by default the VBE mode is only set for that and coreboot would return to text mode before booting the payload (unless configured otherwise, VBE_LINEAR_FRAMEBUFFER, IIRC).
Sorry for a late reply. coreboot bootsplash does not support BMP, only JPG, so I made a JPG image using these commands:
convert $image -resize 1024x768! ./bootsplash.jpg mogrify -density 72x72 -units PixelsPerInch ./bootsplash.jpg
(taken from https://github.com/g0tmi1k/os-scripts/blob/master/backtrack5r3.sh , line 212)
Then I tried it with coreboot by enabling the relevant configs, but - despite I see some bootsplash-related coreboot prints in cbmem log - my coreboot is still not showing this image, although SeaBIOS is showing it. To make sure it's coming from SeaBIOS, I just built a version of SeaBIOS without a bootsplash support, and now couldn't see this image. Even when I enabled the max debug level + FT232H debug dongle to significantly prolong the boot time, still couldn't see.
If you ask me, we could just remove that feature from coreboot.
Maybe this feature is working for someone else, and by the way I'd like to see more features added to coreboot, not removed ;)
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31325 )
Change subject: lenovo/g505s/Kconfig: Set framebuffer graphics mode to VESA 118h mode ......................................................................
Patch Set 4:
If you ask me, we could just remove that feature from coreboot.
Maybe this feature is working for someone else, and by the way I'd like to see more features added to coreboot, not removed ;)
I have to ask, how long does coreboot spend in ramstage were you could actually see that bootsplash image, specially with CBMEM as only console? Perhaps put effort in improving the payload side instead to load its bootsplash faster?
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31325 )
Change subject: lenovo/g505s/Kconfig: Set framebuffer graphics mode to VESA 118h mode ......................................................................
Patch Set 4:
I have to ask, how long does coreboot spend in ramstage were you could actually see that bootsplash image, specially with CBMEM as only console? Perhaps put effort in improving the payload side instead to load its bootsplash faster?
To clarify: I could only see a bootsplash displayed by SeaBIOS (if enabled in its' config), but not by coreboot. Here are my current timestamps:
27 entries total:
0:1st timestamp 10,070 1:start of romstage 10,380 (309) 2:before ram initialization 3,765,008 (3,754,628) 3:after ram initialization 17,144,494 (13,379,486) 4:end of romstage 17,155,433 (10,939) 8:starting to load ramstage 17,155,688 (254) 15:starting LZMA decompress (ignore for x86) 17,155,707 (18) 16:finished LZMA decompress (ignore for x86) 17,225,383 (69,676) 9:finished loading ramstage 17,225,710 (326) 10:start of ramstage 17,225,737 (27) 30:device enumeration 17,235,210 (9,473) 40:device configuration 17,245,759 (10,548) 50:device enable 17,261,071 (15,311) 60:device initialization 17,261,222 (151) 65:Option ROM initialization 17,338,465 (77,243) 66:Option ROM copy done 17,355,515 (17,049) 67:Option ROM run done 17,803,653 (448,138) 65:Option ROM initialization 17,803,932 (279) 66:Option ROM copy done 17,813,016 (9,083) 67:Option ROM run done 18,020,791 (207,775) 70:device setup done 18,020,875 (84) 75:cbmem post 18,090,768 (69,892) 80:write tables 18,090,772 (3) 85:finalize chips 18,095,156 (4,383) 90:load payload 18,095,161 (4) 15:starting LZMA decompress (ignore for x86) 18,095,496 (335) 16:finished LZMA decompress (ignore for x86) 18,125,067 (29,571) 99:selfboot jump 18,125,101 (33)
Total Time: 18,115,018
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31325 )
Change subject: lenovo/g505s/Kconfig: Set framebuffer graphics mode to VESA 118h mode ......................................................................
Patch Set 4:
^^^ it takes a bit long because I enabled the max debug level + FT232H debug dongle to significantly prolong the boot time (although still couldn't see a coreboot's bootsplash)
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31325 )
Change subject: lenovo/g505s/Kconfig: Set framebuffer graphics mode to VESA 118h mode ......................................................................
Patch Set 4:
Patch Set 4:
^^^ it takes a bit long because I enabled the max debug level + FT232H debug dongle to significantly prolong the boot time (although still couldn't see a coreboot's bootsplash)
Huh.. what on earth is that raminit doing, do you have debugging from AGESA as well enabled?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31325 )
Change subject: lenovo/g505s/Kconfig: Set framebuffer graphics mode to VESA 118h mode ......................................................................
Patch Set 4:
67:Option ROM run done 18,020,791 (207,775) 70:device setup done 18,020,875 (84) 75:cbmem post 18,090,768 (69,892) 80:write tables 18,090,772 (3) 85:finalize chips 18,095,156 (4,383) 90:load payload 18,095,161 (4) 15:starting LZMA decompress (ignore for x86) 18,095,496 (335) 16:finished LZMA decompress (ignore for x86) 18,125,067 (29,571) 99:selfboot jump 18,125,101 (33)
If we trust the stamps, that's like 100ms the splash would be shown. On 50hz screen that's five frames. Depending of platform I guess, does OPROM control backlight PWM? Assuming that OPROM may also exit before backlight is really lit, you are left with few frames until payload will wipe the framebuffer.
That's the reasoning why coreboot bootsplash is much ado about nothing.
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31325 )
Change subject: lenovo/g505s/Kconfig: Set framebuffer graphics mode to VESA 118h mode ......................................................................
Patch Set 4:
^^^ it takes a bit long because I enabled the max debug level + FT232H debug dongle to significantly prolong the boot time (although still couldn't see a coreboot's bootsplash)
Huh.. what on earth is that raminit doing, do you have debugging from AGESA as well enabled?
Here is my current .config - https://pastebin.com/qxGeTgR4 . As you see I don't have any other debugging features enabled, it's just FT232H introduces such a significant delays. I haven't physically attached FT232H despite enabling this feature, so maybe coreboot is trying to detect it for some time before failing.
If we trust the stamps, that's like 100ms the splash would be shown. On 50hz screen that's five frames. Depending of platform I guess, does OPROM control backlight PWM? Assuming that OPROM may also exit before backlight is really lit, you are left with few frames until payload will wipe the framebuffer.
Yes, indeed: OPROM controls the backlight PWM at G505S. I know it because once I tried the "not-initialized" OPROM directly extracted from a proprietary UEFI image and my backlight didn't work (it seems that while booting UEFI initializes many of its' platform-dependent fields to make it useful) so I know that backlight really depends on that OpROM.
So during all my attempts I've been trying to light the screen with a handheld flashlight (with this trick I was able to see the SeaBIOS letters when my backlight was temporarily software-broken because of that "not-initialized OpROM"), but it was still hard to see anything, and if there are just a few frames of bootsplash according to your calculations - it could be really difficult to notice them.
That's the reasoning why coreboot bootsplash is much ado about nothing.
Maybe this coreboot feature could be made more useful if we would add some timeout? Please tell me, what is the easiest way to add the delay to coreboot's code - and I could try it to see if this helps me to see the coreboot bootsplash.
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31325 )
Change subject: lenovo/g505s/Kconfig: Set framebuffer graphics mode to VESA 118h mode ......................................................................
Patch Set 4:
Marking it as WIP, see https://review.coreboot.org/c/coreboot/+/31325#message-d488850f662e4c86884de... . Please let me know if you have more testing requests, e.g. regarding the coreboot's own bootsplash feature.
Mike Banon has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/31325 )
Change subject: lenovo/g505s/Kconfig: Set framebuffer graphics mode to VESA 118h mode ......................................................................
Abandoned
Not needed anymore.
-
Kyösti Mälkki (Code Review) -
Mike Banon (Code Review)
-
mikeb mikeb (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)