Re: [PATCH v1] drm/i915/gt: Add sysfs RAPL PL1 interface

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 03 Nov 2022 05:37:23 -0700, Sujaritha Sundaresan wrote:
>

Hi Suja,

> Adding the rapl_pl1_freq_mhz sysfs attribute.
>
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@xxxxxxxxx>
> Cc: Ashutosh Dixit <ashutosh.dixit@xxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 20 ++++++++++
>  drivers/gpu/drm/i915/gt/intel_rps.c         | 44 +++++++++++++++++++++
>  drivers/gpu/drm/i915/gt/intel_rps.h         |  3 ++
>  drivers/gpu/drm/i915/i915_reg.h             |  4 ++
>  4 files changed, 71 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 904160952369..e7f00ec252f8 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
> @@ -496,6 +496,17 @@ static DEVICE_ATTR_RO(vlv_rpe_freq_mhz);
>  static const struct attribute * const gen6_rps_attrs[] = GEN6_RPS_ATTR;
>  static const struct attribute * const gen6_gt_attrs[]  = GEN6_GT_ATTR;
>
> +static ssize_t rapl_pl1_freq_mhz_show(struct device *dev,
> +				      struct device_attribute *attr,
> +				      char *buff)
> +{
> +	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
> +	u32 rapl_pl1 = intel_rps_read_rapl_pl1_frequency(&gt->rps);
> +
> +	return sysfs_emit(buff, "%u\n", rapl_pl1);
> +}
> +
> +
>  static ssize_t punit_req_freq_mhz_show(struct device *dev,
>				       struct device_attribute *attr,
>				       char *buff)
> @@ -534,6 +545,7 @@ struct intel_gt_bool_throttle_attr attr_##sysfs_func__ = { \
>	.mask = mask__, \
>  }
>
> +static DEVICE_ATTR_RO(rapl_pl1_freq_mhz);
>  static DEVICE_ATTR_RO(punit_req_freq_mhz);

Is this patch against old code? Since this is now INTEL_GT_ATTR_RO. Yes the
build failed. So rapl_pl1_freq_mhz will need to follow punit_req_freq_mhz.

>  static INTEL_GT_RPS_BOOL_ATTR_RO(throttle_reason_status, GT0_PERF_LIMIT_REASONS_MASK);
>  static INTEL_GT_RPS_BOOL_ATTR_RO(throttle_reason_pl1, POWER_LIMIT_1_MASK);
> @@ -790,12 +802,20 @@ void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj)
>	if (!is_object_gt(kobj))
>		return;
>
> +	ret = sysfs_create_file(kobj, &dev_attr_rapl_pl1_freq_mhz.attr);

The convention here is to create sysfs files only for platforms on which a
feature (in this case RAPL PL1 freq) is supported.

Also are we sure this is only available on MTL and XEHPSDV and not on DG2?
Since generally a feature appears first on a platform and then is available
for all successive products. If it's available on DG2 too then we can use
something like:

	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50))

See GRAPHICS_VER_FULL for various platforms in i915_pci.c.

> +	if (ret)
> +		drm_warn(&gt->i915->drm,
> +			"failed to create gt%u rapl_pl1_freq_mhz sysfs(%pe)",
> +			gt->info.id, ERR_PTR(ret));
> +
> +
>	ret = sysfs_create_file(kobj, &dev_attr_punit_req_freq_mhz.attr);
>	if (ret)
>		drm_warn(&gt->i915->drm,
>			 "failed to create gt%u punit_req_freq_mhz sysfs (%pe)",
>			 gt->info.id, ERR_PTR(ret));
>
> +

Remove empty line.

>	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 17b40b625e31..0e89b941e3be 100644
> --- a/drivers/gpu/drm/i915/gt/intel_rps.c
> +++ b/drivers/gpu/drm/i915/gt/intel_rps.c
> @@ -9,6 +9,7 @@
>
>  #include "i915_drv.h"
>  #include "i915_irq.h"
> +#include "i915_reg.h"

Not needed, see below.

