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

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

On Tue, May 16, 2023 at 10:24:45AM +0100, Tvrtko Ursulin wrote:
>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.
>
>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
>Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>---
>I am not entirely sure the __builtin_constant_p->BUILD_BUG_ON branch will
>work with all compilers. Lets see...
>
>Compile tested only.
>---
> drivers/gpu/drm/i915/i915_pmu.c | 32 ++++++++++++++++++++++----------
> 1 file changed, 22 insertions(+), 10 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
>index 7ece883a7d95..8736b3418f88 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);
> }
>@@ -82,15 +82,28 @@ static unsigned int other_bit(const u64 config)
>
> static unsigned int config_bit(const u64 config)
> {
>+	unsigned int bit;
>+
> 	if (is_engine_config(config))
>-		return engine_config_sample(config);
>+		bit = engine_config_sample(config);
> 	else
>-		return other_bit(config);
>+		bit = other_bit(config);
>+
>+	if (__builtin_constant_p(config))
>+		BUILD_BUG_ON(bit >
>+			     BITS_PER_TYPE(typeof_member(struct i915_pmu,
>+							 enable)) - 1);
>+	else
>+		WARN_ON_ONCE(bit >
>+			     BITS_PER_TYPE(typeof_member(struct i915_pmu,
>+							 enable)) - 1);

The else is firing for the INTERRUPT event because event_bit() also 
calls config_bit(). It would be best to move this check to config_mask() 
and leave this function as is.

Thanks,
Umesh 

>+
>+	return bit;
> }
>
>-static u64 config_mask(u64 config)
>+static u32 config_mask(const u64 config)
> {
>-	return BIT_ULL(config_bit(config));
>+	return BIT(config_bit(config));
> }
>
> static bool is_engine_event(struct perf_event *event)
>@@ -633,11 +646,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 +663,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 +710,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 +746,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.39.2
>