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

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

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
>