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.