Attention is currently required from: Tim Crawford, Nico Huber, Jeremy Soller, Tim Wawrzynczak, Paul Menzel, Angel Pons, Michael Niewöhner. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/57034 )
Change subject: drivers/gfx/nvidia: Add driver for NVIDIA Optimus ......................................................................
Patch Set 2:
(8 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/57034/comment/3ba4a0c5_f68f5554 PS2, Line 9: NVIDIA Optimus (hybrid) graphics Is there a specific part #?
https://review.coreboot.org/c/coreboot/+/57034/comment/1055b5e7_c901c954 PS2, Line 12: Is there a doc# for the datasheet that details the requirements? I think it would be helpful to add that information to commit message.
File src/drivers/gfx/nvidia/Kconfig:
https://review.coreboot.org/c/coreboot/+/57034/comment/5dae87fb_1916cfe8 PS2, Line 9: default 0x01
Can't this be determined at runtime from the parent device in the devicetree (should be the PCIe RP […]
Yes, this should not be a Kconfig.
File src/drivers/gfx/nvidia/acpi/gpu.asl:
https://review.coreboot.org/c/coreboot/+/57034/comment/d5372bf4_4ff69447 PS2, Line 3: Device (_SB.PCI0.PEGP) { I think this should be generated using soc/intel/common/block/pcie/rtd3. That driver is supposed to generate the ACPI nodes required for power management for PCIe devices.
https://review.coreboot.org/c/coreboot/+/57034/comment/2d19d478_20928694 PS2, Line 37: // Not sure what this stuff is, but it is used to get into GC6 I think this should be based on the datasheet for the dGPU part and/or datasheet for the Intel platform. Also as Angel and Tim commented, this should be done as part of SSDT so that the resource/bus allocation at runtime can be taken into account. Currently, it is not really clear what this code is doing.
https://review.coreboot.org/c/coreboot/+/57034/comment/2b8b4c0b_2a6fab43 PS2, Line 39: CONFIG_MMCONF_BASE_ADDRESS + 0x8000
you mean an SSDT callback? `.acpi_fill_ssdt` ? +1 to this. […]
+1 to SSDT.
File src/drivers/gfx/nvidia/chip.h:
https://review.coreboot.org/c/coreboot/+/57034/comment/1954aec3_2bd5e17c PS2, Line 7: /* TODO: Set GPIOs in devicetree? */
It would also be very useful to generate the Optimus ACPI code using the SSDT generator. […]
+1 for SSDT.
File src/drivers/gfx/nvidia/nvidia.c:
https://review.coreboot.org/c/coreboot/+/57034/comment/c927ce85_4431be81 PS2, Line 13: : // Find all BARs on GPU, mark them above 4g if prefetchable : for (int bar = PCI_BASE_ADDRESS_0; bar <= PCI_BASE_ADDRESS_5; bar += 4) { : struct resource *res = probe_resource(dev, bar); : : if (res) { : if (res->flags & IORESOURCE_PREFETCH) { : printk(BIOS_INFO, " BAR at 0x%02x marked above 4g\n", bar); : res->flags |= IORESOURCE_ABOVE_4G; : } else { : printk(BIOS_DEBUG, " BAR at 0x%02x not prefetch\n", bar); : } : } else { : printk(BIOS_DEBUG, " BAR at 0x%02x not found\n", bar); : } : } : }
Does it make sense to select `PCIEXP_HOTPLUG_PREFETCH_MEM_ABOVE_4G` instead when using the NVIDIA dr […]
Do we also need to consider whether mainboard wants to enable the dGPU pre-OS?
1. If it is required pre-OS, then yes PCIEXP_HOTPLUG_PREFETCH_MEM_ABOVE_4G will have to be set and the device power needs to be enabled early on so that the device is visible during PCIe enumeration in coreboot.
2. If it not required pre-OS, I think we should defer the resource allocation to kernel and device power-on to happen as part of ACPI. This ensures that we do not do unnecessary work in coreboot which will help optimize boot time. In that case, PCIEXP_HOTPLUG_PREFETCH_MEM_ABOVE_4G is also not required.
I think we would want to provide the flexibility to mainboard to determine when the dGPU device should be powered up and when to allocate resources to it.