Re: [Intel-gfx] [PATCH v3] drm/i915/guc/slpc: Provide sysfs for efficient freq
On 4/14/2023 4:49 PM, Dixit, Ashutosh wrote:
On Fri, 14 Apr 2023 15:34:15 -0700, Vinay Belgaumkar wrote:
@@ -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 */
Delete comment.
ok.
+ mutex_lock(>lock);
Actually, don't need the lock itself now so delete the lock.
Or, maybe the lock prevents the race if userspace writes to the sysfs when
GuC reset is going on so let's retain the lock. But the comment is wrong.
yup, ok.
+ wakeref = intel_runtime_pm_get(>runtime_pm);
+
+ /* Ignore efficient freq if lower min freq is requested */
Delete comment, it's wrong.
ok.
+ 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;
+
This extra line can also be deleted.
ok.
+out:
+ intel_runtime_pm_put(>runtime_pm, wakeref);
+ mutex_unlock(>lock);
+ return ret;
+}
+
/**
* intel_guc_slpc_set_min_freq() - Set min frequency limit for SLPC.
* @slpc: pointer to intel_guc_slpc.
@@ -482,16 +511,6 @@ int intel_guc_slpc_set_min_freq(struct intel_guc_slpc
*slpc, u32 val)
mutex_lock(>lock);
wakeref = intel_runtime_pm_get(>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;
- }
-
Great, thanks!
After taking care of the above, and seems there are also a couple of
checkpatch errors, this is:
Reviewed-by: Ashutosh Dixit
Thanks,
Vinay.
Re: [Intel-gfx] [PATCH v3] drm/i915/guc/slpc: Provide sysfs for efficient freq
On Fri, 14 Apr 2023 15:34:15 -0700, Vinay Belgaumkar wrote:
>
> @@ -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 */
Delete comment.
> + mutex_lock(>lock);
Actually, don't need the lock itself now so delete the lock.
Or, maybe the lock prevents the race if userspace writes to the sysfs when
GuC reset is going on so let's retain the lock. But the comment is wrong.
> + wakeref = intel_runtime_pm_get(>runtime_pm);
> +
> + /* Ignore efficient freq if lower min freq is requested */
Delete comment, it's wrong.
> + 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;
> +
This extra line can also be deleted.
> +out:
> + intel_runtime_pm_put(>runtime_pm, wakeref);
> + mutex_unlock(>lock);
> + return ret;
> +}
> +
> /**
> * intel_guc_slpc_set_min_freq() - Set min frequency limit for SLPC.
> * @slpc: pointer to intel_guc_slpc.
> @@ -482,16 +511,6 @@ int intel_guc_slpc_set_min_freq(struct intel_guc_slpc
> *slpc, u32 val)
> mutex_lock(>lock);
> wakeref = intel_runtime_pm_get(>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;
> - }
> -
Great, thanks!
After taking care of the above, and seems there are also a couple of
checkpatch errors, this is:
Reviewed-by: Ashutosh Dixit
[Intel-gfx] [PATCH v3] drm/i915/guc/slpc: Provide sysfs for efficient freq
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.
v2: Keep just one interface to toggle sysfs. With this, user will
be completely responsible for toggling efficient frequency if need
be. There will be no implicit disabling when user sets min < RP1 (Ashutosh)
v3: Fix compile error
Fixes: 95ccf312a1e4 ("drm/i915/guc/slpc: Allow SLPC to use efficient frequency")
Signed-off-by: Vinay Belgaumkar
Reviewed-by: Rodrigo Vivi
---
drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 35 +++
drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 43 ++-
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, 69 insertions(+), 11 deletions(-)
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, );
+ 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[] = {
_media_freq_factor.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, _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..5668f00f02e6 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(>lock);
+ wakeref = intel_runtime_pm_get(>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(>runtime_pm, wakeref);
+ mutex_unlock(>lock);
+ return ret;
+}
+
/**
*
