Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34537 )
Change subject: src/device/oprom: Fix bootsplash display code for optionroms ......................................................................
Patch Set 11:
(5 comments)
A reminder to reviewers, no board selects BOOTSPLASH so jenkins score for passing the build is not of any value.
https://review.coreboot.org/c/coreboot/+/34537/10/src/device/oprom/realmode/... File src/device/oprom/realmode/x86.c:
https://review.coreboot.org/c/coreboot/+/34537/10/src/device/oprom/realmode/... PS10, Line 359: (unsigned char *)mode_info.vesa.phys_base_ptr; Either this should have le32_to_cpu() or the two le16_to_cpu() above should be removed, for consistency.
https://review.coreboot.org/c/coreboot/+/34537/11/src/device/oprom/yabel/vbe... File src/device/oprom/yabel/vbe.c:
https://review.coreboot.org/c/coreboot/+/34537/11/src/device/oprom/yabel/vbe... PS11, Line 58: #if CONFIG(BOOTSPLASH) No guards required. But I would appreciate a more thorough fix.
https://review.coreboot.org/c/coreboot/+/34537/11/src/device/oprom/yabel/vbe... PS11, Line 750: #if CONFIG(BOOTSPLASH) This is what I consider the most "broken" part calling for attention. None of the bootsplash should happen inside "vbe_set_graphics()" or even it's parent function "run_bios()".
If you change this such that bootsplash code evaluates vbe_mode_info_t after run_bios() returns, this might become much cleaner and closer to supporting native graphics init in one go.
https://review.coreboot.org/c/coreboot/+/34537/11/src/device/oprom/yabel/vbe... PS11, Line 758: int ret = oprom_set_bootsplash(framebuffer, x_resolution, y_resolution, fb_resolution) Missing ; at end of line?
https://review.coreboot.org/c/coreboot/+/34537/11/src/device/oprom/yabel/vbe... PS11, Line 760: DEBUG_PRINTF_VBE("Could not find bootsplash.jpg\n"); Strongly recommend braces here. Probably does not build currently.