| Message ID | 20230401020049.3843873-1-vinay.belgaumkar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i915/guc/slpc: Provide sysfs for efficient freq | expand |
On Fri, 31 Mar 2023 19:00:49 -0700, Vinay Belgaumkar wrote: > Hi Vinay, > @@ -478,20 +507,15 @@ int intel_guc_slpc_set_min_freq(struct intel_guc_slpc *slpc, u32 val) > val > slpc->max_freq_softlimit) > return -EINVAL; > > + /* Ignore efficient freq if lower min freq is requested */ > + ret = intel_guc_slpc_set_ignore_eff_freq(slpc, val < slpc->rp1_freq); > + if (ret) > + goto out; > + I don't agree with this. If we are now providing an interface explicitly to ignore RPe, that should be /only/ way to ignore RPe. There should be no other "under the hood" ignoring of RPe. In other words, ignoring RPe should be minimized unless explicitly requested. I don't clearly understand why this was done previously but it makes even less sense to me now after this patch. Thanks. -- Ashutosh > /* Need a lock now since waitboost can be modifying min as well */ > mutex_lock(&slpc->lock); > wakeref = intel_runtime_pm_get(&i915->runtime_pm); > > - /* Ignore efficient freq if lower min freq is requested */ > - ret = slpc_set_param(slpc, > - SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY, > - val < slpc->rp1_freq); > - if (ret) { > - guc_probe_error(slpc_to_guc(slpc), "Failed to toggle efficient freq: %pe\n", > - ERR_PTR(ret)); > - goto out; > - } > - > ret = slpc_set_param(slpc, > SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ, > val);
On Fri, Mar 31, 2023 at 08:11:29PM -0700, Dixit, Ashutosh wrote: > On Fri, 31 Mar 2023 19:00:49 -0700, Vinay Belgaumkar wrote: > > > > Hi Vinay, > > > @@ -478,20 +507,15 @@ int intel_guc_slpc_set_min_freq(struct intel_guc_slpc *slpc, u32 val) > > val > slpc->max_freq_softlimit) > > return -EINVAL; > > > > + /* Ignore efficient freq if lower min freq is requested */ > > + ret = intel_guc_slpc_set_ignore_eff_freq(slpc, val < slpc->rp1_freq); > > + if (ret) > > + goto out; > > + > > I don't agree with this. If we are now providing an interface explicitly to > ignore RPe, that should be /only/ way to ignore RPe. There should be no > other "under the hood" ignoring of RPe. In other words, ignoring RPe should > be minimized unless explicitly requested. > > I don't clearly understand why this was done previously but it makes even > less sense to me now after this patch. well, I had suggested this previously. And just because without this we would be breaking API expectations. When user selects a minimal frequency it expect that to stick. But with the efficient freq enabled in guc if minimal is less than the efficient one, this request is likely ignored. Well, even worse is that we are actually caching the request in the soft values. So we show a minimal, but the hardware without any workload is operating at efficient. So, the thought process was: 'if user requested a very low minimal, we give them the minimal requested, even if that means to disable the efficient freq.' So, that was introduced to avoid API breakage. Removing it now would mean breaking API. (Not sure if the IGT tests for the API got merged already, but think that as the API contract). But I do agree with you that having something selected from multiple places also has the potential to cause some miss-expectations. So I was thinking about multiple even orders where the user select the RP0 as minimal, then enable the efficient or vice versa, but I couldn't think of a bad case. Or at least not as bad as the user asking to get RP0 as minimal and only getting RPe back. With this in mind, and having checked the code: Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> But I won't push this immediately because I'm still open to hear another side/angle. > > Thanks. > -- > Ashutosh > > > > /* Need a lock now since waitboost can be modifying min as well */ > > mutex_lock(&slpc->lock); > > wakeref = intel_runtime_pm_get(&i915->runtime_pm); > > > > - /* Ignore efficient freq if lower min freq is requested */ > > - ret = slpc_set_param(slpc, > > - SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY, > > - val < slpc->rp1_freq); > > - if (ret) { > > - guc_probe_error(slpc_to_guc(slpc), "Failed to toggle efficient freq: %pe\n", > > - ERR_PTR(ret)); > > - goto out; > > - } > > - > > ret = slpc_set_param(slpc, > > SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ, > > val);
On Wed, 05 Apr 2023 06:57:42 -0700, Rodrigo Vivi wrote: > Hi Rodrigo, > On Fri, Mar 31, 2023 at 08:11:29PM -0700, Dixit, Ashutosh wrote: > > On Fri, 31 Mar 2023 19:00:49 -0700, Vinay Belgaumkar wrote: > > > > > > > Hi Vinay, > > > > > @@ -478,20 +507,15 @@ int intel_guc_slpc_set_min_freq(struct intel_guc_slpc *slpc, u32 val) > > > val > slpc->max_freq_softlimit) > > > return -EINVAL; > > > > > > + /* Ignore efficient freq if lower min freq is requested */ > > > + ret = intel_guc_slpc_set_ignore_eff_freq(slpc, val < slpc->rp1_freq); > > > + if (ret) > > > + goto out; > > > + > > > > I don't agree with this. If we are now providing an interface explicitly to > > ignore RPe, that should be /only/ way to ignore RPe. There should be no > > other "under the hood" ignoring of RPe. In other words, ignoring RPe should > > be minimized unless explicitly requested. > > > > I don't clearly understand why this was done previously but it makes even > > less sense to me now after this patch. > > well, I had suggested this previously. And just because without this we would > be breaking API expectations. > > When user selects a minimal frequency it expect that to stick. But with the > efficient freq enabled in guc if minimal is less than the efficient one, > this request is likely ignored. > > Well, even worse is that we are actually caching the request in the soft values. > So we show a minimal, but the hardware without any workload is operating at > efficient. > > So, the thought process was: 'if user requested a very low minimal, we give them > the minimal requested, even if that means to disable the efficient freq.' Hmm, I understand this even less now :) * Why is RPe ignored when min < RPe? Since the freq can be between min and max? Shouldn't the condition be min > RPe, that is turn RPe off if min higher that RPe is requested? * Also isn't RPe dynamic, so we can't say RPe == rp1 when using in KMD? * Finally, we know that enabling RPe broke the kernel freq API because RPe could go over max_freq. So it is actually the max freq which is not obeyed after RPe is enabled. So we ignore RPe in some select cases (which also I don't understand as mentioned above but maybe I am missing something) to claim that we are obeying the freq API, but let the freq API stay broken in other cases? > So, that was introduced to avoid API breakage. Removing it now would mean > breaking API. (Not sure if the IGT tests for the API got merged already, > but think that as the API contract). I think we should take this patch as an opportunity to fix this and give the user a clean interface to ignore RPe and remove this other implicit way to ignore RPe. All IGT changes are unmerged at present. Thanks. -- Ashutosh > > But I do agree with you that having something selected from multiple places > also has the potential to cause some miss-expectations. So I was thinking > about multiple even orders where the user select the RP0 as minimal, then > enable the efficient or vice versa, but I couldn't think of a bad case. > Or at least not as bad as the user asking to get RP0 as minimal and only > getting RPe back. > > With this in mind, and having checked the code: > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > But I won't push this immediately because I'm still open to hear another > side/angle. > > > > > Thanks. > > -- > > Ashutosh > > > > > > > /* Need a lock now since waitboost can be modifying min as well */ > > > mutex_lock(&slpc->lock); > > > wakeref = intel_runtime_pm_get(&i915->runtime_pm); > > > > > > - /* Ignore efficient freq if lower min freq is requested */ > > > - ret = slpc_set_param(slpc, > > > - SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY, > > > - val < slpc->rp1_freq); > > > - if (ret) { > > > - guc_probe_error(slpc_to_guc(slpc), "Failed to toggle efficient freq: %pe\n", > > > - ERR_PTR(ret)); > > > - goto out; > > > - } > > > - > > > ret = slpc_set_param(slpc, > > > SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ, > > > val);
On Wed, Apr 05, 2023 at 12:42:30PM -0700, Dixit, Ashutosh wrote: > On Wed, 05 Apr 2023 06:57:42 -0700, Rodrigo Vivi wrote: > > > > Hi Rodrigo, > > > On Fri, Mar 31, 2023 at 08:11:29PM -0700, Dixit, Ashutosh wrote: > > > On Fri, 31 Mar 2023 19:00:49 -0700, Vinay Belgaumkar wrote: > > > > > > > > > > Hi Vinay, > > > > > > > @@ -478,20 +507,15 @@ int intel_guc_slpc_set_min_freq(struct intel_guc_slpc *slpc, u32 val) > > > > val > slpc->max_freq_softlimit) > > > > return -EINVAL; > > > > > > > > + /* Ignore efficient freq if lower min freq is requested */ > > > > + ret = intel_guc_slpc_set_ignore_eff_freq(slpc, val < slpc->rp1_freq); > > > > + if (ret) > > > > + goto out; > > > > + > > > > > > I don't agree with this. If we are now providing an interface explicitly to > > > ignore RPe, that should be /only/ way to ignore RPe. There should be no > > > other "under the hood" ignoring of RPe. In other words, ignoring RPe should > > > be minimized unless explicitly requested. > > > > > > I don't clearly understand why this was done previously but it makes even > > > less sense to me now after this patch. > > > > well, I had suggested this previously. And just because without this we would > > be breaking API expectations. > > > > When user selects a minimal frequency it expect that to stick. But with the > > efficient freq enabled in guc if minimal is less than the efficient one, > > this request is likely ignored. > > > > Well, even worse is that we are actually caching the request in the soft values. > > So we show a minimal, but the hardware without any workload is operating at > > efficient. > > > > So, the thought process was: 'if user requested a very low minimal, we give them > > the minimal requested, even if that means to disable the efficient freq.' > > Hmm, I understand this even less now :) > > * Why is RPe ignored when min < RPe? Since the freq can be between min and > max? Shouldn't the condition be min > RPe, that is turn RPe off if min > higher that RPe is requested? that is not how guc efficient freq selection works. (unless my memory is tricking me right now.) So, if we select a min that is between RPe and RP0, guc will respect and use the selected min. So we don't need to disable guc selection of the efficient. This is not true when we select a very low min like RPn. If we select RPn as min and guc efficient freq selection is enabled guc will simply ignore our request. So the only way to give the user what is asked, is to also disable guc's efficient freq selection. (I probably confused you in the previous email because I used 'RP0' when I meant 'RPn'. I hope it gets clear now). > > * Also isn't RPe dynamic, so we can't say RPe == rp1 when using in KMD? Oh... yeap, this is an issue indeed. Specially with i915 where we have the soft values cached instead of asking guc everytime. That's a good point. The variance is not big, but we will hit corner cases. One way is to keep checking and updating everytime a sysfs is touched. Other way is do what you are suggesting and let's just accept and deal with the reality that is: "we cannot guarantee a min freq selection if user doesn't disable the efficient freq selection". > > * Finally, we know that enabling RPe broke the kernel freq API because RPe > could go over max_freq. So it is actually the max freq which is not > obeyed after RPe is enabled. Oh! so it was my bad memory indeed and everything is the other way around? But I just looked to Xe code, my most recent memory, and I just needed to toggle the efficient freq off on the case that I mentioned, when min selection is below the efficient one. With that all the API expectation that I coded in IGT works neatly. > > So we ignore RPe in some select cases (which also I don't understand as > mentioned above but maybe I am missing something) to claim that we are > obeying the freq API, but let the freq API stay broken in other cases? what cases it stays broken? This is why we need the IGT tests for all the API behavior in place. > > > So, that was introduced to avoid API breakage. Removing it now would mean > > breaking API. (Not sure if the IGT tests for the API got merged already, > > but think that as the API contract). > > I think we should take this patch as an opportunity to fix this and give > the user a clean interface to ignore RPe and remove this other implicit way > to ignore RPe. All IGT changes are unmerged at present. Yeap, the IGT needs to come with whatever we concluded here and we need to stick with that afterwards, so let's think with care. Vinay, Ashutosh's strongest argument is the variable RPe. Do you have thoughts on that? > > Thanks. > -- > Ashutosh > > > > > > > But I do agree with you that having something selected from multiple places > > also has the potential to cause some miss-expectations. So I was thinking > > about multiple even orders where the user select the RP0 as minimal, then > > enable the efficient or vice versa, but I couldn't think of a bad case. > > Or at least not as bad as the user asking to get RP0 as minimal and only > > getting RPe back. in case I let you confused here, what I meant was RPn, not RP0. > > > > With this in mind, and having checked the code: > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > But I won't push this immediately because I'm still open to hear another > > side/angle. > > > > > > > > Thanks. > > > -- > > > Ashutosh > > > > > > > > > > /* Need a lock now since waitboost can be modifying min as well */ > > > > mutex_lock(&slpc->lock); > > > > wakeref = intel_runtime_pm_get(&i915->runtime_pm); > > > > > > > > - /* Ignore efficient freq if lower min freq is requested */ > > > > - ret = slpc_set_param(slpc, > > > > - SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY, > > > > - val < slpc->rp1_freq); > > > > - if (ret) { > > > > - guc_probe_error(slpc_to_guc(slpc), "Failed to toggle efficient freq: %pe\n", > > > > - ERR_PTR(ret)); > > > > - goto out; > > > > - } > > > > - > > > > ret = slpc_set_param(slpc, > > > > SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ, > > > > val);
On Wed, 05 Apr 2023 13:12:29 -0700, Rodrigo Vivi wrote: > > On Wed, Apr 05, 2023 at 12:42:30PM -0700, Dixit, Ashutosh wrote: > > On Wed, 05 Apr 2023 06:57:42 -0700, Rodrigo Vivi wrote: > > > Hi Rodrigo, > > > > > On Fri, Mar 31, 2023 at 08:11:29PM -0700, Dixit, Ashutosh wrote: > > > > On Fri, 31 Mar 2023 19:00:49 -0700, Vinay Belgaumkar wrote: > > > > > > > > > > > > > Hi Vinay, > > > > > > > > > @@ -478,20 +507,15 @@ int intel_guc_slpc_set_min_freq(struct intel_guc_slpc *slpc, u32 val) > > > > > val > slpc->max_freq_softlimit) > > > > > return -EINVAL; > > > > > > > > > > + /* Ignore efficient freq if lower min freq is requested */ > > > > > + ret = intel_guc_slpc_set_ignore_eff_freq(slpc, val < slpc->rp1_freq); > > > > > + if (ret) > > > > > + goto out; > > > > > + > > > > > > > > I don't agree with this. If we are now providing an interface explicitly to > > > > ignore RPe, that should be /only/ way to ignore RPe. There should be no > > > > other "under the hood" ignoring of RPe. In other words, ignoring RPe should > > > > be minimized unless explicitly requested. > > > > > > > > I don't clearly understand why this was done previously but it makes even > > > > less sense to me now after this patch. > > > > > > well, I had suggested this previously. And just because without this we would > > > be breaking API expectations. > > > > > > When user selects a minimal frequency it expect that to stick. But with the > > > efficient freq enabled in guc if minimal is less than the efficient one, > > > this request is likely ignored. > > > > > > Well, even worse is that we are actually caching the request in the soft values. > > > So we show a minimal, but the hardware without any workload is operating at > > > efficient. > > > > > > So, the thought process was: 'if user requested a very low minimal, we give them > > > the minimal requested, even if that means to disable the efficient freq.' > > > > Hmm, I understand this even less now :) > > > > * Why is RPe ignored when min < RPe? Since the freq can be between min and > > max? Shouldn't the condition be min > RPe, that is turn RPe off if min > > higher that RPe is requested? > > that is not how guc efficient freq selection works. (unless my memory is > tricking me right now.) > > So, if we select a min that is between RPe and RP0, guc will respect and > use the selected min. So we don't need to disable guc selection of the > efficient. > > This is not true when we select a very low min like RPn. If we select RPn > as min and guc efficient freq selection is enabled guc will simply ignore > our request. So the only way to give the user what is asked, is to also > disable guc's efficient freq selection. (I probably confused you in the > previous email because I used 'RP0' when I meant 'RPn'. I hope it gets > clear now). > > > > > * Also isn't RPe dynamic, so we can't say RPe == rp1 when using in KMD? > > Oh... yeap, this is an issue indeed. Specially with i915 where we have > the soft values cached instead of asking guc everytime. > > That's a good point. The variance is not big, but we will hit corner cases. > One way is to keep checking and updating everytime a sysfs is touched. This I believe not possible in all cases. Say the freq's are set through sysfs first and the workload starts later. In this case RPe will probably start changing after the workload starts, not when freq's are set in sysfs. > Other way is do what you are suggesting and let's just accept and deal > with the reality that is: "we cannot guarantee a min freq selection if user > doesn't disable the efficient freq selection". > > > > > * Finally, we know that enabling RPe broke the kernel freq API because RPe > > could go over max_freq. So it is actually the max freq which is not > > obeyed after RPe is enabled. > > Oh! so it was my bad memory indeed and everything is the other way around? > But I just looked to Xe code, my most recent memory, and I just needed > to toggle the efficient freq off on the case that I mentioned, when min > selection is below the efficient one. With that all the API expectation > that I coded in IGT works neatly. From what I saw the following bugs: https://gitlab.freedesktop.org/drm/intel/-/issues/6806 https://gitlab.freedesktop.org/drm/intel/-/issues/6786 and the following patches in response to these bugs (as well as the discussion on these patches): https://patchwork.freedesktop.org/series/111282/ https://patchwork.freedesktop.org/series/110574/ were all due to the fact that the max freq is not obeyed after RPe is enabled. These patches were never merged but I will now try to submit them again after this ignore efficient freq patch gets reviewed and merged. Thanks. -- Ashutosh > > > > So we ignore RPe in some select cases (which also I don't understand as > > mentioned above but maybe I am missing something) to claim that we are > > obeying the freq API, but let the freq API stay broken in other cases? > > what cases it stays broken? This is why we need the IGT tests for all the > API behavior in place. > > > > So, that was introduced to avoid API breakage. Removing it now would mean > > > breaking API. (Not sure if the IGT tests for the API got merged already, > > > but think that as the API contract). > > > > I think we should take this patch as an opportunity to fix this and give > > the user a clean interface to ignore RPe and remove this other implicit way > > to ignore RPe. All IGT changes are unmerged at present. > > Yeap, the IGT needs to come with whatever we concluded here and we need to > stick with that afterwards, so let's think with care. > > Vinay, Ashutosh's strongest argument is the variable RPe. Do you have thoughts > on that? > > > > > Thanks. > > -- > > Ashutosh > > > > > > > > > > > > But I do agree with you that having something selected from multiple places > > > also has the potential to cause some miss-expectations. So I was thinking > > > about multiple even orders where the user select the RP0 as minimal, then > > > enable the efficient or vice versa, but I couldn't think of a bad case. > > > Or at least not as bad as the user asking to get RP0 as minimal and only > > > getting RPe back. > > in case I let you confused here, what I meant was RPn, not RP0. > > > > > > > With this in mind, and having checked the code: > > > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > > But I won't push this immediately because I'm still open to hear another > > > side/angle. > > > > > > > > > > > Thanks. > > > > -- > > > > Ashutosh > > > > > > > > > > > > > /* Need a lock now since waitboost can be modifying min as well */ > > > > > mutex_lock(&slpc->lock); > > > > > wakeref = intel_runtime_pm_get(&i915->runtime_pm); > > > > > > > > > > - /* Ignore efficient freq if lower min freq is requested */ > > > > > - ret = slpc_set_param(slpc, > > > > > - SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY, > > > > > - val < slpc->rp1_freq); > > > > > - if (ret) { > > > > > - guc_probe_error(slpc_to_guc(slpc), "Failed to toggle efficient freq: %pe\n", > > > > > - ERR_PTR(ret)); > > > > > - goto out; > > > > > - } > > > > > - > > > > > ret = slpc_set_param(slpc, > > > > > SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ, > > > > > val);
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..7496de5be580 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c @@ -451,6 +451,33 @@ static ssize_t punit_req_freq_mhz_show(struct kobject *kobj, return sysfs_emit(buff, "%u\n", preq); } +static ssize_t slpc_ignore_eff_freq_show(struct kobject *kobj, + struct kobj_attribute *attr, + char *buff) +{ + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(kobj, attr->attr.name); + struct intel_guc_slpc *slpc = >->uc.guc.slpc; + + return sysfs_emit(buff, "%u\n", slpc->ignore_eff_freq); +} + +static ssize_t slpc_ignore_eff_freq_store(struct kobject *kobj, + struct kobj_attribute *attr, + const char *buff, size_t count) +{ + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(kobj, attr->attr.name); + struct intel_guc_slpc *slpc = >->uc.guc.slpc; + int err; + uint32_t val; + + err = kstrtou32(buff, 0, &val); + if (err) + return err; + + err = intel_guc_slpc_set_ignore_eff_freq(slpc, val); + return err ?: count; +} + struct intel_gt_bool_throttle_attr { struct attribute attr; ssize_t (*show)(struct kobject *kobj, struct kobj_attribute *attr, @@ -663,6 +690,8 @@ static struct kobj_attribute attr_media_freq_factor_scale = INTEL_GT_ATTR_RO(media_RP0_freq_mhz); INTEL_GT_ATTR_RO(media_RPn_freq_mhz); +INTEL_GT_ATTR_RW(slpc_ignore_eff_freq); + static const struct attribute *media_perf_power_attrs[] = { &attr_media_freq_factor.attr, &attr_media_freq_factor_scale.attr, @@ -744,6 +773,12 @@ void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj) if (ret) gt_warn(gt, "failed to create punit_req_freq_mhz sysfs (%pe)", ERR_PTR(ret)); + if (intel_uc_uses_guc_slpc(>->uc)) { + ret = sysfs_create_file(kobj, &attr_slpc_ignore_eff_freq.attr); + if (ret) + gt_warn(gt, "failed to create ignore_eff_freq sysfs (%pe)", ERR_PTR(ret)); + } + if (i915_mmio_reg_valid(intel_gt_perf_limit_reasons_reg(gt))) { ret = sysfs_create_files(kobj, throttle_reason_attrs); if (ret) 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 026d73855f36..eaa73c1fba6d 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c @@ -277,6 +277,7 @@ int intel_guc_slpc_init(struct intel_guc_slpc *slpc) slpc->max_freq_softlimit = 0; slpc->min_freq_softlimit = 0; + slpc->ignore_eff_freq = false; slpc->min_is_rpmax = false; slpc->boost_freq = 0; @@ -457,6 +458,34 @@ int intel_guc_slpc_get_max_freq(struct intel_guc_slpc *slpc, u32 *val) return ret; } +int intel_guc_slpc_set_ignore_eff_freq(struct intel_guc_slpc *slpc, bool val) +{ + struct drm_i915_private *i915 = slpc_to_i915(slpc); + intel_wakeref_t wakeref; + int ret = 0; + + /* Need a lock now since waitboost can be modifying min as well */ + mutex_lock(&slpc->lock); + wakeref = intel_runtime_pm_get(&i915->runtime_pm); + + /* Ignore efficient freq if lower min freq is requested */ + ret = slpc_set_param(slpc, + SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY, + val); + if (ret) { + guc_probe_error(slpc_to_guc(slpc), "Failed to set efficient freq(%d): %pe\n", + val, ERR_PTR(ret)); + goto out; + } + + slpc->ignore_eff_freq = val; + +out: + intel_runtime_pm_put(&i915->runtime_pm, wakeref); + mutex_unlock(&slpc->lock); + return ret; +} + /** * intel_guc_slpc_set_min_freq() - Set min frequency limit for SLPC. * @slpc: pointer to intel_guc_slpc. @@ -478,20 +507,15 @@ int intel_guc_slpc_set_min_freq(struct intel_guc_slpc *slpc, u32 val) val > slpc->max_freq_softlimit) return -EINVAL; + /* Ignore efficient freq if lower min freq is requested */ + ret = intel_guc_slpc_set_ignore_eff_freq(slpc, val < slpc->rp1_freq); + if (ret) + goto out; + /* Need a lock now since waitboost can be modifying min as well */ mutex_lock(&slpc->lock); wakeref = intel_runtime_pm_get(&i915->runtime_pm); - /* Ignore efficient freq if lower min freq is requested */ - ret = slpc_set_param(slpc, - SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY, - val < slpc->rp1_freq); - if (ret) { - guc_probe_error(slpc_to_guc(slpc), "Failed to toggle efficient freq: %pe\n", - ERR_PTR(ret)); - goto out; - } - ret = slpc_set_param(slpc, SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ, val); @@ -499,10 +523,10 @@ int intel_guc_slpc_set_min_freq(struct intel_guc_slpc *slpc, u32 val) if (!ret) slpc->min_freq_softlimit = val; -out: intel_runtime_pm_put(&i915->runtime_pm, wakeref); mutex_unlock(&slpc->lock); +out: /* Return standardized err code for sysfs calls */ if (ret) ret = -EIO; @@ -752,6 +776,9 @@ int intel_guc_slpc_enable(struct intel_guc_slpc *slpc) /* Set cached media freq ratio mode */ intel_guc_slpc_set_media_ratio_mode(slpc, slpc->media_ratio_mode); + /* Set cached value of ignore efficient freq */ + intel_guc_slpc_set_ignore_eff_freq(slpc, slpc->ignore_eff_freq); + return 0; } 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 17ed515f6a85..597eb5413ddf 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h @@ -46,5 +46,6 @@ void intel_guc_slpc_boost(struct intel_guc_slpc *slpc); void intel_guc_slpc_dec_waiters(struct intel_guc_slpc *slpc); int intel_guc_slpc_unset_gucrc_mode(struct intel_guc_slpc *slpc); int intel_guc_slpc_override_gucrc_mode(struct intel_guc_slpc *slpc, u32 mode); +int intel_guc_slpc_set_ignore_eff_freq(struct intel_guc_slpc *slpc, bool val); #endif 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 a6ef53b04e04..a88651331497 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 @@ -31,6 +31,7 @@ struct intel_guc_slpc { /* frequency softlimits */ u32 min_freq_softlimit; u32 max_freq_softlimit; + bool ignore_eff_freq; /* cached media ratio mode */ u32 media_ratio_mode;
SLPC enables use of efficient freq at init by default. It is possible for GuC to request frequencies that are higher than the 'software' max if user has set it lower than the efficient level. Scenarios/tests that require strict fixing of freq below the efficient level will need to disable it through this interface. Another way to disable it is to set min frequency below the efficient level. Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com> --- drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 35 +++++++++++++ drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 49 ++++++++++++++----- drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h | 1 + .../gpu/drm/i915/gt/uc/intel_guc_slpc_types.h | 1 + 4 files changed, 75 insertions(+), 11 deletions(-)