Re: [RFC v7 04/11] drm/i915/pmu: Expose a PMU interface for perf queries

Tvrtko Ursulin
2017-09-15T07:57:11Z

On 15/09/2017 01:00, Rogozhkin, Dmitry V wrote:
> On Wed, 2017-09-13 at 11:34 +0100, Tvrtko Ursulin wrote:
>> +static int i915_pmu_event_init(struct perf_event *event)
>> +{
>> + struct drm_i915_private *i915 =
>> + container_of(event->pmu, typeof(*i915), pmu.base);
>> + int cpu, ret;
>> +
>> + if (event->attr.type != event->pmu->type)
>> + return -ENOENT;
>> +
>> + /* unsupported modes and filters */
>> + if (event->attr.sample_period) /* no sampling */
>> + return -EINVAL;
>> +
>> + if (has_branch_stack(event))
>> + return -EOPNOTSUPP;
>> +
>> + if (event->cpu < 0)
>> + return -EINVAL;
>> +
>> + cpu = cpumask_any_and(&i915_pmu_cpumask,
>> + topology_sibling_cpumask(event->cpu));
>> + if (cpu >= nr_cpu_ids)
>> + return -ENODEV;
>> +
>> + ret = 0;
>> + if (is_engine_event(event)) {
>> + ret = engine_event_init(event);
>> + } else switch (event->attr.config) {
>> + case I915_PMU_ACTUAL_FREQUENCY:
>> + if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915))
>> + ret = -ENODEV; /* requires a mutex for
>> sampling! */
>> + case I915_PMU_REQUESTED_FREQUENCY:
>> + case I915_PMU_ENERGY:
>> + case I915_PMU_RC6_RESIDENCY:
>> + case I915_PMU_RC6p_RESIDENCY:
>> + case I915_PMU_RC6pp_RESIDENCY:
>> + if (INTEL_GEN(i915) < 6)
>> + ret = -ENODEV;
>> + break;
>> + }
>> + if (ret)
>> + return ret;
>
> The switch for non-engine events should error out by default:
>
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c
> b/drivers/gpu/drm/i915/i915_pmu.c
> index d734879..3145e9a 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -329,6 +329,9 @@ static int i915_pmu_event_init(struct perf_event
> *event)
> if (INTEL_GEN(i915) < 6)
> ret = -ENODEV;
> break;
> + default:
> + ret = -ENOENT;
> + break;
> }
> if (ret)
> return ret;
>
>
> Otherwise user may try to enable non-existing metric (> I915_PMU_LAST)
> and eventually will be subject to kernel panic on
> i915_pmu_enable/disable during refcount operations. And we need to have
> an IGT test to check that.

Well spotted, I'll add the test for this. I am working on tests at the
moment anyway. It is taking a bit longer than expected due finding
issues, not limited to this one.

Regards,

Tvrtko