Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] platform/x86: apple-gmux: live mux switch and state desync in gmux_probe() - [Low] platform/x86: apple-gmux: missing kernel-parameters.txt entry for boot_gpu --
commit 67d1397b4ef1b203319bca9aa1d861e7cd7059c6 Author: Atharva Tiwari <[email protected]> platform/apple-gmux: Add parameter to configure boot gpu This commit adds a boot_gpu module parameter to drivers/platform/x86/apple-gmux.c and calls gmux_switchto() during gmux_probe() when boot_gpu is set to 0 or 1. > diff --git a/drivers/platform/x86/apple-gmux.c > b/drivers/platform/x86/apple-gmux.c > index 637b3b216d4e3..6a9580f27b5eb 100644 > --- a/drivers/platform/x86/apple-gmux.c > +++ b/drivers/platform/x86/apple-gmux.c > @@ -109,6 +109,10 @@ struct apple_gmux_config { > char *name; > }; > > +static int boot_gpu = -1; > +module_param(boot_gpu, int, 0); > +MODULE_PARM_DESC(boot_gpu, "Configure the boot gpu (-1 = auto, 0 = iGPU, 1 = > dGPU)"); [Severity: Low] Should this new module parameter also be documented in Documentation/admin-guide/kernel-parameters.txt? Because module_param(boot_gpu, int, 0) sets the sysfs permission to 0, apple_gmux.boot_gpu= is configured via the kernel command line or module load options, and kernel command-line parameters are expected to have a corresponding entry in Documentation/admin-guide/kernel-parameters.txt. > + > #define GMUX_INTERRUPT_ENABLE 0xff > #define GMUX_INTERRUPT_DISABLE 0x00 > > @@ -1215,6 +1219,11 @@ static int gmux_probe(struct pnp_dev *pnp, const > struct pnp_device_id *id) > gmux_enable_interrupts(gmux_data, 1); > gmux_read_switch_state(gmux_data); > > + if (boot_gpu == 0) > + gmux_switchto(VGA_SWITCHEROO_IGD); > + else if (boot_gpu == 1) > + gmux_switchto(VGA_SWITCHEROO_DIS); [Severity: High] Does calling gmux_switchto() unconditionally in gmux_probe() switch the live hardware display mux to an untrained GPU on the current boot and desynchronize the hardware state from vga_switcheroo? Looking at gmux_switchto() in drivers/platform/x86/apple-gmux.c: static int gmux_switchto(enum vga_switcheroo_client_id id) { apple_gmux_data->switch_state_ddc = id; apple_gmux_data->switch_state_display = id; if (apple_gmux_data->external_switchable) apple_gmux_data->switch_state_external = id; gmux_write_switch_state(apple_gmux_data); if (apple_gmux_data->type == APPLE_GMUX_TYPE_MMIO) gmux_write_switch_state_efi(apple_gmux_data); return 0; } Calling gmux_probe() -> gmux_switchto() -> gmux_write_switch_state() immediately writes GMUX_PORT_SWITCH_DDC, GMUX_PORT_SWITCH_DISPLAY, and GMUX_PORT_SWITCH_EXTERNAL. When boot_gpu selects the non-boot GPU, this flips the live panel mux to the secondary GPU that firmware has not trained on the current boot, while vga_default_device() remains pointing to the EFI boot GPU. Because vga_switcheroo_register_client() in drivers/gpu/vga/vga_switcheroo.c determines the active client from vga_default_device(): int vga_switcheroo_register_client(struct pci_dev *pdev, const struct vga_switcheroo_client_ops *ops, bool driver_power_control) { return register_client(pdev, ops, VGA_SWITCHEROO_UNKNOWN_ID, NULL, pdev == vga_default_device(), driver_power_control); } vga_switcheroo still records the EFI boot GPU as active. If a user writes OFF to vgaswitcheroo/switch in vga_switcheroo_debugfs_write(), it powers off the client with !client->active, which is the GPU that gmux_probe() just switched the display mux to. Conversely, if the system booted on the iGPU and boot_gpu=1 is passed to select the discrete GPU for the next reboot, the discrete GPU remains the inactive client on the current boot. When the discrete GPU runtime-suspends, vga_switcheroo_runtime_suspend() in drivers/gpu/vga/vga_switcheroo.c runs: if (vgasr_priv.handler->switchto) { mutex_lock(&vgasr_priv.mux_hw_lock); vgasr_priv.handler->switchto(VGA_SWITCHEROO_IGD); mutex_unlock(&vgasr_priv.mux_hw_lock); } Wouldn't that call gmux_switchto(VGA_SWITCHEROO_IGD) and overwrite both the mux state and the gpu-active EFI variable back to the iGPU before the system reboots? In addition, gmux_switchto() only calls gmux_write_switch_state_efi() when apple_gmux_data->type == APPLE_GMUX_TYPE_MMIO, so on APPLE_GMUX_TYPE_INDEXED systems no EFI variable is written for the next boot. And when the system is already booted on the GPU matching boot_gpu, calling gmux_switchto() unconditionally without checking gmux_data->switch_state_display rewrites the mux ports and the non-volatile gpu-active EFI variable in NVRAM on every boot. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
