Re: [PATCH 2/6] drm/debugfs: Make drm_device use the struct drm_debugfs_list

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 16 Jan 2023, Maíra Canal <mcanal@igalia.com> wrote:
> The struct drm_debugfs_list encapsulates all the debugfs-related
> objects, so that they can be initialized and destroyed with two helpers.
> Therefore, make the struct drm_device use the struct drm_debugfs_list
> instead of instantiating the debugfs list and mutex separated.
>
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>  drivers/gpu/drm/drm_debugfs.c | 10 +++++-----
>  drivers/gpu/drm/drm_drv.c     |  7 ++++---
>  include/drm/drm_debugfs.h     |  3 +++
>  include/drm/drm_device.h      | 10 ++--------
>  4 files changed, 14 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index 2f104a9e4276..176b0f8614e5 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -256,7 +256,7 @@ 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_safe(entry, tmp, &dev->debugfs_list, list) {
> +	list_for_each_entry_safe(entry, tmp, &dev->debugfs_list.list, list) {
>  		debugfs_create_file(entry->file.name, 0444,
>  				    minor->debugfs_root, entry, &drm_debugfs_entry_fops);
>  		list_del(&entry->list);
> @@ -273,7 +273,7 @@ void drm_debugfs_late_register(struct drm_device *dev)
>  	if (!minor)
>  		return;
>  
> -	list_for_each_entry_safe(entry, tmp, &dev->debugfs_list, list) {
> +	list_for_each_entry_safe(entry, tmp, &dev->debugfs_list.list, list) {
>  		debugfs_create_file(entry->file.name, 0444,
>  				    minor->debugfs_root, entry, &drm_debugfs_entry_fops);
>  		list_del(&entry->list);
> @@ -350,9 +350,9 @@ void drm_debugfs_add_file(struct drm_device *dev, const char *name,
>  	entry->file.data = data;
>  	entry->dev = dev;
>  
> -	mutex_lock(&dev->debugfs_mutex);
> -	list_add(&entry->list, &dev->debugfs_list);
> -	mutex_unlock(&dev->debugfs_mutex);
> +	mutex_lock(&dev->debugfs_list.mutex);
> +	list_add(&entry->list, &dev->debugfs_list.list);
> +	mutex_unlock(&dev->debugfs_list.mutex);
>  }
>  EXPORT_SYMBOL(drm_debugfs_add_file);
>  
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 11748dd513c3..89c63ead8653 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -38,6 +38,7 @@
>  #include <drm/drm_cache.h>
>  #include <drm/drm_client.h>
>  #include <drm/drm_color_mgmt.h>
> +#include <drm/drm_debugfs.h>
>  #include <drm/drm_drv.h>
>  #include <drm/drm_file.h>
>  #include <drm/drm_managed.h>
> @@ -575,7 +576,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_debugfs_list_destroy(&dev->debugfs_list);
>  	drm_legacy_destroy_members(dev);
>  }
>  
> @@ -609,14 +610,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);
> +
> +	drm_debugfs_list_init(&dev->debugfs_list);
>  
>  	ret = drmm_add_action_or_reset(dev, drm_dev_init_release, NULL);
>  	if (ret)
> diff --git a/include/drm/drm_debugfs.h b/include/drm/drm_debugfs.h
> index 8658e97a88cf..b4e22e7d4016 100644
> --- a/include/drm/drm_debugfs.h
> +++ b/include/drm/drm_debugfs.h
> @@ -36,6 +36,9 @@
>  #include <linux/mutex.h>
>  #include <linux/types.h>
>  #include <linux/seq_file.h>
> +
> +struct drm_device;
> +

Seems unrelated to this commit.

>  /**
>   * struct drm_info_list - debugfs info list entry
>   *
> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> index 282a171164ee..6ce10f9c7bae 100644
> --- a/include/drm/drm_device.h
> +++ b/include/drm/drm_device.h
> @@ -6,6 +6,7 @@
>  #include <linux/mutex.h>
>  #include <linux/idr.h>
>  
> +#include <drm/drm_debugfs.h>
>  #include <drm/drm_legacy.h>
>  #include <drm/drm_mode_config.h>
>  
> @@ -308,20 +309,13 @@ 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;
> +	struct drm_debugfs_list debugfs_list;

I was kind of thinking this would be a pointer, and struct
drm_debugfs_list would be an opaque type, with the definition inside
drm_debugfs.c. Nobody else needs to know the guts of it.

Plus it helps fight the header dependency complexity by letting the type
be a forward declaration here.

I also think "list" in the name exposes an implementation detail for no
good reason, when you have a chance to hide it. The users don't need to
know it's a list. Also, if we end up adding more things to it later, do
we want to rename everything then, or add things to a structure whose
name no longer describes what it contains?

Daniel, your thoughts?


BR,
Jani.



>  
>  	/* Everything below here is for legacy driver, never use! */
>  	/* private: */

-- 
Jani Nikula, Intel Open Source Graphics Center