| Message ID | 20230506005816.1891043-7-umesh.nerlige.ramappa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add MTL PMU support for multi-gt | expand |
On Fri, May 05, 2023 at 05:58:16PM -0700, Umesh Nerlige Ramappa wrote: >From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >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. UMD specific changes link needed here as well. With that: Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> Thanks, Umesh > >Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@intel.com> >Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> >--- > drivers/gpu/drm/i915/i915_pmu.c | 87 ++++++++++++++++++++++----------- > 1 file changed, 59 insertions(+), 28 deletions(-) > >diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c >index 12b2f3169abf..284e5c5b97bb 100644 >--- a/drivers/gpu/drm/i915/i915_pmu.c >+++ b/drivers/gpu/drm/i915/i915_pmu.c >@@ -546,8 +546,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)) { >@@ -561,6 +562,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) >@@ -930,11 +933,20 @@ 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), \ >+ .global = false, \ >+} >+ >+#define __global_event(__counter, __name, __unit) \ >+{ \ >+ .counter = (__counter), \ >+ .name = (__name), \ >+ .unit = (__unit), \ >+ .global = true, \ > } > > #define __engine_event(__sample, __name) \ >@@ -973,15 +985,16 @@ 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; >+ 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"), >- __event(I915_PMU_SOFTWARE_GT_AWAKE_TIME, "software-gt-awake-time", "ns"), >+ __event(0, "actual-frequency", "M"), >+ __event(1, "requested-frequency", "M"), >+ __global_event(2, "interrupts", NULL), >+ __event(3, "rc6-residency", "ns"), >+ __event(4, "software-gt-awake-time", "ns"), > }; > static const struct { > enum drm_i915_pmu_engine_sample sample; >@@ -996,12 +1009,17 @@ 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)) >- count++; >+ 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++; >+ } > } > > for_each_uabi_engine(engine, i915) { >@@ -1031,26 +1049,39 @@ create_event_attributes(struct i915_pmu *pmu) > attr_iter = attr; > > /* Initialize supported non-engine counters. */ >- for (i = 0; i < ARRAY_SIZE(events); i++) { >- char *str; >- >- if (config_status(i915, events[i].config)) >- continue; >- >- str = kstrdup(events[i].name, GFP_KERNEL); >- if (!str) >- goto err; >+ 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; > >- *attr_iter++ = &i915_iter->attr.attr; >- i915_iter = add_i915_attr(i915_iter, str, events[i].config); >+ if (config_status(i915, config)) >+ continue; > >- if (events[i].unit) { >- str = kasprintf(GFP_KERNEL, "%s.unit", events[i].name); >+ if (events[i].global || !HAS_EXTRA_GT_LIST(i915)) >+ str = kstrdup(events[i].name, GFP_KERNEL); >+ else >+ str = kasprintf(GFP_KERNEL, "%s-gt%u", >+ events[i].name, j); > if (!str) > goto err; > >- *attr_iter++ = &pmu_iter->attr.attr; >- pmu_iter = add_pmu_attr(pmu_iter, str, events[i].unit); >+ *attr_iter++ = &i915_iter->attr.attr; >+ i915_iter = add_i915_attr(i915_iter, str, config); >+ >+ if (events[i].unit) { >+ if (events[i].global || !HAS_EXTRA_GT_LIST(i915)) >+ str = kasprintf(GFP_KERNEL, "%s.unit", >+ events[i].name); >+ else >+ str = kasprintf(GFP_KERNEL, "%s-gt%u.unit", >+ events[i].name, j); >+ if (!str) >+ goto err; >+ >+ *attr_iter++ = &pmu_iter->attr.attr; >+ pmu_iter = add_pmu_attr(pmu_iter, str, >+ events[i].unit); >+ } > } > } > >-- >2.36.1 >
On 06/05/2023 01:58, Umesh Nerlige Ramappa wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > 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 <tvrtko.ursulin@intel.com> > Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@intel.com> > Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> > --- > drivers/gpu/drm/i915/i915_pmu.c | 87 ++++++++++++++++++++++----------- > 1 file changed, 59 insertions(+), 28 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c > index 12b2f3169abf..284e5c5b97bb 100644 > --- a/drivers/gpu/drm/i915/i915_pmu.c > +++ b/drivers/gpu/drm/i915/i915_pmu.c > @@ -546,8 +546,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) Would this be clearer as: unsigned in num_gts = 1 + (unsigned int)HAS_EXTRA_GT_LIST(i915); if (gt_id >= num_gts) ? Just thinking out loud, no real opinion either way. > return -ENOENT; > > switch (config_counter(config)) { > @@ -561,6 +562,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) > @@ -930,11 +933,20 @@ 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), \ > + .global = false, \ > +} > + > +#define __global_event(__counter, __name, __unit) \ > +{ \ > + .counter = (__counter), \ > + .name = (__name), \ > + .unit = (__unit), \ > + .global = true, \ > } > > #define __engine_event(__sample, __name) \ > @@ -973,15 +985,16 @@ 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; > + 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"), > - __event(I915_PMU_SOFTWARE_GT_AWAKE_TIME, "software-gt-awake-time", "ns"), > + __event(0, "actual-frequency", "M"), > + __event(1, "requested-frequency", "M"), > + __global_event(2, "interrupts", NULL), > + __event(3, "rc6-residency", "ns"), > + __event(4, "software-gt-awake-time", "ns"), > }; > static const struct { > enum drm_i915_pmu_engine_sample sample; > @@ -996,12 +1009,17 @@ 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)) > - count++; > + 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++; > + } > } > > for_each_uabi_engine(engine, i915) { > @@ -1031,26 +1049,39 @@ create_event_attributes(struct i915_pmu *pmu) > attr_iter = attr; > > /* Initialize supported non-engine counters. */ > - for (i = 0; i < ARRAY_SIZE(events); i++) { > - char *str; > - > - if (config_status(i915, events[i].config)) > - continue; > - > - str = kstrdup(events[i].name, GFP_KERNEL); > - if (!str) > - goto err; > + 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; > > - *attr_iter++ = &i915_iter->attr.attr; > - i915_iter = add_i915_attr(i915_iter, str, events[i].config); > + if (config_status(i915, config)) > + continue; > > - if (events[i].unit) { > - str = kasprintf(GFP_KERNEL, "%s.unit", events[i].name); > + if (events[i].global || !HAS_EXTRA_GT_LIST(i915)) > + str = kstrdup(events[i].name, GFP_KERNEL); > + else > + str = kasprintf(GFP_KERNEL, "%s-gt%u", > + events[i].name, j); > if (!str) > goto err; > > - *attr_iter++ = &pmu_iter->attr.attr; > - pmu_iter = add_pmu_attr(pmu_iter, str, events[i].unit); > + *attr_iter++ = &i915_iter->attr.attr; > + i915_iter = add_i915_attr(i915_iter, str, config); > + > + if (events[i].unit) { > + if (events[i].global || !HAS_EXTRA_GT_LIST(i915)) Maybe worth moving the condition to for_each_gt? bool use_gt_suffix = HAS_EXTRA_GT_LIST(i915) && !events[i].global; Again, more questionable bike shedding. Regards, Tvrtko > + str = kasprintf(GFP_KERNEL, "%s.unit", > + events[i].name); > + else > + str = kasprintf(GFP_KERNEL, "%s-gt%u.unit", > + events[i].name, j); > + if (!str) > + goto err; > + > + *attr_iter++ = &pmu_iter->attr.attr; > + pmu_iter = add_pmu_attr(pmu_iter, str, > + events[i].unit); > + } > } > } >
On Fri, 05 May 2023 17:58:16 -0700, Umesh Nerlige Ramappa wrote: > One drive-by comment: > diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c > index 12b2f3169abf..284e5c5b97bb 100644 > --- a/drivers/gpu/drm/i915/i915_pmu.c > +++ b/drivers/gpu/drm/i915/i915_pmu.c > @@ -546,8 +546,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; But in Patch 5 we have: #define I915_PMU_MAX_GTS (4) > > - if (gt_id) > + if (gt_id > max_gt_id) > return -ENOENT; > > switch (config_counter(config)) { > @@ -561,6 +562,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) Thanks. -- Ashutosh
On 11/05/2023 19:57, Dixit, Ashutosh wrote: > On Fri, 05 May 2023 17:58:16 -0700, Umesh Nerlige Ramappa wrote: >> > > One drive-by comment: > >> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c >> index 12b2f3169abf..284e5c5b97bb 100644 >> --- a/drivers/gpu/drm/i915/i915_pmu.c >> +++ b/drivers/gpu/drm/i915/i915_pmu.c >> @@ -546,8 +546,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; > > But in Patch 5 we have: > > #define I915_PMU_MAX_GTS (4) AFAIR that one is just to size the internal arrays, while max_gt_id is to report to userspace which events are present. Regards, Tvrtko > >> >> - if (gt_id) >> + if (gt_id > max_gt_id) >> return -ENOENT; >> >> switch (config_counter(config)) { >> @@ -561,6 +562,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) > > Thanks. > -- > Ashutosh
On Fri, 12 May 2023 03:57:35 -0700, Tvrtko Ursulin wrote: > > > On 11/05/2023 19:57, Dixit, Ashutosh wrote: > > On Fri, 05 May 2023 17:58:16 -0700, Umesh Nerlige Ramappa wrote: > >> > > > > One drive-by comment: > > > >> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c > >> index 12b2f3169abf..284e5c5b97bb 100644 > >> --- a/drivers/gpu/drm/i915/i915_pmu.c > >> +++ b/drivers/gpu/drm/i915/i915_pmu.c > >> @@ -546,8 +546,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; > > > > But in Patch 5 we have: > > > > #define I915_PMU_MAX_GTS (4) > > AFAIR that one is just to size the internal arrays, while max_gt_id is to > report to userspace which events are present. Hmm, apart from the #defines's in i915_drm.h in Patch 5, not seeing anything else reported to userspace about which events are present. Also, we already have I915_MAX_GT, we shouldn't need I915_PMU_MAX_GTS, or at least: #define I915_PMU_MAX_GTS I915_MAX_GT Better to use things uniformly. If we want I915_PMU_MAX_GTS to be 2 instead of I915_MAX_GT (but why?, below is just a check) let's do #define I915_PMU_MAX_GTS 2 And use that in the code above. But I think we should just use I915_MAX_GT. Thanks. -- Ashutosh > > > >> > >> - if (gt_id) > >> + if (gt_id > max_gt_id) > >> return -ENOENT; > >> > >> switch (config_counter(config)) { > >> @@ -561,6 +562,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)
On Fri, May 12, 2023 at 10:08:58AM -0700, Dixit, Ashutosh wrote: >On Fri, 12 May 2023 03:57:35 -0700, Tvrtko Ursulin wrote: >> >> >> On 11/05/2023 19:57, Dixit, Ashutosh wrote: >> > On Fri, 05 May 2023 17:58:16 -0700, Umesh Nerlige Ramappa wrote: >> >> >> > >> > One drive-by comment: >> > >> >> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c >> >> index 12b2f3169abf..284e5c5b97bb 100644 >> >> --- a/drivers/gpu/drm/i915/i915_pmu.c >> >> +++ b/drivers/gpu/drm/i915/i915_pmu.c >> >> @@ -546,8 +546,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; >> > >> > But in Patch 5 we have: >> > >> > #define I915_PMU_MAX_GTS (4) >> >> AFAIR that one is just to size the internal arrays, while max_gt_id is to >> report to userspace which events are present. > >Hmm, apart from the #defines's in i915_drm.h in Patch 5, not seeing >anything else reported to userspace about which events are present. Ex: We have only gt0 and gt1 on MTL. When user configures an event (sets event id, tile id etc on the config parameter) and calls the perf_event_open, it results in i915_pmu_event_init() -> config_status() which will return an ENOENT if the event was for say gt2 or gt3. This is for runtime check only. > >Also, we already have I915_MAX_GT, we shouldn't need I915_PMU_MAX_GTS, or >at least: > > #define I915_PMU_MAX_GTS I915_MAX_GT > >Better to use things uniformly. If we want I915_PMU_MAX_GTS to be 2 instead >of I915_MAX_GT (but why?, below is just a check) let's do > > #define I915_PMU_MAX_GTS 2 > >And use that in the code above. But I think we should just use I915_MAX_GT. Agree, Thanks, Umesh > >Thanks. >-- >Ashutosh > > >> > >> >> >> >> - if (gt_id) >> >> + if (gt_id > max_gt_id) >> >> return -ENOENT; >> >> >> >> switch (config_counter(config)) { >> >> @@ -561,6 +562,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)
On Fri, 12 May 2023 11:53:32 -0700, Umesh Nerlige Ramappa wrote: > > On Fri, May 12, 2023 at 10:08:58AM -0700, Dixit, Ashutosh wrote: > > On Fri, 12 May 2023 03:57:35 -0700, Tvrtko Ursulin wrote: > >> > >> > >> On 11/05/2023 19:57, Dixit, Ashutosh wrote: > >> > On Fri, 05 May 2023 17:58:16 -0700, Umesh Nerlige Ramappa wrote: > >> >> > >> > > >> > One drive-by comment: > >> > > >> >> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c > >> >> index 12b2f3169abf..284e5c5b97bb 100644 > >> >> --- a/drivers/gpu/drm/i915/i915_pmu.c > >> >> +++ b/drivers/gpu/drm/i915/i915_pmu.c > >> >> @@ -546,8 +546,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; > >> > > >> > But in Patch 5 we have: > >> > > >> > #define I915_PMU_MAX_GTS (4) > >> > >> AFAIR that one is just to size the internal arrays, while max_gt_id is to > >> report to userspace which events are present. > > > > Hmm, apart from the #defines's in i915_drm.h in Patch 5, not seeing > > anything else reported to userspace about which events are present. > > Ex: We have only gt0 and gt1 on MTL. When user configures an event (sets > event id, tile id etc on the config parameter) and calls the > perf_event_open, it results in i915_pmu_event_init() -> config_status() > which will return an ENOENT if the event was for say gt2 or gt3. This is > for runtime check only. Ah ok, sorry I missed that. In that case what we have above is fine. xe has a tile_count field but in i915 there's no easy way to find number of gt's, short of using for_each_gt() and incrementing a count. That seems like an overkill. So maybe what we have above is fine. Thanks. -- Ashutosh > > > > > Also, we already have I915_MAX_GT, we shouldn't need I915_PMU_MAX_GTS, or > > at least: > > > > #define I915_PMU_MAX_GTS I915_MAX_GT > > > > Better to use things uniformly. If we want I915_PMU_MAX_GTS to be 2 instead > > of I915_MAX_GT (but why?, below is just a check) let's do > > > > #define I915_PMU_MAX_GTS 2 > > > > And use that in the code above. But I think we should just use I915_MAX_GT. > > Agree, > > Thanks, > Umesh > > > > Thanks. > > -- > > Ashutosh > > > > > >> > > >> >> > >> >> - if (gt_id) > >> >> + if (gt_id > max_gt_id) > >> >> return -ENOENT; > >> >> > >> >> switch (config_counter(config)) { > >> >> @@ -561,6 +562,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)
diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c index 12b2f3169abf..284e5c5b97bb 100644 --- a/drivers/gpu/drm/i915/i915_pmu.c +++ b/drivers/gpu/drm/i915/i915_pmu.c @@ -546,8 +546,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)) { @@ -561,6 +562,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) @@ -930,11 +933,20 @@ 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), \ + .global = false, \ +} + +#define __global_event(__counter, __name, __unit) \ +{ \ + .counter = (__counter), \ + .name = (__name), \ + .unit = (__unit), \ + .global = true, \ } #define __engine_event(__sample, __name) \ @@ -973,15 +985,16 @@ 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; + 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"), - __event(I915_PMU_SOFTWARE_GT_AWAKE_TIME, "software-gt-awake-time", "ns"), + __event(0, "actual-frequency", "M"), + __event(1, "requested-frequency", "M"), + __global_event(2, "interrupts", NULL), + __event(3, "rc6-residency", "ns"), + __event(4, "software-gt-awake-time", "ns"), }; static const struct { enum drm_i915_pmu_engine_sample sample; @@ -996,12 +1009,17 @@ 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)) - count++; + 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++; + } } for_each_uabi_engine(engine, i915) { @@ -1031,26 +1049,39 @@ create_event_attributes(struct i915_pmu *pmu) attr_iter = attr; /* Initialize supported non-engine counters. */ - for (i = 0; i < ARRAY_SIZE(events); i++) { - char *str; - - if (config_status(i915, events[i].config)) - continue; - - str = kstrdup(events[i].name, GFP_KERNEL); - if (!str) - goto err; + 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; - *attr_iter++ = &i915_iter->attr.attr; - i915_iter = add_i915_attr(i915_iter, str, events[i].config); + if (config_status(i915, config)) + continue; - if (events[i].unit) { - str = kasprintf(GFP_KERNEL, "%s.unit", events[i].name); + if (events[i].global || !HAS_EXTRA_GT_LIST(i915)) + str = kstrdup(events[i].name, GFP_KERNEL); + else + str = kasprintf(GFP_KERNEL, "%s-gt%u", + events[i].name, j); if (!str) goto err; - *attr_iter++ = &pmu_iter->attr.attr; - pmu_iter = add_pmu_attr(pmu_iter, str, events[i].unit); + *attr_iter++ = &i915_iter->attr.attr; + i915_iter = add_i915_attr(i915_iter, str, config); + + if (events[i].unit) { + if (events[i].global || !HAS_EXTRA_GT_LIST(i915)) + str = kasprintf(GFP_KERNEL, "%s.unit", + events[i].name); + else + str = kasprintf(GFP_KERNEL, "%s-gt%u.unit", + events[i].name, j); + if (!str) + goto err; + + *attr_iter++ = &pmu_iter->attr.attr; + pmu_iter = add_pmu_attr(pmu_iter, str, + events[i].unit); + } } }