Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40798 )
Change subject: [WIP]framebuffer_info: Add new method to specify an exact pixel format ......................................................................
Patch Set 2:
(8 comments)
https://review.coreboot.org/c/coreboot/+/40798/2/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/40798/2/src/device/Kconfig@470 PS2, Line 470: config FRAMEBUFFER_PATTERN nit: I feel like this might be better as a small in-tree payload using CBGFX
https://review.coreboot.org/c/coreboot/+/40798/2/src/include/framebuffer_inf... File src/include/framebuffer_info.h:
https://review.coreboot.org/c/coreboot/+/40798/2/src/include/framebuffer_inf... PS2, Line 27: enum fb_pixelformat { Should mention which endianness is assumed here for clarity.
https://review.coreboot.org/c/coreboot/+/40798/2/src/include/framebuffer_inf... PS2, Line 38: fb_add_framebuffer_info_pf Why a new function? Shouldn't we just update fb_add_framebuffer_info()?
https://review.coreboot.org/c/coreboot/+/40798/2/src/include/framebuffer_inf... PS2, Line 51: pf_bpp_to_pixelformat nit: I'd prefix these with fb_, not pf_, since they still belong to the framebuffer code (and you have "pixelformat" separately in the name anyway).
https://review.coreboot.org/c/coreboot/+/40798/2/src/lib/framebuffer_info.c File src/lib/framebuffer_info.c:
https://review.coreboot.org/c/coreboot/+/40798/2/src/lib/framebuffer_info.c@... PS2, Line 75: for (uint8_t i = 0; i < sizeof(*tmp) * 8; i++) { This seems like a very complicated way to say
*(uint32_t *)fb_ptr = r << fb->red_mask_pos | g << fb->green_mask_pos | b << fb->blue_mask_pos;
https://review.coreboot.org/c/coreboot/+/40798/2/src/lib/framebuffer_info.c@... PS2, Line 103: for (int y = fb->y_resolution / 4; y < fb->y_resolution / 4 * 3; y++) { Are you intentionally not drawing the whole screen? Why?
https://review.coreboot.org/c/coreboot/+/40798/2/src/lib/framebuffer_info.c@... PS2, Line 242: 8, 8, 8, 0, 8); Are you sure this naming convention is common? You're essentially viewing this from a little-endian angle here, i.e. when you say "XRGB" that means B is the lowest address and X is the highest address. In my mind it would be more intuitive from a big-endian view point (i.e. call this format "RGBX"). I don't know much about standard conventions among graphics people, but some quick googling brought me to http://jpeg-turbo.dpldocs.info/libjpeg.turbojpeg.TJPF.html and https://www.kernel.org/doc/html/v4.9/media/uapi/v4l/pixfmt-packed-rgb.html (specifically RGB3 vs BGR3 there), both of which also seem to name formats from a big-endian viewpoint.
https://review.coreboot.org/c/coreboot/+/40798/2/src/lib/framebuffer_info.c@... PS2, Line 304: pf_bpp_to_pixelformat nit: maybe call it bpp_to_default_pixelformat, to clarify that it's arbitrarily choosing one of many options.