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

Tvrtko Ursulin
2020-01-10T14:09:15Z

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