Hi Ashutosh,
> > > > > > 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
#ifndef I915_MAX_GT
#define I915_MAX_GT 2
#endif
#define I915_PMU_MAX_GTS I915_MAX_GT
Side note: I915_PMU_MAX_GTS in plural is not the best of the
names as we don't really know what the 'S' stands for, is it
G.T.S. or GT's? MAX_GT is already intrinsically plural.
> 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.
Nothing wrong, you can try sending a patch and kick-start a
discussion, let's also see what CI thinks about.
Andi
