Re: [PATCH v2 08/12] drm/amdgpu: Add Apple GMUX runtime PM support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] drm/amdgpu: missing switcheroo readiness checks in
  amdgpu_device_supports_gmux()
- [High] drm/amdgpu: broken smart-suspend handling for GMUX in
  amdgpu_pci_probe()
- [High] drm/amdgpu: NULL deref on shutdown when GMUX GPU is
  runtime-suspended
- [Medium] drm/amdgpu: amdgpu_device_init() ignores runpm param for GMUX
  switcheroo
--

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

drm/amdgpu: Add Apple GMUX runtime PM support

This commit adds runtime PM support for T2 MacBook Pro discrete GPUs
using the vga_switcheroo PM domain and skips initial eDP detection when
the GMUX GPU is not the boot display device.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 9269e780feb73..ac336594bf055 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
[ ... ]
> @@ -570,6 +571,46 @@ 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) &&
> +	       !pci_is_thunderbolt_attached(adev->pdev) &&
> +	       !dev_is_removable(&adev->pdev->dev) &&
> +	       apple_gmux_detect(NULL, NULL);
> +}

[Severity: High]
Can amdgpu_device_supports_gmux() return true before the vga_switcheroo
handler and peer GPU client are registered?

When the system boots with the discrete GPU as the default VGA device
(pdev == vga_default_device()), vga_switcheroo_client_probe_defer() does
not defer probing even if apple-gmux has not registered its handler yet.
Because apple_gmux_detect(NULL, NULL) only checks the PNP MMIO resource,
amdgpu attaches vga_pm_domain and enables runtime autosuspend; when the
autosuspend timer fires while vgasr_priv.handler is NULL,
vga_switcheroo_runtime_suspend() dereferences vgasr_priv.handler->switchto
and crashes:

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);
	...
}

Even after apple-gmux registers its handler,
vga_switcheroo_client_probe_defer() does not wait for the integrated GPU
client (i915) to register, so vga_switcheroo_enable() has not run and
gmux_set_discrete_state() fails with -ENODEV during runtime suspend
without saving bridge state or cutting power.  Also, unlike
amdgpu_device_supports_px(), amdgpu_device_supports_gmux() does not check
IS_ENABLED(CONFIG_VGA_SWITCHEROO), and calling
vga_switcheroo_client_probe_defer() unconditionally in amdgpu_pci_probe()
defers Thunderbolt eGPUs or systems with stub APP000B ACPI devices where
gmux_probe() returns -ENODEV.

[ ... ]

> @@ -4202,12 +4246,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]
Does enabling driver_power_control and attaching vga_pm_domain here ignore
the amdgpu.runpm module parameter?

amdgpu_device_init() runs before amdgpu_device_detect_runtime_pm_mode(),
which only selects AMDGPU_RUNPM_GMUX when amdgpu_runtime_pm is -1 or -2.
Booting with amdgpu.runpm=0 leaves runtime PM disabled while
driver_power_control is true, turning manual vga_switcheroo OFF/ON
commands into no-ops.  Booting with amdgpu.runpm=1 or 2 selects
AMDGPU_RUNPM_BACO or AMDGPU_RUNPM_BAMACO while vga_pm_domain is still
attached, so runtime suspend enters BACO without caching PCI state or
setting PP_MP1_STATE_UNLOAD before vga_switcheroo_runtime_suspend() cuts
power via GMUX.

[Severity: High]
Can pci_device_shutdown() trigger a NULL pointer dereference in
vga_switcheroo_power_switch() when the GMUX GPU is runtime-suspended at
shutdown?

PNP devices are registered after PCI devices in devices_kset, and no
device_link orders the PCI GPU before the apple-gmux PNP device.  During
device_shutdown(), gmux_shutdown() -> gmux_remove() ->
vga_switcheroo_unregister_handler() runs first and sets
vgasr_priv.handler = NULL while the GPU remains in D3cold.  When
pci_device_shutdown() then runs on the GPU, it calls pm_runtime_resume()
-> vga_switcheroo_runtime_resume() -> vga_switcheroo_power_switch(), which
dereferences vgasr_priv.handler->power_state:

drivers/gpu/vga/vga_switcheroo.c:vga_switcheroo_power_switch() {
	if (!vgasr_priv.handler->power_state)
		return;
	...
}

In addition, amdgpu_pci_shutdown() then calls amdgpu_device_prepare() and
amdgpu_device_suspend(), which only check for DRM_SWITCH_POWER_OFF (not
DRM_SWITCH_POWER_DYNAMIC_OFF) and access hardware on the powered-off GPU.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index d9d794a7db964..3978ed284048f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
[ ... ]
> @@ -2494,11 +2497,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 +2614,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]
What happens during system sleep or hibernation when direct_complete is
not used on a runtime-suspended GMUX GPU?

When the HDA audio consumer (snd_hda_intel) is bound, its child PCM
devices clear direct_complete during dpm_prepare() after
amdgpu_pmops_prepare() on the supplier GPU has already returned 1 and
skipped amdgpu_device_prepare().  In dpm_suspend(), pci_pm_suspend() on
the HDA device runtime-resumes the GMUX GPU and clears its
direct_complete, so amdgpu_pmops_suspend() -> amdgpu_device_suspend() runs
without amdgpu_device_prepare() having executed, and because
vga_pm_domain only implements runtime PM callbacks, GMUX never powers off
the GPU for system sleep.

Conversely, if direct_complete is cleared while the GMUX GPU stays
runtime-suspended (for example via a child backlight device, wakeup
settings, or during hibernation in pci_pm_poweroff()), DPM_FLAG_SMART_SUSPEND
causes pci_pm_suspend() and pci_pm_poweroff() to skip pm_runtime_resume()
and call amdgpu_device_suspend() while the GPU is powered off in
DRM_SWITCH_POWER_DYNAMIC_OFF.  On resume, pci_pm_resume() sees
pci_dev->state_saved == true from runtime suspend and calls
amdgpu_pmops_resume() while GMUX still has power cut, where
pci_device_is_present() reads 0xffff, sets adev->no_hw_access = true, and
runs amdgpu_device_resume() on the powered-off device.

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