Re: [PATCH 4/5] drm/amd/pm: allow editing the smu8 overdrive sclk table

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] drm/amd/pm: stale SclkHardMin level after
  smu8_od_upload_sclk_table()
- [Medium] drm/amd/pm: fractional boundary clocks rejected in
  smu8_od_edit_sclk_level()
--

commit f271cff6b522292ee10f53b957a90f73690433ee
Author: Joshua Horne <hornejd@etsu.edu>

drm/amd/pm: allow editing the smu8 overdrive sclk table

This commit implements pp_od_clk_voltage editing, reset, and commit
commands for the SMU8 SCLK OverDrive table on Carrizo and Stoney.

> diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu8_hwmgr.c b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu8_hwmgr.c
> index 93a7e7e176f37..c6045234965c0 100644
> --- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu8_hwmgr.c
> +++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu8_hwmgr.c
> @@ -860,10 +860,86 @@ static void smu8_populate_umdpstate_clocks(struct pp_hwmgr *hwmgr)

[ ... ]

> +static int smu8_od_upload_sclk_table(struct pp_hwmgr *hwmgr)
> +{
> +	struct smu8_hwmgr *data = hwmgr->backend;
> +	const struct phm_odn_clock_levels *od = &data->od_edit_sclk;
> +	struct phm_clock_voltage_dependency_table *vddc_table =
> +				hwmgr->dyn_state.vddc_dependency_on_sclk;
> +	struct phm_clock_voltage_dependency_table *vdd_gfx_table =
> +				hwmgr->dyn_state.vdd_gfx_dependency_on_sclk;
> +	struct pp_power_state *ps;
> +	uint32_t i;
> +
> +	data->od_update_pending = false;
> +
> +	/* the power states refer to the levels by clock, update them first */
> +	ps = hwmgr->ps;
> +	for (i = 0; i < hwmgr->num_ps; i++) {
> +		smu8_od_update_power_state(hwmgr, ps);
> +		ps = (struct pp_power_state *)
> +			((unsigned long)ps + hwmgr->ps_size);
> +	}
> +	smu8_od_update_power_state(hwmgr, hwmgr->current_ps);
> +	smu8_od_update_power_state(hwmgr, hwmgr->request_ps);
> +
> +	/* the NB voltages (GnbVid) of the levels are left untouched */
> +	for (i = 0; i < od->num_of_pl; i++) {
> +		vddc_table->entries[i].clk = od->entries[i].clock;
> +		vdd_gfx_table->entries[i].clk = od->entries[i].clock;
> +		vdd_gfx_table->entries[i].v =
> +			convert_to_vid(od->entries[i].vddc);
> +	}
> +
> +	smu8_populate_umdpstate_clocks(hwmgr);
> +
> +	/*
> +	 * The SMU switches to the new clock and voltage of a level the next
> +	 * time it enters that level.
> +	 */
> +	return smu8_upload_pptable_to_smu(hwmgr);
> +}
> +
>  static int smu8_set_power_state_tasks(struct pp_hwmgr *hwmgr, const void *input)
>  {
> +	struct smu8_hwmgr *data = hwmgr->backend;
>  	int ret = 0;
>  
> +	if (data->od_update_pending) {
> +		ret = smu8_od_upload_sclk_table(hwmgr);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	smu8_update_sclk_limit(hwmgr);

[Severity: Medium]
Does smu8_update_sclk_limit() skip updating PPSMC_MSG_SetSclkHardMin on
the SMU after smu8_od_upload_sclk_table() modifies
vddc_dependency_on_sclk?

smu8_update_sclk_limit() resolves hwmgr->display_config->min_core_set_clock
against vddc_dependency_on_sclk via smu8_get_sclk_level(), but only sends
PPSMC_MSG_SetSclkHardMin when data->sclk_dpm.hard_min_clk differs from
min_core_set_clock:

drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu8_hwmgr.c:smu8_update_sclk_limit() {
	...
	clock = hwmgr->display_config->min_core_set_clock;
	if (clock == 0)
		pr_debug("min_core_set_clock not set\n");

	if (data->sclk_dpm.hard_min_clk != clock) {
		data->sclk_dpm.hard_min_clk = clock;

		smum_send_msg_to_smc_with_parameter(hwmgr,
						PPSMC_MSG_SetSclkHardMin,
						 smu8_get_sclk_level(hwmgr,
					data->sclk_dpm.hard_min_clk,
					     PPSMC_MSG_SetSclkHardMin),
						 NULL);
	}
	...
}

When committing an OverDrive edit while min_core_set_clock is unchanged,
data->sclk_dpm.hard_min_clk already equals clock, so the SMU keeps the old
DPM level index even if the edited frequencies shift which level satisfies
min_core_set_clock, allowing SCLK to drop below the display minimum or
staying pinned higher than needed.  Invalidating
data->sclk_dpm.hard_min_clk in smu8_od_upload_sclk_table() would force
smu8_update_sclk_limit() to resend the updated level.

[ ... ]

> @@ -2057,6 +2133,145 @@ static void smu8_dpm_powergate_vce(struct pp_hwmgr *hwmgr, bool bgate)
>  	}
>  }
>  
> +/* the engine clock that the PLL produces for @clk */
> +static uint32_t smu8_od_real_sclk(struct pp_hwmgr *hwmgr, uint32_t clk)
> +{
> +	pp_atomctrl_clock_dividers_kong dividers;
> +
> +	if (atomctrl_get_engine_pll_dividers_kong(hwmgr, clk, &dividers) ||
> +	    !dividers.real_clock)
> +		return clk;
> +
> +	return dividers.real_clock;
> +}
> +
> +static int smu8_od_edit_sclk_level(struct pp_hwmgr *hwmgr,
> +				   struct phm_odn_clock_levels *od,
> +				   long level, long mhz, long mv)
> +{
> +	struct smu8_hwmgr *data = hwmgr->backend;
> +	struct amdgpu_device *adev = hwmgr->adev;
> +	uint32_t min_clk, max_clk, clk;
> +
> +	min_clk = data->od_default_sclk.entries[0].clock;
> +	max_clk = hwmgr->platform_descriptor.overdriveLimit.engineClock;
> +
> +	if (level < 0 || level >= od->num_of_pl) {
> +		dev_info(adev->dev,
> +			 "OD sclk level %ld is out of range [0 - %u]\n",
> +			 level, od->num_of_pl - 1);
> +		return -EINVAL;
> +	}
> +
> +	if (mhz < min_clk / 100 || mhz > max_clk / 100) {
> +		dev_info(adev->dev,
> +			 "OD engine clock is out of range [%u - %u] MHz\n",
> +			 min_clk / 100, max_clk / 100);
> +		return -EINVAL;
> +	}
> +
> +	if (mv < data->od_min_vddc || mv > data->od_max_vddc) {
> +		dev_info(adev->dev, "OD voltage is out of range [%u - %u] mV\n",
> +			 data->od_min_vddc, data->od_max_vddc);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * OD_SCLK shows whole MHz, so keep the exact clock of a level that is
> +	 * written back unchanged. Otherwise use the clock the PLL produces.
> +	 */
> +	clk = mhz * 100;
> +	if (clk == od->entries[level].clock / 100 * 100)
> +		clk = od->entries[level].clock;
> +	else
> +		clk = smu8_od_real_sclk(hwmgr, clk);
> +
> +	if (clk < min_clk || clk > max_clk) {
> +		dev_info(adev->dev,
> +			 "OD engine clock %u.%02u MHz is out of range\n",
> +			 clk / 100, clk % 100);
> +		return -EINVAL;
> +	}

[Severity: Medium]
Can smu8_od_edit_sclk_level() reject the minimum MHz shown in OD_RANGE when
min_clk has a non-zero fractional MHz part (min_clk % 100 != 0)?

On Carrizo and Stoney, VBIOS clocks are stored in 10 kHz units and often
have fractional MHz values (such as 30476 for 304.76 MHz), while OD_RANGE
and OD_SCLK print truncated whole MHz (min_clk / 100).  Once level 0 is
edited to another frequency, writing back mhz = min_clk / 100 no longer
matches od->entries[level].clock / 100 * 100, so clk = (min_clk / 100) * 100
is passed to smu8_od_real_sclk().  Because that input is already strictly
less than min_clk and ComputeMemoryEnginePLL rounds down to the nearest PLL
step, the resulting clk is below min_clk and fails the clk < min_clk check
with -EINVAL.  Similarly, when max_clk % 100 != 0, writing max_clk / 100
after an edit passes (max_clk / 100) * 100 < max_clk to smu8_od_real_sclk()
and cannot reach the max_clk PLL step.

Checking data->od_default_sclk.entries[level].clock (as well as min_clk and
max_clk) for a whole-MHz match before calling smu8_od_real_sclk() would
allow restoring a level's default or boundary clock.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-smu8-overdrive-v1-0-763f07bdfca9@etsu.edu?part=4