Arthur, thank you very much for reviewing this patch. I've tried to address your suggestions, please could you take a look?
11 comments:
Patch Set #10, 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.
Patch Set #10, 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.
Patch Set #10, 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.
Patch Set #10, Line 839: Integrated
use a ternary operator in the printk statement.
Done.
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.
if (run_rom == NULL)
return NULL;
isn't this captured by the next statement?
Done.
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.
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.
Patch Set #10, 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 ?
Patch Set #10, Line 231: /* Copy Integrated VGA VBIOS from CBFS to ACPI VFCT. */
There is just a printk statement below, no copying is yet happening.
Done.
/* Write ACPI VFCT only for Discrete VGA. */
if ((device->class >> 8) == PCI_CLASS_DISPLAY_OTHER)
;
else
return current;
invert the statement.
Done.
To view, visit change 31448. To unsubscribe, or for help writing mail filters, visit settings.