| Message ID | 20230116102815.95063-3-mcanal@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/debugfs: Make the debugfs structure more generic | expand |
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: */
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; + /** * 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; /* Everything below here is for legacy driver, never use! */ /* private: */
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(-)