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@xxxxxxxxx> 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@xxxxxxxxx> Cc: Ashutosh Dixit <ashutosh.dixit@xxxxxxxxx> Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@xxxxxxxxx> --- 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
