Re: [Intel-gfx] [PATCH] drm/i915: sync I915_PMU_MAX_GTS to I915_MAX_GT

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

On Thu, 01 Jun 2023 11:30:44 -0700, Dixit, Ashutosh wrote:
>
> On Thu, 01 Jun 2023 11:22:18 -0700, Dixit, Ashutosh wrote:
> >
> > On Wed, 31 May 2023 14:35:47 -0700, Matt Atwood wrote:
> > >
> >
> > Hi Matt,
> >
> > > Set I915_PMU_MAX_GTS to value in I915_MAX_GT, theres no reason for these
> > > values to be different.
>
> Also, we can't be so sure so as to be able to say "theres no reason for
> these values to be different" till we have actually verified it. E.g. there
> are various bitfields in the code which might not fit in a u32 if we
> increase MAX_GT from 2 to 4. Has this been verified?
>
> If anything, to keep the code from doing unnecessary stuff, IMO I915_MAX_GT
> should be reduced to 2 and should be increased to 4 only once/if we have
> i915 supported platforms with 4 GT's.

Matt explained the issue offline to me (it would have helped to explain the
reason for the patch in the commit message). The issue is that in uses of
for_each_gt such as below (there are others too in the PMU code):

        for_each_gt(gt, i915, i) {
                intel_wakeref_t wakeref;

                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();
                }
        }

static checkers are complaining that for_each_gt can read/write outside the
bounds of PMU arrays. Because absent gt's will be NULL in for_each_gt this
cannot really happen but we still need to keep static checkers happy.

So to resolve this issue we need I915_PMU_MAX_GTS and I915_MAX_GT to have
the same value. So either we need to increase I915_PMU_MAX_GTS to 4 or
reduce I915_MAX_GT to 2.

Regards,
Ashutosh

>
>
> > >
> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> > > Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
> >
> > I don't believe the mailer actually Cc'd us. I just saw this and am Cc'ing
> > the people who authored/reviewed the previous series now.
> >
> > > Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_pmu.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
> > > index 33d80fbaab8b..aa929d8c224a 100644
> > > --- a/drivers/gpu/drm/i915/i915_pmu.h
> > > +++ b/drivers/gpu/drm/i915/i915_pmu.h
> > > @@ -38,7 +38,7 @@ enum {
> > >	__I915_NUM_PMU_SAMPLERS
> > >  };
> > >
> > > -#define I915_PMU_MAX_GTS 2
> > > +#define I915_PMU_MAX_GTS 4
> >
> > This was a discussed during the previous review and it was decided to keep
> > the two values (I915_PMU_MAX_GTS and I915_MAX_GT) different. There are
> > currently no platforms and there will be no i915 supported platforms with
> > MAX_GT 4. So I prefer to leave the values as they currently are. Unless
> > Umesh or Tvrtko agrees to this patch.
> >
> > Thanks.
> > --
> > Ashutosh
> >
> > >
> > >  /*
> > >   * How many different events we track in the global PMU mask.
> > > --
> > > 2.40.0
> > >