Change in coreboot[master]: device: Select VGA_ROM_RUN for non-FSP solution with PCIE DGPU
Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/49016 )
Change subject: device: Select VGA_ROM_RUN for non-FSP solution with PCIE DGPU ......................................................................
device: Select VGA_ROM_RUN for non-FSP solution with PCIE DGPU
For onboard graphics initialization, GFX PEIM module inside FSP binary is taking care of graphics initialization based on HAVE_FSP_GOP Kconfig option and input VBT file. Hence no need to load/execute legacy VGA OpROM in order to initialize onboard GFX.
In case of non-FSP solution, platform need to select VGA_ROM_RUN Kconfig to perform graphics initialization for PCI-E based discrete card through VGA OpRom (SoC or Mainboard user can't select VGA_ROM_RUN directly because it's part of choice option).
TEST=Able to get Pre-OS splash screen with PCI-E DGPU when SoC user doesn't select HAVE_FSP_GOP.
Change-Id: Iecb2fcdb105af449bc20ad727759cdef17d5e376 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/device/Kconfig 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/49016/1
diff --git a/src/device/Kconfig b/src/device/Kconfig index a472a6a..b93330c 100644 --- a/src/device/Kconfig +++ b/src/device/Kconfig @@ -81,7 +81,7 @@ # TODO: Explain differences (if any) for onboard cards. config VGA_ROM_RUN bool "Run VGA Option ROMs" - depends on PCI && (ARCH_X86 || ARCH_PPC64) && !MAINBOARD_FORCE_NATIVE_VGA_INIT + depends on PCI && (ARCH_X86 || ARCH_PPC64) && !HAVE_FSP_GOP && !MAINBOARD_FORCE_NATIVE_VGA_INIT select HAVE_VGA_TEXT_FRAMEBUFFER help Execute VGA Option ROMs in coreboot if found. This can be used
Hello build bot (Jenkins), Furquan Shaikh, Stefan Reinauer, Tim Wawrzynczak, Duncan Laurie, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/49016
to look at the new patch set (#2).
Change subject: device: Select VGA_ROM_RUN for non-FSP solution with PCIE DGPU ......................................................................
device: Select VGA_ROM_RUN for non-FSP solution with PCIE DGPU
For onboard graphics initialization, GFX PEIM module inside FSP binary is taking care of graphics initialization based on HAVE_FSP_GOP Kconfig option and input VBT file. Hence no need to load/execute legacy VGA OpROM in order to initialize onboard GFX.
In case of non-FSP solution, platform need to select VGA_ROM_RUN Kconfig to perform graphics initialization for PCI-E based discrete card through VGA OpRom (SoC or Mainboard user can't select VGA_ROM_RUN directly because it's part of choice option).
Create MAINBOARD_HAS_DGPU_VGA_INIT Kconfig mainboard to select design with DGPU.
TEST=Able to get Pre-OS splash screen with PCI-E DGPU when mainboard user select MAINBOARD_HAS_DGPU_VGA_INIT.
Change-Id: Iecb2fcdb105af449bc20ad727759cdef17d5e376 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/device/Kconfig 1 file changed, 7 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/49016/2
Hello build bot (Jenkins), Furquan Shaikh, Stefan Reinauer, Tim Wawrzynczak, Duncan Laurie, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/49016
to look at the new patch set (#3).
Change subject: device: Select VGA_ROM_RUN for non-FSP solution with PCIE DGPU ......................................................................
device: Select VGA_ROM_RUN for non-FSP solution with PCIE DGPU
For onboard graphics initialization, GFX PEIM module inside FSP binary is taking care of graphics initialization based on HAVE_FSP_GOP Kconfig option and input VBT file. Hence no need to load/execute legacy VGA OpROM in order to initialize onboard GFX.
In case of non-FSP solution, platform need to select VGA_ROM_RUN Kconfig to perform graphics initialization for PCI-E based discrete card through VGA OpRom (SoC or Mainboard user can't select VGA_ROM_RUN directly because it's part of choice option).
Create MAINBOARD_HAS_DGPU_VGA_INIT Kconfig for mainboard to select design with DGPU where OpROM is embedded inside the DGPU card.
TEST=Able to get Pre-OS splash screen with PCI-E DGPU when mainboard user select MAINBOARD_HAS_DGPU_VGA_INIT.
Change-Id: Iecb2fcdb105af449bc20ad727759cdef17d5e376 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/device/Kconfig 1 file changed, 8 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/49016/3
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49016 )
Change subject: device: Select VGA_ROM_RUN for non-FSP solution with PCIE DGPU ......................................................................
Patch Set 3:
Is this for mainboards that have both integrated and discrete graphics onboard? or only discrete graphics?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49016 )
Change subject: device: Select VGA_ROM_RUN for non-FSP solution with PCIE DGPU ......................................................................
Patch Set 3:
Patch Set 3:
Is this for mainboards that have both integrated and discrete graphics onboard? or only discrete graphics?
Very nice question Nico, i was about to come to this disucssion. We are looking for two mainboard design
1. only discrete GFX over PCIE (no IGD)=> This i'm could able to achieve easily using MAINBOARD_HAS_DGPU_VGA_INIT and MAINBOARD_NO_FSP_GOP Kconfig select from mainboard.
2. Both onboard (IGD) and discrete GFX (DGPU) and based on some switch change the display dynamically => This is problem at present as RUN_FSP_GOP and VGA_ROM_RUN both are part of choice option hence couldn't able to select both from mainboard. Ideally for this type of mainboard design, we need to have both RUN_FSP_GOP and VGA_ROM_RUN enable in Kconfig and coreboot decide based on switch position if like to call FSP for IGD display or load/execute VGA OpROM for DGPU init. Right now, i'm commenting choice option in my local repo and selecting both Kconfig to make this dynamic display possible.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49016 )
Change subject: device: Select VGA_ROM_RUN for non-FSP solution with PCIE DGPU ......................................................................
Patch Set 3:
Is this for mainboards that have both integrated and discrete graphics onboard? or only discrete graphics?
Very nice question Nico, i was about to come to this disucssion. We are looking for two mainboard design
- only discrete GFX over PCIE (no IGD)=> This i'm could able to achieve easily using MAINBOARD_HAS_DGPU_VGA_INIT and MAINBOARD_NO_FSP_GOP Kconfig select from mainboard.
I guess MAINBOARD_HAS_DGPU_VGA_INIT is not even needed in this case because VGA_ROM_RUN would be the only option.
- Both onboard (IGD) and discrete GFX (DGPU) and based on some switch change the display dynamically => This is problem at present as RUN_FSP_GOP and VGA_ROM_RUN both are part of choice option hence couldn't able to select both from mainboard. Ideally for this type of mainboard design, we need to have both RUN_FSP_GOP and VGA_ROM_RUN enable in Kconfig and coreboot decide based on switch position if like to call FSP for IGD display or load/execute VGA OpROM for DGPU init. Right now, i'm commenting choice option in my local repo and selecting both Kconfig to make this dynamic display possible.
The choice is a bit obsolete now. Originally, we could not select multiple options because of linking errors. Now, since Patrick's topic:framebuffer_info, we can actually build multiple drivers for grapchics initialization at once.
Maybe it's time to get rid of the choice? If multiple drivers are run, we'd end up with multiple framebuffers. But we could add some more Kconfigs, e.g. "Only initialize primary graphics adapter". If, in your case, the switch disables either GPU, the other would automatically be primary I guess.
(There should still be a choice if we have multiple drivers for a single GPU, though, e.g. libgfxinit vs. the unnecessary FSP/GOP blob).
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49016 )
Change subject: device: Select VGA_ROM_RUN for non-FSP solution with PCIE DGPU ......................................................................
Patch Set 3:
Patch Set 3:
Is this for mainboards that have both integrated and discrete graphics onboard? or only discrete graphics?
Very nice question Nico, i was about to come to this disucssion. We are looking for two mainboard design
- only discrete GFX over PCIE (no IGD)=> This i'm could able to achieve easily using MAINBOARD_HAS_DGPU_VGA_INIT and MAINBOARD_NO_FSP_GOP Kconfig select from mainboard.
I guess MAINBOARD_HAS_DGPU_VGA_INIT is not even needed in this case because VGA_ROM_RUN would be the only option.
Yes, if VGA_ROM_RUN is outside choice then we could select for case #1 here directly from mb kconfig and we don't need any new kconfig option like this.
- Both onboard (IGD) and discrete GFX (DGPU) and based on some switch change the display dynamically => This is problem at present as RUN_FSP_GOP and VGA_ROM_RUN both are part of choice option hence couldn't able to select both from mainboard. Ideally for this type of mainboard design, we need to have both RUN_FSP_GOP and VGA_ROM_RUN enable in Kconfig and coreboot decide based on switch position if like to call FSP for IGD display or load/execute VGA OpROM for DGPU init. Right now, i'm commenting choice option in my local repo and selecting both Kconfig to make this dynamic display possible.
The choice is a bit obsolete now. Originally, we could not select multiple options because of linking errors. Now, since Patrick's topic:framebuffer_info, we can actually build multiple drivers for grapchics initialization at once.
Maybe it's time to get rid of the choice?
yes, i think that is the need for now, because board design need more option.
If multiple drivers are run, we'd end up with multiple framebuffers. But we could add some more Kconfigs, e.g. "Only initialize primary graphics adapter". If, in your case, the switch disables either GPU, the other would automatically be primary I guess.
yes, default primary and HW switch makes IGD disable and DGPU enable via OpRom
(There should still be a choice if we have multiple drivers for a single GPU, though, e.g. libgfxinit vs. the unnecessary FSP/GOP blob).
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49016 )
Change subject: device: Select VGA_ROM_RUN for non-FSP solution with PCIE DGPU ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 3:
Is this for mainboards that have both integrated and discrete graphics onboard? or only discrete graphics?
Very nice question Nico, i was about to come to this disucssion. We are looking for two mainboard design
- only discrete GFX over PCIE (no IGD)=> This i'm could able to achieve easily using MAINBOARD_HAS_DGPU_VGA_INIT and MAINBOARD_NO_FSP_GOP Kconfig select from mainboard.
I guess MAINBOARD_HAS_DGPU_VGA_INIT is not even needed in this case because VGA_ROM_RUN would be the only option.
I have laptops which only have a dGPU, so the only option is to run the VBIOS (or not, e.g. when using SeaBIOS). The iGPU PCI device is disabled in the devicetree.
Yes, if VGA_ROM_RUN is outside choice then we could select for case #1 here directly from mb kconfig and we don't need any new kconfig option like this.
- Both onboard (IGD) and discrete GFX (DGPU) and based on some switch change the display dynamically => This is problem at present as RUN_FSP_GOP and VGA_ROM_RUN both are part of choice option hence couldn't able to select both from mainboard. Ideally for this type of mainboard design, we need to have both RUN_FSP_GOP and VGA_ROM_RUN enable in Kconfig and coreboot decide based on switch position if like to call FSP for IGD display or load/execute VGA OpROM for DGPU init. Right now, i'm commenting choice option in my local repo and selecting both Kconfig to make this dynamic display possible.
The choice is a bit obsolete now. Originally, we could not select multiple options because of linking errors. Now, since Patrick's topic:framebuffer_info, we can actually build multiple drivers for grapchics initialization at once.
Maybe it's time to get rid of the choice?
yes, i think that is the need for now, because board design need more option.
One example of this case is a desktop mainboard. To choose a primary video adapter, the usual behavior is to use a dGPU if present, otherwise fall back to the iGPU. The `if present` part could easily be determined after coreboot does PCI enumeration, but FSP-S gets executed before that point... I know some blobs (Haswell MRC.bin) do some sort of PCI enumeration in romstage to detect whether a dGPU is present, but IMHO it's a rather crude approach. Still, it's better than nothing. Any other ideas are welcome.
Another thing to consider: we need to keep the functionality of the ONBOARD_VGA_IS_PRIMARY Kconfig setting. This is useful when a dGPU is present but coreboot should still use the iGPU as primary video adapter. On laptops with Nvidia Optimus or the AMD counterpart (AMD Enduro, IIRC), this option needs to be enabled, and should be selected from the mainboard's Kconfig. This case is easy to handle: ensure the iGPU is to be enabled, and always use it as primary video adapter. It shouldn't be necessary to run the VBIOS of the dGPU, but I have no idea.
If multiple drivers are run, we'd end up with multiple framebuffers. But we could add some more Kconfigs, e.g. "Only initialize primary graphics adapter". If, in your case, the switch disables either GPU, the other would automatically be primary I guess.
yes, default primary and HW switch makes IGD disable and DGPU enable via OpRom
(There should still be a choice if we have multiple drivers for a single GPU, though, e.g. libgfxinit vs. the unnecessary FSP/GOP blob).
I agree. I feel the easiest approach is to add a new option for dGPU lightup, as the only choice is whether to have coreboot run the VBIOS or not. However, I fear we may need to touch Kconfig on all mainboards to only show the valid options for a given board. It's work, but it shouldn't be too hard. For example, `MAINBOARD_HAS_LIBGFXINIT` indicates the iGPU is usable.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49016 )
Change subject: device: Select VGA_ROM_RUN for non-FSP solution with PCIE DGPU ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 3:
Patch Set 3:
Is this for mainboards that have both integrated and discrete graphics onboard? or only discrete graphics?
Very nice question Nico, i was about to come to this disucssion. We are looking for two mainboard design
- only discrete GFX over PCIE (no IGD)=> This i'm could able to achieve easily using MAINBOARD_HAS_DGPU_VGA_INIT and MAINBOARD_NO_FSP_GOP Kconfig select from mainboard.
I guess MAINBOARD_HAS_DGPU_VGA_INIT is not even needed in this case because VGA_ROM_RUN would be the only option.
I have laptops which only have a dGPU, so the only option is to run the VBIOS (or not, e.g. when using SeaBIOS). The iGPU PCI device is disabled in the devicetree.
Problem with our VGA_BIOS Kconfig is that, it expects to have VBIOS as part of coreboot rather probing PCIE card and load/execute VBIOS from there. Hence thought is adding a new Kconfig which will make things clear that mb has DGPU and VBIOS is inside that. coreboot don't need to pack VBIOS.
And for disabling IGD, we have FSP UPD to make IGD disable prior to PCI enumeration hence we can ignore devicetree PCI device if needed.
Yes, if VGA_ROM_RUN is outside choice then we could select for case #1 here directly from mb kconfig and we don't need any new kconfig option like this.
- Both onboard (IGD) and discrete GFX (DGPU) and based on some switch change the display dynamically => This is problem at present as RUN_FSP_GOP and VGA_ROM_RUN both are part of choice option hence couldn't able to select both from mainboard. Ideally for this type of mainboard design, we need to have both RUN_FSP_GOP and VGA_ROM_RUN enable in Kconfig and coreboot decide based on switch position if like to call FSP for IGD display or load/execute VGA OpROM for DGPU init. Right now, i'm commenting choice option in my local repo and selecting both Kconfig to make this dynamic display possible.
The choice is a bit obsolete now. Originally, we could not select multiple options because of linking errors. Now, since Patrick's topic:framebuffer_info, we can actually build multiple drivers for grapchics initialization at once.
Maybe it's time to get rid of the choice?
yes, i think that is the need for now, because board design need more option.
One example of this case is a desktop mainboard. To choose a primary video adapter, the usual behavior is to use a dGPU if present, otherwise fall back to the iGPU. The `if present` part could easily be determined after coreboot does PCI enumeration, but FSP-S gets executed before that point... I know some blobs (Haswell MRC.bin) do some sort of PCI enumeration in romstage to detect whether a dGPU is present, but IMHO it's a rather crude approach. Still, it's better than nothing. Any other ideas are welcome.
Another thing to consider: we need to keep the functionality of the ONBOARD_VGA_IS_PRIMARY Kconfig setting. This is useful when a dGPU is present but coreboot should still use the iGPU as primary video adapter. On laptops with Nvidia Optimus or the AMD counterpart (AMD Enduro, IIRC), this option needs to be enabled, and should be selected from the mainboard's Kconfig. This case is easy to handle: ensure the iGPU is to be enabled, and always use it as primary video adapter. It shouldn't be necessary to run the VBIOS of the dGPU, but I have no idea.
If multiple drivers are run, we'd end up with multiple framebuffers. But we could add some more Kconfigs, e.g. "Only initialize primary graphics adapter". If, in your case, the switch disables either GPU, the other would automatically be primary I guess.
yes, default primary and HW switch makes IGD disable and DGPU enable via OpRom
(There should still be a choice if we have multiple drivers for a single GPU, though, e.g. libgfxinit vs. the unnecessary FSP/GOP blob).
I agree. I feel the easiest approach is to add a new option for dGPU lightup, as the only choice is whether to have coreboot run the VBIOS or not. However, I fear we may need to touch Kconfig on all mainboards to only show the valid options for a given board. It's work, but it shouldn't be too hard. For example, `MAINBOARD_HAS_LIBGFXINIT` indicates the iGPU is usable.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49016 )
Change subject: device: Select VGA_ROM_RUN for non-FSP solution with PCIE DGPU ......................................................................
Patch Set 3:
Review please
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49016 )
Change subject: device: Select VGA_ROM_RUN for non-FSP solution with PCIE DGPU ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/49016/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/49016/3//COMMIT_MSG@19 PS3, Line 19: Create MAINBOARD_HAS_DGPU_VGA_INIT Kconfig for mainboard to select design : with DGPU where OpROM is embedded inside the DGPU card. : : TEST=Able to get Pre-OS splash screen with PCI-E DGPU when mainboard user Overlong lines.
https://review.coreboot.org/c/coreboot/+/49016/3/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/49016/3/src/device/Kconfig@49 PS3, Line 49: config MAINBOARD_HAS_DGPU_VGA_INIT DGPU doesn't always imply that we need to run a VGA OpROM (we have native coreboot drivers for some discrete chips (e.g. ASpeed), and I hope for DG1 we are able to extend libgfxinit; I have lower hopes for AMD and Nvidia chips).
Maybe name this simply by its intended effect, e.g. `VGA_ROM_RUN_DEFAULT`?
https://review.coreboot.org/c/coreboot/+/49016/3/src/device/Kconfig@64 PS3, Line 64: PAYLOAD_SEABIOS This is supposed to guard us from a wrong VGA_ROM_RUN default in case SeaBIOS is used (it would run the VBIOS again). I guess this needs to be updated to
default NO_GFX_INIT if (VGA_BIOS || MAINBOARD_HAS_DGPU_VGA_INIT) && PAYLOAD_SEABIOS
Hello build bot (Jenkins), Furquan Shaikh, Stefan Reinauer, Tim Wawrzynczak, Duncan Laurie, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/49016
to look at the new patch set (#4).
Change subject: device: Select VGA_ROM_RUN for non-FSP solution with PCIE DGPU ......................................................................
device: Select VGA_ROM_RUN for non-FSP solution with PCIE DGPU
For onboard graphics initialization, GFX PEIM module inside FSP binary is taking care of graphics initialization based on HAVE_FSP_GOP Kconfig option and input VBT file. Hence no need to load/execute legacy VGA OpROM in order to initialize onboard GFX.
In case of non-FSP solution, platform need to select VGA_ROM_RUN Kconfig to perform graphics initialization for PCI-E based discrete card through VGA OpRom (SoC or Mainboard user can't select VGA_ROM_RUN directly because it's part of choice option).
Create VGA_ROM_RUN_DEFAULT Kconfig for mainboard to select design with DGPU where OpROM is embedded inside the DGPU card.
Also updated NO_GFX_INIT Kconfig to avoid running VGA_ROM_RUN by default in case SeaBIOS is used.
TEST=Able to get Pre-OS splash screen with PCI-E DGPU when mainboard user select VGA_ROM_RUN_DEFAULT.
Change-Id: Iecb2fcdb105af449bc20ad727759cdef17d5e376 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/device/Kconfig 1 file changed, 9 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/49016/4
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49016 )
Change subject: device: Select VGA_ROM_RUN for non-FSP solution with PCIE DGPU ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/49016/3/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/49016/3/src/device/Kconfig@49 PS3, Line 49: config MAINBOARD_HAS_DGPU_VGA_INIT
DGPU doesn't always imply that we need to run a VGA OpROM (we have native coreboot drivers for some discrete chips (e.g. ASpeed), and I hope for DG1 we are able to extend libgfxinit;
I'm also looking for DG enabling using libgfxinit, i'm up for POC.
I have lower hopes for AMD and Nvidia chips).
Maybe name this simply by its intended effect, e.g. `VGA_ROM_RUN_DEFAULT`?
Done
https://review.coreboot.org/c/coreboot/+/49016/3/src/device/Kconfig@64 PS3, Line 64: PAYLOAD_SEABIOS
This is supposed to guard us from a wrong VGA_ROM_RUN default in […]
Ack
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49016 )
Change subject: device: Select VGA_ROM_RUN for non-FSP solution with PCIE DGPU ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/49016/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/49016/3//COMMIT_MSG@19 PS3, Line 19: Create MAINBOARD_HAS_DGPU_VGA_INIT Kconfig for mainboard to select design : with DGPU where OpROM is embedded inside the DGPU card. : : TEST=Able to get Pre-OS splash screen with PCI-E DGPU when mainboard user
Overlong lines.
Ack
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49016 )
Change subject: device: Select VGA_ROM_RUN for non-FSP solution with PCIE DGPU ......................................................................
Patch Set 4:
(8 comments)
Are we talking about pluggable discrete cards, or discrete chips soldered on the board?
https://review.coreboot.org/c/coreboot/+/49016/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/49016/4//COMMIT_MSG@9 PS4, Line 9: For onboard graphics initialization It’d be great, if you could be more specific to add, it’s about graphics in the payload (or OS without graphics drivers).
https://review.coreboot.org/c/coreboot/+/49016/4//COMMIT_MSG@14 PS4, Line 14: platform need to needs
https://review.coreboot.org/c/coreboot/+/49016/4//COMMIT_MSG@14 PS4, Line 14: In case of non-FSP solution, platform need to select VGA_ROM_RUN : Kconfig to perform graphics initialization for PCI-E based discrete : card through VGA OpRom (SoC or Mainboard user can't select VGA_ROM_RUN : directly because it's part of choice option). Some payloads, like SeaBIOS, are also able to run Option ROMs, so coreboot does not need to (and should not) execute then. (I see below, you reference SeaBIOS, but it would be great to rephrase this to avoid confusion.)
https://review.coreboot.org/c/coreboot/+/49016/4//COMMIT_MSG@22 PS4, Line 22: updated update
https://review.coreboot.org/c/coreboot/+/49016/4//COMMIT_MSG@25 PS4, Line 25: PCI-E DGPU As coreboot sometimes has problems executing Option ROMs, please say exactly which card and what Video BIOS Option ROM version.
https://review.coreboot.org/c/coreboot/+/49016/4//COMMIT_MSG@26 PS4, Line 26: select selects
https://review.coreboot.org/c/coreboot/+/49016/4/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/49016/4/src/device/Kconfig@53 PS4, Line 53: discrete graphics card Is it really only related to discrete graphics cards?
https://review.coreboot.org/c/coreboot/+/49016/4/src/device/Kconfig@53 PS4, Line 53: need needs
Hello build bot (Jenkins), Furquan Shaikh, Stefan Reinauer, Tim Wawrzynczak, Duncan Laurie, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/49016
to look at the new patch set (#5).
Change subject: device: Select VGA_ROM_RUN for non-FSP solution with PCIE DGPU ......................................................................
device: Select VGA_ROM_RUN for non-FSP solution with PCIE DGPU
There are several options to have graphics in the payload (or OS without dedicated GFX driver)
1. For onboard graphics initialization, GFX PEIM module inside FSP binary is taking care of graphics initialization based on HAVE_FSP_GOP Kconfig option and input VBT file. Hence no need to load/execute legacy VGA OpROM in order to initialize onboard GFX.
2. In case of non-FSP solution, platform needs to select VGA_ROM_RUN Kconfig to perform graphics initialization for PCI-E based discrete card through VGA OpRom (SoC or Mainboard user can't select VGA_ROM_RUN directly because it's part of choice option).
(Note: Some payloads, like SeaBIOS, are also able to run Option ROMs, so coreboot does not need to consider option #2 above)
For payload like depthcharge, create VGA_ROM_RUN_DEFAULT Kconfig for mainboard to select design with DGPU where OpROM is embedded inside the DGPU card.
Also update NO_GFX_INIT Kconfig to avoid running VGA_ROM_RUN by default in case SeaBIOS is used.
TEST=Able to get Pre-OS splash screen with AMD Radeon RX 5700 PCI-E DGPU when mainboard user selects VGA_ROM_RUN_DEFAULT.
Change-Id: Iecb2fcdb105af449bc20ad727759cdef17d5e376 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/device/Kconfig 1 file changed, 9 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/49016/5
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49016 )
Change subject: device: Select VGA_ROM_RUN for non-FSP solution with PCIE DGPU ......................................................................
Patch Set 5:
(8 comments)
https://review.coreboot.org/c/coreboot/+/49016/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/49016/4//COMMIT_MSG@9 PS4, Line 9: For onboard graphics initialization
It’d be great, if you could be more specific to add, it’s about graphics in the payload (or OS witho […]
Ack
https://review.coreboot.org/c/coreboot/+/49016/4//COMMIT_MSG@14 PS4, Line 14: platform need to
needs
Ack
https://review.coreboot.org/c/coreboot/+/49016/4//COMMIT_MSG@14 PS4, Line 14: In case of non-FSP solution, platform need to select VGA_ROM_RUN : Kconfig to perform graphics initialization for PCI-E based discrete : card through VGA OpRom (SoC or Mainboard user can't select VGA_ROM_RUN : directly because it's part of choice option).
Some payloads, like SeaBIOS, are also able to run Option ROMs, so coreboot does not need to (and sho […]
Ack
https://review.coreboot.org/c/coreboot/+/49016/4//COMMIT_MSG@22 PS4, Line 22: updated
update
Ack
https://review.coreboot.org/c/coreboot/+/49016/4//COMMIT_MSG@25 PS4, Line 25: PCI-E DGPU
As coreboot sometimes has problems executing Option ROMs, please say exactly which card and what Vid […]
Ack
https://review.coreboot.org/c/coreboot/+/49016/4//COMMIT_MSG@26 PS4, Line 26: select
selects
Ack
https://review.coreboot.org/c/coreboot/+/49016/4/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/49016/4/src/device/Kconfig@53 PS4, Line 53: discrete graphics card
Is it really only related to discrete graphics cards?
got your point, this could be both discrete and onboard GFX, but do ppl really use oprom days for onboard GFX i mean integrated GFX, The purpose of this CL is to ensure VGA_ROM_RUN can run default when it need the most.
Do you suggest to remove "discrete" work explicitly and cover a case where oprom can be use for integrated gfx as well ? i believe for other case we already have VGA_BIOS Kconfig hence this is only for DGPU.
https://review.coreboot.org/c/coreboot/+/49016/4/src/device/Kconfig@53 PS4, Line 53: need
needs
Ack
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49016 )
Change subject: device: Select VGA_ROM_RUN for non-FSP solution with PCIE DGPU ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/49016/4/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/49016/4/src/device/Kconfig@53 PS4, Line 53: discrete graphics card
got your point, this could be both discrete and onboard GFX, but do ppl really use oprom days for on […]
Some AMD platforms still need a VBIOS for integrated/onboard GFX.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49016 )
Change subject: device: Select VGA_ROM_RUN for non-FSP solution with PCIE DGPU ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/49016/4/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/49016/4/src/device/Kconfig@53 PS4, Line 53: discrete graphics card
Some AMD platforms still need a VBIOS for integrated/onboard GFX.
Sure Angel, do you recommend to add both discrete and integrated GFX using VGA oprom rather only specifying discrete?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49016 )
Change subject: device: Select VGA_ROM_RUN for non-FSP solution with PCIE DGPU ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/49016/4/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/49016/4/src/device/Kconfig@53 PS4, Line 53: discrete graphics card
Sure Angel, do you recommend to add both discrete and integrated GFX using VGA oprom rather only spe […]
I believe we need independent settings for integrated and discrete GFX. Currently, we only have one option for both kinds of GFX devices, and it's making things unnecessarily complex.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49016 )
Change subject: device: Select VGA_ROM_RUN for non-FSP solution with PCIE DGPU ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/49016/4/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/49016/4/src/device/Kconfig@53 PS4, Line 53: discrete graphics card
I believe we need independent settings for integrated and discrete GFX. […]
I agree with Angel here, it might be simpler to split this up into different Kconfigs
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49016 )
Change subject: device: Select VGA_ROM_RUN for non-FSP solution with PCIE DGPU ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/49016/4/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/49016/4/src/device/Kconfig@53 PS4, Line 53: discrete graphics card
I agree with Angel here, it might be simpler to split this up into different Kconfigs
Thanks Angel and Tim. so this CL or rather VGA_ROM_RUN_DEFAULT can we convert for discrete as below?
config VGA_ROM_RUN_DEFAULT_FOR_DGPU def_bool n
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49016 )
Change subject: device: Select VGA_ROM_RUN for non-FSP solution with PCIE DGPU ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/49016/4/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/49016/4/src/device/Kconfig@53 PS4, Line 53: discrete graphics card
Thanks Angel and Tim. […]
I'll have to take a closer look. Let me try something.
Attention is currently required from: Paul Menzel, Tim Wawrzynczak, Subrata Banik, Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49016 )
Change subject: device: Select VGA_ROM_RUN for non-FSP solution with PCIE DGPU ......................................................................
Patch Set 5:
(4 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/49016/comment/025ad6ba_c0aca6d6 PS5, Line 7: non-FSP solution It doesn't really matter if FSP is used or not (coreboot is not 100% FSP). "Select VGA_ROM_RUN for PCIe DGPU" would say more (AFAIK, we don't have any drivers for DGPUs in coreboot, hence that we don't have a driver would be implicit).
https://review.coreboot.org/c/coreboot/+/49016/comment/18282bb4_601bf907 PS5, Line 12: 1. For onboard graphics initialization, GFX PEIM module inside : FSP binary is taking care of graphics initialization based on : HAVE_FSP_GOP Kconfig option and input VBT file. Hence no need : to load/execute legacy VGA OpROM in order to initialize onboard GFX. : : 2. In case of non-FSP solution, platform needs to select VGA_ROM_RUN : Kconfig to perform graphics initialization for PCI-E based discrete : card through VGA OpRom (SoC or Mainboard user can't select VGA_ROM_RUN : directly because it's part of choice option). This is only background information and mostly wrong (wrt. coreboot as a whole). It ignores all open-source means of graphics initialization.
In case you want to keep it here, you could make 1. about "coreboot graphics drivers" in general. No need to highlight the niche FSP spare wheel here.
File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/49016/comment/d87e175d_90ccc766 PS4, Line 53: discrete graphics card
I'll have to take a closer look. Let me try something.
It doesn't seem like we need the IGPU/DGPU distinction right now. Loading OpRoms from cards is the default, so we treat them the same currently.
Also, if we'd add individual options for IGPU/DGPU later, I'd still prefer to have a single one that just says `VGA_ROM_RUN` is the default (see also my comment on PS5, line 65).
Subrata, I guess it would be enough for now to update the help text, i.e. remove 2x "discrete".
File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/49016/comment/43bd8609_993872ae PS5, Line 64: default NO_GFX_INIT if (VGA_BIOS || VGA_ROM_RUN_DEFAULT) && PAYLOAD_SEABIOS : default VGA_ROM_RUN if VGA_BIOS || VGA_ROM_RUN_DEFAULT Looking at this again, I realized that we should only have `VGA_ROM_RUN_DEFAULT` here and let `VGA_BIOS` select it. I can easily do that in a follow-up if you don't want to delay this change.
Attention is currently required from: Paul Menzel, Tim Wawrzynczak, Subrata Banik, Angel Pons. Hello build bot (Jenkins), Furquan Shaikh, Stefan Reinauer, Tim Wawrzynczak, Duncan Laurie, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/49016
to look at the new patch set (#6).
Change subject: device: Add new Kconfig VGA_ROM_RUN_DEFAULT for mainboard to select ......................................................................
device: Add new Kconfig VGA_ROM_RUN_DEFAULT for mainboard to select
Platform can now select VGA_ROM_RUN_DEFAULT Kconfig to perform graphics initialization for PCI-E based discrete card through VGA OpRom (SoC or Mainboard user can't select VGA_ROM_RUN directly because it's part of choice option).
(Note: Some payloads, like SeaBIOS, are also able to run Option ROMs, so coreboot does not need to consider option #2 above)
For payload like depthcharge, create VGA_ROM_RUN_DEFAULT Kconfig for mainboard to select design with DGPU where OpROM is embedded inside the DGPU card.
Allow auto selection of VGA_ROM_RUN_DEFAULT from VGA_BIOS Kconfig.
Also NO_GFX_INIT Kconfig to avoid running VGA_ROM_RUN by default in case SeaBIOS is used.
TEST=Able to get Pre-OS splash screen with AMD Radeon RX 5700 PCI-E DGPU when mainboard user selects VGA_ROM_RUN_DEFAULT.
Change-Id: Iecb2fcdb105af449bc20ad727759cdef17d5e376 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/device/Kconfig 1 file changed, 9 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/49016/6
Attention is currently required from: Nico Huber, Paul Menzel, Tim Wawrzynczak, Angel Pons. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49016 )
Change subject: device: Add new Kconfig VGA_ROM_RUN_DEFAULT for mainboard to select ......................................................................
Patch Set 5:
(3 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/49016/comment/baa37233_f2849022 PS5, Line 7: non-FSP solution
It doesn't really matter if FSP is used or not (coreboot is not […]
Ack
https://review.coreboot.org/c/coreboot/+/49016/comment/199b6f38_c3546eab PS5, Line 12: 1. For onboard graphics initialization, GFX PEIM module inside : FSP binary is taking care of graphics initialization based on : HAVE_FSP_GOP Kconfig option and input VBT file. Hence no need : to load/execute legacy VGA OpROM in order to initialize onboard GFX. : : 2. In case of non-FSP solution, platform needs to select VGA_ROM_RUN : Kconfig to perform graphics initialization for PCI-E based discrete : card through VGA OpRom (SoC or Mainboard user can't select VGA_ROM_RUN : directly because it's part of choice option).
This is only background information and mostly wrong (wrt. coreboot […]
Ack
File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/49016/comment/f4390810_81c8f5ce PS4, Line 53: discrete graphics card
It doesn't seem like we need the IGPU/DGPU distinction right now. Loading […]
Ack
Attention is currently required from: Nico Huber, Paul Menzel, Tim Wawrzynczak, Angel Pons. Hello build bot (Jenkins), Furquan Shaikh, Stefan Reinauer, Tim Wawrzynczak, Duncan Laurie, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/49016
to look at the new patch set (#7).
Change subject: device: Add new Kconfig VGA_ROM_RUN_DEFAULT for mainboard user ......................................................................
device: Add new Kconfig VGA_ROM_RUN_DEFAULT for mainboard user
Platform can now select VGA_ROM_RUN_DEFAULT Kconfig to perform graphics initialization for PCI-E based discrete card through VGA OpRom (SoC or Mainboard user can't select VGA_ROM_RUN directly because it's part of choice option).
(Note: Some payloads, like SeaBIOS, are also able to run Option ROMs, so coreboot does not need to enable VGA_ROM_RUN Kconfig)
For payload like depthcharge, create VGA_ROM_RUN_DEFAULT Kconfig for mainboard to select design with DGPU where OpROM is embedded inside the DGPU card.
Allow auto selection of VGA_ROM_RUN_DEFAULT from VGA_BIOS Kconfig.
Also NO_GFX_INIT Kconfig to avoid running VGA_ROM_RUN by default in case SeaBIOS is used.
TEST=Able to get Pre-OS splash screen with AMD Radeon RX 5700 PCI-E DGPU when mainboard user selects VGA_ROM_RUN_DEFAULT.
Change-Id: Iecb2fcdb105af449bc20ad727759cdef17d5e376 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/device/Kconfig 1 file changed, 9 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/49016/7
Attention is currently required from: Paul Menzel, Tim Wawrzynczak, Subrata Banik, Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49016 )
Change subject: device: Add new Kconfig VGA_ROM_RUN_DEFAULT for mainboard user ......................................................................
Patch Set 7: Code-Review+2
Subrata Banik has submitted this change. ( https://review.coreboot.org/c/coreboot/+/49016 )
Change subject: device: Add new Kconfig VGA_ROM_RUN_DEFAULT for mainboard user ......................................................................
device: Add new Kconfig VGA_ROM_RUN_DEFAULT for mainboard user
Platform can now select VGA_ROM_RUN_DEFAULT Kconfig to perform graphics initialization for PCI-E based discrete card through VGA OpRom (SoC or Mainboard user can't select VGA_ROM_RUN directly because it's part of choice option).
(Note: Some payloads, like SeaBIOS, are also able to run Option ROMs, so coreboot does not need to enable VGA_ROM_RUN Kconfig)
For payload like depthcharge, create VGA_ROM_RUN_DEFAULT Kconfig for mainboard to select design with DGPU where OpROM is embedded inside the DGPU card.
Allow auto selection of VGA_ROM_RUN_DEFAULT from VGA_BIOS Kconfig.
Also NO_GFX_INIT Kconfig to avoid running VGA_ROM_RUN by default in case SeaBIOS is used.
TEST=Able to get Pre-OS splash screen with AMD Radeon RX 5700 PCI-E DGPU when mainboard user selects VGA_ROM_RUN_DEFAULT.
Change-Id: Iecb2fcdb105af449bc20ad727759cdef17d5e376 Signed-off-by: Subrata Banik subrata.banik@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/49016 Reviewed-by: Nico Huber nico.h@gmx.de Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/device/Kconfig 1 file changed, 9 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved
diff --git a/src/device/Kconfig b/src/device/Kconfig index a472a6a..bb4e913 100644 --- a/src/device/Kconfig +++ b/src/device/Kconfig @@ -46,6 +46,12 @@ Selected by mainboards / chipsets whose graphics driver can't or shouldn't be disabled.
+config VGA_ROM_RUN_DEFAULT + def_bool n + help + Selected by mainboards whose graphics initialization depends on VGA OpROM. + coreboot needs to load/execute legacy VGA OpROM in order to initialize GFX. + config MAINBOARD_HAS_LIBGFXINIT def_bool n help @@ -54,8 +60,8 @@
choice prompt "Graphics initialization" - default NO_GFX_INIT if VGA_BIOS && PAYLOAD_SEABIOS - default VGA_ROM_RUN if VGA_BIOS + default NO_GFX_INIT if VGA_ROM_RUN_DEFAULT && PAYLOAD_SEABIOS + default VGA_ROM_RUN if VGA_ROM_RUN_DEFAULT default MAINBOARD_DO_NATIVE_VGA_INIT default MAINBOARD_USE_LIBGFXINIT default RUN_FSP_GOP if INTEL_GMA_HAVE_VBT @@ -690,6 +696,7 @@ config VGA_BIOS bool "Add a VGA BIOS image" depends on ARCH_X86 + select VGA_ROM_RUN_DEFAULT help Select this option if you have a VGA BIOS image that you would like to add to your ROM.
-
Angel Pons (Code Review) -
Nico Huber (Code Review)
-
Paul Menzel (Code Review)
-
Subrata Banik (Code Review)
-
Tim Wawrzynczak (Code Review)