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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux