This series introduces the initial structure to make DRM debugfs more
device-centered and it is the first step to drop the
drm_driver->debugfs_init hooks in the future [1].
Currently, DRM debugfs files are created using drm_debugfs_create_files()
on request. The first patch of this series makes it possible for DRM devices
for creating debugfs files during drm_dev_register(). For it, it introduces
two new functions that can be used by the drivers: drm_debugfs_add_files()
and drm_debugfs_add_file(). The requests are added to a list and are created
all at once during drm_dev_register(). Moreover, the first patch was based on
this RFC series [2].
The main difference between the RFC series and the current series is the
creation of a new fops structure to accommodate the new structs and, also,
the creation of a new drm_debugfs_open. Moreover, the new series uses
device-managed allocation, returns memory allocation errors, and converts
more drivers to the new structure.
Apart from the first patch, the following patches are converting some drivers
to the new DRM debugfs structure and the last patch update the TODO task
related to it.
[1] https://cgit.freedesktop.org/drm/drm/tree/Documentation/gpu/todo.rst#n506
[2] https://lore.kernel.org/dri-devel/[email protected]/
Best Regards,
- Maíra Canal
Maíra Canal (6):
drm/debugfs: create device-centered debugfs functions
drm: use new debugfs device-centered functions on DRM core files
drm/vc4: use new debugfs device-centered functions
drm/v3d: use new debugfs device-centered functions
drm/vkms: use new debugfs device-centered functions
drm/todo: update the debugfs clean up task
Documentation/gpu/todo.rst | 9 +--
drivers/gpu/drm/drm_atomic.c | 11 ++--
drivers/gpu/drm/drm_client.c | 11 ++--
drivers/gpu/drm/drm_debugfs.c | 94 ++++++++++++++++++++++++---
drivers/gpu/drm/drm_drv.c | 3 +
drivers/gpu/drm/drm_framebuffer.c | 11 ++--
drivers/gpu/drm/drm_gem_vram_helper.c | 11 ++--
drivers/gpu/drm/v3d/v3d_debugfs.c | 22 +++----
drivers/gpu/drm/vc4/vc4_bo.c | 6 +-
drivers/gpu/drm/vc4/vc4_debugfs.c | 30 ++-------
drivers/gpu/drm/vc4/vc4_drv.c | 1 -
drivers/gpu/drm/vc4/vc4_drv.h | 16 -----
drivers/gpu/drm/vc4/vc4_hdmi.c | 6 +-
drivers/gpu/drm/vc4/vc4_hvs.c | 12 ++--
drivers/gpu/drm/vc4/vc4_v3d.c | 6 +-
drivers/gpu/drm/vkms/vkms_drv.c | 10 +--
include/drm/drm_debugfs.h | 45 +++++++++++++
include/drm/drm_device.h | 15 +++++
18 files changed, 203 insertions(+), 116 deletions(-)
--
2.38.1
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 <[email protected]>
Signed-off-by: Wambui Karuga <[email protected]>
Signed-off-by: Maíra Canal <[email protected]>
---
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);
+
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)
--
2.38.1
On Tue, 22 Nov 2022, Maíra Canal <[email protected]> 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 <[email protected]>
> Signed-off-by: Wambui Karuga <[email protected]>
> Signed-off-by: Maíra Canal <[email protected]>
> ---
> 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
Hi Jani,
On 11/23/22 08:06, Jani Nikula wrote:
> On Tue, 22 Nov 2022, Maíra Canal <[email protected]> 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 <[email protected]>
>> Signed-off-by: Wambui Karuga <[email protected]>
>> Signed-off-by: Maíra Canal <[email protected]>
>> ---
>> 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(+)
>>>> +/**
>> + * 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?
Drivers such as vc4 can use the return values from debugfs-related
functions for error handling. Although the return values are not
explicitly necessary, some drivers can benefit from them for error handling.
Best Regards,
- Maíra Canal
>
> 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)
>
On 11/23/22 08:59, Jani Nikula wrote:
> On Wed, 23 Nov 2022, Maíra Canal <[email protected]> wrote:
>> Hi Jani,
>>
>> On 11/23/22 08:06, Jani Nikula wrote:
>>> On Tue, 22 Nov 2022, Maíra Canal <[email protected]> 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 <[email protected]>
>>>> Signed-off-by: Wambui Karuga <[email protected]>
>>>> Signed-off-by: Maíra Canal <[email protected]>
>>>> ---
>>>> 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(+)
>>>>>> +/**
>>>> + * 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?
>>
>> Drivers such as vc4 can use the return values from debugfs-related
>> functions for error handling. Although the return values are not
>> explicitly necessary, some drivers can benefit from them for error handling.
>
> Arguably they should cease to do error handling on debugfs failures
> too. No driver should stop probe if debugfs fails, and that's been the
> direction for years.
Is it not even reasonable to return errors only to create drm_WARN_ON
when the creation of debugfs files fails? Currently, vc4 doesn't stop to
probe if debugfs fails, but only creates some warnings to let the user
knows that it failed.
Best Regards,
- Maíra Canal
>
> BR,
> Jani.
>
>>
>> Best Regards,
>> - Maíra Canal
>>
>>>
>>> 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)
>>>
>
On Wed, 23 Nov 2022, Maíra Canal <[email protected]> wrote:
> Hi Jani,
>
> On 11/23/22 08:06, Jani Nikula wrote:
>> On Tue, 22 Nov 2022, Maíra Canal <[email protected]> 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 <[email protected]>
>>> Signed-off-by: Wambui Karuga <[email protected]>
>>> Signed-off-by: Maíra Canal <[email protected]>
>>> ---
>>> 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(+)
>>>>> +/**
>>> + * 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?
>
> Drivers such as vc4 can use the return values from debugfs-related
> functions for error handling. Although the return values are not
> explicitly necessary, some drivers can benefit from them for error handling.
Arguably they should cease to do error handling on debugfs failures
too. No driver should stop probe if debugfs fails, and that's been the
direction for years.
BR,
Jani.
>
> Best Regards,
> - Maíra Canal
>
>>
>> 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
On Wed, 23 Nov 2022, Maíra Canal <[email protected]> wrote:
> On 11/23/22 08:59, Jani Nikula wrote:
>> On Wed, 23 Nov 2022, Maíra Canal <[email protected]> wrote:
>>> Hi Jani,
>>>
>>> On 11/23/22 08:06, Jani Nikula wrote:
>>>> On Tue, 22 Nov 2022, Maíra Canal <[email protected]> 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 <[email protected]>
>>>>> Signed-off-by: Wambui Karuga <[email protected]>
>>>>> Signed-off-by: Maíra Canal <[email protected]>
>>>>> ---
>>>>> 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(+)
>>>>>>> +/**
>>>>> + * 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?
>>>
>>> Drivers such as vc4 can use the return values from debugfs-related
>>> functions for error handling. Although the return values are not
>>> explicitly necessary, some drivers can benefit from them for error handling.
>>
>> Arguably they should cease to do error handling on debugfs failures
>> too. No driver should stop probe if debugfs fails, and that's been the
>> direction for years.
>
> Is it not even reasonable to return errors only to create drm_WARN_ON
> when the creation of debugfs files fails? Currently, vc4 doesn't stop to
> probe if debugfs fails, but only creates some warnings to let the user
> knows that it failed.
First, the only failure mode currently is -ENOMEM, for which you should
never warn or debug log anyway.
Second, Greg's been telling folks for years to not care about the
results of debugfs creation calls [1]. Sure, this is slightly different
in that it's preparation for making those calls, but for practical
purposes the end result is the same, right?
If there were other failure modes than -ENOMEM, I guess you could debug
log them from within drm_debugfs_add_file(), but what does the driver do
with the return code? Most drivers don't care anyway, and IMO they
shouldn't care.
BR,
Jani.
[1] https://lore.kernel.org/r/[email protected]
>
> Best Regards,
> - Maíra Canal
>
>>
>> BR,
>> Jani.
>>
>>>
>>> Best Regards,
>>> - Maíra Canal
>>>
>>>>
>>>> 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