Re: [Intel-gfx] [PATCH v7 6/7] drm/i915/gt: Create per-tile RPS sysfs interfaces

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

Hi Andi,

On 18/03/2022 23:39, Andi Shyti wrote:
> Now tiles have their own sysfs interfaces under the gt/
> directory. Because RPS is a property that can be configured on a
> tile basis, then each tile should have its own interface
>
> The new sysfs structure will have a similar layout for the 4 tile
> case:
>
> /sys/.../card0
>           ├── gt
>           │   ├── gt0
>           │   │   ├── id
>           │   │   ├── rc6_enable
>           │   │   ├── rc6_residency_ms
>           │   │   ├── rps_act_freq_mhz
>           │   │   ├── rps_boost_freq_mhz
>           │   │   ├── rps_cur_freq_mhz
>           │   │   ├── rps_max_freq_mhz
>           │   │   ├── rps_min_freq_mhz
>           │   │   ├── rps_RP0_freq_mhz
>           │   │   ├── rps_RP1_freq_mhz
>           │   │   └── rps_RPn_freq_mhz
>           .   .
>           .   .
>           .   .
>           │   └── gtN
>           │       ├── id
>           │       ├── rc6_enable
>           │       ├── rc6_residency_ms
>           │       ├── rps_act_freq_mhz
>           │       ├── rps_boost_freq_mhz
>           │       ├── rps_cur_freq_mhz
>           │       ├── rps_max_freq_mhz
>           │       ├── rps_min_freq_mhz
>           │       ├── rps_RP0_freq_mhz
>           │       ├── rps_RP1_freq_mhz
>           │       └── rps_RPn_freq_mhz
>           ├── gt_act_freq_mhz   -+
>           ├── gt_boost_freq_mhz  |
>           ├── gt_cur_freq_mhz    |    Original interface
>           ├── gt_max_freq_mhz    +─-> kept as existing ABI;
>           ├── gt_min_freq_mhz    |    it points to gt0/
>           ├── gt_RP0_freq_mhz    |
>           ├── gt_RP1_freq_mhz    |
>           └── gt_RPn_freq_mhz   -+
>
> The existing interfaces have been kept in their original location
> to preserve the existing ABI. They act on all the GTs: when
> writing they loop through all the GTs and write the information
> on each interface. When reading they provide the average value
> from all the GTs.
>
> This patch is not really adding exposing new interfaces (new
> ABI) other than adapting the existing one to more tiles. In any
> case this new set of interfaces will be a basic tool for system
> managers and administrators when using i915.
>
> Signed-off-by: Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> Cc: Sujaritha Sundaresan <sujaritha.sundaresan@xxxxxxxxx>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> Reviewed-by: Andrzej Hajda <andrzej.hajda@xxxxxxxxx>
> ---
>   drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 283 ++++++++++++++++++++
>   drivers/gpu/drm/i915/i915_sysfs.c           | 177 ------------
>   2 files changed, 283 insertions(+), 177 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 144b004e4de82..b0a1ea95d028e 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
> @@ -14,6 +14,7 @@
>   #include "intel_gt_sysfs.h"
>   #include "intel_gt_sysfs_pm.h"
>   #include "intel_rc6.h"
> +#include "intel_rps.h"
>   
>   #ifdef CONFIG_PM
>   enum intel_gt_sysfs_op {
> @@ -21,6 +22,30 @@ enum intel_gt_sysfs_op {
>   	INTEL_GT_SYSFS_MAX,
>   };
>   
> +static int
> +sysfs_gt_attribute_w_func(struct device *dev, struct device_attribute *attr,
> +			  int (func)(struct intel_gt *gt, u32 val), u32 val)
> +{
> +	struct intel_gt *gt;
> +	int ret;
> +
> +	if (!is_object_gt(&dev->kobj)) {
> +		int i;
> +		struct drm_i915_private *i915 = kdev_minor_to_i915(dev);
> +
> +		for_each_gt(gt, i915, i) {
> +			ret = func(gt, val);
> +			if (ret)
> +				break;
> +		}
> +	} else {
> +		gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
> +		ret = func(gt, val);
> +	}
> +
> +	return ret;
> +}
> +
>   static u32
>   sysfs_gt_attribute_r_func(struct device *dev, struct device_attribute *attr,
>   			  u32 (func)(struct intel_gt *gt),
> @@ -62,6 +87,7 @@ sysfs_gt_attribute_r_func(struct device *dev, struct device_attribute *attr,
>   #define sysfs_gt_attribute_r_min_func(d, a, f) \
>   		sysfs_gt_attribute_r_func(d, a, f, INTEL_GT_SYSFS_MIN)
>   
> +/* Frequency interfaces will show the maximum frequency value */
>   #define sysfs_gt_attribute_r_max_func(d, a, f) \
>   		sysfs_gt_attribute_r_func(d, a, f, INTEL_GT_SYSFS_MAX)
>   
> @@ -238,7 +264,264 @@ static void intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj)
>   }
>   #endif /* CONFIG_PM */
>   
> +static u32 __act_freq_mhz_show(struct intel_gt *gt)
> +{
> +	return intel_rps_read_actual_frequency(&gt->rps);
> +}
> +
> +static ssize_t act_freq_mhz_show(struct device *dev,
> +				 struct device_attribute *attr, char *buff)
> +{
> +	u32 actual_freq = sysfs_gt_attribute_r_max_func(dev, attr,
> +						    __act_freq_mhz_show);

Because sysfs_gt_attribute_r_max_func is only defined if CONFIG_PM is 
set, the !CONFIG_PM builds are broken. I guess just move it to be always 
available.

Regards,

Tvrtko