On Mon, 16 Jan 2023, Maíra Canal <mcanal@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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
