Re: [RFC 2/5] drm/i915: Expose list of clients in sysfs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 14/02/2018 19:13, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2018-02-14 18:50:32)
From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>

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

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
---
  drivers/gpu/drm/i915/i915_drv.h   |  13 +++++
  drivers/gpu/drm/i915/i915_gem.c   | 112 +++++++++++++++++++++++++++++++++++---
  drivers/gpu/drm/i915/i915_sysfs.c |   8 +++
  3 files changed, 126 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 70cf289855af..2133e67f5fbc 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -345,6 +345,16 @@ struct drm_i915_file_private {
   */
  #define I915_MAX_CLIENT_CONTEXT_BANS 3
         atomic_t context_bans;
+

struct {

+       unsigned int client_sysfs_id;
+       unsigned int client_pid;
+       char *client_name;
+       struct kobject *client_root;

} client;

client_sysfs?

+
+       struct {
+               struct device_attribute pid;
+               struct device_attribute name;
+       } attr;

sysfs_attr?

Ok.

  };
/* Interface history:
@@ -2365,6 +2375,9 @@ struct drm_i915_private {
struct i915_pmu pmu; + struct kobject *clients_root;
+       atomic_t client_serial;

I'd start a new substruct { } clients;

Ok.

         /*
          * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
          * will be rejected. Instead look for a better place.
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index fc68b35854df..24607bce2efe 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5613,6 +5613,89 @@ int i915_gem_freeze_late(struct drm_i915_private *dev_priv)
         return 0;
  }
+static ssize_t
+show_client_name(struct device *kdev, struct device_attribute *attr, char *buf)
+{
+       struct drm_i915_file_private *file_priv =
+               container_of(attr, struct drm_i915_file_private, attr.name);
+
+       return snprintf(buf, PAGE_SIZE, "%s", file_priv->client_name);
+}
+
+static ssize_t
+show_client_pid(struct device *kdev, struct device_attribute *attr, char *buf)
+{
+       struct drm_i915_file_private *file_priv =
+               container_of(attr, struct drm_i915_file_private, attr.pid);
+
+       return snprintf(buf, PAGE_SIZE, "%u", file_priv->client_pid);
+}
+
+static int
+i915_gem_add_client(struct drm_i915_private *i915,
+               struct drm_i915_file_private *file_priv,
+               struct task_struct *task,
+               unsigned int serial)
+{
+       int ret = -ENOMEM;
+       struct device_attribute *attr;
+       char id[32];
+
+       file_priv->client_name = kstrdup(task->comm, GFP_KERNEL);
+       if (!file_priv->client_name)
+               goto err_name;
+
+       snprintf(id, sizeof(id), "%u", serial);
+       file_priv->client_root = kobject_create_and_add(id,
+                                                       i915->clients_root);

Do we have to care about i915->clients_root being NULL here?

Yep, due our lax handling of sysfs registration failures. :/

+       if (!file_priv->client_root)
+               goto err_client;
+
+       attr = &file_priv->attr.name;
+       attr->attr.name = "name";
+       attr->attr.mode = 0444;
+       attr->show = show_client_name;
+
+       ret = sysfs_create_file(file_priv->client_root,
+                               (struct attribute *)attr);
+       if (ret)
+               goto err_attr_name;
+
+       attr = &file_priv->attr.pid;
+       attr->attr.name = "pid";
+       attr->attr.mode = 0444;
+       attr->show = show_client_pid;
+
+       ret = sysfs_create_file(file_priv->client_root,
+                               (struct attribute *)attr);
+       if (ret)
+               goto err_attr_pid;
+
+       file_priv->client_pid = pid_nr(get_task_pid(task, PIDTYPE_PID));

Are we before or after the "create_context get new pid"? Just wondering
aloud how that's going to tie in.

Before, create_context then updates it in a following patch. Which probably needs improving to avoid remove and re-add, and add a proper update helper.


+
+       return 0;
+
+err_attr_pid:
+       sysfs_remove_file(file_priv->client_root,
+                         (struct attribute *)&file_priv->attr.name);
+err_attr_name:
+       kobject_put(file_priv->client_root);
+err_client:
+       kfree(file_priv->client_name);
+err_name:
+       return ret;
+}
+
+static void i915_gem_remove_client(struct drm_i915_file_private *file_priv)
+{
+       sysfs_remove_file(file_priv->client_root,
+                         (struct attribute *)&file_priv->attr.pid);
+       sysfs_remove_file(file_priv->client_root,
+                         (struct attribute *)&file_priv->attr.name);
+       kobject_put(file_priv->client_root);
+       kfree(file_priv->client_name);
+}
+
  void i915_gem_release(struct drm_device *dev, struct drm_file *file)
  {
         struct drm_i915_file_private *file_priv = file->driver_priv;
@@ -5626,32 +5709,47 @@ void i915_gem_release(struct drm_device *dev, struct drm_file *file)
         list_for_each_entry(request, &file_priv->mm.request_list, client_link)
                 request->file_priv = NULL;
         spin_unlock(&file_priv->mm.lock);
+
+       i915_gem_remove_client(file_priv);
  }
int i915_gem_open(struct drm_i915_private *i915, struct drm_file *file)
  {
+       int ret = -ENOMEM;
         struct drm_i915_file_private *file_priv;
-       int ret;
DRM_DEBUG("\n"); file_priv = kzalloc(sizeof(*file_priv), GFP_KERNEL);
         if (!file_priv)
-               return -ENOMEM;
+               goto err_alloc;
+
+       file_priv->client_sysfs_id = atomic_inc_return(&i915->client_serial);

What's the use for client_sysfd_id? I don't see it used again.

Used from context create pid update, but its weak.. I'll eliminate it after above mentioned improvements.

Regards,

Tvrtko


+       ret = i915_gem_add_client(i915, file_priv, current,
+                                 file_priv->client_sysfs_id);
+       if (ret)
+               goto err_client;
file->driver_priv = file_priv;
+       ret = i915_gem_context_open(i915, file);
+       if (ret)
+               goto err_context;
+
         file_priv->dev_priv = i915;
         file_priv->file = file;
+       file_priv->bsd_engine = -1;
spin_lock_init(&file_priv->mm.lock);
         INIT_LIST_HEAD(&file_priv->mm.request_list);
- file_priv->bsd_engine = -1;
-
-       ret = i915_gem_context_open(i915, file);
-       if (ret)
-               kfree(file_priv);
+       return 0;
+err_context:
+       i915_gem_remove_client(file_priv);
+err_client:
+       atomic_dec(&i915->client_serial);
+       kfree(file_priv);
+err_alloc:
         return ret;
  }
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index b33d2158c234..6cf032bc1573 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -575,6 +575,11 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv)
         struct device *kdev = dev_priv->drm.primary->kdev;
         int ret;
+ dev_priv->clients_root =
+               kobject_create_and_add("clients", &kdev->kobj);
+       if (!dev_priv->clients_root)
+               DRM_ERROR("Per-client sysfs setup failed\n");
+
  #ifdef CONFIG_PM
         if (HAS_RC6(dev_priv)) {
                 ret = sysfs_merge_group(&kdev->kobj,
@@ -635,4 +640,7 @@ void i915_teardown_sysfs(struct drm_i915_private *dev_priv)
         sysfs_unmerge_group(&kdev->kobj, &rc6_attr_group);
         sysfs_unmerge_group(&kdev->kobj, &rc6p_attr_group);
  #endif
+
+       if (dev_priv->clients_root)
+               kobject_put(dev_priv->clients_root);
  }
--
2.14.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux