Re: [Intel-gfx] [PATCH v5 1/7] drm/i915/pmu: Change bitmask of enabled events to u32

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Tue, 16 May 2023 16:35:28 -0700, Umesh Nerlige Ramappa wrote:
>

Hi Umesh/Tvrtko,

Mostly repeating comments/questions made on the previous patch below.

> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Having it as u64 was a confusing (but harmless) mistake.
>
> Also add some asserts to make sure the internal field does not overflow
> in the future.
>
> v2: Fix WARN_ON firing for INTERRUPT event (Umesh)
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_pmu.c | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index 7ece883a7d95..96543dce2db1 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -50,7 +50,7 @@ static u8 engine_event_instance(struct perf_event *event)
>	return (event->attr.config >> I915_PMU_SAMPLE_BITS) & 0xff;
>  }
>
> -static bool is_engine_config(u64 config)
> +static bool is_engine_config(const u64 config)
>  {
>	return config < __I915_PMU_OTHER(0);
>  }
> @@ -88,9 +88,20 @@ static unsigned int config_bit(const u64 config)
>		return other_bit(config);
>  }
>
> -static u64 config_mask(u64 config)
> +static u32 config_mask(const u64 config)
>  {
> -	return BIT_ULL(config_bit(config));
> +	unsigned int bit = config_bit(config);

Give that config_bit() can return -1 (I understand it is avoided in moving
the code to config_mask from config_bit), maybe the code below should also
have that check?

	int bit = config_bit(config);

	if (bit != -1)
	{
		...
	}

Though as mentioned below the 'if (__builtin_constant_p())' would have to
go. Maybe the code could even have stayed in config_bit with the check.

> +
> +	if (__builtin_constant_p(config))
> +		BUILD_BUG_ON(bit >
> +			     BITS_PER_TYPE(typeof_member(struct i915_pmu,
> +							 enable)) - 1);

Given that config comes from the event (it is event->attr.config), can this
ever be a builtin constant?

> +	else
> +		WARN_ON_ONCE(bit >
> +			     BITS_PER_TYPE(typeof_member(struct i915_pmu,
> +							 enable)) - 1);

There is really an even stricter limit on what the bit can be, which is the
total number of possible events but anyway this is good enough.

After addressing the above, this patch is:

Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>

> +
> +	return BIT(config_bit(config));
>  }
>
>  static bool is_engine_event(struct perf_event *event)
> @@ -633,11 +644,10 @@ static void i915_pmu_enable(struct perf_event *event)
>  {
>	struct drm_i915_private *i915 =
>		container_of(event->pmu, typeof(*i915), pmu.base);
> +	const unsigned int bit = event_bit(event);
>	struct i915_pmu *pmu = &i915->pmu;
>	unsigned long flags;
> -	unsigned int bit;
>
> -	bit = event_bit(event);
>	if (bit == -1)
>		goto update;
>
> @@ -651,7 +661,7 @@ static void i915_pmu_enable(struct perf_event *event)
>	GEM_BUG_ON(bit >= ARRAY_SIZE(pmu->enable_count));
>	GEM_BUG_ON(pmu->enable_count[bit] == ~0);
>
> -	pmu->enable |= BIT_ULL(bit);
> +	pmu->enable |= BIT(bit);
>	pmu->enable_count[bit]++;
>
>	/*
> @@ -698,7 +708,7 @@ static void i915_pmu_disable(struct perf_event *event)
>  {
>	struct drm_i915_private *i915 =
>		container_of(event->pmu, typeof(*i915), pmu.base);
> -	unsigned int bit = event_bit(event);
> +	const unsigned int bit = event_bit(event);
>	struct i915_pmu *pmu = &i915->pmu;
>	unsigned long flags;
>
> @@ -734,7 +744,7 @@ static void i915_pmu_disable(struct perf_event *event)
>	 * bitmask when the last listener on an event goes away.
>	 */
>	if (--pmu->enable_count[bit] == 0) {
> -		pmu->enable &= ~BIT_ULL(bit);
> +		pmu->enable &= ~BIT(bit);
>		pmu->timer_enabled &= pmu_needs_timer(pmu, true);
>	}
>
> --
> 2.36.1
>