Re: [PATCH 06/13] platform/apple-gmux: Add parameter to configure boot gpu

[sashiko-bot@kernel.org]

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 <atharvatiwarilinuxdev@gmail.com>

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/20261025040046.1602-1-atharvatiwarilinuxdev@gmail.com?part=6