On Wed, 23 Nov 2022, Maíra Canal <mcanal@xxxxxxxxxx> wrote:
> On 11/23/22 08:59, Jani Nikula wrote:
>> On Wed, 23 Nov 2022, Maíra Canal <mcanal@xxxxxxxxxx> wrote:
>>> Hi Jani,
>>>
>>> On 11/23/22 08:06, Jani Nikula wrote:
>>>> 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(+)
>>>>>>> +/**
>>>>> + * 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/YWAmZdRwnAt6wh9B@xxxxxxxxx
>
> 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
