On Wed, Mar 22, 2017 at 09:54:01PM +0100, Daniel Vetter wrote:

I've decided to not document drm_debugfs_remove_files, it's on the way out.

The biggest part is a huge todo.rst entry with what all should be improved.

v2: Nits from Gabriel.

Cc: Gabriel Krisman Bertazi krisman@collabora.co.uk Reviewed-by: Gabriel Krisman Bertazi krisman@collabora.co.uk Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch

Merged up to this patch to drm-misc-next.

I will trade reviews for more reviews to help get the remaining in!

Thanks, Daniel

---

Documentation/gpu/drm-uapi.rst | 9 ++++++++ Documentation/gpu/todo.rst | 26 +++++++++++++++++++++ drivers/gpu/drm/drm_debugfs.c | 51 ++++++------------------------------------ drivers/gpu/drm/drm_internal.h | 2 +- include/drm/drm_debugfs.h | 38 +++++++++++++++++++++++++------ 5 files changed, 74 insertions(+), 52 deletions(-)

diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst index 369e8ca12b8e..76356c86e358 100644 --- a/Documentation/gpu/drm-uapi.rst +++ b/Documentation/gpu/drm-uapi.rst @@ -210,6 +210,15 @@ Display CRC Support .. kernel-doc:: drivers/gpu/drm/drm_debugfs_crc.c :export:

+Debugfs Support +---------------

+.. kernel-doc:: include/drm/drm_debugfs.h

• :internal:

+.. kernel-doc:: drivers/gpu/drm/drm_debugfs.c

• :export:

VBlank event handling

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst index 64e9d16170ce..0cdaddad2b9b 100644 --- a/Documentation/gpu/todo.rst +++ b/Documentation/gpu/todo.rst @@ -272,6 +272,32 @@ This is a really varied tasks with lots of little bits and pieces:

Contact: Daniel Vetter

+Clean up the debugfs support +----------------------------

+There's a bunch of issues with it:

+- The drm_info_list ->show() function doesn't even bother to cast to the drm

• structure for you. This is lazy.

+- We probably want to have some support for debugfs files on crtc/connectors and

• maybe other kms objects directly in core. There's even drm_print support in
• the funcs for these objects to dump kms state, so it's all there. And then the
• ->show() functions should obviously give you a pointer to the right object.

+- The drm_info_list stuff is centered on drm_minor instead of drm_device. For

• anything we want to print drm_device (or maybe drm_file) is the right thing.

+- The drm_driver->debugfs_init hooks we have is just an artifact of the old

• midlayered load sequence. DRM debugfs should work more like sysfs, where you
• can create properties/files for an object anytime you want, and the core
• takes care of publishing/unpuplishing all the files at register/unregister
• time. Drivers shouldn't need to worry about these technicalities, and fixing
• this (together with the drm_minor->drm_device move) would allow us to remove
• debugfs_init.

+Contact: Daniel Vetter

Better Testing

diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c index 4b02f4230562..c1807d5754b2 100644 --- a/drivers/gpu/drm/drm_debugfs.c +++ b/drivers/gpu/drm/drm_debugfs.c @@ -1,10 +1,3 @@ -/**

• • \file drm_debugfs.c
• • debugfs support for DRM
• • \author Ben Gamari bgamari@gmail.com
• */

/*

• Created: Sun Dec 21 13:08:50 2008 by bgamari@gmail.com

@@ -75,16 +68,15 @@ static const struct file_operations drm_debugfs_fops = {

/**

• • Initialize a given set of debugfs files for a device
• • \param files The array of files to create
• • \param count The number of files given
• • \param root DRI debugfs dir entry.
• • \param minor device minor number
• • \return Zero on success, non-zero on failure

• • drm_debugfs_create_files - Initialize a given set of debugfs files for DRM

• • 	minor
    

• • @files: The array of files to create
• • @count: The number of files given
• • @root: DRI debugfs dir entry.
• • @minor: device minor number
  • Create a given set of debugfs files represented by an array of

• • &drm_info_list in the given root directory. These files will be removed

• • &struct drm_info_list in the given root directory. These files will be removed
  • automatically on drm_debugfs_cleanup().
  */

