Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34877 )
Change subject: mb/google/kukui: Move panel description to CBFS files ......................................................................
Patch Set 4: Code-Review+2
(5 comments)
https://review.coreboot.org/c/coreboot/+/34877/3/src/mainboard/google/kukui/... File src/mainboard/google/kukui/mainboard.c:
https://review.coreboot.org/c/coreboot/+/34877/3/src/mainboard/google/kukui/... PS3, Line 151: edid->y_resolution ? edid->y_resolution : edid->mode.va,
For MIPI panels I can be sure the ha is exactly x resolution, but for eDP panels I remember that may […]
ha is only different from x_resolution if the framebuffer uses aligned rows and the resolution doesn't cleanly fit that alignment (see edid_set_framebuffer_bits_per_pixel(). I think that's actually wrong when looking at it again right now. Libpayload uses x_resolution directly as the size of the screen it draws on, but if it counts that alignment padding then it will think there are more pixels than there actually are. This would cause things to scale that shouldn't get scaled and make the image slightly off-center.
Didn't really have time to test that theory yet. I think in practice there are almost no resolutions actually end up with misaligned lines (800, 1024, 1200 and 1920 all divide cleanly by 64/4), so maybe we just never really see it. But it looks wrong to me.
https://review.coreboot.org/c/coreboot/+/34877/4/src/mainboard/google/kukui/... File src/mainboard/google/kukui/mainboard.c:
https://review.coreboot.org/c/coreboot/+/34877/4/src/mainboard/google/kukui/... PS4, Line 105: static u8 buffer[8 * 1024]; nit: write this as
static union { u8 raw[8*KiB]; struct panel_serializable_data s; } buffer;
Then you don't need the cast below and more importantly it will make sure the buffer is correctly aligned for the structure.
https://review.coreboot.org/c/coreboot/+/34877/4/src/mainboard/google/kukui/... PS4, Line 111: cbfs_name = malloc(name_len); nit: just use 64 or 128 byte buffer on the stack, no need to precisely measure this out
https://review.coreboot.org/c/coreboot/+/34877/4/src/mainboard/google/kukui/... PS4, Line 116: int r = cbfs_boot_load_file(cbfs_name, buffer, sizeof(buffer), nit: technically should be size_t
https://review.coreboot.org/c/coreboot/+/34877/4/src/mainboard/google/kukui/... PS4, Line 154: assert(panel->s); nit: this would make more sense in get_active_panel() before you dereference panel->s for the first time