On Wed, 13 May 2020, Thomas Zimmermann wrote: > Hi > > Am 13.05.20 um 13:41 schrieb Wambui Karuga: >> 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 file requests to a new list tied to drm_device. >> During drm_dev_register(), the new function drm_debugfs_create_file() >> will iterate over the list of added files on a given minor to create >> them. >> >> Two new structs are introduced in this change: struct drm_simple_info >> which represents a drm debugfs file entry and struct >> drm_simple_info_entry which is used to track file requests and is the >> main parameter of choice passed by functions. Each drm_simple_info_entry is >> added to the new struct drm_device->debugfs_list for file requests. >> >> Signed-off-by: Wambui Karuga >> --- >> drivers/gpu/drm/drm_debugfs.c | 59 ++++++++++++++++++++++++++++++++--- >> drivers/gpu/drm/drm_drv.c | 2 ++ >> include/drm/drm_debugfs.h | 38 ++++++++++++++++++++++ >> include/drm/drm_device.h | 12 +++++++ >> 4 files changed, 107 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c >> index 2bea22130703..03b0588ede68 100644 >> --- a/drivers/gpu/drm/drm_debugfs.c >> +++ b/drivers/gpu/drm/drm_debugfs.c >> @@ -145,9 +145,10 @@ static const struct drm_info_list drm_debugfs_list[] = { >> >> static int drm_debugfs_open(struct inode *inode, struct file *file) >> { >> - struct drm_info_node *node = inode->i_private; >> + struct drm_simple_info_entry *entry = inode->i_private; >> + struct drm_simple_info *node = &entry->file; >> >> - return single_open(file, node->info_ent->show, node); >> + return single_open(file, node->show_fn, entry); >> } >> >> >> @@ -159,6 +160,25 @@ static const struct file_operations drm_debugfs_fops = { >> .release = single_release, >> }; >> >> +/** >> + * drm_debugfs_create_file - create DRM debugfs file. >> + * @dev: drm_device that the file belongs to >> + * >> + * Create a DRM debugfs file from the list of files to be created >> + * from dev->debugfs_list. >> + */ >> +static void drm_debugfs_create_file(struct drm_minor *minor) > > This function creates several files. I'd rather call it > drm_debugfs_create_added_files(). > Okay, that makes sense. I can change that. >> +{ >> + struct drm_device *dev = minor->dev; >> + struct drm_simple_info_entry *entry; >> + >> + 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_fops); >> + } > > I think the created items should be removed from the list. That way, > drivers can call the function multiple times without recreating the same > files again. > Hadn't thought of that - I can try add that. >> +} >> >> /** >> * drm_debugfs_create_files - Initialize a given set of debugfs files for DRM >> @@ -213,8 +233,7 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id, >> sprintf(name, "%d", minor_id); >> minor->debugfs_root = debugfs_create_dir(name, root); >> >> - drm_debugfs_create_files(drm_debugfs_list, DRM_DEBUGFS_ENTRIES, >> - minor->debugfs_root, minor); > > By removing these two lines, aren't you losing the files listed in > DRM_DEBUGFS_ENTRIES? > Yes. When using the new functions, drm_debugfs_create_files() should not be called at this point, but for compatibility these two lines should be put back, I think. >> + drm_debugfs_create_file(minor); >> >> if (drm_drv_uses_atomic_modeset(dev)) { >> drm_atomic_debugfs_init(minor); >> @@ -449,4 +468,36 @@ void drm_debugfs_crtc_remove(struct drm_crtc *crtc) >> crtc->debugfs_entry = NULL; >> } >> >> +void drm_debugfs_add_file(struct drm_device *dev, const char *name, >> + drm_simple_show_t show_fn, void *data) >> +{ >> + struct drm_simple_info_entry *entry = >> + kzalloc(sizeof(*entry), GFP_KERNEL); >> + >> + if (!entry) >> + return; >> + >> + entry->file.name = name; >> + entry->file.show_fn = show_fn; >> + entry->file.data = data; >> + entry->dev = dev; >> + >> + mutex_lock(&dev->debugfs_mutex); >> + list_add(&entry->list, &dev->debugfs_list); >> + mutex_unlock(&dev->debugfs_mutex); >> +} >> +EXPORT_SYMBOL(drm_debugfs_add_file); >> + >> +void drm_debugfs_add_files(struct drm_device *dev, >> + const struct drm_simple_info *files, int count) >> +{ >> + int i; >> + >> + for (i = 0; i < count; i++) { >> + drm_debugfs_add_file(dev, files[i].name, files[i].show_fn, >> + files[i].data); >> + } >> +} >> +EXPORT_SYMBOL(drm_debugfs_add_files); >> + >> #endif /* CONFIG_DEBUG_FS */ >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c >> index bc38322f306e..c68df4e31aa0 100644 >> --- a/drivers/gpu/drm/drm_drv.c >> +++ b/drivers/gpu/drm/drm_drv.c >> @@ -646,12 +646,14 @@ 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..bbce580c3b38 100644 >> --- a/include/drm/drm_debugfs.h >> +++ b/include/drm/drm_debugfs.h >> @@ -34,6 +34,44 @@ >> >> #include >> #include >> + >> +struct drm_device; >> + >> +typedef int (*drm_simple_show_t)(struct seq_file *, void *); >> + >> +/** >> + * struct drm_simple_info - debugfs file entry >> + * >> + * This struct represents a debugfs file to be created. >> + */ >> +struct drm_simple_info { > > drm_simple_info and drm_simple_info_entry seem to be misnomers. They > should probably have some reference to debugfs in their name. > I'll change the names. Thanks, wambui karuga > Best regards > Thomas > > >> + const char *name; >> + drm_simple_show_t show_fn; >> + u32 driver_features; >> + void *data; >> +}; >> + >> +/** >> + * struct drm_simple_info_entry - debugfs list entry >> + * >> + * This struct is used in tracking requests for new debugfs files >> + * to be created. >> + */ >> +struct drm_simple_info_entry { >> + struct drm_device *dev; >> + struct drm_simple_info file; >> + struct list_head list; >> +}; >> + >> +void drm_debugfs_add_file(struct drm_device *dev, >> + const char *name, >> + drm_simple_show_t show_fn, >> + void *data); >> + >> +void drm_debugfs_add_files(struct drm_device *dev, >> + const struct drm_simple_info *files, >> + int count); >> + >> /** >> * struct drm_info_list - debugfs info list entry >> * >> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h >> index a55874db9dd4..b84dfdac27b7 100644 >> --- a/include/drm/drm_device.h >> +++ b/include/drm/drm_device.h >> @@ -326,6 +326,18 @@ 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 add. >> + * Files are 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) >> > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Felix Imendörffer > >