Re: [PATCH 1/6] drm/debugfs: create device-centered debugfs functions

[Jani Nikula]

On Tue, 22 Nov 2022, Maíra Canal <mcanal@xxxxxxxxxx> wrote:
> 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 files to a device-managed list and, during
> drm_dev_register(), all added files will be created at once.
>
> Now, the drivers can use the functions drm_debugfs_add_file() and
> drm_debugfs_add_files() to create DRM debugfs files instead of using the
> drm_debugfs_create_files() function.
>
> Co-developed-by: Wambui Karuga <wambui.karugax@xxxxxxxxx>
> Signed-off-by: Wambui Karuga <wambui.karugax@xxxxxxxxx>
> Signed-off-by: Maíra Canal <mcanal@xxxxxxxxxx>
> ---
>  drivers/gpu/drm/drm_debugfs.c | 76 +++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_drv.c     |  3 ++
>  include/drm/drm_debugfs.h     | 45 +++++++++++++++++++++
>  include/drm/drm_device.h      | 15 +++++++
>  4 files changed, 139 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index ee445f4605ba..ca27c2b05051 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -38,6 +38,7 @@
>  #include <drm/drm_edid.h>
>  #include <drm/drm_file.h>
>  #include <drm/drm_gem.h>
> +#include <drm/drm_managed.h>
>  
>  #include "drm_crtc_internal.h"
>  #include "drm_internal.h"
> @@ -151,6 +152,21 @@ static int drm_debugfs_open(struct inode *inode, struct file *file)
>  	return single_open(file, node->info_ent->show, node);
>  }
>  
> +static int drm_debugfs_entry_open(struct inode *inode, struct file *file)
> +{
> +	struct drm_debugfs_entry *entry = inode->i_private;
> +	struct drm_debugfs_info *node = &entry->file;
> +
> +	return single_open(file, node->show, entry);
> +}
> +
> +static const struct file_operations drm_debugfs_entry_fops = {
> +	.owner = THIS_MODULE,
> +	.open = drm_debugfs_entry_open,
> +	.read = seq_read,
> +	.llseek = seq_lseek,
> +	.release = single_release,
> +};
>  
>  static const struct file_operations drm_debugfs_fops = {
>  	.owner = THIS_MODULE,
> @@ -207,6 +223,7 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>  		     struct dentry *root)
>  {
>  	struct drm_device *dev = minor->dev;
> +	struct drm_debugfs_entry *entry;
>  	char name[64];
>  
>  	INIT_LIST_HEAD(&minor->debugfs_list);
> @@ -230,6 +247,11 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>  	if (dev->driver->debugfs_init)
>  		dev->driver->debugfs_init(minor);
>  
> +	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_entry_fops);
> +	}
> +
>  	return 0;
>  }
>  
> @@ -281,6 +303,60 @@ void drm_debugfs_cleanup(struct drm_minor *minor)
>  	minor->debugfs_root = NULL;
>  }
>  
> +/**
> + * drm_debugfs_add_file - Add a given file to the DRM device debugfs file list
> + * @dev: drm device for the ioctl
> + * @name: debugfs file name
> + * @show: show callback
> + * @data: driver-private data, should not be device-specific
> + *
> + * Add a given file entry to the DRM device debugfs file list to be created on
> + * drm_debugfs_init.
> + */
> +int drm_debugfs_add_file(struct drm_device *dev, const char *name,
> +			 int (*show)(struct seq_file*, void*), void *data)
> +{
> +	struct drm_debugfs_entry *entry = drmm_kzalloc(dev, sizeof(*entry), GFP_KERNEL);
> +
> +	if (!entry)
> +		return -ENOMEM;
> +
> +	entry->file.name = name;
> +	entry->file.show = show;
> +	entry->file.data = data;
> +	entry->dev = dev;
> +
> +	mutex_lock(&dev->debugfs_mutex);
> +	list_add(&entry->list, &dev->debugfs_list);
> +	mutex_unlock(&dev->debugfs_mutex);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_debugfs_add_file);
> +
> +/**
> + * drm_debugfs_add_files - Add an array of files to the DRM device debugfs file list
> + * @dev: drm device for the ioctl
> + * @files: The array of files to create
> + * @count: The number of files given
> + *
> + * Add a given set of debugfs files represented by an array of
> + * &struct drm_debugfs_info in the DRM device debugfs file list.
> + */
> +int drm_debugfs_add_files(struct drm_device *dev, const struct drm_debugfs_info *files, int count)
> +{
> +	int i, ret = 0, err;
> +
> +	for (i = 0; i < count; i++) {
> +		err = drm_debugfs_add_file(dev, files[i].name, files[i].show, files[i].data);
> +		if (err)
> +			ret = err;
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_debugfs_add_files);

Do we want to add return values and error handling to debugfs related
functions at all?

BR,
Jani.


> +
>  static int connector_show(struct seq_file *m, void *data)
>  {
>  	struct drm_connector *connector = m->private;
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 8214a0b1ab7f..803942008fcb 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -575,6 +575,7 @@ static void drm_dev_init_release(struct drm_device *dev, void *res)
>  	mutex_destroy(&dev->clientlist_mutex);
>  	mutex_destroy(&dev->filelist_mutex);
>  	mutex_destroy(&dev->struct_mutex);
> +	mutex_destroy(&dev->debugfs_mutex);
>  	drm_legacy_destroy_members(dev);
>  }
>  
> @@ -608,12 +609,14 @@ static 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..c5684d6c5055 100644
> --- a/include/drm/drm_debugfs.h
> +++ b/include/drm/drm_debugfs.h
> @@ -79,12 +79,43 @@ struct drm_info_node {
>  	struct dentry *dent;
>  };
>  
> +/**
> + * struct drm_debugfs_info - debugfs info list entry
> + *
> + * This structure represents a debugfs file to be created by the drm
> + * core.
> + */
> +struct drm_debugfs_info {
> +	const char *name;
> +	int (*show)(struct seq_file*, void*);
> +	u32 driver_features;
> +	void *data;
> +};
> +
> +/**
> + * struct drm_debugfs_entry - Per-device debugfs node structure
> + *
> + * This structure represents a debugfs file, as an instantiation of a &struct
> + * drm_debugfs_info on a &struct drm_device.
> + */
> +struct drm_debugfs_entry {
> +	struct drm_device *dev;
> +	struct drm_debugfs_info file;
> +	struct list_head list;
> +};
> +
>  #if defined(CONFIG_DEBUG_FS)
>  void drm_debugfs_create_files(const struct drm_info_list *files,
>  			      int count, struct dentry *root,
>  			      struct drm_minor *minor);
>  int drm_debugfs_remove_files(const struct drm_info_list *files,
>  			     int count, struct drm_minor *minor);
> +
> +int drm_debugfs_add_file(struct drm_device *dev, const char *name,
> +			 int (*show)(struct seq_file*, void*), void *data);
> +
> +int drm_debugfs_add_files(struct drm_device *dev,
> +			  const struct drm_debugfs_info *files, int count);
>  #else
>  static inline void drm_debugfs_create_files(const struct drm_info_list *files,
>  					    int count, struct dentry *root,
> @@ -96,6 +127,20 @@ static inline int drm_debugfs_remove_files(const struct drm_info_list *files,
>  {
>  	return 0;
>  }
> +
> +static inline int drm_debugfs_add_file(struct drm_device *dev, const char *name,
> +				       int (*show)(struct seq_file*, void*),
> +				       void *data)
> +{
> +	return 0;
> +}
> +
> +static inline int drm_debugfs_add_files(struct drm_device *dev,
> +					const struct drm_debugfs_info *files,
> +					int count)
> +{
> +	return 0;
> +}
>  #endif
>  
>  #endif /* _DRM_DEBUGFS_H_ */
> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> index 9923c7a6885e..fa6af1d57929 100644
> --- a/include/drm/drm_device.h
> +++ b/include/drm/drm_device.h
> @@ -295,6 +295,21 @@ 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 be created by the DRM device. The files
> +	 * must be 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)

-- 
Jani Nikula, Intel Open Source Graphics Center