diff mbox series

[v4] drm/i915/slpc: Use platform limits for min/max frequency

Message ID 20221024225453.4856-1-vinay.belgaumkar@intel.com (mailing list archive)
State New, archived
Headers show
Series [v4] drm/i915/slpc: Use platform limits for min/max frequency | expand

Commit Message

Vinay Belgaumkar Oct. 24, 2022, 10:54 p.m. UTC 
GuC will set the min/max frequencies to theoretical max on
ATS-M. This will break kernel ABI, so limit min/max frequency
to RP0(platform max) instead.

Also modify the SLPC selftest to update the min frequency
when we have a server part so that we can iterate between
platform min and max.

v2: Check softlimits instead of platform limits (Riana)
v3: More review comments (Ashutosh)
v4: No need to use saved_min_freq and other comments (Ashutosh)

Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/7030

Acked-by: Nirmoy Das <nirmoy.das@intel.com>
Reviewed-by: Riana Tauro <riana.tauro@intel.com>
Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
---
 drivers/gpu/drm/i915/gt/selftest_slpc.c       | 20 +++++-----
 drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c   | 37 +++++++++++++++++++
 drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h   |  2 +
 .../gpu/drm/i915/gt/uc/intel_guc_slpc_types.h |  3 ++
 4 files changed, 53 insertions(+), 9 deletions(-)

Comments

Dixit, Ashutosh Oct. 25, 2022, 3:22 p.m. UTC | #1 
On Mon, 24 Oct 2022 15:54:53 -0700, Vinay Belgaumkar wrote:
>
> GuC will set the min/max frequencies to theoretical max on
> ATS-M. This will break kernel ABI, so limit min/max frequency
> to RP0(platform max) instead.
>
> Also modify the SLPC selftest to update the min frequency
> when we have a server part so that we can iterate between
> platform min and max.
>
> v2: Check softlimits instead of platform limits (Riana)
> v3: More review comments (Ashutosh)
> v4: No need to use saved_min_freq and other comments (Ashutosh)

OK, much better now overall.

> Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/7030
>
> +static void update_server_min_softlimit(struct intel_guc_slpc *slpc)
> +{
> +	/* For server parts, SLPC min will be at RPMax.
> +	 * Use min softlimit to clamp it to RP0 instead.
> +	 */
> +	if (!slpc->min_freq_softlimit &&
> +	    is_slpc_min_freq_rpmax(slpc)) {
> +		slpc->min_is_rpmax = true;

The only remaining issue is slpc->min_is_rpmax is now set but never used so
it can possibly be removed, or retained for debuggability (I think it's a
fair reason to retain it). Though I am not sure if we will hit a "variable
set but never used" error from these clever compilers.

> +		slpc->min_freq_softlimit = slpc->rp0_freq;
> +		(slpc_to_gt(slpc))->defaults.min_freq = slpc->min_freq_softlimit;
> +	}
> +}

In any case, this is now:

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

Patch

diff --git a/drivers/gpu/drm/i915/gt/selftest_slpc.c b/drivers/gpu/drm/i915/gt/selftest_slpc.c
index 4c6e9257e593..82ec95a299f6 100644
--- a/drivers/gpu/drm/i915/gt/selftest_slpc.c
+++ b/drivers/gpu/drm/i915/gt/selftest_slpc.c
@@ -239,6 +239,11 @@  static int run_test(struct intel_gt *gt, int test_type)
 	if (!intel_uc_uses_guc_slpc(&gt->uc))
 		return 0;
 
+	if (slpc->min_freq == slpc->rp0_freq) {
+		pr_err("Min/Max are fused to the same value\n");
+		return -EINVAL;
+	}
+
 	if (igt_spinner_init(&spin, gt))
 		return -ENOMEM;
 
@@ -253,17 +258,14 @@  static int run_test(struct intel_gt *gt, int test_type)
 	}
 
 	/*
-	 * FIXME: With efficient frequency enabled, GuC can request
-	 * frequencies higher than the SLPC max. While this is fixed
-	 * in GuC, we level set these tests with RPn as min.
+	 * 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.
 	 */
 	err = slpc_set_min_freq(slpc, slpc->min_freq);
