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 03:13:01PM -0700, Umesh Nerlige Ramappa wrote:
>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.

I posted the modified version here - 
https://patchwork.freedesktop.org/patch/537361/?series=117843&rev=1 as 
part of the MTL PMU series so that it Tests out with IGT patches.

Thanks,
Umesh

>
>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
>>