Re: [PATCH v4 17/19] cpufreq/amd-pstate: Rework CPPC enabling

[Mario Limonciello <superm1@kernel.org>]

>> +	/* Enable autonomous mode for EPP */
>> +	if (cppc_state == AMD_PSTATE_ACTIVE) {
>> +		/* Set desired perf as zero to allow EPP firmware control */
>> +		perf_ctrls.desired_perf = 0;
>> +		ret = cppc_set_perf(policy->cpu, &perf_ctrls);
> 
> I'm thinking do we need this "setting of desired_perf" as a part of shmem_cppc_enable,
> one thing is we're not doing it in the "msr_" counterpart
> also, I guess this would be taken care as part of amd_pstate_epp_set_policy()->amd_pstate_epp_update_limit()->amd_pstate_update_perf()

Great point, agreed will drop it.

> 
>>   	}
>>   
>> -	cppc_enabled = enable;
>>   	return ret;
>>   }
>>   
>>   DEFINE_STATIC_CALL(amd_pstate_cppc_enable, msr_cppc_enable);
>>   
>> -static inline int amd_pstate_cppc_enable(bool enable)
>> +static inline int amd_pstate_cppc_enable(struct cpufreq_policy *policy)
>>   {
>> -	return static_call(amd_pstate_cppc_enable)(enable);
>> +	return static_call(amd_pstate_cppc_enable)(policy);
>>   }
>>   
>>   static int msr_init_perf(struct amd_cpudata *cpudata)
>> @@ -1115,28 +1060,6 @@ static void amd_pstate_cpu_exit(struct cpufreq_policy *policy)
>>   	kfree(cpudata);
>>   }
>>   
>> -static int amd_pstate_cpu_resume(struct cpufreq_policy *policy)
>> -{
>> -	int ret;
>> -
>> -	ret = amd_pstate_cppc_enable(true);
>> -	if (ret)
>> -		pr_err("failed to enable amd-pstate during resume, return %d\n", ret);
>> -
>> -	return ret;
>> -}
>> -
>> -static int amd_pstate_cpu_suspend(struct cpufreq_policy *policy)
>> -{
>> -	int ret;
>> -
>> -	ret = amd_pstate_cppc_enable(false);
>> -	if (ret)
>> -		pr_err("failed to disable amd-pstate during suspend, return %d\n", ret);
>> -
>> -	return ret;
>> -}
>> -
>>   /* Sysfs attributes */
>>   
>>   /*
>> @@ -1228,8 +1151,10 @@ static ssize_t show_energy_performance_available_preferences(
>>   static ssize_t store_energy_performance_preference(
>>   		struct cpufreq_policy *policy, const char *buf, size_t count)
>>   {
>> +	struct amd_cpudata *cpudata = policy->driver_data;
>>   	char str_preference[21];
>>   	ssize_t ret;
>> +	u8 epp;
>>   
>>   	ret = sscanf(buf, "%20s", str_preference);
>>   	if (ret != 1)
>> @@ -1239,7 +1164,29 @@ static ssize_t store_energy_performance_preference(
>>   	if (ret < 0)
>>   		return -EINVAL;
>>   
>> -	ret = amd_pstate_set_energy_pref_index(policy, ret);
>> +	if (!ret)
>> +		epp = cpudata->epp_default;
>> +	else
>> +		epp = epp_values[ret];
>> +
>> +	if (epp > 0 && policy->policy == CPUFREQ_POLICY_PERFORMANCE) {
>> +		pr_debug("EPP cannot be set under performance policy\n");
>> +		return -EBUSY;
>> +	}
>> +
>> +	if (trace_amd_pstate_epp_perf_enabled()) {
>> +		union perf_cached perf = cpudata->perf;
>> +
>> +		trace_amd_pstate_epp_perf(cpudata->cpu, perf.highest_perf,
>> +					  epp,
>> +					  FIELD_GET(AMD_CPPC_MIN_PERF_MASK, cpudata->cppc_req_cached),
>> +					  FIELD_GET(AMD_CPPC_MAX_PERF_MASK, cpudata->cppc_req_cached),
>> +					  policy->boost_enabled,
>> +					  FIELD_GET(AMD_CPPC_EPP_PERF_MASK,
>> +						    cpudata->cppc_req_cached) != epp);
> 
> We are doing the tracing in amd_pstate_set_epp() as well right?, Isnt this one redundant?

Yup! Great catch.

> 
>> +	}
>> +
>> +	ret = amd_pstate_set_epp(policy, epp);
>>   
>>   	return ret ? ret : count;
>>   }
>> @@ -1272,7 +1219,6 @@ static ssize_t show_energy_performance_preference(
>>   
>>   static void amd_pstate_driver_cleanup(void)
>>   {
>> -	amd_pstate_cppc_enable(false);
>>   	cppc_state = AMD_PSTATE_DISABLE;
>>   	current_pstate_driver = NULL;
>>   }
>> @@ -1306,14 +1252,6 @@ static int amd_pstate_register_driver(int mode)
>>   
>>   	cppc_state = mode;
>>   
>> -	ret = amd_pstate_cppc_enable(true);
>> -	if (ret) {
>> -		pr_err("failed to enable cppc during amd-pstate driver registration, return %d\n",
>> -		       ret);
>> -		amd_pstate_driver_cleanup();
>> -		return ret;
>> -	}
>> -
>>   	/* at least one CPU supports CPB */
>>   	current_pstate_driver->boost_enabled = cpu_feature_enabled(X86_FEATURE_CPB);
>>   
>> @@ -1553,11 +1491,15 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
>>   	policy->cpuinfo.max_freq = policy->max = perf_to_freq(perf,
>>   							      cpudata->nominal_freq,
>>   							      perf.highest_perf);
>> +	policy->driver_data = cpudata;
>> +
>> +	ret = amd_pstate_cppc_enable(policy);
> 
> I think we missed cppc_enable in "amd_pstate_cpu_init". Confirmed this by booting with "amd_pstate=passive"

