Re: [Intel-gfx] [PATCH v2] drm/i915/pmu: Only enumerate available counters in sysfs

2018-01-09 Thread Chris Wilson
Quoting Tvrtko Ursulin (2018-01-09 12:20:05)
> From: Tvrtko Ursulin 
> 
> Switch over to dynamically creating device attributes, which are in turn
> used by the perf core to expose available counters in sysfs.
> 
> This way we do not expose counters which are not avaiable on the current
> platform, and are so more consistent between what we reply to open
> attempts via the perf_event_open(2), and what is discoverable in sysfs.
> 
> v2:
>  * Simplify attribute pointer freeing loop.
>  * Changed attr init from macro to function.
>  * More common error unwind. (Chris Wilson)
>  * Rename some locals. (Chris Wilson)
> 
> Signed-off-by: Tvrtko Ursulin 
> Cc: Chris Wilson 
> ---
> @@ -337,30 +372,10 @@ static int i915_pmu_event_init(struct perf_event *event)
> if (!cpumask_test_cpu(event->cpu, &i915_pmu_cpumask))
> return -EINVAL;
>  
> -   if (is_engine_event(event)) {
> +   if (is_engine_event(event))
> ret = engine_event_init(event);
> -   } else {
> -   ret = 0;
> -   switch (event->attr.config) {
> -   case I915_PMU_ACTUAL_FREQUENCY:
> -   if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915))
> -/* Requires a mutex for sampling! */
> -   ret = -ENODEV;
> -   case I915_PMU_REQUESTED_FREQUENCY:
> -   if (INTEL_GEN(i915) < 6)
> -   ret = -ENODEV;
> -   break;
> -   case I915_PMU_INTERRUPTS:
> -   break;
> -   case I915_PMU_RC6_RESIDENCY:
> -   if (!HAS_RC6(i915))
> -   ret = -ENODEV;
> -   break;
> -   default:
> -   ret = -ENOENT;
> -   break;
> -   }
> -   }
> +   else
> +   ret = config_status(i915, event->attr.config);;

Found something! s/;;/;/

It was fairly readable, keeping the sysfs clean seems a reasonable
goal, and I couldn't find anything significant to complain about,

Reviewed-by: Chris Wilson 
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


[Intel-gfx] [PATCH v2] drm/i915/pmu: Only enumerate available counters in sysfs

2018-01-09 Thread Tvrtko Ursulin
From: Tvrtko Ursulin 

Switch over to dynamically creating device attributes, which are in turn
used by the perf core to expose available counters in sysfs.

This way we do not expose counters which are not avaiable on the current
platform, and are so more consistent between what we reply to open
attempts via the perf_event_open(2), and what is discoverable in sysfs.

v2:
 * Simplify attribute pointer freeing loop.
 * Changed attr init from macro to function.
 * More common error unwind. (Chris Wilson)
 * Rename some locals. (Chris Wilson)

Signed-off-by: Tvrtko Ursulin 
Cc: Chris Wilson 
---
 drivers/gpu/drm/i915/i915_pmu.c | 321 ++--
 drivers/gpu/drm/i915/i915_pmu.h |   8 +
 2 files changed, 251 insertions(+), 78 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 55a8a1e29424..43ad990123a0 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -290,23 +290,44 @@ static void i915_pmu_event_destroy(struct perf_event 
*event)
WARN_ON(event->parent);
 }
 
-static int engine_event_init(struct perf_event *event)
+static int
+engine_event_status(struct intel_engine_cs *engine,
+   enum drm_i915_pmu_engine_sample sample)
 {
-   struct drm_i915_private *i915 =
-   container_of(event->pmu, typeof(*i915), pmu.base);
-
-   if (!intel_engine_lookup_user(i915, engine_event_class(event),
- engine_event_instance(event)))
-   return -ENODEV;
-
-   switch (engine_event_sample(event)) {
+   switch (sample) {
case I915_SAMPLE_BUSY:
case I915_SAMPLE_WAIT:
break;
case I915_SAMPLE_SEMA:
+   if (INTEL_GEN(engine->i915) < 6)
+   return -ENODEV;
+   break;
+   default:
+   return -ENOENT;
+   }
+
+   return 0;
+}
+
+static int
+config_status(struct drm_i915_private *i915, u64 config)
+{
+   switch (config) {
+   case I915_PMU_ACTUAL_FREQUENCY:
+   if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915))
+   /* Requires a mutex for sampling! */
+   return -ENODEV;
+   /* Fall-through. */
+   case I915_PMU_REQUESTED_FREQUENCY:
if (INTEL_GEN(i915) < 6)
return -ENODEV;
break;
+   case I915_PMU_INTERRUPTS:
+   break;
+   case I915_PMU_RC6_RESIDENCY:
+   if (!HAS_RC6(i915))
+   return -ENODEV;
+   break;
default:
return -ENOENT;
}
@@ -314,6 +335,20 @@ static int engine_event_init(struct perf_event *event)
return 0;
 }
 
+static int engine_event_init(struct perf_event *event)
+{
+   struct drm_i915_private *i915 =
+   container_of(event->pmu, typeof(*i915), pmu.base);
+   struct intel_engine_cs *engine;
+
+   engine = intel_engine_lookup_user(i915, engine_event_class(event),
+ engine_event_instance(event));
+   if (!engine)
+   return -ENODEV;
+
+   return engine_event_status(engine, engine_event_sample(event));
+}
+
 static int i915_pmu_event_init(struct perf_event *event)
 {
struct drm_i915_private *i915 =
@@ -337,30 +372,10 @@ static int i915_pmu_event_init(struct perf_event *event)
if (!cpumask_test_cpu(event->cpu, &i915_pmu_cpumask))
return -EINVAL;
 
-   if (is_engine_event(event)) {
+   if (is_engine_event(event))
ret = engine_event_init(event);
-   } else {
-   ret = 0;
-   switch (event->attr.config) {
-   case I915_PMU_ACTUAL_FREQUENCY:
-   if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915))
-/* Requires a mutex for sampling! */
-   ret = -ENODEV;
-   case I915_PMU_REQUESTED_FREQUENCY:
-   if (INTEL_GEN(i915) < 6)
-   ret = -ENODEV;
-   break;
-   case I915_PMU_INTERRUPTS:
-   break;
-   case I915_PMU_RC6_RESIDENCY:
-   if (!HAS_RC6(i915))
-   ret = -ENODEV;
-   break;
-   default:
-   ret = -ENOENT;
-   break;
-   }
-   }
+   else
+   ret = config_status(i915, event->attr.config);;
if (ret)
return ret;
 
@@ -657,52 +672,9 @@ static ssize_t i915_pmu_event_show(struct device *dev,
return sprintf(buf, "config=0x%lx\n", eattr->val);
 }
 
-#define I915_EVENT_ATTR(_name, _config) \
-   (&((struct i915_ext_attribute[]) { \
-   { .attr = __ATTR(_name, 0444, i915_pmu_event_show, NULL), \
- .val = _config, } \
-