| Message ID | 20230602214022.1583682-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 |
On Fri, Jun 02, 2023 at 02:40:22PM -0700, Matt Atwood wrote: > Set I915_PMU_MAX_GTS to value in I915_MAX_GT, theres no reason for these > values to be different. > > v2: Change name of I915_PMU_MAX_GTS to I915_PMU_MAX_GT (Andi), Change > I915_MAX_GT from 4 to 2 (Ashutosh), Explicitly set I915_PMU_MAX_GT to > I915_MAX_GT (Andi) There are too many things combined into one patch here. The headline and commit message make it sound like we're just changing a PMU-specific define, but now the real functional change is that you're changing the value of I915_MAX_GT, which could affect all the code outside of PMU. Renaming I915_PMU_MAX_GTS to I915_PMU_MAX_GT is a general cleanup with no functional change, so that should be a patch by itself. Changing the value of I915_MAX_GT is a completely different change (that does have a functional impact) and should be a separate patch. > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Andi Shyti <andi.shyti@linux.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_drv.h | 2 +- > drivers/gpu/drm/i915/i915_pmu.c | 2 +- > drivers/gpu/drm/i915/i915_pmu.h | 12 ++++++++---- > 3 files changed, 10 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index f1205ed3ba71..c3923f52ca56 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -314,7 +314,7 @@ struct drm_i915_private { > /* > * i915->gt[0] == &i915->gt0 > */ > -#define I915_MAX_GT 4 > +#define I915_MAX_GT 2 > struct intel_gt *gt[I915_MAX_GT]; > > struct kobject *sysfs_gt; > diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c > index f96fe92dca4e..d35973b41186 100644 > --- a/drivers/gpu/drm/i915/i915_pmu.c > +++ b/drivers/gpu/drm/i915/i915_pmu.c > @@ -132,7 +132,7 @@ static u32 frequency_enabled_mask(void) > unsigned int i; > u32 mask = 0; > > - for (i = 0; i < I915_PMU_MAX_GTS; i++) > + for (i = 0; i < I915_PMU_MAX_GT; i++) > mask |= config_mask(__I915_PMU_ACTUAL_FREQUENCY(i)) | > config_mask(__I915_PMU_REQUESTED_FREQUENCY(i)); > > diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h > index d20592e7db99..260a39aaa06b 100644 > --- a/drivers/gpu/drm/i915/i915_pmu.h > +++ b/drivers/gpu/drm/i915/i915_pmu.h > @@ -38,7 +38,11 @@ enum { > __I915_NUM_PMU_SAMPLERS > }; > > -#define I915_PMU_MAX_GTS 2 > +#ifndef I915_MAX_GT > +#define I915_MAX_GT 2 > +#endif > + > +#define I915_PMU_MAX_GT I915_MAX_GT It seems like this just makes the code harder to understand. i915's header include spaghetti makes it very difficult to see easily whether I915_MAX_GT would actually be visible here or not. If the other definition of I915_MAX_GT gets changed in the future, then the value here may or may not stay in sync (and may even behave inconsistently when included from different .c files). It's probably better to either leave this explicitly defined as 2 for now to avoid any ambiguity about its value, or move the single definition of I915_MAX_GT somewhere that can be used more easily everywhere. The display code has an intel_display_limits.h and it might be worth considering something similar for GT-related enums and limits. Matt > > /* > * How many different events we track in the global PMU mask. > @@ -47,7 +51,7 @@ enum { > */ > #define I915_PMU_MASK_BITS \ > (I915_ENGINE_SAMPLE_COUNT + \ > - I915_PMU_MAX_GTS * __I915_PMU_TRACKED_EVENT_COUNT) > + I915_PMU_MAX_GT * __I915_PMU_TRACKED_EVENT_COUNT) > > #define I915_ENGINE_SAMPLE_COUNT (I915_SAMPLE_SEMA + 1) > > @@ -127,11 +131,11 @@ struct i915_pmu { > * Only global counters are held here, while the per-engine ones are in > * struct intel_engine_cs. > */ > - struct i915_pmu_sample sample[I915_PMU_MAX_GTS][__I915_NUM_PMU_SAMPLERS]; > + struct i915_pmu_sample sample[I915_PMU_MAX_GT][__I915_NUM_PMU_SAMPLERS]; > /** > * @sleep_last: Last time GT parked for RC6 estimation. > */ > - ktime_t sleep_last[I915_PMU_MAX_GTS]; > + ktime_t sleep_last[I915_PMU_MAX_GT]; > /** > * @irq_count: Number of interrupts > * > -- > 2.40.0 >
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index f1205ed3ba71..c3923f52ca56 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -314,7 +314,7 @@ struct drm_i915_private { /* * i915->gt[0] == &i915->gt0 */ -#define I915_MAX_GT 4 +#define I915_MAX_GT 2 struct intel_gt *gt[I915_MAX_GT]; struct kobject *sysfs_gt; diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c index f96fe92dca4e..d35973b41186 100644 --- a/drivers/gpu/drm/i915/i915_pmu.c +++ b/drivers/gpu/drm/i915/i915_pmu.c @@ -132,7 +132,7 @@ static u32 frequency_enabled_mask(void) unsigned int i; u32 mask = 0; - for (i = 0; i < I915_PMU_MAX_GTS; i++) + for (i = 0; i < I915_PMU_MAX_GT; i++) mask |= config_mask(__I915_PMU_ACTUAL_FREQUENCY(i)) | config_mask(__I915_PMU_REQUESTED_FREQUENCY(i)); diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h index d20592e7db99..260a39aaa06b 100644 --- a/drivers/gpu/drm/i915/i915_pmu.h +++ b/drivers/gpu/drm/i915/i915_pmu.h @@ -38,7 +38,11 @@ enum { __I915_NUM_PMU_SAMPLERS }; -#define I915_PMU_MAX_GTS 2 +#ifndef I915_MAX_GT +#define I915_MAX_GT 2 +#endif + +#define I915_PMU_MAX_GT I915_MAX_GT /* * How many different events we track in the global PMU mask. @@ -47,7 +51,7 @@ enum { */ #define I915_PMU_MASK_BITS \ (I915_ENGINE_SAMPLE_COUNT + \ - I915_PMU_MAX_GTS * __I915_PMU_TRACKED_EVENT_COUNT) + I915_PMU_MAX_GT * __I915_PMU_TRACKED_EVENT_COUNT) #define I915_ENGINE_SAMPLE_COUNT (I915_SAMPLE_SEMA + 1) @@ -127,11 +131,11 @@ struct i915_pmu { * Only global counters are held here, while the per-engine ones are in * struct intel_engine_cs. */ - struct i915_pmu_sample sample[I915_PMU_MAX_GTS][__I915_NUM_PMU_SAMPLERS]; + struct i915_pmu_sample sample[I915_PMU_MAX_GT][__I915_NUM_PMU_SAMPLERS]; /** * @sleep_last: Last time GT parked for RC6 estimation. */ - ktime_t sleep_last[I915_PMU_MAX_GTS]; + ktime_t sleep_last[I915_PMU_MAX_GT]; /** * @irq_count: Number of interrupts *
Set I915_PMU_MAX_GTS to value in I915_MAX_GT, theres no reason for these values to be different. v2: Change name of I915_PMU_MAX_GTS to I915_PMU_MAX_GT (Andi), Change I915_MAX_GT from 4 to 2 (Ashutosh), Explicitly set I915_PMU_MAX_GT to I915_MAX_GT (Andi) Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Andi Shyti <andi.shyti@linux.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_drv.h | 2 +- drivers/gpu/drm/i915/i915_pmu.c | 2 +- drivers/gpu/drm/i915/i915_pmu.h | 12 ++++++++---- 3 files changed, 10 insertions(+), 6 deletions(-)