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

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Tue, 06 Sep 2022 07:13:03 -0700, Rodrigo Vivi wrote:
>

Hi Rodrigo,

> 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?

Yes, looks like it.

> What does the bits are really telling us?

The v1 commit message above hinted at what was happening, I've clarified
the commit message in v2 as follows:

    Add perf_limit_reasons in debugfs. The upper 16 perf_limit_reasons RW "log"
    bits are identical to the lower 16 RO "status" bits except that the "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.

I've also expanded the comment in perf_limit_reasons_clear() to explain this.

> 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.

I think the new commit message above and comment has the answer to this
now. Also, won't there be a public copy of the Bspec where someone can look
up the bit definitions?

Also, are these "log" bits useful enough to expose them in sysfs like we
have the lower "status" bits exposed today but that is probably the
question for a different patch.

Thanks.
--
Ashutosh