Felix Singer has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44369 )
Change subject: soc/intel/skylake: Factor out unnecessary if-else-block ......................................................................
soc/intel/skylake: Factor out unnecessary if-else-block
Move InternalGfx config option out of the if-else-block and replace the left over config option IgdDvmt50PreAlloc by a ternary expression and adjust related code comments.
This changes the logic of the code, since InternalGfx is configured first and IgdDvmt50PreAlloc depends on its value. The negation in the ternary expression is removed to improve the readability.
Change-Id: I89ff17f4574a7ade228c1791f17ea072fb731775 Signed-off-by: Felix Singer felixsinger@posteo.net --- M src/soc/intel/skylake/romstage/romstage.c 1 file changed, 14 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/44369/1
diff --git a/src/soc/intel/skylake/romstage/romstage.c b/src/soc/intel/skylake/romstage/romstage.c index da5ec9e..cefe742 100644 --- a/src/soc/intel/skylake/romstage/romstage.c +++ b/src/soc/intel/skylake/romstage/romstage.c @@ -241,25 +241,20 @@ const struct device *dev;
dev = pcidev_path_on_root(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; - m_cfg->IgdDvmt50PreAlloc = 0; - } else { - m_cfg->InternalGfx = 1; - /* - * Set IGD stolen size to 64MB. The FBC hardware for skylake - * does not have access to the bios_reserved range so it always - * assumes 8MB is used and so the kernel will avoid the last - * 8MB of the stolen window. With the default stolen size of - * 32MB(-8MB) there is not enough space for FBC to work with - * a high resolution panel - */ - m_cfg->IgdDvmt50PreAlloc = 2; - } + m_cfg->InternalGfx = dev && dev->enabled; + + /* + * If iGPU is enabled, set IGD stolen size to 64MB. The FBC + * hardware for skylake does not have access to the bios + * reserved range so it always assumes 8MB is used and so the + * kernel will avoid the last 8MB of the stolen window. With + * the default stolen size of 32MB(-8MB) there is not enough + * space for FBC to work with a high resolution panel. + * + * If disabled, don't reserve memory for it. + */ + m_cfg->IgdDvmt50PreAlloc = m_cfg->InternalGfx ? 2 : 0; + m_cfg->PrimaryDisplay = config->PrimaryDisplay; }
Hello Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44369
to look at the new patch set (#2).
Change subject: soc/intel/skylake: Factor out unnecessary if-else-block ......................................................................
soc/intel/skylake: Factor out unnecessary if-else-block
Move InternalGfx config option out of the if-else-block and replace the left over config option IgdDvmt50PreAlloc by a ternary expression. Also, adjust related code comments to fit the new logic of this code.
This changes the logic of the code, since InternalGfx is configured first and IgdDvmt50PreAlloc depends on its value. The negation in the ternary expression is removed to improve the readability.
Change-Id: I89ff17f4574a7ade228c1791f17ea072fb731775 Signed-off-by: Felix Singer felixsinger@posteo.net --- M src/soc/intel/skylake/romstage/romstage.c 1 file changed, 14 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/44369/2
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44369 )
Change subject: soc/intel/skylake: Factor out unnecessary if-else-block ......................................................................
Patch Set 2: Code-Review+1
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44369 )
Change subject: soc/intel/skylake: Factor out unnecessary if-else-block ......................................................................
Patch Set 5: Code-Review+2
Michael Niewöhner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44369 )
Change subject: soc/intel/skylake: Factor out unnecessary if-else-block ......................................................................
soc/intel/skylake: Factor out unnecessary if-else-block
Move InternalGfx config option out of the if-else-block and replace the left over config option IgdDvmt50PreAlloc by a ternary expression. Also, adjust related code comments to fit the new logic of this code.
This changes the logic of the code, since InternalGfx is configured first and IgdDvmt50PreAlloc depends on its value. The negation in the ternary expression is removed to improve the readability.
Change-Id: I89ff17f4574a7ade228c1791f17ea072fb731775 Signed-off-by: Felix Singer felixsinger@posteo.net Reviewed-on: https://review.coreboot.org/c/coreboot/+/44369 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Michael Niewöhner --- M src/soc/intel/skylake/romstage/romstage.c 1 file changed, 14 insertions(+), 19 deletions(-)
Approvals: build bot (Jenkins): Verified Michael Niewöhner: Looks good to me, approved
diff --git a/src/soc/intel/skylake/romstage/romstage.c b/src/soc/intel/skylake/romstage/romstage.c index 7410925..9add1e6 100644 --- a/src/soc/intel/skylake/romstage/romstage.c +++ b/src/soc/intel/skylake/romstage/romstage.c @@ -247,25 +247,20 @@ const struct device *dev;
dev = pcidev_path_on_root(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; - m_cfg->IgdDvmt50PreAlloc = 0; - } else { - m_cfg->InternalGfx = 1; - /* - * Set IGD stolen size to 64MB. The FBC hardware for skylake - * does not have access to the bios_reserved range so it always - * assumes 8MB is used and so the kernel will avoid the last - * 8MB of the stolen window. With the default stolen size of - * 32MB(-8MB) there is not enough space for FBC to work with - * a high resolution panel - */ - m_cfg->IgdDvmt50PreAlloc = 2; - } + m_cfg->InternalGfx = dev && dev->enabled; + + /* + * If iGPU is enabled, set IGD stolen size to 64MB. The FBC + * hardware for skylake does not have access to the bios + * reserved range so it always assumes 8MB is used and so the + * kernel will avoid the last 8MB of the stolen window. With + * the default stolen size of 32MB(-8MB) there is not enough + * space for FBC to work with a high resolution panel. + * + * If disabled, don't reserve memory for it. + */ + m_cfg->IgdDvmt50PreAlloc = m_cfg->InternalGfx ? 2 : 0; + m_cfg->PrimaryDisplay = config->PrimaryDisplay; }
-
Felix Singer (Code Review) -
Michael Niewöhner (Code Review)