[PATCH v2] drm/i915/slpc: Use platform limits for min/max frequency

[Belgaumkar, Vinay]

On 10/13/2022 3:28 PM, Dixit, Ashutosh wrote:
> On Thu, 13 Oct 2022 08:55:24 -0700, Vinay Belgaumkar wrote:
> Hi Vinay,
>
>> GuC will set the min/max frequencies to theoretical max on
>> ATS-M. This will break kernel ABI, so limit min/max frequency
>> to RP0(platform max) instead.
> Isn't what we are calling "theoretical max" or "RPmax" really just -1U
> (0xFFFFFFFF)? Though I have heard this is not a max value but -1U indicates
> FW default values unmodified by host SW, which would mean frequencies are
> fully controlled by FW (min == max == -1U). But if this were the case I
> don't know why this would be the case only for server, why doesn't FW set
> these for clients too to indicate it is fully in control?
FW sets max to -1U for client products(we already pull it down to RP0). 
It additionally makes min=max for server parts.
>
> So the question what does -1U actually represent? Is it the RPmax value or
> does -1U represent "FW defaults"?
>
> Also this concept of using -1U as "FW defaults" is present in Level0/OneAPI
> (and likely in firmware) but we seem to have blocked in the i915 ABI.
>
> I understand we may not be able to make such changes at present but this
> provides some context for the review comments below.
>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
>> index fdd895f73f9f..11613d373a49 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
>> @@ -263,6 +263,7 @@ int intel_guc_slpc_init(struct intel_guc_slpc *slpc)
>>
>> 	slpc->max_freq_softlimit = 0;
>> 	slpc->min_freq_softlimit = 0;
>> +	slpc->min_is_rpmax = false;
>>
>> 	slpc->boost_freq = 0;
>> 	atomic_set(&slpc->num_waiters, 0);
>> @@ -588,6 +589,31 @@ static int slpc_set_softlimits(struct intel_guc_slpc *slpc)
>> 	return 0;
>>   }
>>
>> +static bool is_slpc_min_freq_rpmax(struct intel_guc_slpc *slpc)
>> +{
>> +	int slpc_min_freq;
>> +
>> +	if (intel_guc_slpc_get_min_freq(slpc, &slpc_min_freq))
>> +		return false;
>> +
>> +	if (slpc_min_freq > slpc->rp0_freq)
>> or >=.
> If what we are calling "rpmax" really -1U then why don't we just check for
> -1U here?
>
> 	u32 slpc_min_freq;
>
> 	if (slpc_min_freq == -1U)
That'll work similarly too. Only time slpc_min_freq is greater than rp0 
is for a server part.
>
>> +		return true;
>> +	else
>> +		return false;
>> +}
>> +
>> +static void update_server_min_softlimit(struct intel_guc_slpc *slpc)
>> +{
>> +	/* For server parts, SLPC min will be at RPMax.
>> +	 * Use min softlimit to clamp it to RP0 instead.
>> +	 */
>> +	if (is_slpc_min_freq_rpmax(slpc) &&
>> +	    !slpc->min_freq_softlimit) {
>> +		slpc->min_is_rpmax = true;
>> +		slpc->min_freq_softlimit = slpc->rp0_freq;
> Isn't it safer to use a platform check such as IS_ATSM or IS_XEHPSDV (or
> even #define IS_SERVER()) to set min freq to RP0 rather than this -1U value
> from FW? What if -1U means "FW defaults" and FW starts setting this on
> client products tomorrow?

We are not checking for -1 specifically, but only if FW has set min > 
RP0 as an indicator. Also, might be worth having IS_SERVER at some point 
if there are other places we need this info as well.

>
> Also, we need to set gt->defaults.min_freq here.

yes, need to add that.

Thanks,

Vinay.

>
> Thanks.
> --
> Ashutosh
>
>
>> +	}
>> +}
>> +
>>   static int slpc_use_fused_rp0(struct intel_guc_slpc *slpc)
>>   {
>> 	/* Force SLPC to used platform rp0 */
>> @@ -647,6 +673,9 @@ int intel_guc_slpc_enable(struct intel_guc_slpc *slpc)
>>
>> 	slpc_get_rp_values(slpc);
>>
>> +	/* Handle the case where min=max=RPmax */
>> +	update_server_min_softlimit(slpc);
>> +
>> 	/* Set SLPC max limit to RP0 */
>> 	ret = slpc_use_fused_rp0(slpc);
>> 	if (unlikely(ret)) {
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc_types.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc_types.h
>> index 73d208123528..a6ef53b04e04 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc_types.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc_types.h
>> @@ -19,6 +19,9 @@ struct intel_guc_slpc {
>> 	bool supported;
>> 	bool selected;
>>
>> +	/* Indicates this is a server part */
>> +	bool min_is_rpmax;
>> +
>> 	/* platform frequency limits */
>> 	u32 min_freq;
>> 	u32 rp0_freq;
>> --
>> 2.35.1
>>