int drm_debugfs_create_files(const struct drm_info_list *files, int count, @@ -133,17 +125,6 @@ int drm_debugfs_create_files(const struct drm_info_list *files, int count, } EXPORT_SYMBOL(drm_debugfs_create_files);

-/**

• • Initialize the DRI debugfs filesystem for a device
• • \param dev DRM device
• • \param minor device minor number
• • \param root DRI debugfs dir entry.
• • Create the DRI debugfs root entry "/sys/kernel/debug/dri", the device debugfs root entry
• • "/sys/kernel/debug/dri/%minor%/", and each entry in debugfs_list as
• • "/sys/kernel/debug/dri/%minor%/%name%".
• */

int drm_debugfs_init(struct drm_minor *minor, int minor_id, struct dentry *root) { @@ -189,16 +170,6 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id, }

-/**

• • Remove a list of debugfs files
• • \param files The list of files
• • \param count The number of files
• • \param minor The minor of which we should remove the files
• • \return always zero.
• • Remove all debugfs entries created by debugfs_init().
• */

int drm_debugfs_remove_files(const struct drm_info_list *files, int count, struct drm_minor *minor) { @@ -235,14 +206,6 @@ static void drm_debugfs_remove_all_files(struct drm_minor *minor) mutex_unlock(&minor->debugfs_lock); }

-/**

• • Cleanup the debugfs filesystem resources.
• • \param minor device minor number.
• • \return always zero.
• • Remove all debugfs entries created by debugfs_init().
• */

int drm_debugfs_cleanup(struct drm_minor *minor) { if (!minor->debugfs_root) diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h index 92ff4b9393b1..3d8e8f878924 100644 --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -100,7 +100,7 @@ int drm_gem_open_ioctl(struct drm_device *dev, void *data, void drm_gem_open(struct drm_device *dev, struct drm_file *file_private); void drm_gem_release(struct drm_device *dev, struct drm_file *file_private);

-/* drm_debugfs.c */ +/* drm_debugfs.c drm_debugfs_crc.c */ #if defined(CONFIG_DEBUG_FS) int drm_debugfs_init(struct drm_minor *minor, int minor_id, struct dentry *root); diff --git a/include/drm/drm_debugfs.h b/include/drm/drm_debugfs.h index 56924196d08d..ac0f75df1ac9 100644 --- a/include/drm/drm_debugfs.h +++ b/include/drm/drm_debugfs.h @@ -33,23 +33,47 @@ #define _DRM_DEBUGFS_H_

/**

• • Info file list entry. This structure represents a debugfs or proc file to
• • be created by the drm core

• • struct drm_info_list - debugfs info list entry
• • This structure represents a debugfs file to be created by the drm
• • core.
  */

struct drm_info_list {

• const char *name; /** file name */
• int (*show)(struct seq_file*, void*); /** show callback */
• u32 driver_features; /**< Required driver features for this entry */

• /** @name: file name */
• const char *name;
• /**

• * @show:
  

• *
  

• * Show callback. &seq_file->private will be set to the &struct
  

• * drm_info_node corresponding to the instance of this info on a given
  

• * &struct drm_minor.
  

• */
  

• int (*show)(struct seq_file*, void*);
• /** @driver_features: Required driver features for this entry */
• u32 driver_features;
• /** @data: Driver-private data, should not be device-specific. */ void *data;

};

/**

• • debugfs node structure. This structure represents a debugfs file.

• • struct drm_info_node - Per-minor debugfs node structure
• • This structure represents a debugfs file, as an instantiation of a &struct
• • drm_info_list on a &struct drm_minor.
• • FIXME:
• • No it doesn't make a hole lot of sense that we duplicate debugfs entries for
• • both the render and the primary nodes, but that's how this has organically
• • grown. It should probably be fixed, with a compatibility link, if needed.
  */

struct drm_info_node {

• struct list_head list;

• /** @minor: &struct drm_minor for this node. */ struct drm_minor *minor;
• /** @info_ent: template for this node. */ const struct drm_info_list *info_ent;
• /* private: */
• struct list_head list; struct dentry *dent;

};

-- 2.11.0