Re: [Intel-gfx] [PATCH v2] drm/i915/guc/slpc: Restore efficient freq earlier

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Jul 25, 2023 at 06:00:44PM -0700, Vinay Belgaumkar wrote:
> This should be done before the soft min/max frequencies are restored.
> When we disable the "Ignore efficient frequency" flag, GuC does not
> actually bring the requested freq down to RPn.
> 
> Specifically, this scenario-
> 
> - ignore efficient freq set to true
> - reduce min to RPn (from efficient)
> - suspend
> - resume (includes GuC load, restore soft min/max, restore efficient freq)
> - validate min freq has been resored to RPn
> 
> This will fail if we didn't first restore(disable, in this case) efficient
> freq flag before setting the soft min frequency.
> 
> v2: Bring the min freq down to RPn when we disable efficient freq (Rodrigo)
> Also made the change to set the min softlimit to RPn at init. Otherwise, we
> were storing RPe there.

Thanks
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> 
> Link: https://gitlab.freedesktop.org/drm/intel/-/issues/8736
> Fixes: 55f9720dbf23 ("drm/i915/guc/slpc: Provide sysfs for efficient freq")
> Fixes: 95ccf312a1e4 ("drm/i915/guc/slpc: Allow SLPC to use efficient frequency")
> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 22 +++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> 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 ee9f83af7cf6..477df260ae3a 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
> @@ -470,12 +470,19 @@ int intel_guc_slpc_set_ignore_eff_freq(struct intel_guc_slpc *slpc, bool val)
>  	ret = slpc_set_param(slpc,
>  			     SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY,
>  			     val);
> -	if (ret)
> +	if (ret) {
>  		guc_probe_error(slpc_to_guc(slpc), "Failed to set efficient freq(%d): %pe\n",
>  				val, ERR_PTR(ret));
> -	else
> +	} else {
>  		slpc->ignore_eff_freq = val;
>  
> +		/* Set min to RPn when we disable efficient freq */
> +		if (val)
> +			ret = slpc_set_param(slpc,
> +					     SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ,
> +					     slpc->min_freq);
> +	}
> +
>  	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
>  	mutex_unlock(&slpc->lock);
>  	return ret;
> @@ -602,9 +609,8 @@ static int slpc_set_softlimits(struct intel_guc_slpc *slpc)
>  		return ret;
>  
>  	if (!slpc->min_freq_softlimit) {
> -		ret = intel_guc_slpc_get_min_freq(slpc, &slpc->min_freq_softlimit);
> -		if (unlikely(ret))
> -			return ret;
> +		/* Min softlimit is initialized to RPn */
> +		slpc->min_freq_softlimit = slpc->min_freq;
>  		slpc_to_gt(slpc)->defaults.min_freq = slpc->min_freq_softlimit;
>  	} else {
>  		return intel_guc_slpc_set_min_freq(slpc,
> @@ -755,6 +761,9 @@ int intel_guc_slpc_enable(struct intel_guc_slpc *slpc)
>  		return ret;
>  	}
>  
> +	/* Set cached value of ignore efficient freq */
> +	intel_guc_slpc_set_ignore_eff_freq(slpc, slpc->ignore_eff_freq);
> +
>  	/* Revert SLPC min/max to softlimits if necessary */
>  	ret = slpc_set_softlimits(slpc);
>  	if (unlikely(ret)) {
> @@ -765,9 +774,6 @@ int intel_guc_slpc_enable(struct intel_guc_slpc *slpc)
>  	/* Set cached media freq ratio mode */
>  	intel_guc_slpc_set_media_ratio_mode(slpc, slpc->media_ratio_mode);
>  
> -	/* Set cached value of ignore efficient freq */
> -	intel_guc_slpc_set_ignore_eff_freq(slpc, slpc->ignore_eff_freq);
> -
>  	return 0;
>  }
>  
> -- 
> 2.38.1
>