-	if (err)
+	if (err) {
+		pr_err("Unable to update min freq!");
 		return err;
-
-	if (slpc->min_freq == slpc->rp0_freq) {
-		pr_err("Min/Max are fused to the same value\n");
-		return -EINVAL;
 	}
 
 	intel_gt_pm_wait_for_idle(gt);
@@ -330,7 +332,7 @@  static int run_test(struct intel_gt *gt, int test_type)
 				engine->name, max_act_freq);
 
 			/* Actual frequency should rise above min */
-			if (max_act_freq <= slpc_min_freq) {
+			if (max_act_freq <= slpc->min_freq) {
 				pr_err("Actual freq did not rise above min\n");
 				pr_err("Perf Limit Reasons: 0x%x\n",
 				       intel_uncore_read(gt->uncore, GT0_PERF_LIMIT_REASONS));
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 fdd895f73f9f..c7e568af1e9c 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
@@ -263,6 +263,7 @@  int intel_guc_slpc_init(struct intel_guc_slpc *slpc)
 
 	slpc->max_freq_softlimit = 0;
 	slpc->min_freq_softlimit = 0;
+	slpc->min_is_rpmax = false;
 
 	slpc->boost_freq = 0;
 	atomic_set(&slpc->num_waiters, 0);
@@ -588,6 +589,39 @@  static int slpc_set_softlimits(struct intel_guc_slpc *slpc)
 	return 0;
 }
 
+static bool is_slpc_min_freq_rpmax(struct intel_guc_slpc *slpc)
+{
+	struct drm_i915_private *i915 = slpc_to_i915(slpc);
+	int slpc_min_freq;
+	int ret;
+
+	ret = intel_guc_slpc_get_min_freq(slpc, &slpc_min_freq);
+	if (ret) {
+		drm_err(&i915->drm,
+			"Failed to get min freq: (%d)\n",
+			ret);
+		return false;
+	}
+
+	if (slpc_min_freq == SLPC_MAX_FREQ_MHZ)
+		return true;
+	else
+		return false;
+}
+
+static void update_server_min_softlimit(struct intel_guc_slpc *slpc)
+{
+	/* For server parts, SLPC min will be at RPMax.
+	 * Use min softlimit to clamp it to RP0 instead.
+	 */
+	if (!slpc->min_freq_softlimit &&
+	    is_slpc_min_freq_rpmax(slpc)) {
+		slpc->min_is_rpmax = true;
+		slpc->min_freq_softlimit = slpc->rp0_freq;
+		(slpc_to_gt(slpc))->defaults.min_freq = slpc->min_freq_softlimit;
+	}
+}
+
 static int slpc_use_fused_rp0(struct intel_guc_slpc *slpc)
 {
 	/* Force SLPC to used platform rp0 */
@@ -647,6 +681,9 @@  int intel_guc_slpc_enable(struct intel_guc_slpc *slpc)
 
 	slpc_get_rp_values(slpc);
 
+	/* Handle the case where min=max=RPmax */
+	update_server_min_softlimit(slpc);
+
 	/* Set SLPC max limit to RP0 */
 	ret = slpc_use_fused_rp0(slpc);
 	if (unlikely(ret)) {
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h
index 82a98f78f96c..11975a31c9d0 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h
@@ -9,6 +9,8 @@ 
 #include "intel_guc_submission.h"
 #include "intel_guc_slpc_types.h"
 
+#define SLPC_MAX_FREQ_MHZ 4250
+
 struct intel_gt;
 struct drm_printer;
 
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc_types.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc_types.h
index 73d208123528..a6ef53b04e04 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc_types.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc_types.h
@@ -19,6 +19,9 @@  struct intel_guc_slpc {
 	bool supported;
 	bool selected;
 
+	/* Indicates this is a server part */
+	bool min_is_rpmax;
+
 	/* platform frequency limits */
 	u32 min_freq;
 	u32 rp0_freq;