Re: [PATCH AUTOSEL 6.18-5.10] drm/amd/pm/si: Fix updating clock limits from power states

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incomplete fix: the `radeon` driver still contains the identical bug, leaving laptops using that driver stuck on the lowest power level on battery.
--

commit fcad112d0493cf1ee7c6a33503caa587b8452254
Author: Jeremy Klarenbeek <jeremy.klarenbeek99@gmail.com>
drm/amd/pm/si: Fix updating clock limits from power states

This commit fixes a bug where laptop GPUs were stuck on the lowest possible
clock frequency and power level on battery by updating maximum clock limits
for both AC and DC states in the amdgpu SI DPM code.

> diff --git a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
> index 3a9522c17fee3..828bfb51e9174 100644
> --- a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
> +++ b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c

[ ... ]

> @@ -7285,12 +7286,30 @@ static void si_parse_pplib_clock_info(struct amdgpu_device *adev,
>  		si_pi->mvdd_bootup_value = mvdd;
>  	}
>  
> +	/*
> +	 * Update maximum allowed clock limits.
> +	 * VBIOS can contain conflicting values between:
> +	 * - the maximum allowed clocks and voltages on AC or DC
> +	 * - the clocks and voltages in power states on AC or DC
> +	 */
>  	if ((rps->class & ATOM_PPLIB_CLASSIFICATION_UI_MASK) ==
> -	    ATOM_PPLIB_CLASSIFICATION_UI_PERFORMANCE) {
> -		adev->pm.dpm.dyn_state.max_clock_voltage_on_ac.sclk = pl->sclk;
> -		adev->pm.dpm.dyn_state.max_clock_voltage_on_ac.mclk = pl->mclk;
> -		adev->pm.dpm.dyn_state.max_clock_voltage_on_ac.vddc = pl->vddc;
> -		adev->pm.dpm.dyn_state.max_clock_voltage_on_ac.vddci = pl->vddci;
> +	    ATOM_PPLIB_CLASSIFICATION_UI_PERFORMANCE)
> +		limits = &adev->pm.dpm.dyn_state.max_clock_voltage_on_ac;
> +	else if ((rps->class & ATOM_PPLIB_CLASSIFICATION_UI_MASK) ==
> +		 ATOM_PPLIB_CLASSIFICATION_UI_BATTERY)
> +		limits = &adev->pm.dpm.dyn_state.max_clock_voltage_on_dc;
> +	else
> +		limits = NULL;
> +
> +	if (limits) {
> +		if (pl->sclk > limits->sclk)
> +			limits->sclk = pl->sclk;
> +		if (pl->mclk > limits->mclk)
> +			limits->mclk = pl->mclk;
> +		if (pl->vddc > limits->vddc)
> +			limits->vddc = pl->vddc;
> +		if (pl->vddci > limits->vddci)
> +			limits->vddci = pl->vddci;
>  	}
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does the radeon driver need
this exact same fix?

Looking at drivers/gpu/drm/radeon/si_dpm.c in si_parse_pplib_clock_info()
and drivers/gpu/drm/radeon/ni_dpm.c in ni_parse_pplib_clock_info(), they
both still contain the older, unpatched logic that unconditionally
overwrites AC limits and completely ignores DC/BATTERY limits:

        if ((rps->class & ATOM_PPLIB_CLASSIFICATION_UI_MASK) ==
            ATOM_PPLIB_CLASSIFICATION_UI_PERFORMANCE) {
                rdev->pm.dpm.dyn_state.max_clock_voltage_on_ac.sclk = pl->sclk;
                rdev->pm.dpm.dyn_state.max_clock_voltage_on_ac.mclk = pl->mclk;
                rdev->pm.dpm.dyn_state.max_clock_voltage_on_ac.vddc = pl->vddc;
                rdev->pm.dpm.dyn_state.max_clock_voltage_on_ac.vddci = pl->vddci;
        }

Since the radeon driver is often the default for SI (GCN 1.0) and earlier
GPUs, affected laptops (like those with the FirePro W4170M or Radeon HD
8790M) might still get stuck on the lowest power level on battery when
using that driver.

Would it make sense to port this fix back to the radeon driver as well?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831133314.4125787-205-sashal@kernel.org?part=1