>  #include "intel_breadcrumbs.h"
>  #include "intel_gt.h"
>  #include "intel_gt_clock_utils.h"
> @@ -2422,6 +2423,49 @@ bool rps_read_mask_mmio(struct intel_rps *rps,
>	return rps_read_mmio(rps, reg32) & mask;
>  }
>
> +u32 intel_rps_read_rapl_pl1(struct intel_rps *rps)
> +{
> +	struct drm_i915_private *i915 = rps_to_i915(rps);
> +	i915_reg_t rgadr;
> +	u32 rapl_pl1;
> +
> +	if (IS_METEORLAKE(i915)) {
> +		rgadr = MTL_RAPL_PL1_FREQ_LIMIT;
> +	} else if (IS_XEHPSDV(i915)) {
> +		rgadr = XEHPSDV_RAPL_PL1_FREQ_LIMIT;
> +	} else {
> +		MISSING_CASE(GRAPHICS_VER(i915));
> +		rgadr = INVALID_MMIO_REG;

No need for this, the sysfs file will only be visible for platforms on
which this is supported so this will never be hit.

> +	}
> +
> +	if (!i915_mmio_reg_valid(rgadr))
> +		rapl_pl1 = 0;

No need for this either.

> +	else
> +		rapl_pl1 = rps_read_mmio(rps, rgadr);
> +
> +	return rapl_pl1;
> +}
> +
> +u32 intel_rps_get_rapl(struct intel_rps *rps, u32 rapl_pl1)
> +{
> +	struct drm_i915_private *i915 = rps_to_i915(rps);
> +	u32 rapl = 0;
> +
> +	if (IS_METEORLAKE(i915) || IS_XEHPSDV(i915))
> +		rapl = rapl_pl1 & RAPL_PL1_FREQ_LIMIT_MASK;
> +	else
> +		MISSING_CASE(GRAPHICS_VER(i915));

No need for this either.

> +
> +	return rapl;
> +}
> +
> +u32 intel_rps_read_rapl_pl1_frequency(struct intel_rps *rps)
> +{
> +	u32 rapl_freq = intel_rps_get_rapl(rps, intel_rps_read_rapl_pl1(rps));
> +
> +	return (rapl_freq >> 8) * GT_FREQUENCY_MULTIPLIER;

Use REG_FIELD_GET to extract the freq.

There doesn't seem any need to have 3 functions here, let's combine them
into a single function and use REG_FIELD_GET.

> +}
> +
>  /* External interface for intel_ips.ko */
>
>  static struct drm_i915_private __rcu *ips_mchdev;
> diff --git a/drivers/gpu/drm/i915/gt/intel_rps.h b/drivers/gpu/drm/i915/gt/intel_rps.h
> index 4509dfdc52e0..4adc6aaedba0 100644
> --- a/drivers/gpu/drm/i915/gt/intel_rps.h
> +++ b/drivers/gpu/drm/i915/gt/intel_rps.h
> @@ -34,6 +34,7 @@ void intel_rps_mark_interactive(struct intel_rps *rps, bool interactive);
>  int intel_gpu_freq(struct intel_rps *rps, int val);
>  int intel_freq_opcode(struct intel_rps *rps, int val);
>  u32 intel_rps_get_cagf(struct intel_rps *rps, u32 rpstat1);
> +u32 intel_rps_get_rapl(struct intel_rps *rps, u32 rapl_pl1);
>  u32 intel_rps_read_actual_frequency(struct intel_rps *rps);
>  u32 intel_rps_get_requested_frequency(struct intel_rps *rps);
>  u32 intel_rps_get_min_frequency(struct intel_rps *rps);
> @@ -47,6 +48,8 @@ u32 intel_rps_get_rp1_frequency(struct intel_rps *rps);
>  u32 intel_rps_get_rpn_frequency(struct intel_rps *rps);
>  u32 intel_rps_read_punit_req(struct intel_rps *rps);
>  u32 intel_rps_read_punit_req_frequency(struct intel_rps *rps);
> +u32 intel_rps_read_rapl_pl1(struct intel_rps *rps);
> +u32 intel_rps_read_rapl_pl1_frequency(struct intel_rps *rps);

These should be static and not in the .h file. Anyway I think as pointed
out above these 3 functions should probably be combined into just one.

>  void gen6_rps_get_freq_caps(struct intel_rps *rps, struct intel_rps_freq_caps *caps);
>  void intel_rps_raise_unslice(struct intel_rps *rps);
>  void intel_rps_lower_unslice(struct intel_rps *rps);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 5003a5ffbc6a..68ff98e27b8d 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1818,6 +1818,10 @@
>  #define   GT0_PERF_LIMIT_REASONS_LOG_MASK REG_GENMASK(31, 16)
>  #define MTL_MEDIA_PERF_LIMIT_REASONS	_MMIO(0x138030)
>
> +#define XEHPSDV_RAPL_PL1_FREQ_LIMIT	_MMIO(0x250070)
> +#define MTL_RAPL_PL1_FREQ_LIMIT	_MMIO(0x281070)
> +#define RAPL_PL1_FREQ_LIMIT_MASK	0xffff

Use REG_GENMASK(15, 0) here.

Also:
* The registers now need to be sorted by offset. Let's move these #defines
  to gt/intel_gt_regs.h
* RAPL_PL1_FREQ_LIMIT_MASK will need to be indented a couple of spaces as
  done for other registers

>  #define CHV_CLK_CTL1			_MMIO(0x101100)
>  #define VLV_CLK_CTL2			_MMIO(0x101104)
>  #define   CLK_CTL2_CZCOUNT_30NS_SHIFT	28
> --
> 2.34.1
>

Thanks.
--
Ashutosh



[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux