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

[Andi Shyti]

Hi Ashutosh,

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.

Thanks for chiming in,
Andi