On 4/17/2023 6:39 PM, Andi Shyti wrote:
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?
The selftest was relying on setting min freq < RP1 to disable efficient
freq, now that we have an interface, the test should use that (former
method will not work). Should this be a separate 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?
I guess we should, will add.
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.
ok.
+
+ 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.
ok.
Thanks,
Vinay.
Andi
