Re: [PATCH] drm/sysfs: Add drm class-wide attribute to get active device clients

[Tvrtko Ursulin]

On 05/04/2024 18:59, Rob Clark wrote:
> On Wed, Apr 3, 2024 at 11:37 AM Adrián Larumbe
> <adrian.larumbe@xxxxxxxxxxxxx> wrote:
> > Up to this day, all fdinfo-based GPU profilers must traverse the entire
> > /proc directory structure to find open DRM clients with fdinfo file
> > descriptors. This is inefficient and time-consuming.
> >
> > This patch adds a new device class attribute that will install a sysfs file
> > per DRM device, which can be queried by profilers to get a list of PIDs for
> > their open clients. This file isn't human-readable, and it's meant to be
> > queried only by GPU profilers like gputop and nvtop.
> >
> > Cc: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> > Cc: Tvrtko Ursulin <tursulin@xxxxxxxxxxx>
> > Cc: Christopher Healy <healych@xxxxxxxxxx>
> > Signed-off-by: Adrián Larumbe <adrian.larumbe@xxxxxxxxxxxxx>
>
> It does seem like a good idea.. idk if there is some precedent to
> prefer binary vs ascii in sysfs, but having a way to avoid walking
> _all_ processes is a good idea.

I naturally second that it is a needed feature, but I do not think 
binary format is justified. AFAIR it should be used for things like 
hw/fw standardised tables or firmware images, not when exporting a 
simple list of PIDs. It also precludes easy shell/script access and the 
benefit of avoiding parsing a short list is I suspect completely dwarfed 
by needing to parse all the related fdinfo etc.

