[Intel-gfx] [PATCH 08/10] drm/i915: Expose per-engine client busyness

[Tvrtko Ursulin]

On 18/03/2020 14:34, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-03-18 14:15:31)
>>
>> On 18/03/2020 14:00, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2020-03-18 12:11:37)
>>>> +static u64
>>>> +pphwsp_busy_add(struct i915_gem_context *ctx, unsigned int class)
>>>> +{
>>>> +       struct i915_gem_engines *engines = rcu_dereference(ctx->engines);
>>>> +       struct i915_gem_engines_iter it;
>>>> +       struct intel_context *ce;
>>>> +       u64 total = 0;
>>>> +
>>>> +       for_each_gem_engine(ce, engines, it) {
>>>> +               if (ce->engine->uabi_class == class)
>>>> +                       total += ce->runtime.total;
>>>> +       }
>>>> +
>>>> +       return total;
>>>> +}
>>>> +
>>>> +static ssize_t
>>>> +show_client_busy(struct device *kdev, struct device_attribute *attr, char *buf)
>>>> +{
>>>> +       struct i915_engine_busy_attribute *i915_attr =
>>>> +               container_of(attr, typeof(*i915_attr), attr);
>>>> +       unsigned int class = i915_attr->engine_class;
>>>> +       struct i915_drm_client *client = i915_attr->client;
>>>> +       u64 total = atomic64_read(&client->past_runtime[class]);
>>>> +       struct list_head *list = &client->ctx_list;
>>>> +       struct i915_gem_context *ctx;
>>>> +
>>>> +       rcu_read_lock();
>>>> +       list_for_each_entry_rcu(ctx, list, client_link) {
>>>> +               total += atomic64_read(&ctx->past_runtime[class]);
>>>> +               total += pphwsp_busy_add(ctx, class);
>>>
>>> Hmm. I would like to have some GEM context agnosticism here. At the
>>> moment, all I have to offer is
>>>
>>> struct client_runtime {
>>>        struct list_head client_link;
>>>        atomic64_t past_runtime;
>>>        u64 (*cur_runtime)(struct client_runtime *);
>>> };
>>
>> What exactly do you mean here? Who keeps a list and of what and what
>> does the vfunc do?
> 
> This is what you've added to GEM context. Here in i915/i915_drm_client.c
> we shouldn't have to know about i915/gem/i915_gem_context internals. So
> the GEM context registers its client_runtime (by coupling that into the
> list).

I've ignored this for now as it was feeling a bit too much for too 
little benefit and just decided to send out another simple rebase, given 
that there seems to be some urgency around this work now.

Regards,

Tvrtko