On Fri, Jan 17, 2025 at 10:21:53AM -0800, Belgaumkar, Vinay wrote:
>
> On 1/17/2025 6:29 AM, Rodrigo Vivi wrote:
> > On Thu, Jan 16, 2025 at 03:51:03PM -0800, Belgaumkar, Vinay wrote:
> > > On 1/16/2025 2:57 PM, Rodrigo Vivi wrote:
> > > > On Fri, Jan 10, 2025 at 03:21:51PM -0800, Vinay Belgaumkar wrote:
> > > > > Default SLPC power profile is Base(0). Power Saving mode(1)
> > > > > has conservative up/down thresholds and is suitable for use with
> > > > > apps that typically need to be power efficient.
> > > > >
> > > > > Selected power profile will be displayed in this format-
> > > > >
> > > > > $ cat slpc_power_profile
> > > > >
> > > > > [base] power_saving
> > > > >
> > > > > $ echo power_saving > slpc_power_profile
> > > > > $ cat slpc_power_profile
> > > > >
> > > > > base [power_saving]
> > > > >
> > > > > v2: Disable waitboost in power saving profile and updated sysfs
> > > > > format and add some kernel doc for SLPC (Rodrigo)
> > > > >
> > > > > Cc: Sushma Venkatesh Reddy <sushma.venkatesh.reddy@xxxxxxxxx>
> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > > > > Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@xxxxxxxxx>
> > > > > ---
> > > > > drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 47 +++++++++++++++
> > > > > drivers/gpu/drm/i915/gt/intel_rps.c | 4 ++
> > > > > .../drm/i915/gt/uc/abi/guc_actions_slpc_abi.h | 5 ++
> > > > > drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 60 +++++++++++++++++++
> > > > > drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h | 1 +
> > > > > .../gpu/drm/i915/gt/uc/intel_guc_slpc_types.h | 3 +
> > > > > 6 files changed, 120 insertions(+)
> > > > >
> > > > > 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 d7784650e4d9..83a7cc7dfbc8 100644
> > > > > --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
> > > > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
> > > > > @@ -464,6 +464,45 @@ static ssize_t slpc_ignore_eff_freq_store(struct kobject *kobj,
> > > > > return err ?: count;
> > > > > }
> > > > > +static ssize_t slpc_power_profile_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;
> > > > > +
> > > > > + switch (slpc->power_profile) {
> > > > > + case SLPC_POWER_PROFILES_BASE:
> > > > > + return sysfs_emit(buff, "[%s] %s\n", "base", "power_saving");
> > > > > + case SLPC_POWER_PROFILES_POWER_SAVING:
> > > > > + return sysfs_emit(buff, "%s [%s]\n", "base", "power_saving");
> > > > I had thought about something generic like kernel/power/main.c, but that is
> > > > indeed not needed since we do only have 2 options. This came out cleaner
> > > > than I though, although not generic...
> > > >
> > > > > + }
> > > > > +
> > > > > + return sysfs_emit(buff, "%u\n", slpc->power_profile);
> > > > > +}
> > > > > +
> > > > > +static ssize_t slpc_power_profile_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;
> > > > > + char power_saving[] = "power_saving";
> > > > > + char base[] = "base";
> > > > > + int err;
> > > > > + u32 val;
> > > > > +
> > > > > + if (!strncmp(buff, power_saving, sizeof(power_saving) - 1))
> > > > > + val = SLPC_POWER_PROFILES_POWER_SAVING;
> > > > > + else if (!strncmp(buff, base, sizeof(base) - 1))
> > > > > + val = SLPC_POWER_PROFILES_BASE;
> > > > > + else
> > > > > + return -EINVAL;
> > > > > +
> > > > > + err = intel_guc_slpc_set_power_profile(slpc, val);
> > > > > + return err ?: count;
> > > > > +}
> > > > > +
> > > > > struct intel_gt_bool_throttle_attr {
> > > > > struct attribute attr;
> > > > > ssize_t (*show)(struct kobject *kobj, struct kobj_attribute *attr,
> > > > > @@ -668,6 +707,7 @@ INTEL_GT_ATTR_RO(media_RP0_freq_mhz);
> > > > > INTEL_GT_ATTR_RO(media_RPn_freq_mhz);
> > > > > INTEL_GT_ATTR_RW(slpc_ignore_eff_freq);
> > > > > +INTEL_GT_ATTR_RW(slpc_power_profile);
> > > > > static const struct attribute *media_perf_power_attrs[] = {
> > > > > &attr_media_freq_factor.attr,
> > > > > @@ -864,6 +904,13 @@ void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj)
> > > > > gt_warn(gt, "failed to create ignore_eff_freq sysfs (%pe)", ERR_PTR(ret));
> > > > > }
> > > > > + if (intel_uc_uses_guc_slpc(>->uc)) {
> > > > > + ret = sysfs_create_file(kobj, &attr_slpc_power_profile.attr);
> > > > > + if (ret)
> > > > > + gt_warn(gt, "failed to create slpc_power_profile 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/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c
> > > > > index fa304ea088e4..2cfaedb04876 100644
> > > > > --- a/drivers/gpu/drm/i915/gt/intel_rps.c
> > > > > +++ b/drivers/gpu/drm/i915/gt/intel_rps.c
> > > > > @@ -1025,6 +1025,10 @@ void intel_rps_boost(struct i915_request *rq)
> > > > > if (rps_uses_slpc(rps)) {
> > > > > slpc = rps_to_slpc(rps);
> > > > > + /* Waitboost should not be done with power saving profile */
> > > > > + if (slpc->power_profile == SLPC_POWER_PROFILES_POWER_SAVING)
> > > > > + return;
> > > > hmmm... I'm afraid this is not enough... Although I just noticed that we
> > > > still have a problem for the low context strategy.
> > > >
> > > > Please notice the intel_display_rps_boost_after_vblank...
> > > boost_after_vblank() also ends up calling intel_rps_boost(), so it will skip
> > > correctly whenever the power saving profile is being used. The only extra
> > > thing is an additional work queue addition, I guess. We could avoid that.
> > hmm, that is better than I thought then... although it is probably good to
> > ensure we don't add an extra queue...
> > But also, shouldn't we ensure that the boost counter goes immediatelly to zero
> > and that we really immediatelly stop request the boost freq when we set this
> > mode? or that is too fast that we shouldn't bother?
>
> There are 2 workqueues at play here - one from intel_display_rps_boost() and
> one where we place boost requests in a queue on the rps side. We check for
> slpc level criteria(power profile, current min etc.) as well as context
> level ones (low-latency), we could split the slpc level ones out into
> another function. It is better to keep all the context related ones in the
> same intel_rps_boost() function, I think.
>
> I don't think we should set the boost counter to 0. That is per context, so
> could be needed for something that is in-flight.
Fair enough! Let's just add the doc then...
>
> Thanks,
>
> Vinay.
>
> >
> > > > So we probably need something like these:
> > > > https://github.com/rodrigovivi/linux/commit/42e24a146239a1b950ed047f619f334f5205ae8a
> > > >
> > > > other than that I believe this is good, thanks for adding this
> > > >
> > > > > +
> > > > > if (slpc->min_freq_softlimit >= slpc->boost_freq)
> > > > > return;
> > > > > diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_slpc_abi.h b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_slpc_abi.h
> > > > > index c34674e797c6..6de87ae5669e 100644
> > > > > --- a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_slpc_abi.h
> > > > > +++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_slpc_abi.h
> > > > > @@ -228,6 +228,11 @@ struct slpc_optimized_strategies {
> > > > > #define SLPC_OPTIMIZED_STRATEGY_COMPUTE REG_BIT(0)
> > > > > +enum slpc_power_profiles {
> > > > > + SLPC_POWER_PROFILES_BASE = 0x0,
> > > > > + SLPC_POWER_PROFILES_POWER_SAVING = 0x1
> > > > > +};
> > > > > +
> > > > > /**
> > > > > * DOC: SLPC H2G MESSAGE FORMAT
> > > > > *
> > > > > 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 706fffca698b..bee78467d4a3 100644
> > > > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
> > > > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
> > > > > @@ -15,6 +15,29 @@
> > > > > #include "gt/intel_gt_regs.h"
> > > > > #include "gt/intel_rps.h"
> > > > > +/**
> > > > > + * DOC: SLPC - Dynamic Frequency management
> > > > > + *
> > > > > + * Single Loop Power Control is a GuC based algorithm which manages
> > > > > + * GT frequency based on how KMD initializes its parameters. SLPC is
> > > > > + * almost completely in control after initialization except for the
> > > > > + * waitboost scenario.
> > > > > + *
> > > > > + * KMD uses concept of waitboost to ramp frequency up to RP0 when
> > > > > + * there are pending submissions. The addition of power profiles adds
> > > > > + * another level of control to these mechanisms. When we choose the power
> > > > > + * saving profile, SLPC will use conservative thresholds to ramp frequency,
> > > > > + * thus saving power. KMD will disable waitboosts when this happens to aid
> > > > > + * further power savings. The user has some level of control through sysfs
> > > > > + * where min/max frequencies can be altered and the use of efficient freq
> > > > > + * can be modified as well.
> > > > > + *
> > > > > + * Another form of frequency control happens through per context hints.
> > > > > + * A context can be marked as low latency during creation. That will ensure
> > > > > + * that SLPC uses an aggressive frequency ramp when that context is active.
> > > > > + *
> > > > Thanks for adding the doc!
> > > > but now I'm missing the documentation of these new profile strategies in here...
> > > ok, will call it out specifically.
> > >
> > > Thanks,
> > >
> > > Vinay.
> > >
> > > > > + */
> > > > > +
> > > > > static inline struct intel_guc *slpc_to_guc(struct intel_guc_slpc *slpc)
> > > > > {
> > > > > return container_of(slpc, struct intel_guc, slpc);
> > > > > @@ -265,6 +288,8 @@ int intel_guc_slpc_init(struct intel_guc_slpc *slpc)
> > > > > slpc->num_boosts = 0;
> > > > > slpc->media_ratio_mode = SLPC_MEDIA_RATIO_MODE_DYNAMIC_CONTROL;
> > > > > + slpc->power_profile = SLPC_POWER_PROFILES_BASE;
> > > > > +
> > > > > mutex_init(&slpc->lock);
> > > > > INIT_WORK(&slpc->boost_work, slpc_boost_work);
> > > > > @@ -567,6 +592,34 @@ int intel_guc_slpc_set_media_ratio_mode(struct intel_guc_slpc *slpc, u32 val)
> > > > > return ret;
> > > > > }
> > > > > +int intel_guc_slpc_set_power_profile(struct intel_guc_slpc *slpc, u32 val)
> > > > > +{
> > > > > + struct drm_i915_private *i915 = slpc_to_i915(slpc);
> > > > > + intel_wakeref_t wakeref;
> > > > > + int ret = 0;
> > > > > +
> > > > > + if (val > SLPC_POWER_PROFILES_POWER_SAVING)
> > > > > + return -EINVAL;
> > > > > +
> > > > > + mutex_lock(&slpc->lock);
> > > > > + wakeref = intel_runtime_pm_get(&i915->runtime_pm);
> > > > > +
> > > > > + ret = slpc_set_param(slpc,
> > > > > + SLPC_PARAM_POWER_PROFILE,
> > > > > + val);
> > > > > + if (ret)
> > > > > + guc_err(slpc_to_guc(slpc),
> > > > > + "Failed to set power profile to %d: %pe\n",
> > > > > + val, ERR_PTR(ret));
> > > > > + else
> > > > > + slpc->power_profile = val;
> > > > > +
> > > > > + intel_runtime_pm_put(&i915->runtime_pm, wakeref);
> > > > > + mutex_unlock(&slpc->lock);
> > > > > +
> > > > > + return ret;
> > > > > +}
> > > > > +
> > > > > void intel_guc_pm_intrmsk_enable(struct intel_gt *gt)
> > > > > {
> > > > > u32 pm_intrmsk_mbz = 0;
> > > > > @@ -728,6 +781,13 @@ int intel_guc_slpc_enable(struct intel_guc_slpc *slpc)
> > > > > /* Enable SLPC Optimized Strategy for compute */
> > > > > intel_guc_slpc_set_strategy(slpc, SLPC_OPTIMIZED_STRATEGY_COMPUTE);
> > > > > + /* Set cached value of power_profile */
> > > > > + ret = intel_guc_slpc_set_power_profile(slpc, slpc->power_profile);
> > > > > + if (unlikely(ret)) {
> > > > > + guc_probe_error(guc, "Failed to set SLPC power profile: %pe\n", ERR_PTR(ret));
> > > > > + return ret;
> > > > > + }
> > > > > +
> > > > > 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 1cb5fd44f05c..fc9f761b4372 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_set_ignore_eff_freq(struct intel_guc_slpc *slpc, bool val);
> > > > > int intel_guc_slpc_set_strategy(struct intel_guc_slpc *slpc, u32 val);
> > > > > +int intel_guc_slpc_set_power_profile(struct intel_guc_slpc *slpc, u32 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 a88651331497..83673b10ac4e 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
> > > > > @@ -33,6 +33,9 @@ struct intel_guc_slpc {
> > > > > u32 max_freq_softlimit;
> > > > > bool ignore_eff_freq;
> > > > > + /* Base or power saving */
> > > > > + u32 power_profile;
> > > > > +
> > > > > /* cached media ratio mode */
> > > > > u32 media_ratio_mode;
> > > > > --
> > > > > 2.38.1
> > > > >
