Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Add option to enable display over PCI external GFX ......................................................................
Patch Set 10:
(6 comments)
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... File src/soc/intel/icelake/Kconfig:
PS9:
`HAVE_EXT_GFX` should be put into `src/device/Kconfig`, so it’s common between all boards. […]
I have explained below, please check, VGA_ROM_RUN is just to check below code if need to look for OpRom
/** Default handler: only runs the relevant PCI BIOS. */ void pci_dev_init(struct device *dev) { struct rom_header *rom, *ram;
if (!CONFIG(VGA_ROM_RUN)) return;
/* Only execute VGA ROMs. */ if (((dev->class >> 8) != PCI_CLASS_DISPLAY_VGA)) return;
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... PS9, Line 212: bool
Is it selected by default or not?
no, its not default enable
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... PS9, Line 213: select ALWAYS_LOAD_OPROM
I guess Kconfig isn’t selecting this automatically when selecting selecting `ALWAYS_RUN_OPROM`?
i don't get you. Once mb selects HAVE_EXT_GFX kconfig option, then only required kconfig will get auto selected
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... PS9, Line 216: select FRAMEBUFFER_SET_VESA_MODE
Why does this need to be selected?
CONFIG_VGA_ROM_RUN is just to check if cb will look for OpRom or not. thats all there is not much scope of this config. followed by there are dependent kconfig's like ALWAYS_LOAD_OPROM or ALWAYS_RUN_OPROM without those OpRom won't load or execute. if user miss to select any of those then purpose won't be solve. Hence i was trying to create a single kconfig which helps to select all required supported kconfig
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/romst... File src/soc/intel/icelake/romstage/fsp_params.c:
PS9:
If the changes to this file would be in a separate commit, could it submitted already?
i didn't get u
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/romst... PS9, Line 40: /* Set IGD stolen size to 60MB. */
Off-topic: Why 60 MB? Shouldn’t this also be configurable?
its UPD choice right hence we have decided to set 60MB