Maxim Polyakov has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32044
Change subject: soc/skl: Use devtree options to set primary GPU ......................................................................
soc/skl: Use devtree options to set primary GPU
List of options: - InternalGfx, - PrimaryDisplay, - SkipExtGfxScan.
Change-Id: Ie3f9362676105e41c69139a094dbb9e8b865689f Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/soc/intel/skylake/romstage/romstage_fsp20.c 1 file changed, 10 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/32044/1
diff --git a/src/soc/intel/skylake/romstage/romstage_fsp20.c b/src/soc/intel/skylake/romstage/romstage_fsp20.c index 43ba9c9..f38a775 100644 --- a/src/soc/intel/skylake/romstage/romstage_fsp20.c +++ b/src/soc/intel/skylake/romstage/romstage_fsp20.c @@ -274,6 +274,16 @@ /* Enable SMBus controller based on config */ m_cfg->SmbusEnable = config->SmbusEnable;
+ /* Set primary graphic device */ + if (CONFIG(ONBOARD_VGA_IS_PRIMARY)) { + m_cfg->PrimaryDisplay = 0; + m_cfg->InternalGfx = 1; + } else { + m_cfg->PrimaryDisplay = config->PrimaryDisplay; + m_cfg->InternalGfx = config->InternalGfx; + } + m_t_cfg->SkipExtGfxScan = config->SkipExtGfxScan; + mainboard_memory_init_params(mupd); }
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32044 )
Change subject: soc/skl: Use devtree options to set primary GPU ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32044/1/src/soc/intel/skylake/romstage/romst... File src/soc/intel/skylake/romstage/romstage_fsp20.c:
https://review.coreboot.org/#/c/32044/1/src/soc/intel/skylake/romstage/romst... PS1, Line 282: PrimaryDisplay I don't see any skylake/kabylake boards currently setting this in the device tree. And from the FSP UPD descriptions, it looks like the default value for this field is 3. So, if you plan to update the UPD, you will have to update all the boards which were defaulting to 3 to set this in their device tree.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32044 )
Change subject: soc/skl: Use devtree options to set primary GPU ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/#/c/32044/1/src/soc/intel/skylake/romstage/romst... File src/soc/intel/skylake/romstage/romstage_fsp20.c:
https://review.coreboot.org/#/c/32044/1/src/soc/intel/skylake/romstage/romst... PS1, Line 280: m_cfg->InternalGfx = 1; set internalGfx to enabled if PCI 0:2.0 is present and enabled in devicetree.
https://review.coreboot.org/#/c/32044/1/src/soc/intel/skylake/romstage/romst... PS1, Line 285: m_t_cfg->SkipExtGfxScan = config->SkipExtGfxScan; SkipExtGfxScan should only be set to 1 on all boards that only use IGD (like chromebooks).
Hello Patrick Rudolph, Angel Pons, Paul Menzel, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32044
to look at the new patch set (#2).
Change subject: soc/skl: Use devtree options to set primary GPU ......................................................................
soc/skl: Use devtree options to set primary GPU
List of options: - InternalGfx, - PrimaryDisplay, - SkipExtGfxScan.
Change-Id: Ie3f9362676105e41c69139a094dbb9e8b865689f Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/romstage/romstage_fsp20.c 2 files changed, 41 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/32044/2
Hello Patrick Rudolph, Angel Pons, Paul Menzel, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32044
to look at the new patch set (#3).
Change subject: soc/skl: Use devtree options to set primary GPU ......................................................................
soc/skl: Use devtree options to set primary GPU
List of options: - InternalGfx, - PrimaryDisplay, - SkipExtGfxScan.
Change-Id: Ie3f9362676105e41c69139a094dbb9e8b865689f Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/romstage/romstage_fsp20.c 2 files changed, 41 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/32044/3
Hello Patrick Rudolph, Angel Pons, Paul Menzel, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32044
to look at the new patch set (#4).
Change subject: soc/skl: Use devtree options to set primary GPU ......................................................................
soc/skl: Use devtree options to set primary GPU
List of options: - InternalGfx, - PrimaryDisplay, - SkipExtGfxScan.
Change-Id: Ie3f9362676105e41c69139a094dbb9e8b865689f Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/romstage/romstage_fsp20.c 2 files changed, 44 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/32044/4
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32044 )
Change subject: soc/skl: Use devtree options to set primary GPU ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/#/c/32044/4/src/soc/intel/skylake/chip.h File src/soc/intel/skylake/chip.h:
https://review.coreboot.org/#/c/32044/4/src/soc/intel/skylake/chip.h@314 PS4, Line 314: /* I think it's better to use enum here.
https://review.coreboot.org/#/c/32044/1/src/soc/intel/skylake/romstage/romst... File src/soc/intel/skylake/romstage/romstage_fsp20.c:
https://review.coreboot.org/#/c/32044/1/src/soc/intel/skylake/romstage/romst... PS1, Line 280: m_cfg->InternalGfx = 1;
set internalGfx to enabled if PCI 0:2.0 is present and enabled in devicetree.
Ok. Done
https://review.coreboot.org/#/c/32044/1/src/soc/intel/skylake/romstage/romst... PS1, Line 282: PrimaryDisplay
I don't see any skylake/kabylake boards currently setting this in the device tree. […]
I set PrimaryDisplay devicetree.cb for all skl/kbl board.
patch : Change-Id: Ibed05fc9171e2bd73654f0da6273a8534746913d
For h110m - set "Display_PEG" (1) others - set "Display_Auto" (3)
https://review.coreboot.org/#/c/32044/1/src/soc/intel/skylake/romstage/romst... PS1, Line 285: m_t_cfg->SkipExtGfxScan = config->SkipExtGfxScan;
SkipExtGfxScan should only be set to 1 on all boards that only use IGD (like chromebooks).
devicetree.cb for all boards with a skl / kbl processor already contain this option.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32044 )
Change subject: soc/skl: Use devtree options to set primary GPU ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/32044/4/src/soc/intel/skylake/romstage/romst... File src/soc/intel/skylake/romstage/romstage_fsp20.c:
https://review.coreboot.org/#/c/32044/4/src/soc/intel/skylake/romstage/romst... PS4, Line 266: InternalGfx Does it make sense to have it in devicetree? I'd default to 1 as the pci device is enabled.
Hello Patrick Rudolph, Angel Pons, Paul Menzel, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32044
to look at the new patch set (#5).
Change subject: soc/intel/skylake: Update GFX devtree options ......................................................................
soc/intel/skylake: Update GFX devtree options
This patch includes the following changes:
1. Sets FSP options in romstage_fsp20.c to select primary GPU. List of options: - InternalGfx, - PrimaryDisplay, - SkipExtGfxScan.
2. iGPU will be initialized if the corresponding pci device is defined in the device tree as:
device pci 02.0 on end
In this case, the InternalGfx in struct soc_intel_skylake_config is not required. This parameter is removed from chip.h.
3. Adds PrimaryDisplay and removes InternalGfx option in the devicetree.cb for all motherboards with Skylake/Kaby Lake processor.
Change-Id: Ie3f9362676105e41c69139a094dbb9e8b865689f Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/mainboard/asrock/h110m/devicetree.cb M src/mainboard/google/eve/devicetree.cb M src/mainboard/google/fizz/variants/baseboard/devicetree.cb M src/mainboard/google/glados/variants/asuka/devicetree.cb M src/mainboard/google/glados/variants/caroline/devicetree.cb M src/mainboard/google/glados/variants/cave/devicetree.cb M src/mainboard/google/glados/variants/chell/devicetree.cb M src/mainboard/google/glados/variants/glados/devicetree.cb M src/mainboard/google/glados/variants/lars/devicetree.cb M src/mainboard/google/glados/variants/sentry/devicetree.cb M src/mainboard/google/poppy/variants/atlas/devicetree.cb M src/mainboard/google/poppy/variants/baseboard/devicetree.cb M src/mainboard/google/poppy/variants/nami/devicetree.cb M src/mainboard/google/poppy/variants/nautilus/devicetree.cb M src/mainboard/google/poppy/variants/nocturne/devicetree.cb M src/mainboard/google/poppy/variants/rammus/devicetree.cb M src/mainboard/google/poppy/variants/soraka/devicetree.cb M src/mainboard/intel/kblrvp/variants/baseboard/devicetree.cb M src/mainboard/intel/kunimitsu/devicetree.cb M src/mainboard/intel/saddlebrook/devicetree.cb M src/mainboard/purism/librem_skl/variants/librem13v2/devicetree.cb M src/mainboard/purism/librem_skl/variants/librem15v3/devicetree.cb M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/romstage/romstage_fsp20.c 24 files changed, 66 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/32044/5
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32044 )
Change subject: soc/intel/skylake: Update GFX devtree options ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/32044/4/src/soc/intel/skylake/romstage/romst... File src/soc/intel/skylake/romstage/romstage_fsp20.c:
https://review.coreboot.org/#/c/32044/4/src/soc/intel/skylake/romstage/romst... PS4, Line 266: InternalGfx
Does it make sense to have it in devicetree? I'd default to 1 as the pci device is enabled.
Ok. Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32044 )
Change subject: soc/intel/skylake: Update GFX devtree options ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/#/c/32044/5/src/soc/intel/skylake/chip.h File src/soc/intel/skylake/chip.h:
https://review.coreboot.org/#/c/32044/5/src/soc/intel/skylake/chip.h@314 PS5, Line 314: /* : * Selection of the primary display device : * 0: Display_iGFX : * 1: Display_PEG : * 2: Display_PCH_PCIe : * 3: Display_Auto (Default) : * 4: Display_Switchable : */ Please no comments that just repeat the code.
https://review.coreboot.org/#/c/32044/5/src/soc/intel/skylake/chip.h@323 PS5, Line 323: Display_iGFX, If this is 0, it's the default and can be left unset in the devicetrees.
https://review.coreboot.org/#/c/32044/5/src/soc/intel/skylake/romstage/romst... File src/soc/intel/skylake/romstage/romstage_fsp20.c:
https://review.coreboot.org/#/c/32044/5/src/soc/intel/skylake/romstage/romst... PS5, Line 264: if (CONFIG(ONBOARD_VGA_IS_PRIMARY)) Redundant configuration in Kconfig and the devicetree is discouraged. I'd prefer to hide the Kconfig for these platforms instead.
Hello Patrick Rudolph, Angel Pons, Paul Menzel, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32044
to look at the new patch set (#6).
Change subject: {mb,soc/intel/skylake}: Update GFX devtree options ......................................................................
{mb,soc/intel/skylake}: Update GFX devtree options
This patch includes the following changes:
1. Sets FSP options in romstage_fsp20.c to select primary GPU. List of options: - InternalGfx, - PrimaryDisplay, - SkipExtGfxScan.
2. iGPU will be initialized if the corresponding pci device is defined in the device tree as:
device pci 02.0 on end
In this case, the InternalGfx in struct soc_intel_skylake_config is not required. This parameter is removed from chip.h.
3. Removes InternalGfx option in the devicetree.cb for all motherboards with skl/kbl processor. 4. Overrides PrimaryDisplay for some skl/kbl mainboards. This is not required for chromebooks, as these devices use the default value - Primary_iGFX.
Change-Id: Ie3f9362676105e41c69139a094dbb9e8b865689f Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/mainboard/asrock/h110m/devicetree.cb M src/mainboard/google/eve/devicetree.cb M src/mainboard/google/fizz/variants/baseboard/devicetree.cb M src/mainboard/google/glados/variants/asuka/devicetree.cb M src/mainboard/google/glados/variants/caroline/devicetree.cb M src/mainboard/google/glados/variants/cave/devicetree.cb M src/mainboard/google/glados/variants/chell/devicetree.cb M src/mainboard/google/glados/variants/glados/devicetree.cb M src/mainboard/google/glados/variants/lars/devicetree.cb M src/mainboard/google/glados/variants/sentry/devicetree.cb M src/mainboard/google/poppy/variants/atlas/devicetree.cb M src/mainboard/google/poppy/variants/baseboard/devicetree.cb M src/mainboard/google/poppy/variants/nami/devicetree.cb M src/mainboard/google/poppy/variants/nautilus/devicetree.cb M src/mainboard/google/poppy/variants/nocturne/devicetree.cb M src/mainboard/google/poppy/variants/rammus/devicetree.cb M src/mainboard/google/poppy/variants/soraka/devicetree.cb M src/mainboard/intel/kblrvp/variants/baseboard/devicetree.cb M src/mainboard/intel/kunimitsu/devicetree.cb M src/mainboard/intel/saddlebrook/devicetree.cb M src/mainboard/purism/librem_skl/variants/librem13v2/devicetree.cb M src/mainboard/purism/librem_skl/variants/librem15v3/devicetree.cb M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/romstage/romstage_fsp20.c 24 files changed, 39 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/32044/6
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32044 )
Change subject: {mb,soc/intel/skylake}: Update GFX devtree options ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/#/c/32044/5/src/soc/intel/skylake/chip.h File src/soc/intel/skylake/chip.h:
https://review.coreboot.org/#/c/32044/5/src/soc/intel/skylake/chip.h@314 PS5, Line 314: /* : * Selection of the primary display device : * 0: Display_iGFX : * 1: Display_PEG : * 2: Display_PCH_PCIe : * 3: Display_Auto (Default) : * 4: Display_Switchable : */
Please no comments that just repeat the code.
Fixed
https://review.coreboot.org/#/c/32044/5/src/soc/intel/skylake/chip.h@323 PS5, Line 323: Display_iGFX,
If this is 0, it's the default and can be left unset in the devicetrees.
Fixed in the DTs.
https://review.coreboot.org/#/c/32044/5/src/soc/intel/skylake/romstage/romst... File src/soc/intel/skylake/romstage/romstage_fsp20.c:
https://review.coreboot.org/#/c/32044/5/src/soc/intel/skylake/romstage/romst... PS5, Line 264: if (CONFIG(ONBOARD_VGA_IS_PRIMARY))
Redundant configuration in Kconfig and the devicetree is […]
I removed CONFIG(ONBOARD_VGA_IS_PRIMARY) from this patch-set.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32044 )
Change subject: {mb,soc/intel/skylake}: Update GFX devtree options ......................................................................
Patch Set 6: Code-Review+1
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32044 )
Change subject: {mb,soc/intel/skylake}: Update GFX devtree options ......................................................................
Patch Set 6:
why set PrimaryDisplay for the Librems (and most other Google devices) to Auto (which was the previous default since not set by coreboot?), but set Chromebooks to iGFX (via the new enum default)?
Hello Patrick Rudolph, Angel Pons, Youness Alaoui, caveh jalali, Matt DeVillier, Duncan Laurie, Paul Menzel, Gaggery Tsai, build bot (Jenkins), Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32044
to look at the new patch set (#7).
Change subject: {mb,soc/intel/skylake}: Update GFX devtree options ......................................................................
{mb,soc/intel/skylake}: Update GFX devtree options
This patch includes the following changes:
1. Sets FSP options in romstage_fsp20.c to select primary GPU. List of options: - InternalGfx, - PrimaryDisplay, - SkipExtGfxScan.
2. iGPU will be initialized if the corresponding pci device is defined in the device tree as:
device pci 02.0 on end
In this case, the InternalGfx in struct soc_intel_skylake_config is not required. This parameter is removed from chip.h.
3. Removes InternalGfx option in the devicetree.cb for all motherboards with skl/kbl processor. 4. Overrides PrimaryDisplay for some skl/kbl mainboards. If this option is not defined in the devicetree.cb, the default value Primary_iGFX is used.
Change-Id: Ie3f9362676105e41c69139a094dbb9e8b865689f Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/mainboard/asrock/h110m/devicetree.cb M src/mainboard/google/eve/devicetree.cb M src/mainboard/google/fizz/variants/baseboard/devicetree.cb M src/mainboard/google/glados/variants/asuka/devicetree.cb M src/mainboard/google/glados/variants/caroline/devicetree.cb M src/mainboard/google/glados/variants/cave/devicetree.cb M src/mainboard/google/glados/variants/chell/devicetree.cb M src/mainboard/google/glados/variants/glados/devicetree.cb M src/mainboard/google/glados/variants/lars/devicetree.cb M src/mainboard/google/glados/variants/sentry/devicetree.cb M src/mainboard/google/poppy/variants/atlas/devicetree.cb M src/mainboard/google/poppy/variants/baseboard/devicetree.cb M src/mainboard/google/poppy/variants/nami/devicetree.cb M src/mainboard/google/poppy/variants/nautilus/devicetree.cb M src/mainboard/google/poppy/variants/nocturne/devicetree.cb M src/mainboard/google/poppy/variants/rammus/devicetree.cb M src/mainboard/google/poppy/variants/soraka/devicetree.cb M src/mainboard/intel/kblrvp/variants/baseboard/devicetree.cb M src/mainboard/intel/kunimitsu/devicetree.cb M src/mainboard/intel/saddlebrook/devicetree.cb M src/mainboard/purism/librem_skl/variants/librem13v2/devicetree.cb M src/mainboard/purism/librem_skl/variants/librem15v3/devicetree.cb M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/romstage/romstage_fsp20.c 24 files changed, 37 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/32044/7
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32044 )
Change subject: {mb,soc/intel/skylake}: Update GFX devtree options ......................................................................
Patch Set 7:
Patch Set 6:
why set PrimaryDisplay for the Librems (and most other Google devices) to Auto (which was the previous default since not set by coreboot?), but set Chromebooks to iGFX (via the new enum default)?
I know that Chromebooks use only the internal graphics card. However, I’m not sure about Intel reference design boards in src/mb/intel. Are these Chromebooks too? I have never seen them.
I have Intel KBL-R RVP with i5-8350 soc. This mainboard doesn`t have a PEG port, but I can insert a GPU card into PCH PCIe x4 and it should work. However, if you use the value Primary_iGFX, then the external GPU won't be used for output (unless dynamic switching is active in OS). For this reason, for the reference design board it is better to use the automatic detection of the primary GPU (or use the Display_PCH_PCIe value, but I decided to use the Auto).
iGFX should be used only when the system uses a GPU inside soc and does not provide for connecting external devices.
I read about the Librem article https://www.pcworld.com/article/2849795/purism-librem-15-linux-laptop-blends... , which mentions external GPU: "The Librem 15 includes a 15.6-inch 1920x1080 display, an 8-core 2.3 GHz Intel Core i7 CPU, NVIDIA graphics..." But I didn`t find information about a discrete GPU in the coreboot for these laptops. Maybe it was not implemented. I will remove this parameter from the devicetree.cb to use the Primary_iGFX value for these notebooks.
Thanks
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32044 )
Change subject: {mb,soc/intel/skylake}: Update GFX devtree options ......................................................................
Patch Set 7:
(1 comment)
I read about the Librem article
that was the original model, never ported to coreboot. Every model since has coreboot support and only has Intel iGPU, and no capability to use an external or dGPU. So I think defaulting to iGFX makes sense for them
https://review.coreboot.org/#/c/32044/6/src/mainboard/purism/librem_skl/vari... File src/mainboard/purism/librem_skl/variants/librem13v2/devicetree.cb:
https://review.coreboot.org/#/c/32044/6/src/mainboard/purism/librem_skl/vari... PS6, Line 61: register "PrimaryDisplay" = "Display_Auto" why are we setting the Librems (and other non-Chromebooks) to Auto here (which was the previous default when unset), but setting Chromebooks to iGFX (via the new enum default)?
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32044 )
Change subject: {mb,soc/intel/skylake}: Update GFX devtree options ......................................................................
Patch Set 7:
(sorry for the dupe, looks like gerrit pulled in a draft comment from an earlier patch set)
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32044 )
Change subject: {mb,soc/intel/skylake}: Update GFX devtree options ......................................................................
Patch Set 7:
Patch Set 7:
(1 comment)
I read about the Librem article
that was the original model, never ported to coreboot. Every model since has coreboot support and only has Intel iGPU, and no capability to use an external or dGPU. So I think defaulting to iGFX makes sense for them
Yes, I agree with you. I fixed it in the last patch.
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32044 )
Change subject: {mb,soc/intel/skylake}: Update GFX devtree options ......................................................................
Patch Set 7:
Patch Set 7:
Patch Set 7:
(1 comment)
I read about the Librem article
that was the original model, never ported to coreboot. Every model since has coreboot support and only has Intel iGPU, and no capability to use an external or dGPU. So I think defaulting to iGFX makes sense for them
Yes, I agree with you. I fixed it in the last patch.
In patch-set 7
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32044 )
Change subject: {mb,soc/intel/skylake}: Update GFX devtree options ......................................................................
Patch Set 7: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32044 )
Change subject: {mb,soc/intel/skylake}: Update GFX devtree options ......................................................................
Patch Set 7: Code-Review-1
IMO, this patch does too much at once to keep track of everything...
Open questions:
* Why change the PrimaryDisplay setting of h110m? * Why would we ever set `PrimaryDisplay = Display_Auto` together with `SkipExtGfxScan`? * If I didn't miss it, the `SkipExtGfxScan` UPD wasn't set before? So this change changes all boards without mentioning it? * What does `SkipExtGfxScan` do anyway?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32044 )
Change subject: {mb,soc/intel/skylake}: Update GFX devtree options ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/32044/5/src/soc/intel/skylake/romstage/romst... File src/soc/intel/skylake/romstage/romstage_fsp20.c:
https://review.coreboot.org/#/c/32044/5/src/soc/intel/skylake/romstage/romst... PS5, Line 264: if (CONFIG(ONBOARD_VGA_IS_PRIMARY))
I removed CONFIG(ONBOARD_VGA_IS_PRIMARY) from this patch-set.
I somehow missed earlier that no matter the Kconfig value, the corresponding coreboot code to route legacy VGA resources will be executed anyway. I have a bad feeling about this.
Furquan Shaikh has removed a vote on this change.
Change subject: {mb,soc/intel/skylake}: Update GFX devtree options ......................................................................
Removed Code-Review+2 by Furquan Shaikh furquan@google.com
Hello Patrick Rudolph, Angel Pons, Youness Alaoui, Matt DeVillier, Duncan Laurie, Paul Menzel, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, caveh jalali, Gaggery Tsai, Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32044
to look at the new patch set (#8).
Change subject: soc/intel/skylake: Update GFX devtree options ......................................................................
soc/intel/skylake: Update GFX devtree options
This patch includes the following changes:
1. Sets FSP options in romstage_fsp20.c to select primary GPU. List of options: - InternalGfx, - PrimaryDisplay, - SkipExtGfxScan.
2. iGPU will be initialized if the corresponding PCI device is defined in the device tree as:
device pci 02.0 on end
In this case, it is not necessary to set the InternalGfx option to enable this device
3. Primary_iGFX is used as the default value for all skl/kbl boards (since the PrimaryDisplay option isn`t defined in the devicetree.cb)
Change-Id: Ie3f9362676105e41c69139a094dbb9e8b865689f Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/romstage/romstage_fsp20.c 2 files changed, 32 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/32044/8
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32044 )
Change subject: soc/intel/skylake: Update GFX devtree options ......................................................................
Patch Set 8:
Patch Set 7: Code-Review-1
IMO, this patch does too much at once to keep track of everything...
Open questions:
- Why change the PrimaryDisplay setting of h110m?
- Why would we ever set `PrimaryDisplay = Display_Auto` together with `SkipExtGfxScan`?
- If I didn't miss it, the `SkipExtGfxScan` UPD wasn't set before? So this change changes all boards without mentioning it?
- What does `SkipExtGfxScan` do anyway?
1) "Why change the PrimaryDisplay setting of h110m?" - This board has a PEG port. The Primary_PEG value gives a higher priority to an external graphics card. This means that if I inserted a GPU into this port, then this device outputs information from the coreboot and linux on the display that is connected to this device. But, if there is no external GPU, then another device is used for output, for example, a gfx card connected to the PCH via a PCIe, or an internal iGPU.
In the case when PrimaryDisplay = Auto, I do not know how the priorities in the device sequence are distributed: - iGPU - PEG_GPU - PCH_PCIe_GPU.
Maybe the "Switchable Graphics" mode will be used. I don't know what "Auto" means:)
In case with h110m mb, I want to be sure that the main gfx device is PEG_GPU, as in the AMI BIOS.
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32044 )
Change subject: soc/intel/skylake: Update GFX devtree options ......................................................................
Patch Set 8:
Patch Set 7: Code-Review-1
IMO, this patch does too much at once to keep track of everything...
Open questions:
- Why change the PrimaryDisplay setting of h110m?
- Why would we ever set `PrimaryDisplay = Display_Auto` together with `SkipExtGfxScan`?
- If I didn't miss it, the `SkipExtGfxScan` UPD wasn't set before? So this change changes all boards without mentioning it?
- What does `SkipExtGfxScan` do anyway?
"Why would we ever set PrimaryDisplay = Display_Auto together with SkipExtGfxScan?" -
Sorry. I don`t pay attention to the value "1" for this parameter on intel boards. Matt DeVillier was right.
According to my experiments, SkipExtGfxScan option has higher priority than PrimaryDisplay. Therefore, if PrimaryDisplay = Display_Auto and SkipExtGfxScan = 1, then this is equivalent to the condition PrimaryDisplay = Display_iGFX. This is not a disaster, but it looks bad. I will remove PrimaryDisplay = Display_Auto from intel rvp boards (then the default condition will be used: PrimaryDisplay = Display_iGFX). Thanks you.
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32044 )
Change subject: soc/intel/skylake: Update GFX devtree options ......................................................................
Patch Set 8:
Patch Set 7:
(sorry for the dupe, looks like gerrit pulled in a draft comment from an earlier patch set)
Sorry. I don`t pay attention to the value "1" for this parameter on intel boards. Please ignore previous comment. Since SkipExtGfxScan = 1, only the internal GPU will be used. I will fix this in the new patchset. Therefore, there is no need to use Display_Auto for intel rvp boards. Thanks
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32044 )
Change subject: soc/intel/skylake: Update GFX devtree options ......................................................................
Patch Set 8:
Patch Set 7: Code-Review-1
IMO, this patch does too much at once to keep track of everything...
Open questions:
- Why change the PrimaryDisplay setting of h110m?
- Why would we ever set `PrimaryDisplay = Display_Auto` together with `SkipExtGfxScan`?
- If I didn't miss it, the `SkipExtGfxScan` UPD wasn't set before? So this change changes all boards without mentioning it?
- What does `SkipExtGfxScan` do anyway?
3) "If I didn't miss it, the SkipExtGfxScan UPD wasn't set before? So this change changes all boards without mentioning it?" - This parameter would be defined in the devicetree.cb, but was not set to upd. In this case, the default upd value 0 is used for the option (Disable: Scan external display devices).
Now, if in dt.cb set SkipExtGfxScan=1, we set so that FSP doesn`t scan external gfx devices.
I will add this information to the commit message. (I move these changes to a separate patch Change-Id: Ie88a41bdf31f7c3e88df6c70c82a1cbf866372c4 )
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32044 )
Change subject: soc/intel/skylake: Update GFX devtree options ......................................................................
Patch Set 8:
Patch Set 7: Code-Review-1
IMO, this patch does too much at once to keep track of everything...
Open questions:
- Why change the PrimaryDisplay setting of h110m?
- Why would we ever set `PrimaryDisplay = Display_Auto` together with `SkipExtGfxScan`?
- If I didn't miss it, the `SkipExtGfxScan` UPD wasn't set before? So this change changes all boards without mentioning it?
- What does `SkipExtGfxScan` do anyway?
4) "What does SkipExtGfxScan do anyway?" - If this parameter = 1, then FSP will not scan existing external graphics cards on the pci bus.
At the same time, it is worth noting that some payloads can ignore these parameters. For example, tianocore uses an NVIDIA GPU even if SkipExtGfxScan is set to 1. I think this happens because tianocore initializes the GPU again, it executes the code and registers the handlers from the uefi-rom-extension. In this case, the bootsplash first appears on the display connected to iGFX, and then the linux boot information is displayed on the NVIDIA GPU Display.
seabios uses the configuration from coreboot and doesn`t override the displays.
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32044 )
Change subject: soc/intel/skylake: Update GFX devtree options ......................................................................
Patch Set 8:
Patch Set 7: Code-Review-1
IMO, this patch does too much at once to keep track of everything...
Open questions:
- Why change the PrimaryDisplay setting of h110m?
- Why would we ever set `PrimaryDisplay = Display_Auto` together with `SkipExtGfxScan`?
- If I didn't miss it, the `SkipExtGfxScan` UPD wasn't set before? So this change changes all boards without mentioning it?
- What does `SkipExtGfxScan` do anyway?
Yes, I agree with you about the patch. As a result of all the fixes, it became very large. I reworked it and split it into 4 different patches:
- this patch - "Set PEG as primary GFX device" Change-Id: Iea846179fc309c2b98093de37c05ceb332081f4f - "remove unused InternalGfx" Change-Id: I41ecca3fdfb1d4b20ee634a13263ff481dcf440e - "Update SkipExtGfxScan in UPD from devtree" Change-Id: Ie88a41bdf31f7c3e88df6c70c82a1cbf866372c4
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32044 )
Change subject: soc/intel/skylake: Update GFX devtree options ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/#/c/32044/5/src/soc/intel/skylake/romstage/romst... File src/soc/intel/skylake/romstage/romstage_fsp20.c:
https://review.coreboot.org/#/c/32044/5/src/soc/intel/skylake/romstage/romst... PS5, Line 264: if (CONFIG(ONBOARD_VGA_IS_PRIMARY))
I somehow missed earlier that no matter the Kconfig value, the corresponding […]
I will test the image with this option
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32044 )
Change subject: soc/intel/skylake: Update GFX devtree options ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/#/c/32044/5/src/soc/intel/skylake/romstage/romst... File src/soc/intel/skylake/romstage/romstage_fsp20.c:
https://review.coreboot.org/#/c/32044/5/src/soc/intel/skylake/romstage/romst... PS5, Line 264: if (CONFIG(ONBOARD_VGA_IS_PRIMARY))
I will test the image with this option
I tested the image with ONBOARD_VGA_IS_PRIMARY=y on asrock-h110m-dvs with gfx options:
register "SkipExtGfxScan" = "0" register "PrimaryDisplay" = "Display_PEG" register "Device4Enable" = "1"
- seabios + grub output information at the iGPU display, but linux use the PEG.
- tianocore ignores ONBOARD_VGA_IS_PRIMARY and displays the bootspalsh and menus in PEG_GPU, linux too.
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32044 )
Change subject: soc/intel/skylake: Update GFX devtree options ......................................................................
Patch Set 8:
Do we need to update these options for fsp 1.1 in romstage.c?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32044 )
Change subject: soc/intel/skylake: Update GFX devtree options ......................................................................
Patch Set 8: Code-Review+2
(2 comments)
Please update the commit message before merge.
https://review.coreboot.org/#/c/32044/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32044/8//COMMIT_MSG@15 PS8, Line 15: - SkipExtGfxScan. Not part of this patch anymore?
https://review.coreboot.org/#/c/32044/8/src/soc/intel/skylake/romstage/romst... File src/soc/intel/skylake/romstage/romstage_fsp20.c:
https://review.coreboot.org/#/c/32044/8/src/soc/intel/skylake/romstage/romst... PS8, Line 258: if (config->PrimaryDisplay == Display_iGFX) : m_cfg->PrimaryDisplay = Display_Auto; : else Is this really necessary? If I understand your findings correctly, FSP would fall back by itself if the "primary adapter" doesn't exist?
Hello Patrick Rudolph, Angel Pons, Youness Alaoui, Matt DeVillier, Duncan Laurie, Paul Menzel, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, caveh jalali, Gaggery Tsai, Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32044
to look at the new patch set (#9).
Change subject: soc/intel/skylake: Update GFX devtree options ......................................................................
soc/intel/skylake: Update GFX devtree options
This patch includes the following changes:
1. Sets FSP options in romstage_fsp20.c to select primary GPU. List of options: - InternalGfx, - PrimaryDisplay.
2. iGPU will be initialized if the corresponding PCI device is defined in the device tree as:
device pci 02.0 on end
In this case, it is not necessary to set the InternalGfx option to enable this device
3. Primary_iGFX is used as the default value for all skl/kbl boards (since the PrimaryDisplay option isn`t defined in the devicetree.cb)
Change-Id: Ie3f9362676105e41c69139a094dbb9e8b865689f Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/romstage/romstage_fsp20.c 2 files changed, 32 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/32044/9
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32044 )
Change subject: soc/intel/skylake: Update GFX devtree options ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/#/c/32044/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32044/8//COMMIT_MSG@15 PS8, Line 15: - SkipExtGfxScan.
Not part of this patch anymore?
Removed
Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32044 )
Change subject: soc/intel/skylake: Update GFX devtree options ......................................................................
soc/intel/skylake: Update GFX devtree options
This patch includes the following changes:
1. Sets FSP options in romstage_fsp20.c to select primary GPU. List of options: - InternalGfx, - PrimaryDisplay.
2. iGPU will be initialized if the corresponding PCI device is defined in the device tree as:
device pci 02.0 on end
In this case, it is not necessary to set the InternalGfx option to enable this device
3. Primary_iGFX is used as the default value for all skl/kbl boards (since the PrimaryDisplay option isn`t defined in the devicetree.cb)
Change-Id: Ie3f9362676105e41c69139a094dbb9e8b865689f Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/32044 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Nico Huber nico.h@gmx.de --- M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/romstage/romstage_fsp20.c 2 files changed, 32 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved
diff --git a/src/soc/intel/skylake/chip.h b/src/soc/intel/skylake/chip.h index a8ee064..f87a811 100644 --- a/src/soc/intel/skylake/chip.h +++ b/src/soc/intel/skylake/chip.h @@ -311,7 +311,13 @@
/* Gfx related */ u8 IgdDvmt50PreAlloc; - u8 PrimaryDisplay; + enum { + Display_iGFX, + Display_PEG, + Display_PCH_PCIe, + Display_Auto, + Display_Switchable, + } PrimaryDisplay; u8 InternalGfx; u8 ApertureSize; u8 SkipExtGfxScan; diff --git a/src/soc/intel/skylake/romstage/romstage_fsp20.c b/src/soc/intel/skylake/romstage/romstage_fsp20.c index 43ba9c9..b65c9ff 100644 --- a/src/soc/intel/skylake/romstage/romstage_fsp20.c +++ b/src/soc/intel/skylake/romstage/romstage_fsp20.c @@ -243,6 +243,28 @@ } }
+static void soc_primary_gfx_config_params(FSP_M_CONFIG *m_cfg, + const struct soc_intel_skylake_config *config) +{ + const struct device *dev; + + dev = dev_find_slot(0, SA_DEVFN_IGD); + if (!dev || !dev->enabled) { + /* + * If iGPU is disabled or not defined in the devicetree.cb, + * the FSP does not initialize this device + */ + m_cfg->InternalGfx = 0; + if (config->PrimaryDisplay == Display_iGFX) + m_cfg->PrimaryDisplay = Display_Auto; + else + m_cfg->PrimaryDisplay = config->PrimaryDisplay; + } else { + m_cfg->InternalGfx = 1; + m_cfg->PrimaryDisplay = config->PrimaryDisplay; + } +} + void platform_fsp_memory_init_params_cb(FSPM_UPD *mupd, uint32_t version) { const struct device *dev; @@ -274,6 +296,9 @@ /* Enable SMBus controller based on config */ m_cfg->SmbusEnable = config->SmbusEnable;
+ /* Set primary graphic device */ + soc_primary_gfx_config_params(m_cfg, config); + mainboard_memory_init_params(mupd); }
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32044 )
Change subject: soc/intel/skylake: Update GFX devtree options ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/#/c/32044/8/src/soc/intel/skylake/romstage/romst... File src/soc/intel/skylake/romstage/romstage_fsp20.c:
https://review.coreboot.org/#/c/32044/8/src/soc/intel/skylake/romstage/romst... PS8, Line 258: if (config->PrimaryDisplay == Display_iGFX) : m_cfg->PrimaryDisplay = Display_Auto; : else
Is this really necessary? If I understand your findings correctly, […]
I think it is better not to use iGPU as a primary graphic device when it is turned off. And if this happened, then the universal solution to this problem would be to use the “Auto” mode.
For example, in the future, I can add this check here: If PEG is enabled, then m_cfg->PrimaryDisplay = Primary_PEG, otherwise, m_cfg-> PrimaryDisplay = PCIe_PCH.
Or something like that...
-
Furquan Shaikh (Code Review) -
Matt DeVillier (Code Review)
-
Maxim Polyakov (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)