Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 11:
(6 comments)
https://review.coreboot.org/c/coreboot/+/39002/11/src/include/framebuffer_in... File src/include/framebuffer_info.h:
https://review.coreboot.org/c/coreboot/+/39002/11/src/include/framebuffer_in... PS11, Line 30: /* Any reason this now slipped into this patch? Seems to me like it belongs in CB:39430.
https://review.coreboot.org/c/coreboot/+/39002/11/src/include/framebuffer_in... PS11, Line 35: static Is there a good reason this is an inline? It's not *that* small...
https://review.coreboot.org/c/coreboot/+/39002/11/src/include/framebuffer_in... PS11, Line 37: uint16_t bpp) Since the use of this is pretty specific to VESA OPROMs, maybe it would be better to just make it take a vbe_mode_info_t* (e.g. call it vbe_get_row_alignment() or something and put it in src/include/vbe.h)?
https://review.coreboot.org/c/coreboot/+/39002/11/src/include/framebuffer_in... PS11, Line 39: DIV_ROUND_UP(bpp, 8) / 8 If bpp is 24 this will be 3 but we need it to be 4. In fact, I think this is also a mistake in CB:39430. Maybe it would help to have a bytes_per_pixel(int bits_per_pixel) helper function to do that translation (should basically be (1 << log2_ceil(bits_per_pixel)) to always get the next-larger power of two).
https://review.coreboot.org/c/coreboot/+/39002/11/src/include/framebuffer_in... PS11, Line 43: /* find first set bit */ We have an __ffs() function for this... but I don't think this is the right way to do this anyway. I think you just want to find the next power of two that's larger than the difference:
row_alignment = 1 << log2_ceil(scanline_bytes - h_active);
https://review.coreboot.org/c/coreboot/+/39002/11/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/11/src/lib/edid_fill_fb.c@163 PS11, Line 163: Odd indentation?