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@xxxxxxxxx>
> Cc: Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>
> Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@xxxxxxxxx>
> Cc: Ashutosh Dixit <ashutosh.dixit@xxxxxxxxx>
> Signed-off-by: Matt Atwood <matthew.s.atwood@xxxxxxxxx>
> ---
> 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
>
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
