| Message ID | 20230531213547.1525692-1-matthew.s.atwood@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: sync I915_PMU_MAX_GTS to I915_MAX_GT | expand |
Hi Matt, On Wed, May 31, 2023 at 02:35:47PM -0700, Matt Atwood wrote: > Set I915_PMU_MAX_GTS to value in I915_MAX_GT, theres no reason for these > values to be different. > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> > Cc: Ashutosh Dixit <ashutosh.dixit@intel.com> > 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 right! Why not having #define I915_PMU_MAX_GTS I915_MAX_GT or... why having I915_PMU_MAX_GTS at all? Andi > /* > * How many different events we track in the global PMU mask. > -- > 2.40.0
On Wed, May 31, 2023 at 11:48:33PM +0200, Andi Shyti wrote: > Hi Matt, > > On Wed, May 31, 2023 at 02:35:47PM -0700, Matt Atwood wrote: > > Set I915_PMU_MAX_GTS to value in I915_MAX_GT, theres no reason for these > > values to be different. > > > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> > > Cc: Ashutosh Dixit <ashutosh.dixit@intel.com> > > 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 > > right! Why not having > > #define I915_PMU_MAX_GTS I915_MAX_GT > > or... why having I915_PMU_MAX_GTS at all? Originally I went the route of s/I915_PMU_MAX_GTS/I915_MAX_GT/g. However, this introduces many changes to where you then move I915_MAX_GT to and #include blocks in fiels that require these values. In the end I decided it was better to keep define and just change the value. > > Andi > > > /* > > * How many different events we track in the global PMU mask. > > -- > > 2.40.0 MattA
Hi Matt, On Wed, May 31, 2023 at 03:07:22PM -0700, Matt Atwood wrote: > On Wed, May 31, 2023 at 11:48:33PM +0200, Andi Shyti wrote: > > Hi Matt, > > > > On Wed, May 31, 2023 at 02:35:47PM -0700, Matt Atwood wrote: > > > Set I915_PMU_MAX_GTS to value in I915_MAX_GT, theres no reason for these > > > values to be different. > > > > > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> > > > Cc: Ashutosh Dixit <ashutosh.dixit@intel.com> > > > 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 > > > > right! Why not having > > > > #define I915_PMU_MAX_GTS I915_MAX_GT > > > > or... why having I915_PMU_MAX_GTS at all? > Originally I went the route of s/I915_PMU_MAX_GTS/I915_MAX_GT/g. > However, this introduces many changes to where you then move > I915_MAX_GT to and #include blocks in fiels that require these values. > In the end I decided it was better to keep define and just change the > value. OK, makes sense, then how about #define I915_PMU_MAX_GTS I915_MAX_GT i915_pmu.h has visibility on I915_MAX_GT. Andi > > > > Andi > > > > > /* > > > * How many different events we track in the global PMU mask. > > > -- > > > 2.40.0 > MattA
Hi Matt, > > > > --- 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 > > > > > > right! Why not having > > > > > > #define I915_PMU_MAX_GTS I915_MAX_GT > > > > > > or... why having I915_PMU_MAX_GTS at all? > > Originally I went the route of s/I915_PMU_MAX_GTS/I915_MAX_GT/g. > > However, this introduces many changes to where you then move > > I915_MAX_GT to and #include blocks in fiels that require these values. > > In the end I decided it was better to keep define and just change the > > value. > > OK, makes sense, then how about > > #define I915_PMU_MAX_GTS I915_MAX_GT > > i915_pmu.h has visibility on I915_MAX_GT. ops... it doesn't... sorry! Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> Andi > Andi > > > > > > > Andi > > > > > > > /* > > > > * How many different events we track in the global PMU mask. > > > > -- > > > > 2.40.0 > > MattA
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. > > 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 >
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. Thanks. -- 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 > >
On Thu, Jun 01, 2023 at 11:22:18AM -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. >> >> 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. I would leave it as 2 since we specifically changed it to 2 (was 4 earlier) during review of the PMU multi tile support patches. Thanks, Umesh > >Thanks. >-- >Ashutosh > >> >> /* >> * How many different events we track in the global PMU mask. >> -- >> 2.40.0 >>
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 > > >
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
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
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
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 /* * How many different events we track in the global PMU mask.
Set I915_PMU_MAX_GTS to value in I915_MAX_GT, theres no reason for these values to be different. Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> Cc: Ashutosh Dixit <ashutosh.dixit@intel.com> 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(-)