Re: [Intel-gfx] [PATCH 4/6] drm/i915/debugfs: Add perf_limit_reasons in debugfs

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Sep 02, 2022 at 04:53:00PM -0700, Ashutosh Dixit wrote:
> From: Tilak Tangudu <tilak.tangudu@intel.com>
> 
> Add perf_limit_reasons in debugfs. Unlike the lower 16 perf_limit_reasons
> status bits, the upper 16 log bits remain set until cleared, thereby
> ensuring the throttling occurrence is not missed. The clear fop clears
> the upper 16 log bits, the get fop gets all 32 log and status bits.
> 
> Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c | 27 +++++++++++++++++++
>  drivers/gpu/drm/i915/i915_reg.h               |  1 +
>  2 files changed, 28 insertions(+)
> 
> 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 108b9e76c32e..5c95cba5e5df 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> @@ -655,6 +655,32 @@ static bool rps_eval(void *data)
>  
>  DEFINE_INTEL_GT_DEBUGFS_ATTRIBUTE(rps_boost);
>  
> +static int perf_limit_reasons_get(void *data, u64 *val)
> +{
> +	struct intel_gt *gt = data;
> +	intel_wakeref_t wakeref;
> +
> +	with_intel_runtime_pm(gt->uncore->rpm, wakeref)
> +		*val = intel_uncore_read(gt->uncore, GT0_PERF_LIMIT_REASONS);
> +
> +	return 0;
> +}
> +
> +static int perf_limit_reasons_clear(void *data, u64 val)
> +{
> +	struct intel_gt *gt = data;
> +	intel_wakeref_t wakeref;
> +
> +	/* 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,
> +				 GT0_PERF_LIMIT_REASONS_LOG_MASK, 0);
> +
> +	return 0;
> +}
> +DEFINE_SIMPLE_ATTRIBUTE(perf_limit_reasons_fops, perf_limit_reasons_get,
> +			perf_limit_reasons_clear, "%llu\n");
> +
>  void intel_gt_pm_debugfs_register(struct intel_gt *gt, struct dentry *root)
>  {
>  	static const struct intel_gt_debugfs_file files[] = {
> @@ -664,6 +690,7 @@ void intel_gt_pm_debugfs_register(struct intel_gt *gt, struct dentry *root)
>  		{ "forcewake_user", &forcewake_user_fops, NULL},
>  		{ "llc", &llc_fops, llc_eval },
>  		{ "rps_boost", &rps_boost_fops, rps_eval },
> +		{ "perf_limit_reasons", &perf_limit_reasons_fops, NULL },
>  	};
>  
>  	intel_gt_debugfs_register_files(root, files, ARRAY_SIZE(files), gt);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 5e6239864c35..10126995e1f6 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1802,6 +1802,7 @@
>  #define   POWER_LIMIT_4_MASK		REG_BIT(9)
>  #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)

Is this valid for all platforms?
What does the bits are really telling us?
Could we expand the reasons? The previous bits we know exactly
what kind of limits we are dealing of, but with this combined
one without any explanation I'm afraid this will bring more
confusion than help. We will get bugged by many folks trying
to debug this out there when bit 13, for instance, is set.
"What does bit 13 mean?" will be a recurrent question with
only a tribal knowledge kind of answer.

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