Re: [Intel-gfx] [PATCH 4/6] drm/i915/debugfs: Add perf_limit_reasons in debugfs
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
> >
> > 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
> > ---
> > 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", _user_fops, NULL},
> > { "llc", _fops, llc_eval },
> > { "rps_boost", _boost_fops, rps_eval },
> > + { "perf_limit_reasons", _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
Re: [Intel-gfx] [PATCH 4/6] drm/i915/debugfs: Add perf_limit_reasons in debugfs
On Tue, 06 Sep 2022 07:13:03 -0700, Rodrigo Vivi wrote:
>
Copying author.
> On Fri, Sep 02, 2022 at 04:53:00PM -0700, Ashutosh Dixit wrote:
> > From: Tilak Tangudu
> >
> > 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
> > ---
> > 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", _user_fops, NULL},
> > { "llc", _fops, llc_eval },
> > { "rps_boost", _boost_fops, rps_eval },
> > + { "perf_limit_reasons", _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
> >
Re: [Intel-gfx] [PATCH 4/6] drm/i915/debugfs: Add perf_limit_reasons in debugfs
On Fri, Sep 02, 2022 at 04:53:00PM -0700, Ashutosh Dixit wrote:
> From: Tilak Tangudu
>
> 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
> ---
> 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", _user_fops, NULL},
> { "llc", _fops, llc_eval },
> { "rps_boost", _boost_fops, rps_eval },
> + { "perf_limit_reasons", _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
>
[Intel-gfx] [PATCH 4/6] drm/i915/debugfs: Add perf_limit_reasons in debugfs
From: Tilak Tangudu
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
---
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", _user_fops, NULL},
{ "llc", _fops, llc_eval },
{ "rps_boost", _boost_fops, rps_eval },
+ { "perf_limit_reasons", _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)
#define CHV_CLK_CTL1 _MMIO(0x101100)
#define VLV_CLK_CTL2 _MMIO(0x101104)
--
2.34.1
