Hello Julius Werner,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/38538
to review the following change.
Change subject: libpayload/corebootfb: Add option to center a 80x25 console ......................................................................
libpayload/corebootfb: Add option to center a 80x25 console
This makes payloads which are hardcoded to a 80x25 console look much better, e.g. FILO with its "GRUB" user interface.
Change-Id: I9f4752328d85d148cd40a0c2337c7191e1d6a586 Signed-off-by: Nico Huber nico.huber@secunet.com --- M payloads/libpayload/Kconfig M payloads/libpayload/drivers/video/corebootfb.c 2 files changed, 28 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/38538/1
diff --git a/payloads/libpayload/Kconfig b/payloads/libpayload/Kconfig index f7501e3..36f4af5 100644 --- a/payloads/libpayload/Kconfig +++ b/payloads/libpayload/Kconfig @@ -315,6 +315,13 @@ Say Y here if coreboot switched to a graphics mode and your payload wants to use it.
+config COREBOOT_VIDEO_CENTERED + bool "Center a classic 80x25 console on bigger screens" + depends on COREBOOT_VIDEO_CONSOLE + help + Say 'y' here if your payload is hardcoded to a 80x25 console. Otherwise + its output would look squeezed into the upper-left corner of the screen. + config FONT_SCALE_FACTOR int "Scale factor for the included font" depends on GEODELX_VIDEO_CONSOLE || COREBOOT_VIDEO_CONSOLE diff --git a/payloads/libpayload/drivers/video/corebootfb.c b/payloads/libpayload/drivers/video/corebootfb.c index c4b5048..8e7ac11 100644 --- a/payloads/libpayload/drivers/video/corebootfb.c +++ b/payloads/libpayload/drivers/video/corebootfb.c @@ -236,8 +236,16 @@
font_init(FI->x_resolution);
- coreboot_video_console.columns = FI->x_resolution / font_width; - coreboot_video_console.rows = FI->y_resolution / font_height; + /* Draw centered on the framebuffer if requested and feasible, */ + const int center = + IS_ENABLED(CONFIG_LP_COREBOOT_VIDEO_CENTERED) + && coreboot_video_console.columns * font_width <= FI->x_resolution + && coreboot_video_console.rows * font_height <= FI->y_resolution; + /* adapt to the framebuffer size, otherwise. */ + if (!center) { + coreboot_video_console.columns = FI->x_resolution / font_width; + coreboot_video_console.rows = FI->y_resolution / font_height; + }
chars = malloc(coreboot_video_console.rows * coreboot_video_console.columns * 2); @@ -246,6 +254,17 @@
// clear boot splash screen if there is one. corebootfb_clear(); + + if (center) { + FI->physical_address += + (FI->x_resolution - coreboot_video_console.columns * font_width) + / 2 * FI->bits_per_pixel / 8 + + (FI->y_resolution - coreboot_video_console.rows * font_height) + / 2 * FI->bytes_per_line; + FI->x_resolution = coreboot_video_console.columns * font_width; + FI->y_resolution = coreboot_video_console.rows * font_height; + } + return 0; }
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38538 )
Change subject: libpayload/corebootfb: Add option to center a 80x25 console ......................................................................
Patch Set 1: Code-Review+1
(2 comments)
\o/
https://review.coreboot.org/c/coreboot/+/38538/1/payloads/libpayload/Kconfig File payloads/libpayload/Kconfig:
https://review.coreboot.org/c/coreboot/+/38538/1/payloads/libpayload/Kconfig... PS1, Line 319: 80x25 Does it work with other sizes as well, or is this only applicable to 80x25?
https://review.coreboot.org/c/coreboot/+/38538/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/corebootfb.c:
https://review.coreboot.org/c/coreboot/+/38538/1/payloads/libpayload/drivers... PS1, Line 244: /* adapt to the framebuffer size, otherwise. */ nit: add a blank line before this comment?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38538 )
Change subject: libpayload/corebootfb: Add option to center a 80x25 console ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38538/1/payloads/libpayload/Kconfig File payloads/libpayload/Kconfig:
https://review.coreboot.org/c/coreboot/+/38538/1/payloads/libpayload/Kconfig... PS1, Line 319: 80x25
Does it work with other sizes as well, or is this only applicable to 80x25?
That's the default that was already hardcoded (and probably the only common setting anyway). We could make it configurable here in Kconfig. But I'd wait until somebody has a real use case for this.
https://review.coreboot.org/c/coreboot/+/38538/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/corebootfb.c:
https://review.coreboot.org/c/coreboot/+/38538/1/payloads/libpayload/drivers... PS1, Line 244: /* adapt to the framebuffer size, otherwise. */
nit: add a blank line before this comment?
It felt like it belongs together. Let me know if you have strong feelings about it.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38538 )
Change subject: libpayload/corebootfb: Add option to center a 80x25 console ......................................................................
Patch Set 1: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38538 )
Change subject: libpayload/corebootfb: Add option to center a 80x25 console ......................................................................
Patch Set 1: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/38538/1/payloads/libpayload/Kconfig File payloads/libpayload/Kconfig:
https://review.coreboot.org/c/coreboot/+/38538/1/payloads/libpayload/Kconfig... PS1, Line 319: 80x25
That's the default that was already hardcoded (and probably the only common […]
Ack
https://review.coreboot.org/c/coreboot/+/38538/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/corebootfb.c:
https://review.coreboot.org/c/coreboot/+/38538/1/payloads/libpayload/drivers... PS1, Line 244: /* adapt to the framebuffer size, otherwise. */
It felt like it belongs together. Let me know if you have strong feelings […]
I have more murderous feelings about the C++ style comment in line 255, so I don't think not having a blank line is too much of an issue.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38538 )
Change subject: libpayload/corebootfb: Add option to center a 80x25 console ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38538/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/corebootfb.c:
https://review.coreboot.org/c/coreboot/+/38538/1/payloads/libpayload/drivers... PS1, Line 244: /* adapt to the framebuffer size, otherwise. */
I have more murderous feelings about the C++ style comment in line 255, so I don't think not having […]
Done, I guess
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38538 )
Change subject: libpayload/corebootfb: Add option to center a 80x25 console ......................................................................
libpayload/corebootfb: Add option to center a 80x25 console
This makes payloads which are hardcoded to a 80x25 console look much better, e.g. FILO with its "GRUB" user interface.
Change-Id: I9f4752328d85d148cd40a0c2337c7191e1d6a586 Signed-off-by: Nico Huber nico.huber@secunet.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/38538 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net Reviewed-by: Angel Pons th3fanbus@gmail.com --- M payloads/libpayload/Kconfig M payloads/libpayload/drivers/video/corebootfb.c 2 files changed, 28 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Paul Menzel: Looks good to me, but someone else must approve Angel Pons: Looks good to me, approved
diff --git a/payloads/libpayload/Kconfig b/payloads/libpayload/Kconfig index f7501e3..36f4af5 100644 --- a/payloads/libpayload/Kconfig +++ b/payloads/libpayload/Kconfig @@ -315,6 +315,13 @@ Say Y here if coreboot switched to a graphics mode and your payload wants to use it.
+config COREBOOT_VIDEO_CENTERED + bool "Center a classic 80x25 console on bigger screens" + depends on COREBOOT_VIDEO_CONSOLE + help + Say 'y' here if your payload is hardcoded to a 80x25 console. Otherwise + its output would look squeezed into the upper-left corner of the screen. + config FONT_SCALE_FACTOR int "Scale factor for the included font" depends on GEODELX_VIDEO_CONSOLE || COREBOOT_VIDEO_CONSOLE diff --git a/payloads/libpayload/drivers/video/corebootfb.c b/payloads/libpayload/drivers/video/corebootfb.c index c4b5048..8e7ac11 100644 --- a/payloads/libpayload/drivers/video/corebootfb.c +++ b/payloads/libpayload/drivers/video/corebootfb.c @@ -236,8 +236,16 @@
font_init(FI->x_resolution);
- coreboot_video_console.columns = FI->x_resolution / font_width; - coreboot_video_console.rows = FI->y_resolution / font_height; + /* Draw centered on the framebuffer if requested and feasible, */ + const int center = + IS_ENABLED(CONFIG_LP_COREBOOT_VIDEO_CENTERED) + && coreboot_video_console.columns * font_width <= FI->x_resolution + && coreboot_video_console.rows * font_height <= FI->y_resolution; + /* adapt to the framebuffer size, otherwise. */ + if (!center) { + coreboot_video_console.columns = FI->x_resolution / font_width; + coreboot_video_console.rows = FI->y_resolution / font_height; + }
chars = malloc(coreboot_video_console.rows * coreboot_video_console.columns * 2); @@ -246,6 +254,17 @@
// clear boot splash screen if there is one. corebootfb_clear(); + + if (center) { + FI->physical_address += + (FI->x_resolution - coreboot_video_console.columns * font_width) + / 2 * FI->bits_per_pixel / 8 + + (FI->y_resolution - coreboot_video_console.rows * font_height) + / 2 * FI->bytes_per_line; + FI->x_resolution = coreboot_video_console.columns * font_width; + FI->y_resolution = coreboot_video_console.rows * font_height; + } + return 0; }
-
Angel Pons (Code Review) -
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)