Re: [Intel-gfx] [PATCH 5/6] drm/i915/mtl: PERF_LIMIT_REASONS changes for MTL
On Mon, 05 Sep 2022 02:30:45 -0700, Jani Nikula wrote:
>
> On Fri, 02 Sep 2022, Ashutosh Dixit wrote:
> > PERF_LIMIT_REASONS register for MTL media gt is different now.
> >
> > Cc: Badal Nilawar
> > Signed-off-by: Ashutosh Dixit
> > ---
> > 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, >->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.
MTL_MEDIA_PERF_LIMIT_REASONS is an actual register so I'd think it needs to
be in a _regs.h file. And here we need to choose the register offset at
runtime based on the gt. So I don't see any way round what's happening
above unless you have other suggestions.
> 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.
Based on your feedback I've eliminated the static inline and moved the
function definition to a .c in v2 (though gcc allows taking addresses of
static inline's in .h files).
Thanks.
--
Ashutosh
>
>
> 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(>->rps, t_attr->reg32, t_attr->mask);
> > + bool val = rps_read_mask_mmio(>->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 a
Re: [Intel-gfx] [PATCH 5/6] drm/i915/mtl: PERF_LIMIT_REASONS changes for MTL
On Fri, 02 Sep 2022, Ashutosh Dixit wrote:
> PERF_LIMIT_REASONS register for MTL media gt is different now.
>
> Cc: Badal Nilawar
> Signed-off-by: Ashutosh Dixit
> ---
> 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, >->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(>->rps, t_attr->reg32, t_attr->mask);
> + bool val = rps_read_mask_mmio(>->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)
>
[Intel-gfx] [PATCH 5/6] drm/i915/mtl: PERF_LIMIT_REASONS changes for MTL
PERF_LIMIT_REASONS register for MTL media gt is different now.
Cc: Badal Nilawar
Signed-off-by: Ashutosh Dixit
---
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, >->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;
+}
+
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(>->rps, t_attr->reg32, t_attr->mask);
+ bool val = rps_read_mask_mmio(>->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)
--
2.34.1
