Change in coreboot[master]: src/device/pci: Add support for discrete VGA initialization and OpROM...
Mike Banon has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38202 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
src/device/pci: Add support for discrete VGA initialization and OpROM loading
Required to make the discrete VGA working at some boards like AMD Lenovo G505S.
If CONFIG_MULTIPLE_VGA_ADAPTERS is enabled: coreboot will try to initialize the discrete VGA of PCI_CLASS_DISPLAY_OTHER class and load OpROM on it. ACPI VFCT table will be created only for discrete VGA of PCI_CLASS_DISPLAY_OTHER class instead of integrated (to avoid the discrete VGA init problems), but everything is working fine. Later will try to make a common VFCT table for both adapters.
Based on the original patches by Hans Jürgen Kitter eforname@freemail.hu.
Signed-off-by: Mike Banon mikebdp2@gmail.com Change-Id: I9a7aa31c956d8f716e2d5a73ff032134c144d3db --- M src/device/pci_device.c M src/device/pci_rom.c 2 files changed, 74 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/38202/1
diff --git a/src/device/pci_device.c b/src/device/pci_device.c index 36b7c82..f0634fd 100644 --- a/src/device/pci_device.c +++ b/src/device/pci_device.c @@ -712,7 +712,9 @@ * ROMs when coming out of an S3 resume. */ if (!CONFIG(S3_VGA_ROM_RUN) && acpi_is_wakeup_s3() && - ((dev->class >> 8) == PCI_CLASS_DISPLAY_VGA)) + (((dev->class >> 8) == PCI_CLASS_DISPLAY_VGA) || + (CONFIG(MULTIPLE_VGA_ADAPTERS) && + ((dev->class >> 8) == PCI_CLASS_DISPLAY_OTHER)))) return 0; if (CONFIG(ALWAYS_LOAD_OPROM)) return 1; @@ -726,34 +728,55 @@ void pci_dev_init(struct device *dev) { struct rom_header *rom, *ram; + unsigned int pci_class = (dev->class >> 8);
if (!CONFIG(VGA_ROM_RUN)) return;
- /* Only execute VGA ROMs. */ - if (((dev->class >> 8) != PCI_CLASS_DISPLAY_VGA)) + /* Only load VGA ROMs. */ + switch (pci_class) { + case PCI_CLASS_DISPLAY_VGA: + break; + case PCI_CLASS_DISPLAY_OTHER: + if (!CONFIG(MULTIPLE_VGA_ADAPTERS)) + return; + break; + default: return; + }
if (!should_load_oprom(dev)) return; timestamp_add_now(TS_OPROM_INITIALIZE);
rom = pci_rom_probe(dev); - if (rom == NULL) + if (rom == NULL) { + printk(BIOS_DEBUG, "%s%s ROM probe failed\n", + pci_class == PCI_CLASS_DISPLAY_VGA ? "" : "Non-", + "PCI_CLASS_DISPLAY_VGA"); return; + }
ram = pci_rom_load(dev, rom); - if (ram == NULL) + if (ram == NULL) { + printk(BIOS_DEBUG, "%s%s ROM load failed\n", + pci_class == PCI_CLASS_DISPLAY_VGA ? "" : "Non-", + "PCI_CLASS_DISPLAY_VGA"); return; + } timestamp_add_now(TS_OPROM_COPY_END);
if (!should_run_oprom(dev, rom)) return;
- run_bios(dev, (unsigned long)ram); + /* Only execute Integrated VGA Option ROM. */ + if (pci_class == PCI_CLASS_DISPLAY_VGA) { + run_bios(dev, (unsigned long)ram); + gfx_set_init_done(1); + printk(BIOS_DEBUG, "PCI_CLASS_DISPLAY_VGA Option ROM was run\n"); + } else + printk(BIOS_DEBUG, "Not running non-PCI_CLASS_DISPLAY_VGA Option ROM\n");
- gfx_set_init_done(1); - printk(BIOS_DEBUG, "VGA Option ROM was run\n"); timestamp_add_now(TS_OPROM_END); }
diff --git a/src/device/pci_rom.c b/src/device/pci_rom.c index 3676f9c..f1f7978 100644 --- a/src/device/pci_rom.c +++ b/src/device/pci_rom.c @@ -181,7 +181,20 @@ if (!CONFIG(VGA_ROM_RUN)) return NULL;
- run_rom = (struct rom_header *)(uintptr_t)PCI_VGA_RAM_IMAGE_START; + /* Set the start of shadow memory for this Integrated or Discrete VGA. */ + switch (dev->class >> 8) { + case PCI_CLASS_DISPLAY_VGA: + run_rom = (struct rom_header *)(uintptr_t)PCI_VGA_RAM_IMAGE_START; + break; + case PCI_CLASS_DISPLAY_OTHER: + if (!CONFIG(MULTIPLE_VGA_ADAPTERS)) + return NULL; + run_rom = (struct rom_header *)(uintptr_t)PCI_RAM_IMAGE_START; + break; + default: + return NULL; + } + if (read_le16(&run_rom->signature) != PCI_ROM_HDR) return NULL;
@@ -211,11 +224,26 @@ return current; }
- printk(BIOS_DEBUG, " Copying %sVBIOS image from %p\n", + /* Setup ROM address for copying it later from CBFS to ACPI VFCT. */ + switch (device->class >> 8) { + case PCI_CLASS_DISPLAY_VGA: + printk(BIOS_DEBUG, " Copying %sVBIOS image from %p\n", rom == (struct rom_header *) (uintptr_t)PCI_VGA_RAM_IMAGE_START ? "initialized " : "", rom); + break; + case PCI_CLASS_DISPLAY_OTHER: + printk(BIOS_DEBUG, " Copying %sVBIOS image from %p\n", + rom == (struct rom_header *) + (uintptr_t)PCI_RAM_IMAGE_START ? + "initialized " : "", + rom); + break; + default: + printk(BIOS_DEBUG, " Copying VBIOS image from %p\n", + rom); + }
header->DeviceID = device->device; header->VendorID = device->vendor; @@ -236,8 +264,20 @@ struct acpi_rsdp *rsdp) { /* Only handle VGA devices */ - if ((device->class >> 8) != PCI_CLASS_DISPLAY_VGA) + switch (device->class >> 8) { + case PCI_CLASS_DISPLAY_VGA: + if (CONFIG(MULTIPLE_VGA_ADAPTERS)) + return current; + /* Write ACPI VFCT only for Discrete VGA. */ + /* TODO: do this also for Integrated VGA. */ + break; + case PCI_CLASS_DISPLAY_OTHER: + if (!CONFIG(MULTIPLE_VGA_ADAPTERS)) + return current; + break; + default: return current; + }
/* Only handle enabled devices */ if (!device->enabled)
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38202 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 2:
(1 comment)
I will test on a device with dGPU and see if it works.
https://review.coreboot.org/c/coreboot/+/38202/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38202/2//COMMIT_MSG@11 PS2, Line 11: If CONFIG_MULTIPLE_VGA_ADAPTERS is enabled: coreboot will try to initialize the : discrete VGA of PCI_CLASS_DISPLAY_OTHER class and load OpROM on it. ACPI VFCT : table will be created only for discrete VGA of PCI_CLASS_DISPLAY_OTHER class : instead of integrated (to avoid the discrete VGA init problems), but everything : is working fine. Later will try to make a common VFCT table for both adapters. Please wrap at 72 characters
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38202
to look at the new patch set (#3).
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
src/device/pci: Add support for discrete VGA initialization and OpROM loading
Required to make the discrete VGA working at boards like Lenovo G505S where the discrete VGA doesn't have its own memory for storing a VBIOS.
If CONFIG_MULTIPLE_VGA_ADAPTERS is enabled: coreboot will try to initialize the discrete VGA of PCI_CLASS_DISPLAY_OTHER class and load VBIOS OpROM on it. ACPI VFCT table will be created only for discrete VGA of PCI_CLASS_DISPLAY_OTHER class instead of integrated (to avoid the discrete VGA init problems), but everything is working fine. Later will try to make a common VFCT table for both adapters.
Based on the original patches by Hans Jürgen Kitter eforname@freemail.hu.
Signed-off-by: Mike Banon mikebdp2@gmail.com Change-Id: I9a7aa31c956d8f716e2d5a73ff032134c144d3db --- M src/device/pci_device.c M src/device/pci_rom.c 2 files changed, 74 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/38202/3
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38202 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 3:
(1 comment)
This change makes a difference only if your discrete VGA doesn't have its own memory for storing a VBIOS (or maybe if you'd like to load a custom VBIOS on your GPU without flashing it to its' memory - but I haven't explored this yet). Otherwise there shouldn't be any difference. So currently it benefits only a few boards like Lenovo G505S.
https://review.coreboot.org/c/coreboot/+/38202/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38202/2//COMMIT_MSG@11 PS2, Line 11: If CONFIG_MULTIPLE_VGA_ADAPTERS is enabled: coreboot will try to initialize the : discrete VGA of PCI_CLASS_DISPLAY_OTHER class and load OpROM on it. ACPI VFCT : table will be created only for discrete VGA of PCI_CLASS_DISPLAY_OTHER class : instead of integrated (to avoid the discrete VGA init problems), but everything : is working fine. Later will try to make a common VFCT table for both adapters.
Please wrap at 72 characters
Done.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38202
to look at the new patch set (#7).
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
src/device/pci: Add support for discrete VGA initialization and OpROM loading
Required to make the discrete VGA working at boards like Lenovo G505S where the discrete VGA doesn't have its own memory for storing a VBIOS.
If CONFIG_MULTIPLE_VGA_ADAPTERS is enabled: coreboot will try to initialize the discrete VGA of PCI_CLASS_DISPLAY_OTHER class and load VBIOS OpROM on it. ACPI VFCT table will be created only for discrete VGA of PCI_CLASS_DISPLAY_OTHER class instead of integrated (to avoid the discrete VGA init problems), but everything is working fine. Later will try to make a common VFCT table for both adapters.
Based on the original patches by Hans Jürgen Kitter eforname@freemail.hu.
Signed-off-by: Mike Banon mikebdp2@gmail.com Change-Id: I9a7aa31c956d8f716e2d5a73ff032134c144d3db --- M src/device/pci_device.c M src/device/pci_rom.c 2 files changed, 74 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/38202/7
Attention is currently required from: Mike Banon. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38202 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 7: Code-Review-1
(4 comments)
Patchset:
PS7: This looks like a board/device specific hack, that cannot be generalised. Instead of modifying the pci_dev_init, why not have a PCI driver for the discrete GPU on your device with a different init function. It could even load the option rom at other available addresses, to not conflict with the VGA option ROM loading.
File src/device/pci_device.c:
https://review.coreboot.org/c/coreboot/+/38202/comment/9b819f43_6a6ebdfa PS7, Line 749: if (!CONFIG(VGA_ROM_RUN)) : return; One can assume that DISPLAY_OTHER does not decode VGA. So this makes sense.
https://review.coreboot.org/c/coreboot/+/38202/comment/7fb285fe_b21f479b PS7, Line 756: PCI_CLASS_DISPLAY_OTHER Why not create a PCI specific driver for your device to load the option ROM without executing it. I don't think this behavior should be generalised or guarded with vague Kconfig options.
https://review.coreboot.org/c/coreboot/+/38202/comment/e87a2046_ed40fa63 PS7, Line 776: pci_rom_load This always loads the rom at 0xC0000 which is generally used for the device decoding VGA. Why not have a device specific driver to also load roms at the next option ROM address 0xD0000?
Attention is currently required from: Arthur Heymans. Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38202 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 7:
(1 comment)
Patchset:
PS7:
This looks like a board/device specific hack
In addition to lenovo/g505s it could've benefited a similar hp/pavilion_m6_1035dx laptop which (like G505S) has AMD integrated GPU + AMD discrete GPU and both without a dedicated storage for their VBIOS ROMs. But it seems no-one is using 1035dx, so yes this hack benefits only G505S at the moment.
why not have a PCI driver for the discrete GPU on your device with a different init function. It could even load the option rom at other available addresses, to not conflict with the VGA option ROM loading.
I hope you mean a coreboot's PCI driver? If yes, I need to search for a good example and figure out how to do this - with keeping you helpful comments below in mind, of course
Mike Banon has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/38202 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Abandoned
Abandoned in favor of a fresh CB:58652. Thank you for your comments, I will move the unresolved ones to a new change.
Mike Banon has restored this change. ( https://review.coreboot.org/c/coreboot/+/38202 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Restored
Mike Banon has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/38202 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Abandoned
Abandoned in favor of a fresh CB:58652. Thank you for your comments, I will move the unresolved ones to a new change.
-
Arthur Heymans (Code Review) -
Michał Żygowski (Code Review)
-
Mike Banon (Code Review)