| Message ID | e4c5f650a41fa7955c3ddabbb32846b3fafb3134.1651261886.git.ashutosh.dixit@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Media freq factor and per-gt enhancements/fixes | expand |
On 29/04/2022 20:56, Ashutosh Dixit wrote: > Create a gt/gtN/.defaults directory (similar to > engine/<engine-name>/.defaults) to expose default parameter values for each > gt in sysfs. Populate the .defaults directory with RPS parameter default > values in order to allow userspace to revert to default values when needed. > > This patch adds the following sysfs files to gt/gtN/.defaults: > * default_min_freq_mhz > * default_max_freq_mhz > * default_boost_freq_mhz Possibly an uninformed question - max will not be the existing rp0, min rpN, and boost I don't know? > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_gt_sysfs.c | 10 ++-- > drivers/gpu/drm/i915/gt/intel_gt_sysfs.h | 6 +++ > drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 51 +++++++++++++++++++++ > drivers/gpu/drm/i915/gt/intel_gt_types.h | 10 ++++ > drivers/gpu/drm/i915/gt/intel_rps.c | 3 ++ > drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 17 +++++-- > 6 files changed, 87 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c > index 9e4ebf53379b..d651ccd0ab20 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c > +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c > @@ -22,11 +22,6 @@ bool is_object_gt(struct kobject *kobj) > return !strncmp(kobj->name, "gt", 2); > } > > -static struct intel_gt *kobj_to_gt(struct kobject *kobj) > -{ > - return container_of(kobj, struct intel_gt, sysfs_gt); > -} > - > struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev, > const char *name) > { > @@ -101,6 +96,10 @@ void intel_gt_sysfs_register(struct intel_gt *gt) > gt->i915->sysfs_gt, "gt%d", gt->info.id)) > goto exit_fail; > > + gt->sysfs_defaults = kobject_create_and_add(".defaults", >->sysfs_gt); > + if (!gt->sysfs_defaults) > + goto exit_fail; > + > intel_gt_sysfs_pm_init(gt, >->sysfs_gt); > > return; > @@ -113,5 +112,6 @@ void intel_gt_sysfs_register(struct intel_gt *gt) > > void intel_gt_sysfs_unregister(struct intel_gt *gt) > { > + kobject_put(gt->sysfs_defaults); Is this needed - won't below clean it up? And not sure I am liking the mix of embedded and allocated kobjects.. Why we couldn't have it uniform? > kobject_put(>->sysfs_gt); > } > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h > index a99aa7e8b01a..6232923a420d 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h > +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h > @@ -10,6 +10,7 @@ > #include <linux/kobject.h> > > #include "i915_gem.h" /* GEM_BUG_ON() */ > +#include "intel_gt_types.h" > > struct intel_gt; > > @@ -22,6 +23,11 @@ intel_gt_create_kobj(struct intel_gt *gt, > struct kobject *dir, > const char *name); > > +static inline struct intel_gt *kobj_to_gt(struct kobject *kobj) > +{ > + return container_of(kobj, struct intel_gt, sysfs_gt); > +} > + > void intel_gt_sysfs_register(struct intel_gt *gt); > void intel_gt_sysfs_unregister(struct intel_gt *gt); > struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev, > 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 ab91e9cf9deb..5a191973322e 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c > +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c > @@ -726,6 +726,51 @@ static const struct attribute *media_perf_power_attrs[] = { > NULL > }; > > +static ssize_t > +default_min_freq_mhz_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf) > +{ > + struct intel_gt *gt = kobj_to_gt(kobj->parent); > + > + return sysfs_emit(buf, "%d\n", gt->rps_defaults.min_freq); > +} > + > +static struct kobj_attribute default_min_freq_mhz = > +__ATTR(rps_min_freq_mhz, 0444, default_min_freq_mhz_show, NULL); > + > +static ssize_t > +default_max_freq_mhz_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf) > +{ > + struct intel_gt *gt = kobj_to_gt(kobj->parent); > + > + return sysfs_emit(buf, "%d\n", gt->rps_defaults.max_freq); > +} > + > +static struct kobj_attribute default_max_freq_mhz = > +__ATTR(rps_max_freq_mhz, 0444, default_max_freq_mhz_show, NULL); > + > +static ssize_t > +default_boost_freq_mhz_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf) > +{ > + struct intel_gt *gt = kobj_to_gt(kobj->parent); > + > + return sysfs_emit(buf, "%d\n", gt->rps_defaults.boost_freq); > +} > + > +static struct kobj_attribute default_boost_freq_mhz = > +__ATTR(rps_boost_freq_mhz, 0444, default_boost_freq_mhz_show, NULL); > + > +static const struct attribute * const rps_defaults_attrs[] = { > + &default_min_freq_mhz.attr, > + &default_max_freq_mhz.attr, > + &default_boost_freq_mhz.attr, > + NULL > +}; > + > +static int add_rps_defaults(struct intel_gt *gt) > +{ > + return sysfs_create_files(gt->sysfs_defaults, rps_defaults_attrs); > +} > + > static int intel_sysfs_rps_init(struct intel_gt *gt, struct kobject *kobj, > const struct attribute * const *attrs) > { > @@ -775,4 +820,10 @@ void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj) > "failed to create add gt%u media_perf_power_attrs sysfs (%pe)\n", > gt->info.id, ERR_PTR(ret)); > } > + > + ret = add_rps_defaults(gt); > + if (ret) > + drm_warn(>->i915->drm, > + "failed to add gt%u rps defaults (%pe)\n", > + gt->info.id, ERR_PTR(ret)); > } > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h > index edd7a3cf5f5f..8b696669b846 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h > +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h > @@ -62,6 +62,12 @@ enum intel_steering_type { > NUM_STEERING_TYPES > }; > > +struct intel_rps_defaults { > + u32 min_freq; > + u32 max_freq; > + u32 boost_freq; > +}; > + > enum intel_submission_method { > INTEL_SUBMISSION_RING, > INTEL_SUBMISSION_ELSP, > @@ -227,6 +233,10 @@ struct intel_gt { > > /* gt/gtN sysfs */ > struct kobject sysfs_gt; > + > + /* sysfs defaults per gt */ > + struct intel_rps_defaults rps_defaults; > + struct kobject *sysfs_defaults; > }; > > enum intel_gt_scratch_field { > diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c > index 6b68b40ebff0..6f2461e12409 100644 > --- a/drivers/gpu/drm/i915/gt/intel_rps.c > +++ b/drivers/gpu/drm/i915/gt/intel_rps.c > @@ -1976,7 +1976,9 @@ void intel_rps_init(struct intel_rps *rps) > > /* Derive initial user preferences/limits from the hardware limits */ > rps->max_freq_softlimit = rps->max_freq; > + rps_to_gt(rps)->rps_defaults.max_freq = rps->max_freq_softlimit; > rps->min_freq_softlimit = rps->min_freq; > + rps_to_gt(rps)->rps_defaults.min_freq = rps->min_freq_softlimit; > > /* After setting max-softlimit, find the overclock max freq */ > if (GRAPHICS_VER(i915) == 6 || IS_IVYBRIDGE(i915) || IS_HASWELL(i915)) { > @@ -1994,6 +1996,7 @@ void intel_rps_init(struct intel_rps *rps) > > /* Finally allow us to boost to max by default */ > rps->boost_freq = rps->max_freq; > + rps_to_gt(rps)->rps_defaults.boost_freq = rps->boost_freq; > rps->idle_freq = rps->min_freq; > > /* Start in the middle, from here we will autotune based on workload */ > 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 2df31af70d63..cefd864c84eb 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c > @@ -547,20 +547,24 @@ static int slpc_set_softlimits(struct intel_guc_slpc *slpc) > * unless they have deviated from defaults, in which case, > * we retain the values and set min/max accordingly. > */ > - if (!slpc->max_freq_softlimit) > + if (!slpc->max_freq_softlimit) { > slpc->max_freq_softlimit = slpc->rp0_freq; > - else if (slpc->max_freq_softlimit != slpc->rp0_freq) > + slpc_to_gt(slpc)->rps_defaults.max_freq = slpc->max_freq_softlimit; > + } else if (slpc->max_freq_softlimit != slpc->rp0_freq) { > ret = intel_guc_slpc_set_max_freq(slpc, > slpc->max_freq_softlimit); > + } > > if (unlikely(ret)) > return ret; > > - if (!slpc->min_freq_softlimit) > + if (!slpc->min_freq_softlimit) { > slpc->min_freq_softlimit = slpc->min_freq; > - else if (slpc->min_freq_softlimit != slpc->min_freq) > + slpc_to_gt(slpc)->rps_defaults.min_freq = slpc->min_freq_softlimit; > + } else if (slpc->min_freq_softlimit != slpc->min_freq) { > return intel_guc_slpc_set_min_freq(slpc, > slpc->min_freq_softlimit); > + } > > return 0; > } > @@ -606,8 +610,11 @@ static void slpc_get_rp_values(struct intel_guc_slpc *slpc) > slpc->rp1_freq = intel_gpu_freq(rps, caps.rp1_freq); > slpc->min_freq = intel_gpu_freq(rps, caps.min_freq); > > - if (!slpc->boost_freq) > + /* Boost freq is RP0, unless already set */ > + if (!slpc->boost_freq) { > slpc->boost_freq = slpc->rp0_freq; > + slpc_to_gt(slpc)->rps_defaults.boost_freq = slpc->boost_freq; > + } Not liking that there are two places which set each of the default. Is it that there are GuC and non-GuC paths which initialize the parent struct? Is there a way to set the defaults at one common place after either branch has run? Regards, Tvrtko > } > > /*
Hi Ashutosh, > > +static ssize_t > > +default_min_freq_mhz_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf) > > +{ > > + struct intel_gt *gt = kobj_to_gt(kobj->parent); > > + > > + return sysfs_emit(buf, "%d\n", gt->rps_defaults.min_freq); I guess this is %u. > > +} > > + > > +static struct kobj_attribute default_min_freq_mhz = > > +__ATTR(rps_min_freq_mhz, 0444, default_min_freq_mhz_show, NULL); > > + > > +static ssize_t > > +default_max_freq_mhz_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf) > > +{ > > + struct intel_gt *gt = kobj_to_gt(kobj->parent); > > + > > + return sysfs_emit(buf, "%d\n", gt->rps_defaults.max_freq); > > +} > > + > > +static struct kobj_attribute default_max_freq_mhz = > > +__ATTR(rps_max_freq_mhz, 0444, default_max_freq_mhz_show, NULL); > > + > > +static ssize_t > > +default_boost_freq_mhz_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf) > > +{ > > + struct intel_gt *gt = kobj_to_gt(kobj->parent); > > + > > + return sysfs_emit(buf, "%d\n", gt->rps_defaults.boost_freq); > > +} > > + > > +static struct kobj_attribute default_boost_freq_mhz = > > +__ATTR(rps_boost_freq_mhz, 0444, default_boost_freq_mhz_show, NULL); > > + > > +static const struct attribute * const rps_defaults_attrs[] = { > > + &default_min_freq_mhz.attr, > > + &default_max_freq_mhz.attr, > > + &default_boost_freq_mhz.attr, > > + NULL > > +}; Do you think this in the default group of kobj_gt_type like the gt_id? [...] > > +struct intel_rps_defaults { > > + u32 min_freq; > > + u32 max_freq; > > + u32 boost_freq; > > +}; > > + > > enum intel_submission_method { > > INTEL_SUBMISSION_RING, > > INTEL_SUBMISSION_ELSP, > > @@ -227,6 +233,10 @@ struct intel_gt { > > /* gt/gtN sysfs */ > > struct kobject sysfs_gt; > > + > > + /* sysfs defaults per gt */ > > + struct intel_rps_defaults rps_defaults; more of a matter of taste, but this looks natural to me to be in rps rather then in the gt. [...] Andi
On Tue, 10 May 2022 00:53:23 -0700, Tvrtko Ursulin wrote: > > On 29/04/2022 20:56, Ashutosh Dixit wrote: > > Create a gt/gtN/.defaults directory (similar to > > engine/<engine-name>/.defaults) to expose default parameter values for each > > gt in sysfs. Populate the .defaults directory with RPS parameter default > > values in order to allow userspace to revert to default values when needed. > > > > This patch adds the following sysfs files to gt/gtN/.defaults: > > * default_min_freq_mhz > > * default_max_freq_mhz > > * default_boost_freq_mhz > > Possibly an uninformed question - max will not be the existing rp0, min > rpN, and boost I don't know? This is actually the case at present (with boost also RP0). However, PVC also sets min to RP0 (instead of RPn). And then there is a completely different scheme being considered for GuC where min = max = boost = "-1" where -1 denotes a "default managed by firmware". For it is not clear why i915 should set these min/max in FW (which is autonomous and can better manage the freq control algorithm) especially when these controls are exposed to userspace in sysfs. This patch was written because user space was asking a control to restore all RPS parameters to their default values. Then Chris suggested doing the same for gt sysfs as what we do for the engine sysfs, expose default values in sysfs and have userspace restore them when needed. > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > drivers/gpu/drm/i915/gt/intel_gt_sysfs.c | 10 ++-- > > drivers/gpu/drm/i915/gt/intel_gt_sysfs.h | 6 +++ > > drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 51 +++++++++++++++++++++ > > drivers/gpu/drm/i915/gt/intel_gt_types.h | 10 ++++ > > drivers/gpu/drm/i915/gt/intel_rps.c | 3 ++ > > drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 17 +++++-- > > 6 files changed, 87 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c > > index 9e4ebf53379b..d651ccd0ab20 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c > > @@ -22,11 +22,6 @@ bool is_object_gt(struct kobject *kobj) > > return !strncmp(kobj->name, "gt", 2); > > } > > -static struct intel_gt *kobj_to_gt(struct kobject *kobj) > > -{ > > - return container_of(kobj, struct intel_gt, sysfs_gt); > > -} > > - > > struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev, > > const char *name) > > { > > @@ -101,6 +96,10 @@ void intel_gt_sysfs_register(struct intel_gt *gt) > > gt->i915->sysfs_gt, "gt%d", gt->info.id)) > > goto exit_fail; > > + gt->sysfs_defaults = kobject_create_and_add(".defaults", > > >->sysfs_gt); > > + if (!gt->sysfs_defaults) > > + goto exit_fail; > > + > > intel_gt_sysfs_pm_init(gt, >->sysfs_gt); > > return; > > @@ -113,5 +112,6 @@ void intel_gt_sysfs_register(struct intel_gt *gt) > > void intel_gt_sysfs_unregister(struct intel_gt *gt) > > { > > + kobject_put(gt->sysfs_defaults); > > Is this needed - won't below clean it up? Child kobject's take a reference on their parents so child kobjects need a kobject_put before the parent kobject_put to free the memory allocated for the parent (i.e. doing a kobject_put on the parent will not automatically free all the child kobjects). > > kobject_put(>->sysfs_gt); > > } > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h > > index a99aa7e8b01a..6232923a420d 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h > > @@ -10,6 +10,7 @@ > > #include <linux/kobject.h> > > #include "i915_gem.h" /* GEM_BUG_ON() */ > > +#include "intel_gt_types.h" > > struct intel_gt; > > @@ -22,6 +23,11 @@ intel_gt_create_kobj(struct intel_gt *gt, > > struct kobject *dir, > > const char *name); > > +static inline struct intel_gt *kobj_to_gt(struct kobject *kobj) > > +{ > > + return container_of(kobj, struct intel_gt, sysfs_gt); > > +} > > + > > void intel_gt_sysfs_register(struct intel_gt *gt); > > void intel_gt_sysfs_unregister(struct intel_gt *gt); > > struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev, > > 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 ab91e9cf9deb..5a191973322e 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c > > @@ -726,6 +726,51 @@ static const struct attribute *media_perf_power_attrs[] = { > > NULL > > }; > > +static ssize_t > > +default_min_freq_mhz_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf) > > +{ > > + struct intel_gt *gt = kobj_to_gt(kobj->parent); > > + > > + return sysfs_emit(buf, "%d\n", gt->rps_defaults.min_freq); > > +} > > + > > +static struct kobj_attribute default_min_freq_mhz = > > +__ATTR(rps_min_freq_mhz, 0444, default_min_freq_mhz_show, NULL); > > + > > +static ssize_t > > +default_max_freq_mhz_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf) > > +{ > > + struct intel_gt *gt = kobj_to_gt(kobj->parent); > > + > > + return sysfs_emit(buf, "%d\n", gt->rps_defaults.max_freq); > > +} > > + > > +static struct kobj_attribute default_max_freq_mhz = > > +__ATTR(rps_max_freq_mhz, 0444, default_max_freq_mhz_show, NULL); > > + > > +static ssize_t > > +default_boost_freq_mhz_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf) > > +{ > > + struct intel_gt *gt = kobj_to_gt(kobj->parent); > > + > > + return sysfs_emit(buf, "%d\n", gt->rps_defaults.boost_freq); > > +} > > + > > +static struct kobj_attribute default_boost_freq_mhz = > > +__ATTR(rps_boost_freq_mhz, 0444, default_boost_freq_mhz_show, NULL); > > + > > +static const struct attribute * const rps_defaults_attrs[] = { > > + &default_min_freq_mhz.attr, > > + &default_max_freq_mhz.attr, > > + &default_boost_freq_mhz.attr, > > + NULL > > +}; > > + > > +static int add_rps_defaults(struct intel_gt *gt) > > +{ > > + return sysfs_create_files(gt->sysfs_defaults, rps_defaults_attrs); > > +} > > + > > static int intel_sysfs_rps_init(struct intel_gt *gt, struct kobject *kobj, > > const struct attribute * const *attrs) > > { > > @@ -775,4 +820,10 @@ void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj) > > "failed to create add gt%u media_perf_power_attrs sysfs (%pe)\n", > > gt->info.id, ERR_PTR(ret)); > > } > > + > > + ret = add_rps_defaults(gt); > > + if (ret) > > + drm_warn(>->i915->drm, > > + "failed to add gt%u rps defaults (%pe)\n", > > + gt->info.id, ERR_PTR(ret)); > > } > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h > > index edd7a3cf5f5f..8b696669b846 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h > > @@ -62,6 +62,12 @@ enum intel_steering_type { > > NUM_STEERING_TYPES > > }; > > +struct intel_rps_defaults { > > + u32 min_freq; > > + u32 max_freq; > > + u32 boost_freq; > > +}; > > + > > enum intel_submission_method { > > INTEL_SUBMISSION_RING, > > INTEL_SUBMISSION_ELSP, > > @@ -227,6 +233,10 @@ struct intel_gt { > > /* gt/gtN sysfs */ > > struct kobject sysfs_gt; > > + > > + /* sysfs defaults per gt */ > > + struct intel_rps_defaults rps_defaults; > > + struct kobject *sysfs_defaults; > And not sure I am liking the mix of embedded and allocated kobjects.. Why > we couldn't have it uniform? We could use an embedded sysfs_defaults kobject but it is more code, the simplest code pattern is to use an allocated kobject (the kernel takes care of everything in this case). There were two reasons for the embedded sysfs_gt kobject: (a) it had default attributes (default_groups) and (b) embedded allows using container_of() to get to the gt (see kobj_to_gt()). Still talking to Andi about whether we need default attributes for sysfs_defaults too and if that happens we'll need to convert this to an embedded kobject too. But otherwise I prefer to leave this as is (and imo promoting multiple valid code patterns should be fine). > > }; > > enum intel_gt_scratch_field { > > diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c > > index 6b68b40ebff0..6f2461e12409 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_rps.c > > +++ b/drivers/gpu/drm/i915/gt/intel_rps.c > > @@ -1976,7 +1976,9 @@ void intel_rps_init(struct intel_rps *rps) > > /* Derive initial user preferences/limits from the hardware limits > > */ > > rps->max_freq_softlimit = rps->max_freq; > > + rps_to_gt(rps)->rps_defaults.max_freq = rps->max_freq_softlimit; > > rps->min_freq_softlimit = rps->min_freq; > > + rps_to_gt(rps)->rps_defaults.min_freq = rps->min_freq_softlimit; > > /* After setting max-softlimit, find the overclock max freq */ > > if (GRAPHICS_VER(i915) == 6 || IS_IVYBRIDGE(i915) || IS_HASWELL(i915)) { > > @@ -1994,6 +1996,7 @@ void intel_rps_init(struct intel_rps *rps) > > /* Finally allow us to boost to max by default */ > > rps->boost_freq = rps->max_freq; > > + rps_to_gt(rps)->rps_defaults.boost_freq = rps->boost_freq; > > rps->idle_freq = rps->min_freq; > > /* Start in the middle, from here we will autotune based on > > workload */ > > 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 2df31af70d63..cefd864c84eb 100644 > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c > > @@ -547,20 +547,24 @@ static int slpc_set_softlimits(struct intel_guc_slpc *slpc) > > * unless they have deviated from defaults, in which case, > > * we retain the values and set min/max accordingly. > > */ > > - if (!slpc->max_freq_softlimit) > > + if (!slpc->max_freq_softlimit) { > > slpc->max_freq_softlimit = slpc->rp0_freq; > > - else if (slpc->max_freq_softlimit != slpc->rp0_freq) > > + slpc_to_gt(slpc)->rps_defaults.max_freq = slpc->max_freq_softlimit; > > + } else if (slpc->max_freq_softlimit != slpc->rp0_freq) { > > ret = intel_guc_slpc_set_max_freq(slpc, > > slpc->max_freq_softlimit); > > + } > > if (unlikely(ret)) > > return ret; > > - if (!slpc->min_freq_softlimit) > > + if (!slpc->min_freq_softlimit) { > > slpc->min_freq_softlimit = slpc->min_freq; > > - else if (slpc->min_freq_softlimit != slpc->min_freq) > > + slpc_to_gt(slpc)->rps_defaults.min_freq = slpc->min_freq_softlimit; > > + } else if (slpc->min_freq_softlimit != slpc->min_freq) { > > return intel_guc_slpc_set_min_freq(slpc, > > slpc->min_freq_softlimit); > > + } > > return 0; > > } > > @@ -606,8 +610,11 @@ static void slpc_get_rp_values(struct intel_guc_slpc *slpc) > > slpc->rp1_freq = intel_gpu_freq(rps, caps.rp1_freq); > > slpc->min_freq = intel_gpu_freq(rps, caps.min_freq); > > - if (!slpc->boost_freq) > > + /* Boost freq is RP0, unless already set */ > > + if (!slpc->boost_freq) { > > slpc->boost_freq = slpc->rp0_freq; > > + slpc_to_gt(slpc)->rps_defaults.boost_freq = slpc->boost_freq; > > + } > > Not liking that there are two places which set each of the default. Is it > that there are GuC and non-GuC paths which initialize the parent struct? Is > there a way to set the defaults at one common place after either branch has > run? Basically the RPS (non-GuC) and SLPC (Guc) are independent algorithms with independent data structs. So I think we would need to set the default inside those RPS and SLPC "modules" (or files). My approach was to "capture" the defaults in places in the code where they are set (since the values sometimes change later even during the course of initialization). The issue with doing it this way is that we don't necessarily clearly see what those defaults are, they are just set at the right place in the code. So that is something we can debate about. So an alternative would be to e.g. set defaults in rps_set_defaults() and slpc_set_defaults() functions (which would use explicit knowledge of what those defaults are). So I would be open to this if you think that's a better way to go. I think these functions can just be called during the course of RPS or SLPC initialization so I don't really see the need for doing it from a common place. Thanks, Ashutosh PS: I have now sent out a new series for these patches, latest rev here: https://patchwork.freedesktop.org/series/104423/
On Tue, 10 May 2022 03:58:13 -0700, Andi Shyti wrote: > > Hi Ashutosh, Hi Andi, > > > +static ssize_t > > > +default_min_freq_mhz_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf) > > > +{ > > > + struct intel_gt *gt = kobj_to_gt(kobj->parent); > > > + > > > + return sysfs_emit(buf, "%d\n", gt->rps_defaults.min_freq); > > I guess this is %u. Fixed in v2. > > > +} > > > + > > > +static struct kobj_attribute default_min_freq_mhz = > > > +__ATTR(rps_min_freq_mhz, 0444, default_min_freq_mhz_show, NULL); > > > + > > > +static ssize_t > > > +default_max_freq_mhz_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf) > > > +{ > > > + struct intel_gt *gt = kobj_to_gt(kobj->parent); > > > + > > > + return sysfs_emit(buf, "%d\n", gt->rps_defaults.max_freq); > > > +} > > > + > > > +static struct kobj_attribute default_max_freq_mhz = > > > +__ATTR(rps_max_freq_mhz, 0444, default_max_freq_mhz_show, NULL); > > > + > > > +static ssize_t > > > +default_boost_freq_mhz_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf) > > > +{ > > > + struct intel_gt *gt = kobj_to_gt(kobj->parent); > > > + > > > + return sysfs_emit(buf, "%d\n", gt->rps_defaults.boost_freq); > > > +} > > > + > > > +static struct kobj_attribute default_boost_freq_mhz = > > > +__ATTR(rps_boost_freq_mhz, 0444, default_boost_freq_mhz_show, NULL); > > > + > > > +static const struct attribute * const rps_defaults_attrs[] = { > > > + &default_min_freq_mhz.attr, > > > + &default_max_freq_mhz.attr, > > > + &default_boost_freq_mhz.attr, > > > + NULL > > > +}; > > Do you think this in the default group of kobj_gt_type like the > gt_id? gt_id I think fits well in the category of a "default" attribute for a gt. I am not sure if these attributes similarly qualify as "default" attributes (for the .defaults sysfs), what do you think? That is why I have followed what is done in the engine sysfs which also does not use default attributes. But we can revisit based on your comments. > > [...] > > > > +struct intel_rps_defaults { > > > + u32 min_freq; > > > + u32 max_freq; > > > + u32 boost_freq; > > > +}; > > > + > > > enum intel_submission_method { > > > INTEL_SUBMISSION_RING, > > > INTEL_SUBMISSION_ELSP, > > > @@ -227,6 +233,10 @@ struct intel_gt { > > > /* gt/gtN sysfs */ > > > struct kobject sysfs_gt; > > > + > > > + /* sysfs defaults per gt */ > > > + struct intel_rps_defaults rps_defaults; > > more of a matter of taste, but this looks natural to me to be in > rps rather then in the gt. In v2 I have changed the name of this from 'struct intel_rps_defaults' to 'struct gt_defaults'. So the idea is to have a central place in the gt where any "module" (RPS, PM, ...) can store their defaults which are then exposed via sysfs files in gt/gtN/.defaults directory. There are multiple ways of doing this but this is what I've done in this series, basically to keep things simple. Thanks. -- Ashutosh
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c index 9e4ebf53379b..d651ccd0ab20 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c @@ -22,11 +22,6 @@ bool is_object_gt(struct kobject *kobj) return !strncmp(kobj->name, "gt", 2); } -static struct intel_gt *kobj_to_gt(struct kobject *kobj) -{ - return container_of(kobj, struct intel_gt, sysfs_gt); -} - struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev, const char *name) { @@ -101,6 +96,10 @@ void intel_gt_sysfs_register(struct intel_gt *gt) gt->i915->sysfs_gt, "gt%d", gt->info.id)) goto exit_fail; + gt->sysfs_defaults = kobject_create_and_add(".defaults", >->sysfs_gt); + if (!gt->sysfs_defaults) + goto exit_fail; + intel_gt_sysfs_pm_init(gt, >->sysfs_gt); return; @@ -113,5 +112,6 @@ void intel_gt_sysfs_register(struct intel_gt *gt) void intel_gt_sysfs_unregister(struct intel_gt *gt) { + kobject_put(gt->sysfs_defaults); kobject_put(>->sysfs_gt); } diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h index a99aa7e8b01a..6232923a420d 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h @@ -10,6 +10,7 @@ #include <linux/kobject.h> #include "i915_gem.h" /* GEM_BUG_ON() */ +#include "intel_gt_types.h" struct intel_gt; @@ -22,6 +23,11 @@ intel_gt_create_kobj(struct intel_gt *gt, struct kobject *dir, const char *name); +static inline struct intel_gt *kobj_to_gt(struct kobject *kobj) +{ + return container_of(kobj, struct intel_gt, sysfs_gt); +} + void intel_gt_sysfs_register(struct intel_gt *gt); void intel_gt_sysfs_unregister(struct intel_gt *gt); struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev, 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 ab91e9cf9deb..5a191973322e 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c @@ -726,6 +726,51 @@ static const struct attribute *media_perf_power_attrs[] = { NULL }; +static ssize_t +default_min_freq_mhz_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf) +{ + struct intel_gt *gt = kobj_to_gt(kobj->parent); + + return sysfs_emit(buf, "%d\n", gt->rps_defaults.min_freq); +} + +static struct kobj_attribute default_min_freq_mhz = +__ATTR(rps_min_freq_mhz, 0444, default_min_freq_mhz_show, NULL); + +static ssize_t +default_max_freq_mhz_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf) +{ + struct intel_gt *gt = kobj_to_gt(kobj->parent); + + return sysfs_emit(buf, "%d\n", gt->rps_defaults.max_freq); +} + +static struct kobj_attribute default_max_freq_mhz = +__ATTR(rps_max_freq_mhz, 0444, default_max_freq_mhz_show, NULL); + +static ssize_t +default_boost_freq_mhz_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf) +{ + struct intel_gt *gt = kobj_to_gt(kobj->parent); + + return sysfs_emit(buf, "%d\n", gt->rps_defaults.boost_freq); +} + +static struct kobj_attribute default_boost_freq_mhz = +__ATTR(rps_boost_freq_mhz, 0444, default_boost_freq_mhz_show, NULL); + +static const struct attribute * const rps_defaults_attrs[] = { + &default_min_freq_mhz.attr, + &default_max_freq_mhz.attr, + &default_boost_freq_mhz.attr, + NULL +}; + +static int add_rps_defaults(struct intel_gt *gt) +{ + return sysfs_create_files(gt->sysfs_defaults, rps_defaults_attrs); +} + static int intel_sysfs_rps_init(struct intel_gt *gt, struct kobject *kobj, const struct attribute * const *attrs) { @@ -775,4 +820,10 @@ void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj) "failed to create add gt%u media_perf_power_attrs sysfs (%pe)\n", gt->info.id, ERR_PTR(ret)); } + + ret = add_rps_defaults(gt); + if (ret) + drm_warn(>->i915->drm, + "failed to add gt%u rps defaults (%pe)\n", + gt->info.id, ERR_PTR(ret)); } diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h index edd7a3cf5f5f..8b696669b846 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h @@ -62,6 +62,12 @@ enum intel_steering_type { NUM_STEERING_TYPES }; +struct intel_rps_defaults { + u32 min_freq; + u32 max_freq; + u32 boost_freq; +}; + enum intel_submission_method { INTEL_SUBMISSION_RING, INTEL_SUBMISSION_ELSP, @@ -227,6 +233,10 @@ struct intel_gt { /* gt/gtN sysfs */ struct kobject sysfs_gt; + + /* sysfs defaults per gt */ + struct intel_rps_defaults rps_defaults; + struct kobject *sysfs_defaults; }; enum intel_gt_scratch_field { diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c index 6b68b40ebff0..6f2461e12409 100644 --- a/drivers/gpu/drm/i915/gt/intel_rps.c +++ b/drivers/gpu/drm/i915/gt/intel_rps.c @@ -1976,7 +1976,9 @@ void intel_rps_init(struct intel_rps *rps) /* Derive initial user preferences/limits from the hardware limits */ rps->max_freq_softlimit = rps->max_freq; + rps_to_gt(rps)->rps_defaults.max_freq = rps->max_freq_softlimit; rps->min_freq_softlimit = rps->min_freq; + rps_to_gt(rps)->rps_defaults.min_freq = rps->min_freq_softlimit; /* After setting max-softlimit, find the overclock max freq */ if (GRAPHICS_VER(i915) == 6 || IS_IVYBRIDGE(i915) || IS_HASWELL(i915)) { @@ -1994,6 +1996,7 @@ void intel_rps_init(struct intel_rps *rps) /* Finally allow us to boost to max by default */ rps->boost_freq = rps->max_freq; + rps_to_gt(rps)->rps_defaults.boost_freq = rps->boost_freq; rps->idle_freq = rps->min_freq; /* Start in the middle, from here we will autotune based on workload */ 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 2df31af70d63..cefd864c84eb 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c @@ -547,20 +547,24 @@ static int slpc_set_softlimits(struct intel_guc_slpc *slpc) * unless they have deviated from defaults, in which case, * we retain the values and set min/max accordingly. */ - if (!slpc->max_freq_softlimit) + if (!slpc->max_freq_softlimit) { slpc->max_freq_softlimit = slpc->rp0_freq; - else if (slpc->max_freq_softlimit != slpc->rp0_freq) + slpc_to_gt(slpc)->rps_defaults.max_freq = slpc->max_freq_softlimit; + } else if (slpc->max_freq_softlimit != slpc->rp0_freq) { ret = intel_guc_slpc_set_max_freq(slpc, slpc->max_freq_softlimit); + } if (unlikely(ret)) return ret; - if (!slpc->min_freq_softlimit) + if (!slpc->min_freq_softlimit) { slpc->min_freq_softlimit = slpc->min_freq; - else if (slpc->min_freq_softlimit != slpc->min_freq) + slpc_to_gt(slpc)->rps_defaults.min_freq = slpc->min_freq_softlimit; + } else if (slpc->min_freq_softlimit != slpc->min_freq) { return intel_guc_slpc_set_min_freq(slpc, slpc->min_freq_softlimit); + } return 0; } @@ -606,8 +610,11 @@ static void slpc_get_rp_values(struct intel_guc_slpc *slpc) slpc->rp1_freq = intel_gpu_freq(rps, caps.rp1_freq); slpc->min_freq = intel_gpu_freq(rps, caps.min_freq); - if (!slpc->boost_freq) + /* Boost freq is RP0, unless already set */ + if (!slpc->boost_freq) { slpc->boost_freq = slpc->rp0_freq; + slpc_to_gt(slpc)->rps_defaults.boost_freq = slpc->boost_freq; + } } /*