Mimoja has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34518 )
Change subject: Remove broken bootsplash display code ......................................................................
Remove broken bootsplash display code
Legacy code, that most likely never worked for most people. It only allowed for 1024x768@16 jpegs, and did not check return values. This resolution is somewhat deprecated and displaying the bootsplash can be done by some payloads anyways (e.g. SeaBIOS, tianocore).
With no more use, the jpeg library as well as depedencies were removed as well.
Change-Id: I77619e1a885ed87123f4f5f185969d16d557e490 Signed-off-by: Johanna Schander coreboot@mimoja.de --- M src/device/oprom/realmode/x86.c 1 file changed, 1 insertion(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/34518/1
diff --git a/src/device/oprom/realmode/x86.c b/src/device/oprom/realmode/x86.c index 67e550c..a55a510 100644 --- a/src/device/oprom/realmode/x86.c +++ b/src/device/oprom/realmode/x86.c @@ -361,19 +361,7 @@ }
vbe_set_mode(&mode_info); -#if CONFIG(BOOTSPLASH) - struct jpeg_decdata *decdata; - unsigned char *jpeg = cbfs_boot_map_with_leak("bootsplash.jpg", - CBFS_TYPE_BOOTSPLASH, - NULL); - if (!jpeg) { - printk(BIOS_DEBUG, "VBE: No bootsplash found.\n"); - return; - } - decdata = malloc(sizeof(*decdata)); - int ret = 0; - ret = jpeg_decode(jpeg, framebuffer, 1024, 768, 16, decdata); -#endif + }
void vbe_textmode_console(void)
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34518 )
Change subject: Remove broken bootsplash display code ......................................................................
Patch Set 1: Code-Review+1
I agree with the arguments, but also touch oprom/yabel/ and src/device/Kconfig.
Hello Kyösti Mälkki, Patrick Rudolph, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34518
to look at the new patch set (#2).
Change subject: Remove broken bootsplash display code ......................................................................
Remove broken bootsplash display code
Legacy code, that most likely never worked for most people. It only allowed for 1024x768@16 jpegs, and did not check return values. This resolution is somewhat deprecated and displaying the bootsplash can be done by some payloads anyways (e.g. SeaBIOS, tianocore).
With no more use, the jpeg library as well as depedencies were removed as well.
Change-Id: I77619e1a885ed87123f4f5f185969d16d557e490 Signed-off-by: Johanna Schander coreboot@mimoja.de --- M src/device/oprom/realmode/x86.c M src/device/oprom/yabel/vbe.c D src/lib/jpeg.c D src/lib/jpeg.h M util/README.md D util/fuzz-tests/Makefile D util/fuzz-tests/README D util/fuzz-tests/description.md D util/fuzz-tests/jpeg-test-cases/coreboot.jpg D util/fuzz-tests/jpeg-test-cases/coreboot_2.jpg D util/fuzz-tests/jpeg-test.c 11 files changed, 1 insertion(+), 1,179 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/34518/2
Mimoja has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34518 )
Change subject: Remove broken bootsplash display code ......................................................................
Patch Set 1:
Yep. Added the rest. Forgot to add it to the commit. I disagree with touching src/device/Kconfig as it does not only affect coreboot, but also SeaBIOS. I think, it should be moved in a different commit
Mimoja has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34518 )
Change subject: Remove broken bootsplash display code ......................................................................
Patch Set 2:
Move it anyways, as it appears only SeaBIOS is using it.
Mimoja has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34518 )
Change subject: Remove broken bootsplash display code ......................................................................
Patch Set 3:
Patch Set 2:
Move it anyways, as it appears only SeaBIOS is using it.
Oups. That should not be moved. Just deleted
Hello Kyösti Mälkki, Patrick Rudolph, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34518
to look at the new patch set (#5).
Change subject: src/device/oprom src/lib: Remove broken bootsplash display code ......................................................................
src/device/oprom src/lib: Remove broken bootsplash display code
Legacy code, that most likely never worked for most people. It only allowed for 1024x768@16 jpegs, and did not check return values. This resolution is somewhat deprecated and displaying the bootsplash can be done by some payloads anyways (e.g. SeaBIOS, tianocore).
With no more use, the jpeg library as well as depedencies were removed as well.
Change-Id: I77619e1a885ed87123f4f5f185969d16d557e490 Signed-off-by: Johanna Schander coreboot@mimoja.de --- M src/device/Kconfig M src/device/oprom/realmode/x86.c M src/device/oprom/yabel/vbe.c D src/lib/jpeg.c D src/lib/jpeg.h M util/README.md D util/fuzz-tests/Makefile D util/fuzz-tests/README D util/fuzz-tests/description.md D util/fuzz-tests/jpeg-test-cases/coreboot.jpg D util/fuzz-tests/jpeg-test-cases/coreboot_2.jpg D util/fuzz-tests/jpeg-test.c 12 files changed, 1 insertion(+), 1,190 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/34518/5
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34518 )
Change subject: src/device/oprom src/lib: Remove broken bootsplash display code ......................................................................
Patch Set 4:
Patch Set 1:
Yep. Added the rest. Forgot to add it to the commit. I disagree with touching src/device/Kconfig as it does not only affect coreboot, but also SeaBIOS. I think, it should be moved in a different commit
I did not see where SeaBIOS would inherit CONFIG_BOOTSPLASH from coreboot .config..
Anyways.. remove: src/lib/Makefile.inc:ramstage-$(CONFIG_BOOTSPLASH) += jpeg.c
Most of the time, I prefer changes to util/ as separate commits.
Christoph Pomaska has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34518 )
Change subject: src/device/oprom src/lib: Remove broken bootsplash display code ......................................................................
Patch Set 5: Code-Review+1
Hello Kyösti Mälkki, Patrick Rudolph, Christoph Pomaska, Philipp Deppenwiese, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34518
to look at the new patch set (#6).
Change subject: src/device/oprom src/lib: Remove broken bootsplash display code ......................................................................
src/device/oprom src/lib: Remove broken bootsplash display code
Legacy code, that most likely never worked for most people. It only allowed for 1024x768@16 jpegs, and did not check return values. This resolution is somewhat deprecated and displaying the bootsplash can be done by some payloads anyways (e.g. SeaBIOS, tianocore).
With no more use, the jpeg library as well as depedencies were removed as well.
Change-Id: I77619e1a885ed87123f4f5f185969d16d557e490 Signed-off-by: Johanna Schander coreboot@mimoja.de --- M Documentation/util.md M src/device/Kconfig M src/device/oprom/realmode/x86.c M src/device/oprom/yabel/vbe.c M src/lib/Makefile.inc D src/lib/jpeg.c D src/lib/jpeg.h M util/README.md D util/fuzz-tests/Makefile D util/fuzz-tests/README D util/fuzz-tests/description.md D util/fuzz-tests/jpeg-test-cases/coreboot.jpg D util/fuzz-tests/jpeg-test-cases/coreboot_2.jpg D util/fuzz-tests/jpeg-test.c 14 files changed, 0 insertions(+), 1,194 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/34518/6
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34518 )
Change subject: src/device/oprom src/lib: Remove broken bootsplash display code ......................................................................
Patch Set 6:
The code is neither legacy nor does it only work with one resolution. I disagree with this removal
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34518 )
Change subject: src/device/oprom src/lib: Remove broken bootsplash display code ......................................................................
Patch Set 6: Code-Review-1
Christoph Pomaska has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34518 )
Change subject: src/device/oprom src/lib: Remove broken bootsplash display code ......................................................................
Patch Set 6: Code-Review-1
Christoph Pomaska has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/34518 )
Change subject: src/device/oprom src/lib: Remove broken bootsplash display code ......................................................................
Removed Code-Review-1 by Christoph Pomaska github@aufmachen.jetzt
Christoph Pomaska has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34518 )
Change subject: src/device/oprom src/lib: Remove broken bootsplash display code ......................................................................
Patch Set 6: Code-Review+1
ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34518 )
Change subject: src/device/oprom src/lib: Remove broken bootsplash display code ......................................................................
Patch Set 6: Code-Review-2
It's limited but I don't see that it's broken. Rather than remove it, add your comments about its limits to the Kconfig so people know about the limits. If you don't like it, don't use it, but I don't see a reason to take this away from people who might want it.
ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34518 )
Change subject: src/device/oprom src/lib: Remove broken bootsplash display code ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34518/6/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/34518/6/src/device/Kconfig@a395 PS6, Line 395: So just propose a comment to add here as to limits.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34518 )
Change subject: src/device/oprom src/lib: Remove broken bootsplash display code ......................................................................
Patch Set 6: Code-Review+2
(1 comment)
If you wish to keep it, please fix it out of the device/oprom/ directory since we have other and better means to setup graphics framebuffer nowadays.
https://review.coreboot.org/c/coreboot/+/34518/6/src/device/oprom/realmode/x... File src/device/oprom/realmode/x86.c:
https://review.coreboot.org/c/coreboot/+/34518/6/src/device/oprom/realmode/x... PS6, Line 375: This makes me question Stefan's argument it is not limited to single resolution.
Mimoja has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34518 )
Change subject: src/device/oprom src/lib: Remove broken bootsplash display code ......................................................................
Patch Set 6:
@Ron, @Stefan Please allow me to disagree
While int jpeg_check_size(unsigned char *buf, int width, int height) exists, it is not used anywhere in the code. (Same as with jpeg_fetch_size)
If the FB is smaller then 1024*768*2 Bytes this code will overflow, given an appropriate jpeg image. For smaller FBs, for bigger bit-depth it will not display the result correctly.
If the image does not match 1024*768@16 it will not work (jpeg.c:300), limiting it to only this one resolution
Therefore i think this code was never able to work in its current state, something I would label "broken".
Mimoja has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34518 )
Change subject: src/device/oprom src/lib: Remove broken bootsplash display code ......................................................................
Patch Set 6:
Therefore I think it should be fixed or removed. I decided for the later with in favor of payload handling.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34518 )
Change subject: src/device/oprom src/lib: Remove broken bootsplash display code ......................................................................
Patch Set 6: Code-Review+1
I agree to the removal, not to the commit summary, though. The code is definitely not broken.
If we don't remove it, we should at least change the Kconfig prompt. Something like "Showcase proof-of-concept bootsplash" would fit the implementation much better. Currently, many new people actually believe that this is _the_ coreboot mechanism to display a bootsplash, and that they need a proprietary VGA BIOS to make it work...
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34518 )
Change subject: src/device/oprom src/lib: Remove broken bootsplash display code ......................................................................
Patch Set 6:
If we keep it, is there a maintainer for it? Do we run the tests for it?
If the answer to those is no, then what?
I'm opposed to just deleting it unilaterally, but if it's broken, it should be fixed.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34518 )
Change subject: src/device/oprom src/lib: Remove broken bootsplash display code ......................................................................
Patch Set 6:
Patch Set 6:
If we keep it, is there a maintainer for it? Do we run the tests for it?
If the answer to those is no, then what?
I'm opposed to just deleting it unilaterally, but if it's broken, it should be fixed.
If the resulting fix is a flickering 100ms of the (coreboot rendered) bootsplash being displayed until payload takes over and possibly changes to different videomode, will you call that a 'fix'.
I am evaluatinh this from the perspective that if the code under review was proposed to be submitted today, would it get in...
Mimoja has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34518 )
Change subject: src/device/oprom src/lib: Remove broken bootsplash display code ......................................................................
Patch Set 6:
If the resulting fix is a flickering 100ms of the (coreboot rendered) bootsplash being displayed until payload takes over and possibly changes to different videomode, will you call that a 'fix'.
It is more than 100ms tbh - but it has a visible "appearing" effect on the screen, which should be accounted for, if we want to keep and fix this feature..
Also: SeaBios drawing over it is resulting in ugly flicker..
https://mimoja.de/files/30ec7436d7d790a4c5ef593c355317432d385001f906636b7b88...
(1280x1024x32)
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34518 )
Change subject: src/device/oprom src/lib: Remove broken bootsplash display code ......................................................................
Patch Set 6:
Patch Set 6:
If the resulting fix is a flickering 100ms of the (coreboot rendered) bootsplash being displayed until payload takes over and possibly changes to different videomode, will you call that a 'fix'.
It is more than 100ms tbh - but it has a visible "appearing" effect on the screen, which should be accounted for, if we want to keep and fix this feature..
Also: SeaBios drawing over it is resulting in ugly flicker..
I agree that if you're using SeaBIOS as your payload, this probably isn't the best option, but not everyone uses SeaBIOS, and not every payload resets the screen immediately.
I also agree that it should be fixed if it's broken and there's someone willing to maintain it.
We can add a note saying that if you're using SeaBIOS, not to use that option, to use the SeaBIOS splash screen. Or we can add a configurable delay so that people can see the coreboot splash screen for a sufficient amount of time if your complaint is that it goes by too fast. Or we can run a payload that doesn't clear the screen.
What I don't want is to just delete it without discussion. If nobody's willing to step up for this feature, then I'll stop fussing.
Mimoja has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34518 )
Change subject: src/device/oprom src/lib: Remove broken bootsplash display code ......................................................................
Patch Set 6:
We should also keep in mind, that the current code structure requires Oprom execution. To my understanding it is currently impossbile to get a bootsplash together with native graphics init. So if we were to keep it, maybe it should be moved to src/lib or something alike and call it from all the framebuffer initialization codes.
How are usually decisions like this worked out?
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34518 )
Change subject: src/device/oprom src/lib: Remove broken bootsplash display code ......................................................................
Patch Set 6:
We should also keep in mind, that the current code structure requires Oprom execution. To my understanding it is currently impossbile to get a bootsplash together with native graphics init.
That's because this code predates native graphics init by 8 years or so and the NGI devs didn't bother to update it ;-)
So if we were to keep it, maybe it should be moved to src/lib or something alike and call it from all the framebuffer initialization codes.
Sounds good. do you want to do it, or shall I take over?
How are usually decisions like this worked out?
Discussion on the mailing list, or a patch to review on Gerrit. In either case it helps to avoid terms like "broken", "most likely never worked", "no more use" though unless you're _really_, _really_ sure about that.
As an aside, the whole coreboot ecosystem isn't SeaBIOS and Tianocore. In fact, those are the newcomers.
Mimoja has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34518 )
Change subject: src/device/oprom src/lib: Remove broken bootsplash display code ......................................................................
Patch Set 6:
I'm totally fine doing it. I just want to work in either way. Will fix it for option-roms first, and move it afterwards.
I will abandon this commit then and move on -> https://review.coreboot.org/c/coreboot/+/34537
Mimoja has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/34518 )
Change subject: src/device/oprom src/lib: Remove broken bootsplash display code ......................................................................
Abandoned
Will fix rather than rant
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34518 )
Change subject: src/device/oprom src/lib: Remove broken bootsplash display code ......................................................................
Patch Set 6:
Thanks Johanna, I think this was actually a really good issue to bring up. We definitely need to make sure that all of our modules are working as intended.
I also appreciate your willingness to fix it instead of removing it.
Johanna Schander has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34518 )
Change subject: src/device/oprom src/lib: Remove broken bootsplash display code ......................................................................
Patch Set 6:
Patch Set 6:
Thanks Johanna, I think this was actually a really good issue to bring up. We definitely need to make sure that all of our modules are working as intended.
I also appreciate your willingness to fix it instead of removing it.
Thank you! Always nice to hear <3
-
Christoph Pomaska (Code Review) -
Johanna Schander (Code Review)
-
Kyösti Mälkki (Code Review)
-
Martin Roth (Code Review)
-
Mimoja (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
ron minnich (Code Review)
-
Stefan Reinauer (Code Review)