Quoting Tvrtko Ursulin (2017-09-25 16:15:43)
> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
>
> This reduces the cost of the software engine busyness tracking
> to a single no-op instruction when there are no listeners.
>
> v2: Rebase and some comments.
> v3: Rebase.
> v4: Checkpatch fixes.
> v5: Rebase.
> v6: Use system_long_wq to avoid being blocked by struct_mutex
> users.
> v7: Fix bad conflict resolution from last rebase. (Dmitry Rogozhkin)
> v8: Rebase.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Bah, still unhappy about the global. I know in all likelihood it doesn't
matter, but it still bugs me.
> ---
> drivers/gpu/drm/i915/i915_pmu.c | 54 +++++++++++++++--
> drivers/gpu/drm/i915/intel_engine_cs.c | 17 ++++++
> drivers/gpu/drm/i915/intel_ringbuffer.h | 101 ++++++++++++++++++++------------
> 3 files changed, 130 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index 228aa50ce709..e768f33ebb3d 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -501,11 +501,17 @@ static void i915_pmu_enable(struct perf_event *event)
> GEM_BUG_ON(sample >= I915_PMU_SAMPLE_BITS);
> GEM_BUG_ON(engine->pmu.enable_count[sample] == ~0);
> if (engine->pmu.enable_count[sample]++ == 0) {
> + /*
> + * Enable engine busy stats tracking if needed or
> + * alternatively cancel the scheduled disabling of the
> + * same.
> + */
> if (engine_needs_busy_stats(engine) &&
> !engine->pmu.busy_stats) {
> - engine->pmu.busy_stats =
> - intel_enable_engine_stats(engine) == 0;
> - WARN_ON_ONCE(!engine->pmu.busy_stats);
> + engine->pmu.busy_stats = true;
> + if (!cancel_delayed_work(&engine->pmu.disable_busy_stats))
> + queue_work(system_long_wq,
> + &engine->pmu.enable_busy_stats);
> }
> }
> }
> @@ -548,7 +554,15 @@ static void i915_pmu_disable(struct perf_event *event)
> if (!engine_needs_busy_stats(engine) &&
> engine->pmu.busy_stats) {
> engine->pmu.busy_stats = false;
> - intel_disable_engine_stats(engine);
> + /*
> + * We request a delayed disable to handle the
> + * rapid on/off cycles on events which can
> + * happen when tools like perf stat start in a
> + * nicer way.
> + */
> + queue_delayed_work(system_long_wq,
> + &engine->pmu.disable_busy_stats,
> + round_jiffies_up_relative(HZ));
What's preventing the perverse system from executing the enable after
the disable? Say the enable was scheduled on another cpu that was
stalled?
Something like a if (cancel_work(enable)) return ?
> }
> }
> }
> @@ -739,9 +753,27 @@ static int i915_pmu_cpu_offline(unsigned int cpu, struct hlist_node *node)
> return 0;
> }
>
> +static void __enable_busy_stats(struct work_struct *work)
> +{
> + struct intel_engine_cs *engine =
> + container_of(work, typeof(*engine), pmu.enable_busy_stats);
> +
> + WARN_ON_ONCE(intel_enable_engine_stats(engine));
> +}
> +
> +static void __disable_busy_stats(struct work_struct *work)
> +{
> + struct intel_engine_cs *engine =
> + container_of(work, typeof(*engine), pmu.disable_busy_stats.work);
> +
> + intel_disable_engine_stats(engine);
> +}
> +
> void i915_pmu_register(struct drm_i915_private *i915)
> {
> int ret;
> + struct intel_engine_cs *engine;
> + enum intel_engine_id id;
What a nice Christmas tree you are growing :-p
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
