Re: [PATCH] drm/i915/guc/slpc: Provide sysfs for efficient freq

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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



[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux