Re: [RFC PATCH 1/3] drm/debugfs: create debugfs files during drm_dev_register().

[Daniel Vetter]

On Wed, May 13, 2020 at 8:12 PM Wambui Karuga <wambui.karugax@xxxxxxxxx> wrote:
>
>
>
> On Wed, 13 May 2020, Thomas Zimmermann wrote:
>
> > Hi
> >
> > Am 13.05.20 um 13:41 schrieb Wambui Karuga:
> >> Introduce the ability to track requests for the addition of drm debugfs
> >> files at any time and have them added all at once during
> >> drm_dev_register().
> >>
> >> Drivers can add drm debugfs file requests to a new list tied to drm_device.
> >> During drm_dev_register(), the new function drm_debugfs_create_file()
> >> will iterate over the list of added files on a given minor to create
> >> them.
> >>
> >> Two new structs are introduced in this change: struct drm_simple_info
> >> which represents a drm debugfs file entry and struct
> >> drm_simple_info_entry which is used to track file requests and is the
> >> main parameter of choice passed by functions. Each drm_simple_info_entry is
> >> added to the new struct drm_device->debugfs_list for file requests.
> >>
> >> Signed-off-by: Wambui Karuga <wambui.karugax@xxxxxxxxx>
> >> ---
> >>  drivers/gpu/drm/drm_debugfs.c | 59 ++++++++++++++++++++++++++++++++---
> >>  drivers/gpu/drm/drm_drv.c     |  2 ++
> >>  include/drm/drm_debugfs.h     | 38 ++++++++++++++++++++++
> >>  include/drm/drm_device.h      | 12 +++++++
> >>  4 files changed, 107 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> >> index 2bea22130703..03b0588ede68 100644
> >> --- a/drivers/gpu/drm/drm_debugfs.c
> >> +++ b/drivers/gpu/drm/drm_debugfs.c
> >> @@ -145,9 +145,10 @@ static const struct drm_info_list drm_debugfs_list[] = {
> >>
> >>  static int drm_debugfs_open(struct inode *inode, struct file *file)
> >>  {
> >> -    struct drm_info_node *node = inode->i_private;
> >> +    struct drm_simple_info_entry *entry = inode->i_private;
> >> +    struct drm_simple_info *node = &entry->file;
> >>
> >> -    return single_open(file, node->info_ent->show, node);
> >> +    return single_open(file, node->show_fn, entry);
> >>  }
> >>
> >>
> >> @@ -159,6 +160,25 @@ static const struct file_operations drm_debugfs_fops = {
> >>      .release = single_release,
> >>  };
> >>
> >> +/**
> >> + * drm_debugfs_create_file - create DRM debugfs file.
> >> + * @dev: drm_device that the file belongs to
> >> + *
> >> + * Create a DRM debugfs file from the list of files to be created
> >> + * from dev->debugfs_list.
> >> + */
> >> +static void drm_debugfs_create_file(struct drm_minor *minor)
> >
> > This function creates several files. I'd rather call it
> > drm_debugfs_create_added_files().
> >
> Okay, that makes sense. I can change that.
>
> >> +{
> >> +    struct drm_device *dev = minor->dev;
> >> +    struct drm_simple_info_entry *entry;
> >> +
> >> +    list_for_each_entry(entry, &dev->debugfs_list, list) {
> >> +            debugfs_create_file(entry->file.name,
> >> +                                S_IFREG | S_IRUGO, minor->debugfs_root,
> >> +                                entry,
> >> +                                &drm_debugfs_fops);
> >> +    }
> >
> > I think the created items should be removed from the list. That way,
> > drivers can call the function multiple times without recreating the same
> > files again.
> >
> Hadn't thought of that - I can try add that.

The function here is static, called once by the core. So no need for
complicated logic, it's guaranteed to only be called once.

I guess what confused Thomas is the kerneldoc. We generally only do
that for functions exported to drivers, that drivers should call. Not
the case here. The function name itself is descriptive enough I think
(with Thomas' suggestion even better). So I'd just remove the
kerneldoc here.
-Daniel

> >> +}
> >>
> >>  /**
> >>   * drm_debugfs_create_files - Initialize a given set of debugfs files for DRM
> >> @@ -213,8 +233,7 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
> >>      sprintf(name, "%d", minor_id);
> >>      minor->debugfs_root = debugfs_create_dir(name, root);
> >>
> >> -    drm_debugfs_create_files(drm_debugfs_list, DRM_DEBUGFS_ENTRIES,
> >> -                             minor->debugfs_root, minor);
> >
> > By removing these two lines, aren't you losing the files listed in
> > DRM_DEBUGFS_ENTRIES?
> >
> Yes.
> When using the new functions, drm_debugfs_create_files() should not
> be called at this point, but for compatibility these two lines should
> be put back, I think.
>
> >> +    drm_debugfs_create_file(minor);
> >>
> >>      if (drm_drv_uses_atomic_modeset(dev)) {
> >>              drm_atomic_debugfs_init(minor);
> >> @@ -449,4 +468,36 @@ void drm_debugfs_crtc_remove(struct drm_crtc *crtc)
> >>      crtc->debugfs_entry = NULL;
> >>  }
> >>
> >> +void drm_debugfs_add_file(struct drm_device *dev, const char *name,
> >> +                      drm_simple_show_t show_fn, void *data)
> >> +{
> >> +    struct drm_simple_info_entry *entry =
> >> +            kzalloc(sizeof(*entry), GFP_KERNEL);
> >> +
> >> +    if (!entry)
> >> +            return;
> >> +
> >> +    entry->file.name = name;
> >> +    entry->file.show_fn = show_fn;
> >> +    entry->file.data = data;
> >> +    entry->dev = dev;
> >> +
> >> +    mutex_lock(&dev->debugfs_mutex);
> >> +    list_add(&entry->list, &dev->debugfs_list);
> >> +    mutex_unlock(&dev->debugfs_mutex);
> >> +}
> >> +EXPORT_SYMBOL(drm_debugfs_add_file);
> >> +
> >> +void drm_debugfs_add_files(struct drm_device *dev,
> >> +                       const struct drm_simple_info *files, int count)
> >> +{
> >> +    int i;
> >> +
> >> +    for (i = 0; i < count; i++) {
> >> +            drm_debugfs_add_file(dev, files[i].name, files[i].show_fn,
> >> +                                 files[i].data);
> >> +    }
> >> +}
> >> +EXPORT_SYMBOL(drm_debugfs_add_files);
> >> +
> >>  #endif /* CONFIG_DEBUG_FS */
> >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> >> index bc38322f306e..c68df4e31aa0 100644
> >> --- a/drivers/gpu/drm/drm_drv.c
> >> +++ b/drivers/gpu/drm/drm_drv.c
> >> @@ -646,12 +646,14 @@ int drm_dev_init(struct drm_device *dev,
> >>      INIT_LIST_HEAD(&dev->filelist_internal);
> >>      INIT_LIST_HEAD(&dev->clientlist);
> >>      INIT_LIST_HEAD(&dev->vblank_event_list);
> >> +    INIT_LIST_HEAD(&dev->debugfs_list);
> >>
> >>      spin_lock_init(&dev->event_lock);
> >>      mutex_init(&dev->struct_mutex);
> >>      mutex_init(&dev->filelist_mutex);
> >>      mutex_init(&dev->clientlist_mutex);
> >>      mutex_init(&dev->master_mutex);
> >> +    mutex_init(&dev->debugfs_mutex);
> >>
> >>      ret = drmm_add_action(dev, drm_dev_init_release, NULL);
> >>      if (ret)
> >> diff --git a/include/drm/drm_debugfs.h b/include/drm/drm_debugfs.h
> >> index 2188dc83957f..bbce580c3b38 100644
> >> --- a/include/drm/drm_debugfs.h
> >> +++ b/include/drm/drm_debugfs.h
> >> @@ -34,6 +34,44 @@
> >>
> >>  #include <linux/types.h>
> >>  #include <linux/seq_file.h>
> >> +
> >> +struct drm_device;
> >> +
> >> +typedef int (*drm_simple_show_t)(struct seq_file *, void *);
> >> +
> >> +/**
> >> + * struct drm_simple_info - debugfs file entry
> >> + *
> >> + * This struct represents a debugfs file to be created.
> >> + */
> >> +struct drm_simple_info {
> >
> > drm_simple_info and drm_simple_info_entry seem to be misnomers. They
> > should probably have some reference to debugfs in their name.
> >
> I'll change the names.
>
> Thanks,
> wambui karuga
> > Best regards
> > Thomas
> >
> >
> >> +    const char *name;
> >> +    drm_simple_show_t show_fn;
> >> +    u32 driver_features;
> >> +    void *data;
> >> +};
> >> +
> >> +/**
> >> + * struct drm_simple_info_entry - debugfs list entry
> >> + *
> >> + * This struct is used in tracking requests for new debugfs files
> >> + * to be created.
> >> + */
> >> +struct drm_simple_info_entry {
> >> +    struct drm_device *dev;
> >> +    struct drm_simple_info file;
> >> +    struct list_head list;
> >> +};
> >> +
> >> +void drm_debugfs_add_file(struct drm_device *dev,
> >> +                      const char *name,
> >> +                      drm_simple_show_t show_fn,
> >> +                      void *data);
> >> +
> >> +void drm_debugfs_add_files(struct drm_device *dev,
> >> +                       const struct drm_simple_info *files,
> >> +                       int count);
> >> +
> >>  /**
> >>   * struct drm_info_list - debugfs info list entry
> >>   *
> >> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> >> index a55874db9dd4..b84dfdac27b7 100644
> >> --- a/include/drm/drm_device.h
> >> +++ b/include/drm/drm_device.h
> >> @@ -326,6 +326,18 @@ struct drm_device {
> >>       */
> >>      struct drm_fb_helper *fb_helper;
> >>
> >> +    /**
> >> +     * @debugfs_mutex:
> >> +     * Protects debugfs_list access.
> >> +     */
> >> +    struct mutex debugfs_mutex;
> >> +
> >> +    /** @debugfs_list:
> >> +     * List of debugfs files to add.
> >> +     * Files are added during drm_dev_register().
> >> +     */
> >> +    struct list_head debugfs_list;
> >> +
> >>      /* Everything below here is for legacy driver, never use! */
> >>      /* private: */
> >>  #if IS_ENABLED(CONFIG_DRM_LEGACY)
> >>
> >
> > --
> > Thomas Zimmermann
> > Graphics Driver Developer
> > SUSE Software Solutions Germany GmbH
> > Maxfeldstr. 5, 90409 NÃrnberg, Germany
> > (HRB 36809, AG NÃrnberg)
> > GeschÃftsfÃhrer: Felix ImendÃrffer
> >
> >



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch