Change in coreboot[master]: lib/bootsplash: Improve bootsplash logging

[Johanna Schander (Code Review)]

Johanna Schander has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34598 )

Change subject: lib/bootsplash: Improve bootsplash logging
......................................................................

lib/bootsplash: Improve bootsplash logging

Change-Id: Ib4a06d53c0134b99d3e9e6d3eda9fa30fca9ef7d
Signed-off-by: Johanna Schander 
---
M src/lib/bootsplash.c
1 file changed, 7 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/34598/1

diff --git a/src/lib/bootsplash.c b/src/lib/bootsplash.c
index 5527b23..e7f9ba1 100644
--- a/src/lib/bootsplash.c
+++ b/src/lib/bootsplash.c
@@ -25,7 +25,6 @@
 {
    const vbe_mode_info_t *mode_info = vbe_mode_info();
    if (mode_info != NULL) {
-		printk(BIOS_INFO, "Setting up bootsplash\n");
    	unsigned int x_resolution = le16_to_cpu(mode_info->vesa.x_resolution);
    	unsigned int y_resolution = le16_to_cpu(mode_info->vesa.y_resolution);
    	unsigned int fb_resolution = mode_info->vesa.bits_per_pixel;
@@ -42,6 +41,8 @@
 void set_bootsplash(unsigned char *framebuffer, unsigned int x_resolution,
    	    unsigned int y_resolution, unsigned int fb_resolution)
 {
+	printk(BIOS_INFO, "Setting up bootsplash in %dx%d@%d\n", x_resolution, y_resolution,
+	       fb_resolution);
    struct jpeg_decdata *decdata;
    unsigned char *jpeg =
    	cbfs_boot_map_with_leak("bootsplash.jpg", CBFS_TYPE_BOOTSPLASH, NULL);
@@ -50,6 +51,11 @@
    	return;
    }

+	int image_width, image_height;
+	jpeg_fetch_size(jpeg, &image_width, &image_height);
+
+	printk(BIOS_DEBUG, "Bootsplash resolution: %dx%d\n", image_width, image_height);
+
    decdata = malloc(sizeof(*decdata));
    int ret = jpeg_decode(jpeg, framebuffer, x_resolution, y_resolution, fb_resolution,
    		      decdata);

[Johanna Schander (Code Review)]

Johanna Schander has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/34598 )

Change subject: lib/bootsplash: Improve bootsplash logging
......................................................................

lib/bootsplash: Improve bootsplash logging

Change-Id: Ib4a06d53c0134b99d3e9e6d3eda9fa30fca9ef7d
Signed-off-by: Johanna Schander 
---
M src/lib/bootsplash.c
1 file changed, 7 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/34598/2

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34598 )

Change subject: lib/bootsplash: Improve bootsplash logging
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/34598/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34598/2//COMMIT_MSG@8 
PS2, Line 8: 
Maybe be more specific: Output frame buffer data and image size

You could give an old and new example output in the commit message.

Change in coreboot[master]: lib/bootsplash: Log bootsplash dimensions and framebuffer dimensions

[Mimoja (Code Review)]

Hello Paul Menzel, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/34598

to look at the new patch set (#3).

Change subject: lib/bootsplash: Log bootsplash dimensions and framebuffer dimensions
......................................................................

lib/bootsplash: Log bootsplash dimensions and framebuffer dimensions

The bootsplash.jpg needs to match the framebuffer resolution.
Configuration errors are more visible if they can be compared
easily.

Changed message to be always printed:
"Setting up bootsplash in ${FRAMEBUFFER_RESOLUTION}"

Added message:
"Bootsplash image resolution: ${IMAGE_RESOLUTION}"

Change-Id: Ib4a06d53c0134b99d3e9e6d3eda9fa30fca9ef7d
Signed-off-by: Johanna Schander 
---
M src/lib/bootsplash.c
1 file changed, 7 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/34598/3

Change in coreboot[master]: lib/bootsplash: Log bootsplash dimensions and framebuffer dimensions

[Mimoja (Code Review)]

Mimoja has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34598 )

Change subject: lib/bootsplash: Log bootsplash dimensions and framebuffer dimensions
......................................................................

Patch Set 3:

(1 comment)

Jenkins bugged out - Hopefully ready to go :)

https://review.coreboot.org/c/coreboot/+/34598/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34598/2//COMMIT_MSG@8 
PS2, Line 8:
> Maybe be more specific: Output frame buffer data and image size […]

Should be improved now :)

Change in coreboot[master]: lib/bootsplash: Log bootsplash dimensions and framebuffer dimensions

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34598 )

Change subject: lib/bootsplash: Log bootsplash dimensions and framebuffer dimensions
......................................................................

Patch Set 4: Code-Review+1

Change in coreboot[master]: lib/bootsplash: Log bootsplash dimensions and framebuffer dimensions

[Mimoja (Code Review)]

Mimoja has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34598 )

Change subject: lib/bootsplash: Log bootsplash dimensions and framebuffer dimensions
......................................................................

Patch Set 4:

Can this be merged?

Change in coreboot[master]: lib/bootsplash: Log bootsplash dimensions and framebuffer dimensions

[Jonathan Neuschäfer (Code Review)]

Jonathan Neuschäfer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34598 )

Change subject: lib/bootsplash: Log bootsplash dimensions and framebuffer dimensions
......................................................................

Patch Set 4: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/34598/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34598/4//COMMIT_MSG@9 
PS4, Line 9: The bootsplash.jpg needs to match the framebuffer resolution.
What does the code do when they don't match? Does it silently fail?

How about just logging the dimensions in case of a mismatch? This would allow for a more explicit log message.

Change in coreboot[master]: lib/bootsplash: Log bootsplash dimensions and framebuffer dimensions

[Mimoja (Code Review)]

Mimoja has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34598 )

Change subject: lib/bootsplash: Log bootsplash dimensions and framebuffer dimensions
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34598/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34598/4//COMMIT_MSG@9 
PS4, Line 9: The bootsplash.jpg needs to match the framebuffer resolution.
> What does the code do when they don't match? Does it silently fail? […]

decode_jpeg will return an error and log. 
Also in the configuration it states that it has to match.

Change in coreboot[master]: lib/bootsplash: Log bootsplash dimensions and framebuffer dimensions

[Jonathan Neuschäfer (Code Review)]

Jonathan Neuschäfer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34598 )

Change subject: lib/bootsplash: Log bootsplash dimensions and framebuffer dimensions
......................................................................

Patch Set 4: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/34598/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34598/4//COMMIT_MSG@9 
PS4, Line 9: The bootsplash.jpg needs to match the framebuffer resolution.
> decode_jpeg will return an error and log.  […]

Ok, then I still think it might be a good idea to print a log message about the size only if it does match, but then make it clear in the log message that there is a mismatch.

Your choice though.

Change in coreboot[master]: lib/bootsplash: Log bootsplash dimensions and framebuffer dimensions

[Jonathan Neuschäfer (Code Review)]

Jonathan Neuschäfer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34598 )

Change subject: lib/bootsplash: Log bootsplash dimensions and framebuffer dimensions
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34598/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34598/4//COMMIT_MSG@9 
PS4, Line 9: The bootsplash.jpg needs to match the framebuffer resolution.
> Ok, then I still think it might be a good idea to print a log message about the size only if it does […]

TYPO: I meant "only if it does *not* match", sorry.

Change in coreboot[master]: lib/bootsplash: Log bootsplash dimensions and framebuffer dimensions

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34598 )

Change subject: lib/bootsplash: Log bootsplash dimensions and framebuffer dimensions
......................................................................

lib/bootsplash: Log bootsplash dimensions and framebuffer dimensions

The bootsplash.jpg needs to match the framebuffer resolution.
Configuration errors are more visible if they can be compared
easily.

Changed message to be always printed:
"Setting up bootsplash in ${FRAMEBUFFER_RESOLUTION}"

Added message:
"Bootsplash image resolution: ${IMAGE_RESOLUTION}"

Change-Id: Ib4a06d53c0134b99d3e9e6d3eda9fa30fca9ef7d
Signed-off-by: Johanna Schander 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/34598
Reviewed-by: Paul Menzel 
Reviewed-by: Jonathan Neuschäfer 
Tested-by: build bot (Jenkins) 
---
M src/lib/bootsplash.c
1 file changed, 7 insertions(+), 1 deletion(-)

Approvals:
  build bot (Jenkins): Verified
  Paul Menzel: Looks good to me, but someone else must approve
  Jonathan Neuschäfer: Looks good to me, approved

diff --git a/src/lib/bootsplash.c b/src/lib/bootsplash.c
index 5527b23..ee14b92 100644
--- a/src/lib/bootsplash.c
+++ b/src/lib/bootsplash.c
@@ -25,7 +25,6 @@
 {
    const vbe_mode_info_t *mode_info = vbe_mode_info();
    if (mode_info != NULL) {
-		printk(BIOS_INFO, "Setting up bootsplash\n");
    	unsigned int x_resolution = le16_to_cpu(mode_info->vesa.x_resolution);
    	unsigned int y_resolution = le16_to_cpu(mode_info->vesa.y_resolution);
    	unsigned int fb_resolution = mode_info->vesa.bits_per_pixel;
@@ -42,6 +41,8 @@
 void set_bootsplash(unsigned char *framebuffer, unsigned int x_resolution,
    	    unsigned int y_resolution, unsigned int fb_resolution)
 {
+	printk(BIOS_INFO, "Setting up bootsplash in %dx%d@%d\n", x_resolution, y_resolution,
+	       fb_resolution);
    struct jpeg_decdata *decdata;
    unsigned char *jpeg =
    	cbfs_boot_map_with_leak("bootsplash.jpg", CBFS_TYPE_BOOTSPLASH, NULL);
@@ -50,6 +51,11 @@
    	return;
    }

+	int image_width, image_height;
+	jpeg_fetch_size(jpeg, &image_width, &image_height);
+
+	printk(BIOS_DEBUG, "Bootsplash image resolution: %dx%d\n", image_width, image_height);
+
    decdata = malloc(sizeof(*decdata));
    int ret = jpeg_decode(jpeg, framebuffer, x_resolution, y_resolution, fb_resolution,
    		      decdata);