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?
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)
> + 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?
Also, we need to set gt->defaults.min_freq here.
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
>
