Re: [PATCH 8/8] drm/i915: Gate engine stats collection with a static key

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

On Mon, 2017-09-18 at 12:38 +0100, Tvrtko Ursulin wrote:
>  static inline void intel_engine_context_out(struct intel_engine_cs
> *engine)
>  {
>         unsigned long flags;
>  
> -       if (READ_ONCE(engine->stats.enabled) == 0)
> -               return;
> -
> -       spin_lock_irqsave(&engine->stats.lock, flags);
> -
> -       if (engine->stats.enabled > 0) {
> -               ktime_t last, now = ktime_get();
> -
> -               if (engine->stats.active && --engine->stats.active ==
> 0) {
> -                       /*
> -                        * Decrement the active context count and in
> case GPU
> -                        * is now idle add up to the running total.
> -                        */
> -                       last = ktime_sub(now, engine->stats.start);
> -
> -                       engine->stats.total =
> ktime_add(engine->stats.total,
> -                                                       last);
> -               } else if (engine->stats.active == 0) {
> -                       /*
> -                        * After turning on engine stats, context out
> might be
> -                        * the first event in which case we account
> from the
> -                        * time stats gathering was turned on.
> -                        */
> -                       last = ktime_sub(now,
> engine->stats.enabled_at);
> -
> -                       engine->stats.total =
> ktime_add(engine->stats.total,
> -                                                       last);
> +       if (static_branch_unlikely(&i915_engine_stats_key)) {
> +               if (READ_ONCE(engine->stats.enabled) == 0)
> +                       return;
> +
> +               spin_lock_irqsave(&engine->stats.lock, flags);
> +
> +               if (engine->stats.enabled > 0) {
> +                       ktime_t last, now = ktime_get();
> +
> +                       if (engine->stats.active &&
> +                           --engine->stats.active == 0) {
> +                               /*
> +                                * Decrement the active context count
> and in
> +                                * case GPU is now idle add up to the
> running
> +                                * total.
> +                                */
> +                               last = ktime_sub(now,
> engine->stats.start);
> +
> +                               engine->stats.total =
> +                                       ktime_add(engine->stats.total,
> last);
> +                       } else if (engine->stats.active == 0) {
> +                               /*
> +                                * After turning on engine stats,
> context out
> +                                * might be the first event in which
> case we
> +                                * account from the time stats
> gathering was
> +                                * turned on.
> +                                */
> +                               last = ktime_sub(now,
> engine->stats.enabled_at);
> +
> +                               engine->stats.total =
> +                                       ktime_add(engine->stats.total,
> last);
> +                       }
> +
> +                       spin_unlock_irqrestore(&engine->stats.lock,
> flags);

This irqrestore looks placed asymmetric with irqsave.

>                 }
>         }
> -
> -       spin_unlock_irqrestore(&engine->stats.lock, flags);
>  }

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