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

["Rogozhkin, Dmitry V" <dmitry.v.rogozhkin@intel.com>]

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.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx