Re: [Intel-gfx] [PATCH 6/6] drm/i915/pmu: Export counters from all tiles

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

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)