On 18/07/2017 16:22, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2017-07-18 15:36:16)
>> +u64 intel_engine_get_current_busy_ns(struct intel_engine_cs *engine)
>> +{
>> + unsigned long flags;
>> + u64 total;
>> +
>> + spin_lock_irqsave(&engine->stats.lock, flags);
>> +
>> + total = engine->stats.total;
>> +
>> + /*
>> + * If the engine is executing something at the moment
>> + * add it to the total.
>> + */
>> + if (engine->stats.ref)
>> + total += ktime_get_real_ns() - engine->stats.start;
>> +
>> + spin_unlock_irqrestore(&engine->stats.lock, flags);
>
> Answers to another patch found here. I would say this is the other half
> of the interface and should be kept together.
Yes, it was an ugly split.
On 18/07/2017 16:43, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2017-07-18 15:36:16)
>> +int intel_enable_engine_stats(struct drm_i915_private *dev_priv)
>> +{
>> + if (!i915.enable_execlists)
>> + return -ENODEV;
>> +
>> + mutex_lock(&i915_engine_stats_mutex);
>> + if (i915_engine_stats_ref++ == 0) {
>> + struct intel_engine_cs *engine;
>> + enum intel_engine_id id;
>> +
>> + for_each_engine(engine, dev_priv, id) {
>> + memset(&engine->stats, 0, sizeof(engine->stats));
>> + spin_lock_init(&engine->stats.lock);
>> + }
>> +
>> + static_branch_enable(&i915_engine_stats_key);
>> + }
>> + mutex_unlock(&i915_engine_stats_mutex);
>
> I don't think static_branch_enable() is a might_sleep, so it looks like
> you can rewrite this avoiding the mutex and thus not requiring the
> worker and then can use the error code here to decide if you need to
> use the timer instead.
Perhaps I could get rid of the mutex though by using atomic_inc/dec_return.
But there is a mutex in jump label handling, so I think the workers have
to stay - and it is also beneficial to have delayed static branch disable,
since the perf core seems to like calling start/stop on the events a lot.
But it is recommended practice for static branches in any way.
So from that angle I could perhaps even move the delayed disable to this
patch so it is automatically shared by all callers.
>> +static DEFINE_MUTEX(i915_engine_stats_mutex);
>> +static int i915_engine_stats_ref;
>>
>> /* Haswell does have the CXT_SIZE register however it does not appear to be
>> * valid. Now, docs explain in dwords what is in the context object. The full
>> @@ -1340,6 +1342,57 @@ void intel_engines_mark_idle(struct drm_i915_private *i915)
>> }
>> }
>>
>> +int intel_enable_engine_stats(struct drm_i915_private *dev_priv)
>
> The pattern I've been trying to use here is
>
> intel_engine_* - operate on the named engine
>
> intel_engines_* - operate on all engines
Ok.
> Long term though having a global static key is going to be a nasty wart.
> Joonas will definitely ask the question how much will it cost us to use
> an engine->bool and what we can do to minimise that cost.
Why you think it is nasty? Sounds pretty cool to me.
But I think can re-organize the series to start with a normal branch and
then add the static one on top if so is desired.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
