Re: [Intel-gfx] [PATCH 1/9] drm/i915: Expose list of clients in sysfs

* Re: [Intel-gfx] [PATCH 1/9] drm/i915: Expose list of clients in sysfs
       [not found] ` <20200415101138.26126-2-tvrtko.ursulin@linux.intel.com>
@ 2020-08-26  1:11   ` Lucas De Marchi
  2020-09-01 15:09     ` Tvrtko Ursulin
  0 siblings, 1 reply; 7+ messages in thread
From: Lucas De Marchi @ 2020-08-26  1:11 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: Intel Graphics, Chris Wilson

Hi,

Any update on this? It now conflicts in a few places so it needs a rebase.

Lucas De Marchi

On Wed, Apr 15, 2020 at 3:11 AM Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>
> From: Tvrtko Ursulin <tvrtko.ursulin@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.
>
> v7:
>  * Make drm_client more standalone concept.
>
> v8:
>  * Simplify sysfs show. (Chris)
>  * Always track name and pid.
>
> v9:
>  * Fix cyclic id assignment.
>
> v10:
>  * No need for a mutex around xa_alloc_cyclic.
>  * Refactor sysfs into own function.
>  * Unregister sysfs before freeing pid and name.
>  * Move clients setup into own function.
>
> v11:
>  * Call clients init directly from driver init. (Chris)
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/Makefile          |   3 +-
>  drivers/gpu/drm/i915/i915_drm_client.c | 179 +++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_drm_client.h |  64 +++++++++
>  drivers/gpu/drm/i915/i915_drv.c        |   3 +
>  drivers/gpu/drm/i915/i915_drv.h        |   5 +
>  drivers/gpu/drm/i915/i915_gem.c        |  25 +++-
>  drivers/gpu/drm/i915/i915_sysfs.c      |   8 ++
>  7 files changed, 283 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/i915_drm_client.c
>  create mode 100644 drivers/gpu/drm/i915/i915_drm_client.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 44c506b7e117..b30f3d51c66a 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -33,7 +33,8 @@ subdir-ccflags-y += -I$(srctree)/$(src)
>  # Please keep these build lists sorted!
>
>  # core driver code
> -i915-y += i915_drv.o \
> +i915-y += i915_drm_client.o \
> +         i915_drv.o \
>           i915_irq.o \
>           i915_getparam.o \
>           i915_params.o \
> diff --git a/drivers/gpu/drm/i915/i915_drm_client.c b/drivers/gpu/drm/i915/i915_drm_client.c
> new file mode 100644
> index 000000000000..2067fbcdb795
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_drm_client.c
> @@ -0,0 +1,179 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2020 Intel Corporation
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +
> +#include "i915_drm_client.h"
> +#include "i915_gem.h"
> +#include "i915_utils.h"
> +
> +void i915_drm_clients_init(struct i915_drm_clients *clients)
> +{
> +       clients->next_id = 0;
> +       xa_init_flags(&clients->xarray, XA_FLAGS_ALLOC);
> +}
> +
> +static ssize_t
> +show_client_name(struct device *kdev, struct device_attribute *attr, char *buf)
> +{
> +       struct i915_drm_client *client =
> +               container_of(attr, typeof(*client), attr.name);
> +
> +       return snprintf(buf, PAGE_SIZE,
> +                       READ_ONCE(client->closed) ? "<%s>" : "%s",
> +                       client->name);
> +}
> +
> +static ssize_t
> +show_client_pid(struct device *kdev, struct device_attribute *attr, char *buf)
> +{
> +       struct i915_drm_client *client =
> +               container_of(attr, typeof(*client), attr.pid);
> +
> +       return snprintf(buf, PAGE_SIZE,
> +                       READ_ONCE(client->closed) ? "<%u>" : "%u",
> +                       pid_nr(client->pid));
> +}
> +
> +static int
> +__client_register_sysfs(struct i915_drm_client *client)
> +{
> +       const struct {
> +               const char *name;
> +               struct device_attribute *attr;
> +               ssize_t (*show)(struct device *dev,
> +                               struct device_attribute *attr,
> +                               char *buf);
> +       } files[] = {
> +               { "name", &client->attr.name, show_client_name },
> +               { "pid", &client->attr.pid, show_client_pid },
> +       };
> +       unsigned int i;
> +       char buf[16];
> +       int ret;
> +
> +       ret = scnprintf(buf, sizeof(buf), "%u", client->id);
> +       if (ret == sizeof(buf))
> +               return -EINVAL;
> +
> +       client->root = kobject_create_and_add(buf, client->clients->root);
> +       if (!client->root)
> +               return -ENOMEM;
> +
> +       for (i = 0; i < ARRAY_SIZE(files); i++) {
> +               struct device_attribute *attr = files[i].attr;
> +
> +               sysfs_attr_init(&attr->attr);
> +
> +               attr->attr.name = files[i].name;
> +               attr->attr.mode = 0444;
> +               attr->show = files[i].show;
> +
> +               ret = sysfs_create_file(client->root, (struct attribute *)attr);
> +               if (ret)
> +                       break;
> +       }
> +
> +       if (ret)
> +               kobject_put(client->root);
> +
> +       return ret;
> +}
> +
> +static void __client_unregister_sysfs(struct i915_drm_client *client)
> +{
> +       kobject_put(fetch_and_zero(&client->root));
> +}
> +
> +static int
> +__i915_drm_client_register(struct i915_drm_client *client,
> +                          struct task_struct *task)
> +{
> +       struct i915_drm_clients *clients = client->clients;
> +       char *name;
> +       int ret;
> +
> +       name = kstrdup(task->comm, GFP_KERNEL);
> +       if (!name)
> +               return -ENOMEM;
> +
> +       client->pid = get_task_pid(task, PIDTYPE_PID);
> +       client->name = name;
> +
> +       if (!clients->root)
> +               return 0; /* intel_fbdev_init registers a client before sysfs */
> +
> +       ret = __client_register_sysfs(client);
> +       if (ret)
> +               goto err_sysfs;
> +
> +       return 0;
> +
> +err_sysfs:
> +       put_pid(client->pid);
> +       kfree(client->name);
> +
> +       return ret;
> +}
> +
> +static void
> +__i915_drm_client_unregister(struct i915_drm_client *client)
> +{
> +       __client_unregister_sysfs(client);
> +
> +       put_pid(fetch_and_zero(&client->pid));
> +       kfree(fetch_and_zero(&client->name));
> +}
> +
> +struct i915_drm_client *
> +i915_drm_client_add(struct i915_drm_clients *clients, struct task_struct *task)
> +{
> +       struct i915_drm_client *client;
> +       int ret;
> +
> +       client = kzalloc(sizeof(*client), GFP_KERNEL);
> +       if (!client)
> +               return ERR_PTR(-ENOMEM);
> +
> +       kref_init(&client->kref);
> +       client->clients = clients;
> +
> +       ret = xa_alloc_cyclic(&clients->xarray, &client->id, client,
> +                             xa_limit_32b, &clients->next_id, GFP_KERNEL);
> +       if (ret)
> +               goto err_id;
> +
> +       ret = __i915_drm_client_register(client, task);
> +       if (ret)
> +               goto err_register;
> +
> +       return client;
> +
> +err_register:
> +       xa_erase(&clients->xarray, client->id);
> +err_id:
> +       kfree(client);
> +
> +       return ERR_PTR(ret);
> +}
> +
> +void __i915_drm_client_free(struct kref *kref)
> +{
> +       struct i915_drm_client *client =
> +               container_of(kref, typeof(*client), kref);
> +
> +       __i915_drm_client_unregister(client);
> +       xa_erase(&client->clients->xarray, client->id);
> +       kfree_rcu(client, rcu);
> +}
> +
> +void i915_drm_client_close(struct i915_drm_client *client)
> +{
> +       GEM_BUG_ON(READ_ONCE(client->closed));
> +       WRITE_ONCE(client->closed, true);
> +       i915_drm_client_put(client);
> +}
> diff --git a/drivers/gpu/drm/i915/i915_drm_client.h b/drivers/gpu/drm/i915/i915_drm_client.h
> new file mode 100644
> index 000000000000..af6998c74d4c
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_drm_client.h
> @@ -0,0 +1,64 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2020 Intel Corporation
> + */
> +
> +#ifndef __I915_DRM_CLIENT_H__
> +#define __I915_DRM_CLIENT_H__
> +
> +#include <linux/device.h>
> +#include <linux/kobject.h>
> +#include <linux/kref.h>
> +#include <linux/pid.h>
> +#include <linux/rcupdate.h>
> +#include <linux/sched.h>
> +#include <linux/xarray.h>
> +
> +struct i915_drm_clients {
> +       struct xarray xarray;
> +       u32 next_id;
> +
> +       struct kobject *root;
> +};
> +
> +struct i915_drm_client {
> +       struct kref kref;
> +
> +       struct rcu_head rcu;
> +
> +       unsigned int id;
> +       struct pid *pid;
> +       char *name;
> +       bool closed;
> +
> +       struct i915_drm_clients *clients;
> +
> +       struct kobject *root;
> +       struct {
> +               struct device_attribute pid;
> +               struct device_attribute name;
> +       } attr;
> +};
> +
> +void i915_drm_clients_init(struct i915_drm_clients *clients);
> +
> +static inline struct i915_drm_client *
> +i915_drm_client_get(struct i915_drm_client *client)
> +{
> +       kref_get(&client->kref);
> +       return client;
> +}
> +
> +void __i915_drm_client_free(struct kref *kref);
> +
> +static inline void i915_drm_client_put(struct i915_drm_client *client)
> +{
> +       kref_put(&client->kref, __i915_drm_client_free);
> +}
> +
> +void i915_drm_client_close(struct i915_drm_client *client);
> +
> +struct i915_drm_client *i915_drm_client_add(struct i915_drm_clients *clients,
> +                                           struct task_struct *task);
> +
> +#endif /* !__I915_DRM_CLIENT_H__ */
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 641f5e03b661..dac84b17d23d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -70,6 +70,7 @@
>  #include "gt/intel_rc6.h"
>
>  #include "i915_debugfs.h"
> +#include "i915_drm_client.h"
>  #include "i915_drv.h"
>  #include "i915_ioc32.h"
>  #include "i915_irq.h"
> @@ -456,6 +457,8 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
>
>         i915_gem_init_early(dev_priv);
>
> +       i915_drm_clients_init(&dev_priv->clients);
> +
>         /* This must be called before any calls to HAS_PCH_* */
>         intel_detect_pch(dev_priv);
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e9ee4daa9320..f9f0c3ba6e4a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -91,6 +91,7 @@
>  #include "intel_wakeref.h"
>  #include "intel_wopcm.h"
>
> +#include "i915_drm_client.h"
>  #include "i915_gem.h"
>  #include "i915_gem_gtt.h"
>  #include "i915_gpu_error.h"
> @@ -226,6 +227,8 @@ 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 *client;
>  };
>
>  /* Interface history:
> @@ -1201,6 +1204,8 @@ struct drm_i915_private {
>
>         struct i915_pmu pmu;
>
> +       struct i915_drm_clients clients;
> +
>         struct i915_hdcp_comp_master *hdcp_master;
>         bool hdcp_comp_added;
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 0cbcb9f54e7d..5a0b5fae8b92 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1234,6 +1234,8 @@ void i915_gem_cleanup_early(struct drm_i915_private *dev_priv)
>         GEM_BUG_ON(!llist_empty(&dev_priv->mm.free_list));
>         GEM_BUG_ON(atomic_read(&dev_priv->mm.free_count));
>         drm_WARN_ON(&dev_priv->drm, dev_priv->mm.shrink_count);
> +       drm_WARN_ON(&dev_priv->drm, !xa_empty(&dev_priv->clients.xarray));
> +       xa_destroy(&dev_priv->clients.xarray);
>  }
>
>  int i915_gem_freeze(struct drm_i915_private *dev_priv)
> @@ -1288,6 +1290,8 @@ void i915_gem_release(struct drm_device *dev, struct drm_file *file)
>         struct drm_i915_file_private *file_priv = file->driver_priv;
>         struct i915_request *request;
>
> +       i915_drm_client_close(file_priv->client);
> +
>         /* Clean up our request list when the client is going away, so that
>          * later retire_requests won't dereference our soon-to-be-gone
>          * file_priv.
> @@ -1301,17 +1305,25 @@ void i915_gem_release(struct drm_device *dev, struct drm_file *file)
>  int i915_gem_open(struct drm_i915_private *i915, struct drm_file *file)
>  {
>         struct drm_i915_file_private *file_priv;
> -       int ret;
> +       struct i915_drm_client *client;
> +       int ret = -ENOMEM;
>
>         DRM_DEBUG("\n");
>
>         file_priv = kzalloc(sizeof(*file_priv), GFP_KERNEL);
>         if (!file_priv)
> -               return -ENOMEM;
> +               goto err_alloc;
> +
> +       client = i915_drm_client_add(&i915->clients, current);
> +       if (IS_ERR(client)) {
> +               ret = PTR_ERR(client);
> +               goto err_client;
> +       }
>
>         file->driver_priv = file_priv;
>         file_priv->dev_priv = i915;
>         file_priv->file = file;
> +       file_priv->client = client;
>
>         spin_lock_init(&file_priv->mm.lock);
>         INIT_LIST_HEAD(&file_priv->mm.request_list);
> @@ -1321,8 +1333,15 @@ int i915_gem_open(struct drm_i915_private *i915, struct drm_file *file)
>
>         ret = i915_gem_context_open(i915, file);
>         if (ret)
> -               kfree(file_priv);
> +               goto err_context;
> +
> +       return 0;
>
> +err_context:
> +       i915_drm_client_close(client);
> +err_client:
> +       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 45d32ef42787..b7d4a6d2dd5c 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -560,6 +560,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,
> @@ -627,4 +632,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.20.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 7+ messages in thread