Commit d8b879bb authored by Tvrtko Ursulin's avatar Tvrtko Ursulin Committed by Jani Nikula

drm/i915/pmu: Fix enable count array size and bounds checking

Enable count array is supposed to have one counter for each possible
engine sampler. As such, array sizing and bounds checking is not correct
and would blow up the asserts if more samplers were added.

No ill-effect in the current code base but lets fix it for correctness.

At the same time tidy the assert for readability and robustness.

v2:
 * One check per assert. (Chris Wilson)
Signed-off-by: default avatarTvrtko Ursulin <tvrtko.ursulin@intel.com>
Fixes: b46a33e2 ("drm/i915/pmu: Expose a PMU interface for perf queries")
Reviewed-by: default avatarChris Wilson <chris@chris-wilson.co.uk>
Link: https://patchwork.freedesktop.org/patch/msgid/20190205130353.21105-1-tvrtko.ursulin@linux.intel.com
(cherry picked from commit 26a11dee)
Signed-off-by: default avatarJani Nikula <jani.nikula@intel.com>
parent 3b91a935
...@@ -594,7 +594,8 @@ static void i915_pmu_enable(struct perf_event *event) ...@@ -594,7 +594,8 @@ static void i915_pmu_enable(struct perf_event *event)
* Update the bitmask of enabled events and increment * Update the bitmask of enabled events and increment
* the event reference counter. * the event reference counter.
*/ */
GEM_BUG_ON(bit >= I915_PMU_MASK_BITS); BUILD_BUG_ON(ARRAY_SIZE(i915->pmu.enable_count) != I915_PMU_MASK_BITS);
GEM_BUG_ON(bit >= ARRAY_SIZE(i915->pmu.enable_count));
GEM_BUG_ON(i915->pmu.enable_count[bit] == ~0); GEM_BUG_ON(i915->pmu.enable_count[bit] == ~0);
i915->pmu.enable |= BIT_ULL(bit); i915->pmu.enable |= BIT_ULL(bit);
i915->pmu.enable_count[bit]++; i915->pmu.enable_count[bit]++;
...@@ -615,11 +616,16 @@ static void i915_pmu_enable(struct perf_event *event) ...@@ -615,11 +616,16 @@ static void i915_pmu_enable(struct perf_event *event)
engine = intel_engine_lookup_user(i915, engine = intel_engine_lookup_user(i915,
engine_event_class(event), engine_event_class(event),
engine_event_instance(event)); engine_event_instance(event));
GEM_BUG_ON(!engine);
engine->pmu.enable |= BIT(sample);
GEM_BUG_ON(sample >= I915_PMU_SAMPLE_BITS); BUILD_BUG_ON(ARRAY_SIZE(engine->pmu.enable_count) !=
I915_ENGINE_SAMPLE_COUNT);
BUILD_BUG_ON(ARRAY_SIZE(engine->pmu.sample) !=
I915_ENGINE_SAMPLE_COUNT);
GEM_BUG_ON(sample >= ARRAY_SIZE(engine->pmu.enable_count));
GEM_BUG_ON(sample >= ARRAY_SIZE(engine->pmu.sample));
GEM_BUG_ON(engine->pmu.enable_count[sample] == ~0); GEM_BUG_ON(engine->pmu.enable_count[sample] == ~0);
engine->pmu.enable |= BIT(sample);
engine->pmu.enable_count[sample]++; engine->pmu.enable_count[sample]++;
} }
...@@ -649,9 +655,11 @@ static void i915_pmu_disable(struct perf_event *event) ...@@ -649,9 +655,11 @@ static void i915_pmu_disable(struct perf_event *event)
engine = intel_engine_lookup_user(i915, engine = intel_engine_lookup_user(i915,
engine_event_class(event), engine_event_class(event),
engine_event_instance(event)); engine_event_instance(event));
GEM_BUG_ON(!engine);
GEM_BUG_ON(sample >= I915_PMU_SAMPLE_BITS); GEM_BUG_ON(sample >= ARRAY_SIZE(engine->pmu.enable_count));
GEM_BUG_ON(sample >= ARRAY_SIZE(engine->pmu.sample));
GEM_BUG_ON(engine->pmu.enable_count[sample] == 0); GEM_BUG_ON(engine->pmu.enable_count[sample] == 0);
/* /*
* Decrement the reference count and clear the enabled * Decrement the reference count and clear the enabled
* bitmask when the last listener on an event goes away. * bitmask when the last listener on an event goes away.
...@@ -660,7 +668,7 @@ static void i915_pmu_disable(struct perf_event *event) ...@@ -660,7 +668,7 @@ static void i915_pmu_disable(struct perf_event *event)
engine->pmu.enable &= ~BIT(sample); engine->pmu.enable &= ~BIT(sample);
} }
GEM_BUG_ON(bit >= I915_PMU_MASK_BITS); GEM_BUG_ON(bit >= ARRAY_SIZE(i915->pmu.enable_count));
GEM_BUG_ON(i915->pmu.enable_count[bit] == 0); GEM_BUG_ON(i915->pmu.enable_count[bit] == 0);
/* /*
* Decrement the reference count and clear the enabled * Decrement the reference count and clear the enabled
......
...@@ -31,6 +31,8 @@ enum { ...@@ -31,6 +31,8 @@ enum {
((1 << I915_PMU_SAMPLE_BITS) + \ ((1 << I915_PMU_SAMPLE_BITS) + \
(I915_PMU_LAST + 1 - __I915_PMU_OTHER(0))) (I915_PMU_LAST + 1 - __I915_PMU_OTHER(0)))
#define I915_ENGINE_SAMPLE_COUNT (I915_SAMPLE_SEMA + 1)
struct i915_pmu_sample { struct i915_pmu_sample {
u64 cur; u64 cur;
}; };
......
...@@ -415,16 +415,17 @@ struct intel_engine_cs { ...@@ -415,16 +415,17 @@ struct intel_engine_cs {
/** /**
* @enable_count: Reference count for the enabled samplers. * @enable_count: Reference count for the enabled samplers.
* *
* Index number corresponds to the bit number from @enable. * Index number corresponds to @enum drm_i915_pmu_engine_sample.
*/ */
unsigned int enable_count[I915_PMU_SAMPLE_BITS]; unsigned int enable_count[I915_ENGINE_SAMPLE_COUNT];
/** /**
* @sample: Counter values for sampling events. * @sample: Counter values for sampling events.
* *
* Our internal timer stores the current counters in this field. * Our internal timer stores the current counters in this field.
*
* Index number corresponds to @enum drm_i915_pmu_engine_sample.
*/ */
#define I915_ENGINE_SAMPLE_MAX (I915_SAMPLE_SEMA + 1) struct i915_pmu_sample sample[I915_ENGINE_SAMPLE_COUNT];
struct i915_pmu_sample sample[I915_ENGINE_SAMPLE_MAX];
} pmu; } pmu;
/* /*
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment