Re: [PATCH 09/13] drm/amdgpu: Add Apple GMUX runtime PM support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] drm/amdgpu: missing thunderbolt check in amdgpu_device_supports_gmux()
- [High] drm/amdgpu: NULL deref on GMUX runtime suspend if handler or iGPU not ready
- [High] drm/amdgpu: broken smart-suspend handling for GMUX in amdgpu_pci_probe()
- [Medium] drm/amdgpu: unconditional GMUX switcheroo runtime PM setup ignores runpm param
--

commit f1610061faecf7abbb15383a149d6591a343aafc
Author: Andre Eikmeyer <dev@deq.rocks>

drm/amdgpu: Add Apple GMUX runtime PM support

This commit adds runtime power management support for discrete GPUs on T2
MacBook Pro models controlled via Apple gmux using the vga_switcheroo power
domain. It also marks inactive gmux eDP panels as disconnected prior to
Display Core initialization so that disconnected internal panels do not hold
the discrete GPU runtime active.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 9269e780feb73..c1f1962425fee 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
[ ... ]
> @@ -570,6 +571,45 @@ bool amdgpu_device_supports_px(struct amdgpu_device *adev)
>  	return false;
>  }
>  
> +static const struct dmi_system_id amdgpu_gmux_runpm_dmi_table[] = {
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro15,1"),
> +		},
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro15,3"),
> +		},
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro16,1"),
> +		},
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro16,4"),
> +		},
> +	},
> +	{ }
> +};
> +
> +bool amdgpu_device_supports_gmux(struct amdgpu_device *adev)
> +{
> +	return dmi_check_system(amdgpu_gmux_runpm_dmi_table) &&
> +	       !dev_is_removable(&adev->pdev->dev) &&

[Severity: High]
Should amdgpu_device_supports_gmux() also check
!pci_is_thunderbolt_attached(adev->pdev)?

On MacBook Pro systems, Intel Thunderbolt hierarchies are identified via
pci_is_thunderbolt_attached(adev->pdev) rather than the ExternalFacingPort
property that sets dev_is_removable() (as seen in commit 8587d48d694d and in
amdgpu_device_should_register_switcheroo()):

drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:amdgpu_device_should_register_switcheroo() {
	return !pci_is_thunderbolt_attached(adev->pdev) &&
	       (px || (!dev_is_removable(&adev->pdev->dev) &&
		       apple_gmux_detect(NULL, NULL)));
}

Because dmi_check_system() and apple_gmux_detect(NULL, NULL) are host-wide
checks, connecting a Thunderbolt external GPU to a MacBookPro15,1, 15,3,
16,1, or 16,4 causes amdgpu_device_supports_gmux() to return true for the
external GPU while amdgpu_device_should_register_switcheroo() returns false.

In amdgpu_device_init(), this skips vga_switcheroo_register_client() for the
Thunderbolt GPU, but still executes vga_switcheroo_init_domain_pm_ops()
because gmux is true, binding the external GPU to vga_switcheroo's power
domain without registering it as a switcheroo client. In addition,
amdgpu_device_has_gmux_edp() returns true for the external GPU on
MacBookPro15,3, 16,1, and 16,4, causing amdgpu_dm_init() to set
init_data.flags.edp_not_connected = true and
amdgpu_dm_initialize_drm_device() to skip eDP detection on the external GPU.

> +	       apple_gmux_detect(NULL, NULL);
> +}

[Severity: High]
Can this trigger a NULL pointer dereference when the 5-second runtime
autosuspend timer fires after amdgpu_pci_probe() if the apple-gmux handler or
the integrated GPU client has not been registered yet?

The inline helper apple_gmux_detect(NULL, NULL) only checks for hardware
presence in ACPI/PNP and returns true even if the apple-gmux driver
(CONFIG_APPLE_GMUX) has not probed or registered a vga_switcheroo handler.
Because amdgpu_pci_probe() does not call vga_switcheroo_client_probe_defer()
before enabling runtime autosuspend, two failure paths can occur when
vga_switcheroo_runtime_suspend() runs:

1. If apple-gmux has not probed yet (for instance, when built as a module and
not loaded in initramfs, or if its probe failed), vgasr_priv.handler is NULL
when vga_switcheroo_runtime_suspend() dereferences it:

drivers/gpu/vga/vga_switcheroo.c:vga_switcheroo_runtime_suspend() {
	...
	mutex_lock(&vgasr_mutex);
	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);
	}
	pci_bus_set_current_state(pdev->bus, PCI_D3cold);
	vga_switcheroo_power_switch(pdev, VGA_SWITCHEROO_OFF);
	mutex_unlock(&vgasr_mutex);
	...
}

2. Even if apple-gmux has registered its handler, vga_switcheroo_enable()
only runs and invokes gmux_get_client_id() (which is the only place that
populates gmux_data->discrete_pdev) once two vga_switcheroo clients have
registered. If i915 has not probed or is disabled when amdgpu autosuspends,
client->id remains VGA_SWITCHEROO_UNKNOWN_ID, and
vga_switcheroo_power_switch() -> gmux_set_power_state() ->
gmux_set_discrete_state() dereferences gmux_data->discrete_pdev while it is
NULL:

drivers/platform/x86/apple-gmux.c:gmux_set_discrete_state() {
	reinit_completion(&gmux_data->powerchange_done);
	acpi_handle discrete_handle = ACPI_HANDLE(&gmux_data->discrete_pdev->dev);
	...
}

[ ... ]
> @@ -4202,12 +4245,13 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>  		vga_client_register(adev->pdev, amdgpu_device_vga_set_decode);
>  
>  	px = amdgpu_device_supports_px(adev);
> +	gmux = amdgpu_device_supports_gmux(adev);
>  
>  	if (amdgpu_device_should_register_switcheroo(adev, px))
>  		vga_switcheroo_register_client(adev->pdev,
> -					       &amdgpu_switcheroo_ops, px);
> +					       &amdgpu_switcheroo_ops, px || gmux);
>  
> -	if (px)
> +	if (px || gmux)
>  		vga_switcheroo_init_domain_pm_ops(adev->dev, &adev->vga_pm_domain);

[Severity: Medium]
What happens here when the module parameter amdgpu.runpm is set to 0, 1, or 2
on a supported MacBook Pro?

In amdgpu_driver_load_kms(), amdgpu_device_init() runs before
amdgpu_device_detect_runtime_pm_mode() evaluates amdgpu_runtime_pm:

drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c:amdgpu_driver_load_kms() {
	...
	r = amdgpu_device_init(adev, flags);
	if (r) {
		dev_err(dev->dev, "Fatal error during GPU init\n");
		goto out;
	}

	amdgpu_device_detect_runtime_pm_mode(adev);
	...
}

Because amdgpu_device_init() sets driver_power_control to true (px || gmux)
and attaches adev->vga_pm_domain unconditionally whenever
amdgpu_device_supports_gmux(adev) is true:

1. With amdgpu.runpm=0, amdgpu_device_detect_runtime_pm_mode() leaves
adev->pm.rpm_mode as AMDGPU_RUNPM_NONE, so amdgpu_pci_probe() does not enable
runtime power management. At the same time, vga_switchoff() and
vga_switchon() in drivers/gpu/vga/vga_switcheroo.c return early when
client->driver_power_control is true, disabling manual vga_switcheroo power
control as well.

2. With amdgpu.runpm=1 or amdgpu.runpm=2 on an ASIC that reports
BACO_SUPPORT or MACO_SUPPORT, amdgpu_device_detect_runtime_pm_mode() selects
AMDGPU_RUNPM_BACO or AMDGPU_RUNPM_BAMACO while vga_pm_domain remains attached
to adev->dev. On runtime suspend, amdgpu_pmops_runtime_suspend() enters BACO
without saving PCI state or disabling the PCI device, and
vga_switcheroo_runtime_suspend() then cuts power to the GPU and bridge via
gmux.

