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 <[email protected]> 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/[email protected]?part=1
