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

Tvrtko Ursulin
2022-05-05T13:14:37Z

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@linux.intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
> ---
> 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