Re: [Intel-gfx] [PATCH 5/6] drm/i915/mtl: PERF_LIMIT_REASONS changes for MTL

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 02 Sep 2022, Ashutosh Dixit <ashutosh.dixit@intel.com> wrote:
> PERF_LIMIT_REASONS register for MTL media gt is different now.
>
> Cc: Badal Nilawar <badal.nilawar@intel.com>
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_gt.h            | 8 ++++++++
>  drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c | 4 ++--
>  drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c   | 6 +++---
>  drivers/gpu/drm/i915/i915_reg.h               | 1 +
>  4 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h b/drivers/gpu/drm/i915/gt/intel_gt.h
> index c9a359f35d0f..7286d47113ee 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.h
> @@ -9,6 +9,7 @@
>  #include "intel_engine_types.h"
>  #include "intel_gt_types.h"
>  #include "intel_reset.h"
> +#include "i915_reg.h"
>  
>  struct drm_i915_private;
>  struct drm_printer;
> @@ -86,6 +87,13 @@ static inline bool intel_gt_is_wedged(const struct intel_gt *gt)
>  	return unlikely(test_bit(I915_WEDGED, &gt->reset.flags));
>  }
>  
> +static inline
> +i915_reg_t intel_gt_perf_limit_reasons_reg(struct intel_gt *gt)
> +{
> +	return gt->type == GT_MEDIA ?
> +		MTL_MEDIA_PERF_LIMIT_REASONS : GT0_PERF_LIMIT_REASONS;
> +}

Nowadays, I pretty much think of everything from the standpoint of
setting the example for future changes. Is this what we want people to
copy? Because that's what we do, look for examples for what we want to
achieve, and emulate.

Do we want this to be duplicated for other registers? Choose register
offset based on platform/engine/fusing/whatever parameter? Is this a
register definition that should be in a _regs.h file?

I don't know.

I've also grown to dislike static inlines a lot, and this one's the
worst because it actually can't be static inline because its passed as a
function pointer.


BR,
Jani.



> +
>  int intel_gt_probe_all(struct drm_i915_private *i915);
>  int intel_gt_tiles_init(struct drm_i915_private *i915);
>  void intel_gt_release_all(struct drm_i915_private *i915);
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> index 5c95cba5e5df..fe0091f953c1 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> @@ -661,7 +661,7 @@ static int perf_limit_reasons_get(void *data, u64 *val)
>  	intel_wakeref_t wakeref;
>  
>  	with_intel_runtime_pm(gt->uncore->rpm, wakeref)
> -		*val = intel_uncore_read(gt->uncore, GT0_PERF_LIMIT_REASONS);
> +		*val = intel_uncore_read(gt->uncore, intel_gt_perf_limit_reasons_reg(gt));
>  
>  	return 0;
>  }
> @@ -673,7 +673,7 @@ static int perf_limit_reasons_clear(void *data, u64 val)
>  
>  	/* Clear the upper 16 log bits, the lower 16 status bits are read-only */
>  	with_intel_runtime_pm(gt->uncore->rpm, wakeref)
> -		intel_uncore_rmw(gt->uncore, GT0_PERF_LIMIT_REASONS,
> +		intel_uncore_rmw(gt->uncore, intel_gt_perf_limit_reasons_reg(gt),
>  				 GT0_PERF_LIMIT_REASONS_LOG_MASK, 0);
>  
>  	return 0;
> 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 e066cc33d9f2..54deae45d81f 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
> @@ -510,7 +510,7 @@ struct intel_gt_bool_throttle_attr {
>  	struct attribute attr;
>  	ssize_t (*show)(struct device *dev, struct device_attribute *attr,
>  			char *buf);
> -	i915_reg_t reg32;
> +	i915_reg_t (*reg32)(struct intel_gt *gt);
>  	u32 mask;
>  };
>  
> @@ -521,7 +521,7 @@ static ssize_t throttle_reason_bool_show(struct device *dev,
>  	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
>  	struct intel_gt_bool_throttle_attr *t_attr =
>  				(struct intel_gt_bool_throttle_attr *) attr;
> -	bool val = rps_read_mask_mmio(&gt->rps, t_attr->reg32, t_attr->mask);
> +	bool val = rps_read_mask_mmio(&gt->rps, t_attr->reg32(gt), t_attr->mask);
>  
>  	return sysfs_emit(buff, "%u\n", val);
>  }
> @@ -530,7 +530,7 @@ static ssize_t throttle_reason_bool_show(struct device *dev,
>  struct intel_gt_bool_throttle_attr attr_##sysfs_func__ = { \
>  	.attr = { .name = __stringify(sysfs_func__), .mode = 0444 }, \
>  	.show = throttle_reason_bool_show, \
> -	.reg32 = GT0_PERF_LIMIT_REASONS, \
> +	.reg32 = intel_gt_perf_limit_reasons_reg, \
>  	.mask = mask__, \
>  }
>  
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 10126995e1f6..06d555321651 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1803,6 +1803,7 @@
>  #define   POWER_LIMIT_1_MASK		REG_BIT(11)
>  #define   POWER_LIMIT_2_MASK		REG_BIT(12)
>  #define   GT0_PERF_LIMIT_REASONS_LOG_MASK REG_GENMASK(31, 16)
> +#define MTL_MEDIA_PERF_LIMIT_REASONS	_MMIO(0x138030)
>  
>  #define CHV_CLK_CTL1			_MMIO(0x101100)
>  #define VLV_CLK_CTL2			_MMIO(0x101104)

-- 
Jani Nikula, Intel Open Source Graphics Center