[RFC v2 0/4] Expose RPS thresholds in sysfs

[RFC v2 0/4] Expose RPS thresholds in sysfs

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

From patch 4:

    User feedback indicates significant performance gains are possible in
    specific games with non default RPS up/down thresholds.

    Expose these tunables via sysfs which will allow users to achieve best
    performance when running games and best power efficiency elsewhere.

    Note this patch supports non GuC based platforms only.

    References: https://gitlab.freedesktop.org/drm/intel/-/issues/8389

Issue 8389 suggests 10-15% performance gains are possible with tweaked
thresholds.

One question is are we able to find a "one size fits all" values.

However regardless of that, given we already expose frequency controls in sysfs
with the same reasoning of allowing system owners explicit control if so wanted,
I think exposing the thresholds can be equally justified.

v2:
 * Hopefully fixed the debug build issue.
 * Re-program the hw registers on change too!

Tvrtko Ursulin (4):
  drm/i915: Move setting of rps thresholds to init
  drm/i915: Record default rps threshold values
  drm/i915: Add helpers for managing rps thresholds
  drm/i915: Expose RPS thresholds in sysfs

 drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 104 ++++++++++++++++++++
 drivers/gpu/drm/i915/gt/intel_gt_types.h    |   3 +
 drivers/gpu/drm/i915/gt/intel_rps.c         |  70 ++++++++++---
 drivers/gpu/drm/i915/gt/intel_rps.h         |   4 +
 4 files changed, 170 insertions(+), 11 deletions(-)

-- 
2.37.2

[Intel-gfx] [RFC v2 0/4] Expose RPS thresholds in sysfs

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

From patch 4:

    User feedback indicates significant performance gains are possible in
    specific games with non default RPS up/down thresholds.

    Expose these tunables via sysfs which will allow users to achieve best
    performance when running games and best power efficiency elsewhere.

    Note this patch supports non GuC based platforms only.

    References: https://gitlab.freedesktop.org/drm/intel/-/issues/8389

Issue 8389 suggests 10-15% performance gains are possible with tweaked
thresholds.

One question is are we able to find a "one size fits all" values.

However regardless of that, given we already expose frequency controls in sysfs
with the same reasoning of allowing system owners explicit control if so wanted,
I think exposing the thresholds can be equally justified.

v2:
 * Hopefully fixed the debug build issue.
 * Re-program the hw registers on change too!

Tvrtko Ursulin (4):
  drm/i915: Move setting of rps thresholds to init
  drm/i915: Record default rps threshold values
  drm/i915: Add helpers for managing rps thresholds
  drm/i915: Expose RPS thresholds in sysfs

 drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 104 ++++++++++++++++++++
 drivers/gpu/drm/i915/gt/intel_gt_types.h    |   3 +
 drivers/gpu/drm/i915/gt/intel_rps.c         |  70 ++++++++++---
 drivers/gpu/drm/i915/gt/intel_rps.h         |   4 +
 4 files changed, 170 insertions(+), 11 deletions(-)

-- 
2.37.2

[RFC 1/4] drm/i915: Move setting of rps thresholds to init

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Since 36d516be867c ("drm/i915/gt: Switch to manual evaluation of RPS")
thresholds are invariant so lets move their setting to init time.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_rps.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c
index 80968e49e2c3..d78699e2b13b 100644
--- a/drivers/gpu/drm/i915/gt/intel_rps.c
+++ b/drivers/gpu/drm/i915/gt/intel_rps.c
@@ -670,7 +670,6 @@ static void rps_set_power(struct intel_rps *rps, int new_power)
 {
 	struct intel_gt *gt = rps_to_gt(rps);
 	struct intel_uncore *uncore = gt->uncore;
-	u32 threshold_up = 0, threshold_down = 0; /* in % */
 	u32 ei_up = 0, ei_down = 0;
 
 	lockdep_assert_held(&rps->power.mutex);
@@ -678,9 +677,6 @@ static void rps_set_power(struct intel_rps *rps, int new_power)
 	if (new_power == rps->power.mode)
 		return;
 
-	threshold_up = 95;
-	threshold_down = 85;
-
 	/* Note the units here are not exactly 1us, but 1280ns. */
 	switch (new_power) {
 	case LOW_POWER:
@@ -707,17 +703,22 @@ static void rps_set_power(struct intel_rps *rps, int new_power)
 
 	GT_TRACE(gt,
 		 "changing power mode [%d], up %d%% @ %dus, down %d%% @ %dus\n",
-		 new_power, threshold_up, ei_up, threshold_down, ei_down);
+		 new_power,
+		 rps->power.up_threshold, ei_up,
+		 rps->power.down_threshold, ei_down);
 
 	set(uncore, GEN6_RP_UP_EI,
 	    intel_gt_ns_to_pm_interval(gt, ei_up * 1000));
 	set(uncore, GEN6_RP_UP_THRESHOLD,
-	    intel_gt_ns_to_pm_interval(gt, ei_up * threshold_up * 10));
+	    intel_gt_ns_to_pm_interval(gt,
+				       ei_up * rps->power.up_threshold * 10));
 
 	set(uncore, GEN6_RP_DOWN_EI,
 	    intel_gt_ns_to_pm_interval(gt, ei_down * 1000));
 	set(uncore, GEN6_RP_DOWN_THRESHOLD,
-	    intel_gt_ns_to_pm_interval(gt, ei_down * threshold_down * 10));
+	    intel_gt_ns_to_pm_interval(gt,
+				       ei_down *
+				       rps->power.down_threshold * 10));
 
 	set(uncore, GEN6_RP_CONTROL,
 	    (GRAPHICS_VER(gt->i915) > 9 ? 0 : GEN6_RP_MEDIA_TURBO) |
@@ -729,8 +730,6 @@ static void rps_set_power(struct intel_rps *rps, int new_power)
 
 skip_hw_write:
 	rps->power.mode = new_power;
-	rps->power.up_threshold = threshold_up;
-	rps->power.down_threshold = threshold_down;
 }
 
 static void gen6_rps_set_thresholds(struct intel_rps *rps, u8 val)
@@ -1556,10 +1555,12 @@ void intel_rps_enable(struct intel_rps *rps)
 		return;
 
 	GT_TRACE(rps_to_gt(rps),
-		 "min:%x, max:%x, freq:[%d, %d]\n",
+		 "min:%x, max:%x, freq:[%d, %d], thresholds:[%u, %u]\n",
 		 rps->min_freq, rps->max_freq,
 		 intel_gpu_freq(rps, rps->min_freq),
-		 intel_gpu_freq(rps, rps->max_freq));
+		 intel_gpu_freq(rps, rps->max_freq),
+		 rps->power.up_threshold,
+		 rps->power.down_threshold);
 
 	GEM_BUG_ON(rps->max_freq < rps->min_freq);
 	GEM_BUG_ON(rps->idle_freq > rps->max_freq);
@@ -2012,6 +2013,10 @@ void intel_rps_init(struct intel_rps *rps)
 		}
 	}
 
+	/* Set default thresholds in % */
+	rps->power.up_threshold = 95;
+	rps->power.down_threshold = 85;
+
 	/* Finally allow us to boost to max by default */
 	rps->boost_freq = rps->max_freq;
 	rps->idle_freq = rps->min_freq;
