Re: [Intel-gfx] [PATCH 7/8] drm/i915/gt: Expose per-gt RPS defaults in sysfs

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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", &gt->sysfs_gt);
> +	if (!gt->sysfs_defaults)
> +		goto exit_fail;
> +
>   	intel_gt_sysfs_pm_init(gt, &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(&gt->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(&gt->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

>   }
>   
>   /*