Quoting Tvrtko Ursulin (2019-02-05 10:29:05)
> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
>
> 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.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> Fixes: b46a33e271ed ("drm/i915/pmu: Expose a PMU interface for perf queries")
> ---
> drivers/gpu/drm/i915/i915_pmu.c | 22 +++++++++++++++-------
> drivers/gpu/drm/i915/i915_pmu.h | 2 ++
> drivers/gpu/drm/i915/intel_ringbuffer.h | 9 +++++----
> 3 files changed, 22 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index b1cb2d3cae16..44a14ef1035c 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -599,7 +599,8 @@ static void i915_pmu_enable(struct perf_event *event)
> * Update the bitmask of enabled events and increment
> * 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); /* !overflow! */
Took me a moment to realise what you were checking. I was thinking
around the lines of an enable mask before I saw the count.
> i915->pmu.enable |= BIT_ULL(bit);
> i915->pmu.enable_count[bit]++;
> @@ -620,11 +621,16 @@ static void i915_pmu_enable(struct perf_event *event)
> engine = intel_engine_lookup_user(i915,
> engine_event_class(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 ||
> + ARRAY_SIZE(engine->pmu.sample) !=
> + I915_ENGINE_SAMPLE_COUNT);
Ugh, a bit of a mouthful. Split into two at least it is less of a
towering cliff (and will be easier to understand should it ever fire).
Can apply the same rationale to split up the GEM_BUG_ON.
Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
