Re: [Intel-gfx] [PATCH 6/9] drm/i915/pmu: Export counters from all tiles
On 30/03/2023 18:33, Umesh Nerlige Ramappa wrote:
On Thu, Mar 30, 2023 at 02:01:42PM +0100, Tvrtko Ursulin wrote:
On 30/03/2023 01:41, Umesh Nerlige Ramappa wrote:
From: Tvrtko Ursulin
Start exporting frequency and RC6 counters from all tiles.
Existing counters keep their names and config values and new one use the
namespace added in the previous patch, with the "-gtN" added to their
names.
The part about keeping the names is not in the code any more. So
something will have to give, either the commit text or the code.
Even without that detail, I suspect someone might want to add them
Co-developed-by since I *think* someone did some changes.
Interrupts counter is an odd one off. Because it is the global device
counters (not only GT) we choose not to add per tile versions for now.
Signed-off-by: Tvrtko Ursulin
Signed-off-by: Aravind Iddamsetty
---
drivers/gpu/drm/i915/i915_pmu.c | 96 ++---
1 file changed, 77 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_pmu.c
b/drivers/gpu/drm/i915/i915_pmu.c
index 5d1de98d86b4..2a5deabff088 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -548,8 +548,9 @@ config_status(struct drm_i915_private *i915, u64
config)
struct intel_gt *gt = to_gt(i915);
unsigned int gt_id = config_gt_id(config);
+ unsigned int max_gt_id = HAS_EXTRA_GT_LIST(i915) ? 1 : 0;
- if (gt_id)
+ if (gt_id > max_gt_id)
return -ENOENT;
switch (config_counter(config)) {
@@ -563,6 +564,8 @@ config_status(struct drm_i915_private *i915, u64
config)
return -ENODEV;
break;
case I915_PMU_INTERRUPTS:
+ if (gt_id)
+ return -ENOENT;
break;
case I915_PMU_RC6_RESIDENCY:
if (!gt->rc6.supported)
@@ -932,9 +935,9 @@ static const struct attribute_group
i915_pmu_cpumask_attr_group = {
.attrs = i915_cpumask_attrs,
};
-#define __event(__config, __name, __unit) \
+#define __event(__counter, __name, __unit) \
{ \
- .config = (__config), \
+ .counter = (__counter), \
.name = (__name), \
.unit = (__unit), \
}
@@ -975,15 +978,21 @@ create_event_attributes(struct i915_pmu *pmu)
{
struct drm_i915_private *i915 = container_of(pmu, typeof(*i915),
pmu);
static const struct {
- u64 config;
+ unsigned int counter;
const char *name;
const char *unit;
} events[] = {
- __event(I915_PMU_ACTUAL_FREQUENCY, "actual-frequency", "M"),
- __event(I915_PMU_REQUESTED_FREQUENCY, "requested-frequency",
"M"),
- __event(I915_PMU_INTERRUPTS, "interrupts", NULL),
- __event(I915_PMU_RC6_RESIDENCY, "rc6-residency", "ns"),
- __event(I915_PMU_SOFTWARE_GT_AWAKE_TIME,
"software-gt-awake-time", "ns"),
+ __event(0, "actual-frequency", "M"),
+ __event(1, "requested-frequency", "M"),
+ __event(3, "rc6-residency", "ns"),
+ __event(4, "software-gt-awake-time", "ns"),
+ };
+ static const struct {
+ unsigned int counter;
+ const char *name;
+ const char *unit;
+ } global_events[] = {
+ __event(2, "interrupts", NULL),
};
static const struct {
enum drm_i915_pmu_engine_sample sample;
@@ -998,14 +1007,29 @@ create_event_attributes(struct i915_pmu *pmu)
struct i915_ext_attribute *i915_attr = NULL, *i915_iter;
struct attribute **attr = NULL, **attr_iter;
struct intel_engine_cs *engine;
- unsigned int i;
+ struct intel_gt *gt;
+ unsigned int i, j;
/* Count how many counters we will be exposing. */
- for (i = 0; i < ARRAY_SIZE(events); i++) {
- if (!config_status(i915, events[i].config))
+ /* per gt counters */
Two comments one by another, two styles - the inconsistency hurts.
Not sure why global events needed to be split out into a separate
array? Like this below two loops are needed for each stage instead of
one. AFAIR one array and one loop would just work because
config_status wold report global ones as unsupported for gt > 0.
The idea was to add the legacy events into the global array. These
events will not have -gtN appeneded to it. Note that on a single gt
platform, my idea is to have both legacy as well as gt0 events.
ADLP:
actual-frequency
actual-frequency-gt0
IMO that would be pointless and harmful even.
MTL:
actual-frequency
actual-frequency-gt0
actual-frequency-gt1
This one lets cover in discussion against 9/9.
[Comes back later. It looked like this in my code:
static const struct {
- u64 config;
+ unsigned int counter;
const char *name;
const char *unit;
+ bool global;
} events[] = {
- __event(I915_PMU_ACTUAL_FREQUENCY, "actual-frequency",
"M"),
- __event(I915_PMU_REQUESTED_FREQUENCY,
"requested-frequency", "M"),
- __event(I915_PMU_INTERRUPTS, "interrupts", NULL),
-
Re: [Intel-gfx] [PATCH 6/9] drm/i915/pmu: Export counters from all tiles
On Thu, Mar 30, 2023 at 02:01:42PM +0100, Tvrtko Ursulin wrote:
On 30/03/2023 01:41, Umesh Nerlige Ramappa wrote:
From: Tvrtko Ursulin
Start exporting frequency and RC6 counters from all tiles.
Existing counters keep their names and config values and new one use the
namespace added in the previous patch, with the "-gtN" added to their
names.
The part about keeping the names is not in the code any more. So something will
have to give, either the commit text or the code.
Even without that detail, I suspect someone might want to add them
Co-developed-by since I *think* someone did some changes.
Interrupts counter is an odd one off. Because it is the global device
counters (not only GT) we choose not to add per tile versions for now.
Signed-off-by: Tvrtko Ursulin
Signed-off-by: Aravind Iddamsetty
---
drivers/gpu/drm/i915/i915_pmu.c | 96 ++---
1 file changed, 77 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 5d1de98d86b4..2a5deabff088 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -548,8 +548,9 @@ config_status(struct drm_i915_private *i915, u64 config)
struct intel_gt *gt = to_gt(i915);
unsigned int gt_id = config_gt_id(config);
+ unsigned int max_gt_id = HAS_EXTRA_GT_LIST(i915) ? 1 : 0;
- if (gt_id)
+ if (gt_id > max_gt_id)
return -ENOENT;
switch (config_counter(config)) {
@@ -563,6 +564,8 @@ config_status(struct drm_i915_private *i915, u64 config)
return -ENODEV;
break;
case I915_PMU_INTERRUPTS:
+ if (gt_id)
+ return -ENOENT;
break;
case I915_PMU_RC6_RESIDENCY:
if (!gt->rc6.supported)
@@ -932,9 +935,9 @@ static const struct attribute_group
i915_pmu_cpumask_attr_group = {
.attrs = i915_cpumask_attrs,
};
-#define __event(__config, __name, __unit) \
+#define __event(__counter, __name, __unit) \
{ \
- .config = (__config), \
+ .counter = (__counter), \
.name = (__name), \
.unit = (__unit), \
}
@@ -975,15 +978,21 @@ create_event_attributes(struct i915_pmu *pmu)
{
struct drm_i915_private *i915 = container_of(pmu, typeof(*i915), pmu);
static const struct {
- u64 config;
+ unsigned int counter;
const char *name;
const char *unit;
} events[] = {
- __event(I915_PMU_ACTUAL_FREQUENCY, "actual-frequency", "M"),
- __event(I915_PMU_REQUESTED_FREQUENCY, "requested-frequency",
"M"),
- __event(I915_PMU_INTERRUPTS, "interrupts", NULL),
- __event(I915_PMU_RC6_RESIDENCY, "rc6-residency", "ns"),
- __event(I915_PMU_SOFTWARE_GT_AWAKE_TIME, "software-gt-awake-time",
"ns"),
+ __event(0, "actual-frequency", "M"),
+ __event(1, "requested-frequency", "M"),
+ __event(3, "rc6-residency", "ns"),
+ __event(4, "software-gt-awake-time", "ns"),
+ };
+ static const struct {
+ unsigned int counter;
+ const char *name;
+ const char *unit;
+ } global_events[] = {
+ __event(2, "interrupts", NULL),
};
static const struct {
enum drm_i915_pmu_engine_sample sample;
@@ -998,14 +1007,29 @@ create_event_attributes(struct i915_pmu *pmu)
struct i915_ext_attribute *i915_attr = NULL, *i915_iter;
struct attribute **attr = NULL, **attr_iter;
struct intel_engine_cs *engine;
- unsigned int i;
+ struct intel_gt *gt;
+ unsigned int i, j;
/* Count how many counters we will be exposing. */
- for (i = 0; i < ARRAY_SIZE(events); i++) {
- if (!config_status(i915, events[i].config))
+ /* per gt counters */
Two comments one by another, two styles - the inconsistency hurts.
Not sure why global events needed to be split out into a separate array? Like this
below two loops are needed for each stage instead of one. AFAIR one array and one
loop would just work because config_status wold report global ones as unsupported
for gt > 0.
The idea was to add the legacy events into the global array. These
events will not have -gtN appeneded to it. Note that on a single gt
platform, my idea is to have both legacy as well as gt0 events.
ADLP:
actual-frequency
actual-frequency-gt0
MTL:
actual-frequency
actual-frequency-gt0
actual-frequency-gt1
[Comes back later. It looked like this in my code:
static const struct {
- u64 config;
+ unsigned int counter;
const char *name;
const char *unit;
+ bool global;
} events[] = {
- __event(I915_PMU_ACTUAL_FREQUENCY, "actual-frequency", "M"),
-
Re: [Intel-gfx] [PATCH 6/9] drm/i915/pmu: Export counters from all tiles
On 30/03/2023 01:41, Umesh Nerlige Ramappa wrote:
From: Tvrtko Ursulin
Start exporting frequency and RC6 counters from all tiles.
Existing counters keep their names and config values and new one use the
namespace added in the previous patch, with the "-gtN" added to their
names.
The part about keeping the names is not in the code any more. So something will
have to give, either the commit text or the code.
Even without that detail, I suspect someone might want to add them
Co-developed-by since I *think* someone did some changes.
Interrupts counter is an odd one off. Because it is the global device
counters (not only GT) we choose not to add per tile versions for now.
Signed-off-by: Tvrtko Ursulin
Signed-off-by: Aravind Iddamsetty
---
drivers/gpu/drm/i915/i915_pmu.c | 96 ++---
1 file changed, 77 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 5d1de98d86b4..2a5deabff088 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -548,8 +548,9 @@ config_status(struct drm_i915_private *i915, u64 config)
struct intel_gt *gt = to_gt(i915);
unsigned int gt_id = config_gt_id(config);
+ unsigned int max_gt_id = HAS_EXTRA_GT_LIST(i915) ? 1 : 0;
- if (gt_id)
+ if (gt_id > max_gt_id)
return -ENOENT;
switch (config_counter(config)) {
@@ -563,6 +564,8 @@ config_status(struct drm_i915_private *i915, u64 config)
return -ENODEV;
break;
case I915_PMU_INTERRUPTS:
+ if (gt_id)
+ return -ENOENT;
break;
case I915_PMU_RC6_RESIDENCY:
if (!gt->rc6.supported)
@@ -932,9 +935,9 @@ static const struct attribute_group
i915_pmu_cpumask_attr_group = {
.attrs = i915_cpumask_attrs,
};
-#define __event(__config, __name, __unit) \
+#define __event(__counter, __name, __unit) \
{ \
- .config = (__config), \
+ .counter = (__counter), \
.name = (__name), \
.unit = (__unit), \
}
@@ -975,15 +978,21 @@ create_event_attributes(struct i915_pmu *pmu)
{
struct drm_i915_private *i915 = container_of(pmu, typeof(*i915), pmu);
static const struct {
- u64 config;
+ unsigned int counter;
const char *name;
const char *unit;
} events[] = {
- __event(I915_PMU_ACTUAL_FREQUENCY, "actual-frequency", "M"),
- __event(I915_PMU_REQUESTED_FREQUENCY, "requested-frequency",
"M"),
- __event(I915_PMU_INTERRUPTS, "interrupts", NULL),
- __event(I915_PMU_RC6_RESIDENCY, "rc6-residency", "ns"),
- __event(I915_PMU_SOFTWARE_GT_AWAKE_TIME, "software-gt-awake-time",
"ns"),
+ __event(0, "actual-frequency", "M"),
+ __event(1, "requested-frequency", "M"),
+ __event(3, "rc6-residency", "ns"),
+ __event(4, "software-gt-awake-time", "ns"),
+ };
+ static const struct {
+ unsigned int counter;
+ const char *name;
+ const char *unit;
+ } global_events[] = {
+ __event(2, "interrupts", NULL),
};
static const struct {
enum drm_i915_pmu_engine_sample sample;
@@ -998,14 +1007,29 @@ create_event_attributes(struct i915_pmu *pmu)
struct i915_ext_attribute *i915_attr = NULL, *i915_iter;
struct attribute **attr = NULL, **attr_iter;
struct intel_engine_cs *engine;
- unsigned int i;
+ struct intel_gt *gt;
+ unsigned int i, j;
/* Count how many counters we will be exposing. */
- for (i = 0; i < ARRAY_SIZE(events); i++) {
- if (!config_status(i915, events[i].config))
+ /* per gt counters */
Two comments one by another, two styles - the inconsistency hurts.
Not sure why global events needed to be split out into a separate array? Like this
below two loops are needed for each stage instead of one. AFAIR one array and one
loop would just work because config_status wold report global ones as unsupported
for gt > 0.
[Comes back later. It looked like this in my code:
static const struct {
- u64 config;
+ unsigned int counter;
const char *name;
const char *unit;
+ bool global;
} events[] = {
- __event(I915_PMU_ACTUAL_FREQUENCY, "actual-frequency", "M"),
- __event(I915_PMU_REQUESTED_FREQUENCY, "requested-frequency",
"M"),
- __event(I915_PMU_INTERRUPTS, "interrupts", NULL),
- __event(I915_PMU_RC6_RESIDENCY, "rc6-residency", "ns"),
+ /*
+* #define __I915_PMU_ACTUAL_FREQUENCY(gt)
___I915_PMU_OTHER(gt, 0)
+* #define __I915_PMU_REQUESTED_FREQUENCY(gt)
[Intel-gfx] [PATCH 6/9] drm/i915/pmu: Export counters from all tiles
From: Tvrtko Ursulin
Start exporting frequency and RC6 counters from all tiles.
Existing counters keep their names and config values and new one use the
namespace added in the previous patch, with the "-gtN" added to their
names.
Interrupts counter is an odd one off. Because it is the global device
counters (not only GT) we choose not to add per tile versions for now.
Signed-off-by: Tvrtko Ursulin
Signed-off-by: Aravind Iddamsetty
---
drivers/gpu/drm/i915/i915_pmu.c | 96 ++---
1 file changed, 77 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 5d1de98d86b4..2a5deabff088 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -548,8 +548,9 @@ config_status(struct drm_i915_private *i915, u64 config)
struct intel_gt *gt = to_gt(i915);
unsigned int gt_id = config_gt_id(config);
+ unsigned int max_gt_id = HAS_EXTRA_GT_LIST(i915) ? 1 : 0;
- if (gt_id)
+ if (gt_id > max_gt_id)
return -ENOENT;
switch (config_counter(config)) {
@@ -563,6 +564,8 @@ config_status(struct drm_i915_private *i915, u64 config)
return -ENODEV;
break;
case I915_PMU_INTERRUPTS:
+ if (gt_id)
+ return -ENOENT;
break;
case I915_PMU_RC6_RESIDENCY:
if (!gt->rc6.supported)
@@ -932,9 +935,9 @@ static const struct attribute_group
i915_pmu_cpumask_attr_group = {
.attrs = i915_cpumask_attrs,
};
-#define __event(__config, __name, __unit) \
+#define __event(__counter, __name, __unit) \
{ \
- .config = (__config), \
+ .counter = (__counter), \
.name = (__name), \
.unit = (__unit), \
}
@@ -975,15 +978,21 @@ create_event_attributes(struct i915_pmu *pmu)
{
struct drm_i915_private *i915 = container_of(pmu, typeof(*i915), pmu);
static const struct {
- u64 config;
+ unsigned int counter;
const char *name;
const char *unit;
} events[] = {
- __event(I915_PMU_ACTUAL_FREQUENCY, "actual-frequency", "M"),
- __event(I915_PMU_REQUESTED_FREQUENCY, "requested-frequency",
"M"),
- __event(I915_PMU_INTERRUPTS, "interrupts", NULL),
- __event(I915_PMU_RC6_RESIDENCY, "rc6-residency", "ns"),
- __event(I915_PMU_SOFTWARE_GT_AWAKE_TIME,
"software-gt-awake-time", "ns"),
+ __event(0, "actual-frequency", "M"),
+ __event(1, "requested-frequency", "M"),
+ __event(3, "rc6-residency", "ns"),
+ __event(4, "software-gt-awake-time", "ns"),
+ };
+ static const struct {
+ unsigned int counter;
+ const char *name;
+ const char *unit;
+ } global_events[] = {
+ __event(2, "interrupts", NULL),
};
static const struct {
enum drm_i915_pmu_engine_sample sample;
@@ -998,14 +1007,29 @@ create_event_attributes(struct i915_pmu *pmu)
struct i915_ext_attribute *i915_attr = NULL, *i915_iter;
struct attribute **attr = NULL, **attr_iter;
struct intel_engine_cs *engine;
- unsigned int i;
+ struct intel_gt *gt;
+ unsigned int i, j;
/* Count how many counters we will be exposing. */
- for (i = 0; i < ARRAY_SIZE(events); i++) {
- if (!config_status(i915, events[i].config))
+ /* per gt counters */
+ for_each_gt(gt, i915, j) {
+ for (i = 0; i < ARRAY_SIZE(events); i++) {
+ u64 config = ___I915_PMU_OTHER(j, events[i].counter);
+
+ if (!config_status(i915, config))
+ count++;
+ }
+ }
+
+ /* global (per GPU) counters */
+ for (i = 0; i < ARRAY_SIZE(global_events); i++) {
+ u64 config = ___I915_PMU_OTHER(0, global_events[i].counter);
+
+ if (!config_status(i915, config))
count++;
}
+ /* per engine counters */
for_each_uabi_engine(engine, i915) {
for (i = 0; i < ARRAY_SIZE(engine_events); i++) {
if (!engine_event_status(engine,
@@ -1033,26 +1057,60 @@ create_event_attributes(struct i915_pmu *pmu)
attr_iter = attr;
/* Initialize supported non-engine counters. */
- for (i = 0; i < ARRAY_SIZE(events); i++) {
+ /* per gt counters */
+ for_each_gt(gt, i915, j) {
+ for (i = 0; i < ARRAY_SIZE(events); i++) {
+ u64 config = ___I915_PMU_OTHER(j, events[i].counter);
+ char *str;
+
+ if (config_status(i915, config))
+ continue;
+
+ str = kasprintf(GFP_KERNEL, "%s-gt%u",
+
