Re: [Intel-gfx] [PATCH v3] drm/i915/guc/slpc: Provide sysfs for efficient freq

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

On Fri, 14 Apr 2023 15:34:15 -0700, Vinay Belgaumkar wrote:
>
> @@ -457,6 +458,34 @@ int intel_guc_slpc_get_max_freq(struct intel_guc_slpc *slpc, u32 *val)
>	return ret;
>  }
>
> +int intel_guc_slpc_set_ignore_eff_freq(struct intel_guc_slpc *slpc, bool val)
> +{
> +	struct drm_i915_private *i915 = slpc_to_i915(slpc);
> +	intel_wakeref_t wakeref;
> +	int ret = 0;
> +
> +	/* Need a lock now since waitboost can be modifying min as well */

Delete comment.

> +	mutex_lock(&slpc->lock);

Actually, don't need the lock itself now so delete the lock.

Or, maybe the lock prevents the race if userspace writes to the sysfs when
GuC reset is going on so let's retain the lock. But the comment is wrong.

> +	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
> +
> +	/* Ignore efficient freq if lower min freq is requested */

Delete comment, it's wrong.

> +	ret = slpc_set_param(slpc,
> +			     SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY,
> +			     val);
> +	if (ret) {
> +		guc_probe_error(slpc_to_guc(slpc), "Failed to set efficient freq(%d): %pe\n",
> +				val, ERR_PTR(ret));
> +		goto out;
> +	}
> +
> +	slpc->ignore_eff_freq = val;
> +

This extra line can also be deleted.

> +out:
> +	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
> +	mutex_unlock(&slpc->lock);
> +	return ret;
> +}
> +
>  /**
>   * intel_guc_slpc_set_min_freq() - Set min frequency limit for SLPC.
>   * @slpc: pointer to intel_guc_slpc.
> @@ -482,16 +511,6 @@ int intel_guc_slpc_set_min_freq(struct intel_guc_slpc *slpc, u32 val)
>	mutex_lock(&slpc->lock);
>	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
>
> -	/* Ignore efficient freq if lower min freq is requested */
> -	ret = slpc_set_param(slpc,
> -			     SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY,
> -			     val < slpc->rp1_freq);
> -	if (ret) {
> -		guc_probe_error(slpc_to_guc(slpc), "Failed to toggle efficient freq: %pe\n",
> -				ERR_PTR(ret));
> -		goto out;
> -	}
> -

Great, thanks!

After taking care of the above, and seems there are also a couple of
checkpatch errors, this is:

Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>