On Fri, 12 May 2023 13:57:59 -0700, Umesh Nerlige Ramappa wrote:
>
> On Fri, May 12, 2023 at 11:56:18AM +0100, Tvrtko Ursulin wrote:
> >
> > On 12/05/2023 02:08, Dixit, Ashutosh wrote:
> >> On Fri, 05 May 2023 17:58:15 -0700, Umesh Nerlige Ramappa wrote:
> >>>
> >>> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> >>>
> >>> Reserve some bits in the counter config namespace which will carry the
> >>> tile id and prepare the code to handle this.
> >>>
> >>> No per tile counters have been added yet.
> >>>
> >>> v2:
> >>> - Fix checkpatch issues
> >>> - Use 4 bits for gt id in non-engine counters. Drop FIXME.
> >>> - Set MAX GTs to 4. Drop FIXME.
> >>>
> >>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> >>> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@xxxxxxxxx>
> >>> ---
> >>> drivers/gpu/drm/i915/i915_pmu.c | 150 +++++++++++++++++++++++---------
> >>> drivers/gpu/drm/i915/i915_pmu.h | 9 +-
> >>> include/uapi/drm/i915_drm.h | 17 +++-
> >>> 3 files changed, 129 insertions(+), 47 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> >>> index 669a42e44082..12b2f3169abf 100644
> >>> --- a/drivers/gpu/drm/i915/i915_pmu.c
> >>> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> >>> @@ -56,11 +56,21 @@ static bool is_engine_config(u64 config)
> >>> return config < __I915_PMU_OTHER(0);
> >>> }
> >>>
> >>> +static unsigned int config_gt_id(const u64 config)
> >>> +{
> >>> + return config >> __I915_PMU_GT_SHIFT;
> >>> +}
> >>> +
> >>> +static u64 config_counter(const u64 config)
> >>> +{
> >>> + return config & ~(~0ULL << __I915_PMU_GT_SHIFT);
> >>
> >> ok, but another possibility:
> >>
> >> return config & ~REG_GENMASK64(63, __I915_PMU_GT_SHIFT);
> >
> > It's not a register so no. :) GENMASK_ULL maybe but meh.
>
> leaving as is.
>
> >
> >>> +}
> >>> +
> >>> static unsigned int other_bit(const u64 config)
> >>> {
> >>> unsigned int val;
> >>>
> >>> - switch (config) {
> >>> + switch (config_counter(config)) {
> >>> case I915_PMU_ACTUAL_FREQUENCY:
> >>> val = __I915_PMU_ACTUAL_FREQUENCY_ENABLED;
> >>> break;
> >>> @@ -78,15 +88,20 @@ static unsigned int other_bit(const u64 config)
> >>> return -1;
> >>> }
> >>>
> >>> - return I915_ENGINE_SAMPLE_COUNT + val;
> >>> + return I915_ENGINE_SAMPLE_COUNT +
> >>> + config_gt_id(config) * __I915_PMU_TRACKED_EVENT_COUNT +
> >>> + val;
> >>> }
> >>>
> >>> static unsigned int config_bit(const u64 config)
> >>> {
> >>> - if (is_engine_config(config))
> >>> + if (is_engine_config(config)) {
> >>> + GEM_BUG_ON(config_gt_id(config));
> >>
> >> This GEM_BUG_ON is not needed since:
> >>
> >> static bool is_engine_config(u64 config)
> >> {
> >> return config < __I915_PMU_OTHER(0);
> >> }
> >
> > True!
>
> dropping BUG_ON
>
> >
> >>> +
> >>> return engine_config_sample(config);
> >>> - else
> >>> + } else {
> >>> return other_bit(config);
> >>> + }
> >>> }
> >>>
> >>> static u64 config_mask(u64 config)
> >>> @@ -104,6 +119,18 @@ static unsigned int event_bit(struct perf_event *event)
> >>> return config_bit(event->attr.config);
> >>> }
> >>>
> >>> +static u64 frequency_enabled_mask(void)
> >>> +{
> >>> + unsigned int i;
> >>> + u64 mask = 0;
> >>> +
> >>> + for (i = 0; i < I915_PMU_MAX_GTS; i++)
> >>> + mask |= config_mask(__I915_PMU_ACTUAL_FREQUENCY(i)) |
> >>> + config_mask(__I915_PMU_REQUESTED_FREQUENCY(i));
> >>> +
> >>> + return mask;
> >>> +}
> >>> +
> >>> static bool pmu_needs_timer(struct i915_pmu *pmu, bool gpu_active)
> >>> {
> >>> struct drm_i915_private *i915 = container_of(pmu, typeof(*i915), pmu);
> >>> @@ -120,9 +147,7 @@ static bool pmu_needs_timer(struct i915_pmu *pmu, bool gpu_active)
> >>> * Mask out all the ones which do not need the timer, or in
> >>> * other words keep all the ones that could need the timer.
> >>> */
> >>> - enable &= config_mask(I915_PMU_ACTUAL_FREQUENCY) |
> >>> - config_mask(I915_PMU_REQUESTED_FREQUENCY) |
> >>> - ENGINE_SAMPLE_MASK;
> >>> + enable &= frequency_enabled_mask() | ENGINE_SAMPLE_MASK;
> >>
> >> u32 enable & u64 frequency_enabled_mask
> >>
> >> ugly but ok I guess? Or change enable to u64?
>
> making pmu->enable u64 as well as other places where it is assigned to
> local variables.
Yes, that's the way to do it.
>
> >
> > Hmm.. yes very ugly. Could have been an accident which happened to work
> > because there is a single timer (not per tile).
>
> Happened to work because the frequency mask does not spill over to the
> upper 32 bits (even for multi tile).
Even with 4 tiles, I checked.
>
> --------------------- START_SECTION ----------------
> >
> > Similar issue in frequency_sampling_enabled too. Gt_id argument to it
> > seems pointless.
>
> Not sure why it's pointless. We need the gt_id to determine the right mask
> for that specific gt. If it's not enabled, then we just return without
> pm_get and async put (like you mention later).
> And this piece of code is called within for_each_gt.
>
> >
> > So I now think whole frequency_enabled_mask() is just pointless and
> > should be removed. And then pmu_needs_time code can stay as is. Possibly
> > add a config_mask_32 helper which ensures no bits in upper 32 bits are
> > returned.
> >
> > That is if we are happy for the frequency_sampling_enabled returning true
> > for all gts, regardless of which ones actually have frequency sampling
> > enabled.
> >
> > Or if we want to implement it as I probably have intended, we will need
> > to add some gt bits into pmu->enable. Maybe reserve top four same as with
> > config counters.
>
> Nope. What you have here works just fine. pmu->enable should not include
> any gt id info. gt_id[63:60] is only a concept for pmu config sent by user.
> config_mask and pmu->enable are i915 internal bookkeeping (bit masks) just
> to track what events need to be sampled. The 'other' bit masks are a
> function of gt_id because we use gt_id to calculate a contiguous numerical
> value for these 'other' events. That's all. Once the numerical value is
> calculated, there is no need for gt_id because config_mask is
> BIT_ULL(numerical_value). Since the numerical values never exceeded 31
> (even for multi-gts), everything worked even with 32 bit pmu->enable.
Yeah, agree with Umesh, I also didn't follow what Tvrtko was saying here.
>
> >
> > In this case the config_mask needs to be updated to translate not just
> > the config counter into the pmu tracked event bits, but config counter gt
> > id into the pmu->enabled gt id.
> >
> > Sounds easily doable on a first thought.
>
> ------------------------ END_SECTION ----------------
>
>
> >>>
> >>> /*
> >>> * When the GPU is idle per-engine counters do not need to be
> >>> @@ -164,9 +189,37 @@ static inline s64 ktime_since_raw(const ktime_t kt)
> >>> return ktime_to_ns(ktime_sub(ktime_get_raw(), kt));
> >>> }
> >>>
> >>> +static unsigned int
> >>> +__sample_idx(struct i915_pmu *pmu, unsigned int gt_id, int sample)
> >>> +{
> >>> + unsigned int idx = gt_id * __I915_NUM_PMU_SAMPLERS + sample;
> >>> +
> >>> + GEM_BUG_ON(idx >= ARRAY_SIZE(pmu->sample));
> >>
> >> Does this GEM_BUG_ON need to be split up as follows:
> >>
> >> GEM_BUG_ON(gt_id >= I915_PMU_MAX_GTS);
> >> GEM_BUG_ON(sample >= __I915_NUM_PMU_SAMPLERS);
> >>
> >> Since that is what we really mean here isn't it?
> >
> > ARRAY_SIZE check seems the safest option to me, given it is defined as:
> >
> > sample[I915_PMU_MAX_GTS * __I915_NUM_PMU_SAMPLERS];
> >
> > What problem do you see here?
OK to leave as is.
> >
> >>> +
> >>> + return idx;
> >>> +}
> >>> +
> >>> +static u64 read_sample(struct i915_pmu *pmu, unsigned int gt_id, int sample)
> >>> +{
> >>> + return pmu->sample[__sample_idx(pmu, gt_id, sample)].cur;
> >>> +}
> >>> +
> >>> +static void
> >>> +store_sample(struct i915_pmu *pmu, unsigned int gt_id, int sample, u64 val)
> >>> +{
> >>> + pmu->sample[__sample_idx(pmu, gt_id, sample)].cur = val;
> >>> +}
> >>> +
> >>> +static void
> >>> +add_sample_mult(struct i915_pmu *pmu, unsigned int gt_id, int sample, u32 val, u32 mul)
> >>> +{
> >>> + pmu->sample[__sample_idx(pmu, gt_id, sample)].cur += mul_u32_u32(val, mul);
> >>> +}
> >>
> >> Gripe: I think this code should have per event data structures which store
> >> all information about a particular event. Rather than storing it in these
> >> arrays common to all events (and in bit-fields common to all events) which
> >> results in the kind of dance we have to do here. Anyway too big a change to
> >> make now but something to consider if we ever do this for xe.
> >
> > Could do a two dimensional array like:
> >
> > sample[I915_PMU_MAX_GTS][__I915_NUM_PMU_SAMPLERS];
> >
> > Any better? Honestly I don't remember if there was a special reason I
> > went for a flat array back then.
>
> Maybe we improve it in XE. I am looking for the shortest path to get this
> merged without any functional issues.
I like Tvrtko's 2-d array idea. Anyway Umesh you can leave this as is and I
will submit a follow on patch to fix this up.
>
> >
> >>
> >>> +
> >>> static u64 get_rc6(struct intel_gt *gt)
> >>> {
> >>> struct drm_i915_private *i915 = gt->i915;
> >>> + const unsigned int gt_id = gt->info.id;
> >>> struct i915_pmu *pmu = &i915->pmu;
> >>> unsigned long flags;
> >>> bool awake = false;
> >>> @@ -181,7 +234,7 @@ static u64 get_rc6(struct intel_gt *gt)
> >>> spin_lock_irqsave(&pmu->lock, flags);
> >>>
> >>> if (awake) {
> >>> - pmu->sample[__I915_SAMPLE_RC6].cur = val;
> >>> + store_sample(pmu, gt_id, __I915_SAMPLE_RC6, val);
> >>> } else {
> >>> /*
> >>> * We think we are runtime suspended.
> >>> @@ -190,14 +243,14 @@ static u64 get_rc6(struct intel_gt *gt)
> >>> * on top of the last known real value, as the approximated RC6
> >>> * counter value.
> >>> */
> >>> - val = ktime_since_raw(pmu->sleep_last);
> >>> - val += pmu->sample[__I915_SAMPLE_RC6].cur;
> >>> + val = ktime_since_raw(pmu->sleep_last[gt_id]);
> >>> + val += read_sample(pmu, gt_id, __I915_SAMPLE_RC6);
> >>> }
> >>>
> >>> - if (val < pmu->sample[__I915_SAMPLE_RC6_LAST_REPORTED].cur)
> >>> - val = pmu->sample[__I915_SAMPLE_RC6_LAST_REPORTED].cur;
> >>> + if (val < read_sample(pmu, gt_id, __I915_SAMPLE_RC6_LAST_REPORTED))
> >>> + val = read_sample(pmu, gt_id, __I915_SAMPLE_RC6_LAST_REPORTED);
> >>> else
> >>> - pmu->sample[__I915_SAMPLE_RC6_LAST_REPORTED].cur = val;
> >>> + store_sample(pmu, gt_id, __I915_SAMPLE_RC6_LAST_REPORTED, val);
> >>>
> >>> spin_unlock_irqrestore(&pmu->lock, flags);
> >>>
> >>> @@ -207,13 +260,20 @@ static u64 get_rc6(struct intel_gt *gt)
> >>> static void init_rc6(struct i915_pmu *pmu)
> >>> {
> >>> struct drm_i915_private *i915 = container_of(pmu, typeof(*i915), pmu);
> >>> - intel_wakeref_t wakeref;
> >>> + struct intel_gt *gt;
> >>> + unsigned int i;
> >>> +
> >>> + for_each_gt(gt, i915, i) {
> >>> + intel_wakeref_t wakeref;
> >>>
> >>> - with_intel_runtime_pm(to_gt(i915)->uncore->rpm, wakeref) {
> >>> - pmu->sample[__I915_SAMPLE_RC6].cur = __get_rc6(to_gt(i915));
> >>> - pmu->sample[__I915_SAMPLE_RC6_LAST_REPORTED].cur =
> >>> - pmu->sample[__I915_SAMPLE_RC6].cur;
> >>> - pmu->sleep_last = ktime_get_raw();
> >>> + with_intel_runtime_pm(gt->uncore->rpm, wakeref) {
> >>> + u64 val = __get_rc6(gt);
> >>> +
> >>> + store_sample(pmu, i, __I915_SAMPLE_RC6, val);
> >>> + store_sample(pmu, i, __I915_SAMPLE_RC6_LAST_REPORTED,
> >>> + val);
> >>> + pmu->sleep_last[i] = ktime_get_raw();
> >>> + }
> >>> }
> >>> }
> >>>
> >>> @@ -221,8 +281,8 @@ static void park_rc6(struct intel_gt *gt)
> >>> {
> >>> struct i915_pmu *pmu = >->i915->pmu;
> >>>
> >>> - pmu->sample[__I915_SAMPLE_RC6].cur = __get_rc6(gt);
> >>> - pmu->sleep_last = ktime_get_raw();
> >>> + store_sample(pmu, gt->info.id, __I915_SAMPLE_RC6, __get_rc6(gt));
> >>> + pmu->sleep_last[gt->info.id] = ktime_get_raw();
> >>> }
> >>>
> >>> static void __i915_pmu_maybe_start_timer(struct i915_pmu *pmu)
> >>> @@ -362,34 +422,30 @@ engines_sample(struct intel_gt *gt, unsigned int period_ns)
> >>> }
> >>> }
> >>>
> >>> -static void
> >>> -add_sample_mult(struct i915_pmu_sample *sample, u32 val, u32 mul)
> >>> -{
> >>> - sample->cur += mul_u32_u32(val, mul);
> >>> -}
> >>> -
> >>> -static bool frequency_sampling_enabled(struct i915_pmu *pmu)
> >>> +static bool
> >>> +frequency_sampling_enabled(struct i915_pmu *pmu, unsigned int gt)
> >>> {
> >>> return pmu->enable &
> >>> - (config_mask(I915_PMU_ACTUAL_FREQUENCY) |
> >>> - config_mask(I915_PMU_REQUESTED_FREQUENCY));
> >>> + (config_mask(__I915_PMU_ACTUAL_FREQUENCY(gt)) |
> >>> + config_mask(__I915_PMU_REQUESTED_FREQUENCY(gt)));
> >>
> >> Here again:
> >>
> >> u32 pmu->enable & u64 config_mask
> >>
> >> Probably ok?
> >>
> >> And also in i915_pmu_enable() we have:
> >>
> >> pmu->enable |= BIT_ULL(bit);
> >>
> >> So change pmu->enable to u64?
>
> Right, changing to u64
>
> >>
> >>> }
> >>>
> >>> static void
> >>> frequency_sample(struct intel_gt *gt, unsigned int period_ns)
> >>> {
> >>> struct drm_i915_private *i915 = gt->i915;
> >>> + const unsigned int gt_id = gt->info.id;
> >>> struct i915_pmu *pmu = &i915->pmu;
> >>> struct intel_rps *rps = >->rps;
> >>>
> >>> - if (!frequency_sampling_enabled(pmu))
> >>> + if (!frequency_sampling_enabled(pmu, gt_id))
> >>> return;
> >>
> >> Pre-existing issue, but why do we need this check? This is already checked
> >> in the two individual checks for actual and requested freq below:
> >>
> >> if (pmu->enable & config_mask(__I915_PMU_ACTUAL_FREQUENCY(gt_id)))
> >>
> >> and
> >>
> >> if (pmu->enable & config_mask(__I915_PMU_REQUESTED_FREQUENCY(gt_id)))
> >>
> >> So can we delete frequency_sampling_enabled()? Or is it there to avoid the
> >> overhead of intel_gt_pm_get_if_awake() (which doesn't seem to be much)?
> >
> > I think it was to avoid even getting an already active pm ref if
> > frequency events are not enabled. Timer could be running for instance if
> > only engine wait/sema is enabled. So yeah, just a little bit cheaper than
> > pm get + async put and avoid prolonging the delayed put for no
> > reason. (As the timer races with regular GT pm activities (see
> > mod_delayed_work in __intel_wakeref_put_last).)
>
> leaving as is.
>
> >
> >>
> >>>
> >>> /* Report 0/0 (actual/requested) frequency while parked. */
> >>> if (!intel_gt_pm_get_if_awake(gt))
> >>> return;
> >>>
> >>> - if (pmu->enable & config_mask(I915_PMU_ACTUAL_FREQUENCY)) {
> >>> + if (pmu->enable & config_mask(__I915_PMU_ACTUAL_FREQUENCY(gt_id))) {
> >>> u32 val;
> >>>
> >>> /*
> >>> @@ -405,12 +461,12 @@ frequency_sample(struct intel_gt *gt, unsigned int period_ns)
> >>> if (!val)
> >>> val = intel_gpu_freq(rps, rps->cur_freq);
> >>>
> >>> - add_sample_mult(&pmu->sample[__I915_SAMPLE_FREQ_ACT],
> >>> + add_sample_mult(pmu, gt_id, __I915_SAMPLE_FREQ_ACT,
> >>> val, period_ns / 1000);
> >>> }
> >>>
> >>> - if (pmu->enable & config_mask(I915_PMU_REQUESTED_FREQUENCY)) {
> >>> - add_sample_mult(&pmu->sample[__I915_SAMPLE_FREQ_REQ],
> >>> + if (pmu->enable & config_mask(__I915_PMU_REQUESTED_FREQUENCY(gt_id))) {
> >>> + add_sample_mult(pmu, gt_id, __I915_SAMPLE_FREQ_REQ,
> >>> intel_rps_get_requested_frequency(rps),
> >>> period_ns / 1000);
> >>> }
> >>> @@ -447,9 +503,7 @@ static enum hrtimer_restart i915_sample(struct hrtimer *hrtimer)
> >>> continue;
> >>>
> >>> engines_sample(gt, period_ns);
> >>> -
> >>> - if (i == 0) /* FIXME */
> >>> - frequency_sample(gt, period_ns);
> >>> + frequency_sample(gt, period_ns);
> >>> }
> >>>
> >>> hrtimer_forward(hrtimer, now, ns_to_ktime(PERIOD));
> >>> @@ -491,7 +545,12 @@ config_status(struct drm_i915_private *i915, u64 config)
> >>> {
> >>> struct intel_gt *gt = to_gt(i915);
> >>>
> >>> - switch (config) {
> >>> + unsigned int gt_id = config_gt_id(config);
> >>> +
> >>> + if (gt_id)
> >>> + return -ENOENT;
> >>
> >> This is just wrong. It is fixed in the next patch:
> >>
> >> if (gt_id > max_gt_id)
> >> return -ENOENT;
> >>
> >> But probably should be fixed in this patch itself. Or dropped from this
> >> patch and let it come in in Patch 6, since it's confusing. Though it
> >> probably belongs in this patch.
> >
> > Hmm my thinking was probably to reject gt > 0 in this patch since only
> > the last patch was supposed to be exposing the other tiles. Granted that
> > is not entirely true since this patch already makes access to them
> > available via i915_drm.h. Last patch only makes then discoverable via
> > sysfs.
> >
> > In this case yes, I'd pull in "gt_id > max_gt_id" into this patch. And
> > this hunk from the next patch too:
> >
> > case I915_PMU_INTERRUPTS:
> > + if (gt_id)
> > + return -ENOENT;
> >
>
> pulling in the above snippets from patch 6 to patch 5
>
> >>
> >>> +
> >>> + switch (config_counter(config)) {
> >>> case I915_PMU_ACTUAL_FREQUENCY:
> >>> if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915))
> >>> /* Requires a mutex for sampling! */
> >>> @@ -599,22 +658,27 @@ static u64 __i915_pmu_event_read(struct perf_event *event)
> >>> val = engine->pmu.sample[sample].cur;
> >>> }
> >>> } else {
> >>> - switch (event->attr.config) {
> >>> + const unsigned int gt_id = config_gt_id(event->attr.config);
> >>> + const u64 config = config_counter(event->attr.config);
> >>> +
> >>> + switch (config) {
> >>> case I915_PMU_ACTUAL_FREQUENCY:
> >>> val =
> >>> - div_u64(pmu->sample[__I915_SAMPLE_FREQ_ACT].cur,
> >>> + div_u64(read_sample(pmu, gt_id,
> >>> + __I915_SAMPLE_FREQ_ACT),
> >>> USEC_PER_SEC /* to MHz */);
> >>> break;
> >>> case I915_PMU_REQUESTED_FREQUENCY:
> >>> val =
> >>> - div_u64(pmu->sample[__I915_SAMPLE_FREQ_REQ].cur,
> >>> + div_u64(read_sample(pmu, gt_id,
> >>> + __I915_SAMPLE_FREQ_REQ),
> >>> USEC_PER_SEC /* to MHz */);
> >>> break;
> >>> case I915_PMU_INTERRUPTS:
> >>> val = READ_ONCE(pmu->irq_count);
> >>> break;
> >>> case I915_PMU_RC6_RESIDENCY:
> >>> - val = get_rc6(to_gt(i915));
> >>> + val = get_rc6(i915->gt[gt_id]);
> >>> break;
> >>> case I915_PMU_SOFTWARE_GT_AWAKE_TIME:
> >>> val = ktime_to_ns(intel_gt_get_awake_time(to_gt(i915)));
> >>> diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
> >>> index 3a811266ac6a..d47846f21ddf 100644
> >>> --- a/drivers/gpu/drm/i915/i915_pmu.h
> >>> +++ b/drivers/gpu/drm/i915/i915_pmu.h
> >>> @@ -38,13 +38,16 @@ enum {
> >>> __I915_NUM_PMU_SAMPLERS
> >>> };
> >>>
> >>> +#define I915_PMU_MAX_GTS (4)
> >>
> >> 4 or (4)? :-)
> >
> > Bike shed was strong with you on the day of review I see. :)
> >
> > I would rather get rid of this define altogether if we could use the
> > "normal" MAX_GT define. As I was saying earlier, I think this one was
> > here just because header dependencies were too convulted back then. Maybe
> > today things are better? Worth I try probably.
>
> dropping this and using I915_MAX_GTS
>
> >
> >>
> >>> +
> >>> /*
> >>> * How many different events we track in the global PMU mask.
> >>> *
> >>> * It is also used to know to needed number of event reference counters.
> >>> */
> >>> #define I915_PMU_MASK_BITS \
> >>> - (I915_ENGINE_SAMPLE_COUNT + __I915_PMU_TRACKED_EVENT_COUNT)
> >>> + (I915_ENGINE_SAMPLE_COUNT + \
> >>> + I915_PMU_MAX_GTS * __I915_PMU_TRACKED_EVENT_COUNT)
> >>>
> >>> #define I915_ENGINE_SAMPLE_COUNT (I915_SAMPLE_SEMA + 1)
> >>>
> >>> @@ -124,11 +127,11 @@ struct i915_pmu {
> >>> * Only global counters are held here, while the per-engine ones are in
> >>> * struct intel_engine_cs.
> >>> */
> >>> - struct i915_pmu_sample sample[__I915_NUM_PMU_SAMPLERS];
> >>> + struct i915_pmu_sample sample[I915_PMU_MAX_GTS * __I915_NUM_PMU_SAMPLERS];
> >>> /**
> >>> * @sleep_last: Last time GT parked for RC6 estimation.
> >>> */
> >>> - ktime_t sleep_last;
> >>> + ktime_t sleep_last[I915_PMU_MAX_GTS];
> >>> /**
> >>> * @irq_count: Number of interrupts
> >>> *
> >>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> >>> index dba7c5a5b25e..d5ac1fdeb2b1 100644
> >>> --- a/include/uapi/drm/i915_drm.h
> >>> +++ b/include/uapi/drm/i915_drm.h
> >>> @@ -280,7 +280,16 @@ enum drm_i915_pmu_engine_sample {
> >>> #define I915_PMU_ENGINE_SEMA(class, instance) \
> >>> __I915_PMU_ENGINE(class, instance, I915_SAMPLE_SEMA)
> >>>
> >>> -#define __I915_PMU_OTHER(x) (__I915_PMU_ENGINE(0xff, 0xff, 0xf) + 1 + (x))
> >>> +/*
> >>> + * Top 4 bits of every non-engine counter are GT id.
> >>> + */
> >>> +#define __I915_PMU_GT_SHIFT (60)
> >>
> >> REG_GENMASK64 or GENMASK_ULL would be nicer but of course we can't put in
> >> the uapi header, so ok.
> >
> > Yep.
>
> leaving as is.
>
> >
> >>> +
> >>> +#define ___I915_PMU_OTHER(gt, x) \
> >>> + (((__u64)__I915_PMU_ENGINE(0xff, 0xff, 0xf) + 1 + (x)) | \
> >>> + ((__u64)(gt) << __I915_PMU_GT_SHIFT))
> >>> +
> >>> +#define __I915_PMU_OTHER(x) ___I915_PMU_OTHER(0, x)
> >>>
> >>> #define I915_PMU_ACTUAL_FREQUENCY __I915_PMU_OTHER(0)
> >>> #define I915_PMU_REQUESTED_FREQUENCY __I915_PMU_OTHER(1)
> >>> @@ -290,6 +299,12 @@ enum drm_i915_pmu_engine_sample {
> >>>
> >>> #define I915_PMU_LAST /* Deprecated - do not use */ I915_PMU_RC6_RESIDENCY
> >>>
> >>> +#define __I915_PMU_ACTUAL_FREQUENCY(gt) ___I915_PMU_OTHER(gt, 0)
> >>> +#define __I915_PMU_REQUESTED_FREQUENCY(gt) ___I915_PMU_OTHER(gt, 1)
> >>> +#define __I915_PMU_INTERRUPTS(gt) ___I915_PMU_OTHER(gt, 2)
> >>> +#define __I915_PMU_RC6_RESIDENCY(gt) ___I915_PMU_OTHER(gt, 3)
> >>> +#define __I915_PMU_SOFTWARE_GT_AWAKE_TIME(gt) ___I915_PMU_OTHER(gt, 4)
> >>> +
> >>> /* Each region is a minimum of 16k, and there are at most 255 of them.
> >>> */
> >>> #define I915_NR_TEX_REGIONS 255 /* table size 2k - maximum due to use
> >>> --
> >>> 2.36.1
> >>>
> >>
> >> Above comments are mostly nits so after addressing the above comments, this
> >> is:
> >>
> >> Reviewed-by: Ashutosh Dixit <ashutosh.dixit@xxxxxxxxx>
> >
> > Well you found some ugly bits (or I got confused, double check me please)
> > so I'd say hold off with r-b just yet. Sadly it's on Umesh now to fix up
> > my mess. :I
>
> I don't see anything wrong with the SECTION I marked above. As in, the
> pmu_needs_timer and the sampling code for events that need to be
> sampled. If you agree, I can spin the next revision.
So overall I am fine with Umesh's proposal mentioned above and Umesh you
can keep my R-b. So if Tvrtko is also ok please spin the next rev.
Thanks.
--
Ashutosh
