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)