[ ... ]
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index d9d794a7db964..5ad7b6dd8b2b5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -2494,11 +2494,12 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
>  		DRM_ERROR("Creating debugfs files failed (%d).\n", ret);
>  
>  	if (adev->pm.rpm_mode != AMDGPU_RUNPM_NONE) {
> -		/* only need to skip on ATPX */
> +		/* ATPX requires a full system-sleep transition. */
>  		if (amdgpu_device_supports_px(adev))
>  			dev_pm_set_driver_flags(ddev->dev, DPM_FLAG_NO_DIRECT_COMPLETE);
> -		/* we want direct complete for BOCO */
> -		if (amdgpu_device_supports_boco(adev))
> +		/* BOCO and GMUX can remain runtime suspended across system sleep. */
> +		if (amdgpu_device_supports_boco(adev) ||
> +		    adev->pm.rpm_mode == AMDGPU_RUNPM_GMUX)
>  			dev_pm_set_driver_flags(ddev->dev, DPM_FLAG_SMART_PREPARE |
>  						DPM_FLAG_SMART_SUSPEND |
>  						DPM_FLAG_MAY_SKIP_RESUME);
> @@ -2610,7 +2611,9 @@ static int amdgpu_pmops_prepare(struct device *dev)
>  	/* Return a positive number here so
>  	 * DPM_FLAG_SMART_SUSPEND works properly
>  	 */
> -	if (amdgpu_device_supports_boco(adev) && pm_runtime_suspended(dev))
> +	if ((amdgpu_device_supports_boco(adev) ||
> +	     adev->pm.rpm_mode == AMDGPU_RUNPM_GMUX) &&
> +	    pm_runtime_suspended(dev))
>  		return 1;

[Severity: High]
Is setting DPM_FLAG_SMART_PREPARE, DPM_FLAG_SMART_SUSPEND, and
DPM_FLAG_MAY_SKIP_RESUME safe for AMDGPU_RUNPM_GMUX when direct_complete is
not used during system sleep or hibernation?

When a gmux discrete GPU is runtime-suspended, amdgpu_pmops_runtime_suspend()
caches PCI state (setting pdev->state_saved = true), sets pdev->current_state
to PCI_D3cold, and sets drm_dev->switch_power_state to
DRM_SWITCH_POWER_DYNAMIC_OFF, while vga_switcheroo_runtime_suspend() cuts
power via apple-gmux (and powers down the PCIe bridge on MacBookPro16,1 and
16,4).

During system sleep or hibernation, direct_complete can be cleared in two
ways, neither of which is handled by vga_pm_domain or the amdgpu system sleep
callbacks:

1. When the companion HDA audio device (linked as a consumer to the discrete
GPU with DL_FLAG_PM_RUNTIME by quirk_gpu_hda()) is bound to snd_hda_intel,
its child PCM devices have .prepare = NULL and .suspend = do_pcm_suspend,
which clears direct_complete up to the HDA PCI device via
dpm_clear_superiors_direct_complete(). In pci_pm_suspend(), because
snd_hda_intel does not set DPM_FLAG_SMART_SUSPEND, pm_runtime_resume() is
called on the HDA device, which via rpm_get_suppliers() runtime-resumes the
gmux GPU during dpm_suspend() after amdgpu_pmops_prepare() already returned 1
and skipped amdgpu_device_prepare(). Then dpm_clear_superiors_direct_complete()
clears direct_complete on the GPU, so amdgpu_pmops_suspend() calls
amdgpu_device_suspend() without amdgpu_device_prepare() having run, and
because vga_pm_domain only implements runtime PM callbacks, gmux never cuts
power to the GPU for system sleep.

2. If direct_complete is false while the gmux GPU stays runtime-suspended
(for example, during hibernation in pci_pm_poweroff(), when
device_may_wakeup() is enabled, when a child amdgpu_bl backlight device whose
class PM ops have .prepare = NULL clears direct_complete, or when
snd_hda_intel is unbound), pci_pm_suspend() and pci_pm_poweroff() evaluate
(!dev_pm_smart_suspend(dev) || pci_dev_need_resume(pci_dev)) as false because
DPM_FLAG_SMART_SUSPEND is set and pci_dev_need_resume() is false for a
non-ACPI PCI device already in PCI_D3cold. They therefore skip
pm_runtime_resume() and invoke amdgpu_pmops_suspend() or
amdgpu_pmops_poweroff() -> amdgpu_device_suspend() while the GPU is powered
off:

drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:amdgpu_device_suspend() {
	if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
		return 0;
	...
}

Because amdgpu_device_suspend() only checks for DRM_SWITCH_POWER_OFF (and not
DRM_SWITCH_POWER_DYNAMIC_OFF or pm_runtime_suspended(dev)), it proceeds to
access registers on the powered-off GPU. On the subsequent resume path,
pci_pm_resume() (which does not check dev_pm_skip_resume()) sees
pci_dev->state_saved == true from runtime suspend, attempts
pci_restore_standard_config() on the powered-off device, and calls
amdgpu_pmops_resume(), where pci_device_is_present() reads 0xffff, sets
adev->no_hw_access = true, and runs amdgpu_device_resume() while gmux still
has power cut.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261025040046.1602-1-atharvatiwarilinuxdev@gmail.com?part=9