Hi Vinay,
Looks good, just few minor comments below,
[...]
> @@ -267,13 +267,11 @@ static int run_test(struct intel_gt *gt, int test_type)
> }
>
> /*
> - * Set min frequency to RPn so that we can test the whole
> - * range of RPn-RP0. This also turns off efficient freq
> - * usage and makes results more predictable.
> + * Turn off efficient freq so RPn/RP0 ranges are obeyed
> */
> - err = slpc_set_min_freq(slpc, slpc->min_freq);
> + err = intel_guc_slpc_set_ignore_eff_freq(slpc, true);
> if (err) {
> - pr_err("Unable to update min freq!");
> + pr_err("Unable to turn off efficient freq!");
drm_err()? or gt_err()? As we are here we can use a proper
printing.
How is this change related to the scope of this patch?
> return err;
> }
>
> @@ -358,9 +356,10 @@ static int run_test(struct intel_gt *gt, int test_type)
> break;
> }
>
> - /* Restore min/max frequencies */
> - slpc_set_max_freq(slpc, slpc_max_freq);
> + /* Restore min/max frequencies and efficient flag */
> slpc_set_min_freq(slpc, slpc_min_freq);
> + slpc_set_max_freq(slpc, slpc_max_freq);
> + intel_guc_slpc_set_ignore_eff_freq(slpc, false);
mmhhh... do we care here about the return value?
>
> if (igt_flush_test(gt->i915))
> err = -EIO;
> 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 026d73855f36..b1b70ee3001b 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
> @@ -277,6 +277,7 @@ int intel_guc_slpc_init(struct intel_guc_slpc *slpc)
>
> slpc->max_freq_softlimit = 0;
> slpc->min_freq_softlimit = 0;
> + slpc->ignore_eff_freq = false;
> slpc->min_is_rpmax = false;
>
> slpc->boost_freq = 0;
> @@ -457,6 +458,31 @@ 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;
no need to initialize ret here.
> +
> + mutex_lock(&slpc->lock);
> + wakeref = intel_runtime_pm_get(&i915->runtime_pm);
> +
> + 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;
nit that you can ignore: if you put this under else and save
brackets and a goto.
Andi
