On 10/20/2022 3:57 PM, Dixit, Ashutosh wrote:
On Tue, 18 Oct 2022 11:30:31 -0700, Vinay Belgaumkar wrote:
Hi Vinay,
diff --git a/drivers/gpu/drm/i915/gt/selftest_slpc.c
b/drivers/gpu/drm/i915/gt/selftest_slpc.c
index 4c6e9257e593..e42bc215e54d 100644
--- a/drivers/gpu/drm/i915/gt/selftest_slpc.c
+++ b/drivers/gpu/drm/i915/gt/selftest_slpc.c
@@ -234,6 +234,7 @@ static int run_test(struct intel_gt *gt, int test_type)
enum intel_engine_id id;
struct igt_spinner spin;
u32 slpc_min_freq, slpc_max_freq;
+ u32 saved_min_freq;
int err = 0;
if (!intel_uc_uses_guc_slpc(>->uc))
@@ -252,20 +253,35 @@ static int run_test(struct intel_gt *gt, int test_type)
return -EIO;
}
- /*
- * 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.
- */
- err = slpc_set_min_freq(slpc, slpc->min_freq);
- if (err)
- return err;
+ if (slpc_min_freq == slpc_max_freq) {
+ /* Server parts will have min/max clamped to RP0 */
+ if (slpc->min_is_rpmax) {
+ err = slpc_set_min_freq(slpc, slpc->min_freq);
+ if (err) {
+ pr_err("Unable to update min freq on server
part");
+ return err;
+ }
- if (slpc->min_freq == slpc->rp0_freq) {
- pr_err("Min/Max are fused to the same value\n");
- return -EINVAL;
+ } else {
+ pr_err("Min/Max are fused to the same value\n");
+ return -EINVAL;
Sorry but I am not following this else case here. Why are we saying min/max
are fused to the same value? In this case we can't do
"slpc_set_min_freq(slpc, slpc->min_freq)" ? That is, we can't change SLPC
min freq?
This would be an error case due to a faulty part. We may come across a
part where min/max is fused to the same value.
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..b7cdeec44bd3 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,32 @@ static int slpc_set_softlimits(struct intel_guc_slpc *slpc)
return 0;
}
+static bool is_slpc_min_freq_rpmax(struct intel_guc_slpc *slpc)
+{
+ int slpc_min_freq;
+
+ if (intel_guc_slpc_get_min_freq(slpc, &slpc_min_freq))
+ return false;
I am wondering what happens if the above fails on server? Should we return
true or false on server and what are the consequences of returning false on
server?
Any case I think we should at least put a drm_err or something here just in
case this ever fails so we'll know something weird happened.
Makes sense.
Thanks,
Vinay.
+
+ 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 (is_slpc_min_freq_rpmax(slpc) &&
+ !slpc->min_freq_softlimit) {
+ 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 +674,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
This seems to be really a value (255 converted to freq) so seems ok to
intepret in MHz.
Thanks.
--
Ashutosh
