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@xxxxxxxxxxxxxxx> wrote:
>
> 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
>
> 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@xxxxxxxxx>
> Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
