| Message ID | 20221018183031.33704-1-vinay.belgaumkar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] drm/i915/slpc: Use platform limits for min/max frequency | expand |
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? > 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. > + > + 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
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
On Fri, 21 Oct 2022 18:38:57 -0700, Belgaumkar, Vinay wrote: > 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. But even then the original check is much clearer since it is actually comparing the fused freq's: if (slpc->min_freq == slpc->rp0_freq) Because if min/max have been changed slpc_min_freq and slpc_max_freq are no longer fused freq. And also this check should be right at the top of run_test, right after if (!intel_uc_uses_guc_slpc), rather than in the middle here (otherwise because we are basically not doing any error rewinding so causing memory leaks if any of the functions return error). > >>+ } > >>+ } else { > >>+ /* > >>+ * 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; > >> } So let's do what is suggested above and then see what remains here and if we need all these code changes. Most likely we can just do unconditionally what we were doing before, i.e.: err = slpc_set_min_freq(slpc, slpc->min_freq); if (err) return err; > >> > >>+ saved_min_freq = slpc_min_freq; > >>+ > >>+ /* New temp min freq = RPn */ > >>+ slpc_min_freq = slpc->min_freq; Why do we need saved_min_freq? We can retain slpc_min_freq and in the check below: if (max_act_freq <= slpc_min_freq) We can just change the check to: if (max_act_freq <= slpc->min_freq) Looks like to have been a bug in the original code? > >>+ > >> intel_gt_pm_wait_for_idle(gt); > >> intel_gt_pm_get(gt); > >> for_each_engine(engine, gt, id) { > >>@@ -347,7 +363,7 @@ static int run_test(struct intel_gt *gt, int test_type) > >> > >> /* Restore min/max frequencies */ > >> slpc_set_max_freq(slpc, slpc_max_freq); > >>- slpc_set_min_freq(slpc, slpc_min_freq); > >>+ slpc_set_min_freq(slpc, saved_min_freq); > >> > >> 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 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. > > >> + > >> + 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) { This should be swapped around: if (!slpc->min_freq_softlimit && is_slpc_min_freq_rpmax(slpc)) So we should only have to call is_slpc_min_freq_rpmax if slpc->min_freq_softlimit is 0 (that is only once the first time during init). > >> + 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
On 10/21/2022 10:26 PM, Dixit, Ashutosh wrote: > On Fri, 21 Oct 2022 18:38:57 -0700, Belgaumkar, Vinay wrote: >> 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. > But even then the original check is much clearer since it is actually > comparing the fused freq's: > > if (slpc->min_freq == slpc->rp0_freq) > > Because if min/max have been changed slpc_min_freq and slpc_max_freq are no > longer fused freq. > > And also this check should be right at the top of run_test, right after if > (!intel_uc_uses_guc_slpc), rather than in the middle here (otherwise > because we are basically not doing any error rewinding so causing memory > leaks if any of the functions return error). ok. > >>>> + } >>>> + } else { >>>> + /* >>>> + * 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; >>>> } > So let's do what is suggested above and then see what remains here and if > we need all these code changes. Most likely we can just do unconditionally > what we were doing before, i.e.: > > err = slpc_set_min_freq(slpc, slpc->min_freq); > if (err) > return err; > >>>> + saved_min_freq = slpc_min_freq; >>>> + >>>> + /* New temp min freq = RPn */ >>>> + slpc_min_freq = slpc->min_freq; > Why do we need saved_min_freq? We can retain slpc_min_freq and in the check below: > > if (max_act_freq <= slpc_min_freq) > > We can just change the check to: > > if (max_act_freq <= slpc->min_freq) > > Looks like to have been a bug in the original code? Not a bug, it wasn't needed until we didn't have server parts (slpc_min_freq would typically be slpc->min_freq on non-server parts). >>>> + >>>> intel_gt_pm_wait_for_idle(gt); >>>> intel_gt_pm_get(gt); >>>> for_each_engine(engine, gt, id) { >>>> @@ -347,7 +363,7 @@ static int run_test(struct intel_gt *gt, int test_type) >>>> >>>> /* Restore min/max frequencies */ >>>> slpc_set_max_freq(slpc, slpc_max_freq); >>>> - slpc_set_min_freq(slpc, slpc_min_freq); >>>> + slpc_set_min_freq(slpc, saved_min_freq); >>>> >>>> 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 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. >> >>>> + >>>> + 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) { > This should be swapped around: > > if (!slpc->min_freq_softlimit && is_slpc_min_freq_rpmax(slpc)) > > So we should only have to call is_slpc_min_freq_rpmax if > slpc->min_freq_softlimit is 0 (that is only once the first time during > init). ok. > >>>> + 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. yes, GuC returns the value in MHz. Thanks, Vinay. >>> >>> Thanks. >>> -- >>> Ashutosh
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; + } + } else { + /* + * 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; } + saved_min_freq = slpc_min_freq; + + /* New temp min freq = RPn */ + slpc_min_freq = slpc->min_freq; + intel_gt_pm_wait_for_idle(gt); intel_gt_pm_get(gt); for_each_engine(engine, gt, id) { @@ -347,7 +363,7 @@ static int run_test(struct intel_gt *gt, int test_type) /* Restore min/max frequencies */ slpc_set_max_freq(slpc, slpc_max_freq); - slpc_set_min_freq(slpc, slpc_min_freq); + slpc_set_min_freq(slpc, saved_min_freq); 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 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; + + 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 + 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;