Yeah; true.  I will add this.

> 
> Also, one weird behavior I saw while testing this part, if we boot with "amd_pstate=passive"
> initially, the MSR_AMD_CPPC_ENABLE register is 0. But after I run the amd-pstate-ut (which fails
> the check_amd_pstate_enabled() test the first time) the MSR_AMD_CPPC_ENABLE gets set to 1. But I
> didnt see any code in amd-pstate-ut that sets it. We can ignore this quirk for now, just
> mentioned to see if you have any idea about this.

amd-pstate-ut will change modes, so I expect this is the reason it 
happens.  It's also the reason I didn't catch it.  I always started in 
active when I was testing switching to passive.

> 
>> +	if (ret)
>> +		goto free_cpudata1;
>>   
>>   	/* It will be updated by governor */
>>   	policy->cur = policy->cpuinfo.min_freq;
>>   
>> -	policy->driver_data = cpudata;
>>   
>>   	policy->boost_enabled = READ_ONCE(cpudata->boost_supported);
>>   
>> @@ -1649,31 +1591,21 @@ static int amd_pstate_epp_set_policy(struct cpufreq_policy *policy)
>>   	return 0;
>>   }
>>   
>> -static int amd_pstate_epp_reenable(struct cpufreq_policy *policy)
>> -{
>> -	int ret;
>> -
>> -	ret = amd_pstate_cppc_enable(true);
>> -	if (ret)
>> -		pr_err("failed to enable amd pstate during resume, return %d\n", ret);
>> -
>> -
>> -	return amd_pstate_epp_update_limit(policy);
>> -}
>> -
>>   static int amd_pstate_epp_cpu_online(struct cpufreq_policy *policy)
>>   {
>>   	struct amd_cpudata *cpudata = policy->driver_data;
>>   	int ret;
>>   
>> -	pr_debug("AMD CPU Core %d going online\n", cpudata->cpu);
>> +	pr_debug("AMD CPU Core %d going online\n", policy->cpu);
>>   
>> -	ret = amd_pstate_epp_reenable(policy);
>> +	ret = amd_pstate_cppc_enable(policy);
>>   	if (ret)
>>   		return ret;
>> +
>>   	cpudata->suspended = false;
> 
> Do we need this here?, shouldn't only resume() have this statement?

The reason I had in mind for it was this sequence:
* Suspend
* CPU goes offline
* CPU goes online
* Resume

But I don't think that's realistic even with parallel boot.  I will drop 
this.

> 
>>   
>>   	return 0;
>> +
>>   }
>>   
>>   static int amd_pstate_epp_cpu_offline(struct cpufreq_policy *policy)
>> @@ -1691,11 +1623,6 @@ static int amd_pstate_epp_cpu_offline(struct cpufreq_policy *policy)
>>   static int amd_pstate_epp_suspend(struct cpufreq_policy *policy)
>>   {
>>   	struct amd_cpudata *cpudata = policy->driver_data;
>> -	int ret;
>> -
>> -	/* avoid suspending when EPP is not enabled */
>> -	if (cppc_state != AMD_PSTATE_ACTIVE)
>> -		return 0;
>>   
>>   	/* invalidate to ensure it's rewritten during resume */
>>   	cpudata->cppc_req_cached = 0;
>> @@ -1703,11 +1630,6 @@ static int amd_pstate_epp_suspend(struct cpufreq_policy *policy)
>>   	/* set this flag to avoid setting core offline*/
>>   	cpudata->suspended = true;
>>   
>> -	/* disable CPPC in lowlevel firmware */
>> -	ret = amd_pstate_cppc_enable(false);
>> -	if (ret)
>> -		pr_err("failed to suspend, return %d\n", ret);
>> -
>>   	return 0;
>>   }
>>   
>> @@ -1716,8 +1638,12 @@ static int amd_pstate_epp_resume(struct cpufreq_policy *policy)
>>   	struct amd_cpudata *cpudata = policy->driver_data;
>>   
>>   	if (cpudata->suspended) {
>> +		int ret;
>> +
>>   		/* enable amd pstate from suspend state*/
>> -		amd_pstate_epp_reenable(policy);
>> +		ret = amd_pstate_epp_update_limit(policy);
>> +		if (ret)
>> +			return ret;
>>   
>>   		cpudata->suspended = false;
>>   	}
>> @@ -1732,8 +1658,6 @@ static struct cpufreq_driver amd_pstate_driver = {
>>   	.fast_switch    = amd_pstate_fast_switch,
>>   	.init		= amd_pstate_cpu_init,
>>   	.exit		= amd_pstate_cpu_exit,
>> -	.suspend	= amd_pstate_cpu_suspend,
>> -	.resume		= amd_pstate_cpu_resume,
>>   	.set_boost	= amd_pstate_set_boost,
>>   	.update_limits	= amd_pstate_update_limits,
>>   	.name		= "amd-pstate",
>> @@ -1748,8 +1672,8 @@ static struct cpufreq_driver amd_pstate_epp_driver = {
>>   	.exit		= amd_pstate_epp_cpu_exit,
>>   	.offline	= amd_pstate_epp_cpu_offline,
>>   	.online		= amd_pstate_epp_cpu_online,
>> -	.suspend	= amd_pstate_epp_suspend,
>> -	.resume		= amd_pstate_epp_resume,
>> +	.suspend        = amd_pstate_epp_suspend,
>> +	.resume         = amd_pstate_epp_resume,
> 
> Spurious whitespace change?
> 
>>   	.update_limits	= amd_pstate_update_limits,
>>   	.set_boost	= amd_pstate_set_boost,
>>   	.name		= "amd-pstate-epp",
>> @@ -1900,7 +1824,6 @@ static int __init amd_pstate_init(void)
>>   
>>   global_attr_free:
>>   	cpufreq_unregister_driver(current_pstate_driver);
>> -	amd_pstate_cppc_enable(false);
>>   	return ret;
>>   }
>>   device_initcall(amd_pstate_init);
>