| Message ID | 20221013155524.25886-1-vinay.belgaumkar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/i915/slpc: Use platform limits for min/max frequency | expand |
On 10/13/2022 9:25 PM, 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) > > Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/7030 > > Acked-by: Nirmoy Das <nirmoy.das@intel.com> > Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com> Reviewed-by: Riana Tauro <riana.tauro@intel.com> > --- > drivers/gpu/drm/i915/gt/selftest_slpc.c | 40 +++++++++++++------ > drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 29 ++++++++++++++ > .../gpu/drm/i915/gt/uc/intel_guc_slpc_types.h | 3 ++ > 3 files changed, 60 insertions(+), 12 deletions(-) > > 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..11613d373a49 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,31 @@ 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->rp0_freq) > + 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; > + } > +} > + > static int slpc_use_fused_rp0(struct intel_guc_slpc *slpc) > { > /* Force SLPC to used platform rp0 */ > @@ -647,6 +673,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_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;
On Thu, 13 Oct 2022 08:55:24 -0700, Vinay Belgaumkar wrote: > Hi Vinay, > 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. Isn't what we are calling "theoretical max" or "RPmax" really just -1U (0xFFFFFFFF)? Though I have heard this is not a max value but -1U indicates FW default values unmodified by host SW, which would mean frequencies are fully controlled by FW (min == max == -1U). But if this were the case I don't know why this would be the case only for server, why doesn't FW set these for clients too to indicate it is fully in control? So the question what does -1U actually represent? Is it the RPmax value or does -1U represent "FW defaults"? Also this concept of using -1U as "FW defaults" is present in Level0/OneAPI (and likely in firmware) but we seem to have blocked in the i915 ABI. I understand we may not be able to make such changes at present but this provides some context for the review comments below. > 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..11613d373a49 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,31 @@ 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->rp0_freq) > or >=. If what we are calling "rpmax" really -1U then why don't we just check for -1U here? u32 slpc_min_freq; if (slpc_min_freq == -1U) > + 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; Isn't it safer to use a platform check such as IS_ATSM or IS_XEHPSDV (or even #define IS_SERVER()) to set min freq to RP0 rather than this -1U value from FW? What if -1U means "FW defaults" and FW starts setting this on client products tomorrow? Also, we need to set gt->defaults.min_freq here. Thanks. -- Ashutosh > + } > +} > + > static int slpc_use_fused_rp0(struct intel_guc_slpc *slpc) > { > /* Force SLPC to used platform rp0 */ > @@ -647,6 +673,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_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; > -- > 2.35.1 >
On 10/13/2022 3:28 PM, Dixit, Ashutosh wrote: > On Thu, 13 Oct 2022 08:55:24 -0700, Vinay Belgaumkar wrote: > Hi Vinay, > >> 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. > Isn't what we are calling "theoretical max" or "RPmax" really just -1U > (0xFFFFFFFF)? Though I have heard this is not a max value but -1U indicates > FW default values unmodified by host SW, which would mean frequencies are > fully controlled by FW (min == max == -1U). But if this were the case I > don't know why this would be the case only for server, why doesn't FW set > these for clients too to indicate it is fully in control? FW sets max to -1U for client products(we already pull it down to RP0). It additionally makes min=max for server parts. > > So the question what does -1U actually represent? Is it the RPmax value or > does -1U represent "FW defaults"? > > Also this concept of using -1U as "FW defaults" is present in Level0/OneAPI > (and likely in firmware) but we seem to have blocked in the i915 ABI. > > I understand we may not be able to make such changes at present but this > provides some context for the review comments below. > >> 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..11613d373a49 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,31 @@ 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->rp0_freq) >> or >=. > If what we are calling "rpmax" really -1U then why don't we just check for > -1U here? > > u32 slpc_min_freq; > > if (slpc_min_freq == -1U) That'll work similarly too. Only time slpc_min_freq is greater than rp0 is for a server part. > >> + 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; > Isn't it safer to use a platform check such as IS_ATSM or IS_XEHPSDV (or > even #define IS_SERVER()) to set min freq to RP0 rather than this -1U value > from FW? What if -1U means "FW defaults" and FW starts setting this on > client products tomorrow? We are not checking for -1 specifically, but only if FW has set min > RP0 as an indicator. Also, might be worth having IS_SERVER at some point if there are other places we need this info as well. > > Also, we need to set gt->defaults.min_freq here. yes, need to add that. Thanks, Vinay. > > Thanks. > -- > Ashutosh > > >> + } >> +} >> + >> static int slpc_use_fused_rp0(struct intel_guc_slpc *slpc) >> { >> /* Force SLPC to used platform rp0 */ >> @@ -647,6 +673,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_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; >> -- >> 2.35.1 >>
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..11613d373a49 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,31 @@ 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->rp0_freq) + 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; + } +} + static int slpc_use_fused_rp0(struct intel_guc_slpc *slpc) { /* Force SLPC to used platform rp0 */ @@ -647,6 +673,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_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;