Patrick Rudolph 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 9: Code-Review-1
(3 comments)
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... File src/soc/intel/icelake/Kconfig:
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... PS9, Line 211: config HAVE_EXT_GFX 1) The Kconfig is poorly named. Just because an EXT GFX *exists*, doens't mean you want to use it as primary VGA device. For example on Nvidia Optimus enabled laptops, that do not support hybrid graphics, the Intel GPU is always connected to the panel. It has an ext gfx, but it's never used as primary VGA device. 2) Also by selecting this on mainboard level you force the use of untrusted legacy blobs. 3) It's not even visible to the user why this board selects the method as the bool has no name. 4) I don't see why this is icelace specific.
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/fsp_p... File src/soc/intel/icelake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/fsp_p... PS9, Line 89: if (!dev || !dev->enabled) good
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/romst... File src/soc/intel/icelake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/romst... PS9, Line 31: if (!dev || !dev->enabled) { good