Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39003 )
Change subject: drivers: Replace multiple fill_fb_framebuffer with single instance ......................................................................
Patch Set 15:
(6 comments)
https://review.coreboot.org/c/coreboot/+/39003/15/src/device/oprom/realmode/... File src/device/oprom/realmode/x86.c:
https://review.coreboot.org/c/coreboot/+/39003/15/src/device/oprom/realmode/... PS15, Line 357: if (!vbe_mode_info_valid()) : return; This is redundant now. We know it is valid, because we just set it. In fill_lb_framebuffer(), it was only necessary because we didn't know if this code ran.
https://review.coreboot.org/c/coreboot/+/39003/15/src/device/oprom/realmode/... PS15, Line 372: mode_info.vesa.blue_mask_size); Passing a struct would really make this less error-prone. I guess we could even write it like this:
fb_add_framebuffer_info_ex((struct lb_framebuffer){ .bits_per_pixel = modeinfo.vesa.bits_per_pixel, ... });
https://review.coreboot.org/c/coreboot/+/39003/15/src/drivers/intel/fsp1_1/f... File src/drivers/intel/fsp1_1/fsp_gop.c:
https://review.coreboot.org/c/coreboot/+/39003/15/src/drivers/intel/fsp1_1/f... PS15, Line 32: BOOT_STATE_INIT_ENTRY(BS_DEV_INIT, BS_ON_EXIT, fill_framebuffer_info, NULL); How about calling it from fsp_run_silicon_init() along with the gfx_set_init_done() call there?
https://review.coreboot.org/c/coreboot/+/39003/15/src/drivers/intel/fsp2_0/g... File src/drivers/intel/fsp2_0/graphics.c:
https://review.coreboot.org/c/coreboot/+/39003/15/src/drivers/intel/fsp2_0/g... PS15, Line 119: BOOT_STATE_INIT_ENTRY(BS_DEV_INIT, BS_ON_EXIT, fill_framebuffer_info, NULL); At end of fsp_silicon_init()?
https://review.coreboot.org/c/coreboot/+/39003/15/src/drivers/intel/gma/hire... File src/drivers/intel/gma/hires_fb/gma-gfx_init.adb:
https://review.coreboot.org/c/coreboot/+/39003/15/src/drivers/intel/gma/hire... PS15, Line 35: end fb_add_framebuffer_info; What's the purpose of this function? Can't we just call the C function directly, below?
https://review.coreboot.org/c/coreboot/+/39003/15/src/drivers/intel/gma/hire... PS15, Line 106: end if; Would be nicer to have only one `if` and call the C function directly, e.g.
if (linear_fb_addr /= 0) then c_fb_add_framebuffer_info (fb_addr => Interfaces.C.size_t (linear_fb_addr), x_resolution => word32 (fb.Width), y_resolution => word32 (fb.Height), bytes_per_line => word32 (fb.Stride) * 4, bits_per_pixel => 32); lightup_ok := 1; else lightup_ok := 0; end if;