> > ---
> >   drivers/gpu/drm/drm_internal.h       |  2 +-
> >   drivers/gpu/drm/drm_privacy_screen.c |  2 +-
> >   drivers/gpu/drm/drm_sysfs.c          | 89 ++++++++++++++++++++++------
> >   3 files changed, 74 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> > index 2215baef9a3e..9a399b03d11c 100644
> > --- a/drivers/gpu/drm/drm_internal.h
> > +++ b/drivers/gpu/drm/drm_internal.h
> > @@ -145,7 +145,7 @@ bool drm_master_internal_acquire(struct drm_device *dev);
> >   void drm_master_internal_release(struct drm_device *dev);
> >
> >   /* drm_sysfs.c */
> > -extern struct class *drm_class;
> > +extern struct class drm_class;
> >
> >   int drm_sysfs_init(void);
> >   void drm_sysfs_destroy(void);
> > diff --git a/drivers/gpu/drm/drm_privacy_screen.c b/drivers/gpu/drm/drm_privacy_screen.c
> > index 6cc39e30781f..2fbd24ba5818 100644
> > --- a/drivers/gpu/drm/drm_privacy_screen.c
> > +++ b/drivers/gpu/drm/drm_privacy_screen.c
> > @@ -401,7 +401,7 @@ struct drm_privacy_screen *drm_privacy_screen_register(
> >          mutex_init(&priv->lock);
> >          BLOCKING_INIT_NOTIFIER_HEAD(&priv->notifier_head);
> >
> > -       priv->dev.class = drm_class;
> > +       priv->dev.class = &drm_class;
> >          priv->dev.type = &drm_privacy_screen_type;
> >          priv->dev.parent = parent;
> >          priv->dev.release = drm_privacy_screen_device_release;
> > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> > index a953f69a34b6..56ca9e22c720 100644
> > --- a/drivers/gpu/drm/drm_sysfs.c
> > +++ b/drivers/gpu/drm/drm_sysfs.c
> > @@ -58,8 +58,6 @@ static struct device_type drm_sysfs_device_connector = {
> >          .name = "drm_connector",
> >   };
> >
> > -struct class *drm_class;
> > -
> >   #ifdef CONFIG_ACPI
> >   static bool drm_connector_acpi_bus_match(struct device *dev)
> >   {
> > @@ -128,6 +126,62 @@ static const struct component_ops typec_connector_ops = {
> >
> >   static CLASS_ATTR_STRING(version, S_IRUGO, "drm 1.1.0 20060810");
> >
> > +static ssize_t clients_show(struct device *cd, struct device_attribute *attr, char *buf)
> > +{
> > +       struct drm_minor *minor = cd->driver_data;
> > +       struct drm_device *ddev = minor->dev;
> > +       struct drm_file *priv;
> > +       ssize_t offset = 0;
> > +       void *pid_buf;
> > +
> > +       if (minor->type != DRM_MINOR_RENDER)
> > +               return 0;

Why this?

> > +
> > +       pid_buf = kvmalloc(PAGE_SIZE, GFP_KERNEL);

I don't quite get the kvmalloc for just one page (or why even a temporay 
buffer and not write into buf directly?).

> > +       if (!pid_buf)
> > +               return 0;
> > +
> > +       mutex_lock(&ddev->filelist_mutex);
> > +       list_for_each_entry_reverse(priv, &ddev->filelist, lhead) {
> > +               struct pid *pid;
> > +
> > +               if (drm_WARN_ON(ddev, (PAGE_SIZE - offset) < sizeof(pid_t)))
> > +                       break;

Feels bad.. I would suggest exploring implementing a read callback 
(instead of show) and handling arbitrary size output.

> > +
> > +               rcu_read_lock();
> > +               pid = rcu_dereference(priv->pid);
> > +               (*(pid_t *)(pid_buf + offset)) = pid_vnr(pid);
> > +               rcu_read_unlock();
> > +
> > +               offset += sizeof(pid_t);
> > +       }
> > +       mutex_unlock(&ddev->filelist_mutex);
> > +
> > +       if (offset < PAGE_SIZE)
> > +               (*(pid_t *)(pid_buf + offset)) = 0;

Either NULL terminated or PAGE_SIZE/sizeof(pid) entries and not NULL 
terminated feels weird. If I got that right.

For me everything points towards going for text output.

> > +
> > +       memcpy(buf, pid_buf, offset);
> > +
> > +       kvfree(pid_buf);
> > +
> > +       return offset;
> > +
> > +}
> > +static DEVICE_ATTR_RO(clients);

Shouldn't BIN_ATTR_RO be used for binary files in sysfs?

Regards,

Tvrtko

P.S. Or maybe it is time for drmfs? Where each client gets a directory 
and drivers can populate files. Such as per client logging streams and 
whatnot.

> > +
> > +static struct attribute *drm_device_attrs[] = {
> > +       &dev_attr_clients.attr,
> > +       NULL,
> > +};
> > +ATTRIBUTE_GROUPS(drm_device);
> > +
> > +struct class drm_class = {
> > +       .name           = "drm",
> > +       .dev_groups     = drm_device_groups,
> > +};
> > +
> > +static bool drm_class_initialised;
> > +
> >   /**
> >    * drm_sysfs_init - initialize sysfs helpers
> >    *
> > @@ -142,18 +196,19 @@ int drm_sysfs_init(void)
> >   {
> >          int err;
> >
> > -       drm_class = class_create("drm");
> > -       if (IS_ERR(drm_class))
> > -               return PTR_ERR(drm_class);
> > +       err = class_register(&drm_class);
> > +       if (err)
> > +               return err;
> >
> > -       err = class_create_file(drm_class, &class_attr_version.attr);
> > +       err = class_create_file(&drm_class, &class_attr_version.attr);
> >          if (err) {
> > -               class_destroy(drm_class);
> > -               drm_class = NULL;
> > +               class_destroy(&drm_class);
> >                  return err;
> >          }
> >
> > -       drm_class->devnode = drm_devnode;
> > +       drm_class.devnode = drm_devnode;
> > +
> > +       drm_class_initialised = true;
> >
> >          drm_sysfs_acpi_register();
> >          return 0;
> > @@ -166,12 +221,12 @@ int drm_sysfs_init(void)
> >    */
> >   void drm_sysfs_destroy(void)
> >   {
> > -       if (IS_ERR_OR_NULL(drm_class))
> > +       if (!drm_class_initialised)
> >                  return;
> >          drm_sysfs_acpi_unregister();
> > -       class_remove_file(drm_class, &class_attr_version.attr);
> > -       class_destroy(drm_class);
> > -       drm_class = NULL;
> > +       class_remove_file(&drm_class, &class_attr_version.attr);
> > +       class_destroy(&drm_class);
> > +       drm_class_initialised = false;
> >   }
> >
> >   static void drm_sysfs_release(struct device *dev)
> > @@ -372,7 +427,7 @@ int drm_sysfs_connector_add(struct drm_connector *connector)
> >                  return -ENOMEM;
> >
> >          device_initialize(kdev);
> > -       kdev->class = drm_class;
> > +       kdev->class = &drm_class;
> >          kdev->type = &drm_sysfs_device_connector;
> >          kdev->parent = dev->primary->kdev;
> >          kdev->groups = connector_dev_groups;
> > @@ -550,7 +605,7 @@ struct device *drm_sysfs_minor_alloc(struct drm_minor *minor)
> >                          minor_str = "card%d";
> >
> >                  kdev->devt = MKDEV(DRM_MAJOR, minor->index);
> > -               kdev->class = drm_class;
> > +               kdev->class = &drm_class;
> >                  kdev->type = &drm_sysfs_device_minor;
> >          }
> >
> > @@ -579,10 +634,10 @@ struct device *drm_sysfs_minor_alloc(struct drm_minor *minor)
> >    */
> >   int drm_class_device_register(struct device *dev)
> >   {
> > -       if (!drm_class || IS_ERR(drm_class))
> > +       if (!drm_class_initialised)
> >                  return -ENOENT;
> >
> > -       dev->class = drm_class;
> > +       dev->class = &drm_class;
> >          return device_register(dev);
> >   }
> >   EXPORT_SYMBOL_GPL(drm_class_device_register);
> >
> > base-commit: 45c734fdd43db14444025910b4c59dd2b8be714a
> > --
> > 2.44.0