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 17:40:18 -0700, Andi Shyti wrote:
>

Hi Andi,

> On Thu, Jun 01, 2023 at 05:23:24PM -0700, Dixit, Ashutosh wrote:
> > 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.
>
> the number of GT's is a GPU concept and should remain as such all
> over the GPU. If max GT is 4 then it should be 4 everywhere.
>
> The I915_PMU_MAX_GTS define should not exist at all as it is
> creating this sort of inconsistencies and everything should refer
> to a single I915_MAX_GT. The reason for having I915_PMU_MAX_GTS,
> in a first place, is purely practical to avoid over inclusions.
> Still I consider it hacky.
>
> On the other had, already I915_MAX_GT is a hardcoded value and
> many times there have been discussions about removing it and
> fetch it dynamically during the i915 boot. But this requires
> quite a good amount of refactoring that no one is willing to do.
>
> If we can't get rid of I915_PMU_MAX_GTS then I strongly believe
> it should be aligned with I915_MAX_GT and for this reason I gave
> my r-b. The use of for_each_gt() is a clear consequence of this
> difference.

Yes, not disagreeing. At this point I think my preferred solution is
something like:

#define I915_MAX_GT 2
#define I915_PMU_MAX_GTS I915_MAX_GT

Unless someone can explain why I915_MAX_GT cannot be 2. As I see it,
there's no need for I915_MAX_GT to be 4 after xehpsdv disappeared and
support for future platforms is moving to xe.

Thanks.
--
Ashutosh