-- 
2.37.2

[Intel-gfx] [RFC 1/4] drm/i915: Move setting of rps thresholds to init

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Since 36d516be867c ("drm/i915/gt: Switch to manual evaluation of RPS")
thresholds are invariant so lets move their setting to init time.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_rps.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c
index 80968e49e2c3..d78699e2b13b 100644
--- a/drivers/gpu/drm/i915/gt/intel_rps.c
+++ b/drivers/gpu/drm/i915/gt/intel_rps.c
@@ -670,7 +670,6 @@ static void rps_set_power(struct intel_rps *rps, int new_power)
 {
 	struct intel_gt *gt = rps_to_gt(rps);
 	struct intel_uncore *uncore = gt->uncore;
-	u32 threshold_up = 0, threshold_down = 0; /* in % */
 	u32 ei_up = 0, ei_down = 0;
 
 	lockdep_assert_held(&rps->power.mutex);
@@ -678,9 +677,6 @@ static void rps_set_power(struct intel_rps *rps, int new_power)
 	if (new_power == rps->power.mode)
 		return;
 
-	threshold_up = 95;
-	threshold_down = 85;
-
 	/* Note the units here are not exactly 1us, but 1280ns. */
 	switch (new_power) {
 	case LOW_POWER:
@@ -707,17 +703,22 @@ static void rps_set_power(struct intel_rps *rps, int new_power)
 
 	GT_TRACE(gt,
 		 "changing power mode [%d], up %d%% @ %dus, down %d%% @ %dus\n",
-		 new_power, threshold_up, ei_up, threshold_down, ei_down);
+		 new_power,
+		 rps->power.up_threshold, ei_up,
+		 rps->power.down_threshold, ei_down);
 
 	set(uncore, GEN6_RP_UP_EI,
 	    intel_gt_ns_to_pm_interval(gt, ei_up * 1000));
 	set(uncore, GEN6_RP_UP_THRESHOLD,
-	    intel_gt_ns_to_pm_interval(gt, ei_up * threshold_up * 10));
+	    intel_gt_ns_to_pm_interval(gt,
+				       ei_up * rps->power.up_threshold * 10));
 
 	set(uncore, GEN6_RP_DOWN_EI,
 	    intel_gt_ns_to_pm_interval(gt, ei_down * 1000));
 	set(uncore, GEN6_RP_DOWN_THRESHOLD,
-	    intel_gt_ns_to_pm_interval(gt, ei_down * threshold_down * 10));
+	    intel_gt_ns_to_pm_interval(gt,
+				       ei_down *
+				       rps->power.down_threshold * 10));
 
 	set(uncore, GEN6_RP_CONTROL,
 	    (GRAPHICS_VER(gt->i915) > 9 ? 0 : GEN6_RP_MEDIA_TURBO) |
@@ -729,8 +730,6 @@ static void rps_set_power(struct intel_rps *rps, int new_power)
 
 skip_hw_write:
 	rps->power.mode = new_power;
-	rps->power.up_threshold = threshold_up;
-	rps->power.down_threshold = threshold_down;
 }
 
 static void gen6_rps_set_thresholds(struct intel_rps *rps, u8 val)
@@ -1556,10 +1555,12 @@ void intel_rps_enable(struct intel_rps *rps)
 		return;
 
 	GT_TRACE(rps_to_gt(rps),
-		 "min:%x, max:%x, freq:[%d, %d]\n",
+		 "min:%x, max:%x, freq:[%d, %d], thresholds:[%u, %u]\n",
 		 rps->min_freq, rps->max_freq,
 		 intel_gpu_freq(rps, rps->min_freq),
-		 intel_gpu_freq(rps, rps->max_freq));
+		 intel_gpu_freq(rps, rps->max_freq),
+		 rps->power.up_threshold,
+		 rps->power.down_threshold);
 
 	GEM_BUG_ON(rps->max_freq < rps->min_freq);
 	GEM_BUG_ON(rps->idle_freq > rps->max_freq);
@@ -2012,6 +2013,10 @@ void intel_rps_init(struct intel_rps *rps)
 		}
 	}
 
+	/* Set default thresholds in % */
+	rps->power.up_threshold = 95;
+	rps->power.down_threshold = 85;
+
 	/* Finally allow us to boost to max by default */
 	rps->boost_freq = rps->max_freq;
 	rps->idle_freq = rps->min_freq;
-- 
2.37.2

[RFC 2/4] drm/i915: Record default rps threshold values

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Record the default values as preparation for exposing the sysfs controls.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_gt_types.h | 3 +++
 drivers/gpu/drm/i915/gt/intel_rps.c      | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h
