Change in ...coreboot[master]: src/device/pci: Add support for discrete VGA initialization and OpROM...
Hello Mike Banon,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/31448
to review the following change.
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 and load OpROM on it. ACPI VFCT table will be created only for discrete VGA instead of integrated, but everything seems to be working fine. Maybe later we would find a way 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 Signed-off-by: Hans Jürgen Kitter eforname@freemail.hu Change-Id: If3269a0911e86ea07014484365c7c830485e52ec --- M src/device/pci_device.c M src/device/pci_rom.c 2 files changed, 68 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/31448/1
diff --git a/src/device/pci_device.c b/src/device/pci_device.c index 82033a6..96dec0b 100644 --- a/src/device/pci_device.c +++ b/src/device/pci_device.c @@ -764,7 +764,9 @@ * ROMs when coming out of an S3 resume. */ if (!IS_ENABLED(CONFIG_S3_VGA_ROM_RUN) && acpi_is_wakeup_s3() && - ((dev->class >> 8) == PCI_CLASS_DISPLAY_VGA)) + (((dev->class >> 8) == PCI_CLASS_DISPLAY_VGA) || + (IS_ENABLED(CONFIG_MULTIPLE_VGA_ADAPTERS) && + ((dev->class >> 8) == PCI_CLASS_DISPLAY_OTHER)))) return 0; if (IS_ENABLED(CONFIG_ALWAYS_LOAD_OPROM)) return 1; @@ -778,25 +780,41 @@ void pci_dev_init(struct device *dev) { struct rom_header *rom, *ram; + unsigned int pci_class = (dev->class >> 8);
if (!IS_ENABLED(CONFIG_VGA_ROM_RUN)) return;
- /* Only execute VGA ROMs. */ - if (((dev->class >> 8) != PCI_CLASS_DISPLAY_VGA)) - return; + /* Only load VGA ROMs. */ + if (pci_class != PCI_CLASS_DISPLAY_VGA) { + if (!IS_ENABLED(CONFIG_MULTIPLE_VGA_ADAPTERS)) { + return; + } else if (pci_class != PCI_CLASS_DISPLAY_OTHER) { + return; + } + }
if (!should_load_oprom(dev)) return; timestamp_add_now(TS_OPROM_INITIALIZE);
rom = pci_rom_probe(dev); - if (rom == NULL) + if (rom == NULL) { + if (pci_class == PCI_CLASS_DISPLAY_VGA) + printk(BIOS_DEBUG, "Integrated VGA ROM probe failed\n"); + else + printk(BIOS_DEBUG, "Discrete VGA ROM probe failed\n"); return; + }
ram = pci_rom_load(dev, rom); - if (ram == NULL) + if (ram == NULL) { + if (pci_class == PCI_CLASS_DISPLAY_VGA) + printk(BIOS_DEBUG, "Integrated VGA ROM load failed\n"); + else + printk(BIOS_DEBUG, "Discrete VGA ROM load failed\n"); return; + } timestamp_add_now(TS_OPROM_COPY_END);
if (!should_run_oprom(dev)) @@ -804,7 +822,11 @@
run_bios(dev, (unsigned long)ram); gfx_set_init_done(1); - printk(BIOS_DEBUG, "VGA Option ROM was run\n"); + if (pci_class == PCI_CLASS_DISPLAY_VGA) + printk(BIOS_DEBUG, "Integrated VGA Option ROM was run\n"); + else + printk(BIOS_DEBUG, "Discrete 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 82d9a30..60f9aed 100644 --- a/src/device/pci_rom.c +++ b/src/device/pci_rom.c @@ -179,13 +179,24 @@ /* VBIOS may be modified after oprom init so use the copy if present. */ static struct rom_header *check_initialized(struct device *dev) { - struct rom_header *run_rom; + struct rom_header *run_rom = NULL; struct pci_data *rom_data;
if (!IS_ENABLED(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. */ + if ((dev->class >> 8) == PCI_CLASS_DISPLAY_VGA) { + run_rom = (struct rom_header *)(uintptr_t)PCI_VGA_RAM_IMAGE_START; + } else if (IS_ENABLED(CONFIG_MULTIPLE_VGA_ADAPTERS)) { + if ((dev->class >> 8) == PCI_CLASS_DISPLAY_OTHER) { + run_rom = (struct rom_header *)(uintptr_t)PCI_RAM_IMAGE_START; + } + } + + if (run_rom == NULL) + return NULL; + if (read_le16(&run_rom->signature) != PCI_ROM_HDR) return NULL;
@@ -217,11 +228,22 @@ return current; }
- printk(BIOS_DEBUG, " Copying %sVBIOS image from %p\n", + if (!IS_ENABLED(CONFIG_MULTIPLE_VGA_ADAPTERS)) { + /* Copy Integrated VGA VBIOS from CBFS to ACPI VFCT. */ + printk(BIOS_DEBUG, " Copying %sVBIOS image from %p\n", rom == (struct rom_header *) (uintptr_t)PCI_VGA_RAM_IMAGE_START ? "initialized " : "", rom); + } else { + /* Copy Discrete VGA VBIOS from CBFS to ACPI VFCT. */ + printk(BIOS_DEBUG, " Copying %sVBIOS image from %p\n", + rom == (struct rom_header *) + (uintptr_t)PCI_RAM_IMAGE_START ? + "initialized " : "", + rom); + /* TODO: do this also for Integrated VGA VBIOS. */ + }
header->DeviceID = device->device; header->VendorID = device->vendor; @@ -243,8 +265,20 @@ struct rom_header *rom;
/* Only handle VGA devices */ - if ((device->class >> 8) != PCI_CLASS_DISPLAY_VGA) - return current; + + if (!IS_ENABLED(CONFIG_MULTIPLE_VGA_ADAPTERS)) { + if ((device->class >> 8) != PCI_CLASS_DISPLAY_VGA) { + return current; + } + } else { + /* Write ACPI VFCT only for Discrete VGA. */ + if ((device->class >> 8) == PCI_CLASS_DISPLAY_OTHER) { + ; + } else { + return current; + } + /* TODO: do this also for Integrated VGA. */ + }
/* Only handle enabled devices */ if (!device->enabled)
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 1:
(7 comments)
https://review.coreboot.org/#/c/31448/1/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/#/c/31448/1/src/device/pci_device.c@790 PS1, Line 790: if (!IS_ENABLED(CONFIG_MULTIPLE_VGA_ADAPTERS)) { braces {} are not necessary for any arm of this statement
https://review.coreboot.org/#/c/31448/1/src/device/pci_rom.c File src/device/pci_rom.c:
https://review.coreboot.org/#/c/31448/1/src/device/pci_rom.c@188 PS1, Line 188: /* Set the start of shadow memory for this Integrated or Discrete VGA. */ line over 80 characters
https://review.coreboot.org/#/c/31448/1/src/device/pci_rom.c@190 PS1, Line 190: run_rom = (struct rom_header *)(uintptr_t)PCI_VGA_RAM_IMAGE_START; line over 80 characters
https://review.coreboot.org/#/c/31448/1/src/device/pci_rom.c@192 PS1, Line 192: if ((dev->class >> 8) == PCI_CLASS_DISPLAY_OTHER) { braces {} are not necessary for single statement blocks
https://review.coreboot.org/#/c/31448/1/src/device/pci_rom.c@193 PS1, Line 193: run_rom = (struct rom_header *)(uintptr_t)PCI_RAM_IMAGE_START; line over 80 characters
https://review.coreboot.org/#/c/31448/1/src/device/pci_rom.c@270 PS1, Line 270: if ((device->class >> 8) != PCI_CLASS_DISPLAY_VGA) { braces {} are not necessary for single statement blocks
https://review.coreboot.org/#/c/31448/1/src/device/pci_rom.c@275 PS1, Line 275: if ((device->class >> 8) == PCI_CLASS_DISPLAY_OTHER) { braces {} are not necessary for any arm of this statement
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 1:
The Kconfig option seems useless.
Can't you simply write two VFCT acpi tables ? Looking at the code that be possible.
Is it safe to load / run two option ROMs ? I'd only load the primary one.
This patch should be split into two parts: One for providing multiple VFCT tables, and one for option ROM loading.
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 1:
Patch Set 1:
The Kconfig option seems useless.
Which option, and why useless?
Patch Set 1:
Can't you simply write two VFCT acpi tables ? Looking at the code that be possible.
I tried doing that but my discrete GPU did not work because of initialization errors. And I don't know how to create a single working table for both of them, so I am not doing it yet.
Is it safe to load / run two option ROMs ? I'd only load the primary one.
If you will not load the OpROM for your discrete GPU, and it does not have its' own memory with a local copy of OpROM stored, then your discrete GPU will fail to initialize and will not be working. At least that's the case for laptop discrete AMD GPUs. But if you don't need a working discrete GPU (e.g. because you are not satisfied with its' performance), then you could include only your primary option ROM to CBFS.
This patch should be split into two parts: One for providing multiple VFCT tables, and one for option ROM loading.
I do not plan to change the VFCT part because of the reasons mentioned above and at commit message, but please let me know if I still need to separate it into a different patch.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 1:
The Kconfig option seems useless.
Which option, and why useless?
I assume CONFIG_MULTIPLE_VGA_ADAPTERS? and have to say, I never understood it either. It doesn't seem to do anything AMD specific so why do other platforms don't need it? IMHO, we should get rid of that option, first, or document what it does if it does some- thing useful.
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 1:
Patch Set 1:
The Kconfig option seems useless.
Which option, and why useless?
I assume CONFIG_MULTIPLE_VGA_ADAPTERS? and have to say, I never understood it either. It doesn't seem to do anything AMD specific so why do other platforms don't need it? IMHO, we should get rid of that option, first, or document what it does if it does some- thing useful.
There is only small code at pci_rom.c (line ~150) which is common for all the boards:
/* * We check to see if the device thinks it is a VGA device not * whether the ROM image is for a VGA device because some * devices have a mismatch between the hardware and the ROM. */ if ((dev->class >> 8) == PCI_CLASS_DISPLAY_VGA) { #if !IS_ENABLED(CONFIG_MULTIPLE_VGA_ADAPTERS) extern struct device *vga_pri; /* Primary VGA device (device.c). */ if (dev != vga_pri) return NULL; /* Only one VGA supported. */ #endif
All the other code related to this option - is AMD only:
cd ./coreboot/ find . -type f -print0 | xargs -0 grep -l -n "MULTIPLE_VGA_ADAPTERS" ./src/northbridge/amd/amdfam10/northbridge.c ./src/northbridge/amd/agesa/family15tn/northbridge.c ./src/northbridge/amd/agesa/family16kb/northbridge.c ./src/northbridge/amd/pi/00660F01/northbridge.c ./src/northbridge/amd/pi/00730F01/northbridge.c ./src/northbridge/amd/pi/00630F01/northbridge.c ./src/device/pci_rom.c ./src/device/Kconfig
This code inside northbridge/amd files seems to be important, e.g. ./src/northbridge/amd/agesa/family15tn/northbridge.c (line ~370)
#if IS_ENABLED(CONFIG_MULTIPLE_VGA_ADAPTERS) extern struct device *vga_pri; // the primary vga device, defined in device.c printk(BIOS_DEBUG, "VGA: vga_pri bus num = %d bus range [%d,%d]\n", vga_pri->bus->secondary, link->secondary,link->subordinate); /* We need to make sure the vga_pri is under the link */ if ((vga_pri->bus->secondary >= link->secondary) && (vga_pri->bus->secondary <= link->subordinate)) #endif
But enabling such pieces of code for all the AMD boards (by removing CONFIG_MULTIPLE_VGA_ADAPTERS option) maybe could cause problems for some of them
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
The Kconfig option seems useless.
Which option, and why useless?
I assume CONFIG_MULTIPLE_VGA_ADAPTERS? and have to say, I never understood it either. It doesn't seem to do anything AMD specific so why do other platforms don't need it? IMHO, we should get rid of that option, first, or document what it does if it does some- thing useful.
$ git grep create_vga_resource .. create_vga_resource(dev, nodeid); //TODO: do we need this? ..
Does not rise much confidence...
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
The Kconfig option seems useless.
Which option, and why useless?
I assume CONFIG_MULTIPLE_VGA_ADAPTERS? and have to say, I never understood it either. It doesn't seem to do anything AMD specific so why do other platforms don't need it? IMHO, we should get rid of that option, first, or document what it does if it does some- thing useful.
$ git grep create_vga_resource .. create_vga_resource(dev, nodeid); //TODO: do we need this? ..
Does not rise much confidence...
Even if this option does not seem too useful to you at the moment: it already exists and has a nice name, so we could take it over by writing useful code for it.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 1:
Does not rise much confidence...
Even if this option does not seem too useful to you at the moment: it already exists and has a nice name, so we could take it over by writing useful code for it.
No. Don't invent new purpose for old Kconfig.
AFAIK it is forbidden to route the IO and MMIO regions related to VGA Option Rom to two different PCI functions. Not sure how you could even run two option roms without redoing the routing in between.
I think the intention here has been to make sure that the graphics card, for which we have option rom included in CBFS, gets the routing setup. Then again, the code is multi-node/hypertransport related so all that may be leftovers from fam15 model0 (before TN/RL).
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31448/1/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/#/c/31448/1/src/device/pci_device.c@803 PS1, Line 803: if (pci_class == PCI_CLASS_DISPLAY_VGA) I don't quite understand how you could use the class to distinguish between integrated and discrete graphics. Don't standard PCIe graphics cards usually advertise PCI_CLASS_DISPLAY_VGA?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 1: Code-Review-1
Mike, so turns out I did already look into this (CB:20638) and asked you to test the effect of MULTIPLE_VGA_ADAPTERS. Can you please do that?
Also, IIRC, I asked to check what Linux actually requires to bring graphics up. So we don't have to twist coreboot for some legacy stuff that isn't really required. If the rumors are true, Linux only needs a way to find the Atom- BIOS code and doesn't require the firmware to execute any of it. So please, find out what ways Linux' driver sup- ports to acquire the AtomBIOS. There might be much clea- ner ways than running this nasty stuff in coreboot.
AFAIK it is forbidden to route the IO and MMIO regions related to VGA Option Rom to two different PCI functions.
Yeah that just wouldn't work.
Not sure how you could even run two option roms without redoing the routing in between.
I'm not sure about this. But a hunch tells me that the option rom shouldn't assume the routing but check it and act accordingly.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 1:
Not sure how you could even run two option roms without redoing the routing in between.
I'm not sure about this. But a hunch tells me that the option rom shouldn't assume the routing but check it and act accordingly.
I was looking at that AMD NB code, seemed to touch non-standard PCI config registers. Well, maybe standard to HyperTransport but still outside PCI bridge header specs.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31448/1/src/device/pci_rom.c File src/device/pci_rom.c:
https://review.coreboot.org/#/c/31448/1/src/device/pci_rom.c@274 PS1, Line 274: /* Write ACPI VFCT only for Discrete VGA. */ looking at the kernel source: https://github.com/torvalds/linux/blob/6f0d349d922ba44e4348a17a78ea51b713596... The VFCT should be used for the iGPU in dual graphics systems, or the main GPU in single graphics systems.
For dual-graphics dGPU there's the "ATRM" method, which seems to be similar to _ROM, which is already implemented (below this function).
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 1:
https://review.coreboot.org/c/coreboot/+/31502 should fix the issue on dual gpu systems.
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 1:
(1 comment)
@Kyösti :
Patch Set 1:
AFAIK it is forbidden to route the IO and MMIO regions related to VGA Option Rom to two different PCI functions. Not sure how you could even run two option roms without redoing the routing in between.
I think the intention here has been to make sure that the graphics card, for which we have option rom included in CBFS, gets the routing setup. Then again, the code is multi-node/hypertransport related so all that may be leftovers from fam15 model0 (before TN/RL).
Original patches by HJK loaded both integrated GPU's and discrete GPU's Option ROMs , but executed only the integrated GPU's OpROM. While trying to optimize these patches I noticed that there are no negative consequences from running both of these OpROMs, so I removed a check and let them run both - also hoping that it could help to get Crossfire working sometime in the future. The GPUs of this laptop - integrated and discrete - have nearly the same performance and were meant to be ran in tandem, that's why I thought that letting both OpROMs to run could be a good idea because maybe a discrete OpROM is configuring something important related to Crossfire.
https://review.coreboot.org/#/c/31448/1/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/#/c/31448/1/src/device/pci_device.c@803 PS1, Line 803: if (pci_class == PCI_CLASS_DISPLAY_VGA)
I don't quite understand how you could use the class to distinguish between integrated and discrete […]
If this detection will turn out as imperfect - I will improve it. Or maybe I could simplify it to just "VGA ROM probe failed" because it will be obvious if this GPU is integrated or discrete from reading the debug lines above it. Please let me know if I should do it. By the way the standard desktop GPUs should have their own non-volatile memory with Option ROM stored inside.
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 1:
Patch Set 1: Code-Review-1
Mike, so turns out I did already look into this (CB:20638) and asked you to test the effect of MULTIPLE_VGA_ADAPTERS. Can you please do that?
Although it is a bit discouraging to get -1 for a patch that - unlike this old CB:20638 - really helps to get a G505S discrete GPU working (together with CB:31449 and CB:31450 of course - I split a big patch in these three) , thank you for reminding about your request.
Today I made multiple tests and found out that commenting out the already-existing-at-coreboot-master "if MULTIPLE_VGA_ADAPTERS" options (at this ./src/device/pci_rom.c and ./src/northbridge/amd/agesa/family15tn/northbridge.c) or even removing the code surrounded by these already existing options - did not affect anything: I have not observed any differences in discrete GPU functioning or cbmem/kernel logs. If I am including these three patches in their current version to coreboot, my discrete GPU is working even with this old code removed.
However, later I found out that code at ./src/northbridge/amd/agesa/family15tn/northbridge.c (line ~370)
#if IS_ENABLED(CONFIG_MULTIPLE_VGA_ADAPTERS) extern struct device *vga_pri; // the primary vga device, defined in device.c printk(BIOS_DEBUG, "VGA: vga_pri bus num = %d bus range [%d,%d]\n", vga_pri->bus->secondary, link->secondary,link->subordinate); /* We need to make sure the vga_pri is under the link */ if ((vga_pri->bus->secondary >= link->secondary) && (vga_pri->bus->secondary <= link->subordinate)) #endif
has never been executed before, because a check surrounding this code
for (link = dev->link_list; link; link = link->next) { if (link->bridge_ctrl & PCI_BRIDGE_CTL_VGA) { // <---- this one #if IS_ENABLED(CONFIG_MULTIPLE_VGA_ADAPTERS)
had been always failing for me. So although this existing code is useless to me, I don't know if it is useless to all the other coreboot boards also (could this check above be passing for some?) , and also if this primary/secondary/subordinate stuff could be used for getting a Crossfire working on this laptop.
Also, IIRC, I asked to check what Linux actually requires to bring graphics up. So we don't have to twist coreboot for some legacy stuff that isn't really required. If the rumors are true, Linux only needs a way to find the Atom- BIOS code and doesn't require the firmware to execute any of it. So please, find out what ways Linux' driver sup- ports to acquire the AtomBIOS. There might be much clea- ner ways than running this nasty stuff in coreboot.
Although there's also BSD in addition to Linux (didn't want my changes to take OS in consideration), I think we could guess what a Linux driver wants by reading its' error messages which it prints in case of initialization failure - e.g. when today I tried following Rudolph's "VFCT should be used for the integrated GPU in dual graphics systems" advice above and enabled VFCT filling for integrated GPU only in both cases, the discrete GPU/driver failed to initialize and I got these familiar kernel log messages:
radeon 0000:00:01.0: fb0: radeondrmfb frame buffer device [drm] Initialized radeon 2.50.0 20080528 for 0000:00:01.0 on minor 0 [drm] initializing kernel modesetting (HAINAN 0x1002:0x6665 0x0000:0x0000 0x00). [drm:radeon_get_bios [radeon]] *ERROR* ACPI VFCT image header truncated radeon 0000:01:00.0: Invalid PCI ROM header signature: expecting 0xaa55, got 0x0000 radeon 0000:01:00.0: Invalid PCI ROM header signature: expecting 0xaa55, got 0x0000 [drm:radeon_get_bios [radeon]] *ERROR* Unable to locate a BIOS ROM radeon 0000:01:00.0: Fatal error during GPU init [drm] radeon: finishing device.
So it seems to me that we need the discrete GPU OpROM loaded (running isn't required, and whether could be useful - I don't know yet), and also VFCT filling for discrete GPU is required - because otherwise it does not work.
AFAIK it is forbidden to route the IO and MMIO regions related to VGA Option Rom to two different PCI functions.
Yeah that just wouldn't work.
Not sure how you could even run two option roms without redoing the routing in between.
I'm not sure about this. But a hunch tells me that the option rom shouldn't assume the routing but check it and act accordingly.
Please see the comment addressed to Kyösti above; in short - I have not noticed any difference from letting both OptionROMs run and cbmem/kernel logs are almost the same.
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 1:
(1 comment)
Patch Set 1:
https://review.coreboot.org/c/coreboot/+/31502 should fix the issue on dual gpu systems.
See my comment below, also today I tested your [WIP] patch, and - in its' current state - it does not seem to affect the discrete GPU : if I do VFCT filling for integated GPU like you suggested, the discrete GPU does not work , but if I do VFCT for discrete GPU instead - both integrated and discrete GPUs are working ; and these outcomes are happening this way both without and with your patch merged.
https://review.coreboot.org/#/c/31448/1/src/device/pci_rom.c File src/device/pci_rom.c:
https://review.coreboot.org/#/c/31448/1/src/device/pci_rom.c@274 PS1, Line 274: /* Write ACPI VFCT only for Discrete VGA. */
looking at the kernel source: […]
Today I tried following your "VFCT should be used for the integrated GPU in dual graphics systems" advice above and enabled VFCT filling for integrated GPU only in both cases, but sadly the discrete GPU/driver failed to initialize and I got the errors in kernel log which you could see in a message to Nico above. So I still need to do VFCT filling for discrete GPU instead of integrated - because otherwise it does not work.
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 1:
Patch Set 1:
Don't invent new purpose for old Kconfig.
This MULTIPLE_VGA_ADAPTERS name is perfectly suitable for what I am doing, but if you disagree, please suggest a better name.
Hello Mike Banon, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31448
to look at the new patch set (#2).
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 and load OpROM on it. ACPI VFCT table will be created only for discrete VGA instead of integrated, but everything seems to be working fine. Maybe later we would find a way 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 Signed-off-by: Hans Jürgen Kitter eforname@freemail.hu Change-Id: If3269a0911e86ea07014484365c7c830485e52ec --- M src/device/pci_device.c M src/device/pci_rom.c 2 files changed, 72 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/31448/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 2:
(8 comments)
https://review.coreboot.org/#/c/31448/2/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/#/c/31448/2/src/device/pci_device.c@790 PS2, Line 790: if (!IS_ENABLED(CONFIG_MULTIPLE_VGA_ADAPTERS)) { braces {} are not necessary for any arm of this statement
https://review.coreboot.org/#/c/31448/2/src/device/pci_device.c@829 PS2, Line 829: else else should follow close brace '}'
https://review.coreboot.org/#/c/31448/2/src/device/pci_rom.c File src/device/pci_rom.c:
https://review.coreboot.org/#/c/31448/2/src/device/pci_rom.c@188 PS2, Line 188: /* Set the start of shadow memory for this Integrated or Discrete VGA. */ line over 80 characters
https://review.coreboot.org/#/c/31448/2/src/device/pci_rom.c@190 PS2, Line 190: run_rom = (struct rom_header *)(uintptr_t)PCI_VGA_RAM_IMAGE_START; line over 80 characters
https://review.coreboot.org/#/c/31448/2/src/device/pci_rom.c@192 PS2, Line 192: if ((dev->class >> 8) == PCI_CLASS_DISPLAY_OTHER) { braces {} are not necessary for single statement blocks
https://review.coreboot.org/#/c/31448/2/src/device/pci_rom.c@193 PS2, Line 193: run_rom = (struct rom_header *)(uintptr_t)PCI_RAM_IMAGE_START; line over 80 characters
https://review.coreboot.org/#/c/31448/2/src/device/pci_rom.c@270 PS2, Line 270: if ((device->class >> 8) != PCI_CLASS_DISPLAY_VGA) { braces {} are not necessary for single statement blocks
https://review.coreboot.org/#/c/31448/2/src/device/pci_rom.c@275 PS2, Line 275: if ((device->class >> 8) == PCI_CLASS_DISPLAY_OTHER) { braces {} are not necessary for any arm of this statement
Hello Mike Banon, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31448
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 some boards like AMD Lenovo G505S. If CONFIG_MULTIPLE_VGA_ADAPTERS is enabled, coreboot will try to initialize the discrete VGA and load OpROM on it. ACPI VFCT table will be created only for discrete VGA instead of integrated, but everything seems to be working fine. Maybe later we would find a way 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 Signed-off-by: Hans Jürgen Kitter eforname@freemail.hu Change-Id: If3269a0911e86ea07014484365c7c830485e52ec --- M src/device/pci_device.c M src/device/pci_rom.c 2 files changed, 67 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/31448/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/#/c/31448/3/src/device/pci_rom.c File src/device/pci_rom.c:
https://review.coreboot.org/#/c/31448/3/src/device/pci_rom.c@188 PS3, Line 188: /* Set the start of shadow memory for this Integrated or Discrete VGA. */ line over 80 characters
https://review.coreboot.org/#/c/31448/3/src/device/pci_rom.c@190 PS3, Line 190: run_rom = (struct rom_header *)(uintptr_t)PCI_VGA_RAM_IMAGE_START; line over 80 characters
https://review.coreboot.org/#/c/31448/3/src/device/pci_rom.c@193 PS3, Line 193: run_rom = (struct rom_header *)(uintptr_t)PCI_RAM_IMAGE_START; line over 80 characters
Hello Mike Banon, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31448
to look at the new patch set (#4).
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 and load OpROM on it. ACPI VFCT table will be created only for discrete VGA instead of integrated, but everything seems to be working fine. Maybe later we would find a way 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 Signed-off-by: Hans Jürgen Kitter eforname@freemail.hu Change-Id: If3269a0911e86ea07014484365c7c830485e52ec --- M src/device/pci_device.c M src/device/pci_rom.c 2 files changed, 67 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/31448/4
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/#/c/31448/4/src/device/pci_rom.c File src/device/pci_rom.c:
https://review.coreboot.org/#/c/31448/4/src/device/pci_rom.c@188 PS4, Line 188: /* Set the start of shadow memory for this Integrated or Discrete VGA. */ line over 80 characters
https://review.coreboot.org/#/c/31448/4/src/device/pci_rom.c@190 PS4, Line 190: run_rom = (struct rom_header *)(uintptr_t)PCI_VGA_RAM_IMAGE_START; line over 80 characters
https://review.coreboot.org/#/c/31448/4/src/device/pci_rom.c@193 PS4, Line 193: run_rom = (struct rom_header *)(uintptr_t)PCI_RAM_IMAGE_START; line over 80 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/#/c/31448/5/src/device/pci_rom.c File src/device/pci_rom.c:
https://review.coreboot.org/#/c/31448/5/src/device/pci_rom.c@188 PS5, Line 188: /* Set the start of shadow memory for this Integrated or Discrete VGA. */ line over 80 characters
https://review.coreboot.org/#/c/31448/5/src/device/pci_rom.c@190 PS5, Line 190: run_rom = (struct rom_header *)(uintptr_t)PCI_VGA_RAM_IMAGE_START; line over 80 characters
https://review.coreboot.org/#/c/31448/5/src/device/pci_rom.c@193 PS5, Line 193: run_rom = (struct rom_header *)(uintptr_t)PCI_RAM_IMAGE_START; line over 80 characters
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 5:
In short, now only Integrated VGA Option ROM will be executed (sorry for this Jenkins spam)
Hello Mike Banon, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31448
to look at the new patch set (#6).
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 and load OpROM on it. ACPI VFCT table will be created only for discrete VGA instead of integrated, but everything seems to be working fine. Maybe later we would find a way 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 Signed-off-by: Hans Jürgen Kitter eforname@freemail.hu Change-Id: If3269a0911e86ea07014484365c7c830485e52ec --- M src/device/pci_device.c M src/device/pci_rom.c 2 files changed, 67 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/31448/6
Hello Mike Banon, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31448
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 some boards like AMD Lenovo G505S. If CONFIG_MULTIPLE_VGA_ADAPTERS is enabled, coreboot will try to initialize the discrete VGA and load OpROM on it. ACPI VFCT table will be created only for discrete VGA instead of integrated, but everything seems to be working fine. Maybe later we would find a way 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 Signed-off-by: Hans Jürgen Kitter eforname@freemail.hu Change-Id: If3269a0911e86ea07014484365c7c830485e52ec --- M src/device/pci_device.c M src/device/pci_rom.c 2 files changed, 67 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/31448/7
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 7:
Please take a look, I rebased this patch and changed IF_ENABLED(CONFIG_X) to CONFIG(X). Also, some time ago we figured out that the existing MULTIPLE_VGA_ADAPTERS code seems to be useless - at least for G505S ( https://review.coreboot.org/c/coreboot/+/31448/#message-86a7c4f33a958847d16f... ) , but there was a suggestion that I should invent a new config for this new code. Please, could you help me to come up with a good name? Or maybe I could rename the old CONFIG_MULTIPLE_VGA_ADAPTERS to another name, to take over this one?
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 8:
(3 comments)
https://review.coreboot.org/#/c/31448/8/src/device/pci_rom.c File src/device/pci_rom.c:
https://review.coreboot.org/#/c/31448/8/src/device/pci_rom.c@188 PS8, Line 188: /* Set the start of shadow memory for this Integrated or Discrete VGA. */ line over 80 characters
https://review.coreboot.org/#/c/31448/8/src/device/pci_rom.c@190 PS8, Line 190: run_rom = (struct rom_header *)(uintptr_t)PCI_VGA_RAM_IMAGE_START; line over 80 characters
https://review.coreboot.org/#/c/31448/8/src/device/pci_rom.c@193 PS8, Line 193: run_rom = (struct rom_header *)(uintptr_t)PCI_RAM_IMAGE_START; line over 80 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 9:
(3 comments)
https://review.coreboot.org/#/c/31448/9/src/device/pci_rom.c File src/device/pci_rom.c:
https://review.coreboot.org/#/c/31448/9/src/device/pci_rom.c@188 PS9, Line 188: /* Set the start of shadow memory for this Integrated or Discrete VGA. */ line over 80 characters
https://review.coreboot.org/#/c/31448/9/src/device/pci_rom.c@190 PS9, Line 190: run_rom = (struct rom_header *)(uintptr_t)PCI_VGA_RAM_IMAGE_START; line over 80 characters
https://review.coreboot.org/#/c/31448/9/src/device/pci_rom.c@193 PS9, Line 193: run_rom = (struct rom_header *)(uintptr_t)PCI_RAM_IMAGE_START; line over 80 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 10:
(3 comments)
https://review.coreboot.org/#/c/31448/10/src/device/pci_rom.c File src/device/pci_rom.c:
https://review.coreboot.org/#/c/31448/10/src/device/pci_rom.c@188 PS10, Line 188: /* Set the start of shadow memory for this Integrated or Discrete VGA. */ line over 80 characters
https://review.coreboot.org/#/c/31448/10/src/device/pci_rom.c@190 PS10, Line 190: run_rom = (struct rom_header *)(uintptr_t)PCI_VGA_RAM_IMAGE_START; line over 80 characters
https://review.coreboot.org/#/c/31448/10/src/device/pci_rom.c@193 PS10, Line 193: run_rom = (struct rom_header *)(uintptr_t)PCI_RAM_IMAGE_START; line over 80 characters
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 10:
(11 comments)
https://review.coreboot.org/#/c/31448/10//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31448/10//COMMIT_MSG@10 PS10, Line 10: CONFIG_MULTIPLE_VGA_ADAPTERS You want to handle non VGA compatible PCI devices in a similar way as VGA compatible devices. This does not have much to do with integrated or discrete.
https://review.coreboot.org/#/c/31448/10/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/#/c/31448/10/src/device/pci_device.c@795 PS10, Line 795: MULTIPLE_VGA_ADAPTERS This name is confusing. All it does is allowing coreboot to initialize PCI_CLASS_DISPLAY_OTHER in a similar fashion to PCI_CLASS_DISLAY?
https://review.coreboot.org/#/c/31448/10/src/device/pci_device.c@830 PS10, Line 830: Integrated use a ternary operator in the printk statement. It would also be better to split off adding these debug messages into a different patch. PCI_CLASS_DISPLAY_VGA means VGA compatible, not integrated or discrete.
https://review.coreboot.org/#/c/31448/10/src/device/pci_device.c@839 PS10, Line 839: Integrated use a ternary operator in the printk statement.
https://review.coreboot.org/#/c/31448/1/src/device/pci_rom.c File src/device/pci_rom.c:
https://review.coreboot.org/#/c/31448/1/src/device/pci_rom.c@190 PS1, Line 190: run_rom = (struct rom_header *)(uintptr_t)PCI_VGA_RAM_IMAGE_START; : } else if (IS_ENABLED(CONFIG_MULTIPLE_VGA_ADAPTERS)) { : if ((dev->class >> 8) == PCI_CLASS_DISPLAY_OTHER) { : run_rom = (struct rom_header *)(uintptr_t)PCI_RAM_IMAGE_START; : } : } use a switch statement for the PCI class.
https://review.coreboot.org/#/c/31448/1/src/device/pci_rom.c@196 PS1, Line 196: : if (run_rom == NULL) : return NULL; isn't this captured by the next statement?
https://review.coreboot.org/#/c/31448/1/src/device/pci_rom.c@231 PS1, Line 231: if (!IS_ENABLED(CONFIG_MULTIPLE_VGA_ADAPTERS)) { : /* Copy Integrated VGA VBIOS from CBFS to ACPI VFCT. */ : printk(BIOS_DEBUG, " Copying %sVBIOS image from %p\n", : rom == (struct rom_header *) : (uintptr_t)PCI_VGA_RAM_IMAGE_START ? : "initialized " : "", : rom); : } else { : /* Copy Discrete VGA VBIOS from CBFS to ACPI VFCT. */ : printk(BIOS_DEBUG, " Copying %sVBIOS image from %p\n", : rom == (struct rom_header *) : (uintptr_t)PCI_RAM_IMAGE_START ? : "initialized " : "", : rom); : /* TODO: do this also for Integrated VGA VBIOS. */ : } isn't this the same?
https://review.coreboot.org/#/c/31448/1/src/device/pci_rom.c@269 PS1, Line 269: if (!IS_ENABLED(CONFIG_MULTIPLE_VGA_ADAPTERS)) { : if ((device->class >> 8) != PCI_CLASS_DISPLAY_VGA) { : return current; : } : } else { : /* Write ACPI VFCT only for Discrete VGA. */ : if ((device->class >> 8) == PCI_CLASS_DISPLAY_OTHER) { : ; : } else { : return current; : } : /* TODO: do this also for Integrated VGA. */ : } use a switch statement.
https://review.coreboot.org/#/c/31448/10/src/device/pci_rom.c File src/device/pci_rom.c:
https://review.coreboot.org/#/c/31448/10/src/device/pci_rom.c@230 PS10, Line 230: if (!CONFIG(MULTIPLE_VGA_ADAPTERS)) { this ought to depend on the device itself, not on Kconfig...
https://review.coreboot.org/#/c/31448/10/src/device/pci_rom.c@231 PS10, Line 231: /* Copy Integrated VGA VBIOS from CBFS to ACPI VFCT. */ There is just a printk statement below, no copying is yet happening.
https://review.coreboot.org/#/c/31448/10/src/device/pci_rom.c@272 PS10, Line 272: /* Write ACPI VFCT only for Discrete VGA. */ : if ((device->class >> 8) == PCI_CLASS_DISPLAY_OTHER) : ; : else : return current; invert the statement.
Hello Mike Banon, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31448
to look at the new patch set (#11).
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 Signed-off-by: Hans Jürgen Kitter eforname@freemail.hu Change-Id: If3269a0911e86ea07014484365c7c830485e52ec --- M src/device/pci_device.c M src/device/pci_rom.c 2 files changed, 69 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/31448/11
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 11:
(7 comments)
https://review.coreboot.org/#/c/31448/11/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/#/c/31448/11/src/device/pci_device.c@830 PS11, Line 830: pci_class == PCI_CLASS_DISPLAY_VGA ? "Integrated" : "Discrete"); line over 80 characters
https://review.coreboot.org/#/c/31448/11/src/device/pci_device.c@837 PS11, Line 837: pci_class == PCI_CLASS_DISPLAY_VGA ? "Integrated" : "Discrete"); line over 80 characters
https://review.coreboot.org/#/c/31448/11/src/device/pci_rom.c File src/device/pci_rom.c:
https://review.coreboot.org/#/c/31448/11/src/device/pci_rom.c@188 PS11, Line 188: /* Set the start of shadow memory for this Integrated or Discrete VGA. */ line over 80 characters
https://review.coreboot.org/#/c/31448/11/src/device/pci_rom.c@189 PS11, Line 189: switch (dev->class >> 8) { switch and case should be at the same indent
https://review.coreboot.org/#/c/31448/11/src/device/pci_rom.c@191 PS11, Line 191: run_rom = (struct rom_header *)(uintptr_t)PCI_VGA_RAM_IMAGE_START; line over 80 characters
https://review.coreboot.org/#/c/31448/11/src/device/pci_rom.c@195 PS11, Line 195: run_rom = (struct rom_header *)(uintptr_t)PCI_RAM_IMAGE_START; line over 80 characters
https://review.coreboot.org/#/c/31448/11/src/device/pci_rom.c@272 PS11, Line 272: switch (device->class >> 8) { switch and case should be at the same indent
Hello Mike Banon, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31448
to look at the new patch set (#12).
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 Signed-off-by: Hans Jürgen Kitter eforname@freemail.hu Change-Id: If3269a0911e86ea07014484365c7c830485e52ec --- M src/device/pci_device.c M src/device/pci_rom.c 2 files changed, 70 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/31448/12
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 12:
(4 comments)
https://review.coreboot.org/#/c/31448/12/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/#/c/31448/12/src/device/pci_device.c@834 PS12, Line 834: pci_class == PCI_CLASS_DISPLAY_VGA ? "Integrated" : "Discrete"); line over 80 characters
https://review.coreboot.org/#/c/31448/12/src/device/pci_device.c@841 PS12, Line 841: pci_class == PCI_CLASS_DISPLAY_VGA ? "Integrated" : "Discrete"); line over 80 characters
https://review.coreboot.org/#/c/31448/12/src/device/pci_rom.c File src/device/pci_rom.c:
https://review.coreboot.org/#/c/31448/12/src/device/pci_rom.c@188 PS12, Line 188: /* Set the start of shadow memory for this Integrated or Discrete VGA. */ line over 80 characters
https://review.coreboot.org/#/c/31448/12/src/device/pci_rom.c@191 PS12, Line 191: run_rom = (struct rom_header *)(uintptr_t)PCI_VGA_RAM_IMAGE_START; line over 80 characters
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 12:
(11 comments)
Arthur, thank you very much for reviewing this patch. I've tried to address your suggestions, please could you take a look?
https://review.coreboot.org/#/c/31448/10//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31448/10//COMMIT_MSG@10 PS10, Line 10: CONFIG_MULTIPLE_VGA_ADAPTERS
You want to handle non VGA compatible PCI devices in a similar way as VGA compatible devices. […]
Changed a commit message to clarify it, i.e. that this "discrete VGA" is of PCI_CLASS_DISPLAY_OTHER class". For some reason the discrete GPUs of Lenovo G505S are of this weird class. More classes could be added later if that could benefit some other coreboot-supported boards.
https://review.coreboot.org/#/c/31448/10/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/#/c/31448/10/src/device/pci_device.c@795 PS10, Line 795: MULTIPLE_VGA_ADAPTERS
This name is confusing. […]
Almost: e.g. if this config is enabled, ACPI VFCT tables will be created only for a "discrete VGA" since creating them for both integrated and discrete currently causes the discrete VGA initialization problems, while not filling them for an "integrated VGA" does not prevent it from functioning.
https://review.coreboot.org/#/c/31448/10/src/device/pci_device.c@830 PS10, Line 830: Integrated
use a ternary operator in the printk statement. […]
Ternary operator done. You are right that "PCI_CLASS_DISPLAY_VGA means VGA compatible, not integrated or discrete", but currently I don't know any other coreboot boards that could benefit from this patch whose discrete VGA is also of the same PCI_CLASS_DISPLAY_VGA class as integrated. So (until this approach is proven wrong) I'm using a device class to distinguish between the integrated and discrete, and these messages reflect it.
https://review.coreboot.org/#/c/31448/10/src/device/pci_device.c@839 PS10, Line 839: Integrated
use a ternary operator in the printk statement.
Done.
https://review.coreboot.org/#/c/31448/1/src/device/pci_rom.c File src/device/pci_rom.c:
https://review.coreboot.org/#/c/31448/1/src/device/pci_rom.c@190 PS1, Line 190: run_rom = (struct rom_header *)(uintptr_t)PCI_VGA_RAM_IMAGE_START; : } else if (IS_ENABLED(CONFIG_MULTIPLE_VGA_ADAPTERS)) { : if ((dev->class >> 8) == PCI_CLASS_DISPLAY_OTHER) { : run_rom = (struct rom_header *)(uintptr_t)PCI_RAM_IMAGE_START; : } : }
use a switch statement for the PCI class.
Done.
https://review.coreboot.org/#/c/31448/1/src/device/pci_rom.c@196 PS1, Line 196: : if (run_rom == NULL) : return NULL;
isn't this captured by the next statement?
Done.
https://review.coreboot.org/#/c/31448/1/src/device/pci_rom.c@231 PS1, Line 231: if (!IS_ENABLED(CONFIG_MULTIPLE_VGA_ADAPTERS)) { : /* Copy Integrated VGA VBIOS from CBFS to ACPI VFCT. */ : printk(BIOS_DEBUG, " Copying %sVBIOS image from %p\n", : rom == (struct rom_header *) : (uintptr_t)PCI_VGA_RAM_IMAGE_START ? : "initialized " : "", : rom); : } else { : /* Copy Discrete VGA VBIOS from CBFS to ACPI VFCT. */ : printk(BIOS_DEBUG, " Copying %sVBIOS image from %p\n", : rom == (struct rom_header *) : (uintptr_t)PCI_RAM_IMAGE_START ? : "initialized " : "", : rom); : /* TODO: do this also for Integrated VGA VBIOS. */ : }
isn't this the same?
there's a small but important difference: PCI_VGA_RAM_IMAGE_START and PCI_RAM_IMAGE_START addresses.
https://review.coreboot.org/#/c/31448/1/src/device/pci_rom.c@269 PS1, Line 269: if (!IS_ENABLED(CONFIG_MULTIPLE_VGA_ADAPTERS)) { : if ((device->class >> 8) != PCI_CLASS_DISPLAY_VGA) { : return current; : } : } else { : /* Write ACPI VFCT only for Discrete VGA. */ : if ((device->class >> 8) == PCI_CLASS_DISPLAY_OTHER) { : ; : } else { : return current; : } : /* TODO: do this also for Integrated VGA. */ : }
use a switch statement.
Done.
https://review.coreboot.org/#/c/31448/10/src/device/pci_rom.c File src/device/pci_rom.c:
https://review.coreboot.org/#/c/31448/10/src/device/pci_rom.c@230 PS10, Line 230: if (!CONFIG(MULTIPLE_VGA_ADAPTERS)) {
this ought to depend on the device itself, not on Kconfig...
Currently G505S is the only device using this MULTIPLE_VGA_ADAPTERS config, and if I remember correctly - someone else said the board-specific configs should be kept outside these global files. Should I still replace it with CONFIG_BOARD_LENOVO_G505S ?
https://review.coreboot.org/#/c/31448/10/src/device/pci_rom.c@231 PS10, Line 231: /* Copy Integrated VGA VBIOS from CBFS to ACPI VFCT. */
There is just a printk statement below, no copying is yet happening.
Done.
https://review.coreboot.org/#/c/31448/10/src/device/pci_rom.c@272 PS10, Line 272: /* Write ACPI VFCT only for Discrete VGA. */ : if ((device->class >> 8) == PCI_CLASS_DISPLAY_OTHER) : ; : else : return current;
invert the statement.
Done.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 12:
(4 comments)
https://review.coreboot.org/#/c/31448/12//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31448/12//COMMIT_MSG@15 PS12, Line 15: Later will try to make a common VFCT table for both adapters. I'd focus on that. This patch just seems like a hack...
https://review.coreboot.org/#/c/31448/10/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/#/c/31448/10/src/device/pci_device.c@795 PS10, Line 795: MULTIPLE_VGA_ADAPTERS
Almost: e.g. […]
It seems like a better idea to get that working instead of this hack... Maybe look at the vendor ACPI code to get a clue of how they do it.
https://review.coreboot.org/#/c/31448/10/src/device/pci_device.c@830 PS10, Line 830: Integrated
Ternary operator done. […]
That is poor reasoning. It's not because on your board one PCI device is VGA compatible and the other not that you can generalize that to discrete/integrated. PCI classes mean exactly what they mean and coreboot code should not invent a new meaning for it.
https://review.coreboot.org/#/c/31448/10/src/device/pci_rom.c File src/device/pci_rom.c:
https://review.coreboot.org/#/c/31448/10/src/device/pci_rom.c@230 PS10, Line 230: if (!CONFIG(MULTIPLE_VGA_ADAPTERS)) {
Currently G505S is the only device using this MULTIPLE_VGA_ADAPTERS config, and if I remember correc […]
No board Kconfigs in here please. What I meant is that it is printing a message depending on a Kconfig while this code get's passed a struct device *. That makes no sense.
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 12:
(2 comments)
https://review.coreboot.org/#/c/31448/10/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/#/c/31448/10/src/device/pci_device.c@830 PS10, Line 830: Integrated
That is poor reasoning. […]
Should I mention a PCI device class instead of Integrated / Discrete? Something like
if (pci_class == PCI_CLASS_DISPLAY_VGA) printk(BIOS_DEBUG, "PCI_CLASS_DISPLAY_VGA ROM probe failed\n"); else printk(BIOS_DEBUG, "PCI_CLASS_DISPLAY_OTHER VGA ROM probe failed\n");
"else" message is valid since we could reach this code only when pci_class is either PCI_CLASS_DISPLAY_VGA or PCI_CLASS_DISPLAY_OTHER with CONFIG(MULTIPLE_VGA_ADAPTERS) enabled.
https://review.coreboot.org/#/c/31448/10/src/device/pci_rom.c File src/device/pci_rom.c:
https://review.coreboot.org/#/c/31448/10/src/device/pci_rom.c@230 PS10, Line 230: if (!CONFIG(MULTIPLE_VGA_ADAPTERS)) {
No board Kconfigs in here please. […]
Thank you, will try to change it without using a Kconfig
Hello Mike Banon, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31448
to look at the new patch set (#13).
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 Signed-off-by: Hans Jürgen Kitter eforname@freemail.hu Change-Id: If3269a0911e86ea07014484365c7c830485e52ec --- M src/device/pci_device.c M src/device/pci_rom.c 2 files changed, 73 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/31448/13
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 13:
(4 comments)
https://review.coreboot.org/#/c/31448/13/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/#/c/31448/13/src/device/pci_device.c@834 PS13, Line 834: pci_class == PCI_CLASS_DISPLAY_VGA ? "Integrated" : "Discrete"); line over 80 characters
https://review.coreboot.org/#/c/31448/13/src/device/pci_device.c@841 PS13, Line 841: pci_class == PCI_CLASS_DISPLAY_VGA ? "Integrated" : "Discrete"); line over 80 characters
https://review.coreboot.org/#/c/31448/13/src/device/pci_rom.c File src/device/pci_rom.c:
https://review.coreboot.org/#/c/31448/13/src/device/pci_rom.c@188 PS13, Line 188: /* Set the start of shadow memory for this Integrated or Discrete VGA. */ line over 80 characters
https://review.coreboot.org/#/c/31448/13/src/device/pci_rom.c@191 PS13, Line 191: run_rom = (struct rom_header *)(uintptr_t)PCI_VGA_RAM_IMAGE_START; line over 80 characters
Hello Mike Banon, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31448
to look at the new patch set (#14).
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 Signed-off-by: Hans Jürgen Kitter eforname@freemail.hu Change-Id: If3269a0911e86ea07014484365c7c830485e52ec --- M src/device/pci_device.c M src/device/pci_rom.c 2 files changed, 75 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/31448/14
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 14:
(2 comments)
https://review.coreboot.org/#/c/31448/14/src/device/pci_rom.c File src/device/pci_rom.c:
https://review.coreboot.org/#/c/31448/14/src/device/pci_rom.c@188 PS14, Line 188: /* Set the start of shadow memory for this Integrated or Discrete VGA. */ line over 80 characters
https://review.coreboot.org/#/c/31448/14/src/device/pci_rom.c@191 PS14, Line 191: run_rom = (struct rom_header *)(uintptr_t)PCI_VGA_RAM_IMAGE_START; line over 80 characters
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/#/c/31448/14/src/device/pci_rom.c File src/device/pci_rom.c:
https://review.coreboot.org/#/c/31448/14/src/device/pci_rom.c@276 PS14, Line 276: 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; So if I understood it correctly only one entry of the VFCT gets used by the radeon driver and this is a hack to make sure the option rom you want gets loaded in VFCT, while the other option rom is at the legacy 0xc0000 location? I'd pursue what is attempted in https://review.coreboot.org/c/coreboot/+/31502 , it seems more promising.
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/#/c/31448/14/src/device/pci_rom.c File src/device/pci_rom.c:
https://review.coreboot.org/#/c/31448/14/src/device/pci_rom.c@276 PS14, Line 276: 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;
So if I understood it correctly only one entry of the VFCT gets used by the radeon driver and this i […]
I don't know how much time it'll take to implement the alternative solution, and for the coreboot users it would be nice if this "hack" could be accepted until the alternative approach is successfully completed. Although then there would be less reasons to develop this alternative better approach ("since the things are already working"), so I could understand the people for not wanting to accept this.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/#/c/31448/14/src/device/pci_rom.c File src/device/pci_rom.c:
https://review.coreboot.org/#/c/31448/14/src/device/pci_rom.c@276 PS14, Line 276: 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;
I don't know how much time it'll take to implement the alternative solution, and for the coreboot us […]
Getting code upstream is about working on nice and maintainable solutions to problems and getting feedback on how to achieve that. Getting hardware to work is nice ofc, but not sufficient reason to accept it upstream. If you don't want to work on it anymore, users can still get your patches.
If you have questions about the suggestions made in https://review.coreboot.org/c/coreboot/+/31502 , I and others are very willing to answer them. It might also be a good idea to look at the vendor ACPI code to figure out what it does?
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/#/c/31448/14/src/device/pci_rom.c File src/device/pci_rom.c:
https://review.coreboot.org/#/c/31448/14/src/device/pci_rom.c@276 PS14, Line 276: 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;
Getting code upstream is about working on nice and maintainable solutions to problems and getting fe […]
I'm still working on this patch (rebasing when it becomes incompatible with upstream + some minor improvements) and updating a set of scripts at CB:31929 to install it, but of course I am interested in CB:31502 - just didn't feel I have the time/skills to pull it off.
Please tell, how to accomplish this - "to look at the vendor ACPI code to figure out what it does?" I could temporarily install a proprietary UEFI, run a set of commands (what commands?) and share their detailed output, and do any other requests helpful for CB:31502 patch
Hello Mike Banon, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31448
to look at the new patch set (#16).
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 Signed-off-by: Hans Jürgen Kitter eforname@freemail.hu Change-Id: If3269a0911e86ea07014484365c7c830485e52ec --- M src/device/pci_device.c M src/device/pci_rom.c 2 files changed, 75 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/31448/16
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 16:
(2 comments)
https://review.coreboot.org/#/c/31448/16/src/device/pci_rom.c File src/device/pci_rom.c:
https://review.coreboot.org/#/c/31448/16/src/device/pci_rom.c@188 PS16, Line 188: /* Set the start of shadow memory for this Integrated or Discrete VGA. */ line over 80 characters
https://review.coreboot.org/#/c/31448/16/src/device/pci_rom.c@191 PS16, Line 191: run_rom = (struct rom_header *)(uintptr_t)PCI_VGA_RAM_IMAGE_START; line over 80 characters
Hello Mike Banon, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31448
to look at the new patch set (#19).
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 Signed-off-by: Hans Jürgen Kitter eforname@freemail.hu Change-Id: If3269a0911e86ea07014484365c7c830485e52ec --- 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/48/31448/19
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 20:
(2 comments)
https://review.coreboot.org/c/coreboot/+/31448/12//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/31448/12//COMMIT_MSG@15 PS12, Line 15: Later will try to make a common VFCT table for both adapters.
I'd focus on that. This patch just seems like a hack...
That's probably for another, future patch...
https://review.coreboot.org/c/coreboot/+/31448/1/src/device/pci_rom.c File src/device/pci_rom.c:
https://review.coreboot.org/c/coreboot/+/31448/1/src/device/pci_rom.c@231 PS1, Line 231: if (!IS_ENABLED(CONFIG_MULTIPLE_VGA_ADAPTERS)) { : /* Copy Integrated VGA VBIOS from CBFS to ACPI VFCT. */ : printk(BIOS_DEBUG, " Copying %sVBIOS image from %p\n", : rom == (struct rom_header *) : (uintptr_t)PCI_VGA_RAM_IMAGE_START ? : "initialized " : "", : rom); : } else { : /* Copy Discrete VGA VBIOS from CBFS to ACPI VFCT. */ : printk(BIOS_DEBUG, " Copying %sVBIOS image from %p\n", : rom == (struct rom_header *) : (uintptr_t)PCI_RAM_IMAGE_START ? : "initialized " : "", : rom); : /* TODO: do this also for Integrated VGA VBIOS. */ : }
there's a small but important difference: PCI_VGA_RAM_IMAGE_START and PCI_RAM_IMAGE_START addresses.
Resolved.
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 20:
(1 comment)
Patch Set 1:
The Kconfig option seems useless.
Can't you simply write two VFCT acpi tables ? Looking at the code that be possible.
Is it safe to load / run two option ROMs ? I'd only load the primary one.
This patch should be split into two parts: One for providing multiple VFCT tables, and one for option ROM loading.
https://review.coreboot.org/c/coreboot/+/31448/10/src/device/pci_rom.c File src/device/pci_rom.c:
https://review.coreboot.org/c/coreboot/+/31448/10/src/device/pci_rom.c@230 PS10, Line 230: if (!CONFIG(MULTIPLE_VGA_ADAPTERS)) {
Thank you, will try to change it without using a Kconfig
This has been done already.
Mike Banon has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Abandoned
Superseeded by CB:38202
-
Arthur Heymans (Code Review) -
build bot (Jenkins) (Code Review)
-
Kyösti Mälkki (Code Review)
-
Mike Banon (Code Review)
-
mikeb mikeb (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)