[Intel-gfx] [PATCH v2] drm/i915/slpc: Use platform limits for min/max frequency
Belgaumkar, Vinay
vinay.belgaumkar at intel.com
Fri Oct 14 00:24:35 UTC 2022
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
>>
More information about the Intel-gfx
mailing list