index f08c2556aa25..1b22d7a50665 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
@@ -83,6 +83,9 @@ enum intel_submission_method {
 struct gt_defaults {
 	u32 min_freq;
 	u32 max_freq;
+
+	u8 rps_up_threshold;
+	u8 rps_down_threshold;
 };
 
 enum intel_gt_type {
diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c
index d78699e2b13b..a39eee444849 100644
--- a/drivers/gpu/drm/i915/gt/intel_rps.c
+++ b/drivers/gpu/drm/i915/gt/intel_rps.c
@@ -2015,7 +2015,9 @@ void intel_rps_init(struct intel_rps *rps)
 
 	/* Set default thresholds in % */
 	rps->power.up_threshold = 95;
+	rps_to_gt(rps)->defaults.rps_up_threshold = rps->power.up_threshold;
 	rps->power.down_threshold = 85;
+	rps_to_gt(rps)->defaults.rps_down_threshold = rps->power.down_threshold;
 
 	/* Finally allow us to boost to max by default */
 	rps->boost_freq = rps->max_freq;
-- 
2.37.2

[Intel-gfx] [RFC 2/4] drm/i915: Record default rps threshold values

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Record the default values as preparation for exposing the sysfs controls.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_gt_types.h | 3 +++
 drivers/gpu/drm/i915/gt/intel_rps.c      | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h
index f08c2556aa25..1b22d7a50665 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
@@ -83,6 +83,9 @@ enum intel_submission_method {
 struct gt_defaults {
 	u32 min_freq;
 	u32 max_freq;
+
+	u8 rps_up_threshold;
+	u8 rps_down_threshold;
 };
 
 enum intel_gt_type {
diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c
index d78699e2b13b..a39eee444849 100644
--- a/drivers/gpu/drm/i915/gt/intel_rps.c
+++ b/drivers/gpu/drm/i915/gt/intel_rps.c
@@ -2015,7 +2015,9 @@ void intel_rps_init(struct intel_rps *rps)
 
 	/* Set default thresholds in % */
 	rps->power.up_threshold = 95;
+	rps_to_gt(rps)->defaults.rps_up_threshold = rps->power.up_threshold;
 	rps->power.down_threshold = 85;
+	rps_to_gt(rps)->defaults.rps_down_threshold = rps->power.down_threshold;
 
 	/* Finally allow us to boost to max by default */
 	rps->boost_freq = rps->max_freq;
-- 
2.37.2

[RFC 3/4] drm/i915: Add helpers for managing rps thresholds

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

In preparation for exposing via sysfs add helpers for managing rps
thresholds.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_rps.c | 36 +++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/gt/intel_rps.h |  4 ++++
 2 files changed, 40 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c
index a39eee444849..a5a7315f5ace 100644
--- a/drivers/gpu/drm/i915/gt/intel_rps.c
+++ b/drivers/gpu/drm/i915/gt/intel_rps.c
@@ -2573,6 +2573,42 @@ int intel_rps_set_min_frequency(struct intel_rps *rps, u32 val)
 		return set_min_freq(rps, val);
 }
 
+u8 intel_rps_get_up_threshold(struct intel_rps *rps)
+{
+	return rps->power.up_threshold;
+}
+
+static int rps_set_threshold(struct intel_rps *rps, u8 *threshold, u8 val)
+{
+	int ret;
+
+	if (val > 100)
+		return -EINVAL;
+
+	ret = mutex_lock_interruptible(&rps->lock);
+	if (ret)
+		return ret;
+	*threshold = val;
+	mutex_unlock(&rps->lock);
+
+	return 0;
+}
+
+int intel_rps_set_up_threshold(struct intel_rps *rps, u8 threshold)
+{
+	return rps_set_threshold(rps, &rps->power.up_threshold, threshold);
+}
+
+u8 intel_rps_get_down_threshold(struct intel_rps *rps)
+{
+	return rps->power.down_threshold;
+}
+
+int intel_rps_set_down_threshold(struct intel_rps *rps, u8 threshold)
+{
+	return rps_set_threshold(rps, &rps->power.down_threshold, threshold);
+}
+
 static void intel_rps_set_manual(struct intel_rps *rps, bool enable)
 {
 	struct intel_uncore *uncore = rps_to_uncore(rps);
diff --git a/drivers/gpu/drm/i915/gt/intel_rps.h b/drivers/gpu/drm/i915/gt/intel_rps.h
index a3fa987aa91f..92fb01f5a452 100644
--- a/drivers/gpu/drm/i915/gt/intel_rps.h
+++ b/drivers/gpu/drm/i915/gt/intel_rps.h
@@ -37,6 +37,10 @@ void intel_rps_mark_interactive(struct intel_rps *rps, bool interactive);
 
 int intel_gpu_freq(struct intel_rps *rps, int val);
 int intel_freq_opcode(struct intel_rps *rps, int val);
+u8 intel_rps_get_up_threshold(struct intel_rps *rps);
+int intel_rps_set_up_threshold(struct intel_rps *rps, u8 threshold);
+u8 intel_rps_get_down_threshold(struct intel_rps *rps);
+int intel_rps_set_down_threshold(struct intel_rps *rps, u8 threshold);
 u32 intel_rps_read_actual_frequency(struct intel_rps *rps);
 u32 intel_rps_read_actual_frequency_fw(struct intel_rps *rps);
 u32 intel_rps_get_requested_frequency(struct intel_rps *rps);
-- 
2.37.2

[Intel-gfx] [RFC 3/4] drm/i915: Add helpers for managing rps thresholds

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

In preparation for exposing via sysfs add helpers for managing rps
thresholds.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_rps.c | 36 +++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/gt/intel_rps.h |  4 ++++
 2 files changed, 40 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c
index a39eee444849..a5a7315f5ace 100644
--- a/drivers/gpu/drm/i915/gt/intel_rps.c
+++ b/drivers/gpu/drm/i915/gt/intel_rps.c
@@ -2573,6 +2573,42 @@ int intel_rps_set_min_frequency(struct intel_rps *rps, u32 val)
 		return set_min_freq(rps, val);
 }
 
+u8 intel_rps_get_up_threshold(struct intel_rps *rps)
+{
+	return rps->power.up_threshold;
+}
+
+static int rps_set_threshold(struct intel_rps *rps, u8 *threshold, u8 val)
+{
+	int ret;
+
+	if (val > 100)
+		return -EINVAL;
+
+	ret = mutex_lock_interruptible(&rps->lock);
+	if (ret)
+		return ret;
+	*threshold = val;
+	mutex_unlock(&rps->lock);
+
+	return 0;
+}
+
+int intel_rps_set_up_threshold(struct intel_rps *rps, u8 threshold)
+{
+	return rps_set_threshold(rps, &rps->power.up_threshold, threshold);
+}
+
+u8 intel_rps_get_down_threshold(struct intel_rps *rps)
+{
+	return rps->power.down_threshold;
+}
+
+int intel_rps_set_down_threshold(struct intel_rps *rps, u8 threshold)
+{
+	return rps_set_threshold(rps, &rps->power.down_threshold, threshold);
+}
+
 static void intel_rps_set_manual(struct intel_rps *rps, bool enable)
 {
 	struct intel_uncore *uncore = rps_to_uncore(rps);
diff --git a/drivers/gpu/drm/i915/gt/intel_rps.h b/drivers/gpu/drm/i915/gt/intel_rps.h
index a3fa987aa91f..92fb01f5a452 100644
--- a/drivers/gpu/drm/i915/gt/intel_rps.h
+++ b/drivers/gpu/drm/i915/gt/intel_rps.h
@@ -37,6 +37,10 @@ void intel_rps_mark_interactive(struct intel_rps *rps, bool interactive);
 
 int intel_gpu_freq(struct intel_rps *rps, int val);
 int intel_freq_opcode(struct intel_rps *rps, int val);
+u8 intel_rps_get_up_threshold(struct intel_rps *rps);
+int intel_rps_set_up_threshold(struct intel_rps *rps, u8 threshold);
+u8 intel_rps_get_down_threshold(struct intel_rps *rps);
+int intel_rps_set_down_threshold(struct intel_rps *rps, u8 threshold);
 u32 intel_rps_read_actual_frequency(struct intel_rps *rps);
 u32 intel_rps_read_actual_frequency_fw(struct intel_rps *rps);
 u32 intel_rps_get_requested_frequency(struct intel_rps *rps);
-- 
2.37.2

[RFC 4/4] drm/i915: Expose RPS thresholds in sysfs

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

User feedback indicates significant performance gains are possible in
specific games with non default RPS up/down thresholds.

Expose these tunables via sysfs which will allow users to achieve best
performance when running games and best power efficiency elsewhere.

Note this patch supports non GuC based platforms only.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
References: https://gitlab.freedesktop.org/drm/intel/-/issues/8389
---
 drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 104 ++++++++++++++++++++
 drivers/gpu/drm/i915/gt/intel_rps.c         |   7 +-
 2 files changed, 110 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
index 28f27091cd3b..df1f9ef08475 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
@@ -671,6 +671,76 @@ static const struct attribute *media_perf_power_attrs[] = {
 	NULL
 };
 
+static ssize_t
+rps_up_threshold_pct_show(struct kobject *kobj, struct kobj_attribute *attr,
+			  char *buf)
+{
+	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(kobj, attr->attr.name);
+	struct intel_rps *rps = &gt->rps;
+
+	return sysfs_emit(buf, "%u\n", intel_rps_get_up_threshold(rps));
+}
+
+static ssize_t
+rps_up_threshold_pct_store(struct kobject *kobj, struct kobj_attribute *attr,
+			   const char *buf, size_t count)
+{
+	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(kobj, attr->attr.name);
+	struct intel_rps *rps = &gt->rps;
+	int ret;
+	u8 val;
+
+	ret = kstrtou8(buf, 10, &val);
+	if (ret)
+		return ret;
+
+	ret = intel_rps_set_up_threshold(rps, val);
+
+	return ret == 0 ? count : ret;
+}
+
+static struct kobj_attribute rps_up_threshold_pct =
+__ATTR(rps_up_threshold_pct, 0664,
+       rps_up_threshold_pct_show, rps_up_threshold_pct_store);
+
+static ssize_t
+rps_down_threshold_pct_show(struct kobject *kobj, struct kobj_attribute *attr,
+			    char *buf)
+{
+	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(kobj, attr->attr.name);
+	struct intel_rps *rps = &gt->rps;
+
+	return sysfs_emit(buf, "%u\n", intel_rps_get_down_threshold(rps));
+}
+
+static ssize_t
+rps_down_threshold_pct_store(struct kobject *kobj, struct kobj_attribute *attr,
+			     const char *buf, size_t count)
+{
+	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(kobj, attr->attr.name);
+	struct intel_rps *rps = &gt->rps;
+	int ret;
+	u8 val;
+
+	ret = kstrtou8(buf, 10, &val);
+	if (ret)
+		return ret;
+
+	ret = intel_rps_set_down_threshold(rps, val);
+
+	return ret == 0 ? count : ret;
+}
+
+static struct kobj_attribute rps_down_threshold_pct =
+__ATTR(rps_down_threshold_pct, 0664,
+       rps_down_threshold_pct_show, rps_down_threshold_pct_store);
+
+static const struct attribute * const gen6_gt_rps_attrs[] = {
+	&rps_up_threshold_pct.attr,
+	&rps_down_threshold_pct.attr,
+	NULL
+};
+
 static ssize_t
 default_min_freq_mhz_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
 {
@@ -693,9 +763,37 @@ default_max_freq_mhz_show(struct kobject *kobj, struct kobj_attribute *attr, cha
 static struct kobj_attribute default_max_freq_mhz =
 __ATTR(rps_max_freq_mhz, 0444, default_max_freq_mhz_show, NULL);
 
+static ssize_t
+default_rps_up_threshold_pct_show(struct kobject *kobj,
+				  struct kobj_attribute *attr,
+				  char *buf)
+{
+	struct intel_gt *gt = kobj_to_gt(kobj->parent);
+
+	return sysfs_emit(buf, "%u\n", gt->defaults.rps_up_threshold);
+}
+
+static struct kobj_attribute default_rps_up_threshold_pct =
+__ATTR(rps_up_threshold_pct, 0444, default_rps_up_threshold_pct_show, NULL);
+
+static ssize_t
+default_rps_down_threshold_pct_show(struct kobject *kobj,
+				    struct kobj_attribute *attr,
+				    char *buf)
+{
+	struct intel_gt *gt = kobj_to_gt(kobj->parent);
+
+	return sysfs_emit(buf, "%u\n", gt->defaults.rps_down_threshold);
+}
+
+static struct kobj_attribute default_rps_down_threshold_pct =
+__ATTR(rps_down_threshold_pct, 0444, default_rps_down_threshold_pct_show, NULL);
+
 static const struct attribute * const rps_defaults_attrs[] = {
 	&default_min_freq_mhz.attr,
 	&default_max_freq_mhz.attr,
+	&default_rps_up_threshold_pct.attr,
+	&default_rps_down_threshold_pct.attr,
 	NULL
 };
 
@@ -723,6 +821,12 @@ static int intel_sysfs_rps_init(struct intel_gt *gt, struct kobject *kobj)
 	if (IS_VALLEYVIEW(gt->i915) || IS_CHERRYVIEW(gt->i915))
 		ret = sysfs_create_file(kobj, vlv_attr);
 
+	if (is_object_gt(kobj) && !intel_uc_uses_guc_slpc(&gt->uc)) {
+		ret = sysfs_create_files(kobj, gen6_gt_rps_attrs);
+		if (ret)
+			return ret;
+	}
+
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c
index a5a7315f5ace..f790e81546ff 100644
--- a/drivers/gpu/drm/i915/gt/intel_rps.c
+++ b/drivers/gpu/drm/i915/gt/intel_rps.c
@@ -2588,7 +2588,12 @@ static int rps_set_threshold(struct intel_rps *rps, u8 *threshold, u8 val)
 	ret = mutex_lock_interruptible(&rps->lock);
 	if (ret)
 		return ret;
-	*threshold = val;
+	if (*threshold != val) {
+		*threshold = val;
+		intel_rps_set(rps, clamp(rps->cur_freq,
+					 rps->min_freq_softlimit,
+					 rps->max_freq_softlimit));
+	}
 	mutex_unlock(&rps->lock);
 
 	return 0;
-- 
2.37.2

[Intel-gfx] [RFC 4/4] drm/i915: Expose RPS thresholds in sysfs

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

User feedback indicates significant performance gains are possible in
specific games with non default RPS up/down thresholds.

Expose these tunables via sysfs which will allow users to achieve best
performance when running games and best power efficiency elsewhere.

Note this patch supports non GuC based platforms only.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
References: https://gitlab.freedesktop.org/drm/intel/-/issues/8389
---
 drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 104 ++++++++++++++++++++
 drivers/gpu/drm/i915/gt/intel_rps.c         |   7 +-
 2 files changed, 110 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
index 28f27091cd3b..df1f9ef08475 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
@@ -671,6 +671,76 @@ static const struct attribute *media_perf_power_attrs[] = {
 	NULL
 };
 
+static ssize_t
+rps_up_threshold_pct_show(struct kobject *kobj, struct kobj_attribute *attr,
+			  char *buf)
+{
+	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(kobj, attr->attr.name);
+	struct intel_rps *rps = &gt->rps;
+
+	return sysfs_emit(buf, "%u\n", intel_rps_get_up_threshold(rps));
+}
+
+static ssize_t
+rps_up_threshold_pct_store(struct kobject *kobj, struct kobj_attribute *attr,
+			   const char *buf, size_t count)
+{
+	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(kobj, attr->attr.name);
+	struct intel_rps *rps = &gt->rps;
+	int ret;
+	u8 val;
+
+	ret = kstrtou8(buf, 10, &val);
+	if (ret)
+		return ret;
+
+	ret = intel_rps_set_up_threshold(rps, val);
+
+	return ret == 0 ? count : ret;
+}
+
+static struct kobj_attribute rps_up_threshold_pct =
+__ATTR(rps_up_threshold_pct, 0664,
+       rps_up_threshold_pct_show, rps_up_threshold_pct_store);
+
+static ssize_t
+rps_down_threshold_pct_show(struct kobject *kobj, struct kobj_attribute *attr,
+			    char *buf)
+{
+	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(kobj, attr->attr.name);
+	struct intel_rps *rps = &gt->rps;
+
+	return sysfs_emit(buf, "%u\n", intel_rps_get_down_threshold(rps));
+}
+
+static ssize_t
+rps_down_threshold_pct_store(struct kobject *kobj, struct kobj_attribute *attr,
+			     const char *buf, size_t count)
+{
+	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(kobj, attr->attr.name);
+	struct intel_rps *rps = &gt->rps;
+	int ret;
+	u8 val;
+
+	ret = kstrtou8(buf, 10, &val);
+	if (ret)
+		return ret;
+
+	ret = intel_rps_set_down_threshold(rps, val);
+
+	return ret == 0 ? count : ret;
+}
+
+static struct kobj_attribute rps_down_threshold_pct =
+__ATTR(rps_down_threshold_pct, 0664,
+       rps_down_threshold_pct_show, rps_down_threshold_pct_store);
+
+static const struct attribute * const gen6_gt_rps_attrs[] = {
+	&rps_up_threshold_pct.attr,
+	&rps_down_threshold_pct.attr,
+	NULL
+};
+
 static ssize_t
 default_min_freq_mhz_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
 {
@@ -693,9 +763,37 @@ default_max_freq_mhz_show(struct kobject *kobj, struct kobj_attribute *attr, cha
 static struct kobj_attribute default_max_freq_mhz =
 __ATTR(rps_max_freq_mhz, 0444, default_max_freq_mhz_show, NULL);
 
+static ssize_t
+default_rps_up_threshold_pct_show(struct kobject *kobj,
+				  struct kobj_attribute *attr,
+				  char *buf)
+{
+	struct intel_gt *gt = kobj_to_gt(kobj->parent);
+
+	return sysfs_emit(buf, "%u\n", gt->defaults.rps_up_threshold);
+}
+
+static struct kobj_attribute default_rps_up_threshold_pct =
+__ATTR(rps_up_threshold_pct, 0444, default_rps_up_threshold_pct_show, NULL);
+
+static ssize_t
+default_rps_down_threshold_pct_show(struct kobject *kobj,
+				    struct kobj_attribute *attr,
+				    char *buf)
+{
+	struct intel_gt *gt = kobj_to_gt(kobj->parent);
+
+	return sysfs_emit(buf, "%u\n", gt->defaults.rps_down_threshold);
+}
+
+static struct kobj_attribute default_rps_down_threshold_pct =
+__ATTR(rps_down_threshold_pct, 0444, default_rps_down_threshold_pct_show, NULL);
+
 static const struct attribute * const rps_defaults_attrs[] = {
 	&default_min_freq_mhz.attr,
 	&default_max_freq_mhz.attr,
+	&default_rps_up_threshold_pct.attr,
+	&default_rps_down_threshold_pct.attr,
 	NULL
 };
 
@@ -723,6 +821,12 @@ static int intel_sysfs_rps_init(struct intel_gt *gt, struct kobject *kobj)
 	if (IS_VALLEYVIEW(gt->i915) || IS_CHERRYVIEW(gt->i915))
 		ret = sysfs_create_file(kobj, vlv_attr);
 
+	if (is_object_gt(kobj) && !intel_uc_uses_guc_slpc(&gt->uc)) {
+		ret = sysfs_create_files(kobj, gen6_gt_rps_attrs);
+		if (ret)
+			return ret;
+	}
+
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c
index a5a7315f5ace..f790e81546ff 100644
--- a/drivers/gpu/drm/i915/gt/intel_rps.c
+++ b/drivers/gpu/drm/i915/gt/intel_rps.c
@@ -2588,7 +2588,12 @@ static int rps_set_threshold(struct intel_rps *rps, u8 *threshold, u8 val)
 	ret = mutex_lock_interruptible(&rps->lock);
 	if (ret)
 		return ret;
-	*threshold = val;
+	if (*threshold != val) {
+		*threshold = val;
+		intel_rps_set(rps, clamp(rps->cur_freq,
+					 rps->min_freq_softlimit,
+					 rps->max_freq_softlimit));
+	}
 	mutex_unlock(&rps->lock);
 
 	return 0;
-- 
2.37.2

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for Expose RPS thresholds in sysfs (rev2)

[Patchwork]

== Series Details ==

Series: Expose RPS thresholds in sysfs (rev2)
URL   : https://patchwork.freedesktop.org/series/117054/
State : warning

== Summary ==

Error: dim checkpatch failed
561c9fe0d33c drm/i915: Move setting of rps thresholds to init
-:6: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 36d516be867c ("drm/i915/gt: Switch to manual evaluation of RPS")'
#6: 
Since 36d516be867c ("drm/i915/gt: Switch to manual evaluation of RPS")

total: 1 errors, 0 warnings, 0 checks, 73 lines checked
da670c452e89 drm/i915: Record default rps threshold values
3d681b908892 drm/i915: Add helpers for managing rps thresholds
19cabe9996b2 drm/i915: Expose RPS thresholds in sysfs
-:55: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#55: FILE: drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c:704:
+       rps_up_threshold_pct_show, rps_up_threshold_pct_store);$

-:87: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#87: FILE: drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c:736:
+       rps_down_threshold_pct_show, rps_down_threshold_pct_store);$

total: 0 errors, 2 warnings, 0 checks, 138 lines checked

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for Expose RPS thresholds in sysfs (rev2)

[Patchwork]

== Series Details ==

Series: Expose RPS thresholds in sysfs (rev2)
URL   : https://patchwork.freedesktop.org/series/117054/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

Re: [Intel-gfx] [RFC 4/4] drm/i915: Expose RPS thresholds in sysfs

[Tvrtko Ursulin]

On 28/04/2023 09:14, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> User feedback indicates significant performance gains are possible in
> specific games with non default RPS up/down thresholds.
> 
> Expose these tunables via sysfs which will allow users to achieve best
> performance when running games and best power efficiency elsewhere.
> 
> Note this patch supports non GuC based platforms only.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> References: https://gitlab.freedesktop.org/drm/intel/-/issues/8389

[snip]

> diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c
> index a5a7315f5ace..f790e81546ff 100644
> --- a/drivers/gpu/drm/i915/gt/intel_rps.c
> +++ b/drivers/gpu/drm/i915/gt/intel_rps.c
> @@ -2588,7 +2588,12 @@ static int rps_set_threshold(struct intel_rps *rps, u8 *threshold, u8 val)
>   	ret = mutex_lock_interruptible(&rps->lock);
>   	if (ret)
>   		return ret;
> -	*threshold = val;
> +	if (*threshold != val) {
> +		*threshold = val;
> +		intel_rps_set(rps, clamp(rps->cur_freq,
> +					 rps->min_freq_softlimit,
> +					 rps->max_freq_softlimit));
> +	}
>   	mutex_unlock(&rps->lock);
>   
>   	return 0;

This hunk belongs to a previous patch - moved locally.

Regards,

Tvrtko

[Intel-gfx] ✗ Fi.CI.BAT: failure for Expose RPS thresholds in sysfs (rev2)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 5937 bytes --]

== Series Details ==

Series: Expose RPS thresholds in sysfs (rev2)
URL   : https://patchwork.freedesktop.org/series/117054/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_13071 -> Patchwork_117054v2
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_117054v2 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_117054v2, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117054v2/index.html

Participating hosts (38 -> 37)
------------------------------

  Additional (1): fi-kbl-soraka 
  Missing    (2): bat-mtlp-8 fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_117054v2:

### IGT changes ###

#### Possible regressions ####

  * igt@gem_lmem_swapping@verify-random:
    - fi-kbl-soraka:      NOTRUN -> [INCOMPLETE][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117054v2/fi-kbl-soraka/igt@gem_lmem_swapping@verify-random.html

  
Known issues
------------

  Here are the changes found in Patchwork_117054v2 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_huc_copy@huc-copy:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][2] ([fdo#109271] / [i915#2190])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117054v2/fi-kbl-soraka/igt@gem_huc_copy@huc-copy.html

  * igt@gem_lmem_swapping@basic:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][3] ([fdo#109271] / [i915#4613]) +2 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117054v2/fi-kbl-soraka/igt@gem_lmem_swapping@basic.html

  * igt@i915_selftest@live@gt_pm:
    - fi-kbl-soraka:      NOTRUN -> [FAIL][4] ([i915#7913] / [i915#8383])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117054v2/fi-kbl-soraka/igt@i915_selftest@live@gt_pm.html

  * igt@kms_chamelium_frames@hdmi-crc-fast:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][5] ([fdo#109271]) +16 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117054v2/fi-kbl-soraka/igt@kms_chamelium_frames@hdmi-crc-fast.html

  * igt@kms_chamelium_hpd@common-hpd-after-suspend:
    - bat-rpls-1:         NOTRUN -> [SKIP][6] ([i915#7828])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117054v2/bat-rpls-1/igt@kms_chamelium_hpd@common-hpd-after-suspend.html

  * igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-1:
    - bat-dg2-8:          [PASS][7] -> [FAIL][8] ([i915#7932]) +1 similar issue
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13071/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-1.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117054v2/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-1.html

  * igt@kms_pipe_crc_basic@suspend-read-crc:
    - bat-rpls-1:         NOTRUN -> [SKIP][9] ([i915#1845])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117054v2/bat-rpls-1/igt@kms_pipe_crc_basic@suspend-read-crc.html

  
#### Possible fixes ####

  * igt@gem_exec_suspend@basic-s3@smem:
    - bat-rpls-1:         [ABORT][10] ([i915#6687]) -> [PASS][11]
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13071/bat-rpls-1/igt@gem_exec_suspend@basic-s3@smem.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117054v2/bat-rpls-1/igt@gem_exec_suspend@basic-s3@smem.html

  * igt@i915_selftest@live@gt_heartbeat:
    - fi-glk-j4005:       [FAIL][12] ([i915#7916]) -> [PASS][13]
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13071/fi-glk-j4005/igt@i915_selftest@live@gt_heartbeat.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117054v2/fi-glk-j4005/igt@i915_selftest@live@gt_heartbeat.html

  
#### Warnings ####

  * igt@core_hotunplug@unbind-rebind:
    - fi-kbl-8809g:       [ABORT][14] ([i915#8299]) -> [ABORT][15] ([i915#8299] / [i915#8397])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13071/fi-kbl-8809g/igt@core_hotunplug@unbind-rebind.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117054v2/fi-kbl-8809g/igt@core_hotunplug@unbind-rebind.html

  
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#6687]: https://gitlab.freedesktop.org/drm/intel/issues/6687
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
  [i915#7913]: https://gitlab.freedesktop.org/drm/intel/issues/7913
  [i915#7916]: https://gitlab.freedesktop.org/drm/intel/issues/7916
  [i915#7932]: https://gitlab.freedesktop.org/drm/intel/issues/7932
  [i915#8299]: https://gitlab.freedesktop.org/drm/intel/issues/8299
  [i915#8383]: https://gitlab.freedesktop.org/drm/intel/issues/8383
  [i915#8397]: https://gitlab.freedesktop.org/drm/intel/issues/8397


Build changes
-------------

  * Linux: CI_DRM_13071 -> Patchwork_117054v2

  CI-20190529: 20190529
  CI_DRM_13071: b9458e7075652669ec0e04abe039a5ed001701fe @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7273: f40ef4b058466219968b7792d22ff0648b82396b @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_117054v2: b9458e7075652669ec0e04abe039a5ed001701fe @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

6ffdb3fbddd1 drm/i915: Expose RPS thresholds in sysfs
fa42779cd9b5 drm/i915: Add helpers for managing rps thresholds
e09a6976a174 drm/i915: Record default rps threshold values
b87468817206 drm/i915: Move setting of rps thresholds to init

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117054v2/index.html

[-- Attachment #2: Type: text/html, Size: 7037 bytes --]

[RFC 4/4] drm/i915: Expose RPS thresholds in sysfs

[Prahlad Kilambi]

> One question is are we able to find a "one size fits all" values.

> However regardless of that, given we already expose frequency controls in sysfs
> with the same reasoning of allowing system owners explicit control if so wanted,
> I think exposing the thresholds can be equally justified.

Exposing these RPS thresholds via sysfs allows for dynamic tuning of these values at runtime. Common scenarios where we can benefit from variable frequency ramping include plugged in vs battery where differing thresholds allow to weight more for performance vs power. Data from testing on ChromeOS Gen12 platforms where GuC isn't enabled indicates gains > 10% across several games. See https://gitlab.freedesktop.org/drm/intel/-/issues/8389#note_1890428 for details.

[Intel-gfx] [RFC 4/4] drm/i915: Expose RPS thresholds in sysfs

[Prahlad Kilambi]

> One question is are we able to find a "one size fits all" values.

> However regardless of that, given we already expose frequency controls in sysfs
> with the same reasoning of allowing system owners explicit control if so wanted,
> I think exposing the thresholds can be equally justified.

Exposing these RPS thresholds via sysfs allows for dynamic tuning of these values at runtime. Common scenarios where we can benefit from variable frequency ramping include plugged in vs battery where differing thresholds allow to weight more for performance vs power. Data from testing on ChromeOS Gen12 platforms where GuC isn't enabled indicates gains > 10% across several games. See https://gitlab.freedesktop.org/drm/intel/-/issues/8389#note_1890428 for details.

Re: [Intel-gfx] [RFC 4/4] drm/i915: Expose RPS thresholds in sysfs

[Rodrigo Vivi]

On Fri, Apr 28, 2023 at 09:44:53AM +0100, Tvrtko Ursulin wrote:
> 
> On 28/04/2023 09:14, Tvrtko Ursulin wrote:
> > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > 
> > User feedback indicates significant performance gains are possible in
> > specific games with non default RPS up/down thresholds.
> > 
> > Expose these tunables via sysfs which will allow users to achieve best
> > performance when running games and best power efficiency elsewhere.
> > 
> > Note this patch supports non GuC based platforms only.
> > 
> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > References: https://gitlab.freedesktop.org/drm/intel/-/issues/8389
> 
> [snip]
> 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c
> > index a5a7315f5ace..f790e81546ff 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_rps.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_rps.c
> > @@ -2588,7 +2588,12 @@ static int rps_set_threshold(struct intel_rps *rps, u8 *threshold, u8 val)
> >   	ret = mutex_lock_interruptible(&rps->lock);
> >   	if (ret)
> >   		return ret;
> > -	*threshold = val;
> > +	if (*threshold != val) {
> > +		*threshold = val;
> > +		intel_rps_set(rps, clamp(rps->cur_freq,
> > +					 rps->min_freq_softlimit,
> > +					 rps->max_freq_softlimit));
> > +	}
> >   	mutex_unlock(&rps->lock);
> >   	return 0;
> 
> This hunk belongs to a previous patch - moved locally.

I probably missed something then becuase I didn't miss this in any
of the previous patches. To the point that this looked like a new
separated patch.

> 
> Regards,
> 
> Tvrtko

Re: [Intel-gfx] [RFC 3/4] drm/i915: Add helpers for managing rps thresholds

[Rodrigo Vivi]

On Fri, Apr 28, 2023 at 09:14:56AM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> In preparation for exposing via sysfs add helpers for managing rps
> thresholds.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_rps.c | 36 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/gt/intel_rps.h |  4 ++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c
> index a39eee444849..a5a7315f5ace 100644
> --- a/drivers/gpu/drm/i915/gt/intel_rps.c
> +++ b/drivers/gpu/drm/i915/gt/intel_rps.c
> @@ -2573,6 +2573,42 @@ int intel_rps_set_min_frequency(struct intel_rps *rps, u32 val)
>  		return set_min_freq(rps, val);
>  }
>  
> +u8 intel_rps_get_up_threshold(struct intel_rps *rps)
> +{
> +	return rps->power.up_threshold;
> +}
> +
> +static int rps_set_threshold(struct intel_rps *rps, u8 *threshold, u8 val)
> +{
> +	int ret;
> +
> +	if (val > 100)
> +		return -EINVAL;
> +
> +	ret = mutex_lock_interruptible(&rps->lock);
> +	if (ret)
> +		return ret;
> +	*threshold = val;
> +	mutex_unlock(&rps->lock);
> +
> +	return 0;
> +}
> +
> +int intel_rps_set_up_threshold(struct intel_rps *rps, u8 threshold)
> +{
> +	return rps_set_threshold(rps, &rps->power.up_threshold, threshold);
> +}
> +
> +u8 intel_rps_get_down_threshold(struct intel_rps *rps)
> +{
> +	return rps->power.down_threshold;
> +}
> +
> +int intel_rps_set_down_threshold(struct intel_rps *rps, u8 threshold)
> +{
> +	return rps_set_threshold(rps, &rps->power.down_threshold, threshold);
> +}
> +

Isn't this breaking compilation with the unused functions?

>  static void intel_rps_set_manual(struct intel_rps *rps, bool enable)
>  {
>  	struct intel_uncore *uncore = rps_to_uncore(rps);
> diff --git a/drivers/gpu/drm/i915/gt/intel_rps.h b/drivers/gpu/drm/i915/gt/intel_rps.h
> index a3fa987aa91f..92fb01f5a452 100644
> --- a/drivers/gpu/drm/i915/gt/intel_rps.h
> +++ b/drivers/gpu/drm/i915/gt/intel_rps.h
> @@ -37,6 +37,10 @@ void intel_rps_mark_interactive(struct intel_rps *rps, bool interactive);
>  
>  int intel_gpu_freq(struct intel_rps *rps, int val);
>  int intel_freq_opcode(struct intel_rps *rps, int val);
> +u8 intel_rps_get_up_threshold(struct intel_rps *rps);
> +int intel_rps_set_up_threshold(struct intel_rps *rps, u8 threshold);
> +u8 intel_rps_get_down_threshold(struct intel_rps *rps);
> +int intel_rps_set_down_threshold(struct intel_rps *rps, u8 threshold);
>  u32 intel_rps_read_actual_frequency(struct intel_rps *rps);
>  u32 intel_rps_read_actual_frequency_fw(struct intel_rps *rps);
>  u32 intel_rps_get_requested_frequency(struct intel_rps *rps);
> -- 
> 2.37.2
> 

Re: [RFC 4/4] drm/i915: Expose RPS thresholds in sysfs

[Rodrigo Vivi]

On Fri, May 19, 2023 at 07:36:56PM +0000, Prahlad Kilambi wrote:
> > One question is are we able to find a "one size fits all" values.
> 
> > However regardless of that, given we already expose frequency controls in sysfs
> > with the same reasoning of allowing system owners explicit control if so wanted,
> > I think exposing the thresholds can be equally justified.
> 
> Exposing these RPS thresholds via sysfs allows for dynamic tuning of these values at runtime. Common scenarios where we can benefit from variable frequency ramping include plugged in vs battery where differing thresholds allow to weight more for performance vs power. Data from testing on ChromeOS Gen12 platforms where GuC isn't enabled indicates gains > 10% across several games. See https://gitlab.freedesktop.org/drm/intel/-/issues/8389#note_1890428 for details.
> 

In general we should always try to reduce the knobs and specially with a register
that doesn't work with the new platforms with FW on control of all these variations.
But this is a compelling argument.

Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
(if patch 3 doesn't break compilation and the other chunk has the placement explained
this is a rv-b for the series)

Re: [Intel-gfx] [RFC 4/4] drm/i915: Expose RPS thresholds in sysfs

[Rodrigo Vivi]

On Fri, May 19, 2023 at 07:36:56PM +0000, Prahlad Kilambi wrote:
> > One question is are we able to find a "one size fits all" values.
> 
> > However regardless of that, given we already expose frequency controls in sysfs
> > with the same reasoning of allowing system owners explicit control if so wanted,
> > I think exposing the thresholds can be equally justified.
> 
> Exposing these RPS thresholds via sysfs allows for dynamic tuning of these values at runtime. Common scenarios where we can benefit from variable frequency ramping include plugged in vs battery where differing thresholds allow to weight more for performance vs power. Data from testing on ChromeOS Gen12 platforms where GuC isn't enabled indicates gains > 10% across several games. See https://gitlab.freedesktop.org/drm/intel/-/issues/8389#note_1890428 for details.
> 

In general we should always try to reduce the knobs and specially with a register
that doesn't work with the new platforms with FW on control of all these variations.
But this is a compelling argument.

Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
(if patch 3 doesn't break compilation and the other chunk has the placement explained
this is a rv-b for the series)

Re: [Intel-gfx] [RFC 3/4] drm/i915: Add helpers for managing rps thresholds

[Tvrtko Ursulin]

On 19/05/2023 21:56, Rodrigo Vivi wrote:
> On Fri, Apr 28, 2023 at 09:14:56AM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> In preparation for exposing via sysfs add helpers for managing rps
>> thresholds.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/intel_rps.c | 36 +++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/gt/intel_rps.h |  4 ++++
>>   2 files changed, 40 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c
>> index a39eee444849..a5a7315f5ace 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_rps.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_rps.c
>> @@ -2573,6 +2573,42 @@ int intel_rps_set_min_frequency(struct intel_rps *rps, u32 val)
>>   		return set_min_freq(rps, val);
>>   }
>>   
>> +u8 intel_rps_get_up_threshold(struct intel_rps *rps)
>> +{
>> +	return rps->power.up_threshold;
>> +}
>> +
>> +static int rps_set_threshold(struct intel_rps *rps, u8 *threshold, u8 val)
>> +{
>> +	int ret;
>> +
>> +	if (val > 100)
>> +		return -EINVAL;
>> +
>> +	ret = mutex_lock_interruptible(&rps->lock);
>> +	if (ret)
>> +		return ret;
>> +	*threshold = val;
>> +	mutex_unlock(&rps->lock);
>> +
>> +	return 0;
>> +}
>> +
>> +int intel_rps_set_up_threshold(struct intel_rps *rps, u8 threshold)
>> +{
>> +	return rps_set_threshold(rps, &rps->power.up_threshold, threshold);
>> +}
>> +
>> +u8 intel_rps_get_down_threshold(struct intel_rps *rps)
>> +{
>> +	return rps->power.down_threshold;
>> +}
>> +
>> +int intel_rps_set_down_threshold(struct intel_rps *rps, u8 threshold)
>> +{
>> +	return rps_set_threshold(rps, &rps->power.down_threshold, threshold);
>> +}
>> +
> 
> Isn't this breaking compilation with the unused functions?

I checked and no - guess because they are exported. Would you prefer me 
to squash 3&4 so that there isn't a patch which adds unused helpers?

Regards,

Tvrtko

> 
>>   static void intel_rps_set_manual(struct intel_rps *rps, bool enable)
>>   {
>>   	struct intel_uncore *uncore = rps_to_uncore(rps);
>> diff --git a/drivers/gpu/drm/i915/gt/intel_rps.h b/drivers/gpu/drm/i915/gt/intel_rps.h
>> index a3fa987aa91f..92fb01f5a452 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_rps.h
>> +++ b/drivers/gpu/drm/i915/gt/intel_rps.h
>> @@ -37,6 +37,10 @@ void intel_rps_mark_interactive(struct intel_rps *rps, bool interactive);
>>   
>>   int intel_gpu_freq(struct intel_rps *rps, int val);
>>   int intel_freq_opcode(struct intel_rps *rps, int val);
>> +u8 intel_rps_get_up_threshold(struct intel_rps *rps);
>> +int intel_rps_set_up_threshold(struct intel_rps *rps, u8 threshold);
>> +u8 intel_rps_get_down_threshold(struct intel_rps *rps);
>> +int intel_rps_set_down_threshold(struct intel_rps *rps, u8 threshold);
>>   u32 intel_rps_read_actual_frequency(struct intel_rps *rps);
>>   u32 intel_rps_read_actual_frequency_fw(struct intel_rps *rps);
>>   u32 intel_rps_get_requested_frequency(struct intel_rps *rps);
>> -- 
>> 2.37.2
>>

Re: [Intel-gfx] [RFC 4/4] drm/i915: Expose RPS thresholds in sysfs

[Tvrtko Ursulin]

On 19/05/2023 21:56, Rodrigo Vivi wrote:
> On Fri, Apr 28, 2023 at 09:44:53AM +0100, Tvrtko Ursulin wrote:
>>
>> On 28/04/2023 09:14, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> User feedback indicates significant performance gains are possible in
>>> specific games with non default RPS up/down thresholds.
>>>
>>> Expose these tunables via sysfs which will allow users to achieve best
>>> performance when running games and best power efficiency elsewhere.
>>>
>>> Note this patch supports non GuC based platforms only.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> References: https://gitlab.freedesktop.org/drm/intel/-/issues/8389
>>
>> [snip]
>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c
>>> index a5a7315f5ace..f790e81546ff 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_rps.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_rps.c
>>> @@ -2588,7 +2588,12 @@ static int rps_set_threshold(struct intel_rps *rps, u8 *threshold, u8 val)
>>>    	ret = mutex_lock_interruptible(&rps->lock);
>>>    	if (ret)
>>>    		return ret;
>>> -	*threshold = val;
>>> +	if (*threshold != val) {
>>> +		*threshold = val;
>>> +		intel_rps_set(rps, clamp(rps->cur_freq,
>>> +					 rps->min_freq_softlimit,
>>> +					 rps->max_freq_softlimit));
>>> +	}
>>>    	mutex_unlock(&rps->lock);
>>>    	return 0;
>>
>> This hunk belongs to a previous patch - moved locally.
> 
> I probably missed something then becuase I didn't miss this in any
> of the previous patches. To the point that this looked like a new
> separated patch.

Depends how you look at it. 3/4 adds a helper which changes the 
threshold, but it was only updating the sw state and so when those 
values would actually get written into registers (derived) would depend 
on the workload. So I thought it was important to move the call to 
intel_rps_set, which actually will trigger hw reprogramming, into the 
patch which claims to be adding the helper.

Note v2 of the helper (just sent) which actually fixes this further by 
forcing this re-programming to happen.

Regards,

Tvrtko

[RFC 3/4] drm/i915: Add helpers for managing rps thresholds

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

In preparation for exposing via sysfs add helpers for managing rps
thresholds.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_rps.c | 36 +++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/gt/intel_rps.h |  4 ++++
 2 files changed, 40 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c
index 343a50188c5e..c02b7104d11e 100644
--- a/drivers/gpu/drm/i915/gt/intel_rps.c
+++ b/drivers/gpu/drm/i915/gt/intel_rps.c
@@ -2573,6 +2573,42 @@ int intel_rps_set_min_frequency(struct intel_rps *rps, u32 val)
 		return set_min_freq(rps, val);
 }
 
+u8 intel_rps_get_up_threshold(struct intel_rps *rps)
+{
+	return rps->power.up_threshold;
+}
+
+static int rps_set_threshold(struct intel_rps *rps, u8 *threshold, u8 val)
+{
+	int ret;
+
+	if (val > 100)
+		return -EINVAL;
+
+	ret = mutex_lock_interruptible(&rps->lock);
+	if (ret)
+		return ret;
+	*threshold = val;
+	mutex_unlock(&rps->lock);
+
+	return 0;
+}
+
+int intel_rps_set_up_threshold(struct intel_rps *rps, u8 threshold)
+{
+	return rps_set_threshold(rps, &rps->power.up_threshold, threshold);
+}
+
+u8 intel_rps_get_down_threshold(struct intel_rps *rps)
+{
+	return rps->power.down_threshold;
+}
+
+int intel_rps_set_down_threshold(struct intel_rps *rps, u8 threshold)
+{
+	return rps_set_threshold(rps, &rps->power.down_threshold, threshold);
+}
+
 static void intel_rps_set_manual(struct intel_rps *rps, bool enable)
 {
 	struct intel_uncore *uncore = rps_to_uncore(rps);
diff --git a/drivers/gpu/drm/i915/gt/intel_rps.h b/drivers/gpu/drm/i915/gt/intel_rps.h
index a3fa987aa91f..92fb01f5a452 100644
--- a/drivers/gpu/drm/i915/gt/intel_rps.h
+++ b/drivers/gpu/drm/i915/gt/intel_rps.h
@@ -37,6 +37,10 @@ void intel_rps_mark_interactive(struct intel_rps *rps, bool interactive);
 
 int intel_gpu_freq(struct intel_rps *rps, int val);
 int intel_freq_opcode(struct intel_rps *rps, int val);
+u8 intel_rps_get_up_threshold(struct intel_rps *rps);
+int intel_rps_set_up_threshold(struct intel_rps *rps, u8 threshold);
+u8 intel_rps_get_down_threshold(struct intel_rps *rps);
+int intel_rps_set_down_threshold(struct intel_rps *rps, u8 threshold);
 u32 intel_rps_read_actual_frequency(struct intel_rps *rps);
 u32 intel_rps_read_actual_frequency_fw(struct intel_rps *rps);
 u32 intel_rps_get_requested_frequency(struct intel_rps *rps);
-- 
2.37.2