[Intel-gfx] [RFC 3/8] drm/i915: Expose list of clients in sysfs

[Tvrtko Ursulin]

On 19/12/2019 20:48, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-12-19 18:00:14)
>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>
>> Expose a list of clients with open file handles in sysfs.
>>
>> This will be a basis for a top-like utility showing per-client and per-
>> engine GPU load.
>>
>> Currently we only expose each client's pid and name under opaque numbered
>> directories in /sys/class/drm/card0/clients/.
>>
>> For instance:
>>
>> /sys/class/drm/card0/clients/3/name: Xorg
>> /sys/class/drm/card0/clients/3/pid: 5664
>>
>> v2:
>>   Chris Wilson:
>>   * Enclose new members into dedicated structs.
>>   * Protect against failed sysfs registration.
>>
>> v3:
>>   * sysfs_attr_init.
>>
>> v4:
>>   * Fix for internal clients.
>>
>> v5:
>>   * Use cyclic ida for client id. (Chris)
>>   * Do not leak pid reference. (Chris)
>>   * Tidy code with some locals.
>>
>> v6:
>>   * Use xa_alloc_cyclic to simplify locking. (Chris)
>>   * No need to unregister individial sysfs files. (Chris)
>>   * Rebase on top of fpriv kref.
>>   * Track client closed status and reflect in sysfs.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h   |  20 +++++
>>   drivers/gpu/drm/i915/i915_gem.c   | 133 ++++++++++++++++++++++++++++--
>>   drivers/gpu/drm/i915/i915_sysfs.c |   8 ++
>>   3 files changed, 155 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 6f13f0c619e9..e1d8361aafd7 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -188,6 +188,7 @@ struct i915_hotplug {
>>   struct drm_i915_private;
>>   struct i915_mm_struct;
>>   struct i915_mmu_object;
>> +struct i915_drm_clients;
>>   
>>   struct drm_i915_file_private {
>>          struct kref kref;
>> @@ -226,6 +227,19 @@ struct drm_i915_file_private {
>>          /** ban_score: Accumulated score of all ctx bans and fast hangs. */
>>          atomic_t ban_score;
>>          unsigned long hang_timestamp;
>> +
>> +       struct i915_drm_client {
> 
> I agree with the distinction here between drm_client and
> gem_client. (This concept will be required beyond GEM.)

So you think I should keep the i915_drm_client naming for these bits 
throughout?

>> +               unsigned int id;
>> +               struct pid *pid;
>> +               char *name;
>> +               bool closed;
>> +
>> +               struct kobject *root;
>> +               struct {
>> +                       struct device_attribute pid;
>> +                       struct device_attribute name;
>> +               } attr;
>> +       } client;
>>   };
>>   
>>   /* Interface history:
>> @@ -1280,6 +1294,12 @@ struct drm_i915_private {
>>   
>>          struct i915_pmu pmu;
>>   
>> +       struct i915_drm_clients {
>> +               struct xarray xarray;
>> +
>> +               struct kobject *root;
>> +       } clients;
> 
> You might as well pull this out into i915_drm_client.[ch]
> Actually make that into a please. :)

Np. Maybe next year though. :)

Regards,

Tvrtko