Re: [PATCH v2 2/3] drm/amdgpu: add Apple GMUX runtime PM support

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] State leak on runtime PM error paths for GMUX and PX devices.

Pre-existing issues:
- [High] BOCO capability checks incorrectly overlap with PX mode, breaking system suspend.
--

--- Patch [2]: [PATCH v2 2/3] drm/amdgpu: add Apple GMUX runtime PM support ---
commit 0b96cccee1dc38f37893eddd321b413c6829882e
Author: Andre Eikmeyer <[email protected]>
Subject: drm/amdgpu: add Apple GMUX runtime PM support

This commit adds runtime PM support for the discrete GPU on the Apple
MacBookPro15,1 using the vga_switcheroo PM domain. It allows the dGPU
to enter D3cold while the iGPU is primary and wake on demand.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 0ab380ca7e645..a231a002831bb 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 */
>  		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)

[Severity: High]
This is a pre-existing issue, but does this direct capability check for BOCO
incorrectly evaluate to true for hybrid devices? If a device supports both
PX (ATPX) and BOCO capabilities, it seems amdgpu_device_supports_boco()
will return true in amdgpu_pci_probe(), overwriting the
DPM_FLAG_NO_DIRECT_COMPLETE flag just set for PX.

>  			dev_pm_set_driver_flags(ddev->dev, DPM_FLAG_SMART_PREPARE |
>  						DPM_FLAG_SMART_SUSPEND |
>  						DPM_FLAG_MAY_SKIP_RESUME);
[ ... ]
> @@ -2609,7 +2610,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))

[Severity: High]
This is a pre-existing issue, but does this capability check erroneously
instruct the PM core to skip system suspend and resume for a PX-designated
device? If the device supports both PX and BOCO, returning 1 here in
amdgpu_pmops_prepare() will skip system suspend, which ATPX explicitly
requires.

>  		return 1;
>  
>  	/* if we will not support s3 or s2i for the device
[ ... ]
> @@ -2872,7 +2875,8 @@ static int amdgpu_pmops_runtime_suspend(struct device *dev)
>  	}
>  
>  	adev->in_runpm = true;
> -	if (adev->pm.rpm_mode == AMDGPU_RUNPM_PX)
> +	if (adev->pm.rpm_mode == AMDGPU_RUNPM_PX ||
> +	    adev->pm.rpm_mode == AMDGPU_RUNPM_GMUX)
>  		drm_dev->switch_power_state = DRM_SWITCH_POWER_CHANGING;
>  
>  	/*
[ ... ]
>  	ret = amdgpu_device_prepare(drm_dev);
>  	if (ret)
>  		return ret;
>  	ret = amdgpu_device_suspend(drm_dev, false);
>  	if (ret) {
>  		adev->in_runpm = false;
>  		if (adev->pm.rpm_mode == AMDGPU_RUNPM_BOCO)
>  			adev->mp1_state = PP_MP1_STATE_NONE;
>  		return ret;
>  	}

[Severity: Medium]
If amdgpu_device_suspend() fails later in amdgpu_pmops_runtime_suspend(),
is drm_dev->switch_power_state left permanently in
DRM_SWITCH_POWER_CHANGING? 

The early return on failure does not appear to restore the previous power
state, and this state leak now applies to the GMUX path as well.

[ ... ]
> @@ -2947,10 +2952,11 @@ static int amdgpu_pmops_runtime_resume(struct device *dev)
>  	if (!pci_device_is_present(adev->pdev))
>  		adev->no_hw_access = true;
>  
> -	if (adev->pm.rpm_mode == AMDGPU_RUNPM_PX) {
> +	if (adev->pm.rpm_mode == AMDGPU_RUNPM_PX ||
> +	    adev->pm.rpm_mode == AMDGPU_RUNPM_GMUX) {
>  		drm_dev->switch_power_state = DRM_SWITCH_POWER_CHANGING;
>  
> -		/* Only need to handle PCI state in the driver for ATPX
> +		/* Only need to handle PCI state in the driver for ATPX and GMUX.
>  		 * PCI core handles it for _PR3.
>  		 */
>  		pci_set_power_state(pdev, PCI_D0);
> @@ -2970,12 +2976,14 @@ static int amdgpu_pmops_runtime_resume(struct device *dev)
>  	}
>  	ret = amdgpu_device_resume(drm_dev, false);
>  	if (ret) {
> -		if (adev->pm.rpm_mode == AMDGPU_RUNPM_PX)
> +		if (adev->pm.rpm_mode == AMDGPU_RUNPM_PX ||
> +		    adev->pm.rpm_mode == AMDGPU_RUNPM_GMUX)
>  			pci_disable_device(pdev);
>  		return ret;
>  	}

[Severity: Medium]
If either pci_enable_device() or amdgpu_device_resume() fails and returns
early in amdgpu_pmops_runtime_resume(), does this leave
drm_dev->switch_power_state permanently set to DRM_SWITCH_POWER_CHANGING,
since the failure paths do not restore it?

> -	if (adev->pm.rpm_mode == AMDGPU_RUNPM_PX)
> +	if (adev->pm.rpm_mode == AMDGPU_RUNPM_PX ||
> +	    adev->pm.rpm_mode == AMDGPU_RUNPM_GMUX)
>  		drm_dev->switch_power_state = DRM_SWITCH_POWER_ON;
>  
>  	amdgpu_restore_umd_profile_pstate_after_runpm(adev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2