[Intel-gfx] [RFC 6/8] drm/i915: Expose per-engine client busyness

[Tvrtko Ursulin]

On 10/01/2020 13:58, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-01-10 13:30:47)
>> +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);
>> +       struct list_head *list = &i915_attr->client->ctx_list;
>> +       unsigned int engine_class = i915_attr->engine_class;
>> +       struct i915_gem_context *ctx;
>> +       u64 total = 0;
>> +
>> +       if (i915_attr->no_busy_stats)
>> +               return -ENODEV;
>> +
>> +       rcu_read_lock();
>> +       list_for_each_entry_rcu(ctx, list, client_link)
>> +               total += sw_busy_add(ctx, engine_class);
>> +       rcu_read_unlock();
>> +
>> +       return snprintf(buf, PAGE_SIZE, "%llu\n", total);
>> +}
>> +
>> +static const char *uabi_class_names[] = {
>> +       [I915_ENGINE_CLASS_RENDER] = "0",
>> +       [I915_ENGINE_CLASS_COPY] = "1",
>> +       [I915_ENGINE_CLASS_VIDEO] = "2",
>> +       [I915_ENGINE_CLASS_VIDEO_ENHANCE] = "3",
>> +};
> 
> Hmm. /sys/class/drm/card0/clients/0/busy/0
> 
> Ok. I was worried this was 0/0 and so very bland and liable to clash
> later.
> 
>> +
>>   int
>>   __i915_drm_client_register(struct i915_drm_client *client,
>>                             struct task_struct *task)
>>   {
>>          struct i915_drm_clients *clients = client->clients;
>> +       struct drm_i915_private *i915 =
>> +               container_of(clients, typeof(*i915), clients);
>> +       struct intel_engine_cs *engine;
>>          struct device_attribute *attr;
>> -       int ret = -ENOMEM;
>> +       int i, ret = -ENOMEM;
>>          char idstr[32];
>>   
>>          if (!clients->root)
>> @@ -77,10 +130,71 @@ __i915_drm_client_register(struct i915_drm_client *client,
>>          if (ret)
>>                  goto err_attr;
>>   
>> +       if (HAS_LOGICAL_RING_CONTEXTS(i915)) {
>> +               client->busy_root =
>> +                       kobject_create_and_add("busy", client->root);
>> +               if (!client->busy_root)
>> +                       goto err_attr;
>> +
>> +               for (i = 0; i < ARRAY_SIZE(uabi_class_names); i++) {
>> +                       struct i915_engine_busy_attribute *i915_attr =
>> +                               &client->attr.busy[i];
> 
> 
> if (!intel_engine_lookup_user(i915, i, 0))
> 	continue;
> 
> i.e. skip if we don't have any engines of that class in the system.

Yes, thanks.

>> +
>> +                       i915_attr->client = client;
>> +                       i915_attr->engine_class = i;
>> +
>> +                       attr = &i915_attr->attr;
>> +
>> +                       sysfs_attr_init(&attr->attr);
>> +
>> +                       attr->attr.name = uabi_class_names[i];
>> +                       attr->attr.mode = 0444;
>> +                       attr->show = show_client_busy;
>> +
>> +                       ret = sysfs_create_file(client->busy_root,
>> +                                               (struct attribute *)attr);
>> +                       if (ret)
>> +                               goto err_busy;
>> +               }
>> +
>> +               /* Enable busy stats on all engines. */
>> +               i = 0;
>> +               for_each_uabi_engine(engine, i915) {
>> +                       ret = intel_enable_engine_stats(engine);
> 
> Hmm. We gave it a global bit in
> 
> 	i915->caps.scheduler & I915_SCHEDULER_CAP_ENABLED.
> 
> That'll avoid having to do the individual checking and rollback.

I could add a top level check as a short circuit, but I prefer to check 
return code from intel_enable_engine_stats since it returns one.

Also if new GuC will have I915_SCHEDULER_CAP_ENABLED it will still fail 
to enable engine stats and then fallback to pphwsp has to happen.

Regards,

Tvrtko